From 7e1c4c26ec7f9699e30dce181448a0c0c6c2f564 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Sat, 25 Apr 2026 09:07:44 -0700 Subject: [PATCH] Reorganize: fix moved-count + status/total UX issues from PR #377 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/library_reorganize.py | 35 ++++++-- tests/test_library_reorganize_orchestrator.py | 8 +- web_server.py | 10 +++ webui/static/library.js | 81 +++++++++++++++---- 4 files changed, 109 insertions(+), 25 deletions(-) diff --git a/core/library_reorganize.py b/core/library_reorganize.py index 537c9729..6d62da98 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -1025,25 +1025,29 @@ def _run_post_process_for_track(ctx: _RunContext, track_id, title, api_track, st return new_path -def _finalize_track(ctx: _RunContext, track_id, resolved_src, new_path) -> None: +def _finalize_track(ctx: _RunContext, track_id, resolved_src, new_path) -> bool: """Update the DB row, then remove the original (in that order — DB failure leaves the file at both locations, recoverable by library scan; the reverse would orphan the row). Records src/dst dirs for - end-of-run cleanup, deletes per-track sidecars.""" - db_updated = True + end-of-run cleanup, deletes per-track sidecars. + + Returns ``True`` if the track is fully landed (DB row points to + ``new_path`` AND the original is dealt with), ``False`` if DB + update failed. Caller MUST treat False as a failure for counting + purposes — the file is at both locations, the DB still points to + the old path, and counting it as "moved" overstates how many + tracks the user can actually find via the UI.""" if ctx.update_track_path_fn: try: ctx.update_track_path_fn(track_id, new_path) except Exception as db_err: - db_updated = False logger.warning( f"[Reorganize] DB path update failed for {track_id}: {db_err} " f"— leaving original at {resolved_src} so the library scan can recover." ) - if not db_updated: - return + return False if os.path.normpath(resolved_src) == os.path.normpath(new_path): - return # in-place edit; nothing more to do + return True # in-place edit; DB already correct, nothing to remove with ctx.state_lock: ctx.src_dirs_touched.add(os.path.dirname(resolved_src)) ctx.dst_dirs_touched.add(os.path.dirname(new_path)) @@ -1052,6 +1056,7 @@ def _finalize_track(ctx: _RunContext, track_id, resolved_src, new_path) -> None: except OSError as rm_err: logger.warning(f"[Reorganize] Couldn't remove original {resolved_src}: {rm_err}") _delete_track_sidecars(resolved_src) + return True def _process_one_track(ctx: _RunContext, plan_item: dict) -> None: @@ -1085,7 +1090,21 @@ def _process_one_track(ctx: _RunContext, plan_item: dict) -> None: if new_path is None: return - _finalize_track(ctx, track_id, resolved_src, new_path) + finalized = _finalize_track(ctx, track_id, resolved_src, new_path) + if not finalized: + # File landed at new_path but DB row + original-removal didn't. + # User can still find the track (library scan will re-index from + # new_path), but we can't honestly count it as "moved" — that + # would overstate how many tracks the UI knows are at their new + # locations. Surfacing as failed lets the user see something + # needs attention (per kettui's PR #377 review). + ctx.record_error( + track_id, title, + 'Track landed at new location but DB update failed — ' + 'file is at both old and new paths until library scan re-indexes.', + kind='failed', + ) + return with ctx.state_lock: ctx.summary['moved'] += 1 diff --git a/tests/test_library_reorganize_orchestrator.py b/tests/test_library_reorganize_orchestrator.py index 846e6ec9..62537b3c 100644 --- a/tests/test_library_reorganize_orchestrator.py +++ b/tests/test_library_reorganize_orchestrator.py @@ -867,7 +867,7 @@ def test_db_update_failure_leaves_original_in_place(monkeypatch, tmpdirs): def update_path_explodes(track_id, new_path): raise RuntimeError("simulated DB failure") - library_reorganize.reorganize_album( + summary = library_reorganize.reorganize_album( album_id='alb-1', db=db, staging_root=str(staging), resolve_file_path_fn=lambda p: p, post_process_fn=pp, update_track_path_fn=update_path_explodes, @@ -875,6 +875,12 @@ def test_db_update_failure_leaves_original_in_place(monkeypatch, tmpdirs): assert os.path.exists(src), "Original must still exist when DB update failed" assert os.path.exists(final_path), "New path file should also exist (post-process succeeded)" + # kettui PR #377 review: a DB-update failure must NOT increment + # `moved` — that would overstate how many tracks the UI knows are + # at their new locations. Track is reported as failed instead. + assert summary['moved'] == 0 + assert summary['failed'] == 1 + assert any('DB update failed' in e['error'] for e in summary['errors']) def test_successful_run_removes_original_and_updates_db(monkeypatch, tmpdirs): diff --git a/web_server.py b/web_server.py index 8ed9b258..97f77879 100644 --- a/web_server.py +++ b/web_server.py @@ -13691,6 +13691,11 @@ def reorganize_album_files(album_id): _reorganize_state.update({ 'total': 0, 'processed': 0, 'moved': 0, 'skipped': 0, 'failed': 0, 'current_track': '', 'errors': [], + # Set after the run from the orchestrator's summary so the + # frontend can distinguish 'completed' from 'no_source_id' + # / 'no_album' / 'no_tracks' / 'setup_failed' (otherwise + # zero-failure skips look green to the user). + 'result_status': None, 'result_source': None, }) download_dir = docker_resolve_path(config_manager.get('soulseek.download_path', './downloads')) @@ -13745,8 +13750,13 @@ def reorganize_album_files(album_id): f"(source={summary.get('source')}, moved={summary['moved']}, " f"skipped={summary['skipped']}, failed={summary['failed']})" ) + with _reorganize_lock: + _reorganize_state['result_status'] = summary.get('status') + _reorganize_state['result_source'] = summary.get('source') except Exception as run_err: logger.error(f"[Reorganize] Background error: {run_err}", exc_info=True) + with _reorganize_lock: + _reorganize_state['result_status'] = 'error' finally: with _reorganize_lock: _reorganize_state['status'] = 'done' diff --git a/webui/static/library.js b/webui/static/library.js index 816ebda1..cd6ee1bb 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -6310,7 +6310,10 @@ async function executeReorganize() { if (!result.success) throw new Error(result.error); closeReorganizeModal(); - showToast(`Reorganizing ${result.total} tracks...`, 'info'); + // /reorganize no longer returns `total` (track count is determined + // server-side after planning runs); use the message from the + // backend instead. kettui PR #377 review. + showToast(result.message || 'Reorganization started', 'info'); _pollReorganizeStatus(); } catch (error) { @@ -6322,6 +6325,42 @@ async function executeReorganize() { } } +// kettui PR #377 review: distinguish 'completed' from non-completed +// outcomes so zero-failure skips (no_source_id, no_album, no_tracks, +// setup_failed, error) don't get a green checkmark. +function _classifyReorganizeOutcome(state) { + const status = state.result_status; + if (status && status !== 'completed') return 'warning'; + if (state.failed && state.failed > 0) return 'warning'; + return 'success'; +} + +function _formatReorganizeResultMessage(state) { + const status = state.result_status; + if (status === 'no_source_id') { + return 'Reorganize skipped — album has no metadata source ID. Run enrichment first.'; + } + if (status === 'no_album') { + return 'Reorganize skipped — album not found in DB.'; + } + if (status === 'no_tracks') { + return 'Reorganize skipped — album has no tracks.'; + } + if (status === 'setup_failed') { + return 'Reorganize failed — couldn\'t create staging directory.'; + } + if (status === 'error') { + return 'Reorganize failed — see server logs for details.'; + } + let msg = `Reorganized: ${state.moved || 0} moved`; + if (state.skipped > 0) msg += `, ${state.skipped} skipped`; + if (state.failed > 0) msg += `, ${state.failed} failed`; + if (state.failed > 0 && state.errors && state.errors.length > 0) { + msg += ` (${state.errors[0].error})`; + } + return msg; +} + function _pollReorganizeStatus() { if (_reorganizePollTimer) clearTimeout(_reorganizePollTimer); @@ -6335,13 +6374,7 @@ function _pollReorganizeStatus() { showToast(`Reorganizing: ${state.processed}/${state.total} (${pct}%) — ${state.current_track}`, 'info'); _reorganizePollTimer = setTimeout(poll, 800); } else if (state.status === 'done') { - let msg = `Reorganized: ${state.moved} moved`; - if (state.skipped > 0) msg += `, ${state.skipped} skipped`; - if (state.failed > 0) msg += `, ${state.failed} failed`; - if (state.failed > 0 && state.errors && state.errors.length > 0) { - msg += ` (${state.errors[0].error})`; - } - showToast(msg, state.failed > 0 ? 'warning' : 'success'); + showToast(_formatReorganizeResultMessage(state), _classifyReorganizeOutcome(state)); _reorganizePollTimer = null; // Refresh the enhanced view to show updated paths @@ -6466,7 +6499,7 @@ async function _executeReorganizeAll() { // Source picker is captured ONCE before the loop — same source for every album const chosenSource = document.getElementById('reorganize-source-select')?.value || ''; - let succeeded = 0, failed = 0; + let succeeded = 0, skipped = 0, failed = 0; for (let i = 0; i < total; i++) { const album = albums[i]; @@ -6485,9 +6518,21 @@ async function _executeReorganizeAll() { continue; } - // Wait for this album to finish - await _waitForReorganizeComplete(); - succeeded++; + // kettui PR #377 review: don't count any HTTP-success as + // "succeeded" — check the final state's result_status. + // Albums with no source ID, no tracks, etc. complete the + // request but aren't actually reorganized. + const finalState = await _waitForReorganizeComplete(); + if (finalState && finalState.result_status === 'completed' && (finalState.failed || 0) === 0) { + succeeded++; + } else if (finalState && finalState.result_status && finalState.result_status !== 'completed') { + skipped++; + showToast(`Skipped: ${album.title} — ${_formatReorganizeResultMessage(finalState)}`, 'warning'); + } else { + // result_status === 'completed' but failed > 0 → partial + failed++; + showToast(`Partial: ${album.title} — ${_formatReorganizeResultMessage(finalState || {})}`, 'warning'); + } } catch (err) { showToast(`Error: ${album.title} — ${err.message}`, 'error'); failed++; @@ -6495,8 +6540,9 @@ async function _executeReorganizeAll() { } let msg = `Reorganized ${succeeded} of ${total} album${total !== 1 ? 's' : ''}`; - if (failed > 0) msg += ` (${failed} failed)`; - showToast(msg, failed > 0 ? 'warning' : 'success'); + if (skipped > 0) msg += `, ${skipped} skipped`; + if (failed > 0) msg += `, ${failed} failed`; + showToast(msg, (failed > 0 || skipped > 0) ? 'warning' : 'success'); _reorganizeAllRunning = false; if (applyBtn) { applyBtn.disabled = false; applyBtn.textContent = 'Reorganize All'; } @@ -6508,6 +6554,9 @@ async function _executeReorganizeAll() { } function _waitForReorganizeComplete() { + // Resolves with the final state object so the caller can inspect + // result_status / failed / errors instead of treating every + // completion as success (kettui PR #377 review). return new Promise(resolve => { const poll = setInterval(async () => { try { @@ -6515,11 +6564,11 @@ function _waitForReorganizeComplete() { const state = await resp.json(); if (state.status === 'done' || state.status === 'idle') { clearInterval(poll); - resolve(); + resolve(state); } } catch { clearInterval(poll); - resolve(); + resolve(null); } }, 800); });