Fix: Make Glicko-2 rating period suitable for frequent matches#312
Fix: Make Glicko-2 rating period suitable for frequent matches#312SoorejS wants to merge 2 commits into
Conversation
…e wait period and align with expected usage frequency
📝 WalkthroughWalkthroughBackend: rating service now initializes rating system with an explicit configuration. Frontend: debate WebSocket hook adds error state, throttled error handling, exponential backoff retries, reconnection logic, safer message parsing, and graceful cleanup. Changes
Sequence DiagramsequenceDiagram
participant Client as React Component
participant Hook as useDebateWS Hook
participant WS as WebSocket
participant Atom as wsErrorAtom
Client->>Hook: mount / initialize
Hook->>WS: open connection
WS-->>Hook: onopen
Hook->>Hook: reset retry counter & clear error
Hook->>WS: try send join message (spectatorHash)
WS-->>Hook: onmessage (payload)
Hook->>Hook: validate & parse message
Hook->>Atom: clear error state
Hook->>Client: update debate state
rect rgba(200,100,100,0.5)
WS-->>Hook: onerror / abnormal close
Hook->>Hook: handleError() (throttle 5000ms)
Hook->>Atom: set error state
Hook->>Hook: if retries < 3 compute backoff & schedule reconnect
Hook->>WS: close & attempt reconnect after backoff
end
WS-->>Hook: onopen (after backoff)
Hook->>Hook: reset retry state
Hook->>WS: send join message
WS-->>Hook: onmessage
Hook->>Atom: clear error state
Hook->>Client: resume updates
Client->>Hook: unmount
Hook->>WS: close gracefully
Hook->>Atom: reset error & refs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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
🤖 Fix all issues with AI agents
In `@backend/services/rating_service.go`:
- Around line 22-28: The file hardcodes fallback values inconsistent with
rating.Config; replace literal fallbacks with values from the canonical rating
config: use ratingSystem.Config.InitialRating and ratingSystem.Config.InitialRD
instead of the magic 1200.0/350.0 in sanitizePlayerStats/updateUserRating, and
set player.Volatility from ratingSystem.Config.InitialVol (or pass InitialVol as
an extra parameter if ratingSystem may be uninitialized), ensuring
sanitizePlayerMetrics already uses the same config so all defaults come from
ratingSystem.Config rather than literals.
In `@frontend/src/hooks/useDebateWS.ts`:
- Around line 388-403: The onclose handler for the ReconnectingWebSocket
(rws.onclose) should not call handleReconnect because ReconnectingWebSocket
already performs reconnection; update the rws.onclose implementation to remove
the call to handleReconnect on abnormal closures (keep the existing event.code
!== 1000 branch to call handleError with the error details for UI logging), and
restrict the handler to only update UI state (setWsStatus('disconnected'),
setWsError(...), setWs(null)) and clear wsRef when rws matches—do not trigger
manual reconnection from onclose.
- Around line 24-27: The manual reconnect logic conflicts with
ReconnectingWebSocket's built-in retries: remove the handleReconnect function
and any calls to wsRef.current.reconnect(), and stop manually invoking reconnect
attempts from useDebateWS; instead rely on the existing ReconnectingWebSocket
instance (constructed with maxRetries: Infinity and its backoff) for
reconnection, keeping only handleError (or similar error-surfacing code) to
report errors to the UI and clearing any retry counters/constants that were used
exclusively by the manual reconnect path; update references in useDebateWS
(e.g., where handleReconnect is called and where wsRef.current.reconnect() is
used) so no manual retry lifecycle remains.
- Around line 377-380: The catch block inside the message-processing logic in
useDebateWS is calling handleReconnect() on a parse/error while handling a
single incoming WebSocket message; remove the handleReconnect() invocation and
only call handleError(error, 'Error processing WebSocket message') so malformed
or empty messages are logged/surfaced but do not trigger a full reconnect; keep
reconnection behavior driven by WebSocket onerror/onclose handlers (leave
handleReconnect usage there) so only real connection failures cause reconnects.
- Around line 406-422: The cleanup function calls rws.close() twice when
ownsConnection is true (first with rws.close(1000, 'Component unmounting') if
rws.readyState === WebSocket.OPEN, then unconditionally rws.close()), which is
redundant and may trigger unexpected onclose behavior; change the logic inside
the ownsConnection branch to call rws.close(1000, 'Component unmounting') only
when readyState === WebSocket.OPEN and otherwise skip the extra close (i.e.,
remove the unconditional second rws.close()), keeping the finally block that
clears wsRef.current, setWs(null), setWsStatus('disconnected') and
setWsError(null) intact so cleanup of wsRef and state remains the same.
🧹 Nitpick comments (2)
frontend/src/hooks/useDebateWS.ts (2)
111-123: Error throttling only gates the atom update, butsetWsStatus('error')fires every time.This is fine as-is for status tracking, but note that
setWsErroris only set when the throttle allows it, whilesetWsStatus('error')runs unconditionally. If consumers rely onwsErrorbeing non-null whenever status is'error', they'll see a stale or null error message during the throttle window. Consider whether this is the intended behavior.
161-173: Throwing on emptyevent.datamay be overly strict.Some WebSocket implementations can deliver heartbeat/ping frames or empty keep-alive messages. Throwing an error and (currently) triggering a reconnect for an empty payload is aggressive. Consider silently returning instead:
Suggested fix
try { - if (!event.data) { - throw new Error('Received empty message'); - } + if (!event.data) return;
| // Constants for error handling | ||
| const ERROR_THROTTLE_MS = 5000; // 5 seconds between error messages | ||
| const MAX_RETRIES = 3; | ||
| const RETRY_BASE_DELAY = 1000; // 1 second initial delay |
There was a problem hiding this comment.
Manual reconnect logic conflicts with ReconnectingWebSocket's built-in retry.
ReconnectingWebSocket is already configured with maxRetries: Infinity and its own exponential backoff (lines 99–105). The manual handleReconnect function calls wsRef.current.reconnect() on top of that, which will trigger competing reconnection attempts — the library's automatic one and your manual one — leading to duplicated connections, race conditions, or rapid connection churn.
Either:
- Remove
handleReconnectentirely and rely on the library's built-in reconnection (it already does exponential backoff). UsehandleErroronly for surfacing errors to the UI. - Or replace
ReconnectingWebSocketwith a plainWebSocketand own the full reconnection lifecycle manually.
Option 1 is strongly recommended since you already depend on the library.
Also applies to: 99-105, 125-140
🤖 Prompt for AI Agents
In `@frontend/src/hooks/useDebateWS.ts` around lines 24 - 27, The manual reconnect
logic conflicts with ReconnectingWebSocket's built-in retries: remove the
handleReconnect function and any calls to wsRef.current.reconnect(), and stop
manually invoking reconnect attempts from useDebateWS; instead rely on the
existing ReconnectingWebSocket instance (constructed with maxRetries: Infinity
and its backoff) for reconnection, keeping only handleError (or similar
error-surfacing code) to report errors to the UI and clearing any retry
counters/constants that were used exclusively by the manual reconnect path;
update references in useDebateWS (e.g., where handleReconnect is called and
where wsRef.current.reconnect() is used) so no manual retry lifecycle remains.
| } catch (error) { | ||
| handleError(error, 'Error processing WebSocket message'); | ||
| handleReconnect(); | ||
| } |
There was a problem hiding this comment.
Reconnecting on a message parse error is too aggressive.
A single malformed or empty message does not indicate a broken connection. Calling handleReconnect() here will tear down and re-establish a perfectly healthy WebSocket just because one message couldn't be parsed. This can cause unnecessary disconnects and UI flicker for users.
Log the error and surface it via handleError, but don't reconnect.
Suggested fix
} catch (error) {
handleError(error, 'Error processing WebSocket message');
- handleReconnect();
}📝 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.
| } catch (error) { | |
| handleError(error, 'Error processing WebSocket message'); | |
| handleReconnect(); | |
| } | |
| } catch (error) { | |
| handleError(error, 'Error processing WebSocket message'); | |
| } |
🤖 Prompt for AI Agents
In `@frontend/src/hooks/useDebateWS.ts` around lines 377 - 380, The catch block
inside the message-processing logic in useDebateWS is calling handleReconnect()
on a parse/error while handling a single incoming WebSocket message; remove the
handleReconnect() invocation and only call handleError(error, 'Error processing
WebSocket message') so malformed or empty messages are logged/surfaced but do
not trigger a full reconnect; keep reconnection behavior driven by WebSocket
onerror/onclose handlers (leave handleReconnect usage there) so only real
connection failures cause reconnects.
| 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); | ||
| } | ||
|
|
||
| setWs(null); | ||
|
|
||
| if (wsRef.current === rws) { | ||
| wsRef.current = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
onclose also calls handleReconnect, compounding the double-reconnect issue.
Same concern as the onerror handler — ReconnectingWebSocket will already attempt to reconnect on abnormal closures. Calling handleReconnect here adds a competing retry. If you keep the library, limit this handler to UI state updates only.
🤖 Prompt for AI Agents
In `@frontend/src/hooks/useDebateWS.ts` around lines 388 - 403, The onclose
handler for the ReconnectingWebSocket (rws.onclose) should not call
handleReconnect because ReconnectingWebSocket already performs reconnection;
update the rws.onclose implementation to remove the call to handleReconnect on
abnormal closures (keep the existing event.code !== 1000 branch to call
handleError with the error details for UI logging), and restrict the handler to
only update UI state (setWsStatus('disconnected'), setWsError(...), setWs(null))
and clear wsRef when rws matches—do not trigger manual reconnection from
onclose.
| return () => { | ||
| if (ownsConnection) { | ||
| rws.close(); | ||
| if (wsRef.current === rws) { | ||
| wsRef.current = null; | ||
| try { | ||
| if (rws.readyState === WebSocket.OPEN) { | ||
| rws.close(1000, 'Component unmounting'); | ||
| } | ||
| rws.close(); | ||
| } catch (error) { | ||
| console.error('Error during WebSocket cleanup:', error); | ||
| } finally { | ||
| if (wsRef.current === rws) { | ||
| wsRef.current = null; | ||
| } | ||
| setWs(null); | ||
| setWsStatus('disconnected'); | ||
| setWsError(null); | ||
| } |
There was a problem hiding this comment.
Double close() call in cleanup.
When readyState === WebSocket.OPEN, rws.close(1000, 'Component unmounting') is called on line 410, then rws.close() is called unconditionally on line 412. The second call is redundant at best, and at worst may trigger the onclose handler during teardown with unexpected state.
Suggested fix
return () => {
if (ownsConnection) {
try {
- if (rws.readyState === WebSocket.OPEN) {
- rws.close(1000, 'Component unmounting');
- }
- rws.close();
+ rws.close(1000, 'Component unmounting');
} catch (error) {
console.error('Error during WebSocket cleanup:', error);
} finally {📝 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.
| return () => { | |
| if (ownsConnection) { | |
| rws.close(); | |
| if (wsRef.current === rws) { | |
| wsRef.current = null; | |
| try { | |
| if (rws.readyState === WebSocket.OPEN) { | |
| rws.close(1000, 'Component unmounting'); | |
| } | |
| rws.close(); | |
| } catch (error) { | |
| console.error('Error during WebSocket cleanup:', error); | |
| } finally { | |
| if (wsRef.current === rws) { | |
| wsRef.current = null; | |
| } | |
| setWs(null); | |
| setWsStatus('disconnected'); | |
| setWsError(null); | |
| } | |
| return () => { | |
| if (ownsConnection) { | |
| try { | |
| rws.close(1000, 'Component unmounting'); | |
| } catch (error) { | |
| console.error('Error during WebSocket cleanup:', error); | |
| } finally { | |
| if (wsRef.current === rws) { | |
| wsRef.current = null; | |
| } | |
| setWs(null); | |
| setWsStatus('disconnected'); | |
| setWsError(null); | |
| } |
🤖 Prompt for AI Agents
In `@frontend/src/hooks/useDebateWS.ts` around lines 406 - 422, The cleanup
function calls rws.close() twice when ownsConnection is true (first with
rws.close(1000, 'Component unmounting') if rws.readyState === WebSocket.OPEN,
then unconditionally rws.close()), which is redundant and may trigger unexpected
onclose behavior; change the logic inside the ownsConnection branch to call
rws.close(1000, 'Component unmounting') only when readyState === WebSocket.OPEN
and otherwise skip the extra close (i.e., remove the unconditional second
rws.close()), keeping the finally block that clears wsRef.current, setWs(null),
setWsStatus('disconnected') and setWsError(null) intact so cleanup of wsRef and
state remains the same.
ad0c690 to
d0a39c8
Compare
|
this PR unintentionally includes a small change in useDebateWS.ts. |
1 participant
fixes issue #305
Description
The Glicko-2 implementation was using a 24-hour (86400 sec) rating period, which caused Rating Deviation (RD) to decay too slowly when users played many matches in a short time (e.g., 10 matches in an hour). This made ratings overly “sticky” and unresponsive to real performance changes.
This PR updates the system to use a 1-hour (3600 sec) rating period, making RD updates more dynamic while keeping rating stability intact.
✅ Changes Made
Updated InitRatingService to initialize Glicko-2 with an explicit configuration instead of nil, ensuring proper control over rating behavior:
🎯 Expected Impact
After this change:
RD will adjust more realistically for active players
Ratings will reflect recent performance faster
Players won’t remain overrated/underrated after rapid match streaks
System still avoids extreme rating swings
Summary by CodeRabbit