Fix sandbox review findings - #7128
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change centralizes debug-action validation, supports validated zero-count no-ops, adds protocol acknowledgements, updates client handling, improves debug UI localization, surfaces scry results, and validates backup metadata before merging. ChangesZero-count debug actions
Client UI and session state
Backup record validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerSession
participant Engine
participant CardDatabase
Client->>ServerSession: submit debug action
ServerSession->>Engine: preflight_debug_action
alt zero-count creation
Engine-->>ServerSession: validated empty result
ServerSession-->>Client: ActionNoOp
else nonzero CreateCard
ServerSession->>CardDatabase: resolve card source
CardDatabase-->>ServerSession: card source
ServerSession->>Engine: create_debug_cards
Engine-->>ServerSession: state transition result
ServerSession-->>Client: action result and state update
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthewevans
enabled auto-merge
August 9, 2026 09:31
matthewevans
force-pushed
the
fix/sandbox-review-followup
branch
2 times, most recently
from
August 9, 2026 09:38
7833638 to
5f4ca7a
Compare
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
force-pushed
the
fix/sandbox-review-followup
branch
from
August 9, 2026 10:04
5f4ca7a to
f2dcbe8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@client/src/adapter/p2p-adapter.ts`:
- Around line 570-580: Replace action-shape-based no-op detection with a typed
engine outcome such as Applied or NoOp, and propagate it without erasing NoOp.
In client/src/adapter/p2p-adapter.ts at lines 570-580 remove
isZeroCountDebugCreate; update lines 1706-1710 to return the typed outcome,
lines 2165-2169 to emit action_noop only for NoOp, and lines 3058-3065 to
resolve pending requests with that outcome. In client/src/adapter/ws-adapter.ts
lines 1453-1461 preserve NoOp rather than converting it to an empty normal
result. Update client/src/game/dispatch.ts to return before getSnapshot() when
the resolved outcome is NoOp.
In `@client/src/game/dispatch.ts`:
- Line 508: Ensure completed-scry events received through
processRemoteUpdateInner trigger flashCompletedScry just like processAction
events, preferably from the shared event boundary to avoid duplicate
notifications. Preserve existing local behavior and add WebSocket/P2P coverage
verifying the public scry overlay appears for the affected seat.
In `@crates/phase-server/src/main.rs`:
- Around line 3865-3869: Update handle_full_game_submission to accept the
per-connection mpsc sender used by connections, and in the
is_zero_count_debug_create branch enqueue ServerMessage::ActionNoOp through that
sender instead of writing directly to socket. Ensure callers pass the matching
connection sender so acknowledgements preserve ordering with queued StateUpdate
messages.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 42dbc2cb-e6a9-41dd-92e1-f053a1f965fe
📒 Files selected for processing (36)
client/src/adapter/__tests__/p2p-adapter-multiplayer.test.tsclient/src/adapter/__tests__/wasm-adapter.test.tsclient/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/engine-worker.tsclient/src/adapter/p2p-adapter.tsclient/src/adapter/types.tsclient/src/adapter/wasm-adapter.tsclient/src/adapter/ws-adapter.tsclient/src/components/chrome/DebugCardContextMenu.tsxclient/src/components/chrome/DebugCreateActions.tsxclient/src/components/chrome/DebugObjectActions.tsxclient/src/components/settings/PreferencesModal.tsxclient/src/game/__tests__/diceContest.test.tsclient/src/game/__tests__/sessionCleanup.test.tsclient/src/game/dispatch.tsclient/src/game/sessionCleanup.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/network/__tests__/protocol.test.tsclient/src/network/protocol.tsclient/src/services/__tests__/backup.test.tsclient/src/services/backup.tscrates/engine-wasm/src/lib.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_debug.rscrates/engine/src/game/mod.rscrates/lobby-broker/src/protocol.rscrates/phase-server/src/main.rscrates/server-core/src/protocol.rscrates/server-core/src/session.rsscripts/check-protocol-version.mjs
💤 Files with no reviewable changes (1)
- client/src/adapter/engine-worker.ts
Comment on lines
+570
to
+580
| function isZeroCountDebugCreate(action: GameAction): boolean { | ||
| if (action.type !== "Debug") return false; | ||
| switch (action.data.type) { | ||
| case "CreateCard": | ||
| case "CreateToken": | ||
| case "CreateTokenCopy": | ||
| return action.data.data.count === 0; | ||
| default: | ||
| return false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep the no-op outcome engine-owned and typed.
The adapters derive no-op semantics from GameAction and then resolve ActionNoOp as a normal SubmitResult. client/src/game/dispatch.ts calls adapter.getSnapshot() after every resolved action. Therefore, the host path at Line 1709 still requests a worker snapshot for a zero-count action.
Expose a typed engine result such as Applied or NoOp. Propagate it through both adapters. Update client/src/game/dispatch.ts to return before snapshot retrieval for NoOp.
client/src/adapter/p2p-adapter.ts#L570-L580: remove the local action-shape classifier.client/src/adapter/p2p-adapter.ts#L1706-L1710: return the typed engine outcome without erasingNoOp.client/src/adapter/p2p-adapter.ts#L2165-L2169: sendaction_nooponly from the typed engine outcome.client/src/adapter/p2p-adapter.ts#L3058-L3065: resolve the pending request with the typedNoOpoutcome.client/src/adapter/ws-adapter.ts#L1453-L1461: preserve the typedNoOpoutcome instead of returning a normal empty result.
📍 Affects 2 files
client/src/adapter/p2p-adapter.ts#L570-L580(this comment)client/src/adapter/p2p-adapter.ts#L1706-L1710client/src/adapter/p2p-adapter.ts#L2165-L2169client/src/adapter/p2p-adapter.ts#L3058-L3065client/src/adapter/ws-adapter.ts#L1453-L1461
🤖 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/adapter/p2p-adapter.ts` around lines 570 - 580, Replace
action-shape-based no-op detection with a typed engine outcome such as Applied
or NoOp, and propagate it without erasing NoOp. In
client/src/adapter/p2p-adapter.ts at lines 570-580 remove
isZeroCountDebugCreate; update lines 1706-1710 to return the typed outcome,
lines 2165-2169 to emit action_noop only for NoOp, and lines 3058-3065 to
resolve pending requests with that outcome. In client/src/adapter/ws-adapter.ts
lines 1453-1461 preserve NoOp rather than converting it to an empty normal
result. Update client/src/game/dispatch.ts to return before getSnapshot() when
the resolved outcome is NoOp.
Source: Path instructions
| // way the turn banner bypasses the animation queue. These events are marked | ||
| // NON_VISUAL so normalizeEvents skips them below. | ||
| flashInGameRolls(events); | ||
| flashCompletedScry(events); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'processRemoteUpdateInner|flashInGameRolls|flashCompletedScry' \
client/src/game/dispatch.ts
rg -n -C 8 'flashCompletedScry|scry_top_count|scry_bottom_count' \
client/src/game/__tests__ client/src/adapter/__tests__Repository: phase-rs/phase
Length of output: 12193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dispatch.ts remote path =="
sed -n '790,900p' client/src/game/dispatch.ts
echo "== diceContest.ts implementation =="
fd -a diceContest.ts client/src/game/dispatch.ts client/src | head -20
rg -n -C 12 'export function flashCompletedScry|function flashCompletedScry|export const flashCompletedScry|flashCompletedScry' client/src/game/dispatch.ts client/src/game/*.ts
echo "== dispatch tests around remote/update =="
wc -l client/src/game/__tests__/dispatch.test.ts client/src/game/__tests__/diceContest.test.ts
sed -n '160,240p' client/src/game/__tests__/diceContest.test.ts
rg -n -C 8 'processRemote|remote update|flash.*Scry|scryOutcome' client/src/game __tests__ client/src/adapter client/src -g '*.test.ts' -g '*.test.js' -g '*.ts'Repository: phase-rs/phase
Length of output: 14886
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== diceContest.ts =="
sed -n '86,112p' client/src/game/diceContest.ts
echo "== scry tests =="
sed -n '142,220p' client/src/game/__tests__/diceContest.test.ts
echo "== all scry mentions in tests =="
rg -n -C 12 'flashCompletedScry|scryOutcome|scry_top_count|scry_bottom_count|playerPerformedAction|PlayerPerformedAction' client/src ** -g '*.test.ts' -g '*.test.js' -g '*.ts' | head -n 240Repository: phase-rs/phase
Length of output: 23367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== completed scry event type definitions =="
rg -n -C 8 'CompletedScryEvent|ProcessCompletedScry|PlayerPerformedAction' . -g '*.ts' -g '*.tsx' -g '*.rs'
echo "== scry engine/rust occurrences =="
rg -n -C 8 'Scry|scry_top_count|scry_bottom_count|ProcessCompletedScry|completed_scry|CompletedScry' . -g '*.rs' | head -n 260
echo "== adapter snapshots/events in ts =="
rg -n 'submitAction|processRemoteUpdate|processSnapshot|GameEvent' client/src/adapter client/src/game/dispatch.ts -g '*.ts'Repository: phase-rs/phase
Length of output: 50371
Route Scry counts through the remote update path.
processAction calls flashCompletedScry(events), but processRemoteUpdateInner handles remote snapshots separately and never calls this helper. Multiplayer clients can receive a completed-scry event without seeing the public scry-overlay result for that seat. Move the notification into the shared event boundary or add it to the remote path, then add WebSocket/P2P coverage.
🤖 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/dispatch.ts` at line 508, Ensure completed-scry events
received through processRemoteUpdateInner trigger flashCompletedScry just like
processAction events, preferably from the shared event boundary to avoid
duplicate notifications. Preserve existing local behavior and add WebSocket/P2P
coverage verifying the public scry overlay appears for the affected seat.
Source: Path instructions
Comment on lines
+3865
to
+3869
| 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; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Queue ActionNoOp through the per-connection sender.
Normal StateUpdate messages use the connection mpsc queue. This branch writes ActionNoOp directly to socket. If an earlier state update is queued, the acknowledgement can overtake that update and expose messages out of order.
Pass the same sender used by connections into handle_full_game_submission and enqueue ServerMessage::ActionNoOp through it.
Based on the transport-layer async-ordering requirement, acknowledgements and state updates must preserve per-connection ordering.
🤖 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 `@crates/phase-server/src/main.rs` around lines 3865 - 3869, Update
handle_full_game_submission to accept the per-connection mpsc sender used by
connections, and in the is_zero_count_debug_create branch enqueue
ServerMessage::ActionNoOp through that sender instead of writing directly to
socket. Ensure callers pass the matching connection sender so acknowledgements
preserve ordering with queued StateUpdate messages.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the follow-up review findings from #7123 after that PR merged.\n\n- completes debug create count, authorization, and zero-no-op handling across engine, WASM, WebSocket, and P2P\n- fixes scry overlay delivery, counter adjustment UI, and deck-sync merge validation\n- includes targeted regression coverage and protocol version updates
Summary by CodeRabbit