fix(dev): proxy websocket upgrades for devProxy routes with ws - #4480
fix(dev): proxy websocket upgrades for devProxy routes with ws#4480danielroe wants to merge 1 commit into
devProxy routes with ws#4480Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChangesNitroDevApp now routes WebSocket upgrades through matching Dev proxy WebSocket handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
![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 (3)
test/unit/dev-proxy-ws.test.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an options object for request options.
Change
upgradeRequest(port, path)to accept{ path }as its second argument and update the call sites. As per coding guidelines, “For multi-argument functions, use an options object as the second parameter.”🤖 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 `@test/unit/dev-proxy-ws.test.ts` at line 33, Update upgradeRequest to accept an options object containing path as its second parameter instead of a separate path argument, then revise every call site to pass that object while preserving the existing request behavior.Source: Coding guidelines
src/dev/app.ts (2)
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an options object for
proxyUpgrade.Change the new positional
socket, headparameters to a second options object, then update callers insrc/dev/server.tsand the test. As per coding guidelines, “For multi-argument functions, use an options object as the second parameter.”🤖 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 `@src/dev/app.ts` at line 121, Update proxyUpgrade to accept an options object as its second parameter containing the current socket and head values, then adjust all callers in server.ts and the test to pass those fields through the object while preserving existing behavior.Source: Coding guidelines
116-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unrequested explanatory comments.
src/dev/app.ts#L116-L120: remove the behavior-restating JSDoc.test/unit/dev-proxy-ws.test.ts#L20-L20: remove the helper-description comment.As per coding guidelines, “Do not add comments explaining what the line does unless prompted.”
🤖 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 `@src/dev/app.ts` around lines 116 - 120, Remove the behavior-restating JSDoc immediately preceding the WebSocket proxy helper in src/dev/app.ts (lines 116-120), and remove the helper-description comment in test/unit/dev-proxy-ws.test.ts (line 20); retain the associated code unchanged.Source: Coding guidelines
🤖 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 `@test/unit/dev-proxy-ws.test.ts`:
- Around line 41-42: Update the request setup in the WebSocket upgrade test so
the timeout created by setTimeout is cleared before rejecting on req’s error
event. Preserve the existing “upgrade timed out” rejection and error propagation
while ensuring both fallback failures do not leave the five-second timer active.
---
Nitpick comments:
In `@src/dev/app.ts`:
- Line 121: Update proxyUpgrade to accept an options object as its second
parameter containing the current socket and head values, then adjust all callers
in server.ts and the test to pass those fields through the object while
preserving existing behavior.
- Around line 116-120: Remove the behavior-restating JSDoc immediately preceding
the WebSocket proxy helper in src/dev/app.ts (lines 116-120), and remove the
helper-description comment in test/unit/dev-proxy-ws.test.ts (line 20); retain
the associated code unchanged.
In `@test/unit/dev-proxy-ws.test.ts`:
- Line 33: Update upgradeRequest to accept an options object containing path as
its second parameter instead of a separate path argument, then revise every call
site to pass that object while preserving the existing request behavior.
🪄 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 Plus
Run ID: 2c774a78-b11f-4014-8249-4632afd344d1
📒 Files selected for processing (3)
src/dev/app.tssrc/dev/server.tstest/unit/dev-proxy-ws.test.ts
| const timeout = setTimeout(() => reject(new Error("upgrade timed out")), 5000); | ||
| req.on("error", reject); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Clear the timeout on request errors.
The two expected fallback failures reject through error but retain their five-second timers, unnecessarily extending the test process.
Proposed fix
- req.on("error", reject);
+ req.on("error", (error) => {
+ clearTimeout(timeout);
+ reject(error);
+ });📝 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.
| const timeout = setTimeout(() => reject(new Error("upgrade timed out")), 5000); | |
| req.on("error", reject); | |
| const timeout = setTimeout(() => reject(new Error("upgrade timed out")), 5000); | |
| req.on("error", (error) => { | |
| clearTimeout(timeout); | |
| reject(error); | |
| }); |
🤖 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 `@test/unit/dev-proxy-ws.test.ts` around lines 41 - 42, Update the request
setup in the WebSocket upgrade test so the timeout created by setTimeout is
cleared before rejecting on req’s error event. Preserve the existing “upgrade
timed out” rejection and error propagation while ensuring both fallback failures
do not leave the five-second timer active.
1 participant
🔗 Linked issue
resolves #4269
❓ Type of change
📚 Description
this is also reproducible on v2 (see nuxt/cli#107).
the issue is that
devProxywithws: trueproxies HTTP fine but the upgrade falls through to the dev worker and gets answered as a normal request📝 Checklist