Overhaul game page

[anoek]

This patch switches to using our GobanView class on the game page, completely getting rid of the pop out side bar and introducing a new layout option to have the player and clock info on the top and bottom of the board.

[github-actions]

Code Review

This is a substantial and well-structured refactor. A few issues worth noting.

Bug: Popovers not closed on component unmount

src/views/Game/Game.tsx

openSettings and openMoreActions both spin up imperative popover instances stored in settings_popover_ref and more_actions_popover_ref, but there is no useEffect cleanup that closes them when Game unmounts. If the user navigates away while either popover is open, the popover container remains in the DOM as an orphaned node (the popover utility appends it to document.body).

Suggested fix: add a cleanup effect, e.g.:

React.useEffect(() => {
    return () => {
        more_actions_popover_ref.current?.close();
        settings_popover_ref.current?.close();
    };
}, []);

Performance: MutationObserver { subtree: true } fires on every stone placement in SVG mode

src/views/Game/Game.tsx — stacked-layout effect

mutation_observer.observe(container, { childList: true, subtree: true });

The intent is to detect when PersistentElement replaces the inner .Goban node on a game-id change. PersistentElement appends the .Goban wrapper as a direct child of .goban-container, so { childList: true } without subtree is sufficient for that purpose.

With subtree: true, the observer also fires on every SVG element addition inside the board — i.e., on every stone placement in the SVG renderer. Each fire calls syncGobanObservation() + update(), which reads offsetWidth and writes card.style.width. During move-tree playback or AI review with many rapid moves, this causes repeated forced layout reads that are unnecessary because the board size has not changed.

Dead code: unreachable null guard in handleForkGameClick

src/views/Game/GameActionsPanel.tsx

if (\!user.anonymous && \!engine.rengo && \!goban.isAnalysisDisabled()) {
    if (\!goban) {   // unreachable: goban was already dereferenced above
        return;
    }
    showModal(ModalTypes.Fork, { goban });
}

The inner if (\!goban) guard can never be reached because goban.isAnalysisDisabled() was already called in the outer condition. This was carried over from the deleted GameDock.tsx. The inner guard can be dropped.

[github-actions]

Code Review — Bug Findings

PR: Overhaul game page (#3575)

---

1. ClockWithPauseOverlay shows wrong initial label for already-paused games

File: src/views/Game/PlayerCards.tsx — ClockWithPauseOverlay

The component initializes paused as false unconditionally:

const [paused, set_paused] = React.useState<boolean>(false);
React.useEffect(() => {
    const onClock = (clock: JGOFClockWithTransmitting | null) => {
        set_paused(\!\!clock?.pause_state);
    };
    goban.on("clock", onClock);
    ...
}, [goban]);

If a game is already paused when the component mounts (common for paused correspondence games), the button reads "Pause game" instead of "Resume game" until the next clock event fires. On correspondence servers, clock ticks are infrequent, so the wrong label may persist for a long time.

The established pattern elsewhere in this codebase (e.g. AntiGrief.tsx line 69) is to seed the initial state from goban.last_emitted_clock:

const [paused, set_paused] = React.useState<boolean>(
    \!\!goban.last_emitted_clock?.pause_state,
);

---

2. Moderators can no longer pause games — no replacement control provided

File: src/views/Game/PlayerCards.tsx — can_pause condition

const can_pause =
    \!goban.review_id &&
    user_is_player &&
    engine.phase \!== "finished" &&
    \!engine.rengo &&
    \!engine.config.disable_vacation;

The old GameDock also allowed moderators to pause games via user_can_intervene (user.is_moderator && phase \!== "finished"). That path is gone: GameModToolsPanel.tsx and GameActionsPanel.tsx have no pause/resume action. Moderators who need to pause a game mid-play have no UI affordance after this change.

---

3. Autoplay button removed with no replacement

File: src/views/Game/Game.tsx — removal of <FragBelowBoardControls />

FragBelowBoardControls rendered the full action-bar below the goban, including a play/pause autoplay toggle (goban_controller.togglePlayPause). The new MoveNumberSlider provides only single-step ±1 buttons. togglePlayPause has no remaining UI entry point — keyboard shortcuts don't cover it either. Reviewers who use autoplay to step through a game at a set speed will find the feature silently gone.

(The ±10 skip buttons are also removed; those at least have Page Up/Down keyboard equivalents.)

---

4. Unscoped document.querySelector in ResizeObserver effect

File: src/views/Game/Game.tsx — stacked-layout ResizeObserver

const root = document.querySelector(".GobanView.Game") as HTMLElement | null;

Using a global DOM query instead of a React ref means that in development (React 18 Strict Mode double-mounts) or during rapid route transitions where one Game unmounts while another mounts, the effect may attach to the wrong element. A React.useRef on the <GobanView> root would scope this correctly and is more aligned with how goban_view_ref is already used in this component.

[github-actions]

Code Review — Bugs Found

Three confirmed bugs in the new code:

---

1. ClockWithPauseOverlay shows wrong button on already-paused games

File: src/views/Game/PlayerCards.tsx

const [paused, set_paused] = React.useState<boolean>(false);
React.useEffect(() => {
    const onClock = (clock: JGOFClockWithTransmitting | null) => {
        set_paused(\!\!clock?.pause_state);
    };
    goban.on("clock", onClock);
    ...
}, [goban]);

paused is always initialised to false regardless of the actual game state. If the component mounts on a game that is already paused, the overlay shows "Pause game" (with a pause icon) instead of "Resume game" until the next clock event fires. A user who clicks at that moment sends a duplicate pause request to the server.

Fix: seed the initial state from the goban directly, e.g.

const [paused, set_paused] = React.useState<boolean>(() => \!\!goban.last_clock?.pause_state);

---

2. Undo-request attribution wrong for rengo secondary players

File: src/views/Game/GameStateHeader.tsx

const player =
    engine.undo_requested_by === engine.players.black?.id
        ? engine.players.black
        : engine.players.white;   // ← no fallback
return player?.username ?? "";

In a rengo game, secondary team members have IDs that are neither players.black.id nor players.white.id. When one of them requests an undo the ternary falls through to engine.players.white, incorrectly attributing the request to the white player. The old PlayControls.tsx had an explicit third branch for this:

if (requested_by === engine.players.black.id) return engine.players.black.username;
if (requested_by === engine.players.white.id) return engine.players.white.username;
return _("A player");   // ← rengo fallback

Fix: restore the three-way check instead of the binary ternary.

---

3. Score-breakdown popup permanently disabled in stacked and mobile layouts

File: src/views/Game/Game.tsx

const renderPlayerCard = (color: "black" | "white") => (
    <PlayerCard
        ...
        show_score_breakdown={false}   // ← always false
        onScoreClick={() => undefined} // ← no-op
    />
);

renderPlayerCard is called for both the stacked layout (player cards above/below the board) and the portrait/mobile layout. PlayerCards (the sidebar path) correctly manages a show_score_breakdown toggle state and passes toggleScorePopup as onScoreClick, but renderPlayerCard hard-codes the popup as permanently off. Clicking the score area in stacked or mobile layout does nothing.

[github-actions]

Code Review

Bug: falsy-zero in useCurrentMoveNumber inverts conditional-moves tab visibility at game start

GameHooks.ts line 100:

(goban: Goban | null) => goban?.engine.cur_move?.move_number || -1,

0 || -1 evaluates to -1, so at game start (before any move is played), useCurrentMoveNumber returns -1 instead of 0.

The new code in Game.tsx uses this to decide which player is to move:

const live_player_to_move =
    goban.engine.getMoveNumber() === current_move_number   // 0 === -1 → false at move 0
        ? player_to_move
        : goban.engine.playerNotToMove();                  // always taken at game start

At move 0, getMoveNumber() returns 0 but current_move_number is -1, so live_player_to_move is always playerNotToMove() — which is white, since black moves first. This inverts which player sees the conditional-moves tab on the very first move: black incorrectly sees it (it's their turn to play, not plan), and white doesn't (it isn't their turn, they should be able to plan).

The fix is to use nullish coalescing instead of the logical OR:

(goban: Goban | null) => goban?.engine.cur_move?.move_number ?? -1,

?? only falls back to -1 when the left side is null or undefined, leaving 0 (a valid move number) intact.

Note: the old GameDock code had the same || -1 bug but masked it with visibility: hidden rather than gating the DOM node entirely, so the effect was less visible. The new tab-based rendering makes the incorrect visibility more apparent.

[github-actions]

Finding 1: Missing navigation controls -- FragBelowBoardControls removed with no mouse-accessible replacement. FragBelowBoardControls provided gotoFirstMove, gotoLastMove, previous10Moves, forwardTenMoves, and togglePlayPause. MoveNumberSlider only has previousMove/nextMove (one step) and a drag slider. Keyboard shortcuts via GameKeyboardShortcuts still work, but mouse-only users reviewing a long game have lost fast-navigation. Suggestion: expose first/last/skip-10 buttons in MoveNumberSlider, or render a condensed equivalent inside the game-main always-panel.

[github-actions]

Finding 2: AI-detector role loses per-player AI Suspected indicator -- black_ai_suspected / white_ai_suspected were removed from PlayerCardsProps. The new GameModeratorAreaPanel guards with if (!user?.is_moderator || !game_id) return null. Meanwhile show_mod_tab in Game.tsx correctly includes user_detects_ai, so users with moderator_powers & AI_DETECTOR but is_moderator === false can open the gavel tab -- but GameModeratorAreaPanel returns null for them. The per-player AI Suspected label they previously saw inline on the player card is gone; GameModToolsPanel renders for them but only provides action buttons, not the passive flag. Suggestion: mirror the show_mod_tab logic in the GameModeratorAreaPanel guard (include user_detects_ai), or pass ai_suspected back into PlayerCard so the inline indicator still renders.

[GreenAsJade]

We need a decent effort to get e2e passing on this - I haven't tried yet, but it seems likely it will break selectors.

AI is pretty good at reasoning about e2e - let me know if/when you need me to do that - or maybe your AI can run e2e now and handle it?