refactor(server): extract the playlists routes into routers/playlists.py (R3)

[byrongamatos]

The biggest router yet — 12 playlist routes + custom covers — and the first that needs the config-path seam. server.py: 9,302 → 9,085 (−217).

Three causally-linked pieces (all required for playlists)

1. config_dir joins the appstate seam. The plan always put the path constants in appstate; I deferred them in S3 to dodge the STATIC_DIR retargets, and now need config_dir for playlist covers. It's env-derived, so the ~49 pop-and-reimport fixtures reconfigure it for free — zero setattr retargeting. STATIC_DIR/SLOPPAK_CACHE_DIR (which are patched via setattr) stay in server.py until a router that reads them is extracted, and get retargeted then.
2. _clean_str → lib/reqfields.py. The pure request-field sanitizer (14 callers across playlists/collections/profile/progression/shop/stats). server.py imports it back. Unblocks wanted/saved/collections/… later.
3. routers/playlists.py. Bodies verbatim; @app→@router, meta_db→appstate.meta_db, CONFIG_DIR→appstate.config_dir, _clean_str from reqfields, _ART_CACHE_HEADERS as a local const (art keeps server.py's). The two exclusive cover helpers move with it.

Verification

• Route table identical to origin/main (143, paths/methods/order).
• One test retarget: test_playlists_api called server._playlist_cover_path directly → now imports it from routers.playlists (which reads appstate.config_dir, configured by the server fixture). This is the seam working: the helper resolves the fresh config dir across a pop-and-reimport without the test knowing appstate exists.
• Boot smoke drives create / rename / add-song / cover-upload / serve / delete: the cover writes 1.png under CONFIG_DIR through appstate.config_dir and serves 200 with an mtime cache-bust token; a wrong-typed name field still 400s via _clean_str.
• pytest 2401 passed (28 in playlists+collections+appstate); packaging guard 51 (auto-picked up reqfields); pyflakes clean; npm run lint 0; Codex 0 findings.

Next: wanted/saved now unblock (_clean_str homed). The path-setattr routers (ws_highway @ 902 lines, song, audio-local-path) need STATIC_DIR/SLOPPAK_CACHE_DIR in the seam + their 8 test retargets; library/collections need the provider registry; enrichment needs the network cluster. Those are the remaining substrates.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added playlist management, including creation, editing, deletion, song management, and full reordering.
  • Added custom playlist cover uploads, retrieval, replacement, and removal.
  • Playlist listings now include cover images with automatic cache refresh.
• Bug Fixes
  • Improved handling of missing, trimmed, or incorrectly typed request fields.
  • Added validation for playlist names, types, song data, and reorder requests.
  • System playlists remain protected from renaming and deletion.
• Documentation
  • Updated changelog and size-exemption documentation to reflect current router extraction progress.

[coderabbitai]

📝 Walkthrough

Walkthrough

The playlist endpoints were extracted from server.py into a dedicated router. Shared request coercion and config_dir state were added, custom cover management was implemented, and tests and extraction documentation were updated.

Changes

Playlist router extraction

Layer / File(s)	Summary
Shared state and request helpers lib/appstate.py, lib/reqfields.py	Adds the config_dir application-state slot and centralizes string coercion for raw request fields.
Playlist and song API lib/routers/playlists.py	Adds playlist CRUD, song management, curated-album slot updates, and exact-permutation reorder validation.
Custom playlist covers lib/routers/playlists.py	Adds resized PNG upload, retrieval, cache-busted URLs, and deletion using the configured cover directory.
Router wiring and validation updates server.py, tests/test_playlists_api.py, docs/size-exemptions.md, CHANGELOG.md	Mounts the playlist router, passes config_dir through app state, updates cover-path tests, and revises extraction documentation.
Estimated code review effort: 3 (Moderate)	~20 minutes

Possibly related PRs

• got-feedBack/feedBack#833: Introduces the related appstate router-seam mechanism extended here.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: extracting playlist routes from server.py into routers/playlists.py.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/r3-router-playlists

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@lib/routers/playlists.py`:
- Around line 98-105: Serialize playlist cover upload and deletion through a
shared per-playlist critical section, including the delete flow and the upload
logic around the existence check and file write. Re-check playlist existence
while holding the lock before committing an upload, and ensure cover cleanup
occurs under the same lock. Replace the shared `{pid}.png.tmp` path with a
unique temporary file in the cover’s directory, preserving atomic
same-filesystem replacement and cleanup on failure.
- Around line 214-215: The broad exception handler around cover image processing
incorrectly treats persistence failures as client errors and exposes filesystem
details. In the cover validation and save flow, catch decode/validation
exceptions separately and return the existing-style 400 response only for those;
handle failures from img.save(...) and tmp.replace(...) by logging the exception
server-side and returning a generic 500 response without exposing error details.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6373e736-d162-4018-a059-127313ef296c

📥 Commits

Reviewing files that changed from the base of the PR and between 4fd0cd4 and 7cbbb26.

📒 Files selected for processing (7)

• CHANGELOG.md
• docs/size-exemptions.md
• lib/appstate.py
• lib/reqfields.py
• lib/routers/playlists.py
• server.py
• tests/test_playlists_api.py

[byrongamatos]

Both cover findings are real and both are pre-existing — the flagged lines (tmp = cover.with_suffix(".png.tmp"), except Exception as e: … status_code=400, the get_playlist(pid) existence check) are byte-identical to origin/main:server.py. This PR moved the cover handlers verbatim, so fixing them here would break the move-only contract.

Handling them in a follow-up, same as the loops races on #839 → #840:

• Minor / info-leak (400 vs 500, leaking e): worth fixing cleanly — split PIL decode/validation (400) from img.save/tmp.replace failures (log + generic 500, no filesystem detail). Small and safe.
• Major / cover lifecycle race + shared .tmp: I'll do the cheap, substantive parts — a unique same-dir temp filename and an existence re-check immediately before the atomic replace. A full per-playlist critical section is overkill here: FeedBack is single-user (Principle I), so a cover upload racing a playlist delete on the same id essentially can't happen. I'll document that rationale rather than add a locking framework for a race the deployment model precludes.

Follow-up PR incoming; will link it and resolve these threads.

[byrongamatos]

Follow-up fix opened: #842 — splits cover decode (400) from persist (500, logged, no leak), unique temp file + existence re-check. Both findings addressed; negative-checked 3 ways.