feat(editor): per-part view switcher — any fretted part opens in the piano roll (read-first) - #119
Merged
Merged
Conversation
…ding pitch Roadmap 4.16a remainder (guitar-lane scale-degree tint). Extends the merged song key/scale highlight (#108) from the piano roll to guitar/bass lanes. - New _soundingPitchPure: openMidi + tuning offset + CAPO + fret, capo added exactly ONCE. Chart frets are capo-relative — verified against core lib/song.py pitch_from_base, the single source of the formula the tuner and highway scale-degree derivation share. - The flagged double-count trap is now pinned in code and tests: _absolutePitch (string-moves) still deliberately omits capo (it cancels when comparing two pitches on one arrangement) and both helpers document the division of labor. - Out-of-key fretted notes dim (body alpha cc->55, softened fret number — the piano-roll treatment; never red), unresolvable pitches stay fully lit. Highlight context is hoisted once per draw, zero per-note arrangement work. Key controls now show for any pitched arrangement. Tests: tests/fret_key_highlight.test.js (8 cases) — the formula against known pitches, Drop-D + capo composition, the capo-flips-membership case an uncapoed resolver gets wrong, and the omits-capo pin on _absolutePitch. Full JS suite green except tests/section_coverage.test.js, which fails on current MAIN itself (pre-existing _afterEdit/#107 merge interaction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
…piano roll (read-first) EDITOR-VIEW-MODALITY-DESIGN P1 (VA.1+VA.2, decisions V1-V4/V9). The editing view was derived from the arrangement NAME; it is now a per-part choice. - viewFor(part): per-part pref in editor localStorage keyed song + stable part id (never index/display-name; keys parts piano-locked). Kind inference stays the default. - isKeysMode() split: piano-SURFACE predicate (draw geometry, hit-testing, viewport) vs new isKeysArr() keys-DATA predicate (string moves, chord-sibling grouping, anchors, resize chord-expansion) — a fretted part in the roll still groups chords and keeps string-move machinery (P5 position cycling depends on exactly this). - Read-first roll for fretted parts: one sounding-pitch mapping (_rollMidiForNote via _soundingPitchPure — capo once) hoisted per pass and shared by draw, hitNote, marquee, and updatePianoRange; null pitches skip, never render wrong. - Edit-lock (V4): central gate in EditHistory.exec (typeof-guarded for extracted-test envs) + the live-mutating drag starts (move/resize) + dblclick add + EOF right-click edit; selection still works; a visible pill + status explain why. Lock lifts live on switching back. - Toolbar String/Piano-roll segmented switcher + registry cycleViewMode; selection/drag/note-UI cleared on switch (V3). STACKED ON #115 (feat/editor-key-highlight-guitar) — needs its _soundingPitchPure; merge #115 first. Tests: tests/view_switcher.test.js (11) — pure view resolution, pref persistence/rename stability over stub localStorage, sounding-pitch roll mapping + viewport fit (asserts NOT the wire packing), and the exec gate (inert+notice / regression / live-unlock). Full suite green except the pre-existing CRLF section_coverage failure (#116). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Read-only roll (fretted part in the piano roll) was enforced only at the EditHistory exec chokepoint and the mouse/right-click-add handlers. Two gaps: - exec blocked ALL commands, including songScope (drum tab, tempo grid) edits, so switching an unrelated part into the roll froze tempo/drum editing. songScope commands now pass through the lock (exec + undo/redo). - Several note-edit paths bypass EditHistory entirely and so escaped the lock: note-scope undo/redo, promptSlide/promptSlideUnpitch, the inspector setters (editorInspectorSetTech/SetFlag), and the context-menu editorToggleTech. The context menu opens in the roll under the default right-click behavior and the inspector renders for any selection, so all were reachable. Each is now guarded with _rollReadOnly()/_rollLockNotice. Regression tests (tests/view_switcher.test.js) fail on pre-fix code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
byrongamatos
changed the base branch from
feat/editor-key-highlight-guitar
to
main
July 7, 2026 23:36
…cher # Conflicts: # CHANGELOG.md # screen.js # tests/fret_key_highlight.test.js
This was referenced Jul 8, 2026
byrongamatos
added a commit
that referenced
this pull request
Jul 8, 2026
) * feat(editor): adopt a MIDI's own tempo map on import (Use-vs-Keep) DAW roadmap 3.2. Wires core convert_midi_tempo_map (feedback #796) into the editor's existing single-track keys/drums MIDI import. Until now a .mid import baked note times but discarded the file's tempo + time-signature grid, so every import landed with an implied 4/4 and no bars. Backend (routes.py): - New module-level _sanitize_midi_tempo_map (pure gate/shape: returns the {tempos, time_signatures, beats} map only when it carries >=1 numbered downbeat, else {}) and _safe_midi_tempo_map (wraps the core call; returns {} on ImportError from an older host or any extraction error — an import never 500s over timing). Both are module-level so they're unit-testable without a real .mid or the core lib. - import-keys-midi and import-drums-midi now compute the tempo map in the SAME worker as the note conversion (both endpoints rmtree the temp dir right after) and return it as `tempo_map`. track_index is forwarded for SMF type-2. Frontend (screen.js): - @pure:midi-tempo-choice block: project-grid detection (>=2 downbeats), beat- row sanitizing, the Use-vs-Keep default policy (Keep when a grid exists so an audio-aligned timeline is never stomped; Use when it doesn't), and a summary string. - _maybeOfferMidiTempoMap: a radio dialog offered after a MIDI import; Apply with "Use" runs S.history.exec(new TempoGridCmd(...)) — one undoable step that re-locks the loop. No-op (no dialog) for a gridless MIDI, a GP import, or an older host. On a drum import it's chained before the unmapped-notes triage so the two dialogs never stack. - TempoGridCmd is now correctly song-scoped (like the drum commands), matching the EditHistory contract comment. Without it, applying an imported grid right after a drums import — active part still fretted-in-roll — would be silently blocked by the #119 read-only-roll lock. Tests: tests/test_midi_tempo_import.py (gate/shape purity, the two core-call failure floors via injection, track_index forwarding — 8) and tests/midi_tempo_import.test.js (grid detection, sanitizing, default policy, summary, TempoGridCmd songScope + round-trip — 10). Full JS suite green; editor pytest 221 passed / 2 skipped. Both features absent on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzMBtxWnLGHYkMMXtK38Bg * feat(editor): review fixes for #127 (midi tempo import) - Shift the adopted MIDI tempo-map grid by audio_offset so bars land on the same absolute-time mapping as the just-imported notes (notes are placed at raw+audio_offset; the grid was emitted at raw times, so Use would misalign downbeats from the content by audio_offset). - Match the frontend Use-vs-Keep offer gate to the backend: a single numbered downbeat is enough (routes.py _sanitize_midi_tempo_map / the backend already ships single-bar maps); the frontend's 2-downbeat project-grid threshold was silently dropping them. - Regression tests (JS + Python) that fail on pre-fix code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(editor): fold MIDI time-signature denominators onto adopted downbeats Core convert_midi_tempo_map may convey a non-4 denominator only in the time_signatures list, leaving downbeat rows bare. The editor's canonical home for the denominator is beat.den (frontend + _build_song_timeline read that, not the list), so adopting such a grid would save e.g. 6/8 as 6/4. Fold the signatures onto downbeats via the existing _apply_timeline_signatures_to_beats inside _sanitize_midi_tempo_map (before the audio_offset shift, while raw times still match). Regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(editor): carry time-signature denominators forward across downbeats _apply_timeline_signatures_to_beats folded a denominator only onto the exact downbeat matching a time-signature event, but both song_timeline and core MIDI maps emit one event per CHANGE, not per bar. A persistent 7/8 therefore restored/saved as 7/8 on the change bar and /4 on every following bar (latent in the song_timeline load path too, now that the MIDI import path also folds). Apply each event forward until the next change. Regression tests. 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.
P1 of the view-modality train (
EDITOR-VIEW-MODALITY-DESIGN.mdVA.1+VA.2, decisions V1–V4/V9 — the per-track "hit it with every barrel" ask). ⚠ STACKED ON #115 (base branch =feat/editor-key-highlight-guitar): it needs #115''s capo-aware_soundingPitchPure. Merge #115 first; GitHub retargets this to main automatically.What
KEYS_PATTERN→ roll, else lanes). New String · Piano roll toolbar switcher + registry commandcycleViewMode; the pref lives in editor localStorage keyed by song + stable part id (rename/reorder safe, never pack data). Keys parts stay piano-locked — their wire packing has no string semantics for the lane view.isKeysMode()split into the piano-SURFACE predicate (draw geometry, hit-testing, viewport — all ~20 legacy call sites keep working) and a new keys-DATA predicateisKeysArr()for data-semantics sites (string-move helpers, chord-sibling grouping, anchors, resize chord-expansion). A fretted part shown in the roll still groups its chords and keeps its string-move machinery — the P5 position-cycling slice depends on exactly this split._rollMidiForNote— keys packing, or open string + tuning + capo, added once via feat(editor): in-key highlight on the fretted lanes — capo-aware sounding pitch #115''s resolver) hoisted once per pass and shared by draw,hitNote, marquee select, andupdatePianoRange, so they can never disagree; unresolvable pitches skip rather than render wrong. In-key row shading applies in the roll automatically.EditHistory.exec(catches keys, menus, inspector, dialogs; typeof-guarded so extracted-test envs are unaffected) plus the live-mutating drag starts (move/resize), double-click add, and the EOF right-click edit. Selection still works; a visible pill + status explain why; the lock lifts live on switching back. Suggest-position (P6) unlocks it.Tests
tests/view_switcher.test.js(11): pure view resolution (keys locked, junk-safe), pref persistence + rename stability over stub localStorage, the sounding-pitch mapping (capo-once) and viewport fit (asserts the range comes from sounding pitch, NOTstring*24+fret), and the exec gate (inert + notice / unlocked regression / live unlock). Full suite green except the pre-existing CRLFsection_coveragefailure (#116 fixes it).🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu