editor: upgrade snap grid and timeline jumps - #46
Conversation
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR reworks the editor snap system and timeline navigation, adds snap preference persistence and UI wiring, updates keyboard shortcuts/help text, and expands coverage for the pure snap-navigation helpers. ChangesSnap and Navigation Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
screen.html (1)
116-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatus hint omits the Arrow-key equivalents.
Per the downstream
onKeyDownhandler,ArrowLeft/ArrowRighttrigger the same navigation modes asPageUp/PageDown, but this help text only mentions PgUp/PgDn.♻️ Suggested wording tweak
- <span class="text-xs text-gray-600">Scroll: pan | Ctrl+scroll: zoom | PgUp/PgDn: beats | Shift+PgUp/PgDn: notes | Ctrl+Shift+PgUp/PgDn: grid | Alt+PgUp/PgDn: anchors</span> + <span class="text-xs text-gray-600">Scroll: pan | Ctrl+scroll: zoom | PgUp/PgDn or ←/→: beats | Shift+PgUp/PgDn: notes | Ctrl+Shift+PgUp/PgDn: grid | Alt+PgUp/PgDn: anchors</span>🤖 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.html` at line 116, The status hint text in screen.html is missing the Arrow-key equivalents used by the downstream onKeyDown navigation logic. Update the help copy next to the existing PgUp/PgDn references so it also mentions ArrowLeft/ArrowRight for beats, notes, grid, and anchors, keeping the wording aligned with the key handling in onKeyDown.tests/snap_navigation.test.js (1)
1-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for note/anchor timeline jumps.
_navigateTimeline()still has untestednoteandanchorbranches; add a small test that drives those shortcuts too.🤖 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/snap_navigation.test.js` around lines 1 - 70, Add a small test in snap_navigation.test.js that exercises the untested note and anchor branches inside _navigateTimeline(), using the existing screen.js pure snap-nav helpers as the entry point. Reuse the same test style to drive the timeline-jump shortcut handling for note and anchor targets, and assert the returned navigation times/selection behavior matches the expected forward and backward jumps. Keep the new coverage alongside the current _nextBeatTime, _nextGridTime, and _nextTimeInList tests so the _navigateTimeline() shortcuts are verified without relying on line numbers.
🤖 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 100-126: Backward grid navigation in _nextGridTime currently scans
intervals from earliest to latest, so the dir < 0 path can return an older
sub-grid candidate instead of the nearest one below current. Update the backward
branch to iterate beats/intervals from later to earlier and keep the existing
candidate checks in _nextGridTime so cross-beat jumps pick the closest prior
snap point. Add a regression test covering a backward step across a beat
boundary with snapValue enabled.
---
Nitpick comments:
In `@screen.html`:
- Line 116: The status hint text in screen.html is missing the Arrow-key
equivalents used by the downstream onKeyDown navigation logic. Update the help
copy next to the existing PgUp/PgDn references so it also mentions
ArrowLeft/ArrowRight for beats, notes, grid, and anchors, keeping the wording
aligned with the key handling in onKeyDown.
In `@tests/snap_navigation.test.js`:
- Around line 1-70: Add a small test in snap_navigation.test.js that exercises
the untested note and anchor branches inside _navigateTimeline(), using the
existing screen.js pure snap-nav helpers as the entry point. Reuse the same test
style to drive the timeline-jump shortcut handling for note and anchor targets,
and assert the returned navigation times/selection behavior matches the expected
forward and backward jumps. Keep the new coverage alongside the current
_nextBeatTime, _nextGridTime, and _nextTimeInList tests so the
_navigateTimeline() shortcuts are verified without relying on line numbers.
🪄 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: 33c1aa55-1eb1-48f6-9b51-6e1a8af95140
📒 Files selected for processing (3)
screen.htmlscreen.jstests/snap_navigation.test.js
The snap-model upgrade replaced SNAP_VALUES with SNAP_OPTIONS and added a pure _snapTimeToGrid, but left the core snapTime() still reading SNAP_VALUES[snapIdx] — a ReferenceError the moment any snap-aligned edit (drag/paste/nudge) runs. node --check only checks syntax and the snap-nav test only exercises the @pure block, so neither caught it. Delegate snapTime to _snapTimeToGrid wired to the live model (SNAP_OPTIONS step + the split-out S.snapEnabled flag), and add a source guard against a leftover SNAP_VALUES reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_nextGridTime walked intervals ascending for both directions, so backward from a cursor past the first beat returned the end of interval 0 (e.g. 1.0 from a 1.5 cursor) instead of the grid point just before it (1.25). Walk intervals in reverse for the backward case; forward is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The timeline-jump handler's 'beat' fall-through matched any modifier combo, so Cmd+Arrow (browser Back/Forward), Ctrl+PageUp/Down (tab switch) etc. were captured and preventDefault()'d while the editor was focused. Match only the four documented combos exactly (beat/note/grid/anchor); everything else falls through to the browser. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Review + fix pass (Claude Code). Three fixes pushed (all JS suites green, 8 snap-nav tests):
|
…stem Re-does PR 46 (editor-snap-navigation) on top of the 53–56 shortcut-profile stack, resolving the Alt+Arrow collision between them. PR 46 hardcoded Alt+Arrow to anchor-jump in a profile-agnostic onKeyDown switch; PR 55's FeedBack Native profile binds Alt+Arrow to note-jump (anchor → Ctrl+Alt+Arrow). That's two independent modern schemes for one key (EOF Legacy never uses Alt+Arrow), so the fix is: the profile system owns all key bindings, and 46 contributes only its engine. Kept from PR 46: - Expanded snap grid: SNAP_OPTIONS with straight + triplet divisions and a split-out S.snapEnabled on/off toggle (button + JS-populated select + persistence); snapTime() rewritten onto the pure _snapTimeToGrid. - Interval-aware grid math: _editorJumpGrid now uses _nextGridTime (the grid point in the beat interval CONTAINING the cursor, correct across tempo changes) instead of a fixed step add, seeking via _editorSeekToTime so playback follows like every other jump. Dropped from PR 46: - The hardcoded Alt/Shift/Ctrl+Arrow onKeyDown block — Alt+Arrow now resolves per active profile via the command-ID tables (Native = note, anchor on Ctrl+Alt+Arrow; EOF = Alt+PageUp/Dn for anchors). - Its redundant stateful jump engine (_navigateTimeline/_jumpCursorTo/ _maxScrollX/_currentNoteTimes/_currentAnchorTimes): the profile's _editorJump* already own beat/note/anchor jumps. Also fixes a guaranteed integration break: PR 46 removed SNAP_VALUES, but the stack's _editorSnapStepSeconds (which also powers sustain-resize) and the snapUp command still read it — both migrated to SNAP_OPTIONS[idx].step + S.snapEnabled. Supersedes #46. All 20 editor test suites pass (snap_navigation 8/8, eof_shortcuts 11/11). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Testing
Scope
Summary by CodeRabbit