Conversation
Closes the remaining follow-up from #154 after #201 landed automatic liveness. Adds `ping`/`pong` hooks to observe inbound control frames and `peer.ping(data?)` to send one, wired for Node (ws), uWebSockets, and Bun (all natively support it); Deno, Cloudflare, Bunny, and SSE have no such runtime API and fall back to a warning no-op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds application-level ChangesPing/Pong support
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Poem
🚥 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 (4)
src/peer.ts (1)
187-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnthrottled warning on unsupported adapters.
The default
ping()callsconsole.warnon every invocation. If a caller sets up a periodic app-level heartbeat viapeer.ping()(a natural use case per the docs) on an adapter without support (Deno, Cloudflare, Bunny, SSE), this will spam the log on every tick indefinitely.Consider warning only once per process (e.g. module-level flag) rather than per call.
♻️ Proposed fix to warn once
+let _pingWarned = false; + export abstract class Peer<Internal extends AdapterInternal = AdapterInternal> { ... ping(_data?: unknown): number | void | undefined { - console.warn("[crossws] `peer.ping()` is not supported by this adapter."); + if (!_pingWarned) { + _pingWarned = true; + console.warn("[crossws] `peer.ping()` is not supported by this adapter."); + } }🤖 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/peer.ts` around lines 187 - 189, The unsupported-adapter warning in peer.ping() is emitted on every call, which can spam logs for periodic heartbeats; update the ping method in peer.ts to warn only once per process by adding a module-level guard/flag around the console.warn call, so repeated peer.ping() invocations on adapters like Deno, Cloudflare, Bunny, or SSE do not keep logging.src/adapters/bun.ts (1)
171-174: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPing payload bypasses the normalization used by
send/_publish.
send()and_publish()both route throughtoBufferLike(data)to safely coerce arbitrary input (objects, numbers, etc.) into a sendable form.ping()instead force-casts theunknownpayload straight toanyand hands it to Bun'sws.ping(), which only acceptsstring | BufferSource. If a caller passes a plain object/number (as they can withsend), this will likely throw an opaque runtime error from Bun's native binding rather than failing predictably or being normalized. The same pattern repeats in the Node and uWS adapters.💡 Possible approach
- override ping(data?: unknown): number { - return this._internal.ws.ping(data as any); - } + override ping(data?: unknown): number { + return this._internal.ws.ping(toBufferLike(data) as any); + }🤖 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/adapters/bun.ts` around lines 171 - 174, The ping payload is bypassing the same normalization used by send and _publish, so arbitrary inputs can reach Bun’s ws.ping() and fail at runtime. Update ping() in bun.ts to normalize the optional payload with toBufferLike(data) before calling this._internal.ws.ping(), and apply the same fix in the matching ping implementations in the Node and uWS adapters so behavior stays consistent across adapters.src/adapters/uws.ts (1)
232-235: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame normalization gap noted in the Bun/Node adapters.
this._internal.uws.ping(data)is called with the raw payload typed asuws.RecognizedString, without going throughtoBufferLikethe waysend/_publishdo. This is consistent with the nativeuws.ping()signature itself, but if a caller passes a value that doesn't satisfyRecognizedString(allowed at the basePeer.ping(data?: unknown)level), it will fail at the native binding rather than being normalized likesend()does.🤖 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/adapters/uws.ts` around lines 232 - 235, The ping path in the uWS adapter still bypasses the same normalization used by send and _publish, so Peer.ping(data?: unknown) can reach the native binding with an unnormalized value. Update the uws.ts override of ping to normalize the incoming payload through toBufferLike before calling this._internal.uws.ping, and keep the method behavior aligned with the adapter’s other data-handling paths and the same pattern used in the Bun/Node adapters.src/adapters/node.ts (1)
301-304: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame normalization gap as the Bun/uWS adapters.
ws.ping(data)is called with the rawunknownpayload;send()normalizes viatoBufferLikefirst. Passing an object/number here would be handed straight to the underlyingwslibrary, which expectsstring | Buffer | ArrayBuffer | ..., not arbitrary JS values. Worth aligning withsend()'s handling for consistency across the peer API surface.🤖 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/adapters/node.ts` around lines 301 - 304, The Node adapter’s ping path has the same payload-normalization gap as the Bun/uWS adapters: `NodeWebSocket.ping` currently forwards the raw `unknown` value directly to `this._internal.ws.ping`. Update `ping()` to normalize the optional payload the same way `send()` does by converting it through `toBufferLike` before calling the underlying `ws` method, so the behavior stays consistent across the adapter API and only supported binary/string-like values are passed through.
🤖 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/1.guide/3.peer.md`:
- Around line 140-157: The RTT example in the peer guide is inconsistent with
the prose because it stores the timestamp in peer.context and sends an empty
ping instead of embedding the timestamp in ping data. Update the example in
defineHooks so the message handler attaches the timestamp to the ping payload,
and the pong handler reads that payload to compute RTT, keeping the code aligned
with the “embed a timestamp in data” wording.
---
Nitpick comments:
In `@src/adapters/bun.ts`:
- Around line 171-174: The ping payload is bypassing the same normalization used
by send and _publish, so arbitrary inputs can reach Bun’s ws.ping() and fail at
runtime. Update ping() in bun.ts to normalize the optional payload with
toBufferLike(data) before calling this._internal.ws.ping(), and apply the same
fix in the matching ping implementations in the Node and uWS adapters so
behavior stays consistent across adapters.
In `@src/adapters/node.ts`:
- Around line 301-304: The Node adapter’s ping path has the same
payload-normalization gap as the Bun/uWS adapters: `NodeWebSocket.ping`
currently forwards the raw `unknown` value directly to `this._internal.ws.ping`.
Update `ping()` to normalize the optional payload the same way `send()` does by
converting it through `toBufferLike` before calling the underlying `ws` method,
so the behavior stays consistent across the adapter API and only supported
binary/string-like values are passed through.
In `@src/adapters/uws.ts`:
- Around line 232-235: The ping path in the uWS adapter still bypasses the same
normalization used by send and _publish, so Peer.ping(data?: unknown) can reach
the native binding with an unnormalized value. Update the uws.ts override of
ping to normalize the incoming payload through toBufferLike before calling
this._internal.uws.ping, and keep the method behavior aligned with the adapter’s
other data-handling paths and the same pattern used in the Bun/Node adapters.
In `@src/peer.ts`:
- Around line 187-189: The unsupported-adapter warning in peer.ping() is emitted
on every call, which can spam logs for periodic heartbeats; update the ping
method in peer.ts to warn only once per process by adding a module-level
guard/flag around the console.warn call, so repeated peer.ping() invocations on
adapters like Deno, Cloudflare, Bunny, or SSE do not keep logging.
🪄 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: 246d6604-2659-4402-8444-6aae7341b467
📒 Files selected for processing (13)
docs/1.guide/2.hooks.mddocs/1.guide/3.peer.mdsrc/adapters/bun.tssrc/adapters/node.tssrc/adapters/uws.tssrc/hooks.tssrc/peer.tssrc/server/_resolve.tstest/adapters/bun.test.tstest/adapters/node.test.tstest/adapters/uws.test.tstest/fixture/_shared.tstest/tests.ts
…ongs - peer.ping() now catches native validation errors (e.g. non-buffer payloads, >125-byte control frames) and routes them through the error hook instead of crashing the process. - Node's internal keepalive pings are tagged so their echoed pongs don't fire the app-level pong hook; the RTT doc example now embeds a correlatable payload since Bun/uWS keepalive pings can't be tagged. - Warn once per peer (not per call) when ping() is unsupported. - Skip the uWS ping/pong Uint8Array copy when no hook/resolver needs it. - Simplify the pingPongTests message queue to a plain FIFO. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Summary
Closes #154 (the remaining follow-up scope after #201 landed automatic server-side liveness):
ping(peer, data)/pong(peer, data)hooks to theHooksinterface, firing when an application-level ping/pong control frame arrives from the peer.peer.ping(data?)to send an application-level ping.ws), uWebSockets (previously dropped itsping/pongbehavior handlers), and Bun (which turns out to fully supportws.ping()/ws.pong()andping/ponghandlers, better than the issue assumed). Deno, Cloudflare (incl. Durable Objects), Bunny, and SSE have no such control-frame API exposed to user code, sopeer.ping()warns and no-ops there and the hooks never fire.ping()/ping+ponghooks alongside the existingdrainrow in the peer guide, and adds both hooks to the hooks guide example.Every runtime already auto-replies to an inbound ping with a pong per the WebSocket spec — these hooks only observe the control frames, they don't need to answer them.
Test plan
pnpm lint/pnpm typecheckpasspnpm vitest run— full suite green (248 passed, 6 skipped)test/tests.ts(pingPongTests, using thewspackage client since raw ping/pong control frames aren't reachable through the standardWebSocketAPI), wired intonode.test.ts,uws.test.ts, andbun.test.ts:pinghook firesponghook firespeer.ping()→ client observes a real ping frame🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
ping()method on peers to send ping frames and help measure connection latency.Bug Fixes