From dc2323cde6085c9789bad6dc0e674398d67dff70 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Thu, 7 May 2026 20:05:39 -0700 Subject: [PATCH] Discover cleanup: controller extensions, toast errors, migrate skipped sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`). --- webui/static/discover-section-controller.js | 266 ++++++++++++++------ webui/static/discover.js | 260 ++++++++++--------- webui/static/helper.js | 1 + 3 files changed, 328 insertions(+), 199 deletions(-) diff --git a/webui/static/discover-section-controller.js b/webui/static/discover-section-controller.js index fdfef9af..f3caf0b8 100644 --- a/webui/static/discover-section-controller.js +++ b/webui/static/discover-section-controller.js @@ -5,17 +5,17 @@ * Owns the lifecycle every discover-page section already does by hand: * * 1. show a loading spinner in the carousel container - * 2. fetch the section's endpoint + * 2. fetch the section's endpoint (or use pre-fetched data) * 3. parse the response, decide whether the data is empty - * 4. either show the empty state, render the items, or show an error + * 4. either show the empty state, render the items, show a stale + * "still updating" state, or show an error * 5. wire any post-render handlers (download buttons, hover, etc) * 6. expose a refresh() method so the same lifecycle can re-fire * - * Each section currently re-implements this 30 times in `discover.js` + * Each section currently re-implements this by hand in `discover.js` * with subtle drift — different empty-state messages, inconsistent - * error handling (some console.debug, some silently swallowed, some - * leave the spinner spinning forever), inconsistent refresh-button - * feedback. This controller is the "lift what's truly shared" + * error handling, inconsistent refresh-button feedback, no consistent + * error toast. This controller is the "lift what's truly shared" * extraction: register a section once, the controller handles the * lifecycle, the section provides only its renderer. * @@ -24,12 +24,6 @@ * rows). The controller is the lifecycle wrapper around those * renderers, not a forced visual abstraction. * - * MIGRATION STATUS: this is the foundation commit. Only `Recent - * Releases` has been migrated as a proof. The other sections - * (Your Artists, Your Albums, Seasonal, Fresh Tape, The Archives, - * etc) still use their hand-rolled load functions in discover.js - * and will migrate one section per commit. - * * USAGE: * * const ctrl = createDiscoverSectionController({ @@ -45,19 +39,48 @@ * }); * ctrl.load(); * - * Future enhancements (not in this foundation commit): - * - global error toast wrapper (so users see something when an - * endpoint fails instead of the silent-empty-state default) - * - registry-driven section list (so the dead-section audit - * becomes registry edits, not section-by-section deletions) - * - per-section "requires X primary source" gate + * EXTENSIONS: + * + * `fetchUrl` accepts a function returning a string for sections + * whose endpoint depends on runtime state (e.g. seasonal playlist + * keyed by `currentSeasonKey`). + * + * `data` lets a section bypass fetch entirely — the controller still + * runs success / empty / render / onRendered, just without going to + * the network. Use when a parent already fetched and just wants the + * shared lifecycle. `data` may be a value or a `() => value` + * function. Sections must supply EITHER `fetchUrl` OR `data`, not + * both. + * + * `beforeLoad(ctx)` runs before the spinner shows. Useful for + * ensuring `contentEl` exists (e.g. dynamically inserted sections) + * or updating sibling headers / subtitles before any visual change. + * + * `onSuccess(data, ctx)` runs after the success check passes but + * before isEmpty / isStale checks. Cleaner home for header text + * updates that depend on response data (vs folding them into + * renderItems). + * + * `isStale(items, data)` + `onStale(ctx)` give sections a third + * render state for "data is empty but the upstream is still + * discovering". Returning true from `isStale` renders the stale + * state (default: spinner + "Updating..." copy, override via + * `renderStale` or `staleMessage`) and fires `onStale` so the + * section can start a poller. Stale wins over empty when both apply. + * + * `showErrorToast: true` opens a global `showToast(...)` on error + * 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. */ (function () { 'use strict'; - // Validate the config object up front — bad configs fail fast at - // section-register time instead of silently breaking on load. function _validateConfig(cfg) { if (!cfg || typeof cfg !== 'object') { throw new Error('createDiscoverSectionController: config required'); @@ -68,8 +91,13 @@ if (typeof cfg.contentEl !== 'string' && !(cfg.contentEl instanceof Element)) { throw new Error(`[discover:${cfg.id}] config.contentEl required (selector or Element)`); } - if (typeof cfg.fetchUrl !== 'string' || !cfg.fetchUrl) { - throw new Error(`[discover:${cfg.id}] config.fetchUrl required`); + const hasFetch = (typeof cfg.fetchUrl === 'string' && cfg.fetchUrl) || typeof cfg.fetchUrl === 'function'; + const hasData = cfg.data !== undefined; + if (!hasFetch && !hasData) { + throw new Error(`[discover:${cfg.id}] either config.fetchUrl or config.data required`); + } + if (hasFetch && hasData) { + throw new Error(`[discover:${cfg.id}] config.fetchUrl and config.data are mutually exclusive`); } if (typeof cfg.renderItems !== 'function') { throw new Error(`[discover:${cfg.id}] config.renderItems required (function)`); @@ -90,47 +118,42 @@ _validateConfig(cfg); const config = Object.assign({ - // Wrapper element to show/hide when section becomes empty. - // Null = always visible, only the contents change. sectionEl: null, - // Hide the whole section if the response is empty - // (vs showing an empty-state message inside the carousel). hideWhenEmpty: false, - // For sections that need to show data even on empty (e.g. - // "Recent Releases" with "no recent releases" copy). - // When true and items.length === 0, render the empty state. renderEmptyState: true, - // HTTP method + options for the fetch call. Default GET, no - // body. fetchOptions is a function so callers can compute it - // at load time (e.g. read filter selects). fetchMethod: 'GET', fetchOptions: null, - // Pull the items array from the response. Default looks - // for `data.items` then `data.albums` then `data.artists`. + // Either fetchUrl (string or () => string) or data + // (value or () => value). Validated mutually exclusive above. extractItems: null, - // Override the success check. Default: require data.success - // when present, otherwise treat any 2xx as success. isSuccess: null, - // Override empty-detection. Default: items.length === 0. isEmpty: null, - // Post-render hook — attach event handlers, etc. - onRendered: null, - // Lifecycle copy. Empty string = no message. + // Stale = data is empty but upstream is still discovering. + // Returning true here renders the stale state instead of + // empty, and fires onStale so the section can poll. + isStale: null, + renderStale: null, + staleMessage: 'Updating...', + // Hooks + beforeLoad: null, // (ctx) => void — before spinner shows + onSuccess: null, // (data, ctx) => void — after success gate + onStale: null, // (ctx) => void — when stale state renders + onRendered: null, // (ctx) => void — after content renders + // UX copy loadingMessage: 'Loading...', emptyMessage: 'Nothing to show', errorMessage: 'Failed to load', - // CSS class names — let surfaces override for styling. loadingClass: 'discover-loading', emptyClass: 'discover-empty', errorClass: 'discover-empty', - // When true, log full error stacks to console.error. Default - // logs to console.debug — keeps the console quiet for users - // while staying inspectable when devtools is open. + staleClass: 'discover-loading', + // Errors verboseErrors: false, + showErrorToast: false, // also fire window.showToast on error }, cfg); const state = { - phase: 'idle', // idle | loading | rendered | empty | error + phase: 'idle', // idle | loading | rendered | empty | stale | error lastData: null, lastError: null, inFlight: null, @@ -140,6 +163,13 @@ if (el) el.innerHTML = html; } + function _ctx(extra) { + return Object.assign( + { contentEl: _resolveEl(config.contentEl), config }, + extra || {}, + ); + } + function _showLoading() { const contentEl = _resolveEl(config.contentEl); if (!contentEl) return; @@ -176,6 +206,40 @@ state.phase = 'empty'; } + function _showStale(items, data) { + const contentEl = _resolveEl(config.contentEl); + if (!contentEl) return; + _showSection(); + // Custom renderStale wins. Otherwise default spinner + copy. + let html; + if (typeof config.renderStale === 'function') { + try { + html = config.renderStale(items, data, _ctx({ items, data })); + } catch (err) { + console.debug(`[discover:${config.id}] renderStale threw:`, err); + html = null; + } + } + if (html === null || html === undefined) { + html = ` +
+ +

${config.staleMessage}

+
+ `; + } + _setHtml(contentEl, html); + state.phase = 'stale'; + + if (typeof config.onStale === 'function') { + try { + config.onStale(_ctx({ items, data })); + } catch (err) { + console.debug(`[discover:${config.id}] onStale hook threw:`, err); + } + } + } + function _showError(error) { const contentEl = _resolveEl(config.contentEl); if (!contentEl) return; @@ -188,6 +252,13 @@ state.lastError = error; const log = config.verboseErrors ? console.error : console.debug; log(`[discover:${config.id}]`, error); + if (config.showErrorToast && typeof window.showToast === 'function') { + try { + window.showToast(config.errorMessage, 'error'); + } catch (toastErr) { + console.debug(`[discover:${config.id}] toast failed:`, toastErr); + } + } } function _showSection() { @@ -197,7 +268,6 @@ function _extractItems(data) { if (config.extractItems) return config.extractItems(data) || []; - // Sensible defaults for the most common response shapes. if (Array.isArray(data?.items)) return data.items; if (Array.isArray(data?.albums)) return data.albums; if (Array.isArray(data?.artists)) return data.artists; @@ -208,8 +278,6 @@ function _isSuccess(data) { if (config.isSuccess) return config.isSuccess(data); - // If `success` is present, require it to be truthy. Otherwise - // a 2xx response with parseable JSON counts as success. if (data && Object.prototype.hasOwnProperty.call(data, 'success')) { return Boolean(data.success); } @@ -221,11 +289,40 @@ return !Array.isArray(items) || items.length === 0; } + function _isStale(items, data) { + if (typeof config.isStale !== 'function') return false; + try { + return Boolean(config.isStale(items, data)); + } catch (err) { + console.debug(`[discover:${config.id}] isStale threw:`, err); + return false; + } + } + + function _resolveFetchUrl() { + if (typeof config.fetchUrl === 'function') return config.fetchUrl(); + return config.fetchUrl; + } + + function _resolveStaticData() { + if (typeof config.data === 'function') return config.data(); + return config.data; + } + async function load() { - // Coalesce concurrent loads — if a fetch is already in flight, - // return the same promise rather than firing a second call. + // Coalesce concurrent loads — refresh() bypasses the coalesce. if (state.inFlight) return state.inFlight; + // Run beforeLoad first so it can set up `contentEl` (dynamic + // section creation) before the visibility check below. + if (typeof config.beforeLoad === 'function') { + try { + config.beforeLoad(_ctx()); + } catch (err) { + console.debug(`[discover:${config.id}] beforeLoad hook threw:`, err); + } + } + const contentEl = _resolveEl(config.contentEl); if (!contentEl) { console.debug(`[discover:${config.id}] contentEl not found, skipping load`); @@ -234,49 +331,70 @@ _showLoading(); - const fetchOpts = (typeof config.fetchOptions === 'function') - ? (config.fetchOptions() || {}) - : {}; - const init = Object.assign( - { method: config.fetchMethod }, - fetchOpts, - ); - const promise = (async () => { try { - const resp = await fetch(config.fetchUrl, init); - if (!resp.ok) { - throw new Error(`HTTP ${resp.status}`); + let data; + if (config.data !== undefined) { + // No-fetch mode — parent already has the data. + data = _resolveStaticData(); + } else { + const fetchOpts = (typeof config.fetchOptions === 'function') + ? (config.fetchOptions() || {}) + : {}; + const init = Object.assign( + { method: config.fetchMethod }, + fetchOpts, + ); + const url = _resolveFetchUrl(); + const resp = await fetch(url, init); + if (!resp.ok) { + throw new Error(`HTTP ${resp.status}`); + } + data = await resp.json(); } - const data = await resp.json(); state.lastData = data; if (!_isSuccess(data)) { - // Treat success=false as empty rather than error so - // the user sees the "nothing here" copy. Endpoints - // returning success=false with a network/auth reason - // can opt into error treatment via isSuccess. _showEmpty(); return; } + if (typeof config.onSuccess === 'function') { + try { + config.onSuccess(data, _ctx({ data })); + } catch (err) { + console.debug(`[discover:${config.id}] onSuccess hook threw:`, err); + } + } + const items = _extractItems(data); + + // Stale wins over empty — section is empty *now* but + // upstream is still discovering, so show updating UI + // rather than the bare "nothing here" copy. + if (_isStale(items, data)) { + _showStale(items, data); + return; + } + if (_isEmpty(items, data)) { _showEmpty(); return; } _showSection(); - const html = config.renderItems(items, data, { contentEl, config }); - _setHtml(contentEl, html || ''); + 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); + } state.phase = 'rendered'; if (typeof config.onRendered === 'function') { try { - config.onRendered({ contentEl, items, data, config }); + config.onRendered(_ctx({ items, data })); } catch (hookErr) { - // Don't let a renderer hook error rip down the - // controller — log + continue. console.debug(`[discover:${config.id}] onRendered hook threw:`, hookErr); } } @@ -292,8 +410,6 @@ } async function refresh() { - // Clear in-flight first so refresh() always re-fires the - // network call (load() coalesces, refresh() bypasses). state.inFlight = null; return load(); } @@ -306,8 +422,6 @@ } function getState() { - // Expose a copy so callers can inspect without holding onto - // mutable internal state. return { phase: state.phase, hasData: state.lastData !== null, @@ -318,7 +432,5 @@ return { load, refresh, destroy, getState }; } - // Expose globally — the discover page is one big shared script - // surface, no module system in play. window.createDiscoverSectionController = createDiscoverSectionController; })(); diff --git a/webui/static/discover.js b/webui/static/discover.js index 20bbd8c5..d89b5927 100644 --- a/webui/static/discover.js +++ b/webui/static/discover.js @@ -885,6 +885,7 @@ async function loadDiscoverRecentReleases() { loadingMessage: 'Loading recent releases...', emptyMessage: 'No recent releases found', errorMessage: 'Failed to load recent releases', + showErrorToast: true, }); } return _recentReleasesCtrl.load(); @@ -909,46 +910,64 @@ function debouncedYourAlbumsSearch() { }, 400); } +let _yourAlbumsCtrl = null; + async function loadYourAlbums() { - const section = document.getElementById('your-albums-section'); - if (!section) return; - try { - const resp = await fetch('/api/discover/your-albums?page=1&per_page=48&status=all'); - if (!resp.ok) return; - const data = await resp.json(); - if (!data.success) return; - - const totalCount = (data.stats && data.stats.total) || 0; - if (totalCount === 0 && !data.stale) return; // Nothing to show yet - - section.style.display = ''; - yourAlbums = data.albums || []; - yourAlbumsTotal = data.total || 0; - yourAlbumsPage = 1; - - const subtitle = document.getElementById('your-albums-subtitle'); - if (subtitle && data.stats) { - const s = data.stats; - subtitle.textContent = `${s.total} albums \u00B7 ${s.owned} owned \u00B7 ${s.missing} missing`; - } - - const filters = document.getElementById('your-albums-filters'); - if (filters && totalCount > 0) filters.style.display = ''; - - const downloadBtn = document.getElementById('your-albums-download-btn'); - if (downloadBtn && data.stats && data.stats.missing > 0) downloadBtn.style.display = ''; - - _renderYourAlbumsGrid(yourAlbums); - _renderYourAlbumsPagination(yourAlbumsTotal, yourAlbumsPage); - - if (data.stale && totalCount === 0) { - const grid = document.getElementById('your-albums-grid'); - if (grid) grid.innerHTML = '

Fetching your albums from connected services...

'; - _pollYourAlbums(); - } - } catch (e) { - console.error('Error loading your albums:', e); + if (!_yourAlbumsCtrl) { + _yourAlbumsCtrl = createDiscoverSectionController({ + id: 'your-albums', + sectionEl: '#your-albums-section', + contentEl: '#your-albums-grid', + fetchUrl: '/api/discover/your-albums?page=1&per_page=48&status=all', + extractItems: (data) => data.albums || [], + // Truly empty (no data + not stale) \u2192 hide the whole section + // (matches the legacy "Nothing to show yet" early-return). The + // outer hideWhenEmpty + sectionEl handle the visibility flip. + isEmpty: (items, data) => { + const total = (data && data.stats && data.stats.total) || 0; + return total === 0 && !data.stale; + }, + hideWhenEmpty: true, + // Stale + no albums yet \u2192 show the "fetching from connected + // services" UI and start the poller. Fires before isEmpty. + isStale: (items, data) => { + const total = (data && data.stats && data.stats.total) || 0; + return Boolean(data && data.stale) && total === 0; + }, + renderStale: () => + '

Fetching your albums from connected services...

', + onStale: () => _pollYourAlbums(), + // Side-effects against sibling DOM (subtitle / filters / + // download button) belong here, not in renderItems. + onSuccess: (data) => { + const subtitle = document.getElementById('your-albums-subtitle'); + if (subtitle && data.stats) { + const s = data.stats; + subtitle.textContent = `${s.total} albums \u00B7 ${s.owned} owned \u00B7 ${s.missing} missing`; + } + const totalCount = (data.stats && data.stats.total) || 0; + const filters = document.getElementById('your-albums-filters'); + if (filters && totalCount > 0) filters.style.display = ''; + const downloadBtn = document.getElementById('your-albums-download-btn'); + 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. + 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, + showErrorToast: true, + }); } + return _yourAlbumsCtrl.load(); } function _pollYourAlbums() { @@ -1368,6 +1387,7 @@ async function loadDiscoverReleaseRadar() { emptyMessage: 'No new releases available', errorMessage: 'Failed to load release radar', verboseErrors: true, + showErrorToast: true, }); } return _releaseRadarCtrl.load(); @@ -1391,6 +1411,7 @@ async function loadDiscoverWeekly() { emptyMessage: 'No tracks available yet', errorMessage: 'Failed to load discovery weekly', verboseErrors: true, + showErrorToast: true, }); } return _weeklyCtrl.load(); @@ -1434,6 +1455,7 @@ async function loadDecadeBrowser() { emptyMessage: 'No decade content available yet. Run a watchlist scan to populate your discovery pool!', errorMessage: 'Failed to load decades', verboseErrors: true, + showErrorToast: true, }); } return _decadeBrowserCtrl.load(); @@ -1525,6 +1547,7 @@ async function loadGenreBrowser() { emptyMessage: 'No genre content available yet. Run a watchlist scan to populate your discovery pool!', errorMessage: 'Failed to load genres', verboseErrors: true, + showErrorToast: true, }); } return _genreBrowserCtrl.load(); @@ -1654,6 +1677,31 @@ async function openGenrePlaylist(genre) { let decadeTracksCache = {}; // Store tracks for each decade let activeDecade = null; +// Shared sync-status display block. Used by per-tab playlists +// (decade browser, genre browser) where we show download progress +// in the standard "✓ completed | ⏳ pending | ✗ failed (N%)" format. +// ListenBrainz playlists use a different shape (total/matched/failed) +// because they show MATCHING progress against the library, not +// download progress, so they intentionally don't use this helper. +function _renderSyncStatusBlock(idPrefix) { + return ` + + `; +} + async function loadDecadeBrowserTabs() { try { const tabsContainer = document.getElementById('decade-tabs'); @@ -1713,21 +1761,7 @@ async function loadDecadeBrowserTabs() { - - + ${_renderSyncStatusBlock(tabId)} @@ -2110,21 +2144,7 @@ async function loadGenreBrowserTabs() { - - + ${_renderSyncStatusBlock(tabId)} @@ -3533,59 +3553,52 @@ async function loadSeasonalContent() { } } +function _renderSeasonalAlbumCard(album, index) { + const coverUrl = album.album_cover_url || '/static/placeholder-album.png'; + return ` +
+
+ ${album.album_name} +
+
+

${album.album_name}

+

${album.artist_name}

+ ${album.release_date ? `

${album.release_date}

` : ''} +
+
+ `; +} + +// Seasonal Albums uses no-fetch `data:` mode — the parent +// `loadSeasonalContent` already fetched the season payload and passes +// the album list directly. Controller is recreated per call so the +// per-season `data` snapshot is current. async function loadSeasonalAlbums(seasonData) { - try { - const carousel = document.getElementById('seasonal-albums-carousel'); - if (!carousel) return; - - // Show seasonal section - const seasonalSection = document.getElementById('seasonal-albums-section'); - if (seasonalSection) { - seasonalSection.style.display = 'block'; - } - - // Update header - const seasonalTitle = document.getElementById('seasonal-albums-title'); - const seasonalSubtitle = document.getElementById('seasonal-albums-subtitle'); - - if (seasonalTitle) { - seasonalTitle.textContent = `${seasonData.icon} ${seasonData.name}`; - } - if (seasonalSubtitle) { - seasonalSubtitle.textContent = seasonData.description; - } - - // Store albums for download functionality - discoverSeasonalAlbums = seasonData.albums || []; - - if (discoverSeasonalAlbums.length === 0) { - carousel.innerHTML = '

No seasonal albums found

'; - return; - } - - // Build carousel HTML - let html = ''; - discoverSeasonalAlbums.forEach((album, index) => { - const coverUrl = album.album_cover_url || '/static/placeholder-album.png'; - html += ` -
-
- ${album.album_name} -
-
-

