Repository navigation
Flag-gated AppKit sidebar list (NSTableView rows + headers) - #8270
Conversation
sidebar-appkit-list-experiment (default OFF) switches the workspace sidebar between the existing SwiftUI LazyVStack list (default) and a new pure-AppKit NSTableView implementation: virtualized rows, measured-once height cache, per-row Combine churn pump, native cells for workspace rows and group headers (badges, spinners, media glyphs, branch/dir candidates, PR vector icons, ports, checklist, hint pills, drop indicators), NSMenu context menus with full SwiftUI parity, and table-owned hover/middle-click/drag. Fixes the LazyVStack scrollbar stutter and scroll-edge tap latency at 100+ workspaces by construction when the flag is enabled.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a feature-flagged AppKit ChangesAppKit sidebar workspace list
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 (8 errors, 2 warnings)
✅ Passed checks (15 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 |
…list popover style, header truncation, divider drag cost - Row selection via NSTableView target/action (cell gesture recognizers lose to the table's mouse tracking loop); double-click routes to inline rename. - Unread badge digit drawn directly with optical cap-height centering. - Checklist honors the popover style setting: never inline; summary opens an NSPopover with the same AppKit item lines. - Group header name field owns all remaining width; badge tracks measured text, so names no longer truncate early. - Divider resize: heights pinned to last stable width during drags with one trailing re-measure; per-workspace snapshot memo so container re-renders do dictionary lookups instead of full snapshot rebuilds.
A cancelled divider DragGesture never fires onEnded, stranding isResizerDragging; the 16ms cursor stabilizer then re-asserts the resize cursor indefinitely. The stabilizer now reconciles against the physical button state and force-releases the cursor when the button is up.
…lush Divider drags previously committed width to SwiftUI state and waited for the next runloop turn + vsync to lay out and present, so the divider trailed the cursor while the main thread sat idle (profiled). Each drag event now flushes layoutSubtreeIfNeeded + displayIfNeeded in the same mouse event, riding the terminal portal's immediate anchor-sync path during interactive resizes.
Per-event synchronous flushing did redundant layout+draw passes when mouse events outpaced the display (high-polling mice). The flush now runs at most once per refresh interval (screen maximumFramesPerSecond, min 60); events in between only update state and ride the next eligible flush.
…ard restore - Close/plus hover reveals fade with 120ms ease-out (hit-testing follows target state immediately so fading buttons never swallow clicks). - en/ja catalog entries for the sidebar-appkit-list feature flag strings. - Restore main's check-sidebar-lazy-layout.py + self-test + scale tests: the flag-off default path preserves the original LazyVStack contract byte-faithfully, so the original guard passes unchanged; the AppKit table path is additive.
Merges origin/main (a5a70ff) including the sidebar row-layer refactor (SidebarWorkspaceRowInput projections, container-owned snapshots and observation, selectWorkspaceRow) plus #8211 and the #8240 terminal-resize coalescing. The AppKit table now consumes the same projection as the SwiftUI list: workspaceTableRowConfiguration builds its row model from SidebarWorkspaceRowInput and the shared context-menu aggregates, container observation replaces the per-cell pump and snapshot memo, and the AppKit actions bundle is renamed SidebarAppKitRowActions to coexist with upstream's type. Verified rendering and interaction on both flag states.
The clicked workspace cell paints its selected background and title color the instant the press lands, before NSTableView's click tracking and the container round trip confirm the selection; the authoritative configure reconciles (or reverts) immediately after. Selection now reads as instantaneous instead of tracking the async model path.
With the flag on, the resizer handle hosts an NSView that runs NSSplitView's tracking technique: pull mouse events with nextEvent(matching:) in .eventTracking mode, apply the width per event, spin the runloop once so SwiftUI/CA commit, then present via displayIfNeeded — all inside the same mouse event. This removes the async runloop hop between HID drag events and on-screen width changes that made SwiftUI DragGesture resizes feel detached from the cursor. The view accepts first mouse and owns its cursor rect, matching native split-divider behavior. With the flag off, the original DragGesture path is unchanged. The per-event flush throttle (flushSidebarResizeToDisplay/MutableTimeBox) is removed; it ran after SwiftUI had already deferred the commit past the event, so it presented stale layout and had no user-visible effect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile SummaryThis PR rewrites the workspace sidebar list behind the
Confidence Score: 4/5Flag-off path is byte-for-byte identical to main; the AppKit path is safe to land behind the disabled-by-default flag while two open concerns from prior review passes are addressed. The implementation is well-structured and the feature flag gate fully protects the default user experience. Two concerns surfaced in earlier review passes remain open in the code: the Task.sleep debounce in scheduleWidthRemeasure uses a fixed wall-clock wait for layout-width settlement rather than a real signal, and the #if DEBUG test-observability seams in production Sources/ files have no production callers and should be reached via @testable import instead. Neither is a flag-off regression, but both remain unaddressed. Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift — Task.sleep debounce and #if DEBUG production seams; Sources/Sidebar/AppKitList/SidebarWorkspaceTableCellView.swift — additional #if DEBUG identity-exposure seams. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[VerticalTabsSidebar.workspaceScrollArea] --> B{isAppKitSidebarListEnabled?}
B -- "OFF (default)" --> C[legacyWorkspaceScrollArea\nSwiftUI LazyVStack + ScrollView]
B -- "ON" --> D[appKitWorkspaceScrollArea\nSidebarWorkspaceTableContainerView]
D --> E[SidebarWorkspaceTableController\nNSTableView delegate + datasource]
E --> F[apply rows]
F --> G[SidebarWorkspaceTableRowHeightCache\nmeasured-once heights]
F --> H{Row type}
H --> I[SidebarWorkspaceRowTableCellView\npure-AppKit workspace row]
H --> J[SidebarGroupHeaderTableCellView\npure-AppKit group header]
H --> K[SidebarWorkspaceTableCellView\nNSHostingView legacy row]
I --> L[installPump\nCombine per-cell observer]
A2[ContentView.sidebarResizerHandleOverlay] --> B2{isAppKitSidebarListEnabled?}
B2 -- "OFF" --> C2[SwiftUI DragGesture]
B2 -- "ON" --> D2[SidebarDividerTracker\nnextEvent synchronous loop]
%%{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[VerticalTabsSidebar.workspaceScrollArea] --> B{isAppKitSidebarListEnabled?}
B -- "OFF (default)" --> C[legacyWorkspaceScrollArea\nSwiftUI LazyVStack + ScrollView]
B -- "ON" --> D[appKitWorkspaceScrollArea\nSidebarWorkspaceTableContainerView]
D --> E[SidebarWorkspaceTableController\nNSTableView delegate + datasource]
E --> F[apply rows]
F --> G[SidebarWorkspaceTableRowHeightCache\nmeasured-once heights]
F --> H{Row type}
H --> I[SidebarWorkspaceRowTableCellView\npure-AppKit workspace row]
H --> J[SidebarGroupHeaderTableCellView\npure-AppKit group header]
H --> K[SidebarWorkspaceTableCellView\nNSHostingView legacy row]
I --> L[installPump\nCombine per-cell observer]
A2[ContentView.sidebarResizerHandleOverlay] --> B2{isAppKitSidebarListEnabled?}
B2 -- "OFF" --> C2[SwiftUI DragGesture]
B2 -- "ON" --> D2[SidebarDividerTracker\nnextEvent synchronous loop]
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
| private func scheduleWidthRemeasure() { | ||
| widthRemeasureTask?.cancel() | ||
| widthRemeasureTask = Task { [weak self] in | ||
| try? await Task.sleep(nanoseconds: 120_000_000) | ||
| guard let self, !Task.isCancelled else { return } | ||
| self.widthRemeasureTask = nil | ||
| let width = self.currentColumnWidth() | ||
| guard width > 0 else { return } | ||
| let changed = self.rowHeightCache.prepareHostedRows(self.rows, columnWidth: width) | ||
| self.lastMeasuredWidth = width | ||
| self.pumpHeightOverrides.removeAll(keepingCapacity: true) | ||
| if !changed.isEmpty { | ||
| self.containerView?.tableView.noteHeightOfRows(withIndexesChanged: changed) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Task.sleep used as a debounce timer in the render path
scheduleWidthRemeasure uses Task.sleep(nanoseconds: 120_000_000) as a 120ms wall-clock debounce before re-measuring row heights after a sidebar resize. The cmux blocking-runtime rule explicitly flags Task.sleep in production Swift as a timing-based synchronization primitive. While the task is cancellation-aware (try? + .isCancelled guard), the 120ms delay is a fixed wall-clock wait — if the resize settle signal fires sooner or later the height update is still wrong for those frames. A DispatchSourceTimer on the main queue (cancel + reschedule on each resize event) or observing the NSScrollView delegate's scrollViewDidEndLiveScroll / column-width-settled notification would provide a real signal without a sleep.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| #if DEBUG | ||
| var reconfigurationProbe: (() -> Void)? | ||
| var dropTargetComputationProbe: (() -> Void)? { | ||
| get { dropTargetGeometry.computationProbe } | ||
| set { dropTargetGeometry.computationProbe = newValue } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
New
#if DEBUG test-observability seams in production source
reconfigurationProbe and dropTargetComputationProbe are #if DEBUG-guarded vars with no production caller — they exist solely so SidebarLazyLayoutScaleTests can observe cell reconfiguration events. Per the no-test-debug-seam-in-production-source rule these should not live in a production Sources/ file. The same contract check can be satisfied by widening reconfigurationProbe to internal and reading it from the test target via @testable import, which is the canonical fix pattern (PR #6452). The dropTargetComputationProbe chain similarly belongs in test infrastructure, not the shipped controller.
Rule Used: Do not add new test/debug seams (ForTesting-styl... (source)
| DispatchQueue.main.async { | ||
| alertWindow.makeFirstResponder(input) | ||
| input.selectText(nil) | ||
| } |
There was a problem hiding this comment.
Legacy
DispatchQueue.main.async inside @MainActor-isolated function
promptRename (and promptCustomColor at line 204) use DispatchQueue.main.async for first-responder setup inside an @MainActor-isolated struct — a legacy Dispatch pattern. Note: because alert.runModal() spins a nested AppKit event loop, a bare Task { @MainActor in ... } might not process until after the modal closes. The safer fix is RunLoop.main.perform { ... } (fires inside any active run loop including the modal's) or confirming that initialFirstResponder alone is sufficient and dropping the deferred dispatch.
Rule Used: Flag new legacy async patterns in cmux-owned Swift... (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!
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 `@Sources/ContentView.swift`:
- Line 12999: Remove the redundant second call that ends the same signpost in
the surrounding profiling flow: keep the new defer-based
SidebarProfilingSignposts.end(signpost) and delete the later explicit end at the
corresponding cleanup point.
- Around line 1414-1425: The cancellation branch in the resizer gesture
lifecycle must use the same teardown path as normal completion. Remove the 16 ms
mouse-state polling and its direct state cleanup, add an explicit cancellation
callback, and route it through the existing resize-session end logic so
TerminalWindowPortalRegistry.endInteractiveGeometryResize(owner:) is invoked and
final geometry synchronization occurs.
- Around line 10782-10802: Update the .groupHeader branch in the tableRows
construction to retrieve the group from the existing
renderContext.workspaceGroupById index using groupId, instead of calling
renderContext.workspaceGroups.first(where:). Preserve the existing nil handling
and sidebarWorkspaceGroupTableConfiguration arguments.
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swift`:
- Around line 91-99: Update showOptimisticSelectionHighlight() to synchronously
update the authoritative selection model instead of directly painting
speculative background and text colors. Then invoke the normal model-driven
rendering/configuration path so selection UI has a single source of truth and
automatically preserves rollback behavior when selection fails.
- Around line 844-855: Update the visible port layout logic around visiblePorts,
portX, and lineHeight to keep each port within the available leading-to-trailing
width by wrapping controls onto additional lines when needed. Advance y and
include every wrapped line’s height and spacing in the returned row height,
while preserving frame placement for visible controls.
- Around line 420-426: Update the pluralized tooltip handling near agentCount to
use the explicit sidebar.agentActivity.tooltip.other localization key instead of
sidebar.agentActivity.tooltip.many, and rename the corresponding entry in the
string catalog for every supported locale while preserving the existing .one key
and formatting behavior.
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCommands.swift`:
- Around line 164-169: Remove the DispatchQueue.main.async focus-repair block in
the alert setup within SidebarWorkspaceRowCommands, including the
makeFirstResponder and selectText calls. Keep alertWindow.initialFirstResponder
= input as the sole initial focus configuration, and apply the same cleanup to
the corresponding second alert setup.
- Around line 253-259: Update the workspace-move logic in the
orderedWorkspaceIds handling so the final workspace is excluded from the
non-focused loop and moved exactly once with focus enabled. Preserve the
existing behavior for preceding workspaces and avoid invoking
app.moveWorkspaceToWindow twice for the finalWorkspaceId.
- Around line 295-315: The menu-building flow should not return early when
notificationStore is unavailable. Update the guard around tabManager and
notificationStore to require only tabManager, then conditionally call
addNotificationItems and its separator when notificationStore exists, while
preserving all unrelated actions such as rename, move, close, and copy.
- Around line 697-721: Filter the bulk mutations in addNotificationItems so each
selected workspace is changed only when its current notification state requires
it: mark read only unread workspaces and mark unread only read workspaces. Reuse
the notificationStore state/query APIs, preserving the existing menu enablement
and iteration behavior while preventing redundant markUnread calls from setting
a manual unread flag.
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowModel.swift`:
- Around line 61-86: Remove the duplicate SidebarRowSnapshotCache ownership from
SidebarWorkspaceRowModel.swift, or transfer sole ownership to the table
controller with explicit invalidation tied to model/lifecycle events. In
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowModel.swift lines 61-86,
eliminate the redundant mutable snapshot cache or centralize its ownership; in
Sources/ContentView.swift line 10803, remove render-time pruning and update
cache state only through model/lifecycle handling.
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift`:
- Around line 84-101: Update the closed-status rendering in the
SidebarWorkspaceRowSlotViews drawing block to draw the prepared tinted image
rather than the original image, and use source-over compositing so it remains
visible on the transparent view. Preserve the existing icon sizing and centered
rect while applying the intended tint through the image’s mask or palette.
In `@Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSupportViews.swift`:
- Around line 690-744: The SidebarRowChecklistPopoverController’s item lines
capture stale model and item state, so mutations leave the popover outdated and
repeated toggles submit the same transition. Update loadView and the
onToggle/onRemove handlers to refresh from the authoritative row snapshot
through the existing SidebarAppKitRowActions path, or dismiss the popover
immediately after each mutation; avoid retaining duplicated item state in the
closures.
In `@Sources/Sidebar/AppKitList/SidebarWorkspaceTableCellView.swift`:
- Around line 12-20: Remove the test/debug-only accessors around
representedRowId, reconfigurationProbe, hostingViewIdentity, and
hostedRootIdentity in
Sources/Sidebar/AppKitList/SidebarWorkspaceTableCellView.swift; remove the
corresponding forwarding probes in
Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift:24-30 and the
computation callback in
Sources/Sidebar/AppKitList/SidebarWorkspaceTableDropTargetGeometryGate.swift:14-16.
Update cmuxTests/SidebarWorkspaceTableTests.swift:150-243 to verify observable
behavior or use test-target instrumentation instead of these production seams.
In `@Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift`:
- Around line 353-369: Replace the delayed Task.sleep-based implementation in
scheduleWidthRemeasure with a shared immediate remeasurement action, and have
the divider/live-resize owner invoke that action when tracking ends. Preserve
the existing width validation, cache preparation, override clearing, and table
row-height notification behavior, while removing the fixed-delay task
coordination.
- Around line 119-128: Eliminate full-list work from targeted row updates: in
Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift lines 119-128,
pass changed IDs/indexes and perform structural scans only when structural
changes occur; in
Sources/Sidebar/AppKitList/SidebarWorkspaceTableRowHeightCache.swift lines
61-84, update affected cache entries in place rather than rebuilding the
complete dictionary. Preserve existing content-change and height-measurement
behavior.
- Around line 241-251: Update both the drag path in
tableView(_:pasteboardWriterForRow:) and the middle-click close path to reject
group-header and fallback rows before accessing their workspaceId or performing
any action. Validate that the selected row represents a real workspace, while
preserving existing bounds and behavior for valid workspace rows.
In `@Sources/Sidebar/SidebarDividerTrackingView.swift`:
- Around line 48-73: Replace the blocking event loop in
SidebarDividerTrackingView.mouseDown(with:) with non-blocking mouseDragged,
mouseUp, and cancellation handling, or an equivalent gesture recognizer
coordinated on the MainActor. Preserve the existing onBegan, onChanged, onEnded,
cursor cleanup, and horizontal delta behavior without using nextEvent, RunLoop
spinning, or manual window presentation.
In `@Sources/VerticalTabsSidebar`+WorkspaceGroups.swift:
- Around line 95-191: Extract the command closures currently assembled in the
SidebarGroupHeaderRowActions initializer into a shared group-header action
owner, including focus, creation, rename, notification, grouping, deletion, and
navigation actions. Update both SwiftUI and AppKit renderers, including
sidebarWorkspaceGroupRow, to consume that shared owner instead of wiring
equivalent behavior separately, preserving the live membership and
notification-state checks used by onMarkAllRead and onMarkAllUnread.
- Line 90: Update the isFirstRow calculation in the group-header rendering flow
to derive the first-row state from the visible render order rather than
sidebarReorderIds, which is empty outside active drag operations. Ensure the
first visible group is marked first during normal rendering while preserving the
existing reorder behavior when a drag is active.
🪄 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: fcc15abb-ad4c-4567-b94b-9d6896d4eb17
📒 Files selected for processing (34)
Resources/Localizable.xcstringsSources/App/WorkspaceRuntimeSettings.swiftSources/ContentView.swiftSources/Debug/SidebarLazyContractProbe.swiftSources/FeatureFlags.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowModel.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCommands.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowModel.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSupportViews.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableActions.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableCellModel.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableCellRootView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableCellState.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableCellView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableClipView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableContainerView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableController.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableDropTargetGeometryGate.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableEmptyDropIndicatorView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableEnvironmentSnapshot.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableHoverResolver.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableRowConfiguration.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableRowHeightCache.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableRowHeightCalculator.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableView.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableViewImpl.swiftSources/Sidebar/SidebarDividerTrackingView.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarWorkspaceTableTests.swift
💤 Files with no reviewable changes (1)
- Sources/App/WorkspaceRuntimeSettings.swift
| /// Paints the selected-row background instantly on press; the next | ||
| /// authoritative configure() repaints the full selected treatment (or | ||
| /// reverts if the selection did not land). | ||
| func showOptimisticSelectionHighlight() { | ||
| guard let model, !model.isActive else { return } | ||
| let palette = SidebarRowPalette(model: model) | ||
| backgroundView.layer?.backgroundColor = palette.selectedBackground.cgColor | ||
| titleView.textColor = palette.selectedForeground(1.0) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Render selection only from the authoritative selection model.
This directly paints a speculative selected state outside model.isActive and has no local rollback if selection fails to trigger reconfiguration. Update the authoritative selection synchronously, then render through the normal model path.
As per path instructions, correctness-critical selection UI must use one reliable source of truth rather than a best-effort fallback.
🤖 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/AppKitList/Cells/SidebarWorkspaceRowCellView.swift` around
lines 91 - 99, Update showOptimisticSelectionHighlight() to synchronously update
the authoritative selection model instead of directly painting speculative
background and text colors. Then invoke the normal model-driven
rendering/configuration path so selection UI has a single source of truth and
automatically preserves rollback behavior when selection fails.
Sources: Coding guidelines, Path instructions
| let agentCount = model.snapshot.activeCodingAgentCount | ||
| let tooltip = agentCount == 1 | ||
| ? String(localized: "sidebar.agentActivity.tooltip.one", defaultValue: "1 agent running") | ||
| : String.localizedStringWithFormat( | ||
| String(localized: "sidebar.agentActivity.tooltip.many", defaultValue: "%lld agents running"), | ||
| agentCount | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the .other plural localization key.
Replace sidebar.agentActivity.tooltip.many with an .other key and update every supported locale in the string catalog.
Based on learnings, pluralized strings in this repository use explicit .one and .other keys.
Proposed key change
- String(localized: "sidebar.agentActivity.tooltip.many", defaultValue: "%lld agents running"),
+ String(localized: "sidebar.agentActivity.tooltip.other", defaultValue: "%lld agents running"),📝 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.
| let agentCount = model.snapshot.activeCodingAgentCount | |
| let tooltip = agentCount == 1 | |
| ? String(localized: "sidebar.agentActivity.tooltip.one", defaultValue: "1 agent running") | |
| : String.localizedStringWithFormat( | |
| String(localized: "sidebar.agentActivity.tooltip.many", defaultValue: "%lld agents running"), | |
| agentCount | |
| ) | |
| let agentCount = model.snapshot.activeCodingAgentCount | |
| let tooltip = agentCount == 1 | |
| ? String(localized: "sidebar.agentActivity.tooltip.one", defaultValue: "1 agent running") | |
| : String.localizedStringWithFormat( | |
| String(localized: "sidebar.agentActivity.tooltip.other", defaultValue: "%lld agents running"), | |
| agentCount | |
| ) |
🤖 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/AppKitList/Cells/SidebarWorkspaceRowCellView.swift` around
lines 420 - 426, Update the pluralized tooltip handling near agentCount to use
the explicit sidebar.agentActivity.tooltip.other localization key instead of
sidebar.agentActivity.tooltip.many, and rename the corresponding entry in the
string catalog for every supported locale while preserving the existing .one key
and formatting behavior.
Sources: Coding guidelines, Learnings
| func tableView(_ tableView: NSTableView, pasteboardWriterForRow row: Int) -> (any NSPasteboardWriting)? { | ||
| guard rows.indices.contains(row), let actions else { return nil } | ||
| let workspaceId = rows[row].workspaceId | ||
| actions.beginWorkspaceDrag(workspaceId) | ||
| workspaceDragSessionDidBegin() | ||
| let item = NSPasteboardItem() | ||
| item.setString( | ||
| "\(SidebarTabDragPayload.prefix)\(workspaceId.uuidString)", | ||
| forType: NSPasteboard.PasteboardType(SidebarTabDragPayload.typeIdentifier) | ||
| ) | ||
| return item |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-workspace rows before dragging or closing.
Both paths accept group headers and fallback rows, so middle-clicking a header can close its associated workspace and dragging it can start an unintended workspace reorder.
Proposed fix
func tableView(_ tableView: NSTableView, pasteboardWriterForRow row: Int)
-> (any NSPasteboardWriting)? {
- guard rows.indices.contains(row), let actions else { return nil }
+ guard rows.indices.contains(row),
+ rows[row].appKitWorkspaceRowModel != nil,
+ let actions else { return nil }
let workspaceId = rows[row].workspaceId func middleClick(row: Int) {
- guard rows.indices.contains(row) else { return }
+ guard rows.indices.contains(row),
+ rows[row].appKitWorkspaceRowModel != nil else { return }
actions?.closeWorkspace(rows[row].workspaceId)
}Also applies to: 285-288
🤖 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/AppKitList/SidebarWorkspaceTableController.swift` around
lines 241 - 251, Update both the drag path in
tableView(_:pasteboardWriterForRow:) and the middle-click close path to reject
group-header and fallback rows before accessing their workspaceId or performing
any action. Validate that the selected row represents a real workspace, while
preserving existing bounds and behavior for valid workspace rows.
| /// One trailing re-measure ~120ms after the sidebar width stops moving; | ||
| /// per-pixel divider drags otherwise re-measure every row every frame. | ||
| private func scheduleWidthRemeasure() { | ||
| widthRemeasureTask?.cancel() | ||
| widthRemeasureTask = Task { [weak self] in | ||
| try? await Task.sleep(nanoseconds: 120_000_000) | ||
| guard let self, !Task.isCancelled else { return } | ||
| self.widthRemeasureTask = nil | ||
| let width = self.currentColumnWidth() | ||
| guard width > 0 else { return } | ||
| let changed = self.rowHeightCache.prepareHostedRows(self.rows, columnWidth: width) | ||
| self.lastMeasuredWidth = width | ||
| self.pumpHeightOverrides.removeAll(keepingCapacity: true) | ||
| if !changed.isEmpty { | ||
| self.containerView?.tableView.noteHeightOfRows(withIndexesChanged: changed) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Replace the 120 ms timer with an explicit resize-completion signal.
The fixed delay makes row-height correctness timing-dependent. Have the divider/live-resize owner invoke one shared remeasurement action when tracking ends.
As per coding guidelines, production Swift must not use Task.sleep for delayed coordination or readiness synchronization.
🤖 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/AppKitList/SidebarWorkspaceTableController.swift` around
lines 353 - 369, Replace the delayed Task.sleep-based implementation in
scheduleWidthRemeasure with a shared immediate remeasurement action, and have
the divider/live-resize owner invoke that action when tracking ends. Preserve
the existing width validation, cache preparation, override clearing, and table
row-height notification behavior, while removing the fixed-delay task
coordination.
Source: Coding guidelines
| override func mouseDown(with event: NSEvent) { | ||
| guard let window else { return } | ||
| onBegan?() | ||
| let startX = event.locationInWindow.x | ||
| NSCursor.resizeLeftRight.push() | ||
| defer { | ||
| NSCursor.pop() | ||
| onEnded?() | ||
| } | ||
| while true { | ||
| guard let next = window.nextEvent( | ||
| matching: [.leftMouseDragged, .leftMouseUp], | ||
| until: .distantFuture, | ||
| inMode: .eventTracking, | ||
| dequeue: true | ||
| ) else { continue } | ||
| if next.type == .leftMouseUp { | ||
| break | ||
| } | ||
| onChanged?(next.locationInWindow.x - startX) | ||
| // Drain the SwiftUI/CA commit scheduled by the width write, then | ||
| // present — all inside this event, like NSSplitView's own loop. | ||
| RunLoop.current.run(mode: .eventTracking, before: Date()) | ||
| window.contentView?.layoutSubtreeIfNeeded() | ||
| window.displayIfNeeded() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not block the MainActor in a custom mouse-event loop.
nextEvent(... until: .distantFuture) owns the UI thread until mouse-up while manually spinning and presenting the window. Use normal mouseDragged/mouseUp/cancellation callbacks or a gesture recognizer under one MainActor coordinator instead.
As per coding guidelines, do not introduce blocking repair paths for lifecycle or rendering races.
🤖 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/SidebarDividerTrackingView.swift` around lines 48 - 73,
Replace the blocking event loop in SidebarDividerTrackingView.mouseDown(with:)
with non-blocking mouseDragged, mouseUp, and cancellation handling, or an
equivalent gesture recognizer coordinated on the MainActor. Preserve the
existing onBegan, onChanged, onEnded, cursor cleanup, and horizontal delta
behavior without using nextEvent, RunLoop spinning, or manual window
presentation.
Source: Coding guidelines
| globalFontMagnificationPercent: renderContext.environment.globalFontMagnificationPercent, | ||
| cwdContextMenuItems: cwdContextMenuItems, | ||
| rowSpacing: tabRowSpacing, | ||
| isFirstRow: renderContext.sidebarReorderIds.first == group.anchorWorkspaceId, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Derive first-row state from visible render order.
sidebarReorderIds is empty whenever no drag is active, so the first group header is never marked first during normal rendering.
Proposed fix
- isFirstRow: renderContext.sidebarReorderIds.first == group.anchorWorkspaceId,
+ isFirstRow: renderContext.visibleWorkspaceRowIds.first == group.anchorWorkspaceId,📝 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.
| isFirstRow: renderContext.sidebarReorderIds.first == group.anchorWorkspaceId, | |
| isFirstRow: renderContext.visibleWorkspaceRowIds.first == group.anchorWorkspaceId, |
🤖 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/VerticalTabsSidebar`+WorkspaceGroups.swift at line 90, Update the
isFirstRow calculation in the group-header rendering flow to derive the
first-row state from the visible render order rather than sidebarReorderIds,
which is empty outside active drag operations. Ensure the first visible group is
marked first during normal rendering while preserving the existing reorder
behavior when a drag is active.
| let actions = SidebarGroupHeaderRowActions( | ||
| onToggleCollapsed: { [weak tabManager, groupId = group.id] in | ||
| tabManager?.toggleWorkspaceGroupCollapsed(groupId: groupId) | ||
| }, | ||
| onFocusAnchor: { [weak tabManager, anchorId = group.anchorWorkspaceId, selectedTabIds = $selectedTabIds, lastSidebarSelectionIndex = $lastSidebarSelectionIndex] in | ||
| guard let tabManager else { return } | ||
| guard let anchorTab = tabManager.tabs.first(where: { $0.id == anchorId }) else { return } | ||
| tabManager.selectWorkspace(anchorTab) | ||
| if selectedTabIds.wrappedValue != [anchorId] { | ||
| selectedTabIds.wrappedValue = [anchorId] | ||
| } | ||
| if let anchorIndex = tabManager.tabs.firstIndex(where: { $0.id == anchorId }) { | ||
| lastSidebarSelectionIndex.wrappedValue = anchorIndex | ||
| } | ||
| }, | ||
| onTapPlus: { [weak tabManager, groupId = group.id, placement = newWorkspacePlacement] in | ||
| guard let tabManager else { return } | ||
| let resolved = placement | ||
| ?? UserDefaultsSettingsClient(defaults: .standard).value(for: SettingCatalog().workspaceGroups.newWorkspacePlacement) | ||
| _ = tabManager.createWorkspaceInGroup(groupId: groupId, placement: resolved) | ||
| }, | ||
| onRunResolvedItem: { [weak tabManager, groupId = group.id] item in | ||
| guard let tabManager else { return } | ||
| SidebarWorkspaceGroupContextMenuRunner.run( | ||
| item: item, | ||
| tabManager: tabManager, | ||
| groupId: groupId | ||
| ) | ||
| }, | ||
| onRename: { [weak tabManager, groupId = group.id, currentName = group.name] in | ||
| guard let tabManager else { return } | ||
| presentSidebarWorkspaceGroupRenamePrompt( | ||
| tabManager: tabManager, | ||
| groupId: groupId, | ||
| currentName: currentName | ||
| ) | ||
| }, | ||
| onTogglePinned: { [weak tabManager, groupId = group.id] in | ||
| tabManager?.toggleWorkspaceGroupPinned(groupId: groupId) | ||
| }, | ||
| onMarkRead: { [weak notificationStore, anchorId = group.anchorWorkspaceId] in | ||
| notificationStore?.markRead(forTabId: anchorId) | ||
| }, | ||
| onMarkUnread: { [weak notificationStore, anchorId = group.anchorWorkspaceId] in | ||
| notificationStore?.markUnread(forTabId: anchorId) | ||
| }, | ||
| onClearLatestNotifications: { [weak notificationStore, anchorId = group.anchorWorkspaceId] in | ||
| notificationStore?.clearLatestNotification(forTabId: anchorId) | ||
| }, | ||
| onMarkAllRead: { [weak tabManager, weak notificationStore, groupId = group.id, anchorId = group.anchorWorkspaceId] in | ||
| guard let tabManager, let notificationStore else { return } | ||
| // Resolve members live at action time: closures are excluded | ||
| // from model equality, so a captured ID list could go stale | ||
| // across a same-count membership swap. | ||
| let ids = tabManager.tabs.compactMap { $0.groupId == groupId && $0.id != anchorId ? $0.id : nil } | ||
| // Only touch members that are actually unread, so we never run | ||
| // notification teardown on already-read workspaces. | ||
| for id in ids where notificationStore.canMarkWorkspaceRead(forTabIds: [id]) { | ||
| notificationStore.markRead(forTabId: id) | ||
| } | ||
| }, | ||
| onMarkAllUnread: { [weak tabManager, weak notificationStore, groupId = group.id, anchorId = group.anchorWorkspaceId] in | ||
| guard let tabManager, let notificationStore else { return } | ||
| let ids = tabManager.tabs.compactMap { $0.groupId == groupId && $0.id != anchorId ? $0.id : nil } | ||
| // Only mark members that are not already unread. Calling | ||
| // markUnread on an already-unread member would set its manual | ||
| // unread flag, which a later notification dismissal cannot | ||
| // clear, leaving the workspace stuck unread. | ||
| for id in ids where notificationStore.canMarkWorkspaceUnread(forTabIds: [id]) { | ||
| notificationStore.markUnread(forTabId: id) | ||
| } | ||
| }, | ||
| onUngroup: { [weak tabManager, groupId = group.id] in | ||
| tabManager?.ungroupWorkspaceGroup(groupId: groupId) | ||
| }, | ||
| onDelete: { [weak tabManager, groupId = group.id] in | ||
| guard let tabManager, | ||
| let confirmation = tabManager.workspaceGrouping.deletionConfirmation( | ||
| groupId: groupId, | ||
| fallbackGroupName: group.name, | ||
| fallbackAnchorWorkspaceId: group.anchorWorkspaceId | ||
| ) else { return } | ||
| if confirmation.containedWorkspaceCount > 0 { | ||
| guard confirmDeleteWorkspaceGroup( | ||
| groupName: confirmation.groupName, | ||
| memberCount: confirmation.containedWorkspaceCount | ||
| ) else { return } | ||
| } | ||
| tabManager.workspaceGrouping.deleteWorkspaceGroup(confirmed: confirmation) | ||
| }, | ||
| onEditConfig: { | ||
| SidebarWorkspaceGroupConfigOpener.openCmuxConfigInEditor() | ||
| }, | ||
| onOpenDocs: { | ||
| SidebarWorkspaceGroupConfigOpener.openWorkspaceGroupsDocs() | ||
| } | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use one group-header action path for SwiftUI and AppKit.
This repeats the existing sidebarWorkspaceGroupRow command wiring and has already diverged—for example, the SwiftUI mark-read actions revalidate state while these do not. Extract a shared command/action owner and adapt both renderers to it.
As per coding guidelines, do not wire the same behavior separately through multiple surfaces.
🤖 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/VerticalTabsSidebar`+WorkspaceGroups.swift around lines 95 - 191,
Extract the command closures currently assembled in the
SidebarGroupHeaderRowActions initializer into a shared group-header action
owner, including focus, creation, rename, notification, grouping, deletion, and
navigation actions. Update both SwiftUI and AppKit renderers, including
sidebarWorkspaceGroupRow, to consume that shared owner instead of wiring
equivalent behavior separately, preserving the live membership and
notification-state checks used by onMarkAllRead and onMarkAllUnread.
Source: Coding guidelines
…ar rows (flag ON) Time Profiler attribution of a real drag (68ms per width commit, ~11 updates/s): 57% was the portal's per-anchor failsafe fan-out (every anchor callback re-synced all hosted views and re-reconciled every visible surface), 14% the sidebar row-projection prelude, 5% a DEBUG log helper doing a synchronous pasteboard XPC per pane layout. With the AppKit sidebar flag on, the anchor callback now syncs only its own hosted view and coalesces the failsafe fan-out to one deferred pass per main-queue turn (still within the same drag tick — the tracking loop spins the runloop per event). Flag off keeps the per-callback fan-out. Row building freezes against the last-built rows while the portal registry reports an interactive resize; rows cannot change while the resizer owns the mouse. The drag-layout debug log now gates on app-owned drag state before touching NSPasteboard. The tracking loop gains per-phase timing (write/commit/layout/display/flush) and a hit-test routing diagnostic, and its runloop pass uses a real 1ms deadline so SwiftUI/CA commits run inside the event. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CodeRabbit/Greptile round: end the registry's interactive-resize session from the cursor failsafe (portals stayed latched in drag mode); O(1) group lookup in table row building; remove double signpost end; wrap port controls at the row's trailing edge; drop the deferred modal-focus repair in rename/color prompts (initialFirstResponder suffices); menu no longer disappears without a notification store (only its notification section does); per-workspace filters on bulk mark read/unread (legacy parity); group headers rejected for middle-click close and row drags (they carry the anchor's workspaceId); closed-PR icon tint drawn inside the image (.sourceAtop against transparent backing drew nothing); agent tooltip defaultValues aligned with the canonical translations. Settings title for the flag is now 'Lawrence Sidebar' (en/ja; key unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # Resources/Localizable.xcstrings # Sources/FeatureFlags.swift
NSImage's escaping drawing handler referenced the view's color property without explicit capture semantics, which is a hard compile error; main merged #8270 with PR CI off, so tagged app builds from current main fail. Capture the color by value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift from main (#8270) fails to compile under Xcode 26.6, which marks the NSImage(size:flipped:) drawing closure so property access requires explicit self. Main-side fix needed too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rewrites the workspace sidebar list in AppKit behind the feature flag
sidebar-appkit-list-experiment(default OFF; PostHog-overridable, local override via Internal Flags).Flag OFF (default): the existing SwiftUI LazyVStack sidebar and DragGesture resizer, unchanged.
Flag ON: pure-AppKit NSTableView list. Virtualized rows with a measured-once height cache (fixes the scrollbar stutter from LazyVStack height re-estimation and the seconds-long tap latency after flinging a 100-workspace sidebar). Native cells render every row slot without NSHostingView: title wrap + inline rename, unread badges, GPU spinner, media glyphs, close button, description/notification preview, remote SSH line + reconnect, metadata entries + markdown blocks, log line, progress bar, three branch/directory layouts with width-adaptive candidates, custom-vector PR icons, ports, checklist section (respecting the popover setting), shortcut hint pills, drop indicators. Context menus are NSMenu with item-for-item SwiftUI parity (groups, status lanes, colors with swatches, move-to-window, notifications submenu). Hover/middle-click/drag are table-owned; rows rebuild from the container-observed
SidebarWorkspaceRowInputprojections so workspace churn repaints single cells without container re-renders.Flag ON also replaces the sidebar resizer's SwiftUI DragGesture with a native tracking view (
SidebarDividerTrackingView) using NSSplitView's technique: a synchronousnextEvent(matching:)loop in.eventTrackingmode that applies the width, spins the runloop once so SwiftUI/CA commit, and presents viadisplayIfNeededinside the same mouse event. It accepts first mouse and owns its cursor rect. Selection paints optimistically on mouse-down and reconciles from the authoritative configure pass.Verification (all on tag
sbak, LG HDR 4K):sidebar.selectdebug-log timestamp: flag ON 30/32/38 ms (source=appKitRow), flag OFF 25/27/27 ms. Click latency is at parity; the AppKit win is scroll/render cost (no LazyVStack height re-estimation livelock), not click dispatch.scripts/check-sidebar-lazy-layout.pyandSidebarLazyLayoutScaleTestsremain main's originals and pass (legacy path untouched).Resources/Localizable.xcstrings; no other user-visible strings added (AppKit cells reuse existing localized keys, verified item-for-item against the SwiftUI menus).Branch is merged with current main (post-#8211/#8240 row-projection and resize-coalescing refactors adopted).
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a feature-flagged AppKit sidebar list that replaces the SwiftUI list with an
NSTableViewfor smoother scrolling and lower tap latency with many workspaces. With the flag on, divider drags use a native tracker that sticks to the cursor and now coalesce portal updates to cut drag cost; flag-off behavior is unchanged. Off by default; enablesidebar-appkit-list-experimentin Internal Flags.New Features
NSTableViewsidebar behindsidebar-appkit-list-experiment(off by default). Virtualized rows with measured-once height caching replaceLazyVStackre-measure churn.NSMenu; table-owned hover/middle-click/drag. Selection via table target/action; double-click to inline rename.SidebarWorkspaceRowInput): container-observed projections replace the prior per-row Combine pump and snapshot memo; AppKit row actions renamed to avoid type conflicts.Bug Fixes
Written for commit fab9097. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes