Skip to content

fix(frontend): surface Glasses of Urza's private hand-look to the caster#5464

Open
rykerwilliams wants to merge 2 commits into
phase-rs:mainfrom
rykerwilliams:fix/5251-glasses-of-urza
Open

fix(frontend): surface Glasses of Urza's private hand-look to the caster#5464
rykerwilliams wants to merge 2 commits into
phase-rs:mainfrom
rykerwilliams:fix/5251-glasses-of-urza

Conversation

@rykerwilliams

Copy link
Copy Markdown
Contributor

Summary

Fixes #5251 — [[Glasses of Urza]] ({T}: Look at target player's hand. — verified via Scryfall) does not visibly reveal the target player's hand to the caster.

Root cause is entirely in the frontend, not the engine. The engine already parses and resolves this correctly:

  • parse_hand_reveal_ast (oracle_effect/imperative.rs) recognizes "look at target player's hand" and lowers to Effect::RevealHand { target: TargetFilter::Player, reveal: false, .. }.
  • reveal_hand::resolve (CR 701.20e) populates state.private_look_ids / state.private_look_player for the looking player, distinct from the public reveal: true path (Thoughtseize/Duress).
  • filter_state_for_viewer (game/visibility.rs) leaves those object ids un-redacted for the looker — this is already covered end-to-end by crates/engine/tests/integration/issue_3263_gitaxian_probe.rs (Gitaxian Probe has identical Oracle wording).

So the looking player's client does receive the real card names. The gap: OpponentHand.tsx's isRevealed check only consulted the public reveal sets (revealed_cards, public_revealed_cards), never private_look_ids/private_look_player, so the UI kept rendering card backs for a hand the engine had already unredacted client-side.

isLibraryCardRevealedToViewer (viewmodel/gameStateView.ts) already implements the identical CR 701.20e check for library peeks (Mishra's Bauble, scry) — its doc comment even says "This is the same pattern OpponentHand uses for opponent hand cards," which turned out not to be true. This PR extracts the private-look half into a shared, zone-agnostic isPrivatelyLookedAtByViewer (the engine's private_look_ids carries object ids regardless of which zone they're in) and wires it into OpponentHand's reveal check.

Changes

  • client/src/viewmodel/gameStateView.ts: extract isPrivatelyLookedAtByViewer, reused by isLibraryCardRevealedToViewer.
  • client/src/components/hand/OpponentHand.tsx: consult it in the card-face reveal check (CR 701.20e).
  • client/src/components/hand/__tests__/OpponentHand.test.tsx: two new tests — the looker sees the privately-looked-at card without showCards, and a different player does not.

Test plan

  • Verified Glasses of Urza's exact Oracle text via Scryfall (not the issue's paraphrase)
  • pnpm run type-check clean
  • eslint clean on both changed source files
  • New tests pass; confirmed discriminating via live-revert-and-rerun (fail without the fix, pass with it restored)
  • Confirmed engine-side resolution already correct via existing issue_3263_gitaxian_probe.rs integration test (same Oracle wording, "Look at target player's hand.")
  • Full frontend suite (pnpm test) was not clean in this environment — 69/230 files fail with ECONNREFUSED 127.0.0.1:3000 (a local mock/dev server this fresh worktree never started), entirely unrelated files (audio, cloud sync, lobby, deck builder, etc.). Neither changed file (OpponentHand.test.tsx, gameStateView.ts) appears anywhere in that failure list; the targeted test file passes cleanly in isolation.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K

Glasses of Urza ({T}: Look at target player's hand. — verified via
Scryfall) and Gitaxian Probe already parse and resolve correctly on the
engine side: Effect::RevealHand { reveal: false } populates
state.private_look_ids / private_look_player (CR 701.20e), and the
filtered GameState sent to the looking player already carries the real
card names for those ids.

The gap is entirely in the frontend. OpponentHand.tsx's isRevealed check
only consulted the public reveal sets (revealed_cards,
public_revealed_cards), never the private-look fields, so the looker's
own client kept rendering card backs for a hand it could already see.

isLibraryCardRevealedToViewer (gameStateView.ts) already implements the
identical CR 701.20e check for library peeks (Mishra's Bauble, scry) —
its own doc comment even claimed this was "the same pattern OpponentHand
uses for opponent hand cards," which wasn't true. Extracted the private-
look half into a shared, zone-agnostic isPrivatelyLookedAtByViewer
(private_look_ids carries object ids regardless of zone) and wired it
into OpponentHand's reveal check.

Added regression tests exercising both the positive case (the looker sees
the card without showCards) and the negative case (a different player
does not), verified via live-revert-and-rerun.

Fixes phase-rs#5251.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request implements support for showing opponent hand cards that have been privately looked at by the viewer (e.g., via Glasses of Urza or Gitaxian Probe) by introducing the isPrivatelyLookedAtByViewer helper in gameStateView.ts and integrating it into OpponentHand.tsx along with corresponding unit tests. The review feedback recommends hardening isPrivatelyLookedAtByViewer against null or undefined viewerId values to prevent potential information leaks or runtime type errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +124 to +134
export function isPrivatelyLookedAtByViewer(
gameState: GameState | null,
objectId: ObjectId,
viewerId: PlayerId,
): boolean {
if (!gameState) return false;
return (
gameState.private_look_player === viewerId &&
(gameState.private_look_ids?.includes(objectId) ?? false)
);
}

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.

security-medium medium

[MEDIUM] Potential state leak or type error when viewerId is null or undefined. Evidence: client/src/viewmodel/gameStateView.ts:124-134.

Why it matters: If viewerId is null/undefined (e.g., for spectators or during loading states) and gameState.private_look_player is also null/undefined, they will match and potentially leak private information or cause type-checking errors.

Suggested fix: Update viewerId type to PlayerId | null | undefined and add a viewerId == null guard.

Suggested change
export function isPrivatelyLookedAtByViewer(
gameState: GameState | null,
objectId: ObjectId,
viewerId: PlayerId,
): boolean {
if (!gameState) return false;
return (
gameState.private_look_player === viewerId &&
(gameState.private_look_ids?.includes(objectId) ?? false)
);
}
export function isPrivatelyLookedAtByViewer(
gameState: GameState | null,
objectId: ObjectId,
viewerId: PlayerId | null | undefined,
): boolean {
if (!gameState || viewerId == null) return false;
return (
gameState.private_look_player === viewerId &&
(gameState.private_look_ids?.includes(objectId) ?? false)
);
}

@matthewevans matthewevans added the defer-fe Frontend/client/UI PR deferred to Matt's direct review label Jul 9, 2026
…ewer

gemini-code-assist review on phase-rs#5464: if a caller's viewerId hasn't
resolved yet (mid-load) and gameState.private_look_player also happens
to be unset, the bare === would match undefined against undefined and
incorrectly report the look as visible. Widen viewerId's type and
short-circuit on null/undefined before the comparison.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
@rykerwilliams

Copy link
Copy Markdown
Contributor Author

Fixed in 22ba6ba. Widened viewerId to PlayerId | null | undefined and added a viewerId == null short-circuit before the private_look_player comparison, so a mid-load caller with an unresolved viewer can't accidentally match against an also-unset private_look_player. Confirmed the existing tests and type-check stay clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

defer-fe Frontend/client/UI PR deferred to Matt's direct review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Glasses of Urza — [[Glasses of Urza]] is not revealing target opponents hand.

2 participants