Add scroll timeline acceleration via useScroll + useTransform

[mattgperry]

Summary

• Add AccelerateConfig type and accelerate property to MotionValue to carry scroll timeline metadata
• Set accelerate on scrollXProgress/scrollYProgress in useScroll with a factory closure that defers ref resolution to mount time
• Propagate accelerate through range-mapping useTransform calls, mapping input/output ranges to WAAPI times/keyframes
• In bindToMotionValue, create a NativeAnimation attached to the scroll timeline when the value has accelerate metadata and the CSS property is acceleratable (opacity, clipPath, filter)

This bypasses the JS per-frame pipeline (motionValue.set → onChange → build → renderInstance) for scroll-driven animations on supported properties, running them entirely on the compositor thread via WAAPI ScrollTimeline.

Test plan

• Verify in Chrome DevTools Animations panel that a scroll-linked WAAPI animation appears for opacity
• Verify in Performance tab that no JS runs per-frame for the opacity update
• Verify fallback: non-accelerated properties (e.g. transform components) still use the JS onChange path
• Existing tests pass: cd packages/framer-motion && yarn test-client (88 suites, 739 tests, 0 failures)

🤖 Generated with Claude Code

[claude]

Pull Request Review: Scroll Timeline Acceleration

Summary

This PR adds native WAAPI scroll timeline acceleration for scroll-driven animations, bypassing the JavaScript per-frame pipeline for supported properties (opacity, clipPath, filter). The implementation is well-designed and follows a clean architecture pattern.

✅ Strengths

1. Clean Architecture: The acceleration metadata pattern using AccelerateConfig is elegant and keeps concerns well-separated
2. Performance First: Offloading to compositor thread via WAAPI ScrollTimeline is the right approach for smooth scroll animations
3. Graceful Fallback: Non-accelerated properties continue using the JS path, maintaining compatibility
4. Type Safety: Good use of TypeScript interfaces and proper type propagation through the transform chain
5. Factory Pattern: Using a factory closure to defer ref resolution until mount time is smart (use-scroll.ts:37-43, 50-56)

🔍 Issues & Concerns

1. Type Mismatch in use-scroll.ts (Critical)

Lines 36-61: Type incompatibility between AnimationPlaybackControls and expected AnimationPlaybackControlsWithThen

// Current code
values.scrollXProgress.accelerate = {
    factory: (animation: AnimationPlaybackControls) => // ❌ Wrong type
        scroll(animation, { ... }),
    // ...
}

The AccelerateConfig interface (motion-dom/src/value/index.ts:63) expects:

factory: (animation: AnimationPlaybackControlsWithThen) => VoidFunction

But scroll() function accepts AnimationPlaybackControls (render/dom/scroll/index.ts:8). This type mismatch could cause runtime issues since AnimationPlaybackControlsWithThen extends the base interface with a then method.

Recommendation: Update the import and type annotation:

import { AnimationPlaybackControlsWithThen, motionValue } from "motion-dom"

values.scrollXProgress.accelerate = {
    factory: (animation: AnimationPlaybackControlsWithThen) =>
        scroll(animation, { ... }),
    // ...
}

2. Potential Memory Leak (High Priority)

VisualElement.ts:561-564: The cleanup function properly cancels animations, but there's a concern about the subscription management flow.

const cleanup = factory(animation)

this.valueSubscriptions.set(key, () => {
    cleanup()
    animation.cancel()
})

Questions:

• What happens if bindToMotionValue is called multiple times for the same key? The previous subscription is cleaned up at line 538, but is this always guaranteed to happen before a new animation is created?
• Should we verify that factory returns a valid cleanup function (not undefined)?

Recommendation: Add defensive checks:

const cleanup = factory(animation)

this.valueSubscriptions.set(key, () => {
    cleanup?.()
    animation.cancel()
})

3. Missing Validation in use-transform.ts (Medium Priority)

Lines 220-233: The acceleration propagation logic has several unchecked assumptions:

if (
    !Array.isArray(input) &&
    typeof inputRangeOrTransformer !== "function" &&
    Array.isArray(outputRangeOrMap) &&
    (input as MotionValue).accelerate &&
    options?.clamp !== false
) {
    result.accelerate = {
        ...(input as MotionValue).accelerate!,
        times: inputRangeOrTransformer as number[], // ❌ Unsafe cast
        keyframes: outputRangeOrMap,
        ...(options?.ease ? { ease: options.ease } : {}),
    }
}

Concerns:

• The cast inputRangeOrTransformer as number[] is unsafe - TypeScript can't guarantee this is actually a number array
• No validation that inputRangeOrTransformer.length === outputRangeOrMap.length (WAAPI requires matching lengths)
• The condition options?.clamp !== false means clamping is opt-out, but accelerated animations might have different clamping behavior than JS animations

Recommendation: Add runtime validation:

if (
    !Array.isArray(input) &&
    typeof inputRangeOrTransformer !== "function" &&
    Array.isArray(outputRangeOrMap) &&
    Array.isArray(inputRangeOrTransformer) && // ✅ Proper check
    (input as MotionValue).accelerate &&
    options?.clamp !== false
) {
    if (inputRangeOrTransformer.length !== outputRangeOrMap.length) {
        // Handle error or skip acceleration
        return result
    }
    
    result.accelerate = {
        ...(input as MotionValue).accelerate!,
        times: inputRangeOrTransformer,
        keyframes: outputRangeOrMap,
        ...(options?.ease ? { ease: options.ease } : {}),
    }
}

4. Test Coverage Gap (Medium Priority)

The test plan mentions manual verification but lacks automated tests:

Missing tests:

• Unit test for AccelerateConfig propagation through useTransform
• Unit test for cleanup when motion values are unbound
• E2E test verifying WAAPI animation is created (can check via element.getAnimations())
• Test for edge case: what happens when container/target refs change after acceleration is set up?

Recommendation: Add tests similar to:

describe("scroll timeline acceleration", () => {
    it("should propagate accelerate config through useTransform", () => {
        const scrollProgress = motionValue(0)
        scrollProgress.accelerate = { /* config */ }
        
        const opacity = useTransform(scrollProgress, [0, 1], [0, 1])
        
        expect(opacity.accelerate).toBeDefined()
        expect(opacity.accelerate?.times).toEqual([0, 1])
    })
    
    it("should create WAAPI animation when binding accelerated value", () => {
        // Test NativeAnimation creation in VisualElement
    })
})

5. Documentation & Error Handling (Low Priority)

Missing documentation:

• No JSDoc comments explaining when/why acceleration is used
• No inline comments explaining the defer-to-mount pattern in use-scroll.ts
• No explanation of why clamp !== false is required for acceleration

Edge cases not handled:

• What if acceleratedValues.has(key) is false but value.accelerate is set? Silent fallback to JS path is fine, but could be logged
• What if this.current is not an HTMLElement but has accelerate metadata?

Recommendation: Add documentation and consider development-mode warnings:

if (value.accelerate) {
    if (!acceleratedValues.has(key)) {
        warnOnce(false, `Property "${key}" has accelerate metadata but is not acceleratable`)
    }
    if (!(this.current instanceof HTMLElement)) {
        warnOnce(false, `Accelerate requires HTMLElement, got ${typeof this.current}`)
    }
}

🎯 Performance Considerations

Positive:

• Excellent use of compositor-thread animations for scroll-driven effects
• Avoids unnecessary per-frame JS execution for supported properties
• Factory pattern defers work until necessary

Potential concerns:

• Creating a new NativeAnimation instance for each accelerated property could be expensive if many properties are bound. Consider object pooling if this becomes a bottleneck.
• The spread operator in use-transform.ts:227-228 creates new objects on every transform - this is fine for hook initialization but worth noting

🔒 Security Considerations

No security concerns identified. The implementation doesn't introduce XSS vectors or allow arbitrary code execution.

📋 Code Style & Best Practices

Follows CLAUDE.md guidelines:

• ✅ Uses optional chaining appropriately
• ✅ Uses interface for type definitions
• ✅ No default exports
• ✅ Arrow callbacks
• ✅ Strict equality

Minor style notes:

• Consistent with existing codebase patterns
• Good variable naming (factory, cleanup, result)

🎬 Recommended Actions

Must Fix (Blocking)

1. Fix type mismatch in use-scroll.ts (AnimationPlaybackControls → AnimationPlaybackControlsWithThen)
2. Add array type guard in use-transform.ts before casting

Should Fix (High Priority)

3. Add defensive null check for cleanup function
4. Add validation for matching input/output range lengths
5. Add automated tests for acceleration propagation

Nice to Have

6. Add JSDoc comments explaining the acceleration system
7. Add development-mode warnings for misused acceleration
8. Consider adding E2E test verifying compositor-thread execution

Summary

This is a solid feature implementation with good architecture. The main concerns are type safety issues that could cause runtime errors and missing test coverage. Once the type mismatch and validation issues are addressed, this will be production-ready.

Overall Assessment: Approve with required changes ✅

Great work on the compositor-thread optimization! This will significantly improve scroll animation performance.

[claude]

Pull Request Review: Scroll Timeline Acceleration

Summary

This PR adds native WAAPI scroll timeline acceleration for useScroll + useTransform by attaching acceleration metadata to MotionValues. This is a significant performance optimization that offloads scroll-driven animations to the compositor thread.

Code Quality & Architecture

Strengths ✅

