feat(editor): notes carry a notated rhythm value (note-value model foundation)#360
Open
ChrisBeWithYou wants to merge 1 commit into
Open
feat(editor): notes carry a notated rhythm value (note-value model foundation)#360ChrisBeWithYou wants to merge 1 commit into
ChrisBeWithYou wants to merge 1 commit into
Conversation
…undation) The first slice of the note-value / rhythm-duration model (the roadmap's "deep foundation under everything"): a note can carry a symbolic `rhythm` — base (whole…128th), dots, nested tuplets, grace — as a THIRD quantity beside its onset and its sounding `sustain`, so a staccato quarter and a legato quarter are the same value, different ring. Lands the pure engine + the undoable command; no UI. - `src/rhythm-value.js` (pure tier): whole-note magnitude (base × dots × nested tuplet, grace = 0), the beat conversion, best-fit with an HONEST null past tolerance, greedy tie-decomposition, and the GP/MIDI/MusicXML tick projection with a lossy flag. Fully unit-tested. - `SetRhythmValueCmd`: sets/clears value on a selection as one undoable edit, snapshotting the absent-vs-present distinction with a sentinel and cloning the value per note (no aliasing). - value is a runtime field like `note.beat`: it SURVIVES chord reconstruction (the field-mapper vanish hazard) but never rides the seconds-only note wire (`_stripBeat`), so a value-free pack stays byte-identical and the content signature is untouched. Authored values persist later via the §7.6 notation side-file, not the note dict. - tests drive the REAL EditHistory / reconstructChords / save-strip, not stubs: exec→rollback→redo round-trips, chord-member survival, off-wire strip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
📝 WalkthroughWalkthroughAdds a first-class rhythm-value model with tuplet, beat, tick, and bar conversions; introduces an undoable rhythm-setting command; preserves rhythm through chord rebuilding while stripping it from serialized runtime data; and adds model and integration tests. ChangesRhythm value foundation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant SetRhythmValueCmd
participant NoteModel
participant SaveStrip
Editor->>SetRhythmValueCmd: execute rhythm update
SetRhythmValueCmd->>NoteModel: set or clear cloned rhythm
NoteModel-->>SetRhythmValueCmd: retain prior state for rollback
Editor->>SaveStrip: build serialized data
SaveStrip-->>Editor: omit runtime rhythm fields
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rhythm-value.js (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider freezing the exported constant arrays.
RHYTHM_BASESandCOMMON_TUPLETSare exported as plain mutable arrays/objects. Given this module's own stated concern about shared-mutable-state bugs (thecloneRhythm"bend_values aliasing bug" comment), a consumer accidentally doingRHYTHM_BASES.push(...)or mutating aCOMMON_TUPLETSentry would silently corrupt best-fit results for every caller.🛡️ Proposed fix
-export const RHYTHM_BASES = [1, 2, 4, 8, 16, 32, 64, 128]; +export const RHYTHM_BASES = Object.freeze([1, 2, 4, 8, 16, 32, 64, 128]); -export const COMMON_TUPLETS = [ - { n: 3, m: 2 }, { n: 5, m: 4 }, { n: 6, m: 4 }, { n: 7, m: 4 }, { n: 9, m: 8 }, -]; +export const COMMON_TUPLETS = Object.freeze([ + { n: 3, m: 2 }, { n: 5, m: 4 }, { n: 6, m: 4 }, { n: 7, m: 4 }, { n: 9, m: 8 }, +].map(Object.freeze));🤖 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 `@src/rhythm-value.js` around lines 31 - 37, Freeze the exported RHYTHM_BASES and COMMON_TUPLETS constants to prevent consumers from modifying shared search configuration, and also freeze each tuple object within COMMON_TUPLETS. Preserve their current values and exported names while making both the arrays and nested entries immutable.
🤖 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 `@src/rhythm-value.js`:
- Around line 31-37: Freeze the exported RHYTHM_BASES and COMMON_TUPLETS
constants to prevent consumers from modifying shared search configuration, and
also freeze each tuple object within COMMON_TUPLETS. Preserve their current
values and exported names while making both the arrays and nested entries
immutable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d6cf4a0-7f5d-4e10-bd85-f9c4c4088543
📒 Files selected for processing (7)
CHANGELOG.mdsrc/chords.jssrc/commands.jssrc/rhythm-value.jssrc/tempo.jstests/rhythm_value.test.mjstests/rhythm_value_cmd.test.mjs
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
1 participant
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.
What this is
The foundation slice of the note-value / rhythm-duration model — the missing rhythmic layer under the editor. Today a note is onset +
sustain(both seconds) with no notated value, and the live Tab view fakes durations off a 16th grid. This adds notated value as a first-class, symbolic quantity so the editor can eventually engrave, round-trip, and teach rhythm honestly.The model (three decoupled quantities on one beat coordinate)
note.time) — where it starts.note.rhythm, new) — a quarter / dotted-eighth / triplet member. Symbolic, optional, best-fit-derived when absent.note.sustain) — the free ring-out.A staccato quarter and a legato quarter are the same value, different ring — so value and sustain both persist and neither overwrites the other.
What this PR lands (no UI yet)
src/rhythm-value.js(pure tier): whole-note magnitude (base × dots × nested tuplet; grace = 0), the beat conversion, best-fit with an honestnullpast tolerance, greedy tie-decomposition, and the GP/MIDI/MusicXML tick projection with a lossy flag.SetRhythmValueCmd: set/clear value on a selection as one undoable edit; snapshots the absent-vs-present distinction with a sentinel; clones the value per note (no aliasing).Load-bearing invariants (each has a test that fails without the fix)
rhythmis a runtime field likenote.beat—_stripBeatpeels it off the seconds-only note wire, so a value-free pack stays byte-identical and the arrangement content signature is untouched. Authored values will persist later via the feedpak §7.6 notation side-file, never the note dict.reconstructChordsrebuilds chord members through an explicit field-mapper that would otherwise silently droprhythm; it now carries it (omitting the key when absent).nullpast tolerance rather than a plausible-but-wrong value.Tests
tests/rhythm_value.test.mjs(pure) +tests/rhythm_value_cmd.test.mjsdrive the real EditHistory / reconstructChords / save-strip (not stubs): exec→rollback→redo round-trips including absent-vs-present, per-note clone independence, chord-member survival, and the off-wire strip. Full JS suite green (317), lint clean (0 errors).What's next
This is slice 1 of the model. Follow-ups: honest live-Tab from authored value, triplet/tie authoring UI, value-aware entry, import value lift/inference, and persistence + honest export. Persisting engraving-grade values (grace/nested tuplets) requires an FEP to extend feedpak §7.6 — a parallel spec track that gates only the persistence slice; this PR and the authoring slices don't depend on it.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests