From 56eff933d4aef3c7a10a8b004fd15381f1c6ebe5 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 18 May 2026 21:56:59 -0700 Subject: [PATCH 01/24] Delete pr_description.md --- pr_description.md | 57 ----------------------------------------------- 1 file changed, 57 deletions(-) delete mode 100644 pr_description.md diff --git a/pr_description.md b/pr_description.md deleted file mode 100644 index 510794f5..00000000 --- a/pr_description.md +++ /dev/null @@ -1,57 +0,0 @@ -## Summary - -Adds source-aware artist detail deep links so artist pages can be opened directly as `/artist-detail/:source/:id`, including metadata-source artists from Spotify, Deezer, iTunes, Discogs, Amazon, Hydrabase, and existing library artists. - -This also fixes Discover/download modal artist links that were falling back to `/artist-detail/library/` or using the wrong album/card ID as an artist ID. - -## What Changed - -- Added canonical artist detail routes in the SPA: - - `/artist-detail/library/` - - `/artist-detail/spotify/` - - `/artist-detail/deezer/` - - `/artist-detail/itunes/` - - plus other supported metadata sources. -- Preserved legacy `/artist-detail/` behavior as a library fallback. -- Updated shell routing and deep-link activation so refresh/direct navigation works for nested artist-detail URLs. -- Updated artist detail navigation to carry `artistSource` through the SPA instead of relying only on artist name/id. -- Improved source-only artist detail loading so provider-fetched artist names are used when the URL only contains the source ID. -- Prevented source-only artist pages from running library-only ownership/enhancement checks. -- Treats unknown ownership on source-only discographies as missing/clickable instead of leaving cards stuck on "still checking ownership." -- Uses release artwork as a generic artist-detail hero fallback when an artist portrait is missing or fails to load. -- Preserved source/artist IDs from Discover album modals, seasonal albums, cached discovery albums, recent releases, and download modal hero links. -- Prevented modal artist links from falling back to fake library routes when a real source artist ID is unavailable. -- Returned seasonal album `source` from cached seasonal album rows so seasonal modal links retain their provider context. - -## Behavior - -- Clicking a Spotify artist result can now land on: - - `/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg` -- Clicking a Deezer artist result can now land on: - - `/artist-detail/deezer/525046` -- Existing library artist links continue to resolve through the library path. -- If a source artist resolves to an existing library artist, the page upgrades to the library-backed artist and keeps library-only tools/checks available. -- If a source artist is not in the library, the page shows source discography as missing/clickable and skips library-only endpoints. -- If a modal lacks a trustworthy source artist ID, it shows a warning instead of navigating to an invalid library artist URL. - -## Tests - -Frontend route tests: - -```bash -cd webui -npm.cmd test -- --run src/platform/shell/route-manifest.test.ts src/platform/shell/bridge.test.ts -``` - -Result: - -```text -2 test files passed -10 tests passed -``` - -Recommended backend verification: - -```bash -./.venv/bin/python -m pytest tests/test_spa_deep_linking.py tests/metadata/test_artist_source_detail.py -``` From 728481db31034ecb921bf5455439201c52a628b6 Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Tue, 19 May 2026 08:13:13 +0300 Subject: [PATCH 02/24] refactor(webui): route artist-detail handoff - add canonical /artist-detail/:source/:id TanStack route - hand the legacy page off through the shell bridge - remove artist-detail branching from generic shell helpers --- webui/src/app/router.test.tsx | 1 + webui/src/platform/shell/bridge.test.ts | 9 +++ webui/src/platform/shell/bridge.ts | 3 + webui/src/platform/shell/globals.d.ts | 9 +++ .../src/platform/shell/route-manifest.test.ts | 3 +- webui/src/platform/shell/route-manifest.ts | 10 ++- webui/src/routeTree.gen.ts | 24 +++++- .../src/routes/artist-detail/$source/$id.tsx | 27 +++++++ .../src/routes/artist-detail/-route.test.tsx | 76 +++++++++++++++++++ webui/src/routes/issues/-route.test.tsx | 1 + webui/static/init.js | 43 +++-------- webui/static/library.js | 2 +- webui/static/search.js | 11 +-- webui/static/shell-bridge.js | 21 +---- 14 files changed, 172 insertions(+), 68 deletions(-) create mode 100644 webui/src/routes/artist-detail/$source/$id.tsx create mode 100644 webui/src/routes/artist-detail/-route.test.tsx diff --git a/webui/src/app/router.test.tsx b/webui/src/app/router.test.tsx index f347a5be..7f1921f9 100644 --- a/webui/src/app/router.test.tsx +++ b/webui/src/app/router.test.tsx @@ -60,6 +60,7 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { resolveLegacyPath: vi.fn<(pathname: string) => ShellPageId | null>(() => 'search'), setActivePageChrome: vi.fn(), activateLegacyPath: vi.fn(), + navigateToArtistDetail: vi.fn(), showReactHost: vi.fn(), ...overrides, }; diff --git a/webui/src/platform/shell/bridge.test.ts b/webui/src/platform/shell/bridge.test.ts index 00dd88d4..a33f44cd 100644 --- a/webui/src/platform/shell/bridge.test.ts +++ b/webui/src/platform/shell/bridge.test.ts @@ -88,4 +88,13 @@ describe('bindWindowWebRouter', () => { replace: true, }); }); + + it('refuses artist detail navigation without an artist id', async () => { + const navigate = vi.fn().mockResolvedValue(undefined); + + bindWindowWebRouter({ navigate } as never); + + await expect(window.SoulSyncWebRouter?.navigateToPage('artist-detail', {} as never)).resolves.toBe(false); + expect(navigate).not.toHaveBeenCalled(); + }); }); diff --git a/webui/src/platform/shell/bridge.ts b/webui/src/platform/shell/bridge.ts index 1ca85768..1c9683f5 100644 --- a/webui/src/platform/shell/bridge.ts +++ b/webui/src/platform/shell/bridge.ts @@ -88,6 +88,9 @@ export function bindWindowWebRouter(router: AnyRouter) { async navigateToPage(pageId, options) { const route = getShellRouteByPageId(pageId); if (!route) return false; + if (pageId === 'artist-detail' && !options?.artistId) { + return false; + } let href: `/${string}` = route.path; if (pageId === 'artist-detail' && options?.artistId) { diff --git a/webui/src/platform/shell/globals.d.ts b/webui/src/platform/shell/globals.d.ts index 27de813c..241464d3 100644 --- a/webui/src/platform/shell/globals.d.ts +++ b/webui/src/platform/shell/globals.d.ts @@ -35,6 +35,15 @@ declare global { resolveLegacyPath: (pathname: string) => ShellPageId | null; setActivePageChrome: (pageId: ShellPageId) => void; activateLegacyPath: (pathname: string) => void; + navigateToArtistDetail: ( + artistId: string | number, + artistName: string, + sourceOverride?: string | null, + options?: { + skipOriginPush?: boolean; + skipRouteChange?: boolean; + }, + ) => void; showReactHost: (pageId: ShellPageId) => void; }; } diff --git a/webui/src/platform/shell/route-manifest.test.ts b/webui/src/platform/shell/route-manifest.test.ts index 4430dd11..369f7b3d 100644 --- a/webui/src/platform/shell/route-manifest.test.ts +++ b/webui/src/platform/shell/route-manifest.test.ts @@ -16,7 +16,7 @@ describe('shellRouteManifest', () => { expect(resolveShellPageFromPath('/discover')).toBe('discover'); expect(resolveShellPageFromPath('/watchlist')).toBe('watchlist'); expect(resolveShellPageFromPath('/active-downloads')).toBe('active-downloads'); - expect(resolveShellPageFromPath('/artist-detail')).toBe('artist-detail'); + expect(resolveShellPageFromPath('/artist-detail')).toBeNull(); expect(resolveShellPageFromPath('/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg')).toBe('artist-detail'); expect(resolveShellPageFromPath('/artists')).toBeNull(); }); @@ -50,6 +50,7 @@ describe('shellRouteManifest', () => { expect(resolveLegacyShellPageFromPath('/search')).toBe('search'); expect(resolveLegacyShellPageFromPath('/active-downloads')).toBe('active-downloads'); expect(resolveLegacyShellPageFromPath('/tools')).toBe('tools'); + expect(resolveLegacyShellPageFromPath('/artist-detail')).toBeNull(); expect(resolveLegacyShellPageFromPath('/artist-detail/deezer/12345')).toBe('artist-detail'); expect(resolveLegacyShellPageFromPath('/issues')).toBeNull(); expect(resolveLegacyShellPageFromPath('/does-not-exist')).toBeNull(); diff --git a/webui/src/platform/shell/route-manifest.ts b/webui/src/platform/shell/route-manifest.ts index 6b1a209e..469d46d1 100644 --- a/webui/src/platform/shell/route-manifest.ts +++ b/webui/src/platform/shell/route-manifest.ts @@ -72,7 +72,10 @@ export function getShellRouteByPath(pathname: string): ShellRouteDefinition | un export function resolveShellPageFromPath(pathname: string): ShellPageId | null { const normalized = normalizeShellPath(pathname); - if (normalized === '/artist-detail' || normalized.startsWith('/artist-detail/')) { + if (normalized === '/artist-detail') { + return null; + } + if (normalized.startsWith('/artist-detail/')) { return 'artist-detail'; } return getShellRouteByPath(pathname)?.pageId ?? null; @@ -80,7 +83,10 @@ export function resolveShellPageFromPath(pathname: string): ShellPageId | null { export function resolveLegacyShellPageFromPath(pathname: string): ShellPageId | null { const normalized = normalizeShellPath(pathname); - if (normalized === '/artist-detail' || normalized.startsWith('/artist-detail/')) { + if (normalized === '/artist-detail') { + return null; + } + if (normalized.startsWith('/artist-detail/')) { return 'artist-detail'; } const route = getShellRouteByPath(pathname); diff --git a/webui/src/routeTree.gen.ts b/webui/src/routeTree.gen.ts index d90bb0ce..ca94e35b 100644 --- a/webui/src/routeTree.gen.ts +++ b/webui/src/routeTree.gen.ts @@ -12,6 +12,7 @@ import { Route as rootRouteImport } from './routes/__root' import { Route as SplatRouteImport } from './routes/$' import { Route as IssuesRouteRouteImport } from './routes/issues/route' import { Route as IndexRouteImport } from './routes/index' +import { Route as ArtistDetailSourceIdRouteImport } from './routes/artist-detail/$source/$id' const SplatRoute = SplatRouteImport.update({ id: '/$', @@ -28,35 +29,44 @@ const IndexRoute = IndexRouteImport.update({ path: '/', getParentRoute: () => rootRouteImport, } as any) +const ArtistDetailSourceIdRoute = ArtistDetailSourceIdRouteImport.update({ + id: '/artist-detail/$source/$id', + path: '/artist-detail/$source/$id', + getParentRoute: () => rootRouteImport, +} as any) export interface FileRoutesByFullPath { '/': typeof IndexRoute '/issues': typeof IssuesRouteRoute '/$': typeof SplatRoute + '/artist-detail/$source/$id': typeof ArtistDetailSourceIdRoute } export interface FileRoutesByTo { '/': typeof IndexRoute '/issues': typeof IssuesRouteRoute '/$': typeof SplatRoute + '/artist-detail/$source/$id': typeof ArtistDetailSourceIdRoute } export interface FileRoutesById { __root__: typeof rootRouteImport '/': typeof IndexRoute '/issues': typeof IssuesRouteRoute '/$': typeof SplatRoute + '/artist-detail/$source/$id': typeof ArtistDetailSourceIdRoute } export interface FileRouteTypes { fileRoutesByFullPath: FileRoutesByFullPath - fullPaths: '/' | '/issues' | '/$' + fullPaths: '/' | '/issues' | '/$' | '/artist-detail/$source/$id' fileRoutesByTo: FileRoutesByTo - to: '/' | '/issues' | '/$' - id: '__root__' | '/' | '/issues' | '/$' + to: '/' | '/issues' | '/$' | '/artist-detail/$source/$id' + id: '__root__' | '/' | '/issues' | '/$' | '/artist-detail/$source/$id' fileRoutesById: FileRoutesById } export interface RootRouteChildren { IndexRoute: typeof IndexRoute IssuesRouteRoute: typeof IssuesRouteRoute SplatRoute: typeof SplatRoute + ArtistDetailSourceIdRoute: typeof ArtistDetailSourceIdRoute } declare module '@tanstack/react-router' { @@ -82,6 +92,13 @@ declare module '@tanstack/react-router' { preLoaderRoute: typeof IndexRouteImport parentRoute: typeof rootRouteImport } + '/artist-detail/$source/$id': { + id: '/artist-detail/$source/$id' + path: '/artist-detail/$source/$id' + fullPath: '/artist-detail/$source/$id' + preLoaderRoute: typeof ArtistDetailSourceIdRouteImport + parentRoute: typeof rootRouteImport + } } } @@ -89,6 +106,7 @@ const rootRouteChildren: RootRouteChildren = { IndexRoute: IndexRoute, IssuesRouteRoute: IssuesRouteRoute, SplatRoute: SplatRoute, + ArtistDetailSourceIdRoute: ArtistDetailSourceIdRoute, } export const routeTree = rootRouteImport ._addFileChildren(rootRouteChildren) diff --git a/webui/src/routes/artist-detail/$source/$id.tsx b/webui/src/routes/artist-detail/$source/$id.tsx new file mode 100644 index 00000000..8ae2f2c9 --- /dev/null +++ b/webui/src/routes/artist-detail/$source/$id.tsx @@ -0,0 +1,27 @@ +import { createFileRoute } from '@tanstack/react-router'; +import { useLayoutEffect } from 'react'; + +import { useShellBridge } from '@/platform/shell/route-controllers'; + +export const Route = createFileRoute('/artist-detail/$source/$id')({ + component: ArtistDetailPage, +}); + +// Thin legacy handoff: TanStack owns the URL shape here, but the vanilla JS +// artist-detail page still renders the actual experience for now. +function ArtistDetailPage() { + const bridge = useShellBridge(); + const { source, id } = Route.useParams(); + + useLayoutEffect(() => { + if (!bridge) return; + + const normalizedSource = source.toLowerCase() === 'library' ? null : source.toLowerCase(); + bridge.navigateToArtistDetail(id, '', normalizedSource, { + skipOriginPush: true, + skipRouteChange: true, + }); + }, [bridge, id, source]); + + return null; +} diff --git a/webui/src/routes/artist-detail/-route.test.tsx b/webui/src/routes/artist-detail/-route.test.tsx new file mode 100644 index 00000000..0f96ac5c --- /dev/null +++ b/webui/src/routes/artist-detail/-route.test.tsx @@ -0,0 +1,76 @@ +import { createMemoryHistory } from '@tanstack/react-router'; +import { render, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { ShellBridge, ShellPageId } from '@/platform/shell/bridge'; + +import { createAppQueryClient } from '@/app/query-client'; +import { AppRouterProvider, createAppRouter } from '@/app/router'; + +function createShellBridge(overrides: Partial = {}): ShellBridge { + return { + getCurrentProfileContext: vi.fn(() => ({ profileId: 2, isAdmin: false })), + isPageAllowed: vi.fn(() => true), + getProfileHomePage: vi.fn<() => ShellPageId>(() => 'discover'), + resolveLegacyPath: vi.fn<(pathname: string) => ShellPageId | null>(() => 'artist-detail'), + setActivePageChrome: vi.fn(), + activateLegacyPath: vi.fn(), + navigateToArtistDetail: vi.fn(), + showReactHost: vi.fn(), + ...overrides, + }; +} + +function renderArtistDetailRoute(initialEntries = ['/artist-detail/library/42']) { + const queryClient = createAppQueryClient(); + const history = createMemoryHistory({ initialEntries }); + const router = createAppRouter({ history, queryClient }); + + return { + history, + router, + ...render(), + }; +} + +describe('artist-detail route', () => { + beforeEach(() => { + window.SoulSyncWebShellBridge = createShellBridge(); + }); + + afterEach(() => { + window.SoulSyncWebShellBridge = undefined; + }); + + it('hands off canonical artist-detail URLs to the legacy shell', async () => { + renderArtistDetailRoute(['/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg']); + + await waitFor(() => { + expect(window.SoulSyncWebShellBridge?.navigateToArtistDetail).toHaveBeenCalledWith( + '2YZyLoL8N0Wb9xBt1NhZWg', + '', + 'spotify', + { + skipOriginPush: true, + skipRouteChange: true, + }, + ); + }); + }); + + it('normalizes library sources before handing off', async () => { + renderArtistDetailRoute(['/artist-detail/library/42']); + + await waitFor(() => { + expect(window.SoulSyncWebShellBridge?.navigateToArtistDetail).toHaveBeenCalledWith( + '42', + '', + null, + { + skipOriginPush: true, + skipRouteChange: true, + }, + ); + }); + }); +}); diff --git a/webui/src/routes/issues/-route.test.tsx b/webui/src/routes/issues/-route.test.tsx index 402966e1..547745ef 100644 --- a/webui/src/routes/issues/-route.test.tsx +++ b/webui/src/routes/issues/-route.test.tsx @@ -22,6 +22,7 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { resolveLegacyPath: vi.fn<(pathname: string) => ShellPageId | null>(() => 'search'), setActivePageChrome: vi.fn(), activateLegacyPath: vi.fn(), + navigateToArtistDetail: vi.fn(), showReactHost: vi.fn(), ...overrides, }; diff --git a/webui/static/init.js b/webui/static/init.js index c874f6fc..f00e5852 100644 --- a/webui/static/init.js +++ b/webui/static/init.js @@ -2154,10 +2154,6 @@ function _getPageFromPath() { const basePage = segs[0]; if (!_DEEPLINK_VALID_PAGES.has(basePage)) return 'dashboard'; // Context-dependent pages fall back to a sensible parent - if (basePage === 'artist-detail') { - // /artist-detail/:id deep-link β€” keep on artist-detail; bare /artist-detail falls back to library - return (segs.length >= 2 && segs[1]) ? 'artist-detail' : 'library'; - } if (basePage === 'playlist-explorer') return 'library'; return basePage; } @@ -2168,35 +2164,13 @@ function _normalizeArtistDetailSource(source) { } function buildArtistDetailPath(artistId, source = null) { - if (!artistId) return '/artist-detail'; + if (!artistId) { + throw new Error('artistId is required for artist-detail navigation'); + } const normalizedSource = _normalizeArtistDetailSource(source); return '/artist-detail/' + encodeURIComponent(normalizedSource) + '/' + encodeURIComponent(String(artistId)); } -/** Extract artist source + ID from /artist-detail/:source/:id or legacy /artist-detail/:id URLs. */ -function _getDeepLinkArtistDetail(pathname = window.location.pathname) { - const path = (pathname || '').replace(/^\/+|\/+$/g, ''); - const segs = path.split('/'); - if (segs[0] !== 'artist-detail' || !segs[1]) return null; - - if (segs[2]) { - return { - source: _normalizeArtistDetailSource(decodeURIComponent(segs[1])), - artistId: decodeURIComponent(segs.slice(2).join('/')), - }; - } - - return { - source: 'library', - artistId: decodeURIComponent(segs[1]), - }; -} - -/** Legacy convenience wrapper for callers that only need the artist ID. */ -function _getDeepLinkArtistId() { - return _getDeepLinkArtistDetail()?.artistId || null; -} - // =============================== // MOBILE NAVIGATION // =============================== @@ -2310,6 +2284,10 @@ function navigateToPage(pageId, options = {}) { return; } + if (pageId === 'artist-detail' && !options.artistId) { + return false; + } + const router = getWebRouter(); if (router && !options.skipRouteChange) { notifyPageWillChange(pageId); @@ -2386,7 +2364,10 @@ async function loadPageData(pageId) { case 'library': // Check if we should return to artist detail view instead of list if (artistDetailPageState.currentArtistId && artistDetailPageState.currentArtistName) { - navigateToPage('artist-detail'); + navigateToPage('artist-detail', { + artistId: artistDetailPageState.currentArtistId, + artistSource: artistDetailPageState.currentArtistSource, + }); if (!artistDetailPageState.isInitialized) { initializeArtistDetailPage(); loadArtistDetailData(artistDetailPageState.currentArtistId, artistDetailPageState.currentArtistName); @@ -2400,7 +2381,7 @@ async function loadPageData(pageId) { } break; case 'artist-detail': - // Artist detail page is handled separately by navigateToArtistDetail() + // Artist detail page is entered through the route handoff and legacy navigator. break; case 'discover': if (!discoverPageInitialized) { diff --git a/webui/static/library.js b/webui/static/library.js index 19818b16..dfdc5fac 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -815,7 +815,7 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt navigateToPage('artist-detail', { artistId, artistSource: normalizedSource, - skipRouteChange: true + skipRouteChange: options.skipRouteChange === true }); _updateArtistDetailBackButtonLabel(); } diff --git a/webui/static/search.js b/webui/static/search.js index fd6d2604..682d48f0 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -1197,16 +1197,7 @@ async function loadInitialData() { return; } - if (targetPage === 'artist-detail') { - const deepArtist = _getDeepLinkArtistDetail(); - if (deepArtist?.artistId && typeof navigateToArtistDetail === 'function') { - navigateToArtistDetail(deepArtist.artistId, '', deepArtist.source === 'library' ? null : deepArtist.source); - } else { - navigateToPage('library', { skipRouteChange: true, forceReload: true }); - } - } else { - navigateToPage(targetPage, { skipRouteChange: true, forceReload: true }); - } + navigateToPage(targetPage, { forceReload: true }); } catch (error) { console.error('Error loading initial data:', error); } diff --git a/webui/static/shell-bridge.js b/webui/static/shell-bridge.js index 8d2ac58a..64c73f6e 100644 --- a/webui/static/shell-bridge.js +++ b/webui/static/shell-bridge.js @@ -80,16 +80,6 @@ function activateLegacyPath(pathname) { return; } - if (targetPage === 'artist-detail') { - const deepArtist = _getDeepLinkArtistDetail(pathname); - if (deepArtist?.artistId && typeof navigateToArtistDetail === 'function') { - navigateToArtistDetail(deepArtist.artistId, '', deepArtist.source === 'library' ? null : deepArtist.source, { skipOriginPush: true, skipRouteChange: true }); - return; - } - navigateToPage('library', { replace: true }); - return; - } - notifyPageWillChange(targetPage); activatePage(targetPage, { forceReload: true }); } @@ -107,16 +97,6 @@ function syncActivePageFromLocation() { return; } - if (targetPage === 'artist-detail') { - const deepArtist = _getDeepLinkArtistDetail(); - if (deepArtist?.artistId && typeof navigateToArtistDetail === 'function') { - navigateToArtistDetail(deepArtist.artistId, '', deepArtist.source === 'library' ? null : deepArtist.source, { skipOriginPush: true, skipRouteChange: true }); - return; - } - navigateToPage('library', { replace: true }); - return; - } - notifyPageWillChange(targetPage); const route = router?.routeManifest?.find((entry) => entry.pageId === targetPage); if (route?.kind === 'react') { @@ -185,6 +165,7 @@ window.SoulSyncWebShellBridge = { activateLegacyPath(pathname) { activateLegacyPath(pathname); }, + navigateToArtistDetail, showReactHost(pageId) { showReactHost(pageId); }, From 5e39f1ee09375667358fee50f6ebef4954bce034 Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Tue, 19 May 2026 09:26:10 +0300 Subject: [PATCH 03/24] refactor(webui): centralize artist-detail handoff - add a canonical TanStack route for artist-detail and keep the legacy page as the renderer target - expose page-level artist-detail navigation on the shell bridge for legacy callers - remove artist-detail-specific routing, origin stack, and back-label logic from the shared shell helpers --- webui/index.html | 2 +- webui/src/platform/shell/globals.d.ts | 9 +- .../src/routes/artist-detail/$source/$id.tsx | 1 - .../src/routes/artist-detail/-route.test.tsx | 2 - webui/static/api-monitor.js | 2 +- webui/static/discover.js | 13 +- webui/static/downloads.js | 5 +- webui/static/library.js | 123 ++++-------------- webui/static/media-player.js | 3 +- webui/static/search.js | 4 +- webui/static/shared-helpers.js | 2 +- webui/static/shell-bridge.js | 1 + webui/static/stats-automations.js | 8 +- 13 files changed, 52 insertions(+), 123 deletions(-) diff --git a/webui/index.html b/webui/index.html index c0c86dd8..5ddbcb2a 100644 --- a/webui/index.html +++ b/webui/index.html @@ -2418,7 +2418,7 @@
- @@ -6595,7 +6595,7 @@ function _artMapSetupInteraction(canvas) {
ⓘ Artist Info
-
+
💿 View Discography
@@ -7275,7 +7275,7 @@ async function openGenreDeepDive(genre) { // Always open on Artists page with discography β€” pass source for correct routing const imgUrl = _esc(a.image_url || ''); const artSource = _esc(a.source || ''); - const clickAction = `onclick="document.getElementById('genre-deep-dive-modal').remove();navigateToArtistDetail('${_esc(a.entity_id)}','${_esc(a.name)}','${artSource}' || null)"`; + const clickAction = `onclick="document.getElementById('genre-deep-dive-modal').remove();navigateToArtistDetailPage('${_esc(a.entity_id)}','${_esc(a.name)}','${artSource}' || null)"`; const srcClass = (a.source || '').toLowerCase(); return `
@@ -8768,4 +8768,3 @@ if (document.readyState === 'loading') { } // ============================================================================ - diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 7374753d..5e5cad38 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -672,7 +672,7 @@ function _navigateToArtistFromModal(artistId, artistName, imageUrl, source, play showToast(`Artist details are not available for ${artistName}`, 'warning'); return; } - navigateToArtistDetail(resolvedArtistId, artistName, resolvedSource); + navigateToArtistDetailPage(resolvedArtistId, artistName, resolvedSource); } async function closeDownloadMissingModal(playlistId) { @@ -5782,7 +5782,7 @@ function _gsClickArtist(id, name, isLibrary) { _gsDeactivate(); const activeSource = _gsController && _gsController.state.activeSource; const source = isLibrary ? null : (activeSource || null); - navigateToArtistDetail(id, name, source); + navigateToArtistDetailPage(id, name, source); } async function _gsClickAlbum(albumId, albumName, artistName, imageUrl, source) { @@ -6400,4 +6400,3 @@ const additionalStyles = ` document.head.insertAdjacentHTML('beforeend', additionalStyles); // ============================================================================ - diff --git a/webui/static/library.js b/webui/static/library.js index dfdc5fac..f96a2bee 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -235,7 +235,7 @@ function displayLibraryArtists(artists) { } const card = e.target.closest('.library-artist-card'); if (card) { - navigateToArtistDetail(card.dataset.artistId, card.dataset.artistName); + navigateToArtistDetailPage(card.dataset.artistId, card.dataset.artistName); } }; } @@ -688,11 +688,6 @@ let artistDetailPageState = { currentArtistId: null, currentArtistName: null, currentArtistSource: null, - // Stack of origins captured by navigateToArtistDetail for the back button. - // Each entry is either {type:'page', pageId} or {type:'artist', id, name, source} - // so chained navigation (Search β†’ A β†’ similar B β†’ similar C) walks back one - // step at a time instead of jumping straight to Search. - originStack: [], enhancedView: false, enhancedData: null, expandedAlbums: new Set(), @@ -710,7 +705,6 @@ function clearArtistDetailPageState() { artistDetailPageState.currentArtistId = null; artistDetailPageState.currentArtistName = null; artistDetailPageState.currentArtistSource = null; - artistDetailPageState.originStack = []; } if (typeof window !== 'undefined') { @@ -786,21 +780,26 @@ if (typeof window !== 'undefined') { } -// Friendly labels for the dynamic "← Back to X" button on the artist-detail page. -// Page id (the value of currentPage) -> button label. -const _ARTIST_DETAIL_BACK_LABELS = { - library: 'Back to Library', - search: 'Back to Search', - discover: 'Back to Discover', - watchlist: 'Back to Watchlist', - wishlist: 'Back to Wishlist', - stats: 'Back to Stats', - 'playlist-explorer': 'Back to Explorer', - automations: 'Back to Automations', - dashboard: 'Back to Dashboard', - sync: 'Back to Sync', - 'active-downloads': 'Back to Downloads', -}; +// Public navigation entrypoint for artist detail. Callers should use this so +// artist-detail navigations stay URL-driven, while the renderer handoff below +// remains the legacy implementation detail. +function navigateToArtistDetailPage(artistId, artistName, sourceOverride = null, options = {}) { + const normalizedSource = sourceOverride || null; + + if (!artistId) return false; + if (currentPage === 'artist-detail' && + String(artistId) === String(artistDetailPageState.currentArtistId) && + String(normalizedSource || '') === String(artistDetailPageState.currentArtistSource || '')) { + return true; + } + + return navigateToPage('artist-detail', { + artistId, + artistSource: normalizedSource, + forceReload: true, + replace: options.replace === true, + }); +} function navigateToArtistDetail(artistId, artistName, sourceOverride = null, options = {}) { const normalizedSource = sourceOverride || null; @@ -817,49 +816,11 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt artistSource: normalizedSource, skipRouteChange: options.skipRouteChange === true }); - _updateArtistDetailBackButtonLabel(); } return; } console.log(`🎡 Navigating to artist detail: ${artistName} (ID: ${artistId}${sourceOverride ? `, source: ${sourceOverride}` : ''})`); - // Capture the current location on the origin stack BEFORE navigateToPage - // flips currentPage. The back button walks this stack one step at a time, - // so a chain like Search β†’ A β†’ similar B β†’ similar C steps back through - // C β†’ B β†’ A β†’ Search instead of jumping straight home. `skipOriginPush` - // lets the back button re-enter a prior artist without re-pushing. - if (!options.skipOriginPush) { - // Fresh entry (from a non-artist page) starts a new chain; any stale - // entries from a prior artist-detail visit are dropped. - if (currentPage !== 'artist-detail') { - artistDetailPageState.originStack = []; - } - - let entry; - if (currentPage === 'artist-detail' && artistDetailPageState.currentArtistId) { - entry = { - type: 'artist', - id: artistDetailPageState.currentArtistId, - name: artistDetailPageState.currentArtistName, - source: artistDetailPageState.currentArtistSource, - }; - } else { - const pageId = (typeof currentPage === 'string' && currentPage && currentPage !== 'artist-detail') - ? currentPage : 'library'; - entry = { type: 'page', pageId }; - } - - // Avoid pushing a duplicate top entry on repeated clicks of the same target. - const top = artistDetailPageState.originStack[artistDetailPageState.originStack.length - 1]; - const isDuplicate = top && top.type === entry.type && ( - (entry.type === 'page' && top.pageId === entry.pageId) || - (entry.type === 'artist' && String(top.id) === String(entry.id)) - ); - if (!isDuplicate) { - artistDetailPageState.originStack.push(entry); - } - } - // Abort any in-progress completion stream if (artistDetailPageState.completionController) { artistDetailPageState.completionController.abort(); @@ -909,9 +870,6 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt skipRouteChange: options.skipRouteChange === true }); - // Update back-button label to reflect where the next pop will land. - _updateArtistDetailBackButtonLabel(); - // Initialize if needed and load data if (!artistDetailPageState.isInitialized) { initializeArtistDetailPage(); @@ -921,27 +879,11 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt loadArtistDetailData(artistId, artistName); } -function _updateArtistDetailBackButtonLabel() { - const backBtnLabel = document.querySelector('#artist-detail-back-btn span'); - if (!backBtnLabel) return; - const stack = artistDetailPageState.originStack || []; - const top = stack[stack.length - 1]; - if (!top) { - backBtnLabel.textContent = `← ${_ARTIST_DETAIL_BACK_LABELS.library}`; - } else if (top.type === 'artist') { - backBtnLabel.textContent = `← Back to ${top.name}`; - } else { - const friendly = _ARTIST_DETAIL_BACK_LABELS[top.pageId] || _ARTIST_DETAIL_BACK_LABELS.library; - backBtnLabel.textContent = `← ${friendly}`; - } -} - function initializeArtistDetailPage() { console.log("πŸ”§ Initializing Artist Detail page..."); - // Initialize back button β€” pops the origin stack one step at a time so a - // chain like Search β†’ A β†’ B β†’ C walks back through C β†’ B β†’ A β†’ Search - // instead of jumping straight to the original entry page. + // Initialize back button β€” use browser history when possible, with a + // simple library fallback if the user lands here without in-app history. const backBtn = document.getElementById("artist-detail-back-btn"); if (backBtn) { backBtn.addEventListener("click", () => { @@ -951,27 +893,12 @@ function initializeArtistDetailPage() { artistDetailPageState.completionController = null; } - const stack = artistDetailPageState.originStack || []; - if (stack.length > 0) { - const target = stack.pop(); - if (target.type === 'artist') { - // Re-enter a prior artist in the chain without re-pushing, - // so the stack keeps shrinking as the user steps back. - navigateToArtistDetail(target.id, target.name, target.source, { skipOriginPush: true }); - return; - } - // target.type === 'page' β€” fully exit the artist-detail chain - artistDetailPageState.currentArtistId = null; - artistDetailPageState.currentArtistName = null; - artistDetailPageState.originStack = []; - navigateToPage(target.pageId); + if (window.history.length > 1) { + window.history.back(); return; } // No history β€” default to library - artistDetailPageState.currentArtistId = null; - artistDetailPageState.currentArtistName = null; - artistDetailPageState.originStack = []; navigateToPage('library'); }); } diff --git a/webui/static/media-player.js b/webui/static/media-player.js index 85f25796..a6889aa4 100644 --- a/webui/static/media-player.js +++ b/webui/static/media-player.js @@ -1237,7 +1237,7 @@ function initExpandedPlayer() { gotoArtistBtn.addEventListener('click', () => { if (currentTrack && currentTrack.artist_id) { closeNowPlayingModal(); - navigateToArtistDetail(currentTrack.artist_id, currentTrack.artist || ''); + navigateToArtistDetailPage(currentTrack.artist_id, currentTrack.artist || ''); } }); } @@ -2396,4 +2396,3 @@ function updateMediaSessionPlaybackState() { } // =============================== - diff --git a/webui/static/search.js b/webui/static/search.js index 682d48f0..8cedc3c1 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -320,7 +320,7 @@ function initializeSearchModeToggle() { onClick: () => { console.log(`🎡 Opening library artist detail: ${artist.name} (ID: ${artist.id})`); hideDropdown(); - navigateToArtistDetail(artist.id, artist.name); + navigateToArtistDetailPage(artist.id, artist.name); } }) ); @@ -341,7 +341,7 @@ function initializeSearchModeToggle() { const sourceOverride = searchController.state.activeSource; console.log(`🎡 Opening artist detail: ${artist.name} (ID: ${artist.id}, source: ${sourceOverride})`); hideDropdown(); - navigateToArtistDetail(artist.id, artist.name, sourceOverride || null); + navigateToArtistDetailPage(artist.id, artist.name, sourceOverride || null); } }) ); diff --git a/webui/static/shared-helpers.js b/webui/static/shared-helpers.js index 7a509f6c..85957c65 100644 --- a/webui/static/shared-helpers.js +++ b/webui/static/shared-helpers.js @@ -3862,7 +3862,7 @@ function createSimilarArtistBubble(artist) { // library and source artists thanks to the source-aware backend endpoint. bubble.addEventListener('click', () => { console.log(`🎡 Clicked similar artist: ${artist.name} (ID: ${artist.id})`); - navigateToArtistDetail(artist.id, artist.name, artist.source || null); + navigateToArtistDetailPage(artist.id, artist.name, artist.source || null); }); return bubble; diff --git a/webui/static/shell-bridge.js b/webui/static/shell-bridge.js index 64c73f6e..9eb4b04e 100644 --- a/webui/static/shell-bridge.js +++ b/webui/static/shell-bridge.js @@ -165,6 +165,7 @@ window.SoulSyncWebShellBridge = { activateLegacyPath(pathname) { activateLegacyPath(pathname); }, + navigateToArtistDetailPage, navigateToArtistDetail, showReactHost(pageId) { showReactHost(pageId); diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index 265d71eb..46304405 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -162,7 +162,7 @@ async function loadStatsData() { ${i + 1} ${item.image_url ? `` : ''}
-
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
+
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
${item.global_listeners ? _fmt(item.global_listeners) + ' global listeners' : ''}
${_fmt(item.play_count)} plays @@ -176,7 +176,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
${_fmt(item.play_count)} plays
@@ -189,7 +189,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' Β· ' + _esc(item.album) : ''}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' Β· ' + _esc(item.album) : ''}
${_fmt(item.play_count)} plays @@ -231,7 +231,7 @@ function _renderTopArtistsVisual(artists) { ${top5.map((a, i) => { const pct = Math.round((a.play_count / maxPlays) * 100); const size = 44 + (4 - i) * 6; // Largest first: 68, 62, 56, 50, 44 - return `
+ return `
${!a.image_url ? `${(a.name || '?')[0]}` : ''}
From 30c687ae7bcd28a7521a1d2b573739f42692ac58 Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Tue, 19 May 2026 09:28:05 +0300 Subject: [PATCH 04/24] refactor(webui): cancel similar-artists in route - expose a shell-bridge cancel primitive for similar-artists loading - stop stale similar-artists streams from the artist-detail route lifecycle - keep the legacy loader abort-only and make abort logs page-agnostic - update bridge and route tests for the new cleanup path --- webui/src/app/router.test.tsx | 1 + webui/src/platform/shell/bridge.test.ts | 2 + webui/src/platform/shell/globals.d.ts | 1 + .../src/routes/artist-detail/$source/$id.tsx | 14 ++++++- .../src/routes/artist-detail/-route.test.tsx | 25 +++++++++++ webui/src/routes/issues/-route.test.tsx | 1 + webui/static/core.js | 7 ++++ webui/static/library.js | 3 ++ webui/static/shared-helpers.js | 41 ++++++++++++++----- webui/static/shell-bridge.js | 5 +++ 10 files changed, 87 insertions(+), 13 deletions(-) diff --git a/webui/src/app/router.test.tsx b/webui/src/app/router.test.tsx index 7f1921f9..63295b4b 100644 --- a/webui/src/app/router.test.tsx +++ b/webui/src/app/router.test.tsx @@ -61,6 +61,7 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { setActivePageChrome: vi.fn(), activateLegacyPath: vi.fn(), navigateToArtistDetail: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), ...overrides, }; diff --git a/webui/src/platform/shell/bridge.test.ts b/webui/src/platform/shell/bridge.test.ts index a33f44cd..9682bbe7 100644 --- a/webui/src/platform/shell/bridge.test.ts +++ b/webui/src/platform/shell/bridge.test.ts @@ -17,6 +17,7 @@ describe('waitForShellContext', () => { getCurrentProfileContext: vi.fn(() => ({ profileId: 2, isAdmin: true })), resolveLegacyPath: vi.fn(() => 'issues'), setActivePageChrome: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), } as NonNullable; @@ -38,6 +39,7 @@ describe('waitForShellContext', () => { getCurrentProfileContext, resolveLegacyPath: vi.fn(() => 'issues'), setActivePageChrome: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), } as NonNullable; diff --git a/webui/src/platform/shell/globals.d.ts b/webui/src/platform/shell/globals.d.ts index 82915a63..2db2c644 100644 --- a/webui/src/platform/shell/globals.d.ts +++ b/webui/src/platform/shell/globals.d.ts @@ -51,6 +51,7 @@ declare global { replace?: boolean; }, ) => void; + cancelSimilarArtistsLoad: () => void; showReactHost: (pageId: ShellPageId) => void; }; } diff --git a/webui/src/routes/artist-detail/$source/$id.tsx b/webui/src/routes/artist-detail/$source/$id.tsx index a3348a82..5430d2c3 100644 --- a/webui/src/routes/artist-detail/$source/$id.tsx +++ b/webui/src/routes/artist-detail/$source/$id.tsx @@ -1,5 +1,5 @@ import { createFileRoute } from '@tanstack/react-router'; -import { useLayoutEffect } from 'react'; +import { useEffect, useLayoutEffect } from 'react'; import { useShellBridge } from '@/platform/shell/route-controllers'; @@ -8,7 +8,8 @@ export const Route = createFileRoute('/artist-detail/$source/$id')({ }); // Thin legacy handoff: TanStack owns the URL shape here, but the vanilla JS -// artist-detail page still renders the actual experience for now. +// artist-detail page still renders the actual experience for now. The route +// owns cancellation so similar-artist loading stops when this page changes. function ArtistDetailPage() { const bridge = useShellBridge(); const { source, id } = Route.useParams(); @@ -16,11 +17,20 @@ function ArtistDetailPage() { useLayoutEffect(() => { if (!bridge) return; + bridge.cancelSimilarArtistsLoad(); const normalizedSource = source.toLowerCase() === 'library' ? null : source.toLowerCase(); bridge.navigateToArtistDetail(id, '', normalizedSource, { skipRouteChange: true, }); }, [bridge, id, source]); + useEffect(() => { + if (!bridge) return; + + return () => { + bridge.cancelSimilarArtistsLoad(); + }; + }, [bridge, id, source]); + return null; } diff --git a/webui/src/routes/artist-detail/-route.test.tsx b/webui/src/routes/artist-detail/-route.test.tsx index 92ac09eb..232ff944 100644 --- a/webui/src/routes/artist-detail/-route.test.tsx +++ b/webui/src/routes/artist-detail/-route.test.tsx @@ -16,6 +16,7 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { setActivePageChrome: vi.fn(), activateLegacyPath: vi.fn(), navigateToArtistDetail: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), ...overrides, }; @@ -71,4 +72,28 @@ describe('artist-detail route', () => { ); }); }); + + it('cancels the similar artists stream when the route unmounts', async () => { + const { unmount } = renderArtistDetailRoute(['/artist-detail/spotify/2YZyLoL8N0Wb9xBt1NhZWg']); + + await waitFor(() => { + expect(window.SoulSyncWebShellBridge?.navigateToArtistDetail).toHaveBeenCalledWith( + '2YZyLoL8N0Wb9xBt1NhZWg', + '', + 'spotify', + { + skipRouteChange: true, + }, + ); + }); + + const cancelSimilarArtistsLoad = window.SoulSyncWebShellBridge?.cancelSimilarArtistsLoad as ReturnType; + cancelSimilarArtistsLoad.mockClear(); + + unmount(); + + await waitFor(() => { + expect(cancelSimilarArtistsLoad).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/webui/src/routes/issues/-route.test.tsx b/webui/src/routes/issues/-route.test.tsx index 547745ef..7bbc0353 100644 --- a/webui/src/routes/issues/-route.test.tsx +++ b/webui/src/routes/issues/-route.test.tsx @@ -23,6 +23,7 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { setActivePageChrome: vi.fn(), activateLegacyPath: vi.fn(), navigateToArtistDetail: vi.fn(), + cancelSimilarArtistsLoad: vi.fn(), showReactHost: vi.fn(), ...overrides, }; diff --git a/webui/static/core.js b/webui/static/core.js index 426db439..f1b047f6 100644 --- a/webui/static/core.js +++ b/webui/static/core.js @@ -201,6 +201,13 @@ let artistsSearchController = null; let artistCompletionController = null; // Track ongoing completion check to cancel when navigating away let similarArtistsController = null; // Track ongoing similar artists stream to cancel when navigating away +function cancelSimilarArtistsLoad() { + if (similarArtistsController) { + similarArtistsController.abort(); + similarArtistsController = null; + } +} + // --- Lazy Background Image Observer --- // Watches elements with data-bg-src, applies background-image when visible, unobserves after. const lazyBgObserver = new IntersectionObserver((entries) => { diff --git a/webui/static/library.js b/webui/static/library.js index f96a2bee..0ef9f1f2 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -1192,6 +1192,9 @@ function populateArtistDetailPage(data) { // MusicMap name lookup). Fire-and-forget β€” the function handles its own // loading state and errors. if (artist && artist.name && typeof loadSimilarArtists === 'function') { + if (typeof cancelSimilarArtistsLoad === 'function') { + cancelSimilarArtistsLoad(); + } loadSimilarArtists(artist.name); } } diff --git a/webui/static/shared-helpers.js b/webui/static/shared-helpers.js index 85957c65..bbc9ca0c 100644 --- a/webui/static/shared-helpers.js +++ b/webui/static/shared-helpers.js @@ -695,7 +695,7 @@ async function checkDiscographyCompletion(artistId, discography) { } catch (error) { // Don't show error if it was aborted (user navigated away) if (error.name === 'AbortError') { - console.log('⏹️ Completion check aborted (user navigated to new artist)'); + console.log('⏹️ Completion check aborted (user navigated away)'); return; } @@ -3574,16 +3574,21 @@ async function loadSimilarArtists(artistName) { container.innerHTML = ''; section.style.display = 'block'; + let controller = null; + let signal = null; + try { // Create new abort controller for this similar artists stream - similarArtistsController = new AbortController(); + controller = new AbortController(); + similarArtistsController = controller; + signal = controller.signal; // Use streaming endpoint for real-time bubble creation const url = `/api/artist/similar/${encodeURIComponent(artistName)}/stream`; console.log(`πŸ“‘ Streaming from: ${url}`); const response = await fetch(url, { - signal: similarArtistsController.signal + signal }); if (!response.ok) { @@ -3612,6 +3617,7 @@ async function loadSimilarArtists(artistName) { buffer = messages.pop() || ''; // Keep incomplete message in buffer for (const message of messages) { + if (signal?.aborted) return; if (!message.trim() || !message.startsWith('data: ')) continue; try { @@ -3622,6 +3628,7 @@ async function loadSimilarArtists(artistName) { } if (jsonData.artist) { + if (signal?.aborted) return; // Hide loading on first artist if (artistCount === 0) { loadingEl.classList.add('hidden'); @@ -3636,6 +3643,7 @@ async function loadSimilarArtists(artistName) { } if (jsonData.complete) { + if (signal?.aborted) return; console.log(`πŸŽ‰ Streaming complete: ${jsonData.total} artists`); if (artistCount === 0) { @@ -3648,7 +3656,7 @@ async function loadSimilarArtists(artistName) { `; } else { // Lazy load images for similar artists that don't have them - lazyLoadSimilarArtistImages(container); + await lazyLoadSimilarArtistImages(container, signal); } } } catch (parseError) { @@ -3658,13 +3666,14 @@ async function loadSimilarArtists(artistName) { } // Clear the controller when done - similarArtistsController = null; + if (similarArtistsController === controller) { + similarArtistsController = null; + } } catch (error) { // Don't show error if it was aborted (user navigated away) - if (error.name === 'AbortError') { - console.log('⏹️ Similar artists stream aborted (user navigated to new artist)'); - loadingEl.classList.add('hidden'); + if (error.name === 'AbortError' || signal?.aborted) { + console.log('⏹️ Similar artists stream aborted (user navigated away)'); return; } @@ -3683,15 +3692,18 @@ async function loadSimilarArtists(artistName) { `; } finally { // Always clear the controller - similarArtistsController = null; + if (similarArtistsController === controller) { + similarArtistsController = null; + } } } /** * Lazy load images for similar artist bubbles that don't have images */ -async function lazyLoadSimilarArtistImages(container) { +async function lazyLoadSimilarArtistImages(container, signal) { if (!container) return; + if (signal?.aborted) return; const bubblesNeedingImages = container.querySelectorAll('.similar-artist-bubble[data-needs-image="true"]'); @@ -3707,9 +3719,11 @@ async function lazyLoadSimilarArtistImages(container) { const bubbles = Array.from(bubblesNeedingImages); for (let i = 0; i < bubbles.length; i += batchSize) { + if (signal?.aborted) return; const batch = bubbles.slice(i, i + batchSize); await Promise.all(batch.map(async (bubble) => { + if (signal?.aborted) return; const artistId = bubble.getAttribute('data-artist-id'); const artistSource = bubble.getAttribute('data-artist-source') || ''; const artistPlugin = bubble.getAttribute('data-artist-plugin') || ''; @@ -3724,9 +3738,11 @@ async function lazyLoadSimilarArtistImages(container) { ? `/api/artist/${encodeURIComponent(artistId)}/image?${params.toString()}` : `/api/artist/${encodeURIComponent(artistId)}/image`; - const response = await fetch(imageUrl); + const response = await fetch(imageUrl, { signal }); const data = await response.json(); + if (signal?.aborted) return; + if (data.success && data.image_url) { const imageContainer = bubble.querySelector('.similar-artist-bubble-image'); if (imageContainer) { @@ -3737,6 +3753,9 @@ async function lazyLoadSimilarArtistImages(container) { } } } catch (error) { + if (error?.name === 'AbortError' || signal?.aborted) { + return; + } console.warn(`⚠️ Failed to load image for similar artist ${artistId}:`, error); } })); diff --git a/webui/static/shell-bridge.js b/webui/static/shell-bridge.js index 9eb4b04e..16167032 100644 --- a/webui/static/shell-bridge.js +++ b/webui/static/shell-bridge.js @@ -167,6 +167,11 @@ window.SoulSyncWebShellBridge = { }, navigateToArtistDetailPage, navigateToArtistDetail, + cancelSimilarArtistsLoad() { + if (typeof cancelSimilarArtistsLoad === 'function') { + cancelSimilarArtistsLoad(); + } + }, showReactHost(pageId) { showReactHost(pageId); }, From 0d683d87c0f39cdc24e7511842c4dbc7b6b7bbec Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Tue, 19 May 2026 10:19:06 +0300 Subject: [PATCH 05/24] refactor(webui): link artist detail navigation - replace click-driven artist-detail hops with semantic links - keep SPA transitions via shell bridge interception for /artist-detail/:source/:id - drop legacy page helper wrappers and dead bridge plumbing --- webui/index.html | 11 +-- webui/src/platform/shell/globals.d.ts | 8 -- webui/static/api-monitor.js | 45 +++++------- webui/static/discover.js | 101 +++++++++----------------- webui/static/downloads.js | 53 +------------- webui/static/library.js | 30 +------- webui/static/media-player.js | 33 +++++++-- webui/static/search.js | 13 +--- webui/static/shared-helpers.js | 27 ++++--- webui/static/shell-bridge.js | 43 ++++++++++- webui/static/stats-automations.js | 10 +-- webui/static/style.css | 9 ++- webui/static/sync-spotify.js | 24 +++++- 13 files changed, 187 insertions(+), 220 deletions(-) diff --git a/webui/index.html b/webui/index.html index 5ddbcb2a..4bcfba98 100644 --- a/webui/index.html +++ b/webui/index.html @@ -2769,11 +2769,11 @@
- +
diff --git a/webui/src/platform/shell/globals.d.ts b/webui/src/platform/shell/globals.d.ts index 2db2c644..65d5b471 100644 --- a/webui/src/platform/shell/globals.d.ts +++ b/webui/src/platform/shell/globals.d.ts @@ -43,14 +43,6 @@ declare global { skipRouteChange?: boolean; }, ) => void; - navigateToArtistDetailPage: ( - artistId: string | number, - artistName: string, - sourceOverride?: string | null, - options?: { - replace?: boolean; - }, - ) => void; cancelSimilarArtistsLoad: () => void; showReactHost: (pageId: ShellPageId) => void; }; diff --git a/webui/static/api-monitor.js b/webui/static/api-monitor.js index aa356e8b..d53d0667 100644 --- a/webui/static/api-monitor.js +++ b/webui/static/api-monitor.js @@ -2316,6 +2316,25 @@ async function openWatchlistArtistDetailView(artistId, artistName) { if (artist.mood) metaTags.push(`${escapeHtml(artist.mood)}`); if (artist.label) metaTags.push(`${escapeHtml(artist.label)}`); + let discogId = null; + let discogSource = null; + const activeSrc = (currentMusicSourceName || '').toLowerCase(); + if (activeSrc.includes('spotify') && spotify_artist_id) { + discogId = spotify_artist_id; discogSource = 'spotify'; + } else if (activeSrc.includes('discogs') && discogs_artist_id) { + discogId = discogs_artist_id; discogSource = 'discogs'; + } else if (activeSrc.includes('deezer') && deezer_artist_id) { + discogId = deezer_artist_id; discogSource = 'deezer'; + } else if (activeSrc.includes('musicbrainz') && musicbrainz_artist_id) { + discogId = musicbrainz_artist_id; discogSource = 'musicbrainz'; + } else if (itunes_artist_id) { + discogId = itunes_artist_id; discogSource = 'itunes'; + } else { + discogId = spotify_artist_id || discogs_artist_id || deezer_artist_id || musicbrainz_artist_id || itunes_artist_id; + discogSource = spotify_artist_id ? 'spotify' : discogs_artist_id ? 'discogs' : deezer_artist_id ? 'deezer' : musicbrainz_artist_id ? 'musicbrainz' : 'itunes'; + } + const discogHref = discogId ? buildArtistDetailPath(discogId, discogSource) : '#'; + overlay.innerHTML = ` ${artist.banner_url ? `
@@ -2398,7 +2417,7 @@ async function openWatchlistArtistDetailView(artistId, artistName) {
- + View Discography
@@ -2410,30 +2429,6 @@ async function openWatchlistArtistDetailView(artistId, artistName) { closeWatchlistArtistDetailView(); }); - overlay.querySelector('.watchlist-detail-discog-action').addEventListener('click', () => { - // Use the ID matching the active metadata source - let discogId, source; - const activeSrc = (currentMusicSourceName || '').toLowerCase(); - if (activeSrc.includes('spotify') && spotify_artist_id) { - discogId = spotify_artist_id; source = 'spotify'; - } else if (activeSrc.includes('discogs') && discogs_artist_id) { - discogId = discogs_artist_id; source = 'discogs'; - } else if (activeSrc.includes('deezer') && deezer_artist_id) { - discogId = deezer_artist_id; source = 'deezer'; - } else if (activeSrc.includes('musicbrainz') && musicbrainz_artist_id) { - discogId = musicbrainz_artist_id; source = 'musicbrainz'; - } else if (itunes_artist_id) { - discogId = itunes_artist_id; source = 'itunes'; - } else { - discogId = spotify_artist_id || discogs_artist_id || deezer_artist_id || musicbrainz_artist_id || itunes_artist_id; - source = spotify_artist_id ? 'spotify' : discogs_artist_id ? 'discogs' : deezer_artist_id ? 'deezer' : musicbrainz_artist_id ? 'musicbrainz' : 'itunes'; - } - if (discogId) { - closeWatchlistArtistDetailView(); - navigateToArtistDetailPage(discogId, artistName, source); - } - }); - overlay.querySelector('.watchlist-detail-settings-action').addEventListener('click', () => { // Remove overlay immediately so it doesn't block the config modal const detailOverlay = document.querySelector('.watchlist-artist-detail-overlay'); diff --git a/webui/static/discover.js b/webui/static/discover.js index 99191459..e1d05c12 100644 --- a/webui/static/discover.js +++ b/webui/static/discover.js @@ -269,10 +269,9 @@ function displayDiscoverHeroArtist(artist) { if (discographyBtn && artistId) { discographyBtn.setAttribute('data-artist-id', artistId); discographyBtn.setAttribute('data-artist-name', artist.artist_name); - // Source the click handler will pass to navigateToArtistDetail. Without - // this, source-only hero artists (which is the typical case β€” they - // come from discover similar-artists, not the library) get looked up - // as library IDs and 404. Backend always includes artist.source. + discographyBtn.href = buildArtistDetailPath(artistId, artist.source || null); + // Keep the source on the link so source-only hero artists resolve to + // the correct artist-detail URL instead of being treated as library IDs. if (artist.source) discographyBtn.setAttribute('data-source', artist.source); else discographyBtn.removeAttribute('data-source'); // Also store both IDs for cross-source operations @@ -557,7 +556,7 @@ function renderRecommendedArtistsModal(modal, artists, source = null) { const similarText = artist.occurrence_count > 1 ? `Similar to ${artist.occurrence_count} in your watchlist` : 'Similar to an artist in your watchlist'; - const artistSource = artist.source || source || _recommendedArtistsSource || ''; + const artistSource = artist.source || source || _recommendedArtistsSource || ''; return `
Add to Watchlist -
- ${artist.image_url ? ` - ${escapeHtml(artist.artist_name)} - ` : ` -
🎀
- `} -
-
- ${escapeHtml(artist.artist_name)} - ${similarText} -
${genreTags}
-
+ +
+ ${artist.image_url ? ` + ${escapeHtml(artist.artist_name)} + ` : ` +
🎀
+ `} +
+
+ ${escapeHtml(artist.artist_name)} + ${similarText} +
${genreTags}
+
+
`; }).join('')} @@ -599,16 +602,6 @@ function renderRecommendedArtistsModal(modal, artists, source = null) { if (watchlistBtn) { e.stopPropagation(); toggleRecommendedWatchlist(watchlistBtn); - return; - } - - const card = e.target.closest('.recommended-artist-card'); - if (card) { - const artistId = card.getAttribute('data-artist-id'); - const artistSource = card.getAttribute('data-artist-source') || _recommendedArtistsSource || null; - const nameEl = card.querySelector('.recommended-card-name'); - const artistName = nameEl ? nameEl.textContent : ''; - viewRecommendedArtistDiscography(artistId, artistName, artistSource); } }); } @@ -739,11 +732,6 @@ async function checkRecommendedWatchlistStatuses(artists) { } } -async function viewRecommendedArtistDiscography(artistId, artistName, source = null) { - closeRecommendedArtistsModal(); - navigateToArtistDetailPage(artistId, artistName, source || null); -} - async function checkAllHeroWatchlistStatus() { const btn = document.getElementById('discover-hero-watch-all'); if (!btn || !discoverHeroArtists || discoverHeroArtists.length === 0) return; @@ -811,26 +799,6 @@ function jumpToDiscoverHeroSlide(index) { updateDiscoverHeroIndicators(); } -async function viewDiscoverHeroDiscography() { - const button = document.getElementById('discover-hero-discography'); - if (!button) return; - - const artistId = button.getAttribute('data-artist-id'); - const artistName = button.getAttribute('data-artist-name'); - // Pass the source so /api/artist-detail knows to synthesize from that - // metadata provider instead of doing a local DB lookup. Hero similar - // artists are almost always source-only (not in the library). - const source = button.getAttribute('data-source') || null; - - if (!artistId || !artistName) { - console.error('No artist data found for discography view'); - return; - } - - console.log(`🎡 Navigating to artist detail for: ${artistName} (source: ${source || 'library'})`); - navigateToArtistDetailPage(artistId, artistName, source); -} - function showDiscoverHeroEmpty() { const titleEl = document.getElementById('discover-hero-title'); const subtitleEl = document.getElementById('discover-hero-subtitle'); @@ -4514,10 +4482,7 @@ function _renderYourArtistCard(artist) { const detailSource = _pickArtistDetailSource(artist); const hasId = detailSource.id && detailSource.id !== ''; - // Navigate to Artists page (name click) β€” source artist id, needs inline view - const navAction = hasId - ? `event.stopPropagation(); navigateToArtistDetailPage('${escapeForInlineJs(detailSource.id)}', '${escapeForInlineJs(artist.artist_name)}', '${escapeForInlineJs(detailSource.source)}' || null)` - : ''; + const detailHref = hasId ? buildArtistDetailPath(detailSource.id, detailSource.source) : ''; // Open info modal (card body click) β€” pass pool ID so we can look up all data const infoAction = hasId @@ -4545,7 +4510,9 @@ function _renderYourArtistCard(artist) {
${originDots}
-
${_esc(artist.artist_name)}
+ ${hasId + ? `${_esc(artist.artist_name)}` + : `
${_esc(artist.artist_name)}
`}
`; @@ -4695,10 +4662,10 @@ async function openYourArtistInfoModal(poolId) { Explore - + `; } } catch (err) { @@ -6595,9 +6562,9 @@ function _artMapSetupInteraction(canvas) {
ⓘ Artist Info
-
+ 💿 View Discography -
+
👁 ${node.type === 'watchlist' ? 'On Watchlist' : 'Add to Watchlist'}
@@ -7275,9 +7242,9 @@ async function openGenreDeepDive(genre) { // Always open on Artists page with discography β€” pass source for correct routing const imgUrl = _esc(a.image_url || ''); const artSource = _esc(a.source || ''); - const clickAction = `onclick="document.getElementById('genre-deep-dive-modal').remove();navigateToArtistDetailPage('${_esc(a.entity_id)}','${_esc(a.name)}','${artSource}' || null)"`; + const detailHref = a.entity_id ? buildArtistDetailPath(a.entity_id, a.source || null) : '#'; const srcClass = (a.source || '').toLowerCase(); - return `
+ return `
${!a.image_url ? '🎀' : ''}
@@ -7285,7 +7252,7 @@ async function openGenreDeepDive(genre) {
${_esc(a.name)}
${a.followers ? `
${_fmtNum(a.followers)} followers
` : ''} ${a.library_id ? '
In Library
' : ''} -
`; + `; }).join('')}
`; diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 5e5cad38..678c1796 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -633,48 +633,6 @@ async function openDownloadMissingModalForYouTube(virtualPlaylistId, playlistNam hideLoadingOverlay(); } -function _navigateToArtistFromModal(artistId, artistName, imageUrl, source, playlistId) { - if (!artistName) return; - // Close the download modal - const process = playlistId ? activeDownloadProcesses[playlistId] : null; - const artistContext = process?.artist || {}; - const inferredSource = artistContext.spotify_artist_id ? 'spotify' - : artistContext.itunes_artist_id ? 'itunes' - : (artistContext.deezer_artist_id || artistContext.deezer_id) ? 'deezer' - : (artistContext.discogs_artist_id || artistContext.discogs_id) ? 'discogs' - : (artistContext.amazon_artist_id || artistContext.amazon_id) ? 'amazon' - : (artistContext.soul_id || artistContext.hydrabase_artist_id) ? 'hydrabase' - : null; - const resolvedSource = source || process?.artist?.source || process?.album?.source || process?.source || inferredSource; - const sourceKey = (resolvedSource || '').toString().toLowerCase(); - const sourceIdFields = { - spotify: ['spotify_artist_id', 'id', 'artist_id'], - itunes: ['itunes_artist_id', 'artist_id', 'id'], - deezer: ['deezer_artist_id', 'deezer_id', 'artist_id', 'id'], - discogs: ['discogs_artist_id', 'discogs_id', 'artist_id', 'id'], - amazon: ['amazon_artist_id', 'amazon_id', 'artist_id', 'id'], - hydrabase: ['soul_id', 'hydrabase_artist_id', 'artist_id', 'id'], - musicbrainz: ['musicbrainz_id', 'artist_id', 'id'], - }; - let resolvedArtistId = artistId; - for (const field of (sourceIdFields[sourceKey] || ['artist_id', 'id'])) { - const candidate = artistContext?.[field]; - if (candidate) { - resolvedArtistId = candidate; - break; - } - } - if (resolvedArtistId && String(resolvedArtistId).toLowerCase() === String(artistName).toLowerCase()) { - resolvedArtistId = null; - } - if (playlistId) closeDownloadMissingModal(playlistId); - if (!resolvedArtistId || !resolvedSource) { - showToast(`Artist details are not available for ${artistName}`, 'warning'); - return; - } - navigateToArtistDetailPage(resolvedArtistId, artistName, resolvedSource); -} - async function closeDownloadMissingModal(playlistId) { const process = activeDownloadProcesses[playlistId]; if (!process) { @@ -5698,13 +5656,13 @@ function _gsRenderFromState(state) { if (dbArtists.length) { h += '
πŸ“š In Your Library
'; - h += dbArtists.map(a => `
${a.image_url ? `` : '🎀'}
${_escToast(a.name)}
Library
`).join(''); + h += dbArtists.map(a => `${a.image_url ? `
` : '
🎀
'}
${_escToast(a.name)}
Library
`).join(''); h += '
'; } if (artists.length) { h += `
🎀 Artists ${srcLabel}
`; - h += artists.map(a => `
${a.image_url ? `` : '🎀'}
${_escToast(a.name)}
`).join(''); + h += artists.map(a => `${a.image_url ? `
` : '
🎀
'}
${_escToast(a.name)}
`).join(''); h += '
'; } @@ -5778,13 +5736,6 @@ async function _gsLazyLoadArtistImages() { } } -function _gsClickArtist(id, name, isLibrary) { - _gsDeactivate(); - const activeSource = _gsController && _gsController.state.activeSource; - const source = isLibrary ? null : (activeSource || null); - navigateToArtistDetailPage(id, name, source); -} - async function _gsClickAlbum(albumId, albumName, artistName, imageUrl, source) { _gsDeactivate(); // Same flow as handleEnhancedSearchAlbumClick β€” fetch album, open download modal diff --git a/webui/static/library.js b/webui/static/library.js index 0ef9f1f2..f915d73a 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -218,6 +218,7 @@ function displayLibraryArtists(artists) { // Ignore clicks on badge icons (they open external links / toggle watchlist) const badge = e.target.closest('.source-card-icon'); if (badge) { + e.preventDefault(); e.stopPropagation(); const url = badge.dataset.url; if (url) { window.open(url, '_blank'); return; } @@ -233,10 +234,6 @@ function displayLibraryArtists(artists) { } return; } - const card = e.target.closest('.library-artist-card'); - if (card) { - navigateToArtistDetailPage(card.dataset.artistId, card.dataset.artistName); - } }; } @@ -299,14 +296,14 @@ function buildLibraryArtistCardHTML(artist, index) { // Track stats const trackStat = artist.track_count > 0 ? `${artist.track_count} track${artist.track_count !== 1 ? 's' : ''}` : ''; - return `
+ return ` ${badgeContainerHTML} ${imageHTML}

${_esc(artist.name)}

${trackStat}
-
`; + `; } function updateLibraryPagination(pagination) { @@ -780,27 +777,6 @@ if (typeof window !== 'undefined') { } -// Public navigation entrypoint for artist detail. Callers should use this so -// artist-detail navigations stay URL-driven, while the renderer handoff below -// remains the legacy implementation detail. -function navigateToArtistDetailPage(artistId, artistName, sourceOverride = null, options = {}) { - const normalizedSource = sourceOverride || null; - - if (!artistId) return false; - if (currentPage === 'artist-detail' && - String(artistId) === String(artistDetailPageState.currentArtistId) && - String(normalizedSource || '') === String(artistDetailPageState.currentArtistSource || '')) { - return true; - } - - return navigateToPage('artist-detail', { - artistId, - artistSource: normalizedSource, - forceReload: true, - replace: options.replace === true, - }); -} - function navigateToArtistDetail(artistId, artistName, sourceOverride = null, options = {}) { const normalizedSource = sourceOverride || null; diff --git a/webui/static/media-player.js b/webui/static/media-player.js index a6889aa4..b300de5a 100644 --- a/webui/static/media-player.js +++ b/webui/static/media-player.js @@ -107,6 +107,21 @@ function setTrackInfo(track) { document.getElementById('no-track-message').classList.add('hidden'); document.getElementById('media-player').classList.remove('idle'); + const gotoArtistBtn = document.getElementById('np-goto-artist'); + if (gotoArtistBtn) { + if (track.artist_id) { + gotoArtistBtn.href = buildArtistDetailPath(track.artist_id, track.artist_source || null); + gotoArtistBtn.style.pointerEvents = ''; + gotoArtistBtn.setAttribute('aria-disabled', 'false'); + gotoArtistBtn.tabIndex = 0; + } else { + gotoArtistBtn.href = '#'; + gotoArtistBtn.style.pointerEvents = 'none'; + gotoArtistBtn.setAttribute('aria-disabled', 'true'); + gotoArtistBtn.tabIndex = -1; + } + } + // Sync expanded player and media session updateNpTrackInfo(); updateMediaSessionMetadata(); @@ -184,6 +199,14 @@ function clearTrack() { document.getElementById('no-track-message').classList.remove('hidden'); document.getElementById('media-player').classList.add('idle'); + const gotoArtistBtn = document.getElementById('np-goto-artist'); + if (gotoArtistBtn) { + gotoArtistBtn.href = '#'; + gotoArtistBtn.style.pointerEvents = 'none'; + gotoArtistBtn.setAttribute('aria-disabled', 'true'); + gotoArtistBtn.tabIndex = -1; + } + // Reset queue state npQueue = []; npQueueIndex = -1; @@ -1231,15 +1254,11 @@ function initExpandedPlayer() { }); } - // Action button (Go to Artist) + // Action link (Go to Artist) const gotoArtistBtn = document.getElementById('np-goto-artist'); if (gotoArtistBtn) { - gotoArtistBtn.addEventListener('click', () => { - if (currentTrack && currentTrack.artist_id) { - closeNowPlayingModal(); - navigateToArtistDetailPage(currentTrack.artist_id, currentTrack.artist || ''); - } - }); + gotoArtistBtn.style.textDecoration = 'none'; + gotoArtistBtn.style.color = 'inherit'; } // Buffering state listeners on audioPlayer if (audioPlayer) { diff --git a/webui/static/search.js b/webui/static/search.js index 8cedc3c1..4974fad9 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -317,11 +317,7 @@ function initializeSearchModeToggle() { name: artist.name, meta: 'In Your Library', badge: { text: 'Library', class: 'enh-badge-library' }, - onClick: () => { - console.log(`🎡 Opening library artist detail: ${artist.name} (ID: ${artist.id})`); - hideDropdown(); - navigateToArtistDetailPage(artist.id, artist.name); - } + href: buildArtistDetailPath(artist.id), }) ); @@ -337,12 +333,7 @@ function initializeSearchModeToggle() { name: artist.name, meta: 'Artist', badge: sourceBadge, - onClick: () => { - const sourceOverride = searchController.state.activeSource; - console.log(`🎡 Opening artist detail: ${artist.name} (ID: ${artist.id}, source: ${sourceOverride})`); - hideDropdown(); - navigateToArtistDetailPage(artist.id, artist.name, sourceOverride || null); - } + href: buildArtistDetailPath(artist.id, searchController.state.activeSource || null), }) ); diff --git a/webui/static/shared-helpers.js b/webui/static/shared-helpers.js index bbc9ca0c..26745e78 100644 --- a/webui/static/shared-helpers.js +++ b/webui/static/shared-helpers.js @@ -545,7 +545,8 @@ function renderCompactSection(sectionId, listId, countId, items, mapItem) { items.forEach(item => { const config = mapItem(item); - const elem = document.createElement('div'); + const isLink = isArtist && !!config.href; + const elem = document.createElement(isLink ? 'a' : 'div'); // Add appropriate card class if (isArtist) { @@ -566,6 +567,13 @@ function renderCompactSection(sectionId, listId, countId, items, mapItem) { elem.className = 'enh-compact-item track-item'; } + if (isLink) { + elem.href = config.href; + elem.style.color = 'inherit'; + elem.style.textDecoration = 'none'; + elem.setAttribute('aria-label', config.name || 'Artist'); + } + // Build image HTML with type-specific classes let imageClass = 'enh-item-image'; let placeholderClass = 'enh-item-image-placeholder'; @@ -612,7 +620,9 @@ function renderCompactSection(sectionId, listId, countId, items, mapItem) { ${badgeHtml} `; - elem.addEventListener('click', config.onClick); + if (config.onClick) { + elem.addEventListener('click', config.onClick); + } // Add play button handler for tracks if (isTrack && config.onPlay) { @@ -3823,8 +3833,12 @@ function displaySimilarArtists(artists) { */ function createSimilarArtistBubble(artist) { // Create bubble container - const bubble = document.createElement('div'); + const bubble = document.createElement('a'); bubble.className = 'similar-artist-bubble'; + bubble.href = buildArtistDetailPath(artist.id, artist.source || null); + bubble.style.color = 'inherit'; + bubble.style.textDecoration = 'none'; + bubble.setAttribute('aria-label', artist.name); bubble.setAttribute('data-artist-id', artist.id); bubble.setAttribute('data-artist-source', artist.source || ''); if (artist.plugin) { @@ -3877,13 +3891,6 @@ function createSimilarArtistBubble(artist) { bubble.appendChild(genres); } - // Click β†’ navigate to the standalone artist-detail page. Works for both - // library and source artists thanks to the source-aware backend endpoint. - bubble.addEventListener('click', () => { - console.log(`🎡 Clicked similar artist: ${artist.name} (ID: ${artist.id})`); - navigateToArtistDetailPage(artist.id, artist.name, artist.source || null); - }); - return bubble; } diff --git a/webui/static/shell-bridge.js b/webui/static/shell-bridge.js index 16167032..66127869 100644 --- a/webui/static/shell-bridge.js +++ b/webui/static/shell-bridge.js @@ -165,7 +165,6 @@ window.SoulSyncWebShellBridge = { activateLegacyPath(pathname) { activateLegacyPath(pathname); }, - navigateToArtistDetailPage, navigateToArtistDetail, cancelSimilarArtistsLoad() { if (typeof cancelSimilarArtistsLoad === 'function') { @@ -177,5 +176,47 @@ window.SoulSyncWebShellBridge = { }, }; +function _handleShellLinkClick(event) { + if (event.defaultPrevented || event.button !== 0 || _isModifiedLinkClick(event)) return; + + const anchor = event.target?.closest?.('a[href]'); + if (!anchor || (anchor.target && anchor.target !== '_self')) return; + + const href = anchor.getAttribute('href'); + if (!href || href === '#' || href.startsWith('javascript:')) return; + + const router = getWebRouter(); + if (!router?.navigateToPage) return; + + const pathname = anchor.pathname || new URL(anchor.href, window.location.href).pathname; + + if (pathname.startsWith('/artist-detail/')) { + _handleArtistDetailLinkClick(event, pathname, router); + return; + } +} + +function _handleArtistDetailLinkClick(event, pathname, router) { + const parts = pathname.split('/').filter(Boolean); + if (parts.length < 3) return; + + // Keep the semantic link, but hand the click back to TanStack so artist + // detail navigations stay in the SPA when the router is available. + const source = decodeURIComponent(parts[1] || ''); + const artistId = decodeURIComponent(parts.slice(2).join('/')); + if (!source || !artistId) return; + + event.preventDefault(); + void router.navigateToPage('artist-detail', { + artistId, + artistSource: source, + }); +} + +function _isModifiedLinkClick(event) { + return event.metaKey || event.ctrlKey || event.shiftKey || event.altKey; +} + window.addEventListener('popstate', syncActivePageFromLocation); +document.addEventListener('click', _handleShellLinkClick, true); window.dispatchEvent(new CustomEvent(SHELL_BRIDGE_READY_EVENT)); diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index 46304405..8e5e5271 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -162,7 +162,7 @@ async function loadStatsData() { ${i + 1} ${item.image_url ? `` : ''}
-
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
+
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
${item.global_listeners ? _fmt(item.global_listeners) + ' global listeners' : ''}
${_fmt(item.play_count)} plays @@ -176,7 +176,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
${_fmt(item.play_count)} plays
@@ -189,7 +189,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' Β· ' + _esc(item.album) : ''}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' Β· ' + _esc(item.album) : ''}
${_fmt(item.play_count)} plays @@ -231,7 +231,7 @@ function _renderTopArtistsVisual(artists) { ${top5.map((a, i) => { const pct = Math.round((a.play_count / maxPlays) * 100); const size = 44 + (4 - i) * 6; // Largest first: 68, 62, 56, 50, 44 - return `
+ return `
${!a.image_url ? `${(a.name || '?')[0]}` : ''}
@@ -240,7 +240,7 @@ function _renderTopArtistsVisual(artists) {
${_esc(a.name)}
${_fmt(a.play_count)}
- `; + `; }).join('')} `; } diff --git a/webui/static/style.css b/webui/static/style.css index a17d2abd..a6f7619c 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -16876,7 +16876,7 @@ body.helper-mode-active #dashboard-activity-feed:hover { margin-top: 32px; } -.watchlist-detail-actions button { +.watchlist-detail-actions > * { flex: 1; padding: 12px 20px; border-radius: 8px; @@ -16885,9 +16885,14 @@ body.helper-mode-active #dashboard-activity-feed:hover { font-weight: 600; cursor: pointer; transition: background 0.15s ease, transform 0.1s ease; + display: inline-flex; + align-items: center; + justify-content: center; + text-decoration: none; + box-sizing: border-box; } -.watchlist-detail-actions button:active { +.watchlist-detail-actions > *:active { transform: scale(0.98); } diff --git a/webui/static/sync-spotify.js b/webui/static/sync-spotify.js index e86fe83b..ad8ba041 100644 --- a/webui/static/sync-spotify.js +++ b/webui/static/sync-spotify.js @@ -1993,6 +1993,28 @@ function generateDownloadModalHeroSection(context) { const artistImage = artist?.image_url || artist?.images?.[0]?.url; const albumImage = album?.image_url || album?.images?.[0]?.url; const artistSource = artist?.source || album?.source || context.source || ''; + const sourceKey = (artistSource || '').toString().toLowerCase(); + const sourceIdFields = { + spotify: ['spotify_artist_id', 'id', 'artist_id'], + itunes: ['itunes_artist_id', 'artist_id', 'id'], + deezer: ['deezer_artist_id', 'deezer_id', 'artist_id', 'id'], + discogs: ['discogs_artist_id', 'discogs_id', 'artist_id', 'id'], + amazon: ['amazon_artist_id', 'amazon_id', 'artist_id', 'id'], + hydrabase: ['soul_id', 'hydrabase_artist_id', 'artist_id', 'id'], + musicbrainz: ['musicbrainz_id', 'artist_id', 'id'], + }; + let detailArtistId = artist?.id || artist?.artist_id || ''; + for (const field of (sourceIdFields[sourceKey] || ['artist_id', 'id'])) { + const candidate = artist?.[field]; + if (candidate) { + detailArtistId = candidate; + break; + } + } + if (detailArtistId && String(detailArtistId).toLowerCase() === String(artist?.name || '').toLowerCase()) { + detailArtistId = ''; + } + const artistHref = detailArtistId ? buildArtistDetailPath(detailArtistId, artistSource || null) : '#'; // Use album image as background if available if (albumImage) { @@ -2007,7 +2029,7 @@ function generateDownloadModalHeroSection(context) {

${escapeHtml(album.name || 'Unknown Album')}

-
by ${escapeHtml(artist.name || 'Unknown Artist')}
+
by ${escapeHtml(artist.name || 'Unknown Artist')}
${album.album_type || 'Album'} ${trackCount} tracks From fa0ac4ced33e2e4cb1025196366ab598597cffb3 Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Tue, 19 May 2026 10:27:21 +0300 Subject: [PATCH 06/24] refactor(webui): simplify similar artists cleanup - no need for a separate effect since we can use the existing one - no need to cancel the similar artists query upon entering, since the unregister callback already does it --- webui/src/routes/artist-detail/$source/$id.tsx | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/webui/src/routes/artist-detail/$source/$id.tsx b/webui/src/routes/artist-detail/$source/$id.tsx index 5430d2c3..6987c00e 100644 --- a/webui/src/routes/artist-detail/$source/$id.tsx +++ b/webui/src/routes/artist-detail/$source/$id.tsx @@ -1,5 +1,5 @@ import { createFileRoute } from '@tanstack/react-router'; -import { useEffect, useLayoutEffect } from 'react'; +import { useLayoutEffect } from 'react'; import { useShellBridge } from '@/platform/shell/route-controllers'; @@ -17,15 +17,10 @@ function ArtistDetailPage() { useLayoutEffect(() => { if (!bridge) return; - bridge.cancelSimilarArtistsLoad(); const normalizedSource = source.toLowerCase() === 'library' ? null : source.toLowerCase(); bridge.navigateToArtistDetail(id, '', normalizedSource, { skipRouteChange: true, }); - }, [bridge, id, source]); - - useEffect(() => { - if (!bridge) return; return () => { bridge.cancelSimilarArtistsLoad(); From 54efb85240f50f97f9b94361a8c9ada8a1911442 Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Tue, 19 May 2026 19:51:54 +0300 Subject: [PATCH 07/24] fix(webui): guard similar artist bubbles - avoid calling buildArtistDetailPath when a similar artist has no usable id - render a disabled bubble instead so empty MusicBrainz IDs do not crash the panel --- webui/static/shared-helpers.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/webui/static/shared-helpers.js b/webui/static/shared-helpers.js index 26745e78..e5aeb0bc 100644 --- a/webui/static/shared-helpers.js +++ b/webui/static/shared-helpers.js @@ -3832,14 +3832,23 @@ function displaySimilarArtists(artists) { * Create a similar artist bubble card element */ function createSimilarArtistBubble(artist) { + const artistId = artist.id ? String(artist.id).trim() : ''; + const hasArtistDetail = !!artistId; + // Create bubble container - const bubble = document.createElement('a'); + const bubble = document.createElement(hasArtistDetail ? 'a' : 'div'); bubble.className = 'similar-artist-bubble'; - bubble.href = buildArtistDetailPath(artist.id, artist.source || null); + if (hasArtistDetail) { + bubble.href = buildArtistDetailPath(artistId, artist.source || null); + } else { + bubble.setAttribute('aria-disabled', 'true'); + bubble.style.cursor = 'default'; + bubble.style.pointerEvents = 'none'; + } bubble.style.color = 'inherit'; bubble.style.textDecoration = 'none'; bubble.setAttribute('aria-label', artist.name); - bubble.setAttribute('data-artist-id', artist.id); + bubble.setAttribute('data-artist-id', artistId); bubble.setAttribute('data-artist-source', artist.source || ''); if (artist.plugin) { bubble.setAttribute('data-artist-plugin', artist.plugin); From 37f1adef4ac8cf28d9beb96302b8f387254238ed Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Tue, 19 May 2026 21:23:11 +0300 Subject: [PATCH 08/24] fix(stats): remove non-existent artist_source --- webui/static/stats-automations.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index 8e5e5271..100f4a55 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -162,7 +162,7 @@ async function loadStatsData() { ${i + 1} ${item.image_url ? `` : ''}
-
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
+
${item.id ? `${_esc(item.name)}` : _esc(item.name)}${item.soul_id && !String(item.soul_id).startsWith('soul_unnamed_') ? ' ' : ''}
${item.global_listeners ? _fmt(item.global_listeners) + ' global listeners' : ''}
${_fmt(item.play_count)} plays @@ -176,7 +176,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}
${_fmt(item.play_count)} plays
@@ -189,7 +189,7 @@ async function loadStatsData() { ${item.image_url ? `` : ''}
${_esc(item.name)}
-
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' Β· ' + _esc(item.album) : ''}
+
${item.artist_id ? `${_esc(item.artist || '')}` : _esc(item.artist || '')}${item.album ? ' Β· ' + _esc(item.album) : ''}
${_fmt(item.play_count)} plays From 4bfa43bece36a6fff7a747bad05ac3ce155f213f Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 12:34:35 -0700 Subject: [PATCH 09/24] Fix MusicBrainz artist detail showing MBID as name URL-driven routing (PR #644) no longer passes the display name as a query param to the artist-detail endpoint. The source-only detail builder fell back to artist_id when artist_name was empty, surfacing the raw MBID as the page title for MusicBrainz artists. Two fixes in build_source_only_artist_detail: - Drop the artist_id fallback in resolved_name so an MBID can never become the display name - Add a musicbrainz elif branch (matching the Spotify/Deezer/iTunes pattern) that calls MusicBrainzSearchClient.get_artist() to resolve the real name and genres from the MBID when no name is provided --- core/artist_source_detail.py | 13 ++++++++++++- webui/static/helper.js | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/core/artist_source_detail.py b/core/artist_source_detail.py index d8e8b45c..5576f224 100644 --- a/core/artist_source_detail.py +++ b/core/artist_source_detail.py @@ -52,7 +52,7 @@ def build_source_only_artist_detail( ``jsonify`` or equivalent. Status is 200 on success, 404 when the source's discography lookup returned no releases. """ - resolved_name = (artist_name or artist_id or "").strip() + resolved_name = (artist_name or "").strip() # 1. Image URL via the same helper /api/artist//image uses. image_url: Optional[str] = None @@ -101,6 +101,17 @@ def build_source_only_artist_detail( source_genres = az_artist.get("genres") or [] if not image_url and az_artist.get("images"): image_url = az_artist["images"][0].get("url") + elif source == "musicbrainz": + try: + from core.musicbrainz_search import MusicBrainzSearchClient + mb = MusicBrainzSearchClient() + mb_artist = mb.get_artist(artist_id) + if mb_artist: + if not artist_name and mb_artist.get("name"): + resolved_name = mb_artist["name"] + source_genres = mb_artist.get("genres") or [] + except Exception as e: + logger.debug(f"MusicBrainz artist info lookup failed for {artist_id}: {e}") except Exception as e: logger.debug(f"Source-side artist info lookup failed for {source}:{artist_id}: {e}") diff --git a/webui/static/helper.js b/webui/static/helper.js index fc8036d9..71d57a67 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3416,6 +3416,7 @@ const WHATS_NEW = { '2.5.6': [ { date: 'May 18, 2026 β€” 2.5.6 release' }, { title: 'MusicBrainz as Primary Metadata Source', desc: 'MusicBrainz is now a full primary metadata source on equal footing with Deezer, iTunes, Spotify, and Discogs. switch to it in Settings β†’ Metadata Source β€” always available, no account or API key needed, rate-limited to 1 req/sec. covers all primary source flows: search, album/track/artist lookup, watchlist scans, discover hero, similar artist backfill, artist map.', page: 'settings' }, + { title: 'Fix: MusicBrainz artist detail showing MBID as name', desc: 'clicking a MusicBrainz artist from search results was showing the raw MBID as the artist name on the detail page. URL-driven routing (PR #644) no longer passes the display name to the backend, so the source detail endpoint now looks it up directly from MusicBrainz by MBID.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From e7ba5408aa53197b5e64de3b5fdacdeb556ebbff Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 12:53:37 -0700 Subject: [PATCH 10/24] Restore smart back-button label on artist detail page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #644 removed the back-button label logic as collateral when removing the full originStack. The label is independent of the stack β€” restore it without restoring the old click-handler navigation (browser history handles that now). - _artistDetailLabelStack: module-level stack of {type:'page',pageId} or {type:'artist',name} entries, pushed on forward navigation, popped on back - _artistDetailGoingBack flag: set by the back button click handler so navigateToArtistDetail knows to pop instead of push when called by the React route on browser-history navigation - Backfill currentArtistName from the API response so URL-driven entries (which pass '' for name) have real names on state before the next similar- artist navigation pushes them onto the stack - No-history fallback navigates to the recorded origin page --- webui/static/helper.js | 1 + webui/static/library.js | 71 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/webui/static/helper.js b/webui/static/helper.js index 71d57a67..e4be3c99 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3417,6 +3417,7 @@ const WHATS_NEW = { { date: 'May 18, 2026 β€” 2.5.6 release' }, { title: 'MusicBrainz as Primary Metadata Source', desc: 'MusicBrainz is now a full primary metadata source on equal footing with Deezer, iTunes, Spotify, and Discogs. switch to it in Settings β†’ Metadata Source β€” always available, no account or API key needed, rate-limited to 1 req/sec. covers all primary source flows: search, album/track/artist lookup, watchlist scans, discover hero, similar artist backfill, artist map.', page: 'settings' }, { title: 'Fix: MusicBrainz artist detail showing MBID as name', desc: 'clicking a MusicBrainz artist from search results was showing the raw MBID as the artist name on the detail page. URL-driven routing (PR #644) no longer passes the display name to the backend, so the source detail endpoint now looks it up directly from MusicBrainz by MBID.' }, + { title: 'Fix: artist detail back button always showing "← Back"', desc: 'PR #644 removed the back-button label logic along with the origin stack. restored: a label stack (separate from browser history, which handles actual navigation) tracks where you came from across the full similar-artist chain β€” "← Back to Search", "← Back to Artist A", "← Back to Artist B", etc. API response backfills the current artist name so the stack has real names when clicking similar artists.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, diff --git a/webui/static/library.js b/webui/static/library.js index f915d73a..bce5bfbf 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -680,6 +680,26 @@ async function toggleLibraryCardWatchlist(btn, artist) { // =============================================== // Artist detail page state +const _ARTIST_DETAIL_BACK_LABELS = { + library: 'Back to Library', + search: 'Back to Search', + discover: 'Back to Discover', + watchlist: 'Back to Watchlist', + wishlist: 'Back to Wishlist', + stats: 'Back to Stats', + 'playlist-explorer': 'Back to Explorer', + automations: 'Back to Automations', + dashboard: 'Back to Dashboard', + sync: 'Back to Sync', + 'active-downloads': 'Back to Downloads', +}; + +// Stack of origins for the back-button label. Each entry: {type:'page', pageId} +// or {type:'artist', name}. Pushed on forward navigation, popped on back. +// Separate from browser history β€” only used for the label display. +let _artistDetailLabelStack = []; +let _artistDetailGoingBack = false; + let artistDetailPageState = { isInitialized: false, currentArtistId: null, @@ -797,6 +817,23 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt } console.log(`🎡 Navigating to artist detail: ${artistName} (ID: ${artistId}${sourceOverride ? `, source: ${sourceOverride}` : ''})`); + // Maintain the label stack. Back navigations pop; forward navigations push. + if (_artistDetailGoingBack) { + _artistDetailLabelStack.pop(); + _artistDetailGoingBack = false; + } else { + if (currentPage !== 'artist-detail') { + _artistDetailLabelStack = []; // fresh chain from a non-artist page + } + if (currentPage === 'artist-detail' && artistDetailPageState.currentArtistName) { + _artistDetailLabelStack.push({ type: 'artist', name: artistDetailPageState.currentArtistName }); + } else { + const pageId = (typeof currentPage === 'string' && currentPage && currentPage !== 'artist-detail') + ? currentPage : 'library'; + _artistDetailLabelStack.push({ type: 'page', pageId }); + } + } + // Abort any in-progress completion stream if (artistDetailPageState.completionController) { artistDetailPageState.completionController.abort(); @@ -851,10 +888,28 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt initializeArtistDetailPage(); } + _updateArtistDetailBackButtonLabel(); + // Load artist data loadArtistDetailData(artistId, artistName); } +function _updateArtistDetailBackButtonLabel() { + const backBtnLabel = document.querySelector('#artist-detail-back-btn span'); + if (!backBtnLabel) return; + const top = _artistDetailLabelStack[_artistDetailLabelStack.length - 1]; + if (!top) { + backBtnLabel.textContent = '← Back'; + return; + } + if (top.type === 'artist') { + backBtnLabel.textContent = `← Back to ${top.name}`; + } else { + const friendly = _ARTIST_DETAIL_BACK_LABELS[top.pageId] || _ARTIST_DETAIL_BACK_LABELS.library; + backBtnLabel.textContent = `← ${friendly}`; + } +} + function initializeArtistDetailPage() { console.log("πŸ”§ Initializing Artist Detail page..."); @@ -870,12 +925,15 @@ function initializeArtistDetailPage() { } if (window.history.length > 1) { + _artistDetailGoingBack = true; window.history.back(); return; } - // No history β€” default to library - navigateToPage('library'); + // No history β€” fall back to recorded origin page or library + const top = _artistDetailLabelStack.pop(); + _updateArtistDetailBackButtonLabel(); + navigateToPage(top?.type === 'page' ? (top.pageId || 'library') : 'library'); }); } @@ -981,6 +1039,15 @@ async function loadArtistDetailData(artistId, artistName) { artistDetailPageState.currentArtistId = data.artist.id; } + // Backfill name from API response β€” URL-driven navigation passes '' for the + // name so the label stack has real names when the user clicks a similar artist. + if (data.artist?.name && !artistDetailPageState.currentArtistName) { + artistDetailPageState.currentArtistName = data.artist.name; + if (typeof _updateSidebarLibraryBreadcrumb === 'function') { + _updateSidebarLibraryBreadcrumb(); + } + } + // Keep the resolved metadata source for album-track lookups. artistDetailPageState.currentArtistSource = data.discography?.source || data.artist?.source || null; From 1575ba4684570ec2ddf88e856abe79d7949d8780 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 12:55:38 -0700 Subject: [PATCH 11/24] Fix stale _artistDetailGoingBack flag when back lands on non-artist page If history.back() navigated away from artist-detail entirely (e.g. to library), _artistDetailGoingBack stayed true. The next forward artist navigation would then pop the label stack instead of pushing, causing the back-button label to show plain Back instead of the correct page. Guard the pop with currentPage === artist-detail; clear the flag unconditionally in the else branch. --- webui/static/library.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/webui/static/library.js b/webui/static/library.js index bce5bfbf..f8ed0960 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -818,10 +818,13 @@ function navigateToArtistDetail(artistId, artistName, sourceOverride = null, opt console.log(`🎡 Navigating to artist detail: ${artistName} (ID: ${artistId}${sourceOverride ? `, source: ${sourceOverride}` : ''})`); // Maintain the label stack. Back navigations pop; forward navigations push. - if (_artistDetailGoingBack) { + // Only treat the flag as a back-nav signal when we're still on artist-detail β€” + // if history.back() landed on a non-artist page first, the flag is stale. + if (_artistDetailGoingBack && currentPage === 'artist-detail') { _artistDetailLabelStack.pop(); _artistDetailGoingBack = false; } else { + _artistDetailGoingBack = false; // clear any stale flag if (currentPage !== 'artist-detail') { _artistDetailLabelStack = []; // fresh chain from a non-artist page } From 478bcc5d3b1d6d456c8183a4008b3192523616ad Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 14:04:26 -0700 Subject: [PATCH 12/24] fix(amazon): search albums/artists and track numbers for t2tunes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit t2tunes uses HTTP 400 for transient Amazon-side failures instead of 5xx. The first API call in a fresh session hit this every time, so album and artist searches always failed while the track search (called 0.5 s later) got through. - _get_json: retry up to 3 times (1 s, 2 s backoff) on t2tunes-specific 400 "Failed to search" responses - All search_raw calls switched from types="track,album" to types="track" β€” t2tunes album-type queries are currently broken server-side; albums and artists are now derived from track result metadata instead - search_albums: drop is_album filter, extract album fields from track hits - get_album_tracks: fall back to stream index (1-based) when t2tunes tags omit trackNumber, preventing every track landing as track 01 --- core/amazon_client.py | 65 +++++++++++++++++++++++++----------------- webui/static/helper.js | 1 + 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/core/amazon_client.py b/core/amazon_client.py index 8d85e3cd..2414051e 100644 --- a/core/amazon_client.py +++ b/core/amazon_client.py @@ -327,7 +327,7 @@ class AmazonClient: def search_tracks(self, query: str, limit: int = 20) -> List[Track]: _rate_limit() - items = self.search_raw(query, types="track,album") + items = self.search_raw(query, types="track") track_pairs: List[tuple] = [] # (Track, album_asin) seen_album_asins: List[str] = [] for item in items: @@ -360,7 +360,7 @@ class AmazonClient: def search_artists(self, query: str, limit: int = 20) -> List[Artist]: _rate_limit() - items = self.search_raw(query, types="track,album") + items = self.search_raw(query, types="track") seen: Dict[str, Artist] = {} artist_album_asin: Dict[str, str] = {} # artist name β†’ first album ASIN seen for item in items: @@ -384,14 +384,14 @@ class AmazonClient: def search_albums(self, query: str, limit: int = 20) -> List[Album]: _rate_limit() - items = self.search_raw(query, types="track,album") + items = self.search_raw(query, types="track") album_candidates: List[tuple] = [] # (Album, asin) seen_keys: set = set() for item in items: - if not item.is_album: + album_asin = item.album_asin + raw_name = item.album_name + if not raw_name: continue - album_asin = item.album_asin or item.asin - raw_name = item.album_name or item.title display_name = _strip_edition(raw_name) artist = _primary_artist(item.artist_name) # Collapse Explicit/Clean variants: same normalised name + artist = same album @@ -519,7 +519,7 @@ class AmazonClient: artist_name = _primary_artist(streams[0].artist) try: search_items = self.search_raw( - f"{album_name} {artist_name}", types="track,album" + f"{album_name} {artist_name}", types="track" ) for item in search_items: if item.album_asin == asin and item.duration_seconds: @@ -533,12 +533,12 @@ class AmazonClient: "name": _strip_edition(s.title), "artists": [{"name": _primary_artist(s.artist), "id": ""}], "duration_ms": duration_map.get(s.asin, 0), - "track_number": s.track_number, + "track_number": s.track_number if s.track_number is not None else idx + 1, "disc_number": s.disc_number, "release_date": s.date or "", "isrc": s.isrc, } - for s in streams + for idx, s in enumerate(streams) ] return {"items": items, "total": len(items), "limit": 50, "next": None} @@ -547,7 +547,7 @@ class AmazonClient: _rate_limit() search_name = _unslugify(artist_name) try: - items = self.search_raw(search_name, types="track,album") + items = self.search_raw(search_name, types="track") except AmazonClientError: return None name_lower = search_name.lower() @@ -577,7 +577,7 @@ class AmazonClient: _rate_limit() search_name = _unslugify(artist_name) try: - items = self.search_raw(f"{search_name} album", types="track,album") + items = self.search_raw(f"{search_name} album", types="track") except AmazonClientError: return [] album_candidates: List[tuple] = [] # (Album, asin) @@ -630,7 +630,7 @@ class AmazonClient: """Return an album cover as artist image stand-in (T2Tunes has no artist images).""" search_name = _unslugify(artist_id) try: - items = self.search_raw(search_name, types="track,album") + items = self.search_raw(search_name, types="track") except AmazonClientError: return None name_lower = search_name.lower() @@ -686,20 +686,33 @@ class AmazonClient: def _get_json(self, path: str, params: Optional[Dict[str, Any]] = None) -> Any: url = urljoin(f"{self.base_url}/", path.lstrip("/")) - try: - resp = self.session.get(url, params=params, timeout=self.timeout) - resp.raise_for_status() - except requests.HTTPError as exc: - raise AmazonClientError( - f"HTTP {exc.response.status_code} for {url}" - ) from exc - except requests.RequestException as exc: - raise AmazonClientError(f"Request failed for {url}: {exc}") from exc - try: - return resp.json() - except ValueError as exc: - preview = resp.text[:200].replace("\n", " ") - raise AmazonClientError(f"Response not JSON for {url}: {preview!r}") from exc + last_exc: Optional[Exception] = None + for attempt in range(3): + if attempt: + time.sleep(1.0 * attempt) + try: + resp = self.session.get(url, params=params, timeout=self.timeout) + resp.raise_for_status() + except requests.HTTPError as exc: + body = exc.response.text[:500].replace("\n", " ") + # T2Tunes returns 400 for transient Amazon-side failures β€” retry those. + if exc.response.status_code == 400 and "Failed to search" in exc.response.text: + logger.debug("T2Tunes transient 400 on attempt %d, retrying: %s", attempt + 1, url) + last_exc = AmazonClientError( + f"HTTP {exc.response.status_code} for {url} β€” body: {body!r}" + ) + continue + raise AmazonClientError( + f"HTTP {exc.response.status_code} for {url} β€” body: {body!r}" + ) from exc + except requests.RequestException as exc: + raise AmazonClientError(f"Request failed for {url}: {exc}") from exc + try: + return resp.json() + except ValueError as exc: + preview = resp.text[:200].replace("\n", " ") + raise AmazonClientError(f"Response not JSON for {url}: {preview!r}") from exc + raise last_exc # type: ignore[misc] @staticmethod def _iter_search_items(response: Any) -> Iterator[T2TunesSearchItem]: diff --git a/webui/static/helper.js b/webui/static/helper.js index e4be3c99..2dbf6c91 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3418,6 +3418,7 @@ const WHATS_NEW = { { title: 'MusicBrainz as Primary Metadata Source', desc: 'MusicBrainz is now a full primary metadata source on equal footing with Deezer, iTunes, Spotify, and Discogs. switch to it in Settings β†’ Metadata Source β€” always available, no account or API key needed, rate-limited to 1 req/sec. covers all primary source flows: search, album/track/artist lookup, watchlist scans, discover hero, similar artist backfill, artist map.', page: 'settings' }, { title: 'Fix: MusicBrainz artist detail showing MBID as name', desc: 'clicking a MusicBrainz artist from search results was showing the raw MBID as the artist name on the detail page. URL-driven routing (PR #644) no longer passes the display name to the backend, so the source detail endpoint now looks it up directly from MusicBrainz by MBID.' }, { title: 'Fix: artist detail back button always showing "← Back"', desc: 'PR #644 removed the back-button label logic along with the origin stack. restored: a label stack (separate from browser history, which handles actual navigation) tracks where you came from across the full similar-artist chain β€” "← Back to Search", "← Back to Artist A", "← Back to Artist B", etc. API response backfills the current artist name so the stack has real names when clicking similar artists.' }, + { title: 'Fix: Amazon search albums/artists missing, album downloads all track 01', desc: 't2tunes proxies Amazon Music and uses 400 to signal transient failures β€” first API call in a session hit this consistently, so album/artist searches always failed while track search (called 0.5s later) scraped through. added up to 3 retries with backoff on t2tunes-specific 400s. also: all search methods were using types=track,album but t2tunes album-type queries are broken β€” switched everything to types=track and derive albums/artists from track metadata instead. track numbers from album downloads were also always 1 β€” added index-based fallback when t2tunes tags omit trackNumber.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From 43ed30b4d2bbeca84a6ec872832cf12abbffa35b Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 15:38:24 -0700 Subject: [PATCH 13/24] fix(musicbrainz): user-facing search recall + album-detail 404 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs surfacing on the Fix popup and enhanced-search MB tab: 1. Strict Lucene phrase queries (`recording:"X" AND artist:"Y"`) killed recall on user-facing manual search β€” diacritics ("Bjork" vs canonical "BjΓΆrk"), bracketed suffixes like "(Live)", and any AND-clause mismatch returned zero results. Added `strict: bool = True` param to `search_release` / `search_recording`; when False, sends a bare query joining title + artist so MB hits alias/sortname indexes with diacritic folding. `/api/musicbrainz/search` (Fix popup) and `core/library/service_search.py` (service tabs) now pass strict=False. Enrichment workers stay on strict mode β€” precision matters there because they auto-accept the top hit above a confidence threshold. 2. Every MB album click was silently 404-ing β€” `_render_release_as_album` passed `cover-art-archive` as an MB `inc` param, but it's not a valid include for the /release resource (MB rejects with 400). The CAA flags come back on every release response by default, so dropping the bad include preserves the image-scope picker logic intact. --- core/library/service_search.py | 8 ++- core/musicbrainz_client.py | 83 ++++++++++++++++------- core/musicbrainz_search.py | 7 +- tests/metadata/test_musicbrainz_search.py | 2 +- web_server.py | 12 +++- webui/static/helper.js | 2 + 6 files changed, 82 insertions(+), 32 deletions(-) diff --git a/core/library/service_search.py b/core/library/service_search.py index cb8b1874..38fd450e 100644 --- a/core/library/service_search.py +++ b/core/library/service_search.py @@ -100,12 +100,14 @@ def _search_service(service, entity_type, query): if not mb_worker or not mb_worker.mb_service: raise ValueError("MusicBrainz worker not initialized") mb_client = mb_worker.mb_service.mb_client + # User-facing manual search β€” prefer recall (fuzzy / alias / diacritic- + # folded) over strict phrase precision. User picks correct hit from list. if entity_type == 'artist': - items = mb_client.search_artist(query, limit=8) + items = mb_client.search_artist(query, limit=8, strict=False) return [{'id': a['id'], 'name': a.get('name', ''), 'image': None, 'extra': f"Score: {a.get('score', '')} Β· {a.get('disambiguation', '') or a.get('country', '')}"} for a in items] elif entity_type == 'album': - items = mb_client.search_release(query, limit=8) + items = mb_client.search_release(query, limit=8, strict=False) results = [] for r in items: artists = ', '.join(ac.get('name', '') for ac in r.get('artist-credit', []) if isinstance(ac, dict)) @@ -115,7 +117,7 @@ def _search_service(service, entity_type, query): 'extra': f"{artists} Β· {r.get('date', '')} Β· Score: {r.get('score', '')}"}) return results elif entity_type == 'track': - items = mb_client.search_recording(query, limit=8) + items = mb_client.search_recording(query, limit=8, strict=False) results = [] for r in items: artists = ', '.join(ac.get('name', '') for ac in r.get('artist-credit', []) if isinstance(ac, dict)) diff --git a/core/musicbrainz_client.py b/core/musicbrainz_client.py index b152f28f..f1d59491 100644 --- a/core/musicbrainz_client.py +++ b/core/musicbrainz_client.py @@ -128,27 +128,45 @@ class MusicBrainzClient: return [] @rate_limited - def search_release(self, album_name: str, artist_name: Optional[str] = None, limit: int = 10) -> List[Dict[str, Any]]: + def search_release(self, album_name: str, artist_name: Optional[str] = None, + limit: int = 10, strict: bool = True) -> List[Dict[str, Any]]: """ - Search for releases (albums) by name - + Search for releases (albums) by name. + Args: album_name: Name of the album to search for artist_name: Optional artist name to narrow search limit: Maximum number of results to return - + strict: When True (default), builds a phrase-match Lucene query + against the `release` and `artist` fields β€” correct for + enrichment flows where exact name+artist are known. When + False, sends a bare query (album + artist joined) so MB + hits alias / sortname indexes and folds diacritics, + dramatically improving recall for user-facing fuzzy + lookups (e.g. the manual Fix popup). + Returns: List of release results """ try: - # Escape quotes and backslashes for Lucene query - safe_album = album_name.replace('\\', '\\\\').replace('"', '\\"') - query = f'release:"{safe_album}"' - - if artist_name: - safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') - query += f' AND artist:"{safe_artist}"' - + if strict: + # Escape quotes and backslashes for Lucene query + safe_album = album_name.replace('\\', '\\\\').replace('"', '\\"') + query = f'release:"{safe_album}"' + + if artist_name: + safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') + query += f' AND artist:"{safe_artist}"' + else: + # Bare query β€” MB tokenizes against title + artist credit + + # alias + sortname indexes together with diacritic folding. + # Recovers cases like "Bjork" β†’ "BjΓΆrk" that strict phrase + # queries miss. + parts = [album_name] + if artist_name: + parts.append(artist_name) + query = ' '.join(p for p in parts if p) + params = { 'query': query, 'fmt': 'json', @@ -173,27 +191,44 @@ class MusicBrainzClient: return [] @rate_limited - def search_recording(self, track_name: str, artist_name: Optional[str] = None, limit: int = 10) -> List[Dict[str, Any]]: + def search_recording(self, track_name: str, artist_name: Optional[str] = None, + limit: int = 10, strict: bool = True) -> List[Dict[str, Any]]: """ - Search for recordings (tracks) by name - + Search for recordings (tracks) by name. + Args: track_name: Name of the track to search for artist_name: Optional artist name to narrow search limit: Maximum number of results to return - + strict: When True (default), builds a phrase-match Lucene query + against the `recording` and `artist` fields β€” correct for + enrichment flows where exact name+artist are known. When + False, sends a bare query (track + artist joined) so MB + hits alias / sortname indexes and folds diacritics. The + bare path also avoids the AND-clause that kills recall + when either side mis-matches (e.g. "Bjork" vs canonical + "BjΓΆrk", or a track title with bracketed suffix like + "(Live)" that strict phrase match rejects). + Returns: List of recording results """ try: - # Escape quotes and backslashes for Lucene query - safe_track = track_name.replace('\\', '\\\\').replace('"', '\\"') - query = f'recording:"{safe_track}"' - - if artist_name: - safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') - query += f' AND artist:"{safe_artist}"' - + if strict: + # Escape quotes and backslashes for Lucene query + safe_track = track_name.replace('\\', '\\\\').replace('"', '\\"') + query = f'recording:"{safe_track}"' + + if artist_name: + safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') + query += f' AND artist:"{safe_artist}"' + else: + # Bare query β€” see search_release for rationale. + parts = [track_name] + if artist_name: + parts.append(artist_name) + query = ' '.join(p for p in parts if p) + params = { 'query': query, 'fmt': 'json', diff --git a/core/musicbrainz_search.py b/core/musicbrainz_search.py index 21001118..4ce82e87 100644 --- a/core/musicbrainz_search.py +++ b/core/musicbrainz_search.py @@ -856,8 +856,13 @@ class MusicBrainzSearchClient: shape the download modal expects. `rg_fallback` supplies release-group metadata (type, artist credits) when resolving from a release-group whose releases may be lightly populated.""" + # NOTE: `cover-art-archive` is NOT a valid `inc` param for the + # /release resource β€” MB returns 400 if you pass it. The CAA flags + # (`{'front': True, 'back': True, ...}`) come back on every release + # response by default, so we read them below without requesting an + # include. release = self._client.get_release( - release_mbid, includes=['recordings', 'artist-credits', 'release-groups', 'cover-art-archive'] + release_mbid, includes=['recordings', 'artist-credits', 'release-groups'] ) if not release: return None diff --git a/tests/metadata/test_musicbrainz_search.py b/tests/metadata/test_musicbrainz_search.py index 4269a5f8..719f02ae 100644 --- a/tests/metadata/test_musicbrainz_search.py +++ b/tests/metadata/test_musicbrainz_search.py @@ -541,7 +541,7 @@ def test_get_album_resolves_release_group_mbid_to_release(): 'rg-damn', includes=['releases', 'artist-credits'] ) client._client.get_release.assert_called_once_with( - 'rel-official', includes=['recordings', 'artist-credits', 'release-groups', 'cover-art-archive'] + 'rel-official', includes=['recordings', 'artist-credits', 'release-groups'] ) assert album is not None assert album['id'] == 'rg-damn' # Canonical ID stays the release-group MBID. diff --git a/web_server.py b/web_server.py index 695b9a57..46bdd39c 100644 --- a/web_server.py +++ b/web_server.py @@ -15734,8 +15734,14 @@ def musicbrainz_search_api(): mb_client = mb_svc.mb_client results = [] + # Manual Fix popup is user-facing fuzzy search β€” recall matters more + # than precision because the user picks the right hit from the list. + # Use bare-query mode so diacritics, aliases, and bracketed suffixes + # like "(Live)" don't kill matches the way strict field-scoped + # phrase queries do. Enrichment workers stay on strict mode (the + # default) since they auto-accept the top hit and need precision. if entity_type == 'artist': - raw = mb_client.search_artist(query, limit=limit) + raw = mb_client.search_artist(query, limit=limit, strict=False) for r in raw: results.append({ 'mbid': r.get('id', ''), @@ -15746,7 +15752,7 @@ def musicbrainz_search_api(): 'country': r.get('country', ''), }) elif entity_type == 'release': - raw = mb_client.search_release(query, artist_name=artist or None, limit=limit) + raw = mb_client.search_release(query, artist_name=artist or None, limit=limit, strict=False) for r in raw: artist_credit = ', '.join(a.get('name', '') for a in r.get('artist-credit', []) if isinstance(a, dict)) results.append({ @@ -15760,7 +15766,7 @@ def musicbrainz_search_api(): 'track_count': r.get('track-count', 0), }) elif entity_type == 'recording': - raw = mb_client.search_recording(query, artist_name=artist or None, limit=limit) + raw = mb_client.search_recording(query, artist_name=artist or None, limit=limit, strict=False) for r in raw: artist_credit = ', '.join(a.get('name', '') for a in r.get('artist-credit', []) if isinstance(a, dict)) releases = r.get('releases', []) diff --git a/webui/static/helper.js b/webui/static/helper.js index 2dbf6c91..40c74509 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3419,6 +3419,8 @@ const WHATS_NEW = { { title: 'Fix: MusicBrainz artist detail showing MBID as name', desc: 'clicking a MusicBrainz artist from search results was showing the raw MBID as the artist name on the detail page. URL-driven routing (PR #644) no longer passes the display name to the backend, so the source detail endpoint now looks it up directly from MusicBrainz by MBID.' }, { title: 'Fix: artist detail back button always showing "← Back"', desc: 'PR #644 removed the back-button label logic along with the origin stack. restored: a label stack (separate from browser history, which handles actual navigation) tracks where you came from across the full similar-artist chain β€” "← Back to Search", "← Back to Artist A", "← Back to Artist B", etc. API response backfills the current artist name so the stack has real names when clicking similar artists.' }, { title: 'Fix: Amazon search albums/artists missing, album downloads all track 01', desc: 't2tunes proxies Amazon Music and uses 400 to signal transient failures β€” first API call in a session hit this consistently, so album/artist searches always failed while track search (called 0.5s later) scraped through. added up to 3 retries with backoff on t2tunes-specific 400s. also: all search methods were using types=track,album but t2tunes album-type queries are broken β€” switched everything to types=track and derive albums/artists from track metadata instead. track numbers from album downloads were also always 1 β€” added index-based fallback when t2tunes tags omit trackNumber.' }, + { title: 'Fix: MusicBrainz manual search missing results', desc: 'the Fix popup and manual library service search were using strict Lucene phrase-match queries against the `recording` / `release` / `artist` fields β€” diacritics ("Bjork" vs canonical "BjΓΆrk"), bracketed suffixes like "(Live)", and any AND-clause mismatch all killed recall. switched user-facing manual lookups to bare queries that hit MB\'s alias / sortname indexes with diacritic folding. enrichment workers stay strict for precision.' }, + { title: 'Fix: MusicBrainz album clicks 404ing in enhanced search', desc: 'every click on a MusicBrainz album result was silently 404-ing β€” the /release fetch was passing `cover-art-archive` as an `inc` param, which MB rejects with 400 (that field is returned on every release response by default, no include needed). dropped the bad include; album detail now loads correctly.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From 036faff8b186a729820c2b9b40706c0ccec5dbb7 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 17:03:19 -0700 Subject: [PATCH 14/24] feat(fix-popup): paste MusicBrainz URL/MBID to match directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Power-user escape hatch on the Discovery Fix Track Match modal β€” when fuzzy auto-search ranks the wrong recording among many same-title versions (10 remasters, live cuts, alt sessions), paste the MusicBrainz recording URL or bare UUID into the new field and resolve straight to that record. Layout: - Shape adapter `get_recording_flat(mbid)` lives in `core/musicbrainz_search.py` next to existing `get_track_details`. Returns the flat Fix-popup track shape (artists as `string[]`, album as string, single `image_url`) β€” distinct from the Spotify-shaped nested dict `get_track_details` returns. - New route `GET /api/musicbrainz/recording/` is a thin wrapper: validates MBID format with an anchored UUID regex, calls the adapter, returns 400 / 404 / 200 with no inline shape massaging. - Frontend `parseMusicBrainzMbid()` lives in `shared-helpers.js` β€” pure URL/UUID parser, reusable from other surfaces (failed-MB cache, manual match) without duplication. - Fix modal HTML gets one new input row + button; existing search row and result render pipeline are untouched. New `lookupDiscoveryFixByMbid()` fetches the endpoint and feeds the single result through the existing `renderDiscoveryFixResults` -> confirm-dialog -> match pipeline, so MB- paste matches go through the exact same selection flow as auto-search results. - Enter-key bound on the MBID input via a separate handler ref so its lifecycle matches the search-input handlers without conflating the two submit targets. 7 unit tests cover the adapter: happy path, empty/None MBID, MB returns None, recording-without-release (empty album), multi-artist credits, includes-list contract, and client-error swallow. Out of scope: the Fix popup's fuzzy cascade is still hardcoded to spotify/deezer/itunes regardless of which primary source the user has configured. Adding MB to that cascade (when MB is the active primary) is a separate concern. --- core/musicbrainz_search.py | 51 +++++++++ tests/metadata/test_musicbrainz_search.py | 122 ++++++++++++++++++++++ web_server.py | 27 +++++ webui/static/helper.js | 1 + webui/static/shared-helpers.js | 28 +++++ webui/static/sync-services.js | 13 +++ webui/static/wishlist-tools.js | 84 +++++++++++++++ 7 files changed, 326 insertions(+) diff --git a/core/musicbrainz_search.py b/core/musicbrainz_search.py index 4ce82e87..ff05be13 100644 --- a/core/musicbrainz_search.py +++ b/core/musicbrainz_search.py @@ -725,6 +725,57 @@ class MusicBrainzSearchClient: logger.error(f'get_track_details({track_id}) error: {e}') return None + def get_recording_flat(self, mbid: str) -> Optional[Dict[str, Any]]: + """Return a Fix-popup-compatible flat track dict by recording MBID. + + Distinct from `get_track_details` which returns a Spotify-shaped + nested dict (artists as objects, album as nested object with + images array). The Discovery Fix popup expects the flat shape that + the spotify/deezer/itunes search endpoints produce β€” artists as a + list of strings, album as a string, single image_url field. + + Used by `GET /api/musicbrainz/recording/` to support the + MBID-paste lookup field β€” power-user escape hatch when fuzzy auto- + search ranks the wrong recording among many same-title versions. + + Returns None when the MBID is missing or MB returns no recording. + Recording-without-release is valid (album = '', image_url = ''). + """ + if not mbid: + return None + try: + rec = self._client.get_recording( + mbid, includes=['releases', 'artist-credits', 'release-groups'] + ) + if not rec: + return None + + releases = rec.get('releases', []) or [] + releases.sort(key=self._release_preference_key) + first_rel = releases[0] if releases else {} + rg = first_rel.get('release-group', {}) or {} + release_id = first_rel.get('id', '') + rg_id = rg.get('id', '') + + artists = _extract_artist_credit(rec.get('artist-credit', [])) + album_name = first_rel.get('title', '') or '' + image_url = self._cached_art(release_id, rg_id) if (release_id or rg_id) else None + + return { + 'id': rec.get('id', '') or mbid, + 'name': rec.get('title', '') or '', + 'artists': artists if artists else [], + 'album': album_name, + 'duration_ms': rec.get('length') or 0, + 'image_url': image_url or '', + 'external_urls': { + 'musicbrainz': f'https://musicbrainz.org/recording/{mbid}' + }, + } + except Exception as e: + logger.error(f'get_recording_flat({mbid}) error: {e}') + return None + def get_album_tracks(self, album_mbid: str) -> Optional[Dict[str, Any]]: """Return {items: [...], total: N} track listing for a release/release-group MBID.""" album = self.get_album(album_mbid, include_tracks=True) diff --git a/tests/metadata/test_musicbrainz_search.py b/tests/metadata/test_musicbrainz_search.py index 719f02ae..350a52a4 100644 --- a/tests/metadata/test_musicbrainz_search.py +++ b/tests/metadata/test_musicbrainz_search.py @@ -765,3 +765,125 @@ def test_search_tracks_text_path_filters_by_score(): titles = [t.name for t in tracks] assert 'Good' in titles assert 'Bad' not in titles + + +# --------------------------------------------------------------------------- +# get_recording_flat β€” Fix-popup MBID paste adapter +# --------------------------------------------------------------------------- + +def test_get_recording_flat_happy_path(): + """Recording with a release returns flat shape with album + image.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = { + 'id': 'rec-abc', + 'title': 'Army of Me', + 'length': 234000, + 'artist-credit': [{'artist': {'name': 'BjΓΆrk'}}], + 'releases': [{ + 'id': 'rel-xyz', + 'title': 'Post', + 'date': '1995-06-13', + 'status': 'Official', + 'media': [{'track-count': 11}], + 'release-group': {'id': 'rg-post', 'primary-type': 'Album', 'secondary-types': []}, + }], + } + + track = client.get_recording_flat('rec-abc') + + assert track is not None + assert track['id'] == 'rec-abc' + assert track['name'] == 'Army of Me' + assert track['artists'] == ['BjΓΆrk'] # flat list of strings, not Spotify-shaped objects + assert track['album'] == 'Post' # flat string, not nested dict + assert track['duration_ms'] == 234000 + assert track['image_url'] # CAA URL present + assert 'musicbrainz.org/recording/rec-abc' in track['external_urls']['musicbrainz'] + + +def test_get_recording_flat_missing_mbid_returns_none(): + """No MBID β†’ no API call, returns None.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + + assert client.get_recording_flat('') is None + assert client.get_recording_flat(None) is None + client._client.get_recording.assert_not_called() + + +def test_get_recording_flat_mb_returns_no_recording(): + """MB returns None (404 / missing) β†’ adapter returns None.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = None + + assert client.get_recording_flat('rec-missing') is None + + +def test_get_recording_flat_recording_without_release(): + """Standalone recording (no releases) β€” album stays empty, + image_url empty, but the rest of the shape is intact.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = { + 'id': 'rec-standalone', + 'title': 'Untitled Demo', + 'length': 120000, + 'artist-credit': [{'artist': {'name': 'Unknown'}}], + 'releases': [], + } + + track = client.get_recording_flat('rec-standalone') + + assert track is not None + assert track['name'] == 'Untitled Demo' + assert track['album'] == '' + assert track['image_url'] == '' + assert track['artists'] == ['Unknown'] + assert track['duration_ms'] == 120000 + + +def test_get_recording_flat_multi_artist_credit(): + """Recording with multiple credited artists β€” all flatten to list of strings.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = { + 'id': 'rec-collab', + 'title': 'Collab Track', + 'length': 180000, + 'artist-credit': [ + {'artist': {'name': 'Artist A'}}, + {'artist': {'name': 'Artist B'}}, + ], + 'releases': [], + } + + track = client.get_recording_flat('rec-collab') + + assert track['artists'] == ['Artist A', 'Artist B'] + + +def test_get_recording_flat_includes_match_get_track_details(): + """Sanity: passes the same includes list so the API call is cacheable + against the same key as get_track_details (one network request can + serve both surfaces if MB ever adds response caching upstream).""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.return_value = None + + client.get_recording_flat('rec-x') + + client._client.get_recording.assert_called_once_with( + 'rec-x', includes=['releases', 'artist-credits', 'release-groups'] + ) + + +def test_get_recording_flat_swallows_client_errors(): + """MB client raising must not propagate to the route handler β€” return + None so the endpoint can render a friendly 404 instead of 500.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_recording.side_effect = RuntimeError('boom') + + assert client.get_recording_flat('rec-err') is None diff --git a/web_server.py b/web_server.py index 46bdd39c..347e938c 100644 --- a/web_server.py +++ b/web_server.py @@ -15789,6 +15789,33 @@ def musicbrainz_search_api(): return jsonify({"error": str(e)}), 500 +# Recording MBID format: standard UUID, 8-4-4-4-12 hex. +_MB_RECORDING_MBID_RE = re.compile( + r'^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$', re.IGNORECASE +) + + +@app.route('/api/musicbrainz/recording/', methods=['GET']) +def musicbrainz_recording_lookup_api(mbid): + """Look up a single MusicBrainz recording by MBID and return it in the + Fix-popup-compatible flat track shape. Powers the MBID-paste field β€” + user-facing escape hatch when fuzzy auto-search ranks the wrong + recording among many same-title versions.""" + mbid = (mbid or '').strip().lower() + if not _MB_RECORDING_MBID_RE.match(mbid): + return jsonify({"error": "Invalid MusicBrainz recording MBID"}), 400 + try: + from core.musicbrainz_search import MusicBrainzSearchClient + mb_search = MusicBrainzSearchClient() + track = mb_search.get_recording_flat(mbid) + if not track: + return jsonify({"error": "Recording not found on MusicBrainz"}), 404 + return jsonify(track) + except Exception as e: + logger.error(f"Error looking up MB recording {mbid}: {e}") + return jsonify({"error": str(e)}), 500 + + @app.route('/api/metadata-cache/mb-match', methods=['POST']) def metadata_cache_save_mb_match(): """Save a manual MusicBrainz match for a failed lookup.""" diff --git a/webui/static/helper.js b/webui/static/helper.js index 40c74509..0b581183 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3421,6 +3421,7 @@ const WHATS_NEW = { { title: 'Fix: Amazon search albums/artists missing, album downloads all track 01', desc: 't2tunes proxies Amazon Music and uses 400 to signal transient failures β€” first API call in a session hit this consistently, so album/artist searches always failed while track search (called 0.5s later) scraped through. added up to 3 retries with backoff on t2tunes-specific 400s. also: all search methods were using types=track,album but t2tunes album-type queries are broken β€” switched everything to types=track and derive albums/artists from track metadata instead. track numbers from album downloads were also always 1 β€” added index-based fallback when t2tunes tags omit trackNumber.' }, { title: 'Fix: MusicBrainz manual search missing results', desc: 'the Fix popup and manual library service search were using strict Lucene phrase-match queries against the `recording` / `release` / `artist` fields β€” diacritics ("Bjork" vs canonical "BjΓΆrk"), bracketed suffixes like "(Live)", and any AND-clause mismatch all killed recall. switched user-facing manual lookups to bare queries that hit MB\'s alias / sortname indexes with diacritic folding. enrichment workers stay strict for precision.' }, { title: 'Fix: MusicBrainz album clicks 404ing in enhanced search', desc: 'every click on a MusicBrainz album result was silently 404-ing β€” the /release fetch was passing `cover-art-archive` as an `inc` param, which MB rejects with 400 (that field is returned on every release response by default, no include needed). dropped the bad include; album detail now loads correctly.' }, + { title: 'Fix popup: paste a MusicBrainz URL or MBID to match directly', desc: 'new escape hatch on the Fix Track Match modal (the πŸ”§ Fix button on mirrored / YouTube / Tidal / Deezer / Beatport / ListenBrainz / Spotify-public discovery rows). when fuzzy search keeps ranking the wrong recording among many same-title versions, paste the MusicBrainz recording URL like `https://musicbrainz.org/recording/` or the bare UUID into the new field and hit "Look up". skips all fuzzy logic, resolves straight to that record, and runs it through the same confirm + match pipeline.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, diff --git a/webui/static/shared-helpers.js b/webui/static/shared-helpers.js index e5aeb0bc..9840fea3 100644 --- a/webui/static/shared-helpers.js +++ b/webui/static/shared-helpers.js @@ -3968,3 +3968,31 @@ function showCompletionError() { overlay.title = 'Failed to check completion status'; }); } + + +// ---------------------------------------------------------------------------- +// MusicBrainz MBID parsing β€” accept full URLs or bare UUIDs. +// Used by the Discovery Fix popup's MBID-paste lookup; reusable anywhere the +// user can paste a MusicBrainz identifier (failed-MB cache, manual match, +// future surfaces). Returns the bare lowercase UUID when valid, or null. +// ---------------------------------------------------------------------------- + +const _MB_UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +function parseMusicBrainzMbid(input) { + if (!input || typeof input !== 'string') return null; + let candidate = input.trim(); + if (!candidate) return null; + + // Strip a musicbrainz.org URL down to the final UUID segment. Accepts + // /recording/, /release/, /release-group/, /artist/ β€” the caller decides + // which entity type is appropriate for its endpoint. + const urlMatch = candidate.match(/musicbrainz\.org\/[a-z-]+\/([0-9a-f-]+)/i); + if (urlMatch) candidate = urlMatch[1]; + + // Strip trailing query / fragment that survived a copy-paste from a URL + // (e.g. "?utm=...", "#tab"). + candidate = candidate.split(/[?#]/)[0].trim().toLowerCase(); + + return _MB_UUID_RE.test(candidate) ? candidate : null; +} diff --git a/webui/static/sync-services.js b/webui/static/sync-services.js index 8eeececb..11b50933 100644 --- a/webui/static/sync-services.js +++ b/webui/static/sync-services.js @@ -7414,6 +7414,19 @@ function openYouTubeDiscoveryModal(urlHash) { πŸ” Search
+ + +
+ + +
diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index ee11148f..ad705785 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -12,6 +12,9 @@ let currentDiscoveryFix = { // Store event handler reference to allow proper removal let discoveryFixEnterHandler = null; +// Separate handler for the MBID-paste input β€” targets lookup-by-MBID +// instead of fuzzy search so Enter does the obvious right thing per field. +let discoveryFixMbidEnterHandler = null; /** * Open discovery fix modal for a specific track @@ -113,11 +116,20 @@ function openDiscoveryFixModal(platform, identifier, trackIndex) { artist: artistInput.value }); + // MBID input β€” separate ref because its Enter binding targets a + // different function (direct lookup vs fuzzy search). Optional: older + // modal markup that doesn't have the MBID row will get null here. + const mbidInput = fixModalOverlay.querySelector('#fix-modal-mbid-input'); + if (mbidInput) mbidInput.value = ''; + // Remove old enter key handler if exists if (discoveryFixEnterHandler) { trackInput.removeEventListener('keypress', discoveryFixEnterHandler); artistInput.removeEventListener('keypress', discoveryFixEnterHandler); } + if (discoveryFixMbidEnterHandler && mbidInput) { + mbidInput.removeEventListener('keypress', discoveryFixMbidEnterHandler); + } // Add new enter key handler discoveryFixEnterHandler = function (e) { @@ -126,6 +138,13 @@ function openDiscoveryFixModal(platform, identifier, trackIndex) { trackInput.addEventListener('keypress', discoveryFixEnterHandler); artistInput.addEventListener('keypress', discoveryFixEnterHandler); + if (mbidInput) { + discoveryFixMbidEnterHandler = function (e) { + if (e.key === 'Enter') lookupDiscoveryFixByMbid(); + }; + mbidInput.addEventListener('keypress', discoveryFixMbidEnterHandler); + } + // Show modal BEFORE auto-search so elements are visible fixModalOverlay.classList.remove('hidden'); console.log('βœ… Fix modal opened, starting auto-search...'); @@ -238,6 +257,71 @@ async function searchDiscoveryFix() { } } +/** + * Look up a track directly by MusicBrainz recording MBID β€” bypasses fuzzy + * search entirely. Escape hatch for cases where the user knows the exact + * record (e.g. there are 10 same-title recordings from different sessions + * and auto-search keeps ranking the wrong one). Accepts full URLs + * (`https://musicbrainz.org/recording/`) or bare UUIDs. + */ +async function lookupDiscoveryFixByMbid() { + if (!currentDiscoveryFix.identifier) { + console.error('No active fix modal context'); + return; + } + + const discoveryModal = document.getElementById(`youtube-discovery-modal-${currentDiscoveryFix.identifier}`); + if (!discoveryModal) return; + const fixModalOverlay = discoveryModal.querySelector('.discovery-fix-modal-overlay'); + if (!fixModalOverlay) return; + + const mbidInput = fixModalOverlay.querySelector('#fix-modal-mbid-input'); + if (!mbidInput) return; + + const mbid = parseMusicBrainzMbid(mbidInput.value); + const resultsContainer = fixModalOverlay.querySelector('#fix-modal-results'); + + if (!mbid) { + if (resultsContainer) { + resultsContainer.innerHTML = '
❌ Not a valid MusicBrainz recording URL or MBID. Paste a URL like https://musicbrainz.org/recording/<uuid> or the bare UUID.
'; + } + showToast('Invalid MusicBrainz MBID', 'error'); + return; + } + + if (resultsContainer) { + resultsContainer.innerHTML = '
πŸ”— Looking up MusicBrainz recording...
'; + } + + try { + const response = await fetch(`/api/musicbrainz/recording/${encodeURIComponent(mbid)}`); + if (response.status === 404) { + if (resultsContainer) { + resultsContainer.innerHTML = '
Recording not found on MusicBrainz. Double-check the MBID.
'; + } + return; + } + if (!response.ok) { + throw new Error(`HTTP ${response.status}`); + } + const track = await response.json(); + if (!track || !track.id) { + if (resultsContainer) { + resultsContainer.innerHTML = '
Recording not found on MusicBrainz.
'; + } + return; + } + // Render as a single-result list β€” user still clicks to confirm, + // matching the existing search-result flow exactly. + renderDiscoveryFixResults([track], fixModalOverlay); + } catch (error) { + console.error('MBID lookup error:', error); + if (resultsContainer) { + resultsContainer.innerHTML = '
❌ MBID lookup failed. Try again.
'; + } + } +} + /** * Render search results as clickable cards */ From da415a4a7e95dd8cc5d86357b37c7b3477c9675d Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 18:22:56 -0700 Subject: [PATCH 15/24] test(amazon): update search_albums test for derived-from-tracks behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 478bcc5d (`fix(amazon): search albums/artists and track numbers for t2tunes`) switched `search_albums` to query `types=track` and derive Album objects from the album metadata on each track hit β€” Amazon's album-type query is broken upstream. The matching test was left asserting the old "filter out track hits β†’ return []" behavior and has been failing in CI ever since. Rewritten to assert the current intended behavior: track hits yield distinct albums by album ASIN, with the artist credit + name preserved. No code change. --- tests/tools/test_amazon_client.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_amazon_client.py b/tests/tools/test_amazon_client.py index a2e21938..33fbac08 100644 --- a/tests/tools/test_amazon_client.py +++ b/tests/tools/test_amazon_client.py @@ -624,11 +624,20 @@ class TestSearchAlbums: albums = client.search_albums("GNX") assert len(albums) == 1 - def test_ignores_track_hits(self): + def test_derives_albums_from_track_hits(self): + """search_albums now intentionally queries `types=track` and derives + Album objects from the album metadata carried on each track hit β€” + Amazon's album-type query is broken upstream, so the t2tunes fix + switched everything to track-type and reconstructs albums from the + results. Distinct album ASINs across the track hits yield distinct + albums; duplicates collapse via the explicit/clean dedup key.""" client = _make_client({"amazon-music/search": SEARCH_RESPONSE_TRACKS}) with patch("core.amazon_client._rate_limit"): albums = client.search_albums("Kendrick") - assert albums == [] + # Two track hits β†’ two distinct album ASINs β†’ two derived albums. + assert {a.id for a in albums} == {"B0ABCDE123", "B0ABCDE456"} + assert {a.name for a in albums} == {"GNX", "euphoria"} + assert all(a.artists == ["Kendrick Lamar"] for a in albums) def test_strips_explicit_from_album_name(self): resp = { From 97f35de44e3067541a6c1564ee5aaa1214fbd2a6 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 18:22:56 -0700 Subject: [PATCH 16/24] test(amazon): update search_albums test for derived-from-tracks behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 478bcc5d (`fix(amazon): search albums/artists and track numbers for t2tunes`) switched `search_albums` to query `types=track` and derive Album objects from the album metadata on each track hit β€” Amazon's album-type query is broken upstream. The matching test was left asserting the old "filter out track hits β†’ return []" behavior and has been failing in CI ever since. Rewritten to assert the current intended behavior: track hits yield distinct albums by album ASIN, with the artist credit + name preserved. No code change. --- tests/tools/test_amazon_client.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_amazon_client.py b/tests/tools/test_amazon_client.py index a2e21938..33fbac08 100644 --- a/tests/tools/test_amazon_client.py +++ b/tests/tools/test_amazon_client.py @@ -624,11 +624,20 @@ class TestSearchAlbums: albums = client.search_albums("GNX") assert len(albums) == 1 - def test_ignores_track_hits(self): + def test_derives_albums_from_track_hits(self): + """search_albums now intentionally queries `types=track` and derives + Album objects from the album metadata carried on each track hit β€” + Amazon's album-type query is broken upstream, so the t2tunes fix + switched everything to track-type and reconstructs albums from the + results. Distinct album ASINs across the track hits yield distinct + albums; duplicates collapse via the explicit/clean dedup key.""" client = _make_client({"amazon-music/search": SEARCH_RESPONSE_TRACKS}) with patch("core.amazon_client._rate_limit"): albums = client.search_albums("Kendrick") - assert albums == [] + # Two track hits β†’ two distinct album ASINs β†’ two derived albums. + assert {a.id for a in albums} == {"B0ABCDE123", "B0ABCDE456"} + assert {a.name for a in albums} == {"GNX", "euphoria"} + assert all(a.artists == ["Kendrick Lamar"] for a in albums) def test_strips_explicit_from_album_name(self): resp = { From daf9a527d998169ae1937510b688805ddd60f626 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 19:06:41 -0700 Subject: [PATCH 17/24] feat(fix-popup): include MusicBrainz in the auto-search cascade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Fix Track Match modal's auto-search was hardcoded to query only Spotify -> Deezer -> iTunes, ignoring MusicBrainz entirely β€” even for users with MB set as their primary metadata source. MB-niche recordings (canonical entries with diacritics, fringe / non-mainstream tracks that the commercial catalogues don't carry) had no chance. Wiring: - New `MusicBrainzSearchClient.search_tracks_with_artist(track, artist, limit)` for surfaces that already have title + artist split. Uses MB's bare-query mode (strict=False) β€” diacritic-folded, alias/sortname indexed β€” same recall rationale as the earlier MBID-paste endpoint. - New route `GET /api/musicbrainz/search_tracks` mirrors the existing /api/{spotify,itunes,deezer}/search_tracks endpoints exactly: accepts `track`+`artist` (or legacy `query`) + `limit`, returns `{tracks: [{id, name, artists, album, duration_ms, image_url, source}]}`. Applies the same `core.metadata.relevance.rerank_tracks` pass Deezer / iTunes use, which is critical because MB's free-text scoring weighs title-text matches heavily and would otherwise rank cover / tribute recordings above the canonical version. - `_search_tracks_text` gains a `min_score` parameter. The cascade path passes 20 (vs the enhanced-search-tab default of 80) so MB recordings whose title doesn't literally contain the artist name still enter the candidate pool β€” without that, "Army of Me" + "Bjork" only surfaces the HIRS Collective cover (score 100) and drops BjΓΆrk's canonical recording (score 28). The rerank pass then surfaces BjΓΆrk by artist match. Verified against real MB API: pre-fix returned only the cover; post-fix top 5 are all BjΓΆrk. - Fix popup `allSources` array (wishlist-tools.js) gets MB appended. The existing `activeIdx` reorder logic moves MB to the front when it's the active primary; otherwise MB sits last (1 req/sec rate limit makes it the slowest source). 7 new unit tests on the adapter: bare-query mode is used, missing artist falls back to None (drops AND-clause), empty inputs short-circuit, low-score candidates are kept for rerank to handle, default strict + default min_score behaviour preserved for the existing search-tab path, client errors are swallowed so the cascade falls through to the next source. Discogs intentionally absent β€” Discogs has no track-level search API (see core/discogs_client.py:575 β€” returns []). Adding a Flask endpoint that always returns empty would be a permanent no-op. --- core/musicbrainz_search.py | 57 +++++++++- tests/metadata/test_musicbrainz_search.py | 132 ++++++++++++++++++++++ web_server.py | 63 +++++++++++ webui/static/helper.js | 1 + webui/static/wishlist-tools.js | 10 +- 5 files changed, 256 insertions(+), 7 deletions(-) diff --git a/core/musicbrainz_search.py b/core/musicbrainz_search.py index ff05be13..ebd5184c 100644 --- a/core/musicbrainz_search.py +++ b/core/musicbrainz_search.py @@ -638,13 +638,34 @@ class MusicBrainzSearchClient: logger.warning(f"MusicBrainz track search failed: {e}") return [] - def _search_tracks_text(self, track_name: str, artist_name: Optional[str], limit: int) -> List[Track]: - """Fallback text-search path for structured/fuzzy track queries.""" + def _search_tracks_text(self, track_name: str, artist_name: Optional[str], limit: int, + strict: bool = True, min_score: Optional[int] = None) -> List[Track]: + """Fallback text-search path for structured/fuzzy track queries. + + `strict=True` (default) keeps the field-scoped Lucene phrase match β€” + precise enough for enrichment-style flows where the inputs are + already known-clean. `strict=False` switches to a bare-query + MB lookup that hits alias/sortname indexes with diacritic folding β€” + needed for user-facing fuzzy surfaces (Fix popup cascade) where + recall beats precision because the user picks from the result list. + Mirrors the same toggle already on `search_recording` in + `core/musicbrainz_client.py`. + + `min_score` defaults to `self._MIN_SCORE` (80) β€” sized for the + enhanced search tab where unfiltered MB results are noisy. Pass + a lower value (or 0) when a downstream stage like + `core.metadata.relevance.rerank_tracks` will re-sort by artist + match β€” MB's free-text score heavily favours title-text matches + ("Army of Me (Bjork)" cover by HIRS Collective scores 100, + BjΓΆrk's canonical "Army of Me" scores 28) so a high floor drops + the right answer. + """ try: - results = self._client.search_recording(track_name, artist_name=artist_name, limit=limit) - # Score filter matches the artist/album logic β€” cuts garbage - # title collisions from unrelated recordings. - results = [r for r in results if (r.get('score', 0) or 0) >= self._MIN_SCORE] + results = self._client.search_recording( + track_name, artist_name=artist_name, limit=limit, strict=strict + ) + threshold = self._MIN_SCORE if min_score is None else min_score + results = [r for r in results if (r.get('score', 0) or 0) >= threshold] tracks = [] for r in results: @@ -656,6 +677,30 @@ class MusicBrainzSearchClient: logger.warning(f"MusicBrainz track search failed: {e}") return [] + def search_tracks_with_artist(self, track: str, artist: str, + limit: int = 10) -> List[Track]: + """Search MB tracks with track + artist passed as separate fields. + + Powers the Fix-popup metadata cascade (`GET /api/musicbrainz/search_tracks`) + and any future surface where the caller already has the title/artist + split and wants the fuzzy-recall MB lookup without going through + `search_tracks`'s structured-query dispatch (`Artist - Track` + splitting, bare-name artist-first browse). + + Uses bare-query mode (`strict=False`) β€” diacritic-folded, hits + alias/sortname indexes, no `AND`-clause that kills recall when + either side mis-matches. Score floor lowered to 20 (vs the search + tab's 80) so MB recordings whose title doesn't literally contain + the artist name still enter the candidate pool β€” the endpoint's + `rerank_tracks` pass then sorts by artist-match relevance. Without + this, queries like `Army of Me` + `Bjork` only surface covers + (score 73-100) and miss BjΓΆrk's canonical recording (score 28). + """ + if not track and not artist: + return [] + return self._search_tracks_text(track, artist or None, limit, + strict=False, min_score=20) + def _pick_representative_release(self, releases: List[Dict[str, Any]]) -> Optional[Dict[str, Any]]: """Pick the best release out of a release-group's editions. diff --git a/tests/metadata/test_musicbrainz_search.py b/tests/metadata/test_musicbrainz_search.py index 350a52a4..0a97a89b 100644 --- a/tests/metadata/test_musicbrainz_search.py +++ b/tests/metadata/test_musicbrainz_search.py @@ -887,3 +887,135 @@ def test_get_recording_flat_swallows_client_errors(): client._client.get_recording.side_effect = RuntimeError('boom') assert client.get_recording_flat('rec-err') is None + + +# --------------------------------------------------------------------------- +# search_tracks_with_artist β€” Fix-popup cascade adapter +# --------------------------------------------------------------------------- + +def test_search_tracks_with_artist_uses_bare_query_mode(): + """The Fix-popup cascade needs MB's bare-query mode so diacritics and + bracketed suffixes don't kill recall. The adapter must pass strict=False + through to the underlying search_recording call.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [ + {'id': 'rec-1', 'title': 'Army of Me', 'score': 95, + 'releases': [{'id': 'rel-1', 'title': 'Post', 'date': '1995'}], + 'artist-credit': [{'name': 'BjΓΆrk'}]}, + ] + + tracks = client.search_tracks_with_artist('Army of Me', 'BjΓΆrk', limit=10) + + # strict=False is the critical bit β€” fuzzy recall, not phrase precision + client._client.search_recording.assert_called_once_with( + 'Army of Me', artist_name='BjΓΆrk', limit=10, strict=False + ) + assert len(tracks) == 1 + assert tracks[0].name == 'Army of Me' + assert 'BjΓΆrk' in tracks[0].artists + + +def test_search_tracks_with_artist_handles_missing_artist(): + """Track-only query (no artist) still works β€” empty string becomes + None, and the underlying client searches recordings without an + artist filter.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [ + {'id': 'rec-1', 'title': 'Some Song', 'score': 90, + 'releases': [], 'artist-credit': [{'name': 'Unknown'}]}, + ] + + client.search_tracks_with_artist('Some Song', '', limit=5) + + # Empty artist β†’ None passed to the client so MB drops the AND clause + client._client.search_recording.assert_called_once_with( + 'Some Song', artist_name=None, limit=5, strict=False + ) + + +def test_search_tracks_with_artist_empty_returns_empty_list(): + """No track and no artist β†’ return [] without hitting the network.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + + assert client.search_tracks_with_artist('', '', limit=10) == [] + client._client.search_recording.assert_not_called() + + +def test_search_tracks_with_artist_keeps_low_score_for_rerank(): + """Cascade path uses a low score floor (20) so MB recordings whose + title doesn't literally contain the artist name still enter the + candidate pool β€” the endpoint's rerank pass surfaces them by + artist-match relevance. Real example: "Army of Me" + "Bjork" β€” the + canonical BjΓΆrk recording scores 28 in MB (title doesn't contain + "Bjork"), while title-collision covers like "Army of Me (Bjork)" + score 73-100. Strict 80 floor drops the right answer.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [ + {'id': 'rec-cover', 'title': 'Army of Me (Bjork)', 'score': 100, + 'releases': [], 'artist-credit': [{'name': 'HIRS Collective'}]}, + {'id': 'rec-canonical', 'title': 'Army of Me', 'score': 28, + 'releases': [], 'artist-credit': [{'name': 'BjΓΆrk'}]}, + {'id': 'rec-noise', 'title': 'Bjork', 'score': 5, + 'releases': [], 'artist-credit': [{'name': 'Random'}]}, + ] + + tracks = client.search_tracks_with_artist('Army of Me', 'Bjork', limit=50) + + ids = [t.id for t in tracks] + # Score=28 canonical BjΓΆrk recording is kept β€” the endpoint's rerank + # will surface it by artist match. + assert 'rec-canonical' in ids + assert 'rec-cover' in ids + # Score=5 is below the 20 floor β€” true garbage still filtered out. + assert 'rec-noise' not in ids + + +def test_search_tracks_text_keeps_min_score_default_80_for_enhanced_search(): + """The enhanced search tab path keeps the historical 80 floor because + it has no downstream rerank β€” unfiltered MB results would be noisy + for free-text user search.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [ + {'id': 'rec-good', 'title': 'Good', 'score': 95, + 'releases': [], 'artist-credit': [{'name': 'A'}]}, + {'id': 'rec-mid', 'title': 'Mid', 'score': 40, + 'releases': [], 'artist-credit': [{'name': 'A'}]}, + ] + + # No min_score β†’ defaults to _MIN_SCORE (80) + tracks = client._search_tracks_text('Good', 'A', limit=10) + + titles = [t.name for t in tracks] + assert 'Good' in titles + assert 'Mid' not in titles + + +def test_search_tracks_text_strict_param_default_true(): + """Default strict=True preserves the historical behaviour of the + structured-query text-search fallback path β€” important so the + enrichment-style `search_tracks('Artist - Track')` flow stays on + field-scoped Lucene phrase matching as before.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.return_value = [] + + client._search_tracks_text('Track', 'Artist', limit=10) + + client._client.search_recording.assert_called_once_with( + 'Track', artist_name='Artist', limit=10, strict=True + ) + + +def test_search_tracks_with_artist_swallows_client_errors(): + """MB client raising must not crash the endpoint β€” return [] so the + Fix-popup cascade falls through to the next source.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_recording.side_effect = RuntimeError('network down') + + assert client.search_tracks_with_artist('Track', 'Artist', limit=10) == [] diff --git a/web_server.py b/web_server.py index 347e938c..b735c7c7 100644 --- a/web_server.py +++ b/web_server.py @@ -19190,6 +19190,69 @@ def search_deezer_tracks(): return jsonify({"error": str(e)}), 500 +@app.route('/api/musicbrainz/search_tracks', methods=['GET']) +def search_musicbrainz_tracks(): + """Search for tracks on MusicBrainz β€” used by the Discovery Fix popup + cascade and any future surface that needs track-level MB search in the + Fix-popup track shape. + + Mirrors the spotify / itunes / deezer search_tracks endpoints exactly: + accepts `track` + `artist` (or legacy `query`) plus `limit`, returns + `{tracks: [{id, name, artists, album, duration_ms, image_url, source}]}`. + + Uses MB's bare-query mode for max recall (diacritic-folded, + alias/sortname indexed) β€” same rationale as the manual MBID-paste + endpoint shipped earlier. The Fix popup is a user-facing fuzzy search + where the user picks from the result list, so recall beats precision. + """ + try: + track_q = request.args.get('track', '').strip() + artist_q = request.args.get('artist', '').strip() + legacy_query = request.args.get('query', '').strip() + limit = int(request.args.get('limit', 20)) + + if not (track_q or artist_q or legacy_query): + return jsonify({"error": "Query parameter is required"}), 400 + + from core.musicbrainz_search import MusicBrainzSearchClient + mb_search = MusicBrainzSearchClient() + + if track_q or artist_q: + tracks = mb_search.search_tracks_with_artist( + track_q or legacy_query, artist_q, limit=limit + ) + else: + # Legacy single-string query β€” let MB's structured-query + # dispatch decide artist-first browse vs text search. + tracks = mb_search.search_tracks(legacy_query, limit=limit) + + # Local rerank β€” same helper Deezer / iTunes use. Penalises + # cover / karaoke / tribute patterns + boosts exact-artist match. + if track_q or artist_q: + from core.metadata.relevance import rerank_tracks + tracks = rerank_tracks( + tracks, + expected_title=track_q, + expected_artist=artist_q, + ) + + tracks_dict = [{ + 'id': t.id, + 'name': t.name, + 'artists': t.artists, + 'album': t.album, + 'duration_ms': t.duration_ms, + 'image_url': t.image_url, + 'source': 'musicbrainz', + } for t in tracks] + + return jsonify({'tracks': tracks_dict}) + + except Exception as e: + logger.error(f"Error searching MusicBrainz tracks: {e}") + return jsonify({"error": str(e)}), 500 + + @app.route('/api/itunes/album/', methods=['GET']) def get_itunes_album_tracks(album_id): """Fetches full track details for a specific iTunes album.""" diff --git a/webui/static/helper.js b/webui/static/helper.js index 0b581183..ca5f330d 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3422,6 +3422,7 @@ const WHATS_NEW = { { title: 'Fix: MusicBrainz manual search missing results', desc: 'the Fix popup and manual library service search were using strict Lucene phrase-match queries against the `recording` / `release` / `artist` fields β€” diacritics ("Bjork" vs canonical "BjΓΆrk"), bracketed suffixes like "(Live)", and any AND-clause mismatch all killed recall. switched user-facing manual lookups to bare queries that hit MB\'s alias / sortname indexes with diacritic folding. enrichment workers stay strict for precision.' }, { title: 'Fix: MusicBrainz album clicks 404ing in enhanced search', desc: 'every click on a MusicBrainz album result was silently 404-ing β€” the /release fetch was passing `cover-art-archive` as an `inc` param, which MB rejects with 400 (that field is returned on every release response by default, no include needed). dropped the bad include; album detail now loads correctly.' }, { title: 'Fix popup: paste a MusicBrainz URL or MBID to match directly', desc: 'new escape hatch on the Fix Track Match modal (the πŸ”§ Fix button on mirrored / YouTube / Tidal / Deezer / Beatport / ListenBrainz / Spotify-public discovery rows). when fuzzy search keeps ranking the wrong recording among many same-title versions, paste the MusicBrainz recording URL like `https://musicbrainz.org/recording/` or the bare UUID into the new field and hit "Look up". skips all fuzzy logic, resolves straight to that record, and runs it through the same confirm + match pipeline.' }, + { title: 'Fix popup: MusicBrainz added to the auto-search cascade', desc: 'the Fix Track Match modal used to query only Spotify β†’ Deezer β†’ iTunes for the auto-search, leaving MusicBrainz out of the loop entirely β€” even for users with MusicBrainz set as their primary metadata source. now MB is part of the cascade. when MB is your primary, it gets queried first; otherwise it sits as the last fallback. catches niche / non-mainstream / canonical-with-diacritics recordings that the commercial sources miss. Discogs is intentionally absent β€” Discogs has no track-level search API.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index ad705785..1a50bf19 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -214,12 +214,20 @@ async function searchDiscoveryFix() { } params.set('limit', '50'); - // Use the user's active metadata source first, then fall back to others + // Use the user's active metadata source first, then fall back to others. + // MusicBrainz is included so users on MB-as-primary get MB queried first, + // and so MB is available as a fallback for fuzzy / niche / non-mainstream + // recordings that Spotify / Deezer / iTunes miss (different catalogues, + // different cover coverage). MB sits last by default because it's + // rate-limited to 1 req/sec β€” when it's the active primary the activeIdx + // reorder below moves it to the front. Discogs is intentionally absent β€” + // Discogs has no track-level search API (releases only). const activeSource = (currentMusicSourceName || 'Spotify').toLowerCase(); const allSources = [ { key: 'spotify', endpoint: '/api/spotify/search_tracks', label: 'Spotify' }, { key: 'deezer', endpoint: '/api/deezer/search_tracks', label: 'Deezer' }, { key: 'itunes', endpoint: '/api/itunes/search_tracks', label: 'iTunes' }, + { key: 'musicbrainz', endpoint: '/api/musicbrainz/search_tracks', label: 'MusicBrainz' }, ]; // Put the active source first, keep others as fallbacks const activeIdx = allSources.findIndex(s => activeSource.includes(s.key)); From a33faaeb38a57dcdd7271a476736afbd8cc7e772 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 19:30:54 -0700 Subject: [PATCH 18/24] fix(docker): pre-bake /app/Stream so basic-search playback works on rootless Docker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `core/streaming/prepare.py:94-97` creates /app/Stream lazily via `os.makedirs(stream_folder, exist_ok=True)` on first playback. Under standard Docker this works because the container's `root` writes /app without restriction. Under rootless Docker / Podman the in-container soulsync UID maps to a host UID that can't write to /app, so the mkdir silently fails and the streaming "Play" flow errors out with no obvious user-facing cause. Same root cause + same fix shape as the May 2026 /app/Staging restart- loop fix β€” pre-bake the directory at image build time (when the layer is owned by root), and thread it through every entrypoint.sh spot that touches the canonical app-dir list. Not added to VOLUME β€” /app/Stream is a transient single-file cache (cleared on every new playback), no persistence value. Touched lines: - Dockerfile: mkdir + chown line that pre-bakes runtime dirs. - entrypoint.sh: the recursive chown gated on UID change, the always-runs mkdir + chown, and the writability audit loop. No code change. Streaming tests pass unchanged (they use tmp_path, not /app/Stream). --- Dockerfile | 10 ++++++++-- entrypoint.sh | 8 ++++---- webui/static/helper.js | 1 + 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/Dockerfile b/Dockerfile index a225b8b0..fdf5a21f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -73,8 +73,14 @@ COPY --chown=soulsync:soulsync --from=webui-builder /app/webui/static/dist /app/ # pre-baked directory would crash the container into a restart loop on # rootless Docker/Podman where in-container "root" can't write to /app. # Pre-baking the dir here makes the entrypoint mkdir a guaranteed no-op. -RUN mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts && \ - chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts +# NOTE: /app/Stream is the transient single-file streaming cache used by +# the basic-search "Play" flow (cleared per use, never persistent). It's +# created lazily by `core/streaming/prepare.py` via `os.makedirs`, which +# fails silently on rootless Docker where the soulsync UID can't write +# to /app β€” playback then errors out with no obvious cause. Pre-baking +# at build time (when the layer is owned by root) avoids that path. +RUN mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/MusicVideos /app/scripts && \ + chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/MusicVideos /app/scripts # Create defaults directory and copy template files # These will be used by entrypoint.sh to initialize empty volumes diff --git a/entrypoint.sh b/entrypoint.sh index 7d830525..8cde8e0f 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -40,7 +40,7 @@ if [ "$CURRENT_UID" != "$PUID" ] || [ "$CURRENT_GID" != "$PGID" ]; then DATA_OWNER=$(stat -c '%u:%g' /app/data 2>/dev/null || echo "unknown") if [ "$DATA_OWNER" != "$PUID:$PGID" ]; then echo "πŸ”’ Fixing permissions on app directories..." - chown -R soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true + chown -R soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true else echo "βœ… App directory permissions already correct" fi @@ -81,15 +81,15 @@ chown soulsync:soulsync /app/config/settings.py 2>/dev/null || true # Pre-mid-2026 the chown line had `|| true` but mkdir didn't β€” combined # with `set -e`, a permission-denied mkdir crashed the container into a # restart loop. Both lines are now best-effort. -mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true -chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true +mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true +chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream 2>/dev/null || true # Writability audit β€” surface a loud warning if any bind-mounted dir # isn't writable by the soulsync user. The restart-loop fix above makes # the container start regardless, but a non-writable Staging / downloads # / Transfer will fail silently inside the app (auto-import quarantine, # download writes). Better to log now than to debug missing files later. -for dir in /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts; do +for dir in /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/Stream /app/MusicVideos /app/scripts; do if [ -d "$dir" ] && ! gosu soulsync test -w "$dir" 2>/dev/null; then echo "⚠️ WARNING: $dir is not writable by soulsync (uid $(id -u soulsync))." echo " Host bind-mount perms likely mismatch the PUID/PGID env vars." diff --git a/webui/static/helper.js b/webui/static/helper.js index ca5f330d..9b497dd8 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3423,6 +3423,7 @@ const WHATS_NEW = { { title: 'Fix: MusicBrainz album clicks 404ing in enhanced search', desc: 'every click on a MusicBrainz album result was silently 404-ing β€” the /release fetch was passing `cover-art-archive` as an `inc` param, which MB rejects with 400 (that field is returned on every release response by default, no include needed). dropped the bad include; album detail now loads correctly.' }, { title: 'Fix popup: paste a MusicBrainz URL or MBID to match directly', desc: 'new escape hatch on the Fix Track Match modal (the πŸ”§ Fix button on mirrored / YouTube / Tidal / Deezer / Beatport / ListenBrainz / Spotify-public discovery rows). when fuzzy search keeps ranking the wrong recording among many same-title versions, paste the MusicBrainz recording URL like `https://musicbrainz.org/recording/` or the bare UUID into the new field and hit "Look up". skips all fuzzy logic, resolves straight to that record, and runs it through the same confirm + match pipeline.' }, { title: 'Fix popup: MusicBrainz added to the auto-search cascade', desc: 'the Fix Track Match modal used to query only Spotify β†’ Deezer β†’ iTunes for the auto-search, leaving MusicBrainz out of the loop entirely β€” even for users with MusicBrainz set as their primary metadata source. now MB is part of the cascade. when MB is your primary, it gets queried first; otherwise it sits as the last fallback. catches niche / non-mainstream / canonical-with-diacritics recordings that the commercial sources miss. Discogs is intentionally absent β€” Discogs has no track-level search API.' }, + { title: 'Fix: Docker basic-search streaming silently failed under rootless Docker', desc: 'the streaming "Play" flow on the basic search page tried to create `/app/Stream` lazily at runtime, which fails silently when the container runs under rootless Docker / Podman (in-container root can\'t write to `/app`). pre-baked the directory at image build time, matching the same pattern that fixed `/app/Staging` earlier in the cycle. non-persistent β€” no volume needed.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From 54e4ba843fd2ec23fb85080c1f855dd69f5c0833 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 19:44:17 -0700 Subject: [PATCH 19/24] fix(soulseek): suppress connection-error log spam when slskd unreachable (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When slskd_url is configured but the host is unreachable (slskd not running, wrong port, host.docker.internal not resolving), the frontend's /api/downloads/status polling fanned out to every download plugin including Soulseek. soulseek_client._make_request hit a DNS / connect failure on each poll and logged it at ERROR. Result: one "Cannot connect to host host.docker.internal:5030" log line every ~2-3 seconds for the entire duration of any download β€” visible spam even when the user wasn't using Soulseek at all. Caught aiohttp.ClientConnectorError explicitly in both _make_request and _make_direct_request. First failure emits one WARNING with actionable context (start slskd, or clear soulseek.slskd_url if you don't use Soulseek). Subsequent failures demote to DEBUG. The _last_unreachable_logged flag resets on any successful (200/201/204) response so a later outage warns again β€” suppression is per-outage, not per-process-lifetime. Same shape as the existing _last_401_logged suppression for auth failures. The architectural gap (status polling fans out to soulseek even when the user has soulseek disabled in their active download sources) is intentionally left for a follow-up. The plugin-iteration code lives in core/download_engine/engine.py and core/download_orchestrator.py; threading a "skip-when-not-active" gate through every caller is a bigger refactor than this user-facing log cleanup warrants. The WARNING-once message tells the user what to do in the meantime. 5 new pinning tests cover the suppression contract: connection error returns None (not raises), first failure WARNs + sets flag, repeats stay quiet, successful response resets the flag, _make_direct_request follows the same pattern, and non-connection exceptions still log at ERROR so real bugs aren't hidden behind the new suppression. --- core/soulseek_client.py | 33 +++++ tests/downloads/test_soulseek_pinning.py | 164 +++++++++++++++++++++++ webui/static/helper.js | 1 + 3 files changed, 198 insertions(+) diff --git a/core/soulseek_client.py b/core/soulseek_client.py index 6b928688..96cdb19d 100644 --- a/core/soulseek_client.py +++ b/core/soulseek_client.py @@ -250,6 +250,7 @@ class SoulseekClient(DownloadSourcePlugin): if response.status in [200, 201, 204]: # Accept 200 OK, 201 Created, and 204 No Content self._last_401_logged = False # Reset on success + self._last_unreachable_logged = False # Same reset for unreachable-host suppression try: if response_text.strip(): # Only parse if there's content return await response.json() @@ -291,6 +292,25 @@ class SoulseekClient(DownloadSourcePlugin): f"{method} {url} β€” slskd may be overloaded or unreachable" ) return None + except aiohttp.ClientConnectorError as e: + # Issue #649: slskd_url is configured but the host is unreachable + # (slskd not running, wrong port, DNS / Docker bridge issue). + # Status polling at /api/downloads/status fans out to every plugin + # including soulseek even when the user has soulseek toggled out + # of their active download sources, so each frontend poll + # produced an ERROR log line β€” visible spam during any + # non-soulseek download. Suppress repeats to debug; emit one + # WARNING with actionable context, then reset on any successful + # response (slskd came back up). + if not getattr(self, '_last_unreachable_logged', False): + logger.warning( + f"slskd unreachable at {self.base_url}: {e}. " + f"Either start slskd or clear `soulseek.slskd_url` in settings " + f"if you don't use Soulseek. Suppressing further connection errors." + ) + self._last_unreachable_logged = True + logger.debug(f"slskd connection failed: {method} {url}: {e}") + return None except Exception as e: logger.error(f"Error making API request: {e}") return None @@ -348,6 +368,19 @@ class SoulseekClient(DownloadSourcePlugin): f"{method} {url} β€” slskd may be overloaded or unreachable" ) return None + except aiohttp.ClientConnectorError as e: + # Issue #649 β€” same suppression as _make_request. Direct + # request is a less common path but uses the same base_url, + # so the same unreachable-host condition fires here. + if not getattr(self, '_last_unreachable_logged', False): + logger.warning( + f"slskd unreachable at {self.base_url}: {e}. " + f"Either start slskd or clear `soulseek.slskd_url` in settings " + f"if you don't use Soulseek. Suppressing further connection errors." + ) + self._last_unreachable_logged = True + logger.debug(f"slskd direct connection failed: {method} {url}: {e}") + return None except Exception as e: logger.error(f"Error making direct API request: {e}") return None diff --git a/tests/downloads/test_soulseek_pinning.py b/tests/downloads/test_soulseek_pinning.py index 253be224..ef4daec7 100644 --- a/tests/downloads/test_soulseek_pinning.py +++ b/tests/downloads/test_soulseek_pinning.py @@ -347,3 +347,167 @@ def test_make_direct_request_returns_none_on_timeout(configured_client): result = _run_async(configured_client._make_direct_request('GET', 'health')) assert result is None + + +# --------------------------------------------------------------------------- +# Issue #649 β€” connection-error log spam suppression +# --------------------------------------------------------------------------- + + +def _build_unreachable_session(error_message: str = 'Cannot connect to host'): + """Stub aiohttp session whose request() raises ClientConnectorError.""" + import aiohttp + from unittest.mock import MagicMock + + class _Cm: + async def __aenter__(self_inner): + # ClientConnectorError needs a connection_key + OSError. The + # exact values don't matter for the test β€” we just need an + # instance of the right class so the except-branch fires. + os_err = OSError(-2, 'Name or service not known') + raise aiohttp.ClientConnectorError(MagicMock(), os_err) + async def __aexit__(self_inner, *args): + return None + + class _StubSession: + async def __aenter__(self): + return self + async def __aexit__(self, *args): + return None + def request(self, *args, **kwargs): + return _Cm() + async def close(self): + return None + + return _StubSession + + +def test_unreachable_slskd_returns_none_not_raises(configured_client): + """Pin: ClientConnectorError must not propagate. Caller treats None + as a normal failure (same as a 5xx) β€” every consumer that gates on + `if response is None` keeps working when slskd is unreachable.""" + StubSession = _build_unreachable_session() + with patch('aiohttp.ClientSession', return_value=StubSession()): + result = _run_async(configured_client._make_request('GET', 'transfers/downloads')) + assert result is None + + +def test_unreachable_slskd_logs_warning_once_then_debug(configured_client, caplog): + """Issue #649: status polling at /api/downloads/status fans out to + every plugin including soulseek even when the user has soulseek + toggled out, so each frontend poll produced an ERROR log line. Pin + that the FIRST unreachable response emits one WARNING with + actionable context, and subsequent repeats demote to DEBUG so the + log isn't spammed for the lifetime of every non-soulseek download.""" + import logging + configured_client._last_unreachable_logged = False + StubSession = _build_unreachable_session() + + with patch('aiohttp.ClientSession', return_value=StubSession()): + with caplog.at_level(logging.DEBUG, logger='soulseek_client'): + # Three repeated polls β€” first must warn, rest must stay quiet. + _run_async(configured_client._make_request('GET', 'transfers/downloads')) + _run_async(configured_client._make_request('GET', 'transfers/downloads')) + _run_async(configured_client._make_request('GET', 'transfers/downloads')) + + warning_records = [r for r in caplog.records if r.levelno == logging.WARNING + and 'slskd unreachable' in r.message] + error_records = [r for r in caplog.records if r.levelno == logging.ERROR + and 'Error making API request' in r.message] + assert len(warning_records) == 1, \ + f"Expected exactly 1 WARNING (one-time slskd-unreachable notice), got {len(warning_records)}" + assert len(error_records) == 0, \ + "Connection errors must not log at ERROR β€” that's the spam pattern #649 reported" + assert configured_client._last_unreachable_logged is True + + +def test_unreachable_flag_resets_on_successful_response(configured_client, caplog): + """When slskd comes back up after a stretch of being down, a fresh + WARNING should fire if it goes down again later β€” the suppression is + per-outage, not per-process-lifetime. The flag resets on any + successful (200/201/204) response.""" + import logging + configured_client._last_unreachable_logged = True # Simulate prior outage already warned + + # Simulate a 200 response β€” must reset the suppression flag. + class _OkCm: + async def __aenter__(self_inner): + class _Resp: + status = 200 + reason = 'OK' + async def text(self_resp): + return '{"ok": true}' + async def json(self_resp): + return {'ok': True} + return _Resp() + async def __aexit__(self_inner, *args): + return None + + class _OkSession: + async def __aenter__(self): + return self + async def __aexit__(self, *args): + return None + def request(self, *args, **kwargs): + return _OkCm() + async def close(self): + return None + + with patch('aiohttp.ClientSession', return_value=_OkSession()): + _run_async(configured_client._make_request('GET', 'server/state')) + + assert configured_client._last_unreachable_logged is False, \ + "Successful response must reset the suppression flag so a future outage warns again" + + +def test_make_direct_request_also_suppresses_unreachable_spam(configured_client, caplog): + """`_make_direct_request` shares the same base_url and same outage + mode, so it gets the same WARNING-once + DEBUG-after treatment.""" + import logging + configured_client._last_unreachable_logged = False + StubSession = _build_unreachable_session() + + with patch('aiohttp.ClientSession', return_value=StubSession()): + with caplog.at_level(logging.DEBUG, logger='soulseek_client'): + _run_async(configured_client._make_direct_request('GET', 'health')) + _run_async(configured_client._make_direct_request('GET', 'health')) + + warning_records = [r for r in caplog.records if r.levelno == logging.WARNING + and 'slskd unreachable' in r.message] + error_records = [r for r in caplog.records if r.levelno == logging.ERROR + and 'Error making direct API request' in r.message] + assert len(warning_records) == 1 + assert len(error_records) == 0 + + +def test_non_connection_exception_still_logs_error(configured_client, caplog): + """Guard: only ClientConnectorError gets the suppression treatment. + Any other exception (programming bug, unexpected aiohttp behaviour, + etc.) must still surface at ERROR so we don't accidentally hide + real problems behind the noise reduction.""" + import logging + + class _BoomCm: + async def __aenter__(self_inner): + raise ValueError("not a connection error β€” should still log ERROR") + async def __aexit__(self_inner, *args): + return None + + class _BoomSession: + async def __aenter__(self): + return self + async def __aexit__(self, *args): + return None + def request(self, *args, **kwargs): + return _BoomCm() + async def close(self): + return None + + with patch('aiohttp.ClientSession', return_value=_BoomSession()): + with caplog.at_level(logging.DEBUG, logger='soulseek_client'): + result = _run_async(configured_client._make_request('GET', 'transfers/downloads')) + + assert result is None # Still returns None β€” non-raising contract preserved + error_records = [r for r in caplog.records if r.levelno == logging.ERROR + and 'Error making API request' in r.message] + assert len(error_records) == 1, "Non-connection exceptions must still log ERROR" diff --git a/webui/static/helper.js b/webui/static/helper.js index 9b497dd8..e0caf2c3 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3424,6 +3424,7 @@ const WHATS_NEW = { { title: 'Fix popup: paste a MusicBrainz URL or MBID to match directly', desc: 'new escape hatch on the Fix Track Match modal (the πŸ”§ Fix button on mirrored / YouTube / Tidal / Deezer / Beatport / ListenBrainz / Spotify-public discovery rows). when fuzzy search keeps ranking the wrong recording among many same-title versions, paste the MusicBrainz recording URL like `https://musicbrainz.org/recording/` or the bare UUID into the new field and hit "Look up". skips all fuzzy logic, resolves straight to that record, and runs it through the same confirm + match pipeline.' }, { title: 'Fix popup: MusicBrainz added to the auto-search cascade', desc: 'the Fix Track Match modal used to query only Spotify β†’ Deezer β†’ iTunes for the auto-search, leaving MusicBrainz out of the loop entirely β€” even for users with MusicBrainz set as their primary metadata source. now MB is part of the cascade. when MB is your primary, it gets queried first; otherwise it sits as the last fallback. catches niche / non-mainstream / canonical-with-diacritics recordings that the commercial sources miss. Discogs is intentionally absent β€” Discogs has no track-level search API.' }, { title: 'Fix: Docker basic-search streaming silently failed under rootless Docker', desc: 'the streaming "Play" flow on the basic search page tried to create `/app/Stream` lazily at runtime, which fails silently when the container runs under rootless Docker / Podman (in-container root can\'t write to `/app`). pre-baked the directory at image build time, matching the same pattern that fixed `/app/Staging` earlier in the cycle. non-persistent β€” no volume needed.' }, + { title: 'Fix: slskd-unreachable log spam during non-Soulseek downloads', desc: 'when slskd was configured but not actually running (or unreachable on its configured port), the `/api/downloads/status` polling loop fanned out to every download plugin including Soulseek, producing one `ERROR - Cannot connect to host ... [Name or service not known]` log line per poll for the entire duration of any download β€” visible spam even when the user wasn\'t using Soulseek at all. Connection failures now emit one WARNING with actionable context (start slskd, or clear the slskd_url if you don\'t use Soulseek) and demote subsequent failures to debug. The flag resets on the next successful slskd response so a later outage warns again.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From 987409508b6861d8c5d2eeddee13a888c2c0f589 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 20:20:28 -0700 Subject: [PATCH 20/24] fix(metadata): surface MusicBrainz 'Other' release-groups in discography (#650) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S-Bryce reported that for some artists (Vocaloid producers, JP indie acts, niche Western indie) the artist detail page was missing whole release-groups visible on musicbrainz.org. Downloaded tracks from those release-groups appeared in artist track counts but were not bound to any visible album / single card β€” orphan "ghost" tracks the user couldn't browse to. Two duplicated bugs fed each other: 1. `core/musicbrainz_search.py` browsed MB release-groups with `release_types=['album', 'ep', 'single']`. MB's primary-type vocabulary is {Album, Single, EP, Broadcast, Other} β€” music videos, one-off web releases, and broadcast singles use Other. Pre-fix the filter dropped them at the API layer. 2. Three sites duplicated the same "raw primary-type β†’ internal album_type" mapping with slightly different vocabularies and all silently defaulted unknown values (including 'Other') to 'album': core/musicbrainz_search.py `_map_release_type` core/metadata/types.py inline `{single:single, ep:ep}.get(...)` core/metadata/cache.py Deezer-specific record_type guard Letting Other through the filter without a real mapper would have placed music videos in the Albums view alongside LPs β€” visually misleading. Fix shape: - New `core/metadata/release_type.py` β€” single canonical mapper consumed by every provider's rawβ†’Album projection. Knows the full MB vocabulary including 'other' and 'broadcast'; routes both into the singles bucket since they're functionally single-track releases. Compilation secondary-type override preserved (MB's canonical Greatest-Hits pattern is `primary=Album, secondary=[Compilation]`). - `core/musicbrainz_search.py` `_map_release_type` becomes a thin alias for the new helper so the six internal call sites stay intact. API filter gains 'other'. - `core/metadata/types.py` Album projection drops its inline mini- mapper and calls the canonical helper. Now also handles the compilation secondary-type override it was previously missing. - The Deezer-specific cache.py guard stays as-is β€” Deezer's record_type vocabulary is closed (album|single|ep), not affected by this issue. Verified end-to-end against MB for S-Bryce's artist (`46196b9c-affa- 4616-b53b-e967c8bd70e0`, inabakumori): pre-fix returned 22 release- groups; post-fix returns 27, with the 5 extra all landing in the Singles section with album_type='single' as intended. 23 new unit tests pin the mapper contract (case-insensitive primary types, compilation secondary override, Other/Broadcast β†’ single, unknown β†’ album default preserved, defensive empty/None inputs). 2 new tests in test_musicbrainz_search pin the API filter inclusion of 'other' and the round-trip into the Singles bucket. All 516 existing metadata tests still green β€” refactor leaves historical behaviour for {album, ep, single, compilation} unchanged. --- core/metadata/release_type.py | 93 ++++++++++++++ core/metadata/types.py | 12 +- core/musicbrainz_search.py | 28 +++-- tests/metadata/test_musicbrainz_search.py | 55 +++++++++ tests/metadata/test_release_type.py | 140 ++++++++++++++++++++++ webui/static/helper.js | 1 + 6 files changed, 313 insertions(+), 16 deletions(-) create mode 100644 core/metadata/release_type.py create mode 100644 tests/metadata/test_release_type.py diff --git a/core/metadata/release_type.py b/core/metadata/release_type.py new file mode 100644 index 00000000..1f0b6b9b --- /dev/null +++ b/core/metadata/release_type.py @@ -0,0 +1,93 @@ +"""Canonical mapping from raw provider release-type vocabulary to the +internal `album_type` field that drives discography binning + UI. + +Why this exists +--------------- +Three sites historically duplicated the same "best-effort primary-type +β†’ album_type" mapping, each with a slightly different vocabulary: + + core/musicbrainz_search.py: `_map_release_type` knew about + {album, single, ep, compilation}, defaulted unknown β†’ 'album'. + core/metadata/types.py: inline `{single: single, ep: ep}.get(...)` β€” + didn't even know about 'compilation', also defaulted β†’ 'album'. + core/metadata/cache.py: Deezer-specific record_type validator β€” + intentionally narrow, kept here for its provider. + +Issue #650 (S-Bryce) reported that MusicBrainz tags music videos and +some legitimate singles with primary-type=`Other`, which both mappers +silently routed to `album_type='album'`. Combined with the API-level +filter at `musicbrainz_search.search_albums` (which only requested +`type=album|ep|single` from MB and dropped 'Other' entirely), users +with MB-as-primary saw entire release-groups go missing from artist +discography views, and downloaded tracks from those release-groups +appeared as orphan "ghost" tracks bound to no album card. + +Fix shape: one shared mapper consumed by every provider's +`raw β†’ Album dataclass` projection. Knows about 'other' and +'broadcast' (MB's two remaining primary-type vocabulary words) and +maps them to 'single' so they land in the Singles section of the +artist detail page β€” they're almost always single-track music +releases (music videos, broadcast singles, one-off web releases). +Falling through to 'album' was the original sin β€” places them in +Albums view where they look misleading and clutter the proper LP +list. +""" + +from __future__ import annotations + +from typing import List, Optional + + +# MB primary-type vocabulary as of 2026 β€” `Album | Single | EP | +# Broadcast | Other`. Compilation is a *secondary* type; querying MB +# with type=compilation silently breaks (returns ~10% of expected +# results) β€” see musicbrainz_client.browse_artist_release_groups docs. +_AUDIO_OTHER_PRIMARY_TYPES = frozenset({'other', 'broadcast'}) + + +def map_release_group_type(primary_type: Optional[str], + secondary_types: Optional[List[str]] = None) -> str: + """Project a raw provider release-group primary-type + secondary-types + into the internal `album_type` value the UI binning expects. + + Returns one of: `'album'`, `'single'`, `'ep'`, `'compilation'`. + + Mapping rules: + + - `single` / `ep` pass through unchanged. + - `compilation` (primary or secondary) becomes `'compilation'`. The + compilation secondary-type check is required because MB's + canonical pattern is `primary=Album, secondary=[Compilation]`. + - `other` / `broadcast` become `'single'`. Almost always + single-track music releases (music videos, one-off web drops, + broadcast singles). Placing them in Singles is the pragmatic + bucket β€” they're not LPs, but excluding them entirely (the + pre-fix behaviour) hid legitimate tracks. + - Anything else (including empty/None) β†’ `'album'`. Matches the + pre-fix default so no existing classifications shift. + + `secondary_types` is optional because the legacy types.py call site + doesn't have access to it from the same level of raw structure. + Pass `None` (or omit) for the secondary-types-unavailable path. + """ + pt = (primary_type or '').strip().lower() + + if pt == 'single': + return 'single' + if pt == 'ep': + return 'ep' + if pt == 'compilation': + return 'compilation' + + # Secondary-type override: MB's compilation albums always carry + # `primary=Album` with `secondary=[Compilation]`, so the primary + # check above can't catch them. + if secondary_types: + normalized = {str(s).strip().lower() for s in secondary_types if s} + if 'compilation' in normalized: + return 'compilation' + + if pt in _AUDIO_OTHER_PRIMARY_TYPES: + return 'single' + + return 'album' diff --git a/core/metadata/types.py b/core/metadata/types.py index 27d5a7c5..b3d5e7f0 100644 --- a/core/metadata/types.py +++ b/core/metadata/types.py @@ -400,10 +400,16 @@ class Album: if raw.get('barcode'): external_ids['barcode'] = _str(raw['barcode']) - # MB `release-group` carries the album-level type (album/single/ep) + # MB `release-group` carries the album-level type (album/single/ep/ + # compilation/other/broadcast). Centralized mapper handles the + # full vocabulary including 'other' / 'broadcast' (issue #650 β€” + # music videos and one-off releases) so this projection matches + # the search-adapter projection in `core/musicbrainz_search.py`. + from core.metadata.release_type import map_release_group_type rg = raw.get('release-group') or {} - primary_type = _str(rg.get('primary-type'), default='Album').lower() - album_type = {'single': 'single', 'ep': 'ep'}.get(primary_type, 'album') + primary_type = _str(rg.get('primary-type'), default='Album') + secondary_types = rg.get('secondary-types') or [] + album_type = map_release_group_type(primary_type, secondary_types) if rg.get('id'): external_ids['musicbrainz_release_group'] = _str(rg['id']) diff --git a/core/musicbrainz_search.py b/core/musicbrainz_search.py index ebd5184c..f01292d4 100644 --- a/core/musicbrainz_search.py +++ b/core/musicbrainz_search.py @@ -114,18 +114,12 @@ def _extract_title_hint(query: str, artist_name: str) -> Optional[str]: return None -def _map_release_type(primary_type: str, secondary_types: List[str] = None) -> str: - """Map MusicBrainz release group type to standard album_type.""" - pt = (primary_type or '').lower() - if pt == 'album': - return 'album' - elif pt == 'single': - return 'single' - elif pt == 'ep': - return 'ep' - elif pt == 'compilation' or 'compilation' in (secondary_types or []): - return 'compilation' - return 'album' +# Thin module-level alias retained so callers inside this file keep +# working without touching every call site. The canonical implementation +# (including the 'other' / 'broadcast' handling that fixes issue #650) +# lives in `core/metadata/release_type.py` so every provider's `raw β†’ +# Album` projection shares one mapper. +from core.metadata.release_type import map_release_group_type as _map_release_type class MusicBrainzSearchClient: @@ -365,7 +359,15 @@ class MusicBrainzSearchClient: # the filter silently breaks. Actual compilations # (primary-type=Album with secondary-types=[Compilation]) # are handled by the studio-preference filter below. - release_types=['album', 'ep', 'single'], + # 'other' added per issue #650 β€” MB tags music videos + # and one-off web/broadcast releases with primary=Other, + # and many artists (Vocaloid producers, indie acts, JP + # solo artists) have legitimate singles classified + # there. Pre-fix this filter dropped them at the API + # layer, hiding tracks the user had downloaded. + # `map_release_group_type` routes 'other' into the + # singles bucket so they appear in the right UI section. + release_types=['album', 'ep', 'single', 'other'], limit=100, ) diff --git a/tests/metadata/test_musicbrainz_search.py b/tests/metadata/test_musicbrainz_search.py index 0a97a89b..cec77648 100644 --- a/tests/metadata/test_musicbrainz_search.py +++ b/tests/metadata/test_musicbrainz_search.py @@ -680,6 +680,61 @@ def test_search_albums_bare_artist_no_hint_no_filter(): assert 'Revolver' in titles +# --------------------------------------------------------------------------- +# Issue #650 β€” 'Other' primary-type release-groups must surface +# --------------------------------------------------------------------------- + + +def test_search_albums_browse_filter_requests_other_primary_type(): + """Issue #650: pre-fix the MB browse filter requested only + `album|ep|single`, dropping every primary-type=`Other` release-group + at the API layer. For artists like Vocaloid producers and JP indie + acts whose music videos / one-off web releases are tagged Other, + that hid legitimate tracks. Pin that the filter now includes + 'other' so those release-groups round-trip into the discography.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_artist.return_value = [_mk_artist('Inabakumori', 'mb-i', score=100)] + client._client.browse_artist_release_groups.return_value = [] + + client.search_albums('inabakumori', limit=10) + + # Inspect the actual call args β€” the API filter is the lever that + # decides whether MB returns Other-typed groups at all. + args, kwargs = client._client.browse_artist_release_groups.call_args + requested_types = kwargs.get('release_types') or (args[1] if len(args) > 1 else None) + assert requested_types is not None, \ + "browse_artist_release_groups must receive an explicit release_types filter" + assert 'other' in requested_types, \ + f"'other' must be in the requested types so #650 Other-typed releases surface; got {requested_types}" + + +def test_search_albums_other_type_release_groups_appear_as_singles(): + """When MB returns an Other-typed release-group (music video, + one-off web release), it must arrive in the discography as an + Album dataclass with album_type='single' β€” so the downstream + binner in `core/metadata/discography.py` routes it to the Singles + section rather than burying it among LPs.""" + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.search_artist.return_value = [_mk_artist('Inabakumori', 'mb-i', score=100)] + client._client.browse_artist_release_groups.return_value = [ + {'id': 'rg-mv', 'title': 'γƒ­γ‚Ήγƒˆγ‚’γƒ³γƒ–γƒ¬γƒ©', 'primary-type': 'Other', + 'first-release-date': '2018-02-27', 'secondary-types': []}, + {'id': 'rg-single', 'title': 'γƒ©γ‚°γƒˆγƒ¬γ‚€γƒ³', 'primary-type': 'Single', + 'first-release-date': '2020-01-01', 'secondary-types': []}, + ] + + albums = client.search_albums('inabakumori', limit=10) + + by_id = {a.id: a for a in albums} + assert 'rg-mv' in by_id, "Other-typed release-group must survive the filter and arrive in the result" + assert by_id['rg-mv'].album_type == 'single', \ + "Other-typed release-group must map to album_type='single' so it lands in the Singles section" + # Pre-existing single behaviour unchanged. + assert by_id['rg-single'].album_type == 'single' + + def test_recording_to_track_total_tracks_matches_media_count(): """Regression: total_tracks was initialized at 1 and summed with media track-counts, producing an off-by-one. An 11-track album reported 12.""" diff --git a/tests/metadata/test_release_type.py b/tests/metadata/test_release_type.py new file mode 100644 index 00000000..9b6be12a --- /dev/null +++ b/tests/metadata/test_release_type.py @@ -0,0 +1,140 @@ +"""Tests for the canonical release-type mapper. + +Covers issue #650 β€” MusicBrainz's `Other` and `Broadcast` primary +types previously defaulted to `album_type='album'`, hiding music +videos and one-off releases from artist discography views. The mapper +now routes them to `single` so they land in the Singles bucket of the +artist detail page. + +Also pins the existing mappings (album/ep/single/compilation) so the +refactor of three sibling type-mappers into one shared helper doesn't +drift the historical behaviour. +""" + +from __future__ import annotations + +import pytest + +from core.metadata.release_type import map_release_group_type + + +# --------------------------------------------------------------------------- +# Pin existing primary-type mappings (no regression from refactor) +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("primary_type,expected", [ + ("album", "album"), + ("Album", "album"), # MB returns title-cased values + ("ALBUM", "album"), + ("single", "single"), + ("Single", "single"), + ("ep", "ep"), + ("EP", "ep"), + ("compilation", "compilation"), + ("Compilation", "compilation"), +]) +def test_known_primary_types_map_canonically(primary_type, expected): + """Pin: case-insensitive primary-type mapping for the four + canonical types every consumer relied on pre-refactor.""" + assert map_release_group_type(primary_type) == expected + + +# --------------------------------------------------------------------------- +# Issue #650 β€” 'Other' and 'Broadcast' primary types +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("primary_type", ["other", "Other", "OTHER"]) +def test_other_primary_type_routes_to_singles(primary_type): + """Issue #650: MB tags music videos and one-off web releases with + `primary-type=Other`. They're functionally single-track releases, + so route them to `single` (lands in Singles section). Pre-fix + they fell through to the `album` default β€” placed in Albums view + where they cluttered the LP list AND, paired with the API filter, + were sometimes dropped from the discography entirely.""" + assert map_release_group_type(primary_type) == "single" + + +@pytest.mark.parametrize("primary_type", ["broadcast", "Broadcast"]) +def test_broadcast_primary_type_routes_to_singles(primary_type): + """Broadcasts (radio sessions, one-off live single transmissions) + are also single-track in practice. Same routing as 'Other'.""" + assert map_release_group_type(primary_type) == "single" + + +# --------------------------------------------------------------------------- +# Secondary-type compilation handling +# --------------------------------------------------------------------------- + + +def test_compilation_secondary_type_overrides_album_primary(): + """MB's canonical compilation pattern is `primary=Album, + secondary=[Compilation]`. The compilation secondary check must + fire even when the primary is Album, so 'Greatest Hits' style + releases land in the compilation bucket.""" + assert map_release_group_type("Album", ["Compilation"]) == "compilation" + + +def test_compilation_secondary_type_case_insensitive(): + """Secondary-type matching tolerates case + whitespace variations + in the provider response.""" + assert map_release_group_type("Album", ["compilation"]) == "compilation" + assert map_release_group_type("Album", [" Compilation "]) == "compilation" + + +def test_other_secondary_types_do_not_override_primary(): + """Only 'compilation' is checked as a secondary-type override. + Other MB secondary types (Live, Remix, Soundtrack, etc.) belong + to the discography filter at the search-adapter layer, not the + type mapper.""" + assert map_release_group_type("Album", ["Live"]) == "album" + assert map_release_group_type("Single", ["Remix"]) == "single" + + +def test_compilation_secondary_overrides_other_primary(): + """An 'Other' release tagged as Compilation lands in compilation, + not singles β€” secondary-type compilation is the strongest + classification signal.""" + assert map_release_group_type("Other", ["Compilation"]) == "compilation" + + +# --------------------------------------------------------------------------- +# Empty / unknown / defensive +# --------------------------------------------------------------------------- + + +def test_empty_primary_type_defaults_to_album(): + """Pin: empty / None primary-type still defaults to 'album' so + consumers that build records without complete provider data don't + suddenly land in a different bucket.""" + assert map_release_group_type("") == "album" + assert map_release_group_type(None) == "album" + + +def test_unknown_primary_type_defaults_to_album(): + """Pin: a primary-type value we don't know about defaults to + 'album'. Matches the pre-refactor fall-through so new MB + vocabulary doesn't accidentally cause a behaviour shift.""" + assert map_release_group_type("audiobook") == "album" + assert map_release_group_type("video") == "album" + + +def test_secondary_types_none_is_safe(): + """Pin: omitting secondary_types (legacy types.py call site) still + works β€” None and missing-arg both treated as no-secondary-types.""" + assert map_release_group_type("Album", None) == "album" + assert map_release_group_type("Album") == "album" + + +def test_secondary_types_with_none_entries_skipped(): + """Defensive: provider responses occasionally include None or empty + string in the secondary-types list. The mapper must not crash.""" + assert map_release_group_type("Album", [None, "", "Compilation"]) == "compilation" + assert map_release_group_type("Album", [None, ""]) == "album" + + +def test_whitespace_in_primary_type_normalized(): + """Defensive: a stray-whitespace primary-type still classifies.""" + assert map_release_group_type(" single ") == "single" + assert map_release_group_type(" Other ") == "single" diff --git a/webui/static/helper.js b/webui/static/helper.js index e0caf2c3..868e5a5f 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3425,6 +3425,7 @@ const WHATS_NEW = { { title: 'Fix popup: MusicBrainz added to the auto-search cascade', desc: 'the Fix Track Match modal used to query only Spotify β†’ Deezer β†’ iTunes for the auto-search, leaving MusicBrainz out of the loop entirely β€” even for users with MusicBrainz set as their primary metadata source. now MB is part of the cascade. when MB is your primary, it gets queried first; otherwise it sits as the last fallback. catches niche / non-mainstream / canonical-with-diacritics recordings that the commercial sources miss. Discogs is intentionally absent β€” Discogs has no track-level search API.' }, { title: 'Fix: Docker basic-search streaming silently failed under rootless Docker', desc: 'the streaming "Play" flow on the basic search page tried to create `/app/Stream` lazily at runtime, which fails silently when the container runs under rootless Docker / Podman (in-container root can\'t write to `/app`). pre-baked the directory at image build time, matching the same pattern that fixed `/app/Staging` earlier in the cycle. non-persistent β€” no volume needed.' }, { title: 'Fix: slskd-unreachable log spam during non-Soulseek downloads', desc: 'when slskd was configured but not actually running (or unreachable on its configured port), the `/api/downloads/status` polling loop fanned out to every download plugin including Soulseek, producing one `ERROR - Cannot connect to host ... [Name or service not known]` log line per poll for the entire duration of any download β€” visible spam even when the user wasn\'t using Soulseek at all. Connection failures now emit one WARNING with actionable context (start slskd, or clear the slskd_url if you don\'t use Soulseek) and demote subsequent failures to debug. The flag resets on the next successful slskd response so a later outage warns again.' }, + { title: 'Fix: MusicBrainz "Other" release-groups now visible in discography', desc: 'MB tags music videos, one-off web releases, and broadcast singles with `primary-type=Other` β€” common pattern for Vocaloid producers, JP indie artists, and some Western indie acts. The release-group browse filter only requested `album|ep|single`, dropping every Other-typed group at the API layer. Combined with the inline type mapper defaulting unknown primary types to "album", this hid legitimate tracks from the artist detail page and left downloaded tracks orphaned (visible in track counts but not bound to any visible album / single card). Centralised the type-mapper into one shared helper (`core/metadata/release_type.py`) used by every provider\'s rawβ†’Album projection, added `Other` and `Broadcast` handling that routes those release-groups into the Singles section, and added `other` to the MB API filter. For inabakumori, this surfaces 5 previously-invisible releases. 23 unit tests pin the mapper contract; existing 65 search-adapter tests still green.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From 79ad4d885d3877d2a5f45fd6e13bc92497b1ac83 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 21:19:50 -0700 Subject: [PATCH 21/24] fix(quarantine): drop already-quarantined sources from candidate picker (#652) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a file failed AcoustID verification and got quarantined, the next auto-wishlist cycle would search for the same track, the deterministic quality picker would re-select the same (uploader, filename) source, re-download it, and re-quarantine it. Users woke up to hundreds of duplicate .quarantined entries from a single bad upload β€” same source URL repeatedly, byte-for-byte identical files. Root cause: `SoulseekClient.filter_results_by_quality_preference` ranks candidates by quality + bitrate density only. Quarantine history wasn't consulted, so a high-bitrate FLAC upload with a wrong-track AcoustID fingerprint kept winning the picker against every other candidate. Fix shape: - New helper `core/imports/quarantine.py::get_quarantined_source_keys` reads every quarantine sidecar's `context.original_search_result` and returns the set of `(username, filename)` tuples for O(1) membership checks. Sidecars missing the context field (legacy thin sidecars written pre-Feb 2026, or orphaned files) and corrupt JSON are skipped silently β€” defensive against transient FS / encoding issues. - `SoulseekClient._drop_quarantined_sources` runs the membership filter against incoming TrackResults, drops matches, logs a single INFO line with the skip count. Called first inside `filter_results_by_quality_preference` so all four callers (search-and-download, master worker, validation, orchestrator) benefit transparently. - Approving or deleting a quarantine entry removes its sidecar, so the dedup key disappears from the set on the next search β€” gives the user a way to opt back in to a previously-quarantined source without restarting the app. 7 helper tests cover: missing dir, empty dir, well-formed sidecars collected as tuples, legacy sidecars skipped, empty source fields skipped (so empty-string keys can't accidentally drop unrelated results), corrupt JSON tolerated, duplicate quarantines collapse. 5 integration tests pin: clean candidates pass, known-bad candidates drop, missing quarantine dir returns input unchanged, filesystem errors swallowed (defensive), full `filter_results_by_quality_preference` runs the dedup BEFORE the quality picker β€” so a high-quality quarantined source can't win on bitrate. 692 existing download + import tests still green. Cosmetic surface of the fix is invisible β€” same UX as today when no quarantine entries exist; loop only kicks in once a sidecar has been written. Out of scope: bulk-select / multi-delete UI for the quarantine tab β€” S-Bryce mentioned this as a separate pain point in the issue, but it's its own UX work, not a one-commit drive-by. --- core/imports/quarantine.py | 59 +++++++ core/soulseek_client.py | 57 +++++++ tests/downloads/test_soulseek_pinning.py | 169 ++++++++++++++++++++ tests/imports/test_quarantine_management.py | 113 +++++++++++++ webui/static/helper.js | 1 + 5 files changed, 399 insertions(+) diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 38be1b41..43ed5191 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -93,6 +93,65 @@ def entry_id_from_quarantined_filename(quarantined_filename: str) -> str: return _entry_id_from_filename(os.path.basename(quarantined_filename)) +def get_quarantined_source_keys(quarantine_dir: str) -> set: + """Return a set of ``(username, filename)`` tuples for every Soulseek + source that has been quarantined. + + Used to gate the Soulseek candidate filter against re-picking the + exact same upload that already failed post-download verification. + Issue #652 β€” without this gate, the auto-wishlist processor's + candidate ranking is deterministic, so the same `(uploader, file)` + keeps winning the quality picker, downloading, quarantining, and + re-queueing in an infinite loop. Users wake up to hundreds of + duplicate `.quarantined` files for the same source URL. + + The keys come from the sidecar JSON's + ``context.original_search_result`` field which `move_to_quarantine` + persists from the originating SearchResult. Sidecars missing either + field (legacy thin sidecars written pre-Feb 2026, or orphaned + files) are skipped silently β€” they can't gate anything anyway. + + Returns an empty set when the directory doesn't exist or has no + parseable sidecars. Never raises; filesystem / JSON errors are + swallowed at debug level so a corrupt sidecar can't block the + download pipeline. + """ + keys: set = set() + if not quarantine_dir or not os.path.isdir(quarantine_dir): + return keys + + try: + names = os.listdir(quarantine_dir) + except OSError as exc: + logger.debug("get_quarantined_source_keys: listdir failed: %s", exc) + return keys + + for name in names: + if not name.endswith('.json'): + continue + sidecar_path = os.path.join(quarantine_dir, name) + try: + with open(sidecar_path, encoding='utf-8') as f: + sidecar = json.load(f) + except Exception as exc: + logger.debug("get_quarantined_source_keys: sidecar read failed for %s: %s", name, exc) + continue + if not isinstance(sidecar, dict): + continue + ctx = sidecar.get('context') + if not isinstance(ctx, dict): + continue + osr = ctx.get('original_search_result') + if not isinstance(osr, dict): + continue + username = osr.get('username') or '' + filename = osr.get('filename') or '' + if username and filename: + keys.add((str(username), str(filename))) + + return keys + + def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: """Enumerate quarantined files paired with their sidecars. diff --git a/core/soulseek_client.py b/core/soulseek_client.py index 96cdb19d..71b20345 100644 --- a/core/soulseek_client.py +++ b/core/soulseek_client.py @@ -1497,14 +1497,71 @@ class SoulseekClient(DownloadSourcePlugin): 'other': (0, 500), } + def _drop_quarantined_sources(self, results: List[TrackResult]) -> List[TrackResult]: + """Filter out candidates whose `(username, filename)` is on the + quarantine record. Issue #652. + + Reads quarantine sidecars fresh each call so newly-quarantined + sources are honored immediately on the next search β€” no client + state to invalidate. Filesystem cost is bounded (one listdir + + N small JSON reads) and dwarfed by the Soulseek search itself. + + Returns the input list unchanged when the quarantine directory + is absent, empty, or unreadable β€” i.e. defaults to today's + behaviour if anything goes wrong on the dedup path. + """ + try: + from core.imports.quarantine import get_quarantined_source_keys + download_path = config_manager.get('soulseek.download_path', './downloads') + quarantine_dir = os.path.join(download_path, 'ss_quarantine') + blocked = get_quarantined_source_keys(quarantine_dir) + except Exception as exc: + logger.debug("quarantine dedup: failed to load source keys, skipping filter: %s", exc) + return results + + if not blocked: + return results + + kept: List[TrackResult] = [] + skipped = 0 + for candidate in results: + key = (candidate.username or '', candidate.filename or '') + if key in blocked: + skipped += 1 + continue + kept.append(candidate) + + if skipped: + logger.info( + f"Quarantine dedup: dropped {skipped} candidate(s) matching previously-quarantined sources; " + f"{len(kept)} remain" + ) + return kept + def filter_results_by_quality_preference(self, results: List[TrackResult]) -> List[TrackResult]: """ Filter candidates based on user's quality profile with bitrate density constraints. Uses priority waterfall logic: tries highest priority quality first, falls back to lower priorities. Returns candidates matching quality profile constraints, sorted by confidence and effective bitrate. + + Issue #652: also drops candidates whose `(username, filename)` + matches a previously-quarantined download. Without this pre-filter + the auto-wishlist processor's ranking is deterministic β€” the same + `(uploader, file)` keeps winning the quality picker, downloading, + failing AcoustID, quarantining, and re-queueing in an infinite + loop. Users wake up to hundreds of duplicate `.quarantined` files + for the same source URL. """ from database.music_database import MusicDatabase + if not results: + return [] + + # Drop sources already quarantined β€” bypass the quality picker + # entirely so the same bad upload doesn't get re-selected on the + # next wishlist cycle. Filesystem read is bounded (~few hundred + # sidecars in practical use Γ— <1ms each). + results = self._drop_quarantined_sources(results) if not results: return [] diff --git a/tests/downloads/test_soulseek_pinning.py b/tests/downloads/test_soulseek_pinning.py index ef4daec7..0c7a8202 100644 --- a/tests/downloads/test_soulseek_pinning.py +++ b/tests/downloads/test_soulseek_pinning.py @@ -511,3 +511,172 @@ def test_non_connection_exception_still_logs_error(configured_client, caplog): error_records = [r for r in caplog.records if r.levelno == logging.ERROR and 'Error making API request' in r.message] assert len(error_records) == 1, "Non-connection exceptions must still log ERROR" + + +# --------------------------------------------------------------------------- +# Issue #652 β€” quarantined-source dedup in the candidate filter +# --------------------------------------------------------------------------- + + +def _mk_track_result(username='peer', filename='song.flac', quality='flac', + bitrate=1411, size=10_000_000, duration=180_000): + """Build a minimal TrackResult for the candidate filter tests.""" + from core.download_plugins.types import TrackResult + return TrackResult( + username=username, + filename=filename, + size=size, + bitrate=bitrate, + duration=duration, + quality=quality, + free_upload_slots=1, + upload_speed=1_000_000, + queue_length=0, + ) + + +def test_drop_quarantined_sources_keeps_clean_candidates(configured_client, tmp_path, monkeypatch): + """When no candidate matches a quarantined `(username, filename)`, + every result passes through. Filter is a no-op for clean searches.""" + quarantine_dir = tmp_path / 'ss_quarantine' + quarantine_dir.mkdir() + + # Patch config_manager to point at our temp download path. + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + results = [ + _mk_track_result(username='goodpeer1', filename='a.flac'), + _mk_track_result(username='goodpeer2', filename='b.flac'), + ] + + kept = configured_client._drop_quarantined_sources(results) + + assert len(kept) == 2 + assert {r.username for r in kept} == {'goodpeer1', 'goodpeer2'} + + +def test_drop_quarantined_sources_drops_known_bad(configured_client, tmp_path, monkeypatch): + """Issue #652 core contract: a candidate whose `(username, filename)` + matches a quarantined entry is dropped before the quality picker + ranks it. Stops the loop where the same source kept winning the + quality picker and re-downloading itself.""" + import json as _json + quarantine_dir = tmp_path / 'ss_quarantine' + quarantine_dir.mkdir() + + # Write a sidecar matching the bad source. + sidecar = { + "original_filename": "bad.flac", + "quarantine_reason": "AcoustID mismatch", + "context": { + "original_search_result": { + "username": "badpeer", "filename": "albums/bad.flac", + }, + }, + } + (quarantine_dir / "20260518_120000.json").write_text(_json.dumps(sidecar)) + + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + results = [ + _mk_track_result(username='badpeer', filename='albums/bad.flac'), + _mk_track_result(username='goodpeer', filename='albums/good.flac'), + ] + + kept = configured_client._drop_quarantined_sources(results) + + assert len(kept) == 1 + assert kept[0].username == 'goodpeer' + + +def test_drop_quarantined_sources_returns_input_when_quarantine_missing(configured_client, tmp_path, monkeypatch): + """No quarantine directory yet (fresh install / never used) β€” + helper returns an empty set; filter returns the input unchanged. + Defaults to today's behaviour for users with no quarantine history.""" + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + results = [_mk_track_result(username='peer', filename='song.flac')] + + kept = configured_client._drop_quarantined_sources(results) + + assert kept == results + + +def test_drop_quarantined_sources_swallows_filesystem_errors(configured_client, monkeypatch): + """If something goes wrong loading the quarantine keys (permissions, + OS quirk, etc.), the filter must NOT break the download pipeline. + Returns input unchanged so legitimate downloads keep working β€” + same defensive contract as the existing 401/connection handlers.""" + import core.soulseek_client as sc + + def _broken_get(key, default=None): + raise RuntimeError("config explosion") + + monkeypatch.setattr(sc.config_manager, 'get', _broken_get) + + results = [_mk_track_result(username='peer', filename='song.flac')] + + kept = configured_client._drop_quarantined_sources(results) + + assert kept == results + + +def test_filter_results_by_quality_runs_quarantine_dedup_first(configured_client, tmp_path, monkeypatch): + """Integration pin: `filter_results_by_quality_preference` calls + the quarantine dedup BEFORE the quality picker. If a candidate is + on the quarantine record, it can't win the picker by virtue of + superior bitrate β€” that's how the #652 loop manifested.""" + import json as _json + quarantine_dir = tmp_path / 'ss_quarantine' + quarantine_dir.mkdir() + + sidecar = { + "context": { + "original_search_result": { + "username": "badpeer", "filename": "high_bitrate_bad.flac", + }, + }, + } + (quarantine_dir / "20260518_120000.json").write_text(_json.dumps(sidecar)) + + import core.soulseek_client as sc + monkeypatch.setattr(sc.config_manager, 'get', + lambda key, default=None: str(tmp_path) if key == 'soulseek.download_path' else default) + + # Mock the DB call inside filter_results_by_quality_preference so the + # test doesn't need a real DB. Quality profile permits FLAC. + class _FakeDB: + def get_quality_profile(self): + return { + 'preset': 'flac', + 'qualities': { + 'flac': {'enabled': True, 'min_kbps': 800, 'max_kbps': 99999}, + 'mp3_320': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0}, + 'mp3_256': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0}, + 'mp3_192': {'enabled': False, 'min_kbps': 0, 'max_kbps': 0}, + }, + 'priority': ['flac'], + } + + import database.music_database as md + monkeypatch.setattr(md, 'MusicDatabase', lambda: _FakeDB()) + + results = [ + # The "bad" source has the BEST quality on paper β€” pre-fix would win the picker. + _mk_track_result(username='badpeer', filename='high_bitrate_bad.flac', + quality='flac', bitrate=1411, size=20_000_000, duration=180_000), + _mk_track_result(username='goodpeer', filename='good.flac', + quality='flac', bitrate=1411, size=20_000_000, duration=180_000), + ] + + kept = configured_client.filter_results_by_quality_preference(results) + + usernames = {r.username for r in kept} + assert 'badpeer' not in usernames, "Quarantined source must be filtered before the quality picker" + assert 'goodpeer' in usernames diff --git a/tests/imports/test_quarantine_management.py b/tests/imports/test_quarantine_management.py index 97aebe04..d2c527c0 100644 --- a/tests/imports/test_quarantine_management.py +++ b/tests/imports/test_quarantine_management.py @@ -5,6 +5,7 @@ from core.imports.quarantine import ( approve_quarantine_entry, delete_quarantine_entry, entry_id_from_quarantined_filename, + get_quarantined_source_keys, list_quarantine_entries, recover_to_staging, serialize_quarantine_context, @@ -285,3 +286,115 @@ def test_recover_removes_sidecar_after_move(tmp_path): recover_to_staging(str(quarantine), str(staging), "20260514_120000_song") assert not sidecar.exists() + + +# ────────────────────────────────────────────────────────────────────── +# get_quarantined_source_keys β€” issue #652 dedup primitive +# ────────────────────────────────────────────────────────────────────── + + +def _write_quarantine_sidecar_with_source(quarantine_dir, entry_id, *, + username=None, filename=None): + """Helper that writes a sidecar matching the shape `move_to_quarantine` + produces β€” `context.original_search_result.{username, filename}` is + the path `get_quarantined_source_keys` pulls from.""" + sidecar = { + "original_filename": "song.flac", + "quarantine_reason": "boom", + "timestamp": "2026-05-14T12:00:00", + "trigger": "acoustid", + } + if username is not None or filename is not None: + sidecar["context"] = { + "original_search_result": { + "username": username or "", + "filename": filename or "", + } + } + path = quarantine_dir / f"{entry_id}.json" + path.write_text(json.dumps(sidecar)) + return path + + +def test_source_keys_empty_for_missing_dir(tmp_path): + """Defensive: caller may pass a path that doesn't exist (config not + initialised, quarantine never used). Don't crash, just return an + empty set β€” Soulseek filter then keeps every candidate.""" + assert get_quarantined_source_keys(str(tmp_path / "nope")) == set() + + +def test_source_keys_empty_for_empty_dir(tmp_path): + """Empty quarantine dir β†’ empty set.""" + assert get_quarantined_source_keys(str(tmp_path)) == set() + + +def test_source_keys_collects_username_filename_tuples(tmp_path): + """Sidecars with `context.original_search_result.username` and + `.filename` round-trip into `(username, filename)` tuples β€” that's + the exact shape the Soulseek candidate filter looks up against.""" + _write_quarantine_sidecar_with_source( + tmp_path, "20260514_120000_a", + username="badpeer", filename="path/to/bad.flac", + ) + _write_quarantine_sidecar_with_source( + tmp_path, "20260514_120100_b", + username="otherpeer", filename="other.mp3", + ) + + keys = get_quarantined_source_keys(str(tmp_path)) + + assert ("badpeer", "path/to/bad.flac") in keys + assert ("otherpeer", "other.mp3") in keys + assert len(keys) == 2 + + +def test_source_keys_skip_legacy_sidecars_without_context(tmp_path): + """Sidecars written pre-Feb 2026 don't have the `context` field β€” + can't gate against them since the originating source is unknown. + Must skip silently rather than crashing the dedup path.""" + _write_quarantine_sidecar_with_source(tmp_path, "legacy_id") # no username/filename + + assert get_quarantined_source_keys(str(tmp_path)) == set() + + +def test_source_keys_skip_sidecars_with_empty_source_fields(tmp_path): + """Defensive: a sidecar with an empty string for username OR filename + can't gate anything meaningfully β€” dropping every result whose + username equals '' would catch unrelated downloads. Skip those + entries entirely.""" + _write_quarantine_sidecar_with_source(tmp_path, "empty_user", username="", filename="x.flac") + _write_quarantine_sidecar_with_source(tmp_path, "empty_file", username="u", filename="") + + assert get_quarantined_source_keys(str(tmp_path)) == set() + + +def test_source_keys_skip_corrupt_sidecars(tmp_path): + """A corrupt JSON sidecar (truncated write, encoding glitch) must + not propagate up and break the dedup path. Filesystem read errors + are swallowed at debug level.""" + bad = tmp_path / "corrupt.json" + bad.write_text("{not valid json") + _write_quarantine_sidecar_with_source( + tmp_path, "good", username="good_peer", filename="good.flac", + ) + + keys = get_quarantined_source_keys(str(tmp_path)) + + assert keys == {("good_peer", "good.flac")} + + +def test_source_keys_dedup_repeated_sources(tmp_path): + """If the SAME `(username, filename)` was quarantined twice (which + is exactly the #652 bug β€” but until now wasn't being prevented), + the set collapses to one entry. The Soulseek filter still acts as + a single-membership check, so a single set entry is enough.""" + _write_quarantine_sidecar_with_source( + tmp_path, "first", username="peer", filename="dupe.flac", + ) + _write_quarantine_sidecar_with_source( + tmp_path, "second", username="peer", filename="dupe.flac", + ) + + keys = get_quarantined_source_keys(str(tmp_path)) + + assert keys == {("peer", "dupe.flac")} diff --git a/webui/static/helper.js b/webui/static/helper.js index 868e5a5f..b38faa41 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3426,6 +3426,7 @@ const WHATS_NEW = { { title: 'Fix: Docker basic-search streaming silently failed under rootless Docker', desc: 'the streaming "Play" flow on the basic search page tried to create `/app/Stream` lazily at runtime, which fails silently when the container runs under rootless Docker / Podman (in-container root can\'t write to `/app`). pre-baked the directory at image build time, matching the same pattern that fixed `/app/Staging` earlier in the cycle. non-persistent β€” no volume needed.' }, { title: 'Fix: slskd-unreachable log spam during non-Soulseek downloads', desc: 'when slskd was configured but not actually running (or unreachable on its configured port), the `/api/downloads/status` polling loop fanned out to every download plugin including Soulseek, producing one `ERROR - Cannot connect to host ... [Name or service not known]` log line per poll for the entire duration of any download β€” visible spam even when the user wasn\'t using Soulseek at all. Connection failures now emit one WARNING with actionable context (start slskd, or clear the slskd_url if you don\'t use Soulseek) and demote subsequent failures to debug. The flag resets on the next successful slskd response so a later outage warns again.' }, { title: 'Fix: MusicBrainz "Other" release-groups now visible in discography', desc: 'MB tags music videos, one-off web releases, and broadcast singles with `primary-type=Other` β€” common pattern for Vocaloid producers, JP indie artists, and some Western indie acts. The release-group browse filter only requested `album|ep|single`, dropping every Other-typed group at the API layer. Combined with the inline type mapper defaulting unknown primary types to "album", this hid legitimate tracks from the artist detail page and left downloaded tracks orphaned (visible in track counts but not bound to any visible album / single card). Centralised the type-mapper into one shared helper (`core/metadata/release_type.py`) used by every provider\'s rawβ†’Album projection, added `Other` and `Broadcast` handling that routes those release-groups into the Singles section, and added `other` to the MB API filter. For inabakumori, this surfaces 5 previously-invisible releases. 23 unit tests pin the mapper contract; existing 65 search-adapter tests still green.' }, + { title: 'Fix: quarantined files no longer re-downloaded on auto-wishlist cycles', desc: 'when a file failed AcoustID verification and got quarantined, the next auto-wishlist run would search for the same track again, and the candidate picker\'s deterministic quality ranking kept selecting the same `(uploader, filename)` source β€” re-downloading and re-quarantining the exact same file every cycle. Users woke up to hundreds of duplicate `.quarantined` entries from a single bad upload. The Soulseek candidate filter now reads quarantine sidecars and drops any candidate whose `(username, filename)` matches a previously-quarantined source before the quality picker ranks it. Filesystem read on every search (sub-ms in practice). Approving or deleting a quarantine entry removes its source from the dedup set automatically β€” the user can give the source a second chance by explicitly approving / deleting the quarantine record.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From 735dd738656c9c78d70dd52e44ba8264087d5734 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 22:09:15 -0700 Subject: [PATCH 22/24] fix(repair): rewire Unknown Artist Fixer deferred imports (#646) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Fix Unknown Artists" repair job crashed on every run with: ImportError: cannot import name '_build_path_from_template' from 'core.repair_jobs.library_reorganize' Commit ca5c9316 ("Rewrite Library Reorganize job to delegate to per- album planner") moved the private path-builder + quality-string helpers out of `core.repair_jobs.library_reorganize` and into the import pipeline. `unknown_artist_fixer.py:163` still imported them from the old module β€” its scan() defers the imports to avoid pulling web_server's Flask boot into the test harness, so the broken target only surfaces at runtime when the user actually runs the job. The tool was completely unrunnable. Re-wired the deferred imports: core.repair_jobs.library_reorganize._build_path_from_template -> core.imports.paths.get_file_path_from_template_raw core.repair_jobs.library_reorganize._get_audio_quality -> core.imports.file_ops.get_audio_quality_string Both replacements have identical signatures + return shapes (verified by inspecting library_reorganize's pre-refactor implementations vs the import-pipeline equivalents): get_file_path_from_template_raw(template: str, context: dict) -> tuple[folder: str, filename_base: str] get_audio_quality_string(file_path: str) -> str No call-site changes needed beyond the import target. 2 new regression tests in `tests/test_unknown_artist_fixer.py`: test_deferred_path_imports_resolve β€” runs the same import statements scan() runs, so the NEXT refactor that moves these helpers fails CI rather than reaching the user. test_deferred_path_helper_shape_matches_fixer_usage β€” pins the `(folder, filename_base)` 2-tuple contract the fixer's unpack relies on. Catches return-shape drift even when the import target stays valid. Audited every consumer of `core.repair_jobs.library_reorganize` β€” only one stale import (this file). The test suite covers the only production caller. 5 fixer tests pass (3 existing + 2 new regression guards). --- core/repair_jobs/unknown_artist_fixer.py | 16 +++++-- tests/test_unknown_artist_fixer.py | 58 ++++++++++++++++++++++++ webui/static/helper.js | 1 + 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/core/repair_jobs/unknown_artist_fixer.py b/core/repair_jobs/unknown_artist_fixer.py index bf037748..774cf32c 100644 --- a/core/repair_jobs/unknown_artist_fixer.py +++ b/core/repair_jobs/unknown_artist_fixer.py @@ -160,8 +160,18 @@ class UnknownArtistFixerJob(RepairJob): # Compute expected file path expected_rel = None if reorganize_files and corrected.get('artist') and corrected.get('album'): - from core.repair_jobs.library_reorganize import _build_path_from_template, _get_audio_quality - quality = _get_audio_quality(resolved) + # Issue #646: `core.repair_jobs.library_reorganize` was + # rewritten in commit ca5c9316 ("Rewrite Library + # Reorganize job to delegate to per-album planner") and + # the private `_build_path_from_template` / + # `_get_audio_quality` helpers moved out. They live in + # the import pipeline now β€” same shape, different + # module. Without this re-wire the Unknown Artist Fixer + # crashes on first scan with `ImportError: cannot + # import name '_build_path_from_template'`. + from core.imports.paths import get_file_path_from_template_raw + from core.imports.file_ops import get_audio_quality_string + quality = get_audio_quality_string(resolved) tmpl_ctx = { 'artist': corrected['artist'], 'albumartist': corrected['artist'], @@ -173,7 +183,7 @@ class UnknownArtistFixerJob(RepairJob): 'quality': quality, 'albumtype': 'Album', } - folder, fname_base = _build_path_from_template(album_template, tmpl_ctx) + folder, fname_base = get_file_path_from_template_raw(album_template, tmpl_ctx) file_ext = os.path.splitext(resolved)[1] if quality and f'[{quality}]' not in fname_base: fname_base = f"{fname_base} [{quality}]" diff --git a/tests/test_unknown_artist_fixer.py b/tests/test_unknown_artist_fixer.py index 4ff15f59..5da80e8d 100644 --- a/tests/test_unknown_artist_fixer.py +++ b/tests/test_unknown_artist_fixer.py @@ -239,3 +239,61 @@ def test_unknown_artist_fixer_supports_hydrabase_title_search(monkeypatch): assert result["source"] == "hydrabase_title_search" assert hydrabase_client.search_calls == [("Hydra Match", 5)] assert spotify_client.search_calls == [] + + +# --------------------------------------------------------------------------- +# Issue #646 β€” deferred imports inside scan() must resolve at runtime +# --------------------------------------------------------------------------- + + +def test_deferred_path_imports_resolve(): + """Issue #646 regression guard. The Unknown Artist Fixer's scan() + defers `get_file_path_from_template_raw` + `get_audio_quality_string` + imports to keep web_server's heavy boot off the test harness β€” but + that means a stale import target only surfaces at *runtime*, mid- + scan, with an ImportError. The fixer crashed with exactly that: + + ImportError: cannot import name '_build_path_from_template' from + 'core.repair_jobs.library_reorganize' + + after commit ca5c9316 rewrote `library_reorganize` and moved the + helpers into the import pipeline. + + This test runs the same import statements scan() runs, so the next + refactor that moves these helpers fails CI rather than reaching the + user.""" + from core.imports.paths import get_file_path_from_template_raw # noqa: F401 + from core.imports.file_ops import get_audio_quality_string # noqa: F401 + + +def test_deferred_path_helper_shape_matches_fixer_usage(): + """Pin the shape contract the fixer relies on: pass a template + string + a context dict with the same keys scan() builds, expect a + `(folder, filename_base)` tuple back. If either of those moves, the + fixer's `folder, fname_base = ...` unpack would fail loudly here + instead of producing a malformed expected_rel path.""" + from core.imports.paths import get_file_path_from_template_raw + + template = "$albumartist/$albumartist - $album/$track - $title" + tmpl_ctx = { + "artist": "Test Artist", + "albumartist": "Test Artist", + "album": "Test Album", + "title": "Test Track", + "track_number": 1, + "disc_number": 1, + "year": "2026", + "quality": "FLAC 16bit", + "albumtype": "Album", + } + + result = get_file_path_from_template_raw(template, tmpl_ctx) + + assert isinstance(result, tuple) and len(result) == 2, \ + "Must return a 2-tuple β€” fixer does `folder, fname_base = result`" + folder, fname_base = result + assert isinstance(folder, str) and isinstance(fname_base, str) + # Folder path must include the album-artist segment from the template. + assert "Test Artist" in folder + # Filename base must include the title from the template. + assert "Test Track" in fname_base diff --git a/webui/static/helper.js b/webui/static/helper.js index b38faa41..0ef26643 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3427,6 +3427,7 @@ const WHATS_NEW = { { title: 'Fix: slskd-unreachable log spam during non-Soulseek downloads', desc: 'when slskd was configured but not actually running (or unreachable on its configured port), the `/api/downloads/status` polling loop fanned out to every download plugin including Soulseek, producing one `ERROR - Cannot connect to host ... [Name or service not known]` log line per poll for the entire duration of any download β€” visible spam even when the user wasn\'t using Soulseek at all. Connection failures now emit one WARNING with actionable context (start slskd, or clear the slskd_url if you don\'t use Soulseek) and demote subsequent failures to debug. The flag resets on the next successful slskd response so a later outage warns again.' }, { title: 'Fix: MusicBrainz "Other" release-groups now visible in discography', desc: 'MB tags music videos, one-off web releases, and broadcast singles with `primary-type=Other` β€” common pattern for Vocaloid producers, JP indie artists, and some Western indie acts. The release-group browse filter only requested `album|ep|single`, dropping every Other-typed group at the API layer. Combined with the inline type mapper defaulting unknown primary types to "album", this hid legitimate tracks from the artist detail page and left downloaded tracks orphaned (visible in track counts but not bound to any visible album / single card). Centralised the type-mapper into one shared helper (`core/metadata/release_type.py`) used by every provider\'s rawβ†’Album projection, added `Other` and `Broadcast` handling that routes those release-groups into the Singles section, and added `other` to the MB API filter. For inabakumori, this surfaces 5 previously-invisible releases. 23 unit tests pin the mapper contract; existing 65 search-adapter tests still green.' }, { title: 'Fix: quarantined files no longer re-downloaded on auto-wishlist cycles', desc: 'when a file failed AcoustID verification and got quarantined, the next auto-wishlist run would search for the same track again, and the candidate picker\'s deterministic quality ranking kept selecting the same `(uploader, filename)` source β€” re-downloading and re-quarantining the exact same file every cycle. Users woke up to hundreds of duplicate `.quarantined` entries from a single bad upload. The Soulseek candidate filter now reads quarantine sidecars and drops any candidate whose `(username, filename)` matches a previously-quarantined source before the quality picker ranks it. Filesystem read on every search (sub-ms in practice). Approving or deleting a quarantine entry removes its source from the dedup set automatically β€” the user can give the source a second chance by explicitly approving / deleting the quarantine record.' }, + { title: 'Fix: Unknown Artist Fixer tool crashed on every run', desc: 'the "Fix Unknown Artists" repair job crashed instantly with `ImportError: cannot import name \'_build_path_from_template\'` β€” totally unrunnable. Commit ca5c9316 ("Rewrite Library Reorganize job to delegate to per-album planner") moved the private path-builder + quality-string helpers out of `core.repair_jobs.library_reorganize` and into the import pipeline (`core.imports.paths` / `core.imports.file_ops`), but the Unknown Artist Fixer\'s deferred-import path was left pointing at the old module. Re-wired to the new locations. Added two regression tests that exercise the deferred imports directly β€” the next refactor that moves these helpers fails CI rather than reaching the user.' }, ], '2.5.5': [ { date: 'May 17, 2026 β€” 2.5.5 release' }, From a685f9ca4a86ea41b6056ffe120bda12248aa9f9 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 19 May 2026 22:31:29 -0700 Subject: [PATCH 23/24] diag: log every cancel_download caller with a trigger label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnostic-only change for issue Technodude reported: Tidal sync-playlist downloads getting mass-cancelled mid-flight with no clear cause in the logs. App.log shows ~91 second gaps between Tidal download start and cancel β€” matches the monitor's 90s queue-timeout exactly β€” but none of the monitor's WARNING log lines fire, so the trigger is ambiguous between five `_should_retry_task` paths, three web_server cancel paths, and the API endpoints. Added a single `[CancelTrigger: