Repository navigation
Add a flag-gated AppKit NSTableView core for the workspace sidebar - #8281
Closed
azooz2003-bit wants to merge 5 commits into
Closed
azooz2003-bit wants to merge 5 commits into
azooz2003-bit wants to merge 5 commits into
Conversation
added 5 commits
July 15, 2026 21:49
The workspace list becomes one container-level NSViewRepresentable mounting NSTableView with measured-once row heights (usesAutomaticRowHeights=false + explicit height cache), an explicit differ (ID-order change → reloadData; content change → reconfigure visible cells only), AppKit-owned hover via table tracking areas, and table-geometry drop targets. Cells host the existing SwiftUI row content (TabItemView / SidebarWorkspaceGroupHeaderView) through one reused NSHostingView each, fed by the same immutable snapshots as before; the snapshot boundary from #8211 is unchanged. Deletes the SwiftUI list plumbing this replaces: scroll-follow proxy state, the drag-gated row-frame preference reader, the per-row drop modifiers and drag gate, both drop-overlay representables in the default list path, and the sidebar pointer-interaction monitor (hover and middle-click are native table events now). scripts/check-sidebar-lazy-layout.py now guards the AppKit boundary: the old lazy-layout functions must stay deleted, row types must not mount platform representables, and table mutations are banned from AppKit layout callbacks. SidebarLazyLayoutScaleTests keeps the 300-workspace realization/convergence gates against the table.
# Conflicts: # cmux.xcodeproj/project.pbxproj
sidebar.beta.appKitList.enabled (Settings → Beta Features → AppKit Sidebar List, also settable in cmux.json) selects the list core: off (default) keeps the existing SwiftUI LazyVStack path unchanged — scroll-follow proxy, pointer-interaction monitor, drag-gated frame reader, drop overlays, and row drag mounts all restored verbatim; on mounts the NSTableView path. workspaceScrollArea branches on the flag and hosts the shared workspace-observation/snapshot-refresh stack. Table-hosted cells set a sidebarPlatformListHosted environment flag so TabItemView keeps its SwiftUI drag/drop mounts in the legacy list but skips them inside table cells, where NSTableView owns the drag session and drop-target geometry.
…ence Two defects found by at-scale runtime verification (128 workspaces): 1. The representable's default sizing falls back to the container's fitting size, which derives from the table's full content height. At 128 workspaces the ideal-size pass adopted ~5,200pt and the window grew to fit every row. sizeThatFits now reports exactly the proposal, with unspecified dimensions reporting zero, so content-derived metrics never escape the viewport. 2. Row-configuration equivalence was an Equatable TabItemView / SidebarWorkspaceGroupHeaderView built per row per body pass, which also built each row's ~45-closure actions bundle just to be thrown away on the next pass — destroying the previous pass's bundles dominated idle main-thread time. Equivalence now compares the immutable row/group snapshots directly (the same values the views diff on), and SidebarWorkspaceGroupRowSnapshot gains a synthesized Equatable.
📝 WalkthroughWalkthroughAdds a beta-gated AppKit ChangesAppKit Sidebar List
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 2 warnings)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 10550-10600: Move the shared handlers for checklist popover
dismissal, workspace CWD revision, multi-selection hiding, and multi-selection
collapse out of the beta-specific branch and attach them once to the common
workspaceScrollArea flow. Remove the duplicate legacy handlers, while keeping
only the scrollProxy request branch-specific. Preserve the existing state
updates and notification guards in the shared handlers.
In `@Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift`:
- Around line 289-309: Replace repeated full-sidebar scans in reconfigureRows
and positionAppKitDropIndicator with ID-to-index maps maintained by apply.
Rebuild the maps whenever rows are applied or reordered, then resolve hovered,
dragged, and drop-indicator IDs through those indexes while preserving existing
visible-row reconfiguration and indicator behavior.
In `@Sources/Sidebar/AppKitList/SidebarWorkspaceTableRowHeightCache.swift`:
- Around line 31-36: Update the height-cache preparation and cache-hit paths
around prepareHostedRows, the shared prepare method, and the prototype
measurement state so cached entries retain only reusable height data, not
previous row configurations, makeContent closures, full list snapshots, or the
last measured row. Ensure each measurement uses the current rows and releases
temporary row-content/prototype references after preparation.
In `@Sources/VerticalTabsSidebar`+WorkspaceGroups.swift:
- Around line 112-168: The group header still enables a SwiftUI drag path while
hosted by the AppKit table, causing duplicate drag handling. In
sidebarWorkspaceGroupHeader, conditionally apply
.onDrag(...).internalOnlyTabDrag() only when sidebarPlatformListHosted is false,
matching the existing workspace-row behavior and leaving NSTableView as the sole
drag owner when hosted.
🪄 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: 4cd4deb1-554a-497d-9565-889f1b2f8c26
📒 Files selected for processing (29)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BetaFeaturesCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BetaFeaturesSection.swiftResources/Localizable.xcstringsSources/ContentView.swiftSources/Debug/SidebarLazyContractProbe.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/SidebarWorkspaceGroupHeaderView.swiftSources/SidebarWorkspaceGroupRowSnapshot.swiftSources/SidebarWorkspaceRowActions.swiftSources/SidebarWorkspaceRowInput.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarWorkspaceTableTests.swift
Comment on lines
+10550
to
+10600
| .onChange(of: tabManager.selectedTabId) { _, _ in | ||
| // Workspace switches produce no outside click for .transient auto-dismiss; close popovers explicitly. | ||
| if let dismissed = checklistPopoverWorkspaceId { checklistAddFieldActivationTokens[dismissed] = nil } | ||
| checklistPopoverWorkspaceId = nil | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: .workspaceCurrentDirectoryDidChange)) { _ in | ||
| // Drive a revision counter that the group-header resolver | ||
| // reads. Forces SwiftUI to re-invoke `cmuxConfigStore.resolveWorkspaceGroupConfig(forCwd:)` | ||
| // when the anchor's cwd changes while the anchor is not | ||
| // the selected workspace — otherwise group color/icon/menu | ||
| // and `+` placement reflect the previous cwd until some | ||
| // unrelated sidebar event fires. | ||
| anchorCwdRevision &+= 1 | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: SidebarMultiSelectionDidHideEvent.notificationName)) { notification in | ||
| // Group collapse hides some workspaces without changing | ||
| // focus or wiping the rest of the multi-selection. Strip | ||
| // only the hidden ids; if focus moved, make sure the new | ||
| // focused id is still represented. | ||
| guard let model = notification.object as? SidebarMultiSelectionModel, | ||
| model === tabManager.sidebarMultiSelection, | ||
| let event = SidebarMultiSelectionDidHideEvent(notification) else { return } | ||
| var next = selectedTabIds.subtracting(event.hiddenWorkspaceIds) | ||
| if let movedFocus = event.focusedWorkspaceId { | ||
| next.insert(movedFocus) | ||
| if let index = tabManager.tabs.firstIndex(where: { $0.id == movedFocus }) { | ||
| lastSidebarSelectionIndex = index | ||
| } | ||
| } | ||
| if next != selectedTabIds { | ||
| selectedTabIds = next | ||
| } | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: SidebarMultiSelectionShouldCollapseEvent.notificationName)) { notification in | ||
| // Keyboard nav (selectNextTab/selectPreviousTab) posts | ||
| // this so any stale Shift-click range in the sidebar's | ||
| // SwiftUI selectedTabIds collapses to just the newly- | ||
| // focused workspace. Without this, batch context-menu / | ||
| // shortcut actions would still target the stale range. | ||
| guard let model = notification.object as? SidebarMultiSelectionModel, | ||
| model === tabManager.sidebarMultiSelection, | ||
| let event = SidebarMultiSelectionShouldCollapseEvent(notification) else { return } | ||
| let focusedId = event.focusedWorkspaceId | ||
| let next: Set<UUID> = tabManager.tabs.contains(where: { $0.id == focusedId }) ? [focusedId] : [] | ||
| if selectedTabIds != next { | ||
| selectedTabIds = next | ||
| } | ||
| if let index = tabManager.tabs.firstIndex(where: { $0.id == focusedId }) { | ||
| lastSidebarSelectionIndex = index | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move shared sidebar lifecycle handling above the beta branch.
These popover, CWD-revision, and multi-selection handlers duplicate the legacy branch at Lines 10663-10726. Attach the common behavior once in workspaceScrollArea; keep only the scrollProxy request branch-local to prevent the two paths drifting.
As per coding guidelines, “Do not wire the same behavior separately through multiple surfaces; use one shared action path.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/ContentView.swift` around lines 10550 - 10600, Move the shared
handlers for checklist popover dismissal, workspace CWD revision,
multi-selection hiding, and multi-selection collapse out of the beta-specific
branch and attach them once to the common workspaceScrollArea flow. Remove the
duplicate legacy handlers, while keeping only the scrollProxy request
branch-specific. Preserve the existing state updates and notification guards in
the shared handlers.
Source: Coding guidelines
Comment on lines
+289
to
+309
| private func setHoveredRowId(_ next: SidebarWorkspaceRenderItemID?) { | ||
| guard hoveredRowId != next else { return } | ||
| let previous = hoveredRowId | ||
| hoveredRowId = next | ||
| reconfigureRows(withIds: [previous, next].compactMap { $0 }) | ||
| } | ||
|
|
||
| private func contextMenuDidOpen(rowId: SidebarWorkspaceRenderItemID) { | ||
| contextMenuRowId = rowId | ||
| } | ||
|
|
||
| private func contextMenuDidClose(rowId: SidebarWorkspaceRenderItemID) { | ||
| guard contextMenuRowId == rowId else { return } | ||
| contextMenuRowId = nil | ||
| recomputeHoveredRow() | ||
| } | ||
|
|
||
| private func reconfigureRows(withIds ids: [SidebarWorkspaceRenderItemID]) { | ||
| let idSet = Set(ids) | ||
| let indexes = IndexSet(rows.indices.filter { idSet.contains(rows[$0].id) }) | ||
| reconfigureVisibleRows(indexes) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Index rows instead of rescanning the full sidebar on hover and drag updates.
reconfigureRows performs an O(n) scan for each hover transition, while positionAppKitDropIndicator repeats another O(n) lookup during active-drag viewport updates. Maintain ID-to-index maps rebuilt in apply so these hot paths are O(1).
As per path instructions, production code over scalable user data must avoid repeated hot-path full-collection scans and use indexes or single-pass plans.
Also applies to: 453-457
🤖 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 289 - 309, Replace repeated full-sidebar scans in reconfigureRows and
positionAppKitDropIndicator with ID-to-index maps maintained by apply. Rebuild
the maps whenever rows are applied or reordered, then resolve hovered, dragged,
and drop-indicator IDs through those indexes while preserving existing
visible-row reconfiguration and indicator behavior.
Source: Path instructions
Comment on lines
+31
to
+36
| func prepareHostedRows( | ||
| _ rows: [SidebarWorkspaceTableRowConfiguration], | ||
| columnWidth: CGFloat | ||
| ) -> IndexSet { | ||
| return prepare(rows: rows, columnWidth: columnWidth, measure: measureHostedRow) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not retain stale row-content closures in the height cache.
A cache hit stores previous, preserving its old makeContent closure and full list snapshot. The prototype also retains its last measured row. Across staggered invalidations this can keep multiple sidebar generations and removed workspaces alive.
Proposed fix
func prepareHostedRows(
_ rows: [SidebarWorkspaceTableRowConfiguration],
columnWidth: CGFloat
) -> IndexSet {
+ defer { prototypeView.rootView = AnyView(EmptyView()) }
return prepare(rows: rows, columnWidth: columnWidth, measure: measureHostedRow)
}
if let previous, previous.matches(row: row, columnWidth: columnWidth) {
- nextEntries[row.id] = previous
+ nextEntries[row.id] = Entry(
+ row: row,
+ columnWidth: columnWidth,
+ height: previous.height
+ )
continue
}Also applies to: 64-68, 111-118
🤖 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/SidebarWorkspaceTableRowHeightCache.swift` around
lines 31 - 36, Update the height-cache preparation and cache-hit paths around
prepareHostedRows, the shared prepare method, and the prototype measurement
state so cached entries retain only reusable height data, not previous row
configurations, makeContent closures, full list snapshots, or the last measured
row. Ensure each measurement uses the current rows and releases temporary
row-content/prototype references after preparation.
Comment on lines
+112
to
168
| /// Builds one group-header table row configuration. Hover is AppKit-owned | ||
| /// (table tracking areas); the content factory overlays it on the | ||
| /// immutable snapshot, and context-menu open/close freezes hover in the | ||
| /// table controller. | ||
| func sidebarWorkspaceGroupTableConfiguration( | ||
| snapshot: SidebarWorkspaceGroupRowSnapshot, | ||
| renderContext: WorkspaceListRenderContext | ||
| ) -> SidebarWorkspaceTableRowConfiguration { | ||
| let makeHeader: (Bool, SidebarWorkspaceTableContextMenuActions) -> SidebarWorkspaceGroupHeaderView = { isPointerHovering, contextMenuActions in | ||
| var headerSnapshot = snapshot | ||
| headerSnapshot.isPointerHovering = isPointerHovering | ||
| return sidebarWorkspaceGroupHeader( | ||
| snapshot: headerSnapshot, | ||
| onContextMenuAppear: contextMenuActions.didOpen, | ||
| onContextMenuDisappear: contextMenuActions.didClose | ||
| ) | ||
| } | ||
| // Equivalence compares the immutable group snapshot directly (see the | ||
| // workspace-row configuration for why views are not built per pass). | ||
| var equivalenceSnapshot = snapshot | ||
| equivalenceSnapshot.isPointerHovering = false | ||
| let equivalenceValue = equivalenceSnapshot | ||
| return SidebarWorkspaceTableRowConfiguration( | ||
| id: .group(snapshot.groupId), | ||
| workspaceId: snapshot.anchorWorkspaceId, | ||
| groupId: snapshot.groupId, | ||
| isGroupHeader: true, | ||
| isPinned: snapshot.isPinned, | ||
| environment: renderContext.environment, | ||
| equivalenceValue: equivalenceValue | ||
| ) { isPointerHovering, contextMenuActions in | ||
| AnyView( | ||
| renderContext.environment.apply( | ||
| to: makeHeader(isPointerHovering, contextMenuActions) | ||
| .equatable() | ||
| .id(snapshot.anchorWorkspaceId) | ||
| .accessibilityIdentifier("sidebarWorkspaceGroup.\(snapshot.groupId.uuidString)") | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /// Assembles one group header from immutable values when the table's | ||
| /// content factory asks for it. Model references appear only inside | ||
| /// user-invoked action closures; row realization performs no observable | ||
| /// reads or mutations. | ||
| func sidebarWorkspaceGroupHeader( | ||
| snapshot: SidebarWorkspaceGroupRowSnapshot, | ||
| onContextMenuAppear: @escaping () -> Void, | ||
| onContextMenuDisappear: @escaping () -> Void | ||
| ) -> SidebarWorkspaceGroupHeaderView { | ||
| let onDragStart: () -> NSItemProvider = { [anchorId = snapshot.anchorWorkspaceId] in | ||
| #if DEBUG | ||
| cmuxDebugLog("sidebar.onDrag groupAnchor=\(anchorId.uuidString.prefix(5))") | ||
| #endif | ||
| dragState.beginDragging(tabId: anchorId) | ||
| return SidebarTabDragPayload(tabId: anchorId).provider() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Disable SwiftUI dragging for AppKit-hosted group headers.
The table factory hosts SidebarWorkspaceGroupHeaderView, which still unconditionally applies .onDrag(...).internalOnlyTabDrag() at Lines 301-302. NSTableView simultaneously owns the native drag session, so dragging a group anchor can invoke two drag paths and mutate dragState twice.
Gate the group header’s SwiftUI drag modifier with sidebarPlatformListHosted, as workspace rows already do.
As per coding guidelines, “Do not wire the same behavior separately through multiple surfaces; use one shared action path.”
🤖 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 112 - 168,
The group header still enables a SwiftUI drag path while hosted by the AppKit
table, causing duplicate drag handling. In sidebarWorkspaceGroupHeader,
conditionally apply .onDrag(...).internalOnlyTabDrag() only when
sidebarPlatformListHosted is false, matching the existing workspace-row behavior
and leaving NSTableView as the sole drag owner when hosted.
Source: Coding guidelines
Contributor
Greptile SummaryThis PR introduces a flag-gated AppKit
Confidence Score: 4/5Safe to merge behind the flag (default off); the existing SwiftUI path is untouched. The new AppKit path has one structural issue that should be resolved before the flag is turned on by default. The implementation is architecturally sound — the height cache, explicit differ, and drop-target gating all work correctly, and the verification numbers in the PR description are backed by concrete profiling. The one issue is that Sources/Sidebar/AppKitList/SidebarWorkspaceTableCellView.swift and Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift need the test-only #if DEBUG members removed or replaced with @testable import access. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant SUI as VerticalTabsSidebar (SwiftUI)
participant NSVRep as SidebarWorkspaceTableView (NSViewRepresentable)
participant Ctrl as SidebarWorkspaceTableController
participant Cache as SidebarWorkspaceTableRowHeightCache
participant Table as NSTableView (SidebarWorkspaceTableViewImpl)
participant Cell as SidebarWorkspaceTableCellView
SUI->>NSVRep: updateNSView(rows, actions, ...)
NSVRep->>Ctrl: apply(rows, actions, workspaceIds, selection)
Ctrl->>Cache: prepareHostedRows(nextRows, columnWidth)
Cache-->>Ctrl: IndexSet(changedHeights)
alt structural change (ID order differs)
Ctrl->>Table: reloadData()
Table->>Ctrl: numberOfRows(in:)
Table->>Ctrl: "tableView(_:heightOfRow:) -> cache lookup"
Table->>Ctrl: tableView(_:viewFor:row:)
Ctrl->>Cell: configure(row, isPointerHovering, ...)
else content-only change
Ctrl->>Cell: configure(changed visible cells)
Ctrl->>Table: noteHeightOfRows(withIndexesChanged:)
end
Table->>Ctrl: pointerDidLeaveTable() / mouseMoved
Ctrl->>Ctrl: recomputeHoveredRow()
Ctrl->>Cell: configure(affected cells, isPointerHovering: true/false)
Table->>Ctrl: pasteboardWriterForRow (drag start)
Ctrl->>SUI: actions.beginWorkspaceDrag(workspaceId)
Ctrl->>Ctrl: dropTargetGeometry.setWorkspaceDragSessionActive(true)
Note over Ctrl: Reorder/bonsplit rects computed lazily while drag is active
%%{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 SUI as VerticalTabsSidebar (SwiftUI)
participant NSVRep as SidebarWorkspaceTableView (NSViewRepresentable)
participant Ctrl as SidebarWorkspaceTableController
participant Cache as SidebarWorkspaceTableRowHeightCache
participant Table as NSTableView (SidebarWorkspaceTableViewImpl)
participant Cell as SidebarWorkspaceTableCellView
SUI->>NSVRep: updateNSView(rows, actions, ...)
NSVRep->>Ctrl: apply(rows, actions, workspaceIds, selection)
Ctrl->>Cache: prepareHostedRows(nextRows, columnWidth)
Cache-->>Ctrl: IndexSet(changedHeights)
alt structural change (ID order differs)
Ctrl->>Table: reloadData()
Table->>Ctrl: numberOfRows(in:)
Table->>Ctrl: "tableView(_:heightOfRow:) -> cache lookup"
Table->>Ctrl: tableView(_:viewFor:row:)
Ctrl->>Cell: configure(row, isPointerHovering, ...)
else content-only change
Ctrl->>Cell: configure(changed visible cells)
Ctrl->>Table: noteHeightOfRows(withIndexesChanged:)
end
Table->>Ctrl: pointerDidLeaveTable() / mouseMoved
Ctrl->>Ctrl: recomputeHoveredRow()
Ctrl->>Cell: configure(affected cells, isPointerHovering: true/false)
Table->>Ctrl: pasteboardWriterForRow (drag start)
Ctrl->>SUI: actions.beginWorkspaceDrag(workspaceId)
Ctrl->>Ctrl: dropTargetGeometry.setWorkspaceDragSessionActive(true)
Note over Ctrl: Reorder/bonsplit rects computed lazily while drag is active
Reviews (1): Last reviewed commit: "Clamp table sizing to the proposal; comp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+12
to
+16
| #if DEBUG | ||
| var reconfigurationProbe: (() -> Void)? | ||
| var hostingViewIdentity: ObjectIdentifier { ObjectIdentifier(hostingView) } | ||
| var hostedRootIdentity: UUID { hostingView.rootView.identity } | ||
| #endif |
Contributor
There was a problem hiding this comment.
#if DEBUG seams in production source
hostingViewIdentity and hostedRootIdentity are accessed exclusively in cmuxTests/SidebarWorkspaceTableTests.swift (the cellReusePreservesOneHostingViewAndStableRootIdentity test) and have zero production callers. Similarly, dropTargetComputationProbe in SidebarWorkspaceTableController (and the backing computationProbe in SidebarWorkspaceTableDropTargetGeometryGate) is only set from the #if DEBUG-guarded test suite — its #if DEBUG computationProbe?() invocation in refreshIfActive is only meaningful when a test has wired it up.
Per the no-test-debug-seam rule, the canonical fix is to widen private let hostingView to internal and read it directly from the test target via @testable import, removing the two computed shims. For the drop-target computation count, @testable import lets tests observe the controller's own rows.count or side-effects without a probe property. The reconfigurationProbe closures that feed SidebarLazyContractProbe.tableRootViewReconfigure fall in the "genuine debug facility" exception since they connect to an established contract-testing channel, but the introspection-only accessors do not.
Rule Used: Flag Swift files under a production Sources path (... (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!
This was referenced Jul 17, 2026
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.
Problem
#6707 is not fixed at scale. On current main (with #8211 and #8236), 128 workspaces with mixed-height rows: after a selection/scroll sweep ends, the main thread never converges — CPU stays pinned at ~100% indefinitely while idle, with 86% of main-thread samples inside
NSHostingView.beginTransaction → GraphHost.flushTransactions → AG::Subgraph::updateand the hot cmux frames inVerticalTabsSidebar.workspaceRows/workspaceRow(LazyVStack realization re-running forever). That is the exact field signature from the issue, reproduced on a symbolicated Debug build. The scroller-knob stutter also remains: the sidebar's document height changed 61 times in one sweep (44pt oscillation band), because LazyVStack re-estimates unrealized row heights from the realized average on every scroll. This class has now regressed through six distinct mechanisms (#2586, #5764, #5845, #6210, #6556, #8004); each fix removed one feedback edge and the next one found another.Full capture artifacts (samples, probe logs) are in cmuxterm-hq
out/sidebar-post8211-verify/.What changed
sidebar.beta.appKitList.enabled(Settings → Beta Features → "AppKit Sidebar List", also in cmux.json) selects the workspace list core. Default off: the existing SwiftUI LazyVStack path is byte-for-byte unchanged. With the flag on,workspaceScrollAreamounts an NSTableView-based list instead:NSViewRepresentable(SidebarWorkspaceTableView) owns anNSScrollView+NSTableViewwithusesAutomaticRowHeights = false. Row heights are measured once per (row content, column width, font scale) into an explicit cache and returned fromheightOfRow, so the document height is deterministic — zero doc-height churn while scrolling, by construction.reloadData; content changes → reconfigure only the affected visible cells (value-equality via the same Equatable row snapshots the SwiftUI rows use). Unrealized rows never enter SwiftUI layout.TabItemView,SidebarWorkspaceGroupHeaderView) through one reusedNSHostingVieweach (sizing negotiation disabled), fed by the same immutable snapshot values as the SwiftUI path — the Fix sidebar scroll layout livelock #8211 snapshot boundary is unchanged, and both paths share the row-input/group-snapshot builders and the action factory.otherMouseDown,scrollRowToVisible). Workspace reorder and bonsplit drops reuse the existing AppKit drop views, with drop-target rects resolved from table row geometry only while a drag is active.sidebarPlatformListHostedenvironment flag soTabItemViewskips its SwiftUI drag/bonsplit-drop mounts inside the table (NSTableView owns the drag session); in the legacy list those mounts are untouched.Verification (tag
sbtbl, 128 workspaces + 42 notification rows)scripts/check-sidebar-lazy-layout.pykeeps guarding the SwiftUI path exactly as on main; table-specific source guards and 300-workspace table scale tests land next on this branch (implementation was prioritized first).Supersedes #8068.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a beta switch to render the workspace sidebar with an AppKit
NSTableViewinstead of the SwiftUI list. Default is off. When on, scrolling is stable and idle CPU stays low on large workspaces.sidebar.beta.appKitList.enabled(Settings → Beta Features → “AppKit Sidebar List”, also incmux.json).NSTableViewwith measured-once row heights and explicit diffing; unrealized rows never enter SwiftUI.TabItemView, group headers) via a single reusedNSHostingViewper cell; both paths share the same immutable snapshots and actions.sidebarPlatformListHostedenvironment flag so table-hosted rows skip SwiftUI drag/drop mounts; legacy SwiftUI list is unchanged when the flag is off.Written for commit 3ff4b53. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests