diff --git a/desktop/src/components/chat/AskUserQuestion.test.tsx b/desktop/src/components/chat/AskUserQuestion.test.tsx index a691668d..eeafb13b 100644 --- a/desktop/src/components/chat/AskUserQuestion.test.tsx +++ b/desktop/src/components/chat/AskUserQuestion.test.tsx @@ -966,4 +966,150 @@ describe('AskUserQuestion', () => { expect(storedDraft()).toBeUndefined() }) }) + + // Issue #1400. The input is whatever the model emitted. The server validates it and + // answers a bad call with InputValidationError, but the transcript keeps the + // tool_use as sent, so a card is rebuilt from it on every replay. One provider's + // tool-call parser turned "`rm `" inside a description into an object + // ({ $text, path }); rendering that as a child threw React #31, which took the + // whole app down — again on every launch, since the open tab is restored. + describe('input the model got wrong', () => { + // The exact shape from the report: text around an inline became an object. + const BROKEN_TEXT = { + path: '`（一次一个，不用 wildcard）。CLAUDE.md 禁止 wildcard 删多个文件。', + $text: '我会逐个执行 `rm ', + } + const INVALID_INPUT_RESULT = + 'InputValidationError: AskUserQuestion failed due to the following issue:\n' + + 'The parameter `questions[0].options[0].description` type is expected as `string` but provided as `object`' + + function singleQuestion(question: Record) { + return { questions: [{ question: 'Pick one?', ...question }] } + } + + it('renders the rest of a question whose option description is an object', () => { + render() + + expect(screen.getByText('Delete these 5 files?')).toBeTruthy() + // The broken description is dropped with nothing rendered in its place. + expect(screen.getByRole('button', { name: /^Delete all 5$/ })).toBeTruthy() + // Its sound sibling keeps its own. + expect(screen.getByText('Back up first.')).toBeTruthy() + }) + + it('still answers a question that has a broken option, handing the original input back', () => { + const input = singleQuestion({ + question: 'Delete these 5 files?', + options: [{ label: 'Delete all 5', description: BROKEN_TEXT }, { label: 'Wait' }], + }) + render() + + fireEvent.click(screen.getByRole('button', { name: /^Wait$/ })) + fireEvent.click(screen.getByRole('button', { name: /submit/i })) + + // Only what is shown is cleaned up; the answer travels with the input untouched. + expect(sendMock).toHaveBeenCalledWith(ACTIVE_TAB, { + type: 'permission_response', + requestId: 'perm-1', + allowed: true, + updatedInput: { ...input, answers: { 'Delete these 5 files?': 'Wait' } }, + }) + }) + + it('shows a failed call from history together with its error result', () => { + render() + + expect(screen.getByText('Delete these 5 files?')).toBeTruthy() + expect(screen.getByText(/InputValidationError/)).toBeTruthy() + }) + + it('renders nothing for a question whose text is an object', () => { + const { container } = render() + + expect(container.textContent).toBe('') + }) + + it('drops an option whose label is an object and keeps its siblings', () => { + render() + + expect(screen.getByRole('button', { name: /^B$/ })).toBeTruthy() + expect(screen.queryByText('goes with its label')).toBeNull() + }) + + it.each([ + ['a string', 'A or B'], + ['an object', { A: 'first', B: 'second' }], + ])('renders the question when options is %s instead of a list', (_shape, options) => { + render() + + expect(screen.getByText('Pick one?')).toBeTruthy() + expect(screen.getByRole('textbox')).toBeTruthy() + }) + + it('skips null and non-object entries among the options', () => { + render() + + expect(screen.getByRole('button', { name: /^Real$/ })).toBeTruthy() + expect(screen.queryByText('stray text')).toBeNull() + }) + + it('skips entries of questions that are not questions', () => { + render() + + expect(screen.getByText('Real?')).toBeTruthy() + // One question is left, so there is no tab strip to number. + expect(screen.queryByRole('button', { name: 'Q1' })).toBeNull() + }) + + it('numbers the tab of a question whose header is not text', () => { + render() + + expect(screen.getByRole('button', { name: 'Q1' })).toBeTruthy() + expect(screen.getByRole('button', { name: 'Sound' })).toBeTruthy() + }) + + // chatStore rebuilds tool_use messages under a stable id, so a mounted card can be + // handed a repaired input after it was handed a broken one. + it('recovers when a broken input is replaced by a sound one while mounted', () => { + const { container, rerender } = render() + expect(container.textContent).toBe('') + + rerender() + + expect(screen.getByText('Ship it?')).toBeTruthy() + }) + }) }) diff --git a/desktop/src/components/chat/AskUserQuestion.tsx b/desktop/src/components/chat/AskUserQuestion.tsx index d4c72c41..810eb5cf 100644 --- a/desktop/src/components/chat/AskUserQuestion.tsx +++ b/desktop/src/components/chat/AskUserQuestion.tsx @@ -24,14 +24,6 @@ type Question = { multiSelect?: boolean } -type AskUserInput = { - questions?: Question[] - question?: string - header?: string - options?: QuestionOption[] - multiSelect?: boolean -} - type Props = { sessionId?: string | null toolUseId: string @@ -45,29 +37,51 @@ type Props = { supersededByUserMessage?: boolean } +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +// The input is whatever the model emitted, and the transcript keeps it even when the +// server rejected the call as invalid — so a card is rebuilt from it on every replay +// (issue #1400: an option description that came back as an object threw React #31 and +// took the whole app down, again on every launch). Only string fields are trusted, +// because a non-string child throws while rendering; anything else is left out rather +// than shown. That cleanup is display-only: the answer still travels with the +// original input (see handleSubmit). + +function toOption(value: unknown): QuestionOption | null { + if (!isRecord(value) || typeof value.label !== 'string') return null + return typeof value.description === 'string' + ? { label: value.label, description: value.description } + : { label: value.label } +} + +function toQuestion(value: unknown): Question | null { + if (!isRecord(value) || typeof value.question !== 'string') return null + return { + question: value.question, + header: typeof value.header === 'string' ? value.header : undefined, + options: Array.isArray(value.options) + ? value.options.flatMap((option) => toOption(option) ?? []) + : undefined, + multiSelect: value.multiSelect === true, + } +} + /** * Parse the AskUserQuestion input which may come in different shapes. */ function parseInput(input: unknown): Question[] { - if (!input || typeof input !== 'object') return [] - const obj = input as AskUserInput + if (!isRecord(input)) return [] // Shape 1: { questions: [...] } - if (Array.isArray(obj.questions)) { - return obj.questions + if (Array.isArray(input.questions)) { + return input.questions.flatMap((question) => toQuestion(question) ?? []) } // Shape 2: { question: "...", options: [...] } - if (typeof obj.question === 'string') { - return [{ - question: obj.question, - header: obj.header, - options: obj.options, - multiSelect: obj.multiSelect, - }] - } - - return [] + const single = toQuestion(input) + return single ? [single] : [] } type QuestionSelections = Record @@ -100,7 +114,7 @@ export function AskUserQuestion({ const sessionConnectionState = useChatStore((s) => targetSessionId ? s.sessions[targetSessionId]?.connectionState : undefined) const t = useTranslation() - const questions = parseInput(input) + const questions = useMemo(() => parseInput(input), [input]) const inputObject = (input && typeof input === 'object') ? input as Record : {} // Read once instead of subscribing: this card writes the draft on every // change, and a subscription would feed its own writes back as re-renders. diff --git a/desktop/src/components/chat/MessageList.itemBoundary.test.tsx b/desktop/src/components/chat/MessageList.itemBoundary.test.tsx new file mode 100644 index 00000000..0a8f6d61 --- /dev/null +++ b/desktop/src/components/chat/MessageList.itemBoundary.test.tsx @@ -0,0 +1,163 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { render, screen } from '@testing-library/react' + +const { reportMock } = vi.hoisted(() => ({ + reportMock: vi.fn(async () => undefined), +})) + +vi.mock('../../lib/diagnosticsCapture', async (importOriginal) => ({ + ...(await importOriginal()), + reportReactError: reportMock, +})) + +// A card that fails the way an unforeseen stored record would. What breaks a real one +// is beside the point here; this file is about where the failure is allowed to land. +vi.mock('./AskUserQuestion', () => ({ + AskUserQuestion: ({ toolUseId }: { toolUseId: string }) => { + if (toolUseId === 'tool-poisoned') throw new Error('card exploded') + return
question card {toolUseId}
+ }, +})) + +import { MessageList } from './MessageList' +import { sessionsApi } from '../../api/sessions' +import { useChatStore, type PerSessionState } from '../../stores/chatStore' +import { useSettingsStore } from '../../stores/settingsStore' +import { useSessionStore } from '../../stores/sessionStore' +import { useTabStore } from '../../stores/tabStore' +import { useTeamStore } from '../../stores/teamStore' +import { useWorkspaceChatContextStore } from '../../stores/workspaceChatContextStore' +import { useWorkspaceStore } from '../../stores/workspaceStore' +import type { UIMessage } from '../../types/chat' + +const ACTIVE_TAB = 'active-tab' + +function makeSessionState(messages: UIMessage[]): PerSessionState { + return { + messages, + chatState: 'idle', + connectionState: 'connected', + historyStatus: 'ready', + historyHydrated: true, + streamingText: '', + streamingToolInput: '', + activeToolUseId: null, + activeToolName: null, + activeThinkingId: null, + pendingPermission: null, + pendingComputerUsePermission: null, + tokenUsage: { input_tokens: 0, output_tokens: 0 }, + streamingResponseChars: 0, + elapsedSeconds: 0, + statusVerb: '', + apiRetry: null, + slashCommands: [], + agentTaskNotifications: {}, + elapsedTimer: null, + composerPrefill: null, + } +} + +function askMessage(toolUseId: string, timestamp: number): UIMessage { + return { + id: `ask-${toolUseId}`, + type: 'tool_use', + toolName: 'AskUserQuestion', + toolUseId, + input: { questions: [{ question: 'Which scope?', options: [{ label: 'A' }, { label: 'B' }] }] }, + timestamp, + } +} + +describe('MessageList item containment', () => { + beforeEach(() => { + vi.restoreAllMocks() + reportMock.mockClear() + // React logs every caught render error; keep the output readable. + vi.spyOn(console, 'error').mockImplementation(() => {}) + useSettingsStore.setState({ locale: 'en' }) + useTabStore.setState({ + activeTabId: ACTIVE_TAB, + tabs: [{ sessionId: ACTIVE_TAB, title: 'Test', type: 'session' as const, status: 'idle' }], + }) + useSessionStore.setState({ sessions: [], activeSessionId: null, isLoading: false, error: null }) + useTeamStore.getState().clearTeam() + useWorkspaceChatContextStore.setState(useWorkspaceChatContextStore.getInitialState(), true) + useWorkspaceStore.setState(useWorkspaceStore.getInitialState(), true) + vi.spyOn(sessionsApi, 'getTurnCheckpoints').mockImplementation(() => new Promise(() => {})) + vi.spyOn(sessionsApi, 'getWorkspaceStatus').mockResolvedValue({ + state: 'ok', + workDir: '/tmp/example-project', + repoName: 'example-project', + branch: null, + isGitRepo: false, + changedFiles: [], + }) + }) + + // Issue #1400: one poisoned record in a saved transcript used to replace the whole + // app with the root error page, again on every launch. + it('keeps a row that fails to render from taking the transcript down', () => { + useChatStore.setState({ + sessions: { + [ACTIVE_TAB]: makeSessionState([ + { id: 'user-1', type: 'user_text', content: 'clean up the temp files', timestamp: 1 }, + { id: 'assistant-1', type: 'assistant_text', content: 'reply before the bad card', timestamp: 2 }, + askMessage('tool-poisoned', 3), + { id: 'assistant-2', type: 'assistant_text', content: 'reply after the bad card', timestamp: 4 }, + ]), + }, + }) + + render() + + expect(screen.getByText('clean up the temp files')).toBeTruthy() + expect(screen.getByText('reply before the bad card')).toBeTruthy() + expect(screen.getByText('reply after the bad card')).toBeTruthy() + expect(screen.getByText(/This item couldn't be displayed/)).toBeTruthy() + expect(reportMock).toHaveBeenCalledTimes(1) + }) + + it('leaves healthy cards in the same transcript rendered', () => { + useChatStore.setState({ + sessions: { + [ACTIVE_TAB]: makeSessionState([ + // Answered, so it stays in the history; only the latest unresolved question + // is ever shown, which would hide it otherwise. + askMessage('tool-fine', 1), + { + id: 'result-fine', + type: 'tool_result', + toolUseId: 'tool-fine', + content: { answers: { 'Which scope?': 'A' } }, + isError: false, + timestamp: 2, + }, + askMessage('tool-poisoned', 3), + ]), + }, + }) + + render() + + expect(screen.getByText('question card tool-fine')).toBeTruthy() + expect(screen.getAllByText(/This item couldn't be displayed/)).toHaveLength(1) + }) + + it('shows no notice and reports nothing for a transcript that renders', () => { + useChatStore.setState({ + sessions: { + [ACTIVE_TAB]: makeSessionState([ + { id: 'assistant-1', type: 'assistant_text', content: 'all good here', timestamp: 1 }, + askMessage('tool-fine', 2), + ]), + }, + }) + + render() + + expect(screen.getByText('all good here')).toBeTruthy() + expect(screen.queryByText(/couldn't be displayed/)).toBeNull() + expect(reportMock).not.toHaveBeenCalled() + }) +}) diff --git a/desktop/src/components/chat/MessageList.tsx b/desktop/src/components/chat/MessageList.tsx index 9318e6ba..a6c13d1c 100644 --- a/desktop/src/components/chat/MessageList.tsx +++ b/desktop/src/components/chat/MessageList.tsx @@ -28,6 +28,7 @@ import type { ActivityStep } from './activityGroupModel' import { ToolResultBlock } from './ToolResultBlock' import { PermissionDialog } from './PermissionDialog' import { AskUserQuestion } from './AskUserQuestion' +import { RenderItemBoundary } from './RenderItemBoundary' import { StreamingIndicator } from './StreamingIndicator' import { InlineTaskSummary } from './InlineTaskSummary' import { CurrentTurnChangeCard } from './CurrentTurnChangeCard' @@ -3555,70 +3556,72 @@ export function MessageList({ return ( <> - {item.kind === 'tool_group' ? ( - !toolResultMap.has(tc.toolUseId)) - } - // Only the tail of a live turn can still grow. Everything above it - // is finished, whatever any individual tool's state looks like this - // instant — which is why this, and not `isStreaming`, decides - // whether a run stands open. - isLive={chatState !== 'idle' && index === renderItems.length - 1 && !hasTrailingStreamingItem} - disclosureKey={getRenderItemKey(item)} - /> - ) : item.kind === 'team_card' ? ( - resolvedSessionId ? (() => { - const cardSnapshot = snapshotForTeamCard(teamSnapshot, item) - const fallbackPhase = item.endedAt !== undefined || teamTaskWindows.some((window) => ( - item.startedAt >= window.startedAt && - window.endedAt !== undefined && - item.startedAt <= window.endedAt - )) ? 'completed' : 'forming' - return ( - openTeamWorkbench(resolvedSessionId, cardSnapshot.team.name) - : undefined} - > - - - ) - })() : null - ) : ( - - )} + + {item.kind === 'tool_group' ? ( + !toolResultMap.has(tc.toolUseId)) + } + // Only the tail of a live turn can still grow. Everything above it + // is finished, whatever any individual tool's state looks like this + // instant — which is why this, and not `isStreaming`, decides + // whether a run stands open. + isLive={chatState !== 'idle' && index === renderItems.length - 1 && !hasTrailingStreamingItem} + disclosureKey={getRenderItemKey(item)} + /> + ) : item.kind === 'team_card' ? ( + resolvedSessionId ? (() => { + const cardSnapshot = snapshotForTeamCard(teamSnapshot, item) + const fallbackPhase = item.endedAt !== undefined || teamTaskWindows.some((window) => ( + item.startedAt >= window.startedAt && + window.endedAt !== undefined && + item.startedAt <= window.endedAt + )) ? 'completed' : 'forming' + return ( + openTeamWorkbench(resolvedSessionId, cardSnapshot.team.name) + : undefined} + > + + + ) + })() : null + ) : ( + + )} + {resolvedSessionId && cardsForItem.map((card) => { diff --git a/desktop/src/components/chat/RenderItemBoundary.test.tsx b/desktop/src/components/chat/RenderItemBoundary.test.tsx new file mode 100644 index 00000000..d5212e52 --- /dev/null +++ b/desktop/src/components/chat/RenderItemBoundary.test.tsx @@ -0,0 +1,73 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { render, screen } from '@testing-library/react' + +const { reportMock } = vi.hoisted(() => ({ + reportMock: vi.fn(async () => undefined), +})) + +vi.mock('../../lib/diagnosticsCapture', () => ({ + reportReactError: reportMock, +})) + +import { RenderItemBoundary } from './RenderItemBoundary' +import { useSettingsStore } from '../../stores/settingsStore' + +function Explodes(): never { + throw new Error('row exploded') +} + +describe('RenderItemBoundary', () => { + beforeEach(() => { + reportMock.mockClear() + useSettingsStore.setState({ locale: 'en' }) + // React logs every caught render error; keep the output readable. + vi.spyOn(console, 'error').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('renders its children untouched when nothing throws', () => { + render(fine) + + expect(screen.getByText('fine')).toBeTruthy() + expect(screen.queryByText(/couldn't be displayed/)).toBeNull() + expect(reportMock).not.toHaveBeenCalled() + }) + + // The point of the boundary: the failure stays where it happened. + it('replaces only the item that throws and leaves its neighbours alone', () => { + render( +
+ before + + after +
, + ) + + expect(screen.getByText('before')).toBeTruthy() + expect(screen.getByText('after')).toBeTruthy() + expect(screen.getByText(/couldn't be displayed/)).toBeTruthy() + }) + + it('still records the error in Diagnostics, with the component stack', () => { + render() + + expect(reportMock).toHaveBeenCalledTimes(1) + const [error, info] = reportMock.mock.calls[0] as unknown as [Error, { componentStack: string }] + expect(error.message).toBe('row exploded') + expect(info.componentStack).toContain('Explodes') + }) + + it('does not report a failed item again when its parent re-renders', () => { + const { rerender } = render() + expect(reportMock).toHaveBeenCalledTimes(1) + + rerender() + rerender() + + expect(reportMock).toHaveBeenCalledTimes(1) + expect(screen.getByText(/couldn't be displayed/)).toBeTruthy() + }) +}) diff --git a/desktop/src/components/chat/RenderItemBoundary.tsx b/desktop/src/components/chat/RenderItemBoundary.tsx new file mode 100644 index 00000000..42c8055f --- /dev/null +++ b/desktop/src/components/chat/RenderItemBoundary.tsx @@ -0,0 +1,48 @@ +import React from 'react' +import { TriangleAlert } from 'lucide-react' +import { t } from '../../i18n' +import { reportReactError } from '../../lib/diagnosticsCapture' + +type Props = { + children: React.ReactNode +} + +type State = { + failed: boolean +} + +/** + * Keeps one transcript item that fails to render from taking the whole app with it. + * + * The root ErrorBoundary replaces everything, and a transcript is rebuilt from saved + * history — so a single bad record (issue #1400: an AskUserQuestion input the model + * got wrong) crashed the app again on every launch, because the open tab is restored. + * Here the failure stays in its row: the rest of the conversation, the sidebar and the + * composer keep working, and the error still goes to Diagnostics. + * + * There is deliberately no automatic retry. A stored record fails the same way every + * time, and retrying on each render would report it on every update of a live list. + * The row is tried again whenever it is mounted afresh (switching session, reloading). + */ +export class RenderItemBoundary extends React.Component { + state: State = { failed: false } + + static getDerivedStateFromError(): State { + return { failed: true } + } + + componentDidCatch(error: unknown, errorInfo: React.ErrorInfo) { + void reportReactError(error, errorInfo) + } + + render() { + if (!this.state.failed) return this.props.children + + return ( +
+ + {t('errorBoundary.item')} +
+ ) + } +} diff --git a/desktop/src/i18n/locales/en.ts b/desktop/src/i18n/locales/en.ts index cf625384..3d69b9ca 100644 --- a/desktop/src/i18n/locales/en.ts +++ b/desktop/src/i18n/locales/en.ts @@ -1320,6 +1320,7 @@ Row 9, all 8 cells: continuing from straight down, turning left through lower-le 'settings.diagnostics.doctorNoKeys': 'None', 'errorBoundary.title': 'Something went wrong.', 'errorBoundary.description': 'The error was recorded in Diagnostics.', + 'errorBoundary.item': "This item couldn't be displayed. The error was recorded in Diagnostics.", // Settings > Claude Official Login 'settings.claudeOfficialLogin.intro': 'Using official Claude models requires signing in to your Claude.ai account. Click the button below to open the official Claude login page in your browser; you\'ll be returned here after authorizing.', diff --git a/desktop/src/i18n/locales/jp.ts b/desktop/src/i18n/locales/jp.ts index 1fb4f81e..7e7570b5 100644 --- a/desktop/src/i18n/locales/jp.ts +++ b/desktop/src/i18n/locales/jp.ts @@ -1321,6 +1321,7 @@ export const jp: Record = { 'settings.diagnostics.doctorNoKeys': 'なし', 'errorBoundary.title': '問題が発生しました。', 'errorBoundary.description': 'エラーは診断に記録されました。', + 'errorBoundary.item': 'この項目は表示できませんでした。エラーは診断に記録されました。', // Settings > Claude Official Login 'settings.claudeOfficialLogin.intro': '公式 Claude モデルを使用するには、Claude.ai アカウントにサインインする必要があります。下のボタンをクリックすると、ブラウザで公式 Claude ログインページが開きます。承認後、ここに戻ります。', diff --git a/desktop/src/i18n/locales/kr.ts b/desktop/src/i18n/locales/kr.ts index 89373906..dce664ce 100644 --- a/desktop/src/i18n/locales/kr.ts +++ b/desktop/src/i18n/locales/kr.ts @@ -1323,6 +1323,7 @@ export const kr: Record = { 'settings.diagnostics.doctorNoKeys': '없음', 'errorBoundary.title': '문제가 발생했습니다.', 'errorBoundary.description': '오류가 진단에 기록되었습니다.', + 'errorBoundary.item': '이 항목을 표시할 수 없습니다. 오류가 진단에 기록되었습니다.', // Settings > Claude Official Login 'settings.claudeOfficialLogin.intro': '공식 Claude 모델을 사용하려면 Claude.ai 계정에 로그인해야 합니다. 아래 버튼을 클릭하면 브라우저에서 공식 Claude 로그인 페이지가 열립니다. 승인 후 이곳으로 돌아옵니다.', diff --git a/desktop/src/i18n/locales/zh-TW.ts b/desktop/src/i18n/locales/zh-TW.ts index 27831c77..6f94b6b2 100644 --- a/desktop/src/i18n/locales/zh-TW.ts +++ b/desktop/src/i18n/locales/zh-TW.ts @@ -1320,6 +1320,7 @@ export const zh: Record = { 'settings.diagnostics.doctorNoKeys': '無', 'errorBoundary.title': '出現異常。', 'errorBoundary.description': '錯誤已記錄到診斷日誌。', + 'errorBoundary.item': '此則內容無法顯示。錯誤已記錄到診斷日誌。', // Settings > Claude Official Login 'settings.claudeOfficialLogin.intro': '使用官方 Claude 模型需要登入你的 Claude.ai 賬號。點選下方按鈕,瀏覽器會開啟 Claude 官方登入頁面,授權後自動回到這裡。', diff --git a/desktop/src/i18n/locales/zh.ts b/desktop/src/i18n/locales/zh.ts index 6cd47d2e..0646ff5b 100644 --- a/desktop/src/i18n/locales/zh.ts +++ b/desktop/src/i18n/locales/zh.ts @@ -1319,6 +1319,7 @@ export const zh: Record = { 'settings.diagnostics.doctorNoKeys': '无', 'errorBoundary.title': '出现异常。', 'errorBoundary.description': '错误已记录到诊断日志。', + 'errorBoundary.item': '此条内容无法显示。错误已记录到诊断日志。', // Settings > Claude Official Login 'settings.claudeOfficialLogin.intro': '使用官方 Claude 模型需要登录你的 Claude.ai 账号。点击下方按钮,浏览器会打开 Claude 官方登录页面,授权后自动回到这里。',