refactor(server): extract the audio-effects routes into routers/audio_effects.py (R3) - #834
Conversation
…_effects.py (R3) The first route module through the appstate seam (#833). Picked BY MEASUREMENT, not by the plan's guess: a transitive dep-closure scan over every route group ranked audio-effects at 0 monkeypatch.setattr targets and exactly one exclusive helper. (The same scan disproved the plan's assumption that artists/aliases was free -- api_artist_links reaches _mb_http_get and _enrich_network_enabled, both setattr targets.) Bodies are verbatim. The only edits are mechanical: @app.get(...) -> @router.get(...) audio_effect_mappings.x -> appstate.audio_effect_mappings.x The singleton read must stay a module attribute resolved at call time, so a re-imported server re-publishes a fresh DB into the seam and monkeypatch reaches this module. `routers/` never imports `server`: server -> routers -> appstate. `app.include_router(...)` sits exactly where the routes used to be defined -- FastAPI matches in registration order, so the mount site preserves it. Verified by diffing the FULL route table against origin/main: 143 routes, identical paths, methods AND order. server.py: 9,445 -> 9,386 lines. `fastapi.Query` went dead with the move and was removed (the other four unused imports are pre-existing on main). Packaging: COPY routers/ /app/routers/ plus `!routers/` + `!routers/**` in .dockerignore (that file opens with a blanket `*`). Verified against the real docker daemon: routers/ reaches the build context, __pycache__ does not. Verified: pyflakes clean on routers/; no new undefined name in server.py; pytest 2348 passed (75 in the audio-effects + demo-mode suites); eslint 0 errors; boot smoke drives all five routes end-to-end (create -> read back -> activate -> clear -> delete -> 404 on missing -> 400 on bad body), Query(...) still 422s on a missing required param, and demo mode still 403s all four moved write routes while allowing the read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe audio-effects mapping endpoints were extracted from ChangesAudio-effects router extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI_app
participant audio_effects_router
participant appstate_audio_effect_mappings
Client->>FastAPI_app: audio-effects request
FastAPI_app->>audio_effects_router: dispatch registered route
audio_effects_router->>appstate_audio_effect_mappings: perform mapping operation
appstate_audio_effect_mappings-->>audio_effects_router: return result or error
audio_effects_router-->>Client: JSON response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
routers/audio_effects.py (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence the Ruff B008 false positive via config, not code.
Body(...)/Body(default_factory=dict)as argument defaults are the standard FastAPI idiom; this is a known B008 false positive, not a real mutable-default bug, and pre-exists the extraction (verbatim per the module docstring). Rather than restructuring handlers, add these fastapi callables to Ruff's immutable-calls allowlist.🔧 Suggested pyproject.toml addition
[tool.ruff.lint.flake8-bugbear] extend-immutable-calls = ["fastapi.Body", "fastapi.Query", "fastapi.Depends", "fastapi.Path"]Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@routers/audio_effects.py` at line 43, Configure Ruff rather than changing the FastAPI handlers: under [tool.ruff.lint.flake8-bugbear] in pyproject.toml, add fastapi.Body, fastapi.Query, fastapi.Depends, and fastapi.Path to extend-immutable-calls, covering the defaults used by upsert_audio_effect_mapping and the handler at the other reported location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@routers/audio_effects.py`:
- Line 43: Configure Ruff rather than changing the FastAPI handlers: under
[tool.ruff.lint.flake8-bugbear] in pyproject.toml, add fastapi.Body,
fastapi.Query, fastapi.Depends, and fastapi.Path to extend-immutable-calls,
covering the defaults used by upsert_audio_effect_mapping and the handler at the
other reported location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cd519b9-8eff-419e-9217-950b4f84c6f1
📒 Files selected for processing (8)
.dockerignoreCHANGELOG.mdDockerfiledocker-compose.ymldocs/size-exemptions.mdrouters/__init__.pyrouters/audio_effects.pyserver.py
… ships them (#836) The packaged desktop app died at startup: File ".../Resources/slopsmith/server.py", line 71, in <module> import appstate ModuleNotFoundError: No module named 'appstate' feedback-desktop's scripts/bundle-slopsmith.sh copies a HARDCODED list of core files into the bundle -- server.py, VERSION, lib/, data/, static/, plugins/__init__.py. The root-level appstate.py (#833) and routers/ (#834) shipped fine in Docker, passed every test, and were silently dropped from the packaged app. Both now live under lib/, the one core directory every packaging path already copies wholesale -- Dockerfile `COPY lib/`, docker-compose.yml, and the desktop bundler's `cp -r lib` -- and that all three put on sys.path (on Windows via the embeddable-Python ._pth, where PYTHONPATH is ignored: build-windows.sh writes `../slopsmith` and `../slopsmith/lib`). No feedback-desktop change and no new release are needed for this to take effect. lib/ is also the CORRECT home under Principle V, and always was once the design settled: with the injection seam, appstate.py constructs nothing and does no import-time IO, and a route module only builds an APIRouter. The premise that forced root placement -- "appstate opens sqlite at import" -- stopped being true when configure() replaced ownership. The Dockerfile / .dockerignore / docker-compose.yml entries added for the root layout are reverted; nothing else in core changes (git mv, so --follow survives). tests/test_packaging.py is the guard: it walks server.py's module-level imports, keeps the ones resolving inside this repo, and fails if any lives outside a directory the packagers copy -- with the ModuleNotFoundError spelled out. So the next root-level core module can't ship broken. Negative-checked: restoring appstate.py to the root fails it; the message names the file and the four packaging files a root module would have to teach. (It also has to skip `origin in {"built-in","frozen"}` -- on 3.14 the frozen stdlib reports origin="frozen", and Path("frozen").resolve() lands inside the repo, which flagged `os` and `stat` as first-party.) Verified: the bundler's copy replicated exactly into a temp dir and booted with PYTHONPATH=<bundle>:<bundle>/lib -- `import server`, `import appstate`, `import routers.audio_effects` all resolve, appstate.meta_db is server.meta_db, 143 routes. The same simulation against origin/main reproduces the production ModuleNotFoundError. pytest 2398 passed (2348 + 50 new); route table still identical to origin/main (paths, methods, order); docker build context reaches lib/appstate.py and lib/routers/ with no __pycache__; native uvicorn boot smoke serves /api/version, /api/library, the moved audio-effects router, and all three migrated plugins' src/ graphs; eslint 0 errors. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The first route module through the
appstateseam from #833. Move-only, backend-only.Picked by measurement, not by the plan
The plan assumed
artists/aliaseswas the cheapest first router. A transitive dependency-closure scan over every route group says otherwise —api_artist_linksreaches_mb_http_getand_enrich_network_enabled, bothmonkeypatch.setattrtargets (13 + 15 sites). Ranking every group by (setattrtargets, helpers needing relocation):_audio_effects_error(exclusive)audio-effectswins as the first cut: zerosetattrtargets, one exclusive helper, and — unlike the alias group — it exercises the second seam slot (audio_effect_mappings, notmeta_db) and the_demo_mode_guardmiddleware, which has four regexes pointed at these exact paths.What changed
Bodies are verbatim. Two mechanical edits, nothing else:
The singleton read stays a module attribute resolved at call time, so a re-imported
serverre-publishes a fresh DB into the seam andmonkeypatch.setattrreaches this module.routers/never importsserver: the graph isserver → routers → appstate.app.include_router(...)sits exactly where the routes used to be defined. FastAPI matches in registration order, so the mount site is load-bearing.server.py: 9,445 → 9,386 lines.fastapi.Querywent dead with the move and was removed; the other four unused imports are pre-existing onmain.Verification
origin/main: 143 routes, identical paths, methods, and registration order. This is the assertion that matters forinclude_router.404on a missing id →400on a bad body.Query(...)still422s a missing required param.403s all four moved write routes and still allows the read — checked against a running server withFEEDBACK_DEMO_MODE=1.COPY routers/ /app/routers/plus!routers/+!routers/**in.dockerignore(that file opens with a blanket*exclusion, so a new package is invisible to the build without it — this is what would have broken the image). Verified against the real daemon:routers/reaches the build context,__pycache__does not.pytest2348 passed (75 across the audio-effects + demo-mode suites);pyflakesclean onrouters/;npm run lint0 errors.Pattern for the rest of the train
routers/__init__.pydocuments it: expose a module-levelrouter, read singletons asappstate.<name>at call time, neverfrom appstate import …(frozen binding), neverimport server.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Infrastructure
Documentation