Skip to content

refactor(editor): move EditHistory to src/history.js (R2, step 14) #165

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 9, 2026
Merged

refactor(editor): move EditHistory to src/history.js (R2, step 14) #165

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
21 changes: 21 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed

- **`EditHistory` now lives in `src/history.js` (R2, step 14).** 121 lines out of
`src/main.js`, which is down to 18,471. The 47 command classes stay put — they
are interleaved with the feature code that constructs them and each reaches
deep into it; only the stack lifts cleanly. Commands were always duck-typed
(`exec`, `rollback`, and the three opt-out flags), so nothing about them had to
change.
`history.js` imports `S`/`bumpEditGen` from `state.js` and the view predicates
from `keys.js`. Its remaining three main.js symbols — `_historyEnsureArr`,
`draw`, `updateStatus` — cannot be imported back without closing a cycle, so
they arrive through `setHistoryHooks()`, the same shape as `canvas.js`'s
`setCanvas()` and `geometry.js`'s `setLaneMetrics()`. The three duplicated
read-only-roll checks in `exec`/`doUndo`/`doRedo` collapse into one `_locked()`.
Thirteen suites used to slice the class out of `main.js` and hand it a
fabricated `S`, with `_rollReadOnly` stubbed to a boolean. They now import the
real class, seed the real `S`, and drive the real lock through real view state
(`tests/_history_env.mjs`). That turned up a stubbed lie: a keys-DATA part in
the roll had been forced read-only in `roll_position_cycle`, when a keys part
is never read-only. Six of them were CJS and are now `.mjs`.

### Fixed

- **Opening a song from the library card or the 3D highway left an invisible
Expand Down
132 changes: 132 additions & 0 deletions src/history.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
// ════════════════════════════════════════════════════════════════════
// Undo / redo stack.
//
// The 47 command classes still live in src/main.js — they are interleaved with
// the feature code that constructs them, and each reaches deep into it. Only
// the stack itself lifts cleanly. Every command is duck-typed: `exec()`,
// `rollback()`, and the three opt-out flags this file reads (`songScope`,
// `pitchPreserving`, `suggestResolved`).
//
// Browser surface: `document.getElementById` in _ui(), for the toolbar's
// undo/redo buttons.
//
// main.js coupling is three symbols — `_historyEnsureArr`, `draw`,
// `updateStatus` — and importing them would close a cycle (main.js imports this
// module). They arrive through setHistoryHooks() instead, the same shape as
// canvas.js's setCanvas() and geometry.js's setLaneMetrics().
// ════════════════════════════════════════════════════════════════════
import { S, bumpEditGen } from './state.js';
import { isKeysMode, updatePianoRange, _rollReadOnly, _rollLockNotice } from './keys.js';

// Cap the undo stack so a marathon session can't grow memory without bound —
// the stack held every command since the last save/load. Oldest entries drop
// first; 500 comfortably exceeds any realistic between-saves editing run.
export const MAX_UNDO = 500;

// Defaults keep the class usable before main.js wires it up (and in tests that
// only exercise the stack): ensureArr never refuses, the UI callbacks no-op.
const _hooks = {
ensureArr: () => true,
draw: () => {},
updateStatus: () => {},
};

export function setHistoryHooks(hooks) { Object.assign(_hooks, hooks); }

// A NOTE-scope command is refused while a fretted part is shown in the
// read-only piano roll (V4). Three carve-outs pass:
// songScope — edits song-level data (drum tab, tempo grid), not the
// fretted chart, so an unrelated part being in the roll
// must not freeze tempo/drum editing.
// pitchPreserving — the VA.5 position cycle and sustain resize can never
// change what a note SOUNDS like, only which string/fret
// plays it (or for how long), so the "no silent pitch
// writes" contract the lock protects is unbreakable here
// by construction.
// suggestResolved — the VA.3 suggest-position writer (resolved adds +
// Accept) IS the sanctioned string/fret write path the
// lock was holding the door for. It marks, never guesses.
// Nothing else opts out. Returns true when the command must not run.
function _locked(cmd) {
if (cmd.songScope === true || cmd.pitchPreserving === true || cmd.suggestResolved === true) return false;
if (!_rollReadOnly()) return false;
_rollLockNotice();
return true;
}

