Add generic ClosureBox to fix thunk stack growing

[muukii]

Summary

• Create generic ClosureBox<R> in withTracking.swift to eliminate code duplication
• Fix thunk stack growing for didChange closure in withContinuousStateGraphTracking by wrapping closures once at entry point and passing them directly in recursive calls
• Remove duplicate private ClosureBox definitions from withGraphTrackingGroup.swift and withGraphTrackingMap.swift
• Add callAsFunction support for cleaner invocation syntax

Background

PR #88 fixed thunk stack growing issue for handler closures by wrapping them in ClosureBox. However:

1. ClosureBox was duplicated in two files
2. didChange closure in withContinuousStateGraphTracking still had the same thunk stack growing issue due to recursive wrapping/unwrapping via UnsafeSendable

Changes

File	Changes
withTracking.swift	Add generic ClosureBox<R> with callAsFunction, add private _withContinuousStateGraphTracking
withGraphTrackingGroup.swift	Remove private ClosureBox, use shared one
withGraphTrackingMap.swift	Remove private ClosureBox, use shared one

Test plan

• swift build passes
• swift test passes (99 tests)

🤖 Generated with Claude Code

[claude]

Claude finished @muukii's task —— View job

---

Code Review

I've analyzed the PR and found it to be a well-executed refactoring that addresses a specific performance issue. Here's my detailed feedback:

✅ Strengths

1. Generic ClosureBox Design (withTracking.swift:50-60)

• Clean, focused design with a single generic parameter <R>
• Addition of callAsFunction() provides convenient invocation syntax
• Centralizes closure boxing logic to prevent future duplication

2. Thunk Stack Growing Fix (withTracking.swift:81-121)

• Root Cause Addressed: Previously, UnsafeSendable wrapping/unwrapping on each recursive call caused thunk stack growth
• Elegant Solution: Closures are now wrapped once at entry point (withContinuousStateGraphTracking) and passed through all recursive iterations via private _withContinuousStateGraphTracking
• Performance Impact: Eliminates repeated closure type conversions in recursive scenarios

3. Code Duplication Removal

• Successfully eliminates duplicate ClosureBox definitions from withGraphTrackingGroup.swift and withGraphTrackingMap.swift
• Both files now use the shared generic version, reducing maintenance burden

🎯 Architecture Review

Thread Safety: ✅

• Maintains existing OSAllocatedUnfairLock patterns
• No new concurrency concerns introduced

Memory Management: ✅

• Proper weak reference handling preserved
• No new retain cycle risks

API Compatibility: ✅

• All public APIs remain unchanged
• Internal refactoring is transparent to consumers

🔍 Technical Deep Dive

Before (Issue):

// Each recursive call re-wrapped closures via UnsafeSendable
withContinuousStateGraphTracking(apply: apply, didChange: didChange)
  -> withContinuousStateGraphTracking(apply: UnsafeSendable(apply)._value, ...) // Stack growth!

After (Solution):

// Wrap once, pass through recursion
let applyBox = ClosureBox(apply)      // Single wrapping
let didChangeBox = ClosureBox(didChange)
_withContinuousStateGraphTracking(apply: applyBox, didChange: didChangeBox) // Direct passing

🐛 Potential Issues & Recommendations

Minor: Documentation

• Consider adding inline documentation to _withContinuousStateGraphTracking explaining why it's private and its relationship to the public API

Performance Validation

• The fix should significantly reduce stack pressure in continuous tracking scenarios
• Would benefit from a performance test to validate the improvement (though existing tests pass)

🧪 Test Coverage Assessment

Current State: ✅ 99 tests passing

• Existing test suite validates functional correctness
• Integration tests likely cover the continuous tracking paths

Recommendation: Consider adding a specific test case that exercises deep recursion in continuous tracking to validate the stack growth fix (though this may be challenging to test reliably).

🔒 Security Review

No security concerns identified:

• No new external dependencies
• No changes to data handling or validation
• Maintains existing isolation and sendability constraints

📊 Performance Considerations

Positive Impact:

• Eliminates closure re-wrapping overhead in recursive scenarios
• Reduces stack frame growth in continuous tracking
• callAsFunction adds syntactic convenience without performance cost

No Negative Impact:

• Single-use scenarios have identical performance (one ClosureBox allocation)
• Memory footprint unchanged

🎉 Final Assessment

This is a high-quality refactoring that:

• ✅ Fixes a legitimate performance issue (thunk stack growing)
• ✅ Improves code organization (eliminates duplication)
• ✅ Maintains API compatibility
• ✅ Follows existing code patterns
• ✅ Passes all tests

Recommendation: Approve and merge

The implementation is solid, addresses the stated problem effectively, and improves the codebase without introducing new risks.

Todo List

• Read and analyze the 3 changed files to understand the implementation
• Review the generic ClosureBox design and architecture
• Analyze the code duplication removal and refactoring
• Evaluate the thunk stack growing fix
• Check for potential bugs, security issues, and performance concerns
• Assess test coverage and provide comprehensive review feedback

[muukii]

@JohnEstropia TIL