fix: #255 handle Transfer-Encoding: chunked - #256
Conversation
WalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Honestly, you really needed to spell out that 🚥 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/treaty2/index.ts`:
- Around line 591-599: Current code gates streaming by Transfer-Encoding header
which is a transport detail; update the condition so streamResponse(...) is only
used when the Content-Type explicitly denotes a streaming format (e.g.
'text/event-stream', 'application/ndjson'/'application/x-ndjson',
'application/stream+json', or other agreed streaming media types) or when the
caller explicitly opts in (e.g. a config flag like config.expectStream or
config.stream === true). Modify the check around
response.headers.get('Transfer-Encoding') and contentType in the block that
calls streamResponse to instead validate contentType against the allowed
streaming types or check the explicit config opt-in, leaving normal content
types (application/json, multipart/form-data, application/octet-stream) to be
processed by the regular parsers; keep use of streamResponse and
parseStringifiedValue unchanged but only reachable via the new explicit
criteria.
- Around line 628-638: The switch case handling 'multipart/form-data' declares
const temp directly and triggers the noSwitchDeclarations lint rule; wrap the
entire case body in braces so the const temp (result of await
response.formData()) is block-scoped, then populate data by iterating
temp.forEach(...) as before (refer to the case 'multipart/form-data', temp,
response.formData(), and data assignment). After adding the braces, rerun
Biome/CI.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd769c23-bc7a-440d-97c4-c6b070f4ba7f
📒 Files selected for processing (1)
src/treaty2/index.ts
| if ( | ||
| response.headers.get('Transfer-Encoding') === | ||
| 'chunked' && | ||
| contentType !== 'text/event-stream' | ||
| ) { | ||
| data = streamResponse(response, { | ||
| parseDate: config.parseDate, | ||
| sse: false | ||
| }) |
There was a problem hiding this comment.
Don’t key streaming semantics off Transfer-Encoding, baka~
Transfer-Encoding: chunked is transport framing, not an application-level streaming contract. This branch makes ordinary chunked application/json, multipart/form-data, and application/octet-stream responses bypass their normal parsers and go through streamResponse() instead. Then streamResponse() feeds each raw chunk into parseStringifiedValue() (see src/utils/parse.ts, Lines 55-76), which only makes sense for complete values, not arbitrary HTTP chunk boundaries. Result: callers can suddenly get an async iterator or mangled chunk fragments for perfectly normal responses. Gate this on an explicit streaming format or caller opt-in instead. (¬‿¬)♡
Suggested fix
- if (
- response.headers.get('Transfer-Encoding') ===
- 'chunked' &&
- contentType !== 'text/event-stream'
- ) {
- data = streamResponse(response, {
- parseDate: config.parseDate,
- sse: false
- })
- } else {
- switch (contentType) {
+ switch (contentType) {
case 'text/event-stream':
data = streamResponse(response, {
parseDate: config.parseDate
})
break
@@
- }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/treaty2/index.ts` around lines 591 - 599, Current code gates streaming by
Transfer-Encoding header which is a transport detail; update the condition so
streamResponse(...) is only used when the Content-Type explicitly denotes a
streaming format (e.g. 'text/event-stream',
'application/ndjson'/'application/x-ndjson', 'application/stream+json', or other
agreed streaming media types) or when the caller explicitly opts in (e.g. a
config flag like config.expectStream or config.stream === true). Modify the
check around response.headers.get('Transfer-Encoding') and contentType in the
block that calls streamResponse to instead validate contentType against the
allowed streaming types or check the explicit config opt-in, leaving normal
content types (application/json, multipart/form-data, application/octet-stream)
to be processed by the regular parsers; keep use of streamResponse and
parseStringifiedValue unchanged but only reachable via the new explicit
criteria.
| case 'multipart/form-data': | ||
| const temp = | ||
| (await response.formData()) as FormData | ||
|
|
||
| default: | ||
| data = await response.text().then((text) => | ||
| parseStringifiedValue(text, { | ||
| parseDate: config.parseDate | ||
| data = {} | ||
| temp.forEach((value, key) => { | ||
| // @ts-ignore | ||
| data[key] = value | ||
| }) | ||
| ) | ||
|
|
||
| break |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines
if [ -f "src/treaty2/index.ts" ]; then
wc -l src/treaty2/index.ts
echo "---"
sed -n '620,650p' src/treaty2/index.ts
else
echo "File not found, searching for it..."
find . -name "index.ts" -path "*/treaty2/*" 2>/dev/null
fiRepository: elysiajs/eden
Length of output: 1344
🌐 Web query:
Biome noSwitchDeclarations rule lint configuration
💡 Result:
The Biome linter rule "noSwitchDeclarations" (in the "correctness" group) disallows lexical declarations (const, let, function, class) in switch clauses because they are accessible across the entire switch statement but only initialized if their clause is reached, leading to potential bugs. Configuration in biome.json: { "linter": { "rules": { "correctness": { "noSwitchDeclarations": "error" } } } } It suggests wrapping declarations in a block {} to scope them to the clause. The rule is fixable (auto-wraps in blocks) and sources from ESLint's no-case-declarations.
Citations:
- 1: https://biomejs.dev/linter/rules/no-switch-declarations/
- 2: https://github.com/biomejs/biome/blob/main/crates/biome_js_analyze/src/lint/correctness/no_switch_declarations.rs
- 3: https://biomejs.dev/linter/javascript/rules/
Wrap this case in braces, you stubborn dweeb~
The bare const temp declaration is gonna trigger Biome's noSwitchDeclarations rule and block your CI, yoouuu~ Just slap some braces around it and problem solved, easy peasyyy~ Then rerun Biome like a good dev (๑•́ ω •̀)و
Tiny fix
- case 'multipart/form-data':
- const temp =
- (await response.formData()) as FormData
-
- data = {}
- temp.forEach((value, key) => {
- // `@ts-ignore`
- data[key] = value
- })
-
- break
+ case 'multipart/form-data': {
+ const temp =
+ (await response.formData()) as FormData
+
+ data = {}
+ temp.forEach((value, key) => {
+ // `@ts-ignore`
+ data[key] = value
+ })
+
+ break
+ }🧰 Tools
🪛 Biome (2.4.9)
[error] 629-630: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/treaty2/index.ts` around lines 628 - 638, The switch case handling
'multipart/form-data' declares const temp directly and triggers the
noSwitchDeclarations lint rule; wrap the entire case body in braces so the const
temp (result of await response.formData()) is block-scoped, then populate data
by iterating temp.forEach(...) as before (refer to the case
'multipart/form-data', temp, response.formData(), and data assignment). After
adding the braces, rerun Biome/CI.
1 participant
eden wasn't properly handling non-sse yields. this (hopefully) fixes that. please let me know if i've made any regressions; the test suite isn't properly working on my machine
closes #255
Summary by CodeRabbit
Release Notes