fix(multiplayer): route host seat mutations to the active lobby backend - #1745
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors multiplayer lobby actions to prevent race conditions during game start. It introduces asynchronous seat mutations (seatMutateAsync) and sequential lobby initialization (startLobbyWithCurrentPlayers), alongside cleaner WebSocket and P2P connection teardown helpers. Feedback focuses on improving state-transition robustness by handling connection loss correctly in seatMutateAsync to avoid misleading P2P errors, and consistently selecting store actions via the React hook in HostControlTile instead of bypassing reactivity with getState().
| seatMutateAsync: async (mutation) => { | ||
| if (isServerHostingActive(get)) { | ||
| hostWs!.send(JSON.stringify({ | ||
| type: "SeatMutate", | ||
| data: { mutation }, | ||
| })); | ||
| return; | ||
| } | ||
| await runP2PSeatMutation(mutation, set); | ||
| }, |
There was a problem hiding this comment.
[HIGH] Misleading error message on server-hosted lobby connection loss.
Evidence:client/src/stores/multiplayerStore.ts:1049-1058.
Why it matters: When a server-hosted lobby loses connection,isServerHostingActivereturns false, causing the mutation to fall through to P2P handling and throw a confusing "P2P host is not active" error instead of "Host connection is not active".
Suggested fix: Check foractiveP2PHostAdapterfirst, and if absent, verify thathostWsis open before sending the mutation.
seatMutateAsync: async (mutation) => {
if (activeP2PHostAdapter) {
await runP2PSeatMutation(mutation, set);
return;
}
if (!isServerHostingActive(get)) {
throw new Error("Host connection is not active.");
}
hostWs!.send(JSON.stringify({
type: "SeatMutate",
data: { mutation },
}));
},References
- L4. Edge cases: Ensure robust handling of connection loss and other edge cases in multiplayer state transitions. (link)
| const startLobbyWithCurrentPlayers = useMultiplayerStore( | ||
| (s) => s.startLobbyWithCurrentPlayers, | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Inconsistent state access and action dispatching.
Evidence:client/src/components/chrome/HostControlTile.tsx:257-259.
Why it matters: Bypassing the selected store actions and usinguseMultiplayerStore.getState()directly insidefillWithAiAndStartis inconsistent with the rest of the component, which selects actions likestartLobbyWithCurrentPlayersandseatMutatevia the hook.
Suggested fix: SelectseatMutateAsyncandshowToastat the top of the component using theuseMultiplayerStorehook, and use them directly.
| const startLobbyWithCurrentPlayers = useMultiplayerStore( | |
| (s) => s.startLobbyWithCurrentPlayers, | |
| ); | |
| const startLobbyWithCurrentPlayers = useMultiplayerStore( | |
| (s) => s.startLobbyWithCurrentPlayers, | |
| ); | |
| const seatMutateAsync = useMultiplayerStore((s) => s.seatMutateAsync); | |
| const showToast = useMultiplayerStore((s) => s.showToast); |
References
- L5. Idiomatic code: Adhere to consistent and idiomatic React/Zustand state management patterns across the codebase. (link)
| const fillWithAiAndStart = () => { | ||
| if (!haveAnyDeck) return; | ||
| for (const slot of waitingSeats) { | ||
| const deck = pickRandomAiDeck(); | ||
| if (!deck) return; | ||
| seatMutate({ | ||
| type: "SetKind", | ||
| data: { | ||
| seatIndex: slot.playerId, | ||
| kind: { type: "Ai", data: { difficulty: "Medium", deck } }, | ||
| }, | ||
| }); | ||
| } | ||
| seatMutate({ type: "Start" }); | ||
| void (async () => { | ||
| for (const slot of waitingSeats) { | ||
| const deck = pickRandomAiDeck(); | ||
| if (!deck) return; | ||
| await useMultiplayerStore.getState().seatMutateAsync({ | ||
| type: "SetKind", | ||
| data: { | ||
| seatIndex: slot.playerId, | ||
| kind: { type: "Ai", data: { difficulty: "Medium", deck } }, | ||
| }, | ||
| }); | ||
| } | ||
| await useMultiplayerStore.getState().seatMutateAsync({ type: "Start" }); | ||
| })().catch((err) => { | ||
| useMultiplayerStore.getState().showToast( | ||
| err instanceof Error ? err.message : String(err), | ||
| ); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
[MEDIUM] Inconsistent state access and action dispatching.
Evidence:client/src/components/chrome/HostControlTile.tsx:327-347.
Why it matters: Bypassing the selected store actions and usinguseMultiplayerStore.getState()directly insidefillWithAiAndStartis inconsistent with the rest of the component, which selects actions likestartLobbyWithCurrentPlayersandseatMutatevia the hook.
Suggested fix: SelectseatMutateAsyncandshowToastat the top of the component using theuseMultiplayerStorehook, and use them directly.
const fillWithAiAndStart = () => {
if (!haveAnyDeck) return;
void (async () => {
for (const slot of waitingSeats) {
const deck = pickRandomAiDeck();
if (!deck) return;
await seatMutateAsync({
type: "SetKind",
data: {
seatIndex: slot.playerId,
kind: { type: "Ai", data: { difficulty: "Medium", deck } },
},
});
}
await seatMutateAsync({ type: "Start" });
})().catch((err) => {
showToast(err instanceof Error ? err.message : String(err));
});
};
References
- L5. Idiomatic code: Adhere to consistent and idiomatic React/Zustand state management patterns across the codebase. (link)
| onClick={() => { | ||
| void startLobbyWithCurrentPlayers().catch((err) => { | ||
| useMultiplayerStore.getState().showToast( | ||
| err instanceof Error ? err.message : String(err), | ||
| ); | ||
| }); | ||
| }} |
There was a problem hiding this comment.
[MEDIUM] Inconsistent state access and action dispatching.
Evidence:client/src/components/chrome/HostControlTile.tsx:438-444.
Why it matters: Bypassing the selected store actions and usinguseMultiplayerStore.getState()directly insidefillWithAiAndStartis inconsistent with the rest of the component, which selects actions likestartLobbyWithCurrentPlayersandseatMutatevia the hook.
Suggested fix: SelectseatMutateAsyncandshowToastat the top of the component using theuseMultiplayerStorehook, and use them directly.
| onClick={() => { | |
| void startLobbyWithCurrentPlayers().catch((err) => { | |
| useMultiplayerStore.getState().showToast( | |
| err instanceof Error ? err.message : String(err), | |
| ); | |
| }); | |
| }} | |
| onClick={() => { | |
| void startLobbyWithCurrentPlayers().catch((err) => { | |
| showToast(err instanceof Error ? err.message : String(err)); | |
| }); | |
| }} |
References
- L5. Idiomatic code: Adhere to consistent and idiomatic React/Zustand state management patterns across the codebase. (link)
|
Pushed maintainer review fixes on top of current What changed:
Local verification:
|
matthewevans
left a comment
There was a problem hiding this comment.
Implementation review is clean. The PR now keeps server-host seat mutations routed through the active server socket, reports disconnected server-host state without falling through to P2P, preserves ordered P2P start mutations, and has focused regression coverage plus green CI on head 2097a6e.
Summary
seatMutateto the server WebSocket when a waiting server lobby is active, instead of always preferring a stale P2P adapter left over from an earlier session.hostingStatus === "waiting"(failed Start shows a toast instead of callingcancelHosting()).seatMutateAsyncandstartLobbyWithCurrentPlayersso P2P "Start now" and "Fill with AI & start" run seat mutations in order.seat_state.is_full(), matching manualStartand avoiding spurious starts whenstart_when_fullis enabled.Fixes #1506
Test plan
pnpm exec vitest run src/stores/__tests__/multiplayerStore.test.tspasses.