diff --git a/tests/static/test_discover_section_controller.mjs b/tests/static/test_discover_section_controller.mjs new file mode 100644 index 00000000..8cc1b655 --- /dev/null +++ b/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 `${items.length}`; + }, + }); + await ctrl.load(); + assert.equal(renderCalls, 1); + assert.equal(sandbox._elements.get('#carousel').innerHTML, '2'); + 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: () => '', + 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: () => '', + 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: () => '', + 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: () => '', + }); + await ctrl.load(); + assert.equal(sandbox._elements.get('#x').innerHTML, ''); + }); +}); + +// ========================================================================= +// 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: () => '', + }); + 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'); + }); +}); diff --git a/tests/test_discover_section_controller_js.py b/tests/test_discover_section_controller_js.py new file mode 100644 index 00000000..8ebcd1d1 --- /dev/null +++ b/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, + ) diff --git a/webui/static/discover-section-controller.js b/webui/static/discover-section-controller.js index f3caf0b8..fc60635b 100644 --- a/webui/static/discover-section-controller.js +++ b/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 - * `contentEl` untouched. Lets a renderer do its own DOM manipulation - * (e.g. dynamic per-item child containers) without fighting the - * controller's `innerHTML` swap. + * `manualDom: true` tells the controller to NOT write the + * `renderItems` return value into `contentEl`. The renderer takes + * full responsibility for the DOM (e.g. delegating to an existing + * 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) || []; - if (Array.isArray(data?.items)) return data.items; - if (Array.isArray(data?.albums)) return data.albums; - if (Array.isArray(data?.artists)) return data.artists; - if (Array.isArray(data?.tracks)) return data.tracks; - if (Array.isArray(data?.results)) return data.results; - return []; + // Validation guarantees `extractItems` is a function. Wrap + // the call so a renderer-side typo (returning undefined etc) + // doesn't crash the loop — fall back to empty list. + const items = config.extractItems(data); + return Array.isArray(items) ? items : []; } 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 - // DOM work, leave the container alone. - if (html !== null && html !== undefined) { - _setHtml(contentEl, html); + if (!config.manualDom) { + // Default: controller owns the DOM. Renderer + // returns the HTML, controller swaps it in. + // 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') { diff --git a/webui/static/discover.js b/webui/static/discover.js index d89b5927..c7ba7e36 100644 --- a/webui/static/discover.js +++ b/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 - // null keeps the controller from clobbering it. + // writes its own DOM into `#your-albums-grid`. `manualDom` + // 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, diff --git a/webui/static/helper.js b/webui/static/helper.js index 2324a496..7323a284 100644 --- a/webui/static/helper.js +++ b/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' },