bito-code-review[bot] commented on code in PR #40679:
URL: https://github.com/apache/superset/pull/40679#discussion_r3877192158
##########
superset-frontend/src/views/CRUD/types.ts:
##########
@@ -61,7 +61,11 @@ export interface Dashboard {
changed_on_utc?: string;
changed_by: string;
dashboard_title: string;
+ // Title resolved for the viewer's locale (read-only); falls back to
+ // dashboard_title. Present only when asset-metadata translation is enabled.
+ localized_title?: string;
slice_name?: string;
+ localized_name?: string;
Review Comment:
<!-- Bito Reply -->
The user's assessment is correct: the `localized_name` field is indeed
appropriate for the `Dashboard` interface because the home activity list uses a
unified `ActivityObject` union that includes both chart and dashboard entities.
The reviewer's concern about `localized_name` being a chart-specific field is
understandable given its usage in other parts of the codebase, but in the
context of the home activity list, it is consistent with the existing
`slice_name` field. The addition of `localized_title` and `localized_name` to
the `Dashboard` interface correctly supports the asset-metadata translation
feature.
**superset-frontend/src/views/CRUD/types.ts**
```
dashboard_title: string;
+ // Title resolved for the viewer's locale (read-only); falls back to
+ // dashboard_title. Present only when asset-metadata translation is
enabled.
+ localized_title?: string;
slice_name?: string;
+ localized_name?: string;
```
##########
superset-frontend/src/pages/ChartList/ChartList.test.tsx:
##########
@@ -273,6 +273,34 @@ describe('ChartList', () => {
expect(screen.queryByTestId('delete-modal-input')).not.toBeInTheDocument();
});
+
+ test('renders the localized chart name, falling back to slice_name', async
() => {
+ // Served from a response of its own rather than the shared mock, which
+ // other suites assert against by canonical name.
+ const [translated, untranslated] = mockCharts;
+ fetchMock.removeRoutes();
Review Comment:
<!-- Bito Reply -->
The decision to retain `removeRoutes()` is reasonable given the observed
side effects of the alternative approach. The behavior described indicates that
the catch-all route is interfering with the specific route registration order,
and maintaining the full reset ensures the test environment remains isolated as
intended. The comparison to the existing test at line 220 is also noted,
confirming that the current implementation aligns with established patterns in
the test suite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
