Improve desktop hand fan and hover previews#6239
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes frontend memory usage and rendering performance by routing display-only card queries through the shared WasmAdapter worker instead of duplicating the WASM module on the main thread. It also unifies hand fan layouts across viewports using a shared geometry module, introduces viewport locking to prevent iOS rubber-band scrolling, and refactors hover preview latency to avoid spurious clears. The review feedback suggests improving the robustness of the card art fallback check by using loose equality for oracleText, and preventing potential test pollution by ensuring module-scoped timers are cleared in test suites via clearInspect().
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.
|
Deferred by maintainer intake policy — not ignored. This current head ( A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change. |
matthewevans
added
the
defer-fe
|
Maintainer value/scope review for current head
No workflow/instruction paths are in the contributor diff. The current checks are green. A maintainer rebase onto current |
matthewevans
removed
the
defer-fe
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer value/scope review for current head 59f0644fa0c6cdac1964e56e1fea58043dc72f4a:
- The shared-worker card queries remove the duplicate main-thread WASM/card corpus at the correct adapter boundary; the worker remains the single engine authority.
- The hand fan/preview work centralizes geometry and presentation rather than duplicating surface-specific calculations, with targeted boundary and interaction tests.
- This is broad client work, but the touched seams form one coherent card-viewing interaction path (memory, preview, fan layout, and viewport behavior). The older #4407 remains deferred as a narrow alternate fix for the same issue; it is not being enqueued alongside this comprehensive implementation.
No workflow/instruction paths are in the contributor diff. The current checks are green. A maintainer rebase onto current main completed cleanly locally, but GitHub rejected pushing the rewritten fork branch because the available OAuth token lacks workflow scope; the merge queue will perform the authoritative speculative rebase.
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughChangesThe pull request updates desktop card previews with hand-origin anchoring, motion transitions, configurable footer content, and hover-latency handling. It also introduces shared responsive hand-fan geometry, applies viewport locking and safe-area styling to game pages, and adds related settings, localization, markup, and tests. Card preview lifecycle and settings
Responsive hand-fan presentation
Game viewport and UI surface updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GameCardPreview
participant CardPreview
participant HandCard
participant CardImagePreview
GameCardPreview->>CardPreview: pass handSourceObjectId for hand objects
CardPreview->>HandCard: measure source bounding rectangle
CardPreview->>CardImagePreview: render bottom-anchored compact preview
CardPreview->>CardImagePreview: apply footer preference and motion state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
client/src/components/card/__tests__/CardPreview.test.tsx (1)
101-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuarantee the manually-appended hand-source node is removed even on assertion failure.
Both tests append a
sourceelement withdata-object-id="101"todocument.bodyand onlysource.remove()on the success path.cleanup()/theafterEachat Line 54-55 unmount React and reset prefs but don't remove these hand-crafted nodes. If an assertion throws, the stale (hovered) node survives into the next test, where the[data-hand-card][data-object-id="101"]query can resolve it and flip the anchored-vs-normal outcome — order-dependent pollution. Remove appended nodes inafterEach(ortry/finally).🤖 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/card/__tests__/CardPreview.test.tsx` around lines 101 - 135, Ensure the manually appended hand-source nodes created by the affected CardPreview tests are removed even when assertions fail. Update the shared afterEach cleanup to remove these nodes, or wrap each test’s append-and-assert flow in try/finally, while preserving the existing React cleanup and preference reset behavior.client/src/components/hand/handFanPresentation.ts (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the 0.78 / 1.4 card-scale ratios instead of duplicating them as literals. The opponent-card scale (0.78) and hand-card height scale (1.4) are hardcoded independently in two files; if either changes, the other silently falls out of sync and the mirrored opponent fan's vertical depth stops matching the actual rendered card size.
client/src/components/hand/handFanPresentation.ts#L12-L14: exportHAND_CARD_HEIGHT_SCALE = 1.4andOPPONENT_CARD_SCALE = 0.78as named constants, and deriveOPPONENT_HAND_VERTICAL_SCALE = OPPONENT_CARD_SCALE / HAND_CARD_HEIGHT_SCALEfrom them (also reuseHAND_CARD_HEIGHT_SCALEinplayerHandFanSizingStyle's* 1.4).client/src/components/hand/OpponentHand.tsx#L69-L71: import and useOPPONENT_CARD_SCALEin the--opponent-hand-card-w/-hcalc strings instead of the literal0.78.🤖 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/hand/handFanPresentation.ts` around lines 12 - 14, Centralize the card-scale ratios in handFanPresentation.ts by exporting HAND_CARD_HEIGHT_SCALE as 1.4 and OPPONENT_CARD_SCALE as 0.78, deriving OPPONENT_HAND_VERTICAL_SCALE from them, and reusing HAND_CARD_HEIGHT_SCALE in playerHandFanSizingStyle instead of the literal 1.4. In OpponentHand.tsx, import OPPONENT_CARD_SCALE and use it in both opponent card width and height calculations instead of 0.78.
🤖 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/hooks/useGameViewportLock.ts`:
- Around line 16-24: Update useGameViewportLock to track shared ownership of
GAME_VIEWPORT_LOCK_CLASS across overlapping hook instances using a reference
count or ownership tokens for both root and body; only remove each class when
its final owner unmounts. Add a regression test covering overlapping mounts and
unmounting the first instance while the second remains mounted, ensuring the
lock persists until the last unmount.
In `@client/src/stores/__tests__/preferencesStore.test.ts`:
- Line 18: Update the preferences store tests around buildDefaultPreferences and
the v27→v28 migration so the default showCardPreviewFooter assertion exercises a
fresh initialization or rehydration path without the shared reset forcing it to
true. Keep the shared reset for test isolation, but remove or bypass its
explicit showCardPreviewFooter override in the default-focused test.
---
Nitpick comments:
In `@client/src/components/card/__tests__/CardPreview.test.tsx`:
- Around line 101-135: Ensure the manually appended hand-source nodes created by
the affected CardPreview tests are removed even when assertions fail. Update the
shared afterEach cleanup to remove these nodes, or wrap each test’s
append-and-assert flow in try/finally, while preserving the existing React
cleanup and preference reset behavior.
In `@client/src/components/hand/handFanPresentation.ts`:
- Around line 12-14: Centralize the card-scale ratios in handFanPresentation.ts
by exporting HAND_CARD_HEIGHT_SCALE as 1.4 and OPPONENT_CARD_SCALE as 0.78,
deriving OPPONENT_HAND_VERTICAL_SCALE from them, and reusing
HAND_CARD_HEIGHT_SCALE in playerHandFanSizingStyle instead of the literal 1.4.
In OpponentHand.tsx, import OPPONENT_CARD_SCALE and use it in both opponent card
width and height calculations instead of 0.78.
🪄 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: 28a96eb1-38c6-462c-9117-b895fc0b0a2d
📒 Files selected for processing (34)
client/index.htmlclient/src/components/board/AttachmentFan.tsxclient/src/components/card/CardImage.tsxclient/src/components/card/CardPreview.tsxclient/src/components/card/GameCardPreview.tsxclient/src/components/card/__tests__/CardPreview.test.tsxclient/src/components/card/__tests__/fanGeometry.test.tsclient/src/components/card/fanGeometry.tsclient/src/components/chrome/GameMenu.tsxclient/src/components/chrome/__tests__/GameMenu.test.tsxclient/src/components/hand/CompanionFanCard.tsxclient/src/components/hand/OpponentHand.tsxclient/src/components/hand/PlayerHand.tsxclient/src/components/hand/__tests__/OpponentHand.test.tsxclient/src/components/hand/__tests__/PlayerHand.test.tsclient/src/components/hand/handFanPresentation.tsclient/src/components/mana/ManaCostPips.tsxclient/src/components/settings/PreferencesModal.tsxclient/src/components/settings/__tests__/PreferencesModal.cardPreview.test.tsxclient/src/hooks/__tests__/useGameViewportLock.test.tsxclient/src/hooks/useGameViewportLock.tsclient/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/index.cssclient/src/pages/GamePage.tsxclient/src/stores/__tests__/preferencesStore.test.tsclient/src/stores/__tests__/uiStoreHoverLatency.test.tsclient/src/stores/preferencesStore.tsclient/src/stores/uiStore.ts
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the focused client-side work. I found three merge-blocking gaps on the current head 374316300d24191fd5d20dcfa03dce9501e2e717:
-
useGameViewportLockdoes not preserve the lock for overlapping hook owners. Each mount snapshots whether the class existed before it mounted; with two hook instances, both snapshots arefalse, so unmounting either instance removes the root and body classes while the other game viewport is still mounted. Use shared ownership/reference counting (including pre-existing external ownership) and add an overlapping-mount regression test. -
The preview-footer default assertion is masked by the shared test reset. The reset sets
showCardPreviewFooter: truebefore the test, so the assertion does not provebuildDefaultPreferences()or the v27-to-v28 migration supplies the default. Exercise a fresh initialization or rehydration path without that override; keep the reset for isolation. -
The new hand-origin preview behavior is tested by rendering
CardPreviewwith a hand-source id directly. Production computes and passes that id throughGameCardPreview/PlayerHand; add an integration-level test that drives that production wiring so a dropped prop or wrong source selection fails.
The current head also remains behind main, and Rust lint plus shard 2 are still pending. Please address the three gaps and update against current main; then this can be reviewed again.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: the current head resolves the prior blocking findings and passed independent implementation review; the merge queue will gate on remaining required checks.
2 participants
Summary
Resolution scope
The new fanned-hand hover and source-card preview animation is intentionally desktop-resolution/pointer-hover only. Mobile keeps the established interaction: tap the resting hand to open the full-hand drawer, then tap a playable card there to cast it. Mobile-specific changes in this PR are limited to responsive fan/pip sizing and viewport/menu stability.
Why
The former desktop fan was steep and tightly overlapped, making neighboring cards difficult to reach. Hover previews also appeared abruptly, could disappear when a configured hover delay raced an animated layout shift, and lacked a clear visual separation from the battlefield.
Separately, iOS could exceed its WebContent memory budget when card inspection initialized a second WASM runtime/card database during game startup. Routing display queries through the resident game worker removes that duplicate allocation.
Demo
hand-fan-hover-720.mov
Validation
npx vitest run— 2,133 tests passed across 248 filesnpx tsc -b --noEmitnpx eslint src— no errors (18 existing warnings outside the changed files)Closes #4377
Summary by CodeRabbit