feat(editor): make section add/rename/delete undoable - #104
Merged
Conversation
Adding (Shift+M / the beat-bar right-click menu), renaming, and deleting a section mutated S.sections raw (push/splice/direct assignment) with no undo — a stray Delete on a section was unrecoverable, and Ctrl+Z after adding one rolled back the last NOTE edit instead. Three command classes (AddSectionCmd / RemoveSectionCmd / RenameSectionCmd) route those mutations through EditHistory. They hold the section OBJECT by reference (the array re-sorts, so indices go stale but refs survive) and restore S.sections exactly on undo/redo. RemoveSectionCmd captures the removed index and splices the section back at that exact slot on rollback, so a section restored beside an equal-start_time sibling returns to its original order (LIFO guarantees the array at rollback matches the state exec left). RenameSectionCmd is name-only, matching the prior behavior — just now undoable. The context menu's "near a section" lookup shares one pure helper (_sectionNearestIndexPure) with the tests. Tests: tests/section_undo.test.js (7 cases) drive the REAL section commands through the REAL EditHistory (no stub of the subject); exec -> rollback asserts deep-equality of the whole S.sections array (sort order included) and exec -> rollback -> redo reproduces; adversarial equal-start_time delete/restore, out-of-order insert, LIFO interleaving; the helper is proven to key off its time/tol arguments. These fail on main (the command classes don't exist 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
|
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 (1)
📝 WalkthroughWalkthroughSection editing actions now go through undo/redo-aware commands instead of mutating ChangesSection Undo/Redo Commands
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)
tests/section_undo.test.js (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring claims "delete-nonexistent" coverage that doesn't exist.
The header comment (Lines 14-16) states adversarial inputs including "delete-nonexistent" are covered, but none of the 7 test cases exercises
RemoveSectionCmdon a section not present inS.sections— theidx === -1branches inexec()/rollback()(screen.js Lines 3801-3813) remain untested.🧪 Suggested missing test
t('delete: removing a section absent from S.sections is a no-op; rollback falls back to sorted insert', () => { const s1 = { name: 'a', number: 1, start_time: 0 }; const ghost = { name: 'ghost', number: 1, start_time: 5 }; const env = makeEnv([s1]); env.S.history.exec(new env.RemoveSectionCmd(ghost)); assert.deepStrictEqual(env.S.sections.map(s => s.name), ['a'], 'no-op when section absent at exec'); env.S.history.doUndo(); assert.deepStrictEqual(env.S.sections.map(s => s.name), ['a', 'ghost'], 'rollback falls back to sorted insert'); });Also applies to: 86-98
🤖 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/section_undo.test.js` around lines 14 - 16, The test docstring overstates coverage by claiming “delete-nonexistent” is exercised, but `RemoveSectionCmd`’s missing-section paths in `exec()` and `rollback()` are not covered. Update the header comment in the `section_undo` test suite to accurately reflect the cases actually covered, or add a new test that calls `env.S.history.exec(new env.RemoveSectionCmd(...))` with a section absent from `S.sections` and verifies `doUndo()` restores via sorted insert.screen.js (1)
3829-3833: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated section-number computation.
S.sections.filter(s => s.name === name).length + 1is repeated here and in the "add" handler (Line 4251). Consider extracting a small_nextSectionNumber(name)helper.🤖 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 3829 - 3833, The section-number calculation is duplicated in _editorAddSectionAtCursor and the add handler, so extract the shared logic into a small _nextSectionNumber(name) helper and use it in both places. Keep the helper responsible for computing S.sections.filter(...) length + 1 so the AddSectionCmd call sites only request the next number for the given section name.
🤖 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 3777-3788: _update _sectionNearestIndexPure so it selects the
section with the smallest absolute distance to time rather than returning the
first item within tol. Keep the existing argument-based behavior and use the
current function name _sectionNearestIndexPure as the place to change the search
logic. Iterate through sections, track the best index and minimum distance, and
only return an index when the closest section is within the tolerance; otherwise
return -1._
---
Nitpick comments:
In `@screen.js`:
- Around line 3829-3833: The section-number calculation is duplicated in
_editorAddSectionAtCursor and the add handler, so extract the shared logic into
a small _nextSectionNumber(name) helper and use it in both places. Keep the
helper responsible for computing S.sections.filter(...) length + 1 so the
AddSectionCmd call sites only request the next number for the given section
name.
In `@tests/section_undo.test.js`:
- Around line 14-16: The test docstring overstates coverage by claiming
“delete-nonexistent” is exercised, but `RemoveSectionCmd`’s missing-section
paths in `exec()` and `rollback()` are not covered. Update the header comment in
the `section_undo` test suite to accurately reflect the cases actually covered,
or add a new test that calls `env.S.history.exec(new env.RemoveSectionCmd(...))`
with a section absent from `S.sections` and verifies `doUndo()` restores via
sorted insert.
🪄 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: e0ec5885-a6a1-4ee7-aa68-a7eccac9a0fe
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/section_undo.test.js
Comment on lines
+3777
to
+3788
| // Index of the first section within `tol` seconds of `time`, else -1 — | ||
| // keyed off the ARGUMENTS, never a global cursor (the section context menu | ||
| // tests a click time against each section here). | ||
| function _sectionNearestIndexPure(sections, time, tol) { | ||
| if (!Array.isArray(sections)) return -1; | ||
| const t = Number(time); | ||
| const w = Number.isFinite(tol) ? tol : 1.0; | ||
| for (let i = 0; i < sections.length; i++) { | ||
| if (Math.abs(Number(sections[i].start_time) - t) <= w) return i; | ||
| } | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_sectionNearestIndexPure returns first match, not nearest.
The loop returns the first index within tol, not the section with the smallest distance to time. If two sections both fall within tolerance of a click (plausible with closely-spaced sections and the 1.0s default), the farther one can win, causing rename/delete to target the wrong section.
🎯 Proposed fix to track minimum distance
function _sectionNearestIndexPure(sections, time, tol) {
if (!Array.isArray(sections)) return -1;
const t = Number(time);
const w = Number.isFinite(tol) ? tol : 1.0;
- for (let i = 0; i < sections.length; i++) {
- if (Math.abs(Number(sections[i].start_time) - t) <= w) return i;
- }
- return -1;
+ let best = -1, bestDist = Infinity;
+ for (let i = 0; i < sections.length; i++) {
+ const d = Math.abs(Number(sections[i].start_time) - t);
+ if (d <= w && d < bestDist) { best = i; bestDist = d; }
+ }
+ return best;
}📝 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
| // Index of the first section within `tol` seconds of `time`, else -1 — | |
| // keyed off the ARGUMENTS, never a global cursor (the section context menu | |
| // tests a click time against each section here). | |
| function _sectionNearestIndexPure(sections, time, tol) { | |
| if (!Array.isArray(sections)) return -1; | |
| const t = Number(time); | |
| const w = Number.isFinite(tol) ? tol : 1.0; | |
| for (let i = 0; i < sections.length; i++) { | |
| if (Math.abs(Number(sections[i].start_time) - t) <= w) return i; | |
| } | |
| return -1; | |
| } | |
| // Index of the first section within `tol` seconds of `time`, else -1 — | |
| // keyed off the ARGUMENTS, never a global cursor (the section context menu | |
| // tests a click time against each section here). | |
| function _sectionNearestIndexPure(sections, time, tol) { | |
| if (!Array.isArray(sections)) return -1; | |
| const t = Number(time); | |
| const w = Number.isFinite(tol) ? tol : 1.0; | |
| let best = -1, bestDist = Infinity; | |
| for (let i = 0; i < sections.length; i++) { | |
| const d = Math.abs(Number(sections[i].start_time) - t); | |
| if (d <= w && d < bestDist) { best = i; bestDist = d; } | |
| } | |
| return best; | |
| } |
🤖 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 3777 - 3788, _update _sectionNearestIndexPure so it
selects the section with the smallest absolute distance to time rather than
returning the first item within tol. Keep the existing argument-based behavior
and use the current function name _sectionNearestIndexPure as the place to
change the search logic. Iterate through sections, track the best index and
minimum distance, and only return an index when the closest section is within
the tolerance; otherwise return -1._
…ack clear The section_undo docstring advertised a delete-nonexistent adversarial case that did not exist. Add it: constructing RemoveSectionCmd with a section not in S.sections makes exec a no-op (idx stays -1), so undo takes the idx<0 sorted-insert fallback branch. The ghost's start_time is placed after every existing section so the sorted result diverges from a naive splice(-1, 0, ...), making the assertion actually discriminate the fallback. Also add a redo-stack-clear test: these are ordinary EditHistory commands, so a fresh exec() after an undo drops the redo stack (add -> undo -> rename leaves nothing to redo). Docstring reworded to scope the round-trip claim and flag the one deliberate non-round-trip (delete-nonexistent). No source change: S.sections is song-level state and the section commands already operate on it directly, matching the TempoGridCmd (S.beats) precedent. This plugin's EditHistory does no per-arrangement command tagging, so there is no songScope flag to set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 7, 2026
Resolve CHANGELOG.md [Unreleased] conflict (take-both: keep the key/scale entry alongside the section-undo/duplicate/inspector-time/coverage entries). Fix cross-PR integration break: #104-#107 landing together left EditHistory ._afterEdit() (in the @pure:edit-history block) bumping _coverageEditGen, which is declared outside that block — so the undo-test sandboxes that extract edit-history in isolation threw "_coverageEditGen is not defined". Guard the bump with typeof, matching the isKeysMode guard two lines below. Full JS suite 46/46 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Adding (Shift+M or the beat-bar right-click menu), renaming, and deleting a section all mutated
S.sectionsraw —push/splice/ direct assignment — with no undo. A stray Delete on a section was unrecoverable, and Ctrl+Z right after adding one rolled back the last note edit instead. (Same class of gap as the drum-undo fix in #89, in a different region.)Three command classes route those mutations through the editor's
EditHistory:AddSectionCmd— inserts sorted; rollback removes the ref.RemoveSectionCmd— captures the removed index and splices the section back at that exact slot on rollback, so a section restored beside an equal-start_timesibling returns to its original order (undo LIFO guarantees the array at rollback matches the stateexecleft).RenameSectionCmd— name-only, matching prior behavior; just now undoable.The context menu's "near a section" lookup now shares one pure helper (
_sectionNearestIndexPure) with the tests rather than an inline loop.Verification — held to the new testing habits
tests/section_undo.test.js(7 cases) drives the real section commands through the realEditHistory(extracted@pure:edit-history+@pure:section-cmds, S/draw injected — no stub of the subject).exec → rollbackasserts deep-equality of the wholeS.sectionsarray (sort order included), andexec → rollback → redoreproduces.start_time(ref-delete removes the right one, undo restores order — this is the case that caught and drove the index-capture fix), out-of-order insert lands sorted, LIFO interleaving of add+rename.time/tol→ different results; degeneratenull/[]/NaN).node --checkclean; all 26 JS test files pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit