fix(scroll): keep one staff-system row of lookahead visible - #24
Merged
Conversation
Previously the auto-scroll in _svUpdateMarker triggered only when the cursor marker reached within padY of the viewport bottom, then jumped to center the marker. This left no lookahead: the next row was invisible until the very last note on the current row played. New behaviour (page layout only): - Locate the next staff system after the current beat's row. - If that system's bottom is below the viewport, scroll immediately so the current row sits at ~25% from the top — revealing the next row in the remaining 75% of the viewport. - Backward-seek guard: if the marker is within the top 15% of the viewport, scroll up to the 15% inset (handles seek-to-beginning). - Last-row fallback: if there is no next system and the marker nears the bottom edge, centre it (original behaviour). - Horizontal layout mode is unchanged (single continuous row; the Y axis is not a scrolling dimension). tests/scroll.test.js covers the four branches and asserts study mode scrolls identically: _svStudySnapCursor drives the same _svUpdateMarker, so the lookahead must not be gated on study mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com> (cherry picked from commit 612100971a2215ebc886b12786f835d86e68d2a3)
|
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 (2)
📝 WalkthroughWalkthrough
ChangesMarker auto-scroll
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Playback
participant _svUpdateMarker
participant _svApi
participant _svContainer
Playback->>_svUpdateMarker: Update marker for target row
_svUpdateMarker->>_svApi: Find current and next staff bounds
_svApi-->>_svUpdateMarker: Return row geometry
_svUpdateMarker->>_svContainer: Scroll to calculated position
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
carochacs
pushed a commit
to get-flashbacks/feedBack-plugin-staffview
that referenced
this pull request
Aug 13, 2026
…en compliance Review feedback on PR #2. The 0.4.0 cut missed commit 2929344 (got-feedBack#24, "keep one staff-system row of lookahead visible"), a user-facing auto-scroll change that landed 2026-07-23 — after the 0.3.0 release. Every other post-0.3.0 functional commit was recorded; this one slipped, which is the same defect class the release itself exists to close. Compliance workflow, same fixes as the sibling repos: diff against the merge base rather than the base tip (a stale branch was being failed for a bump it did not owe), require the [Unreleased] section itself to change rather than merely touching CHANGELOG.md, count .css as functional source, and record the guard job's known limitation. The [Unreleased] check would have caught this PR's own subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B4NYejYW2dW7dNtYh7srDy
carochacs
added a commit
to get-flashbacks/feedBack-plugin-staffview
that referenced
this pull request
Aug 13, 2026
Version Bump: Upgraded from 0.3.0 to 0.4.0 in package.json and plugin.json to trigger plugin manager update detection.
SemVer Justification: Minor version chosen over patch (0.3.1) due to new functionality (Metronome feature) and a breaking change/feature removal (study-mode preroll count-in removed).
Unrecorded Fixes Backfilled:
2883dfa: Added URL encoding fix for _buildWsUrl to prevent reserved characters ('&', '#', '?') in filenames from breaking arrangement parameters or truncating paths.
2929344 (got-feedBack#24): Added auto-scroll fix keeping one staff-system row of lookahead visible.
CI Workflow Enhancements: Introduced targeted compliance checks: version-bumped-on-change, changelog-updated, and idempotent-top-level-guard.
CI Compliance Hardening:
Evaluates git diffs against the merge base rather than main branch tip to avoid false positives on stale branches.
Verifies changes specifically targeting the [Unreleased] changelog section.
Expanded functional source classification to include .css files.
Technical Evaluation
Semantic Versioning Logic: The rationale for opting for a minor bump (0.4.0) rather than a patch (0.3.1) aligns with Semantic Versioning 2.0.0 guidelines, as removing existing functionality (preroll count-in) and introducing new capabilities (Metronome) alters the public API/behavior footprint.
URL Construction Hardening: The fix in _buildWsUrl addresses improper URI handling where unencoded query parameter delimiters ('&', '#', '?') alter URI routing structure. Applying encodeURIComponent post-decoding resolves parameter injection and truncation vulnerabilities.
CI Diff Strategy: Calculating diffs against git merge-base is standard practice for continuous integration to isolate pull request commits from upstream divergence.
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.
Summary
Restores the lookahead auto-scroll lost in the clean-relaunch snapshot (the fix existed as
6121009on the legacy history but predates the relaunch, so it never carried over). Reported as "chart not scrolling" — actually: no lookahead, the next row stayed hidden until the last note of the current row.Study mode gets the same behaviour for free:
_svStudySnapCursordrives the same_svUpdateMarker, and the lookahead is deliberately not gated on_svStudyMode— covered by test.Test plan
tests/scroll.test.js: 4 branch tests + study/playback scroll-equivalence; verified non-vacuous (pre-fix code fails the lookahead test).npm test: 122/122.🤖 Generated with Claude Code
Summary by CodeRabbit