improve general error handling @renderToStream - #15213
Conversation
…ove edge case error handling when destroy error happens.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe stream renderer documents the ChangesStream error handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🧹 Nitpick comments (1)
packages/server-renderer/src/renderToStream.ts (1)
211-215: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the destruction path.
The existing
packages/server-renderer/__tests__/webStream.spec.ts:41-66test covers only successful output. Add a case that triggersdestroy(err)and verifies thatwriter.abort(err)receives the same error,writer.close()is not called, and a rejected abort does not escape.🤖 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 `@packages/server-renderer/src/renderToStream.ts` around lines 211 - 215, Add regression coverage for the destroy(err) path in the web stream tests, exercising the stream implementation around destroy and writer.abort. Verify the original error is passed to writer.abort, writer.close is not called, and a rejected abort is swallowed without escaping.
🤖 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 `@packages/server-renderer/src/renderToStream.ts`:
- Around line 18-22: Update the exported SimpleReadable.destroy method signature
to accept unknown instead of any, keeping its existing void return type and
behavior unchanged.
---
Nitpick comments:
In `@packages/server-renderer/src/renderToStream.ts`:
- Around line 211-215: Add regression coverage for the destroy(err) path in the
web stream tests, exercising the stream implementation around destroy and
writer.abort. Verify the original error is passed to writer.abort, writer.close
is not called, and a rejected abort is swallowed without escaping.
🪄 Autofix
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: 736cb460-d796-4a6a-b8cc-d86b5ecb1f76
📒 Files selected for processing (1)
packages/server-renderer/src/renderToStream.ts
| /** | ||
| Call destroy when an error occurred | ||
| @param error - error that happened. Typed as unknown. | ||
| */ | ||
| destroy(error: any): void |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\bSimpleReadable\b|\bdestroy\s*\(' packages --glob '*.ts'Repository: vuejs/core
Length of output: 10210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,220p' packages/server-renderer/src/renderToStream.ts | nl -ba -v60Repository: vuejs/core
Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/server-renderer/src/renderToStream.ts")
lines = p.read_text(encoding="utf-8").splitlines()
for i in range(59, 220):
print(f"{i+1}: {lines[i]}")
PYRepository: vuejs/core
Length of output: 4822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("packages/server-renderer/src/renderToStream.ts")
lines = p.read_text(encoding="utf-8").splitlines()
for i in range(1, 60):
print(f"{i:03d}: {lines[i-1]}")
PYRepository: vuejs/core
Length of output: 1900
Align SimpleReadable.destroy with its documented type.
destroy is documented as receiving unknown, but the exported interface declares any, allowing callers to pass any destruction error without narrowing. Use unknown in the signature.
Proposed fix
- destroy(error: any): void
+ destroy(error: unknown): void📝 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.
| /** | |
| Call destroy when an error occurred | |
| @param error - error that happened. Typed as unknown. | |
| */ | |
| destroy(error: any): void | |
| /** | |
| Call destroy when an error occurred | |
| `@param` error - error that happened. Typed as unknown. | |
| */ | |
| destroy(error: unknown): void |
🤖 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 `@packages/server-renderer/src/renderToStream.ts` around lines 18 - 22, Update
the exported SimpleReadable.destroy method signature to accept unknown instead
of any, keeping its existing void return type and behavior unchanged.
update(webStream.spec.ts): Add regression coverage for the destruction path. (code rabbit)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server-renderer/__tests__/webStream.spec.ts (1)
88-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the destruction diagnostic.
Both tests replace
console.errorbut do not verify its call.toHaveBeenWarned()verifies the render warning. It does not verify the newpipeToWebWritablelog. Assert thatconsole.errorreceives the destruction message and the original render error.Proposed fix
expect(abortMock).toHaveBeenCalledWith(error) + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error while destroying stream: ', + error, + ) expect(closeMock).not.toHaveBeenCalled()Also applies to: 126-139
🤖 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 `@packages/server-renderer/__tests__/webStream.spec.ts` around lines 88 - 103, Update the affected tests around pipeToWebWritable to assert consoleErrorSpy was called with the destruction diagnostic message and the original render error, while retaining the existing toHaveBeenWarned assertion for the render warning. Apply the same assertion to both test cases that mock console.error.
🤖 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 `@packages/server-renderer/__tests__/webStream.spec.ts`:
- Around line 15-26: Update the populate function’s recursive reader.read flow
to decode each chunk with the TextDecoder streaming option enabled, preserving
incomplete UTF-8 sequences across chunk boundaries, and flush the decoder with a
final decode after done before returning res.
---
Nitpick comments:
In `@packages/server-renderer/__tests__/webStream.spec.ts`:
- Around line 88-103: Update the affected tests around pipeToWebWritable to
assert consoleErrorSpy was called with the destruction diagnostic message and
the original render error, while retaining the existing toHaveBeenWarned
assertion for the render warning. Apply the same assertion to both test cases
that mock console.error.
🪄 Autofix
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: 864c7675-f422-4b19-b497-da9fe660de3c
📒 Files selected for processing (2)
packages/server-renderer/__tests__/webStream.spec.tspackages/server-renderer/src/renderToStream.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server-renderer/src/renderToStream.ts
…efore the next chunk
2 participants
SimpleReadableSummary by CodeRabbit