fix(nav): nobody may monkey-patch window.showScreen — add screen:changing, make the shell listen (#924)

static/js/session.js

@@ -105,8 +105,61 @@ 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;
+
+    // ── 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
@@ -186,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 = '';

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 ────────────────────────────────────────────────────────────────

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');
+});

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'");
+});