refactor(editor): move the inspector panel to src/inspector.js (R2, step 24) - #176
Merged
Conversation
…tep 24) src/main.js 9,779 -> 9,144. Twenty-sixth module; the graph stays acyclic. 639 lines: note attributes on one face (fret, string, time, sustain, techniques, bend intent, teaching marks), chord name / voicing / fingering / function on the other. Every edit goes through a command in src/commands.js, so undo works and the read-only-roll lock applies. Its 19 window.editor* handlers are the ones the panel's own innerHTML calls by name; they are exported plain functions that main.js re-attaches. Two main.js symbols travel back as host hooks: the bend-curve dialog (promptBend) and the canvas-resize scheduler. CODEX CAUGHT A REAL BUG, and it is worth recording why. My edit that added the two hooks to setHostHooks() silently did nothing: I used `str.replace(old, new, 1)` with an anchor that no longer matched, and Python does not complain. So inspector.js called host.js's inert defaults. "Edit curve…" opened nothing. No error, no log, and all 89 unit tests green — they import the module directly and never see the wiring. verify_inspector.py now asserts that editorOpenBendCurve() opens #editor-bend-modal, and fails on exactly that bug. Every subsequent scripted edit in this refactor asserts the replacement landed. The harness deliberately does NOT assert host.scheduleCanvasResize. At this viewport the panel overlays the canvas rather than shrinking it, so the callback has no observable effect, and a check for it would pass for the wrong reason. Saying so in the file is better than a green tick that means nothing. Tests: inspector_time and view_switcher slice src/inspector.js now instead of main.js, stripping the `export` keyword (a SyntaxError inside `new Function`). inspector_time injects a `host` stub so the reject branch's re-render stays observable; view_switcher gained an extractInspectorFn for the exported-function shape. node --test 89/89, pytest 248/248, npm run lint 0 errors (7 warnings), Codex clean on re-review, all 18 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe inspector UI and note/chord editing logic were moved from ChangesInspector module extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Refactors the editor’s inspector panel out of src/main.js into a dedicated src/inspector.js module, keeping the dependency graph acyclic by routing a small set of host callbacks through src/host.js, and updating tests to extract inspector logic from the new module.
Changes:
- Introduces
src/inspector.jscontaining the inspector renderer and its exportedwindow.editor*-style handler functions (reattached bymain.js). - Updates
src/main.jsto import/reattach inspector handlers and to wire new host hooks (promptBend,scheduleCanvasResize) viasetHostHooks(...). - Updates inspector-related tests to slice functions from
src/inspector.jsinstead ofsrc/main.js, and documents the move inCHANGELOG.md.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/view_switcher.test.mjs | Switches function extraction to handle exported inspector handlers from src/inspector.js. |
| tests/inspector_time.test.mjs | Updates harness extraction to slice inspector bounds/coercion/dispatcher from src/inspector.js with export stripped. |
| src/main.js | Imports inspector exports, reattaches them onto window.*, and wires new host hooks. |
| src/inspector.js | New module implementing inspector rendering + handlers, using host for callbacks into main.js. |
| src/host.js | Adds default host hooks for bend dialog and canvas resize scheduling used by the inspector module. |
| CHANGELOG.md | Documents the inspector refactor/move. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot, on #176. Three findings, one fact: I over-claimed. Audited every mutator in the module: undoable (S.history.exec) editorInspectorSetField, editorInspectorSetBendIntent, the chord patches, and everything routed through _applyTeachingMark -> SetTeachingMarkCmd: fret finger, scale degree, editorGroupAsStrum, editorUngroupStrum in place, NOT undoable editorInspectorSetTech, editorInspectorSetFlag Both exceptions DO honour the read-only-roll lock, so the other half of the claim stands; they cannot silently write a chart the roll shows read-only. Also corrected a comment that predates this refactor: 'Edits skip the undo history for now' was written for PR3b when that was true of the whole panel. It stopped being true once setField, setBendIntent and the chord patches landed, and it now says which edits it means. Header, in-file comment and CHANGELOG all corrected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
All three correct — I over-claimed, in the header, in an inherited comment, and in the CHANGELOG. I audited every mutator in the module rather than guessing: Undoable ( In place, not undoable: Worth adding: both exceptions do call Your second finding also caught something older than this PR. |
![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/inspector.js`:
- Around line 4-7: Update the technique and flag edit handlers in inspector.js,
including the ranges around lines 264-268, 353-388, and 407-419, so they no
longer mutate notes directly. Add and use a bulk command in src/commands.js that
snapshots and restores each affected note’s complete technique state, including
bend curves, and routes bend, slide, and flag changes through it so undo/redo
and read-only locking work consistently.
- Around line 111-143: Escape every note-derived value before interpolating it
into HTML assigned via innerHTML, including sharedString, sharedFret, numeric
technique values, and fr. Update the relevant rendering logic and all additional
affected ranges to use a shared HTML-escaping helper that safely encodes text
and attribute characters, while preserving intended empty or mixed-value
behavior.
🪄 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: c47452ee-74d4-4d75-9d0c-979d8fe2a84b
📒 Files selected for processing (6)
CHANGELOG.mdsrc/host.jssrc/inspector.jssrc/main.jstests/inspector_time.test.mjstests/view_switcher.test.mjs
…depth) CodeRabbit, on #176. Two findings: 1. (Major, undo) The technique toggles and boolean flags mutate n.techniques in place and are not undoable. That is real, but it is the deferred TechBulkCmd from PR3b, not a regression — a feature, tracked. The docs now state exactly which edits are undoable (previous commit). Not implementing it here. 2. (Major, XSS) sharedString / sharedFret / numeric technique values reach innerHTML unescaped. Investigated #2 before acting. The server's _note() serializer coerces string/fret with int(), so a crafted feedpak cannot reach the client raw through the load path — I built a malicious .sloppak with , loaded it through the running server, and the inspector showed . Not exploitable today. But the panel should not DEPEND on a coercion happening in another process. A note that ever reached it un-coerced would inject markup, and the coupling is implicit. So every note-derived value the inspector interpolates now runs through _chordAttrEsc (the module's existing escaper): fmtStr for the string/fret readouts, inputVal for the numeric input attributes. tests/inspector_xss.test.mjs drives the REAL _renderInspector with a hostile fret and asserts no raw <img> reaches innerHTML — jsdom-free, via a recording document stub. It fails on the unescaped code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Both addressed. Undo (finding 1): real, but it is the deferred XSS (finding 2): I investigated before patching. The server’s But the panel should not depend on a coercion that happens in another process, and that coupling was implicit. So every note-derived value the inspector interpolates now runs through
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/inspector_xss.test.mjs (1)
54-58: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSecond assertion's OR fallback weakens the escaping check.
lastHtml.includes('<')alone would pass for any escaped<in the output, not necessarily the payload's<img. Consider asserting on a payload-specific escaped substring (e.g.<img) only, to keep the regression test tightly coupled to the actual hostile input.Tighten the assertion
t('the payload is present, but as escaped entities', () => { // It should still be visible to the user — escaped, not stripped. - assert.ok(lastHtml.includes('<img') || lastHtml.includes('<'), - 'the payload was neither escaped nor present; expected <img…'); + assert.ok(lastHtml.includes('<img'), + 'the payload was neither escaped nor present; expected <img…'); });🤖 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/inspector_xss.test.mjs` around lines 54 - 58, In the test “the payload is present, but as escaped entities,” remove the broad `lastHtml.includes('<')` fallback and assert only that `lastHtml` contains the payload-specific escaped substring `<img`, ensuring the hostile `<img` input is actually escaped.
🤖 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.
Nitpick comments:
In `@tests/inspector_xss.test.mjs`:
- Around line 54-58: In the test “the payload is present, but as escaped
entities,” remove the broad `lastHtml.includes('<')` fallback and assert only
that `lastHtml` contains the payload-specific escaped substring `<img`,
ensuring the hostile `<img` input is actually escaped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f2cb339-5b0d-420e-81cb-e71bb94dc02e
📒 Files selected for processing (2)
src/inspector.jstests/inspector_xss.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/inspector.js
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
src/main.js 8,760 -> 7,715. Twenty-eighth module; the graph stays acyclic. main.js is now 64% below the 21,176 lines this refactor started from. 1,039 lines: the playback engine (startPlayback / stopPlayback / playbackTick), the waveform, the onset strip, follow-scroll and the WebAudio graph, plus the guide claps, the metronome, the A/B reference loop, the per-bus mixer and the edit blip. Stacks on #178 (the two loop-region pures it needs from transport.js). It owns the rAF loop: `rafId` is module-scope, set by playbackTick and cancelled by teardownAudio(), which main.js's screen teardown calls. The import-time button seeding folded into initAudio(), called from init(). The 8 window.editor* toolbar handlers are exported and re-attached. Five main.js symbols arrive as host hooks: draw, drawNow, the scroll-bounds math (editorClampScrollX / editorApplyScrollBounds), and the A/B loop-region selection (selectedLoopRegion / setLoopRegionEnabled). TWO REAL BUGS, both found on review, neither caught by the 90 unit tests: 1. main.js was REASSIGNING the now-imported _abPhase and _abOn bindings (illegal — an import binding is read-only). The A/B state's only writers outside the engine were two disarm sites; they now call the new export _abDisarm(), so all A/B state writes stay inside audio.js. Same live-binding rule as every prior step: the writer couldn't move, so it crosses as a function. 2. setHostHooks() was missing all 5 new hooks — my scripted edit's anchor didn't match and Python didn't complain (the exact trap from #176). So audio.js ran against host.js's inert defaults: playback advanced the cursor without repainting, follow-scroll was identity, A/B saw no region. Codex caught it. verify_audio.py now plays in compose mode (grid-driven, no audio buffer) and counts canvas repaints — with the drawNow hook unwired the cursor advances and the canvas never clears. It fails on exactly bug #2 while all 90 unit tests pass. And a third, latent: teardownAudio() completes what the old inline teardown skipped — it stops the guide/metronome setInterval (module-scope, so it outlived a re-injected screen). Codex flagged the gap; the fix is S.playing=false + _guideTimerSync() + _guideCancelVoices(). Tests: ~11 audio suites retarget their source read from main.js to audio.js and strip the `export` keyword before eval; loop_ab and boot_teardown gained host stubs for the new hooks. Several CJS -> .mjs. node --test 90/90, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean on three rounds, all 20 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
…27) (#179) * refactor(editor): move the audio subsystem to src/audio.js (R2, step 27) src/main.js 8,760 -> 7,715. Twenty-eighth module; the graph stays acyclic. main.js is now 64% below the 21,176 lines this refactor started from. 1,039 lines: the playback engine (startPlayback / stopPlayback / playbackTick), the waveform, the onset strip, follow-scroll and the WebAudio graph, plus the guide claps, the metronome, the A/B reference loop, the per-bus mixer and the edit blip. Stacks on #178 (the two loop-region pures it needs from transport.js). It owns the rAF loop: `rafId` is module-scope, set by playbackTick and cancelled by teardownAudio(), which main.js's screen teardown calls. The import-time button seeding folded into initAudio(), called from init(). The 8 window.editor* toolbar handlers are exported and re-attached. Five main.js symbols arrive as host hooks: draw, drawNow, the scroll-bounds math (editorClampScrollX / editorApplyScrollBounds), and the A/B loop-region selection (selectedLoopRegion / setLoopRegionEnabled). TWO REAL BUGS, both found on review, neither caught by the 90 unit tests: 1. main.js was REASSIGNING the now-imported _abPhase and _abOn bindings (illegal — an import binding is read-only). The A/B state's only writers outside the engine were two disarm sites; they now call the new export _abDisarm(), so all A/B state writes stay inside audio.js. Same live-binding rule as every prior step: the writer couldn't move, so it crosses as a function. 2. setHostHooks() was missing all 5 new hooks — my scripted edit's anchor didn't match and Python didn't complain (the exact trap from #176). So audio.js ran against host.js's inert defaults: playback advanced the cursor without repainting, follow-scroll was identity, A/B saw no region. Codex caught it. verify_audio.py now plays in compose mode (grid-driven, no audio buffer) and counts canvas repaints — with the drawNow hook unwired the cursor advances and the canvas never clears. It fails on exactly bug #2 while all 90 unit tests pass. And a third, latent: teardownAudio() completes what the old inline teardown skipped — it stops the guide/metronome setInterval (module-scope, so it outlived a re-injected screen). Codex flagged the gap; the fix is S.playing=false + _guideTimerSync() + _guideCancelVoices(). Tests: ~11 audio suites retarget their source read from main.js to audio.js and strip the `export` keyword before eval; loop_ab and boot_teardown gained host stubs for the new hooks. Several CJS -> .mjs. node --test 90/90, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean on three rounds, all 20 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(editor): address audio review — self-contained _abDisarm, stale test paths Copilot + CodeRabbit, on #179. - CodeRabbit (Major, audio.js): _abDisarm() now calls _guideTimerSync() itself. Disarming A/B flips _guideTimerSync's "want" (which includes _abActive()), so leaving each caller to remember it invited exactly the kind of stranded-timer leak the teardown fix already chased. The one caller that did call it explicitly (the song-change reset) drops the now-redundant call. - Copilot (loop_ab.js): the A/B runtime slice regex matched a bare `let _abOn` but the code is now an exported binding. It worked by substring luck; made the `export` prefix optional-explicit so it can't silently mis-slice. - CodeRabbit (x3): metronome_click / onset_snap / onset_strip printed "not found in src/main.js" in their block-missing guard, but they read src/audio.js now. Corrected. NOT changed, with reasons: - loadAudio() swallowing decode errors (CodeRabbit Major, Copilot): real, but PRE-EXISTING and verbatim-moved. A decode failure leaving the prior song's buffer is a latent bug, and fixing it means changing loadCDLC's flow and deciding what the UI shows on failure — a behaviour change that belongs in its own PR, not a mechanical extraction. node --test 90/90, npm run lint 0 errors, verify_audio.py passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
…p.js (R2, step 28) src/main.js 7,714 -> 7,183. Twenty-ninth module; the graph stays acyclic. 66% below the 21,176 lines this refactor started from. 528 lines: the A/B loop strip and its drag / nudge / keyboard handling, bar-range selection, the scroll-bounds math, and snapTime — the one place a raw time becomes a snapped one (grid, or the nearest audio onset). Every module that places something on the timeline snaps through host.snapTime, which resolves here. Three main.js symbols arrive as host hooks: the seek, the snap-step query, and the Loop-in-3D button refresh. The loop-region + scroll functions and snapTime were already host hooks pointing at main.js; they resolve to the loop.js exports now. loop.js imports the A/B functions from audio.js; audio.js reaches loop's _selectedLoopRegion / _setLoopRegionEnabled through host, not import, so no cycle. Caught while wiring: updateLoopIn3DBtn was ALREADY a host hook from a prior step. My scripted add made a second one — removed the duplicate in both host.js and the setHostHooks call. (The 3-new-hook wiring itself landed correctly this time; verified by Codex before trusting it, given the same edit silently no-op'd in #176 and #179.) Also drops a dead _guideTimerSync import left in main.js when #179's _abDisarm consolidation removed its last caller. Tests: 9 loop/onset/group suites retarget their source read to src/loop.js and strip the `export` keyword before eval; loop_ab / loop_nudge_live gained host stubs for the new hooks; loop_undo_mode reads both loop.js and main.js (its @pure:pending-view block stayed behind). Several CJS -> .mjs. verify_loop.py drives editorSetLoopSnapMode end to end (persists the pref + reports through setStatus) and asserts the window.* re-attach; snapTime is covered by verify_drum. Comment out the re-attach and it throws where 90 unit tests pass. node --test 90/90, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean, all 21 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
…p.js (R2, step 28) (#180) * refactor(editor): move the loop region and scroll viewport to src/loop.js (R2, step 28) src/main.js 7,714 -> 7,183. Twenty-ninth module; the graph stays acyclic. 66% below the 21,176 lines this refactor started from. 528 lines: the A/B loop strip and its drag / nudge / keyboard handling, bar-range selection, the scroll-bounds math, and snapTime — the one place a raw time becomes a snapped one (grid, or the nearest audio onset). Every module that places something on the timeline snaps through host.snapTime, which resolves here. Three main.js symbols arrive as host hooks: the seek, the snap-step query, and the Loop-in-3D button refresh. The loop-region + scroll functions and snapTime were already host hooks pointing at main.js; they resolve to the loop.js exports now. loop.js imports the A/B functions from audio.js; audio.js reaches loop's _selectedLoopRegion / _setLoopRegionEnabled through host, not import, so no cycle. Caught while wiring: updateLoopIn3DBtn was ALREADY a host hook from a prior step. My scripted add made a second one — removed the duplicate in both host.js and the setHostHooks call. (The 3-new-hook wiring itself landed correctly this time; verified by Codex before trusting it, given the same edit silently no-op'd in #176 and #179.) Also drops a dead _guideTimerSync import left in main.js when #179's _abDisarm consolidation removed its last caller. Tests: 9 loop/onset/group suites retarget their source read to src/loop.js and strip the `export` keyword before eval; loop_ab / loop_nudge_live gained host stubs for the new hooks; loop_undo_mode reads both loop.js and main.js (its @pure:pending-view block stayed behind). Several CJS -> .mjs. verify_loop.py drives editorSetLoopSnapMode end to end (persists the pref + reports through setStatus) and asserts the window.* re-attach; snapTime is covered by verify_drum. Comment out the re-attach and it throws where 90 unit tests pass. node --test 90/90, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean, all 21 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(test): correct stale src/main.js paths in the loop suites Copilot + CodeRabbit, on #180. Six stale references, all the same class: after retargeting the loop suites' source reads from main.js to loop.js (and audio.js for the @pure:loop-ab / onset-snap blocks), the comments and block-missing failure messages still named main.js. Corrected each to the actual source so a CI failure points at the right file. Verified each against where the sliced symbol now lives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- 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
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.
src/main.js9,779 → 9,144. Twenty-sixth module; the graph stays acyclic.639 lines: note attributes on one face (fret, string, time, sustain, techniques, bend intent, teaching marks), chord name / voicing / fingering / function on the other. Every edit goes through a command in
src/commands.js, so undo works and the read-only-roll lock applies.Its 19
window.editor*handlers are the ones the panel’s owninnerHTMLcalls by name; they are exported plain functions thatmain.jsre-attaches. Twomain.jssymbols travel back as host hooks: the bend-curve dialog (promptBend) and the canvas-resize scheduler.Codex caught a real bug, and it is worth recording why
My edit that added the two hooks to
setHostHooks()silently did nothing. I usedstr.replace(old, new, 1)with an anchor that no longer matched — and Python does not complain.So
inspector.jscalledhost.js’s inert defaults. “Edit curve…” opened nothing. No error, no log, and all 89 unit tests green: they import the module directly and never see the wiring.verify_inspector.pynow assertseditorOpenBendCurve()opens#editor-bend-modal:promptBendhook unwiredsetField(time)moves the note and repaintseditorOpenBendCurveopens the bend modalEvery subsequent scripted edit in this refactor asserts the replacement landed.
What the harness deliberately does not assert
host.scheduleCanvasResize. At this viewport the panel overlays the canvas rather than shrinking it, so the callback has no observable effect — a check for it would pass for the wrong reason. Saying so in the file is better than a green tick that means nothing.Tests
inspector_timeandview_switcherslicesrc/inspector.jsnow instead ofmain.js, stripping theexportkeyword (aSyntaxErrorinsidenew Function).inspector_timeinjects ahoststub so the reject branch’s re-render stays observable;view_switchergained anextractInspectorFnfor the exported-function shape.Verification
node --test89/89 ·pytest248/248 ·npm run lint0 errors (7 warnings) · Codex clean on re-review (it also confirmedpromptBendis not a reassigned binding, so passing it by name is safe — unlikedraw) · all 18 headless harnesses pass.🤖 Generated with Claude Code
Summary by CodeRabbit