fix(editor): the drum grid scrolls vertically — kick and snare reachable again - #327
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 23 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 (8)
✨ 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 |
ChrisBeWithYou
force-pushed
the
fix/editor-lane-vertical-scroll
branch
from
July 19, 2026 18:01
b868975 to
5fa1515
Compare
This was referenced Jul 19, 2026
…ble again DRUM_LANE_H is a fixed 22px and _drumLaneIdxToY had no scroll term, so an 18-piece kit (396px of lanes) ran straight off the bottom of a short canvas with nothing to reach it. DRUM_PIECE_ORDER ends `snare, snare_xstick, kick`, so the first things to vanish were the two most-used pieces on the kit. Measured in a browser at a 1600x660 window: a 449px canvas with 356px of lane band. The bottom two rows were simply gone. New src/lane-scroll.js owns the model, mirroring the horizontal one: one offset (S.laneScrollY, in PIXELS — a lane stack has no natural time unit), one clamp every writer passes through, and one geometry funnel per view. _drumLaneIdxToY/_drumYToLaneIdx ARE that funnel — 29 call sites across 7 modules go through them, so the offset lands in one place and reaches every painter and hit-test at once. The accessor sits in geometry.js rather than lane-scroll.js purely to break a cycle: lane-scroll.js imports drum.js to answer "which view is showing", so drum.js cannot import it back. The bar auto-hides when the content fits — Logic shows the Tracks area's vertical scroll bar only when there are more tracks than fit (p.297). It is painted on the canvas, not added as a DOM rail: a layout-participating rail would change #editor-canvas-wrap's client size, which feeds setLaneMetrics(), so the lane heights would shift the moment the bar appeared. Wheel behaviour is strictly additive. laneScrollBy() returns false when there is nothing to scroll, so a grid that already fits falls straight through to the long-standing horizontal pan — unchanged for anyone whose window fits. Bounds are re-clamped on resize and reset on a density switch (Full's 18 rows to Compact's 7 would otherwise leave the grid scrolled past its own end). Verified in a browser: the grid scrolls, and Snare/Sn(x) — off-screen before — come into view; the scrollbar renders; the clip keeps scrolled lanes off the ruler and waveform; no page errors. NOT verified end-to-end: clicking to author a hit, because the control case (clicking an always-visible lane in the same empty-tab state) was equally inert, so that measurement said nothing either way. The scrolled hit-test is covered by a round-trip unit test on the real geometry instead. 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-lane-vertical-scroll
branch
from
July 19, 2026 21:24
5fa1515 to
9ac57e3
Compare
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 19, 2026
STACKED ON #327 (fix/editor-lane-vertical-scroll) — it needs the shared lane-scroll model that PR introduces. Merge #327 first; this diff shows only its own changes against that branch. The roll never overflowed because it SQUASHED instead. Lane height was derived and nothing could touch it — max(4, min(14, 350 / range)) — packing any pitch range into ~350px. A wide range collapsed to 4px per semitone: passable for reading, useless for editing, with no way out. That is exactly why #327 left the roll alone and fixed only the drum grid's reachability. That derived value is now the DEFAULT. S.rollLaneH is a user override, and the vertical scrolling shared with the drum grid is what makes a taller-than- viewport roll usable — the two halves only work together, which is why they land together. Alt+wheel is Live's binding for key-track zoom (Live 12 manual p.153, p.240); Logic exposes the same as a Vertical Zoom slider (p.297). Checked BEFORE the ctrl branch so Alt+Ctrl can't land on horizontal zoom while the user means vertical, and gated on isKeysMode() so every other view keeps today's gestures untouched. midiToY/yToMidi take the scroll term, mirroring _drumLaneIdxToY's pair — every painter and hit-test funnels through them, so one edit reaches all of them. _beatBarTopY() follows the roll's scroll: the anchor, handshape and beat-bar strips annotate the chart, so they ride with the note content rather than floating over it. The string view has no vertical scroll, so laneScrollY() is 0 there and that expression is unchanged. The lane-band clip is pushed only when the view actually scrolls, so the string view's pixels are untouched — clipping it unconditionally would be a silent behaviour change for the common case. It is released in the `finally`, not after the last painter: a throw in between would otherwise leave the context one save deep and the outer restore would pop the wrong level. Song load resets both the override and the scroll offset, before updatePianoRange so it re-derives the auto height instead of honouring a stale override from the previous arrangement's range. Verified in a browser on a real song, roll view: the scrollbar goes from ABSENT to 158px of thumb after stretching — and it only ever paints when content exceeds the viewport, so its appearance is proof the lanes grew. Plain wheel then scrolls. Waveform and ruler stay clean above the clip. Status reads "Roll lane height 23.6px — Alt+scroll to stretch/compact". 249 JS tests, 346 pytest, lint clean. 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
* feat(editor): the piano roll stretches, compacts and scrolls STACKED ON #327 (fix/editor-lane-vertical-scroll) — it needs the shared lane-scroll model that PR introduces. Merge #327 first; this diff shows only its own changes against that branch. The roll never overflowed because it SQUASHED instead. Lane height was derived and nothing could touch it — max(4, min(14, 350 / range)) — packing any pitch range into ~350px. A wide range collapsed to 4px per semitone: passable for reading, useless for editing, with no way out. That is exactly why #327 left the roll alone and fixed only the drum grid's reachability. That derived value is now the DEFAULT. S.rollLaneH is a user override, and the vertical scrolling shared with the drum grid is what makes a taller-than- viewport roll usable — the two halves only work together, which is why they land together. Alt+wheel is Live's binding for key-track zoom (Live 12 manual p.153, p.240); Logic exposes the same as a Vertical Zoom slider (p.297). Checked BEFORE the ctrl branch so Alt+Ctrl can't land on horizontal zoom while the user means vertical, and gated on isKeysMode() so every other view keeps today's gestures untouched. midiToY/yToMidi take the scroll term, mirroring _drumLaneIdxToY's pair — every painter and hit-test funnels through them, so one edit reaches all of them. _beatBarTopY() follows the roll's scroll: the anchor, handshape and beat-bar strips annotate the chart, so they ride with the note content rather than floating over it. The string view has no vertical scroll, so laneScrollY() is 0 there and that expression is unchanged. The lane-band clip is pushed only when the view actually scrolls, so the string view's pixels are untouched — clipping it unconditionally would be a silent behaviour change for the common case. It is released in the `finally`, not after the last painter: a throw in between would otherwise leave the context one save deep and the outer restore would pop the wrong level. Song load resets both the override and the scroll offset, before updatePianoRange so it re-derives the auto height instead of honouring a stale override from the previous arrangement's range. Verified in a browser on a real song, roll view: the scrollbar goes from ABSENT to 158px of thumb after stretching — and it only ever paints when content exceeds the viewport, so its appearance is proof the lanes grew. Plain wheel then scrolls. Waveform and ruler stay clean above the clip. Status reads "Roll lane height 23.6px — Alt+scroll to stretch/compact". 249 JS tests, 346 pytest, lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix * Keep roll controls and annotation lanes reachable --------- 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
DRUM_LANE_His a fixed 22px and_drumLaneIdxToYhad no scroll term, so an 18-piece kit (396px of lanes) ran straight off the bottom of a short canvas with nothing to reach it.DRUM_PIECE_ORDERendssnare, snare_xstick, kick— so the first pieces to vanish were the kick and both snares. Not "awkward to reach": gone, with no gesture that could bring them back.Measured in a browser at a 1600×660 window: a 449px canvas with 356px of lane band, losing the bottom two rows outright.
The model
New
src/lane-scroll.js, mirroring the horizontal story exactly — one offset (S.laneScrollY, in pixels; a lane stack has no natural time unit), one clamp every writer passes through, and one geometry funnel per view._drumLaneIdxToY/_drumYToLaneIdxare that funnel: 29 call sites across 7 modules go through them, so the offset lands in one place and reaches every painter and hit-test at once. Nothing else had to learn about scrolling.The bar auto-hides. Logic shows the Tracks area's vertical scroll bar only when there are more tracks than fit (Logic Pro user guide, p.297). Same here — no overflow, no bar.
It's painted on the canvas, not a DOM rail. A layout-participating rail would change
#editor-canvas-wrap's client size, which feedssetLaneMetrics()— so every lane height would shift the moment the bar appeared, moving the very content it measures.The wheel change is strictly additive.
laneScrollBy()returnsfalsewhen there's nothing to scroll, so a grid that already fits falls straight through to the long-standing horizontal pan. Anyone whose window fits sees no change at all.Bounds are re-clamped on resize and reset on a density switch — Full's 18 rows → Compact's 7 would otherwise leave the grid scrolled past its own end.
Verification
Unit —
tests/lane_scroll.test.mjs(15 cases) runs against the real_drumLaneIdxToY, not a copy: kick-is-last, the off-screen case, reachability at max scroll, scrolled hit-test round-trip, unscrolled geometry unchanged, clamping, degenerate metrics, thumb sizing/flooring, thumb-reaches-bottom, drag inversion, overscroll clamp.The load-bearing one fails on main with:
Browser — the grid scrolls; Snare and Sn(x), off-screen before, come into view; the scrollbar renders; the clip keeps scrolled lanes off the ruler and waveform; no page errors.
What I did not verify, honestly: clicking to author a hit on a scrolled lane. I tried, got no pixel change — then ran the control (clicking an always-visible lane in the same empty-drum-tab state) and got no change either. So the click path is inert in that state for unrelated reasons and my measurement said nothing either way. Rather than report a result I hadn't earned, the scrolled hit-test is covered by the round-trip unit test on the real geometry instead. Worth a manual click-check on a song with real drum content.
233/233 JS tests pass; lint clean (3 pre-existing warnings in
create.js/main.js).Scope — deliberately drums only
The piano roll is a follow-up, not an oversight. Its content is at least always reachable (it squashes to a 4px lane rather than overflowing), and unlike the drum grid it shares the main draw chain with the string view and has the anchor + handshape lanes pinned below it via
_beatBarTopY(). Making that stack scroll-aware is a bigger change than this reachability fix and doesn't belong bundled with it.The string/tab view doesn't need this: its lanes auto-fit via
setLaneMetrics(), and at a 30px floor even a 9-string instrument fits every realistic canvas. The tempo map has no lane stack at all.Separate thing spotted
At 1600×660 the canvas measures
top=216 h=449 bottom=665against a 660px viewport — it overhangs the window by ~5px, and the status bar overlays its bottom edge. Unrelated to this fix, not addressed here.🤖 Generated with Claude Code
https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix