Conversation
WalkthroughAdded CORS preflight support to the public stats endpoint by introducing a new OPTIONS handler and CORS headers constant. Updated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
echo/server/dembrane/api/stats.py (1)
243-248:⚠️ Potential issue | 🔴 CriticalBug: 503 response is missing CORS headers — browser clients will get an opaque error.
This is the one path that doesn't return a
JSONResponsewith_PUBLIC_CORS_HEADERS. When the browser receives a 503 withoutAccess-Control-Allow-Origin, the fetch API will surface a generic network error instead of the 503 +Retry-After. Defeats the purpose of this PR for the unhappy path.🐛 Proposed fix — return JSONResponse instead of raising HTTPException
# Lock holder likely failed — return 503 instead of stampeding Directus logger.warning("Stats computation timed out waiting for lock holder") - raise HTTPException( - status_code=503, - detail="Stats temporarily unavailable. Try again shortly.", - headers={"Retry-After": "5"}, + return JSONResponse( + status_code=503, + content={"detail": "Stats temporarily unavailable. Try again shortly."}, + headers={**_PUBLIC_CORS_HEADERS, "Retry-After": "5"}, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/stats.py` around lines 243 - 248, The 503 error path currently raises HTTPException (logger.warning("Stats computation timed out waiting for lock holder") then raise HTTPException(...)), which omits CORS headers and breaks browser clients; replace the raise with returning a fastapi.responses.JSONResponse containing the same detail message and status_code=503 and set headers by merging _PUBLIC_CORS_HEADERS with {"Retry-After": "5"} so the response includes Access-Control-Allow-Origin and the Retry-After header; update the lock-timeout branch in stats.py to return that JSONResponse instead of raising HTTPException and keep the existing logger.warning call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@echo/server/dembrane/api/stats.py`:
- Around line 191-196: The code defines a hand-rolled _PUBLIC_CORS_HEADERS
constant which is fragile—replace this by enabling FastAPI's CORSMiddleware for
the app or for the specific sub-application/router that serves the endpoints in
this module so CORS and preflight are applied to every response (including
errors) automatically; if you cannot add middleware at the app level, create a
sub-app with FastAPI(), mount the router there, and add
fastapi.middleware.cors.CORSMiddleware with allow_origins=["*"],
allow_methods=["GET","OPTIONS"], allow_headers=["*"], max_age=86400 instead of
using _PUBLIC_CORS_HEADERS, or as a fallback ensure every response path and
error handler (e.g., the 503 path mentioned) explicitly merges
_PUBLIC_CORS_HEADERS into the Response headers.
- Around line 199-202: The preflight handler stats_preflight currently returns a
JSONResponse with content=None which yields a 200/JSON null body; change it to
return an empty 204 No Content response instead (use
starlette.responses.Response or FastAPI Response) with status_code=204 and the
same _PUBLIC_CORS_HEADERS so the endpoint sends no body; update the return in
the StatsRouter.options("/") handler (stats_preflight) accordingly.
---
Outside diff comments:
In `@echo/server/dembrane/api/stats.py`:
- Around line 243-248: The 503 error path currently raises HTTPException
(logger.warning("Stats computation timed out waiting for lock holder") then
raise HTTPException(...)), which omits CORS headers and breaks browser clients;
replace the raise with returning a fastapi.responses.JSONResponse containing the
same detail message and status_code=503 and set headers by merging
_PUBLIC_CORS_HEADERS with {"Retry-After": "5"} so the response includes
Access-Control-Allow-Origin and the Retry-After header; update the lock-timeout
branch in stats.py to return that JSONResponse instead of raising HTTPException
and keep the existing logger.warning call.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
echo/server/dembrane/api/stats.py
| _PUBLIC_CORS_HEADERS = { | ||
| "Access-Control-Allow-Origin": "*", | ||
| "Access-Control-Allow-Methods": "GET, OPTIONS", | ||
| "Access-Control-Allow-Headers": "*", | ||
| "Access-Control-Max-Age": "86400", | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using FastAPI's CORSMiddleware instead of hand-rolling headers.
Manual CORS is fragile — every response path (including error handlers) needs these headers or browsers will block the response. FastAPI's built-in CORSMiddleware handles this automatically, including error responses and preflight. If you only need CORS on this single router, you can apply the middleware selectively or use a sub-application.
That said, if there's a deliberate reason to keep it manual (e.g., no middleware access at this layer), then the immediate fix is ensuring all response paths include these headers — see the 503 comment below.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/stats.py` around lines 191 - 196, The code defines a
hand-rolled _PUBLIC_CORS_HEADERS constant which is fragile—replace this by
enabling FastAPI's CORSMiddleware for the app or for the specific
sub-application/router that serves the endpoints in this module so CORS and
preflight are applied to every response (including errors) automatically; if you
cannot add middleware at the app level, create a sub-app with FastAPI(), mount
the router there, and add fastapi.middleware.cors.CORSMiddleware with
allow_origins=["*"], allow_methods=["GET","OPTIONS"], allow_headers=["*"],
max_age=86400 instead of using _PUBLIC_CORS_HEADERS, or as a fallback ensure
every response path and error handler (e.g., the 503 path mentioned) explicitly
merges _PUBLIC_CORS_HEADERS into the Response headers.
| @StatsRouter.options("/") | ||
| async def stats_preflight() -> JSONResponse: | ||
| """Handle CORS preflight for the public stats endpoint.""" | ||
| return JSONResponse(content=None, headers=_PUBLIC_CORS_HEADERS) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: preflight responses conventionally use 204 No Content.
content=None serializes to a JSON null body with a 200 status. The canonical preflight response is 204 with no body. Not a correctness issue — browsers don't care — but it's cleaner.
✨ Proposed tweak
`@StatsRouter.options`("/")
async def stats_preflight() -> JSONResponse:
"""Handle CORS preflight for the public stats endpoint."""
- return JSONResponse(content=None, headers=_PUBLIC_CORS_HEADERS)
+ return JSONResponse(content=None, status_code=204, headers=_PUBLIC_CORS_HEADERS)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @StatsRouter.options("/") | |
| async def stats_preflight() -> JSONResponse: | |
| """Handle CORS preflight for the public stats endpoint.""" | |
| return JSONResponse(content=None, headers=_PUBLIC_CORS_HEADERS) | |
| `@StatsRouter.options`("/") | |
| async def stats_preflight() -> JSONResponse: | |
| """Handle CORS preflight for the public stats endpoint.""" | |
| return JSONResponse(content=None, status_code=204, headers=_PUBLIC_CORS_HEADERS) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/stats.py` around lines 199 - 202, The preflight
handler stats_preflight currently returns a JSONResponse with content=None which
yields a 200/JSON null body; change it to return an empty 204 No Content
response instead (use starlette.responses.Response or FastAPI Response) with
status_code=204 and the same _PUBLIC_CORS_HEADERS so the endpoint sends no body;
update the return in the StatsRouter.options("/") handler (stats_preflight)
accordingly.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes