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 <noreply@anthropic.com>
This commit is contained in:
parent
1c898ddc0b
commit
e9ff93d3de
|
|
@ -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 |
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue