From e9ff93d3de1d5b6add6217427e73800eea73a9f3 Mon Sep 17 00:00:00 2001 From: Nuwan Date: Sun, 8 Feb 2026 14:17:50 +0530 Subject: [PATCH] audit(19-01): chat window components memory leak audit - Audited JKSessionChatWindow.js, JKChatMessageList.js, sessionChatSlice.js - Found CHAT-01: Messages accumulate without limit (HIGH) - Found CHAT-02: chatState in useEffect dependencies (LOW) - Found CHAT-03: No message cleanup on session leave (MEDIUM) - Verified WebSocket callback cleanup in useSessionWebSocket.js Co-Authored-By: Claude Opus 4.5 --- .../phases/19-audit-and-discovery/19-AUDIT.md | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/.planning/phases/19-audit-and-discovery/19-AUDIT.md b/.planning/phases/19-audit-and-discovery/19-AUDIT.md index a42e22c0d..4299f1142 100644 --- a/.planning/phases/19-audit-and-discovery/19-AUDIT.md +++ b/.planning/phases/19-audit-and-discovery/19-AUDIT.md @@ -117,3 +117,116 @@ When tracks are added/removed during a session, the `vuStates` object grows but --- +## Chat Window Audit + +**Files Audited:** +- `jam-ui/src/components/client/JKSessionChatWindow.js` +- `jam-ui/src/components/client/chat/JKChatMessageList.js` +- `jam-ui/src/store/features/sessionChatSlice.js` +- `jam-ui/src/hooks/useSessionWebSocket.js` + +**Requirements Coverage:** CHAT-01, CHAT-02, CHAT-03 + +### JKSessionChatWindow.js + +| Line | Pattern | Description | Has Cleanup | Severity | Notes | +|------|---------|-------------|-------------|----------|-------| +| 59-67 | useEffect + addEventListener | Keyboard listener for Escape key | YES | LOW | Properly removes listener on cleanup (line 66) | +| 70-79 | useEffect + setTimeout | Focus textarea on window open | NO | LOW | Short 100ms timeout, not a significant leak | + +**Analysis:** The event listener pattern is properly implemented with cleanup. The setTimeout at line 73 for focus delay is a one-time short delay and does not require explicit cleanup. + +### JKChatMessageList.js + +| Line | Pattern | Description | Has Cleanup | Severity | Notes | +|------|---------|-------------|-------------|----------|-------| +| 44 | useRef | `scrollTimeoutRef` for debounce tracking | YES | LOW | Proper ref usage | +| 86-93 | setTimeout | Scroll debounce timeout | YES | LOW | Cleared on next scroll (line 79) | +| 112-113 | setTimeout | Initial scroll delay | NO | LOW | One-time 100ms delay, no cleanup needed | +| 120-126 | useEffect | Cleanup effect for scrollTimeoutRef | YES | LOW | Properly clears timeout on unmount | + +**Analysis:** The scroll debounce pattern is well implemented. The `scrollTimeoutRef` is cleared both when a new scroll occurs (line 79) and on unmount (lines 122-124). The initial scroll timeout at line 112 is a one-time delay that doesn't require cleanup. + +### sessionChatSlice.js (CRITICAL) + +| Line | Pattern | Description | Has Cleanup | Severity | Notes | +|------|---------|-------------|-------------|----------|-------| +| 120 | initialState | `messagesByChannel: {}` - message storage | N/A | HIGH | Messages accumulate without limit | +| 202 | push | `state.messagesByChannel[channel].push(message)` | N/A | HIGH | Unbounded array growth | +| 366 | spread + concat | `[...newMessages, ...state.messagesByChannel[channel]]` | N/A | HIGH | Merges without limiting | +| 418 | push | Optimistic message push | N/A | MEDIUM | Part of the same pattern | +| 512 | push | Attachment message push | N/A | MEDIUM | Part of the same pattern | + +**Analysis:** + +**CHAT-01 - CRITICAL FINDING:** The `messagesByChannel` Redux state has **NO MAX MESSAGE LIMIT**: + +```javascript +// Line 120: Initial state +messagesByChannel: {}, + +// Line 202: WebSocket messages pushed without limit +state.messagesByChannel[channel].push(message); + +// Line 366: Pagination merges without limit +state.messagesByChannel[channel] = [...newMessages, ...state.messagesByChannel[channel]]; +``` + +During a 10-minute session with active chat: +- At ~5 messages/minute = 50+ messages +- Each message object includes: id, senderId, senderName, message, createdAt, channel, attachments +- With file attachments, message objects can be substantial in size +- **No mechanism exists to limit or purge old messages** + +**Evidence of unbounded growth:** +- Line 202: `push(message)` - adds every incoming WebSocket message +- Line 366: Spread operator concatenates all fetched history +- Line 512: Attachment messages add to the same arrays +- **No slice(), splice(), or max-length check anywhere in the reducer** + +### useSessionWebSocket.js + +| Line | Pattern | Description | Has Cleanup | Severity | Notes | +|------|---------|-------------|-------------|----------|-------| +| 74-308 | useEffect | WebSocket callback registration | YES | LOW | Properly unregisters all callbacks on cleanup (lines 299-306) | +| 289-296 | forEach | Callback registration loop | YES | LOW | Each callback stored for later unregistration | +| 300-306 | forEach | Cleanup unregistration loop | YES | LOW | Iterates same callbacks object | + +**Analysis:** The WebSocket callback lifecycle is properly managed: + +```javascript +// Lines 289-296: Registration +Object.entries(callbacks).forEach(([event, callback]) => { + if (jamServer.on) { + jamServer.on(event, callback); + } else if (jamServer.registerMessageCallback) { + jamServer.registerMessageCallback(event, callback); + } +}); + +// Lines 299-306: Cleanup +return () => { + Object.entries(callbacks).forEach(([event, callback]) => { + if (jamServer.off) { + jamServer.off(event, callback); + } else if (jamServer.unregisterMessageCallback) { + jamServer.unregisterMessageCallback(event, callback); + } + }); +}; +``` + +The `callbacks` object is defined within the useEffect, so the cleanup function correctly references all registered callbacks. + +**CHAT-02 - POTENTIAL ISSUE:** The `chatState` selector at line 72 is included in the dependency array at line 308. Since `chatState` includes `isWindowOpen` and `activeChannel`, this effect could re-run more often than necessary, potentially causing multiple callback registrations/unregistrations. However, this is a performance issue, not a memory leak. + +### Chat Window Findings Summary + +| ID | Finding | File | Lines | Severity | Fix Recommendation | +|----|---------|------|-------|----------|-------------------| +| CHAT-01 | Messages accumulate without limit | sessionChatSlice.js | 202, 366, 512 | HIGH | Implement MAX_MESSAGES limit (e.g., 500 messages per channel) | +| CHAT-02 | chatState in useEffect dependencies | useSessionWebSocket.js | 308 | LOW | Extract only needed values from chatState, or use useRef | +| CHAT-03 | No message cleanup on session leave | sessionChatSlice.js | N/A | MEDIUM | Add clearChannel action called when leaving session | + +--- +