Skip to content

Comments

Add useTransform signature with motion values#3466

Merged
mattgperry merged 6 commits intomainfrom
claude/add-usetransform-signature-0BN2J
Jan 9, 2026
Merged

Add useTransform signature with motion values#3466
mattgperry merged 6 commits intomainfrom
claude/add-usetransform-signature-0BN2J

Conversation

@mattgperry
Copy link
Collaborator

Adds a new signature to useTransform that accepts an object map of output ranges and returns an object of motion values with the same keys:

const { opacity, x } = useTransform(inputValue, inputRange, {
opacity: [0, 1],
x: [100, 200]
})

The keys are captured once on first render, ensuring hooks are called in a consistent order across renders. This allows the hook to be safely used within loops.

Adds a new signature to useTransform that accepts an object map of output
ranges and returns an object of motion values with the same keys:

const { opacity, x } = useTransform(inputValue, inputRange, {
  opacity: [0, 1],
  x: [100, 200]
})

The keys are captured once on first render, ensuring hooks are called in a
consistent order across renders. This allows the hook to be safely used
within loops.
@claude
Copy link

claude bot commented Jan 9, 2026

Code Review for PR #3466

Summary

This PR adds a new signature to useTransform that accepts an object map of output ranges, returning an object of motion values with the same keys. This is a valuable addition that simplifies deriving multiple values from a single input.

✅ Strengths

  1. Well-tested: Comprehensive test coverage including edge cases (color interpolation, transform options, stable references, input range changes)
  2. Good documentation: Clear JSDoc comments with examples
  3. Follows existing patterns: Implementation mirrors useCombineMotionValues pattern closely
  4. Type-safe: Proper TypeScript generics ensure type inference works correctly
  5. Consistent API: Fits naturally with existing useTransform overloads

🔍 Potential Issues

Critical: Motion Value Creation Pattern

Location: use-transform.ts:274

The implementation directly instantiates MotionValue with new MotionValue(initialValues[i]), but this differs from the established pattern in the codebase:

// Current PR approach:
const motionValues = useConstant(() =>
    keys.map((_, i) => new MotionValue(initialValues[i]))
)

Issue: Looking at useMotionValue (use-motion-value.ts:26) and useCombineMotionValues (use-combine-values.ts:14), the standard pattern is to use the factory function motionValue(initial) from motion-dom, not direct instantiation.

Recommendation:

import { motionValue } from "motion-dom"

const motionValues = useConstant(() =>
    keys.map((_, i) => motionValue(initialValues[i]))
)

This ensures consistency and may be important for proper initialization (the factory might do additional setup).

Moderate: Dependency Management for Transform Functions

Location: use-transform.ts:259-261

const transformers = keys.map((key) =>
    transform(inputRange, outputMap[key], options)
)

Issue: The transformers array is recreated on every render when inputRange, outputMap, or options change. However, there's no dependency tracking mechanism to ensure the subscription in useIsomorphicLayoutEffect updates when these change.

Scenario: If inputRange or outputMap values change between renders, the old transformers will continue to be used by the subscription callback, but the synchronous updateValues() call will use the new transformers. This could lead to inconsistent behavior.

Example:

const [max, setMax] = useState(200)
const { opacity } = useTransform(x, [0, max], { opacity: [0, 1] })
// When max changes, updateValues uses new transformer
// But the subscription still references old transformers

Recommendation: Either:

  1. Wrap transformers in useMemo with proper dependencies, or
  2. Move transformer creation inside the effect so the subscription always uses current values

Minor: Synchronous Update Timing

Location: use-transform.ts:292

updateValues()

Observation: This synchronous call during render ensures DOM styles are current, matching the pattern in useCombineMotionValues. However, initialValues are computed from inputValue.get() at the time of the useConstant call, then updateValues() is called later in the same render.

Question: Could there be a scenario where inputValue changes between the useConstant invocation and the updateValues() call? This seems unlikely in practice but worth considering.

Recommendation: Consider computing initial values lazily or ensuring they're always current.

Minor: Memory Efficiency

Location: use-transform.ts:311-315

const result: { [key: string]: MotionValue<O> } = {}
for (let i = 0; i < keys.length; i++) {
    result[keys[i]] = motionValues[i]
}
return result

Observation: A new result object is created on every render. While JavaScript engines optimize this well, and the motion values themselves are stable, this could be cached.

Recommendation: Consider using useConstant for the result object:

return useConstant(() => {
    const result: { [key: string]: MotionValue<O> } = {}
    for (let i = 0; i < keys.length; i++) {
        result[keys[i]] = motionValues[i]
    }
    return result
})

🧪 Testing Suggestions

The test suite is excellent, but consider adding:

  1. Cleanup test: Verify that subscriptions are properly cleaned up on unmount
  2. Error handling: What happens with mismatched array lengths between input range and output ranges?
  3. Empty map test: Behavior with {}
  4. Performance test: Verify no unnecessary updates when input value doesn't change

📝 Documentation

The JSDoc is clear and well-written. One minor suggestion: emphasize in the @remarks that keys must remain constant:

* @remarks
*
* This is useful when you want to derive multiple values from a single input value.
* **IMPORTANT**: The keys of the output map must remain constant across renders to comply
* with React's Rules of Hooks.

🎯 Overall Assessment

This is a solid implementation that adds valuable functionality. The critical issue with motion value instantiation should be addressed, and the dependency management concern should be investigated. Once these are resolved, this will be a great addition to the API.

Recommendation: Request changes for the motion value instantiation pattern, and consider addressing the transformer dependency issue before merging.

Reuse the existing useTransform in a loop instead of duplicating logic.
The keys are captured once with useConstant, ensuring hooks are called
in consistent order across renders.
@claude
Copy link

claude bot commented Jan 9, 2026

PR Review: Add useTransform signature with motion values

Overview

This PR adds a new signature to useTransform that accepts an object map of output ranges and returns an object of motion values with the same keys. This is a well-designed feature that improves DX when deriving multiple values from a single input.

Strengths

1. Well-Structured Implementation

  • The detection logic for the output map signature is clean and unambiguous (use-transform.ts:191-194)
  • Using useConstant to capture keys once is the right approach to ensure hook call order consistency
  • The implementation correctly reuses the existing useTransform function internally, avoiding code duplication

2. Comprehensive Test Coverage

  • Tests cover all critical scenarios: initial values, updates, stability across renders, color values, transform options, input range changes, and TypeScript typing
  • The test for stable motion values across renders (use-transform.test.tsx:45-76) is particularly important for React's rules of hooks
  • Good edge case coverage with the key consistency test (use-transform.test.tsx:119-146)

3. Excellent Documentation

  • Clear JSDoc comments with practical examples
  • Explicitly mentions that keys of the output map must remain constant across renders
  • The example code is concise and shows real-world usage

Observations & Suggestions

1. Performance Consideration
In useMapTransform (use-transform.ts:238-255), each key creates a new MotionValue with its own subscription and frame update. For objects with many keys (10+ properties), this could create performance overhead. While acceptable for the common case (2-4 properties), consider documenting this limitation.

2. Rules of Hooks Compliance
The implementation correctly uses useConstant to freeze the keys, making it safe to use in loops. The test at use-transform.test.tsx:119-146 verifies key stability, but users might expect an error or warning when keys change. Consider adding a dev-mode warning if Object.keys(outputMap) differs from the captured keys.

3. Edge Case: Empty Output Map
The implementation safely handles empty objects (returns empty object, no conditional hooks), but this might be confusing. Consider adding validation to throw a helpful error in development mode.

Potential Issues

None identified. The implementation is solid and follows React best practices.

Security Concerns

No security issues identified. The implementation does not introduce any XSS, injection, or other vulnerabilities.

Test Coverage Assessment

Excellent. The test suite covers all critical scenarios including initial value computation, value updates, motion value stability, different value types, transform options, key stability, input range reactivity, and TypeScript type inference.

Recommendations

  1. Merge as-is - The implementation is production-ready
  2. Consider adding dev-mode validation for empty output maps
  3. Consider documenting performance characteristics for large output maps
  4. Consider adding a dev-mode warning when output map keys change across renders

Verdict: Approved

This is a well-crafted feature with excellent test coverage and documentation. The implementation correctly handles React's rules of hooks and provides a valuable DX improvement for common use cases. The suggestions above are minor enhancements, not blockers.

Great work!

@claude
Copy link

claude bot commented Jan 9, 2026

Pull Request Review: Add useTransform signature with motion values

I've reviewed this PR that adds a new signature to useTransform allowing it to accept an object map of output ranges.

