Repository navigation
Conversation
|
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. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to
|
7979a8b to
f69719a
Compare
nwparker
left a comment
There was a problem hiding this comment.
Thanks, the placement fix looks right. The new tests fail on current main and pass here, and the sidebar and src/shared/sidebar-list folders pass.
One gap to fix before this can merge. The project's … menu still works from the anchor row, so it doesn't match where the project is now drawn:
SectionHeader.tsx(around line 384) passesrow.repotoRepoHeaderProjectActionsMenu. Take a header drawn inside a group through a paired host's row (row.projectGroupRepo), where the local anchor has no group. On that header, Remove from group is hidden, even though the project sits inside the group. Move to group also acts on the local row, not on the row that put the project in the group.- Suggested fix: the group actions (Move to group, Remove from group, the disabled check) should use
row.projectGroupRepo ?? row.repo. Other repository actions should keep the anchor. A test where the header is drawn in a group through the other host's row, checking that Remove from group shows and targets that row's host, would cover it.
This interacts with #29863: that PR filters Move to group by the host of the repo it receives, so both PRs should agree on which row the menu gets.
|
Thanks. Fixed in the new commit on top, no rebase, so it shows as a plain diff.
On #29863: whichever of the two lands second will filter Move to group by the same row, |
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:
48e08230-1895-481c-9dc9-99506fb3cb34
📥 Commits
Reviewing files that changed from the base of the PR and between f69719abcb7277b1f7bbffe6c7908faff2386f7b and 8c0dc57cd0e7631f4ded136c0c3821a3fbe609ad.
📒 Files selected for processing: 5
src/renderer/src/components/sidebar/worktree-list-groups-cross-host-project-group.test-fixture.tssrc/renderer/src/components/sidebar/worktree-list-groups-cross-host-project-group.test.tssrc/renderer/src/components/sidebar/worktree-list-groups-cross-host-project-menu.test.tsxsrc/renderer/src/components/sidebar/worktree-list/rows/SectionHeader.tsxsrc/renderer/src/components/sidebar/worktree-list/rows/repo-header-project-actions.tsx
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
|
Rebased onto main |
3791468 to
81bafa5
Compare
|
Rebased onto main
|
c5b9e79 to
b37f05d
Compare
|
Rebased onto main Gates: |
b37f05d to
d52141b
Compare
… is in A project set up on several hosts is one sidebar section anchored on each Orca's own row, and placement read only the anchor's projectGroupId. Move to group changes one host's row, so only the Orca anchored on that row drew the project inside the group; the others showed the group empty. - getProjectSectionMemberRepoIds maps every catalog row to its section, including rows whose workspaces are pinned away or absent. - getProjectSectionGroupRepo picks the first member in manual order that is in a known group, else the anchor. Placement, in-group rank and the reveal path read that row. - Header rows carry it as projectGroupRepo; header drag buckets, ranks and commits use it, and the commit names that row's host. Ported onto src/shared/sidebar-list after stablyai#29906. Fixes stablyai#29876 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aced it in the group A header drawn inside a paired host's group, while this Orca's own row is ungrouped, hid Remove from group and moved the local row. Move to group, its current-group check and Remove from group now use the header's projectGroupRepo; the other project actions keep the anchor row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d52141b to
45d4741
Compare
|
@jbamlee Any follow-up? |
ELI5
When the same project is added on two paired computers, the sidebar shows it once, with a row for each computer. It picked the project's group by looking at only one of those rows, and each computer looked at its own row. So after you put the project in a group on one computer, the other computer showed an empty group and left the project outside it. Now, if any of the project's rows is in a group, the project is shown inside that group.
What Changed
Before: Orca A and Orca B are paired and both have the same Git project. On Orca A, Move to group puts the project inside the group. Orca B shows the group header empty, with the project at the top level.
After: Orca B also shows the project inside that group. If the computers' rows are in different groups, each Orca uses the group of the first row in its own manual order. That is normally its own row.
Mechanism:
src/shared/sidebar-list/project-group-sections.ts: newgetProjectSectionMemberRepoIds(repoMap, projectIndex)maps every catalog row to its section key, once per row build. It covers rows whose workspaces are pinned away and rows that have no workspaces yet.getProjectSectionGroupRepo(section, memberRepoIds, …)returns the row that places a section: the first member row in manual order that is in a known group. If no row is in a known group, it is the anchor, so the section stays at the root as today. Because it reads catalog rows rather than visible ones, pins and placeholders cannot move a project. The drawn sidebar and the Cmd+1–9 order also always agree.appendProjectGroupSectionsplaces sections, and orders them inside a group, by that row'sprojectGroupIdandprojectGroupOrder.sidebar-row-plan.ts(planSidebarRows) takes the reveal path (which group to expand to reveal a workspace) from the same row, so reveal matches where the project is drawn. Every caller ofplanSidebarRows(the window's list, the Cmd+1–9 order, reveal, and the sidebar feed) reads the same placement.projectGroupRepowhen it is not the anchor. Header drag passes it into the drag session next to the header's own row (since fix(projects): a project move names the row it moves; a paired server's twin by its SSH target #30794 the session carries rows, not ids). The drag takes its bucket from it, sibling ranks read it, and the commit moves that row (moveProjectToGroup(row, …)). The header's own id still marks its place in the list. Dragging a project inside a group therefore reorders the row that put it there, on that row's own host.…menu takes the same row (RepoHeaderProjectActionsMenu'sprojectGroupRepo). Move to group, its "current group" check and Remove from group act on it, so they match where the project is drawn. Project settings, icon, worktree visibility, New group from project and Remove Project keep the anchor row.Why
Root cause: the anchor is the first of the section's rows in each Orca's catalog order, which is usually its own host's row (
getManualOrderAnchorRepo). Placement read only the anchor'sprojectGroupId, and Move to group changes only one host's row. So only the Orca whose anchor is the moved row draws the project in the group. The group catalogs themselves sync correctly. A two-Electron run showed the paired client receives both the group and the moved row.Alternatives considered:
Linked Issue
Fixes #29876
Related: #29536 (folds same-named groups from different hosts into one header). This PR is about one group that only one host has; the two do not overlap in logic but touch the same file.
Visual Proof
Rerun on this branch rebased onto main
a439dfa451(two background Electron apps, host and paired client):Original run, before the rebase:
Two real Orca apps ran in the background, built from this branch and from its base. One is the host and one a paired client. The same Git remote was added on both, and the client lists its own row first. The project was grouped on the host. Sidebar header positions as rendered (
leftis the label's x offset, 14 = top level, 24 = nested):Drag inside the group on the paired client (this branch). The project placed through the host's row is dragged above a host-only project in the same group:
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
New
worktree-list-groups-cross-host-project-group.test.ts:project-header-drag-commit.test.ts: a header placed by another host's row reorders that row.project-header-drag-start.test.ts: the drag takes that row's bucket. Both fail with main's drag code, which reads only the header's own row.New
worktree-list-groups-cross-host-project-menu.test.tsx(review follow-up): the planned header, with this Orca's ungrouped row as anchor, renders the menu with the propsSectionHeaderpasses. Remove from group shows and removes the paired host's row, and Move to group marks that group current and moves that row. Both fail before the change. Project Settings and Remove Project stay on the local row. The two-host fixture is shared throughworktree-list-groups-cross-host-project-group.test-fixture.ts.Rebased onto main
a439dfa451. It conflicted with Keep project names short and show their machine #28250, which wrapped the project header in the machine tooltip and re-indented it. The PR's twoSectionHeaderlines (the drag start and the…menu getrow.projectGroupRepo) are reapplied unchanged. Gates:pnpm test src/renderer/src/components/sidebar src/shared/sidebar-list src/renderer/src/store/slices(7327 passed),pnpm tc,oxlint,oxfmt --check,pnpm run check:code-quality:changed, and the two-Electron E2E above.Rebased onto main
0a3d203f61. It conflicted with fix(projects): a project move names the row it moves; a paired server's twin by its SSH target #30794, which made the drag session carry the dragged row. The session now also carriesprojectGroupRepo; the commit moves that row and keeps the header's id for list positions. Gates:pnpm test src/renderer/src/components/sidebar src/shared/sidebar-list src/renderer/src/store/slices(7235 passed),pnpm tc,oxlint,oxfmt --check,pnpm run check:code-quality:changed.Review follow-up gates:
pnpm test src/renderer/src/components/sidebar src/shared/sidebar-list,pnpm tc,oxlint,pnpm run check:code-quality:changedpass.Rebased onto main
90e99c48f4(the list builder moved tosrc/shared/sidebar-listin refactor(sidebar): move the pure workspace-list builder into src/shared/sidebar-list (no visible change) #29906):pnpm test src/shared/sidebar-list src/renderer/src/components/sidebar src/renderer/src/store/slices(7221 passed, including the refactor(sidebar): move the pure workspace-list builder into src/shared/sidebar-list (no visible change) #29906 golden row files),pnpm tc,oxlint,pnpm run check:code-quality:changed. With main's builder files, 6 of the 8 new tests fail; the 2 guards pass.Platforms: macOS (unit tests and a two-Electron background E2E). The change is renderer-only with no platform branches.
AI Disclosure
Implemented with Claude Code (Claude Opus 5.5). Independent review by GPT-6 Astra (xhigh reasoning) through Codex CLI before the rebase. Its findings (pinned members; Cmd+1–9 parity, including competing groups; drag buckets) are fixed in this PR.
Review
Agent skill upstream boundary
Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
planSidebarRowsthrough the sidebar feed, so it places the project the same way with no phone code change.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)🤖 Generated with Claude Code