Use chart data from bundle (post-chart-transformation plugins) - #1
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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: Organization UI 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:
📝 WalkthroughWalkthroughThe plugin replaces server-side GP5 conversion with direct client-side alphaTab score construction from renderer bundles. It adds chart quantization and score-builder modules, updates package and plugin metadata, refactors rendering lifecycle and cursor synchronization, and adds Node-based tests. ChangesBundle rendering migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Highway
participant screen.js
participant buildScoreFromBundle
participant AlphaTabApi
Highway->>screen.js: Provide bundle and currentTime
screen.js->>buildScoreFromBundle: Build score from bundle
buildScoreFromBundle-->>screen.js: Return alphaTab Score
screen.js->>AlphaTabApi: renderScore(score)
AlphaTabApi-->>screen.js: Report renderFinished or error
Highway->>screen.js: Update audio.currentTime
screen.js->>screen.js: Update beat marker
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
screen.js (1)
150-167: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBoth
_countLEcall sites usecount - 1as an index without clamping, so a NaN target dereferences element-1._countLEreturns0when the comparison is false for every element (which is what NaN does), and each site's "below the first element" early return also fails for NaN — so the derived index becomes-1and the lookup throws on the rAF path.
screen.js#L150-L167: clamp withMath.max(0, Math.min(count - 1, beats.length - 2))before readingbeats[idx].time, and refresh the stale claim at lines 802-804 that NaN "resolves to lookupTick 0".screen.js#L743-L749: clamp withMath.max(0, _countLE(arr, tick, b => b.start) - 1)before readingarr[ans].beat.🤖 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 150 - 167, The `_countLE` results can produce index -1 for NaN inputs, causing invalid array access. In screen.js lines 150-167, clamp the computed `idx` to zero before reading `beats[idx]`; in screen.js lines 743-749, clamp the computed `ans` similarly before reading `arr[ans].beat`. Also update the stale NaN behavior claim at screen.js lines 802-804 to reflect the corrected handling.
🧹 Nitpick comments (2)
src/chart-quantize.js (1)
25-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard
b.measurebefore treating it as a measure marker.
null >= 0istrue, so a beat withmeasure: nullwould split on every beat and render one-beat measures. Use a numerictypeofcheck before the>= 0test, keeping omitted/no-marker beats grouped.🤖 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 `@src/chart-quantize.js` around lines 25 - 28, Update the grouping condition in the beats loop to require that b.measure has numeric type before applying the >= 0 measure-marker check. Preserve the existing cur.length guard and ensure null, omitted, and other non-numeric measure values remain grouped with the current measure.src/score-builder.js (1)
27-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
note.addBendPoint(...)for constructed bends.
Note.addBendPoint()updatesmaxBendPoint, but assigningnote.bendPointsdirectly leaves it null/undefined on new notes. Renderer paths readnote.maxBendPoint.value, so this can produce incorrect bend-glyph layout even though the test only checkshasBendandbendPoints.🤖 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 `@src/score-builder.js` around lines 27 - 36, The constructed bend in the bend-processing block must use Note’s addBendPoint API so maxBendPoint is maintained. Replace direct assignment to note.bendPoints with calls to note.addBendPoint for each BendPoint, preserving the existing points and bendType 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 `@README.md`:
- Line 56: Update the README test documentation to mention both Node test
modules, including test/chart-quantize.test.mjs alongside the existing
src/score-builder.js suite, while preserving the npm test command description.
In `@screen.js`:
- Around line 990-994: Separate ref presence from ref value in the chart-build
guards around _tvPendingNotesRef and _tvFailedNotesRef: add paired active flags,
set them whenever a build starts or fails, and clear them on successful
completion and teardown. Update buildInFlight and previouslyFailed in
_tvRenderFromBundle to use those flags, while preserving the notesRef
comparisons for non-null values; also ensure notes-less bundles receive distinct
chart identity so consecutive null notes can trigger a rebuild.
In `@src/chart-quantize.js`:
- Around line 71-78: Update the fallback beat configuration in the returned
object so beatTimes matches numBeats and spans the full len-second measure using
the expected subdivision spacing. Keep numBeats at 4 and ensure the generated
beatTimes contains the corresponding entries across the measure rather than the
hard-coded 0–3.5 second range.
In `@src/score-builder.js`:
- Around line 107-113: Strengthen the validation before constructing the Note in
the wire-processing loop: require wire.s to be a finite integer within [0,
stringCount), and require wire.f to be a valid finite fret value before
assigning either field. Preserve the existing seen filtering and add
score-builder tests covering undefined/fractional string indices and missing or
invalid frets alongside the out-of-range case.
In `@test/score-builder.test.mjs`:
- Around line 31-35: Update the tempo assertion in the score builder test to
inspect the tempo automation created on the first master bar by
buildScoreFromBundle, rather than asserting score.tempo. Verify the automation
contains the expected tempo value of 120 while leaving the other score
assertions unchanged.
---
Outside diff comments:
In `@screen.js`:
- Around line 150-167: The `_countLE` results can produce index -1 for NaN
inputs, causing invalid array access. In screen.js lines 150-167, clamp the
computed `idx` to zero before reading `beats[idx]`; in screen.js lines 743-749,
clamp the computed `ans` similarly before reading `arr[ans].beat`. Also update
the stale NaN behavior claim at screen.js lines 802-804 to reflect the corrected
handling.
---
Nitpick comments:
In `@src/chart-quantize.js`:
- Around line 25-28: Update the grouping condition in the beats loop to require
that b.measure has numeric type before applying the >= 0 measure-marker check.
Preserve the existing cur.length guard and ensure null, omitted, and other
non-numeric measure values remain grouped with the current measure.
In `@src/score-builder.js`:
- Around line 27-36: The constructed bend in the bend-processing block must use
Note’s addBendPoint API so maxBendPoint is maintained. Replace direct assignment
to note.bendPoints with calls to note.addBendPoint for each BendPoint,
preserving the existing points and bendType behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd8ee046-d8ca-4f77-a750-8a6a1f41211b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.gitignore.specify/memory/constitution.mdREADME.mdpackage.jsonplugin.jsonrequirements.txtroutes.pyrs2gp.pyscreen.jssrc/chart-quantize.jssrc/score-builder.jstest/chart-quantize.test.mjstest/score-builder.test.mjstests/conftest.pytests/test_rs2gp_helpers.py
💤 Files with no reviewable changes (5)
- requirements.txt
- tests/test_rs2gp_helpers.py
- tests/conftest.py
- rs2gp.py
- routes.py
| | `screen.js` | Frontend: alphaTab integration, cursor sync, UI | | ||
| | `src/chart-quantize.js` | Pure chart math: measures, quantization, tuning table (no alphaTab dependency) | | ||
| | `src/score-builder.js` | bundle → alphaTab `Score` builder (techniques, track/staff/bar assembly) | | ||
| | `test/` | Node tests for `src/score-builder.js` (`npm test`) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both Node test modules.
Line [56] omits test/chart-quantize.test.mjs, despite npm test running all test/*.test.mjs files and the constitution requiring both suites.
Proposed fix
-| `test/` | Node tests for `src/score-builder.js` (`npm test`) |
+| `test/` | Node tests for `src/score-builder.js` and `src/chart-quantize.js` (`npm test`) |📝 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.
| | `test/` | Node tests for `src/score-builder.js` (`npm test`) | | |
| | `test/` | Node tests for `src/score-builder.js` and `src/chart-quantize.js` (`npm test`) | |
🤖 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 `@README.md` at line 56, Update the README test documentation to mention both
Node test modules, including test/chart-quantize.test.mjs alongside the existing
src/score-builder.js suite, while preserving the npm test command description.
| const notesRef = bundle.notes || null; | ||
| const chartChanged = notesRef !== _tvCurrentNotesRef; | ||
| const buildInFlight = _tvPendingNotesRef !== null && _tvPendingNotesRef === notesRef; | ||
| const previouslyFailed = _tvFailedNotesRef !== null && _tvFailedNotesRef === notesRef; | ||
| if (chartChanged && !buildInFlight && !previouslyFailed) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
null doubles as both "no build in flight/failed" and a valid normalized notesRef, defeating both guards.
If bundle.notes is ever falsy (transient frames during a song/arrangement swap), notesRef is null, so _tvPendingNotesRef = null while a build is in flight is indistinguishable from "nothing in flight": buildInFlight stays false, chartChanged stays true (against the previous chart's array), and every frame bumps _tvInitToken and kicks off another build+renderScore — the per-frame storm this guard exists to prevent. The failure path has the same hole (_tvFailedNotesRef = null can never satisfy previouslyFailed), and two consecutive notes-less charts won't rebuild at all since both normalize to the same sentinel.
Separate the "is set" bit from the value.
🐛 Proposed fix
const notesRef = bundle.notes || null;
const chartChanged = notesRef !== _tvCurrentNotesRef;
- const buildInFlight = _tvPendingNotesRef !== null && _tvPendingNotesRef === notesRef;
- const previouslyFailed = _tvFailedNotesRef !== null && _tvFailedNotesRef === notesRef;
+ const buildInFlight = _tvPendingActive && _tvPendingNotesRef === notesRef;
+ const previouslyFailed = _tvFailedActive && _tvFailedNotesRef === notesRef;Paired flags, set/cleared wherever the refs are today (declarations near line 211, _tvRenderFromBundle 657/665/677/703, renderFinished 588-589, error 610-611, _teardown 895-896):
let _tvPendingActive = false; // a build is in flight for _tvPendingNotesRef
let _tvFailedActive = false; // _tvFailedNotesRef holds a real failureAlso worth tracking chart identity with a monotonic counter rather than bundle.notes identity if the host can hand you a notes-less bundle for a real chart.
🤖 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 990 - 994, Separate ref presence from ref value in
the chart-build guards around _tvPendingNotesRef and _tvFailedNotesRef: add
paired active flags, set them whenever a build starts or fails, and clear them
on successful completion and teardown. Update buildInFlight and previouslyFailed
in _tvRenderFromBundle to use those flags, while preserving the notesRef
comparisons for non-null values; also ensure notes-less bundles receive distinct
chart identity so consecutive null notes can trigger a rebuild.
| const len = (typeof length === 'number' && length > 0) ? length : 60.0; | ||
| return { | ||
| startTime: 0.0, | ||
| endTime: len, | ||
| numBeats: 4, | ||
| beatTimes: Array.from({ length: 8 }, (_, i) => i * 0.5), | ||
| bpm: 120.0, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
numBeats (4) and beatTimes (8 entries, spanning only 0-3.5 s) disagree, so fallback measures collapse note onsets.
createBeats sizes the measure as numBeats * SUBDIV = 32 slots, but quantizeThirtySecond walks all 8 beatTimes and can return up to 63 — clamped to slot 31. Combined with beatTimes covering 3.5 s of a len-second measure, every event past the 4th beat piles onto the last slot.
🐛 Make beatTimes match numBeats and span the measure
return {
startTime: 0.0,
endTime: len,
numBeats: 4,
- beatTimes: Array.from({ length: 8 }, (_, i) => i * 0.5),
+ beatTimes: Array.from({ length: 4 }, (_, i) => (len * i) / 4),
bpm: 120.0,
};test/chart-quantize.test.mjs only asserts endTime/numBeats/bpm, so this needs no test change.
📝 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.
| const len = (typeof length === 'number' && length > 0) ? length : 60.0; | |
| return { | |
| startTime: 0.0, | |
| endTime: len, | |
| numBeats: 4, | |
| beatTimes: Array.from({ length: 8 }, (_, i) => i * 0.5), | |
| bpm: 120.0, | |
| }; | |
| const len = (typeof length === 'number' && length > 0) ? length : 60.0; | |
| return { | |
| startTime: 0.0, | |
| endTime: len, | |
| numBeats: 4, | |
| beatTimes: Array.from({ length: 4 }, (_, i) => (len * i) / 4), | |
| bpm: 120.0, | |
| }; |
🤖 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 `@src/chart-quantize.js` around lines 71 - 78, Update the fallback beat
configuration in the returned object so beatTimes matches numBeats and spans the
full len-second measure using the expected subdivision spacing. Keep numBeats at
4 and ensure the generated beatTimes contains the corresponding entries across
the measure rather than the hard-coded 0–3.5 second range.
| if (wire.s < 0 || wire.s >= stringCount) continue; | ||
| if (seen.has(wire.s)) continue; | ||
| seen.add(wire.s); | ||
| const note = new atModel.Note(); | ||
| // RS string index 0 = lowest; alphaTab Note.string 1 = lowest. | ||
| note.string = wire.s + 1; | ||
| note.fret = wire.f; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The bounds check doesn't catch the case it was written for: undefined/NaN/fractional wire.s.
undefined < 0 and undefined >= stringCount are both false (as are all NaN comparisons), so a missing or non-numeric string index sails through and produces note.string = NaN — the exact NaN-pitch corruption the comment above says is being prevented. A fractional s (e.g. 2.5) also passes. wire.f has no guard at all, so a missing fret yields a NaN realValue.
🐛 Proposed fix
for (const wire of wireNotes) {
- if (wire.s < 0 || wire.s >= stringCount) continue;
+ if (!Number.isInteger(wire.s) || wire.s < 0 || wire.s >= stringCount) continue;
+ if (!Number.isFinite(wire.f)) continue;
if (seen.has(wire.s)) continue;Worth a test/score-builder.test.mjs case with { s: undefined } / { s: 2.5 } alongside the existing out-of-range test.
📝 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.
| if (wire.s < 0 || wire.s >= stringCount) continue; | |
| if (seen.has(wire.s)) continue; | |
| seen.add(wire.s); | |
| const note = new atModel.Note(); | |
| // RS string index 0 = lowest; alphaTab Note.string 1 = lowest. | |
| note.string = wire.s + 1; | |
| note.fret = wire.f; | |
| if (!Number.isInteger(wire.s) || wire.s < 0 || wire.s >= stringCount) continue; | |
| if (!Number.isFinite(wire.f)) continue; | |
| if (seen.has(wire.s)) continue; | |
| seen.add(wire.s); | |
| const note = new atModel.Note(); | |
| // RS string index 0 = lowest; alphaTab Note.string 1 = lowest. | |
| note.string = wire.s + 1; | |
| note.fret = wire.f; |
🤖 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 `@src/score-builder.js` around lines 107 - 113, Strengthen the validation
before constructing the Note in the wire-processing loop: require wire.s to be a
finite integer within [0, stringCount), and require wire.f to be a valid finite
fret value before assigning either field. Preserve the existing seen filtering
and add score-builder tests covering undefined/fractional string indices and
missing or invalid frets alongside the out-of-range case.
| assert.equal(score.title, 'My Song'); | ||
| assert.equal(score.artist, 'Someone'); | ||
| assert.equal(score.tracks[0].name, 'Lead'); | ||
| assert.equal(score.tempo, 120); | ||
| assert.deepEqual(score.tracks[0].staves[0].tuning, [64, 59, 55, 50, 45, 40]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is Score.tempo pre-initialised (making the current assertion vacuous)?
curl -sL "https://cdn.jsdelivr.net/npm/@coderline/alphatab@1.8.2/dist/alphaTab.js" -o /tmp/at.js
grep -noP 'tempo\s*=\s*\d+' /tmp/at.js | head -20
grep -oP 'tempoAutomations' /tmp/at.js | wc -lRepository: jphinspace/feedBack-plugin-tabview
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidates =="
fd -a 'score-builder|package.json|*.test.mjs' . | sed 's#^\./##' | head -80
echo "== package deps =="
if [ -f package.json ]; then
cat package.json | sed -n '1,160p'
fi
echo "== test file excerpt =="
if [ -f test/score-builder.test.mjs ]; then
wc -l test/score-builder.test.mjs
cat -n test/score-builder.test.mjs | sed -n '1,90p'
fi
echo "== buildScoreFromBundle location =="
rg -n "buildScoreFromBundle|tempoAutomations|score\.tempo|masterBar\.tempo" -S .Repository: jphinspace/feedBack-plugin-tabview
Length of output: 533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, re, urllib.request, pathlib, sys
# Inspect the bundled/minified UMD source for Score tempo initialization and tempoAutomation handling.
for url in (
"https://cdn.jsdelivr.net/npm/@coderline/alphatab@1.8.2/dist/alphaTab.js",
"https://cdn.jsdelivr.net/npm/@coderline/alphatab@1.8.2/dist/alphaTab.min.js",
):
print(f"\n== {url} ==")
text = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
hits = re.finditer(r"tempo\s*[=<>!]=\s*(\d+)", text)
matched = list(hits) if matched is None else matched
print("tempo=NUMBER hits:", len([m for m in hits]) if False else "SKIP")
PYRepository: jphinspace/feedBack-plugin-tabview
Length of output: 2845
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files =="
git ls-files | sed -n '1,200p'
echo "== package/imports =="
for f in package.json test/score-builder.test.mjs src/score-builder.js src/score-builder.ts; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,140p' "$f" | cat -n
fi
done
echo "== tempo references =="
rg -n "buildScoreFromBundle|score\.tempo|masterBar\.tempoAutomations|tempoAutomations|tempo:" -S . || trueRepository: jphinspace/feedBack-plugin-tabview
Length of output: 19994
🏁 Script executed:
#!/bin/bash
set -e
echo "== alphaTab bundle search with SSL options =="
for url in \
"https://cdn.jsdelivr.net/npm/@coderline/alphatab@1.8.2/dist/alphaTab.js" \
"https://cdn.jsdelivr.net/npm/@coderline/alphatab@1.8.2/dist/alphaTab.min.js"
do
echo "--- $url ---"
curl -fsSL "$url" > /tmp/at_bundle.js
python3 - <<'PY' /tmp/at_bundle.js
import sys
import re
path = sys.argv[1]
text = open(path, encoding="utf-8", errors="replace").read()
print("length:", len(text))
for m in re.finditer(r"\btempo\b", text):
start = max(0, m.start()-120); end = min(len(text), m.end()+120)
print(f"\n@ {m.start()}:\n{text[start:end].replace(chr(10), ' ')}")
for m in re.finditer(r"\btempoAutomations\b", text):
start = max(0, m.start()-180); end = min(len(text), m.end()+180)
print(f"\nautomation @ {m.start()}:\n{text[start:end].replace(chr(10), ' ')}")
PY
doneRepository: jphinspace/feedBack-plugin-tabview
Length of output: 50390
Assert the tempo automation instead of score.tempo.
buildScoreFromBundle writes tempo as masterBar.tempoAutomations; Score.tempo falls back to 120 only when the first master bar has no tempo automation, so this assertion can pass even if tempo parsing is broken. Checking the created automation exercises the real builder output.
🤖 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 `@test/score-builder.test.mjs` around lines 31 - 35, Update the tempo assertion
in the score builder test to inspect the tempo automation created on the first
master bar by buildScoreFromBundle, rather than asserting score.tempo. Verify
the automation contains the expected tempo value of 120 while leaving the other
score assertions unchanged.
1 participant
Summary by CodeRabbit