fix(sloppak/ws_highway): drums stay in picker and show correct hit count

[0-Scr4tch-0]

What

Fixes drums disappearing from the arrangement picker when imported alongside other instruments (e.g. bass), and shows the correct note count instead of "Drums (0)".

The editor strips drum arrangements from the manifest and converts them to drum_tab.json, so a saved drum+bass sloppak has no drum arrangement — only bass plus a drum_tab. The sloppak loader's placeholder-creation trigger was "arrangements list is empty", which only fired for drum-only packs. Broadened to "no drum arrangement exists" so drums get a placeholder even when other instruments are present. The placeholder arrangement also now displays the actual drum_tab hit count instead of 0.

feedpak surface

• This PR does not change how the app reads/writes feedpaks (manifest keys, pack files, folder layout)

Checklist

• CHANGELOG.md [Unreleased] updated (user-visible changes)
• Tests added/updated for new behaviour
• Commits are DCO signed off (git commit -s)

Summary by CodeRabbit

• Bug Fixes
  • Ensured drum tab data results in a “Drums” placeholder arrangement when no existing drum/percussion arrangement is present.
  • Fixed displayed note counts for drum-only arrangements to match the underlying drum hit totals.
  • Improved song duration calculation when duration metadata is missing/invalid by deriving it from available drum timing information.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Drum-only song metadata

Layer / File(s)	Summary
Drum arrangement placeholder detection lib/sloppak.py	load_song() now adds the “Drums” placeholder when drum data exists and no existing arrangement name begins with drum, case-insensitively.
Drum hit count in WebSocket metadata lib/routers/ws_highway.py	Arrangement metadata now reports drum hit counts for arrangements with no notes or chords while preserving the existing calculation otherwise.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: topkoa, byrongamatos, chrisbewithyou

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the main changes to sloppak and ws_highway.
Description check	✅ Passed	The description matches the template well, with What, feedpak surface, and Checklist sections covered.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/gp_drum_import

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 479-494: Update the notes-count override in the arr_list
construction to apply only when the current arrangement is the drum arrangement,
while retaining the existing empty-arrangement and _dt_hit_count checks. Leave
note counting unchanged for non-drum arrangements such as Vocals or Keys.

🪄 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: bed572ff-4eb5-4a31-91a5-9ca1d21c9176

📥 Commits

Reviewing files that changed from the base of the PR and between 35c0d0e and db04c4b.

📒 Files selected for processing (2)

• lib/routers/ws_highway.py
• lib/sloppak.py

[coderabbitai]

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)

1256-1276: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align arrangement naming logic with extract_meta and improve condition readability.

This function has a couple of minor issues that affect accurate metadata extraction:

1. Inconsistent naming fallback: extract_meta (lines 1287) uses entry.get("id") as a fallback when name is missing, but this function only checks name. An arrangement with "id": "bass" and no name will be recognized as Bass by the library index but skipped here, causing its tuning to be ignored.
2. Null handling: If the arrangement JSON contains {"name": null}, entry.get("name", "") returns None, resulting in str(None) which evaluates to the literal string "none".
3. Readability: The inline conditional name == role if match_exact else role in name is valid Python but difficult to read.

Consider aligning the fallback logic, properly handling null values, and using parentheses to clarify the conditional evaluation.

♻️ Proposed fix

     for match_exact in (True, False):
         for entry in arrangements_manifest:
-            name = str(entry.get("name", "")).lower()
+            name_val = entry.get("name", entry.get("id", ""))
+            name = str(name_val or "").lower()
             tun = entry.get("tuning")
             if not (tun and isinstance(tun, list)):
                 continue
-            if name == role if match_exact else role in name:
+            if (name == role) if match_exact else (role in name):
                 return list(tun)
     return None

🤖 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 1256 - 1276, Update _role_tuning_for_meta to
derive the arrangement label from name, falling back to id when name is absent
or null, and normalize it without converting None to "none". Rewrite the match
condition with explicit parentheses so exact matching and containment matching
are unambiguous, while preserving the existing exact-first then loose-search
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.

Outside diff comments:
In `@lib/sloppak.py`:
- Around line 1256-1276: Update _role_tuning_for_meta to derive the arrangement
label from name, falling back to id when name is absent or null, and normalize
it without converting None to "none". Rewrite the match condition with explicit
parentheses so exact matching and containment matching are unambiguous, while
preserving the existing exact-first then loose-search behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5e48d334-18e9-4062-98d6-92b80637f85b

📥 Commits

Reviewing files that changed from the base of the PR and between db04c4b and 196008a.

📒 Files selected for processing (2)

• lib/routers/ws_highway.py
• lib/sloppak.py

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/routers/ws_highway.py

[gionnibgud]

Evaluated the PR together with the roadmap report. Verdict: correct fix, right layer — reader-side compensation in lib/, zero feedpak surface touched — and it's future-proof against everything currently planned. Two blockers before merge.

Future-proofing (checked against #61, the parts-model note, and the editor work)

• parts[]: drum_tab is explicitly preserved as the single-part projection, so this code keeps operating on the same data. Once a real drum part exists in the arrangement list, the placeholder trigger simply stops firing — the code goes dormant for new packs and keeps serving legacy ones. No revert needed.
• feedpak-spec#61: binds sound to the song-level drum_tab anchor because drums have no arrangement entry — it assumes exactly the world this PR fixes. This PR is a prerequisite for it, not a conflict.
• Editor drums-as-arrangements: host-side runtime model in the editor plugin, gated on a spec FEP; the editor still strips drums to drum_tab on save, so packs reaching this loader don't change shape.

Blockers

1. Checklist doesn't match the diff. "Tests added/updated" and "CHANGELOG updated" are checked, but the PR is two source files — no test, no CHANGELOG entry. tests/test_sloppak_drumtab_load.py already has the _write_dir_sloppak fixture; two cases cover this PR (~10 lines): a drum+bass pack grows a trailing Drums arrangement, and a pack with an existing drum-named arrangement does not grow a duplicate. Please also cover the ws_highway count-override branch — that's the part that will silently rot untested. And add the [Unreleased] CHANGELOG entry.
2. Drop the keyword sniff from ws_highway — the loader already knows which arrangement is the placeholder. Instead of duplicating _DRUM_KEYWORDS + the substring loop in lib/routers/ws_highway.py, have load_song() record the index it appends (e.g. drum_placeholder_index: int | None on LoadedSloppak — note arrangement_ids[i] is None won't do as the marker, since a manifest entry missing an id also lands None). The count override then becomes i == loaded_slop.drum_placeholder_index: deterministic, no name-guessing, no risk of lighting up an unrelated empty track. That leaves exactly one sniff — the loader's "does a drum arrangement already exist?" check — which should live as is_drum_arrangement_name(name) in lib/drums.py. Name-guessing there is unavoidable today (manifest arrangement entries carry no instrument field), and becomes a one-line key check inside that helper once parts[] / feedpak-spec#56 grants one. Keep the keyword list conservative: a false negative costs a cosmetic duplicate placeholder, a false positive re-hides drums — the original bug.

One reframing request on the report

The report calls the editor's manifest-stripping the "root cause". It isn't a defect — drum_tab is the spec's home for drums, so arrangements[] should exclude them. The loader's synthesized placeholder is the correct projection of song-level drum data into the arrangement list (the same projection parts[] will later formalize), not a patch over editor misbehavior. Worth stating it that way so nobody "fixes" the editor later.

Follow-up filed

#1027 — extract_meta() still reads only manifest["arrangements"], so the library index/instrument filters won't show drums for a drum+bass pack even though the picker now does. Out of scope for this PR; the issue includes the fix shape (reuse the same lib/drums.py helper — never a third copy of the sniff).

With the tests, the CHANGELOG entry, and the placeholder-index + shared-helper change, this is a merge.

[byrongamatos]

Review — please rebase & rework before this can merge (do not merge as-is)

Thanks for this — the two problems it targets are real, but the branch forked before #1020 (multi-drum loader) landed on main, and once rebased it collides with #1020's model. Details:

1. Stale base → semantic revert of #1020 (blocker). This branch's merge-base is a9be210 (pre-#1020/#1019), so the PR diff looks clean but hides that main has since rewritten the same two files. Trial-merging onto current main auto-merges with no textual conflict but then fails #1020's grading-invariant test tests/test_sloppak_drum_parts.py::test_pointer_entries_never_become_fretted_arrangements (a Lead + 2 drum-pointer manifest expects song.arrangements == ["Lead"]; with this change it becomes ["Lead", "Drums"]). #1020 deliberately keeps type:"drums" pointer entries out of song.arrangements and surfaces them via song_info.drum_parts; the name-based "Drums" placeholder here re-injects one for exactly the multi-part case #1020 already handles → drums double-surface (once in the main picker collapsing all parts to part0, once via the drum_parts sub-picker in #1028).

2. The single-drum + melodic gap is genuine, but the fix is too broad. For one drum part + a melodic arrangement, #1020 puts a single entry in drum_parts while the drum_part picker (#1028) only engages for len > 1, so that lone drum part isn't selectable. Worth fixing — but scope it to that case via drum_parts, not by appending a placeholder to song.arrangements on a name match. The hit-count root cause (0-note placeholder → "Drums (0)") is correctly diagnosed.

3. Missing before merge: no CHANGELOG [Unreleased] entry, and no loader test.

To land this

• Rebase onto current main (post-feat(sloppak): load multiple drum parts (feedpak 1.17.0 drums-as-arrangements) #1020/fix(song): make bass detection instrument-type-aware, not name-only #1019).
• Re-express the single-drum+melodic fix in terms of drum_parts (don't add to song.arrangements); ideally also populate per-part hit counts in the drum_parts payload so the sub-picker shows counts.
• Add a CHANGELOG entry + a loader test, and confirm test_pointer_entries_never_become_fretted_arrangements stays green.

Happy to re-review once rebased. 🙏