feat(editor): first-class Cut/Copy/Paste — visible, snapped, guarded, and bend-safe - #270
Conversation
Copy/paste existed only as inline keydown code — invisible to the menu, shortcut panel and palette, no Cut, shallow technique copies sharing bend-curve arrays across pastes, raw-cursor paste, and no lane/kind guards (pasting onto fewer strings wrote notes on lanes that don't exist). Now: registry commands copySelection/cutSelection/ pasteAtPlayhead (Ctrl+C/X/V; EOF keeps Ctrl+X=mute so Cut is Shift+Del there), Edit menu rows, deep-cloned pack/plan pures with relative times, snap-honouring paste at the playhead, lane clamp with honest skip counts, keys<->fretted refusal, read-only-roll guard for cross-track pitch writes, pasted notes selected, one undoable step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds session-scoped note copy, cut, and paste commands with normalized timing, cloned technique data, undo integration, menu and shortcut wiring, mode guards, tests, and changelog documentation. ChangesClipboard commands
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ShortcutRegistry
participant EditorCommands
participant NoteClipboard
participant EditHistory
User->>ShortcutRegistry: Ctrl+C, Ctrl+X, or Ctrl+V
ShortcutRegistry->>EditorCommands: copySelection, cutSelection, or pasteAtPlayhead
EditorCommands->>NoteClipboard: pack selection or plan paste
NoteClipboard->>EditHistory: record delete or add operation
EditHistory-->>User: update notes and selection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/clipboard.test.mjs (1)
119-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winActually exercise the keys ↔ fretted guard.
This test only checks drum and tempo modes; it never changes track shape or asserts rejection. Add a copy from one track type followed by a paste attempt into the other.
🤖 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/clipboard.test.mjs` around lines 119 - 128, Extend the “mode and shape guards” test to exercise the keys↔fretted incompatibility: copy a selection from one track shape, switch the active track to the other shape, and assert _editorPasteAtPlayhead() returns false. Preserve the existing drumEditMode and tempoMapMode rejection assertions.
🤖 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 `@src/input.js`:
- Around line 499-505: Update the Cut handling around the clipboard mutation to
reject recording mode, Parts view, and read-only fretted-roll state before
DeleteNotesCmd or success status; also update every Paste path in the
corresponding clipboard handler to apply the same guards, including same-track
writes, while preserving existing permitted behavior.
---
Nitpick comments:
In `@tests/clipboard.test.mjs`:
- Around line 119-128: Extend the “mode and shape guards” test to exercise the
keys↔fretted incompatibility: copy a selection from one track shape, switch the
active track to the other shape, and assert _editorPasteAtPlayhead() returns
false. Preserve the existing drumEditMode and tempoMapMode rejection assertions.
🪄 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: 7caec7da-47dc-4c78-9896-8026705fba31
📒 Files selected for processing (5)
CHANGELOG.mdsrc/input.jssrc/menu-bar.jssrc/shortcuts.jstests/clipboard.test.mjs
…GELOG) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
…KeyDown Review finding (community bot, valid): the registry made Cut/Paste menu- and palette-invokable, which bypasses onKeyDown's mode gates — Cut/Paste could mutate during MIDI recording, in the Tracks overview, and Paste could add pitches to the read-only fretted roll on the SAME track. _clipboardWriteBlocked() now gates every clipboard write inside the commands themselves (recording / Tracks overview / read-only roll — the same trio the right-click delete guard enforces); plain Copy stays free. The cross-track-only roll check is replaced by the all-pastes guard. Tests: the keys↔fretted refusal is now actually exercised in BOTH directions (was asserted only by mode flags), and a new registry-path case proves the overview blocks cut + paste while copy stays allowed — it fails without the guard move. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
PR #270 moved the note clipboard to a module-local _noteClipboard and removed every reader/writer of S.clipboard, but left the now-dead `clipboard: null` field in the session state. Remove it — zero refs remain (grep-verified), and a stale field misleads the next reader. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/clipboard.test.mjs (1)
61-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the negative-anchor clamp.
This test claims to validate
t >= 0, but all calls useatTime = 10. A regression removingMath.max(0, ...)would still pass. Add a negative-anchor assertion.🧪 Proposed test
const bass = _clipboardPastePlanPure(clip, 10, 4); assert.strictEqual(bass.notes.length, 1); assert.strictEqual(bass.laneSkipped, 1); + const clamped = _clipboardPastePlanPure(clip, -2, 6); + assert.deepStrictEqual(clamped.notes.map(n => n.time), [0, 3]); assert.strictEqual(_clipboardPastePlanPure(null, 10, 6), 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 `@tests/clipboard.test.mjs` around lines 61 - 69, Add an assertion in the test covering _clipboardPastePlanPure that invokes it with a negative atTime and verifies the resulting note times are clamped to zero or greater, specifically exercising the Math.max(0, ...) behavior while preserving the existing retiming and missing-string checks.
🤖 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 `@CHANGELOG.md`:
- Around line 57-58: Remove the duplicate ### Fixed heading in the Unreleased
changelog section and append the Shift Audio persistence and Resnap selection
entries to the existing ### Fixed list.
- Line 67: Remove the stray truncated “Inspector technique edits are undoable
now.” bullet fragment near the “Resnap selection” entry, while preserving the
complete existing version of that changelog item and all other list entries.
- Around line 142-143: Remove the duplicate Command palette bullet from
CHANGELOG.md, retaining only the entry that references View ▸ Command palette
and deleting the one that references Help ▸ Command palette.
---
Outside diff comments:
In `@tests/clipboard.test.mjs`:
- Around line 61-69: Add an assertion in the test covering
_clipboardPastePlanPure that invokes it with a negative atTime and verifies the
resulting note times are clamped to zero or greater, specifically exercising the
Math.max(0, ...) behavior while preserving the existing retiming and
missing-string checks.
🪄 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: addaba90-99e2-4c24-b536-638abc23ce45
📒 Files selected for processing (6)
CHANGELOG.mdsrc/input.jssrc/menu-bar.jssrc/shortcuts.jssrc/state.jstests/clipboard.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/menu-bar.js
- src/shortcuts.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/clipboard.test.mjs (1)
61-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the negative-anchor clamp.
This test claims to validate
t >= 0, but all calls useatTime = 10. A regression removingMath.max(0, ...)would still pass. Add a negative-anchor assertion.🧪 Proposed test
const bass = _clipboardPastePlanPure(clip, 10, 4); assert.strictEqual(bass.notes.length, 1); assert.strictEqual(bass.laneSkipped, 1); + const clamped = _clipboardPastePlanPure(clip, -2, 6); + assert.deepStrictEqual(clamped.notes.map(n => n.time), [0, 3]); assert.strictEqual(_clipboardPastePlanPure(null, 10, 6), 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 `@tests/clipboard.test.mjs` around lines 61 - 69, Add an assertion in the test covering _clipboardPastePlanPure that invokes it with a negative atTime and verifies the resulting note times are clamped to zero or greater, specifically exercising the Math.max(0, ...) behavior while preserving the existing retiming and missing-string checks.
🤖 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 `@CHANGELOG.md`:
- Around line 57-58: Remove the duplicate ### Fixed heading in the Unreleased
changelog section and append the Shift Audio persistence and Resnap selection
entries to the existing ### Fixed list.
- Line 67: Remove the stray truncated “Inspector technique edits are undoable
now.” bullet fragment near the “Resnap selection” entry, while preserving the
complete existing version of that changelog item and all other list entries.
- Around line 142-143: Remove the duplicate Command palette bullet from
CHANGELOG.md, retaining only the entry that references View ▸ Command palette
and deleting the one that references Help ▸ Command palette.
---
Outside diff comments:
In `@tests/clipboard.test.mjs`:
- Around line 61-69: Add an assertion in the test covering
_clipboardPastePlanPure that invokes it with a negative atTime and verifies the
resulting note times are clamped to zero or greater, specifically exercising the
Math.max(0, ...) behavior while preserving the existing retiming and
missing-string checks.
🪄 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: addaba90-99e2-4c24-b536-638abc23ce45
📒 Files selected for processing (6)
CHANGELOG.mdsrc/input.jssrc/menu-bar.jssrc/shortcuts.jssrc/state.jstests/clipboard.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/menu-bar.js
- src/shortcuts.js
🛑 Comments failed to post (3)
CHANGELOG.md (3)
57-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicate
### Fixedheading within the same Unreleased block.Line 57 introduces a second
### Fixedheading while one already exists (unmarked) at line 25, both inside the same## [Unreleased]section — this is exactly what the markdownlint MD024 warning flags. The new fix bullets that follow (Shift Audio persistence, Resnap selection) should be appended to the existing### Fixedlist instead of starting a new heading.📝 Proposed fix
recording and your notes ride along. -### Fixed - - **"Shift Audio…" now survives saving and reopening.** The recording-vs-chart📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 57-57: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 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 `@CHANGELOG.md` around lines 57 - 58, Remove the duplicate ### Fixed heading in the Unreleased changelog section and append the Shift Audio persistence and Resnap selection entries to the existing ### Fixed list.Source: Linters/SAST tools
67-67: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stray duplicate bullet fragment breaks the list.
Line 67 is a truncated copy of the "Inspector technique edits are undoable now." bullet (
Toggling a technique flagwith no continuation) inserted right before the new "Resnap selection" bullet. The full, correct version of this exact bullet already exists further down (unchanged, lines 79-86), so line 67 is a stray leftover that renders as a broken, unfinished list item.📝 Proposed fix
byte-identical to before. -- **Inspector technique edits are undoable now.** Toggling a technique flag - **Resnap selection now works with Snap toggled off — and snaps both edges.**🤖 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 `@CHANGELOG.md` at line 67, Remove the stray truncated “Inspector technique edits are undoable now.” bullet fragment near the “Resnap selection” entry, while preserving the complete existing version of that changelog item and all other list entries.
142-143: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm the actual menu path that opens the command palette. rg -n -i "command palette" src/menu-bar.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 173
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- CHANGELOG.md around lines 132-150 ---' sed -n '132,150p' CHANGELOG.md echo echo '--- repo-wide matches for command palette / menu locations ---' rg -n -i 'command palette|View ▸ Command palette|Help ▸ Command palette' .Repository: got-feedBack/feedBack-plugin-editor
Length of output: 3761
Remove the stray duplicate Command palette bullet
Drop the extra line at
CHANGELOG.md:142; the command palette belongs underView ▸ Command palette, notHelp ▸ Command palette.🤖 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 `@CHANGELOG.md` around lines 142 - 143, Remove the duplicate Command palette bullet from CHANGELOG.md, retaining only the entry that references View ▸ Command palette and deleting the one that references Help ▸ Command palette.
2 participants
What
Copy/paste existed — but only as hardwired inline keydown code: invisible in the Edit menu, the shortcut panel, and the command palette (this session's recurring failure mode), no Cut at all, and four latent defects. All three are first-class registry commands now.
copySelection/cutSelection/pasteAtPlayhead— Ctrl+C / Ctrl+X / Ctrl+V in the FeedBack profile; the EOF profile keeps its legacy Ctrl+X = mute-open, so Cut is Shift+Del there. Edit-menu rows added; the palette and shortcut panel pick them up from the registry automatically.Latent bugs fixed on the way
bend_valuesarray — editing any of them silently edited all. Pack and plan nowstructuredClone; independence is pinned by test.The old inline blocks are removed; dispatch now flows through the same resolver/registry path as every command (both hand resolvers gained the three bindings). Ctrl+D duplicate is untouched.
Tests
tests/clipboard.test.mjs(6, fail on main): relative-time packing, deep-clone independence (mutating the original, or one paste, never touches the others), the retime/clamp/lane-skip plan, the full copy→paste flow through the realEditHistory(one undo entry; exec → rollback → redo reproduces exactly), the cut/undo/clipboard-survives contract, and the mode guards. Full suite 147 green, lint 0 errors,routes.pyuntouched.Live-verified
Real project, real keystrokes: Ctrl+C → "Copied 4 notes", seek to 100s, Ctrl+V → "Pasted 4 notes at the playhead" (1114 → 1118 notes, exactly one undo entry), Ctrl+X → "Cut 2 notes" (→ 1116). Zero page errors.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit