refactor(desktop): loop-proof route redirects and contain content crashes

Merged
KI
3mo ago
+367-87

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 upstream main: 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 Redirect component (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 Biome noRestrictedImports (importNames: ["Navigate"]), pointing offenders at Redirect or beforeLoad. 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:

  • Reactive redirects (root index, sign-in, create-org, onboarding, authenticated layout ×3, dashboard versionMismatch, cloud gates ×2) → Redirect.
  • Static/legacy redirects (settings index, presets→terminal, tasks ?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_ACCESS flag reads undefined while loading, which the old gates treated as "no access" and bounced entitled users out. Now renders nothing until the flag resolves.

5. Adds a CatchBoundary around 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)

  • All four migrated redirect targets resolve correctly (/workspace/$id→/new-workspace, /tasks?type=prs→/pull-requests, /settings→/settings/account, /settings/presets→/settings/terminal) with zero #185 signals and the layout surviving every transition.
  • Forced a render throw in a dashboard route (/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.
  • Redirect vs an equivalent direct navigate({search}) produce identical router state — no behavioral change beyond loop-proofing.
  • typecheck clean, lint clean, 1454 renderer tests pass.

Follow-ups (not in this PR)

  • Renderer Sentry didn't capture Jose's crash despite ErrorPage calling captureException; render-process-gone in main only logs locally. Telemetry PR worth doing so this class is visible in monitoring.
  • A Redirect unit 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 Redirect for @tanstack/react-router, enforces it repo‑wide, moves static redirects to beforeLoad, and proves it with a renderer test.

  • Bug Fixes

    • Stop infinite re-navigation: replace inline <Navigate> with loop‑proof Redirect; move static redirects to beforeLoad so nothing navigates during render.
    • Contain dashboard content-route crashes with a catch boundary that resets on full location href; keep sidebar/terminals mounted, report to Sentry, log reporting failures, and wrap long messages.
    • Fix cloud settings gate: wait for CLOUD_ACCESS to resolve instead of treating undefined as no access.
    • Handle failed navigations: Redirect catches errors, logs them, and clears its marker so later renders can retry.

  • Refactors

    • Add renderer/components/Redirect that navigates only when the resolved href changes.
    • Ban Navigate in the renderer via Biome noRestrictedImports; use Redirect or beforeLoad + redirect(...).
    • Migrate routes (root, sign-in, create-org, onboarding, authenticated layout, dashboard version mismatch, tasks legacy PR routes, settings index, presets→terminal) to the new patterns.
    • Add a renderer unit test using @testing-library/react and @happy-dom/global-registrator to verify single navigation across re-renders.

Summary by CodeRabbit

  • New Features

    • Added reliable redirect handling across authentication, onboarding, settings, dashboard, and legacy routes.
    • Preserved relevant URL parameters during legacy route redirects.
    • Added dashboard error recovery with error details, Go Home, and Reload App actions.

  • Bug Fixes

    • Prevented repeated navigation and improved retry behavior when redirects fail.
    • Prevented dashboard content errors from escaping the dashboard layout.

  • Tests

    • Added comprehensive coverage for redirect behavior, including navigation updates and loop prevention.