1. Clean Factory Pattern: The deferred ref resolution using a factory closure in useScroll is elegant and correctly handles the refs not being available during hook initialization.

2. Metadata Propagation: The AccelerateConfig interface cleanly carries scroll timeline metadata through the value chain from useScroll → useTransform → bindToMotionValue.

3. Explicit Conditions: The conditions in useTransform for propagating acceleration are appropriately strict and well-documented.

4. Documentation: The CLAUDE.md updates provide excellent guidance for future testing of scroll timeline features.

Critical Issues 🚨

1. Missing Tests (Blocking)

IMPORTANT: According to CLAUDE.md and the PR's own test plan checklist, tests are mandatory for every feature. This PR currently has NO tests to verify:

• ✗ WAAPI animations are created for acceleratable properties (opacity, clipPath, filter)
• ✗ Non-acceleratable properties fall back to JS (e.g., backgroundColor, transform components)
• ✗ Chained useTransform does NOT accelerate (as documented in CLAUDE.md)
• ✗ Edge cases: refs not yet mounted, container/target options, x/y axis

Required Action: Before merging, you MUST add Cypress E2E tests following the pattern in CLAUDE.md. Tests should verify WAAPI animations exist using element.getAnimations() and check scroll timeline attachment.

2. Transform Property Ambiguity in accelerated-values.ts

Line 8 includes "transform" in the accelerated values set. However:

• WAAPI can accelerate transform as a whole (like transform: translateX(100px))
• But the codebase uses individual transform properties (x, y, scale, etc.) which are not acceleratable
• The condition in VisualElement.ts:542 checks acceleratedValues.has(key), so "x" would NOT match "transform"

Question: Is "transform" intended to be in this set? If not, remove it to avoid confusion.

3. Memory Leak Risk in VisualElement.ts:541-565

The factory cleanup function is called correctly, but add a comment explaining the cleanup behavior when bindToMotionValue is called multiple times for the same key. The existing code at line 536 handles this, but it's not immediately obvious.

Performance Considerations ⚡

Positive

1. Compositor Thread Offload: Excellent performance win for scroll-driven animations
2. Selective Acceleration: Only accelerates when conditions are met, avoiding overhead
3. Factory Pattern: Defers expensive operations until mount time

Potential Issues

1. No Garbage Collection Check: The NativeAnimation instances are created but may not be cleaned up if the element unmounts during the factory execution. Verify this in testing.

Security Concerns 🔒

No security issues identified. The code properly validates inputs and uses browser-native WAAPI safely.

Best Practices & Style

Good ✅

• Follows project conventions (named exports, arrow functions)
• Appropriate use of optional chaining
• Clear variable names

Suggestions 📝

1. useTransform.ts:219-241: The acceleration propagation logic is dense. Consider extracting to a helper function for better readability.

2. use-scroll.ts:35-60: The duplication between X and Y progress acceleration could be extracted to reduce code repetition.

3. Missing JSDoc: Add documentation for the AccelerateConfig interface explaining its purpose and when it's used.

Test Coverage Summary

Current coverage: ❌ 0% (No tests written)

Required coverage:

• Acceleration works for opacity, clipPath, filter
• Fallback to JS for non-acceleratable properties
• Chained useTransform disables acceleration
• Edge cases (unmounted refs, axis selection, container/target options)
• Cleanup on unmount

Recommendations

Must Fix Before Merge 🔴

1. Add comprehensive Cypress E2E tests following CLAUDE.md guidelines
2. Verify and fix the "transform" entry in accelerated-values.ts
3. Test memory cleanup when elements unmount mid-animation

Should Fix 🟡

1. Extract complex conditional logic in useTransform to helper function
2. Add JSDoc comments to AccelerateConfig interface
3. Consider extracting duplicate acceleration config creation in useScroll

Nice to Have 🟢

1. Add inline comments explaining the factory pattern rationale
2. Add a console warning in dev mode when chained transforms are detected

Conclusion

This is a well-architected feature with a clean approach to scroll timeline acceleration. However, it cannot be merged without tests. The CLAUDE.md guidelines are explicit that tests are mandatory for features, and scroll timeline acceleration requires specific E2E testing to verify WAAPI integration.

Once tests are added and the minor issues addressed, this will be an excellent performance improvement for the library.

Status: ❌ Changes Requested (Missing tests)

---

🤖 Generated by Claude Code

[claude]

PR Review: Scroll Timeline Acceleration via useScroll + useTransform

This is an impressive performance optimization that enables scroll-driven animations to run entirely on the compositor thread via WAAPI ScrollTimeline. The implementation is well-architected and follows the codebase conventions outlined in CLAUDE.md.

Strengths

1. Architecture & Design

• Clean separation of concerns: metadata flows through AccelerateConfig without coupling components
• The factory closure pattern elegantly defers ref resolution to mount time (line 37-43 in use-scroll.ts)
• isTransformed flag provides a simple, robust guard against chained transform propagation (line 226 in use-transform.ts)

2. Code Quality

• Follows size optimization principles with concise patterns (optional chaining, no verbose checks)
• Proper cleanup in bindToMotionValue with subscription management (line 561-564 in VisualElement.ts)
• Type safety with AccelerateConfig interface

3. Test Coverage

• E2E tests verify both acceleration (direct transform) and fallback (chained transform) paths
• Test page correctly uses getAnimations() to verify WAAPI animation creation
• CLAUDE.md documentation improvements help future contributors understand testing patterns

Potential Issues & Suggestions

1. Type Safety Concern (High Priority)

In use-scroll.ts:36-61, the accelerate property is assigned with type AccelerateConfig, but the factory expects AnimationPlaybackControlsWithThen while scroll() is typed to accept AnimationPlaybackControls. Check if this type mismatch could cause issues.

2. Memory Leak Risk (Medium Priority)

In VisualElement.ts:559, if factory() throws an error, the animation and cleanup are not registered in valueSubscriptions, potentially leaving the NativeAnimation uncanceled. Consider wrapping in try-catch or ensuring cleanup is registered before calling factory.

3. Edge Cases to Test

• Clamp option: Line 229 checks options?.clamp !== false. What happens with scroll values outside [0,1] range when using non-default input ranges?
• Multiple properties: Test page only checks opacity. Should verify behavior when multiple acceleratable properties (opacity + filter + clipPath) are used simultaneously.
• Unmounting: Verify cleanup when component unmounts mid-scroll.

4. Documentation Gap (Low Priority)

Consider adding a comment in the test file explaining why backgroundColor is expected to not accelerate (Chromium bug referenced in accelerated-values.ts).

5. Performance Consideration

In the test page (scroll-accelerate.tsx:20-31), useEffect with empty dependency array might execute when directRef.current is null on first render. Consider using layout effect or adding refs to dependencies.

6. Type Annotation Consistency

Line 65 in value/index.ts: keyframes: any[] could be more specific. Consider keyframes: AnyResolvedKeyframe[] for consistency.

Security Assessment

No security concerns identified. The implementation properly validates acceleratable properties and handles user input safely.

Performance Impact

Positive:

• Eliminates JS per-frame updates for scroll-driven opacity/filter/clipPath animations
• Reduces main thread pressure during scroll
• Native WAAPI animations benefit from compositor thread acceleration

Considerations:

• Memory overhead per accelerated MotionValue is minimal
• No performance regression for non-accelerated properties

Overall Assessment

This is a high-quality PR that delivers significant performance wins. The architecture is sound, the implementation follows best practices, and test coverage demonstrates thorough consideration of edge cases.

Recommendation: Approve with minor fixes

Priority fixes before merge:

1. Verify type compatibility between AnimationPlaybackControls types in use-scroll.ts
2. Consider error handling in factory() call to prevent memory leaks

Nice-to-have improvements:
3. Add test coverage for multiple simultaneous acceleratable properties
4. Document backgroundColor non-acceleration in test file
5. Strengthen type safety for keyframes property

Code Quality: 5/5
Test Coverage: 4/5
Documentation: 4/5
Performance Impact: 5/5

Great work on this optimization!

[claude]

PR Review: Scroll Timeline WAAPI Acceleration

This is an excellent performance optimization that offloads scroll-driven animations to the compositor thread. The implementation is well-designed overall. Here's my detailed feedback:

---

✅ Strengths

1. Smart architecture: The AccelerateConfig metadata pattern cleanly separates scroll timeline concerns from the core MotionValue logic
2. Deferred ref resolution: Using a factory closure to defer container.current / target.current resolution until mount is the right approach
3. Selective acceleration: Properly checking acceleratedValues.has(key) prevents unsupported properties from attempting WAAPI
4. Transform propagation: The isTransformed flag prevents chained useTransform calls from incorrectly accelerating
5. Good test coverage: Both E2E tests verify acceleration behavior and fallback scenarios

---

🐛 Potential Issues

1. Memory leak in useScroll (High Priority)

Location: packages/framer-motion/src/value/use-scroll.ts:35-60

The accelerate config is set on every render, but MotionValues are created once via useConstant. This means the config references container?.current and target?.current from the first render, not current values.

Problem: If refs update after mount, the scroll timeline will still reference the stale elements.

Suggested fix: Move the accelerate assignment into a useEffect or useLayoutEffect:

