Skip to content

fix(scroll): keep one staff-system row of lookahead visible - #24

Merged
byrongamatos merged 1 commit into
mainfrom
fix/scroll-lookahead
Jul 23, 2026
Merged

fix(scroll): keep one staff-system row of lookahead visible#24
byrongamatos merged 1 commit into
mainfrom
fix/scroll-lookahead

Conversation

@gionnibgud

@gionnibgud gionnibgud commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Restores the lookahead auto-scroll lost in the clean-relaunch snapshot (the fix existed as 6121009 on 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.

  • Page layout: when the next staff system's bottom is below the viewport, scroll immediately so the current row sits at ~25% from the top, revealing the next row.
  • Backward-seek guard (top 15% inset) and last-row centre fallback preserved.
  • Horizontal layout unchanged.

Study mode gets the same behaviour for free: _svStudySnapCursor drives 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.
  • Manual: play a multi-line page-layout chart; the next system is visible before the current row's last note.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved playback auto-scrolling to keep the current staff row visible and reveal the next row earlier.
    • Added smoother handling when navigating backward through the score.
    • Prevented unnecessary vertical scrolling in horizontal layouts.
    • Aligned scrolling behavior between normal playback and study mode.
    • Improved handling of the final staff row by avoiding attempts to reveal a nonexistent next row.

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)
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 02aa1610-e702-44f0-9281-3978e8ce295d

📥 Commits

Reviewing files that changed from the base of the PR and between 3cf5b38 and 1785b1d.

📒 Files selected for processing (2)
  • screen.js
  • tests/scroll.test.js

📝 Walkthrough

Walkthrough

_svUpdateMarker() now uses page-layout row lookahead for vertical auto-scrolling while preserving horizontal scrolling. New tests validate playback, study mode, edge cases, and the absence of study-mode gating.

Changes

Marker auto-scroll

Layer / File(s) Summary
Page-layout lookahead scrolling
screen.js
Vertical scrolling uses the next staff system row in page layout, with fallback handling for the last row; horizontal pad-based scrolling remains unchanged.
Playback and study-mode scroll validation
tests/scroll.test.js
Adds isolated harness tests for lookahead scrolling, last-row centering, backward seeks, horizontal layouts, study-mode equivalence, and source-level gating checks.

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
Loading

Possibly related PRs

Suggested reviewers: byrongamatos

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: preserving one staff-system row of lookahead during scroll behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/scroll-lookahead

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@byrongamatos
byrongamatos merged commit 2929344 into main Jul 23, 2026
3 checks passed
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants