danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Reorganize: fix moved-count + status/total UX issues from PR #377 review

Browse source 
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.
This commit is contained in:
Broque Thomas 2026-04-25 09:07:44 -07:00
parent 2b15260b88
commit 7e1c4c26ec
4 changed files with 109 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
core/library_reorganize.py
View file

@ -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

8
tests/test_library_reorganize_orchestrator.py
View file

@ -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):

10
web_server.py
View file

@ -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'

81
webui/static/library.js
View file

@ -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);
});