Repository navigation
Redesign AppKit sidebar groups and live reordering - #9446
lawrencecchen wants to merge 33 commits into
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:
📝 WalkthroughWalkthroughThe PR adds configurable sidebar workspace spacing with settings, persistence, JSON support, localization, and runtime layout updates. It also updates sidebar Markdown rendering, typography, unread badges, accessibility metadata, and related tests. ChangesSidebar rendering and presentation
Workspace spacing contract and settings surface
Sidebar spacing propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ContentView
participant SidebarWorkspaceTableView
participant SidebarWorkspaceTableController
participant NSTableView
ContentView->>SidebarWorkspaceTableView: Pass configured rowSpacing
SidebarWorkspaceTableView->>SidebarWorkspaceTableController: Apply rowSpacing
SidebarWorkspaceTableController->>SidebarWorkspaceTableController: Clamp and stage value
SidebarWorkspaceTableController->>NSTableView: Update intercell spacing and layout
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@cmuxTests/SidebarAppKitRowCellTests.swift`:
- Around line 353-383: Extend workspaceTitleRendersMarkdownWithRegularBaseWeight
and workspaceDescriptionPreservesMarkdownFontTraits to assert both rendered
strings exclude Markdown delimiters and both fields use the required 12.5-point
base font. Reuse the existing font inspection helpers where applicable, while
preserving the current bold, italic, and regular-trait assertions.
- Around line 385-400: The test
unreadBadgeExpandsForLargeCountsAndKeepsFullAccessibleCount only covers the
plural unread-count branch. Add a separate count-1 case using
Self.configuredCell and assert the badge’s accessibilityLabel() uses the
singular ICU .one localization value, while retaining the existing 125-count
.other assertion.
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection.swift`:
- Line 16: Make the `@State` property workspaceSpacing private to satisfy SwiftUI
state encapsulation. Update workspaceSpacingRow in
SidebarSection+WorkspaceSpacing.swift by moving it into SidebarSection.swift or
extracting a separate row view that receives a Binding, preserving its current
behavior while avoiding direct access to the private state.
In `@Resources/Localizable.xcstrings`:
- Around line 190620-190636: Complete localization coverage in
Resources/Localizable.xcstrings for the three workspace-spacing keys:
settings.search.alias.setting.sidebarAppearance.workspace-spacing at lines
190620-190636, settings.sidebarAppearance.workspaceSpacing at lines
194985-195018, and settings.sidebarAppearance.workspaceSpacing.subtitle. Add
translated values for every locale currently supported, preserving the existing
English and Japanese entries.
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift`:
- Around line 443-450: Remove the DEBUG-only diagnostic in the title-paint path
that logs displayTitle from the sidebar row cell. If retaining a diagnostic is
necessary, replace the raw workspace title with non-sensitive metadata such as
its byte count, while preserving the existing mismatch check and avoiding any
user-controlled title content.
- Around line 618-621: Update the unread accessibility label construction near
model.unreadCount to select explicit ICU-style .one and .other localization keys
based on whether the count equals one or not, instead of using
String.localizedStringWithFormat. Add matching catalog entries for both keys
with locale-appropriate singular and plural wording.
In `@web/data/cmux.schema.json`:
- Around line 1047-1054: Add non-empty localized entries for
schemaDescriptions.sidebar.workspaceSpacing in the sidebar sections of
web/messages/en.json and web/messages/ja.json, matching the existing message
structure so the referenced schema description resolves in both locales.
🪄 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 Plus
Run ID: c7e1f3f9-3811-4ee4-b863-938d11df85fb
📒 Files selected for processing (48)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarMarkdownRenderer.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/SidebarCatalogSection.swiftPackages/macOS/CmuxSettings/Tests/CmuxSettingsTests/UserDefaultsSettingsClientTests.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection+WorkspaceSpacing.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSupportViews.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableApplyInput.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableController.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableView.swiftSources/SidebarWorkspaceGroupHeaderMetrics.swiftcmuxTests/SidebarAppKitRowCellTests.swiftcmuxTests/SidebarMarkdownRendererTests.swiftcmuxTests/SidebarWidthPolicyTests.swiftcmuxTests/SidebarWorkspaceTableSuspensionTests.swiftweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.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.jsonweb/tests/docs-search-index.test.ts
254f394 to
64621da
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Resources/Localizable.xcstrings (1)
190620-190636:⚠️ Potential issue | 🟠 MajorComplete localization coverage for the new workspace-spacing keys.
The three new keys contain only
enandja, while the catalog supports additional locales. Add translated entries for every supported locale.
Resources/Localizable.xcstrings#L190620-L190636: add translations forsettings.search.alias.setting.sidebarAppearance.workspace-spacing.Resources/Localizable.xcstrings#L194985-L195018: add translations forsettings.sidebarAppearance.workspaceSpacingandsettings.sidebarAppearance.workspaceSpacing.subtitle.As per path instructions, changed catalog keys require complete coverage across supported locales. Based on learnings, this repository treats the full internationalization rule as authoritative.
🤖 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 `@Resources/Localizable.xcstrings` around lines 190620 - 190636, Complete localization coverage for the three workspace-spacing keys: Resources/Localizable.xcstrings lines 190620-190636 requires translated entries for every supported locale for settings.search.alias.setting.sidebarAppearance.workspace-spacing; lines 194985-195018 likewise require every supported locale for settings.sidebarAppearance.workspaceSpacing and settings.sidebarAppearance.workspaceSpacing.subtitle. Preserve the existing English and Japanese entries while adding appropriate translations for all remaining supported locales.Sources: Path instructions, Learnings
🤖 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.
Duplicate comments:
In `@Resources/Localizable.xcstrings`:
- Around line 190620-190636: Complete localization coverage for the three
workspace-spacing keys: Resources/Localizable.xcstrings lines 190620-190636
requires translated entries for every supported locale for
settings.search.alias.setting.sidebarAppearance.workspace-spacing; lines
194985-195018 likewise require every supported locale for
settings.sidebarAppearance.workspaceSpacing and
settings.sidebarAppearance.workspaceSpacing.subtitle. Preserve the existing
English and Japanese entries while adding appropriate translations for all
remaining supported locales.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 08d929b8-35bf-4449-8029-0d3ef460f6b8
📒 Files selected for processing (44)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarMarkdownRenderer.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/SidebarCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection+WorkspaceSpacing.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSupportViews.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableApplyInput.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableController.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableView.swiftSources/SidebarWorkspaceGroupHeaderMetrics.swiftcmuxTests/SidebarWidthPolicyTests.swiftweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.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.jsonweb/tests/docs-search-index.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/SidebarAppKitRowCellTests.swift (2)
454-480: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare pin positions in a common coordinate space.
frame.minXis expressed in each pin view’s immediate superview coordinate space, so this comparison does not check alignment inside the workspace and group cells. Convert both pin origins to their enclosing cells before comparing them.Proposed test change
- `#expect`(groupPin.frame.minX == workspacePin.frame.minX) + let workspacePinX = workspacePin.convert(.zero, to: workspace).x + let groupPinX = groupPin.convert(.zero, to: group).x + `#expect`(abs(groupPinX - workspacePinX) < 0.5)🤖 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 `@cmuxTests/SidebarAppKitRowCellTests.swift` around lines 454 - 480, Update groupPinAlignsWithWorkspacePin to convert both pin origins into their enclosing cell coordinate spaces before comparing horizontal positions; compare the converted minX values rather than the raw frame.minX values, while preserving the existing pin discovery and alignment assertion.
428-452: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck every attributed font run in the group name.
Line [442] checks only
NSTextField.font.SidebarMarkdownRendererproduces per-run fonts, so a bold or italic run can use the wrong point size while this assertion passes. Assert every group-name run is 12.5 points.Proposed test change
- `#expect`(name.font?.pointSize == 12.5) + `#expect`(Self.fontPointSizes(in: name).allSatisfy { abs($0 - 12.5) < 0.001 })🤖 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 `@cmuxTests/SidebarAppKitRowCellTests.swift` around lines 428 - 452, Update groupHeaderUsesMarkdownRegularWorkspaceSizedTextWithoutFolderIcon to inspect every attributed font run in the group-name NSTextField, rather than relying only on name.font. Assert that each run’s font has a 12.5-point size while preserving the existing bold, italic, plain-run, text, and icon assertions.
🤖 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.
Outside diff comments:
In `@cmuxTests/SidebarAppKitRowCellTests.swift`:
- Around line 454-480: Update groupPinAlignsWithWorkspacePin to convert both pin
origins into their enclosing cell coordinate spaces before comparing horizontal
positions; compare the converted minX values rather than the raw frame.minX
values, while preserving the existing pin discovery and alignment assertion.
- Around line 428-452: Update
groupHeaderUsesMarkdownRegularWorkspaceSizedTextWithoutFolderIcon to inspect
every attributed font run in the group-name NSTextField, rather than relying
only on name.font. Assert that each run’s font has a 12.5-point size while
preserving the existing bold, italic, plain-run, text, and icon assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 030c0a60-dd33-48e4-a603-bec469b4ec15
📒 Files selected for processing (1)
cmuxTests/SidebarAppKitRowCellTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
782f5cd to
2b6a398
Compare
2b6a398 to
97fbbef
Compare
…ce-markdown # Conflicts: # cmuxTests/SidebarAppKitRowCellTests.swift
|
Optical unread badge follow-up for the third centering report. Head: Commits:
Hosted proof:
Dogfood/preflight:
|
…ce-markdown # Conflicts: # cmuxTests/SidebarAppKitRowCellTests.swift
|
Final gray Liquid Glass round Head:
Proof:
|
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Summary:
sidebar.workspaceSpacingfrom 0 to 8 pt with a compact 1 pt default, Settings UI, schema documentation, and localization.Implementation:
SidebarWorkspaceTableControllerowns transient reorder rows, nativeNSTableView.moveRowanimation, rollback, and authoritative reconciliation.Testing:
SidebarDragStateRegistryTestspassed locally with Swift Package Manager.sidearc11./tmp/cmux-debug-sidearc11.sockcompleted 40-step and 100-step AppKit drag simulations without aborting, restored canonical order after cancellation, and captured expanded, collapsed, and live group-block states.Preview: