Add MotionValueState infrastructure for atomic DOM updates

[mattgperry]

Summary

• Introduces MotionValueState class hierarchy to enable per-value DOM rendering instead of rebuilding all styles every frame
• Adds HTMLMotionValueState for atomic HTML style updates (transform, CSS variables, regular styles)
• Adds SVGMotionValueState for atomic SVG attribute updates
• Adds createMotionValueState() abstract method to VisualElement with implementations in HTML, SVG, Object, and State visual elements
• Enhances base MotionValueState with forEach(), getStatic(), and prevValue tracking

Technical Notes

The state infrastructure is in place but values are currently bound via bindToMotionValue to maintain backward-compatible onUpdate behavior. This prepares the codebase for future atomic update optimizations where:

• Transform props share a computed transform MotionValue
• Each style property gets its own atomic render callback
• DOM writes happen per-value instead of full style rebuilds

Test plan

• All existing tests pass (680 tests)
• CSS variables tests pass
• Variant stagger order tests pass
• onUpdate callback tests pass
• Keyframe transition tests pass

🤖 Generated with Claude Code

[claude]

Code Review: MotionValueState Infrastructure

This is a well-architected refactoring that introduces a solid foundation for atomic DOM updates. The implementation is careful to maintain backward compatibility while setting up infrastructure for future optimizations. Here's my detailed feedback:

---

🎯 Architecture & Design

Strengths:

• Clean abstraction hierarchy: MotionValueState → HTMLMotionValueState → SVGMotionValueState follows a logical progression
• Abstract factory pattern: Using createMotionValueState() as an abstract method allows each VisualElement type to provide its own state implementation
• Computed values pattern: Transform and transformOrigin as computed values that depend on multiple inputs is elegant (lines 81-99 in HTMLMotionValueState.ts)
• Backward compatibility: The infrastructure is in place but currently uses bindToMotionValue to maintain existing behavior - smart incremental approach

Concerns:

• Dual state management: Having both VisualElement.values and VisualElement.state during the transition could be confusing. The deprecation comment helps, but consider documenting the migration path more explicitly.

---

🐛 Potential Issues

1. Memory Leak Risk in removeValue() (VisualElement.ts:710-750)

The logic for stopping owned values has changed:

// Before: value.stop() was called in the subscription cleanup
if (value.owner) value.stop()

// After: Moved to removeValue()
if (value?.owner) {
    value.stop()
}

Concern: The comment says "allows rebinding without stopping owned values," but if a value is rebound (replaced with a new value at the same key), the old value's stop() is only called via removeValue(). However, in setStaticValue() line 723, when existingValue exists, it calls this.removeValue(key) before binding the new value - so this should be fine. But verify that all paths properly clean up old values.

Recommendation: Add a test case that verifies proper cleanup when a MotionValue is replaced at the same key multiple times.

2. Race Condition in Initial Mount (VisualElement.ts:450-456)

this.update(this.props, this.presenceContext)
this.state = this.createMotionValueState()

The state is created AFTER the initial update. This means:

• Initial values use bindToMotionValue
• Subsequent values could use the new state system

Concern: If update() triggers async operations that add values after createMotionValueState() returns, those values might use the new state path unexpectedly. The comment suggests this is intentional for backward compatibility, but edge cases around async updates should be tested.

3. onChange Deduplication Logic (MotionValueState.ts:53-77)

const onChange = (isInitial = false) => {
    const typedValue = shouldUseDefaultValueType
        ? getValueAsType(v, numberValueTypes[name])
        : v
    
    if (\!isInitial && typedValue === prevValue) {
        return
    }

Issue: This uses strict equality (===) for comparison. For object values (like colors), this would fail to deduplicate properly since {r: 255, g: 0, b: 0} \!== {r: 255, g: 0, b: 0}.

Recommendation: Consider a deep equality check for objects, or document that deduplication only works for primitives.

4. Transform Building Uses getLatestValues Callback (HTMLMotionValueState.ts:123-173)

const latestValues = this.getLatestValues?.() ?? this.latest

Concern: This creates a dependency on VisualElement's latestValues from within the state class. While necessary for the transition period, this tight coupling means:

• The state class can't be easily tested in isolation
• Changes to VisualElement.latestValues timing could break transform building

Recommendation: Consider adding integration tests that verify transform building works correctly when values come from different sources (state vs latestValues).

---

⚡ Performance Considerations

Strengths:

• Frame scheduling: Using frame.render() for DOM writes batches updates correctly
• Computed values: Transform calculation only rebuilds when dependencies change (lines 81-96 in HTMLMotionValueState.ts)
• Skip internal values: Correctly skips syncing computed values like "transform" and "transformOrigin" to latestValues (lines 22-24 in HTMLVisualElement.ts)

Concerns:

1. Multiple postRender calls: Both HTMLMotionValueState and SVGMotionValueState schedule frame.postRender(this.notifyUpdate) on every value change (lines 17-19 in HTMLVisualElement.ts). If multiple values change in the same frame, this could schedule the same callback multiple times.

   Recommendation: Consider using a flag or scheduleOnce pattern to ensure onUpdate only fires once per frame.

2. Transform string rebuilding: buildTransformString() iterates all transformPropOrder props (16+ properties) on every transform value change. For animations updating multiple transform properties, this rebuilds the string multiple times per frame.

   Future optimization: Track which transform props changed and only rebuild when necessary.

---

🔒 Security Concerns

No major security issues identified. The code properly:

• Uses String(v) for attribute values (SVGMotionValueState.ts:122)
• Validates property setters exist before using them (SVGMotionValueState.ts:130-140)
• Doesn't use eval or innerHTML

Minor note: CSS variable names from user input are directly set via element.style.setProperty(key, value). While this is safe in modern browsers, ensure CSS variable names are validated earlier in the pipeline if they come from untrusted sources.

---

🧪 Test Coverage

Critical Gap: No dedicated unit tests for the new MotionValueState classes.

Recommendations:

1. Unit tests for MotionValueState:

   • Value change deduplication
   • Cleanup on destroy()
   • forEach() and getStatic() methods

2. Unit tests for HTMLMotionValueState:

   • Transform building with various combinations of transform props
   • CSS variable handling
   • TransformOrigin computation
   • Interaction between computed values and dependencies

3. Unit tests for SVGMotionValueState:

   • Path animation properties (pathLength, pathOffset, pathSpacing)
   • Attribute vs property detection
   • attr* prefix handling
   • isSVGTag conditional logic

4. Integration tests:

   • Value replacement at same key (memory leak verification)
   • Transform values from different sources (state vs latestValues)
   • Multiple values changing in same frame
   • Async value updates during mount

Note: The PR description mentions "All existing tests pass (680 tests)" which is good for regression testing, but doesn't cover the new code paths.

---

📝 Code Quality

Strengths:

• Well-commented, especially around tricky timing issues
• Consistent naming conventions
• Good use of TypeScript types and interfaces
• Follows project conventions (interfaces, named exports, arrow callbacks)

Minor improvements:

1. Inconsistent null checks (HTMLMotionValueState.ts:72-76):

   if (\!element) {
       return super.set(key, value, undefined, undefined, useDefaultValueType)
   }

   The element is typed as HTMLElement | SVGElement | null but checking happens in different methods. Consider consolidating or documenting when null is expected.

2. Magic number (SVGMotionValueState.ts:66):

   frame.render(() => element.setAttribute("pathLength", "1"))

   Consider extracting "1" as a named constant like NORMALIZED_PATH_LENGTH.

3. Type assertion (HTMLMotionValueState.ts:107):

   element.style[key as any] = this.latest[key] as string

   The double assertion suggests type system limitations. Consider using a more specific type or adding a comment explaining why any is necessary.

---

✅ Summary

This is a high-quality refactoring that demonstrates careful attention to backward compatibility and architectural cleanliness. The main concerns are:

1. Test coverage gap - Critical new code paths lack dedicated tests
2. Potential edge cases around value replacement and async timing
3. Minor performance opportunities for onUpdate deduplication

Recommendation: ✅ Approve with suggestions - The infrastructure is solid, but adding unit tests for the new state classes would significantly de-risk future changes.

---

📋 Checklist for Follow-up

• Add unit tests for MotionValueState base class
• Add unit tests for HTMLMotionValueState (especially transform building)
• Add unit tests for SVGMotionValueState (especially path properties)
• Add integration test for value replacement memory handling
• Consider deduplicating postRender(onUpdate) calls
• Document migration path from values Map to state
• Verify onChange deduplication works for object values

Great work on maintaining backward compatibility while introducing significant architectural improvements! 🎉

[claude]

Pull Request Review: Add MotionValueState infrastructure for atomic DOM updates

Summary

This PR introduces a foundational infrastructure for per-value DOM rendering, preparing the codebase for atomic update optimizations. The implementation is well-architected and maintains backward compatibility while setting the stage for future performance improvements.

---

Code Quality & Architecture ✅

Strengths:

• Clean class hierarchy: MotionValueState → HTMLMotionValueState → SVGMotionValueState follows good OOP principles
• Separation of concerns: Each class has a clear responsibility (base state management, HTML rendering, SVG rendering)
• Good documentation: Comments clearly explain the transition strategy and architectural decisions
• Named exports: Follows the repository's style guide (no default exports)
• Type safety: Proper use of TypeScript interfaces throughout

Architecture highlights:

• The abstract createMotionValueState() method in VisualElement allows each visual element type to provide its own state implementation
• The computed value pattern (transform, transformOrigin, stroke-dasharray) is elegant for batching related properties
• Proper lifecycle management with destroy() cleanup

---

Potential Issues & Concerns

1. Memory Leak Risk in HTMLMotionValueState constructor ⚠️

Location: packages/motion-dom/src/effects/HTMLMotionValueState.ts:49-56

The onValueChangeCallback is shadowed by a local property in the constructor, but the parent class also has onValueChangeCallback. This creates potential confusion:

// Parent class (MotionValueState) has:
protected onValueChangeCallback?: (key: string, value: AnyResolvedKeyframe) => void

// HTMLMotionValueState constructor:
protected onValueChangeCallback?: (key: string, value: AnyResolvedKeyframe) => void
constructor(options: HTMLMotionValueStateOptions) {
    super()  // Parent constructor is called but doesn't receive options
    this.onValueChangeCallback = options.onValueChange  // Shadows parent property
}

Recommendation: Pass the callback to the parent constructor:

constructor(options: HTMLMotionValueStateOptions) {
    super({ onValueChange: options.onValueChange })
    // ... rest of initialization
}

2. Duplicate onUpdate callbacks ⚠️

Location: Multiple files

The onUpdate callback is called in three places for transform changes:

1. In the render callback (HTMLMotionValueState.ts:119)
2. In onValueChange callback (HTMLMotionValueState.ts:31)
3. Via scheduleRender() in the visual element (HTMLVisualElement.ts:42)

While the prevValue tracking prevents duplicate callbacks at the MotionValue level, the multiple onUpdate calls might still fire. Consider consolidating or adding guards.

3. Inconsistent initialization timing 🤔

Location: packages/framer-motion/src/render/VisualElement.ts:450-456

The comment explains that state is created AFTER initial update to ensure backward compatibility, but this creates an asymmetry:

• Initial values: use bindToMotionValue path
• Subsequent values: intended to use state (but currently still use bindToMotionValue per line 729)

This works for now but could be confusing during the transition to atomic updates. Consider adding a TODO or more detailed documentation about when this will change.

4. Missing null check in buildTransformString ⚠️

Location: packages/motion-dom/src/effects/HTMLMotionValueState.ts:145

valueIsDefault = parseFloat(value as string) === 0

If value is null or undefined, parseFloat returns NaN, which doesn't equal 0, so this works accidentally. However, it's fragile. Consider:

valueIsDefault = value === null || value === undefined || parseFloat(value as string) === 0

5. Race condition with frame scheduling 🤔

Location: packages/framer-motion/src/render/html/HTMLVisualElement.ts:18

frame.postRender(this.notifyUpdate)

Using postRender for onUpdate while the state uses frame.render(render) for actual DOM updates could cause the callback to fire before the DOM is actually updated in some edge cases. This might be intentional but should be documented.

---

Performance Considerations

Positives ✅

• The benchmark (transform-rebuild-overhead.html) is excellent for measuring the future optimization impact
• Proper use of frame.render() batching for DOM writes
• prevValue tracking prevents unnecessary work

Concerns ⚠️

1. Double work during transition: Currently maintaining both state infrastructure AND bindToMotionValue paths adds overhead. This is acknowledged as temporary.
2. Transform string rebuilding: buildTransformString() still iterates all transform properties on every change. The optimization will come when individual transform properties trigger this selectively.
3. Computed value overhead: Creating computed MotionValues (transform, transformOrigin, stroke-dasharray) adds objects and subscriptions. Ensure these are worth the abstraction cost.

---

Test Coverage

Status: According to the PR description, all 680 existing tests pass ✅

Gap identified: No unit tests specifically for the new MotionValueState classes. While integration tests cover the behavior, consider adding:

• Unit tests for MotionValueState.set() with duplicate value prevention
• Unit tests for HTMLMotionValueState.buildTransformString() edge cases
• Unit tests for SVG path property handling

---

Security Concerns

✅ No security issues identified. The code:

• Properly sanitizes CSS variable names via isCSSVar()
• Uses setAttribute() safely with string coercion
• No user input is directly interpolated into styles

---

Code Style Compliance

✅ Follows repository conventions:

• Uses interface for type definitions
• Named exports only
• Arrow callbacks where appropriate
• No var declarations

---

Recommendations

High Priority

1. Fix the constructor pattern to properly pass callbacks to parent class
2. Add unit tests for the new state classes
3. Document the frame scheduling strategy for onUpdate callbacks

Medium Priority

4. Add null guards in buildTransformString for robustness
5. Consider adding a __DEV__ mode warning if both old and new paths are active
6. Add metrics/logging to the benchmark to quantify the current overhead

Low Priority

7. Consider extracting the computed value pattern into a helper for reuse
8. Add JSDoc to public methods in MotionValueState classes

---

Conclusion

This is a well-designed foundational PR that prepares the codebase for significant performance improvements. The architecture is sound, backward compatibility is maintained, and the transition strategy is clear.

The issues identified are mostly about edge cases and potential confusion during the transition period. None are blockers, but addressing the constructor pattern and adding tests would strengthen the implementation.

Recommendation: Approve with minor revisions ✅

The benchmark addition is particularly valuable for measuring the impact of future optimizations. Great work on the progressive enhancement approach!

[mattgperry]

Closing for now as although interesting, not quite as large of an improvement for such a big change