🔧 Refactor: Improve WebSocket error handling, reliability, and recovery#310
Open
SoorejS wants to merge 5 commits into
Open
🔧 Refactor: Improve WebSocket error handling, reliability, and recovery#310SoorejS wants to merge 5 commits into
SoorejS wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds centralized, throttled WebSocket error handling and wsErrorAtom integration to the debate hook; caps reconnection retries (3) with backoff; hardens message parsing and join/send logic; distinguishes normal vs abnormal closes; and improves safe cleanup on unmount while preserving public API (ws, sendMessage). Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as DebateHook
participant WS as WebSocket
participant Server as DebateServer
participant Store as AtomStore
Hook->>WS: open connection (send join with spectatorHash)
WS->>Server: join message
Server-->>WS: emits messages (poll_snapshot, vote, presence, question, reaction)
WS->>Hook: onmessage (raw payload)
Hook->>Hook: validate message (reject empty/malformed)
alt valid message
Hook->>Store: update atoms/state
Hook->>Hook: clear wsErrorAtom & reset retry state
else invalid / parse error
Hook->>Hook: handleError (throttled -> set wsErrorAtom)
Hook->>WS: attempt reconnect (backoff, capped at maxRetries)
end
WS-->>Hook: onerror / onclose
alt normal close (1000)
Hook->>Hook: treat as non-error, cleanup
else abnormal close
Hook->>Hook: handleError & reconnect logic
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/Pages/Authentication/forms.tsx (2)
29-37:⚠️ Potential issue | 🟡 Minor
login()call is not wrapped intry/catch, unlike other forms in this file.
SignUpForm,OTPVerificationForm, andResetPasswordFormall wrap their async auth calls in try/catch.LoginFormdoes not, so iflogin()rejects, it will produce an unhandled promise rejection. Apply the same pattern for consistency.Proposed fix
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); if (password.length < MIN_PASSWORD_LENGTH) { setLocalError(`Password must be at least ${MIN_PASSWORD_LENGTH} characters`); return; } setLocalError(null); - await login(email, password); + try { + await login(email, password); + } catch { + // Error is handled via AuthContext error state + } };
92-96:⚠️ Potential issue | 🟡 Minor
localErrorand globalerrorcan display simultaneously in LoginForm.In
SignUpForm(line 257), you guard the global error with{error && !localError && ...}, so only one error shows at a time. InLoginForm, bothlocalError(line 92) anderror(line 108) can render simultaneously, which is inconsistent and could confuse users with two red error messages.Also applies to: 108-108
🤖 Fix all issues with AI agents
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 383-386: The onerror and onclose handlers on the websocket
(rws.onerror and rws.onclose) both call handleReconnect, causing duplicate
reconnection attempts; remove the call from one handler (preferably remove
handleReconnect from rws.onerror and keep it in rws.onclose), or implement a
single guarded reconnection trigger inside handleReconnect (e.g., an inFlight
flag) so only one reconnection is initiated per failure; update the event
handlers referencing rws.onerror, rws.onclose, and handleReconnect accordingly.
- Around line 24-27: The custom reconnection routine is conflicting with
ReconnectingWebSocket's internal reconnect logic: remove the handleReconnect
function and all invocations of it (references to handleReconnect(),
MAX_RETRIES, RETRY_BASE_DELAY) so reconnection is solely driven by the
ReconnectingWebSocket instance configured with reconnectionDelayGrowFactor and
maxRetries; instead, update only the ReconnectingWebSocket options (e.g.,
maxRetries) if you need a finite cap and ensure event handlers (onopen, onclose,
onerror, onmessage) only update state atoms (status/error) and do not call
rws.reconnect() or implement backoff logic.
- Around line 377-380: The catch block that handles message processing should
not call handleReconnect() because parse/data errors (e.g., JSON.parse failures
or empty payloads) indicate bad message data, not a broken socket; update the
catch in the message handler inside useDebateWS (the block currently calling
handleError(error, 'Error processing WebSocket message') and handleReconnect())
to only log the error via handleError (retain the existing message/context) and
remove the handleReconnect() call so reconnect logic remains triggered only by
connection-level events (e.g., onclose/onerror), leaving handleReconnect() used
elsewhere for actual socket failures.
- Around line 408-412: The cleanup currently calls rws.close(1000, 'Component
unmounting') when rws.readyState === WebSocket.OPEN and then always calls
rws.close() again; update the cleanup in useDebateWS (the code using the rws
variable) so close() is only invoked once — either remove the unconditional
rws.close() and keep the existing open-state close call, or change it to an else
branch that calls rws.close() when the socket isn't OPEN (e.g., else
rws.close()), ensuring you reference the same rws variable and preserve the
1000/reason close for the OPEN case.
- Around line 111-123: The problem is that setWsStatus('error') runs outside the
throttle guard in handleError, causing the status to update while
setWsError(errorMessage) is skipped; inside handleError (referencing
handleError, lastErrorTime, ERROR_THROTTLE_MS, setWsError, setWsStatus) either
move the setWsStatus('error') call into the same throttle branch so both status
and message update together when the guard allows, or ensure you always update
the error message alongside the status (e.g., compute and call setWsError every
invocation) so the UI never shows an 'error' status with a stale/null message;
adjust lastErrorTime.current only inside that branch accordingly.
🧹 Nitpick comments (2)
frontend/src/Pages/Authentication/forms.tsx (2)
132-133: RepetitivesetIsSubmitting(false)on each validation branch — considertry/finally.Each early-return validation branch manually calls
setIsSubmitting(false). This is fragile if more branches are added later. You can restructure usingtry/finallyto guaranteesetIsSubmitting(false)runs regardless of path.♻️ Proposed refactor
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setLocalError(null); setIsSubmitting(true); - // Basic validation - if (!email || !password || !confirmPassword) { - setLocalError('All fields are required'); - setIsSubmitting(false); - return; - } - - if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email)) { - setLocalError('Please enter a valid email address'); - setIsSubmitting(false); - return; - } - - if (password.length < 8) { - setLocalError('Password must be at least 8 characters long'); - setIsSubmitting(false); - return; - } - - if (password !== confirmPassword) { - setLocalError('Passwords do not match'); - setIsSubmitting(false); - return; - } - - try { - await signup(email, password); - startOtpVerification(email); - } catch (err) { - console.error('Signup error:', err); - setLocalError('Failed to create account. Please try again.'); - } finally { - setIsSubmitting(false); - } + try { + if (!email || !password || !confirmPassword) { + setLocalError('All fields are required'); + return; + } + if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email)) { + setLocalError('Please enter a valid email address'); + return; + } + if (password.length < 8) { + setLocalError('Password must be at least 8 characters long'); + return; + } + if (password !== confirmPassword) { + setLocalError('Passwords do not match'); + return; + } + await signup(email, password); + startOtpVerification(email); + } catch (err) { + console.error('Signup error:', err); + setLocalError('Failed to create account. Please try again.'); + } finally { + setIsSubmitting(false); + } };Also applies to: 144-180
160-163: Magic number8— use a shared constant for password minimum length.
LoginFormdefinesMIN_PASSWORD_LENGTH = 8, but here the same constraint uses a hard-coded8. Extract and share the constant to keep validation rules in sync.
SoorejS
force-pushed
the
fix-ws-error-handling
branch
from
February 8, 2026 17:47
d6a21a3 to
4de47c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/hooks/useDebateWS.ts (2)
385-389:⚠️ Potential issue | 🟡 MinorCleanup still calls
close()twice.When open,
rws.close(1000, 'Component unmounting')is followed by unconditionalrws.close(). This is redundant and can be confusing.💡 Proposed fix
try { if (rws.readyState === WebSocket.OPEN) { rws.close(1000, 'Component unmounting'); + } else { + rws.close(); } - rws.close(); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useDebateWS.ts` around lines 385 - 389, The cleanup currently calls rws.close() twice; update the effect cleanup in useDebateWS.ts so the WebSocket instance rws is closed only once—call rws.close(1000, 'Component unmounting') when rws.readyState === WebSocket.OPEN, otherwise call rws.close() in an else branch (or remove the unconditional second call entirely) so there is no duplicate close invocation; ensure you reference the same rws variable and keep the existing try/catch around the close call.
359-362:⚠️ Potential issue | 🔴 Critical
handleReconnectis called but not defined, causing a build break.
handleReconnect()is invoked in both handlers, but there is no declaration in this file. This will fail TypeScript compilation and breaks runtime recovery flow.💡 Proposed fix
rws.onerror = (error) => { handleError(error, 'WebSocket connection error'); - handleReconnect(); }; rws.onclose = (event) => { // Don't treat normal closure as error if (event.code !== 1000) { // 1000 is normal closure handleError(new Error(`Connection closed with code ${event.code}: ${event.reason || 'Unknown reason'}`)); - handleReconnect(); } else { setWsStatus('disconnected'); setWsError(null); }Also applies to: 364-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useDebateWS.ts` around lines 359 - 362, rws.onerror and rws.onclose call handleReconnect(), but handleReconnect is not defined causing a TypeScript build error; add a local function named handleReconnect in useDebateWS.ts that encapsulates the module's reconnect logic (e.g., cancel/clear any existing timers/refs and trigger the existing connect/reconnect routine used by this hook such as connectWebSocket or whichever function performs the WS (re)initialization), then call that function from rws.onerror and rws.onclose so both handlers compile and perform the intended reconnect/backoff behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 146-150: The current logic inside useDebateWS resets
retryCount.current and clears the UI error only when retryCount.current > 0,
which lets a previously set non-retry error remain after a successful message;
update the success-handler so that setWsError(null) is always called on any
successful incoming message (not conditioned on retryCount), and keep the
existing retryCount.current reset when appropriate (i.e., call setWsError(null)
unconditionally and then if (retryCount.current > 0) reset retryCount.current =
0).
---
Duplicate comments:
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 385-389: The cleanup currently calls rws.close() twice; update the
effect cleanup in useDebateWS.ts so the WebSocket instance rws is closed only
once—call rws.close(1000, 'Component unmounting') when rws.readyState ===
WebSocket.OPEN, otherwise call rws.close() in an else branch (or remove the
unconditional second call entirely) so there is no duplicate close invocation;
ensure you reference the same rws variable and keep the existing try/catch
around the close call.
- Around line 359-362: rws.onerror and rws.onclose call handleReconnect(), but
handleReconnect is not defined causing a TypeScript build error; add a local
function named handleReconnect in useDebateWS.ts that encapsulates the module's
reconnect logic (e.g., cancel/clear any existing timers/refs and trigger the
existing connect/reconnect routine used by this hook such as connectWebSocket or
whichever function performs the WS (re)initialization), then call that function
from rws.onerror and rws.onclose so both handlers compile and perform the
intended reconnect/backoff behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bca19eb-9793-43da-b9fc-a1733f38f7ec
📒 Files selected for processing (1)
frontend/src/hooks/useDebateWS.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/hooks/useDebateWS.ts (2)
146-150:⚠️ Potential issue | 🟡 MinorAlways clear
wsErrorafter a successfully processed message.At Line 147, clearing is conditional on
retryCount.current > 0, so a non-retry parse/processing error can remain visible even after later valid messages.Suggested fix
- // Reset error state on successful message - if (retryCount.current > 0) { - retryCount.current = 0; - setWsError(null); - } + // Reset error state on successful message + setWsError(null); + if (retryCount.current > 0) { + retryCount.current = 0; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useDebateWS.ts` around lines 146 - 150, The current success path in useDebateWS checks retryCount.current > 0 before clearing wsError, which can leave a stale error shown after a later valid message; update the success handling (the block that currently references retryCount.current and setWsError) to always call setWsError(null) whenever a message is successfully parsed/processed (and still reset retryCount.current when appropriate), ensuring wsError is cleared unconditionally on success.
363-376:⚠️ Potential issue | 🟠 MajorAvoid recreating a new socket instance in
onclosewhile usingReconnectingWebSocketretries.
oncloseclearsws/wsRef(Line 372, Line 375), andwsis in the effect deps (Line 404). That causes the hook to spin up a fresh socket instance on close, which can conflict with/backdoor around configured retry/backoff behavior.Suggested direction
rws.onclose = (event) => { // Don't treat normal closure as error if (event.code !== 1000) { // 1000 is normal closure handleError(new Error(`Connection closed with code ${event.code}: ${event.reason || 'Unknown reason'}`)); + setWsStatus('connecting'); + return; // keep refs; let current instance handle retries } else { setWsStatus('disconnected'); setWsError(null); + setWs(null); + if (wsRef.current === rws) { + wsRef.current = null; + } } - - setWs(null); - - if (wsRef.current === rws) { - wsRef.current = null; - } };In reconnecting-websocket, does a single instance automatically retry after abnormal close, and is creating a new instance from onclose considered an anti-pattern that can bypass/duplicate retry behavior?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useDebateWS.ts` around lines 363 - 376, The onclose handler is clearing the socket state (calling setWs(null) and setting wsRef.current = null) which, combined with ws being in the effect dependencies, causes a new ReconnectingWebSocket to be created and can bypass the built-in retry/backoff; instead, remove the calls that clear the instance from the onclose handler (delete setWs(null) and the wsRef.current reset) and only update observable status/error (e.g., keep setWsStatus and setWsError/handleError) so the existing rws (ReconnectingWebSocket instance) can perform its reconnection logic; ensure any explicit/manual close logic elsewhere still clears ws/wsRef when you intentionally stop reconnecting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 13-15: The import of wsErrorAtom in useDebateWS.ts is failing
because wsErrorAtom is not exported from debateAtoms.ts; open debateAtoms.ts,
define (or locate) the Atom for WS errors named wsErrorAtom if missing, and add
a named export (export const wsErrorAtom = ... or include it in the existing
export list) so the line "import { wsErrorAtom } from '../atoms/debateAtoms';"
resolves; then ensure useDebateWS.ts uses the same symbol (const [, setWsError]
= useAtom(wsErrorAtom)) and run TypeScript checks.
---
Duplicate comments:
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 146-150: The current success path in useDebateWS checks
retryCount.current > 0 before clearing wsError, which can leave a stale error
shown after a later valid message; update the success handling (the block that
currently references retryCount.current and setWsError) to always call
setWsError(null) whenever a message is successfully parsed/processed (and still
reset retryCount.current when appropriate), ensuring wsError is cleared
unconditionally on success.
- Around line 363-376: The onclose handler is clearing the socket state (calling
setWs(null) and setting wsRef.current = null) which, combined with ws being in
the effect dependencies, causes a new ReconnectingWebSocket to be created and
can bypass the built-in retry/backoff; instead, remove the calls that clear the
instance from the onclose handler (delete setWs(null) and the wsRef.current
reset) and only update observable status/error (e.g., keep setWsStatus and
setWsError/handleError) so the existing rws (ReconnectingWebSocket instance) can
perform its reconnection logic; ensure any explicit/manual close logic elsewhere
still clears ws/wsRef when you intentionally stop reconnecting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c73a53f9-b9dd-49bc-ac44-f688f81b0553
📒 Files selected for processing (1)
frontend/src/hooks/useDebateWS.ts
SoorejS
commented
Mar 13, 2026
… and used constant with error clearing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/hooks/useDebateWS.ts (1)
121-124: Remove stale comment about retry counter.The comment "Reset retry counter on successful connection" at line 122 is outdated—the manual reconnection logic and
retryCountref were removed. This comment is now misleading.Proposed fix
rws.onopen = () => { - // Reset retry counter on successful connection setWsStatus('connected'); setWsError(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useDebateWS.ts` around lines 121 - 124, The inline comment "Reset retry counter on successful connection" in the rws.onopen handler is stale and should be removed or replaced with an accurate comment; update the rws.onopen block (the handler that calls setWsStatus('connected') and setWsError(null)) to either delete that misleading comment or change it to a current description such as "mark connection as established" so the comment matches the existing logic (no retryCount/reconnection refs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 13-15: The import of wsErrorAtom in useDebateWS.ts fails because
wsErrorAtom is not exported from debateAtoms; add an export for wsErrorAtom
(e.g., export const wsErrorAtom = atom<string | null>(null)) inside the module
that defines the debate atoms (debateAtoms.ts) or, if wsErrorAtom is defined
elsewhere, correct the import path in useDebateWS.ts to point to the module that
actually exports wsErrorAtom; ensure the symbol name matches exactly and that
the module exports it so useDebateWS.ts can import wsErrorAtom successfully.
---
Nitpick comments:
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 121-124: The inline comment "Reset retry counter on successful
connection" in the rws.onopen handler is stale and should be removed or replaced
with an accurate comment; update the rws.onopen block (the handler that calls
setWsStatus('connected') and setWsError(null)) to either delete that misleading
comment or change it to a current description such as "mark connection as
established" so the comment matches the existing logic (no
retryCount/reconnection refs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae2e4a0b-b7f7-4dcd-89db-4514e00281a8
📒 Files selected for processing (1)
frontend/src/hooks/useDebateWS.ts
Comment on lines
+13
to
15
| wsErrorAtom, // <-- NEW | ||
| PollInfo, | ||
| } from '../atoms/debateAtoms'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that wsErrorAtom is exported from debateAtoms.ts
ATOM_FILE=$(fd 'debateAtoms\.ts$' | head -n1)
echo "=== Checking for wsErrorAtom in ${ATOM_FILE} ==="
# Check for export declaration
rg -n 'export\s+(const\s+)?wsErrorAtom' "${ATOM_FILE}" || echo "No direct export found"
# Check for atom declaration (might be exported elsewhere)
rg -n 'wsErrorAtom' "${ATOM_FILE}" || echo "wsErrorAtom not found in file"Repository: AOSSIE-Org/DebateAI
Length of output: 185
wsErrorAtom is not exported from debateAtoms.ts.
The import at line 13 will fail at runtime. Add the export to frontend/src/atoms/debateAtoms.ts:
export const wsErrorAtom = atom<string | null>(null);Or verify that wsErrorAtom is defined elsewhere if this import path is incorrect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useDebateWS.ts` around lines 13 - 15, The import of
wsErrorAtom in useDebateWS.ts fails because wsErrorAtom is not exported from
debateAtoms; add an export for wsErrorAtom (e.g., export const wsErrorAtom =
atom<string | null>(null)) inside the module that defines the debate atoms
(debateAtoms.ts) or, if wsErrorAtom is defined elsewhere, correct the import
path in useDebateWS.ts to point to the module that actually exports wsErrorAtom;
ensure the symbol name matches exactly and that the module exports it so
useDebateWS.ts can import wsErrorAtom successfully.
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
1 participant
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.
fixes issue #309
This PR addresses brittle error handling in
useDebateWS.ts, which previously included emptycatchblocks and several reliability gaps around WebSocket lifecycle management.What this PR changes (aligned with the issue)
✅ 1) Eliminates silent failures
Replaced empty
catch {}blocks with structured handling that:✅ 2) Adds application-level error state
Introduces a new atom in
frontend/src/atoms/debateAtoms.ts:This enables the UI (or other hooks) to observe and react to WebSocket failures instead of keeping them hidden.
✅ 3) Centralized error handling with throttling
Added a shared
handleErrorfunction that:5s) to avoid console spam✅ 4) Smarter reconnection behavior
Implemented controlled recovery logic featuring:
✅ 5) Correct handling of connection closure
Now distinguishes:
✅ 6) Safer cleanup on unmount
WebSocket teardown is now wrapped in
try/finallyto ensure:Files changed
frontend/src/atoms/debateAtoms.tswsErrorAtomfrontend/src/hooks/useDebateWS.tsWhy this matters
Before this change:
After this change:
Closes: #309
Summary by CodeRabbit
Bug Fixes
Improvements