Skip to content

Conversation

@tudddorrr
Copy link
Contributor

No description provided.

@tudddorrr tudddorrr added the fix This pull request contains a fix label Jan 3, 2026
@claude
Copy link

claude bot commented Jan 3, 2026

PR Review: Fix sockets

🟡 Code Quality & Best Practices

Potential race condition in HandleIdentifySuccess (PlayersAPI.cs:53)

The new condition:

if (\!Talo.IsOffline() && Talo.Socket.IsIdentified())
{
    await Talo.Socket.ResetConnection();
}

Only resets the socket if it's already identified. However, there's a scenario where this could cause issues:

  1. User identifies with Player A (socket identifies)
  2. Network drops briefly, socket disconnects but identified flag remains true
  3. User identifies with Player B while offline
  4. Network restores
  5. Socket reconnects and auto-identifies with the old Player A credentials (via tempSocketToken from step 1)

Consider checking socket connection state in addition to identification:

if (\!Talo.IsOffline() && (Talo.Socket.IsIdentified() || socket.State == WebSocketState.Connected))

Or reset identified flag in CloseConnection() to match the reset behavior in ResetConnection().


🔵 Other Observations

WebSocket heartbeat configuration (TaloSocket.cs:22-26)

The hardcoded ping configuration looks reasonable, but verify that the WebSocket library's WebSocketMessage constructor accepts a plain string for the heartbeat format. If the server expects JSON, you may need:

PingMessage = new WebSocketMessage("{\"type\":\"v1.heartbeat\"}")

✅ Security & Performance

No issues found.


✅ Backwards Compatibility

No issues found - the new IsIdentified() method is additive only.

@tudddorrr tudddorrr merged commit 633020c into develop Jan 3, 2026
2 checks passed
@tudddorrr tudddorrr deleted the fix-sockets branch January 3, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix This pull request contains a fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants