fix(router): browser back works through resource drawer + relationship links (SKY-849)#618
Conversation
Two browser-back regressions fixed: - Clicking a Parent Gateway from a TCPRoute drawer rewrote the URL with history.replace, so back did nothing on the resulting page. - Clicking a different resource in the same list while a drawer was open also replaced, so back skipped the previously selected resource and jumped to whatever preceded the list view. ResourcesView's URL writer now pushes history when the pathname is about to change (external setSelectedResource targeting a different kind) or when selectedResource transitions A -> B (both non-null, different) -- a real navigation. Initial open and close stay as URL replaces. App.tsx onNavigateToResource (drawer expanded path) now pushes too so back from a related-resource workload returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pushing drawer-to-drawer history entries gives the browser back button a
target, but ResourcesView only re-reads ?resource= on mount. Same-kind
back (e.g. /resources/pods?resource=ns/A from B) doesn't change pathname,
so React doesn't re-mount and selectedResource stays stale, leaving the
URL pointing at A while the drawer renders B. Reconcile selectedResource
from the URL alongside the existing helm/namespace sync so back/forward
within /resources/{kind} restores the matching drawer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reading ?resource= on every URL change clobbers deliberate setSelectedResource calls that race the URL writer - e.g. the helm drawer's "navigate to resource" calls navigate() then setSelectedResource in the same tick, but the writer only adds ?resource= on the next render, so the App-level sync momentarily sees a URL without the param and nulls the selection. Gate the sync on useNavigationType() === POP so it only runs for browser back/forward, where the URL is the actual source of truth and no concurrent setSelectedResource is in flight. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hosts that embed the app under a non-empty basename inject the basename-stripped pathname via the locationPathname prop. Reading window.location.pathname directly would include the basename and never match the basename-relative targetPath, so pathChanged would be true on every URL write and every filter keystroke would push a history entry instead of replacing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Two CRDs can share a kind name across different API groups (e.g. Service in serving.knative.dev vs core). Switching between them yielded drawerSwitched=false and replaced history instead of pushing — back button couldn't return to the prior resource. Aligns with the App-level POP sync equality check which already includes group. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
handleCollapseFromExpanded calls navigate(-1) to return to the pre- expand state. With push semantics on related-resource clicks inside the expanded drawer, navigate(-1) only popped one drawer-internal step instead of returning to the pre-expand list view. Replace semantics here match the user mental model of "I'm exploring inside the expanded drawer; collapse closes it" without breaking bug 8: the load-bearing fix lives in the ResourcesView URL writer (push when pathname differs or selectedResource A->B), which is unrelated to this flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Visual test - Bug 8 verifiedTested locally on Repro path (the user repro from the PR description):
State A (after click row A)
State B (after click row B - drawer-to-drawer switch via push)
After Back (POP-only URL→drawer sync swaps drawer back to A)
URL on each step matches what the PR describes:
Bug 11 (Parent Gateways)verified manually |
The post-POP state catch-up was pushing a duplicate history entry on top of
the popped state. After clicking a "Parent Gateways" badge from a TCPRoute
drawer to a Gateway drawer, browser Back appeared to do nothing — or in
clusters where the route's parentRef namespace defaults to a non-existent
gateway, the URL flipped to a sibling resource via auto-resolution.
Root cause: when App's POP→state sync re-set selectedResource/selectedKind to
match the popped URL, the URL writer effect re-fired with the new state. Its
drawerSwitched check (prev != current) was true (gateway -> tcproute), so it
pushed/replaced — but the URL was already correct, so the write was a no-op
on path but still bumped history (or, when the resource didn't exist in the
target list, it wrote whatever the auto-resolver picked).
Guard: before navigating, compare target newPath/queryStr to
window.location's pathname/search. If they already match, return without
calling navigate. Path comparison treats suffix matches as equal so
basename-mounted hosts (Radar Cloud at /c/{cluster}) aren't false-triggered.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@nadaverell ready |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 990e9be. Configure here.
| } else if (kindFromPath && !resourceParam) { | ||
| setSelectedResource(prev => (prev === null ? prev : null)) | ||
| } | ||
| } |
There was a problem hiding this comment.
POP-triggered state change bypasses URL sync guard
Medium Severity
When the App.tsx POP sync updates selectedResource, the ResourcesView URL-write effect doesn't know this was POP-triggered because isSyncingFromURL is never set (it's only set by the locationPathname/locationSearch sync effect, which App.tsx doesn't use). If filter state (e.g. searchTerm) diverged from the popped URL's params, the no-op guard in updateURL fails to match, and drawerSwitched being true causes a push of stale filter state on top of the popped entry — creating the same "back does nothing" history pollution the PR is fixing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 990e9be. Configure here.
There was a problem hiding this comment.
False positive — the flag-rAF gate and the no-op guard already cover this.
Trace for the worst case (POP changes both ?resource= and a filter param):
- POP changes
useLocation().search. Since?resource=is in search, any POP that causes App to callsetSelectedResource(App.tsx#L688-L711) necessarily also changeslocationSearch. - ResourcesView render 1:
locationSearchprop changed. The locationPathname/Search sync effect (packages/k8s-ui/src/components/resources/ResourcesView.tsx#L2332-L2372) runs first in declaration order, setsisSyncingFromURL.current = truesynchronously, calls setSearchTerm/etc, schedulesrequestAnimationFrameto clear the flag. - App POP-sync queues
setSelectedResource. Render 2. - Render 2: URL-write effect (L2467-L2518) runs.
isSyncingFromURL.currentis still true becauserAFruns after paint, and effects run before paint. Early return at L2470.prevSelectedResourceRefis updated to the post-POP selection so future writes compare against the right baseline. - After paint, rAF clears the flag.
No-op guard is the second safety net. Even if the flag missed, L2447-L2456 reads window.location.pathname + window.location.search (post-POP) and compares against the target built from current state. After the locationSearch effect synced filter state to popped values, queryStr matches window.location.search → return without navigating.
There is no scenario in the current code where a POP changes selectedResource while locationSearch stays the same — POP-driven setSelectedResource is gated on navigationType === NavigationType.Pop && mainView === 'resources' (App.tsx#L689) and reads ?resource= straight from the URL it's reconciling against.
Verified: cd web && npm run tsc clean; cd packages/k8s-ui && npm test 128/128 pass on HEAD 990e9be. The end-to-end fix for Bug 11 was visually tested as documented in the PR description. Not changing the code.
|
@nadaverell ready |
nadaverell
deleted the
feat/stop-replace-nav-on-resource-and-parent-link-clicks
branch
3 participants
Summary
4 commits on this branch addressing two history bugs:
Changes:
122c82fResourcesView URL writer: pushes (instead of replaces) when pathname differs (cross-kind nav like Parent Gateways) or selectedResource A→B (same-list drawer-to-drawer). Initial open (null→X) and close (X→null) stay as replace.122c82fApp.tsx L1294: dropped{ replace: true }fromonNavigateToResource(expanded-drawer related-resource navigation).796926e+c00e19a: POP-only URL→drawer state sync — without this, back changed the URL but the drawer still showed the previous resource. Restricted toNavigationType.Popto avoid clobbering helm/audit programmatic nav.4487545: basename-aware path compare —window.location.pathnameincludes a host's basename (e.g./c/{cluster}under embed) whiletargetPathis basename-relative; reading window directly would force every URL write to push, polluting history with filter keystrokes.Why
SKY-849. The most invasive of the audit bugs — touches the URL writer directly. 4 review iterations across pre-pr + deep-deep.
Where to start
packages/k8s-ui/src/components/resources/ResourcesView.tsxlines 2466-2497 — push/replace decision logic.web/src/App.tsxlines 684-712 — POP-only URL→state sync effect.web/src/App.tsxline 1324 —onNavigateToResourcepush (no longer replace).Risky vs mechanical
useNavigationType()which reflects the LAST navigation; in extreme race conditions a non-POP nav between the back and the effect run could break sync, but no realistic trigger.getInitialKindFromURL(line 1700-1705).Reviewer focus
🤖 Generated with Claude Code
Note
Medium Risk
Touches URL-writing and back/forward reconciliation logic in
ResourcesView/App, which can subtly affect navigation history and embedded-basepath deployments. Scope is contained to resources-view URL updates and POP navigation handling.Overview
Fixes browser Back/Forward behavior for the resource drawer by adjusting when URL updates push vs replace history entries.
ResourcesViewnow (1) avoids redundant navigations when the computed target URL already matches the address bar, and (2) pushes history on kind/path changes or drawer-to-drawer resource switches while keeping initial open/close as replaces (with basename-aware path comparisons).Appadds POP-only URL→state reconciliation usinguseNavigationType()so same-kind back/forward updates the selected resource drawer state even whenResourcesViewdoesn’t remount.Reviewed by Cursor Bugbot for commit 990e9be. Bugbot is set up for automated code reviews on this repo. Configure here.