3 Commits (main)

Author	SHA1	Message	Date
Broque Thomas	c557d9196e	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	3 weeks ago
Broque Thomas	dc2323cde6	Discover cleanup: controller extensions, toast errors, migrate skipped sections ... Follow-up to the controller migration commits. Closes out the extension list the per-section migrations surfaced as needed. CONTROLLER EXTENSIONS - Callable `fetchUrl: () => string` — resolves the seasonal-playlist recreate-on-key-change hack from the prior commit. - No-fetch `data:` mode — value or `() => value`. Lets render-only sections like Seasonal Albums use the controller without inventing a fake endpoint. Mutually exclusive with `fetchUrl`; validated up front so misuse fails at register-time. - `beforeLoad(ctx)` hook — runs before the spinner shows. Lets dynamically-inserted sections like Because You Listen To ensure their `contentEl` exists before the visibility check. - `onSuccess(data, ctx)` hook — runs after the success gate but before isEmpty / isStale. Cleaner home for sibling header / subtitle / button updates than folding them into renderItems. - `isStale(items, data)` + `onStale(ctx)` + `renderStale(items, data)` + `staleMessage` — third render state for "data is empty BUT upstream is still discovering". Stale wins over empty when both apply. Default stale UI is the same spinner block used elsewhere. - `showErrorToast: true` config — opens a global `showToast(...)` in addition to the in-section error block. Default off; sections that have no recovery action shouldn't shout at the user. - `renderItems` returning null/undefined now leaves contentEl untouched. Lets a renderer do its own DOM manipulation (e.g. delegating to an existing grid-render fn that targets a child element) without fighting the controller's innerHTML swap. MIGRATED THE 2 SKIPPED SECTIONS - `loadYourAlbums` — uses `isStale`/`onStale`/`renderStale` for the stale-fetch state, `onSuccess` for the subtitle/filters/download side-effects, `hideWhenEmpty` + `sectionEl` for the truly-empty case, `renderItems` returning null since it delegates to the existing `_renderYourAlbumsGrid` + `_renderYourAlbumsPagination`. - `loadSeasonalAlbums` — uses no-fetch `data:` mode because the parent `loadSeasonalContent` already fetched the season payload. `beforeLoad` updates the sibling title/subtitle text. ERROR TOASTS ON ALL MIGRATED SECTIONS Every migrated section now has `showErrorToast: true`. Section load failures surface a global toast instead of silently spinning forever or swallowing into console.debug. Same pattern JohnBaumb #369 asked for at the Python layer, applied at the UI layer. SHARED SYNC-STATUS BLOCK Lifted the duplicated decade-tab + genre-tab sync-status HTML (✓ completed | ⏳ pending | ✗ failed | percentage) into a single `_renderSyncStatusBlock(idPrefix)` helper. Two call sites now share one implementation. ListenBrainz playlists keep their own block because the semantics differ — matching progress (total / matched / failed) vs download progress. DEAD-SECTION AUDIT — NONE DEAD Audited the 13 supposedly-dead hidden sections from DISCOVER_REVIEW.md. All 13 are alive: gated on user data (discovery pool, library content, metadata cache) and self-surface when their data exists via `style.display = 'block'` on the success path. The review's grep missed the toggle. No deletions made. DAILY MIXES ORPHAN CALL Removed the orphaned `loadPersonalizedDailyMixes()` call from `blockDiscoveryArtist` — Daily Mixes is intentionally paused (its load call in `loadDiscoverPage` is commented out) so refreshing it from the post-block hook was a no-op. 2204/2204 full suite green. JS parses clean (`node --check`).	3 weeks ago
Broque Thomas	07a71f0432	Discover section controller foundation + migrate Recent Releases ... Every section on the discover page (Recent Releases, Your Artists, Your Albums, Seasonal Albums, Seasonal Mix, Fresh Tape, The Archives, Build Playlist, Time Machine, Browse by Genre, ListenBrainz Playlists, Because You Listen To, plus ~13 hidden sections) currently re-implements the same lifecycle by hand: 1. show a loading spinner in the carousel container 2. fetch the section's endpoint 3. parse the response, decide if the data is empty 4. either render the items, show an empty-state, or show an error 5. wire post-render handlers (download buttons, hover behavior, etc) 6. maybe expose refresh() ~30 sections worth of duplicated boilerplate, all subtly drifting. Different empty-state messages. Different error handling (some `console.debug`, some silently swallowed, some leave the spinner spinning forever). Different sync-status icons (✓/⏳/✗ vs ♪/✓/✗). No consistent error toast. Lifted the lifecycle into a shared `createDiscoverSectionController` in `webui/static/discover-section-controller.js`. 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. Foundation contract: createDiscoverSectionController({ id: 'recent-releases', // for diagnostic logging contentEl: '#carousel', // selector or Element fetchUrl: '/api/discover/...', extractItems: (data) => [...], // pull list from response renderItems: (items, data, ctx) => '<html>', onRendered: (ctx) => { ... }, // optional post-render hook loadingMessage / emptyMessage / errorMessage: copy sectionEl + hideWhenEmpty: optional whole-section visibility isSuccess / isEmpty: optional gate overrides }) Returns `{ load, refresh, destroy, getState }`. Validates config up front so misuse fails at register-time, not silently on load. Coalesces concurrent loads (same in-flight promise returned) so a double-click or repeated trigger doesn't double-fetch. `refresh()` bypasses the coalesce so the refresh button always re-fires. Errors are logged (console.debug by default, console.error when verboseErrors=true). Renderer hook errors are caught + logged so a buggy render callback can't tear down the controller — keeps the page resilient. Migrated `Recent Releases` as the proof — simplest album-card shape, no source-gating, no refresh button. Verified the contract covers it end-to-end. The legacy `loadDiscoverRecentReleases` entry-point stays public so existing callers don't change; internally it lazy-builds the controller and triggers `load()`. NOT in this commit: - Other section migrations (one section per follow-up commit, keeps reviews small + lets us sequence the work) - Registry-driven section list (so the dead-section audit becomes registry deletions instead of section-by-section removal) - Global error toast wrapper - Per-section "requires X primary source" gate - Sync-status icon renderer unification Once every section is on the controller, the discover-page cleanup work (kill the 13 dead sections, standardize sync-status icons, add error toasts) becomes single-line registry-level edits instead of 30 separate section-by-section rewrites. 2204/2204 full suite green. JS parses clean (`node --check`). Manual smoke deferred until follow-up commits — Recent Releases unchanged on the wire (same endpoint, same payload shape, same render output).	3 weeks ago