${album.album_name}

-

${album.artist_name}

- ${album.release_date ? `

${album.release_date}

` : ''} -
-
- `; - }); - - carousel.innerHTML = html; - - } catch (error) { - console.error('Error loading seasonal albums:', error); - } + const albums = (seasonData && seasonData.albums) || []; + const ctrl = createDiscoverSectionController({ + id: 'seasonal-albums', + sectionEl: '#seasonal-albums-section', + contentEl: '#seasonal-albums-carousel', + data: { success: true, albums }, + extractItems: (data) => data.albums, + beforeLoad: () => { + const section = document.getElementById('seasonal-albums-section'); + if (section) section.style.display = 'block'; + const titleEl = document.getElementById('seasonal-albums-title'); + const subtitleEl = document.getElementById('seasonal-albums-subtitle'); + if (titleEl && seasonData) titleEl.textContent = `${seasonData.icon} ${seasonData.name}`; + if (subtitleEl && seasonData) subtitleEl.textContent = seasonData.description; + }, + renderItems: (items) => { + discoverSeasonalAlbums = items; + return items.map((album, i) => _renderSeasonalAlbumCard(album, i)).join(''); + }, + emptyMessage: 'No seasonal albums found', + errorMessage: 'Failed to load seasonal albums', + verboseErrors: true, + showErrorToast: true, + }); + return ctrl.load(); } let _seasonalPlaylistCtrl = null; @@ -3629,6 +3642,7 @@ async function loadSeasonalPlaylist(seasonData) { emptyMessage: 'No tracks available yet', errorMessage: 'Failed to load playlist', verboseErrors: true, + showErrorToast: true, }); _seasonalPlaylistCtrlKey = currentSeasonKey; } @@ -3983,10 +3997,11 @@ async function blockDiscoveryArtist(artistName) { const data = await res.json(); if (data.success) { showToast(`Blocked ${artistName} from discovery`, 'success'); - // Refresh all discovery sections to remove the artist + // Refresh discovery sections to remove the artist. Daily Mixes + // is intentionally paused (see loadDiscoverPage), so don't + // refresh it — the section isn't on the page anyway. loadPersonalizedHiddenGems(); loadDiscoveryShuffle(); - loadPersonalizedDailyMixes(); } else { showToast(data.error || 'Failed to block artist', 'error'); } @@ -4181,6 +4196,7 @@ async function loadYourArtists() { emptyMessage: 'No followed artists found', errorMessage: 'Failed to load your artists', verboseErrors: true, + showErrorToast: true, }); } return _yourArtistsCtrl.load(); diff --git a/webui/static/helper.js b/webui/static/helper.js index 6d9e9869..2324a496 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 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' }, ],