Repository navigation
Conversation
Pinning a folder workspace persisted isPinned and showed Unpin, but buildRows only collected git worktrees for the Pinned section, so the card stayed under its project group. Treat pin as placement on the same renderable set: a pinned folder workspace moves to Pinned (or duplicates there when the in-groups policy is on), host counts include it, and reveal expands Pinned.
📝 WalkthroughWalkthroughPinned folder workspaces are partitioned by display policy and emitted in the Pinned section. Folder workspace rows now use section-qualified keys across grouping, rendering, activation, and accessibility. Navigation selects the correct Pinned or project-group section. Duplicate folder workspace rows are deduplicated in sidebar ordering and host counts. Tests cover grouping, reveal behavior, active descendants, row lookup, and rendering. Merge Risk: 🔵 Low · up to Pinned folder workspaces now appear in Pinned, but mixed fallback ordering, collapsed host counts, and accessibility state can be incorrect when duplicate-in-groups is enabled. These are bounded sidebar presentation issues that should be corrected before relying on the new duplicate behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description thoroughly covers the change, rationale, testing, visual proof, review considerations, and checklist. However, it omits the required Linked Issue section and does not provide an issue reference, and it omits the Agent skill upstream boundary section from the template. Resolution Add the required Linked Issue section with a valid issue reference, such as
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 |
|
@nwparker Would you please take a look at this one? This is a bug that prevents folder workspaces to be pinned in the pinned section. |
|
Do you mind adding some screenshots? Would help me review faster @lafraia ! |
|
@nwparker sorry to hide most of the sensitive data, but as you can see the folder workspace is pinned but it's not showing in the pinned section (another issue is subagents should show under the pinned item). |
|
you're right. Will get this in |
Pinned folder workspaces need two distinct rows when the duplicate policy is on, which an optional sectionKey only half-modelled: the virtualizer key diverged from the DOM option id, so aria-activedescendant and the reveal walkers pointed at an element that no longer existed. FolderWorkspaceRow now carries a required sectionKey and derives its key from it, exactly as WorktreeRow does, so one key flows through the virtualizer, the DOM id, the reveal walkers and the sidebar-order preference.
… surface Review follow-ups, all consequences of a folder workspace now being able to render twice: - reveal expanded the project group on top of Pinned. Under the default policy the card renders only in Pinned, and expandGroupsForWorktreeReveal persists every toggle, so revealing permanently un-collapsed a section the card is not in. Now an either/or, mirroring getPinnedWorktreeRevealCollapsedGroupKeys. - the host badge counted a duplicated folder workspace twice; item rows already deduped for exactly this reason. - both copies drew the primary active surface, because the active-surface rule only understood item rows. Generalized to any row that can duplicate, which also gives folder rows the click-promotion worktrees already had. - collectRenderedSidebarRowKeys still emitted the pre-change folder key, disagreeing with getRenderRowSidebarKey.
Two review follow-ups: useEffectiveCollapsedGroups already held the pinned display policy but did not pass it to getFolderWorkspaceRevealGroupKeys, the same silent-policy-assumption shape as the row-key divergence above; and the active-surface lookup resolved a ref for every row before finding its match, on the one path the sidebar hand-optimizes for pointer latency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/sidebar/worktree-list/grouping/pinned-group-rows.ts (1)
141-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPlace imported fallback cards after pinned folder rows.
Line 142 inserts each fallback directly after the last worktree item. Folder rows are not included in
lastResultIndexByRepoId. With one pinned worktree, one pinned folder workspace, and one fallback candidate, the result becomesworktree → fallback → folder, notworktree → folder → fallback.Defer fallback insertion until after the folder-workspace block. Add a regression test for this mixed Pinned section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 4041b051-418b-4023-a610-6fc5296245c5
📒 Files selected for processing (27)
src/renderer/src/components/sidebar/WorktreeList.folder-workspace-rows.test.tssrc/renderer/src/components/sidebar/host-section-folder-workspace-counts.test.tssrc/renderer/src/components/sidebar/host-section-rows.test.tssrc/renderer/src/components/sidebar/host-section-rows.tssrc/renderer/src/components/sidebar/worktree-keyboard-cycle.test.tssrc/renderer/src/components/sidebar/worktree-list/grouping/build-rows.folder-workspace-lanes.test.tssrc/renderer/src/components/sidebar/worktree-list/grouping/build-rows.pinned-folder-workspaces.test.tssrc/renderer/src/components/sidebar/worktree-list/grouping/build-rows.tssrc/renderer/src/components/sidebar/worktree-list/grouping/group-sections.tssrc/renderer/src/components/sidebar/worktree-list/grouping/pinned-group-rows.tssrc/renderer/src/components/sidebar/worktree-list/grouping/project-group-sections.tssrc/renderer/src/components/sidebar/worktree-list/grouping/row-builders.tssrc/renderer/src/components/sidebar/worktree-list/grouping/row-types.tssrc/renderer/src/components/sidebar/worktree-list/listing/use-collapsed-groups.tssrc/renderer/src/components/sidebar/worktree-list/listing/use-section-rows.tssrc/renderer/src/components/sidebar/worktree-list/navigation/active-descendant-option.folder-workspace.test.tssrc/renderer/src/components/sidebar/worktree-list/navigation/active-descendant-option.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/pending-reveal-inputs.tssrc/renderer/src/components/sidebar/worktree-list/navigation/render-row-lookup.folder-workspace.test.tssrc/renderer/src/components/sidebar/worktree-list/navigation/render-row-lookup.tssrc/renderer/src/components/sidebar/worktree-list/navigation/use-active-row.tssrc/renderer/src/components/sidebar/worktree-list/rows/folder-row.tsxsrc/renderer/src/components/sidebar/worktree-list/viewport/virtual-row-context.tssrc/renderer/src/components/sidebar/worktree-sidebar-row-preference.test.tssrc/renderer/src/components/sidebar/worktree-sidebar-row-preference.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| // Why dedupe: a pinned folder workspace renders in Pinned and in its group | ||
| // under duplicate-in-groups, but it is still one workspace to the badge. | ||
| if (!seenFolderWorkspaceIds.has(row.folderWorkspace.id)) { | ||
| count += 1 | ||
| seenFolderWorkspaceIds.add(row.folderWorkspace.id) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deduplicate collapsed header counts.
Lines 127-132 deduplicate only materialized folder rows. If Pinned and the natural group are collapsed, flushHeader adds both aggregate header counts and the same folder workspace increases the host badge twice. Carry folder-workspace IDs in header metadata and deduplicate them in flushHeader. Add a test with both sections collapsed.
| role="option" | ||
| aria-selected={ctx.selectedWorktreeIds.has(folderWorktreeIdentity)} | ||
| aria-current={ctx.activeWorktreeId === folderWorktree.id ? 'page' : undefined} | ||
| aria-current={isActive ? 'page' : undefined} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose only the primary duplicate as current.
With duplicate-in-groups, both copies have the same folderWorktree.id. Both options then receive aria-current="page". Use getActiveSurfaceVariant(row) and set aria-current only for the primary copy.
Port of stablyai#16799 onto current main, where the row builder moved to src/shared/sidebar-list. Pinning a folder workspace persisted isPinned and showed Unpin, but the Pinned emitter only accepted git worktrees, so the card stayed under its project group. Pin is now placement on the renderable folder set: a pinned folder workspace moves to Pinned (or duplicates there under duplicate-in-groups), and FolderWorkspaceRow carries a sectionKey so the two copies get distinct row keys, option ids, active surfaces, reveal and selection order. Folder row keys stay host-qualified. Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
Port of stablyai#16799 onto current main, where the row builder moved to src/shared/sidebar-list. Pinning a folder workspace persisted isPinned and showed Unpin, but the Pinned emitter only accepted git worktrees, so the card stayed under its project group. Pin is now placement on the renderable folder set: a pinned folder workspace moves to Pinned (or duplicates there under duplicate-in-groups), and FolderWorkspaceRow carries a sectionKey so the two copies get distinct row keys, option ids, active surfaces, reveal and selection order. Folder row keys stay host-qualified. Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
Port of stablyai#16799 onto current main, where the row builder moved to src/shared/sidebar-list. Pinning a folder workspace persisted isPinned and showed Unpin, but the Pinned emitter only accepted git worktrees, so the card stayed under its project group. Pin is now placement on the renderable folder set: a pinned folder workspace moves to Pinned (or duplicates there under duplicate-in-groups), and FolderWorkspaceRow carries a sectionKey so the two copies get distinct row keys, option ids, active surfaces, reveal and selection order. Folder row keys stay host-qualified. Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
ELI5
Pinning a folder workspace now puts it in the left Pinned list, the same way pinning a git worktree already does.
Visual Proof
Captured from the running Electron app over CDP. Same persisted
isPinned: truestate on both sides — only the code differs, so this is the fix and not a setup difference.Also verified live, through the real UI rather than the store:
showPinnedWorktreesInGroupsrenders both copies, with distinct DOM ids, and exactly one primary active surface; clicking the Pinned copy promotes it and demotes the group copy.What Changed
buildRowsalready treated pin as placement for git worktrees. Folder workspaces had anisPinnedflag, a Pin/Unpin menu item and persistence, but the Pinned emitter only acceptedWorktree[], so a pinned folder workspace stayed under its project group. Pin is now a placement choice on the same renderable folder-workspace set (#15362), partitioned right beside the existing worktree partition:FolderWorkspaceRownow carries a requiredsectionKey, and deriveskeyfrom it — exactly asWorktreeRowderivesrowKey. Rendering the same workspace twice is only safe if the two copies are distinguishable everywhere, and previously they were not: the virtualizer key, the DOM option id, the reveal walkers and the sidebar-order preference each answered "which row is this?" differently. One key now flows through all of them, and the type makes a section-less folder row unrepresentable.Falling out of that:
emitPinnedGrouptook 15 positional parameters; it now takes one options object.prefer the natural copy, fall back to Pinned) is written once and shared by worktree and folder rows.Review follow-ups
Reviewed against
/readiness-checklist; each of these is a consequence of a folder workspace newly being able to render twice, found in review and fixed here:expandGroupsForWorktreeRevealpersists every toggle it makes. Adding Pinned on top of the project-group chain permanently un-collapsed a section the card is not in. Now an either/or, mirroringgetPinnedWorktreeRevealCollapsedGroupKeys.collectRenderedSidebarRowKeysstill emitted the pre-change folder key, disagreeing withgetRenderRowSidebarKey.useEffectiveCollapsedGroupsheld the pinned display policy but did not pass it down; the active-surface lookup resolved a ref for every row on the click path.Why
The menu already said Unpin, so the pin write was working — the card just never moved. Same class of gap as #15362: Pinned was another lane the git-worktree path owned and the folder-workspace path never joined.
Testing
build-rows.pinned-folder-workspaces.test.tswas run againstmainfirst: 9 of 10 failed, with no Pinned header for a pinned folder workspace. New coverage for the review follow-ups: reveal either/or per policy, host-badge dedup, and option-id/aria-activedescendant identity for the duplicated row. Navigation fixtures build rows throughbuildFolderWorkspaceRowso they cannot drift from the real key shape.pnpm tcclean;pnpm test src/renderer26,506 passing;pnpm run check:code-quality:changed0 new findings.getFolderWorkspaceHostId, the same resolver the status/PR lanes use, so a pinned SSH folder workspace cannot leak into the local host section. The Pinned header's host map was verified identical to the previous local tally for the git-worktree-only case.Review
collapsedGroupsstores header keys only; folder-workspace rows are never headers, so no saved state references the old key. No mobile or paired-web surface imports these modules — no wire impact.buildRows(memoized, unchanged deps). Folder rows re-key once on upgrade, then only on grouping changes — the same behavior item rows have always had.encodeURIComponent-wrapped and only ever read bygetElementById.Known gaps (pre-existing, not introduced here)
Folder rows pass neither
renameRowKeynorrevealHighlight, so rename-on-reveal and the reveal glow do not fire for folder workspaces — identical onmain. Folder workspaces still do not join Cmd+Shift+Arrow cycling (that walker is item-row-only). Both are out of scope.Checklist
pnpm typecheck,pnpm test, lint pass locallyAuthorship
Original fix and problem diagnosis by @lafraia (commit 1). Commits 2-4 are a takeover pass: the section-key rework, the readiness-checklist review, and its follow-up fixes.