diff --git a/changelog.d/chat-run-fold-first-part.md b/changelog.d/chat-run-fold-first-part.md new file mode 100644 index 0000000000..731d271d1d --- /dev/null +++ b/changelog.d/chat-run-fold-first-part.md @@ -0,0 +1,3 @@ +### Fixed +- A chat work block you folded no longer opens again when the words before its tools join it, as + when a later Codex record marks them commentary or a blank commentary is filled in. diff --git a/scripts/chat-fallback-browser-qa.ts b/scripts/chat-fallback-browser-qa.ts index 179b765273..f79b5f2bc7 100644 --- a/scripts/chat-fallback-browser-qa.ts +++ b/scripts/chat-fallback-browser-qa.ts @@ -249,6 +249,29 @@ try { assert.deepEqual(await opened(), [false, false, false], "a run that splits moves no fold to another block: the folded run stays folded"); await headRows.waitFor({ state: "hidden" }); if (process.env.UI_EVIDENCE_DIR) await page.screenshot({ path: join(process.env.UI_EVIDENCE_DIR, "claude-run-split.png") }); + // Words before a run's tools join it later (a Codex record marks them commentary): the run + // keeps the fold the reader gave it (#2063). A history without an ID keeps each row's memory + // by its place, so the turn sits on row 5, which the four rows above left with none. + const sorting = (words: { kind: "text"; text: string; phase?: "commentary" }): ConversationTurn[] => [ + ...shelves("Shelf a is empty."), + { role: "user", ts: null, parts: [said("Sort the shelves.")] }, + { role: "assistant", ts: null, parts: [words, bash("sort a")] }, + ]; + turns = sorting(said("Sorting shelf a.")); + await page.evaluate(() => window.chatFallbackQA.refresh()); + await page.getByText("Sorting shelf a.", { exact: true }).waitFor(); + assert.deepEqual(await rows(), ["Sorting shelf a.", "work Working\u2026 \u00b7 1 command"], "the words stand above the working run"); + assert.deepEqual(await opened(), [true], "the working run is open"); + await heads.last().click(); + await headRows.waitFor({ state: "hidden" }); + assert.deepEqual(await opened(), [false], "the reader folds the working run"); + turns = sorting({ ...said("Sorting shelf a."), phase: "commentary" }); + await page.evaluate(() => window.chatFallbackQA.refresh()); + // drawn once the words left the turn for its run of work + await page.waitForFunction(() => [...[...document.querySelectorAll(".chat-turn-agent")].at(-1)!.children].every((child) => child.classList.contains("work-block"))); + assert.deepEqual(await opened(), [false], "words that join the run keep it folded"); + assert.equal(await page.getByText("Sorting shelf a.", { exact: true }).isVisible(), false, "and fold with it"); + if (process.env.UI_EVIDENCE_DIR) await page.screenshot({ path: join(process.env.UI_EVIDENCE_DIR, "run-words-join-fold.png") }); await page.evaluate(() => window.chatFallbackQA.status(undefined)); await live.waitFor({ state: "detached" }); console.log("PASS words and runs of work keep the agent's order, settled and live"); diff --git a/src/components/ChatTimeline.tsx b/src/components/ChatTimeline.tsx index eca5f3b769..f876b54520 100644 --- a/src/components/ChatTimeline.tsx +++ b/src/components/ChatTimeline.tsx @@ -151,7 +151,7 @@ export function ChatTimeline({ rows, scope, source, const rendered = useCallback(({ item, index }: { item: Row; index: number }) => { let memory = memories.get(item.key); if (!memory) { - memory = { toggles: new Map(), outputs: new Map() }; + memory = { toggles: new Map(), outputs: new Map(), runs: new Map() }; memories.set(item.key, memory); } return diff --git a/src/components/ChatTurn.test.ts b/src/components/ChatTurn.test.ts index cfed6670f9..addde23cc8 100644 --- a/src/components/ChatTurn.test.ts +++ b/src/components/ChatTurn.test.ts @@ -1,8 +1,10 @@ -import { describe, expect, it } from "bun:test"; -import { createElement } from "react"; +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { act, createElement } from "react"; import { renderToStaticMarkup } from "react-dom/server"; +import { create, type ReactTestRenderer } from "react-test-renderer"; import type { ConversationPart, ConversationTurn } from "../../shared/protocol.ts"; +import { ChatRowMemoryContext, type ChatRowMemory } from "../lib/chatRowState.tsx"; import { SettingsProvider } from "../lib/settings.ts"; import { Turn } from "./ChatView.tsx"; @@ -55,3 +57,83 @@ describe("an agent turn's work blocks", () => { expect(html.indexOf("Worked for 1m")).toBeLessThan(html.indexOf("Done.")); }); }); + +describe("an agent turn's folded work, drawn again", () => { + const commentary = (value: string): ConversationPart => ({ kind: "text", text: value, phase: "commentary" }); + const answer = (value: string): ConversationPart => ({ kind: "text", text: value, phase: "final_answer" }); + const descriptors = new Map(); + let memory: ChatRowMemory; + let renderer: ReactTestRenderer | undefined; + + const view = (parts: ConversationPart[]) => createElement(ChatRowMemoryContext.Provider, { value: memory, children: createElement(SettingsProvider, { children: createElement(Turn, { + paneId: "w1:p1", turn: { role: "assistant", ts: "2026-01-01T12:00:00Z", parts }, rowKey: "turn:1", live: true, waiting: false, showThinking: false, + }) }) }); + const draw = async (parts: ConversationPart[]) => { + await act(async () => { if (renderer) renderer.update(view(parts)); else renderer = create(view(parts)); }); + }; + const heads = () => renderer!.root.findAll((node) => node.props.className === "work-block-head"); + const opened = () => heads().map((node) => node.props["aria-expanded"] as boolean); + const toggle = async (index: number) => { await act(async () => { heads()[index]!.props.onClick(); }); }; + // the row scrolled out of the list and back: only its memory is left + const remount = async (parts: ConversationPart[]) => { + await act(async () => { renderer!.unmount(); }); + renderer = undefined; + await draw(parts); + }; + + beforeEach(() => { + for (const key of ["window", "document", "getComputedStyle", "IS_REACT_ACT_ENVIRONMENT"]) descriptors.set(key, Object.getOwnPropertyDescriptor(globalThis, key)); + descriptors.set("languages", Object.getOwnPropertyDescriptor(navigator, "languages")); + const listeners = { addEventListener() {}, removeEventListener() {} }; + Object.defineProperty(globalThis, "window", { configurable: true, value: { ...listeners, matchMedia: () => ({ matches: false, ...listeners }) } }); + Object.defineProperty(globalThis, "document", { configurable: true, value: { ...listeners, querySelector: () => null, documentElement: { dataset: {}, style: { setProperty() {}, removeProperty() {} } } } }); + Object.defineProperty(globalThis, "getComputedStyle", { configurable: true, value: () => ({ getPropertyValue: () => "" }) }); + Object.defineProperty(globalThis, "IS_REACT_ACT_ENVIRONMENT", { configurable: true, value: true }); + Object.defineProperty(navigator, "languages", { configurable: true, value: ["en"] }); + memory = { toggles: new Map(), outputs: new Map(), runs: new Map() }; + }); + + afterEach(async () => { + if (renderer) await act(async () => { renderer!.unmount(); }); + renderer = undefined; + for (const [key, descriptor] of descriptors) { + const target = key === "languages" ? navigator : globalThis; + if (descriptor) Object.defineProperty(target, key, descriptor); + else Reflect.deleteProperty(target, key); + } + }); + + it("keeps a block folded when the words before its tools join it (#2063)", async () => { + // a later Codex record marks the words commentary; an empty commentary record is filled in + for (const [before, after] of [[text("Checking."), commentary("Checking.")], [commentary(""), commentary("Checking.")]] as const) { + memory = { toggles: new Map(), outputs: new Map(), runs: new Map() }; + await draw([before, tool("ls")]); + await toggle(0); + expect(opened()).toEqual([false]); + await draw([after, tool("ls")]); + expect(opened()).toEqual([false]); + await remount([after, tool("ls")]); + expect(opened()).toEqual([false]); + await act(async () => { renderer!.unmount(); }); + renderer = undefined; + } + }); + + it("keeps a merged block's fold when a part leaves it", async () => { + await draw([tool("ls"), text("One shelf."), commentary("Checking it.")]); + expect(opened()).toEqual([false, true]); + // the reader folds the working run and opens it again + await toggle(1); + await toggle(1); + // the words between become commentary: one run, which the reader folds + await draw([tool("ls"), commentary("One shelf."), commentary("Checking it.")]); + expect(opened()).toEqual([true]); + await toggle(0); + expect(opened()).toEqual([false]); + // its last words become the answer and leave it + await draw([tool("ls"), commentary("One shelf."), answer("Checking it.")]); + expect(opened()).toEqual([false]); + await remount([tool("ls"), commentary("One shelf."), answer("Checking it.")]); + expect(opened()).toEqual([false]); + }); +}); diff --git a/src/components/ChatView.tsx b/src/components/ChatView.tsx index 8e4e08734d..064b88ce81 100644 --- a/src/components/ChatView.tsx +++ b/src/components/ChatView.tsx @@ -32,7 +32,7 @@ import { copyText } from "../lib/clipboard.ts"; import { attachmentFileName, mentionFileName, type GalleryImage } from "../lib/imageViewer.ts"; import { useMachineApi } from "../lib/machineContext.tsx"; import { toTranscriptMessages, type TranscriptMessage } from "../lib/transcript.ts"; -import { isLiveWorkTurn, isWaitingWorkTurn, formatWorkDuration, splitTurn, turnSegments, type WorkSegment, todoGroups, visibleWorkParts, workFailed, workStartsOpen, workSummary, type TodoAttempt, type TodoGroup, type ToolPart as ToolPartType } from "../lib/workBlocks.ts"; +import { isLiveWorkTurn, isWaitingWorkTurn, formatWorkDuration, runNames, splitTurn, turnSegments, type WorkSegment, todoGroups, visibleWorkParts, workFailed, workStartsOpen, workSummary, type TodoAttempt, type TodoGroup, type ToolPart as ToolPartType } from "../lib/workBlocks.ts"; import { phaseRows, planRows, taskRows, todoRows, type ChecklistRow } from "../lib/checklist.ts"; import { isTodoTool, parseTodoAnswer, todoCallSummary, type TodoItem, type TodoStatus } from "../lib/todos.ts"; import { formatGoalTime, turnGoal, type GoalState, type GoalStatus } from "../lib/goals.ts"; @@ -1014,6 +1014,15 @@ function noticeLabel(t: ReturnType, notice: Extract | null>(null); + named.current = null; + useLayoutEffect(() => { + if (memory === null || named.current === null) return; + memory.runs.clear(); + for (const [index, name] of named.current) memory.runs.set(index, name); + }); // under the turn, not in its meta row: that row fades out once the pointer and focus leave const [copyFailed, setCopyFailed] = useChatRowToggle("copy-failed", false); const copyError = copyFailed &&

