- keep single-track import lookup in imports/resolution.py
- normalize simple-download search_result data before wishlist matching
- run wishlist cleanup for simple-download post-processing
- keep source-only artist detail on resolved names and MB short-circuit
- pass the selected manual match through singles import
- keep the import context source-aware so artist and album stay correct
- avoid treating non-Spotify IDs as wishlist Spotify IDs
- make wishlist logging and local variable names source-neutral
- Move the import pipeline runtime factory into core.imports.pipeline
- Move the metadata runtime factory into core.metadata.enrichment
- Keep the web server wiring thin and drop the shared glue module
- Add contract tests that keep the two runtime bundles separate
- Move the metadata and MusicBrainz-related tests into a dedicated tests/metadata subfolder.
- Keep the rest of the suite flat for now.
- Preserve the existing test filenames so the change stays organizational rather than behavioral.
- Relocate the shared metadata helper module from core/metadata_common.py into core/metadata/common.py.
- Update the new metadata package, the import pipeline, and the web entrypoint to use the package-scoped helper.
- Keep the shared config, mutagen, file-lock, and tag-writing helpers centralized without touching unrelated files.
- Pass the live runtime bundle into the shared metadata facade so worker-backed source enrichment can actually run.
- Forward runtime from the import pipeline and web-server wrapper into embed_source_ids.
- Add a regression test that verifies the runtime object reaches the source-ID embedding path.
- Keep existing metadata_cache and metadata_service at the top level for now
- Move the new branch-local metadata helpers under core/metadata
- Share MusicBrainz release cache state from core.metadata.source and update import sites
- Move app-wide task and activity registries out of core/imports
- Share one runtime-state module across the web server, API, and import pipeline
- Keep import-specific helpers focused on context and post-processing
- Move import flow modules into a dedicated package
- Update app and test imports to the new namespace
- Group the import-focused tests under tests/imports
- remove stale wrapper helpers from web_server and metadata_common
- import provider helpers directly in metadata_source
- keep the metadata modules' public surface explicit
- remove runtime from metadata helper APIs where it only carried config, logger, mutagen, and database access
- keep runtime only for the source-ID enrichment path that still needs live worker handles
- add the new metadata helper modules and update the tests to match the slimmer interfaces
- Move filename and staging helpers into their canonical modules
- Extract album naming and grouping from path handling
- Update import and test call sites to the new layout
- Extract the import pipeline, album import, staging, path, file ops, guards, runtime state, side effects, and metadata enrichment out of .
- Canonicalize the refactored import path around and remove legacy , , , and request shapes from the import endpoints.
- Make album and track metadata lookups follow the configured provider priority instead of hard-coding Spotify, while still falling back when needed.
- Update the import routes and frontend payloads to use the new core helpers.
- Add coverage for the extracted helpers and the refactored import flows.
PS. apologies to anyone who might check this commit out - the intention was to start small, but things kinda snowballed out of control at some point since the logic just kept going on and on, and everything kinda had to be changed all at once for it all to make any sense
Addresses #365 (reported by JohnBaumb), parts 3 & 5. Client-side
IDB / sessionStorage data cache (part 4) deferred to its own PR.
Cover art on Library and Discover used to re-fetch from the source
CDN on every page visit. Now a service worker caches images locally
in CacheStorage with cache-first strategy — second visit serves art
instantly with zero network round-trips. PWA manifest added so the
app is installable to home screen / desktop.
Service worker (`webui/static/sw.js`):
- Cache-first for images: 10 known CDN hosts (Spotify, Last.fm,
Apple, Deezer, Discogs, MusicBrainz CAA, YouTube thumbnails) plus
the local `/api/image-proxy` endpoint plus same-origin .png/.jpg/
.webp/.gif/.svg paths. Cross-origin file-extension matches are
refused so we don't accidentally cache trackers.
- Stale-while-revalidate for `/static/*`: serve cached instantly,
refresh in background. Combined with the existing `?v=static_v`
cache-bust, deploys still ship live (different query → different
cache entry, old ages out).
- HTML / API / everything else: no caching, pass through.
- Cache-versioned (CACHE_VERSION = 'v1'); activate handler wipes any
cache whose name doesn't match the current version.
- skipWaiting + clients.claim so deploys propagate to open tabs
without requiring a full close-and-reopen.
PWA manifest (`webui/static/manifest.json`):
- Standalone display mode, theme color #1db954 (matches --accent-rgb).
- Two icons (192, 512) with both `any` and `maskable` purpose,
generated from favicon.png with aspect-preserving transparent
padding so the existing logo lands inside the safe zone for
OS-applied masks.
Wiring:
- `web_server.py` adds a `/sw.js` route that serves the file from
root scope (a service worker only controls URLs at or below its
served path; `/static/sw.js` would scope to `/static/*` only).
`Cache-Control: no-cache` on the SW response so deploys propagate
on next page load instead of being pinned by the 1yr static cache
the rest of /static/ uses.
- `webui/index.html` adds the manifest link, theme-color meta, and
an apple-touch-icon for iOS.
- `webui/static/init.js` registers the SW on `window.load`.
Feature-detected — no-op on browsers without serviceWorker support
or on non-secure origins (SW requires https or localhost).
One bug caught + fixed during line-by-line self-review:
`_staleWhileRevalidate` could return null to `respondWith()` when
both the cache miss AND the network fetch failed (the `.catch(() =>
null)` collapsed the rejection to null, which then short-circuited
through the falsy chain). Now explicitly awaits the network promise
and falls back to `Response.error()` when it resolves to null —
matches the `_cacheFirst` pattern.
Browser-verified: sw.js registers, status "activated and is running"
in DevTools. 603 tests pass.
Self-review nit on b0e7dae. Discover data is user-specific (hero
artists from your watchlist, similar artists from your taste,
recently-played derivations, etc.) — `Cache-Control: public` would let
intermediate proxies (corporate caching proxy, Cloudflare with cache
rules, Nginx with proxy_cache) store one user's response and serve
it to another. Privacy leak.
Switched to `private, max-age=300`. Browser-only cache, proxies skip.
Static assets stay `public` (shared content — everyone gets the same
library.js). Streaming and backup endpoints already correct
(`no-cache` and `no-store` respectively).
603 tests pass.
Addresses #365 (reported by JohnBaumb), parts 1 & 2 of the proposal.
Service worker, client-side IDB/sessionStorage, and PWA manifest
deferred to follow-up PRs.
1. Static asset cache (CSS/JS/icons/fonts).
`SEND_FILE_MAX_AGE_DEFAULT` flipped from 0 to 31536000 (1 year) in
production. Safe because every static URL is bust-tagged with
`?v=static_v` (computed once per process start), so each server
restart effectively invalidates every cached asset for every user.
Within a single deploy, repeat page loads hit zero round-trips on
static files — was a 304 round-trip per asset before.
Dev override (`SOULSYNC_WEB_DEV_NO_CACHE=1`) keeps it at 0 so
iterating on JS/CSS doesn't need a server restart between edits.
Collateral fixes from the bump:
- Music streaming endpoint (L16140): `response.headers.add('Cache-Control',
'no-cache')` → bracket-assign. Under the old max-age=0, send_file
set `no-cache` and `.add()` duplicated harmlessly. Under the new
max-age=31536000, `.add()` would APPEND a second Cache-Control
value → two conflicting headers, browser-undefined behavior.
Bracket-assign replaces.
- Backup download endpoint (L25181): explicit `Cache-Control:
no-store` on the response so DB backups don't inherit the new
long max-age — sensitive content, must never cache.
2. Discover GET browser cache (5 min).
New `@app.after_request` hook scoped to `/api/discover/` and
`/api/discovery/` paths, GET method, 2xx responses only. Sets
`Cache-Control: public, max-age=300`. Skipped when the endpoint
already set its own Cache-Control. Toggling between Discover
sections within 5 min serves from browser cache, no backend hit.
Try/except wraps the hook body and logs a warning if anything
throws — never let a header-tagging bug turn a successful response
into a 500. (Logging instead of `pass` since silent except-pass is
exactly the anti-pattern issue #369 is about.)
Audited every other Cache-Control set site in web_server.py — only
the two `send_file` callers needed adjustment. Range-branch streaming
uses `Response()` directly, unaffected by the config change.
603 tests pass.
Closes#370 (reported by JohnBaumb).
The /api/settings endpoint and three siblings (/log-level,
/config-status, /verify) had no auth check — any logged-in profile
could read or modify service tokens, OAuth secrets, and API keys.
Cin's "minimum" suggestion from the issue: gate to admin profile.
Added an `admin_only` decorator near `get_current_profile_id` that
returns 403 when the current profile isn't admin (id=1). Applied
to all four endpoints.
Auth model note (documented in the decorator docstring): SoulSync's
existing model is "trust local network" — single-admin / no-multi-
profile installs default `get_current_profile_id()` to 1, so the
gate is a no-op for solo users. The decorator is meaningful in
multi-profile setups where non-admin sessions exist. Tightening to
real per-request auth is out of scope.
Did NOT consolidate with api/settings.py (Cin's "better" suggestion):
that endpoint uses API-key auth (for external tools), the web_server.py
copy uses session/profile auth (for the web UI). Different consumers,
different auth models — merging would break one or the other.
603 tests pass.
Six items from a Cin-style line-by-line pass on PR #383:
- resolve_cors_origins: list of non-string entries (`[None, 123]`) now
drops them instead of coercing to junk strings like `'None'`/`'123'`.
- will_reject: backwards-compat shim removed. Production callers always
pass `request.scheme` (Flask-guaranteed); the shim only existed for
tests/non-Flask callers and made the production code path branchier
than necessary. Tests now pass scheme explicitly.
- maybe_log: redundant `if not origin` early-return dropped. will_reject
handles missing origin (engineio's own behavior — server.py:207).
- RejectionLogger.__init__: `int(dedup_cap)` wrapped in try/except so
bad-type input falls back to DEFAULT_DEDUP_CAP instead of raising.
- web_server.py: docstring on the before_request hook explains why the
hook fires on every request (Flask doesn't scope before_request to a
path prefix; the early-return string compare is the cheapest option).
- settings.js: cors-origins URL regex tightened from `[^\s/]+` to
`[^\s/?#]+` so query/fragment chars don't pass validation. Engineio
would silently fail to match those anyway; better to flag at save.
Test changes:
- parametrize gained an explicit `scheme` column (12 cases updated).
- New explicit case: scheme-mismatch rejects (engineio compares full
`{scheme}://{host}` strings).
- `test_will_reject_falls_back_to_host_only_when_no_scheme_info`
deleted — the shim it tested is gone.
- `test_will_reject_honors_x_forwarded_host` now passes scheme info.
Net: -9 production lines, -3 test lines. Production code path is
straight-line. 603 tests pass.
Self-review nits on PR #384:
- requirements.txt: 5-line comment for one pin → 1 line. Rationale
lives in commit body and #367; no need to repeat in-tree.
- helper.js: dropped `page: 'settings'` from the yt-dlp WHATS_NEW
entry. Settings page has no yt-dlp UI; the link would have
navigated users somewhere irrelevant.
553 tests pass.
Closes#367 (reported by JohnBaumb).
The Docker entrypoint ran `pip install -U yt-dlp --quiet --no-cache-dir`
on every container start. Three problems with that:
- Non-deterministic startup: each restart could pick up a different
yt-dlp version, making "works on my machine" debugging harder.
- Network dependency at boot: PyPI being slow/unreachable gated the
app coming up.
- In-place upgrades inside running containers can race with active
yt-dlp invocations and aren't a great pattern.
Picked Option A from the issue: pin to an exact version in
requirements.txt (`yt-dlp==2026.3.17`) and remove the entrypoint
install entirely. yt-dlp comes baked into the image now via the
existing `pip install -r requirements.txt` in the Dockerfile.
Tradeoff: YouTube fixes ship via SoulSync releases now instead of
"next container restart". The pin is documented inline with how to
bump it.
Net change: -3 entrypoint lines, requirements.txt pin tightened,
WHATS_NEW '2.4.1' block opened (entries hidden until version bumps).
553 tests pass.
Self-review pass on the security fix uncovered five issues, all fixed
here:
1. will_reject scheme handling. Engineio compares full {scheme}://{host}
strings, not just hostnames. A TLS-terminating proxy can leave the
backend seeing http while the browser's Origin is https — engineio
rejects, but the original predictor said "allow" → no helpful log
line. Added request_scheme + forwarded_proto params, build full
candidate strings to match engineio.
2. EITHER-forwarded-header rule. Engineio adds the forwarded candidate
when EITHER X-Forwarded-Proto OR X-Forwarded-Host is present (it
falls back to HTTP_HOST for the missing one). The original predictor
only added it when forwarded_host was set — false negative for
misconfigs sending only X-Forwarded-Proto. Now mirrors engineio.
3. will_reject incorrectly rejected missing-Origin requests. Engineio
(server.py:207: `if origin: validate`) skips CORS validation when
no Origin header is sent — non-browser clients (curl etc.) are
intentionally permitted. The original code rejected them. Test was
asserting the wrong behavior. Both fixed.
4. RejectionLogger had unbounded dedup set growth. A hostile actor
opening connections from many distinct fake origins would fill
memory unboundedly. Capped at 100 unique origins (configurable);
when cap hit, one overflow notice is emitted and further rejections
are silently dropped until restart.
5. Lock pattern: the overflow log path called logger.warning() while
holding the dedup lock, inconsistent with the normal path. Fixed
to pick the message under the lock and log after release. Critical
section is now minimal and uniform.
Plus polish:
- Stale module docstring fixed (said "empty list" instead of "None").
- settings.js validates each cors_origins line against a URL regex on
save; toasts a one-shot warning if entries are malformed (resolver
silently filters them, but user gets feedback now).
- web_server.py wiring passes request.scheme + X-Forwarded-Proto so
the predictor has full proxy info.
Tests:
- 51 unit tests in tests/test_socketio_cors.py (was 45). New cases:
* scheme comparison (5 cases including TLS-terminating proxies)
* forwarded_proto-alone misconfig
* missing-origin matches engineio (was asserting wrong behavior)
* dedup cap with overflow + reset
* default cap is reasonable (uses public DEFAULT_DEDUP_CAP constant)
Engineio behavior independently verified by reading engineio/server.py
and engineio/base_server.py source. Predictor mirrors both files.
604 tests pass.
Closes#366 (reported by JohnBaumb).
Socket.IO was initialized with `cors_allowed_origins='*'`, accepting
WebSocket connections from any origin. A malicious site could open a
WS to a user's local SoulSync instance and exfiltrate live progress /
toast / activity events.
This commit:
- Defaults to engineio's same-origin behavior (`cors_allowed_origins=None`),
which automatically honors X-Forwarded-Host so reverse proxies that
send that header (Caddy / Traefik by default, properly-configured
Nginx) work transparently.
- Adds a `security.cors_origins` config setting + Settings → Security
textarea where users behind unusual proxies / Electron wrappers /
cross-origin integrations can whitelist their origin. Accepts comma
or newline separated values; `*` on its own line opts back into the
legacy wildcard with a startup-warning log.
- Logs a clear warning the first time engineio rejects each unique
origin, naming the rejected Origin and request Host and pointing
users to the settings field. Without this, engineio silently 403s
the upgrade and the user just sees a half-broken UI with no clue
why. Threadsafe dedup so a hostile origin can't spam logs.
Logic lives in `core/socketio_cors.py` (resolver, rejection
predictor, dedup logger class, startup-status emitter) — pure
functions, no Flask dependency. `web_server.py` adds 23 lines of
wiring and imports.
Important catch during review: my first pass used `cors_allowed_origins=[]`
as the "secure default." Reading engineio's source revealed `[]` actually
means "DISABLE CORS HANDLING" (engineio/server.py:202: `if cors_allowed_origins != []:`)
— identical security to `'*'`. Fixed to use `None` (engineio's actual
same-origin sentinel) and pinned with a regression test that asserts
the resolver never returns `[]` for any input shape.
Tests:
- tests/test_socketio_cors.py — 45 unit tests covering 19 resolver shape
cases (None, empty, whitespace, comma, newline, garbage types, lists),
the `[]`-must-never-be-returned security regression, 12 rejection
prediction cases, X-Forwarded-Host handling, dedup logger behavior,
threadsafe race (8 threads × 50 hammers → exactly 1 warning), and
startup-status emitter outputs.
Frontend:
- Settings → Security gains an "Allowed WebSocket Origins" textarea
with help text explaining same-origin default + when to add a domain
+ the `*` opt-out.
- helper.js — new '2.4.1' WHATS_NEW block (hidden until version bump)
with a chill-voice entry describing the change.
Conftest.py left at `'*'` — test environment, no security concern.
598 tests pass.
Trimmed the WHATS_NEW '2.4.0' block (27 entries) and the full
VERSION_MODAL_SECTIONS array (23 sections) from the diagnostic-paragraph
style I'd been defaulting to into something terse and casual:
- Descriptions are 1-2 short sentences instead of multi-clause writeups.
- Modal feature bullets capped at 3-7 short items each.
- Stripped parenthetical credits from titles (no more "(kettui Review)",
"(Images, Counts, Title Hints)" — those belong in git history, not UI).
- Lowercase casual tone throughout description bodies.
- No reporter handles in entry text.
Net: 176 insertions / 194 deletions. helper.js parses, 553 tests pass.
The version modal pulled its content from /api/version-info — a 295-line
hand-curated Python dict in web_server.py. The "What's New" panel pulled
its content from WHATS_NEW in helper.js. Same release notes, two files,
two languages, hand-edited at every release — drift was inevitable
(and happened: the kettui-fix entries I added recently differed in
detail between the two surfaces).
This commit makes helper.js the single editing surface:
- Adds VERSION_MODAL_SECTIONS const in helper.js right beside WHATS_NEW,
with a comment block documenting the relationship: WHATS_NEW is the
per-version detailed log used by the helper popover; VERSION_MODAL_SECTIONS
is the curated highlight reel shown by the sidebar version button. Both
edited at release time, both in the same file.
- Rewires showVersionInfo() in downloads.js to read from those consts
directly. No backend round-trip; the changelog content ships in the
same JS bundle the browser already loaded.
- Deletes the /api/version-info route and its 295-line version_data dict.
- Updates the line-39 comment to drop the now-stale "version-info endpoint"
reference.
Note: this is collocation, not true unification. WHATS_NEW and
VERSION_MODAL_SECTIONS are still two distinct structures with overlapping
content, linked by a comment convention rather than a shared schema. A
deeper refactor (e.g. a `featured` flag on WHATS_NEW entries that the
modal aggregates) was rejected as out-of-scope — the curated section
titles ("Earlier in v2.3", "Recent Fixes") aren't 1:1 mappable to
WHATS_NEW entries. Saving for a follow-up if the drift problem persists.
Risk audit:
- Load order: helper.js loads at line 7967, downloads.js at line 7873.
Both classic scripts execute synchronously before any clickable
interaction, so showVersionInfo (only invoked on the version-button
onclick) always sees both consts defined.
- populateVersionModal() unchanged — receives the same {title, subtitle,
sections: [{title, description, features, usage_note?}]} shape.
- Stale-cache window during deploy: old downloads.js hitting a 404 on
the deleted endpoint falls through to the existing catch + toast path
("Failed to load version information"). Cache-buster ?v=static_v
resolves on next page load.
553 tests pass. helper.js + downloads.js parse cleanly. No residual
references to /api/version-info anywhere in the repo.
_OLD_V22_NOTES (655 lines) and _OLD_V2_NOTES (556 lines) were
triple-quoted Python strings holding old release-notes JSON. No code
references them — `grep _OLD_V22_NOTES|_OLD_V2_NOTES` returns only
the definitions themselves. They were leftover from earlier
version-info refactors and have been sitting in the file unread.
Pure deletion. No behavior change.
Forgot to bump the hardcoded label in index.html during the 2.4.0
version commit. _getCurrentVersion() reads this textContent, so the
What's New surfacing logic was still seeing 2.3.
- _SOULSYNC_BASE_VERSION → 2.4.0 (was 2.39).
- Migrate WHATS_NEW key '2.40' → '2.4.0', strip unreleased flags off
the 27 entries shipping in this release, set release date.
- Replace parseFloat() version compare with proper int-tuple semver
comparator — parseFloat('2.4.0') and parseFloat('2.4.1') both return
2.4, which would have made future patch bumps invisible to the
What's New surfacing logic.
Five issues kettui flagged on PR #377:
- Worker race (reorganize_queue.py): _next_queued() picked an item and
released the lock, then re-acquired to flip status='running'. A
cancel() landing in that window marked the item cancelled but the
worker still ran it. Replaced with _claim_next_or_wait() that picks
AND flips under one lock acquisition.
- Wakeup race (reorganize_queue.py): _wakeup.clear() after the empty
check could lose an enqueue's _wakeup.set(), parking a freshly-queued
album for up to 60 seconds. Replaced Lock + Event with a single
threading.Condition; cond.wait() releases and re-acquires atomically
on notify.
- Bulk dedupe (reorganize_queue.py:enqueue_many): looped single-item
enqueue, so a duplicate album_id later in the same batch could slip
through if the worker finished the first copy before the loop
reached the second. Now holds the lock for the whole batch and tracks
a per-batch seen set, so intra-batch duplicates dedupe against each
other and not just pre-existing items.
- Preview button stuck disabled (library.js:loadReorganizePreview):
early returns and thrown errors skipped the re-enable line. Moved
state into a canApply flag committed in finally, so any exit path
lands the button correctly.
- DB helpers swallowing failures (music_database.py): get_album_display_meta
and get_artist_albums_for_reorganize used to catch every Exception
and return None / [], so a real DB outage masqueraded as "album not
found" / "no albums". Now lets exceptions bubble; the route layer
already wraps them as 500.
Tests:
- test_cancel_and_run_are_mutually_exclusive — hammers enqueue+cancel
pairs and asserts the invariant that no successfully-cancelled item
ever ran (catches regressions to the atomic pick).
- test_enqueue_many_dedupes_batch_internal_duplicates — pins the
intra-batch dedupe.
- test_get_album_display_meta_propagates_db_errors and
test_get_artist_albums_for_reorganize_propagates_db_errors — pin
the bubble-up behavior.
Changelog updated in helper.js and version modal.
Replaces the single-slot "one reorganize at a time, return 409 on collision"
model with a per-user FIFO queue. Buttons stay clickable, "Reorganize All"
is one backend call instead of an N-call JS loop, and a status panel mounted
at the top of the artist actions bar shows live progress (active item,
queued count, recent completions) with per-item cancel buttons.
Backend
- core/reorganize_queue.py: singleton queue + worker thread, dedupe-on-
enqueue, cancel rules (queued cancellable, running not), enqueue_many
for bulk operations, progress fan-out via update_active_progress
- core/reorganize_runner.py: factory builds the worker's runner closure
with injected dependencies. Reads config per-call so changing the
download path in Settings takes effect on the next reorganize without
a server restart
- database/music_database.py: get_album_display_meta and
get_artist_albums_for_reorganize — moves the SQL out of route handlers
- web_server.py: thin enqueue/snapshot/cancel/clear endpoints, runner
registration at module load. Old _reorganize_state globals + status
endpoint deleted. Static-asset cache buster (?v=<server-start>)
added so JS/CSS updates ship live without users clearing cache
Frontend
- webui/static/library.js: status panel mount, polling (1.5s when
active, 8s when idle), expand/collapse, per-item cancel, debounced
enhanced-view reload (one reload per artist batch instead of N).
Per-album reorganize button paints with queued/running indicator
and short-circuits to a toast when the album is already in queue
- webui/static/style.css: panel + button styling matching the existing
glass-UI accents
- webui/static/helper.js + version modal: WHATS_NEW entry
Tests (22 new)
- tests/test_reorganize_queue.py (19 tests): FIFO order, dedupe,
per-item source, cancel rules, continue-on-failure, snapshot
shape, progress propagation, bulk enqueue
- tests/test_reorganize_runner.py (4 tests): per-call config reads,
setup-failure summary, dependency injection, progress fan-out
- tests/test_reorganize_db_methods.py (7 tests): SQL JOIN behavior,
ordering, fallback for blank strings, artist isolation
Full suite 549 passed in 27s.
Four changes addressing kettui's PR #377 review comments:
1. **`_finalize_track` no longer over-counts on DB failure (🔴 bug).**
The function previously bailed on DB-update failure but
`_process_one_track` still incremented `summary['moved']`
unconditionally — overstating how many tracks the UI knows are
at their new locations. Fixed by:
- `_finalize_track` now returns ``bool`` (True only when DB row
was updated AND original was dealt with)
- Caller checks the return; on False, records as a failed track
with a clear message ("Track landed at new location but DB
update failed — file is at both old and new paths until library
scan re-indexes")
- Existing `test_db_update_failure_leaves_original_in_place` now
also asserts `moved == 0`, `failed == 1`, and that the error
message names the cause
2. **`executeReorganize` toast no longer says "undefined tracks" (🐛
bug).** `/reorganize` doesn't return `result.total` anymore (the
track count is determined server-side after planning), so the
"Reorganizing undefined tracks..." string was meaningless. Now uses
`result.message` from the backend instead.
3. **`_pollReorganizeStatus` distinguishes completed from skipped
(🟡 risk).** Backend now propagates the orchestrator's status
(`completed` / `no_source_id` / `no_album` / `no_tracks` /
`setup_failed` / `error`) into `_reorganize_state['result_status']`
so the frontend can warn appropriately. Two new helpers:
- `_classifyReorganizeOutcome(state)` — returns 'success' only
when `result_status === 'completed'` AND `failed === 0`;
'warning' otherwise
- `_formatReorganizeResultMessage(state)` — returns a message
specific to the outcome ("Reorganize skipped — album has no
metadata source ID. Run enrichment first." for `no_source_id`,
etc.)
Zero-failure non-completed runs now show as warnings instead of
green checkmarks.
4. **Bulk mode no longer counts skipped albums as succeeded (🟡
risk).** `_executeReorganizeAll`'s loop was treating any HTTP
200 response as success, ignoring the orchestrator's actual
outcome for that album. Fixed by:
- `_waitForReorganizeComplete()` now resolves with the final
state object (was: void)
- Loop checks `finalState.result_status === 'completed'` AND
`finalState.failed === 0` before counting `succeeded++`;
otherwise increments `skipped` (with a per-album warning
toast) or `failed` accordingly
- Final summary toast now reads
"Reorganized N of M albums, K skipped, J failed" and only
shows green when nothing was skipped or failed
All four addressed in a single commit because they form one
coherent UX-correctness fix — the bug bug (#1) and the count-
overstatement bug (#4) both made the user see "everything succeeded"
when reality was different. Together they make the UI honestly
reflect what actually happened.
Files:
- core/library_reorganize.py — `_finalize_track` returns bool,
`_process_one_track` reads it
- web_server.py — `_reorganize_state['result_status']` populated
from orchestrator's summary on success and on exception
- webui/static/library.js — `_classifyReorganizeOutcome` /
`_formatReorganizeResultMessage` helpers, single-album +
bulk-mode flows both consume them
- tests/test_library_reorganize_orchestrator.py — strengthened
the existing DB-failure test to assert moved/failed counts
Credit: kettui — four PR #377 review comments named all of these
precisely with line numbers and severity.
Reported by kettui on PR #374 review:
> api_track_count is not copied during the ratingKey migration, so
> the cache disappears when an album row is rekeyed. Add it to
> enrichment_cols or the next completeness scan will fall back to
> live API lookups again.
When Plex changes an album's ratingKey (after a library rescan), the
sync code rekeys the album row by inserting a new row at the new ID
and copying enrichment columns from the old row. The list of
columns to copy did not include `api_track_count`, so the cached
authoritative track count was lost on rekey — and the next completeness
scan would hit the fallback path that calls back out to the
metadata source's API. Defeats the cache.
Added `api_track_count` to the album-level `enrichment_cols` at
`music_database.py:4724`. The artist-level lists at lines 4238 and
4554 don't need updating — those are for artist rekeys and don't
carry album-scoped fields.
No new test — existing migration code has no test infrastructure
and writing a Plex-mocked one is larger than this fix. Cin will say
if he wants test coverage in his next review pass.
Credit: kettui — PR #374 review comment that flagged the missing
column in the rekey allowlist.
Reported by kettui on PR #374 review: the inline filter that backed
`set_album_api_track_count` only counted rows where `type_ == 'track'`,
but `discogs_client.get_album_tracks` itself accepts both `'track'`
AND empty `type_` as real songs (line 660: `type_ in ('track', '')`).
Releases where Discogs returns some real tracks with an empty `type_`
field would be undercounted, which would silently disagree with the
repair job's fallback `_get_expected_total` path (which calls into
`get_album_tracks_for_source` and therefore uses the client's count).
Extracted the filter into `count_discogs_real_tracks(tracklist)` —
single source of truth for the rule, testable in isolation, and the
worker call site is now a one-liner that names what it's doing. Also
defensive about the input shape: `type_ == None`, missing field, and
empty/None tracklist all handled cleanly.
10 tests pin the behavior:
- empty/missing/None type_ all count as a real track (the kettui case)
- 'heading', 'index', 'sub_track' excluded
- unknown future type strings excluded conservatively
- realistic multi-disc tracklist with mixed shapes counts correctly
- empty/None input returns 0 without raising
Credit: kettui — the PR #374 review comment that flagged this.
Reported on Discord by winecountrygames. The library "Reorganize" tool
had several layered bugs that all traced to the same root cause: the
endpoint reinvented every wheel post-processing already turns — its own
template engine, its own disc-number resolution from file tags, its own
sidecar sweep, its own collision detection — and each had drifted from
the canonical path used by fresh downloads. Reported symptoms:
- 3-disc Aerosmith deluxe collapsed to a flat single-disc layout
- Half the tracks on other albums silently skipped, no error / no count
- Re-runs left empty leftover album folders cluttering the artist dir
Architecture: stop reinventing wheels. Route reorganize through exactly
the same pipeline downloads use. Per-album:
1. Fetch the canonical tracklist from a metadata source (Spotify /
iTunes / Deezer / Discogs / Hydrabase) using the album's stored
source IDs. New `core/library_reorganize.py::plan_album_reorganize`
does this — primary-source-first, fall through priority chain
unless the user picked a specific source in the modal (strict mode).
2. For each local track, find the matching API entry via a scored
candidate matcher. Score components: exact-title (100),
substring-with-length-ratio (40-90), track-number agreement (20).
Hard reject when the two titles have different version
differentiators (Remix vs no-remix means different recordings,
not annotation drift). Below threshold = unmatched, surfaced as
"not in source's tracklist, left in place" rather than silently
mis-routing.
3. Copy the file to a per-album staging directory, build the same
context dict the import flow builds (`spotify_album` /
`track_info` / etc. with `is_album_download=True` so the path
builder enters ALBUM mode, not SINGLE mode), call
`_post_process_matched_download(...)` — same function fresh
downloads use. Post-process handles tagging, multi-disc subfolder
decisions, sidecar regeneration, AcoustID verification.
4. Read `context['_final_processed_path']` to learn where it landed.
Update `tracks.file_path` in the DB BEFORE removing the original
(DB-update failure leaves the file at both locations, recoverable
via library scan; the reverse would orphan the row). Delete
per-track sidecars (post-process recreates them at the new
destination).
3 concurrent workers per album via ThreadPoolExecutor, matching the
download path's per-batch worker count. State mutations all guarded by
a single lock; staging filenames carry a UUID prefix so concurrent
copies of identically-named source files don't overwrite each other.
Source picker in the modal lets the user choose which source to read
the tracklist from. Two endpoints feed it:
- `/api/library/album/<id>/reorganize/sources` — sources for THIS
album that are both authed AND have a stored ID. For the per-
album modal.
- `/api/library/reorganize/sources` — all authed sources globally.
For the bulk "Reorganize All" modal where per-album ID coverage
varies.
When the user picks a specific source, the orchestrator runs in
`strict_source=True` mode (no fallback chain) — picking Spotify means
"use Spotify or fail", not "use Spotify and silently fall back."
Preview endpoint shares the same planning logic as apply via
`preview_album_reorganize` — the destination path comes from the same
`_build_final_path_for_track` post-process uses, so what you see in
the preview is exactly what you get on apply.
Empty destination folders (from earlier failed runs OR from the
current run when post-process creates a dir then fails AcoustID)
get cleaned up after each successful run: walk up to the artist
folder from any successful destination, prune empty album-sibling
folders one level deep. Bounded scope = won't touch unrelated user
dirs.
Web_server.py shrinks by ~450 net lines. The endpoint handler is now
a thin wrapper that builds injected callables (path resolver, post-
process function, DB updater, empty-dir cleaner), spawns a thread
that calls `reorganize_album()`, and returns. All actual logic lives
in `core/library_reorganize.py` where it's unit-testable without
spinning up Flask.
Frontend cleanup: the per-call template input in both reorganize
modals (per-album and bulk) was redundant — the backend always uses
the configured global download template. Removed the input and the
variables-grid reference UI it was for.
39 new unit tests pin every contract:
- source resolution (no_source_id when album has none, fallthrough
chain when primary returns nothing, strict mode bypasses fallback)
- matcher scoring (exact / substring / multi-disc disambiguation /
smart-quote tolerance / dash-vs-parens / bonus-track substring /
Remix-vs-original differentiator rejection / "Real" doesn't false-
match "Real Real Real" / track-number-only no longer fires)
- file safety (DB-update failure leaves original in place, post-
process failure leaves original in place, post-process exception
caught and original preserved, success removes original AND
updates DB in the right order)
- sidecar handling (per-track .lrc/.nfo deleted on success, kept on
failure; album-level cover.jpg/folder.jpg cleaned only when
directory has no remaining audio)
- staging cleanup (recreated between tracks because post-process
nukes it, dir cleaned up on success AND on failure)
- destination-dir prune (empty siblings removed, real album with
files preserved, no recursive sweep)
- source picker (only authed-with-stored-ID sources for per-album,
all authed sources for bulk; strict mode doesn't fall back)
- concurrency (3 workers in flight, state stays consistent under
races, stop_check cuts off pending tasks)
- preview parity (preview produces same destination as apply for
multi-disc; ALBUM mode not SINGLE mode; unmatched/no-path tracks
surfaced with reasons)
Limitations (deliberate punts, NOT in this PR):
- Renamed local titles on multi-disc albums where track_number
also disagrees: matcher returns nothing (track is "not in
source"). Fixable by using duration_ms as a tertiary signal.
- Per-track in-modal source switching with per-album track-count
hints (would need a second API call before opening the modal).
- UI status panel on the artist page during a run — currently
just toasts. Documented as a follow-up PR.
Files:
- core/library_reorganize.py — new module: plan_album_reorganize,
preview_album_reorganize, reorganize_album, available_sources_for_album,
authed_sources, _score_candidate, helpers for staging/post-
processing/finalizing, sidecar + dest-dir cleanup
- core/metadata_service.py — no changes; reused get_album_for_source,
get_album_tracks_for_source, get_source_priority,
get_client_for_source
- web_server.py — three endpoints (preview / apply / sources GETs)
are thin wrappers; -450 net lines
- tests/test_library_reorganize_orchestrator.py — 39 tests covering
every contract above
- webui/static/library.js — source picker UI in both modals; dead
template input + variables-grid removed
- webui/static/style.css — dropdown option styling fix (white-on-
white was unreadable)
Reported on Discord by winecountrygames — his bug report named the
trigger button (Enhanced view → Reorganize All) and both symptoms
(multi-disc collapse, half-album skip), which let the diagnosis go
straight to the architectural problem.