export class EditHistory {
constructor() { this.undo = []; this.redo = []; }

exec(cmd) {
if (_locked(cmd)) return;
// Tag each command with the arrangement it was executed against: most
// commands resolve their target through the notes()/chords() accessors
// at rollback time, so an undo issued after switching arrangements
// would silently mutate the WRONG arrangement's notes.
cmd._arrIdx = (cmd.songScope === true) ? -1 : (S.currentArr ?? -1);
cmd.exec();
this.undo.push(cmd);
if (this.undo.length > MAX_UNDO) this.undo.shift();
this.redo = [];
this._afterEdit();
this._ui();
}

doUndo() {
if (!this.undo.length) return;
const c = this.undo[this.undo.length - 1];
// Peek-then-pop: if the command belongs to another arrangement,
// ensureArr switches to it (or refuses when it's gone) BEFORE the
// command leaves the stack, so a refused undo loses nothing.
if (!_hooks.ensureArr(c)) return;
// Rolling a NOTE-scope command back would write the fretted chart shown
// read-only in the roll, bypassing the exec/drag lock. Refuse — peek
// only, so the command stays on the stack. ensureArr above has already
// switched to the command's arrangement, so this evaluates against the
// part the rollback would actually touch.
if (_locked(c)) return;
this.undo.pop(); c.rollback(); this.redo.push(c);
this._afterEdit(); this._ui(); _hooks.draw(); _hooks.updateStatus();
}

doRedo() {
if (!this.redo.length) return;
const c = this.redo[this.redo.length - 1];
if (!_hooks.ensureArr(c)) return;
// Re-exec of a NOTE-scope command writes the read-only chart: same lock.
if (_locked(c)) return;
this.redo.pop(); c.exec(); this.undo.push(c);
// Re-apply the MAX_UNDO cap: a redo pushes back onto the undo stack, so
// without this a redo-heavy session could grow it past the bound that
// exec()/doUndo already enforce. Oldest drops first, mirroring exec().
if (this.undo.length > MAX_UNDO) this.undo.shift();
this._afterEdit(); this._ui(); _hooks.draw(); _hooks.updateStatus();
}

// #18: drop the whole stack when the model is rebuilt under us (the save /
// build flatten+reconstructChords round-trip renumbers arr.notes, so every
// index-based command would now roll back into the wrong note). Reuse the
// live instance + its _ui() wiring rather than reassigning S.history.
// Not _afterEdit() — that nudges the piano viewport, which a clear shouldn't.
reset() { this.undo = []; this.redo = []; this._ui(); }

_afterEdit() {
// Bump the shared edit generation: the section-coverage, chord-display
// and drum-lint memos all key on it. An in-place note-time move keeps
// the notes array's identity and length, so their cheap cache keys
// can't see it — this bump is what forces a recompute.
bumpEditGen();
// Keep the keys viewport in sync with the current note range so
// multi-octave authoring works without manual range control.
// expandOnly=true so adding a note outside the current viewport
// extends it instead of collapsing to the latest note's octave.
if (isKeysMode()) updatePianoRange(true);
}

_ui() {
const u = document.getElementById('editor-undo');
const r = document.getElementById('editor-redo');
if (u) u.disabled = !this.undo.length;
if (r) r.disabled = !this.redo.length;
}
}
127 changes: 7 additions & 120 deletions src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
} from './position.js';
import { setStatus } from './ui.js';
import { hitNote, hitNoteEdge } from './hit-test.js';
import { EditHistory, setHistoryHooks } from './history.js';
import {
_editorCommandById,
_editorEffectiveRightClickBehaviorPure,
Expand Down Expand Up @@ -49,7 +50,7 @@
drawSelectionRect,
editorKeyHighlightEnabled,
} from './draw.js';
import { S, bumpEditGen, editGen } from './state.js';
import { S, editGen } from './state.js';
import {
LC,
_openMidiForArr,
Expand Down Expand Up @@ -1149,125 +1150,6 @@
// Undo / Redo
// ════════════════════════════════════════════════════════════════════

/* @pure:edit-history:start */
// Cap the undo stack so a marathon session can't grow memory without bound —
// the stack held every command since the last save/load. Oldest entries drop
// first; 500 comfortably exceeds any realistic between-saves editing run.
const MAX_UNDO = 500;
class EditHistory {
constructor() { this.undo = []; this.redo = []; }
// Tag each command with the arrangement it was executed against: most
// commands resolve their target through the notes()/chords() accessors at
// rollback time, so an undo issued after switching arrangements would
// silently mutate the WRONG arrangement's notes. Commands that edit
// song-level state (drum tab, tempo grid) opt out via `songScope = true`.
// The `typeof` guards keep this block browser-free for the test harness.
exec(cmd) {
// Read-only roll (V4): a FRETTED part shown in the piano roll is
// edit-locked until the suggest-position writer exists, so every
// NOTE-scope command entry point (keys, menus, inspector, dialogs)
// is inert here in one place. Song-scope commands (drum tab, tempo
// grid) edit song-level data, not the fretted chart, so they still
// pass through even while a fretted part is shown in the roll —
// otherwise switching an unrelated part to the roll would freeze
// tempo/drum editing. typeof-guarded so extracted-test envs without
// the view layer are unaffected. `pitchPreserving` commands (the
// VA.5 position cycle) are the one deliberate carve-out: they can
// never change what a note SOUNDS like — only which string/fret
// plays it — so the "no silent pitch writes" contract the lock
// protects is unbreakable by construction. `suggestResolved` commands
// (the VA.3 suggest-position writer: resolved adds + Accept) are the
// OTHER carve-out — they ARE the sanctioned string/fret write path the
// lock was holding the door for, so they pass here (and mark, never
// guess). Nothing else opts out.
if (cmd.songScope !== true && cmd.pitchPreserving !== true && cmd.suggestResolved !== true
&& typeof _rollReadOnly === 'function' && _rollReadOnly()) {
if (typeof _rollLockNotice === 'function') _rollLockNotice();
return;
}
cmd._arrIdx = (cmd.songScope === true) ? -1
: (typeof S !== 'undefined' ? (S.currentArr ?? -1) : -1);
cmd.exec();
this.undo.push(cmd);
if (this.undo.length > MAX_UNDO) this.undo.shift();
this.redo = [];
this._afterEdit();
this._ui();
}
doUndo() {
if (!this.undo.length) return;
const c = this.undo[this.undo.length - 1];
// Peek-then-pop: if the command belongs to another arrangement,
// _historyEnsureArr switches to it (or refuses when it's gone) BEFORE
// the command leaves the stack, so a refused undo loses nothing.
if (typeof _historyEnsureArr === 'function' && !_historyEnsureArr(c)) return;
// Read-only roll (V4): rolling a NOTE-scope command back would write
// the fretted chart currently shown read-only in the piano roll, so
// undo would silently bypass the exec/drag lock. Refuse (peek only —
// the command stays on the stack, nothing is lost). _historyEnsureArr
// above has already switched to the command's arrangement, so this
// evaluates read-only against the part the rollback would touch.
// Song-scope commands (drum/tempo) don't touch the fretted chart and
// undo normally; pitchPreserving commands (VA.5 position cycle)
// can't change pitch and round-trip freely (see exec); suggestResolved
// commands (VA.3 suggest-position writer) are the sanctioned string/fret
// write path and round-trip too.
if (c.songScope !== true && c.pitchPreserving !== true && c.suggestResolved !== true
&& typeof _rollReadOnly === 'function' && _rollReadOnly()) {
if (typeof _rollLockNotice === 'function') _rollLockNotice();
return;
}
this.undo.pop(); c.rollback(); this.redo.push(c);
this._afterEdit(); this._ui(); draw(); updateStatus();
}
doRedo() {
if (!this.redo.length) return;
const c = this.redo[this.redo.length - 1];
if (typeof _historyEnsureArr === 'function' && !_historyEnsureArr(c)) return;
// Read-only roll (V4): re-exec of a NOTE-scope command writes the
// fretted chart that is read-only in the roll — same lock (and same
// pitchPreserving / suggestResolved carve-outs) as doUndo.
if (c.songScope !== true && c.pitchPreserving !== true && c.suggestResolved !== true
&& typeof _rollReadOnly === 'function' && _rollReadOnly()) {
if (typeof _rollLockNotice === 'function') _rollLockNotice();
return;
}
this.redo.pop(); c.exec(); this.undo.push(c);
// Re-apply the MAX_UNDO cap: a redo pushes back onto the undo stack, so
// without this a redo-heavy session could grow it past the bound that
// exec()/doUndo already enforce. Oldest drops first, mirroring exec().
if (this.undo.length > MAX_UNDO) this.undo.shift();
this._afterEdit(); this._ui(); draw(); updateStatus();
}
// #18: drop the whole stack when the model is rebuilt under us (the save /
// build flatten+reconstructChords round-trip renumbers arr.notes, so every
// index-based command would now roll back into the wrong note). Reuse the
// live instance + its _ui() wiring rather than reassigning S.history.
// Not _afterEdit() — that nudges the piano viewport, which a clear shouldn't.
reset() { this.undo = []; this.redo = []; this._ui(); }
_afterEdit() {
// Invalidate the section-coverage memo: an in-place note-time move
// keeps the notes array's identity + length, so the cheap cache key
// can't see it — this generation bump is what forces a recompute.
// typeof-guarded (like isKeysMode below): declared outside this @pure block.
// Bump the shared edit generation: the section-coverage, chord-display and
// drum-lint memos all key on it. typeof-guarded (like isKeysMode below):
// declared outside this @pure block, so a sliced sandbox may not have it.
if (typeof bumpEditGen === 'function') bumpEditGen();
// Keep the keys viewport in sync with the current note range so
// multi-octave authoring works without manual range control.
// expandOnly=true so adding a note outside the current viewport
// extends it instead of collapsing to the latest note's octave.
if (typeof isKeysMode === 'function' && isKeysMode()) updatePianoRange(true);
}
_ui() {
const u = document.getElementById('editor-undo');
const r = document.getElementById('editor-redo');
if (u) u.disabled = !this.undo.length;
if (r) r.disabled = !this.redo.length;
}
}
/* @pure:edit-history:end */

// Guard: true only while _historyEnsureArr drives an arrangement switch to
// replay an undo/redo. editorSelectArrangement reads it to distinguish a
Expand Down Expand Up @@ -1304,6 +1186,11 @@
return true;
}

// src/history.js cannot import these three back out of main.js without closing
// a cycle. All are hoisted function declarations, so this top-level call is
// safe wherever it sits.
setHistoryHooks({ ensureArr: _historyEnsureArr, draw, updateStatus });

class MoveNoteCmd {
constructor(indices, dtimes, dstrings, dfrets) {
this.indices = indices;
Expand Down Expand Up @@ -1429,7 +1316,7 @@
}
}

class ResizeSustainCmd {

Check warning on line 1319 in src/main.js

View workflow job for this annotation

GitHub Actions / lint 

'ResizeSustainCmd' is defined but never used
constructor(index, newSustain) {
this.index = index;
this.newSustain = newSustain;
Expand Down Expand Up @@ -2854,7 +2741,7 @@
function onMouseUp(e) {
if (!S.drag) return;
if (_loopStripOnMouseUp()) return;
const { x, y } = getMousePos(e);

Check warning on line 2744 in src/main.js

View workflow job for this annotation

GitHub Actions / lint 

'x' is assigned a value but never used

// Bar-range select finalise — refresh the Loop-in-3D button state.
if (S.drag.type === 'barsel') {
Expand Down Expand Up @@ -6514,7 +6401,7 @@

// Empty/initial state for the load list: prompt to search rather than
// rendering every custom song up front.
function renderSongPrompt() {

Check warning on line 6404 in src/main.js

View workflow job for this annotation

GitHub Actions / lint 

'renderSongPrompt' is defined but never used
const list = document.getElementById('editor-load-list');
if (list) {
list.innerHTML = '<div class="text-xs text-gray-500 p-3 text-center">Start typing to search by song, artist, or filename…</div>';
Expand Down Expand Up @@ -8199,7 +8086,7 @@
// the same save path as the Save button (in-place sloppak write, not the
// heavy create-mode build).
if (S.sessionId) {
try { await saveCDLC(); } catch (e) { /* surfaced via setStatus */ }

Check warning on line 8089 in src/main.js

View workflow job for this annotation

GitHub Actions / lint 

'e' is defined but never used. Allowed unused caught errors must match /^_/u
}
// Capture where we are so the return trip lands on the same spot.
const returnCtx = {
Expand Down Expand Up @@ -10233,7 +10120,7 @@
document.getElementById('editor-art-popup')?.remove();
}

function _populateCreateArrButtons() {

Check warning on line 10123 in src/main.js

View workflow job for this annotation

GitHub Actions / lint 

'_populateCreateArrButtons' is defined but never used
const wrap = document.getElementById('editor-create-arr-buttons');
if (!wrap) return;
wrap.replaceChildren();
Expand Down Expand Up @@ -10400,7 +10287,7 @@
createState.lastSync = { ...createState.lastSync, ...data };
}
return data;
} catch (e) {

Check warning on line 10290 in src/main.js

View workflow job for this annotation

GitHub Actions / lint 

'e' is defined but never used. Allowed unused caught errors must match /^_/u
return null;
}
}
Expand Down
Loading
Loading