From 1ab698c8898a2c83513c8b7d18bcb71ed3ba6fc4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 03:47:03 +0000 Subject: [PATCH 1/2] fix: clear stale section-map bar when the current song has no sections _smUpdate() bailed out on "no sections" (empty array / missing duration) before touching any state, so switching to a song or arrangement with no section data (a GP import, or a core re-stream that bypasses the playSong wrapper's own _smRemove()/reset -- e.g. an in-player arrangement switch) left the previous song's blocks painted and the marker/active-highlight frozen wherever the last real song left them. Now clears _smSections/_smDuration/the bar's innerHTML/cached DOM refs the first time sections disappear, and is a no-op on every later tick once already cleared (clearing is idempotent, so no risk of rebuilding an empty bar every 200ms while genuinely no song is loaded). Bumped plugin.json to 1.2.6 (bug fix affecting real behavior). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HVxQShyx8Mrqe1GKNstjHy --- plugin.json | 2 +- screen.js | 22 +++++++++++++++++++++- tests/screen.test.js | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/plugin.json b/plugin.json index 8d0dd09..6ec4e44 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "id": "section_map", "name": "Section Map", - "version": "1.2.5", + "version": "1.2.6", "private": false, "script": "screen.js", "category": "practice", diff --git a/screen.js b/screen.js index 8627dd1..90f2202 100644 --- a/screen.js +++ b/screen.js @@ -249,7 +249,27 @@ function _smUpdate() { const info = highway.getSongInfo(); const t = highway.getTime(); - if (!sections || sections.length === 0 || !info.duration) return; + if (!sections || sections.length === 0 || !info.duration) { + // The previous song's bar must not linger once the CURRENT song has + // no section data. This early-return used to bail before touching + // any state, so switching to a song/arrangement with no sections + // (a GP import, or a core re-stream that bypasses the playSong + // wrapper's own reset -- e.g. an in-player arrangement switch) left + // the old blocks painted and the marker/active-highlight frozen at + // wherever the last real song left them. Only do the work once -- + // clearing is idempotent, so bail immediately on every later tick + // once _smSections is already empty, rather than rebuilding an + // already-empty bar every 200ms while no song/sections are loaded. + if (_smSections.length > 0) { + _smSections = []; + _smDuration = 0; + _smBar.innerHTML = ''; + _smMarkerEl = null; + _smBlockEls = []; + _smActiveIdx = -1; + } + return; + } _smDuration = info.duration; diff --git a/tests/screen.test.js b/tests/screen.test.js index 0ed7640..62cbe86 100644 --- a/tests/screen.test.js +++ b/tests/screen.test.js @@ -358,6 +358,44 @@ test('_smUpdate only touches the blocks whose active state changed, leaving cach } }); +test('_smUpdate clears a stale bar/state once the current song has no section data', () => { + const mod = freshPlugin(); + const originalHighway = global.highway; + const bar = new FakeBar(); + const sections = [{ name: 'Intro', time: 0 }, { name: 'Chorus', time: 30 }]; + try { + global.highway = { + getSections: () => sections, + getSongInfo: () => ({ duration: 120 }), + getTime: () => 5, + }; + mod._setState({ bar, sections: [], duration: 0 }); + mod._smUpdate(); + assert.ok(bar.innerHTML.includes('Intro'), 'sanity: first song rendered'); + + // Switch to a song/arrangement with no section data WITHOUT going + // through _smRemove() (mirrors a core re-stream that bypasses the + // playSong wrapper, e.g. an in-player arrangement switch, or any + // path that skips the wrapper's own state reset). + global.highway.getSections = () => []; + global.highway.getSongInfo = () => ({ duration: 200 }); + mod._smUpdate(); + + const state = mod._getState(); + assert.deepEqual(state.sections, []); + assert.equal(state.duration, 0); + assert.equal(state.activeIdx, -1); + assert.equal(bar.innerHTML, '', 'stale blocks from the previous song must not linger'); + + // A further tick with still-no-sections is a no-op, not a rebuild. + mod._smUpdate(); + assert.equal(bar.innerHTML, ''); + } finally { + if (typeof originalHighway === 'undefined') delete global.highway; + else global.highway = originalHighway; + } +}); + // _smUpdateDifficultyFills updates an existing glass in place (no DOM // rebuild) on a difficulty refresh, only rebuilding the one glass element // when its size bucket changes, per its own doc comment. From 44cbb884575b2ca39005d937376be6a9ad7a2d5f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 22:23:41 +0000 Subject: [PATCH 2/2] fix: song:ready's own reset made the stale-bar clear a no-op on that path Pullfrog review finding on PR #12: the song:ready handler (installed in _smStartRealtimeHooks) sets _smSections = [] itself and THEN calls _smUpdate() directly. The previous fix's guard, `if (_smSections.length > 0)`, was therefore already false by the time _smUpdate() ran on exactly the transition it was meant to catch -- song:ready fires on essentially every song/arrangement switch, so the clear rarely if ever actually ran. Switched the guard to `_smBar.innerHTML !== ''` -- the bar's actual painted content, which only _smRender() sets and only _smRemove()/this same block ever clears -- so it correctly reflects "is there stale content to clear" regardless of which code path zeroed _smSections first. Also clears _smSectionDifficulty in the same block now, for parity with the other two reset sites (playSong wrapper, song:ready). Added a test that mirrors song:ready's actual ordering (sections already [] before _smUpdate() runs, stale blockEls/markerEl/bar content still live) and confirmed it fails against the previous fix, passes against this one. Full suite: 23 passed. Bumped plugin.json to 1.2.7. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HVxQShyx8Mrqe1GKNstjHy --- plugin.json | 2 +- screen.js | 22 +++++++++++++++++----- tests/screen.test.js | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/plugin.json b/plugin.json index 6ec4e44..04e89e9 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "id": "section_map", "name": "Section Map", - "version": "1.2.6", + "version": "1.2.7", "private": false, "script": "screen.js", "category": "practice", diff --git a/screen.js b/screen.js index 90f2202..cfc1062 100644 --- a/screen.js +++ b/screen.js @@ -256,13 +256,25 @@ function _smUpdate() { // (a GP import, or a core re-stream that bypasses the playSong // wrapper's own reset -- e.g. an in-player arrangement switch) left // the old blocks painted and the marker/active-highlight frozen at - // wherever the last real song left them. Only do the work once -- - // clearing is idempotent, so bail immediately on every later tick - // once _smSections is already empty, rather than rebuilding an - // already-empty bar every 200ms while no song/sections are loaded. - if (_smSections.length > 0) { + // wherever the last real song left them. + // + // Gate on the bar's actual painted content, NOT _smSections: the + // song:ready handler above already sets _smSections = [] itself + // before calling _smUpdate() directly, so by the time this runs + // _smSections.length is already 0 on exactly the transition this + // guard exists to catch -- gating on it made the clear a no-op on + // that path, since a bar with stale rendered blocks would report + // "nothing to clear". _smBar.innerHTML is only ever set by + // _smRender() and only ever emptied by _smRemove() or this same + // block, so it accurately tracks whether the bar still shows + // painted content regardless of which path zeroed _smSections + // first. Clearing is idempotent, so this still bails immediately + // on every later tick once already cleared, rather than rebuilding + // an already-empty bar every 200ms while no song/sections are loaded. + if (_smBar.innerHTML !== '') { _smSections = []; _smDuration = 0; + _smSectionDifficulty = {}; _smBar.innerHTML = ''; _smMarkerEl = null; _smBlockEls = []; diff --git a/tests/screen.test.js b/tests/screen.test.js index 62cbe86..463f31f 100644 --- a/tests/screen.test.js +++ b/tests/screen.test.js @@ -396,6 +396,48 @@ test('_smUpdate clears a stale bar/state once the current song has no section da } }); +test('_smUpdate clears the bar even when song:ready already zeroed _smSections first', () => { + // The song:ready handler (installed in _smStartRealtimeHooks) sets + // _smSections = [] itself and THEN calls _smUpdate() directly -- so on + // that path _smSections is already empty by the time _smUpdate's guard + // runs. Gating the clear on `_smSections.length > 0` (the original fix) + // made it a no-op on exactly this path: the bar's stale rendered blocks + // and cached marker/block-el refs never got cleared. This test + // reproduces that ordering directly (bypassing the real event + // subscription) and asserts the bar clears anyway. + const mod = freshPlugin(); + const originalHighway = global.highway; + const bar = new FakeBar(); + bar.innerHTML = '
Intro
'; + const staleBlockEl = { style: {} }; + const staleMarkerEl = { style: {} }; + try { + global.highway = { + getSections: () => [], + getSongInfo: () => ({ duration: 200 }), + getTime: () => 0, + }; + // Mirrors song:ready's own reset (sections already []) landing + // just before _smUpdate() is called, while the bar DOM/cached + // refs from the PREVIOUS song are still live. + mod._setState({ + bar, sections: [], duration: 0, + blockEls: [staleBlockEl], markerEl: staleMarkerEl, activeIdx: 0, + }); + + mod._smUpdate(); + + const state = mod._getState(); + assert.equal(bar.innerHTML, '', 'stale blocks must clear even though _smSections was already []'); + assert.equal(state.markerEl, null); + assert.deepEqual(state.blockEls, []); + assert.equal(state.activeIdx, -1); + } finally { + if (typeof originalHighway === 'undefined') delete global.highway; + else global.highway = originalHighway; + } +}); + // _smUpdateDifficultyFills updates an existing glass in place (no DOM // rebuild) on a difficulty refresh, only rebuilding the one glass element // when its size bucket changes, per its own doc comment.