Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds localized WebSocket lifecycle status messages and a WebSocketStatusSnackbar UI; implements a ping/pong health-check module (setupWebSocketPing) that detects dead connections and triggers reconnection; updates WebSocketGate to wire ping monitoring, handle errors/close/online-offline events, and show status to users; adds a no-op handler and new inbound event key for calendar client registration; introduces test scaffolding (TestWrapper, richer socket mocks, ping lifecycle mocks, and integration tests); and adds websocket translations and test globals for ping timings. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 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 |
Eriikah
force-pushed
the
474-websocket-add-a-snack-bar-to-notify-the-user-on-websocket-status-changes
branch
from
5b50454 to
7200636
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/websocket/WebSocketGate.tsx`:
- Around line 103-106: The onError handler currently passes the raw Event into
i18n causing “[object Event]” output; update the onError callback to extract a
user-friendly string (e.g., prefer (error as CloseEvent).reason, (error as
ErrorEvent).message, or fall back to error.type || "unknown error") then pass
that string to t and to setWebSocketStatus; keep using the same onError
useCallback and update calls to setWebSocketStatus and setWebSocketStatusSerity
to use the extracted message variable.
In `@src/websocket/WebSocketStatusSnackbar.tsx`:
- Around line 21-31: The Alert content in WebSocketStatusSnackbar.jsx/tsx is
rendering localized messages that include "\n" but HTML collapses whitespace;
update the component that renders {message} (inside the Alert in
WebSocketStatusSnackbar, using props severity/onClose/message and i18n t()) so
line breaks are preserved—either apply style whiteSpace: "pre-line" to the Alert
content or wrap {message} in an element (e.g., a span or Typography) with
sx/style={{ whiteSpace: "pre-line" }} to ensure newline characters are rendered.
| <Alert | ||
| severity={severity} | ||
| onClose={onClose} | ||
| sx={{ width: "100%" }} | ||
| action={ | ||
| <Button color="inherit" size="small" onClick={onClose}> | ||
| {t("common.ok")} | ||
| </Button> | ||
| } | ||
| > | ||
| {message} |
There was a problem hiding this comment.
Preserve intended line breaks in localized messages.
The locale strings include \n, but HTML collapses whitespace. Add whiteSpace: "pre-line" (or wrap the message) so the line break actually renders.
🛠️ Suggested fix
- <Alert
- severity={severity}
- onClose={onClose}
- sx={{ width: "100%" }}
+ <Alert
+ severity={severity}
+ onClose={onClose}
+ sx={{ width: "100%", whiteSpace: "pre-line" }}
action={
<Button color="inherit" size="small" onClick={onClose}>
{t("common.ok")}
</Button>
}
>
{message}
</Alert>📝 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.
| <Alert | |
| severity={severity} | |
| onClose={onClose} | |
| sx={{ width: "100%" }} | |
| action={ | |
| <Button color="inherit" size="small" onClick={onClose}> | |
| {t("common.ok")} | |
| </Button> | |
| } | |
| > | |
| {message} | |
| <Alert | |
| severity={severity} | |
| onClose={onClose} | |
| sx={{ width: "100%", whiteSpace: "pre-line" }} | |
| action={ | |
| <Button color="inherit" size="small" onClick={onClose}> | |
| {t("common.ok")} | |
| </Button> | |
| } | |
| > | |
| {message} | |
| </Alert> |
🤖 Prompt for AI Agents
In `@src/websocket/WebSocketStatusSnackbar.tsx` around lines 21 - 31, The Alert
content in WebSocketStatusSnackbar.jsx/tsx is rendering localized messages that
include "\n" but HTML collapses whitespace; update the component that renders
{message} (inside the Alert in WebSocketStatusSnackbar, using props
severity/onClose/message and i18n t()) so line breaks are preserved—either apply
style whiteSpace: "pre-line" to the Alert content or wrap {message} in an
element (e.g., a span or Typography) with sx/style={{ whiteSpace: "pre-line" }}
to ensure newline characters are rendered.
|
Am I missing something but turning on-and-off wifi I was not able to see thos snackbar... |
|
That's weird indeed, I've updated the docker image just in case |
|
So it seems the window.listener isn't really transmitting the reality of the connection. We'll have to send a ping to the websocket to handle true interet deconnection. |
Eriikah
force-pushed
the
474-websocket-add-a-snack-bar-to-notify-the-user-on-websocket-status-changes
branch
from
554090a to
8bc5114
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/websocket/connection/lifecycle/pingWebSocket.ts`:
- Around line 139-163: The wrapper on socket.onmessage currently calls
handlePong() for every message and isn't fully removed when originalOnMessage is
undefined; update the handler in the pingWebSocket lifecycle so it first
inspects the incoming event (e.g., parse event.data safely and check the exact
pong shape such as a { type: "pong" } or backend-specific property) and only
call handlePong() / clear isWaitingForPong / invoke onPongReceived() when that
validation passes, forward the event to originalOnMessage if present, and on
stop() always detach the wrapper by restoring socket.onmessage to
originalOnMessage if defined or to undefined/null otherwise so the pong handler
cannot fire after cleanup; reference functions/vars: handlePong,
socket.onmessage, originalOnMessage, startPinging, stop, cleanup,
isWaitingForPong, onPongReceived.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/websocket/connection/lifecycle/pingWebSocket.ts`:
- Around line 22-23: DEFAULT_PING_INTERVAL and DEFAULT_PONG_TIMEOUT can be
undefined if window.WS_PING_PERIOD_MS or window.WS_PING_TIMEOUT_PERIOD_MS are
absent, which will coerce to 0 and cause immediate timers; update their
definitions in pingWebSocket.ts to use safe fallbacks (e.g.,
window.WS_PING_PERIOD_MS ?? 30000 and window.WS_PING_TIMEOUT_PERIOD_MS ?? 5000)
or coerce to Number with a fallback, so the values passed to
setInterval/setTimeout are valid non-zero numbers when undefined.
🧹 Nitpick comments (2)
src/setupTests.ts (1)
63-66: Consider moving WebSocket globals outside thegetComputedStyleblock.The
WS_PING_PERIOD_MSandWS_PING_TIMEOUT_PERIOD_MSassignments are unrelated to thegetComputedStyleoverride. Placing them inside this conditional block is misleading since they aren't dependent onwindow.getComputedStyleexisting.♻️ Suggested refactor
+(window as any).WS_PING_PERIOD_MS = 5000; +(window as any).WS_PING_TIMEOUT_PERIOD_MS = 5000; + // Suppress jsdom CSS selector parsing errors for Emotion/MUI if (typeof window !== "undefined" && window.getComputedStyle) { const originalGetComputedStyle = window.getComputedStyle; - (window as any).WS_PING_PERIOD_MS = 5000; - (window as any).WS_PING_TIMEOUT_PERIOD_MS = 5000; window.getComputedStyle = function (__test__/features/websocket/WebSocketGate.test.tsx (1)
906-920: UnusedmockPingCleanupvariable and misleading test suite name.The
mockPingCleanupobject (lines 907-914) is declared but never used in any test within this suite. Additionally, sincepingWebSocketis mocked at line 14, these tests don't actually exercise ping/pong behavior—they test close event handling and reconnection logic, which is already covered by the "Reconnection Logic" suite.Consider either:
- Removing the unused
mockPingCleanupand renaming the suite to reflect what it actually tests- Or implementing actual ping/pong integration tests that verify
setupWebSocketPingis invoked with correct callbacks♻️ Option 1: Remove unused variable
describe("Ping/Pong Integration", () => { - let mockPingCleanup: { stop: jest.Mock; sendPing: jest.Mock }; - beforeEach(() => { jest.useFakeTimers(); - mockPingCleanup = { - stop: jest.fn(), - sendPing: jest.fn(), - }; });
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/websocket/connection/lifecycle/pingWebSocket.ts`:
- Around line 74-112: The sendPing function can throw from socket.send and
although cleanup() is called, the interval may fire again before isStopped is
set; modify the catch block in sendPing to set isStopped = true (and clear any
timers/ids if cleanup doesn't already) so no further ping attempts occur after a
send failure; update references in sendPing to ensure isStopped is set before
calling cleanup and that pingIntervalId and pongTimeoutId are cleared (or that
cleanup clears them) to fully stop the retry loop.
|
(rebase needed) |
Eriikah
force-pushed
the
474-websocket-add-a-snack-bar-to-notify-the-user-on-websocket-status-changes
branch
from
1b9184d to
e5185a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/websocket/connection/lifecycle/pingWebSocket.ts`:
- Around line 80-88: When detecting a dead connection in the isWaitingForPong
branch, set the module/state flag isStopped = true before invoking
onPingFail/onConnectionDead and calling cleanup so external callers (e.g.,
sendPing) cannot be used after the connection is marked dead; update the block
that currently references isWaitingForPong, onPingFail, onConnectionDead,
cleanup and sendPing to assign isStopped = true first, then call the callbacks
and cleanup.
🧹 Nitpick comments (1)
src/websocket/connection/lifecycle/pingWebSocket.ts (1)
90-94: Misleading comment: this is an application-level ping, not a WebSocket protocol-level ping frame.Lines 91-93 reference "WebSocket protocol-level ping" and state "Most WebSocket implementations handle this automatically," but
socket.send(...)sends a regular data frame, not a protocol-level ping frame (which is only accessible server-side in most environments). The comment could confuse future maintainers.📝 Suggested comment update
- // Send a ping frame (WebSocket protocol-level ping) - // Most WebSocket implementations handle this automatically - // If your server expects a custom ping message, modify this: + // Send an application-level ping message socket.send(JSON.stringify({ type: "ping", timestamp: Date.now() }));
| if (isWaitingForPong) { | ||
| console.warn( | ||
| "Pong not received for previous ping. Connection may be dead." | ||
| ); | ||
| onPingFail?.(); | ||
| onConnectionDead?.(); | ||
| cleanup(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
isStopped not set when connection is deemed dead.
When isWaitingForPong is true and sendPing fires again, onConnectionDead is called and timers are cleaned up, but isStopped remains false. This means the externally-exposed sendPing() (line 176-179) can still be called after the connection is considered dead, potentially triggering callbacks on a broken socket.
🛠️ Proposed fix
if (isWaitingForPong) {
console.warn(
"Pong not received for previous ping. Connection may be dead."
);
onPingFail?.();
onConnectionDead?.();
+ isStopped = true;
cleanup();
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.
| if (isWaitingForPong) { | |
| console.warn( | |
| "Pong not received for previous ping. Connection may be dead." | |
| ); | |
| onPingFail?.(); | |
| onConnectionDead?.(); | |
| cleanup(); | |
| return; | |
| } | |
| if (isWaitingForPong) { | |
| console.warn( | |
| "Pong not received for previous ping. Connection may be dead." | |
| ); | |
| onPingFail?.(); | |
| onConnectionDead?.(); | |
| isStopped = true; | |
| cleanup(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@src/websocket/connection/lifecycle/pingWebSocket.ts` around lines 80 - 88,
When detecting a dead connection in the isWaitingForPong branch, set the
module/state flag isStopped = true before invoking onPingFail/onConnectionDead
and calling cleanup so external callers (e.g., sendPing) cannot be used after
the connection is marked dead; update the block that currently references
isWaitingForPong, onPingFail, onConnectionDead, cleanup and sendPing to assign
isStopped = true first, then call the callbacks and cleanup.
2 participants
related to #474
docker image on eriikaah/twake-calendar-front:issue-474-websocket-add-a-snack-bar-to-notify-the-user-on-websocket-status-changes
this PR depends on #472
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores