fix: prevent renderer memory leak from unbounded expansion state#168
Open
psypeal wants to merge 9 commits intomatt1398:mainfrom
Open
fix: prevent renderer memory leak from unbounded expansion state#168psypeal wants to merge 9 commits intomatt1398:mainfrom
psypeal wants to merge 9 commits intomatt1398:mainfrom
Conversation
# Conflicts: # src/renderer/components/chat/ContextBadge.tsx # src/renderer/utils/contextTracker.ts
# Conflicts: # src/renderer/components/chat/ContextBadge.tsx
sessionAnalyzer now reads pre-computed costUsd from calculateMetrics() instead of re-computing costs independently. Parent cost uses detail.metrics.costUsd, subagent cost uses proc.metrics.costUsd. This ensures the cost analysis panel and chat header always agree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Electron's before-input-event preventDefault blocks both Chromium's built-in reload AND keydown propagation to the renderer. Route Ctrl+R through IPC instead: main process intercepts and sends session:refresh, preload bridges to renderer, store listener refreshes in place. Switch all refresh paths (IPC, refresh button, keyboard shortcut) to refreshSessionInPlace to avoid unmounting the scroll container, then dispatch a custom event to smoothly scroll to the bottom. Fixes matt1398#85
# Conflicts: # src/renderer/utils/sessionAnalyzer.ts # test/renderer/utils/sessionAnalyzer.test.ts
…hlighting - Add BashToolViewer with syntax-highlighted command input via CodeBlockViewer - Add CollapsibleOutputSection (collapsed by default) for tool output - Apply collapsible output to DefaultToolViewer and BashToolViewer - Add bash/zsh/fish keyword sets for syntax highlighting - Add keyword sets for c, cpp, java, kotlin, swift, lua, html, yaml - Fix markdown/html false type-coloring on capitalized words - Add markdown preview toggle to ReadToolViewer (defaults to preview) - Default WriteToolViewer to preview mode for markdown files - Extend comment detection for zsh, fish, yaml (#) and lua (--)
Clear aiGroupExpansionLevels, expandedStepIds, expandedDisplayItemIds, and expandedAIGroupIds Maps/Sets when switching sessions. These were accumulating entries from every session viewed without cleanup, causing the renderer to grow to 3GB+ after 47 hours of use. Also add periodic pruning for sessionRefreshGeneration and projectRefreshGeneration Maps to prevent unbounded growth in the store. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
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.
Comment on lines
+513
to
+521
| 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); | ||
| } | ||
| } |
Contributor
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).
Comment on lines
+261
to
+269
| 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); | ||
| } | ||
| } |
Contributor
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.
Comment on lines
925
to
928
| 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('#') |
Contributor
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)
📝 WalkthroughWalkthroughImplements a Ctrl+R / Cmd+R session refresh flow via IPC that refreshes the active session in-place, fetches updated sessions, and triggers chat history scroll-to-bottom. Adds Bash tool rendering, expands syntax highlighting for multiple languages, introduces markdown preview toggling in file viewers, resets conversation expansion state on session changes, and prunes generation maps to prevent unbounded growth. Changes
Possibly related PRs
Suggested labels
✨ Finishing Touches⚔️ Resolve merge conflicts
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
Comment on lines
+380
to
+387
| // 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.
Comment on lines
+149
to
+150
| // 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.
Comment on lines
+924
to
929
| // 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.
Comment on lines
+260
to
+269
| // 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
aiGroupExpansionLevels,expandedStepIds,expandedDisplayItemIds,expandedAIGroupIds) on session switch infetchSessionDetailandsetActiveTabgetConversationExpansionResetState()helper to centralize reset logicsessionRefreshGenerationandprojectRefreshGenerationMaps (100 cap, prune to 50)Test plan
pnpm typecheckpasses (pre-existing errors inLinkedToolItem.tsxare unrelated)pnpm test-- all 652 tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements