feat(editor): edge-drag note-duration resize in the read-only fretted roll - #141
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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)
📝 WalkthroughWalkthroughThis PR enables read-only fretted piano-roll sustain edge resizing, marks sustain resize commands as pitch-preserving, changes the sustain resize helper to clamp each member independently, and updates tests plus the changelog to match. ChangesRead-only sustain edge-drag resizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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)
tests/roll_edge_resize.test.js (1)
24-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer throwing over
process.exit(1)inextractBlockfor consistent failure reporting.
extractClassreports missing symbols viaassert.ok(caught by thet()harness, counted infail), butextractBlockcallsprocess.exit(1)directly. If the@pure:edit-historymarker is ever renamed/removed in screen.js, this silently kills the whole process before thet()catch block or the final pass/fail summary runs — masking which specific test failed and skipping any tests after the firstmakeEnv()call.♻️ Suggested fix
function extractBlock(name) { const re = new RegExp('/\\* `@pure`:' + name + ':start[\\s\\S]*?`@pure`:' + name + ':end \\*/'); const m = src.match(re); - if (!m) { console.error(`FAIL: `@pure`:${name} block not found in screen.js`); process.exit(1); } + if (!m) { throw new Error(`@pure:${name} block not found in screen.js`); } return m[0]; }🤖 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/roll_edge_resize.test.js` around lines 24 - 40, `extractBlock` currently terminates the process directly, unlike `extractClass`, which lets the `t()` harness report failures cleanly. Update `extractBlock` to raise an assertion or throw an error instead of calling `process.exit(1)`, so missing `@pure:${name}` markers in screen.js are reported through the normal test failure path and don’t abort the remaining tests or summary output.
🤖 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/roll_edge_resize.test.js`:
- Around line 24-40: `extractBlock` currently terminates the process directly,
unlike `extractClass`, which lets the `t()` harness report failures cleanly.
Update `extractBlock` to raise an assertion or throw an error instead of calling
`process.exit(1)`, so missing `@pure:${name}` markers in screen.js are reported
through the normal test failure path and don’t abort the remaining tests or
summary output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95b3ea3b-6190-4830-9e49-f671b98c5e69
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/roll_edge_resize.test.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
… roll A fretted part shown in the piano roll is edit-locked (no silent string/fret writes until the suggest-position writer lands). But a sustain edit is a DURATION change - it never changes what a note SOUNDS like, only how long it rings - so it should apply directly, exactly like the VA.5 position-cycle's pitchPreserving carve-out. - onMouseDown no longer suppresses the sustain edge grab in the read-only roll (edgeIdx = hitNoteEdge(x, y), not `_rollReadOnly() ? -1 : ...`). - ResizeSustainCmd + ResizeSustainGroupCmd carry pitchPreserving = true, so the EditHistory edit lock passes them (exec + undo + redo). The lock still blocks every non-pitchPreserving (pitch/position) write. Now you can tighten an imported fretted note's duration by dragging its right edge without switching to String view. Tests: tests/roll_edge_resize.test.js (4) - both commands declare pitchPreserving; a single and a group resize apply in a read-only roll and round-trip through undo/redo; an ordinary (unflagged) command is still blocked and the user is told why. All fail on main. Full JS suite green. 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>
…iew) A group sustain edge-drag computed one `anchorOrig + delta` and assigned it to every chord member, flattening members that had different original ring-outs (e.g. imported GP data: bass 1.0, top 0.5) to a single value — silent duration-data loss, first exposed by the read-only fretted-roll resize. Now the drag delta is applied to each member's OWN original sustain (`sustain_i = clamp(origSustain_i + delta)`), and each member clamps independently to >=0 and to its own next same-string onset: a member that would collide stops at its limit while the others keep extending. Single-note resize is unchanged (scalar orig broadcast). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5b44df6 to
c5e64fa
Compare
2 participants
What & why
A fretted part shown in the piano roll is edit-locked (V4) — no silent
string/fret writes until the suggest-position writer lands. But a sustain
edit is a duration change: it never changes what a note sounds like, only
how long it rings. So it should apply directly, exactly like the VA.5
position-cycle's
pitchPreservingcarve-out already does.This lets you tighten an imported fretted note's duration by dragging its
right edge, without switching to String view.
How
onMouseDownno longer suppresses the sustain edge grab in the read-onlyroll:
edgeIdx = hitNoteEdge(x, y)(was_rollReadOnly() ? -1 : …).ResizeSustainCmdandResizeSustainGroupCmdcarrypitchPreserving = true,so the
EditHistoryedit lock passes them onexec/undo/redo. Thelock still blocks every non-pitchPreserving (pitch/position) write.
Tests
tests/roll_edge_resize.test.js(4) — both commands declarepitchPreserving;a single and a group resize apply in a read-only roll and round-trip through
undo/redo; an ordinary (unflagged) command is still blocked and the user is
told why. All fail on
main. Full JS suite green (72/72).Part of the fretted-roll authoring pile. Companion to the suggest-position PR
(which adds notes at a sensible default length); this is the duration-editing
half.
Summary by CodeRabbit