fix: sync set.status with actual response status in onAfterResponse#1820
fix: sync set.status with actual response status in onAfterResponse#1820eL1fe wants to merge 2 commits intoelysiajs:mainfrom
Conversation
When a handler returns a Response object with a non-200 status, set.status in onAfterResponse still showed 200. This broke logging and monitoring middleware that relies on set.status. Now set.status is updated from the response before afterResponse hooks run — for both Response objects and ElysiaCustomStatusResponse. Fixes elysiajs#1445
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPropagates HTTP status from returned Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/dynamic-handle.ts (1)
854-858: Consider reordering checks for consistency with compose.ts.The fix correctly syncs
set.statusbefore afterResponse hooks. However, the order ofinstanceofchecks differs fromcompose.ts, which checksElysiaCustomStatusResponsefirst, thenResponse. While this doesn't affect correctness (sinceElysiaCustomStatusResponsedoesn't extendResponse), aligning the order improves maintainability.♻️ Suggested reorder for consistency
- if (context.response instanceof Response) - set.status = context.response.status - else if (context.response instanceof ElysiaCustomStatusResponse) + if (context.response instanceof ElysiaCustomStatusResponse) set.status = (context.response as any).code + else if (context.response instanceof Response) + set.status = context.response.status🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dynamic-handle.ts` around lines 854 - 858, The status-assignment branch in dynamic-handle.ts should check for ElysiaCustomStatusResponse before Response to match compose.ts and improve consistency: in the block where you set set.status from context.response (referencing context.response, ElysiaCustomStatusResponse, Response, and set.status), reorder the instanceof checks so you first test for ElysiaCustomStatusResponse and assign its .code, then fall back to Response and assign .status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dynamic-handle.ts`:
- Around line 854-858: The status-assignment branch in dynamic-handle.ts should
check for ElysiaCustomStatusResponse before Response to match compose.ts and
improve consistency: in the block where you set set.status from context.response
(referencing context.response, ElysiaCustomStatusResponse, Response, and
set.status), reorder the instanceof checks so you first test for
ElysiaCustomStatusResponse and assign its .code, then fall back to Response and
assign .status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 040e63e3-5347-4349-af58-29e4414e4181
📒 Files selected for processing (2)
src/compose.tssrc/dynamic-handle.ts
Summary
set.statusinonAfterResponsealways showed200even when the actual response had a different status code. This broke logging/monitoring middleware.Problem
The handler returns a 401 Response, the client gets 401, but
set.statusinonAfterResponsewas never updated from the response.Changes
src/compose.ts: in the afterResponse codegen, syncc.set.statusfromc.responseValue.statuswhen it's a Response object (route handler + 404 handler paths)src/dynamic-handle.ts: in the finally block, syncset.statusfromcontext.responsebefore running afterResponse hooksTest plan
Responsewith non-200 status →set.statusreflects actual statusstatus()helper → still works correctlyFixes #1445
Summary by CodeRabbit