Skip to content

fix(editor): make inspector technique edits undoable #243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
byrongamatos merged 1 commit into from
Jul 13, 2026
Merged

fix(editor): make inspector technique edits undoable #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- **Inspector technique edits are undoable now.** Toggling a technique flag
(Palm Mute, Hammer-On, Tap, …) or setting a bend/slide value from the
inspector panel used to mutate the note in place with no undo — so Ctrl+Z
couldn't take it back, even though the same toggle from the keyboard could.
Both paths now commit through the editor's undo history (the flags via the
same command the keyboard toggles use; bend/slide via a new command that also
carries any authored bend curve through the edit), so a technique tweak is one
Ctrl+Z like a fret or time change. They still refuse on a read-only piano roll.
- **Author credits now match the feedpak spec.** The manifest `authors:` array
was written as plain strings; the spec (§5.4) requires objects with a `name`
(plus optional `role`), so string credits failed schema validation and the
Expand Down
51 changes: 50 additions & 1 deletion src/commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { _flattenArrChords, _mergeChordFn } from './chords.js';
import { host } from './host.js';
import { PIANO_LANE_H, _rollPitchCtx, isKeysArr } from './keys.js';
import { _openMidiForArr, _soundingPitchPure, _stringCountFor } from './lanes.js';
import { _clearSuggested, _isSuggested, _markSuggested, notes } from './notes.js';
import { _clearSuggested, _isSuggested, _markSuggested, notes, rescaleBendCurveToPeak, sanitizeBendCurve } from './notes.js';
import {
_absolutePitch, _cyclePositionCandidatesPure, _cycleStepPure, _suggestPositionPure,
} from './position.js';
Expand Down Expand Up @@ -264,6 +264,55 @@ export class ToggleTechniqueCmd {
}
}

// Set a SCALAR technique (`bend` peak, `slide_to`, `slide_unpitch_to`) to an
// absolute value across a selection as one undoable edit — the inspector's
// numeric technique inputs, which used to mutate n.techniques in place with no
// undo (the documented PR3b trap). Snapshots the prior scalar per note; for
// `bend` it also carries the authored curve (bend_values) through the same
// rescale-to-new-peak the in-place path did, and snapshots it so undo restores
// the exact prior shape. Applies to every selected note (the inspector's
// "set all" semantics) as a single Ctrl+Z.
export class SetTechScalarCmd {
constructor(indices, key, value) {
this.indices = indices.slice();
this.key = key;
this.value = value;
this.old = this.indices.map(i => {
const t = notes()[i].techniques || {};
// bend_values is only touched for `bend`; snapshot it defensively so
// undo restores the exact curve even when the rescale nulls it.
return { v: t[key], bnv: t.bend_values };
});
}
exec() {
for (const i of this.indices) {
const n = notes()[i];
if (!n.techniques) n.techniques = {};
n.techniques[this.key] = this.value;
// Editing the scalar peak must keep any authored curve consistent
// (renderers/graders read bnv as authoritative): rescale the curve to
// the new peak, or drop it when the peak is 0 / the curve is unscalable.
if (this.key === 'bend' && sanitizeBendCurve(n.techniques.bend_values)) {
const scaled = this.value > 0
? rescaleBendCurveToPeak(n.techniques.bend_values, this.value)
: null;
n.techniques.bend_values = scaled;
// bnv rounds points to 0.1, so a non-0.1 `value` (e.g. 0.25) would
// leave bn disagreeing with the curve's real peak. Snap bn to it.
if (scaled) n.techniques.bend = scaled.reduce((m, p) => Math.max(m, p.v), 0);
}
}
}
rollback() {
this.indices.forEach((i, k) => {
const n = notes()[i];
if (!n.techniques) n.techniques = {};
n.techniques[this.key] = this.old[k].v;
if (this.key === 'bend') n.techniques.bend_values = this.old[k].bnv;
});
}
}

// Set the full bend shape (peak `bend`, intent `bend_intent`, curve
// `bend_values` — §6.2.1) on one or more notes as a single undoable edit.
// Snapshots the prior bend triple per note so undo restores it exactly.
Expand Down
80 changes: 34 additions & 46 deletions src/inspector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,15 @@
// techniques, bend intent, teaching marks) and, when the selection is a chord,
// its name / voicing / fingering / function.
//
// MOST edits commit through a command in src/commands.js and are undoable:
// EVERY edit commits through a command in src/commands.js and is undoable:
// time/sustain (MoveNoteCmd, ResizeSustainGroupCmd), bend intent, the chord
// patches, and everything routed through _applyTeachingMark — fret finger,
// scale degree, strum grouping.
//
// The technique toggles (editorInspectorSetTech) and the boolean flags
// (editorInspectorSetFlag) are the exception: they still mutate n.techniques in
// place and are NOT undoable, a deliberate scope limit from PR3b. Both DO honour
// the read-only-roll lock, so they cannot silently write a chart the roll is
// showing read-only.
// patches, everything routed through _applyTeachingMark (fret finger, scale
// degree, strum grouping), the scalar technique inputs (editorInspectorSetTech
// → SetTechScalarCmd), and the boolean flags (editorInspectorSetFlag →
// ToggleTechniqueCmd, the same command the keyboard toggles use). The last two
// used to mutate n.techniques in place with no undo — a documented PR3b trap,
// now closed. All still honour the read-only-roll lock, so they cannot write a
// chart the roll is showing read-only.
//
// It renders innerHTML and reads back through the window.editorInspector* and
// window.editorChord* handlers that markup calls. Those are exported as plain
Expand All @@ -29,13 +28,13 @@ import {
} from './chords.js';
import {
EditChordFnCmd, MoveNoteCmd, ResizeSustainGroupCmd, SetBendIntentCmd, SetTeachingMarkCmd,
SetTechScalarCmd, ToggleTechniqueCmd,
} from './commands.js';
import { host } from './host.js';
import { _rollLockNotice, _rollReadOnly } from './keys.js';
import { lanes } from './lanes.js';
import {
BEND_INTENTS, FRET_FINGER_OPTIONS, nextUnusedStrumGroup, notes, rescaleBendCurveToPeak,
sanitizeBendCurve,
BEND_INTENTS, FRET_FINGER_OPTIONS, nextUnusedStrumGroup, notes,
} from './notes.js';
import { S } from './state.js';

