perf: add Worker Thread for non-blocking session parsing#167
perf: add Worker Thread for non-blocking session parsing#167psypeal 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 (--)
Move the JS parsing pipeline (parseJsonlFile → processMessages → buildChunks → resolveSubagents) to a Node.js Worker Thread so the main Electron process stays responsive during large session loads. Falls back to inline parsing if the worker fails or times out (30s). 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}
There was a problem hiding this comment.
Code Review
This pull request implements a high-performance session parsing architecture using a native Rust module and a background worker thread fallback to ensure the main process remains responsive. It optimizes IPC data transfer by stripping raw messages and introduces several UI improvements, including a dedicated Bash tool viewer, collapsible output sections, and Markdown preview capabilities. Technical feedback focuses on awaiting worker termination, dynamically scaling concurrency limits, improving subagent timestamp fallbacks, and eliminating redundant metric calculations, while also addressing a regression in Markdown syntax highlighting.
|
|
||
| private restartWorker(): void { | ||
| logger.warn('Restarting worker due to timeout'); | ||
| void this.worker?.terminate(); |
There was a problem hiding this comment.
The terminate() call on the worker is asynchronous and returns a Promise. It should be awaited to ensure the worker is fully shut down before the pool continues, especially during a restart where a new worker might be spawned immediately.
await this.worker?.terminate();| } | ||
|
|
||
| // Parse subagent files with bounded concurrency | ||
| const concurrency = 24; |
There was a problem hiding this comment.
The concurrency limit for subagent parsing is hardcoded to 24. While this might be fine for I/O-bound tasks, it could lead to resource contention on machines with fewer cores if the parsing logic is CPU-intensive. Consider using a value based on os.cpus().length or a more conservative constant.
| if (timestamps.length === 0) { | ||
| const now = new Date(); | ||
| startTime = now; | ||
| endTime = now; | ||
| durationMs = 0; |
There was a problem hiding this comment.
Falling back to new Date() when no timestamps are found in a subagent file might lead to incorrect chronological ordering in the UI, as the subagent will appear to have started 'now'. Consider using the file's creation or modification time as a more accurate fallback if available.
| session.hasSubagents = subagents.length > 0; | ||
|
|
||
| const chunks = chunkBuilder.buildChunks(parsedSession.messages, subagents); | ||
| const metrics = calculateMetrics(parsedSession.messages); |
There was a problem hiding this comment.
This call to calculateMetrics is redundant because parsedSession.metrics was already calculated during the processMessages call on line 421.
| const metrics = calculateMetrics(parsedSession.messages); | |
| const metrics = parsedSession.metrics; |
|
|
||
| // 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 this exclusion list while not adding it to the KEYWORDS map will cause markdown files viewed in code mode to be rendered as plain text without any generic highlighting (e.g., for strings or comments).
| if (keywords.size === 0 && !['json', 'css', 'bash'].includes(language)) { | |
| if (keywords.size === 0 && !['json', 'css', 'bash', 'markdown'].includes(language)) { |
📝 WalkthroughWalkthroughThis PR introduces worker-thread session parsing with native Rust chunking fallback, implements a Ctrl/Cmd+R keyboard shortcut for session refresh via IPC bridge, and adds new renderer components for Bash tool viewing and collapsible output sections. Additional enhancements include syntax highlighting expansion and markdown preview toggles for tool viewers. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/hooks/useKeyboardShortcuts.ts (1)
259-271:⚠️ Potential issue | 🔴 CriticalRemove the Cmd+R keydown handler — it is dead code unreachable by the main process.
The main process
before-input-eventhandler (src/main/index.ts:491-498) interceptsCtrl+Cmd+Rby callingevent.preventDefault(), which blocks the keydown event from reaching the renderer entirely. The explicit comment confirms: "Chromium handles Ctrl+R at the browser engine level, which also blocks the keydown from reaching the renderer." The IPC-driven refresh path (session:refresh) is the only active code path. The keydown handler at lines 259-271 will never execute.🤖 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 259 - 271, Remove the unreachable Cmd+R keydown handler inside useKeyboardShortcuts: delete the block that checks for event.key === 'r' and calls refreshSessionInPlace(selectedProjectId, selectedSessionId), fetchSessions(selectedProjectId), and dispatches the 'session-refresh-scroll-bottom' CustomEvent, since the main process intercepts Ctrl+Cmd+R and prevents this renderer handler from ever running; rely on the existing IPC-driven session:refresh path instead.src/renderer/components/chat/viewers/syntaxHighlighter.ts (1)
846-851:⚠️ Potential issue | 🟡 MinorRemove json and css from the pass-through exclusion list to prevent mis-highlighting.
Line 850 explicitly allows
jsonandcssto bypass the plain-text fallback even though neither language has keywords defined in KEYWORDS. Both will fall through to the generic tokenizer where the unguarded//comment check (line 914) will mis-highlight valid CSS constructs likeurl(http://...)and potentially break JSON content. Onlybashhas actual keyword support and should bypass the fallback;jsonandcssshould return plain text.Suggested fix
- if (keywords.size === 0 && !['json', 'css', 'bash'].includes(language)) { + if (keywords.size === 0) { return [line]; }🤖 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 846 - 851, In highlightLine, remove 'json' and 'css' from the pass-through exclusion so they don't bypass the plain-text fallback; currently the condition uses KEYWORDS and the hardcoded array ['json', 'css', 'bash'] which lets json/css fall through to the generic tokenizer (and the unguarded '//' comment check) causing mis-highlighting — change the condition to only exempt languages with actual keyword support (or only 'bash') so that KEYWORDS[language] === undefined || keywords.size === 0 causes json and css to return [line] instead of being tokenized.
🧹 Nitpick comments (8)
src/renderer/components/chat/ChatHistory.tsx (1)
380-387: Consider scoping scroll-to-bottom to the active tab only.The event listener triggers
scrollToBottomregardless of whether thisChatHistoryinstance belongs to the currently active tab. If multiple tabs are open, each inactive tab'sChatHistorywill also scroll, which is unnecessary DOM work.♻️ Optional: Guard with isThisTabActive
// Listen for session-refresh-scroll-bottom events (from Ctrl+R / refresh button) useEffect(() => { const handler = (): void => { + if (!isThisTabActive) return; scrollToBottom('smooth'); }; window.addEventListener('session-refresh-scroll-bottom', handler); return () => window.removeEventListener('session-refresh-scroll-bottom', handler); - }, [scrollToBottom]); + }, [scrollToBottom, isThisTabActive]);🤖 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 handler in ChatHistory currently calls scrollToBottom unconditionally; modify the handler to first check whether this ChatHistory instance is the active tab (use the existing isThisTabActive check/prop or an equivalent selector) and only call scrollToBottom('smooth') when true. Update the handler attached in useEffect (the function declared as handler and the window.addEventListener/removeEventListener pair) to include that guard so inactive tabs do no DOM work.src/main/utils/nativeJsonl.ts (1)
97-102: Consider logging a warning for invalid timestamp values.The
toDatefunction silently falls back tonew Date()(current time) when the input is neither aDatenor a string. This could mask data corruption from the Rust module—if a timestamp comes through asnull,undefined, or a number, the chunk would get an incorrect timestamp without any indication.♻️ Proposed improvement
/** Convert ISO-8601 timestamp string to Date object. */ function toDate(value: unknown): Date { if (value instanceof Date) return value; if (typeof value === 'string') return new Date(value); + if (value != null) { + logger.warn('Unexpected timestamp type from native module:', typeof value, value); + } return new Date(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/nativeJsonl.ts` around lines 97 - 102, The toDate function currently silently falls back to new Date() for non-Date/non-string inputs; update toDate to log a warning (e.g., console.warn) when the input is an unexpected type or when parsing a string yields an invalid Date (isNaN(date.getTime())), include the original value and typeof in the message, and still return new Date() as the safe default; reference the toDate function to locate where to add these checks and warnings.src/main/workers/sessionParseWorker.ts (2)
441-447: Unnecessary message serialization overhead — worker returns messages that are immediately discarded.The worker includes
messages: parsedSession.messagesin the response, butsrc/main/ipc/sessions.ts(line 338) immediately strips them withmessages: []before IPC transfer to renderer. For large sessions with thousands of messages, this wastes structured-clone serialization time.⚡ Proposed optimization: Don't include messages in worker response
const sessionDetail: SessionDetail = { session, - messages: parsedSession.messages, + messages: [], // Stripped by caller anyway; skip serialization overhead chunks, processes: subagents, metrics, };Note: If
processes[].messagesis also stripped (per sessions.ts line 339), consider clearing those too:- processes: subagents, + processes: subagents.map(p => ({ ...p, messages: [] })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/workers/sessionParseWorker.ts` around lines 441 - 447, The worker is including parsedSession.messages in the SessionDetail response which is immediately discarded downstream; remove or omit messages from the sessionDetail object to avoid expensive structured-clone serialization. Update the construction of sessionDetail (symbol: sessionDetail) to exclude parsedSession.messages (and also clear any messages arrays on process entries in subagents/processes if they exist) so the worker returns only needed fields (session, chunks, processes, metrics) before IPC. Ensure any callers expecting messages are adjusted or validated so behavior remains correct after removal.
57-101: Code duplication with SubagentResolver requires synchronized maintenance.This worker duplicates significant logic from
SubagentResolverandSessionParser:
processMessagesmirrorsSessionParser.processMessagesresolveSubagentsFromPathsmirrorsSubagentResolver.resolveSubagentslinkToTaskCallsmirrorsSubagentResolver's task-call linking phasesThis is a reasonable tradeoff to avoid cross-process dependencies in the worker thread, but future changes to the main-thread implementations must be synchronized here.
Consider extracting the pure-function logic into a shared module that both the worker and main-thread services can import, if the maintenance burden becomes significant.
Also applies to: 109-156, 245-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/workers/sessionParseWorker.ts` around lines 57 - 101, The worker duplicates pure parsing/linking logic from SessionParser.processMessages, SubagentResolver.resolveSubagents, and SubagentResolver.linkToTaskCalls which causes maintenance drift; extract the shared pure functions (e.g., processMessages logic, resolveSubagentsFromPaths behavior, and linkToTaskCalls phases) into a new shared module (e.g., parsing/shared or session/utils) that exports the functions, update sessionParseWorker.ts to import and call those exports (instead of keeping its own copies), and update the main-thread SessionParser and SubagentResolver to import the same shared functions so both sides use the single implementation; run/adjust existing tests to ensure behavior is unchanged.src/main/ipc/sessions.ts (1)
37-38: Minor:nativeDisabledSessionsSet grows unbounded during app lifetime.The Set accumulates session keys and is only cleared when handlers are removed (app shutdown). For long-running sessions with many failed native attempts, this could grow. Consider adding a size cap or LRU eviction if this becomes an issue in practice, though for typical usage patterns this is likely acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/sessions.ts` around lines 37 - 38, nativeDisabledSessions currently is an unbounded Set that can grow indefinitely; replace it with a bounded LRU structure (either a small custom Map-based LRU or a dependency like lru-cache) so adding a session key evicts the oldest entry when capacity is exceeded. Locate usages of nativeDisabledSessions (creation and calls to .add()/.has()) and change initialization to an LRU cache (e.g., new LRU({ max: N })) or implement a simple LRU via Map: on insert, if size > MAX then remove first Map key; keep lookup semantics via .has()/ .set()/ .delete() to match existing code paths in sessions.ts (nativeDisabledSessions).src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx (1)
62-88: Consider extracting the markdown mode toggle into a shared component.The toggle markup/style here is effectively duplicated from
WriteToolViewer; extracting a small shared control would reduce drift and simplify future changes.🤖 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 62 - 88, The markdown mode toggle UI in ReadToolViewer (the block that checks isMarkdownFile and uses viewMode/setViewMode with the two buttons) is duplicated with WriteToolViewer; extract this into a small shared component (e.g., MarkdownViewToggle) that accepts props {viewMode, onChange} or similar and encapsulates the two styled buttons and active styling, then replace the inline toggle in ReadToolViewer and WriteToolViewer with the new component to avoid drift and centralize styling/behavior.src/renderer/utils/toolRendering/index.ts (1)
7-13: Avoid extending renderer-utils barrel exports.At Lines 7-13, this adds another re-export in a renderer
utilsbarrel. Prefer direct imports fromtoolContentChecks.tsto stay aligned with the renderer import rule.As per coding guidelines: Renderer utils, hooks, and types should NOT use barrel exports - import directly from specific files.
🤖 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, This barrel re-export adds renderer-utils exports that violate the "no barrel exports" rule; remove the re-export lines exporting hasBashContent, hasEditContent, hasReadContent, hasSkillInstructions, and hasWriteContent from this barrel (index.ts) and instead update any consumers to import those symbols directly from './toolContentChecks' (use the function names hasBashContent, hasEditContent, hasReadContent, hasSkillInstructions, hasWriteContent to locate usages). Ensure the index.ts no longer re-exports these symbols and that all imports in renderer code reference the specific file './toolContentChecks'.src/renderer/utils/toolRendering/toolContentChecks.ts (1)
63-65: Harden Bash content detection with a string type guard.At Line 64, truthiness allows non-string payloads (e.g., objects) to pass as “content.” Validate command type/emptiness explicitly.
Suggested fix
+function isNonEmptyString(value: unknown): value is string { + return typeof value === 'string' && value.trim().length > 0; +} + export function hasBashContent(linkedTool: LinkedToolItem): boolean { - return !!linkedTool.input?.command; + return isNonEmptyString(linkedTool.input?.command); }As per coding guidelines: Use TypeScript type guards with isXxx naming convention for runtime type checking.
🤖 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, The current hasBashContent(linkedTool: LinkedToolItem) uses truthiness which allows non-string values; add a TypeScript runtime type guard named isBashCommand(value: unknown): value is string that returns true only for typeof value === 'string' && value.trim().length > 0, then update hasBashContent to call isBashCommand(linkedTool.input?.command) and return that result; reference the LinkedToolItem type and the hasBashContent and isBashCommand symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron.vite.config.ts`:
- Around line 20-24: The resolveId hook contains a dead exemption for the
literal 'claude-devtools-native' that is never imported or used; remove the
if-check that returns null for source.includes('claude-devtools-native') to
eliminate dead code in the resolveId function, or if you intend to reserve this
behavior, replace it with a concise comment above resolveId explaining why the
exemption exists and when it should be re-enabled; reference the resolveId
function and the string 'claude-devtools-native' when making the change so
reviewers can verify the intent.
In `@src/main/index.ts`:
- Around line 496-500: Replace the hardcoded IPC channel string with the
exported constant: add an import for SESSION_REFRESH (from the same ipc channel
module used elsewhere) at the top of the file near the other IPC constants, then
change the call mainWindow.webContents.send('session:refresh') to use
SESSION_REFRESH instead; keep the rest of the handler (event.preventDefault,
return) unchanged so the behavior remains the same.
In `@src/main/workers/SessionParserPool.ts`:
- Around line 79-85: The exit handler in SessionParserPool currently nulls
this.worker unconditionally, causing a race where an old worker's exit can
nullify a newly created worker; update the handlers (the worker.on('exit') block
and the similar handler around lines 92-96) to capture the specific worker
instance in a local variable (e.g., const current = this.worker) when
registering the listener and inside the listener only call
this.rejectAllPending(...) and set this.worker = null if and only if this.worker
=== current; use the same pattern in restartWorker(), ensureWorker(), and any
other worker event listeners so handlers operate on the exact worker instance
that emitted the event.
In `@src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx`:
- Around line 45-47: The CollapsibleOutputSection is receiving a fully-evaluated
child because renderOutput(linkedTool.result.content) runs eagerly; change to
pass a lazy render prop or deferred child so the expensive render only runs when
the section is expanded. Locate the usage in BashToolViewer where linkedTool,
CollapsibleOutputSection and renderOutput are referenced and replace the direct
call renderOutput(linkedTool.result.content) with a render prop (e.g.,
renderContent={() => renderOutput(linkedTool.result.content)}) or a conditional
mount (only call renderOutput when CollapsibleOutputSection reports open) so the
heavy rendering is deferred until expansion.
- Around line 24-26: Replace the unsafe type assertions in BashToolViewer by
validating linkedTool.input.command and linkedTool.input.description at runtime
using an isString-style guard (e.g., isString(value)): check that
linkedTool.input.command is a string before passing it to CodeBlockViewer and
that linkedTool.input.description is a string before accessing
description.length; provide sensible fallbacks or early returns when validation
fails so you don't call CodeBlockViewer or access description.length on
non-string values. Reference BashToolViewer, linkedTool.input.command,
linkedTool.input.description, CodeBlockViewer, and description.length when
making these changes.
In `@src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx`:
- Around line 30-40: The collapse toggle in CollapsibleOutputSection does not
expose ARIA disclosure semantics; update the button that toggles isExpanded to
include aria-expanded={isExpanded} and aria-controls referencing the controlled
content's id (generate a stable id, e.g., `${label}-panel` or derive one inside
the component) and add the same id to the collapsible container (the element
rendered when isExpanded) so assistive tech can associate the button and panel;
also ensure the panel reflects hidden state (e.g., aria-hidden={!isExpanded}) if
needed.
In `@src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx`:
- Around line 57-58: Initializing viewMode from filePath only once can
desynchronize the UI when filePath changes; update viewMode whenever filePath
changes by recomputing isMarkdownFile (using the same /\.mdx?$/i.test(filePath)
logic) inside a React.useEffect that depends on filePath and call
setViewMode('preview' | 'code') only if the computed mode differs from the
current viewMode to avoid stomping user toggles; reference the existing
isMarkdownFile computation, viewMode, setViewMode and filePath when locating
where to add the effect.
In `@src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx`:
- Line 24: The initial viewMode state in WriteToolViewer is only set at mount
and can become stale if the file type changes; add an effect that watches
isMarkdownFile (or filePath) and calls setViewMode(isMarkdownFile ? 'preview' :
'code') to sync the mode whenever the file type changes so the default preview
for markdown is applied after updates.
In `@src/renderer/components/chat/items/LinkedToolItem.tsx`:
- Around line 149-151: The LinkedToolItem change introduces a toolVariant and
passes variant={toolVariant} into BaseItem, but BaseItemProps and BaseItem do
not accept or use a variant prop (type-breaking/ignored). Fix by either removing
the unsupported prop from the JSX in LinkedToolItem (delete
variant={toolVariant}) or if you intend to support variants, add a variant field
to BaseItemProps and update BaseItem to consume and render based on that prop
(update type and implementation). Refer to LinkedToolItem (toolVariant, JSX prop
usage) and BaseItem/BaseItemProps when making the change.
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 924-929: The comment-start detection in syntaxHighlighter.ts
currently uses remaining.startsWith('#') for multiple languages and
mis-highlights cases like `${var#pat}` or URLs; update the logic so that for
bash/zsh/fish and yaml you only treat '#' as a comment when it's at the start of
line or the previous character is a whitespace or a token boundary/operator
(e.g., whitespace, start of string, or shell operators like | & ; < > = ( ) , +
- * /), while keeping the existing behavior for python/ruby/php (allow '#'
anywhere). Locate the check using language and remaining.startsWith('#') and
split it into two branches: one branch for shell/yaml that inspects the
preceding character in the input/token buffer to confirm a boundary before
treating '#' as comment, and the other branch for python/ruby/php that continues
to accept '#' unconditionally.
---
Outside diff comments:
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 846-851: In highlightLine, remove 'json' and 'css' from the
pass-through exclusion so they don't bypass the plain-text fallback; currently
the condition uses KEYWORDS and the hardcoded array ['json', 'css', 'bash']
which lets json/css fall through to the generic tokenizer (and the unguarded
'//' comment check) causing mis-highlighting — change the condition to only
exempt languages with actual keyword support (or only 'bash') so that
KEYWORDS[language] === undefined || keywords.size === 0 causes json and css to
return [line] instead of being tokenized.
In `@src/renderer/hooks/useKeyboardShortcuts.ts`:
- Around line 259-271: Remove the unreachable Cmd+R keydown handler inside
useKeyboardShortcuts: delete the block that checks for event.key === 'r' and
calls refreshSessionInPlace(selectedProjectId, selectedSessionId),
fetchSessions(selectedProjectId), and dispatches the
'session-refresh-scroll-bottom' CustomEvent, since the main process intercepts
Ctrl+Cmd+R and prevents this renderer handler from ever running; rely on the
existing IPC-driven session:refresh path instead.
---
Nitpick comments:
In `@src/main/ipc/sessions.ts`:
- Around line 37-38: nativeDisabledSessions currently is an unbounded Set that
can grow indefinitely; replace it with a bounded LRU structure (either a small
custom Map-based LRU or a dependency like lru-cache) so adding a session key
evicts the oldest entry when capacity is exceeded. Locate usages of
nativeDisabledSessions (creation and calls to .add()/.has()) and change
initialization to an LRU cache (e.g., new LRU({ max: N })) or implement a simple
LRU via Map: on insert, if size > MAX then remove first Map key; keep lookup
semantics via .has()/ .set()/ .delete() to match existing code paths in
sessions.ts (nativeDisabledSessions).
In `@src/main/utils/nativeJsonl.ts`:
- Around line 97-102: The toDate function currently silently falls back to new
Date() for non-Date/non-string inputs; update toDate to log a warning (e.g.,
console.warn) when the input is an unexpected type or when parsing a string
yields an invalid Date (isNaN(date.getTime())), include the original value and
typeof in the message, and still return new Date() as the safe default;
reference the toDate function to locate where to add these checks and warnings.
In `@src/main/workers/sessionParseWorker.ts`:
- Around line 441-447: The worker is including parsedSession.messages in the
SessionDetail response which is immediately discarded downstream; remove or omit
messages from the sessionDetail object to avoid expensive structured-clone
serialization. Update the construction of sessionDetail (symbol: sessionDetail)
to exclude parsedSession.messages (and also clear any messages arrays on process
entries in subagents/processes if they exist) so the worker returns only needed
fields (session, chunks, processes, metrics) before IPC. Ensure any callers
expecting messages are adjusted or validated so behavior remains correct after
removal.
- Around line 57-101: The worker duplicates pure parsing/linking logic from
SessionParser.processMessages, SubagentResolver.resolveSubagents, and
SubagentResolver.linkToTaskCalls which causes maintenance drift; extract the
shared pure functions (e.g., processMessages logic, resolveSubagentsFromPaths
behavior, and linkToTaskCalls phases) into a new shared module (e.g.,
parsing/shared or session/utils) that exports the functions, update
sessionParseWorker.ts to import and call those exports (instead of keeping its
own copies), and update the main-thread SessionParser and SubagentResolver to
import the same shared functions so both sides use the single implementation;
run/adjust existing tests to ensure behavior is unchanged.
In `@src/renderer/components/chat/ChatHistory.tsx`:
- Around line 380-387: The session-refresh-scroll-bottom handler in ChatHistory
currently calls scrollToBottom unconditionally; modify the handler to first
check whether this ChatHistory instance is the active tab (use the existing
isThisTabActive check/prop or an equivalent selector) and only call
scrollToBottom('smooth') when true. Update the handler attached in useEffect
(the function declared as handler and the
window.addEventListener/removeEventListener pair) to include that guard so
inactive tabs do no DOM work.
In `@src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx`:
- Around line 62-88: The markdown mode toggle UI in ReadToolViewer (the block
that checks isMarkdownFile and uses viewMode/setViewMode with the two buttons)
is duplicated with WriteToolViewer; extract this into a small shared component
(e.g., MarkdownViewToggle) that accepts props {viewMode, onChange} or similar
and encapsulates the two styled buttons and active styling, then replace the
inline toggle in ReadToolViewer and WriteToolViewer with the new component to
avoid drift and centralize styling/behavior.
In `@src/renderer/utils/toolRendering/index.ts`:
- Around line 7-13: This barrel re-export adds renderer-utils exports that
violate the "no barrel exports" rule; remove the re-export lines exporting
hasBashContent, hasEditContent, hasReadContent, hasSkillInstructions, and
hasWriteContent from this barrel (index.ts) and instead update any consumers to
import those symbols directly from './toolContentChecks' (use the function names
hasBashContent, hasEditContent, hasReadContent, hasSkillInstructions,
hasWriteContent to locate usages). Ensure the index.ts no longer re-exports
these symbols and that all imports in renderer code reference the specific file
'./toolContentChecks'.
In `@src/renderer/utils/toolRendering/toolContentChecks.ts`:
- Around line 63-65: The current hasBashContent(linkedTool: LinkedToolItem) uses
truthiness which allows non-string values; add a TypeScript runtime type guard
named isBashCommand(value: unknown): value is string that returns true only for
typeof value === 'string' && value.trim().length > 0, then update hasBashContent
to call isBashCommand(linkedTool.input?.command) and return that result;
reference the LinkedToolItem type and the hasBashContent and isBashCommand
symbols when making the change.
🪄 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: edaa895c-aecf-428b-88a0-b8811e475aae
📒 Files selected for processing (25)
electron.vite.config.tssrc/main/index.tssrc/main/ipc/sessions.tssrc/main/types/chunks.tssrc/main/utils/nativeJsonl.tssrc/main/workers/SessionParserPool.tssrc/main/workers/sessionParseWorker.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
| enforce: 'pre', | ||
| resolveId(source) { | ||
| // Don't stub our native JSONL parser — it's loaded dynamically at runtime | ||
| if (source.includes('claude-devtools-native')) return null | ||
| if (source.endsWith('.node')) return STUB_ID |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any imports or requires that might contain 'claude-devtools-native'
echo "=== Searching for claude-devtools-native imports/requires ==="
rg -n "claude-devtools-native" --type=ts --type=js
echo ""
echo "=== Checking package.json for native module references ==="
rg -n "claude-devtools-native" package.json || echo "Not found in package.json"Repository: matt1398/claude-devtools
Length of output: 307
Remove or explain the unused 'claude-devtools-native' exemption.
Verification confirms this exemption is never triggered. The string 'claude-devtools-native' appears only in the exemption check itself and is not imported or required anywhere in the codebase or referenced in package.json. The actual native module loading occurs at runtime via dynamic require(candidate) with filesystem paths, which bypass this vite plugin entirely. This check appears to be dead code and should be removed unless it's intentionally reserved for future use (in which case, add a clarifying comment).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron.vite.config.ts` around lines 20 - 24, The resolveId hook contains a
dead exemption for the literal 'claude-devtools-native' that is never imported
or used; remove the if-check that returns null for
source.includes('claude-devtools-native') to eliminate dead code in the
resolveId function, or if you intend to reserve this behavior, replace it with a
concise comment above resolveId explaining why the exemption exists and when it
should be re-enabled; reference the resolveId function and the string
'claude-devtools-native' when making the change so reviewers can verify the
intent.
| if ((input.control || input.meta) && !input.shift && input.key.toLowerCase() === 'r') { | ||
| event.preventDefault(); | ||
| mainWindow.webContents.send('session:refresh'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the SESSION_REFRESH constant instead of hardcoded string.
Line 498 uses the hardcoded string 'session:refresh' while src/preload/index.ts imports and uses the SESSION_REFRESH constant from ipcChannels.ts. This inconsistency could lead to silent bugs if the channel name changes.
♻️ Proposed fix
Import the constant at the top of the file (around line 48 where other IPC constants are duplicated):
const SSH_STATUS = 'ssh:status';
const CONTEXT_CHANGED = 'context:changed';
+const SESSION_REFRESH = 'session:refresh';Then use it in the handler:
if ((input.control || input.meta) && !input.shift && input.key.toLowerCase() === 'r') {
event.preventDefault();
- mainWindow.webContents.send('session:refresh');
+ mainWindow.webContents.send(SESSION_REFRESH);
return;
}Based on learnings: "IPC channel constants must be defined in preload/constants/ipcChannels.ts"
📝 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.
| if ((input.control || input.meta) && !input.shift && input.key.toLowerCase() === 'r') { | |
| event.preventDefault(); | |
| mainWindow.webContents.send('session:refresh'); | |
| return; | |
| } | |
| if ((input.control || input.meta) && !input.shift && input.key.toLowerCase() === 'r') { | |
| event.preventDefault(); | |
| mainWindow.webContents.send(SESSION_REFRESH); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/index.ts` around lines 496 - 500, Replace the hardcoded IPC channel
string with the exported constant: add an import for SESSION_REFRESH (from the
same ipc channel module used elsewhere) at the top of the file near the other
IPC constants, then change the call
mainWindow.webContents.send('session:refresh') to use SESSION_REFRESH instead;
keep the rest of the handler (event.preventDefault, return) unchanged so the
behavior remains the same.
| this.worker.on('exit', (code) => { | ||
| if (code !== 0) { | ||
| logger.warn(`Worker exited with code ${code}`); | ||
| this.rejectAllPending(new Error(`Worker exited with code ${code}`)); | ||
| } | ||
| this.worker = null; | ||
| }); |
There was a problem hiding this comment.
Potential race condition: exit handler may null the wrong worker instance.
When restartWorker() is called:
void this.worker?.terminate()starts async terminationthis.worker = nullruns immediately- Next
parse()call creates a new worker viaensureWorker() - Old worker eventually exits, triggering the
exithandler - Exit handler sets
this.worker = null(line 84), nullifying the new worker
The exit handler doesn't track which worker instance triggered the event.
🐛 Proposed fix: Track worker instance to avoid nullifying wrong one
+ private workerGeneration = 0;
+
private ensureWorker(): Worker {
if (!this.worker) {
+ const generation = ++this.workerGeneration;
const workerPath = join(__dirname, 'sessionParseWorker.cjs');
this.worker = new Worker(workerPath);
// ... message handler unchanged ...
this.worker.on('exit', (code) => {
+ // Only handle exit for current generation
+ if (this.workerGeneration !== generation) return;
if (code !== 0) {
logger.warn(`Worker exited with code ${code}`);
this.rejectAllPending(new Error(`Worker exited with code ${code}`));
}
this.worker = null;
});
+ this.worker.on('error', (err: Error) => {
+ if (this.workerGeneration !== generation) return;
+ logger.error('Worker error:', err);
+ this.rejectAllPending(new Error('Worker error: ' + err.message));
+ this.worker = null;
+ });Also applies to: 92-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/workers/SessionParserPool.ts` around lines 79 - 85, The exit handler
in SessionParserPool currently nulls this.worker unconditionally, causing a race
where an old worker's exit can nullify a newly created worker; update the
handlers (the worker.on('exit') block and the similar handler around lines
92-96) to capture the specific worker instance in a local variable (e.g., const
current = this.worker) when registering the listener and inside the listener
only call this.rejectAllPending(...) and set this.worker = null if and only if
this.worker === current; use the same pattern in restartWorker(),
ensureWorker(), and any other worker event listeners so handlers operate on the
exact worker instance that emitted the event.
| export const BashToolViewer: React.FC<BashToolViewerProps> = ({ linkedTool, status }) => { | ||
| 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:
# Find and examine BashToolViewer.tsx
find . -name "BashToolViewer.tsx" -type fRepository: matt1398/claude-devtools
Length of output: 134
🏁 Script executed:
# Also search for the file using git
git ls-files | grep -i "bashtoolviewer"Repository: matt1398/claude-devtools
Length of output: 132
🏁 Script executed:
# Check if the directory structure matches
git ls-files "src/renderer/components/chat/items/linkedTool/" | head -20Repository: matt1398/claude-devtools
Length of output: 723
🏁 Script executed:
# Read BashToolViewer.tsx to examine the code at lines 24-26
cat -n src/renderer/components/chat/items/linkedTool/BashToolViewer.tsx | head -60Repository: matt1398/claude-devtools
Length of output: 1886
🏁 Script executed:
# Also check the type definitions for linkedTool
rg -A 10 "type.*LinkedTool|interface.*LinkedTool" src/renderer --type ts --type tsxRepository: matt1398/claude-devtools
Length of output: 95
🏁 Script executed:
# Check if toolContentChecks.ts exists and what it contains
find . -name "toolContentChecks.ts" -type fRepository: matt1398/claude-devtools
Length of output: 123
🏁 Script executed:
# Search for LinkedToolItem type definition
rg "type LinkedToolItem|interface LinkedToolItem" src/renderer --type ts -A 15Repository: matt1398/claude-devtools
Length of output: 2492
🏁 Script executed:
# Check toolContentChecks.ts to see what validation utilities exist
cat -n src/renderer/utils/toolRendering/toolContentChecks.tsRepository: matt1398/claude-devtools
Length of output: 2530
🏁 Script executed:
# Search for how input properties are typed on tool items
rg "input\s*:" src/renderer/types --type ts -B 2 -A 3Repository: matt1398/claude-devtools
Length of output: 758
🏁 Script executed:
# Check how other tool viewers handle input validation
rg "linkedTool\.input\." src/renderer/components/chat/items/linkedTool --type ts -B 2 -A 2 | head -50Repository: matt1398/claude-devtools
Length of output: 4440
🏁 Script executed:
# Look at how similar tools use assertions or guards elsewhere
rg "as string|as string \|" src/renderer/components/chat/items/linkedTool --type ts -B 1 -A 1Repository: matt1398/claude-devtools
Length of output: 4120
🏁 Script executed:
# Check if there are existing type guard patterns used elsewhere in renderer
rg "isString|typeof.*===.*string" src/renderer --type ts -l | head -10Repository: matt1398/claude-devtools
Length of output: 500
🏁 Script executed:
# Check how tool data is loaded/parsed to see if validation happens upstream
rg "linkedTool.*=" src/renderer --type ts -B 2 -A 2 | grep -A 5 -B 5 "parse\|validate\|input:" | head -40Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Look at the complete ReadToolViewer to see how it uses assertions
cat -n src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx | head -50Repository: matt1398/claude-devtools
Length of output: 1976
🏁 Script executed:
# Check DefaultToolViewer for comparison
cat -n src/renderer/components/chat/items/linkedTool/DefaultToolViewer.tsxRepository: matt1398/claude-devtools
Length of output: 1781
🏁 Script executed:
# Check how the input is actually used in BashToolViewer more carefully
cat -n src/renderer/components/chat/items/linkedTool/BashToolViewer.tsxRepository: matt1398/claude-devtools
Length of output: 1886
🏁 Script executed:
# Verify line 30 where description.length is called
sed -n '24,35p' src/renderer/components/chat/items/linkedTool/BashToolViewer.tsxRepository: matt1398/claude-devtools
Length of output: 518
Guard linkedTool.input values at runtime instead of using type assertions.
The as string assertions on lines 25–26 do not validate runtime data. If linkedTool.input.command or linkedTool.input.description contain non-string values, line 30 (description.length) and the CodeBlockViewer call will still fail at runtime. The coding guidelines require type guards with isXxx naming for runtime type checking.
Suggested fix
+const isString = (value: unknown): value is string => typeof value === 'string';
+
export const BashToolViewer: React.FC<BashToolViewerProps> = ({ linkedTool, status }) => {
- const command = linkedTool.input.command as string;
- const description = linkedTool.input.description as string | undefined;
+ const command = isString(linkedTool.input.command) ? linkedTool.input.command : '';
+ const description = isString(linkedTool.input.description)
+ ? linkedTool.input.description
+ : undefined;🤖 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, Replace the unsafe type assertions in BashToolViewer by
validating linkedTool.input.command and linkedTool.input.description at runtime
using an isString-style guard (e.g., isString(value)): check that
linkedTool.input.command is a string before passing it to CodeBlockViewer and
that linkedTool.input.description is a string before accessing
description.length; provide sensible fallbacks or early returns when validation
fails so you don't call CodeBlockViewer or access description.length on
non-string values. Reference BashToolViewer, linkedTool.input.command,
linkedTool.input.description, CodeBlockViewer, and description.length when
making these changes.
| {!linkedTool.isOrphaned && linkedTool.result && ( | ||
| <CollapsibleOutputSection status={status}> | ||
| {renderOutput(linkedTool.result.content)} |
There was a problem hiding this comment.
Collapsed output is still rendered eagerly.
renderOutput(linkedTool.result.content) runs before CollapsibleOutputSection can decide whether the section is open. For large Bash results, that keeps the expensive render path hot even when the UI is collapsed, which works against the perf goal for large session loads. Prefer a lazy render prop or deferred child mount here.
🤖 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 45 - 47, The CollapsibleOutputSection is receiving a fully-evaluated child
because renderOutput(linkedTool.result.content) runs eagerly; change to pass a
lazy render prop or deferred child so the expensive render only runs when the
section is expanded. Locate the usage in BashToolViewer where linkedTool,
CollapsibleOutputSection and renderOutput are referenced and replace the direct
call renderOutput(linkedTool.result.content) with a render prop (e.g.,
renderContent={() => renderOutput(linkedTool.result.content)}) or a conditional
mount (only call renderOutput when CollapsibleOutputSection reports open) so the
heavy rendering is deferred until expansion.
| <button | ||
| type="button" | ||
| className="mb-1 flex items-center gap-2 text-xs" | ||
| style={{ color: 'var(--tool-item-muted)', background: 'none', border: 'none', padding: 0, cursor: 'pointer' }} | ||
| onClick={() => setIsExpanded((prev) => !prev)} | ||
| > | ||
| {isExpanded ? <ChevronDown className="size-3" /> : <ChevronRight className="size-3" />} | ||
| {label} | ||
| <StatusDot status={status} /> | ||
| </button> | ||
| {isExpanded && ( |
There was a problem hiding this comment.
Add disclosure ARIA state to the collapse button.
At Line 30 and Line 41, the toggle lacks aria-expanded/aria-controls, so screen readers don’t get clear collapse state semantics.
Suggested fix
-import React, { useState } from 'react';
+import React, { useId, useState } from 'react';
@@
const [isExpanded, setIsExpanded] = useState(false);
+ const panelId = useId();
@@
<button
type="button"
+ aria-expanded={isExpanded}
+ aria-controls={panelId}
className="mb-1 flex items-center gap-2 text-xs"
@@
{isExpanded && (
<div
+ id={panelId}
className="max-h-96 overflow-auto rounded p-3 font-mono text-xs"Also applies to: 41-53
🤖 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 collapse toggle in CollapsibleOutputSection does not
expose ARIA disclosure semantics; update the button that toggles isExpanded to
include aria-expanded={isExpanded} and aria-controls referencing the controlled
content's id (generate a stable id, e.g., `${label}-panel` or derive one inside
the component) and add the same id to the collapsible container (the element
rendered when isExpanded) so assistive tech can associate the button and panel;
also ensure the panel reflects hidden state (e.g., aria-hidden={!isExpanded}) if
needed.
| const isMarkdownFile = /\.mdx?$/i.test(filePath); | ||
| const [viewMode, setViewMode] = React.useState<'code' | 'preview'>(isMarkdownFile ? 'preview' : 'code'); |
There was a problem hiding this comment.
viewMode can drift when filePath changes.
At Line 58, initial state is derived once; if the underlying file path changes, the default mode may no longer match the file type.
Suggested fix
- const [viewMode, setViewMode] = React.useState<'code' | 'preview'>(isMarkdownFile ? 'preview' : 'code');
+ const [viewMode, setViewMode] = React.useState<'code' | 'preview'>('code');
+
+ React.useEffect(() => {
+ setViewMode(isMarkdownFile ? 'preview' : 'code');
+ }, [isMarkdownFile, filePath]);📝 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.
| const isMarkdownFile = /\.mdx?$/i.test(filePath); | |
| const [viewMode, setViewMode] = React.useState<'code' | 'preview'>(isMarkdownFile ? 'preview' : 'code'); | |
| const isMarkdownFile = /\.mdx?$/i.test(filePath); | |
| const [viewMode, setViewMode] = React.useState<'code' | 'preview'>('code'); | |
| React.useEffect(() => { | |
| setViewMode(isMarkdownFile ? 'preview' : 'code'); | |
| }, [isMarkdownFile, filePath]); |
🤖 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 57 - 58, Initializing viewMode from filePath only once can desynchronize
the UI when filePath changes; update viewMode whenever filePath changes by
recomputing isMarkdownFile (using the same /\.mdx?$/i.test(filePath) logic)
inside a React.useEffect that depends on filePath and call setViewMode('preview'
| 'code') only if the computed mode differs from the current viewMode to avoid
stomping user toggles; reference the existing isMarkdownFile computation,
viewMode, setViewMode and filePath when locating where to add the effect.
| const isCreate = toolUseResult?.type === 'create'; | ||
| const isMarkdownFile = /\.mdx?$/i.test(filePath); | ||
| const [viewMode, setViewMode] = React.useState<'code' | 'preview'>('code'); | ||
| const [viewMode, setViewMode] = React.useState<'code' | 'preview'>(isMarkdownFile ? 'preview' : 'code'); |
There was a problem hiding this comment.
Sync viewMode when file type changes after mount.
At Line 24, viewMode is only initialized once. If filePath changes on an existing component instance, the mode can stay stale and miss the intended default (preview for markdown).
Suggested fix
- const [viewMode, setViewMode] = React.useState<'code' | 'preview'>(isMarkdownFile ? 'preview' : 'code');
+ const [viewMode, setViewMode] = React.useState<'code' | 'preview'>('code');
+
+ React.useEffect(() => {
+ setViewMode(isMarkdownFile ? 'preview' : 'code');
+ }, [isMarkdownFile, filePath]);🤖 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` at line
24, The initial viewMode state in WriteToolViewer is only set at mount and can
become stale if the file type changes; add an effect that watches isMarkdownFile
(or filePath) and calls setViewMode(isMarkdownFile ? 'preview' : 'code') to sync
the mode whenever the file type changes so the default preview for markdown is
applied after updates.
| // Determine step variant for colored borders/icons | ||
| const toolVariant: StepVariant = status === 'error' ? 'tool-error' : 'tool'; | ||
|
|
There was a problem hiding this comment.
BaseItem does not support variant; this change is currently non-functional (and likely type-breaking).
At Line 175, variant={toolVariant} is passed, but BaseItemProps (in src/renderer/components/chat/items/BaseItem.tsx) does not define variant, and BaseItem does not consume it. This either fails typecheck or is silently ignored.
Immediate safe fix (remove unsupported prop until BaseItem contract is updated)
-import type { StepVariant } from '@renderer/constants/stepVariants';
@@
- // Determine step variant for colored borders/icons
- const toolVariant: StepVariant = status === 'error' ? 'tool-error' : 'tool';
@@
- variant={toolVariant}Also applies to: 175-175
🤖 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 -
151, The LinkedToolItem change introduces a toolVariant and passes
variant={toolVariant} into BaseItem, but BaseItemProps and BaseItem do not
accept or use a variant prop (type-breaking/ignored). Fix by either removing the
unsupported prop from the JSX in LinkedToolItem (delete variant={toolVariant})
or if you intend to support variants, add a variant field to BaseItemProps and
update BaseItem to consume and render based on that prop (update type and
implementation). Refer to LinkedToolItem (toolVariant, JSX prop usage) and
BaseItem/BaseItemProps when making the change.
| // 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:
cd src/renderer/components/chat/viewers && wc -l syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 93
🏁 Script executed:
cd src/renderer/components/chat/viewers && sed -n '910,940p' syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 954
🏁 Script executed:
cd src/renderer/components/chat/viewers && sed -n '850,930p' syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 2378
🏁 Script executed:
cd src/renderer/components/chat/viewers && sed -n '1,100p' syntaxHighlighter.ts | head -50Repository: matt1398/claude-devtools
Length of output: 967
🏁 Script executed:
cd src/renderer/components/chat/viewers && sed -n '780,850p' syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 1424
🏁 Script executed:
# Let me check if there's any preceding character validation in the surrounding context
cd src/renderer/components/chat/viewers && sed -n '900,950p' syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 1507
🌐 Web query:
bash shell comment syntax rules # token boundary
💡 Result:
In Bash and POSIX-compliant shells, comments are introduced by an unquoted '#' character that begins a word. A word starts at the beginning of a line, after unquoted whitespace (spaces or tabs), or after an operator (like ';', '|', '&', '>', '<', etc.). Everything from that '#' to the end of the line is ignored, excluding the newline itself. Key rules: - The '#' must not be quoted (e.g., inside '' or "") and must start a new word—preceding unquoted whitespace or operator separates it. - In non-interactive shells (scripts), comments always work this way. - In interactive Bash shells, comments require the 'interactive_comments' shopt option (enabled by default): shopt -s interactive_comments. - During token recognition, the shell scans left-to-right; if it hits an unquoted '#', the rest of the line (including any later backslashes) is discarded as comment before further processing like line continuation. - Exception: #! shebang on first line is not a comment. Examples: echo hello # this is a comment # comment after whitespace ls | # comment after operator grep foo echo '#quoted' # '#' is literal, not comment echo hello#world # prints "hello#world" This token boundary ensures '#' only comments when it unambiguously starts a new lexical unit, preventing accidental commenting mid-word (e.g., var=val#ue works as assignment if no space). Rules unchanged as of 2026.
Citations:
- 1: https://www.gnu.org/software/bash/manual/html_node/Comments.html
- 2: https://www.gnu.org/s/bash/manual/html_node/Shell-Syntax.html
- 3: https://unix.stackexchange.com/questions/703221/posix-shell-comment-vs-line-continuation
- 4: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
- 5: https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/utilities/V3_chap02.html
- 6: https://www.filibeto.org/sun/lib/standards/susv3/utilities/xcu_chap02.html
- 7: http://www.gnu.org/s/bash/manual/html_node/Comments.html
🌐 Web query:
YAML comment syntax rules when # starts comment
💡 Result:
In YAML, a comment begins when the '#' character (octothorpe) appears outside of scalar content, after white space separating it from any preceding tokens. Everything from the '#' to the end of the line is the comment and is ignored by parsers. Comments must be separated from other tokens by white space. There are no block comments; multi-line comments require '#' on each line. Inline comments are allowed after values. '#' inside quoted or literal scalars is treated as literal content, not a comment start. Key rules from YAML 1.2.2 spec: - [12] c-comment ::= '#' - Comments on their own line: l-comment ::= s-separate-in-line c-nb-comment-text? b-comment - End-of-line comments: s-b-comment - Multiple comments allowed after lines ending with comments: s-l-comments Examples: # Full line comment key: value # inline comment # This is a multi-line comment (each line prefixed with #) Note: A recent GitHub issue highlights a potential grammar bug in rule 78 (l-comment), but existing parsers handle it correctly, and no errata issued as of 2026-04-05.
Citations:
- 1: https://yaml.org/spec/1.2.2
- 2: https://yaml.org/spec/1.2.2/
- 3: https://yaml.cc/tutorial/yaml-comment.html
- 4: https://stackoverflow.com/questions/2276572/how-do-you-do-block-comments-in-yaml
- 5: https://testkube.io/blog/yaml-commenting-best-practices-kubernetes-testing
- 6: https://kodekloud.com/blog/yaml-add-comments/
- 7: https://stackoverflow.com/questions/44583624/inline-commenting-in-yaml-files
- 8: Bug in Rule 78 (l-comment): b-comment allows content lines to be consumed as comments yaml/yaml-spec#356
🌐 Web query:
Ruby PHP comment syntax # rules
💡 Result:
Ruby comment syntax: - Single-line: Starts with # and goes to end of line. Can be anywhere, including indented. - Block: =begin on its own line (no indent) to =end on its own line (no indent). Cannot be nested or indented. - Magic comments: Special # comments like # frozen_string_literal: true or # encoding: UTF-8 at file start. PHP comment syntax: - Single-line: // or # to end of line. - Multi-line: /* to / (can span lines, nestable in some contexts but avoid nesting; works anywhere). - Note: In PHP 8+, #[...] has special attribute meaning, so prefer // for single-line. Key rules/differences: - Ruby lacks / /; uses =begin/=end instead (more restrictive). - PHP has three single-line options (# shared); Ruby only #. - Both ignore comments at parse time. - Common practice: Multiple # lines for "multiline" in Ruby; / */ for blocks/PHPDoc in PHP.
Citations:
- 1: https://ruby-doc.org/core-3.0.1/doc/syntax/comments_rdoc.html
- 2: https://docs.ruby-lang.org/en/master/syntax/comments_rdoc.html
- 3: https://docs.ruby-lang.org/en/trunk/syntax/comments_rdoc.html
- 4: https://www.php.net/manual/en/language.basic-syntax.comments.php
- 5: https://webreference.com/php/basics/comments
Add token boundary validation for bash/YAML comment detection.
The current code incorrectly treats # as a comment starter regardless of context in bash, YAML, and similar languages. In bash, # only starts a comment at word boundaries (after whitespace, operators, or line start), so ${var#pattern} and echo foo#bar would be mis-highlighted as comments. Similarly, YAML requires # to be preceded by whitespace to denote a comment, so url: https://x/#frag would be incorrectly flagged.
Python, Ruby, and PHP allow # to start comments anywhere, so they don't require this validation. Consider separating the bash/shell/yaml languages and adding a check that the preceding character (if any) is whitespace or an operator.
🤖 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 comment-start detection in syntaxHighlighter.ts currently uses
remaining.startsWith('#') for multiple languages and mis-highlights cases like
`${var#pat}` or URLs; update the logic so that for bash/zsh/fish and yaml you
only treat '#' as a comment when it's at the start of line or the previous
character is a whitespace or a token boundary/operator (e.g., whitespace, start
of string, or shell operators like | & ; < > = ( ) , + - * /), while keeping the
existing behavior for python/ruby/php (allow '#' anywhere). Locate the check
using language and remaining.startsWith('#') and split it into two branches: one
branch for shell/yaml that inspects the preceding character in the input/token
buffer to confirm a boundary before treating '#' as comment, and the other
branch for python/ruby/php that continues to accept '#' unconditionally.
1 participant
Summary
Test plan
pnpm typecheckpassespnpm test— all tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements