fix: prevent renderer memory leak with lazy subagent rendering#168
fix: prevent renderer memory leak with lazy subagent rendering#168psypeal wants to merge 1 commit intomatt1398:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a custom session refresh mechanism for Electron, adds a specialized Bash tool viewer with syntax highlighting and collapsible output, and enhances Read/Write tools with Markdown previews. It also significantly expands syntax highlighting support and introduces state management optimizations to clear UI expansion states. Feedback identifies a critical race condition in the new pruning logic that could cause in-flight requests to be ignored and suggests refactoring the syntax highlighter for improved maintainability.
| if (sessionRefreshGeneration.size > 100) { | ||
| const entries = Array.from(sessionRefreshGeneration.entries()) | ||
| .sort((a, b) => b[1] - a[1]) | ||
| .slice(0, 50); | ||
| sessionRefreshGeneration.clear(); | ||
| for (const [key, value] of entries) { | ||
| sessionRefreshGeneration.set(key, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
The pruning logic for sessionRefreshGeneration has a correctness issue and a logic flaw:
- Race Condition / Correctness Bug: If a pruning event occurs (triggered by any session refresh) while a specific session refresh is in flight (awaiting the API call at line 524), the
refreshKeyfor that in-flight request might be evicted from the Map. When the API call returns, the check at line 527 (sessionRefreshGeneration.get(refreshKey) !== generation) will fail because the key is missing (returningundefined), causing the legitimate update to be silently dropped. - Recency vs. Frequency: Sorting by the generation counter (
b[1] - a[1]) keeps the most frequently refreshed sessions, not the most recently refreshed ones as stated in the PR description. A session refreshed 100 times yesterday will be prioritized over a session refreshed for the first time just now.
Given that these Maps only store a number per session ID, the memory overhead is negligible (a few hundred KB for 10,000 sessions). It is safer to remove this pruning entirely. If pruning is required, ensure that keys currently in sessionRefreshInFlight are never evicted and use a proper LRU approach (e.g., by re-inserting keys to leverage JS Map's insertion order).
| if (projectRefreshGeneration.size > 100) { | ||
| const entries = Array.from(projectRefreshGeneration.entries()) | ||
| .sort((a, b) => b[1] - a[1]) | ||
| .slice(0, 50); | ||
| projectRefreshGeneration.clear(); | ||
| for (const [key, value] of entries) { | ||
| projectRefreshGeneration.set(key, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
This pruning logic suffers from the same issues identified in sessionDetailSlice.ts. It can cause in-flight project refreshes to be ignored if their key is evicted during the API call, and it incorrectly uses frequency (counter value) as a proxy for recency.
| if ( | ||
| (language === 'python' || language === 'bash' || language === 'r' || language === 'ruby' || language === 'php') && | ||
| (language === 'python' || language === 'bash' || language === 'zsh' || language === 'fish' || | ||
| language === 'r' || language === 'ruby' || language === 'php' || language === 'yaml') && | ||
| remaining.startsWith('#') |
There was a problem hiding this comment.
The list of languages using # for comments is growing. To improve maintainability and readability, consider defining a Set of languages for each comment style at the top of the file (e.g., const HASH_COMMENT_LANGUAGES = new Set(['python', 'bash', ...])) and checking against that set here.
References
- Adhering to general clean code principles for maintainability by avoiding long conditional chains when a Set lookup is more appropriate. (link)
📝 WalkthroughWalkthroughThis PR introduces lazy loading for subagent messages with client-side caching, adds a new IPC channel for message retrieval, precomputes subagent display metadata to avoid runtime message scanning, and implements memory management improvements (generational tracking pruning, LRU cache eviction) across store slices. Changes
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx (1)
31-57: Consider extracting shared markdown toggle component.The toggle button UI (lines 31-57) is nearly identical to
ReadToolViewer.tsx(lines 62-88). Consider extracting a sharedMarkdownTogglecomponent to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx` around lines 31 - 57, Extract the duplicated markdown toggle UI into a shared component (e.g., MarkdownToggle) and use it from both WriteToolViewer and ReadToolViewer: identify the repeated JSX block that reads/writes viewMode and setViewMode in WriteToolViewer.tsx and the similar block in ReadToolViewer.tsx, create a new MarkdownToggle component that accepts props {viewMode, setViewMode} (or onChange and value) and encapsulates the two buttons and their styling/behavior, then replace the in-file JSX in both WriteToolViewer and ReadToolViewer with <MarkdownToggle .../> calls passing the existing viewMode and setViewMode handlers.src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx (2)
10-17: Keep the@rendererimport block above the relative imports.This new file currently breaks the repo ordering rule by placing
@renderer/types/groupsafter../and./imports.As per coding guidelines, "Organize imports in order: external packages, path aliases (
@main,@renderer,@shared,@preload), then relative imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx` around lines 10 - 17, The import ordering in BashToolViewer.tsx is incorrect: move all `@renderer` path-alias imports (e.g., the LinkedToolItem import from '@renderer/types/groups' and CodeBlockViewer from '@renderer/components/chat/viewers') above the relative imports (../BaseItem, ./CollapsibleOutputSection, ./renderHelpers) so the file follows the rule "external packages, path aliases (`@renderer`, etc.), then relative imports"; update the import block so CodeBlockViewer and LinkedToolItem appear before ItemStatus, CollapsibleOutputSection, and renderOutput.
24-26: Implement a type guard for Bash tool input to validate string types before casting.The upstream guard
hasBashContent()only checks truthiness ofinput.command, not whether it's actually a string. A malformed payload withcommand: 123orcommand: {}would pass the guard and then be unsafely cast inBashToolViewer. Following the isXxx naming convention from the coding guidelines, add anisBashToolInput()guard intoolContentChecks.tsthat validates bothcommandanddescriptionare strings or undefined, then use it inBashToolViewerinstead of rawas stringcasts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx` around lines 24 - 26, The BashToolViewer currently casts linkedTool.input.command/description unsafely (using as string) and relies on hasBashContent(), which only checks truthiness; add an isBashToolInput() type guard in toolContentChecks.ts that returns true only when input.command is a string and input.description is either a string or undefined, then update BashToolViewer to call isBashToolInput(linkedTool.input) before reading values and avoid direct casts (handle the invalid case by returning null or a safe fallback UI). Ensure names match the isXxx convention and reference isBashToolInput, hasBashContent, BashToolViewer, and linkedTool.input.command/description when making changes.src/main/index.ts (1)
487-495: Consider mirroringSESSION_REFRESHin the local constants block.This file already duplicates preload channel names locally to avoid a main→preload import. Inlining
'session:refresh'at Line 494 makes that one channel easier to drift during a future rename.🧹 Small cleanup
const HTTP_SERVER_GET_STATUS = 'httpServer:getStatus'; +const SESSION_REFRESH = 'session:refresh'; ... - mainWindow.webContents.send('session:refresh'); + mainWindow.webContents.send(SESSION_REFRESH);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 487 - 495, Replace the inlined channel string 'session:refresh' with a local constant declared alongside the other preload channel names and use that constant in mainWindow.webContents.send; specifically, add a constant (e.g., SESSION_REFRESH) in the local constants block where other channel names are duplicated and change the send call from mainWindow.webContents.send('session:refresh') to mainWindow.webContents.send(SESSION_REFRESH) so the channel name mirrors the existing constants and avoids drift on renames.src/renderer/store/slices/sessionDetailSlice.ts (1)
407-423: Consider usinggetConversationExpansionResetState()for consistency.The inline reset of expansion Maps/Sets duplicates the logic in
getConversationExpansionResetState(). If the helper is updated (e.g., new expansion structures added), this inline version could drift out of sync.♻️ Proposed refactor to use the helper
+import { getConversationExpansionResetState } from '../utils/stateResetHelpers'; + // In fetchSessionDetail, around line 407: - // Clear expansion Maps/Sets from previous session to prevent unbounded growth over long uptime - set({ - aiGroupExpansionLevels: new Map(), - expandedStepIds: new Set(), - expandedDisplayItemIds: new Map(), - expandedAIGroupIds: new Set(), - activeDetailItem: null, + set({ + ...getConversationExpansionResetState(), sessionDetail: detail, sessionDetailLoading: false, // ... rest of the state });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/store/slices/sessionDetailSlice.ts` around lines 407 - 423, The inline reset of expansion Maps/Sets duplicates logic in getConversationExpansionResetState(); replace the explicit entries (aiGroupExpansionLevels, expandedStepIds, expandedDisplayItemIds, expandedAIGroupIds, activeDetailItem) inside the set({...}) call by calling getConversationExpansionResetState() and spreading its return into the object so the expansion reset remains centralized and future changes to expansion structures only need to be made in getConversationExpansionResetState(); ensure you still assign sessionDetail, sessionDetailLoading, conversation, conversationLoading, visibleAIGroupId, selectedAIGroup, sessionClaudeMdStats, sessionContextStats, and sessionPhaseInfo as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/chat/ChatHistory.tsx`:
- Around line 380-387: The global 'session-refresh-scroll-bottom' listener in
ChatHistory subscribes across all mounted tabs and must be scoped per-tab:
change the useEffect handler in ChatHistory to accept the CustomEvent, read
event.detail.tabId, and only call scrollToBottom('smooth') when that tabId
matches this ChatHistory's tab identifier (e.g., the prop/state holding this
tab's id); also update refresh dispatch sites to send the starting tab id via
window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom', { detail:
{ tabId: activeTabId } })). Ensure references to scrollToBottom, ChatHistory,
window.addEventListener, and CustomEvent.detail are used so the handler ignores
events for other tabs.
In `@src/renderer/components/chat/items/LinkedToolItem.tsx`:
- Around line 149-150: LinkedToolItem.tsx is passing variant={toolVariant} to
BaseItem but BaseItemProps (in BaseItem.tsx) doesn't declare a variant prop; add
a variant field to the BaseItemProps interface (e.g., variant?: StepVariant) and
update BaseItem component's props type to accept and forward that prop (use the
existing StepVariant type or import it where BaseItem is declared) so BaseItem
can use variant for styling; ensure any usages of BaseItem (including
LinkedToolItem) still type-check and adjust default behavior in BaseItem if
variant is optional.
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 924-929: The current comment detection in the if block that checks
(language === ... ) && remaining.startsWith('#') is too naive; update the check
around the variables language and remaining (the comment-start branch in
syntaxHighlighter.ts) to only treat '#' as a comment when it is at the start of
the token or is preceded by a boundary (e.g., start-of-string or whitespace or
an operator/punctuation like = : , ; ( [ { | & >) and not when embedded in
words/variable expansions or URLs; implement this by replacing
remaining.startsWith('#') with a context-aware check (e.g., a regex or explicit
lookbehind/peek at the preceding character) that excludes cases like `${`#var`}`,
`${var#prefix}` and `http://...#frag` while still matching real comments in
shell/YAML.
In `@src/renderer/store/slices/sessionSlice.ts`:
- Around line 260-269: The pruning logic using projectRefreshGeneration and
per-project generation can evict a just-started refresh because generation is
not a global recency metric; modify the pruning to preserve recency correctly by
either (A) tracking last-touch order separately (e.g., maintain a lastTouched
map or timestamp per key and sort by that when pruning) or (B) explicitly never
prune the current/in-flight key (detect the in-flight project id and always keep
it) instead of sorting by generation; apply the same change to the identical
helper in src/renderer/store/slices/sessionDetailSlice.ts so both slices use
last-touch timestamps or honor the in-flight key when pruning.
---
Nitpick comments:
In `@src/main/index.ts`:
- Around line 487-495: Replace the inlined channel string 'session:refresh' with
a local constant declared alongside the other preload channel names and use that
constant in mainWindow.webContents.send; specifically, add a constant (e.g.,
SESSION_REFRESH) in the local constants block where other channel names are
duplicated and change the send call from
mainWindow.webContents.send('session:refresh') to
mainWindow.webContents.send(SESSION_REFRESH) so the channel name mirrors the
existing constants and avoids drift on renames.
In `@src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx`:
- Around line 10-17: The import ordering in BashToolViewer.tsx is incorrect:
move all `@renderer` path-alias imports (e.g., the LinkedToolItem import from
'@renderer/types/groups' and CodeBlockViewer from
'@renderer/components/chat/viewers') above the relative imports (../BaseItem,
./CollapsibleOutputSection, ./renderHelpers) so the file follows the rule
"external packages, path aliases (`@renderer`, etc.), then relative imports";
update the import block so CodeBlockViewer and LinkedToolItem appear before
ItemStatus, CollapsibleOutputSection, and renderOutput.
- Around line 24-26: The BashToolViewer currently casts
linkedTool.input.command/description unsafely (using as string) and relies on
hasBashContent(), which only checks truthiness; add an isBashToolInput() type
guard in toolContentChecks.ts that returns true only when input.command is a
string and input.description is either a string or undefined, then update
BashToolViewer to call isBashToolInput(linkedTool.input) before reading values
and avoid direct casts (handle the invalid case by returning null or a safe
fallback UI). Ensure names match the isXxx convention and reference
isBashToolInput, hasBashContent, BashToolViewer, and
linkedTool.input.command/description when making changes.
In `@src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx`:
- Around line 31-57: Extract the duplicated markdown toggle UI into a shared
component (e.g., MarkdownToggle) and use it from both WriteToolViewer and
ReadToolViewer: identify the repeated JSX block that reads/writes viewMode and
setViewMode in WriteToolViewer.tsx and the similar block in ReadToolViewer.tsx,
create a new MarkdownToggle component that accepts props {viewMode, setViewMode}
(or onChange and value) and encapsulates the two buttons and their
styling/behavior, then replace the in-file JSX in both WriteToolViewer and
ReadToolViewer with <MarkdownToggle .../> calls passing the existing viewMode
and setViewMode handlers.
In `@src/renderer/store/slices/sessionDetailSlice.ts`:
- Around line 407-423: The inline reset of expansion Maps/Sets duplicates logic
in getConversationExpansionResetState(); replace the explicit entries
(aiGroupExpansionLevels, expandedStepIds, expandedDisplayItemIds,
expandedAIGroupIds, activeDetailItem) inside the set({...}) call by calling
getConversationExpansionResetState() and spreading its return into the object so
the expansion reset remains centralized and future changes to expansion
structures only need to be made in getConversationExpansionResetState(); ensure
you still assign sessionDetail, sessionDetailLoading, conversation,
conversationLoading, visibleAIGroupId, selectedAIGroup, sessionClaudeMdStats,
sessionContextStats, and sessionPhaseInfo as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c00556c9-a8c2-46f4-8f34-0af83d41001e
📒 Files selected for processing (23)
src/main/index.tssrc/preload/constants/ipcChannels.tssrc/preload/index.tssrc/renderer/api/httpClient.tssrc/renderer/components/chat/ChatHistory.tsxsrc/renderer/components/chat/items/LinkedToolItem.tsxsrc/renderer/components/chat/items/linkedTool/BashToolViewer.tsxsrc/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsxsrc/renderer/components/chat/items/linkedTool/DefaultToolViewer.tsxsrc/renderer/components/chat/items/linkedTool/ReadToolViewer.tsxsrc/renderer/components/chat/items/linkedTool/WriteToolViewer.tsxsrc/renderer/components/chat/items/linkedTool/index.tssrc/renderer/components/chat/viewers/syntaxHighlighter.tssrc/renderer/components/layout/TabBar.tsxsrc/renderer/hooks/useKeyboardShortcuts.tssrc/renderer/store/index.tssrc/renderer/store/slices/sessionDetailSlice.tssrc/renderer/store/slices/sessionSlice.tssrc/renderer/store/slices/tabSlice.tssrc/renderer/store/utils/stateResetHelpers.tssrc/renderer/utils/toolRendering/index.tssrc/renderer/utils/toolRendering/toolContentChecks.tssrc/shared/types/api.ts
| // Listen for session-refresh-scroll-bottom events (from Ctrl+R / refresh button) | ||
| useEffect(() => { | ||
| const handler = (): void => { | ||
| scrollToBottom('smooth'); | ||
| }; | ||
| window.addEventListener('session-refresh-scroll-bottom', handler); | ||
| return () => window.removeEventListener('session-refresh-scroll-bottom', handler); | ||
| }, [scrollToBottom]); |
There was a problem hiding this comment.
Scope this scroll event to a single tab.
Inactive session tabs stay mounted, so every ChatHistory instance subscribes to this global event. Refreshing one tab will scroll background tabs to bottom too, and if the active tab changes before the async refresh finishes, the wrong tab can jump. Pass the refreshed tabId in CustomEvent.detail and ignore events for other tabs here.
🎯 Suggested fix
useEffect(() => {
- const handler = (): void => {
+ const handler = (event: Event): void => {
+ const { tabId: targetTabId } =
+ (event as CustomEvent<{ tabId?: string }>).detail ?? {};
+ if (targetTabId !== effectiveTabId) return;
scrollToBottom('smooth');
};
window.addEventListener('session-refresh-scroll-bottom', handler);
return () => window.removeEventListener('session-refresh-scroll-bottom', handler);
- }, [scrollToBottom]);
+ }, [effectiveTabId, scrollToBottom]);Update the refresh call sites to dispatch the tab id captured when the refresh starts:
window.dispatchEvent(
new CustomEvent('session-refresh-scroll-bottom', { detail: { tabId: activeTabId } })
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/chat/ChatHistory.tsx` around lines 380 - 387, The
global 'session-refresh-scroll-bottom' listener in ChatHistory subscribes across
all mounted tabs and must be scoped per-tab: change the useEffect handler in
ChatHistory to accept the CustomEvent, read event.detail.tabId, and only call
scrollToBottom('smooth') when that tabId matches this ChatHistory's tab
identifier (e.g., the prop/state holding this tab's id); also update refresh
dispatch sites to send the starting tab id via window.dispatchEvent(new
CustomEvent('session-refresh-scroll-bottom', { detail: { tabId: activeTabId }
})). Ensure references to scrollToBottom, ChatHistory, window.addEventListener,
and CustomEvent.detail are used so the handler ignores events for other tabs.
| // Determine step variant for colored borders/icons | ||
| const toolVariant: StepVariant = status === 'error' ? 'tool-error' : 'tool'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify BaseItemProps interface definition
echo "=== Checking BaseItemProps interface ==="
rg -n -A 30 'interface BaseItemProps' --type=ts
echo ""
echo "=== Checking if variant prop exists anywhere in BaseItem ==="
rg -n 'variant' src/renderer/components/chat/items/BaseItem.tsxRepository: matt1398/claude-devtools
Length of output: 2809
🏁 Script executed:
#!/bin/bash
# Verify LinkedToolItem.tsx around line 175
echo "=== Checking LinkedToolItem.tsx lines 170-180 ==="
sed -n '170,180p' src/renderer/components/chat/items/LinkedToolItem.tsx
echo ""
echo "=== Full BaseItem usage in LinkedToolItem.tsx ==="
rg -n -B 5 -A 5 'BaseItem' src/renderer/components/chat/items/LinkedToolItem.tsx | head -50Repository: matt1398/claude-devtools
Length of output: 1585
variant prop is not defined on BaseItemProps.
LinkedToolItem.tsx passes variant={toolVariant} to BaseItem (line 175), but the BaseItemProps interface in BaseItem.tsx does not accept a variant prop. This will cause a TypeScript error or the prop will be silently ignored at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/chat/items/LinkedToolItem.tsx` around lines 149 -
150, LinkedToolItem.tsx is passing variant={toolVariant} to BaseItem but
BaseItemProps (in BaseItem.tsx) doesn't declare a variant prop; add a variant
field to the BaseItemProps interface (e.g., variant?: StepVariant) and update
BaseItem component's props type to accept and forward that prop (use the
existing StepVariant type or import it where BaseItem is declared) so BaseItem
can use variant for styling; ensure any usages of BaseItem (including
LinkedToolItem) still type-check and adjust default behavior in BaseItem if
variant is optional.
| // Check for comment (# style for Python/Shell/R/Ruby/PHP/YAML) | ||
| if ( | ||
| (language === 'python' || language === 'bash' || language === 'r' || language === 'ruby' || language === 'php') && | ||
| (language === 'python' || language === 'bash' || language === 'zsh' || language === 'fish' || | ||
| language === 'r' || language === 'ruby' || language === 'php' || language === 'yaml') && | ||
| remaining.startsWith('#') | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the current rule in the file, then show representative inputs that
# would be classified as comments by the same predicate.
sed -n '924,929p' src/renderer/components/chat/viewers/syntaxHighlighter.ts
python <<'PY'
samples = [
('bash', 'echo ${`#var`}'),
('bash', 'echo ${var#prefix}'),
('yaml', 'url: http://example.com/#frag'),
]
for language, line in samples:
for i in range(len(line)):
if line[i:].startswith('#'):
print(f"{language}: {line!r} -> current rule would start a comment at index {i}")
break
PYRepository: matt1398/claude-devtools
Length of output: 616
Hash comments are too eager for shell and YAML.
The # check treats any occurrence as a comment start, which mis-highlights valid syntax like echo ${#var}, echo ${var#prefix}, and YAML URLs like http://example.com/#frag. Add context-aware boundary checks (e.g., preceded by whitespace or operator) before treating # as a comment start.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts` around lines 924 -
929, The current comment detection in the if block that checks (language === ...
) && remaining.startsWith('#') is too naive; update the check around the
variables language and remaining (the comment-start branch in
syntaxHighlighter.ts) to only treat '#' as a comment when it is at the start of
the token or is preceded by a boundary (e.g., start-of-string or whitespace or
an operator/punctuation like = : , ; ( [ { | & >) and not when embedded in
words/variable expansions or URLs; implement this by replacing
remaining.startsWith('#') with a context-aware check (e.g., a regex or explicit
lookbehind/peek at the preceding character) that excludes cases like `${`#var`}`,
`${var#prefix}` and `http://...#frag` while still matching real comments in
shell/YAML.
| // Prune to prevent unbounded growth: when exceeding 100 entries, keep only the 50 most recent | ||
| if (projectRefreshGeneration.size > 100) { | ||
| const entries = Array.from(projectRefreshGeneration.entries()) | ||
| .sort((a, b) => b[1] - a[1]) | ||
| .slice(0, 50); | ||
| projectRefreshGeneration.clear(); | ||
| for (const [key, value] of entries) { | ||
| projectRefreshGeneration.set(key, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
This pruning can evict the refresh that just started.
generation is only per-project, so it does not represent global recency. After 101 projects have each refreshed once, the newly touched project still has generation 1 and can be pruned immediately here; then Line 280 reads undefined !== generation and drops its own fresh response. Please track last-touch order separately, or never prune the current/in-flight key, instead of sorting by generation count. The same helper in src/renderer/store/slices/sessionDetailSlice.ts should be updated too.
🧩 One recency-safe approach
+const projectRefreshTouchedAt = new Map<string, number>();
+let projectRefreshSequence = 0;
...
const generation = (projectRefreshGeneration.get(projectId) ?? 0) + 1;
projectRefreshGeneration.set(projectId, generation);
+projectRefreshTouchedAt.set(projectId, ++projectRefreshSequence);
if (projectRefreshGeneration.size > 100) {
- const entries = Array.from(projectRefreshGeneration.entries())
- .sort((a, b) => b[1] - a[1])
- .slice(0, 50);
- projectRefreshGeneration.clear();
- for (const [key, value] of entries) {
- projectRefreshGeneration.set(key, value);
+ const staleKeys = Array.from(projectRefreshTouchedAt.entries())
+ .filter(([key]) => key !== projectId)
+ .sort((a, b) => a[1] - b[1])
+ .slice(0, projectRefreshGeneration.size - 50)
+ .map(([key]) => key);
+ for (const key of staleKeys) {
+ projectRefreshGeneration.delete(key);
+ projectRefreshTouchedAt.delete(key);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/store/slices/sessionSlice.ts` around lines 260 - 269, The
pruning logic using projectRefreshGeneration and per-project generation can
evict a just-started refresh because generation is not a global recency metric;
modify the pruning to preserve recency correctly by either (A) tracking
last-touch order separately (e.g., maintain a lastTouched map or timestamp per
key and sort by that when pruning) or (B) explicitly never prune the
current/in-flight key (detect the in-flight project id and always keep it)
instead of sorting by generation; apply the same change to the identical helper
in src/renderer/store/slices/sessionDetailSlice.ts so both slices use last-touch
timestamps or honor the in-flight key when pruning.
…ion state Integrates psypeal/claude-devtools#168 into fork with conflict resolution: - ipcChannels.ts: kept both PR's session:refresh and upstream's search API channels - linkedTool/index.ts: added BashToolViewer export from PR - sessionDetailSlice.ts: merged PR's expansion state clearing with upstream's slimDetail optimization Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Clear expansion Maps/Sets (aiGroupExpansionLevels, expandedStepIds, expandedDisplayItemIds, expandedAIGroupIds) when switching sessions to prevent unbounded growth over long uptime - Add periodic pruning for sessionRefreshGeneration and projectRefreshGeneration Maps - Add SubagentDisplayMeta type to Process for memory-efficient subagent rendering without holding full transcripts - Add subagentMessageCacheSlice (Zustand) with single-flight Promise dedup and LRU eviction for on-demand subagent message loading - Refactor SubagentItem to read displayMeta for collapsed headers and lazy-load message bodies only when expanded via useSubagentMessages - Update AIChatGroup and modelExtractor to prefer displayMeta over iterating raw messages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
psypeal
force-pushed
the
fix/renderer-memory-leak
branch
from
106665c to
4b8c12a
Compare
psypeal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/renderer/store/index.ts (1)
88-93: Consider adding a fallback or documenting the limitation.The
performance.memoryAPI is Chrome-specific and non-standard. While returning0when unavailable is a safe fallback, this means memory pressure gating will be disabled in non-Chromium environments. Since this is an Electron app (Chromium-based), this should work, but the code comment could clarify this assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/store/index.ts` around lines 88 - 93, Update getRendererHeapMB to either document the Chromium-only assumption or add a graceful fallback: clarify in the function comment that performance.memory is a Chromium/Renderer-only API (Electron) and will return 0 in non-Chromium environments, or implement an alternative/path (e.g., use process.getProcessMemoryInfo or IPC to main which can call process.getProcessMemoryInfo) when performance.memory is unavailable; reference the function name getRendererHeapMB to locate where to add the comment or fallback logic.src/renderer/store/slices/subagentMessageCacheSlice.ts (1)
158-176: Consider clearinginflightPromisesininvalidateSubagentMessagesForSession.The
clearSubagentMessageCachemethod correctly clearsinflightPromises, butinvalidateSubagentMessagesForSessiondoes not. If a fetch is in-flight when a session switch triggers invalidation, the fetch will complete and re-populate the cache with potentially stale data.However, this is a minor edge case since:
- Session switches are infrequent
- The cache is small (10 entries)
- The data would be from the same session that was just invalidated
♻️ Optional: Clear in-flight promises on session invalidation
invalidateSubagentMessagesForSession: (_sessionId) => { // We only key by subagentId in the cache (no session in the key), so // a coarse clear is correct: when any session refreshes we drop all // cached subagent bodies. The cache is small so this is cheap. + inflightPromises.clear(); set({ subagentMessageCache: new Map(), subagentMessageErrors: new Map(), }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/store/slices/subagentMessageCacheSlice.ts` around lines 158 - 176, The invalidateSubagentMessagesForSession function currently resets subagentMessageCache and subagentMessageErrors but does not clear inflightPromises, so in-flight fetches can repopulate the cache with stale data; update invalidateSubagentMessagesForSession to also call inflightPromises.clear() (same as clearSubagentMessageCache) so any pending promises are discarded when invalidating, keeping the cache and inflightPromises consistent (refer to invalidateSubagentMessagesForSession, clearSubagentMessageCache, inflightPromises, subagentMessageCache, and subagentMessageErrors).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/chat/AIChatGroup.tsx`:
- Around line 27-28: ChatHistory now pre-computes preceding-slash info but
ChatHistoryItem fails to pass it down to AIChatGroup; update ChatHistory to
compute the precedingSlash map (using the existing extractPrecedingSlashInfo or
the pre-computed map already added), thread that value through ChatHistoryItem
(add a precedingSlash prop to its props) and then pass precedingSlash into the
AIChatGroup instantiation (the component expecting the precedingSlash prop),
ensuring AIChatGroup receives the pre-computed value for each aiGroup so
slash-command linking works.
In `@src/renderer/hooks/useSubagentMessages.ts`:
- Line 50: Replace the `void loadSubagentMessages(projectId, sessionId,
subagentId)` call with explicit promise handling: call
`loadSubagentMessages(projectId, sessionId, subagentId)` and append a
`.catch(...)` handler to acknowledge the returned promise from the async
function (the function already manages errors via store state so the catch can
be a no-op or a minimal logger). Update the call site in useSubagentMessages.ts
where `loadSubagentMessages` is invoked to remove the `void` operator and add
the `.catch()` to satisfy sonarjs/void-use.
---
Nitpick comments:
In `@src/renderer/store/index.ts`:
- Around line 88-93: Update getRendererHeapMB to either document the
Chromium-only assumption or add a graceful fallback: clarify in the function
comment that performance.memory is a Chromium/Renderer-only API (Electron) and
will return 0 in non-Chromium environments, or implement an alternative/path
(e.g., use process.getProcessMemoryInfo or IPC to main which can call
process.getProcessMemoryInfo) when performance.memory is unavailable; reference
the function name getRendererHeapMB to locate where to add the comment or
fallback logic.
In `@src/renderer/store/slices/subagentMessageCacheSlice.ts`:
- Around line 158-176: The invalidateSubagentMessagesForSession function
currently resets subagentMessageCache and subagentMessageErrors but does not
clear inflightPromises, so in-flight fetches can repopulate the cache with stale
data; update invalidateSubagentMessagesForSession to also call
inflightPromises.clear() (same as clearSubagentMessageCache) so any pending
promises are discarded when invalidating, keeping the cache and inflightPromises
consistent (refer to invalidateSubagentMessagesForSession,
clearSubagentMessageCache, inflightPromises, subagentMessageCache, and
subagentMessageErrors).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e32f83b3-ee0a-4c8e-ad83-e55a2380f296
📒 Files selected for processing (19)
src/main/types/chunks.tssrc/preload/constants/ipcChannels.tssrc/preload/index.tssrc/renderer/api/httpClient.tssrc/renderer/components/chat/AIChatGroup.tsxsrc/renderer/components/chat/items/SubagentItem.tsxsrc/renderer/hooks/useSubagentMessages.tssrc/renderer/store/index.tssrc/renderer/store/slices/sessionDetailSlice.tssrc/renderer/store/slices/sessionSlice.tssrc/renderer/store/slices/subagentMessageCacheSlice.tssrc/renderer/store/slices/tabSlice.tssrc/renderer/store/types.tssrc/renderer/store/utils/stateResetHelpers.tssrc/renderer/utils/modelExtractor.tssrc/shared/types/api.tstest/mocks/electronAPI.tstest/renderer/store/storeTestUtils.tstest/renderer/store/subagentMessageCacheSlice.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/store/utils/stateResetHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/renderer/store/slices/sessionSlice.ts
- src/preload/constants/ipcChannels.ts
- src/renderer/store/slices/tabSlice.ts
- src/renderer/store/slices/sessionDetailSlice.ts
- src/shared/types/api.ts
- src/preload/index.ts
| // extractPrecedingSlashInfo moved to ChatHistory — pre-computed as a map to | ||
| // avoid O(n) scan per visible group per refresh cycle. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of AIChatGroup to verify precedingSlash is passed
rg -n '<AIChatGroup' --type=tsx --type=ts -A 5Repository: matt1398/claude-devtools
Length of output: 95
🏁 Script executed:
rg '<AIChatGroup' -t typescript -A 5Repository: matt1398/claude-devtools
Length of output: 102
🏁 Script executed:
rg -g '*.tsx' '<AIChatGroup' -A 5Repository: matt1398/claude-devtools
Length of output: 1062
🏁 Script executed:
cat -n src/renderer/components/chat/AIChatGroup.tsx | head -200Repository: matt1398/claude-devtools
Length of output: 9275
🏁 Script executed:
cat -n src/renderer/components/chat/ChatHistoryItem.tsx | head -150Repository: matt1398/claude-devtools
Length of output: 5401
🏁 Script executed:
rg -g '*.tsx' 'ChatHistory' --type-add 'tsx:*.tsx' -l | head -20Repository: matt1398/claude-devtools
Length of output: 365
🏁 Script executed:
cat -n src/renderer/components/chat/ChatHistory.tsx | head -300Repository: matt1398/claude-devtools
Length of output: 13387
🏁 Script executed:
cat -n src/renderer/components/chat/ChatHistory.tsx | tail -n +300 | head -200Repository: matt1398/claude-devtools
Length of output: 8757
🏁 Script executed:
cat -n src/renderer/components/chat/ChatHistory.tsx | tail -n +500 | head -200Repository: matt1398/claude-devtools
Length of output: 8979
🏁 Script executed:
cat -n src/renderer/components/chat/ChatHistory.tsx | tail -n +700 | head -300Repository: matt1398/claude-devtools
Length of output: 8814
🏁 Script executed:
rg -g '*.tsx' 'precedingSlash' -B 3 -A 3Repository: matt1398/claude-devtools
Length of output: 3371
🏁 Script executed:
rg -g '*.tsx' 'extractPrecedingSlashInfo\|precedingSlash\s*=' -B 2 -A 2Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
rg -g '*.ts' -g '*.tsx' 'extractPrecedingSlashInfo' -B 2 -A 2Repository: matt1398/claude-devtools
Length of output: 496
🏁 Script executed:
rg -g '*.ts' -g '*.tsx' 'precedingSlash.*=' -A 2Repository: matt1398/claude-devtools
Length of output: 300
🏁 Script executed:
rg -g '*.ts' -g '*.tsx' 'enhanceAIGroup' -B 5 -A 5Repository: matt1398/claude-devtools
Length of output: 4031
🏁 Script executed:
cat -n src/renderer/utils/aiGroupEnhancer.ts | head -100Repository: matt1398/claude-devtools
Length of output: 3866
🏁 Script executed:
cat -n src/renderer/utils/slashCommandExtractor.ts | head -150Repository: matt1398/claude-devtools
Length of output: 6160
Pass precedingSlash prop from ChatHistory to AIChatGroup instantiation.
The precedingSlash prop is optional but expected to be pre-computed and passed from ChatHistory. However, ChatHistoryItem instantiates AIChatGroup without the precedingSlash prop (lines 116-121):
<AIChatGroup
aiGroup={item.group}
highlightToolUseId={toolUseIdForGroup}
highlightColor={highlightColor}
registerToolRef={registerToolRef}
/>This means precedingSlash is always undefined, preventing proper slash command linking. The comments in AIChatGroup.tsx (lines 27-28, 175-177) indicate this was intended to be an optimization—moving the O(n) precedingSlash extraction from component render to ChatHistory—but the implementation is incomplete. Compute precedingSlash for each AI group in ChatHistory and pass it down through ChatHistoryItem to AIChatGroup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/chat/AIChatGroup.tsx` around lines 27 - 28,
ChatHistory now pre-computes preceding-slash info but ChatHistoryItem fails to
pass it down to AIChatGroup; update ChatHistory to compute the precedingSlash
map (using the existing extractPrecedingSlashInfo or the pre-computed map
already added), thread that value through ChatHistoryItem (add a precedingSlash
prop to its props) and then pass precedingSlash into the AIChatGroup
instantiation (the component expecting the precedingSlash prop), ensuring
AIChatGroup receives the pre-computed value for each aiGroup so slash-command
linking works.
| if (!projectId || !sessionId) return; | ||
| if (messages !== null) return; // already cached | ||
| if (isLoading) return; // already in flight | ||
| void loadSubagentMessages(projectId, sessionId, subagentId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current violation and whether the Sonar rule is configured in this repo.
rg -n "void\\s+loadSubagentMessages\\(" src/renderer/hooks/useSubagentMessages.ts
rg -n "sonarjs/void-use" --iglob "*eslint*" .Repository: matt1398/claude-devtools
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Find eslint configuration files
fd -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \)
# Also check package.json for eslintConfig
grep -l "eslintConfig" package.json 2>/dev/null
# Check the context around line 50 in the file
echo "=== Context around line 50 in useSubagentMessages.ts ==="
sed -n '45,55p' src/renderer/hooks/useSubagentMessages.ts
# Find the loadSubagentMessages function definition
echo "=== Searching for loadSubagentMessages definition ==="
rg -n "loadSubagentMessages\s*[=:]|function loadSubagentMessages" src/Repository: matt1398/claude-devtools
Length of output: 1087
🏁 Script executed:
#!/bin/bash
# Find and display eslint configuration
echo "=== ESLint Config Files ==="
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \)
# Check package.json
echo "=== ESLint in package.json ==="
grep -A 30 '"eslint' package.json | head -40
# Verify loadSubagentMessages is async and returns Promise
echo "=== Full loadSubagentMessages definition ==="
sed -n '86,100p' src/renderer/store/slices/subagentMessageCacheSlice.ts
# Check if sonarjs package is installed
echo "=== ESLint plugins in package ==="
rg "sonarjs" package.json package-lock.json 2>/dev/null | head -10Repository: matt1398/claude-devtools
Length of output: 2493
🏁 Script executed:
#!/bin/bash
# Check the eslint.config.js file for sonarjs configuration
echo "=== ESLint Config Content ==="
cat eslint.config.js | head -150Repository: matt1398/claude-devtools
Length of output: 4809
🏁 Script executed:
#!/bin/bash
# Continue reading eslint.config.js to see sonarjs rule configuration
echo "=== ESLint Config (continued) ==="
cat eslint.config.js | tail -n +150 | head -100
# Also search for any mention of "void-use" in the config
echo "=== Searching for void-use in config ==="
rg "void-use" eslint.config.jsRepository: matt1398/claude-devtools
Length of output: 2769
🏁 Script executed:
#!/bin/bash
# Get the rest of eslint.config.js
wc -l eslint.config.js
cat eslint.config.js | tail -100
# Also check what sonarjs.configs.recommended includes
# by checking if sonarjs has any void-use rule at all
npm ls eslint-plugin-sonarjs 2>/dev/null | head -5Repository: matt1398/claude-devtools
Length of output: 3770
🏁 Script executed:
#!/bin/bash
# Check sonarjs documentation and what rules are in the recommended config
echo "=== Checking sonarjs rule info ==="
# Search in node_modules for sonarjs rules
find node_modules/eslint-plugin-sonarjs -name "*.js" -o -name "*.json" | head -20
# Look for void-use rule definition
rg "void-use" node_modules/eslint-plugin-sonarjs/ 2>/dev/null | head -10
# Also check what the sonarjs recommended config includes
rg "void-use|configs\.recommended" node_modules/eslint-plugin-sonarjs/ 2>/dev/null | head -20Repository: matt1398/claude-devtools
Length of output: 1719
🏁 Script executed:
#!/bin/bash
# Check the sonarjs void-use documentation
cat node_modules/eslint-plugin-sonarjs/docs/void-use.md
# Also verify loadSubagentMessages returns a Promise
echo "=== Full context of loadSubagentMessages return type ==="
sed -n '86,120p' src/renderer/store/slices/subagentMessageCacheSlice.tsRepository: matt1398/claude-devtools
Length of output: 1798
🏁 Script executed:
#!/bin/bash
# Verify the full implementation to see if error state is actually handled
echo "=== Full loadSubagentMessages implementation ==="
sed -n '86,150p' src/renderer/store/slices/subagentMessageCacheSlice.tsRepository: matt1398/claude-devtools
Length of output: 2759
Replace void loadSubagentMessages(...) with explicit promise handling to satisfy sonarjs/void-use.
The void operator is flagged by the enabled sonarjs/void-use rule in ESLint config. Since loadSubagentMessages is async and already handles errors internally via store state, use .catch() to explicitly acknowledge the promise:
Proposed fix
- void loadSubagentMessages(projectId, sessionId, subagentId);
+ loadSubagentMessages(projectId, sessionId, subagentId).catch(() => {
+ // Error state is already captured in `subagentMessageErrors` by the slice.
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void loadSubagentMessages(projectId, sessionId, subagentId); | |
| loadSubagentMessages(projectId, sessionId, subagentId).catch(() => { | |
| // Error state is already captured in `subagentMessageErrors` by the slice. | |
| }); |
🧰 Tools
🪛 ESLint
[error] 50-50: Remove this use of the "void" operator.
(sonarjs/void-use)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/useSubagentMessages.ts` at line 50, Replace the `void
loadSubagentMessages(projectId, sessionId, subagentId)` call with explicit
promise handling: call `loadSubagentMessages(projectId, sessionId, subagentId)`
and append a `.catch(...)` handler to acknowledge the returned promise from the
async function (the function already manages errors via store state so the catch
can be a no-op or a minimal logger). Update the call site in
useSubagentMessages.ts where `loadSubagentMessages` is invoked to remove the
`void` operator and add the `.catch()` to satisfy sonarjs/void-use.
1 participant
Summary
aiGroupExpansionLevels,expandedStepIds,expandedDisplayItemIds,expandedAIGroupIds) when switching sessions to prevent unbounded growth over long uptimesessionRefreshGenerationandprojectRefreshGenerationMapsdisplayMetafor collapsed headers (toolCount, modelName, turnCount, isShutdownOnly) instead of iterating full message arrayscontainsToolUseId/findHighlightedItemIdto preferdisplayMeta.toolUseIdsover message scandisplayMeta.modelNameover message scanMemory savings
~50MB per session (50 subagents × 1MB each) → ~50KB in cached SessionDetails. Renderer holds full bodies only for actively-expanded subagents (LRU 10). Across DataCache's 50-entry LRU: ~2.5GB → ~2.5MB.
Test plan
pnpm typecheck && pnpm lint:fix— cleanpnpm test— all tests pass (including new subagentMessageCacheSlice tests)🤖 Generated with Claude Code