Adopt TanStack Router for thread-based chat navigation - #68
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntegrates TanStack Router, introduces route-driven chat layout and thread routes, moves active-thread selection from global state into URL params, adds route tree generation and thread factory, and updates components and store to propagate per-thread context and a threadsHydrated flag. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Router as TanStack Router
participant Root as RootRoute
participant ChatLayout as Chat Layout
participant ThreadRoute as ChatThreadRoute
participant Store
User->>Browser: Open /_chat/$threadId
Browser->>Router: route match
Router->>Root: mount RootRoute with context
Root->>Store: access store (useStore)
Router->>ChatLayout: mount chat layout (Sidebar + Outlet)
Router->>ThreadRoute: mount ThreadRoute (has threadId param)
ThreadRoute->>Store: validate thread exists & check threadsHydrated
alt thread missing or not hydrated
ThreadRoute->>Router: navigate("/")
else
ThreadRoute->>ChatLayout: render ChatView with threadId prop
ChatLayout->>Store: read threads by threadId for rendering
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Adopt TanStack Router to drive thread navigation via URL and render chat under '/_chat/$threadId' with route-scoped events and virtualizationReplace store-based 📍Where to StartStart with router setup and entry in main.tsx, then review route definitions in routes/__root.tsx and routes/_chat.tsx, followed by the parameterized thread view in routes/_chat.$threadId.tsx. Macroscope summarized cdde1ff. |
Greptile SummaryThis PR introduces
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Router as TanStack Router
participant Layout as ChatRouteLayout
participant Index as ChatIndexRouteView
participant Thread as ChatThreadRouteView
participant Store as AppState Store
participant Sidebar
User->>Router: Load app at /
Router->>Layout: Render ChatRouteLayout
Layout->>Index: Render ChatIndexRouteView (Outlet)
Note over Layout: AutoProjectBootstrap runs
Layout->>Store: ADD_THREAD (sets activeThreadId)
Store-->>Layout: state.activeThreadId updated
Layout->>Router: navigate(/$threadId, replace)
Router->>Thread: Render ChatThreadRouteView
Thread->>Store: SET_ACTIVE_THREAD (sync URL → store)
Thread->>Thread: Render ChatView
User->>Sidebar: Click thread in sidebar
Sidebar->>Router: navigate(/$threadId)
Router->>Thread: Render ChatThreadRouteView
Thread->>Store: SET_ACTIVE_THREAD
Thread->>Thread: Render ChatView
User->>Sidebar: Click "New thread"
Sidebar->>Store: ADD_THREAD (via createThread)
Sidebar->>Router: navigate(/$newThreadId)
Router->>Thread: Render ChatThreadRouteView
Last reviewed commit: 6d885eb |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/Sidebar.tsx (1)
495-515:⚠️ Potential issue | 🟠 MajorUse
routeThreadIddirectly in keyboard shortcut handler instead ofstate.activeThreadId.The keyboard handler (line 387) extracts the active thread from
state.activeThreadId, but this depends onSET_ACTIVE_THREADbeing dispatched synchronously when the route changes. If a user triggers a shortcut during the brief window between route navigation and store update—or if a thread is deleted while navigated to it—the handler could target the wrong thread or a deleted one. SincerouteThreadIdis already in scope and represents the definitive navigation state, the handler should derive from it instead.Suggested fix
- const activeThread = state.threads.find((t) => t.id === state.activeThreadId); + const activeThread = state.threads.find((t) => t.id === routeThreadId);- }, [handleNewThread, keybindings, state.activeThreadId, state.projects, state.threads]); + }, [handleNewThread, keybindings, routeThreadId, state.projects, state.threads]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 495 - 515, The keyboard shortcut handler currently reads the active thread from state.activeThreadId which can lag behind route changes; update the handler to use the routeThreadId variable already in scope (instead of state.activeThreadId) so shortcuts always operate against the canonical route-derived thread, and handle the case where routeThreadId may be undefined or reference a deleted thread by early-returning or validating existence (references: routeThreadId, state.activeThreadId, SET_ACTIVE_THREAD, the keyboard handler function).
🧹 Nitpick comments (2)
apps/web/src/components/ChatView.tsx (1)
339-340: Consider passing threadId to nested components that need it.The
OpenInPickercomponent (line 2083) still readsstate.activeThreadIddirectly rather than using thethreadIdprop passed toChatView. While the route component keeps these in sync viaSET_ACTIVE_THREAD, there's a brief window where they could differ. This is low-risk given the synchronization, but for consistency, consider threading thethreadIdprop through to child components that need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 339 - 340, The OpenInPicker component reads state.activeThreadId instead of using the threadId prop passed into ChatView, which can briefly diverge; update ChatView to pass its computed activeThreadId (from activeThread?.id ?? null) down to OpenInPicker (and any other children that currently read state.activeThreadId) via a prop, and update OpenInPicker to accept and use that prop (rename its internal reference from state.activeThreadId to the new prop) so components rely on the threadId coming from ChatView rather than reading global state directly (references: ChatView, activeThread, activeThreadId, threadId, OpenInPicker, SET_ACTIVE_THREAD).apps/web/src/routes/_chat.index.tsx (1)
27-31: Consider extracting title truncation logic.The title truncation pattern (
length > 50 ? slice(0, 50) + "..." : title) is duplicated here and inChatView.tsx(around line 1071). Consider extracting to a shared utility liketruncateTitle(text: string, maxLength = 50): stringfor consistency.💡 Example shared utility
// In a shared utils file export function truncateTitle(text: string, maxLength = 50): string { const trimmed = text.trim(); return trimmed.length > maxLength ? `${trimmed.slice(0, maxLength)}...` : trimmed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.index.tsx` around lines 27 - 31, Extract the duplicated truncation logic into a shared utility function named truncateTitle(text: string, maxLength = 50) and replace the inline logic that builds titleSeed/threadTitle in _chat.index.tsx (the titleSeed and threadTitle expressions) and the corresponding truncation in ChatView (around where titles are trimmed/sliced) to call truncateTitle(draft) or truncateTitle(text); ensure the utility trims input and returns the truncated string with "..." when length exceeds maxLength so both places use the same implementation for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 495-515: The keyboard shortcut handler currently reads the active
thread from state.activeThreadId which can lag behind route changes; update the
handler to use the routeThreadId variable already in scope (instead of
state.activeThreadId) so shortcuts always operate against the canonical
route-derived thread, and handle the case where routeThreadId may be undefined
or reference a deleted thread by early-returning or validating existence
(references: routeThreadId, state.activeThreadId, SET_ACTIVE_THREAD, the
keyboard handler function).
---
Nitpick comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 339-340: The OpenInPicker component reads state.activeThreadId
instead of using the threadId prop passed into ChatView, which can briefly
diverge; update ChatView to pass its computed activeThreadId (from
activeThread?.id ?? null) down to OpenInPicker (and any other children that
currently read state.activeThreadId) via a prop, and update OpenInPicker to
accept and use that prop (rename its internal reference from
state.activeThreadId to the new prop) so components rely on the threadId coming
from ChatView rather than reading global state directly (references: ChatView,
activeThread, activeThreadId, threadId, OpenInPicker, SET_ACTIVE_THREAD).
In `@apps/web/src/routes/_chat.index.tsx`:
- Around line 27-31: Extract the duplicated truncation logic into a shared
utility function named truncateTitle(text: string, maxLength = 50) and replace
the inline logic that builds titleSeed/threadTitle in _chat.index.tsx (the
titleSeed and threadTitle expressions) and the corresponding truncation in
ChatView (around where titles are trimmed/sliced) to call truncateTitle(draft)
or truncateTitle(text); ensure the utility trims input and returns the truncated
string with "..." when length exceeds maxLength so both places use the same
implementation for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/routes/_chat`.$threadId.tsx:
- Around line 13-29: The effect in useEffect that redirects when threadExists is
false breaks deep links during bootstrap because state.threads may be empty
until onServerWelcome populates them; add a threadsHydrated boolean to your
store (set true after onServerWelcome completes) and change the effect in the
component to skip the redirect until threadsHydrated is true (i.e., only check
threadExists and dispatch SET_ACTIVE_THREAD when threadsHydrated && threadId are
present), or alternatively move the thread existence check into a route
beforeLoad/loader that awaits bootstrap data before deciding to redirect. Ensure
you update the store initializer and the code paths that set threads
(onServerWelcome) to flip threadsHydrated so the component can rely on it.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
2be5956 to
a3abdde
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/src/components/ChatView.tsx (1)
2397-2429: Minor redundancy:OpenInPickerre-looks up thread from state.
OpenInPickerreceivesactiveThreadIdas a prop but then performs its ownstate.threads.findlookup (Line 2429). SinceChatViewalready resolved the thread, consider passing the relevant data (e.g.,worktreePath) directly to avoid the duplicate lookup.This is a minor efficiency concern and doesn't affect correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 2397 - 2429, OpenInPicker currently receives activeThreadId but re-queries state.threads.find to get the active thread; instead, change OpenInPicker's props to accept the already-resolved data it needs (e.g., worktreePath or the full activeThread object) from ChatView and remove the internal lookup (the call to state.threads.find inside OpenInPicker). Update the prop type for OpenInPicker and the call site in ChatView to pass the thread.worktreePath (or thread) so OpenInPicker uses that prop directly and no longer imports or reads state to find the thread.apps/web/src/routes/_chat.index.tsx (1)
17-22: Consider removing unnecessaryuseMemo.The
placeholdercomputation is a simple conditional on a boolean. The memoization overhead likely exceeds the cost of the inline ternary.♻️ Suggested simplification
- const placeholder = useMemo(() => { - if (!canCreateThread) { - return "Add a project in the sidebar to start chatting."; - } - return "Start with a goal, bug report, or implementation idea..."; - }, [canCreateThread]); + const placeholder = canCreateThread + ? "Start with a goal, bug report, or implementation idea..." + : "Add a project in the sidebar to start chatting.";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.index.tsx` around lines 17 - 22, The placeholder value is over-memoized: remove the useMemo wrapper and replace "const placeholder = useMemo(() => { ... }, [canCreateThread]);" with a plain conditional assignment like "const placeholder = !canCreateThread ? 'Add a project in the sidebar to start chatting.' : 'Start with a goal, bug report, or implementation idea...';" and then remove the now-unused useMemo import (and any unused React import) so canCreateThread and placeholder remain as simple, readable locals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/routes/__root.tsx`:
- Around line 50-95: The onServerWelcome callback closes over state.projects and
state.threads causing stale checks when the welcome payload is replayed; update
the callback to read latest values via refs (e.g., create projectsRef and
threadsRef that you keep in sync in the reducer effect) or perform the duplicate
check inside a reducer by dispatching an action that includes payload and doing
the existence check in the reducer so you use the latest state; specifically
modify the onServerWelcome handler in the useEffect (and keep bootstrappedRef
logic) to either consult projectsRef.current and threadsRef.current instead of
state.projects/state.threads or to dispatch a single action like
"HANDLE_SERVER_WELCOME" that the reducer uses to add project/thread (using
createThread(projectId) invoked from reducer code or a subsequent effect)
ensuring no duplicates are created.
In `@apps/web/src/store.ts`:
- Around line 124-130: readPersistedState currently sets threadsHydrated to
threads.length > 0 which conflicts with the explicit SET_THREADS_HYDRATED: true
dispatched in __root.tsx; change readPersistedState (the return object in
apps/web/src/store.ts) to set threadsHydrated = true whenever persisted state
has been loaded (regardless of threads.length), keep threads and activeThreadId
as-is, and leave the bootstrap dispatch of SET_THREADS_HYDRATED untouched so
both paths mean "persisted state loaded"; update any logic that needs to know
whether threads exist (e.g., guards in _chat.$threadId.tsx) to check
threads.length > 0 or a new explicit flag (like hasThreads) instead of relying
on threadsHydrated.
---
Nitpick comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 2397-2429: OpenInPicker currently receives activeThreadId but
re-queries state.threads.find to get the active thread; instead, change
OpenInPicker's props to accept the already-resolved data it needs (e.g.,
worktreePath or the full activeThread object) from ChatView and remove the
internal lookup (the call to state.threads.find inside OpenInPicker). Update the
prop type for OpenInPicker and the call site in ChatView to pass the
thread.worktreePath (or thread) so OpenInPicker uses that prop directly and no
longer imports or reads state to find the thread.
In `@apps/web/src/routes/_chat.index.tsx`:
- Around line 17-22: The placeholder value is over-memoized: remove the useMemo
wrapper and replace "const placeholder = useMemo(() => { ... },
[canCreateThread]);" with a plain conditional assignment like "const placeholder
= !canCreateThread ? 'Add a project in the sidebar to start chatting.' : 'Start
with a goal, bug report, or implementation idea...';" and then remove the
now-unused useMemo import (and any unused React import) so canCreateThread and
placeholder remain as simple, readable locals.
| useEffect(() => { | ||
| // Browser mode bootstraps from server welcome. | ||
| // Electron bootstraps from persisted projects via DesktopProjectBootstrap. | ||
| if (isElectron) return; | ||
|
|
||
| return onServerWelcome((payload) => { | ||
| if (bootstrappedRef.current) return; | ||
|
|
||
| // Don't create duplicate projects for the same cwd | ||
| const existing = state.projects.find((project) => project.cwd === payload.cwd); | ||
| if (existing) { | ||
| bootstrappedRef.current = true; | ||
| // Ensure a thread is active | ||
| const existingThread = state.threads.find((thread) => thread.projectId === existing.id); | ||
| if (existingThread && !state.activeThreadId) { | ||
| dispatch({ | ||
| type: "SET_ACTIVE_THREAD", | ||
| threadId: existingThread.id, | ||
| }); | ||
| } | ||
| dispatch({ type: "SET_THREADS_HYDRATED", hydrated: true }); | ||
| return; | ||
| } | ||
|
|
||
| bootstrappedRef.current = true; | ||
|
|
||
| // Create project + thread from server cwd | ||
| const projectId = crypto.randomUUID(); | ||
| dispatch({ | ||
| type: "ADD_PROJECT", | ||
| project: { | ||
| id: projectId, | ||
| name: payload.projectName, | ||
| cwd: payload.cwd, | ||
| model: DEFAULT_MODEL, | ||
| expanded: true, | ||
| scripts: [], | ||
| }, | ||
| }); | ||
| dispatch({ | ||
| type: "ADD_THREAD", | ||
| thread: createThread(projectId), | ||
| }); | ||
| dispatch({ type: "SET_THREADS_HYDRATED", hydrated: true }); | ||
| }); | ||
| }, [state.projects, state.threads, state.activeThreadId, dispatch]); |
There was a problem hiding this comment.
Potential stale closure when server welcome replays.
The onServerWelcome callback captures state.projects and state.threads at effect registration time. If the welcome payload is replayed to a late subscriber (as documented in wsNativeApi.ts), the callback may check against stale project/thread lists, potentially creating duplicates or missing existing matches.
Consider using refs or moving the duplicate check inside the dispatch logic.
🛠️ Possible fix using refs
function AutoProjectBootstrap() {
const { state, dispatch } = useStore();
const bootstrappedRef = useRef(false);
+ const projectsRef = useRef(state.projects);
+ const threadsRef = useRef(state.threads);
+ const activeThreadIdRef = useRef(state.activeThreadId);
+
+ useEffect(() => {
+ projectsRef.current = state.projects;
+ threadsRef.current = state.threads;
+ activeThreadIdRef.current = state.activeThreadId;
+ }, [state.projects, state.threads, state.activeThreadId]);
useEffect(() => {
if (isElectron) return;
return onServerWelcome((payload) => {
if (bootstrappedRef.current) return;
- const existing = state.projects.find((project) => project.cwd === payload.cwd);
+ const existing = projectsRef.current.find((project) => project.cwd === payload.cwd);
if (existing) {
bootstrappedRef.current = true;
- const existingThread = state.threads.find((thread) => thread.projectId === existing.id);
- if (existingThread && !state.activeThreadId) {
+ const existingThread = threadsRef.current.find((thread) => thread.projectId === existing.id);
+ if (existingThread && !activeThreadIdRef.current) {
dispatch({
type: "SET_ACTIVE_THREAD",
threadId: existingThread.id,
});
}
// ...
}
// ...
});
- }, [state.projects, state.threads, state.activeThreadId, dispatch]);
+ }, [dispatch]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/__root.tsx` around lines 50 - 95, The onServerWelcome
callback closes over state.projects and state.threads causing stale checks when
the welcome payload is replayed; update the callback to read latest values via
refs (e.g., create projectsRef and threadsRef that you keep in sync in the
reducer effect) or perform the duplicate check inside a reducer by dispatching
an action that includes payload and doing the existence check in the reducer so
you use the latest state; specifically modify the onServerWelcome handler in the
useEffect (and keep bootstrappedRef logic) to either consult projectsRef.current
and threadsRef.current instead of state.projects/state.threads or to dispatch a
single action like "HANDLE_SERVER_WELCOME" that the reducer uses to add
project/thread (using createThread(projectId) invoked from reducer code or a
subsequent effect) ensuring no duplicates are created.
| return { | ||
| ...hydrated, | ||
| threads, | ||
| activeThreadId, | ||
| threadsHydrated: threads.length > 0, | ||
| diffOpen: false, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "threadsHydrated|SET_THREADS_HYDRATED" --type ts --type tsxRepository: pingdotgg/codething-mvp
Length of output: 94
🏁 Script executed:
cat -n apps/web/src/store.ts | head -150 | tail -50Repository: pingdotgg/codething-mvp
Length of output: 2031
🏁 Script executed:
rg -n "threadsHydrated" --type tsRepository: pingdotgg/codething-mvp
Length of output: 1024
🏁 Script executed:
rg -n "SET_THREADS_HYDRATED" --type tsRepository: pingdotgg/codething-mvp
Length of output: 578
🏁 Script executed:
cat -n apps/web/src/store.ts | head -110Repository: pingdotgg/codething-mvp
Length of output: 4496
🏁 Script executed:
cat -n apps/web/src/routes/__root.tsx | sed -n '60,100p'Repository: pingdotgg/codething-mvp
Length of output: 1509
🏁 Script executed:
cat -n apps/web/src/routes/__root.tsx | sed -n '80,130p'Repository: pingdotgg/codething-mvp
Length of output: 1852
🏁 Script executed:
cat -n apps/web/src/store.test.ts | sed -n '630,650p'Repository: pingdotgg/codething-mvp
Length of output: 673
🏁 Script executed:
cat -n apps/web/src/store.test.ts | sed -n '534,550p'Repository: pingdotgg/codething-mvp
Length of output: 550
🏁 Script executed:
cat -n apps/web/src/routes/__root.tsx | sed -n '1,50p'Repository: pingdotgg/codething-mvp
Length of output: 1806
🏁 Script executed:
cat -n apps/web/src/store.ts | sed -n '450,470p'Repository: pingdotgg/codething-mvp
Length of output: 848
🏁 Script executed:
cat -n apps/web/src/routes/_chat.$threadId.tsx | sed -n '15,40p'Repository: pingdotgg/codething-mvp
Length of output: 129
🏁 Script executed:
find apps/web/src/routes -name "*chat*" -type fRepository: pingdotgg/codething-mvp
Length of output: 172
🏁 Script executed:
cat -n "apps/web/src/routes/_chat.\$threadId.tsx" | sed -n '15,40p'Repository: pingdotgg/codething-mvp
Length of output: 816
🏁 Script executed:
cat -n apps/web/src/store.ts | sed -n '175,210p'Repository: pingdotgg/codething-mvp
Length of output: 1521
🏁 Script executed:
rg -n "readPersistedState" --type tsRepository: pingdotgg/codething-mvp
Length of output: 240
🏁 Script executed:
cat -n apps/web/src/store.ts | sed -n '895,910p'Repository: pingdotgg/codething-mvp
Length of output: 629
🏁 Script executed:
rg -n "threadsHydrated" -B 2 -A 2 --type tsRepository: pingdotgg/codething-mvp
Length of output: 3539
Semantic inconsistency: threadsHydrated has conflicting meanings across initialization paths.
readPersistedState() ties threadsHydrated to thread existence (threads.length > 0), but bootstrap effects in __root.tsx explicitly dispatch SET_THREADS_HYDRATED: true regardless of thread count. This creates an edge case where the flag's meaning is ambiguous.
However, the guard in _chat.$threadId.tsx includes threadsHydrated in its dependency array, so it re-runs whenever the state changes. When bootstrap dispatches the explicit SET_THREADS_HYDRATED: true, the guard is not permanently stuck—it re-evaluates.
The suggested fix improves semantics by decoupling "hydration complete" from "threads exist":
-threadsHydrated: threads.length > 0,
+threadsHydrated: true,This makes threadsHydrated consistently mean "persisted state loaded successfully" across both paths (localStorage and bootstrap), and allows checking thread existence separately when needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| ...hydrated, | |
| threads, | |
| activeThreadId, | |
| threadsHydrated: threads.length > 0, | |
| diffOpen: false, | |
| }; | |
| return { | |
| ...hydrated, | |
| threads, | |
| activeThreadId, | |
| threadsHydrated: true, | |
| diffOpen: false, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/store.ts` around lines 124 - 130, readPersistedState currently
sets threadsHydrated to threads.length > 0 which conflicts with the explicit
SET_THREADS_HYDRATED: true dispatched in __root.tsx; change readPersistedState
(the return object in apps/web/src/store.ts) to set threadsHydrated = true
whenever persisted state has been loaded (regardless of threads.length), keep
threads and activeThreadId as-is, and leave the bootstrap dispatch of
SET_THREADS_HYDRATED untouched so both paths mean "persisted state loaded";
update any logic that needs to know whether threads exist (e.g., guards in
_chat.$threadId.tsx) to check threads.length > 0 or a new explicit flag (like
hasThreads) instead of relying on threadsHydrated.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/routes/__root.tsx`:
- Around line 84-124: The onServerWelcome callback closes over stale state
(state.projects, state.threads, state.activeThreadId) which can cause
duplicates/missed matches; fix by reading live values via refs or delegating the
existence logic into the reducer: keep bootstrappedRef for one-time bootstrap,
replace direct checks of state.projects/state.threads/state.activeThreadId
inside the onServerWelcome handler with current refs (e.g., projectsRef.current,
threadsRef.current, activeThreadIdRef.current) or dispatch a single action like
"ENSURE_PROJECT_FROM_SERVER" with the payload and perform the existence check
and thread activation within the reducer (or reducer-handled helper) so the
latest state is used atomically.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/routes/__root.tsx`:
- Around line 92-120: The onServerWelcome callback closes over state.projects
causing a stale-closure bug; change to read the latest projects from a ref
(e.g., projectsRef.current) inside the callback instead of using state.projects,
update that ref whenever state.projects changes, and keep bootstrappedRef and
dispatch usage intact; ensure the effect registers onServerWelcome uses the
ref-based check for existing project (compare payload.cwd against
projectsRef.current) before dispatching ADD_PROJECT and SET_THREADS_HYDRATED so
late subscriber replays see current project state.
In `@apps/web/src/store.ts`:
- Around line 119-124: In readPersistedState (apps/web/src/store.ts) change the
semantic of threadsHydrated so it reflects "persisted state loaded" rather than
"threads exist": replace the current threadsHydrated: threads.length > 0 with
threadsHydrated: true so that successful hydration always marks threadsHydrated
as true (bootstrap can still dispatch SET_THREADS_HYDRATED to indicate
completion); update any related return object that includes threads and diffOpen
accordingly.
| if (event.method === "turn/completed") { | ||
| void invalidateGitQueries(queryClient); | ||
| } | ||
| if (!activeThreadId) return; |
There was a problem hiding this comment.
Provider events silently dropped when no thread active
High Severity
The if (!activeThreadId) return; guard in EventRouter causes ALL APPLY_EVENT dispatches to be skipped when the user is on the index route (no threadId in the URL). The reducer's findThreadBySessionId already correctly routes events to the right thread by sessionId, so activeThreadId is only needed for the lastVisitedAt optimization. By short-circuiting entirely, background threads lose all messages, session state updates, and completion events whenever the user navigates away. The dispatch needs to fire unconditionally, with activeThreadId passed as optional context.
Additional Locations (1)
| worktreePath: null, | ||
| }, | ||
| }); | ||
| dispatch({ type: "SET_THREADS_HYDRATED", hydrated: true }); |
There was a problem hiding this comment.
Browser bootstrap no longer creates initial thread
Medium Severity
AutoProjectBootstrap previously created both a project and an initial thread on first browser-mode load. The refactor removed the ADD_THREAD dispatch while the comment on line 105 still says "Create project + thread from server cwd." New browser users get a project but no thread, landing on a blank "Select a thread" placeholder with no automatic way to start chatting.
| }; | ||
| } | ||
|
|
||
| case "SET_ACTIVE_THREAD": { |
There was a problem hiding this comment.
Thread visit no longer updates lastVisitedAt timestamp
Medium Severity
The removed SET_ACTIVE_THREAD action was the only place lastVisitedAt was updated when a user navigated to a thread. Now lastVisitedAt is only set on thread creation and turn/completed events. The sidebar's hasUnseenCompletion compares latestTurnCompletedAt to lastVisitedAt to show a "Completed" pill — without updating lastVisitedAt on navigation, this indicator can never be dismissed by visiting the thread.
Additional Locations (1)
| outDir: "dist", | ||
| emptyOutDir: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Duplicate vite.config.js committed alongside TypeScript version
Low Severity
vite.config.js is a new file with the same configuration as the existing vite.config.ts. It appears to be a transpiled or auto-generated copy. Having both files is confusing and Vite may pick one over the other unexpectedly depending on resolution order.
This comment has been minimized.
This comment has been minimized.
- add route-based chat layout with index and `/$threadId` views - navigate sidebar thread actions through router instead of store-only selection - extract shared thread creation into `threadFactory` and wire new router deps
- Switch web router setup to TanStack file routes and generated `routeTree.gen.ts` - Move app bootstrap/event wiring into new root route component - Enable TanStack router Vite plugin and ignore generated route tree in oxlint
Co-authored-by: codex <codex@users.noreply.github.com>
- remove `activeThreadId` from web store state and persistence schema - drive thread-scoped event handling and toast visibility from router params - simplify chat index/route flow and add explicit `apps/web/vite.config.js`
de9c059 to
5817848
Compare
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (470cabc110)diff --git a/apps/web/src/routes/_chat.tsx b/apps/web/src/routes/_chat.tsx
--- a/apps/web/src/routes/_chat.tsx
+++ b/apps/web/src/routes/_chat.tsx
@@ -1,6 +1,6 @@
import { Outlet, createFileRoute } from "@tanstack/react-router";
-import DiffPanel from "../components/DiffPanel";
+import DiffPanel, { DiffWorkerPoolProvider } from "../components/DiffPanel";
import Sidebar from "../components/Sidebar";
import { useStore } from "../store";
@@ -11,7 +11,11 @@
<div className="flex h-screen overflow-hidden bg-background text-foreground isolate">
<Sidebar />
<Outlet />
- {state.diffOpen && <DiffPanel />}
+ {state.diffOpen && (
+ <DiffWorkerPoolProvider>
+ <DiffPanel />
+ </DiffWorkerPoolProvider>
+ )}
</div>
);
} |
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
| ); | ||
| const DIFF_INLINE_LAYOUT_MEDIA_QUERY = "(max-width: 1180px)"; | ||
|
|
||
| const DiffPanelWrapper = (props: { children: ReactNode; sheet: boolean }) => { |
There was a problem hiding this comment.
🟡 Medium
routes/_chat.tsx:17 Conditionally returning <Sheet> vs <aside> as root elements causes React to unmount/remount children when crossing the 1180px breakpoint, losing scroll position and re-initializing workers. Consider always rendering both elements and toggling visibility, or lifting the shared children into a portal that persists across layout changes.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/routes/_chat.tsx around line 17:
Conditionally returning `<Sheet>` vs `<aside>` as root elements causes React to unmount/remount children when crossing the 1180px breakpoint, losing scroll position and re-initializing workers. Consider always rendering both elements and toggling visibility, or lifting the shared children into a portal that persists across layout changes.
Evidence trail:
Viewed `apps/web/src/routes/_chat.tsx` lines 12-53 at commit `36234e70c6` (conditional return of `<Sheet>` vs `<aside>` in `DiffPanelWrapper`).
- Add `@tanstack/react-virtual` to web dependencies - Render chat timeline with virtualized rows for work logs, messages, and working indicator - Replace end-of-list sentinel approach with scroll-container-based virtualization
1 participant
Summary
@tanstack/react-routerand replace the monolithicApplayout rendering with a router-driven app shell./and/$threadId) with a shared chat layout.ChatViewto acceptthreadIdas a prop so route state drives the active thread.createThreadfactory and reuse it across bootstrap and thread creation flows.Testing
Note
Medium Risk
This is a broad navigation/state refactor that changes how the active thread is determined and how events/toasts are scoped, so regressions could affect thread routing and diff targeting despite being largely UI/state-layer changes.
Overview
Migrates the web app from a monolithic
Appshell to TanStack Router with/and/$threadIdroutes, making the URL the source of truth for the active chat thread (including Electron hash history support).Refactors thread selection/creation/deletion flows to navigate via the router instead of storing
activeThreadIdin the app state; addsthreadsHydratedgating for route validity, updates toasts and git controls to be thread-aware via route params, and updatesDiffPanelto resolve its target thread from either stored diff state or the route.Improves chat rendering performance by virtualizing the message timeline with
@tanstack/react-virtual, and adds small utilities/tests (createThread,truncateTitle) plus build/lint updates (router Vite plugin, ignore generatedrouteTree.gen.ts).Written by Cursor Bugbot for commit cdde1ff. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Refactor
Tests