fix(player): record actual server-loaded arrangement instead of defaulting to 0#1036
fix(player): record actual server-loaded arrangement instead of defaulting to 0#1036ColeChiodo wants to merge 2 commits into
Conversation
…ting to 0 Signed-off-by: ColeChiodo <colechiodo@gmail.com>
Signed-off-by: ColeChiodo <colechiodo@gmail.com>
📝 WalkthroughWalkthroughThe stats recorder now updates active sessions from the server-provided arrangement index on ChangesArrangement attribution correction
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/js/stats_recorder_arrangement.test.js (1)
60-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake scenario 3 cover a distinct preservation path.
It currently uses arrangement
0both before and after loading, duplicating scenario 2’s observable result. Test a nonzero explicit arrangement with a missing/invalidarrangementIndex, or remove the scenario because it currently adds no regression coverage.🤖 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 `@tests/js/stats_recorder_arrangement.test.js` around lines 60 - 68, Update scenario 3 in the stats recorder test to exercise a distinct explicit-arrangement preservation path: use a nonzero arrangement and a missing or invalid arrangementIndex in the loading event, then assert that the posted stats preserve that explicit arrangement. Alternatively, remove scenario 3 if this coverage is not needed.
🤖 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 `@static/v3/stats-recorder.js`:
- Around line 198-202: Update the song:loaded handler to correlate the event
with the active session before assigning cur.arrangement: require d.filename to
match cur.filename or use the existing load-session identity. Ignore stale
events from prior playSong or closeCurrentSong operations, while preserving
arrangement updates for the current song.
---
Nitpick comments:
In `@tests/js/stats_recorder_arrangement.test.js`:
- Around line 60-68: Update scenario 3 in the stats recorder test to exercise a
distinct explicit-arrangement preservation path: use a nonzero arrangement and a
missing or invalid arrangementIndex in the loading event, then assert that the
posted stats preserve that explicit arrangement. Alternatively, remove scenario
3 if this coverage is not needed.
🪄 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: 2aaee26a-29a3-488c-9c64-be60a1b5e1bc
📒 Files selected for processing (3)
CHANGELOG.mdstatic/v3/stats-recorder.jstests/js/stats_recorder_arrangement.test.js
| sm.on('song:loaded', (e) => { | ||
| const d = (e && e.detail) || {}; | ||
| if (cur && Number.isFinite(d.arrangementIndex)) { | ||
| cur.arrangement = Number(d.arrangementIndex); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching stats-recorder.js:"
fd -a 'stats-recorder.js$' . || true
echo
echo "Line count:"
wc -l static/v3/stats-docs.js static/v3/stats-recorder.js 2>/dev/null || true
echo
echo "Relevant section:"
sed -n '150,230p' static/v3/stats-recorder.js 2>/dev/null || true
echo
echo "Search for song:loaded handler/tests/usages:"
rg -n "song:loaded|arrangementIndex|arrangement" -S . --glob '*.{js,html,css}' || trueRepository: got-feedBack/feedBack
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "stats recorder test file:"
sed -n '1,120p' tests/js/stats_recorder_arrangement.test.js 2>/dev/null || true
echo
echo "stats recorder remaining relevant section:"
sed -n '70,210p' static/v3/stats-recorder.js 2>/dev/null || true
echo
echo "Find exact highway song_info -> song:loaded evidence:"
rg -n "songInfo|arrangement_index|song:loaded|emits?|dispatch" plugins/highway_3d/screen.js plugins/keys_highway_3d/screen.js plugins/highway/screen.js plugins/drum_highway_3d/screen.js 2>/dev/null --max-count 80 || true
echo
echo "Focus highway.js related files:"
git ls-files | rg 'highway|playback|session' | head -60Repository: got-feedBack/feedBack
Length of output: 23517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "static/highway.js line counts:"
wc -l static/js/highway-colors.js static/js/highway-constants.js static/highway.js static/js/transport.js static/js/session.js 2>/dev/null || true
echo
echo "static/highway.js song:loaded/song_info/reconnect sections:"
rg -n -C 8 "song:loaded|song_info|songInfo|arrangement_index|song:loading|song:ready" static/highway.js static/js/highway-colors.js 2>/dev/null || true
echo
echo "static/js/session.js playSong/reconnect/transport sections:"
sed -n '620,760p' static/js/session.js 2>/dev/null || true
rg -n -C 6 "reconnect|song:loading|song:loaded|song:ready|playSong|socket" static/js/transport.js static/js/session.js 2>/dev/null || true
echo
echo "highway 3d explicit song_info -> song:loaded path sections around main init:"
sed -n '6060,6170p' plugins/highway_3d/screen.js 2>/dev/null || true
sed -n '11640,11670p' plugins/highway_3d/screen.js 2>/dev/null || trueRepository: got-feedBack/feedBack
Length of output: 50377
Correlate song:loaded with the current session.
song:loaded is emitted from the WebSocket song_info path and includes filename, while song:loading resets cur.filename. If an in-flight connection’s song_info arrives after a new playSong or closeCurrentSong, this handler can overwrite the active session’s arrangement and cause the next stats post to be attributed to the wrong song/arrangement pair. Require d.filename === cur.filename or gate updates against the same load/load-session identity before updating cur.arrangement.
🤖 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 `@static/v3/stats-recorder.js` around lines 198 - 202, Update the song:loaded
handler to correlate the event with the active session before assigning
cur.arrangement: require d.filename to match cur.filename or use the existing
load-session identity. Ignore stale events from prior playSong or
closeCurrentSong operations, while preserving arrangement updates for the
current song.
1 participant
What
Fixes "Jump back in!" (home) and "Keep practicing" (song library) loading the wrong arrangement when the user has an instrument preference set (e.g. bass), which resulted in poor UX. The stats recorder defaulted to arrangement 0 because it never learned what arrangement the server actually loaded via instrument routing. Downstream shelves read back the stale 0 and passed it as an explicit index, bypassing server-side routing.
Fix: listen for
song:loadedinstats-recorder.jsand correct the recorded arrangement from thearrangement_indexthe server echoes insong_info.feedpak surface
Checklist
CHANGELOG.md[Unreleased]updated (user-visible changes)git commit -s)Summary by CodeRabbit