Conversation
When repeat >= 20 (including Infinity) was used in a sequence segment transition, it either threw an error in dev or caused an infinite loop in production. Instead of trying to expand keyframes for large repeat counts, pass repeat/repeatType/repeatDelay through to the final transition and let the animation engine handle repeating natively. Fixes #2915 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a crash ( Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolveValueSequence()"] --> B{repeat defined?}
B -- No --> G[Proceed to addKeyframes]
B -- Yes --> C{repeat >= MAX_REPEAT\ni.e. >= 20?}
C -- Yes --> D["Store in repeatPassthrough Map\n{ repeat, repeatType, repeatDelay }"]
D --> E["duration stays as base duration\n⚠️ NOT expanded for finite repeat >= 20"]
E --> G
C -- No --> F["calculateRepeatDuration()\nExpand keyframes inline\nExpand times & ease arrays\nnormalizeTimes()"]
F --> G
G["addKeyframes()\nupdate maxDuration / totalDuration"] --> H["Second pass: build final transition"]
H --> I["definition.transition[key] = {\n ...remainingDefaultTransition,\n duration: totalDuration,\n ease, times,\n ...sequenceTransition,\n ...repeatPassthrough.get(valueSequence) 👈 new\n}"]
Last reviewed commit: 137e874 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (repeat) { | ||
| invariant( | ||
| repeat < MAX_REPEAT, | ||
| "Repeat count too high, must be less than 20", | ||
| "repeat-count-high" | ||
| ) | ||
|
|
||
| duration = calculateRepeatDuration( | ||
| duration, | ||
| repeat, | ||
| repeatDelay | ||
| ) | ||
|
|
||
| const originalKeyframes = [...valueKeyframesAsList] | ||
| const originalTimes = [...times] | ||
| ease = Array.isArray(ease) ? [...ease] : [ease] | ||
| const originalEase = [...ease] | ||
| if (repeat >= MAX_REPEAT) { | ||
| /** | ||
| * For large/infinite repeat counts, don't expand keyframes. | ||
| * Pass repeat options through to the final transition | ||
| * and let the animation engine handle repeating. | ||
| */ | ||
| repeatPassthrough.set(valueSequence, { | ||
| repeat, | ||
| repeatType: repeatType as Transition["repeatType"], | ||
| repeatDelay: repeatDelay || undefined, | ||
| }) | ||
| } else { |
There was a problem hiding this comment.
repeat >= MAX_REPEAT in multi-segment sequences
When repeat >= 20 but is still finite (e.g., repeat: 25), the passthrough path skips calling calculateRepeatDuration, so duration remains the base per-iteration duration. This means maxDuration and targetTime — used to position subsequent segments in the sequence — are based only on the single-iteration duration.
Consider this scenario:
animate([
[el, { x: [0, 100] }, { duration: 1, repeat: 25 }],
[el, { y: [0, 100] }, { duration: 1 }], // Should start at t=26s, actually starts at t=1s
])For repeat: 19 (just below threshold), calculateRepeatDuration is called and the next segment correctly starts at 20s. For repeat: 20, it starts at 1s instead of 21s. There's a hard discontinuity at the MAX_REPEAT boundary.
For repeat: Infinity this is unavoidable (infinite duration can't be expanded), but for finite repeat >= 20, the correct duration could still be computed with calculateRepeatDuration and applied to maxDuration/targetTime, even if keyframe expansion is skipped.
if (repeat >= MAX_REPEAT) {
repeatPassthrough.set(valueSequence, {
repeat,
repeatType: repeatType as Transition["repeatType"],
repeatDelay: repeatDelay || undefined,
})
// Still adjust duration for finite repeats so subsequent segments are placed correctly
if (isFinite(repeat)) {
duration = calculateRepeatDuration(duration, repeat, repeatDelay)
}
} else {| repeatPassthrough.set(valueSequence, { | ||
| repeat, | ||
| repeatType: repeatType as Transition["repeatType"], | ||
| repeatDelay: repeatDelay || undefined, | ||
| }) |
There was a problem hiding this comment.
repeatType: undefined spreads into final transition unconditionally
When the user doesn't provide repeatType, it destructures as undefined. Spreading { repeatType: undefined } into the final transition object will set repeatType to undefined as an explicit key, potentially overriding any repeatType set in sequenceTransition (which is spread just before this in line 409).
| repeatPassthrough.set(valueSequence, { | |
| repeat, | |
| repeatType: repeatType as Transition["repeatType"], | |
| repeatDelay: repeatDelay || undefined, | |
| }) | |
| repeatPassthrough.set(valueSequence, { | |
| repeat, | |
| ...(repeatType !== undefined && { repeatType: repeatType as Transition["repeatType"] }), | |
| ...(repeatDelay ? { repeatDelay } : {}), | |
| }) |
| test("It passes repeat: Infinity through to the final transition (#2915)", () => { | ||
| const animations = createAnimationsFromSequence( | ||
| [ | ||
| [ | ||
| a, | ||
| { x: [0, 100] }, | ||
| { duration: 1, repeat: Infinity, ease: "linear" }, | ||
| ], | ||
| ], | ||
| undefined, | ||
| undefined, | ||
| { spring } | ||
| ) | ||
|
|
||
| expect(animations.get(a)!.keyframes.x).toEqual([0, 100]) | ||
| const { duration, times, ease, repeat } = | ||
| animations.get(a)!.transition.x | ||
| expect(duration).toEqual(1) | ||
| expect(times).toEqual([0, 1]) | ||
| expect(ease).toEqual(["linear", "linear"]) | ||
| expect(repeat).toEqual(Infinity) | ||
| }) |
There was a problem hiding this comment.
The new test covers repeat: Infinity but leaves several important cases untested:
repeatTypepassthrough: No assertion thatrepeatType(e.g."mirror") is correctly propagated through to the final transition.repeatDelaypassthrough: No assertion that a non-zerorepeatDelaymakes it to the final transition.- Finite
repeat >= MAX_REPEATin multi-segment sequences: A test like the existing "Repeating a segment correctly places the next segment at the end" test but withrepeat: 25would expose the timing discontinuity noted in the logic comment above. repeat: 20boundary: This is the smallest value that now follows the passthrough path, and its behavior (timing for subsequent segments) is not exercised at all.
1 participant
Summary
repeatoptions (includingrepeat: Infinity) being broken when used inAnimationSequencesegment transitionsrepeat >= 20(includingInfinity), passesrepeat/repeatType/repeatDelaythrough to the final transition instead of attempting keyframe expansion (which threw in dev or caused an infinite loop in production)Bug
When passing an array of animate definitions to
animate(),repeatoptions in segment transitions were broken for large/infinite values. The sequence builder tried to expand keyframes inline for all repeat counts, but:repeat: Infinitytriggered an invariant error in dev ("Repeat count too high")forloopFix
For
repeat >= MAX_REPEAT(20), skip keyframe expansion and instead store the repeat options (repeat,repeatType,repeatDelay) to be passed through to the final transition object. The animation engine already handles these properties natively, so no keyframe expansion is needed.Fixes #2915
🤖 Generated with Claude Code