From 35061e8a2397165bc1dbe939079e0546119a6e81 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sun, 19 Jul 2026 22:17:16 -0500 Subject: [PATCH 01/18] feat(editor): honor an authored instrument type over the name (keys/bass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First brick of the multitrack program (MULTITRACK-FEEDPAK-DESIGN.md Phase 1 / Milestone A): make a track's instrument identity first-class DATA instead of name-inferred. The backend already persists a manifest `type` facet (feedpak-spec §5.2) and never clobbers an authored value; the frontend has been ignoring it, inferring the instrument from the NAME in a dozen places with two subtly disagreeing rules (the runtime prefix test vs the save-side word-boundary test — the disagreement the rename guard band-aids). - New leaf `src/instrument.js`: `_typeKind` canonicalizes the manifest `type` vocabulary ("piano"→keys, plus the plural set drums/vocals grow into) to the editor's runtime kind, or null when absent/blank/unrecognized; `_arrTypeKind` reads it off an arrangement. Imports nothing from src/ so keys.js / lanes.js consult it without closing a cycle. - The load-bearing keys/bass DATA + view predicates now HONOR an authored `type`, falling back to their EXACT prior name test when untyped: `isKeysArr` + `viewFor` (keys.js) and the 4-vs-6 bass baseline in `_seedExtendedStringsFromTuning` (lanes.js). Untyped/legacy packs are byte-identical (a typed part named against its instrument — "Grand Piano" that is really a guitar — now resolves to its authored identity, which name inference could never do). Deliberately scoped: the ~30 other name-inference sites and the rename guard still key off the name — relaxing them is only safe once every identity reader consults `type`, so they follow behind this same seam. The load path that populates `arr.type` from the manifest (making existing packs' inferred type flow through) is the immediate next step. Tests: tests/instrument_type.test.mjs (7) — the vocabulary map, type-over-name in both directions, and the byte-identical untyped fallback for keys + the bass baseline. JS 296/0 + 7 new, lint 0 errors / 3 baseline warnings, routes.py untouched (no pytest). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- src/instrument.js | 48 +++++++++++++++ src/keys.js | 11 +++- src/lanes.js | 12 +++- tests/instrument_type.test.mjs | 106 +++++++++++++++++++++++++++++++++ 4 files changed, 174 insertions(+), 3 deletions(-) create mode 100644 src/instrument.js create mode 100644 tests/instrument_type.test.mjs diff --git a/src/instrument.js b/src/instrument.js new file mode 100644 index 00000000..aa089e02 --- /dev/null +++ b/src/instrument.js @@ -0,0 +1,48 @@ +// ════════════════════════════════════════════════════════════════════ +// Instrument identity — a track's INSTRUMENT TYPE as first-class DATA. +// +// Historically the editor inferred a part's instrument from its NAME, in a +// dozen places with subtly different rules — the runtime prefix test +// (`/^(keys|piano|…)/`) vs the save-side word-boundary test — the very +// disagreement the rename guard exists to band-aid ("Electric Piano" reads +// runtime-guitar but save-keys). This module is the seam that makes identity +// first-class: when an arrangement carries an authored `type` (the feedpak-spec +// §5.2 manifest facet, which the backend already persists and never clobbers), +// it WINS; name inference stays only as the fallback for untyped/legacy packs. +// +// The format writes "piano" for the keys family and (today) leaves drums/vocals +// as side-files (type ""); this canonicalizes the manifest vocabulary to the +// editor's runtime kinds and accepts the plural set the multitrack work +// (drums-as-arrangements, vocals) grows into. Leaf module (imports nothing from +// src/) so keys.js / lanes.js / arrangement.js consult it without closing a cycle. +// +// Deliberately NOT touched yet: the ~30 other name-inference sites and the +// rename guard still key off the name. Relaxing them is only safe once every +// identity reader consults `type`; this PR converts the load-bearing keys/bass +// DATA + view predicates and leaves the rest to follow behind the same seam. +// ════════════════════════════════════════════════════════════════════ + +// Manifest `type` value (any case/whitespace) → the editor's runtime instrument +// kind, or null when the type is absent / blank / unrecognized (the caller then +// falls back to its own name inference, so an untyped pack is byte-identical). +// piano/keyboard/synth fold into keys; lead/rhythm are guitar; drum & vocal +// synonyms are accepted ahead of their arrangement-native support landing. +const _TYPE_KIND = { + keys: 'keys', piano: 'keys', keyboard: 'keys', synth: 'keys', + bass: 'bass', + guitar: 'guitar', lead: 'guitar', rhythm: 'guitar', + drums: 'drums', drum: 'drums', + vocals: 'vocals', vocal: 'vocals', voice: 'vocals', +}; + +export function _typeKind(rawType) { + if (typeof rawType !== 'string') return null; + const t = rawType.trim().toLowerCase(); + return (t && Object.prototype.hasOwnProperty.call(_TYPE_KIND, t)) ? _TYPE_KIND[t] : null; +} + +// The authored instrument kind of an arrangement, or null when it is untyped +// (caller falls back to name inference). The one place a `type` facet is read. +export function _arrTypeKind(arr) { + return _typeKind(arr && arr.type); +} diff --git a/src/keys.js b/src/keys.js index 48a45801..7a0afa8b 100644 --- a/src/keys.js +++ b/src/keys.js @@ -19,6 +19,7 @@ import { _openMidiForArr, _soundingPitchPure, _stringCountFor } from './lanes.js import { notes } from './notes.js'; import { S } from './state.js'; import { PIANO_NOTE_NAMES, _noteNamesForKeyPure } from './theory.js'; +import { _arrTypeKind } from './instrument.js'; // ── Piano roll constants ──────────────────────────────────────────── export const PIANO_OCTAVE_COLORS = [ @@ -82,6 +83,10 @@ export function _viewPrefsSave() { } catch (_) { /* ignore */ } } export function viewFor(arr) { + // An authored keys `type` piano-locks the part regardless of its name; + // otherwise the legacy name test drives the piano-lock + stored preference. + const k = _arrTypeKind(arr); + if (k) return k === 'keys' ? 'piano' : (_viewPrefs()[_partViewKeyPure(arr)] === 'piano' ? 'piano' : 'string'); return _viewForPure(arr && arr.name, _viewPrefs()[_partViewKeyPure(arr)]); } @@ -92,7 +97,11 @@ export function viewFor(arr) { export function isKeysArr() { if (!S.arrangements.length) return false; const arr = S.arrangements[S.currentArr]; - return !!(arr && KEYS_PATTERN.test(arr.name || '')); + if (!arr) return false; + // An authored `type` is authoritative (instrument identity is DATA); the + // legacy prefix name test is the fallback for untyped/legacy packs. + const k = _arrTypeKind(arr); + return k ? k === 'keys' : KEYS_PATTERN.test(arr.name || ''); } // Piano SURFACE predicate: the piano-roll view is active for the current diff --git a/src/lanes.js b/src/lanes.js index 27c39c6d..7f023336 100644 --- a/src/lanes.js +++ b/src/lanes.js @@ -7,6 +7,7 @@ */ import { S } from './state.js'; +import { _arrTypeKind } from './instrument.js'; export const MAX_LANES = 8; @@ -65,7 +66,12 @@ export function colorForLane(l) { function isBassArr() { if (!S.arrangements.length) return false; const arr = S.arrangements[S.currentArr]; - return !!arr && /bass/i.test(arr.name || ''); + if (!arr) return false; + // Authored `type` wins; else the legacy /bass/ name test (an untyped + // "Synth Bass" stays bass here, exactly as before — a typed keys part + // named that resolves to keys, its authored identity). + const k = _arrTypeKind(arr); + return k ? k === 'bass' : /bass/i.test(arr.name || ''); } // Active arrangement string count. Mirrors lib/song.py:arrangement_string_count @@ -95,7 +101,9 @@ function isBassArr() { export function _seedExtendedStringsFromTuning(arrangements, authoritativeLength) { for (const arr of arrangements || []) { if (typeof arr._extendedStrings === 'number') continue; // already set - const isBass = /bass/i.test(arr.name || ''); + // Authored `type` wins for the 4-vs-6 baseline; else the legacy name test. + const k = _arrTypeKind(arr); + const isBass = k ? k === 'bass' : /bass/i.test(arr.name || ''); const baseline = isBass ? 4 : 6; const tuningLen = Array.isArray(arr.tuning) ? arr.tuning.length : baseline; if (tuningLen > 6) { diff --git a/tests/instrument_type.test.mjs b/tests/instrument_type.test.mjs new file mode 100644 index 00000000..37e02e22 --- /dev/null +++ b/tests/instrument_type.test.mjs @@ -0,0 +1,106 @@ +/* + * Instrument type as first-class DATA (src/instrument.js). + * + * Pinned here: + * - _typeKind maps the manifest `type` facet (feedpak-spec §5.2 — "piano" for + * the keys family, plus the plural set the multitrack work grows into) to the + * editor's runtime kind, or null when absent/blank/unrecognized; + * - the load-bearing DATA/view predicates now HONOR an authored `type` over the + * name — isKeysArr (keys.js) and the 4-vs-6 bass baseline + * (_seedExtendedStringsFromTuning, lanes.js); + * - and they stay BYTE-IDENTICAL for untyped/legacy arrangements: with no + * `type`, every predicate falls back to its exact previous name test. + * + * Fails on main: src/instrument.js does not exist and the predicates read only + * the name (a typed part named against its instrument can't override). + * + * Run: node tests/instrument_type.test.mjs + */ +import assert from 'node:assert'; + +import { _typeKind, _arrTypeKind } from '../src/instrument.js'; +import { isKeysArr } from '../src/keys.js'; +import { _seedExtendedStringsFromTuning } from '../src/lanes.js'; +import { seedState } from './_history_env.mjs'; + +let pass = 0; let fail = 0; +const tests = []; +const t = (name, fn) => tests.push([name, fn]); + +// ── _typeKind ───────────────────────────────────────────────────────── +t('_typeKind maps the manifest vocabulary to runtime kinds', () => { + for (const v of ['piano', 'keyboard', 'synth', 'keys']) assert.strictEqual(_typeKind(v), 'keys', v); + assert.strictEqual(_typeKind('bass'), 'bass'); + for (const v of ['guitar', 'lead', 'rhythm']) assert.strictEqual(_typeKind(v), 'guitar', v); + for (const v of ['drum', 'drums']) assert.strictEqual(_typeKind(v), 'drums', v); + for (const v of ['vocal', 'vocals', 'voice']) assert.strictEqual(_typeKind(v), 'vocals', v); + // case + whitespace insensitive + assert.strictEqual(_typeKind(' Piano '), 'keys'); + assert.strictEqual(_typeKind('BASS'), 'bass'); +}); + +t('_typeKind is null for absent / blank / unrecognized / non-string', () => { + assert.strictEqual(_typeKind(''), null); + assert.strictEqual(_typeKind(' '), null); + assert.strictEqual(_typeKind('tuba'), null, 'unrecognized → null (falls back to name)'); + assert.strictEqual(_typeKind(undefined), null); + assert.strictEqual(_typeKind(null), null); + assert.strictEqual(_typeKind(42), null); +}); + +t('_arrTypeKind reads arr.type (and is null when untyped)', () => { + assert.strictEqual(_arrTypeKind({ type: 'piano' }), 'keys'); + assert.strictEqual(_arrTypeKind({ type: ' ' }), null); + assert.strictEqual(_arrTypeKind({ name: 'Piano' }), null, 'no type field → null (name is the caller’s fallback)'); + assert.strictEqual(_arrTypeKind(null), null); +}); + +// ── isKeysArr: authored type wins; untyped falls back to name ────────── +const setArr = (arr) => { seedState({ arrangements: [arr], currentArr: 0 }); }; + +t('isKeysArr: an authored `type` overrides the name (both directions)', () => { + setArr({ name: 'Lead', type: 'keys' }); + assert.strictEqual(isKeysArr(), true, 'typed keys, non-keys name → keys'); + setArr({ name: 'Grand Piano', type: 'guitar' }); + assert.strictEqual(isKeysArr(), false, 'typed guitar, keys-looking name → NOT keys'); +}); + +t('isKeysArr: untyped falls back to the exact prefix name test (byte-identical)', () => { + setArr({ name: 'Piano' }); + assert.strictEqual(isKeysArr(), true, 'keys-prefix name → keys'); + setArr({ name: 'Synth Lead' }); + assert.strictEqual(isKeysArr(), true, 'synth-prefix → keys'); + setArr({ name: 'Electric Piano' }); + assert.strictEqual(isKeysArr(), false, 'NOT keys-prefixed → falls through, same as today'); + setArr({ name: 'Lead' }); + assert.strictEqual(isKeysArr(), false); +}); + +// ── the bass 4-vs-6 baseline honors type, else the name ─────────────── +t('_seedExtendedStringsFromTuning: type drives the 4-vs-6 baseline', () => { + // typed bass with a NON-bass name → baseline 4 (len 5 → +1 extended) + const a = [{ name: 'Rhythm', type: 'bass', tuning: [0, 0, 0, 0, 0] }]; + _seedExtendedStringsFromTuning(a, true); + assert.strictEqual(a[0]._extendedStrings, 1, 'typed bass → baseline 4'); + // typed KEYS named "Synth Bass" → NOT bass → baseline 6 (len 5 < 6 → unset) + const b = [{ name: 'Synth Bass', type: 'keys', tuning: [0, 0, 0, 0, 0] }]; + _seedExtendedStringsFromTuning(b, true); + assert.strictEqual(b[0]._extendedStrings, undefined, 'typed keys → baseline 6, len 5 not extended'); +}); + +t('_seedExtendedStringsFromTuning: untyped falls back to the /bass/ name test (byte-identical)', () => { + const bass = [{ name: 'Bass', tuning: [0, 0, 0, 0, 0] }]; // len 5, name bass → baseline 4 + _seedExtendedStringsFromTuning(bass, true); + assert.strictEqual(bass[0]._extendedStrings, 1, 'untyped bass name → baseline 4, +1'); + const gtr = [{ name: 'Rhythm', tuning: [0, 0, 0, 0, 0] }]; // len 5, non-bass → baseline 6, unset + _seedExtendedStringsFromTuning(gtr, true); + assert.strictEqual(gtr[0]._extendedStrings, undefined, 'untyped non-bass, len 5 → baseline 6, unset'); +}); + +for (const [name, fn] of tests) { + try { await fn(); pass++; console.log(' ok ' + name); } + catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } +} + +console.log(`\n${pass} passed, ${fail} failed`); +process.exit(fail ? 1 : 0); From 252363bd21846a08fb90ac94ecf681d9ab011862 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sun, 19 Jul 2026 22:29:53 -0500 Subject: [PATCH 02/18] feat(editor): consume the manifest instrument type on load + save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Makes the type-as-data seam live: the frontend now receives an authored instrument `type` and round-trips it, so the keys/bass predicates (previous commit) act on real data instead of a name guess. - Load (`_load_sloppak`): carry each manifest arrangement entry's `type` facet (feedpak-spec §5.2) onto the arrangement the frontend receives — mirroring the existing `id` carry — but only when authored/non-empty, so an untyped entry stays untyped and the frontend falls back to name inference (byte-identical). - Save (full-snapshot path): carry an editor-provided `type` into the rebuilt manifest entry, so a type SET in the editor persists and a new typed arrangement isn't re-inferred from its name by the infer-once pass. The existing `_merge_manifest_entry` still preserves an on-disk type when the editor sends none, and infer-once still stamps an untyped entry from its name. For existing packs this is behavior-identical (their `type` was inferred from the same name the predicates read); the win is that identity is now authoritative DATA that survives a rename and can be set explicitly — the contract drums-as-arrangements and a future "set instrument" action build on. Tests: tests/test_manifest_type_preserve.py +2 — a newly-typed arrangement persists its type, and an editor-set type overrides a stale on-disk value while unrelated preserved keys survive. pytest 381/0; JS 296/0 + 7; lint 0 err / 3 baseline. CHANGELOG updated. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- CHANGELOG.md | 12 ++++++++++++ routes.py | 23 +++++++++++++++++++++-- tests/test_manifest_type_preserve.py | 27 +++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 723ae71f..8f476adb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **A track's instrument is now DATA, not a guess from its name.** The editor used + to infer whether a part was keys / bass / guitar from its *name*, in a dozen + places with two subtly disagreeing rules — the reason renaming a track could flip + its lane layout, and why the rename dialog has to refuse some names. The pack + format already records an authored instrument `type` per arrangement; the editor + now **reads it** (carrying it through load and save) and lets it drive the + keys/bass view and string-count decisions, falling back to the old name inference + only when a track is untyped. Existing songs open exactly as before — but a part + named against its instrument (a guitar called "Grand Piano") finally reads as + what it *is*. Foundation for first-class drum tracks, vocals, and freely + renaming a track without changing its instrument. + - **The piano roll stretches, compacts and scrolls.** Its lane height used to be derived and untouchable — the whole pitch range packed into about 350px — so a wide range collapsed to four pixels per semitone: passable for reading, useless diff --git a/routes.py b/routes.py index 1f4a36fb..a93b02ed 100644 --- a/routes.py +++ b/routes.py @@ -4273,8 +4273,10 @@ def _load_sloppak(): # Build a per-arrangement id list from the manifest so we can map # edits back to the correct JSON file on save. arrangement_ids = [] + arrangement_types = [] for entry in (loaded.manifest.get("arrangements", []) or []): arrangement_ids.append(entry.get("id", "")) + arrangement_types.append(entry.get("type", "")) # Pick the audio for editor playback. A freshly-converted # sloppak has one `full` stem — use it directly. A stem-split @@ -4476,6 +4478,14 @@ def _safe_stem_path(stem_entry: dict) -> "Path | None": if not aid: aid = _arrangement_id(arr_data["name"], used_ids) arr_data["id"] = aid + # Carry the manifest `type` facet (feedpak-spec §5.2) onto the + # arrangement so the frontend reads instrument identity as DATA, + # not from the name. Only when authored/non-empty — an untyped + # entry stays untyped and the frontend falls back to name + # inference, byte-identical. + atype = arrangement_types[i] if i < len(arrangement_types) else "" + if isinstance(atype, str) and atype.strip(): + arr_data["type"] = atype.strip() # Round-trip load: populate the piano-roll from notation for any # notation-only arrangement (see _populate_notation_notes). @@ -4860,13 +4870,22 @@ def _build_wire(arr_dict, is_first): aid = _arrangement_id(ad.get("name", "arr"), used_ids) used_ids.add(aid) wire = _build_wire(ad, i == 0) - _entry = _merge_manifest_entry(_old_by_id.get(aid), { + _rebuilt = { "id": aid, "name": ad.get("name", "arr"), "file": f"arrangements/{aid}.json", "tuning": list(ad.get("tuning", [0]*6)), "capo": int(ad.get("capo", 0)), - }) + } + # Carry an editor-authored instrument `type` into the entry + # so a type SET in the editor persists — and a new typed + # arrangement (e.g. a drums-as-arrangement) isn't re-inferred + # from its name by the infer-once pass below. Only a non-empty + # string; the merge preserves an existing type when absent. + _atype = ad.get("type") + if isinstance(_atype, str) and _atype.strip(): + _rebuilt["type"] = _atype.strip() + _entry = _merge_manifest_entry(_old_by_id.get(aid), _rebuilt) # Carry any GP-import notation alongside the entry (NOT on # it — keeping it off the manifest entry means it can never # leak into manifest.yaml); the sidecar writer consumes it. diff --git a/tests/test_manifest_type_preserve.py b/tests/test_manifest_type_preserve.py index 5770636c..33841f55 100644 --- a/tests/test_manifest_type_preserve.py +++ b/tests/test_manifest_type_preserve.py @@ -119,3 +119,30 @@ def test_merge_does_not_mutate_inputs(): out["name"] = "changed" assert old["type"] == "bass" assert rebuilt["name"] == "A" + + +# ── editor-authored type round-trips (the save-carry contract) ──────────────── +# The save path now carries an editor-provided `type` into the rebuilt entry +# (previously the rebuilt entry never had one, so a set/changed type could only +# survive by accident of the merge preserving the OLD value). These pin that a +# type SET in the editor persists and can override a stale on-disk value — the +# contract drums-as-arrangements and a future "set instrument" action rely on. + +def test_a_newly_typed_arrangement_persists_its_type(): + # No old entry (a freshly added arrangement): the editor-set type survives. + rebuilt = {"id": "drm", "name": "Kit", "file": "arrangements/drm.json", + "tuning": [0] * 6, "capo": 0, "type": "drums"} + out = _merge_manifest_entry(None, rebuilt) + assert out["type"] == "drums" + + +def test_editor_set_type_overrides_a_stale_on_disk_type(): + # The rebuilt (editor) type wins over the old entry's type — so changing an + # instrument in the editor actually re-types the entry, rather than the old + # value silently sticking. + old = {"id": "x", "name": "X", "type": "guitar", "centOffset": -3} + rebuilt = {"id": "x", "name": "X", "file": "arrangements/x.json", + "tuning": [0] * 6, "capo": 0, "type": "drums"} + out = _merge_manifest_entry(old, rebuilt) + assert out["type"] == "drums", "editor type wins" + assert out["centOffset"] == -3, "unrelated preserved keys still survive" From 8bbd00d0dc9b39074cf4a820d635ef979c52520c Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sun, 19 Jul 2026 23:14:32 -0500 Subject: [PATCH 03/18] feat(editor): canonical arrKind resolver + Tracks-view instrument badge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turns the type-as-data seam into a visible feature and consolidates the name inference into one place. - `src/instrument.js`: `KEYS_PATTERN` moves here (its canonical home — keys.js re-exports it so its ~10 importers are unchanged), plus `_arrKindFromName` (the ONE runtime name-inference implementation) and `arrKind(arr)` — the canonical instrument kind, authored `type` first, name inference fallback. (arrangement.js `_arrKindPure` stays self-contained because its `@pure:rename-arr` block is sliced+eval'd by rename_part.test.mjs; it mirrors `_arrKindFromName`.) - `src/track-session.js`: the Tracks list badges a transcription row with its INSTRUMENT (GTR / BAS / KEY / DRM / VOX) via `arrKind`, not a generic "MIDI"; audio rows still show their layer (MIX / AUD). New pure `_trackKindBadgePure` so it unit-tests without the DOM; a mis-named part badges by what it IS. Tests: tests/instrument_type.test.mjs +4 — the re-exported KEYS_PATTERN, `_arrKindFromName` (keys-before-bass, /^drums/ prefix), `arrKind` type-over-name, and the badge for audio/drums/keys/bass(typed)/vocals rows. JS 296/0, lint 0 err / 3 baseline. CHANGELOG updated. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- CHANGELOG.md | 6 +++-- src/instrument.js | 26 ++++++++++++++++++++ src/keys.js | 9 ++++--- src/track-session.js | 25 +++++++++++++++++-- tests/instrument_type.test.mjs | 44 ++++++++++++++++++++++++++++++++-- 5 files changed, 99 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f476adb..8d994fa5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,8 +18,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 keys/bass view and string-count decisions, falling back to the old name inference only when a track is untyped. Existing songs open exactly as before — but a part named against its instrument (a guitar called "Grand Piano") finally reads as - what it *is*. Foundation for first-class drum tracks, vocals, and freely - renaming a track without changing its instrument. + what it *is*. **The Tracks list now badges each transcription track with its + instrument** (GTR / BAS / KEY / DRM / VOX) instead of a generic "MIDI", read + from that same authoritative identity. Foundation for first-class drum tracks, + vocals, and freely renaming a track without changing its instrument. - **The piano roll stretches, compacts and scrolls.** Its lane height used to be derived and untouchable — the whole pitch range packed into about 350px — so a diff --git a/src/instrument.js b/src/instrument.js index aa089e02..48e83343 100644 --- a/src/instrument.js +++ b/src/instrument.js @@ -46,3 +46,29 @@ export function _typeKind(rawType) { export function _arrTypeKind(arr) { return _typeKind(arr && arr.type); } + +// The keys-family name matcher (prefix-anchored): arrangements whose name STARTS +// with keys/piano/keyboard/synth open as piano-roll charts. Its canonical home +// is this leaf so the whole name→kind fallback lives in one place; keys.js +// re-exports it for the many sites that import KEYS_PATTERN from there. +export const KEYS_PATTERN = /^(keys|piano|keyboard|synth)/i; + +// The runtime instrument kind inferred from a NAME — the legacy fallback for an +// untyped track. Prefix-anchored keys/drums, then /bass/ anywhere, else guitar; +// keys is tested before bass (so "Synth Bass" reads keys). This is now the ONE +// name-inference implementation — arrangement.js `_arrKindPure` delegates here. +export function _arrKindFromName(name) { + const n = String(name || ''); + if (KEYS_PATTERN.test(n)) return 'keys'; + if (/^drums/i.test(n)) return 'drums'; + if (/bass/i.test(n)) return 'bass'; + return 'guitar'; +} + +// The canonical instrument kind of an arrangement: an authored `type` WINS +// (identity is DATA), else name inference. The type-authoritative resolver the +// Tracks-view badge, the remaining view/routing sites, and drums-as-arrangements +// read — so identity is decided in exactly one place. +export function arrKind(arr) { + return _arrTypeKind(arr) || _arrKindFromName(arr && arr.name); +} diff --git a/src/keys.js b/src/keys.js index 7a0afa8b..07defc05 100644 --- a/src/keys.js +++ b/src/keys.js @@ -19,7 +19,10 @@ import { _openMidiForArr, _soundingPitchPure, _stringCountFor } from './lanes.js import { notes } from './notes.js'; import { S } from './state.js'; import { PIANO_NOTE_NAMES, _noteNamesForKeyPure } from './theory.js'; -import { _arrTypeKind } from './instrument.js'; +import { KEYS_PATTERN, _arrTypeKind } from './instrument.js'; +// Re-export so the many sites that import KEYS_PATTERN from keys.js keep working; +// its canonical definition now lives in the instrument-identity leaf. +export { KEYS_PATTERN }; // ── Piano roll constants ──────────────────────────────────────────── export const PIANO_OCTAVE_COLORS = [ @@ -28,10 +31,6 @@ export const PIANO_OCTAVE_COLORS = [ ]; export let PIANO_LANE_H = 10; // pixels per MIDI semitone export let pianoRange = { lo: 36, hi: 96 }; // MIDI range, updated per arrangement -// Names that should open in keys (piano-roll) editor mode. Arrangements -// named "Piano", "Keyboard", or "Synth" render as piano-roll charts rather -// than 6-string guitar charts. -export const KEYS_PATTERN = /^(keys|piano|keyboard|synth)/i; // Per-part editing-view choice (V2/V9 of EDITOR-VIEW-MODALITY-DESIGN): // 'string' (fretted lanes) or 'piano' (the roll). Keys-DATA arrangements diff --git a/src/track-session.js b/src/track-session.js index 3af6c14d..3eefeac1 100644 --- a/src/track-session.js +++ b/src/track-session.js @@ -33,6 +33,7 @@ import { host } from './host.js'; import { _renameGuardPure } from './arrangement.js'; import { _partViewKeyPure } from './keys.js'; +import { arrKind } from './instrument.js'; import { _mixerPanelRefresh, _mixerPartStatePure, mixerSetPart, mixerTogglePart } from './mixer-panel.js'; import { S, markSessionDirty } from './state.js'; import { _editorEscHtml, setStatus } from './ui.js'; @@ -41,6 +42,24 @@ const MASTER_ID = 'master'; const DRUM_TARGET_ID = 'drums'; const VERSION = 2; const TRACK_LANE_DEFAULT = 56; + +// The Tracks-view kind badge [abbr, tooltip]. An audio row shows its LAYER +// (Mix / Audio); a transcription row shows its INSTRUMENT, read +// type-authoritatively via `arrKind` (so a mis-named part badges by what it IS, +// not its name) — the drum target is drums by identity. Pure: takes the row + +// arrangements, so it unit-tests without the DOM. +const _KIND_ABBR = { + guitar: ['GTR', 'Guitar'], bass: ['BAS', 'Bass'], keys: ['KEY', 'Keys'], + drums: ['DRM', 'Drums'], vocals: ['VOX', 'Vocals'], +}; +export function _trackKindBadgePure(row, arrangements) { + if (!row) return ['', '']; + if (row.type === 'audio') return row.sourceKind === 'master' ? ['MIX', 'Master mix'] : ['AUD', 'Audio']; + if (row.targetId === DRUM_TARGET_ID) return _KIND_ABBR.drums; + const idx = (row.mixKey && row.mixKey.startsWith('arr:')) ? Number(row.mixKey.slice(4)) : -1; + const arr = (idx >= 0 && Array.isArray(arrangements)) ? arrangements[idx] : null; + return _KIND_ABBR[arrKind(arr)] || _KIND_ABBR.guitar; +} const TRACK_LANE_MIN = 28; const TRACK_LANE_MAX = 160; const idOf = (value) => typeof value === 'string' && value.trim().length > 0 && value.length <= 160 ? value.trim() : ''; @@ -697,8 +716,10 @@ function render() { const style = `--track-indent:${indent}px;--track-row-height:${height}px`; const selected = row.id === S.selectedTrackId ? ' editor-track-selected' : ''; if (row.type === 'folder') return `
${trackName(row, `${name}`)}${resizeGrip(row)}
`; - if (row.type === 'audio') return `
${row.sourceKind === 'master' ? 'MIX' : 'AUD'}${trackName(row, `${name}`)}${mixControls(row)}${resizeGrip(row)}
`; - return `
${row.targetId === DRUM_TARGET_ID ? 'DRM' : 'MIDI'}${trackName(row, ``)}${mixControls(row)}${resizeGrip(row)}
`; + const [kindAbbr, kindTitle] = _trackKindBadgePure(row, S.arrangements); + const kindBadge = `${kindAbbr}`; + if (row.type === 'audio') return `
${kindBadge}${trackName(row, `${name}`)}${mixControls(row)}${resizeGrip(row)}
`; + return `
${kindBadge}${trackName(row, ``)}${mixControls(row)}${resizeGrip(row)}
`; }).join('')}`; const list = el.querySelector('.editor-track-session-list'); if (list) list.scrollTop = Math.max(0, Number(S.trackScrollY) || 0); diff --git a/tests/instrument_type.test.mjs b/tests/instrument_type.test.mjs index 37e02e22..c33c0178 100644 --- a/tests/instrument_type.test.mjs +++ b/tests/instrument_type.test.mjs @@ -18,9 +18,10 @@ */ import assert from 'node:assert'; -import { _typeKind, _arrTypeKind } from '../src/instrument.js'; -import { isKeysArr } from '../src/keys.js'; +import { _typeKind, _arrTypeKind, _arrKindFromName, arrKind } from '../src/instrument.js'; +import { isKeysArr, KEYS_PATTERN } from '../src/keys.js'; import { _seedExtendedStringsFromTuning } from '../src/lanes.js'; +import { _trackKindBadgePure } from '../src/track-session.js'; import { seedState } from './_history_env.mjs'; let pass = 0; let fail = 0; @@ -97,6 +98,45 @@ t('_seedExtendedStringsFromTuning: untyped falls back to the /bass/ name test (b assert.strictEqual(gtr[0]._extendedStrings, undefined, 'untyped non-bass, len 5 → baseline 6, unset'); }); +// ── _arrKindFromName + arrKind (the canonical resolver) ─────────────── +t('KEYS_PATTERN is re-exported from keys.js (its home is now the leaf)', () => { + assert.strictEqual(KEYS_PATTERN.test('Piano'), true); + assert.strictEqual(KEYS_PATTERN.test('Electric Piano'), false, 'prefix-anchored, unchanged'); +}); + +t('_arrKindFromName mirrors the legacy runtime inference (keys before bass)', () => { + assert.strictEqual(_arrKindFromName('Piano'), 'keys'); + assert.strictEqual(_arrKindFromName('Synth Lead'), 'keys'); + assert.strictEqual(_arrKindFromName('Drums'), 'drums'); + assert.strictEqual(_arrKindFromName('Drums 2'), 'drums'); + assert.strictEqual(_arrKindFromName('Drumkit'), 'guitar', 'prefix is /^drums/ — "Drumkit" is not "drums"'); + assert.strictEqual(_arrKindFromName('Synth Bass'), 'keys', 'keys wins over bass'); + assert.strictEqual(_arrKindFromName('Bass'), 'bass'); + assert.strictEqual(_arrKindFromName('Lead'), 'guitar'); + assert.strictEqual(_arrKindFromName(''), 'guitar'); +}); + +t('arrKind: authored type wins, name inference is the fallback', () => { + assert.strictEqual(arrKind({ name: 'Lead', type: 'drums' }), 'drums', 'type wins'); + assert.strictEqual(arrKind({ name: 'Grand Piano', type: 'guitar' }), 'guitar', 'type wins over keys name'); + assert.strictEqual(arrKind({ name: 'Piano' }), 'keys', 'untyped → name inference'); + assert.strictEqual(arrKind({ name: 'Bass' }), 'bass'); + assert.strictEqual(arrKind({ name: 'Backing Vox', type: 'vocals' }), 'vocals'); + assert.strictEqual(arrKind(null), 'guitar', 'no arr → guitar default'); +}); + +// ── the Tracks-view kind badge ──────────────────────────────────────── +t('_trackKindBadgePure: audio shows the layer, transcription shows the instrument', () => { + assert.deepStrictEqual(_trackKindBadgePure({ type: 'audio', sourceKind: 'master' }, []), ['MIX', 'Master mix']); + assert.deepStrictEqual(_trackKindBadgePure({ type: 'audio', sourceKind: 'stem' }, []), ['AUD', 'Audio']); + assert.deepStrictEqual(_trackKindBadgePure({ type: 'transcription', targetId: 'drums' }, []), ['DRM', 'Drums']); + const arrs = [{ name: 'Lead' }, { name: 'Piano' }, { name: 'Rhythm', type: 'bass' }, { name: 'Choir', type: 'vocals' }]; + assert.deepStrictEqual(_trackKindBadgePure({ type: 'transcription', targetId: 'Lead', mixKey: 'arr:0' }, arrs), ['GTR', 'Guitar']); + assert.deepStrictEqual(_trackKindBadgePure({ type: 'transcription', targetId: 'Piano', mixKey: 'arr:1' }, arrs), ['KEY', 'Keys']); + assert.deepStrictEqual(_trackKindBadgePure({ type: 'transcription', targetId: 'Rhythm', mixKey: 'arr:2' }, arrs), ['BAS', 'Bass'], 'type wins over the non-bass name'); + assert.deepStrictEqual(_trackKindBadgePure({ type: 'transcription', targetId: 'Choir', mixKey: 'arr:3' }, arrs), ['VOX', 'Vocals']); +}); + for (const [name, fn] of tests) { try { await fn(); pass++; console.log(' ok ' + name); } catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } From 125b5a550e5d6846d2e67f2cfea1a7509979d9c4 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sun, 19 Jul 2026 23:30:26 -0500 Subject: [PATCH 04/18] feat(editor): route the keys/drums view + audio sites through arrKind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the type-authoritative identity from the keys/bass DATA predicates to the keys/drums VIEW and audio routing — every one honors an authored `type` first, falling back to its exact prior name test, so all are byte-identical for untyped/legacy packs (verified: full suite green) but correct for a typed part named against its instrument. Converted (13 sites, 6 files): key-view.js (the view switcher's fretted / isDrums / "keys are piano-locked" / drums-have-no-tab decisions), audio.js (drum parts voice their rhythm as claps, not GM pitch), tab-view-live.js (the engraved lens refuses keys/drums), main.js (the Strings button hides for keys/ drums), parts-view.js (the keys silhouette mini-roll), keys.js `_rollPitchCtxFor` (no fretted pitch context for keys). KEYS_PATTERN's now-unused imports dropped where every use was converted (its home is the instrument leaf; keys.js re-exports it for the sites still on the name). Still on the name (a follow-up completes them, and the rename guard STAYS as the safety net until every runtime layout reader is converted): the bass/string-count sites (strings.js, lanes add/remove) and the import/export helpers. Tests: tests/instrument_type.test.mjs +1 — `_rollPitchCtxFor` returns null for a typed-keys part (and a real ctx for a typed guitar named "Piano"). JS 296/0, lint 0 err / 3 baseline. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- src/audio.js | 5 +++-- src/key-view.js | 13 +++++++------ src/keys.js | 4 ++-- src/main.js | 8 ++++---- src/parts-view.js | 4 ++-- src/tab-view-live.js | 6 +++--- tests/instrument_type.test.mjs | 10 +++++++++- 7 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/audio.js b/src/audio.js index 4e8f028c..c21ef795 100644 --- a/src/audio.js +++ b/src/audio.js @@ -31,6 +31,7 @@ import { host } from './host.js'; import { _pickOnsetsPure, _spectralFluxOnsetsPlan, _spectralFluxStep } from './onsets.js'; import { _tourNoteAction } from './tour.js'; import { _rollMidiForNote, _rollPitchCtx, _rollPitchCtxFor, midiToFreq } from './keys.js'; +import { arrKind } from './instrument.js'; import { _recState } from './midi-record.js'; import { notes } from './notes.js'; import { S } from './state.js'; @@ -2155,7 +2156,7 @@ function _stemGainsReset() { // per arrangement) — drum parts return [] here; their hits clap instead. function _bandPartPitchedEvents(idx) { const arr = S.arrangements[idx]; - if (!arr || /^drums/i.test(arr.name || '')) return []; + if (!arr || arrKind(arr) === 'drums') return []; const rctx = _rollPitchCtxFor(arr); return _gmSanitizeEventsPure((arr.notes || []).map(n => ({ t: n.time, @@ -2334,7 +2335,7 @@ function _guideTick() { // no pitch, so _bandPartPitchedEvents returns []) claps its rhythm // through this part's gain, else it voices neither GM nor clap and // goes silent (review #280 follow-up; GM percussion here is a follow-up). - if (arr && /^drums/i.test(arr.name || '')) { + if (arr && arrKind(arr) === 'drums') { const times = _guideSanitizeTimesPure((arr.notes || []).map(n => n.time)); for (const t of _guideClapTimesInWindowPure(times, from, to)) { const k = _bandFiredKeyPure(part.key, t); diff --git a/src/key-view.js b/src/key-view.js index 939abe9c..8e949537 100644 --- a/src/key-view.js +++ b/src/key-view.js @@ -6,7 +6,8 @@ import { hideAddNote } from './add-note.js'; import { hideContextMenu } from './context-menu.js'; import { _loadEditorKeyIfNeeded, _persistEditorKey, editorKeyHighlightEnabled } from './draw.js'; -import { KEYS_PATTERN, _partViewKeyPure, _rollMidiForNote, _rollPitchCtx, _viewPrefs, _viewPrefsSave, updatePianoRange, viewFor } from './keys.js'; +import { _partViewKeyPure, _rollMidiForNote, _rollPitchCtx, _viewPrefs, _viewPrefsSave, updatePianoRange, viewFor } from './keys.js'; +import { arrKind } from './instrument.js'; import { notes } from './notes.js'; import { S } from './state.js'; import { PIANO_NOTE_NAMES, SCALE_INTERVALS, SCALE_LABELS, _detectKeyPure, _noteNamesForKeyPure } from './theory.js'; @@ -103,8 +104,8 @@ export function _refreshViewSwitch() { const el = document.getElementById('editor-view-switch'); if (!el) return; const arr = S.arrangements.length ? S.arrangements[S.currentArr] : null; - const fretted = !!arr && !KEYS_PATTERN.test(arr.name || ''); - const isDrums = !!arr && /^drums/i.test(arr.name || ''); + const fretted = !!arr && arrKind(arr) !== 'keys'; + const isDrums = !!arr && arrKind(arr) === 'drums'; // The DRUM editor gets its own two options (grid / percussion staff) — // its mode flag stays on under the drum lens, so it is the one signal. const drumMode = !!S.drumEditMode && !!S.drumTab; @@ -172,7 +173,7 @@ export const editorSetViewMode = (mode) => { if (mode !== 'string' && mode !== 'piano') return; const arr = S.arrangements.length ? S.arrangements[S.currentArr] : null; if (!arr) return; - if (KEYS_PATTERN.test(arr.name || '')) { + if (arrKind(arr) === 'keys') { setStatus('Keys tracks always use the piano roll'); return; } @@ -203,7 +204,7 @@ export const editorSetViewMode = (mode) => { export function _editorCycleViewMode() { const arr = S.arrangements.length ? S.arrangements[S.currentArr] : null; if (!arr) { setStatus('Load a song first'); return true; } - if (KEYS_PATTERN.test(arr.name || '')) { + if (arrKind(arr) === 'keys') { setStatus('Keys tracks always use the piano roll'); return true; } @@ -218,7 +219,7 @@ export function _editorCycleViewMode() { // Drums have no tab (same refusal as the lens's own guard) — the // toggle would refuse WITHOUT changing mode and the cycle would // stick on the roll. Skip the Tab stop and wrap to String view. - if (/^drums/i.test(arr.name || '')) { + if (arrKind(arr) === 'drums') { window.editorSetViewMode('string'); return true; } diff --git a/src/keys.js b/src/keys.js index 07defc05..c5e5b9b3 100644 --- a/src/keys.js +++ b/src/keys.js @@ -19,7 +19,7 @@ import { _openMidiForArr, _soundingPitchPure, _stringCountFor } from './lanes.js import { notes } from './notes.js'; import { S } from './state.js'; import { PIANO_NOTE_NAMES, _noteNamesForKeyPure } from './theory.js'; -import { KEYS_PATTERN, _arrTypeKind } from './instrument.js'; +import { KEYS_PATTERN, _arrTypeKind, arrKind } from './instrument.js'; // Re-export so the many sites that import KEYS_PATTERN from keys.js keep working; // its canonical definition now lives in the instrument-identity leaf. export { KEYS_PATTERN }; @@ -133,7 +133,7 @@ export function _rollPitchCtx() { // The per-arrangement form (multi-track MIDI playback schedules EVERY part, // not just the current one): same rules, arr injected. export function _rollPitchCtxFor(arr) { - if (!arr || KEYS_PATTERN.test(arr.name || '')) return null; + if (!arr || arrKind(arr) === 'keys') return null; const laneCount = _stringCountFor(arr); const tuning = (Array.isArray(arr.tuning) ? arr.tuning : []).slice(0, laneCount); while (tuning.length < laneCount) tuning.push(0); diff --git a/src/main.js b/src/main.js index 42064c92..49f7e0d4 100644 --- a/src/main.js +++ b/src/main.js @@ -186,8 +186,9 @@ import { LABEL_W, TIMELINE_TOP, clampZoom, setLaneMetrics } from './geometry.js'; import { _laneClipActive, applyLaneScrollBounds, drawLaneScrollbar, laneBandTop } from './lane-scroll.js'; import { - KEYS_PATTERN, _rollLockNotice, + _rollLockNotice, _rollMidiForNote, _rollPitchCtx, _rollReadOnly, editorKeyNoteNames, isKeysMode, midiToNote, updatePianoRange } from './keys.js'; +import { arrKind } from './instrument.js'; import { _restoreSuggestedMarks, _saveSuggestedMarks, _suggestedCount, chords, notes @@ -1068,9 +1069,8 @@ function updateArrangementSelector() { const stringsBtn = document.getElementById('editor-strings-btn'); if (stringsBtn) { const active = S.arrangements[S.currentArr]; - const stringsMode = !!active - && !KEYS_PATTERN.test(active.name || '') - && !/^drums/i.test(active.name || ''); + const activeKind = active && arrKind(active); + const stringsMode = !!active && activeKind !== 'keys' && activeKind !== 'drums'; stringsBtn.classList.toggle('hidden', !S.sessionId || !stringsMode); } diff --git a/src/parts-view.js b/src/parts-view.js index b1c57a99..525521e3 100644 --- a/src/parts-view.js +++ b/src/parts-view.js @@ -9,7 +9,7 @@ import { ctx } from './canvas.js'; import { hideContextMenu } from './context-menu.js'; import { DRUM_PIECE_META, _refreshDrumEditButton } from './drum.js'; import { LABEL_W, TIMELINE_TOP, timeToX, xToTime } from './geometry.js'; -import { KEYS_PATTERN } from './keys.js'; +import { arrKind } from './instrument.js'; import { _stringCountFor } from './lanes.js'; import { _downbeatTimes } from './loop.js'; import { _recState } from './midi-record.js'; @@ -148,7 +148,7 @@ function _partsDrawSilhouette(part, y0, laneH, w) { } } if (!events.length) return; - if (KEYS_PATTERN.test(arr.name || '')) { + if (arrKind(arr) === 'keys') { // Keys: pitch-mapped mini roll, range auto-fit to the part. let lo = Infinity, hi = -Infinity; for (const n of events) { diff --git a/src/tab-view-live.js b/src/tab-view-live.js index 9a673fdf..79cb507e 100644 --- a/src/tab-view-live.js +++ b/src/tab-view-live.js @@ -27,7 +27,7 @@ import { setStatus } from './ui.js'; import { notes } from './notes.js'; import { beatOf } from './beats.js'; import { _openMidiForArr, _stringCountFor } from './lanes.js'; -import { KEYS_PATTERN } from './keys.js'; +import { arrKind } from './instrument.js'; import { _alphaTexFromDrumHitsPure, _alphaTexFromNotesPure } from './alphatex.js'; import { TAB_RENDERER_FONT_DIR, _tabPreviewLoadScript } from './tab-preview.js'; @@ -231,7 +231,7 @@ export function _tabViewPing() { } } else { const arr = S.arrangements && S.arrangements[S.currentArr]; - if (!arr || KEYS_PATTERN.test(arr.name || '') || /^drums/i.test(arr.name || '')) { + if (!arr || arrKind(arr) === 'keys' || arrKind(arr) === 'drums') { S.tabViewMode = false; _tabViewHideIfShown(); setStatus('Tab view is for fretted tracks — switched back to this track’s normal view.'); @@ -285,7 +285,7 @@ export function editorToggleTabView(force) { } const arr = S.arrangements && S.arrangements[S.currentArr]; if (!arr) { setStatus('Load a song first.'); return true; } - if (KEYS_PATTERN.test(arr.name || '') || /^drums/i.test(arr.name || '')) { + if (arrKind(arr) === 'keys' || arrKind(arr) === 'drums') { setStatus('Tab view is for fretted tracks — keys and drums have no tab.'); return true; } diff --git a/tests/instrument_type.test.mjs b/tests/instrument_type.test.mjs index c33c0178..3b1a2fb9 100644 --- a/tests/instrument_type.test.mjs +++ b/tests/instrument_type.test.mjs @@ -19,7 +19,7 @@ import assert from 'node:assert'; import { _typeKind, _arrTypeKind, _arrKindFromName, arrKind } from '../src/instrument.js'; -import { isKeysArr, KEYS_PATTERN } from '../src/keys.js'; +import { isKeysArr, KEYS_PATTERN, _rollPitchCtxFor } from '../src/keys.js'; import { _seedExtendedStringsFromTuning } from '../src/lanes.js'; import { _trackKindBadgePure } from '../src/track-session.js'; import { seedState } from './_history_env.mjs'; @@ -137,6 +137,14 @@ t('_trackKindBadgePure: audio shows the layer, transcription shows the instrumen assert.deepStrictEqual(_trackKindBadgePure({ type: 'transcription', targetId: 'Choir', mixKey: 'arr:3' }, arrs), ['VOX', 'Vocals']); }); +// ── view routing honors type (a converted reader) ──────────────────── +t('_rollPitchCtxFor: a typed-keys part has no fretted context regardless of name', () => { + assert.strictEqual(_rollPitchCtxFor({ name: 'Lead', type: 'keys' }), null, 'typed keys → null (no fretted ctx)'); + assert.strictEqual(_rollPitchCtxFor({ name: 'Piano' }), null, 'untyped keys name → null (fallback, unchanged)'); + const gtr = _rollPitchCtxFor({ name: 'Grand Piano', type: 'guitar', tuning: [40, 45, 50, 55, 59, 64], capo: 0 }); + assert.notStrictEqual(gtr, null, 'typed guitar named "Piano" → a real fretted ctx (type wins)'); +}); + for (const [name, fn] of tests) { try { await fn(); pass++; console.log(' ok ' + name); } catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } From 660261e86385ca80d175a779ad754755aeac89b6 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Mon, 20 Jul 2026 00:07:44 -0500 Subject: [PATCH 05/18] feat(editor): route the string-count + import/export readers through type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finishes the RENAME-SAFETY-CRITICAL readers: every string-count / open-tuning decision — where a bass↔guitar flip would strand notes onto invisible strings — now honors an authored `type` first, via `_isBassArr` (new in instrument.js). Crucially `_isBassArr` keeps the INDEPENDENT `/bass/` name fallback, NOT `arrKind === 'bass'`: bass and keys are independent facets in the legacy inference, so an untyped "Synth Bass" must stay a 4-string bass (arrKind, being single-kind with keys-wins, would wrongly make it 6). Byte-identical for untyped. Converted: lanes.js `_stringCountFor` / `_openMidiForArr` / isBassArr / `_seedExtendedStringsFromTuning` (unified on `_isBassArr`); strings.js (the modal gate + all 7 add/remove-string bass sites + `_stringButtonsVisiblePure`, now kind-based); file-ops.js archive-limit; and the import/create readers (import.js keys/bass filters, create.js keys-detection). KEYS_PATTERN's now-unused imports dropped where every use was converted. Still name-based ON PURPOSE (the capstone follow-up converts them + relaxes the rename guard): the @pure/self-contained NON-stranding readers — gm-guide voice `_gmKindPure`, the tab-preview / gp5-export guards, the parts-view silhouette `_partsArrKindPure` — and the rename guard's own name inference (arrangement.js), which stays until every reader honors type. Tests: instrument_type.test.mjs +2 — `_isBassArr` independent-fallback and `_stringCountFor` type-driven baseline; strings_modal.test.mjs injects the real `_isBassArr` into its sliced-handler env; canvas_string_buttons passes kinds. JS 296/0, lint 0 err / 3 baseline. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- src/create.js | 6 +++--- src/file-ops.js | 3 ++- src/import.js | 10 +++++----- src/instrument.js | 11 +++++++++++ src/lanes.js | 16 +++++----------- src/strings.js | 28 +++++++++++++++------------- tests/canvas_string_buttons.test.mjs | 12 ++++++------ tests/instrument_type.test.mjs | 20 +++++++++++++++++++- tests/strings_modal.test.mjs | 4 +++- 9 files changed, 69 insertions(+), 41 deletions(-) diff --git a/src/create.js b/src/create.js index 65380063..fb5271f2 100644 --- a/src/create.js +++ b/src/create.js @@ -26,7 +26,8 @@ import { import { _handshapesAreDirty, flattenChords, reconstructChords } from './chords.js'; import { EditHistory } from './history.js'; import { host } from './host.js'; -import { KEYS_PATTERN, isKeysMode, updatePianoRange } from './keys.js'; +import { isKeysMode, updatePianoRange } from './keys.js'; +import { arrKind } from './instrument.js'; import { _seedExtendedStringsFromTuning } from './lanes.js'; import { S, markSessionDirty, markSessionSaved } from './state.js'; import { _marksSanitizePure } from './tempo-marks.js'; @@ -2422,8 +2423,7 @@ export async function editorApplyCreateResult(data) { resetStemAudioCache(); // …nor stale stem buffers void syncStemAudio().finally(() => host.draw()); // decode stems, then repaint their lanes const _importHasDrums = !!(S.drumTab && (S.drumTab.hits || []).length); - const _importHasKeys = (S.arrangements || []).some( - a => KEYS_PATTERN.test(a.name || '')); + const _importHasKeys = (S.arrangements || []).some(a => arrKind(a) === 'keys'); if (_importHasDrums || _importHasKeys) S.format = 'sloppak'; // Reset offset UI so _effectiveAudioOffset() doesn't carry over a diff --git a/src/file-ops.js b/src/file-ops.js index 64b88fd3..fdfb7ba2 100644 --- a/src/file-ops.js +++ b/src/file-ops.js @@ -11,6 +11,7 @@ import { editorBuild } from './create.js'; import { EditHistory } from './history.js'; import { isKeysMode, rollResetLaneH, updatePianoRange } from './keys.js'; import { _seedExtendedStringsFromTuning, _stringCountFor } from './lanes.js'; +import { _isBassArr } from './instrument.js'; import { _updateLoopRegionControls } from './loop.js'; import { _marksSanitizePure } from './tempo-marks.js'; import { _recState } from './midi-record.js'; @@ -563,7 +564,7 @@ export function filterSongs(q) { export function _activeArrangementExceedsArchiveLimit() { const a = S.arrangements[S.currentArr]; if (!a) return false; - const isBass = /bass/i.test(a.name || ''); + const isBass = _isBassArr(a); const roleLimit = isBass ? 4 : 6; return _stringCountFor(a) > roleLimit; } diff --git a/src/import.js b/src/import.js index 8ffb6487..ab7792cf 100644 --- a/src/import.js +++ b/src/import.js @@ -11,7 +11,8 @@ import { _editorEscHtml, setStatus } from './ui.js'; import { ReplaceArrangementChartCmd } from './commands.js'; import { TempoGridCmd, _tempoRemapMarksByTime } from './tempo.js'; import { DRUM_PIECE_META, DRUM_PIECE_ORDER, _drumImportHitPure } from './drum.js'; -import { KEYS_PATTERN, _uniqueKeysName, updatePianoRange } from './keys.js'; +import { _uniqueKeysName, updatePianoRange } from './keys.js'; +import { arrKind, _isBassArr } from './instrument.js'; import { flattenChords } from './chords.js'; import { host } from './host.js'; @@ -593,7 +594,7 @@ async function _addEmptyArrangement(arrangement, statusElId, okStatus) { if (sel) sel.value = S.currentArr; flattenChords(); - if (KEYS_PATTERN.test(arrangement.name || '') && typeof updatePianoRange === 'function') { + if (arrKind(arrangement) === 'keys' && typeof updatePianoRange === 'function') { updatePianoRange(); } host.updateArrangementSelector(); @@ -729,9 +730,8 @@ export function editorImportGuitarRefreshReplaceTargets() { const eligible = S.arrangements .map((a, i) => ({ a, i })) .filter(({ a }) => { - const nm = a.name || ''; - if (KEYS_PATTERN.test(nm) || /^drums/i.test(nm)) return false; - return /bass/i.test(nm) === wantBass; + if (arrKind(a) === 'keys' || arrKind(a) === 'drums') return false; + return _isBassArr(a) === wantBass; }); if (replaceSel) { replaceSel.innerHTML = eligible.map(({ a, i }) => diff --git a/src/instrument.js b/src/instrument.js index 48e83343..e9ec9733 100644 --- a/src/instrument.js +++ b/src/instrument.js @@ -72,3 +72,14 @@ export function _arrKindFromName(name) { export function arrKind(arr) { return _arrTypeKind(arr) || _arrKindFromName(arr && arr.name); } + +// Bass predicate — type-authoritative, but its NAME fallback stays the INDEPENDENT +// `/bass/` test, NOT `arrKind === 'bass'`. Bass and keys are independent facets in +// the legacy inference ("Synth Bass" is bass for string-count AND keys for the +// view), so the single-kind `arrKind` (keys wins) would wrongly flip a "Synth +// Bass" from a 4-string baseline to 6. Every string-count / open-tuning site that +// used `/bass/i.test(arr.name)` must route through here to stay byte-identical. +export function _isBassArr(arr) { + const k = _arrTypeKind(arr); + return k ? k === 'bass' : /bass/i.test((arr && arr.name) || ''); +} diff --git a/src/lanes.js b/src/lanes.js index 7f023336..6403c884 100644 --- a/src/lanes.js +++ b/src/lanes.js @@ -7,7 +7,7 @@ */ import { S } from './state.js'; -import { _arrTypeKind } from './instrument.js'; +import { _isBassArr } from './instrument.js'; export const MAX_LANES = 8; @@ -67,11 +67,7 @@ function isBassArr() { if (!S.arrangements.length) return false; const arr = S.arrangements[S.currentArr]; if (!arr) return false; - // Authored `type` wins; else the legacy /bass/ name test (an untyped - // "Synth Bass" stays bass here, exactly as before — a typed keys part - // named that resolves to keys, its authored identity). - const k = _arrTypeKind(arr); - return k ? k === 'bass' : /bass/i.test(arr.name || ''); + return _isBassArr(arr); } // Active arrangement string count. Mirrors lib/song.py:arrangement_string_count @@ -101,9 +97,7 @@ function isBassArr() { export function _seedExtendedStringsFromTuning(arrangements, authoritativeLength) { for (const arr of arrangements || []) { if (typeof arr._extendedStrings === 'number') continue; // already set - // Authored `type` wins for the 4-vs-6 baseline; else the legacy name test. - const k = _arrTypeKind(arr); - const isBass = k ? k === 'bass' : /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); // type-authoritative; independent /bass/ fallback const baseline = isBass ? 4 : 6; const tuningLen = Array.isArray(arr.tuning) ? arr.tuning.length : baseline; if (tuningLen > 6) { @@ -124,7 +118,7 @@ export function _seedExtendedStringsFromTuning(arrangements, authoritativeLength export function _stringCountFor(arr) { if (!arr) return 6; - const isBass = /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); const baseline = isBass ? 4 : 6; // User-added strings via the Strings modal — authoritative even // when tuning happens to be ambiguous length 6 (the standard RS-XML @@ -264,7 +258,7 @@ const _BASS_OPEN_MIDI = [28, 33, 38, 43]; // 4-string standard // extended/trimmed to `laneCount` strings. Extended-range strings // are prepended at the low end (matching how AddStringCmd works). export function _openMidiForArr(arr, laneCount) { - const isBass = /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); const base = isBass ? _BASS_OPEN_MIDI.slice() : _GUITAR_OPEN_MIDI.slice(); // Extend low end: each additional low string is 5 semitones below // the current lowest (perfect 4th), matching standard guitar/bass diff --git a/src/strings.js b/src/strings.js index 4f19f6de..99af4382 100644 --- a/src/strings.js +++ b/src/strings.js @@ -4,7 +4,8 @@ import { AddStringCmd, RemoveStringCmd, RemoveStringWithNotesCmd } from './commands.js'; import { LANE_H, TIMELINE_TOP, WAVEFORM_H } from './geometry.js'; -import { isKeysMode, KEYS_PATTERN } from './keys.js'; +import { isKeysMode } from './keys.js'; +import { arrKind, _isBassArr } from './instrument.js'; import { _stringCountFor, laneLabels } from './lanes.js'; import { S, editGen } from './state.js'; import { host } from './host.js'; @@ -98,11 +99,12 @@ class SetStringTuningCmd { // Every lens that owns the timeline instead (piano roll, drum editor, // Tempo Map, Parts, Tab view) hides them, as do the non-fretted // arrangement kinds the Strings modal already refuses. -export function _stringButtonsVisiblePure(arrName, flags) { +export function _stringButtonsVisiblePure(kind, flags) { const f = flags || {}; if (f.keysMode || f.drumEdit || f.tempoMap || f.partsView || f.tabView) return false; - const name = arrName || ''; - return !KEYS_PATTERN.test(name) && !/^drums/i.test(name); + // `kind` is the resolved instrument (arrKind — type-authoritative); the + // string controls are for fretted parts only, so keys/drums hide them. + return kind !== 'keys' && kind !== 'drums'; } // Tooltip copy for the buttons. Count-centric: they always grow/shrink @@ -132,7 +134,7 @@ export function _stringRemoveLabelPure(isBass, cur) { function _stringsRangeForActive() { const arr = S.arrangements[S.currentArr]; - const isBass = arr && /bass/i.test(arr.name || ''); + const isBass = arr && _isBassArr(arr); return _stringsRangePure(!!isBass); } @@ -154,7 +156,7 @@ function _renderStringsModal() { const tuning = (arr.tuning || []).slice(0, labels.length); while (tuning.length < labels.length) tuning.push(0); const { min, max } = _stringsRangeForActive(); - const isBass = /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); const summary = document.getElementById('editor-strings-summary'); if (summary) { @@ -244,7 +246,7 @@ function _renderStringsModal() { export const editorShowStringsModal = () => { const arr = S.arrangements[S.currentArr]; if (!arr) return; - if (KEYS_PATTERN.test(arr.name || '') || /^drums/i.test(arr.name || '')) return; + if (arrKind(arr) === 'keys' || arrKind(arr) === 'drums') return; document.getElementById('editor-strings-modal').classList.remove('hidden'); _renderStringsModal(); }; @@ -256,7 +258,7 @@ export const editorHideStringsModal = () => { export const editorAddString = (pos) => { const arr = S.arrangements[S.currentArr]; if (!arr) return; - const isBass = /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); // Compute the count directly from the active arrangement rather // than going through `lanes()` — the latter consults a per-draw // cache and our intent here is explicitly "what is this @@ -279,7 +281,7 @@ export const editorAddString = (pos) => { export const editorRemoveString = (pos) => { const arr = S.arrangements[S.currentArr]; if (!arr) return; - const isBass = /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); // Same reasoning as editorAddString — anchor on `arr` directly // rather than the cached `lanes()`. const cur = _stringCountFor(arr); @@ -311,7 +313,7 @@ export function editorStringButtonsRefresh() { const box = document.getElementById('editor-string-btns'); if (!box) return; const arr = S.arrangements[S.currentArr]; - const show = !!arr && _stringButtonsVisiblePure(arr.name, { + const show = !!arr && _stringButtonsVisiblePure(arrKind(arr), { keysMode: isKeysMode(), drumEdit: !!S.drumEditMode, tempoMap: !!S.tempoMapMode, partsView: !!S.partsViewMode, tabView: !!S.tabViewMode, }); @@ -319,7 +321,7 @@ export function editorStringButtonsRefresh() { if (_stringBtnsKey !== 'hidden') { box.classList.add('hidden'); _stringBtnsKey = 'hidden'; } return; } - const isBass = /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); // _stringCountFor walks every note, so memo it on the edit generation // (the repo's standard dirty key — in-place moves keep array identity) // rather than paying O(notes) on every rAF flush. @@ -354,14 +356,14 @@ export function editorStringButtonsRefresh() { export const editorCanvasStringAdd = () => { const arr = S.arrangements[S.currentArr]; if (!arr) return; - const pos = _addPositionPure(/bass/i.test(arr.name || ''), _stringCountFor(arr)); + const pos = _addPositionPure(_isBassArr(arr), _stringCountFor(arr)); if (pos) editorAddString(pos); }; export const editorCanvasStringRemove = () => { const arr = S.arrangements[S.currentArr]; if (!arr) return; - const isBass = /bass/i.test(arr.name || ''); + const isBass = _isBassArr(arr); const cur = _stringCountFor(arr); const pos = _removePositionPure(isBass, cur); if (!pos) return; diff --git a/tests/canvas_string_buttons.test.mjs b/tests/canvas_string_buttons.test.mjs index fba5e73b..185e2565 100644 --- a/tests/canvas_string_buttons.test.mjs +++ b/tests/canvas_string_buttons.test.mjs @@ -126,14 +126,14 @@ t('remove-with-notes undo and redo stay bound to their original arrangement', () assert.deepStrictEqual(snap(other), otherBefore, 'redo leaves the selected arrangement untouched'); }); -t('visibility: fretted String view only', () => { +t('visibility: fretted parts only (now takes the resolved kind, not a name)', () => { const none = {}; - assert.strictEqual(_stringButtonsVisiblePure('Lead', none), true); - assert.strictEqual(_stringButtonsVisiblePure('Bass', none), true); - assert.strictEqual(_stringButtonsVisiblePure('Keys', none), false); - assert.strictEqual(_stringButtonsVisiblePure('Drums', none), false); + assert.strictEqual(_stringButtonsVisiblePure('guitar', none), true); + assert.strictEqual(_stringButtonsVisiblePure('bass', none), true); + assert.strictEqual(_stringButtonsVisiblePure('keys', none), false); + assert.strictEqual(_stringButtonsVisiblePure('drums', none), false); for (const flag of ['keysMode', 'drumEdit', 'tempoMap', 'partsView', 'tabView']) { - assert.strictEqual(_stringButtonsVisiblePure('Lead', { [flag]: true }), false, flag); + assert.strictEqual(_stringButtonsVisiblePure('guitar', { [flag]: true }), false, flag); } }); diff --git a/tests/instrument_type.test.mjs b/tests/instrument_type.test.mjs index 3b1a2fb9..ab79711e 100644 --- a/tests/instrument_type.test.mjs +++ b/tests/instrument_type.test.mjs @@ -20,7 +20,8 @@ import assert from 'node:assert'; import { _typeKind, _arrTypeKind, _arrKindFromName, arrKind } from '../src/instrument.js'; import { isKeysArr, KEYS_PATTERN, _rollPitchCtxFor } from '../src/keys.js'; -import { _seedExtendedStringsFromTuning } from '../src/lanes.js'; +import { _seedExtendedStringsFromTuning, _stringCountFor } from '../src/lanes.js'; +import { _isBassArr } from '../src/instrument.js'; import { _trackKindBadgePure } from '../src/track-session.js'; import { seedState } from './_history_env.mjs'; @@ -137,6 +138,23 @@ t('_trackKindBadgePure: audio shows the layer, transcription shows the instrumen assert.deepStrictEqual(_trackKindBadgePure({ type: 'transcription', targetId: 'Choir', mixKey: 'arr:3' }, arrs), ['VOX', 'Vocals']); }); +// ── _isBassArr keeps the INDEPENDENT /bass/ fallback (not arrKind===bass) ── +t('_isBassArr: type wins; untyped keeps the independent /bass/ test ("Synth Bass" stays bass)', () => { + assert.strictEqual(_isBassArr({ name: 'Rhythm', type: 'bass' }), true, 'typed bass, non-bass name'); + assert.strictEqual(_isBassArr({ name: 'Sub Bass', type: 'guitar' }), false, 'typed guitar wins'); + assert.strictEqual(_isBassArr({ name: 'Synth Bass' }), true, 'untyped "Synth Bass" is bass (arrKind would say keys)'); + assert.strictEqual(_isBassArr({ name: 'Lead' }), false); +}); + +// ── string count (the rename-safety-critical reader) honors type ────── +t('_stringCountFor: type drives the 4-vs-6 baseline; untyped independent /bass/ fallback', () => { + const mk = (o) => ({ tuning: [], notes: [], chords: [], chord_templates: [], ...o }); + assert.strictEqual(_stringCountFor(mk({ name: 'Rhythm', type: 'bass', tuning: [0, 0, 0, 0] })), 4, 'typed bass → 4'); + assert.strictEqual(_stringCountFor(mk({ name: 'Sub Bass', type: 'guitar', tuning: [0, 0, 0, 0, 0, 0] })), 6, 'typed guitar → 6'); + assert.strictEqual(_stringCountFor(mk({ name: 'Synth Bass', tuning: [0, 0, 0, 0] })), 4, 'untyped "Synth Bass" → 4 (unchanged)'); + assert.strictEqual(_stringCountFor(mk({ name: 'Lead', tuning: [0, 0, 0, 0, 0, 0] })), 6); +}); + // ── view routing honors type (a converted reader) ──────────────────── t('_rollPitchCtxFor: a typed-keys part has no fretted context regardless of name', () => { assert.strictEqual(_rollPitchCtxFor({ name: 'Lead', type: 'keys' }), null, 'typed keys → null (no fretted ctx)'); diff --git a/tests/strings_modal.test.mjs b/tests/strings_modal.test.mjs index fdbc175a..f0f099be 100644 --- a/tests/strings_modal.test.mjs +++ b/tests/strings_modal.test.mjs @@ -21,6 +21,7 @@ import { setHostHooks } from '../src/host.js'; import { seedState, trackHooks } from './_history_env.mjs'; import fs from 'node:fs'; import { _stringCountFor } from '../src/lanes.js'; +import { _isBassArr } from '../src/instrument.js'; const src = fs.readFileSync(new URL('../src/strings.js', import.meta.url), 'utf8'); @@ -80,7 +81,7 @@ function makeHandlerEnv(seed) { // editorAddString / editorRemoveString reach draw / updateStatus through host // now (they moved to src/strings.js), so inject a host stub for those two. const env = new Function( - 'window', 'S', 'host', '_renderStringsModal', '_stringCountFor', + 'window', 'S', 'host', '_renderStringsModal', '_stringCountFor', '_isBassArr', 'AddStringCmd', 'RemoveStringCmd', '"use strict";' + tuningBlock + '\n' + notesOnStringSrc + '\n' @@ -93,6 +94,7 @@ function makeHandlerEnv(seed) { { draw: () => {}, updateStatus: () => {} }, // host () => {}, // _renderStringsModal (DOM-free) _stringCountFor, // the REAL one, imported from src/lanes.js + _isBassArr, // type-authoritative bass predicate (src/instrument.js) AddStringCmd, RemoveStringCmd, ); S.history = new EditHistory(); From 9d2d8f6f7c2e72fc5d434978327deff52d2b9946 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Mon, 20 Jul 2026 00:36:21 -0500 Subject: [PATCH 06/18] =?UTF-8?q?feat(editor):=20typed=20tracks=20rename?= =?UTF-8?q?=20freely=20=E2=80=94=20the=20last=20identity=20readers=20honor?= =?UTF-8?q?=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The capstone of the instrument-type-as-data arc: convert the remaining NON-stranding name readers to the resolved kind, then RELAX the rename guard so a typed track's name becomes a pure display label. Every reader now consults an authored `type` (via arrKind), so identity no longer rides the name anywhere. Converted (each takes the resolved kind; caller passes arrKind(arr)): - gm-guide `_gmKindPure` — the guide-instrument VOICE (keys/bass/guitar; drums & vocals fall to the guitar voice). Callers: audio.js x2, menu-bar.js. - tab-preview `_tabPreviewGuardPure` + gp5-export `_gp5ExportGuardPure` — the fretted-only guards; keys/drums are refused by identity, not a name regex. - parts-view `_partsArrKindPure` — the per-lane silhouette tag. KEYS_PATTERN's now-unused import dropped from gm-guide.js. Rename guard: `_renameGuardPure` gains a `typed` param — when the arrangement carries an authored `type` the kind-change refusal is SKIPPED (a rename can't re-lane a part whose identity is data), while empty/too-long/duplicate/no-op still hold. Untyped/legacy packs keep the full name-inference guard, so they're still protected from silently re-laning notes onto invisible strings. Threaded through arrangement.js (`!!_arrTypeKind(arr)`) and track-session.js's `_trackTranscriptionRenameGuardPure`. Payoff: a guitar authored `type:"guitar"` but named "Piano" now sounds with a guitar voice, previews/exports its tab, silhouettes as guitar, and renames freely — on main all of these keyed off the misleading name. Byte-identical for UNTYPED inputs, so the 296-suite can't catch a bad conversion — added a TYPED test per reader: gm_guide / gp5_export (incl. the "guitar-typed Piano-named exports" payoff through the real orchestrator) / rename_part (typed renames across kinds; structural checks still hold) / parts_view (kind→tag), plus a consolidated "capstone readers honor type via arrKind" block in instrument_type.test.mjs (exported the tab-preview & parts pures to compose them there). The sliced-source tests now pass kinds; tab_preview_race injects arrKind. JS 296/0, lint 0 err / 3 baseline. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- CHANGELOG.md | 22 +++++++---- src/arrangement.js | 69 ++++++++++++++++++++-------------- src/audio.js | 4 +- src/gm-guide.js | 22 ++++++----- src/gp5-export.js | 17 +++++---- src/menu-bar.js | 3 +- src/parts-view.js | 24 ++++++------ src/tab-preview.js | 20 +++++----- src/track-session.js | 8 ++-- tests/gm_guide.test.mjs | 34 ++++++++++++----- tests/gp5_export.test.mjs | 47 +++++++++++++++++------ tests/instrument_type.test.mjs | 30 +++++++++++++++ tests/parts_view.test.js | 24 ++++++------ tests/rename_part.test.mjs | 24 +++++++++++- tests/tab_preview.test.js | 39 ++++++++++--------- tests/tab_preview_race.test.js | 3 ++ 16 files changed, 251 insertions(+), 139 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d994fa5..4ad3db34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,14 +14,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 places with two subtly disagreeing rules — the reason renaming a track could flip its lane layout, and why the rename dialog has to refuse some names. The pack format already records an authored instrument `type` per arrangement; the editor - now **reads it** (carrying it through load and save) and lets it drive the - keys/bass view and string-count decisions, falling back to the old name inference - only when a track is untyped. Existing songs open exactly as before — but a part - named against its instrument (a guitar called "Grand Piano") finally reads as - what it *is*. **The Tracks list now badges each transcription track with its - instrument** (GTR / BAS / KEY / DRM / VOX) instead of a generic "MIDI", read - from that same authoritative identity. Foundation for first-class drum tracks, - vocals, and freely renaming a track without changing its instrument. + now **reads it** (carrying it through load and save) and lets it drive *every* + identity decision — the keys/bass view, string counts, the guide-instrument + voice, the fretted-only Tab preview and Guitar-Pro export guards, and the + Parts-view silhouettes — falling back to the old name inference only when a + track is untyped. Existing songs open exactly as before — but a part named + against its instrument (a guitar called "Grand Piano") finally reads as, sounds + as, and exports as what it *is*. **The Tracks list now badges each transcription + track with its instrument** (GTR / BAS / KEY / DRM / VOX) instead of a generic + "MIDI", read from that same authoritative identity. And **renaming a typed track + is now free**: its identity is data, so the name is a pure display label and the + rename dialog no longer refuses a name that merely *looks* like another + instrument (untyped/legacy tracks keep the old kind-change guard, which protects + them from silently re-laning notes). Foundation for first-class drum tracks and + vocals. - **The piano roll stretches, compacts and scrolls.** Its lane height used to be derived and untouchable — the whole pitch range packed into about 350px — so a diff --git a/src/arrangement.js b/src/arrangement.js index 95cf47cd..b4e92176 100644 --- a/src/arrangement.js +++ b/src/arrangement.js @@ -11,6 +11,7 @@ import { S, markSessionDirty } from './state.js'; import { _editorEscHtml, _editorPromptText, setStatus } from './ui.js'; import { flattenChords } from './chords.js'; import { KEYS_PATTERN } from './keys.js'; +import { _arrTypeKind } from './instrument.js'; import { _recState } from './midi-record.js'; import { _maybeOfferMidiTempoMap, _showDrumImportUnmappedModal } from './import.js'; import { host } from './host.js'; @@ -22,12 +23,16 @@ import { host } from './host.js'; // ── Part rename (EDITOR-VIEW-MODALITY / DAW-workspace 2.2b) ────────── // Unblocked by the manifest `type` stamping + merge-not-rebuild save // (#101): a rename no longer strips the entry's `type`/unknown keys, and -// sloppak sessions carry a stable `id` the view prefs key off. One hard -// limit stays, enforced honestly: the NAME still drives kind inference -// (keys → piano roll + notation sidecar, /bass/i → 4-lane layout, -// /^drums/i → drum routing), so a rename that would CHANGE the inferred -// kind is refused — silently re-laning a 6-string chart as a 4-string -// bass would strand notes on invisible strings. +// sloppak sessions carry a stable `id` the view prefs key off. +// +// For a TYPED part the rename is FREE: its instrument identity is DATA (the +// authored `type`, which every identity reader now honors), so the name is a +// pure display label and can change to anything. The kind-change refusal only +// applies to UNTYPED / legacy packs, where the NAME still drives kind inference +// (keys → piano roll + notation sidecar, /bass/i → 4-lane layout, /^drums/i → +// drum routing) — there, a rename that would CHANGE the inferred kind is +// refused, since silently re-laning a 6-string chart as a 4-string bass would +// strand notes on invisible strings. /* @pure:rename-arr:start */ // A part name feeds TWO independent interpreters, and a rename must not shift @@ -72,11 +77,13 @@ function _arrKindLabelPure(name) { if (/bass/i.test(n)) return 'bass'; return 'guitar'; } -// Validate a rename: trimmed non-empty, bounded, unique among the OTHER -// parts (case-insensitive — the save-side name discipline), and never a -// kind change under EITHER interpreter. Returns {ok, reason, name} with the -// trimmed name. -function _renameGuardPure(oldName, rawNewName, otherNames) { +// Validate a rename: trimmed non-empty, bounded, and unique among the OTHER +// parts (case-insensitive — the save-side name discipline). For an UNTYPED +// part it is additionally never a kind change under EITHER interpreter; a +// TYPED part (`typed` true) skips that check — its identity is the authored +// `type`, not the name, so a rename can't re-lane it. Returns {ok, reason, +// name} with the trimmed name. +function _renameGuardPure(oldName, rawNewName, otherNames, typed) { const name = String(rawNewName || '').trim(); if (!name) return { ok: false, reason: 'Name can’t be empty.', name }; if (name.length > 60) return { ok: false, reason: 'Name too long (max 60 characters).', name }; @@ -85,23 +92,27 @@ function _renameGuardPure(oldName, rawNewName, otherNames) { if (taken.has(name.toLowerCase())) { return { ok: false, reason: `Another track is already named “${name}”.`, name }; } - const runtimeMoved = _arrKindPure(oldName) !== _arrKindPure(name); - const saveMoved = _arrSaveKindPure(oldName) !== _arrSaveKindPure(name); - if (runtimeMoved || saveMoved) { - const oldLabel = _arrKindLabelPure(oldName); - const newLabel = _arrKindLabelPure(name); - const reason = (oldLabel !== newLabel) - // A clean instrument change (e.g. guitar → bass, guitar → keys). - ? `That name would change the track’s instrument (${oldLabel} → ${newLabel}) — ` - + 'lane layout and notation still key off the name. Add a new track instead.' - // Same label, but the two interpreters disagree on this exact name - // (e.g. "Piano" → "Electric Piano": the editor keys off the first - // word and would drop to guitar lanes, while the save still writes - // keys). Re-laning either way strands notes, so refuse. - : 'That name is read differently by the editor and the saved file, so it would ' - + 'change the track’s layout. The editor keys off the FIRST word, the save off ' - + 'any keys word — pick a name both agree on.'; - return { ok: false, reason, name }; + // Identity is DATA for a typed part — the name is a free display label, so + // the kind-change refusal below (a NAME-inference guard) does not apply. + if (!typed) { + const runtimeMoved = _arrKindPure(oldName) !== _arrKindPure(name); + const saveMoved = _arrSaveKindPure(oldName) !== _arrSaveKindPure(name); + if (runtimeMoved || saveMoved) { + const oldLabel = _arrKindLabelPure(oldName); + const newLabel = _arrKindLabelPure(name); + const reason = (oldLabel !== newLabel) + // A clean instrument change (e.g. guitar → bass, guitar → keys). + ? `That name would change the track’s instrument (${oldLabel} → ${newLabel}) — ` + + 'lane layout and notation still key off the name. Add a new track instead.' + // Same label, but the two interpreters disagree on this exact name + // (e.g. "Piano" → "Electric Piano": the editor keys off the first + // word and would drop to guitar lanes, while the save still writes + // keys). Re-laning either way strands notes, so refuse. + : 'That name is read differently by the editor and the saved file, so it would ' + + 'change the track’s layout. The editor keys off the FIRST word, the save off ' + + 'any keys word — pick a name both agree on.'; + return { ok: false, reason, name }; + } } return { ok: true, reason: '', name }; } @@ -144,7 +155,7 @@ export async function editorRenameArrangement() { const others = S.arrangements .filter((_, i) => i !== S.currentArr) .map(a => a && a.name); - const guard = _renameGuardPure(arr.name, val, others); + const guard = _renameGuardPure(arr.name, val, others, !!_arrTypeKind(arr)); if (!guard.ok) { if (guard.reason) setStatus(guard.reason); return; diff --git a/src/audio.js b/src/audio.js index c21ef795..185f8d6a 100644 --- a/src/audio.js +++ b/src/audio.js @@ -1676,7 +1676,7 @@ function _guideGmProgram() { if (S.drumEditMode || !S.arrangements.length) return null; const arr = S.arrangements[S.currentArr]; if (!arr) return null; - return editorGmVoiceFor(_gmKindPure(arr.name)); + return editorGmVoiceFor(_gmKindPure(arrKind(arr))); } // Pitched events for the current arrangement: the same charted times the @@ -2350,7 +2350,7 @@ function _guideTick() { _drumKitVoicesInWindow(from, to, target, 1); continue; } - const gm = editorGmVoiceFor(_gmKindPure(arr && arr.name)); + const gm = editorGmVoiceFor(_gmKindPure(arrKind(arr))); const ready = gm !== null && gmPresetReady(gm); if (gm !== null && !ready) ensureGmPreset(gm, S.audioCtx); // clap while it loads const groups = _gmEventsInWindowPure(_bandPartPitchedEvents(part.idx), from, to, 4); diff --git a/src/gm-guide.js b/src/gm-guide.js index 4f1bd92f..539104d5 100644 --- a/src/gm-guide.js +++ b/src/gm-guide.js @@ -24,8 +24,9 @@ * * Voice choice (1.5) is per part KIND — keys / bass / guitar (drums keep * their clap; the drum strip owns drum sounds) — stored as editor prefs - * (`editorGmVoice:`), never the pack. Kind inference mirrors the - * repo rule: KEYS_PATTERN (start-anchored) > /bass/i > guitar. + * (`editorGmVoice:`), never the pack. The part's instrument kind is + * resolved by the caller (arrKind — an authored `type` wins over the name) + * and collapsed here to the three voice families. * * This module is deliberately a leaf over state/keys: the audio scheduler * (src/audio.js) passes in the AudioContext and the guide bus, so no @@ -33,7 +34,6 @@ * node (that's how the unit tests run). */ -import { KEYS_PATTERN } from './keys.js'; import { setStatus } from './ui.js'; /* @pure:gm-guide:start */ @@ -69,13 +69,15 @@ export const GM_VOICE_CHOICES = Object.freeze({ ]), }); -// Part kind for guide-voice purposes. Mirrors the repo's kind-inference -// rule (keys > drums > bass > guitar) minus drums: the drum grid is a -// separate edit mode (S.drumEditMode) that never reaches this path. -export function _gmKindPure(arrName) { - const name = typeof arrName === 'string' ? arrName : ''; - if (KEYS_PATTERN.test(name)) return 'keys'; - if (/bass/i.test(name)) return 'bass'; +// The guide-voice family for a resolved instrument kind (arrKind — an +// authored `type` wins over the name). The GM voices come in three families, +// keys / bass / guitar; drums and vocals fall to the guitar voice — a drums +// arrangement is edited through the drum grid (S.drumEditMode) and never +// reaches this pitched path. Callers pass arrKind(arr), so a keys part named +// like a guitar takes the keys voice (identity is DATA, not the name). +export function _gmKindPure(kind) { + if (kind === 'keys') return 'keys'; + if (kind === 'bass') return 'bass'; return 'guitar'; } diff --git a/src/gp5-export.js b/src/gp5-export.js index da17357f..72a44272 100644 --- a/src/gp5-export.js +++ b/src/gp5-export.js @@ -15,18 +15,19 @@ import { S } from './state.js'; import { setStatus } from './ui.js'; import { guardSessionTransition } from './session-lifecycle.js'; import { _tabPreviewUrlPure } from './tab-preview.js'; +import { arrKind } from './instrument.js'; /* @pure:gp5-export:start */ // Which parts can export, with the exact user-facing reason when one can't. // Mirrors the Tab preview guard: fretted-only (keys/drums pack as pitch, not // string·fret, so a GP conversion would engrave nonsense), and a SAVED pack is -// required because the converter reads the last-saved pack. Regexes inlined so -// this @pure block stays self-contained and slice-testable (the tab-preview -// block inlines the same fretted test for the same reason). -function _gp5ExportGuardPure(filename, arrName, hasArrangements) { +// required because the converter reads the last-saved pack. The caller passes +// the RESOLVED instrument kind (arrKind — an authored `type` wins over the +// name), so keys/drums are refused by identity, not by a name guess; taking a +// kind keeps this @pure block self-contained and slice-testable. +function _gp5ExportGuardPure(filename, kind, hasArrangements) { if (!hasArrangements) return { ok: false, reason: 'Load a song first.' }; - const nm = String(arrName || ''); - if (/^(keys|piano|keyboard|synth)/i.test(nm) || /^drums/i.test(nm)) { + if (kind === 'keys' || kind === 'drums') { return { ok: false, reason: 'Guitar Pro export is for fretted tracks — keys and drums tracks have no tab.' }; } if (!filename) { @@ -79,7 +80,7 @@ function _downloadBytes(bytes, name) { export async function editorExportGp5() { const cur = () => (S.arrangements.length ? S.arrangements[S.currentArr] : null); let arr = cur(); - let guard = _gp5ExportGuardPure(S.filename, arr && arr.name, !!S.arrangements.length); + let guard = _gp5ExportGuardPure(S.filename, arrKind(arr), !!S.arrangements.length); if (!guard.ok) { setStatus(guard.reason); return; } // The converter reads the SAVED pack and indexes it by S.currentArr — and // it CLAMPS that index into the saved track list. So an unsaved session @@ -92,7 +93,7 @@ export async function editorExportGp5() { } // The prompt awaited: the song (and the current part) may have moved. arr = cur(); - guard = _gp5ExportGuardPure(S.filename, arr && arr.name, !!S.arrangements.length); + guard = _gp5ExportGuardPure(S.filename, arrKind(arr), !!S.arrangements.length); if (!guard.ok) { setStatus(guard.reason); return; } const name = _gp5ExportNamePure(S.filename, arr && arr.name); setStatus('Exporting ' + name + '…'); diff --git a/src/menu-bar.js b/src/menu-bar.js index fa8d343c..7fe2da43 100644 --- a/src/menu-bar.js +++ b/src/menu-bar.js @@ -35,6 +35,7 @@ import { _editorSetGuideVoiceMode, editorFollowEnabled, editorGuideVoiceMode, editorScrollInPlayEnabled } from './audio.js'; import { GM_VOICE_CHOICES, _gmKindPure, editorGmVoiceFor, editorSetGmVoice } from './gm-guide.js'; +import { arrKind } from './instrument.js'; import { _editorRunEofCommand } from './input.js'; import { _editorShortcutRowsPure, editorShortcutProfile } from './shortcuts.js'; import { S } from './state.js'; @@ -451,7 +452,7 @@ function windowFns() { // model to render no instrument rows. function _gmGuideMenuCtx() { const arr = (S.arrangements && S.arrangements[S.currentArr]) || null; - const kind = (!arr || S.drumEditMode) ? null : _gmKindPure(arr.name); + const kind = (!arr || S.drumEditMode) ? null : _gmKindPure(arrKind(arr)); return { mode: editorGuideVoiceMode(), kind, diff --git a/src/parts-view.js b/src/parts-view.js index 525521e3..f7095f13 100644 --- a/src/parts-view.js +++ b/src/parts-view.js @@ -58,15 +58,14 @@ export function _partsLaneAtYPure(y, waveformH, laneH, count) { const i = Math.floor((y - waveformH) / laneH); return i >= 0 && i < count ? i : -1; } -// Instrument tag for a fretted/keyed arrangement, inferred from its NAME -// alone so every Parts-view lane reflects its OWN part rather than the armed -// arrangement. Self-contained (regexes inlined) so it stays unit-testable -// inside this @pure block. Mirrors KEYS_PATTERN (/^(keys|piano|keyboard| -// synth)/i) and isBassArr's /bass/i test, keys taking precedence. -function _partsArrKindPure(name) { - const s = String(name || ''); - if (/^(keys|piano|keyboard|synth)/i.test(s)) return 'Keys'; - if (/bass/i.test(s)) return 'Bass'; +// Instrument tag for a fretted/keyed arrangement lane, from its RESOLVED +// instrument kind (the caller passes arrKind — an authored `type` wins over +// the name), so every Parts-view lane reflects its OWN part's identity rather +// than the armed arrangement OR a misleading name. Self-contained (a plain +// kind→tag map) so it stays unit-testable inside this @pure block. +export function _partsArrKindPure(kind) { + if (kind === 'keys') return 'Keys'; + if (kind === 'bass') return 'Bass'; return 'Guitar'; } // Unified-row hit test over the EXACT layout the header column shares — @@ -169,9 +168,10 @@ function _partsDrawSilhouette(part, y0, laneH, w) { // Fretted: string-ribbon rows, low strings at the bottom to match the // focus editor's orientation. const strings = Math.max(1, _stringCountFor(arr)); - // Per-lane bass detection: isBassArr(arr) ignores its arg and tests the - // armed part, which would paint every lane the armed part's colour. - ctx.fillStyle = _partsArrKindPure(arr.name) === 'Bass' ? 'rgba(255,170,90,0.8)' : 'rgba(150,220,150,0.8)'; + // Per-lane bass detection via the resolved kind (keys already returned + // above): an authored `type` wins over the name, so a bass lane paints + // orange regardless of what part is armed or how it's named. + ctx.fillStyle = _partsArrKindPure(arrKind(arr)) === 'Bass' ? 'rgba(255,170,90,0.8)' : 'rgba(150,220,150,0.8)'; for (const n of events) { const x = timeToX(n.time); if (x < PARTS_GUTTER || x > w) continue; diff --git a/src/tab-preview.js b/src/tab-preview.js index 52008081..d28ac108 100644 --- a/src/tab-preview.js +++ b/src/tab-preview.js @@ -7,20 +7,20 @@ // exported explicitly. import { S } from './state.js'; +import { arrKind } from './instrument.js'; /* @pure:tab-preview:start */ // Guard: which parts can preview, with the exact user-facing reason when // one can't. NON-FRETTED parts (keys AND drums) are excluded — their wire // packing isn't fret/string, so a GP conversion of it would engrave -// nonsense tab. The non-fretted test mirrors the editor-wide one -// (KEYS_PATTERN /^(keys|piano|keyboard|synth)/i plus /^drums/i, e.g. the -// Strings modal's gate) but is INLINED so this @pure block stays -// self-contained and extractable — no reference to the outer KEYS_PATTERN -// global, matching the parts-view block's "regexes inlined" convention. -function _tabPreviewGuardPure(filename, arrName, hasArrangements) { +// nonsense tab. The caller passes the RESOLVED instrument kind (arrKind — an +// authored `type` wins over the name), so a keys part named like a guitar is +// still refused, and a guitar part named "Piano" now correctly previews. +// keys/drums are the two non-fretted runtime kinds; taking a kind (not a +// name) also keeps this @pure block self-contained — no regex, no outer ref. +function _tabPreviewGuardPure(filename, kind, hasArrangements) { if (!hasArrangements) return { ok: false, reason: 'Load a song first.' }; - const nm = String(arrName || ''); - if (/^(keys|piano|keyboard|synth)/i.test(nm) || /^drums/i.test(nm)) { + if (kind === 'keys' || kind === 'drums') { return { ok: false, reason: 'Tab preview is for fretted tracks — keys and drums tracks have no tab.' }; } if (!filename) { @@ -61,7 +61,7 @@ function _tabPreviewHttpMessagePure(status, bodyText) { // _tabPreviewUrlPure is the tabview-plugin GP5 conversion endpoint — the shared // contract. File ▸ Export ▸ Guitar Pro (src/gp5-export.js) downloads the bytes // from the same URL, so the endpoint format lives here only, never duplicated. -export { _tabPreviewKeyPolicyPure, _tabPreviewUrlPure }; +export { _tabPreviewGuardPure, _tabPreviewKeyPolicyPure, _tabPreviewUrlPure }; // Same pinned version + memoized loader idiom as the Tab View plugin — // pinning insulates the preview from CDN latest-tag churn (V12: alphaTab @@ -113,7 +113,7 @@ async function _tabPreviewRender() { if (!mount) return; const arr = S.arrangements.length ? S.arrangements[S.currentArr] : null; const guard = _tabPreviewGuardPure( - S.filename, arr && arr.name, !!S.arrangements.length); + S.filename, arrKind(arr), !!S.arrangements.length); if (!guard.ok) { _tabPreviewDestroyApi(); _tabPreviewStatus(guard.reason); diff --git a/src/track-session.js b/src/track-session.js index 3eefeac1..ad03453d 100644 --- a/src/track-session.js +++ b/src/track-session.js @@ -33,7 +33,7 @@ import { host } from './host.js'; import { _renameGuardPure } from './arrangement.js'; import { _partViewKeyPure } from './keys.js'; -import { arrKind } from './instrument.js'; +import { arrKind, _arrTypeKind } from './instrument.js'; import { _mixerPanelRefresh, _mixerPartStatePure, mixerSetPart, mixerTogglePart } from './mixer-panel.js'; import { S, markSessionDirty } from './state.js'; import { _editorEscHtml, setStatus } from './ui.js'; @@ -451,8 +451,8 @@ export function _trackFocusSourcePure(row) { return row.pairedSourceId || MASTER_ID; } -export function _trackTranscriptionRenameGuardPure(oldName, requested, otherNames) { - return _renameGuardPure(oldName, requested, otherNames); +export function _trackTranscriptionRenameGuardPure(oldName, requested, otherNames, typed) { + return _renameGuardPure(oldName, requested, otherNames, typed); } // True when the tree carries nothing the canonical song doesn't already @@ -772,7 +772,7 @@ function applyTrackRename(trackId, requested) { const otherNames = S.arrangements .filter((_, i) => i !== index).map(arr => arr && arr.name); const guard = _trackTranscriptionRenameGuardPure( - S.arrangements[index].name, requested, otherNames); + S.arrangements[index].name, requested, otherNames, !!_arrTypeKind(S.arrangements[index])); if (!guard.ok) { if (guard.reason) setStatus(guard.reason); return false; } clean = guard.name; } diff --git a/tests/gm_guide.test.mjs b/tests/gm_guide.test.mjs index e7a07e5c..a788fffa 100644 --- a/tests/gm_guide.test.mjs +++ b/tests/gm_guide.test.mjs @@ -42,6 +42,7 @@ const { } = await import('../src/gm-guide.js'); const { EDITOR_MENUS, _menuModelPure } = await import('../src/menu-bar.js'); const { _editorShortcutRowsPure } = await import('../src/shortcuts.js'); +const { arrKind } = await import('../src/instrument.js'); let pass = 0, fail = 0; function t(name, fn) { @@ -102,18 +103,33 @@ t('URL builder: org without an http(s) base yields null (chain moves on), junk s // ── Kind inference + per-kind voices ────────────────────────────────── -t('kind inference mirrors the repo rule: KEYS_PATTERN start-anchored > /bass/i > guitar', () => { - assert.strictEqual(_gmKindPure('Keys'), 'keys'); - assert.strictEqual(_gmKindPure('Piano 2'), 'keys'); - assert.strictEqual(_gmKindPure('Synth Lead'), 'keys'); - assert.strictEqual(_gmKindPure('Electric Piano'), 'guitar', - 'start-anchored: "Electric Piano" is NOT a keys name (the pinned trap)'); - assert.strictEqual(_gmKindPure('Bass'), 'bass'); - assert.strictEqual(_gmKindPure('Lead Guitar'), 'guitar'); - assert.strictEqual(_gmKindPure(''), 'guitar'); +t('voice family collapses the resolved kind to keys / bass / guitar', () => { + // _gmKindPure now takes the RESOLVED instrument kind (arrKind), not a name. + // The three GM voice families are keys / bass / guitar; drums and vocals + // (and anything unrecognized) fall to the guitar voice. + assert.strictEqual(_gmKindPure('keys'), 'keys'); + assert.strictEqual(_gmKindPure('bass'), 'bass'); + assert.strictEqual(_gmKindPure('guitar'), 'guitar'); + assert.strictEqual(_gmKindPure('drums'), 'guitar', 'drums never reach the pitched voice'); + assert.strictEqual(_gmKindPure('vocals'), 'guitar'); assert.strictEqual(_gmKindPure(null), 'guitar'); }); +t('an authored `type` drives the guide voice — a mis-NAMED part voices by what it IS', () => { + // The rename-safety payoff: identity is DATA. arrKind resolves the type + // ahead of the name, so a bass-typed part named "Lead Guitar" takes the + // BASS voice, and a keys-typed part named "Gtr" takes the KEYS voice — + // on main (name-only) both would have voiced as guitar. + assert.strictEqual(_gmKindPure(arrKind({ type: 'bass', name: 'Lead Guitar' })), 'bass'); + assert.strictEqual(_gmKindPure(arrKind({ type: 'piano', name: 'Gtr' })), 'keys'); + // Untyped still reads the name (byte-identical to the old behavior): the + // pinned start-anchored trap — "Electric Piano" is guitar, "Synth …" keys. + assert.strictEqual(_gmKindPure(arrKind({ name: 'Electric Piano' })), 'guitar', + 'untyped: start-anchored name inference is unchanged'); + assert.strictEqual(_gmKindPure(arrKind({ name: 'Synth Lead' })), 'keys'); + assert.strictEqual(_gmKindPure(arrKind({ name: 'Bass' })), 'bass'); +}); + t('voice-for-kind: valid pref wins, garbage falls to the kind default, unknown kind is null', () => { assert.strictEqual(_gmVoiceForKindPure('34', 'bass'), 34); assert.strictEqual(_gmVoiceForKindPure(0, 'keys'), 0, 'program 0 is a real choice'); diff --git a/tests/gp5_export.test.mjs b/tests/gp5_export.test.mjs index 1495d0bd..b0f40693 100644 --- a/tests/gp5_export.test.mjs +++ b/tests/gp5_export.test.mjs @@ -17,6 +17,7 @@ import { _gp5ExportGuardPure, _gp5ExportNamePure, _gp5ExportHttpMessagePure, } from '../src/gp5-export.js'; import { _tabPreviewUrlPure } from '../src/tab-preview.js'; +import { arrKind } from '../src/instrument.js'; let pass = 0, fail = 0; function t(name, fn) { @@ -28,32 +29,30 @@ async function ta(name, fn) { catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } } -// ── 1. guard truth table ───────────────────────────────────────────────────── +// ── 1. guard truth table (the guard now takes the RESOLVED kind, not a name) ── t('guard: no arrangements → load a song first', () => { - const g = _gp5ExportGuardPure('song.feedpak', 'Lead', false); + const g = _gp5ExportGuardPure('song.feedpak', 'guitar', false); assert.strictEqual(g.ok, false); assert.match(g.reason, /Load a song first/); }); -t('guard: keys / piano / synth / drums parts are refused (no tab)', () => { - for (const nm of ['Keys', 'Piano', 'Keyboard 2', 'Synth Lead', 'Drums', 'Drums (kit)']) { - const g = _gp5ExportGuardPure('song.feedpak', nm, true); - assert.strictEqual(g.ok, false, nm + ' should be refused'); +t('guard: keys / drums kinds are refused (no tab)', () => { + for (const kind of ['keys', 'drums']) { + const g = _gp5ExportGuardPure('song.feedpak', kind, true); + assert.strictEqual(g.ok, false, kind + ' should be refused'); assert.match(g.reason, /fretted tracks/); } }); t('guard: fretted part but no saved filename → save first', () => { - const g = _gp5ExportGuardPure('', 'Lead', true); + const g = _gp5ExportGuardPure('', 'guitar', true); assert.strictEqual(g.ok, false); assert.match(g.reason, /Save the song first/); }); -t('guard: a fretted, saved part is OK', () => { - const g = _gp5ExportGuardPure('song.feedpak', 'Lead', true); - assert.deepStrictEqual(g, { ok: true, reason: '' }); - // a name that merely CONTAINS "keys" mid-string is still fretted (start-anchored) - assert.strictEqual(_gp5ExportGuardPure('song.feedpak', 'Rhythm (keys double)', true).ok, true); +t('guard: a fretted, saved part is OK (bass and guitar are the fretted kinds)', () => { + assert.deepStrictEqual(_gp5ExportGuardPure('song.feedpak', 'guitar', true), { ok: true, reason: '' }); + assert.strictEqual(_gp5ExportGuardPure('song.feedpak', 'bass', true).ok, true); }); // ── 2. download filename ───────────────────────────────────────────────────── @@ -123,6 +122,8 @@ function exportEnv(overrides = {}) { S: { arrangements: [{ name: 'Lead' }], currentArr: 0, filename: 'song.feedpak' }, statuses: [], downloads: [], fetched: [], setStatus: (msg) => env.statuses.push(msg), + // The orchestrator resolves the part's kind via arrKind before the guard. + arrKind, _gp5ExportGuardPure, _gp5ExportNamePure, _gp5ExportHttpMessagePure, _tabPreviewUrlPure, _downloadBytes: (_bytes, name) => env.downloads.push(name), fetch: (url) => { @@ -170,5 +171,27 @@ await ta('the song can close during the prompt — guards are re-checked after t assert.match(env.statuses.join(' '), /Load a song first/); }); +// ── 6. authored `type` drives the guard through the real orchestrator ───────── +// Byte-identical for untyped inputs, so these typed cases are what prove the +// conversion actually consults arrKind and not the name (the capstone payoff). +await ta('a keys-TYPED part named like a guitar is refused — identity is data', async () => { + const env = exportEnv({ + S: { arrangements: [{ type: 'piano', name: 'Lead' }], currentArr: 0, filename: 'song.feedpak' }, + }); + await env.run(); + assert.deepStrictEqual(env.fetched, [], 'a keys part exports no tab, whatever its name'); + assert.deepStrictEqual(env.downloads, []); + assert.match(env.statuses.join(' '), /fretted tracks/); +}); + +await ta('a guitar-TYPED part named "Piano" now exports — the free-rename payoff', async () => { + const env = exportEnv({ + S: { arrangements: [{ type: 'guitar', name: 'Piano' }], currentArr: 0, filename: 'song.feedpak' }, + }); + await env.run(); + assert.deepStrictEqual(env.downloads, ['song — Piano.gp5'], 'exports under its display name'); + assert.strictEqual(env.fetched.length, 1, 'the conversion actually ran'); +}); + console.log(`\n${pass} passed, ${fail} failed`); if (fail) process.exit(1); diff --git a/tests/instrument_type.test.mjs b/tests/instrument_type.test.mjs index ab79711e..3c16c4bd 100644 --- a/tests/instrument_type.test.mjs +++ b/tests/instrument_type.test.mjs @@ -23,6 +23,10 @@ import { isKeysArr, KEYS_PATTERN, _rollPitchCtxFor } from '../src/keys.js'; import { _seedExtendedStringsFromTuning, _stringCountFor } from '../src/lanes.js'; import { _isBassArr } from '../src/instrument.js'; import { _trackKindBadgePure } from '../src/track-session.js'; +import { _gmKindPure } from '../src/gm-guide.js'; +import { _tabPreviewGuardPure } from '../src/tab-preview.js'; +import { _gp5ExportGuardPure } from '../src/gp5-export.js'; +import { _partsArrKindPure } from '../src/parts-view.js'; import { seedState } from './_history_env.mjs'; let pass = 0; let fail = 0; @@ -163,6 +167,32 @@ t('_rollPitchCtxFor: a typed-keys part has no fretted context regardless of name assert.notStrictEqual(gtr, null, 'typed guitar named "Piano" → a real fretted ctx (type wins)'); }); +// ── the capstone readers consume arrKind → they honor an authored type ──── +// Each of these was NAME-based and is now fed arrKind's output by its caller. +// The conversion is byte-identical for untyped arrangements, so ONLY a typed, +// contrarily-named arrangement proves the reader actually consults the type. +// Composed here with the real arrKind exactly as each call site does. +t('gm-guide voice: a bass-typed part named "Lead Guitar" takes the bass voice', () => { + assert.strictEqual(_gmKindPure(arrKind({ type: 'bass', name: 'Lead Guitar' })), 'bass'); + assert.strictEqual(_gmKindPure(arrKind({ type: 'piano', name: 'Gtr' })), 'keys'); + assert.strictEqual(_gmKindPure(arrKind({ name: 'Lead' })), 'guitar', 'untyped name fallback'); +}); + +t('tab-preview / gp5-export guards: a keys-typed guitar-named part is refused; a guitar-typed keys-named part is allowed', () => { + const keysTyped = arrKind({ type: 'piano', name: 'Lead' }); // → keys + const gtrTyped = arrKind({ type: 'guitar', name: 'Piano' }); // → guitar + assert.strictEqual(_tabPreviewGuardPure('s.sloppak', keysTyped, true).ok, false, 'keys-typed → no tab preview'); + assert.strictEqual(_tabPreviewGuardPure('s.sloppak', gtrTyped, true).ok, true, 'guitar-typed "Piano" previews (payoff)'); + assert.strictEqual(_gp5ExportGuardPure('s.feedpak', keysTyped, true).ok, false, 'keys-typed → no gp5 export'); + assert.strictEqual(_gp5ExportGuardPure('s.feedpak', gtrTyped, true).ok, true, 'guitar-typed "Piano" exports (payoff)'); +}); + +t('parts-view silhouette tag: a bass-typed part named "Rhythm" tags Bass', () => { + assert.strictEqual(_partsArrKindPure(arrKind({ type: 'bass', name: 'Rhythm' })), 'Bass'); + assert.strictEqual(_partsArrKindPure(arrKind({ type: 'guitar', name: 'Bass Line' })), 'Guitar', 'typed guitar over /bass/ name'); + assert.strictEqual(_partsArrKindPure(arrKind({ name: 'Bass' })), 'Bass', 'untyped name fallback'); +}); + for (const [name, fn] of tests) { try { await fn(); pass++; console.log(' ok ' + name); } catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } diff --git a/tests/parts_view.test.js b/tests/parts_view.test.js index 5001e738..2bf7bfdf 100644 --- a/tests/parts_view.test.js +++ b/tests/parts_view.test.js @@ -83,19 +83,17 @@ t('lane hit-test respects the waveform band and lane bounds', () => { }); // ── per-lane instrument tag ──────────────────────────────────────────────────── -t('kind tag is inferred from each lane\'s OWN name, independent of the armed part', () => { - // Regression: _partsKindTag used to call the param-less isBassArr(), which - // always tested the armed arrangement — so a Bass lane got mistagged - // "Guitar" whenever a guitar part was armed. The pure helper keys off the - // lane's own name only, so a Bass lane tags Bass regardless of what's armed. - assert.strictEqual(_partsArrKindPure('Bass'), 'Bass'); - assert.strictEqual(_partsArrKindPure('Lead Guitar'), 'Guitar'); - assert.strictEqual(_partsArrKindPure('Rhythm'), 'Guitar'); - assert.strictEqual(_partsArrKindPure('Piano'), 'Keys'); - assert.strictEqual(_partsArrKindPure('Synth Lead'), 'Keys'); - // Keys precedence + name-anchored keys pattern (matches KEYS_PATTERN). - assert.strictEqual(_partsArrKindPure('Bass Synth'), 'Bass', 'unanchored keys word does not win'); - assert.strictEqual(_partsArrKindPure(''), 'Guitar', 'empty → Guitar'); +t('tag maps the RESOLVED kind → display label, per lane', () => { + // _partsArrKindPure now takes each lane's RESOLVED instrument kind (the + // caller passes arrKind(arr), so an authored `type` wins over the name and + // the tag is independent of both the armed part AND a misleading name). + // The old regression — the param-less isBassArr() always tested the armed + // arrangement — is prevented at the call site by resolving per lane. + assert.strictEqual(_partsArrKindPure('bass'), 'Bass'); + assert.strictEqual(_partsArrKindPure('guitar'), 'Guitar'); + assert.strictEqual(_partsArrKindPure('keys'), 'Keys'); + // Anything else (drums/vocals/unrecognized) falls to the guitar silhouette. + assert.strictEqual(_partsArrKindPure('vocals'), 'Guitar'); assert.strictEqual(_partsArrKindPure(null), 'Guitar', 'nullish → Guitar'); }); diff --git a/tests/rename_part.test.mjs b/tests/rename_part.test.mjs index c9e6fdba..a36adbc6 100644 --- a/tests/rename_part.test.mjs +++ b/tests/rename_part.test.mjs @@ -107,7 +107,7 @@ t('a name the editor and the save read differently is refused (both facets guard assert.strictEqual(P._renameGuardPure('Rhythm', 'Synthwave Lead', []).ok, false); }); -t('cross-kind renames refuse and say why', () => { +t('cross-kind renames refuse and say why (untyped: name still drives the kind)', () => { const g = P._renameGuardPure('Lead', 'Bass 2', []); assert.strictEqual(g.ok, false); assert.ok(/guitar → bass/.test(g.reason), 'names the kind change'); @@ -115,6 +115,28 @@ t('cross-kind renames refuse and say why', () => { assert.strictEqual(P._renameGuardPure('Lead', 'Keys Solo', []).ok, false); }); +t('a TYPED part renames freely across name-inferred kinds — identity is data', () => { + // The capstone payoff: when the arrangement carries an authored `type`, + // the caller passes typed=true and the kind-change refusal is skipped — + // the name is a pure display label. Every case below is REFUSED untyped + // (see the cross-kind test) and ALLOWED typed. + assert.strictEqual(P._renameGuardPure('Lead', 'Bass 2', [], true).ok, true, 'guitar→bass name ok when typed'); + assert.strictEqual(P._renameGuardPure('Lead', 'Electric Piano', [], true).ok, true, 'save-side keys ok when typed'); + assert.strictEqual(P._renameGuardPure('Piano', 'Electric Piano', [], true).ok, true, 'runtime-facet move ok when typed'); + const g = P._renameGuardPure('Lead', ' Grand Piano ', [], true); + assert.deepStrictEqual(g, { ok: true, reason: '', name: 'Grand Piano' }, 'still trims'); +}); + +t('a TYPED part still obeys the name-agnostic rules (empty / too-long / duplicate / no-op)', () => { + // typed relaxes ONLY the kind-change refusal — the structural checks stay. + assert.strictEqual(P._renameGuardPure('Lead', ' ', [], true).ok, false, 'empty still refused'); + assert.strictEqual(P._renameGuardPure('Lead', 'x'.repeat(61), [], true).ok, false, 'too-long still refused'); + assert.strictEqual(P._renameGuardPure('Lead', 'rhythm', ['Rhythm'], true).ok, false, 'duplicate still refused'); + const noop = P._renameGuardPure('Lead', 'Lead', ['Rhythm'], true); + assert.strictEqual(noop.ok, false); + assert.strictEqual(noop.reason, '', 'a no-op is still a silent no-op'); +}); + t('empty, too-long, duplicate, and no-op inputs are handled', () => { assert.strictEqual(P._renameGuardPure('Lead', ' ', []).ok, false); assert.strictEqual(P._renameGuardPure('Lead', 'x'.repeat(61), []).ok, false); diff --git a/tests/tab_preview.test.js b/tests/tab_preview.test.js index a8c8a2ce..e881c474 100644 --- a/tests/tab_preview.test.js +++ b/tests/tab_preview.test.js @@ -22,9 +22,8 @@ if (!m) { // Extract the @pure block ALONE — no outer globals prepended. This is the // self-containment contract (@pure convention): the block must reference no // global declared outside it, or the extracted sandbox throws -// "X is not defined". Pre-fix this block referenced the outer KEYS_PATTERN, -// so building it in isolation and calling the guard on a keys part threw; -// the guard now inlines its regexes. +// "X is not defined". The guard now takes the RESOLVED instrument kind (the +// caller passes arrKind), so it carries no regex and no outer ref at all. const { _tabPreviewGuardPure, _tabPreviewUrlPure, _tabPreviewHttpMessagePure, _tabPreviewKeyPolicyPure } = new Function( '"use strict";' + m[0] + '\nreturn { _tabPreviewGuardPure, _tabPreviewUrlPure, _tabPreviewHttpMessagePure, _tabPreviewKeyPolicyPure };' @@ -38,43 +37,43 @@ function t(name, fn) { t('guard: fretted saved parts preview; every refusal names its reason', () => { assert.deepStrictEqual( - _tabPreviewGuardPure('song.sloppak', 'Lead', true), { ok: true, reason: '' }); - assert.strictEqual(_tabPreviewGuardPure('s', 'Lead', false).ok, false, 'no arrangements'); - const keys = _tabPreviewGuardPure('s', 'Piano', true); + _tabPreviewGuardPure('song.sloppak', 'guitar', true), { ok: true, reason: '' }); + assert.strictEqual(_tabPreviewGuardPure('s', 'guitar', false).ok, false, 'no arrangements'); + const keys = _tabPreviewGuardPure('s', 'keys', true); assert.strictEqual(keys.ok, false); assert.ok(/fretted/.test(keys.reason), 'keys refusal explains itself'); - const unsaved = _tabPreviewGuardPure('', 'Lead', true); + const unsaved = _tabPreviewGuardPure('', 'guitar', true); assert.strictEqual(unsaved.ok, false); assert.ok(/Save/.test(unsaved.reason), 'unsaved refusal points at Save'); }); t('guard order: an empty session reads as "load a song", not "save first"', () => { - assert.ok(/Load/.test(_tabPreviewGuardPure('', '', false).reason)); + assert.ok(/Load/.test(_tabPreviewGuardPure('', 'guitar', false).reason)); }); t('guard order: an unsaved keys part reads as "fretted only", not "save first" (keys wins)', () => { // Both the keys and the unsaved conditions hold; the non-fretted check // runs first, so the honest reason is the modality one, not Save-first. - const r = _tabPreviewGuardPure('', 'Piano', true); + const r = _tabPreviewGuardPure('', 'keys', true); assert.strictEqual(r.ok, false); assert.ok(/fretted/.test(r.reason) && !/Save/.test(r.reason)); }); -t('guard: drums parts are non-fretted and refused (legacy guitar-encoded drums arrangements)', () => { - const drums = _tabPreviewGuardPure('song.sloppak', 'Drums', true); +t('guard: drums parts are non-fretted and refused', () => { + const drums = _tabPreviewGuardPure('song.sloppak', 'drums', true); assert.strictEqual(drums.ok, false, 'a drums arrangement has no fret/string tab'); assert.ok(/fretted/.test(drums.reason), 'drums refusal explains itself'); - // Case-insensitive, prefix-anchored — matches the editor-wide /^drums/i gate. - assert.strictEqual(_tabPreviewGuardPure('s', 'drums (EOF)', true).ok, false); }); -t('guard is self-contained: extracting the @pure block alone still classifies keys/drums (no outer KEYS_PATTERN ref)', () => { - // These calls execute the inlined regexes inside the isolated block; a - // reference to an outer KEYS_PATTERN would have thrown before we got here. - assert.strictEqual(_tabPreviewGuardPure('s', 'Piano', true).ok, false); - assert.strictEqual(_tabPreviewGuardPure('s', 'Keyboard', true).ok, false); - assert.strictEqual(_tabPreviewGuardPure('s', 'Synth Lead', true).ok, false); - assert.strictEqual(_tabPreviewGuardPure('s.sloppak', 'Rhythm', true).ok, true); +t('guard classifies by RESOLVED kind — bass and guitar are the fretted kinds', () => { + // The guard now consumes arrKind's output (an authored `type` wins over + // the name upstream), so name inference no longer lives here. bass and + // guitar are the fretted kinds; keys/drums are refused; anything else + // (e.g. an unrecognized kind) is treated as fretted, unchanged. + assert.strictEqual(_tabPreviewGuardPure('s.sloppak', 'bass', true).ok, true); + assert.strictEqual(_tabPreviewGuardPure('s.sloppak', 'guitar', true).ok, true); + assert.strictEqual(_tabPreviewGuardPure('s.sloppak', 'keys', true).ok, false); + assert.strictEqual(_tabPreviewGuardPure('s.sloppak', 'drums', true).ok, false); }); t('key policy: preview modal is a read-only lens — only Escape acts (closes), every other key is swallowed', () => { diff --git a/tests/tab_preview_race.test.js b/tests/tab_preview_race.test.js index fdda006a..3722f7b9 100644 --- a/tests/tab_preview_race.test.js +++ b/tests/tab_preview_race.test.js @@ -37,6 +37,9 @@ if (!m) { S: { arrangements: [{ name: 'Lead' }], currentArr: 0, filename: 'song.sloppak' }, alphaTab: {}, document: { getElementById: () => ({ innerHTML: '' }) }, + // _tabPreviewRender resolves the part's kind via arrKind before the + // guard; this race test stubs the guard, so a trivial resolver suffices. + arrKind: () => 'guitar', _tabPreviewGuardPure: () => ({ ok: true, reason: '' }), _tabPreviewUrlPure: () => '/api/plugins/tabview/gp5/song.sloppak?arrangement=0&t=1', _tabPreviewHttpMessagePure: (status) => 'Preview failed (' + status + ')', From e4d0e8393aa9a52a30bd2e074f96ab392764f256 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Mon, 20 Jul 2026 01:46:59 -0500 Subject: [PATCH 07/18] feat(editor): drums become a type:"drums" arrangement (model foundation) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The foundation for multiple drum charts (the drums-as-arrangements arc). The single drum tab has always lived ENTIRELY OUTSIDE S.arrangements[] as a lone off-array singleton edited through a global mode — the one instrument that wasn't an ordinary arrangement. This gives it a home IN the list as a derived `type:"drums"` arrangement (via the #335 instrument-type-as-data seam), the substrate multiple drum charts grow from. BYTE-IDENTICAL: no pack change, no UI change, drum-editor undo untouched. New leaf `src/drum-arrangement.js`: - `syncDrumArrangement(S)` materializes / updates / removes the drums arrangement so its `.drumTab` payload IS S.drumTab — the SAME object reference, so every existing S.drumTab reader/mutator and every drum undo command (which hold references into S.drumTab.hits) keep working unchanged. APPENDS (never inserts), so existing arr indices / arr: keys are stable. Called at every S.drumTab (re)assignment: load migration (file-ops), GP/MIDI import + empty-add (arrangement.js), delete + its undo-restore (DeleteDrumTabCmd). - `isDrumArrangement` keys on the authored `type` ALONE (normalized), NOT arrKind — arrKind would name-infer and wrongly catch a pitched part a user literally named "Drums", then hide/drop it. The materialized arrangement always carries `type:"drums"`, so a type-only test is exact and safe. - `pitchedArrangementCount` / `clampAwayFromDrums` / `pitchedIndexOf` keep the index math correct now that S.arrangements[] can hold a drums entry: the remove-last-arrangement guard counts pitched parts, S.currentArr never lands on the drums arrangement, and the /remove-arrangement backend index is mapped to its pitched-only position (the backend manifest has no drums arrangement). Byte-identical surfaces (the drums arrangement is bridged to the legacy paths; promoting them to arr: + drum-edit-via-selection is the follow-up): - Save (file-ops `_buildSaveBody`, create `editorBuild`): the drums arrangement is EXCLUDED from body.arrangements — drums still persist as the song-level `drum_tab`, so the built pack is byte-identical (and no drums entry reaches arrangements[], where an old core would fretted-grade it as garbage). - Tracks targets (`_trackSessionTargetsPure`), Parts view (`_partsListPure`), band roster (`_bandPartsPure`), pitched switcher (updateArrangementSelector): each skips the drums arrangement so it isn't listed twice — drums stay the legacy `'drums'` target/key. routes.py untouched. Tests (tests/drum_arrangement.test.mjs, +15): the sync state machine (materialize/update/remove/idempotent/append/same-ref/degrade), the remove→restore undo round-trip, byte-identical load→save, the "Drums"-NAMED-but- untyped safety case (survives save, keeps its arr target), no duplicate tracks/band rows, and the index helpers incl. the interspersed-drums backend index. JS 297/0, lint 0 err / 3 baseline, routes.py untouched (no pytest). Stacked on #335 (needs arrKind / _arrTypeKind / the `type` round-trip). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou --- CHANGELOG.md | 12 ++ src/arrangement.js | 16 ++- src/audio.js | 4 + src/create.js | 4 + src/drum-arrangement.js | 117 +++++++++++++++++++ src/file-ops.js | 12 +- src/main.js | 11 +- src/parts-view.js | 3 + src/track-session.js | 8 ++ tests/drum_arrangement.test.mjs | 200 ++++++++++++++++++++++++++++++++ 10 files changed, 381 insertions(+), 6 deletions(-) create mode 100644 src/drum-arrangement.js create mode 100644 tests/drum_arrangement.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ad3db34..d06f6180 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Drums are now a first-class arrangement (groundwork for multiple drum charts).** + The single drum tab has always lived *outside* the arrangement list as a lone + off-array singleton — the one instrument that wasn't an ordinary track. It now + also lives *in* the arrangement list as a `type:"drums"` arrangement, migrated + in automatically when a song loads. Nothing changes for you yet: the drum grid, + the Tracks row, the mixer, and saving all behave exactly as before (a saved pack + is byte-for-byte identical — drums still persist as the song-level drum tab), and + the drum editor's undo history is untouched. This is the load-bearing model + change that lets a song hold *more than one* drum chart (two drummers, an aux- + percussion layer) in a following release, built on the new authored-instrument + `type` so a drums track is identified by what it *is*, never its name. + - **A track's instrument is now DATA, not a guess from its name.** The editor used to infer whether a part was keys / bass / guitar from its *name*, in a dozen places with two subtly disagreeing rules — the reason renaming a track could flip diff --git a/src/arrangement.js b/src/arrangement.js index b4e92176..00f817b1 100644 --- a/src/arrangement.js +++ b/src/arrangement.js @@ -12,6 +12,7 @@ import { _editorEscHtml, _editorPromptText, setStatus } from './ui.js'; import { flattenChords } from './chords.js'; import { KEYS_PATTERN } from './keys.js'; import { _arrTypeKind } from './instrument.js'; +import { clampAwayFromDrums, isDrumArrangement, pitchedArrangementCount, pitchedIndexOf, syncDrumArrangement } from './drum-arrangement.js'; import { _recState } from './midi-record.js'; import { _maybeOfferMidiTempoMap, _showDrumImportUnmappedModal } from './import.js'; import { host } from './host.js'; @@ -171,9 +172,12 @@ export async function editorRemoveArrangement() { setStatus('Cannot remove an arrangement while recording. Stop the take first.'); return false; } - if (S.arrangements.length <= 1) return false; + // Count PITCHED arrangements — the derived drums arrangement doesn't count + // (removing the last pitched part would leave a drums-only, invalid song). + if (pitchedArrangementCount(S.arrangements) <= 1) return false; const removeIdx = S.currentArr; const arr = S.arrangements[removeIdx]; + if (!arr || isDrumArrangement(arr)) return false; // never the drums arrangement if (!confirm(`Remove "${arr.name}" arrangement?`)) return false; // Remove from backend first @@ -184,7 +188,9 @@ export async function editorRemoveArrangement() { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ session_id: S.sessionId, - arrangement_index: removeIdx, + // The backend's arrangements[] has no drums arrangement — map + // the frontend index to its pitched-only position. + arrangement_index: pitchedIndexOf(S.arrangements, removeIdx), }), }); const result = await resp.json(); @@ -207,7 +213,9 @@ export async function editorRemoveArrangement() { // reconstructChords() reset (#18): drop the stack when the model shifts // under it. if (S.history) S.history.reset(); - S.currentArr = Math.min(removeIdx, S.arrangements.length - 1); + // Clamp to a PITCHED index — never leave the selection on the drums + // arrangement (which the removed slot may now expose). + S.currentArr = clampAwayFromDrums(S.arrangements, removeIdx); S.sel.clear(); flattenChords(); host.updateArrangementSelector(); @@ -262,6 +270,7 @@ export function editorAddEmptyDrums() { S.drumTab = { version: 1, name: 'Drums', kit: [], hits: [] }; S.drumTabDirty = true; S.drumSel = new Set(); + syncDrumArrangement(S); // materialize the type:"drums" arrangement markSessionDirty(); host.updateArrangementSelector(); host.updateStatus(); @@ -395,6 +404,7 @@ export async function editorDoAddDrums() { } S.drumTabDirty = true; // user-imported — persist on next save S.drumSel = new Set(); + syncDrumArrangement(S); // reflect the imported tab in S.arrangements[] editorHideAddDrumsModal(); const hitCount = Array.isArray(data.drum_tab.hits) diff --git a/src/audio.js b/src/audio.js index 185f8d6a..463e68e4 100644 --- a/src/audio.js +++ b/src/audio.js @@ -1707,6 +1707,10 @@ function _guidePitchedEvents() { function _bandPartsPure(arrangements, drumTab) { const out = []; (arrangements || []).forEach((a, i) => { + // The drums arrangement plays through the `'drums'` band key (from + // drumTab) below, not as an `arr:` part — skip it so it doesn't add + // a phantom (empty-note) band entry. + if (a && a.type === 'drums') return; if (a) out.push({ key: 'arr:' + i, idx: i, name: a.name || ('Track ' + (i + 1)) }); }); if (drumTab && Array.isArray(drumTab.hits) && drumTab.hits.length) { diff --git a/src/create.js b/src/create.js index fb5271f2..dbeabd8b 100644 --- a/src/create.js +++ b/src/create.js @@ -24,6 +24,7 @@ import { _updateTonesButtonVisibility, } from './annotation-lanes.js'; import { _handshapesAreDirty, flattenChords, reconstructChords } from './chords.js'; +import { isDrumArrangement } from './drum-arrangement.js'; import { EditHistory } from './history.js'; import { host } from './host.js'; import { isKeysMode, updatePianoRange } from './keys.js'; @@ -2568,6 +2569,9 @@ export async function editorBuild() { const savedArr = S.currentArr; const allArrangements = []; for (let i = 0; i < S.arrangements.length; i++) { + // Drums ship via `drum_tab`, never as a manifest arrangement (a session + // may hold a materialized type:"drums" arrangement). + if (isDrumArrangement(S.arrangements[i])) continue; S.currentArr = i; flattenChords(); reconstructChords(); diff --git a/src/drum-arrangement.js b/src/drum-arrangement.js new file mode 100644 index 00000000..3dca6407 --- /dev/null +++ b/src/drum-arrangement.js @@ -0,0 +1,117 @@ +// ════════════════════════════════════════════════════════════════════ +// Drums as a first-class arrangement — the drums-as-arrangements foundation. +// +// Historically the single drum tab (`S.drumTab`) lived ENTIRELY OUTSIDE +// `S.arrangements[]`, as a lone off-array singleton edited through a global +// mode (`S.drumEditMode`) — the one instrument that wasn't an ordinary +// arrangement. This leaf gives it a home IN the arrangement list as a +// `type:"drums"` entry, so drums are an ordinary typed arrangement +// (`arrKind(arr) === 'drums'`, via the #335 instrument-type-as-data seam) that +// the arrangement infrastructure can hold — the substrate multiple drum charts +// will grow from. +// +// FOUNDATION SCOPE (this PR): the drums arrangement is MATERIALIZED into +// `S.arrangements[]` and `S.drumTab` stays the live editing surface — the +// arrangement's `.drumTab` payload IS `S.drumTab`, the SAME object reference, +// so every existing `S.drumTab` reader/mutator and every drum undo command +// (which hold references into `S.drumTab.hits`) keep working byte-for-byte. +// The tracks/mixer/switcher still address drums through the legacy `'drums'` +// target/mix-key for now; promoting those to `arr:` and routing the drum +// grid off arrangement selection (instead of the global mode) are the +// follow-ups. Build/save still persists drums as the song-level `drum_tab` +// primary — the drums arrangement is DERIVED on load and never written into +// the manifest `arrangements[]` (a drums entry there would default to guitar +// and be fretted-graded as garbage until the core loader learns `type:drums`). +// +// Leaf module (imports only the instrument-identity leaf), so state/load/ +// track-session can call it without closing a cycle. +// ════════════════════════════════════════════════════════════════════ + +import { _arrTypeKind } from './instrument.js'; + +export const DRUMS_ARR_TYPE = 'drums'; +// Stable synthetic id for the derived drums arrangement. Never persisted (the +// arrangement is excluded from the save body); it exists only so track-session +// rows/targets that key off an arrangement id have a value that won't collide. +const DRUMS_ARR_ID = 'drums'; + +// Is this arrangement a drums arrangement? Keyed on the authored `type` ALONE +// (normalized: "drum"/"Drums" → drums), NOT on `arrKind` — `arrKind` would name- +// infer, wrongly catching a pitched arrangement a user literally named "Drums" +// and then hiding/dropping it. The materialized drums arrangement always carries +// `type:"drums"`, so a type-only test is exact and safe. +export function isDrumArrangement(arr) { + return _arrTypeKind(arr) === DRUMS_ARR_TYPE; +} + +// The (first) drums arrangement in the list, or null. Single drum still today; +// this is the seam the multiple-drum-charts work extends. +export function findDrumArrangement(arrangements) { + return (Array.isArray(arrangements) ? arrangements : []).find(isDrumArrangement) || null; +} + +// The number of PITCHED (non-drums) arrangements — what "how many arrangements +// are there" means everywhere the derived drums arrangement must not be counted +// (the remove-last-arrangement guard, single-arrangement checks). +export function pitchedArrangementCount(arrangements) { + return (Array.isArray(arrangements) ? arrangements : []).filter(a => !isDrumArrangement(a)).length; +} + +// Clamp an arrangement index so `S.currentArr` never lands on the drums +// arrangement — the "current" arrangement is always a pitched one (drums are +// edited through the drum grid, not selected as the pitched arrangement). Walks +// DOWN to the nearest pitched index; falls back to 0. +export function clampAwayFromDrums(arrangements, idx) { + const arrs = Array.isArray(arrangements) ? arrangements : []; + let i = Math.max(0, Math.min(Number(idx) || 0, arrs.length - 1)); + while (i > 0 && isDrumArrangement(arrs[i])) i--; + return i; +} + +// The BACKEND arrangement index for a frontend index: the count of pitched +// arrangements before it. The backend's `arrangements[]` never contains the +// session-only drums arrangement, so a frontend index (which may sit after an +// interspersed drums entry) must be mapped to its pitched-only position before +// it is sent to /remove-arrangement. +export function pitchedIndexOf(arrangements, idx) { + return (Array.isArray(arrangements) ? arrangements : []) + .slice(0, Math.max(0, Number(idx) || 0)).filter(a => !isDrumArrangement(a)).length; +} + +// Reconcile `S.arrangements[]` with `S.drumTab`: materialize / update / remove +// the single drums arrangement so its `.drumTab` payload IS `S.drumTab` (same +// object reference — the live editing surface). Idempotent, so it is safe to +// call after EVERY `S.drumTab` (re)assignment (load, GP/MIDI import, empty-add, +// delete + its undo-restore). Appended at the END, so existing arrangement +// indices — and therefore every `arr:` mix key — are preserved. Returns +// the drums arrangement, or null when there are no drums. +export function syncDrumArrangement(S) { + if (!S || !Array.isArray(S.arrangements)) return null; + const existing = findDrumArrangement(S.arrangements); + const tab = S.drumTab; + if (!tab || typeof tab !== 'object') { + // Drums removed → drop the arrangement (leave every other entry in place). + if (existing) S.arrangements.splice(S.arrangements.indexOf(existing), 1); + return null; + } + if (existing) { + // Re-point to the live payload (import replaces the object) and follow + // its name; the SAME object reference keeps drum-editor undo refs valid. + existing.drumTab = tab; + existing.name = String(tab.name || 'Drums').slice(0, 120); + return existing; + } + const arr = { + id: DRUMS_ARR_ID, + name: String(tab.name || 'Drums').slice(0, 120), + type: DRUMS_ARR_TYPE, + // SAME object reference as S.drumTab — the drum grid edits this in place. + drumTab: tab, + // Drums carry no fretted/pitched content; empty arrays keep every + // arrangement iterator (band audio, draw guards) safe. + notes: [], + chords: [], + }; + S.arrangements.push(arr); + return arr; +} diff --git a/src/file-ops.js b/src/file-ops.js index fdfb7ba2..c2beecd4 100644 --- a/src/file-ops.js +++ b/src/file-ops.js @@ -8,6 +8,7 @@ import { _abDisarm, _guideAnalysisReset, _resetAuditionForNewSong, loadAudio, re import { _handshapesAreDirty, _normalizeHandshape, flattenChords, reconstructChords } from './chords.js'; import { _normalizeTuningToLanes } from './commands.js'; import { editorBuild } from './create.js'; +import { isDrumArrangement, syncDrumArrangement } from './drum-arrangement.js'; import { EditHistory } from './history.js'; import { isKeysMode, rollResetLaneH, updatePianoRange } from './keys.js'; import { _seedExtendedStringsFromTuning, _stringCountFor } from './lanes.js'; @@ -218,6 +219,10 @@ export async function loadCDLC(filename, options = {}) { if (S.drumTab && Array.isArray(S.drumTab.hits)) { S.drumTab.hits.sort((a, b) => (a.t || 0) - (b.t || 0)); } + // Migrate the song-level drum tab into a `type:"drums"` arrangement + // (S.arrangements comes from data.arrangements above; drums are appended + // last so no arr: shifts). The arrangement's payload IS S.drumTab. + syncDrumArrangement(S); // Freshly loaded from disk — not dirty until the user edits it. S.drumTabDirty = false; // The master recording's URL is the active source's anchor — held @@ -650,7 +655,12 @@ export function _buildSaveBody(forceFullSnapshot) { // overwrite the on-disk `tones: null` sentinel with an // empty `{base, slots, changes, definitions}` dict on the // next sloppak save. - body.arrangements = S.arrangements.map(a => { + // Drums persist as the song-level `drum_tab` (body.drum_tab below), not + // as a manifest arrangement — the type:"drums" arrangement is a session- + // only derivation, so exclude it here to keep the built pack byte- + // identical (and to keep a drums entry out of arrangements[], where an + // old core would fretted-grade it). + body.arrangements = S.arrangements.filter(a => !isDrumArrangement(a)).map(a => { if (!a) return a; // Strip `_anchorEditCount` / `_handshapeEditCount` from every // arrangement so the dirty counters never leak to the backend's diff --git a/src/main.js b/src/main.js index 49f7e0d4..93693c78 100644 --- a/src/main.js +++ b/src/main.js @@ -189,6 +189,7 @@ import { _rollLockNotice, _rollMidiForNote, _rollPitchCtx, _rollReadOnly, editorKeyNoteNames, isKeysMode, midiToNote, updatePianoRange } from './keys.js'; import { arrKind } from './instrument.js'; +import { clampAwayFromDrums } from './drum-arrangement.js'; import { _restoreSuggestedMarks, _saveSuggestedMarks, _suggestedCount, chords, notes @@ -1039,19 +1040,25 @@ function updateArrangementSelector() { const sel = document.getElementById('editor-arrangement'); sel.innerHTML = ''; S.arrangements.forEach((arr, i) => { + // The drums arrangement isn't offered in this pitched-part switcher — + // it's opened via 🥁 Edit Drums / its Tracks row. Skipping it keeps the + // dropdown (and its show/hide threshold below) byte-identical. + if (arr && arr.type === 'drums') return; const opt = document.createElement('option'); opt.value = i; opt.textContent = arr.name; sel.appendChild(opt); }); - sel.style.display = S.arrangements.length > 1 ? '' : 'none'; + sel.style.display = sel.options.length > 1 ? '' : 'none'; // Re-apply the active arrangement after the rebuild so callers that // changed S.currentArr (e.g. + Keys / + Drums append, remove-arr) // don't end up with a `, so a charter opens the drum editor by picking it — exactly like switching to Lead/Rhythm/Bass — instead of reaching for a mode button off to the side. The drum chart becomes a first-class part in the one place parts are chosen, and the set-up for choosing between MULTIPLE drum charts later. SAFE invariant kept: S.currentArr NEVER moves onto the drums arrangement. The drum grid stays a MODE (S.drumEditMode) OVER the current pitched part — a brand- new "currentArr === drums" state would have exposed the drums arrangement (empty notes, no tuning) to the pervasive "current arrangement" readers, none of which expect it. So a new `editorSwitcherSelect` wrapper (the - diff --git a/src/drum-arrangement.js b/src/drum-arrangement.js index 3dca6407..969e1312 100644 --- a/src/drum-arrangement.js +++ b/src/drum-arrangement.js @@ -50,6 +50,21 @@ export function findDrumArrangement(arrangements) { return (Array.isArray(arrangements) ? arrangements : []).find(isDrumArrangement) || null; } +// The index of the drums arrangement in the list, or -1. The arrangement switcher +// uses it to DISPLAY the drums option as selected while drum-edit mode is on — +// even though S.currentArr itself stays on a pitched arrangement. +export function drumArrangementIndex(arrangements) { + return (Array.isArray(arrangements) ? arrangements : []).findIndex(isDrumArrangement); +} + +// Which arrangement index the switcher should DISPLAY as selected: the drums +// arrangement while drum-edit mode is on (its view is the drum grid), else the +// current pitched arrangement. currentArr itself never moves onto drums. +export function switcherShownIndex(arrangements, currentArr, drumEditMode) { + const di = drumArrangementIndex(arrangements); + return (drumEditMode && di >= 0) ? di : currentArr; +} + // The number of PITCHED (non-drums) arrangements — what "how many arrangements // are there" means everywhere the derived drums arrangement must not be counted // (the remove-last-arrangement guard, single-arrangement checks). diff --git a/src/drum.js b/src/drum.js index e8679c43..228b12d9 100644 --- a/src/drum.js +++ b/src/drum.js @@ -1228,6 +1228,7 @@ function _ensureDrumEditButton() { host.refreshTempoMapButton(); host.refreshPartsViewButton(); host.refreshDrumPadStrip(); + host.updateArrangementSelector(); // reflect drum-edit mode in the switcher host.draw(); }; drumsBtn.parentNode.insertBefore(btn, drumsBtn.nextSibling); diff --git a/src/main.js b/src/main.js index 93693c78..2c3debe8 100644 --- a/src/main.js +++ b/src/main.js @@ -189,7 +189,7 @@ import { _rollLockNotice, _rollMidiForNote, _rollPitchCtx, _rollReadOnly, editorKeyNoteNames, isKeysMode, midiToNote, updatePianoRange } from './keys.js'; import { arrKind } from './instrument.js'; -import { clampAwayFromDrums } from './drum-arrangement.js'; +import { clampAwayFromDrums, isDrumArrangement, switcherShownIndex } from './drum-arrangement.js'; import { _restoreSuggestedMarks, _saveSuggestedMarks, _suggestedCount, chords, notes @@ -614,6 +614,7 @@ setHostHooks({ _refreshPartsViewButton(); _refreshDrumEditButton(); _refreshTempoMapButton(); + updateArrangementSelector(); // reflect drums/pitched selection in the switcher draw(); updateStatus(); }, @@ -1040,13 +1041,12 @@ function updateArrangementSelector() { const sel = document.getElementById('editor-arrangement'); sel.innerHTML = ''; S.arrangements.forEach((arr, i) => { - // The drums arrangement isn't offered in this pitched-part switcher — - // it's opened via 🥁 Edit Drums / its Tracks row. Skipping it keeps the - // dropdown (and its show/hide threshold below) byte-identical. - if (arr && arr.type === 'drums') return; + if (!arr) return; const opt = document.createElement('option'); opt.value = i; - opt.textContent = arr.name; + // Drums are a selectable part whose view is the drum grid — mark the + // option with 🥁 so it reads as the drum editor, not a pitched chart. + opt.textContent = (arr.type === 'drums') ? ('🥁 ' + (arr.name || 'Drums')) : arr.name; sel.appendChild(opt); }); sel.style.display = sel.options.length > 1 ? '' : 'none'; @@ -1056,11 +1056,11 @@ function updateArrangementSelector() { // canvas edits the appended arrangement. Clamp to the valid range // so an out-of-bounds S.currentArr doesn't render as a blank value. if (S.arrangements.length > 0) { - // Clamp to a PITCHED index — the derived drums arrangement is never the - // selected pitched arrangement (it has no option in this switcher). - const idx = clampAwayFromDrums(S.arrangements, S.currentArr || 0); - S.currentArr = idx; - sel.value = String(idx); + // currentArr stays a PITCHED index (the drum grid is a MODE over it, not + // a move onto the drums arrangement). But while drum-edit mode is on, + // DISPLAY the drums option as selected so the dropdown matches the canvas. + S.currentArr = clampAwayFromDrums(S.arrangements, S.currentArr || 0); + sel.value = String(switcherShownIndex(S.arrangements, S.currentArr, S.drumEditMode)); } // "+ Track" — the single New Track entry (the old + Drums / + Keys / @@ -1801,6 +1801,41 @@ window.editorSelectArrangement = (val) => { draw(); updateStatus(); }; +// The arrangement before the sloppak guard's early-return. The pitched branch also failed to clear partsViewMode/tempoMapMode/tempoSel, so switching to a pitched part from Tempo Map or Parts view left the old lens painted over it; clear those too, mirroring the drums branch and openTrackSessionTarget. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/main.js | 30 ++++++++++++-- tests/view_switcher.test.mjs | 79 ++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/src/main.js b/src/main.js index 2c3debe8..b2d2b5b3 100644 --- a/src/main.js +++ b/src/main.js @@ -602,6 +602,10 @@ setHostHooks({ S.partsViewMode = false; S.tempoMapMode = false; S.tempoSel = -1; + // draw() checks tabViewMode first — drop the engraved-tab lens on any + // target switch (shared root cause with editorSwitcherSelect / the + // Edit-Drums button) or it paints the old part's tab over the new one. + S.tabViewMode = false; if (targetId === 'drums' && S.drumTab && S.format === 'sloppak') { S.drumEditMode = true; S.drumSel = new Set(); @@ -1814,11 +1818,19 @@ window.editorSwitcherSelect = (val) => { // arrangement only exists in a sloppak session that has a drum tab; guard // anyway so a drums index can NEVER fall through to editorSelectArrangement // (which would move currentArr onto the drums arrangement). - if (!(S.drumTab && S.format === 'sloppak')) return; + // Resync the even when the drums path is unavailable', () => { + Object.assign(S, { + arrangements: [GTR('Lead'), DRUMS()], currentArr: 0, + drumTab: null, format: 'sloppak', // no drum tab → the guarded early return + tabViewMode: false, drumEditMode: false, sel: new Set(), + }); + const { fn, counts } = makeSwitcher(); + fn('1'); + assert.strictEqual(counts.selectorSyncs, 1, + 'the dropdown is snapped back off the Drums option it optimistically showed'); + assert.strictEqual(S.drumEditMode, false, 'nothing switched'); +}); + console.log(`\n${pass} passed, ${fail} failed`); if (fail) process.exit(1); From c75d123fd068091d4748bb3e2a860f13179808cd Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Mon, 20 Jul 2026 22:32:52 +0200 Subject: [PATCH 13/18] fix(editor): make drum-tab delete undo a true inverse (renumber arr keys) DeleteDrumTabCmd deleted the drums arrangement without renumbering the higher arr: partMix keys, so deleting drums from a non-last position stranded every pitched strip's mute/solo/volume and could silence the band via an orphaned solo. Route the drop through the existing _partMixDropArrangementPure so keys renumber like the pitched-delete path. Undo must be a true inverse: syncDrumArrangement re-appends drums LAST, which shifts later arrangements and strands index-based undo commands + currentArr. Restore drums to its ORIGINAL slot on rollback; add _partMixInsertArrangementPure (exact inverse of the drop) so live mix edits made while drums was gone ride along instead of being clobbered; snapshot/restore currentArr; guard drumIndex>=0 so an unmaterialized drums tab can't shift arr:0 to arr:-1. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/audio.js | 3 +- src/track-session.js | 72 +++++++++--- tests/drum_delete_undo_middle.test.mjs | 153 +++++++++++++++++++++++++ 3 files changed, 211 insertions(+), 17 deletions(-) create mode 100644 tests/drum_delete_undo_middle.test.mjs diff --git a/src/audio.js b/src/audio.js index 2244fd37..9db0191d 100644 --- a/src/audio.js +++ b/src/audio.js @@ -1972,7 +1972,8 @@ export function _stemCatchupPure(playStartTime, playStartWall, currentTime, rate // playing node, drop its gain node, and — the one that bites — delete its // 'audio:' entry from S.partMix. That entry is counted by the whole-map // solo rule, so a stale SOLO left behind by a removed stem would silence every -// live track. Mirrors the drum-delete path (delete S.partMix.drums). +// live track. Same hazard the arrangement-delete path guards against by +// renumbering the arr: keys (see _partMixDropArrangementPure). export function _pruneStaleStems(liveIds) { for (const id of [...playingStemSources.keys()]) { if (liveIds.has(id)) continue; diff --git a/src/track-session.js b/src/track-session.js index 0de64ec4..49b3eb89 100644 --- a/src/track-session.js +++ b/src/track-session.js @@ -154,6 +154,22 @@ export function _partMixDropArrangementPure(partMix, index) { return out; } +// Inverse of _partMixDropArrangementPure: open a slot at `index` (renumber every +// arr: with n >= index UP one) and drop `strip` in at arr:. Renumbers +// the CURRENT map, so any live mute/solo/volume edits made while the slot was +// gone ride along to their restored key instead of being clobbered. +export function _partMixInsertArrangementPure(partMix, index, strip) { + const out = {}; + for (const [key, value] of Object.entries(partMix && typeof partMix === 'object' ? partMix : {})) { + if (!key.startsWith('arr:')) { out[key] = value; continue; } + const n = Number(key.slice(4)); + if (!Number.isInteger(n)) continue; + out[n >= index ? 'arr:' + (n + 1) : key] = value; + } + if (strip !== undefined) out['arr:' + index] = strip; + return out; +} + // Normalize any persisted/half-trusted tree against the loaded song: drop // rows whose source/target no longer exists, append rows for anything new, // repair parent cycles, and default the tempo guide. Idempotent — this is @@ -880,25 +896,40 @@ export class DeleteDrumTabCmd { this._name = rowName; this._tab = S.drumTab; this._dirty = !!S.drumTabDirty; - // The drums arrangement's mix strip is keyed `arr:` now (PR2b). - // Capture the key at construct time — exec clears S.drumTab, which drops - // the arrangement and its index. Drums appends last, so on undo it - // re-materializes at the same slot (rollback re-derives the index). - this._mixKey = 'arr:' + drumArrangementIndex(S.arrangements); - this._hadMixStrip = !!(S.partMix && (this._mixKey in S.partMix)); - this._mixStrip = S.partMix ? S.partMix[this._mixKey] : undefined; + // Snapshot everything the splice renumbers so undo is a TRUE inverse: + // deleting a drums arrangement that sits mid-list shifts every later + // arrangement (and its `arr:` mix strip) down a slot. Undo must + // put drums back in its ORIGINAL slot — NOT re-append it last — or the + // index-based undo commands, currentArr, and mix keys that ride on the + // old order would all point one arrangement off after the delete is undone. + this._drumIndex = drumArrangementIndex(S.arrangements); + this._drumStrip = (S.partMix && this._drumIndex >= 0) ? S.partMix['arr:' + this._drumIndex] : undefined; + this._prevCurrentArr = S.currentArr; this._links = S.stemLinks; this._tree = S.trackSession; this._selectedTrackId = S.selectedTrackId; } exec() { + // Capture the drum slot BEFORE syncDrumArrangement splices it out — + // dropping it must renumber the higher pitched arr: keys down a + // slot, same as the pitched-delete path. A bare delete stranded every + // strip after drums (a stranded solo silenced the whole band). + const drumIndex = drumArrangementIndex(S.arrangements); S.drumTab = null; syncDrumArrangement(S); // remove the derived type:"drums" arrangement // Dirty is what ships the explicit `drum_tab: null` removal on the // next save (see _buildSaveBody) — without it the backend's // absent→preserve path would resurrect drum_tab.json on reload. S.drumTabDirty = true; - if (S.partMix) delete S.partMix[this._mixKey]; + // drumIndex is -1 if the drums arrangement was never materialized (the + // 'drums' target row exists off drumTab.hits alone) — dropping index -1 + // would renumber EVERY arr: down and corrupt the map, so only renumber + // when a real slot was found. + if (S.partMix && drumIndex >= 0) S.partMix = _partMixDropArrangementPure(S.partMix, drumIndex); + // The splice shifted every arrangement after drums down one slot; follow + // the selection so currentArr stays in bounds (and off drums) while + // drums is gone. Undo restores it from the snapshot. Mirrors the key renumber. + if (drumIndex >= 0 && drumIndex < S.currentArr) S.currentArr -= 1; S.stemLinks = _trackLinksRetargetPure(S.stemLinks, DRUM_TARGET_ID); if (S.selectedTrackId === transcriptionTrackId(DRUM_TARGET_ID)) { S.selectedTrackId = ''; @@ -908,16 +939,25 @@ export class DeleteDrumTabCmd { } rollback() { S.drumTab = this._tab; - syncDrumArrangement(S); // restore the derived type:"drums" arrangement + const arr = syncDrumArrangement(S); // re-materialize (syncDrumArrangement appends LAST) + // Move drums back to its ORIGINAL slot so every later arrangement + // returns to its pre-delete index — otherwise index-based undo commands + // (and currentArr) would target the wrong arrangement after this undo. + if (arr && this._drumIndex >= 0) { + const cur = S.arrangements.indexOf(arr); + if (cur !== this._drumIndex) { + S.arrangements.splice(cur, 1); + S.arrangements.splice(this._drumIndex, 0, arr); + } + } S.drumTabDirty = this._dirty; - if (this._hadMixStrip) { - if (!S.partMix) S.partMix = {}; - // syncDrumArrangement (above) re-appended the drums arrangement; - // restore its strip under the CURRENT index (robust if the pitched - // count shifted while drums was deleted). - const di = drumArrangementIndex(S.arrangements); - S.partMix[di >= 0 ? 'arr:' + di : this._mixKey] = this._mixStrip; + // Re-open the drum slot and shift the surviving strips back UP — the + // inverse of exec's drop. Operates on the CURRENT map, so live mix edits + // made while drums was gone survive the undo (they aren't history commands). + if (this._drumIndex >= 0 && S.partMix) { + S.partMix = _partMixInsertArrangementPure(S.partMix, this._drumIndex, this._drumStrip); } + S.currentArr = this._prevCurrentArr; S.stemLinks = this._links; if (this._selectedTrackId === transcriptionTrackId(DRUM_TARGET_ID) && !S.selectedTrackId) { diff --git a/tests/drum_delete_undo_middle.test.mjs b/tests/drum_delete_undo_middle.test.mjs new file mode 100644 index 00000000..ba88f5b8 --- /dev/null +++ b/tests/drum_delete_undo_middle.test.mjs @@ -0,0 +1,153 @@ +/* + * Regression: deleting the drum transcription when it is NOT the last + * arrangement (a pitched arrangement sits after it). + * + * The drums channel is `arr:` (PR2b). Splicing drums out shifts every + * higher pitched arrangement down one slot, so its `arr:` mix key must + * shift down too — exactly what the pitched-delete path does via + * _partMixDropArrangementPure. A bare `delete S.partMix['arr:']` + * stranded the pitched strips after drums (lost mute/solo/vol; a stranded solo + * silenced the whole band) and undo then OVERWROTE the stranded strip. + * + * Roster: Lead(arr:0) + Drums(arr:1) + Bass(arr:2). + * + * Run: node tests/drum_delete_undo_middle.test.mjs + */ +import assert from 'node:assert'; + +globalThis.localStorage = globalThis.localStorage || { + getItem: () => null, setItem: () => {}, removeItem: () => {}, +}; +globalThis.document = globalThis.document || { getElementById: () => null }; + +const { DeleteDrumTabCmd } = await import('../src/track-session.js'); +const { EditHistory } = await import('../src/history.js'); +const { syncDrumArrangement, isDrumArrangement } = await import('../src/drum-arrangement.js'); +const { _mixerAnySoloPure } = await import('../src/mixer-panel.js'); +const { S } = await import('../src/state.js'); + +let pass = 0, fail = 0; +function t(name, fn) { + try { fn(); pass++; console.log(' ok ' + name); } + catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } +} + +function seed() { + const pitched = () => ({ tuning: [0, 0, 0, 0, 0, 0], capo: 0, notes: [], chords: [], chord_templates: [] }); + const tab = { name: 'Drums', version: 1, hits: [{ t: 0.5, lane: 'kick' }] }; + Object.assign(S, { + sessionId: 'sess-1', createMode: false, format: 'sloppak', sloppakForm: 'zip', + filename: 'song.feedpak', title: 'T', artist: 'A', + // Lead at 0; syncDrumArrangement appends drums at 1; Bass added at 2 AFTER. + arrangements: [{ name: 'Lead', ...pitched() }], + currentArr: 0, beats: [], sections: [], + drumTab: tab, drumTabDirty: false, + partMix: { + 'arr:0': { audible: true, vol: 1 }, // Lead + 'arr:1': { audible: false, vol: 0.5 }, // Drums (mute-ish) + }, + stemLinks: {}, trackSession: null, trackHeights: {}, stems: [], + sessionDirty: false, history: new EditHistory(), sel: new Set(), + }); + syncDrumArrangement(S); // drums → arr:1 + // Now add a pitched arrangement AFTER drums so drums is in the middle. + S.arrangements.push({ name: 'Bass', ...pitched() }); // Bass → arr:2 + S.partMix['arr:2'] = { audible: true, vol: 0.8, solo: true }; + return tab; +} + +t('delete drums-in-the-middle renumbers higher pitched strips down', () => { + seed(); + S.history.exec(new DeleteDrumTabCmd('Drums')); + // Bass's arrangement is now index 1; its strip must follow to arr:1. + assert.deepStrictEqual(S.partMix['arr:1'], { audible: true, vol: 0.8, solo: true }, + 'Bass strip renumbered arr:2 → arr:1'); + assert.strictEqual('arr:2' in S.partMix, false, 'no stray arr:2 left behind'); + assert.strictEqual(_mixerAnySoloPure(S.partMix), true, + 'the surviving solo stays on a LIVE strip — band not silenced'); +}); + +t('undo restores every strip AND the original arrangement order', () => { + seed(); + S.history.exec(new DeleteDrumTabCmd('Drums')); + S.history.doUndo(); + // True inverse: drums returns to its ORIGINAL middle slot (arr:1) and Bass + // back at arr:2 — NOT re-appended last, so index-based undo/mix keys stay valid. + assert.deepStrictEqual(S.partMix['arr:0'], { audible: true, vol: 1 }, 'Lead intact'); + assert.deepStrictEqual(S.partMix['arr:1'], { audible: false, vol: 0.5 }, + 'drums strip restored at its original middle slot'); + assert.deepStrictEqual(S.partMix['arr:2'], { audible: true, vol: 0.8, solo: true }, + 'Bass strip (solo+vol) restored at its original slot'); + assert.strictEqual(isDrumArrangement(S.arrangements[1]), true, 'drums back in the middle'); + assert.strictEqual(S.arrangements[2].name, 'Bass', 'Bass back last — order preserved'); +}); + +t('an index-based undo command survives a drums-in-the-middle delete', () => { + seed(); + S.currentArr = 2; // Bass selected (sits AFTER drums) + const bass = S.arrangements[2]; + // Stand-in for MoveNoteCmd et al: they target S.arrangements[arrIdx] by index. + let touched = null; + S.history.exec({ + arrIdx: 2, + exec() { touched = S.arrangements[this.arrIdx]; }, + rollback() { touched = S.arrangements[this.arrIdx]; }, + }); + S.history.exec(new DeleteDrumTabCmd('Drums')); // drums-in-the-middle removed + S.history.doUndo(); // un-delete drums + S.history.doUndo(); // the note command rolls back + assert.strictEqual(touched, bass, + 'index-based command still targets Bass, not the re-appended drums arrangement'); + assert.strictEqual(isDrumArrangement(touched), false, 'never lands on drums'); +}); + +t('currentArr follows the selected pitched arrangement across delete + undo', () => { + seed(); + S.currentArr = 2; // Bass selected — sits AFTER drums (arr:1) + S.history.exec(new DeleteDrumTabCmd('Drums')); + // Drums spliced out shifts Bass 2→1; currentArr must follow, not dangle + // out of bounds (arrangements.length is now 2). + assert.ok(S.arrangements[S.currentArr] && !isDrumArrangement(S.arrangements[S.currentArr]), + 'currentArr points at a live pitched arrangement, not out of bounds'); + assert.strictEqual(S.arrangements[S.currentArr].name, 'Bass', 'still Bass selected after delete'); + S.history.doUndo(); + // True-inverse undo restores the original slot, so currentArr returns to 2. + assert.strictEqual(S.currentArr, 2, 'currentArr restored to Bass original slot'); + assert.ok(!isDrumArrangement(S.arrangements[S.currentArr]), + 'currentArr is not the drums arrangement'); + assert.strictEqual(S.arrangements[S.currentArr].name, 'Bass', 'still Bass selected after undo'); +}); + +t('a live mix edit made while drums is gone survives the undo', () => { + seed(); + S.history.exec(new DeleteDrumTabCmd('Drums')); // Bass now at arr:1 + // Mixer/fader edits mutate S.partMix in place, outside EditHistory. + S.partMix['arr:1'] = { audible: true, vol: 0.3, solo: false }; // user re-faders Bass + S.history.doUndo(); + // Bass returns to arr:2 carrying the LIVE edit, not the stale seeded value. + assert.deepStrictEqual(S.partMix['arr:2'], { audible: true, vol: 0.3, solo: false }, + 'the post-delete Bass fader edit rode the undo, not clobbered'); + assert.deepStrictEqual(S.partMix['arr:1'], { audible: false, vol: 0.5 }, 'drums strip back'); +}); + +t('delete does not corrupt mix keys when the drums arrangement was never materialized', () => { + const pitched = () => ({ tuning: [0, 0, 0, 0, 0, 0], capo: 0, notes: [], chords: [], chord_templates: [] }); + Object.assign(S, { + sessionId: 'sess-1', createMode: false, format: 'sloppak', sloppakForm: 'zip', + filename: 'song.feedpak', title: 'T', artist: 'A', + arrangements: [{ name: 'Lead', ...pitched() }], // NO drums arrangement materialized + currentArr: 0, beats: [], sections: [], + drumTab: { name: 'Drums', version: 1, hits: [{ t: 0.5, lane: 'kick' }] }, // but drumTab set + drumTabDirty: false, + partMix: { 'arr:0': { audible: true, vol: 1 } }, + stemLinks: {}, trackSession: null, trackHeights: {}, stems: [], + sessionDirty: false, history: new EditHistory(), sel: new Set(), + }); + // drumArrangementIndex is -1 here; a -1 drop would rewrite arr:0 → arr:-1. + S.history.exec(new DeleteDrumTabCmd('Drums')); + assert.deepStrictEqual(S.partMix['arr:0'], { audible: true, vol: 1 }, 'Lead strip untouched'); + assert.strictEqual('arr:-1' in S.partMix, false, 'no corrupt arr:-1 key'); +}); + +console.log(`\n${pass} passed, ${fail} failed`); +process.exit(fail ? 1 : 0); From 9c68ed8f9cbd8134457150988ec05da38187cef1 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Mon, 20 Jul 2026 22:38:14 +0200 Subject: [PATCH 14/18] feat(editor): add a per-arrangement instrument-type control Instrument is now first-class data, so an already-saved pack whose fretted chart was stamped type=piano (keys-word name) opened piano-locked with no way back to string view. Add a Guitar/Bass/Keys selector by the arrangement dropdown that authors arr.type, undoable via S.history (SetArrangementTypeCmd), rebuilding the chart in place with no data loss. Codex preflight fixes: - metadataScope lock opt-out so the type set works in the read-only piano roll (the escape hatch was non-functional exactly where it is needed) - canonicalize keys -> "piano" on write (spec canonical spelling; other consumers key on piano) - carry authored type through the create-mode build whitelist (was dropped) - guard the no-op check on the authored kind so stamping an inferred kind works (unblocks the rename-proofing workflow) Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 11 ++ screen.html | 7 + src/arrangement.js | 60 ++++++++- src/create.js | 7 + src/history.js | 8 +- src/main.js | 18 ++- tests/arrangement_type_control.test.mjs | 168 ++++++++++++++++++++++++ tests/create_save_routing.test.mjs | 18 +++ 8 files changed, 294 insertions(+), 3 deletions(-) create mode 100644 tests/arrangement_type_control.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ad3db34..48f6ef1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 them from silently re-laning notes). Foundation for first-class drum tracks and vocals. +- **A per-track instrument-type control (next to the arrangement selector).** + Now that the editor honors an authored instrument `type` above the name for the + first time, an already-saved pack whose fretted (string+fret) chart has a keys + word *mid-name* — "Electric Piano", "Lead Synth" — can open **piano-locked**: + string buttons hidden, view stuck on the roll, with previously no way to + override it. The new **Guitar / Bass / Keys** dropdown sets the track's type + directly, which wins over the name everywhere — so a mis-named fretted part can + be corrected back to Guitar and reopens in string view immediately. The change + is undoable (Ctrl+Z) and persists with the pack on save; switching type rebuilds + the chart in place with no note loss. + - **The piano roll stretches, compacts and scrolls.** Its lane height used to be derived and untouchable — the whole pitch range packed into about 350px — so a wide range collapsed to four pixels per semitone: passable for reading, useless diff --git a/screen.html b/screen.html index 0e4db38c..ac6f22c8 100644 --- a/screen.html +++ b/screen.html @@ -38,6 +38,13 @@ + + diff --git a/src/arrangement.js b/src/arrangement.js index b4e92176..ea53f4ac 100644 --- a/src/arrangement.js +++ b/src/arrangement.js @@ -11,7 +11,7 @@ import { S, markSessionDirty } from './state.js'; import { _editorEscHtml, _editorPromptText, setStatus } from './ui.js'; import { flattenChords } from './chords.js'; import { KEYS_PATTERN } from './keys.js'; -import { _arrTypeKind } from './instrument.js'; +import { _arrTypeKind, _typeKind } from './instrument.js'; import { _recState } from './midi-record.js'; import { _maybeOfferMidiTempoMap, _showDrumImportUnmappedModal } from './import.js'; import { host } from './host.js'; @@ -166,6 +166,64 @@ export async function editorRenameArrangement() { setStatus(`Renamed to “${guard.name}”`); } +// Undoable instrument-type set. Instrument identity is DATA (the feedpak-spec +// §5.2 `type` facet): an authored type WINS over name inference in every reader +// — arrKind / isKeysArr / the 4-vs-6 string baseline / view routing — so this is +// the escape hatch for a pack whose NAME loaded it into the wrong instrument +// (e.g. a fretted chart named "Electric Piano" opening keys-locked with no way +// to override). Rebuilds in place (Principle VI): only arr.type moves, notes and +// strings are untouched. Refreshes the selector + lane metrics on exec AND +// rollback so the view flips immediately in both directions (undo/redo call +// host.draw()/updateStatus() themselves). +class SetArrangementTypeCmd { + constructor(arrIdx, newType) { + this.arrIdx = arrIdx; + this.newType = newType; + const arr = S.arrangements[arrIdx]; + this.oldType = arr ? arr.type : undefined; // undefined = was untyped + // Metadata-only — writes arr.type, never a note. Opts out of the + // read-only-roll note lock so the escape hatch works even for a fretted + // part currently shown read-only in the roll (see history.js _locked). + this.metadataScope = true; + } + _set(type) { + const arr = S.arrangements[this.arrIdx]; + if (!arr) return; + if (type) arr.type = type; else delete arr.type; + host.updateArrangementSelector(); + host.resizeForLaneChange(this.arrIdx); + } + exec() { this._set(this.newType); } + rollback() { this._set(this.oldType); } +} + +// Set the active arrangement's instrument type from the toolbar selector. +// Only guitar / bass / keys are authorable here — drums-as-arrangement authoring +// lands in a later stacked PR. A no-op only when the type is ALREADY AUTHORED to +// this kind — NOT merely name-inferred: stamping an untyped part's inferred kind +// is a real, useful op (it makes identity DATA, which frees the rename guard's +// name-inference lock so the part can be renamed to a neutral display label). +export function editorSetArrangementType(value) { + if (_recState !== 'idle') { + setStatus('Cannot change the instrument type while recording. Stop the take first.'); + return; + } + const arr = S.arrangements[S.currentArr]; + if (!arr) return; + const kind = _typeKind(value); + if (kind !== 'guitar' && kind !== 'bass' && kind !== 'keys') return; + if (kind === _arrTypeKind(arr)) return; // already AUTHORED to this kind — nothing to do + // WRITE the canonical feedpak-spec §5.2 spelling: the keys family serializes + // as "piano" ("keys" is only a READ alias _typeKind folds in). The backend + // persists arr.type verbatim, so authoring "keys" would leave a non-canonical + // manifest value other consumers keyed on "piano" wouldn't recognize. + const canonType = kind === 'keys' ? 'piano' : kind; + S.history.exec(new SetArrangementTypeCmd(S.currentArr, canonType)); + host.draw(); + host.updateStatus(); + setStatus(`Instrument type set to ${kind}`); +} + export async function editorRemoveArrangement() { if (_recState !== 'idle') { setStatus('Cannot remove an arrangement while recording. Stop the take first.'); diff --git a/src/create.js b/src/create.js index fb5271f2..b9b203ed 100644 --- a/src/create.js +++ b/src/create.js @@ -2601,6 +2601,13 @@ export async function editorBuild() { chords: arr.chords, chord_templates: arr.chord_templates, }; + // Ship an authored instrument `type` (feedpak-spec §5.2) into the build + // payload — without it the backend re-infers the type from the NAME on + // /build, so a create-mode part re-typed via the toolbar selector (e.g. a + // fretted chart named "Electric Piano" corrected to guitar) would lose the + // correction on its very first build. The save path already ships it via + // the full-arrangement spread in _buildSaveBody; the build whitelist must too. + if (arr.type) arrEntry.type = arr.type; if (buildTones) arrEntry.tones = buildTones; if (arr._gp_notation) arrEntry._gp_notation = arr._gp_notation; // PR3d: include authored anchors too — same dirty-gate as diff --git a/src/history.js b/src/history.js index e6cdfc60..a9618e7f 100644 --- a/src/history.js +++ b/src/history.js @@ -36,9 +36,15 @@ export const MAX_UNDO = 500; // suggestResolved — the VA.3 suggest-position writer (resolved adds + // Accept) IS the sanctioned string/fret write path the // lock was holding the door for. It marks, never guesses. +// metadataScope — arrangement metadata (its `type`), never a single note. +// The lock guards silent string/fret WRITES; a type set +// touches no note, and the type control is the escape +// hatch a fretted part shown read-only in the roll needs +// most (re-type it out of keys). Unlike songScope it keeps +// _arrIdx, so undo still switches to the retyped part. // Nothing else opts out. Returns true when the command must not run. function _locked(cmd) { - if (cmd.songScope === true || cmd.pitchPreserving === true || cmd.suggestResolved === true) return false; + if (cmd.songScope === true || cmd.pitchPreserving === true || cmd.suggestResolved === true || cmd.metadataScope === true) return false; if (!_rollReadOnly()) return false; _rollLockNotice(); return true; diff --git a/src/main.js b/src/main.js index 49f7e0d4..d2908cea 100644 --- a/src/main.js +++ b/src/main.js @@ -69,7 +69,7 @@ import { import { editorDoAddDrums, editorDrumsFileSelected, editorDrumsGPSelected, editorHideAddDrumsModal, editorRemoveArrangement, editorRenameArrangement, - editorShowAddDrumsModal + editorSetArrangementType, editorShowAddDrumsModal } from './arrangement.js'; import { _activeArrangementExceedsArchiveLimit, _editorLoadsInFlight, _resetOffsetUI, @@ -643,6 +643,7 @@ window.editorDoImportGuitar = editorDoImportGuitar; // Arrangement management (rename / remove / add-drums import) — arrangement.js. window.editorRenameArrangement = editorRenameArrangement; +window.editorSetArrangementType = editorSetArrangementType; window.editorRemoveArrangement = editorRemoveArrangement; window.editorShowAddDrumsModal = editorShowAddDrumsModal; window.editorHideAddDrumsModal = editorHideAddDrumsModal; @@ -1074,6 +1075,21 @@ function updateArrangementSelector() { stringsBtn.classList.toggle('hidden', !S.sessionId || !stringsMode); } + // Instrument-type selector — the escape hatch that AUTHORS the arrangement's + // `type` (which every identity reader now honors over the name). Shown on a + // live session for any string/keys arrangement so a fretted chart opened + // piano-locked by a keys word in its name can be re-typed to guitar/bass; + // drums-as-arrangement authoring is a later PR, so a drums part hides it. + const typeSel = document.getElementById('editor-arr-type'); + if (typeSel) { + const active = S.arrangements[S.currentArr]; + const k = active && arrKind(active); + const showType = !!active && !!S.sessionId + && (k === 'guitar' || k === 'bass' || k === 'keys'); + typeSel.classList.toggle('hidden', !showType); + if (showType) typeSel.value = k; + } + // Show "● Record" (live MIDI) button on sloppak sessions only — archive's // add-arrangement path requires an xml_path we can't synthesize, and // archive build silently drops extra arrangements anyway. Mirror the diff --git a/tests/arrangement_type_control.test.mjs b/tests/arrangement_type_control.test.mjs new file mode 100644 index 00000000..cfcec89a --- /dev/null +++ b/tests/arrangement_type_control.test.mjs @@ -0,0 +1,168 @@ +/* + * Instrument-type control — the escape hatch that AUTHORS an arrangement's + * `type` so a fretted (string+fret) chart whose NAME contains a keys word + * ("Electric Piano", "Lead Synth") is no longer piano-locked. The authored + * type WINS over name inference in every reader (arrKind / _isBassArr / + * viewFor / isKeysArr), and the set is undoable (Principle IV). + * + * Fails on unfixed code: SetArrangementTypeCmd does not exist in + * src/arrangement.js, so there is no way to override the name inference and a + * keys-named fretted part stays keys-locked. + * + * Run: node tests/arrangement_type_control.test.mjs + */ +import assert from 'node:assert'; +import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { arrKind, _isBassArr } from '../src/instrument.js'; +import { isKeysArr, viewFor } from '../src/keys.js'; +import { seedState, setRollView, trackHooks } from './_history_env.mjs'; +import { editorSetArrangementType } from '../src/arrangement.js'; + +const src = fs.readFileSync(new URL('../src/arrangement.js', import.meta.url), 'utf8'); +function extractClass(name) { + const start = src.indexOf('class ' + name); + assert.ok(start >= 0, `class ${name} must exist in src/arrangement.js`); + const open = src.indexOf('{', start); + let depth = 0; + for (let i = open; i < src.length; i++) { + if (src[i] === '{') depth++; + else if (src[i] === '}' && --depth === 0) return src.slice(start, i + 1); + } + throw new Error(`unbalanced braces extracting ${name}`); +} + +let pass = 0, fail = 0; +const t = (name, fn) => { + try { fn(); pass++; console.log(' ok ' + name); } + catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } +}; + +// EditHistory closes over the REAL S; the sliced command must share it (same +// pattern as rename_part.test.mjs). Inject the host stubs the command calls. +function makeEnv(arrangements) { + const S = seedState({ arrangements }); + const calls = { selector: 0, lane: 0 }; + const env = new Function( + 'S', 'host', + '"use strict";' + extractClass('SetArrangementTypeCmd') + + '\nreturn { SetArrangementTypeCmd };', + )(S, { + updateArrangementSelector: () => { calls.selector++; }, + resizeForLaneChange: () => { calls.lane++; }, + }); + trackHooks(); + return { ...env, S, calls, history: new EditHistory() }; +} + +t('authoring type=guitar rescues a fretted chart stamped type=piano (arrKind/isKeysArr/viewFor → guitar/string)', () => { + // The real bug: an older save word-boundary-inferred "Electric Piano" and + // stamped `type: piano` into the manifest. This PR honors that stamped type, + // so the fretted chart now loads piano-locked with no way out — until this + // control overrides it. + const { S, history, SetArrangementTypeCmd, calls } = makeEnv( + [{ id: 'a1', name: 'Electric Piano', type: 'piano', notes: [], tuning: [40, 45, 50, 55, 59, 64] }]); + assert.strictEqual(arrKind(S.arrangements[0]), 'keys', 'stamped type=piano loads it keys'); + assert.strictEqual(viewFor(S.arrangements[0]), 'piano', 'so it opens piano-locked'); + history.exec(new SetArrangementTypeCmd(0, 'guitar')); + assert.strictEqual(S.arrangements[0].type, 'guitar'); + assert.strictEqual(arrKind(S.arrangements[0]), 'guitar', 'authored type wins over the keys name'); + assert.strictEqual(isKeysArr(), false, 'no longer keys-locked'); + assert.strictEqual(viewFor(S.arrangements[0]), 'string', 'now opens in string view'); + assert.ok(calls.selector >= 1, 'selector refreshed so the control follows'); + assert.ok(calls.lane >= 1, 'lane metrics rebuilt in place'); +}); + +t('authoring type=bass wins over a keys-word name for the bass predicate', () => { + const { S, history, SetArrangementTypeCmd } = makeEnv( + [{ id: 'a1', name: 'Grand Piano', notes: [], tuning: [28, 33, 38, 43] }]); + assert.strictEqual(_isBassArr(S.arrangements[0]), false, 'name is not /bass/'); + history.exec(new SetArrangementTypeCmd(0, 'bass')); + assert.strictEqual(_isBassArr(S.arrangements[0]), true, 'authored bass wins'); + assert.strictEqual(arrKind(S.arrangements[0]), 'bass'); +}); + +t('the type set is undoable — undo clears an authored-onto-untyped type, redo re-applies', () => { + // Untyped, keys-PREFIX name → keys by name inference (the fallback path). + const { S, history, SetArrangementTypeCmd } = makeEnv( + [{ id: 'a1', name: 'Piano', notes: [] }]); + assert.strictEqual(S.arrangements[0].type, undefined, 'starts untyped'); + history.exec(new SetArrangementTypeCmd(0, 'guitar')); + assert.strictEqual(arrKind(S.arrangements[0]), 'guitar'); + history.doUndo(); + assert.strictEqual(S.arrangements[0].type, undefined, 'undo removes the authored type'); + assert.strictEqual(arrKind(S.arrangements[0]), 'keys', 'back to name inference'); + history.doRedo(); + assert.strictEqual(S.arrangements[0].type, 'guitar', 'redo re-applies'); +}); + +t('undo restores a PRIOR authored type (keys → guitar → undo → keys), never deletes it', () => { + const { S, history, SetArrangementTypeCmd } = makeEnv( + [{ id: 'a1', name: 'Lead', type: 'keys', notes: [] }]); + assert.strictEqual(arrKind(S.arrangements[0]), 'keys'); + history.exec(new SetArrangementTypeCmd(0, 'guitar')); + assert.strictEqual(arrKind(S.arrangements[0]), 'guitar'); + history.doUndo(); + assert.strictEqual(S.arrangements[0].type, 'keys', 'prior authored type restored, not deleted'); +}); + +t('the escape hatch works even when the fretted part is shown READ-ONLY in the roll (lock opt-out)', () => { + // A fretted part with a non-keys name, manually flipped into the piano roll, + // is read-only (_rollReadOnly). The type control is still shown there — and + // is exactly the escape hatch that state needs. A type set writes no note, + // so the note-write lock must not swallow it. Pre-fix: SetArrangementTypeCmd + // carried no lock opt-out, so _locked() blocked it and arr.type never moved. + const { S, history, SetArrangementTypeCmd } = makeEnv( + [{ id: 'a1', name: 'Rhythm', notes: [], tuning: [40, 45, 50, 55, 59, 64] }]); + setRollView(true); // fretted part → piano roll → _rollReadOnly() === true + assert.strictEqual(arrKind(S.arrangements[0]), 'guitar', 'starts fretted'); + history.exec(new SetArrangementTypeCmd(0, 'bass')); + assert.strictEqual(S.arrangements[0].type, 'bass', 'type set applied despite the read-only roll'); + assert.strictEqual(arrKind(S.arrangements[0]), 'bass', 'reader honors it'); + history.doUndo(); + assert.strictEqual(S.arrangements[0].type, undefined, 'undo also passes the lock'); +}); + +t('picking Keys authors the CANONICAL spec type "piano", not the read-only alias "keys"', () => { + // Spec §5.2 spells the keyboard type "piano"; "keys" is only a READ alias. + // The backend persists arr.type verbatim, so the control must WRITE "piano" + // or a corrected keys track lands in the manifest with a spelling other + // consumers (Keys Highway 3D, Staff View) keyed on "piano" won't recognize. + // Pre-fix: editorSetArrangementType passed the raw "keys" option value through. + const S = seedState({ + arrangements: [{ id: 'a1', name: 'Rhythm', notes: [], tuning: [40, 45, 50, 55, 59, 64] }], + }); + S.history = new EditHistory(); + trackHooks(); + editorSetArrangementType('keys'); + assert.strictEqual(S.arrangements[0].type, 'piano', 'writes the canonical spec spelling'); + assert.strictEqual(arrKind(S.arrangements[0]), 'keys', 'still resolves to the keys kind'); + // guitar/bass are already canonical spellings — verify they pass through as-is. + editorSetArrangementType('bass'); + assert.strictEqual(S.arrangements[0].type, 'bass', 'bass is its own canonical spelling'); +}); + +t('picking the currently-INFERRED kind AUTHORS it (frees the rename guard), not a no-op', () => { + // An untyped "Bass" track infers bass by NAME. The rename guard refuses to + // rename an untyped track to a name that changes its inferred kind + // ("Bass" → "Low End" would read guitar), and only a typed track escapes + // that. So stamping the inferred kind is the intended unlock — the old + // guard (kind === arrKind, effective) wrongly no-op'd it because the name + // already inferred bass, trapping the workflow. + const S = seedState({ + arrangements: [{ id: 'a1', name: 'Bass', notes: [], tuning: [28, 33, 38, 43] }], + }); + S.history = new EditHistory(); + trackHooks(); + assert.strictEqual(S.arrangements[0].type, undefined, 'starts untyped (name-inferred bass)'); + assert.strictEqual(arrKind(S.arrangements[0]), 'bass', 'effective kind already bass by name'); + editorSetArrangementType('bass'); + assert.strictEqual(S.arrangements[0].type, 'bass', 'authored the inferred kind so identity is now DATA'); + // Re-picking the SAME authored kind is a genuine no-op (no second history entry). + const undoLen = S.history.undo.length; + editorSetArrangementType('bass'); + assert.strictEqual(S.history.undo.length, undoLen, 're-picking an already-authored kind costs nothing'); +}); + +console.log(`\n${pass} passed, ${fail} failed`); +if (fail) process.exit(1); diff --git a/tests/create_save_routing.test.mjs b/tests/create_save_routing.test.mjs index 86401027..058cd439 100644 --- a/tests/create_save_routing.test.mjs +++ b/tests/create_save_routing.test.mjs @@ -71,6 +71,24 @@ await t('create-mode saveCDLC routes to /build (never /save) and reports durable 'a build IS the durable save — the dirty flag clears so the close guard stays quiet'); }); +await t('create-mode build ships an authored arr.type (a re-typed track is not re-inferred from its name)', async () => { + seedCreateSession(); + // A fretted part the user re-typed to guitar via the new selector, whose NAME + // still reads keys. Build must carry the authored type or /build re-infers + // "piano" from the name and the correction is lost on the first save. + S.arrangements[0].name = 'Electric Piano'; + S.arrangements[0].type = 'guitar'; + let body = null; + globalThis.fetch = async (url, opts) => { + body = JSON.parse(opts.body); + return { json: async () => ({ success: true, filename: 'x.feedpak' }) }; + }; + await saveCDLC(); + assert.ok(Array.isArray(body.arrangements), 'build ships the arrangement snapshot'); + assert.strictEqual(body.arrangements[0].type, 'guitar', + 'the authored type rides the build payload (else /build re-infers keys from the name)'); +}); + await t('a failed build reports save failure and keeps the session dirty', async () => { seedCreateSession(); globalThis.fetch = async () => ({ json: async () => ({ error: 'DLC folder not configured' }) }); From 1f428708f3fe63220a2f3a450d0abdefbd745268 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Mon, 20 Jul 2026 23:24:52 +0200 Subject: [PATCH 15/18] fix(editor): persist the primary drum part's id so a promoted primary survives reload The primary drum part's id was regenerated positionally as "drums" instead of round-tripping. After deleting the original primary, the promoted survivor (e.g. drums-2) was re-saved with alias id "drums" while its stem link and tree placement stayed keyed by drums-2, so _trackSessionNormalizePure dropped them on reload (chart data survived; the stem pairing + folder placement were lost). Make the id follow parts[0].id end to end: ship body.drum_tab_id on save; add _primary_drum_alias_id() (filename-safe, defaults "drums") so routes.py writes the alias entry id from it and surfaces it on load; syncDrumArrangement honors a non-colliding persisted id. Legacy single-drum packs round-trip byte-identically (no drum_tab_id -> primary stays "drums"). Adds a JS load-seam round-trip test and a backend alias-id test (closes the untested save/reload seam). Co-Authored-By: Claude Opus 4.8 (1M context) --- routes.py | 50 +++++++++++- src/drum-arrangement.js | 11 ++- src/file-ops.js | 11 ++- tests/drum_primary_id_roundtrip.test.mjs | 99 ++++++++++++++++++++++++ tests/test_drum_parts.py | 24 +++++- 5 files changed, 187 insertions(+), 8 deletions(-) create mode 100644 tests/drum_primary_id_roundtrip.test.mjs diff --git a/routes.py b/routes.py index 123afa72..9bed3e58 100644 --- a/routes.py +++ b/routes.py @@ -211,6 +211,27 @@ def _plugin_version() -> str: _DRUM_TAB_ABSENT = object() +def _primary_drum_alias_id(raw): + """The id under which the PRIMARY drum part's song-level ``drum_tab`` + alias entry is persisted. + + The frontend sends the promoted primary's ACTUAL id (``parts[0].id``) as + ``drum_tab_id`` so that deleting the original primary and promoting a + survivor (e.g. ``drums-2``) round-trips the part's identity: the alias + entry keeps that id, so on reload the arrangement returns under the same + id its ``editor_stem_links`` / track-session tree rows are keyed by (they + would otherwise be dropped as orphans). Sanitized to the same filename-safe + charset as the extra parts; falls back to the legacy ``"drums"`` when the + field is absent/blank — a legacy single-drum pack (and an old client that + doesn't send the field) stays byte-identical, and old readers still find + the song-level ``drum_tab``. + + Module-level so pytest can reach it. + """ + pid = re.sub(r"[^a-z0-9_-]+", "-", str(raw or "").strip().lower()).strip("-_") + return pid or "drums" + + def _drum_arrs_to_drum_tab(drum_arrs, out_unmapped=None): """Fold drum arrangements into a drum_tab. @@ -4545,6 +4566,7 @@ def _safe_stem_path(stem_entry: dict) -> "Path | None": # above; never load it twice. A malformed part file is skipped, # never fatal (a reader must not crash on a bad side-file). _primary_rel = loaded.manifest.get("drum_tab") + _primary_id = "" _drum_parts = [] _seen_part_rels: set = set() for _entry in (loaded.manifest.get("arrangements", []) or []): @@ -4554,7 +4576,16 @@ def _safe_stem_path(stem_entry: dict) -> "Path | None": if not isinstance(_rel, str) or not _rel.strip(): continue _rel = _rel.strip() - if _rel == _primary_rel or _rel in _seen_part_rels: + if _rel == _primary_rel: + # The alias entry for the song-level drum_tab IS the + # primary — never load it as an extra. Surface its id so + # the frontend re-materializes the primary under the same + # id (a promoted, non-"drums" primary keeps its stem + # links / tree rows on reload). + if not _primary_id: + _primary_id = str(_entry.get("id") or "") + continue + if _rel in _seen_part_rels: continue _seen_part_rels.add(_rel) _src = Path(loaded.source_dir).resolve() @@ -4578,6 +4609,13 @@ def _safe_stem_path(stem_entry: dict) -> "Path | None": }) if _drum_parts: result["drum_parts"] = _drum_parts + # The primary's persisted id (from its alias entry) so the + # frontend re-materializes it under the same id it saved — only + # when there IS a primary drum_tab and an explicit alias id. A + # legacy pack with no alias entry omits it → the frontend keeps + # the default "drums", byte-identical. + if _loaded_drum_tab is not None and _primary_id: + result["drum_tab_id"] = _primary_id # Carry the manifest-derived arrangement id list onto each # arrangement so the frontend can round-trip it back to us. # Use a single `used_ids` set when generating fallback ids so two @@ -5236,10 +5274,14 @@ def _build_wire(arr_dict, is_first): # The primary's alias entry — same file the song-level # `drum_tab:` key names, so a reader that predates the # spec stays with one drum and a spec reader must not - # load it twice. - used_part_ids.add("drums") + # load it twice. Its id FOLLOWS the incoming primary's id + # (the frontend's `drum_tab_id`, = parts[0].id) so a + # promoted primary round-trips its identity; legacy / + # old-client saves fall back to "drums". + _primary_id = _primary_drum_alias_id(data.get("drum_tab_id")) + used_part_ids.add(_primary_id) new_drum_entries.append({ - "id": "drums", + "id": _primary_id, "name": str(drum_tab_payload.get("name") or "Drums")[:120], "type": "drums", "drum_tab": "drum_tab.json", diff --git a/src/drum-arrangement.js b/src/drum-arrangement.js index e2d5ef82..f5a287cd 100644 --- a/src/drum-arrangement.js +++ b/src/drum-arrangement.js @@ -155,7 +155,7 @@ function _nextDrumArrId(arrangements) { // arrangement itself), so with several parts this is a careful no-op that // leaves the non-active parts alone. Returns the arrangement holding // `S.drumTab`, or null when there are no drums. -export function syncDrumArrangement(S) { +export function syncDrumArrangement(S, primaryId) { if (!S || !Array.isArray(S.arrangements)) return null; const all = drumArrangements(S.arrangements); const tab = S.drumTab; @@ -182,7 +182,14 @@ export function syncDrumArrangement(S) { all[0].name = _tabName(tab); return all[0]; } - const arr = _drumArrShell(_nextDrumArrId(S.arrangements), tab); + // Materialize the primary under its PERSISTED id when the loader supplied + // one (feedpak-spec 1.17.0 alias entry id) and it doesn't collide — so a + // promoted, non-"drums" primary keeps the id its stem links / tree rows are + // keyed by. No/blank/colliding id → the legacy "drums"-first default. + const wantId = (primaryId !== undefined && primaryId !== null) ? String(primaryId).trim() : ''; + const usedNow = new Set(S.arrangements.map(a => (a && a.id !== undefined && a.id !== null) ? String(a.id) : '')); + const id = (wantId && !usedNow.has(wantId)) ? wantId : _nextDrumArrId(S.arrangements); + const arr = _drumArrShell(id, tab); S.arrangements.push(arr); return arr; } diff --git a/src/file-ops.js b/src/file-ops.js index ee96aa26..f4576231 100644 --- a/src/file-ops.js +++ b/src/file-ops.js @@ -222,7 +222,10 @@ export async function loadCDLC(filename, options = {}) { // Migrate the song-level drum tab into a `type:"drums"` arrangement // (S.arrangements comes from data.arrangements above; drums are appended // last so no arr: shifts). The arrangement's payload IS S.drumTab. - syncDrumArrangement(S); + // Pass the persisted primary id (the alias entry id) so a promoted, + // non-"drums" primary re-materializes under the same id its stem links + // / tree rows are keyed by; absent → the legacy "drums" default. + syncDrumArrangement(S, data.drum_tab_id); // EXTRA drum parts (a song can hold several): the backend reads them // from the manifest's `type:"drums"` arrangement entries (per- // arrangement `drum_tab` pointers, feedpak-spec 1.17.0) and ships @@ -751,6 +754,12 @@ export function _buildSaveBody(forceFullSnapshot) { // Legacy unmaterialized tab (create-mode compose) has no drums // arrangement — its S.drumTab IS the primary, as before. body.drum_tab = parts.length ? (parts[0].drumTab ?? null) : S.drumTab; + // The primary's durable id (parts[0].id) — the backend persists the + // alias entry under it so a promoted primary (e.g. "drums-2" after the + // original "drums" was deleted) round-trips its identity and its stem + // links / tree rows survive reload. Blank for a legacy unmaterialized + // tab → the backend defaults to "drums". + body.drum_tab_id = parts.length ? String(parts[0].id || '') : ''; body.drum_parts = parts.slice(1).map(a => ({ id: String(a.id || ''), name: String(a.name || 'Drums').slice(0, 120), diff --git a/tests/drum_primary_id_roundtrip.test.mjs b/tests/drum_primary_id_roundtrip.test.mjs new file mode 100644 index 00000000..78cc4d31 --- /dev/null +++ b/tests/drum_primary_id_roundtrip.test.mjs @@ -0,0 +1,99 @@ +/* + * MED regression: promoting a non-original primary must not silently change + * its persisted id on reload (which orphans its stem link + tree placement). + * + * feedpak-spec 1.17.0: the PRIMARY drum part persists as the song-level + * `drum_tab` alias entry. Before the fix that alias entry was HARDCODED + * id "drums" (routes.py) and the frontend re-materialized the primary as + * "drums" (`_nextDrumArrId`) regardless of what it saved. So after the user + * deletes the original primary and a survivor "drums-2" is promoted to + * parts[0], a save→reload returned the arrangement as id "drums" while + * `editor_stem_links["drums-2"]` and the track-session tree row for + * "drums-2" no longer matched → `_trackSessionNormalizePure` dropped them. + * A stem pairing and a custom folder placement were LOST across the round + * trip. + * + * The fix makes the persisted primary id FOLLOW parts[0].id end to end: the + * save body ships `drum_tab_id`, the backend writes the alias entry under it, + * the load surfaces it, and `syncDrumArrangement(S, primaryId)` re-materializes + * the primary under it. This test drives the LOAD-side seam with the REAL + * pure functions and simulates the backend persist that ties them together; + * it FAILS on unfixed code (the primary comes back as "drums"). + * + * Run: node tests/drum_primary_id_roundtrip.test.mjs + */ +import assert from 'node:assert'; + +import { syncDrumArrangement, adoptDrumParts, findDrumArrangement } from '../src/drum-arrangement.js'; +import { _trackSessionTargetsPure, _trackSessionNormalizePure } from '../src/track-session.js'; + +let pass = 0, fail = 0; +function t(name, fn) { + try { fn(); pass++; console.log(' ok ' + name); } + catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } +} + +const drumTab = (name = 'Drums (Live)') => ({ version: 1, name, kit: [], hits: [{ t: 0, p: 'kick' }] }); +const gtr = (name = 'Lead') => ({ id: name.toLowerCase(), name, notes: [], chords: [] }); + +// The wire the backend answers with AFTER a delete-primary → promote → save. +// The original primary "drums" was deleted; "drums-2" was promoted to parts[0] +// and shipped as the song-level drum_tab whose alias entry the backend now +// persists under the incoming `drum_tab_id` (= parts[0].id). A stem link and a +// custom folder placement are keyed by that promoted id. +const PROMOTED_ID = 'drums-2'; +const reloadWire = () => ({ + drum_tab: drumTab(), + drum_tab_id: PROMOTED_ID, // the fix: persisted primary id follows parts[0].id + drum_parts: [], // only the promoted part remains + stem_links: { [PROMOTED_ID]: 'stem:kit' }, + track_session: { + tracks: [ + { id: 'folder:1', type: 'folder', name: 'My Folder', parentId: '' }, + // the promoted part's tree row, placed inside the custom folder + { id: 'transcription:' + PROMOTED_ID, type: 'transcription', targetId: PROMOTED_ID, parentId: 'folder:1' }, + ], + }, +}); + +// Mirror the file-ops.js load materialization (drums appended after the +// pitched arrangements; the persisted primary id threaded into +// syncDrumArrangement, then the extras adopted). +function loadDrums(data) { + const S = { arrangements: [gtr('Lead')], drumTab: null, stemLinks: {} }; + S.stemLinks = (data.stem_links && typeof data.stem_links === 'object') ? data.stem_links : {}; + S.drumTab = data.drum_tab ?? null; + syncDrumArrangement(S, data.drum_tab_id); + adoptDrumParts(S, data.drum_parts); + return S; +} + +t('a promoted primary re-materializes under its PERSISTED id (not the hardcoded "drums")', () => { + const S = loadDrums(reloadWire()); + const primary = findDrumArrangement(S.arrangements); + assert.ok(primary, 'the primary drums arrangement exists'); + assert.strictEqual(primary.id, PROMOTED_ID, + 'the primary keeps the id it saved — a promoted part does not silently become "drums"'); +}); + +t('the stem link keyed by the promoted id survives reload (a matching target exists)', () => { + const S = loadDrums(reloadWire()); + const targets = _trackSessionTargetsPure(S.arrangements, S.drumTab); + const linked = Object.keys(S.stemLinks); + assert.deepStrictEqual(linked, [PROMOTED_ID]); + assert.ok(targets.some(x => x.id === PROMOTED_ID), + 'a chart-track target matches the stem link key — the pairing is not orphaned'); +}); + +t('the custom folder placement of the promoted part survives normalization', () => { + const wire = reloadWire(); + const S = loadDrums(wire); + const model = _trackSessionNormalizePure(wire.track_session, [], S.arrangements, S.drumTab); + const row = model.tracks.find(r => r.type === 'transcription' && r.targetId === PROMOTED_ID); + assert.ok(row, 'the promoted part still has a tree row after normalization'); + assert.strictEqual(row.parentId, 'folder:1', + 'and it stays inside the custom folder — placement is not reset to the tree root'); +}); + +console.log(`\n${pass} passed, ${fail} failed`); +process.exit(fail ? 1 : 0); diff --git a/tests/test_drum_parts.py b/tests/test_drum_parts.py index e8abafc3..4e5e2e06 100644 --- a/tests/test_drum_parts.py +++ b/tests/test_drum_parts.py @@ -17,7 +17,29 @@ mutated. """ -from routes import _is_drum_pointer_entry, _sanitize_extra_drum_tab +from routes import ( + _is_drum_pointer_entry, + _primary_drum_alias_id, + _sanitize_extra_drum_tab, +) + + +def test_primary_alias_id_follows_the_incoming_primary_id(): + # The MED fix: the song-level drum_tab alias entry is persisted under the + # promoted primary's actual id (parts[0].id), NOT a hardcoded "drums" — so + # deleting the original primary and promoting a survivor round-trips its + # id, keeping its stem links / tree rows matched on reload. + assert _primary_drum_alias_id("drums-2") == "drums-2" + assert _primary_drum_alias_id("drums_live") == "drums_live" + # Legacy / old-client back-compat: absent or blank → the legacy "drums", + # so a single-drum pack stays byte-identical and old readers still match. + assert _primary_drum_alias_id(None) == "drums" + assert _primary_drum_alias_id("") == "drums" + assert _primary_drum_alias_id(" ") == "drums" + # Sanitized to the same filename-safe charset the extra parts use. + assert _primary_drum_alias_id("Drums (Live)") == "drums-live" + assert _primary_drum_alias_id("../evil") == "evil" + assert _primary_drum_alias_id("!!!") == "drums" def test_pointer_entry_is_type_drums_without_a_note_file(): From 76d37ef13c34f290cef9c3115703b4192eedd25c Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Mon, 20 Jul 2026 16:50:34 -0500 Subject: [PATCH 16/18] 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 | 81 +++++++++++++++++++++++++- 8 files changed, 303 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc14193d..92bbbdac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,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. + - **A song can hold several drum parts.** The payoff of the drums-as-arrangement arc below: with drums already present, **+ Track ▸ Drums** adds *another* drum part (a second drummer, an aux-percussion layer), and a GP/MIDI drum import is diff --git a/routes.py b/routes.py index 123afa72..aca28287 100644 --- a/routes.py +++ b/routes.py @@ -318,6 +318,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 @@ -8517,6 +8566,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. @@ -8554,6 +8629,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 ""), @@ -8664,6 +8740,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, @@ -8954,6 +9031,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 127e7f8f..2fa3986f 100644 --- a/src/arrangement.js +++ b/src/arrangement.js @@ -258,13 +258,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)); } @@ -276,13 +277,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 fcb3f3ac..c555eb4e 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, @@ -2667,9 +2686,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 86401027..970bad3d 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; @@ -140,5 +141,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 e8abafc3..34eb23a9 100644 --- a/tests/test_drum_parts.py +++ b/tests/test_drum_parts.py @@ -15,9 +15,18 @@ 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. """ -from routes import _is_drum_pointer_entry, _sanitize_extra_drum_tab +import json + +from routes import ( + _create_build_drum_entries, + _is_drum_pointer_entry, + _sanitize_extra_drum_tab, +) def test_pointer_entry_is_type_drums_without_a_note_file(): @@ -76,3 +85,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 eaf318e61590f0938a24559efb8ae5a114362449 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Mon, 20 Jul 2026 22:10:44 -0500 Subject: [PATCH 17/18] Hide string controls for non-fretted tracks --- src/instrument.js | 5 +++++ src/main.js | 4 ++-- tests/instrument_type.test.mjs | 10 +++++++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/instrument.js b/src/instrument.js index e9ec9733..bc2638f8 100644 --- a/src/instrument.js +++ b/src/instrument.js @@ -73,6 +73,11 @@ export function arrKind(arr) { return _arrTypeKind(arr) || _arrKindFromName(arr && arr.name); } +// True only for instrument kinds that have strings/tuning to edit. +export function _isFrettedKind(kind) { + return kind === 'guitar' || kind === 'bass'; +} + // Bass predicate — type-authoritative, but its NAME fallback stays the INDEPENDENT // `/bass/` test, NOT `arrKind === 'bass'`. Bass and keys are independent facets in // the legacy inference ("Synth Bass" is bass for string-count AND keys for the diff --git a/src/main.js b/src/main.js index d2908cea..4b6e70c7 100644 --- a/src/main.js +++ b/src/main.js @@ -188,7 +188,7 @@ import { _laneClipActive, applyLaneScrollBounds, drawLaneScrollbar, laneBandTop import { _rollLockNotice, _rollMidiForNote, _rollPitchCtx, _rollReadOnly, editorKeyNoteNames, isKeysMode, midiToNote, updatePianoRange } from './keys.js'; -import { arrKind } from './instrument.js'; +import { _isFrettedKind, arrKind } from './instrument.js'; import { _restoreSuggestedMarks, _saveSuggestedMarks, _suggestedCount, chords, notes @@ -1071,7 +1071,7 @@ function updateArrangementSelector() { if (stringsBtn) { const active = S.arrangements[S.currentArr]; const activeKind = active && arrKind(active); - const stringsMode = !!active && activeKind !== 'keys' && activeKind !== 'drums'; + const stringsMode = !!active && _isFrettedKind(activeKind); stringsBtn.classList.toggle('hidden', !S.sessionId || !stringsMode); } diff --git a/tests/instrument_type.test.mjs b/tests/instrument_type.test.mjs index 3c16c4bd..3e4b903c 100644 --- a/tests/instrument_type.test.mjs +++ b/tests/instrument_type.test.mjs @@ -18,7 +18,7 @@ */ import assert from 'node:assert'; -import { _typeKind, _arrTypeKind, _arrKindFromName, arrKind } from '../src/instrument.js'; +import { _typeKind, _arrTypeKind, _arrKindFromName, _isFrettedKind, arrKind } from '../src/instrument.js'; import { isKeysArr, KEYS_PATTERN, _rollPitchCtxFor } from '../src/keys.js'; import { _seedExtendedStringsFromTuning, _stringCountFor } from '../src/lanes.js'; import { _isBassArr } from '../src/instrument.js'; @@ -130,6 +130,14 @@ t('arrKind: authored type wins, name inference is the fallback', () => { assert.strictEqual(arrKind(null), 'guitar', 'no arr → guitar default'); }); +t('_isFrettedKind gates string controls to guitar and bass only', () => { + assert.strictEqual(_isFrettedKind('guitar'), true); + assert.strictEqual(_isFrettedKind('bass'), true); + for (const kind of ['keys', 'drums', 'vocals', null, undefined]) { + assert.strictEqual(_isFrettedKind(kind), false, `${kind} has no string controls`); + } +}); + // ── the Tracks-view kind badge ──────────────────────────────────────── t('_trackKindBadgePure: audio shows the layer, transcription shows the instrument', () => { assert.deepStrictEqual(_trackKindBadgePure({ type: 'audio', sourceKind: 'master' }, []), ['MIX', 'Master mix']); From a79c65354ea61e94833fdd25d0cb446f7390e056 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Mon, 20 Jul 2026 22:38:06 -0500 Subject: [PATCH 18/18] Preserve materialized drum mixer strips --- src/mixer-panel.js | 4 ---- tests/mixer_panel.test.mjs | 15 --------------- 2 files changed, 19 deletions(-) diff --git a/src/mixer-panel.js b/src/mixer-panel.js index 081b9e74..4f7dee6b 100644 --- a/src/mixer-panel.js +++ b/src/mixer-panel.js @@ -29,7 +29,6 @@ import { drumArrangementIndex } from './drum-arrangement.js'; import { host } from './host.js'; import { S, editGen } from './state.js'; import { _editorEscHtml, setStatus } from './ui.js'; -import { isDrumArrangement } from './drum-arrangement.js'; /* @pure:mixer-panel:start */ // One strip per part, keyed the way S.currentArr addresses parts (by index). @@ -59,9 +58,6 @@ export function _mixerPartsPure(arrangements, drumTab, stems, removedSourceIds, parts.push({ key: 'audio:' + id, name: stem.name || id, kind: 'audio' }); } (arrangements || []).forEach((arr, i) => { - // The drums arrangement gets its OWN 'drums' strip below (from drumTab), - // not an 'arr:' strip — skip it here so drums don't show twice. - if (isDrumArrangement(arr)) return; parts.push({ key: 'arr:' + i, name: (arr && arr.name) || 'Track ' + (i + 1), diff --git a/tests/mixer_panel.test.mjs b/tests/mixer_panel.test.mjs index ff3ab4ee..5cd17e5b 100644 --- a/tests/mixer_panel.test.mjs +++ b/tests/mixer_panel.test.mjs @@ -98,21 +98,6 @@ t('one strip per arrangement, keyed by index; the drums arrangement is an ordina assert.deepStrictEqual(_mixerPartsPure(null, null), []); }); -// #336 regression: the drums arrangement (materialized into S.arrangements[]) -// must NOT get an 'arr:' strip — it already gets the dedicated 'drums' -// strip from drumTab. Pre-fix it got both → two Drums strips after reload. -t('the drums arrangement is not double-listed — exactly one Drums strip', () => { - const arrs = [{ name: 'Lead' }, { name: 'Bass' }, { name: 'Drums', type: 'drums' }]; - const drumTab = { hits: [{ t: 0, p: 'kick' }] }; - const parts = _mixerPartsPure(arrs, drumTab); - assert.deepStrictEqual(parts, [ - { key: 'arr:0', name: 'Lead' }, - { key: 'arr:1', name: 'Bass' }, - { key: 'drums', name: 'Drums' }, - ], 'the type:"drums" arrangement gets no arr:2 strip; only the drums strip'); - assert.strictEqual(parts.filter(p => p.name === 'Drums').length, 1, 'exactly one Drums strip'); - assert.ok(!parts.some(p => p.key === 'arr:2'), 'no arr: strip for the drums arrangement'); -}); t('strip state defaults to audible unity; volume clamps into [0, 110] (+10 dB ceiling)', () => { assert.deepStrictEqual(_mixerPartStatePure({}, 'arr:0'), { vol: 100, mute: false, solo: false });