Code Quality and Best Practices - STRONG

  • Follows existing code style conventions (named exports, arrow functions, interface usage)
  • Comprehensive JSDoc documentation with clear usage examples
  • Well-structured type definitions with proper generic constraints
  • Consistent with existing useTransform overload patterns

Minor Suggestions:

  1. Line 247-248: The output object is initialized in useConstant and mutated inside the loop. Consider documenting why mutation is safe here.
  2. Line 191-194: The isOutputMap detection logic could benefit from a comment explaining the disambiguation logic.

Potential Bugs - CRITICAL ISSUE FOUND

Rules of Hooks Violation at lines 250-252:

The implementation contains a conditional hook call pattern that violates React's Rules of Hooks. The number of hook calls depends on runtime data (number of keys in outputMap):

for (const key of keys) {
    output[key] = useTransform(inputValue, inputRange, outputMap[key], options)
}

While keys is captured with useConstant, this is fragile because:

  1. Dynamic hook count depends on runtime data
  2. ESLint react-hooks/rules-of-hooks should flag this pattern
  3. Safety relies entirely on user following documentation - no runtime enforcement

Recommended Fix: Add DEV mode assertion that validates Object.keys(outputMap) matches the captured keys on subsequent renders to catch violations early.

Performance Considerations

  1. Each key creates a separate MotionValue subscription chain - consider documenting recommended use case (2-4 values)
  2. When inputRange changes, all transforms recreate interpolation functions (correct but worth noting)
  3. Good: output object created once with useConstant

Security Concerns

None identified - pure computational hook with no external I/O.

Test Coverage - EXCELLENT

184 added lines testing all key scenarios. Suggested additions:

  • Empty object edge case
  • Single key edge case
  • Many keys (10+) stress test
  • Memory leak verification on unmount
  • React 18+ concurrent mode behavior

Additional Observations

  1. Object.keys() iteration order is consistent (ES2015+ insertion order)
  2. TypeScript generics properly capture literal types for autocomplete
  3. Documentation about key stability could be more prominent in code examples

Recommendations

Before merging:

  1. Add DEV mode validation for key stability (critical)
  2. Add ESLint disable comment with justification for the loop pattern
  3. Add comment explaining output object mutation safety
  4. Add empty object edge case test

Nice to have:

  • Performance benchmarks for 5+ key maps
  • Memory leak test

Verdict

Well-implemented feature with excellent tests and documentation. Main concern is the potential Rules of Hooks violation which should be addressed with runtime validation in DEV mode.


Reviewed by Claude Code

@claude
Copy link

claude bot commented Jan 9, 2026

Code Review

This is a well-crafted PR that adds a useful new signature to useTransform for creating multiple transformed motion values from a single input. The implementation is clean and the test coverage is comprehensive.

Strengths

  1. Excellent Test Coverage - The test suite covers all critical scenarios including initial values, dynamic updates, motion value stability across renders, color interpolation, transform options, and edge cases.

  2. Clean API Design - The new signature is intuitive and fits naturally with the existing API.

  3. Proper Hook Safety - The use of useConstant for capturing keys ensures hooks are called in a consistent order across renders, making this safe to use even in loops.

  4. Good Documentation - The JSDoc comments clearly explain the purpose, usage, and constraints of the new API.

  5. Type Safety - The TypeScript signature correctly captures and preserves the keys from the output map.

Observations

Implementation Efficiency: The useMapTransform function calls useTransform for each key in the output map (line 251). This creates multiple subscriptions to the same input value. While this works correctly, it could be slightly less efficient than a single subscription. However, this approach maintains code simplicity, reuses existing well-tested logic, and is unlikely to cause performance issues in practice.

Type Detection: The isOutputMap detection at line 191-194 is clever but does not verify that object values are arrays at runtime. TypeScript types provide compile-time safety, so this is acceptable.

Test Improvement: The test about maintaining keys across renders could be stronger by explicitly verifying that new keys are ignored after initial render.

Edge Case: Consider what happens if outputMap is an empty object.

Summary

No bugs identified! Performance characteristics are good. No security concerns. Code follows repository conventions from CLAUDE.md.

LGTM with minor suggestions! This is a solid PR that adds valuable functionality with excellent test coverage. The implementation is clean, type-safe, and follows project conventions. The suggestions above are optional improvements, not blockers.

