Skip to content

Fix transport review findings #7129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
matthewevans merged 3 commits into from
Aug 9, 2026
Merged

Fix transport review findings #7129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae21354
fix(transport): preserve no-op ordering and remote scry alerts
matthewevans Aug 9, 2026
4e6331d
fix(scry): process every completed outcome
matthewevans Aug 9, 2026
c3c2ed8
fix(scry): queue concurrent outcome notices
matthewevans Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions client/src/game/__tests__/diceContest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,29 @@ describe("flashCompletedScry", () => {
expect(useUiStore.getState().scryOutcome).toBeNull();
});

it("queues every completed scry in an event batch in order", () => {
flashCompletedScry([scry(1, 1, 2), scry(2, 3, 0)]);

expect(useUiStore.getState().scryOutcome).toEqual({
playerId: 1,
topCount: 1,
bottomCount: 2,
});
expect(useUiStore.getState().scryOutcomeQueue).toEqual([
{ playerId: 2, topCount: 3, bottomCount: 0 },
]);

vi.advanceTimersByTime(4_000);
expect(useUiStore.getState().scryOutcome).toEqual({
playerId: 2,
topCount: 3,
bottomCount: 0,
});

vi.advanceTimersByTime(4_000);
expect(useUiStore.getState().scryOutcome).toBeNull();
});

