Repository navigation
fix(sidebar): draw a project group once when two hosts publish it under one id - #30128
Conversation
…er one id Moving a project to a managed server copies its project group there under the same id while the desktop keeps its copy. The row builder emitted a header per copy, so the second sat empty and both shared one virtual-row key. Fixes #30045
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
bd9a5997-40ad-4685-8488-b03a845e5139
📒 Files selected for processing: 11
src/renderer/src/components/cmd-j/palette-project-results.tssrc/renderer/src/components/cmd-j/palette-results.test.tssrc/renderer/src/components/sidebar/WorktreeList.tsxsrc/renderer/src/components/sidebar/worktree-list-groups-same-id-host-groups.test.tssrc/renderer/src/components/sidebar/worktree-list/navigation/folder-reveal.test.tssrc/renderer/src/components/sidebar/worktree-list/navigation/folder-reveal.tssrc/renderer/src/components/sidebar/worktree-list/navigation/reveal-ancestors.tssrc/renderer/src/components/sidebar/worktree-list/viewport/VirtualizedWorktreeViewport.tsxsrc/renderer/src/components/sidebar/worktree-list/viewport/viewport-props.tssrc/shared/project-groups.tssrc/shared/sidebar-list/project-group-sections.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| const childGroupsByParentId = new Map<string | null, ProjectGroup[]>() | ||
| for (const group of projectGroups) { | ||
| // Why distinct: membership is id-keyed, so a same-id copy's header would sit empty (#30045). | ||
| for (const group of getDistinctProjectGroups(projectGroups)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep conflicting host trees from hiding grouped projects.
If same-ID copies from two hosts interleave, selecting the last copy of each ID can create a parent cycle. For example, local A is a root and local B is its child, while remote B is a root and remote A is its child. In the order [local A, remote B, remote A, local B], this loop selects A → B → A. Neither group reaches the root traversal at Line 132. The fallback at Lines 137-139 then skips their grouped repos because both IDs exist. Resolve conflicting parents or detect cycles before building the tree so those projects remain visible.
ELI5
When you move a project to a managed server, the server gets a copy of the project's group with the same id, and this computer keeps its own copy. The sidebar drew a header for each copy, so the group showed up twice: one header with the project, one empty, and because both headers had the same row key they could pile up on top of each other. Now each group is drawn once.
What Changed
Before: after moving a grouped project to a managed server (local copy kept), with the server connected and the sidebar grouped by project, the group header appeared twice. The second was empty. Both had the row key
hdr:project-group:<id>, so the virtual list could overlap them and leave ghost rows after quick updates (for example, removing several workspaces in a row). Cmd+J also listed the group twice.After: one header for the group, holding the project's workspaces from both hosts. Cmd+J lists it once. Reveal (Cmd+J, jump to workspace) opens the parents of that same header.
Mechanism: a group id that two hosts publish now resolves to one copy everywhere the sidebar reads it: the last copy in the catalog, which is the copy every existing id-keyed lookup already used.
src/shared/project-groups.ts: newgetDistinctProjectGroups, one row per id in first-seen order with the last copy winning.src/shared/sidebar-list/project-group-sections.ts: builds group headers and their nesting from the distinct list, so a second copy no longer emits an empty header with the same key.getProjectGroupOccurrencePathsuses the same list.cmd-j/palette-project-results.ts: lists groups from the distinct list, so search finds the name the sidebar shows.reveal-ancestors.tsandfolder-reveal.ts: walk the parents of the copy that is drawn. Folder-workspace reveal still opens the folder's own host section.WorktreeListpasses the host-filtered group list (the one the rows are built from) to reveal, so a host filter cannot make reveal open a hidden host's parent.Why
Root cause: the sidebar fetches every host's groups into one list (
fetchProjectGroupsForAllHosts), and a managed-server move copies a group to the server under the same id. Group membership in the row builder is keyed by id only (repo.projectGroupId), butappendProjectGroupSectionsemitted one header per catalog row. The first header took the project (groupByProjectGroupId.delete), the second was empty, and both headers had the same key.Alternatives considered:
Linked Issue
Fixes #30045
Visual Proof
Real Orca dev app (background launch, isolated profile), with one project in a group that the local catalog and a runtime server both publish under the same id. Before uses
main'sproject-group-sections.ts, after uses this branch's:Before: "Platform" twice, the second one empty. After: one "Platform" holding both hosts' workspaces.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
New
worktree-list-groups-same-id-host-groups.test.ts: one header holding the project; unique render keys; nesting of a same-id child drawn once; reveal paths follow the drawn copy when copies have different parents. These fail onmain.palette-results.test.ts: a same-id group is listed once, found by the name the sidebar shows.folder-reveal.test.ts: folder reveal follows the drawn copy's parents. The existing host-owner case now lists the copies in the order where that host's copy is the drawn one, because the drawn copy decides which parents exist.Ran
src/renderer/src/components/sidebar(370 files),src/shared,cmd-j,pnpm tc, and the static-analysis job steps frompr.ymllocally.Verified live, on macOS only: the rendered sidebar in the real app with a seeded two-host catalog, before and after (screenshots above). Not verified live: a real managed-server move. The seeded state matches the on-disk data in the issue (same group id on both hosts).
Review
Codex review (
gpt-6.1-sol, high), four rounds. Each finding was fixed:getDistinctProjectGroups.Final round: no findings.
Agent skill upstream boundary
Notes
Overlap: #29877 (open, conflicting) changes which row places a multi-host project inside
appendProjectGroupSections. It touches the same function, but its rule is independent of this one. #29863 (open) filters Move to group by host. #22023 handles different-id groups that share a name. SSH and folder workspaces: folder workspaces in a same-id group render under the single header with their own host identity, and the host section key used by reveal is unchanged. No wire change.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)