Repository navigation
Add sidebar folder grouping - #3988
Closed
austinywang wants to merge 14 commits into
Closed
austinywang wants to merge 14 commits into
austinywang wants to merge 14 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements workspace sidebar grouping by directory with collapsible folder sections and a pinned bookmarks area. It adds a mutable ChangesSidebar Workspace Folder Grouping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR adds folder-based grouping to the sidebar: unpinned workspaces are grouped by
Confidence Score: 5/5Safe to merge — all selection, drag-drop, and session-persistence paths are correctly updated to use the new visual order, and no functional regressions were found. The new TabManager move APIs correctly guard @published writes behind changed-flag checks, the session snapshot round-trips initialDirectory separately from currentDirectory with a safe nil-default for old sessions, and every lastSidebarSelectionIndex sync site was updated to use the grouped visual order. The only non-critical observation is the intentional switch from LazyVStack to VStack that removes a performance comment without leaving an explanation for future contributors. Sources/Sidebar/SidebarWorkspaceList.swift — the VStack/LazyVStack trade-off should be documented inline. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TabManager.tabs] -->|SidebarWorkspaceGroupingPlanner.plan| B[SidebarWorkspaceGroupingPlan]
B --> C[bookmarkIds]
B --> D[folderGroups: SidebarWorkspaceFolderGroup]
D -->|collapsedGroupIds| E[renderedWorkspaceIds]
C --> E
E --> F[SidebarWorkspaceList body]
F --> G[SidebarBookmarkHeader + rows]
F --> H[SidebarGroupHeader + rows]
F --> I[SidebarAddFolderButton]
F --> J[SidebarBonsplitTabWorkspaceDropOverlay]
G -->|drop| K[moveWorkspaceToBookmarksEnd]
H -->|drop| L[moveWorkspaceToInitialDirectoryGroupEnd]
H -->|row drop| M[moveWorkspaceInSidebarVisualOrder]
I -->|pick dir| N[addWorkspace workingDirectory]
K --> A
L --> A
M --> A
N --> A
A -->|sessionSnapshot| O[SessionWorkspaceSnapshot]
O -->|initialDirectory field| P[restoreSessionSnapshot]
P --> A
Reviews (6): Last reviewed commit: "merge: sync issue 2522 with main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
9434-9563: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the new grouped-sidebar UI and drop logic out of
ContentView.swift.This adds several new responsibilities to an already oversized production file: grouped rendering, folder headers, add-folder UI, and header-specific drop delegates. Please move these sidebar-specific types/helpers into dedicated files under
Sources/before merge.As per coding guidelines,
do not accept more than 250 lines added to an existing production Swift file that is already over 800 lines.Also applies to: 11890-12016, 14576-14672
🤖 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 `@Sources/ContentView.swift` around lines 9434 - 9563, The ContentView.swift file now contains new grouped-sidebar UI and drop logic that should be moved into focused files; extract the workspaceRows view and its helpers (workspaceRows, renderFolderGroups, WorkspaceFolderRenderGroup) plus the related custom views/delegates (SidebarBonsplitTabWorkspaceDropOverlay, SidebarGroupHeader, SidebarBookmarkHeader, SidebarAddFolderButton and any drop-related closures or preference keys) into one or more new files under Sources/ (e.g. Sidebar/SidebarWorkspaceList.swift and Sidebar/SidebarDropOverlay.swift). Ensure you preserve all external dependencies (tabManager, draggedTabId, selectedTabIds, collapsedGroups, dragAutoScrollController, dropIndicator, WorkspaceListRenderContext types) by keeping their interfaces intact and replacing the moved code in ContentView.swift with a small wrapper call to the new Sidebar workspace view.
🤖 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 `@Sources/ContentView.swift`:
- Around line 9565-9571: toggleCollapsedGroup(_:) currently allows collapsing a
group that contains the active tab, hiding the selected workspace; update this
function to check tabManager.selectedTabId against the group's member IDs and
prevent collapsing (or remove the group from collapsedGroups) if the selected
tab is inside that group, and additionally ensure any code that changes
tabManager.selectedTabId auto-expands its containing group by removing that
groupId from collapsedGroups; reference toggleCollapsedGroup(_:),
collapsedGroups, and tabManager.selectedTabId to locate where to add the guard
and where to auto-expand on selection changes.
- Around line 14833-14849: The current
sidebarVisualIndex/sidebarVisibleWorkspaceIds functions derive visible IDs from
SidebarWorkspaceGroupingPlanner.plan(...).visibleWorkspaceIds which ignores
folder collapse state, allowing lastSidebarSelectionIndex anchors to point at
hidden workspaces; update sidebarVisualIndex to compute visible/rendered
workspace order using the actual rendered order (e.g. the same source used by
VerticalTabsSidebar for expansion/collapse state) or accept that rendered IDs as
an input, and if the workspaceId is not present in that rendered list
clear/return nil so callers (and lastSidebarSelectionIndex logic) never anchor
to a hidden workspace; reference sidebarVisualIndex, sidebarVisibleWorkspaceIds
and lastSidebarSelectionIndex/VerticalTabsSidebar when making the change.
In `@Sources/TabManager.swift`:
- Around line 4066-4070: The current branch that handles nextInitialDirectory ==
workspace.initialDirectory sets insertionIndex = min(currentIndex,
remainingTabs.count), which can place a moved pinned workspace before other
pinned items and later survive an unpin, breaking pinned-first ordering; change
that branch to clamp the computed index to be no less than pinnedCount (e.g.
insertionIndex = max(min(currentIndex, remainingTabs.count), pinnedCount)) so
moved items in the same directory will always be inserted after the pinned
block, referencing nextInitialDirectory, workspace.initialDirectory,
insertionIndex, currentIndex, remainingTabs, and pinnedCount.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 9434-9563: The ContentView.swift file now contains new
grouped-sidebar UI and drop logic that should be moved into focused files;
extract the workspaceRows view and its helpers (workspaceRows,
renderFolderGroups, WorkspaceFolderRenderGroup) plus the related custom
views/delegates (SidebarBonsplitTabWorkspaceDropOverlay, SidebarGroupHeader,
SidebarBookmarkHeader, SidebarAddFolderButton and any drop-related closures or
preference keys) into one or more new files under Sources/ (e.g.
Sidebar/SidebarWorkspaceList.swift and Sidebar/SidebarDropOverlay.swift). Ensure
you preserve all external dependencies (tabManager, draggedTabId,
selectedTabIds, collapsedGroups, dragAutoScrollController, dropIndicator,
WorkspaceListRenderContext types) by keeping their interfaces intact and
replacing the moved code in ContentView.swift with a small wrapper call to the
new Sidebar workspace view.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6db4e28-6f41-49a8-b96f-3fb1c12b575f
📒 Files selected for processing (9)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/SessionPersistence.swiftSources/Sidebar/SidebarDropPlanner.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/SidebarWorkspaceDropPlannerTests.swiftcmuxTests/WorkspaceUnitTests.swift
…lder-grouping # Conflicts: # Sources/ContentView.swift # Sources/Workspace.swift # cmuxTests/WorkspaceUnitTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 5 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit caad618. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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.
Closes #2522
Summary
Testing
Note
Medium Risk
Touches core sidebar ordering, drag-and-drop, multi-select, and session persistence; regressions could mis-order workspaces or lose grouping on restore, though behavior is covered by new unit tests.
Overview
The sidebar now groups unpinned workspaces by
initialDirectory, with collapsible folder headers (shown when there is more than one folder group), a BOOKMARKS block for pinned workspaces, and an Add Folder control that opens a directory picker to spawn a workspace in that path.Selection, shift-range, Move Up/Down, and tab drag-and-drop use the rendered workspace order (respecting collapsed groups) instead of raw
TabManager.tabsorder. Drops on bookmark or folder headers move workspaces into those segments; row drops can change folder viamoveWorkspaceInSidebarVisualOrder.initialDirectoryis stored on each workspace and persisted in session snapshots separately fromcurrentDirectory(with restore and autosave hashing updates). List rendering and reorder logic live inSidebarWorkspaceList/SidebarWorkspaceListRenderPolicy, with grouping planned inSidebarWorkspaceGroupingPlanner.New EN/JA strings cover bookmarks and add-folder UI. Tests cover grouping, rendered IDs, group/bookmark moves, sidebar observation, and session restore of
initialDirectory.Reviewed by Cursor Bugbot for commit 13269a8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds folder-based grouping to the sidebar with collapsible headers, a BOOKMARKS section, and an Add Folder button, all using the visual (rendered) order for selection and drag-and-drop. Persists a per-workspace initialDirectory separate from currentDirectory (addresses Linear #2522).
New Features
Bug Fixes
Written for commit 13269a8. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests