Avoid logging the WebSocket fallback even if it's not being used. #576
Conversation
WalkthroughRefactors WebSocket fallback by moving headstart delay and cancellation gating into connectWebSocket (now accepts a delay). Adds Promise.race-based preflight to abort if canceled before delay, logs when delayed fallback runs, and simplifies outer connect() while preserving zero-delay behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Caller
participant Conn as connect()
participant WT as WebTransport
participant WS as connectWebSocket(delay,cancel)
App->>Conn: connect(url, headstart)
par WebTransport attempt
Conn->>WT: start WT connection
WT-->>Conn: success or failure
and WebSocket fallback (delayed)
Conn->>WS: start with delay=headstart, cancel
WS->>WS: Promise.race(cancel, timer(delay))
alt canceled before delay
WS-->>Conn: return undefined
else delay elapsed
WS->>WS: initiate WebSocket connect
WS-->>Conn: WS result (success/failure)
end
end
alt First successful transport
Conn-->>App: return transport
else Both fail / canceled
Conn-->>App: propagate failure/undefined
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
js/moq/src/connection.ts (1)
169-173: Gating logic is correct; tiny naming nit.
activeholds a truthy “proceed” flag (boolean|void). Consider renaming for clarity.- const active = await Promise.race([cancel, timer.then(() => true)]); - if (!active) return undefined; + const proceed = await Promise.race([cancel, timer.then(() => true)]); + if (!proceed) return undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
js/moq/src/connection.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/moq/src/connection.ts
🔇 Additional comments (1)
js/moq/src/connection.ts (1)
65-65: connectWebSocket call sites verified
Search withrg -nP '\bconnectWebSocket\s*\(' js/shows only the updated invocation at line 65 and the function definition at line 168—no stale call sites remain.
| if (delay) { | ||
| console.debug(url.toString(), `no WebTransport after ${delay}ms, attempting WebSocket fallback`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove pre-connection fallback log to meet PR objective.
This debug line will log even when QUIC wins after the delay (fallback not used), which contradicts “Avoid logging the WebSocket fallback even if it's not being used.” The existing warn at Line 82 already logs when WebSocket actually wins.
- if (delay) {
- console.debug(url.toString(), `no WebTransport after ${delay}ms, attempting WebSocket fallback`);
- }📝 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.
| if (delay) { | |
| console.debug(url.toString(), `no WebTransport after ${delay}ms, attempting WebSocket fallback`); | |
| } | |
| // – if (delay) { | |
| // – console.debug(url.toString(), `no WebTransport after ${delay}ms, attempting WebSocket fallback`); | |
| // – } |
🤖 Prompt for AI Agents
In js/moq/src/connection.ts around lines 174-176, remove the pre-connection
debug log that prints a fallback message after the delay; this log can fire even
when QUIC succeeds and contradicts the PR objective. Either delete those two
lines entirely or move the logging so it only executes in the actual
WebSocket-fallback branch (the same place where the existing warn at line 82
logs when WebSocket wins), ensuring no fallback message is emitted unless
WebSocket is actually used.
Summary by CodeRabbit
Refactor
Chore