Fix stale shared layout nodes during SPA navigations

[mattgperry]

Summary

• Guard against zombie projection nodes that accumulate during SPA navigations (SvelteKit, etc.) where elements are removed externally without going through Motion's unmount path
• Add isConnected checks in NodeStack.promote(), relegate(), and add() to detect and handle stale nodes with disconnected DOM instances
• The key insight: React's unmount() sets instance = undefined (safe to use as resumeFrom), while SPA zombies retain a disconnected HTMLElement (stale, must be skipped)

Test plan

• New E2E fixture shared-element-spa-navigation.html simulates SPA navigation (external element removal + animateLayout remount) with two navigation cycles
• All existing layout unit tests pass
• All Cypress E2E tests pass (layout-shared.ts, layout-shared-fragment.ts, layout.ts, drag.ts — 0 regressions)
• All Playwright tests pass (176/176)
• All HTML animate-layout tests pass (24/24 including new fixture)

🤖 Generated with Claude Code

[claude]

Code Review

This PR addresses a critical bug where zombie projection nodes accumulate during SPA navigations when elements are removed externally (outside Motion's unmount path). The solution adds isConnected checks to detect and skip stale nodes with disconnected DOM instances.

✅ Code Quality & Best Practices

Strengths:

• Clear problem identification: The PR description excellently explains the key insight distinguishing React's unmount() (sets instance = undefined) from SPA zombies (retain disconnected HTMLElement)
• Focused changes: Modifications are surgical and limited to the three critical methods in NodeStack (packages/motion-dom/src/projection/shared/stack.ts:12-18, 48, 91)
• Type safety: Proper type casting with as HTMLElement | undefined and safe optional chaining
• Code style: Follows repository conventions (optional chaining, arrow functions, strict equality)

Minor observations:

• The cleanup logic in add() (lines 12-19) iterates backwards, which is correct for safe removal during iteration
• The guard conditions are comprehensive: checks for current node, lead, prevLead, snapshot existence, and isPresent state

🔍 Potential Bugs & Edge Cases

Well-handled edge cases:

1. Guards against critical nodes (line 14): Correctly skips removal for the current node being added, lead, and prevLead
2. Snapshot preservation (line 16): The !m.snapshot check prevents removing nodes that still have valid snapshot data
3. Present state check (line 16): m.isPresent !== false ensures nodes explicitly marked as not present are candidates for cleanup

Questions/Considerations:

1. Race conditions: In add(), the cleanup happens after addUniqueItem() but before scheduleRender(). This ordering appears correct since we want to clean stale nodes before scheduling renders.

2. Memory safety in promote() (line 91): The isStale check correctly combines:

   • prevInstance && prevInstance.isConnected === false (DOM disconnected)
   • !prevLead.snapshot (no valid snapshot to resume from)

   This prevents using disconnected nodes as resumeFrom sources, which would cause incorrect animations.

3. Asymmetry in checks: In relegate() (line 48), the check uses (!inst || inst.isConnected !== false), which means:

   • Accept if inst is undefined (unmounted properly via React)
   • Accept if isConnected is not explicitly false (connected or unknown)

   This differs slightly from the promote() check which explicitly requires isConnected === false. Both approaches seem correct for their contexts.

⚡ Performance Considerations

Positive:

• Cleanup in add() iterates backwards through what should typically be a small array of shared layout nodes
• The isConnected property is a browser-native DOM API (O(1) lookup), not computed
• Cleanup prevents memory leaks and reduces future iteration costs

Minimal impact:

• The additional checks add negligible overhead compared to the cost of incorrect layout animations
• Only runs during node addition/promotion, not on every frame

🔒 Security Concerns

No security issues identified. The changes are defensive and:

• Don't introduce XSS, injection, or DOM clobbering risks
• Use safe type casting with proper guards
• Don't expose internal state or create new attack surfaces

🧪 Test Coverage

Excellent test design:

• E2E test added (dev/html/public/animate-layout/shared-element-spa-navigation.html): Simulates real-world SPA navigation with two complete cycles
• Regression testing: Verifies no data-layout-correct="false" elements appear
• Test completeness: PR description confirms all existing tests pass (Jest unit, Cypress E2E, Playwright, HTML fixtures)

Suggestions for additional testing:

1. Consider adding a unit test directly for NodeStack methods to test the isConnected logic in isolation
2. Edge case: What happens with nested shared layout elements during SPA navigation?
3. Stress test: Multiple rapid navigations before animations complete

📋 Additional Observations

Documentation:

• The PR description is exemplary with clear summary and test plan
• Consider adding an inline code comment at line 16 explaining why we check !m.snapshot (preserves nodes with valid animation data)

Example inline comment suggestion for line 15-17:

const inst = m.instance as HTMLElement | undefined
// Remove zombie nodes: disconnected DOM elements that bypassed unmount()
// Preserve nodes with snapshots as they may be mid-animation
if (inst && inst.isConnected === false && m.isPresent !== false && !m.snapshot) {
    removeItem(this.members, m)
}

Summary

This is a high-quality fix for a subtle but important bug in shared layout animations. The implementation is sound, well-tested, and follows repository conventions. The fix is defensive without being overly aggressive (preserves nodes with snapshots, properly unmounted nodes).

Recommendation: Approve ✅

The only suggestion is adding inline documentation for the cleanup logic to help future maintainers understand the nuanced conditions.