feat(ligretto): unify card interactions and drag and drop - #686
feat(ligretto): unify card interactions and drag and drop#686memebattle-dev wants to merge 18 commits into
Conversation
StepanDyubin
left a comment
There was a problem hiding this comment.
@memebattle-dev fix
|
Fixed the onboarding E2E regressions in dc0b9f7 and 2ef5127: playground interactions now target the deck drop surface, including empty decks, and the free-play scenario performs the required manual select-then-place sequence. Frontend typecheck and all 76 unit tests pass; repository formatting and lint checks pass. The complete Ligretto CI rerun is green, including all 9 Playwright tests across desktop and mobile projects. |
|
Implemented the component-owned interaction refinement in 4743ab1.
Validation: frontend typecheck and all 97 unit tests passed; build, formatter, lint and independent review passed. Local onboarding E2E could not reach the tutorial because the core backend at 127.0.0.1:3333 was unavailable ( |
| <CardPlace> | ||
| {stackOpenDeckCard && ( | ||
| <CardHotkeyBadge hotkey={Hotkey.x}> | ||
| <CardHotkeyBadge hotkey={inputEnabled ? Hotkey.x : undefined}> |
There was a problem hiding this comment.
hotkey should always be available, and cancel drag if it is in process
There was a problem hiding this comment.
Implemented in 7048d9b. Hotkeys stay available during both active and pending mouse/touch gestures. The native sensor is cancelled before the component-owned command runs; regression tests cover new focus surviving the abandoned release/click and a fresh subsequent drag.
There was a problem hiding this comment.
Following the latest follow-up, removed all custom drag cancellation in e104ef0. Standard MouseSensor/TouchSensor replace the adapters; forced Escape interception, sensor cancellation refs and release-click suppression are gone. Hotkeys remain available and no longer cancel or invalidate an ongoing drag: the original source can still drop normally. Only standard dnd-kit lifecycle synchronization remains. Frontend typecheck, 98 tests and build pass; the independent review found no blockers.
|
|
||
| // A native drag owns input until release/cancel; clicks and shortcuts must not | ||
| // replace its selection or resurrect a gesture invalidated by a card update. | ||
| export const useCardInputEnabled = () => { |
There was a problem hiding this comment.
Do not pass enabled to no components inside a provider. The provider should encapsulate all logic that disables interactions inside.
There was a problem hiding this comment.
Implemented in 7048d9b, including the provider-encapsulation follow-up. Removed useCardInputEnabled and consumer-level global enabled propagation. The provider now blocks disabled pointer/click input and hotkey commands and cancels pending/active native gestures; components retain only their own game-specific availability rules.
There was a problem hiding this comment.
Isn't that always hidden? Can we remove that prop, and card also?
There was a problem hiding this comment.
Implemented in 7048d9b. Interaction state is now a useReducer discriminated union with idle, focused and dragging states. Selection and drop validation use that state; imperative refs are limited to native sensor cancellation and abandoned-release cleanup.
| if ( | ||
| enabled && | ||
| activeTarget && | ||
| dragged?.target && | ||
| over?.id === id && | ||
| draggableNodes.get(active.id)?.node.current?.isConnected && | ||
| droppableContainers.get(id)?.node.current?.isConnected && | ||
| isSameCardInteractionTarget(activeTarget, dragged.target) |
There was a problem hiding this comment.
Why is it better than it was in previous version (passing handler through data)?
There was a problem hiding this comment.
Removed in 7048d9b. Pointer commands are direct component-owned callbacks again; useCardHotkey handles keyboard input and the provider handles cancellation/disablement. There is no useCardAction export or hook.
| } | ||
|
|
||
| export const CardInteractionProvider = ({ children, enabled }: CardInteractionProviderProps) => { | ||
| const [activeTarget, setActiveTarget] = useState<CardInteractionTarget>() |
There was a problem hiding this comment.
use reducer here (like a state-machine)
type CardInteractionState =
| { mode: 'idle' }
| {
mode: 'focused'
target: CardTarget
}
| {
mode: 'dragging'
target: CardTarget
card: Card // not sure if it is nessesary
}
There was a problem hiding this comment.
Implemented in 7048d9b. PlayerStackDeck now receives no props and selects its own stack data. Populated closed-stack cards remain face-down; the empty-stack placeholder and reshuffle availability are preserved and covered by owner tests.
| import { useCardInteractionContext } from './CardInteractionContext' | ||
| import { useCardInputEnabled } from './useCardInputEnabled' | ||
|
|
||
| export const useCardAction = (onActivate: () => void, available = true) => { |
There was a problem hiding this comment.
Restored in 7048d9b. useDroppableTarget puts onDrop in the droppable data again instead of subscribing each target with useDndMonitor. The provider routes a valid current drag to that callback; placement rules and commands remain in the owning component.
| const { clearFocus } = useCardFocus() | ||
| const { stackDeckCards, stackOpenDeckCard } = useSelector(playerCardsStackSelector) | ||
| const hasCards = !!stackDeckCards?.length | ||
| const available = hasCards || !!stackOpenDeckCard |
There was a problem hiding this comment.
Remove available. It should be always available if provider is avalable
There was a problem hiding this comment.
Addressed in a1554e3. Removed available and the conditional stack handlers. The empty-stack Space shortcut remains available whenever the provider is enabled; owner tests cover the shortcut and exact dispatch.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
| const enabledRef = useRef(enabled) | ||
| enabledRef.current = enabled |
There was a problem hiding this comment.
remove that ref. and runCommand
There was a problem hiding this comment.
Addressed in a1554e3. Removed enabledRef and runCommand. Hotkey clearing lives in useCardHotkey; provider state and internal hooks retain the enablement gate without consumer flags or replacement command machinery.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
|
|
||
| export const useCardHotkey = (hotkey: Hotkey | undefined, onActivate: () => void) => { | ||
| const { runCommand } = useCardInteractionContext() | ||
| const activate = useCallback(() => runCommand(onActivate), [onActivate, runCommand]) |
There was a problem hiding this comment.
useCardHotkey should not return activate
There was a problem hiding this comment.
Addressed in a1554e3. useCardHotkey now returns nothing. It clears selection before the keyboard callback, while clicks invoke owner callbacks directly and use document-driven dismissal. The public API and owner dispatch paths are covered by tests.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
| import type { CardDragData, CardDragTarget } from '../model/types' | ||
| import { getInteractionTargetKey, useCardInteractionContext } from './CardInteractionContext' | ||
|
|
||
| export const useDraggableCard = (target: CardDragTarget, card: Card, disabled = false) => { |
There was a problem hiding this comment.
if component is rendered and uses that hook, it should be enable. Remove disabled
There was a problem hiding this comment.
Addressed in a1554e3. Removed the disabled parameter and drag-data property. Mounted sources are available under an enabled provider; the hook reads global provider enablement internally. Native mouse/touch regressions cover disablement and re-enablement.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
| const captureInput = (event: React.SyntheticEvent) => { | ||
| if (!enabled) { | ||
| event.preventDefault() | ||
| event.stopPropagation() | ||
| } | ||
| } |
There was a problem hiding this comment.
Addressed in a1554e3. Removed captureInput and all capture handlers. The provider uses native inert for pointer/focus disablement, with internal hotkey/drag guards. A real Chromium check against PlayerStackDeck verifies enabled click/Space dispatch exactly once and disabled pointer/focus/Space dispatch nothing, including after re-disabling.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
| })) | ||
| import { playgroundDecksSelector } from '#ducks/game' | ||
|
|
||
| export const PlaygroundContainer = () => { |
There was a problem hiding this comment.
Remove Container, just use selectors inside Playground component
There was a problem hiding this comment.
Addressed in a1554e3. Deleted PlaygroundContainer; Playground selects its decks directly. Updated its export and GameContainer consumer, with rendered tests for row/open-stack placement and exact payloads.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
| [cardDeck, placeCard], | ||
| ) | ||
| const { id: dropId, isOver, setNodeRef } = useDroppableTarget({ type: 'playground', index: deckIndex }, handleDrop) | ||
| const isValid = !!activeCard && canPlaceCardOnDeck(activeCard, cardDeck) |
There was a problem hiding this comment.
return isValid from useDroppableTarget.
move selectors call and check inside the hook
There was a problem hiding this comment.
Addressed in a1554e3. useDroppableTarget now owns the card/deck selectors and returns isValid. It also validates the live source and destination before delivering the callback; PlaygroundDeck retains only command dispatch and visuals. Tests cover changing source/deck data and drag identity after hotkey focus changes.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
| isDimmed: boolean | ||
| toggleActiveTarget: () => void | ||
| } | ||
| export function useCardInteraction(target?: CardInteractionTarget, deps: DependencyList = []) { |
There was a problem hiding this comment.
Do not pass deps.
call selector inside and use values to clear effect
There was a problem hiding this comment.
create cardByInteractionTarget selector inside the feature
There was a problem hiding this comment.
Addressed in a1554e3. Removed caller-provided dependencies and added the feature-owned cardByInteractionTarget selector. The hook clears on selected card color/value changes and unmount, not object reallocation. The overlay reuses the selector; onboarding supplies its isolated tutorial selector at the provider boundary. Selection replacement and tutorial isolation regressions pass.
Validation: 105 frontend tests, frontend typecheck/build, repository unit tests (234), formatting and lint pass; independent review found no blockers.
Themezv
left a comment
There was a problem hiding this comment.
Review of the card interaction rewrite. The first comment is a confirmed bug with concrete proposals. The other four are findings I would like the author to verify against the current head before fixing, since the branch moved during the review (the highlight desync, the input-swallowing wrapper, the hotkey enablement and the duplicated target-to-card selector were already fixed in a1554e3).
| const [state, dispatch] = useReducer(reducer, { mode: 'idle' }) | ||
| const sensors = useSensors( | ||
| useSensor(MouseSensor, { activationConstraint: { distance: 6 } }), | ||
| useSensor(TouchSensor, { activationConstraint: { delay: 150, tolerance: 8 } }), |
There was a problem hiding this comment.
Bug: a touch held for 150 ms without moving becomes a drag that ends nowhere, and the tap is lost.
With a delay constraint dnd-kit's AbstractPointerSensor.attach() schedules handleStart after 150 ms regardless of movement (the touchDrag helper in dnd.spec.tsx relies on exactly this). So on a phone a deliberate tap of ~150-300 ms:
- activates the drag:
onDragStart→dragStartreplaces anyfocusedstate withdragging; - the finger lifts at the same spot,
pointerWithingivesover === null, noonDrop,dragTerminal→idle; - dnd-kit installs a capture-phase document
clicklistener withstopPropagationon activation and keeps it for 50 ms after release, so the card'sonClicktoggle never runs.
Net effect: the card is not selected, and any previous selection is silently cleared. 150 ms is well inside a normal tap; dnd-kit's own docs suggest ~250 ms for touch.
Proposals, in order of preference:
- Use a distance constraint instead of a delay:
useSensor(TouchSensor, { activationConstraint: { distance: 8 } }). A still finger then never activates a drag, the click goes through normally and the click suppression never kicks in. There is no scroll conflict because the cards already settouchAction: 'none'. This is a one-line change. - If the delay must stay: treat a drag that ends with
over === nulland a displacement belowtoleranceas a tap.onDragEndreceivesdelta; dispatchtoggleinstead ofdragTerminalin that case. This needs thedraggingstate to remember the previousStateso a tap on an already-selected card deselects it, and it keeps the 150 ms latency before a real drag starts. - At minimum, raise the delay to ~250 ms so fewer taps fall into the window.
I would also add a unit case: touch start, wait past the delay, touch end without moving → activeTarget toggles.
There was a problem hiding this comment.
Fixed in 3b5d4f4: touch dragging now requires movement beyond 8 px, so a stationary 300 ms touch reaches normal click selection. Added select/deselect regressions and updated the native touch-drag helper. All 110 frontend tests and 239 repository tests pass; frontend typecheck/build, formatting and lint pass.
| data-drop-over={isOver || undefined} | ||
| onClick={() => { | ||
| if (activeTarget?.type === 'row' || activeTarget?.type === 'open-stack') { | ||
| placeCard(activeTarget) |
There was a problem hiding this comment.
Please verify before fixing: the click path neither validates nor clears, so a double-tap can place a second, unchosen card.
The drop path now prevalidates in useDroppableTarget.onDrop, but this click handler dispatches placeCard(activeTarget) unconditionally, and the selection survives the click because TableCards carries data-card-interaction-element; it is only cleared when the card identity changes after the server echo (there is no optimistic reducer).
Scenario: open-stack card selected, double-tap on a pile → two putCardFromStackOpenDeck before updateGameAction arrives. Backend playerPutFromStackOpenDeck reads last(deck.cards) and pop()s, so the second request evaluates the next open-stack card and places it whenever it happens to be the same colour and value+1. Row cards are safe only because removeCard nulls the slot. Every misclick on an invalid pile is also a round trip plus a room-wide broadcast for a no-op.
The README says click selection is intentionally kept until the server confirms, and the base PlaygroundContainer had the same shape, so this is carried over rather than new. Suggested minimum: run the same canPlaceCardOnDeck check the drop path uses (the isValid returned by useDroppableTarget is already computed here), and either clear the selection on dispatch or ignore a second click while a put for that target is in flight.
There was a problem hiding this comment.
Verified: two pile clicks before the server echo dispatch two identical open-stack placement commands. 3b5d4f4 adds the isValid guard for invalid piles. Earlier review feedback #686 (comment) assigned click clearing to the document. Should successful placement now clear selection immediately as an explicit exception, or retain focus with an in-flight guard? The double-click case remains unfixed pending that choice.
| disableRipple={isDisabled} | ||
| size={size} | ||
| onMouseDown={onClick} | ||
| onClick={onClick} |
There was a problem hiding this comment.
Please verify before fixing: onMouseDown → onClick is an unacknowledged trade-off, and with the 6 px MouseSensor constraint a jittery press is swallowed.
onMouseDown={onClick} was introduced deliberately in 8c6e400 (LIG-144, "put card on mousedown instead of click") as a speed-game affordance. This PR flips every Card to onClick without mentioning it in the description or README.
For draggable cards the combination with activationConstraint: { distance: 6 } has a concrete failure: mousedown on a row card, drift ≥ 6 px, mouseup over the card itself → dnd-kit activates, over === null, onDragEnd only dispatches dragTerminal, and the 50 ms capture-phase click suppression prevents onCardActivate. The card is neither selected nor placed; on the base branch it toggled on mousedown regardless of drift. For non-draggable cards (stack deck, ligretto pack, onboarding) activation now waits for mouseup and press-drag-off-release no longer fires.
If the change is intended, please say so in the PR body and the README. If not, options are a larger distance, or activating on onMouseDown for non-draggable cards only.
There was a problem hiding this comment.
3b5d4f4 documents the current release-time activation, non-draggable latency and threshold-crossing cancelled placement in the README; the PR description is synchronized. LIG-144 deliberately used mousedown. Should we retain the current onClick trade-off, or restore mousedown activation for non-draggable cards? I left behavior unchanged rather than choose silently.
| hotkey ?? '', | ||
| event => { | ||
| event.preventDefault() | ||
| clearActiveTarget() |
There was a problem hiding this comment.
Please verify before fixing: there is no longer any keyboard way to deselect a card.
The base CardFocusProvider bound Escape to clear focus, and a repeated row hotkey toggled the selection off. Now the Escape handler is gone (Hotkey.escape in ducks/game/utils.ts is dead) and every hotkey does clearActiveTarget() then onActivate(); after the clear the reducer is idle, so toggle always ends in focused. Pressing q twice keeps row 0 selected (asserted in cardOwnedHotkeys.spec.tsx), and index.spec.tsx asserts Escape is a no-op.
So after q the board dims and the only side-effect-free exit is a mouse click outside a card element; Space and L clear only by dispatching a game command. Issue #685 asks to preserve keyboard accessibility.
Suggested: either restore an Escape → clearActiveTarget() binding in the provider (gated on enabled), or let a repeated hotkey on the already-active target toggle it off by skipping the clearActiveTarget() when onActivate is a toggle of the same target.
There was a problem hiding this comment.
Confirmed: focused Escape is a no-op and repeated row hotkeys retain selection. Earlier feedback required every hotkey to clear first (#686 (comment)), and the later follow-up removed custom Escape/drag cancellation (#686 (comment)). Should we restore enabled-only Escape deselection while leaving native drag cancellation unchanged? No keyboard behavior was changed in this follow-up.
| const data = useMemo<CardDragData>(() => ({ target, card }), [card, target]) | ||
| const { isDragging, listeners, setNodeRef } = useDraggable({ id, data, disabled: !enabled }) | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Please verify before fixing: this cleanup effect duplicates the one in useCardInteraction.
Both production callers (PlayerRowCard, PlayerStackOpenCard) call useCardInteraction(target) and useDraggableCard(target, card). useCardInteraction keys its effect on [clearActiveTarget, targetKey, card?.color, card?.value] and returns () => clearActiveTarget(target); this effect keys on id = \${targetKey}.${card.color}.${card.value}`(the same identity) and returns the same cleanup. Every slot refill or unmount therefore dispatches twoclearactions for one target, each behind its owneslint-disable`, and the next source owner has to remember to double-hook.
Suggested: let one hook own identity and cleanup. Either drop this effect and keep only the !enabled branch, or fold useDraggableCard into useCardInteraction so a card source is one hook.
There was a problem hiding this comment.
Fixed in 3b5d4f4: useCardInteraction is now the sole source identity/unmount selection-cleanup owner; useDraggableCard only owns native registration. Provider disablement already clears centrally. Regression tests observed two clear calls before the change and exactly one afterward, with native stale-source and disablement tests still green. All 110 frontend tests and 239 repository tests pass; frontend typecheck/build, formatting and lint pass.
Summary
cardInteraction, with anidle/focused/draggingreducer.inertand internal hook guards; remove capture handlers,enabledRefandrunCommand.useCardHotkeyclears selection before keyboard commands and returns nothing; clicks remain direct callbacks with document-owned dismissal.cardByInteractionTarget.useCardInteractionalone owns semantic card replacement/unmount selection cleanup;useDraggableCardowns native source registration. Keep onboarding isolated through its provider-level selector.Playgroundselects decks directly.useDroppableTargetowns live placement validation/isValid; drop callbacks travel through droppable data with current source/destination validation. Click placement now checksisValidbefore dispatch.PlayerStackDeckprop-free and available whenever its provider is available, including the empty-stack shortcut.Interaction trade-offs and unresolved review decisions
8c6e400b). This separates selection from native dragging but delays non-draggable stack, Ligretto and onboarding activation until release; press-drag-off-release no longer activates them. A mouse source dragged past 6 px and released back over itself does not select: native dnd-kit suppresses the follow-up click. README documents this trade-off; confirmation is requested before restoring press activation or changing thresholds.These unresolved behaviors are not claimed fixed. No merge is requested.
Validation
pnpm test:ci: 239 tests pass;pnpm fmt:check,pnpm lint:checkandgit diff --checkpass.apps/blog/.next/types/validator.tsTS2344 locale constraint errors at lines 81, 94 and 103 (en | ruversusstring). No new type errors or blog changes are included.PlayerStackDeckestablished enabled click/Space dispatch and native inert disablement. This incremental fix reran unit tests/build, not browser E2E; jsdom touch tests explicitly emit the browser's compatibility click.3b5d4f427d513118fb1b418c7eb0c207f9083a2f: all completed checks pass, including Ligretto E2E. Only Storybook remains waiting for protected-environment approval from the core team; the worker cannot approve it.Closes #685