From 4c113759307432d64650a76ef11ba71028618abb Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Thu, 7 May 2026 08:28:17 -0700 Subject: [PATCH] =?UTF-8?q?Repair=20job=20card=20badge=20=E2=80=94=20show?= =?UTF-8?q?=20pending=20count,=20not=20last-scan=20count?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/repair_worker.py | 37 ++++++++- tests/test_create_finding_dedup_counter.py | 93 ++++++++++++++++++++++ webui/static/enrichment.js | 22 ++++- webui/static/helper.js | 1 + webui/static/style.css | 7 ++ 5 files changed, 155 insertions(+), 5 deletions(-) diff --git a/core/repair_worker.py b/core/repair_worker.py index 8f9d8d6e..2d431c05 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -257,8 +257,22 @@ class RepairWorker: self._config_manager.set(f'repair.jobs.{job_id}.settings', current) def get_all_job_info(self) -> List[dict]: - """Get info for all jobs (for API response).""" + """Get info for all jobs (for API response). + + Includes ``pending_findings_count`` per job so the job-card + badge can show CURRENT pending state instead of the + ``last_run.findings_created`` historical scan count. Without + this, a scan that creates 372 findings + a subsequent bulk- + fix that resolves all of them leaves the badge displaying + "372 findings" while the Findings tab Pending filter shows 0 + — confusing UX flagged on the Library Maintenance page. + """ self._ensure_jobs_loaded() + + # Single query → per-job pending count dict. O(1) lookup per + # job instead of N round trips. + pending_by_job = self._get_pending_count_by_job() + jobs_info = [] for job_id, job in self._jobs.items(): config = self.get_job_config(job_id) @@ -284,9 +298,30 @@ class RepairWorker: 'last_run': last_run, 'next_run': next_run, 'is_running': self._current_job_id == job_id, + 'pending_findings_count': pending_by_job.get(job_id, 0), }) return jobs_info + def _get_pending_count_by_job(self) -> dict: + """Return ``{job_id: pending_count}`` for every job that has + any pending findings. Single SQL aggregation.""" + conn = None + try: + conn = self.db._get_connection() + cursor = conn.cursor() + cursor.execute(""" + SELECT job_id, COUNT(*) FROM repair_findings + WHERE status = 'pending' + GROUP BY job_id + """) + return {row[0]: row[1] for row in cursor.fetchall()} + except Exception as e: + logger.debug("Error counting pending findings per job: %s", e) + return {} + finally: + if conn: + conn.close() + # ------------------------------------------------------------------ # Lifecycle # ------------------------------------------------------------------ diff --git a/tests/test_create_finding_dedup_counter.py b/tests/test_create_finding_dedup_counter.py index 7701a678..1327606e 100644 --- a/tests/test_create_finding_dedup_counter.py +++ b/tests/test_create_finding_dedup_counter.py @@ -206,3 +206,96 @@ def test_caller_pattern_counts_only_real_inserts(repair_worker_with_temp_db): assert result.findings_created == 1 assert result.findings_skipped_dedup == 4 + + +# --------------------------------------------------------------------------- +# _get_pending_count_by_job — feeds the job-card "X pending" badge +# --------------------------------------------------------------------------- + + +def test_get_pending_count_by_job_returns_per_job_dict(repair_worker_with_temp_db): + """Pin: helper returns ``{job_id: pending_count}`` based on rows + with status='pending' only. Job-card badge uses this so it shows + CURRENT pending state instead of the historical + ``last_run.findings_created`` (which inflates after a bulk-fix + moves all findings to status='resolved').""" + worker, path = repair_worker_with_temp_db + + conn = sqlite3.connect(path) + cur = conn.cursor() + # 3 pending duplicate_tracks for dup_detector + cur.executemany(""" + INSERT INTO repair_findings (job_id, finding_type, severity, status, + entity_type, entity_id, file_path, title, description) + VALUES (?, 'duplicate_tracks', 'info', 'pending', 'track', ?, ?, 'T', 'D') + """, [ + ('dup_detector', '1', '/a'), + ('dup_detector', '2', '/b'), + ('dup_detector', '3', '/c'), + ]) + # 2 pending missing_cover_art for cover_art_filler + cur.executemany(""" + INSERT INTO repair_findings (job_id, finding_type, severity, status, + entity_type, entity_id, file_path, title, description) + VALUES ('cover_art_filler', 'missing_cover_art', 'info', 'pending', + 'album', ?, ?, 'T', 'D') + """, [('10', '/x'), ('11', '/y')]) + # 1 RESOLVED finding — must NOT be counted + cur.execute(""" + INSERT INTO repair_findings (job_id, finding_type, severity, status, + entity_type, entity_id, file_path, title, description) + VALUES ('dup_detector', 'duplicate_tracks', 'info', 'resolved', + 'track', '99', '/z', 'T', 'D') + """) + # 1 DISMISSED finding — must NOT be counted + cur.execute(""" + INSERT INTO repair_findings (job_id, finding_type, severity, status, + entity_type, entity_id, file_path, title, description) + VALUES ('cover_art_filler', 'missing_cover_art', 'info', 'dismissed', + 'album', '88', '/w', 'T', 'D') + """) + conn.commit() + conn.close() + + result = worker._get_pending_count_by_job() + + assert result == { + 'dup_detector': 3, + 'cover_art_filler': 2, + } + + +def test_get_pending_count_by_job_returns_empty_when_no_pending(repair_worker_with_temp_db): + """Pin: jobs with zero pending findings are absent from the dict + (caller handles missing key as 0).""" + worker, path = repair_worker_with_temp_db + + conn = sqlite3.connect(path) + cur = conn.cursor() + cur.execute(""" + INSERT INTO repair_findings (job_id, finding_type, severity, status, + entity_type, entity_id, file_path, title, description) + VALUES ('dup_detector', 'duplicate_tracks', 'info', 'resolved', + 'track', '1', '/a', 'T', 'D') + """) + conn.commit() + conn.close() + + result = worker._get_pending_count_by_job() + + assert result == {} + + +def test_get_pending_count_by_job_handles_db_error_gracefully(repair_worker_with_temp_db): + """Pin: any DB exception → returns ``{}``. Job-card badge falls + back to historical count via the ``or 0`` safety in JS.""" + worker, _ = repair_worker_with_temp_db + + # Replace _get_connection with one that raises. + def _raise(): + raise sqlite3.OperationalError("simulated") + worker.db._get_connection = _raise + + result = worker._get_pending_count_by_job() + + assert result == {} diff --git a/webui/static/enrichment.js b/webui/static/enrichment.js index 3c916042..23c44935 100644 --- a/webui/static/enrichment.js +++ b/webui/static/enrichment.js @@ -1555,11 +1555,25 @@ async function loadRepairJobs() { flowParts.push('Auto-fix'); } } - // Show pending findings count - const findingsCount = job.last_run ? (job.last_run.findings_created || 0) : 0; - if (findingsCount > 0) { + // Badge: prefer the CURRENT pending count from the API + // (matches what the Findings tab actually shows); fall + // back to the historical ``findings_created`` from the + // last run when pending = 0 but the last scan did find + // something — that way users still see a hint that a scan + // happened recently. Without this, a scan that finds 372 + // duplicates and then has them all bulk-fixed shows + // "372 findings" on the badge while the Findings tab + // Pending filter is empty, which reads as a bug. + const pendingCount = job.pending_findings_count || 0; + const lastScanCount = job.last_run ? (job.last_run.findings_created || 0) : 0; + if (pendingCount > 0) { flowParts.push('→'); - flowParts.push(`${findingsCount} finding${findingsCount !== 1 ? 's' : ''}`); + flowParts.push(`${pendingCount} pending`); + } else if (lastScanCount > 0) { + // Show historical count with a clear "found in last scan" + // qualifier so users don't expect them on the Pending tab. + flowParts.push('→'); + flowParts.push(`${lastScanCount} found in last scan`); } // Build meta parts diff --git a/webui/static/helper.js b/webui/static/helper.js index 62075dd5..fdd4a3ed 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,7 @@ const WHATS_NEW = { '2.4.2': [ // --- post-2.4.1 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.2 dev cycle' }, + { title: 'Fix: Repair Job Card "X Findings" Badge Was Misleading After Bulk-Fix', desc: 'discord report: duplicate detector card said "372 findings" and cover art filler said "60 findings", but clicking the findings tab pending filter showed 0 — read like a bug ("findings aren\'t being created"). actual cause: job-card badge displayed `last_run.findings_created` (historical "found in last scan") which doesn\'t reflect current state when those findings have since been bulk-fixed and moved to status="resolved". fix: api response now includes `pending_findings_count` per job (current pending count from a single sql aggregation). badge now shows "X pending" when pending count > 0 (urgent red color), or "X found in last scan" with a muted grey color when pending = 0 but the last scan did surface something. user can tell at a glance whether something needs review vs whether it\'s a historical reminder. 3 new tests pin the per-job pending count helper.', page: 'stats' }, { title: 'Fix: Downloads Stop After 2-3 Hours (slskd HTTP Timeout)', desc: '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, but slskd ui showed no actual searches happening. only fix was restarting the soulsync container — which would buy another 2-3 hours. 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. download executor only has `max_workers=3` for download workers. once 3 worker threads were wedged on hung calls, no further downloads could start. batch-level "stuck detection" (10-min) was correctly marking tasks `not_found` and trying to start replacements, but the executor pool was exhausted — replacements queued forever. fix: bounded `aiohttp.ClientTimeout` (total 120s, connect 15s, sock_read 60s) on every slskd `ClientSession` construction. legitimate metadata calls (search submission, status polls, download enqueue) finish in seconds — slskd doesn\'t stream files through these requests, so the timeout can\'t kill a real operation. when timeout fires, the request raises `asyncio.TimeoutError` which is now explicitly caught + logged + returns None to the caller (treats as a normal failure, same code path as a 5xx response). worker thread unblocks → executor pool stays healthy → downloads keep flowing. 3 new tests pin the timeout config + the `asyncio.TimeoutError` handler so future drift fails at the test boundary instead of at runtime against a wedged executor.', page: 'downloads' }, { title: 'Fix: Library Reorganize Job Misclassified Album Tracks As Singles', desc: 'github issue #500 (bafoed): library reorganize repair job moved tracks like `Surf Curse/Surf Curse - Nothing Yet (2017)/01 - Christine F.flac` to single-template paths like `Surf Curse/Surf Curse - Christine F/Surf Curse - Christine F.flac`. root cause: the job used `is_album = (group_size > 1)` where `group_size` was the count of tracks for the same album currently sitting in the transfer folder being scanned — when only one track of an album was in transfer (rest already moved to library, or album tags varied across tracks like "Buds" vs "Buds (Bonus)"), each track became a 1-element group → all routed through single template. fix: rewrote the job to delegate to the per-album planner (`core.library_reorganize.preview_album_reorganize` / `reorganize_queue`) — the same planner the artist-detail "reorganize" modal uses. db-driven: the planner knows the album has multiple tracks regardless of how many sit in the transfer folder, so the album-vs-single classification is structurally correct. apply mode delegates to the existing reorganize queue → file move + post-processing + db update + sidecar handling all flow through one code path. only iterates albums for the ACTIVE media server (matches the artist-detail modal\'s scope) — multi-server users (plex + jellyfin etc) won\'t accidentally have the job touch the inactive server\'s files. albums missing a metadata source id get a single "needs enrichment first" finding instead of n per-track "no source" findings. dropped ~500 loc of tag-reading + transfer-walk + template logic that was duplicated against the per-album path. files in transfer with no db entry are now exclusively the orphan_file_detector\'s domain (clean separation). 12 tests pin the delegation contract.', page: 'library' }, { title: 'Fix: Enrich Honors Manual Album Matches', desc: 'github issue #501 (tacobell444): if you manually matched an album to a specific source ID via the match-chip UI, then clicked "enrich" on that album, the worker would search by name and overwrite your manual match with whatever the search returned (or revert status to "not_found" if it found nothing). reorganize then read the now-wrong id and moved files to the wrong destination. fix: extracted a shared `core/enrichment/manual_match_honoring.py` helper. every per-source enrichment worker (spotify / itunes / deezer / tidal / qobuz) now reads its stored id column at the top of `_process_*_individual` — if present, it fetches via `client.get_album(stored_id)` directly and refreshes metadata without touching the id. fuzzy name search only runs as fallback for never-matched entities. discogs / audiodb / musicbrainz already had inline stored-id fast paths and are left alone. lastfm / genius are name-based and don\'t store ids. cin-shape lift: same fix in 5 workers gets exactly one helper, per-worker variability (column name, client method, response shape) plugs in via callbacks. 11 new helper tests pin: stored-id fast-path, no-id fallthrough, fetch-failure fallthrough, table/column whitelist, callback contract.', page: 'library' }, diff --git a/webui/static/style.css b/webui/static/style.css index 25ef3398..b49ad298 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -50833,6 +50833,13 @@ tr.tag-diff-same { background: rgba(239, 68, 68, 0.10); color: #f87171; } +/* Historical "found in last scan" — present but already resolved / + dismissed. Muted so it doesn't read as urgent action like the + current-pending badge. */ +.repair-flow-badge.findings-historical { + background: rgba(148, 163, 184, 0.10); + color: #94a3b8; +} .repair-flow-arrow { color: rgba(255, 255, 255, 0.25); font-size: 12px;