Skip to content

Add hardware acceleration support for useSpring via WAAPI#3547

Closed
mattgperry wants to merge 2 commits intomainfrom
claude/accelerate-usespring-gVv7Q
Closed

Add hardware acceleration support for useSpring via WAAPI#3547
mattgperry wants to merge 2 commits intomainfrom
claude/accelerate-usespring-gVv7Q

Conversation

@mattgperry
Copy link
Collaborator

When useSpring creates a spring animation and the browser supports
linear() CSS easing, the MotionValue now carries an .accelerate config
through the useSpring → useTransform → bindToMotionValue pipeline.

  • Add AccelerateConfig interface and .accelerate property to MotionValue
  • In attachFollow, set .accelerate with spring factory/options when the
    spring can be hardware-accelerated (supportsLinearEasing + damping != 0)
  • In useTransform, propagate .accelerate from input to output when using
    range-based transform with clamp enabled, replacing times/keyframes/ease
  • In bindToMotionValue, when .accelerate is present on an acceleratable
    value (opacity, transform, filter, clipPath), listen for animationStart
    and create a WAAPI animation with spring easing via linear() CSS function

https://claude.ai/code/session_01L9oKDA2qPgTAzt5MYPLUbU

When useSpring creates a spring animation and the browser supports
linear() CSS easing, the MotionValue now carries an .accelerate config
through the useSpring → useTransform → bindToMotionValue pipeline.

- Add AccelerateConfig interface and .accelerate property to MotionValue
- In attachFollow, set .accelerate with spring factory/options when the
  spring can be hardware-accelerated (supportsLinearEasing + damping != 0)
- In useTransform, propagate .accelerate from input to output when using
  range-based transform with clamp enabled, replacing times/keyframes/ease
- In bindToMotionValue, when .accelerate is present on an acceleratable
  value (opacity, transform, filter, clipPath), listen for animationStart
  and create a WAAPI animation with spring easing via linear() CSS function

https://claude.ai/code/session_01L9oKDA2qPgTAzt5MYPLUbU
@claude
Copy link

claude bot commented Feb 8, 2026

PR Review: Hardware Acceleration Support for useSpring via WAAPI

This is an impressive feature that enables hardware-accelerated spring animations by leveraging WAAPI with CSS linear() easing functions. The implementation cleverly threads acceleration metadata through the useSpringuseTransformbindToMotionValue pipeline. Here's my detailed feedback:


✅ Strengths

  1. Well-designed architecture: The AccelerateConfig interface and .accelerate property provide a clean way to carry animation metadata through the pipeline without breaking existing APIs.

  2. Strong test coverage: Comprehensive tests cover the critical paths:

    • Acceleration config propagation through transforms
    • Feature detection (linear easing support)
    • Edge cases (damping=0, clamp=false, function transforms)
    • Spring options preservation
  3. Progressive enhancement: Gracefully degrades when supportsLinearEasing() returns false, ensuring backward compatibility.

  4. Proper cleanup: Animation cancellation and subscription removal in VisualElement.ts:641-646 prevents memory leaks.


🔍 Potential Issues & Suggestions

1. Error handling in VisualElement.ts (Lines 578, 645)

The empty catch blocks silently swallow errors:

try { activeAcceleration.cancel() } catch (e) {}

Issue: If cancel() fails for unexpected reasons, debugging becomes difficult.

Suggestion: Add minimal logging or comment explaining why errors are expected:

try { 
    activeAcceleration.cancel() 
} catch (e) {
    // Animation may already be cancelled/finished - safe to ignore
}

2. Type safety in follow-value.ts:123

const { type, onUpdate, onComplete, onPlay, onRepeat, onStop, ...springOptions } = options as any

Issue: Using as any bypasses type checking. If the options interface changes, this could silently break.

Suggestion: Define a proper type or use a type assertion with a more specific type:

type SpringTransitionOptions = Omit<ValueAnimationTransition, 'type' | 'onUpdate' | 'onComplete' | 'onPlay' | 'onRepeat' | 'onStop'>
const { type, onUpdate, onComplete, onPlay, onRepeat, onStop, ...springOptions } = options as SpringTransitionOptions

3. Non-null assertion in VisualElement.ts:595

factory.applyToOptions!(transition)

Issue: The ! non-null assertion assumes applyToOptions exists, but the interface defines it as optional (applyToOptions?: ...).

Suggestion: Add a runtime check:

if (!factory.applyToOptions) {
    // Fallback or early return
    return
}
factory.applyToOptions(transition)

4. Potential race condition in VisualElement.ts:582-583

const accel = value.accelerate
if (!accel) return

