fix(dashboard): prevent double-click on create dashboard from creating duplicates#40833
fix(dashboard): prevent double-click on create dashboard from creating duplicates#40833massucattoj wants to merge 1 commit into
Conversation
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| @@ -30,7 +32,10 @@ export const navigateTo = ( | |||
| 'noopener noreferrer', | |||
| ); | |||
| } else if (options?.assign) { | |||
| window.location.assign(sanitizeUrl(ensureAppRoot(url))); | |||
| const sanitized = sanitizeUrl(ensureAppRoot(url)); | |||
| if (pendingAssignUrl === sanitized) return; | |||
| pendingAssignUrl = sanitized; | |||
| window.location.assign(sanitized); | |||
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The dedupe latch (pendingAssignUrl) is only ever set and never cleared, so after a successful navigateTo('/dashboard/new', { assign: true }) the module-level flag remains set; when the Dashboards or Welcome page is restored from back-forward cache and the user clicks the same "+ Dashboard"/"+ Chart" button again, navigateTo sees the same sanitized URL and becomes a no-op, breaking legitimate repeat create actions.
Suggestion: Ensure the guard is not permanent for the entire page lifetime: either make it time-bounded (e.g., a short double-click window) or explicitly reset pendingAssignUrl on appropriate lifecycle events (such as pageshow/popstate) so duplicate-click protection does not block later, valid navigations.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/src/utils/navigationUtils.ts
**Line:** 22:38
**Comment:**
*HIGH: The dedupe latch (pendingAssignUrl) is only ever set and never cleared, so after a successful navigateTo('/dashboard/new', { assign: true }) the module-level flag remains set; when the Dashboards or Welcome page is restored from back-forward cache and the user clicks the same "+ Dashboard"/"+ Chart" button again, navigateTo sees the same sanitized URL and becomes a no-op, breaking legitimate repeat create actions.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The architectural review finding is correct. The superset-frontend/src/utils/navigationUtils.ts |
![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 #764792
Actionable Suggestions - 1
-
superset-frontend/src/utils/navigationUtils.test.ts - 1
- Module state pollution in tests · Line 21-33
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/utils/navigationUtils.ts - 1
- State never cleared memory leak · Line 36-37
Review Details
-
Files reviewed - 2 · Commit Range:
92a6f98..92a6f98- superset-frontend/src/utils/navigationUtils.test.ts
- superset-frontend/src/utils/navigationUtils.ts
-
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
| test('navigateTo with assign: true ignores repeated calls to the same URL', () => { | ||
| const assignSpy = jest.fn(); | ||
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: { assign: assignSpy, href: '' }, | ||
| }); | ||
|
|
||
| navigateTo('/dashboard/new', { assign: true }); | ||
| navigateTo('/dashboard/new', { assign: true }); | ||
|
|
||
| expect(assignSpy).toHaveBeenCalledTimes(1); | ||
| expect(assignSpy).toHaveBeenCalledWith('/dashboard/new'); | ||
| }); |
There was a problem hiding this comment.
The test relies on pendingAssignUrl in navigationUtils being null at start, but this module-level variable persists across test files. If another test in the same process calls navigateTo(..., { assign: true }) before this test, the first assertion will fail because pendingAssignUrl is already set to a value. Additionally, ensureAppRoot is not mocked — in production deployments with SUPERSET_APP_ROOT=/superset, the URL would be transformed to /superset/dashboard/new, causing a mismatch with the expected value.
Code Review Run #764792
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| const sanitized = sanitizeUrl(ensureAppRoot(url)); | ||
| if (pendingAssignUrl === sanitized) return; | ||
| pendingAssignUrl = sanitized; | ||
| window.location.assign(sanitized); |
There was a problem hiding this comment.
Suggestion: The dedupe flag is written before navigation is guaranteed to succeed, and it is never cleared on cancellation/failure. If window.location.assign is blocked (for example by a beforeunload confirmation where the user stays on the page), future clicks to the same URL become permanent no-ops in that page session. Reset or roll back the pending value when navigation does not actually complete. [cache]
Severity Level: Major ⚠️
- ❌ New-dashboard button can stop navigating after canceled attempt.
- ⚠️ Same behavior possible for new-chart button on Welcome.
- ⚠️ Dedupe state stays stuck until full page reload.Steps of Reproduction ✅
1. Observe the navigation helper in
`superset-frontend/src/utils/navigationUtils.ts:24–41`, where `navigateTo()` handles `{
assign: true }` by computing `sanitized`, checking `pendingAssignUrl`, storing
`pendingAssignUrl = sanitized`, then calling `window.location.assign(sanitized)` (lines
35–38).
2. From a caller such as `superset-frontend/src/features/home/DashboardTable.tsx:204–206`,
click the "+ Dashboard" button, which calls `navigateTo('/dashboard/new', { assign: true
})`; this sets `pendingAssignUrl` before attempting navigation.
3. In a realistic failure scenario, `window.location.assign` does not complete
navigation—for example, in a unit/integration test where `window.location.assign` is
mocked to throw, or in the browser when a `beforeunload` handler blocks navigation and the
user chooses to stay on the page (the repo already registers `beforeunload` for unsaved
changes in places like `src/dashboard/components/Dashboard.tsx:11–24` and via the
`useBeforeUnload` hook in `src/hooks/useBeforeUnload/index.ts:26–13` and
`src/features/themes/ThemeModal.tsx:392–403`).
4. After that failed/canceled navigation, the page remains loaded and `pendingAssignUrl`
is still set to the sanitized URL, so subsequent clicks to the same callers
(`DashboardTable` at 204–206, `ChartTable` at 203–205, or `DashboardList` at
`src/pages/DashboardList/index.tsx:732–739`) invoke `navigateTo()` again, hit the `if
(pendingAssignUrl === sanitized) return;` guard, and permanently skip
`window.location.assign` for that URL in this page session.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/utils/navigationUtils.ts
**Line:** 35:38
**Comment:**
*Cache: The dedupe flag is written before navigation is guaranteed to succeed, and it is never cleared on cancellation/failure. If `window.location.assign` is blocked (for example by a `beforeunload` confirmation where the user stays on the page), future clicks to the same URL become permanent no-ops in that page session. Reset or roll back the pending value when navigation does not actually complete.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40833 +/- ##
=======================================
Coverage 64.13% 64.13%
=======================================
Files 2667 2667
Lines 144127 144131 +4
Branches 33135 33136 +1
=======================================
+ Hits 92434 92438 +4
Misses 50082 50082
Partials 1611 1611
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
Double-clicking the + Dashboard button on the Dashboards page caused Superset to create two (or more) duplicate dashboards. Each click fired
navigateTo('/dashboard/new', { assign: true }), which callswindow.location.assign(...). With a real OS-level double-click (~50–300ms gap between events), bothassigncalls reach the browser before the page tears down, so the backendDashboard.new()route receivestwo GETs and commits two
DashboardModelrows.This PR adds a small module-level dedupe inside
navigateTo: whenassign: trueis used, repeated calls to the same URL within the same page session become no-ops. The guard naturally resets when the pageunloads after a successful navigation.
The fix is centralized at the util level, so the same protection also applies to the + Chart button on the Welcome page (
/chart/add), which has the same potential for duplicates.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF