fix(dashboard): handle favorite fetch for deleted dashboards #37497
Conversation
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #8471c0
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/actions/dashboardState.test.js - 1
- Missing global test function definitions in scope · Line 241-241
Review Details
-
Files reviewed - 4 · Commit Range:
54d2779..54d2779- superset-frontend/packages/superset-ui-core/src/components/FaveStar/index.tsx
- superset-frontend/src/dashboard/actions/dashboardState.js
- superset-frontend/src/dashboard/actions/dashboardState.test.js
- superset-frontend/src/dashboard/components/Header/index.jsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| }); | ||
|
|
||
| // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks | ||
| describe('fetchFaveStar', () => { |
There was a problem hiding this comment.
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
|
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
There was a problem hiding this comment.
Pull request overview
Fixes spurious “favorite status” error toasts that can occur after deleting a dashboard by treating 404s from the favorite-status endpoint as an expected outcome and by adding guards to avoid unnecessary favorite-status fetches.
Changes:
- Suppress the danger toast for expected 404 responses when fetching dashboard favorite status.
- Guard favorite-status fetch calls when the relevant dashboard/item ID is not available.
- Add regression tests covering success, 404 suppression, and non-404 error behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| superset-frontend/src/dashboard/components/Header/index.jsx | Avoids passing a fetch handler when the dashboard id is falsy. |
| superset-frontend/src/dashboard/actions/dashboardState.js | Handles 404 from favorite-status fetch as expected (no toast) while keeping toast behavior for other failures. |
| superset-frontend/src/dashboard/actions/dashboardState.test.js | Adds tests for fetchFaveStar success/404/non-404 scenarios. |
| superset-frontend/packages/superset-ui-core/src/components/FaveStar/index.tsx | Prevents calling fetchFaveStar when itemId is not valid at runtime. |
| const error404 = new Error('Not found'); | ||
| error404.status = 404; | ||
| getStub = sinon.stub(SupersetClient, 'get').rejects(error404); |
There was a problem hiding this comment.
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).
| 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, | |
| }, | |
| }); |
| getStub.restore(); | ||
| const error500 = new Error('Internal server error'); | ||
| error500.status = 500; | ||
| getStub = sinon.stub(SupersetClient, 'get').rejects(error500); |
There was a problem hiding this comment.
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).
| // 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 |
There was a problem hiding this comment.
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.
| // 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. |
| getStub.restore(); | ||
| const error404 = new Error('Not found'); | ||
| error404.status = 404; | ||
| getStub = sinon.stub(SupersetClient, 'get').rejects(error404); | ||
|
|
There was a problem hiding this comment.
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.
| getStub.restore(); | ||
| const error500 = new Error('Internal server error'); | ||
| error500.status = 500; | ||
| getStub = sinon.stub(SupersetClient, 'get').rejects(error500); |
There was a problem hiding this comment.
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.
| expect(dispatchedAction.type).toBe('ADD_TOAST'); | ||
| expect(dispatchedAction.payload.toastType).toBe('danger'); |
There was a problem hiding this comment.
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.
| .catch(() => | ||
| .catch(async error => { | ||
| const errorObject = await getClientErrorObject(error); | ||
|
|
There was a problem hiding this comment.
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.
|
Thanks for the review! |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| status: 404, | ||
| statusText: 'Not Found', | ||
| bodyUsed: false, | ||
| }, |
There was a problem hiding this comment.
Suggestion: The mocked 404 error object lacks a Response-like json() method; the production helper getClientErrorObject may attempt to call response.json() on the rejected value and throw a TypeError, causing the test to fail for the wrong reason — include a json async method on the mocked response so it behaves like a real fetch Response. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Unit test 'does not dispatch error toast on 404 response' affected.
- ⚠️ CI dashboard tests may fail with parsing TypeError.| }, | |
| json: async () => ({ message: 'Not Found' }), |
Steps of Reproduction ✅
1. Run the unit test "does not dispatch error toast on 404 response" in
superset-frontend/src/dashboard/actions/dashboardState.test.js (test block
starts at the fetchFaveStar tests around lines 262-281). The test file
stubs SupersetClient.get to reject with the object defined at
lines 266-274 (the error404 object).
2. The test calls fetchFaveStar(dashboardId) (thunk defined in
src/dashboard/actions/dashboardState.js: export function fetchFaveStar(id)
— see the implementation at lines ~108-140) which executes SupersetClient.get
and hits the .catch branch.
3. Inside fetchFaveStar catch, the code invokes getClientErrorObject(error)
(see src/dashboard/actions/dashboardState.js catch at ~lines 128-136),
which in the real client code may call response.json() on the rejected
value to parse the body.
4. Because the mocked error404.response object (lines 267-273 in the test)
lacks an async json() method, getClientErrorObject could throw a
TypeError while attempting to parse the body, causing the test to fail
for the wrong reason (TypeError) instead of exercising the intended
404-handling path. Adding response.json resolves this mismatch.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/actions/dashboardState.test.js
**Line:** 272:272
**Comment:**
*Possible Bug: The mocked 404 error object lacks a Response-like `json()` method; the production helper `getClientErrorObject` may attempt to call `response.json()` on the rejected value and throw a TypeError, causing the test to fail for the wrong reason — include a `json` async method on the mocked `response` so it behaves like a real fetch Response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| status: 500, | ||
| statusText: 'Internal Server Error', | ||
| bodyUsed: false, | ||
| }, |
There was a problem hiding this comment.
Suggestion: The mocked 500 error object also lacks a Response-like json() method; similar to the 404 case, getClientErrorObject could attempt to parse the response body and throw — add an async json method to the mocked response so the test exercises error-handling code rather than causing an unrelated TypeError. [possible bug]
Severity Level: Major ⚠️
- ❌ Unit test for non-404 fetchFaveStar error path broken.
- ⚠️ CI failure potential for dashboardState tests.| }, | |
| json: async () => ({ message: 'Internal Server Error' }), |
Steps of Reproduction ✅
1. Run the unit test "dispatches error toast on non-404 errors" in
superset-frontend/src/dashboard/actions/dashboardState.test.js (the test
block is located near lines ~283-305). The test replaces SupersetClient.get
with a rejection using the error500 object defined at lines 287-295.
2. The test invokes fetchFaveStar(dashboardId) (thunk in
src/dashboard/actions/dashboardState.js around lines 108-140) which
catches the rejection and calls getClientErrorObject(error) to normalize
the error.
3. If getClientErrorObject attempts to call response.json() on the mocked
response, the missing json() implementation on error500.response (lines
288-294) can raise a TypeError, causing the test to fail unexpectedly
instead of validating the non-404 error handling (dispatching an
ADD_TOAST action).
4. Adding an async json() method to the mocked response exercises the real
code path and ensures the test verifies dispatching of the error toast.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/actions/dashboardState.test.js
**Line:** 293:293
**Comment:**
*Possible Bug: The mocked 500 error object also lacks a Response-like `json()` method; similar to the 404 case, `getClientErrorObject` could attempt to parse the response body and throw — add an async `json` method to the mocked `response` so the test exercises error-handling code rather than causing an unrelated TypeError.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Code Review Agent Run #c246b1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Fixes an issue where navigating to another dashboard after deleting a dashboard
could trigger an error toast when fetching the dashboard favorite status.
After dashboard deletion, a stale dashboard ID could still be used by the
frontend to fetch favorite metadata. The backend correctly returns a 404 for
deleted dashboards, but the frontend previously treated this as an unexpected
error.
This change:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable.
TESTING INSTRUCTIONS
Manual testing:
Automated testing:
ADDITIONAL INFORMATION