useLayoutEffect(() => {
    values.scrollXProgress.accelerate = {
        factory: (animation) => scroll(animation, {
            ...options,
            axis: "x",
            container: container?.current || undefined,
            target: target?.current || undefined,
        }),
        times: [0, 1],
        keyframes: [0, 1],
        ease: (v) => v,
        duration: 1,
    }
    // ... same for scrollYProgress
}, [container, target, options]) // Careful: options may need memoization

---

2. Missing cleanup in VisualElement (Medium Priority)

Location: packages/motion-dom/src/render/VisualElement.ts:541-565

The valueSubscriptions.set(key, cleanup) is good, but there's no guarantee the subscription map is cleared when the element unmounts if the value persists.

Concern: If a MotionValue with accelerate is reused across component instances, old NativeAnimations might leak.

Suggested approach:

• Ensure unbindFromMotionValue (or equivalent) properly cleans up subscriptions
• Consider adding a comment explaining the lifecycle

---

3. Type mismatch for ease in AccelerateConfig (Low Priority)

Location: packages/motion-dom/src/value/index.ts:66

ease?: EasingFunction | EasingFunction[] allows an array, but useScroll always sets a single function:

ease: (v: number) => v,

And useTransform conditionally adds:

...(options?.ease ? { ease: options.ease } : {})

Question: Where does the array case come from? If it's for future multi-segment easing, consider documenting it. If unused, simplify to ease?: EasingFunction.

---

4. Transform chaining edge case (Low Priority)

Location: packages/framer-motion/src/value/use-transform.ts:220-237

The check \!inputAccelerate.isTransformed correctly prevents double-transformation, but consider this scenario:

const a = useTransform(scrollYProgress, [0, 1], [0, 100])
const b = useTransform(a, (v) => v * 2) // custom function
const c = useTransform(b, [0, 200], [0, 1])

• a gets accelerate (✅)
• b loses accelerate because it's a function transformer (expected)
• c doesn't get accelerate because b.accelerate is undefined (✅)

This is correct! Just wanted to verify the logic handles this.

---

🎨 Code Style & Best Practices

1. Consistency with CLAUDE.md: ✅ Uses optional chaining, arrow functions, and avoids var
2. File size conscious: ✅ No unnecessary abstractions
3. Named exports: ✅ Consistent with repo style

---

🧪 Test Coverage

Current tests are good:

• ✅ Verifies WAAPI animation creation for accelerated properties
• ✅ Verifies non-accelerated properties fall back to JS
• ✅ Verifies chained transforms don't accelerate

Suggested additions:

1. Test that refs work when passed after mount:

   const containerRef = useRef(null)
   const { scrollYProgress } = useScroll({ container: containerRef })
   // Render without ref, then update it

2. Test cleanup: Verify that unmounting doesn't leak animations

3. Test multiple range segments:

   useTransform(scrollYProgress, [0, 0.3, 0.7, 1], [0, 50, 50, 100])

---

🔒 Security Concerns

None identified. This feature doesn't introduce XSS, injection, or DOM clobbering risks.

---

⚡ Performance Considerations

1. Excellent: Offloading to compositor thread is the whole point ✅
2. Minor concern: Setting accelerate on every render (even though values are constant) — see Issue Initial API (WIP) #1 above
3. Question: What's the memory overhead of carrying accelerate metadata on every MotionValue? (Probably negligible, but worth profiling in large apps with many scroll-driven animations)

---

📝 Documentation

The PR description is excellent! Consider adding:

• Inline comment in VisualElement.ts explaining when the WAAPI path is taken vs. JS path
• JSDoc on AccelerateConfig interface explaining the factory pattern

---

🎯 Final Recommendation

Approve with minor changes. The core architecture is solid, but please address:

1. ✅ Fix the ref lifecycle issue in useScroll (High priority)
2. ⚠️ Verify cleanup behavior in VisualElement (Medium priority)
3. 💡 Consider clarifying the ease array type (Low priority)

Excellent work on this optimization! The test-driven approach and thoughtful design are much appreciated.

---

Generated with Claude Code

[claude]

PR Review: Scroll Timeline Acceleration via useScroll + useTransform

Summary

This PR implements native WAAPI scroll timeline acceleration for scroll-driven animations, bypassing the JavaScript per-frame pipeline for supported CSS properties (opacity, clipPath, filter). Great performance optimization!

✅ Strengths

1. Excellent test coverage - Both Cypress E2E tests and test pages included
2. Smart deferred ref resolution - The factory closure pattern properly handles ref timing
3. Proper fallback handling - Non-acceleratable properties still use JS onChange path
4. Updated documentation - CLAUDE.md improvements are helpful
5. Focused scope - Only accelerates when safe (direct transforms, clamped ranges)

🐛 Issues Found

Critical: Debug Code Left In

Location: packages/motion-dom/src/render/VisualElement.ts:550