- Use scale [0.5, 1.5] so scale(1.5) is visible in transform
- Use opacity [0, 0.5] with clamp:false to test extrapolation
- Simplify stable values test to check output object reference
@claude
Copy link

claude bot commented Jan 9, 2026

Code Review: Add useTransform signature with motion values

Thank you for this PR! This is a well-implemented feature that adds a convenient API for deriving multiple motion values from a single input. Here's my detailed review:


✅ Strengths

  1. Excellent API Design: The new signature is intuitive and reduces boilerplate significantly. Going from multiple useTransform calls to a single call with an object map is a clear improvement.

  2. Proper Hook Safety: The use of useConstant to capture keys on first render is crucial and correctly prevents violations of React's Rules of Hooks. This ensures hooks are always called in the same order across renders.

  3. Comprehensive Test Coverage: The test suite is thorough, covering:

    • Initial value setting
    • Dynamic updates
    • Reference stability
    • Color interpolation
    • Transform options
    • Input range changes
    • Edge case: key stability across renders
    • TypeScript types
  4. Good Documentation: The JSDoc is clear with a practical example showing the intended usage.

  5. Follows Repository Conventions: Uses interface for type definitions, arrow callbacks, and proper naming conventions per CLAUDE.md.


🔍 Observations & Suggestions

1. Type Safety Consideration

The implementation at use-transform.ts:191-194 uses runtime type detection to distinguish between output maps and arrays:

const isOutputMap =
    outputRangeOrMap \!== undefined &&
    \!Array.isArray(outputRangeOrMap) &&
    typeof inputRangeOrTransformer \!== "function"

Potential Issue: This could theoretically match unexpected inputs. For example, if someone passes a non-array object as outputRange to the original signature by mistake, it would be treated as an output map.

Suggestion: Consider adding a more defensive check:

const isOutputMap =
    outputRangeOrMap \!== undefined &&
    \!Array.isArray(outputRangeOrMap) &&
    typeof outputRangeOrMap === 'object' &&
    outputRangeOrMap \!== null &&
    typeof inputRangeOrTransformer \!== "function"

2. Empty Object Edge Case

At use-transform.ts:250-252, if outputMap is an empty object {}, the function will return an empty object but still call useConstant twice. This is harmless but worth noting.

for (const key of keys) {  // Empty loop if keys.length === 0
    output[key] = useTransform(inputValue, inputRange, outputMap[key], options)
}

Suggestion: Consider documenting that outputMap should contain at least one key, or add a warning in development mode.

3. Key Consistency Warning

The test at line 418-442 correctly verifies that keys are captured on first render. However, users might not realize this constraint.

Suggestion: Consider adding a development-only warning if Object.keys(outputMap) differs from the captured keys in subsequent renders. This would help catch misuse early:

if (process.env.NODE_ENV \!== 'production') {
    const currentKeys = Object.keys(outputMap)
    if (currentKeys.length \!== keys.length || 
        \!currentKeys.every((k, i) => k === keys[i])) {
        console.warn(
            'useTransform: outputMap keys changed between renders. ' +
            'Keys are captured on first render and must remain consistent.'
        )
    }
}

4. Test File - Async Consistency

