perf: render Overview row selection on the GPU to fix scroll stutter (#1674)#1740
Open
hhh2210 wants to merge 2 commits into
Open
perf: render Overview row selection on the GPU to fix scroll stutter (#1674)#1740hhh2210 wants to merge 2 commits into
hhh2210 wants to merge 2 commits into
Conversation
Overview rows are rich SwiftUI views hosted in NSMenu. Every scroll step flipped `menuItemHighlighted`, which re-rasterized the whole row subtree (~2.4 ms avg, ~25 ms spikes — matching the dropped frames reported in steipete#1674). A headless benchmark over the real hosting path measured A: SwiftUI recolor avg 2.35 ms / max 7.29 ms; `.drawingGroup()` (Metal rasterization only) helped ~28% and still missed the 8.3 ms/120 Hz budget. Move selection rendering off the SwiftUI graph entirely: - AppKit NSVisualEffectView(.selection) background, crossfaded via Core Animation - a CIColorMatrix content filter tinting the row to the selected text color, which Core Image runs on the GPU (Metal) and matches the existing all-white selected look Per-toggle cost drops to ~0.044 ms / max 1.39 ms — 53x faster, well within the 120 Hz budget. Scoped to Overview rows via `makeMenuCardItem(usesGPUSelection:)`; it does not override hitTest, so embedded controls keep working. Also pass precise trackpad scrolls through to AppKit's native menu scroller instead of thresholded row-highlight jumps, so the menu follows the finger; classic notched wheels keep row-to-row navigation. (Trackpad pass-through and the deterministic bypass test came from a parallel Codex exploration.) Adds `gpu selection highlight bypasses swiftui highlight state`, a timing-free test proving the GPU selection never invalidates the hosted SwiftUI highlight state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 10:47 PM ET / 02:47 UTC. Summary Reproducibility: no. high-confidence local reproduction was run in this read-only review. The linked issue and PR body provide sample/benchmark evidence, and current source matches the Overview NSMenu/SwiftUI highlight path. Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land exactly one profiled Overview stutter fix after maintainer-visible macOS 27 runtime proof confirms scrolling, selection, click, and submenu behavior, then close or park the alternative PRs. Do we have a high-confidence way to reproduce the issue? No high-confidence local reproduction was run in this read-only review. The linked issue and PR body provide sample/benchmark evidence, and current source matches the Overview NSMenu/SwiftUI highlight path. Is this the best way to solve the issue? Unclear. Moving selection off the SwiftUI graph is a coherent narrow implementation, but the precise-scroll behavior change and sibling alternatives still need maintainer choice and runtime proof before this is clearly the best shipping fix. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ada3660e9d61. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets the Overview-menu scroll stutter by removing per-highlight SwiftUI invalidation work during NSMenu tracking, shifting selection rendering to an AppKit/CoreAnimation + Core Image (GPU) path and improving scroll “hand-feel” by letting precise trackpad scrolling use native menu scrolling.
Changes:
- Add
GPUSelectionHostingViewto render selection background viaNSVisualEffectView(.selection)and tint content via a Core Image filter, without toggling SwiftUI highlight state. - Update Overview scroll handling to pass precise scrolling deltas through to AppKit (native continuous scrolling), while keeping coarse wheel step navigation.
- Extend test coverage for precise-vs-coarse scrolling behavior and for verifying GPU selection bypasses SwiftUI highlight state; add an investigation write-up doc.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusMenuOverviewScrollTests.swift | Updates scroll navigation tests to distinguish coarse wheel stepping vs precise pass-through behavior. |
| Tests/CodexBarTests/MenuCardViewRecyclingTests.swift | Adds a regression test proving GPU-highlight does not flip SwiftUI highlight state. |
| Sources/CodexBar/StatusItemController+OverviewScroll.swift | Passes precise scrolling through to AppKit; keeps wheel-based step navigation behavior. |
| Sources/CodexBar/StatusItemController+MenuCardItems.swift | Adds usesGPUSelection option and wires GPU hosting view creation for menu card items. |
| Sources/CodexBar/StatusItemController+Menu.swift | Enables GPU selection for Overview rows via usesGPUSelection: true. |
| Sources/CodexBar/MenuCardGPUSelectionView.swift | Introduces the GPU/AppKit selection hosting view implementation. |
| docs/overview-scroll-stutter-investigation.md | Adds detailed investigation notes, measurements, and rationale for the chosen approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+180
to
+190
| /// Maps every pixel's RGB to white while preserving alpha, reproducing the all-white selected | ||
| /// row appearance. Core Image runs this on the GPU (Metal), so it composites for free per frame. | ||
| private static func makeSelectedTextTintFilter() -> CIFilter? { | ||
| guard let filter = CIFilter(name: "CIColorMatrix") else { return nil } | ||
| filter.setValue(CIVector(x: 0, y: 0, z: 0, w: 0), forKey: "inputRVector") | ||
| filter.setValue(CIVector(x: 0, y: 0, z: 0, w: 0), forKey: "inputGVector") | ||
| filter.setValue(CIVector(x: 0, y: 0, z: 0, w: 0), forKey: "inputBVector") | ||
| filter.setValue(CIVector(x: 0, y: 0, z: 0, w: 1), forKey: "inputAVector") | ||
| filter.setValue(CIVector(x: 1, y: 1, z: 1, w: 0), forKey: "inputBiasVector") | ||
| return filter | ||
| } |
Comment on lines
33
to
35
| // Momentum-phase events after a flick would keep moving the highlight long after | ||
| // the fingers left the trackpad; swallow them without stepping. | ||
| guard event.momentumPhase.isEmpty else { return true } |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
…ment - Derive the selection tint bias from NSColor.selectedMenuItemTextColor instead of hard-coding pure white, so graphite/high-contrast/accessibility appearances tint correctly and the code matches its own doc comment. - Clarify the momentum-phase guard comment: precise trackpad/Magic Mouse scrolling already returns earlier, so the guard only covers non-precise devices that still report a momentum phase. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
P2
Contributor
Author
CleanShot.2026-06-25.at.10.40.40.mp4@clawsweeper re-review |
clawsweeper
Bot
added
the
proof: 🎥 video
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
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.
Third direction for #1674, superseding the lite-row (#1675) and rich-row (#1676) drafts: keep the rich Overview rows, but move selection rendering off the SwiftUI graph onto the GPU. Two parallel explorations (Claude + Codex) independently converged on this design.
Root cause (measured)
Overview rows are rich SwiftUI views hosted in
NSMenu. Each scroll step flipsmenuItemHighlighted, which re-rasterizes the whole row subtree. A headless benchmark drove the real production hosting path (OverviewMenuCardRowView → MenuCardSectionContainerView → hosting view), toggling selection 200× (setHighlighted → layout → display → runloop-flush; macOS 27, Swift 6.4, debug):.drawingGroup()(Metal rasterization only)CIColorMatrix+ AppKit selection (this PR)Before/after on the production types, same loop/machine:
This lines up with the report: the reporter's recording is a 120 fps capture running ~57 fps effective with worst frame gaps ~25 ms, matching variant A's spikes. Variant D (content pinned, only the container's highlight modifiers change) still cost ~3–8 ms, proving the cost is content re-rasterization on highlight — so the fix must take selection off the SwiftUI graph.
Fix
Two GPU-composited steps, zero SwiftUI work per scroll step:
NSVisualEffectView(.selection)selection background (the existingPersistentRefreshMenuViewpattern), crossfaded via Core Animation.CIColorMatrixcontent filter mapping the row to the selected text color — Core Image runs on the GPU (Metal), matching the existing design where selected rows already go white.Scoped to Overview rows via
makeMenuCardItem(usesGPUSelection:); it does not overridehitTest, so embedded controls (e.g. the error copy button flagged on #1676) keep working.Hand-feel (
不跟手) — separate, also addressedCheap highlight toggles don't fix feel:
handleOverviewScrollWheelquantizes the wheel into discrete row jumps. Precise trackpad events are now passed through to AppKit's native menu scroller (continuous, finger-following); classic notched wheels keep row navigation.Verification (timing-independent)
gpu selection highlight bypasses swiftui highlight state: highlighting a GPU row marks the AppKit view selected while the hostedMenuCardHighlightState.isHighlightedstays false — proving selection never re-invalidates the SwiftUI graph.swift buildclean; Overview-scroll + menu-card recycling/highlight suites green.See
docs/overview-scroll-stutter-investigation.mdfor the full analysis.Adding the local recording that motivated the final
GPUSelectionHostingView+ precise-trackpad pass-through direction.Scope: this is a baseline / problem recording, captured before the GPU-selection + native precise-scroll fix in this PR. It shows the Overview menu feeling non-finger-following / stuttery while scrolling a provider-heavy rich-row menu — repro evidence for #1674, and context for why this PR also passes precise trackpad deltas through to AppKit's native menu scrolling. The clip is cropped to remove account identifiers; the original local capture was 120 fps H.264.
The proof for the patch itself remains the benchmark + tests in the PR body:
codexbar-overview-scroll-proof-public-crop.mp4
A follow-up real-menu recording from the packaged build would be the strongest end-to-end proof.
🤖 Generated with Claude Code