Fix animateMini crash when element removed from DOM during scroll cancellation

[mattgperry]

Check if element is still connected to the DOM before calling commitStyles()
in NativeAnimation. This prevents the WAAPI commitStyles() method from throwing
when the animation is stopped after the element has been removed from the DOM
(e.g., in disconnectedCallback).

Fixes #3509

[claude]

Code Review: PR #3510

Summary

This PR fixes a crash in animateMini when an element is removed from the DOM during scroll cancellation by adding an isConnected check before calling commitStyles().

---

✅ Positive Aspects

1. Correct Problem Identification

The fix correctly identifies that calling WAAPI's commitStyles() on a disconnected element throws an error. The isConnected check is the standard way to handle this scenario.

2. Minimal, Focused Change

The implementation change is surgical - only adding the necessary guard without over-engineering. This follows the CLAUDE.md guidance to avoid over-engineering solutions.

3. Good Test Coverage

• Test correctly reproduces the bug scenario (element removed before stop())
• Test verifies normal operation still works (element in DOM)
• Both tests follow existing patterns in the codebase (similar to JSAnimation.test.ts)

---

🔍 Potential Issues & Suggestions

1. Missing Test: Verify commitStyles Actually Called

Severity: Medium

The current tests only verify "does not throw", but don't verify that commitStyles() is actually called when the element IS connected. Consider adding a test with a spy:

test("should call commitStyles when element is connected", () => {
    const element = document.createElement("div")
    document.body.appendChild(element)

    const animation = new NativeAnimation({
        element,
        name: "opacity",
        keyframes: [0, 1],
        duration: 1000,
    })

    // Spy on the underlying WAAPI animation's commitStyles
    const commitStylesSpy = jest.spyOn(animation['animation'], 'commitStyles')

    animation.stop()

    expect(commitStylesSpy).toHaveBeenCalled()

    document.body.removeChild(element)
})

2. Optional Chaining on element

Severity: Low

The code uses element?.isConnected, which is good defensive programming. However, based on the constructor (NativeAnimation.ts:55-89), element should always be defined when options exists. The optional chaining is still the right approach here for safety.

3. Missing Edge Case: Element Connected but Not in Document

Severity: Low

An element can have isConnected === false in two scenarios:

1. Never added to DOM
2. Removed from DOM

The current test only covers scenario #2. Consider adding a test for scenario #1:

test("should not throw when element never added to DOM", () => {
    const element = document.createElement("div")
    // Note: element NOT added to document.body

    const animation = new NativeAnimation({
        element,
        name: "opacity",
        keyframes: [0, 1],
        duration: 1000,
    })

    expect(() => animation.stop()).not.toThrow()
})

4. Test Cleanup Consistency

Severity: Low

In the second test (lines 25-43), you clean up the element after the test. In the first test (lines 5-23), the element is already removed so no cleanup is needed. This is fine, but consider using afterEach for consistency:

describe("commitStyles", () => {
    let element: HTMLDivElement

    beforeEach(() => {
        element = document.createElement("div")
    })

    afterEach(() => {
        if (element.isConnected) {
            document.body.removeChild(element)
        }
    })

    // ... tests
})

---

🎯 Best Practices & Code Quality

✅ Follows Repository Conventions

• Uses interface over type ✓
• Named exports ✓
• Proper TypeScript typing ✓
• Follows existing code style ✓

✅ Security Considerations

No security concerns. This is purely defensive error handling.

✅ Performance Considerations

The isConnected check is a simple property access (negligible overhead). The change actually improves performance by avoiding unnecessary WAAPI calls and error handling.

---

📋 Final Recommendations

Required Before Merge:

• Add test to verify commitStyles() is actually called when element is connected (using spy)

Recommended Before Merge:

• Add test for element never added to DOM scenario
• Consider refactoring tests to use beforeEach/afterEach for cleanup

Optional Enhancements:

• Consider adding a comment explaining why isConnected check is necessary (prevents WAAPI error when element is disconnected)

---

✅ Overall Assessment

Verdict: Approve with minor suggestions

This is a solid bug fix that:

• Solves the reported issue ([BUG] Animate mini crash when cancelling scroll #3509)
• Follows best practices
• Includes test coverage
• Has minimal performance impact
• Doesn't introduce breaking changes

The suggestions above are primarily about test coverage completeness rather than functional issues. The core fix is sound and ready to merge with the minor test improvements.

Great work! 🎉

[claude]

Code Review

Summary

This PR fixes a crash in animateMini when stop() is called after an element has been removed from the DOM. The fix adds an isConnected check before calling WAAPI's commitStyles() method.

✅ Positive Aspects

1. Targeted fix: The solution is minimal and surgical - only adds the necessary check without over-engineering
2. Correct approach: Checking element?.isConnected is the standard way to verify DOM connectivity
3. Good test coverage: Includes both E2E test (Playwright) and manual HTML test case
4. Clear documentation: The test clearly references issue [BUG] Animate mini crash when cancelling scroll #3509 and demonstrates the exact failure scenario
5. Safe implementation: Uses optional chaining (element?.isConnected) which handles undefined/null gracefully

🔍 Code Quality Observations

packages/motion-dom/src/animation/NativeAnimation.ts:181-184

protected commitStyles() {
    const element = this.options?.element
    if (!this.isPseudoElement && element?.isConnected) {
        this.animation.commitStyles?.()
    }
}

The logic is sound and follows defensive programming practices. The conditions are properly ordered:

1. First check if it's not a pseudo-element (as before)
2. Then check if element exists and is connected to DOM (new)
3. Finally call commitStyles() with optional chaining

🧪 Test Coverage Assessment

Strengths:

• E2E test validates the fix doesn't crash (tests/animate/mini.spec.ts:66-71)
• HTML test demonstrates real-world usage pattern (dev/html/public/playwright/animate/mini.html:166-193)
• Test properly simulates the disconnectedCallback scenario mentioned in issue [BUG] Animate mini crash when cancelling scroll #3509

Minor observation:
The test follows the pattern of "no error = success" which is appropriate here since the bug was a crash. The test correctly shows "complete" status when no error is thrown.

🚀 Performance Considerations

• Impact: Negligible - adds one property access check (isConnected) before method call
• Benefit: Prevents unnecessary exception throwing and catching, which is actually a performance improvement

🔒 Security Considerations

No security concerns. This is a defensive fix that prevents a method from being called on disconnected elements.

🎯 Recommendations

The PR is well-implemented and ready to merge. No changes required.

Optional enhancement (future consideration):
If this pattern of checking isConnected becomes common across other methods, consider adding a helper method like isElementAvailable() for consistency. However, given this appears to be the only place where commitStyles() is called on potentially disconnected elements, the current approach is perfectly fine.

✨ Conclusion

Approval Status: ✅ Approved

This is a clean, well-tested bug fix that solves the reported issue without introducing complexity or side effects. The fix aligns with the repository's code style guidelines and includes appropriate test coverage.

Great work on the thorough test coverage and minimal implementation! 🎉