Fix transform animation jumping under CPU load

[mattgperry]

Summary

• Fixed transform animations jumping when rapidly interrupting animations under CPU load
• The issue occurred because WAAPI's currentTime doesn't accurately reflect elapsed time when the main thread is blocked
• The fix uses time.now() from the frameloop to track wall-clock elapsed time independently

Changes

• Add startedAt timestamp using time.now() in NativeAnimationExtended
• Use elapsed wall-clock time for animation sampling instead of WAAPI's currentTime
• Add E2E test reproducing the rapid hover scenario (row of circles with floating indicator)
• Document time.now() usage in CLAUDE.md

Test plan

• New Playwright E2E test: animate-transform-jump.html
• Test simulates rapid hovers with CPU load blocking
• Verifies no jumps > 80px occur between frames
• All existing animate tests pass (108 tests)

🤖 Generated with Claude Code

[claude]

Code Review

Summary

This PR fixes a critical bug where transform animations jump during rapid interruptions under CPU load. The root cause was correctly identified: WAAPI's currentTime doesn't accurately reflect elapsed wall-clock time when the main thread is blocked, leading to incorrect sampling and visual discontinuities.

✅ Strengths

1. Root Cause Analysis: Excellent diagnosis of the timing issue. The insight that WAAPI's currentTime lags behind real elapsed time under CPU pressure is correct and well-documented.

2. Test Coverage: Strong E2E test that reproduces the real-world scenario:

   • Simulates rapid hovers with CPU blocking
   • Uses real DOM events via dispatchEvent
   • Validates no jumps > 80px occur
   • Includes visual debugging aids

3. Documentation: Good updates to CLAUDE.md explaining when to use time.now() vs performance.now().

4. Minimal Changes: The fix is surgical and doesn't unnecessarily refactor surrounding code.

🔍 Issues & Concerns

1. Critical: startedAt Never Updates on Animation Restart

private startedAt: number = time.now()

Problem: startedAt is initialized once in the field initializer and never reset. When an animation is interrupted and a new animation starts, the elapsed time calculation time.now() - this.startedAt will use the timestamp from the first animation, not the current one.

Impact: On rapid interruptions (the exact scenario this PR aims to fix), the elapsed time will grow unboundedly, causing incorrect sampling.

Example:

• Animation 1 starts at t=0ms, startedAt = 0
• Animation 1 interrupted at t=50ms
• Animation 2 starts at t=50ms (new NativeAnimationExtended instance)
• But if the same instance is reused: elapsedTime = 100 - 0 = 100ms ❌ (should be 50ms)

Fix Needed: Reset startedAt in the constructor or when play() is called:

constructor(options: NativeAnimationOptionsExtended<T>) {
    // ... existing code ...
    this.startedAt = time.now() // Move here after super()
}

2. Logic Issue: Sampling Time Calculation (packages/motion-dom/src/animation/NativeAnimationExtended.ts:92-93)

const sampleTime = Math.max(sampleDelta, elapsedTime)
const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

Problem: This logic is confusing and potentially incorrect:

• If elapsedTime < sampleDelta (early in animation), sampleTime = sampleDelta, which means you're sampling 10ms into the future
• The delta calculation clamp(0, sampleDelta, sampleTime - sampleDelta) will be 0 when sampleTime = sampleDelta, making velocity calculation inaccurate

Expected behavior: Should sample at the actual elapsed time, not clamp to sampleDelta:

const sampleTime = elapsedTime
const delta = Math.min(sampleDelta, elapsedTime) // Can't look back further than animation start

3. Edge Case: Very Short Animations (packages/motion-dom/src/animation/NativeAnimationExtended.ts:96)

motionValue.setWithVelocity(
    sampleAnimation.sample(Math.max(0, sampleTime - delta)).value,
    sampleAnimation.sample(sampleTime).value,
    delta
)

If delta = 0 (which can happen with current logic), setWithVelocity will divide by zero or produce Infinity. Verify that setWithVelocity handles delta = 0 gracefully.

4. Test Reliability Concern (tests/animate/animate.spec.ts:524)

await page.waitForTimeout(800)

Issue: Fixed timeouts in E2E tests can be flaky on slower CI runners. Consider waiting for a specific condition instead:

await page.waitForFunction(() => {
    const text = document.querySelector('#result')?.innerText
    return text && (text.startsWith('smooth:') || text.startsWith('jump:'))
}, { timeout: 2000 })

5. Minor: Missing Import (packages/motion-dom/src/animation/NativeAnimationExtended.ts:1)

You removed the import for secondsToMilliseconds but it might be used elsewhere in this file. Verify the code still compiles.

🎯 Recommendations

1. High Priority: Fix the startedAt initialization issue (Initial API (WIP) #1)
2. High Priority: Clarify the sampling time logic (Fixing types and es references #2)
3. Medium Priority: Verify setWithVelocity handles zero delta (Add motionValue prop #3)
4. Low Priority: Improve test reliability (adding circleci config #4)
5. Low Priority: Add unit tests for edge cases (zero elapsed time, very long elapsed time)

📊 Performance & Security

• Performance: ✅ Negligible impact. time.now() is efficient and the additional timestamp tracking is minimal.
• Security: ✅ No security concerns. Changes are isolated to animation timing logic.

🧪 Testing

• E2E test is comprehensive for the happy path
• Missing unit tests for edge cases:
  • Animation interrupted immediately (elapsedTime ≈ 0)
  • Animation interrupted after long delay
  • Multiple rapid interruptions (>3)

Overall Assessment

This PR addresses a real bug with the right approach, but has implementation issues in the timing calculation logic that need correction before merge. The core insight is sound—the execution needs refinement.

Recommendation: Request changes to address issues #1 and #2 before merging.

---

Review generated with Claude Code

[claude]

Code Review

I've reviewed this PR and here's my detailed feedback:

✅ Overall Assessment

This is a well-executed fix for a subtle timing bug in WAAPI animations under CPU load. The approach is sound, well-documented, and properly tested.

---

🎯 Code Quality & Best Practices

Strengths:

• Clear, descriptive comments explaining the root cause and fix (lines 24-28, 87-91 in NativeAnimationExtended.ts)
• Follows repository conventions (uses interface, named exports, arrow functions)
• Good use of private member for startedAt to encapsulate timing logic
• Documentation added to CLAUDE.md helps future maintainers understand when to use time.now()

Minor Issue - Clamp Usage:
There's a potential bug in line 95 of NativeAnimationExtended.ts:

const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

The clamp function signature is clamp(min, max, value), so this is correct. However, the logic seems questionable:

• sampleTime is defined as Math.max(sampleDelta, elapsedTime)
• When elapsedTime < sampleDelta, sampleTime = sampleDelta, so sampleTime - sampleDelta = 0
• When elapsedTime >= sampleDelta, sampleTime = elapsedTime, so sampleTime - sampleDelta can be much larger than sampleDelta
• The clamp then limits this to [0, sampleDelta]

Question: Is this intentional? It seems like delta is meant to represent the time difference for velocity calculation, but the logic is complex. Consider simplifying or adding a comment explaining why this specific calculation is needed.

Suggested improvement:

// Clamp delta to ensure we don't sample too far back when starting or under extreme CPU load
const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

Or if the intent is simpler:

const delta = Math.min(sampleDelta, elapsedTime)

---

🐛 Potential Bugs

1. Edge case on line 98: Math.max(0, sampleTime - delta) could theoretically be redundant since delta is already clamped to not exceed sampleTime - sampleDelta and we know sampleTime >= sampleDelta. However, defensive programming is fine here.

2. Race condition consideration: The startedAt timestamp is set in the constructor (line 54) AFTER calling super(options). If the parent constructor triggers any async operations that call updateMotionValue, there could be an issue. However, reviewing NativeAnimation, the WAAPI animation starts synchronously but callbacks are async, so this should be safe.

---

⚡ Performance Considerations

Excellent:

• Using time.now() instead of performance.now() is correct for frame synchronization
• The fix is minimal overhead (one subtraction per update)
• Creating a temporary JSAnimation for sampling is already the pattern, so no new performance concerns

Note: The time.now() implementation uses a microtask to clear cached time, which is efficient and appropriate.

---

🔒 Security Concerns

No security issues identified. This is internal animation timing logic with no user input or external data handling.

---

🧪 Test Coverage

Strong:

• New E2E test reproduces the exact scenario (rapid hovers with CPU blocking)
• Test uses realistic conditions with dispatchEvent for mouseenter
• Clear pass/fail criteria (80px jump threshold)
• Test includes debugging output (#result, #debug divs)

Suggestions:

1. Test robustness: The test uses hardcoded timing (8ms CPU block, 200ms settle). Consider if this could be flaky on slower CI environments. Might want to increase settle time or make it proportional to animation duration.

2. Coverage gap: The test only validates the final result text matches /^smooth:\d+$/. Consider also checking:

   const maxJump = await page.locator('#indicator').getAttribute('data-maxJump')
   expect(Number(maxJump)).toBeLessThan(80)

3. Edge case tests: Consider adding tests for:

   • Animation interrupted immediately after start (elapsedTime ~ 0)
   • Very long-running animations
   • Negative playback rates

---

📝 Documentation & Comments

Excellent:

• CHANGELOG.md updated appropriately
• CLAUDE.md now documents time.now() usage pattern
• Inline comments explain the "why" not just the "what"

---

🔧 Suggestions for Improvement

1. Line 94-95 clarity: Simplify the delta calculation or add more explanation:

   // Use wall-clock elapsed time for accurate sampling under CPU load
   const elapsedTime = time.now() - this.startedAt
   
   // Sample at least sampleDelta ms into the animation for velocity calculation
   const sampleTime = Math.max(sampleDelta, elapsedTime)
   
   // Calculate actual time delta for velocity, clamped to sampleDelta
   // to avoid erratic velocity when animation just started
   const delta = Math.min(sampleDelta, elapsedTime)

2. Test naming: The test description "rapid hover should not cause transform jump" is good, but consider being more specific about what causes the jump (CPU load + WAAPI timing).

3. Consider adding a unit test: While the E2E test is great, a unit test directly testing NativeAnimationExtended.updateMotionValue with mocked time.now() could verify the calculation logic more precisely.

---

✨ Final Verdict

LGTM with minor suggestions. This is a solid fix that:

• Correctly identifies and solves the root cause
• Uses appropriate APIs (time.now() for frame sync)
• Includes comprehensive testing
• Maintains backward compatibility
• Follows project conventions

The only hesitation is the delta calculation logic which seems overly complex for what appears to be Math.min(sampleDelta, elapsedTime). Clarification or simplification there would strengthen confidence in the fix.

---

Recommendation: ✅ Approve with suggestions for follow-up

Great work! 🎉

[claude]

Code Review

Summary

This PR fixes a critical bug where transform animations would jump when rapidly interrupted under CPU load. The fix correctly uses wall-clock time from time.now() instead of WAAPI's currentTime to track animation progress, which addresses the root cause of timing inconsistencies when the main thread is blocked.

---

✅ Strengths

1. Root Cause Analysis: The fix correctly identifies that WAAPI's currentTime doesn't accurately reflect elapsed time when the main thread is blocked, leading to incorrect sampling.

2. Clean Architecture: The use of manualStartTime with proper lifecycle management (clearing on play() and time setter) is well-designed and maintains WAAPI compatibility.

3. Documentation: Adding the timing guidance to CLAUDE.md helps future contributors understand when to use time.now() vs performance.now().

4. Minimal Scope: The fix is surgical - it only touches the necessary code paths without over-engineering.

---

🔍 Issues & Concerns

1. Missing Test File ⚠️

The PR description mentions:

New Playwright E2E test: animate-transform-jump.html

However, this test file is not included in the PR. The only test change is adding a blank line to tests/animate/animate.spec.ts.

Action Required:

• Add the E2E test file that reproduces the rapid hover scenario with CPU blocking
• Update animate.spec.ts to include the actual test case that verifies no jumps > 80px occur

2. Potential Edge Case: Negative Elapsed Time ⚠️

const elapsedTime = time.now() - this.startTime
const sampleTime = Math.max(sampleDelta, elapsedTime)

If time.now() is called before this.startTime is set (or if there's clock skew), elapsedTime could be negative. While Math.max(sampleDelta, elapsedTime) provides some protection, the downstream Math.max(0, sampleTime - delta) may still behave unexpectedly.

Recommendation: Add explicit check:

const elapsedTime = Math.max(0, time.now() - this.startTime)

3. Inconsistent Time Units ⚠️

const sampleDelta = 10 //ms

However, time.now() and this.startTime appear to be in milliseconds (based on performance.now() in sync-time.ts), but this isn't clearly documented. The calculation seems to work, but the comment should clarify units.

Recommendation: Update comment:

const sampleDelta = 10 // ms (matches time.now() units)

4. Setter Override May Cause Confusion

set time(newTime: number) {
    this.manualStartTime = null
    super.time = newTime
}

get time() {
    return super.time
}

The getter and setter have asymmetric behavior - the getter is a simple passthrough but the setter has side effects. This is correct for the fix but could be surprising.

Recommendation: Add JSDoc comment explaining the clearing behavior:

/**
 * Setting time manually should revert to WAAPI-controlled timing.
 */
set time(newTime: number) {
    this.manualStartTime = null
    super.time = newTime
}

---

💭 Questions

1. What happens if this.startTime returns NaN?

   • When manualStartTime is null and animation.startTime is null/undefined, the getter returns Number(null) = 0 or NaN. This could cause elapsedTime to be unexpectedly large.

2. Why is the test file missing from the PR?

   • Was it accidentally not committed, or is it coming in a follow-up?

3. Have you verified this works with scroll-linked animations?

   • The fix is in the base class, so it should apply to all animation types, but scroll timelines have different timing semantics.

---

🎯 Code Quality

✅ Follows CLAUDE.md conventions:

• Uses interface (not applicable here)
• Named exports used
• Arrow functions where appropriate
• Strict equality used
• No var declarations
• Uses time.now() as documented

✅ No security concerns identified

✅ Performance: Minimal overhead - just one additional time.now() call during interruption

---

📋 Recommendations

Before merging:

1. ✅ Add the missing E2E test file mentioned in PR description
2. ✅ Add test coverage for edge cases (negative elapsed time, null startTime)
3. ⚠️ Consider adding explicit Math.max(0, ...) for elapsedTime
4. ⚠️ Add JSDoc comments for the asymmetric time getter/setter
5. ⚠️ Clarify time units in comments

Nice to have:

• Add inline comment in NativeAnimation.ts:228 explaining why manualStartTime takes precedence
• Consider adding a console warning in dev mode if elapsedTime < 0 is detected

---

🎉 Overall Assessment

Strong fix that addresses a real production issue with a well-reasoned approach. The architecture is sound and the performance impact is minimal. Main concern is the missing test file which is marked as completed in the PR description. Once the test is added and edge cases are validated, this should be good to merge.

Rating: Approve with changes ✅ (pending test file addition)

[claude]

Code Review

I've reviewed PR #3463 and here's my feedback:

✅ Strengths

1. Clear Problem Identification: The fix addresses a real issue where WAAPI's currentTime doesn't accurately reflect elapsed time under CPU load, causing transform animation jumps during rapid interruptions.

2. Clean Implementation: The manualStartTime pattern is elegant - it tracks wall-clock time independently and clears automatically when WAAPI takes over timing (play() or time setter).

3. Defensive Programming: Good use of clamp() and Math.max(0, ...) to prevent negative values.

4. Documentation: Updated CLAUDE.md with timing guidelines, which helps maintain consistency across the codebase.

5. Proper Test Coverage: The PR mentions an E2E test that reproduces the rapid hover scenario with CPU load.

🔍 Potential Issues & Questions

1. Time Synchronization Concern (packages/motion-dom/src/animation/NativeAnimationExtended.ts:100)

The fix uses time.now() which returns frame-synchronized time in certain contexts:

const sampleTime =
    this.manualStartTime !== null
        ? Math.max(sampleDelta, time.now() - this.manualStartTime)
        : secondsToMilliseconds(this.finishedTime ?? this.time)

Question: When updateMotionValue() is called during the frame processing phase, time.now() returns frameData.timestamp (a cached value). If multiple rapid interruptions occur within the same frame, all would get identical time.now() values, potentially causing incorrect elapsed time calculations. Have you tested the scenario where multiple animations interrupt within a single frame?

2. Missing Type Safety (packages/motion-dom/src/animation/NativeAnimation.ts:53)

The manualStartTime property is added to the base class but only used by the extended class:

protected manualStartTime: number | null = null

Suggestion: Consider documenting this in NativeAnimation's constructor or adding a comment explaining that this is intentionally used by subclasses.

3. Edge Case: Zero Delta (packages/motion-dom/src/animation/NativeAnimationExtended.ts:102)

const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

If sampleTime < sampleDelta, delta becomes 0. Then:

motionValue.setWithVelocity(
    sampleAnimation.sample(Math.max(0, sampleTime - delta)).value,
    sampleAnimation.sample(sampleTime).value,
    delta  // Could be 0
)

Concern: With delta = 0, setWithVelocity sets prevUpdatedAt = this.updatedAt - 0, making prev and current timestamps identical. This could cause velocity calculations to divide by zero or return incorrect values. Should there be a minimum delta threshold?

4. Test File Missing from Diff

The PR description mentions an E2E test file (animate-transform-jump.html), but it's not in the diff. Was this test file added in an earlier commit or is it missing from the PR?

5. Potential Race Condition (packages/motion-dom/src/animation/NativeAnimationExtended.ts:54-65)

The overridden play() and time setter clear manualStartTime:

play() {
    this.manualStartTime = null
    super.play()
}

Edge case: If updateMotionValue() is called asynchronously (e.g., via a setTimeout or Promise) after play() is called but before the animation actually runs, manualStartTime would be null, causing the code to fall back to WAAPI's currentTime. Is this the intended behavior?

📊 Performance Considerations

Positive: Using time.now() is performant as it caches the time value within synchronous contexts, avoiding repeated performance.now() calls.

Minor concern: Creating a new JSAnimation instance in updateMotionValue() for every interruption adds some overhead, but this is likely negligible and was already present in the code.

🔒 Security Considerations

No security concerns identified. This is pure animation timing logic with no user input or external data processing.

🧪 Test Coverage

Strengths:

• E2E test for the specific bug scenario
• Tests rapid hovers with CPU load blocking
• Validates no large jumps (>80px) occur

Recommendations:

1. Add a unit test for the manualStartTime lifecycle (set in constructor, cleared on play/time setter)
2. Test the edge case where sampleTime < sampleDelta
3. Test multiple rapid interruptions within the same frame
4. Verify the E2E test file is included in the PR

📝 Code Style

✅ Follows repository conventions:

• Uses interface over type (where applicable)
• Proper use of arrow functions
• Consistent with existing patterns
• Good inline documentation

🎯 Overall Assessment

This is a solid fix for a tricky timing issue. The core approach is sound, but there are a few edge cases worth investigating:

1. Verify behavior when multiple animations interrupt in the same frame
2. Consider minimum delta threshold to prevent velocity calculation issues
3. Ensure the E2E test file is included in the PR
4. Document the intended behavior when play() is called before updateMotionValue() executes

Recommendation: Approve with minor revisions to address the delta edge case and ensure test coverage is complete.

---

Great work on tracking down and fixing this CPU load timing issue! The manualStartTime pattern is a clean solution.

[claude]

Code Review

Summary

This PR fixes a subtle timing bug in transform animations when rapidly interrupting animations under CPU load. The fix uses wall-clock time tracking via time.now() instead of relying on WAAPI's currentTime, which can lag when the main thread is blocked.

Positive Aspects ✅

1. Well-identified root cause: The issue correctly identifies that WAAPI's currentTime doesn't accurately reflect elapsed time when the main thread is blocked, which is a real limitation of the Web Animations API.

2. Clean implementation: The manualStartTime mechanism is elegant - it tracks wall-clock time when set via options.startTime, then clears itself when the animation is explicitly played or time is set, allowing WAAPI to take over.

3. Consistent with existing patterns: Using time.now() from sync-time.ts aligns with the codebase's approach to frame-synced timing.

4. Documentation updated: The addition to CLAUDE.md about time.now() usage is helpful for future contributors.

5. Safety guards: The use of Math.max(sampleDelta, ...) and clamp() prevents edge cases with negative or invalid time values.

Issues and Concerns ⚠️

1. Missing E2E Test 🔴 Critical

The PR description mentions:

• New Playwright E2E test: animate-transform-jump.html

However, this test file does not exist in the PR. The only change to tests is adding a single blank line to tests/animate/animate.spec.ts. This is a significant gap - the PR claims to have a test that reproduces the issue, but it's not included.

Recommendation: Add the missing E2E test file that reproduces the rapid hover scenario with CPU load. According to CLAUDE.md, UI interaction bugs should use E2E tests.

2. Potential Edge Case in Time Calculation ⚠️ Medium

In NativeAnimationExtended.ts:98-102:

const sampleTime =
    this.manualStartTime !== null
        ? Math.max(sampleDelta, time.now() - this.manualStartTime)
        : secondsToMilliseconds(this.finishedTime ?? this.time)
const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

When sampleTime is very close to sampleDelta (e.g., animation just started), delta will be clamped to nearly 0. This could result in:

• sampleAnimation.sample(sampleTime - delta) and sampleAnimation.sample(sampleTime) being very close
• The velocity calculation in setWithVelocity becoming unreliable due to division by a very small delta

Recommendation: Consider adding a minimum threshold for delta (e.g., 1-2ms) or special handling when sampleTime < sampleDelta * 2.

3. Inconsistent Units ⚠️ Low

The code mixes time units without clear conversion:

• time.now() returns milliseconds (from performance.now())
• sampleDelta is in milliseconds (10ms)
• But this.time is in seconds (from NativeAnimation.get time())
• secondsToMilliseconds is used to convert

This works correctly but could be clearer. Consider adding a comment explaining that time.now() returns milliseconds and manualStartTime is also in milliseconds.

4. Base Class Property Without Override ⚠️ Low

In NativeAnimation.ts:53, manualStartTime is added as a protected property. However, the base class doesn't use it at all - only NativeAnimationExtended uses it.

Recommendation: Consider moving this property to NativeAnimationExtended where it's actually used, unless there's a future plan for the base class to use it.

5. Type Safety in Time Setter ⚠️ Low

In NativeAnimationExtended.ts:59-62:

set time(newTime: number) {
    this.manualStartTime = null
    super.time = newTime
}

The getter is defined but just delegates to super. This is correct but the pattern is unusual - typically if you override the setter, you need the getter to avoid asymmetry. The current implementation is fine but could have a comment explaining why the getter is needed.

Testing Recommendations 📋

1. Add the missing E2E test that reproduces the rapid interruption scenario
2. Add unit tests for edge cases:
   • Animation interrupted immediately after starting (time.now() - manualStartTime < sampleDelta)
   • play() called after animation starts (verifies manualStartTime is cleared)
   • Direct time setter (verifies manualStartTime is cleared)
3. Verify behavior when options.startTime is 0 (falsy but valid)

Performance Considerations ⚡

The change adds minimal overhead:

• One additional time.now() call per interruption
• One null check in the hot path
• No additional memory allocations

Performance impact is negligible. ✅

Security Concerns 🔒

No security concerns identified. ✅

Code Style ✨

The code follows the repository's conventions from CLAUDE.md:

• Uses interface for type definitions ✅
• No default exports ✅
• Arrow callbacks where appropriate ✅
• Strict equality ✅
• No var declarations ✅

Changelog

The changelog entry is clear and concise. ✅

---

Verdict

The fix is fundamentally sound and addresses a real timing issue with WAAPI. However, the missing E2E test is a critical gap that should be addressed before merging. The test is claimed to exist in the PR description but is not included in the changes.

Recommendation: Add the missing test, consider the edge case handling for very small time deltas, and this will be ready to merge.

Action Items

• Add animate-transform-jump.html E2E test
• Add test case in animate.spec.ts that uses the new HTML file
• Consider minimum threshold for delta to ensure reliable velocity calculation
• Add comment explaining time.now() returns milliseconds

---

Great work identifying and fixing this subtle timing bug! The wall-clock time approach is the right solution.

[claude]

Code Review: Fix transform animation jumping under CPU load

Overview

This PR addresses a critical timing issue where WAAPI animations would jump to incorrect positions when rapidly interrupted under CPU load. The fix switches from using WAAPI's currentTime to using time.now() for accurate wall-clock elapsed time tracking.

---

✅ Strengths

1. Root Cause Identified: The PR correctly identifies that WAAPI's currentTime doesn't accurately reflect elapsed time when the main thread is blocked, which is the source of the visual jumps.

2. Elegant Solution: Using time.now() from the frameloop provides frame-synced timestamps that remain consistent within synchronous contexts.

3. Documentation: Added clear documentation to CLAUDE.md about using time.now() instead of performance.now(), which will help future contributors.

4. Changelog Updated: Properly documented the fix in CHANGELOG.md for version 12.24.10.

---

⚠️ Potential Issues & Questions

1. Missing E2E Test File

The PR description mentions a new Playwright E2E test (animate-transform-jump.html), but this file is not included in the diff. Only a trivial newline was added to tests/animate/animate.spec.ts.

Impact: Without the E2E test, we cannot verify:

• The fix actually resolves the rapid hover scenario
• No regressions occur under CPU load
• The "no jumps > 80px" assertion passes

Recommendation: Ensure the E2E test file is committed and included in the PR.

2. Potential NaN/Negative Values in Edge Cases

In NativeAnimationExtended.ts:79, the code calculates:

const sampleTime = Math.max(sampleDelta, time.now() - this.startTime)

Concern: If this.startTime is undefined, null, or not yet initialized, time.now() - this.startTime could produce NaN or unexpected values.

Analysis:

• this.startTime is inherited from NativeAnimation class
• It's a getter that returns Number(this.animation.startTime)
• If WAAPI's animation.startTime is null (common before animation starts), this would return NaN

Recommendation: Add a safety check:

const elapsed = this.startTime ? time.now() - this.startTime : 0
const sampleTime = Math.max(sampleDelta, elapsed)

3. Delta Calculation Complexity

Line 80 has:

const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

This clamps between 0 and sampleTime - sampleDelta. Let me trace through scenarios:

• If sampleTime = 50ms and sampleDelta = 10ms: delta = clamp(0, 10, 40) = 10 ✅
• If sampleTime = 5ms and sampleDelta = 10ms: delta = clamp(0, 10, -5) = 0 ✅

The logic appears sound, but the intent could be clearer.

Suggestion: Add a comment explaining why we need this clamp:

// Ensure delta doesn't exceed actual elapsed time
const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

4. Removed Constructor Logic

The original NativeAnimationExtended constructor had:

if (options.startTime) {
    this.startTime = options.startTime
}

This was removed. Why?

Potential Impact: If any code was passing startTime in options expecting it to be set, that behavior has changed. The startTime option is still in the interface (NativeAnimationOptions), so this could be a breaking change.

Recommendation:

• Verify no callers rely on this behavior, OR
• Restore this logic if it's needed for other use cases

5. Time Synchronization Concerns

The fix uses time.now() which returns frame-synced time, while WAAPI uses performance.now() internally. This creates two different time sources:

• Frame-synced time (time.now()): Frozen within synchronous execution, updated per microtask
• WAAPI time (performance.now()): Wall-clock time that always advances

Question: Could this mismatch cause issues when animations resume/play/pause, since WAAPI is still managing the underlying timeline with its own time source?

Analysis Needed: Test scenarios:

• Pause → Resume animation
• Change playback speed mid-animation
• Seek to specific time with animation.currentTime = X

---

🔍 Code Quality

Style Adherence ✅

• Uses const appropriately
• Proper imports organization
• Clear comments explaining the "why" (NativeAnimationExtended.ts:74-77)

Type Safety ✅

• No type assertions or any usage
• Proper use of existing types

---

🧪 Testing Coverage

Mentioned but Missing:

• ❌ New E2E test file not in diff
• ❌ No unit tests for edge cases (startTime null/undefined)
• ❌ No test for rapid interruptions with CPU load simulation

Existing Tests:

• ✅ PR claims "all existing animate tests pass (108 tests)"

Recommendation: Add unit tests for:

describe("NativeAnimationExtended timing", () => {
  test("handles null startTime gracefully", () => { ... })
  test("calculates correct sampleTime under load", () => { ... })
  test("delta never exceeds elapsed time", () => { ... })
})

---

🚀 Performance Considerations

Positive:

• time.now() uses caching (clears via microtask), so multiple calls in sync context return same value
• No additional WAAPI calls added
• Sample animation is created and destroyed efficiently

Neutral:

• Creating a temporary JSAnimation for sampling adds overhead, but this is existing behavior

---

🔒 Security Concerns

No security issues identified. This is internal timing logic with no user input.

---

📋 Final Recommendations

1. Critical: Add the missing E2E test file or clarify why it's not in the diff
2. Important: Add null/undefined check for this.startTime before arithmetic
3. Important: Verify removing options.startTime assignment doesn't break existing functionality
4. Nice to have: Add inline comment explaining the delta clamp logic
5. Nice to have: Add unit tests for edge cases
6. Consider: Document any limitations around pause/resume/seek with the new timing approach

---

Summary

The core fix is sound and addresses a real issue. The use of time.now() for consistent timing is appropriate. However, the PR needs the E2E test file and should add safety checks for edge cases. Overall: Approve with requested changes.

Rating: 7/10

• Deductions for missing test file and potential null handling issue
• Strong points for clear problem identification and elegant solution

[claude]

Code Review - PR #3463

Summary

This PR fixes a critical bug where transform animations would jump when rapidly interrupted under CPU load. The fix correctly addresses the root cause by using wall-clock time (time.now()) instead of relying on WAAPI's currentTime, which doesn't accurately reflect elapsed time when the main thread is blocked.

---

✅ Strengths

1. Root Cause Analysis: Excellent identification of the issue - WAAPI's currentTime being unreliable under CPU load is a subtle but important distinction.

2. Architecture: The solution properly separates concerns:

   • manualStartTime in NativeAnimation provides the override mechanism
   • NativeAnimationExtended uses it for accurate sampling
   • Clear lifecycle management (cleared on play() and time setter)

3. Documentation: Adding the timing guidance to CLAUDE.md is valuable for future contributors.

4. Changelog: Properly documented with clear user-facing description.

---

🔍 Issues & Suggestions

1. Missing E2E Test File ⚠️

The PR description mentions animate-transform-jump.html test and a Playwright spec, but:

• The test file is not included in the PR
• tests/animate/animate.spec.ts only has a blank line added (line 514)

Action Required: The E2E test should be included in this PR to:

• Verify the fix works
• Prevent regressions
• Document the expected behavior

2. Potential Timing Issue in updateMotionValue 🐛

In NativeAnimationExtended.ts:83-84:

const sampleTime = Math.max(sampleDelta, time.now() - this.startTime)
const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

Problem: If sampleTime < sampleDelta * 2, then delta could be very small (approaching 0), which would:

• Make velocity calculation unreliable (setWithVelocity divides by delta)
• Potentially cause division by very small numbers

Example:

• If sampleTime = 15ms (just started)
• Then delta = clamp(0, 10, 15 - 10) = 5ms
• But if sampleTime = 10ms exactly
• Then delta = clamp(0, 10, 10 - 10) = 0ms ← Division by zero!

Suggestion: Consider:

const sampleTime = Math.max(sampleDelta * 2, time.now() - this.startTime)
const delta = sampleDelta

Or ensure delta has a minimum value to prevent velocity calculation issues.

3. Type Safety in Constructor ℹ️

In NativeAnimationExtended.ts:47:

if (options.startTime !== undefined) {
    this.manualStartTime = options.startTime
}

Good: Changed from if (options.startTime) to check !== undefined, correctly handling startTime: 0.

Minor: The base class also accepts startTime but doesn't use manualStartTime. Consider if the base constructor should handle this, or document why extended class needs special handling.

4. Inconsistent Time Units ⚠️

The code mixes time units:

• sampleDelta = 10 (milliseconds, constant at line 17)
• time.now() returns milliseconds
• this.startTime returns milliseconds (from getter at NativeAnimation.ts:230)
• But this.time getter returns seconds (NativeAnimation.ts:199)

Current Code:

const sampleTime = Math.max(sampleDelta, time.now() - this.startTime)

This appears correct since:

• time.now() is in ms
• this.startTime getter returns manualStartTime ?? animation.startTime in ms
• sampleDelta is 10ms

But the naming and documentation could be clearer about units. Consider adding a comment:

// All values in milliseconds
const sampleTime = Math.max(sampleDelta, time.now() - this.startTime)

5. Edge Case: Negative startTime 🤔

If this.startTime is negative or time.now() is less than this.startTime, sampleTime could behave unexpectedly. This might be intentional, but worth validating.

---

🎯 Test Coverage

Missing:

• The E2E test mentioned in the PR description
• Unit tests for the manualStartTime lifecycle
• Tests verifying correct behavior when startTime = 0

Recommendation: Add tests covering:

1. Rapid animation interruptions under simulated CPU load
2. manualStartTime cleared on play() and time setter
3. Edge cases (very early interruption, startTime = 0, etc.)

---

🔒 Security & Performance

Security: ✅ No concerns - internal timing logic only.

Performance: ✅ Excellent:

• time.now() is efficiently cached per event loop
• No additional allocations
• Minimal computational overhead

---

📋 Checklist for Author

• Add the animate-transform-jump.html E2E test file
• Add the Playwright test spec that verifies no jumps > 80px
• Address the potential delta = 0 edge case in velocity calculation
• Consider adding unit tests for manualStartTime lifecycle
• Verify behavior when animation is interrupted very early (< 10ms)

---

Verdict

Conditional Approval ✅ (pending test inclusion)

The core fix is solid and addresses the root cause correctly. The architecture is clean. However, the PR is incomplete without the E2E test that validates the fix. Please add the test file and address the delta = 0 edge case before merging.

Once tests are added and the edge case is addressed, this will be a high-quality fix for an important bug.

---

Overall Code Quality: 8/10 (would be 9/10 with tests included)

[claude]

Code Review

Thanks for this fix! I've reviewed the changes and have some feedback on code quality, potential issues, and test coverage.

✅ Strengths

1. Root Cause Analysis: The fix correctly identifies that WAAPI's currentTime doesn't accurately reflect elapsed time when the main thread is blocked, which is a subtle but critical insight.

2. Appropriate Use of time.now(): Using the frameloop-synchronized timing is the right approach here, consistent with how it's used elsewhere in the codebase (JSAnimation, etc.).

3. Documentation: Good addition to CLAUDE.md explaining when to use time.now().

4. Manual Start Time Tracking: The manualStartTime mechanism in NativeAnimation is well-designed, allowing WAAPI to take over timing when appropriate.

🔍 Potential Issues

1. Critical: startTime May Be null or NaN

In NativeAnimationExtended.ts:83, the calculation assumes this.startTime is always a valid number:

const sampleTime = Math.max(sampleDelta, time.now() - this.startTime)

However, WAAPI's animation.startTime can be null when the animation hasn't started playing yet. The getter in NativeAnimation.ts:230 returns:

return this.manualStartTime ?? Number(this.animation.startTime)

If both are null, Number(null) returns 0, which could cause incorrect timing calculations. More critically, if the animation hasn't started, startTime could be legitimately null, making time.now() - 0 return a very large number.

Recommendation: Add a guard to handle the case where startTime is not yet valid:

const startTime = this.startTime
if (!startTime || startTime < 0) {
    // Animation hasn't started yet, use finishedTime or time as fallback
    motionValue.set(this.finishedTime !== null 
        ? sampleAnimation.sample(this.finishedTime).value
        : sampleAnimation.sample(this.time).value)
    sampleAnimation.stop()
    return
}

const sampleTime = Math.max(sampleDelta, time.now() - startTime)

2. Type Safety: options.startTime Check Changed

The change from if (options.startTime) to if (options.startTime !== undefined) in NativeAnimationExtended.ts:47 is good for handling startTime: 0, but this should be applied consistently in NativeAnimation.ts constructor as well if startTime is ever passed there.

3. Delta Calculation Logic Needs Clarification

In line 84:

const delta = clamp(0, sampleDelta, sampleTime - sampleDelta)

This seems incorrect. The clamp signature is clamp(min, max, value), but this is passing (0, sampleDelta, sampleTime - sampleDelta).

Looking at the previous code, it used sampleDelta directly as the time delta. The new code appears to be trying to use sampleDelta as the delta when possible, but clamp it if sampleTime is very small (early in animation).

Issue: If sampleTime = 50ms, then delta = clamp(0, 10, 40) = 10. But if sampleTime = 5ms, then delta = clamp(0, 10, -5) = 0.

This means:

• When sampleTime >= 2 * sampleDelta: delta = sampleDelta (correct)
• When sampleTime < 2 * sampleDelta: delta is reduced (good edge case handling)

However, the intent is unclear. Consider renaming variables or adding a comment:

// For velocity calculation, sample 10ms apart, but clamp if we're early in the animation
const delta = Math.min(sampleDelta, Math.max(0, sampleTime - sampleDelta))

Or keep it simple:

const delta = Math.min(sampleDelta, sampleTime)
const prevSampleTime = sampleTime - delta

🧪 Test Coverage Concerns

1. Missing E2E Test File

The PR description mentions a new Playwright E2E test animate-transform-jump.html, but this file is not included in the PR diff. The only test file change is a blank line addition to animate.spec.ts.

Action Required: The E2E test file needs to be added to properly validate the fix. Without it:

• We can't verify the fix actually prevents jumps under CPU load
• We can't prevent regressions in the future
• The test plan checklist item is not actually satisfied

2. Unit Test Coverage

Consider adding a unit test for NativeAnimationExtended.updateMotionValue() that:

• Mocks time.now() to return specific values
• Verifies the correct sample times are calculated
• Tests edge cases (animation just started, startTime is 0, etc.)

🎯 Performance Considerations

The change should have minimal performance impact since:

• time.now() is optimized for synchronous calls
• The calculation is simple arithmetic
• This only runs during animation interruptions, not every frame

🔒 Security

No security concerns identified.

📝 Code Style

Follows the repository conventions well:

• Uses interface appropriately
• Named exports
• Strict equality
• Good comments

Summary & Recommendations

Required Changes:

1. ❗ Add the missing E2E test file animate-transform-jump.html
2. ❗ Add null/undefined safety check for startTime before calculation
3. ⚠️ Clarify or simplify the delta calculation logic

Optional Improvements:
4. Consider adding unit tests for the new timing logic
5. Verify behavior when startTime === 0 (valid but edge case)

The core fix is sound and addresses a real issue, but the missing test file and potential null handling issue should be resolved before merging.

---

🤖 Review generated with Claude Code