Expand Down Expand Up @@ -279,12 +278,13 @@ export function _renderInspector() {
// Inspector mutators. All operate on the full S.sel so a multi-select edit
// applies bulk-style.
//
// The TECHNIQUE toggles below (editorInspectorSetTech, editorInspectorSetFlag)
// skip the undo history — PR3b kept that scope tight, and a TechBulkCmd lands
// when the inspector grows to need richer per-edit undo. Everything else in this
// file commits through a command: setField, setBendIntent, the chord patches,
// and _applyTeachingMark's SetTeachingMarkCmd. (This comment used to say "edits"
// without qualification, which stopped being true once those landed.)
// The TECHNIQUE edits below now commit through the undo history like everything
// else in this file: editorInspectorSetTech via SetTechScalarCmd (bend peak /
// slide targets, carrying the authored bend curve through the rescale) and
// editorInspectorSetFlag via ToggleTechniqueCmd (the boolean flags, sharing the
// command the keyboard toggles use). They used to mutate n.techniques in place
// with no undo — the PR3b scope limit, closed here so a technique tweak is one
// Ctrl+Z like a fret or time edit.

// Bounds for the inspector's numeric inputs. Mirrors the limits the
// prompt-based editors (`promptFret`, `promptSlide`, `promptBend`)
Expand Down Expand Up @@ -370,11 +370,10 @@ export function editorInspectorSetField(field, raw) {
}

export function editorInspectorSetTech(key, raw) {
const sel = _selectedNotes();
if (sel.length === 0) return;
// Read-only roll (V4): scalar technique edits mutate n.techniques in
// place (no EditHistory command), so the exec lock never sees them.
// Refuse and bounce the input back to the model value.
const idxs = [...(S.sel || [])];
if (!idxs.length) return;
// Read-only roll (V4): a scalar technique edit would write n.techniques on a
// chart the roll is showing read-only. Refuse and bounce the input back.
if (_rollReadOnly()) { _rollLockNotice(); _renderInspector(); return; }
const bounds = _INSPECTOR_BOUNDS[key];
if (!bounds) return;
Expand All @@ -386,22 +385,10 @@ export function editorInspectorSetTech(key, raw) {
_renderInspector();
return;
}
for (const n of sel) {
if (!n.techniques) n.techniques = {};
n.techniques[key] = v;
// Editing the scalar peak must keep any authored curve consistent
// (renderers/graders read bnv as authoritative): rescale the curve to
// the new peak, or drop it when the peak is 0 / the curve is unscalable.
if (key === 'bend' && sanitizeBendCurve(n.techniques.bend_values)) {
const scaled = v > 0
? rescaleBendCurveToPeak(n.techniques.bend_values, v)
: null;
n.techniques.bend_values = scaled;
// bnv rounds points to 0.1, so a non-0.1 `v` (e.g. 0.25) would leave
// bn disagreeing with the curve's real peak. Snap bn to the curve.
if (scaled) n.techniques.bend = scaled.reduce((m, p) => Math.max(m, p.v), 0);
}
}
// 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();
}
Comment on lines +388 to 394

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.js

Repository: 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" src

Repository: 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.js

Repository: 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.js

Repository: 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.

Expand All @@ -424,17 +411,18 @@ export function editorOpenBendCurve() {
}

export function editorInspectorSetFlag(key, on) {
const sel = _selectedNotes();
if (sel.length === 0) return;
// Read-only roll (V4): flag toggles mutate n.techniques directly — same
// bypass as editorInspectorSetTech. Refuse and re-render to reset the box.
const idxs = [...(S.sel || [])];
if (!idxs.length) return;
// Read-only roll (V4): a flag toggle would write n.techniques on a chart the
// roll is showing read-only. Refuse and re-render to reset the checkbox.
if (_rollReadOnly()) { _rollLockNotice(); _renderInspector(); return; }
for (const n of sel) {
if (!n.techniques) n.techniques = {};
n.techniques[key] = !!on;
}
// Route through the same command the keyboard technique toggles use, so an
// inspector checkbox is one undoable step across the whole selection.
S.history.exec(new ToggleTechniqueCmd(idxs, key, !!on));
host.draw();
host.updateStatus();
// Reflect the committed value (a mixed selection becomes all-on/all-off).
_renderInspector();
}

// ─── Teaching marks (§6.2.2) ────────────────────────────────────────
Expand Down
Loading
Loading