feat(editor): cycle same-pitch positions in the piano roll (Shift+↑/↓) - #126
Conversation
Roadmap VA.5 (design V13.1). A fretted part shown in the piano roll is
read-only — its Y axis is pitch, so the string axis Shift+↑/↓ walks in
String view has no meaning there. Rather than leave those keys dead, they
now cycle the selected note(s) through every {string, fret} that sounds
the SAME pitch on this arrangement's tuning.
- `_cyclePositionCandidatesPure` enumerates integer frets 0–24 on every
string whose capo-less absolute pitch matches (the capo cancels on both
sides — the same _absolutePitch/_soundingPitchPure pairing #115
documents), ordered low string → high; `_cycleStepPure` walks + wraps.
- `_execCyclePosition` drives it through the existing MoveToStringCmd, so
a cycle is one undo step that round-trips like every string move. The
command carries `pitchPreserving` and is the ONE deliberate carve-out
from the read-only-roll lock in EditHistory (exec/undo/redo) — it can
never change what a note sounds like, only which string/fret plays it,
so the "no silent pitch writes" contract is unbreakable by construction.
Everything else stays locked until suggest-position (P6) lands.
- Multi-select cycles each note independently and skips single-position
notes; corrupt/out-of-range positions refuse rather than guess.
- Fretted-in-roll notes now render in their string's lane color with an
`s·f` position chip (octave color + note name are redundant with the Y
axis), so a cycle step reads as a color flip at a fixed height.
Read-first per V4; no pitch-changing write is added. Tuning-aware:
Drop-D, capo, re-entrant all covered. Tests:
tests/roll_position_cycle.test.js (candidate enumeration, wrap, capo pair,
exec/rollback/redo deep-equality inside the locked roll, the lock still
blocks unflagged commands, multi-select independence, all-adversarial
refusals, dispatch routing) — all fail on main.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzMBtxWnLGHYkMMXtK38Bg
|
Warning Review limit reached
Next review available in: 32 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 (2)
📝 WalkthroughWalkthroughAdds a Shift+↑/↓ position-cycling feature for fretted notes in the read-only piano roll, preserving pitch via a ChangesPiano roll fretted position cycling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant KeyRouting
participant _execCyclePosition
participant EditHistory
participant MoveToStringCmd
User->>KeyRouting: Shift+Up/Down (roll locked)
KeyRouting->>_execCyclePosition: transposeStringUp/Down
_execCyclePosition->>_execCyclePosition: compute candidates via _cyclePositionCandidatesPure
_execCyclePosition->>_execCyclePosition: advance via _cycleStepPure
_execCyclePosition->>MoveToStringCmd: construct with pitchPreserving=true
_execCyclePosition->>EditHistory: exec(cmd)
EditHistory-->>_execCyclePosition: applied to note position
_execCyclePosition->>KeyRouting: redraw and refresh inspector
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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)
screen.js (1)
2697-2748: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider hardening the
pitchPreservingbypass against future misuse.The lock's carve-out trusts
cmd.pitchPreserving === trueunconditionally inexec/doUndo/doRedo— it never verifies the command actually preserves pitch, it just believes the flag. Today that's safe because the only caller (_execCyclePosition) derivesmovesexclusively from_cyclePositionCandidatesPure/_cycleStepPure, which are pitch-invariant by construction. But the flag is a plain property set post-construction on a genericMoveToStringCmd, so nothing stops a future call site from settingpitchPreserving = trueon a command that isn't actually pitch-preserving, silently defeating the "no silent pitch writes" contract this lock exists to protect.A cheap hardening: assert pitch equivalence (old vs. new position) inside
MoveToStringCmditself whenpitchPreservingis set, or only allow the flag to be set via a dedicated factory that performs that check, rather than a bare property assignment at the call site.🤖 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 `@screen.js` around lines 2697 - 2748, The `pitchPreserving` lock bypass is trusted too broadly in `exec`, `doUndo`, and `doRedo`, since it accepts a plain flag on `MoveToStringCmd` without verifying the command really preserves pitch. Harden this by moving the check into `MoveToStringCmd` itself or a dedicated factory used by `_execCyclePosition`, and only set `pitchPreserving` after asserting the before/after pitch remains identical. Keep the existing carve-out behavior, but make the flag impossible to misuse from future call sites.
🤖 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 `@screen.js`:
- Around line 2697-2748: The `pitchPreserving` lock bypass is trusted too
broadly in `exec`, `doUndo`, and `doRedo`, since it accepts a plain flag on
`MoveToStringCmd` without verifying the command really preserves pitch. Harden
this by moving the check into `MoveToStringCmd` itself or a dedicated factory
used by `_execCyclePosition`, and only set `pitchPreserving` after asserting the
before/after pitch remains identical. Keep the existing carve-out behavior, but
make the flag impossible to misuse from future call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2ecd4f0-068d-4647-abc2-2e58a556bcce
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/roll_position_cycle.test.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
…tion-cycle # Conflicts: # CHANGELOG.md
2 participants
Roadmap VA.5 (design decision V13.1). Wave 2 of the view-modality plan — gate cleared: #115 (
_soundingPitchPure) and #119 (per-part view switcher + read-first roll) are both merged on main.What this does
A fretted part shown in the piano roll is read-only (V4): the roll's Y axis is pitch, so the string axis that Shift+↑/↓ walks in String view has no meaning there. Instead of leaving those keys dead, they now cycle the selected note(s) through every
{string, fret}that sounds the same pitch on this arrangement's tuning — the honest meaning of "move up/down" when Y is fixed to pitch._cyclePositionCandidatesPure(@pure:position-cycle) enumerates integer frets 0–24 on every string whose capo-less absolute pitch matches the note, ordered low string → high. The capo is deliberately omitted: both sides live on the same arrangement so it cancels — the exact_absolutePitch/_soundingPitchPurepairing feat(editor): in-key highlight on the fretted lanes — capo-aware sounding pitch #115 documents (adding it to one side would be the double-count bug those comments warn about)._cycleStepPurewalks the list and wraps._execCyclePositiondrives it through the existingMoveToStringCmd, so a cycle is one undo step that round-trips like every other string move.pitchPreserving;EditHistory.exec/doUndo/doRedolet exactly that flag through. It can never change what a note sounds like — only which string/fret plays it — so the "no silent pitch writes" contract the lock protects is unbreakable by construction. Everything else stays locked until suggest-position (P6) lands.s·fposition chip, replacing the octave color + note name (both redundant with the Y axis). A cycle step reads as a color flip at a fixed height, making the move visible.Read-first per V4 — no pitch-changing write is added.
Tests —
tests/roll_position_cycle.test.js(15, all fail on main)Candidate enumeration (standard / Drop-D / capo-cancels-via-#115-pair / fret ceiling / adversarial →
[]), cycle wrap both directions, single-position + corrupt-current no-ops, exec/rollback/redo deep-equality inside the locked roll, the lock still blocking every unflagged command, multi-select independence (skips single-position, wraps low), keys-data + empty-selection guards, NaN-fret adversarial skip, and Shift+↑/↓ dispatch routing (cycle in the roll, string-move otherwise). Full JS suite green; confirmed the resolver + flag are absent on main.Fresh-region: no overlap with the open PRs — the new pures sit above
MoveToStringCmd, and the lock/render/dispatch edits are the same shared seams the handoff's rebase recipes already cover.Summary by CodeRabbit
New Features
Bug Fixes