feat(code): TanStack Router architectural cleanup#2456
Closed
adamleithp wants to merge 1 commit into
Closed
Conversation
Follow-up to #2455. Lands the high-impact cleanups from that PR's review; defers the navigationStore consumer migration (~91 call sites) and full store deletion to follow-up PRs that can be reviewed independently. PR 2 cleanups: - Check in routeTree.gen.ts; .gitattributes marks it as linguist-generated so reviewers don't get noise from regen diffs. Fresh-clone typecheck now works without a prior dev/build run. - TanStack Router DevTools moved to lazy() + Suspense behind import.meta.env.DEV so the ~50KB devtools chunk is excluded from the prod bundle (was getting bundled despite the conditional render). - Drop `as string` cast in syncToRouter; cleaner narrowing via early return. - settings close() now uses router.history.back() instead of hard router.navigate("/code"), so dismissing settings returns to the prior page (e.g. /code/inbox) rather than always landing at /code. - settings open() no longer issues window.history.pushState — it was fighting hashHistory's own state management. Circular import fix: - New apps/code/src/renderer/navigationBridge.ts holds all router.navigate calls used by Zustand stores. Stores import the bridge, not the router. Breaks the store → router → routeTree → __root → store cycle that was working only because ES module bindings are live. - SettingsCategory moved to features/settings/types.ts to keep type imports cycle-free. Settings as a real route (PR 3): - New SettingsPanel.tsx holds the visual content (sidebar + sections). - /settings/$category route renders SettingsPanel as a full-screen page. - __root.tsx detects /settings/* matches and skips the app chrome (HeaderRow, MainSidebar, SpaceSwitcher, TourOverlay, HedgehogMode) for those routes — settings takes the full window. - SettingsDialog.tsx becomes a thin overlay wrapper around SettingsPanel, used only in pre-router shells (AiApprovalScreen) where RouterProvider isn't mounted yet. Cold-boot URL restore (partial PR 5): - router.ts writes window.location.hash to localStorage on every router resolution and restores it synchronously before createTanStackRouter reads location. Eliminates the TaskInput flash users saw on cold start when their last route was a task or inbox view. New taskInputPrefillStore scaffold (unused yet, prep for PR 6) that will hold transient TaskInput prefill (initialPrompt, reportAssociation, etc.) when navigationStore is deleted. Deferred to follow-up PRs (documented in the PR body): - PR 6: full navigationStore deletion. 91 call sites, ~25 read view.data (the Task object), need queryClient module ref to populate view.data from cache when URL is the navigation source. - PR 7: settingsDialogStore consumer migration to useNavigate/<Link/>. - Route loaders for TaskDetail/Inbox, menu Cmd+[/] hotkeys. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
11 tasks
Contributor
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
apps/code/src/renderer/features/settings/components/SettingsDialog.tsx:14-23
**Popstate condition is always-false guard — dead code**
`open()` no longer calls `window.history.pushState({ settingsOpen: true })`, so `window.history.state?.settingsOpen` is always `undefined` / falsy. The condition `!window.history.state?.settingsOpen` is therefore always `true`, meaning any `popstate` event while the overlay is open (e.g. the user pressing Electron's back button in `AiApprovalScreen`) will unconditionally close the dialog — which was never the original intent.
Either remove this handler entirely (the overlay is now the only remaining caller and there's no pushed state to guard against), or replace the condition with one that reflects the current behaviour.
### Issue 2 of 4
apps/code/src/renderer/features/settings/components/SettingsDialog.tsx:25-35
**Redundant Escape handler — `SettingsPanel` already owns it**
`SettingsPanel` now registers `useHotkeys("escape", close, { enabled: true, ... })` unconditionally while it is mounted. When the overlay is open, `SettingsPanel` is rendered inside `SettingsDialog`, so Escape already fires `close()` via the hotkey. This second listener on `window` fires a duplicate `close()` call immediately after. The guard `if (!wasOpen) return` in `close()` makes it harmless, but it violates the OnceAndOnlyOnce rule. This `keydown` effect can be dropped from `SettingsDialog`.
### Issue 3 of 4
apps/code/src/renderer/routes/__root.tsx:46-48
**Import declarations after module-level `const`**
The `import` statements for `GlobalEventHandlers`, `useNewTaskDeepLink`, and `useTaskDeepLink` (lines 46–48) appear after the `const TanStackRouterDevtools = ...` declaration at line 38. ES module imports are hoisted so this is functionally correct, but placing `import` declarations after non-import code is unconventional and may trip static analysis. Moving the three imports above line 35 (joining the existing import block) would keep the file idiomatic.
### Issue 4 of 4
apps/code/src/renderer/features/task-detail/stores/taskInputPrefillStore.ts:32-39
The team convention is to use `null` rather than `undefined` to express an intentional absence. Both fields being cleared here (`reportAssociation`, `initialCloudRepository`) represent a deliberate removal of a value, not a missing optional. Using `null` makes that intent explicit and avoids potential confusion with fields that are simply absent from a partial update.
```suggestion
clearReportAssociation: () =>
set((s) => ({
prefill: {
...s.prefill,
reportAssociation: null,
initialCloudRepository: null,
},
})),
```
Reviews (1): Last reviewed commit: "feat(code): TanStack Router architectura..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
14
to
23
| useEffect(() => { | ||
| if (!isOpen) return; | ||
| const handlePopState = () => { | ||
| if (isOpen && !window.history.state?.settingsOpen) { | ||
| if (!window.history.state?.settingsOpen) { | ||
| useSettingsDialogStore.setState({ isOpen: false }); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("popstate", handlePopState); | ||
| return () => window.removeEventListener("popstate", handlePopState); | ||
| }, [isOpen]); |
Contributor
There was a problem hiding this comment.
open() no longer calls window.history.pushState({ settingsOpen: true }), so window.history.state?.settingsOpen is always undefined / falsy. The condition !window.history.state?.settingsOpen is therefore always true, meaning any popstate event while the overlay is open (e.g. the user pressing Electron's back button in AiApprovalScreen) will unconditionally close the dialog — which was never the original intent.
Either remove this handler entirely (the overlay is now the only remaining caller and there's no pushed state to guard against), or replace the condition with one that reflects the current behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/SettingsDialog.tsx
Line: 14-23
Comment:
**Popstate condition is always-false guard — dead code**
`open()` no longer calls `window.history.pushState({ settingsOpen: true })`, so `window.history.state?.settingsOpen` is always `undefined` / falsy. The condition `!window.history.state?.settingsOpen` is therefore always `true`, meaning any `popstate` event while the overlay is open (e.g. the user pressing Electron's back button in `AiApprovalScreen`) will unconditionally close the dialog — which was never the original intent.
Either remove this handler entirely (the overlay is now the only remaining caller and there's no pushed state to guard against), or replace the condition with one that reflects the current behaviour.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+25
to
+35
| useEffect(() => { | ||
| if (!isOpen) return; | ||
| const handleEscape = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| e.preventDefault(); | ||
| close(); | ||
| } | ||
| }; | ||
| window.addEventListener("keydown", handleEscape); | ||
| return () => window.removeEventListener("keydown", handleEscape); | ||
| }, [isOpen, close]); |
Contributor
There was a problem hiding this comment.
SettingsPanel already owns it
SettingsPanel now registers useHotkeys("escape", close, { enabled: true, ... }) unconditionally while it is mounted. When the overlay is open, SettingsPanel is rendered inside SettingsDialog, so Escape already fires close() via the hotkey. This second listener on window fires a duplicate close() call immediately after. The guard if (!wasOpen) return in close() makes it harmless, but it violates the OnceAndOnlyOnce rule. This keydown effect can be dropped from SettingsDialog.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/SettingsDialog.tsx
Line: 25-35
Comment:
**Redundant Escape handler — `SettingsPanel` already owns it**
`SettingsPanel` now registers `useHotkeys("escape", close, { enabled: true, ... })` unconditionally while it is mounted. When the overlay is open, `SettingsPanel` is rendered inside `SettingsDialog`, so Escape already fires `close()` via the hotkey. This second listener on `window` fires a duplicate `close()` call immediately after. The guard `if (!wasOpen) return` in `close()` makes it harmless, but it violates the OnceAndOnlyOnce rule. This `keydown` effect can be dropped from `SettingsDialog`.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
46
to
48
| import { GlobalEventHandlers } from "../components/GlobalEventHandlers"; | ||
| import { useNewTaskDeepLink } from "../hooks/useNewTaskDeepLink"; | ||
| import { useTaskDeepLink } from "../hooks/useTaskDeepLink"; |
Contributor
There was a problem hiding this comment.
const
The import statements for GlobalEventHandlers, useNewTaskDeepLink, and useTaskDeepLink (lines 46–48) appear after the const TanStackRouterDevtools = ... declaration at line 38. ES module imports are hoisted so this is functionally correct, but placing import declarations after non-import code is unconventional and may trip static analysis. Moving the three imports above line 35 (joining the existing import block) would keep the file idiomatic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/routes/__root.tsx
Line: 46-48
Comment:
**Import declarations after module-level `const`**
The `import` statements for `GlobalEventHandlers`, `useNewTaskDeepLink`, and `useTaskDeepLink` (lines 46–48) appear after the `const TanStackRouterDevtools = ...` declaration at line 38. ES module imports are hoisted so this is functionally correct, but placing `import` declarations after non-import code is unconventional and may trip static analysis. Moving the three imports above line 35 (joining the existing import block) would keep the file idiomatic.
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!
Comment on lines
+32
to
+39
| clearReportAssociation: () => | ||
| set((s) => ({ | ||
| prefill: { | ||
| ...s.prefill, | ||
| reportAssociation: undefined, | ||
| initialCloudRepository: undefined, | ||
| }, | ||
| })), |
Contributor
There was a problem hiding this comment.
null rather than undefined to express an intentional absence. Both fields being cleared here (reportAssociation, initialCloudRepository) represent a deliberate removal of a value, not a missing optional. Using null makes that intent explicit and avoids potential confusion with fields that are simply absent from a partial update.
Suggested change
| clearReportAssociation: () => | |
| set((s) => ({ | |
| prefill: { | |
| ...s.prefill, | |
| reportAssociation: undefined, | |
| initialCloudRepository: undefined, | |
| }, | |
| })), | |
| clearReportAssociation: () => | |
| set((s) => ({ | |
| prefill: { | |
| ...s.prefill, | |
| reportAssociation: null, | |
| initialCloudRepository: null, | |
| }, | |
| })), |
Rule Used: Use 'null' instead of 'undefined' to indicate an i... (source)
Learned From
PostHog/posthog#32556
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/task-detail/stores/taskInputPrefillStore.ts
Line: 32-39
Comment:
The team convention is to use `null` rather than `undefined` to express an intentional absence. Both fields being cleared here (`reportAssociation`, `initialCloudRepository`) represent a deliberate removal of a value, not a missing optional. Using `null` makes that intent explicit and avoids potential confusion with fields that are simply absent from a partial update.
```suggestion
clearReportAssociation: () =>
set((s) => ({
prefill: {
...s.prefill,
reportAssociation: null,
initialCloudRepository: null,
},
})),
```
**Rule Used:** Use 'null' instead of 'undefined' to indicate an i... ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=0631667e-2260-42be-a589-0d8da9ef007d))
**Learned From**
[PostHog/posthog#32556](https://github.com/PostHog/posthog/pull/32556)
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!
46 tasks
Contributor
Author
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
1 participant
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.
Stacks on top of #2455. Lands the high-impact cleanups from that PR's architect review; defers the navigationStore consumer migration and full store deletion to follow-up PRs that can be reviewed independently.
What's in this PR
PR 2 cleanups
routeTree.gen.ts..gitattributesmarks it linguist-generated so reviewers can collapse regen diffs. Fresh-clonepnpm typecheckworks without a priorpnpm dev/build.lazy()+<Suspense/>behindimport.meta.env.DEV. The ~50KB devtools chunk no longer ships in the prod bundle (was getting included despite the conditional render).as stringcast insyncToRouter— narrower types via early-return.close()now callsrouter.history.back()instead of hardrouter.navigate("/code"). Dismissing settings returns the user to their prior page (e.g./code/inbox).open()no longer issueswindow.history.pushState— it was colliding withhashHistory's own state management.Break the circular import
apps/code/src/renderer/navigationBridge.tsowns allrouter.navigatecalls used by Zustand stores. Stores import the bridge, not@renderer/router. Breaks thestore → router → routeTree → __root → storecycle that previously worked only because of ES module live bindings.SettingsCategorymoved tofeatures/settings/types.tsso the type can be shared without crossing the cycle.Settings as a real route (PR 3)
SettingsPanel.tsxholds the visual content (left nav + sections). Identical UI to the previousSettingsDialogcontent, minus thefixed inset-0 z-[100]overlay positioning./settings/$categoryroute renders<SettingsPanel/>as a full-screen page.__root.tsxdetects/settings/*matches viauseRouterStateand renders without app chrome for those routes (noHeaderRow, noMainSidebar, noSpaceSwitcher, noTourOverlay, noHedgehogMode). Settings takes the full window.SettingsDialog.tsxis now a thin overlay wrapper aroundSettingsPanel, used only in pre-router shells (AiApprovalScreen) whereRouterProviderisn't mounted yet.Cold-boot URL restore (partial PR 5)
router.tswriteswindow.location.hashtolocalStorageon every router resolution and restores it synchronously beforecreateTanStackRouterreads location. Eliminates theTaskInputflash users saw on cold start when their last route was a task or inbox view. (Electron'sBrowserWindow.loadFileresets the URL hash on relaunch, so this is the workaround.)Scaffolding for PR 6
taskInputPrefillStore.ts(unused yet). Will hold transient TaskInput prefill state (initialPrompt,reportAssociation, etc.) whennavigationStoreis deleted. Shipped as a separate file now so PR 6's diff is purely consumer migration.Deferred to follow-up PRs
PR 6 — Delete
navigationStoreWhy deferred: 91 call sites. ~25 read
view.data(the fullTaskobject, not just the id). Safely porting requires aqueryClientmodule ref soview.datacan be populated from React Query's task cache when the URL is the navigation source (deep links, back/forward).Plan:
apps/code/src/renderer/queryClientRef.ts—Providerspopulates a module-level ref at mount.navigationStore.tsas a derived facade:viewcomes from router state + the query-cache lookup; actions callnavigationBridge. Drop persistence, history stack,hydrateTask..getState()/.setState()callers (~6 files).syncToRouter(dead code once the store no longer sets view).PR 7 — Delete
settingsDialogStoreWhy deferred: depends on PR 6 landing so the patterns are consistent. ~20 call sites; cleanly maps to
useNavigate/<Link to=\"/settings/\$category\"/>.Polish (PR 8 or distributed)
TaskDetail/Inbox(currently each route doesuseTasks().findin render).router.history.go(±1).autoCodeSplitting: trueonce nav is stable (Electron loads from local disk — 12 tiny chunks may net negative on parse).Test plan
Automated (passing):
pnpm exec tsc -p tsconfig.web.json --noEmit)Manual (please verify):
registerBillingSubscriptions) still works — note this now yanks user to /settings/plan-usage; verify the UX is acceptable or that we add a "silent" flag in PR 6@tanstack/react-router-devtools(checkdist/assets/index-*.jsfor the absence ofTanStackRouterDevtools)🤖 Generated with Claude Code