feat(editor): suggest-position — author fretted parts in the piano roll (P6/VA.3) - #143
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds suggest-position editing for fretted parts in the piano roll, including resolver-backed add and drag flows, provisional suggested-position marking, persistence across save/reload and arrangement changes, confirm/accept actions, and a chord re-pitch fix that avoids double-booking strings. ChangesSuggest-position feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FrettedRollUI
participant SuggestPositionResolver
participant History
participant SuggestedMarkStore
User->>FrettedRollUI: click or drag pitch
FrettedRollUI->>SuggestPositionResolver: resolve SOUNDING pitch to {string,fret}
SuggestPositionResolver->>SuggestPositionResolver: enumerate candidates and apply constraints
alt unambiguous
SuggestPositionResolver->>History: execute resolver-backed add/move
History->>SuggestedMarkStore: mark note suggested
else ambiguous
SuggestPositionResolver->>FrettedRollUI: open confirmation popover
User->>FrettedRollUI: pick candidate
FrettedRollUI->>History: execute confirmed add/move
end
User->>FrettedRollUI: accept position
FrettedRollUI->>History: execute AcceptPositionsCmd
History->>SuggestedMarkStore: clear suggested mark
User->>FrettedRollUI: save or switch arrangement
FrettedRollUI->>SuggestedMarkStore: save and restore suggested marks
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ll (P6/VA.3)
A fretted part shown in the piano roll was read-only: adding a note would
force a silent string/fret guess, so it was refused. This is the
suggest-position write path (design V4/VA.3) — the machine enumerates the
playable positions and either writes the unambiguous one (marked suggested)
or asks; it never silently decides ("a confidently-wrong tab is worse than
an honest gap").
- @pure:suggest-position: _suggestPositionPure resolves a sounding pitch to a
{string, fret} — anchor-window first, then nearest previous hand, then
lowest fret; refuses out-of-range / string-occupied / outside-anchor-window
/ open-vs-fretted.
- Add path: double-click / EOF right-click in the fretted roll ->
_rollAddByPitch. Unambiguous -> _commitAddResolved writes a note marked
suggested (dimmed + dashed); ambiguous -> _rollConfirmPosition popover lists
the free candidates and writes a CONFIRMED note. New notes land at the grid
length (_defaultAddSustain), not sustain:0.
- Move path: dragging a fretted note vertically re-pitches it through the
resolver (_rollDragPitchMove), keeping the hand where it was and HOLDING on
a refusal or a technique-locked fret (slide/bend/harmonic). Commits a
suggested-marked MoveNoteCmd; a repitched-nothing drag explains why.
- Marks are a module WeakSet<note>, NOT a note field: a field LEAKS on solo
notes (serialized by reference) and VANISHES on chord members (rebuilt by an
explicit field mapper). The suggestResolved commands are the sanctioned
carve-out that passes the read-only-roll edit lock; a deliberate position
move (MoveToStringCmd) or Accept CONFIRMS (drops the mark; undo re-marks).
Tests: tests/suggest_position.test.js (17, resolver) +
suggest_position_wiring.test.js (9, the write path + wire purity through the
real reconstructChords) + suggest_position_move.test.js (7, drag re-pitch).
All fail on main. Full JS suite green.
Stacked on the group-move PR (fix/editor-group-move-snap): the vertical
re-pitch drag reuses the rigid group time-delta, so this branches off it.
Rebase onto main once it merges.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013JgxKh99UAeQqmhzSc73tv
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
…marks (review) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/editor-suggest-position-p6
branch
from
July 9, 2026 07:48
1723383 to
eb91f5d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
screen.js (1)
4954-4992: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRefusal-hint status message is immediately overwritten by the trailing
updateStatus()call.
setStatus('Couldn’t repitch here…')(Line 4960) runs, butonMouseUpunconditionally falls through todraw(); updateStatus();(Lines 4990-4992), andupdateStatus()itself unconditionally ends withsetStatus('Ready')(Line 9266) — clobbering the hint before the user ever sees it. Compare with_commitAddResolvedand_execAcceptPositions, which both correctly callupdateStatus()before their final meaningfulsetStatus(...)— that's the established working pattern here, and this one call site has it backwards.🐛 Proposed fix: defer the hint until after the trailing updateStatus()
if (rollFretted && (!cmd.markSuggestedIdx || !cmd.markSuggestedIdx.length) && Math.abs(y - S.drag.startY) >= PIANO_LANE_H) { - setStatus('Couldn’t repitch here — hand position or a locked technique blocks it. Try String view or add an anchor.'); + S._pendingRollHint = 'Couldn’t repitch here — hand position or a locked technique blocks it. Try String view or add an anchor.'; } } if (S.drag.type === 'select') { ... } S.drag = null; draw(); updateStatus(); + if (S._pendingRollHint) { setStatus(S._pendingRollHint); S._pendingRollHint = null; } }🤖 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 4954 - 4992, The refusal-hint in onMouseUp is being overwritten by the unconditional updateStatus() path. Move the meaningful setStatus('Couldn’t repitch here…') so it runs after the final draw()/updateStatus() sequence, or otherwise prevent updateStatus() from resetting it here; use the existing _commitAddResolved and _execAcceptPositions flow as the pattern for preserving the last status message.
🧹 Nitpick comments (2)
tests/suggest_position_persist.test.js (1)
89-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining persistence edge cases
- Cover Save-As migration of
editorSuggested:*keys.- Add a duplicate-identity case so
_applySuggestedMarksPure’s greedy 1:1 claim is exercised.🤖 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/suggest_position_persist.test.js` around lines 89 - 193, The suggested-mark persistence tests are missing coverage for Save-As key migration and duplicate-identity matching behavior. Extend the existing persistence test suite around makeEnv, _saveSuggestedMarks, _restoreSuggestedMarks, and _applySuggestedMarksPure to verify that editorSuggested:* data is moved to the new song key on Save-As without leaving stale source keys behind, and add a duplicate-note scenario with identical string/fret/time proximity so the greedy 1:1 re-attachment logic only marks one matching note.tests/suggest_position_move.test.js (1)
90-206: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider adding a case for a mixed locked+movable chord drag.
Good coverage of position-locking, holds, multi-note occupancy exclusion, and the sequential same-string chord fix — but none of these mix a technique-locked note with a movable sibling in the same drag that resolves onto the locked note's string. That's exactly the scenario flagged in
screen.js(Lines 3816-3862) where a held note never joinsclaimed, so a sibling could silently double-book its string. Worth a test like the "SAME string lands DISTINCT" one (Lines 160-186) but with one member carryingslide_to/bend/harmonicinstead of both being freely resolvable.Happy to draft this test if useful.
🤖 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/suggest_position_move.test.js` around lines 90 - 206, Add a test in suggest_position_move.test.js for a mixed chord drag where one note is position-locked and a sibling is movable, and the movable note resolves onto the locked note’s string. Use the existing helpers like makeMoveEnv, startDrag, and _rollDragPitchMove to verify the locked note stays on its original string/fret while the sibling is repicked to a different string instead of silently double-booking; reference the same occupancy behavior covered by the sequential chord test and the _positionLocked/_rollDragPitchMove paths.
🤖 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 `@screen.js`:
- Around line 3776-3862: The drag resolver in _rollDragPitchMove still lets
mixed chords reuse strings because held notes are excluded from the drag-local
occupancy and _occupiedStringsAt only checks a single time point. Seed the local
`claimed`/occupancy set with technique-locked or zero-pitch movers too, so free
siblings cannot repick those strings, and extend the non-moving occupancy check
to reject any note whose full time span overlaps the moving note’s span, not
just notes containing the mover’s onset. Use _rollDragPitchMove and
_occupiedStringsAt as the main touchpoints.
---
Outside diff comments:
In `@screen.js`:
- Around line 4954-4992: The refusal-hint in onMouseUp is being overwritten by
the unconditional updateStatus() path. Move the meaningful setStatus('Couldn’t
repitch here…') so it runs after the final draw()/updateStatus() sequence, or
otherwise prevent updateStatus() from resetting it here; use the existing
_commitAddResolved and _execAcceptPositions flow as the pattern for preserving
the last status message.
---
Nitpick comments:
In `@tests/suggest_position_move.test.js`:
- Around line 90-206: Add a test in suggest_position_move.test.js for a mixed
chord drag where one note is position-locked and a sibling is movable, and the
movable note resolves onto the locked note’s string. Use the existing helpers
like makeMoveEnv, startDrag, and _rollDragPitchMove to verify the locked note
stays on its original string/fret while the sibling is repicked to a different
string instead of silently double-booking; reference the same occupancy behavior
covered by the sequential chord test and the _positionLocked/_rollDragPitchMove
paths.
In `@tests/suggest_position_persist.test.js`:
- Around line 89-193: The suggested-mark persistence tests are missing coverage
for Save-As key migration and duplicate-identity matching behavior. Extend the
existing persistence test suite around makeEnv, _saveSuggestedMarks,
_restoreSuggestedMarks, and _applySuggestedMarksPure to verify that
editorSuggested:* data is moved to the new song key on Save-As without leaving
stale source keys behind, and add a duplicate-note scenario with identical
string/fret/time proximity so the greedy 1:1 re-attachment logic only marks one
matching note.
🪄 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: 2a4d826c-9c2d-4dcc-95ac-cc40f9d00155
📒 Files selected for processing (6)
CHANGELOG.mdscreen.jstests/suggest_position.test.jstests/suggest_position_move.test.jstests/suggest_position_persist.test.jstests/suggest_position_wiring.test.js
Comment on lines
+3776
to
+3862
| // Strings sounding across `time` — a string can't play two notes at the same | ||
| // instant. A note occupies its string from n.time to n.time+sustain (a | ||
| // zero-sustain note occupies only its onset). `except` skips notes being edited | ||
| // from their own occupancy check: a single index, a Set of indices, or -1/null | ||
| // for none (so a multi-note drag can exclude its whole moving set). | ||
| function _occupiedStringsAt(arr, time, except) { | ||
| const skip = except instanceof Set ? except | ||
| : (typeof except === 'number' && except >= 0 ? new Set([except]) : null); | ||
| const occ = new Set(); | ||
| const nn = (arr && Array.isArray(arr.notes)) ? arr.notes : []; | ||
| for (let i = 0; i < nn.length; i++) { | ||
| if (skip && skip.has(i)) continue; | ||
| const n = nn[i]; | ||
| if (!n || typeof n.time !== 'number') continue; | ||
| const end = n.time + (n.sustain || 0); | ||
| if (time >= n.time - 1e-6 && time <= end + 1e-6) occ.add(n.string); | ||
| } | ||
| return occ; | ||
| } | ||
|
|
||
| // A note whose FRET is coupled to a technique, so a roll pitch-move can't repick | ||
| // its string/fret without corrupting the technique's meaning (V5): a slide | ||
| // targets a fret, a bend/harmonic anchors to one. Such notes REFUSE the roll | ||
| // pitch-move (edit them in the fretted views). Pitch-portable techniques | ||
| // (palm mute, vibrato, accent…) ride along and don't lock. | ||
| function _positionLocked(n) { | ||
| const t = (n && n.techniques) || {}; | ||
| return t.slide_to >= 0 || t.slide_unpitch_to >= 0 | ||
| || t.bend > 0 || (Array.isArray(t.bend_values) && t.bend_values.length > 0) | ||
| || !!t.harmonic || !!t.harmonic_pinch; | ||
| } | ||
|
|
||
| // Live pitch-MOVE by drag in the fretted roll (VA.3): `dy` is a SOUNDING-pitch | ||
| // delta; the resolver repicks {string,fret} at the new pitch, biased to keep the | ||
| // hand where it was (prevNote = the note's original fret). `snappedDt` is the | ||
| // shared group time delta (already snapped + clamped ≥ 0 by _groupTimeDeltaPure) | ||
| // and always applies (a pitch-domain edit). A position-locked note, or a pitch | ||
| // the resolver refuses (ambiguous / out of the hand window / occupied), HOLDS at | ||
| // its last resolvable position — the note visibly sticks rather than the machine | ||
| // guessing. The drop (onMouseUp) commits the net change as a suggested MoveNoteCmd. | ||
| function _rollDragPitchMove(nn, snappedDt, dy) { | ||
| const arr = S.arrangements[S.currentArr]; | ||
| const ctx = _rollPitchCtx(); | ||
| if (!arr || !ctx) return; | ||
| const dMidi = -Math.round(dy / PIANO_LANE_H); | ||
| const moving = new Set(S.drag.indices); | ||
| // Time always applies (a pitch-domain edit); gather the actual pitch-movers. | ||
| const movers = []; | ||
| for (let i = 0; i < S.drag.indices.length; i++) { | ||
| const ni = S.drag.indices[i]; | ||
| const n = nn[ni]; | ||
| if (!n) continue; | ||
| n.time = S.drag.origTimes[i] + snappedDt; | ||
| if (dMidi === 0 || _positionLocked(n)) continue; // no pitch move / technique-locked ⇒ hold | ||
| const origPitch = _soundingPitchPure( | ||
| ctx.openMidi, ctx.tuning, ctx.capo, S.drag.origStrings[i], S.drag.origFrets[i]); | ||
| if (origPitch === null) continue; | ||
| movers.push({ n, target: origPitch + dMidi, prevFret: S.drag.origFrets[i] }); | ||
| } | ||
| // Resolve SEQUENTIALLY, in ascending target sounding-pitch (ties keep drag | ||
| // order — Array.sort is stable). Each resolved member's chosen string joins | ||
| // the occupancy the later members see, so two members of a vertically-dragged | ||
| // chord can't independently pick the SAME string: at save reconstructChords | ||
| // does frets[n.string] = n.fret, and a shared string would drop a member | ||
| // (the chord template silently loses a note). A member the resolver refuses | ||
| // HOLDS its old position and contributes NO occupancy (unchanged behaviour). | ||
| movers.sort((a, b) => a.target - b.target); | ||
| const claimed = []; // {time, end, string} chosen by already-resolved siblings this drag | ||
| for (const mv of movers) { | ||
| const n = mv.n; | ||
| const occ = _occupiedStringsAt(arr, n.time, moving); | ||
| // Full interval overlap (not just onset containment): movers are ordered | ||
| // by pitch, not time, so a later-resolved member can start BEFORE an | ||
| // already-claimed one yet sustain through it — both directions collide. | ||
| const nEnd = n.time + (n.sustain || 0); | ||
| for (const c of claimed) { | ||
| if (n.time <= c.end + 1e-6 && nEnd >= c.time - 1e-6) occ.add(c.string); | ||
| } | ||
| const res = _suggestPositionPure( | ||
| mv.target, n.time, { fret: mv.prevFret }, _rollAnchorList(arr), occ, ctx); | ||
| if (res.resolved) { | ||
| n.string = res.resolved.string; | ||
| n.fret = res.resolved.fret; | ||
| claimed.push({ time: n.time, end: n.time + (n.sustain || 0), string: res.resolved.string }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "=== locate relevant symbols ==="
rg -n "_occupiedStringsAt|_positionLocked|_rollDragPitchMove|reconstructChords|claimed.push|_suggestPositionPure|_rollPitchCtx|S.drag.indices" screen.js
echo
echo "=== inspect surrounding lines around drag code ==="
sed -n '3760,3885p' screen.js
echo
echo "=== inspect chord reconstruction/save logic ==="
rg -n "reconstructChords|frets\\[n\\.string\\] = n\\.fret|chord" screen.js
sed -n '7200,7345p' screen.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 41141
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path('screen.js')
text = p.read_text()
for needle in ['function _occupiedStringsAt', 'function _positionLocked', 'function _rollDragPitchMove', 'frets[n.string] = n.fret']:
idx = text.find(needle)
print(f'--- {needle} @ {idx}')
if idx != -1:
start = text.rfind('\n', 0, idx-200)
end = text.find('\n\n', idx)
print(text[idx: end if end!=-1 else idx+800])
PYRepository: got-feedBack/feedBack-plugin-editor
Length of output: 7443
🏁 Script executed:
#!/bin/bash
set -e
sed -n '3600,3668p' screen.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 4259
screen.js:3824-3859 — Drag resolution still allows duplicate strings in mixed chords
- Technique-locked or zero-pitch members are skipped before
claimedis seeded, so a free sibling in the same drag can still repick the held note’s string. Seed the drag-local occupancy with held notes too. _occupiedStringsAt()only checks the mover’s onset against existing spans. A static note that starts during the mover’s sustain on the same string can still slip through; this path needs interval-overlap checks for non-moving notes as well.
🤖 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 3776 - 3862, The drag resolver in _rollDragPitchMove
still lets mixed chords reuse strings because held notes are excluded from the
drag-local occupancy and _occupiedStringsAt only checks a single time point.
Seed the local `claimed`/occupancy set with technique-locked or zero-pitch
movers too, so free siblings cannot repick those strings, and extend the
non-moving occupancy check to reject any note whose full time span overlaps the
moving note’s span, not just notes containing the mover’s onset. Use
_rollDragPitchMove and _occupiedStringsAt as the main touchpoints.
This was referenced Jul 9, 2026
Merged
Merged
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.
What & why
A fretted part shown in the piano roll was read-only — adding a note there
would force a silent string/fret guess, so it was refused. This is the
suggest-position write path (design V4/VA.3): the machine enumerates the
playable positions and either writes the unambiguous one (marked suggested)
or asks. It never silently decides — a confidently-wrong tab is worse than an
honest gap.
How
@pure:suggest-position):_suggestPositionPureresolves asounding pitch →
{string, fret}— anchor-window first, then nearestprevious hand, then lowest fret; refuses
out-of-range/string-occupied/outside-anchor-window/open-vs-fretted._rollAddByPitch.Unambiguous →
_commitAddResolvedwrites a note marked suggested (dimmed +dashed); ambiguous →
_rollConfirmPositionpopover lists the free candidatesand writes a confirmed note. New notes land at the grid length
(
_defaultAddSustain), notsustain:0.resolver (
_rollDragPitchMove), keeping the hand where it was and holdingon a refusal or a technique-locked fret (slide/bend/harmonic). Commits a
suggested-marked
MoveNoteCmd; a re-pitched-nothing drag explains why.WeakSet<note>, never a note field — a field leakson solo notes (serialized by reference) and vanishes on chord members
(rebuilt via an explicit field mapper). The
suggestResolvedcommands are thesanctioned carve-out that passes the read-only-roll edit lock; a deliberate
position move (
MoveToStringCmd) or Accept confirms (drops the mark; undore-marks). A status nudge shows how many positions are still unconfirmed.
Tests
suggest_position.test.js(17, resolver) +suggest_position_wiring.test.js(9, the write path + wire purity through the real
reconstructChords) +suggest_position_move.test.js(7, drag re-pitch). All fail onmain. Full JSsuite green.
⚠ Stacked PR
Branches off #142 (
fix/editor-group-move-snap) — the vertical re-pitch dragreuses that PR's rigid group time-delta (
_groupTimeDeltaPure+primaryOrigTime),so this is a real dependency. Base is set to that branch; GitHub retargets to
mainwhen #142 merges. Review #142 first.Summary by CodeRabbit
New Features
Bug Fixes