console.log(name, keyframes, times, ease)

Issue: Debug console.log statement must be removed before merging. This will pollute user consoles in production.

Impact: High - affects all users of scroll acceleration
Fix: Delete line 550

Medium: Potential undefined reference in console.log

Location: Same line 550

console.log(name, keyframes, times, ease)

Issue: Variable name is used but appears to be undefined in this scope. Should be key instead.
Fix: Even though this line should be deleted, it suggests a copy-paste error.

🤔 Code Quality Concerns

1. Mutation of motion values in hook

Location: packages/framer-motion/src/value/use-scroll.ts:36-61

values.scrollXProgress.accelerate = {
    factory: (animation: AnimationPlaybackControls) => ...
}

Concern: Mutating MotionValue instances returned from useConstant on every render. While useConstant ensures the same object reference, this mutation pattern is unconventional.

Question: Should this be inside useConstant or wrapped in useEffect? The current approach works but mutates the same object repeatedly on re-renders when container/target refs haven't changed.

Suggestion:

const values = useConstant(() => {
    const vals = createScrollMotionValues()
    vals.scrollXProgress.accelerate = { /* config */ }
    vals.scrollYProgress.accelerate = { /* config */ }
    return vals
})

However, this won't work because container?.current isn't available at init time. Consider using useMemo or documenting why repeated mutation is safe.

2. Closure captures stale refs

Location: packages/framer-motion/src/value/use-scroll.ts:37-43

factory: (animation: AnimationPlaybackControls) =>
    scroll(animation, {
        ...options,
        axis: "x",
        container: container?.current || undefined,
        target: target?.current || undefined,
    }),

Concern: The closure captures container and target refs. When these change, the accelerate config remains with the old closure. This might work because ref objects are stable, but it's fragile.

Question: What happens if someone passes a different ref object on re-render? The old accelerate factory would still reference the old ref.

3. Type safety: AccelerateConfig interface

Location: packages/motion-dom/src/value/index.ts:62-68

export interface AccelerateConfig {
    factory: (animation: AnimationPlaybackControlsWithThen) => VoidFunction
    times: number[]
    keyframes: any[]  // ⚠️
    ease?: EasingFunction | EasingFunction[]
    duration: number
    isTransformed?: boolean
}

Issue: keyframes: any[] loses type safety. Should be AnyResolvedKeyframe[] or at least unknown[].

4. Missing null check safety

Location: packages/motion-dom/src/render/VisualElement.ts:548-549

const { factory, keyframes, times, ease, duration } = value.accelerate

Concern: Destructuring assumes all properties exist. If ease or duration are missing, this could cause issues. Add validation or make optional properties explicit.

🎯 Test Coverage Analysis

Good:

• ✅ Tests verify WAAPI animation creation
• ✅ Tests verify non-acceleratable properties aren't accelerated
• ✅ Tests verify chained transforms don't accelerate

Missing:

• ❌ No test for scrollX/scrollXProgress (only Y axis tested)
• ❌ No test verifying the actual scroll-linked animation behavior (just checks getAnimations().length)
• ❌ No test for edge cases: empty ranges, single keyframe, mismatched array lengths
• ❌ No test for options?.ease propagation through useTransform

Suggestion: Add a test that actually scrolls and verifies opacity changes without JS execution (using Performance API or checking frame callbacks aren't called).

🔒 Security & Performance

Performance:

✅ Excellent - Main goal achieved: moves scroll animations to compositor thread

Memory:

⚠️ Minor concern - Each scroll-driven motion value creates a NativeAnimation instance. Ensure cleanup happens correctly when components unmount (looks good: cleanup is registered in valueSubscriptions).

Security:

✅ No security concerns identified

📋 Recommendations

Must fix before merge:

1. Remove console.log on line 550 of VisualElement.ts

Should fix:

2. Clarify the mutation pattern in useScroll - either document why it's safe or refactor
3. Change keyframes: any[] to keyframes: AnyResolvedKeyframe[]
4. Add validation for required AccelerateConfig properties

Consider:

5. Add tests for X-axis scrolling
6. Add tests for actual scroll behavior (not just animation creation)
7. Document the limitation that chained transforms don't accelerate
8. Consider warning users when acceleration is skipped (e.g., options?.clamp === false)

🎓 Learning & Best Practices

According to CLAUDE.md:

• ✅ Tests were written (Cypress E2E)
• ✅ Small file size prioritized (concise patterns)
• ✅ Used optional chaining appropriately
• ⚠️ Debug code present (violates "minimize output bytes")

Verdict

Overall: Good implementation with one critical fix needed

The core implementation is solid and achieves the performance goals. The main blocker is the debug console.log. After removing it, this is ready to merge with the suggestions considered for follow-up work.

Estimated risk: Low (after removing console.log)
Innovation: High - elegant use of factory pattern for deferred ref resolution

---

🤖 Review generated by Claude Code

[claude]

PR Review: Scroll Timeline Acceleration via useScroll + useTransform

This PR implements a sophisticated optimization that accelerates scroll-driven animations by leveraging WAAPI ScrollTimeline when possible, bypassing the JavaScript per-frame pipeline for supported CSS properties.

✅ Code Quality & Implementation

Strong Points:

• Clean architecture: The metadata-passing approach via AccelerateConfig is elegant and maintains separation of concerns
• Progressive enhancement: Gracefully falls back to JS-driven animations for unsupported properties
• Proper cleanup: Memory management looks solid with cleanup functions in valueSubscriptions
• Test coverage: E2E tests verify both accelerated and non-accelerated paths

⚠️ Potential Issues & Concerns

1. Critical: Stale Ref Capture in Factory Closures

values.scrollXProgress.accelerate = {
    factory: (animation: AnimationPlaybackControls) =>
        scroll(animation, {
            ...options,
            axis: "x",
            container: container?.current || undefined,  // ⚠️ Captured at hook call time
            target: target?.current || undefined,        // ⚠️ Captured at hook call time
        }),
    // ...
}

Problem: The factory closures capture container?.current and target?.current at useScroll call time (lines 41-42, 54-55), which is typically before refs are populated. This differs from the regular scroll path (lines 85-86) which reads refs at mount time.

Impact:

• If refs aren't hydrated yet, the factory will always pass undefined
• When bindToMotionValue calls factory(animation) later, it uses stale/empty refs
• Acceleration may silently fail or use wrong scroll containers

Recommendation: The factory should capture the refs by reference, not by value:

factory: (animation: AnimationPlaybackControls) =>
    scroll(animation, {
        ...options,
        axis: "x",
        container: container?.current || undefined,  // Still evaluates at factory call time
        target: target?.current || undefined,
    })

Wait, this already does that. Let me reconsider...

Actually, the factory is a function that gets called later in bindToMotionValue (VisualElement.ts:560). At that point, refs should be hydrated. The closure captures the container and target RefObjects (not their .current values), so when the factory executes, it reads the current ref values. This should work correctly. ✅

2. Edge Case: Chained Transform Propagation Logic

if (
    inputAccelerate &&
    \!inputAccelerate.isTransformed &&  // ⚠️ Only transforms once
    typeof inputRangeOrTransformer \!== "function" &&
    Array.isArray(outputRangeOrMap) &&
    options?.clamp \!== false
) {
    result.accelerate = {
        ...inputAccelerate,
        isTransformed: true,  // Prevents further chaining
    }
}

Analysis: The isTransformed flag prevents chained transforms (as shown in the test). This is intentional and correct—WAAPI can't express arbitrary transform chains. However:

Minor concern: The condition options?.clamp \!== false means that if a user explicitly sets clamp: false, acceleration is disabled. This is correct (WAAPI would naturally clamp with keyframe times), but there's no warning or documentation about this limitation.

Recommendation: Consider adding a dev-mode warning when acceleration is disabled due to clamp: false, or document this in JSDoc/types.

3. Missing Edge Case Tests

The Cypress tests cover:

• ✅ Direct transform acceleration
• ✅ Chained transform fallback
• ✅ Non-acceleratable properties (backgroundColor)

Missing coverage:

• ❌ Scroll with custom container ref
• ❌ Scroll with target ref
• ❌ Multiple transforms from same scroll value
• ❌ Transform with clamp: false
• ❌ Transform with custom ease options
• ❌ Component unmount/cleanup verification
• ❌ Ref changes after mount (edge case)

Recommendation: Add unit tests or E2E tests for these scenarios, especially custom ref handling since that's the most complex part.

4. Type Safety Concern

const { factory, keyframes, times, ease, duration } = value.accelerate

All these values are pulled from the AccelerateConfig without validation. While TypeScript ensures they exist, runtime validation might help in edge cases.

Low priority: Consider adding dev-mode assertions that validate the accelerate config structure.

5. Performance: Redundant Acceleration Setup

In useScroll, the accelerate config is set every render:

const values = useConstant(createScrollMotionValues)  // Only runs once

values.scrollXProgress.accelerate = { /* ... */ }  // Runs every render ⚠️
values.scrollYProgress.accelerate = { /* ... */ }  // Runs every render ⚠️

Impact: On every render, new factory closures are created and assigned. While not a major leak, this is unnecessary allocation.

Recommendation: Move the accelerate assignment into createScrollMotionValues or use useMemo/useEffect to set it once:

useEffect(() => {
    values.scrollXProgress.accelerate = { /* ... */ }
    values.scrollYProgress.accelerate = { /* ... */ }
}, [container, target, /* ...dependencies */])

Or even better, create the accelerate configs in useConstant:

const values = useConstant(() => {
    const vals = createScrollMotionValues()
    vals.scrollXProgress.accelerate = { /* ... */ }
    vals.scrollYProgress.accelerate = { /* ... */ }
    return vals
})

But wait—the factory closures need to capture the current options, container, and target refs, which may change. So this would need to be in a useEffect with proper dependencies. Actually, looking at the existing code, the start callback has this same issue and uses useCallback with dependencies. The accelerate config should follow the same pattern.

🔒 Security

No security concerns identified. The code doesn't:

• Accept user-controlled selectors
• Inject CSS/HTML
• Execute arbitrary code
• Introduce XSS vectors

🚀 Performance

Pros:

• ✅ Major win: Offloads scroll-driven animations to compositor thread
• ✅ Eliminates JS per-frame updates for accelerated properties
• ✅ Should significantly improve scroll-linked animation performance

Cons:

• ⚠️ Creates new factory closures on every render (see issue Initial event setup #5 above)
• Minor: Extra NativeAnimation instance per accelerated property

📋 Best Practices Alignment

CLAUDE.md Compliance:

• ✅ E2E tests created in correct locations
• ✅ Test page follows naming convention
• ✅ Uses time.now() pattern (not directly visible but likely in animation code)
• ✅ No default exports
• ✅ Named exports used throughout
• ⚠️ Could use more thorough test coverage (per "Be thorough - I am at risk of losing my job")

🎯 Recommendations Summary

Must Fix:

1. Move accelerate config assignment to avoid creating new closures every render (use useEffect or similar)

Should Fix:
2. Add more comprehensive E2E tests for ref handling, clamp, ease, cleanup
3. Add dev-mode warning when acceleration is disabled due to clamp: false

Nice to Have:
4. Add JSDoc comments to AccelerateConfig explaining limitations (no chaining, clamp requirement)
5. Add runtime validation of accelerate config in dev mode
6. Document the feature in user-facing docs (if not already planned)

✨ Overall Assessment

This is a high-quality optimization with a clean design. The metadata-passing approach is elegant, and the implementation follows the codebase patterns well. The main concern is the potential performance regression from creating closures on every render. With the recommended fixes, this would be production-ready.

Score: 8/10 - Solid implementation, needs minor refinement around closure creation and test coverage.

---

Great work on this performance optimization! The scroll timeline acceleration will provide real benefits to users. 🎉

[claude]

PR Review: Scroll Timeline Acceleration via useScroll + useTransform

Summary

This PR adds WAAPI scroll timeline acceleration for scroll-driven animations, allowing supported CSS properties to animate on the compositor thread instead of via JavaScript per-frame updates. This is a significant performance optimization for scroll-linked animations.

Code Quality & Architecture ✅

Strengths:

• Clean separation of concerns with the AccelerateConfig interface
• Factory pattern elegantly handles React ref lifecycle
• Graceful fallback for non-acceleratable properties
• Good TypeScript usage

Potential Issues & Concerns

1. Memory Management ⚠️

In VisualElement.ts:543-566, verify that circular references are cleaned up properly when components unmount.

2. Duplicate scroll() Calls ⚠️

The scroll() function is called in two places - in accelerate.factory and in the start callback. This creates two scroll timeline subscriptions.

Recommendation: Consider whether JS scroll updates can be skipped when using WAAPI acceleration, or document why both are needed.

3. Options Dependency Issue ⚠️

Line 93 uses JSON.stringify(options.offset) as a dependency, which is fragile and creates a new string on every render.

4. Missing Error Handling ⚠️

The factory call at VisualElement.ts:560 could throw.

Recommendation: Add try-catch to handle errors from scroll timeline creation for browser compatibility.

Test Coverage Gaps ⭐

Current tests only check the accelerate property existence, not actual WAAPI creation.

Must add:

• Test that verifies actual WAAPI ScrollTimeline creation using element.getAnimations() (per CLAUDE.md guidelines)
• Browser fallback test
• Cleanup/unmount test
• Ref change test

Documentation Missing

• No JSDoc comment explaining AccelerateConfig interface purpose
• No comment explaining why isTransformed prevents chained acceleration
• No comment in useScroll explaining the factory pattern

Final Verdict

This is high-quality work with a clean architecture. Main action items:

Must fix:

• Add test verifying WAAPI ScrollTimeline creation with element.getAnimations()
• Add error handling around factory(animation) call

Should fix:

• Add JSDoc documentation
• Fix JSON.stringify dependency issue
• Optimize or document duplicate scroll subscriptions

Overall: Solid work following codebase conventions. With test improvements and documentation, this will be production-ready! 🚀