feat(ws): allow optional HTTP handling in defineWebSocketHandler#1425
Conversation
|
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 (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an optional ChangesWebSocket HTTP Fallback Support
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Non-upgrade (plain HTTP) requests to a WebSocket route return `426 Upgrade Required` by default. Pass an `http` handler as the second argument to serve those requests instead, letting the same route handle both WebSocket upgrades and regular HTTP requests. Upgrade requests always go to the hooks. Co-Authored-By: Claude Opus 4.8 <[email protected]>
The GitHub-style `> [!NOTE]` alert in the JSDoc was flattened by automd into a single broken line. Use a plain "Note:" sentence instead. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Use the `html` util to serve a minimal self-contained playground for plain HTTP requests, demonstrating the new optional `http` handler: the same route serves the page and the WebSocket endpoint the page connects back to. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Fixes CI lint: the autofix-ci commit left the doc code blocks in a form that `oxfmt --check` rejects. This is the idempotent `pnpm fmt` output. Co-Authored-By: Claude Opus 4.8 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/2.utils/9.more.md`:
- Around line 94-98: The `defineWebSocketHandler` docs currently show the wrong
signature and suggest `http` is the only argument. Update the heading and any
nearby description to use `defineWebSocketHandler(hooks, http?)`, making it
clear that `hooks` is the required first parameter and `http` is optional. Keep
the explanation aligned with the actual API behavior in the
`defineWebSocketHandler` section.
In `@src/utils/ws.ts`:
- Around line 55-65: `defineWebSocketHandler` currently takes `http` as a
positional second argument, which conflicts with the repo’s options-object
guideline for multi-argument APIs. Update the public signature so the second
parameter is an options object (for example, an object containing `http`), then
adjust the `defineHandler`/`_webSocketHandler` implementation to read from that
options object while preserving the existing fallback behavior when
non-WebSocket requests are handled.
🪄 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: b22fdfcf-67c1-4cd2-a98e-64c383bacace
📒 Files selected for processing (4)
docs/1.guide/901.advanced/2.websocket.mddocs/2.utils/9.more.mdsrc/utils/ws.tstest/ws.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/websocket.mjs (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVariable name
wsreused for both the crossws plugin and the client-side WebSocket instance.Line 2 imports the crossws plugin as
ws; the embedded browser script (line 63) also declaresconst ws = new WebSocket(url). These live in separate execution contexts (server module vs. generated HTML string) so there's no actual bug, but the shared name can confuse readers skimming the example.Also applies to: 63-63
🤖 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 `@examples/websocket.mjs` at line 2, The identifier `ws` is being reused for two different concepts in `examples/websocket.mjs`, which makes the example harder to read. Rename the imported crossws plugin binding from `ws` to a more explicit name in the module scope, and keep the browser-side `const ws = new WebSocket(url)` as-is or rename that one for clarity; update all references tied to the import so the `plugin` usage remains consistent.docs/1.guide/901.advanced/2.websocket.md (1)
122-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute path inconsistency in HTTP-fallback example.
The main "Full example" registers the combined route at
/(line 47), but this snippet uses/_ws(line 130). Since the whole point of the section is to demonstrate the same combined-route pattern, using a consistent path avoids reader confusion.✏️ Suggested tweak
app.get( - "/_ws", + "/", defineWebSocketHandler( { message: (peer, message) => peer.send(message.text()) }, () => "Send a WebSocket upgrade request to connect.", ), );🤖 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 `@docs/1.guide/901.advanced/2.websocket.md` around lines 122 - 137, The HTTP-fallback example uses a different route path than the earlier combined-route example, which can confuse readers about the pattern being demonstrated. Update the app.get call in this snippet to use the same route path as the “Full example” so the defineWebSocketHandler usage stays consistent across the section.
🤖 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.
Nitpick comments:
In `@docs/1.guide/901.advanced/2.websocket.md`:
- Around line 122-137: The HTTP-fallback example uses a different route path
than the earlier combined-route example, which can confuse readers about the
pattern being demonstrated. Update the app.get call in this snippet to use the
same route path as the “Full example” so the defineWebSocketHandler usage stays
consistent across the section.
In `@examples/websocket.mjs`:
- Line 2: The identifier `ws` is being reused for two different concepts in
`examples/websocket.mjs`, which makes the example harder to read. Rename the
imported crossws plugin binding from `ws` to a more explicit name in the module
scope, and keep the browser-side `const ws = new WebSocket(url)` as-is or rename
that one for clarity; update all references tied to the import so the `plugin`
usage remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eef97a78-3b90-4662-8cc6-24f4608bdd4a
📒 Files selected for processing (2)
docs/1.guide/901.advanced/2.websocket.mdexamples/websocket.mjs
`pnpm fmt` already runs `automd` (then `oxfmt` last), so the trailing `pnpm automd` re-emitted embedded code blocks in a form that `oxfmt --check` (run by `pnpm lint` in CI) rejects — causing autofix and CI to fight in a loop. Removing it makes the bot's output match `pnpm fmt`. Also restructure the WebSocket example to extract the playground HTML into a top-level `const`, keeping the handler a trivial `() => playground`. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Summary
defineWebSocketHandler(hooks)currently returns426 Upgrade Requiredfor any request that isn't a WebSocket upgrade. This adds an optional second argument so the same route can serve both WebSocket upgrades and plain HTTP requests.Upgrade: websocket) always go to the crosswshooks.httphandler when provided; otherwise the existing426response is returned (fully backward compatible).Design notes
httphandler is a plainEventHandlerpassed as the second argument, keeping the call site terse.upgradehook for rejecting/customizing the handshake itself (documented in JSDoc).upgrade: websocketheader) mirrors crossws' own canonical check.Changes
src/utils/ws.ts— optionalhttpfallback handler + JSDoc examples.test/ws.test.ts— regression tests for the HTTP fallback and for hooks still attaching on upgrade requests.docs/2.utils/9.more.md— regenerated viaautomd(pnpm fmt).Testing
pnpm vitest run test/ws.test.ts— 5/5 pass, types cleanpnpm lint— clean (lint + format + typecheck)🤖 Generated with Claude Code
Summary by CodeRabbit