feat(coaching): practice/coaching plugin suite (5 additive plugins) - #1051
Open
ahonnecke wants to merge 1 commit into
Open
feat(coaching): practice/coaching plugin suite (5 additive plugins)#1051ahonnecke wants to merge 1 commit into
ahonnecke wants to merge 1 commit into
Conversation
…ock, mute_master, pitch_match, practice_journal) Five self-contained, additive plugins ported onto feedBack's own contracts (window.feedBack bus + the native minigames framework — gamekit was NOT ported, it's redundant with the minigames plugin): - coaching — real-time coaching feedback + skill-drill panel - metronome_lock — timing-lock minigame - mute_master — muting/dynamics minigame - pitch_match — intonation minigame - practice_journal— per-song practice metrics dashboard (has routes.py) All 5 register cleanly on current upstream (backend + practice_journal routes load with no errors); coaching's suite passes 66/66 node tests. New plugin dirs only — no core changes, so it can't conflict with core churn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds an event-driven coaching pipeline with optional Anthropic summaries, three skill-drill plugins, and a Practice Journal that records sessions and displays aggregate and per-song statistics. ChangesCoaching feedback pipeline
Pitch-based skill drills
Practice Journal
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
plugins/coaching/test/coach.test.js (1)
100-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTautological override assertion — the override value equals the default.
COACH_MODELisclaude-haiku-4-5, so this passes even ifopts.modelwere ignored. Same issue at lines 190-191, wheresetModel('claude-haiku-4-5')is indistinguishable from a no-op. Use a distinct string.💚 Proposed fix
- assert.equal(buildCoachRequest(sampleFeedback(), { model: 'claude-haiku-4-5' }).model, 'claude-haiku-4-5'); + assert.equal(buildCoachRequest(sampleFeedback(), { model: 'test-model-override' }).model, 'test-model-override');🤖 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 `@plugins/coaching/test/coach.test.js` around lines 100 - 101, Update the model override assertions in buildCoachRequest and setModel tests to use a model string distinct from the COACH_MODEL default, so they fail when the override is ignored. Keep asserting that the returned/requested model equals the distinct override value.plugins/coaching/tools/headless-emit-check.js (1)
1-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHeader comment contradicts the implementation;
SONG/ARRare dead config.The script never loads a song or plays anything — it dispatches synthetic
notedetect:*events (as the line 19-20 comment correctly states).SONGandARRare parsed and never used, and thewaitForFunctiononwindow.playSong(line 31) is likewise vestigial. Trim the unused env plumbing and fix the header so the tool's contract is unambiguous.🤖 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 `@plugins/coaching/tools/headless-emit-check.js` around lines 1 - 16, The header comment for the headless check incorrectly claims it loads and plays a real song; update it to describe the synthetic notedetect event flow. Remove the unused SONG and ARR environment parsing, and remove the vestigial waitForFunction targeting window.playSong while preserving the actual Playwright check behavior.plugins/coaching/README.md (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced block (MD040).
📝 Proposed fix
-``` +```text notedetect:hit/miss ──┐🤖 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 `@plugins/coaching/README.md` at line 14, Add the text language identifier to the fenced code block containing the notedetect example in the coaching README, changing the opening fence to use text while preserving the example content.Source: Linters/SAST tools
plugins/coaching/coaching.js (1)
1516-1519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
settingsin_gatherSessionMeta— it shadows the module-scope settings object.The outer
settings(line 1366) is the localStorage-backed coach settings; the local here is note_detect's diagnostic settings. Same identifier, different meaning, in nested scope.♻️ Suggested rename
function _gatherSessionMeta(agg) { const nd = window.noteDetect; - let settings = null, song = agg.song || null, arrangement = agg.arrangement || null; + let ndSettings = null, song = agg.song || null, arrangement = agg.arrangement || null; let recording = null, tuning = agg.tuning || null, capo = agg.capo; try { if (nd && nd.getDiagnostic) { const d = nd.getDiagnostic() || {}; - settings = d.settings || null; + ndSettings = d.settings || null;…and update the returned object at line 1533 to
settings: ndSettings.🤖 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 `@plugins/coaching/coaching.js` around lines 1516 - 1519, Rename the local settings variable in _gatherSessionMeta to ndSettings to avoid shadowing the module-scope settings object, and update all references within the function, including the returned object’s settings property, to use ndSettings.plugins/practice_journal/screen.js (1)
106-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
beforeunloadhandler usesfetch, which can be cancelled mid-flight.Browsers may abort in-flight
fetchrequests during page unload, so the last session on tab-close can silently fail to record.navigator.sendBeaconis the standard mechanism for reliable unload-time requests.🤖 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 `@plugins/practice_journal/screen.js` around lines 106 - 108, Update the beforeunload flow around _pjEndSession to use navigator.sendBeacon for the final session-recording request instead of fetch, preserving the existing endpoint and payload and ensuring the unload handler remains non-blocking.plugins/practice_journal/routes.py (1)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse timezone-aware UTC timestamps instead of
datetime.utcnow().
datetime.utcnow()is deprecated in Python 3.12; this route also uses it at line 84. Replace both calls withdatetime.now(timezone.utc).replace(tzinfo=None)or the project’s preferred timezone-aware type to avoid future deprecation/removal issues.🤖 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 `@plugins/practice_journal/routes.py` at line 70, Replace both datetime.utcnow() calls in the route handling this data, including the defaults near the started_at and line-84 timestamp usages, with the project’s timezone-aware UTC timestamp approach: datetime.now(timezone.utc).replace(tzinfo=None) or the established equivalent. Update imports as needed while preserving existing timestamp formatting and behavior.
🤖 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 `@plugins/coaching/coaching.js`:
- Around line 1111-1116: Update _stringLabel so bass string names follow the
low-to-high ordering for extended-range instruments: use B-E-A-D-G for
five-string basses and B-E-A-D-G-C for six-string basses, while preserving
guitar labels and the fallback for unknown indices. Ensure the selected bass
mapping reflects the arrangement’s string count.
In `@plugins/coaching/README.md`:
- Around line 44-48: Update the coaching README’s documented default model from
claude-opus-4-8 to claude-haiku-4-5, including the model description, the
“default is opus-4-8” statement, and the _model sample so they match COACH_MODEL
and settings.html.
- Around line 29-33: Update the mistake.faultVerdict documentation in the
coaching README to reflect that confirmed_detector_bug can be assigned in-app by
_faultVerdict when a no_detection miss has notePresent, and remove the outdated
external-harness-only claim while preserving the other routing semantics.
In `@plugins/coaching/test/panel_integration.test.js`:
- Around line 143-149: Update the clustering-threshold comment in test “no-key:
a clustered miss play surfaces a Practice panel at song end” to state
minMisses=2 and gap=3 seconds, and revise the neighboring clean-play test
comment to reflect that one miss versus two is the boundary. Keep the test
behavior unchanged.
In `@plugins/metronome_lock/game.js`:
- Around line 110-136: Prevent stale _startMetronome continuations from
configuring shared metronome state after a newer start request. Add a
run-generation or cancellation guard spanning _startMetronome’s await resume()
boundary, and ensure only the latest invocation sets timing fields, _armed, and
_schedTimer; clean up any superseded AudioContext without leaving an untracked
interval. Keep _setBpm, _setMode, and _toggleDrop behavior unchanged.
- Around line 160-165: Update _onPitch so low-confidence or invalid pitch
updates do not immediately re-arm the note; use the SDK’s note-off/end signal
when available, or require a stable low/unpitched interval before setting _armed
= true. Preserve the existing single-attack consumption behavior so another
high-confidence update cannot score until a genuine release occurs.
In `@plugins/mute_master/game.js`:
- Line 38: Update the mute-scoring logic associated with FRETS and the `dTarget`
checks so a single matching pitch cannot qualify a repetition as clean. Require
an independent `mute_fail` or multi-pitch signal that detects adjacent
open-string bleed alongside the intended note, and apply the same correction to
the logic around the referenced later scoring block.
In `@plugins/mute_master/README.md`:
- Around line 6-12: Update the README paragraph to state that scoring uses
sdk.scoring.createContinuous() with locally classified pitch frames, removing
the claim that it consumes the live note_detect judgment stream. Preserve the
existing description of mute_fail taxonomy and shared skill-drill mechanics.
In `@plugins/pitch_match/game.js`:
- Around line 129-131: Update the target-advancement flow around _render,
_finish, and _nextTarget so advancing to the next target does not immediately
overwrite the judged feedback rendered for ev. Preserve the existing completion
behavior, while ensuring clean/sharp/flat feedback remains visible before the
next target is rendered.
In `@plugins/practice_journal/routes.py`:
- Around line 56-58: Validate the duration value in the route before the
comparison in the duration handling block: treat None and other non-numeric
inputs as invalid or skipped according to the route’s existing input-validation
behavior, then only evaluate the under-5-second condition for a valid numeric
duration.
- Around line 14-41: Update _get_conn() to use double-checked locking with the
existing _lock: retain the initial _conn check, acquire _lock before creating
the connection and running schema initialization, then recheck _conn inside the
lock before proceeding. Return the single initialized connection while
preserving the existing setup and commit behavior.
In `@plugins/practice_journal/screen.js`:
- Line 79: The session payload in plugins/practice_journal/screen.js at line 79
must not claim loop tracking by always sending an empty loops_used array: either
implement accumulation of loop-start/loop-end events before _pjEndSession, or
remove the field until tracking exists. Update the “Loop tracking” documentation
in plugins/practice_journal/README.md lines 14-16 to remove or clearly caveat
the claim, matching the chosen implementation.
- Around line 185-201: Escape s.arrangement before interpolating it into arrStr
in the recent-session rendering map within the practice journal UI. Reuse the
existing esc() helper, preserving the current conditional display and leaving
the already escaped title and artist handling unchanged.
---
Nitpick comments:
In `@plugins/coaching/coaching.js`:
- Around line 1516-1519: Rename the local settings variable in
_gatherSessionMeta to ndSettings to avoid shadowing the module-scope settings
object, and update all references within the function, including the returned
object’s settings property, to use ndSettings.
In `@plugins/coaching/README.md`:
- Line 14: Add the text language identifier to the fenced code block containing
the notedetect example in the coaching README, changing the opening fence to use
text while preserving the example content.
In `@plugins/coaching/test/coach.test.js`:
- Around line 100-101: Update the model override assertions in buildCoachRequest
and setModel tests to use a model string distinct from the COACH_MODEL default,
so they fail when the override is ignored. Keep asserting that the
returned/requested model equals the distinct override value.
In `@plugins/coaching/tools/headless-emit-check.js`:
- Around line 1-16: The header comment for the headless check incorrectly claims
it loads and plays a real song; update it to describe the synthetic notedetect
event flow. Remove the unused SONG and ARR environment parsing, and remove the
vestigial waitForFunction targeting window.playSong while preserving the actual
Playwright check behavior.
In `@plugins/practice_journal/routes.py`:
- Line 70: Replace both datetime.utcnow() calls in the route handling this data,
including the defaults near the started_at and line-84 timestamp usages, with
the project’s timezone-aware UTC timestamp approach:
datetime.now(timezone.utc).replace(tzinfo=None) or the established equivalent.
Update imports as needed while preserving existing timestamp formatting and
behavior.
In `@plugins/practice_journal/screen.js`:
- Around line 106-108: Update the beforeunload flow around _pjEndSession to use
navigator.sendBeacon for the final session-recording request instead of fetch,
preserving the existing endpoint and payload and ensuring the unload handler
remains non-blocking.
🪄 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: 04b7bdae-2e8b-4c03-8498-e22bbe60561e
📒 Files selected for processing (22)
plugins/coaching/README.mdplugins/coaching/coaching.jsplugins/coaching/plugin.jsonplugins/coaching/settings.htmlplugins/coaching/test/coach.test.jsplugins/coaching/test/feedback-schema.test.jsplugins/coaching/test/panel_integration.test.jsplugins/coaching/test/skill-drills.test.jsplugins/coaching/tools/headless-emit-check.jsplugins/metronome_lock/game.jsplugins/metronome_lock/plugin.jsonplugins/mute_master/README.mdplugins/mute_master/game.jsplugins/mute_master/plugin.jsonplugins/pitch_match/game.jsplugins/pitch_match/plugin.jsonplugins/practice_journal/.gitignoreplugins/practice_journal/README.mdplugins/practice_journal/plugin.jsonplugins/practice_journal/routes.pyplugins/practice_journal/screen.htmlplugins/practice_journal/screen.js
Comment on lines
+1111
to
+1116
| function _stringLabel(s, arrangement) { | ||
| const bass = ['E', 'A', 'D', 'G', 'C', 'B']; | ||
| const gtr = ['E', 'A', 'D', 'G', 'B', 'e']; | ||
| const names = (arrangement && /bass/i.test(arrangement)) ? bass : gtr; | ||
| return (names[s] != null) ? names[s] : `string ${s}`; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_stringLabel indices 4-5 are mislabelled for extended-range basses.
Indices are low→high, but a 5/6-string bass is B-E-A-D-G-C, so index 0 isn't E and indices 4-5 aren't C,B in that order. Only affects 5/6-string arrangements, but it tells the player to look at the wrong string.
🤖 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 `@plugins/coaching/coaching.js` around lines 1111 - 1116, Update _stringLabel
so bass string names follow the low-to-high ordering for extended-range
instruments: use B-E-A-D-G for five-string basses and B-E-A-D-G-C for six-string
basses, while preserving guitar labels and the fallback for unknown indices.
Ensure the selected bass mapping reflects the arrangement’s string count.
Comment on lines
+29
to
+33
| - **`mistake.faultVerdict`** (`player_error` | `detector_suspect` | | ||
| `confirmed_detector_bug`) — routes a miss to *drill it* vs *feed it to the | ||
| harness*. `no_detection` → `detector_suspect`; a detection that fired but was | ||
| off → `player_error`; `confirmed_detector_bug` is only ever set by an external | ||
| harness replay. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale claim: confirmed_detector_bug is now set in-app.
_faultVerdict in coaching.js (lines 247-248) promotes a no_detection miss to confirmed_detector_bug whenever notePresent is true — the external-harness-only wording is out of date.
📝 Suggested wording
- off → `player_error`; `confirmed_detector_bug` is only ever set by an external
- harness replay.
+ off → `player_error`; `confirmed_detector_bug` is set when note_detect measured
+ the expected note as present in the audio (`notePresent`) despite the miss.📝 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
| - **`mistake.faultVerdict`** (`player_error` | `detector_suspect` | | |
| `confirmed_detector_bug`) — routes a miss to *drill it* vs *feed it to the | |
| harness*. `no_detection` → `detector_suspect`; a detection that fired but was | |
| off → `player_error`; `confirmed_detector_bug` is only ever set by an external | |
| harness replay. | |
| - **`mistake.faultVerdict`** (`player_error` | `detector_suspect` | | |
| `confirmed_detector_bug`) — routes a miss to *drill it* vs *feed it to the | |
| harness*. `no_detection` → `detector_suspect`; a detection that fired but was | |
| off → `player_error`; `confirmed_detector_bug` is set when note_detect measured | |
| the expected note as present in the audio (`notePresent`) despite the miss. |
🤖 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 `@plugins/coaching/README.md` around lines 29 - 33, Update the
mistake.faultVerdict documentation in the coaching README to reflect that
confirmed_detector_bug can be assigned in-app by _faultVerdict when a
no_detection miss has notePresent, and remove the outdated external-harness-only
claim while preserving the other routing semantics.
Comment on lines
+44
to
+48
| `session.summary` is filled by a **client-side** Anthropic Messages API call: | ||
|
|
||
| - **Model** `claude-opus-4-8`, a frozen + prompt-cached system prompt, and | ||
| `output_config.format` (json_schema) so the reply parses deterministically. | ||
| - The request **distills** the feedback to the session block + hotspots + |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Documented default model contradicts the code.
COACH_MODEL is claude-haiku-4-5 (coaching.js line 534) and settings.html line 48 also states haiku is the default, but this section — and line 82 (default is opus-4-8) plus the _model sample on line 71 — says claude-opus-4-8.
🤖 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 `@plugins/coaching/README.md` around lines 44 - 48, Update the coaching
README’s documented default model from claude-opus-4-8 to claude-haiku-4-5,
including the model description, the “default is opus-4-8” statement, and the
_model sample so they match COACH_MODEL and settings.html.
Comment on lines
+143
to
+149
| test('no-key: a clustered miss play surfaces a Practice panel at song end', () => { | ||
| const { host, fireWin } = harness(); | ||
| // 3 misses within 2s → one hotspot (findHotspots minMisses=3, gap=2s). | ||
| fireWin('notedetect:miss', missJudgment(20.0)); | ||
| fireWin('notedetect:miss', missJudgment(20.4)); | ||
| fireWin('notedetect:miss', missJudgment(20.8)); | ||
| fireWin('notedetect:session', { song: 'Gasoline', arrangement: 'bass', sections: [] }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale comment: the clustering thresholds are minMisses=2, gap=3.0.
findHotspots defaults (coaching.js lines 347-348) were loosened; the comment still cites the old minMisses=3, gap=2s rule, which also makes the neighbouring "clean play" test at line 183 read as if 1 miss vs 3 were the boundary.
📝 Proposed fix
- // 3 misses within 2s → one hotspot (findHotspots minMisses=3, gap=2s).
+ // 3 misses within 0.8s → one hotspot (findHotspots minMisses=2, gap=3s).🤖 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 `@plugins/coaching/test/panel_integration.test.js` around lines 143 - 149,
Update the clustering-threshold comment in test “no-key: a clustered miss play
surfaces a Practice panel at song end” to state minMisses=2 and gap=3 seconds,
and revise the neighboring clean-play test comment to reflect that one miss
versus two is the boundary. Keep the test behavior unchanged.
Comment on lines
+110
to
+136
| async function _startMetronome(bpm) { | ||
| _stopMetronome(); | ||
| _bpm = bpm; | ||
| _beatPeriodMs = 60000 / bpm; | ||
| _subdivPerBeat = (_clickMode === 'eighths') ? 2 : 1; | ||
| _subdivPeriodMs = _beatPeriodMs / _subdivPerBeat; | ||
| const AC = window.AudioContext || window.webkitAudioContext; | ||
| _clickCtx = new AC(); | ||
| try { await _clickCtx.resume(); } catch (_) {} | ||
| const lead = 0.25; // small runway before tick 0 | ||
| _anchorAudio = _clickCtx.currentTime + lead; | ||
| _anchorPerf = performance.now() + lead * 1000; | ||
| _nextTickAudio = _anchorAudio; | ||
| _tickNum = 0; | ||
| _armed = true; | ||
| _schedTimer = setInterval(_scheduler, SCHED_INTERVAL_MS); | ||
| } | ||
|
|
||
| function _stopMetronome() { | ||
| if (_schedTimer) { clearInterval(_schedTimer); _schedTimer = null; } | ||
| try { if (_clickCtx) _clickCtx.close(); } catch (_) {} | ||
| _clickCtx = null; | ||
| } | ||
|
|
||
| function _setBpm(bpm) { if (!_finished) { _startMetronome(bpm); _render(); } } | ||
| function _setMode(mode) { if (!_finished) { _clickMode = mode; _startMetronome(_bpm); _render(); } } | ||
| function _toggleDrop() { if (!_finished) { _dropOn = !_dropOn; _startMetronome(_bpm); _render(); } } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent overlapping metronome runs.
_startMetronome() awaits resume(), while Lines 134-136 can start another run before the earlier call resumes. The stale continuation then overwrites shared timing state and installs an untracked interval, causing duplicate clicks and an incorrect scoring grid.
Proposed fix
let _anchorPerf = 0, _anchorAudio = 0, _nextTickAudio = 0, _tickNum = 0, _schedTimer = null;
+let _metronomeRun = 0;
async function _startMetronome(bpm) {
_stopMetronome();
+ const run = ++_metronomeRun;
_bpm = bpm;
_beatPeriodMs = 60000 / bpm;
_subdivPerBeat = (_clickMode === 'eighths') ? 2 : 1;
_subdivPeriodMs = _beatPeriodMs / _subdivPerBeat;
const AC = window.AudioContext || window.webkitAudioContext;
- _clickCtx = new AC();
- try { await _clickCtx.resume(); } catch (_) {}
+ const clickCtx = new AC();
+ _clickCtx = clickCtx;
+ try { await clickCtx.resume(); } catch (_) {}
+ if (run !== _metronomeRun || _clickCtx !== clickCtx) {
+ try { clickCtx.close(); } catch (_) {}
+ return;
+ }
const lead = 0.25; // small runway before tick 0
- _anchorAudio = _clickCtx.currentTime + lead;
+ _anchorAudio = clickCtx.currentTime + lead;
_anchorPerf = performance.now() + lead * 1000;
_nextTickAudio = _anchorAudio;
_tickNum = 0;
_armed = true;
- _schedTimer = setInterval(_scheduler, SCHED_INTERVAL_MS);
+ _schedTimer = setInterval(() => {
+ if (run === _metronomeRun) _scheduler();
+ }, SCHED_INTERVAL_MS);
}
function _stopMetronome() {
+ _metronomeRun++;
if (_schedTimer) { clearInterval(_schedTimer); _schedTimer = null; }📝 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
| async function _startMetronome(bpm) { | |
| _stopMetronome(); | |
| _bpm = bpm; | |
| _beatPeriodMs = 60000 / bpm; | |
| _subdivPerBeat = (_clickMode === 'eighths') ? 2 : 1; | |
| _subdivPeriodMs = _beatPeriodMs / _subdivPerBeat; | |
| const AC = window.AudioContext || window.webkitAudioContext; | |
| _clickCtx = new AC(); | |
| try { await _clickCtx.resume(); } catch (_) {} | |
| const lead = 0.25; // small runway before tick 0 | |
| _anchorAudio = _clickCtx.currentTime + lead; | |
| _anchorPerf = performance.now() + lead * 1000; | |
| _nextTickAudio = _anchorAudio; | |
| _tickNum = 0; | |
| _armed = true; | |
| _schedTimer = setInterval(_scheduler, SCHED_INTERVAL_MS); | |
| } | |
| function _stopMetronome() { | |
| if (_schedTimer) { clearInterval(_schedTimer); _schedTimer = null; } | |
| try { if (_clickCtx) _clickCtx.close(); } catch (_) {} | |
| _clickCtx = null; | |
| } | |
| function _setBpm(bpm) { if (!_finished) { _startMetronome(bpm); _render(); } } | |
| function _setMode(mode) { if (!_finished) { _clickMode = mode; _startMetronome(_bpm); _render(); } } | |
| function _toggleDrop() { if (!_finished) { _dropOn = !_dropOn; _startMetronome(_bpm); _render(); } } | |
| let _metronomeRun = 0; | |
| async function _startMetronome(bpm) { | |
| _stopMetronome(); | |
| const run = ++_metronomeRun; | |
| _bpm = bpm; | |
| _beatPeriodMs = 60000 / bpm; | |
| _subdivPerBeat = (_clickMode === 'eighths') ? 2 : 1; | |
| _subdivPeriodMs = _beatPeriodMs / _subdivPerBeat; | |
| const AC = window.AudioContext || window.webkitAudioContext; | |
| const clickCtx = new AC(); | |
| _clickCtx = clickCtx; | |
| try { await clickCtx.resume(); } catch (_) {} | |
| if (run !== _metronomeRun || _clickCtx !== clickCtx) { | |
| try { clickCtx.close(); } catch (_) {} | |
| return; | |
| } | |
| const lead = 0.25; // small runway before tick 0 | |
| _anchorAudio = clickCtx.currentTime + lead; | |
| _anchorPerf = performance.now() + lead * 1000; | |
| _nextTickAudio = _anchorAudio; | |
| _tickNum = 0; | |
| _armed = true; | |
| _schedTimer = setInterval(() => { | |
| if (run === _metronomeRun) _scheduler(); | |
| }, SCHED_INTERVAL_MS); | |
| } | |
| function _stopMetronome() { | |
| _metronomeRun++; | |
| if (_schedTimer) { clearInterval(_schedTimer); _schedTimer = null; } | |
| try { if (_clickCtx) _clickCtx.close(); } catch (_) {} | |
| _clickCtx = null; | |
| } | |
| function _setBpm(bpm) { if (!_finished) { _startMetronome(bpm); _render(); } } | |
| function _setMode(mode) { if (!_finished) { _clickMode = mode; _startMetronome(_bpm); _render(); } } | |
| function _toggleDrop() { if (!_finished) { _dropOn = !_dropOn; _startMetronome(_bpm); _render(); } } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 124-124: Avoid using the initial state variable in setState
Context: setInterval(_scheduler, SCHED_INTERVAL_MS)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[error] 124-124: React's useState should not be directly called
Context: setInterval(_scheduler, SCHED_INTERVAL_MS)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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 `@plugins/metronome_lock/game.js` around lines 110 - 136, Prevent stale
_startMetronome continuations from configuring shared metronome state after a
newer start request. Add a run-generation or cancellation guard spanning
_startMetronome’s await resume() boundary, and ensure only the latest invocation
sets timing fields, _armed, and _schedTimer; clean up any superseded
AudioContext without leaving an untracked interval. Keep _setBpm, _setMode, and
_toggleDrop behavior unchanged.
Comment on lines
+129
to
+131
| _render(ev); | ||
| if (view.passed) _finish(); | ||
| else _nextTarget(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the judged feedback when advancing targets.
Line 129 renders ev, but Line 131 immediately rerenders without it before the browser can paint. Players therefore never see clean/sharp/flat feedback except at completion.
Proposed fix
-function _nextTarget() {
+function _nextTarget(ev) {
const i = _drill ? _drill.reps : 0;
const s = STRINGS[i % STRINGS.length];
const fret = FRETS[(Math.floor(i / STRINGS.length)) % FRETS.length];
_target = { stringName: s.name, fret, targetMidi: s.openMidi + fret };
_hold = [];
- _render();
+ _render(ev);
}
...
- else _nextTarget();
+ else _nextTarget(ev);🤖 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 `@plugins/pitch_match/game.js` around lines 129 - 131, Update the
target-advancement flow around _render, _finish, and _nextTarget so advancing to
the next target does not immediately overwrite the judged feedback rendered for
ev. Preserve the existing completion behavior, while ensuring clean/sharp/flat
feedback remains visible before the next target is rendered.
Comment on lines
+14
to
+41
| def _get_conn(): | ||
| global _conn | ||
| if _conn is None: | ||
| _conn = sqlite3.connect(_db_path, check_same_thread=False) | ||
| _conn.execute("PRAGMA journal_mode=WAL") | ||
| _conn.execute(""" | ||
| CREATE TABLE IF NOT EXISTS practice_sessions ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| filename TEXT NOT NULL, | ||
| title TEXT, | ||
| artist TEXT, | ||
| started_at TEXT NOT NULL, | ||
| duration_seconds REAL NOT NULL DEFAULT 0, | ||
| avg_speed REAL NOT NULL DEFAULT 1.0, | ||
| loops_used TEXT DEFAULT '[]', | ||
| arrangement TEXT | ||
| ) | ||
| """) | ||
| _conn.execute(""" | ||
| CREATE INDEX IF NOT EXISTS idx_practice_filename | ||
| ON practice_sessions(filename) | ||
| """) | ||
| _conn.execute(""" | ||
| CREATE INDEX IF NOT EXISTS idx_practice_started | ||
| ON practice_sessions(started_at) | ||
| """) | ||
| _conn.commit() | ||
| return _conn |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Lazy singleton init is racy under concurrent requests.
_get_conn() checks _conn is None and creates the connection outside _lock. Two concurrent first requests can both see None, each opening its own sqlite3.connect(...)/schema setup; one connection object gets silently discarded (never closed), and the schema-creation statements run twice. Guard the check-and-create with the existing _lock (double-checked locking).
🔒 Proposed fix
def _get_conn():
global _conn
if _conn is None:
- _conn = sqlite3.connect(_db_path, check_same_thread=False)
- _conn.execute("PRAGMA journal_mode=WAL")
- _conn.execute("""
- CREATE TABLE IF NOT EXISTS practice_sessions (
- ...
- )
- """)
- _conn.execute("""...""")
- _conn.execute("""...""")
- _conn.commit()
+ with _lock:
+ if _conn is None:
+ _conn = sqlite3.connect(_db_path, check_same_thread=False)
+ _conn.execute("PRAGMA journal_mode=WAL")
+ _conn.execute("""
+ CREATE TABLE IF NOT EXISTS practice_sessions (
+ ...
+ )
+ """)
+ _conn.execute("""...""")
+ _conn.execute("""...""")
+ _conn.commit()
return _conn📝 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
| def _get_conn(): | |
| global _conn | |
| if _conn is None: | |
| _conn = sqlite3.connect(_db_path, check_same_thread=False) | |
| _conn.execute("PRAGMA journal_mode=WAL") | |
| _conn.execute(""" | |
| CREATE TABLE IF NOT EXISTS practice_sessions ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| filename TEXT NOT NULL, | |
| title TEXT, | |
| artist TEXT, | |
| started_at TEXT NOT NULL, | |
| duration_seconds REAL NOT NULL DEFAULT 0, | |
| avg_speed REAL NOT NULL DEFAULT 1.0, | |
| loops_used TEXT DEFAULT '[]', | |
| arrangement TEXT | |
| ) | |
| """) | |
| _conn.execute(""" | |
| CREATE INDEX IF NOT EXISTS idx_practice_filename | |
| ON practice_sessions(filename) | |
| """) | |
| _conn.execute(""" | |
| CREATE INDEX IF NOT EXISTS idx_practice_started | |
| ON practice_sessions(started_at) | |
| """) | |
| _conn.commit() | |
| return _conn | |
| def _get_conn(): | |
| global _conn | |
| if _conn is None: | |
| with _lock: | |
| if _conn is None: | |
| _conn = sqlite3.connect(_db_path, check_same_thread=False) | |
| _conn.execute("PRAGMA journal_mode=WAL") | |
| _conn.execute(""" | |
| CREATE TABLE IF NOT EXISTS practice_sessions ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| filename TEXT NOT NULL, | |
| title TEXT, | |
| artist TEXT, | |
| started_at TEXT NOT NULL, | |
| duration_seconds REAL NOT NULL DEFAULT 0, | |
| avg_speed REAL NOT NULL DEFAULT 1.0, | |
| loops_used TEXT DEFAULT '[]', | |
| arrangement TEXT | |
| ) | |
| """) | |
| _conn.execute(""" | |
| CREATE INDEX IF NOT EXISTS idx_practice_filename | |
| ON practice_sessions(filename) | |
| """) | |
| _conn.execute(""" | |
| CREATE INDEX IF NOT EXISTS idx_practice_started | |
| ON practice_sessions(started_at) | |
| """) | |
| _conn.commit() | |
| return _conn |
🤖 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 `@plugins/practice_journal/routes.py` around lines 14 - 41, Update _get_conn()
to use double-checked locking with the existing _lock: retain the initial _conn
check, acquire _lock before creating the connection and running schema
initialization, then recheck _conn inside the lock before proceeding. Return the
single initialized connection while preserving the existing setup and commit
behavior.
Comment on lines
+56
to
+58
| duration = data.get("duration", 0) | ||
| if duration < 5: # ignore sessions under 5 seconds | ||
| return {"ok": True, "skipped": True} |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unvalidated duration can crash the route.
filename is defensively checked, but duration = data.get("duration", 0) returns None (not the default) if the client sends "duration": null, and None < 5 raises TypeError, returning a 500. Validate the type before comparing.
🛡️ Proposed fix
- duration = data.get("duration", 0)
- if duration < 5: # ignore sessions under 5 seconds
+ duration = data.get("duration") or 0
+ if not isinstance(duration, (int, float)) or duration < 5: # ignore sessions under 5 seconds
return {"ok": True, "skipped": True}📝 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
| duration = data.get("duration", 0) | |
| if duration < 5: # ignore sessions under 5 seconds | |
| return {"ok": True, "skipped": True} | |
| duration = data.get("duration") or 0 | |
| if not isinstance(duration, (int, float)) or duration < 5: # ignore sessions under 5 seconds | |
| return {"ok": True, "skipped": True} |
🤖 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 `@plugins/practice_journal/routes.py` around lines 56 - 58, Validate the
duration value in the route before the comparison in the duration handling
block: treat None and other non-numeric inputs as invalid or skipped according
to the route’s existing input-validation behavior, then only evaluate the
under-5-second condition for a valid numeric duration.
| started_at: _pjSessionStart, | ||
| duration: duration, | ||
| avg_speed: avgSpeed, | ||
| loops_used: [], |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
"Loop tracking" is documented but never implemented. plugins/practice_journal/screen.js always posts a hardcoded empty loops_used array, so no loop usage is ever actually recorded, contradicting the README's claim.
plugins/practice_journal/screen.js#L79: replace the hardcodedloops_used: []with real tracking of loops activated during the session (e.g., accumulate loop-start/loop-end events into an array before_pjEndSessionfires), or remove the field until implemented.plugins/practice_journal/README.md#L14-L16: until loop tracking is implemented, remove or caveat the "Loop tracking" bullet so the docs match actual behavior.
📍 Affects 2 files
plugins/practice_journal/screen.js#L79-L79(this comment)plugins/practice_journal/README.md#L14-L16
🤖 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 `@plugins/practice_journal/screen.js` at line 79, The session payload in
plugins/practice_journal/screen.js at line 79 must not claim loop tracking by
always sending an empty loops_used array: either implement accumulation of
loop-start/loop-end events before _pjEndSession, or remove the field until
tracking exists. Update the “Loop tracking” documentation in
plugins/practice_journal/README.md lines 14-16 to remove or clearly caveat the
claim, matching the chosen implementation.
Comment on lines
+185
to
+201
| recent.innerHTML = data.recent.map(s => { | ||
| const date = new Date(s.started_at); | ||
| const timeStr = date.toLocaleDateString('en', { month: 'short', day: 'numeric' }) | ||
| + ' ' + date.toLocaleTimeString('en', { hour: '2-digit', minute: '2-digit' }); | ||
| const speedStr = s.speed !== 1.0 ? ` · ${s.speed.toFixed(2)}x` : ''; | ||
| const arrStr = s.arrangement ? ` · ${s.arrangement}` : ''; | ||
| return `<div class="flex items-center justify-between py-2 px-3 bg-dark-700/30 rounded-lg"> | ||
| <div class="min-w-0"> | ||
| <span class="text-sm text-white truncate block">${esc(s.title || s.filename)}</span> | ||
| <span class="text-xs text-gray-500">${esc(s.artist)}${arrStr}</span> | ||
| </div> | ||
| <div class="text-right flex-shrink-0 ml-2"> | ||
| <span class="text-xs text-gray-300 block">${_pjFormatDuration(s.duration)}${speedStr}</span> | ||
| <span class="text-xs text-gray-600">${timeStr}</span> | ||
| </div> | ||
| </div>`; | ||
| }).join(''); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Unescaped arrangement breaks the XSS protection applied one line above.
arrStr (line 190) interpolates raw s.arrangement and is concatenated unescaped into the template at line 194, even though s.artist right next to it is passed through esc(). arrangement originates from song metadata (_pjSongMeta() → cur.arrangement/info.arrangement), which can be attacker-controlled if a crafted song file is loaded, then persists through the DB and is rendered here via .innerHTML. This is a stored XSS gap.
🛡️ Proposed fix
- const arrStr = s.arrangement ? ` · ${s.arrangement}` : '';
+ const arrStr = s.arrangement ? ` · ${esc(s.arrangement)}` : '';📝 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
| recent.innerHTML = data.recent.map(s => { | |
| const date = new Date(s.started_at); | |
| const timeStr = date.toLocaleDateString('en', { month: 'short', day: 'numeric' }) | |
| + ' ' + date.toLocaleTimeString('en', { hour: '2-digit', minute: '2-digit' }); | |
| const speedStr = s.speed !== 1.0 ? ` · ${s.speed.toFixed(2)}x` : ''; | |
| const arrStr = s.arrangement ? ` · ${s.arrangement}` : ''; | |
| return `<div class="flex items-center justify-between py-2 px-3 bg-dark-700/30 rounded-lg"> | |
| <div class="min-w-0"> | |
| <span class="text-sm text-white truncate block">${esc(s.title || s.filename)}</span> | |
| <span class="text-xs text-gray-500">${esc(s.artist)}${arrStr}</span> | |
| </div> | |
| <div class="text-right flex-shrink-0 ml-2"> | |
| <span class="text-xs text-gray-300 block">${_pjFormatDuration(s.duration)}${speedStr}</span> | |
| <span class="text-xs text-gray-600">${timeStr}</span> | |
| </div> | |
| </div>`; | |
| }).join(''); | |
| recent.innerHTML = data.recent.map(s => { | |
| const date = new Date(s.started_at); | |
| const timeStr = date.toLocaleDateString('en', { month: 'short', day: 'numeric' }) | |
| ' ' + date.toLocaleTimeString('en', { hour: '2-digit', minute: '2-digit' }); | |
| const speedStr = s.speed !== 1.0 ? ` · ${s.speed.toFixed(2)}x` : ''; | |
| const arrStr = s.arrangement ? ` · ${esc(s.arrangement)}` : ''; | |
| return `<div class="flex items-center justify-between py-2 px-3 bg-dark-700/30 rounded-lg"> | |
| <div class="min-w-0"> | |
| <span class="text-sm text-white truncate block">${esc(s.title || s.filename)}</span> | |
| <span class="text-xs text-gray-500">${esc(s.artist)}${arrStr}</span> | |
| </div> | |
| <div class="text-right flex-shrink-0 ml-2"> | |
| <span class="text-xs text-gray-300 block">${_pjFormatDuration(s.duration)}${speedStr}</span> | |
| <span class="text-xs text-gray-600">${timeStr}</span> | |
| </div> | |
| </div>`; | |
| }).join(''); |
🤖 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 `@plugins/practice_journal/screen.js` around lines 185 - 201, Escape
s.arrangement before interpolating it into arrStr in the recent-session
rendering map within the practice journal UI. Reuse the existing esc() helper,
preserving the current conditional display and leaving the already escaped title
and artist handling unchanged.
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.
What
Five self-contained, additive practice plugins (new dirs only — no core changes):
coachingmetronome_lockmute_masterpitch_matchpractice_journalroutes.py)Ported onto feedBack's own contracts (
window.feedBackbus + the nativeminigamesframework — gamekit was not ported, it's redundant with the minigames plugin; the three games register/score with no shims).Validation
main(backend +practice_journalroutes load, no errors).coachingsuite: 66/66 node tests pass.Note
Loads + backend/tests are verified; frontend runtime behaviour on current
mainwants a quick review (these were adapted to feedBack's bus but the app has moved since).🤖 Generated with Claude Code
Summary by CodeRabbit