Fix AppKit sidebar settings fidelity - #8432
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChangesThe sidebar now resolves branch-directory and workspace-detail preferences through shared settings snapshots. SwiftUI and AppKit rendering consume the same structured layout, placement, visibility, and agent-activity values, with tests covering defaults and stored preferences. Sidebar settings parity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UserDefaults
participant SettingsSnapshot
participant SnapshotFactory
participant SwiftUI Sidebar
participant AppKit Row
UserDefaults->>SettingsSnapshot: resolve sidebar preferences
SettingsSnapshot->>SnapshotFactory: provide branch and detail settings
SnapshotFactory->>SwiftUI Sidebar: build sidebar snapshot
SnapshotFactory->>AppKit Row: provide row settings and branch data
SwiftUI Sidebar->>AppKit Row: render matching layout values
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 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 |
![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
🤖 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`:
- Line 69: Update the test fixture helpers supplying showsAgentActivity to
require both resolvedSettings.details.showAgentActivity and
isSidebarWorkspaceAgentSpinnerEnabled, matching the production feature gate.
Apply the same change to both referenced fixture locations and preserve the
existing preference-based behavior otherwise.
- Around line 243-355: Update the tests around makeSwiftUIRow and makeModel to
inspect generated presentation/row content rather than only comparing propagated
settings. Add representative assertions for vertical, inline-stacked, and
inline-combined branch layouts, plus path and workspace-detail rendering, so
each renderer’s behavior is validated when settings vary.
🪄 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: b2323363-3413-4005-9aad-639e2afa08b0
📒 Files selected for processing (11)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/SidebarCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/SidebarTabItemSettingsSnapshot.swiftSources/SidebarWorkspaceBranchDirectorySettings.swiftSources/SidebarWorkspaceDetailSettings.swiftSources/SidebarWorkspaceSnapshotFactory.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarAppKitRowCellTests.swift
Contributor
Greptile SummaryThis PR consolidates sidebar settings resolution into a single immutable
Confidence Score: 4/5The change is well-tested and architecturally sound — SidebarTabItemSettingsStore observes all UserDefaults changes, so every setting that moved into the shared snapshot (including showAgentActivity) still drives SwiftUI re-renders correctly. Three extension methods in CmuxSettingsJSONPathSupport.swift create a fresh SidebarCatalogSection() when the enum's private static let sidebar is accessible in the same file; this is a minor inconsistency rather than a correctness problem. No actor isolation issues, no blocking primitives, no raw key or default drift, and the two-setting compatibility contract is correctly encoded and tested. The three utility methods in CmuxSettingsJSONPathSupport.swift (showPullRequestsValue, showBranchDirectoryValue, watchGitStatusValue) could reuse the existing private static instead of allocating fresh instances, but this has no correctness impact. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
UD[UserDefaults]
STORE[SidebarTabItemSettingsStore\nObservableObject]
SNAP[SidebarTabItemSettingsSnapshot\nEquatable struct]
BD[SidebarWorkspaceBranchDirectorySettings\nbranchLayout · branchDirectoryPlacement]
DD[SidebarWorkspaceDetailSettings\nshowAgentActivity · showSSH · etc.]
CAT[SidebarCatalogSection\nDefaultsKey definitions\nstacksBranchAndDirectory]
UD -->|didChangeNotification| STORE
STORE -->|refreshSnapshot| SNAP
CAT --> BD
CAT --> DD
BD --> SNAP
DD --> SNAP
SNAP --> SWIFTUI[TabItemView\nSwiftUI renderer]
SNAP --> APPKIT[SidebarWorkspaceRowTableCellView\nAppKit renderer]
SNAP --> FACTORY[SidebarWorkspaceSnapshotFactory]
style CAT fill:#e8f4e8
style SNAP fill:#dbeafe
style BD fill:#dbeafe
style DD fill:#dbeafe
%%{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
UD[UserDefaults]
STORE[SidebarTabItemSettingsStore\nObservableObject]
SNAP[SidebarTabItemSettingsSnapshot\nEquatable struct]
BD[SidebarWorkspaceBranchDirectorySettings\nbranchLayout · branchDirectoryPlacement]
DD[SidebarWorkspaceDetailSettings\nshowAgentActivity · showSSH · etc.]
CAT[SidebarCatalogSection\nDefaultsKey definitions\nstacksBranchAndDirectory]
UD -->|didChangeNotification| STORE
STORE -->|refreshSnapshot| SNAP
CAT --> BD
CAT --> DD
BD --> SNAP
DD --> SNAP
SNAP --> SWIFTUI[TabItemView\nSwiftUI renderer]
SNAP --> APPKIT[SidebarWorkspaceRowTableCellView\nAppKit renderer]
SNAP --> FACTORY[SidebarWorkspaceSnapshotFactory]
style CAT fill:#e8f4e8
style SNAP fill:#dbeafe
style BD fill:#dbeafe
style DD fill:#dbeafe
Reviews (1): Last reviewed commit: "fix: honor legacy stacked sidebar layout" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
49
to
59
| static func showPullRequestsValue(defaults: UserDefaults) -> Bool { | ||
| boolValue(defaults: defaults, key: showPullRequestsKey, defaultValue: showPullRequests) | ||
| UserDefaultsSettingsClient(defaults: defaults).value(for: SidebarCatalogSection().showPullRequests) | ||
| } | ||
|
|
||
| static func showBranchDirectoryValue(defaults: UserDefaults) -> Bool { | ||
| boolValue(defaults: defaults, key: showBranchDirectoryKey, defaultValue: showBranchDirectory) | ||
| UserDefaultsSettingsClient(defaults: defaults).value(for: SidebarCatalogSection().showBranchDirectory) | ||
| } | ||
|
|
||
| static func watchGitStatusValue(defaults: UserDefaults) -> Bool { | ||
| boolValue(defaults: defaults, key: watchGitStatusKey, defaultValue: watchGitStatus) | ||
| UserDefaultsSettingsClient(defaults: defaults).value(for: SidebarCatalogSection().watchGitStatus) | ||
| } |
Contributor
There was a problem hiding this comment.
SidebarCatalogSection() and a fresh UserDefaultsSettingsClient, while the private sidebar instance declared in the enum body is accessible from same-file extensions and already caches the catalog keys. Reusing the shared instance avoids the redundant allocation.
Suggested change
| static func showPullRequestsValue(defaults: UserDefaults) -> Bool { | |
| boolValue(defaults: defaults, key: showPullRequestsKey, defaultValue: showPullRequests) | |
| UserDefaultsSettingsClient(defaults: defaults).value(for: SidebarCatalogSection().showPullRequests) | |
| } | |
| static func showBranchDirectoryValue(defaults: UserDefaults) -> Bool { | |
| boolValue(defaults: defaults, key: showBranchDirectoryKey, defaultValue: showBranchDirectory) | |
| UserDefaultsSettingsClient(defaults: defaults).value(for: SidebarCatalogSection().showBranchDirectory) | |
| } | |
| static func watchGitStatusValue(defaults: UserDefaults) -> Bool { | |
| boolValue(defaults: defaults, key: watchGitStatusKey, defaultValue: watchGitStatus) | |
| UserDefaultsSettingsClient(defaults: defaults).value(for: SidebarCatalogSection().watchGitStatus) | |
| } | |
| static func showPullRequestsValue(defaults: UserDefaults) -> Bool { | |
| UserDefaultsSettingsClient(defaults: defaults).value(for: sidebar.showPullRequests) | |
| } | |
| static func showBranchDirectoryValue(defaults: UserDefaults) -> Bool { | |
| UserDefaultsSettingsClient(defaults: defaults).value(for: sidebar.showBranchDirectory) | |
| } | |
| static func watchGitStatusValue(defaults: UserDefaults) -> Bool { | |
| UserDefaultsSettingsClient(defaults: defaults).value(for: sidebar.watchGitStatus) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
There was a problem hiding this comment.
Fixed in 029b2fd. The three helpers now reuse SidebarWorkspaceDetailDefaults.sidebar rather than constructing another SidebarCatalogSection.
— Claude Code
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
1 participant
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
Closes #8430
Issue: #8430
Two-setting compatibility contract
These settings are deliberately distinct:
sidebarBranchVerticalLayout/sidebar.branchVerticalLayoutis the shipped Bool-backed legacy topology preference (defaulttrue).trueuses vertical branch records and preserves its historical stacked branch/directory subrows.falseuses the compact branch topology. The key remains Bool-backed, and this PR performs no migration or stored-value rewrite.sidebarBranchDirectoryStacked/sidebar.stackBranchDirectoryis the newer placement preference (defaultfalse). When the legacy topology is inline, it independently chooses whether branch and directory share one line or use separate subrows. Vertical legacy layout already implies stacked placement, so Settings displays the effective toggle as on and disables it until Inline layout is selected.The resulting truth table is:
truefalseortruefalsefalsefalsetrueStructural fix and audit
SidebarTabItemSettingsSnapshotis now the single resolved input for both renderers. It owns catalog-backedSidebarWorkspaceBranchDirectorySettingsandSidebarWorkspaceDetailSettings, so AppKit no longer maintains a parallel raw-defaults interpretation.The audit covers all workspace detail defaults (
showBranchDirectory,showPullRequests,watchGitStatus,showSSH,showPorts,showLog,showProgress,showAgentActivity, andshowCustomMetadata), title wrapping, path presentation, shortcut hints, colors, indicator positions, browser-link behavior, notification details, and the remaining settings consumed by AppKit rows. Behavior tests pass the same resolved snapshot into both row models and verify defaults plus stored overrides.Coordination
Verification
15d58b93b2thenb49710feea3b81ee6d5cthen5bdac63059(fresh-default stacked rendering caught during dogfood)arch -arm64 swift testinPackages/macOS/CmuxSettings— 260 tests passedarch -arm64 swift testinPackages/macOS/CmuxSettingsUI— 115 tests passedscripts/check-pbxproj.shscripts/lint-pbxproj-test-wiring.sh— 528 test files checkedgit diff --checkSidebarSection.swiftremains at 499; neither budget TSV changed (the requested file-length script is absent on this base)Build command:
Visual verification
The tagged defaults domain was verified with the AppKit flag enabled and both layout keys absent before launch. Before/after and both saved-preference screenshots will be attached here from the tag-bound dev instance during PR verification.
Localization
No new user-facing strings were added. The Settings change reuses the existing localized English/Japanese keys for Vertical/Inline layout and Stack Branch and Directory; no localization catalog changes are required.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes drift between AppKit and SwiftUI sidebar rows by resolving sidebar settings once and sharing them across both renderers. Restores the legacy default (vertical branch layout with branch and directory stacked) and preserves existing saved values. Closes #8430
SidebarTabItemSettingsSnapshotfor both SwiftUI and AppKit rows.SidebarWorkspaceBranchDirectorySettingsto encode the compatibility rule: legacysidebar.branchVerticalLayout(Bool, defaulttrue) sets topology;sidebar.stackBranchDirectoryonly affects inline layout; vertical is always stacked.SidebarCatalogSection.stacksBranchAndDirectory(...)and disables the toggle when vertical.Written for commit 5bdac63. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes