fix(editor): stop dropping manifest type/centOffset on save + infer-once type - #101
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds manifest-entry merging and infer-once arrangement type stamping in the save route, plus tests and changelog updates. Existing manifest fields and unknown keys are preserved during full-snapshot saves. ChangesManifest Merge and Type Inference
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_manifest_type_preserve.py (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead no-op loop — flagged by Ruff (B007).
The
for name in (...): passloop does nothing (the loop variable is never used, and the actual assertions below are hardcoded literals, not driven by the loop). This is leftover cruft from refactoring the earlier keys-family style loop pattern into explicit asserts; remove it.🧹 Proposed cleanup
def test_infer_bass_anywhere_in_the_name(): - for name in ("Bass", "5-string Bass", "bass (DI)", "Synth Bass"): - # NB: "Synth Bass" hits the keys prefix first — see next assert. - pass + # NB: "Synth Bass" hits the keys prefix first — see the assert below. assert _infer_arrangement_type("Bass") == "bass" assert _infer_arrangement_type("5-string Bass") == "bass" assert _infer_arrangement_type("bass (DI)") == "bass" - # Keys prefix wins for synth-family names (the piano-roll pathway). assert _infer_arrangement_type("Synth Bass") == "piano"As per static analysis hints, Ruff flags "Loop control variable
namenot used within loop body" (B007) at line 28.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_manifest_type_preserve.py` around lines 27 - 36, Remove the dead no-op loop in test_infer_bass_anywhere_in_the_name, since the loop variable name is unused and Ruff flags it as B007; keep the explicit _infer_arrangement_type assertions only, and delete the leftover for name in (...) : pass block so the test reads as direct coverage for the Bass and Synth Bass cases.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@routes.py`:
- Around line 178-205: _infer_arrangement_type currently checks _TYPE_BASS_RE
before _TYPE_SKIP_RE, so mixed names like “Bass Vocals” can be misclassified as
bass instead of staying untyped. Reorder the guards in _infer_arrangement_type
so the skip regex is evaluated before the bass regex, while keeping
_TYPE_KEYS_RE first so “Synth Bass” still maps to piano. Add or update a test in
tests/test_manifest_type_preserve.py covering a name that combines bass and skip
cues and verifies it returns an empty type.
---
Nitpick comments:
In `@tests/test_manifest_type_preserve.py`:
- Around line 27-36: Remove the dead no-op loop in
test_infer_bass_anywhere_in_the_name, since the loop variable name is unused and
Ruff flags it as B007; keep the explicit _infer_arrangement_type assertions
only, and delete the leftover for name in (...) : pass block so the test reads
as direct coverage for the Bass and Synth Bass cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4399c7ed-70ab-4733-b7c2-ad784abe194b
📒 Files selected for processing (3)
CHANGELOG.mdroutes.pytests/test_manifest_type_preserve.py
Comment on lines
+178
to
+205
| _TYPE_KEYS_RE = re.compile(r"^(keys|piano|keyboard|synth)", re.I) | ||
| _TYPE_BASS_RE = re.compile(r"bass", re.I) | ||
| _TYPE_SKIP_RE = re.compile(r"vocal|voice|sing|drum", re.I) | ||
| _TYPE_GUITAR_RE = re.compile(r"guitar|lead|rhythm|combo|acoustic|electric", re.I) | ||
|
|
||
|
|
||
| def _infer_arrangement_type(name) -> str: | ||
| """Manifest ``type`` (feedpak-spec §5.2) inferred from a display name. | ||
|
|
||
| The infer-once migration: callers write this only when an entry carries | ||
| no ``type`` yet, never clobbering an authored value. Conservative on | ||
| purpose — a name that doesn't clearly identify an instrument returns "" | ||
| and the entry stays untyped, because writing a WRONG type is worse than | ||
| writing none. Vocals/drums names also return "": per the spec those are | ||
| side-file mechanisms, not arrangement types. | ||
| """ | ||
| n = (name or "").strip() | ||
| if not n: | ||
| return "" | ||
| if _TYPE_KEYS_RE.match(n): | ||
| return "piano" | ||
| if _TYPE_BASS_RE.search(n): | ||
| return "bass" | ||
| if _TYPE_SKIP_RE.search(n): | ||
| return "" | ||
| if _TYPE_GUITAR_RE.search(n): | ||
| return "guitar" | ||
| return "" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Regex precedence lets a "bass"+"drum"/"vocal" name slip past the skip guard.
_infer_arrangement_type checks _TYPE_BASS_RE before _TYPE_SKIP_RE. Both are unanchored .search(), so a name containing both a bass cue and a skip cue (e.g. "Bass Vocals", "Bass/Drums") returns "bass" instead of "", contradicting the function's own documented guarantee that vocals/drums names must stay untyped rather than risk a wrong classification.
♻️ Proposed reorder
n = (name or "").strip()
if not n:
return ""
if _TYPE_KEYS_RE.match(n):
return "piano"
- if _TYPE_BASS_RE.search(n):
- return "bass"
if _TYPE_SKIP_RE.search(n):
return ""
+ if _TYPE_BASS_RE.search(n):
+ return "bass"
if _TYPE_GUITAR_RE.search(n):
return "guitar"
return ""Note: verify this reorder doesn't break the existing "Synth Bass" → "piano" test (keys check stays first, unaffected) and add a case combining bass+skip cues to tests/test_manifest_type_preserve.py.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _TYPE_KEYS_RE = re.compile(r"^(keys|piano|keyboard|synth)", re.I) | |
| _TYPE_BASS_RE = re.compile(r"bass", re.I) | |
| _TYPE_SKIP_RE = re.compile(r"vocal|voice|sing|drum", re.I) | |
| _TYPE_GUITAR_RE = re.compile(r"guitar|lead|rhythm|combo|acoustic|electric", re.I) | |
| def _infer_arrangement_type(name) -> str: | |
| """Manifest ``type`` (feedpak-spec §5.2) inferred from a display name. | |
| The infer-once migration: callers write this only when an entry carries | |
| no ``type`` yet, never clobbering an authored value. Conservative on | |
| purpose — a name that doesn't clearly identify an instrument returns "" | |
| and the entry stays untyped, because writing a WRONG type is worse than | |
| writing none. Vocals/drums names also return "": per the spec those are | |
| side-file mechanisms, not arrangement types. | |
| """ | |
| n = (name or "").strip() | |
| if not n: | |
| return "" | |
| if _TYPE_KEYS_RE.match(n): | |
| return "piano" | |
| if _TYPE_BASS_RE.search(n): | |
| return "bass" | |
| if _TYPE_SKIP_RE.search(n): | |
| return "" | |
| if _TYPE_GUITAR_RE.search(n): | |
| return "guitar" | |
| return "" | |
| _TYPE_KEYS_RE = re.compile(r"^(keys|piano|keyboard|synth)", re.I) | |
| _TYPE_BASS_RE = re.compile(r"bass", re.I) | |
| _TYPE_SKIP_RE = re.compile(r"vocal|voice|sing|drum", re.I) | |
| _TYPE_GUITAR_RE = re.compile(r"guitar|lead|rhythm|combo|acoustic|electric", re.I) | |
| def _infer_arrangement_type(name) -> str: | |
| """Manifest ``type`` (feedpak-spec §5.2) inferred from a display name. | |
| The infer-once migration: callers write this only when an entry carries | |
| no ``type`` yet, never clobbering an authored value. Conservative on | |
| purpose — a name that doesn't clearly identify an instrument returns "" | |
| and the entry stays untyped, because writing a WRONG type is worse than | |
| writing none. Vocals/drums names also return "": per the spec those are | |
| side-file mechanisms, not arrangement types. | |
| """ | |
| n = (name or "").strip() | |
| if not n: | |
| return "" | |
| if _TYPE_KEYS_RE.match(n): | |
| return "piano" | |
| if _TYPE_SKIP_RE.search(n): | |
| return "" | |
| if _TYPE_BASS_RE.search(n): | |
| return "bass" | |
| if _TYPE_GUITAR_RE.search(n): | |
| return "guitar" | |
| return "" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@routes.py` around lines 178 - 205, _infer_arrangement_type currently checks
_TYPE_BASS_RE before _TYPE_SKIP_RE, so mixed names like “Bass Vocals” can be
misclassified as bass instead of staying untyped. Reorder the guards in
_infer_arrangement_type so the skip regex is evaluated before the bass regex,
while keeping _TYPE_KEYS_RE first so “Synth Bass” still maps to piano. Add or
update a test in tests/test_manifest_type_preserve.py covering a name that
combines bass and skip cues and verifies it returns an empty type.
…nce type
The full-snapshot save path rebuilt every manifest arrangement entry
from scratch ({id, name, file, tuning, capo}), silently dropping the
spec fields the editor doesn't author — type (spec 5.2), centOffset,
and any future additive key — on EVERY save. That violates the format's
unknown-key preservation rule (spec 1.2) and would have made the
arrangement type facet impossible to keep.
- New module-level _merge_manifest_entry: rebuilt entries merge ONTO
the existing manifest entry for the same id; editor-owned keys
(id/name/file/tuning/capo) take the fresh values, everything else
survives.
- New _infer_arrangement_type + infer-once stamping in the common save
loop (both save paths): entries with no type get one inferred from
the display name (keys-family -> piano per spec spelling, bass names
-> bass, classic guitar roles -> guitar). Conservative on purpose:
vocals/drums/ambiguous names stay untyped (a wrong type is worse
than none, and vocals/drums are side-file mechanisms per the spec),
and an authored type is never clobbered.
type as the queryable instrument facet — with names as free labels —
is the groundwork for safe track renaming in the Parts view.
Tests: tests/test_manifest_type_preserve.py (7 cases: inference incl.
the conservative refusals, merge preservation of type/centOffset/
notation/unknown keys, editor-key override, no-old-entry passthrough,
input immutability). Full pytest suite: 198 passed, 2 skipped
(pre-existing skips).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
ChrisBeWithYou
force-pushed
the
feat/editor-manifest-type-preserve
branch
from
July 6, 2026 23:20
c22cf2b to
e275d9a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_manifest_type_preserve.py (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove dead loop with no assertions.
The
for name in (...)loop body only contains a comment andpass— it never executes an assertion and the loop variable is unused, as flagged by static analysis (B007). It's dead code left over from drafting the test.🧹 Proposed fix
def test_infer_bass_anywhere_in_the_name(): - for name in ("Bass", "5-string Bass", "bass (DI)", "Synth Bass"): - # NB: "Synth Bass" hits the keys prefix first — see next assert. - pass assert _infer_arrangement_type("Bass") == "bass" assert _infer_arrangement_type("5-string Bass") == "bass" assert _infer_arrangement_type("bass (DI)") == "bass" # Keys prefix wins for synth-family names (the piano-roll pathway). assert _infer_arrangement_type("Synth Bass") == "piano"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_manifest_type_preserve.py` around lines 27 - 36, The test contains dead code in test_infer_bass_anywhere_in_the_name: remove the unused for name in (...) loop and its pass/comment-only body, since it does not assert anything and triggers the B007 warning. Keep the actual assertions for _infer_arrangement_type intact, and if needed move the explanatory comment next to the relevant Synth Bass assertion.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_manifest_type_preserve.py`:
- Around line 27-36: The test contains dead code in
test_infer_bass_anywhere_in_the_name: remove the unused for name in (...) loop
and its pass/comment-only body, since it does not assert anything and triggers
the B007 warning. Keep the actual assertions for _infer_arrangement_type intact,
and if needed move the explanatory comment next to the relevant Synth Bass
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d43555a5-1985-46c0-ac4a-f2aa0392098d
📒 Files selected for processing (3)
CHANGELOG.mdroutes.pytests/test_manifest_type_preserve.py
🚧 Files skipped from review as they are similar to previous changes (1)
- routes.py
Type inference used a prefix-anchored `_TYPE_KEYS_RE = ^(keys|piano| keyboard|synth)` (.match) while the pre-existing keys notation-sidecar detector used `_KEYS_NAME_RE = \b(keys|piano|keyboard|synth)\b` (.search) over the identical keyword set. The two disagreed: "Electric Piano" and "Lead Synth" inferred `guitar`, "Grand Piano" inferred "", yet all three still earned a keys notation sidecar — so the manifest `type` facet and the notation renderer contradicted each other for the same entry. Reuse `_KEYS_NAME_RE.search()` for type inference (moved its definition up beside the type helpers) and drop the now-unused `_TYPE_KEYS_RE`. Branch order is unchanged (keys before bass before guitar), so the intentional "Synth Bass" -> piano behavior is preserved. Tests: drop the dead pass-only loop; add mid-name keys cases (Electric Piano / Grand Piano / Lead Synth / Rhodes Keys -> piano) and a `centOffset: 0` merge-preservation case guarding a truthiness regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 7, 2026
Merged
Merged
byrongamatos
added a commit
that referenced
this pull request
Jul 7, 2026
* feat(editor): undoable part rename, kind-guarded DAW-workspace 2.2b (first half — rename; reorder is a separable follow-up), unblocked by #101: the merge-not-rebuild save keeps type/unknown keys across a rename, and sloppak sessions carry a stable manifest id. - RenameArrangementCmd: captured-index targeting (undo after a switch lands on the right part), exec/rollback refresh the selector, the stable id never changes (view prefs + manifest merge survive). - The hard limit, enforced honestly: the NAME still drives kind inference (KEYS_PATTERN -> piano roll + notation sidecar, /bass/i -> 4-lane layout, /^drums/i -> drum routing), so a rename that would change the inferred instrument is REFUSED with an explanation — silently re-laning a 6-string chart as a bass would strand notes on invisible strings. Cross-kind moves stay "add a new part". - Duplicate names refused case-insensitively (pack name discipline); empty/overlong refused; exact no-op fails silently. - Toolbar pencil button next to remove-arr + registry renamePart. Tests: tests/rename_part.test.js (6) — kind table (incl. the anchored KEYS_PATTERN nuance: "Electric Piano" is NOT a keys name by the layout rules), guard truth table, and the real command round-tripped through EditHistory (selector refresh, id stability, captured-index targeting). Full suite green except pre-existing CRLF section_coverage (#116). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu * feat(editor): review fixes for #124 (part rename) Guard the rename against BOTH name interpreters, not one: the live lane/roll router keys off prefix-anchored KEYS_PATTERN while the save side (routes.py _KEYS_NAME_RE / _TYPE_BASS_RE) keys off word-boundary matches. They disagree on names like "Electric Piano" (save-keys, runtime-guitar) and "Synthwave Lead" (runtime-keys, save-guitar), so a one-facet guard let a rename silently re-lane a chart on save/reload or on the next draw. _renameGuardPure now refuses when either _arrKindPure or _arrSaveKindPure moves; regression tests cover both directions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 8, 2026
DAW-workspace 2.2b (second half; rename shipped as #124). Completes the 2.2b remainders that were unblocked by #101. - New < / > buttons next to the arrangement selector + registry commands movePartEarlier/movePartLater: one-slot moves, per-end disabling so the affordance always tells the truth. - Order persists: sloppak saves ship the CLIENT S.arrangements array as the full snapshot and the manifest merge keys entries by id — verified against _buildSaveBody before building. - A move renumbers arrangement indices, so the undo history RESETS (the remove-arrangement rationale) — which is also why the move itself is not undoable: move it back. Blocked mid-recording (a take pins its arrangement index). Selection cleared; selector rebuilt; currentArr follows the moved part. Tests: tests/reorder_part.test.js (4) — pure target math (ends, degenerate inputs), the real handler over an injected env (object identity through the swap, currentArr follow, history reset, selection clear), the clean-no-op-at-ends case (no gratuitous reset), and the recording block. Full suite green except pre-existing CRLF section_coverage (#116). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
byrongamatos
added a commit
that referenced
this pull request
Jul 8, 2026
) * feat(editor): part reorder — move earlier/later, persisted on save DAW-workspace 2.2b (second half; rename shipped as #124). Completes the 2.2b remainders that were unblocked by #101. - New < / > buttons next to the arrangement selector + registry commands movePartEarlier/movePartLater: one-slot moves, per-end disabling so the affordance always tells the truth. - Order persists: sloppak saves ship the CLIENT S.arrangements array as the full snapshot and the manifest merge keys entries by id — verified against _buildSaveBody before building. - A move renumbers arrangement indices, so the undo history RESETS (the remove-arrangement rationale) — which is also why the move itself is not undoable: move it back. Blocked mid-recording (a take pins its arrangement index). Selection cleared; selector rebuilt; currentArr follows the moved part. Tests: tests/reorder_part.test.js (4) — pure target math (ends, degenerate inputs), the real handler over an injected env (object identity through the swap, currentArr follow, history reset, selection clear), the clean-no-op-at-ends case (no gratuitous reset), and the recording block. Full suite green except pre-existing CRLF section_coverage (#116). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu * feat(editor): review fixes for #125 (part reorder) Gate part-reorder to sloppak sessions. The new order persists only on the full-arrangement snapshot that _buildSaveBody ships for sloppak saves; an archive save writes just the active arrangement keyed by arrangement_index, so a client-side reorder was silently lost on reload and, worse, the stale index re-targeted the moved part into the wrong original slot. Hide the buttons for non-sloppak (matching +Keys/Record) and refuse in the handler so the command-palette/keyboard paths can't bypass the hidden buttons. Regression test: archive sessions refuse the move (fails on pre-fix code). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found while wiring the Parts view's instrument tags: the full-snapshot save path rebuilds every manifest arrangement entry from scratch (
{id, name, file, tuning, capo}) — so spec fields the editor doesn't author (typeper §5.2,centOffset, and any future additive key) are silently dropped on every save, violating the format's unknown-key preservation rule (§1.2)._merge_manifest_entry— rebuilt entries merge onto the existing manifest entry for the same id. Editor-owned keys (id/name/file/tuning/capo) take the fresh values; everything else (type,centOffset,notationpointers, unknown additive keys) survives.typestamping: entries with notypeget one inferred from the display name in the common save loop (both save paths) — keys-family →piano(the spec's spelling), bass names →bass, classic guitar roles (Lead/Rhythm/Combo/Guitar/Acoustic/Electric) →guitar. Deliberately conservative: vocals/drums/ambiguous names stay untyped (vocals and drums are side-file mechanisms per the spec, and a wrong type is worse than none), and an authoredtypeis never clobbered.This makes
typea durable queryable instrument facet with display names as free labels — the groundwork for safe track renaming in the Parts view (#100) and for library instrument filtering downstream.Verification
tests/test_manifest_type_preserve.py— 7 cases (inference incl. conservative refusals and the Synth-Bass/keys-prefix precedence, merge preservation oftype/centOffset/notation/unknown keys, editor-key override, no-old-entry passthrough, input immutability)🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit
type,centOffset, and any previously stored unknown fields) instead of stripping them out.typeis now inferred from the arrangement name only when it’s missing, without overwriting an authoredtype.typeinference and manifest merge/preservation semantics, includingcentOffset: 0handling and regression cases for name matching.