feat(sloppak): load multiple drum parts (feedpak 1.17.0 drums-as-arrangements)#1020
Merged
Conversation
…ngements)
A song can now ship SEVERAL drum charts (a second drummer, an aux-percussion
layer). The Arrangement Editor already writes them per the feedpak 1.17.0 FEP
(feedpak-spec#63): the primary stays the song-level `drum_tab:` key (what this
app has always played), and each part rides the manifest as a `type: drums`
arrangement entry carrying a per-arrangement `drum_tab` file pointer and NO
note `file` — an entry this loader's file/notation gate already skips, which
is exactly why old builds are unaffected by such packs.
lib/sloppak.py:
- The arrangements loop collects drum-part pointer entries instead of merely
skipping them — but still NEVER turns one into a fretted Arrangement. That
skip is the grading invariant (an empty drum chart must not reach the
fretted pipeline / note-detection grading) and is now pinned by test.
- New `LoadedSloppak.drum_parts`: [{id, name, drum_tab}], primary FIRST. The
entry aliasing the song-level file contributes its id/name but is never
loaded twice (the primary's payload IS `loaded.drum_tab`, same object).
Legacy single-drum packs read as a one-part list; a pointer-only pack (a
writer omitted the alias) promotes its first part so has_drum_tab, the
default stream, and the drum-only placeholder keep working.
- The song-level drum_tab loading block is extracted verbatim into
`_load_drum_tab_file()` and shared by both paths, so every part gets the
same permissive posture: missing file → that part silently absent;
traversal / parse / validation failure → that part skipped with a warning,
never an aborted load. (The 9 pinned drumtab-load tests pass unchanged.)
lib/routers/ws_highway.py:
- `song_info` gains `drum_parts` (names only; always a list, empty without
drums) so a part picker can bind unconditionally.
- `?drum_part=<id>` on the WS URL selects which part's tab streams as the
`drum_tab`/`drum_hits` messages; the default and any unknown id fall back
to the primary — byte-identical legacy behavior. The `drum_tab` message
carries `part_id` only when a parts list exists, keeping the legacy frame
unchanged.
Tests: tests/test_sloppak_drum_parts.py (9) — the grading invariant +
parallel-ids pin, primary-first resolution with alias identity, legacy
one-part list, pointer-only promotion, per-part failure isolation (bad JSON,
path traversal, duplicate rels), and the drum-only placeholder with pointer
entries. Full suite: the only failures are 9 machine-environmental tests
(installed desktop plugins under LOCALAPPDATA, CRLF/path-shape assertions)
that fail identically on an untouched origin/main checkout on this box.
tools/check_spec_conformance.py passes against the spec's current HEAD
(`drum_tab` and `type` are declared keys); the semantics of the
per-arrangement placement land in feedpak-spec#63 — this PR should merge
after it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
Contributor
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSloppak loading now represents multiple drum parts separately from fretted arrangements. The highway WebSocket advertises available parts, accepts a selected part id, and streams the corresponding drum tab. Tests cover resolution, fallback, validation, deduplication, and drum-only packs. ChangesMultiple drum parts
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant highway_ws
participant load_song
participant DrumTabFiles
Client->>highway_ws: Connect with drum_part
highway_ws->>load_song: Load sloppak
load_song->>DrumTabFiles: Resolve drum-tab references
DrumTabFiles-->>load_song: Validated drum parts
load_song-->>highway_ws: LoadedSloppak with drum_parts
highway_ws-->>Client: song_info and selected drum_tab
Possibly related PRs
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
22-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOrphaned bullet continuation — Line 23 dangles without its lead-in.
The new drum-parts bullet (11-22) ends with a period, then Line 23 abruptly continues with "coordinator. Synchronous transforms run after difficulty filtering..." — this is leftover tail text from the removed "chart-transform" bullet whose opening sentence was deleted. As written, this renders as an incoherent fragment attached to the wrong bullet.
📝 Proposed fix
- by test. Legacy single-drum packs read exactly as before, as a one-part list. - coordinator. Synchronous transforms run after difficulty filtering; host + by test. Legacy single-drum packs read exactly as before, as a one-part list. +- **Chart-transform coordinator.** Synchronous transforms run after difficulty filtering; host(Restore the deleted lead-in for the chart-transform bullet — exact wording TBD — rather than leaving lines 23-28 attached to the drum-parts bullet.)
🤖 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 `@CHANGELOG.md` around lines 22 - 28, Restore a complete lead-in and separate changelog bullet for the chart-transform behavior before the continuation beginning “coordinator.” Ensure the drum-parts bullet ends cleanly, and preserve the existing chart-transform details about synchronous transforms, provider isolation, sorting, fallback behavior, renderer metadata, song info, and provider selection.
🧹 Nitpick comments (2)
lib/sloppak.py (2)
927-979: 📐 Maintainability & Code Quality | 🔵 TrivialConsider extracting this block into a dedicated helper.
The primary/alias/dedup/promotion resolution is dense (flagged by the line-range complexity hint) and buried inside an already-long
load_song(). Pulling it into a_resolve_drum_parts(source_dir, drum_tab_rel, drum_tab_data, drum_pointer_entries)helper alongside_load_drum_tab_file()would isolate it for independent unit testing and shrinkload_song().🤖 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 `@lib/sloppak.py` around lines 927 - 979, Extract the drum-part resolution block from load_song() into a dedicated _resolve_drum_parts(source_dir, drum_tab_rel, drum_tab_data, drum_pointer_entries) helper near _load_drum_tab_file(). Preserve the existing primary alias matching, deduplication, extra-part loading, pointer-only promotion, and drum_parts/drum_tab results, then replace the inline logic in load_song() with a call to the helper.
811-819: 📐 Maintainability & Code Quality | 🔵 TrivialSilent drop for
drum_tab-bearing entries with the wrong/missingtype.An entry with no
file/notationand a stringdrum_tabbut atypeoutside{"drums","drum"}(or missing) is dropped with no log — unlike every other malformed-entry path in this loop (e.g. Line 804), which warns. A pack author mistypingtypewould get silently-missing drums with no diagnostic.🔎 Proposed diagnostic
_etype = str(entry.get("type") or "").strip().lower() if _etype in ("drums", "drum") and isinstance(entry.get("drum_tab"), str): drum_pointer_entries.append(entry) + elif isinstance(entry.get("drum_tab"), str): + log.warning( + "sloppak: arrangement entry has drum_tab %r but type=%r — ignored", + entry.get("drum_tab"), entry.get("type"), + ) continue🤖 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 `@lib/sloppak.py` around lines 811 - 819, Update the entry-filtering logic around the _etype and drum_pointer_entries handling to detect entries with a string drum_tab but no file/notation when type is missing or outside “drums”/“drum”. Emit the same warning used by the malformed-entry path near line 804, then prevent the invalid entry from being silently dropped while preserving valid drum-pointer collection and the fretted-pipeline skip.
🤖 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 `@lib/routers/ws_highway.py`:
- Around line 606-634: Update the drum-tab message construction around
_dt_part_id and the loaded_slop.drum_parts check so part_id is added to _dt_msg
only when the pack contains multiple drum parts. Preserve the existing
primary-part selection and ensure legacy packs with a synthesized single-element
drum_parts list omit part_id entirely.
In `@lib/sloppak.py`:
- Around line 925-979: Ensure resolved drum part IDs are unique before appending
entries to _extra_parts or _parts, covering both explicit IDs and generated
drums-N fallbacks. Update the ID generation in the drum_parts resolution block
to detect collisions and assign the next unused identifier, so drum_part
selection remains unambiguous.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 22-28: Restore a complete lead-in and separate changelog bullet
for the chart-transform behavior before the continuation beginning
“coordinator.” Ensure the drum-parts bullet ends cleanly, and preserve the
existing chart-transform details about synchronous transforms, provider
isolation, sorting, fallback behavior, renderer metadata, song info, and
provider selection.
---
Nitpick comments:
In `@lib/sloppak.py`:
- Around line 927-979: Extract the drum-part resolution block from load_song()
into a dedicated _resolve_drum_parts(source_dir, drum_tab_rel, drum_tab_data,
drum_pointer_entries) helper near _load_drum_tab_file(). Preserve the existing
primary alias matching, deduplication, extra-part loading, pointer-only
promotion, and drum_parts/drum_tab results, then replace the inline logic in
load_song() with a call to the helper.
- Around line 811-819: Update the entry-filtering logic around the _etype and
drum_pointer_entries handling to detect entries with a string drum_tab but no
file/notation when type is missing or outside “drums”/“drum”. Emit the same
warning used by the malformed-entry path near line 804, then prevent the invalid
entry from being silently dropped while preserving valid drum-pointer collection
and the fretted-pipeline skip.
🪄 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: 32042956-5b4b-4531-923b-514debd1a307
📒 Files selected for processing (4)
CHANGELOG.mdlib/routers/ws_highway.pylib/sloppak.pytests/test_sloppak_drum_parts.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/sloppak.py (1)
876-891: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrum-pointer entries should be gated by
typefirst
if not rel and not has_notation_keylets atype: drumsentry with a straynotationstring bypass the drum-pointer path and fall through toarrangement_from_wire(...), appending an empty fretted arrangement instead of keeping the part out of the fretted pipeline. That breaks the stated invariant.
type: drumswith a missing/invaliddrum_tabis still dropped silently; add a warning there too.🤖 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 `@lib/sloppak.py` around lines 876 - 891, Update the arrangement-entry handling before the rel/has_notation_key branching so type values drums or drum always use the drum-pointer path and never reach arrangement_from_wire. Append only valid string drum_tab entries to drum_pointer_entries; warn and drop drum entries with missing or invalid drum_tab. Preserve existing handling for non-drum entries and stray drum_tab values.Source: Path instructions
🤖 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 `@lib/sloppak.py`:
- Around line 793-816: Normalize both the pointer entry path and the top-level
drum_tab path before comparing them in the primary-match logic around
_load_drum_tab_file. Use the repository’s existing path-normalization convention
if available, so equivalent spellings such as "drum_tab.json" and
"./drum_tab.json" match; retain the existing primary id/name overrides and
prevent the matched entry from being appended to extra_parts.
---
Outside diff comments:
In `@lib/sloppak.py`:
- Around line 876-891: Update the arrangement-entry handling before the
rel/has_notation_key branching so type values drums or drum always use the
drum-pointer path and never reach arrangement_from_wire. Append only valid
string drum_tab entries to drum_pointer_entries; warn and drop drum entries with
missing or invalid drum_tab. Preserve existing handling for non-drum entries and
stray drum_tab values.
🪄 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: 17d4de7b-6c00-4e45-aedc-8e18402c3505
📒 Files selected for processing (5)
CHANGELOG.mdlib/routers/ws_highway.pylib/sloppak.pytests/test_highway_ws_drum_parts.pytests/test_sloppak_drum_parts.py
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- lib/routers/ws_highway.py
- tests/test_sloppak_drum_parts.py
- Gate the drum-pointer skip on type FIRST: a type:drums/drum entry never becomes a fretted Arrangement even if it carries a note file/notation (with drum_tab it is collected as a drum part, without it dropped+warned). Closes the spec §5.2/§7.5 MUST-NOT hole (a malformed drums+file entry was being fretted-graded). - Make test_drum_pointer_with_wrong_type_logs_warning robust (attach handler to the feedBack logger + set WARNING, restore in finally) and fix the root-cause level leak in test_tuning_provider_isolation.py (finally restored the handler but not the level, leaking ERROR onto the feedBack tree and turning the suite red under full ordering). - Restore the chart-transform CHANGELOG bullet (#952) the drum entry had truncated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
pushed a commit
that referenced
this pull request
Jul 21, 2026
…1021) (#1028) * feat(player): drum-part picker for multiple drum charts (feedpak 1.17.0) The last mile of the multiple-drum-parts feature: let a player CHOOSE which drum chart plays. #1020 taught the loader + highway WS to carry several drum parts (song_info.drum_parts + ?drum_part=<id> + a part_id echo on drum_tab); this adds the host-chrome selector that drives it. A "Drum part" <select> sits beside the arrangement switcher in the advanced settings popover, shown only when a song has 2+ drum charts (drum_parts is always present — empty for non-drum songs — so single-drum / no-drum songs hide the row and nothing changes for them). Selecting a part re-streams that part's tab over the highway WS, exactly like an arrangement switch. - static/highway.js: - reconnect() gains a third `drumPart` arg → sets `?drum_part=<id>` on the WS URL (mirrors the existing `arrangement` param one line up). Empty/undefined → the primary part, i.e. byte-identical to today for any pack untouched. - song_info handler populates #drum-part-select from msg.drum_parts and shows/hides #v3-drum-part-row on `length > 1` (parallel to the #arr-select block right above it). - drum_tab handler carries msg.part_id onto hwState.drumTab (plugins can read bundle.drumTab.part_id) and reflects it as the picker's selected value, so the dropdown stays honest even when the server resolves an unknown/absent selection to the primary. - static/app.js: - changeArrangement() gains an optional `drumPart`; at reconnect it forwards the explicit part, else preserves the current picker selection — so an ARRANGEMENT switch keeps the chosen drum part (parts are song-level). - new changeDrumPart(id) delegates to changeArrangement with the current arrangement held + the new part applied (a part switch is the same re-stream, so it reuses all the transition ceremony). Exported on window. - static/v3/index.html: the #drum-part-select row (hidden by default). No plugin change: the drum renderers just draw whatever drum_tab streams. RUNTIME-VERIFIED (Playwright, the core player, a 2-drum pack + a no-drum pack): 10/10 — the picker populates with both parts and shows for the multi-drum song; song_info.drum_parts reaches getSongInfo(); the primary is pre-selected; selecting the 2nd part drives highway.reconnect with the id and the WS URL carries `?drum_part=drums-2`; the picker then reflects the server's part_id echo; a no-drum song hides the row; no page errors. ESLint 0 errors (the two max-lines warnings are pre-existing on these files). No pytest touched (JS-only). Stacked on #1020. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> * Update reconnect source contract test --------- Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The core half of the multiple-drum-parts program. The Arrangement Editor now writes packs with several drum charts per the feedpak 1.17.0 FEP (got-feedBack/feedpak-spec#63, editor side: got-feedBack/feedBack-plugin-editor#339): the primary stays the song-level
drum_tab:key, and each part rides the manifest as atype: drumsarrangement entry with a per-arrangementdrum_tabpointer and no notefile— an entry this loader's file/notation gate already skips, which is exactly why old builds are unaffected. This PR teaches core to read those parts.What it does
lib/sloppak.py— collects the pointer entries (still never loading one as a fretted Arrangement: that skip is the grading invariant — an empty drum chart must not reach the fretted pipeline / note-detection grading — now pinned by test) and resolves them intoLoadedSloppak.drum_parts:[{id, name, drum_tab}], primary first. The entry aliasing the song-level file contributes its id/name but is never loaded twice (the primary's payload isloaded.drum_tab). Legacy single-drum packs read as a one-part list; a pointer-only pack promotes its first part sohas_drum_tab/ the default stream / the drum-only placeholder keep working. The song-level loading block is extracted verbatim into_load_drum_tab_file()and shared, so every part gets the same permissive posture (the 9 pinned drumtab-load tests pass unchanged).lib/routers/ws_highway.py—song_infogainsdrum_parts(names only; always a list) so a part picker can bind unconditionally;?drum_part=<id>on the WS URL selects which part's tab streams (drum_tabmessages carrypart_idonly when a parts list exists — the legacy frame stays byte-identical; unknown ids fall back to the primary).What it deliberately does NOT do
No picker UI (that's the drum-highway / visualization plugins' side, using
song_info.drum_parts+ thedrum_partparam), no library-index changes, no scoring changes.Verification
tests/test_sloppak_drum_parts.py(9): the grading invariant + parallel-ids pin, primary-first resolution with alias identity, legacy one-part list, pointer-only promotion, per-part failure isolation (bad JSON, path traversal, duplicate rels), drum-only placeholder with pointer entries.test_sloppak_drumtab_load.py9/9 unchanged (the helper extraction is behavior-identical).%LOCALAPPDATA%, CRLF/path-shape assertions) that fail identically on an untouched origin/main checkout.tools/check_spec_conformance.pypasses against the spec's current HEAD (drum_tabandtypeare already declared key names).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
drum_tabloading and exposure with IDs and names.drum_tabis sent (includingpart_idwhen applicable).Bug Fixes
Tests
Documentation