From fbbb54ab26dd0e3e90ce6748100e8e9209253588 Mon Sep 17 00:00:00 2001 From: Keith Avery Date: Fri, 24 Apr 2026 04:03:22 -0400 Subject: [PATCH 1/2] fix(ui): clear dice TARGET + result widget after narrator resolves the roll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a combat roll resolved, the InlineDiceTray's TARGET banner and "Rolled N vs M — Fail" badge stayed pinned through the next narration cycle. When the next action buttons became clickable, the previous roll's numbers were still on screen — players read the stale DC as the target for their next click. App.tsx now clears diceRequest and diceResult inside the NARRATION_END branch (the same turn-boundary signal that re-enables input and clears a resolved confrontation). The InlineDiceTray and DiceOverlay are prop-driven, so both widgets return to a neutral resting state without further changes — the 3D die stays idle until a new DICE_REQUEST triggers the next throw. Added a wiring test in dice-overlay-wiring-34-5.test.ts that asserts both setters are invoked inside the NARRATION_END branch, plus a regression guard that the overlay props still read from the cleared state. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/App.tsx | 19 +++++++-- .../dice-overlay-wiring-34-5.test.ts | 40 +++++++++++++++++++ src/dice/InlineDiceTray.tsx | 11 +++-- 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index e29aca4..ee68b76 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -259,10 +259,14 @@ function AppInner() { const pendingBeatIdRef = useRef(null); // Dice overlay persists after result so the table can see "rolled N vs - // target M → outcome" until the next beat begins. Cleared by: (a) a new - // DiceRequest arriving (DICE_REQUEST handler below), (b) a local beat - // click (handleBeatSelect sets a fresh request), (c) the confrontation - // ending — handled by the effect below. + // target M → outcome" through the narrator's resolution. Cleared by: + // (a) a new DiceRequest arriving (DICE_REQUEST handler below), + // (b) a local beat click (handleBeatSelect sets a fresh request), + // (c) the confrontation ending — handled by the effect below, + // (d) NARRATION_END — the narrator has accepted the roll; holding the + // stale TARGET/result widget past the turn boundary makes players + // read it as the DC for the next click. Cleared in the NARRATION_END + // branch of handleMessage (playtest-pingpong 2026-04-24). useEffect(() => { if (confrontationData) return; setDiceRequest(null); @@ -372,6 +376,13 @@ function AppInner() { setConfrontationData(null); } confrontationReceivedThisTurnRef.current = false; + // Clear the dice TARGET banner and roll-result widget once the + // narrator resolves the roll. Without this, the previous roll's + // "TARGET 18 · need 17" + "Rolled 4 vs 18 Fail" stays pinned + // beside the next set of beat buttons, and players read it as + // the DC for the next click (playtest-pingpong 2026-04-24). + setDiceRequest(null); + setDiceResult(null); } return; } diff --git a/src/__tests__/dice-overlay-wiring-34-5.test.ts b/src/__tests__/dice-overlay-wiring-34-5.test.ts index 6051318..d213246 100644 --- a/src/__tests__/dice-overlay-wiring-34-5.test.ts +++ b/src/__tests__/dice-overlay-wiring-34-5.test.ts @@ -246,3 +246,43 @@ describe("Wiring: Physics-is-the-roll (story 34-12)", () => { ); }); }); + +// ══════════════════════════════════════════════════════════════════════════════ +// Wiring: NARRATION_END clears the stale dice TARGET + result widget +// ────────────────────────────────────────────────────────────────────────────── +// Playtest-pingpong 2026-04-24: after a roll resolves, the TARGET banner and +// "Rolled N vs M — Fail" readout stayed on screen through the narrator's +// next turn. Players read the stale numbers as the DC for the next click. +// The fix: on NARRATION_END (the narrator's turn boundary, which also +// re-enables input), clear diceRequest + diceResult so the widgets return +// to a neutral state until the next beat issues a fresh DICE_REQUEST. +// ══════════════════════════════════════════════════════════════════════════════ + +describe("Wiring: NARRATION_END clears dice TARGET + result widget", () => { + it("App.tsx clears diceRequest and diceResult inside the NARRATION_END branch", () => { + const appSrc = readSrc("App.tsx"); + // Locate the NARRATION_END branch — the same block that toggles + // setCanType(true) and clears confrontation on turn boundary. + const narrationEndIdx = appSrc.indexOf( + "if (msg.type === MessageType.NARRATION_END) {", + ); + expect(narrationEndIdx).toBeGreaterThanOrEqual(0); + // Take a bounded window covering the branch body. + const body = appSrc.slice(narrationEndIdx, narrationEndIdx + 2000); + // Both dice state setters must be invoked with null inside this block. + // Without these, the previous roll's TARGET + result badge persists + // until the next DICE_REQUEST arrives. + expect(body).toMatch(/setDiceRequest\(\s*null\s*\)/); + expect(body).toMatch(/setDiceResult\(\s*null\s*\)/); + }); + + it("App.tsx still passes the cleared dice state through to the overlay", () => { + // Regression guard: if someone later moves the setters out of the + // NARRATION_END block, the overlay props wiring still reads from + // the same state. Keep the wiring surface pinned so the clear + // actually reaches the UI. + const appSrc = readSrc("App.tsx"); + expect(appSrc).toMatch(/diceRequest=\{diceRequest\}/); + expect(appSrc).toMatch(/diceResult=\{diceResult\}/); + }); +}); diff --git a/src/dice/InlineDiceTray.tsx b/src/dice/InlineDiceTray.tsx index 0f52404..6105633 100644 --- a/src/dice/InlineDiceTray.tsx +++ b/src/dice/InlineDiceTray.tsx @@ -302,10 +302,13 @@ export function InlineDiceTray({ diceRequest, diceResult, playerId, onThrow, gen {/* Result readout — "Rolled N vs Target M — Outcome". - Persists alongside the target banner above until the next beat - issues a new DiceRequest (App.tsx clears on new request / on - confrontation end). Tabletop parity: the DM never erases the - target number mid-resolution. */} + Persists alongside the target banner above through the + narrator's resolution. App.tsx clears the request + result on + new DiceRequest, on confrontation end, and on NARRATION_END + (the narrator's turn boundary) — so the stale TARGET doesn't + linger beside the next set of beat buttons. Tabletop parity: + the DM never erases the target mid-resolution, but the slate + wipes clean before the next roll. */} {diceResult && (
Date: Fri, 24 Apr 2026 04:31:59 -0400 Subject: [PATCH 2/2] fix(ui): reset slug-connect latches on Leave so next game opens WS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Playtest 2026-04-24 BLOCKING bug: leaving a game and starting a fresh one (any genre) hung on the post-POST navigate. POST /api/games returned 201 with the new slug, URL flipped to /solo/, but the UI stuck on "The pages are turning..." indefinitely. Server logs showed NO ws.connection_accepted, NO mp.slug_connect, NO chargen.phase — the client never even opened the WebSocket for the new session. Root cause: react-router-dom v6 reconciles children by element type + position. Our routes "/", "/solo/:slug", and "/play/:slug" all render at the same reconciler slot, so React reuses the AppInner instance across navigate(). All refs survive route changes, including slugConnectFired.current which latches to true after the first session's successful fetch + connect. handleLeave reset several refs (autoReconnectAttempted, seenEventKeys, sessionPhaseRef) but missed this one and justConnectedRef. On the second game's slug-connect effect run, the if (slugConnectFired.current) return; gate short-circuited — no fetch, no WebSocket, no recovery. Typing the URL manually "worked" only because it triggers a full page reload, which remounts AppInner with ref defaults. Not a race — a stale-ref session-lifecycle bug. Fix: - Move slugConnectFired ref declaration up to the other session-lifecycle refs so handleLeave can reset it alongside autoReconnectAttempted. - handleLeave now clears slugConnectFired.current, justConnectedRef.current, gameMetaError, and currentGenre so the next game's slug-connect effect starts from a clean slate. Wiring test: src/__tests__/lobby-start-ws-open.test.tsx drives the exact playtest sequence — mount at /solo/first-slug, drive through connect → ready, click Leave, click Start again. Asserts a fresh WebSocket connects and SESSION_EVENT{connect, game_slug: second-slug} arrives without any manual re-navigation. Verified this test FAILS (5s timeout waiting for secondServer.connected) when the fix is reverted. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/App.tsx | 25 ++- src/__tests__/lobby-start-ws-open.test.tsx | 244 +++++++++++++++++++++ 2 files changed, 268 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/lobby-start-ws-open.test.tsx diff --git a/src/App.tsx b/src/App.tsx index ee68b76..1e5ad6d 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -202,6 +202,15 @@ function AppInner() { // Latched when slug-connect fires so the mid-session reconnect effect // doesn't double-send SESSION_EVENT on the same WS OPEN transition. const justConnectedRef = useRef(false); + // Latched AFTER the slug-connect fetch + connect() runs successfully. + // Declared here (alongside the other session-lifecycle refs) rather than + // inline next to the effect so `handleLeave` can reset it — AppInner is a + // single persistent instance across "/" and "/solo/:slug" (react-router-dom + // v6 reuses the LobbyRoot element across route matches), so this ref + // survives navigate() and must be explicitly cleared on Leave or the + // next game's slug-connect effect short-circuits and never opens the WS + // (playtest 2026-04-24 "post-lobby hang" bug). + const slugConnectFired = useRef(false); // Overlay data from server messages const [characterSheet, setCharacterSheet] = useState(null); @@ -821,6 +830,19 @@ function AppInner() { sessionPhaseRef.current = "connect"; setSessionPhase("connect"); autoReconnectAttempted.current = false; + // Reset slug-connect session state so the NEXT game's slug-connect + // effect fetches GET /api/games/:new-slug and opens a fresh WebSocket. + // Without this, `slugConnectFired.current` stays latched to `true` from + // the prior session — the short-circuit at the top of the effect + // (`if (slugConnectFired.current) return;`) fires, no fetch happens, + // no WS opens, UI hangs on "The pages are turning…" forever. The user + // can only escape by typing the URL manually (full page reload resets + // all refs). Playtest 2026-04-24 "Post-lobby hang on new-genre first + // game" bug — confirmed genre-agnostic (MW → C&C, C&C → SO). + slugConnectFired.current = false; + justConnectedRef.current = false; + setGameMetaError(null); + setCurrentGenre(null); // Route off the slug — otherwise the slug-connect effect re-fires. // disconnect() above already flushed the SESSION_EVENT outbound. navigate("/"); @@ -924,7 +946,8 @@ function AppInner() { // // Dependency on `displayName` means it re-fires after NamePrompt confirms — // which is the intended behavior (we need a name before connecting). - const slugConnectFired = useRef(false); + // (`slugConnectFired` ref is declared above with the other + // session-lifecycle refs so `handleLeave` can reset it.) useEffect(() => { if (!slug) return; if (!displayName) return; // Wait for NamePrompt diff --git a/src/__tests__/lobby-start-ws-open.test.tsx b/src/__tests__/lobby-start-ws-open.test.tsx new file mode 100644 index 0000000..f60c51f --- /dev/null +++ b/src/__tests__/lobby-start-ws-open.test.tsx @@ -0,0 +1,244 @@ +// Lobby → Start → slug-connect wiring test (playtest 2026-04-24 BLOCKING bug) +// +// Bug: after playing one game and clicking Leave, starting a fresh game from +// the lobby (any genre) hung on the post-POST navigate. POST /api/games +// returned 201 + the new slug, the URL flipped to /solo/, AppInner +// saw the slug param change — but the slug-connect effect short-circuited +// on the `if (slugConnectFired.current) return;` gate. No GET /api/games/:slug +// was issued, no WebSocket opened, the UI stuck on "The pages are turning…". +// +// Root cause: react-router-dom v6 reconciles routes by element type + position. +// "/" and "/solo/:slug" both render `` from separate +// declarations — same type at the same reconciler slot inside → +// React reuses the AppInner instance across navigate(). All refs survive, +// including `slugConnectFired.current` which is latched to `true` after the +// first session's successful connect. `handleLeave` reset several refs +// (autoReconnectAttempted, seenEventKeys, sessionPhaseRef) but missed this +// one. On the second game's slug-connect effect run, the latch blocks the +// fetch and WS connect — the only escape is a full page reload (typing the +// URL manually), which re-mounts AppInner with ref defaults. +// +// Fix: handleLeave explicitly clears `slugConnectFired.current`, +// `justConnectedRef.current`, `gameMetaError`, and `currentGenre` so the +// next game starts from a clean slate. +// +// This test drives the exact playtest sequence: +// 1. Fresh mount at /solo/:slug-A (simulates the first game in progress). +// 2. Server sends SESSION_EVENT{ready} → AppInner enters game phase. +// 3. Click Leave → navigates back to "/". +// 4. Lobby renders ConnectScreen with pre-filled genre/world. +// 5. Click Start → POST /api/games returns {slug: slug-B, ...}. +// 6. Assert: a NEW WebSocket connects AND SESSION_EVENT{connect, game_slug: slug-B} +// arrives WITHOUT any manual re-navigation. + +import { StrictMode } from 'react'; +import { render, screen, act } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { MemoryRouter } from 'react-router-dom'; +import { WS } from 'jest-websocket-mock'; +import { installWebAudioMock, installLocalStorageMock } from '@/audio/__tests__/web-audio-mock'; +import { AudioEngine } from '@/audio/AudioEngine'; +import App from '../App'; + +const LOBBY_STORAGE_KEY = 'sidequest-connect'; + +const GENRES_RESPONSE = { + low_fantasy: { + name: 'Low Fantasy', + description: 'Gritty medieval adventures.', + worlds: [ + { + slug: 'greyhawk', + name: 'Greyhawk', + description: 'The Flanaess.', + era: null, + setting: null, + inspirations: [], + axis_snapshot: {}, + hero_image: null, + }, + ], + }, +}; + +const FIRST_SLUG = '2026-04-24-first-session'; +const SECOND_SLUG = '2026-04-24-second-session'; + +const GAME_META = { + genre_slug: 'low_fantasy', + world_slug: 'greyhawk', + mode: 'solo', +}; + +// Fetch mock that returns SECOND_SLUG for every POST /api/games (the test +// only exercises one Start click on the lobby path, so a single slug is +// sufficient — FIRST_SLUG comes from the initial route entry). +function makeFetchMock() { + return vi.fn().mockImplementation((url: string, opts?: RequestInit) => { + if (typeof url === 'string' && /\/api\/games\/[^?]+/.test(url) && (!opts || opts.method !== 'POST')) { + return Promise.resolve( + new Response(JSON.stringify(GAME_META), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ); + } + if (typeof url === 'string' && url === '/api/games' && opts?.method === 'POST') { + return Promise.resolve( + new Response(JSON.stringify({ slug: SECOND_SLUG, mode: 'solo' }), { + status: 201, + headers: { 'Content-Type': 'application/json' }, + }), + ); + } + if (typeof url === 'string' && url.startsWith('/api/sessions')) { + return Promise.resolve( + new Response(JSON.stringify({ sessions: [] }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ); + } + if (typeof url === 'string' && url.includes('/api/genres')) { + return Promise.resolve( + new Response(JSON.stringify(GENRES_RESPONSE), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ); + } + return Promise.resolve(new Response(JSON.stringify([]), { status: 200 })); + }); +} + +beforeEach(() => { + AudioEngine.resetInstance(); + installWebAudioMock(); + installLocalStorageMock(); + vi.stubGlobal('fetch', makeFetchMock()); +}); + +afterEach(() => { + WS.clean(); + AudioEngine.resetInstance(); + vi.unstubAllGlobals(); + localStorage.clear(); + document.documentElement.removeAttribute('data-archetype'); +}); + +describe('lobby → slug navigation — Leave + Start fresh game (playtest 2026-04-24 hang bug)', () => { + it('fresh lobby Start opens WebSocket + sends SESSION_EVENT{connect} for the new slug', async () => { + // Initial sanity: on a clean page load, the very first Start click must + // open the WS. This is the "first game of the session" smoke case. + localStorage.setItem( + LOBBY_STORAGE_KEY, + JSON.stringify({ + playerName: 'Keith', + genre: 'low_fantasy', + world: 'greyhawk', + }), + ); + + const wsUrl = `ws://${location.host}/ws`; + const server = new WS(wsUrl, { jsonProtocol: true }); + + const user = userEvent.setup(); + + render( + + + + + , + ); + + const startBtn = await screen.findByRole('button', { name: /start/i }); + await user.click(startBtn); + + await server.connected; + const msg = (await server.nextMessage) as { + type: string; + payload: Record; + }; + expect(msg.type).toBe('SESSION_EVENT'); + expect(msg.payload.event).toBe('connect'); + expect(msg.payload.game_slug).toBe(SECOND_SLUG); + expect(msg.payload.player_name).toBe('Keith'); + }); + + it('Leave + Start opens a new WebSocket for the new slug (no manual re-navigate)', async () => { + // Playtest 2026-04-24 exact repro: finish a game, click Leave (which + // navigates to "/"), pick a world in the lobby, click Start. Must open + // a WS and send SESSION_EVENT{connect} for the NEW slug — not hang on + // the "pages are turning" loader because slugConnectFired is stale. + localStorage.setItem( + LOBBY_STORAGE_KEY, + JSON.stringify({ + playerName: 'Keith', + genre: 'low_fantasy', + world: 'greyhawk', + }), + ); + localStorage.setItem('sq:display-name', 'Keith'); + + const wsUrl = `ws://${location.host}/ws`; + const firstServer = new WS(wsUrl, { jsonProtocol: true }); + + const user = userEvent.setup(); + + render( + + + + + , + ); + + // Boot the first session through connect → ready so GameBoard (and its + // Leave button) mounts. This mirrors the real playtest state. + await firstServer.connected; + const firstConnect = (await firstServer.nextMessage) as { + type: string; + payload: Record; + }; + expect(firstConnect.payload.game_slug).toBe(FIRST_SLUG); + + act(() => { + firstServer.send({ type: 'SESSION_EVENT', payload: { event: 'ready' } }); + }); + + // Leave button lives in the running header. Click it — AppInner should + // disconnect and navigate("/") so ConnectScreen renders. + const leaveBtn = await screen.findByRole('button', { name: /^leave$/i }); + await user.click(leaveBtn); + + // Back at "/" — ConnectScreen's Start button must be reachable. + const startBtn = await screen.findByRole('button', { name: /^start$/i }); + + // Stand up a SECOND WS server — the first one is closed via disconnect(). + // A fresh WS object is about to be created by the slug-connect effect + // for the new slug. `jest-websocket-mock` doesn't queue handshakes across + // two sequential WS servers at the same URL cleanly, so we close the + // first and build the second before the click. + firstServer.close(); + WS.clean(); + const secondServer = new WS(wsUrl, { jsonProtocol: true }); + + await user.click(startBtn); + + // THE GATE: this is where the bug hit. Without the fix, + // `slugConnectFired.current` is still true from the first session, the + // slug-connect effect short-circuits, and `secondServer.connected` never + // resolves — the test times out. + await secondServer.connected; + const secondConnect = (await secondServer.nextMessage) as { + type: string; + payload: Record; + }; + expect(secondConnect.type).toBe('SESSION_EVENT'); + expect(secondConnect.payload.event).toBe('connect'); + expect(secondConnect.payload.game_slug).toBe(SECOND_SLUG); + expect(secondConnect.payload.player_name).toBe('Keith'); + }); +});