From 360ea0749d8dcb4629328dccdc806e6f8ee97a39 Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 00:25:42 +0800 Subject: [PATCH 1/9] docs: add domain glossary and ADRs 0001-0003 Co-Authored-By: Claude Opus 4.8 (1M context) --- CONTEXT.md | 65 +++++++++++++++++++ ...0001-drive-streamrip-via-cli-subprocess.md | 22 +++++++ docs/adr/0002-in-memory-download-state.md | 21 ++++++ .../0003-completeness-from-embedded-tags.md | 28 ++++++++ 4 files changed, 136 insertions(+) create mode 100644 CONTEXT.md create mode 100644 docs/adr/0001-drive-streamrip-via-cli-subprocess.md create mode 100644 docs/adr/0002-in-memory-download-state.md create mode 100644 docs/adr/0003-completeness-from-embedded-tags.md diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..06684b8 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,65 @@ +# Context: Streamrip Web GUI + +A web front-end over the `streamrip` CLI (`rip`) for searching music sources and +downloading albums, tracks, and playlists. + +## Glossary + +### Download +A single user-requested unit of work, corresponding to one `rip url ` +invocation. A Download is *not* a single audio file — an album Download produces +many track files. A Download moves through a fixed lifecycle (see **Download +lifecycle**). + +### Download lifecycle +The states a Download passes through, in order: + +- **Queued** — accepted by the server and waiting for a free worker. A Queued + Download is visible to the user the instant it is submitted, even when no + worker is free to start it. +- **Downloading** — a worker is actively running `rip` for this Download. +- **Completed** — `rip` finished and fetched the tracks successfully. +- **Failed** — `rip` exited with an error. +- **Skipped** — `rip` did no work because every track was already recorded in + the streamrip database (see **Streamrip database**). Surfaced to the user as + "already downloaded". + +A Download is *active* while it is Queued or Downloading. Once it reaches a +terminal state (Completed / Failed / Skipped) it becomes part of the **History**. + +### Active +The set of Downloads that are currently Queued or Downloading. Presented to the +user as a single unified list, not split by state. + +### History +The record of Downloads that have reached a terminal state. + +### Streamrip database +The SQLite database maintained by `rip` itself, recording what it has already +downloaded so it can skip re-downloading. This is distinct from any state this +web app keeps. Bypassing it (so an already-recorded item downloads again) is a +**Redownload**. + +### Redownload +Re-running a Download for an item that streamrip would otherwise Skip, forcing +it to ignore the **Streamrip database**. Only meaningful for items whose source +URL is known — i.e. items in the **History** — never for arbitrary folders on +disk. + +### Library +The set of already-downloaded albums as they exist *on disk*, independent of +this app's Download History. The Library is the unit the user browses to verify +**Album completeness**. + +### Album completeness +Whether every track an album is supposed to contain is actually present on disk. +The expected track count is not stored by this app — it is read from the +embedded tags (`tracktotal`, `disctotal`, `tracknumber`, `discnumber`) of the +tracks that *are* present, which streamrip writes into every file. An album is: + +- **Complete** — every expected (disc, track) is present on disk. +- **Incomplete** — at least one expected track is absent. Each absent track is a + **Missing track**, identified by its number (its title is unknown, because a + track that was never downloaded left no tags on disk). +- **Unknown** — completeness cannot be determined, because no present track has + readable tags to reveal the expected total. diff --git a/docs/adr/0001-drive-streamrip-via-cli-subprocess.md b/docs/adr/0001-drive-streamrip-via-cli-subprocess.md new file mode 100644 index 0000000..6d75447 --- /dev/null +++ b/docs/adr/0001-drive-streamrip-via-cli-subprocess.md @@ -0,0 +1,22 @@ +# Drive streamrip via the `rip` CLI subprocess, not as a Python library + +We considered importing `streamrip` and driving downloads in-process to obtain +accurate per-track progress via its `get_progress_callback` hook. We chose +instead to keep shelling out to the `rip` CLI with `subprocess`. + +## Considered Options + +- **Import streamrip as a library.** Would give true per-track byte progress and + exact track totals, but couples us to streamrip's internal async API (which + changes between versions), removes process isolation (a streamrip crash would + take down a Flask worker), and forces asyncio into our threaded worker model. +- **Shell out to `rip` (chosen).** Stable public interface, full process + isolation, trivial to upgrade streamrip. + +## Consequences + +We cannot observe fine-grained download progress: `rip` renders per-track byte +bars through `rich.Live` and logs nothing on track success, so progress is not +parseable from stdout. This is why the UI shows a status badge + spinner rather +than a "4/12" track counter. Album completeness is therefore verified after the +fact from disk (see ADR-0003), not streamed live. diff --git a/docs/adr/0002-in-memory-download-state.md b/docs/adr/0002-in-memory-download-state.md new file mode 100644 index 0000000..e7cddf6 --- /dev/null +++ b/docs/adr/0002-in-memory-download-state.md @@ -0,0 +1,21 @@ +# Download state is held in memory; streamrip's DB is the durability backstop + +The server owns the authoritative Active, Queue, and History state in memory and +the frontend rehydrates it from `/api/status` on load. State survives a page +refresh but not a server/container restart. We deliberately did not add a +persistence layer (SQLite/JSON) at this stage. + +## Considered Options + +- **Persist History/queue to disk.** Survives restarts and gives a permanent, + queryable log, but adds a schema, migrations, and write-concurrency handling. +- **In-memory, server as source of truth (chosen).** Far less machinery; a page + refresh is seamless via `/api/status`. + +## Consequences + +History vanishes on restart, which is acceptable because streamrip's own SQLite +database (see ADR-0003 and the Streamrip database glossary entry) is the real +record of what has been downloaded — nothing is permanently lost. The code is +structured so a disk-persistence layer can be dropped in later without changing +the API surface. diff --git a/docs/adr/0003-completeness-from-embedded-tags.md b/docs/adr/0003-completeness-from-embedded-tags.md new file mode 100644 index 0000000..4578fd0 --- /dev/null +++ b/docs/adr/0003-completeness-from-embedded-tags.md @@ -0,0 +1,28 @@ +# Album completeness is derived from embedded tags, not a stored tracklist + +The Library/Files view verifies whether an album on disk is complete by reading +the embedded tags (`tracktotal`, `disctotal`, `tracknumber`, `discnumber`) of +the tracks that are present, and comparing the present track numbers against the +expected `1…tracktotal` per disc. streamrip writes these tags into every file +(`metadata/tagger.py`), and mutagen — already a streamrip dependency — reads +them back. + +## Considered Options + +- **Store the expected tracklist per download** (or re-fetch it from the source + on demand). Requires persisting metadata keyed to each folder, or a network + round-trip and the source URL/ID — which a bare folder on disk does not carry. +- **Infer gaps from track-number sequence alone.** Cannot detect trailing gaps + (e.g. tracks 10–12 missing) because the true total is unknown. +- **Read totals from present tags (chosen).** Any single present track reveals + the album's true `tracktotal`, so all gaps — including trailing ones — are + detectable from disk alone, with no network, no stored metadata, and no DB. + +## Consequences + +A missing track can only be shown by number ("Track 7 — missing"), never by +title, because a track that was never downloaded left no tags on disk. An album +with zero readable-tag tracks is reported as **Unknown** rather than guessed. +The Library view is read-only: it cannot offer Redownload, because a folder on +disk carries no source URL (Redownload lives in History — see the Redownload +glossary entry). From a0be682c55c6036127acc29f0953be533c64b777 Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 00:32:46 +0800 Subject: [PATCH 2/9] feat: queued Downloads visible instantly via lifecycle core (#2) Register every submitted Download in server-owned Active state as Queued and broadcast a download_queued event the instant the server accepts it, so a card appears even when all workers are busy. The worker transitions the card in place to Downloading, then finalizes it into server-owned History with its terminal state (Completed / Failed / Skipped), surviving a page refresh (ADR-0002). - Make the rip subprocess boundary injectable (run_rip seam) so tests never launch a real rip process; streamrip stays a CLI subprocess (ADR-0001). - Extract build_rip_command (pure) with --no-db support for later Redownload. - Skip detection keys on the "Marked as downloaded in the database" log line via classify_download, replacing the prior heuristic. - /api/status returns Active as a list + full History for frontend rehydration. - Frontend rehydrates from /api/status on load, handles the queued event, renders Queued cards (no spinner) that transition in place to Downloading. - Bootstrap pytest + Flask test client suite covering enqueue->queued via API and stubbed runs reaching completed/failed/skipped. Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitignore | 3 + app.py | 301 +++++++++++++++++++++++----------- pytest.ini | 3 + requirements-dev.txt | 1 + static/css/style.css | 4 + static/js/app.js | 86 ++++++++-- tests/conftest.py | 104 ++++++++++++ tests/test_classify.py | 31 ++++ tests/test_command_builder.py | 38 +++++ tests/test_lifecycle_api.py | 124 ++++++++++++++ 10 files changed, 584 insertions(+), 111 deletions(-) create mode 100644 pytest.ini create mode 100644 requirements-dev.txt create mode 100644 tests/conftest.py create mode 100644 tests/test_classify.py create mode 100644 tests/test_command_builder.py create mode 100644 tests/test_lifecycle_api.py diff --git a/.gitignore b/.gitignore index 66bca12..e297b1e 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,5 @@ .env venv/ +__pycache__/ +*.pyc +.pytest_cache/ diff --git a/app.py b/app.py index 05293be..22a4a76 100644 --- a/app.py +++ b/app.py @@ -37,118 +37,231 @@ logger.error("=" * 60) download_queue = queue.Queue() +#Server is the source of truth (ADR-0002): Active + History live here in memory. +#active_downloads maps task_id -> Download record (status: queued | downloading). active_downloads = {} +active_lock = threading.Lock() download_history = [] +history_lock = threading.Lock() sse_clients = [] album_art_cache = {} cache_lock = threading.Lock() - +MAX_HISTORY = 50 + +#The exact line streamrip logs once per track it refuses to re-download because the +#track is recorded in its own database. Skip detection keys on this line. +SKIP_LINE_RE = re.compile(r'Marked as downloaded in the database') + + +def build_rip_command(url, quality, *, config_path=None, download_dir=None, no_db=False): + """Construct the `rip url` argv. Pure (no side effects) so it can be tested + directly and reused for Redownload (no_db -> --no-db). This is the single + place the download invocation is assembled.""" + cmd = ['rip'] + if config_path and os.path.exists(config_path): + cmd.extend(['--config-path', config_path]) + if no_db: + cmd.append('--no-db') + if download_dir: + cmd.extend(['-f', download_dir]) + cmd.extend(['-q', str(quality)]) + cmd.extend(['url', url]) + return cmd + + +def classify_download(returncode, output): + """Map a finished `rip` run to a terminal Download state from its exit code + and stdout. Pure, so the worker's terminal-state logic is unit-testable.""" + if returncode != 0: + return 'failed' + #Skipped: rip did no downloading work because every track was already + #recorded in its database. Keyed on the skip log line; only a skip when + #nothing was actually downloaded. + if SKIP_LINE_RE.search(output) and '─ Downloading' not in output: + return 'skipped' + return 'completed' + + +def _default_runner(cmd): + """Run `rip` as a subprocess (ADR-0001), yielding stripped stdout lines and + finally returning the exit code. This is the seam tests replace with a fake + so no test ever launches a real `rip` process.""" + process = subprocess.Popen( + cmd, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + encoding='utf-8', + errors='replace', + bufsize=1, + ) + try: + for line in process.stdout: + line = line.strip() + if line: + yield line + process.wait() + finally: + if process.poll() is None: + process.terminate() + return process.returncode + + +#Injectable subprocess boundary. Tests swap this for a fake generator that emits +#canned output and a return code; production uses the real `rip` runner. +run_rip = _default_runner + + +def register_queued(task): + """Register a submitted Download in server Active state as `queued` and + announce it immediately, before any worker is free. Guarantees a submitted + Download is visible the instant the server accepts it.""" + record = { + 'id': task['id'], + 'url': task['url'], + 'quality': task.get('quality', 3), + 'metadata': task.get('metadata', {}), + 'status': 'queued', + 'queued_at': time.time(), + } + with active_lock: + active_downloads[task['id']] = record + broadcast_sse({ + 'type': 'download_queued', + 'id': record['id'], + 'metadata': record['metadata'], + 'status': 'queued', + }) + + +def enqueue_download(url, quality=3, metadata=None): + """Create a Download, register it as Queued (visible immediately), and hand + it to a worker. Returns the task id.""" + task_id = f"dl_{int(time.time() * 1000)}_{len(active_downloads)}" + task = { + 'id': task_id, + 'url': url, + 'quality': quality, + 'metadata': metadata or {}, + } + register_queued(task) + download_queue.put(task) + return task_id + + class DownloadWorker(threading.Thread): def __init__(self): super().__init__(daemon=True) self.current_process = None - + def run(self): while True: task = download_queue.get() if task is None: break - + task_id = task['id'] url = task['url'] quality = task.get('quality', 3) metadata = task.get('metadata', {}) - - active_downloads[task_id] = { - 'status': 'downloading', - 'url': url, - 'metadata': metadata, - 'started': time.time() - } - + no_db = task.get('no_db', False) + + #Transition the existing Queued card in place to Downloading. + with active_lock: + record = active_downloads.get(task_id) + if record is not None: + record['status'] = 'downloading' + record['started'] = time.time() + broadcast_sse({ 'type': 'download_started', 'id': task_id, 'metadata': metadata, 'status': 'downloading' }) - + output_lines = [] - process = None - + cmd = build_rip_command( + url, quality, + config_path=STREAMRIP_CONFIG, + download_dir=DOWNLOAD_DIR, + no_db=no_db, + ) + try: - cmd = ['rip'] - if os.path.exists(STREAMRIP_CONFIG): - cmd.extend(['--config-path', STREAMRIP_CONFIG]) - cmd.extend(['-f', DOWNLOAD_DIR]) - cmd.extend(['-q', str(quality)]) - cmd.extend(['url', url]) - - process = subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - text=True, - encoding='utf-8', - errors='replace', - bufsize=1, - ) - - self.current_process = process - - for line in process.stdout: - line = line.strip() - if line: - output_lines.append(line) - if len(output_lines) % 10 == 0: - broadcast_sse({ - 'type': 'download_progress', - 'id': task_id, - 'output': "\n".join(output_lines[-5:]), - 'progress': {'raw_output': True} - }) - - process.wait() + runner = run_rip(cmd) + returncode = 0 + try: + while True: + line = next(runner) + if line: + output_lines.append(line) + if len(output_lines) % 10 == 0: + broadcast_sse({ + 'type': 'download_progress', + 'id': task_id, + 'output': "\n".join(output_lines[-5:]), + 'progress': {'raw_output': True} + }) + except StopIteration as stop: + returncode = stop.value if stop.value is not None else 0 full_output = "\n".join(output_lines) + status = classify_download(returncode, full_output) - if process.returncode != 0: - status = 'failed' - logger.error(f"Download failed (exit code {process.returncode}): {' '.join(cmd)}") + if status == 'failed': + logger.error(f"Download failed (exit code {returncode}): {' '.join(cmd)}") logger.error("rip output (last 30 lines):\n%s", "\n".join(output_lines[-30:])) - elif re.search(r'Skipping track \d+', full_output) and '─ Downloading' not in full_output: - #rip skipped every track (already in its database) and only fetched cover art - status = 'skipped' + elif status == 'skipped': logger.info(f"Already downloaded (marked in streamrip database): {url}") - else: - status = 'completed' - - broadcast_sse({ - 'type': 'download_completed', - 'id': task_id, - 'status': status, - 'metadata': metadata, - 'output': full_output - }) - + + finalize_download(task_id, status, metadata, full_output) + except Exception as e: logger.exception(f"Download worker error for {url}") - broadcast_sse({ - 'type': 'download_error', - 'id': task_id, - 'error': str(e), - 'output': "\n".join(output_lines) if output_lines else str(e) - }) - + finalize_download( + task_id, 'failed', metadata, + "\n".join(output_lines) if output_lines else str(e), + error=str(e), + ) + finally: self.current_process = None - if task_id in active_downloads: - del active_downloads[task_id] - if process and process.poll() is None: - process.terminate() - + download_queue.task_done() + +def finalize_download(task_id, status, metadata, output, error=None): + """Move a Download out of Active into server-owned History with its terminal + state, and broadcast the transition. History is appended server-side here so + it survives a page refresh (ADR-0002).""" + with active_lock: + record = active_downloads.pop(task_id, None) + + entry = { + 'id': task_id, + 'url': record.get('url') if record else None, + 'metadata': metadata or (record.get('metadata') if record else {}), + 'status': status, + 'output': output, + 'completed_at': time.time(), + } + if error: + entry['error'] = error + + with history_lock: + download_history.insert(0, entry) + del download_history[MAX_HISTORY:] + + broadcast_sse({ + 'type': 'download_completed', + 'id': task_id, + 'status': status, + 'metadata': entry['metadata'], + 'output': output, + **({'error': error} if error else {}), + }) + def broadcast_sse(data): message = f"data: {json.dumps(data)}\n\n" dead_clients = [] @@ -220,25 +333,23 @@ def start_download(): return jsonify({'error': 'Unsupported service URL'}), 400 metadata = extract_metadata_from_url(url) - - task_id = f"dl_{int(time.time() * 1000)}" - task = { - 'id': task_id, - 'url': url, - 'quality': quality, - 'metadata': metadata - } - - download_queue.put(task) - + + task_id = enqueue_download(url, quality, metadata) + return jsonify({'task_id': task_id, 'status': 'queued'}) @app.route('/api/status') def get_all_status(): + #Server is the source of truth (ADR-0002). Active is returned as a list so the + #frontend can rehydrate the unified Active list (Queued + Downloading) on load. + with active_lock: + active = list(active_downloads.values()) + with history_lock: + history = list(download_history) return jsonify({ - 'active': active_downloads, - 'history': download_history[-20:], + 'active': active, + 'history': history, 'queue_size': download_queue.qsize() }) @@ -851,18 +962,10 @@ def download_from_url(): else: metadata = extract_metadata_from_url(url) - task_id = f"dl_{int(time.time() * 1000)}" - task = { - 'id': task_id, - 'url': url, - 'quality': quality, - 'metadata': metadata - } - - download_queue.put(task) - + task_id = enqueue_download(url, quality, metadata) + return jsonify({ - 'task_id': task_id, + 'task_id': task_id, 'status': 'queued', 'metadata': metadata }) diff --git a/pytest.ini b/pytest.ini new file mode 100644 index 0000000..0bc5130 --- /dev/null +++ b/pytest.ini @@ -0,0 +1,3 @@ +[pytest] +testpaths = tests +addopts = -ra diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..b3b2b3c --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1 @@ +pytest==9.0.3 diff --git a/static/css/style.css b/static/css/style.css index be84a5e..599570d 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -185,6 +185,10 @@ button:disabled { border-color: var(--text-tertiary); } +.download-item.queued { + border-left: 3px solid var(--text-tertiary); +} + .download-item.downloading { border-left: 3px solid var(--warning); } diff --git a/static/js/app.js b/static/js/app.js index 5fc2a5a..4e145b0 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -33,8 +33,11 @@ function initializeSSE() { } function handleSSEMessage(data) { - + switch(data.type) { + case 'download_queued': + handleDownloadQueued(data); + break; case 'download_started': handleDownloadStarted(data); break; @@ -47,24 +50,80 @@ function handleSSEMessage(data) { case 'download_error': handleDownloadError(data); break; - case 'heartbeat': - console.log('badump') + case 'connected': + case 'heartbeat': + break; default: console.log('Unknown SSE message type:', data.type); } } -function handleDownloadStarted(data) { +// Pull the authoritative Active list + History from the server (ADR-0002) so a +// page refresh loses nothing. Runs before live SSE deltas are processed. +async function rehydrateState() { + try { + const response = await fetch('/api/status'); + if (!response.ok) return; + const data = await response.json(); + + activeDownloads.clear(); + (data.active || []).forEach(item => { + activeDownloads.set(item.id, { + id: item.id, + metadata: item.metadata || {}, + status: item.status || 'queued', + output: '' + }); + }); + + downloadHistory = (data.history || []).map(item => ({ + id: item.id, + metadata: item.metadata || {}, + status: item.status, + output: item.output || '', + completedAt: item.completed_at ? item.completed_at * 1000 : Date.now() + })); + } catch (error) { + console.error('Failed to rehydrate state:', error); + } finally { + if (currentTab === 'active') { + renderActiveDownloads(); + } else if (currentTab === 'history') { + renderDownloadHistory(); + } + } +} + +function handleDownloadQueued(data) { + // A Queued card appears the instant the server accepts a Download, even + // when every worker is busy. activeDownloads.set(data.id, { id: data.id, - metadata: data.metadata, - status: 'downloading', - progress: 0, - output: [], - startTime: Date.now() + metadata: data.metadata || {}, + status: 'queued', + output: '', + queuedAt: Date.now() }); - + + if (currentTab === 'active') { + renderActiveDownloads(); + } +} + +function handleDownloadStarted(data) { + // Transition the existing Queued card in place to Downloading. + const download = activeDownloads.get(data.id) || { + id: data.id, + output: '' + }; + download.metadata = data.metadata || download.metadata || {}; + download.status = 'downloading'; + download.startTime = Date.now(); + activeDownloads.set(data.id, download); + if (currentTab === 'active') { + // Re-render so the card gains its spinner; updateDownloadElement alone + // cannot add the spinner that a Queued card lacks. renderActiveDownloads(); } } @@ -146,7 +205,7 @@ function handleDownloadCompleted(data) { if (download) { download.status = data.status; download.endTime = Date.now(); - download.output = data.output || download.allOutput.join('\n') || 'No output captured'; + download.output = data.output || (download.allOutput && download.allOutput.join('\n')) || 'No output captured'; updateDownloadElement(data.id, download); setTimeout(() => { @@ -217,7 +276,7 @@ function renderActiveDownloads() { ${item.output ? `SHOW OUTPUT` : ''} - + ${item.status === 'downloading' ? '' : ''} ${item.output ? `
@@ -655,6 +714,9 @@ async function downloadFromUrl(url) { window.addEventListener('load', () => { + // Rehydrate authoritative server state first (ADR-0002), then attach the + // live SSE delta channel on top of it. + rehydrateState(); initializeSSE(); }); diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..34d8f48 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,104 @@ +"""Shared pytest fixtures for the Streamrip Web GUI suite. + +Tests drive the app through its externally observable seams (HTTP API, the +injectable subprocess runner) and never shell out to the real `rip` binary. +""" +import threading +import time + +import pytest + +import app as app_module + + +@pytest.fixture +def client(): + app_module.app.config['TESTING'] = True + with app_module.app.test_client() as c: + yield c + + +@pytest.fixture(autouse=True) +def clean_state(): + """Reset server-owned Active/History/queue and restore the real runner + around every test so tests do not bleed into each other.""" + original_runner = app_module.run_rip + with app_module.active_lock: + app_module.active_downloads.clear() + with app_module.history_lock: + app_module.download_history.clear() + # Drain any leftover queued tasks. + try: + while True: + app_module.download_queue.get_nowait() + app_module.download_queue.task_done() + except Exception: + pass + yield + # Keep a harmless fast runner in place while we drain, so any worker that + # was mid-flight or grabs a leftover task never shells out to real `rip`. + app_module.run_rip = fake_runner(["teardown"], returncode=0) + try: + while True: + app_module.download_queue.get_nowait() + app_module.download_queue.task_done() + except Exception: + pass + # Let workers settle so they are idle before the next test reconfigures things. + time.sleep(0.05) + app_module.run_rip = original_runner + with app_module.active_lock: + app_module.active_downloads.clear() + with app_module.history_lock: + app_module.download_history.clear() + + +def fake_runner(lines, returncode=0): + """Build a fake subprocess runner: yields each canned line, then returns the + canned exit code via StopIteration.value (matching the real runner's shape).""" + def runner(cmd): + for line in lines: + yield line + return returncode + return runner + + +def wait_for(predicate, timeout=5.0, interval=0.01): + """Poll until predicate() is truthy or the timeout elapses.""" + deadline = time.time() + timeout + while time.time() < deadline: + if predicate(): + return True + time.sleep(interval) + return False + + +class BlockingRunner: + """A fake runner that, once a worker begins iterating it, blocks until + released. Lets a test saturate all workers so it can prove that further + submissions stay Queued rather than being silently dropped. + + Mirrors the real runner's generator shape: the body runs on first ``next``, + not on the call, so a worker is held inside the run only after it has + transitioned the Download to ``downloading``.""" + + def __init__(self, returncode=0): + self.started = threading.Semaphore(0) + self.release = threading.Event() + self.returncode = returncode + + def __call__(self, cmd): + def gen(): + self.started.release() + self.release.wait(timeout=10) + yield "blocked output" + return self.returncode + return gen() + + def wait_started(self, count, timeout=5.0): + deadline = time.time() + timeout + acquired = 0 + while acquired < count and time.time() < deadline: + if self.started.acquire(timeout=deadline - time.time()): + acquired += 1 + return acquired == count diff --git a/tests/test_classify.py b/tests/test_classify.py new file mode 100644 index 0000000..b704bdc --- /dev/null +++ b/tests/test_classify.py @@ -0,0 +1,31 @@ +"""Terminal-state classification of a finished rip run (pure function).""" +from app import classify_download + +SKIP_LINE = ( + "Skipping track 1: 'Song'. " + "Marked as downloaded in the database." +) + + +def test_nonzero_exit_is_failed(): + assert classify_download(1, "anything") == 'failed' + + +def test_completed_when_downloading_happened(): + output = "─ Downloading track 1\nDone" + assert classify_download(0, output) == 'completed' + + +def test_skipped_when_only_skip_lines_and_nothing_downloaded(): + output = f"{SKIP_LINE}\n{SKIP_LINE}\nFetching cover art" + assert classify_download(0, output) == 'skipped' + + +def test_not_skipped_when_some_tracks_downloaded(): + # Mixed run: some skipped, some downloaded -> completed, not skipped. + output = f"{SKIP_LINE}\n─ Downloading track 2" + assert classify_download(0, output) == 'completed' + + +def test_plain_success_with_no_skip_line_is_completed(): + assert classify_download(0, "Resolving metadata\nDone") == 'completed' diff --git a/tests/test_command_builder.py b/tests/test_command_builder.py new file mode 100644 index 0000000..0f1cb1d --- /dev/null +++ b/tests/test_command_builder.py @@ -0,0 +1,38 @@ +"""Seam 2: the rip command builder is a pure function.""" +import app as app_module +from app import build_rip_command + + +def test_command_includes_quality_url_and_download_dir(): + cmd = build_rip_command( + 'https://qobuz.com/album/123', 3, + config_path=None, download_dir='/music', + ) + assert cmd[0] == 'rip' + assert '-q' in cmd and cmd[cmd.index('-q') + 1] == '3' + assert '-f' in cmd and cmd[cmd.index('-f') + 1] == '/music' + assert cmd[-2:] == ['url', 'https://qobuz.com/album/123'] + + +def test_command_omits_config_path_when_missing(tmp_path): + missing = str(tmp_path / 'nope.toml') + cmd = build_rip_command('https://x/1', 2, config_path=missing, download_dir='/m') + assert '--config-path' not in cmd + + +def test_command_includes_config_path_when_present(tmp_path): + cfg = tmp_path / 'config.toml' + cfg.write_text('x = 1') + cmd = build_rip_command('https://x/1', 2, config_path=str(cfg), download_dir='/m') + assert '--config-path' in cmd + assert cmd[cmd.index('--config-path') + 1] == str(cfg) + + +def test_redownload_adds_no_db_flag(): + cmd = build_rip_command('https://x/1', 3, download_dir='/m', no_db=True) + assert '--no-db' in cmd + + +def test_normal_download_omits_no_db_flag(): + cmd = build_rip_command('https://x/1', 3, download_dir='/m') + assert '--no-db' not in cmd diff --git a/tests/test_lifecycle_api.py b/tests/test_lifecycle_api.py new file mode 100644 index 0000000..ab76cbe --- /dev/null +++ b/tests/test_lifecycle_api.py @@ -0,0 +1,124 @@ +"""Seam 1 + Seam 4: HTTP API lifecycle, driven by a stubbed subprocess runner. + +No test here launches a real `rip` process. +""" +import app as app_module +from conftest import fake_runner, wait_for, BlockingRunner + +QOBUZ_URL = 'https://qobuz.com/album/123' +SKIP_LINE = "Skipping track 1: 'x'. Marked as downloaded in the database." + + +def _active_ids(client): + data = client.get('/api/status').get_json() + return {item['id']: item for item in data['active']} + + +def _history(client): + return client.get('/api/status').get_json()['history'] + + +def test_submit_creates_queued_card_immediately(client): + # Hold every worker so nothing can start; the submission must still appear. + blocker = BlockingRunner() + app_module.run_rip = blocker + + # Saturate all workers first. + for _ in range(app_module.MAX_CONCURRENT_DOWNLOADS): + client.post('/api/download', json={'url': QOBUZ_URL, 'quality': 3}) + assert blocker.wait_started(app_module.MAX_CONCURRENT_DOWNLOADS) + + # Now submit one more while all workers are busy. + resp = client.post('/api/download', json={'url': QOBUZ_URL, 'quality': 3}) + assert resp.status_code == 200 + body = resp.get_json() + assert body['status'] == 'queued' + task_id = body['task_id'] + + # It is visible immediately as queued, even though no worker is free. + active = _active_ids(client) + assert task_id in active + assert active[task_id]['status'] == 'queued' + + blocker.release.set() + + +def test_no_submitted_download_is_invisible(client): + blocker = BlockingRunner() + app_module.run_rip = blocker + + # Saturate workers, then submit many more. + submitted = [] + for _ in range(app_module.MAX_CONCURRENT_DOWNLOADS): + r = client.post('/api/download', json={'url': QOBUZ_URL, 'quality': 3}) + submitted.append(r.get_json()['task_id']) + assert blocker.wait_started(app_module.MAX_CONCURRENT_DOWNLOADS) + + extra = 8 + for _ in range(extra): + r = client.post('/api/download', json={'url': QOBUZ_URL, 'quality': 3}) + submitted.append(r.get_json()['task_id']) + + active = _active_ids(client) + # Every submission is visible: the saturating ones downloading, the rest queued. + for task_id in submitted: + assert task_id in active + downloading = [a for a in active.values() if a['status'] == 'downloading'] + queued = [a for a in active.values() if a['status'] == 'queued'] + assert len(downloading) == app_module.MAX_CONCURRENT_DOWNLOADS + assert len(queued) == extra + + blocker.release.set() + + +def test_stubbed_run_reaches_completed(client): + app_module.run_rip = fake_runner(["─ Downloading track 1", "Done"], returncode=0) + + task_id = client.post('/api/download', json={'url': QOBUZ_URL}).get_json()['task_id'] + + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + entry = next(h for h in _history(client) if h['id'] == task_id) + assert entry['status'] == 'completed' + # Once terminal it has left the Active list. + assert task_id not in _active_ids(client) + + +def test_stubbed_run_reaches_failed(client): + app_module.run_rip = fake_runner(["boom"], returncode=1) + + task_id = client.post('/api/download', json={'url': QOBUZ_URL}).get_json()['task_id'] + + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + entry = next(h for h in _history(client) if h['id'] == task_id) + assert entry['status'] == 'failed' + + +def test_stubbed_run_reaches_skipped(client): + app_module.run_rip = fake_runner([SKIP_LINE, "Fetching cover art"], returncode=0) + + task_id = client.post('/api/download', json={'url': QOBUZ_URL}).get_json()['task_id'] + + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + entry = next(h for h in _history(client) if h['id'] == task_id) + assert entry['status'] == 'skipped' + + +def test_history_is_populated_server_side_for_rehydration(client): + app_module.run_rip = fake_runner(["─ Downloading track 1"], returncode=0) + task_id = client.post('/api/download', json={'url': QOBUZ_URL}).get_json()['task_id'] + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + + # Rehydration payload carries everything the frontend needs. + entry = next(h for h in _history(client) if h['id'] == task_id) + assert set(['id', 'status', 'metadata', 'completed_at']).issubset(entry.keys()) + + +def test_unsupported_url_is_rejected_and_not_queued(client): + resp = client.post('/api/download', json={'url': 'https://example.com/x'}) + assert resp.status_code == 400 + assert _active_ids(client) == {} + + +def test_missing_url_is_rejected(client): + resp = client.post('/api/download', json={}) + assert resp.status_code == 400 From 1ab41cbe955c3e4dec6f0e202af854e062d70cab Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 00:35:07 +0800 Subject: [PATCH 3/9] feat: retain URL and quality on History entries for rehydration (#5) History entries built in finalize_download now carry the Download's original URL and quality (read from the Active record) alongside its metadata and final status, so the server-owned History the frontend rehydrates from /api/status is complete enough for the Redownload slice to re-run an entry verbatim. The download_queued/started/completed SSE deltas carry url+quality too, and the frontend threads them through the Active map and History so live deltas and rehydration stay in sync. Co-Authored-By: Claude Opus 4.8 (1M context) --- app.py | 13 +++++++++- static/js/app.js | 12 +++++++++ tests/test_lifecycle_api.py | 52 +++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/app.py b/app.py index 22a4a76..779e3ac 100644 --- a/app.py +++ b/app.py @@ -129,6 +129,8 @@ def register_queued(task): broadcast_sse({ 'type': 'download_queued', 'id': record['id'], + 'url': record['url'], + 'quality': record['quality'], 'metadata': record['metadata'], 'status': 'queued', }) @@ -176,6 +178,8 @@ def run(self): broadcast_sse({ 'type': 'download_started', 'id': task_id, + 'url': url, + 'quality': quality, 'metadata': metadata, 'status': 'downloading' }) @@ -234,13 +238,18 @@ def run(self): def finalize_download(task_id, status, metadata, output, error=None): """Move a Download out of Active into server-owned History with its terminal state, and broadcast the transition. History is appended server-side here so - it survives a page refresh (ADR-0002).""" + it survives a page refresh (ADR-0002). + + The History entry retains the Download's original URL and quality (taken from + the Active record) alongside its metadata and final status, because the + Redownload slice re-runs a History entry from exactly those fields.""" with active_lock: record = active_downloads.pop(task_id, None) entry = { 'id': task_id, 'url': record.get('url') if record else None, + 'quality': record.get('quality') if record else None, 'metadata': metadata or (record.get('metadata') if record else {}), 'status': status, 'output': output, @@ -256,6 +265,8 @@ def finalize_download(task_id, status, metadata, output, error=None): broadcast_sse({ 'type': 'download_completed', 'id': task_id, + 'url': entry['url'], + 'quality': entry['quality'], 'status': status, 'metadata': entry['metadata'], 'output': output, diff --git a/static/js/app.js b/static/js/app.js index 4e145b0..4838dc6 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -70,14 +70,20 @@ async function rehydrateState() { (data.active || []).forEach(item => { activeDownloads.set(item.id, { id: item.id, + url: item.url, + quality: item.quality, metadata: item.metadata || {}, status: item.status || 'queued', output: '' }); }); + // History retains the original URL and quality (plus metadata) so the + // Redownload slice can re-run a History entry verbatim. downloadHistory = (data.history || []).map(item => ({ id: item.id, + url: item.url, + quality: item.quality, metadata: item.metadata || {}, status: item.status, output: item.output || '', @@ -99,6 +105,8 @@ function handleDownloadQueued(data) { // when every worker is busy. activeDownloads.set(data.id, { id: data.id, + url: data.url, + quality: data.quality, metadata: data.metadata || {}, status: 'queued', output: '', @@ -116,6 +124,8 @@ function handleDownloadStarted(data) { id: data.id, output: '' }; + download.url = data.url || download.url; + download.quality = data.quality != null ? data.quality : download.quality; download.metadata = data.metadata || download.metadata || {}; download.status = 'downloading'; download.startTime = Date.now(); @@ -204,6 +214,8 @@ function handleDownloadCompleted(data) { const download = activeDownloads.get(data.id); if (download) { download.status = data.status; + download.url = data.url || download.url; + download.quality = data.quality != null ? data.quality : download.quality; download.endTime = Date.now(); download.output = data.output || (download.allOutput && download.allOutput.join('\n')) || 'No output captured'; updateDownloadElement(data.id, download); diff --git a/tests/test_lifecycle_api.py b/tests/test_lifecycle_api.py index ab76cbe..606a01e 100644 --- a/tests/test_lifecycle_api.py +++ b/tests/test_lifecycle_api.py @@ -113,6 +113,58 @@ def test_history_is_populated_server_side_for_rehydration(client): assert set(['id', 'status', 'metadata', 'completed_at']).issubset(entry.keys()) +def test_terminal_history_retains_url_and_quality(client): + # The Redownload slice re-runs a History entry from its retained URL + + # quality, so a terminal Download must land in History with both. + app_module.run_rip = fake_runner(["─ Downloading track 1"], returncode=0) + task_id = client.post( + '/api/download', json={'url': QOBUZ_URL, 'quality': 4} + ).get_json()['task_id'] + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + + entry = next(h for h in _history(client) if h['id'] == task_id) + assert entry['url'] == QOBUZ_URL + assert entry['quality'] == 4 + assert entry['status'] == 'completed' + + +def test_status_payload_shape(client): + # The status endpoint is the single rehydration source: Active (a list), + # History (a list), and the queue size in one payload. + data = client.get('/api/status').get_json() + assert set(['active', 'history', 'queue_size']).issubset(data.keys()) + assert isinstance(data['active'], list) + assert isinstance(data['history'], list) + assert isinstance(data['queue_size'], int) + + +def test_queued_items_present_in_status_before_any_worker_starts(client): + # Saturate every worker so the extra submissions cannot start, then prove + # they are already authoritative in /api/status as queued Active entries. + blocker = BlockingRunner() + app_module.run_rip = blocker + + for _ in range(app_module.MAX_CONCURRENT_DOWNLOADS): + client.post('/api/download', json={'url': QOBUZ_URL, 'quality': 3}) + assert blocker.wait_started(app_module.MAX_CONCURRENT_DOWNLOADS) + + extra = 3 + queued_ids = [] + for _ in range(extra): + r = client.post('/api/download', json={'url': QOBUZ_URL, 'quality': 3}) + queued_ids.append(r.get_json()['task_id']) + + active = _active_ids(client) + for task_id in queued_ids: + assert task_id in active + assert active[task_id]['status'] == 'queued' + # Active entries carry the fields rehydration and Redownload rely on. + assert active[task_id]['url'] == QOBUZ_URL + assert active[task_id]['quality'] == 3 + + blocker.release.set() + + def test_unsupported_url_is_rejected_and_not_queued(client): resp = client.post('/api/download', json={'url': 'https://example.com/x'}) assert resp.status_code == 400 From 2509df05f30357dc20c2a516dc36436d5d577da4 Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 00:37:48 +0800 Subject: [PATCH 4/9] feat: redownload History entries bypassing streamrip database (#6) Add a Redownload action to History cards that re-enqueues the item as a brand-new Download whose `rip` invocation includes --no-db, forcing streamrip to ignore its own database (see the Redownload glossary entry). The action is emphasized on `skipped` ("already downloaded") cards and available quietly on every other History entry. The new /api/redownload endpoint re-enqueues by History id, reusing the original URL, quality, and metadata; enqueue_download now threads no_db through to the worker, which passes it to the existing pure build_rip_command. The new Download follows the normal lifecycle and appears as queued immediately with a fresh id distinct from the original. Co-Authored-By: Claude Opus 4.8 (1M context) --- app.py | 53 ++++++++++++-- static/css/style.css | 27 ++++++++ static/js/app.js | 52 ++++++++++++-- tests/test_redownload_api.py | 129 +++++++++++++++++++++++++++++++++++ 4 files changed, 251 insertions(+), 10 deletions(-) create mode 100644 tests/test_redownload_api.py diff --git a/app.py b/app.py index 779e3ac..e0c564e 100644 --- a/app.py +++ b/app.py @@ -136,15 +136,20 @@ def register_queued(task): }) -def enqueue_download(url, quality=3, metadata=None): +def enqueue_download(url, quality=3, metadata=None, no_db=False): """Create a Download, register it as Queued (visible immediately), and hand - it to a worker. Returns the task id.""" + it to a worker. Returns the task id. + + ``no_db`` forces a Redownload: the worker's `rip` invocation gets --no-db so + streamrip ignores its own database and downloads an already-recorded item + again (see the Redownload glossary entry).""" task_id = f"dl_{int(time.time() * 1000)}_{len(active_downloads)}" task = { 'id': task_id, 'url': url, 'quality': quality, 'metadata': metadata or {}, + 'no_db': no_db, } register_queued(task) download_queue.put(task) @@ -980,10 +985,48 @@ def download_from_url(): 'status': 'queued', 'metadata': metadata }) - - - + +@app.route('/api/redownload', methods=['POST']) +def redownload(): + """Redownload a History entry, bypassing the Streamrip database. + + Re-enqueues the item identified by its History ``id`` as a brand-new + Download, reusing the original URL, quality, and metadata, with --no-db so + streamrip ignores its own record and downloads it again even when it would + otherwise Skip (see the Redownload glossary entry). The new Download follows + the normal lifecycle: it is registered as Queued and visible immediately, + with a fresh id distinct from the original History entry.""" + data = request.json or {} + entry_id = data.get('id') + + if not entry_id: + return jsonify({'error': 'History entry id is required'}), 400 + + with history_lock: + entry = next((e for e in download_history if e['id'] == entry_id), None) + + if entry is None: + return jsonify({'error': 'History entry not found'}), 404 + + url = entry.get('url') + if not url: + #Redownload is only meaningful for items whose source URL is known. + return jsonify({'error': 'History entry has no source URL to redownload'}), 400 + + quality = entry.get('quality') + if quality is None: + quality = 3 + metadata = entry.get('metadata') or {} + + task_id = enqueue_download(url, quality, metadata, no_db=True) + + return jsonify({ + 'task_id': task_id, + 'status': 'queued', + 'metadata': metadata, + }) + if __name__ == '__main__': logger.info("Starting Streamrip Web application...") diff --git a/static/css/style.css b/static/css/style.css index 599570d..88f81fc 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -212,6 +212,33 @@ button:disabled { position: relative; } +/* Redownload action on History cards. Quiet by default (sits to the right of + the card), and emphasized on `skipped` ("already downloaded") items. */ +.redownload-btn { + margin-left: auto; + flex-shrink: 0; + padding: 8px 14px; + font-size: 10px; + border-color: var(--border); + color: var(--text-secondary); + white-space: nowrap; +} + +.redownload-btn:hover:not(:disabled) { + background: var(--accent); + color: var(--bg-primary); +} + +.redownload-btn.prominent { + border-color: var(--info); + color: var(--info); +} + +.redownload-btn.prominent:hover:not(:disabled) { + background: var(--info); + color: var(--bg-primary); +} + .download-spinner { margin-left: auto; margin-right: 15px; diff --git a/static/js/app.js b/static/js/app.js index 4838dc6..5551bf8 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -178,12 +178,23 @@ function renderDownloadHistory() { const isOk = item.status === 'completed' || item.status === 'skipped'; const statusIcon = isOk ? '✓' : '✗'; const statusClass = isOk ? 'success' : 'error'; - + + // Every History entry whose source URL is known offers a Redownload + // (re-run with --no-db). On `skipped` ("already downloaded") items it is + // emphasized as the natural next action; elsewhere it stays quiet. + const isSkipped = item.status === 'skipped'; + let redownloadAction = ''; + if (item.url) { + redownloadAction = isSkipped + ? `` + : ``; + } + return ` -
+
- ${item.metadata?.album_art ? - `` : + ${item.metadata?.album_art ? + `` : `
${statusIcon}
` }
@@ -191,15 +202,46 @@ function renderDownloadHistory() {
${item.metadata?.artist || 'Unknown Artist'}
${statusLabel(item.status)} - ${item.metadata?.service ? + ${item.metadata?.service ? `${item.metadata.service.toUpperCase()}` : ''}
+ ${redownloadAction}
`}).join(''); } +// Redownload a History entry: re-enqueue it bypassing streamrip's database +// (--no-db) so an already-downloaded item downloads again. The new Download +// enters the normal lifecycle and appears in the Active tab as queued. +async function redownload(historyId) { + const btn = document.querySelector(`[data-history-id="${historyId}"] .redownload-btn`); + if (btn) { + btn.disabled = true; + } + + try { + const response = await fetch('/api/redownload', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ id: historyId }) + }); + + const data = await response.json(); + + if (response.ok) { + switchTab('active'); + } else { + alert(data.error || 'Failed to redownload'); + if (btn) btn.disabled = false; + } + } catch (error) { + alert('Error: ' + error.message); + if (btn) btn.disabled = false; + } +} + function handleDownloadProgress(data) { const download = activeDownloads.get(data.id); diff --git a/tests/test_redownload_api.py b/tests/test_redownload_api.py new file mode 100644 index 0000000..c0bca55 --- /dev/null +++ b/tests/test_redownload_api.py @@ -0,0 +1,129 @@ +"""Seam 1 + Seam 4: Redownload re-enqueues a History entry bypassing streamrip's +database (--no-db), via the HTTP API and the stubbed subprocess runner. + +No test here launches a real `rip` process. +""" +import app as app_module +from conftest import fake_runner, wait_for, BlockingRunner + +QOBUZ_URL = 'https://qobuz.com/album/123' +SKIP_LINE = "Skipping track 1: 'x'. Marked as downloaded in the database." + + +def _active_ids(client): + data = client.get('/api/status').get_json() + return {item['id']: item for item in data['active']} + + +def _history(client): + return client.get('/api/status').get_json()['history'] + + +def _drive_to_history(client, url=QOBUZ_URL, quality=3, lines=("─ Downloading track 1",), returncode=0): + """Submit a normal Download and wait for it to land in History; return its id.""" + app_module.run_rip = fake_runner(list(lines), returncode=returncode) + task_id = client.post( + '/api/download', json={'url': url, 'quality': quality} + ).get_json()['task_id'] + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + return task_id + + +def test_redownload_creates_new_active_entry_distinct_from_original(client): + # Hold workers so the redownloaded item is observable as a queued Active + # entry that is distinct from the original History entry. + original_id = _drive_to_history(client, lines=[SKIP_LINE], returncode=0) + entry = next(h for h in _history(client) if h['id'] == original_id) + assert entry['status'] == 'skipped' + + blocker = BlockingRunner() + app_module.run_rip = blocker + + resp = client.post('/api/redownload', json={'id': original_id}) + assert resp.status_code == 200 + body = resp.get_json() + assert body['status'] == 'queued' + new_id = body['task_id'] + + # A brand-new Download with a fresh id distinct from the History entry. + assert new_id != original_id + active = _active_ids(client) + assert new_id in active + # The original History entry is untouched. + assert any(h['id'] == original_id for h in _history(client)) + + blocker.release.set() + + +def test_redownload_reuses_original_url_quality_and_metadata(client): + # Submit with a distinctive quality and let it finish, then redownload. + app_module.run_rip = fake_runner(["─ Downloading track 1"], returncode=0) + task_id = client.post( + '/api/download', json={'url': QOBUZ_URL, 'quality': 4} + ).get_json()['task_id'] + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + + blocker = BlockingRunner() + app_module.run_rip = blocker + + new_id = client.post('/api/redownload', json={'id': task_id}).get_json()['task_id'] + + active = _active_ids(client) + assert active[new_id]['url'] == QOBUZ_URL + assert active[new_id]['quality'] == 4 + + blocker.release.set() + + +def test_redownload_command_includes_no_db_flag(client): + # The original (normal) Download's command must NOT carry --no-db, while the + # redownloaded one MUST. Capture both commands through the runner seam. + captured = [] + + def capturing_runner(cmd): + captured.append(list(cmd)) + def gen(): + yield "─ Downloading track 1" + return 0 + return gen() + + app_module.run_rip = capturing_runner + + task_id = client.post( + '/api/download', json={'url': QOBUZ_URL, 'quality': 3} + ).get_json()['task_id'] + assert wait_for(lambda: any(h['id'] == task_id for h in _history(client))) + + new_id = client.post('/api/redownload', json={'id': task_id}).get_json()['task_id'] + assert wait_for(lambda: any(h['id'] == new_id for h in _history(client))) + + assert len(captured) == 2 + normal_cmd, redownload_cmd = captured[0], captured[1] + assert '--no-db' not in normal_cmd + assert '--no-db' in redownload_cmd + + +def test_redownloaded_item_follows_normal_lifecycle_to_history(client): + original_id = _drive_to_history(client, lines=[SKIP_LINE], returncode=0) + + app_module.run_rip = fake_runner(["─ Downloading track 1"], returncode=0) + new_id = client.post('/api/redownload', json={'id': original_id}).get_json()['task_id'] + + # It reaches a terminal state on its own and lands in History as a distinct + # entry from the original. + assert wait_for(lambda: any(h['id'] == new_id for h in _history(client))) + new_entry = next(h for h in _history(client) if h['id'] == new_id) + assert new_entry['status'] == 'completed' + assert new_id not in _active_ids(client) + + +def test_redownload_unknown_id_is_rejected(client): + resp = client.post('/api/redownload', json={'id': 'does_not_exist'}) + assert resp.status_code == 404 + assert _active_ids(client) == {} + + +def test_redownload_missing_id_is_rejected(client): + resp = client.post('/api/redownload', json={}) + assert resp.status_code == 400 + assert _active_ids(client) == {} From 1109b4f133cb8997c2696d5831fd826e5c790c5d Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 00:40:54 +0800 Subject: [PATCH 5/9] feat: route user feedback through toasts; drop auto-jump to Active on submit (#3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace every alert() call site with a non-blocking, stacking, auto-dismissing toast (success/error/info). Download submit and download-from-search now confirm acceptance with a 'Queued' toast and no longer yank the user to the Active tab — the toast confirms, the user chooses when to look. Config save, search/connection errors, and load failures also surface as toasts. Co-Authored-By: Claude Opus 4.8 (1M context) --- static/css/style.css | 58 ++++++++++++++++++++++++++++ static/js/app.js | 91 +++++++++++++++++++++++++++++++------------- templates/index.html | 4 +- 3 files changed, 126 insertions(+), 27 deletions(-) diff --git a/static/css/style.css b/static/css/style.css index 88f81fc..5672275 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -1029,3 +1029,61 @@ button:disabled { letter-spacing: 0.5px; } } + +/* Toast notifications — non-blocking, stacking, auto-dismissing feedback that + replaces every blocking alert() call site. */ +.toast-container { + position: fixed; + bottom: 24px; + right: 24px; + z-index: 1000; + display: flex; + flex-direction: column; + gap: 10px; + max-width: min(360px, calc(100vw - 48px)); + pointer-events: none; +} + +.toast { + background: var(--bg-tertiary); + border: 1px solid var(--border); + border-left: 3px solid var(--text-tertiary); + color: var(--text-primary); + padding: 12px 16px; + font-size: 12px; + letter-spacing: 0.5px; + line-height: 1.4; + box-shadow: 0 4px 16px rgba(0, 0, 0, 0.5); + pointer-events: auto; + cursor: pointer; + word-break: break-word; + opacity: 0; + transform: translateX(20px); + transition: opacity 0.2s ease, transform 0.2s ease; +} + +.toast.visible { + opacity: 1; + transform: translateX(0); +} + +.toast.success { + border-left-color: var(--success); +} + +.toast.error { + border-left-color: var(--error); +} + +.toast.info { + border-left-color: var(--info); +} + +@media (max-width: 600px) { + .toast-container { + left: 16px; + right: 16px; + bottom: 16px; + max-width: none; + } +} diff --git a/static/js/app.js b/static/js/app.js index 5551bf8..d886acd 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -13,6 +13,39 @@ let activeDownloads = new Map(); let downloadHistory = []; +// Non-blocking, stacking, auto-dismissing toast. Replaces every alert() so user +// feedback (Download accepted/failed, config saved, search/connection errors) +// never blocks the page or yanks focus. `type` is 'success' | 'error' | 'info'. +function showToast(message, type = 'info', duration = 4000) { + const container = document.getElementById('toastContainer'); + if (!container) return; + + const toast = document.createElement('div'); + toast.className = `toast ${type}`; + toast.textContent = message; + toast.setAttribute('role', type === 'error' ? 'alert' : 'status'); + + const dismiss = () => { + if (toast.dataset.dismissed) return; + toast.dataset.dismissed = '1'; + toast.classList.remove('visible'); + // Wait for the fade-out transition before removing from the DOM. + setTimeout(() => toast.remove(), 200); + }; + + toast.addEventListener('click', dismiss); + container.appendChild(toast); + // Force a reflow so the entrance transition runs from the hidden state. + requestAnimationFrame(() => toast.classList.add('visible')); + + if (duration > 0) { + setTimeout(dismiss, duration); + } + + return toast; +} + + function initializeSSE() { if (eventSource) { eventSource.close(); @@ -231,13 +264,13 @@ async function redownload(historyId) { const data = await response.json(); if (response.ok) { - switchTab('active'); + showToast('Redownload queued', 'success'); } else { - alert(data.error || 'Failed to redownload'); + showToast(data.error || 'Failed to redownload', 'error'); if (btn) btn.disabled = false; } } catch (error) { - alert('Error: ' + error.message); + showToast('Error: ' + error.message, 'error'); if (btn) btn.disabled = false; } } @@ -381,30 +414,32 @@ async function startDownload() { const quality = document.getElementById('qualitySelect').value; if (!url) { - alert('Please enter a URL'); + showToast('Please enter a URL', 'error'); return; } - + const btn = document.getElementById('downloadBtn'); btn.disabled = true; - + try { const response = await fetch('/api/download', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ url, quality: parseInt(quality) }) }); - + const data = await response.json(); - + if (response.ok) { document.getElementById('urlInput').value = ''; - document.querySelector('.tab').click(); + // The toast confirms acceptance; the user chooses when to look at the + // Active tab, so we no longer yank them there on submit. + showToast('Queued', 'success'); } else { - alert(data.error || 'Failed to start download'); + showToast(data.error || 'Failed to start download', 'error'); } } catch (error) { - alert('Error: ' + error.message); + showToast('Error: ' + error.message, 'error'); } finally { btn.disabled = false; } @@ -417,7 +452,7 @@ async function loadConfig() { const data = await response.json(); document.getElementById('configEditor').value = data.config || ''; } catch (error) { - alert('Failed to load config: ' + error.message); + showToast('Failed to load config: ' + error.message, 'error'); } } @@ -432,13 +467,13 @@ async function saveConfig() { }); if (response.ok) { - alert('Config saved successfully'); + showToast('Config saved successfully', 'success'); } else { const data = await response.json(); - alert('Failed to save config: ' + (data.error || 'Unknown error')); + showToast('Failed to save config: ' + (data.error || 'Unknown error'), 'error'); } } catch (error) { - alert('Error: ' + error.message); + showToast('Error: ' + error.message, 'error'); } } @@ -464,7 +499,7 @@ async function loadFiles() {
`).join(''); } catch (error) { - alert('Failed to load files: ' + error.message); + showToast('Failed to load files: ' + error.message, 'error'); } } @@ -479,7 +514,7 @@ async function searchMusic() { const source = document.getElementById('searchSource').value; if (!query) { - alert('Please enter a search query'); + showToast('Please enter a search query', 'error'); return; } @@ -528,6 +563,7 @@ async function searchMusic() { errorHtml += `
`; resultsDiv.innerHTML = errorHtml; + showToast(errorMsg, 'error'); updatePaginationControls(); return; } @@ -549,6 +585,7 @@ async function searchMusic() {
⚠ CONNECTION ERROR
${escapeHtml(error.message)}
`; + showToast('Connection error: ' + error.message, 'error'); updatePaginationControls(); } } @@ -742,27 +779,29 @@ async function downloadFromUrl(url) { } }); - switchTab('active'); - try { const response = await fetch('/api/download-from-url', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ + body: JSON.stringify({ url: url, quality: parseInt(quality), ...metadata }) }); - + const data = await response.json(); - - if (!response.ok) { - alert('Failed to start download: ' + (data.error || 'Unknown error')); + + if (response.ok) { + // Confirm acceptance via toast and leave the user on Search; they + // choose when to switch to the Active tab. + showToast('Queued', 'success'); + } else { + showToast('Failed to start download: ' + (data.error || 'Unknown error'), 'error'); } - + } catch (error) { - alert('Error: ' + error.message); + showToast('Error: ' + error.message, 'error'); } } diff --git a/templates/index.html b/templates/index.html index c1ebcad..bb8429d 100644 --- a/templates/index.html +++ b/templates/index.html @@ -126,7 +126,9 @@

INFO

- + +
+ From 604dff835bf5694011822d9172b41ed8d9e34dc8 Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 00:45:12 +0800 Subject: [PATCH 6/9] feat: Library tree (Artist -> Album -> Track) with lazy loading (#4) Replace the Files tab's flat file dump with a browsable Library view over the albums on disk. /api/library walks the download dir and returns album folders grouped by artist without reading any tags, so it stays instant on large libraries. /api/library/album lazily reads one album's present tracks, with titles and track numbers from embedded tags (ADR-0003) via an injectable read_audio_tags seam (mirrors run_rip), so tests need neither mutagen nor real audio files. Non-audio files (cover art, logs) are never listed as tracks, and an untagged present track falls back to its filename rather than being hidden. The view is read-only: a folder on disk carries no source URL, so there is no Redownload here. Co-Authored-By: Claude Opus 4.8 (1M context) --- app.py | 173 ++++++++++++++++++++++++++++++++- static/css/style.css | 95 ++++++++++++++++++ static/js/app.js | 112 ++++++++++++++++++---- templates/index.html | 6 +- tests/test_library_api.py | 197 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 561 insertions(+), 22 deletions(-) create mode 100644 tests/test_library_api.py diff --git a/app.py b/app.py index e0c564e..f6df5c7 100644 --- a/app.py +++ b/app.py @@ -52,6 +52,133 @@ #track is recorded in its own database. Skip detection keys on this line. SKIP_LINE_RE = re.compile(r'Marked as downloaded in the database') +#The audio file extensions streamrip writes. Used to tell tracks apart from the +#non-audio files (cover art, logs) that share an album folder, so the Library +#never lists cover.jpg as a track. +AUDIO_EXTENSIONS = ('.mp3', '.flac', '.m4a', '.opus', '.ogg', '.wav', '.aac', '.alac') + + +def _default_tag_reader(filepath): + """Read embedded tags from one audio file with mutagen, returning the raw + tracknumber/title (and the disc/total fields a later completeness slice + needs). This is the seam tests replace with a fake so the Library tests + never touch real files or require mutagen. + + streamrip writes these tags into every file (ADR-0003); mutagen reads them + back. Returns {} when the file has no readable tags.""" + from mutagen import File as MutagenFile + + audio = MutagenFile(filepath, easy=True) + if audio is None or audio.tags is None: + return {} + + def first(key): + value = audio.tags.get(key) + if isinstance(value, list): + return value[0] if value else None + return value + + return { + 'title': first('title'), + 'tracknumber': first('tracknumber'), + 'discnumber': first('discnumber'), + 'tracktotal': first('tracktotal'), + 'disctotal': first('disctotal'), + } + + +#Injectable tag-reading boundary (mirrors run_rip). Tests swap this for a fake +#that returns canned tags so the Library suite needs neither mutagen nor real +#audio files; production reads tags off disk with mutagen. +read_audio_tags = _default_tag_reader + + +def _is_audio_file(filename): + return filename.lower().endswith(AUDIO_EXTENSIONS) + + +def _parse_track_number(raw): + """Coerce a tag tracknumber/discnumber into an int. Tags are often stored as + "7" or "7/12"; we take the leading integer and ignore anything unparseable.""" + if raw is None: + return None + match = re.match(r'\s*(\d+)', str(raw)) + return int(match.group(1)) if match else None + + +def list_library_albums(download_dir): + """List the album folders in the Library (the albums on disk, ADR/Library + glossary) without reading a single tag, so it stays instant on large + libraries. An album folder is any directory that directly contains at least + one audio file; its parent directory name is taken as the Artist. + + Returns a list of {artist, album, path} sorted by artist then album, where + ``path`` is the album folder relative to ``download_dir`` (the handle the + per-album endpoint expands).""" + albums = [] + if not os.path.isdir(download_dir): + return albums + + for root, dirs, filenames in os.walk(download_dir): + dirs.sort() + if any(_is_audio_file(name) for name in filenames): + rel_path = os.path.relpath(root, download_dir) + album_name = os.path.basename(root) + parent = os.path.dirname(rel_path) + artist = os.path.basename(parent) if parent and parent != '.' else 'Unknown Artist' + albums.append({ + 'artist': artist, + 'album': album_name, + 'path': rel_path, + }) + + albums.sort(key=lambda a: (a['artist'].lower(), a['album'].lower())) + return albums + + +def read_album_tracks(download_dir, rel_path): + """Read one album folder's present tracks, lazily — called only when the user + expands an album. Each audio file's title and track number come from its + embedded tags (ADR-0003) via the read_audio_tags seam; non-audio files (cover + art, etc.) are skipped so they are never listed as tracks. + + Returns a list of {tracknumber, discnumber, title, filename} sorted by + (disc, track). A file whose tags are unreadable still appears (its title + falls back to the filename) so present tracks are never hidden.""" + album_dir = os.path.join(download_dir, rel_path) + tracks = [] + if not os.path.isdir(album_dir): + return tracks + + for filename in os.listdir(album_dir): + if not _is_audio_file(filename): + continue + filepath = os.path.join(album_dir, filename) + if not os.path.isfile(filepath): + continue + try: + tags = read_audio_tags(filepath) or {} + except Exception as e: + logger.warning(f"Failed to read tags from {filepath}: {e}") + tags = {} + + tracknumber = _parse_track_number(tags.get('tracknumber')) + discnumber = _parse_track_number(tags.get('discnumber')) + title = tags.get('title') or filename + tracks.append({ + 'tracknumber': tracknumber, + 'discnumber': discnumber, + 'title': title, + 'filename': filename, + }) + + tracks.sort(key=lambda t: ( + t['discnumber'] if t['discnumber'] is not None else 0, + t['tracknumber'] if t['tracknumber'] is not None else 0, + t['filename'].lower(), + )) + return tracks + def build_rip_command(url, quality, *, config_path=None, download_dir=None, no_db=False): """Construct the `rip url` argv. Pure (no side effects) so it can be tested @@ -711,8 +838,52 @@ def browse_downloads(): }) files.sort(key=lambda x: x['modified'], reverse=True) - return jsonify(files[:100]) + return jsonify(files[:100]) + except Exception as e: + return jsonify({'error': str(e)}), 500 + + +@app.route('/api/library') +def library_albums(): + """List the Library's album folders (Artist -> Album) for the Files tab tree. + + Cheap directory walk only — no tags are read here, so it returns instantly + even on large libraries (the per-track tags are fetched lazily per album by + /api/library/album). The Library is read-only (ADR-0003): a folder on disk + carries no source URL, so there is no Redownload here.""" + try: + return jsonify({'albums': list_library_albums(DOWNLOAD_DIR)}) + except Exception as e: + logger.exception("Failed to list Library albums") + return jsonify({'error': str(e)}), 500 + + +@app.route('/api/library/album') +def library_album_tracks(): + """Lazily read one album folder's present tracks, with titles and track + numbers from embedded tags (ADR-0003). Called when the user expands an album + in the tree. + + The ``path`` query parameter is the album's path relative to DOWNLOAD_DIR, as + returned by /api/library; it is confined to DOWNLOAD_DIR so the endpoint + cannot be used to read tags from arbitrary places on disk.""" + rel_path = request.args.get('path') + if not rel_path: + return jsonify({'error': 'path is required'}), 400 + + #Confine the resolved album folder to DOWNLOAD_DIR (reject traversal). + base = os.path.realpath(DOWNLOAD_DIR) + target = os.path.realpath(os.path.join(base, rel_path)) + if target != base and not target.startswith(base + os.sep): + return jsonify({'error': 'invalid path'}), 400 + if not os.path.isdir(target): + return jsonify({'error': 'album not found'}), 404 + + try: + tracks = read_album_tracks(DOWNLOAD_DIR, rel_path) + return jsonify({'path': rel_path, 'tracks': tracks}) except Exception as e: + logger.exception(f"Failed to read album tracks for {rel_path}") return jsonify({'error': str(e)}), 500 diff --git a/static/css/style.css b/static/css/style.css index 5672275..494eca8 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -478,6 +478,101 @@ button:disabled { letter-spacing: 0.5px; } +/* Library tree: Artist -> Album -> Track */ +.library-tree { + max-height: 500px; + overflow-y: auto; + margin-top: 20px; +} + +.library-artist { + margin-bottom: 16px; +} + +.library-artist-name { + font-size: 12px; + color: var(--text-tertiary); + text-transform: uppercase; + letter-spacing: 0.5px; + padding: 6px 4px; + border-bottom: 1px solid var(--border); + margin-bottom: 6px; +} + +.library-album { + margin-bottom: 4px; +} + +.library-album-header { + display: flex; + align-items: center; + gap: 8px; + width: 100%; + text-align: left; + padding: 10px 12px; + background: var(--bg-tertiary); + border: 1px solid var(--border); + color: var(--text-primary); + font-family: 'IBM Plex Mono', monospace; + font-size: 13px; + cursor: pointer; + transition: border-color 0.2s; +} + +.library-album-header:hover { + border-color: var(--text-tertiary); +} + +.library-album-caret { + color: var(--text-tertiary); + font-size: 11px; + width: 12px; + flex-shrink: 0; +} + +.library-album-name { + flex: 1; + word-break: break-word; +} + +.library-tracks { + display: none; + padding-left: 20px; +} + +.library-album.expanded .library-tracks { + display: block; +} + +.library-track { + display: flex; + align-items: baseline; + gap: 12px; + padding: 7px 12px; + border-left: 1px solid var(--border); + font-family: 'IBM Plex Mono', monospace; + font-size: 12px; + color: var(--text-secondary); +} + +.library-track-number { + color: var(--text-tertiary); + flex-shrink: 0; +} + +.library-track-title { + word-break: break-word; +} + +.library-tracks-loading, +.library-tracks-empty { + padding: 8px 12px; + font-size: 11px; + color: var(--text-tertiary); + text-transform: uppercase; + letter-spacing: 0.5px; +} + /* Search Styles */ .search-container { margin-bottom: 20px; diff --git a/static/js/app.js b/static/js/app.js index d886acd..559fe6e 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -405,7 +405,7 @@ function switchTab(tab, element) { } else if (tab === 'config') { loadConfig(); } else if (tab === 'files') { - loadFiles(); + loadLibrary(); } } @@ -477,29 +477,105 @@ async function saveConfig() { } } -async function loadFiles() { +// The Library view: an Artist -> Album -> Track tree over the albums on disk. +// The album list comes back instantly (no tags read server-side); a given +// album's tracks are fetched lazily the first time it is expanded. Read-only: +// a folder on disk carries no source URL, so there is no Redownload here. +async function loadLibrary() { + const container = document.getElementById('libraryTree'); + container.innerHTML = '
LOADING LIBRARY...
'; try { - const response = await fetch('/api/browse'); - const files = await response.json(); - - const container = document.getElementById('fileList'); - - if (files.length === 0) { - container.innerHTML = '
NO FILES FOUND
'; + const response = await fetch('/api/library'); + const data = await response.json(); + + if (!response.ok) { + throw new Error(data.error || 'Failed to load library'); + } + + const albums = data.albums || []; + if (albums.length === 0) { + container.innerHTML = '
NO ALBUMS FOUND
'; return; } - - container.innerHTML = files.map(file => ` -
-
${file.name}
-
- ${(file.size / 1024 / 1024).toFixed(2)} MB • - ${new Date(file.modified * 1000).toLocaleDateString()} -
+ + // Group albums under their artist to build the Artist -> Album tree. + const byArtist = new Map(); + albums.forEach(album => { + if (!byArtist.has(album.artist)) { + byArtist.set(album.artist, []); + } + byArtist.get(album.artist).push(album); + }); + + container.innerHTML = Array.from(byArtist.entries()).map(([artist, artistAlbums]) => ` +
+
${escapeHtml(artist)}
+ ${artistAlbums.map(album => ` +
+ +
+
+ `).join('')}
`).join(''); } catch (error) { - showToast('Failed to load files: ' + error.message, 'error'); + container.innerHTML = '
FAILED TO LOAD LIBRARY
'; + showToast('Failed to load library: ' + error.message, 'error'); + } +} + +async function toggleAlbum(button) { + const albumEl = button.closest('.library-album'); + const tracksEl = albumEl.querySelector('.library-tracks'); + const caret = button.querySelector('.library-album-caret'); + + // Collapse if already expanded. + if (albumEl.classList.contains('expanded')) { + albumEl.classList.remove('expanded'); + caret.textContent = '▸'; + return; + } + + albumEl.classList.add('expanded'); + caret.textContent = '▾'; + + // Lazily fetch this album's tracks only the first time it is expanded. + if (albumEl.dataset.loaded === 'true') { + return; + } + + const path = albumEl.dataset.path; + tracksEl.innerHTML = '
LOADING TRACKS...
'; + + try { + const response = await fetch('/api/library/album?path=' + encodeURIComponent(path)); + const data = await response.json(); + + if (!response.ok) { + throw new Error(data.error || 'Failed to load tracks'); + } + + const tracks = data.tracks || []; + if (tracks.length === 0) { + tracksEl.innerHTML = '
NO TRACKS
'; + } else { + tracksEl.innerHTML = tracks.map(track => { + const num = track.tracknumber != null ? String(track.tracknumber).padStart(2, '0') : '--'; + return ` +
+ ${escapeHtml(num)} + ${escapeHtml(track.title || '')} +
+ `; + }).join(''); + } + albumEl.dataset.loaded = 'true'; + } catch (error) { + tracksEl.innerHTML = '
FAILED TO LOAD TRACKS
'; + showToast('Failed to load tracks: ' + error.message, 'error'); } } diff --git a/templates/index.html b/templates/index.html index bb8429d..a0b0f86 100644 --- a/templates/index.html +++ b/templates/index.html @@ -111,9 +111,9 @@

INFO

- -
-
CLICK REFRESH TO LOAD FILES
+ +
+
CLICK REFRESH TO LOAD LIBRARY
diff --git a/tests/test_library_api.py b/tests/test_library_api.py new file mode 100644 index 0000000..27cc21f --- /dev/null +++ b/tests/test_library_api.py @@ -0,0 +1,197 @@ +"""Seam 1: the Library tree endpoints, driven through the Flask test client +against a temp download directory. + +The Library is the set of already-downloaded albums as they exist on disk +(CONTEXT.md / Library glossary). These tests stub the tag-reading seam +(read_audio_tags) so they exercise the folder-listing and per-album endpoints +without mutagen and without touching real audio files. They never shell out to +real `rip`. +""" +import os + +import pytest + +import app as app_module + + +def _make_album(base, artist, album, audio_files, extra_files=()): + """Create an Artist/Album folder containing the given audio files (and any + non-audio files, e.g. cover art) under the temp download dir.""" + album_dir = os.path.join(base, artist, album) + os.makedirs(album_dir, exist_ok=True) + for name in audio_files: + open(os.path.join(album_dir, name), 'wb').close() + for name in extra_files: + open(os.path.join(album_dir, name), 'wb').close() + return album_dir + + +@pytest.fixture +def library(tmp_path, monkeypatch): + """Point the app at a temp download dir and return its path. Restores the + real DOWNLOAD_DIR and tag reader afterwards via monkeypatch.""" + base = str(tmp_path / 'Music') + os.makedirs(base, exist_ok=True) + monkeypatch.setattr(app_module, 'DOWNLOAD_DIR', base) + return base + + +def _albums(client): + return client.get('/api/library').get_json()['albums'] + + +def _tracks(client, path): + resp = client.get('/api/library/album', query_string={'path': path}) + return resp + + +def test_library_lists_album_folders_grouped_by_artist(client, library): + _make_album(library, 'Radiohead', 'OK Computer', ['01.flac', '02.flac']) + _make_album(library, 'Boards of Canada', 'Geogaddi', ['1.mp3']) + + albums = _albums(client) + pairs = {(a['artist'], a['album']) for a in albums} + assert ('Radiohead', 'OK Computer') in pairs + assert ('Boards of Canada', 'Geogaddi') in pairs + # Sorted by artist then album. + assert albums[0]['artist'] == 'Boards of Canada' + + +def test_library_listing_reads_no_tags(client, library, monkeypatch): + # Listing album folders must be a cheap walk: it must never call the tag + # reader (which is what keeps it instant on large libraries). + _make_album(library, 'Artist', 'Album', ['01.flac', '02.flac']) + + def boom(filepath): + raise AssertionError("listing albums must not read tags") + + monkeypatch.setattr(app_module, 'read_audio_tags', boom) + + albums = _albums(client) + assert len(albums) == 1 + + +def test_library_empty_when_no_albums(client, library): + assert _albums(client) == [] + + +def test_folder_without_audio_is_not_an_album(client, library): + # An artist folder that only holds non-audio files is not an album folder. + bare = os.path.join(library, 'SomeArtist') + os.makedirs(bare, exist_ok=True) + open(os.path.join(bare, 'notes.txt'), 'wb').close() + assert _albums(client) == [] + + +def test_album_tracks_use_tagged_title_and_number(client, library, monkeypatch): + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['a.flac', 'b.flac']), + library, + ) + + tags = { + 'a.flac': {'title': 'Opener', 'tracknumber': '1', 'tracktotal': '2'}, + 'b.flac': {'title': 'Closer', 'tracknumber': '2', 'tracktotal': '2'}, + } + monkeypatch.setattr( + app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)], + ) + + resp = _tracks(client, rel) + assert resp.status_code == 200 + tracks = resp.get_json()['tracks'] + assert [t['title'] for t in tracks] == ['Opener', 'Closer'] + assert [t['tracknumber'] for t in tracks] == [1, 2] + + +def test_album_tracks_sorted_by_track_number(client, library, monkeypatch): + # Files on disk in arbitrary order must come back sorted by track number. + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['z.flac', 'a.flac', 'm.flac']), + library, + ) + tags = { + 'z.flac': {'title': 'Three', 'tracknumber': '3'}, + 'a.flac': {'title': 'One', 'tracknumber': '1'}, + 'm.flac': {'title': 'Two', 'tracknumber': '2'}, + } + monkeypatch.setattr( + app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)], + ) + + tracks = _tracks(client, rel).get_json()['tracks'] + assert [t['title'] for t in tracks] == ['One', 'Two', 'Three'] + + +def test_non_audio_files_are_not_listed_as_tracks(client, library, monkeypatch): + # Cover art and other non-audio files share the album folder but must never + # appear as tracks. + rel = os.path.relpath( + _make_album( + library, 'Artist', 'Album', + audio_files=['01.flac'], + extra_files=['cover.jpg', 'folder.png', 'streamrip.log'], + ), + library, + ) + monkeypatch.setattr( + app_module, 'read_audio_tags', + lambda fp: {'title': 'Only Track', 'tracknumber': '1'}, + ) + + tracks = _tracks(client, rel).get_json()['tracks'] + assert len(tracks) == 1 + assert tracks[0]['title'] == 'Only Track' + filenames = [t['filename'] for t in tracks] + assert 'cover.jpg' not in filenames + + +def test_track_falls_back_to_filename_when_untagged(client, library, monkeypatch): + # A present file whose tags are unreadable is still shown (present tracks are + # never hidden); its title falls back to the filename. + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['mystery.flac']), + library, + ) + monkeypatch.setattr(app_module, 'read_audio_tags', lambda fp: {}) + + tracks = _tracks(client, rel).get_json()['tracks'] + assert len(tracks) == 1 + assert tracks[0]['title'] == 'mystery.flac' + assert tracks[0]['tracknumber'] is None + + +def test_album_tracks_requires_path(client, library): + resp = client.get('/api/library/album') + assert resp.status_code == 400 + + +def test_album_tracks_rejects_path_traversal(client, library): + resp = _tracks(client, '../../etc') + assert resp.status_code in (400, 404) + + +def test_album_tracks_unknown_path_is_404(client, library): + resp = _tracks(client, 'No/Such/Album') + assert resp.status_code == 404 + + +def test_disc_number_orders_before_track_number(client, library, monkeypatch): + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['d2t1.flac', 'd1t2.flac', 'd1t1.flac']), + library, + ) + tags = { + 'd2t1.flac': {'title': 'D2T1', 'discnumber': '2', 'tracknumber': '1'}, + 'd1t2.flac': {'title': 'D1T2', 'discnumber': '1', 'tracknumber': '2'}, + 'd1t1.flac': {'title': 'D1T1', 'discnumber': '1', 'tracknumber': '1'}, + } + monkeypatch.setattr( + app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)], + ) + + tracks = _tracks(client, rel).get_json()['tracks'] + assert [t['title'] for t in tracks] == ['D1T1', 'D1T2', 'D2T1'] From 551304d10e07cf4cfddbf0706293a65016685bf6 Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 00:51:26 +0800 Subject: [PATCH 7/9] feat: album completeness badges and missing-track gap rows in the Library (#7) Derive Album completeness (ADR-0003) entirely from the embedded tags of present tracks: a pure assess_completeness() takes plain disc/track/tracktotal /disctotal data and returns Complete / Incomplete (missing numbers per disc) / Unknown, detecting interior and trailing gaps per disc with no network and no stored metadata. The per-album endpoint now serves the full expected sequence (present tracks plus greyed, number-only missing gap rows) and the badge data, cached per album keyed on folder mtime so re-expanding an unchanged album reads no tags. The Library UI shows a COMPLETE / INCOMPLETE (n missing) / UNKNOWN badge per album and renders "Track N - missing" gap rows in sequence position. Co-Authored-By: Claude Opus 4.8 (1M context) --- app.py | 174 +++++++++++++++++++++++++++++++++- static/css/style.css | 41 ++++++++ static/js/app.js | 68 ++++++++++++- tests/test_completeness.py | 118 +++++++++++++++++++++++ tests/test_library_api.py | 143 ++++++++++++++++++++++++++++ tests/test_mutagen_adapter.py | 102 ++++++++++++++++++++ 6 files changed, 639 insertions(+), 7 deletions(-) create mode 100644 tests/test_completeness.py create mode 100644 tests/test_mutagen_adapter.py diff --git a/app.py b/app.py index f6df5c7..145210e 100644 --- a/app.py +++ b/app.py @@ -106,6 +106,80 @@ def _parse_track_number(raw): return int(match.group(1)) if match else None +def assess_completeness(present_tracks): + """Assess an album's Album completeness (ADR-0003 / CONTEXT glossary) purely + from the embedded tags of the tracks *present* on disk — no network, no + stored tracklist. ``present_tracks`` is a list of plain dicts with raw tag + fields ``disc``/``track``/``tracktotal``/``disctotal`` (any may be missing). + + Returns a dict:: + + { + 'status': 'complete' | 'incomplete' | 'unknown', + 'missing': [{'disc': D, 'track': N}, ...], # sorted, [] unless incomplete + 'discs': [ + {'disc': D, 'tracktotal': T, 'present': [...], 'missing': [...]}, + ... + ], + } + + Completeness logic: any present track reveals its disc's true ``tracktotal``, + so every gap — interior and trailing — is detectable. Each disc is assessed + independently and missing tracks are reported against their own disc. An + album with no present track carrying a readable ``tracktotal`` is **Unknown** + (never guessed); a track with no readable disc is treated as disc 1.""" + discs = {} + saw_total = False + + for raw in present_tracks: + disc = _parse_track_number(raw.get('disc')) + track = _parse_track_number(raw.get('track')) + tracktotal = _parse_track_number(raw.get('tracktotal')) + + #A track with no readable disc tag belongs to disc 1 (the common + #single-disc case where streamrip omits/zeroes the disc number). + if disc is None or disc < 1: + disc = 1 + + entry = discs.setdefault(disc, {'present': set(), 'tracktotal': None}) + if track is not None and track >= 1: + entry['present'].add(track) + if tracktotal is not None and tracktotal >= 1: + saw_total = True + #Keep the largest total seen on this disc; a present track number + #beyond it would itself imply a larger album, handled below. + if entry['tracktotal'] is None or tracktotal > entry['tracktotal']: + entry['tracktotal'] = tracktotal + + if not saw_total: + return {'status': 'unknown', 'missing': [], 'discs': []} + + disc_reports = [] + all_missing = [] + for disc in sorted(discs): + entry = discs[disc] + present = sorted(entry['present']) + total = entry['tracktotal'] + #If a present track number exceeds the tagged total (or no total on this + #disc but present numbers exist), trust the highest present number so we + #never report a present track as missing. + highest_present = present[-1] if present else 0 + if total is None or highest_present > total: + total = highest_present + missing = [n for n in range(1, total + 1) if n not in entry['present']] + disc_reports.append({ + 'disc': disc, + 'tracktotal': total, + 'present': present, + 'missing': missing, + }) + all_missing.extend({'disc': disc, 'track': n} for n in missing) + + all_missing.sort(key=lambda m: (m['disc'], m['track'])) + status = 'complete' if not all_missing else 'incomplete' + return {'status': status, 'missing': all_missing, 'discs': disc_reports} + + def list_library_albums(download_dir): """List the album folders in the Library (the albums on disk, ADR/Library glossary) without reading a single tag, so it stays instant on large @@ -170,6 +244,10 @@ def read_album_tracks(download_dir, rel_path): 'discnumber': discnumber, 'title': title, 'filename': filename, + #Carry the raw totals through so completeness can be assessed from + #the same tag read (ADR-0003) without a second pass over the files. + 'tracktotal': _parse_track_number(tags.get('tracktotal')), + 'disctotal': _parse_track_number(tags.get('disctotal')), }) tracks.sort(key=lambda t: ( @@ -180,6 +258,94 @@ def read_album_tracks(download_dir, rel_path): return tracks +#Per-album completeness assessment cache, keyed on the album folder path and its +#mtime (ADR-0003: completeness comes from the tags on disk). Re-expanding an +#unchanged album returns the cached payload without re-reading any tags; +#downloading/deleting a track changes the folder mtime and invalidates it. +album_assessment_cache = {} +album_cache_lock = threading.Lock() + + +def _album_folder_mtime(album_dir): + """The album folder's own mtime — bumped by the filesystem whenever a track + file is added to or removed from the directory, which is exactly the events + that can change completeness. Returns None when the folder is missing.""" + try: + return os.stat(album_dir).st_mtime + except OSError: + return None + + +def _build_track_rows(present_tracks, assessment): + """Merge the present tracks with greyed "missing" gap rows so the expanded + album renders the full expected sequence 1…tracktotal per disc, with absent + tracks shown in sequence position (interior and trailing). A missing track is + shown by number only — it left no title on disk (ADR-0003).""" + rows = list(present_tracks) + for miss in assessment.get('missing', []): + rows.append({ + 'tracknumber': miss['track'], + 'discnumber': miss['disc'], + 'title': None, + 'filename': None, + 'tracktotal': None, + 'disctotal': None, + 'missing': True, + }) + #Sort present and missing rows into one expected sequence. A present track + #with no disc tag belongs to disc 1, exactly as assess_completeness assigns + #its gap rows, so the two interleave correctly (no disc tag -> disc 1). + rows.sort(key=lambda t: ( + t['discnumber'] if t['discnumber'] is not None and t['discnumber'] >= 1 else 1, + t['tracknumber'] if t['tracknumber'] is not None else 0, + (t.get('filename') or '').lower(), + )) + for row in rows: + row.setdefault('missing', False) + return rows + + +def get_album_assessment(download_dir, rel_path): + """Read one album's present tracks, assess its Album completeness, and return + the payload the per-album endpoint serves: the full track sequence (present + tracks plus greyed missing gap rows) and the completeness badge data. + + Cached per album keyed on the folder's mtime (ADR-0003): re-expanding an + unchanged album returns the cached result and does not re-read a single tag.""" + album_dir = os.path.join(download_dir, rel_path) + mtime = _album_folder_mtime(album_dir) + + cache_key = os.path.realpath(album_dir) + with album_cache_lock: + cached = album_assessment_cache.get(cache_key) + if cached is not None and cached['mtime'] == mtime and mtime is not None: + return cached['payload'] + + present_tracks = read_album_tracks(download_dir, rel_path) + assessment = assess_completeness([ + { + 'disc': t['discnumber'], + 'track': t['tracknumber'], + 'tracktotal': t['tracktotal'], + 'disctotal': t['disctotal'], + } + for t in present_tracks + ]) + payload = { + 'tracks': _build_track_rows(present_tracks, assessment), + 'completeness': { + 'status': assessment['status'], + 'missing_count': len(assessment['missing']), + 'missing': assessment['missing'], + }, + } + + if mtime is not None: + with album_cache_lock: + album_assessment_cache[cache_key] = {'mtime': mtime, 'payload': payload} + return payload + + def build_rip_command(url, quality, *, config_path=None, download_dir=None, no_db=False): """Construct the `rip url` argv. Pure (no side effects) so it can be tested directly and reused for Redownload (no_db -> --no-db). This is the single @@ -880,8 +1046,12 @@ def library_album_tracks(): return jsonify({'error': 'album not found'}), 404 try: - tracks = read_album_tracks(DOWNLOAD_DIR, rel_path) - return jsonify({'path': rel_path, 'tracks': tracks}) + payload = get_album_assessment(DOWNLOAD_DIR, rel_path) + return jsonify({ + 'path': rel_path, + 'tracks': payload['tracks'], + 'completeness': payload['completeness'], + }) except Exception as e: logger.exception(f"Failed to read album tracks for {rel_path}") return jsonify({'error': str(e)}), 500 diff --git a/static/css/style.css b/static/css/style.css index 494eca8..8138285 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -535,6 +535,39 @@ button:disabled { word-break: break-word; } +/* Album completeness badge (ADR-0003): COMPLETE / INCOMPLETE (n missing) / + UNKNOWN. Derived entirely from embedded tags of present tracks. */ +.library-album-badge { + flex-shrink: 0; + font-size: 10px; + letter-spacing: 0.5px; + text-transform: uppercase; + padding: 2px 8px; + border: 1px solid var(--border); + border-radius: 2px; + white-space: nowrap; +} + +.library-album-badge.badge-pending { + color: var(--text-tertiary); + border-color: var(--border); +} + +.library-album-badge.badge-complete { + color: var(--success); + border-color: var(--success); +} + +.library-album-badge.badge-incomplete { + color: var(--warning); + border-color: var(--warning); +} + +.library-album-badge.badge-unknown { + color: var(--text-tertiary); + border-color: var(--text-tertiary); +} + .library-tracks { display: none; padding-left: 20px; @@ -560,6 +593,14 @@ button:disabled { flex-shrink: 0; } +/* A missing track (never downloaded, no tags on disk): greyed, number-only, + rendered in its sequence position so interior and trailing gaps are visible. */ +.library-track-missing { + color: var(--text-tertiary); + font-style: italic; + opacity: 0.7; +} + .library-track-title { word-break: break-word; } diff --git a/static/js/app.js b/static/js/app.js index 559fe6e..728cc37 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -515,18 +515,60 @@ async function loadLibrary() {
`).join('')} `).join(''); + + // Every album carries a completeness badge (ADR-0003). The badge is + // derived from embedded tags, so it is filled lazily per album from the + // per-album endpoint (cached server-side on folder mtime) without + // blocking the instant, tag-free album listing above. + document.querySelectorAll('#libraryTree .library-album').forEach(loadAlbumBadge); } catch (error) { container.innerHTML = '
FAILED TO LOAD LIBRARY
'; showToast('Failed to load library: ' + error.message, 'error'); } } +// Render a completeness badge into an album's header: COMPLETE / INCOMPLETE +// (n missing) / UNKNOWN. Caches the fetched album payload on the element so +// expanding it later reuses it without a second request. +function applyCompletenessBadge(albumEl, completeness) { + const badge = albumEl.querySelector('[data-role="badge"]'); + if (!badge) return; + const status = completeness ? completeness.status : 'unknown'; + badge.classList.remove('badge-pending', 'badge-complete', 'badge-incomplete', 'badge-unknown'); + if (status === 'complete') { + badge.classList.add('badge-complete'); + badge.textContent = 'COMPLETE'; + } else if (status === 'incomplete') { + const n = completeness.missing_count || 0; + badge.classList.add('badge-incomplete'); + badge.textContent = `INCOMPLETE (${n} missing)`; + } else { + badge.classList.add('badge-unknown'); + badge.textContent = 'UNKNOWN'; + } +} + +async function loadAlbumBadge(albumEl) { + const path = albumEl.dataset.path; + try { + const response = await fetch('/api/library/album?path=' + encodeURIComponent(path)); + const data = await response.json(); + if (!response.ok) throw new Error(data.error || 'Failed'); + // Stash the payload so toggleAlbum reuses it (server already cached it). + albumEl._albumData = data; + applyCompletenessBadge(albumEl, data.completeness); + } catch (error) { + applyCompletenessBadge(albumEl, { status: 'unknown' }); + } +} + async function toggleAlbum(button) { const albumEl = button.closest('.library-album'); const tracksEl = albumEl.querySelector('.library-tracks'); @@ -551,19 +593,35 @@ async function toggleAlbum(button) { tracksEl.innerHTML = '
LOADING TRACKS...
'; try { - const response = await fetch('/api/library/album?path=' + encodeURIComponent(path)); - const data = await response.json(); - - if (!response.ok) { - throw new Error(data.error || 'Failed to load tracks'); + // Reuse the payload the badge already fetched (the server caches it on + // folder mtime, so this would hit that cache anyway). + let data = albumEl._albumData; + if (!data) { + const response = await fetch('/api/library/album?path=' + encodeURIComponent(path)); + data = await response.json(); + if (!response.ok) { + throw new Error(data.error || 'Failed to load tracks'); + } + albumEl._albumData = data; } + applyCompletenessBadge(albumEl, data.completeness); const tracks = data.tracks || []; if (tracks.length === 0) { tracksEl.innerHTML = '
NO TRACKS
'; } else { + // The expected sequence 1…tracktotal per disc, with absent tracks as + // greyed, number-only "Track N — missing" rows (ADR-0003). tracksEl.innerHTML = tracks.map(track => { const num = track.tracknumber != null ? String(track.tracknumber).padStart(2, '0') : '--'; + if (track.missing) { + return ` +
+ ${escapeHtml(num)} + Track ${escapeHtml(String(track.tracknumber))} — missing +
+ `; + } return `
${escapeHtml(num)} diff --git a/tests/test_completeness.py b/tests/test_completeness.py new file mode 100644 index 0000000..25ad3ce --- /dev/null +++ b/tests/test_completeness.py @@ -0,0 +1,118 @@ +"""Pure Album-completeness assessment (ADR-0003). + +``assess_completeness`` takes plain tag data — ``disc``/``track``/``tracktotal`` +/``disctotal`` per present track — and returns Complete / Incomplete (with the +missing numbers per disc) / Unknown, with no filesystem, network, or mutagen +involvement. Any present track reveals its disc's true total, so every gap — +interior and trailing — is detectable from disk alone. +""" +import app as app_module + +assess = app_module.assess_completeness + + +def test_complete_album_when_every_expected_track_present(): + result = assess([ + {'track': '1', 'tracktotal': '3'}, + {'track': '2', 'tracktotal': '3'}, + {'track': '3', 'tracktotal': '3'}, + ]) + assert result['status'] == 'complete' + assert result['missing'] == [] + + +def test_interior_gap_is_detected(): + result = assess([ + {'track': '1', 'tracktotal': '3'}, + {'track': '3', 'tracktotal': '3'}, + ]) + assert result['status'] == 'incomplete' + assert result['missing'] == [{'disc': 1, 'track': 2}] + + +def test_trailing_gap_is_detected(): + # Only track 1 of 3 present: tracks 2 and 3 are trailing gaps, detectable + # only because a present track reveals the true total of 3. + result = assess([ + {'track': '1', 'tracktotal': '3'}, + ]) + assert result['status'] == 'incomplete' + assert result['missing'] == [{'disc': 1, 'track': 2}, {'disc': 1, 'track': 3}] + + +def test_multi_disc_gap_reported_against_its_disc(): + result = assess([ + {'disc': '1', 'track': '1', 'tracktotal': '2', 'disctotal': '2'}, + {'disc': '1', 'track': '2', 'tracktotal': '2', 'disctotal': '2'}, + {'disc': '2', 'track': '1', 'tracktotal': '3', 'disctotal': '2'}, + {'disc': '2', 'track': '3', 'tracktotal': '3', 'disctotal': '2'}, + ]) + assert result['status'] == 'incomplete' + # Disc 1 complete; disc 2 missing track 2 — reported against disc 2. + assert result['missing'] == [{'disc': 2, 'track': 2}] + + +def test_multi_disc_complete(): + result = assess([ + {'disc': '1', 'track': '1', 'tracktotal': '2'}, + {'disc': '1', 'track': '2', 'tracktotal': '2'}, + {'disc': '2', 'track': '1', 'tracktotal': '2'}, + {'disc': '2', 'track': '2', 'tracktotal': '2'}, + ]) + assert result['status'] == 'complete' + assert result['missing'] == [] + + +def test_zero_readable_tags_is_unknown_not_guessed(): + # No present track carries a readable total: completeness cannot be + # determined, so it is Unknown rather than guessed from the sequence. + result = assess([ + {'track': None}, + {'track': None}, + ]) + assert result['status'] == 'unknown' + assert result['missing'] == [] + + +def test_empty_album_is_unknown(): + assert assess([])['status'] == 'unknown' + + +def test_present_numbers_without_total_is_unknown(): + # Track numbers present but no tracktotal anywhere -> can't know the true + # total, so Unknown (cannot detect trailing gaps). + result = assess([ + {'track': '1'}, + {'track': '2'}, + ]) + assert result['status'] == 'unknown' + + +def test_track_beyond_tagged_total_is_never_reported_missing(): + # A present track number larger than the tagged total must not be reported + # missing; trust the highest present number. + result = assess([ + {'track': '1', 'tracktotal': '2'}, + {'track': '2', 'tracktotal': '2'}, + {'track': '3', 'tracktotal': '2'}, + ]) + assert result['status'] == 'complete' + assert result['missing'] == [] + + +def test_track_with_no_disc_tag_is_disc_one(): + result = assess([ + {'track': '1', 'tracktotal': '2'}, + {'track': '2', 'tracktotal': '2'}, + ]) + assert result['discs'][0]['disc'] == 1 + assert result['status'] == 'complete' + + +def test_slashed_totals_are_parsed(): + # streamrip-style "1/3" tracknumber/tracktotal forms parse to the leading int. + result = assess([ + {'track': '1/3', 'tracktotal': '3/3'}, + ]) + assert result['status'] == 'incomplete' + assert result['missing'] == [{'disc': 1, 'track': 2}, {'disc': 1, 'track': 3}] diff --git a/tests/test_library_api.py b/tests/test_library_api.py index 27cc21f..5ac5f13 100644 --- a/tests/test_library_api.py +++ b/tests/test_library_api.py @@ -178,6 +178,149 @@ def test_album_tracks_unknown_path_is_404(client, library): assert resp.status_code == 404 +def test_album_endpoint_reports_complete(client, library, monkeypatch): + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['a.flac', 'b.flac']), + library, + ) + tags = { + 'a.flac': {'title': 'One', 'tracknumber': '1', 'tracktotal': '2'}, + 'b.flac': {'title': 'Two', 'tracknumber': '2', 'tracktotal': '2'}, + } + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)]) + + data = _tracks(client, rel).get_json() + assert data['completeness']['status'] == 'complete' + assert data['completeness']['missing_count'] == 0 + # No gap rows: every row is a present track. + assert all(t['missing'] is False for t in data['tracks']) + + +def test_album_endpoint_reports_incomplete_with_gap_rows(client, library, monkeypatch): + # Track 2 of 3 missing on disk -> INCOMPLETE (1 missing), with a greyed gap + # row at sequence position 2. + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['t1.flac', 't3.flac']), + library, + ) + tags = { + 't1.flac': {'title': 'One', 'tracknumber': '1', 'tracktotal': '3'}, + 't3.flac': {'title': 'Three', 'tracknumber': '3', 'tracktotal': '3'}, + } + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)]) + + data = _tracks(client, rel).get_json() + assert data['completeness']['status'] == 'incomplete' + assert data['completeness']['missing_count'] == 1 + rows = data['tracks'] + assert [t['tracknumber'] for t in rows] == [1, 2, 3] + assert rows[1]['missing'] is True + assert rows[1]['title'] is None # a missing track has no title on disk + + +def test_album_endpoint_trailing_gap_rows(client, library, monkeypatch): + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['t1.flac']), + library, + ) + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: {'title': 'One', 'tracknumber': '1', 'tracktotal': '3'}) + + rows = _tracks(client, rel).get_json()['tracks'] + assert [t['tracknumber'] for t in rows] == [1, 2, 3] + assert [t['missing'] for t in rows] == [False, True, True] + + +def test_album_endpoint_multi_disc_missing_against_disc(client, library, monkeypatch): + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', + ['d1t1.flac', 'd1t2.flac', 'd2t1.flac']), + library, + ) + tags = { + 'd1t1.flac': {'tracknumber': '1', 'discnumber': '1', 'tracktotal': '2', 'disctotal': '2'}, + 'd1t2.flac': {'tracknumber': '2', 'discnumber': '1', 'tracktotal': '2', 'disctotal': '2'}, + 'd2t1.flac': {'tracknumber': '1', 'discnumber': '2', 'tracktotal': '2', 'disctotal': '2'}, + } + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)]) + + data = _tracks(client, rel).get_json() + assert data['completeness']['status'] == 'incomplete' + # Disc 2 track 2 missing, reported against disc 2. + assert data['completeness']['missing'] == [{'disc': 2, 'track': 2}] + gap = [t for t in data['tracks'] if t['missing']] + assert len(gap) == 1 + assert gap[0]['discnumber'] == 2 and gap[0]['tracknumber'] == 2 + + +def test_album_endpoint_no_tags_is_unknown(client, library, monkeypatch): + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['x.flac']), + library, + ) + monkeypatch.setattr(app_module, 'read_audio_tags', lambda fp: {}) + + data = _tracks(client, rel).get_json() + assert data['completeness']['status'] == 'unknown' + # No total known -> no gap rows are invented. + assert all(t['missing'] is False for t in data['tracks']) + + +def test_assessment_cached_on_folder_mtime(client, library, monkeypatch): + # Re-expanding an unchanged album must not re-read tags: the per-album + # assessment is cached keyed on the folder mtime (ADR-0003). + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', ['a.flac', 'b.flac']), + library, + ) + app_module.album_assessment_cache.clear() + + calls = {'n': 0} + + def counting_reader(fp): + calls['n'] += 1 + return {'title': 'T', 'tracknumber': '1', 'tracktotal': '2'} + + monkeypatch.setattr(app_module, 'read_audio_tags', counting_reader) + + _tracks(client, rel) + first = calls['n'] + assert first > 0 + + # Second request on the unchanged folder: served from cache, no new reads. + _tracks(client, rel) + assert calls['n'] == first + + +def test_assessment_cache_invalidated_when_folder_changes(client, library, monkeypatch): + # Deleting/adding a track changes the folder mtime, which must invalidate the + # cached assessment so completeness reflects the new contents. + album_dir = _make_album(library, 'Artist', 'Album', ['a.flac', 'b.flac']) + rel = os.path.relpath(album_dir, library) + app_module.album_assessment_cache.clear() + + tags = { + 'a.flac': {'title': 'One', 'tracknumber': '1', 'tracktotal': '2'}, + 'b.flac': {'title': 'Two', 'tracknumber': '2', 'tracktotal': '2'}, + } + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)]) + + assert _tracks(client, rel).get_json()['completeness']['status'] == 'complete' + + # Delete track 2 and bump the folder mtime so the cache key changes. + os.remove(os.path.join(album_dir, 'b.flac')) + future = os.stat(album_dir).st_mtime + 10 + os.utime(album_dir, (future, future)) + + data = _tracks(client, rel).get_json() + assert data['completeness']['status'] == 'incomplete' + assert data['completeness']['missing'] == [{'disc': 1, 'track': 2}] + + def test_disc_number_orders_before_track_number(client, library, monkeypatch): rel = os.path.relpath( _make_album(library, 'Artist', 'Album', ['d2t1.flac', 'd1t2.flac', 'd1t1.flac']), diff --git a/tests/test_mutagen_adapter.py b/tests/test_mutagen_adapter.py new file mode 100644 index 0000000..f58e4f2 --- /dev/null +++ b/tests/test_mutagen_adapter.py @@ -0,0 +1,102 @@ +"""Integration tests for the mutagen tag-reading adapter (ADR-0003) against real +tagged sample files on disk — not the stubbed seam. + +streamrip writes tracknumber/tracktotal/discnumber/disctotal into every file; +these tests build genuine FLAC files (silent, encoded with the system `flac`), +tag them with mutagen exactly as streamrip would, and assert the production +adapter (``app.read_audio_tags``, i.e. the real mutagen reader) reads them back +and that the whole album endpoint then assesses completeness from those tags. + +If the system `flac` encoder or mutagen is unavailable the module is skipped. +""" +import os +import shutil +import struct +import subprocess +import wave + +import pytest + +import app as app_module + +pytest.importorskip("mutagen") +from mutagen.flac import FLAC # noqa: E402 + +if shutil.which("flac") is None: + pytest.skip("system `flac` encoder not available", allow_module_level=True) + + +def _write_flac(path, **tags): + """Encode a short silent FLAC at ``path`` and write the given Vorbis tags + (the same fields streamrip embeds).""" + wav_path = path + ".wav" + with wave.open(wav_path, "wb") as w: + w.setnchannels(1) + w.setsampwidth(2) + w.setframerate(8000) + w.writeframes(struct.pack("<800h", *([0] * 800))) + subprocess.run( + ["flac", "--totally-silent", "-f", "-o", path, wav_path], + check=True, + ) + os.remove(wav_path) + audio = FLAC(path) + for key, value in tags.items(): + audio[key] = str(value) + audio.save() + + +def test_real_flac_tags_are_read_by_production_adapter(tmp_path): + path = str(tmp_path / "01.flac") + _write_flac( + path, + title="Opener", + tracknumber="1", + tracktotal="3", + discnumber="1", + disctotal="1", + ) + # The production reader (real mutagen), not the test seam. + tags = app_module._default_tag_reader(path) + assert tags["title"] == "Opener" + assert tags["tracknumber"] == "1" + assert tags["tracktotal"] == "3" + + +def test_album_endpoint_assesses_real_tagged_files(client, tmp_path, monkeypatch): + base = str(tmp_path / "Music") + album_dir = os.path.join(base, "Artist", "Album") + os.makedirs(album_dir, exist_ok=True) + # A 3-track album with track 2 missing on disk -> INCOMPLETE (1 missing). + _write_flac(os.path.join(album_dir, "01.flac"), + title="One", tracknumber="1", tracktotal="3") + _write_flac(os.path.join(album_dir, "03.flac"), + title="Three", tracknumber="3", tracktotal="3") + monkeypatch.setattr(app_module, "DOWNLOAD_DIR", base) + # Use the real mutagen adapter (the seam is left at its production default). + + resp = client.get("/api/library/album", query_string={"path": "Artist/Album"}) + assert resp.status_code == 200 + data = resp.get_json() + assert data["completeness"]["status"] == "incomplete" + assert data["completeness"]["missing_count"] == 1 + assert data["completeness"]["missing"] == [{"disc": 1, "track": 2}] + + # The expected sequence renders the gap row at position 2. + rows = data["tracks"] + assert [t["tracknumber"] for t in rows] == [1, 2, 3] + assert rows[1]["missing"] is True + assert rows[0]["missing"] is False and rows[2]["missing"] is False + + +def test_album_with_no_readable_tags_is_unknown(client, tmp_path, monkeypatch): + base = str(tmp_path / "Music") + album_dir = os.path.join(base, "Artist", "Album") + os.makedirs(album_dir, exist_ok=True) + # A FLAC with no completeness tags at all (only a title). + _write_flac(os.path.join(album_dir, "mystery.flac"), title="Mystery") + monkeypatch.setattr(app_module, "DOWNLOAD_DIR", base) + + resp = client.get("/api/library/album", query_string={"path": "Artist/Album"}) + data = resp.get_json() + assert data["completeness"]["status"] == "unknown" From 2545910e86808a2b18ab957fd90dc3e1d0a01605 Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 01:57:19 +0800 Subject: [PATCH 8/9] ci: removed docker image upload --- .github/workflows/docker-publish.yml | 124 +++++++++++++-------------- 1 file changed, 62 insertions(+), 62 deletions(-) diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 6c0ca6a..930985e 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -1,62 +1,62 @@ -name: Build and Push Docker Image - -on: - push: - branches: [ "main", "master" ] - tags: [ "v*.*.*" ] - pull_request: - branches: [ "main", "master" ] - -env: - REGISTRY_IMAGE: anoddname/streamrip-web-gui - -jobs: - build: - runs-on: ubuntu-latest - permissions: - contents: read - packages: write - id-token: write - - steps: - - name: Checkout - uses: actions/checkout@v4 - - - name: Extract metadata - id: meta - uses: docker/metadata-action@v5 - with: - images: ${{ env.REGISTRY_IMAGE }} - tags: | - type=ref,event=branch - type=ref,event=pr - type=semver,pattern={{version}} - type=semver,pattern={{major}}.{{minor}} - type=raw,value=latest,enable={{is_default_branch}} - - - name: Set up QEMU - uses: docker/setup-qemu-action@v3 - - - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 - - - name: Log in to Docker Hub - if: github.event_name != 'pull_request' - uses: docker/login-action@v3 - with: - username: ${{ secrets.DOCKERHUB_USERNAME }} - password: ${{ secrets.DOCKERHUB_TOKEN }} - - - name: Build and push Docker image - uses: docker/build-push-action@v6 - with: - context: . - platforms: linux/amd64,linux/arm64 - push: ${{ github.event_name != 'pull_request' }} - tags: ${{ steps.meta.outputs.tags }} - labels: ${{ steps.meta.outputs.labels }} - annotations: ${{ steps.meta.outputs.annotations }} - cache-from: type=gha - cache-to: type=gha,mode=max - provenance: true - sbom: true +# name: Build and Push Docker Image + +# on: +# push: +# branches: [ "main", "master" ] +# tags: [ "v*.*.*" ] +# pull_request: +# branches: [ "main", "master" ] + +# env: +# REGISTRY_IMAGE: anoddname/streamrip-web-gui + +# jobs: +# build: +# runs-on: ubuntu-latest +# permissions: +# contents: read +# packages: write +# id-token: write + +# steps: +# - name: Checkout +# uses: actions/checkout@v4 + +# - name: Extract metadata +# id: meta +# uses: docker/metadata-action@v5 +# with: +# images: ${{ env.REGISTRY_IMAGE }} +# tags: | +# type=ref,event=branch +# type=ref,event=pr +# type=semver,pattern={{version}} +# type=semver,pattern={{major}}.{{minor}} +# type=raw,value=latest,enable={{is_default_branch}} + +# - name: Set up QEMU +# uses: docker/setup-qemu-action@v3 + +# - name: Set up Docker Buildx +# uses: docker/setup-buildx-action@v3 + +# - name: Log in to Docker Hub +# if: github.event_name != 'pull_request' +# uses: docker/login-action@v3 +# with: +# username: ${{ secrets.DOCKERHUB_USERNAME }} +# password: ${{ secrets.DOCKERHUB_TOKEN }} + +# - name: Build and push Docker image +# uses: docker/build-push-action@v6 +# with: +# context: . +# platforms: linux/amd64,linux/arm64 +# push: ${{ github.event_name != 'pull_request' }} +# tags: ${{ steps.meta.outputs.tags }} +# labels: ${{ steps.meta.outputs.labels }} +# annotations: ${{ steps.meta.outputs.annotations }} +# cache-from: type=gha +# cache-to: type=gha,mode=max +# provenance: true +# sbom: true From 169bf9c088542db0ba30b25837e7bb3674769fe8 Mon Sep 17 00:00:00 2001 From: QuantumFF Date: Thu, 4 Jun 2026 02:26:54 +0800 Subject: [PATCH 9/9] fix: Library tree handles streamrip's real on-disk layout and tag semantics Root cause (two false assumptions, both contradicted by real rips): 1. list_library_albums assumed an Artist/Album directory nesting; streamrip's default folder_format is FLAT, with multi-disc audio in 'Disc N' subfolders. Disc folders were listed as separate albums grouped under the album name as if it were an artist, and every flat album fell under 'Unknown Artist'. 2. assess_completeness assumed tracktotal was per-disc; streamrip tags it ALBUM-wide while tracknumber restarts per disc, so each disc of a multi-disc album reported phantom missing tracks. Now: disc folders fold into their parent album; root-level albums render ungrouped (artist=null); tracks are collected across disc subfolders; the assessment cache mtime includes disc subfolders; completeness follows the real tag semantics (interior gaps locatable per disc, trailing multi-disc gaps counted exactly but reported as unlocated, wholly missing discs detected via disctotal). ADR-0003 and CONTEXT.md amended with the verified semantics. Also in this commit (inseparable from the fix hunks): - app.py reformatted wholesale by the on-save formatter (black/isort style); behavior-neutral, full suite green (68 passed) - style.css: history-card redownload button wraps on mobile viewports Co-Authored-By: Claude Opus 4.8 (1M context) --- CONTEXT.md | 10 +- app.py | 1551 ++++++++++------- .../0003-completeness-from-embedded-tags.md | 12 + static/css/style.css | 13 + static/js/app.js | 48 +- tests/test_completeness.py | 24 + tests/test_library_api.py | 148 +- 7 files changed, 1120 insertions(+), 686 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 06684b8..bc47c0c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -58,8 +58,12 @@ embedded tags (`tracktotal`, `disctotal`, `tracknumber`, `discnumber`) of the tracks that *are* present, which streamrip writes into every file. An album is: - **Complete** — every expected (disc, track) is present on disk. -- **Incomplete** — at least one expected track is absent. Each absent track is a - **Missing track**, identified by its number (its title is unknown, because a - track that was never downloaded left no tags on disk). +- **Incomplete** — at least one expected track is absent. An absent track whose + position is determinable is a **Missing track**, identified by its number (its + title is unknown, because a track that was never downloaded left no tags on + disk). On a multi-disc album the tags carry only the album-wide total, so a + trailing absence there is counted exactly but its disc/position is unknown + (an **unlocated** absence); a disc with no tracks on disk at all is a + **Missing disc**. - **Unknown** — completeness cannot be determined, because no present track has readable tags to reveal the expected total. diff --git a/app.py b/app.py index 145210e..ceab462 100644 --- a/app.py +++ b/app.py @@ -1,31 +1,40 @@ +import json import logging -from flask import Flask, render_template, request, jsonify, Response, stream_with_context -import subprocess import os -import threading import queue -import time +import re +import shutil +import subprocess import tempfile +import threading +import time + import requests -import shutil -import re -import json +from flask import ( + Flask, + Response, + jsonify, + render_template, + request, + stream_with_context, +) -#new logging config +# new logging config logging.basicConfig( - level=logging.DEBUG, - format='%(asctime)s - %(name)s - %(levelname)s - %(message)s' + level=logging.DEBUG, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" ) logger = logging.getLogger(__name__) app = Flask(__name__) -#Defaults work for a local run; Docker overrides these via docker-compose environment -STREAMRIP_CONFIG = os.environ.get('STREAMRIP_CONFIG', os.path.expanduser('~/.config/streamrip/config.toml')) -DOWNLOAD_DIR = os.environ.get('DOWNLOAD_DIR', os.path.expanduser('~/Music')) -MAX_CONCURRENT_DOWNLOADS = int(os.environ.get('MAX_CONCURRENT_DOWNLOADS', '2')) +# Defaults work for a local run; Docker overrides these via docker-compose environment +STREAMRIP_CONFIG = os.environ.get( + "STREAMRIP_CONFIG", os.path.expanduser("~/.config/streamrip/config.toml") +) +DOWNLOAD_DIR = os.environ.get("DOWNLOAD_DIR", os.path.expanduser("~/Downloads")) +MAX_CONCURRENT_DOWNLOADS = int(os.environ.get("MAX_CONCURRENT_DOWNLOADS", "2")) -#Fail loudly at startup if the download dir is unusable, instead of silently per-download +# Fail loudly at startup if the download dir is unusable, instead of silently per-download try: os.makedirs(DOWNLOAD_DIR, exist_ok=True) if not os.access(DOWNLOAD_DIR, os.W_OK): @@ -33,12 +42,14 @@ except OSError as e: logger.error("=" * 60) logger.error(f"DOWNLOAD_DIR '{DOWNLOAD_DIR}' is not usable: {e}") - logger.error("All downloads WILL FAIL. Set the DOWNLOAD_DIR environment variable to a writable directory.") + logger.error( + "All downloads WILL FAIL. Set the DOWNLOAD_DIR environment variable to a writable directory." + ) logger.error("=" * 60) download_queue = queue.Queue() -#Server is the source of truth (ADR-0002): Active + History live here in memory. -#active_downloads maps task_id -> Download record (status: queued | downloading). +# Server is the source of truth (ADR-0002): Active + History live here in memory. +# active_downloads maps task_id -> Download record (status: queued | downloading). active_downloads = {} active_lock = threading.Lock() download_history = [] @@ -48,14 +59,14 @@ cache_lock = threading.Lock() MAX_HISTORY = 50 -#The exact line streamrip logs once per track it refuses to re-download because the -#track is recorded in its own database. Skip detection keys on this line. -SKIP_LINE_RE = re.compile(r'Marked as downloaded in the database') +# The exact line streamrip logs once per track it refuses to re-download because the +# track is recorded in its own database. Skip detection keys on this line. +SKIP_LINE_RE = re.compile(r"Marked as downloaded in the database") -#The audio file extensions streamrip writes. Used to tell tracks apart from the -#non-audio files (cover art, logs) that share an album folder, so the Library -#never lists cover.jpg as a track. -AUDIO_EXTENSIONS = ('.mp3', '.flac', '.m4a', '.opus', '.ogg', '.wav', '.aac', '.alac') +# The audio file extensions streamrip writes. Used to tell tracks apart from the +# non-audio files (cover art, logs) that share an album folder, so the Library +# never lists cover.jpg as a track. +AUDIO_EXTENSIONS = (".mp3", ".flac", ".m4a", ".opus", ".ogg", ".wav", ".aac", ".alac") def _default_tag_reader(filepath): @@ -79,17 +90,17 @@ def first(key): return value return { - 'title': first('title'), - 'tracknumber': first('tracknumber'), - 'discnumber': first('discnumber'), - 'tracktotal': first('tracktotal'), - 'disctotal': first('disctotal'), + "title": first("title"), + "tracknumber": first("tracknumber"), + "discnumber": first("discnumber"), + "tracktotal": first("tracktotal"), + "disctotal": first("disctotal"), } -#Injectable tag-reading boundary (mirrors run_rip). Tests swap this for a fake -#that returns canned tags so the Library suite needs neither mutagen nor real -#audio files; production reads tags off disk with mutagen. +# Injectable tag-reading boundary (mirrors run_rip). Tests swap this for a fake +# that returns canned tags so the Library suite needs neither mutagen nor real +# audio files; production reads tags off disk with mutagen. read_audio_tags = _default_tag_reader @@ -102,7 +113,7 @@ def _parse_track_number(raw): "7" or "7/12"; we take the leading integer and ignore anything unparseable.""" if raw is None: return None - match = re.match(r'\s*(\d+)', str(raw)) + match = re.match(r"\s*(\d+)", str(raw)) return int(match.group(1)) if match else None @@ -116,98 +127,158 @@ def assess_completeness(present_tracks): { 'status': 'complete' | 'incomplete' | 'unknown', - 'missing': [{'disc': D, 'track': N}, ...], # sorted, [] unless incomplete - 'discs': [ - {'disc': D, 'tracktotal': T, 'present': [...], 'missing': [...]}, - ... - ], + 'missing': [{'disc': D, 'track': N}, ...], # locatable gaps, sorted + 'missing_discs': [D, ...], # discs with no tracks at all + 'unlocated': N, # absent tracks whose disc/number cannot be determined + 'discs': [{'disc': D, 'present': [...], 'missing': [...]}, ...], } - Completeness logic: any present track reveals its disc's true ``tracktotal``, - so every gap — interior and trailing — is detectable. Each disc is assessed - independently and missing tracks are reported against their own disc. An - album with no present track carrying a readable ``tracktotal`` is **Unknown** - (never guessed); a track with no readable disc is treated as disc 1.""" + Tag semantics (verified against streamrip's tagger and real rips): + ``tracktotal`` is the ALBUM-wide track count while ``tracknumber`` restarts + per disc, and ``disctotal`` reveals how many discs the album has. So: + + - Single-disc album: every gap — interior and trailing — is locatable + (expected sequence is 1…tracktotal). + - Multi-disc album: interior gaps are locatable per disc from the numbering; + the exact number of absent tracks is album total minus present count; but a + trailing gap cannot be attributed to a specific disc — those are counted in + ``unlocated``, never guessed. A disc with zero present tracks is a wholly + Missing disc (reported in ``missing_discs``; its tracks count toward + ``unlocated``). + + An album with no present track carrying a readable ``tracktotal`` is + **Unknown** (never guessed); a track with no readable disc is disc 1.""" discs = {} saw_total = False + album_total = 0 + disctotal_seen = 0 for raw in present_tracks: - disc = _parse_track_number(raw.get('disc')) - track = _parse_track_number(raw.get('track')) - tracktotal = _parse_track_number(raw.get('tracktotal')) + disc = _parse_track_number(raw.get("disc")) + track = _parse_track_number(raw.get("track")) + tracktotal = _parse_track_number(raw.get("tracktotal")) + disctotal = _parse_track_number(raw.get("disctotal")) - #A track with no readable disc tag belongs to disc 1 (the common - #single-disc case where streamrip omits/zeroes the disc number). + # A track with no readable disc tag belongs to disc 1 (the common + # single-disc case where streamrip omits/zeroes the disc number). if disc is None or disc < 1: disc = 1 - - entry = discs.setdefault(disc, {'present': set(), 'tracktotal': None}) - if track is not None and track >= 1: - entry['present'].add(track) + if disctotal is not None and disctotal > disctotal_seen: + disctotal_seen = disctotal if tracktotal is not None and tracktotal >= 1: saw_total = True - #Keep the largest total seen on this disc; a present track number - #beyond it would itself imply a larger album, handled below. - if entry['tracktotal'] is None or tracktotal > entry['tracktotal']: - entry['tracktotal'] = tracktotal + album_total = max(album_total, tracktotal) + + present = discs.setdefault(disc, set()) + if track is not None and track >= 1: + present.add(track) if not saw_total: - return {'status': 'unknown', 'missing': [], 'discs': []} + return { + "status": "unknown", + "missing": [], + "missing_discs": [], + "unlocated": 0, + "discs": [], + } + + disctotal_seen = max(disctotal_seen, max(discs, default=1), 1) + missing_discs = [d for d in range(1, disctotal_seen + 1) if d not in discs] + present_count = sum(len(p) for p in discs.values()) + multi_disc = disctotal_seen > 1 disc_reports = [] - all_missing = [] + located = [] for disc in sorted(discs): - entry = discs[disc] - present = sorted(entry['present']) - total = entry['tracktotal'] - #If a present track number exceeds the tagged total (or no total on this - #disc but present numbers exist), trust the highest present number so we - #never report a present track as missing. - highest_present = present[-1] if present else 0 - if total is None or highest_present > total: - total = highest_present - missing = [n for n in range(1, total + 1) if n not in entry['present']] - disc_reports.append({ - 'disc': disc, - 'tracktotal': total, - 'present': present, - 'missing': missing, - }) - all_missing.extend({'disc': disc, 'track': n} for n in missing) - - all_missing.sort(key=lambda m: (m['disc'], m['track'])) - status = 'complete' if not all_missing else 'incomplete' - return {'status': status, 'missing': all_missing, 'discs': disc_reports} + present = sorted(discs[disc]) + highest = present[-1] if present else 0 + if multi_disc: + # Per-disc totals are unknowable (tracktotal is album-wide), so only + # interior gaps — numbers below the highest present — are locatable. + expected_here = highest + else: + # Single disc: tracktotal IS this disc's total, so trailing gaps are + # locatable too. Never report a present track as missing when its + # number exceeds the tagged total. + expected_here = max(album_total, highest) + missing_here = [n for n in range(1, expected_here + 1) if n not in discs[disc]] + disc_reports.append({"disc": disc, "present": present, "missing": missing_here}) + located.extend({"disc": disc, "track": n} for n in missing_here) + + located.sort(key=lambda m: (m["disc"], m["track"])) + # The album-wide total makes the number of absent tracks exact even when + # their positions are not; whatever the locatable gaps do not account for is + # unlocated (trailing tracks on some disc, or the contents of a missing disc). + shortfall = max(0, album_total - present_count) + unlocated = max(0, shortfall - len(located)) + + complete = not located and not missing_discs and not unlocated + return { + "status": "complete" if complete else "incomplete", + "missing": located, + "missing_discs": missing_discs, + "unlocated": unlocated, + "discs": disc_reports, + } + + +DISC_FOLDER_RE = re.compile(r"^(disc|disk|cd)[ _-]*\d+$", re.IGNORECASE) + + +def _is_disc_folder(name): + """streamrip puts a multi-disc album's audio in "Disc N" subfolders + (streamrip media/track.py); such a folder is part of its parent album, never + an album of its own.""" + return bool(DISC_FOLDER_RE.match(name.strip())) def list_library_albums(download_dir): """List the album folders in the Library (the albums on disk, ADR/Library glossary) without reading a single tag, so it stays instant on large - libraries. An album folder is any directory that directly contains at least - one audio file; its parent directory name is taken as the Artist. + libraries. + + streamrip's default folder_format puts album folders FLAT at the download + root ("Artist - Album (Year) [...]"), with multi-disc audio in "Disc N" + subfolders. So: an album folder is a directory that directly contains audio + OR whose "Disc N" subfolders do; a disc folder folds into its parent album. + The Artist is the album folder's parent directory when one exists (nested + Artist/Album folder_format), otherwise None — the frontend renders + root-level albums ungrouped, since their folder name already carries the + artist. Returns a list of {artist, album, path} sorted by artist then album, where ``path`` is the album folder relative to ``download_dir`` (the handle the per-album endpoint expands).""" - albums = [] + albums = {} if not os.path.isdir(download_dir): - return albums + return [] for root, dirs, filenames in os.walk(download_dir): dirs.sort() - if any(_is_audio_file(name) for name in filenames): - rel_path = os.path.relpath(root, download_dir) - album_name = os.path.basename(root) - parent = os.path.dirname(rel_path) - artist = os.path.basename(parent) if parent and parent != '.' else 'Unknown Artist' - albums.append({ - 'artist': artist, - 'album': album_name, - 'path': rel_path, - }) + if not any(_is_audio_file(name) for name in filenames): + continue + + album_dir = root + # Audio inside a "Disc N" folder belongs to the parent album folder. + if _is_disc_folder(os.path.basename(album_dir)): + album_dir = os.path.dirname(album_dir) + if os.path.realpath(album_dir) == os.path.realpath(download_dir): + album_dir = root # a bare "Disc 1" at the root: treat as album + + rel_path = os.path.relpath(album_dir, download_dir) + if rel_path in albums: + continue + parent = os.path.dirname(rel_path) + albums[rel_path] = { + "artist": os.path.basename(parent) if parent and parent != "." else None, + "album": os.path.basename(album_dir), + "path": rel_path, + } - albums.sort(key=lambda a: (a['artist'].lower(), a['album'].lower())) - return albums + return sorted( + albums.values(), + key=lambda a: ((a["artist"] or "").lower(), a["album"].lower()), + ) def read_album_tracks(download_dir, rel_path): @@ -224,54 +295,75 @@ def read_album_tracks(download_dir, rel_path): if not os.path.isdir(album_dir): return tracks - for filename in os.listdir(album_dir): - if not _is_audio_file(filename): - continue + # The album's audio sits directly in the folder, plus inside any "Disc N" + # subfolders for multi-disc albums (streamrip's layout). Track filenames keep + # their disc subfolder prefix so they stay unique within the album. + audio_files = [] + for entry in os.listdir(album_dir): + entry_path = os.path.join(album_dir, entry) + if os.path.isfile(entry_path) and _is_audio_file(entry): + audio_files.append(entry) + elif os.path.isdir(entry_path) and _is_disc_folder(entry): + for sub in os.listdir(entry_path): + if _is_audio_file(sub) and os.path.isfile(os.path.join(entry_path, sub)): + audio_files.append(os.path.join(entry, sub)) + + for filename in audio_files: filepath = os.path.join(album_dir, filename) - if not os.path.isfile(filepath): - continue try: tags = read_audio_tags(filepath) or {} except Exception as e: logger.warning(f"Failed to read tags from {filepath}: {e}") tags = {} - tracknumber = _parse_track_number(tags.get('tracknumber')) - discnumber = _parse_track_number(tags.get('discnumber')) - title = tags.get('title') or filename - tracks.append({ - 'tracknumber': tracknumber, - 'discnumber': discnumber, - 'title': title, - 'filename': filename, - #Carry the raw totals through so completeness can be assessed from - #the same tag read (ADR-0003) without a second pass over the files. - 'tracktotal': _parse_track_number(tags.get('tracktotal')), - 'disctotal': _parse_track_number(tags.get('disctotal')), - }) - - tracks.sort(key=lambda t: ( - t['discnumber'] if t['discnumber'] is not None else 0, - t['tracknumber'] if t['tracknumber'] is not None else 0, - t['filename'].lower(), - )) + tracknumber = _parse_track_number(tags.get("tracknumber")) + discnumber = _parse_track_number(tags.get("discnumber")) + title = tags.get("title") or filename + tracks.append( + { + "tracknumber": tracknumber, + "discnumber": discnumber, + "title": title, + "filename": filename, + # Carry the raw totals through so completeness can be assessed from + # the same tag read (ADR-0003) without a second pass over the files. + "tracktotal": _parse_track_number(tags.get("tracktotal")), + "disctotal": _parse_track_number(tags.get("disctotal")), + } + ) + + tracks.sort( + key=lambda t: ( + t["discnumber"] if t["discnumber"] is not None else 0, + t["tracknumber"] if t["tracknumber"] is not None else 0, + t["filename"].lower(), + ) + ) return tracks -#Per-album completeness assessment cache, keyed on the album folder path and its -#mtime (ADR-0003: completeness comes from the tags on disk). Re-expanding an -#unchanged album returns the cached payload without re-reading any tags; -#downloading/deleting a track changes the folder mtime and invalidates it. +# Per-album completeness assessment cache, keyed on the album folder path and its +# mtime (ADR-0003: completeness comes from the tags on disk). Re-expanding an +# unchanged album returns the cached payload without re-reading any tags; +# downloading/deleting a track changes the folder mtime and invalidates it. album_assessment_cache = {} album_cache_lock = threading.Lock() def _album_folder_mtime(album_dir): - """The album folder's own mtime — bumped by the filesystem whenever a track - file is added to or removed from the directory, which is exactly the events - that can change completeness. Returns None when the folder is missing.""" + """The newest mtime across the album folder and its "Disc N" subfolders — + bumped by the filesystem whenever a track file is added to or removed from + any of them, which is exactly the events that can change completeness. + (A deletion inside "Disc 2" does not touch the parent folder's mtime, so the + subfolders must participate in the cache key.) Returns None when the folder + is missing.""" try: - return os.stat(album_dir).st_mtime + mtime = os.stat(album_dir).st_mtime + for entry in os.listdir(album_dir): + entry_path = os.path.join(album_dir, entry) + if _is_disc_folder(entry) and os.path.isdir(entry_path): + mtime = max(mtime, os.stat(entry_path).st_mtime) + return mtime except OSError: return None @@ -282,26 +374,67 @@ def _build_track_rows(present_tracks, assessment): tracks shown in sequence position (interior and trailing). A missing track is shown by number only — it left no title on disk (ADR-0003).""" rows = list(present_tracks) - for miss in assessment.get('missing', []): - rows.append({ - 'tracknumber': miss['track'], - 'discnumber': miss['disc'], - 'title': None, - 'filename': None, - 'tracktotal': None, - 'disctotal': None, - 'missing': True, - }) - #Sort present and missing rows into one expected sequence. A present track - #with no disc tag belongs to disc 1, exactly as assess_completeness assigns - #its gap rows, so the two interleave correctly (no disc tag -> disc 1). - rows.sort(key=lambda t: ( - t['discnumber'] if t['discnumber'] is not None and t['discnumber'] >= 1 else 1, - t['tracknumber'] if t['tracknumber'] is not None else 0, - (t.get('filename') or '').lower(), - )) + for miss in assessment.get("missing", []): + rows.append( + { + "tracknumber": miss["track"], + "discnumber": miss["disc"], + "title": None, + "filename": None, + "tracktotal": None, + "disctotal": None, + "missing": True, + } + ) + # A wholly Missing disc renders as a single gap row for the disc — its track + # count is unknown, so there are no per-track rows to merge. + for disc in assessment.get("missing_discs", []): + rows.append( + { + "tracknumber": None, + "discnumber": disc, + "title": None, + "filename": None, + "tracktotal": None, + "disctotal": None, + "missing": True, + "missing_disc": True, + } + ) + # Absent tracks whose position is unknowable (trailing gaps on a multi-disc + # album: tracktotal is album-wide, so they cannot be pinned to a disc) render + # as one summary gap row at the end of the sequence. + if assessment.get("unlocated"): + rows.append( + { + "tracknumber": None, + "discnumber": None, + "title": None, + "filename": None, + "tracktotal": None, + "disctotal": None, + "missing": True, + "unlocated_count": assessment["unlocated"], + } + ) + # Sort present and missing rows into one expected sequence. A present track + # with no disc tag belongs to disc 1, exactly as assess_completeness assigns + # its gap rows, so the two interleave correctly (no disc tag -> disc 1). The + # unlocated summary row belongs to no disc and sorts after everything. + def _row_key(t): + if t.get("unlocated_count"): + return (float("inf"), 0, "") + return ( + t["discnumber"] + if t["discnumber"] is not None and t["discnumber"] >= 1 + else 1, + t["tracknumber"] if t["tracknumber"] is not None else 0, + (t.get("filename") or "").lower(), + ) + + rows.sort(key=_row_key) for row in rows: - row.setdefault('missing', False) + row.setdefault("missing", False) return rows @@ -318,47 +451,55 @@ def get_album_assessment(download_dir, rel_path): cache_key = os.path.realpath(album_dir) with album_cache_lock: cached = album_assessment_cache.get(cache_key) - if cached is not None and cached['mtime'] == mtime and mtime is not None: - return cached['payload'] + if cached is not None and cached["mtime"] == mtime and mtime is not None: + return cached["payload"] present_tracks = read_album_tracks(download_dir, rel_path) - assessment = assess_completeness([ - { - 'disc': t['discnumber'], - 'track': t['tracknumber'], - 'tracktotal': t['tracktotal'], - 'disctotal': t['disctotal'], - } - for t in present_tracks - ]) + assessment = assess_completeness( + [ + { + "disc": t["discnumber"], + "track": t["tracknumber"], + "tracktotal": t["tracktotal"], + "disctotal": t["disctotal"], + } + for t in present_tracks + ] + ) payload = { - 'tracks': _build_track_rows(present_tracks, assessment), - 'completeness': { - 'status': assessment['status'], - 'missing_count': len(assessment['missing']), - 'missing': assessment['missing'], + "tracks": _build_track_rows(present_tracks, assessment), + "completeness": { + "status": assessment["status"], + # The album-wide tracktotal makes the absent-track count exact even + # when some positions are unlocatable (multi-disc trailing gaps). + "missing_count": len(assessment["missing"]) + assessment["unlocated"], + "missing": assessment["missing"], + "missing_discs": assessment["missing_discs"], + "unlocated": assessment["unlocated"], }, } if mtime is not None: with album_cache_lock: - album_assessment_cache[cache_key] = {'mtime': mtime, 'payload': payload} + album_assessment_cache[cache_key] = {"mtime": mtime, "payload": payload} return payload -def build_rip_command(url, quality, *, config_path=None, download_dir=None, no_db=False): +def build_rip_command( + url, quality, *, config_path=None, download_dir=None, no_db=False +): """Construct the `rip url` argv. Pure (no side effects) so it can be tested directly and reused for Redownload (no_db -> --no-db). This is the single place the download invocation is assembled.""" - cmd = ['rip'] + cmd = ["rip"] if config_path and os.path.exists(config_path): - cmd.extend(['--config-path', config_path]) + cmd.extend(["--config-path", config_path]) if no_db: - cmd.append('--no-db') + cmd.append("--no-db") if download_dir: - cmd.extend(['-f', download_dir]) - cmd.extend(['-q', str(quality)]) - cmd.extend(['url', url]) + cmd.extend(["-f", download_dir]) + cmd.extend(["-q", str(quality)]) + cmd.extend(["url", url]) return cmd @@ -366,13 +507,13 @@ def classify_download(returncode, output): """Map a finished `rip` run to a terminal Download state from its exit code and stdout. Pure, so the worker's terminal-state logic is unit-testable.""" if returncode != 0: - return 'failed' - #Skipped: rip did no downloading work because every track was already - #recorded in its database. Keyed on the skip log line; only a skip when - #nothing was actually downloaded. - if SKIP_LINE_RE.search(output) and '─ Downloading' not in output: - return 'skipped' - return 'completed' + return "failed" + # Skipped: rip did no downloading work because every track was already + # recorded in its database. Keyed on the skip log line; only a skip when + # nothing was actually downloaded. + if SKIP_LINE_RE.search(output) and "─ Downloading" not in output: + return "skipped" + return "completed" def _default_runner(cmd): @@ -384,8 +525,8 @@ def _default_runner(cmd): stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, - encoding='utf-8', - errors='replace', + encoding="utf-8", + errors="replace", bufsize=1, ) try: @@ -400,8 +541,8 @@ def _default_runner(cmd): return process.returncode -#Injectable subprocess boundary. Tests swap this for a fake generator that emits -#canned output and a return code; production uses the real `rip` runner. +# Injectable subprocess boundary. Tests swap this for a fake generator that emits +# canned output and a return code; production uses the real `rip` runner. run_rip = _default_runner @@ -410,23 +551,25 @@ def register_queued(task): announce it immediately, before any worker is free. Guarantees a submitted Download is visible the instant the server accepts it.""" record = { - 'id': task['id'], - 'url': task['url'], - 'quality': task.get('quality', 3), - 'metadata': task.get('metadata', {}), - 'status': 'queued', - 'queued_at': time.time(), + "id": task["id"], + "url": task["url"], + "quality": task.get("quality", 3), + "metadata": task.get("metadata", {}), + "status": "queued", + "queued_at": time.time(), } with active_lock: - active_downloads[task['id']] = record - broadcast_sse({ - 'type': 'download_queued', - 'id': record['id'], - 'url': record['url'], - 'quality': record['quality'], - 'metadata': record['metadata'], - 'status': 'queued', - }) + active_downloads[task["id"]] = record + broadcast_sse( + { + "type": "download_queued", + "id": record["id"], + "url": record["url"], + "quality": record["quality"], + "metadata": record["metadata"], + "status": "queued", + } + ) def enqueue_download(url, quality=3, metadata=None, no_db=False): @@ -438,11 +581,11 @@ def enqueue_download(url, quality=3, metadata=None, no_db=False): again (see the Redownload glossary entry).""" task_id = f"dl_{int(time.time() * 1000)}_{len(active_downloads)}" task = { - 'id': task_id, - 'url': url, - 'quality': quality, - 'metadata': metadata or {}, - 'no_db': no_db, + "id": task_id, + "url": url, + "quality": quality, + "metadata": metadata or {}, + "no_db": no_db, } register_queued(task) download_queue.put(task) @@ -460,31 +603,34 @@ def run(self): if task is None: break - task_id = task['id'] - url = task['url'] - quality = task.get('quality', 3) - metadata = task.get('metadata', {}) - no_db = task.get('no_db', False) + task_id = task["id"] + url = task["url"] + quality = task.get("quality", 3) + metadata = task.get("metadata", {}) + no_db = task.get("no_db", False) - #Transition the existing Queued card in place to Downloading. + # Transition the existing Queued card in place to Downloading. with active_lock: record = active_downloads.get(task_id) if record is not None: - record['status'] = 'downloading' - record['started'] = time.time() - - broadcast_sse({ - 'type': 'download_started', - 'id': task_id, - 'url': url, - 'quality': quality, - 'metadata': metadata, - 'status': 'downloading' - }) + record["status"] = "downloading" + record["started"] = time.time() + + broadcast_sse( + { + "type": "download_started", + "id": task_id, + "url": url, + "quality": quality, + "metadata": metadata, + "status": "downloading", + } + ) output_lines = [] cmd = build_rip_command( - url, quality, + url, + quality, config_path=STREAMRIP_CONFIG, download_dir=DOWNLOAD_DIR, no_db=no_db, @@ -499,30 +645,40 @@ def run(self): if line: output_lines.append(line) if len(output_lines) % 10 == 0: - broadcast_sse({ - 'type': 'download_progress', - 'id': task_id, - 'output': "\n".join(output_lines[-5:]), - 'progress': {'raw_output': True} - }) + broadcast_sse( + { + "type": "download_progress", + "id": task_id, + "output": "\n".join(output_lines[-5:]), + "progress": {"raw_output": True}, + } + ) except StopIteration as stop: returncode = stop.value if stop.value is not None else 0 full_output = "\n".join(output_lines) status = classify_download(returncode, full_output) - if status == 'failed': - logger.error(f"Download failed (exit code {returncode}): {' '.join(cmd)}") - logger.error("rip output (last 30 lines):\n%s", "\n".join(output_lines[-30:])) - elif status == 'skipped': - logger.info(f"Already downloaded (marked in streamrip database): {url}") + if status == "failed": + logger.error( + f"Download failed (exit code {returncode}): {' '.join(cmd)}" + ) + logger.error( + "rip output (last 30 lines):\n%s", "\n".join(output_lines[-30:]) + ) + elif status == "skipped": + logger.info( + f"Already downloaded (marked in streamrip database): {url}" + ) finalize_download(task_id, status, metadata, full_output) except Exception as e: logger.exception(f"Download worker error for {url}") finalize_download( - task_id, 'failed', metadata, + task_id, + "failed", + metadata, "\n".join(output_lines) if output_lines else str(e), error=str(e), ) @@ -545,156 +701,169 @@ def finalize_download(task_id, status, metadata, output, error=None): record = active_downloads.pop(task_id, None) entry = { - 'id': task_id, - 'url': record.get('url') if record else None, - 'quality': record.get('quality') if record else None, - 'metadata': metadata or (record.get('metadata') if record else {}), - 'status': status, - 'output': output, - 'completed_at': time.time(), + "id": task_id, + "url": record.get("url") if record else None, + "quality": record.get("quality") if record else None, + "metadata": metadata or (record.get("metadata") if record else {}), + "status": status, + "output": output, + "completed_at": time.time(), } if error: - entry['error'] = error + entry["error"] = error with history_lock: download_history.insert(0, entry) del download_history[MAX_HISTORY:] - broadcast_sse({ - 'type': 'download_completed', - 'id': task_id, - 'url': entry['url'], - 'quality': entry['quality'], - 'status': status, - 'metadata': entry['metadata'], - 'output': output, - **({'error': error} if error else {}), - }) + broadcast_sse( + { + "type": "download_completed", + "id": task_id, + "url": entry["url"], + "quality": entry["quality"], + "status": status, + "metadata": entry["metadata"], + "output": output, + **({"error": error} if error else {}), + } + ) + def broadcast_sse(data): message = f"data: {json.dumps(data)}\n\n" dead_clients = [] - + for client in sse_clients: try: client.put(message) except: dead_clients.append(client) - + for client in dead_clients: sse_clients.remove(client) -@app.route('/api/events') + +@app.route("/api/events") def sse_events(): def generate(): q = queue.Queue() sse_clients.append(q) - + try: yield f"data: {json.dumps({'type': 'connected'})}\n\n" - + while True: try: msg = q.get(timeout=30) yield msg except queue.Empty: - continue #previous heartbeat check + continue # previous heartbeat check finally: sse_clients.remove(q) - + return Response( stream_with_context(generate()), - mimetype='text/event-stream', + mimetype="text/event-stream", headers={ - 'Cache-Control': 'no-cache', - 'X-Accel-Buffering': 'no' #disable nginx buffering - } + "Cache-Control": "no-cache", + "X-Accel-Buffering": "no", # disable nginx buffering + }, ) + workers = [] for _ in range(MAX_CONCURRENT_DOWNLOADS): worker = DownloadWorker() worker.start() workers.append(worker) -@app.route('/') + +@app.route("/") def index(): - return render_template('index.html') + return render_template("index.html") -@app.route('/sw.js') + +@app.route("/sw.js") def service_worker(): - #Service worker must be served from the root so its scope covers the whole app - return app.send_static_file('sw.js') + # Service worker must be served from the root so its scope covers the whole app + return app.send_static_file("sw.js") + -@app.route('/api/download', methods=['POST']) +@app.route("/api/download", methods=["POST"]) def start_download(): data = request.json - url = data.get('url') - quality = data.get('quality', 3) - + url = data.get("url") + quality = data.get("quality", 3) + if not url: - return jsonify({'error': 'URL is required'}), 400 - - #Validate URL (basic check) - #youtube-dl for later - valid_services = ['spotify.com', 'deezer.com', 'tidal.com', 'qobuz.com', 'soundcloud.com', 'youtube.com'] + return jsonify({"error": "URL is required"}), 400 + + # Validate URL (basic check) + # youtube-dl for later + valid_services = [ + "spotify.com", + "deezer.com", + "tidal.com", + "qobuz.com", + "soundcloud.com", + "youtube.com", + ] if not any(service in url.lower() for service in valid_services): - return jsonify({'error': 'Unsupported service URL'}), 400 - + return jsonify({"error": "Unsupported service URL"}), 400 + metadata = extract_metadata_from_url(url) task_id = enqueue_download(url, quality, metadata) - return jsonify({'task_id': task_id, 'status': 'queued'}) + return jsonify({"task_id": task_id, "status": "queued"}) -@app.route('/api/status') +@app.route("/api/status") def get_all_status(): - #Server is the source of truth (ADR-0002). Active is returned as a list so the - #frontend can rehydrate the unified Active list (Queued + Downloading) on load. + # Server is the source of truth (ADR-0002). Active is returned as a list so the + # frontend can rehydrate the unified Active list (Queued + Downloading) on load. with active_lock: active = list(active_downloads.values()) with history_lock: history = list(download_history) - return jsonify({ - 'active': active, - 'history': history, - 'queue_size': download_queue.qsize() - }) - - -@app.route('/api/config', methods=['GET', 'POST']) + return jsonify( + {"active": active, "history": history, "queue_size": download_queue.qsize()} + ) + + +@app.route("/api/config", methods=["GET", "POST"]) def config(): - if request.method == 'GET': + if request.method == "GET": if os.path.exists(STREAMRIP_CONFIG): - with open(STREAMRIP_CONFIG, 'r') as f: - return jsonify({'config': f.read()}) - return jsonify({'config': ''}) - - elif request.method == 'POST': + with open(STREAMRIP_CONFIG, "r") as f: + return jsonify({"config": f.read()}) + return jsonify({"config": ""}) + + elif request.method == "POST": data = request.json - config_content = data.get('config', '') - + config_content = data.get("config", "") + try: if os.path.exists(STREAMRIP_CONFIG): shutil.copy2(STREAMRIP_CONFIG, f"{STREAMRIP_CONFIG}.bak") - + os.makedirs(os.path.dirname(STREAMRIP_CONFIG), exist_ok=True) - with open(STREAMRIP_CONFIG, 'w') as f: + with open(STREAMRIP_CONFIG, "w") as f: f.write(config_content) - - return jsonify({'status': 'success'}) + + return jsonify({"status": "success"}) except Exception as e: - return jsonify({'error': str(e)}), 500 - + return jsonify({"error": str(e)}), 500 -@app.route('/api/search', methods=['POST']) + +@app.route("/api/search", methods=["POST"]) def search_music(): data = request.json - query = data.get('query') - search_type = data.get('type', 'album') - source = data.get('source', 'qobuz') - + query = data.get("query") + search_type = data.get("type", "album") + source = data.get("source", "qobuz") + # new logging logger.info("=" * 60) logger.info("SEARCH REQUEST RECEIVED") @@ -702,141 +871,170 @@ def search_music(): logger.info(f"Type: {search_type}") logger.info(f"Source: {source}") logger.info("=" * 60) - + if not query: logger.warning("No query provided") - return jsonify({'error': 'Query required'}), 400 - - if source == 'soundcloud' and search_type in ['album', 'artist']: + return jsonify({"error": "Query required"}), 400 + + if source == "soundcloud" and search_type in ["album", "artist"]: logger.debug(f"SoundCloud doesn't support {search_type} search") - return jsonify({ - 'results': [], - 'query': query, - 'source': source, - 'total_count': 0, - 'message': f'SoundCloud does not support {search_type} searches. Try searching for tracks or playlists instead.' - }) - + return jsonify( + { + "results": [], + "query": query, + "source": source, + "total_count": 0, + "message": f"SoundCloud does not support {search_type} searches. Try searching for tracks or playlists instead.", + } + ) + try: - with tempfile.NamedTemporaryFile(mode='w+', suffix='.txt', delete=False) as tmp_file: + with tempfile.NamedTemporaryFile( + mode="w+", suffix=".txt", delete=False + ) as tmp_file: tmp_path = tmp_file.name - + logger.info(f"Created temp file: {tmp_path}") - - cmd = ['rip'] + + cmd = ["rip"] if os.path.exists(STREAMRIP_CONFIG): - cmd.extend(['--config-path', STREAMRIP_CONFIG]) + cmd.extend(["--config-path", STREAMRIP_CONFIG]) logger.info(f"Using config file: {STREAMRIP_CONFIG}") else: logger.warning(f"Config file not found at: {STREAMRIP_CONFIG}") - - cmd.extend(['search', '--output-file', tmp_path]) + + cmd.extend(["search", "--output-file", tmp_path]) cmd.extend([source, search_type, query]) - + logger.info(f"Executing command: {' '.join(cmd)}") - - result = subprocess.run(cmd, capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=30) - + + result = subprocess.run( + cmd, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=30, + ) + logger.info(f"Command completed with return code: {result.returncode}") - + if result.stdout: logger.info(f"STDOUT ({len(result.stdout)} chars total):\n{result.stdout}") else: logger.info("STDOUT: (empty)") - + if result.stderr: - logger.warning(f"STDERR ({len(result.stderr)} chars total):\n{result.stderr}") + logger.warning( + f"STDERR ({len(result.stderr)} chars total):\n{result.stderr}" + ) else: logger.info("STDERR: (empty)") - + if result.returncode != 0: - logger.error(f"Streamrip command failed with return code {result.returncode}") + logger.error( + f"Streamrip command failed with return code {result.returncode}" + ) error_msg = "Streamrip search failed" - + if result.stdout: - if 'InvalidAppSecretError' in result.stdout: + if "InvalidAppSecretError" in result.stdout: error_msg = "Invalid Qobuz app secrets. Update your config with valid secrets or run 'rip config --update' in the container." - elif 'Traceback' in result.stdout: - error_msg = "Streamrip encountered an error (check logs for full traceback)" - elif 'authentication' in result.stdout.lower(): - error_msg = "Authentication failed - check your Qobuz credentials in config" - elif 'credentials' in result.stdout.lower(): + elif "Traceback" in result.stdout: + error_msg = ( + "Streamrip encountered an error (check logs for full traceback)" + ) + elif "authentication" in result.stdout.lower(): + error_msg = ( + "Authentication failed - check your Qobuz credentials in config" + ) + elif "credentials" in result.stdout.lower(): error_msg = "Invalid credentials - check your Qobuz configuration" - - return jsonify({ - 'error': error_msg, - 'debug_info': { - 'return_code': result.returncode, - 'stdout_preview': result.stdout if result.stdout else '', # Send full output - 'stderr_preview': result.stderr if result.stderr else '', - 'command': ' '.join(cmd) + + return jsonify( + { + "error": error_msg, + "debug_info": { + "return_code": result.returncode, + "stdout_preview": result.stdout + if result.stdout + else "", # Send full output + "stderr_preview": result.stderr if result.stderr else "", + "command": " ".join(cmd), + }, } - }), 500 - + ), 500 + # Check if temp file exists and has content if os.path.exists(tmp_path): file_size = os.path.getsize(tmp_path) logger.info(f"Temp file exists, size: {file_size} bytes") else: logger.error(f"Temp file does not exist: {tmp_path}") - + results = [] - + try: - with open(tmp_path, 'r') as f: + with open(tmp_path, "r") as f: content = f.read() logger.info(f"Streamrip search output: {content[:500]}") logger.info(f"File content length: {len(content)} characters") logger.debug(f"File content (first 500 chars):\n{content[:500]}") - - if not content or content.strip() == '': + + if not content or content.strip() == "": logger.warning("Temp file is empty!") - return jsonify({ - 'results': [], - 'query': query, - 'source': source, - 'total_count': 0, - 'message': 'No results found. The search returned empty results.', - 'debug_info': { - 'return_code': result.returncode, - 'stdout': result.stdout[:200] if result.stdout else '', - 'stderr': result.stderr[:200] if result.stderr else '' + return jsonify( + { + "results": [], + "query": query, + "source": source, + "total_count": 0, + "message": "No results found. The search returned empty results.", + "debug_info": { + "return_code": result.returncode, + "stdout": result.stdout[:200] if result.stdout else "", + "stderr": result.stderr[:200] if result.stderr else "", + }, } - }) - + ) + try: search_data = json.loads(content) - logger.info(f"Successfully parsed JSON with {len(search_data)} items") - + logger.info( + f"Successfully parsed JSON with {len(search_data)} items" + ) + for idx, item in enumerate(search_data): - item_id = item.get('id', '') - media_type = item.get('media_type', search_type) - url = construct_url(item.get('source', source), media_type, item_id) - - desc = item.get('desc', '') - artist = '' + item_id = item.get("id", "") + media_type = item.get("media_type", search_type) + url = construct_url( + item.get("source", source), media_type, item_id + ) + + desc = item.get("desc", "") + artist = "" title = desc - - if ' by ' in desc: - parts = desc.rsplit(' by ', 1) + + if " by " in desc: + parts = desc.rsplit(" by ", 1) title = parts[0] artist = parts[1] - + result_item = { - 'id': item_id, - 'service': item.get('source', source), - 'type': media_type, - 'artist': artist if artist else desc, - 'title': title if artist else '', - 'desc': desc, - 'url': url, - 'album_art': '' + "id": item_id, + "service": item.get("source", source), + "type": media_type, + "artist": artist if artist else desc, + "title": title if artist else "", + "desc": desc, + "url": url, + "album_art": "", } results.append(result_item) - + if idx < 3: # Log first 3 results logger.debug(f"Result {idx + 1}: {result_item}") - + except json.JSONDecodeError as e: logger.error("=" * 60) logger.error("JSON PARSE ERROR") @@ -849,7 +1047,7 @@ def search_music(): logger.error(f"FULL CONTENT (all {len(content)} chars):") logger.error(content) logger.error("=" * 60) - + # Also log what streamrip actually output logger.error("STREAMRIP STDOUT:") logger.error(result.stdout if result.stdout else "(empty)") @@ -857,29 +1055,36 @@ def search_music(): logger.error("STREAMRIP STDERR:") logger.error(result.stderr if result.stderr else "(empty)") logger.error("=" * 60) - - return jsonify({ - 'error': 'Failed to parse search results', - 'debug_info': { - 'parse_error': str(e), - 'content_length': len(content), - 'content_preview': content[:500], - 'full_content': content, # Include full content in response - 'stdout': result.stdout, - 'stderr': result.stderr - } - }), 500 - + + return ( + jsonify( + { + "error": "Failed to parse search results", + "debug_info": { + "parse_error": str(e), + "content_length": len(content), + "content_preview": content[:500], + "full_content": content, # Include full content in response + "stdout": result.stdout, + "stderr": result.stderr, + }, + } + ), + 500, + ) + except FileNotFoundError: logger.error(f"Temp file not found: {tmp_path}") - return jsonify({ - 'error': 'Search output file not found', - 'debug_info': { - 'temp_path': tmp_path, - 'return_code': result.returncode + return jsonify( + { + "error": "Search output file not found", + "debug_info": { + "temp_path": tmp_path, + "return_code": result.returncode, + }, } - }), 500 - + ), 500 + finally: if os.path.exists(tmp_path): try: @@ -887,43 +1092,43 @@ def search_music(): logger.debug(f"Removed temp file: {tmp_path}") except Exception as e: logger.warning(f"Failed to remove temp file: {e}") - + logger.info(f"Returning {len(results)} results") - - return jsonify({ - 'results': results, - 'query': query, - 'source': source, - 'total_count': len(results) - }) - + + return jsonify( + { + "results": results, + "query": query, + "source": source, + "total_count": len(results), + } + ) + except subprocess.TimeoutExpired: logger.error("Search command timed out after 30 seconds") - return jsonify({'error': 'Search timed out'}), 500 + return jsonify({"error": "Search timed out"}), 500 except Exception as e: logger.exception(f"Unexpected error during search: {e}") - return jsonify({ - 'error': str(e), - 'debug_info': { - 'exception_type': type(e).__name__ - } - }), 500 + return jsonify( + {"error": str(e), "debug_info": {"exception_type": type(e).__name__}} + ), 500 -@app.route('/api/album-art', methods=['GET']) + +@app.route("/api/album-art", methods=["GET"]) def get_album_art(): - source = request.args.get('source') - media_type = request.args.get('type') - item_id = request.args.get('id') - + source = request.args.get("source") + media_type = request.args.get("type") + item_id = request.args.get("id") + if not all([source, media_type, item_id]): - return jsonify({'error': 'Missing parameters'}), 400 - - #Todo: handle SoundCloud special case and get correct albums if possible - if source == 'soundcloud': - if '|' in item_id: - item_id = item_id.split('|')[0] - elif 'soundcloud:tracks:' in item_id: - match = re.search(r'soundcloud:tracks:(\d+)', item_id) + return jsonify({"error": "Missing parameters"}), 400 + + # Todo: handle SoundCloud special case and get correct albums if possible + if source == "soundcloud": + if "|" in item_id: + item_id = item_id.split("|")[0] + elif "soundcloud:tracks:" in item_id: + match = re.search(r"soundcloud:tracks:(\d+)", item_id) if match: item_id = match.group(1) @@ -932,84 +1137,93 @@ def get_album_art(): cached = album_art_cache[cache_key] if isinstance(cached, dict): return jsonify(cached) - return jsonify({'album_art': cached}) + return jsonify({"album_art": cached}) try: - if source == 'qobuz': - result = fetch_single_album_art(item_id, media_type, None) + if source == "qobuz": + result = fetch_single_album_art(item_id, media_type, None) album_art_cache[cache_key] = result - return jsonify({ - 'album_art': result.get('album_art', ''), - 'tracks_count': result.get('tracks_count'), - 'release_type': result.get('release_type'), - 'year': result.get('year'), - }) - - elif source == 'tidal': - if media_type == 'artist': + return jsonify( + { + "album_art": result.get("album_art", ""), + "tracks_count": result.get("tracks_count"), + "release_type": result.get("release_type"), + "year": result.get("year"), + } + ) + + elif source == "tidal": + if media_type == "artist": album_art = f"https://resources.tidal.com/images/{item_id}/750x750.jpg" else: album_art = f"https://resources.tidal.com/images/{item_id}/320x320.jpg" - + if album_art: album_art_cache[cache_key] = album_art - return jsonify({'album_art': album_art}) - return jsonify({'album_art': ''}) - - elif source == 'deezer': - if media_type == 'artist': + return jsonify({"album_art": album_art}) + return jsonify({"album_art": ""}) + + elif source == "deezer": + if media_type == "artist": try: - response = requests.get(f"https://api.deezer.com/artist/{item_id}", timeout=3) + response = requests.get( + f"https://api.deezer.com/artist/{item_id}", timeout=3 + ) if response.status_code == 200: data = response.json() - album_art = data.get('picture_medium', data.get('picture', '')) + album_art = data.get("picture_medium", data.get("picture", "")) if album_art: album_art_cache[cache_key] = album_art - return jsonify({'album_art': album_art}) + return jsonify({"album_art": album_art}) except: pass - return jsonify({'album_art': ''}) + return jsonify({"album_art": ""}) else: album_art = f"https://api.deezer.com/{media_type}/{item_id}/image" if album_art: album_art_cache[cache_key] = album_art - return jsonify({'album_art': album_art}) - return jsonify({'album_art': ''}) - - elif source == 'soundcloud': - #SoundCloud doesn't provide easy access to artwork - #Just return empty and let the frontend handle placeholders - return jsonify({'album_art': ''}) - - #Default return for unknown sources - return jsonify({'album_art': ''}) - + return jsonify({"album_art": album_art}) + return jsonify({"album_art": ""}) + + elif source == "soundcloud": + # SoundCloud doesn't provide easy access to artwork + # Just return empty and let the frontend handle placeholders + return jsonify({"album_art": ""}) + + # Default return for unknown sources + return jsonify({"album_art": ""}) + except Exception as e: - logger.error(f"Error fetching album art for {source}/{media_type}/{item_id}: {e}") - return jsonify({'album_art': ''}) + logger.error( + f"Error fetching album art for {source}/{media_type}/{item_id}: {e}" + ) + return jsonify({"album_art": ""}) -@app.route('/api/browse') + +@app.route("/api/browse") def browse_downloads(): try: files = [] for root, dirs, filenames in os.walk(DOWNLOAD_DIR): for filename in filenames: - if filename.endswith(('.mp3', '.flac', '.m4a', '.opus')): + if filename.endswith((".mp3", ".flac", ".m4a", ".opus")): filepath = os.path.join(root, filename) rel_path = os.path.relpath(filepath, DOWNLOAD_DIR) - files.append({ - 'name': rel_path, - 'size': os.path.getsize(filepath), - 'modified': os.path.getmtime(filepath) - }) - - files.sort(key=lambda x: x['modified'], reverse=True) + files.append( + { + "name": rel_path, + "size": os.path.getsize(filepath), + "modified": os.path.getmtime(filepath), + } + ) + + files.sort(key=lambda x: x["modified"], reverse=True) return jsonify(files[:100]) except Exception as e: - return jsonify({'error': str(e)}), 500 + return jsonify({"error": str(e)}), 500 -@app.route('/api/library') +@app.route("/api/library") def library_albums(): """List the Library's album folders (Artist -> Album) for the Files tab tree. @@ -1018,13 +1232,13 @@ def library_albums(): /api/library/album). The Library is read-only (ADR-0003): a folder on disk carries no source URL, so there is no Redownload here.""" try: - return jsonify({'albums': list_library_albums(DOWNLOAD_DIR)}) + return jsonify({"albums": list_library_albums(DOWNLOAD_DIR)}) except Exception as e: logger.exception("Failed to list Library albums") - return jsonify({'error': str(e)}), 500 + return jsonify({"error": str(e)}), 500 -@app.route('/api/library/album') +@app.route("/api/library/album") def library_album_tracks(): """Lazily read one album folder's present tracks, with titles and track numbers from embedded tags (ADR-0003). Called when the user expands an album @@ -1033,106 +1247,114 @@ def library_album_tracks(): The ``path`` query parameter is the album's path relative to DOWNLOAD_DIR, as returned by /api/library; it is confined to DOWNLOAD_DIR so the endpoint cannot be used to read tags from arbitrary places on disk.""" - rel_path = request.args.get('path') + rel_path = request.args.get("path") if not rel_path: - return jsonify({'error': 'path is required'}), 400 + return jsonify({"error": "path is required"}), 400 - #Confine the resolved album folder to DOWNLOAD_DIR (reject traversal). + # Confine the resolved album folder to DOWNLOAD_DIR (reject traversal). base = os.path.realpath(DOWNLOAD_DIR) target = os.path.realpath(os.path.join(base, rel_path)) if target != base and not target.startswith(base + os.sep): - return jsonify({'error': 'invalid path'}), 400 + return jsonify({"error": "invalid path"}), 400 if not os.path.isdir(target): - return jsonify({'error': 'album not found'}), 404 + return jsonify({"error": "album not found"}), 404 try: payload = get_album_assessment(DOWNLOAD_DIR, rel_path) - return jsonify({ - 'path': rel_path, - 'tracks': payload['tracks'], - 'completeness': payload['completeness'], - }) + return jsonify( + { + "path": rel_path, + "tracks": payload["tracks"], + "completeness": payload["completeness"], + } + ) except Exception as e: logger.exception(f"Failed to read album tracks for {rel_path}") - return jsonify({'error': str(e)}), 500 + return jsonify({"error": str(e)}), 500 def get_qobuz_credentials(): try: if os.path.exists(STREAMRIP_CONFIG): - with open(STREAMRIP_CONFIG, 'r') as f: + with open(STREAMRIP_CONFIG, "r") as f: config_content = f.read() app_id = re.search(r'app_id\s*=\s*["\']?([^"\'\n]+)["\']?', config_content) token = re.search(r'password_or_token\s*=\s*"([^"]+)"', config_content) return { - 'app_id': app_id.group(1).strip() if app_id else '950096963', - 'token': token.group(1).strip() if token else None + "app_id": app_id.group(1).strip() if app_id else "950096963", + "token": token.group(1).strip() if token else None, } except Exception as e: logger.error(f"Error reading Qobuz credentials: {e}") - return {'app_id': '950096963', 'token': None} + return {"app_id": "950096963", "token": None} def fetch_single_album_art(item_id, media_type, app_id): creds = get_qobuz_credentials() - if not creds['token']: + if not creds["token"]: return {} try: response = requests.get( f"https://www.qobuz.com/api.json/0.2/{media_type}/get", params={ - 'app_id': creds['app_id'], - f'{media_type}_id': item_id, + "app_id": creds["app_id"], + f"{media_type}_id": item_id, }, headers={ - 'X-App-Id': creds['app_id'], - 'X-User-Auth-Token': creds['token'], + "X-App-Id": creds["app_id"], + "X-User-Auth-Token": creds["token"], }, - timeout=3 + timeout=3, ) if response.status_code == 200: data = response.json() - image = data.get('image', {}) + image = data.get("image", {}) year = None - release_date = data.get('release_date_original', '') + release_date = data.get("release_date_original", "") if release_date: year = release_date[:4] return { - 'album_art': image.get('large') or image.get('small') or image.get('thumbnail') or '', - 'tracks_count': data.get('tracks_count'), - 'release_type': data.get('release_type'), - 'year': year, + "album_art": image.get("large") + or image.get("small") + or image.get("thumbnail") + or "", + "tracks_count": data.get("tracks_count"), + "release_type": data.get("release_type"), + "year": year, } except Exception as e: logger.error(f"Error fetching Qobuz album art: {e}") return {} + def get_qobuz_app_id(): try: if os.path.exists(STREAMRIP_CONFIG): - with open(STREAMRIP_CONFIG, 'r') as f: + with open(STREAMRIP_CONFIG, "r") as f: config_content = f.read() - #logger.debug(f"Config file content: {config_content[:200]}...") # First 200 chars - - app_id_match = re.search(r'app_id\s*=\s*["\']?([^"\'\n]+)["\']?', config_content) - + # logger.debug(f"Config file content: {config_content[:200]}...") # First 200 chars + + app_id_match = re.search( + r'app_id\s*=\s*["\']?([^"\'\n]+)["\']?', config_content + ) + if app_id_match: app_id = app_id_match.group(1).strip() logger.debug(f"Found app_id in config: {app_id}") return app_id else: logger.debug("No app_id found in config, using fallback") - - #Return a known working app_id as fallback + + # Return a known working app_id as fallback fallback_app_id = "950096963" logger.debug(f"Using fallback app_id: {fallback_app_id}") return fallback_app_id - + except Exception as e: logger.error(f"Error extracting app_id: {e}") return "950096963" @@ -1140,195 +1362,194 @@ def get_qobuz_app_id(): def construct_url(source, media_type, item_id): if not item_id: - return '' - + return "" + url_patterns = { - 'qobuz': { - 'album': f'https://open.qobuz.com/album/{item_id}', - 'track': f'https://open.qobuz.com/track/{item_id}', - 'artist': f'https://open.qobuz.com/artist/{item_id}', - 'playlist': f'https://open.qobuz.com/playlist/{item_id}' + "qobuz": { + "album": f"https://open.qobuz.com/album/{item_id}", + "track": f"https://open.qobuz.com/track/{item_id}", + "artist": f"https://open.qobuz.com/artist/{item_id}", + "playlist": f"https://open.qobuz.com/playlist/{item_id}", }, - 'tidal': { - 'album': f'https://tidal.com/browse/album/{item_id}', - 'track': f'https://tidal.com/browse/track/{item_id}', - 'artist': f'https://tidal.com/browse/artist/{item_id}', - 'playlist': f'https://tidal.com/browse/playlist/{item_id}' + "tidal": { + "album": f"https://tidal.com/browse/album/{item_id}", + "track": f"https://tidal.com/browse/track/{item_id}", + "artist": f"https://tidal.com/browse/artist/{item_id}", + "playlist": f"https://tidal.com/browse/playlist/{item_id}", }, - 'deezer': { - 'album': f'https://www.deezer.com/album/{item_id}', - 'track': f'https://www.deezer.com/track/{item_id}', - 'artist': f'https://www.deezer.com/artist/{item_id}', - 'playlist': f'https://www.deezer.com/playlist/{item_id}' + "deezer": { + "album": f"https://www.deezer.com/album/{item_id}", + "track": f"https://www.deezer.com/track/{item_id}", + "artist": f"https://www.deezer.com/artist/{item_id}", + "playlist": f"https://www.deezer.com/playlist/{item_id}", + }, + "soundcloud": { + "track": f"https://soundcloud.com/{item_id}", + "album": f"https://soundcloud.com/{item_id}", + "playlist": f"https://soundcloud.com/{item_id}", }, - 'soundcloud': { - 'track': f'https://soundcloud.com/{item_id}', - 'album': f'https://soundcloud.com/{item_id}', - 'playlist': f'https://soundcloud.com/{item_id}' - } } - + if source in url_patterns and media_type in url_patterns[source]: return url_patterns[source][media_type] - - return f'https://open.{source}.com/{media_type}/{item_id}' - + return f"https://open.{source}.com/{media_type}/{item_id}" + def extract_metadata_from_url(url): metadata = { - 'service': None, - 'type': None, - 'id': None, - 'title': None, - 'artist': None, - 'album_art': None + "service": None, + "type": None, + "id": None, + "title": None, + "artist": None, + "album_art": None, } - + try: - if 'spotify.com' in url: - metadata['service'] = 'spotify' - match = re.search(r'/(album|track|playlist|artist)/([a-zA-Z0-9]+)', url) + if "spotify.com" in url: + metadata["service"] = "spotify" + match = re.search(r"/(album|track|playlist|artist)/([a-zA-Z0-9]+)", url) if match: - metadata['type'] = match.group(1) - metadata['id'] = match.group(2) - #Note: Spotify requires OAuth for metadata, so we can't easily fetch it - - elif 'qobuz.com' in url: - metadata['service'] = 'qobuz' - match = re.search(r'/(album|track|playlist|artist)/([0-9]+)', url) + metadata["type"] = match.group(1) + metadata["id"] = match.group(2) + # Note: Spotify requires OAuth for metadata, so we can't easily fetch it + + elif "qobuz.com" in url: + metadata["service"] = "qobuz" + match = re.search(r"/(album|track|playlist|artist)/([0-9]+)", url) if match: - metadata['type'] = match.group(1) - metadata['id'] = match.group(2) - metadata.update(fetch_qobuz_metadata(metadata['id'], metadata['type'])) - - elif 'tidal.com' in url: - metadata['service'] = 'tidal' - match = re.search(r'/(album|track|playlist|artist)/([0-9]+)', url) + metadata["type"] = match.group(1) + metadata["id"] = match.group(2) + metadata.update(fetch_qobuz_metadata(metadata["id"], metadata["type"])) + + elif "tidal.com" in url: + metadata["service"] = "tidal" + match = re.search(r"/(album|track|playlist|artist)/([0-9]+)", url) if match: - metadata['type'] = match.group(1) - metadata['id'] = match.group(2) - metadata['album_art'] = f"https://resources.tidal.com/images/{metadata['id']}/320x320.jpg" - - elif 'deezer.com' in url: - metadata['service'] = 'deezer' - match = re.search(r'/(album|track|playlist|artist)/([0-9]+)', url) + metadata["type"] = match.group(1) + metadata["id"] = match.group(2) + metadata["album_art"] = ( + f"https://resources.tidal.com/images/{metadata['id']}/320x320.jpg" + ) + + elif "deezer.com" in url: + metadata["service"] = "deezer" + match = re.search(r"/(album|track|playlist|artist)/([0-9]+)", url) if match: - metadata['type'] = match.group(1) - metadata['id'] = match.group(2) - metadata.update(fetch_deezer_metadata(metadata['id'], metadata['type'])) - + metadata["type"] = match.group(1) + metadata["id"] = match.group(2) + metadata.update(fetch_deezer_metadata(metadata["id"], metadata["type"])) + except Exception as e: logger.error(f"Error extracting metadata from URL: {e}") - + return metadata + def fetch_qobuz_metadata(item_id, item_type): metadata = {} try: app_id = get_qobuz_app_id() api_base = "https://www.qobuz.com/api.json/0.2" - - if item_type == 'album': + + if item_type == "album": response = requests.get( f"{api_base}/album/get", - params={'album_id': item_id, 'app_id': app_id}, - timeout=5 + params={"album_id": item_id, "app_id": app_id}, + timeout=5, ) if response.status_code == 200: data = response.json() - metadata['title'] = data.get('title', '') - metadata['artist'] = data.get('artist', {}).get('name', '') - if 'image' in data: - for size in ['small', 'medium', 'large', 'thumbnail']: - if size in data['image']: - metadata['album_art'] = data['image'][size] + metadata["title"] = data.get("title", "") + metadata["artist"] = data.get("artist", {}).get("name", "") + if "image" in data: + for size in ["small", "medium", "large", "thumbnail"]: + if size in data["image"]: + metadata["album_art"] = data["image"][size] break - - elif item_type == 'track': + + elif item_type == "track": response = requests.get( f"{api_base}/track/get", - params={'track_id': item_id, 'app_id': app_id}, - timeout=5 + params={"track_id": item_id, "app_id": app_id}, + timeout=5, ) if response.status_code == 200: data = response.json() - metadata['title'] = data.get('title', '') - metadata['artist'] = data.get('performer', {}).get('name', '') - album = data.get('album', {}) - if 'image' in album: - for size in ['small', 'medium', 'large', 'thumbnail']: - if size in album['image']: - metadata['album_art'] = album['image'][size] + metadata["title"] = data.get("title", "") + metadata["artist"] = data.get("performer", {}).get("name", "") + album = data.get("album", {}) + if "image" in album: + for size in ["small", "medium", "large", "thumbnail"]: + if size in album["image"]: + metadata["album_art"] = album["image"][size] break - + except Exception as e: logger.error(f"Error fetching Qobuz metadata: {e}") - + return metadata + def fetch_deezer_metadata(item_id, item_type): metadata = {} try: api_base = "https://api.deezer.com" - - if item_type == 'album': + + if item_type == "album": response = requests.get(f"{api_base}/album/{item_id}", timeout=5) if response.status_code == 200: data = response.json() - metadata['title'] = data.get('title', '') - metadata['artist'] = data.get('artist', {}).get('name', '') - metadata['album_art'] = data.get('cover_medium', '') - - elif item_type == 'track': + metadata["title"] = data.get("title", "") + metadata["artist"] = data.get("artist", {}).get("name", "") + metadata["album_art"] = data.get("cover_medium", "") + + elif item_type == "track": response = requests.get(f"{api_base}/track/{item_id}", timeout=5) if response.status_code == 200: data = response.json() - metadata['title'] = data.get('title', '') - metadata['artist'] = data.get('artist', {}).get('name', '') - album = data.get('album', {}) - metadata['album_art'] = album.get('cover_medium', '') - + metadata["title"] = data.get("title", "") + metadata["artist"] = data.get("artist", {}).get("name", "") + album = data.get("album", {}) + metadata["album_art"] = album.get("cover_medium", "") + except Exception as e: logger.error(f"Error fetching Deezer metadata: {e}") - + return metadata - - -@app.route('/api/download-from-url', methods=['POST']) + + +@app.route("/api/download-from-url", methods=["POST"]) def download_from_url(): data = request.json - url = data.get('url') - quality = data.get('quality', 3) - - title = data.get('title') - artist = data.get('artist') - album_art = data.get('album_art') - service = data.get('service') - + url = data.get("url") + quality = data.get("quality", 3) + + title = data.get("title") + artist = data.get("artist") + album_art = data.get("album_art") + service = data.get("service") + if not url: - return jsonify({'error': 'URL required'}), 400 - + return jsonify({"error": "URL required"}), 400 + if title and artist and service: metadata = { - 'title': title, - 'artist': artist, - 'album_art': album_art, - 'service': service + "title": title, + "artist": artist, + "album_art": album_art, + "service": service, } else: metadata = extract_metadata_from_url(url) - + task_id = enqueue_download(url, quality, metadata) - return jsonify({ - 'task_id': task_id, - 'status': 'queued', - 'metadata': metadata - }) + return jsonify({"task_id": task_id, "status": "queued", "metadata": metadata}) -@app.route('/api/redownload', methods=['POST']) +@app.route("/api/redownload", methods=["POST"]) def redownload(): """Redownload a History entry, bypassing the Streamrip database. @@ -1339,39 +1560,41 @@ def redownload(): the normal lifecycle: it is registered as Queued and visible immediately, with a fresh id distinct from the original History entry.""" data = request.json or {} - entry_id = data.get('id') + entry_id = data.get("id") if not entry_id: - return jsonify({'error': 'History entry id is required'}), 400 + return jsonify({"error": "History entry id is required"}), 400 with history_lock: - entry = next((e for e in download_history if e['id'] == entry_id), None) + entry = next((e for e in download_history if e["id"] == entry_id), None) if entry is None: - return jsonify({'error': 'History entry not found'}), 404 + return jsonify({"error": "History entry not found"}), 404 - url = entry.get('url') + url = entry.get("url") if not url: - #Redownload is only meaningful for items whose source URL is known. - return jsonify({'error': 'History entry has no source URL to redownload'}), 400 + # Redownload is only meaningful for items whose source URL is known. + return jsonify({"error": "History entry has no source URL to redownload"}), 400 - quality = entry.get('quality') + quality = entry.get("quality") if quality is None: quality = 3 - metadata = entry.get('metadata') or {} + metadata = entry.get("metadata") or {} task_id = enqueue_download(url, quality, metadata, no_db=True) - return jsonify({ - 'task_id': task_id, - 'status': 'queued', - 'metadata': metadata, - }) + return jsonify( + { + "task_id": task_id, + "status": "queued", + "metadata": metadata, + } + ) -if __name__ == '__main__': +if __name__ == "__main__": logger.info("Starting Streamrip Web application...") logger.info(f"Config path: {STREAMRIP_CONFIG}") logger.info(f"Download directory: {DOWNLOAD_DIR}") logger.info(f"Max concurrent downloads: {MAX_CONCURRENT_DOWNLOADS}") - app.run(host='0.0.0.0', port=5000, debug=False) + app.run(host="0.0.0.0", port=5000, debug=False) diff --git a/docs/adr/0003-completeness-from-embedded-tags.md b/docs/adr/0003-completeness-from-embedded-tags.md index 4578fd0..ca0f2fe 100644 --- a/docs/adr/0003-completeness-from-embedded-tags.md +++ b/docs/adr/0003-completeness-from-embedded-tags.md @@ -18,11 +18,23 @@ them back. the album's true `tracktotal`, so all gaps — including trailing ones — are detectable from disk alone, with no network, no stored metadata, and no DB. +## Tag semantics (verified empirically against real rips) + +streamrip writes `tracktotal` as the **album-wide** track count while +`tracknumber` **restarts per disc**; there is no per-disc total in the tags. +`disctotal` reveals how many discs the album has. Multi-disc audio sits in +`Disc N` subfolders of a flat album folder (the default `folder_format` has no +artist directory level). + ## Consequences A missing track can only be shown by number ("Track 7 — missing"), never by title, because a track that was never downloaded left no tags on disk. An album with zero readable-tag tracks is reported as **Unknown** rather than guessed. +On a multi-disc album the *number* of absent tracks is exact (album total minus +present count) but a trailing gap cannot be attributed to a specific disc — +those are reported as "position unknown" rather than guessed; an entirely +absent disc is detected via `disctotal` and reported as a missing disc. The Library view is read-only: it cannot offer Redownload, because a folder on disk carries no source URL (Redownload lives in History — see the Redownload glossary entry). diff --git a/static/css/style.css b/static/css/style.css index 8138285..4085c94 100644 --- a/static/css/style.css +++ b/static/css/style.css @@ -1132,6 +1132,19 @@ button:disabled { height: 48px; } + /* History cards: let the row wrap so the redownload button drops to a + full-width line below the info instead of overflowing the viewport */ + .download-content { + flex-wrap: wrap; + } + + .redownload-btn { + flex-basis: 100%; + margin-left: 0; + padding: 12px 20px; + white-space: normal; + } + .download-spinner { margin-right: 0; } diff --git a/static/js/app.js b/static/js/app.js index 728cc37..24cbaee 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -499,6 +499,9 @@ async function loadLibrary() { } // Group albums under their artist to build the Artist -> Album tree. + // Albums at the download root have no artist directory (streamrip's + // default flat folder_format — the folder name carries the artist): + // they render ungrouped, without a fabricated artist header. const byArtist = new Map(); albums.forEach(album => { if (!byArtist.has(album.artist)) { @@ -507,19 +510,24 @@ async function loadLibrary() { byArtist.get(album.artist).push(album); }); - container.innerHTML = Array.from(byArtist.entries()).map(([artist, artistAlbums]) => ` + const renderAlbum = album => ` +
+ +
+
+ `; + + container.innerHTML = Array.from(byArtist.entries()).map(([artist, artistAlbums]) => + artist === null || artist === undefined || artist === '' + ? artistAlbums.map(renderAlbum).join('') + : `
${escapeHtml(artist)}
- ${artistAlbums.map(album => ` -
- -
-
- `).join('')} + ${artistAlbums.map(renderAlbum).join('')}
`).join(''); @@ -547,8 +555,13 @@ function applyCompletenessBadge(albumEl, completeness) { badge.textContent = 'COMPLETE'; } else if (status === 'incomplete') { const n = completeness.missing_count || 0; + const discs = completeness.missing_discs || []; + const parts = []; + if (n > 0) parts.push(`${n} missing`); + // A wholly missing disc has an unknowable track count — name the disc. + discs.forEach(d => parts.push(`disc ${d} missing`)); badge.classList.add('badge-incomplete'); - badge.textContent = `INCOMPLETE (${n} missing)`; + badge.textContent = `INCOMPLETE (${parts.join(', ')})`; } else { badge.classList.add('badge-unknown'); badge.textContent = 'UNKNOWN'; @@ -615,10 +628,19 @@ async function toggleAlbum(button) { tracksEl.innerHTML = tracks.map(track => { const num = track.tracknumber != null ? String(track.tracknumber).padStart(2, '0') : '--'; if (track.missing) { + // A wholly missing disc gets one gap row (its track count is + // unknowable from disk); unlocated absences (multi-disc + // trailing gaps) get one summary row; a locatable missing + // track names its number. + const label = track.missing_disc + ? `Disc ${escapeHtml(String(track.discnumber))} — missing entirely` + : track.unlocated_count + ? `${escapeHtml(String(track.unlocated_count))} track(s) missing — position unknown` + : `Track ${escapeHtml(String(track.tracknumber))} — missing`; return `
${escapeHtml(num)} - Track ${escapeHtml(String(track.tracknumber))} — missing + ${label}
`; } diff --git a/tests/test_completeness.py b/tests/test_completeness.py index 25ad3ce..fa3aefc 100644 --- a/tests/test_completeness.py +++ b/tests/test_completeness.py @@ -116,3 +116,27 @@ def test_slashed_totals_are_parsed(): ]) assert result['status'] == 'incomplete' assert result['missing'] == [{'disc': 1, 'track': 2}, {'disc': 1, 'track': 3}] + + +def test_wholly_missing_disc_detected_via_disctotal(): + # disctotal on a present track reveals the album has 2 discs; disc 2 has no + # present tracks at all -> incomplete, reported as a missing disc (its track + # count is unknowable from disk, so it is not enumerated in 'missing'). + result = assess([ + {'disc': '1', 'track': '1', 'tracktotal': '2', 'disctotal': '2'}, + ]) + assert result['status'] == 'incomplete' + assert result['missing'] == [] + assert result['missing_discs'] == [2] + # The album-wide tracktotal counts the missing disc's track as unlocated. + assert result['unlocated'] == 1 + + +def test_no_missing_discs_when_all_discs_have_tracks(): + result = assess([ + {'disc': '1', 'track': '1', 'tracktotal': '2', 'disctotal': '2'}, + {'disc': '2', 'track': '1', 'tracktotal': '2', 'disctotal': '2'}, + ]) + assert result['status'] == 'complete' + assert result['missing_discs'] == [] + assert result['unlocated'] == 0 diff --git a/tests/test_library_api.py b/tests/test_library_api.py index 5ac5f13..e296c36 100644 --- a/tests/test_library_api.py +++ b/tests/test_library_api.py @@ -233,29 +233,61 @@ def test_album_endpoint_trailing_gap_rows(client, library, monkeypatch): assert [t['missing'] for t in rows] == [False, True, True] -def test_album_endpoint_multi_disc_missing_against_disc(client, library, monkeypatch): +def test_album_endpoint_multi_disc_interior_gap_against_disc(client, library, monkeypatch): + # streamrip tags tracktotal ALBUM-wide while tracknumber restarts per disc. + # An interior gap (disc 2 has tracks 1 and 3) is locatable from the numbering + # and reported against its disc. rel = os.path.relpath( _make_album(library, 'Artist', 'Album', - ['d1t1.flac', 'd1t2.flac', 'd2t1.flac']), + ['d1t1.flac', 'd1t2.flac', 'd2t1.flac', 'd2t3.flac']), library, ) tags = { - 'd1t1.flac': {'tracknumber': '1', 'discnumber': '1', 'tracktotal': '2', 'disctotal': '2'}, - 'd1t2.flac': {'tracknumber': '2', 'discnumber': '1', 'tracktotal': '2', 'disctotal': '2'}, - 'd2t1.flac': {'tracknumber': '1', 'discnumber': '2', 'tracktotal': '2', 'disctotal': '2'}, + 'd1t1.flac': {'tracknumber': '1', 'discnumber': '1', 'tracktotal': '5', 'disctotal': '2'}, + 'd1t2.flac': {'tracknumber': '2', 'discnumber': '1', 'tracktotal': '5', 'disctotal': '2'}, + 'd2t1.flac': {'tracknumber': '1', 'discnumber': '2', 'tracktotal': '5', 'disctotal': '2'}, + 'd2t3.flac': {'tracknumber': '3', 'discnumber': '2', 'tracktotal': '5', 'disctotal': '2'}, } monkeypatch.setattr(app_module, 'read_audio_tags', lambda fp: tags[os.path.basename(fp)]) data = _tracks(client, rel).get_json() assert data['completeness']['status'] == 'incomplete' - # Disc 2 track 2 missing, reported against disc 2. + # Disc 2 track 2 missing, reported against disc 2; nothing unlocated. assert data['completeness']['missing'] == [{'disc': 2, 'track': 2}] + assert data['completeness']['unlocated'] == 0 gap = [t for t in data['tracks'] if t['missing']] assert len(gap) == 1 assert gap[0]['discnumber'] == 2 and gap[0]['tracknumber'] == 2 +def test_album_endpoint_multi_disc_trailing_gap_is_unlocated(client, library, monkeypatch): + # A trailing gap on a multi-disc album cannot be pinned to a disc (tracktotal + # is album-wide): the absent-track count is exact, the position unknown. + rel = os.path.relpath( + _make_album(library, 'Artist', 'Album', + ['d1t1.flac', 'd1t2.flac', 'd2t1.flac']), + library, + ) + tags = { + 'd1t1.flac': {'tracknumber': '1', 'discnumber': '1', 'tracktotal': '4', 'disctotal': '2'}, + 'd1t2.flac': {'tracknumber': '2', 'discnumber': '1', 'tracktotal': '4', 'disctotal': '2'}, + 'd2t1.flac': {'tracknumber': '1', 'discnumber': '2', 'tracktotal': '4', 'disctotal': '2'}, + } + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)]) + + data = _tracks(client, rel).get_json() + assert data['completeness']['status'] == 'incomplete' + assert data['completeness']['missing'] == [] + assert data['completeness']['unlocated'] == 1 + assert data['completeness']['missing_count'] == 1 + # One summary gap row, sorted last. + rows = data['tracks'] + assert rows[-1]['missing'] is True + assert rows[-1].get('unlocated_count') == 1 + + def test_album_endpoint_no_tags_is_unknown(client, library, monkeypatch): rel = os.path.relpath( _make_album(library, 'Artist', 'Album', ['x.flac']), @@ -321,6 +353,110 @@ def test_assessment_cache_invalidated_when_folder_changes(client, library, monke assert data['completeness']['missing'] == [{'disc': 1, 'track': 2}] +# --- streamrip's real on-disk layout (regression for the Library tree bug) --- +# streamrip's default folder_format puts album folders FLAT at the download +# root ("Artist - Album (Year) [FLAC] [...]"), with no artist directory level, +# and multi-disc albums hold their audio in "Disc N" subfolders. + + +def _make_flat_album(base, album, audio_files): + album_dir = os.path.join(base, album) + os.makedirs(album_dir, exist_ok=True) + for name in audio_files: + open(os.path.join(album_dir, name), 'wb').close() + return album_dir + + +def test_flat_album_folder_has_no_fabricated_artist(client, library): + # A root-level album folder must not be grouped under "Unknown Artist" — + # the folder name already carries the artist; artist comes back null. + _make_flat_album(library, 'Radiohead - The Bends (1995) [FLAC]', ['01.flac']) + + albums = _albums(client) + assert len(albums) == 1 + assert albums[0]['album'] == 'Radiohead - The Bends (1995) [FLAC]' + assert albums[0]['artist'] is None + + +def test_disc_subfolders_fold_into_one_album(client, library): + # A multi-disc album (audio in "Disc 1"/"Disc 2" subfolders) is ONE album — + # the parent folder — never two albums named "Disc 1" and "Disc 2" grouped + # under the album name as if it were an artist. + album = 'Radiohead - OK Computer OKNOTOK 1997 2017 (2017) [FLAC]' + _make_flat_album(library, os.path.join(album, 'Disc 1'), ['01.flac']) + _make_flat_album(library, os.path.join(album, 'Disc 2'), ['01.flac']) + + albums = _albums(client) + assert len(albums) == 1 + assert albums[0]['album'] == album + assert albums[0]['path'] == album + assert albums[0]['artist'] is None + + +def test_nested_artist_album_layout_still_grouped(client, library): + # Users with a configured Artist/Album folder_format keep artist grouping. + _make_album(library, 'Radiohead', 'OK Computer', ['01.flac']) + + albums = _albums(client) + assert len(albums) == 1 + assert albums[0]['artist'] == 'Radiohead' + assert albums[0]['album'] == 'OK Computer' + + +def test_album_tracks_collected_from_disc_subfolders(client, library, monkeypatch): + album = 'Artist - Album (2020) [FLAC]' + _make_flat_album(library, os.path.join(album, 'Disc 1'), ['d1t1.flac']) + _make_flat_album(library, os.path.join(album, 'Disc 2'), ['d2t1.flac']) + + tags = { + 'd1t1.flac': {'title': 'One', 'tracknumber': '1', 'discnumber': '1', + 'tracktotal': '2', 'disctotal': '2'}, + 'd2t1.flac': {'title': 'Two', 'tracknumber': '1', 'discnumber': '2', + 'tracktotal': '2', 'disctotal': '2'}, + } + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: tags[os.path.basename(fp)]) + + data = _tracks(client, album).get_json() + assert [t['title'] for t in data['tracks']] == ['One', 'Two'] + # Both discs present and full -> the album as a whole is complete. + assert data['completeness']['status'] == 'complete' + + +def test_cache_invalidated_when_disc_subfolder_changes(client, library, monkeypatch): + # Deleting a track inside "Disc 2" bumps only the subfolder's mtime, not the + # album folder's — the assessment cache must still be invalidated. + album = 'Artist - Album (2020) [FLAC]' + _make_flat_album(library, os.path.join(album, 'Disc 1'), ['d1t1.flac']) + d2 = _make_flat_album(library, os.path.join(album, 'Disc 2'), ['d2t1.flac']) + app_module.album_assessment_cache.clear() + + tags = { + 'd1t1.flac': {'title': 'One', 'tracknumber': '1', 'discnumber': '1', + 'tracktotal': '2', 'disctotal': '2'}, + 'd2t1.flac': {'title': 'Two', 'tracknumber': '1', 'discnumber': '2', + 'tracktotal': '2', 'disctotal': '2'}, + } + monkeypatch.setattr(app_module, 'read_audio_tags', + lambda fp: tags.get(os.path.basename(fp), {})) + + assert _tracks(client, album).get_json()['completeness']['status'] == 'complete' + + os.remove(os.path.join(d2, 'd2t1.flac')) + future = os.stat(d2).st_mtime + 10 + os.utime(d2, (future, future)) + + data = _tracks(client, album).get_json() + assert data['completeness']['status'] == 'incomplete' + # Disc 2 vanished entirely: its track count is unknowable from disk, so it + # is reported as a wholly missing disc (revealed by disc 1's disctotal tag), + # not as enumerated missing tracks. + assert data['completeness']['missing'] == [] + assert data['completeness']['missing_discs'] == [2] + disc_rows = [t for t in data['tracks'] if t.get('missing_disc')] + assert len(disc_rows) == 1 and disc_rows[0]['discnumber'] == 2 + + def test_disc_number_orders_before_track_number(client, library, monkeypatch): rel = os.path.relpath( _make_album(library, 'Artist', 'Album', ['d2t1.flac', 'd1t2.flac', 'd1t1.flac']),