fix: coalesce diff-pane wheel events to unblock reverse scroll (#179)#180
Merged
Conversation
Rapid wheel events on large diffs previously triggered an O(cursor) pin plus a full SetContent(renderDiff()) per event, so a trackpad flick queued ~50 expensive Update+View cycles. A subsequent reverse-direction wheel event sat at the back of that queue and only took effect once it drained. The diff-pane wheel path now only shifts YOffset synchronously and defers the cursor pin + renderDiff to a single in-flight tea.Tick. A new wheelState.tickInFlight gate guarantees one debounce goroutine per burst regardless of event count (the first wheel schedules a tick; subsequent wheels just bump gen). When the tick fires, handleWheelDebounce either reschedules for the current gen (burst still going) or flushes (burst settled). handleKey and handleResize call flushWheelPending up-front so cursor-relative actions and post-resize scroll see a fresh cursor. Measured on a 4700-event trackpad burst: debounce messages dropped from 4201 to 560, View() calls 9144 to 5469, total View time 21s to 12.6s.
- [major] loaders.go:364 handleBlameLoaded flushes wheel state before syncViewportToCursor (codex) - [major] mouse.go:300 handleWheelDebounce godoc — replaced "stale ticks no-op" with accurate three-branch description - [major] mouse.go:381 flushWheelPending godoc — listed actual callers (handleKey, handleResize, handleBlameLoaded, handleWheelDebounce); dropped false "non-wheel handleMouse" claim - [major] mouse_test.go RendersOnMatchingGen — now exercises real pin + verifies cursor + tickInFlight transitions - [major] mouse_test.go NoopWhenRenderNotPending — seeds tickInFlight=true and verifies cleanup - [major] new test TestModel_HandleKey_FlushesPendingWheelBeforeAction — pins the handleKey flush contract - [major] new test TestModel_HandleResize_FlushesPendingWheelBeforeSync — pins the handleResize flush contract - [major] new test TestModel_HandleBlameLoaded_FlushesPendingWheelBeforeSync — pins the handleBlameLoaded flush contract - [minor] mouse.go:17 wheelRenderDelay comment — dropped stale "pins synchronously" claim - [minor] mouse.go:51 wheelDebounceMsg godoc — replaced "dropped" with accurate reschedule/flush description - [minor] mouse.go:386 flushWheelPending now clears tickInFlight too — prevents next burst waiting ~2x debounce on rare flush-then-wheel sequence
- [test-gap] mouse_test.go:1133 RendersOnMatchingGen — asserts viewport.View() actually changes after the debounce flush. Without this assertion, removing SetContent(renderDiff()) from flushWheelPending would have kept the test passing on state flags alone.
- [doc] mouse.go:18 — wheelRenderDelay comment dropped vague "long enough to coalesce" wording for a concrete "into one render" - [doc] mouse.go:28 — wheelState gen description now states it bumps only when scrollDiffViewportBy returns true (at-edge no-ops skip the bump) - [doc] CLAUDE.md — debounce-msg numbers aligned with the commit's 560 total; noted that ~110 flushes + ~486 reschedules don't tie out exactly because of no-pending/cleanup paths - [doc] README.md:710 Mouse Support — added a sentence noting the cursor highlight catches up after fast scrolls, matches less/vim - [test] mouse_test.go updateWheelAndFlush — t.Fatalf when the wheel handler returns a non-wheelDebounceMsg cmd, so future refactors (e.g. tea.Batch) surface immediately instead of degrading the helper into a plain Update
There was a problem hiding this comment.
Pull request overview
This PR addresses UI latency in the diff pane during rapid mouse-wheel scrolling (issue #179) by coalescing expensive “cursor pin + full diff re-render” work into a single debounce tick, so opposite-direction wheel input can be processed promptly even during large scroll bursts.
Changes:
- Added diff-pane wheel-event coalescing via a single in-flight
tea.Tickdebounce (wheelState+wheelDebounceMsg), making per-wheel handling O(1) (SetYOffsetonly). - Added
flushWheelPending()and wired it into key handling, resize handling, and blame-load handling to ensure cursor-dependent actions see a fresh pinned cursor/content. - Updated README/architecture docs and added/updated tests to cover burst behavior, debounce rescheduling, and flush paths.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents cursor-highlight catch-up behavior after fast scroll bursts. |
| docs/ARCHITECTURE.md | Updates architecture notes to describe wheel coalescing + flush callsites and adds wheelState to state table. |
| CLAUDE.md | Adds detailed internal documentation for the wheel coalescing design and rationale. |
| app/ui/mouse.go | Implements wheel coalescing (wheelState, debounce tick, deferred flush) and refactors diff scrolling to be YOffset-only per wheel. |
| app/ui/mouse_test.go | Adds helper and new tests validating debounce scheduling/rescheduling, burst behavior, and flush triggers. |
| app/ui/model.go | Adds wheelState to Model, routes wheelDebounceMsg, flushes pending wheel work before key actions and resize sync. |
| app/ui/loaders.go | Flushes pending wheel work before blame-driven syncViewportToCursor(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+404
to
+406
| m.pinDiffCursorTo(m.layout.viewport.YOffset) | ||
| m.syncTOCActiveSection() | ||
| m.layout.viewport.SetContent(m.renderDiff()) |
…ays in view Copilot review on #180 caught that flushWheelPending unconditionally re-renders even when pinDiffCursorTo returns false (cursor stayed visible through the burst). In that case the existing viewport content already has the correct cursor highlight, so SetContent(renderDiff()) is wasted work — and on the handleKey path it double-renders because the key handler will also render after its action runs. Gate syncTOCActiveSection + SetContent on the pin's bool return. Always clear renderPending and tickInFlight either way. Also dropped a redundant tickInFlight=false in handleWheelDebounce — flushWheelPending already clears it. New test TestModel_HandleMouse_WheelDebounceMsg_SkipsRenderWhenCursorStaysInView locks in the contract: a flush with cursor still in view must not change viewport.View().
Deploying revdiff with
|
| Latest commit: |
b7e9505
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9feb6945.revdiff.pages.dev |
| Branch Preview URL: | https://fix-wheel-scroll-coalesce.revdiff.pages.dev |
…e is unreachable post-flushWheelPending tightening After da60558 flushWheelPending clears both flags atomically, so the test scenario can't actually arise in production anymore. Reworded the comment to describe the test as a defensive guard for that state combination rather than a real "external flush left tickInFlight true" path.
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.
Fixes #179.
The problem
Rapid wheel events on a large diff used to do O(cursor) work per event in
pinDiffCursorTo(loops throughm.file.linesto compute visual rows) plus a fullSetContent(renderDiff()). On a trackpad flick the message queue piled up with expensive Update+View cycles, and a subsequent wheel-up sat at the back until they all drained. Measured on one user's burst: 4609 wheel events triggered 4201 debounce messages and 21 seconds ofView()time before any reverse-direction event could be processed.The fix
Diff-pane wheel events now do only
SetYOffsetper event. Cursor pin andrenderDiffare deferred to a single in-flighttea.Tick(30ms)debounce.wheelState.tickInFlightkeeps one tick alive at a time regardless of burst length:tickInFlight=truegen, no new tick scheduledhandleWheelDebouncechecks gen: matches → flush + cleartickInFlight; lags → reschedule for current genflushWheelPending()runs the deferred pin + render and is also called byhandleKey,handleResize, andhandleBlameLoadedso cursor-relative actions and anysyncViewportToCursorcallsite always see a fresh cursor.Measurement on the same burst as before
View()callsView()timeReverse-direction wheel now reacts within one Update cycle instead of queueing behind a backlog.
During a burst
The diff cursor index and the cursor highlight in the rendered string stay stale until burst end (30ms idle) or until a key/resize/blame-load triggers an early flush. Matches
less/vimbehavior: no visible cursor during fast scroll, snaps back once the burst settles.Commits
867026ainitial coalescing implementation1fa3985review-fix iteration 1:handleBlameLoadedflush, godoc accuracy, weak tests strengthened, three new flush-path tests67921fffinal cleanup:RendersOnMatchingGennow asserts viewport content changedf846bb9post-review:wheelStatedoc tightening, README mouse-support note, test helper hardening