refactor(editor): move the Strings (tuning) editor to src/strings.js (R2, step 37) - #189
Conversation
…(R2, step 38) Extract the strings-tuning editor (add/remove strings on the active fretted arrangement, edit per-string tuning offsets, all undoable) out of the src/main.js monolith into a new native ES module. Function-level cut: the parts-view and the draw() reassignment that share the "Strings" banner stay in main.js. - src/strings.js: 5 window.editor* handlers (re-attached by main.js) + the @pure:string-tuning block (with SetStringTuningCmd) kept export-free for the test slice. draw/updateStatus route through host; no new hooks, 0 back-exports. - Removed 2 dead main.js imports (AddStringCmd, RemoveStringCmd). - tests/strings_modal.test.mjs reworked: handlers are export const now (sliced with the export prefix stripped) and reach draw/updateStatus through an injected host stub. main.js drops ~267 lines. 38 modules. Verified: 90/90 JS suites pass, ESLint gate clean (0 errors), strict no-undef clean on strings.js, Codex preflight clean. New headless harness drives the modal: 5 handlers bound, editorShowStringsModal renders the string rows, editorAddString adds one (row count +1, proving the undoable command + re-render), editorRemoveString removes it, editorHideStringsModal closes — zero page errors. Negative-checked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Strings tuning editor moves from ChangesStrings editor extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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: 2
🤖 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/strings.js`:
- Around line 99-106: Update _notesOnString() to also inspect
arr.chord_templates and count notes within each template whose string matches
idx, alongside standalone notes and chord notes; ensure RemoveStringCmd’s checks
use this count so removal is blocked when a template-only fingering would be
discarded.
- Around line 71-89: SetStringTuningCmd must restore the original tuning array
shape during rollback, not just the prior value. Capture the original tuning
array existence and length in the constructor, then update rollback to restore
that length or remove the tuning property when it was originally absent; keep
exec behavior unchanged. Use _set and rollback as the implementation points.
🪄 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: 826ca259-007e-4805-a99f-671c4e14dadb
📒 Files selected for processing (3)
src/main.jssrc/strings.jstests/strings_modal.test.mjs
| class SetStringTuningCmd { | ||
| constructor(arrIdx, stringIdx, newOffset) { | ||
| this.arrIdx = arrIdx; | ||
| this.stringIdx = stringIdx; | ||
| this.newOffset = _stringTuningClampPure(newOffset); | ||
| const arr = S.arrangements[arrIdx]; | ||
| const t = (arr && arr.tuning) || []; | ||
| this.oldOffset = Number.isFinite(Number(t[stringIdx])) ? Number(t[stringIdx]) : 0; | ||
| } | ||
| _arr() { return S.arrangements[this.arrIdx]; } | ||
| _set(v) { | ||
| const arr = this._arr(); | ||
| if (!arr) return; | ||
| if (!Array.isArray(arr.tuning)) arr.tuning = []; | ||
| while (arr.tuning.length <= this.stringIdx) arr.tuning.push(0); | ||
| arr.tuning[this.stringIdx] = v; | ||
| } | ||
| exec() { this._set(this.newOffset); } | ||
| rollback() { this._set(this.oldOffset); } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Restore the original tuning shape on undo.
When editing a missing tuning slot, _set() extends the array, but rollback only writes oldOffset; undo therefore leaves extra zero entries and changes serialized state.
Proposed fix
constructor(arrIdx, stringIdx, newOffset) {
this.arrIdx = arrIdx;
this.stringIdx = stringIdx;
this.newOffset = _stringTuningClampPure(newOffset);
const arr = S.arrangements[arrIdx];
- const t = (arr && arr.tuning) || [];
+ const t = Array.isArray(arr?.tuning) ? arr.tuning : [];
+ this.oldLength = t.length;
+ this.hadOldOffset = stringIdx < t.length;
+ this.oldRawOffset = t[stringIdx];
this.oldOffset = Number.isFinite(Number(t[stringIdx])) ? Number(t[stringIdx]) : 0;
}
@@
- rollback() { this._set(this.oldOffset); }
+ rollback() {
+ const arr = this._arr();
+ if (!arr) return;
+ if (!Array.isArray(arr.tuning)) arr.tuning = [];
+ if (this.hadOldOffset) arr.tuning[this.stringIdx] = this.oldRawOffset;
+ arr.tuning.length = this.oldLength;
+ }📝 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.
| class SetStringTuningCmd { | |
| constructor(arrIdx, stringIdx, newOffset) { | |
| this.arrIdx = arrIdx; | |
| this.stringIdx = stringIdx; | |
| this.newOffset = _stringTuningClampPure(newOffset); | |
| const arr = S.arrangements[arrIdx]; | |
| const t = (arr && arr.tuning) || []; | |
| this.oldOffset = Number.isFinite(Number(t[stringIdx])) ? Number(t[stringIdx]) : 0; | |
| } | |
| _arr() { return S.arrangements[this.arrIdx]; } | |
| _set(v) { | |
| const arr = this._arr(); | |
| if (!arr) return; | |
| if (!Array.isArray(arr.tuning)) arr.tuning = []; | |
| while (arr.tuning.length <= this.stringIdx) arr.tuning.push(0); | |
| arr.tuning[this.stringIdx] = v; | |
| } | |
| exec() { this._set(this.newOffset); } | |
| rollback() { this._set(this.oldOffset); } | |
| class SetStringTuningCmd { | |
| constructor(arrIdx, stringIdx, newOffset) { | |
| this.arrIdx = arrIdx; | |
| this.stringIdx = stringIdx; | |
| this.newOffset = _stringTuningClampPure(newOffset); | |
| const arr = S.arrangements[arrIdx]; | |
| const t = Array.isArray(arr?.tuning) ? arr.tuning : []; | |
| this.oldLength = t.length; | |
| this.hadOldOffset = stringIdx < t.length; | |
| this.oldRawOffset = t[stringIdx]; | |
| this.oldOffset = Number.isFinite(Number(t[stringIdx])) ? Number(t[stringIdx]) : 0; | |
| } | |
| _arr() { return S.arrangements[this.arrIdx]; } | |
| _set(v) { | |
| const arr = this._arr(); | |
| if (!arr) return; | |
| if (!Array.isArray(arr.tuning)) arr.tuning = []; | |
| while (arr.tuning.length <= this.stringIdx) arr.tuning.push(0); | |
| arr.tuning[this.stringIdx] = v; | |
| } | |
| exec() { this._set(this.newOffset); } | |
| rollback() { | |
| const arr = this._arr(); | |
| if (!arr) return; | |
| if (!Array.isArray(arr.tuning)) arr.tuning = []; | |
| if (this.hadOldOffset) arr.tuning[this.stringIdx] = this.oldRawOffset; | |
| arr.tuning.length = this.oldLength; | |
| } |
🤖 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/strings.js` around lines 71 - 89, SetStringTuningCmd must restore the
original tuning array shape during rollback, not just the prior value. Capture
the original tuning array existence and length in the constructor, then update
rollback to restore that length or remove the tuning property when it was
originally absent; keep exec behavior unchanged. Use _set and rollback as the
implementation points.
There was a problem hiding this comment.
Valid, but pre-existing — this is a verbatim move (16/16 tests confirm identical behavior), and it only triggers when editing a tuning slot past the array end, which does not happen for a normal full-length tuning. Deferring to a strings-editor hardening follow-up rather than changing command semantics on a move PR.
There was a problem hiding this comment.
@byrongamatos, that's a reasonable call — deferring behavioral/edge-case hardening out of a pure move PR keeps the diff clean and the "16/16 identical behavior" verification is convincing. Agreed this shouldn't block the move.
Would you like me to open a follow-up issue to track this for a future strings-editor hardening pass? If so, just confirm and I'll file it referencing this PR and comment.
✏️ Learnings added
Learnt from: byrongamatos
Repo: got-feedBack/feedBack-plugin-editor PR: 189
File: src/strings.js:71-89
Timestamp: 2026-07-10T09:10:22.127Z
Learning: In `byrongamatos/got-feedBack/feedBack-plugin-editor`, PRs that move code verbatim between files (module extraction refactors) are treated as move-only PRs — pre-existing bugs surfaced in the moved code (e.g., `SetStringTuningCmd.rollback()` in `src/strings.js` not restoring the original tuning array length/shape when editing a tuning slot past the array end) should be deferred to a separate hardening follow-up rather than fixed inline, provided behavior-preservation tests confirm the move is verbatim.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| function _notesOnString(arr, idx) { | ||
| let count = 0; | ||
| for (const n of arr.notes || []) if (n.string === idx) count += 1; | ||
| for (const ch of arr.chords || []) { | ||
| for (const cn of ch.notes || []) if (cn.string === idx) count += 1; | ||
| } | ||
| return count; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Block removal when a chord template uses the target string.
_notesOnString() ignores chord_templates, although RemoveStringCmd deletes their fret/finger column. A template-only fingering can therefore be silently discarded.
Proposed fix
-function _notesOnString(arr, idx) {
+function _contentOnString(arr, idx) {
let count = 0;
for (const n of arr.notes || []) if (n.string === idx) count += 1;
for (const ch of arr.chords || []) {
for (const cn of ch.notes || []) if (cn.string === idx) count += 1;
}
+ for (const ct of arr.chord_templates || []) {
+ if (Array.isArray(ct.frets)
+ && idx < ct.frets.length
+ && ct.frets[idx] !== -1) count += 1;
+ }
return count;
}
@@
- const blockers = removableIdx >= 0 ? _notesOnString(arr, removableIdx) : 0;
+ const blockers = removableIdx >= 0 ? _contentOnString(arr, removableIdx) : 0;
@@
- warn.textContent = `${blockers} note${blockers === 1 ? '' : 's'} on the ${removePos} string — delete or move them before removing.`;
+ warn.textContent = `${blockers} chart item${blockers === 1 ? '' : 's'} on the ${removePos} string — delete or move them before removing.`;
@@
- if (_notesOnString(arr, targetIdx) > 0) return;
+ if (_contentOnString(arr, targetIdx) > 0) return;Also applies to: 185-200, 251-252
🤖 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/strings.js` around lines 99 - 106, Update _notesOnString() to also
inspect arr.chord_templates and count notes within each template whose string
matches idx, alongside standalone notes and chord notes; ensure
RemoveStringCmd’s checks use this count so removal is blocked when a
template-only fingering would be discarded.
There was a problem hiding this comment.
Valid pre-existing issue (unchanged by this move). Fixing it right means deciding removal semantics when a chord template references the string + adding test coverage — that deserves its own PR, not a rider on a verbatim move. Flagging as a follow-up.
There was a problem hiding this comment.
@byrongamatos, that's a reasonable call — scoping this as a verbatim move and deferring the removal-semantics decision (plus test coverage) to a dedicated PR makes sense rather than bundling it here.
Would you like me to open a follow-up GitHub issue to track this (referencing this PR and comment)? Just let me know and I'll file it.
1 participant
What
Step 37 of the R2 module-extraction refactor: move the Strings (tuning) editor out of
src/main.jsinto a new native ES modulesrc/strings.js.Add/remove strings on the active fretted arrangement, and edit per-string tuning offsets — all undoable.
How the seam is drawn
Function-level cut — the parts-view and the
draw()reassignment that share the "Strings" banner stay in main.js:src/strings.js: 5window.editor*handlers (re-attached by main.js) + the@pure:string-tuningblock (withSetStringTuningCmd) kept export-free for the test slice.draw/updateStatusroute throughhost; no new hooks, 0 back-exports.AddStringCmd,RemoveStringCmd).tests/strings_modal.test.mjsreworked: the handlers areexport constnow (sliced with theexportprefix stripped) and reachdraw/updateStatusthrough an injectedhoststub.main.js drops ~267 lines. 38 modules.
Verification
no-undefclean onstrings.js.verify_strings.py) drives the modal: 5 handlers bound,editorShowStringsModalrenders the string rows,editorAddString('low')adds one (row count +1, proving the undoable command + re-render),editorRemoveString('low')removes it,editorHideStringsModalcloses — zero page errors. Negative-checked by dropping a re-attach.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes