docs(25): research memory leak audit domain

Phase 25: Memory Leak Audit - Identified 4 memory leak issues in recording code - Documented cleanup patterns (useRef, ignore flag, timer cleanup) - Created verification method following Phase 23 approach - Confidence: HIGH based on React docs and codebase analysis Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2026-02-24 22:44:50 +05:30 · 2026-02-24 22:44:50 +05:30 · 59f36edd93
parent 677732996f
commit 59f36edd93
1 changed files with 399 additions and 0 deletions
--- a/.planning/phases/25-memory-leak-audit/25-RESEARCH.md
+++ b/.planning/phases/25-memory-leak-audit/25-RESEARCH.md
@ -0,0 +1,399 @@
+# Phase 25: Memory Leak Audit - Research
+
+**Researched:** 2026-02-24
+**Domain:** React memory leak patterns, useEffect cleanup, Chrome DevTools memory profiling
+**Confidence:** HIGH
+
+## Summary
+
+This research identifies memory leak risks in the recording-related code of jam-ui and documents the standard patterns for fixing them. The audit scope includes `useRecordingHelpers.js`, `JKSessionRecordingModal.js`, related Redux state in `sessionUISlice.js`, `fakeJamClientRecordings.js`, and global `window.JK.*` callback registrations.
+
+The primary findings reveal:
+1. **Missing useEffect cleanup functions** in `JKSessionRecordingModal.js` - both useEffect hooks lack cleanup returns
+2. **Timer without cleanup on unmount** in `useRecordingHelpers.js` - the `waitingOnStopTimer` has internal cleanup but no useEffect cleanup return
+3. **Global callback registration without cleanup** in `useRecordingHelpers.js` - `window.JK.*` callbacks are registered but never cleaned up
+
+These patterns are well-documented memory leak sources in React applications. The established fix pattern follows Phase 22's approach: use `useRef` for cleanup stability and always return cleanup functions from useEffect.
+
+**Primary recommendation:** Add cleanup functions to all recording-related useEffect hooks, following the established `useRef` pattern from Phase 22.
+
+## Standard Stack
+
+This phase does not introduce new libraries. The work uses existing React patterns and Chrome DevTools.
+
+### Core Tools
+| Tool | Purpose | Why Standard |
+|------|---------|--------------|
+| Chrome DevTools Memory Panel | Heap snapshot analysis | Official Chrome tool for memory profiling |
+| Chrome Performance Monitor | Real-time heap size tracking | Built into Chrome DevTools |
+| React DevTools | Component state inspection | Official React debugging extension |
+
+### Patterns Used
+| Pattern | Purpose | When to Use |
+|---------|---------|-------------|
+| useRef for cleanup | Store cleanup targets with stable reference | When cleanup function needs current values |
+| AbortController | Cancel async fetch operations | When useEffect performs data fetching |
+| ignore flag | Prevent state updates after unmount | When async operation may complete after unmount |
+
+## Architecture Patterns
+
+### Recommended useEffect Cleanup Pattern
+
+Based on Phase 22's established approach and React official documentation:
+
+```javascript
+// Pattern: useRef for cleanup stability (from Phase 22)
+const resourceRef = useRef(null);
+
+useEffect(() => {
+  // Setup
+  resourceRef.current = someResource;
+
+  // Cleanup function returned
+  return () => {
+    if (resourceRef.current) {
+      resourceRef.current.cleanup();
+      resourceRef.current = null;
+    }
+  };
+}, [dependencies]);
+```
+
+### Pattern 1: Timer Cleanup
+
+**What:** Clear timers when component unmounts or dependencies change
+**When to use:** Any useEffect that calls setTimeout or setInterval
+**Example:**
+```javascript
+// Source: React official docs - https://react.dev/learn/synchronizing-with-effects
+useEffect(() => {
+  const timerId = setTimeout(onTick, 5000);
+  return () => clearTimeout(timerId);
+}, [dependencies]);
+```
+
+### Pattern 2: Global Callback Cleanup
+
+**What:** Remove globally registered callbacks when component unmounts
+**When to use:** When registering callbacks on window, document, or global objects
+**Example:**
+```javascript
+// Pattern for window.JK.* callbacks
+const callbackRef = useRef(null);
+
+useEffect(() => {
+  callbackRef.current = handleCallback;
+  window.JK = window.JK || {};
+  window.JK.HandleSomething = callbackRef.current;
+
+  return () => {
+    // Don't delete if another instance registered a different callback
+    if (window.JK?.HandleSomething === callbackRef.current) {
+      delete window.JK.HandleSomething;
+    }
+  };
+}, [handleCallback]);
+```
+
+### Pattern 3: Async Operation Cleanup with ignore Flag
+
+**What:** Prevent state updates after component unmounts
+**When to use:** Any useEffect with async operations that call setState
+**Example:**
+```javascript
+// Source: React official docs
+useEffect(() => {
+  let ignore = false;
+
+  async function fetchData() {
+    const result = await someAsyncOperation();
+    if (!ignore) {
+      setState(result);
+    }
+  }
+
+  fetchData();
+
+  return () => {
+    ignore = true;
+  };
+}, [dependencies]);
+```
+
+### Anti-Patterns to Avoid
+
+- **Missing cleanup return:** useEffect hooks that set up resources but don't return cleanup functions
+- **Stale closure in cleanup:** Using state directly in cleanup function instead of refs
+- **Conditional cleanup:** Returning cleanup only in some code paths (must always return if needed)
+
+## Don't Hand-Roll
+
+| Problem | Don't Build | Use Instead | Why |
+|---------|-------------|-------------|-----|
+| Async cancellation | Manual flag tracking | AbortController | Built-in browser API, works with fetch |
+| Stable callback refs | Manual closure management | useRef + useCallback | React-optimized approach |
+| Memory profiling | Custom tracking code | Chrome DevTools | Comprehensive, accurate, zero overhead |
+
+**Key insight:** React's useEffect cleanup pattern handles most memory leak scenarios. The Phase 22 useRef pattern specifically addresses stale closure issues that can cause cleanup to fail silently.
+
+## Common Pitfalls
+
+### Pitfall 1: Missing Cleanup Returns
+
+**What goes wrong:** useEffect sets up timers, listeners, or callbacks but doesn't return a cleanup function
+**Why it happens:** Developer focuses on setup logic and forgets cleanup is needed
+**How to avoid:** Treat every useEffect with side effects as requiring a cleanup return
+**Warning signs:** Console warnings about "Can't perform state update on unmounted component"
+
+**Found in audit:**
+- `JKSessionRecordingModal.js` line 54-71: useEffect fetches audio preference but has no cleanup
+- `JKSessionRecordingModal.js` line 74-78: useEffect loads video sources but has no cleanup
+
+### Pitfall 2: Global Callbacks Without Cleanup
+
+**What goes wrong:** Callbacks registered on window/global objects persist after component unmounts
+**Why it happens:** Callbacks seem "harmless" since they're just function references
+**How to avoid:** Always clean up global registrations; use refs to track what was registered
+**Warning signs:** Multiple callback instances running, callbacks referencing stale state
+
+**Found in audit:**
+- `useRecordingHelpers.js` line 459-468: Registers 5 `window.JK.*` callbacks without cleanup
+
+### Pitfall 3: Timer Not Cleared on Unmount
+
+**What goes wrong:** setTimeout/setInterval fires after component unmounts, causing state update on unmounted component
+**Why it happens:** Timer is cleared in internal logic but not in useEffect cleanup
+**How to avoid:** Return cleanup function that clears any active timers
+**Warning signs:** Console errors about state updates on unmounted components
+
+**Found in audit:**
+- `useRecordingHelpers.js` line 186: `waitingOnStopTimer` has internal cleanup but no useEffect unmount cleanup
+
+### Pitfall 4: Async State Updates After Unmount
+
+**What goes wrong:** Async operations (API calls) complete and call setState after component unmounts
+**Why it happens:** No mechanism to cancel or ignore outdated async results
+**How to avoid:** Use ignore flag pattern or AbortController
+**Warning signs:** Console warnings about state updates on unmounted components
+
+**Found in audit:**
+- `JKSessionRecordingModal.js` async calls to `jamClient.GetAudioRecordingPreference()` and `loadAvailableVideoSources()` may complete after modal closes
+
+## Code Examples
+
+### Example 1: Proper Timer Cleanup in useRecordingHelpers
+
+```javascript
+// Current problem (line 31 and 186):
+const waitingOnStopTimer = useRef(null);
+// Timer is set internally but no useEffect cleanup
+
+// Solution: Add cleanup useEffect
+useEffect(() => {
+  return () => {
+    if (waitingOnStopTimer.current) {
+      clearTimeout(waitingOnStopTimer.current);
+      waitingOnStopTimer.current = null;
+    }
+  };
+}, []); // Empty deps - cleanup only on unmount
+```
+
+### Example 2: Global Callback Registration with Cleanup
+
+```javascript
+// Current problem (line 459-468):
+useEffect(() => {
+  if (window) {
+    window.JK = window.JK || {};
+    window.JK.HandleRecordingStartResult = handleRecordingStartResult;
+    // ... more callbacks
+  }
+  // NO CLEANUP RETURNED
+}, [handleRecordingStartResult, ...]);
+
+// Solution: Store refs and clean up conditionally
+const callbackRefs = useRef({});
+
+useEffect(() => {
+  // Store current callback references
+  callbackRefs.current = {
+    HandleRecordingStartResult: handleRecordingStartResult,
+    HandleRecordingStopResult: handleRecordingStopResult,
+    HandleRecordingStopped: handleRecordingStopped,
+    HandleRecordingStarted: handleRecordingStarted,
+    HandleRecordingAborted: handleRecordingAborted,
+  };
+
+  window.JK = window.JK || {};
+  Object.assign(window.JK, callbackRefs.current);
+
+  return () => {
+    // Only delete if we still own the callback
+    Object.keys(callbackRefs.current).forEach(key => {
+      if (window.JK?.[key] === callbackRefs.current[key]) {
+        delete window.JK[key];
+      }
+    });
+  };
+}, [handleRecordingStartResult, handleRecordingStopResult, handleRecordingStopped, handleRecordingStarted, handleRecordingAborted]);
+```
+
+### Example 3: Async Cleanup in Modal
+
+```javascript
+// Current problem (JKSessionRecordingModal.js line 54-71):
+useEffect(() => {
+  const fetchAudioStoreType = async () => {
+    const pref = await jamClient.GetAudioRecordingPreference();
+    setAudioStoreType(/* ... */);  // May run after unmount
+  };
+  fetchAudioStoreType();
+}, [jamClient]);
+
+// Solution: Add ignore flag
+useEffect(() => {
+  let ignore = false;
+
+  const fetchAudioStoreType = async () => {
+    try {
+      if (jamClient) {
+        const pref = await jamClient.GetAudioRecordingPreference();
+        if (!ignore) {
+          // Safe to update state
+          if (AUDIO_STORE_TYPE_MIX_AND_STEMS['backendValues'].includes(pref)) {
+            setAudioStoreType(AUDIO_STORE_TYPE_MIX_AND_STEMS['key']);
+          } else if (AUDIO_STORE_TYPE_MIX_ONLY['backendValues'].includes(pref)) {
+            setAudioStoreType(AUDIO_STORE_TYPE_MIX_ONLY['key']);
+          }
+        }
+      }
+    } catch (error) {
+      if (!ignore) {
+        console.error('Error fetching audio store type preference:', error);
+      }
+    }
+  };
+
+  fetchAudioStoreType();
+
+  return () => {
+    ignore = true;
+  };
+}, [jamClient]);
+```
+
+## Audit Findings Summary
+
+### Files in Scope
+
+| File | Issues Found | Severity |
+|------|--------------|----------|
+| `useRecordingHelpers.js` | 2 (timer + callbacks) | HIGH |
+| `JKSessionRecordingModal.js` | 2 (async + missing cleanup) | MEDIUM |
+| `sessionUISlice.js` | 0 | N/A (Redux, no hooks) |
+| `fakeJamClientRecordings.js` | 1 (timers in class) | LOW (test only) |
+
+### Issue Details
+
+1. **useRecordingHelpers.js - Timer Cleanup (HIGH)**
+   - Line 31: `waitingOnStopTimer` useRef
+   - Line 186: Timer set but no useEffect cleanup on unmount
+   - Risk: Timer fires after hook unmounts, causes stale state updates
+
+2. **useRecordingHelpers.js - Global Callbacks (HIGH)**
+   - Lines 459-468: 5 window.JK.* callbacks registered
+   - No cleanup function returned
+   - Risk: Callbacks accumulate on repeated mount/unmount cycles
+
+3. **JKSessionRecordingModal.js - Async Without Ignore (MEDIUM)**
+   - Lines 54-71: Async fetch without ignore flag
+   - Lines 74-78: Video source loading without cleanup
+   - Risk: State updates after modal closes
+
+4. **fakeJamClientRecordings.js - Test Timers (LOW)**
+   - Lines 45, 78: Timers for test simulation
+   - Lines 199, 205: Timers cleared internally
+   - Risk: Minimal (test code only), but could affect test reliability
+
+## State of the Art
+
+| Old Approach | Current Approach | When Changed | Impact |
+|--------------|------------------|--------------|--------|
+| No cleanup, rely on GC | Always return cleanup functions | React 16.8 (hooks) | Prevents memory leaks |
+| State in cleanup closures | useRef for cleanup stability | Established pattern | Avoids stale closure bugs |
+| Manual async tracking | AbortController | Widespread 2020+ | Native browser support |
+
+**Current best practice:** React Strict Mode (development) remounts components to verify cleanup works correctly. Code should pass this test without console warnings.
+
+## Open Questions
+
+1. **Multiple hook instances sharing window.JK callbacks**
+   - What we know: Both `JKSessionScreen` and `JKSessionRecordingModal` call `useRecordingHelpers`
+   - What's unclear: Whether deleting callbacks on modal unmount affects screen
+   - Recommendation: Use conditional cleanup that only removes if callback reference matches
+
+2. **FakeJamClientRecordings test reliability**
+   - What we know: Timer-based test simulation could be affected by cleanup
+   - What's unclear: Whether tests depend on timer persistence
+   - Recommendation: Low priority - address if test failures occur after fixes
+
+## Memory Verification Method
+
+Following Phase 23 UAT pattern, create checklist for manual verification:
+
+### Chrome DevTools Steps
+
+1. **Baseline Measurement**
+   - Open DevTools > Memory panel
+   - Take heap snapshot (Snapshot 1)
+   - Record JS Heap size in Performance Monitor
+
+2. **Test Scenario 1: Modal Idle**
+   - Open recording modal
+   - Leave open for 15+ minutes
+   - Take heap snapshot (Snapshot 2)
+   - Compare: Growth should be < 5MB
+
+3. **Test Scenario 2: Active Recording**
+   - Start recording
+   - Record for 15+ minutes
+   - Stop recording
+   - Take heap snapshot (Snapshot 3)
+   - Compare: No unbounded growth
+
+4. **Test Scenario 3: Repeated Open/Close**
+   - Open/close recording modal 20 times
+   - Take heap snapshot
+   - Look for: Retained closures, detached DOM nodes
+
+### Success Metrics
+
+- Zero "state update on unmounted component" warnings
+- Heap growth < 50% during 15-minute test
+- No detached DOM nodes from modal
+- Callbacks not accumulating in window.JK
+
+## Sources
+
+### Primary (HIGH confidence)
+- React Official Documentation: https://react.dev/learn/synchronizing-with-effects#step-3-add-cleanup-if-needed - useEffect cleanup patterns
+- Chrome DevTools Documentation: https://developer.chrome.com/docs/devtools/memory-problems/heap-snapshots - Memory profiling
+
+### Secondary (MEDIUM confidence)
+- Phase 22 Summary: `.planning/phases/22-session-screen-fixes/22-01-SUMMARY.md` - useRef cleanup pattern
+- Phase 23 UAT: `.planning/phases/23-verification/23-UAT.md` - Memory verification checklist template
+
+### Tertiary (LOW confidence)
+- LogRocket Blog: https://blog.logrocket.com/understanding-react-useeffect-cleanup-function/ - Cleanup patterns
+- StackInsight Study: https://stackinsight.dev/blog/memory-leak-empirical-study - Memory leak statistics
+
+## Metadata
+
+**Confidence breakdown:**
+- Cleanup patterns: HIGH - Verified against React official docs and established Phase 22 pattern
+- Issue identification: HIGH - Direct codebase analysis with line numbers
+- Verification method: HIGH - Based on Phase 23 proven approach
+
+**Research date:** 2026-02-24
+**Valid until:** 2026-03-24 (30 days - React patterns are stable)