refactor(server): extract the highway WebSocket into routers/ws_highway.py (R3) - #844
Merged
Merged
Conversation
…ay.py (R3)
The single largest handler in server.py — the 902-line /ws/highway/{filename}
chart streamer — plus its 3 exclusive helpers (_pick_smart_arrangement,
_sanitize_authors, _sanitized_song_offset). server.py: 9,008 -> 8,003 (-1,005,
the biggest single R3 cut).
Clean move despite the size: the handler's only server-module deps are the 4
path constants + 3 exclusive helpers + app + log. Everything else it uses is
either NESTED inside the handler (_evict_audio_cache, _fill_scale_degree,
_manifest_entries, _tone_names, _xml_rank, _send_keepalives) or imported from
the shared lib modules (song/audio/sloppak/drums/notation/dlc_paths/metadata_db).
- Path constants read through the appstate seam: added static_dir /
sloppak_cache_dir / audio_cache_dir slots (config_dir already there);
server.py configures them. Bodies otherwise verbatim (@app.websocket ->
@router.websocket, PATHS -> appstate.*, log -> module logger).
- sloppak_cache_dir IS setattr-patched, so the 3 test_highway_ws_* suites now
also `setattr(appstate, "sloppak_cache_dir", ...)` next to their existing
server patch. _sanitize_authors unit tests import it from routers.ws_highway
(it moved). No other test churn.
- Removed 23 now-dead imports from server.py (song/audio/drums/notation/
bisect/contextvars/structlog/WebSocket*/_arr_smart_sort_key) — diffed against
the origin/main unused-import baseline so only NEWLY-dead ones went.
owns_tmp (assigned, never read) moved verbatim — it's pre-existing dead on
origin/main too; left as-is to keep the move faithful.
Verified: route table identical to origin/main (143, paths/methods/order);
handler body verbatim spot-checked; pyflakes clean (server has no new
undefined/dead); pytest 2400 passed; packaging guard 53; eslint 0. Boot smoke:
the highway WS streams the full chart (song_info/beats/sections/notes/chords/
notation/anchors/drum_tab -> ready) BYTE-for-byte the same message sequence as
origin/main across arrangements 0/1/2, zero tracebacks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe highway WebSocket implementation moves from ChangesHighway WebSocket router extraction
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant highway_ws
participant appstate
participant DLCSource
WebSocketClient->>highway_ws: Connect to /ws/highway/{filename}
highway_ws->>appstate: Read configured directories
highway_ws->>DLCSource: Load source and select arrangement
DLCSource-->>highway_ws: Return song and audio data
highway_ws-->>WebSocketClient: Stream song_info and playback data
highway_ws-->>WebSocketClient: Send ready
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/routers/ws_highway.py (2)
214-217: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the dead archive branches
lib/routers/ws_highway.py:217, 451-467, 516— any non-sloppak/non-loose song already raises at line 217, so thenot is_slop and not is_looseaudio path and"format": "archive"case are unreachable. Drop the stale branch and format value.🤖 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 `@lib/routers/ws_highway.py` around lines 214 - 217, The unsupported-format guard already rejects non-sloppak and non-loose songs, so remove the unreachable archive handling. Delete the `not is_slop and not is_loose` audio branch and remove the `"format": "archive"` value from the relevant response logic in `ws_highway.py`, while preserving sloppak and loose-folder behavior.
663-701: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse defusedxml for the song XML walkers here and below
User-supplied song XML is parsed with stdlibxml.etree.ElementTree; switch theseET.parse(...)calls todefusedxml.ElementTree.parse(...)(or another safe parser) to avoid entity-expansion DoS on malicious content.🤖 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 `@lib/routers/ws_highway.py` around lines 663 - 701, Replace the stdlib XML parser import and every song XML parsing call in this section and the related XML walkers with defusedxml.ElementTree, ensuring calls such as ET.parse in the lyrics-loading logic use the safe parser while preserving existing traversal and error handling.Source: Linters/SAST tools
🤖 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/ws_highway.py`:
- Around line 585-660: Wrap the full streaming/send sequence in the WebSocket
handler with a dedicated outer try/except WebSocketDisconnect that returns
quietly, covering sends such as beats, sections, keys, tempos, time_signatures,
anchors, chord_templates, lyrics, tone_changes, notes, chords, handshapes,
phrases, and ready. Place this handler before the existing blanket except
Exception block, while retaining the existing local handling in the drum and
notation sections.
---
Nitpick comments:
In `@lib/routers/ws_highway.py`:
- Around line 214-217: The unsupported-format guard already rejects non-sloppak
and non-loose songs, so remove the unreachable archive handling. Delete the `not
is_slop and not is_loose` audio branch and remove the `"format": "archive"`
value from the relevant response logic in `ws_highway.py`, while preserving
sloppak and loose-folder behavior.
- Around line 663-701: Replace the stdlib XML parser import and every song XML
parsing call in this section and the related XML walkers with
defusedxml.ElementTree, ensuring calls such as ET.parse in the lyrics-loading
logic use the safe parser while preserving existing traversal and error
handling.
🪄 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: 551014c7-2344-43dd-8203-7b2ec6bfa642
📒 Files selected for processing (8)
CHANGELOG.mddocs/size-exemptions.mdlib/appstate.pylib/routers/ws_highway.pyserver.pytests/test_highway_ws_authors.pytests/test_highway_ws_instrument_routing.pytests/test_highway_ws_notation.py
Contributor
Author
|
Real and pre-existing: the inner Follow-up incoming (same pattern as the loops/playlists follow-ups): add |
Contributor
Author
|
Follow-up fix opened: #845 — adds |
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
* fix(ws_highway): swallow a mid-stream WebSocketDisconnect quietly Pre-existing (byte-identical to origin/main), flagged by CodeRabbit on #844: the outer try's only guard was `except Exception as e: log.exception("highway_ws unhandled error")`. The inner `except WebSocketDisconnect` covers ONLY the post-`ready` keep-alive loop, and the drum_tab/notation sends have their own localized guards — but the ~13 other streamed sends (beats, sections, notes, chords, anchors, …) fall through to the blanket handler. Since WebSocketDisconnect is an Exception subclass, a routine tab-close mid-load was logged as an error at whichever send was in flight. Fix: a dedicated `except WebSocketDisconnect: return` before the blanket handler, matching the two localized guards and the lib/ coding guideline. tests/test_ws_highway_disconnect.py drives highway_ws with a fake websocket that raises WebSocketDisconnect on the first streamed send (`loading`), over a real minimal sloppak. Negative-checked: removing the guard makes it fail (the disconnect is logged as `highway_ws unhandled error`); the fix passes. Uses a raw handler on `feedBack.server` rather than caplog, since configure_logging() reroutes that logger through structlog where caplog doesn't observe it. Full suite green. Closes the review thread on #844. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ws_highway): assert streaming stops after disconnect (exactly one send) — CodeRabbit The stub now raises only on the first send and the test asserts ws.sends == 1, so a handler that caught the disconnect and kept streaming would fail. Still negative-checked: removing the guard fails the test. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The single largest handler in
server.py— the 902-line/ws/highway/{filename}chart streamer — plus its 3 exclusive helpers.server.py: 9,008 → 8,003 (−1,005, the biggest single R3 cut).Why it's a clean move despite the size
The handler's only server-module dependencies are: the 4 path constants, 3 exclusive helpers (
_pick_smart_arrangement,_sanitize_authors,_sanitized_song_offset),app, andlog. Everything else it touches is either nested inside the handler (_evict_audio_cache,_fill_scale_degree,_manifest_entries,_tone_names,_xml_rank,_send_keepalives) or imported from the shared lib modules (song,audio,sloppak,drums,notation,dlc_paths,metadata_db). No shared server.py function is called.What changed
static_dir/sloppak_cache_dir/audio_cache_dirslots (config_diralready there);server.pyconfigures them. Otherwise verbatim —@app.websocket→@router.websocket,PATHS→appstate.*,log→ module logger.sloppak_cache_dirissetattr-patched, so the 3test_highway_ws_*suites now alsosetattr(appstate, "sloppak_cache_dir", …)next to their existingserverpatch. The_sanitize_authorsunit tests import it fromrouters.ws_highway(it moved). No other test churn.server.py— diffed against theorigin/mainunused-import baseline so only newly-dead ones went (song/audio/drums/notation/bisect/contextvars/structlog/WebSocket*/_arr_smart_sort_key).owns_tmp(assigned, never read) moved verbatim — pre-existing dead onorigin/maintoo; left as-is to keep the move faithful.Verification
origin/main(143, paths/methods/order); handler body verbatim spot-checked.pyflakesclean (server has no new undefined/dead);pytest2400 passed; packaging guard 53;npm run lint0.song_info/beats/sections/notes/chords/notation/anchors/drum_tab→ready) with the same message sequence asorigin/mainacross arrangements 0/1/2, zero tracebacks. (Confirmed against a side-by-sideorigin/mainworktree.)🤖 Generated with Claude Code
Summary by CodeRabbit