Commit graph

2871 commits

Author SHA1 Message Date
Broque Thomas
d556ec0fa7 Bump version to 2.4.3 + make sidebar version dynamic
- `_SOULSYNC_BASE_VERSION` 2.4.2 → 2.4.3
- helper.js — flip 2.4.3 WHATS_NEW header to "May 8, 2026 — 2.4.3
  release"; bump fallback default from 2.4.2 → 2.4.3
- docker-publish.yml — manual-trigger default tag 2.4.2 → 2.4.3

Drive-by — make sidebar version + version-modal subtitle dynamic.
The sidebar version button (`v2.4.1`) and version-modal subtitle
(`Version 2.4.1 — Latest Changes`) were hardcoded text in the HTML.
2.4.2 shipped without these getting bumped — silent drift, easy to
miss at every release.

Added a Flask context_processor that injects `soulsync_version` and
`soulsync_base_version` into every template, then templated the two
hardcoded values:

  v{{ soulsync_base_version }}
  Version {{ soulsync_base_version }} — Latest Changes

Now bumping `_SOULSYNC_BASE_VERSION` updates the UI everywhere it's
rendered. No more "I forgot to bump the sidebar" at release.

2232/2232 full suite green. Ruff clean. JS parses clean.
2026-05-08 09:17:20 -07:00
BoulderBadgeDad
d7ab37e3b9
Merge pull request #525 from Nezreka/fix/discover-track-selection-correction
Fix/discover track selection correction
2026-05-08 09:04:50 -07:00
Broque Thomas
d75ae48981 Discover: sharpen track selection (diversity, source-aware popularity, library dedup, SQL genre)
Four selection-quality fixes on the SoulSync-made discover playlists.
None change public method signatures; all are tightenings on what's
already there.

(1) Diversity for Hidden Gems + Discovery Shuffle

Both used to be `RANDOM() LIMIT N` with no diversity. Could return
50 tracks from one artist or 20 from one album if the discovery
pool happened to be skewed. Both now over-fetch 3x and run the
existing `_apply_diversity_filter`:

- Hidden Gems: max 2 per album, 3 per artist
- Discovery Shuffle: max 2 per album, 2 per artist (tighter — shuffle
  should feel maximally varied)

(2) Source-aware popularity thresholds

`popularity >= 60` for "Popular Picks" and `popularity < 40` for
"Hidden Gems" was Spotify-shaped (0-100 scale). Deezer writes its
`rank` value into that column (often six-digit integers); iTunes
writes nothing meaningful. For Deezer-primary users:
- Popular Picks pulled essentially everything (rank >= 60 = all)
- Hidden Gems pulled essentially nothing (rank < 40 = none)

New `_get_popularity_thresholds(source)` helper returns per-source
values:

- Spotify: (60, 40) — the existing 0-100 scale
- Deezer: (500_000, 100_000) — ballpark from real rank values
- iTunes / unknown: (None, None) — skip the popularity filter
  entirely, fall back to random + diversity

`get_popular_picks` and `get_hidden_gems` now consult the helper.
When threshold is None they skip the popularity SQL filter. Diversity
+ ID gate still apply.

(3) Push genre keyword filter into SQL

`get_genre_playlist` used to fetch `limit=1_000_000` rows into Python
then run a substring keyword filter on `artist_genres`. Bad on big
discovery pools.

Now the keyword OR chain is generated as SQL placeholders:

    AND (artist_genres LIKE ? OR artist_genres LIKE ? OR ...)

Each placeholder gets `f'%{keyword.lower()}%'` via `extra_params`.
`fetch_limit` drops back to `limit * 10`. `_genre_matches` Python
helper deleted (only intra-file caller; verified via grep).

Parent-genre expansion via `GENRE_MAPPING` preserved — keywords list
feeds the LIKE chain unchanged.

(4) Filter out tracks already in library

Discovery pool can include tracks the user already owns. Hidden Gems
/ Shuffle / Popular Picks shouldn't surface those.

`_select_discovery_tracks` gained `exclude_owned: bool = True`
parameter. When True, adds a correlated NOT EXISTS subquery against
the `tracks` table covering all 3 source IDs:

    AND NOT EXISTS (
        SELECT 1 FROM tracks t WHERE
            (t.spotify_track_id IS NOT NULL AND t.spotify_track_id = discovery_pool.spotify_track_id)
         OR (t.itunes_track_id IS NOT NULL AND t.itunes_track_id = discovery_pool.itunes_track_id)
         OR (t.deezer_id IS NOT NULL AND t.deezer_id = discovery_pool.deezer_track_id)
    )

Note column-name asymmetry: tracks.deezer_id vs
discovery_pool.deezer_track_id. Inline comment marks the trap. All
5 public discovery methods automatically benefit (default True).
Seasonal Playlist doesn't go through the helper so it's unaffected
(curated content, dedup is wrong intent there).

Tests

12 new tests in `tests/test_personalized_playlists_id_gate.py` (27
total in the file):

- Hidden Gems + Discovery Shuffle apply diversity (cap proven by
  inserting 10 same-artist + same-album rows and asserting return
  count ≤ per-album cap)
- Popularity thresholds: Spotify (60, 40), Deezer larger scale,
  iTunes None / None
- Popular Picks skips threshold filter when None
- Genre playlist pushes filter to SQL (parent + child genre expansion)
- Owned-track exclusion: filtered when match, kept when no match,
  opt-out flag works
- Deezer column-name asymmetry pinned (regression footgun)

Test fixture re-added the minimal `tracks` table (4 columns: id,
spotify_track_id, itunes_track_id, deezer_id) — only what the new
NOT EXISTS subquery needs to join. Plus `insert_library_track`
helper.

Verification

- 27/27 in this test file pass (15 prior + 12 new)
- 2232/2232 full suite green
- ruff clean

LOC delta:
- core/personalized_playlists.py: 1030 → 1101 (+71)
- tests/test_personalized_playlists_id_gate.py: 352 → 616 (+264)
2026-05-08 08:49:22 -07:00
Broque Thomas
d123581a39 Fix: ID gate missed Deezer-track-id-only rows
The original gate baked into `_select_discovery_tracks` only checked
Spotify + iTunes:

    AND (spotify_track_id IS NOT NULL OR itunes_track_id IS NOT NULL)

For Deezer-primary users, discovery_pool rows have populated
`deezer_track_id` but NULL Spotify + NULL iTunes IDs. The gate
filtered every row out — Time Machine, Genre Browser, Hidden Gems,
Discovery Shuffle, Popular Picks all rendered "no tracks found" for
every tab on every Deezer-primary install.

Extended the gate to include `deezer_track_id` and added that column
to the standard SELECT column tuple. `_build_track_dict` already
exposed `deezer_track_id` in its output shape, so frontend rendering
needed no changes.

Regression pinned via new test
`test_discovery_helper_accepts_deezer_only_id_rows` — inserts a row
with NULL Spotify + NULL iTunes but a populated `deezer_track_id`
and asserts it survives the gate.

2220/2220 full suite green.
2026-05-08 07:46:09 -07:00
Broque Thomas
959562f6b0 Delete Recently Added / Top Tracks / Forgotten Favorites / Familiar Favorites
Owner decision: not worth shipping. The four library-driven personalized
sections were stubbed returning [] for ages because their schema
prereqs didn't exist; the prior commit re-enabled them by routing
through a new `_select_library_tracks` helper. Owner reviewed and chose
to delete the sections entirely instead.

Removed everywhere:

- `core/personalized_playlists.py` — `get_recently_added`,
  `get_top_tracks`, `get_forgotten_favorites`, `get_familiar_favorites`
  + the `_select_library_tracks` helper (no other callers; verified
  via grep).
- `web_server.py` — 4 route handlers
  (`/api/discover/personalized/recently-added`, `top-tracks`,
  `forgotten-favorites`, `familiar-favorites`).
- `webui/index.html` — 4 `<div class="discover-section">` blocks
  (`#personalized-recently-added`, `#personalized-top-tracks`,
  `#personalized-forgotten-favorites`,
  `#personalized-familiar-favorites`).
- `webui/static/discover.js` — 4 load functions
  (`loadPersonalizedRecentlyAdded`, `loadPersonalizedTopTracks`,
  `loadPersonalizedForgottenFavorites`, `loadFamiliarFavorites`),
  plus their entries in `loadDiscoverPage`'s Promise.all, plus
  4 module-level state vars + 6 dead branches across
  `openDownloadModalForDiscoverPlaylist` / `startDiscoverPlaylistSync`
  and the sync-progress / rehydrate dispatchers.
- `webui/static/helper.js` — 4 tooltip / docs entries.
- `webui/static/sync-spotify.js` — 1 stale rehydrate dispatcher
  branch (`discover_familiar_favorites`) caught during the global
  grep pass.
- `tests/test_personalized_playlists_id_gate.py` — 3 library-method
  tests + the test infrastructure that supported them
  (`tracks` schema, `insert_library_track` helper). Documentation
  header updated to reflect the deletion.

Net: -527 / +2 lines across 7 files.

What stays:

- Daily Mixes (also in personalized package, intentionally paused —
  separate decision).
- Popular Picks + Hidden Gems + Discovery Shuffle (alive, not
  affected by this deletion).
- All 14 tests in the personalized-playlists test file still pass.
- The PersonalizedPlaylistsService lift from the prior commit
  (`_select_discovery_tracks` etc) — those are still in active use
  by the surviving discovery_pool methods.

DISCOVER_TRACK_SELECTION_REVIEW.md at repo root contains historical
references to the four deleted endpoints. Treated as historical
context (same policy as WHATS_NEW), left alone.

2219/2219 full suite green (was 2222 - 3 deleted tests = 2219).
JS parses clean, ruff clean.
2026-05-08 07:31:51 -07:00
Broque Thomas
44dd7f980f Discover: unify Decade + Genre tabbed browsers
Both tabbed-browser sections — Time Machine ("Decade") and Browse by
Genre — re-implemented the same lifecycle by hand: fetch tabs list,
render the tab strip, attach click handlers, fetch content per tab,
render track list with sync + download action buttons + sync-status
block, handle empty/error/loading states. ~314 lines of identical
boilerplate split across two browsers.

Lifted into one shared `createTabbedBrowserSection(config)` helper.
Each browser is now a thin wrapper:

```js
const ctrl = createTabbedBrowserSection({
    id: 'decade-browser',
    tabsContainerEl: '#decade-tabs',
    contentContainerEl: '#decade-content',
    fetchTabs: async () => { ... },
    renderTabButton: (tab, isActive) => `<button>...</button>`,
    fetchTabContent: async (tab) => { ... },
    renderTabContent: (tracks, tab) => `...`,
    onTabContentRendered: (tab, contentEl) => { ... },
    emptyMessage / errorMessage,
});
```

Migrated:

- `loadDecadeBrowserTabs` 85 → 3 lines
- `loadDecadeTracks` 67 → 3 lines
- `loadGenreBrowserTabs` 92 → 3 lines
- `loadGenreTracks` 70 → 3 lines

Helper: ~125 lines + ~100 lines of per-browser config blocks +
~25 lines of shared `_renderTabbedTrackList` (the two browsers had
byte-identical track-row markup so it lifted cleanly).

Public function names preserved — the four migrated functions stay
on the same signature so existing callers (`loadDiscoverPage`,
refresh buttons, inline handlers) don't change.

Side effects preserved — `decadeTracksCache[year]`, `activeDecade`,
`genreTracksCache[name]`, `activeGenre`, `availableGenres` still
mutated at the same lifecycle moments. The decade-specific
`startDecadeSync(decade)` and genre-specific `startGenreSync(name)`
sync-button handlers stay where they are; they're click handlers
attached to rendered content, not part of the tab lifecycle.

What didn't fit (intentionally left alone):

- `_renderCompactTrackRow` (the existing shared track-row helper) is
  NOT used by the tabbed browsers — they had their own template
  with a `track_data_json` fallback chain `_renderCompactTrackRow`
  doesn't do. Unifying these two would change behavior for
  non-tabbed sections, so the tabbed-browser variant lives as
  `_renderTabbedTrackList`. Future cleanup could merge them by
  giving `_renderCompactTrackRow` an opt-in fallback flag.
- `switchDecadeTab` / `switchGenreTab` still know about cache shape
  so they can skip refetch on already-loaded tabs. Keeping that
  in the per-browser switch is fine — it's a click handler, not
  lifecycle.

Net: 8546 → 8578 LOC on `discover.js` (+32). Helper boilerplate
offsets the line count, but the win is single-source-of-truth, not
raw line reduction.

`node --check` clean. 2222/2222 full suite green.
2026-05-08 07:15:37 -07:00
Broque Thomas
0701bcc213 PersonalizedPlaylistsService: bake in ID-validity gate, lift selectors
User-facing bug found in the discover-page audit: multiple sections
(hidden gems, discovery shuffle, popular picks, decade browser,
genre browser) had no `WHERE (spotify_track_id IS NOT NULL OR
itunes_track_id IS NOT NULL ...)` gate. Tracks with no source IDs
in the discovery pool got displayed, the user clicked download, the
download silently failed because there was nothing to look up.

Lift + gate

`PersonalizedPlaylistsService` had 5 selection methods that all shared
the same shape — connect to DB, run a SELECT against `discovery_pool`
with different WHERE clauses, optionally apply diversity, return
list of track dicts. ~366 lines of business logic, ~55% of which was
repeated boilerplate.

Three new private helpers consolidate everything:

- `_select_discovery_tracks(*, source, extra_where, extra_params,
  order_by, fetch_limit, extra_columns)` — shared SELECT against
  `discovery_pool`. The mandatory ID gate is hard-coded into the
  WHERE clause: no opt-out flag, every method inherits it for free.
  Plus the source filter and the blacklist filter — same shape every
  selector needs.
- `_apply_diversity_filter(tracks, *, max_per_album, max_per_artist,
  limit)` — per-album / per-artist cap loop, returns trimmed list.
  Lifted from the inline duplicates in decade / genre / popular_picks.
- `_compute_adaptive_diversity_limits(tracks, *, relaxed=False)` —
  step-function tiers based on unique-artist count. `relaxed=True`
  gives the slightly looser limits the genre playlist used vs the
  decade playlist.

Re-enable 4 library methods

`get_recently_added`, `get_top_tracks`, `get_forgotten_favorites`,
`get_familiar_favorites` were all stubs (`return []`) because they
predated the schema columns they need. Schema now has them:
`tracks.created_at`, `tracks.play_count`, `tracks.last_played`, and
the source ID columns added in earlier work.

New `_select_library_tracks(*, where_clause, params, order_by, limit)`
helper mirrors the discovery selector but targets the `tracks` table
joined against `albums` + `artists`. Mandatory ID gate lives in the
helper too: every library method automatically rejects rows where
spotify_track_id, itunes_track_id, deezer_id,
musicbrainz_recording_id, AND audiodb_id are all NULL.

Selection rules:

- `get_recently_added` — ORDER BY created_at DESC
- `get_top_tracks` — WHERE play_count > 0 ORDER BY play_count DESC
- `get_forgotten_favorites` — WHERE play_count > 5 AND last_played
  < (now - 90 days) ORDER BY play_count DESC
- `get_familiar_favorites` — WHERE play_count BETWEEN 3 AND 15

Tests

`tests/test_personalized_playlists_id_gate.py` — 17 tests pinning:

- `_select_discovery_tracks` filters NULL-id rows, honors source +
  blacklist + extra_where
- `_apply_diversity_filter` caps per-album + per-artist + stops at
  limit
- `_compute_adaptive_diversity_limits` returns the right tier for
  unique-artist count + relaxed flag
- All 5 discovery methods (decade, popular_picks, hidden_gems,
  discovery_shuffle, genre is exercised via the helper) reject
  NULL-id rows
- All 4 library methods reject NULL-id rows + honor their
  play-count rules

Behavior preserved

Same diversity tiers, same over-fetch multipliers (10x for decade /
genre, 3x for popular_picks), same `popularity DESC, RANDOM()`
ordering, same `popularity >= 60` / `< 40` thresholds, same
blacklist filter. Public method signatures unchanged — `web_server.py`
needs zero edits.

Net file: 1089 → ~1170 LOC (helpers + docstrings), but actual
business logic across the 9 methods went from ~418 lines down to
~195 (-53%).

2222/2222 full suite green (was 2205 + 17 new). Ruff clean.
2026-05-08 07:14:36 -07:00
BoulderBadgeDad
1f2b8f8ccd
Merge pull request #522 from Nezreka/feat/discover-section-controller
Feat/discover section controller
2026-05-07 20:51:56 -07:00
Broque Thomas
c557d9196e Discover controller — Cin pre-review polish
Three changes tightening the controller before opening the PR.

DROP MAGIC `extractItems` DEFAULTS

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

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

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

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

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

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

PIN THE CONTROLLER CONTRACT WITH JS TESTS

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

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

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

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

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

VERIFICATION

- 2205/2205 full pytest suite green (was 2204 + 1 new wrapper)
- 32/32 `node --test` pass on the controller test file directly
- ruff clean
- node --check clean on all touched JS files
2026-05-07 20:35:10 -07:00
Broque Thomas
dc2323cde6 Discover cleanup: controller extensions, toast errors, migrate skipped sections
Follow-up to the controller migration commits. Closes out the
extension list the per-section migrations surfaced as needed.

CONTROLLER EXTENSIONS

- Callable `fetchUrl: () => string` — resolves the seasonal-playlist
  recreate-on-key-change hack from the prior commit.
- No-fetch `data:` mode — value or `() => value`. Lets render-only
  sections like Seasonal Albums use the controller without inventing
  a fake endpoint. Mutually exclusive with `fetchUrl`; validated up
  front so misuse fails at register-time.
- `beforeLoad(ctx)` hook — runs before the spinner shows. Lets
  dynamically-inserted sections like Because You Listen To ensure
  their `contentEl` exists before the visibility check.
- `onSuccess(data, ctx)` hook — runs after the success gate but
  before isEmpty / isStale. Cleaner home for sibling header /
  subtitle / button updates than folding them into renderItems.
- `isStale(items, data)` + `onStale(ctx)` + `renderStale(items, data)`
  + `staleMessage` — third render state for "data is empty BUT
  upstream is still discovering". Stale wins over empty when both
  apply. Default stale UI is the same spinner block used elsewhere.
- `showErrorToast: true` config — opens a global `showToast(...)` in
  addition to the in-section error block. Default off; sections that
  have no recovery action shouldn't shout at the user.
- `renderItems` returning null/undefined now leaves contentEl
  untouched. Lets a renderer do its own DOM manipulation (e.g.
  delegating to an existing grid-render fn that targets a child
  element) without fighting the controller's innerHTML swap.

MIGRATED THE 2 SKIPPED SECTIONS

- `loadYourAlbums` — uses `isStale`/`onStale`/`renderStale` for the
  stale-fetch state, `onSuccess` for the subtitle/filters/download
  side-effects, `hideWhenEmpty` + `sectionEl` for the truly-empty
  case, `renderItems` returning null since it delegates to the
  existing `_renderYourAlbumsGrid` + `_renderYourAlbumsPagination`.
- `loadSeasonalAlbums` — uses no-fetch `data:` mode because the
  parent `loadSeasonalContent` already fetched the season payload.
  `beforeLoad` updates the sibling title/subtitle text.

ERROR TOASTS ON ALL MIGRATED SECTIONS

Every migrated section now has `showErrorToast: true`. Section load
failures surface a global toast instead of silently spinning forever
or swallowing into console.debug. Same pattern JohnBaumb #369 asked
for at the Python layer, applied at the UI layer.

SHARED SYNC-STATUS BLOCK

Lifted the duplicated decade-tab + genre-tab sync-status HTML
(✓ completed |  pending | ✗ failed | percentage) into a single
`_renderSyncStatusBlock(idPrefix)` helper. Two call sites now share
one implementation. ListenBrainz playlists keep their own block
because the semantics differ — matching progress (total / matched /
failed) vs download progress.

DEAD-SECTION AUDIT — NONE DEAD

Audited the 13 supposedly-dead hidden sections from
DISCOVER_REVIEW.md. All 13 are alive: gated on user data (discovery
pool, library content, metadata cache) and self-surface when their
data exists via `style.display = 'block'` on the success path. The
review's grep missed the toggle. No deletions made.

DAILY MIXES ORPHAN CALL

Removed the orphaned `loadPersonalizedDailyMixes()` call from
`blockDiscoveryArtist` — Daily Mixes is intentionally paused (its
load call in `loadDiscoverPage` is commented out) so refreshing it
from the post-block hook was a no-op.

2204/2204 full suite green. JS parses clean (`node --check`).
2026-05-07 20:05:39 -07:00
Broque Thomas
4ee78bb973 Migrate 7 more discover sections to the shared controller
Follow-up to the foundation commit. Drops the hand-rolled
try/catch + spinner injection + empty-state HTML + error-swallow
in seven sections by routing them through
`createDiscoverSectionController`. Each section keeps its existing
public function name + signature so callers, refresh buttons, and
dashboard wiring don't notice the swap.

Migrated:

- `loadDiscoverReleaseRadar` (Fresh Tape)
- `loadDiscoverWeekly` (The Archives)
- `loadDecadeBrowser` (Time Machine intro carousel)
- `loadGenreBrowser` (Browse by Genre intro carousel)
- `loadSeasonalPlaylist` (Seasonal Mix)
- `loadYourArtists`
- `loadBecauseYouListenTo`

Skipped (don't fit the controller's single-fetch / single-render-target
shape):

- `loadYourAlbums` — paginated grid + filters, updates four separate
  UI elements (subtitle, filter chips, download button, grid).
- `loadSeasonalAlbums` — receives pre-fetched data from
  `loadSeasonalContent`; no fetch URL to satisfy.

Hidden / dead sections (~13 of them — `loadPersonalized*`,
`loadDiscoveryShuffle`, `loadFamiliarFavorites`, `loadCache*`)
untouched in this pass. Separate audit commit will surface or kill
them.

Two side-effects worth noting:

- `loadDecadeBrowser` and `loadGenreBrowser` migrated for
  completeness, but neither appears wired into `loadDiscoverPage` or
  any inline handler. May be dead code — flagged for the audit pass.
- `loadSeasonalPlaylist` needs a per-load fetch URL (varies by
  `currentSeasonKey`); worked around by recreating the controller
  when the key changes. Cleaner option: extend the controller to
  accept a `fetchUrl: () => string` callable form. Tracked in the
  follow-up extension list below.

Controller extension candidates surfaced for follow-up:

- Callable `fetchUrl` (resolves the seasonal playlist
  recreate-on-key-change hack)
- Explicit `isStale` / `onStale` hook (so Your Artists doesn't
  fold stale handling into renderItems)
- `beforeLoad` / `ensureContentEl` hook (so Because You Listen To
  can let the controller own the dynamic container creation)
- No-fetch `data:` mode (so render-only sections like Seasonal
  Albums can use the controller too)
- `onSuccess(data)` hook (cleaner home for header / subtitle
  side-effects vs folding them into renderItems)

Net: -76 lines in `discover.js` even after adding the per-section
render helpers. 2204/2204 full suite green. JS parses clean.
2026-05-07 19:21:19 -07:00
Broque Thomas
07a71f0432 Discover section controller foundation + migrate Recent Releases
Every section on the discover page (Recent Releases, Your Artists,
Your Albums, Seasonal Albums, Seasonal Mix, Fresh Tape, The Archives,
Build Playlist, Time Machine, Browse by Genre, ListenBrainz Playlists,
Because You Listen To, plus ~13 hidden sections) currently
re-implements the same lifecycle by hand:

  1. show a loading spinner in the carousel container
  2. fetch the section's endpoint
  3. parse the response, decide if the data is empty
  4. either render the items, show an empty-state, or show an error
  5. wire post-render handlers (download buttons, hover behavior, etc)
  6. maybe expose refresh()

