-
-
Notifications
You must be signed in to change notification settings - Fork 430
fix(web): compact terminal tool cards by default #601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
f4f5ead
53aeab0
a0eea20
7953ef3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { shouldShowInlineToolCardBody, shouldUseCompactTerminalToolCard } from '@/components/ToolCard/ToolCard' | ||
|
|
||
| describe('ToolCard terminal display mode helpers', () => { | ||
| it('treats terminal-related cards as compact by default', () => { | ||
| expect(shouldUseCompactTerminalToolCard('CodexBash', 'compact')).toBe(true) | ||
| expect(shouldUseCompactTerminalToolCard('shell_command', 'compact')).toBe(true) | ||
| expect(shouldUseCompactTerminalToolCard('Read', 'compact')).toBe(false) | ||
| }) | ||
|
|
||
| it('hides inline terminal previews in compact mode', () => { | ||
| expect(shouldShowInlineToolCardBody('CodexBash', false, 'compact')).toBe(false) | ||
| }) | ||
|
|
||
| it('keeps inline terminal previews in detailed mode', () => { | ||
| expect(shouldShowInlineToolCardBody('CodexBash', false, 'detailed')).toBe(true) | ||
| }) | ||
|
|
||
| it('still hides inline bodies for minimal and Task/Agent subagent cards', () => { | ||
| expect(shouldShowInlineToolCardBody('CodexBash', true, 'detailed')).toBe(false) | ||
| expect(shouldShowInlineToolCardBody('Task', false, 'detailed')).toBe(false) | ||
| expect(shouldShowInlineToolCardBody('Agent', false, 'detailed')).toBe(false) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import { getToolPresentation } from '@/components/ToolCard/knownTools' | |
| import { getToolFullViewComponent, getToolViewComponent } from '@/components/ToolCard/views/_all' | ||
| import { getToolResultViewComponent } from '@/components/ToolCard/views/_results' | ||
| import { formatTaskChildLabel, TaskStateIcon } from '@/components/ToolCard/helpers' | ||
| import type { TerminalToolDisplayMode } from '@/hooks/useTerminalToolDisplayMode' | ||
| import { usePointerFocusRing } from '@/hooks/usePointerFocusRing' | ||
| import { getInputString, getInputStringAny, truncate } from '@/lib/toolInputUtils' | ||
| import { cn } from '@/lib/utils' | ||
|
|
@@ -25,6 +26,21 @@ import { TraceSection } from '@/components/ToolCard/trace' | |
| import { isSubagentToolName } from '@/chat/subagentTool' | ||
|
|
||
| const ELAPSED_INTERVAL_MS = 1000 | ||
| const TERMINAL_RELATED_TOOL_NAMES = new Set(['Bash', 'CodexBash', 'shell_command']) | ||
|
|
||
| export function shouldUseCompactTerminalToolCard(toolName: string, terminalToolDisplayMode: TerminalToolDisplayMode): boolean { | ||
| return TERMINAL_RELATED_TOOL_NAMES.has(toolName) && terminalToolDisplayMode === 'compact' | ||
| } | ||
|
|
||
| export function shouldShowInlineToolCardBody( | ||
| toolName: string, | ||
| presentationMinimal: boolean, | ||
| terminalToolDisplayMode: TerminalToolDisplayMode | ||
| ): boolean { | ||
| return !presentationMinimal | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MAJOR] Detailed mode still cannot show Bash/shell_command output previews. This helper still requires Suggested fix: export function shouldShowInlineToolCardBody(
toolName: string,
presentationMinimal: boolean,
terminalToolDisplayMode: TerminalToolDisplayMode
): boolean {
if (isSubagentToolName(toolName)) return false
if (TERMINAL_RELATED_TOOL_NAMES.has(toolName)) {
return terminalToolDisplayMode === 'detailed'
}
return !presentationMinimal
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Detailed mode now bypasses the Added helper coverage for:
Re-ran:
|
||
| && !isSubagentToolName(toolName) | ||
| && !shouldUseCompactTerminalToolCard(toolName, terminalToolDisplayMode) | ||
| } | ||
|
|
||
| function ElapsedView(props: { from: number; active: boolean }) { | ||
| const [now, setNow] = useState(() => Date.now()) | ||
|
|
@@ -260,6 +276,7 @@ type ToolCardProps = { | |
| api: ApiClient | ||
| sessionId: string | ||
| metadata: SessionMetadataSummary | null | ||
| terminalToolDisplayMode: TerminalToolDisplayMode | ||
| disabled: boolean | ||
| onDone: () => void | ||
| block: ToolCallBlock | ||
|
|
@@ -289,15 +306,16 @@ function ToolCardInner(props: ToolCardProps) { | |
| const subtitle = presentation.subtitle ?? props.block.tool.description | ||
| const taskSummary = renderTaskSummary(props.block, props.metadata, t) | ||
| const runningFrom = props.block.tool.startedAt ?? props.block.tool.createdAt | ||
| const showInline = !presentation.minimal && !isSubagentToolName(toolName) | ||
| const isCodexAgentCard = toolName === 'CodexAgent' | ||
| const useCompactTerminalCard = shouldUseCompactTerminalToolCard(toolName, props.terminalToolDisplayMode) | ||
| const showInline = shouldShowInlineToolCardBody(toolName, presentation.minimal, props.terminalToolDisplayMode) | ||
| const CompactToolView = showInline ? getToolViewComponent(toolName) : null | ||
| const FullToolView = getToolFullViewComponent(toolName) | ||
| const ResultToolView = getToolResultViewComponent(toolName) | ||
| const permission = props.block.tool.permission | ||
| const isAskUserQuestion = isAskUserQuestionToolName(toolName) | ||
| const isRequestUserInput = isRequestUserInputToolName(toolName) | ||
| const isQuestionTool = isAskUserQuestion || isRequestUserInput | ||
| const isCodexAgentCard = toolName === 'CodexAgent' | ||
| const showsPermissionFooter = Boolean(permission && ( | ||
| permission.status === 'pending' | ||
| || ((permission.status === 'denied' || permission.status === 'canceled') && Boolean(permission.reason)) | ||
|
|
@@ -324,7 +342,7 @@ function ToolCardInner(props: ToolCardProps) { | |
| {subtitle ? ( | ||
| <CardDescription className={cn( | ||
| 'font-mono text-xs text-[var(--app-tool-card-subtitle)]', | ||
| isCodexAgentCard ? 'truncate whitespace-nowrap' : 'break-all' | ||
| isCodexAgentCard || useCompactTerminalCard ? 'truncate whitespace-nowrap' : 'break-all' | ||
| )}> | ||
| {truncate(subtitle, 160)} | ||
| </CardDescription> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { beforeEach, describe, expect, it } from 'vitest' | ||
| import { | ||
| DEFAULT_TERMINAL_TOOL_DISPLAY_MODE, | ||
| getInitialTerminalToolDisplayMode, | ||
| getTerminalToolDisplayModeOptions, | ||
| } from './useTerminalToolDisplayMode' | ||
|
|
||
| describe('useTerminalToolDisplayMode helpers', () => { | ||
| beforeEach(() => { | ||
| window.localStorage.clear() | ||
| }) | ||
|
|
||
| it('returns the allowed terminal tool display options', () => { | ||
| expect(getTerminalToolDisplayModeOptions()).toEqual([ | ||
| { value: 'compact', labelKey: 'settings.chat.terminalToolDisplay.compact' }, | ||
| { value: 'detailed', labelKey: 'settings.chat.terminalToolDisplay.detailed' }, | ||
| ]) | ||
| }) | ||
|
|
||
| it('falls back to the default display mode for missing or invalid storage values', () => { | ||
| expect(getInitialTerminalToolDisplayMode()).toBe(DEFAULT_TERMINAL_TOOL_DISPLAY_MODE) | ||
|
|
||
| window.localStorage.setItem('hapi-terminal-tool-display-mode', 'invalid') | ||
| expect(getInitialTerminalToolDisplayMode()).toBe(DEFAULT_TERMINAL_TOOL_DISPLAY_MODE) | ||
| }) | ||
|
|
||
| it('reads a valid stored terminal tool display mode', () => { | ||
| window.localStorage.setItem('hapi-terminal-tool-display-mode', 'detailed') | ||
|
|
||
| expect(getInitialTerminalToolDisplayMode()).toBe('detailed') | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import { useCallback, useEffect, useState } from 'react' | ||
|
|
||
| export type TerminalToolDisplayMode = 'compact' | 'detailed' | ||
|
|
||
| export const DEFAULT_TERMINAL_TOOL_DISPLAY_MODE: TerminalToolDisplayMode = 'compact' | ||
|
|
||
| export function getTerminalToolDisplayModeOptions(): ReadonlyArray<{ value: TerminalToolDisplayMode; labelKey: string }> { | ||
| return [ | ||
| { value: 'compact', labelKey: 'settings.chat.terminalToolDisplay.compact' }, | ||
| { value: 'detailed', labelKey: 'settings.chat.terminalToolDisplay.detailed' }, | ||
| ] | ||
| } | ||
|
|
||
| function getTerminalToolDisplayModeStorageKey(): string { | ||
| return 'hapi-terminal-tool-display-mode' | ||
| } | ||
|
|
||
| function isBrowser(): boolean { | ||
| return typeof window !== 'undefined' && typeof document !== 'undefined' | ||
| } | ||
|
|
||
| function safeGetItem(key: string): string | null { | ||
| if (!isBrowser()) { | ||
| return null | ||
| } | ||
| try { | ||
| return localStorage.getItem(key) | ||
| } catch { | ||
| return null | ||
| } | ||
| } | ||
|
|
||
| function safeSetItem(key: string, value: string): void { | ||
| if (!isBrowser()) { | ||
| return | ||
| } | ||
| try { | ||
| localStorage.setItem(key, value) | ||
| } catch { | ||
| // Ignore storage errors | ||
| } | ||
| } | ||
|
|
||
| function safeRemoveItem(key: string): void { | ||
| if (!isBrowser()) { | ||
| return | ||
| } | ||
| try { | ||
| localStorage.removeItem(key) | ||
| } catch { | ||
| // Ignore storage errors | ||
| } | ||
| } | ||
|
|
||
| function parseTerminalToolDisplayMode(raw: string | null): TerminalToolDisplayMode { | ||
| if (raw === 'compact' || raw === 'detailed') { | ||
| return raw | ||
| } | ||
| return DEFAULT_TERMINAL_TOOL_DISPLAY_MODE | ||
| } | ||
|
|
||
| export function getInitialTerminalToolDisplayMode(): TerminalToolDisplayMode { | ||
| return parseTerminalToolDisplayMode(safeGetItem(getTerminalToolDisplayModeStorageKey())) | ||
| } | ||
|
|
||
| export function useTerminalToolDisplayMode(): { | ||
| terminalToolDisplayMode: TerminalToolDisplayMode | ||
| setTerminalToolDisplayMode: (mode: TerminalToolDisplayMode) => void | ||
| } { | ||
| const [terminalToolDisplayMode, setTerminalToolDisplayModeState] = useState<TerminalToolDisplayMode>(getInitialTerminalToolDisplayMode) | ||
|
|
||
| useEffect(() => { | ||
| if (!isBrowser()) { | ||
| return | ||
| } | ||
|
|
||
| const onStorage = (event: StorageEvent) => { | ||
| if (event.key !== getTerminalToolDisplayModeStorageKey()) { | ||
| return | ||
| } | ||
| setTerminalToolDisplayModeState(parseTerminalToolDisplayMode(event.newValue)) | ||
| } | ||
|
|
||
| window.addEventListener('storage', onStorage) | ||
| return () => window.removeEventListener('storage', onStorage) | ||
| }, []) | ||
|
|
||
| const setTerminalToolDisplayMode = useCallback((mode: TerminalToolDisplayMode) => { | ||
| setTerminalToolDisplayModeState(mode) | ||
|
|
||
| if (mode === DEFAULT_TERMINAL_TOOL_DISPLAY_MODE) { | ||
| safeRemoveItem(getTerminalToolDisplayModeStorageKey()) | ||
| } else { | ||
| safeSetItem(getTerminalToolDisplayModeStorageKey(), mode) | ||
| } | ||
| }, []) | ||
|
|
||
| return { terminalToolDisplayMode, setTerminalToolDisplayMode } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Major]
run_shell_commandis already exercised as a shell-like tool in the web tests (knownTools.test.tsxexpects it to surface acommandsubtitle), but it is not included here. That means Gemini ACP shell cards keep bypassing the new compact/detailed terminal-card setting, leaving a real terminal path with the old behavior.Suggested fix:
Add a matching regression expectation too:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
run_shell_commandis now treated as a terminal-related card too, so the compact default and detailed toggle cover the Gemini shell path instead of onlyBash/CodexBash/shell_command.Added regression coverage for:
shouldUseCompactTerminalToolCard('run_shell_command', 'compact')shouldShowInlineToolCardBody('run_shell_command', true, 'detailed')Re-ran:
bun run test -- src/components/ToolCard/ToolCard.test.ts src/routes/settings/index.test.tsx src/hooks/useTerminalToolDisplayMode.test.tsbun run typecheck