Copilot commented on code in PR #45298:
URL: https://github.com/apache/superset/pull/45298#discussion_r4328270058
##########
superset-frontend/src/types/Dashboard.ts:
##########
@@ -24,6 +24,7 @@ export interface Dashboard {
slug?: string | null;
url: string;
dashboard_title: string;
+ description?: string | null;
Review Comment:
`description` is now required for correctness in the `useDashboard`
hydration path (explicit `null` prevents inheriting a previous dashboard’s
value), but the type is still optional. Consider making this `description:
string | null` (non-optional) in `Dashboard` (and aligning any corresponding
state types where applicable) so future changes that accidentally omit the
field are caught at compile time. If there are legitimate call sites
constructing partial `Dashboard` objects, an alternative is to introduce a
separate `DashboardFromApi` type (required `description`) vs. a looser
partial/editor type.
##########
superset-frontend/src/hooks/apiResources/dashboards.test.ts:
##########
@@ -45,6 +46,53 @@ test('useDashboard excludes thumbnail_url from request',
async () => {
fetchMock.clearHistory().removeRoutes();
});
+test('useDashboard returns the saved description of each dashboard', async ()
=> {
+ const dashboards: Record<string, Record<string, unknown>> = {
+ '1': {
+ id: 1,
+ dashboard_title: 'Dashboard A',
+ description: 'Description for A',
+ json_metadata: '{}',
+ position_json: '{}',
+ },
+ '2': {
+ id: 2,
+ dashboard_title: 'Dashboard B',
+ description: null,
+ json_metadata: '{}',
+ position_json: '{}',
+ },
+ };
+ // Emulates the API honoring the requested `columns`: only listed fields
+ // are present in the response, as with the real endpoint.
+ fetchMock.get('glob:*/api/v1/dashboard/*', ({ url }) => {
+ const { pathname, searchParams } = new URL(url, 'http://localhost');
+ const dashboard = dashboards[pathname.split('/').pop() as string];
+ const { columns } = rison.decode<{ columns: string[] }>(
+ searchParams.get('q') as string,
+ );
+ return {
+ result: Object.fromEntries(
+ Object.entries(dashboard).filter(([key]) => columns.includes(key)),
+ ),
+ };
+ });
Review Comment:
The fetch-mock matcher `'glob:*/api/v1/dashboard/*'` is broad enough to also
match nested dashboard endpoints (e.g., `/api/v1/dashboard/<id>/datasets`)
within the same test file, which can make tests fragile if additional requests
are added later. Prefer a more specific route matcher that targets only the
single-dashboard GET (e.g., an `express:`/regex matcher for
`/api/v1/dashboard/:id` without extra path segments, optionally ensuring the
presence of `?q=`), so this stub can’t accidentally intercept other
dashboard-related calls.
--
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]
