diff --git a/docs/plans/fix-tool-strip-showtools-toggle.md b/docs/plans/fix-tool-strip-showtools-toggle.md new file mode 100644 index 00000000..e11522a2 --- /dev/null +++ b/docs/plans/fix-tool-strip-showtools-toggle.md @@ -0,0 +1,189 @@ +# Fix Tool Strip showTools Toggle Behavior + +## Overview + +This plan addresses the tool strip toggle behavior to make it session-only (not persisted to localStorage) and controlled by the `showTools` prop as the default state. + +### Requirements + +1. `showTools` is the default state at render +2. `showTools=false`: strip collapsed, all tools collapsed +3. `showTools=true`: strip expanded, all tools expanded +4. Strip chevron toggles strip only (show/hide individual tools list) +5. Tool chevron toggles that specific tool only +6. All toggles are session-only (lost on refresh) +7. On reload: reset to `showTools` default + +## Files to Modify + +### 1. `src/components/agent-chat/ToolStrip.tsx` + +**Changes:** +- Remove `useSyncExternalStore` and related imports from `browser-preferences` +- Remove localStorage-based persistence +- Replace `expandedPref` with local `useState` initialized to `showTools` +- Pass `initialExpanded={showTools}` to each `ToolBlock` instead of `initialExpanded={shouldAutoExpand}` +- Remove the `autoExpandAbove` and `completedToolOffset` props (no longer needed) + +**Before:** +```tsx +import { memo, useMemo, useSyncExternalStore } from 'react' +import { + getToolStripExpandedPreference, + setToolStripExpandedPreference, + subscribeToolStripPreference, +} from '@/lib/browser-preferences' + +// ... +const expandedPref = useSyncExternalStore( + subscribeToolStripPreference, + getToolStripExpandedPreference, + () => false, +) +const expanded = showTools && expandedPref + +const handleToggle = () => { + setToolStripExpandedPreference(!expandedPref) +} +``` + +**After:** +```tsx +import { memo, useMemo, useState } from 'react' + +// ... +const [stripExpanded, setStripExpanded] = useState(showTools) + +const handleToggle = () => { + setStripExpanded(!stripExpanded) +} + +// In ToolBlock rendering: + +``` + +### 2. `src/lib/browser-preferences.ts` + +**Changes:** +- Remove `toolStrip` from `BrowserPreferencesRecord` type +- Remove `toolStrip` handling in `normalizeRecord()` +- Remove `toolStrip` handling in `patchBrowserPreferencesRecord()` +- Remove `toolStrip` handling in `migrateLegacyKeys()` +- Remove `getToolStripExpandedPreference()` function +- Remove `setToolStripExpandedPreference()` function +- Remove `subscribeToolStripPreference()` function +- Remove `LEGACY_TOOL_STRIP_STORAGE_KEY` constant + +**Removed exports:** +- `getToolStripExpandedPreference` +- `setToolStripExpandedPreference` +- `subscribeToolStripPreference` + +### 3. `src/components/agent-chat/MessageBubble.tsx` + +**Changes:** +- Remove `completedToolOffset` and `autoExpandAbove` props from the interface +- Remove the `toolGroupOffsets` useMemo (no longer needed) +- Remove `completedToolOffset` and `autoExpandAbove` from ToolStrip props + +**Before:** +```tsx +interface MessageBubbleProps { + // ... + completedToolOffset?: number + autoExpandAbove?: number +} + +// ... + +``` + +**After:** +```tsx +interface MessageBubbleProps { + // ... + // Remove completedToolOffset and autoExpandAbove +} + +// ... + +``` + +### 4. `src/components/agent-chat/ToolBlock.tsx` + +**No changes required.** The component already supports `initialExpanded` prop which controls the initial expanded state. + +## Test Updates + +### `test/unit/client/components/agent-chat/ToolStrip.test.tsx` + +**Remove tests:** +- `'expands on chevron click and persists to browser preferences'` - no longer persists +- `'starts expanded when browser preferences have a stored preference'` - no longer reads from localStorage +- `'collapses on second chevron click and stores false in browser preferences'` - no longer persists +- `'passes autoExpandAbove props through to ToolBlocks in expanded mode'` - autoExpandAbove removed +- `'migrates the legacy tool-strip key through the browser preferences helper'` - legacy migration removed + +**Modify tests:** +- `'always shows collapsed view when showTools is false, even if localStorage says expanded'` - simplify to just `'always shows collapsed view when showTools is false'` + +**Add new tests:** +- `'starts expanded when showTools is true'` +- `'starts collapsed when showTools is false'` +- `'strip toggle is session-only (not persisted to localStorage)'` +- `'ToolBlocks start expanded when showTools is true'` +- `'ToolBlocks start collapsed when showTools is false'` +- `'individual ToolBlock toggles work independently'` + +### `test/unit/client/components/agent-chat/MessageBubble.test.tsx` + +**Modify tests:** +- Remove `completedToolOffset` and `autoExpandAbove` from any test setup if present +- Update tests that verify localStorage interaction to verify session-only behavior instead + +### `test/unit/lib/browser-preferences.test.ts` (if exists) + +**Remove tests:** +- Any tests for `getToolStripExpandedPreference`, `setToolStripExpandedPreference`, `subscribeToolStripPreference` +- Any tests for `toolStrip` field handling + +## Implementation Steps + +1. **browser-preferences.ts**: Remove tool strip persistence functions and types +2. **ToolStrip.tsx**: Replace localStorage with local state, pass `showTools` to ToolBlocks +3. **MessageBubble.tsx**: Remove unused props +4. **Update tests**: Remove localStorage-related tests, add session-only behavior tests +5. **Run full test suite**: `npm test` +6. **Manual verification**: Test in browser + +## Commit Message + +``` +fix: make tool strip toggle session-only, controlled by showTools prop + +- Remove localStorage persistence for tool strip expanded state +- ToolStrip now uses local useState initialized from showTools prop +- ToolBlocks inherit initial expanded state from showTools +- Remove autoExpandAbove/completedToolOffset props (no longer needed) +- All toggle state is session-only, resets on page refresh +``` \ No newline at end of file diff --git a/src/components/agent-chat/AgentChatView.tsx b/src/components/agent-chat/AgentChatView.tsx index 0dde75d6..75ea77b7 100644 --- a/src/components/agent-chat/AgentChatView.tsx +++ b/src/components/agent-chat/AgentChatView.tsx @@ -451,28 +451,7 @@ export default function AgentChatView({ tabId, paneId, paneContent, hidden }: Ag const timelineItems = useMemo(() => session?.timelineItems ?? [], [session?.timelineItems]) const timelineBodies = session?.timelineBodies ?? {} - // Auto-expand: count completed tools across all messages, expand the most recent N - const RECENT_TOOLS_EXPANDED = 3 const messages = useMemo(() => session?.messages ?? [], [session?.messages]) - const { completedToolOffsets, autoExpandAbove } = useMemo(() => { - let totalCompletedTools = 0 - const offsets: number[] = [] - for (const msg of messages) { - offsets.push(totalCompletedTools) - for (const b of msg.content) { - if (b.type === 'tool_use' && b.id) { - const hasResult = msg.content.some( - r => r.type === 'tool_result' && r.tool_use_id === b.id - ) - if (hasResult) totalCompletedTools++ - } - } - } - return { - completedToolOffsets: offsets, - autoExpandAbove: Math.max(0, totalCompletedTools - RECENT_TOOLS_EXPANDED), - } - }, [messages]) // Debounce streaming text to limit markdown re-parsing to ~20x/sec const debouncedStreamingText = useStreamDebounce( @@ -661,8 +640,6 @@ export default function AgentChatView({ tabId, paneId, paneContent, hidden }: Ag showThinking={paneContent.showThinking ?? defaultShowThinking} showTools={paneContent.showTools ?? defaultShowTools} showTimecodes={paneContent.showTimecodes ?? defaultShowTimecodes} - completedToolOffset={completedToolOffsets[item.msgIndices[1]]} - autoExpandAbove={autoExpandAbove} /> ) @@ -679,8 +656,6 @@ export default function AgentChatView({ tabId, paneId, paneContent, hidden }: Ag showThinking={paneContent.showThinking ?? defaultShowThinking} showTools={paneContent.showTools ?? defaultShowTools} showTimecodes={paneContent.showTimecodes ?? defaultShowTimecodes} - completedToolOffset={completedToolOffsets[item.msgIndex]} - autoExpandAbove={autoExpandAbove} /> ) }) diff --git a/src/components/agent-chat/MessageBubble.tsx b/src/components/agent-chat/MessageBubble.tsx index 7d688a9c..39a4e9c6 100644 --- a/src/components/agent-chat/MessageBubble.tsx +++ b/src/components/agent-chat/MessageBubble.tsx @@ -4,7 +4,6 @@ import type { ChatContentBlock } from '@/store/agentChatTypes' import { LazyMarkdown } from '@/components/markdown/LazyMarkdown' import ToolStrip, { type ToolPair } from './ToolStrip' -/** Strip SDK-injected ... tags from text. */ function stripSystemReminders(text: string): string { return text.replace(/[\s\S]*?<\/system-reminder>/g, '').trim() } @@ -23,14 +22,7 @@ interface MessageBubbleProps { showThinking?: boolean showTools?: boolean showTimecodes?: boolean - /** When true, unpaired tool_use blocks show a spinner (they may still be running). - * When false (default), unpaired tool_use blocks show as complete — their results - * arrived in a later message. */ isLastMessage?: boolean - /** Index offset for this message's completed tool blocks in the global sequence. */ - completedToolOffset?: number - /** Completed tools at globalIndex >= this value get initialExpanded=true. */ - autoExpandAbove?: number } function MessageBubble({ @@ -43,11 +35,8 @@ function MessageBubble({ showTools = true, showTimecodes = false, isLastMessage = false, - completedToolOffset, - autoExpandAbove, }: MessageBubbleProps) { const resolvedSpeaker = speaker ?? role ?? 'assistant' - // Build a map of tool_use_id -> tool_result for pairing const resultMap = useMemo(() => { const map = new Map() for (const block of content) { @@ -58,7 +47,6 @@ function MessageBubble({ return map }, [content]) - // Group content blocks into render groups: text, thinking, or contiguous tool runs. const groups = useMemo(() => { const result: RenderGroup[] = [] let currentToolPairs: ToolPair[] | null = null @@ -80,7 +68,6 @@ function MessageBubble({ currentToolPairs = [] toolStartIndex = i } - // Look up the matching tool_result const resultBlock = block.id ? resultMap.get(block.id) : undefined const rawResult = resultBlock ? (typeof resultBlock.content === 'string' ? resultBlock.content : JSON.stringify(resultBlock.content)) @@ -99,15 +86,12 @@ function MessageBubble({ } if (block.type === 'tool_result') { - // If we're in a tool group, skip (already consumed via resultMap pairing above). if (currentToolPairs) continue - // If it has a matching tool_use elsewhere in this message, skip (already consumed) if (block.tool_use_id && content.some(b => b.type === 'tool_use' && b.id === block.tool_use_id)) { continue } - // Orphaned result: render as standalone tool strip const raw = typeof block.content === 'string' ? block.content : block.content != null ? JSON.stringify(block.content) : '' @@ -127,7 +111,6 @@ function MessageBubble({ continue } - // Non-tool block: flush any pending tool group flushTools() if (block.type === 'text' && block.text) { @@ -137,16 +120,11 @@ function MessageBubble({ } } - // Flush any trailing tool group flushTools() return result }, [content, resultMap, isLastMessage]) - // Check if any blocks will be visible after applying toggle filters. - // Note: tool groups are unconditionally visible (collapsed summary always shows), - // so showTools is intentionally absent from the dependency array. Only thinking - // blocks are conditionally hidden via their toggle. const hasVisibleContent = useMemo(() => { return groups.some((group) => { if (group.kind === 'text') return true @@ -156,19 +134,6 @@ function MessageBubble({ }) }, [groups, showThinking]) - // Track completed tool offset across tool groups for auto-expand - const toolGroupOffsets = useMemo(() => { - const offsets: number[] = [] - let offset = completedToolOffset ?? 0 - for (const group of groups) { - if (group.kind === 'tools') { - offsets.push(offset) - offset += group.pairs.filter(p => p.status === 'complete').length - } - } - return offsets - }, [groups, completedToolOffset]) - if (!hasVisibleContent) return null return ( @@ -219,8 +184,6 @@ function MessageBubble({ key={`tools-${group.startIndex}`} pairs={group.pairs} isStreaming={isStreaming} - completedToolOffset={toolGroupOffsets[group.toolGroupIndex]} - autoExpandAbove={autoExpandAbove} showTools={showTools} /> ) diff --git a/src/components/agent-chat/ToolStrip.tsx b/src/components/agent-chat/ToolStrip.tsx index 84306568..822da7c5 100644 --- a/src/components/agent-chat/ToolStrip.tsx +++ b/src/components/agent-chat/ToolStrip.tsx @@ -1,10 +1,5 @@ -import { memo, useMemo, useSyncExternalStore } from 'react' +import { memo, useEffect, useMemo, useState } from 'react' import { ChevronRight } from 'lucide-react' -import { - getToolStripExpandedPreference, - setToolStripExpandedPreference, - subscribeToolStripPreference, -} from '@/lib/browser-preferences' import { cn } from '@/lib/utils' import { getToolPreview } from './tool-preview' import ToolBlock from './ToolBlock' @@ -22,33 +17,23 @@ export interface ToolPair { interface ToolStripProps { pairs: ToolPair[] isStreaming: boolean - /** Index offset for this strip's completed tool blocks in the global sequence. */ - completedToolOffset?: number - /** Completed tools at globalIndex >= this value get initialExpanded=true. */ - autoExpandAbove?: number /** When false, strip is locked to collapsed view (no expand chevron). Default true. */ showTools?: boolean } -function ToolStrip({ pairs, isStreaming, completedToolOffset, autoExpandAbove, showTools = true }: ToolStripProps) { - const expandedPref = useSyncExternalStore( - subscribeToolStripPreference, - getToolStripExpandedPreference, - () => false, - ) - const expanded = showTools && expandedPref +function ToolStrip({ pairs, isStreaming, showTools = true }: ToolStripProps) { + const [stripExpanded, setStripExpanded] = useState(showTools) + useEffect(() => { setStripExpanded(showTools) }, [showTools]) const handleToggle = () => { - setToolStripExpandedPreference(!expandedPref) + setStripExpanded(!stripExpanded) } const hasErrors = pairs.some(p => p.isError) const allComplete = pairs.every(p => p.status === 'complete') const isSettled = allComplete && !isStreaming - // Determine the current (latest active or last completed) tool for the reel const currentTool = useMemo(() => { - // Find the last running tool, or fall back to the last tool for (let i = pairs.length - 1; i >= 0; i--) { if (pairs[i].status === 'running') return pairs[i] } @@ -58,19 +43,13 @@ function ToolStrip({ pairs, isStreaming, completedToolOffset, autoExpandAbove, s const toolCount = pairs.length const settledText = `${toolCount} tool${toolCount !== 1 ? 's' : ''} used` - // NOTE: ToolStrip is a borderless wrapper. In collapsed mode, the collapsed - // row gets its own tool-colored left border (since no ToolBlock is visible). - // In expanded mode, ToolBlocks render their own border-l-2 exactly as today, - // producing two border levels (MessageBubble > ToolBlock) -- not three. - return (
- {/* Collapsed view: single-line reel with tool-colored border + chevron */} - {!expanded && ( + {!stripExpanded && (
- {showTools && ( - - )} + )} - {/* Expanded view: toggle button + ToolBlock list (looks like today). - No header text -- the user specified expanded mode shows "a list of - tools run so far, with an expando to see each one", matching today. - ToolBlocks provide their own border-l-2, so no border on the wrapper. */} - {expanded && ( + {stripExpanded && ( <> - {pairs.map((pair, i) => { - const globalIndex = (completedToolOffset ?? 0) + i - const shouldAutoExpand = autoExpandAbove != null - ? globalIndex >= autoExpandAbove && pair.status === 'complete' - : false - return ( - - ) - })} + {pairs.map((pair) => ( + + ))} )}
diff --git a/src/lib/browser-preferences.ts b/src/lib/browser-preferences.ts index 5ee28949..ddb784df 100644 --- a/src/lib/browser-preferences.ts +++ b/src/lib/browser-preferences.ts @@ -10,12 +10,10 @@ import { BROWSER_PREFERENCES_STORAGE_KEY as STORAGE_KEY } from '@/store/storage- export const BROWSER_PREFERENCES_STORAGE_KEY = STORAGE_KEY const LEGACY_TERMINAL_FONT_KEY = 'freshell.terminal.fontFamily.v1' -const LEGACY_TOOL_STRIP_STORAGE_KEY = ['freshell', 'toolStripExpanded'].join(':') const DEFAULT_SEARCH_RANGE_DAYS = 30 export type BrowserPreferencesRecord = { settings?: LocalSettingsPatch - toolStrip?: { expanded?: boolean } tabs?: { searchRangeDays?: number } legacyLocalSettingsSeedApplied?: boolean } @@ -47,10 +45,6 @@ function normalizeRecord(value: unknown): BrowserPreferencesRecord { normalized.legacyLocalSettingsSeedApplied = true } - if (isRecord(value.toolStrip) && typeof value.toolStrip.expanded === 'boolean') { - normalized.toolStrip = { expanded: value.toolStrip.expanded } - } - if ( isRecord(value.tabs) && typeof value.tabs.searchRangeDays === 'number' @@ -105,18 +99,6 @@ function migrateLegacyKeys(record: BrowserPreferencesRecord): BrowserPreferences needsPersist = true } } - - const legacyToolStrip = window.localStorage.getItem(LEGACY_TOOL_STRIP_STORAGE_KEY) - if (legacyToolStrip === 'true' || legacyToolStrip === 'false') { - sawLegacyKeys = true - if (next.toolStrip?.expanded === undefined) { - next = { - ...next, - toolStrip: { expanded: legacyToolStrip === 'true' }, - } - needsPersist = true - } - } } catch { return record } @@ -128,7 +110,6 @@ function migrateLegacyKeys(record: BrowserPreferencesRecord): BrowserPreferences if (sawLegacyKeys) { try { window.localStorage.removeItem(LEGACY_TERMINAL_FONT_KEY) - window.localStorage.removeItem(LEGACY_TOOL_STRIP_STORAGE_KEY) } catch { // Ignore cleanup failures and keep the migrated in-memory value. } @@ -175,16 +156,6 @@ export function patchBrowserPreferencesRecord(patch: BrowserPreferencesRecord): } } - if (isRecord(patch.toolStrip) && typeof patch.toolStrip.expanded === 'boolean') { - next = { - ...next, - toolStrip: { - ...(current.toolStrip || {}), - expanded: patch.toolStrip.expanded, - }, - } - } - if ( isRecord(patch.tabs) && typeof patch.tabs.searchRangeDays === 'number' @@ -239,42 +210,6 @@ export function resolveBrowserPreferenceSettings(record?: BrowserPreferencesReco return resolveLocalSettings(record?.settings) } -export function getToolStripExpandedPreference(): boolean { - return loadBrowserPreferencesRecord().toolStrip?.expanded ?? false -} - -export function setToolStripExpandedPreference(expanded: boolean): void { - patchBrowserPreferencesRecord({ - toolStrip: { expanded }, - }) - - if (!canUseStorage()) { - return - } - - try { - window.dispatchEvent(new StorageEvent('storage', { key: BROWSER_PREFERENCES_STORAGE_KEY })) - } catch { - window.dispatchEvent(new Event('storage')) - } -} - export function getSearchRangeDaysPreference(): number { return loadBrowserPreferencesRecord().tabs?.searchRangeDays ?? DEFAULT_SEARCH_RANGE_DAYS } - -export function subscribeToolStripPreference(listener: () => void): () => void { - if (typeof window === 'undefined') { - return () => {} - } - - const handler = (event: Event) => { - if (event instanceof StorageEvent && event.key && event.key !== BROWSER_PREFERENCES_STORAGE_KEY) { - return - } - listener() - } - - window.addEventListener('storage', handler) - return () => window.removeEventListener('storage', handler) -} diff --git a/src/store/browserPreferencesPersistence.ts b/src/store/browserPreferencesPersistence.ts index 547efc0a..12a051af 100644 --- a/src/store/browserPreferencesPersistence.ts +++ b/src/store/browserPreferencesPersistence.ts @@ -143,10 +143,6 @@ function buildBrowserPreferencesRecord(state: BrowserPreferencesState): BrowserP next.legacyLocalSettingsSeedApplied = true } - if (current.toolStrip?.expanded !== undefined) { - next.toolStrip = { expanded: current.toolStrip.expanded } - } - const settingsPatch = buildLocalSettingsPatch(state.settings.localSettings) if (Object.keys(settingsPatch).length > 0) { next.settings = settingsPatch diff --git a/src/store/storage-migration.ts b/src/store/storage-migration.ts index bb1e7b70..2e75cb51 100644 --- a/src/store/storage-migration.ts +++ b/src/store/storage-migration.ts @@ -23,7 +23,6 @@ const STORAGE_VERSION_KEY = 'freshell_version' const AUTH_STORAGE_KEY = 'freshell.auth-token' const LEGACY_BROWSER_PREFERENCE_KEYS = [ 'freshell.terminal.fontFamily.v1', - 'freshell:toolStripExpanded', ] as const function readStorageVersion(): number { diff --git a/test/e2e/agent-chat-context-menu-flow.test.tsx b/test/e2e/agent-chat-context-menu-flow.test.tsx index 4f9d467c..db82416a 100644 --- a/test/e2e/agent-chat-context-menu-flow.test.tsx +++ b/test/e2e/agent-chat-context-menu-flow.test.tsx @@ -22,12 +22,8 @@ import settingsReducer from '@/store/settingsSlice' import type { AgentChatPaneContent } from '@/store/paneTypes' import { buildMenuItems, type MenuActions, type MenuBuildContext } from '@/components/context-menu/menu-defs' import type { ContextTarget } from '@/components/context-menu/context-menu-types' -import { - BROWSER_PREFERENCES_STORAGE_KEY, - setToolStripExpandedPreference, -} from '@/lib/browser-preferences' +import { BROWSER_PREFERENCES_STORAGE_KEY } from '@/store/storage-keys' -// jsdom doesn't implement scrollIntoView beforeAll(() => { Element.prototype.scrollIntoView = vi.fn() }) @@ -132,8 +128,6 @@ describe('freshclaude context menu integration', () => { }) it('right-click on tool input in rendered DOM produces "Copy command" menu item', () => { - // Tool strips are collapsed by default; expand to access ToolBlock data attributes - setToolStripExpandedPreference(true) const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) store.dispatch(addUserMessage({ sessionId: 'sess-1', text: 'Run a command' })) @@ -153,18 +147,11 @@ describe('freshclaude context menu integration', () => { , ) - // Ensure ToolBlock is expanded so data attributes are in the DOM - const toolButton = screen.getByRole('button', { name: /tool call/i }) - if (toolButton.getAttribute('aria-expanded') !== 'true') { - fireEvent.click(toolButton) - } - - // Step 1: Verify the data attributes are present in the rendered DOM + // Tool strips start expanded when showTools=true (default), so ToolBlock data attributes are in the DOM const toolInputEl = container.querySelector('[data-tool-input]') expect(toolInputEl).not.toBeNull() expect(toolInputEl?.getAttribute('data-tool-name')).toBe('Bash') - // Step 2: Feed the actual DOM element into buildMenuItems as clickTarget const mockActions = createMockActions() const ctx = createMockContext(mockActions, { clickTarget: toolInputEl as HTMLElement, @@ -173,7 +160,6 @@ describe('freshclaude context menu integration', () => { const items = buildMenuItems(target, ctx) const ids = items.filter(i => i.type === 'item').map(i => i.id) - // Step 3: Verify the correct context-sensitive menu items appear expect(ids).toContain('fc-copy') expect(ids).toContain('fc-select-all') expect(ids).toContain('fc-copy-command') @@ -181,8 +167,6 @@ describe('freshclaude context menu integration', () => { }) it('right-click on diff in rendered DOM produces diff-specific menu items', () => { - // Tool strips are collapsed by default; expand to access ToolBlock data attributes - setToolStripExpandedPreference(true) const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) store.dispatch(addUserMessage({ sessionId: 'sess-1', text: 'Edit a file' })) @@ -214,22 +198,14 @@ describe('freshclaude context menu integration', () => { , ) - // Ensure ToolBlock is expanded so data attributes are in the DOM - const toolButton = screen.getByRole('button', { name: /tool call/i }) - if (toolButton.getAttribute('aria-expanded') !== 'true') { - fireEvent.click(toolButton) - } - - // Step 1: Verify the data attributes are present in the rendered DOM + // Tool strips start expanded when showTools=true (default) const diffEl = container.querySelector('[data-diff]') expect(diffEl).not.toBeNull() expect(diffEl?.getAttribute('data-file-path')).toBe('/tmp/test.ts') - // The click target would be a child element inside the diff (e.g. a span with diff text) const clickTarget = diffEl?.querySelector('span') ?? diffEl expect(clickTarget).not.toBeNull() - // Step 2: Feed the actual DOM element into buildMenuItems as clickTarget const mockActions = createMockActions() const ctx = createMockContext(mockActions, { clickTarget: clickTarget as HTMLElement, @@ -238,7 +214,6 @@ describe('freshclaude context menu integration', () => { const items = buildMenuItems(target, ctx) const ids = items.filter(i => i.type === 'item').map(i => i.id) - // Step 3: Verify the correct context-sensitive menu items appear expect(ids).toContain('fc-copy') expect(ids).toContain('fc-select-all') expect(ids).toContain('fc-copy-new-version') @@ -248,8 +223,6 @@ describe('freshclaude context menu integration', () => { }) it('right-click on tool output in rendered DOM produces "Copy output" menu item', () => { - // Tool strips are collapsed by default; expand to access ToolBlock data attributes - setToolStripExpandedPreference(true) const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) store.dispatch(addUserMessage({ sessionId: 'sess-1', text: 'List files' })) @@ -268,17 +241,10 @@ describe('freshclaude context menu integration', () => { , ) - // Ensure ToolBlock is expanded so data attributes are in the DOM - const toolButton = screen.getByRole('button', { name: /tool call/i }) - if (toolButton.getAttribute('aria-expanded') !== 'true') { - fireEvent.click(toolButton) - } - - // Verify the tool output data attribute exists in the DOM + // Tool strips start expanded when showTools=true (default) const toolOutputEl = container.querySelector('[data-tool-output]') expect(toolOutputEl).not.toBeNull() - // Feed it into buildMenuItems const mockActions = createMockActions() const ctx = createMockContext(mockActions, { clickTarget: toolOutputEl as HTMLElement, diff --git a/test/e2e/agent-chat-polish-flow.test.tsx b/test/e2e/agent-chat-polish-flow.test.tsx index 67a71b93..0fc4d962 100644 --- a/test/e2e/agent-chat-polish-flow.test.tsx +++ b/test/e2e/agent-chat-polish-flow.test.tsx @@ -21,18 +21,12 @@ import panesReducer from '@/store/panesSlice' import settingsReducer from '@/store/settingsSlice' import type { AgentChatPaneContent } from '@/store/paneTypes' import type { ChatContentBlock } from '@/store/agentChatTypes' -import { - BROWSER_PREFERENCES_STORAGE_KEY, - setToolStripExpandedPreference, -} from '@/lib/browser-preferences' +import { BROWSER_PREFERENCES_STORAGE_KEY } from '@/store/storage-keys' -// jsdom doesn't implement scrollIntoView beforeAll(() => { Element.prototype.scrollIntoView = vi.fn() }) -// Render MarkdownRenderer synchronously to avoid React.lazy timing issues -// when running in the full test suite (dynamic import may not resolve in time) vi.mock('@/components/markdown/LazyMarkdown', async () => { const { MarkdownRenderer } = await import('@/components/markdown/MarkdownRenderer') return { @@ -88,17 +82,14 @@ describe('freshclaude polish e2e: left-border message layout', () => { const messages = screen.getAllByRole('article') expect(messages).toHaveLength(2) - // User message labeled correctly const userMsg = screen.getByLabelText('user message') expect(userMsg).toBeInTheDocument() expect(userMsg.className).toContain('border-l-') - // Assistant message labeled correctly const assistantMsg = screen.getByLabelText('assistant message') expect(assistantMsg).toBeInTheDocument() expect(assistantMsg.className).toContain('border-l-') - // Different border widths distinguish them: user=3px, assistant=2px expect(userMsg.className).toContain('border-l-[3px]') expect(assistantMsg.className).toContain('border-l-2') }) @@ -160,8 +151,6 @@ describe('freshclaude polish e2e: tool block expand/collapse', () => { }) it('collapses and expands tool blocks on click', () => { - // Tool strips are collapsed by default; set expanded to test ToolBlock interaction - setToolStripExpandedPreference(true) const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) store.dispatch(addUserMessage({ sessionId: 'sess-1', text: 'Run a command' })) @@ -185,7 +174,7 @@ describe('freshclaude polish e2e: tool block expand/collapse', () => { const toolButton = screen.getByRole('button', { name: /tool call/i }) expect(toolButton).toBeInTheDocument() - // With only 1 tool (< RECENT_TOOLS_EXPANDED=3), it should start expanded + // With showTools=true (default), ToolBlocks start expanded expect(toolButton).toHaveAttribute('aria-expanded', 'true') // Click to collapse @@ -198,15 +187,13 @@ describe('freshclaude polish e2e: tool block expand/collapse', () => { }) }) -describe('freshclaude polish e2e: auto-collapse old tools', () => { +describe('freshclaude polish e2e: all tools expanded when showTools=true', () => { afterEach(() => { cleanup() localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) }) - it('old tools start collapsed while recent tools start expanded', () => { - // Tool strips are collapsed by default; set expanded to test auto-expand behavior - setToolStripExpandedPreference(true) + it('all tools start expanded when showTools=true', () => { const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) store.dispatch(addUserMessage({ sessionId: 'sess-1', text: 'Do things' })) @@ -230,9 +217,9 @@ describe('freshclaude polish e2e: auto-collapse old tools', () => { const toolButtons = screen.getAllByRole('button', { name: /tool call/i }) expect(toolButtons).toHaveLength(5) - // RECENT_TOOLS_EXPANDED=3: first 2 collapsed, last 3 expanded - expect(toolButtons[0]).toHaveAttribute('aria-expanded', 'false') - expect(toolButtons[1]).toHaveAttribute('aria-expanded', 'false') + // All tools should start expanded when showTools=true (default) + expect(toolButtons[0]).toHaveAttribute('aria-expanded', 'true') + expect(toolButtons[1]).toHaveAttribute('aria-expanded', 'true') expect(toolButtons[2]).toHaveAttribute('aria-expanded', 'true') expect(toolButtons[3]).toHaveAttribute('aria-expanded', 'true') expect(toolButtons[4]).toHaveAttribute('aria-expanded', 'true') @@ -336,8 +323,6 @@ describe('freshclaude polish e2e: diff view for Edit tool', () => { }) it('shows color-coded diff when an Edit tool result contains old_string/new_string', () => { - // Tool strips are collapsed by default; set expanded to test ToolBlock content - setToolStripExpandedPreference(true) const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) store.dispatch(addUserMessage({ sessionId: 'sess-1', text: 'Edit a file' })) @@ -369,11 +354,8 @@ describe('freshclaude polish e2e: diff view for Edit tool', () => { , ) - // Tool block should be present; ensure it is expanded + // Tool block should be present; with showTools=true (default), it starts expanded const toolButton = screen.getByRole('button', { name: /tool call/i }) - if (toolButton.getAttribute('aria-expanded') !== 'true') { - fireEvent.click(toolButton) - } expect(toolButton).toHaveAttribute('aria-expanded', 'true') // DiffView should render with the diff figure role @@ -393,8 +375,6 @@ describe('freshclaude polish e2e: system-reminder stripping', () => { }) it('strips tags from tool result output', () => { - // Tool strips are collapsed by default; set expanded to verify content is sanitized - setToolStripExpandedPreference(true) const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) store.dispatch(addUserMessage({ sessionId: 'sess-1', text: 'Read a file' })) @@ -417,11 +397,8 @@ describe('freshclaude polish e2e: system-reminder stripping', () => { , ) - // Ensure the ToolBlock is expanded to verify the sanitized output + // ToolBlock should be expanded (showTools=true default) const toolButton = screen.getByRole('button', { name: /tool call/i }) - if (toolButton.getAttribute('aria-expanded') !== 'true') { - fireEvent.click(toolButton) - } expect(toolButton).toHaveAttribute('aria-expanded', 'true') // The visible output should contain the real content diff --git a/test/e2e/refresh-context-menu-flow.test.tsx b/test/e2e/refresh-context-menu-flow.test.tsx index 27358b3d..7a256c94 100644 --- a/test/e2e/refresh-context-menu-flow.test.tsx +++ b/test/e2e/refresh-context-menu-flow.test.tsx @@ -205,8 +205,12 @@ describe('refresh context menu flow (e2e)', () => { await waitFor(() => { expect(container.querySelectorAll('[data-context="pane"]')).toHaveLength(2) }) + // Only pane-1 (port 3000) uses TCP forwarding — it matches Freshell's own + // port so the HTTP proxy skips it. Pane-2 (port 3001) is proxied through + // /api/proxy/http/3001/ (same-origin) instead. Each TCP-forwarded pane + // triggers one api.post for the initial render plus one for the refresh. await waitFor(() => { - expect(vi.mocked(api.post)).toHaveBeenCalledTimes(4) + expect(vi.mocked(api.post)).toHaveBeenCalledTimes(2) }) await waitFor(() => { expect(store.getState().panes.refreshRequestsByPane['tab-1']).toBeUndefined() diff --git a/test/unit/client/components/agent-chat/AgentChatView.behavior.test.tsx b/test/unit/client/components/agent-chat/AgentChatView.behavior.test.tsx index d566e591..f663102b 100644 --- a/test/unit/client/components/agent-chat/AgentChatView.behavior.test.tsx +++ b/test/unit/client/components/agent-chat/AgentChatView.behavior.test.tsx @@ -292,15 +292,12 @@ describe('AgentChatView turn-pairing edge cases', () => { }) }) -describe('AgentChatView auto-expand', () => { +describe('AgentChatView tool blocks expanded by default', () => { afterEach(() => { cleanup() - localStorage.removeItem('freshell:toolStripExpanded') }) - it('auto-expands the most recent tool blocks', () => { - // Tool strips are collapsed by default; set expanded to test auto-expand behavior - localStorage.setItem('freshell:toolStripExpanded', 'true') + it('all tool blocks start expanded when showTools is true', () => { const store = makeStore() store.dispatch(sessionCreated({ requestId: 'req-1', sessionId: 'sess-1' })) // Create a turn with 5 completed tools @@ -312,16 +309,13 @@ describe('AgentChatView auto-expand', () => { , ) - // With RECENT_TOOLS_EXPANDED=3, the last 3 tools should be expanded - // and the first 2 collapsed. Check for expanded tool blocks via aria-expanded. + // With showTools=true (default), all tools should start expanded const toolButtons = screen.getAllByRole('button', { name: /tool call/i }) expect(toolButtons).toHaveLength(5) - // First 2 should be collapsed (aria-expanded=false) - expect(toolButtons[0]).toHaveAttribute('aria-expanded', 'false') - expect(toolButtons[1]).toHaveAttribute('aria-expanded', 'false') - - // Last 3 should be expanded (aria-expanded=true) + // All tools should be expanded (aria-expanded=true) + expect(toolButtons[0]).toHaveAttribute('aria-expanded', 'true') + expect(toolButtons[1]).toHaveAttribute('aria-expanded', 'true') expect(toolButtons[2]).toHaveAttribute('aria-expanded', 'true') expect(toolButtons[3]).toHaveAttribute('aria-expanded', 'true') expect(toolButtons[4]).toHaveAttribute('aria-expanded', 'true') diff --git a/test/unit/client/components/agent-chat/MessageBubble.test.tsx b/test/unit/client/components/agent-chat/MessageBubble.test.tsx index efde864a..f93c628b 100644 --- a/test/unit/client/components/agent-chat/MessageBubble.test.tsx +++ b/test/unit/client/components/agent-chat/MessageBubble.test.tsx @@ -3,12 +3,7 @@ import { render, screen, cleanup, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' import MessageBubble from '../../../../../src/components/agent-chat/MessageBubble' import type { ChatContentBlock } from '@/store/agentChatTypes' -import { - BROWSER_PREFERENCES_STORAGE_KEY, -} from '@/lib/browser-preferences' -// Render MarkdownRenderer synchronously to avoid React.lazy timing issues -// when running in the full test suite (dynamic import may not resolve in time) vi.mock('@/components/markdown/LazyMarkdown', async () => { const { MarkdownRenderer } = await import('@/components/markdown/MarkdownRenderer') return { @@ -19,9 +14,6 @@ vi.mock('@/components/markdown/LazyMarkdown', async () => { }) describe('MessageBubble', () => { - beforeEach(() => { - localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) - }) afterEach(() => { cleanup() }) @@ -31,7 +23,6 @@ describe('MessageBubble', () => { ) expect(screen.getByText('Hello world')).toBeInTheDocument() expect(screen.getByRole('article', { name: 'user message' })).toBeInTheDocument() - // User messages have thicker left border const article = container.querySelector('[role="article"]')! expect(article.className).toContain('border-l-[3px]') }) @@ -77,15 +68,15 @@ describe('MessageBubble', () => { expect(screen.getByText(/Thinking/)).toBeInTheDocument() }) - it('renders tool use block inside a tool strip', () => { + it('renders tool use block inside a tool strip (expanded when showTools=true)', () => { render( ) - // Tool is now inside a strip in collapsed mode - expect(screen.getByText('1 tool used')).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) it('renders timestamp and model', async () => { @@ -124,7 +115,6 @@ describe('MessageBubble', () => { content={[{ type: 'text', text: SCRIPT_PAYLOAD }]} /> ) - // react-markdown strips script tags entirely expect(container.querySelector('script')).toBeNull() }) @@ -153,6 +143,7 @@ describe('MessageBubble', () => { ) expect(container.querySelector('script')).toBeNull() @@ -161,9 +152,6 @@ describe('MessageBubble', () => { }) describe('MessageBubble display toggles', () => { - beforeEach(() => { - localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) - }) afterEach(cleanup) const textBlock: ChatContentBlock = { type: 'text', text: 'Hello world' } @@ -194,7 +182,8 @@ describe('MessageBubble display toggles', () => { expect(screen.getByText(/Let me think/)).toBeInTheDocument() }) - it('shows collapsed tool strip when showTools is false', () => { + it('shows collapsed tool strip when showTools is false, chevron still works', async () => { + const user = userEvent.setup() const { container } = render( { showTools={false} /> ) - // Tool strip should still be visible (collapsed summary) expect(container.querySelectorAll('[aria-label="Tool strip"]')).toHaveLength(1) - // But no expand chevron should be available - expect(screen.queryByRole('button', { name: /toggle tool details/i })).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: /toggle tool details/i })).toBeInTheDocument() + expect(screen.queryByRole('button', { name: /Bash tool call/i })).not.toBeInTheDocument() + + const toggle = screen.getByRole('button', { name: /toggle tool details/i }) + await user.click(toggle) + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) it('shows collapsed tool strip for tool_result when showTools is false', () => { @@ -216,7 +208,6 @@ describe('MessageBubble display toggles', () => { showTools={false} /> ) - // Tool strip should still be visible (collapsed summary) expect(container.querySelectorAll('[aria-label="Tool strip"]')).toHaveLength(1) }) @@ -253,16 +244,12 @@ describe('MessageBubble display toggles', () => { /> ) expect(screen.getByText(/Let me think/)).toBeInTheDocument() - // Tool is now in a strip expect(screen.getByRole('region', { name: /tool strip/i })).toBeInTheDocument() expect(screen.getByRole('article').querySelector('time')).not.toBeInTheDocument() }) }) describe('MessageBubble empty message hiding', () => { - beforeEach(() => { - localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) - }) afterEach(cleanup) it('shows collapsed strip when all content is tools and showTools is false', () => { @@ -276,7 +263,6 @@ describe('MessageBubble empty message hiding', () => { showTools={false} /> ) - // Message should still render (collapsed strip is visible content) expect(container.querySelector('[role="article"]')).toBeInTheDocument() expect(container.querySelectorAll('[aria-label="Tool strip"]')).toHaveLength(1) }) @@ -304,7 +290,6 @@ describe('MessageBubble empty message hiding', () => { showTools={false} /> ) - // Message should still render because the collapsed tool strip is visible expect(container.querySelector('[role="article"]')).toBeInTheDocument() expect(container.querySelectorAll('[aria-label="Tool strip"]')).toHaveLength(1) }) @@ -326,13 +311,9 @@ describe('MessageBubble empty message hiding', () => { }) describe('MessageBubble system-reminder stripping', () => { - beforeEach(() => { - localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) - }) afterEach(cleanup) it('strips system-reminder tags from standalone tool result content', async () => { - const user = userEvent.setup() render( { tool_use_id: 't1', content: 'actual content\n\nHidden system text\n\nmore content', }]} + showTools={true} /> ) - // First expand the strip, then click the individual tool - await user.click(screen.getByRole('button', { name: /toggle tool details/i })) - await user.click(screen.getByRole('button', { name: 'Result tool call' })) + expect(screen.getByRole('button', { name: 'Result tool call' })).toHaveAttribute('aria-expanded', 'true') expect(screen.getByText(/actual content/)).toBeInTheDocument() expect(screen.queryByText(/Hidden system text/)).not.toBeInTheDocument() }) it('strips system-reminder tags from paired tool_use/tool_result content', async () => { - const user = userEvent.setup() render( { content: 'file content\n\nSecret metadata\n\nmore', }, ]} + showTools={true} /> ) - // First expand the strip, then click the individual tool - await user.click(screen.getByRole('button', { name: /toggle tool details/i })) - await user.click(screen.getByRole('button', { name: 'Read tool call' })) + expect(screen.getByRole('button', { name: 'Read tool call' })).toHaveAttribute('aria-expanded', 'true') expect(screen.getByText(/file content/)).toBeInTheDocument() expect(screen.queryByText(/Secret metadata/)).not.toBeInTheDocument() }) }) describe('MessageBubble tool strip grouping', () => { - beforeEach(() => { - localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) - }) afterEach(cleanup) - it('groups contiguous tool blocks into a single ToolStrip', () => { + it('groups contiguous tool blocks into a single ToolStrip (expanded when showTools=true)', () => { render( { { type: 'tool_result', tool_use_id: 't2', content: 'content' }, { type: 'text', text: 'More text' }, ]} + showTools={true} /> ) - // Should render a single ToolStrip (with "2 tools used"), not individual ToolBlocks - expect(screen.getByText('2 tools used')).toBeInTheDocument() - // Both text blocks should still be visible outside the strip + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Read tool call/i })).toBeInTheDocument() expect(screen.getByText('Here is some text')).toBeInTheDocument() expect(screen.getByText('More text')).toBeInTheDocument() }) @@ -411,15 +386,15 @@ describe('MessageBubble tool strip grouping', () => { { type: 'tool_use', id: 't2', name: 'Bash', input: { command: 'echo 2' } }, { type: 'tool_result', tool_use_id: 't2', content: '2' }, ]} + showTools={true} /> ) - // Two separate strips, each with 1 tool const strips = container.querySelectorAll('[aria-label="Tool strip"]') expect(strips).toHaveLength(2) expect(screen.getByText('Middle text')).toBeInTheDocument() }) - it('renders a single tool as a strip', () => { + it('renders a single tool as a strip (expanded when showTools=true)', () => { render( { { type: 'tool_use', id: 't1', name: 'Bash', input: { command: 'ls' } }, { type: 'tool_result', tool_use_id: 't1', content: 'output' }, ]} + showTools={true} /> ) - expect(screen.getByText('1 tool used')).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) - it('shows collapsed strips when showTools is false', () => { + it('shows collapsed strips when showTools is false, chevron works', async () => { + const user = userEvent.setup() const { container } = render( { showTools={false} /> ) - // Tool strip should be visible (collapsed summary) expect(container.querySelectorAll('[aria-label="Tool strip"]')).toHaveLength(1) - // But no expand button - expect(screen.queryByRole('button', { name: /toggle tool details/i })).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: /toggle tool details/i })).toBeInTheDocument() + expect(screen.queryByRole('button', { name: /Bash tool call/i })).not.toBeInTheDocument() expect(screen.getByText('Hello')).toBeInTheDocument() + + const toggle = screen.getByRole('button', { name: /toggle tool details/i }) + await user.click(toggle) + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) it('includes running tool_use without result in the strip', () => { @@ -461,31 +441,28 @@ describe('MessageBubble tool strip grouping', () => { { type: 'tool_use', id: 't2', name: 'Read', input: { file_path: 'f.ts' } }, ]} isLastMessage={true} + showTools={true} /> ) - // The strip should contain 2 tools (one complete, one running) const strip = screen.getByRole('region', { name: /tool strip/i }) expect(strip).toBeInTheDocument() }) - it('renders orphaned tool_result as standalone strip named "Result"', async () => { - const user = userEvent.setup() + it('renders orphaned tool_result as standalone strip named "Result"', () => { render( ) - // Should render as a ToolStrip const strip = screen.getByRole('region', { name: /tool strip/i }) expect(strip).toBeInTheDocument() - // Expand the strip and then the "Result" tool block to verify content - await user.click(screen.getByRole('button', { name: /toggle tool details/i })) const resultButton = screen.getByRole('button', { name: 'Result tool call' }) expect(resultButton).toBeInTheDocument() - await user.click(resultButton) + expect(resultButton).toHaveAttribute('aria-expanded', 'true') expect(screen.getByText('orphaned data')).toBeInTheDocument() }) @@ -499,20 +476,19 @@ describe('MessageBubble tool strip grouping', () => { { type: 'tool_use', id: 't1', name: 'Bash', input: { command: 'ls' } }, { type: 'tool_result', tool_use_id: 't1', content: 'output' }, ]} + showTools={true} /> ) expect(screen.getByText(/Let me think/)).toBeInTheDocument() - expect(screen.getByText('1 tool used')).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) }) describe('MessageBubble tool strip visual behavior', () => { - beforeEach(() => { - localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) - }) afterEach(cleanup) - it('renders collapsed strip with summary text when showTools is false', () => { + it('renders collapsed strip with summary text when showTools is false, chevron works', async () => { + const user = userEvent.setup() const { container } = render( { /> ) - // The message renders expect(screen.getByRole('article')).toBeInTheDocument() - // Text blocks are visible expect(screen.getByText('Let me check that for you.')).toBeInTheDocument() expect(screen.getByText('All looks good!')).toBeInTheDocument() - // Tool strip is visible with collapsed summary const strips = container.querySelectorAll('[aria-label="Tool strip"]') expect(strips).toHaveLength(1) expect(screen.getByText('3 tools used')).toBeInTheDocument() - // No expand chevron - expect(screen.queryByRole('button', { name: /toggle tool details/i })).not.toBeInTheDocument() - // No individual tool blocks visible + expect(screen.getByRole('button', { name: /toggle tool details/i })).toBeInTheDocument() expect(screen.queryByRole('button', { name: /Bash tool call/i })).not.toBeInTheDocument() expect(screen.queryByRole('button', { name: /Read tool call/i })).not.toBeInTheDocument() expect(screen.queryByRole('button', { name: /Grep tool call/i })).not.toBeInTheDocument() + + const toggle = screen.getByRole('button', { name: /toggle tool details/i }) + await user.click(toggle) + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Read tool call/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Grep tool call/i })).toBeInTheDocument() }) - it('renders expandable strip with chevron when showTools is true', async () => { + it('renders expanded strip with tool blocks when showTools is true', () => { + render( + + ) + + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /toggle tool details/i })).toBeInTheDocument() + }) + + it('can collapse strip by clicking toggle when showTools is true', async () => { const user = userEvent.setup() render( { /> ) - // Collapsed by default with chevron - expect(screen.getByText('1 tool used')).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() const chevron = screen.getByRole('button', { name: /toggle tool details/i }) - expect(chevron).toBeInTheDocument() - - // Click to expand await user.click(chevron) - expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + expect(screen.getByText('1 tool used')).toBeInTheDocument() }) }) diff --git a/test/unit/client/components/agent-chat/ToolStrip.test.tsx b/test/unit/client/components/agent-chat/ToolStrip.test.tsx index 2bac04f5..0609fc04 100644 --- a/test/unit/client/components/agent-chat/ToolStrip.test.tsx +++ b/test/unit/client/components/agent-chat/ToolStrip.test.tsx @@ -3,13 +3,6 @@ import { render, screen, cleanup } from '@testing-library/react' import userEvent from '@testing-library/user-event' import ToolStrip from '@/components/agent-chat/ToolStrip' import type { ToolPair } from '@/components/agent-chat/ToolStrip' -import { - BROWSER_PREFERENCES_STORAGE_KEY, - getToolStripExpandedPreference, - loadBrowserPreferencesRecord, -} from '@/lib/browser-preferences' - -const LEGACY_TOOL_STRIP_STORAGE_KEY = 'freshell:toolStripExpanded' function makePair( name: string, @@ -29,79 +22,109 @@ function makePair( describe('ToolStrip', () => { beforeEach(() => { - localStorage.removeItem(BROWSER_PREFERENCES_STORAGE_KEY) - localStorage.removeItem(LEGACY_TOOL_STRIP_STORAGE_KEY) + localStorage.clear() }) afterEach(cleanup) - it('renders collapsed by default showing the latest tool preview', () => { + it('starts expanded when showTools is true', () => { const pairs = [ makePair('Bash', { command: 'echo hello' }, 'hello'), makePair('Read', { file_path: '/path/file.ts' }, 'content'), ] - render() - // Collapsed: shows "2 tools used" - expect(screen.getByText('2 tools used')).toBeInTheDocument() + render() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Read tool call/i })).toBeInTheDocument() }) - it('always shows chevron button', () => { + it('always shows chevron button when showTools is true', () => { const pairs = [makePair('Bash', { command: 'ls' }, 'output')] - render() + render() expect(screen.getByRole('button', { name: /toggle tool details/i })).toBeInTheDocument() }) - it('uses compact spacing in collapsed mode', () => { + it('uses compact spacing in expanded mode', () => { const pairs = [makePair('Bash', { command: 'ls' }, 'output')] - const { container } = render() + const { container } = render() const strip = screen.getByRole('region', { name: /tool strip/i }) expect(strip.className).toContain('my-0.5') - - const collapsedRow = container.querySelector('[aria-label="Tool strip"] > div') as HTMLElement - expect(collapsedRow.className).toContain('py-0.5') }) - it('expands on chevron click and persists to browser preferences', async () => { + it('starts collapsed when showTools is false, chevron still works', async () => { const user = userEvent.setup() const pairs = [ makePair('Bash', { command: 'ls' }, 'file1\nfile2'), ] - render() + render() + expect(screen.getByText('1 tool used')).toBeInTheDocument() + expect(screen.getByRole('button', { name: /toggle tool details/i })).toBeInTheDocument() + expect(screen.queryByRole('button', { name: /Bash tool call/i })).not.toBeInTheDocument() const toggle = screen.getByRole('button', { name: /toggle tool details/i }) await user.click(toggle) - - // Expanded: should show individual ToolBlock expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() - // Persisted - expect(loadBrowserPreferencesRecord().toolStrip?.expanded).toBe(true) }) - it('starts expanded when browser preferences have a stored preference', () => { - localStorage.setItem(BROWSER_PREFERENCES_STORAGE_KEY, JSON.stringify({ - toolStrip: { expanded: true }, - })) - const pairs = [ - makePair('Bash', { command: 'ls' }, 'file1\nfile2'), - ] - render() - // Should show individual ToolBlock - expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + it('strip toggle is session-only (not persisted to localStorage)', async () => { + const user = userEvent.setup() + const pairs = [makePair('Bash', { command: 'ls' }, 'file1\nfile2')] + render() + + const toggle = screen.getByRole('button', { name: /toggle tool details/i }) + await user.click(toggle) + + expect(screen.getByText('1 tool used')).toBeInTheDocument() + expect(localStorage.getItem('freshell:browser-preferences')).toBeNull() }) - it('collapses on second chevron click and stores false in browser preferences', async () => { - localStorage.setItem(BROWSER_PREFERENCES_STORAGE_KEY, JSON.stringify({ - toolStrip: { expanded: true }, - })) + it('collapses on second chevron click', async () => { const user = userEvent.setup() const pairs = [makePair('Bash', { command: 'ls' }, 'file1')] - render() + render() + + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() const toggle = screen.getByRole('button', { name: /toggle tool details/i }) await user.click(toggle) + expect(screen.getByText('1 tool used')).toBeInTheDocument() + }) + + it('ToolBlocks start expanded when showTools is true', () => { + const pairs = [ + makePair('Bash', { command: 'ls' }, 'output'), + ] + render() + + const toolButton = screen.getByRole('button', { name: /Bash tool call/i }) + expect(toolButton).toBeInTheDocument() + expect(toolButton).toHaveAttribute('aria-expanded', 'true') + }) + + it('ToolBlocks are not visible when showTools is false', () => { + const pairs = [ + makePair('Bash', { command: 'ls' }, 'output'), + ] + render() - // Should be collapsed again expect(screen.getByText('1 tool used')).toBeInTheDocument() - expect(loadBrowserPreferencesRecord().toolStrip?.expanded).toBe(false) + expect(screen.queryByRole('button', { name: /Bash tool call/i })).not.toBeInTheDocument() + }) + + it('individual ToolBlock toggles work independently', async () => { + const user = userEvent.setup() + const pairs = [ + makePair('Bash', { command: 'ls' }, 'output1'), + makePair('Read', { file_path: 'f.ts' }, 'output2'), + ] + render() + + const toolButtons = screen.getAllByRole('button', { name: /tool call/i }) + expect(toolButtons).toHaveLength(2) + expect(toolButtons[0]).toHaveAttribute('aria-expanded', 'true') + expect(toolButtons[1]).toHaveAttribute('aria-expanded', 'true') + + await user.click(toolButtons[0]) + expect(toolButtons[0]).toHaveAttribute('aria-expanded', 'false') + expect(toolButtons[1]).toHaveAttribute('aria-expanded', 'true') }) it('shows streaming tool activity when isStreaming is true', () => { @@ -109,28 +132,28 @@ describe('ToolStrip', () => { makePair('Bash', { command: 'echo hello' }, 'hello'), makePair('Read', { file_path: '/path/to/file.ts' }), ] - render() - // Should show the currently running tool's info - expect(screen.getByText('Read')).toBeInTheDocument() + render() + expect(screen.getByRole('button', { name: /Read tool call/i })).toBeInTheDocument() }) - it('shows "N tools used" when all tools are complete and not streaming', () => { + it('shows all tools when complete', () => { const pairs = [ makePair('Bash', { command: 'ls' }, 'output'), makePair('Read', { file_path: 'f.ts' }, 'content'), makePair('Grep', { pattern: 'foo' }, 'bar'), ] - render() - expect(screen.getByText('3 tools used')).toBeInTheDocument() + render() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Read tool call/i })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /Grep tool call/i })).toBeInTheDocument() }) it('renders with error indication when any tool has isError', () => { const pairs = [ makePair('Bash', { command: 'false' }, 'error output', true), ] - render() - // The strip should still render; error styling is at the ToolBlock level in expanded view - expect(screen.getByText('1 tool used')).toBeInTheDocument() + render() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) it('shows hasErrors indicator in collapsed mode when a tool errored', () => { @@ -138,63 +161,56 @@ describe('ToolStrip', () => { makePair('Bash', { command: 'false' }, 'error output', true), makePair('Read', { file_path: 'f.ts' }, 'content'), ] - const { container } = render() + const { container } = render() const strip = screen.getByRole('region', { name: /tool strip/i }) expect(strip).toBeInTheDocument() - // Collapsed row should have the error border color instead of the normal tool color const collapsedRow = container.querySelector('.border-l-\\[hsl\\(var\\(--claude-error\\)\\)\\]') expect(collapsedRow).toBeInTheDocument() }) it('renders accessible region with aria-label', () => { const pairs = [makePair('Bash', { command: 'ls' }, 'output')] - render() + render() expect(screen.getByRole('region', { name: /tool strip/i })).toBeInTheDocument() }) - it('always shows collapsed view when showTools is false, even if localStorage says expanded', () => { - localStorage.setItem(BROWSER_PREFERENCES_STORAGE_KEY, JSON.stringify({ - toolStrip: { expanded: true }, - })) + it('shows collapsed view by default when showTools is false, chevron still works', async () => { + const user = userEvent.setup() const pairs = [ makePair('Bash', { command: 'ls' }, 'file1\nfile2'), makePair('Read', { file_path: '/path/file.ts' }, 'content'), ] render() - // Should show collapsed summary text expect(screen.getByText('2 tools used')).toBeInTheDocument() - // Chevron toggle should NOT be rendered - expect(screen.queryByRole('button', { name: /toggle tool details/i })).not.toBeInTheDocument() - // Individual ToolBlocks should NOT be rendered + expect(screen.getByRole('button', { name: /toggle tool details/i })).toBeInTheDocument() expect(screen.queryByRole('button', { name: /Bash tool call/i })).not.toBeInTheDocument() + + const toggle = screen.getByRole('button', { name: /toggle tool details/i }) + await user.click(toggle) + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) - it('passes autoExpandAbove props through to ToolBlocks in expanded mode', async () => { - localStorage.setItem(BROWSER_PREFERENCES_STORAGE_KEY, JSON.stringify({ - toolStrip: { expanded: true }, - })) - const pairs = [ - makePair('Bash', { command: 'echo 1' }, 'output1'), - makePair('Bash', { command: 'echo 2' }, 'output2'), - makePair('Bash', { command: 'echo 3' }, 'output3'), - ] - render( - - ) + it('resets to showTools default when component remounts', async () => { + const user = userEvent.setup() + const pairs = [makePair('Bash', { command: 'ls' }, 'file1')] - const toolButtons = screen.getAllByRole('button', { name: /Bash tool call/i }) - expect(toolButtons).toHaveLength(3) - // Tool at index 0 (globalIndex=0) should be collapsed (below autoExpandAbove=1) - expect(toolButtons[0]).toHaveAttribute('aria-expanded', 'false') - // Tools at indices 1,2 (globalIndex=1,2) should be expanded (>= autoExpandAbove=1) - expect(toolButtons[1]).toHaveAttribute('aria-expanded', 'true') - expect(toolButtons[2]).toHaveAttribute('aria-expanded', 'true') - }) + const { unmount } = render() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() - it('migrates the legacy tool-strip key through the browser preferences helper', () => { - localStorage.setItem(LEGACY_TOOL_STRIP_STORAGE_KEY, 'true') + const toggle = screen.getByRole('button', { name: /toggle tool details/i }) + await user.click(toggle) + expect(screen.getByText('1 tool used')).toBeInTheDocument() + unmount() + + cleanup() + + render() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() + }) - expect(getToolStripExpandedPreference()).toBe(true) - expect(loadBrowserPreferencesRecord().toolStrip?.expanded).toBe(true) + it('defaults to showTools=true when not specified', () => { + const pairs = [makePair('Bash', { command: 'ls' }, 'output')] + render() + expect(screen.getByRole('button', { name: /Bash tool call/i })).toBeInTheDocument() }) }) diff --git a/test/unit/client/lib/browser-preferences.test.ts b/test/unit/client/lib/browser-preferences.test.ts index fd96a727..60bb9b7f 100644 --- a/test/unit/client/lib/browser-preferences.test.ts +++ b/test/unit/client/lib/browser-preferences.test.ts @@ -3,7 +3,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { BROWSER_PREFERENCES_STORAGE_KEY, getSearchRangeDaysPreference, - getToolStripExpandedPreference, loadBrowserPreferencesRecord, patchBrowserPreferencesRecord, seedBrowserPreferencesSettingsIfEmpty, @@ -40,9 +39,8 @@ describe('browser preferences', () => { }) }) - it('migrates legacy font and tool-strip keys into the new blob once', () => { + it('migrates legacy font key into the new blob once', () => { localStorage.setItem('freshell.terminal.fontFamily.v1', 'Fira Code') - localStorage.setItem('freshell:toolStripExpanded', 'true') expect(loadBrowserPreferencesRecord()).toEqual({ settings: { @@ -50,27 +48,19 @@ describe('browser preferences', () => { fontFamily: 'Fira Code', }, }, - toolStrip: { - expanded: true, - }, }) expect(localStorage.getItem('freshell.terminal.fontFamily.v1')).toBeNull() - expect(localStorage.getItem('freshell:toolStripExpanded')).toBeNull() expect(localStorage.getItem(BROWSER_PREFERENCES_STORAGE_KEY)).toBe(JSON.stringify({ settings: { terminal: { fontFamily: 'Fira Code', }, }, - toolStrip: { - expanded: true, - }, })) }) it('keeps legacy keys when migrating into the new blob fails to save', () => { localStorage.setItem('freshell.terminal.fontFamily.v1', 'Fira Code') - localStorage.setItem('freshell:toolStripExpanded', 'true') const originalSetItem = Storage.prototype.setItem const setItemSpy = vi.spyOn(Storage.prototype, 'setItem').mockImplementation(function (key: string, value: string) { @@ -86,12 +76,8 @@ describe('browser preferences', () => { fontFamily: 'Fira Code', }, }, - toolStrip: { - expanded: true, - }, }) expect(localStorage.getItem('freshell.terminal.fontFamily.v1')).toBe('Fira Code') - expect(localStorage.getItem('freshell:toolStripExpanded')).toBe('true') expect(localStorage.getItem(BROWSER_PREFERENCES_STORAGE_KEY)).toBeNull() setItemSpy.mockRestore() @@ -137,17 +123,13 @@ describe('browser preferences', () => { }) }) - it('reads tool-strip and search-range preferences from the new blob', () => { + it('reads search-range preferences from the new blob', () => { patchBrowserPreferencesRecord({ - toolStrip: { - expanded: true, - }, tabs: { searchRangeDays: 365, }, }) - expect(getToolStripExpandedPreference()).toBe(true) expect(getSearchRangeDaysPreference()).toBe(365) }) }) diff --git a/test/unit/client/store/crossTabSync.test.ts b/test/unit/client/store/crossTabSync.test.ts index 9760e7f5..6bd836ba 100644 --- a/test/unit/client/store/crossTabSync.test.ts +++ b/test/unit/client/store/crossTabSync.test.ts @@ -272,7 +272,7 @@ describe('crossTabSync', () => { }) }) - it('ignores toolStrip-only browser-preference writes for Redux local settings and search range', () => { + it('ignores empty browser-preference writes for Redux local settings and search range', () => { const store = configureStore({ reducer: { settings: settingsReducer, tabRegistry: tabRegistryReducer }, }) @@ -286,11 +286,7 @@ describe('crossTabSync', () => { window.dispatchEvent(new StorageEvent('storage', { key: BROWSER_PREFERENCES_STORAGE_KEY, - newValue: JSON.stringify({ - toolStrip: { - expanded: true, - }, - }), + newValue: JSON.stringify({}), })) expect(store.getState().settings.settings.theme).toBe('dark') @@ -320,11 +316,7 @@ describe('crossTabSync', () => { window.dispatchEvent(new StorageEvent('storage', { key: BROWSER_PREFERENCES_STORAGE_KEY, - newValue: JSON.stringify({ - toolStrip: { - expanded: true, - }, - }), + newValue: JSON.stringify({}), })) expect(store.getState().settings.settings.theme).toBe('system') diff --git a/test/unit/client/store/storage-migration.test.ts b/test/unit/client/store/storage-migration.test.ts index 51ea6d18..b9c76b91 100644 --- a/test/unit/client/store/storage-migration.test.ts +++ b/test/unit/client/store/storage-migration.test.ts @@ -64,10 +64,9 @@ describe('storage-migration', () => { expect(document.cookie).not.toContain('freshell-auth=') }) - it('preserves legacy terminal font and tool-strip migration when storage cleanup runs before browser preferences load', async () => { + it('preserves legacy terminal font migration when storage cleanup runs before browser preferences load', async () => { localStorage.setItem('freshell_version', '2') localStorage.setItem('freshell.terminal.fontFamily.v1', 'Fira Code') - localStorage.setItem('freshell:toolStripExpanded', 'true') localStorage.setItem('freshell.tabs.v1', 'legacy-tabs') await importFreshStorageMigration() @@ -80,9 +79,6 @@ describe('storage-migration', () => { fontFamily: 'Fira Code', }, }, - toolStrip: { - expanded: true, - }, }) expect(localStorage.getItem(BROWSER_PREFERENCES_STORAGE_KEY)).toBe(JSON.stringify({ settings: { @@ -90,12 +86,8 @@ describe('storage-migration', () => { fontFamily: 'Fira Code', }, }, - toolStrip: { - expanded: true, - }, })) expect(localStorage.getItem('freshell.terminal.fontFamily.v1')).toBeNull() - expect(localStorage.getItem('freshell:toolStripExpanded')).toBeNull() expect(localStorage.getItem('freshell.tabs.v1')).toBeNull() expect(localStorage.getItem('freshell_version')).toBe('3') })