~30 sections worth of duplicated boilerplate, all subtly drifting.
Different empty-state messages. Different error handling (some
`console.debug`, some silently swallowed, some leave the spinner
spinning forever). Different sync-status icons (✓//✗ vs ♪/✓/✗).
No consistent error toast.

Lifted the lifecycle into a shared `createDiscoverSectionController`
in `webui/static/discover-section-controller.js`. Renderers stay
per-section because section data shapes legitimately differ — album
cards vs artist circles vs playlist tiles vs track rows. The
controller is the wrapper, not a forced visual abstraction.

Foundation contract:

  createDiscoverSectionController({
    id: 'recent-releases',          // for diagnostic logging
    contentEl: '#carousel',          // selector or Element
    fetchUrl: '/api/discover/...',
    extractItems: (data) => [...],   // pull list from response
    renderItems: (items, data, ctx) => '<html>',
    onRendered: (ctx) => { ... },    // optional post-render hook
    loadingMessage / emptyMessage / errorMessage: copy
    sectionEl + hideWhenEmpty: optional whole-section visibility
    isSuccess / isEmpty: optional gate overrides
  })

Returns `{ load, refresh, destroy, getState }`. Validates config up
front so misuse fails at register-time, not silently on load. Coalesces
concurrent loads (same in-flight promise returned) so a double-click
or repeated trigger doesn't double-fetch. `refresh()` bypasses the
coalesce so the refresh button always re-fires. Errors are logged
(console.debug by default, console.error when verboseErrors=true).

Renderer hook errors are caught + logged so a buggy render callback
can't tear down the controller — keeps the page resilient.

Migrated `Recent Releases` as the proof — simplest album-card shape,
no source-gating, no refresh button. Verified the contract covers it
end-to-end. The legacy `loadDiscoverRecentReleases` entry-point stays
public so existing callers don't change; internally it lazy-builds
the controller and triggers `load()`.

NOT in this commit:

- Other section migrations (one section per follow-up commit, keeps
  reviews small + lets us sequence the work)
- Registry-driven section list (so the dead-section audit becomes
  registry deletions instead of section-by-section removal)
- Global error toast wrapper
- Per-section "requires X primary source" gate
- Sync-status icon renderer unification

Once every section is on the controller, the discover-page cleanup
work (kill the 13 dead sections, standardize sync-status icons, add
error toasts) becomes single-line registry-level edits instead of
30 separate section-by-section rewrites.

2204/2204 full suite green. JS parses clean (`node --check`). Manual
smoke deferred until follow-up commits — Recent Releases unchanged
on the wire (same endpoint, same payload shape, same render output).
2026-05-07 18:14:56 -07:00
BoulderBadgeDad
6637c29964
Merge pull request #520 from Nezreka/release/2.4.2
Bump version to 2.4.2
2026-05-07 16:14:54 -07:00
Broque Thomas
6aafcaae93 Bump version to 2.4.2
- `web_server.py` — `_SOULSYNC_BASE_VERSION` 2.4.1 → 2.4.2
- `webui/static/helper.js` — flip the 2.4.2 WHATS_NEW header from
  "Unreleased — 2.4.2 dev cycle" to "May 7, 2026 — 2.4.2 release"
  so the per-version block stops being filtered out by
  `_getLatestWhatsNewVersion`. Also bumps the safety-net default
  inside that helper from 2.4.1 → 2.4.2.
- `.github/workflows/docker-publish.yml` — manual-trigger default
  tag bumped to match.

Drive-by fix: escaped a stray single quote in the `Internal: Download
Engine` 2.4.2 entry that broke `node --check` on the file
(`orchestrator.client('soulseek')` inside a single-quoted desc string
silently terminated the string mid-entry). Pre-existing, unrelated to
the bump but caught while validating JS parse for the release.

VERSION_MODAL_SECTIONS not rotated in this commit — separate
editorial pass.
2026-05-07 16:11:25 -07:00
BoulderBadgeDad
f2fb66340a
Merge pull request #519 from Nezreka/feat/artist-top-tracks-download
Add download buttons + bulk action to artist top-tracks sidebar
2026-05-07 15:50:08 -07:00
Broque Thomas
1a2da016e4 Add download buttons + bulk action to artist top-tracks sidebar
Closes #513 (s66jones).

The artist detail page already showed a "Popular on Last.fm" sidebar —
list of an artist's top tracks by playcount, with a play button per row
but no download action. Issue #513 wanted a way to grab those tracks
the same way zotify let users grab "top X songs" without pulling the
full discography.

Pulls from the configured primary metadata source (Spotify
`artist_top_tracks`, Deezer `/artist/{id}/top`) when available, falls
back to the existing Last.fm display-only mode for sources that don't
expose popularity ranking (iTunes / Discogs / MusicBrainz). Source
label in the section title shifts to match.

Each row gets a hover-revealed download button that wishlists the
single track via the existing /api/add-album-to-wishlist endpoint
(preserves the track's real album metadata, so the wishlist worker
later places the file in its proper album folder).

A "Download All" footer button opens the standard download modal in
PLAYLIST context, not album context — the virtual playlist_id is
`top_tracks_<source>_<artistId>` which doesn't match any of the
album-prefix checks in `startMissingTracksProcess` (downloads.js).
That keeps `is_album_download=false`, so the master worker doesn't
inject a wrapper context as `_explicit_album_context`. Each track
downloads using its own real album metadata, files land in proper
per-album folders on disk (not a fake "Top Tracks" folder).

Backend additions:

- `SpotifyClient.get_artist_top_tracks(artist_id, country, limit)` —
  wraps `spotipy.artist_top_tracks`, returns up to 10 tracks for the
  market (Spotify's API cap). UI-side limit trim only.
- `DeezerClient.get_artist_top_tracks(artist_id, limit)` — wraps
  `/artist/{id}/top?limit=N`, converts Deezer's raw shape to the same
  Spotify-compatible dict layout (id, name, artists, album with
  album_type / total_tracks / images, duration_ms, track_number,
  disc_number) so downstream code doesn't branch on source.
- `GET /api/artist/<id>/top-tracks` — dispatches to whichever client
  matches the primary source. Resolves per-source artist IDs from the
  DB row first (matching what /discography already does) so a Spotify
  ID in the URL still works when Deezer is primary, and vice versa.
  Returns `{success, source, tracks, resolved_artist_id}` on hit;
  `{success: False, reason: 'unsupported_source' | 'spotify_not_authenticated'
  | 'deezer_unavailable' | 'no_tracks_found'}` on miss so the frontend
  can decide whether to fall through to Last.fm.

Frontend:

- `_loadArtistTopTracks` tries the metadata source first, falls
  through to the legacy `/api/artist/0/lastfm-top-tracks` call if the
  source can't deliver. Section title and per-row UI shift based on
  which source answered.
- New per-row `.hero-top-track-download` button (hover-revealed).
- New `.hero-top-tracks-download-all` footer button — only visible
  when metadata-source mode rendered the list (Last.fm fallback hides
  it since rows have no track IDs to download).

Tests: 10 new tests pin the client methods —
- Spotify: returns track list, honors UI limit cap, returns empty when
  unauthed / artist_id missing / API throws.
- Deezer: shape conversion to Spotify-compatible dict, empty when no
  data / artist_id missing, limit clamping at upper bound, default
  fallback when limit=0, malformed entries skipped.

The Flask endpoint dispatcher itself isn't covered by the new test
file because importing web_server at test-collection time spins up
worker threads that race with caplog-using tests elsewhere in the
suite (specifically test_library_reorganize_orchestrator). Endpoint
verified manually; the underlying client methods (the load-bearing
logic) are covered.

2204/2204 full suite green (was 2194 + 10 new).
2026-05-07 15:44:47 -07:00
Broque Thomas
dd48dc8c6e Update style.css 2026-05-07 14:03:14 -07:00
BoulderBadgeDad
5eff659220
Merge pull request #518 from Nezreka/fix/acoustid-version-mismatch
Reject AcoustID matches whose version disagrees with the expected track
2026-05-07 13:42:58 -07:00
Broque Thomas
01c528fd5f Reject AcoustID matches whose version disagrees with the expected track
Discord report (corruption [BWC]): downloads coming through as the
instrumental cut when a vocal track was requested. The verification
step's `_normalize` function strips parentheticals and version-suffix
tags ("(Instrumental)", "- Live", etc) so legitimate name variations
don't false-fail the title-similarity check. That also means "In My
Feelings" and "In My Feelings (Instrumental)" both normalize to "in
my feelings", title similarity is 1.0, and the wrong cut passes
verification.

Detect the version label on each side BEFORE normalization runs. If
the expected and matched recordings disagree on version (one is
original, the other is instrumental / live / acoustic / remix /
etc), return FAIL — the fingerprint identified a real song, just
not the version the caller asked for.

Reuses `MusicMatchingEngine.detect_version_type` so the same regex
patterns the pre-download Soulseek matcher applies also drive
post-download verification. No duplicated tables.

Also gates the secondary fallback scan, so a wrong-version variant
sitting in the same fingerprint cluster can't win the loop after
the best match has already been version-rejected.

6 tests pin the behavior:
- instrumental returned for vocal request → FAIL
- vocal returned for instrumental request → FAIL
- live vs acoustic → FAIL
- matching versions on both sides → PASS
- original-to-original happy path → PASS (regression guard)
- secondary scan skips wrong-version recordings → not PASS

2194/2194 full suite green (was 2188 + 6 new).
2026-05-07 13:25:30 -07:00
BoulderBadgeDad
caef3dc9f1
Merge pull request #517 from Nezreka/fix/primary-source-non-admin-profiles
Fix non-admin profiles defaulting to Spotify on search picker
2026-05-07 11:58:03 -07:00
Broque Thomas
caa1c198e5 Fix non-admin profiles defaulting to Spotify on search picker
Closes #515 (jaruca).

Search-picker controller in shared-helpers.js resolved the user's
configured primary metadata source by fetching `/api/settings`. That
endpoint is `@admin_only` (it returns full config including
credentials), so non-admin profiles got a 403 and the controller
silently fell back to the hardcoded `'spotify'` default — admin's
chosen source (deezer / itunes / discogs / etc) was ignored on every
non-admin profile, forcing manual reselection each session.

Switched to `/status`, which is public and already exposes the
resolved `metadata_source` for the dashboard. Same value the picker
needs — different endpoint that doesn't gate non-admins.

Admins see no behavior change. Non-admins now see admin's configured
primary source as the default active icon.

Refs #515
2026-05-07 11:53:02 -07:00
BoulderBadgeDad
627d32cebd
Merge pull request #516 from Nezreka/fix/silent-exception-swallowing
Fix/silent exception swallowing
2026-05-07 11:28:43 -07:00
Broque Thomas
9602d1827c Final silent-exception sweep + ruff S110 lint guardrail — ~45 sites
Catches the silent excepts the awk-based earlier sweeps missed:

- Bare `except:` followed by `pass` (also swallows KeyboardInterrupt
  and SystemExit — actively wrong). Upgraded to `except Exception as
  e: logger.debug("...: %s", e)`. ~14 sites across connection_detect,
  soulseek_client, listenbrainz_manager, watchlist_scanner,
  youtube_client, navidrome_client, jellyfin_client, web_server.
- `except Exception:` + pass that the awk pattern missed (e.g.
  multi-line or unusual whitespace). ~31 sites across automation_engine,
  database_update_worker, music_database, spotify_client, web_server,
  others.
- 14 legitimate cleanup sites left silent with explicit `# noqa: S110`
  + comment explaining why (atexit handlers, finally-block conn.close
  calls). Logging during shutdown can itself crash because file handles
  get torn down before the handler fires.

Also enables `S110` rule in pyproject.toml so this pattern fails CI
going forward — drift fails at PR review instead of at runtime against
a wedged worker thread. Tests path keeps S110 ignored (test fixtures
legitimately use try-except-pass for cleanup).

Adds a WHATS_NEW entry to helper.js summarizing the full #369 sweep.

Verified: `python -m ruff check .` → All checks passed.
Verified: `python -m pytest tests/` → 2188 passed.

Closes #369
2026-05-07 11:16:06 -07:00
Broque Thomas
aa54bed818 Surface silent exceptions across remaining modules — ~70 sites
Final sweep. Covers:
- Downloads: candidates / lifecycle / master / monitor / wishlist_failed
- Metadata: source / registry / cache / common / artwork (+ plex_client)
- Imports: pipeline / resolution / file_ops / paths / guards
- Library: path_resolver / retag / duplicate_cleaner
- Stats / playlists / wishlist / discovery / automation / enrichment
- Misc: hydrabase_client, soulsync_client, tag_writer, debug_info,
  api_call_tracker, album_consistency, beatport_unified_scraper,
  reorganize_runner, seasonal_discovery, lidarr_download_client,
  services/sync_service.py, automation_engine, automation/progress

Two `_e` renames in imports/file_ops.py (outer scope binding `e`).
A few finally-block sites in metadata/album_mbid_cache.py,
library/track_identity.py, listening_stats_worker.py, watchlist/
auto_scan.py left silent — same reason as the rest of the sweep
(logger calls during cleanup paths can themselves raise).

Refs #369
2026-05-07 10:28:58 -07:00
Broque Thomas
e95452b465 Surface silent exceptions in workers + repair jobs — ~30 sites
Across all background workers (Spotify/Tidal/Deezer/Qobuz/iTunes/
Discogs/Genius/AudioDB/MusicBrainz/Last.fm/SoulID + the metadata-update
worker) and the repair-job scanners. All converted to
`logger.debug("...: %s", e)`.

Two `_e` renames in genius_worker and soulid_worker where outer scope
was already binding `e`. Two finally-block sites in repair_jobs/
library_reorganize.py left silent (conn.close on shutdown path).

Refs #369
2026-05-07 10:27:24 -07:00
Broque Thomas
8219771304 Add module logger + surface silent exceptions in 7 logger-less files — 12 sites
These files had silent `except Exception: pass` blocks but no module
logger. Added `import logging` + `logger = logging.getLogger(__name__)`
at the top of each, then replaced the silent excepts with
`logger.debug(...)`.

- core/replaygain.py — 4 sites (id3 txxx + vorbis + mp4 atom reads)
- core/wishlist/presence.py — 3 sites (wishlist row parsing + queries)
- core/runtime_state.py — 1 site (activity toast emit)
- core/automation/signals.py — 1 site (collect known signals)
- core/download_engine/rate_limit.py — 1 site (plugin rate_limit_policy)
- api/system.py — 1 site (hydrabase status probe)
- api/search.py — 1 site (hydrabase search)

Refs #369
2026-05-07 10:27:04 -07:00
Broque Thomas
8dc9f79f97 Surface silent exceptions in watchlist + discovery + reorganize — 18 sites
- watchlist_scanner.py: 6 sites
- discovery/playlist.py: 5 sites
- discovery/sync.py: 4 sites
- watchlist/auto_scan.py: 1 site (1 left silent — finally-block scanner cleanup)
- library_reorganize.py: 2 sites (4 left silent — all in finally blocks:
  conn.close, staging rmtree, sidecar delete, cleanup_empty_dir)

All non-finally sites converted to `logger.debug("...: %s", e)`.
Finally-block sites kept silent because logger calls during cleanup
(after exception was already raised) can themselves raise.

Refs #369
2026-05-07 09:52:20 -07:00
Broque Thomas
de348981a5 Surface silent exceptions in import pipeline — 11 sites
- imports/side_effects.py: 8 sites (post-import cleanup paths,
  thumbnail+lyrics pulls, Plex refresh)
- auto_import_worker.py: 3 sites (queue/dedup helpers)

All converted to `logger.debug("...: %s", e)`.

Refs #369
2026-05-07 09:50:15 -07:00
Broque Thomas
aa9429d733 Surface silent exceptions in core/artists — 23 sites
- map.py: 15 sites (cache lookups + per-track DB inserts in artist→source
  mapping)
- liked_match.py: 8 sites (Spotify liked-songs match heuristics)

All converted to `logger.debug("...: %s", e)`. No control-flow changes.

Refs #369
2026-05-07 09:49:29 -07:00
Broque Thomas
cc7a3f76ac Surface silent exceptions in metadata clients — 37 sites
- spotify_client.py: 15 sites (mostly `publish_spotify_status` + cache
  parse fallbacks)
- deezer_client.py: 10 sites (cache get/store + dataclass parsing)
- itunes_client.py: 4 sites (cache parsing + lookup fallback)
- discogs_client.py: 3 sites (cache parsing + token-from-config)
- tidal_client.py: 2 sites (used `_e` to avoid shadowing `e` from a
  sibling `except` clause in the same function — defensive)
- deezer_download_client.py: 3 sites

All converted to `logger.debug("...: %s", e)` with lazy formatter.
No control-flow changes.

Refs #369
2026-05-07 09:33:38 -07:00
Broque Thomas
e4e6b6bd5a Surface silent exceptions in repair_worker — 16 sites
Mostly progress-callback try/except (caller-provided fns we don't
control) and best-effort file-move / DB-cleanup paths in the
auto-fixers. All converted to `logger.debug(...)`. No control-flow
changes.

Refs #369
2026-05-07 09:17:51 -07:00
Broque Thomas
bfef2c7579 Surface silent exceptions in music_database.py — 18 sites
Mostly schema-migration ALTER TABLE fallbacks (column-already-exists
is the silent expected case) plus a few cache-purge/notify-migration
spots. Same pattern as the web_server sweep: `except Exception as e:
logger.debug("...: %s", e)`.

Refs #369
2026-05-07 09:17:19 -07:00
Broque Thomas
b0c58a0f91 Surface silent exceptions in web_server.py — 81 sites
Replaces `except Exception: pass` blocks with `except Exception as e:
logger.debug(...)` so failures are inspectable in the log instead of
disappearing silently. Per JohnBaumb's request in #369.

- Pattern is consistent: `logger.debug("<context>: %s", e)` with lazy
  formatter and 2-6 word context describing the operation.
- 2 atexit handlers (lines 2977, 2983) intentionally left silent — the
  log file handles can be torn down before atexit fires, and a
  separate `_atexit_silence_shutdown_logger_errors` already exists for
  this exact reason.
- No behavior changes; control flow is unchanged. Test suite green
  (2188 passed).

Refs #369
2026-05-07 09:09:17 -07:00
BoulderBadgeDad
763d160691
Merge pull request #514 from Nezreka/fix/repair-job-card-pending-count
Repair job card badge — show pending count, not last-scan count
2026-05-07 08:30:45 -07:00
Broque Thomas
4c11375930 Repair job card badge — show pending count, not last-scan count
Discord report: Duplicate Detector card said "372 findings" and Cover
Art Filler said "60 findings", but clicking the Findings tab's Pending
filter showed 0. User read it as "findings aren't being created" —
looked like a detector bug.

Actual cause: the badge sourced ``last_run.findings_created``
(historical "found in last scan") without considering current state.
After the user (or bulk-fix automation) resolved or dismissed those
findings, they no longer appeared on the Pending tab — but the badge
kept showing the last-scan number in red urgent styling.

Backend was correct end-to-end: detectors create pending rows,
bulk-fix moves them to resolved, Findings tab filters by status.
Only the badge display lied about current state.

Fix:

- ``RepairWorker._get_pending_count_by_job()`` — single SQL aggregation
  returning ``{job_id: pending_count}`` for every job with pending
  findings. O(1) lookup per job instead of N round trips.
- ``get_all_job_info()`` calls it once per request and adds
  ``pending_findings_count`` to each job's API response.
- ``enrichment.js`` job card now branches on the count:
  - ``> 0`` → red ``"X pending"`` badge (urgent, action needed)
  - ``= 0`` AND last scan found something → muted grey ``"X found in
    last scan"`` (historical context, no action needed)
- New CSS class ``.repair-flow-badge.findings-historical`` for the
  muted slate color so the two states are visually distinct.

User-visible result with the screenshotted state (372 dup / 60 cover-
art findings, all resolved):
- Before: red "372 findings" / "60 findings" — implied 432 things to
  do, but Findings tab showed 0 pending
- After: grey "372 found in last scan" / "60 found in last scan" —
  the badge text tells the user the count is historical, no surprise
  when Pending is empty

Tests: 3 new tests in ``tests/test_create_finding_dedup_counter.py``
pin the per-job pending count helper:
- returns ``{job_id: count}`` based on status='pending' rows only;
  resolved + dismissed rows excluded
- empty dict when no pending findings exist
- gracefully returns ``{}`` on DB error (badge falls back to
  historical count via the existing JS ``or 0`` safety)

2188/2188 full suite green. Pure UI/state-display fix — no detector
logic, no backend behavior change.
2026-05-07 08:28:17 -07:00
BoulderBadgeDad
7bd15438da
Merge pull request #506 from JohnBaumb/dependencies-pin
Pin all dependencies to exact resolved versions
2026-05-06 23:46:32 -07:00
BoulderBadgeDad
7ceeee2715
Merge pull request #496 from dlynas/feat/fast-entrypoint-permissions
fix: skip recursive chown on startup when UIDs are already correct
2026-05-06 23:36:16 -07:00
BoulderBadgeDad
8e389fc187
Merge pull request #512 from Nezreka/fix/slskd-http-timeout-prevents-worker-deadlock
Bound slskd HTTP timeout — fixes worker thread deadlock
2026-05-06 22:11:03 -07:00
Broque Thomas
5c69b853b4 Bound slskd HTTP timeout — fixes worker thread deadlock
GitHub issue #499 (@bafoed). Big initial sync of Spotify playlists
worked for 2-3 hours then downloads silently stopped:

- 3 active tasks stuck in "Searching" state, replaced every ~10 min
  by different ones
- slskd UI showed no actual searches happening
- Debug log: orphaned-task count grew over time, no jobs executed
- Container restart was the only fix (bought another 2-3 hours)
- Not a rate limit (rates showed 0/min)

Root cause: ``core/soulseek_client.py`` constructed
``aiohttp.ClientSession()`` with no timeout at four sites. When slskd
hung on a request (overloaded, transient network blip, internal
stall), the HTTP call blocked indefinitely — and the worker thread
blocked with it. The download executor only has
``ThreadPoolExecutor(max_workers=3)``, so once 3 worker threads were
wedged on hung calls, no further downloads could start.

Batch-level "stuck detection" (10-minute timer in
``check_batch_completion_v2``) was correctly marking tasks
``not_found`` and trying to start replacements, but the executor pool
was exhausted — replacements queued forever inside the executor with
no thread to run them. Symptom: tasks rotating every ~10 min at the
batch level while the underlying executor stayed wedged.

Fix: bounded ``aiohttp.ClientTimeout`` (total 120s, connect 15s,
sock_read 60s) on every slskd ``ClientSession`` construction. Module-
level constant ``_SLSKD_DEFAULT_TIMEOUT`` so the four sites stay in
lockstep — future sites get the same protection by reusing the
constant.

Why these timeouts are safe:

- Every slskd API call is metadata-level (search submission, status
  polls, download enqueue, transfer state queries). None stream
  files — slskd handles file transfer via its own peer-to-peer
  infrastructure entirely outside our HTTP requests.
- Legitimate metadata calls finish in seconds. 120s ceiling is
  ~50× the normal latency.

Timeout handling:

- ``asyncio.TimeoutError`` caught explicitly BEFORE the generic
  ``except Exception`` — surfaces "slskd timed out" specifically in
  logs (debuggable instead of buried as "Error making API request").
- Returns None to the caller (same code path as a 5xx response or
  any other failure). No new error path; callers already handle
  None as "request failed".
- Worker thread unblocks immediately → executor pool stays healthy
  → downloads keep flowing.

Sites updated:

- ``_make_request`` (general /api/v0/ helper, line 152) — used for
  every slskd API operation
- ``_make_direct_request`` (non-/api/v0/ helper, line 235)
- ``_explore_api_endpoints`` Swagger fetch (line 1566) — diagnostic
- ``_explore_api_endpoints`` per-endpoint probe (line 1617) —
  diagnostic

Tests: 3 new tests in ``tests/downloads/test_soulseek_pinning.py``
pin:

- ``_SLSKD_DEFAULT_TIMEOUT`` is bounded (total set, ≤300s ceiling,
  connect ≤60s) — guards against future regressions that drop or
  unbound the timeout
- ``_make_request`` returns None on ``asyncio.TimeoutError`` rather
  than raising — pins the caller contract
- ``_make_direct_request`` returns None on ``asyncio.TimeoutError``

2185/2185 full suite green.

Closes #499.
2026-05-06 22:02:25 -07:00
BoulderBadgeDad
fdd6700161
Merge pull request #511 from Nezreka/fix/library-reorganize-job-via-per-album-planner
Rewrite Library Reorganize job to delegate to per-album planner
2026-05-06 21:38:39 -07:00
Broque Thomas
ca5c93162c Rewrite Library Reorganize job to delegate to per-album planner
GitHub issue #500 (@bafoed). Library Reorganize repair job moved
album tracks to single-template paths because of a fragile
classification heuristic. Concrete symptom: a track at
``Surf Curse/Surf Curse - Nothing Yet (2017)/01 - Christine F.flac``
got proposed for a move to
``Surf Curse/Surf Curse - Christine F/Surf Curse - Christine F.flac``
(single template) instead of staying under the album folder.

Root cause: the job had its own tag-reading + transfer-folder-walk +
template-application implementation. The classification was
``is_album = (group_size > 1)`` where ``group_size`` was the count
of same-album tracks currently sitting in the transfer folder being
scanned. Two failure modes:

- only one track of an album was in the transfer folder (rest already
  moved to the library, or not yet downloaded), or
- album tags varied slightly across tracks (e.g. ``"Buds"`` vs
  ``"Buds (Bonus)"``)

Either case gave a 1-element group → routed through the SINGLE
template → wrong destination.

Rewrite — delegate to the per-album planner the artist-detail
"Reorganize" modal already uses:

- ``core.library_reorganize.preview_album_reorganize`` for path
  computation (DB-driven, knows the album has N tracks regardless of
  how many sit in transfer; album-vs-single is structurally correct)
- ``core.reorganize_queue.enqueue_many`` for apply mode; the queue
  worker dispatches via ``reorganize_album`` which handles file move
  + post-processing + DB update + sidecar through the same code path
  the per-album modal uses

Job's per-album loop:

- iterate albums for the active media server only (matches the artist-
  detail modal's scope; multi-server users won't have the job touch
  the inactive server's files at paths they can't see)
- preview each album, catch exceptions per-album so one bad row
  doesn't abort the scan
- branch on planner status:
  - ``no_album`` / ``no_tracks`` (race: album deleted mid-scan) →
    skip silently
  - ``no_source_id`` (album never enriched) → emit ONE album-level
    "needs enrichment first" finding (vs N per-track findings cluttering
    the UI)
  - ``planned`` → filter mismatched tracks (matched + new_path +
    not unchanged + file_exists), emit per-track findings (dry-run)
    or collect album for bulk enqueue (apply)
- bulk enqueue at end of loop using the queue's correct return-shape
  (``{'enqueued': N, 'already_queued': M, 'total': K}``)

What's gone (~500 LOC):
- ``_read_tag_metadata`` / ``_get_audio_quality`` / transfer-folder walk
- ``_load_album_years`` / ``_lookup_years_from_api`` (planner does this)
- ``_apply_path_template`` / ``_build_path_from_template``
- direct ``shutil.move`` + sidecar move logic (queue handles)
- the fragile ``is_album = group_size > 1`` heuristic — structurally gone
- ``move_sidecars`` setting (no longer applicable; queue's post-process
  re-downloads cover art at the destination)

What stays:
- dry-run vs apply toggle
- ``file_organization.enabled`` gate
- stop / pause respect
- progress reporting
- findings for the UI

Cleaner separation of concerns:
- this job: DB-known tracks at wrong paths (active server only)
- ``orphan_file_detector``: files on disk with no DB entry
- ``dead_file_cleaner``: DB entries pointing to nonexistent files

Tests: 12 tests in ``tests/test_library_reorganize.py`` pin the
delegation contract — every status branch, every track-filter case,
exception handling, apply-mode enqueue payload, active-server scope,
estimate-scope shape. Three obsolete ``_lookup_years_*`` tests removed
(year handling moved to planner).

Closes #500 (the misclassification half — orphan + dead-file are
downstream sync-gap symptoms, separate concern).
2026-05-06 21:18:20 -07:00
BoulderBadgeDad
4f19b2ffb8
Merge pull request #510 from Nezreka/fix/honor-manual-match-in-enrich
Honor manually-matched source IDs in per-source enrichment workers
2026-05-06 20:00:32 -07:00
Broque Thomas
cceffbd8ec Honor manually-matched source IDs in per-source enrichment workers
GitHub issue #501 (@Tacobell444). After manually matching an album to
a specific source ID via the match-chip UI, clicking "Enrich" on that
album would fuzzy-search by name and overwrite the manual match with
whatever the search returned — or revert the match status to
``not_found`` if name search missed. Reorganize then read the now-
wrong ID and moved files to the wrong destination.

Root cause was in the per-source enrichment workers'
``_process_*_individual`` methods. Several workers (Spotify, iTunes)
ran search-by-name unconditionally with no check for an existing
stored ID. Others (Deezer, Tidal, Qobuz) skipped on existing-ID but
without refreshing metadata — preserved the ID but didn't actually
honor the user's intent of "use this match to pull fresh data".

Cin-shape lift: same fix needed in 5 workers, so extracted the shared
behavior into ``core/enrichment/manual_match_honoring.py``:

    honor_stored_match(
        db, entity_table, entity_id, id_column,
        client_fetch_fn, on_match_fn, log_prefix,
    ) -> bool

Per-worker variability (DB column name, client fetch method, response
shape) plugs in via callbacks. Workers call the helper at the top of
``_process_album_individual`` / ``_process_track_individual``; if it
returns True, the manual match was honored and the search-by-name
fallback is skipped. If False (no stored ID, fetch failed, or empty
response), the worker's existing search-by-name flow runs as before.

Workers wired:

- spotify_worker — album + track (was overwriting; now honors)
- itunes_worker — album + track (was overwriting; now honors)
- deezer_worker — album + track (was skip-on-id; now refreshes)
- tidal_worker — album + track (was skip-on-id; now refreshes)
- qobuz_worker — album + track (was skip-on-id; now refreshes)

Workers left alone (already correct):

- discogs_worker — already had inline stored-ID fast path that
  refreshes metadata. Same behavior, just inline; refactoring to use
  the shared helper would be churn for zero behavior change.
- audiodb_worker — same — inline fast path with full metadata refresh.
- musicbrainz_worker — preserves existing MBID and marks status,
  which is the correct behavior for MB (the MBID itself is the match
  payload — no separate metadata fetch).
- lastfm_worker / genius_worker — name-based services with no
  source-specific IDs to honor. Inherent re-search per call.

Reorganize fixed indirectly — it always honored stored IDs correctly
via ``library_reorganize._extract_source_ids``. The "Reorganize broken"
symptom was downstream of broken Enrich corrupting the stored ID.

Tests:

- ``tests/enrichment/test_manual_match_honoring.py`` — 11 tests
  pinning the shared helper contract: stored-ID fast path, no-ID
  fallthrough, empty-string treated as no ID, missing row, fetch
  exception caught and falls through, fetch returns None falls
  through, callback exceptions propagate, configurable table +
  column, defensive table-name whitelist.

- Per-worker wiring NOT tested individually — the workers depend
  on live DB / client objects that are heavy to mock. The shared
  helper's contract is pinned; per-worker call sites are short
  enough to verify by code review.

2173/2173 full suite green.

Closes #501.
2026-05-06 19:00:53 -07:00
BoulderBadgeDad
4dd0640799
Merge pull request #509 from Nezreka/fix/hifi-instances-table-missing
Fix "no such table: hifi_instances" via defensive lazy-create
2026-05-06 18:02:30 -07:00
Broque Thomas
fd5ccf4cb8 Fix "no such table: hifi_instances" via defensive lazy-create
GitHub issue #503 (@hadshaw21). Adding a HiFi instance via downloader
settings popped up ``no such table: hifi_instances`` even though
"Test Connection" and "Check All Instances" both worked.

Root cause: ``MusicDatabase._initialize_database`` runs every
``CREATE TABLE`` + every migration step inside one sqlite transaction.
Python's sqlite3 module doesn't autocommit DDL by default, so if any
later migration step throws on a user's specific DB shape (e.g. an
old volume from a prior SoulSync version with quirky schema state),
the WHOLE batch rolls back — including the ``hifi_instances`` CREATE
that ran earlier in the function. The user's next boot retries init,
hits the same migration failure, rolls back again. The ``hifi_instances``
table never lands no matter how many restarts.

Fix: defensive lazy-create. New ``_ensure_hifi_instances_table(cursor)``
helper runs ``CREATE TABLE IF NOT EXISTS`` on demand, called immediately
before every CRUD operation that touches ``hifi_instances``:

- ``get_hifi_instances`` / ``get_all_hifi_instances`` (read)
- ``add_hifi_instance`` / ``remove_hifi_instance`` (CRUD)
- ``toggle_hifi_instance`` / ``reorder_hifi_instances`` (CRUD)
- ``seed_hifi_instances`` (defaults seed)

Idempotent — costs one no-op CREATE check when the table is already
present, fully recovers from a broken init state. Read methods now
return empty instead of raising when init failed; write methods work
end-to-end.

Doesn't paper over the underlying init issue (still worth tracking
which migration step breaks for which user DB shapes — separate
concern) but makes HiFi instance management self-healing in the
meantime.

Tests:
- 7 obsolete tests that pinned ``raises sqlite3.OperationalError``
  removed — that contract is no longer correct
- 7 new tests pin the lazy-create behavior: every CRUD method works
  against a DB that's missing the ``hifi_instances`` table, verifying
  the table gets created and the operation completes

2162/2162 full suite green. Pure additive — no behavior change for
users with a healthy DB; affected users get back to working hifi
instance management.

Closes #503.
2026-05-06 17:49:44 -07:00
BoulderBadgeDad
b385034f79
Merge pull request #508 from Nezreka/feat/plex-all-libraries-mode
Feat/plex all libraries mode
2026-05-06 17:10:11 -07:00
Broque Thomas
9f2813fce4 Add cross-section dedup to all-libraries listing layer
Followup to the all-libraries-mode commit. Without dedup, a Plex Home
family where two users both have "Drake" in their music libraries
would see "Drake" twice in SoulSync's library list — Plex returns
distinct ratingKeys for each section's copy of the same artist.

Dedup design — applied selectively, NOT everywhere:

- ``_dedupe_artists(artists)``: groups by lowercased title, picks
  the canonical entry by ``leafCount`` (more tracks wins). Active
  ONLY in all-libraries mode; single-library mode is a no-op fast
  path with zero behavior change.
- ``_dedupe_albums(albums)``: same but keys on
  (lowercased parentTitle, lowercased title) so two artists with
  identically-titled albums (e.g. self-titled releases) stay
  separate.

Applied to:
- ``get_all_artists()`` — public listing for the library view
- ``get_library_stats()`` — count matches what user sees in the list

Deliberately NOT applied to:
- ``get_all_artist_ids()`` / ``get_all_album_ids()`` — these feed
  removal detection (compare returned ratingKey set against DB-linked
  ratingKeys to decide what's been removed). Deduping here would falsely
  flag non-canonical ratingKeys as "removed" and prune SoulSync's DB
  tracks that are linked to them. Pinned by two CRITICAL tests.
- ``_all_tracks()`` — track count stays raw because the same track
  in two sections IS two distinct files / Plex entries, not a logical
  duplicate.
- ``_search_general()`` and ``search_tracks`` Stage 1/2 — search
  results stay raw so cross-section matches aren't lost. Stage 1
  may miss cross-section tracks for the same artist but Stage 2's
  server-wide track search catches them.

Logging: when raw vs deduped artist counts differ, ``get_all_artists``
logs both so users can see "Found 4697 artists across all music
sections (4521 unique after cross-section dedup)" — surfaces the
overlap clearly.

Tests: 8 new tests in test_plex_all_libraries.py pin:
- canonical pick by leafCount (artists + albums)
- case-insensitive name match
- single-library no-op path (zero behavior change for those users)
- album dedup keys on (artist, title) so different-artist same-title
  albums stay separate
- ``get_all_artists`` listing applies dedup
- ``get_all_artist_ids`` does NOT dedup (CRITICAL — removal detection)
- ``get_all_album_ids`` does NOT dedup (CRITICAL — removal detection)
- ``get_library_stats`` uses deduped counts for artists/albums but
  raw count for tracks

Existing pre-stat test updated to use distinct mock instances —
``[MagicMock()] * 5`` creates five references to one mock which now
correctly collapses under dedup.

71/71 media_server tests green, 2162/2162 full suite green.

Honest known limitation acknowledged in WHATS_NEW + version modal:
write-back (genre / poster / metadata updates) targets one
ratingKey at a time — only updates the canonical section's copy of
an artist if it exists in multiple. Other section's copy stays
unchanged. Document and revisit if it matters.
2026-05-06 16:01:37 -07:00
JohnBaumb
0eab09cead unpin yt-dlp to always grab latest on rebuild 2026-05-06 15:47:51 -07:00
JohnBaumb
083f38ca77 pin all dependencies to exact resolved versions 2026-05-06 15:45:19 -07:00
Broque Thomas
620c41f1ac Add "All Libraries (combined)" mode to PlexClient
GitHub issue #505 (PopeBruhLXIX): users with multiple Plex music
libraries (e.g. one per Plex Home user, or two folder roots split
across separate library sections) only saw one library inside SoulSync
because the connection settings forced you to pick a single library
section. SoulSync's PlexClient stored exactly one ``self.music_library``
section reference and every read scanned only that one.

This change adds an opt-in "All Libraries (combined)" dropdown option
that flips the client into a server-wide read mode where every read
method (``get_all_artists`` / ``get_all_album_ids`` /
``search_tracks`` / ``get_library_stats`` / etc) dispatches through
``server.library.search(libtype=...)`` instead of querying a single
section. One Plex API call replaces N per-section iterations; Plex
handles the aggregation server-side.

Implementation:

- ``ALL_LIBRARIES_SENTINEL`` (``'__all_libraries__'``) — module-level
  constant used as the saved DB preference value when the user picks
  the synthetic "All Libraries" entry. Detection is one string compare
  in ``_find_music_library`` / ``set_music_library_by_name``. Existing
  preferences (real library names) are unaffected.

- ``self._all_libraries_mode`` (private flag) + ``is_all_libraries_mode()``
  (public accessor for external callers). When True, ``music_library``
  may stay None — ``is_fully_configured()`` recognizes the mode and
  still returns True so dispatch sites don't bail.

- New private helpers ``_can_query``, ``_get_music_sections``,
  ``_all_artists``, ``_all_albums``, ``_all_tracks``, ``_search_general``,
  ``_search_artists_by_name``. Single dispatch point for the
  section-vs-server branch — every read method funnels through them
  so future drift fails at one place.

- New public helpers for downstream callers:
  - ``get_recently_added_albums(maxresults, libtype)`` — used by
    DatabaseUpdateWorker's deep-scan recent-content sweep
  - ``get_recently_updated_albums(limit)`` — same
  - ``get_music_library_locations()`` — returns folder roots, used
    by web_server.py's file-path resolver

- ``trigger_library_scan`` and ``is_library_scanning`` fan out across
  every music section in all-libraries mode.

- ``get_available_music_libraries`` prepends a synthetic
  ``{'title': 'All Libraries (combined)', 'value': sentinel}`` entry
  ONLY when more than one music library exists. Single-library users
  don't get the extra option. ``value`` field is the canonical
  identifier the frontend submits to ``/api/plex/select-music-library``
  (real libraries: title; synthetic: sentinel string). Backward-
  compatible — entries without ``value`` fall back to ``title``.

Three crash points fixed in downstream consumers (would have failed
during a deep scan after the user picked all-libraries mode):

1. ``database_update_worker.py:411`` — bailed out with "No music
   library found in Plex" because ``not self.media_client.music_library``
   evaluated True in all-libraries mode (music_library is None there).
   Now uses ``is_fully_configured()`` which recognizes the mode.
   This was the root cause of the deep scan never starting.

2. ``database_update_worker.py:_get_recent_albums_plex`` — reached
   ``self.media_client.music_library.recentlyAdded()`` /
   ``.search()`` directly, AttributeError in all-libraries mode.
   Now routes through the new helper methods.

3. ``web_server.py:10947`` (file-path resolver) — accessed
   ``music_library.locations``; gated on ``music_library`` truthy so
   it didn't crash, but silently skipped all-libraries-mode locations.
   Now uses ``get_music_library_locations()`` which unions across
   sections.

Plus polish:

- ``/api/plex/clear-library`` also resets ``_all_libraries_mode``
  so a fresh "select library" flow doesn't inherit stale mode state.
- ``/api/plex/music-libraries`` surfaces "All Libraries (combined)"
  as ``current_library`` when in mode (settings UI displays correctly).
- Frontend ``loadPlexMusicLibraries`` uses ``library.value || library.title``
  so the sentinel-keyed option submits the sentinel string, not the
  human-readable label. Pre-select match handles both paths.

Honest tradeoffs (documented as known limitations):

- Same artist appearing in multiple Plex sections shows as separate
  entries in SoulSync (no dedup). Plex returns distinct ratingKeys
  for each. Cosmetic; revisit if it bites users.
- Write-back (genre / poster updates) targets one ratingKey at a time
  — only updates that section's copy. Other sections' copies stay
  unchanged.
- All-libraries mode includes any audiobook library that Plex
  classifies as ``type='artist'``. Edge case, opt-in only.

Tests: 21 new tests in tests/media_server/test_plex_all_libraries.py
pin both single-library mode (regression guard) and all-libraries mode
for every refactored method. Existing test_plex_pinning.py fixture
updated to initialize the new flag. 63/63 media_server tests green,
2148/2148 full suite green.
2026-05-06 15:39:19 -07:00