fix(editor): the canvas re-fits when the chrome above it changes height - #328
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
resizeCanvas() derives the canvas pixel size — and, through setLaneMetrics(),
every lane height — from #editor-canvas-wrap's client box. It only ran on
WINDOW resize. Plenty of things change the wrap without touching the window:
entering Drum edit mode adds a toolbar row, toolbars wrap at narrow widths,
the inspector opens, the tracks pane resizes.
In those cases the canvas kept its old size and overhung the status bar, so
its bottom rows were covered and unclickable. Measured at 1600x660:
fresh editor wrap 456.6 canvas 457 bottom 635.4 ok
song loaded wrap 448.6 canvas 449 bottom 635.4 ok
drum edit wrap 418.6 canvas 449 bottom 665.4 PAST the 660 viewport
after:
drum edit wrap 418.6 canvas 419 bottom 635.4 ok
Worse than cosmetic: the lane metrics were being computed from that stale
height, so the geometry was sized for a canvas that no longer existed.
A ResizeObserver on the wrap catches every cause at once rather than hunting
call sites one at a time. It compares against the last size applied — the
canvas is a CHILD of the observed element and resizeCanvas writes its style
height, so the guard keeps the no-feedback-loop property local instead of
relying on the flex layout staying the way it is. Disconnected in the
teardown alongside the other observers, or it stacks one per re-injection,
each holding a resizeCanvas closure over a replaced DOM.
Found while measuring the drum-grid overflow for the vertical-scroll work: it
is why the bottom drum lanes were unreachable even after scrolling reached
them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
ChrisBeWithYou
force-pushed
the
fix/editor-canvas-refits-on-chrome-change
branch
from
July 19, 2026 21:25
2001148 to
475b2e0
Compare
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.
The bug
resizeCanvas()derives the canvas pixel size — and, viasetLaneMetrics(), every lane height — from#editor-canvas-wrap's client box. It only ran on window resize.But plenty of things change the space available to the canvas without touching the window: entering Drum edit mode adds a toolbar row, toolbars wrap at narrow widths, the inspector opens, the tracks pane resizes. In all those cases the canvas kept its old size and overhung the status bar — its bottom rows covered and unresponsive to clicks.
Measured at a 1600×660 viewport:
After: drum edit reads wrap 418.6 / canvas 419 / bottom 635.4.
Worse than cosmetic. The lane metrics were being computed from that stale height, so the geometry was sized for a canvas that no longer existed.
The fix
A
ResizeObserveron the wrap. That catches every cause at once — mode toolbars, toolbar wrapping, the inspector, the splitter — rather than hunting call sites one at a time and missing the next one.It compares against the last size applied. The canvas is a child of the observed element and
resizeCanvaswrites its style height, so while the wrap's height is decided by flex rather than content (and shouldn't re-fire), the guard keeps that property local instead of depending on the layout staying as it is.Disconnected in the teardown alongside
_v3LayoutObs/_v3TopbarWatch— without that they stack one per re-injection, each holding aresizeCanvasclosure over a replaced DOM.Verification
Browser-measured before and after at 1600×660, table above — the overhang is gone and the canvas tracks its slot through every state transition.
tests/canvas_refit.test.mjs(9 cases) brace-extracts the real observer and drives it with a fakeResizeObserver: observes the wrap; a changed wrap re-fits; an unchanged wrap does not (the loop guard); successive changes each re-fit; a width-only change re-fits; re-observing disconnects the previous one; a host withoutResizeObserverdegrades quietly; a missing wrap degrades quietly; and a source guard thatinit()calls it and the teardown disconnects it.All 9 fail on main. 227/227 pass with the fix; lint clean.
How I found it
Measuring the drum-grid overflow for the vertical-scroll work (#327). It explains something that had been bothering me there: the bottom drum lanes stayed unclickable even once scrolling brought them into view — they were under the status bar, on canvas that had fallen off the screen.
The two are independent and stand alone, but they compound: with this fix the drum canvas in edit mode is smaller (419px, correctly), so the lane overflow #327 handles is real more often, not less.
🤖 Generated with Claude Code
https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix