Discover controller — Cin pre-review polish

Three changes tightening the controller before opening the PR.

DROP MAGIC `extractItems` DEFAULTS

Controller used to auto-pull `data.items` / `data.albums` /
`data.artists` / `data.tracks` / `data.results` when no extractor
was supplied. Removed the fallback chain — every section now MUST
provide an explicit `extractItems(data) => array`. Validated at
register-time so misuse fails immediately, not silently on first
load against an endpoint that happened to return two arrays.

Cin standard: explicit > implicit. Magic key-grabbing could pick
the wrong one in edge cases (e.g. an endpoint returning both
`data.albums` and `data.results` would have grabbed albums when
the section actually wanted results).

All 10 existing controller call sites already passed explicit
extractors, so no migration churn — this is purely tightening the
contract for future sections.

REPLACE `renderItems` NULL-RETURN CONVENTION WITH `manualDom: true`

Your Albums and similar sections that delegate to existing renderers
that target a CHILD element of `contentEl` used to signal "leave the
container alone" by returning null/undefined from `renderItems`. That
convention is easy to confuse with an accidental missing-return error.

Replaced with an explicit `manualDom: true` config flag. Renderer is
still called for its side-effects, controller just skips the innerHTML
swap. Clearer intent at the call site. Updated `loadYourAlbums` to
use the new flag.

PIN THE CONTROLLER CONTRACT WITH JS TESTS

Added `tests/static/test_discover_section_controller.mjs` — 32 tests
covering the controller's lifecycle contract:

- Config validation (every required field, mutual exclusivity of
  fetchUrl/data, type checks on contentEl)
- Happy-path fetch → parse → render
- Empty state (default empty render, hideWhenEmpty + sectionEl,
  success=false treated as empty, custom isSuccess override)
- Stale state (fires when isStale returns true, wins over empty,
  custom renderStale override)
- Error state (HTTP non-ok, fetch throws, showErrorToast fires
  window.showToast, default off doesn't fire)
- No-fetch `data:` mode (value + function form, doesn't call fetch)
- manualDom mode (skips innerHTML swap, still calls renderer)
- Callable `fetchUrl` (resolved at load time, refresh re-resolves)
- Load coalescing (concurrent loads share one fetch)
- Refresh bypasses coalescing (re-fires fetch every call)
- Hook error containment (throwing renderer/onSuccess hooks don't
  crash the controller)

Runs via Node's stable built-in `--test` runner — no package.json,
no jest/vitest dependency, no compile step. Just `node --test`.

Pytest wrapper at `tests/test_discover_section_controller_js.py`
shells out to node and asserts clean exit, so the JS tests fail
the regular pytest sweep if the controller contract drifts.
Skipped gracefully when node isn't available or is < 22.

Closes the "controller is a contract, pin it at the test boundary"
gap that Cin would have flagged on review.

VERIFICATION

- 2205/2205 full pytest suite green (was 2204 + 1 new wrapper)
- 32/32 `node --test` pass on the controller test file directly
- ruff clean
- node --check clean on all touched JS files

tests/static/test_discover_section_controller.mjs

@@ -0,0 +1,707 @@
+// Tests for `createDiscoverSectionController` in
+// `webui/static/discover-section-controller.js`. Run via:
+//
+//     node --test tests/static/
+//
+// Or through the Python wrapper at
+// tests/test_discover_section_controller_js.py which shells out to
+// `node --test` and surfaces the result inside the regular pytest run.
+//
+// The controller is loaded into a sandboxed `vm` context with stubbed
+// `window` / `document` / `Element` / `fetch`. No DOM or network — just
+// the lifecycle contract.
+
+import { test, describe, before } from 'node:test';
+import assert from 'node:assert/strict';
+import vm from 'node:vm';
+import { readFileSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
+import { dirname, resolve } from 'node:path';
+
+const __dirname = dirname(fileURLToPath(import.meta.url));
+const CONTROLLER_PATH = resolve(__dirname, '..', '..', 'webui', 'static', 'discover-section-controller.js');
+
+// Minimal Element stub — controller uses `instanceof Element` to tell
+// strings (selectors) apart from DOM refs.
+class Element {
+    constructor(id) {
+        this.id = id;
+        this.innerHTML = '';
+        this.style = { display: '' };
+    }
+}
+
+// Build a fresh sandbox per test so state doesn't leak between cases.
+function makeSandbox(opts = {}) {
+    const elements = new Map();
+    const ensureEl = (sel) => {
+        if (!elements.has(sel)) elements.set(sel, new Element(sel));
+        return elements.get(sel);
+    };
+
+    const sandbox = {
+        Element,
+        window: {},
+        console: {
+            // Quiet by default — turn on by passing { logCalls: true }
+            debug: opts.logCalls ? console.debug : () => {},
+            error: opts.logCalls ? console.error : () => {},
+            log: opts.logCalls ? console.log : () => {},
+        },
+        document: {
+            querySelector: (sel) => ensureEl(sel),
+        },
+        fetch: opts.fetch || (async () => {
+            throw new Error('fetch not stubbed for this test');
+        }),
+        // Toast spy — when controller calls window.showToast, capture it
+        _toasts: [],
+    };
+    sandbox.window.showToast = (msg, type) => sandbox._toasts.push({ msg, type });
+    sandbox._elements = elements;
+    return sandbox;
+}
+
+let CONTROLLER_SOURCE;
+before(() => {
+    CONTROLLER_SOURCE = readFileSync(CONTROLLER_PATH, 'utf8');
+});
+
+function loadController(sandbox) {
+    vm.createContext(sandbox);
+    vm.runInContext(CONTROLLER_SOURCE, sandbox);
+    return sandbox.window.createDiscoverSectionController;
+}
+
+// =========================================================================
+// Config validation
+// =========================================================================
+
+describe('config validation', () => {
+    test('throws on missing id', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.throws(() => create({
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: () => [],
+            renderItems: () => '',
+        }), /config.id required/);
+    });
+
+    test('throws on missing contentEl', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.throws(() => create({
+            id: 'x',
+            fetchUrl: '/u',
+            extractItems: () => [],
+            renderItems: () => '',
+        }), /contentEl required/);
+    });
+
+    test('throws when both fetchUrl and data provided', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.throws(() => create({
+            id: 'x',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            data: { ok: true },
+            extractItems: () => [],
+            renderItems: () => '',
+        }), /mutually exclusive/);
+    });
+
+    test('throws when neither fetchUrl nor data provided', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.throws(() => create({
+            id: 'x',
+            contentEl: '#x',
+            extractItems: () => [],
+            renderItems: () => '',
+        }), /either config.fetchUrl or config.data required/);
+    });
+
+    test('throws when extractItems missing', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.throws(() => create({
+            id: 'x',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            renderItems: () => '',
+        }), /extractItems required/);
+    });
+
+    test('throws when renderItems missing', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.throws(() => create({
+            id: 'x',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: () => [],
+        }), /renderItems required/);
+    });
+
+    test('accepts function fetchUrl', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.doesNotThrow(() => create({
+            id: 'x',
+            contentEl: '#x',
+            fetchUrl: () => '/u',
+            extractItems: () => [],
+            renderItems: () => '',
+        }));
+    });
+
+    test('accepts data instead of fetchUrl', () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        assert.doesNotThrow(() => create({
+            id: 'x',
+            contentEl: '#x',
+            data: { ok: true },
+            extractItems: () => [],
+            renderItems: () => '',
+        }));
+    });
+});
+
+// =========================================================================
+// Happy path — fetch → render
+// =========================================================================
+
+describe('fetch + render lifecycle', () => {
+    test('fetches, parses, calls renderItems, writes innerHTML', async () => {
+        const sandbox = makeSandbox({
+            fetch: async (url) => {
+                assert.equal(url, '/api/test');
+                return {
+                    ok: true,
+                    json: async () => ({ success: true, items: [{ id: 1 }, { id: 2 }] }),
+                };
+            },
+        });
+        const create = loadController(sandbox);
+        let renderCalls = 0;
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#carousel',
+            fetchUrl: '/api/test',
+            extractItems: (data) => data.items,
+            renderItems: (items) => {
+                renderCalls++;
+                return `<x>${items.length}</x>`;
+            },
+        });
+        await ctrl.load();
+        assert.equal(renderCalls, 1);
+        assert.equal(sandbox._elements.get('#carousel').innerHTML, '<x>2</x>');
+        assert.equal(ctrl.getState().phase, 'rendered');
+    });
+
+    test('fires onRendered hook after render', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({
+                ok: true,
+                json: async () => ({ success: true, items: [{ id: 1 }] }),
+            }),
+        });
+        const create = loadController(sandbox);
+        let hookCalls = 0;
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => 'rendered',
+            onRendered: (ctx) => {
+                hookCalls++;
+                assert.ok(ctx.contentEl);
+                assert.ok(ctx.items);
+                assert.ok(ctx.data);
+            },
+        });
+        await ctrl.load();
+        assert.equal(hookCalls, 1);
+    });
+
+    test('fires onSuccess hook after success gate, before render', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({
+                ok: true,
+                json: async () => ({ success: true, items: [], stats: { count: 5 } }),
+            }),
+        });
+        const create = loadController(sandbox);
+        const order = [];
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => { order.push('render'); return ''; },
+            onSuccess: (data) => { order.push(`success:${data.stats.count}`); },
+        });
+        await ctrl.load();
+        // Empty items → no render. onSuccess still fires.
+        assert.deepEqual(order, ['success:5']);
+    });
+
+    test('fires beforeLoad hook before spinner shows', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({
+                ok: true,
+                json: async () => ({ success: true, items: [{ id: 1 }] }),
+            }),
+        });
+        const create = loadController(sandbox);
+        const order = [];
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => { order.push('render'); return 'r'; },
+            beforeLoad: () => { order.push('before'); },
+        });
+        await ctrl.load();
+        assert.equal(order[0], 'before');
+        assert.equal(order[1], 'render');
+    });
+});
+
+// =========================================================================
+// Empty state
+// =========================================================================
+
+describe('empty state', () => {
+    test('renders empty message when items array is empty', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [] }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => '<should-not-appear/>',
+            emptyMessage: 'Nothing here',
+        });
+        await ctrl.load();
+        const html = sandbox._elements.get('#x').innerHTML;
+        assert.match(html, /Nothing here/);
+        assert.doesNotMatch(html, /should-not-appear/);
+        assert.equal(ctrl.getState().phase, 'empty');
+    });
+
+    test('hides whole section when hideWhenEmpty + empty', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [] }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            sectionEl: '#wrapper',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => '',
+            hideWhenEmpty: true,
+        });
+        await ctrl.load();
+        assert.equal(sandbox._elements.get('#wrapper').style.display, 'none');
+    });
+
+    test('treats success=false as empty (default)', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: false, items: [{ id: 1 }] }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => '<should-not-appear/>',
+            emptyMessage: 'X',
+        });
+        await ctrl.load();
+        assert.equal(ctrl.getState().phase, 'empty');
+    });
+
+    test('custom isSuccess overrides default success-flag check', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ status: 'ok', items: [{ id: 1 }] }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            isSuccess: (d) => d.status === 'ok',
+            renderItems: (items) => `r:${items.length}`,
+        });
+        await ctrl.load();
+        assert.equal(sandbox._elements.get('#x').innerHTML, 'r:1');
+    });
+});
+
+// =========================================================================
+// Stale state
+// =========================================================================
+
+describe('stale state', () => {
+    test('renders stale UI + fires onStale when isStale returns true', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [], stale: true }) }),
+        });
+        const create = loadController(sandbox);
+        let staleHookCalled = false;
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            isStale: (items, data) => data.stale && items.length === 0,
+            renderItems: () => '<should-not-appear/>',
+            staleMessage: 'Updating from upstream',
+            onStale: () => { staleHookCalled = true; },
+        });
+        await ctrl.load();
+        assert.equal(ctrl.getState().phase, 'stale');
+        assert.equal(staleHookCalled, true);
+        assert.match(sandbox._elements.get('#x').innerHTML, /Updating from upstream/);
+    });
+
+    test('stale wins over empty when both apply', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [], stale: true }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            isStale: () => true,
+            renderItems: () => '',
+            emptyMessage: 'EMPTY',
+            staleMessage: 'STALE',
+        });
+        await ctrl.load();
+        const html = sandbox._elements.get('#x').innerHTML;
+        assert.match(html, /STALE/);
+        assert.doesNotMatch(html, /EMPTY/);
+    });
+
+    test('custom renderStale overrides default stale UI', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [], stale: true }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            isStale: () => true,
+            renderItems: () => '',
+            renderStale: () => '<custom-stale/>',
+        });
+        await ctrl.load();
+        assert.equal(sandbox._elements.get('#x').innerHTML, '<custom-stale/>');
+    });
+});
+
+// =========================================================================
+// Error state
+// =========================================================================
+
+describe('error state', () => {
+    test('renders error block on HTTP non-ok', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: false, status: 500, json: async () => ({}) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => '',
+            errorMessage: 'load failed',
+        });
+        await ctrl.load();
+        assert.equal(ctrl.getState().phase, 'error');
+        assert.match(sandbox._elements.get('#x').innerHTML, /load failed/);
+    });
+
+    test('renders error block when fetch throws', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => { throw new Error('network down'); },
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => '',
+            errorMessage: 'oops',
+        });
+        await ctrl.load();
+        assert.equal(ctrl.getState().phase, 'error');
+        assert.match(sandbox._elements.get('#x').innerHTML, /oops/);
+    });
+
+    test('fires showToast on error when showErrorToast: true', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => { throw new Error('boom'); },
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => '',
+            errorMessage: 'load broke',
+            showErrorToast: true,
+        });
+        await ctrl.load();
+        assert.equal(sandbox._toasts.length, 1);
+        assert.equal(sandbox._toasts[0].msg, 'load broke');
+        assert.equal(sandbox._toasts[0].type, 'error');
+    });
+
+    test('does NOT fire toast when showErrorToast omitted', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => { throw new Error('boom'); },
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => '',
+        });
+        await ctrl.load();
+        assert.equal(sandbox._toasts.length, 0);
+    });
+});
+
+// =========================================================================
+// No-fetch data: mode
+// =========================================================================
+
+describe('no-fetch data mode', () => {
+    test('renders provided data without calling fetch', async () => {
+        let fetchCalled = false;
+        const sandbox = makeSandbox({
+            fetch: async () => { fetchCalled = true; throw new Error('should not fetch'); },
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            data: { success: true, items: [{ id: 1 }, { id: 2 }] },
+            extractItems: (d) => d.items,
+            renderItems: (items) => `n:${items.length}`,
+        });
+        await ctrl.load();
+        assert.equal(fetchCalled, false);
+        assert.equal(sandbox._elements.get('#x').innerHTML, 'n:2');
+    });
+
+    test('accepts data as a function', async () => {
+        const sandbox = makeSandbox();
+        const create = loadController(sandbox);
+        let dataCalls = 0;
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            data: () => { dataCalls++; return { success: true, items: [{ id: 9 }] }; },
+            extractItems: (d) => d.items,
+            renderItems: (items) => `f:${items[0].id}`,
+        });
+        await ctrl.load();
+        assert.equal(dataCalls, 1);
+        assert.equal(sandbox._elements.get('#x').innerHTML, 'f:9');
+    });
+});
+
+// =========================================================================
+// manualDom mode
+// =========================================================================
+
+describe('manualDom mode', () => {
+    test('does NOT write renderItems return into contentEl', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [{ id: 1 }] }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            manualDom: true,
+            extractItems: (d) => d.items,
+            renderItems: () => '<should-not-appear/>',
+        });
+        await ctrl.load();
+        const html = sandbox._elements.get('#x').innerHTML;
+        // Spinner from _showLoading was the last write; manualDom mode
+        // didn't replace it. The renderer gets called for side-effects
+        // (which the test doesn't exercise here) but innerHTML stays
+        // whatever the loading spinner left.
+        assert.doesNotMatch(html, /should-not-appear/);
+        assert.equal(ctrl.getState().phase, 'rendered');
+    });
+
+    test('still fires renderItems for side-effects', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [{ id: 1 }] }) }),
+        });
+        const create = loadController(sandbox);
+        let renderCalled = false;
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            manualDom: true,
+            extractItems: (d) => d.items,
+            renderItems: () => { renderCalled = true; },
+        });
+        await ctrl.load();
+        assert.equal(renderCalled, true);
+    });
+});
+
+// =========================================================================
+// Fetch URL forms
+// =========================================================================
+
+describe('fetchUrl forms', () => {
+    test('callable fetchUrl is invoked at load time', async () => {
+        let urlCalls = 0;
+        const sandbox = makeSandbox({
+            fetch: async (url) => {
+                assert.equal(url, '/u/computed');
+                return { ok: true, json: async () => ({ success: true, items: [{ id: 1 }] }) };
+            },
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: () => { urlCalls++; return '/u/computed'; },
+            extractItems: (d) => d.items,
+            renderItems: () => 'r',
+        });
+        await ctrl.load();
+        assert.equal(urlCalls, 1);
+        // Calling refresh re-resolves the URL — important for sections
+        // whose URL depends on runtime state (e.g. season key).
+        await ctrl.refresh();
+        assert.equal(urlCalls, 2);
+    });
+});
+
+// =========================================================================
+// Coalescing + refresh
+// =========================================================================
+
+describe('load coalescing and refresh', () => {
+    test('two concurrent load() calls share one fetch', async () => {
+        let fetchCalls = 0;
+        const sandbox = makeSandbox({
+            fetch: async () => {
+                fetchCalls++;
+                // Yield once so both load() calls land on the same in-flight promise.
+                await new Promise((r) => setImmediate(r));
+                return { ok: true, json: async () => ({ success: true, items: [{ id: 1 }] }) };
+            },
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => 'r',
+        });
+        await Promise.all([ctrl.load(), ctrl.load(), ctrl.load()]);
+        assert.equal(fetchCalls, 1);
+    });
+
+    test('refresh() bypasses the coalesce and re-fetches', async () => {
+        let fetchCalls = 0;
+        const sandbox = makeSandbox({
+            fetch: async () => {
+                fetchCalls++;
+                return { ok: true, json: async () => ({ success: true, items: [{ id: 1 }] }) };
+            },
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => 'r',
+        });
+        await ctrl.load();
+        await ctrl.refresh();
+        await ctrl.refresh();
+        assert.equal(fetchCalls, 3);
+    });
+});
+
+// =========================================================================
+// Hook error containment
+// =========================================================================
+
+describe('hook error containment', () => {
+    test('throwing renderer hook does not crash the controller', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [{ id: 1 }] }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => 'r',
+            onRendered: () => { throw new Error('hook boom'); },
+        });
+        // Test passes if this doesn't throw out of the await.
+        await ctrl.load();
+        assert.equal(ctrl.getState().phase, 'rendered');
+    });
+
+    test('throwing onSuccess hook does not block the render', async () => {
+        const sandbox = makeSandbox({
+            fetch: async () => ({ ok: true, json: async () => ({ success: true, items: [{ id: 1 }] }) }),
+        });
+        const create = loadController(sandbox);
+        const ctrl = create({
+            id: 'test',
+            contentEl: '#x',
+            fetchUrl: '/u',
+            extractItems: (d) => d.items,
+            renderItems: () => 'rendered-anyway',
+            onSuccess: () => { throw new Error('boom'); },
+        });
+        await ctrl.load();
+        assert.equal(sandbox._elements.get('#x').innerHTML, 'rendered-anyway');
+    });
+});

tests/test_discover_section_controller_js.py

@@ -0,0 +1,76 @@
+"""Run the JS tests for `webui/static/discover-section-controller.js`
+under the regular pytest sweep.
+
+The actual contract tests live in
+`tests/static/test_discover_section_controller.mjs` and run via
+Node.js's stable built-in test runner (`node --test`). This shim
+shells out to that runner and asserts a clean exit so the JS tests
+fail the suite if the controller contract drifts.
+
+Skipped when:
+  - `node` isn't on PATH (e.g. Python-only dev container).
+  - Node version < 22 (the built-in `--test` runner went stable in 18
+    but the assert-flavor we use is 22+).
+
+Run directly:
+    node --test tests/static/test_discover_section_controller.mjs
+"""
+
+from __future__ import annotations
+
+import shutil
+import subprocess
+from pathlib import Path
+
+import pytest
+
+
+_REPO_ROOT = Path(__file__).resolve().parents[1]
+_TEST_FILE = _REPO_ROOT / "tests" / "static" / "test_discover_section_controller.mjs"
+
+
+def _node_available() -> bool:
+    if not shutil.which("node"):
+        return False
+    try:
+        result = subprocess.run(
+            ["node", "--version"],
+            capture_output=True, text=True, timeout=10,
+        )
+    except (subprocess.SubprocessError, FileNotFoundError):
+        return False
+    if result.returncode != 0:
+        return False
+    # Output looks like "v22.21.0"
+    raw = (result.stdout or "").strip().lstrip("v")
+    try:
+        major = int(raw.split(".")[0])
+    except (ValueError, IndexError):
+        return False
+    return major >= 22
+
+
+def test_discover_section_controller_js():
+    """Pin the JS controller's lifecycle contract via `node --test`."""
+    if not _node_available():
+        pytest.skip("Node.js >= 22 required to run the JS controller tests")
+
+    if not _TEST_FILE.exists():
+        pytest.skip(f"JS test file missing: {_TEST_FILE}")
+
+    result = subprocess.run(
+        ["node", "--test", str(_TEST_FILE)],
+        capture_output=True, text=True,
+        cwd=str(_REPO_ROOT),
+        timeout=60,
+    )
+
+    if result.returncode != 0:
+        # Surface the node test runner output so failures are
+        # debuggable from the pytest log without re-running by hand.
+        pytest.fail(
+            "JS controller tests failed:\n\n"
+            f"--- stdout ---\n{result.stdout}\n"
+            f"--- stderr ---\n{result.stderr}",
+            pytrace=False,
+        )

