fix(editor): horizontal scroll works before the first Play in compose/MIDI-only sessions - #307
Conversation
…/MIDI-only sessions Tester report: "I couldn''t scroll left/right on the note view" — then it mysteriously fixed itself. Root cause: a session with no decoded audio only derives S.duration from the grid inside startPlayback(), so before the first Play _editorClampScrollX saw duration 0, maxScroll clamped to 0, and every horizontal-scroll input (wheel, middle-drag pan, and the minimap — whose own songDur() knows the grid length but funnels through the same clamp) silently pinned the view to t = 0. The clamp now falls back to _composeSongDuration() — the same grid/content rule playback itself uses (charrette §1.7) — whenever the audio-derived duration is not positive. Guarded so audio-bounded sessions never pay for the content walk and keep their exact old clamp. Tests: tests/compose_scroll_clamp.test.mjs — the un-played MIDI-only leg fails on main (clamps to 0); content-past-grid extent; empty session still clamps; audio-bounded clamp byte-identical (fallback dormant). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
Warning Review limit reached
Next review available in: 59 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. 📝 WalkthroughWalkthroughHorizontal scroll clamping now uses composed song/grid duration when audio duration is unavailable, enabling pre-play scrolling in MIDI-only sessions. Audio-bounded behavior remains unchanged, with new tests covering the affected scenarios. ChangesMIDI scroll clamping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/loop.js (1)
42-52: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid rescanning authored content on every scroll clamp.
When the audio duration is non-positive,
_editorClampScrollXcalls_composeSongDuration(), which walks authored content. Because this function serves wheel, drag, and minimap paths, large MIDI sessions may perform an O(content) scan per input/frame. Cache the composed extent and invalidate it when beats, notes, guides, orcomposeLengthchange.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/loop.js` around lines 42 - 52, The fallback in _editorClampScrollX must not rescan authored content on every scroll event. Cache the value returned by _composeSongDuration(), reuse it while beats, notes, guides, and composeLength are unchanged, and invalidate or recompute that cache whenever any of those inputs changes; preserve the existing audio-duration path without invoking the content scan.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/loop.js`:
- Around line 42-52: The fallback in _editorClampScrollX must not rescan
authored content on every scroll event. Cache the value returned by
_composeSongDuration(), reuse it while beats, notes, guides, and composeLength
are unchanged, and invalidate or recompute that cache whenever any of those
inputs changes; preserve the existing audio-duration path without invoking the
content scan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f52f14b9-24a9-40de-8d3a-587a33ff6071
📒 Files selected for processing (3)
CHANGELOG.mdsrc/loop.jstests/compose_scroll_clamp.test.mjs
# Conflicts: # CHANGELOG.md
2 participants
The bug (tester report: "I couldn''t scroll left/right on the note view")
A session with no decoded audio only derives
S.durationfrom the grid insidestartPlayback()(audio.js:800). Before the first Play,_editorClampScrollXtherefore saw a 0-length song,_editorMaxScrollXPurereturned 0 (geometry.js:81), and every horizontal-scroll input — wheel, middle-drag pan, and the minimap (whose ownsongDur()knows the grid length but funnels through the same clamp atruler.js:457) — silently pinned the view tot = 0. Pressing Play once set the duration and "cured" it, which is exactly why the report came in as "scroll issues" rather than a repro.The fix
_editorClampScrollXfalls back to_composeSongDuration()— the same grid/authored-content rule playback itself uses (charrette §1.7) — whenever the audio-derived duration is not positive. The fallback is guarded so audio-bounded sessions never pay for the content walk and keep their exact old clamp.Tests
tests/compose_scroll_clamp.test.mjs— the un-played MIDI-only leg fails on main (clamps to 0); authored content past the grid extends the extent; a genuinely empty session still clamps to 0; the audio-bounded clamp is byte-identical with the fallback provably dormant (a longer grid cannot extend it). Full suite: 205 JS green, lint 0 errors.🤖 Generated with Claude Code
Summary by CodeRabbit