From def8298cd829b5d3a54c021c30c6cd20e7fa6ed5 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Thu, 21 May 2026 09:07:21 +0100 Subject: [PATCH] chore: add detailed code quality review notes from agent --- reviews/CODE-REVIEW.md | 130 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 reviews/CODE-REVIEW.md diff --git a/reviews/CODE-REVIEW.md b/reviews/CODE-REVIEW.md new file mode 100644 index 0000000..77483ff --- /dev/null +++ b/reviews/CODE-REVIEW.md @@ -0,0 +1,130 @@ +# StemDeck Code Review + +## Tool Availability + +No Go, protobuf, or static analysis tools applicable — Python/JS/Rust codebase reviewed manually. + +--- + +## Findings + +### Architecture & Design + +| ID | Finding | Severity | +|----|---------|----------| +| ARCH-1 | **`_set` private function leaks across module boundaries.** `app/pipeline/download.py:138` defines `_set()` as a module-private helper but it is imported by three other modules (`runner.py:15`, `analyze.py:9`, `separate.py:25`). `_set` has nothing to do with downloading — it is a generic job-field mutator that all pipeline stages need. It should live in `app/core/models.py` (or a `app/pipeline/utils.py`) and be public. | HIGH | +| ARCH-2 | **Module-level side effects execute at import time.** `app/main.py:156-157` calls `ensure_runtime_dirs()` and `restore_registry(JOBS_DIR)` unconditionally at module level, outside the FastAPI lifespan. This means any test that imports `app.main` hits the real filesystem, breaks test isolation, and makes the module non-reusable. Move both calls inside `lifespan()`. | MEDIUM | +| ARCH-3 | **`app/pipeline/__init__.py` re-exports `STEM_NAMES` from config.** `__init__.py` exports `STEM_NAMES` but it belongs in `app/core/config`. Consumers that import from `app.pipeline` for a config constant create a misleading dependency. Remove from `__init__.py`. | LOW | +| ARCH-4 | **`analyze.py` imports `JOBS_DIR` directly for a path check.** `app/pipeline/analyze.py:7` imports `JOBS_DIR` to guard the `_load_audio_ffmpeg` path check. This couples the analysis module to the global config. The caller (`runner.py`) already has the path; the guard is appropriate, but the source should be a passed argument rather than a module-level global import. | LOW | +| ARCH-5 | **`sections` field omitted from `_write_metadata`.** `runner.py:124-142` writes `metadata.json` after pipeline completion but does not include `job.sections`. Sections are stored via `PATCH /api/jobs/{id}/sections` which writes them to `metadata.json` separately. However if a job is new, the initial metadata file has no `sections` key; recovery via `_recover_done_job` reads `meta.get("sections")` so that is fine. But a `registry.persist()` call via `to_record()` does persist sections because `_JOB_FIELDS` includes it. The inconsistency between the two metadata paths (registry.json vs metadata.json) is subtle and may cause a future regression. | LOW | + +### Python Patterns + +| ID | Finding | Severity | +|----|---------|----------| +| PY-1 | **`app/api/events.py` missing `JOB_ID_RE` input validation.** `events.py:20` calls `registry_get(job_id)` without first validating the `job_id` format against `JOB_ID_RE`. Every other endpoint in `jobs.py` and `stems.py` validates the regex first to block filesystem traversal attempts before touching state. Missing here means a crafted `job_id` with path separators is checked only by the dict lookup (which will miss), but the defence-in-depth pattern is broken. Add `if not JOB_ID_RE.match(job_id): raise HTTPException(404)` before the registry call. | HIGH | +| PY-2 | **Pending-job capacity check is not atomic.** `jobs.py:138-140` and `jobs.py:154-156`: reading the count of queued jobs and registering a new job are two separate operations with no lock between them. Under rapid concurrent requests two clients can both read `pending < MAX_PENDING_JOBS` and both proceed to register. The existing `threading.Lock` in `registry.py` protects individual dict mutations but not the read-then-write pattern here. Fix: move the capacity guard inside a registry function that holds `_lock` for the check + register atomically. | MEDIUM | +| PY-3 | **`_probe_duration` in `jobs.py` is synchronous and blocks the event loop.** `jobs.py:47-69` uses `subprocess.run()` (blocking) directly in an async handler. It is called at line 213 inside `await asyncio.to_thread(...)`, which is correct. But at line 69 the `float(result.stdout.strip())` call happens inside the thread too (correct). **Actually no issue — the whole function is wrapped in `to_thread`.** Annotation-only note: the docstring says nothing about threading; add a comment noting it must only be called from a thread, not the event loop. | LOW | +| PY-4 | **`_create_youtube_job` leaks internal Pydantic validation messages to clients.** `jobs.py:131`: `detail=str(e)` on a Pydantic `ValidationError` can expose schema internals. Use `"Invalid request body"` or extract only the top-level message. | LOW | +| PY-5 | **`Job.status` is typed as `str`, not a `Literal` or `Enum`.** `models.py:16` uses a bare `str` with a comment listing valid values. This means typos like `"separting"` are not caught statically. Replace with `Literal["queued", "downloading", "analyzing", "separating", "done", "error", "cancelled"]` or a `StrEnum`. | LOW | +| PY-6 | **`download.py` retry sleep blocks the event loop.** `download.py:205` calls `time.sleep(wait)` inside `download()`, which runs inside `asyncio.to_thread()`. Blocking `time.sleep` inside a thread is fine (it doesn't block the event loop), but it does hold the thread-pool thread for the backoff period, potentially delaying other `to_thread` calls during a burst of failures. Replace with an async sleep by restructuring the retry loop to be async, or at minimum document that the sleep is intentional inside a thread. | LOW | +| PY-7 | **`runner.py` duplicates error/cancel handling between `run_pipeline` and `run_local_pipeline`.** Lines 145-173 and 176-206 are near-identical. The only difference is whether `job_dir.mkdir` is called upfront. Extract a `_run_pipeline_common(job, job_dir, blocking_fn, *args)` to eliminate the duplication. | MEDIUM | +| PY-8 | **`analyze.py` has a deferred import of `numpy` inside `analyze()`.** `analyze.py:305` imports `numpy as np` inside the function body after the outer `try`. The module-level comment says librosa is pre-warmed, but numpy's deferred import inside the function means the first call pays the import cost. Move `import numpy as np` to module level alongside librosa (with the same `try/except ImportError` guard). | LOW | + +### JS Patterns + +| ID | Finding | Severity | +|----|---------|----------| +| JS-1 | **`renderedJobs` and `jobSources` in `job.js` grow unbounded.** `job.js:19-20`: `renderedJobs` (Set) and `jobSources` (Map) are module-level and never pruned. Every job submitted in the session accumulates. For a local app with ~hundreds of jobs over time this is negligible in practice, but the `jobSources` Map in particular keeps the source URL string for every job ever submitted in the current session. Add cleanup when a job reaches a terminal status or cap to recent N entries. | LOW | +| JS-2 | **`wireAllButton` computes `noneSelected` but never uses it.** `main.js:78`: `const noneSelected = selectedStems.size === 0;` is declared inside the click handler but never read — the branch immediately after checks `allSelected`. Dead code. | LOW | +| JS-3 | **SSE and polling run simultaneously on every job submission.** `job.js:445-446` calls `startJobPolling(jobId)` followed immediately by `connectEvents(jobId)`. During normal operation SSE is the primary mechanism, but REST polling at 1-second intervals is also active from submission until terminal status, meaning 2× the requests on happy path. The polling is an intentional fallback per the comment, but it should be disabled while SSE is connected and only activated on SSE error. | MEDIUM | +| JS-4 | **`visualAudioContext` in `player.js` is never closed.** `player.js:170`: the `AudioContext` created for visual decoding persists across track loads (by design — `??=`). However `destroyPlayer()` (line 502) does not call `visualAudioContext.close()`. The Web Audio spec allows at most 6 concurrent `AudioContext` instances in some browsers; a long session with many track loads could silently fail decoding. Close the context in `destroyPlayer()` and null the reference so a new context is created on the next load. | MEDIUM | +| JS-5 | **`drawFooterPlaceholder` creates an untracked `ResizeObserver`.** `player.js:874-876`: `new ResizeObserver(...).observe(bar)` is created without a reference. It cannot be disconnected during `destroyPlayer()`. This is a minor memory leak per track load. Assign to a module-level variable and disconnect in `destroyPlayer()`. | LOW | +| JS-6 | **`catalog.js:154` silently swallows `loadState` JSON parse errors.** The outer `catch {}` on line 154 discards all errors including bugs. Add at minimum `console.warn("[catalog] loadState error:", e)`. | LOW | +| JS-7 | **`storeSetDebounced` uses `JSON.parse(JSON.stringify(value))` for deep clone.** `utils.js:37`: this throws on values containing `undefined`, `BigInt`, or circular references. For the mixer state this is fine, but it is a fragile pattern. Use `structuredClone(value)` (available in all modern browsers and Node 17+). | LOW | +| JS-8 | **`catalog.js:864` `thumbHtml` injects `track.thumb` directly into innerHTML without sanitization.** The `src` value is a YouTube CDN URL from the server and thus trusted, but if `track.thumb` ever contained a crafted string (e.g., from a malformed server response) it would be injected raw. Use `el.setAttribute("src", track.thumb)` via DOM API instead of template-literal innerHTML. | MEDIUM | +| JS-9 | **`catalog.js:900-911` `renderTrackItem` injects `track.title` via innerHTML.** The `cat-title` div is set by `${track.title ?? "Unknown track"}` in an innerHTML template. If a YouTube video has a title containing `