Fix the example for guest. #126
Conversation
WalkthroughReinstates the full WebTransport client flow in Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
|
I'll read and test this later this evening. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-transport-ws/src/session.ts (1)
410-454:close()does not mark the session as closed, leading to inconsistent stateRight now
#closeassumesthis.#closedholds anErrorto propagate into streams, butclose():
- Calls
this.#close(code, reason)without first settingthis.#closed, sostream.error(this.#closed)usesundefinedduring application‑initiated closes.- Leaves
this.#closedfalsy until the WebSocketoncloseevent, so methods that checkthis.#closed(createBidirectionalStream,createUnidirectionalStream, etc.) can still succeed afterclose()has been called, which is surprising.Consider setting
#closedinsideclose()before delegating to#close, so the state becomes closed immediately and streams receive a meaningful error. For example:- close(info?: { closeCode?: number; reason?: string }) { - if (this.#closed) return; - - const code = info?.closeCode ?? 0; - const reason = info?.reason ?? ""; - - this.#sendPriorityFrame({ - type: "connection_close", - code: VarInt.from(code), - reason, - }); - - setTimeout(() => { - this.#ws.close(); - }, 100); - - this.#close(code, reason); - } + close(info?: { closeCode?: number; reason?: string }) { + if (this.#closed) return; + + const code = info?.closeCode ?? 0; + const reason = info?.reason ?? ""; + + this.#sendPriorityFrame({ + type: "connection_close", + code: VarInt.from(code), + reason, + }); + + setTimeout(() => { + this.#ws.close(); + }, 100); + + // Mark the session as closed so subsequent operations see the closed state + this.#closed = new Error(`Connection closed: ${code} ${reason}`); + this.#close(code, reason); + }This keeps the existing error/close handling for WebSocket‑initiated closures while ensuring application‑initiated closes behave consistently.
🧹 Nitpick comments (3)
justfile (1)
47-49: JS/TS Biome steps look good; consider tightening install behaviorAdding
bun install+bun run check/fixintocheck/fixplugs JS/TS into the existing Rust pipeline nicely. Two optional tweaks you might consider:
- Use a CI‑style install (
bun install --cior equivalent) to avoid accidental lockfile drift.- If these targets run frequently locally, you may want to rely on cached installs (or a separate
justtarget) instead of reinstalling on everycheck/fix.Also applies to: 77-79
web-transport-ws/src/frame.ts (1)
47-60: Frame layout and type surface are tidy; consider dropping unused Padding/PingThe stream and connection_close paths correctly rely on WebSocket message boundaries (no offsets/lengths/reason size), and the new
Anyunion aligns with whatencode/decodeactually handle.PaddingandPinginterfaces remain exported but are no longer part ofAnyand cannot be emitted/decoded via this API; if those frame types are truly deprecated here, you might want to remove the interfaces to avoid confusion.Also applies to: 82-93, 135-146, 148-158, 39-45
web-transport-ws/src/session.ts (1)
43-47: ReadableStream controller wiring is sound; runtime check is mostly defensiveCapturing the
ReadableStreamDefaultControllerinstances forincomingBidirectionalStreamsandincomingUnidirectionalStreamsviastartmatches how you treat per‑stream readers elsewhere. The explicitReadableStream didn't call startguard is effectively an assertion (start is synchronous per spec), so it’s unlikely to ever fire unless a non‑conforming polyfill is in use.Also applies to: 78-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.gitignore(1 hunks)biome.jsonc(1 hunks)flake.nix(1 hunks)justfile(2 hunks)package.json(1 hunks)web-demo/client.js(1 hunks)web-demo/package.json(1 hunks)web-transport-ws/examples/client.ts(1 hunks)web-transport-ws/package.json(1 hunks)web-transport-ws/src/frame.ts(8 hunks)web-transport-ws/src/index.ts(1 hunks)web-transport-ws/src/session.ts(17 hunks)web-transport-ws/src/stream.ts(1 hunks)web-transport-ws/src/varint.ts(1 hunks)web-transport-ws/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- web-transport-ws/tsconfig.json
- web-transport-ws/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- web-transport-ws/examples/client.ts
🧰 Additional context used
🧬 Code graph analysis (3)
web-transport-ws/src/stream.ts (2)
web-transport-ws/src/varint.ts (1)
VarInt(1-97)web-transport-ws/src/stream.rs (1)
dir(26-32)
web-transport-ws/src/session.ts (2)
web-transport-ws/src/varint.ts (1)
VarInt(1-97)web-transport-ws/src/frame.ts (2)
StopSending(26-30)Any(47-47)
web-transport-ws/src/frame.ts (3)
web-transport-ws/src/stream.ts (1)
Id(10-49)web-transport-ws/src/varint.ts (3)
VarInt(1-97)encode(27-50)decode(52-96)web-transport-ws/src/session.ts (7)
frame(139-153)frame(155-248)frame(250-257)frame(259-272)frame(274-282)frame(284-287)code(410-435)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
.gitignore (1)
4-4: Ignoringnode_modulesis appropriate for the new JS/TS tooling.Adding
node_moduleshere is consistent with introducing workspaces and avoids committing install artifacts from each package.flake.nix (1)
20-36: Bun in devShell matches the JS/TS workflow.Including
pkgs.bunintoolsis a good fit for the new JS tooling; devShell remains straightforward and focused.web-demo/client.js (1)
62-69: Template literal logging change is fine.Switching to template literals keeps the logs readable without changing behavior; the example remains clear and minimal.
package.json (1)
1-16: Monorepo root and Biome tooling setup look consistent.Workspace definitions,
private: true,type: "module", and the Biomecheck/fixscripts form a coherent root configuration for the two packages.web-demo/package.json (1)
2-7: Package metadata forweb-demois appropriate.Setting
nameandprivate: trueis a good fit for a workspace-only demo package; no issues with the existing devDependencies.web-transport-ws/src/index.ts (1)
4-9: Polyfill guard and global installation look correct.
install()only patchesglobalThis.WebTransportwhen absent and returns a boolean indicating whether the polyfill was installed; this matches typical polyfill behavior.biome.jsonc (1)
1-39: Biome configuration is coherent and matches the tooling directionSchema, VCS options, formatter defaults, and the targeted rule overrides (a11y media captions, Node import protocol, Tailwind/ts-ignore) all look consistent with the repo’s needs and the new JS/TS checks.
web-transport-ws/src/frame.ts (1)
64-80: RESET_STREAM / STOP_SENDING code encoding/decoding is now consistentThe updated
encodebranches now write both the stream ID and thecodevarint forreset_streamandstop_sending, anddecodemirrors that layout. The buffer sizing with1 + 8 + 8(type + max varint sizes) matchesVarInt.MAX_SIZEand avoids reallocation.Also applies to: 107-119, 121-133
web-transport-ws/src/stream.ts (1)
3-48: Stream ID packing/unpacking logic looks correctThe
Dirflags,Id.createbit layout, andcanRecv/canSendchecks are consistent with the expected uni/bi and initiator semantics and match the Rust counterpart.web-transport-ws/src/session.ts (5)
5-24: Datagrams stub is a reasonable placeholderThe
Datagramsclass satisfiesWebTransportDatagramDuplexStreamstructurally and gives callers something type‑safe to hold onto, while clearly marked with a TODO. Given the current WebSocket transport focus, this is a pragmatic interim step.Also applies to: 48-50
52-58: Option handling aligns with what WebSocket transport can supportRejecting
requireUnreliableand warning onserverCertificateHashesmakes the limitations of the WebSocket‑based implementation explicit instead of silently ignoring options, which is preferable.
128-137: Centralized close/error handling improves robustnessRouting WebSocket
error/closeevents andconnection_closeframes through#close(with a dedicated#closeReasonfor application‑level close frames) ensures that theclosedpromise resolves and active streams are failed consistently instead of leaking.Also applies to: 147-147
155-247: Stream handling and STOP_SENDING/RESET_STREAM signaling look consistentThe
#handleStreamFrame,#handleResetStream, and#handleStopSendingpaths correctly:
- Validate stream direction via
canRecv.- Lazily create incoming streams and queue them on the appropriate
incoming*Streamscontroller.- Propagate STOP_SENDING/RESET_STREAM using the varint
codefield and clean up the stream maps.The early return for streams we initiated (
serverInitiated === this.#isServer) is effectively a no‑op guard and doesn’t appear to affect correctness.
274-287: Send path changes are reasonableUsing
Uint8Arraydirectly withWebSocket.sendand applying simple backpressure onbufferedAmountkeeps the send path straightforward, and the updated abort/close handlers for bidirectional and unidirectional streams properly send RESET or FIN and clean up#sendStreams.Also applies to: 296-335, 363-404
| if (dst.buffer.byteLength < dst.byteLength + size) { | ||
| throw new Error("destination buffer too small"); | ||
| } | ||
|
|
||
| const view = new DataView(dst.buffer, dst.byteOffset + dst.byteLength, size) | ||
| const view = new DataView(dst.buffer, dst.byteOffset + dst.byteLength, size); | ||
|
|
||
| if (size === 1) { | ||
| view.setUint8(0, Number(x)) | ||
| view.setUint8(0, Number(x)); | ||
| } else if (size === 2) { | ||
| view.setUint16(0, (0b01 << 14) | Number(x), false) | ||
| view.setUint16(0, (0b01 << 14) | Number(x), false); | ||
| } else if (size === 4) { | ||
| view.setUint32(0, (0b10 << 30) | Number(x), false) | ||
| view.setUint32(0, (0b10 << 30) | Number(x), false); | ||
| } else if (size === 8) { | ||
| view.setBigUint64(0, (0b11n << 62n) | x, false) | ||
| view.setBigUint64(0, (0b11n << 62n) | x, false); | ||
| } else { | ||
| throw new Error("VarInt value too large") | ||
| throw new Error("VarInt value too large"); | ||
| } | ||
|
|
||
| return new Uint8Array(dst.buffer, dst.byteOffset, dst.byteLength + size) | ||
| return new Uint8Array(dst.buffer, dst.byteOffset, dst.byteLength + size); | ||
| } |
There was a problem hiding this comment.
Capacity check in encode should account for byteOffset.
The current check:
if (dst.buffer.byteLength < dst.byteLength + size) {
throw new Error("destination buffer too small");
}ignores dst.byteOffset. If dst is a subview over a larger ArrayBuffer, this can mis-detect available space and still allow a DataView constructor RangeError.
Consider tightening the check to use the actual end offset of the appended data:
- if (dst.buffer.byteLength < dst.byteLength + size) {
- throw new Error("destination buffer too small");
- }
+ if (dst.byteOffset + dst.byteLength + size > dst.buffer.byteLength) {
+ throw new Error("destination buffer too small");
+ }This keeps behavior the same when dst.byteOffset === 0 (your likely case) while making encode robust if it’s ever called with a sliced view.
🤖 Prompt for AI Agents
In web-transport-ws/src/varint.ts around lines 31 to 50, the capacity check in
encode ignores dst.byteOffset and can allow a DataView RangeError for sliced
ArrayBuffers; change the condition to compare the buffer's length against the
actual end offset (dst.byteOffset + dst.byteLength + size) so you throw
"destination buffer too small" when that end would exceed dst.buffer.byteLength,
keeping the rest of the logic (DataView creation, writes, and returned
Uint8Array) unchanged.
|
https://nodejs.org/en/learn/getting-started/websocket
|
Fix the example
Summary by CodeRabbit
Release Notes
New Features
Chores