Fix shell art and fullscreen - #6520
Conversation
📝 WalkthroughWalkthroughThe PR improves native pregame error propagation, adds Tauri fullscreen support, permits fullscreen and Scryfall image access in desktop configuration, and updates slider focus-visible styling. ChangesNative pregame and desktop client updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)client/src-tauri/capabilities/default.jsonTraceback (most recent call last): client/src-tauri/tauri.conf.jsonTraceback (most recent call last): Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/adapter/ws-adapter.ts (1)
1329-1343: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
DECK_REJECTEDbranch is now unreachable — deck-not-legal errors get the wrong code.
rejectInitialization(...)at Line 1331 setsthis.initReject = nullwhenever an init promise is pending. The subsequentdata.message.includes("Deck not legal") && this.initRejectguard is therefore always false, soinitialize()now rejects withactionRejectionError(...)(ACTION_REJECTED,recoverable=true) instead ofDECK_REJECTED(recoverable=false). Any caller that branches on the deck-rejected code/recoverability will misclassify the failure.Reorder so the deck-specific rejection wins before the generic init rejection:
🐛 Proposed reordering
case "Error": { const data = msg.data as { message: string }; - this.rejectInitialization(actionRejectionError(data.message)); this.rejectPregameMutation(actionRejectionError(data.message)); this.rejectAbandon(actionRejectionError(data.message)); this.emit({ type: "error", message: data.message }); if (data.message.includes("Deck not legal") && this.initReject) { this.initReject( new AdapterError("DECK_REJECTED", data.message, false), ); this.initResolve = null; this.initReject = null; + this.rejectInitialization(actionRejectionError(data.message)); + break; } + this.rejectInitialization(actionRejectionError(data.message)); break; }🤖 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/ws-adapter.ts` around lines 1329 - 1343, Reorder the Error handling in the message-processing switch so the “Deck not legal” case invokes initReject with AdapterError("DECK_REJECTED", ..., false) before rejectInitialization can clear it. Preserve the existing generic rejectInitialization, pregame, abandon, and error-event behavior for all other messages.
🧹 Nitpick comments (1)
client/src/providers/GameProvider.tsx (1)
793-841: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNative-P2P-options resolution is duplicated across
GameProvider.tsxandmultiplayerStore.ts. Both independently callensureNativeEngine(nativeEngineKey), build the same{ expectedServerVersion }shape, and warn-and-fall-back-to-WASM on failure — one shared helper would keep the two hosting entry points from drifting.
client/src/providers/GameProvider.tsx#L793-L841: extract theensureNativeEngine/nativeP2Pconstruction (and its WASM-fallbackcatch) into a shared helper (e.g.resolveNativeP2POptions) importable from both files.client/src/stores/multiplayerStore.ts#L1062-L1078: replace this block's inlineensureNativeEngine/nativeP2Pconstruction with the same shared helper.🤖 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/providers/GameProvider.tsx` around lines 793 - 841, Extract the duplicated native-P2P resolution logic into a shared resolveNativeP2POptions helper, including ensureNativeEngine, expectedServerVersion construction, and warning-based WASM fallback. Update client/src/providers/GameProvider.tsx lines 793-841 and client/src/stores/multiplayerStore.ts lines 1062-1078 to call the helper, preserving the existing native-resume failure behavior and passing each caller’s nativeEngineKey/context as needed.
🤖 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/ws-adapter.ts`:
- Around line 706-714: Update dispose() to reject all pending lifecycle waiters
by invoking rejectInitialization(...) (or explicitly rejecting the pregame and
gameStarted waiters) before clearing initResolve/initReject. Ensure
initializePregame() and waitForGameStarted() settle even when disposal occurs
before this.ws exists, while preserving the existing mutation and abandonment
rejections.
In `@client/src/components/chrome/FullscreenButton.tsx`:
- Around line 38-54: Update the toggle callback in FullscreenButton’s toggle
function to catch rejected Tauri and browser fullscreen operations, including
import and API failures. Preserve the existing fullscreen state when any
operation fails, and ensure the promise invoked by the onClick handler does not
produce an unhandled rejection.
- Around line 38-45: Update the Tauri branch of the FullscreenButton state
management around toggle to subscribe to the current window’s fullscreen state
changes and update isFullscreen whenever external Tauri changes occur. Reuse the
Tauri window API as the source of truth, clean up the listener on unmount or
dependency changes, and preserve the existing browser fullscreenchange handling.
In `@client/src/components/settings/PreferencesModal.tsx`:
- Line 1240: Update the range input styling near the PreferencesModal control to
preserve a visible focus fallback in forced-colors mode: remove
focus-visible:outline-none or replace it with a focus-visible outline and
suitable offset. Keep the existing focus-visible ring styling intact.
---
Outside diff comments:
In `@client/src/adapter/ws-adapter.ts`:
- Around line 1329-1343: Reorder the Error handling in the message-processing
switch so the “Deck not legal” case invokes initReject with
AdapterError("DECK_REJECTED", ..., false) before rejectInitialization can clear
it. Preserve the existing generic rejectInitialization, pregame, abandon, and
error-event behavior for all other messages.
---
Nitpick comments:
In `@client/src/providers/GameProvider.tsx`:
- Around line 793-841: Extract the duplicated native-P2P resolution logic into a
shared resolveNativeP2POptions helper, including ensureNativeEngine,
expectedServerVersion construction, and warning-based WASM fallback. Update
client/src/providers/GameProvider.tsx lines 793-841 and
client/src/stores/multiplayerStore.ts lines 1062-1078 to call the helper,
preserving the existing native-resume failure behavior and passing each caller’s
nativeEngineKey/context as needed.
🪄 Autofix (Beta)
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: 8e3c29c2-ed76-441b-945e-9b409cb27c29
📒 Files selected for processing (30)
client/src-tauri/capabilities/default.jsonclient/src-tauri/tauri.conf.jsonclient/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/p2p-adapter.tsclient/src/adapter/ws-adapter.tsclient/src/components/chrome/FullscreenButton.tsxclient/src/components/menu/MenuActionTile.tsxclient/src/components/settings/PreferencesModal.tsxclient/src/components/settings/__tests__/PreferencesModal.priorityPassing.test.tsxclient/src/i18n/locales/de/settings.jsonclient/src/i18n/locales/en/settings.jsonclient/src/i18n/locales/es/settings.jsonclient/src/i18n/locales/fr/settings.jsonclient/src/i18n/locales/it/settings.jsonclient/src/i18n/locales/pl/settings.jsonclient/src/i18n/locales/pt/settings.jsonclient/src/providers/GameProvider.tsxclient/src/services/gamePersistence.tsclient/src/stores/multiplayerStore.tsclient/src/stores/preferencesStore.tscrates/lobby-broker/src/protocol.rscrates/phase-server/src/main.rscrates/server-core/src/client_message_wire_guard.rscrates/server-core/src/lib.rscrates/server-core/src/persist.rscrates/server-core/src/protocol.rscrates/server-core/src/session.rsfixtures/adapter-contract/game_started.jsonfixtures/adapter-contract/state_update.jsonscripts/check-protocol-version.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/adapter/ws-adapter.ts (2)
438-443: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop sockets that finish connecting after disposal.
If
dispose()runs whileopenPhaseSocket()is pending,this.wsis still null. Once the await resumes, this method still installs the socket, starts pinging, and sends the setup frame—leaking a native connection and potentially creating/attaching a session after teardown. Checkthis.disposedimmediately after the await, closesocket, and return.As per path instructions, check async races, including state updates after unmount.
🤖 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/ws-adapter.ts` around lines 438 - 443, Update attachSocket so it checks this.disposed immediately after openPhaseSocket resolves; if disposed, close the newly opened socket and return before assigning it, starting pinging, or sending the setup frame. Preserve the existing setup flow when disposal has not occurred.Source: Path instructions
520-541: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject all lifecycle waiters when the socket closes.
initializePregame()andwaitForGameStarted()can be pending together, but thiselse ifchain rejects onlypregameReject;gameStartedRejectthen remains pending forever. Replace the branch with onerejectInitialization(new AdapterError(...))call so every active lifecycle latch settles.As per path instructions, check async races, including state updates after unmount.
🤖 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/ws-adapter.ts` around lines 520 - 541, Update the socket-close handling around initializePregame and waitForGameStarted to call rejectInitialization once with the WS_CLOSED AdapterError instead of using the mutually exclusive initReject/pregameReject/gameStartedReject chain. Ensure rejectInitialization settles every active lifecycle waiter and clears their state, including concurrent waiters, while preserving safe behavior if closure races with unmount or other state updates.Source: Path instructions
🤖 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/components/chrome/FullscreenButton.tsx`:
- Around line 39-55: Update the fullscreen synchronization effect around
syncFullscreen and the onResized registration so syncFullscreen handles its own
errors, including resize-triggered rejections. When onResized resolves,
immediately invoke the returned unsubscribe if active is already false;
otherwise retain it for normal cleanup. Preserve the existing active guard and
cleanup behavior in the effect.
---
Outside diff comments:
In `@client/src/adapter/ws-adapter.ts`:
- Around line 438-443: Update attachSocket so it checks this.disposed
immediately after openPhaseSocket resolves; if disposed, close the newly opened
socket and return before assigning it, starting pinging, or sending the setup
frame. Preserve the existing setup flow when disposal has not occurred.
- Around line 520-541: Update the socket-close handling around initializePregame
and waitForGameStarted to call rejectInitialization once with the WS_CLOSED
AdapterError instead of using the mutually exclusive
initReject/pregameReject/gameStartedReject chain. Ensure rejectInitialization
settles every active lifecycle waiter and clears their state, including
concurrent waiters, while preserving safe behavior if closure races with unmount
or other state updates.
🪄 Autofix (Beta)
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: 0b92ec05-6c3a-4df1-b16c-ab26256e90f1
📒 Files selected for processing (4)
client/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/ws-adapter.tsclient/src/components/chrome/FullscreenButton.tsxclient/src/components/settings/PreferencesModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/components/settings/PreferencesModal.tsx
| const syncFullscreen = async () => { | ||
| const fullscreen = await appWindow.isFullscreen(); | ||
| if (active) setIsFullscreen(fullscreen); | ||
| }; | ||
|
|
||
| await syncFullscreen(); | ||
| unlisten = await appWindow.onResized(() => { | ||
| void syncFullscreen(); | ||
| }); | ||
| } catch (error) { | ||
| console.warn("[phase.rs] Could not synchronize Tauri fullscreen state.", error); | ||
| } | ||
| })(); | ||
|
|
||
| return () => { | ||
| active = false; | ||
| unlisten?.(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the asynchronously registered listener and catch later sync failures.
If cleanup runs before onResized() resolves, unlisten is assigned afterward and never called. Also, resize-triggered syncFullscreen() rejections bypass the outer try/catch. Dispose immediately when inactive and handle errors inside syncFullscreen.
As per path instructions, client/src/** requires unmount cleanup for subscriptions and calls out async races after unmount.
🤖 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/components/chrome/FullscreenButton.tsx` around lines 39 - 55,
Update the fullscreen synchronization effect around syncFullscreen and the
onResized registration so syncFullscreen handles its own errors, including
resize-triggered rejections. When onResized resolves, immediately invoke the
returned unsubscribe if active is already false; otherwise retain it for normal
cleanup. Preserve the existing active guard and cleanup behavior in the effect.
Source: Path instructions
fdd7858 to
48fac68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/adapter/ws-adapter.ts (2)
693-699: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel the in-flight handshake when disposing.
dispose()can run beforeopenPhaseSocket()resolves, whenthis.wsis still null. The pendingattachSocket()then continues, installs a socket, starts pinging, and sends the setup frame after disposal. Abort or retain-and-close the pending transport, and return fromattachSocket()ifdisposedbefore assigningthis.ws.As per path instructions, check “async races (state updates after unmount, two reconnects at once).”
🤖 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/ws-adapter.ts` around lines 693 - 699, Update dispose and the attachSocket/openPhaseSocket flow to cancel or retain-and-close any transport that resolves after disposal. In attachSocket, check the disposed state immediately after the pending transport resolves and before assigning this.ws, starting pinging, or sending the setup frame; close the resolved transport and return when disposed, while preserving normal initialization otherwise.Source: Path instructions
524-541: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject every native lifecycle waiter on socket close.
When
initializePregame()andwaitForGameStarted()are both pending, thisif/else ifchain rejects onlypregameReject;gameStartedremains pending forever. UserejectInitialization(...)here, asdispose()already does, and add a close-path regression test.Suggested change
- if (this.initReject) { - // ... - } else if (this.pregameReject) { - // ... - } else if (this.gameStartedReject) { - // ... + if (this.initReject || this.pregameReject || this.gameStartedReject) { + this.rejectInitialization( + new AdapterError("WS_CLOSED", "Connection closed before initialization completed", true), + ); } else if (this.snapshot !== null || this.playerToken !== null) {As per path instructions, check “async races.”
🤖 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/ws-adapter.ts` around lines 524 - 541, Update the socket-close handling in the adapter lifecycle flow to call the existing rejectInitialization(...) helper instead of branching across initReject, pregameReject, and gameStartedReject, ensuring every pending native lifecycle waiter is rejected and cleared. Add a regression test covering simultaneous initializePregame() and waitForGameStarted() pending promises when the socket closes, including the async-race behavior.Source: Path instructions
🤖 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/ws-adapter.ts`:
- Around line 1332-1335: Replace the message-text check in the initialization
rejection handling with a typed wire-protocol rejection/error discriminant,
using the enum value for illegal decks to create DECK_REJECTED and preserving
actionRejectionError for other typed values. Update the associated test to send
the typed discriminant instead of relying on “Deck not legal” text, and follow
the protocol’s typed-enum convention throughout.
---
Outside diff comments:
In `@client/src/adapter/ws-adapter.ts`:
- Around line 693-699: Update dispose and the attachSocket/openPhaseSocket flow
to cancel or retain-and-close any transport that resolves after disposal. In
attachSocket, check the disposed state immediately after the pending transport
resolves and before assigning this.ws, starting pinging, or sending the setup
frame; close the resolved transport and return when disposed, while preserving
normal initialization otherwise.
- Around line 524-541: Update the socket-close handling in the adapter lifecycle
flow to call the existing rejectInitialization(...) helper instead of branching
across initReject, pregameReject, and gameStartedReject, ensuring every pending
native lifecycle waiter is rejected and cleared. Add a regression test covering
simultaneous initializePregame() and waitForGameStarted() pending promises when
the socket closes, including the async-race behavior.
🪄 Autofix (Beta)
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: 2f0deb98-869a-46fa-93f8-c55ba10fdb3d
📒 Files selected for processing (6)
client/src-tauri/capabilities/default.jsonclient/src-tauri/tauri.conf.jsonclient/src/adapter/__tests__/ws-adapter.test.tsclient/src/adapter/ws-adapter.tsclient/src/components/chrome/FullscreenButton.tsxclient/src/components/settings/PreferencesModal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src-tauri/capabilities/default.json
- client/src/components/chrome/FullscreenButton.tsx
| const initializationError = data.message.includes("Deck not legal") | ||
| ? new AdapterError("DECK_REJECTED", data.message, false) | ||
| : actionRejectionError(data.message); | ||
| this.rejectInitialization(initializationError); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not derive a protocol error code from message text.
includes("Deck not legal") is brittle: server wording changes can turn a fatal illegal-deck failure into a recoverable ACTION_REJECTED, or misclassify an unrelated error. Add a typed rejection/error discriminant to the wire protocol and map that enum to DECK_REJECTED; update this test to send the typed value.
As per path instructions, use “typed enums over stringly/bool data.”
🤖 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/ws-adapter.ts` around lines 1332 - 1335, Replace the
message-text check in the initialization rejection handling with a typed
wire-protocol rejection/error discriminant, using the enum value for illegal
decks to create DECK_REJECTED and preserving actionRejectionError for other
typed values. Update the associated test to send the typed discriminant instead
of relying on “Deck not legal” text, and follow the protocol’s typed-enum
convention throughout.
Source: Path instructions
* Fix shell art and fullscreen * Fix shell review findings --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Allows Scryfall card art in the Tauri CSP and uses Tauri's native window fullscreen API.
Summary by CodeRabbit
New Features
Bug Fixes
Accessibility