webui/static/discover-section-controller.js

@@ -72,10 +72,11 @@
  *   in addition to the in-section error block. Default off — sections
  *   that have no recovery action shouldn't shout at the user.
  *
- *   If `renderItems` returns null / undefined, the controller leaves
+ *   `manualDom: true` tells the controller to NOT write the
- *   `contentEl` untouched. Lets a renderer do its own DOM manipulation
+ *   `renderItems` return value into `contentEl`. The renderer takes
- *   (e.g. dynamic per-item child containers) without fighting the
+ *   full responsibility for the DOM (e.g. delegating to an existing
- *   controller's `innerHTML` swap.
+ *   grid renderer that targets a child element). The renderer is
+ *   still called, just for its side-effects. Default false.
  */
 
 (function () {
@@ -102,6 +103,14 @@
         if (typeof cfg.renderItems !== 'function') {
             throw new Error(`[discover:${cfg.id}] config.renderItems required (function)`);
         }
+        // Cin standard — explicit > implicit. Each section knows its own
+        // response shape; the controller refusing to guess prevents
+        // silent wrong-key bugs (e.g. an endpoint that returns
+        // `data.results` getting auto-pulled instead of the intended
+        // `data.tracks`).
+        if (typeof cfg.extractItems !== 'function') {
+            throw new Error(`[discover:${cfg.id}] config.extractItems required (function returning array)`);
+        }
     }
 
     function _resolveEl(el) {
@@ -150,6 +159,11 @@
             // Errors
             verboseErrors: false,
             showErrorToast: false,  // also fire window.showToast on error
+            // Renderer takes responsibility for the DOM — controller
+            // calls renderItems but does NOT write its return value
+            // into contentEl. Use when delegating to an existing
+            // renderer that targets a child element.
+            manualDom: false,
         }, cfg);
 
         const state = {
@@ -267,13 +281,11 @@
         }
 
         function _extractItems(data) {
-            if (config.extractItems) return config.extractItems(data) || [];
+            // Validation guarantees `extractItems` is a function. Wrap
-            if (Array.isArray(data?.items)) return data.items;
+            // the call so a renderer-side typo (returning undefined etc)
-            if (Array.isArray(data?.albums)) return data.albums;
+            // doesn't crash the loop — fall back to empty list.
-            if (Array.isArray(data?.artists)) return data.artists;
+            const items = config.extractItems(data);
-            if (Array.isArray(data?.tracks)) return data.tracks;
+            return Array.isArray(items) ? items : [];
-            if (Array.isArray(data?.results)) return data.results;
-            return [];
         }
 
         function _isSuccess(data) {
@@ -384,11 +396,15 @@
 
                     _showSection();
                     const html = config.renderItems(items, data, _ctx({ items, data }));
-                    // null / undefined return = renderer is doing its own
+                    if (!config.manualDom) {
-                    // DOM work, leave the container alone.
+                        // Default: controller owns the DOM. Renderer
-                    if (html !== null && html !== undefined) {
+                        // returns the HTML, controller swaps it in.
-                        _setHtml(contentEl, html);
+                        // Falsy returns become an empty container.
+                        _setHtml(contentEl, html || '');
                     }
+                    // manualDom mode: renderer already wrote whatever
+                    // it needs into the page; controller leaves
+                    // contentEl alone.
                     state.phase = 'rendered';
 
                     if (typeof config.onRendered === 'function') {

webui/static/discover.js

@@ -952,15 +952,15 @@ async function loadYourAlbums() {
                 if (downloadBtn && data.stats && data.stats.missing > 0) downloadBtn.style.display = '';
             },
             // Renderer delegates to the existing grid renderer, which
-            // writes its own DOM into `#your-albums-grid`. Returning
+            // writes its own DOM into `#your-albums-grid`. `manualDom`
-            // null keeps the controller from clobbering it.
+            // tells the controller not to clobber it.
+            manualDom: true,
             renderItems: (items, data) => {
                 yourAlbums = items;
                 yourAlbumsTotal = data.total || 0;
                 yourAlbumsPage = 1;
                 _renderYourAlbumsGrid(yourAlbums);
                 _renderYourAlbumsPagination(yourAlbumsTotal, yourAlbumsPage);
-                return null;
             },
             errorMessage: 'Failed to load your albums',
             verboseErrors: true,

webui/static/helper.js

@@ -3432,6 +3432,7 @@ const WHATS_NEW = {
     '2.4.3': [
         // --- post-2.4.2 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps ---
         { date: 'Unreleased — 2.4.3 dev cycle' },
+        { title: 'Internal: Discover Controller — Cin Pre-Review Polish', desc: 'tightened the controller before opening the PR. (1) dropped the magic `extractItems` defaults — controller used to auto-pull `data.items` / `data.albums` / `data.artists` / `data.tracks` / `data.results` if no extractor was provided. removed the fallback chain. each section now MUST supply its own `extractItems(data) => array` callback. cin standard: explicit > implicit; the auto-fallback could silently grab the wrong key on endpoints that return multiple arrays. validated at register-time so misuse fails immediately. all 10 existing call sites already had explicit extractors so no migration churn. (2) replaced the `renderItems` returning null convention (used by Your Albums + manualDom-style sections) with an explicit `manualDom: true` config flag. clearer intent at the call site, less likely to be confused with a renderer error. (3) added a minimal node `--test` JS test file at `tests/static/test_discover_section_controller.mjs` — 32 tests pin the lifecycle contract: config validation (every required field), happy-path fetch+render, empty/stale/error states, no-fetch `data:` mode, manualDom mode, callable `fetchUrl`, load coalescing, refresh bypass, hook error containment, error toasts. runs via `node --test tests/static/` directly, OR via the regular pytest sweep (`tests/test_discover_section_controller_js.py` shells out to node and asserts a clean exit). skipped gracefully when node isn\'t available or is < 22. closes the "controller is a contract, pin it at the test boundary" gap that cin would have flagged. 2205/2205 full suite green (was 2204 + 1 new pytest wrapper); 32/32 node --test pass; ruff clean; js parses clean.', page: 'discover' },
         { title: 'Internal: Discover Cleanup Round — Toast Errors, Stale State, Skipped Sections', desc: 'follow-up to the controller migration. extended `createDiscoverSectionController` with the hooks the per-section migrations surfaced as needed: callable `fetchUrl` (resolves the seasonal-playlist recreate-on-key-change hack), no-fetch `data:` mode (lets render-only sections like seasonal albums use the controller without inventing a fake endpoint), `beforeLoad` hook (lets dynamically-inserted sections like because-you-listen-to ensure their container exists before the spinner shows), `onSuccess(data)` hook (cleaner home for sibling header / subtitle / button updates than folding them into renderItems), and an `isStale` / `onStale` / `renderStale` triple for the third render state (data is empty BUT upstream is still discovering — show updating UI + start a poller, instead of the bare empty-state copy). turned on `showErrorToast: true` for every migrated section — section load failures now surface a global toast instead of silently spinning forever or swallowing into console.debug. that\'s the JohnBaumb #369 pattern applied at the UI layer. migrated the two sections that didn\'t fit the original controller contract: `loadYourAlbums` (uses isStale/onStale for stale-fetch UI + onSuccess for subtitle/filters/download-button side-effects + renderItems returning null since it delegates to the existing grid renderer) and `loadSeasonalAlbums` (uses no-fetch data mode since the parent `loadSeasonalContent` already fetched the season payload). also lifted the duplicated decade-tab + genre-tab sync-status block (✓/⏳/✗/percentage) into a `_renderSyncStatusBlock(idPrefix)` helper — two call sites now share one implementation. listenbrainz playlists keep their own block because the semantics differ (matching progress vs download progress). audit found the 13 supposedly-dead hidden sections aren\'t dead at all — they\'re gated on user data (discovery pool, library content, metadata cache) and self-surface when their data exists. removed one orphaned `loadPersonalizedDailyMixes()` call from `blockDiscoveryArtist` — daily mixes is intentionally paused, refreshing it from there was a no-op.', page: 'discover' },
         { title: 'Internal: Migrate 7 More Discover Sections to the Controller', desc: 'follow-up to the foundation commit. migrated fresh tape, the archives, time machine intro carousel, browse by genre intro carousel, seasonal mix, your artists, and because-you-listen-to onto `createDiscoverSectionController`. each one drops its own hand-rolled try/catch + spinner injection + empty-state HTML + error swallow in favor of a config object — controller owns the lifecycle. net 76 lines smaller in discover.js even after adding the per-section render helpers. skipped two sections that don\'t fit the controller\'s single-fetch / single-render-target shape: `loadYourAlbums` (paginated grid + filters, four separate UI elements updated) and `loadSeasonalAlbums` (no fetch — receives pre-fetched data from parent). hidden / dead sections (~13 of them) untouched in this pass — separate audit commit will surface or kill them. controller extension candidates surfaced for follow-up: callable `fetchUrl` (so seasonal playlist doesn\'t need controller-recreate-on-key-change), explicit `isStale` / `onStale` hook (so your-artists doesn\'t fold stale handling into renderItems), `beforeLoad` hook (so because-you-listen-to can let the controller own the dynamic container creation), and a no-fetch `data:` mode (so render-only sections like seasonal albums can use the controller). zero behavior changes — every public load function keeps its name + signature so existing callers, refresh buttons, and dashboard wiring don\'t notice the swap.', page: 'discover' },
         { title: 'Internal: Discover Section Controller Foundation', desc: 'every section on the discover page (recent releases, your artists, your albums, seasonal, fresh tape, the archives, etc) re-implements the same lifecycle by hand: show spinner → fetch endpoint → parse → either render or show empty state or show error → maybe wire post-render handlers → maybe expose refresh. ~30 sections, all subtly drifting — different empty messages, different error handling (some console.debug, some silently swallowed, some leave the spinner spinning forever), different sync-status icons, no consistent error toast. lifted that lifecycle into a shared `createDiscoverSectionController` (renderers stay per-section because section data shapes legitimately differ — album cards vs artist circles vs playlist tiles vs track rows; the controller is the wrapper, not a forced visual abstraction). this commit is the foundation: built the controller + migrated `recent releases` as proof. each remaining section will migrate in its own follow-up commit (keeps reviews small + lets us sequence the work). once everything is on the controller, the discover-page cleanup work (kill 13 dead sections, standardize sync-status icons, add error toasts) becomes single-line registry edits instead of section-by-section rewrites.', page: 'discover' },