feat: Migrate web UI to TanStack Router#19
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the web application's routing from a custom hash-based implementation to TanStack Router. Key changes include the introduction of a file-based routing system, refactoring the main layout to utilize router-native components like Link and Outlet, and updating the CLI to support path-based navigation. Feedback focuses on improving the idiomatic use of the router by suggesting useMatchRoute for tab state management and recommending the extraction of duplicated logic for calculating viewed chapter counts into a shared utility.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
dastratakos
changed the title
TanStack Router's `ParsedLocation.hash` strips the leading `#`
(see router-core/src/router.ts: `hash: decodePath(hash.slice(1)).path`),
so the previous `location.hash.startsWith("#/runs/")` check never matched
and the redirect was dead. `slice(1)` was also stripping the leading `/`
rather than a (non-existent) `#`.
Match `/runs/` and pass `location.hash` through unchanged so legacy
`/#/runs/abc` URLs redirect to `/runs/abc`.
Both `PullRequestLayout` (for the tab count chip) and `ChaptersRoute` (for prop-drilling into ChaptersIndexPage) ran the same useMemo loop to count viewed chapters. The duplication was introduced by the TanStack Router migration since `Outlet` doesn't pass props from layout to child. Extract the loop into a pure helper next to `useViewStateData` and call it from both sites.
`queryClient` was a module-level constant but the router was created fresh on every `getRouter()` call, which is asymmetric and risks multiple routers sharing one queryClient (e.g. under HMR or in tests). Match TanStack Router's canonical setup (https://tanstack.com/router/latest/docs/framework/react/quick-start) by exporting `router` directly. Drop the `getRouter`/`getQueryClient` factory wrappers in favor of named exports.
Pre-ship there are no users with bookmarked `/#/runs/...` URLs to preserve. The earlier fix (041833a) made the redirect actually fire, but the simpler answer is to delete it entirely — YAGNI.
|
|
||
| export const Route = createFileRoute("/")({ | ||
| component: NoRunSelected, | ||
| }); |
There was a problem hiding this comment.
Legacy hash URL redirect is missing
Medium Severity
The PR description states it preserves "a legacy #/runs/... redirect," and the old useHashRunId hook is deleted, but no redirect from hash-based URLs to path-based URLs exists anywhere in the codebase. A user arriving at /#/runs/abc (e.g. from browser history or a cached link) lands on the / route and sees "No run selected" instead of being redirected to /runs/abc. The PR discussion references a fix in commit 041833a, but the redirect logic is absent from the final code.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit db957b3. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3b7d776. Configure here.
| import { createRouter } from "@tanstack/react-router"; | ||
| import { routeTree } from "./routeTree.gen"; | ||
|
|
||
| export const queryClient = new QueryClient(); |
There was a problem hiding this comment.
QueryClient lost default staleTime affecting view state
Low Severity
The QueryClient previously had staleTime: 30_000 as a global default, with a comment noting it "keeps a single tab from re-fetching too eagerly." The new QueryClient() has no defaults. While useChapters and useDiffPatch received explicit staleTime: Number.POSITIVE_INFINITY, useViewStateData was not updated and now defaults to staleTime: 0, causing it to refetch on every component mount and window focus event.
Reviewed by Cursor Bugbot for commit 3b7d776. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b7d776d10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const router = createRouter({ | ||
| routeTree, |
There was a problem hiding this comment.
#/runs/... entry URLs
This router initialization drops support for old hash-based links, because it uses default browser history and no startup migration from window.location.hash to a path route. After this commit, opening a previously shared URL like http://127.0.0.1:5391/#/runs/<id> lands on / and shows “No run selected” instead of the run, since the hash is ignored by route matching. This is a user-facing regression for existing bookmarks/links from earlier stage-cli show output and should be handled via a one-time hash redirect or explicit hash history compatibility.
Useful? React with 👍 / 👎.
1 participant
Summary
Migrate the stage-cli web UI from hash-tab state to TanStack Router so Files changed lives at
/runs/:runId/files.Changes
stage-cli showto open/runs/:runId.#/runs/...redirect.Testing
pnpm typecheck && pnpm lint && pnpm test && pnpm build