feat: add BashToolViewer, collapsible output, and expanded syntax highlighting#90
feat: add BashToolViewer, collapsible output, and expanded syntax highlighting#90Psypeal wants to merge 1 commit intomatt1398:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant enhancements to the tool viewing experience within the application. It adds dedicated support for Bash tools with syntax-highlighted commands and standardizes tool output sections to be collapsible, improving overall UI organization. Furthermore, it greatly expands the range of languages supported by the syntax highlighter and refines markdown file viewing with a new preview toggle, making tool interactions more intuitive and visually appealing. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a new BashToolViewer component, a reusable CollapsibleOutputSection, and significantly expands syntax highlighting capabilities for various programming languages. The changes enhance the user experience by providing syntax-highlighted command inputs for Bash tools and collapsing tool outputs by default, improving readability. Additionally, markdown files in ReadToolViewer and WriteToolViewer now default to a preview mode with a toggle to switch to code view, which is a great usability improvement. The expanded keyword sets for syntax highlighting are a valuable addition, making the code viewer more robust.
| } | ||
|
|
||
| export const BashToolViewer: React.FC<BashToolViewerProps> = ({ linkedTool, status }) => { | ||
| const command = linkedTool.input.command as string; |
There was a problem hiding this comment.
Using as string here performs a type assertion without a runtime check. If linkedTool.input.command is undefined or null, this could lead to unexpected behavior or runtime errors when command is later treated as a string. Consider adding a fallback or a more robust check.
| const command = linkedTool.input.command as string; | |
| const command = (linkedTool.input.command as string) || ''; |
| const fileName = description | ||
| ? description.length > 60 | ||
| ? description.slice(0, 57) + '...' | ||
| : description | ||
| : 'bash'; |
There was a problem hiding this comment.
The numbers 60 and 57 used for truncating the description are 'magic numbers'. It would improve readability and maintainability to define these as named constants, perhaps at the top of the file or in a shared constants file, to make their purpose clear and allow for easier modification if the truncation logic needs to change in the future.
const MAX_DESCRIPTION_LENGTH = 60;
const TRUNCATED_LENGTH = 57;
const fileName = description
? description.length > MAX_DESCRIPTION_LENGTH
? description.slice(0, TRUNCATED_LENGTH) + '...'
: description
: 'bash';
| <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)} |
There was a problem hiding this comment.
The inline styles for the button (e.g., color, background, border, padding, cursor) are static and could be moved to a CSS class. This would improve maintainability, promote consistency across components, and separate concerns between structure and styling. Consider defining these styles in a dedicated CSS module or using a utility-first CSS framework if available.
| className="max-h-96 overflow-auto rounded p-3 font-mono text-xs" | ||
| style={{ | ||
| backgroundColor: 'var(--code-bg)', | ||
| border: '1px solid var(--code-border)', | ||
| color: | ||
| status === 'error' | ||
| ? 'var(--tool-result-error-text)' | ||
| : 'var(--color-text-secondary)', | ||
| }} |
There was a problem hiding this comment.
Similar to the button, the inline styles for this div (e.g., backgroundColor, border, color) are static and could be extracted into a CSS class. This practice enhances maintainability and ensures a more consistent visual language across the application.
| style={{ | ||
| backgroundColor: viewMode === 'code' ? 'var(--tag-bg)' : 'transparent', | ||
| color: viewMode === 'code' ? 'var(--tag-text)' : 'var(--color-text-muted)', | ||
| border: '1px solid var(--tag-border)', |
There was a problem hiding this comment.
The inline styles for the 'Code' button are static and could be extracted into a CSS class. This would improve maintainability and consistency with other UI elements.
| backgroundColor: viewMode === 'preview' ? 'var(--tag-bg)' : 'transparent', | ||
| color: viewMode === 'preview' ? 'var(--tag-text)' : 'var(--color-text-muted)', | ||
| border: '1px solid var(--tag-border)', | ||
| }} |
There was a problem hiding this comment.
The inline styles for the 'Preview' button are static and could be extracted into a CSS class. This would improve maintainability and consistency with other UI elements.
Psypeal
force-pushed
the
feat/bash-viewer-collapsible-output
branch
from
6a80a45 to
4a830f0
Compare
📝 WalkthroughWalkthroughAdds Bash tool support with a new BashToolViewer and exports; introduces a reusable CollapsibleOutputSection for collapsible tool outputs; enhances linked tool viewers with Markdown preview toggles; expands syntax highlighting keyword sets and comment handling; and exports a hasBashContent utility. Changes
Possibly related PRs
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: 2
🧹 Nitpick comments (5)
src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx (1)
62-88: Consider extracting a sharedMarkdownTogglecomponent.The toggle button UI is duplicated between
ReadToolViewerandWriteToolViewer. Extracting a small reusable component would reduce duplication and ensure consistent styling.♻️ Example extraction
// MarkdownToggle.tsx interface MarkdownToggleProps { viewMode: 'code' | 'preview'; onModeChange: (mode: 'code' | 'preview') => void; } export const MarkdownToggle: React.FC<MarkdownToggleProps> = ({ viewMode, onModeChange }) => ( <div className="flex items-center justify-end gap-1"> <button type="button" onClick={() => onModeChange('code')} className="rounded px-2 py-1 text-xs transition-colors" style={{ backgroundColor: viewMode === 'code' ? 'var(--tag-bg)' : 'transparent', color: viewMode === 'code' ? 'var(--tag-text)' : 'var(--color-text-muted)', border: '1px solid var(--tag-border)', }} > Code </button> <button type="button" onClick={() => onModeChange('preview')} className="rounded px-2 py-1 text-xs transition-colors" style={{ backgroundColor: viewMode === 'preview' ? 'var(--tag-bg)' : 'transparent', color: viewMode === 'preview' ? 'var(--tag-text)' : 'var(--color-text-muted)', border: '1px solid var(--tag-border)', }} > Preview </button> </div> );🤖 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 duplicated markdown view toggle UI in ReadToolViewer (and its sibling WriteToolViewer) should be extracted into a small reusable component (e.g., MarkdownToggle) that accepts props like viewMode: 'code' | 'preview' and onModeChange: (mode) => void; replace the duplicated JSX in ReadToolViewer and WriteToolViewer with <MarkdownToggle viewMode={viewMode} onModeChange={setViewMode} /> (or equivalent) to centralize styling/behavior and avoid duplication while preserving current props/state (viewMode and setViewMode).src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx (1)
23-24: Consider syncingviewModewhenfilePathchanges.The
useStateinitializer only runs on mount. IflinkedToolchanges to a different file type after mount,viewModewon't update accordingly. If the component is always unmounted/remounted for different tools, this is fine—otherwise, consider usinguseEffectto resetviewModewhenisMarkdownFilechanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx` around lines 23 - 24, The viewMode state in WriteToolViewer is initialized from isMarkdownFile but never updates when filePath (or isMarkdownFile) changes; add a useEffect in the WriteToolViewer component that watches filePath or isMarkdownFile and calls setViewMode(isMarkdownFile ? 'preview' : 'code') to reset the mode whenever the file type changes so viewMode stays in sync with the current file.src/renderer/components/chat/viewers/syntaxHighlighter.ts (1)
807-840: Minor:autois duplicated in C++ keyword extension.
autois already inKEYWORDS.c(line 471) and is included again in the C++ extension array (line 838). TheSetconstructor handles this gracefully, but you could remove it from the extension array for clarity.🤖 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 807 - 840, The C++ keyword extension block (KEYWORDS.cpp and KEYWORDS.hpp) redundantly includes 'auto' which already exists in KEYWORDS.c; remove 'auto' from the array passed into new Set([...KEYWORDS.c, ...[ ... ]]) so KEYWORDS.cpp is a clean superset without duplicate literals while keeping KEYWORDS.hpp assigned to KEYWORDS.cpp.src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx (1)
30-39: Expose toggle state for assistive technologies.Add
aria-expandedon the button so screen readers can announce whether the section is open.♿ Suggested diff
<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' }} + aria-expanded={isExpanded} onClick={() => setIsExpanded((prev) => !prev)} >🤖 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 - 39, The button in the CollapsibleOutputSection component doesn't expose its toggle state to assistive tech; update the button element (the one using setIsExpanded and isExpanded) to include an aria-expanded attribute bound to the isExpanded boolean so screen readers can announce open/closed state (optionally also add an aria-controls referencing the collapsible region id if present to improve accessibility).src/renderer/components/chat/items/linkedTool/DefaultToolViewer.tsx (1)
7-14: Reorder imports to match repository convention.Place the
@renderer/...import before relative imports to align with the project import ordering rule.♻️ Suggested diff
import React from 'react'; +import type { LinkedToolItem } from '@renderer/types/groups'; import { type ItemStatus } from '../BaseItem'; - import { CollapsibleOutputSection } from './CollapsibleOutputSection'; import { renderInput, renderOutput } from './renderHelpers'; - -import type { LinkedToolItem } from '@renderer/types/groups';As per coding guidelines: "Organize imports in order: external packages, path aliases (
@main,@renderer,@shared,@preload), then relative imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/items/linkedTool/DefaultToolViewer.tsx` around lines 7 - 14, Reorder the imports in DefaultToolViewer.tsx to follow the repo convention: keep external package imports (React) first, then move the path-alias import "import type { LinkedToolItem } from '@renderer/types/groups';" before any relative imports, and finally list the relative imports (ItemStatus from '../BaseItem', CollapsibleOutputSection from './CollapsibleOutputSection', and renderInput/renderOutput from './renderHelpers') so the file imports are ordered: external, `@renderer`, then relative; update import order around the symbols LinkedToolItem, ItemStatus, CollapsibleOutputSection, renderInput, and renderOutput.
🤖 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/items/LinkedToolItem.tsx`:
- Line 175: The LinkedToolItem component is passing variant={toolVariant} to
BaseItem but BaseItemProps doesn't declare a variant prop; either remove the
variant prop from the BaseItem usage in LinkedToolItem (remove the
variant={toolVariant} attribute) or add a variant?: <type> entry to the
BaseItemProps interface (and handle it inside the BaseItem component
rendering/styling) so BaseItem accepts toolVariant; locate the call in
LinkedToolItem.tsx (where BaseItem is rendered) and adjust accordingly, or
update the BaseItemProps/interface and the BaseItem component to consume the new
variant property.
- Line 44: Remove the unused/nonexistent StepVariant import and the erroneous
variant prop usage: delete the import "StepVariant" from LinkedToolItem.tsx and
remove the prop variant={toolVariant} from the BaseItem JSX call (BaseItem does
not accept a variant prop and toolVariant is not defined/typed), leaving
BaseItem with only its supported props.
---
Nitpick comments:
In `@src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx`:
- Around line 30-39: The button in the CollapsibleOutputSection component
doesn't expose its toggle state to assistive tech; update the button element
(the one using setIsExpanded and isExpanded) to include an aria-expanded
attribute bound to the isExpanded boolean so screen readers can announce
open/closed state (optionally also add an aria-controls referencing the
collapsible region id if present to improve accessibility).
In `@src/renderer/components/chat/items/linkedTool/DefaultToolViewer.tsx`:
- Around line 7-14: Reorder the imports in DefaultToolViewer.tsx to follow the
repo convention: keep external package imports (React) first, then move the
path-alias import "import type { LinkedToolItem } from
'@renderer/types/groups';" before any relative imports, and finally list the
relative imports (ItemStatus from '../BaseItem', CollapsibleOutputSection from
'./CollapsibleOutputSection', and renderInput/renderOutput from
'./renderHelpers') so the file imports are ordered: external, `@renderer`, then
relative; update import order around the symbols LinkedToolItem, ItemStatus,
CollapsibleOutputSection, renderInput, and renderOutput.
In `@src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx`:
- Around line 62-88: The duplicated markdown view toggle UI in ReadToolViewer
(and its sibling WriteToolViewer) should be extracted into a small reusable
component (e.g., MarkdownToggle) that accepts props like viewMode: 'code' |
'preview' and onModeChange: (mode) => void; replace the duplicated JSX in
ReadToolViewer and WriteToolViewer with <MarkdownToggle viewMode={viewMode}
onModeChange={setViewMode} /> (or equivalent) to centralize styling/behavior and
avoid duplication while preserving current props/state (viewMode and
setViewMode).
In `@src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx`:
- Around line 23-24: The viewMode state in WriteToolViewer is initialized from
isMarkdownFile but never updates when filePath (or isMarkdownFile) changes; add
a useEffect in the WriteToolViewer component that watches filePath or
isMarkdownFile and calls setViewMode(isMarkdownFile ? 'preview' : 'code') to
reset the mode whenever the file type changes so viewMode stays in sync with the
current file.
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 807-840: The C++ keyword extension block (KEYWORDS.cpp and
KEYWORDS.hpp) redundantly includes 'auto' which already exists in KEYWORDS.c;
remove 'auto' from the array passed into new Set([...KEYWORDS.c, ...[ ... ]]) so
KEYWORDS.cpp is a clean superset without duplicate literals while keeping
KEYWORDS.hpp assigned to KEYWORDS.cpp.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/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/utils/toolRendering/index.tssrc/renderer/utils/toolRendering/toolContentChecks.ts
| WriteToolViewer, | ||
| } from './linkedTool'; | ||
|
|
||
| import type { StepVariant } from '@renderer/constants/stepVariants'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for StepVariant definition and exports
echo "=== Searching for StepVariant definitions ==="
rg -n "export.*StepVariant" --type=ts --type=tsx
echo -e "\n=== Searching for stepVariants module ==="
fd -e ts -e tsx stepVariants
echo -e "\n=== Checking if stepVariants directory exists ==="
fd -d stepVariantsRepository: matt1398/claude-devtools
Length of output: 380
🏁 Script executed:
#!/bin/bash
# Search for StepVariant type anywhere in the codebase
echo "=== Searching for any StepVariant references ==="
rg "StepVariant" --type=ts
echo -e "\n=== Checking LinkedToolItem.tsx file ==="
fd "LinkedToolItem.tsx" -x cat {}
echo -e "\n=== Searching for variant prop type in BaseItem ==="
rg -B5 -A10 "interface BaseItemProps|type BaseItemProps" --type=ts
echo -e "\n=== Checking if constants/stepVariants exists ==="
fd "stepVariants" --type=fRepository: matt1398/claude-devtools
Length of output: 9608
🏁 Script executed:
#!/bin/bash
# Get the full BaseItemProps interface
echo "=== Full BaseItemProps interface ==="
sed -n '/^interface BaseItemProps/,/^}/p' src/renderer/components/chat/items/BaseItem.tsx
echo -e "\n=== Search for variant in BaseItem ==="
rg "variant" src/renderer/components/chat/items/BaseItem.tsx -B2 -A2
echo -e "\n=== Check if constants directory exists ==="
ls -la src/renderer/constants/
echo -e "\n=== Search for StepVariant definition anywhere ==="
rg "type StepVariant|interface StepVariant" --type=ts
echo -e "\n=== Check what's in linkedTool directory ==="
ls -la src/renderer/components/chat/items/linkedTool/
</parameter>Repository: matt1398/claude-devtools
Length of output: 2571
Remove unused variant prop and non-existent import from LinkedToolItem.
BaseItem component does not accept a variant prop. The import on line 44 references a non-existent module @renderer/constants/stepVariants (which is not defined anywhere in the codebase), and the toolVariant variable passed to BaseItem on line 175 will cause a type error.
Delete the import statement on line 44 and remove variant={toolVariant} from the BaseItem component call.
🧰 Tools
🪛 GitHub Check: validate
[failure] 44-44:
Cannot find module '@renderer/constants/stepVariants' or its corresponding type declarations.
🤖 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 44, Remove the
unused/nonexistent StepVariant import and the erroneous variant prop usage:
delete the import "StepVariant" from LinkedToolItem.tsx and remove the prop
variant={toolVariant} from the BaseItem JSX call (BaseItem does not accept a
variant prop and toolVariant is not defined/typed), leaving BaseItem with only
its supported props.
| highlightClasses={highlightClasses} | ||
| highlightStyle={highlightStyle} | ||
| notificationDotColor={notificationDotColor} | ||
| variant={toolVariant} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find BaseItem component and its props definition
rg -n "BaseItemProps|interface BaseItemProps" --type=ts --type=tsx -A 10Repository: matt1398/claude-devtools
Length of output: 95
🏁 Script executed:
# Check the LinkedToolItem.tsx file context around line 175
fd -e tsx -e ts | xargs rg -l "LinkedToolItem" | head -5Repository: matt1398/claude-devtools
Length of output: 286
🏁 Script executed:
# Find BaseItem component file
fd -e tsx -e ts -name "*BaseItem*"Repository: matt1398/claude-devtools
Length of output: 298
🏁 Script executed:
# Search for BaseItemProps definition
rg "interface BaseItemProps|BaseItemProps" -t ts -A 10Repository: matt1398/claude-devtools
Length of output: 1679
🏁 Script executed:
# Find BaseItem component file more carefully
fd "BaseItem" -e tsx -e tsRepository: matt1398/claude-devtools
Length of output: 115
🏁 Script executed:
# Check LinkedToolItem.tsx directly
cat src/renderer/components/chat/items/LinkedToolItem.tsxRepository: matt1398/claude-devtools
Length of output: 7877
🏁 Script executed:
# Get the full BaseItem.tsx file
cat src/renderer/components/chat/items/BaseItem.tsxRepository: matt1398/claude-devtools
Length of output: 6054
Remove variant prop from BaseItem call or add it to BaseItemProps.
The variant={toolVariant} prop being passed at line 175 is not accepted by BaseItemProps. The BaseItem component does not include a variant property in its interface definition. Either remove this prop from the call or update BaseItemProps to include it if the variant styling is needed.
🧰 Tools
🪛 GitHub Check: validate
[failure] 175-175:
Type '{ children: (false | Element | undefined)[]; icon: Element; label: string; summary: string; tokenCount: number; status: ItemStatus; durationMs: number | undefined; ... 5 more ...; variant: StepVariant; }' is not assignable to type 'IntrinsicAttributes & 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 variant={toolVariant} to BaseItem but
BaseItemProps doesn't declare a variant prop; either remove the variant prop
from the BaseItem usage in LinkedToolItem (remove the variant={toolVariant}
attribute) or add a variant?: <type> entry to the BaseItemProps interface (and
handle it inside the BaseItem component rendering/styling) so BaseItem accepts
toolVariant; locate the call in LinkedToolItem.tsx (where BaseItem is rendered)
and adjust accordingly, or update the BaseItemProps/interface and the BaseItem
component to consume the new variant property.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/chat/viewers/syntaxHighlighter.ts (1)
924-942:⚠️ Potential issue | 🟠 MajorGuard
//comments by language to prevent false positives in shell/URL lines.The
//comment check at line ~913 currently applies to all languages without guards. This causes URLs likecurl https://example.comin bash to be incorrectly highlighted as comments, truncating output before the properly language-gated#and--checks can run. The issue is critical because the unguarded//check executes first and breaks out of the loop, preventing language-specific comment detection from working correctly.Recommended fix
+const SLASH_COMMENT_LANGUAGES = new Set([ + 'typescript', 'tsx', 'javascript', 'jsx', + 'java', 'kotlin', 'swift', 'c', 'cpp', 'hpp', 'go', 'rust', 'php' +]); + // Check for comment (// style) - if (remaining.startsWith('//')) { + if (SLASH_COMMENT_LANGUAGES.has(language) && remaining.startsWith('//')) { segments.push( React.createElement( 'span', { key: currentPos, style: { color: 'var(--syntax-comment)', fontStyle: 'italic' } }, remaining ) ); break; }🤖 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 - 942, The unguarded // comment branch is matching URLs in shell/URL lines; update the // comment check (the branch that tests remaining.startsWith('//') and pushes into segments) to only run for languages that actually use // comments (e.g., javascript/typescript, java, c, cpp, csharp, go, rust, swift, kotlin) by adding a language whitelist check (e.g., (language === 'javascript' || language === 'typescript' || ... ) && remaining.startsWith('//')), and ensure this guarded check remains after or does not preempt the existing '#' and '--' checks so shell (bash/zsh/fish/ruby/php/yaml) and sql/lua comment detection still runs first.
🧹 Nitpick comments (1)
src/renderer/components/chat/viewers/syntaxHighlighter.ts (1)
850-851: Optional cleanup: remove redundantbashfrom the no-keyword bypass list.Since
KEYWORDS.bashis now defined, this exception is effectively dead and may confuse future edits.♻️ Suggested simplification
- if (keywords.size === 0 && !['json', 'css', 'bash'].includes(language)) { + if (keywords.size === 0 && !['json', 'css'].includes(language)) { 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 850 - 851, The conditional in syntaxHighlighter.ts that bypasses highlighting when keywords.size === 0 currently includes a redundant 'bash' check; since KEYWORDS.bash is now defined, remove 'bash' from the array (i.e., change the array passed to includes so it no longer lists 'bash') and keep the rest of the logic intact; update any related comments and run unit/visual checks for the highlight function that uses the keywords and language variables to ensure bash now falls through to normal highlighting.
🤖 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/items/linkedTool/ReadToolViewer.tsx`:
- Around line 64-88: Add accessible state to the toggle buttons by adding an
aria-pressed attribute that reflects the current viewMode; update the Code
button (uses setViewMode and viewMode) to include aria-pressed={viewMode ===
'code'} and the Preview button to include aria-pressed={viewMode === 'preview'}
so screen readers can announce which mode is active.
- Line 58: The viewMode state in ReadToolViewer is initialized once and can
become out-of-sync when isMarkdownFile (or filePath) changes; add an effect in
the ReadToolViewer component that watches isMarkdownFile (and/or filePath) and
calls setViewMode(isMarkdownFile ? 'preview' : 'code') so the UI toggles to the
correct default when the detected file type changes, referencing the viewMode
and setViewMode state pair and the isMarkdownFile detection.
In `@src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx`:
- Line 24: The component WriteToolViewer initializes viewMode from
isMarkdownFile only once, so when isMarkdownFile (derived from filePath) changes
while the component is reused, viewMode can be stale; add an effect in
WriteToolViewer that watches isMarkdownFile and calls setViewMode(isMarkdownFile
? 'preview' : 'code') to reset the mode whenever isMarkdownFile changes (keeping
the existing state when unchanged).
In `@src/renderer/utils/toolRendering/toolContentChecks.ts`:
- Around line 63-65: The current hasBashContent(linkedTool: LinkedToolItem)
check is too permissive; update it to ensure linkedTool.input?.command is a
string and not empty/whitespace-only by verifying typeof
linkedTool.input.command === 'string' and linkedTool.input.command.trim().length
> 0 before returning true; keep the function name hasBashContent and the
LinkedToolItem references to locate and replace the existing boolean coercion.
---
Outside diff comments:
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 924-942: The unguarded // comment branch is matching URLs in
shell/URL lines; update the // comment check (the branch that tests
remaining.startsWith('//') and pushes into segments) to only run for languages
that actually use // comments (e.g., javascript/typescript, java, c, cpp,
csharp, go, rust, swift, kotlin) by adding a language whitelist check (e.g.,
(language === 'javascript' || language === 'typescript' || ... ) &&
remaining.startsWith('//')), and ensure this guarded check remains after or does
not preempt the existing '#' and '--' checks so shell
(bash/zsh/fish/ruby/php/yaml) and sql/lua comment detection still runs first.
---
Nitpick comments:
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 850-851: The conditional in syntaxHighlighter.ts that bypasses
highlighting when keywords.size === 0 currently includes a redundant 'bash'
check; since KEYWORDS.bash is now defined, remove 'bash' from the array (i.e.,
change the array passed to includes so it no longer lists 'bash') and keep the
rest of the logic intact; update any related comments and run unit/visual checks
for the highlight function that uses the keywords and language variables to
ensure bash now falls through to normal highlighting.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/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/utils/toolRendering/index.tssrc/renderer/utils/toolRendering/toolContentChecks.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/utils/toolRendering/index.ts
- src/renderer/components/chat/items/LinkedToolItem.tsx
- src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx
src/renderer/components/chat/items/linkedTool/ReadToolViewer.tsx
Outdated
Show resolved
Hide resolved
src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx
Outdated
Show resolved
Hide resolved
|
Hey @Psypeal, thank you for the contribution. The markdown preview is a valuable addition and clearly improves the readability of the output, good improvement there. However, the syntax-highlighted command for Bash tool inputs seems to introduce more noise than benefit. The Bash tool already expects a bash instruction as input, so wrapping it with a CodeBlockViewer makes the information redundant rather than clearer (as shown in the screenshot). IMO, a more practical improvement would be to add a copy button to both the Bash input and its output. That would make it easier to reuse commands in other tools or run them directly in a terminal, providing tangible usability gains without increasing visual complexity. 
|
that's good insight @cesarafonseca I am trying to find a better way to make the current bash command and output visualization better. I will improve it later. |
…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 (--)
Psypeal
force-pushed
the
feat/bash-viewer-collapsible-output
branch
from
4a830f0 to
a6035d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/components/chat/viewers/syntaxHighlighter.ts (1)
405-469: Bash keyword set includes shell utilities — verify this is intentional.The bash keyword set includes common shell utilities (
ls,cat,grep,sed,awk,find,git,docker,curl, etc.) alongside actual bash keywords. While practical for command highlighting, this may over-highlight common words that appear in other contexts (e.g., variable names, comments).If this is intentional for the Bash tool viewer use case, LGTM. Otherwise, consider separating true keywords from common commands.
🤖 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 405 - 469, The bash keyword Set (the "bash" entry created with new Set([...])) mixes core shell keywords with common utilities/commands which can over-highlight; split this into two named Sets (e.g., bashKeywords and bashCommands) and update the highlighting logic (where the "bash" set is used — search for the "bash" key and any highlightToken/highlightWord functions in syntaxHighlighter.ts) to only treat bashKeywords as language keywords by default and use bashCommands optionally (or only in command-context tokens) to avoid matching commands inside variables/comments; ensure callers that need the broader command list opt into using bashCommands.
🤖 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/viewers/syntaxHighlighter.ts`:
- Around line 727-787: The type-coloring logic in highlightLine erroneously
colors capitalized words in HTML content because 'html' is not included in the
early-return bypass; update the early-return condition (the bypass list checked
at the start of highlightLine) to include 'html' or add a language-specific
guard so when language === 'html' the type-coloring block (the capitalized-word
check `(word[0]?.toUpperCase() ?? '') === word[0] && word.length > 1`) is
skipped; target the highlightLine function and the early-return bypass array to
implement this change.
---
Nitpick comments:
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 405-469: The bash keyword Set (the "bash" entry created with new
Set([...])) mixes core shell keywords with common utilities/commands which can
over-highlight; split this into two named Sets (e.g., bashKeywords and
bashCommands) and update the highlighting logic (where the "bash" set is used —
search for the "bash" key and any highlightToken/highlightWord functions in
syntaxHighlighter.ts) to only treat bashKeywords as language keywords by default
and use bashCommands optionally (or only in command-context tokens) to avoid
matching commands inside variables/comments; ensure callers that need the
broader command list opt into using bashCommands.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/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/utils/toolRendering/index.tssrc/renderer/utils/toolRendering/toolContentChecks.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/renderer/components/chat/items/linkedTool/DefaultToolViewer.tsx
- src/renderer/components/chat/items/linkedTool/index.ts
- src/renderer/components/chat/items/linkedTool/WriteToolViewer.tsx
- src/renderer/components/chat/items/linkedTool/CollapsibleOutputSection.tsx
- src/renderer/components/chat/items/LinkedToolItem.tsx
| html: new Set([ | ||
| 'div', | ||
| 'span', | ||
| 'html', | ||
| 'head', | ||
| 'body', | ||
| 'title', | ||
| 'meta', | ||
| 'link', | ||
| 'script', | ||
| 'style', | ||
| 'section', | ||
| 'article', | ||
| 'header', | ||
| 'footer', | ||
| 'nav', | ||
| 'main', | ||
| 'aside', | ||
| 'form', | ||
| 'input', | ||
| 'button', | ||
| 'select', | ||
| 'option', | ||
| 'textarea', | ||
| 'label', | ||
| 'table', | ||
| 'thead', | ||
| 'tbody', | ||
| 'tr', | ||
| 'th', | ||
| 'td', | ||
| 'ul', | ||
| 'ol', | ||
| 'li', | ||
| 'a', | ||
| 'p', | ||
| 'h1', | ||
| 'h2', | ||
| 'h3', | ||
| 'h4', | ||
| 'h5', | ||
| 'h6', | ||
| 'img', | ||
| 'video', | ||
| 'audio', | ||
| 'canvas', | ||
| 'svg', | ||
| 'class', | ||
| 'id', | ||
| 'src', | ||
| 'href', | ||
| 'type', | ||
| 'name', | ||
| 'value', | ||
| 'placeholder', | ||
| 'alt', | ||
| 'width', | ||
| 'height', | ||
| 'true', | ||
| 'false', | ||
| ]), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the syntaxHighlighter.ts file
find . -name "syntaxHighlighter.ts" -type fRepository: matt1398/claude-devtools
Length of output: 127
🏁 Script executed:
# Read the relevant sections of the file to verify the claims
sed -n '725,790p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 877
🏁 Script executed:
# Check the early-return condition around line 850
sed -n '845,855p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 446
🏁 Script executed:
# Check the type-coloring logic around line 979
sed -n '975,985p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 482
🏁 Script executed:
# Search for where HTML language is used in the codebase
rg -n "language.*['\"]html['\"]" --type=ts -C2Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Search for calls to highlightLine function
rg -n "highlightLine" --type=ts -B2 -A2Repository: matt1398/claude-devtools
Length of output: 2570
🏁 Script executed:
# Search for any HTML language specification in the codebase
rg -n "html" --type=ts -i | grep -i "language\|lang" | head -20Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Check if there's any language mapping or detection
rg -n "getLanguage\|detectLanguage\|language.*=" --type=ts | head -20Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Read more context around the syntaxHighlighter file to understand the full picture
wc -l src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 130
🏁 Script executed:
# Check MarkdownViewer to see where 'lang' comes from
sed -n '140,180p' src/renderer/components/chat/viewers/MarkdownViewer.tsxRepository: matt1398/claude-devtools
Length of output: 1500
🏁 Script executed:
# Check CodeBlockViewer to see how detectedLanguage is determined
sed -n '1,50p' src/renderer/components/chat/viewers/CodeBlockViewer.tsxRepository: matt1398/claude-devtools
Length of output: 1561
🏁 Script executed:
# Get more context around detectedLanguage in CodeBlockViewer
sed -n '200,240p' src/renderer/components/chat/viewers/CodeBlockViewer.tsxRepository: matt1398/claude-devtools
Length of output: 1605
🏁 Script executed:
# Look for where detectedLanguage is set
rg -n "detectedLanguage" src/renderer/components/chat/viewers/CodeBlockViewer.tsx -B5 -A5Repository: matt1398/claude-devtools
Length of output: 1202
🏁 Script executed:
# Check if there's any git history or comments about HTML being added to or removed from the bypass list
git log --oneline -n 20 -- src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 154
🏁 Script executed:
# Let's examine the entire KEYWORDS object and the early-return logic together
sed -n '200,250p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 732
🏁 Script executed:
# Get the full early-return condition and surrounding context
sed -n '840,870p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 1076
🏁 Script executed:
# Check what languages have empty keyword sets
rg -n "new Set\(\[\]" src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Let's see the full KEYWORDS object structure to understand which languages are defined
sed -n '200,850p' src/renderer/components/chat/viewers/syntaxHighlighter.ts | grep -E "^\s+[a-z]+:\s+new Set|^\s+[a-z]+:\s+KEYWORDS"Repository: matt1398/claude-devtools
Length of output: 277
🏁 Script executed:
# Let's check the exact commit that added HTML keywords
git show a6035d4 --statRepository: matt1398/claude-devtools
Length of output: 28523
🏁 Script executed:
# Get more details about what changed in that commit regarding HTML
git show a6035d4 -- src/renderer/components/chat/viewers/syntaxHighlighter.ts | head -200Repository: matt1398/claude-devtools
Length of output: 3881
🏁 Script executed:
# Check if there's any documentation or issues mentioning the type-coloring problem
rg -n "type-coloring\|false.*keyword\|capitalize" --type=ts --type=mdRepository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Let's verify the exact HTML keyword set to see if capitalized words are included
sed -n '727,787p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 808
🏁 Script executed:
# Let me check if there's any special handling for HTML in the type-coloring logic
sed -n '970,1000p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 1023
🏁 Script executed:
# Check the complete context around type-coloring to see if HTML is handled specially
sed -n '960,1015p' src/renderer/components/chat/viewers/syntaxHighlighter.tsRepository: matt1398/claude-devtools
Length of output: 1553
HTML type-coloring will cause false positives on capitalized content.
The HTML keyword set (50+ tag and attribute names) won't prevent capitalized words in HTML content from being colored as types. When highlightLine is called with language='html', any word starting with uppercase like "My Company" or "Click Here" will match the type-coloring rule at line 979 ((word[0]?.toUpperCase() ?? '') === word[0] && word.length > 1) since HTML is not in the early-return bypass list (line 850).
The commit objective to "fix markdown/html false type-coloring on capitalized words" is only partially addressed—adding HTML keywords helps with recognized tag names, but doesn't resolve the broader issue for arbitrary capitalized content.
Consider adding 'html' to the early-return bypass condition at line 850 to prevent this, or implement language-specific logic to skip type-coloring for HTML.
🤖 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 727 -
787, The type-coloring logic in highlightLine erroneously colors capitalized
words in HTML content because 'html' is not included in the early-return bypass;
update the early-return condition (the bypass list checked at the start of
highlightLine) to include 'html' or add a language-specific guard so when
language === 'html' the type-coloring block (the capitalized-word check
`(word[0]?.toUpperCase() ?? '') === word[0] && word.length > 1`) is skipped;
target the highlightLine function and the early-return bypass array to implement
this change.
|
@Psypeal — appreciate the PR. I want to keep the CollapsibleOutputSection refactor and the markdown preview toggle — both solid. But the 437 lines of keyword sets for languages like C, Java, Swift, Lua etc. aren't something we need. CONTRIBUTING.md is pretty clear about not committing large static data blobs. Also agree with @cesarafonseca re: Bash highlighting being more noise than help. Can you split out just the collapsible output + markdown preview into its own PR? Happy to merge that. The rest should start as an Issue so we can scope it properly. Going to close this if there's no update soon. |
okay, matt. I will do it later. |
- Add reusable CollapsibleOutputSection component for tool outputs (collapsed by default, with status dot and copy support) - Refactor DefaultToolViewer to use CollapsibleOutputSection - Add Code/Preview toggle for .md/.mdx files in ReadToolViewer - Default to preview mode for markdown files in WriteToolViewer - Export CollapsibleOutputSection from linkedTool barrel Split from matt1398#90 per maintainer feedback — drops BashToolViewer and keyword expansions, keeps the collapsible output refactor and markdown preview improvements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Closing in favor of #112, which splits out just the CollapsibleOutputSection + markdown preview toggle as requested. |
3 participants
Summary
Test plan
pnpm typecheck— no type errorspnpm lint:fix— no lint errorspnpm test— all 654 tests passpnpm build && npx electron-builder --linux deb— builds successfullySummary by CodeRabbit
New Features
Improvements