Persist sidebar project and worktree order#816
Persist sidebar project and worktree order#816Mitch515 wants to merge 8 commits intopingdotgg:mainfrom
Conversation
|
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
the
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| get().clearProjectGroupDraftThreadId(projectId, fallbackGroupId.slice(projectPrefix.length)); | ||
| } | ||
| }, | ||
| clearProjectDraftThreadById: (projectId, threadId) => { |
There was a problem hiding this comment.
🟢 Low src/composerDraftStore.ts:892
clearProjectDraftThreadById returns after clearing only the first matching project-group mapping, so when a thread is mapped to multiple groups it leaves orphaned mappings and partial thread state. Consider iterating through all mappings to remove every instance of the thread.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/composerDraftStore.ts around line 892:
`clearProjectDraftThreadById` returns after clearing only the first matching project-group mapping, so when a thread is mapped to multiple groups it leaves orphaned mappings and partial thread state. Consider iterating through all mappings to remove every instance of the thread.
Evidence trail:
apps/web/src/composerDraftStore.ts lines 892-903 (REVIEWED_COMMIT) - `clearProjectDraftThreadById` function with `return` at line 901 inside the for loop after clearing first match. Contrast with `clearDraftThread` at lines 904-926 which properly filters all matching entries.
There was a problem hiding this comment.
Fixed in b958328. Clearing a project draft thread now removes every matching group mapping instead of stopping after the first one.
| export function reorderProjectOrder(input: { | ||
| currentOrder: readonly ProjectId[]; | ||
| movedProjectId: ProjectId; | ||
| beforeProjectId: ProjectId | null; | ||
| }): ProjectId[] { | ||
| const withoutMoved = input.currentOrder.filter((projectId) => projectId !== input.movedProjectId); |
There was a problem hiding this comment.
🟢 Low src/projectOrder.ts:76
When movedProjectId equals beforeProjectId (e.g., dropping an item onto itself), the function incorrectly moves the item to the start of the list. Line 81 removes movedProjectId from withoutMoved, so withoutMoved.indexOf(beforeProjectId) returns -1 at line 87, triggering the fallback at line 88 that prepends the item. Consider returning input.currentOrder unchanged when the IDs are equal.
if (input.movedProjectId === input.beforeProjectId) {
+ return [...input.currentOrder];
+ }
+
const withoutMoved = input.currentOrder.filter((projectId) => projectId !== input.movedProjectId);Also found in 1 other location(s)
apps/web/src/threadGroups.ts:138
The function
reorderProjectThreadGroupOrderhandles a "drop on self" scenario incorrectly, treating a no-op as a move to the start of the list. IfmovedGroupIdis equal tobeforeGroupId, the item is first removed fromwithoutMoved(line 130). Consequently,withoutMoved.indexOf(beforeGroupId)returns-1(line 137). This triggers the fallback logic at line 139, which places the moved group at the beginning of the array. The expected behavior for dropping an item before itself is to return the list order unchanged.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/projectOrder.ts around lines 76-81:
When `movedProjectId` equals `beforeProjectId` (e.g., dropping an item onto itself), the function incorrectly moves the item to the start of the list. Line 81 removes `movedProjectId` from `withoutMoved`, so `withoutMoved.indexOf(beforeProjectId)` returns `-1` at line 87, triggering the fallback at line 88 that prepends the item. Consider returning `input.currentOrder` unchanged when the IDs are equal.
Evidence trail:
apps/web/src/projectOrder.ts lines 76-98 at REVIEWED_COMMIT: Line 81 filters out `movedProjectId`, line 87 searches for `beforeProjectId` in the filtered list (returns -1 when they're equal), line 88-90 prepends the item when index is -1.
Also found in 1 other location(s):
- apps/web/src/threadGroups.ts:138 -- The function `reorderProjectThreadGroupOrder` handles a "drop on self" scenario incorrectly, treating a no-op as a move to the start of the list. If `movedGroupId` is equal to `beforeGroupId`, the item is first removed from `withoutMoved` (line 130). Consequently, `withoutMoved.indexOf(beforeGroupId)` returns `-1` (line 137). This triggers the fallback logic at line 139, which places the moved group at the beginning of the array. The expected behavior for dropping an item before itself is to return the list order unchanged.
There was a problem hiding this comment.
Fixed in b958328. Self-drop reorder is now treated as a no-op for both project and group ordering.
| if (thread.createdAt > existing.latestActivityAt) { | ||
| existing.latestActivityAt = thread.createdAt; | ||
| } |
There was a problem hiding this comment.
🟡 Medium src/threadGroups.ts:91
When orderProjectThreadGroups aggregates threads into groups, it only updates latestActivityAt when a newer thread is encountered, leaving branch, worktreePath, and label set to the values of the first thread processed. Since input.threads is not guaranteed to be ordered by time, this causes the group to retain stale identity data. Later, resolveProjectThreadGroupPrById compares the stale group.branch against the current git status, so PR status lookups fail whenever the branch name has changed. Update the group's metadata fields when a newer thread is found.
- if (thread.createdAt > existing.latestActivityAt) {
- existing.latestActivityAt = thread.createdAt;
- }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/threadGroups.ts around lines 91-93:
When `orderProjectThreadGroups` aggregates threads into groups, it only updates `latestActivityAt` when a newer thread is encountered, leaving `branch`, `worktreePath`, and `label` set to the values of the first thread processed. Since `input.threads` is not guaranteed to be ordered by time, this causes the group to retain stale identity data. Later, `resolveProjectThreadGroupPrById` compares the stale `group.branch` against the current git status, so PR status lookups fail whenever the branch name has changed. Update the group's metadata fields when a newer thread is found.
Evidence trail:
apps/web/src/threadGroups.ts lines 47-55: buildThreadGroupId uses only worktreePath for ID when set (branch not included in ID); lines 72-76: builds group ID from branch/worktreePath; lines 77-93: when existing group found, only latestActivityAt is updated (lines 91-93), not branch/worktreePath/label; lines 161-164: resolveProjectThreadGroupPrById compares status?.branch against group.branch at line 163, returning null PR status when they don't match.
There was a problem hiding this comment.
Fixed in b958328. Group metadata now refreshes when a newer thread becomes the latest activity source.
|
Assets for review: Muted video: |
1 participant
What Changed
Why
UI Changes
mainChecklist
Note
Persist sidebar project and thread group order via
project.meta.updatethreadGroupOrderandsortOrderfields through contracts, read model, projector, projection DB, and the web store; two new migrations (014, 015) add the columns toprojection_projects.project.meta.updatewith updatedsortOrderorthreadGroupOrder, then refreshes the server snapshot with animated transitions.derivePendingUserInputsnow filters prompts when the session is in error/closed or the owning turn has settled; sidebar status pills gain 'Planning', 'Plan Submitted', and 'Errored' states.reorderProjectsis removed from the store and project order is no longer read from or written tolocalStorage; clients that relied on persisted local order will fall back to serversortOrder.📊 Macroscope summarized 74d3be2. 27 files reviewed, 21 issues evaluated, 5 issues filtered, 3 comments posted
🗂️ Filtered Issues
apps/server/src/orchestration/Layers/ProjectionPipeline.ts — 0 comments posted, 1 evaluated, 1 filtered
applyThreadTurnsProjection, the handling ofthread.turn-diff-completedevents creates a potential "zombie turn" state due to a logic regression. [ Out of scope ]apps/server/src/persistence/Migrations/014_ProjectionProjectsThreadGroupOrder.ts — 0 comments posted, 1 evaluated, 1 filtered
apps/web/src/appSettings.ts — 0 comments posted, 2 evaluated, 1 filtered
Schema.Literalsis not a standard export of theeffectSchema module. The correct function isSchema.Literal, and it accepts variadic arguments (e.g.Schema.Literal("recent-activity", "created-at")), not an array. Passing an array["recent-activity", "created-at"]toSchema.Literal(if corrected) would create a schema that matches the array instance itself, not a union of the strings. This will cause a runtime crash (undefined function) or validation errors for all string inputs. [ Out of scope (triage) ]apps/web/src/components/ChatView.tsx — 0 comments posted, 1 evaluated, 1 filtered
sendInFlightReftracks pending message submissions to prevent double-sends, but it is attached to the component instance (viauseRef) and is not reset when the active thread changes. BecauseChatViewis reused by the router when switching threads, navigating away from a thread while a message is sending (e.g., waiting for worktree creation or network response) keepssendInFlightRef.currentastrue. This effectively locks the chat input on the destination thread, preventing the user from sending messages until the previous thread's operation completes. To fix this,sendInFlightRef.currentshould be reset tofalsein theuseEffectthat handlesthreadIdchanges (around line 2030). [ Out of scope ]apps/web/src/threadGroups.ts — 1 comment posted, 2 evaluated, 1 filtered
reorderProjectThreadGroupOrderhandles a "drop on self" scenario incorrectly, treating a no-op as a move to the start of the list. IfmovedGroupIdis equal tobeforeGroupId, the item is first removed fromwithoutMoved(line 130). Consequently,withoutMoved.indexOf(beforeGroupId)returns-1(line 137). This triggers the fallback logic at line 139, which places the moved group at the beginning of the array. The expected behavior for dropping an item before itself is to return the list order unchanged. [ Cross-file consolidated ]