Merge upstream: Add WebSocket disconnect recovery and slow RPC toast UX (#1730)#49
Merged
aaditagrawal merged 4 commits intomainfrom Apr 5, 2026
Merged
Conversation
Co-authored-by: Hwanseo Choi <hwanseoc@nvidia.com> Co-authored-by: codex <codex@users.noreply.github.com>
This commit depends on clientTracing from upstream pingdotgg#1739 (OTLP trace proxy), which is in a separate PR (#48). This stub provides the required exports so this PR compiles independently. The real implementation from PR #48 will replace this stub when merged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughImplements comprehensive WebSocket/reconnect infrastructure: connection state atom, transport session management with reconnect/resubscribe and reconnection UI, RPC request-latency tracking, transport-error classification/sanitization, wiring for resubscribe recovery, and test + API signature updates to surface resubscribe hooks. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant WsTransport as WsTransport
participant WsConnectionState as WsConnectionState
participant RpcProtocol as RpcProtocol
participant SlowRpcTracker as SlowRpcTracker
participant ToastCoordinator as ToastCoordinator
Browser->>WsTransport: start connection
WsTransport->>WsConnectionState: recordWsConnectionAttempt()
WsTransport->>RpcProtocol: socket opens
alt open succeeded
RpcProtocol->>WsConnectionState: recordWsConnectionOpened()
WsConnectionState->>ToastCoordinator: update (connected)
else open failed / error
RpcProtocol->>WsConnectionState: recordWsConnectionErrored(msg)
WsConnectionState->>ToastCoordinator: update (error/retrying)
ToastCoordinator->>Browser: show retry/offline toast
end
Browser->>RpcProtocol: send RPC request(id, tag)
RpcProtocol->>SlowRpcTracker: trackRpcRequestSent(id, tag)
SlowRpcTracker->>SlowRpcTracker: schedule 2.5s threshold
alt ack before threshold
RpcProtocol->>SlowRpcTracker: acknowledgeRpcRequest(id)
SlowRpcTracker->>ToastCoordinator: clear slow toast if needed
else no ack
SlowRpcTracker->>SlowRpcTracker: mark as slow
SlowRpcTracker->>ToastCoordinator: show slow-request toast
end
alt disconnect occurs
RpcProtocol->>WsConnectionState: recordWsConnectionClosed(code)
WsConnectionState->>WsTransport: compute nextRetryAt / waiting
WsTransport->>WsTransport: reconnect() after delay
WsTransport->>RpcProtocol: new socket open (resubscribe)
RpcProtocol->>WsConnectionState: recordWsConnectionOpened()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
bot
added
vouch:trusted
![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 (3)
apps/web/src/rpc/transportError.test.ts (1)
5-24: Consider adding edge case tests for robustness.The current tests cover the main scenarios well. For completeness, consider adding tests for edge cases that the implementation handles:
null/undefinedinputs to both functions- Empty string or whitespace-only inputs
- Case variations (the regex patterns use
/iflag)💡 Optional: Additional edge case tests
+ it("handles null/undefined inputs", () => { + expect(isTransportConnectionErrorMessage(null)).toBe(false); + expect(isTransportConnectionErrorMessage(undefined)).toBe(false); + expect(sanitizeThreadErrorMessage(null)).toBeNull(); + expect(sanitizeThreadErrorMessage(undefined)).toBeNull(); + }); + + it("handles empty/whitespace strings", () => { + expect(isTransportConnectionErrorMessage("")).toBe(false); + expect(isTransportConnectionErrorMessage(" ")).toBe(false); + expect(sanitizeThreadErrorMessage("")).toBe(""); + }); + + it("matches case-insensitively", () => { + expect(isTransportConnectionErrorMessage("socketcloseerror: 1006")).toBe(true); + expect(isTransportConnectionErrorMessage("SOCKETOPENERROR: TIMEOUT")).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/rpc/transportError.test.ts` around lines 5 - 24, Add edge-case unit tests for isTransportConnectionErrorMessage and sanitizeThreadErrorMessage: include assertions for null and undefined inputs, empty string and whitespace-only strings, and case-variation inputs (e.g., mixed/capitalized versions of known transport messages) to verify the functions handle these inputs consistently; locate tests near the existing describe("transportError") block and add cases that expect boolean results for isTransportConnectionErrorMessage and expect sanitized string, original string, or null for sanitizeThreadErrorMessage as appropriate.apps/web/src/wsRpcClient.ts (1)
20-22: Reuse the contract's subscription-options type here.The same
onResubscribeshape now lives in bothpackages/contracts/src/ipc.ts, Lines 197-202 and this local interface. Deriving it fromNativeApikeeps future option changes to one edit instead of two and avoids drift between the public contract and the transport-facing client.Suggested refactor
-interface StreamSubscriptionOptions { - readonly onResubscribe?: () => void; -} +type StreamSubscriptionOptions = NonNullable< + Parameters<NativeApi["orchestration"]["onDomainEvent"]>[1] +>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/wsRpcClient.ts` around lines 20 - 22, Replace the local StreamSubscriptionOptions declaration with the subscription options type from the public contract so the shape is single-sourced: import the contract type from NativeApi (or the exact subscription/options export inside it) and use that type (e.g., type StreamSubscriptionOptions = /* contract's subscription/options type */) instead of redefining onResubscribe locally; update any call sites that referenced the local interface to use the imported/aliased type (refer to StreamSubscriptionOptions and NativeApi to locate the code to change).apps/web/src/wsTransport.test.ts (1)
676-692: Consider awaiting the dispose call for deterministic cleanup.The test calls
void WsTransport.prototype.dispose.call(transport)without awaiting. While this tests the synchronous disposal flag behavior, the comment at line 692 indicates intentional non-awaiting. This is fine for testing the immediate flag set, but be aware thatcallOrderassertions at line 704 depend onwaitForpolling rather than deterministic sequencing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/wsTransport.test.ts` around lines 676 - 692, The test invokes WsTransport.prototype.dispose via "void WsTransport.prototype.dispose.call(transport)" without awaiting, causing non-deterministic async cleanup; change the test to await the disposal (e.g. make the test async and replace the void call with "await WsTransport.prototype.dispose.call(transport)") so the runtime.dispose and runPromise work complete deterministically before asserting callOrder (and then you can simplify/remove the waitFor polling if desired). Ensure you update the test function signature to async and reference WsTransport.prototype.dispose, transport.session.runtime.dispose, and transport.session.runtime.runPromise where relevant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/wsNativeApi.ts`:
- Around line 11-16: The test reset helper __resetWsNativeApiForTests currently
calls __resetWsRpcClientForTests() without awaiting it; since
__resetWsRpcClientForTests is async and only clears the shared client after
disposal completes, change __resetWsNativeApiForTests to await
__resetWsRpcClientForTests() so disposal finishes before proceeding to
resetRequestLatencyStateForTests(), resetServerStateForTests(), and
resetWsConnectionStateForTests(); update the function signature if necessary to
be async to accommodate the await.
---
Nitpick comments:
In `@apps/web/src/rpc/transportError.test.ts`:
- Around line 5-24: Add edge-case unit tests for
isTransportConnectionErrorMessage and sanitizeThreadErrorMessage: include
assertions for null and undefined inputs, empty string and whitespace-only
strings, and case-variation inputs (e.g., mixed/capitalized versions of known
transport messages) to verify the functions handle these inputs consistently;
locate tests near the existing describe("transportError") block and add cases
that expect boolean results for isTransportConnectionErrorMessage and expect
sanitized string, original string, or null for sanitizeThreadErrorMessage as
appropriate.
In `@apps/web/src/wsRpcClient.ts`:
- Around line 20-22: Replace the local StreamSubscriptionOptions declaration
with the subscription options type from the public contract so the shape is
single-sourced: import the contract type from NativeApi (or the exact
subscription/options export inside it) and use that type (e.g., type
StreamSubscriptionOptions = /* contract's subscription/options type */) instead
of redefining onResubscribe locally; update any call sites that referenced the
local interface to use the imported/aliased type (refer to
StreamSubscriptionOptions and NativeApi to locate the code to change).
In `@apps/web/src/wsTransport.test.ts`:
- Around line 676-692: The test invokes WsTransport.prototype.dispose via "void
WsTransport.prototype.dispose.call(transport)" without awaiting, causing
non-deterministic async cleanup; change the test to await the disposal (e.g.
make the test async and replace the void call with "await
WsTransport.prototype.dispose.call(transport)") so the runtime.dispose and
runPromise work complete deterministically before asserting callOrder (and then
you can simplify/remove the waitFor polling if desired). Ensure you update the
test function signature to async and reference WsTransport.prototype.dispose,
transport.session.runtime.dispose, and transport.session.runtime.runPromise
where relevant.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 452376d7-8742-4b6c-b53c-1be6b7710cbf
📒 Files selected for processing (20)
apps/web/src/components/ChatView.tsxapps/web/src/components/WebSocketConnectionSurface.logic.test.tsapps/web/src/components/WebSocketConnectionSurface.tsxapps/web/src/components/ui/toast.tsxapps/web/src/orchestrationRecovery.tsapps/web/src/routes/__root.tsxapps/web/src/rpc/protocol.tsapps/web/src/rpc/requestLatencyState.test.tsapps/web/src/rpc/requestLatencyState.tsapps/web/src/rpc/transportError.test.tsapps/web/src/rpc/transportError.tsapps/web/src/rpc/wsConnectionState.test.tsapps/web/src/rpc/wsConnectionState.tsapps/web/src/store.tsapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tsapps/web/src/wsRpcClient.tsapps/web/src/wsTransport.test.tsapps/web/src/wsTransport.tspackages/contracts/src/ipc.ts
__resetWsRpcClientForTests() is async but was called without await, risking stale client leaking into subsequent tests. Propagate async through the call chain: wsNativeApi, nativeApi, and all browser test beforeEach/afterEach hooks.
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.
What
Cherry-picks upstream commit
f2cd53f2(PR #1730) onto the fork.Upstream changes
Adds WebSocket disconnect recovery and slow RPC toast UX:
WebSocketConnectionSurface-- New component that shows connection status (disconnected/reconnecting/slow) with toast notificationswsConnectionState-- State machine tracking WS connection lifecycle (connected/disconnected/reconnecting)requestLatencyState-- Tracks RPC request latency to surface "slow" warningstransportError-- Structured transport error typeswsTransport.ts-- Major refactor to session-based architecture with reconnect support, retry loops for streams, and graceful disconnect handlingorchestrationRecovery.ts-- Hooks into reconnect for orchestration replay__root.tsx-- Integrates connection surface into the app shellConflict resolution
wsTransport.ts(7 conflict regions) -- Adopted upstream's session-based architecture replacing the old per-instance runtime pattern. Fork had no unique changes to these code paths.wsTransport.test.ts(2 conflict regions) -- Merged upstream's new test utilities and cleanup hooks.Additional changes
clientTracing.tsstub so this PR compiles standalone without PR Merge upstream: Proxy browser OTLP traces through the server (#1739) #48 (upstream Proxy browser OTLP traces through the server pingdotgg/t3code#1739 OTLP trace proxy). The real implementation will replace it when Merge upstream: Proxy browser OTLP traces through the server (#1739) #48 merges.Verification
bun typecheckpasses (all 7 packages)bun fmtandbun lintcleanwsTransport(12),wsConnectionState(5),requestLatencyState(4),transportError(3),WebSocketConnectionSurface(3) -- 27 tests totalSummary by CodeRabbit
New Features
Bug Fixes
Quality