Conversation
Adds explicit `type` field to WebSocket song_info and arrangements, allowing viz auto-selection to match on real instrument type instead of name-sniffing. Fixes misclassification of keyboard arrangements (e.g., GP imports with piano parts labeled 'Combo') by checking manifest `type` before arrangement name patterns. Reorders GP track classification to yield keyboard parts before assuming guitar. Also adds `_stemsRerouteInProgress` guards matching `_juceRerouteInProgress` to prevent spurious play/pause events during stems plugin Web-Audio takeover.
drawChordDiagram() was passed inverted: _invertedCached at both call sites, flipping its column order (high-e/low-E swapped) whenever the highway's Invert toggle was on. The diagram's orientation should be fixed regardless of that toggle, so both sites now pass inverted: false. Note: plugins/highway_3d/CLAUDE.md had documented the mirroring as this overlay's contract, but that line traces only to a single squashed "Clean release snapshot" commit with no surviving design rationale -- treated here as an inaccurate description of a bug, not a protected feature, and updated accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Update the bundled `highway_3d` plugin version from 3.34.1 to 3.34.2.
|
Important Review skippedAuto reviews are disabled on this repository. 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:
📝 WalkthroughWalkthroughChangesArrangement typing and highway selection
Highway chord orientation
Stems reroute transport handling
Repository ignore rules
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GPXSelector
participant Song
participant HighwayWebSocket
participant HighwayScreen
GPXSelector->>Song: Classify keyboard tracks as keys
Song->>HighwayWebSocket: Provide normalized arrangement type
HighwayWebSocket->>HighwayScreen: Send arrangement_type and arrangement metadata
HighwayScreen->>HighwayScreen: Reject keys before name matching
Suggested reviewers: 🚥 Pre-merge checks | ✅ 21 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 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 |
Resolves the CHANGELOG.md conflict between this branch's highway_3d chord-diagram fix and main's count-in meter fix -- both entries kept.
carochacs
changed the title
|
Fixed the Diagnosis: The
Fix: Rewrote only the subject lines via Verification:
This is the documented fix for this check (fork-maintenance policy / historical procedure) — the offending subjects had to be rewritten in place, so a new commit could not satisfy it. Task list (7/7 completed)
|
 |
|
Overall Grade Focus Area: Reliability |
Security  Reliability Complexity Hygiene |
Feedback
Type shape vs. actual usage
- A bunch of the reliability issues cluster around loose typing:
chord_to_wirecalled withint,objectused where aDict/Optional[Dict]is expected, indexingDict | None, and tuple-shape mismatches intunings.py. - All of these suggest the same thing: the logical data shape is there in your head, but the code/API signatures aren’t yet aligned with it.
Highway 3D plugin state handling
- In
screen.jsyou’ve got several related issues: unused vars, function declarations inside loops closing overpChordBox, use-before-definition (highwayCanvas), and even a=-operator. - They all point at the same fragile area: state and scoping inside this plugin are easy to get subtly wrong, which is exactly where your visual/mirroring bugs live.
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Docker |  |
Aug 16, 2026 3:10p.m. | Review ↗ |
| JavaScript |  |
Aug 16, 2026 3:10p.m. | Review ↗ |
| Python | |
Aug 16, 2026 3:10p.m. | Review ↗ |
| Shell | |
Aug 16, 2026 3:10p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
|
Checked all 3 DeepSource Python findings against the actual diff — each is a small, surgical addition to an already-large pre-existing function, not something this PR's diff drives up:
All three are legitimate architectural debt in genuinely large functions, but refactoring any of them is well out of scope for a targeted bug-fix PR (keyboard misclassification + chord-diagram mirroring + stems-transport race). All 7 real CI jobs (test, lint, manifest-validation, feedpak-spec, tailwind-fresh, core-commit-labeling, upstream-drift) are green. Generated by Claude Code |
convert_wem shells out to vgmstream-cli/ffmpeg via subprocess.run with a 120s timeout. Called bare inside async def highway_ws, it blocked the whole event loop for the duration of the conversion, stalling every other concurrent WebSocket connection on that worker. Wrap both call sites in loop.run_in_executor(), reusing the contextvars.copy_context() snapshot already taken earlier in the function so the bound ws_conn_id correlation ID still applies to log lines raised inside the executor thread (matches this file's existing pattern for load_song/sloppak_mod.load_song). Flagged by CodeRabbit on #42; pre-existing, unrelated to that PR's diff, so fixed separately here.
convert_wem shells out to vgmstream-cli/ffmpeg via subprocess.run with a 120s timeout. Called bare inside async def highway_ws, it blocked the whole event loop for the duration of the conversion, stalling every other concurrent WebSocket connection on that worker. Wrap both call sites in loop.run_in_executor(), reusing the contextvars.copy_context() snapshot already taken earlier in the function so the bound ws_conn_id correlation ID still applies to log lines raised inside the executor thread (matches this file's existing pattern for load_song/sloppak_mod.load_song). Flagged by CodeRabbit on #42; pre-existing, unrelated to that PR's diff, so fixed separately here.
|
Fixed the failing Diagnosis. The check suite's only failure was the Fix. Rewrote the commit's subject with
Remote branch Task list (7/7 completed)
|
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Commit 36d96da fails the fork's core-commit-labeling check, so this PR currently has a red CI check and can't merge as-is.
Reviewed changes — This run covers the delta since 190c60c (the last reviewed version): the convert_wem event-loop offload and its CHANGELOG entry. The rest of the PR (keyboard-arrangement classification, type/arrangement_type WS payload, highway_3d chord-diagram mirroring, stems transport guard) is unchanged from the prior review.
- Offloaded audio conversion off the event loop — Both
convert_wemcall sites inlib/routers/ws_highway.py(loose-folder and archive WEM conversion) now run throughloop.run_in_executor, reusing thecontextvars.copy_context()snapshot taken earlier in the handler so thews_conn_idcorrelation ID still propagates into the worker thread — the same pattern the handler already uses forload_song. The change is correct and idiomatic. - Documented the fix — Added a
[Unreleased]CHANGELOG entry describing the blocking-subprocess bug and the executor fix.
⚠️ Commit 36d96da fails the fork's core-commit-labeling check
The commit introducing this delta's code change touches lib/routers/ws_highway.py — a core path under the fork's maintenance policy — but its subject has no core:/hook:/sync:/fix: prefix. The core-commit-labeling job is red on this PR ("Commit 36d96da ... touches core path(s) but its subject isn't prefixed ..."), so the PR can't merge as-is. The executor change itself is sound — this is purely a commit-labeling matter.
Technical details
# Commit `36d96da` fails `core-commit-labeling`
## Affected sites
- `.github/workflows/fork-audit.yml:43` — regex `^(core|hook|sync|fix)(\([^)]*\))?:` does not match the subject "Offload convert_wem calls in highway_ws off the event loop".
- Commit `36d96dadcb683c882237ec2d81c419841365cecf` — touches `lib/routers/ws_highway.py` (core); the only PR commit the check flags.
## Required outcome
- Reword `36d96da`'s subject to carry a valid prefix (e.g. `fix: Offload convert_wem calls in highway_ws off the event loop`) and force-push the `local` branch. The check iterates every commit in `base..head`, so a follow-up commit cannot satisfy it — the offending commit itself must be rewritten (docs/fork-maintenance.md, Rule 2). `3306b47` (CHANGELOG-only, not core) can be re-parented/reworded as part of the same rewrite.
## Suggested approach
- Rewrite just the subject with `git filter-branch --msg-filter` over the base..head range, confirm the tree hashes are unchanged, then force-push `local` via the MCP push tool.Big Pickle (free) ｜ 𝕏
|
Task list (1/6 completed)
|
There was a problem hiding this comment.
Important
The core-commit-labeling check is still red on head 97dbce9, so the PR still can't merge.
Reviewed changes — This run covers the incremental delta since the prior pullfrog review (old head 3306b47, since rewritten): the convert_wem commit subject rewrite, two branch-merge commits at the head, and a single-blank-line tree change in CHANGELOG.md.
- Rewrote the
convert_wemoffload commit with a valid prefix —36d96da→c4aba80 fix: Offload convert_wem calls in highway_ws off the event loop, which now matches the fork-audit regex (and main's merged371d7a4extends rule 2 to acceptfix:). This specific prior flag is addressed. - Merged the two divergent
localhistories together —981c389and97dbce9bring the un-prefixed original commits back intobase..head, keeping the check red (see below). - Net tree change is one blank line in CHANGELOG.md — all ten files from the initial pass are otherwise unchanged.
⚠️ Un-prefixed duplicate commits reintroduced by the head merges
The two Merge branch 'local' … into local merges at the head pulled the original, un-prefixed versions of two commits back into BASE..HEAD. core-commit-labeling iterates every commit in the range (fork-audit.yml:34), and the check annotation for head 97dbce9 flags exactly:
304a050"Update .gitignore" — tree-identical to8abbb56 core: Update .gitignore(tree97d9b18…)3627fb5"Add arrangement type to WS and fix keyboard detection" — tree-identical tocb6407b fix: Add arrangement type to WS and fix keyboard detection(tree6443946…)
Since both offenders have byte-identical trees to properly-prefixed commits already in the range, a follow-up commit cannot satisfy the check — the un-prefixed lineage must leave base..head.
Technical details
# Un-prefixed duplicate commits keep `core-commit-labeling` red
## Affected sites
- Commit `304a050` ("Update .gitignore") — no `core:`/`hook:`/`sync:`/`fix:` prefix, `.gitignore` is core.
- Commit `3627fb5` ("Add arrangement type to WS and fix keyboard detection") — no prefix, touches `lib/`, `static/` (core).
- Both are in `BASE..HEAD` only because merges `981c389`/`97dbce9` joined the divergent `local` histories; the tree content already exists in-prefix as `8abbb56`/`cb6407b`.
## Required outcome
- Remove the un-prefixed duplicate lineage from `base..head` so the check goes green. Their content is fully represented by the prefixed commits, so the clean fix is to rebuild `local` on the prefixed lineage (drop the `304a050`/`3627fb5` side of the merge, keep `c4aba80`/`f111804` on top) and force-push.
- Alternatively reword `304a050` → `core:` and `3627fb5` → `fix:` using the shallow-clone-safe amend + `git commit-tree` recipe (learnings "Fixing a core-commit-labeling failure"), preserving merge topology and tree hashes, then force-push.
## Suggested approach
- Verify against the workflow's exact step after pushing: `for sha in $(git rev-list $BASE..HEAD); do … grep -qE '^(core|hook|sync|fix)(\([^)]*\))?:' …` — no commit in the range may touch a core path without a matching prefix.
## Open questions for the human
- Are the two duplicate histories expected to persist on this branch, or can `local` be collapsed onto the prefixed lineage before merge?Big Pickle (free) ｜ 𝕏
|
Fixed the Root cause: PR #42 contained two parallel lines of identical work. Line A carried the required commit prefixes ( Fix: Used Verification:
Task list (1/7 completed)
|
There was a problem hiding this comment.
Important
The new commit is clean, but the previously-flagged core-commit-labeling failure is still red on head 7b4f4e6 — the only thing standing between this PR and a green CI.
Reviewed changes — This run covers the incremental delta since the prior pullfrog review (head 97dbce9): one new commit, 7b4f4e6 "fix: restore error handling for sloppak loading in websocket handler".
- Restored clean error handling for sloppak load failures —
sloppak_mod.load_songnow runs inside atry/exceptinhighway_ws(lib/routers/ws_highway.py:212-228) that logs the real cause server-side vialog.exceptionand sends the client a generic{"error": "Failed to load sloppak"}before closing, instead of letting the exception reach the outer handler wherestr(e)(lib/routers/ws_highway.py:1159-1165) would leak filesystem paths to the client. I verified the load-bearing claims:load_songraises rather than returningNoneon failure (lib/sloppak.py:1025), the frontend already renders{error}WS messages (static/highway.js:1936-1939), andexcept Exceptiondoes not swallowasyncio.CancelledError(aBaseExceptionon the 3.12 runtime), so handler cancellation still propagates. The commit also carries a validfix:prefix, so it adds no new labeling offense.
⚠️ core-commit-labeling still failing on head 7b4f4e6 (prior flag, unchanged)
Not a new finding — this is the outstanding item from the last review, and it is still open. Confirmed against the current head via the check-run annotations: the fork-audit job still flags exactly 304a050 ("Update .gitignore") and 3627fb5 ("Add arrangement type to WS and fix keyboard detection"), both un-prefixed duplicate commits whose content already exists in the range as 8abbb56 core: … and cb6407b fix: …. ci / test, ci / lint, and the other checks are green on head, so this is the only blocker left.
Technical details
# core-commit-labeling still red
## Affected sites
- Commit `304a050` ("Update .gitignore") — touches core `.gitignore`; unprefixed subject.
- Commit `3627fb5` ("Add arrangement type to WS and fix keyboard detection") — touches `lib/` + `static/` (core); unprefixed subject.
- Both are in `base..head` only via the `981c389`/`97dbce9` merge of the divergent `local` histories; tree-identical, properly-prefixed counterparts `8abbb56`/`cb6407b` already sit in the range.
## Required outcome
- Get `304a050`/`3627fb5` out of `base..head` (rebuild `local` on the prefixed lineage and force-push), or reword their subjects to `core:`/`fix:`. A follow-up commit cannot satisfy the check — fork-audit.yml iterates every commit in `BASE..HEAD`.
## Suggested approach
- The prior review and the repo learnings carry the recipe (cherry-pick rebuild, or the shallow-clone-safe `git commit-tree` merge recreation), then force-push via the MCP push tool.ℹ️ Nitpicks
- Consider pinning the no-path-leak behavior with a test —
tests/test_ws_highway_disconnect.pyalready driveshighway_wswith a stub WebSocket; a sibling test feeding a corrupt sloppak (e.g. a non-zipbad.sloppak) and asserting the payload is exactly{"error": "Failed to load sloppak"}would fail against the pre-fix code (which sentstr(e)) and would have caught this regression directly.
Big Pickle (free) ｜ 𝕏
Trivial simplification flagged by DeepSource — (songInfo && songInfo.arrangement) || '' and songInfo?.arrangement || '' are equivalent.
2 participants
What
often notates piano/keys parts on a fretted string template, so
string_pitchesalone can't tell a keyboard part from a real guitar. GPimport track selection now checks
is_keysbeforeis_guitar, the manifesttypefield is now authoritative (over name-sniffing) in bothlib/song.py's Lead/Rhythm/Bass grouping and highway_3d'smatchesArrangement, and the highway WS payload now carriestype/arrangement_typeso the frontend can act on real instrument type insteadof guessing from the display name (fixes a keys arrangement literally named
"Combo" being swept into the guitar bucket and stealing the song from the
piano viz).
Its orientation is fixed regardless of the highway's own Invert toggle now;
both call sites pass
inverted: false.CLAUDE.mddocumentation andplugin.jsonversion bumped to match._stemsRerouteInProgressis checked the same way_juceRerouteInProgressalready is, so the deliberate pause/play the stems plugin does while
building its multi-stem transport doesn't stomp the play/pause button state.
.gitignorenow excludes the local/libraryand/static/sloppak_cacheworking directories.
feedpak surface
keys, pack files, folder layout) —
typeis a pre-existing manifest key,not a new one.
Checklist
CHANGELOG.md[Unreleased]updated (user-visible changes)git commit -s) — this PR targets thisfork's own
main, notgot-feedback/feedBackupstreamNotes
convert_wemis called directly (not viarun_in_executor) inside the asynchighway_wshandler — flagged by CodeRabbit's automated checks. Pre-existing,not touched by this PR's diff; same out-of-scope call already noted on #17.