From 6ecb1118c7fc354726d290efe9acacc54b412f30 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Sun, 12 Jul 2026 15:19:07 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(nav):=20the=20library=20sometimes=20sho?= =?UTF-8?q?wed=20the=20legacy=20screen=20=E2=80=94=20map=20'home'=20inside?= =?UTF-8?q?=20showScreen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Testers: "randomly, when moving to the library from another menu option, the library shows the old interface — never when a song ends." ━━━ WHAT WAS ACTUALLY HAPPENING ━━━ #home is the PRE-V3 library screen. The v3 shell replaced it with #v3-songs, and the mapping DID exist — but only inside WRAPPERS on window.showScreen, and only for callers that go through `window`. THREE independent parties monkey-patch it, each capturing whatever happens to be there at the time: app.js publishes the raw function -> shell.js wraps it, adding the home -> v3-songs mapping -> the stems plugin wraps it AGAIN (src/main.js:1029), capturing the current value Plugins load ASYNCHRONOUSLY. The chain links up in whatever order the race settles, and any capture taken before shell.js installs — or any re-assignment after it — silently drops the mapping. Hence "randomly". AND THE INTERNAL CALLERS NEVER TOUCHED window.showScreen AT ALL. closeCurrentSong and the Esc-from-settings shortcut call the IMPORTED showScreen, which no wrapper ever sees. Reproduced in a browser: the unwrapped function with 'home' lands on the dead legacy screen EVERY time. "Never when a song ends" is the tell, and it is what identified the mechanism: closeCurrentSong resolves its target through _resolvePlayerOrigin(), which ALREADY applies this mapping. That one path was fine — which is exactly why the bug looked random rather than total. PRE-EXISTING, not a regression from the module carve: the onclick="showScreen('home')" links and the wrapper-only mapping both date to 2026-06-22. ━━━ THE FIX ━━━ The guard lives inside showScreen now: ONE place, in the function every caller routes through, instead of a chain of monkey-patches that must each remember. Wrapper order stops mattering, and the module-internal callers are covered for the first time. Verified in a browser: the raw, unwrapped showScreen('home') — which reproduced as #home — now lands on #v3-songs, and cannot be undone by any wrapper order. ━━━ AND A [P1] I INTRODUCED, WHICH CODEX CAUGHT ━━━ My first cut mapped BOTH 'home' and 'v3-home', copied straight from _resolvePlayerOrigin. That is correct THERE and wrong HERE. _resolvePlayerOrigin computes where to RETURN TO after a song, and landing on the Songs list from the dashboard is the right behaviour. But #v3-home is the v3 DASHBOARD — a real screen that the shell's Home nav, the onboarding tour and the dashboard re-render listener all target. Redirecting it would have made Home unreachable. A LEGACY ALIAS IS NOT THE SAME THING AS A RETURN TARGET. Only 'home' is mapped now, and a test pins that: re-adding 'v3-home' to the guard fails it. 4 tests, bite-tested both ways. node 1049, pytest 2425, ESLint 0, Codex 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- static/js/session.js | 39 +++++++++++ tests/js/show_screen_legacy_home.test.js | 85 ++++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 tests/js/show_screen_legacy_home.test.js diff --git a/static/js/session.js b/static/js/session.js index 264930c7..c984fc5b 100644 --- a/static/js/session.js +++ b/static/js/session.js @@ -105,6 +105,45 @@ export let _settingsOriginScreen = 'home'; // ── Screen Navigation ───────────────────────────────────────────────────── export async function showScreen(id) { + // ── 'home' is the LEGACY library screen. Always route it to the v3 Songs list. ── + // + // The v3 shell replaced #home with #v3-songs. That mapping DID exist — but only inside + // wrappers on `window.showScreen`, and only for callers that go through `window`: + // + // app.js publishes the raw fn -> shell.js wraps it (adding the mapping) + // -> the stems plugin wraps it AGAIN, capturing whatever + // happened to be there at the time + // + // Two ways that fails, and testers hit both: + // + // 1. ORDER. Three independent parties monkey-patch window.showScreen, each capturing the + // current value. Plugins load ASYNCHRONOUSLY, so the chain links up in whatever order + // the race settles — and any capture taken before shell.js installs, or any + // re-assignment after it, silently drops the mapping. + // + // 2. THE INTERNAL CALLERS NEVER TOUCHED window.showScreen AT ALL. closeCurrentSong and the + // Esc-from-settings shortcut call the IMPORTED showScreen directly, so no wrapper ever + // sees them. Verified in a browser: the unwrapped function with 'home' lands on the dead + // legacy screen every single time. + // + // Hence "randomly, when moving to the library from another menu option" — and "never when a + // song ends", because closeCurrentSong resolves its target through _resolvePlayerOrigin(), + // which already applies this mapping. + // + // So it lives HERE now: ONE guard in the function every caller routes through, rather than a + // chain of monkey-patches that must each remember. + // + // ONLY 'home'. NOT 'v3-home'. _resolvePlayerOrigin() maps BOTH — correctly, because it + // computes where to RETURN TO after a song, and coming back to the Songs list from the + // dashboard is the right behaviour. Copying that condition here was a [P1] (Codex caught it): + // #v3-home is the v3 DASHBOARD, a real screen the shell's Home nav, the onboarding tour and + // the dashboard re-render listener all target. Redirecting it would make Home unreachable. + // + // A legacy alias is not the same thing as a return target. + if (id === 'home' && document.getElementById('v3-songs')) { + id = 'v3-songs'; + } + // Capture the previous screen before changing active classes const prevScreenId = document.querySelector('.screen.active')?.id; document.querySelectorAll('.screen').forEach(s => s.classList.remove('active')); diff --git a/tests/js/show_screen_legacy_home.test.js b/tests/js/show_screen_legacy_home.test.js new file mode 100644 index 00000000..7f0a1cf1 --- /dev/null +++ b/tests/js/show_screen_legacy_home.test.js @@ -0,0 +1,85 @@ +// showScreen('home') must never land on the LEGACY library screen when v3 is present. +// +// Testers: "randomly, when moving to the library from another menu option, the library shows the +// old interface — never when a song ends." +// +// #home is the pre-v3 library screen. The v3 shell replaced it with #v3-songs, and the mapping +// DID exist — but only inside wrappers on `window.showScreen`, which fail two ways: +// +// 1. ORDER. THREE independent parties monkey-patch window.showScreen, each capturing whatever +// is there at the time: app.js publishes the raw function, shell.js wraps it to add the +// mapping, and the stems plugin wraps it again. Plugins load ASYNCHRONOUSLY, so the chain +// links up in whatever order the race settles. A capture taken before shell.js installs — +// or any re-assignment after it — silently drops the mapping. Hence "randomly". +// +// 2. THE INTERNAL CALLERS BYPASS window.showScreen ENTIRELY. closeCurrentSong and the +// Esc-from-settings shortcut call the IMPORTED showScreen, which no wrapper ever sees. +// Verified in a browser: the unwrapped function with 'home' lands on #home, always. +// +// "Never when a song ends" is the tell: closeCurrentSong resolves its target through +// _resolvePlayerOrigin(), which already applied the mapping — so that one path was fine. +// +// The guard now lives inside showScreen itself: one place every caller routes through. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const SESSION_JS = path.join(__dirname, '..', '..', 'static', 'js', 'session.js'); +const src = () => fs.readFileSync(SESSION_JS, 'utf8'); + +function bodyOf(name) { + const s = src(); + const at = s.indexOf(`export async function ${name}(`); + assert.notEqual(at, -1, `${name} not found`); + let depth = 0; + for (let i = s.indexOf('{', at); i < s.length; i++) { + if (s[i] === '{') depth++; + else if (s[i] === '}' && --depth === 0) return s.slice(at, i + 1); + } + throw new Error('unbalanced'); +} + +test('showScreen maps the legacy #home library to #v3-songs', () => { + const fn = bodyOf('showScreen'); + assert.match( + fn, + /id\s*===\s*'home'[\s\S]{0,80}getElementById\('v3-songs'\)[\s\S]{0,60}id\s*=\s*'v3-songs'/, + "showScreen must route 'home' to 'v3-songs' ITSELF — relying on a wrapper over " + + 'window.showScreen loses the mapping whenever a plugin wraps it first, and misses the ' + + 'module-internal callers (closeCurrentSong, Esc-from-settings) altogether', + ); +}); + +test('the guard runs BEFORE the screen is activated', () => { + const fn = bodyOf('showScreen'); + const guard = fn.search(/id\s*=\s*'v3-songs'/); + const activate = fn.indexOf('classList.add(\'active\')'); + assert.ok(guard !== -1 && activate !== -1); + assert.ok(guard < activate, + 'the mapping must be applied before the screen is activated, or #home is shown first'); +}); + +test('the guard is conditional on v3 actually being present', () => { + const fn = bodyOf('showScreen'); + assert.match(fn, /getElementById\('v3-songs'\)/, + 'the mapping must check #v3-songs exists — without it there is nowhere to route to'); +}); + +test('it does NOT redirect v3-home — the dashboard is a real screen', () => { + // Codex [P1] on the first cut. _resolvePlayerOrigin() maps BOTH 'home' and 'v3-home' — + // correctly, because it computes where to RETURN TO after a song, and landing on the Songs + // list from the dashboard is right. Copying that condition into showScreen is NOT: #v3-home + // is the v3 DASHBOARD, which the shell's Home nav, the onboarding tour and the dashboard + // re-render listener all target. Redirecting it makes Home unreachable. + // + // A legacy alias is not the same thing as a return target. + const fn = bodyOf('showScreen'); + // the condition, i.e. everything between `if (` and the `{` that opens `id = 'v3-songs'` + const m = fn.match(/if \(([\s\S]*?)\)\s*\{\s*id = 'v3-songs';/); + assert.ok(m, 'the legacy-home guard was not found'); + assert.doesNotMatch(m[1], /v3-home/, + "showScreen must NOT redirect 'v3-home' — that is the dashboard, not the legacy library"); + assert.match(m[1], /id === 'home'/, "it must still redirect the legacy 'home'"); +}); From 0cb8b4589d027206172d48cf69797d8880e6f5c8 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Sun, 12 Jul 2026 15:55:13 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(nav):=20nobody=20may=20monkey-patch=20w?= =?UTF-8?q?indow.showScreen=20=E2=80=94=20add=20screen:changing,=20make=20?= =?UTF-8?q?the=20shell=20listen=20(#924)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit window.showScreen was wrapped by THREE independent parties, each capturing whatever happened to be there at the time: app.js publishes the raw function -> static/v3/shell.js wrapped it (to call syncActive, and to map home -> v3-songs) -> the stems plugin wrapped it AGAIN (to tear down on leaving the player) Plugins load ASYNCHRONOUSLY, so the chain linked up in whatever order the race settled. A capture taken before shell.js installed silently dropped the mapping it carried — and the library opened on the dead legacy #home screen. Testers saw that as "randomly, the library shows the old interface" (#923). #923 fixed the symptom by moving the mapping inside showScreen. This removes the CAUSE: neither wrapper ever needed to be one. ━━━ TWO EVENTS, AND THE DISTINCTION IS THE WHOLE POINT ━━━ screen:changing emitted BEFORE anything happens. "I am leaving `from`." Teardown/cancel here. screen:changed emitted after the DOM and data settle. "I am on `id`." Now carries `from`. screen:changing is new, and it exists because Codex caught me collapsing the two. The stems plugin tore down its audio graph BEFORE showScreen did anything; screen:changed fires at the very END, after core awaits library and provider loads — so moving the plugin onto it would have delayed teardown behind a slow fetch, or skipped it entirely if that fetch threw, and stems would keep playing on a non-player screen. A test pins the ordering: screen:changing must precede the first await. shell.js is a plain screen:changed listener now, like app.js, audio-mixer.js and tour-engine.js already were. window.showScreen is an unwrapped function again, and tests/js/ no_showscreen_monkeypatch.test.js fails CI if anything in static/ ever assigns to it again — so the hazard is structurally impossible rather than merely avoided. ━━━ AND A FALLBACK THAT COULD NEVER FIRE ━━━ My retry-if-the-bus-is-late path listened for `slopsmith:capabilities:ready`. Core dispatches `feedBack:capabilities:ready` (capabilities.js:1536) — the slopsmith: name is the PRE-DMCA event and nothing has emitted it since the rename. Codex caught it. A guard that cannot fire is worse than no guard: it reads as protection and is decoration. (The same dead-event bug turned out to be sitting in THREE of the stems plugin's fallbacks, where it has silently disabled its lifecycle wiring whenever the bus was late. Fixed in feedback-plugin-stems#38.) VERIFIED. A/B against origin/main: the nav highlight and topbar title follow IDENTICALLY with shell.js as a listener; screen:changing -> screen:changed fire in order with the right {id, from}; window.showScreen is unwrapped; and showScreen('home') still lands on v3-songs. node 1053, pytest 2425, ESLint 0, Codex 0. Closes #924 Co-Authored-By: Claude Opus 4.8 (1M context) --- static/js/session.js | 24 +++++- static/v3/shell.js | 49 ++++++++--- tests/js/no_showscreen_monkeypatch.test.js | 95 ++++++++++++++++++++++ 3 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 tests/js/no_showscreen_monkeypatch.test.js diff --git a/static/js/session.js b/static/js/session.js index c984fc5b..5d6daa23 100644 --- a/static/js/session.js +++ b/static/js/session.js @@ -146,6 +146,20 @@ export async function showScreen(id) { // Capture the previous screen before changing active classes const prevScreenId = document.querySelector('.screen.active')?.id; + + // ── screen:changing — emitted BEFORE any of the work below ────────────────── + // + // Timing matters here, and Codex caught me getting it wrong. The stems plugin used to + // monkey-patch window.showScreen so it could tear down its audio graph BEFORE navigation + // began. screen:changed fires at the very END of this function — after awaiting library and + // provider loads — so moving that plugin onto it would have delayed teardown behind a slow + // fetch, or skipped it entirely if the fetch threw. Stems would keep playing on a non-player + // screen. + // + // So there are two events, and the distinction is the whole point: + // screen:changing — before anything happens. "I am leaving `from`." Cancel/teardown here. + // screen:changed — after the DOM and data are settled. "I am on `id`." + if (window.feedBack) window.feedBack.emit('screen:changing', { id, from: prevScreenId || null }); document.querySelectorAll('.screen').forEach(s => s.classList.remove('active')); document.getElementById(id).classList.add('active'); // Mark the next render as a screen-entry so it scrolls the @@ -225,7 +239,15 @@ export async function showScreen(id) { setPlayButtonState(false); } window.scrollTo(0, 0); - if (window.feedBack) window.feedBack.emit('screen:changed', { id }); + // `from` is the screen we just LEFT. Without it, "I am leaving the player" is not + // expressible from an event, and the only way to express it was to WRAP window.showScreen — + // which is what shell.js and the stems plugin both did, and why the library intermittently + // showed the legacy screen (#923, #924): three parties patching one global, each capturing + // whatever was there at the time, in whatever order the plugin loads settled. + // + // Additive: every existing listener (app.js, audio-mixer.js, tour-engine.js) reads `id` and + // is unaffected. + if (window.feedBack) window.feedBack.emit('screen:changed', { id, from: prevScreenId || null }); } export let currentFilename = ''; diff --git a/static/v3/shell.js b/static/v3/shell.js index 0a935c12..a2590840 100644 --- a/static/v3/shell.js +++ b/static/v3/shell.js @@ -318,22 +318,49 @@ } } - // ── showScreen wrapper (idempotent rehydration — design/05 §Rehydration) ─ + // ── Stay in sync with the active screen (idempotent rehydration — design/05) ─ + // + // This USED to monkey-patch window.showScreen. It doesn't any more, and that is the point. + // + // Three parties were wrapping that one global — app.js publishes it, this wrapped it, and the + // stems plugin wrapped it again — each capturing whatever happened to be there at the time. + // Plugins load ASYNCHRONOUSLY, so the chain linked up in whatever order the race settled, and + // a capture taken before this installed silently dropped the home -> v3-songs mapping this + // wrapper carried. That is why the library intermittently showed the legacy screen (#923). + // + // The mapping lives inside showScreen() now, where no wrapper can lose it. And everything + // left here is just "the screen changed" — which showScreen already EMITS, and which app.js, + // audio-mixer.js and tour-engine.js have always listened for rather than patching. + // + // So: be a listener, like everyone else. window.showScreen is a plain function again. function installShowScreenHook() { const hooks = window.__feedBackV3ShellHooks || (window.__feedBackV3ShellHooks = {}); - hooks.syncActive = syncActive; // always point at the latest impl + hooks.syncActive = syncActive; // always point at the latest impl if (hooks.installed) return; hooks.installed = true; - hooks.baseShowScreen = window.showScreen; - window.showScreen = function (id) { - // Route every "go to the library" navigation to the v3 native Songs - // screen instead of the legacy #home library, so player-close, - // settings-back, the hidden legacy navbar, etc. all stay in v3. - const target = (id === 'home') ? 'v3-songs' : id; - const r = hooks.baseShowScreen ? hooks.baseShowScreen.call(this, target) : undefined; - try { hooks.syncActive && hooks.syncActive(target); } catch (e) { /* non-fatal */ } - return r; + + // RETRY IF THE BUS IS LATE. The old wrapper didn't need window.feedBack to exist; a + // listener does. Bailing out when it isn't ready yet would silently leave the sidebar + // highlight and topbar title frozen forever — a dead nav, with nothing thrown. (Codex + // caught the identical hole in the stems plugin's version of this.) + const wire = () => { + const bus = window.feedBack; + if (!bus || typeof bus.on !== 'function') { + // `feedBack:capabilities:ready` — capabilities.js:1536. NOT the slopsmith: name: + // that was the pre-DMCA event and NOTHING dispatches it any more, so a fallback + // keyed on it can never fire. Codex caught exactly that here. (The old alias is + // kept too, in case an older capabilities build is in play.) + window.addEventListener('feedBack:capabilities:ready', wire, { once: true }); + window.addEventListener('slopsmith:capabilities:ready', wire, { once: true }); + return; + } + bus.on('screen:changed', (ev) => { + const id = ev && ev.detail && ev.detail.id; + if (!id) return; + try { hooks.syncActive && hooks.syncActive(id); } catch (e) { /* non-fatal */ } + }); }; + wire(); } // ── Boot ──────────────────────────────────────────────────────────────── diff --git a/tests/js/no_showscreen_monkeypatch.test.js b/tests/js/no_showscreen_monkeypatch.test.js new file mode 100644 index 00000000..97e81b9c --- /dev/null +++ b/tests/js/no_showscreen_monkeypatch.test.js @@ -0,0 +1,95 @@ +// Nobody may monkey-patch window.showScreen. (#924) +// +// It used to be wrapped by THREE independent parties, each capturing whatever happened to be +// there at the time: +// +// app.js publishes the raw function +// -> static/v3/shell.js wrapped it (to call syncActive, and to map home -> v3-songs) +// -> the stems plugin wrapped it AGAIN (to tear down on leaving the player) +// +// Plugins load ASYNCHRONOUSLY, so the chain linked up in whatever order the race settled. A +// capture taken before shell.js installed silently dropped the mapping it carried — and the +// library opened on the dead legacy #home screen. Testers saw that as "randomly, the library +// shows the old interface" (#923). +// +// Neither wrapper ever needed to be one. showScreen already EMITS screen:changed, and that is +// already how app.js, audio-mixer.js and tour-engine.js do it. Both are listeners now, and +// window.showScreen is a plain function again — so the ordering hazard is structurally +// impossible rather than merely avoided. +// +// This test is the thing that keeps it that way. A wrapper reintroduced anywhere in static/ +// fails CI. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..', '..'); + +function jsFiles(dir) { + const out = []; + for (const e of fs.readdirSync(dir, { withFileTypes: true })) { + const p = path.join(dir, e.name); + if (e.isDirectory()) out.push(...jsFiles(p)); + else if (e.name.endsWith('.js')) out.push(p); + } + return out; +} + +// strip comments so the prose above (and in shell.js) isn't read as an assignment +const scrub = (s) => s.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/[^\n]*$/gm, ''); + +test('nothing in static/ assigns window.showScreen', () => { + const offenders = []; + for (const f of jsFiles(path.join(ROOT, 'static'))) { + const src = scrub(fs.readFileSync(f, 'utf8')); + // `window.showScreen = ...` — an assignment, not a call or a typeof guard + if (/window\.showScreen\s*=(?!=)/.test(src)) offenders.push(path.relative(ROOT, f)); + } + assert.deepEqual( + offenders, [], + 'these files monkey-patch window.showScreen. Do not: three wrappers racing over one ' + + 'global is what made the library open on the legacy screen (#923). Listen to ' + + 'screen:changed instead — showScreen already emits it, with { id, from }.', + ); +}); + +test('showScreen emits screen:changed with the screen it LEFT', () => { + const src = fs.readFileSync(path.join(ROOT, 'static', 'js', 'session.js'), 'utf8'); + assert.match( + src, + /emit\('screen:changed',\s*\{\s*id,\s*from:/, + "screen:changed must carry `from` — without it, \"I am leaving the player\" is not " + + 'expressible from an event, and the only way to say it is to wrap showScreen, which is ' + + 'the bug this exists to prevent', + ); +}); + +test('screen:changing fires BEFORE the navigation work, screen:changed after', () => { + // The distinction is the whole point, and Codex caught me collapsing it. + // + // The stems plugin's wrapper tore down its audio graph BEFORE showScreen did anything. + // screen:changed fires at the very END — after core awaits library and provider loads — so + // moving the plugin onto it would delay teardown behind a slow fetch, or skip it if that + // fetch threw, and stems would keep playing on a non-player screen. + // + // screen:changing before anything happens. "I am leaving `from`." Cancel/teardown here. + // screen:changed after the DOM and data settle. "I am on `id`." + const src = fs.readFileSync(path.join(ROOT, 'static', 'js', 'session.js'), 'utf8'); + const changing = src.indexOf("emit('screen:changing'"); + const changed = src.indexOf("emit('screen:changed'"); + assert.ok(changing !== -1, 'screen:changing must be emitted'); + assert.ok(changed !== -1, 'screen:changed must be emitted'); + assert.ok(changing < changed, 'screen:changing must come first'); + + // and `changing` must precede the first await, or it is no earlier than `changed` in practice + const firstAwait = src.indexOf('await ', changing); + assert.ok(firstAwait === -1 || changing < firstAwait, + 'screen:changing must fire before showScreen awaits anything — that is its entire purpose'); +}); + +test('the v3 shell reacts to screen:changed rather than wrapping showScreen', () => { + const src = fs.readFileSync(path.join(ROOT, 'static', 'v3', 'shell.js'), 'utf8'); + assert.match(scrub(src), /on\('screen:changed'/, 'shell.js must listen, not patch'); +});