Skip to content

feat(code): convert navigationStore to router-derived facade#2457

Closed
adamleithp wants to merge 9 commits into
feat/tanstack-router-full-portfrom
feat/tanstack-router-delete-navstore
Closed

feat(code): convert navigationStore to router-derived facade#2457
adamleithp wants to merge 9 commits into
feat/tanstack-router-full-portfrom
feat/tanstack-router-delete-navstore

Conversation

@adamleithp
Copy link
Copy Markdown
Contributor

Stacks on top of #2456. Closes out the dual-source-of-truth concern raised in #2455's architect review: the Zustand navigationStore no longer holds its own view state — it's now a thin facade that derives view from the router and the React Query task cache on every render.

What changed

useNavigationStore keeps the same public API surface ((selector?), .getState(), .setState()) so the ~91 existing call sites don't change. Internally:

  • No Zustand state. Hook subscribes to useRouterState + useTaskInputPrefillStore to know when to re-render. Snapshot is rebuilt from those two sources on every call.
  • view.data populates from cache. getCachedTask(taskId) in @utils/queryClient walks the React Query ["tasks", "list"] queries. Consumers reading view.data?.id keep working; the value is just undefined until useTasks() resolves (same as the deep-link case in the old store).
  • Actions delegate. Each navigateTo* action calls a navigationBridge function. goBack/goForward use router.history. No more syncToRouter (no state to mirror).
  • Transient TaskInput prefill moves out. initialPrompt, reportAssociation, initialCloudRepository, initialModel, initialMode, folderId, taskInputRequestId now live in useTaskInputPrefillStore (introduced in feat(code): TanStack Router architectural cleanup #2456 for exactly this). navigateToTaskInput(options) writes prefill → navigates to /code. The derived view reads prefill and surfaces those fields under view.* exactly like before.
  • Async side effects preserved. navigateToTask still does the workspace/folder reconciliation it always did — that logic is unchanged.
  • Analytics context now fires from a router.subscribe("onResolved") listener at module init instead of from inside each action.

Persistence, the history stack, and hydrateTask are deleted. Persistence is replaced by router URL + the cold-boot localStorage restore from #2456. The history stack is replaced by router.history. hydrateTask becomes a no-op — the URL is the source of truth, no rehydration needed.

New helpers in navigationBridge

  • getCurrentMatches(), getCurrentLocation() — non-React router state accessors.
  • subscribeToRouterResolved(handler) — hook into route resolution from outside React.
  • goForwardInHistory() — for symmetry with goBackInHistory.

These keep the store cycle-free (navigationStore never imports @renderer/router directly).

Files touched

File Change
stores/navigationStore.ts rewritten (~400 lines → ~200)
stores/navigationStore.test.ts rewritten (~290 → ~200, 16 → 12 tests)
routes/code/tasks/$taskId.tsx removed obsolete URL→store sync effect
navigationBridge.ts added 4 helpers
utils/notifications.test.ts mocks useNavigationStore.getState directly

What's NOT in this PR

  • Settings store consumers (PR 7): the settingsDialogStore is still alive. Its consumers can move to useNavigate / <Link/> directly once this lands.
  • Literal deletion of the file. navigationStore.ts still exists as a hook export. Removing the file means migrating all 91 call sites to useNavigate/useParams/useMatch. Worth doing, but the diff would dwarf the architectural change in this PR. I'd schedule it as PR 8 once the facade has run in production for a release cycle.

Test plan

Automated (passing):

  • Renderer typecheck (pnpm exec tsc -p tsconfig.web.json --noEmit)
  • Vitest suite — 131 files / 1594 tests pass (down from 1598: 4 deleted persistence/history tests no longer apply)
  • Biome lint clean

Manual (please verify):

  • All previous flows from feat(code): introduce TanStack Router file-based routing #2455 + feat(code): TanStack Router architectural cleanup #2456 still work
  • Sidebar highlight tracks the active task across clicks
  • view.data populates correctly in HeaderRow task section (skill buttons, diff stats badge, etc.) when viewing a task
  • Cloud + local task headers still render (consumers in GlobalEventHandlers that pass view.data to handoff buttons)
  • useArchiveTask still navigates away from an archived task correctly (it reads view.data to detect the current task)
  • Deep link to a task that isn't in the task list yet → view.data is undefined briefly, then populates once useTasks resolves (same behavior as before, this case existed previously)
  • Settings UX from feat(code): TanStack Router architectural cleanup #2456 still works
  • Devtools panel shows current route correctly in dev mode

🤖 Generated with Claude Code

richardsolomou and others added 3 commits June 2, 2026 12:17
Removes the dual source of truth between Zustand and TanStack Router.
`useNavigationStore` keeps the same public API surface for the ~91 existing
consumers, but internally has no state of its own — `view` is derived from
router state + the task query cache on every render. Actions delegate to
`navigationBridge`. Persistence, history stack, and `hydrateTask` are gone
(URL is the source of truth, hashHistory + cold-boot restore handle the rest).

Notable changes:
- `navigationStore.ts` rewritten as a custom hook (~200 lines vs ~400). Keeps
  `useNavigationStore()`, `.getState()`, and a no-op `.setState()` so
  consumers don't change.
- `view.data` (the full Task object) now populates from
  `getCachedTask(taskId)` against the React Query cache. Consumers that
  guarded on `view.data` (HeaderRow, GlobalEventHandlers, ArchivedTasksView,
  etc.) keep working; the value is just undefined until tasks load, same as
  before for deep-link cases.
- Transient TaskInput state (initialPrompt, reportAssociation,
  initialCloudRepository, etc.) moves to the existing
  `useTaskInputPrefillStore`. `navigateToTaskInput(options)` writes prefill
  then navigates to `/code`; the route reads prefill via the derived view.
- `syncToRouter` deleted — no internal state to mirror.
- Async side effects in `navigateToTask` (workspace + folder reconciliation)
  preserved unchanged.
- `setActiveTaskAnalyticsContext` now fires from a `router.subscribe(
  "onResolved")` listener at module init, replacing the per-action call.
- New `navigationBridge` accessors (`getCurrentMatches`,
  `getCurrentLocation`, `subscribeToRouterResolved`,
  `goForwardInHistory`) keep the store's router access cycle-free.
- `routes/code/tasks/$taskId.tsx`: removed the URL→store sync effect; with
  the derived view, the sync is automatic.
- `navigationStore.test.ts` rewritten — old tests targeted Zustand
  internals (persistence, history stack) that no longer exist. New 12 tests
  exercise view derivation per route and bridge delegation per action.
- `notifications.test.ts` mocks `useNavigationStore.getState` directly
  instead of driving it via `setState` (which is now no-op).

All 131 test files / 1594 tests pass. Typecheck + lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/code/src/renderer/stores/navigationStore.ts:259-261
`hydrateTask` is defined as an inline arrow function inside `getSnapshot()`, which means a new function reference is created on every call. `__root.tsx` destructures `hydrateTask` from `useNavigationStore()` and places it in a `useEffect` dependency array (`[tasks, hydrateTask]`). Because the reference is never the same across renders, that effect fires on every render of `RootLayout` instead of only when `tasks` changes. The effect body is harmless today (calls a no-op), but the fire frequency is a regression from the old store where Zustand guaranteed stable function references.

```suggestion
    hydrateTask,
```

### Issue 2 of 2
apps/code/src/renderer/stores/navigationStore.ts:257-258
`canGoBack` and `canGoForward` are also inline functions, giving them the same unstable-reference problem as `hydrateTask`. They currently have no call sites, but any future consumer that puts them in a `useCallback` or `useEffect` dependency array would get unexpected re-runs. Lifting them to module-level stubs makes the snapshot fully stable and consistent with the module-level action functions.

```suggestion
    canGoBack,
    canGoForward,
```

Reviews (1): Last reviewed commit: "feat(code): convert navigationStore to r..." | Re-trigger Greptile

Comment on lines 259 to 261
hydrateTask: () => {
/* No-op: the URL is the source of truth now. */
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 hydrateTask is defined as an inline arrow function inside getSnapshot(), which means a new function reference is created on every call. __root.tsx destructures hydrateTask from useNavigationStore() and places it in a useEffect dependency array ([tasks, hydrateTask]). Because the reference is never the same across renders, that effect fires on every render of RootLayout instead of only when tasks changes. The effect body is harmless today (calls a no-op), but the fire frequency is a regression from the old store where Zustand guaranteed stable function references.

Suggested change
hydrateTask: () => {
/* No-op: the URL is the source of truth now. */
},
hydrateTask,
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/stores/navigationStore.ts
Line: 259-261

Comment:
`hydrateTask` is defined as an inline arrow function inside `getSnapshot()`, which means a new function reference is created on every call. `__root.tsx` destructures `hydrateTask` from `useNavigationStore()` and places it in a `useEffect` dependency array (`[tasks, hydrateTask]`). Because the reference is never the same across renders, that effect fires on every render of `RootLayout` instead of only when `tasks` changes. The effect body is harmless today (calls a no-op), but the fire frequency is a regression from the old store where Zustand guaranteed stable function references.

```suggestion
    hydrateTask,
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +257 to +258
canGoBack: () => true,
canGoForward: () => true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 canGoBack and canGoForward are also inline functions, giving them the same unstable-reference problem as hydrateTask. They currently have no call sites, but any future consumer that puts them in a useCallback or useEffect dependency array would get unexpected re-runs. Lifting them to module-level stubs makes the snapshot fully stable and consistent with the module-level action functions.

Suggested change
canGoBack: () => true,
canGoForward: () => true,
canGoBack,
canGoForward,
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/stores/navigationStore.ts
Line: 257-258

Comment:
`canGoBack` and `canGoForward` are also inline functions, giving them the same unstable-reference problem as `hydrateTask`. They currently have no call sites, but any future consumer that puts them in a `useCallback` or `useEffect` dependency array would get unexpected re-runs. Lifting them to module-level stubs makes the snapshot fully stable and consistent with the module-level action functions.

```suggestion
    canGoBack,
    canGoForward,
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

andrewm4894 and others added 6 commits June 2, 2026 09:54
The pure-hook facade had no per-selector memoization — every nav consumer
re-rendered on every router event AND every prefill change, regardless of
whether their selected slice changed. With ~91 consumers, this manifested
as sluggish navigation and broken interactions (settings stopped opening
correctly, task switching crawled).

Fix: back the facade with `create(() => snapshot)` so Zustand's selector
memoization kicks in. Update the store via subscriptions to:
- router.subscribe("onResolved") — for URL changes
- useTaskInputPrefillStore.subscribe — for transient prefill
- queryClient.getQueryCache().subscribe — for view.data populating
  once useTasks resolves

Updates are batched via queueMicrotask to coalesce burst events. Test
fixture exposes a hoisted `_fireRouterResolved` shim so tests can drive
the same subscriber path the runtime uses.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er migration

This completes the TanStack Router migration. The renderer no longer has
parallel routing state — every navigation flows through the router.

## navigationStore deletion (~36 files)

- New `apps/code/src/renderer/hooks/useAppView.ts` exposes the URL-derived
  view as `useAppView()` (subscribes to router state) and
  `getAppViewSnapshot()` (for non-React reads). Replaces every
  `useNavigationStore((s) => s.view)` consumer.
- New `apps/code/src/renderer/hooks/useOpenTask.ts` exports `openTask(task)`
  (router.navigate + workspace/folder reconciliation side effects) and
  `openTaskInput(opts)` (writes prefill, navigates to /code). Replaces every
  `useNavigationStore((s) => s.navigateToTask / .navigateToTaskInput)`
  consumer.
- Direct navigation callers (`navigateToInbox`, `navigateToArchived`, etc.)
  import from `@renderer/navigationBridge` directly — no hook needed.
- Imperative `.getState().view` callers (`useArchiveTask`, `useSessionCallbacks`,
  `useAppBridge`, `notifications`, etc.) use `getAppViewSnapshot()`.
- `navigationStore.ts` and its test are deleted.

## settingsDialogStore deletion (~22 files)

- New `apps/code/src/renderer/features/settings/stores/settingsPageStore.ts`
  holds the UI-only state the dialog store used to own — `context`,
  `initialAction`, `formMode`. No routing in this store.
- New `useOpenSettings.ts` exposes `openSettings(category, contextOrAction)`,
  `closeSettings()`, `useCloseSettings()`, `useIsSettingsOpen()`. These wrap
  the `navigationBridge` so consumers can stay router-unaware.
- `SettingsDialog.tsx` is now a self-contained modal driver for the
  pre-router `AiApprovalScreen` shell only — exports `openSettingsDialog` /
  `closeSettingsDialog` for that case. Inside the main app, the
  `/settings/$category` route renders `SettingsPanel` directly.
- `SettingsPanel` takes optional `activeCategory`, `onClose`,
  `onCategoryChange` props so it can be reused in both the route and the
  pre-router dialog shell.
- `EnvironmentsSettings` reads its segment (local vs cloud) from the URL
  param via `useRouterState`.
- `settingsDialogStore.ts` and its test are deleted; replaced by
  `settingsPageStore.test.ts`.

## Bridge expansion

`navigationBridge.ts` gains `getCurrentMatches()`, `getCurrentLocation()`,
`subscribeToRouterResolved()`, `goForwardInHistory()` so all non-React
router access stays inside the bridge.

## Notes

- Route loaders for TaskDetail/Inbox are deferred — they need the
  task-fetch pipeline extracted from `useAuthenticatedQuery` into plain
  query options that `loader` can call. Worthwhile but a separate refactor.
- Cmd+[ / Cmd+] back/forward shortcuts now go through
  `router.history.back/forward` via the bridge (was the nav store's own
  history stack).
- 1582 tests pass; renderer typecheck and biome lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Peter Kirkham <peter@posthog.com>
…delete-navstore

# Conflicts:
#	apps/code/package.json
#	pnpm-lock.yaml
…l render storm

Three independent defects surfaced after the TanStack Router migration; all
manifested as "routes stuck loading / navigation locked".

1. Circular import (router → routeTree → __root → hooks → navigationBridge →
   router) broke code-split route chunks via TDZ ("Cannot access
   'rootRouteImport' before initialization"). Introduce routerRef, a leaf module
   holding the router singleton; navigationBridge reads it via getRouter()
   instead of importing the router directly, severing the only route-tree edge
   back to router.ts.

2. Infinite render storm: useAppView returned a fresh object every render (the
   old navigationStore.view was a stable ref), so SidebarMenu's [view] effect
   refired forever → markViewed mutation → cache write → re-render (~50x/2s),
   starving the UI thread and blocking navigation + session start. Memoize
   useAppView on the route's primitive values + stable prefill ref. SidebarMenu
   reads view.taskId (new shape; view.data may be undefined).

3. Blocking loader could hang the router: ensureQueryData(getTask) never
   resolves for optimistic/cloud-pending tasks, leaving the route pending and
   un-navigable. Make the task-detail loader synchronous + cache-only; move the
   cold-deep-link fetch + spinner into the component so a hang only affects that
   view. openTask seeds the detail cache so in-app opens never fetch.

Also adds the per-route loading slot: router context (queryClient),
defaultPendingMs: 0 + defaultPendingComponent (RoutePending), and
createRootRouteWithContext. Navigation now commits instantly with a per-route
pending UI that can later become skeletons.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@adamleithp
Copy link
Copy Markdown
Contributor Author

🩹 Stabilization pass (commit 45fa39692)

Shaking this branch out surfaced three independent defects from the migration, all presenting as "routes stuck loading / navigation locked". All fixed here; verified live (new-task flow, deep links, cross-route nav, mid-load navigate-away).

Fixes

  1. Circular import → broken code-split chunks. router → routeTree.gen → __root → hooks → navigationBridge → router produced a TDZ (Cannot access 'rootRouteImport' before initialization) under autoCodeSplitting, leaving route chunks stuck. Added routerRef (leaf module holding the singleton); navigationBridge now reads getRouter() instead of importing the router, severing the only route-tree edge back to router.ts.

  2. Infinite render storm. useAppView returned a fresh object every render (the old navigationStore.view was a stable ref). SidebarMenu's [view] effect refired forever → markViewed mutation → cache write → re-render (~50×/2s, confirmed via [ipc-rate] warnings), starving the UI thread and blocking both navigation and session start ("Starting task…" hang). Memoized useAppView on the route's primitive values + stable prefill ref; SidebarMenu reads view.taskId.

  3. Blocking loader could hang the router. ensureQueryData(getTask) never resolves for optimistic/cloud-pending tasks → route stuck pending → un-navigable. Task-detail loader is now synchronous + cache-only; the cold-deep-link fetch + spinner moved into the component so a hang only affects that view. openTask seeds the detail cache so in-app opens never fetch.

Also added (per-route loading UX)

Router context: { queryClient }, createRootRouteWithContext, defaultPendingMs: 0 + defaultPendingComponent (RoutePending). Navigation commits instantly with a per-route pending slot that can later become skeletons.


🛣️ Follow-up roadmap (separate PRs after the stack merges)

Full architect review exists locally; condensed:

  • P0 — Delete useAppView. It re-derives a view-enum the router already owns (second source of truth) and requires hand-memoization — the footgun that caused defect chore: config housekeeping #2. Replace with primitive useRouterState selectors + per-route Route.useParams(); move the prefill merge into the /code/ route. Removes the storm class permanently.
  • P1 — Rip out navigationBridge. Migrate call sites to typed <Link> / useNavigate(); they read the router from React context, removing the import cycle structurally so routerRef can also go.
  • P1 — Add defaultErrorComponent + defaultNotFoundComponent. Currently none — a thrown loader/render = blank screen (runtime already warned about it).
  • P1 — Finish loader integration + skeletons. Shared queryOptions per data route (pattern in features/tasks/queries.ts), loaders always non-blocking, per-route pendingComponent skeletons.
  • P2 — Typed search params (validateSearch) as URL state to retire settingsPageStore / taskInputPrefillStore; auth as beforeLoad guards; tune defaultPendingMs once loaders are async.
  • P3 — Regression test asserting the view hook returns a stable reference across re-renders (guards the storm forever) + a lint guard for hooks returning fresh objects.

routeTree.gen.ts intentionally left untouched — local generator output differs from the committed file only by quote/semicolon style; that's a formatter-config cleanup, out of scope here.

@adamleithp
Copy link
Copy Markdown
Contributor Author

Superseded by #2469 — the three-PR stack is collapsed into a single PR (rebased on main, conflicts resolved, CI green: typecheck + biome + 1594 unit tests). No intermediate layer was independently green, so the stack was collapsed rather than fixed three times. Closing in favor of #2469.

@adamleithp adamleithp closed this Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants