fix(editor): keyboard seeks scroll the view on a HiDPI display - #320
Merged
Conversation
canvas.width is DEVICE pixels — main.js sets it to cssWidth * DPR — but _editorSeekToTime mixed it straight into LABEL_W and S.zoom, which are both CSS-pixel units. On a 2x display that put the computed right edge a full viewport past the real one (an 800px-CSS canvas measured 12.9s of timeline instead of 6.2s), so seeking to a cursor that had visibly run off the right never fired the scroll branch, and the left-hand branch used a margin twice as wide as intended. The viewport is now read in CSS pixels once (canvas.width / DPR) and used for both the margin and the right edge — the same units the follow path in audio.js has always used. At DPR 1 the arithmetic is unchanged, so standard -density displays are byte-identical. Tests: tests/seek_viewport_dpr.test.mjs — installs a DPR-2 window before the module graph loads, then drives the real _editorSeekToTime. The past-the-right -edge and left-margin cases both fail on main; the inside-the-view and cursor -clamp cases pin the behaviour that must not move. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughKeyboard seek viewport boundaries now use CSS pixels derived from the canvas width and device pixel ratio. New HiDPI tests cover viewport duration, scrolling in both directions, unchanged in-view positions, and cursor clamping. ChangesHiDPI keyboard seeking
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 19, 2026
…ll in Play
Two things, from a look at how Logic/Ableton/Pro Tools handle the playhead
running off-screen during playback.
THE BUG (Christian: "doesn't respect the edge at different resolutions").
_followScrollTargetPure measured its 80% trigger and 30% landing against the
FULL canvas width, but read the cursor x from timeToX — which adds the 52px
LABEL_W gutter. So the usable timeline (viewW - 52) was never what it computed
against, and the effective landing fraction drifted with canvas width: 52px is
a bigger slice of an 800px canvas than a 1920px one. Real, but modest — a
constant ~16px, ~1-2% across resolutions, not a gross failure. Both the forward
follow and the loop-wrap recentre now measure against the usable width, so the
playhead lands at the same on-screen spot at every resolution.
Two things the DAW survey settled, worth recording:
- The DPR half of the hypothesis was WRONG for this path — both cursorX and
viewW are already CSS px (main.js scales the context by DPR). Adding a DPR
term here would REINTRODUCE a bug (that was #320's defect, in input.js).
Deliberately no DPR term.
- The dramatic "ignores the edge at some window sizes" symptom is most likely
a STALE canvas width, which #328 (the wrap ResizeObserver) fixes — not this
formula. This PR is the gutter-math polish; #328 is the other half.
THE FEATURE. Follow now has two manners, matching Logic: page-jump (default,
unchanged) and continuous "Scroll in Play" — the playhead pins at centre and
the timeline glides under it. All three DAWs converge on exactly this page-vs-
continuous choice and offer nothing more, so it's the whole customization worth
having; Logic's full Catch matrix would be over-engineering for one window.
Surfaced as a View-menu checkbox (not a second button, not a 3-state cycle —
the Follow button's aria-pressed has no honest third value), dimmed while
Follow is off, honouring prefers-reduced-motion by falling back to the page
jump. The Follow button's tooltip and the status line name the live manner.
The label-gutter edge is now a faint divider in the chart, so the usable
timeline reads as bounded and a future gutter slip is visible on sight. The
continuous pin is drawn from _followPinXPure with the SAME fraction the scroll
target uses, so the pin can never point where the scroll doesn't hold.
Follow is global (moves S.scrollX), so one fix covers every time-axis view;
Tab view (alphaTab owns its own cursor) is untouched, correctly.
Verified in a browser: continuous mode pins the playhead at x=628, spread 0px
across a 6s pass; catch lands at exactly 0.30 of the USABLE width at both a
606px and a 1206px canvas (the width-invariance); the gutter divider paints at
x=52 and the cyan pin at centre x=629; reduced-motion reads false; no page
errors.
The follow policy fns are now exported and tested by real import (superseding
the sliced follow_toggle / loop_wrap_follow suites). tests/follow_scroll.test.mjs
pins the width-invariance (the behavioural discriminator — the old inline
formula lands at LABEL_W + viewW*0.3, which varies with width), continuous
centring, the shared pin fraction, the loop-wrap recentre, and the tick wiring.
tests/menu_model.test.mjs covers the checkbox checked/dimmed states.
228 JS tests, lint clean. routes.py untouched (no pytest change).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
ChrisBeWithYou
added a commit
that referenced
this pull request
Jul 19, 2026
…ll in Play (#331) * feat(editor): follow respects the canvas edge, and gains Logic's Scroll in Play Two things, from a look at how Logic/Ableton/Pro Tools handle the playhead running off-screen during playback. THE BUG (Christian: "doesn't respect the edge at different resolutions"). _followScrollTargetPure measured its 80% trigger and 30% landing against the FULL canvas width, but read the cursor x from timeToX — which adds the 52px LABEL_W gutter. So the usable timeline (viewW - 52) was never what it computed against, and the effective landing fraction drifted with canvas width: 52px is a bigger slice of an 800px canvas than a 1920px one. Real, but modest — a constant ~16px, ~1-2% across resolutions, not a gross failure. Both the forward follow and the loop-wrap recentre now measure against the usable width, so the playhead lands at the same on-screen spot at every resolution. Two things the DAW survey settled, worth recording: - The DPR half of the hypothesis was WRONG for this path — both cursorX and viewW are already CSS px (main.js scales the context by DPR). Adding a DPR term here would REINTRODUCE a bug (that was #320's defect, in input.js). Deliberately no DPR term. - The dramatic "ignores the edge at some window sizes" symptom is most likely a STALE canvas width, which #328 (the wrap ResizeObserver) fixes — not this formula. This PR is the gutter-math polish; #328 is the other half. THE FEATURE. Follow now has two manners, matching Logic: page-jump (default, unchanged) and continuous "Scroll in Play" — the playhead pins at centre and the timeline glides under it. All three DAWs converge on exactly this page-vs- continuous choice and offer nothing more, so it's the whole customization worth having; Logic's full Catch matrix would be over-engineering for one window. Surfaced as a View-menu checkbox (not a second button, not a 3-state cycle — the Follow button's aria-pressed has no honest third value), dimmed while Follow is off, honouring prefers-reduced-motion by falling back to the page jump. The Follow button's tooltip and the status line name the live manner. The label-gutter edge is now a faint divider in the chart, so the usable timeline reads as bounded and a future gutter slip is visible on sight. The continuous pin is drawn from _followPinXPure with the SAME fraction the scroll target uses, so the pin can never point where the scroll doesn't hold. Follow is global (moves S.scrollX), so one fix covers every time-axis view; Tab view (alphaTab owns its own cursor) is untouched, correctly. Verified in a browser: continuous mode pins the playhead at x=628, spread 0px across a 6s pass; catch lands at exactly 0.30 of the USABLE width at both a 606px and a 1206px canvas (the width-invariance); the gutter divider paints at x=52 and the cyan pin at centre x=629; reduced-motion reads false; no page errors. The follow policy fns are now exported and tested by real import (superseding the sliced follow_toggle / loop_wrap_follow suites). tests/follow_scroll.test.mjs pins the width-invariance (the behavioural discriminator — the old inline formula lands at LABEL_W + viewW*0.3, which varies with width), continuous centring, the shared pin fraction, the loop-wrap recentre, and the tick wiring. tests/menu_model.test.mjs covers the checkbox checked/dimmed states. 228 JS tests, lint clean. routes.py untouched (no pytest change). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix * Describe the effective follow behavior * Avoid stacked changelog conflicts * Hide the follow pin when follow is off * Normalize follow guide lines --------- Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
canvas.widthis device pixels —main.js:1407sets it tocssWidth * DPR— but_editorSeekToTimemixed it straight intoLABEL_WandS.zoom, which are both CSS-pixel units.On a 2× display that put the computed right edge a full viewport past the real one: an 800px-CSS canvas measured 12.9s of timeline instead of 6.2s. So seeking to a cursor that had visibly run off the right never fired the scroll branch, and the left-hand branch used a margin twice as wide as intended.
The follow path in
audio.js:936has always divided byDPR. This makes the seek path use the same viewport.Repro (on main, at DPR 2)
Tests
tests/seek_viewport_dpr.test.mjsinstalls a DPR-2windowbefore the module graph loads, then drives the real_editorSeekToTime. On main:seeking past the right edge scrolls the view on a 2x displayseeking left of the view scrolls back by a CSS-pixel marginThe inside-the-view and cursor-clamp cases pass on main and pin what must not move.
Notes
At DPR 1 with a canvas present the arithmetic is unchanged, so standard-density displays are byte-identical. The no-canvas fallback (tests only) is unified — the old code fell back to
10for the margin and800for the edge, which disagreed with each other.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation