feat(chat): dedupe workspace labels, split sidebar - #2142
Merged
Conversation
Duplicate workspace basenames get the shortest parent-path suffix (app · team-a / app · archive) computed over any given set of labels. Windows and POSIX separators are handled consistently; unique labels keep their compact form. (#2121)
Extract the 2.5k-line WindowSideBar script into seven sidebar composables (workspace groups, pin flight, list auto-fill, session shortcuts, group reorder, workspace actions, remote control) and replace raw window/document API usage with VueUse primitives (useEventListener, useDocumentVisibility, useResizeObserver, useTimeoutFn, usePreferredReducedMotion). Also lands in the sidebar itself: - duplicate workspace labels rendered with minimal parent context over the visible group set, full normalized path on the group header title and an sr-only span (#2121) - a subtle chat-section card background plus a divider above the workspace section so the two lists read separately Template and DOM structure are unchanged. The two tests that poked component internals via wrapper.vm now go through real window events and the poll timer instead.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe sidebar moves remote control, workspace grouping and actions, project reordering, session autofill, pin animations, and shortcuts into dedicated composables. Workspace labels now disambiguate duplicate names with parent paths. Tests cover these behaviors. ChangesSidebar modularization and workspace UX
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
zhangmo8
requested review from
yyhhyyyyyy and
zerob13
and removed request for
yyhhyyyyyy and
zerob13
August 11, 2026 11:58
The sidebar's only scrollTop assignment moved from WindowSideBar.vue into useSessionListAutoFill.ts during the composable split; re-register it under the new path.
zhangmo8
marked this pull request as ready for review
August 12, 2026 02:03
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/shared/utils/workspaceLabels.ts (1)
53-61: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe fallback branch does not normalize separators.
The doc comment states that the suffix always renders with
/. The fallback usesitem.idunchanged, so a Windows id renders asapp · C:\work\app. The branch is reachable only when two items share the same label and the same parent chain, which implies identical paths, so impact is very low. If you keep the branch, join the split segments instead of using the raw id.♻️ Optional normalization of the fallback path
- const uniqueContext = contextCounts.get(context) === 1 ? context : item.id + const uniqueContext = + contextCounts.get(context) === 1 + ? context + : item.id.split(/[\\/]+/).filter(Boolean).join('/')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/utils/workspaceLabels.ts` around lines 53 - 61, Normalize the fallback suffix in the forEach block before storing the override: replace the raw item.id used when contextCounts.get(context) is not unique with the existing path-segment normalization so separators always render as “/”. Preserve the unique-context branch and the `${item.label} · ${uniqueContext}` format.src/renderer/src/components/WindowSideBar.vue (1)
424-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe path is exposed twice to assistive technology.
The button carries
titlewith the normalized path, and it also contains ansr-onlyspan with the same path. Some screen readers announce the accessible name from the content and then thetitleas a description, so the path can be read twice. If you want the tooltip and the announcement, keeptitleand give the group anaria-describedbythat points at thesr-onlyspan, or drop thesr-onlyspan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/WindowSideBar.vue` around lines 424 - 441, Update the project-directory group accessibility markup around getGroupIdentifier(group) so the normalized path is exposed only once to assistive technology: either remove the duplicate sr-only path text or keep it and reference it via a unique aria-describedby instead of relying on the button title as an additional announcement. Preserve the visual title tooltip behavior.src/renderer/src/composables/sidebar/useSidebarWorkspaceGroups.ts (1)
280-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDisambiguation ignores localized group labels.
disambiguateWorkspaceLabelscomparesgroup.labelonly. Groups that carry alabelKeyare excluded byisProjectDirectoryGroup, so this is correct today. If a future workspace group gains alabelKey, two groups could still render the same translated text without an override. Consider passing the resolved display label into this computed if that case appears.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/composables/sidebar/useSidebarWorkspaceGroups.ts` around lines 280 - 297, The current disambiguation uses raw group.label values, so localized groups with labelKey could render duplicate translated names. In workspaceGroups, resolve each group’s actual display label before passing it to disambiguateWorkspaceLabels, while preserving the existing filtering and label override mapping.src/renderer/src/composables/sidebar/useSessionListAutoFill.ts (1)
100-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a rejected
loadNextPageinside the fill loop.
sessionStore.loadNextPage()is awaited without error handling. Every caller invokesensureSessionListFilledwithvoid(line 123 here, anduseProjectGroupReorder.tsline 157). If the store rejects, the result is an unhandled promise rejection. Thefinallyblock still resetsisFillingSessionList, so the loop does not lock, but the failure is silent to the log and noisy to the runtime.Catch the rejection and stop the loop.
♻️ Proposed change
const beforeCount = sessionStore.sessions.length const beforeHasMore = sessionStore.hasMore - await sessionStore.loadNextPage() + try { + await sessionStore.loadNextPage() + } catch (error) { + console.warn('[useSessionListAutoFill] Failed to load next session page:', error) + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/composables/sidebar/useSessionListAutoFill.ts` around lines 100 - 110, Update ensureSessionListFilled around sessionStore.loadNextPage() to catch rejected loads, stop the fill loop, and prevent the rejection from escaping the void-invoked caller. Preserve the existing finally cleanup that resets isFillingSessionList.src/renderer/src/composables/sidebar/useSidebarSessionShortcuts.ts (1)
69-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the
Alt+badge label through vue-i18n.Line 71 hardcodes the
Alt+prefix. This string renders in the sidebar as user-visible copy. The⌘symbol is locale-neutral, but theAltkey name is not.Move the modifier label to a locale key and resolve it with
t().As per coding guidelines,
src/renderer/**/*.{vue,ts,tsx}: "Use vue-i18n for user-facing copy, and prefer existing shadcn-vue primitives and VueUse utilities."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/composables/sidebar/useSidebarSessionShortcuts.ts` around lines 69 - 72, Update getShortcutBadgeLabelForIndex to obtain the non-Mac modifier label through the composable’s vue-i18n t() function instead of hardcoding “Alt+”; add or reuse the appropriate locale key for that modifier while preserving the Mac ⌘ label and existing digit formatting.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/composables/sidebar/useProjectGroupReorder.ts`:
- Around line 62-88: Validate that nextVisiblePaths has the same number of
entries as previousVisiblePaths before mutating nextOrder in
commitVisibleProjectGroupOrder. If the lengths differ, return without calling
projectStore.reorderEnvironments; otherwise preserve the existing reorder logic.
In `@src/renderer/src/composables/sidebar/useSessionPinFlight.ts`:
- Around line 291-299: Guard the pinFlightSessionId reset in the finally block
of the pin-flight flow using the same session-id ownership check as
pinDockedSessionId, so an earlier flight cannot clear the ID belonging to a
later session. Keep clone.remove() unconditional.
In `@src/renderer/src/composables/sidebar/useSidebarRemoteControl.ts`:
- Around line 108-157: Add a composable-local refresh sequence token to
refreshRemoteControlStatus and increment it for each invocation, then ignore any
result whose token is no longer current before replaceRemoteSnapshot. Apply the
same supersession check in runStatusRefresh before updating statusRefreshErrors
or scheduling another poll, so stale overlapping requests cannot mutate state.
---
Nitpick comments:
In `@src/renderer/src/components/WindowSideBar.vue`:
- Around line 424-441: Update the project-directory group accessibility markup
around getGroupIdentifier(group) so the normalized path is exposed only once to
assistive technology: either remove the duplicate sr-only path text or keep it
and reference it via a unique aria-describedby instead of relying on the button
title as an additional announcement. Preserve the visual title tooltip behavior.
In `@src/renderer/src/composables/sidebar/useSessionListAutoFill.ts`:
- Around line 100-110: Update ensureSessionListFilled around
sessionStore.loadNextPage() to catch rejected loads, stop the fill loop, and
prevent the rejection from escaping the void-invoked caller. Preserve the
existing finally cleanup that resets isFillingSessionList.
In `@src/renderer/src/composables/sidebar/useSidebarSessionShortcuts.ts`:
- Around line 69-72: Update getShortcutBadgeLabelForIndex to obtain the non-Mac
modifier label through the composable’s vue-i18n t() function instead of
hardcoding “Alt+”; add or reuse the appropriate locale key for that modifier
while preserving the Mac ⌘ label and existing digit formatting.
In `@src/renderer/src/composables/sidebar/useSidebarWorkspaceGroups.ts`:
- Around line 280-297: The current disambiguation uses raw group.label values,
so localized groups with labelKey could render duplicate translated names. In
workspaceGroups, resolve each group’s actual display label before passing it to
disambiguateWorkspaceLabels, while preserving the existing filtering and label
override mapping.
In `@src/shared/utils/workspaceLabels.ts`:
- Around line 53-61: Normalize the fallback suffix in the forEach block before
storing the override: replace the raw item.id used when
contextCounts.get(context) is not unique with the existing path-segment
normalization so separators always render as “/”. Preserve the unique-context
branch and the `${item.label} · ${uniqueContext}` format.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a7fcc12-7c2d-4a51-a0a3-c741ab5fafa5
📒 Files selected for processing (12)
src/renderer/src/components/WindowSideBar.vuesrc/renderer/src/composables/sidebar/useProjectGroupReorder.tssrc/renderer/src/composables/sidebar/useSessionListAutoFill.tssrc/renderer/src/composables/sidebar/useSessionPinFlight.tssrc/renderer/src/composables/sidebar/useSidebarRemoteControl.tssrc/renderer/src/composables/sidebar/useSidebarSessionShortcuts.tssrc/renderer/src/composables/sidebar/useSidebarWorkspaceActions.tssrc/renderer/src/composables/sidebar/useSidebarWorkspaceGroups.tssrc/shared/utils/workspaceLabels.tstest/main/shared/workspaceLabels.test.tstest/renderer/components/WindowSideBar.test.tstest/renderer/composables/chat/chatScrollArchitecture.test.ts
- Skip the project group reorder commit when the visible path count mismatches, so a short list cannot persist a duplicated order - Only clear pinFlightSessionId when still owned by the finishing flight, mirroring the pinDockedSessionId guards - Drop superseded remote status refreshes so a stale overlapping request cannot replace a newer snapshot, count toward backoff, or fork the poll chain; add a regression test for the overlap Addresses CodeRabbit review feedback on #2142. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/composables/sidebar/useProjectGroupReorder.ts`:
- Around line 65-68: Update the project group reorder validation in the reorder
flow around nextVisiblePaths and previousVisiblePaths to abort unless both lists
contain the same unique paths, not merely equal lengths. Verify membership and
uniqueness for each list before the persistence loop, preserving the existing
warning-and-return behavior when validation fails.
In `@src/renderer/src/composables/sidebar/useSidebarRemoteControl.ts`:
- Around line 114-126: Update refreshRemoteControlStatus to propagate the
boolean result from pluginCatalogStore.replaceRemoteSnapshot instead of always
returning true after the guarded write. Ensure runStatusRefresh handles a false
or distinct superseded outcome without clearing statusRefreshErrors or
scheduling the next poll as a successful refresh.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8437d87-d287-4c8b-9284-23f3fe75494b
📒 Files selected for processing (4)
src/renderer/src/composables/sidebar/useProjectGroupReorder.tssrc/renderer/src/composables/sidebar/useSessionPinFlight.tssrc/renderer/src/composables/sidebar/useSidebarRemoteControl.tstest/renderer/components/WindowSideBar.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/renderer/components/WindowSideBar.test.ts
zerob13
reviewed
Aug 12, 2026
zerob13
left a comment
zerob13
left a comment
Collaborator
There was a problem hiding this comment.
Targeted checks pass (workspaceLabels, WindowSideBar, the scroll-architecture test, both typechecks, Oxfmt, and Oxlint). I found three additional issues below. The existing unresolved threads about reorder-path membership and the guarded remote snapshot result also still need attention.
yyhhyyyyyy
reviewed
Aug 12, 2026
- workspaceLabels: resolve each duplicate at its own shortest unique parent suffix instead of one depth per collision bucket - WindowSideBar: replace the group header title/sr-only path with a hover+focus tooltip associated to the button - useSessionPinFlight: serialize pin toggles so overlapping flights cannot fight over the flight/docked ownership refs - useProjectGroupReorder: reject non-permutation visible path lists, not just count mismatches - useSidebarRemoteControl: distinguish applied/discarded/failed refresh outcomes so a discarded snapshot neither resets nor grows backoff
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
app · team-a/app · archive), computed reactively over the currently visible group set (session groups and environment-only groups). Unique basenames keep their compact label; when a colliding workspace is archived/removed, the survivor shrinks back automatically. The full normalized path is exposed on the group header viatitleand ansr-onlyspan for keyboard/AT users.bg-muted/30rounded card and the workspace title a hairline top divider (shown only when there is content above), so the two identical-looking lists read as separate zones. Class-only change, no DOM restructuring.WindowSideBar.vue2513 → 1199 lines — the script is split into seven focused composables undercomposables/sidebar/; rawwindow.*/document.*usage is replaced with VueUse primitives (useEventListener,useDocumentVisibility,useResizeObserver,useTimeoutFn,usePreferredReducedMotion,tryOnScopeDispose). Template and DOM structure are unchanged.BEFORE / AFTER (sidebar layout)
New composables
useSidebarWorkspaceGroupsuseSessionPinFlightuseSessionListAutoFilluseSidebarSessionShortcutsuseProjectGroupReorderuseSidebarWorkspaceActionsuseSidebarRemoteControlVerification
test/main/shared/workspaceLabels.test.ts— 6 unit tests (same basename, nested duplicates, cross-platform separators, parentless paths, collision removal)WindowSideBar.test.ts65/65,WindowSideBarSessionItem+App.startup24/24,test/main/shared112/112; full renderer suite passes except 4 files already failing ondev(reproduced on a clean tree, unrelated)wrapper.vmwere rewritten against observable behavior (real window keydown, poll timer)oxfmt,oxlint,typecheck:web,typecheck:node,pnpm run lint,pnpm run i18nall clean_electron): disambiguated labels, sr-only paths, section separation render correctly with zero renderer console errorsdev: cold-start pagination (30-per-page; sessions of later groups appear after scrolling) is identical before/after this PR — pre-existing behavior, see note belowNote for reviewers
While verifying, we confirmed a pre-existing UX quirk (unchanged by this PR): in project grouping, managed-environment group headers always render, but their sessions only appear once pagination reaches them, so a group can look temporarily empty on cold start. Worth a separate issue; happy to file it.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests