Commit graph

2 commits

Author SHA1 Message Date
Broque Thomas
7e1c4c26ec Reorganize: fix moved-count + status/total UX issues from PR #377 review
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.
2026-04-25 09:07:44 -07:00
Broque Thomas
2b15260b88 Reorganize: route library files through the post-processing pipeline
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.
2026-04-24 23:00:22 -07:00