Retarget open diff panel to the visited thread#78
Merged
juliusmarminge merged 5 commits intomainfrom Feb 19, 2026
Merged
Conversation
- Prefer route thread id over stored diff thread when rendering diffs - When visiting a different thread with diff open, retarget diff state and clear stale turn/file selection - Add reducer test coverage for open-diff retargeting behavior
Contributor
Drive diff panel open/close and target selection via thread route URL search in
|
|
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:
No actionable comments were generated in the recent review. 🎉 WalkthroughDiff UI control moved from Redux state to URL/search parameters; diff-related store fields and actions were removed. Routes and components now parse/validate diff search params and drive diff UI via navigation. DiffWorkerPoolProvider was extracted to a separate component and wired into layout. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant ChatView
participant DiffPanel
participant WorkerPool
User->>Router: Update URL search (open diff or select turn/file)
Router->>ChatView: render with rawSearch
ChatView->>ChatView: parseDiffRouteSearch(rawSearch)
alt diffOpen === "1"
ChatView->>Router: (may) lazy-load DiffPanel route UI
Router->>DiffPanel: mount with diff params
DiffPanel->>WorkerPool: request diff processing (worker pool)
WorkerPool-->>DiffPanel: diff results
DiffPanel-->>User: display diffs
else diff closed
ChatView-->>User: render chat only
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 |
Greptile SummaryMakes the diff panel follow the currently routed/visited thread instead of persisting to a previously stored diff target. When the diff is open and navigation changes threads, the panel automatically retargets to show diffs for the newly visited thread.
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User navigates to thread page] --> B[Route threadId changes]
B --> C{Is diff panel open?}
C -->|No| D[No action needed]
C -->|Yes| E{Does route threadId match current diffThreadId?}
E -->|Yes| F[No action needed]
E -->|No| G[DiffPanel useEffect triggers]
G --> H[Dispatch SET_DIFF_TARGET with routeThreadId]
H --> I[Update diffThreadId to new thread]
I --> J[Clear diffTurnId and diffFilePath]
J --> K[Diff panel shows new thread's diffs]
L[MARK_THREAD_VISITED dispatched] --> M{Is diff open?}
M -->|No| N[Only update lastVisitedAt]
M -->|Yes| O{Does diffThreadId match visited threadId?}
O -->|Yes| P[Only update lastVisitedAt]
O -->|No| Q[Retarget diff to visited thread]
Q --> R[Update diffThreadId to visited thread]
R --> S[Clear diffTurnId and diffFilePath]
S --> T[Diff panel follows visited thread]
Last reviewed commit: b253d1f |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/store.test.ts (1)
836-862: Minor test setup redundancy.The test creates a thread via
makeState(makeThread(...))but immediately overwrites thethreadsarray. Consider simplifying the setup:✨ Simplified test setup
it("retargets open diff state to the visited thread", () => { const state: AppState = { - ...makeState( - makeThread({ - id: "thread-local-1", - latestTurnCompletedAt: "2026-02-08T10:00:10.000Z", - lastVisitedAt: "2026-02-08T10:00:00.000Z", - }), - ), + ...makeState(makeThread()), threads: [ makeThread({ id: "thread-local-1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/store.test.ts` around lines 836 - 862, The test redundantly seeds threads by calling makeState(makeThread(...)) then immediately overriding threads; update the setup to call makeState() without the initial makeThread argument (or pass no args) and supply the desired threads array directly, keeping the rest of the properties (diffOpen, diffThreadId, diffTurnId, diffFilePath) unchanged; refer to makeState, makeThread, and the threads property when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/store.test.ts`:
- Around line 836-862: The test redundantly seeds threads by calling
makeState(makeThread(...)) then immediately overriding threads; update the setup
to call makeState() without the initial makeThread argument (or pass no args)
and supply the desired threads array directly, keeping the rest of the
properties (diffOpen, diffThreadId, diffTurnId, diffFilePath) unchanged; refer
to makeState, makeThread, and the threads property when making this change.
- Parse and validate `diff`, `diffTurnId`, and `diffFilePath` from chat route search - Switch ChatView/DiffPanel diff open/target actions to router navigation instead of store actions - Remove diff-specific state/actions from the app store and update reducer tests
- Extract `DiffWorkerPoolProvider` into its own component and wrap `_chat` outlet with it - Render/lazy-load `DiffPanel` from `/_chat/$threadId` so diff UI state and close behavior live with thread routing - Keep responsive sheet vs inline diff presentation with route-level fallback loading UI
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/src/routes/_chat.$threadId.tsx (1)
92-101: Consider extracting loading fallback to avoid duplication.The loading fallback has two branches with similar content. If this pattern is used elsewhere, consider a small helper component.
♻️ Optional: Extract loading fallback
const DiffLoadingFallback = ({ isInline }: { isInline: boolean }) => ( isInline ? ( <aside className="flex h-full w-[560px] shrink-0 items-center justify-center border-l border-border bg-card px-4 text-center text-xs text-muted-foreground/70"> Loading diff viewer... </aside> ) : ( <div className="flex h-full min-h-0 items-center justify-center px-4 text-center text-xs text-muted-foreground/70"> Loading diff viewer... </div> ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat`.$threadId.tsx around lines 92 - 101, Extract the duplicated JSX into a small helper React component (e.g., DiffLoadingFallback) and replace the inline ternary used to build diffLoadingFallback with a single call to that component; locate the current variable diffLoadingFallback and create DiffLoadingFallback that accepts a prop like isInline (or shouldUseDiffSheet) to switch between the two variants and reuse the existing classNames and "Loading diff viewer..." content so both branches are consolidated.apps/web/src/components/DiffPanel.tsx (2)
89-94: Type cast weakens type safety.Casting
rawSearch as Record<string, unknown>bypasses TanStack Router's typed search params. SinceuseSearch({ strict: false })already returns a looser type, consider adjustingparseDiffRouteSearchto accept the actual return type or using stricter route matching.♻️ Consider stricter typing
If
parseDiffRouteSearchexpectsRecord<string, unknown>, the function signature could be adjusted to accept the actualuseSearchreturn type, or use route-specific search viaRoute.useSearch()if available in this component's context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DiffPanel.tsx` around lines 89 - 94, The cast "rawSearch as Record<string, unknown>" weakens type safety—update the usage so parseDiffRouteSearch accepts the actual type returned by useSearch or obtain typed params from the route instead of forcing a cast. Specifically, change the parseDiffRouteSearch signature to accept the same type as useSearch()'s return (or create an overload/utility type) and remove the cast around rawSearch in the diffSearch memo, or switch to the route's typed Route.useSearch() if available in this component; touch symbols: rawSearch, useSearch, parseDiffRouteSearch, and diffSearch.
202-233: Consider extracting shared navigation logic.Both
selectTurnandselectWholeConversationshare nearly identical navigation patterns, differing only in the returned search params. A helper could reduce duplication.♻️ Optional: Extract navigation helper
+const navigateToDiff = ( + diffParams: { diffTurnId?: string } = {} +) => { + if (!activeThread) return; + void navigate({ + to: "/$threadId", + params: { threadId: activeThread.id }, + search: (previous) => { + const { diff: _d, diffTurnId: _t, diffFilePath: _f, ...rest } = previous; + return { ...rest, diff: "1", ...diffParams }; + }, + }); +}; const selectTurn = (turnId: string) => { - if (!activeThread) return; - void navigate({ - to: "/$threadId", - params: { threadId: activeThread.id }, - search: (previous) => { - const { diff: _diff, diffTurnId: _diffTurnId, diffFilePath: _diffFilePath, ...rest } = previous; - return { ...rest, diff: "1", diffTurnId: turnId }; - }, - }); + navigateToDiff({ diffTurnId: turnId }); }; const selectWholeConversation = () => { - if (!activeThread) return; - void navigate({ - to: "/$threadId", - params: { threadId: activeThread.id }, - search: (previous) => { - const { diff: _diff, diffTurnId: _diffTurnId, diffFilePath: _diffFilePath, ...rest } = previous; - return { ...rest, diff: "1" }; - }, - }); + navigateToDiff(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DiffPanel.tsx` around lines 202 - 233, Both selectTurn and selectWholeConversation duplicate the navigate(...) pattern; extract a helper (e.g., navigateToThreadWithDiff or buildDiffSearch) that accepts the activeThread (or threadId) and an optional diffTurnId and performs the common navigate call using the same destructuring of previous ({ diff: _diff, diffTurnId: _diffTurnId, diffFilePath: _diffFilePath, ...rest }) then returns { ...rest, diff: "1", ...(diffTurnId ? { diffTurnId } : {}) }; replace selectTurn and selectWholeConversation to call this helper and pass the turnId only where needed (keep references to selectTurn, selectWholeConversation, and navigate to locate the change).
🤖 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:
- Line 2: The React import uses the Activity component which requires React >=
19.2.0; update the apps/web package.json peer/dependency constraint for "react"
(and "react-dom" if present) from "^19.0.0" to "^19.2.0" so the Activity symbol
imported in _chat.$threadId.tsx is guaranteed at runtime, then run npm/yarn
install and verify the app builds and that Activity is resolvable.
---
Nitpick comments:
In `@apps/web/src/components/DiffPanel.tsx`:
- Around line 89-94: The cast "rawSearch as Record<string, unknown>" weakens
type safety—update the usage so parseDiffRouteSearch accepts the actual type
returned by useSearch or obtain typed params from the route instead of forcing a
cast. Specifically, change the parseDiffRouteSearch signature to accept the same
type as useSearch()'s return (or create an overload/utility type) and remove the
cast around rawSearch in the diffSearch memo, or switch to the route's typed
Route.useSearch() if available in this component; touch symbols: rawSearch,
useSearch, parseDiffRouteSearch, and diffSearch.
- Around line 202-233: Both selectTurn and selectWholeConversation duplicate the
navigate(...) pattern; extract a helper (e.g., navigateToThreadWithDiff or
buildDiffSearch) that accepts the activeThread (or threadId) and an optional
diffTurnId and performs the common navigate call using the same destructuring of
previous ({ diff: _diff, diffTurnId: _diffTurnId, diffFilePath: _diffFilePath,
...rest }) then returns { ...rest, diff: "1", ...(diffTurnId ? { diffTurnId } :
{}) }; replace selectTurn and selectWholeConversation to call this helper and
pass the turnId only where needed (keep references to selectTurn,
selectWholeConversation, and navigate to locate the change).
In `@apps/web/src/routes/_chat`.$threadId.tsx:
- Around line 92-101: Extract the duplicated JSX into a small helper React
component (e.g., DiffLoadingFallback) and replace the inline ternary used to
build diffLoadingFallback with a single call to that component; locate the
current variable diffLoadingFallback and create DiffLoadingFallback that accepts
a prop like isInline (or shouldUseDiffSheet) to switch between the two variants
and reuse the existing classNames and "Loading diff viewer..." content so both
branches are consolidated.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Member
Author
…ing logic Consolidate the repeated pattern of destructuring and removing diff/diffTurnId/diffFilePath search params into a single stripDiffSearchParams() function in diffRouteSearch.ts. Replaces 5 identical destructuring blocks across 3 files with calls to the shared helper. Applied via @cursor push command
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/routes/_chat.$threadId.tsx (1)
100-106: Consider adding an ErrorBoundary for the lazy-loaded DiffPanel.The
Suspenseboundary handles loading states, but if the chunk fails to load (e.g., network error), the error will propagate up. AnErrorBoundaryaround or inside theDiffPanelWrapperwould provide a graceful fallback UI for load failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat`.$threadId.tsx around lines 100 - 106, Wrap the lazy-loaded DiffPanel with an ErrorBoundary so chunk-load/runtime errors don’t bubble up; either add a small class-based ErrorBoundary component (implement componentDidCatch and render a fallback UI) or use react-error-boundary’s ErrorBoundary and place it around the existing Suspense (or inside DiffPanelWrapper) so failures show a graceful fallback instead of throwing; reference DiffPanel, DiffPanelWrapper and the Suspense/diffLoadingFallback in your change and ensure the ErrorBoundary renders a succinct error UI and an optional retry/close action tied to closeDiff.
🤖 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/_chat`.$threadId.tsx:
- Line 2: The import of the Activity symbol in _chat.$threadId.tsx requires
React 19.2+ but the project is pinned to 19.0.0; either upgrade React (and
react-dom) in package.json to >=19.2.0 and reinstall/test to ensure Activity is
available, or remove/replace the Activity usage in _chat.$threadId.tsx (use a
custom placeholder component or existing loading UI) so the code no longer
imports Activity when the runtime React version is <19.2.0.
---
Nitpick comments:
In `@apps/web/src/routes/_chat`.$threadId.tsx:
- Around line 100-106: Wrap the lazy-loaded DiffPanel with an ErrorBoundary so
chunk-load/runtime errors don’t bubble up; either add a small class-based
ErrorBoundary component (implement componentDidCatch and render a fallback UI)
or use react-error-boundary’s ErrorBoundary and place it around the existing
Suspense (or inside DiffPanelWrapper) so failures show a graceful fallback
instead of throwing; reference DiffPanel, DiffPanelWrapper and the
Suspense/diffLoadingFallback in your change and ensure the ErrorBoundary renders
a succinct error UI and an optional retry/close action tied to closeDiff.
Resolve PR conflicts while preserving route-scoped diff rendering and _chat desktop menu action handling. Co-authored-by: codex <codex@users.noreply.github.com>
| } | ||
|
|
||
| export const Route = createFileRoute("/_chat/$threadId")({ | ||
| validateSearch: (search) => parseDiffRouteSearch(search), |
Contributor
There was a problem hiding this comment.
Diff panel closes when switching threads via sidebar
Medium Severity
Moving diff state from the store (state.diffOpen) to URL search params (?diff=1) causes the diff panel to close when navigating between threads. The Sidebar navigates with navigate({ to: "/$threadId", params: { threadId } }) without specifying search, so TanStack Router clears search params on navigation. The route's validateSearch lacks a retainSearchParams middleware, meaning ?diff=1 is lost on every thread switch. Previously, store-based diffOpen persisted across navigation. This contradicts the PR's stated goal of retargeting the open diff panel to the visited thread.
Contributor
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (da1cd3307d)diff --git a/apps/web/src/routes/_chat.$threadId.tsx b/apps/web/src/routes/_chat.$threadId.tsx
--- a/apps/web/src/routes/_chat.$threadId.tsx
+++ b/apps/web/src/routes/_chat.$threadId.tsx
@@ -1,4 +1,4 @@
-import { createFileRoute, useNavigate } from "@tanstack/react-router";
+import { createFileRoute, retainSearchParams, useNavigate } from "@tanstack/react-router";
import { Activity, Suspense, lazy, type ReactNode, useCallback, useEffect } from "react";
import ChatView from "../components/ChatView";
@@ -110,5 +110,8 @@
export const Route = createFileRoute("/_chat/$threadId")({
validateSearch: (search) => parseDiffRouteSearch(search),
+ search: {
+ middlewares: [retainSearchParams(true)],
+ },
component: ChatThreadRouteView,
}); |
jjalangtry
pushed a commit
to jjalangtry/t3code
that referenced
this pull request
Mar 16, 2026
Retarget open diff panel to the visited thread
smraikai
pushed a commit
to smraikai/t3code
that referenced
this pull request
Apr 16, 2026
Retarget open diff panel to the visited thread
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
2 participants
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.
Summary
DiffPanelprefer the route thread when present, so diff context follows the active thread page.SET_DIFF_TARGETto retarget the panel.MARK_THREAD_VISITEDto retarget open diff state to the visited thread and clear stale turn/file selection.Testing
store reducer thread continuity->retargets open diff state to the visited threadinapps/web/src/store.test.ts.Note
Medium Risk
Touches routing and navigation flow for diff viewing and removes persisted diff UI state, so regressions could affect deep-linking/back-forward behavior and diff panel visibility across thread navigation.
Overview
The diff panel is now URL-driven: open/close and turn/file targeting are read from and written to route search params (
diff,diffTurnId,diffFilePath) via new helpersparseDiffRouteSearch/stripDiffSearchParams, used by bothChatViewandDiffPanel.Diff UI mounting is moved from the
_chatlayout to the per-thread route (_chat.$threadId), including responsive sheet/inline behavior and lazy-loading, andDiffPanelnow always follows the routethreadIdinstead of stored diff-thread state.State management is simplified by removing diff-related fields and actions from the store/reducer (and associated tests), adding focused unit tests for diff search parsing; the diff worker-pool provider is extracted into
DiffWorkerPoolProviderand applied at the chat layout level.Written by Cursor Bugbot for commit 8a72a2a. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests