Repository navigation
Lawrence Sidebar: coalesce rapid selections + Equatable subtree boundary - #8316
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
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>
📝 WalkthroughWalkthroughSidebar row clicks now preserve captured modifiers and coalesce plain-click selection updates. Visible sidebar cells reconcile hover state after content and viewport changes. Sidebar rendering adds feature-gated equality evaluation and debug row-count logging. ChangesSidebar interaction and rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarWorkspaceTableController
participant SidebarSelectionCoalescer
participant SidebarWorkspaceRowCommands
User->>SidebarWorkspaceTableController: Click workspace row
SidebarWorkspaceTableController->>SidebarSelectionCoalescer: Request plain-click update
SidebarSelectionCoalescer->>SidebarWorkspaceRowCommands: Apply latest updateSelection(modifiers:)
SidebarWorkspaceTableController->>SidebarSelectionCoalescer: Cancel for command/shift click
SidebarWorkspaceTableController->>SidebarWorkspaceRowCommands: Apply modified selection immediately
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge after addressing the missing coalescer cancel in the double-click path. Every code path routes modifier clicks through an immediate cancel+apply, correctly preserving multi-select ordering. The single concrete gap is Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift — the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant TableController as SidebarWorkspaceTableController
participant Coalescer as SidebarSelectionCoalescer
participant Commands as SidebarWorkspaceRowCommands
User->>TableController: click (plain)
TableController->>Coalescer: request(apply)
alt leading edge (no pending task, ≥100ms since last)
Coalescer->>Commands: updateSelection(modifiers) [immediate]
else within window
Coalescer-->>Coalescer: replace pendingApply
Note over Coalescer: trailing Task waits ~100ms
Coalescer->>Commands: updateSelection(modifiers) [deferred]
end
User->>TableController: click (cmd/shift)
TableController->>Coalescer: cancel()
TableController->>Commands: updateSelection(modifiers) [immediate]
User->>TableController: double-click
Note over TableController: ⚠️ cancel() not called here
TableController->>TableController: beginInlineRename()
Note over Coalescer: trailing still live (~50ms later fires)
%%{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 TableController as SidebarWorkspaceTableController
participant Coalescer as SidebarSelectionCoalescer
participant Commands as SidebarWorkspaceRowCommands
User->>TableController: click (plain)
TableController->>Coalescer: request(apply)
alt leading edge (no pending task, ≥100ms since last)
Coalescer->>Commands: updateSelection(modifiers) [immediate]
else within window
Coalescer-->>Coalescer: replace pendingApply
Note over Coalescer: trailing Task waits ~100ms
Coalescer->>Commands: updateSelection(modifiers) [deferred]
end
User->>TableController: click (cmd/shift)
TableController->>Coalescer: cancel()
TableController->>Commands: updateSelection(modifiers) [immediate]
User->>TableController: double-click
Note over TableController: ⚠️ cancel() not called here
TableController->>TableController: beginInlineRename()
Note over Coalescer: trailing still live (~50ms later fires)
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+34
to
+42
| trailingTask = Task { [weak self] in | ||
| try? await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000)) | ||
| 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.
cmux-swift-blocking-runtime rule explicitly states "Do not allow Task.sleep in production code just because it is inside an async function, including for animation timing." The debounce delay here is implemented via Task.sleep in new production Swift. The rule's preferred shape is a real signal or a cancellation-aware timer abstraction rather than a wall-clock sleep. A DispatchSourceTimer (cancelled in cancel()) created on the main queue would satisfy the rule and has identical semantics without relying on cooperative sleep cancellation.
Suggested change
| trailingTask = Task { [weak self] in | |
| try? await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000)) | |
| guard let self, !Task.isCancelled else { return } | |
| self.trailingTask = nil | |
| self.lastApplied = CACurrentMediaTime() | |
| let apply = self.pendingApply | |
| self.pendingApply = nil | |
| apply?() | |
| } | |
| let timer = DispatchSource.makeTimerSource(queue: .main) | |
| timer.schedule(deadline: .now() + delay) | |
| timer.setEventHandler { [weak self] in | |
| guard let self else { return } | |
| self.trailingTask = nil | |
| self.lastApplied = CACurrentMediaTime() | |
| let apply = self.pendingApply | |
| self.pendingApply = nil | |
| apply?() | |
| } | |
| timer.resume() | |
| // trailingTask type should be updated to DispatchSourceTimer |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+14
to
+52
| final class SidebarSelectionCoalescer { | ||
| private var pendingApply: (() -> Void)? | ||
| private var trailingTask: Task<Void, Never>? | ||
| private var lastApplied: CFTimeInterval = 0 | ||
| private let window: TimeInterval | ||
|
|
||
| init(window: TimeInterval = 0.1) { | ||
| self.window = window | ||
| } | ||
|
|
||
| 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)) | ||
| trailingTask = Task { [weak self] in | ||
| try? await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000)) | ||
| guard let self, !Task.isCancelled else { return } | ||
| self.trailingTask = nil | ||
| self.lastApplied = CACurrentMediaTime() | ||
| let apply = self.pendingApply | ||
| self.pendingApply = nil | ||
| apply?() | ||
| } | ||
| } | ||
|
|
||
| /// Drops any pending request. Used before selection paths that must not | ||
| /// be reordered (modifier clicks mutate the multi-selection set). | ||
| func cancel() { | ||
| trailingTask?.cancel() | ||
| trailingTask = nil | ||
| pendingApply = nil | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The cmux-swift-architectural-rethink rule flags timing repair paths used to paper over rendering races. The coalescer reduces how often updateSelection triggers a container re-render and terminal-content swap, but does not name or fix why a single updateSelection call is expensive enough to cause noticeable serial slowdown. A rapid burst on a slower machine, or a future caller outside the flag, will surface the same problem. Consider documenting what the expensive operation is and whether it can be made idempotent (skipping work when the workspace being selected is already selected) or deferred to the next run-loop turn.
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>
azooz2003-bit
changed the title
…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>
Collaborator
Author
|
On the 'timing repair leaves commit cost unaddressed' note: agreed that coalescing alone would paper over the cost, which is why this PR pairs it with the Equatable boundary that removes the sidebar subtree from parent-driven commits entirely. The remaining per-commit cost is portal geometry sync and whole-window layout, tracked separately (Time Profiler attribution in #8270). The coalescer's role is semantic: a click burst has exactly one intended terminal destination, so intermediate content swaps are dropped by design, not merely deferred. Task.sleep is now an injected Clock per the bounded-delay policy. |
…on-coalesce # Conflicts: # Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Collaborator
Author
|
Consolidated into #8366 per dogfood owner request (single PR for the Lawrence Sidebar fix train). |
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.
Rapid sidebar clicks each queued a full selection commit (container re-render + terminal-content swap), so consecutive selections serialized and later ones felt slower — reported during Lawrence Sidebar dogfood. Plain clicks now route through a leading+trailing coalescer (100ms): single clicks apply immediately, bursts collapse to first + newest. The optimistic press highlight still tracks every click instantly; cmd/shift clicks bypass coalescing (order-dependent multi-select). Flag-ON only (
sidebar-appkit-list-experiment).Verified on tag
sbak: a 6-click burst at ~45ms spacing produced exactly 2sidebar.selectcommits (leading + trailing last-wins); single clicks unchanged.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Coalesce rapid sidebar clicks so only the first and latest selection apply, reduce sidebar re-renders with an
.equatable()boundary, and fix hover chrome stranding by enforcing hover on visible cells. Enabled behindsidebar-appkit-list-experiment.Performance
SidebarSelectionCoalescer(100ms lead+trail) to collapse plain-click bursts; Cmd/Shift clicks bypass; modifiers captured at click time; trailing delay uses an injected Clock with cancel.VerticalTabsSidebarwith.equatable()under the flag to skip parent-driven re-evaluations; added a DEBUG rows-build log; verified a 6-click burst -> 2 selection commits, single clicks unchanged.Bug Fixes
Written for commit 729977c. Summary will update on new commits.
Summary by CodeRabbit