From 0d758a1cdd9a2421a2cdc4cf367cf5bb0d829e2c Mon Sep 17 00:00:00 2001 From: cuinhellcat <217210902+cuinhellcat@users.noreply.github.com> Date: Tue, 18 Aug 2026 03:19:22 +0200 Subject: [PATCH] fix(client): resolve the local seat from the mode census (#7524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `usePlayerId()`/`getPlayerId()` are the single authority for "which seat is this client's own" — 50 production call sites across 36 files read one of them. Both resolved it from a hand-typed list of game modes that predates `"draft-match"`, so a pod-draft guest was answered seat 0 no matter what the pod had assigned it. Its own hand came back redacted, `GamePage`'s mulligan `pending.find` searched the host's seat, and once the host kept, both clients sat on "Opponent is deciding their opening hand…" with nobody able to answer for seat 1. `activePlayerId` was already correct (`setupDraftMatchAvatars`); only the resolver discarded it. Give `GAME_MODE_TRAITS` a third axis — `seat: "seat-zero" | "wire-assigned" | "no-seat"` — and read it through the new `seatSource()`. The census is a `Record`, so a mode added later cannot compile without declaring where its seat comes from; that is what the two lists could not do. Both entry points now share one `resolveLocalSeat`, with the spectator split passed in as a parameter so it stays a stated decision rather than a drift. `native-ai` is declared `"seat-zero"`, measured rather than assumed: `handleNativeEvent` (`GameProvider.tsx`) never calls `setActivePlayerId`, so reading the wire there would read a stale value from an earlier online game. Counter-probe: flipping only the `draft-match` census entry back to `"seat-zero"` drops both new rows — `expected +0 to be 1`. Does not cover: the pod-draft guest's seat still reaches the store via `setupDraftMatchAvatars`, which derives it from `matchPairing.type` rather than from the `assignedPlayerId` the host actually sent in `game_setup`. The two agree today; unifying them on the wire value is a separate change. Not evidenced by the new tests: they do not render `GamePage`, so they do not show the mulligan modal appearing for the guest. They pin the seat authority that modal reads. The solo-mode and spectator rows are green with and without the fix and are labelled in the file as nail-downs, not evidence. Co-Authored-By: Claude Opus 5 --- .../__tests__/useConcedeHandler.test.tsx | 8 +- .../__tests__/usePlayerId.draftMatch.test.tsx | 99 +++++++++++++++++++ client/src/hooks/usePlayerId.ts | 57 ++++++++--- client/src/stores/gameStore.ts | 51 ++++++++-- 4 files changed, 190 insertions(+), 25 deletions(-) create mode 100644 client/src/hooks/__tests__/usePlayerId.draftMatch.test.tsx diff --git a/client/src/hooks/__tests__/useConcedeHandler.test.tsx b/client/src/hooks/__tests__/useConcedeHandler.test.tsx index db9fa9fc72..5092b84277 100644 --- a/client/src/hooks/__tests__/useConcedeHandler.test.tsx +++ b/client/src/hooks/__tests__/useConcedeHandler.test.tsx @@ -27,7 +27,13 @@ vi.mock("../../game/sessionCleanup", () => ({ clearPromptOverlayState: () => clearPromptOverlayStateMock(), })); -vi.mock("../../stores/gameStore", () => ({ +// Only the store handle and `clearGame` are stubbed. The module's pure +// helpers — `seatSource` and the `GAME_MODE_TRAITS` census behind it, which +// `getPlayerId()` consults for the conceding seat — come through for real, so +// this test concedes as the seat the census actually resolves rather than as a +// seat the mock asserts. +vi.mock("../../stores/gameStore", async () => ({ + ...(await vi.importActual("../../stores/gameStore")), useGameStore: { getState: () => ({ dispatch: dispatchMock, diff --git a/client/src/hooks/__tests__/usePlayerId.draftMatch.test.tsx b/client/src/hooks/__tests__/usePlayerId.draftMatch.test.tsx new file mode 100644 index 0000000000..af682e8987 --- /dev/null +++ b/client/src/hooks/__tests__/usePlayerId.draftMatch.test.tsx @@ -0,0 +1,99 @@ +/** + * Regression test for the pod-draft guest's collapsed seat. + * + * `usePlayerId`/`getPlayerId` are the single authority for "which seat is this + * client's own" — 54 production call sites read one of them, from the mulligan + * modal's `pending.find` in `GamePage` to `gameLoopController`'s priority gate + * and `useConcedeHandler`'s `player_id`. Both resolved the answer from a + * hand-typed list of game modes that predates `"draft-match"`, so a pod-draft + * guest — correctly told `activePlayerId: 1` by `setupDraftMatchAvatars` + * (`GameProvider.tsx`) — was answered seat 0, the host's seat. + * + * The class guard below is the one that would have caught it: for EVERY mode + * that seats other humans, a client told "you are seat 1" must not answer 0. + * It reads the mode census (`GAME_MODE_TRAITS`) rather than a second hand-typed + * list, so a mode added later is covered on the day it is added. + * + * What these rows do NOT prove: they do not render `GamePage`, so they do not + * show the mulligan modal appearing for the guest. They pin the seat authority + * that modal's `pending.find(e => e.player === playerId)` reads. + */ +import { renderHook } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { PLAYER_ID, SPECTATOR_PLAYER_ID } from "../../constants/game"; +import type { GameMode } from "../../stores/gameStore"; +import { GAME_MODE_TRAITS, hasRemoteHumans, useGameStore } from "../../stores/gameStore"; +import { useMultiplayerStore } from "../../stores/multiplayerStore"; +import { getPlayerId, usePlayerId } from "../usePlayerId"; + +/** Derived from the census itself, never hand-typed: a mode added to + * `GAME_MODE_TRAITS` joins this list without anyone remembering to. */ +const ALL_MODES = Object.keys(GAME_MODE_TRAITS) as GameMode[]; + +function seatMe(gameMode: GameMode, activePlayerId: number | null) { + useGameStore.setState({ gameMode }); + useMultiplayerStore.setState({ activePlayerId, isSpectator: false }); +} + +describe("local seat resolution", () => { + beforeEach(() => { + useGameStore.getState().reset(); + useMultiplayerStore.setState({ activePlayerId: null, isSpectator: false }); + }); + + afterEach(() => { + useGameStore.getState().reset(); + useMultiplayerStore.setState({ activePlayerId: null, isSpectator: false }); + }); + + it.each(ALL_MODES.filter((mode) => hasRemoteHumans(mode)))( + "%s: a client told it holds seat 1 is never answered seat 0", + (mode) => { + seatMe(mode, 1); + // Spectators answer `SPECTATOR_PLAYER_ID` here rather than 1; that is + // still not seat 0, so this assertion needs no by-name exception and + // stays honest for every mode the census grows. + expect(getPlayerId()).not.toBe(PLAYER_ID); + }, + ); + + it("seats a pod-draft guest at the seat the pod gave them", () => { + seatMe("draft-match", 1); + const { result } = renderHook(() => usePlayerId()); + expect(result.current).toBe(1); + expect(getPlayerId()).toBe(1); + }); + + it("still seats the pod-draft host at 0", () => { + seatMe("draft-match", 0); + const { result } = renderHook(() => usePlayerId()); + expect(result.current).toBe(PLAYER_ID); + expect(getPlayerId()).toBe(PLAYER_ID); + }); + + // Counter-direction (the expensive collateral is over-correction): a solo + // game must keep answering 0 even when a previous online game left a stale + // `activePlayerId` behind. These rows are green with and without the fix — + // they nail the behaviour down, they do not evidence it. + it.each(ALL_MODES.filter((mode) => !hasRemoteHumans(mode)))( + "%s: a solo game ignores a stale wire seat", + (mode) => { + seatMe(mode, 1); + const { result } = renderHook(() => usePlayerId()); + expect(result.current).toBe(PLAYER_ID); + expect(getPlayerId()).toBe(PLAYER_ID); + }, + ); + + // Also green either way: the spectator split is a documented contract, not a + // side effect. `HudBadges.tsx` states outright that `usePlayerId()` returns + // `PLAYER_ID` in spectate mode and gates its second-person copy on + // `useSpectatorMode()` because of it. + it("keeps the spectator split: 255 from the getter, 0 from the hook", () => { + seatMe("spectate", SPECTATOR_PLAYER_ID); + const { result } = renderHook(() => usePlayerId()); + expect(getPlayerId()).toBe(SPECTATOR_PLAYER_ID); + expect(result.current).toBe(PLAYER_ID); + }); +}); diff --git a/client/src/hooks/usePlayerId.ts b/client/src/hooks/usePlayerId.ts index bc3ca5ae98..9de7ed2ca0 100644 --- a/client/src/hooks/usePlayerId.ts +++ b/client/src/hooks/usePlayerId.ts @@ -1,30 +1,57 @@ import type { PlayerId, WaitingFor } from "../adapter/types"; import { PLAYER_ID, SPECTATOR_PLAYER_ID } from "../constants/game"; -import { useGameStore } from "../stores/gameStore"; +import type { GameMode } from "../stores/gameStore"; +import { seatSource, useGameStore } from "../stores/gameStore"; import { useMultiplayerStore } from "../stores/multiplayerStore"; -function currentLocalPlayerId(): PlayerId { - const gameMode = useGameStore.getState().gameMode; - if (gameMode === "spectate") { - return SPECTATOR_PLAYER_ID; - } - if (gameMode && (gameMode === "online" || gameMode === "p2p-host" || gameMode === "p2p-join")) { - return useMultiplayerStore.getState().activePlayerId ?? PLAYER_ID; +/** + * The one seat resolver both entry points below share, so they cannot drift the + * way the two hand-typed mode lists they replace did. Which modes take their + * seat off a wire is the census's question, not this file's: see `seatSource` + * in `stores/gameStore.ts`. + * + * `spectatorSeat` is the one place the two entry points legitimately differ, + * and it is a parameter so that the difference is stated rather than repeated. + * `getPlayerId()` answers `SPECTATOR_PLAYER_ID`, which `dispatch.ts` reads to + * refuse submitting actions. `usePlayerId()` answers `PLAYER_ID`, because + * display surfaces render a spectated game from seat 0's side — `HudBadges.tsx` + * states that contract outright and gates its second-person copy on + * `useSpectatorMode()` because of it. Widening the hook to 255 would empty + * every `players[playerId]` lookup on the spectator board. + */ +function resolveLocalSeat( + gameMode: GameMode | null, + activePlayerId: PlayerId | null, + spectatorSeat: PlayerId, +): PlayerId { + switch (seatSource(gameMode)) { + case "wire-assigned": + // Seat not delivered yet (setup still in flight): seat 0 is what every + // caller read before the assignment arrived, so this changes nothing. + return activePlayerId ?? PLAYER_ID; + case "no-seat": + return spectatorSeat; + case "seat-zero": + return PLAYER_ID; } +} - return PLAYER_ID; +function currentLocalPlayerId(): PlayerId { + return resolveLocalSeat( + useGameStore.getState().gameMode, + useMultiplayerStore.getState().activePlayerId, + SPECTATOR_PLAYER_ID, + ); } -/** React hook: returns the current player's game-assigned ID (0 or 1). Falls back to PLAYER_ID (0) for AI/local mode. */ +/** React hook: the seat this client occupies. Solo modes are seat 0 by + * construction; every mode that takes its seat off a wire reads the assignment + * the server, P2P host, or draft pod delivered. */ export function usePlayerId(): PlayerId { const gameMode = useGameStore((s) => s.gameMode); const activePlayerId = useMultiplayerStore((s) => s.activePlayerId); - if (gameMode && (gameMode === "online" || gameMode === "p2p-host" || gameMode === "p2p-join")) { - return activePlayerId ?? PLAYER_ID; - } - - return PLAYER_ID; + return resolveLocalSeat(gameMode, activePlayerId, PLAYER_ID); } /** Non-React getter for use in plain functions (autoPass, gameLoopController). */ diff --git a/client/src/stores/gameStore.ts b/client/src/stores/gameStore.ts index 90efc04ba1..477860f893 100644 --- a/client/src/stores/gameStore.ts +++ b/client/src/stores/gameStore.ts @@ -77,9 +77,26 @@ export type EngineAuthority = "client" | "wire"; * nothing they do can desync a peer or leak hidden info across a wire. */ export type TableCompany = "solo" | "remote-humans"; +/** Where this client's OWN seat number comes from. + * + * `"seat-zero"` — a solo game. There is one local human and the engine seats + * them at 0 by construction; nothing on a wire can say otherwise, so a stale + * `activePlayerId` left behind by an earlier online game must not be read. + * + * `"wire-assigned"` — somebody else hands this client its seat: a server + * (`playerIdentity` from `WebSocketAdapter`), a P2P host (`game_setup`'s + * `assignedPlayerId`), or the pod that paired this match + * (`setupDraftMatchAvatars`). `multiplayerStore.activePlayerId` carries it. + * + * `"no-seat"` — a spectator holds no seat at all. The two seat resolvers in + * `usePlayerId.ts` deliberately answer this case differently; see the comment + * there for the contract `HudBadges.tsx` depends on. */ +export type SeatSource = "seat-zero" | "wire-assigned" | "no-seat"; + interface GameModeTraits { readonly authority: EngineAuthority; readonly company: TableCompany; + readonly seat: SeatSource; } /** @@ -93,15 +110,15 @@ interface GameModeTraits { * * `spectate` is `remote-humans` by the *game* it observes, not by the observer. */ -const GAME_MODE_TRAITS: Record = { - "ai": { authority: "client", company: "solo" }, - "local": { authority: "client", company: "solo" }, - "native-ai": { authority: "wire", company: "solo" }, - "online": { authority: "wire", company: "remote-humans" }, - "p2p-host": { authority: "wire", company: "remote-humans" }, - "p2p-join": { authority: "wire", company: "remote-humans" }, - "draft-match": { authority: "wire", company: "remote-humans" }, - "spectate": { authority: "wire", company: "remote-humans" }, +export const GAME_MODE_TRAITS: Record = { + "ai": { authority: "client", company: "solo", seat: "seat-zero" }, + "local": { authority: "client", company: "solo", seat: "seat-zero" }, + "native-ai": { authority: "wire", company: "solo", seat: "seat-zero" }, + "online": { authority: "wire", company: "remote-humans", seat: "wire-assigned" }, + "p2p-host": { authority: "wire", company: "remote-humans", seat: "wire-assigned" }, + "p2p-join": { authority: "wire", company: "remote-humans", seat: "wire-assigned" }, + "draft-match": { authority: "wire", company: "remote-humans", seat: "wire-assigned" }, + "spectate": { authority: "wire", company: "remote-humans", seat: "no-seat" }, }; /** True when the authoritative engine state lives off this client — i.e. the @@ -144,6 +161,22 @@ export function hasRemoteHumans(mode: GameMode | null): boolean { return mode !== null && GAME_MODE_TRAITS[mode].company === "remote-humans"; } +/** + * The seat axis of the census: where this client's own seat number comes from. + * + * Read this instead of testing `gameMode` against a list at the call site. The + * list form is what seated a pod-draft guest at 0: `"draft-match"` joined the + * union long after `usePlayerId`'s list was written, and nothing made the two + * meet. A mode added to `GAME_MODE_TRAITS` cannot compile without declaring + * its seat source, so it can never again default into somebody else's chair. + * + * `null` — no game yet — is `"seat-zero"`: nothing has assigned this client + * anything. + */ +export function seatSource(mode: GameMode | null): SeatSource { + return mode === null ? "seat-zero" : GAME_MODE_TRAITS[mode].seat; +} + interface GameStoreState { gameId: string | null; gameMode: GameMode | null;