The tests declare functions as async but many don't actually use await. For example:

  • use-transform.test.tsx:312: test("sets initial values", async () => { but no await is used
  • use-transform.test.tsx:355: Similar issue

Suggestion: Remove async from test functions that don't use await, or this is fine if it's a consistent pattern in the codebase.

5. Performance Consideration

The implementation calls useTransform once per key in the map (use-transform.ts:251). Each creates a separate MotionValue with its own subscription. This is the correct approach given the architecture, but for large maps (unlikely in practice), there could be a small overhead.

Note: This is not a concern for typical usage (2-5 keys), just worth documenting if this becomes a performance-critical path.


🛡️ Security & Stability

  • No security concerns: The implementation doesn't handle user input directly or expose vulnerabilities.
  • Memory leaks: Properly handled through existing useTransform cleanup mechanisms.
  • Type safety: Good use of generics to preserve type information through the transformation.

📋 Test Coverage Assessment

The test suite covers:

  • ✅ Core functionality
  • ✅ Reference stability (critical for React performance)
  • ✅ Edge cases (color interpolation, options, input range changes)
  • ✅ TypeScript types compile correctly
  • ⚠️ Missing: Test for empty object {} as outputMap
  • ⚠️ Missing: Test verifying cleanup/unsubscription when component unmounts

Suggestion: Add tests for:

  1. Empty object handling
  2. Unmount cleanup (verify no memory leaks)
  3. Very large outputMap (10+ keys) to ensure no performance regression

🎯 Final Recommendation

✅ Approve with minor suggestions

This is a solid implementation that adds real value to the API. The core logic is sound, properly tested, and follows best practices. The suggestions above are minor improvements for robustness and developer experience, but they're not blockers.

The key strengths:

  • Solves a real DX pain point
  • Maintains backward compatibility
  • Properly handles React hook constraints
  • Well-tested

Great work! 🚀

@claude
Copy link

claude bot commented Jan 9, 2026

Code Review Summary

This PR adds a valuable new signature to useTransform that accepts an object map of output ranges, making it easier to derive multiple motion values from a single input. The implementation is clean and well-tested.

✅ Strengths

  1. Excellent API Design: The new signature is intuitive and addresses a real need - deriving multiple values from a single input without multiple useTransform calls.

  2. Hook Safety: The use of useConstant to capture keys once ensures hooks are called in a consistent order across renders, preventing React hook violations. This is critical for safe usage in loops or conditional logic.

  3. Comprehensive Test Coverage: The test suite covers:

    • Initial value setting
    • Dynamic updates
    • Color interpolation
    • Transform options (clamp)
    • Key stability across renders
    • Input range changes
    • TypeScript typing
  4. Type Safety: The generic <K extends string, O> constraint properly captures and preserves object keys in the return type.

  5. Clean Implementation: The useMapTransform helper is simple and delegates to the existing useTransform logic, maximizing code reuse.

  6. Documentation: Clear JSDoc with helpful examples.

🔍 Minor Observations

  1. Runtime Type Detection (use-transform.ts:191-194): The isOutputMap check is straightforward but could theoretically be confused if someone passes a non-array object with numeric keys. However, this is an edge case and the current implementation is pragmatic.

  2. Key Order Stability: The implementation captures Object.keys(outputMap) which relies on JavaScript's property enumeration order. This is stable in modern JS engines (insertion order for string keys), but worth noting for documentation.

  3. Test Line 97 (use-transform.test.tsx:97): The comment says "Note: In practice, users should not change keys" - this is good defensive programming. The test verifies that keys are captured once and won't change even if the input object changes.

  4. Performance: Each key in the output map creates a separate useTransform subscription. For large maps, this could create many subscriptions. However, this is the expected behavior and acceptable for typical use cases (2-5 keys).

🎯 Suggestions (Optional)

  1. Performance Note in Docs: Consider adding a note in the JSDoc that each output key creates an independent motion value subscription, so this approach is best for a reasonable number of keys (not hundreds).

  2. Empty Object Edge Case: What happens if someone passes an empty object {}? The current implementation would return an empty object with no motion values, which is fine but could be mentioned in tests.

  3. Type Constraint: The signature constrains K extends string, but JavaScript objects can also have symbol keys. This is likely intentional (symbols are rare), but worth considering if it should be documented.

🔒 Security & Performance

  • ✅ No security concerns
  • ✅ No memory leaks (proper cleanup handled by underlying useTransform)
  • ✅ No unnecessary re-renders (keys captured once)
  • ✅ No performance regressions

📋 Test Coverage Assessment

The test coverage is excellent, covering:

  • ✅ Basic functionality
  • ✅ Dynamic updates
  • ✅ Different value types (numbers, colors)
  • ✅ Options passing
  • ✅ Key stability (critical for hook rules)
  • ✅ Input range changes
  • ✅ TypeScript compilation

Minor test gap: Could add a test for an empty output map {} to verify it returns an empty object without errors.

✨ Conclusion

This is a high-quality PR that adds a useful feature with proper implementation, excellent tests, and good documentation. The code follows the repository's conventions (named exports, interfaces over types, arrow callbacks) as specified in CLAUDE.md.

Recommendation: ✅ Approve - Ready to merge with optional minor enhancements above.

Nice work! This will be a helpful addition to the API. 🎉

@mattgperry mattgperry merged commit b23c87b into main Jan 9, 2026
4 checks passed
@mattgperry mattgperry deleted the claude/add-usetransform-signature-0BN2J branch January 9, 2026 15:27
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