Study mode — note-by-note gated practice - #15
Merged
Conversation
Study mode pauses the OGG at each gate (a beat's required notes) and only resumes once they are all played correctly on MIDI. - Gates built from the judge notes: both-hands collapses same-beat treble + bass into one gate (satisfied in any order); hand isolation builds per-hand gates and rebuilds on switch. - Wrong notes draw an orange X at the pitch actually played, with clef-aware ledger lines. Sequential wrong attempts replace; simultaneous held wrong notes accumulate. Marks clear on gate advance and on seek. - Cursor snaps to the current gate beat (not audio time) while active; _svSyncCursor is suppressed. Platform/click seeks re-home the gate. - Optional preroll count-in (one bar of metronome clicks, persisted); its AudioContext is closed on teardown. - Judged notes flow through the same core note-detection / stats channels as free-play (note:hit / note:miss, reportHit / reportMiss). - _svMidiToDiatonic extracted as a pure, unit-tested helper. Ported from the legacy tip (fixes folded in), rewired to current main's namespace, pill, and shared judge-reset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@screen.js`:
- Around line 3033-3046: The Study mode resume path in _svStudyAdvance can get
stuck because audio.play() may reject asynchronously, while the current
try/catch only handles synchronous errors. Update _svStudyAdvance to handle the
Promise returned by audio.play() and clear or recover _svPrerollResuming when
playback fails, and apply the same rejection handling pattern in the preroll
onDone callback so both resume paths behave consistently.
- Around line 3447-3475: The Study mode audio gating in the draw loop is acting
on the shared audio element from every instance, which can cause split-screen
panels to fight over play/pause and gate timing. Update the Study-mode branch in
draw() to run only for the focused/active instance by checking _svIsFocused or
_svActiveInst before touching `#audio`, _svStudyGateTime, or _svPrerollResuming,
and apply the same guard in the related seeked handling so only one Study
controller can manage the track at a time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 316ad4c8-bf0a-4e14-a771-daadc06de658
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/study.test.js
Comment on lines
+3033
to
+3046
| function _svStudyAdvance() { | ||
| _svStudyWrongAttempts.delete(_svStudyGateIdx); | ||
| _svRedrawAllMissDots(); | ||
| _svStudyChordHit.clear(); | ||
| _svStudyGateIdx++; | ||
| _svStudyGateTime = _svStudyGateIdx < _svStudyGates.length | ||
| ? _svStudyGates[_svStudyGateIdx].gateTime : null; | ||
| _svStudySnapCursor(); | ||
| _svPrerollResuming = true; // gate advance — no preroll on resume | ||
| try { | ||
| const audio = document.getElementById('audio'); | ||
| if (audio && audio.paused) audio.play(); | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unhandled audio.play() rejection can leave Study mode silently stuck paused.
audio.play() returns a Promise that can reject (autoplay-policy changes, decode error, etc.). The try/catch here only catches synchronous throws, not promise rejections — if play() rejects after a gate is satisfied, playback never resumes, the user gets no feedback, and _svPrerollResuming is left true forever (silently disabling the next natural preroll trigger too). The same pattern recurs in the preroll onDone callback around line 3463.
🔧 Proposed fix
_svPrerollResuming = true; // gate advance — no preroll on resume
try {
const audio = document.getElementById('audio');
- if (audio && audio.paused) audio.play();
+ if (audio && audio.paused) {
+ const p = audio.play();
+ if (p && typeof p.catch === 'function') {
+ p.catch(() => { _svPrerollResuming = false; });
+ }
+ }
} catch (_) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function _svStudyAdvance() { | |
| _svStudyWrongAttempts.delete(_svStudyGateIdx); | |
| _svRedrawAllMissDots(); | |
| _svStudyChordHit.clear(); | |
| _svStudyGateIdx++; | |
| _svStudyGateTime = _svStudyGateIdx < _svStudyGates.length | |
| ? _svStudyGates[_svStudyGateIdx].gateTime : null; | |
| _svStudySnapCursor(); | |
| _svPrerollResuming = true; // gate advance — no preroll on resume | |
| try { | |
| const audio = document.getElementById('audio'); | |
| if (audio && audio.paused) audio.play(); | |
| } catch (_) {} | |
| } | |
| function _svStudyAdvance() { | |
| _svStudyWrongAttempts.delete(_svStudyGateIdx); | |
| _svRedrawAllMissDots(); | |
| _svStudyChordHit.clear(); | |
| _svStudyGateIdx++; | |
| _svStudyGateTime = _svStudyGateIdx < _svStudyGates.length | |
| ? _svStudyGates[_svStudyGateIdx].gateTime : null; | |
| _svStudySnapCursor(); | |
| _svPrerollResuming = true; // gate advance — no preroll on resume | |
| try { | |
| const audio = document.getElementById('audio'); | |
| if (audio && audio.paused) { | |
| const p = audio.play(); | |
| if (p && typeof p.catch === 'function') { | |
| p.catch(() => { _svPrerollResuming = false; }); | |
| } | |
| } | |
| } catch (_) {} | |
| } |
🤖 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 `@screen.js` around lines 3033 - 3046, The Study mode resume path in
_svStudyAdvance can get stuck because audio.play() may reject asynchronously,
while the current try/catch only handles synchronous errors. Update
_svStudyAdvance to handle the Promise returned by audio.play() and clear or
recover _svPrerollResuming when playback fails, and apply the same rejection
handling pattern in the preroll onDone callback so both resume paths behave
consistently.
Comment on lines
+3447
to
+3475
|
|
||
| // Study mode: preroll count-in on manual play, and gate-pause when | ||
| // the OGG reaches the next required note. | ||
| try { | ||
| const audio = document.getElementById('audio'); | ||
| if (audio) { | ||
| const nowPaused = audio.paused; | ||
| if (_svWasAudioPaused === null) { | ||
| _svWasAudioPaused = nowPaused; // first observation only | ||
| } else if (!nowPaused && _svWasAudioPaused) { | ||
| // paused → playing: if the user pressed play, do a preroll. | ||
| if (_svStudyMode && _svPrerollEnabled | ||
| && !_svPrerollResuming && !_svStudyCountingDown) { | ||
| audio.pause(); | ||
| _svStudyStartCountdown(() => { | ||
| _svPrerollResuming = true; | ||
| try { audio.play(); } catch (_) {} | ||
| }); | ||
| } | ||
| _svPrerollResuming = false; | ||
| } | ||
| if (_svStudyMode && !_svStudyCountingDown && !audio.paused | ||
| && _svStudyGateTime !== null | ||
| && audio.currentTime >= _svStudyGateTime) { | ||
| audio.pause(); | ||
| } | ||
| _svWasAudioPaused = audio.paused; | ||
| } | ||
| } catch (_) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how splitscreen mounts/mixes multiple staffview instances and whether
# Study mode is expected to be usable on more than one panel simultaneously.
rg -n "_ssActiveFull|feedBackSplitscreen|slopsmithSplitscreen" screen.js -A3 -B3Repository: got-feedBack/feedBack-plugin-staffview
Length of output: 2975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the study-mode helpers and lifecycle around the draw loop.
ast-grep outline screen.js --match '_svStudy' --view expanded || true
rg -n "_svStudy|_svInstances|_svActiveInst|_svIsFocused|_svStudyAttachSeekHandler|_onFocusChange|draw\\(" screen.js -A4 -B4
# Inspect the relevant sections with line numbers.
sed -n '3300,3520p' screen.js | cat -nRepository: got-feedBack/feedBack-plugin-staffview
Length of output: 34519
Gate Study mode to the focused instance. The shared #audio element is driven from every instance’s draw() loop and seeked handler. In split-screen, two active Study panels can pause/resume or re-home the same track against different gate lists. Restrict this path to _svIsFocused/_svActiveInst, or make Study mode singleton per tab.
🤖 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 `@screen.js` around lines 3447 - 3475, The Study mode audio gating in the draw
loop is acting on the shared audio element from every instance, which can cause
split-screen panels to fight over play/pause and gate timing. Update the
Study-mode branch in draw() to run only for the focused/active instance by
checking _svIsFocused or _svActiveInst before touching `#audio`, _svStudyGateTime,
or _svPrerollResuming, and apply the same guard in the related seeked handling
so only one Study controller can manage the track at a time.
… freeze) A single note-on now clears every charted notehead at that pitch (unison / octave-double / both-hands same beat), so the gate no longer freezes forever waiting on a duplicate that can never get its own press; adds unison-gate regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Integrate study mode with chain-A explorer/loop + score HUD; keep main's velocity-passing _svMonitorNoteOn call (the #13 fix) alongside study's gate-judging branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 17, 2026
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
2 participants
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.
Study mode — note-by-note gated practice (PR6)
The OGG pauses at each gate (a beat's required notes) and only resumes once
they are all played correctly on MIDI. Reuses PR4's detection + monitor synth
and PR5's judging/stats.
What's here
STUDYtoggle in the options pill. Gates are built from thejudge notes: both-hands collapses same-beat treble + bass into one gate
(satisfied in any order); hand isolation builds per-hand gates and
rebuilds on switch.
clef-aware ledger lines. Sequential wrong attempts replace each other;
simultaneous held wrong notes accumulate. Marks clear on gate advance / seek.
_svSyncCursoris suppressed. Platform/click seeks re-home the gate.resumes; the metronome
AudioContextis closed on teardown.note:hit/note:miss+reportHit/reportMisspath as free-play, soscoring shows in core's HUD / dashboard / song_stats.
Notes for review
core note-detection path as
keys_highway_3d(shared infra) rather than aprivate scorer — worth a look given the piano overlap.
current main's
window.feedBacknamespace, the current pill, and PR5'sshared
_svResetJudgeState. Dead alphaSynth/slopsmithlegacy paths dropped._svMidiToDiatonicextracted as a pure, unit-tested helper(
tests/study.test.js).Verification
the v3 UI (grand-staff): gating advances only on correct notes, wrong-X lands
at the played pitch incl. ledger lines, both-hands vs isolated gates, preroll
count-in (single-fire, no count-in on gate advance), seek re-homes, and clean
teardown (no lingering marks, AudioContext closed).
npm testgreen (54/54).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests