Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ export const FaveStar = ({
}: FaveStarProps) => {
const theme = useTheme();
useEffect(() => {
fetchFaveStar?.(itemId);
// Only attempt to fetch favorite status if itemId is valid
// This prevents unnecessary API calls for deleted or invalid items
if (itemId && fetchFaveStar) {
fetchFaveStar(itemId);
}
}, [fetchFaveStar, itemId]);

const onClick = useCallback(
Expand Down
20 changes: 17 additions & 3 deletions superset-frontend/src/dashboard/actions/dashboardState.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,29 @@ export function fetchFaveStar(id) {
.then(({ json }) => {
dispatch(toggleFaveStar(!!json?.result?.[0]?.value));
})
.catch(() =>
.catch(async error => {
const errorObject = await getClientErrorObject(error);

Copilot AI Jan 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s trailing whitespace on the blank line after getClientErrorObject(error); (shown in the diff). Please remove it to avoid failing lint rules like no-trailing-spaces / prettier formatting checks.

Suggested change

Copilot uses AI. Check for mistakes.
// HTTP 404: Dashboard no longer exists (expected case after deletion)
// This is a normal scenario when navigating after a dashboard has been deleted.
// Skipping error toast to prevent user-facing errors for expected behavior.
if (errorObject.status === 404) {
logging.debug(
`Favorite status fetch returned 404 for dashboard ID ${id}. ` +
`Dashboard may have been deleted. Skipping error notification.`
);
return;
}

// All other errors are unexpected and should be reported to the user
dispatch(
addDangerToast(
t(
'There was an issue fetching the favorite status of this dashboard.',
),
),
),
);
);
});
};
}

Expand Down
59 changes: 59 additions & 0 deletions superset-frontend/src/dashboard/actions/dashboardState.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import {
SAVE_DASHBOARD_STARTED,
saveDashboardRequest,
SET_OVERRIDE_CONFIRM,
fetchFaveStar,
TOGGLE_FAVE_STAR,
} from 'src/dashboard/actions/dashboardState';
import { UPDATE_COMPONENTS_PARENTS_LIST } from 'src/dashboard/actions/dashboardLayout';
import {
Expand Down Expand Up @@ -234,4 +236,61 @@ describe('dashboardState actions', () => {
);
});
});

// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('fetchFaveStar', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing global test function definitions in scope

The new test code uses describe, test, and expect functions without importing them. These are Jest globals that need to be available in the test file. Add the necessary imports or ensure Jest globals are properly configured in your test setup.

