Note-entry preview cell + Logic-style ruler snap - #256
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 (10)
📝 WalkthroughWalkthroughAdds a persisted note-entry caret preview with snap-sized ghost notes and fret digits, wires it through host, menu, and window APIs, updates keyboard placement timing, and makes ruler scrubbing snap-aware with live Alt bypass. ChangesEditor entry preview
Snap-aware ruler scrubbing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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: 2
🤖 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 12-20: Update the changelog entry to reference the correct
`Tempo/Grid ▸ Snap` menu path for the `editorToggleEntryPreview` preference,
replacing the incorrect `View ▸ Snap` reference.
In `@src/ruler.js`:
- Line 507: Update the scrub drag handling around S.drag and scrubTo so the
current e.altKey state is evaluated on every move rather than only when the drag
starts. Pass or refresh the modifier value during each scrub operation while
preserving the existing resume and bypass-snap behavior.
🪄 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: c74d2319-9e51-4255-ad49-3d781144f8b6
📒 Files selected for processing (8)
CHANGELOG.mdsrc/draw.jssrc/host.jssrc/input.jssrc/main.jssrc/menu-bar.jssrc/ruler.jstests/entry_preview.test.mjs
byrongamatos
force-pushed
the
feat/editor-note-entry-preview
branch
from
July 14, 2026 17:43
60bda0f to
e63e659
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@node_modules`:
- Line 1: Remove the committed node_modules symbolic-link entry from the
repository, including its absolute local target, and leave dependency
installation to the project’s existing package-manager workflow.
In `@src/input.js`:
- Around line 114-127: Update the note-entry flow around _editorSeekToTime to
advance from the snapped note time rather than raw S.cursorTime, preventing
drift after free scrubbing. Reuse the positive value returned by
_editorSnapStepSeconds() directly and remove the redundant step > 0 fallback
while preserving sequential note placement.
🪄 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: 76b0bca6-3ea1-4967-bd63-72dfba9342de
📒 Files selected for processing (10)
CHANGELOG.mdnode_modulessrc/draw.jssrc/host.jssrc/input.jssrc/main.jssrc/menu-bar.jssrc/ruler.jssrc/state.jstests/entry_preview.test.mjs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/menu-bar.js
- CHANGELOG.md
- src/draw.js
- src/main.js
- tests/entry_preview.test.mjs
- src/ruler.js
byrongamatos
added a commit
that referenced
this pull request
Jul 14, 2026
… untrack node_modules Drains the four CodeRabbit findings on #256. - input.js: advance the entry caret from the note's SNAPPED time, not the raw cursor. After a free (Alt) scrub — which this same PR introduces — the cursor sits off-grid, so stepping from it left the next note a fraction of a step off instead of flush against the last. Also drop the `step > 0` ternary: _editorSnapStepSeconds() is Math.max(0.001, ...) on every path, never <= 0. - ruler.js: read e.altKey on each scrub move instead of freezing it at mouse-down, matching how the loopedge/barsel drags already re-read e.shiftKey per move. - .gitignore + node_modules: the tracked absolute symlink is the symptom; the cause is that `node_modules/` (trailing slash) only matches directories, so the worktree symlink was never ignored and a `git add -A` swept it in. Drop the slash so the rule covers both, and untrack the blob. - CHANGELOG: the entry-preview toggle lives under Tempo/Grid > Snap, not View. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
The dashed keyboard-entry caret now previews the note a typed fret will drop, not just the insertion point: it is sized to the current note value (the snap step, so the box is the exact footprint the note will fill), sits on the caret's string lane, and ghosts the fret it will carry. Typed notes are placed at that same length so consecutive entries tile the grid instead of stacking as zero-length notes. The preview is a persisted view pref (View > Snap > Note-entry preview) for mouse-first charters who find the cell distracting. Clicking the ruler now snaps the playhead to the nearest beat/subdivision when snap is on (Logic-style), so the caret lands on a real note position; hold Alt for a free, un-snapped scrub. - src/draw.js: _caretCellWidthPure (mirrors note render width) + richer caret block (fill, fret ghost, gated on host.editorEntryPreviewEnabled) - src/input.js: place-at-caret uses sustain=snap step + remembers the fret; _editorEntryPreviewEnabled / editorToggleEntryPreview (localStorage) - src/ruler.js: scrubTo snaps via snapTime; scrub drag carries bypassSnap=Alt - src/host.js, src/main.js: wire editorEntryPreviewEnabled + window toggle - src/menu-bar.js: View > Snap > Note-entry preview - tests/entry_preview.test.mjs: width helper + persisted-toggle coverage Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
…l default-on test Review follow-ups on the note-entry preview: - src/state.js: caretFret was written ad-hoc onto S by _editorPlaceAtCaret but never declared beside caretString. Declared, so the caret's two fields live together and the default (open string, fret 0) is discoverable. - src/draw.js: the fret ghost centred on Math.min(cw, MIN_NOTE_W), but cw is Math.max(MIN_NOTE_W, ...) by construction — the min could never bite. Dropped to MIN_NOTE_W with the intent (the digit stays put as the cell grows) in the comment. - src/input.js: place-at-caret computed the snap step, then recomputed it for the cursor advance. One step, used for both, so "the note is as long as the caret advances" is visible in the code, not a coincidence of two calls. - tests/entry_preview.test.mjs: "defaults ON when nothing is stored" forced the flag ON and then asserted it was ON — a tautology that never touched the localStorage default (the getter's cache is seeded by import). It now probes a fresh module instance per case, covering both the empty-store default and the stored-'0' read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… untrack node_modules Drains the four CodeRabbit findings on #256. - input.js: advance the entry caret from the note's SNAPPED time, not the raw cursor. After a free (Alt) scrub — which this same PR introduces — the cursor sits off-grid, so stepping from it left the next note a fraction of a step off instead of flush against the last. Also drop the `step > 0` ternary: _editorSnapStepSeconds() is Math.max(0.001, ...) on every path, never <= 0. - ruler.js: read e.altKey on each scrub move instead of freezing it at mouse-down, matching how the loopedge/barsel drags already re-read e.shiftKey per move. - .gitignore + node_modules: the tracked absolute symlink is the symptom; the cause is that `node_modules/` (trailing slash) only matches directories, so the worktree symlink was never ignored and a `git add -A` swept it in. Drop the slash so the rule covers both, and untrack the blob. - CHANGELOG: the entry-preview toggle lives under Tempo/Grid > Snap, not View. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Not a CodeRabbit finding — fallout from this PR's own two features meeting. The caret cell drew at the RAW cursor while _editorPlaceAtCaret places at snapTime(cursor). With snap on and the cursor off-grid — after the free (Alt) scrub this same PR adds, or mid-playback — the dashed box promised a position the typed note would not take. The changelog sells this cell as showing "which note lands and how long, not just where"; it has to be right about where. host.snapTime is already wired (main.js), so the cell just snaps like the placement does. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/editor-note-entry-preview
branch
from
July 14, 2026 18:59
000379c to
91b6fa2
Compare
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
Two small entry-usability wins that make the keyboard note-entry caret actually inform you, and make the playhead behave like a DAW's.
1. The entry caret previews the note you'll type
In String view with nothing selected, the dashed cell that marks the entry point used to be a fixed-width box that showed only where a note would land — no more than the playhead marker already tells you. Now it earns its note shape:
Math.max(MIN_NOTE_W, sustain * zoom)), so the box is the footprint the note will fill.Typed notes are now placed at that same length (
sustain = snap step) instead ofsustain: 0, so consecutive0–9entries tile the grid instead of stacking as zero-length notes.It's a persisted view pref — View ▸ Snap ▸ Note-entry preview toggles it off (and back on) for mouse-first charters who find the cell distracting. Default on.
2. Clicking the ruler snaps the playhead to the grid (Logic-style)
A scrub click on the timeline ruler now lands the playhead on the nearest beat/subdivision when snap is on, so the entry caret sits on a real note position — instead of seeking to the raw pixel time. Hold Alt for a free, un-snapped scrub. Respects the existing snap toggle/mode; no change when snap is off.
Implementation
src/draw.js—_caretCellWidthPure(stepSec, zoom, minW)(exported, pure; mirrors note render width, floors to the visible minimum, NaN-safe) + a richer caret block (faint fill, fret ghost, gated onhost.editorEntryPreviewEnabled()).src/input.js— place-at-caret usessustain = snap stepand remembers the fret;_editorEntryPreviewEnabled/editorToggleEntryPreview(localStorageeditorEntryPreview, default on).src/ruler.js—scrubTosnaps viasnapTime; scrub drag carriesbypassSnap = e.altKey.src/host.js,src/main.js— wireeditorEntryPreviewEnabled+window.editorToggleEntryPreview.src/menu-bar.js— the View ▸ Snap toggle row.Tests
tests/entry_preview.test.mjs(new): the width helper (note-value sizing, minimum floor, garbage/NaN degradation, monotonic-with-step) + the persisted toggle (default on, flip persists to localStorage, explicit-force idempotence).npm test), lint clean (npx eslint@9.39.4 src tests— 0 errors), noroutes.pychange.cursor === snapTime(cursor)), Alt-click is free.🤖 Generated with Claude Code
Summary by CodeRabbit