Handle relays without announcement subscription support#1352
Merged
Conversation
The Cloudflare relay skips SUBSCRIBE_NAMESPACE, leaving the connection's announced set permanently empty. Combined with reload=true, Broadcast would wait forever for an announcement that never arrives, so the broadcast silently never subscribed. Expose `announceSupported` on Reload and have Broadcast.#isAnnounced short-circuit to true when announcements aren't supported, matching the reload=false code path.
Replace the separate `announceSupported` signal with `undefined` on the existing `announced` getter to express "relay doesn't support SUBSCRIBE_NAMESPACE". Broadcast now has a single check that mirrors the reload=false fast path, instead of two parallel branches.
Move the announcement-unsupported workaround back into Broadcast so the moq-lite layer stays generic and free of relay-specific quirks. Broadcast now checks the established connection's URL and treats a Cloudflare host as reload=false, since SUBSCRIBE_NAMESPACE will never fire there.
Contributor
WalkthroughHostname-aware logic was added so Broadcast.#isAnnounced treats connections whose hostname ends with 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/lite/src/connection/reload.ts`:
- Around line 161-164: The hostname check using
conn.url.hostname.endsWith("mediaoverquic.com") can false-positively match
lookalikes; update the condition in the block that logs "Cloudflare relay..."
(the code that sets this.#announced and returns from the subscribe_namespace
flow) to perform boundary-safe matching: normalize hostname to lowercase and use
hostname === "mediaoverquic.com" || hostname.endsWith(".mediaoverquic.com") (or
equivalently check for a dot-prefix suffix) instead of the current endsWith call
so only the exact host or subdomains of mediaoverquic.com match.
🪄 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: b435fba6-1c07-4b9e-a77b-647fbede142f
📒 Files selected for processing (2)
js/lite/src/connection/reload.tsjs/watch/src/broadcast.ts
Comment on lines
161
to
164
| if (conn.url.hostname.endsWith("mediaoverquic.com")) { | ||
| console.warn("Cloudflare relay does not support broadcast discovery yet; skipping subscribe_namespace."); | ||
| this.#announced.set(undefined); | ||
| return; |
Contributor
There was a problem hiding this comment.
Make Cloudflare hostname matching boundary-safe.
.endsWith("mediaoverquic.com") also matches lookalike hostnames (for example, notmediaoverquic.com). Use exact-host or dot-suffix matching to avoid false positives.
Proposed fix
- if (conn.url.hostname.endsWith("mediaoverquic.com")) {
+ const hostname = conn.url.hostname;
+ const isCloudflareRelay =
+ hostname === "mediaoverquic.com" || hostname.endsWith(".mediaoverquic.com");
+ if (isCloudflareRelay) {
console.warn("Cloudflare relay does not support broadcast discovery yet; skipping subscribe_namespace.");
this.#announced.set(undefined);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (conn.url.hostname.endsWith("mediaoverquic.com")) { | |
| console.warn("Cloudflare relay does not support broadcast discovery yet; skipping subscribe_namespace."); | |
| this.#announced.set(undefined); | |
| return; | |
| const hostname = conn.url.hostname; | |
| const isCloudflareRelay = | |
| hostname === "mediaoverquic.com" || hostname.endsWith(".mediaoverquic.com"); | |
| if (isCloudflareRelay) { | |
| console.warn("Cloudflare relay does not support broadcast discovery yet; skipping subscribe_namespace."); | |
| this.#announced.set(undefined); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/lite/src/connection/reload.ts` around lines 161 - 164, The hostname check
using conn.url.hostname.endsWith("mediaoverquic.com") can false-positively match
lookalikes; update the condition in the block that logs "Cloudflare relay..."
(the code that sets this.#announced and returns from the subscribe_namespace
flow) to perform boundary-safe matching: normalize hostname to lowercase and use
hostname === "mediaoverquic.com" || hostname.endsWith(".mediaoverquic.com") (or
equivalently check for a dot-prefix suffix) instead of the current endsWith call
so only the exact host or subdomains of mediaoverquic.com match.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/watch/src/broadcast.ts`:
- Around line 71-76: The hostname check using endsWith("mediaoverquic.com") is
too permissive and can match unrelated hosts; update the condition around
effect.get(this.connection) / conn?.url.hostname to perform exact host or
dot-boundary subdomain matching (for example, check hostname ===
"mediaoverquic.com" || hostname.endsWith(".mediaoverquic.com") or use a regex
like /(^|\.)mediaoverquic\.com$/) instead of the plain
endsWith("mediaoverquic.com") so only the root host or its subdomains bypass the
announcement gating.
🪄 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: 906fbb48-8d32-4d41-b5de-0582f608a64c
📒 Files selected for processing (1)
js/watch/src/broadcast.ts
Comment on lines
+71
to
+76
| // Cloudflare's relay does not yet support announcement subscriptions, | ||
| // so an announcement will never arrive. Fall back to subscribing | ||
| // immediately (reload=false behaviour) instead of waiting forever. | ||
| const conn = effect.get(this.connection); | ||
| if (conn?.url.hostname.endsWith("mediaoverquic.com")) return true; | ||
|
|
Contributor
There was a problem hiding this comment.
Harden hostname matching to avoid accidental bypasses.
endsWith("mediaoverquic.com") can also match unrelated hosts like notmediaoverquic.com, which would incorrectly skip announcement gating. Use exact-host or dot-boundary subdomain matching.
Suggested fix
+const ANNOUNCEMENT_UNSUPPORTED_HOST = "mediaoverquic.com";
+
`#isAnnounced`(effect: Effect): boolean {
const reload = effect.get(this.reload);
if (!reload) return true;
@@
- const conn = effect.get(this.connection);
- if (conn?.url.hostname.endsWith("mediaoverquic.com")) return true;
+ const conn = effect.get(this.connection);
+ const hostname = conn?.url.hostname;
+ if (
+ hostname === ANNOUNCEMENT_UNSUPPORTED_HOST ||
+ hostname?.endsWith(`.${ANNOUNCEMENT_UNSUPPORTED_HOST}`)
+ ) {
+ return true;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/watch/src/broadcast.ts` around lines 71 - 76, The hostname check using
endsWith("mediaoverquic.com") is too permissive and can match unrelated hosts;
update the condition around effect.get(this.connection) / conn?.url.hostname to
perform exact host or dot-boundary subdomain matching (for example, check
hostname === "mediaoverquic.com" || hostname.endsWith(".mediaoverquic.com") or
use a regex like /(^|\.)mediaoverquic\.com$/) instead of the plain
endsWith("mediaoverquic.com") so only the root host or its subdomains bypass the
announcement gating.
…are-reload-flag-lDUbA
Move the existing Cloudflare console.warn from the moq-lite reload layer into Broadcast where the reload-skip actually happens, so users see why their reload=true request is being ignored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
js/watch/src/broadcast.ts (1)
75-76:⚠️ Potential issue | 🟡 MinorTighten hostname matching to prevent unintended reload bypasses.
endsWith("mediaoverquic.com")also matches unrelated hosts (for example,notmediaoverquic.com), which can incorrectly skip announcement gating. Match exact host or dot-boundary subdomains only.Proposed fix
+const ANNOUNCEMENT_UNSUPPORTED_HOST = "mediaoverquic.com"; + const conn = effect.get(this.connection); - if (conn?.url.hostname.endsWith("mediaoverquic.com")) { + const hostname = conn?.url.hostname; + if ( + hostname === ANNOUNCEMENT_UNSUPPORTED_HOST || + hostname?.endsWith(`.${ANNOUNCEMENT_UNSUPPORTED_HOST}`) + ) { console.warn("Cloudflare relay does not support broadcast discovery yet; ignoring reload signal."); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/broadcast.ts` around lines 75 - 76, The hostname check using conn?.url.hostname.endsWith("mediaoverquic.com") can match unintended hosts (e.g., notmediaoverquic.com); update the gating in broadcast.ts to only allow an exact match or real subdomains by changing the condition to require hostname === "mediaoverquic.com" OR hostname.endsWith(".mediaoverquic.com") (ensure you still guard for conn?.url.hostname existence). Locate the check around conn?.url.hostname in the broadcast/reload handling and replace the endsWith-only test with the exact-or-dot-prefixed-subdomain logic to prevent false positives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/watch/src/broadcast.ts`:
- Around line 75-76: The hostname check using
conn?.url.hostname.endsWith("mediaoverquic.com") can match unintended hosts
(e.g., notmediaoverquic.com); update the gating in broadcast.ts to only allow an
exact match or real subdomains by changing the condition to require hostname ===
"mediaoverquic.com" OR hostname.endsWith(".mediaoverquic.com") (ensure you still
guard for conn?.url.hostname existence). Locate the check around
conn?.url.hostname in the broadcast/reload handling and replace the
endsWith-only test with the exact-or-dot-prefixed-subdomain logic to prevent
false positives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 996e3cf3-ba49-4854-8824-8289cb35a9f6
📒 Files selected for processing (3)
js/lite/src/connection/reload.tsjs/publish/src/audio/encoder.tsjs/watch/src/broadcast.ts
💤 Files with no reviewable changes (1)
- js/lite/src/connection/reload.ts
✅ Files skipped from review due to trivial changes (1)
- js/publish/src/audio/encoder.ts
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.
Summary
Updated broadcast and connection handling to properly support relays that don't implement announcement subscriptions (like Cloudflare's relay). Instead of waiting indefinitely for announcements that will never arrive, the system now signals this unsupported state with
undefinedand behaves gracefully.Key Changes
announcedgetter type fromSet<Path.Valid>toSet<Path.Valid> | undefinedto distinguish between "supported but empty" and "not supported"announcedtoundefinedinstead of silently skipping announcement subscriptionsundefinedannounced state asreload=false, preventing indefinite waiting for announcements that will never arriveundefinedstateImplementation Details
undefinedvalue is semantically distinct from an emptySet(), allowing consumers to differentiate between "relay doesn't support announcements" vs "relay supports announcements but none are currently active"https://claude.ai/code/session_01ErUfz2GBh8U4pbPhdvawr1