Address gemini review on pod draft fix: connected_seats fallback + dedupe broadcasts by matthewevans · Pull Request #1246 · phase-rs/phase

matthewevans · 2026-05-27T20:40:09Z

Follow-up to a08d4ce8c (which landed directly on main) addressing two HIGH findings from gemini-code-assist's review on PR #1246.

1. `connected_seats` defaults to "all disconnected" for in-flight saves

#[serde(default)] produces SeatFlags(vec![]) for host save snapshots
that predate this field. The view layer then read .get(i) -> false for
every human seat, rendering the entire pod as disconnected until the
first pick lazily-initialised the bitmap via ensure_len.

Fix: add SeatFlags::get_or(seat, default). The view-layer reads
(filter_for_player + filter_for_spectator) pass true, so missing
bitmap entries fall back to "connected". Other call sites still use
get where false-on-absent is correct (seats_picked_this_round reads).

2. Redundant broadcasts during auto-pick sweep

handlePick defaults to resolveBots: true, which internally runs
resolveBotPicks and a full broadcastViews after every pick. In an
N-seat sweep that's N redundant broadcasts plus the final one added in
the original fix.

Fix: pass resolveBots: false inside the loop so the per-pick
broadcast is suppressed. After the loop, run resolveBotPicks once and
broadcast once — but only if the round didn't complete via
allPicksSubmitted inside the last handlePick (which broadcasts on
its own).

Verified

cargo test -p draft-core — 114 pass
cargo clippy -p draft-core -p draft-wasm -p server-core --all-targets
pnpm run type-check && pnpm lint
pnpm test --run — 1018 pass

gemini-code-assist

Code Review

This pull request introduces a robust mechanism for tracking per-seat connection states and pick statuses during draft sessions. It replaces the simple picks_this_round counter with a seats_picked_this_round bitmap to prevent a single seat from repeatedly picking and forcing a pack-pass. It also introduces a connected_seats bitmap to authoritatively track and display player connection states in the UI. The review comments correctly identify a performance issue with redundant P2P broadcasts during auto-pick sweeps, as well as a critical bug where uninitialized connected_seats on loaded old saves default to false (incorrectly showing all players as disconnected in the UI).

gemini-code-assist · 2026-05-27T20:42:16Z

+    let anyPicked = false;
    for (let seat = 0; seat < this.activePodSize; seat++) {
+      if (this.picksThisRound.has(seat)) continue;
      try {
        const view = await this.adapter.getViewForSeat(seat);
        if (view.current_pack && view.current_pack.length > 0) {
          const randomIndex = Math.floor(Math.random() * view.current_pack.length);
          const card = view.current_pack[randomIndex];
          await this.handlePick(seat, card.instance_id);
+          anyPicked = true;
        }
      } catch (err) {
        console.error(`[P2PDraftHost] auto-pick failed for seat ${seat}:`, err);
      }
    }
+    // If the round did not exhaust via handlePick's internal allPicksSubmitted
+    // check, broadcast the swept state so guests don't sit on a stale view
+    // (and so the timer-restart logic in handlePick doesn't strand the UI).
+    if (anyPicked) {
+      await this.broadcastViews();
+    }
  }


[HIGH] Redundant P2P broadcasts and bot resolutions during auto-pick sweep. Evidence: client/src/adapter/p2p-draft-host.ts:805.
Why it matters: Calling handlePick with the default resolveBots = true inside the loop triggers redundant bot pick evaluations and expensive P2P view broadcasts for every single timed-out seat.
Suggested fix: Pass false to handlePick inside the loop, then resolve bots and broadcast once at the end.

let anyPicked = false; for (let seat = 0; seat < this.activePodSize; seat++) { if (this.picksThisRound.has(seat)) continue; try { const view = await this.adapter.getViewForSeat(seat); if (view.current_pack && view.current_pack.length > 0) { const randomIndex = Math.floor(Math.random() * view.current_pack.length); const card = view.current_pack[randomIndex]; await this.handlePick(seat, card.instance_id, false); anyPicked = true; } } catch (err) { console.error(`[P2PDraftHost] auto-pick failed for seat ${seat}:`, err); } } if (anyPicked) { await this.resolveBotPicks({ emit: true, persist: true }); const allPicked = await this.adapter.allPicksSubmitted(); if (!allPicked) { await this.broadcastViews(); } } }

gemini-code-assist · 2026-05-27T20:42:16Z

+    pub fn get(&self, seat: u8) -> bool {
+        self.0.get(seat as usize).copied().unwrap_or(false)
+    }


[HIGH] Uninitialized connected_seats defaults to false for loaded old saves. Evidence: crates/draft-core/src/types.rs:281.
Why it matters: When resuming an old draft session, connected_seats is empty, causing get to return false and incorrectly showing all players as disconnected in the UI until the first pick is made.
Suggested fix: Add a get_or helper to SeatFlags that accepts a default value.

pub fn get(&self, seat: u8) -> bool { self.0.get(seat as usize).copied().unwrap_or(false) } pub fn get_or(&self, seat: u8, default: bool) -> bool { self.0.get(seat as usize).copied().unwrap_or(default) }

gemini-code-assist · 2026-05-27T20:42:16Z

+                    // Source of truth: the runtime `connected_seats` bitmap,
+                    // populated via `DraftAction::SetSeatConnected` by the host
+                    // adapter on (dis)connect. Bots are always considered
+                    // connected by construction.
+                    DraftSeat::Human { .. } => session.connected_seats.get(i as u8),


[HIGH] Uninitialized connected_seats defaults to false for loaded old saves. Evidence: crates/draft-core/src/view.rs:160.
Why it matters: When resuming an old draft session, connected_seats is empty, causing get to return false and incorrectly showing all players as disconnected in the spectator view.
Suggested fix: Use get_or with a default of true to correctly reflect the connection state.

Suggested change

// Source of truth: the runtime `connected_seats` bitmap,

// populated via `DraftAction::SetSeatConnected` by the host

// adapter on (dis)connect. Bots are always considered

// connected by construction.

DraftSeat::Human { .. } => session.connected_seats.get(i as u8),

// Source of truth: the runtime `connected_seats` bitmap,

// populated via `DraftAction::SetSeatConnected` by the host

// adapter on (dis)connect. Bots are always considered

// connected by construction.

DraftSeat::Human { .. } => session.connected_seats.get_or(i as u8, true),

gemini-code-assist · 2026-05-27T20:42:17Z

+                    // Source of truth: the runtime `connected_seats` bitmap,
+                    // populated via `DraftAction::SetSeatConnected` by the host
+                    // adapter on (dis)connect. Bots are always considered
+                    // connected by construction.
+                    DraftSeat::Human { .. } => session.connected_seats.get(i as u8),


[HIGH] Uninitialized connected_seats defaults to false for loaded old saves. Evidence: crates/draft-core/src/view.rs:266.
Why it matters: When resuming an old draft session, connected_seats is empty, causing get to return false and incorrectly showing all players as disconnected in the player view.
Suggested fix: Use get_or with a default of true to correctly reflect the connection state.

Suggested change

// Source of truth: the runtime `connected_seats` bitmap,

// populated via `DraftAction::SetSeatConnected` by the host

// adapter on (dis)connect. Bots are always considered

// connected by construction.

DraftSeat::Human { .. } => session.connected_seats.get(i as u8),

// Source of truth: the runtime `connected_seats` bitmap,

// populated via `DraftAction::SetSeatConnected` by the host

// adapter on (dis)connect. Bots are always considered

// connected by construction.

DraftSeat::Human { .. } => session.connected_seats.get_or(i as u8, true),

…adcasts Two HIGH findings from the gemini-code-assist review: 1. Uninitialized `connected_seats` defaults to false for old saves. `#[serde(default)]` produces `SeatFlags(vec![])` for in-flight host saves that predate this field. The view layer then read `.get(i) -> false` for every human seat, rendering the entire pod as disconnected until the first pick lazily-initialised the bitmap via `ensure_len`. Fix: add `SeatFlags::get_or(seat, default)`. The view-layer reads (`filter_for_player` + `filter_for_spectator`) pass `true`, so missing bitmap entries fall back to "connected". Other call sites still use `get` where the false-on-absent semantic is correct (`seats_picked_this_round` reads). 2. Redundant broadcasts during the auto-pick sweep. `handlePick` defaults to `resolveBots: true`, which internally runs `resolveBotPicks` and a full `broadcastViews` after every pick. In an N-seat sweep that's N redundant broadcasts plus the final one I'd added. Fix: pass `resolveBots: false` inside the loop so the per-pick broadcast is suppressed. After the loop, run `resolveBotPicks` once and broadcast once — but only if the round didn't complete via `allPicksSubmitted` inside the last `handlePick` (which broadcasts on its own). Verified: - cargo test -p draft-core (114 pass) - cargo clippy -p draft-core -p draft-wasm -p server-core --all-targets - pnpm run type-check && pnpm lint - pnpm test --run (1018 pass)

matthewevans · 2026-05-27T20:56:59Z

@gemini-code-assist review — addressed both HIGH findings in commit e79a89c: SeatFlags::get_or with view-layer default true for missing slots, and resolveBots: false during the auto-pick sweep with a single post-sweep resolve+broadcast.

gemini-code-assist

Code Review

This pull request refactors the draft session's connection and pick-tracking logic to prevent a single seat from forcing a pack-pass (Bug #1) and to properly track runtime connection states. It replaces the simple picks_this_round counter with a per-seat seats_picked_this_round bitmap, introduces a SetSeatConnected action to authoritative engine-side connection tracking, and updates the frontend and adapters to handle these changes. One review comment was provided regarding adherence to the style guide rule R2 (No bool fields), which points out that using a raw bool for connection state in the SetSeatConnected action payload violates the repository's architectural guidelines and suggests using a typed enum instead.

gemini-code-assist · 2026-05-27T20:59:54Z

+    SetSeatConnected {
+        seat: u8,
+        connected: bool,
+    },


[MEDIUM] Adherence to R2 (No bool fields — parameterize with existing typed enums). Evidence: crates/draft-core/src/types.rs:361-364. Why it matters: Using a raw bool for connection state in DraftAction variant payloads bypasses the typed design space and violates the repository's R2 architectural rule. Suggested fix: Introduce a typed ConnectionStatus enum (e.g., Connected / Disconnected) and use it instead of bool.

References

R2. No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums instead. ^(link)

…adcasts Two HIGH findings from the gemini-code-assist review: 1. Uninitialized `connected_seats` defaults to false for old saves. `#[serde(default)]` produces `SeatFlags(vec![])` for in-flight host saves that predate this field. The view layer then read `.get(i) -> false` for every human seat, rendering the entire pod as disconnected until the first pick lazily-initialised the bitmap via `ensure_len`. Fix: add `SeatFlags::get_or(seat, default)`. The view-layer reads (`filter_for_player` + `filter_for_spectator`) pass `true`, so missing bitmap entries fall back to "connected". Other call sites still use `get` where the false-on-absent semantic is correct (`seats_picked_this_round` reads). 2. Redundant broadcasts during the auto-pick sweep. `handlePick` defaults to `resolveBots: true`, which internally runs `resolveBotPicks` and a full `broadcastViews` after every pick. In an N-seat sweep that's N redundant broadcasts plus the final one I'd added. Fix: pass `resolveBots: false` inside the loop so the per-pick broadcast is suppressed. After the loop, run `resolveBotPicks` once and broadcast once — but only if the round didn't complete via `allPicksSubmitted` inside the last `handlePick` (which broadcasts on its own). Verified: - cargo test -p draft-core (114 pass) - cargo clippy -p draft-core -p draft-wasm -p server-core --all-targets - pnpm run type-check && pnpm lint - pnpm test --run (1018 pass)

…n-frozen-timer-inv

matthewevans enabled auto-merge (squash) May 27, 2026 20:40

gemini-code-assist Bot reviewed May 27, 2026

View reviewed changes

matthewevans force-pushed the ship/fix-pod-draft-auto-pick-state-corruption-frozen-timer-inv branch from e79a89c to 47b8633 Compare May 27, 2026 22:23

matthewevans changed the title ~~Fix pod draft: auto-pick state corruption, frozen timer, invisible disconnects~~ Address gemini review on pod draft fix: connected_seats fallback + dedupe broadcasts May 27, 2026

Merge branch 'main' into ship/fix-pod-draft-auto-pick-state-corruptio…

0bec4dd

…n-frozen-timer-inv

matthewevans merged commit 13216eb into main May 27, 2026
9 checks passed

matthewevans deleted the ship/fix-pod-draft-auto-pick-state-corruption-frozen-timer-inv branch May 27, 2026 22:32

Navigation Menu

Uh oh!

Uh oh!

Address gemini review on pod draft fix: connected_seats fallback + dedupe broadcasts - #1246

Address gemini review on pod draft fix: connected_seats fallback + dedupe broadcasts#1246
matthewevans merged 2 commits into
mainfrom
ship/fix-pod-draft-auto-pick-state-corruption-frozen-timer-inv

matthewevans commented May 27, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

matthewevans commented May 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

matthewevans commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. connected_seats defaults to "all disconnected" for in-flight saves

2. Redundant broadcasts during auto-pick sweep

Verified

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

matthewevans commented May 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

matthewevans commented May 27, 2026 •

edited

Loading

1. `connected_seats` defaults to "all disconnected" for in-flight saves