From 65c234432b624bd32db0d4bbe3526b542b60e633 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Mon, 20 Jul 2026 16:50:34 -0500 Subject: [PATCH 1/2] feat(editor): create-mode songs can hold several drum parts too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR4 of the drums-as-arrangement arc — closes the last authoring gap. #339 gave N drums to EDIT-mode (re-open + Save via /save_song); a CREATE-mode session (New Song from a GP/MIDI import) still capped at one drum chart, because its build persists through /build → _write_sloppak_pak, which only knew a single song-level drum_tab. Now the create-mode build path reaches parity: multiple drum parts persist per feedpak-spec 1.17.0 (primary as the song-level drum_tab alias, extras as type:"drums" arrangement entries with per-arrangement drum_tab pointers — old readers see one drum, new readers all). Backend (routes.py): - New module-level `_create_build_drum_entries(staging, drum_tab, drum_parts)`: writes each extra part's drum_tab_.json into the staging dir and returns the type:"drums" manifest entries (primary alias + extras; ids sanitized to a safe filename charset and de-collided). Twin of /save_song's inline drum-parts block, sharing `_sanitize_extra_drum_tab` for one on-disk shape. Extracted to module scope (out of _write_sloppak_pak's closure) and kept PURE of the core `lib` — so pytest drives the real file write directly. - `_write_sloppak_pak` gains a `drum_parts` param; appends the entries after the pitched arrangements when the client opted in (a list, possibly empty). Absent → an old frontend → the pack stays byte-identical with just the song-level key. - `build_song_endpoint` reads + boundary-validates `drum_parts` (each part a dict with a schema-valid drum_tab, via lib.drums like /save_song — fail-fast 400) and threads it into the build. The other _write_sloppak_pak callers (save-as-sloppak) pass nothing → unaffected. Frontend: - editorBuild ships the drum payload via new pure `_drumBuildPayloadPure` (create.js): the PRIMARY drum_tab is the FIRST drums arrangement's payload, NOT the active S.drumTab (which tracks whichever part is open in the grid — the user may be editing a secondary at build time); `drum_parts` (the extras) ships only when drums are materialized as arrangements, so a legacy single-tab compose build stays byte-identical. - `_canAddAnotherDrums` drops the `!S.createMode` guard — create sessions add a 2nd part beside a pitched track now that the build persists it. The only remaining one-part case is a drums-only session (no melodic track to sit beside — drums are never index 0); the New-Track note + refusal message say so. Tests: JS 297 files green — `_drumBuildPayloadPure` (primary-not-active, empty-extras-still-ships, byte-identical fallback) + a REAL editorBuild→/build body integration test through saveCDLC (asserts the primary + extras on the wire); new_track create-mode add-a-2nd-part + the drums-only one-part guard. pytest 369 green (+5): `_create_build_drum_entries` real file write — side files + alias, empty extras, hit sanitation + name propagation, id de-collision + filename sanitization, non-list-hits sanitized-not-crashing. Lint 0 err / 3 baseline. Runtime posture: the create-mode /build write is now directly unit-tested against real file I/O (the extracted helper), the frontend build wire is integration-tested against the real editorBuild, and the on-disk shape is identical to /save_song's drum-parts persistence that #339 runtime-verified end-to-end (create parts → save → reload → both parts return). Stacked on #339. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- CHANGELOG.md | 10 ++++ routes.py | 91 ++++++++++++++++++++++++++++++ src/arrangement.js | 22 ++++---- src/create.js | 28 ++++++++- src/new-track.js | 9 +-- tests/create_save_routing.test.mjs | 63 +++++++++++++++++++++ tests/new_track.test.mjs | 23 ++++++-- tests/test_drum_parts.py | 76 +++++++++++++++++++++++++ 8 files changed, 299 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0ff283c..01536c96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Multiple drum parts now work when creating a new song too.** The several- + drum-parts feature previously covered only songs you re-open and Save; a + *create-mode* session (New Song from a Guitar Pro / MIDI import) was capped at + one drum chart. Now, once a melodic track exists, **＋ Track ▸ Drums** adds + another drum part in a create session just like an existing one, and Building + the song persists every part (the first as the song-level drum tab today's + game plays, the rest as `type: drums` arrangement entries per feedpak-spec + 1.17.0). A drums-only draft still holds one part until you add a melodic + track (drums are never the primary chart). Packs stay backward-compatible. + - **Track regions — drag a track's block to move its content.** In the Tracks view you can now grab a region block and slide it along the timeline; it snaps to bar lines (hold **Alt** for a free nudge), shows a dashed preview of where it diff --git a/routes.py b/routes.py index 307fa87a..81c92289 100644 --- a/routes.py +++ b/routes.py @@ -339,6 +339,55 @@ def _sanitize_extra_drum_tab(tab): return out +def _create_build_drum_entries(staging, drum_tab, drum_parts): + """The `type: drums` manifest arrangement entries for a CREATE-MODE build + with multiple drum parts (feedpak 1.17.0 "drums as arrangements"), and the + side effect of writing each EXTRA part's `drum_tab_.json` into + `staging`. The PRIMARY entry aliases the song-level `drum_tab.json` (already + written by the caller) — it contributes id/name only; each extra part gets + its own side file. Ids are sanitized to a safe filename charset and + de-collided. Returns the entries to append AFTER the pitched arrangements. + + Twin of /save_song's inline drum-parts block; shares + `_sanitize_extra_drum_tab` so both wire the same on-disk shape. Extracted + to module scope (out of `_write_sloppak_pak`'s closure) so pytest can drive + the write directly. Pure of the core `lib` (like `_sanitize_extra_drum_tab`): + each part's tab is schema-validated at the request boundary + (`build_song_endpoint`), so here junk hits are merely sanitized away. + """ + from pathlib import Path as _Path + + staging = _Path(staging) + entries = [{ + "id": "drums", + "name": str(drum_tab.get("name") or "Drums")[:120], + "type": "drums", + "drum_tab": "drum_tab.json", + }] + used_ids = {"drums"} + for part in (drum_parts or []): + if not isinstance(part, dict) or not isinstance(part.get("drum_tab"), dict): + continue + pid_raw = str(part.get("id") or "").strip().lower() + pid = re.sub(r"[^a-z0-9_-]+", "-", pid_raw).strip("-_") or "drums-2" + if pid in used_ids: + n = 2 + while f"{pid}-{n}" in used_ids: + n += 1 + pid = f"{pid}-{n}" + used_ids.add(pid) + name = str(part.get("name") or part["drum_tab"].get("name") or "Drums")[:120] + tab = _sanitize_extra_drum_tab(part["drum_tab"]) + tab["name"] = name + (staging / f"drum_tab_{pid}.json").write_text( + json.dumps(tab, separators=(",", ":")), encoding="utf-8") + entries.append({ + "id": pid, "name": name, "type": "drums", + "drum_tab": f"drum_tab_{pid}.json", + }) + return entries + + # Generic "field absent from request" sentinel used by the save endpoint # to distinguish "client didn't send this field" from "client explicitly # sent an empty list / null". The empty-list case is meaningful for @@ -8633,6 +8682,32 @@ async def build_song_endpoint(data: dict): {"error": "drum_tab must be a JSON object with a 'hits' array"}, 400, ) + # EXTRA drum parts (feedpak 1.17.0 "drums as arrangements"): the + # create-mode /build twin of /save_song's `drum_parts`. Absent → an + # old frontend, pack keeps just the song-level drum_tab (byte- + # identical). A list (possibly empty — the new editorBuild always + # ships one) → the authoritative extras, written beside the primary. + drum_parts = data.get("drum_parts") + if drum_parts is not None: + if not isinstance(drum_parts, list): + return JSONResponse({"error": "drum_parts must be a list"}, 400) + if drum_tab is None: + return JSONResponse( + {"error": "drum_parts requires drum_tab in the same build"}, 400) + from lib.drums import validate_drum_tab as _validate_part_tab + for _pi, _part in enumerate(drum_parts): + if not isinstance(_part, dict) or not isinstance(_part.get("drum_tab"), dict): + return JSONResponse( + {"error": f"drum_parts[{_pi}] must be an object with a drum_tab object"}, + 400, + ) + # Schema-validate at the boundary (like /save_song), so a bad + # tab fails fast with 400 rather than being sanitized to empty + # deeper in _create_build_drum_entries. + _p_ok, _p_reason = _validate_part_tab(_part["drum_tab"]) + if not _p_ok: + return JSONResponse( + {"error": f"invalid drum_parts[{_pi}].drum_tab: {_p_reason}"}, 400) def _build_sloppak(): """Build a `.sloppak` for the create-mode session. @@ -8670,6 +8745,7 @@ def _build_sloppak(): meta=meta, output_path=output, drum_tab=drum_tab if isinstance(drum_tab, dict) else None, + drum_parts=drum_parts, audio_tracks=extra_audio_tracks, audio_guide_name=(build_audio_tracks[0]["name"] if build_audio_tracks else ""), @@ -8780,6 +8856,7 @@ def _write_sloppak_pak(*, audio_file: str, art_path: str, arrangements_data: list, beats: list, sections: list, meta: dict, output_path: Path, drum_tab: dict | None = None, + drum_parts: list | None = None, lyrics: list | None = None, preview_path: str = "", fail_if_exists: bool = False, @@ -9070,6 +9147,20 @@ def _write_sloppak_pak(*, audio_file: str, art_path: str, ) manifest["drum_tab"] = "drum_tab.json" + # DRUM-PART entries (feedpak 1.17.0 "drums as arrangements"): + # a create-mode session can hold several drum charts. Written + # only when the client OPTED IN by shipping `drum_parts` (a + # list, possibly empty) — an old frontend omits it and the pack + # stays byte-identical with just the song-level key. The write + # lives in the module-level _create_build_drum_entries so pytest + # exercises it directly (this closure isn't reachable). + if drum_parts is not None: + # Append after the pitched arrangements (drums are never + # index 0 — that slot is the played chart). + manifest["arrangements"] = ( + list(manifest.get("arrangements") or []) + + _create_build_drum_entries(staging, drum_tab, drum_parts)) + # Vocals seed: an empty (or authored) lyrics track. feedpak §7.1 # lyrics.json is a flat array of syllables — an empty array is a # valid, empty track the author fills in later. diff --git a/src/arrangement.js b/src/arrangement.js index 22417eee..2d69211c 100644 --- a/src/arrangement.js +++ b/src/arrangement.js @@ -316,13 +316,14 @@ export function editorShowAddDrumsModal() { } // Can a SECOND (third, …) drum part be added — i.e. does a drum import ADD -// another part (vs replace the existing tab)? True only when the primary part -// is materialized as a type:"drums" arrangement in a non-create sloppak -// session — the create flow's build (create_sloppak) persists a single -// drum_tab, so create mode keeps the legacy one-part replace semantics. -// (Exported for new-track.js's plan gate + the tests.) +// another part (vs replace the existing tab)? True once the primary part is +// materialized as a type:"drums" arrangement in a sloppak session — both the +// /save_song and create-mode /build paths persist the extras now. A part +// only materializes beside a pitched track (drums are never index 0), so a +// drums-only compose session — its lone off-array tab — can't add a second +// until a melodic track exists. (Exported for new-track.js's plan gate + tests.) export function _canAddAnotherDrums() { - return !!(S.drumTab && !S.createMode && S.format === 'sloppak' + return !!(S.drumTab && S.format === 'sloppak' && findDrumArrangement(S.arrangements)); } @@ -334,13 +335,14 @@ export function editorHideAddDrumsModal() { // tab is pure client state until save (S.drumTab + S.drumTabDirty — the // same stash the GP/MIDI import path uses), and the create flow's // init_drums seeds the identical empty shape, so no backend call is -// needed. When drums already exist, ADDS another drum part (a song can -// hold several) — except in create mode, whose build persists one part. +// needed. When drums already exist, ADDS another drum part (a song can hold +// several — create and edit sessions alike, once a melodic track exists). export function editorAddEmptyDrums() { if (!S.sessionId || S.format !== 'sloppak') return false; if (S.drumTab && !_canAddAnotherDrums()) { - // Create-mode (or an unmaterialized legacy tab): still one part max. - setStatus('This song already has a Drums track — open it with 🥁 Edit Drums. Save the song to add more drum parts.'); + // The only remaining one-part case: a drums-only session whose tab + // isn't materialized as an arrangement (no melodic track to sit beside). + setStatus('This song already has a Drums track — open it with 🥁 Edit Drums. Add a melodic track first to hold more than one drum part.'); return false; } const tab = { version: 1, name: 'Drums', kit: [], hits: [] }; diff --git a/src/create.js b/src/create.js index bf4f9ef3..f26192f3 100644 --- a/src/create.js +++ b/src/create.js @@ -801,8 +801,27 @@ function _createAudioPayloadPure(audioTracks, guideId) { })), }; } +// The drum payload the create-mode /build ships (feedpak 1.17.0 N drums). +// The PRIMARY drum_tab is the FIRST drums arrangement's payload — NOT the +// active `drumTab`, which tracks whichever part is open in the grid (the user +// may be editing a secondary at build time). `drum_parts` (the extras) is only +// present when drums are materialized as arrangements, so a legacy single-tab +// compose build (no pitched part → no drums arrangement) stays byte-identical. +function _drumBuildPayloadPure(arrangements, drumTab) { + const drumArrs = (Array.isArray(arrangements) ? arrangements : []).filter(isDrumArrangement); + const primaryTab = drumArrs.length ? drumArrs[0].drumTab : drumTab; + const out = { drum_tab: (primaryTab && Array.isArray(primaryTab.hits)) ? primaryTab : null }; + if (drumArrs.length) { + out.drum_parts = drumArrs.slice(1).map(a => ({ + id: String(a.id || ''), + name: String(a.name || 'Drums').slice(0, 120), + drum_tab: a.drumTab, + })); + } + return out; +} /* @pure:create-track-table:end */ -export { _createAudioPayloadPure, _createGuideIdPure, _createTrackRowsPure }; +export { _createAudioPayloadPure, _createGuideIdPure, _createTrackRowsPure, _drumBuildPayloadPure }; // Community arrangement XML and MusicXML share the .xml extension. Sniff the // document root before choosing an importer; declarations, DOCTYPEs, comments, @@ -2674,9 +2693,12 @@ export async function editorBuild() { // sloppak. editorDoCreate sets S.format='sloppak' when the GP // import brought either; forward that as the build target so // the server writes a sloppak (not a archive that silently drops - // them), and ship the imported drum_tab so it's persisted. + // them), and ship the drum parts so they're persisted. target_format: S.format === 'sloppak' ? 'sloppak' : '', - drum_tab: (S.drumTab && Array.isArray(S.drumTab.hits)) ? S.drumTab : null, + // The drum payload (primary tab + extras) — see + // _drumBuildPayloadPure: the primary is the FIRST drums + // arrangement's tab, NOT the active S.drumTab. + ..._drumBuildPayloadPure(S.arrangements, S.drumTab), // Audio placement shift ("Shift Audio…") — persisted into the // built pack's manifest (read back on load via data.audio_shift) // so the alignment survives the first build, not just re-saves. diff --git a/src/new-track.js b/src/new-track.js index 967c43b3..550f8093 100644 --- a/src/new-track.js +++ b/src/new-track.js @@ -78,16 +78,17 @@ function _renderNewTrackModal() { for (const r of modal.querySelectorAll('input[name="new-track-source"]')) { r.checked = r.value === _sel.source; } - // Drums with an existing drum tab: in a saved sloppak session the empty - // start ADDS another drum part — say so; in create mode (one part max) - // it stays blocked, phrased inline instead of failing at Create. + // Drums with an existing drum tab: once a drum part is materialized the + // empty start ADDS another (create + edit sessions alike) — say so. The + // only blocked case left is a drums-only session (no melodic track to sit + // beside), phrased inline instead of failing at Create. const note = _byId('editor-new-track-note'); if (note) { const drumsEmpty = isTrans && _sel.instrument === 'Drums' && _sel.source === 'empty' && !!S.drumTab; note.textContent = !drumsEmpty ? '' : _canAddAnotherDrums() ? 'This song already has drums — this will add another Drums track.' - : 'This song already has a Drums track — save the song first to add more drum parts, or choose "Import from a file" to replace it.'; + : 'This song already has a Drums track — add a melodic track first to hold more than one drum part, or choose "Import from a file" to replace it.'; } const create = _byId('editor-new-track-create'); if (create) { diff --git a/tests/create_save_routing.test.mjs b/tests/create_save_routing.test.mjs index 058cd439..bcc8f347 100644 --- a/tests/create_save_routing.test.mjs +++ b/tests/create_save_routing.test.mjs @@ -26,6 +26,7 @@ globalThis.document = globalThis.document || { getElementById: () => null }; const { saveCDLC, _removeEmptyPickedFile, _suggestedSaveNamePure } = await import('../src/file-ops.js'); +const { _drumBuildPayloadPure } = await import('../src/create.js'); const { S } = await import('../src/state.js'); let pass = 0, fail = 0; @@ -158,5 +159,67 @@ await t('_suggestedSaveNamePure: create mode derives the name the build will wri assert.strictEqual(_suggestedSaveNamePure(null, null, null), 'song.feedpak'); }); +// ── create-mode N drums: the /build drum payload (feedpak 1.17.0) ───────── +const dtab = (name, hits = [{ t: 1, p: 'kick' }]) => ({ version: 1, name, kit: [], hits }); + +await t('_drumBuildPayloadPure: primary = first drums arr (NOT the active tab); extras follow', () => { + const kit = dtab('Drums'); + const live = dtab('Drums (Live)', [{ t: 2, p: 'snare' }]); + const arrs = [ + { id: 'lead', name: 'Lead', notes: [] }, + { id: 'drums', name: 'Drums', type: 'drums', drumTab: kit }, + { id: 'drums-2', name: 'Drums (Live)', type: 'drums', drumTab: live }, + ]; + // The user is editing the SECOND part (active tab = live) — the primary + // shipped must still be the FIRST part's tab, not the active one. + const payload = _drumBuildPayloadPure(arrs, live); + assert.strictEqual(payload.drum_tab, kit, 'primary is the first drums arrangement, not the active tab'); + assert.deepStrictEqual(payload.drum_parts, [ + { id: 'drums-2', name: 'Drums (Live)', drum_tab: live }, + ]); +}); + +await t('_drumBuildPayloadPure: one drum part ships an EMPTY extras list (writes the alias entry)', () => { + const kit = dtab('Drums'); + const payload = _drumBuildPayloadPure( + [{ id: 'lead', name: 'Lead' }, { id: 'drums', name: 'Drums', type: 'drums', drumTab: kit }], kit); + assert.strictEqual(payload.drum_tab, kit); + assert.deepStrictEqual(payload.drum_parts, [], 'materialized single drum → empty extras (not absent)'); +}); + +await t('_drumBuildPayloadPure: a legacy unmaterialized tab stays byte-identical (no drum_parts key)', () => { + const kit = dtab('Drums'); + // Drums-only compose (no pitched part → no drums arrangement): the tab + // rides S.drumTab and there is NO drum_parts key at all. + const payload = _drumBuildPayloadPure([], kit); + assert.strictEqual(payload.drum_tab, kit); + assert.strictEqual('drum_parts' in payload, false); + // No drums at all → drum_tab null, still no drum_parts. + assert.deepStrictEqual(_drumBuildPayloadPure([{ id: 'lead' }], null), { drum_tab: null }); +}); + +await t('create-mode build ships the drum parts on the /build wire (real editorBuild through saveCDLC)', async () => { + seedCreateSession(); + const kit = dtab('Drums'); + const live = dtab('Drums (Live)', [{ t: 2, p: 'snare' }]); + // Two materialized drum parts; the user is on the secondary in the grid. + S.arrangements.push({ id: 'drums', name: 'Drums', type: 'drums', drumTab: kit }); + S.arrangements.push({ id: 'drums-2', name: 'Drums (Live)', type: 'drums', drumTab: live }); + S.drumTab = live; + let body = null; + globalThis.fetch = async (url, opts) => { + body = JSON.parse(opts.body); + return { json: async () => ({ success: true, filename: 'x.feedpak' }) }; + }; + const ok = await saveCDLC(); + assert.strictEqual(ok, true); + assert.deepStrictEqual(body.drum_tab, kit, 'the PRIMARY tab ships as drum_tab, not the active secondary'); + assert.strictEqual(body.drum_parts.length, 1); + assert.strictEqual(body.drum_parts[0].id, 'drums-2'); + assert.strictEqual(body.drum_parts[0].name, 'Drums (Live)'); + assert.deepStrictEqual(body.drum_parts[0].drum_tab, live); + S.arrangements = []; S.drumTab = null; +}); + console.log(`\n${pass} passed, ${fail} failed`); process.exit(fail ? 1 : 0); diff --git a/tests/new_track.test.mjs b/tests/new_track.test.mjs index 3cbe4736..c8715125 100644 --- a/tests/new_track.test.mjs +++ b/tests/new_track.test.mjs @@ -171,23 +171,31 @@ t('editorAddEmptyDrums: blank shape once; a SECOND part in a saved sloppak sessi assert.strictEqual(S.drumTab.name, 'Drums 2', 'de-duplicated display name'); }); -t('editorAddEmptyDrums: create mode keeps the one-part rule (refuses twice)', () => { +t('editorAddEmptyDrums: create mode ADDS a second part too (its build persists N now)', () => { + // Parity with edit-mode: the create-mode /build path writes the extra + // drum parts, so create sessions add a 2nd part beside a pitched track. Object.assign(S, { format: 'sloppak', sessionId: 'test-session', createMode: true, drumTab: null, drumTabDirty: false, arrangements: [{ name: 'Lead', notes: [] }], drumSel: new Set(), }); assert.strictEqual(editorAddEmptyDrums(), true); - S.drumTab.hits.push({ t: 1, piece: 'kick' }); - assert.strictEqual(editorAddEmptyDrums(), false, 'create mode: one part max (its build persists one)'); - assert.strictEqual(S.drumTab.hits.length, 1, 'existing tab untouched'); + assert.strictEqual(S.arrangements.filter(a => a.type === 'drums').length, 1, + 'primary materialized beside the pitched part'); + const firstTab = S.drumTab; + firstTab.hits.push({ t: 1, piece: 'kick' }); + assert.strictEqual(editorAddEmptyDrums(), true, 'a second part is allowed in create mode'); + assert.strictEqual(S.arrangements.filter(a => a.type === 'drums').length, 2, 'two drum parts'); + assert.notStrictEqual(S.drumTab, firstTab, 'the new part is the active grid target'); + assert.strictEqual(firstTab.hits.length, 1, 'the first tab is untouched'); Object.assign(S, { createMode: false }); }); -t('editorAddEmptyDrums: a drums-only session keeps the tab OFF the arrangement list', () => { +t('editorAddEmptyDrums: a drums-only session keeps the tab OFF the arrangement list, and can’t add a 2nd', () => { // No pitched part: materializing would put drums at index 0, where the // default currentArr lands — the tab must stay a legacy off-array - // singleton instead (the drum grid still edits it through the mode). + // singleton instead (the drum grid still edits it through the mode). And + // with no arrangement to sit beside, a second part can't be added. Object.assign(S, { format: 'sloppak', sessionId: 'test-session', createMode: false, drumTab: null, drumTabDirty: false, arrangements: [], drumSel: new Set(), @@ -195,6 +203,9 @@ t('editorAddEmptyDrums: a drums-only session keeps the tab OFF the arrangement l assert.strictEqual(editorAddEmptyDrums(), true); assert.ok(S.drumTab, 'tab created'); assert.strictEqual(S.arrangements.length, 0, 'no drums arrangement at index 0'); + S.drumTab.hits.push({ t: 1, piece: 'kick' }); + assert.strictEqual(editorAddEmptyDrums(), false, 'drums-only: no melodic track to sit beside, so one part max'); + assert.strictEqual(S.drumTab.hits.length, 1, 'existing tab untouched'); }); t('editorAddEmptyDrums: refuses outside a sloppak session', () => { diff --git a/tests/test_drum_parts.py b/tests/test_drum_parts.py index 4e5e2e06..96634528 100644 --- a/tests/test_drum_parts.py +++ b/tests/test_drum_parts.py @@ -15,9 +15,15 @@ same invariants as the primary (no malformed/duplicate hits, millisecond-rounded, time-ordered) and the request body is never mutated. + - `_create_build_drum_entries` — the CREATE-MODE /build write: the + `type: drums` manifest entries + the extra side files, mirroring the + /save_song drum-parts wire. """ +import json + from routes import ( + _create_build_drum_entries, _is_drum_pointer_entry, _primary_drum_alias_id, _sanitize_extra_drum_tab, @@ -98,3 +104,73 @@ def test_sanitize_drops_malformed_and_duplicate_hits_and_sorts(): def test_sanitize_tolerates_a_missing_or_bogus_hits_field(): assert _sanitize_extra_drum_tab({"version": 1})["hits"] == [] assert _sanitize_extra_drum_tab({"version": 1, "hits": "junk"})["hits"] == [] + + +# ── create-mode /build write (_create_build_drum_entries) ──────────────────── + +def _dtab(name, hits=None): + return {"version": 1, "name": name, "kit": [], + "hits": hits if hits is not None else [{"t": 1.0, "p": "kick"}]} + + +def test_create_build_writes_side_files_and_primary_alias(tmp_path): + primary = _dtab("Drums") + entries = _create_build_drum_entries(tmp_path, primary, [ + {"id": "drums-2", "name": "Drums (Live)", "drum_tab": _dtab("Drums (Live)", [{"t": 2.0, "p": "snare"}])}, + ]) + # Primary alias FIRST (points at the song-level file, no side file written), + # then the extra with its own drum_tab_.json. + assert entries == [ + {"id": "drums", "name": "Drums", "type": "drums", "drum_tab": "drum_tab.json"}, + {"id": "drums-2", "name": "Drums (Live)", "type": "drums", "drum_tab": "drum_tab_drums-2.json"}, + ] + # The primary's file is the caller's job — only the extra is written here. + assert not (tmp_path / "drum_tab.json").exists() + written = json.loads((tmp_path / "drum_tab_drums-2.json").read_text()) + assert written["name"] == "Drums (Live)" + assert [h["p"] for h in written["hits"]] == ["snare"] + + +def test_create_build_empty_extras_writes_only_the_primary_alias(tmp_path): + entries = _create_build_drum_entries(tmp_path, _dtab("Drums"), []) + assert entries == [ + {"id": "drums", "name": "Drums", "type": "drums", "drum_tab": "drum_tab.json"}] + assert list(tmp_path.glob("drum_tab_*.json")) == [] + + +def test_create_build_sanitizes_and_names_the_extra_tab(tmp_path): + # Malformed/duplicate hits are dropped and the entry name propagates into + # the written tab's name field (kept in lockstep for reload). + dirty = _dtab("x", [{"t": 2.0, "p": "kick"}, {"t": 2.0, "p": "kick"}, {"t": -1, "p": "kick"}, {"t": 1.0, "p": "snare"}]) + _create_build_drum_entries(tmp_path, _dtab("Drums"), [ + {"id": "drums-2", "name": "Live Kit", "drum_tab": dirty}]) + tab = json.loads((tmp_path / "drum_tab_drums-2.json").read_text()) + assert tab["name"] == "Live Kit" + assert [(h["t"], h["p"]) for h in tab["hits"]] == [(1.0, "snare"), (2.0, "kick")] + # The request body is never mutated (sanitize returns a copy). + assert len(dirty["hits"]) == 4 + + +def test_create_build_de_collides_ids_and_sanitizes_filenames(tmp_path): + entries = _create_build_drum_entries(tmp_path, _dtab("Drums"), [ + {"id": "drums", "drum_tab": _dtab("A")}, # collides with the primary + {"id": "Weird Name!", "drum_tab": _dtab("B")}, # unsafe filename chars + {"id": "", "drum_tab": _dtab("C")}, # empty → default + ]) + ids = [e["id"] for e in entries] + assert ids[0] == "drums" # the primary alias + assert len(set(ids)) == len(ids), "no duplicate ids" + assert "drums" not in ids[1:] # the colliding extra was renamed + for e in entries[1:]: + assert (tmp_path / f"drum_tab_{e['id']}.json").exists() + assert e["id"] == e["id"].lower() and " " not in e["id"] and "!" not in e["id"] + + +def test_create_build_sanitizes_a_non_list_hits_tab_to_empty(tmp_path): + # Schema validation is the endpoint's job (build_song_endpoint rejects a + # non-list `hits` with 400); if junk still reaches the pure writer, it + # sanitizes to empty rather than crashing the build. + entries = _create_build_drum_entries(tmp_path, _dtab("Drums"), [ + {"id": "bad", "drum_tab": {"version": 1, "hits": "not-a-list"}}]) + assert [e["id"] for e in entries] == ["drums", "bad"] + assert json.loads((tmp_path / "drum_tab_bad.json").read_text())["hits"] == [] From 4e8fbfe51e52aee224fa9630c35003ab75ef6c2e Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Tue, 21 Jul 2026 15:40:53 +0200 Subject: [PATCH 2/2] feat(editor): create-mode songs can hold several drum parts too (re-land of #340) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Includes review fix: round-trip the primary drum part's id through the create/ build path (drum_tab_id → _primary_drum_alias_id), mirroring the #339 save-path fix, so a promoted primary keeps its stem links + tree placement on reload. Co-Authored-By: Claude Opus 4.8 (1M context) --- routes.py | 15 +++++++++++---- src/create.js | 7 +++++++ tests/create_save_routing.test.mjs | 18 ++++++++++++++++-- tests/test_drum_parts.py | 19 +++++++++++++++++++ 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/routes.py b/routes.py index 81c92289..68a89709 100644 --- a/routes.py +++ b/routes.py @@ -339,7 +339,7 @@ def _sanitize_extra_drum_tab(tab): return out -def _create_build_drum_entries(staging, drum_tab, drum_parts): +def _create_build_drum_entries(staging, drum_tab, drum_parts, drum_tab_id=None): """The `type: drums` manifest arrangement entries for a CREATE-MODE build with multiple drum parts (feedpak 1.17.0 "drums as arrangements"), and the side effect of writing each EXTRA part's `drum_tab_.json` into @@ -358,13 +358,18 @@ def _create_build_drum_entries(staging, drum_tab, drum_parts): from pathlib import Path as _Path staging = _Path(staging) + # The primary's alias entry id FOLLOWS the incoming primary's id + # (drum_tab_id = parts[0].id) so a promoted primary round-trips its + # identity; legacy / old-client builds fall back to "drums". Twin of + # /save_song ~5404. + primary_id = _primary_drum_alias_id(drum_tab_id) entries = [{ - "id": "drums", + "id": primary_id, "name": str(drum_tab.get("name") or "Drums")[:120], "type": "drums", "drum_tab": "drum_tab.json", }] - used_ids = {"drums"} + used_ids = {primary_id} for part in (drum_parts or []): if not isinstance(part, dict) or not isinstance(part.get("drum_tab"), dict): continue @@ -8746,6 +8751,7 @@ def _build_sloppak(): output_path=output, drum_tab=drum_tab if isinstance(drum_tab, dict) else None, drum_parts=drum_parts, + drum_tab_id=data.get("drum_tab_id"), audio_tracks=extra_audio_tracks, audio_guide_name=(build_audio_tracks[0]["name"] if build_audio_tracks else ""), @@ -8857,6 +8863,7 @@ def _write_sloppak_pak(*, audio_file: str, art_path: str, meta: dict, output_path: Path, drum_tab: dict | None = None, drum_parts: list | None = None, + drum_tab_id: str | None = None, lyrics: list | None = None, preview_path: str = "", fail_if_exists: bool = False, @@ -9159,7 +9166,7 @@ def _write_sloppak_pak(*, audio_file: str, art_path: str, # index 0 — that slot is the played chart). manifest["arrangements"] = ( list(manifest.get("arrangements") or []) - + _create_build_drum_entries(staging, drum_tab, drum_parts)) + + _create_build_drum_entries(staging, drum_tab, drum_parts, drum_tab_id)) # Vocals seed: an empty (or authored) lyrics track. feedpak §7.1 # lyrics.json is a flat array of syllables — an empty array is a diff --git a/src/create.js b/src/create.js index f26192f3..7cb32659 100644 --- a/src/create.js +++ b/src/create.js @@ -811,6 +811,13 @@ function _drumBuildPayloadPure(arrangements, drumTab) { const drumArrs = (Array.isArray(arrangements) ? arrangements : []).filter(isDrumArrangement); const primaryTab = drumArrs.length ? drumArrs[0].drumTab : drumTab; const out = { drum_tab: (primaryTab && Array.isArray(primaryTab.hits)) ? primaryTab : null }; + // The promoted primary's durable id (drumArrs[0].id) — the backend + // persists the alias entry under it so a survivor promoted to primary + // (e.g. "drums-2" after the original was deleted) round-trips its identity + // and its stem links / tree rows survive reload. Blank when there is no + // materialized drums arrangement → the backend defaults to "drums". Twin + // of file-ops.js `_buildSaveBody`'s `body.drum_tab_id`. + out.drum_tab_id = drumArrs.length ? String(drumArrs[0].id || '') : ''; if (drumArrs.length) { out.drum_parts = drumArrs.slice(1).map(a => ({ id: String(a.id || ''), diff --git a/tests/create_save_routing.test.mjs b/tests/create_save_routing.test.mjs index bcc8f347..8533bd4f 100644 --- a/tests/create_save_routing.test.mjs +++ b/tests/create_save_routing.test.mjs @@ -174,16 +174,28 @@ await t('_drumBuildPayloadPure: primary = first drums arr (NOT the active tab); // shipped must still be the FIRST part's tab, not the active one. const payload = _drumBuildPayloadPure(arrs, live); assert.strictEqual(payload.drum_tab, kit, 'primary is the first drums arrangement, not the active tab'); + assert.strictEqual(payload.drum_tab_id, 'drums', 'ships the primary drums arr id, not the active one'); assert.deepStrictEqual(payload.drum_parts, [ { id: 'drums-2', name: 'Drums (Live)', drum_tab: live }, ]); }); +await t('_drumBuildPayloadPure: a PROMOTED primary ships drum_tab_id = drumArrs[0].id (orphan-on-reload fix)', () => { + // Original "drums" deleted → "drums-2" promoted to parts[0]. drum_tab_id + // must follow it so the backend aliases the entry under "drums-2". + const live = dtab('Drums (Live)', [{ t: 2, p: 'snare' }]); + const payload = _drumBuildPayloadPure( + [{ id: 'lead', name: 'Lead' }, { id: 'drums-2', name: 'Drums (Live)', type: 'drums', drumTab: live }], live); + assert.strictEqual(payload.drum_tab, live); + assert.strictEqual(payload.drum_tab_id, 'drums-2'); +}); + await t('_drumBuildPayloadPure: one drum part ships an EMPTY extras list (writes the alias entry)', () => { const kit = dtab('Drums'); const payload = _drumBuildPayloadPure( [{ id: 'lead', name: 'Lead' }, { id: 'drums', name: 'Drums', type: 'drums', drumTab: kit }], kit); assert.strictEqual(payload.drum_tab, kit); + assert.strictEqual(payload.drum_tab_id, 'drums'); assert.deepStrictEqual(payload.drum_parts, [], 'materialized single drum → empty extras (not absent)'); }); @@ -193,9 +205,10 @@ await t('_drumBuildPayloadPure: a legacy unmaterialized tab stays byte-identical // rides S.drumTab and there is NO drum_parts key at all. const payload = _drumBuildPayloadPure([], kit); assert.strictEqual(payload.drum_tab, kit); + assert.strictEqual(payload.drum_tab_id, '', 'no materialized drums arr → blank id (backend defaults to "drums")'); assert.strictEqual('drum_parts' in payload, false); - // No drums at all → drum_tab null, still no drum_parts. - assert.deepStrictEqual(_drumBuildPayloadPure([{ id: 'lead' }], null), { drum_tab: null }); + // No drums at all → drum_tab null, blank id, still no drum_parts. + assert.deepStrictEqual(_drumBuildPayloadPure([{ id: 'lead' }], null), { drum_tab: null, drum_tab_id: '' }); }); await t('create-mode build ships the drum parts on the /build wire (real editorBuild through saveCDLC)', async () => { @@ -214,6 +227,7 @@ await t('create-mode build ships the drum parts on the /build wire (real editorB const ok = await saveCDLC(); assert.strictEqual(ok, true); assert.deepStrictEqual(body.drum_tab, kit, 'the PRIMARY tab ships as drum_tab, not the active secondary'); + assert.strictEqual(body.drum_tab_id, 'drums', 'the /build wire carries the primary id for round-trip'); assert.strictEqual(body.drum_parts.length, 1); assert.strictEqual(body.drum_parts[0].id, 'drums-2'); assert.strictEqual(body.drum_parts[0].name, 'Drums (Live)'); diff --git a/tests/test_drum_parts.py b/tests/test_drum_parts.py index 96634528..fc3de2c9 100644 --- a/tests/test_drum_parts.py +++ b/tests/test_drum_parts.py @@ -174,3 +174,22 @@ def test_create_build_sanitizes_a_non_list_hits_tab_to_empty(tmp_path): {"id": "bad", "drum_tab": {"version": 1, "hits": "not-a-list"}}]) assert [e["id"] for e in entries] == ["drums", "bad"] assert json.loads((tmp_path / "drum_tab_bad.json").read_text())["hits"] == [] + + +def test_create_build_primary_alias_id_round_trips_a_promoted_primary(tmp_path): + # Regression: promote a survivor to primary in create mode (delete the + # original "drums" → DeleteDrumTabCmd promotes "drums-2" to parts[0]) then + # Build. The primary alias entry must keep id "drums-2" (not the literal + # "drums") so its editor_stem_links / track_session tree rows — keyed by + # "drums-2" — survive reopen instead of being dropped as orphans. Mirrors + # the SAVE-path fix (routes.py ~5404). Extras de-collide against it. + entries = _create_build_drum_entries( + tmp_path, _dtab("Drums (Live)"), + [{"id": "drums", "name": "Drums", "drum_tab": _dtab("Drums")}], + drum_tab_id="drums-2") + assert entries[0]["id"] == "drums-2", "promoted primary keeps its id, not 'drums'" + assert entries[0]["drum_tab"] == "drum_tab.json" + assert "drums-2" not in [e["id"] for e in entries[1:]], "extra de-collides off the primary id" + # Legacy default: absent/blank drum_tab_id → alias id "drums" (byte-identical). + legacy = _create_build_drum_entries(tmp_path, _dtab("Drums"), []) + assert legacy[0]["id"] == "drums"