Issue: If .accelerate is cleared between the outer check (line 570) and this callback execution (line 582), the check is redundant. However, if it's cleared by another thread/callback, there could be subtle timing issues.

Suggestion: This is likely fine given JavaScript's single-threaded nature, but consider documenting the expected lifecycle of .accelerate modifications.

5. useTransform keyframes type assertion (Line 237)

keyframes: outputRange as AnyResolvedKeyframe[],

Issue: The outputRange is typed as O[] | undefined, but we're asserting it as AnyResolvedKeyframe[]. This could fail if O doesn't match AnyResolvedKeyframe.

Suggestion: While the runtime checks ensure this is safe, consider adding a comment explaining why this assertion is valid, or add a runtime type guard if O can be complex objects.

6. Performance consideration: animationStart event

The animationStart event listener (VisualElement.ts:576) is registered for every acceleratable value. For components with many animated properties (e.g., transforms with multiple values), this could create many event listeners.

Question: Has performance been tested with components animating multiple properties simultaneously? Consider profiling with a stress test.

7. Missing validation for keyframes/times array lengths

In VisualElement.ts:601-604, the code assumes keyframes and times arrays are compatible:

const keyframeOptions: PropertyIndexedKeyframes = {
    [key]: keyframes as string[],
}
if (times) keyframeOptions.offset = times

Issue: If times and keyframes have mismatched lengths, WAAPI may throw or behave unexpectedly.

Suggestion: Add validation:

if (times && times.length !== keyframes.length) {
    // Log warning or skip acceleration
    return
}

📝 Code Style (per CLAUDE.md)

✅ Good use of optional chaining (e.g., removeAnimationStart?.())
✅ Concise patterns that minimize bundle size
✅ Named exports used consistently
⚠️ Consider splitting the large VisualElement.ts change into a separate function to improve readability


🧪 Testing Recommendations

  1. Add E2E test: The unit tests are excellent, but consider adding a Playwright test that verifies the WAAPI animation actually runs on real elements with transforms/opacity.

  2. Test useTransform with multiple inputs: The current tests only cover single-input transforms. Test behavior with:

    const output = useTransform([x, y], ([xVal, yVal]) => xVal + yVal)
  3. Test animation cancellation: Verify that rapidly changing values properly cancels/restarts the WAAPI animation without memory leaks.

  4. Test with actual spring options: Add a test that verifies custom spring options (stiffness, damping, mass) produce the expected animation duration and easing curve.


🔒 Security

No security concerns identified. The code doesn't accept user input directly and operates on internal animation state.


🎯 Performance

Overall looks good, but consider:

  • The onfinish callback (line 630) could be replaced with a Promise-based approach for cleaner async handling
  • Verify GC behavior with long-lived components that create/destroy many springs

📦 Bundle Size

The changes add ~150 lines to core packages. Given the performance benefits, this seems reasonable. Consider:

  • Could AccelerateConfig be tree-shakeable for projects not using useSpring?
  • The mapEasingToNativeEasing calls (lines 607, 616) - are these already optimized?

✨ Overall Assessment

Strong approval with minor refinements suggested. This is a well-architected feature that maintains backward compatibility while enabling significant performance improvements. The test coverage is excellent, and the implementation follows the repository's conventions.

Priority fixes:

  1. Add runtime check for factory.applyToOptions (issue Add motionValue prop #3)
  2. Validate keyframes/times array length matching (issue Adding useViewportScroll #7)
  3. Replace as any type assertion (issue Fixing types and es references #2)

Nice-to-haves:

  • Add explanatory comments to empty catch blocks
  • Add E2E test for real WAAPI behavior
  • Performance profiling with many simultaneous animations

Great work! 🚀

Replace manual WAAPI construction (applyToOptions, mapEasingToNativeEasing,
element.animate) with a single NativeAnimation instantiation that handles
spring-to-linear() easing conversion, keyframe commitment, and cleanup
internally.

https://claude.ai/code/session_01L9oKDA2qPgTAzt5MYPLUbU
@claude
Copy link

claude bot commented Feb 8, 2026

PR Review: Hardware Acceleration Support for useSpring via WAAPI

I've reviewed this PR and overall it's a well-architected feature that brings hardware acceleration to spring animations. Here's my detailed feedback:


✅ Strengths

  1. Clean Architecture: The accelerate config propagation through useSpringuseTransformbindToMotionValue is elegant and maintains separation of concerns.

  2. Comprehensive Test Coverage: Excellent test coverage for both the accelerate propagation logic and edge cases (clamp, function transforms, etc.).

  3. Proper Cleanup: Memory management looks good with cleanup handlers in VisualElement.ts:598-604.

  4. Progressive Enhancement: Properly feature-detects supportsLinearEasing() and gracefully degrades.


🔍 Issues & Concerns

1. Type Safety Issue (VisualElement.ts:585)

keyframes: accel.keyframes as string[],

Problem: AccelerateConfig.keyframes is typed as AnyResolvedKeyframe[] (string | number), but it's being cast to string[]. This is unsafe when values are numeric (e.g., opacity: 0 to 1).

Impact: Runtime type mismatch could cause WAAPI to fail silently or throw.

Fix: Either:

  • Update NativeAnimationOptions to accept AnyResolvedKeyframe[]
  • Or convert numbers to strings in the accelerate config setup
  • Or remove the cast and let TypeScript enforce proper types

@claude
Copy link

claude bot commented Feb 8, 2026

2. Potential Race Condition (follow-value.ts:117-131)

value.attach((v, set) => {
    // ... animation setup
}, stopAnimation)

// accelerate config set AFTER attach
value.accelerate = {
    factory: spring,
    // ...
}

Problem: The accelerate config is set after value.attach(). If the attach callback fires synchronously (e.g., immediate value change), the animationStart event in VisualElement could fire before value.accelerate is defined.

Impact: First animation might not be hardware-accelerated.

Fix: Set value.accelerate before calling value.attach().

3. Missing Validation (VisualElement.ts:579-580)

const accel = value.accelerate
if (!accel) return

Problem: No validation that required properties exist on accel. If accel.keyframes, accel.factory, etc. are undefined, NativeAnimation constructor will fail.

Impact: Potential runtime errors.

Fix: Add runtime validation or use TypeScript non-null assertions more carefully.

@claude
Copy link

claude bot commented Feb 8, 2026

4. Performance: Unnecessary Animation Creation (VisualElement.ts:576-590)

removeAnimationStart = value.on("animationStart", () => {
    activeAcceleration?.stop()
    // create new NativeAnimation every time
})

Problem: Creates a new WAAPI animation on every animationStart event, even when spring parameters haven't changed.

Impact: Minor performance overhead, potential jank on rapid value changes.

Suggestion: Consider caching the NativeAnimation and only recreating when accelerate config changes.

5. Code Style Violation (follow-value.ts:123)

const { type, onUpdate, onComplete, onPlay, onRepeat, onStop, ...springOptions } = options as any

Problem:

  • Using as any defeats type safety (against project guidelines per CLAUDE.md)
  • These properties are already excluded from FollowValueOptions type, so this destructuring is redundant

Fix: Just use options directly since the type already excludes those properties:

value.accelerate = {
    factory: spring,
    options,  // Already the right type
    times: [0, 1],
    keyframes: [0, 1],
    ease: "linear",
}

@claude
Copy link

claude bot commented Feb 8, 2026

6. Inconsistent Behavior: clamp Default (use-transform.ts:228-233)

Question: Why is accelerate only propagated when clamp !== false?

The comment says "clamp enabled (default)" but:

  • Function-based transforms never get accelerate (even with implicit clamp)
  • This creates an inconsistency where useTransform(x, v => v) and useTransform(x, [0,1], [0,1]) have different acceleration behavior

Suggestion: Document this decision more clearly, or consider propagating for function transforms too.


🎯 Minor Issues

  1. Magic Numbers: follow-value.ts:127-128 uses hardcoded [0, 1] for times/keyframes. Consider constants for clarity.

  2. Missing Edge Case Test: No test for what happens when accelerate is propagated through multiple chained transforms (A → B → C).

  3. Documentation: The AccelerateConfig interface in value/index.ts:58-63 lacks JSDoc explaining when/how it's used.

@claude
Copy link

claude bot commented Feb 8, 2026

🛡️ Security & Performance

  • ✅ No security concerns
  • ✅ No obvious performance regressions
  • ⚠️ Minor: Creating WAAPI animations on every animationStart could add up with many animated values

📋 Recommendations

Critical (should fix before merge):

  1. Fix type safety issue with keyframes cast
  2. Fix race condition by setting accelerate before attach

Important (should fix):
3. Add validation for accelerate config properties
4. Remove as any type assertion
5. Add JSDoc to AccelerateConfig

Nice to have:
6. Add test for chained transforms
7. Consider caching NativeAnimation instances
8. Document clamp behavior more clearly


Summary

This is solid work that adds an important optimization. The main concerns are around type safety and a potential race condition. With those fixes, this would be ready to merge.

Great job on the test coverage and architectural design! 🚀


Reviewed using repository guidelines from CLAUDE.md

@mattgperry mattgperry closed this Feb 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants