Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an opt-in sync backplane for relaying pub/sub across instances, with new sync contracts, built-in drivers, peer and adapter relay changes, public exports, tests, and docs. ChangesSync Backplane Feature
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
pi0
marked this pull request as draft
June 17, 2026 17:25
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/cloudflare.ts (1)
68-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync backplane not wired for Cloudflare adapter.
Unlike other adapters, this calls
adapterUtils(globalPeers)without passingopts, so even if a user configuressyncin their Cloudflare adapter options, it will be ignored. This inconsistency may confuse users expecting sync to work uniformly across adapters.If sync support is intentionally excluded for Cloudflare (perhaps due to Durable Objects having their own cross-instance mechanism), consider documenting this or throwing an error if
opts.syncis provided.🔧 Suggested fix to wire sync (if intended)
- const { publish: durablePublish, ...utils } = adapterUtils(globalPeers); + const { publish: durablePublish, ...utils } = adapterUtils(globalPeers, opts);🤖 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/cloudflare.ts` at line 68, The adapterUtils function call in the Cloudflare adapter is missing the opts parameter. In the line where adapterUtils is invoked with only globalPeers as an argument, add opts as the second parameter (like adapterUtils(globalPeers, opts)) to ensure the sync backplane configuration from the user's options is properly wired and passed through to the adapter utilities, making it consistent with how other adapters handle this call.
🤖 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/5.pubsub.md`:
- Line 48: The sentence on line 48 starting with "No change to your hooks
and..." has awkward phrasing that affects readability. Restructure this sentence
in the sync adapter explanation to improve clarity - consider rephrasing it so
that the core message (that no code changes are needed while subscribe/publish
functionality now spans the cluster) flows more naturally and is easier for
users to understand.
In `@src/adapter.ts`:
- Around line 36-47: The sync subscription created in the sync driver
initialization block is not being cleaned up when the adapter is destroyed.
Capture the return value of the sync.subscribe() call and store it (as it likely
returns an unsubscribe function or handle). Then implement cleanup logic in the
adapter's shutdown or destroy method to call the unsubscribe function, ensuring
the subscription is properly disposed of when the adapter is no longer needed.
This prevents memory leaks and duplicate message delivery from stale
subscriptions.
- Around line 14-34: In the localPublish function, both send() and _publish()
are called on firstPeerWithTopic, which causes duplicate message delivery in
native pub/sub adapters (Bun, uWS) since their _publish() implementations
broadcast to all peers including self. Decide on the design intent: either
remove the send() call and rely solely on _publish() for all delivery across all
adapter types, or implement adapter-specific handling where loop-based adapters
use send() + _publish() while native pub/sub adapters use only _publish().
Document the chosen approach clearly so all adapters behave consistently without
message duplication.
In `@src/peer.ts`:
- Around line 110-116: The sync.publish() call within the publish method can
return a Promise but unhandled rejections are never caught, which will cause
unhandled promise rejection errors at runtime. Wrap the sync.publish() call with
Promise.resolve().catch() to handle both void and Promise return values,
matching the same error handling pattern used for sync.subscribe() elsewhere in
the codebase. This ensures transient network failures from networked sync
drivers are properly caught and handled instead of surfacing as unhandled
promise rejections.
In `@src/sync.ts`:
- Around line 88-95: The subscribe method's message event handler calls
deliver(envelope.msg) without validating that the msg property is actually a
valid SyncMessage. Add validation logic after the envelope null and id check to
ensure envelope.msg is a valid SyncMessage before passing it to the deliver
function. This will prevent malformed or invalid messages from propagating into
the local fan-out path and causing runtime errors.
In `@test/adapters/bun.test.ts`:
- Line 12: The expect assertion on line 12 is passing result.stderr as a custom
assertion message rather than actually checking its contents against the
assertion. Modify the expect call to test both result.stdout and result.stderr
together so that the assertion actually validates both output streams and
correctly detects "bun sync ok" whether it appears in stdout or stderr.
---
Outside diff comments:
In `@src/adapters/cloudflare.ts`:
- Line 68: The adapterUtils function call in the Cloudflare adapter is missing
the opts parameter. In the line where adapterUtils is invoked with only
globalPeers as an argument, add opts as the second parameter (like
adapterUtils(globalPeers, opts)) to ensure the sync backplane configuration from
the user's options is properly wired and passed through to the adapter
utilities, making it consistent with how other adapters handle this call.
🪄 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: d12e4393-4e9f-4380-912a-99af3a64f666
📒 Files selected for processing (18)
build.config.tsdocs/1.guide/5.pubsub.mdpackage.jsonsrc/adapter.tssrc/adapters/bun.tssrc/adapters/bunny.tssrc/adapters/cloudflare.tssrc/adapters/deno.tssrc/adapters/node.tssrc/adapters/sse.tssrc/adapters/uws.tssrc/index.tssrc/peer.tssrc/sync.tssrc/utils.tstest/adapters/bun.test.tstest/adapters/sync.test.tstest/fixture/bun-sync.ts
pi0
marked this pull request as ready for review
June 18, 2026 13:01
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/adapters/uws.test.ts (1)
103-105: ⚡ Quick winReplace the fixed readiness sleep with a condition-based wait.
Line 104 uses
setTimeout(50)to wait foropenhooks. This can be nondeterministic in CI. Wait for both peers to register (and optionally subscribe) before publishing.♻️ Proposed change
- // Let both `open` hooks run so each peer is subscribed before we publish. - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForPeerCount(server.ws, 2);🤖 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/adapters/uws.test.ts` around lines 103 - 105, The setTimeout with a fixed 50ms delay in the test is unreliable in CI environments. Replace this fixed delay with a condition-based wait mechanism that actually verifies both peers have completed their open hooks and are subscribed before the test proceeds to publish. Instead of relying on time, check for an actual readiness condition (such as a counter or flag that tracks when both peer subscriptions are complete) and wait until that condition is satisfied before resolving the promise.test/adapters/sync.test.ts (1)
146-147: ⚡ Quick winUse state-based waiting instead of a fixed sleep for peer registration.
Line 146 relies on
setTimeout(50), which can be flaky under slow CI. Pollinst.ws.peerswith a bounded timeout before asserting Line 148.♻️ Proposed change
- // Let the open hook run so the peer is registered on the server. - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForPeerCount(inst.ws, 1);async function waitForPeerCount( ws: { peers: Map<string, Set<unknown>> }, expected: number, timeoutMs = 1000, intervalMs = 10, ) { const start = Date.now(); for (;;) { const count = [...ws.peers.values()].reduce((n, set) => n + set.size, 0); if (count === expected) { return; } if (Date.now() - start > timeoutMs) { throw new Error(`Timed out waiting for peerCount=${expected}, got ${count}`); } await new Promise((r) => setTimeout(r, intervalMs)); } }🤖 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/adapters/sync.test.ts` around lines 146 - 147, Replace the fixed setTimeout sleep on line 146 with a state-based polling approach. Create a helper function waitForPeerCount that polls inst.ws.peers with a configurable timeout and interval, checking if the peer count matches the expected value before returning or throwing an error if the timeout expires. Call this new waitForPeerCount helper with inst.ws and the expected peer count instead of the hardcoded 50ms sleep, ensuring the peer registration state is verified before the subsequent assertion on line 148.
🤖 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 `@src/sync/broadcast-channel.ts`:
- Around line 30-40: The message event handler in the BroadcastChannel
addEventListener callback only validates that envelope.msg.topic is a string,
but does not validate the namespace and data properties of the SyncMessage
before passing it to deliver(). Add validation to the existing if condition that
checks envelope.id and envelope.msg?.topic to also verify that envelope.msg has
a valid namespace property (should be a string) and a valid data property
(should be a string or undefined). Only call deliver() if all required
properties pass validation, matching the payload validation rigor of text
drivers.
In `@src/sync/encoding.ts`:
- Around line 20-43: The decodeEnvelope function in src/sync/encoding.ts
validates that id, msg, topic, and namespace are strings, but does not validate
that msg.data is a string. Add a check within the existing validation block (the
if statement starting around line 26) to ensure parsed.msg.data is a string
type. If msg.data is not a string, the function should return undefined to
prevent invalid data types from being passed through as SyncMessage.data values.
---
Nitpick comments:
In `@test/adapters/sync.test.ts`:
- Around line 146-147: Replace the fixed setTimeout sleep on line 146 with a
state-based polling approach. Create a helper function waitForPeerCount that
polls inst.ws.peers with a configurable timeout and interval, checking if the
peer count matches the expected value before returning or throwing an error if
the timeout expires. Call this new waitForPeerCount helper with inst.ws and the
expected peer count instead of the hardcoded 50ms sleep, ensuring the peer
registration state is verified before the subsequent assertion on line 148.
In `@test/adapters/uws.test.ts`:
- Around line 103-105: The setTimeout with a fixed 50ms delay in the test is
unreliable in CI environments. Replace this fixed delay with a condition-based
wait mechanism that actually verifies both peers have completed their open hooks
and are subscribed before the test proceeds to publish. Instead of relying on
time, check for an actual readiness condition (such as a counter or flag that
tracks when both peer subscriptions are complete) and wait until that condition
is satisfied before resolving the promise.
🪄 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: e21b5c6c-a266-4f74-89c5-642d52d4c31e
📒 Files selected for processing (19)
docs/1.guide/5.pubsub.mddocs/1.guide/6.sync.mddocs/1.guide/7.resolver.mddocs/1.guide/8.proxy.mdsrc/adapter.tssrc/adapters/bun.tssrc/adapters/deno.tssrc/adapters/uws.tssrc/peer.tssrc/sync.tssrc/sync/broadcast-channel.tssrc/sync/cluster.tssrc/sync/encoding.tssrc/sync/pgsql.tssrc/sync/redis.tssrc/sync/types.tstest/adapters/sync.test.tstest/adapters/uws.test.tstest/sync.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/sync/types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/adapters/bun.ts
- src/adapters/uws.ts
- src/peer.ts
- src/adapters/deno.ts
- src/adapter.ts
- cloudflare: honor `sync`/`onError` (was silently ignored); wire the
driver into fallback + durable peers and give fallback peers real local
fan-out. Inbound delivery into a hibernated Durable Object is best-effort;
outbound relay is reliable (documented).
- add `onError(error, { stage })` to observe a degraded backplane; centralize
relay error isolation in adapterUtils so peer/adapter publish fire-and-forget
through one reporter, and survive a synchronous subscribe throw.
- docs: add "Delivery semantics" (at-most-once, unordered, no replay) + a
Cloudflare note.
- mark `Peer._publish` @internal; fix `String.fromCharCode(...)` stack overflow
on large binary payloads in `toString`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keep the runtime-specific Durable Object / fallback / hibernation notes with the Cloudflare adapter docs, leaving a short pointer in the sync guide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 (2)
src/utils.ts (1)
45-45: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid broad
new Uint8Array(data)coercion inserializeMessage()andtoString().Message.datacan be aBlob, and this path turns it into empty bytes, so relayed or encoded payloads lose content. HandleArrayBuffer/typed-array views explicitly or reject unsupported payloads.🤖 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/utils.ts` at line 45, The payload coercion in serializeMessage()/toString() is too broad: Message.data may be a Blob, and the current Uint8Array conversion can silently drop its content. Update the handling in utils.ts around the data-to-bytes path to explicitly support ArrayBuffer and typed-array views, and either properly read Blob contents or reject unsupported payload types rather than falling back to new Uint8Array(data). Use the existing serializeMessage and toString helpers as the main places to adjust.src/adapters/cloudflare.ts (1)
300-323: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard fallback fan-out against durable peers in the same namespace.
getPeers(globalPeers, namespace)is shared by both upgrade paths, so a customresolveDurableStubcan leaveCloudflareDurablePeerandCloudflareFallbackPeerin the same Set._publish()assumes every peer haswsServer, so it will throw as soon as it hits a durable peer. A runtime/type guard here would keep namespace publish from crashing.🤖 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/cloudflare.ts` around lines 300 - 323, The fallback fan-out in CloudflareFallbackPeer._publish assumes every peer in this._internal.peers has wsServer, but getPeers(globalPeers, namespace) can mix CloudflareFallbackPeer and CloudflareDurablePeer in the same namespace set. Add a runtime/type guard inside _publish to only send to peers that actually have a wsServer (or otherwise narrow to fallback peers) before calling send, while keeping the existing topic check and send path in CloudflareFallbackPeer.send intact.
🤖 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 `@src/peer.ts`:
- Around line 197-205: The fire-and-forget publish path in Peer.sync.publish
still lets a synchronous driver.publish throw escape, since only async
rejections are currently handled. Update the sync publish wrapper in Peer (and
the adapterUtils-wrapped driver path it uses) so the publish call itself is
protected and both sync throws and Promise rejections are routed to onError,
preserving the existing fire-and-forget behavior without leaking errors.
---
Outside diff comments:
In `@src/adapters/cloudflare.ts`:
- Around line 300-323: The fallback fan-out in CloudflareFallbackPeer._publish
assumes every peer in this._internal.peers has wsServer, but
getPeers(globalPeers, namespace) can mix CloudflareFallbackPeer and
CloudflareDurablePeer in the same namespace set. Add a runtime/type guard inside
_publish to only send to peers that actually have a wsServer (or otherwise
narrow to fallback peers) before calling send, while keeping the existing topic
check and send path in CloudflareFallbackPeer.send intact.
In `@src/utils.ts`:
- Line 45: The payload coercion in serializeMessage()/toString() is too broad:
Message.data may be a Blob, and the current Uint8Array conversion can silently
drop its content. Update the handling in utils.ts around the data-to-bytes path
to explicitly support ArrayBuffer and typed-array views, and either properly
read Blob contents or reject unsupported payload types rather than falling back
to new Uint8Array(data). Use the existing serializeMessage and toString helpers
as the main places to adjust.
🪄 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: 2ab1b234-9a80-40eb-b0d5-8d553e78681d
📒 Files selected for processing (7)
docs/1.guide/6.sync.mdsrc/adapter.tssrc/adapters/cloudflare.tssrc/index.tssrc/peer.tssrc/utils.tstest/adapters/sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/adapter.ts
Addresses CodeRabbit review on #192: - adapter: a *synchronous* driver.publish() throw escaped the fire-and-forget wrapper into the caller's publish(); now routed to onError like rejections. - encoding: decodeEnvelope rejects a non-string `data` (a foreign producer could leak an object/null through as SyncMessage.data). - broadcast-channel: validate namespace + data shape, not just topic, matching the contract the text drivers enforce via decodeEnvelope. - docs: reword the sync intro sentence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fallback _publish rewrite fanned out by calling `.send()` on other peers' sockets, but in the Workers model each WebSocket lives in its own `fetch` invocation and sending to a socket owned by another request context throws "Network connection lost" — hanging the Worker (CI timeouts on the fallback cloudflare suite). Cross-connection pub/sub requires Durable Objects, which is exactly why the original code was a warn stub. Restore it, drop the misleading fallback sync wiring, and correct the docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs: https://feat-sync.h3-crossws.pages.dev/guide/sync
Sync backplane
Opt-in
syncadapter that relays pub/sub between crossws instances over a shared backplane, sopeer.publish()/adapter.publish()reach subscribers connected to other regions, processes, or replicas. Whensyncis absent, pub/sub stays local exactly as before.Built-in drivers (
crossws/sync, zero third-party deps):redis— Redis pub/sub (ioredis + node-redis, auto-detected). Multi-region/host.pgsql— PostgresLISTEN/NOTIFY(node-postgres + postgres.js). For clusters already on Postgres.cluster— NodeclusterIPC (+setupPrimaryCluster()). Forked processes on one host (Node cluster / PM2).broadcastChannel—BroadcastChannel. In-process worker threads / tests.Custom backplanes implement the
SyncAdapterinterface;encodeEnvelope/decodeEnvelopeare exported for the wire format (sender-id echo suppression + base64 binary).Delivery semantics: best-effort, at-most-once, unordered, no replay/buffering. Local delivery is independent of backplane health. Relay failures never throw into
publish()— passonError(error, { stage })(stage:subscribe|publish|delivery) to observe a degraded backplane; defaults toconsole.error.Cloudflare: the single default Durable Object is already cluster-global, so a backplane only matters for multi-DO sharding or the fallback (no-DO) path. Inbound delivery into a hibernated Durable Object is best-effort; outbound relay is reliable. Built-in network drivers target Node-like runtimes — bring a workerd-native custom
SyncAdapter.Summary by CodeRabbit
Release Notes
New Features
./syncentry point for cross-instance pub/sub backplanes (withSyncMessage,SyncDriver, andSyncAdaptertypes).BroadcastChannel, plus Redis, PostgreSQL, and cluster/IPC.onError(error, { stage })for publish/subscribe/delivery failures.Documentation
Tests