Repository navigation
Lawrence Sidebar: consolidated dogfood fixes (badge centering, selection coalescing, Equatable boundary, hover enforcement, highlight width) - #8366
Merged
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.
…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>
…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
Same defect class as the workspace-row badge: NSTextField's intrinsic insets shift a single digit off the circle's optical center. The header now uses the shared direct-draw SidebarRowUnreadBadgeView, which centers on the digit's cap-height band; badge width tracks the measured text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A selection commit re-renders the container and swaps terminal content (~tens of ms); a burst of clicks queued one full commit per click, so later selections felt progressively slower. Plain clicks in the AppKit table now route through a leading+trailing coalescer (100ms window): single clicks apply immediately, clicks inside the window replace the pending request, and one trailing fire applies only the newest. The row's optimistic press highlight still tracks every click instantly, and modifiers are captured at click time so a deferred apply cannot re-read the keyboard. Cmd/shift clicks bypass coalescing entirely (multi-select mutations are order-dependent). Verified on tag sbak: a 6-click burst (~45ms apart) produced exactly 2 selection commits (first + last row), single clicks unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Parent-driven re-evaluations of ContentView (divider width ticks, any unrelated @State churn) re-ran the whole sidebar subtree: body, the 128-row projection prelude, and the AttributeGraph diff. The sidebar's inputs are stable identities (windowId, window, two model objects) with width applied outside via .frame, so with the flag on the subtree now sits behind .equatable(): parent re-evals with identical inputs skip it entirely. Every data source the body renders from (@EnvironmentObject, @ObservedObject, @binding, @State) invalidates the view directly and bypasses the gate — verified live: notifications and selection still rebuild rows (7 rebuilds for notify+select), pure idle settles to ~0.17 rebuilds/s driven only by real shell churn. Flag off keeps the ungated subtree. Adds a DEBUG rowsBuild log line as the countable signal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e Sidebar) Hover-revealed chrome (row close button, header plus) stranded on rows the pointer had left: per-transition repaints resolve row ids against a rows array that can mutate in the same tick, so content churn scrolling rows under a parked pointer left multiple rows showing hover chrome at once. The controller now sweeps visible cells after each apply and viewport change and enforces hover from the authoritative hoveredRowId; cells short-circuit when already correct. Verified on tag sbak with the notification spam environment and a parked cursor: 45s of churn ends with exactly one hovered row showing its close button (previously three or more). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e Sidebar) Hover-revealed chrome (row close button, header plus) stranded on rows the pointer had left: per-transition repaints resolve row ids against a rows array that can mutate in the same tick, so content churn scrolling rows under a parked pointer left multiple rows showing hover chrome at once. The controller now sweeps visible cells after each apply and viewport change and enforces hover from the authoritative hoveredRowId; cells short-circuit when already correct. Verified on tag sbak with the notification spam environment and a parked cursor: 45s of churn ends with exactly one hovered row showing its close button (previously three or more). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ted Clock Greptile P1: enforcePointerHovering only refreshed the close button, but hover also gates the trailing badge and spinner (hidden while the close button shows) — the sweep now re-runs applyModel so every hover-derived slot stays consistent. The coalescer's trailing delay moves from raw Task.sleep to an injected Clock sleep with cancellation, per the bounded-delay policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ted Clock Greptile P1: enforcePointerHovering only refreshed the close button, but hover also gates the trailing badge and spinner (hidden while the close button shows) — the sweep now re-runs applyModel so every hover-derived slot stays consistent. The coalescer's trailing delay moves from raw Task.sleep to an injected Clock sleep with cancellation, per the bounded-delay policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on-coalesce # Conflicts: # Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift
…arity # Conflicts: # Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift
…adge # Conflicts: # Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift # Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift
…e Sidebar) The AppKit sidebar's selection background (and all row content) sat 6pt inboard of the legacy sidebar's geometry: NSTableView's .fullWidth style still insets cell frames by ~6pt per side, which stacked with the cell's own 6pt outer padding for 12pt total vs legacy's 6pt. With .plain the table hands cells the full row width and the cell's padding is the only inset. Measured on tag sbak: selection left inset now exactly 6.0pt, matching the legacy sidebar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dogfood-lawrence-combined
…' into dogfood-lawrence-combined
… dogfood-lawrence-combined
📝 WalkthroughWalkthroughThe PR adds feature-flagged sidebar render gating, coalesced selection updates, click-time modifier handling, optimistic selection painting, authoritative hover synchronization, plain table geometry, and direct-drawn unread badges for the AppKit sidebar. ChangesAppKit sidebar behavior
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarWorkspaceTableController
participant SidebarSelectionCoalescer
participant SidebarWorkspaceRowCommands
User->>SidebarWorkspaceTableController: Click row with modifier flags
SidebarWorkspaceTableController->>SidebarSelectionCoalescer: Queue plain-click selection
SidebarSelectionCoalescer->>SidebarWorkspaceRowCommands: Apply latest selection
SidebarWorkspaceTableController->>SidebarWorkspaceRowCommands: Apply command/shift selection immediately
Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 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 |
This was referenced Jul 17, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Sidebar/AppKitList/SidebarSelectionCoalescer.swift`:
- Around line 14-47: Make SidebarSelectionCoalescer generic over the injected
Clock type, storing lastApplied as C.Instant instead of CFTimeInterval. Replace
CACurrentMediaTime() calculations with clock.now and use the Clock duration APIs
for elapsed-time and delay computation, preserving immediate versus trailing
application behavior while allowing virtual clocks to control all timing.
In `@Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift`:
- Around line 180-182: Update the modifier capture near the click-handling logic
to read modifierFlags from NSApp.currentEvent instead of the global
NSEvent.modifierFlags state. Preserve the existing captured modifiers value used
by the trailing/coalesced apply so it reflects the exact click-time event.
🪄 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: 1c70c751-a06c-48b4-97bc-1e553fcffbd1
📒 Files selected for processing (7)
Sources/ContentView.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCommands.swiftSources/Sidebar/AppKitList/SidebarSelectionCoalescer.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableController.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+14
to
+47
| final class SidebarSelectionCoalescer { | ||
| private var pendingApply: (() -> Void)? | ||
| private var trailingTask: Task<Void, Never>? | ||
| private var lastApplied: CFTimeInterval = 0 | ||
| private let window: TimeInterval | ||
| private let clock: any Clock<Duration> | ||
|
|
||
| init(window: TimeInterval = 0.1, clock: any Clock<Duration> = ContinuousClock()) { | ||
| self.window = window | ||
| self.clock = clock | ||
| } | ||
|
|
||
| func request(_ apply: @escaping @MainActor () -> Void) { | ||
| let now = CACurrentMediaTime() | ||
| if trailingTask == nil, now - lastApplied >= window { | ||
| lastApplied = now | ||
| apply() | ||
| return | ||
| } | ||
| pendingApply = apply | ||
| guard trailingTask == nil else { return } | ||
| let delay = max(0, window - (now - lastApplied)) | ||
| // Injected-Clock sleep with cancellation wired to `cancel()`, per the | ||
| // bounded-delay policy (no raw Task.sleep in production paths). | ||
| trailingTask = Task { [weak self, clock] in | ||
| try? await clock.sleep(for: .seconds(delay)) | ||
| guard let self, !Task.isCancelled else { return } | ||
| self.trailingTask = nil | ||
| self.lastApplied = CACurrentMediaTime() | ||
| let apply = self.pendingApply | ||
| self.pendingApply = nil | ||
| apply?() | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Support virtual time tests by making the coalescer generic over the injected Clock.
lastApplied is checked using CACurrentMediaTime(), but the task delay relies on the injected clock.sleep(). If a test injects a virtual clock (e.g., TestClock from Swift Async Algorithms), CACurrentMediaTime() will not advance alongside virtual time. This causes the condition now - lastApplied >= window to fall back to wall-clock time, defeating the purpose of the injected clock for deterministic testing.
Consider making the class generic over <C: Clock> so it can store lastApplied: C.Instant and fully rely on clock.now for all timing logic.
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 14-14: Classes should have an explicit deinit method
(required_deinit)
🤖 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/SidebarSelectionCoalescer.swift` around lines 14 -
47, Make SidebarSelectionCoalescer generic over the injected Clock type, storing
lastApplied as C.Instant instead of CFTimeInterval. Replace CACurrentMediaTime()
calculations with clock.now and use the Clock duration APIs for elapsed-time and
delay computation, preserving immediate versus trailing application behavior
while allowing virtual clocks to control all timing.
Comment on lines
+180
to
+182
| // Capture modifiers at click time: a coalesced (trailing) apply | ||
| // must not re-read the keyboard ~100ms later. | ||
| let modifiers = NSEvent.modifierFlags |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prefer NSApp.currentEvent?.modifierFlags for exact click-time state.
As per path instructions for correctness-critical UI state, ensure there is a single authoritative source of truth, such as the system-provided event state captured at click time. NSEvent.modifierFlags reads the global keyboard state, which might have changed if the user releases a modifier key very rapidly after clicking. Reading from NSApp.currentEvent ensures you evaluate the exact modifiers active when the click occurred.
💡 Proposed fix
// Capture modifiers at click time: a coalesced (trailing) apply
// must not re-read the keyboard ~100ms later.
- let modifiers = NSEvent.modifierFlags
+ let modifiers = NSApp.currentEvent?.modifierFlags ?? NSEvent.modifierFlags📝 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
| // Capture modifiers at click time: a coalesced (trailing) apply | |
| // must not re-read the keyboard ~100ms later. | |
| let modifiers = NSEvent.modifierFlags | |
| // Capture modifiers at click time: a coalesced (trailing) apply | |
| // must not re-read the keyboard ~100ms later. | |
| let modifiers = NSApp.currentEvent?.modifierFlags ?? NSEvent.modifierFlags |
🤖 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 180 - 182, Update the modifier capture near the click-handling logic to
read modifierFlags from NSApp.currentEvent instead of the global
NSEvent.modifierFlags state. Preserve the existing captured modifiers value used
by the trailing/coalesced apply so it reflects the exact click-time event.
Source: Path instructions
Contributor
Greptile SummaryThis PR consolidates five overnight dogfood fixes for the
Confidence Score: 5/5All changes are flag-gated (sidebar-appkit-list-experiment, default OFF); flag-off paths are untouched and the fixes are well-scoped to the AppKit sidebar layer. The five fixes address real dogfood regressions with clear invariants, verified behavior, and appropriate guards (early-out on hover sweep, modifier bypass in coalescer, No files require special attention beyond what the prior threads already cover on SidebarSelectionCoalescer.swift and SidebarGroupHeaderRowView.swift. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant NSTableView
participant Controller as SidebarWorkspaceTableController
participant Coalescer as SidebarSelectionCoalescer
participant Cell as SidebarWorkspaceRowTableCellView
participant Commands as SidebarWorkspaceRowCommands
User->>NSTableView: mouseDown (press)
NSTableView->>Cell: showOptimisticSelectionHighlight()
Note over Cell: applies mutable copy with isActive=true
User->>NSTableView: mouseUp (click)
NSTableView->>Controller: didClickTableRow()
Controller->>Controller: capture NSEvent.modifierFlags
alt cmd / shift held
Controller->>Coalescer: cancel()
Controller->>Commands: updateSelection(modifiers:)
Commands-->>Controller: selection committed immediately
else plain click
Controller->>Coalescer: request closure
alt leading edge
Coalescer->>Commands: updateSelection immediately
else burst window
Note over Coalescer: trailing Task sleeps ~100ms
Coalescer-->>Commands: updateSelection fires once
end
end
Controller->>Controller: enforceHoverOnVisibleCells()
loop each visible row
Controller->>Cell: enforcePointerHovering(hovering)
alt hover state changed
Cell->>Cell: applyModel + needsLayout
else no change
Note over Cell: guard short-circuits
end
end
%%{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 User
participant NSTableView
participant Controller as SidebarWorkspaceTableController
participant Coalescer as SidebarSelectionCoalescer
participant Cell as SidebarWorkspaceRowTableCellView
participant Commands as SidebarWorkspaceRowCommands
User->>NSTableView: mouseDown (press)
NSTableView->>Cell: showOptimisticSelectionHighlight()
Note over Cell: applies mutable copy with isActive=true
User->>NSTableView: mouseUp (click)
NSTableView->>Controller: didClickTableRow()
Controller->>Controller: capture NSEvent.modifierFlags
alt cmd / shift held
Controller->>Coalescer: cancel()
Controller->>Commands: updateSelection(modifiers:)
Commands-->>Controller: selection committed immediately
else plain click
Controller->>Coalescer: request closure
alt leading edge
Coalescer->>Commands: updateSelection immediately
else burst window
Note over Coalescer: trailing Task sleeps ~100ms
Coalescer-->>Commands: updateSelection fires once
end
end
Controller->>Controller: enforceHoverOnVisibleCells()
loop each visible row
Controller->>Cell: enforcePointerHovering(hovering)
alt hover state changed
Cell->>Cell: applyModel + needsLayout
else no change
Note over Cell: guard short-circuits
end
end
Reviews (2): Last reviewed commit: "Apply the full selection treatment optim..." | Re-trigger Greptile |
Comment on lines
+38
to
+46
| trailingTask = Task { [weak self, clock] in | ||
| try? await clock.sleep(for: .seconds(delay)) | ||
| guard let self, !Task.isCancelled else { return } | ||
| self.trailingTask = nil | ||
| self.lastApplied = CACurrentMediaTime() | ||
| let apply = self.pendingApply | ||
| self.pendingApply = nil | ||
| apply?() | ||
| } |
Contributor
There was a problem hiding this comment.
Timing-based debounce in production path
clock.sleep is a timing-based delay in a @MainActor production path. The cmux no-sleep and architectural-rethink rules flag timing repairs, even when the clock is injected and cancellation is wired. Concretely: if cancel() is called between the sleep completing and the task resuming on the main actor (another event queued at that suspension point), Task.isCancelled will be true and the guard bails — but the window between sleep completion and the guard check is an observable staleness window where the abandoned pendingApply still holds a captured closure.
The injected-clock and bounded-delay design mitigate this for testing, and the 100 ms cap is reasonable. But worth explicitly verifying that this coalescer's existence is not a symptom of an upstream cost that could be reduced further (e.g., making the selection-commit path cheaper so no debounce is needed at all, or moving the coalescing to the SwiftUI state layer where it can be expressed without timing).
Rule Used: Flag new blocking or timing-based synchronization ... (source)
Comment on lines
+267
to
+268
| let textSize = NSString(string: "\(model.anchorUnreadCount)") | ||
| .size(withAttributes: [.font: unreadBadgeFont]) |
Contributor
There was a problem hiding this comment.
Per-layout NSString allocation on a sidebar row path
NSString(string: "\(model.anchorUnreadCount)") bridges a Swift string through NSString on every layout() pass. layout() runs during every resize, scroll bounce, and content-driven height change. Because this is gated on !unreadBadgeView.isHidden, the allocation only occurs when the badge is visible — but that is exactly the case when sidebar churn (notifications, unread count ticks) is most frequent.
The previous unreadBadgeLabel.intrinsicContentSize call had a similar per-call cost. A simple improvement: cache the measured badgeSize alongside unreadBadgeFont and invalidate it only when the font or count changes, avoiding the allocation on re-layout passes where neither changed.
Rule Used: Flag per-call allocating formatting on hot or conc... (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!
…Sidebar) Dogfood video showed a two-phase selection: the press painted only the background and title instantly, while selection-derived colors on the notification preview, secondary text, and badges trailed in with the terminal swap. The optimistic press/deselect paths now apply a selection-flipped copy of the row model through the normal applyModel pass, so every selection-derived color flips with the highlight. The stored model stays authoritative and the next configure reconciles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 17, 2026
azooz2003-bit
added a commit
that referenced
this pull request
Jul 23, 2026
* Sidebar: move workspace reorder drops to native NSTableView validateDrop/acceptDrop The AppKit sidebar table previously received workspace reorder drops through a transparent SidebarWorkspaceReorderDropView overlay stacked above the scroll view, with a geometry gate feeding it visible-row frames and a pending-drop state machine covering the async gap between drop arrival and target collection. The table now registers for the reorder pasteboard type itself and resolves drops in the data source's validateDrop/acceptDrop, building visible-row targets synchronously in table coordinates and passing them to the same shared SidebarWorkspaceReorderDropResolver. draggingDestinationFeedbackStyle is .none, so the row-painted indicator bars and the container's empty-indicator overlay keep owning all drop visuals unchanged. Behavior improvement: the controller stores the accepted drag point in window space and re-plans it on every viewport change, so during edge autoscroll the drop target and indicator track the rows sliding under a stationary pointer; previously the indicator was only geometrically repositioned and the plan froze until the pointer moved. Drag exit and session end (including Escape cancel over the table) now clear the indicator through explicit draggingExited/draggingEnded overrides. The geometry gate is now bonsplit-only, the reorder overlay and its pending-drop machinery are no longer instantiated by the AppKit container (the legacy SwiftUI sidebar still uses that overlay class, unchanged), and the reorder side of SidebarWorkspaceTableActions drops its target-collection closure. Also fixes a stale .fullWidth style assertion in SidebarWorkspaceTableTests that predates the .plain switch in #8366 and slipped through while PR CI was disabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix main build: drop convenience from actor initializer #8521 added public convenience init() to the GitHubPullRequestRequestCoordinator actor. Actor initializers delegate without convenience (SE-0327) and current Swift toolchains reject the keyword as a hard error, so every fleet reload-cloud build and CI compile of main fails with 'initializers in actors are not marked with convenience'. It landed silently because PR CI is currently disabled. Dropping the keyword keeps the same public surface and delegation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: controller-owned reorder indicator painting Dogfood video on a 100-row sidebar showed three drag defects: the drop line lagged the pointer by hundreds of pixels, transitions briefly drew two lines at once, and a line was baked into the drag ghost for the whole drag. Root cause for the first two: the reorder plan was written into the SwiftUI drag state, so every gap change rebuilt every sidebar row and repainted the line one staged apply later. The table's actions now return the plan to the controller (SidebarWorkspaceTableReorderDropUpdate) and never touch dragState; the controller maps the indicator onto the two affected cells with SidebarWorkspaceTableReorderIndicatorPainter, which wraps the same SidebarTabDropIndicatorPredicate and scope-filtered row ids the SwiftUI sidebar uses, so gap semantics are unchanged while transitions become two direct view mutations. Root cause for the ghost: AppKit snapshots the dragged row lazily, after the first validateDrop painted the indicator at the grabbed row's own edge. The painter suppresses both lines on the dragged row, so the snapshot is always clean. Recycled or reconfigured cells re-apply the controller paint after their model reset it, retire paths (drop, exit, session end) sweep the lines clear, and the apply-time indicator sync prefers the live reorder painter so dragState (bonsplit-only now) cannot clear the past-the-end overlay mid-drag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix tuple labels lost through nil-coalescing in indicator sweep Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar drag: stop bottom-edge autoscroll oscillation, re-arm cleared drags Second dogfood round on the 100-row sidebar surfaced two failures. Autoscroll parked at the very bottom stuttered continuously: the 60Hz timer's native NSClipView.autoscroll(with:) path has no content clamp, so each tick overshot into the elastic region and rubber-banded back. The tick now computes the plan first and stops the timer when constrainBoundsRect proves the clip view cannot advance in the planned direction; the next drag update restarts it. The manual fallback already clamped. A long drag went indicator-less and its drop silently no-oped: the app briefly resigned active mid-drag (busy multi-agent desktop), the app_resign_active failsafe cleared dragState AND the process-wide drag registry, and activateSidebarWorkspaceDragIfNeeded could only re-arm foreign drags, so every later validateDrop was rejected while the native session stayed alive. The drag pasteboard id (parsed once per validateDrop and handed through the table actions) now re-arms local drags too: a still-delivering drop callback is proof the session is real. Escape-cancel cannot resurrect because AppKit ends the session and no further callbacks arrive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar drag: robust payload parse for re-arm plus activation probe string(forType:) can be nil for item-provider-promised drag data; fall back to a UTF-8 decode of data(forType:). DEBUG logs record the parsed payload and activation rejections so silent no-op drops are attributable from the tagged log. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar drag: single autoscroll driver, inset-aware clamp Making the table a native drag destination silently added AppKit's built-in drag autoscroll on top of cmux's SidebarDragAutoScrollController. Two drivers with different engagement bands, speeds, and boundary behavior fought during drags: scrolling continued after the pointer left the cmux edge zone (AppKit's band is much taller than 44pt), and edge behavior was erratic. The table now declines AppKit autoscroll (autoscroll(with:) -> false); the cmux controller is the only driver. The controller is also planner-only now. Its autoscroll(with:) fast path read NSApp.currentEvent inside a timer tick — a stale position basis — and had no content clamp. The manual path's [0, contentHeight] clamp ignored the scroll view's content insets, which is why upward autoscroll stopped one top-inset short of the actual top. Ticks now scroll by the planner's ramped step and clamp through constrainBoundsRect, which honors insets and cannot enter the elastic region. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar drag: scope AppKit-autoscroll refusal to active drop sessions The unconditional autoscroll(with:) -> false override killed row-drag initiation: NSTableView's mouseDown tracking loop also calls autoscroll while deciding whether a press becomes a drag, and refusing it there made every drag die at birth (pasteboardWriterForRow ran, then the mouse-up landed as a plain click; log shows dragState.sidebar set followed by sidebar.table.click with no session). The table now declines AppKit's built-in drag autoscroll only while the controller reports a live reorder drop session hovering, which is the only window where the second scroll driver exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Failing test: autoscroll planner measures edge distance in document coordinates Pointer positions converted into the clip view are document coordinates whose origin is the scroll offset. The planner treated them as viewport-relative, so once the list is scrolled more than one viewport height deep, every pointer position measures as past the bottom edge and plans max-speed downward scrolling from anywhere: the runaway scroll-down and the up-scroll stutter (down-plan fighting upward input) from dogfood. Test-only commit plus the visibility change it needs; the fix lands in the next commit so CI shows red then green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix autoscroll planner coordinate space: viewport-relative edge distances Subtract the clip view's bounds origin before measuring distance to the viewport edges. Scroll-down now stops the moment the pointer leaves the 44pt bottom zone regardless of scroll depth, and upward edge scrolling no longer fights a phantom max-speed downward plan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar drag: drop commits the painted plan, not a release-time re-resolve Dogfood on rapid consecutive drags: the row occasionally landed one gap away from the last indicator shown. Two races made the drop diverge from the paint: the pointer can drift a few pixels between the final draggingUpdated and the release (AppKit sends no update for those), and an autoscroll tick can shift rows after the last coalesced repaint but before acceptDrop. Both re-resolved a fresh plan at release time that could cross a row-midpoint boundary the indicator never showed. The accepted update now carries the resolver's full drop plan; the controller stores it alongside the painted indicator and acceptDrop commits that stored plan verbatim through a new commitWorkspaceDropPlan action (falling back to point resolution only when no hover plan exists). Indicator and outcome are now the same object by construction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar drag: keep the viewport at the release position after a drop Half of drops visibly yanked the sidebar away from the drop spot, often toward the dragged row's old position. Cause: the apply after a drop shifts the selected workspace's index, which SidebarSelectedWorkspaceScrollPolicy cannot distinguish from an external change, so scrollSelectedRowToVisibleIfNeeded jumped the viewport to the selected row whenever it was offscreen. It only looked correct when the selected row's index happened not to change or was already visible. A successful drop now arms a one-shot suppression consumed by the next apply; external selection changes still scroll. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Re-mint painter pbxproj UUIDs after collision with main The hand-minted B804x02A build-file/file-reference pair for SidebarWorkspaceTableReorderIndicatorPainter.swift was independently allocated on main for SidebarWorkspaceRowTodoViews.swift. After merging main, both files claimed the same UUIDs; Xcode keeps one definition and silently drops the other file from the build, which broke compilation with 'cannot find SidebarRowClosureMenuItem in scope'. The painter now uses a unique pair outside the sequential B804 series. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: cmux reload-cloud <cmux-reload-cloud@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
5 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates the overnight Lawrence Sidebar fix train into one PR, per dogfood owner request. All flag-ON only (
sidebar-appkit-list-experiment, default OFF); flag-off paths untouched.Group-header badge centering — the header's unread badge rendered its digit off the circle's optical center (NSTextField intrinsic insets). Now uses the shared direct-draw
SidebarRowUnreadBadgeView(cap-height optical centering), one implementation for header and row badges.Selection coalescing — rapid sidebar clicks each queued a full selection commit (container re-render + terminal swap), so burst clicks felt progressively slower. Plain clicks route through a leading+trailing 100ms coalescer: single clicks apply immediately, bursts collapse to first + newest; press highlight still tracks every click; cmd/shift bypass (order-dependent); modifiers captured at click time; injected-Clock delay. Verified: 6-click burst at ~45ms spacing → exactly 2 selection commits.
Equatable boundary on
VerticalTabsSidebar— divider width ticks and unrelated ContentView churn re-ran the whole sidebar subtree (body + 128-row prelude + graph diff). The subtree now sits behind.equatable()(inputs are stable identities; width applies outside via.frame); all rendered data flows through tracked dependencies that bypass the gate. Verified: notifications/selection still rebuild rows; pure idle ~0.17 rebuilds/s.Hover enforcement — hover-revealed chrome (close button, header plus) stranded on rows the pointer left (id/index races during churn). The controller sweeps visible cells after each apply/viewport change, enforcing hover from the authoritative
hoveredRowId; the sweep re-runs the full model apply so badge/spinner slots stay consistent. Verified: 45s parked-cursor churn → exactly one hovered row shows its close button.Highlight width parity — the selection background (and all row content) sat 6pt inboard of legacy: NSTableView
.fullWidthstill insets cell frames ~6pt/side, stacking with the cell's 6pt outer padding..plainhands cells the full row width. Verified: left inset now exactly 6.0pt, matching legacy.Supersedes #8312, #8316, #8323, #8364.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Consolidates Lawrence Sidebar polish behind
sidebar-appkit-list-experiment(default OFF). Selection is snappier with full color flip on press, hover chrome stays correct, badges are centered, and the highlight matches legacy width.Bug Fixes
SidebarRowUnreadBadgeView.NSTableViewto.plainso rows use full width and the highlight inset matches legacy.Refactors
SidebarSelectionCoalescer(100ms leading+trailing) to collapse rapid plain-click selections; captures modifiers at click time; cmd/shift bypass.VerticalTabsSidebarwith.equatable()to skip sidebar subtree rebuilds on unrelated parent updates (tracked data still updates normally).Written for commit f9917aa. Summary will update on new commits.
Summary by CodeRabbit
Performance
Bug Fixes