Normalize sidebar thread state for faster updates#1668
Conversation
- derive sidebar rows from thread summaries and indexed ids - move status pill logic off full thread activity scans
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Coalescing concatenates text ignoring streaming replacement semantics
- Added a check so that when the later event has streaming=false and non-empty text, its text is used as-is (replacement) instead of being concatenated with the previous event's text.
- ✅ Fixed: Exported hook
useThreadIdsByProjectIdis never used- Removed the unused useThreadIdsByProjectId hook and its selectThreadIdsByProjectId import from storeSelectors.ts since there are no call sites in the codebase.
Or push these changes by commenting:
@cursor push 4abafd3efa
Preview (4abafd3efa)
diff --git a/apps/web/src/routes/__root.tsx b/apps/web/src/routes/__root.tsx
--- a/apps/web/src/routes/__root.tsx
+++ b/apps/web/src/routes/__root.tsx
@@ -175,7 +175,10 @@
...event.payload,
attachments: event.payload.attachments ?? previous.payload.attachments,
createdAt: previous.payload.createdAt,
- text: previous.payload.text + event.payload.text,
+ text:
+ !event.payload.streaming && event.payload.text.length > 0
+ ? event.payload.text
+ : previous.payload.text + event.payload.text,
},
};
continue;
diff --git a/apps/web/src/storeSelectors.ts b/apps/web/src/storeSelectors.ts
--- a/apps/web/src/storeSelectors.ts
+++ b/apps/web/src/storeSelectors.ts
@@ -1,10 +1,9 @@
-import { type ProjectId, type ThreadId } from "@t3tools/contracts";
+import { type ThreadId } from "@t3tools/contracts";
import { useMemo } from "react";
import {
selectProjectById,
selectSidebarThreadSummaryById,
selectThreadById,
- selectThreadIdsByProjectId,
useStore,
} from "./store";
import { type Project, type SidebarThreadSummary, type Thread } from "./types";
@@ -25,8 +24,3 @@
const selector = useMemo(() => selectSidebarThreadSummaryById(threadId), [threadId]);
return useStore(selector);
}
-
-export function useThreadIdsByProjectId(projectId: ProjectId | null | undefined): ThreadId[] {
- const selector = useMemo(() => selectThreadIdsByProjectId(projectId), [projectId]);
- return useStore(selector);
-}You can send follow-ups to this agent here.
ApprovabilityVerdict: Needs human review This PR refactors sidebar state management for performance by introducing normalized indexes and extracting the thread row component. While the changes are internal optimizations, an unresolved review comment identifies a potential bug where project deletion doesn't clean up the new thread index maps, leaving stale entries. You can customize Macroscope's approvability policy. Learn more. |
…sed useThreadIdsByProjectId hook - In coalesceOrchestrationUiEvents, when the later event has streaming=false and non-empty text, use the later event's text as a replacement instead of concatenating, matching the store handler's replacement semantics. - Remove unused useThreadIdsByProjectId hook and its selectThreadIdsByProjectId import from storeSelectors.ts. Applied via @cursor push command
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Removing
keyfrom ChatView leaks state across threads- Restored
key={threadId}on both ChatView render sites so the component remounts on thread navigation, clearing all ~25 useState hooks that have no manual reset logic.
- Restored
Or push these changes by commenting:
@cursor push 2c692e6a62
Preview (2c692e6a62)
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
@@ -222,7 +222,7 @@
return (
<>
<SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground">
- <ChatView threadId={threadId} />
+ <ChatView key={threadId} threadId={threadId} />
</SidebarInset>
<DiffPanelInlineSidebar
diffOpen={diffOpen}
@@ -237,7 +237,7 @@
return (
<>
<SidebarInset className="h-dvh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground">
- <ChatView threadId={threadId} />
+ <ChatView key={threadId} threadId={threadId} />
</SidebarInset>
<DiffPanelSheet diffOpen={diffOpen} onCloseDiff={closeDiff}>
{shouldRenderDiffContent ? <LazyDiffPanel mode="sheet" /> : null}You can send follow-ups to this agent here.
- Derive checkpoint diffs from orchestration snapshots - Refresh checked-out branch upstream refs asynchronously - Update git, orchestration, and UI tests for the new flow
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Project deletion doesn't clean up thread index maps
- Added cleanup of threadIdsByProjectId and sidebarThreadsById entries for the deleted project's threads in the project.deleted handler.
Or push these changes by commenting:
@cursor push c8e1571f41
Preview (c8e1571f41)
diff --git a/apps/web/src/store.ts b/apps/web/src/store.ts
--- a/apps/web/src/store.ts
+++ b/apps/web/src/store.ts
@@ -637,8 +637,22 @@
}
case "project.deleted": {
- const projects = state.projects.filter((project) => project.id !== event.payload.projectId);
- return projects.length === state.projects.length ? state : { ...state, projects };
+ const deletedProjectId = event.payload.projectId;
+ const projects = state.projects.filter((project) => project.id !== deletedProjectId);
+ if (projects.length === state.projects.length) {
+ return state;
+ }
+ const orphanedThreadIds = state.threadIdsByProjectId[deletedProjectId];
+ if (!orphanedThreadIds || orphanedThreadIds.length === 0) {
+ const { [deletedProjectId]: _, ...threadIdsByProjectId } = state.threadIdsByProjectId;
+ return _ ? { ...state, projects, threadIdsByProjectId } : { ...state, projects };
+ }
+ const { [deletedProjectId]: _, ...threadIdsByProjectId } = state.threadIdsByProjectId;
+ const sidebarThreadsById = { ...state.sidebarThreadsById };
+ for (const threadId of orphanedThreadIds) {
+ delete sidebarThreadsById[threadId];
+ }
+ return { ...state, projects, threadIdsByProjectId, sidebarThreadsById };
}
case "thread.created": {You can send follow-ups to this agent here.
| ? state.threads.map((thread) => (thread.id === nextThread.id ? nextThread : thread)) | ||
| : [...state.threads, nextThread]; | ||
| return { ...state, threads }; | ||
| const nextSummary = buildSidebarThreadSummary(nextThread); |
There was a problem hiding this comment.
Project deletion doesn't clean up thread index maps
Low Severity
The project.deleted handler removes the project from state.projects but doesn't clean up the newly introduced threadIdsByProjectId or sidebarThreadsById entries. If a project is deleted without prior individual thread.deleted events for its threads, stale entries remain in both indexes.
- Remove the accidental double period in the test command note
- Remove stray period from the test command note
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
2 participants
Summary
Testing
Note
Medium Risk
Refactors core client state and sidebar rendering to rely on new normalized indices/summaries and changes orchestration event application timing/coalescing, which could impact thread list correctness and real-time UI updates.
Overview
Normalizes sidebar thread data by adding
SidebarThreadSummaryplussidebarThreadsByIdandthreadIdsByProjectIdto the web store, and keeping these in sync on read-model sync and all thread mutations (including correct removal when a thread is recreated under a different project).Refactors the sidebar to render by stable IDs: thread rows now select their own summary via
useSidebarThreadSummaryById,getVisibleSidebarThreadIdsconsumesrenderedThreadIds, and status pills use precomputed flags (hasPendingApprovals,hasPendingUserInput,hasActionableProposedPlan) instead of deriving from activities/proposed plans on each render.Reduces redundant UI updates from orchestration events by batching domain events into microtasks and coalescing consecutive
thread.message-sentevents for the same message (streaming) before applying them to the UI.Removes
key={threadId}fromChatView, preventing forced remounts on thread navigation, and updates/extends tests to match the new sidebar/store shapes and status-resolution inputs.Written by Cursor Bugbot for commit 2badf76. This will update automatically on new commits. Configure here.
Note
Normalize sidebar thread state into a dedicated store for faster updates
SidebarThreadSummaryas a normalized type in types.ts capturing per-thread flags (hasPendingApprovals,hasPendingUserInput,hasActionableProposedPlan) and metadata needed by the sidebar.sidebarThreadsByIdandthreadIdsByProjectIdtoAppStatein store.ts, populated on sync and kept current via a newupdateThreadStatehelper that recalculates summaries on every thread mutation.SidebarThreadRowcomponent that fetches its own data viauseSidebarThreadSummaryById.thread.message-sentevents for the same message in __root.tsx and applies domain events in microtask-sized batches to reduce redundant UI updates.ChatViewin _chat.$threadId.tsx is no longer force-remounted when switching threads becausekey={threadId}has been removed.Macroscope summarized a56c655.