refactor(api): extract shared session and message helpers for chat routers - #1072
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The session handlers only need the verified session; the source-level verification happens inside get_verified_source_session. Underscore the unused binding in get/update/delete to make that explicit.
7053b0f to
233102e
Compare
|
Fixed in 233102e: the three session handlers (get/update/delete) now unpack |
What
api/routers/chat.pyandapi/routers/source_chat.pywere heavy copy-paste of each other. This extracts the shared pieces into a newapi/routers/_chat_shared.pyand replaces every duplicated site in both routers. No behavior change — same status codes, same response shapes, same error messages (including the known missing-relation-becomes-500 quirk, deliberately preserved as-is for a later error-handling PR).What was deduplicated
normalize_record_id(table, id)— thex if x.startswith("table:") else f"table:{x}"idiom, previously inlined 12 times across both routers (including twice in the same handler:get_sessionandupdate_sessionrecomputedfull_session_ididentically)get_session_or_404/get_source_or_404/get_verified_source_session— the "verify source exists → normalize session id → fetch session → verifyrefers_torelation" block, copy-pasted 4 times in full and 2 times partially insource_chat.py, plus the session-fetch variant repeated 4 times inchat.pyextract_chat_messages— the LangGraph state →ChatMessageconversion loop, written 3 times (2× chat, 1× source_chat; the SSE streaming loop has a different output shape and was left alone)ChatMessageandSuccessResponsepydantic models — previously defined twice, now a single definitionTests-first approach
Since this is a delicate refactor, characterization tests were written first (commit 1) and run green against the unchanged code, then the extraction (commit 2) was verified against them:
refers_torelation → existing behavior pinned as-is: 500 wrapping the inner 404 on get/delete, real 404 on send-message (which re-raisesHTTPException)type/content/positional-id fallbacks and the empty-state pathThe only test change in commit 2 is repointing mock patch targets to the new module.
Line delta
+156 / −265in the refactor commit: the two routers shed 250 lines (chat.py −92, source_chat.py −158) for 94 lines of shared module.Verification
uv run pytest tests/— 415 passed (403 baseline + 12 new)ruff check .— cleanmypy— 197 errors before and after (the 4 pre-existing errors in these two files are unchanged; none in the new module)