Conversation
📝 WalkthroughWalkthroughThis PR removes debug logging infrastructure from the WebSocket module system. It deletes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
services/websocket/useWebSocketHealth.ts (1)
74-104: Nit: reuse thetokenChangedvariable instead of recomputing on line 99.
currentToken !== previousTokenon line 99 is exactly thetokenChangedvalue already computed on line 76. Reusing it makes the third branch's intent clearer and avoids drift if the comparison is ever extended (e.g., to consider whitespace or length).♻️ Proposed tidy-up
} else if ( currentToken && currentStatus !== WebSocketStatus.DISCONNECTED && - currentToken !== previousToken + tokenChanged ) { currentConnect(currentToken); action = "connect"; reason = "token-changed"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/websocket/useWebSocketHealth.ts` around lines 74 - 104, Replace the direct comparison currentToken !== previousToken in the third conditional with the already-computed tokenChanged variable to avoid duplicate logic; update the condition that now reads (currentToken && currentStatus !== WebSocketStatus.DISCONNECTED && currentToken !== previousToken) to use tokenChanged (i.e., tokenChanged instead of currentToken !== previousToken) so the branch uses the single source of truth and preserves the existing calls to currentConnect(currentToken) and the action/reason assignments.__tests__/services/websocket/WebSocketProvider.test.tsx (1)
349-379: Optional: strengthen this test to verify subscriber isolation, not just "doesn't throw".With the
console.errorassertion gone, this test now only proves the faulty callback was invoked. The real behavioral guarantee of the newfor..of+ per-subscribertry/catchinhandleMessageis that a subsequent subscriber on the same message type still fires even if an earlier one throws. Adding a second subscriber would make that guarantee regression-proof:🧪 Proposed test enhancement
const faultyCallback = jest.fn(() => { throw new Error("Callback error"); }); + const healthyCallback = jest.fn(); act(() => { result.current.subscribe(WsMessageType.DROP_UPDATE, faultyCallback); + result.current.subscribe(WsMessageType.DROP_UPDATE, healthyCallback); }); act(() => { ws.triggerMessage({ type: WsMessageType.DROP_UPDATE, data: { id: 1 } }); }); expect(faultyCallback).toHaveBeenCalledWith({ id: 1 }); + expect(healthyCallback).toHaveBeenCalledWith({ id: 1 });Same spirit applies to
"handles malformed messages gracefully"— asserting that a subscribed callback was not invoked would meaningfully verify the parse-failure early return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/services/websocket/WebSocketProvider.test.tsx` around lines 349 - 379, The test currently only asserts the faulty subscriber was called; update "handles subscriber callback errors gracefully" to also register a second subscriber (e.g., a jest.fn named successCallback) for the same WsMessageType after subscribing faultyCallback, trigger the message via MockWebSocket.triggerMessage, and assert both that faultyCallback was called and that successCallback was also called with the same payload to verify subscriber isolation in handleMessage; similarly, for the "handles malformed messages gracefully" test, add a subscribed callback and assert it was NOT called after sending a malformed message so the early-return on parse failure is enforced.services/websocket/WebSocketProvider.tsx (1)
130-136: Consider preserving observability when swallowing subscriber errors.Removing the
console.errorhere is consistent with the PR goal, but it now leaves production subscriber bugs completely invisible — no breadcrumb, no counter, no sampling. If there is an existing error-reporting/telemetry sink (Sentry, a logger service, etc.), forwarding the caught error through it (even at debug/info level) would retain diagnostic value without re-introducing the noisyconsole.errorthe PR is trying to remove. Same consideration applies to the silent parse failure at line 114 and the non-string guard.Not a blocker — flagging as an observability trade-off to weigh against the logging cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/websocket/WebSocketProvider.tsx` around lines 130 - 136, The loop over subscribers currently swallows exceptions from individual handlers (for subscriber(message.data)) with no observability; modify the catch to forward the caught error to the project's telemetry/logging facility (e.g., call the existing error reporter or logger) with context about the failing subscriber and message.data, so one subscriber failure doesn't block others but is recorded. Also apply the same pattern to the earlier silent parse failure and the non-string guard: when JSON.parse or the type check fails, log/forward the error and contextual info (raw payload) to the telemetry sink at a low/noise level rather than completely dropping it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/services/websocket/WebSocketProvider.test.tsx`:
- Around line 349-379: The test currently only asserts the faulty subscriber was
called; update "handles subscriber callback errors gracefully" to also register
a second subscriber (e.g., a jest.fn named successCallback) for the same
WsMessageType after subscribing faultyCallback, trigger the message via
MockWebSocket.triggerMessage, and assert both that faultyCallback was called and
that successCallback was also called with the same payload to verify subscriber
isolation in handleMessage; similarly, for the "handles malformed messages
gracefully" test, add a subscribed callback and assert it was NOT called after
sending a malformed message so the early-return on parse failure is enforced.
In `@services/websocket/useWebSocketHealth.ts`:
- Around line 74-104: Replace the direct comparison currentToken !==
previousToken in the third conditional with the already-computed tokenChanged
variable to avoid duplicate logic; update the condition that now reads
(currentToken && currentStatus !== WebSocketStatus.DISCONNECTED && currentToken
!== previousToken) to use tokenChanged (i.e., tokenChanged instead of
currentToken !== previousToken) so the branch uses the single source of truth
and preserves the existing calls to currentConnect(currentToken) and the
action/reason assignments.
In `@services/websocket/WebSocketProvider.tsx`:
- Around line 130-136: The loop over subscribers currently swallows exceptions
from individual handlers (for subscriber(message.data)) with no observability;
modify the catch to forward the caught error to the project's telemetry/logging
facility (e.g., call the existing error reporter or logger) with context about
the failing subscriber and message.data, so one subscriber failure doesn't block
others but is recorded. Also apply the same pattern to the earlier silent parse
failure and the non-string guard: when JSON.parse or the type check fails,
log/forward the error and contextual info (raw payload) to the telemetry sink at
a low/noise level rather than completely dropping it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be7c4823-e067-46f6-ae74-4d4ffa934b4c
📒 Files selected for processing (8)
__tests__/contexts/wave/MyStreamContext.resume.test.tsx__tests__/services/websocket/WebSocketProvider.test.tsxcontexts/wave/MyStreamContext.tsxhooks/useWaveIsTyping.tshooks/useWaveWebSocket.tsservices/websocket/WebSocketProvider.tsxservices/websocket/useWebSocketHealth.tsservices/websocket/webSocketDebug.ts
💤 Files with no reviewable changes (2)
- tests/contexts/wave/MyStreamContext.resume.test.tsx
- services/websocket/webSocketDebug.ts
2 participants
Summary by CodeRabbit