feat(editor): editable note Time in the inspector + undoable inspector edits - #106
Merged
Conversation
…r edits The selection inspector showed Time read-only and let you edit Sustain by mutating notes in place with NO undo. Now: - A "Time (s)" number field sets the selected note's start position to a precise onset (millisecond alignment to the recording) — the common single-note case is "move this note to exactly T"; a multi-select sets all to the same absolute time, matching the Sustain field's set-all semantics. - Both numeric edits route through the undo history: Sustain via ResizeSustainGroupCmd (previously a raw n.sustain = v with no undo), Time via MoveNoteCmd (absolute target converted to per-note deltas; no re-sort, same as _editorResnapSelection, and hitNote is a linear scan so note order isn't load-bearing). editorInspectorSetField now works in note INDICES (via _editorCurrentNoteIndices) so it can build the commands. - _INSPECTOR_BOUNDS gains a `time` entry (min 0, non-integer) reusing the existing _coerceInspectorNumber clamp/parse. Tests: tests/inspector_time.test.js (5 cases) drive the REAL command classes through the REAL EditHistory (no stub of the subject) with an injected notes(): sustain and time exec -> rollback assert deep-equality of the note array, exec -> rollback -> redo reproduces, single-note move, already-at-target zero-delta no-op; the real _INSPECTOR_BOUNDS.time is fed through the real _coerceInspectorNumber with adversarial inputs (negative clamps to 0, junk/junk-tail/empty reject, whitespace trims). The time-bounds assertions fail on main (no `time` key there). node --check clean; all 26 JS test files pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
📝 WalkthroughWalkthroughAdds a Time (s) inspector field for editing note start times, extends inspector bounds to validate it, and routes sustain and time edits through undo-history commands instead of direct mutation. New tests cover the new field, coercion, and undo/redo behavior. ChangesInspector Time Editing and Undo Fix
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 |
![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 (2)
screen.js (1)
6044-6058: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo-op edits still push an undo entry.
When the entered value already matches every selected note's current sustain/time,
ResizeSustainGroupCmd/MoveNoteCmdstill execute (all-zero deltas / identical values), adding a functionally empty step to the undo stack. Consider skippingexecwhen nothing actually changes.Possible guard
if (field === 'sustain') { + if (idxs.every(i => (nn[i] ? nn[i].sustain : v) === v)) { draw(); updateStatus(); return; } S.history.exec(new ResizeSustainGroupCmd(idxs, idxs.map(() => v))); } else if (field === 'time') { const dtimes = idxs.map(i => v - (nn[i] ? nn[i].time : 0)); + if (dtimes.every(d => d === 0)) { draw(); updateStatus(); return; } S.history.exec(new MoveNoteCmd(idxs, dtimes, idxs.map(() => 0), 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 `@screen.js` around lines 6044 - 6058, The undo path in the numeric edit handler is still executing empty commands when the selected notes already match the entered sustain or time value. In the branch that uses ResizeSustainGroupCmd and MoveNoteCmd, compare the proposed values against the current notes from notes() and skip S.history.exec when all sustain values or computed dtimes are effectively no-ops; keep the existing set-all behavior for real changes only.tests/inspector_time.test.js (1)
47-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the dispatcher path in the inspector tests
tests/inspector_time.test.jsstill drivesMoveNoteCmd/ResizeSustainGroupCmddirectly, so it won’t catch bugs inwindow.editorInspectorSetFielditself: note selection, field routing, or thetimedelta calculation. If feasible, pull the dispatcher into the harness too and stubS.history/_editorCurrentNoteIndicesso the tests exercise the real branch logic.🤖 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/inspector_time.test.js` around lines 47 - 59, The inspector time test currently bypasses the dispatcher by instantiating MoveNoteCmd and ResizeSustainGroupCmd directly, so it misses bugs in window.editorInspectorSetField routing and time delta handling. Update the harness to invoke the dispatcher path itself, and stub S.history and _editorCurrentNoteIndices so the test can exercise the real selection/field-branch logic while still validating the time update behavior.
🤖 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 5875-5879: The time input in the inspector uses a step that does
not match the stated millisecond precision. Update the numeric input in
screen.js where the shared time field is rendered so its step value matches 1ms
granularity, and keep the existing onchange handler and min/value behavior
intact. This change should be made in the input markup associated with the
sharedTime field so typed values are accepted consistently with the precision
claim.
---
Nitpick comments:
In `@screen.js`:
- Around line 6044-6058: The undo path in the numeric edit handler is still
executing empty commands when the selected notes already match the entered
sustain or time value. In the branch that uses ResizeSustainGroupCmd and
MoveNoteCmd, compare the proposed values against the current notes from notes()
and skip S.history.exec when all sustain values or computed dtimes are
effectively no-ops; keep the existing set-all behavior for real changes only.
In `@tests/inspector_time.test.js`:
- Around line 47-59: The inspector time test currently bypasses the dispatcher
by instantiating MoveNoteCmd and ResizeSustainGroupCmd directly, so it misses
bugs in window.editorInspectorSetField routing and time delta handling. Update
the harness to invoke the dispatcher path itself, and stub S.history and
_editorCurrentNoteIndices so the test can exercise the real
selection/field-branch logic while still validating the time update 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: ebbc18a0-3a50-4d39-b42e-b958c317c8fa
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/inspector_time.test.js
Comment on lines
+5875
to
+5879
| <input type="number" min="0" step="0.01" value="${inputVal(sharedTime)}" | ||
| placeholder="${sharedTime === null ? 'mixed' : ''}" | ||
| onchange="editorInspectorSetField('time', this.value)" | ||
| class="flex-1 bg-dark-700 border border-gray-700 rounded px-1 py-0.5 text-xs" | ||
| title="Set the note's start time in seconds (for aligning to the recording)"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Step increment doesn't match "millisecond precision" claim.
step="0.01" is 10ms granularity, but the PR description states the field supports millisecond precision. Browsers may also flag typed values not aligned to the step as invalid. Consider step="0.001" if 1ms granularity is intended.
Suggested fix
- <input type="number" min="0" step="0.01" value="${inputVal(sharedTime)}"
+ <input type="number" min="0" step="0.001" value="${inputVal(sharedTime)}"📝 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.
Suggested change
| <input type="number" min="0" step="0.01" value="${inputVal(sharedTime)}" | |
| placeholder="${sharedTime === null ? 'mixed' : ''}" | |
| onchange="editorInspectorSetField('time', this.value)" | |
| class="flex-1 bg-dark-700 border border-gray-700 rounded px-1 py-0.5 text-xs" | |
| title="Set the note's start time in seconds (for aligning to the recording)"> | |
| <input type="number" min="0" step="0.001" value="${inputVal(sharedTime)}" | |
| placeholder="${sharedTime === null ? 'mixed' : ''}" | |
| onchange="editorInspectorSetField('time', this.value)" | |
| class="flex-1 bg-dark-700 border border-gray-700 rounded px-1 py-0.5 text-xs" | |
| title="Set the note's start time in seconds (for aligning to the recording)"> |
🤖 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.js` around lines 5875 - 5879, The time input in the inspector uses a
step that does not match the stated millisecond precision. Update the numeric
input in screen.js where the shared time field is rendered so its step value
matches 1ms granularity, and keep the existing onchange handler and min/value
behavior intact. This change should be made in the input markup associated with
the sharedTime field so typed values are accepted consistently with the
precision claim.
The existing inspector_time cases call MoveNoteCmd/ResizeSustainGroupCmd directly and reconstruct the `target - note.time` delta in the test, so the function the PR actually adds — window.editorInspectorSetField — was never exercised. A regression in the dispatcher (wrong delta sign, wrong field routing, double-exec) would pass every prior case. Extracts the real dispatcher (plus _editorCurrentNoteIndices and the command classes) via the same brace-match/@pure convention and runs it against a minimal stubbed S/notes/window/_renderInspector. New cases: - time sets the note to the exact absolute value and grows history by exactly ONE entry (single + multi-select set-all); - sustain routes to ResizeSustainGroupCmd, leaving time untouched (correct field routing); - junk ("2.5x") and empty input are rejected as no-ops that re-render and push no history; - empty selection early-returns before bounds/history/render; - a float round-trip (onset 0.1 -> target 2.0) documents the sub-ULP undo drift: the move lands exactly on 2.0 but the undo restores 0.10000000000000009, so the PR's "restores the exact array" holds only where the arithmetic is exact. Asserted with a tolerance. Test-only; no production logic changed. Full node --test glob green (26). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/inspector_time.test.js (1)
131-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSecond extraction harness duplicates class/bounds extraction from the first harness.
This harness re-extracts
MoveNoteCmd,ResizeSustainGroupCmd,_INSPECTOR_BOUNDS, and_coerceInspectorNumberinto its ownnew Functionscope, mirroring the setup in the earlier command-class harness. This duplication is inherent to needing a self-contained scope forwindow.editorInspectorSetField, but the marker-extraction/bootstrap plumbing itself could be factored into a small shared helper (e.g.buildHarness(extraExtracts, injectedGlobals)) to avoid maintaining two near-identicalnew Functionassembly blocks.🤖 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/inspector_time.test.js` around lines 131 - 165, This inspector-time test harness repeats the same extraction/bootstrap logic for MoveNoteCmd, ResizeSustainGroupCmd, _INSPECTOR_BOUNDS, and _coerceInspectorNumber that already exists in the earlier harness. Refactor the shared new Function setup into a small helper such as buildHarness(extraExtracts, injectedGlobals), and reuse it here to assemble the window.editorInspectorSetField dispatcher scope without duplicating the marker-extraction plumbing.
🤖 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/inspector_time.test.js`:
- Around line 131-165: This inspector-time test harness repeats the same
extraction/bootstrap logic for MoveNoteCmd, ResizeSustainGroupCmd,
_INSPECTOR_BOUNDS, and _coerceInspectorNumber that already exists in the earlier
harness. Refactor the shared new Function setup into a small helper such as
buildHarness(extraExtracts, injectedGlobals), and reuse it here to assemble the
window.editorInspectorSetField dispatcher scope without duplicating the
marker-extraction plumbing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 999fdca1-d40f-42ec-a991-db077bccb26e
📒 Files selected for processing (1)
tests/inspector_time.test.js
This was referenced Jul 9, 2026
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.
Summary
Two related wins in the selection inspector, in a region no open PR touches:
n.sustain = v) with no undo entry — a mistyped value couldn't be taken back. Both edits now go through the undo history: Sustain viaResizeSustainGroupCmd, Time viaMoveNoteCmd(absolute target → per-note deltas; no re-sort, exactly like_editorResnapSelection, andhitNoteis a linear scan so note order isn't load-bearing)._INSPECTOR_BOUNDSgains atimeentry (min 0, non-integer) reusing the existing_coerceInspectorNumberclamp/parse.Verification — held to the new testing habits
tests/inspector_time.test.js(5 cases) drive the realResizeSustainGroupCmd/MoveNoteCmdthrough the realEditHistory(brace-extracted +@pure:edit-history, injectednotes()— no stub of the subject).exec → rollbackassert deep-equality of the note array,exec → rollback → redoreproduces; single-note move; a note already at the target gets a zero delta (clean no-op)._INSPECTOR_BOUNDS.timeis fed through the real_coerceInspectorNumberwith adversarial inputs (negative clamps to 0; junk / junk-tail / empty reject; whitespace trims) — and these assertions fail on main (notimekey exists there).node --checkclean; all 26 JS test files pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit
New Features
Bug Fixes