Code suggestion
Check the AI-generated fix before applying
 @@ -24,6 +24,7 @@
    SAVE_DASHBOARD_STARTED,
    saveDashboardRequest,
    SET_OVERRIDE_CONFIRM,
 +  fetchFaveStar,
 +  TOGGLE_FAVE_STAR,
  } from 'src/dashboard/actions/dashboardState';
  import { UPDATE_COMPONENTS_PARENTS_LIST } from 'src/dashboard/actions/dashboardLayout';
  import {
 @@ -1,3 +1,5 @@
 +import { describe, test, expect } from '@jest/globals';
 +
  import {
    SAVE_DASHBOARD_STARTED,

Code Review Run #8471c0


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

test('dispatches TOGGLE_FAVE_STAR on successful fetch', async () => {
const dashboardId = 123;
const dispatch = sinon.stub();

getStub.restore();
getStub = sinon.stub(SupersetClient, 'get').resolves({
json: {
result: [{ id: dashboardId, value: true }],
},
});

const thunk = fetchFaveStar(dashboardId);
await thunk(dispatch);

await waitFor(() => expect(dispatch.callCount).toBe(1));
expect(dispatch.getCall(0).args[0].type).toBe(TOGGLE_FAVE_STAR);
expect(dispatch.getCall(0).args[0].isStarred).toBe(true);
});

test('does not dispatch error toast on 404 response', async () => {
const dashboardId = 999;
const dispatch = sinon.stub();

getStub.restore();
const error404 = new Error('Not found');
error404.status = 404;
getStub = sinon.stub(SupersetClient, 'get').rejects(error404);

Copilot AI Jan 28, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mocked 404 error here (new Error() with a status field) will not be interpreted as a 404 by getClientErrorObject (it only carries status through when the rejection is a Response-like object, e.g. { response: { status, statusText, bodyUsed } }). As a result, this test won’t actually exercise the 404-suppression path and will likely dispatch the danger toast instead. Update the mock rejection to match the shape SupersetClient/getClientErrorObject expects (or mock getClientErrorObject directly).

Suggested change
const error404 = new Error('Not found');
error404.status = 404;
getStub = sinon.stub(SupersetClient, 'get').rejects(error404);
getStub = sinon
.stub(SupersetClient, 'get')
.rejects({
response: {
status: 404,
statusText: 'Not Found',
bodyUsed: false,
},
});

Copilot uses AI. Check for mistakes.

Comment on lines +266 to +275

Copilot AI Jan 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mocked 404 failure here rejects with a plain Error that has a status field, but fetchFaveStar now calls getClientErrorObject(error) and checks errorObject.status. getClientErrorObject does not read status off a generic Error, so errorObject.status will be undefined and this test will fail (it will dispatch the danger toast instead of skipping). Update the stub to reject with the same shape SupersetClient uses (e.g., { response: new Response(..., { status: 404 }) }) so getClientErrorObject can derive status correctly.

Copilot uses AI. Check for mistakes.
const thunk = fetchFaveStar(dashboardId);
await thunk(dispatch);

// Should not dispatch any action (no toast, no toggle)
expect(dispatch.callCount).toBe(0);
});

test('dispatches error toast on non-404 errors', async () => {
const dashboardId = 123;
const dispatch = sinon.stub();

getStub.restore();
const error500 = new Error('Internal server error');
error500.status = 500;
getStub = sinon.stub(SupersetClient, 'get').rejects(error500);
Comment on lines +287 to +295

Copilot AI Jan 28, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same mocking issue as the 404 test: rejecting with a plain Error (even with status = 500) won’t reliably propagate status through getClientErrorObject. To ensure this test is validating the intended non-404 branch, reject with a Response-like object that includes status/statusText (or mock getClientErrorObject).

Copilot uses AI. Check for mistakes.
Comment on lines +287 to +295

Copilot AI Jan 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the 404 case: this test rejects with a plain Error and sets error500.status, but getClientErrorObject won’t propagate that into errorObject.status. This makes the test less representative of real SupersetClient failures and can cause unexpected behavior. Prefer rejecting with { response: new Response(..., { status: 500 }) } (or whatever SupersetClient actually rejects with) so the thunk exercises the same parsing path as production.

Copilot uses AI. Check for mistakes.

const thunk = fetchFaveStar(dashboardId);
await thunk(dispatch);

await waitFor(() => expect(dispatch.callCount).toBe(1));
// Verify error toast action was dispatched (ADD_TOAST with danger type)
const dispatchedAction = dispatch.getCall(0).args[0];
expect(dispatchedAction.type).toBe('ADD_TOAST');
expect(dispatchedAction.payload.toastType).toBe('danger');

Copilot AI Jan 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the rest of the codebase, avoid asserting on the raw string 'ADD_TOAST' and instead import and use the ADD_TOAST constant from src/components/MessageToasts/actions (e.g. as done in src/dashboard/actions/dashboardLayout.test.js:380). This reduces brittleness if the action type ever changes.

Copilot uses AI. Check for mistakes.
});
});
});
7 changes: 6 additions & 1 deletion superset-frontend/src/dashboard/components/Header/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,12 @@ const Header = () => {
const faveStarProps = useMemo(
() => ({
itemId: dashboardInfo.id,
fetchFaveStar: boundActionCreators.fetchFaveStar,
// Only fetch favorite status for valid, existing dashboards
// Skip fetch entirely if dashboard ID is falsy to prevent
// 404 errors when navigating after dashboard deletion

Copilot AI Jan 28, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment suggests a falsy dashboard ID would lead to 404s, but a 404 implies a truthy ID that no longer exists. Consider rewording this comment to reflect the actual scenario (skip fetch when there is no ID / component is unmounted), and rely on the 404 handling in fetchFaveStar for deleted/stale IDs.

Suggested change
// Only fetch favorite status for valid, existing dashboards
// Skip fetch entirely if dashboard ID is falsy to prevent
// 404 errors when navigating after dashboard deletion
// Only attempt to fetch favorite status when there's a valid dashboard ID.
// When the ID is falsy (e.g., dashboard not yet created or component unmounted),
// skip the fetch and rely on fetchFaveStar to handle 404s for deleted/stale IDs.

Copilot uses AI. Check for mistakes.
fetchFaveStar: dashboardInfo.id
? boundActionCreators.fetchFaveStar
: undefined,
saveFaveStar: boundActionCreators.saveFaveStar,
isStarred,
showTooltip: true,
Expand Down
Loading