feat(editor): keyboard time-nudge — ←/→ moves selected notes in time - #240
Merged
Conversation
Gap-audit item #1. Moving a note in time used to require a mouse drag — the most repeated timing tweak. Now the Left/Right arrows nudge the selection one snap step earlier/later as a single grouped, undoable MoveNoteCmd (mirroring _editorResnapSelection's per-note-delta pattern), clamped so the earliest note can't cross the start. With NOTHING selected, ←/→ seek the playhead a step instead — which also positions the keyboard-entry caret (#239). Alt+←/→ still jump note-to-note; plain ←/→ were free (the loop-edge nudge is gated on Alt). New `nudgeTimeLeft`/`nudgeTimeRight` registry commands (both profiles) bound to plain ←/→; `_editorNudgeSelectionTime(dir)` in input.js. `tests/time_nudge.test.mjs` (3: nudge + undo, clamp at 0, no-selection playhead seek). 129 JS suites green (menu_model incl.), lint 0-err (3 pre-existing warnings). routes.py untouched. Verified live on AC/DC — Back In Black: Ctrl+A then → → "Nudged 1328 notes later by one step", ← nudges them back. No page errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesKeyboard time nudging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Keyboard
participant ShortcutMapping
participant EditorDispatcher
participant EditorState
participant History
Keyboard->>ShortcutMapping: Press plain ArrowLeft or ArrowRight
ShortcutMapping->>EditorDispatcher: Resolve nudgeTimeLeft or nudgeTimeRight
EditorDispatcher->>EditorState: Nudge selected notes or playhead by one snap step
EditorState->>History: Record grouped note movement
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 |
![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/time_nudge.test.mjs (1)
36-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new key-to-command mappings.
These tests call
_editorRunEofCommanddirectly, so they do not verify that plain ArrowLeft/ArrowRight map correctly in either shortcut profile. Add assertions for both mapping functions, including Alt+ArrowLeft/Right remainingprevNote/nextNote.🤖 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/time_nudge.test.mjs` around lines 36 - 58, Extend the time-nudge tests to cover both shortcut-profile mapping functions, asserting plain ArrowLeft/ArrowRight resolve to the appropriate nudge commands and Alt+ArrowLeft/ArrowRight remain mapped to prevNote/nextNote. Keep the existing direct _editorRunEofCommand behavior tests unchanged.
🤖 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 420-430: Update the status message in the note-nudge flow around
_editorSnapStepSeconds and delta so clamped movement is reported accurately
instead of always saying “by one step.” Use the actual applied delta or
explicitly indicate when movement was clamped at time zero, while preserving the
existing movement behavior.
---
Nitpick comments:
In `@tests/time_nudge.test.mjs`:
- Around line 36-58: Extend the time-nudge tests to cover both shortcut-profile
mapping functions, asserting plain ArrowLeft/ArrowRight resolve to the
appropriate nudge commands and Alt+ArrowLeft/ArrowRight remain mapped to
prevNote/nextNote. Keep the existing direct _editorRunEofCommand behavior tests
unchanged.
🪄 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: b7551657-5e8e-4498-9608-7c995cd1803c
📒 Files selected for processing (4)
CHANGELOG.mdsrc/input.jssrc/shortcuts.jstests/time_nudge.test.mjs
Comment on lines
+420
to
+430
| let delta = dir * _editorSnapStepSeconds(); | ||
| const minT = Math.min(...idxs.map(i => nn[i].time)); | ||
| if (minT + delta < 0) delta = -minT; // don't push before 0 | ||
| if (Math.abs(delta) < 1e-9) return false; | ||
| const dtimes = idxs.map(() => delta); | ||
| const dstrings = idxs.map(() => 0); | ||
| S.history.exec(new MoveNoteCmd(idxs, dtimes, dstrings, null)); | ||
| _editBlipAt(); | ||
| host.draw(); | ||
| host.updateStatus(); | ||
| setStatus(`Nudged ${idxs.length} note${idxs.length === 1 ? '' : 's'} ${dir > 0 ? 'later' : 'earlier'} by one step`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report clamped movement accurately.
When the earliest note is near zero, delta is reduced below one snap step, but the status still says “by one step.” The existing clamp test moves by only 0.1 seconds. Use the actual delta or report that the selection was clamped at the start.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 429-429: React's useState should not be directly called
Context: setStatus(Nudged ${idxs.length} note${idxs.length === 1 ? '' : 's'} ${dir > 0 ? 'later' : 'earlier'} by one step)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🪛 OpenGrep (1.23.0)
[ERROR] 426-426: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@src/input.js` around lines 420 - 430, Update the status message in the
note-nudge flow around _editorSnapStepSeconds and delta so clamped movement is
reported accurately instead of always saying “by one step.” Use the actual
applied delta or explicitly indicate when movement was clamped at time zero,
while preserving the existing movement behavior.
# Conflicts: # CHANGELOG.md
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.
Gap-audit item #1. Moving a note in time used to require a mouse drag — the single most-repeated timing tweak.
What's new
MoveNoteCmd(mirrors_editorResnapSelection's per-note-delta pattern). The group is clamped so the earliest note can't cross the start.Implementation
New
nudgeTimeLeft/nudgeTimeRightregistry commands (both keymap profiles) bound to plain ←/→;_editorNudgeSelectionTime(dir)ininput.js.Tests / gates
tests/time_nudge.test.mjs(3: nudge + exact undo, clamp at 0 preserving spacing, no-selection playhead seek). 129 JS suites green (menu_model included), lint 0 errors (3 pre-existing warnings).routes.pyuntouched.Verified live
On AC/DC — Back In Black: Ctrl+A then → → "Nudged 1328 notes later by one step", ← nudges them back. No page errors.
🤖 Generated with Claude Code
Summary by CodeRabbit