Skip to content

refactor(server): extract the loops routes into routers/loops.py (R3) - #839

Merged
byrongamatos merged 1 commit into
mainfrom
refactor/r3-router-loops
Jul 10, 2026
Merged

refactor(server): extract the loops routes into routers/loops.py (R3)#839
byrongamatos merged 1 commit into
mainfrom
refactor/r3-router-loops

Conversation

@byrongamatos

@byrongamatos byrongamatos commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Third router in the R3 train (after audio-effects #834, artist-aliases #838). Move-only.

Practice loops — saved A/B regions per song. router_scan.py ranks it at 0 setattr targets, 0 helpers to relocate; meta_db-only, own banner, contiguous.

  • GET /api/loops · POST /api/loops · DELETE /api/loops/{loop_id}

Bodies verbatim; @app@router, meta_dbappstate.meta_db. include_router at the original site. server.py: 9,337 → 9,302.

Verification

  • Route table identical to origin/main (143, paths/methods/order).
  • No test retargeting (test_demo_mode only names the paths in middleware regexes).
  • Boot smoke: POST auto-names Loop N, GET reads back, DELETE removes, missing fields → error; demo mode 403s both writes, allows the read.
  • pytest 2398 passed; packaging guard green; pyflakes clean; npm run lint 0 errors; Codex 0 findings.

Next candidates (router_scan.py): wanted/saved need _clean_str (shared by 10 callers) relocated to a lib/ home first — a small shared-helpers decision before those move.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added API support for listing, saving, and deleting audio loops.
    • Automatically assigns a default name when saving an unnamed loop.
    • Loop results are ordered by their start time.
  • Documentation

    • Updated release notes and project size documentation to reflect router extraction progress.

Third router. Practice loops (saved A/B regions per song): GET/POST/DELETE
/api/loops, meta_db-only (0 setattr targets, 0 helpers to relocate per
router_scan.py). Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db.
include_router at the original site; 143-route table identical to origin/main.

server.py: 9,337 -> 9,301.

No test retargeting (test_demo_mode only names the paths in middleware regexes).
Verified: pyflakes clean; route table identical; pytest 2398 passed; packaging
guard green; eslint 0; boot smoke drives POST (auto-names "Loop N") / GET / DELETE
/ missing-fields error, and demo mode 403s both writes while allowing the read.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The Loops API was extracted from server.py into lib/routers/loops.py, preserving its three endpoints and database behavior. server.py now mounts the router, and related changelog and size-exemption documentation were updated.

Changes

Loops router extraction

Layer / File(s) Summary
Implement Loops router
lib/routers/loops.py
Adds GET, POST, and DELETE loop endpoints using appstate.meta_db, including validation, default naming, persistence, ordering, and deletion.
Mount router and update progress records
server.py, CHANGELOG.md, docs/size-exemptions.md
Replaces the inlined Loops endpoints with router registration and updates documented extraction progress and line counts.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: extracting the loops routes from server.py into routers/loops.py.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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-loops

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/loops.py`:
- Around line 18-21: The loops list query currently accesses the shared SQLite
connection without synchronization. In the relevant list handler, wrap both the
`appstate.meta_db.conn.execute(...)` call and its `.fetchall()` in `with
appstate.meta_db._lock:` so the entire read is protected from concurrent
POST/DELETE transactions.
- Around line 34-43: Make loop name generation atomic in the unnamed-loop
handling code: acquire appstate.meta_db._lock before executing the COUNT query,
then keep the same lock held through name assignment, INSERT, and commit.
Consolidate the existing lock scopes so the count and persistence occur within
one critical section.
🪄 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: cec452fc-86f7-41c5-9684-f4bde2cbbe31

📥 Commits

Reviewing files that changed from the base of the PR and between 6c98aba and b9c3565.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • docs/size-exemptions.md
  • lib/routers/loops.py
  • server.py

Comment thread lib/routers/loops.py
Comment thread lib/routers/loops.py
@byrongamatos
byrongamatos merged commit b41361e into main Jul 10, 2026
4 checks passed
@byrongamatos

Copy link
Copy Markdown
Contributor Author

Both findings are real, and both are pre-existing — the COUNT(*) sat outside _lock and list_loops' SELECT was unlocked on origin/main too (verified byte-identical to 6c98aba:server.py). This PR moved the code verbatim, so they correctly weren't part of a move-only change. Fixing them here would have made the extraction non-verbatim.

Landing both in a small follow-up: move the COUNT(*) inside the with _lock block (atomic name generation) and wrap the list read in the lock. Severity is low in context — FeedBack is single-user (Principle I), so two concurrent unnamed-loop POSTs essentially can't happen — but the fix is one line each, so it's worth doing. Follow-up PR incoming; will link it here.

@byrongamatos

Copy link
Copy Markdown
Contributor Author

Follow-up fix opened: #840 — locks the count+insert and the list read, with a barrier-based concurrency test (negative-checked 5/5).

byrongamatos added a commit that referenced this pull request Jul 10, 2026
…840)

Two pre-existing races in the loops routes, flagged by CodeRabbit on #839 (the
verbatim extraction moved them unchanged from server.py, so they correctly
weren't fixed there):

1. save_loop computed `COUNT(*)` OUTSIDE meta_db._lock, then inserted inside it.
   Two simultaneous unnamed POSTs read the same count and both mint "Loop N".
   Fix: one lock scope around COUNT + INSERT.
2. list_loops read the shared single connection (check_same_thread=False) with
   no lock, so it could overlap a POST/DELETE commit. Fix: read under the lock,
   like every writer.

Low severity in context — FeedBack is single-user (Principle I), so concurrent
unnamed-loop POSTs essentially can't happen — but each fix is one lock scope.

tests/test_loops_concurrency.py pins both with a threading.Barrier that releases
16 workers into save_loop at once. Negative-checked: reverting the COUNT back
outside the lock fails the uniqueness assertion 5/5 runs; the fix passes 3/3.
pytest 2400 passed; on-device two unnamed POSTs -> ['Loop 1','Loop 2'].

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant