From df294fa4740d249a7dd81a3356c425e55a3f3f98 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 17 Aug 2026 12:05:17 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=A9=20refactor:=20Resolve=20Tool-Card?= =?UTF-8?q?=20State=20Once=20(#14934)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿงฉ refactor: Resolve Tool-Card State Once (AI-1810) Each tool card derived its state several times over โ€” the visible label from one expression, the `aria-live` announcement from another, the icon and shimmer from a third, and since #14906 the follow-scroll from a fourth. Nothing tied them together; they agreed only because each was written to agree. Thirteen of the seventeen review findings on #14873 were instances of one derivation being updated and another left behind, and #14892 added more. `resolveToolCallPhase` is now the single source: one function encoding the precedence rules, each of which a specific review finding established, returning `running | completed | cancelled | failed`. Everything the card shows reads that value. `ProgressText` takes `phase` in place of the `error` + `errorSuffix` pair, which encoded three terminal states in two booleans โ€” `error` meant cancelled, a present `errorSuffix` meant failed โ€” and made every consumer reconstruct the distinction. That shape is precisely what let a duration render beside "failed" (Codex round 1 on #14892). Two things fell out once the state had one home, both dead code rather than deletions of behaviour: - `progress` left `ProgressText` entirely; the phase already carries everything it was used to decide. - The `useProgress` mask went with it. Passing 1 in still matters โ€” it stops the 200ms interval โ€” but masking the output no longer does, because the phase treats an explicit close as terminal outright. The "both halves are load-bearing" subtlety is now one half. Scope: the nine cards that render the shared `ProgressText`. The three with bespoke layouts (`WebSearch`, `SubagentCall`, `OpenAIImageGen`) still resolve their own state and are the natural follow-up โ€” they can adopt the resolver without adopting the component. Refactor-only. 4891/4891 client tests pass unchanged, including the suites that encode the cancelled/failed precedence in both directions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * ๐Ÿ› fix: Infer Cancellation From Reported Progress, Not The Animation `useProgress` holds below 1 for ~200ms after a call reports completion: it emits the previous value, then `0.99`, then `1` on a timeout. The resolver read that animated value for its cancellation inference, so a successful call whose submission ended inside that window rendered โ€” and announced โ€” as "Cancelled". The input is now split. `reportedProgress` is what the stream said and drives the inference; `displayProgress` is the animated value and drives `running` vs `completed`, so the label and shimmer still follow the animation rather than snapping. This restores `ToolCall` and `RetrievalCall`, whose previous predicates used `initialProgress` and were immune, and additionally fixes `useToolCallState`, which inferred from `rawProgress` and therefore carried the bug already โ€” every card the hook backs was exposed to it before this PR. Three tests cover the window: a reported-complete call mid-settle is `running`, a genuinely unfinished one is still `cancelled`, and the card settles to `completed` without a cancelled frame in between. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * ๐Ÿงน chore: Drop Unused Phase Predicates; Correct A Stale Comment `isFailedPhase` and `isRunningPhase` had no callers โ€” every consumer compares the phase directly, which reads better than a wrapper. An unused abstraction is the thing this PR argues against, so it should not ship one. The comment above the hook's resolver call still described "the raw progress the legacy heuristic was written against", which stopped being true when the input split into reported and display progress. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 --------- Co-authored-by: Claude --- .../Chat/Messages/Content/CodeAnalyze.tsx | 18 ++- .../Chat/Messages/Content/Parts/BashCall.tsx | 44 ++--- .../Messages/Content/Parts/ExecuteCode.tsx | 44 ++--- .../Content/Parts/FileAuthoringCall.tsx | 29 ++-- .../Messages/Content/Parts/ReadFileCall.tsx | 18 ++- .../Chat/Messages/Content/Parts/SkillCall.tsx | 18 ++- .../Content/Parts/__tests__/BashCall.test.tsx | 12 +- .../__tests__/FileAuthoringCall.test.tsx | 14 +- .../Content/Parts/useToolCallState.ts | 81 ++++++---- .../Chat/Messages/Content/ProgressText.tsx | 56 +++---- .../Chat/Messages/Content/RetrievalCall.tsx | 45 +++--- .../Chat/Messages/Content/ToolCall.tsx | 42 ++--- .../Content/__tests__/ProgressText.test.tsx | 20 +-- .../src/utils/__tests__/toolCallPhase.spec.ts | 153 ++++++++++++++++++ client/src/utils/index.ts | 1 + client/src/utils/toolCallPhase.ts | 88 ++++++++++ 16 files changed, 483 insertions(+), 200 deletions(-) create mode 100644 client/src/utils/__tests__/toolCallPhase.spec.ts create mode 100644 client/src/utils/toolCallPhase.ts diff --git a/client/src/components/Chat/Messages/Content/CodeAnalyze.tsx b/client/src/components/Chat/Messages/Content/CodeAnalyze.tsx index dd80e95d78..26176ba410 100644 --- a/client/src/components/Chat/Messages/Content/CodeAnalyze.tsx +++ b/client/src/components/Chat/Messages/Content/CodeAnalyze.tsx @@ -1,6 +1,7 @@ import { useState, useEffect } from 'react'; import { useRecoilValue } from 'recoil'; import { Terminal } from 'lucide-react'; +import type { ToolCallPhase } from '~/utils/toolCallPhase'; import { useProgress, useLocalize } from '~/hooks'; import ProgressText from './ProgressText'; import MarkdownLite from './MarkdownLite'; @@ -46,14 +47,22 @@ export default function CodeAnalyze({ return acc; }, ''); + /** + * The legacy assistants-endpoint card: it never receives run-step metadata, + * so it genuinely has only these two states and maps them directly rather + * than through `resolveToolCallPhase`, which needs signals this card has no + * access to. The announcement and the icon below read this same value. + */ + const phase: ToolCallPhase = progress < 1 ? 'running' : 'completed'; + return ( <> - {progress < 1 ? localize('com_ui_analyzing') : localize('com_ui_analyzing_finished')} + {phase === 'running' ? localize('com_ui_analyzing') : localize('com_ui_analyzing_finished')}