fix(editor): make inspector technique edits undoable - #243
Merged
Merged
Conversation
Toggling a technique flag (palm mute, hammer-on, tap, …) or setting a bend peak / slide target from the inspector panel mutated n.techniques in place with NO undo — the documented PR3b trap: a keyboard technique toggle undid, the inspector click didn't. Both paths now commit through the undo history like every other inspector edit. editorInspectorSetFlag routes through ToggleTechniqueCmd (the same command the keyboard toggles use); editorInspectorSetTech routes through a new SetTechScalarCmd, which sets the scalar across the selection and carries any authored bend curve through the rescale-to-new-peak (snapshotting it so undo restores the exact prior shape). Both keep the read-only-roll refusal. So an inspector technique tweak is a single Ctrl+Z, matching a fret or time edit. The two module comments that documented the trap are updated to say it is closed. Tests: tests/inspector_undo.test.mjs (11) — SetTechScalarCmd round-trips (slide targets; bend with curve rescale; bend→0 drops the curve), plus the real dispatchers driven through a stubbed host/S for command routing, the one-commit guarantee, reject-bounce, and the read-only-roll refusal. view_switcher.test.mjs's inspector-flag lock test updated for the new path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
📝 WalkthroughWalkthroughInspector technique scalar and flag edits now use undoable commands. Bend curves are rescaled and restored with bend peaks, read-only rolls still reject edits, and tests cover command behavior, dispatcher routing, validation, and locking. ChangesInspector technique undo flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Inspector
participant EditHistory
participant TechniqueCommand
participant Notes
User->>Inspector: edit technique value or flag
Inspector->>Inspector: validate selection and roll lock
Inspector->>EditHistory: execute technique command
EditHistory->>TechniqueCommand: apply selection update
TechniqueCommand->>Notes: update techniques and bend curve
EditHistory-->>Inspector: committed history state
Inspector-->>User: re-render inspector
Possibly related PRs
🚥 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
🤖 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/inspector.js`:
- Around line 388-394: After executing SetTechScalarCmd in the bend-edit setter,
call _renderInspector() before or alongside the existing host.draw() and
host.updateStatus() calls so the inspector reflects the snapped curve peak
immediately. Keep the single-command undo behavior 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: 924d4c3b-e4c9-44d1-9b8a-5f3a9256797d
📒 Files selected for processing (5)
CHANGELOG.mdsrc/commands.jssrc/inspector.jstests/inspector_undo.test.mjstests/view_switcher.test.mjs
Comment on lines
+388
to
394
| // One undoable command for the whole selection (the "set all" semantic); | ||
| // SetTechScalarCmd carries the bend-curve rescale, so a bend peak edit is a | ||
| // single Ctrl+Z that also restores any authored curve. | ||
| S.history.exec(new SetTechScalarCmd(idxs, key, v)); | ||
| host.draw(); | ||
| host.updateStatus(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/inspector.js --items all \
--match '_INSPECTOR_BOUNDS|_coerceInspectorNumber' --view expanded
sed -n '288,394p' src/inspector.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 5227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tech command implementation and inspector render helpers.
ast-grep outline src/inspector.js --view expanded
printf '\n--- SetTechScalarCmd references ---\n'
rg -n "class SetTechScalarCmd|SetTechScalarCmd\\(|_renderInspector\\(|function _renderInspector|const _renderInspector|export function _renderInspector" src -S
printf '\n--- Relevant file slices ---\n'
sed -n '1,260p' src/inspector.js
printf '\n--- tech command file(s) ---\n'
fd -a "SetTechScalarCmd|tech" srcRepository: got-feedBack/feedBack-plugin-editor
Length of output: 17811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the command behavior around bend scalar edits and any render hook.
sed -n '240,360p' src/commands.js
printf '\n--- bend normalization references ---\n'
rg -n "bend|round|normalize|rescale|peak" src/commands.js src -S
printf '\n--- inspector tech setter slice ---\n'
sed -n '372,402p' src/inspector.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether draw/updateStatus re-render the inspector or only the canvas/status.
sed -n '1,220p' src/host.js
printf '\n--- main draw/updateStatus wiring ---\n'
rg -n "host\\.draw|host\\.updateStatus|_renderInspector\\(" src/main.js src/*.js -S | head -n 120
printf '\n--- inspector setter context ---\n'
sed -n '372,395p' src/inspector.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 15997
Re-render the inspector after bend edits.
SetTechScalarCmd can snap bend to the curve’s rounded peak, but this path still leaves the panel showing the submitted value until the next explicit inspector refresh. Call _renderInspector() here, like the other inspector setters.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 391-391: 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/inspector.js` around lines 388 - 394, After executing SetTechScalarCmd in
the bend-edit setter, call _renderInspector() before or alongside the existing
host.draw() and host.updateStatus() calls so the inspector reflects the snapped
curve peak immediately. Keep the single-command undo behavior unchanged.
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
Toggling a technique flag (Palm Mute, Hammer-On, Tap, …) or setting a bend peak / slide target from the inspector panel mutated
n.techniquesin place with no undo — the documented PR3b trap (inspector.jstop comment): the same technique toggle from the keyboard undid, the inspector click didn't. Gap-audit #3.Approach
Both inspector paths now commit through the undo history, like every other inspector edit:
editorInspectorSetFlag→ToggleTechniqueCmd(the exact command the keyboard technique toggles already use).editorInspectorSetTech→ newSetTechScalarCmd— sets the scalar (bend/slide_to/slide_unpitch_to) across the whole selection as one command, and forbendcarries any authored curve (bend_values) through the same rescale-to-new-peak the in-place path did, snapshotting it so undo restores the exact prior shape.Both keep the read-only-roll refusal (a locked fretted roll still can't be written), and both index off
[...S.sel]like the siblingeditorInspectorSetBendIntent/ teaching-mark handlers. So an inspector technique tweak is a single Ctrl+Z, matching a fret or time edit. The two module comments that documented the trap are updated to say it's closed.Tests
tests/inspector_undo.test.mjs(11) —SetTechScalarCmdround-trips through the realEditHistory(slide targets;bendwith curve rescale;bend→0drops the curve; exec→rollback deep-equality→redo), plus the real dispatchers extracted and driven through a stubbed host/S: command routing, the "exactly one history entry per commit" guarantee, the junk-input reject-bounce, and the read-only-roll refusal for both handlers.view_switcher.test.mjs's inspector-flag read-only-roll test updated for the new command path (now also asserts no commit under the lock, one commit once editable).routes.pyuntouched (no pytest).editorInspectorSetFlag('palm_mute', true)+editorInspectorSetTech('slide_to', '7')grows the undo stack 0→1→2; two undos revert both exactly (slide→-1, palm_mute→false, stack→0); redo restores them.🤖 Generated with Claude Code
Summary by CodeRabbit