it("does not show a result for an incomplete or unrelated player action", () => {
flashCompletedScry([
{
Expand Down
36 changes: 15 additions & 21 deletions client/src/game/diceContest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,6 @@ import { useUiStore } from "../stores/uiStore";
type DieRolledEvent = Extract<GameEvent, { type: "DieRolled" }>;
type CoinFlippedEvent = Extract<GameEvent, { type: "CoinFlipped" }>;
type StartingPlayerContestEvent = Extract<GameEvent, { type: "StartingPlayerContest" }>;
type ScryEvent = Extract<GameEvent, { type: "PlayerPerformedAction" }>;
type CompletedScryEvent = ScryEvent & {
data: ScryEvent["data"] & {
action: "Scry";
scry_top_count: number;
scry_bottom_count: number;
};
};

/**
* Fire the starting-player contest overlay from a game-start event batch.
Expand Down Expand Up @@ -90,17 +82,19 @@ export function flashInGameRolls(events: GameEvent[]): void {
* no display effect.
*/
export function flashCompletedScry(events: GameEvent[]): void {
const scry = events.find(
(event): event is CompletedScryEvent =>
event.type === "PlayerPerformedAction" &&
event.data.action === "Scry" &&
event.data.scry_top_count !== undefined &&
event.data.scry_bottom_count !== undefined,
);
if (!scry) return;
useUiStore.getState().flashScryOutcome({
playerId: scry.data.player_id,
topCount: scry.data.scry_top_count,
bottomCount: scry.data.scry_bottom_count,
});
for (const scry of events) {
if (
scry.type !== "PlayerPerformedAction" ||
scry.data.action !== "Scry" ||
scry.data.scry_top_count === undefined ||
scry.data.scry_bottom_count === undefined
) {
continue;
}
useUiStore.getState().flashScryOutcome({
playerId: scry.data.player_id,
topCount: scry.data.scry_top_count,
bottomCount: scry.data.scry_bottom_count,
});
Comment on lines +94 to +98

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

fd -i -t f 'uiStore\.ts$' client | while IFS= read -r file; do
  rg -n -A8 -B4 'flashScryOutcome|scryOutcomeTimer|scryOutcome' "$file"
done

Repository: phase-rs/phase

Length of output: 4287

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "== candidate files =="
fd -i -t f 'diceContest\.ts$|uiStore\.ts$' client

echo
echo "== diceContest relevant section =="
file="$(fd -i -t f 'diceContest\.ts$' client | head -n 1)"
if [ -n "${file:-}" ]; then
  wc -l "$file"
  sed -n '70,115p' "$file" | cat -n
fi

echo
echo "== all scryOutcome flash usages =="
rg -n "flashScryOutcome|resetScryOutcome|scryOutcome" client --glob '!**/*.snap'

echo
echo "== package/test availability =="
git ls-files client | awk 'tolower($0) ~/test|\.test\./ || /scry/i' | sort -u | head -n 80

Repository: phase-rs/phase

Length of output: 8359

Queue completed-scry outcomes instead of replacing the active notice.

When one event batch contains multiple completed Scry actions, this code calls flashScryOutcome for each action. The UI store has one scryOutcome, and each call replaces the previous payload and resets its timer. Therefore, only the last outcome remains visible.

Make the notification path FIFO, or add a batch API that displays every outcome in event order. Add a regression test with two completed Scry events.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/src/game/diceContest.ts` around lines 94 - 98, Update the
completed-Scry notification flow around flashScryOutcome so multiple outcomes
from one event batch are queued and displayed FIFO rather than replacing the
active scryOutcome or resetting its timer. Preserve event order, and add a
regression test covering two completed Scry events.

Source: Path instructions

}
}
3 changes: 3 additions & 0 deletions client/src/game/dispatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,9 @@ async function processRemoteUpdateInner(
useUiStore.getState().flashTurnBanner(bannerText, turnNumber);
}

flashInGameRolls(events);
flashCompletedScry(events);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// 3. Normalize events into animation steps
const pacingMultipliers = usePreferencesStore.getState().pacingMultipliers;
const steps = normalizeEvents(events, { pacingMultipliers });
Expand Down
52 changes: 37 additions & 15 deletions client/src/stores/uiStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,11 @@ function flushPendingShow(): void {
apply();
}

// Serial FIFO for dice/coin overlays. Full-screen "moment" overlays are mutually
// exclusive (you can't show two rolls at once), so simultaneous/back-to-back
// rolls play one after another rather than clobbering. `diceRoll` is the active
// payload; `diceRollQueue` holds the pending ones. Distinct from the board-event
// step queue (animationStore) — that coordinates spatial per-object effects.
// Serial FIFOs for transient game outcomes. Full-screen dice/coin overlays and
// board-visible scry notices each show one payload at a time, so simultaneous
// outcomes play in event order instead of clobbering one another. The queues are
// distinct from the board-event step queue (animationStore), which coordinates
// spatial per-object effects.
let diceAdvanceTimer: ReturnType<typeof setTimeout> | null = null;
let scryOutcomeTimer: ReturnType<typeof setTimeout> | null = null;

Expand Down Expand Up @@ -167,6 +167,25 @@ function advanceDiceQueue(): void {
scheduleDiceAdvance(next);
}

function scheduleScryOutcomeAdvance(): void {
if (scryOutcomeTimer) {
clearTimeout(scryOutcomeTimer);
}
scryOutcomeTimer = setTimeout(advanceScryOutcomeQueue, 4_000);
}

function advanceScryOutcomeQueue(): void {
const queue = useUiStore.getState().scryOutcomeQueue;
if (queue.length === 0) {
useUiStore.setState({ scryOutcome: null });
scryOutcomeTimer = null;
return;
}
const next = queue[0];
useUiStore.setState({ scryOutcome: next, scryOutcomeQueue: queue.slice(1) });
scheduleScryOutcomeAdvance();
}

interface UiStoreState {
selectedObjectId: ObjectId | null;
hoveredObjectId: ObjectId | null;
Expand Down Expand Up @@ -200,8 +219,10 @@ interface UiStoreState {
/** Pending dice/coin overlays behind the active one. Simultaneous or
* back-to-back rolls play serially instead of clobbering. */
diceRollQueue: DiceRollPayload[];
/** Latest engine-authored public scry result, temporarily shown on board. */
/** Active engine-authored public scry result, temporarily shown on board. */
scryOutcome: ScryOutcomePayload | null;
/** Pending public scry notices, shown FIFO after the active outcome. */
scryOutcomeQueue: ScryOutcomePayload[];
focusedOpponent: number | null;
pendingAbilityChoice: { objectId: ObjectId; actions: ObjectAction[] } | null;
/** When non-null, the AttachmentsDialog is open showing every Aura
Expand Down Expand Up @@ -313,9 +334,9 @@ interface UiStoreActions {
/** Dismiss the current dice/coin overlay immediately (user tap-to-skip),
* advancing to the next queued roll if any. */
skipDiceRoll: () => void;
/** Surface one public scry outcome for a short, non-interactive board notice. */
/** Queue one public scry outcome for a short, non-interactive board notice. */
flashScryOutcome: (payload: ScryOutcomePayload) => void;
/** Clear a visible scry result on a game-session boundary. */
/** Clear the active and queued scry results on a game-session boundary. */
resetScryOutcome: () => void;
setFocusedOpponent: (id: number | null) => void;
setPendingAbilityChoice: (choice: { objectId: ObjectId; actions: ObjectAction[] } | null) => void;
Expand Down Expand Up @@ -377,6 +398,7 @@ export const useUiStore = create<UiStore>()((set, get) => ({
diceRoll: null,
diceRollQueue: [],
scryOutcome: null,
scryOutcomeQueue: [],
focusedOpponent: null,
pendingAbilityChoice: null,
enchantmentsDialogPlayer: null,
Expand Down Expand Up @@ -687,19 +709,19 @@ export const useUiStore = create<UiStore>()((set, get) => ({
advanceDiceQueue();
},
flashScryOutcome: (payload) => {
if (scryOutcomeTimer) clearTimeout(scryOutcomeTimer);
set({ scryOutcome: payload });
scryOutcomeTimer = setTimeout(() => {
scryOutcomeTimer = null;
set({ scryOutcome: null });
}, 4_000);
if (get().scryOutcome === null) {
set({ scryOutcome: payload });
scheduleScryOutcomeAdvance();
} else {
set({ scryOutcomeQueue: [...get().scryOutcomeQueue, payload] });
}
},
resetScryOutcome: () => {
if (scryOutcomeTimer) {
clearTimeout(scryOutcomeTimer);
scryOutcomeTimer = null;
}
set({ scryOutcome: null });
set({ scryOutcome: null, scryOutcomeQueue: [] });
},
setFocusedOpponent: (id) => set({ focusedOpponent: id }),
setPendingAbilityChoice: (choice) => set({ pendingAbilityChoice: choice }),
Expand Down
8 changes: 5 additions & 3 deletions crates/phase-server/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2072,6 +2072,7 @@ async fn handle_socket(

loop {
tokio::select! {
biased;
Some(msg) = rx.recv() => {
if let Ok(json) = serde_json::to_string(&msg) {
if socket.send(Message::text(json)).await.is_err() {
Expand Down Expand Up @@ -3801,6 +3802,7 @@ async fn handle_full_game_submission(
db: &SharedDb,
draft_state: &SharedDraftState,
connections: &SharedConnections,
tx: &mpsc::UnboundedSender<ServerMessage>,
game_db: &SharedGameDb,
game_spectators: &SharedGameSpectators,
// Read-only: this handler reads `game_code`, `player_token`, and `player_id`
Expand Down Expand Up @@ -3864,9 +3866,7 @@ async fn handle_full_game_submission(
Ok(human_result) => {
if is_zero_count_debug_create {
drop(mgr);
if let Ok(json) = serde_json::to_string(&ServerMessage::ActionNoOp) {
let _ = socket.send(Message::text(json)).await;
}
let _ = tx.send(ServerMessage::ActionNoOp);
return;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
let human_revision = mgr
Expand Down Expand Up @@ -4478,6 +4478,7 @@ async fn handle_client_message(
db,
draft_state,
connections,
tx,
game_db,
game_spectators,
identity,
Expand All @@ -4493,6 +4494,7 @@ async fn handle_client_message(
db,
draft_state,
connections,
tx,
game_db,
game_spectators,
identity,
Expand Down
Loading