Add option to hide sidebar workspace close button - #6985
austinywang wants to merge 17 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds a new sidebar setting to hide the workspace close button, wiring it through defaults, settings UI, search/navigation, command palette toggles, localization, persistence, and rendering. ChangesSidebar close-button setting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds a new
Confidence Score: 5/5Safe to merge — the change is a fully opt-in boolean setting with a false default that leaves all existing behavior unchanged. The implementation threads the new setting through every required layer (catalog, UI, JSON parser, template, search, command palette) with no gaps. Localization is complete across all 20 app and web locales. The layout change is intentional and correct: removing the button from the view tree when disabled reclaims width exactly as described. Tests cover the default value and the cmux.json round-trip. No actor isolation, blocking primitive, or architectural concerns were found. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User toggles sidebar.hideWorkspaceCloseButton] --> B{Setting source}
B -->|Settings UI| C[SidebarWorkspaceCloseButtonSettingsRow\nDefaultsValueModel write]
B -->|cmux.json| D[CmuxSettingsFileStore\nSidebarSettingsFileMapping]
B -->|Command Palette| E[CommandPaletteSidebarSettingsToggles\nhideWorkspaceCloseButtonInSidebar]
C --> F[UserDefaults: sidebarHideWorkspaceCloseButton]
D --> F
E --> F
F --> G[SidebarTabItemSettingsSnapshot\nhidesWorkspaceCloseButton: Bool]
G --> H{TabItemView render}
H -->|false default| I[Close button rendered\nwidth reserved, opacity-toggled on hover]
H -->|true| J[Close button omitted\nwidth reclaimed for title]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User toggles sidebar.hideWorkspaceCloseButton] --> B{Setting source}
B -->|Settings UI| C[SidebarWorkspaceCloseButtonSettingsRow\nDefaultsValueModel write]
B -->|cmux.json| D[CmuxSettingsFileStore\nSidebarSettingsFileMapping]
B -->|Command Palette| E[CommandPaletteSidebarSettingsToggles\nhideWorkspaceCloseButtonInSidebar]
C --> F[UserDefaults: sidebarHideWorkspaceCloseButton]
D --> F
E --> F
F --> G[SidebarTabItemSettingsSnapshot\nhidesWorkspaceCloseButton: Bool]
G --> H{TabItemView render}
H -->|false default| I[Close button rendered\nwidth reserved, opacity-toggled on hover]
H -->|true| J[Close button omitted\nwidth reclaimed for title]
Reviews (17): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
…quest-option-to-hide-the-sidebar-wo # Conflicts: # Resources/Localizable.xcstrings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
12515-12615: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRemove the timer-based drag teardown path.
This new
DispatchSourceTimerturns drag cleanup into timing-based synchronization. The repo rules explicitly ban timers/delayed dispatch as repair paths in production Swift, and this will keep the drag lifecycle race-prone instead of making ownership explicit.🤖 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 12515 - 12615, The drag teardown logic currently relies on a DispatchSourceTimer in the failsafe helper, which violates the no-timers/no-delayed-dispatch rule for production synchronization. Remove the timer-based path from the Sidebar drag cleanup flow by eliminating the delayed scheduling in requestClearSoon(reason:) and the pendingClearTimer/pendingClearGeneration mechanism, then make start(onRequestClear:) and the event monitors trigger cleanup directly through explicit ownership/state transitions instead of time-based cancellation.Sources: Coding guidelines, Path instructions
🤖 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 12094-12351: The workspace reorder/drop logic in ContentView is
too large and should be moved out of the view file. Extract the overlay wiring
and drag/drop flow from workspaceReorderDropOverlay,
activateSidebarWorkspaceDragIfNeeded, updateWorkspaceReorderDrop,
performWorkspaceReorderDrop, workspaceReorderPlan, performWorkspaceReorderPlan,
performCrossWindowWorkspaceDrop, clampedCrossWindowTopLevelSlot,
crossWindowTopLevelWorkspaceIds, crossWindowTopLevelPinnedWorkspaceIds,
crossWindowRawInsertIndex, and syncSidebarSelectionAfterWorkspaceReorder into a
dedicated helper/service so the view only delegates to it. Keep the behavior the
same, but give the extracted type a clear API that the view can call for
planning, applying, and clearing reorder drops.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 12515-12615: The drag teardown logic currently relies on a
DispatchSourceTimer in the failsafe helper, which violates the
no-timers/no-delayed-dispatch rule for production synchronization. Remove the
timer-based path from the Sidebar drag cleanup flow by eliminating the delayed
scheduling in requestClearSoon(reason:) and the
pendingClearTimer/pendingClearGeneration mechanism, then make
start(onRequestClear:) and the event monitors trigger cleanup directly through
explicit ownership/state transitions instead of time-based cancellation.
🪄 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: 0cc82e84-7518-425f-8157-62e08f1fa8f7
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (21)
Resources/Localizable.xcstringsSources/ContentView.swiftcmux.xcodeproj/project.pbxprojweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
💤 Files with no reviewable changes (20)
- web/messages/pt-BR.json
- web/messages/zh-CN.json
- web/messages/pl.json
- web/messages/it.json
- web/messages/es.json
- web/messages/da.json
- web/messages/zh-TW.json
- web/messages/fr.json
- web/messages/ko.json
- web/messages/ru.json
- web/messages/bs.json
- web/messages/km.json
- web/messages/tr.json
- web/messages/uk.json
- web/messages/th.json
- web/messages/no.json
- Resources/Localizable.xcstrings
- web/messages/ar.json
- web/messages/de.json
- cmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
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)
12515-12615: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRemove the timer-based drag teardown path.
This new
DispatchSourceTimerturns drag cleanup into timing-based synchronization. The repo rules explicitly ban timers/delayed dispatch as repair paths in production Swift, and this will keep the drag lifecycle race-prone instead of making ownership explicit.🤖 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 12515 - 12615, The drag teardown logic currently relies on a DispatchSourceTimer in the failsafe helper, which violates the no-timers/no-delayed-dispatch rule for production synchronization. Remove the timer-based path from the Sidebar drag cleanup flow by eliminating the delayed scheduling in requestClearSoon(reason:) and the pendingClearTimer/pendingClearGeneration mechanism, then make start(onRequestClear:) and the event monitors trigger cleanup directly through explicit ownership/state transitions instead of time-based cancellation.Sources: Coding guidelines, Path instructions
🤖 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 12094-12351: The workspace reorder/drop logic in ContentView is
too large and should be moved out of the view file. Extract the overlay wiring
and drag/drop flow from workspaceReorderDropOverlay,
activateSidebarWorkspaceDragIfNeeded, updateWorkspaceReorderDrop,
performWorkspaceReorderDrop, workspaceReorderPlan, performWorkspaceReorderPlan,
performCrossWindowWorkspaceDrop, clampedCrossWindowTopLevelSlot,
crossWindowTopLevelWorkspaceIds, crossWindowTopLevelPinnedWorkspaceIds,
crossWindowRawInsertIndex, and syncSidebarSelectionAfterWorkspaceReorder into a
dedicated helper/service so the view only delegates to it. Keep the behavior the
same, but give the extracted type a clear API that the view can call for
planning, applying, and clearing reorder drops.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 12515-12615: The drag teardown logic currently relies on a
DispatchSourceTimer in the failsafe helper, which violates the
no-timers/no-delayed-dispatch rule for production synchronization. Remove the
timer-based path from the Sidebar drag cleanup flow by eliminating the delayed
scheduling in requestClearSoon(reason:) and the
pendingClearTimer/pendingClearGeneration mechanism, then make
start(onRequestClear:) and the event monitors trigger cleanup directly through
explicit ownership/state transitions instead of time-based cancellation.
🪄 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: 0cc82e84-7518-425f-8157-62e08f1fa8f7
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (21)
Resources/Localizable.xcstringsSources/ContentView.swiftcmux.xcodeproj/project.pbxprojweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
💤 Files with no reviewable changes (20)
- web/messages/pt-BR.json
- web/messages/zh-CN.json
- web/messages/pl.json
- web/messages/it.json
- web/messages/es.json
- web/messages/da.json
- web/messages/zh-TW.json
- web/messages/fr.json
- web/messages/ko.json
- web/messages/ru.json
- web/messages/bs.json
- web/messages/km.json
- web/messages/tr.json
- web/messages/uk.json
- web/messages/th.json
- web/messages/no.json
- Resources/Localizable.xcstrings
- web/messages/ar.json
- web/messages/de.json
- cmux.xcodeproj/project.pbxproj
🛑 Comments failed to post (1)
Sources/ContentView.swift (1)
12094-12351: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Extract the workspace-reorder/drop flow out of
ContentView.swift.This adds another large behavior cluster to a file that is already far past the repo’s size budget. Keeping the reorder planner, overlay wiring, and cross-window move path here will make this sidebar path harder to test and maintain.
🤖 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 12094 - 12351, The workspace reorder/drop logic in ContentView is too large and should be moved out of the view file. Extract the overlay wiring and drag/drop flow from workspaceReorderDropOverlay, activateSidebarWorkspaceDragIfNeeded, updateWorkspaceReorderDrop, performWorkspaceReorderDrop, workspaceReorderPlan, performWorkspaceReorderPlan, performCrossWindowWorkspaceDrop, clampedCrossWindowTopLevelSlot, crossWindowTopLevelWorkspaceIds, crossWindowTopLevelPinnedWorkspaceIds, crossWindowRawInsertIndex, and syncSidebarSelectionAfterWorkspaceReorder into a dedicated helper/service so the view only delegates to it. Keep the behavior the same, but give the extracted type a clear API that the view can call for planning, applying, and clearing reorder drops.Sources: Coding guidelines, Learnings
|
CodeRabbit follow-up, corrected: I verified the two |
…quest-option-to-hide-the-sidebar-wo # Conflicts: # .github/swift-file-length-budget.tsv
Add DocC documentation to the new public sidebar.hideWorkspaceCloseButton DefaultsKey to satisfy the Aziz public-symbol documentation policy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sidebar-wo Regenerated .github/swift-file-length-budget.tsv via scripts/swift_file_length_budget.py --write-budget (budget respected). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…quest-option-to-hide-the-sidebar-wo # Conflicts: # cmux.xcodeproj/project.pbxproj
…quest-option-to-hide-the-sidebar-wo # Conflicts: # .github/swift-file-length-budget.tsv
…quest-option-to-hide-the-sidebar-wo # Conflicts: # .github/swift-file-length-budget.tsv
…quest-option-to-hide-the-sidebar-wo # Conflicts: # .github/swift-file-length-budget.tsv
…quest-option-to-hide-the-sidebar-wo # Conflicts: # .github/swift-file-length-budget.tsv
|
Addressed CodeRabbit's Cmux No Ambient Global State finding in c04d900: the sidebar descriptor assembly now lives on the constructable CommandPaletteSidebarSettingsToggles value type, and the static sidebarDescriptors API was removed. |
…quest-option-to-hide-the-sidebar-wo
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Fixes #5635
Summary
sidebar.hideWorkspaceCloseButtonto the settings catalog, cmux.json schema/parser/template, Settings > Sidebar, settings search, and command palette toggles.Testing
git diff --checkjq empty Resources/Localizable.xcstrings web/messages/*.json web/data/cmux.schema.jsonpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsvpython3 scripts/check-workspace-package-groups.py --checkpython3 scripts/check-package-resolved-policy.py./scripts/lint-pbxproj-test-wiring.shscripts/check-pbxproj.shskills/cmux-settings/scripts/cmux-settings list-supported | rg '^sidebar\\.(hideWorkspaceCloseButton|wrapWorkspaceTitles|hideAllDetails)$'swift testinPackages/macOS/CmuxSettingsswift testinPackages/macOS/CmuxSettingsUINo dev app build or launch was run per this issue task's instruction.
Demo Video
Not recorded. This change is an opt-in settings/layout behavior, and this run is explicitly constrained not to launch a dev app.
Review Trigger
Please review the current PR head, including the full localization update for all app and web locales.
Checklist
Fixes #5635Summary by CodeRabbit