{t("Couldn't copy. Select the text and copy it manually.")}

; @@ -1083,12 +1092,14 @@ export const Turn = memo(function Turn({ paneId, turn, rowKey, live, waiting, sh // The runs before it have the agent's words after them: they are done, read Worked and fold. const head = runs.filter((run) => visibleWorkParts(run.parts, showThinking).length > 0).at(-1) ?? runs.at(-1); const work = turnWork(turn.parts, rowKey); + // a run keeps its block and fold while its parts change (runNames); the stand-in is named by the + // part that will start the turn's first run, and is drawn only while the turn has none + const names = runNames(runs, work.at, memory?.runs ?? new Map()); + named.current = new Map(runs.flatMap((run, index) => run.parts.map((part) => [work.at.get(part)!, names[index]!] as const))); // `null`: the head standing in while the running turn has no work yet const block = (run: WorkSegment | null) => { const isHead = run === null || run === head; - // a run is named by its first part, so it keeps its block and fold when an earlier run splits - // or grows; the stand-in by the part that will start the turn's first run, which takes its place - const key = `work:${run?.start ?? turn.parts.length}`; + const key = run === null ? `work:${turn.parts.length}` : names[runs.indexOf(run)]!; return ; readonly outputs: Map; + /** an agent turn's part index → the name of the run of work it was in when last drawn (runNames) */ + readonly runs: Map; } export const ChatRowMemoryContext = createContext(null); diff --git a/src/lib/workBlocks.test.ts b/src/lib/workBlocks.test.ts index 0e833e0ee4..1f544de4c6 100644 --- a/src/lib/workBlocks.test.ts +++ b/src/lib/workBlocks.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "bun:test"; -import { formatWorkDuration, splitTurn, todoGroups, turnSegments, visibleWorkParts, workFailed, workStartsOpen, workSummary, type ToolPart } from "./workBlocks.ts"; +import { formatWorkDuration, runNames, splitTurn, todoGroups, turnSegments, visibleWorkParts, workFailed, workStartsOpen, workSummary, type ToolPart } from "./workBlocks.ts"; import type { ConversationPart } from "../../shared/protocol.ts"; const tool = (name: string, summary = name): Extract => ({ kind: "tool", name, summary, input: "{}", output: "" }); @@ -76,6 +76,30 @@ describe("turnSegments", () => { }); }); +describe("runNames", () => { + // what Turn keeps between draws: each part's run name, by part index + const draw = (parts: ConversationPart[], previous: ReadonlyMap) => { + const runs = turnSegments(parts).flatMap((segment) => segment.kind === "work" ? [segment] : []); + const names = runNames(runs, new Map(parts.map((part, index) => [part, index])), previous); + return { names, next: new Map(runs.flatMap((run, index) => run.parts.map((part) => [parts.indexOf(part), names[index]!] as const))) }; + }; + + it("names a run by its first part the first time, and keeps the name while its parts change", () => { + const narration = { ...text("Checking."), phase: "commentary" as const }; + const first = draw([text("Checking."), tool("ls")], new Map()); + expect(first.names).toEqual(["work:1"]); + expect(draw([narration, tool("ls")], first.next).names).toEqual(["work:1"]); + }); + + it("gives a split run's first part the old name and never hands a shown name to a new run", () => { + const shown = draw([tool("A"), text(""), tool("B"), text("between"), tool("C")], new Map()); + expect(shown.names).toEqual(["work:0", "work:4"]); + expect(draw([tool("A"), text("arrived"), tool("B"), text("between"), tool("C")], shown.next).names).toEqual(["work:0", "work:2", "work:4"]); + // the run named work:0 now starts at 2; a new run at 0 must not take its name + expect(draw([tool("new"), text("between"), tool("A")], new Map([[2, "work:0"]])).names).toEqual(["work:0+", "work:0"]); + }); +}); + describe("workStartsOpen", () => { const commentary = (value: string) => ({ ...text(value), phase: "commentary" as const }); const final = (value: string) => ({ ...text(value), phase: "final_answer" as const }); diff --git a/src/lib/workBlocks.ts b/src/lib/workBlocks.ts index 7ca981caa0..318f3e3ca0 100644 --- a/src/lib/workBlocks.ts +++ b/src/lib/workBlocks.ts @@ -49,6 +49,30 @@ export function turnSegments(parts: ConversationPart[]): TurnSegment[] { return segments; } +/** + * The name of each run's block, which also keeps its fold. A run keeps its name while its parts + * change: words before its tools join it (a record marks them commentary, a blank one is filled + * in), a part leaves it, or it splits or merges with another run (#2063). `previous` holds the name + * of the run each part was in when the turn was last drawn, by part index. A run takes the first + * of those names among its own parts that no earlier run has taken. Only then does a run with none + * get `work:`, so a new run never takes the name of a run already shown. + */ +export function runNames(runs: readonly WorkSegment[], at: ReadonlyMap, previous: ReadonlyMap): string[] { + const used = new Set(); + const kept = runs.map((run) => { + const name = run.parts.map((part) => previous.get(at.get(part)!)).find((name) => name !== undefined && !used.has(name)); + if (name !== undefined) used.add(name); + return name; + }); + return runs.map((run, index) => { + if (kept[index] !== undefined) return kept[index]; + let name = `work:${run.start}`; + while (used.has(name)) name += "+"; + used.add(name); + return name; + }); +} + /** * A work block is open while its turn runs and folded once the turn settles: the answer stays * outside the fold, so a finished turn reads as its answer under one "Worked for" row.