diff --git a/.planning/phases/19-audit-and-discovery/19-RESEARCH.md b/.planning/phases/19-audit-and-discovery/19-RESEARCH.md new file mode 100644 index 000000000..e0ba403f2 --- /dev/null +++ b/.planning/phases/19-audit-and-discovery/19-RESEARCH.md @@ -0,0 +1,353 @@ +# Phase 19: Audit and Discovery - Research + +**Researched:** 2026-02-08 +**Domain:** React memory leak detection and cleanup patterns +**Confidence:** HIGH + +## Summary + +This phase involves auditing the jam-ui React application to identify memory leaks causing the ~10-minute session freeze. Research focused on examining the codebase for common React memory leak patterns: missing cleanup functions in useEffect, uncleared setInterval/setTimeout timers, unbounded state growth, and leaked event listeners. + +The codebase analysis revealed several areas requiring attention: +1. **VU Meter system**: Uses a callback-driven pattern via `window.JK.HandleBridgeCallback2` that is properly cleaned up +2. **WebSocket/Chat**: Multiple polling intervals and message callbacks that need audit for cleanup +3. **Session Screen**: Complex component with multiple timers, event listeners, and callbacks that require verification + +**Primary recommendation:** Systematically audit all `useEffect` hooks, `setInterval`, `setTimeout`, and `addEventListener` calls to ensure proper cleanup functions exist and are called on unmount. + +## Standard Stack + +### Core Tools (Already in Project) +| Tool | Purpose | Why Use | +|------|---------|---------| +| Chrome DevTools Memory Panel | Heap snapshots, allocation timeline | Definitive memory leak evidence | +| React DevTools | Component tree inspection | Detect retained components | +| Browser Console | Error logging | "Can't perform state update on unmounted component" warnings | + +### Audit Methodology +| Approach | Purpose | When to Use | +|----------|---------|-------------| +| Static Code Analysis | Find missing cleanup patterns | First pass - identify candidates | +| Heap Snapshot Comparison | Verify actual memory growth | Validate suspected leaks | +| Allocation Timeline | Trace allocation sources | Isolate specific leak origin | + +### No Additional Libraries Needed +This is a code audit phase - no new dependencies required. The existing codebase provides all the patterns needed for fixes. + +## Architecture Patterns + +### Codebase Timer/Interval Summary + +Based on code analysis, here are the timer/interval locations in the session screen area: + +#### JKSessionScreen.js +- **Lines 454-472**: Three `setTimeout` calls for track sync (timer1, timer2, timer3) - **HAS CLEANUP** in return function +- **Line 985**: `setTimeout` for video loading button - **NO CLEANUP** (not a leak - short timeout in finally block) + +#### JKSessionBackingTrackPlayer.js +- **Lines 155-218**: `setInterval` for playback position polling - **HAS CLEANUP** via `clearInterval(intervalId)` +- **Line 301**: `setTimeout` in async Promise - **NO CLEANUP NEEDED** (one-time delay) +- **Lines 230-239**: Unmount cleanup effect for stopping playback + +#### JKSessionJamTrackPlayer.js +- **Lines 415-461**: `setInterval` for position/duration polling - **HAS CLEANUP** via `clearInterval(intervalId)` return +- **Lines 465-473**: Event listener for visibilitychange - **HAS CLEANUP** + +#### useJamServer.js (WebSocket Hook) +- **Lines 270-272**: `reconnectTimeoutRef` setTimeout - **HAS CLEANUP** on disconnect and unmount +- **Lines 311-320**: Timeout in sendRequest - **HAS CLEANUP** via clearTimeout +- **Lines 485-493**: `startHeartbeatTimeout`, `heartbeatInterval`, `heartbeatAckCheckInterval` - **HAS CLEANUP** via `stopHeartbeat()` +- **Lines 608-634**: `activityTimeout` for user activity - **HAS CLEANUP** in markActive and cleanup functions + +#### useMixerStore.js +- **Line 79**: Sets `window.JK.HandleBridgeCallback2` - **HAS CLEANUP** on line 87 via delete + +#### useSessionStats.js +- **Lines 151-153**: `setInterval` for stats refresh every 2 seconds - **HAS CLEANUP** via return clearInterval + +### Pattern: Proper Interval Cleanup +```javascript +// Source: React official documentation pattern +useEffect(() => { + const intervalId = setInterval(() => { + // callback logic + }, 1000); + + return () => { + clearInterval(intervalId); // CRITICAL: Must clear on unmount + }; +}, [dependencies]); +``` + +### Pattern: Proper Event Listener Cleanup +```javascript +// Source: Current codebase pattern (JKSessionChatWindow.js lines 59-67) +useEffect(() => { + const handleKeyDown = (e) => { + if (e.key === 'Escape') { + dispatch(closeChatWindow()); + } + }; + window.addEventListener('keydown', handleKeyDown); + return () => window.removeEventListener('keydown', handleKeyDown); +}, [dispatch]); +``` + +### Pattern: WebSocket Callback Registration +```javascript +// Source: JKSessionScreen.js lines 616-629 +// Register callbacks +callbacksToRegister.forEach(({ type, callback }) => { + registerMessageCallback(type, callback); +}); + +// Store for cleanup +setRegisteredCallbacks(callbacksToRegister); + +// On leave/unmount +const unregisterMessageCallbacks = useCallback(() => { + registeredCallbacks.forEach(({ type, callback }) => { + unregisterMessageCallback(type, callback); + }); + setRegisteredCallbacks([]); +}, [registeredCallbacks, unregisterMessageCallback]); +``` + +## Don't Hand-Roll + +| Problem | Don't Build | Use Instead | Why | +|---------|-------------|-------------|-----| +| Debounced cleanup | Custom debounce with manual cleanup | useRef + useEffect pattern | Standard React pattern, less error-prone | +| Timer ID tracking | Global variable tracking | useRef for timer IDs | Survives re-renders, cleaned with component | +| WebSocket cleanup | Manual unsubscribe tracking | Store callback references in state | Ensures all callbacks tracked for cleanup | + +**Key insight:** The codebase already follows most React best practices. The focus should be on finding edge cases where cleanup is missing or incomplete. + +## Common Pitfalls + +### Pitfall 1: Stale Closure in Cleanup +**What goes wrong:** Cleanup function captures stale values from closure +**Why it happens:** Dependencies not properly specified in useEffect +**How to avoid:** Include all referenced values in dependency array +**Warning signs:** Cleanup runs but timer/listener still fires + +### Pitfall 2: Conditional Timer Setup Without Conditional Cleanup +**What goes wrong:** Timer created conditionally but cleanup always runs +**Why it happens:** Timer ID undefined when clearInterval called +**How to avoid:** Check if timer ID exists before clearing +**Warning signs:** No error but timer leaks when condition was false + +### Pitfall 3: State Update After Unmount +**What goes wrong:** Console warning "Can't perform state update on unmounted component" +**Why it happens:** Async operation completes after component unmounts, tries to setState +**How to avoid:** Use mounted ref pattern or abort controller +**Warning signs:** Console warnings during navigation + +### Pitfall 4: Unbounded Array Growth in Redux +**What goes wrong:** Redux state grows indefinitely (e.g., chat messages) +**Why it happens:** Messages added without limit or cleanup +**How to avoid:** Implement max message limit or pagination +**Warning signs:** Memory grows linearly with time/messages + +### Pitfall 5: Event Listener Registered Multiple Times +**What goes wrong:** Same listener added on every render +**Why it happens:** Missing dependency array or listener not memoized +**How to avoid:** Empty dependency array for one-time setup, or memoize callback +**Warning signs:** Callback fires multiple times for single event + +## Code Examples + +### Current VU Meter Pattern (VuContext.js + useVuHelpers.js) +```javascript +// Source: jam-ui/src/hooks/useVuHelpers.js lines 117-122 +useEffect(() => { + return () => { + // Cleanup on unmount + setVuStates({}); + }; +}, []); + +// VU updates come from native client via: +// window.JK.HandleBridgeCallback2 in useMixerStore.js +// This is properly cleaned up on line 87 +``` + +### Current WebSocket Chat Pattern (sessionChatSlice.js) +```javascript +// Source: jam-ui/src/store/features/sessionChatSlice.js +// Messages stored in: state.messagesByChannel[channel] +// POTENTIAL ISSUE: No max message limit implemented +// Messages accumulate indefinitely during session +``` + +### Current Polling Pattern (JKSessionBackingTrackPlayer.js) +```javascript +// Source: jam-ui/src/components/client/JKSessionBackingTrackPlayer.js lines 145-227 +useEffect(() => { + let intervalId = null; + + if (isPlaying && jamClient) { + const pollingInterval = isVisible ? 500 : 2000; + + intervalId = setInterval(async () => { + // Poll position/duration + }, pollingInterval); + } + + // CLEANUP: Properly clears interval + return () => { + if (intervalId) { + clearInterval(intervalId); + } + }; +}, [isPlaying, jamClient, backingTrack, isVisible, ...]); +``` + +## Identified Areas for Audit + +### Priority 1: VU Meters (VUMTR) +| File | Issue | Severity | Evidence | +|------|-------|----------|----------| +| `useMixerStore.js` | VU callback on `window.JK` | LOW | Has cleanup on unmount | +| `useVuHelpers.js` | VuStates accumulation | MEDIUM | States cleared on unmount but not per-mixer | +| `SessionTrackVU.js` | Mixer registration | LOW | Has cleanup return function | + +**Audit Action:** Verify VU state doesn't grow unbounded when tracks added/removed + +### Priority 2: Chat Window (CHAT) +| File | Issue | Severity | Evidence | +|------|-------|----------|----------| +| `sessionChatSlice.js` | Message array growth | HIGH | No max limit on messagesByChannel | +| `JKSessionChatWindow.js` | Keyboard listener | LOW | Has cleanup | +| `JKChatMessageList.js` | Scroll timeout | LOW | Has cleanup | +| `useSessionWebSocket.js` | Chat callback | MEDIUM | Callback stored but cleanup unclear | + +**Audit Action:** Implement message limit or pagination; verify WebSocket callback cleanup + +### Priority 3: Session Screen (SESS) +| File | Issue | Severity | Evidence | +|------|-------|----------|----------| +| `JKSessionScreen.js` | Track sync timers | LOW | Has cleanup on lines 470-472 | +| `JKSessionScreen.js` | Message callbacks | MEDIUM | Stored in state, cleanup on leave | +| `JKSessionScreen.js` | window.JK.HandleAlertCallback | LOW | Deleted on cleanup | +| `useJamServer.js` | Heartbeat intervals | LOW | Proper cleanup in stopHeartbeat | +| `useJamServer.js` | Activity timeout | LOW | Cleared in multiple places | +| `useSessionModel.js` | backendMixerAlertThrottleTimer | MEDIUM | Check cleanup completeness | + +**Audit Action:** Verify all cleanup runs on unmount; trace callback lifecycle + +### Priority 4: Players +| File | Issue | Severity | Evidence | +|------|-------|----------|----------| +| `JKSessionBackingTrackPlayer.js` | Position polling | LOW | Has cleanup | +| `JKSessionJamTrackPlayer.js` | Position polling | LOW | Has cleanup | +| `JKSessionMetronomePlayer.js` | Visibility listener | LOW | Has cleanup | + +**Audit Action:** Verify polling stops when hidden/closed + +## Verification Strategy + +### Phase 1: Static Code Audit +1. Grep for all `setInterval`, `setTimeout`, `requestAnimationFrame` calls +2. Verify each has corresponding cleanup in return function +3. Check all `addEventListener` calls have `removeEventListener` +4. Review Redux slices for unbounded array growth patterns + +### Phase 2: Runtime Verification +1. Open Chrome DevTools > Memory tab +2. Take heap snapshot before joining session +3. Join session, interact for 5 minutes +4. Take heap snapshot +5. Compare: look for growing objects (especially arrays, listeners) +6. Force GC and take another snapshot to confirm actual leaks + +### Phase 3: Stress Testing +1. Join session, enable all features (chat, VU meters, players) +2. Monitor memory usage in Chrome Task Manager +3. Run for 15+ minutes +4. Memory should plateau, not grow linearly +5. Document specific growth patterns if found + +## State of the Art + +| Old Approach | Current Approach | When Changed | Impact | +|--------------|------------------|--------------|--------| +| Manual timer tracking | useRef for timer IDs | React 16.8+ | Cleaner cleanup, survives re-renders | +| isMounted flag pattern | AbortController / useEffect cleanup | React 17+ | Better async cancellation | +| ComponentWillUnmount | useEffect return function | React 16.8+ | Hooks-based cleanup | + +**Deprecated/outdated:** +- `isMounted` pattern: Considered an anti-pattern, use AbortController instead +- Direct DOM manipulation for cleanup: Use React refs and effects + +## Open Questions + +1. **What causes the 10-minute freeze specifically?** + - What we know: App freezes, likely memory-related + - What's unclear: Specific trigger - is it memory exhaustion or GC pause? + - Recommendation: Add performance monitoring to pinpoint exact moment + +2. **WebSocket callback lifecycle** + - What we know: Callbacks registered on join, should unregister on leave + - What's unclear: Are they always unregistered on all exit paths (error, timeout)? + - Recommendation: Trace all exit paths in JKSessionScreen + +3. **Redux message limit** + - What we know: Messages accumulate in messagesByChannel + - What's unclear: What's the typical message volume per session? + - Recommendation: Implement 500-message limit as starting point + +## Sources + +### Primary (HIGH confidence) +- Codebase analysis of jam-ui/src components and hooks +- [React & useEffect cleanups | Tasos Kakouris](https://tasoskakour.com/blog/react-use-effect-cleanups/) +- [Chrome DevTools Memory Panel | Chrome for Developers](https://developer.chrome.com/docs/devtools/memory) +- [Fix memory problems | Chrome DevTools](https://developer.chrome.com/docs/devtools/memory-problems) + +### Secondary (MEDIUM confidence) +- [Understanding Memory Leaks in React | Medium](https://medium.com/@ignatovich.dm/understanding-memory-leaks-in-react-how-to-find-and-fix-them-fc782cf182be) +- [How to Fix Memory Leaks in React | freeCodeCamp](https://www.freecodecamp.org/news/fix-memory-leaks-in-react-apps/) +- [Isolating memory leaks with Chrome's Allocation Timeline | LogRocket](https://blog.logrocket.com/isolating-memory-leaks-with-chromes-allocation-timeline-244fa9c48e8e/) +- [Detect and Prevent Memory Leaks in React.js | DhiWise](https://www.dhiwise.com/post/the-complete-guide-to-detect-and-prevent-memory-leaks-in-react-js) + +### Tertiary (LOW confidence) +- General React memory leak best practices from training data + +## Metadata + +**Confidence breakdown:** +- Standard stack: HIGH - Chrome DevTools and React patterns are well-documented +- Architecture patterns: HIGH - Based on direct codebase analysis +- Pitfalls: HIGH - Common React patterns, verified against codebase + +**Research date:** 2026-02-08 +**Valid until:** 30 days (stable React patterns) + +## Appendix: Files to Audit + +### Primary Focus (Session Screen Components) +- `/jam-ui/src/components/client/JKSessionScreen.js` +- `/jam-ui/src/components/client/SessionTrackVU.js` +- `/jam-ui/src/components/client/JKSessionChatWindow.js` +- `/jam-ui/src/components/client/chat/JKChatMessageList.js` +- `/jam-ui/src/components/client/JKSessionBackingTrackPlayer.js` +- `/jam-ui/src/components/client/JKSessionJamTrackPlayer.js` + +### Supporting Hooks +- `/jam-ui/src/hooks/useJamServer.js` +- `/jam-ui/src/hooks/useMixerStore.js` +- `/jam-ui/src/hooks/useMixerHelper.js` +- `/jam-ui/src/hooks/useVuHelpers.js` +- `/jam-ui/src/hooks/useSessionStats.js` +- `/jam-ui/src/hooks/useSessionModel.js` + +### Context Providers +- `/jam-ui/src/context/VuContext.js` +- `/jam-ui/src/context/MixersContext.js` +- `/jam-ui/src/context/JamServerContext.js` + +### Redux Slices +- `/jam-ui/src/store/features/sessionChatSlice.js` +- `/jam-ui/src/store/features/activeSessionSlice.js` +- `/jam-ui/src/store/features/mixersSlice.js`