Fix stale node references in projection stack during SPA navigations

[mattgperry]

Summary

This PR fixes issues with stale DOM node references remaining in the projection stack after single-page application (SPA) navigations. When DOM nodes are disconnected but still referenced in the stack, they can cause incorrect shared element transitions and memory leaks.

Closes: #3516

Key Changes

• Cleanup stale members in add(): Filter out members with disconnected DOM instances before adding new nodes to the stack
• Reset lead references: Clear prevLead and lead pointers if they reference nodes that are no longer in the filtered members list
• Guard prevLead usage: Add a connectivity check before using prevLead for shared element transitions to prevent operations on disconnected DOM nodes

Implementation Details

The fix leverages the DOM's isConnected property to detect when nodes have been removed from the document tree. This is particularly important in SPA scenarios where:

• Navigation can unmount components without properly cleaning up projection state
• Stale node references can interfere with subsequent page transitions
• Shared element transitions may attempt to animate disconnected elements

The cleanup happens at the entry point (add()) to ensure the stack remains consistent, and the transition guard prevents any operations on disconnected nodes during the render phase.

https://claude.ai/code/session_019kNt63LG4sBKvk5m87JVUb

[claude]

Code Review

Summary

This PR provides a minimal, elegant fix for stale DOM node references in the projection stack during SPA navigations. The solution is significantly simpler than the alternative approach in #3516 (15 lines vs 190+ lines) while effectively addressing the core issue.

✅ Strengths

1. Minimal and focused: The fix is concise and targets exactly where the problem occurs - at the entry point (add()) and at usage (promote()).

2. Correct use of isConnected: Leverages the native DOM isConnected property to detect disconnected nodes, which is the standard way to check if a node is still in the document tree.

3. Lazy cleanup strategy: Instead of eagerly cleaning up on every operation, this approach filters stale nodes when new nodes are added, which is a good performance trade-off.

4. Prevents memory leaks: By clearing prevLead and lead references when they're filtered out, the fix prevents memory leaks from retained DOM nodes.

5. Defense in depth: The guard at line 74 (prevLead.instance?.isConnected) provides an additional safety check before using prevLead for shared element transitions.

⚠️ Potential Issues

1. Missing test coverage: This PR does not include tests. The related PR Fix: fix simple SPA layout animations #3516 includes an HTML test fixture (shared-element-spa-repeat.html) that demonstrates the issue. Consider either:

   • Adopting the test from Fix: fix simple SPA layout animations #3516
   • Writing a new test specifically for this implementation
   • Adding a unit test for the NodeStack class

2. Type safety concern: The instance property is typed as generic I | undefined in IProjectionNode<I>. The code assumes instance has an isConnected property, which is true for DOM nodes but not enforced by the type system. This works because:

   • The filter safely handles \!m.instance (nodes without instances are kept)
   • The optional chaining ?.isConnected at line 74 handles the case gracefully

   However, for full type safety, consider whether the generic I should be constrained to extend { isConnected: boolean }.

3. Edge case - nodes without instances: Line 12 has the logic \!m.instance || m.instance.isConnected. This means:

   • Nodes without instances are always kept in the members array
   • Is this intentional? Could this lead to accumulating nodes without instances?

   Looking at PR Fix: fix simple SPA layout animations #3516, it has more sophisticated logic that considers isPresent and snapshot properties. The current PR may be too aggressive in keeping nodes without instances.

4. Redundant check at line 75: After confirming prevLead.instance?.isConnected at line 74, the check prevLead.instance && at line 75 is redundant since we already know instance exists and is connected.

📋 Specific Code Comments

Line 12: Filter logic

this.members = this.members.filter(
    (m) => \!m.instance || m.instance.isConnected
)

Consider if this should also check isPresent and snapshot like PR #3516 does:

this.members = this.members.filter((m) => {
    if (\!m.instance) return m.isPresent === false || m.snapshot
    return m.instance.isConnected || m.isPresent === false || m.snapshot
})

This would handle cases where:

• Exiting elements (isPresent === false) should be kept even if disconnected
• Elements with snapshots should be kept even if disconnected

Line 75: Redundant check

prevLead.instance && prevLead.scheduleRender()

This is redundant since line 74 already verified prevLead.instance?.isConnected. Can simplify to:

prevLead.scheduleRender()

🔍 Comparison with PR #3516

PR #3516 has more comprehensive logic but is significantly more complex:

• Handles isPresent and snapshot edge cases
• Type casts for TypeScript safety
• Attempts to reassign lead to the last valid member if current lead is invalidated
• Captures snapshots if missing during promote

The current PR is simpler but may miss some edge cases. Consider whether the simplicity trade-off is worth potentially missing these scenarios.

🎯 Recommendations

1. Add tests - Critical for preventing regression. Use or adapt the test from Fix: fix simple SPA layout animations #3516.

2. Consider edge cases - Review whether nodes with isPresent === false or existing snapshot should be preserved even when disconnected.

3. Remove redundancy - Clean up the redundant check at line 75.

4. Document the trade-offs - If choosing simplicity over comprehensive handling, document why certain edge cases are not handled.

5. Performance consideration - The filter runs on every add() call. For large member arrays, this could be a concern, though in practice projection stacks are typically small.

🏁 Verdict

This is a well-crafted minimal fix that solves the core problem elegantly. The main concern is the lack of test coverage and potentially missing edge cases around isPresent and snapshot handling. With tests added and edge cases reviewed, this would be a solid improvement over the status quo.

Recommendation: Approve pending test coverage. Consider the edge cases mentioned but they may be acceptable trade-offs for simplicity.

---

Review generated with Claude Code