From 0cd50eecdb28046dd3b7e6a4896bd9468c46f832 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:21:58 -0700 Subject: [PATCH] fix(pet): key sprite keyframes on the resolved track; cap render durations (#9140) Follow-up polish to #8730: key @keyframes on the resolved row/frames (so a same-row fallback like hover on a jumping-less pet doesn't restart idle), mirror the importer's 60s per-frame cap at render, and trim comments to the 1-2 line convention. --- .../PetOverlay.pointer-interaction.test.tsx | 32 ++++++++++++++----- .../src/components/pet/PetOverlay.tsx | 14 ++++---- .../src/components/pet/pet-agent-state.ts | 6 ++-- .../pet/sprite-animation-css.test.ts | 3 ++ .../components/pet/sprite-animation-css.ts | 18 ++++++----- .../pet/usePetPointerInteraction.ts | 6 ++-- 6 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/renderer/src/components/pet/PetOverlay.pointer-interaction.test.tsx b/src/renderer/src/components/pet/PetOverlay.pointer-interaction.test.tsx index 17fcdeb2c..5533b5f90 100644 --- a/src/renderer/src/components/pet/PetOverlay.pointer-interaction.test.tsx +++ b/src/renderer/src/components/pet/PetOverlay.pointer-interaction.test.tsx @@ -75,8 +75,8 @@ function spriteDiv(container: HTMLElement): HTMLDivElement { return div } -// The @keyframes name is `pet---`: the -// animation name keeps a switched-to row from reusing the prior timeline, and +// The @keyframes name is `pet----`: the +// resolved track keeps a switched-to row from reusing the prior timeline, and // the generation suffix restarts a same-row grab from frame 0. function animationName(container: HTMLElement): string { return spriteDiv(container).style.animation.split(' ')[0] @@ -118,7 +118,6 @@ describe('PetOverlay grab-and-hold pointer interaction', () => { // it renders as step-end rather than steps()). expect(spriteDiv(container).style.animationPlayState).toBe('running') expect(spriteDiv(container).style.animation).toContain('step-end') - expect(animationName(container)).toContain('idle') const idleName = animationName(container) // Grab and hold still: mint a fresh restart (frame 0) and freeze there while @@ -139,18 +138,35 @@ describe('PetOverlay grab-and-hold pointer interaction', () => { // Drag right past the 4px deadzone: switch to the running-right row (row 1, // 8 frames, no durations → steps(8)) and resume animating. The keyframes - // name changes with the row, so it starts from frame 0 rather than reusing - // the idle timeline. + // name changes with the resolved row, so it starts from frame 0 rather than + // reusing the idle timeline. firePointer(wrapper, 'pointermove', 70, 80) expect(spriteDiv(container).style.animationPlayState).toBe('running') expect(spriteDiv(container).style.animation).toContain('steps(8)') - expect(animationName(container)).toContain('running-right') expect(animationName(container)).not.toBe(heldName) - // Release restores the live agent state (idle) and resumes animating. + // Release returns to the idle row (step-end) and resumes animating. Same row + // and generation as the grab-hold, so it's the held-idle identity again. firePointer(wrapper, 'pointerup', 70, 80) expect(spriteDiv(container).style.animationPlayState).toBe('running') expect(spriteDiv(container).style.animation).toContain('step-end') - expect(animationName(container)).toContain('idle') + expect(animationName(container)).toBe(heldName) + }) + + it('does not restart the idle track when hover falls back to the same row', () => { + ;({ container, root } = renderPetOverlay()) + const wrapper = container.querySelector('.pointer-events-auto') + if (!wrapper) { + throw new Error('draggable wrapper not found') + } + // The mocked sprite has no jumping row, so hover (→ jumping) resolves back to + // idle. Keying the keyframes on the resolved row means the name must not + // change — otherwise the unchanged idle animation restarts on hover. React + // synthesizes onPointerEnter/Leave from pointerover/pointerout. + const idleName = animationName(container) + firePointer(wrapper, 'pointerover', 0, 0) + expect(animationName(container)).toBe(idleName) + firePointer(wrapper, 'pointerout', 0, 0) + expect(animationName(container)).toBe(idleName) }) }) diff --git a/src/renderer/src/components/pet/PetOverlay.tsx b/src/renderer/src/components/pet/PetOverlay.tsx index a4b13c1d6..052ff6475 100644 --- a/src/renderer/src/components/pet/PetOverlay.tsx +++ b/src/renderer/src/components/pet/PetOverlay.tsx @@ -61,9 +61,7 @@ function SpriteFrame({ // that restarts from frame 0 even when the state row is unchanged. restartKey: number }): React.JSX.Element { - // Why: name the @keyframes per animation (+restartKey for same-row grabs) so a - // switched-to row starts at frame 0 instead of inheriting the prior timeline. - const animKeyframesId = `${useId().replace(/[^a-zA-Z0-9_-]/g, '')}-${animationName}-${restartKey}` + const baseId = useId().replace(/[^a-zA-Z0-9_-]/g, '') const anim = sprite.animations?.[animationName] || (sprite.defaultAnimation && sprite.animations?.[sprite.defaultAnimation]) || @@ -72,6 +70,11 @@ function SpriteFrame({ // Why: clamp to >=1 so an empty/invalid manifest can't produce steps(0), // which is rejected as invalid CSS and freezes the animation. const frames = Math.max(1, anim?.frames ?? sprite.columns ?? 1) + // Why: name the @keyframes by the RESOLVED track (+restartKey for same-row + // grabs), so a genuine row change starts at frame 0 while a state that falls + // back to the same row (e.g. hover on a pet without a jumping row) doesn't + // needlessly restart. + const animKeyframesId = `${baseId}-${row}-${frames}-${restartKey}` // Why: allow fractional downscaling so frames larger than maxSize shrink to // fit instead of overflowing the overlay; mirrors DetectedSpriteFrame's math. const scale = Math.min(maxSize / sprite.frameWidth, maxSize / sprite.frameHeight) @@ -406,9 +409,8 @@ export function PetOverlay(): React.JSX.Element { > {sprite ? ( - // Why: remount per pet so switching cached sprites can't inherit the - // previous pet's animation timeline (same @keyframes name → carried - // currentTime); each pet starts clean. + // Why: remount per pet so a switched-to sprite starts a fresh + // animation instead of inheriting the prior pet's currentTime. =4px horizontal move, so sub-threshold jitter -// and vertical drags keep the last direction without resetting the baseline — -// otherwise a slow diagonal drag would reset before ever crossing 4px. +// Why: direction tracks horizontal travel only; `accepted` (advance the baseline) +// fires only on a >=4px horizontal move so slow diagonal drags still accumulate. export function nextPetDragAnimation( current: PetDragAnimation, deltaX: number diff --git a/src/renderer/src/components/pet/sprite-animation-css.test.ts b/src/renderer/src/components/pet/sprite-animation-css.test.ts index 43f72c4af..cf694e66b 100644 --- a/src/renderer/src/components/pet/sprite-animation-css.test.ts +++ b/src/renderer/src/components/pet/sprite-animation-css.test.ts @@ -31,6 +31,9 @@ describe('buildSpriteAnimationCss', () => { [100, 100], [100, -1, 100], [100, Number.NaN, 100], + // Above the importer's 60s cap: corrupt/hand-edited persisted holds must + // not freeze the overlay. + [100, 70_000, 100], // Untrusted persisted data: non-arrays with a matching `length` must not // reach .every and throw. { length: 3 } as unknown as number[], diff --git a/src/renderer/src/components/pet/sprite-animation-css.ts b/src/renderer/src/components/pet/sprite-animation-css.ts index f04a8ecad..0970acaa7 100644 --- a/src/renderer/src/components/pet/sprite-animation-css.ts +++ b/src/renderer/src/components/pet/sprite-animation-css.ts @@ -1,6 +1,10 @@ // Sprite-sheet keyframe CSS for the pet overlay. Kept as a pure module so the // pacing math is unit-testable without mounting the overlay or a DOM. +// Mirror of the importer's per-frame cap (pet.ts zod schema): a hold longer than +// this at render would freeze the overlay, so reject it on the render side too. +const MAX_FRAME_DURATION_MS = 60_000 + export type SpriteAnimationCss = { keyframesCss: string animationCss: string @@ -57,13 +61,13 @@ function validFrameDurations( frameDurationsMs: number[] | undefined, frames: number ): number[] | null { - // Why: Array.isArray, not a truthiness check — persisted/RPC-synced sprites - // are untrusted, so a corrupt non-array value (e.g. { length: 6 }) must fail - // here rather than throw on .length/.every during render. + // Why: Array.isArray + bounds, not a truthiness check — persisted/RPC-synced + // sprites are untrusted, so a corrupt non-array or out-of-range hold falls back + // to uniform pacing instead of throwing or freezing the overlay. if ( Array.isArray(frameDurationsMs) && frameDurationsMs.length === frames && - frameDurationsMs.every((ms) => Number.isFinite(ms) && ms > 0) + frameDurationsMs.every((ms) => Number.isFinite(ms) && ms > 0 && ms <= MAX_FRAME_DURATION_MS) ) { return frameDurationsMs } @@ -71,10 +75,8 @@ function validFrameDurations( } // Cumulative step-end stops, one per frame. Returns null (→ uniform fallback) -// when a frame is too short to survive our 4-decimal precision: either two -// stops collapse to the same percentage, or the final stop rounds to 100% and -// so has no interval before the loop. Either way the frame would vanish, so we -// degrade to uniform pacing rather than silently drop it. +// when a frame is too short to survive 4-decimal precision (two stops collapse, +// or the final stop rounds to 100%) so no frame is silently dropped. function stepEndStops( durations: number[], totalMs: number, diff --git a/src/renderer/src/components/pet/usePetPointerInteraction.ts b/src/renderer/src/components/pet/usePetPointerInteraction.ts index 9a3afd29d..92d3a45c6 100644 --- a/src/renderer/src/components/pet/usePetPointerInteraction.ts +++ b/src/renderer/src/components/pet/usePetPointerInteraction.ts @@ -35,10 +35,8 @@ export function usePetPointerInteraction( // Why: horizontal baseline for the drag-direction hysteresis, advanced only on // an accepted direction. Kept separate from dragOffsetRef (position math). const dragBaselineXRef = useRef(0) - // Why: direction and the owning pointer are read+written inside pointer - // handlers, so keep them in refs immune to React's render batching. Reading - // the direction from state would let two coalesced moves in one commit - // resurrect a stale direction; `dragAnimation` state exists only to render. + // Why: read+written inside handlers, so keep in refs immune to render batching + // (state would let two coalesced moves resurrect a stale direction). const dragDirectionRef = useRef(null) const activePointerRef = useRef(null)