Repository navigation
Core: Handle WebSocket errors so a bad frame can't crash the dev server - #35863
ghengeveld merged 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe server channel now attaches an error listener to each connected WebSocket and logs emitted errors. A regression test checks that emitting a socket error does not throw. ChangesServer-channel WebSocket error handling
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@code/core/src/core-server/utils/__tests__/server-channel.test.ts`:
- Around line 371-381: Update the test around ServerChannelTransport to release
the transport after asserting the socket error behavior. Emit the transport
socket’s close event during cleanup so the real setInterval started by
ServerChannelTransport is cleared, without adding fake-timer setup.
🪄 Autofix
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf4ce7a7-c264-4124-9d80-e55c6a476a4f
📒 Files selected for processing (2)
code/core/src/core-server/utils/__tests__/server-channel.test.tscode/core/src/core-server/utils/get-server-channel.ts
512cab7 to
9c7afb2
Compare
|
|
…n't crash the dev server When a connected browser tab sends a frame exceeding the ws default maxPayload (100 MB) — or any malformed frame — ws emits an 'error' event on that socket (e.g. WS_ERR_UNSUPPORTED_MESSAGE_LENGTH). The server channel transport only attaches a 'message' listener in the connection handler; in Node an 'error' event with no listener is fatal, so a single bad frame from one stale browser tab crashes the whole storybook dev process. Attach an error listener on each server-channel socket that logs via the already-imported logger and does not rethrow; ws still closes the offending socket itself. Fixes storybookjs#35862 Signed-off-by: liumengjie.ai <liumengjie.ai@bytedance.com>
9c7afb2 to
e0b4846
Compare
Check the diff here: storybookjs/storybook@8af3851...50a8282 List of included PRs since previous version: - storybookjs/storybook#36301 (sharadvc/cursor/fix-mcp-connect-agents-link-6fef) - storybookjs/storybook#35863 (SnowingFox/fix/35862-server-channel-error-handler) - storybookjs/storybook#36688 (ghengeveld/vitest-36221-leftovers) - storybookjs/storybook#36746 (fix/bbc-local-story-manifest-snippets) - storybookjs/storybook#36588 (fix/nextjs-router-mock-bfcache) - storybookjs/storybook#36391 (agent/sto-492-eslint-cjs) - storybookjs/storybook#36751 (agent/sto-280-prerelease-version-alignment) - storybookjs/storybook#36768 (svelte-knip-cleanup) - storybookjs/storybook#35834 (FrancoKaddour/fix/docgen-typescript7-missing-compiler-api) - storybookjs/storybook#36770 (bryce-easygo/fix/svelte-csf-null-arg-snippet)
Fixes #35862
What
When a connected browser tab sends a frame exceeding the
wsdefaultmaxPayload(100 MB) — or any otherwise malformed frame —wsemits anerrorevent on that socket (e.g.WS_ERR_UNSUPPORTED_MESSAGE_LENGTH). The server channel transport incode/core/src/core-server/utils/get-server-channel.tsonly attaches amessagelistener inside theconnectionhandler. In Node.js anerrorevent with no listener is fatal, so a single bad frame from one stale/buggy browser tab crashes the entirestorybook devprocess.Fix
Attach an
errorlistener on each server-channel socket that logs the error via the already-importedloggerand does not rethrow.wsstill closes the offending socket itself, so the dev server keeps running.Test
Added a regression test in
server-channel.test.tsthat connects a mock socket and emits anerrorevent on it, asserting it does not throw. Without the fix,EventEmitterthrows an unhandlederror(the exact process-crash behaviour reported); with the fix, the error is handled and the test passes.Manual testing
storybook devand open the preview in a browser.wsdefaultmaxPayloadof 100 MB).storybook devprocess crashes on the unhandled socketerror; with this fix the error is logged and the dev server keeps running.Note: this is a one-handler fix verified against the current
nextsource; the fullserver-channel.test.tssuite will run on CI here.