fix(explore): open version-history forks in a usable tab, not a blank one - #43830
fix(explore): open version-history forks in a usable tab, not a blank one#43830mikebridge wants to merge 1 commit into
Conversation
Code Review Agent Run #1c196cActionable 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43830 +/- ##
=======================================
Coverage 79.41% 79.42%
=======================================
Files 2895 2895
Lines 167939 167942 +3
Branches 38894 38896 +2
=======================================
+ Hits 133375 133389 +14
+ Misses 32064 32053 -11
Partials 2500 2500
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:
|
2e73786 to
98fdcd1
Compare
|
Ran a 2-lens internal review (React/TypeScript + a structured PR-style pass) over this diff — both approved, no blocking findings. Folded the actionable notes into
File is at 82/82 tests, changed-file pre-commit clean (oxfmt/oxlint/tsc). |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — correct, minimal, and spec-verified. Nice work.
The root-cause analysis is exactly right: per the WHATWG spec window.open(..., 'noopener') always returns null (and noreferrer implies noopener), so on master openBlankTab() always returned null and the tab.location.replace reuse branch in navigateOpenedTab was effectively dead code — the gesture-authorized placeholder tab could never actually be reused. Dropping the features string on the placeholder is what makes the claim-then-navigate pattern work, and gating reuse behind isSameOriginRoute upgrades the same-origin guarantee from a caller convention into a structural invariant. Since assertSafeNavigationUrl(ensureAppRoot(path)) already constrains url to either a single-slash router-relative path or an absolute allow-listed scheme, and the app root is always '' or /segment, the guard is both sound and complete — no cross-origin URL can ever ride the opener-connected tab, so dropping noopener here is a neutral-to-positive security change. Test 7 encoding that same-origin/tabnabbing invariant as an executable assertion is a great touch.
One cleanup I'd ask for before merge, plus two optional test nits:
🟡 Please remove the internal tracker reference. One of the new tests carries a comment referencing an internal, non-Apache ticket ID (and the branch name embeds the same ID). Internal tracker references shouldn't land in the permanent public history via the squashed commit. Could you genericize the comment to describe the regression itself, e.g. // Regression: opening as new stranded a blank about:blank tab because openBlankTab returned null?
🟢 openBlankTab test would stay green on a noreferrer-only regression. The placeholder test only inspects the noopener token. Because noreferrer also forces window.open to return null in a real browser, re-introducing just 'noreferrer' would reship the stranded-tab bug while the test (whose mock returns a tab unconditionally) stays green. Consider asserting the exact call instead: expect(openSpy).toHaveBeenCalledWith('', '_blank').
🟢 URL-validation-ordering test under-pins itself. validates the URL before touching a claimed tab asserts replace/open weren't called, but not close — so a future refactor moving tab.close() ahead of assertSafeNavigationUrl wouldn't be caught. Consider adding expect(tab.close as jest.Mock).not.toHaveBeenCalled();.
🟢 Dead-but-defensive external branch (informational). The !isSameOriginRoute → tab.close() + window.open(...) path is unreachable for all current callers (they all pass router-relative routes). Good hardening to keep; just note that if a future caller ever passes an absolute URL after an await, the reopen will be popup-blocked and the placeholder closes with nothing opening. A one-line docstring caveat would help the next reader.
… one
openBlankTab opened its placeholder with window.open('', '_blank',
'noopener noreferrer'). Per the HTML standard, window.open(..., 'noopener')
always returns null -- so the handle the function exists to return was
discarded on every call. The version-history "Open as new chart/dashboard"
and "open related entity" flows claim a tab up front (while the click's
transient activation is live), then navigate it after awaiting the fork
requests. With a null handle, navigateOpenedTab always fell through to a
one-shot window.open(url) whose activation had lapsed, so the browser
refused it and left the user staring at a stranded about:blank tab -- even
though the fork itself succeeded on the server.
Open the placeholder without noopener so the handle is usable. The
destination is always a same-origin app route (ensureAppRoot), so the
opener relationship carries no cross-origin tabnabbing risk; the one-shot
fallback in navigateOpenedTab keeps noopener since it passes the real URL
and never needs the handle.
Adds coverage for openBlankTab (handle returned, no noopener),
navigateOpenedTab (live-handle replace with app-root prefix, URL validation
before touching the tab, null/closed fallback to window.open), and
closeOpenedTab.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TQeAcprGvkFS8F3nghtvwv
98fdcd1 to
72152ba
Compare
|
Thanks for the careful review @rebenitez1802 — all addressed in
82/82 on the file, changed-file pre-commit clean (oxfmt/oxlint/tsc). |
Code Review Agent Run #b5539cActionable Suggestions - 0Additional Suggestions - 1
Review 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
The version-history panel's "Open as new chart / Open as new dashboard" (and the related-entity open) opened a blank
about:blanktab instead of the forked object — reproduced for both current and older versions, on charts and dashboards.Root cause is in
superset-frontend/src/utils/navigationUtils.ts. These flows follow a claim-then-navigate pattern: because the fork takes several sequential requests (snapshot → resolve → copy), they callopenBlankTab()synchronously in the click handler — while the click's transient user activation is still live — and then point that tab at the destination once the new object's id is known, vianavigateOpenedTab(). This avoids the popup blocker refusing awindow.openissued after the awaits.But
openBlankTab()opened its placeholder withwindow.open('', '_blank', 'noopener noreferrer'), and per the HTML standardwindow.open(..., 'noopener')always returnsnull— that is the entire purpose ofnoopener: sever the opener link, so the caller gets no window handle. So the handle the function exists to return was discarded on every call:openBlankTab()opens a blank tab but returnsnull→tab = null.POST /copy/orPOST /chart/) — all succeed; the server creates the new object.navigateOpenedTab(null, url)sees a null handle and falls through to a secondwindow.open(url, ...), which by now has lost user activation and is silently refused by the popup blocker.about:blank.The fix opens the placeholder without
noopenerso the returned handle is usable, andnavigateOpenedTabcan calltab.location.replace(url)on the live tab. The destination is always a same-origin app route (built throughensureAppRootand validated byassertSafeNavigationUrl), so the opener relationship carries no cross-origin tabnabbing risk. The one-shotwindow.open(url, ...)fallback innavigateOpenedTabkeepsnoopener, since it passes the real URL directly and never needs the handle.Scope: this fixes every claim-then-navigate path — chart open-as-new, dashboard open-as-new, and
openRelatedEntity(all shareopenBlankTab/navigateOpenedTab). In-place Preview was never affected because it renders in the panel and opens no tab.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: selecting "Open as new chart/dashboard" from the version-history kebab opens a new tab that stays on
about:blank— even though the fork succeeded on the server (the copy request returns200with the new id). Verified live:POST /api/v1/dashboard/<id>/copy/→200 {"result":{"id":N}}, and navigating directly to/dashboard/N/renders the fork correctly — only the automatic tab navigation was broken.After: the claimed tab receives a real window handle and is navigated to the new object's route (
/explore/?slice_id=Nor/dashboard/N/), so the forked chart/dashboard opens populated, as intended.TESTING INSTRUCTIONS
Requires the versioning UI (
SOFT_DELETE/ version-history feature) enabled and an object with at least one saved version.Automated:
cd superset-frontend && npx jest src/utils/navigationUtils.test.ts— adds coverage foropenBlankTab(returns a usable handle, nonoopener),navigateOpenedTab(live-handlereplacewith app-root prefix, URL validation before touching the tab, null/closed →window.openfallback), andcloseOpenedTab.ADDITIONAL INFORMATION