Repository navigation
Rewrite built-in sidebar in AppKit - #8275
Closed
azooz2003-bit wants to merge 3 commits into
Closed
azooz2003-bit wants to merge 3 commits into
azooz2003-bit wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdds a feature-flagged native AppKit sidebar with projected workspace/group state, incremental snapshot updates, native cells and controls, context menus, drag/drop, checklist support, SwiftUI hosting, and scale-focused tests. The existing SwiftUI sidebar remains available as a legacy fallback. ChangesAppKit sidebar migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 2 warnings, 1 inconclusive)
✅ Passed checks (16 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 |
Contributor
Greptile SummaryThis PR introduces a native AppKit
Confidence Score: 4/5Safe to merge with one fix: the The implementation is architecturally sound. One real defect is the Sources/TerminalNotificationStore.swift — the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant SwiftUI as SwiftUI Host
participant Rep as Coordinator
participant Proj as SidebarAppKitProjectionSource
participant VC as SidebarAppKitViewController
participant Table as NSTableView
SwiftUI->>Rep: updateNSViewController
Rep->>Proj: updateExternalState(selectedIds)
Proj-->>Rep: onChange(.rows(itemIds))
Rep->>VC: reconfigure(itemIDs:)
VC->>Table: reloadData(forRowIndexes:)
Table->>Rep: workspaceSnapshot(workspaceId)
Rep->>Proj: workspaceSnapshot [O(1) cached]
Proj-->>Table: SidebarWorkspaceRowSnapshot
Note over Proj,Table: Visible-only observation
Proj->>Proj: observeVisibleWorkspace (per visible row)
Note over Rep,Proj: Notification batch update
Proj-->>Proj: receiveUnreadSummaryChanges
Proj->>Proj: updateGroupAggregate O(1) per change
Proj-->>Rep: onChange(.rows(changedIds))
Rep->>VC: reconfigure(itemIDs:)
%%{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"}}}%%
sequenceDiagram
participant SwiftUI as SwiftUI Host
participant Rep as Coordinator
participant Proj as SidebarAppKitProjectionSource
participant VC as SidebarAppKitViewController
participant Table as NSTableView
SwiftUI->>Rep: updateNSViewController
Rep->>Proj: updateExternalState(selectedIds)
Proj-->>Rep: onChange(.rows(itemIds))
Rep->>VC: reconfigure(itemIDs:)
VC->>Table: reloadData(forRowIndexes:)
Table->>Rep: workspaceSnapshot(workspaceId)
Rep->>Proj: workspaceSnapshot [O(1) cached]
Proj-->>Table: SidebarWorkspaceRowSnapshot
Note over Proj,Table: Visible-only observation
Proj->>Proj: observeVisibleWorkspace (per visible row)
Note over Rep,Proj: Notification batch update
Proj-->>Proj: receiveUnreadSummaryChanges
Proj->>Proj: updateGroupAggregate O(1) per change
Proj-->>Rep: onChange(.rows(changedIds))
Rep->>VC: reconfigure(itemIDs:)
Reviews (2): Last reviewed commit: "Fix AppKit sidebar startup" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+62
to
+76
| } | ||
|
|
||
| private(set) var resolverInvocationCounts = ResolverInvocationCounts() | ||
| #endif | ||
|
|
||
| let tableView = SidebarAppKitTableView(frame: .zero) | ||
| let scrollView = NSScrollView(frame: .zero) | ||
| private let clipView = SidebarAppKitClipView(frame: .zero) | ||
|
|
||
| private let headerContainer = NSView(frame: .zero) | ||
| private let contentContainer = NSView(frame: .zero) | ||
| private let footerContainer = NSView(frame: .zero) | ||
| private let trailingBorderView = SidebarAppKitPassiveBorderView(frame: .zero) | ||
| private var borderRefreshObserver: NSObjectProtocol? | ||
| private var installedHeaderView: NSView? |
Contributor
There was a problem hiding this comment.
#if DEBUG test seam in production source
resolverInvocationCounts and resetResolverInvocationCounts() are #if DEBUG-gated members with no production callers — every access site is inside a #if DEBUG block in cmuxTests/SidebarAppKitViewControllerScaleTests.swift and cmuxTests/SidebarLazyLayoutScaleTests.swift. The canonical fix for this codebase is to widen the property to internal (removing private(set)) so tests can reach it via @testable import without a compiled-out debug guard in shipping source.
Rule Used: Do not add new test/debug seams (ForTesting-styl... (source)
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!
Comment on lines
100566
to
100603
| } | ||
| }, | ||
| "featureFlags.appKitSidebar.description": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Uses the native AppKit implementation for the built-in workspace sidebar." | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "組み込みのワークスペースサイドバーにネイティブAppKit実装を使用します。" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "featureFlags.appKitSidebar.title": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "AppKit sidebar" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "AppKitサイドバー" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "featureFlags.sidebarWorkspaceAgentSpinner.description": { | ||
| "extractionState": "manual", |
Contributor
There was a problem hiding this comment.
featureFlags.appKitSidebar.title and featureFlags.appKitSidebar.description are newly introduced string keys that only have en and ja entries. The catalog already supports 20 locales (ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant in addition to en and ja), so any user running a non-English, non-Japanese locale will see these raw key identifiers instead of localized text in the feature-flag settings UI.
Rule Used: Flag production user-facing text that is not fully... (source)
Comment on lines
+510
to
+513
| for id in remoteTargetIds { | ||
| tabManager.tabs.first(where: { $0.id == id })? | ||
| .disconnectRemoteConnection(clearConfiguration: false) | ||
| } |
Contributor
There was a problem hiding this comment.
tabManager.tabs.first(where: { $0.id == id }) scans the full workspace list for each id in remoteTargetIds, while projectionSource.workspaceById[id] (used everywhere else in this file) is an O(1) dictionary lookup backed by the same live data. The difference is negligible for today's typical selection sizes, but it's inconsistent with the established pattern and would quietly degrade for large multi-remote selections.
Suggested change
| for id in remoteTargetIds { | |
| tabManager.tabs.first(where: { $0.id == id })? | |
| .disconnectRemoteConnection(clearConfiguration: false) | |
| } | |
| for id in remoteTargetIds { | |
| projectionSource.workspaceById[id]? | |
| .disconnectRemoteConnection(clearConfiguration: false) | |
| } |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 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 `@cmux.xcodeproj/project.pbxproj`:
- Around line 7344-7349: Remove the PBXSourcesBuildPhase entries for
SidebarAppKitSnapshotStoreTests.swift and
SidebarAppKitViewControllerScaleTests.swift from the cmux app target, while
preserving their existing entries in the cmuxTests target. Do not alter
production source entries or test-target wiring.
In `@cmuxTests/SidebarLazyLayoutScaleTests.swift`:
- Around line 539-544: Replace the systemUptime-based deadline in the polling
loop around workspaceRowInputProjections with a fixed maximum number of run-loop
turns. Continue polling the actual projection predicate, calling
Self.turnMainRunLoopOnce and yielding each iteration, and stop when the
predicate becomes true or the bounded iteration count is exhausted.
In `@Sources/FeatureFlags.swift`:
- Around line 142-153: Replace the positional allFlags[5] lookup in the
appKitSidebar accessor with a named static CmuxFeatureFlagDefinition, and reuse
that same definition in the allFlags collection alongside the other flags.
Preserve the existing key, localization, defaultWhenUnavailable value, and
accessor behavior while ensuring registry reordering cannot change this gate.
In `@Sources/Sidebar/AppKit/SidebarAppKitCellSupport.swift`:
- Around line 19-39: The bounded method in SidebarAppKitCellText currently
processes the entire input before applying limits. Replace the trimming, split,
joined, and final prefix flow with a reserved output buffer that traverses
characters incrementally, skips surrounding whitespace, emits at most
maximumLines non-empty lines and maximumCharacters characters, and stops once
either limit is reached while preserving nil and empty-result behavior.
In `@Sources/Sidebar/AppKit/SidebarAppKitContextMenuController.swift`:
- Around line 859-868: Update the localized title construction in the
hiddenCount branch to select sidebar.checklist.moreItems.one when hiddenCount
equals one and sidebar.checklist.moreItems.other otherwise, preserving the
formatted count. Add both corresponding singular and plural translations to the
localization catalog.
- Around line 115-124: Update the remove-custom-title menu action in
SidebarAppKitContextMenuController to call
interactionCoordinator.clearWorkspaceTitle instead of directly invoking
tabManager.clearCustomTitle. Preserve the existing workspaceId and menu behavior
while routing the operation through the shared coordinator action path.
In `@Sources/Sidebar/AppKit/SidebarAppKitDragCoordinator.swift`:
- Around line 181-184: Remove the test-facing sessionSnapshotBuildCount and
reorderResolverInvocationCount properties from
SidebarAppKitDragCoordinator.swift. Also remove the DEBUG-only projection
callback plus its initializer and invocation wiring from
SidebarAppKitRuntimeHostRepresentable.swift; production Sources/**/*.swift must
not expose these test/debug-only seams.
- Around line 168-171: Use the shared drag registry as the sole owner of native
workspace-drag origin metadata: move the originMetadata state from
SidebarAppKitDragCoordinator into that registry and update all coordinator
accessors to use it. In
Sources/Sidebar/AppKit/SidebarAppKitRuntimeHostRepresentable.swift lines
118-124, remove the private fallback registry and require or inject the same
authoritative shared instance instead.
In `@Sources/Sidebar/AppKit/SidebarAppKitHeaderView.swift`:
- Around line 426-437: Update applyNotificationBadge to set the notifications
button’s accessibility value from state.unreadNotificationCount, using the
localized .one key for exactly one unread notification and .other for all other
positive counts; clear the value when the count is zero before resetting the
badge.
In `@Sources/Sidebar/AppKit/SidebarAppKitInteractionCoordinator.swift`:
- Around line 177-180: Ensure every native-sidebar close entrypoint passes an
explicit CloseTabConfirmationTrigger: update closeWorkspaceFromMiddleClick to
use the middle-click trigger, and update all four batch-close actions in
SidebarAppKitContextMenuController to use a context-menu trigger when calling
the trigger-aware close API.
- Around line 303-319: Update openURL(_:fromWorkspace:prefersCmuxBrowser:) to
activate the target workspace without passing ambient NSEvent.modifierFlags,
using the focus-only activation behavior so sidebar link clicks cannot alter
workspace multi-selection. Preserve the existing browser-opening and NSWorkspace
fallback flow.
In `@Sources/Sidebar/AppKit/SidebarAppKitModifierMonitor.swift`:
- Around line 14-45: Update receive(_:) so flagsChanged events from other
windows do not return without updating state. Recompute Command using the
monitored window’s current key status for every event, clearing it when that
window is no longer key; preserve the existing event handling and
setCommandPressed flow.
In `@Sources/Sidebar/AppKit/SidebarAppKitRuntimeHostRepresentable.swift`:
- Around line 118-124: Update the lazy dragCoordinator initialization to stop
constructing a private SidebarWorkspaceDragRegistry when AppDelegate.shared is
unavailable. Inject the authoritative sidebarWorkspaceDragRegistry into
SidebarAppKitDragCoordinator, or fail closed by disabling workspace drag until
that registry exists; preserve a single shared registry for cross-window drag
identity.
In `@Sources/Sidebar/AppKit/SidebarAppKitShortcutMenuController.swift`:
- Around line 23-29: Update the menu item title construction in
SidebarAppKitShortcutMenuController to format the action label and shortcut
through a localized catalog key rather than hard-coded spacing and ordering. Add
matching translations for that key in every supported locale, preserving both
values in the resulting shortcut row.
In `@Sources/Sidebar/AppKit/SidebarAppKitViewController.swift`:
- Around line 56-65: Remove the DEBUG-only ResolverInvocationCounts type,
resolverInvocationCounts property, reset API, and related instrumentation from
SidebarAppKitViewController. Update tests to observe resolver invocations
through the injected resolver closures or an existing test harness instead,
leaving production Sources free of test-only counters and `#if` DEBUG members.
In `@Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift`:
- Around line 919-927: Centralize checklist progress formatting in a shared
localized formatter, providing separate progress-only and
progress-with-next-item formats. Update SidebarAppKitWorkspaceCellView.swift
lines 919-927 to use those localized formats, and update
SidebarAppKitChecklistController.swift lines 439-442 to reuse the same formatter
instead of raw punctuation; preserve the existing bounded next-item text
behavior.
- Around line 1179-1183: Both SidebarAppKitWorkspaceCellView.swift at lines
1179-1183 and SidebarAppKitGroupCellView.swift at lines 486-490 currently use a
single non-pluralized string key for all unread counts. Update both locations to
use explicit plural form handling by selecting the appropriate `.one` or
`.other` localized string key based on the unreadCount value (using a ternary or
conditional check where snapshot.unreadCount equals 1 selects the `.one` variant
and other counts select the `.other` variant). Create or reference a shared
formatter approach across both files so the same pluralization logic applies
consistently to workspace and group unread accessibility states.
In `@Sources/Sidebar/AppKit/SidebarAppKitWorkspaceDetailsView.swift`:
- Around line 345-359: Update the pull-request row title construction in the
visibility loop to use a localized interpolation key for the complete label,
number, separator, and status summary instead of assembling the string directly.
Also create the tooltip through a localized interpolation key, preserving its
existing meaning and clickability condition; use the existing localized Swift
APIs for both user-facing strings.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 2266-2272: The update path around summaryChanges and
summaryChangesSubject must stop recomputing a full dictionary diff on every
summary update. Change the authoritative summary builder to emit keyed mutations
together with the snapshot, then apply those mutations directly to
summaryByWorkspaceId and publish the resulting changes, preserving existing
add/update/remove semantics without scanning the complete previous and next
maps.
- Around line 2243-2254: Replace summaryChangesSubject and
summaryChangesPublisher in Sources/TerminalNotificationStore.swift:2243-2254
with a keyed async change sequence owned by SidebarUnreadModel, preserving the
existing main-actor emission behavior. In
Sources/TerminalNotificationStore.swift:3-3, remove the Combine import. Update
the unread-change observation in SidebarAppKitProjectionSource.swift:342-348 to
consume the async sequence through a stored task, remove the cancellable
property at Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift:72-72,
and remove its Combine import at
Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift:2-2.
🪄 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: b27bb2bb-54ea-42eb-b106-91f64701d3fa
📒 Files selected for processing (32)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/FeatureFlags.swiftSources/Sidebar/AppKit/SidebarAppKitBadgeView.swiftSources/Sidebar/AppKit/SidebarAppKitCellSupport.swiftSources/Sidebar/AppKit/SidebarAppKitChecklistController.swiftSources/Sidebar/AppKit/SidebarAppKitConfiguration.swiftSources/Sidebar/AppKit/SidebarAppKitContextMenuController.swiftSources/Sidebar/AppKit/SidebarAppKitDragCoordinator.swiftSources/Sidebar/AppKit/SidebarAppKitFooterView.swiftSources/Sidebar/AppKit/SidebarAppKitGroupCellView.swiftSources/Sidebar/AppKit/SidebarAppKitHeaderView.swiftSources/Sidebar/AppKit/SidebarAppKitHelpMenuController.swiftSources/Sidebar/AppKit/SidebarAppKitInteractionCoordinator.swiftSources/Sidebar/AppKit/SidebarAppKitModifierMonitor.swiftSources/Sidebar/AppKit/SidebarAppKitProjectionSource.swiftSources/Sidebar/AppKit/SidebarAppKitRuntimeHostRepresentable.swiftSources/Sidebar/AppKit/SidebarAppKitShortcutMenuController.swiftSources/Sidebar/AppKit/SidebarAppKitSnapshotStore.swiftSources/Sidebar/AppKit/SidebarAppKitTableView.swiftSources/Sidebar/AppKit/SidebarAppKitViewController.swiftSources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swiftSources/Sidebar/AppKit/SidebarAppKitWorkspaceDetailsView.swiftSources/Sidebar/AppKit/VerticalTabsSidebarAppKitRouter.swiftSources/SidebarWorkspaceSnapshotFactory.swiftSources/TerminalNotificationStore.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarAppKitSnapshotStoreTests.swiftcmuxTests/SidebarAppKitViewControllerScaleTests.swiftcmuxTests/SidebarLazyLayoutScaleTests.swiftcmuxTests/SidebarWorkspaceNotificationIndexTests.swift
Comment on lines
+7344
to
7349
| 5BA77100000000000000000F /* SidebarAppKitSnapshotStore.swift in Sources */, | ||
| 5BA771000000000000000010 /* SidebarAppKitTableView.swift in Sources */, | ||
| 5BA771000000000000000011 /* SidebarAppKitViewController.swift in Sources */, | ||
| 5BA771000000000000000012 /* SidebarAppKitWorkspaceCellView.swift in Sources */, | ||
| 5BA771000000000000000015 /* SidebarAppKitWorkspaceDetailsView.swift in Sources */, | ||
| D7AB34310000000000000003 /* SidebarBonsplitTabDropDelegate.swift in Sources */, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Remove test files from the main app target's compile sources.
SidebarAppKitSnapshotStoreTests.swift and SidebarAppKitViewControllerScaleTests.swift have been incorrectly added to the cmux app target's PBXSourcesBuildPhase. Test files must only be compiled in the test target (cmuxTests). Including them here links XCTest into the production app binary, which causes crashes on launch in release builds and App Store rejections.
They are already correctly wired into the cmuxTests target later in this file. As per coding guidelines, keep test logic out of the app target.
💚 Proposed fix to remove tests from the app target
D7AB34310000000000000003 /* SidebarBonsplitTabDropDelegate.swift in Sources */,
- 5BA773000000000000000001 /* SidebarAppKitSnapshotStoreTests.swift in Sources */,
D7AB34300000000000000003 /* SidebarBonsplitTabWorkspaceDropOverlay.swift in Sources */,
D7AB34310000000000000001 /* SidebarBonsplitWorkspaceRowDropModifier.swift in Sources */,
- 5BA773000000000000000002 /* SidebarAppKitViewControllerScaleTests.swift in Sources */,
EA1F00000000000000000003 /* SidebarDirectoryText.swift in Sources */,🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 7344 - 7349, Remove the
PBXSourcesBuildPhase entries for SidebarAppKitSnapshotStoreTests.swift and
SidebarAppKitViewControllerScaleTests.swift from the cmux app target, while
preserving their existing entries in the cmuxTests target. Do not alter
production source entries or test-target wiring.
Source: Coding guidelines
Comment on lines
+539
to
+544
| let refreshDeadline = ProcessInfo.processInfo.systemUptime + 3 | ||
| while harness.counter.workspaceRowInputProjections == 0, | ||
| ProcessInfo.processInfo.systemUptime < refreshDeadline { | ||
| Self.turnMainRunLoopOnce(layingOut: harness.window) | ||
| await Task.yield() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace the wall-clock deadline with a bounded predicate poll.
systemUptime makes completion depend on CI timing. Bound the number of run-loop turns while polling the actual projection predicate.
Proposed fix
- let refreshDeadline = ProcessInfo.processInfo.systemUptime + 3
- while harness.counter.workspaceRowInputProjections == 0,
- ProcessInfo.processInfo.systemUptime < refreshDeadline {
+ for _ in 0..<100 where harness.counter.workspaceRowInputProjections == 0 {
Self.turnMainRunLoopOnce(layingOut: harness.window)
await Task.yield()
}As per coding guidelines and path instructions, tests must not read wall-clock APIs and should use completion signals or deadline-bounded predicate polling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let refreshDeadline = ProcessInfo.processInfo.systemUptime + 3 | |
| while harness.counter.workspaceRowInputProjections == 0, | |
| ProcessInfo.processInfo.systemUptime < refreshDeadline { | |
| Self.turnMainRunLoopOnce(layingOut: harness.window) | |
| await Task.yield() | |
| } | |
| for _ in 0..<100 where harness.counter.workspaceRowInputProjections == 0 { | |
| Self.turnMainRunLoopOnce(layingOut: harness.window) | |
| await Task.yield() | |
| } |
🤖 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/SidebarLazyLayoutScaleTests.swift` around lines 539 - 544, Replace
the systemUptime-based deadline in the polling loop around
workspaceRowInputProjections with a fixed maximum number of run-loop turns.
Continue polling the actual projection predicate, calling
Self.turnMainRunLoopOnce and yielding each iteration, and stop when the
predicate becomes true or the bounded iteration count is exhausted.
Sources: Coding guidelines, Path instructions
Comment on lines
+142
to
+153
| CmuxFeatureFlagDefinition( | ||
| key: "appkit-sidebar-enabled-experiment", | ||
| title: String( | ||
| localized: "featureFlags.appKitSidebar.title", | ||
| defaultValue: "AppKit sidebar" | ||
| ), | ||
| flagDescription: String( | ||
| localized: "featureFlags.appKitSidebar.description", | ||
| defaultValue: "Uses the native AppKit implementation for the built-in workspace sidebar." | ||
| ), | ||
| defaultWhenUnavailable: CmuxFeatureFlags.appKitSidebarDefault | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a named flag definition instead of allFlags[5].
Line 178 silently changes this gate if the registry is reordered, potentially enabling AppKit under an unrelated rollout. Reuse a named static definition in both allFlags and the accessor.
As per coding guidelines, Swift architecture changes must preserve clear ownership and invariants.
Also applies to: 177-179
🤖 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/FeatureFlags.swift` around lines 142 - 153, Replace the positional
allFlags[5] lookup in the appKitSidebar accessor with a named static
CmuxFeatureFlagDefinition, and reuse that same definition in the allFlags
collection alongside the other flags. Preserve the existing key, localization,
defaultWhenUnavailable value, and accessor behavior while ensuring registry
reordering cannot change this gate.
Source: Coding guidelines
Comment on lines
+19
to
+39
| @MainActor | ||
| enum SidebarAppKitCellText { | ||
| static func bounded( | ||
| _ value: String?, | ||
| maximumCharacters: Int, | ||
| maximumLines: Int | ||
| ) -> String? { | ||
| guard let value else { return nil } | ||
| let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !trimmed.isEmpty else { return nil } | ||
|
|
||
| let lines = trimmed | ||
| .split(whereSeparator: { $0.isNewline }) | ||
| .prefix(max(1, maximumLines)) | ||
| .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } | ||
| .filter { !$0.isEmpty } | ||
| guard !lines.isEmpty else { return nil } | ||
| let flattened = lines.joined(separator: " ") | ||
| guard flattened.count > maximumCharacters else { return flattened } | ||
| return String(flattened.prefix(maximumCharacters)) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Apply text limits during traversal, not after full processing.
trimmingCharacters, split, and joined scan or allocate from the complete value before truncation. Build into a reserved buffer and stop at the character/line limits so long titles remain bounded in row-rendering paths.
As per coding guidelines, sidebar and list-row hot paths should avoid large intermediate strings and use bounded, preallocated builds.
🤖 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/Sidebar/AppKit/SidebarAppKitCellSupport.swift` around lines 19 - 39,
The bounded method in SidebarAppKitCellText currently processes the entire input
before applying limits. Replace the trimming, split, joined, and final prefix
flow with a reserved output buffer that traverses characters incrementally,
skips surrounding whitespace, emits at most maximumLines non-empty lines and
maximumCharacters characters, and stops once either limit is reached while
preserving nil and empty-result behavior.
Source: Coding guidelines
Comment on lines
+115
to
+124
| if workspace.hasCustomTitle { | ||
| addAction( | ||
| to: menu, | ||
| title: String( | ||
| localized: "contextMenu.removeCustomWorkspaceName", | ||
| defaultValue: "Remove Custom Workspace Name" | ||
| ) | ||
| ) { [weak self] in | ||
| self?.tabManager.clearCustomTitle(tabId: workspaceId) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the existing clear-title action path.
This menu directly mutates tabManager even though interactionCoordinator.clearWorkspaceTitle already owns the behavior.
Proposed fix
- self?.tabManager.clearCustomTitle(tabId: workspaceId)
+ self?.interactionCoordinator.clearWorkspaceTitle(workspaceId)As per coding guidelines, wire the same behavior through one shared action path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if workspace.hasCustomTitle { | |
| addAction( | |
| to: menu, | |
| title: String( | |
| localized: "contextMenu.removeCustomWorkspaceName", | |
| defaultValue: "Remove Custom Workspace Name" | |
| ) | |
| ) { [weak self] in | |
| self?.tabManager.clearCustomTitle(tabId: workspaceId) | |
| } | |
| if workspace.hasCustomTitle { | |
| addAction( | |
| to: menu, | |
| title: String( | |
| localized: "contextMenu.removeCustomWorkspaceName", | |
| defaultValue: "Remove Custom Workspace Name" | |
| ) | |
| ) { [weak self] in | |
| self?.interactionCoordinator.clearWorkspaceTitle(workspaceId) | |
| } |
🤖 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/Sidebar/AppKit/SidebarAppKitContextMenuController.swift` around lines
115 - 124, Update the remove-custom-title menu action in
SidebarAppKitContextMenuController to call
interactionCoordinator.clearWorkspaceTitle instead of directly invoking
tabManager.clearCustomTitle. Preserve the existing workspaceId and menu behavior
while routing the operation through the shared coordinator action path.
Source: Coding guidelines
Comment on lines
+919
to
+927
| let progress = "\(workspace.checklistCompletedCount)/\(workspace.checklistTotalCount)" | ||
| let firstUnchecked = SidebarAppKitCellText.bounded( | ||
| workspace.checklistFirstUncheckedText, | ||
| maximumCharacters: 1_024, | ||
| maximumLines: 1 | ||
| ) | ||
| checklistButton.title = [progress, firstUnchecked] | ||
| .compactMap { $0 } | ||
| .joined(separator: " · ") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Centralize localized checklist-progress formatting.
Both surfaces independently render the same progress concept with raw punctuation.
Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift#L919-L927: use localized progress-only and progress-with-next-item formats.Sources/Sidebar/AppKit/SidebarAppKitChecklistController.swift#L439-L442: reuse the same localized progress formatter.
As per coding guidelines, user-facing Swift text must use localized APIs.
📍 Affects 2 files
Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift#L919-L927(this comment)Sources/Sidebar/AppKit/SidebarAppKitChecklistController.swift#L439-L442
🤖 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/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift` around lines 919
- 927, Centralize checklist progress formatting in a shared localized formatter,
providing separate progress-only and progress-with-next-item formats. Update
SidebarAppKitWorkspaceCellView.swift lines 919-927 to use those localized
formats, and update SidebarAppKitChecklistController.swift lines 439-442 to
reuse the same formatter instead of raw punctuation; preserve the existing
bounded next-item text behavior.
Source: Coding guidelines
Comment on lines
+1179
to
+1183
| if snapshot.unreadCount > 0 { | ||
| setAccessibilityValue(String.localizedStringWithFormat( | ||
| String(localized: "workspaceGroup.unread.a11y", defaultValue: "%lld unread"), | ||
| Int64(snapshot.unreadCount) | ||
| )) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one shared pluralized unread accessibility formatter.
Both row types use workspaceGroup.unread.a11y for every count instead of the repository’s explicit singular/plural contract.
Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift#L1179-L1183: select the.oneor.otherworkspace unread key.Sources/Sidebar/AppKit/SidebarAppKitGroupCellView.swift#L486-L490: use the same shared formatter for group unread state.
Based on learnings, unread counts must use explicit .one and .other keys.
📍 Affects 2 files
Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift#L1179-L1183(this comment)Sources/Sidebar/AppKit/SidebarAppKitGroupCellView.swift#L486-L490
🤖 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/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift` around lines
1179 - 1183, Both SidebarAppKitWorkspaceCellView.swift at lines 1179-1183 and
SidebarAppKitGroupCellView.swift at lines 486-490 currently use a single
non-pluralized string key for all unread counts. Update both locations to use
explicit plural form handling by selecting the appropriate `.one` or `.other`
localized string key based on the unreadCount value (using a ternary or
conditional check where snapshot.unreadCount equals 1 selects the `.one` variant
and other counts select the `.other` variant). Create or reference a shared
formatter approach across both files so the same pluralization logic applies
consistently to workspace and group unread accessibility states.
Source: Learnings
Comment on lines
+345
to
+359
| if visibility.showsPullRequests { | ||
| for pullRequest in workspace.pullRequestRows { | ||
| let title = "\(pullRequest.label) #\(pullRequest.number) · \(Self.pullRequestStatusLabel(pullRequest.status))" | ||
| let url = pullRequest.url | ||
| result.append(Descriptor( | ||
| title: title, | ||
| symbolName: Self.pullRequestSymbol(pullRequest.status), | ||
| color: secondaryColor.withAlphaComponent(pullRequest.isStale ? 0.5 : 1), | ||
| font: emphasizedFont, | ||
| maximumLines: 1, | ||
| toolTip: snapshot.settings.makesPullRequestsClickable | ||
| ? String( | ||
| localized: "sidebar.pullRequest.openTooltip", | ||
| defaultValue: "Open \(pullRequest.label) #\(pullRequest.number)" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the pull-request summary as one format string.
label #number · status fixes word order and punctuation across locales. Build the title and tooltip through localized interpolation keys.
As per coding guidelines, user-facing Swift strings must use localized APIs.
🤖 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/Sidebar/AppKit/SidebarAppKitWorkspaceDetailsView.swift` around lines
345 - 359, Update the pull-request row title construction in the visibility loop
to use a localized interpolation key for the complete label, number, separator,
and status summary instead of assembling the string directly. Also create the
tooltip through a localized interpolation key, preserving its existing meaning
and clickability condition; use the existing localized Swift APIs for both
user-facing strings.
Source: Coding guidelines
Comment on lines
+2243
to
+2254
| /// Synchronously emitted on the main actor after `summaryByWorkspaceId` is | ||
| /// assigned. Existing `@Published` behavior is preserved for legacy users. | ||
| private let summaryChangesSubject = PassthroughSubject< | ||
| [SidebarWorkspaceUnreadSummaryChange], | ||
| Never | ||
| >() | ||
| var summaryChangesPublisher: AnyPublisher< | ||
| [SidebarWorkspaceUnreadSummaryChange], | ||
| Never | ||
| > { | ||
| summaryChangesSubject.eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use one async unread-change lifecycle instead of a new Combine channel.
The subject and subscriber introduce a parallel cancellation/observation mechanism beside the existing stored tasks.
Sources/TerminalNotificationStore.swift#L2243-L2254: expose keyed changes as an async sequence owned bySidebarUnreadModel.Sources/TerminalNotificationStore.swift#L3-L3: remove the Combine import after replacing the subject.Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift#L342-L348: consume the sequence in a stored task.Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift#L72-L72: remove the cancellable property.Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift#L2-L2: remove the Combine import.
As per coding guidelines, avoid introducing Combine publishers, subscribers, or cancellables for app state when async/await is available.
📍 Affects 2 files
Sources/TerminalNotificationStore.swift#L2243-L2254(this comment)Sources/TerminalNotificationStore.swift#L3-L3Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift#L342-L348Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift#L72-L72Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift#L2-L2
🤖 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/TerminalNotificationStore.swift` around lines 2243 - 2254, Replace
summaryChangesSubject and summaryChangesPublisher in
Sources/TerminalNotificationStore.swift:2243-2254 with a keyed async change
sequence owned by SidebarUnreadModel, preserving the existing main-actor
emission behavior. In Sources/TerminalNotificationStore.swift:3-3, remove the
Combine import. Update the unread-change observation in
SidebarAppKitProjectionSource.swift:342-348 to consume the async sequence
through a stored task, remove the cancellable property at
Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift:72-72, and remove its
Combine import at
Sources/Sidebar/AppKit/SidebarAppKitProjectionSource.swift:2-2.
Source: Coding guidelines
Comment on lines
+2266
to
+2272
| let summaryChanges = Self.summaryChanges( | ||
| from: summaryByWorkspaceId, | ||
| to: summaries | ||
| ) | ||
| if !summaryChanges.isEmpty { | ||
| summaryByWorkspaceId = summaries | ||
| summaryChangesSubject.send(summaryChanges) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Do not re-diff the complete unread map for every update.
This performs O(previous + next) dictionary scans; at 1,000 workspaces, one changed row still incurs roughly 2,000 visits. Have the authoritative summary builder emit keyed mutations with the snapshot, then apply those mutations directly here.
As per coding guidelines and path instructions, production paths over roughly 1,000 records must avoid repeated full-collection scans.
Also applies to: 2305-2328
🤖 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/TerminalNotificationStore.swift` around lines 2266 - 2272, The update
path around summaryChanges and summaryChangesSubject must stop recomputing a
full dictionary diff on every summary update. Change the authoritative summary
builder to emit keyed mutations together with the snapshot, then apply those
mutations directly to summaryByWorkspaceId and publish the resulting changes,
preserving existing add/update/remove semantics without scanning the complete
previous and next maps.
Sources: Coding guidelines, Path instructions
This was referenced Jul 16, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift (2)
919-927: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCentralize localized checklist-progress formatting.
Both surfaces independently render the same progress concept with raw punctuation.
As per coding guidelines, user-facing Swift text must use localized APIs.♻️ Proposed fix to use localized formats
- let progress = "\(workspace.checklistCompletedCount)/\(workspace.checklistTotalCount)" - let firstUnchecked = SidebarAppKitCellText.bounded( - workspace.checklistFirstUncheckedText, - maximumCharacters: 1_024, - maximumLines: 1 - ) - checklistButton.title = [progress, firstUnchecked] - .compactMap { $0 } - .joined(separator: " · ") + let firstUnchecked = SidebarAppKitCellText.bounded( + workspace.checklistFirstUncheckedText, + maximumCharacters: 1_024, + maximumLines: 1 + ) + if let firstUnchecked { + checklistButton.title = String( + localized: "sidebar.checklist.progressWithNext", + defaultValue: "\(workspace.checklistCompletedCount)/\(workspace.checklistTotalCount) · \(firstUnchecked)" + ) + } else { + checklistButton.title = String( + localized: "sidebar.checklist.progressOnly", + defaultValue: "\(workspace.checklistCompletedCount)/\(workspace.checklistTotalCount)" + ) + }🤖 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/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift` around lines 919 - 927, Update the checklist title formatting in the workspace cell view to use the shared localized checklist-progress formatting API instead of constructing the progress string with raw “/” and separator punctuation. Preserve the existing bounded first-unchecked text and compactly omit missing values, reusing the established localization symbol for the rendered user-facing text.Source: Coding guidelines
1179-1183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one shared pluralized unread accessibility formatter.
Both row types use
workspaceGroup.unread.a11yfor every count instead of the repository’s explicit singular/plural contract. Based on learnings, unread counts must use explicit.oneand.otherkeys.
Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift#L1179-L1183: branch onsnapshot.unreadCountto select the.oneor.otherworkspace unread key, or use a shared formatter.Sources/Sidebar/AppKit/SidebarAppKitGroupCellView.swift#L486-L490: apply the same pluralization logic or shared formatter for the group'sanchorUnreadCount.🤖 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/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift` around lines 1179 - 1183, The unread accessibility value currently uses one formatter for all counts. Update the workspace cell’s unread handling and the group cell’s anchorUnreadCount handling to share a formatter or explicitly select the workspaceGroup.unread.a11y.one and workspaceGroup.unread.a11y.other keys, preserving the existing zero-count behavior; apply the corresponding changes in Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift lines 1179-1183 and Sources/Sidebar/AppKit/SidebarAppKitGroupCellView.swift lines 486-490.Source: 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 `@Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift`:
- Around line 919-927: Update the checklist title formatting in the workspace
cell view to use the shared localized checklist-progress formatting API instead
of constructing the progress string with raw “/” and separator punctuation.
Preserve the existing bounded first-unchecked text and compactly omit missing
values, reusing the established localization symbol for the rendered user-facing
text.
- Around line 1179-1183: The unread accessibility value currently uses one
formatter for all counts. Update the workspace cell’s unread handling and the
group cell’s anchorUnreadCount handling to share a formatter or explicitly
select the workspaceGroup.unread.a11y.one and workspaceGroup.unread.a11y.other
keys, preserving the existing zero-count behavior; apply the corresponding
changes in Sources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swift lines
1179-1183 and Sources/Sidebar/AppKit/SidebarAppKitGroupCellView.swift lines
486-490.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24adeda1-b458-4603-9c34-4203b833e341
📒 Files selected for processing (8)
Sources/Sidebar/AppKit/SidebarAppKitChecklistController.swiftSources/Sidebar/AppKit/SidebarAppKitContextMenuController.swiftSources/Sidebar/AppKit/SidebarAppKitDragCoordinator.swiftSources/Sidebar/AppKit/SidebarAppKitGroupCellView.swiftSources/Sidebar/AppKit/SidebarAppKitProjectionSource.swiftSources/Sidebar/AppKit/SidebarAppKitWorkspaceCellView.swiftSources/Sidebar/AppKit/SidebarAppKitWorkspaceDetailsView.swiftSources/Sidebar/AppKit/VerticalTabsSidebarAppKitRouter.swift
💤 Files with no reviewable changes (1)
- Sources/Sidebar/AppKit/SidebarAppKitWorkspaceDetailsView.swift
This was referenced Jul 21, 2026
6 of 8 tasks
lawrencecchen
added
the
stale-revisit
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.
Replaces the built-in workspace sidebar with a retained native AppKit table behind
appkit-sidebar-enabled-experiment, default off. Custom and extension sidebar providers continue through the legacy SwiftUI host.The native path keeps only visible workspace observers, applies keyed workspace and group row updates, snapshots drag state once per session, and implements native groups, menus, remote actions, checklists, footer/update state, global font scaling, and accessibility actions. The AppKit projection omits checklist arrays until the checklist is opened.
The first commit adds the keyed unread projection regression contract. The second commit adds the implementation and 1,000-workspace scale contracts.
Verification was intentionally source-only at the requester’s direction. Passed: Swift parser lint,
git diff --check, feature-flag lint, localization coverage, project-file integrity, test-target wiring, and sidebar lazy-layout source contracts. No compile, build, reload, runtime test, or test execution was performed.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Rewrote the built-in workspace sidebar with a native AppKit implementation behind the
appkit-sidebar-enabled-experimentflag (off by default). Also fixed startup wiring so the AppKit sidebar initializes correctly when enabled.New Features
appkit-sidebar-enabled-experiment; legacy SwiftUI remains default and hosts custom/extension providers.NSTableView: visible-row observation, keyed row updates, stable ids.appkit-sidebar-enabled-experiment).Bug Fixes
VerticalTabsSidebarand injectingtabManager,sidebarUnread, andcmuxConfigStore.LegacyVerticalTabsSidebarto ensure provider hosting remains stable.Written for commit a59b88f. Summary will update on new commits.
Summary by CodeRabbit