Refine sidebar worktree group interactions#815
Refine sidebar worktree group interactions#815Mitch515 wants to merge 7 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}
| groups.set(id, { | ||
| id, | ||
| branch: thread.branch, | ||
| worktreePath: thread.worktreePath, | ||
| label: threadGroupLabel({ | ||
| branch: thread.branch, | ||
| worktreePath: thread.worktreePath, | ||
| }), | ||
| latestActivityAt: thread.createdAt, | ||
| }); | ||
| continue; |
There was a problem hiding this comment.
🟢 Low src/threadGroups.ts:79
buildThreadGroupId normalizes branch and worktreePath (trimming whitespace, treating empty strings as falsy), but the raw unnormalized values are stored in OrderedProjectThreadGroup. This breaks resolveProjectThreadGroupPrById in two ways: (1) when worktreePath is "", the stored "" is used as cwd instead of falling back to projectCwd, causing Git status lookup to fail; (2) when values have leading/trailing whitespace, status.branch === group.branch and status-by-cwd lookups fail due to mismatched strings, resulting in lost PR status. Consider storing the normalized values used to build the ID.
groups.set(id, {
id,
- branch: thread.branch,
- worktreePath: thread.worktreePath,
+ branch: input.branch,
+ worktreePath: input.worktreePath,
label: threadGroupLabel({
branch: thread.branch,
worktreePath: thread.worktreePath,
}),
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 79-89:
`buildThreadGroupId` normalizes `branch` and `worktreePath` (trimming whitespace, treating empty strings as falsy), but the raw unnormalized values are stored in `OrderedProjectThreadGroup`. This breaks `resolveProjectThreadGroupPrById` in two ways: (1) when `worktreePath` is `""`, the stored `""` is used as `cwd` instead of falling back to `projectCwd`, causing Git status lookup to fail; (2) when values have leading/trailing whitespace, `status.branch === group.branch` and status-by-cwd lookups fail due to mismatched strings, resulting in lost PR status. Consider storing the normalized values used to build the ID.
Evidence trail:
apps/web/src/threadGroups.ts lines 26-32 (normalization functions trim whitespace), lines 45-53 (buildThreadGroupId normalizes values), lines 79-81 (raw values stored in OrderedProjectThreadGroup), lines 153-155 in resolveProjectThreadGroupPrById (uses ?? which doesn't treat empty string as fallback, and does direct equality comparison with potentially unnormalized stored values)
There was a problem hiding this comment.
Fixed in 155728d. Group metadata is now stored from normalized identity values so PR lookup uses the same normalized branch/worktree data as the group id.
| export function orderProjectThreadGroups<T extends ThreadGroupSeed>(input: { | ||
| threads: T[]; | ||
| orderedGroupIds?: readonly ThreadGroupId[] | null | undefined; | ||
| }): OrderedProjectThreadGroup[] { | ||
| const groups = new Map<string, OrderedProjectThreadGroup>(); | ||
| for (const thread of input.threads) { | ||
| const id = buildThreadGroupId({ | ||
| branch: thread.branch, | ||
| worktreePath: thread.worktreePath, | ||
| }); | ||
| const existing = groups.get(id); | ||
| if (!existing) { | ||
| groups.set(id, { | ||
| id, | ||
| branch: thread.branch, | ||
| worktreePath: thread.worktreePath, | ||
| label: threadGroupLabel({ | ||
| branch: thread.branch, | ||
| worktreePath: thread.worktreePath, | ||
| }), | ||
| latestActivityAt: thread.createdAt, | ||
| }); | ||
| continue; | ||
| } | ||
| if (thread.createdAt > existing.latestActivityAt) { | ||
| existing.latestActivityAt = thread.createdAt; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟢 Low src/threadGroups.ts:67
When multiple threads map to the same ThreadGroupId, the group retains the branch, label, and worktreePath from the first thread processed, even when a newer thread has different metadata. This causes stale UI labels and breaks resolveProjectThreadGroupPrById, which relies on group.branch matching the current git status to display PR information. Consider updating all group metadata when latestActivityAt changes.
- if (thread.createdAt > existing.latestActivityAt) {
- existing.latestActivityAt = thread.createdAt;
+ if (thread.createdAt > existing.latestActivityAt) {
+ existing.latestActivityAt = thread.createdAt;
+ existing.branch = thread.branch;
+ existing.worktreePath = thread.worktreePath;
+ existing.label = threadGroupLabel({
+ branch: thread.branch,
+ worktreePath: thread.worktreePath,
+ });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/threadGroups.ts around lines 67-94:
When multiple threads map to the same `ThreadGroupId`, the group retains the `branch`, `label`, and `worktreePath` from the *first* thread processed, even when a newer thread has different metadata. This causes stale UI labels and breaks `resolveProjectThreadGroupPrById`, which relies on `group.branch` matching the current git status to display PR information. Consider updating all group metadata when `latestActivityAt` changes.
Evidence trail:
apps/web/src/threadGroups.ts lines 67-92: `orderProjectThreadGroups` function - when existing group found, only `latestActivityAt` is updated (lines 89-92), not `branch`, `label`, or `worktreePath` (commit REVIEWED_COMMIT)
apps/web/src/threadGroups.ts lines 146-167: `resolveProjectThreadGroupPrById` function - line 160 compares `group.branch` against `status?.branch` to determine if PR info should be displayed
apps/web/src/threadGroups.ts lines 46-56: `buildThreadGroupId` function shows that `worktreePath` takes precedence in ID generation, meaning two threads with same worktreePath but different branches get the same `ThreadGroupId`
There was a problem hiding this comment.
Fixed in 155728d. When a newer thread wins the group, the stored branch, worktree path, and label now refresh with it.
| const insertIndex = withoutMoved.indexOf(input.beforeProjectId); | ||
| if (insertIndex === -1) { | ||
| return [input.movedProjectId, ...withoutMoved]; | ||
| } | ||
|
|
||
| return [ | ||
| ...withoutMoved.slice(0, insertIndex), | ||
| input.movedProjectId, | ||
| ...withoutMoved.slice(insertIndex), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🟢 Low src/projectOrder.ts:68
When beforeProjectId === movedProjectId, the function incorrectly moves the item to the start of the list. Line 62 removes movedProjectId from the array, so withoutMoved.indexOf(input.beforeProjectId) returns -1, triggering the fallback on line 70. The item should stay in its original position when dropped on itself.
const insertIndex = withoutMoved.indexOf(input.beforeProjectId);
if (insertIndex === -1) {
return [...withoutMoved, input.movedProjectId];
}
+ if (input.beforeProjectId === input.movedProjectId) {
+ return input.currentOrder.slice();
+ }
return [
...withoutMoved.slice(0, insertIndex),Also found in 1 other location(s)
apps/web/src/threadGroups.ts:135
In
reorderProjectThreadGroupOrder, ifinput.beforeGroupIdequalsinput.movedGroupId(e.g. dropping an item on itself), the item is removed fromwithoutMovedon line 128. Consequently,withoutMoved.indexOf(input.beforeGroupId)on line 135 returns-1because the target ID is no longer in the array. This triggers the condition on line 136, causing the item to jump to the start of the list (line 137) rather than staying in its original position.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/projectOrder.ts around lines 68-78:
When `beforeProjectId === movedProjectId`, the function incorrectly moves the item to the start of the list. Line 62 removes `movedProjectId` from the array, so `withoutMoved.indexOf(input.beforeProjectId)` returns `-1`, triggering the fallback on line 70. The item should stay in its original position when dropped on itself.
Evidence trail:
apps/web/src/projectOrder.ts lines 60-76 (commit REVIEWED_COMMIT): Line 62 filters out movedProjectId from currentOrder into withoutMoved. Line 68 does `withoutMoved.indexOf(input.beforeProjectId)`. When beforeProjectId === movedProjectId, this returns -1 since movedProjectId was removed. Line 69-71 handles insertIndex === -1 by returning `[input.movedProjectId, ...withoutMoved]`, moving the item to the start instead of keeping it in place.
Also found in 1 other location(s):
- apps/web/src/threadGroups.ts:135 -- In `reorderProjectThreadGroupOrder`, if `input.beforeGroupId` equals `input.movedGroupId` (e.g. dropping an item on itself), the item is removed from `withoutMoved` on line 128. Consequently, `withoutMoved.indexOf(input.beforeGroupId)` on line 135 returns `-1` because the target ID is no longer in the array. This triggers the condition on line 136, causing the item to jump to the start of the list (line 137) rather than staying in its original position.
There was a problem hiding this comment.
Fixed in 155728d. Dropping a project or group onto itself now returns the existing order unchanged.
| export function shouldClearOptimisticProjectOrder<T extends string>(input: { | ||
| optimisticOrder: readonly T[] | null | undefined; | ||
| currentOrder: readonly T[] | null | undefined; | ||
| }): boolean { | ||
| if (!input.optimisticOrder || input.optimisticOrder.length === 0) { | ||
| return false; | ||
| } | ||
| return projectOrdersEqual(input.optimisticOrder, input.currentOrder); | ||
| } |
There was a problem hiding this comment.
🟢 Low src/projectOrder.ts:16
shouldClearOptimisticProjectOrder returns false when optimisticOrder is an empty array [], even if currentOrder is also [] and the sync has completed. This prevents the UI from exiting the optimistic state when all items are deleted, leaving it stuck in 'Saving...'. Consider removing the length === 0 check so empty arrays are compared via projectOrdersEqual.
export function shouldClearOptimisticProjectOrder<T extends string>(input: {
optimisticOrder: readonly T[] | null | undefined;
currentOrder: readonly T[] | null | undefined;
}): boolean {
- if (!input.optimisticOrder || input.optimisticOrder.length === 0) {
+ if (!input.optimisticOrder) {
return false;
}
return projectOrdersEqual(input.optimisticOrder, input.currentOrder);
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/projectOrder.ts around lines 16-24:
`shouldClearOptimisticProjectOrder` returns `false` when `optimisticOrder` is an empty array `[]`, even if `currentOrder` is also `[]` and the sync has completed. This prevents the UI from exiting the optimistic state when all items are deleted, leaving it stuck in 'Saving...'. Consider removing the `length === 0` check so empty arrays are compared via `projectOrdersEqual`.
Evidence trail:
apps/web/src/projectOrder.ts lines 16-23: `shouldClearOptimisticProjectOrder` function definition showing the `length === 0` check returning `false` before comparing arrays. apps/web/src/projectOrder.ts lines 3-13: `projectOrdersEqual` function that correctly returns `true` for two empty arrays. apps/web/src/components/Sidebar.tsx lines 494-503: Usage showing that when `shouldClearOptimisticProjectOrder` returns `false`, `setOptimisticProjectOrder(null)` is never called, confirming the consequence.
There was a problem hiding this comment.
Fixed in 155728d. Empty optimistic project order now clears correctly instead of leaving the UI stuck in saving state.
|
Assets for review: Grouped sidebar screenshot:
Compose affordance screenshot:
Muted video: |
1 participant
What Changed
Why
UI Changes
mainChecklist
Note
Reorganize sidebar to group threads by branch/worktree with drag-reorder and context menus
@dnd-kitdrag-and-drop with custom pointer-based drag logic for reordering both projects and per-project thread groups, with CSS transform animations and drop indicators.sortSidebarThreadEntriesand related utilities insidebarThreadOrder.tsto sort threads by recent activity or creation time, configurable via a new Settings page control.sidebarGroupInteractions.tswith ~15 pure utility functions covering drag thresholds, drop validity, collapse keys, chevron/header/indicator class names, and selection suppression.ComposerDraftStoreto track draft threads per project group (branch/worktree) with migration from the flat per-project mapping.@dnd-kitentirely; any downstream code relying on sortable context or DnD sensor behavior will break.📊 Macroscope summarized 7032a72. 14 files reviewed, 17 issues evaluated, 1 issue filtered, 4 comments posted
🗂️ Filtered Issues
apps/web/src/threadGroups.ts — 2 comments posted, 5 evaluated, 1 filtered
reorderProjectThreadGroupOrder, ifinput.beforeGroupIdequalsinput.movedGroupId(e.g. dropping an item on itself), the item is removed fromwithoutMovedon line 128. Consequently,withoutMoved.indexOf(input.beforeGroupId)on line 135 returns-1because the target ID is no longer in the array. This triggers the condition on line 136, causing the item to jump to the start of the list (line 137) rather than staying in its original position. [ Cross-file consolidated ]