fix(listview): stop card clicks creating a duplicate history entry - #43310
Open
aminghadersohi wants to merge 3 commits into
Open
fix(listview): stop card clicks creating a duplicate history entry#43310aminghadersohi wants to merge 3 commits into
aminghadersohi wants to merge 3 commits into
Conversation
ListViewCard renders its cover as a router Link while ChartCard and DashboardCard also make the whole card clickable. A click on the cover was handled by both, pushing two identical history entries for one click, so the browser Back button popped the duplicate and left the user on the page they were trying to leave. Skip the card-level navigation when the click originated inside a link.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43310 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 2876 2876
Lines 164007 164012 +5
Branches 37834 37837 +3
=======================================
+ Hits 109347 109353 +6
+ Misses 52514 52512 -2
- Partials 2146 2147 +1
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:
|
aminghadersohi
marked this pull request as ready for review
August 19, 2026 03:39
dosubot
Bot
added
change:frontend
aminghadersohi
requested review from
msyavuz and
rusackas
and removed request for
msyavuz
August 19, 2026 03:40
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #f222ec
Actionable Suggestions - 1
-
superset-frontend/src/features/dashboards/DashboardCard.test.tsx - 1
- Unrestored spy in test leaks across tests · Line 110-110
Additional Suggestions - 2
-
superset-frontend/src/features/charts/ChartCard.test.tsx - 1
-
Brittle CSS class selector in test · Line 93-93Using a CSS class name `.gradient-container` for test queries is fragile — class names are implementation details that can change without notice, causing tests to break. This class is not defined within this file or ChartCard.tsx (verified by grep search), making the selector opaque. Use a `data-test` attribute or semantic query instead.
-
-
superset-frontend/src/views/CRUD/utils.tsx - 1
-
Missing unit tests for utility · Line 493-496The new `isNavigationHandledByLink` utility has no unit tests, unlike the nearby `isNeedsPassword` function which is tested in `utils.test.tsx`. Per project testing standards, new utilities require coverage for success paths, error scenarios, and edge cases before merging.
-
Review Details
-
Files reviewed - 5 · Commit Range:
2cb4f09..6ac343a- superset-frontend/src/features/charts/ChartCard.test.tsx
- superset-frontend/src/features/charts/ChartCard.tsx
- superset-frontend/src/features/dashboards/DashboardCard.test.tsx
- superset-frontend/src/features/dashboards/DashboardCard.tsx
- superset-frontend/src/views/CRUD/utils.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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
| // the cover used to be handled twice and pushed two identical entries, which | ||
| // left the Back button popping the duplicate rather than returning the user | ||
| // to the page they came from. | ||
| jest.spyOn(global, 'fetch').mockResolvedValue({ |
Contributor
There was a problem hiding this comment.
Unrestored spy in test leaks across tests
The jest.spyOn(global, 'fetch') at line 110 is never restored. Tests that run after this one will still have the mocked fetch, causing cross-test pollution.
Code Review Run #f222ec
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
- restore the fetch spy so it does not leak into later tests - query the cover link by role instead of an internal CSS class - cover isNavigationHandledByLink directly in views/CRUD/utils.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Clicking a chart or dashboard card and then pressing the browser Back button does not return you to the page you came from — you stay on the chart, and only a second Back press gets you out.
The cause is a double navigation.
ListViewCardrenders its cover (the thumbnail) as a react-router<Link to={url}>, whileChartCardandDashboardCardadditionally wrap the whole card in a clickable element that callshistory.push(url). Neither stops propagation, so a single click on the cover is handled twice and pushes two identical history entries. Back pops the duplicate, which resolves to the same Explore/Dashboard URL, so the navigation looks broken.This only shows up where cards are rendered with thumbnails — the homepage chart/dashboard sections. The Chart and Dashboard list pages default to table view (card view is behind
LISTVIEWS_DEFAULT_CARD_VIEW), which uses a plain link and pushes once, which is why those entry points behave correctly.I kept the whole card clickable rather than removing the wrapper handler, since clicking the title or body — which are not inside the link — must still navigate.
What
Added a small
isNavigationHandledByLinkhelper insrc/views/CRUD/utils.tsxthat reports whether a click originated inside an anchor.ChartCardandDashboardCardskip their ownhistory.pushin that case and let the link navigate, so one click produces exactly one history entry.How to test
Regression tests added for both cards asserting that a click on the cover produces exactly one
PUSH, plus a test that clicking outside the cover still navigates. Both new cover tests fail onmaster(the recorded navigations are['PUSH …', 'PUSH …']) and pass with the fix.Manually: enable
THUMBNAILS, go to the homepage, click a chart card thumbnail, then press Back — you land back on the homepage instead of staying on the chart.Risk & rollback
Low and contained to the two card components. The only behavior change is that a click on the cover no longer double-navigates; clicks on the title, description, and card background are unchanged, and
bulkSelectEnabledstill short-circuits navigation as before. Straight revert if needed.Review guidance
Start with
src/views/CRUD/utils.tsxfor the helper and its rationale, then the two one-line call sites. The riskiest assumption is theclosest('a[href]')check — it deliberately matches any anchor ancestor, so if a card ever gains a non-navigating anchor, that click would stop navigating the card.