Fix card hover preview interactions - #7043
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change adds runtime preview placement state, detects side-docked modal containers, and updates preview rendering. Deck-builder hover callbacks now pass structured card metadata. Draft cards support touch long-press previews without triggering card addition. ChangesCard preview placement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ModalContainer
participant useInspectHoverProps
participant uiStore
participant GameCardPreview
ModalContainer->>useInspectHoverProps: identify side-docked preview context
useInspectHoverProps->>uiStore: inspectObject(card, timing, "side")
uiStore->>GameCardPreview: publish previewPlacement
GameCardPreview->>GameCardPreview: render side-docked preview
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
2474cf9 to
50efcee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/components/deck-builder/CommanderPanel.tsx (1)
96-210: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the cached Scryfall ID for commander previews.
These name-only payloads discard the printing identity available in
cardDataCache. The preview can then resolve a different printing than the deck-builder card record.
client/src/components/deck-builder/CommanderPanel.tsx#L96-L210: Build each hover payload withnameandcardDataCache.get(name)?.id.client/src/components/deck-builder/DeckList.tsx#L256-L257: Build the commander hover payload withnameandcardDataCache.get(name)?.id.As per path instructions, “propagate complete printing-aware hover data consistently through all deck-builder components.”
🤖 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/deck-builder/CommanderPanel.tsx` around lines 96 - 210, Preserve printing-aware preview data by updating every mouseHoverPreview call in client/src/components/deck-builder/CommanderPanel.tsx lines 96-210 to pass name and cardDataCache.get(name)?.id, including commander, signature spell, and companion entries. Apply the same payload change to the commander hover preview in client/src/components/deck-builder/DeckList.tsx lines 256-257, using the existing cardDataCache.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/deck-builder/hoverPreview.ts`:
- Around line 2-4: Update the CommanderPanel.tsx and DeckList.tsx callers of
mouseHoverPreview to build CardHoverInfo from the complete underlying deck/card
record, preserving scryfallId and sourcePrinting when available instead of
passing only name. Keep deck-builder hover data complete and printing-aware, and
do not rely on optional CardHoverInfo fields to mask missing metadata.
In `@client/src/components/draft/LimitedDeckBuilder.tsx`:
- Around line 61-68: Update the useLongPress implementation and the handleClick
path in LimitedDeckBuilder so firedRef is cleared when the pointer gesture ends
or is canceled, while retaining one-shot suppression for the synthetic click
generated by a completed long press. Add a regression test covering cancellation
without a click followed by keyboard activation, ensuring the keyboard
activation invokes onClick.
In `@client/src/hooks/useInspectHoverProps.ts`:
- Around line 77-83: Update the onPointerDown handler in useInspectHoverProps to
reject non-primary pointers and non-appropriate buttons before assigning
pressedIdRef, pressedPlacementRef, or calling armLongPress, matching the guards
used by useLongPress. Preserve the active first touch so a secondary touch
cannot retarget the armed preview, and add a colocated regression test covering
two simultaneous touches.
---
Outside diff comments:
In `@client/src/components/deck-builder/CommanderPanel.tsx`:
- Around line 96-210: Preserve printing-aware preview data by updating every
mouseHoverPreview call in client/src/components/deck-builder/CommanderPanel.tsx
lines 96-210 to pass name and cardDataCache.get(name)?.id, including commander,
signature spell, and companion entries. Apply the same payload change to the
commander hover preview in client/src/components/deck-builder/DeckList.tsx lines
256-257, using the existing cardDataCache.
🪄 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: d8b20299-5862-424b-bf60-d0731dac32cd
📒 Files selected for processing (21)
client/src/components/card/GameCardPreview.tsxclient/src/components/card/__tests__/GameCardPreview.test.tsxclient/src/components/deck-builder/CardEntryRow.tsxclient/src/components/deck-builder/CardGrid.tsxclient/src/components/deck-builder/CommanderPanel.tsxclient/src/components/deck-builder/DeckBuilder.tsxclient/src/components/deck-builder/DeckList.tsxclient/src/components/deck-builder/DeckStack.tsxclient/src/components/deck-builder/MoveList.tsxclient/src/components/deck-builder/PrintingPickerModal.tsxclient/src/components/deck-builder/__tests__/CardEntryRow.test.tsxclient/src/components/deck-builder/hoverPreview.tsclient/src/components/draft/LimitedDeckBuilder.tsxclient/src/components/draft/__tests__/LimitedDeckBuilder.test.tsxclient/src/components/modal/ChoiceOverlay.tsxclient/src/components/modal/DialogShell.tsxclient/src/components/modal/DistributeAmongModal.tsxclient/src/hooks/__tests__/useInspectHoverProps.test.tsxclient/src/hooks/useInspectHoverProps.tsclient/src/pages/DeckBuilderPage.tsxclient/src/stores/uiStore.ts
| import type { CardHoverInfo } from "../card/CardPreview"; | ||
|
|
||
| export type CardHoverHandler = (card: CardHoverInfo | null) => void; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve complete hover metadata at every caller.
CardHoverInfo still allows scryfallId and sourcePrinting to be omitted. The provided CommanderPanel.tsx and DeckList.tsx callers pass { name }, so they discard printing metadata when it is available from the underlying card record. Those previews cannot remain printing-aware.
Update the callers to construct CardHoverInfo from the complete deck or card record before calling mouseHoverPreview. Do not use the optional fields to hide a missing cross-component contract.
As per path instructions, deck-builder hover data must remain complete and printing-aware.
Also applies to: 26-32
🤖 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/deck-builder/hoverPreview.ts` around lines 2 - 4,
Update the CommanderPanel.tsx and DeckList.tsx callers of mouseHoverPreview to
build CardHoverInfo from the complete underlying deck/card record, preserving
scryfallId and sourcePrinting when available instead of passing only name. Keep
deck-builder hover data complete and printing-aware, and do not rely on optional
CardHoverInfo fields to mask missing metadata.
Source: Path instructions
| const { handlers, firedRef } = useLongPress(() => onHover(hoverInfo)); | ||
|
|
||
| const handleClick = () => { | ||
| if (firedRef.current) { | ||
| firedRef.current = false; | ||
| return; | ||
| } | ||
| onClick(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope firedRef to the completed pointer gesture.
useLongPress leaves firedRef.current set after the timer fires. Its pointer-cancel and pointer-up paths do not reset this flag. If no synthetic click follows, this handler suppresses the next keyboard or programmatic activation because no new pointerdown resets the flag. Make the suppression one-shot and expire it when the gesture is canceled. Add a regression test for cancellation without a click followed by keyboard activation.
🤖 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/draft/LimitedDeckBuilder.tsx` around lines 61 - 68,
Update the useLongPress implementation and the handleClick path in
LimitedDeckBuilder so firedRef is cleared when the pointer gesture ends or is
canceled, while retaining one-shot suppression for the synthetic click generated
by a completed long press. Add a regression test covering cancellation without a
click followed by keyboard activation, ensuring the keyboard activation invokes
onClick.
1 participant
Fixes modal card-preview placement and incorporates the outstanding hover-preview review feedback from the now-merged #7040.
Validation: formatting, Clippy, parser gates, 619 Oracle-parser tests, and 2,020 phase-AI tests passed. Card-data generation could not run because data/mtgjson/AtomicCards.json is absent locally.
Summary by CodeRabbit