Fix matchmaking double join bug #3065
Conversation
WalkthroughThe changes enhance game session management and connection reliability. They introduce memoized user data caching, prevent mid-game exits and rejoin attempts, add delayed WebSocket joins with periodic game state verification, wire modal close callbacks, and insert a tutorial button UI element. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MatchmakingModal
participant WS as WebSocket
participant Server as Server
Client->>WS: connect()
WS->>WS: opening...
WS-->>Client: onOpen() event
Client->>Client: set connectTimeout (2 sec)
Client->>Client: clear previous gameCheckInterval
Note over Client: Wait 2 seconds...
Client->>Server: dispatch join payload
Server-->>WS: matchmaking message received
WS-->>Client: onmessage
Client->>Client: start gameCheckInterval (1 sec)
loop Every 1 second
Client->>Server: checkGame()
end
Client->>Client: onClose() event
Client->>Client: cleanup connectTimeout<br/>clear gameCheckInterval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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 (1)
src/client/ClientGameRunner.ts (1)
77-102:hasStartedflips too early and blocks forced leave.It is set for any message (including prestart/error), and the early return ignores
force. This can block cleanup and leave users stuck. Only set it on real start (or first turn), and keepforceas an override.🛠️ Suggested fix
- const onmessage = (message: ServerMessage) => { - hasStarted = true; + const onmessage = (message: ServerMessage) => { + if (message.type === "start") { + hasStarted = true; + } if (message.type === "prestart") { console.log( `lobby: game prestarting: ${JSON.stringify(message, replacer)}`, );- return (force: boolean = false) => { - if (hasStarted) { - return false; - } - - if ( - !force && - (currentGameRunner?.shouldPreventWindowClose() || hasStarted) - ) { + return (force: boolean = false) => { + if ( + !force && + (currentGameRunner?.shouldPreventWindowClose() || hasStarted) + ) { console.log("Player is active, prevent leaving game"); return false; }Also applies to: 179-187
🤖 Fix all issues with AI agents
In `@src/client/Api.ts`:
- Around line 51-85: The cached promise __userMe in getUserMe can permanently
hold a failure result, so modify getUserMe to clear the cache before returning
false on any failure path: set __userMe = null immediately before each early
return that returns false (including after userAuth() false, after
response.status === 401 and before calling logOut(), after non-200 responses,
after schema validation failure, and inside the catch block). Also ensure
logOut() clears __userMe (or have getUserMe clear it just before calling
logOut()) so future calls can retry authentication.
In `@src/client/components/BaseModal.ts`:
- Around line 28-31: The modal's onClose handler currently calls this.close()
which can cause close() and its onClose hook to run twice; update the wiring in
BaseModal.firstUpdated (or a small helper method you extract) so modalEl.onClose
first checks this.isModalOpen and only calls this.close() when true, and ensure
close() sets this.isModalOpen = false early to prevent re-entry; reference the
BaseModal class, firstUpdated method, modalEl.onClose assignment, close()
method, and isModalOpen flag when making this change.
In `@src/client/components/PlayPage.ts`:
- Around line 18-21: The new <tutorial-button> element is not being hidden when
the game starts; update the hide-list used in the handleJoinLobby callback
(function handleJoinLobby in src/client/Main.ts) to include "tutorial-button"
alongside the existing entries like "account-button" and "stats-button" so it is
toggled with the other modal/buttons during game start.
In `@src/client/Matchmaking.ts`:
- Line 142: The setInterval call is being created on every message, overwriting
this.gameCheckInterval and leaving old intervals running; modify the logic where
this.gameCheckInterval is assigned so you only start the interval once (e.g., if
(!this.gameCheckInterval) { this.gameCheckInterval = setInterval(() =>
this.checkGame(), 1000); }) or clear the previous interval before creating a new
one (clearInterval(this.gameCheckInterval) then reassign), referencing
this.gameCheckInterval and the checkGame() method to locate and fix the
problematic setInterval usage.
- Around line 119-130: The timeout handler set in Matchmaking
(this.connectTimeout) can call this.socket?.send after the socket has closed,
causing a throw; update the handler to first verify the socket is open (check
this.socket?.readyState === WebSocket.OPEN or numeric 1) before calling
this.socket?.send and only set this.connected = true after a successful send,
and also ensure socket.onerror and socket.onclose clear this.connectTimeout to
avoid the race between closure and the 2s timeout; reference the
this.connectTimeout timeout callback, this.socket?.send, getPlayToken(),
socket.onerror and socket.onclose when making the changes.
| protected firstUpdated(): void { | ||
| if (this.modalEl) { | ||
| this.modalEl.onClose = () => this.close(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -type f -name "*.ts" -o -name "*.js" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 122
🏁 Script executed:
rg -n "customElement\(\"o-modal\"\)|defineCustomElement.*o-modal" src/client -g "*.ts" -A 5Repository: openfrontio/OpenFrontIO
Length of output: 510
🏁 Script executed:
rg -n "onClose" src/client -g "*.ts" -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 5234
🏁 Script executed:
cat -n src/client/components/BaseModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 4001
🏁 Script executed:
rg -n "close\(\)" src/client/components/BaseModal.ts -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 768
🏁 Script executed:
rg "protected onClose\(\)" src/client -A 5 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 3264
Add a guard to prevent close() from running twice when modal closes.
When o-modal.close() fires, it calls onClose, which is wired to close(). This causes close() and its onClose() hook to run a second time. Some cleanup code (like socket.close() or event dispatch) should not run twice. Use isModalOpen to guard against this:
Suggested guard
protected firstUpdated(): void {
if (this.modalEl) {
- this.modalEl.onClose = () => this.close();
+ this.modalEl.onClose = () => {
+ if (this.isModalOpen) {
+ this.close();
+ }
+ };
}
}Optional: Consider moving this setup into a helper method to keep modal wiring composable rather than spreading it in the base class.
📝 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.
| protected firstUpdated(): void { | |
| if (this.modalEl) { | |
| this.modalEl.onClose = () => this.close(); | |
| } | |
| protected firstUpdated(): void { | |
| if (this.modalEl) { | |
| this.modalEl.onClose = () => { | |
| if (this.isModalOpen) { | |
| this.close(); | |
| } | |
| }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/client/components/BaseModal.ts` around lines 28 - 31, The modal's onClose
handler currently calls this.close() which can cause close() and its onClose
hook to run twice; update the wiring in BaseModal.firstUpdated (or a small
helper method you extract) so modalEl.onClose first checks this.isModalOpen and
only calls this.close() when true, and ensure close() sets this.isModalOpen =
false early to prevent re-entry; reference the BaseModal class, firstUpdated
method, modalEl.onClose assignment, close() method, and isModalOpen flag when
making this change.
| <!-- Tutorial Button --> | ||
| <div class="flex justify-center -mb-1"> | ||
| <tutorial-button></tutorial-button> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if tutorial-button is referenced in Main.ts
echo "=== Looking for tutorial-button references in Main.ts ==="
rg -n "tutorial-button" src/client/Main.ts
echo ""
echo "=== Looking for classList.add(\"hidden\") calls in Main.ts ==="
rg -n "classList\.add\(\"hidden\"\)" src/client/Main.ts -C2
echo ""
echo "=== Checking the full Main.ts file size ==="
wc -l src/client/Main.tsRepository: openfrontio/OpenFrontIO
Length of output: 1571
🏁 Script executed:
#!/bin/bash
echo "=== Context around settings-button hiding (line 773) ==="
sed -n '760,810p' src/client/Main.ts
echo ""
echo "=== Search for handleJoinLobby function ==="
rg -n "handleJoinLobby|Closing modals" src/client/Main.ts -B5 -A10Repository: openfrontio/OpenFrontIO
Length of output: 3972
Add tutorial button to the list of elements hidden during game start.
In src/client/Main.ts at line 771, the handleJoinLobby callback hides UI elements when the game starts. The new tutorial button is not included in this list. Add "tutorial-button" to the modal/button list at line 781 so it hides with other buttons like account-button and stats-button.
🤖 Prompt for AI Agents
In `@src/client/components/PlayPage.ts` around lines 18 - 21, The new
<tutorial-button> element is not being hidden when the game starts; update the
hide-list used in the handleJoinLobby callback (function handleJoinLobby in
src/client/Main.ts) to include "tutorial-button" alongside the existing entries
like "account-button" and "stats-button" so it is toggled with the other
modal/buttons during game start.
| this.connectTimeout = setTimeout(async () => { | ||
| console.log("join timeout fired"); | ||
| // Set a delay so the user can see the "connecting" message, | ||
| // otherwise the "searching" message will be shown immediately. | ||
| // Also wait so people don't back out immediately | ||
| this.socket?.send( | ||
| JSON.stringify({ | ||
| type: "join", | ||
| jwt: await getPlayToken(), | ||
| }), | ||
| ); | ||
| this.connected = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its line count
wc -l src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 96
🏁 Script executed:
# Read the relevant section and surrounding context
sed -n '100,150p' src/client/Matchmaking.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 2147
🏁 Script executed:
# Also search for socket lifecycle management (close, error handlers)
rg -A 5 -B 5 "(this\.socket.*close|this\.socket.*error|disconnected)" src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 1000
🏁 Script executed:
# Check for any existing timeout management or clearTimeout calls
rg "clearTimeout|this\.connectTimeout" src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 225
🏁 Script executed:
# Verify WebSocket.OPEN constant and readyState behavior
rg -i "websocket\." src/client/Matchmaking.ts | head -20Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if there are any other timeout guards or socket state checks elsewhere in the file
rg "readyState|OPEN|CLOSING|CLOSED" src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Let's verify the exact line numbers by looking at socket initialization context more carefully
cat -n src/client/Matchmaking.ts | sed -n '15,35p'Repository: openfrontio/OpenFrontIO
Length of output: 843
🏁 Script executed:
# Find the exact line number of the setTimeout with join timeout
rg -n "join timeout fired" src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 113
🏁 Script executed:
# Show the context around that line
cat -n src/client/Matchmaking.ts | sed -n '75,140p'Repository: openfrontio/OpenFrontIO
Length of output: 2784
🏁 Script executed:
# Verify the socket handlers don't clear the timeout
cat -n src/client/Matchmaking.ts | sed -n '140,160p'Repository: openfrontio/OpenFrontIO
Length of output: 820
🏁 Script executed:
# Look for the onClose method
rg -n "onClose\|protected.*onClose" src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Search for onClose more broadly
rg -n "onClose" src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 100
🏁 Script executed:
# Also check for component lifecycle methods
rg -n "protected\s+(async\s+)?on" src/client/Matchmaking.tsRepository: openfrontio/OpenFrontIO
Length of output: 148
🏁 Script executed:
# Check the onClose method implementation
cat -n src/client/Matchmaking.ts | sed -n '185,200p'Repository: openfrontio/OpenFrontIO
Length of output: 631
Add readyState check before sending join message.
If the WebSocket closes or errors before the 2-second timeout fires, the send() call will throw. The socket.onerror and socket.onclose handlers do not clear the timeout, creating a race condition. Add a guard to check the socket is open:
Suggested fix
this.connectTimeout = setTimeout(async () => {
+ if (this.socket?.readyState !== WebSocket.OPEN) {
+ return;
+ }
console.log("join timeout fired");
// Set a delay so the user can see the "connecting" message,
// otherwise the "searching" message will be shown immediately.
// Also wait so people don't back out immediately
this.socket?.send(
JSON.stringify({
type: "join",
jwt: await getPlayToken(),
}),
);
this.connected = true;
this.requestUpdate();
}, 2000);🤖 Prompt for AI Agents
In `@src/client/Matchmaking.ts` around lines 119 - 130, The timeout handler set in
Matchmaking (this.connectTimeout) can call this.socket?.send after the socket
has closed, causing a throw; update the handler to first verify the socket is
open (check this.socket?.readyState === WebSocket.OPEN or numeric 1) before
calling this.socket?.send and only set this.connected = true after a successful
send, and also ensure socket.onerror and socket.onclose clear
this.connectTimeout to avoid the race between closure and the 2s timeout;
reference the this.connectTimeout timeout callback, this.socket?.send,
getPlayToken(), socket.onerror and socket.onclose when making the changes.
| console.log(`matchmaking: got game ID: ${data.gameId}`); | ||
| this.gameID = data.gameId; | ||
| } | ||
| this.gameCheckInterval = setInterval(() => this.checkGame(), 1000); |
There was a problem hiding this comment.
Avoid creating a new interval on every message.
setInterval runs on each message, overwrites the handle, and leaves old intervals running. Start it once (or clear before resetting) to avoid multiple checks per second.
🛠️ Suggested fix
- this.gameCheckInterval = setInterval(() => this.checkGame(), 1000);
+ if (!this.gameCheckInterval) {
+ this.gameCheckInterval = setInterval(() => this.checkGame(), 1000);
+ }📝 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.
| this.gameCheckInterval = setInterval(() => this.checkGame(), 1000); | |
| if (!this.gameCheckInterval) { | |
| this.gameCheckInterval = setInterval(() => this.checkGame(), 1000); | |
| } |
🤖 Prompt for AI Agents
In `@src/client/Matchmaking.ts` at line 142, The setInterval call is being created
on every message, overwriting this.gameCheckInterval and leaving old intervals
running; modify the logic where this.gameCheckInterval is assigned so you only
start the interval once (e.g., if (!this.gameCheckInterval) {
this.gameCheckInterval = setInterval(() => this.checkGame(), 1000); }) or clear
the previous interval before creating a new one
(clearInterval(this.gameCheckInterval) then reassign), referencing
this.gameCheckInterval and the checkGame() method to locate and fix the
problematic setInterval usage.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Description:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME