Repository navigation
refactor(desktop): loop-proof route redirects and contain content crashes - #6354
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe desktop renderer adds a loop-safe ChangesDesktop routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Route
participant Redirect
participant TanStackRouter
participant Destination
Route->>Redirect: render redirect options
Redirect->>TanStackRouter: navigate once per href
TanStackRouter->>Destination: load and render target route
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 17 files
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Architecture diagram
sequenceDiagram
participant UI as React Tree
participant Redirect as Redirect Component
participant Router as TanStack Router
participant Layout as Dashboard Layout
participant Boundary as CatchBoundary
participant ErrorPane as DashboardContentError
participant Sentry as Sentry
participant Biome as Biome Linter
Note over UI,Router: NEW: Loop-proof redirect flow
UI->>UI: Parent re-renders
UI->>Redirect: Renders inline with props
Redirect->>Router: buildLocation(props)
Router-->>Redirect: resolved href
Redirect->>Redirect: Compare href with lastHrefRef
alt href changed
Redirect->>Router: navigate(props)
Router-->>UI: New route rendered
else href unchanged
Note over Redirect: Skip navigation (loop-proof)
end
Note over UI,Router: Static redirects via beforeLoad
UI->>Router: Navigate to static route
Router->>Router: Execute beforeLoad
Router->>Router: throw redirect(...)
Router-->>UI: Redirect to new route before render
Note over UI,Layout: Dashboard content crash containment
UI->>Layout: Render dashboard layout
Layout->>Boundary: Wrap Outlet with CatchBoundary
Boundary->>Boundary: Track location.pathname as reset key
Boundary->>UI: Render content route
alt Content route throws
Boundary-->>ErrorPane: Capture error
ErrorPane->>Sentry: captureException (lazy import)
ErrorPane-->>UI: Render error pane (sidebar alive)
else Content renders fine
Boundary-->>UI: Render normal content
end
Note over UI,Biome: CHANGED: Enforce safe redirect patterns
Biome->>Biome: Check import restrictions
alt Import of Navigate detected
Biome-->>UI: Lint error (CI fails)
else Use Redirect or beforeLoad
Biome-->>UI: Pass
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
force-pushed
the
harden-route-redirects
branch
from
August 11, 2026 21:54
4bc24fd to
9678cd4
Compare
Member
Author
|
Addressed all four review items:
Redirect loop-proof unit test still green; typecheck/lint/full desktop suite (2669) pass. Commit 9678cd4. |
…shes Makes the SUPER-1814 crash class unreachable rather than fixing sites one at a time. - add renderer Redirect component: re-navigates only when the resolved target href changes, not on props-identity change like TanStack's <Navigate> (which loops into React #185 when rendered inline) - ban Navigate in the renderer via biome noRestrictedImports, pointing at Redirect or beforeLoad+redirect; CI already fails on warnings - migrate the 5 hoisted + 1 versionMismatch <Navigate> sites to Redirect - convert static/legacy redirects (settings index, presets, tasks PR tabs, tasks PR detail) to beforeLoad throw redirect() so no navigation happens at render time at all - fix cloud settings gates bouncing entitled users while the CLOUD_ACCESS flag is still loading (undefined read as no-access) - wrap the dashboard Outlet in a CatchBoundary so a crashing content route shows an error pane with the sidebar/terminals alive, instead of bubbling to the root boundary and unmounting the whole app Verified over CDP: all redirect targets resolve with no #185 signals; a forced render throw in a dashboard route is contained to the content pane (sidebar survives, root error page not shown).
Renders Redirect through a real TanStack router (happy-dom + testing- library, mirroring packages/chat) and asserts it navigates exactly once even when its parent re-renders 10x handing it a fresh props object each time — the precise condition that loops <Navigate> into React #185. Mutation-verified: reverting Redirect to props-identity comparison makes this test report 11 navigations instead of 1. happy-dom registers process-wide globals and Bun runs test files in one process, so the file unregisters in afterAll to restore the shared mock document — without it, readonly DOM globals leak and break unrelated suites (e.g. v1-migration's globalThis.localStorage assignment). Full desktop suite verified green (2669 pass, exit 0).
- Redirect: catch failed navigations (log + clear the handled marker so a later render can retry) instead of leaking an unhandled rejection - dashboard CatchBoundary: reset on full location href, not just pathname, so a same-path search/hash change (filter/tab) recovers a stuck error pane - DashboardContentError: log Sentry reporting failures instead of swallowing them; add break-words so long path/URL errors wrap
Kitenite
force-pushed
the
harden-route-redirects
branch
from
August 11, 2026 22:13
9678cd4 to
bce1863
Compare
This branch was previously deployed
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
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.
Stacked on #6353. That PR fixes the specific SUPER-1814 crash; this one makes the whole crash class unreachable so it can't come back at a different site.
Background
TanStack's
<Navigate>re-navigates whenever its props object identity changes (confirmed in the installed source and upstreammain:if (previousPropsRef.current !== props)). Rendered inline, it re-navigates on every parent re-render and loops into React error #185 — which unmounts the tree. The codebase had been defending this by hoisting each<Navigate>element by hand (#5729), which is whack-a-mole: the six un-hoisted sites I found were all latent instances of the same bug.What this does
1. A loop-proof
Redirectcomponent (renderer/components/Redirect) — re-navigates only when the resolved target href changes, not on props identity. Safe to render inline with dynamic params/search.2. Bans
<Navigate>in the renderer via BiomenoRestrictedImports(importNames: ["Navigate"]), pointing offenders atRedirectorbeforeLoad. CI already fails on Biome warnings, so this is binding — mutation-tested with a canary file that the rule correctly rejected.3. Migrates all sites:
Redirect.?type=prs, tasks PR detail) →beforeLoad: () => { throw redirect(...) }, so no navigation happens at render time at all.4. Fixes a latent cloud-settings bug — the
CLOUD_ACCESSflag readsundefinedwhile loading, which the old gates treated as "no access" and bounced entitled users out. Now renders nothing until the flag resolves.5. Adds a
CatchBoundaryaround the dashboard Outlet — a crashing content route now shows a contained error pane with the sidebar and terminals still alive, instead of bubbling to the root boundary and unmounting the whole app (which is what made SUPER-1814 present as "Superset restarted itself").Verification (CDP, dev desktop from this worktree)
/workspace/$id→/new-workspace,/tasks?type=prs→/pull-requests,/settings→/settings/account,/settings/presets→/settings/terminal) with zero fix(desktop): use full PATH for open in app in production builds #185 signals and the layout surviving every transition./automations): the content-error pane rendered with the sidebar fully intact and the root "Something went wrong" page not shown — screenshot confirms containment. Reverted the probe after.Redirectvs an equivalent directnavigate({search})produce identical router state — no behavioral change beyond loop-proofing.Follow-ups (not in this PR)
ErrorPagecallingcaptureException;render-process-gonein main only logs locally. Telemetry PR worth doing so this class is visible in monitoring.Redirectunit test would need a router test harness the repo doesn't have yet.Summary by cubic
Hardened renderer navigation to stop React #185 redirect loops and contain dashboard content crashes (SUPER-1814). Adds a loop‑proof
Redirectfor@tanstack/react-router, enforces it repo‑wide, moves static redirects tobeforeLoad, and proves it with a renderer test.Bug Fixes
<Navigate>with loop‑proofRedirect; move static redirects tobeforeLoadso nothing navigates during render.href; keep sidebar/terminals mounted, report to Sentry, log reporting failures, and wrap long messages.CLOUD_ACCESSto resolve instead of treatingundefinedas no access.Redirectcatches errors, logs them, and clears its marker so later renders can retry.Refactors
renderer/components/Redirectthat navigates only when the resolvedhrefchanges.Navigatein the renderer via BiomenoRestrictedImports; useRedirectorbeforeLoad+redirect(...).@testing-library/reactand@happy-dom/global-registratorto verify single navigation across re-renders.Written for commit bce1863. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests