fix: crash prevention with bounded caches and lazy subagent message loading#170
fix: crash prevention with bounded caches and lazy subagent message loading#170psypeal 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 introduces robust crash handling and memory monitoring in the main process, including a new crash logging utility and automated renderer process recovery. The chat interface is enhanced with a specialized Bash tool viewer, collapsible output sections for tool results, and Markdown preview toggles for file-related tools. Additionally, syntax highlighting is expanded to support several new languages, and a session refresh mechanism is implemented via IPC. Feedback addresses a regression in Markdown syntax highlighting, improvements to error logging for crash reports, the use of constants for IPC channels, and fixes for cache eviction logic and UI state persistence.
|
|
||
| // If no highlighting support, return plain text as single-element array | ||
| if (keywords.size === 0 && !['json', 'css', 'html', 'bash', 'markdown'].includes(language)) { | ||
| if (keywords.size === 0 && !['json', 'css', 'bash'].includes(language)) { |
There was a problem hiding this comment.
Removing markdown from the exclusion list causes a regression in syntax highlighting for Markdown files. Since markdown is not present in the KEYWORDS map, keywords.size will be 0, and the function will now return the plain text line immediately, bypassing the basic tokenization (strings, numbers, comments) that was previously applied.
| if (keywords.size === 0 && !['json', 'css', 'bash'].includes(language)) { | |
| if (keywords.size === 0 && !['json', 'css', 'bash', 'markdown'].includes(language)) { |
| const entry = | ||
| `[${new Date().toISOString()}] ${label}\n` + | ||
| Object.entries(details) | ||
| .map(([k, v]) => ` ${k}: ${typeof v === 'string' ? v : JSON.stringify(v)}`) |
There was a problem hiding this comment.
Using JSON.stringify(v) for error details will result in an empty object {} if v is an Error instance, as error properties like message and stack are non-enumerable. It's better to explicitly handle Error objects to ensure useful information is logged.
| .map(([k, v]) => ` ${k}: ${typeof v === 'string' ? v : JSON.stringify(v)}`) | |
| .map(([k, v]) => ` ${k}: ${v instanceof Error ? v.stack ?? v.message : (typeof v === 'string' ? v : JSON.stringify(v))}`) |
| // hence the IPC bridge. | ||
| if ((input.control || input.meta) && !input.shift && input.key.toLowerCase() === 'r') { | ||
| event.preventDefault(); | ||
| mainWindow.webContents.send('session:refresh'); |
There was a problem hiding this comment.
The IPC channel name 'session:refresh' is hardcoded here as a string literal. It should use the SESSION_REFRESH constant defined in src/preload/constants/ipcChannels.ts to maintain consistency and avoid potential typos. You should add it to the duplicated constants list at the top of the file to follow the existing pattern.
| mainWindow.webContents.send('session:refresh'); | |
| mainWindow.webContents.send(SESSION_REFRESH); |
| private pruneCache<V>(cache: Map<string, V>): void { | ||
| if (cache.size <= ProjectScanner.MAX_CACHE_ENTRIES) return; | ||
| const excess = cache.size - ProjectScanner.MAX_CACHE_ENTRIES; | ||
| let removed = 0; | ||
| for (const key of cache.keys()) { | ||
| if (removed >= excess) break; | ||
| cache.delete(key); | ||
| removed++; | ||
| } | ||
| } |
There was a problem hiding this comment.
The pruneCache implementation follows a FIFO (First-In-First-Out) eviction strategy rather than LRU (Least Recently Used), as Map.keys() iterates in insertion order and the order is never updated when an entry is accessed via get(). If LRU behavior is intended, you should move the accessed key to the end of the map on every get operation. Otherwise, the documentation should be updated to reflect FIFO behavior.
| : undefined; | ||
|
|
||
| const isMarkdownFile = /\.mdx?$/i.test(filePath); | ||
| const [viewMode, setViewMode] = React.useState<'code' | 'preview'>(isMarkdownFile ? 'preview' : 'code'); |
There was a problem hiding this comment.
The viewMode state is initialized only when the component mounts. If the component is reused for a different linkedTool (e.g., when switching between files in the same session), the viewMode will persist from the previous file instead of resetting to the appropriate default for the new file type. This can lead to a confusing UX where a code file opens in 'preview' mode if the previous file was Markdown.
const isMarkdownFile = /\.mdx?$/i.test(filePath);
const [viewMode, setViewMode] = React.useState<'code' | 'preview'>(isMarkdownFile ? 'preview' : 'code');
// Reset view mode when file path changes
React.useEffect(() => {
setViewMode(isMarkdownFile ? 'preview' : 'code');
}, [filePath, isMarkdownFile]);
📝 WalkthroughWalkthroughThis PR introduces crash logging and recovery features in the main process, implements cache eviction and session-scoped invalidation in ProjectScanner, and adds a SubagentMessageCache service along with corresponding IPC and HTTP endpoints to lazy-load and serve subagent parsed messages. 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: 6
🧹 Nitpick comments (6)
src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx (1)
61-100: Consider extracting the markdown mode toggle into a shared component.This block is now very similar to
WriteToolViewer, so a sharedMarkdownModeToggle(or hook + tiny component) would reduce duplication and prevent drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx` around lines 61 - 100, Extract the repeated markdown view toggle UI from ReadToolViewer into a shared component (e.g., MarkdownModeToggle) and use it in both ReadToolViewer and WriteToolViewer: create a new component that accepts props {isMarkdownFile, viewMode, setViewMode} (or a hook that returns [viewMode, setViewMode]) and moves the two buttons and styling there, then replace the inline toggle block in ReadToolViewer (the fragment using isMarkdownFile, viewMode, setViewMode) with <MarkdownModeToggle .../> and do the same in WriteToolViewer so both reuse the same logic and visuals.src/main/services/discovery/ProjectScanner.ts (1)
109-122: FIFO eviction, not true LRU.The
pruneCache()method evicts entries based on Map insertion order, which is FIFO (first-in, first-out). True LRU eviction requires moving entries to the end of the map on cache hits (reads), so that frequently-accessed entries remain protected from eviction.For the use case here (session metadata caching), FIFO is likely acceptable since cache hits rarely repeat immediately—sessions are scanned, then users navigate away. However, if this becomes a concern, consider using a proper LRU cache like
lru-cacheor re-inserting on hits.Based on learnings: "Use LRU Cache to avoid re-parsing large JSONL files"—the current implementation is functionally a bounded FIFO cache rather than LRU.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/discovery/ProjectScanner.ts` around lines 109 - 122, pruneCache currently evicts by Map insertion order (FIFO) which is not true LRU; update the cache strategy used by ProjectScanner so reads refresh entry recency or replace the Map with an LRU implementation: either (1) on every read/update of the cache entry move it to the end of the Map (re-insert the key/value) so pruneCache continues to work as LRU, or (2) swap the Map for a standard LRU utility (e.g., the lru-cache package) and enforce MAX_CACHE_ENTRIES there; modify places that access the cache to use the chosen approach and keep pruneCache/ProjectScanner.MAX_CACHE_ENTRIES semantics consistent.src/main/index.ts (1)
715-745: Memory monitor interval is not cleared on window close.The
memoryMonitorIntervalis created but never cleared when the window is closed or destroyed. Whileunref()prevents it from blocking app exit, the interval will continue running and potentially throw errors when accessingmainWindow.webContentsafter the window is destroyed (though theif (!mainWindow || mainWindow.isDestroyed())guard catches this).For cleaner resource management, consider clearing the interval in the
mainWindow.on('closed')handler.🧹 Proposed fix to clear the interval
Add interval tracking and cleanup:
+ // Track interval for cleanup + let memoryMonitorInterval: ReturnType<typeof setInterval> | null = null; + // ... existing code ... - const memoryMonitorInterval = setInterval(() => { + memoryMonitorInterval = setInterval(() => { // ... existing code ... }, MEMORY_CHECK_INTERVAL_MS); memoryMonitorInterval.unref(); // In the 'closed' handler: mainWindow.on('closed', () => { mainWindow = null; + if (memoryMonitorInterval) { + clearInterval(memoryMonitorInterval); + memoryMonitorInterval = null; + } // ... existing cleanup ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 715 - 745, The memoryMonitorInterval is never cleared when the window is closed; add cleanup to stop the interval to avoid background checks against a destroyed window. In the component that creates memoryMonitorInterval (the block using MEMORY_CHECK_INTERVAL_MS and calling mainWindow.webContents.getOSProcessId()), ensure you store the interval id in memoryMonitorInterval and call clearInterval(memoryMonitorInterval) from the mainWindow 'closed' (or 'close') event handler (e.g., inside mainWindow.on('closed', ...) or similar teardown logic) and remove/unregister any references so the interval is not left running after the window is destroyed.src/renderer/components/chat/items/LinkedToolItem.tsx (1)
44-45: Keep imports in guideline order.
Line 44adds a path-alias type import after relative imports. Group alias imports before relative imports to stay consistent.As per coding guidelines: organize imports as external packages, then path aliases, 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/LinkedToolItem.tsx` around lines 44 - 45, Reorder the imports in LinkedToolItem.tsx to follow the project guideline: place path-alias imports (e.g., "import type { StepVariant } from '@renderer/constants/stepVariants';" and "import type { LinkedToolItem as LinkedToolItemType } from '@renderer/types/groups';") before any relative imports; update the import block so external packages remain first, then the path-alias imports (StepVariant, LinkedToolItemType), and finally relative imports to restore the prescribed grouping order.src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx (1)
30-40: Add explicit ARIA state to the toggle button.This is already keyboard-friendly; adding
aria-expandedandaria-controlswould improve screen-reader clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx` around lines 30 - 40, The toggle button in CollapsibleOutputSection lacks explicit ARIA state; update the button rendered in the CollapsibleOutputSection component to include aria-expanded tied to isExpanded and aria-controls pointing to the id of the collapsible panel (the element rendered when isExpanded is true). Ensure the panel element has a stable id (generate one using React's useId or a prop fallback) so the button's aria-controls references that id. Keep the existing onClick (setIsExpanded) behavior and only add these attributes to the <button> and the corresponding panel element to improve screen-reader clarity.src/renderer/utils/toolRendering/index.ts (1)
7-13: Avoid expanding renderer utility barrel exports.Adding
hasBashContenthere increases reliance on a renderer utils barrel; guideline direction is to import directly from specific files in renderer utilities.As per coding guidelines:
src/renderer/**/*.{ts,tsx}renderer utils/hooks/types should NOT use barrel exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/toolRendering/index.ts` around lines 7 - 13, The barrel export index.ts is exposing hasBashContent which violates the renderer utils rule against using barrel exports; remove hasBashContent from the export list in src/renderer/utils/toolRendering/index.ts and update any callers to import hasBashContent directly from './toolContentChecks' (leave the other exports—hasEditContent, hasReadContent, hasSkillInstructions, hasWriteContent—if they remain allowed); ensure all references that previously imported hasBashContent from the barrel now import it from the specific module to comply with the renderer utils guideline.
🤖 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 session-refresh-scroll-bottom listener is global and
causes every mounted ChatHistory to scroll; update the handler in the useEffect
so it only acts for the intended tab/pane by checking an identifier on the
incoming event (e.g., event.detail?.chatId or event.detail?.paneId) against this
component's id/active prop (e.g., props.chatId, props.sessionId or isActive
flag) before calling scrollToBottom; if the app dispatching the event doesn't
include an identifier, change the emitter to include one and then compare in the
handler (identify the handler, useEffect, scrollToBottom, and the
'session-refresh-scroll-bottom' event).
In `@src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx`:
- Around line 25-27: The code in BashToolViewer currently unsafely casts
linkedTool.input with "as string"; replace this with a runtime type guard to
validate the shape before using fields. Add a reusable guard function (e.g.,
isBashToolInput) that checks input is a non-null object and that input.command
is a string (and optionally input.description is string), import it into
BashToolViewer, call it to narrow linkedTool.input, and only then read command
and description; update other viewers (EditToolViewer, WriteToolViewer) to use
corresponding guards for consistent validation.
In `@src/renderer/components/chat/items/LinkedToolItem.tsx`:
- Line 175: The LinkedToolItem component is passing an undeclared prop
variant={toolVariant} to BaseItem even though BaseItemProps does not define
variant; remove the prop from the JSX in LinkedToolItem (search for
LinkedToolItem and the BaseItem usage) so BaseItem is only given supported
props, or alternatively add variant to BaseItemProps and implement handling
inside BaseItem (update BaseItemProps and BaseItem component) if variant
behavior is required; prefer removing the prop here to resolve the TypeScript
error.
In `@src/renderer/hooks/useKeyboardShortcuts.ts`:
- Around line 263-268: The Promise.all call that runs
refreshSessionInPlace(selectedProjectId, selectedSessionId) and
fetchSessions(selectedProjectId) lacks error handling and can produce unhandled
promise rejections; wrap the Promise.all(...) chain with a .catch(...) to handle
errors (e.g., log with console.error or the app logger) and, if appropriate,
surface user feedback, while preserving the existing .then(...) that dispatches
the 'session-refresh-scroll-bottom' CustomEvent; reference the Promise.all call
and the functions refreshSessionInPlace, fetchSessions, and the dispatch of
'session-refresh-scroll-bottom' when making the change.
In `@src/renderer/store/index.ts`:
- Around line 278-283: The Promise.all call that invokes
state.refreshSessionInPlace and state.fetchSessions can reject and currently
leaves an unhandled rejection; update the code around the
Promise.all([...]).then(...) to append a .catch handler that explicitly handles
errors from refreshSessionInPlace and fetchSessions (e.g., log the error via
console.error or your app logger and/or surface a user-facing error) and ensure
post-refresh behavior is only triggered on success; reference the
refreshSessionInPlace and fetchSessions calls and the
'session-refresh-scroll-bottom' CustomEvent so you add the .catch after the
.then to avoid silent failures and unhandled rejections.
In `@src/renderer/utils/toolRendering/toolContentChecks.ts`:
- Around line 63-65: hasBashContent currently returns true for any truthy
linkedTool.input?.command (numbers, objects, booleans); change it into a proper
type guard by updating the function signature to a type predicate (e.g.,
hasBashContent(linkedTool: LinkedToolItem): linkedTool is LinkedToolItem & {
input: { command: string } }) and return only when typeof
linkedTool.input?.command === 'string' && linkedTool.input.command.trim().length
> 0 so callers can safely assume command is a non-empty string.
---
Nitpick comments:
In `@src/main/index.ts`:
- Around line 715-745: The memoryMonitorInterval is never cleared when the
window is closed; add cleanup to stop the interval to avoid background checks
against a destroyed window. In the component that creates memoryMonitorInterval
(the block using MEMORY_CHECK_INTERVAL_MS and calling
mainWindow.webContents.getOSProcessId()), ensure you store the interval id in
memoryMonitorInterval and call clearInterval(memoryMonitorInterval) from the
mainWindow 'closed' (or 'close') event handler (e.g., inside
mainWindow.on('closed', ...) or similar teardown logic) and remove/unregister
any references so the interval is not left running after the window is
destroyed.
In `@src/main/services/discovery/ProjectScanner.ts`:
- Around line 109-122: pruneCache currently evicts by Map insertion order (FIFO)
which is not true LRU; update the cache strategy used by ProjectScanner so reads
refresh entry recency or replace the Map with an LRU implementation: either (1)
on every read/update of the cache entry move it to the end of the Map (re-insert
the key/value) so pruneCache continues to work as LRU, or (2) swap the Map for a
standard LRU utility (e.g., the lru-cache package) and enforce MAX_CACHE_ENTRIES
there; modify places that access the cache to use the chosen approach and keep
pruneCache/ProjectScanner.MAX_CACHE_ENTRIES semantics consistent.
In `@src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx`:
- Around line 30-40: The toggle button in CollapsibleOutputSection lacks
explicit ARIA state; update the button rendered in the CollapsibleOutputSection
component to include aria-expanded tied to isExpanded and aria-controls pointing
to the id of the collapsible panel (the element rendered when isExpanded is
true). Ensure the panel element has a stable id (generate one using React's
useId or a prop fallback) so the button's aria-controls references that id. Keep
the existing onClick (setIsExpanded) behavior and only add these attributes to
the <button> and the corresponding panel element to improve screen-reader
clarity.
In `@src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx`:
- Around line 61-100: Extract the repeated markdown view toggle UI from
ReadToolViewer into a shared component (e.g., MarkdownModeToggle) and use it in
both ReadToolViewer and WriteToolViewer: create a new component that accepts
props {isMarkdownFile, viewMode, setViewMode} (or a hook that returns [viewMode,
setViewMode]) and moves the two buttons and styling there, then replace the
inline toggle block in ReadToolViewer (the fragment using isMarkdownFile,
viewMode, setViewMode) with <MarkdownModeToggle .../> and do the same in
WriteToolViewer so both reuse the same logic and visuals.
In `@src/renderer/components/chat/items/LinkedToolItem.tsx`:
- Around line 44-45: Reorder the imports in LinkedToolItem.tsx to follow the
project guideline: place path-alias imports (e.g., "import type { StepVariant }
from '@renderer/constants/stepVariants';" and "import type { LinkedToolItem as
LinkedToolItemType } from '@renderer/types/groups';") before any relative
imports; update the import block so external packages remain first, then the
path-alias imports (StepVariant, LinkedToolItemType), and finally relative
imports to restore the prescribed grouping order.
In `@src/renderer/utils/toolRendering/index.ts`:
- Around line 7-13: The barrel export index.ts is exposing hasBashContent which
violates the renderer utils rule against using barrel exports; remove
hasBashContent from the export list in src/renderer/utils/toolRendering/index.ts
and update any callers to import hasBashContent directly from
'./toolContentChecks' (leave the other exports—hasEditContent, hasReadContent,
hasSkillInstructions, hasWriteContent—if they remain allowed); ensure all
references that previously imported hasBashContent from the barrel now import it
from the specific module to comply with the renderer utils guideline.
🪄 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: 0be83daf-cb95-431a-858e-1a8b9929ab66
📒 Files selected for processing (22)
src/main/index.tssrc/main/services/discovery/ProjectScanner.tssrc/main/services/infrastructure/FileWatcher.tssrc/main/services/infrastructure/ServiceContext.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/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 refresh-scroll handling to the intended tab/pane.
This handler runs for every mounted ChatHistory, so one refresh event can scroll unrelated views to bottom and disrupt reading position.
💡 Proposed fix
useEffect(() => {
const handler = (): void => {
- scrollToBottom('smooth');
+ if (!isThisTabActive) return;
+ scrollToBottom('smooth');
};
window.addEventListener('session-refresh-scroll-bottom', handler);
return () => window.removeEventListener('session-refresh-scroll-bottom', handler);
- }, [scrollToBottom]);
+ }, [isThisTabActive, scrollToBottom]);🤖 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
session-refresh-scroll-bottom listener is global and causes every mounted
ChatHistory to scroll; update the handler in the useEffect so it only acts for
the intended tab/pane by checking an identifier on the incoming event (e.g.,
event.detail?.chatId or event.detail?.paneId) against this component's id/active
prop (e.g., props.chatId, props.sessionId or isActive flag) before calling
scrollToBottom; if the app dispatching the event doesn't include an identifier,
change the emitter to include one and then compare in the handler (identify the
handler, useEffect, scrollToBottom, and the 'session-refresh-scroll-bottom'
event).
| const command = linkedTool.input.command as string; | ||
| const description = linkedTool.input.description as string | undefined; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx | head -35Repository: matt1398/claude-devtools
Length of output: 1329
🏁 Script executed:
# Search for type guard utilities in the codebase
fd -t f "isXxx" --iglob "*.ts" --iglob "*.tsx" src/Repository: matt1398/claude-devtools
Length of output: 270
🏁 Script executed:
# Look for existing type guard patterns in renderer utils
rg "^export const is[A-Z]" src/renderer/ -A 2 | head -30Repository: matt1398/claude-devtools
Length of output: 276
🏁 Script executed:
# Check if there are validation utilities for linked tools
rg "linkedTool" src/renderer/ -t ts -t tsx -l | head -10Repository: matt1398/claude-devtools
Length of output: 95
🏁 Script executed:
# Find LinkedToolItem type definition
rg "type LinkedToolItem" src/renderer/ -A 10Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Search for type guards with isXxx pattern
rg "export const is[A-Z]" src/ -t ts | head -20Repository: matt1398/claude-devtools
Length of output: 160
🏁 Script executed:
# Check for tool input validation patterns
rg "linkedTool\.input" src/renderer/ -B 2 -A 2 | head -40Repository: matt1398/claude-devtools
Length of output: 3361
🏁 Script executed:
# Check the types/groups file for LinkedToolItem
cat -n src/renderer/types/groups.ts | head -100Repository: matt1398/claude-devtools
Length of output: 3494
🏁 Script executed:
# Search for LinkedToolItem interface
rg "interface LinkedToolItem" src/renderer/types/Repository: matt1398/claude-devtools
Length of output: 130
🏁 Script executed:
# Get the full types/groups.ts file
wc -l src/renderer/types/groups.tsRepository: matt1398/claude-devtools
Length of output: 100
🏁 Script executed:
# Search for LinkedToolItem definition in all types files
rg "LinkedToolItem" src/renderer/types/ -A 15 | head -60Repository: matt1398/claude-devtools
Length of output: 3120
🏁 Script executed:
# Check other tool viewers for validation patterns
cat -n src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx | head -40Repository: matt1398/claude-devtools
Length of output: 1909
🏁 Script executed:
# Check EditToolViewer
cat -n src/renderer/components/chat/items/linkedTool/EditToolViewer.tsx | head -40Repository: matt1398/claude-devtools
Length of output: 1533
🏁 Script executed:
# Search for validation utilities or type guards related to tools
rg "function is" src/renderer/utils/ -A 3Repository: matt1398/claude-devtools
Length of output: 1736
Create type guards for tool input validation instead of unsafe casts.
Lines 25-26 use unsafe as string casts on untyped input fields. Per coding guidelines, runtime validation should use isXxx type guards. This pattern appears across multiple tool viewers (BashToolViewer, EditToolViewer, WriteToolViewer).
Create reusable type guards like isBashToolInput(input): input is { command: string; description?: string } and use them consistently:
Suggested approach
- const command = linkedTool.input.command as string;
- const description = linkedTool.input.description as string | undefined;
+ if (!isBashToolInput(linkedTool.input)) {
+ return null;
+ }
+ const { command, description } = linkedTool.input;
+ if (command.trim().length === 0) return null;Define in a shared validation module (e.g., utils/toolRendering/toolInputGuards.ts):
export function isBashToolInput(input: unknown): input is { command: string; description?: string } {
return typeof input === 'object' && input !== null &&
typeof (input as Record<string, unknown>).command === 'string';
}🤖 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 25 - 27, The code in BashToolViewer currently unsafely casts
linkedTool.input with "as string"; replace this with a runtime type guard to
validate the shape before using fields. Add a reusable guard function (e.g.,
isBashToolInput) that checks input is a non-null object and that input.command
is a string (and optionally input.description is string), import it into
BashToolViewer, call it to narrow linkedTool.input, and only then read command
and description; update other viewers (EditToolViewer, WriteToolViewer) to use
corresponding guards for consistent validation.
| highlightClasses={highlightClasses} | ||
| highlightStyle={highlightStyle} | ||
| notificationDotColor={notificationDotColor} | ||
| variant={toolVariant} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether BaseItemProps declares `variant` and where BaseItem is called with it.
rg -nP --type=tsx -C3 'interface\s+BaseItemProps|variant\??\s*:' src/renderer/components/chat/items/BaseItem.tsx
rg -nP --type=tsx -C2 '<BaseItem[^>]*\bvariant=' src/renderer/components/chat/items/LinkedToolItem.tsxRepository: matt1398/claude-devtools
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Search for BaseItemProps interface and variant usage without type restrictions
rg -nA5 'interface\s+BaseItemProps' src/renderer/components/chat/items/BaseItem.tsxRepository: matt1398/claude-devtools
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Read the LinkedToolItem.tsx file to see line 175 and surrounding context
wc -l src/renderer/components/chat/items/LinkedToolItem.tsxRepository: matt1398/claude-devtools
Length of output: 125
🏁 Script executed:
#!/bin/bash
# List files in the items directory to confirm they exist
ls -la src/renderer/components/chat/items/Repository: matt1398/claude-devtools
Length of output: 913
🏁 Script executed:
#!/bin/bash
# Read the full BaseItemProps interface definition
sed -n '15,40p' src/renderer/components/chat/items/BaseItem.tsxRepository: matt1398/claude-devtools
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Read line 175 and surrounding context from LinkedToolItem.tsx
sed -n '170,180p' src/renderer/components/chat/items/LinkedToolItem.tsxRepository: matt1398/claude-devtools
Length of output: 459
🏁 Script executed:
#!/bin/bash
# Continue reading BaseItemProps to see if variant is declared later
sed -n '40,60p' src/renderer/components/chat/items/BaseItem.tsxRepository: matt1398/claude-devtools
Length of output: 691
🏁 Script executed:
#!/bin/bash
# Read more context around line 175 to see which component the variant prop is on
sed -n '160,180p' src/renderer/components/chat/items/LinkedToolItem.tsxRepository: matt1398/claude-devtools
Length of output: 808
🏁 Script executed:
#!/bin/bash
# Check BaseItem component render/export signature
sed -n '1,100p' src/renderer/components/chat/items/BaseItem.tsx | grep -A 30 'export\|function\|const.*BaseItem'Repository: matt1398/claude-devtools
Length of output: 2455
🏁 Script executed:
#!/bin/bash
# Read exact lines around 175 to identify the opening tag
sed -n '165,178p' src/renderer/components/chat/items/LinkedToolItem.tsxRepository: matt1398/claude-devtools
Length of output: 595
🏁 Script executed:
#!/bin/bash
# Read earlier lines to see the opening tag of the component
sed -n '155,175p' src/renderer/components/chat/items/LinkedToolItem.tsxRepository: matt1398/claude-devtools
Length of output: 709
Remove undeclared variant prop from BaseItem component.
Line 175 passes variant={toolVariant} to <BaseItem>, but BaseItemProps does not include this prop. The BaseItem component will not accept or use this prop, causing a TypeScript error.
Fix
- variant={toolVariant}📝 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.
| variant={toolVariant} | |
| {/* variant prop removed - not supported by BaseItemProps */} |
🤖 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` at line 175, The
LinkedToolItem component is passing an undeclared prop variant={toolVariant} to
BaseItem even though BaseItemProps does not define variant; remove the prop from
the JSX in LinkedToolItem (search for LinkedToolItem and the BaseItem usage) so
BaseItem is only given supported props, or alternatively add variant to
BaseItemProps and implement handling inside BaseItem (update BaseItemProps and
BaseItem component) if variant behavior is required; prefer removing the prop
here to resolve the TypeScript error.
| void Promise.all([ | ||
| fetchSessionDetail(selectedProjectId, selectedSessionId), | ||
| refreshSessionInPlace(selectedProjectId, selectedSessionId), | ||
| fetchSessions(selectedProjectId), | ||
| ]); | ||
| ]).then(() => { | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| }); |
There was a problem hiding this comment.
Add error handling for the refresh shortcut async flow.
This Promise chain can reject without a handler, causing unhandled rejection noise and harder failure diagnosis.
💡 Proposed fix
void Promise.all([
refreshSessionInPlace(selectedProjectId, selectedSessionId),
fetchSessions(selectedProjectId),
- ]).then(() => {
- window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom'));
- });
+ ])
+ .then(() => {
+ window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom'));
+ })
+ .catch((error: unknown) => {
+ console.error('[KeyboardShortcuts] Session refresh failed:', error);
+ });📝 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 Promise.all([ | |
| fetchSessionDetail(selectedProjectId, selectedSessionId), | |
| refreshSessionInPlace(selectedProjectId, selectedSessionId), | |
| fetchSessions(selectedProjectId), | |
| ]); | |
| ]).then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }); | |
| void Promise.all([ | |
| refreshSessionInPlace(selectedProjectId, selectedSessionId), | |
| fetchSessions(selectedProjectId), | |
| ]) | |
| .then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }) | |
| .catch((error: unknown) => { | |
| console.error('[KeyboardShortcuts] Session refresh failed:', error); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/useKeyboardShortcuts.ts` around lines 263 - 268, The
Promise.all call that runs refreshSessionInPlace(selectedProjectId,
selectedSessionId) and fetchSessions(selectedProjectId) lacks error handling and
can produce unhandled promise rejections; wrap the Promise.all(...) chain with a
.catch(...) to handle errors (e.g., log with console.error or the app logger)
and, if appropriate, surface user feedback, while preserving the existing
.then(...) that dispatches the 'session-refresh-scroll-bottom' CustomEvent;
reference the Promise.all call and the functions refreshSessionInPlace,
fetchSessions, and the dispatch of 'session-refresh-scroll-bottom' when making
the change.
| void Promise.all([ | ||
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | ||
| state.fetchSessions(activeTab.projectId), | ||
| ]).then(() => { | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| }); |
There was a problem hiding this comment.
Handle refresh Promise rejections explicitly.
If either async call fails, this currently produces an unhandled rejection and silently skips post-refresh behavior.
💡 Proposed fix
- void Promise.all([
- state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId),
- state.fetchSessions(activeTab.projectId),
- ]).then(() => {
- window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom'));
- });
+ void Promise.all([
+ state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId),
+ state.fetchSessions(activeTab.projectId),
+ ])
+ .then(() => {
+ window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom'));
+ })
+ .catch((error: unknown) => {
+ console.error('[Store] Session refresh failed:', error);
+ });📝 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 Promise.all([ | |
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | |
| state.fetchSessions(activeTab.projectId), | |
| ]).then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }); | |
| void Promise.all([ | |
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | |
| state.fetchSessions(activeTab.projectId), | |
| ]) | |
| .then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }) | |
| .catch((error: unknown) => { | |
| console.error('[Store] Session refresh failed:', error); | |
| }); |
🤖 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 278 - 283, The Promise.all call
that invokes state.refreshSessionInPlace and state.fetchSessions can reject and
currently leaves an unhandled rejection; update the code around the
Promise.all([...]).then(...) to append a .catch handler that explicitly handles
errors from refreshSessionInPlace and fetchSessions (e.g., log the error via
console.error or your app logger and/or surface a user-facing error) and ensure
post-refresh behavior is only triggered on success; reference the
refreshSessionInPlace and fetchSessions calls and the
'session-refresh-scroll-bottom' CustomEvent so you add the .catch after the
.then to avoid silent failures and unhandled rejections.
| export function hasBashContent(linkedTool: LinkedToolItem): boolean { | ||
| return !!linkedTool.input?.command; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat src/renderer/utils/toolRendering/toolContentChecks.tsRepository: matt1398/claude-devtools
Length of output: 2075
🏁 Script executed:
cd src/renderer && find . -name 'groups.ts' -o -name 'groups.tsx' | head -5Repository: matt1398/claude-devtools
Length of output: 85
🏁 Script executed:
cat src/renderer/types/groups.ts | head -100Repository: matt1398/claude-devtools
Length of output: 2794
🏁 Script executed:
rg "export (interface|type) LinkedToolItem" src/renderer/types/groups.ts -A 20Repository: matt1398/claude-devtools
Length of output: 726
🏁 Script executed:
rg "!!.*\?" src/renderer/utils/toolRendering/toolContentChecks.tsRepository: matt1398/claude-devtools
Length of output: 105
🏁 Script executed:
rg "!!.*\?" src/renderer/utils/ -t ts -t tsx --max-count 10Repository: matt1398/claude-devtools
Length of output: 95
🏁 Script executed:
rg "!!.*\?" src/renderer/utils/ -t ts --max-count 10Repository: matt1398/claude-devtools
Length of output: 159
Use a proper type guard for bash command validation.
The current implementation accepts any truthy command value (objects, numbers, booleans), which could incorrectly route to the Bash viewer and cause unsafe type casts downstream. Implement a type guard with proper string validation.
Suggested fix
+function isBashInput(input: Record<string, unknown>): input is { command: string } {
+ return typeof input.command === 'string' && input.command.trim().length > 0;
+}
+
export function hasBashContent(linkedTool: LinkedToolItem): boolean {
- return !!linkedTool.input?.command;
+ return isBashInput(linkedTool.input);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/utils/toolRendering/toolContentChecks.ts` around lines 63 - 65,
hasBashContent currently returns true for any truthy linkedTool.input?.command
(numbers, objects, booleans); change it into a proper type guard by updating the
function signature to a type predicate (e.g., hasBashContent(linkedTool:
LinkedToolItem): linkedTool is LinkedToolItem & { input: { command: string } })
and return only when typeof linkedTool.input?.command === 'string' &&
linkedTool.input.command.trim().length > 0 so callers can safely assume command
is a non-empty string.
…tion - Add LRU eviction (500 entries) for ProjectScanner caches to prevent unbounded growth over long sessions - Add invalidateCachesForSession() for single-file cache invalidation instead of purging entire project on every file-change event - Add renderer memory monitoring (5-min interval, 2GB warning threshold) to detect leaks before they crash the renderer - Add crash recovery with retry caps, unresponsive watchdog, and crash logging to ~/.claude/claude-devtools-crash.log - Add SubagentMessageCache (LRU, 10 entries, 10min TTL) for lazy-loaded subagent message bodies, distinct from the main DataCache - Add IPC + HTTP handlers for on-demand subagent message loading so the renderer can fetch bodies only when a subagent is expanded - Fix SubagentDetailBuilder path construction to use buildSubagentsPath() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
psypeal
force-pushed
the
fix/crash-prevention-bounded-caches
branch
from
2f04d29 to
75e23ec
Compare
psypeal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/main/services/discovery/ProjectScanner.ts (1)
63-64: Move cache size to shared cache constants.
MAX_CACHE_ENTRIESis hardcoded inProjectScanner. Please centralize this insrc/shared/constants/cache.tsso cache sizing stays consistent and tunable across services.Based on learnings: Applies to src/shared/constants/cache.ts : Use
cache.tsin constants for cache configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/discovery/ProjectScanner.ts` around lines 63 - 64, Replace the hardcoded MAX_CACHE_ENTRIES in ProjectScanner with a shared constant: import the cache size constant exported from src/shared/constants/cache.ts and use it in place of ProjectScanner.MAX_CACHE_ENTRIES; remove the private static readonly MAX_CACHE_ENTRIES declaration from the ProjectScanner class and update any references (e.g., cache construction or LRU init inside ProjectScanner) to reference the shared symbol (e.g., DEFAULT_CACHE_MAX_ENTRIES or CACHE_MAX_ENTRIES) exported by cache.ts so cache sizing is centralized and configurable across services.src/main/http/index.ts (1)
31-31: Use barrel export forSubagentMessageCachetype import.Please import this type from
../services(and export it there if needed) instead of importing from../services/infrastructure/...directly.As per coding guidelines:
src/main/**/*.{ts,tsx}: Services and domain subdirectories in src/main/services/ must export via barrel exports (index.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/http/index.ts` at line 31, The import of the SubagentMessageCache type in index.ts should use the services barrel export: update the import to pull SubagentMessageCache from '../services' (i.e., import type { SubagentMessageCache } from '../services') and, if the type is not already exported, add an export of SubagentMessageCache in the services barrel (src/main/services/index.ts) re-exporting it from './infrastructure/SubagentMessageCache' so callers can rely on the top-level services barrel.src/main/services/infrastructure/ServiceContext.ts (1)
115-115: Extract subagent cache size to shared cache constants.
10is hardcoded here. Please define and consume a named constant fromsrc/shared/constants/cache.tsto keep cache knobs centralized.Based on learnings: Applies to src/shared/constants/cache.ts : Use
cache.tsin constants for cache configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/infrastructure/ServiceContext.ts` at line 115, Replace the hardcoded 10 used when constructing SubagentMessageCache in ServiceContext with a named constant: add a new exported constant (e.g. SUBAGENT_MESSAGE_CACHE_SIZE) to your shared cache constants module and then change the constructor call in ServiceContext (the line creating new SubagentMessageCache(...)) to use that constant instead of 10; ensure the constant is exported from the constants file and imported into ServiceContext so cache knobs remain centralized.src/main/index.ts (1)
720-756: Consider clearing the memory monitor interval on window close.The
memoryMonitorIntervalis created insidecreateWindowbut not cleared when the window is closed or destroyed. Whileunref()prevents it from blocking app exit, the interval continues running and may log errors whenmainWindowis null after the window closes but before the app quits (e.g., on macOS where closing the window doesn't quit the app).💡 Suggested fix: Clear interval in 'closed' handler
+ let memoryMonitorInterval: ReturnType<typeof setInterval> | null = null; + // Periodic memory monitoring via app.getAppMetrics(). // Logs all-process memory every 5 minutes so we have data leading up to crashes. // Warns when the renderer exceeds 2 GB. const MEMORY_CHECK_INTERVAL_MS = 5 * 60_000; const RENDERER_MEMORY_WARNING_KB = 2048 * 1024; // 2 GB in KB - const memoryMonitorInterval = setInterval(() => { + memoryMonitorInterval = setInterval(() => { if (!mainWindow || mainWindow.isDestroyed()) return; // ... rest of implementation }, MEMORY_CHECK_INTERVAL_MS); memoryMonitorInterval.unref(); // Don't prevent app exitThen in the
mainWindow.on('closed')handler around line 568:mainWindow.on('closed', () => { mainWindow = null; + if (memoryMonitorInterval) { + clearInterval(memoryMonitorInterval); + memoryMonitorInterval = null; + } // Clear main window references if (notificationManager) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 720 - 756, The memory monitor interval created as memoryMonitorInterval inside createWindow is never cleared when the window closes, so it keeps running and may log errors after mainWindow becomes null; update the mainWindow.on('closed') / 'closed' handler to call clearInterval(memoryMonitorInterval) (and optionally set memoryMonitorInterval to undefined) so the interval stops when the window is closed, ensuring the periodic check and writeCrashLog calls cease once the window is destroyed.
🤖 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/main/services/analysis/SubagentDetailBuilder.ts`:
- Line 46: The JSDoc for SubagentDetailBuilder still references an unused
`_sessionId` but the code now accepts and uses `sessionId`; update the docblock
for the constructor (or the function that declares `sessionId`) to describe the
`sessionId: string` parameter correctly, remove the outdated `_sessionId` note,
and ensure the param tag matches the exact symbol name `sessionId` used in the
method signature so documentation and code are in sync.
In `@src/main/services/infrastructure/SubagentMessageCache.ts`:
- Around line 29-33: The constructor of SubagentMessageCache must guard against
invalid inputs: clamp maxSize to at least 1 and clamp ttlMinutes to a sensible
positive minimum (e.g. 1 minute) before assigning this.maxSize and this.ttlMs so
set() behavior is predictable; update the constructor (constructor(...),
this.maxSize, this.ttlMs, enabled) to validate/clamp inputs (or throw on
invalid) and convert ttlMinutes to milliseconds after validation so downstream
methods like set() rely on safe values.
In `@test/main/services/infrastructure/SubagentMessageCache.test.ts`:
- Around line 75-83: The TTL test for SubagentMessageCache uses
vi.useFakeTimers() but restores with vi.useRealTimers() unconditionally, which
can leak fake timers if an assertion throws; update the 'expires entries past
their TTL' test to ensure timers are always reset by wrapping the test body in a
try/finally (call vi.useRealTimers() in finally) or move timer setup/teardown
into a shared beforeEach/afterEach, referencing the
vi.useFakeTimers()/vi.useRealTimers() calls and the test that constructs new
SubagentMessageCache(3, 1) so cleanup always runs even on failures.
---
Nitpick comments:
In `@src/main/http/index.ts`:
- Line 31: The import of the SubagentMessageCache type in index.ts should use
the services barrel export: update the import to pull SubagentMessageCache from
'../services' (i.e., import type { SubagentMessageCache } from '../services')
and, if the type is not already exported, add an export of SubagentMessageCache
in the services barrel (src/main/services/index.ts) re-exporting it from
'./infrastructure/SubagentMessageCache' so callers can rely on the top-level
services barrel.
In `@src/main/index.ts`:
- Around line 720-756: The memory monitor interval created as
memoryMonitorInterval inside createWindow is never cleared when the window
closes, so it keeps running and may log errors after mainWindow becomes null;
update the mainWindow.on('closed') / 'closed' handler to call
clearInterval(memoryMonitorInterval) (and optionally set memoryMonitorInterval
to undefined) so the interval stops when the window is closed, ensuring the
periodic check and writeCrashLog calls cease once the window is destroyed.
In `@src/main/services/discovery/ProjectScanner.ts`:
- Around line 63-64: Replace the hardcoded MAX_CACHE_ENTRIES in ProjectScanner
with a shared constant: import the cache size constant exported from
src/shared/constants/cache.ts and use it in place of
ProjectScanner.MAX_CACHE_ENTRIES; remove the private static readonly
MAX_CACHE_ENTRIES declaration from the ProjectScanner class and update any
references (e.g., cache construction or LRU init inside ProjectScanner) to
reference the shared symbol (e.g., DEFAULT_CACHE_MAX_ENTRIES or
CACHE_MAX_ENTRIES) exported by cache.ts so cache sizing is centralized and
configurable across services.
In `@src/main/services/infrastructure/ServiceContext.ts`:
- Line 115: Replace the hardcoded 10 used when constructing SubagentMessageCache
in ServiceContext with a named constant: add a new exported constant (e.g.
SUBAGENT_MESSAGE_CACHE_SIZE) to your shared cache constants module and then
change the constructor call in ServiceContext (the line creating new
SubagentMessageCache(...)) to use that constant instead of 10; ensure the
constant is exported from the constants file and imported into ServiceContext so
cache knobs remain centralized.
🪄 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: c30f1feb-36c6-4e29-bf6f-d3dc50973176
📒 Files selected for processing (16)
src/main/http/index.tssrc/main/http/subagents.tssrc/main/index.tssrc/main/ipc/subagents.tssrc/main/services/analysis/SubagentDetailBuilder.tssrc/main/services/discovery/ProjectScanner.tssrc/main/services/infrastructure/FileWatcher.tssrc/main/services/infrastructure/ServiceContext.tssrc/main/services/infrastructure/SubagentMessageCache.tssrc/main/services/infrastructure/index.tssrc/main/standalone.tssrc/preload/constants/ipcChannels.tssrc/preload/index.tssrc/renderer/api/httpClient.tssrc/shared/types/api.tstest/main/services/infrastructure/SubagentMessageCache.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/services/infrastructure/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/preload/constants/ipcChannels.ts
- src/preload/index.ts
- src/shared/types/api.ts
- src/main/services/infrastructure/FileWatcher.ts
| export async function buildSubagentDetail( | ||
| projectId: string, | ||
| _sessionId: string, // Unused but kept for API consistency | ||
| sessionId: string, |
There was a problem hiding this comment.
Update JSDoc to match the new sessionId usage.
The signature now uses sessionId, but the doc block still describes _sessionId as unused (see Line 35). Please sync the docs with actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/services/analysis/SubagentDetailBuilder.ts` at line 46, The JSDoc
for SubagentDetailBuilder still references an unused `_sessionId` but the code
now accepts and uses `sessionId`; update the docblock for the constructor (or
the function that declares `sessionId`) to describe the `sessionId: string`
parameter correctly, remove the outdated `_sessionId` note, and ensure the param
tag matches the exact symbol name `sessionId` used in the method signature so
documentation and code are in sync.
| constructor(maxSize: number = 10, ttlMinutes: number = 10, enabled: boolean = true) { | ||
| this.maxSize = maxSize; | ||
| this.ttlMs = ttlMinutes * 60 * 1000; | ||
| this.enabled = enabled; | ||
| } |
There was a problem hiding this comment.
Guard invalid cache config values in constructor.
If maxSize <= 0, set() can still retain one entry. Clamp or reject invalid maxSize/ttlMinutes to keep behavior predictable.
Suggested patch
constructor(maxSize: number = 10, ttlMinutes: number = 10, enabled: boolean = true) {
- this.maxSize = maxSize;
- this.ttlMs = ttlMinutes * 60 * 1000;
+ this.maxSize = Math.max(1, maxSize);
+ this.ttlMs = Math.max(1, ttlMinutes) * 60 * 1000;
this.enabled = enabled;
}📝 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.
| constructor(maxSize: number = 10, ttlMinutes: number = 10, enabled: boolean = true) { | |
| this.maxSize = maxSize; | |
| this.ttlMs = ttlMinutes * 60 * 1000; | |
| this.enabled = enabled; | |
| } | |
| constructor(maxSize: number = 10, ttlMinutes: number = 10, enabled: boolean = true) { | |
| this.maxSize = Math.max(1, maxSize); | |
| this.ttlMs = Math.max(1, ttlMinutes) * 60 * 1000; | |
| this.enabled = enabled; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/services/infrastructure/SubagentMessageCache.ts` around lines 29 -
33, The constructor of SubagentMessageCache must guard against invalid inputs:
clamp maxSize to at least 1 and clamp ttlMinutes to a sensible positive minimum
(e.g. 1 minute) before assigning this.maxSize and this.ttlMs so set() behavior
is predictable; update the constructor (constructor(...), this.maxSize,
this.ttlMs, enabled) to validate/clamp inputs (or throw on invalid) and convert
ttlMinutes to milliseconds after validation so downstream methods like set()
rely on safe values.
| it('expires entries past their TTL', () => { | ||
| vi.useFakeTimers(); | ||
| const shortLived = new SubagentMessageCache(3, 1); // 1 minute TTL | ||
| shortLived.set('k', fakeMessages(1)); | ||
| expect(shortLived.get('k')).toBeDefined(); | ||
| vi.advanceTimersByTime(2 * 60 * 1000); | ||
| expect(shortLived.get('k')).toBeUndefined(); | ||
| vi.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Harden fake-timer cleanup in TTL test.
Wrap fake-timer usage in try/finally (or restore in afterEach) so timers are always reset even if an assertion throws.
Suggested patch
it('expires entries past their TTL', () => {
- vi.useFakeTimers();
- const shortLived = new SubagentMessageCache(3, 1); // 1 minute TTL
- shortLived.set('k', fakeMessages(1));
- expect(shortLived.get('k')).toBeDefined();
- vi.advanceTimersByTime(2 * 60 * 1000);
- expect(shortLived.get('k')).toBeUndefined();
- vi.useRealTimers();
+ vi.useFakeTimers();
+ try {
+ const shortLived = new SubagentMessageCache(3, 1); // 1 minute TTL
+ shortLived.set('k', fakeMessages(1));
+ expect(shortLived.get('k')).toBeDefined();
+ vi.advanceTimersByTime(2 * 60 * 1000);
+ expect(shortLived.get('k')).toBeUndefined();
+ } finally {
+ vi.useRealTimers();
+ }
});📝 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.
| it('expires entries past their TTL', () => { | |
| vi.useFakeTimers(); | |
| const shortLived = new SubagentMessageCache(3, 1); // 1 minute TTL | |
| shortLived.set('k', fakeMessages(1)); | |
| expect(shortLived.get('k')).toBeDefined(); | |
| vi.advanceTimersByTime(2 * 60 * 1000); | |
| expect(shortLived.get('k')).toBeUndefined(); | |
| vi.useRealTimers(); | |
| }); | |
| it('expires entries past their TTL', () => { | |
| vi.useFakeTimers(); | |
| try { | |
| const shortLived = new SubagentMessageCache(3, 1); // 1 minute TTL | |
| shortLived.set('k', fakeMessages(1)); | |
| expect(shortLived.get('k')).toBeDefined(); | |
| vi.advanceTimersByTime(2 * 60 * 1000); | |
| expect(shortLived.get('k')).toBeUndefined(); | |
| } finally { | |
| vi.useRealTimers(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/main/services/infrastructure/SubagentMessageCache.test.ts` around lines
75 - 83, The TTL test for SubagentMessageCache uses vi.useFakeTimers() but
restores with vi.useRealTimers() unconditionally, which can leak fake timers if
an assertion throws; update the 'expires entries past their TTL' test to ensure
timers are always reset by wrapping the test body in a try/finally (call
vi.useRealTimers() in finally) or move timer setup/teardown into a shared
beforeEach/afterEach, referencing the vi.useFakeTimers()/vi.useRealTimers()
calls and the test that constructs new SubagentMessageCache(3, 1) so cleanup
always runs even on failures.
1 participant
Summary
invalidateCachesForSession()for single-file cache invalidation instead of purging entire project on every file-change event~/.claude/claude-devtools-crash.logsubagent:get-messages) for on-demand subagent message loading so the renderer can fetch bodies only when a subagent is expandedbuildSubagentsPath()(was missing sessionId segment)Test plan
pnpm typecheck && pnpm lint:fix— cleanpnpm test— all tests pass (including new SubagentMessageCache tests)🤖 Generated with Claude Code
Summary by CodeRabbit