Skip to content

Keep the final answer visible when a Goal reloads - #510

Merged
xintaofei merged 2 commits into
xintaofei:mainfrom
Adam-Dalloul:fix/goal-keeps-final-text
Aug 21, 2026
Merged

Keep the final answer visible when a Goal reloads#510
xintaofei merged 2 commits into
xintaofei:mainfrom
Adam-Dalloul:fix/goal-keeps-final-text

Conversation

@Adam-Dalloul

Copy link
Copy Markdown
Contributor

If a /goal is still active, Codeg folds the rest of the turn into the Goal chip. That chip starts collapsed, so after a reload you just see Goal: … plus New files / Files changed, and the wrap-up is gone until you expand the chip.

This lifts trailing prose out of a settled unfinished goal so the answer stays under the chip. A live goal that still has process text starts open.

Reload a chat that used /goal and didn't close it. The last assistant message should still be there, not only the Goal pill.

An unfinished /goal stays active without a closing update_goal. On
reload the whole turn was folded into the collapsed Goal chip, so the
wrap-up disappeared. Lift trailing prose out of a settled unfinished
run, and open a live goal that still has process text.
@xintaofei

Copy link
Copy Markdown
Owner

Thanks for this — nice catch, and the diagnosis is spot on. A /goal that codex never closes really does swallow the rest of the turn into a chip that starts collapsed, and lifting the trailing prose out once the run settles is the right place to fix it.

I went through it fairly carefully and it holds up:

  • The pushGoalRun refactor is behavior-faithful. Only flushActive() ever passes end === null, so shouldLiftTrailing can only become true for an unfinished run that has settled — the other three call sites keep the old behavior exactly. And AdaptedGoalRunPart has precisely the five fields the explicit construction writes, so swapping {...part} for an explicit object doesn't drop anything.
  • body / trailing are disjoint slices pushed in order, so no duplication, loss or reordering — including through the merged-sub-turn pass in message-list-view.tsx and the stale / cross-turn branches of groupGoalRuns.
  • The GoalCard change does behave the way you describe in the live path: when prose starts after a completed tool the runtime splits a new assistant sub-turn, mergeConsecutiveAssistantTurns re-keys the row to merged-…, and the card remounts with populated items and opens.

Checks on your branch (8b32de7): vitest 315 files / 4252 tests green, tsc --noEmit clean, eslint clean on the four touched files.

No blocking issues — I'll merge this shortly. 🙌

Three small non-blocking notes, purely as possible follow-ups; please don't hold this PR for them:

  1. bodyOpen is mount-initialized, so the auto-open depends on that remount happening rather than on the data itself. A same-key incremental update won't open it, and since the thread is virtualized (Virtualizer with bufferSize and no keepMounted), scrolling a live goal off-screen and back re-runs the initializer. Deriving it would make the default deterministic while still letting the user override:

    const [userOpen, setUserOpen] = useState<boolean | null>(null)
    const bodyOpen = userOpen ?? (isError || (isRunning && items.length > 0))
  2. A test that mounts with items: [] and then rerenders with items would pin that contract — the current one mounts already-populated, so it can't catch the incremental case.

  3. splitTrailingTextParts is a text allowlist, so a single non-text tail part stops the backward scan and the answer just before it stays inside the chip (same for a trailing proposed-plan / generated-image). Reasonable to leave out of scope here.

Thanks again for the clear write-up and the repro steps — made this easy to verify.

@Adam-Dalloul

Copy link
Copy Markdown
Contributor Author

Sounds good, thanks.

@xintaofei

Copy link
Copy Markdown
Owner

Follow-up on my review above — I went ahead and wrote the three non-blocking notes up as an actual patch rather than leaving them as homework. All three are addressed, with tests that fail without the fix:

1. bodyOpen is now derived instead of mount-seeded. A useState seed is evaluated once, and a goal run always mounts before its body exists (create_goal is adapted on its own, so the run starts with items: []). That left the default hostage to whether the row happened to remount later — it does on the sub-turn merge (merged-${first.key}) and on virtualizer recycling, but not on a plain same-key update. Now:

const [userOpen, setUserOpen] = useState<boolean | null>(null)
const bodyOpen = userOpen ?? (isError || (isRunning && items.length > 0))

The default follows the data — a live run opens as soon as it holds anything, settling collapses it again (its answer has been lifted out by then), a late error still opens — and a manual toggle wins once set.

2. Tests for the incremental shape. Three new cases in goal-tool-call.test.tsx: mount empty then rerender with items (opens with no remount), manual collapse survives later body updates, and settling re-collapses the capsule. The first and third fail against the current useState seed, so they actually pin the contract.

3. The lift now covers every part that is the answer, not just text. splitTrailingTextParts became splitTrailingAnswerParts, driven by GOAL_ANSWER_PART_TYPES = {text, proposed-plan, generated-image} — a Plan-mode document and a generated image were still buried in the collapsed capsule, same bug class. Process parts (tool calls/results/groups, reasoning, todo plans, delegation and background-task polls) stay inside, and I kept the backward-scan shape on purpose: an answer part sitting before a process part stays put, because prose followed by more work is a mid-run note and lifting it would reorder the reply. That case is covered by a test too.

Gates on top of your commit: vitest 315 files / 4258 tests green, tsc --noEmit clean, eslint + prettier --check clean on the four files.

One logistics note — I couldn't push this to your branch directly because Allow edits by maintainers is off on this PR, so the patch is below. Either flip that checkbox and I'll push it, or git am it yourself, or say the word and I'll land it as a separate commit right after merging. Whatever's easiest for you.

git am-able patch (4 files, +212/-15)
From 906be0736e039548b32469ff4c3e769a42136138 Mon Sep 17 00:00:00 2001
From: xintaofei <itpkcn@gmail.com>
Date: Fri, 21 Aug 2026 08:16:50 +0800
Subject: [PATCH] fix(chat): derive the Goal card's open state and lift every
 trailing answer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The card seeded `bodyOpen` at mount, but a goal run always mounts before
its body exists (`create_goal` is adapted on its own, so the run starts
with `items: []`). That left the default hostage to whether the row
happened to remount later — it does on the sub-turn merge and on
virtualizer recycling, but not on a plain same-key update. Derive it from
the data instead, with an explicit user override that wins once set.

Widen the settled-run lift from `text` to every part that IS the answer:
a Plan-mode document and a generated image were still buried in the
collapsed capsule. Process parts (tools, reasoning, todo plans, polls)
stay inside, and answer parts sitting before a process part stay put so
the reply is never reordered.
---
 .../message/goal-tool-call.test.tsx           | 100 ++++++++++++++++++
 src/components/message/goal-tool-call.tsx     |  17 ++-
 src/lib/adapters/ai-elements-adapter.test.ts  |  67 ++++++++++++
 src/lib/adapters/ai-elements-adapter.ts       |  39 +++++--
 4 files changed, 208 insertions(+), 15 deletions(-)

diff --git a/src/components/message/goal-tool-call.test.tsx b/src/components/message/goal-tool-call.test.tsx
index 25ab5c1e..73dd3cfb 100644
--- a/src/components/message/goal-tool-call.test.tsx
+++ b/src/components/message/goal-tool-call.test.tsx
@@ -5,6 +5,10 @@ import { describe, expect, it } from "vitest"
 
 import { GoalRunPart, GoalToolCallPart } from "./goal-tool-call"
 import { GoalControlProvider } from "./goal-control-context"
+import type {
+  AdaptedContentPart,
+  AdaptedGoalRunPart,
+} from "@/lib/adapters/ai-elements-adapter"
 import enMessages from "@/i18n/messages/en.json"
 import zhMessages from "@/i18n/messages/zh-CN.json"
 
@@ -20,6 +24,25 @@ function renderWithIntl(
   )
 }
 
+function runningGoalRun(items: AdaptedContentPart[]): AdaptedGoalRunPart {
+  return {
+    type: "goal-run",
+    start: {
+      type: "tool-call",
+      toolCallId: "call-create-goal",
+      toolName: "create_goal",
+      input: JSON.stringify({ objective: "Analyze README file" }),
+      state: "output-available",
+    },
+    end: null,
+    items,
+    isRunning: true,
+  }
+}
+
+const renderTextPart = (part: AdaptedContentPart, key: string) =>
+  part.type === "text" ? <div key={key}>{part.text}</div> : null
+
 describe("GoalToolCallPart", () => {
   it("renders Codex goal completion as a compact goal card", () => {
     renderWithIntl(
@@ -93,6 +116,83 @@ describe("GoalToolCallPart", () => {
     expect(screen.queryByText("Reading README.md")).not.toBeInTheDocument()
   })
 
+  it("opens a live goal when its body arrives without a remount", () => {
+    // The production mount order: `create_goal` is adapted on its own, so the
+    // card first renders with an EMPTY body and the process content streams in
+    // afterwards under the same key. A mount-time seed would miss this.
+    const { rerender } = renderWithIntl(
+      <GoalRunPart part={runningGoalRun([])} renderPart={renderTextPart} />
+    )
+
+    expect(screen.queryByText("Reading README.md")).not.toBeInTheDocument()
+
+    rerender(
+      <NextIntlClientProvider locale="en" messages={enMessages}>
+        <GoalRunPart
+          part={runningGoalRun([{ type: "text", text: "Reading README.md" }])}
+          renderPart={renderTextPart}
+        />
+      </NextIntlClientProvider>
+    )
+
+    expect(screen.getByText("Reading README.md")).toBeInTheDocument()
+  })
+
+  it("keeps a manual collapse across later body updates", () => {
+    const { rerender } = renderWithIntl(
+      <GoalRunPart
+        part={runningGoalRun([{ type: "text", text: "Reading README.md" }])}
+        renderPart={renderTextPart}
+      />
+    )
+
+    fireEvent.click(screen.getByRole("button"))
+    expect(screen.queryByText("Reading README.md")).not.toBeInTheDocument()
+
+    rerender(
+      <NextIntlClientProvider locale="en" messages={enMessages}>
+        <GoalRunPart
+          part={runningGoalRun([
+            { type: "text", text: "Reading README.md" },
+            { type: "text", text: "Reading CLAUDE.md" },
+          ])}
+          renderPart={renderTextPart}
+        />
+      </NextIntlClientProvider>
+    )
+
+    // The user's choice wins over the derived default.
+    expect(screen.queryByText("Reading README.md")).not.toBeInTheDocument()
+    expect(screen.queryByText("Reading CLAUDE.md")).not.toBeInTheDocument()
+  })
+
+  it("collapses the capsule again once the run settles", () => {
+    // Settling lifts the answer out of the run (see `groupGoalRuns`), so the
+    // capsule folds back to a status chip without hiding anything.
+    const { rerender } = renderWithIntl(
+      <GoalRunPart
+        part={runningGoalRun([{ type: "text", text: "Reading README.md" }])}
+        renderPart={renderTextPart}
+      />
+    )
+
+    expect(screen.getByText("Reading README.md")).toBeInTheDocument()
+
+    rerender(
+      <NextIntlClientProvider locale="en" messages={enMessages}>
+        <GoalRunPart
+          part={{
+            ...runningGoalRun([{ type: "text", text: "Reading README.md" }]),
+            isRunning: false,
+          }}
+          renderPart={renderTextPart}
+        />
+      </NextIntlClientProvider>
+    )
+
+    expect(screen.queryByText("Reading README.md")).not.toBeInTheDocument()
+  })
+
   it("shows active status for wrapper-prefixed create_goal names", () => {
     renderWithIntl(
       <GoalRunPart
diff --git a/src/components/message/goal-tool-call.tsx b/src/components/message/goal-tool-call.tsx
index cdb157c7..337e379f 100644
--- a/src/components/message/goal-tool-call.tsx
+++ b/src/components/message/goal-tool-call.tsx
@@ -225,9 +225,18 @@ function GoalCard({
     Boolean(startPart.errorText) ||
     endPart?.state === "output-error" ||
     Boolean(endPart?.errorText)
-  const [bodyOpen, setBodyOpen] = useState(
-    isError || (isRunning && items.length > 0)
-  )
+  // Derived, not mount-initialised: a `useState` seed is evaluated once, and
+  // the card always mounts BEFORE its body exists (create_goal is adapted on
+  // its own, so the run starts with `items: []`). Seeding would leave the
+  // default hostage to whether the row happens to remount later — it does on
+  // the sub-turn merge and on virtualizer recycling, but not on a plain
+  // same-key update. Deriving keeps the DEFAULT a function of the data:
+  // a live run opens as soon as it holds anything, settling collapses it
+  // again (its answer has been lifted out by then), and an error opens even
+  // when it lands late. `userOpen` is the manual override and wins once set,
+  // so any of those defaults yields to a deliberate toggle.
+  const [userOpen, setUserOpen] = useState<boolean | null>(null)
+  const bodyOpen = userOpen ?? (isError || (isRunning && items.length > 0))
   const goal = useMemo(
     () => parseGoal(startPart, endPart),
     [startPart, endPart]
@@ -263,7 +272,7 @@ function GoalCard({
     (normalizedStatus === "active" || normalizedStatus === "paused")
 
   return (
-    <Collapsible open={bodyOpen} onOpenChange={setBodyOpen} className="w-full">
+    <Collapsible open={bodyOpen} onOpenChange={setUserOpen} className="w-full">
       <CollapsibleTrigger
         className={cn(
           "group inline-flex max-w-full items-center gap-1.5 rounded-full px-3.5 py-2 text-xs font-medium transition-colors",
diff --git a/src/lib/adapters/ai-elements-adapter.test.ts b/src/lib/adapters/ai-elements-adapter.test.ts
index e03415a0..fdf9605f 100644
--- a/src/lib/adapters/ai-elements-adapter.test.ts
+++ b/src/lib/adapters/ai-elements-adapter.test.ts
@@ -507,6 +507,73 @@ describe("groupGoalRuns", () => {
     expect(out[1]).toEqual(text)
   })
 
+  it("lifts a trailing proposed plan and generated image too", () => {
+    // A Plan-mode document and a generated image ARE the turn's answer; a
+    // collapsed capsule must not hide them either. Reasoning is process and
+    // stays inside.
+    const proposedPlan: AdaptedContentPart = {
+      type: "proposed-plan",
+      markdown: "## Plan\n1. do the thing",
+      isStreaming: false,
+    }
+    const generatedImage: AdaptedContentPart = {
+      type: "generated-image",
+      revisedPrompt: null,
+      image: null,
+      status: "completed",
+    }
+    const reasoning: AdaptedContentPart = {
+      type: "reasoning",
+      content: "thinking about it",
+      isStreaming: false,
+    }
+
+    const out = groupGoalRuns(
+      [poll("create_goal"), reasoning, text, proposedPlan, generatedImage],
+      false
+    )
+
+    expect(out.map((p) => p.type)).toEqual([
+      "goal-run",
+      "text",
+      "proposed-plan",
+      "generated-image",
+    ])
+    expect(goalRunOf(out[0]).items).toEqual([reasoning])
+    expect(out.slice(1)).toEqual([text, proposedPlan, generatedImage])
+  })
+
+  it("keeps mid-run prose inside a settled unfinished goal", () => {
+    // Prose followed by more work is a mid-run note, not a wrap-up: lifting it
+    // would reorder the reply, so the scan stops at the last process part.
+    const midRunNote: AdaptedContentPart = { type: "text", text: "checking" }
+    const out = groupGoalRuns(
+      [poll("create_goal"), midRunNote, poll("exec_command")],
+      false
+    )
+
+    expect(out.map((p) => p.type)).toEqual(["goal-run"])
+    expect(goalRunOf(out[0]).items.map((p) => p.type)).toEqual([
+      "text",
+      "tool-call",
+    ])
+  })
+
+  it("keeps the answer inside a live unfinished goal", () => {
+    // While streaming, the card holds the answer and opens itself; lifting
+    // mid-stream would make the prose jump out and back as tools interleave.
+    const proposedPlan: AdaptedContentPart = {
+      type: "proposed-plan",
+      markdown: "## Plan",
+      isStreaming: false,
+    }
+    const out = groupGoalRuns([poll("create_goal"), text, proposedPlan], true)
+
+    expect(out.map((p) => p.type)).toEqual(["goal-run"])
+    expect(goalRunOf(out[0]).items).toEqual([text, proposedPlan])
+    expect(goalRunOf(out[0]).isRunning).toBe(true)
+  })
+
   it("does not mutate a reopened unfinished goal run when closing across turns", () => {
     const firstText: AdaptedContentPart = {
       type: "text",
diff --git a/src/lib/adapters/ai-elements-adapter.ts b/src/lib/adapters/ai-elements-adapter.ts
index 1d4e4dbf..b5bcca0f 100644
--- a/src/lib/adapters/ai-elements-adapter.ts
+++ b/src/lib/adapters/ai-elements-adapter.ts
@@ -1566,18 +1566,35 @@ function mergeGoalObjectiveHints(
  * run flushes with `isRunning: isStreaming`, so it settles (static) once the
  * turn stops or on history reload, and shimmers only while live.
  *
- * The Goal card starts collapsed, so a settled run must not keep the turn's
- * trailing prose inside the chip. After the last non-text item (or when the
- * body is only text), lift those text parts out so a reload still shows the
- * final answer under the chip. While the turn is live the prose stays in the
- * card; the card opens itself while running.
+ * The Goal card starts collapsed once the run settles, so a settled run must
+ * not keep the turn's answer inside the chip. After the last process item (or
+ * when the body is only answer parts), lift those parts out so a reload still
+ * shows the final answer under the chip. While the turn is live the answer
+ * stays in the card, which opens itself as soon as it holds anything.
  */
-function splitTrailingTextParts(items: AdaptedContentPart[]): {
+
+/**
+ * Parts that ARE the turn's answer rather than the process that produced it:
+ * prose, the codex Plan-mode document the user has to read, and a generated
+ * image. Everything else (tool calls/results/groups, reasoning, todo plans,
+ * delegation and background-task polls) is process and belongs in the capsule.
+ */
+const GOAL_ANSWER_PART_TYPES: ReadonlySet<AdaptedContentPart["type"]> = new Set(
+  ["text", "proposed-plan", "generated-image"]
+)
+
+/**
+ * Split a settled unfinished run's body at the last process part: everything
+ * after it is the answer and gets lifted out, in order. Answer parts BEFORE a
+ * process part stay inside — prose followed by more work is a mid-run note,
+ * not a wrap-up, and lifting it would reorder the reply.
+ */
+function splitTrailingAnswerParts(items: AdaptedContentPart[]): {
   body: AdaptedContentPart[]
   trailing: AdaptedContentPart[]
 } {
   let end = items.length
-  while (end > 0 && items[end - 1]?.type === "text") {
+  while (end > 0 && GOAL_ANSWER_PART_TYPES.has(items[end - 1]!.type)) {
     end -= 1
   }
   if (end === items.length) {
@@ -1618,9 +1635,9 @@ export function groupGoalRuns(
     items: AdaptedContentPart[],
     isRunning: boolean
   ) => {
-    // Keep live prose inside the running card (it opens while in flight).
-    // Once the turn settles, lift trailing text so a collapsed chip on
-    // reload does not hide the answer.
+    // Keep the live answer inside the running card (it opens while in flight).
+    // Once the turn settles, lift the trailing answer so a collapsed chip on
+    // reload does not hide it.
     // Only lift when the run never closed. A completed update_goal already
     // leaves later prose as siblings; mid-run notes stay in that card.
     const shouldLiftTrailing = !isRunning && end === null
@@ -1634,7 +1651,7 @@ export function groupGoalRuns(
       })
       return
     }
-    const { body, trailing } = splitTrailingTextParts(items)
+    const { body, trailing } = splitTrailingAnswerParts(items)
     result.push({
       type: "goal-run",
       start,
-- 
2.53.0

Thanks again for digging into this one — the root cause writeup made it easy to build on.

…answer

The card seeded `bodyOpen` at mount, but a goal run always mounts before
its body exists (`create_goal` is adapted on its own, so the run starts
with `items: []`). That left the default hostage to whether the row
happened to remount later — it does on the sub-turn merge and on
virtualizer recycling, but not on a plain same-key update. Derive it from
the data instead, with an explicit user override that wins once set.

Widen the settled-run lift from `text` to every part that IS the answer:
a Plan-mode document and a generated image were still buried in the
collapsed capsule. Process parts (tools, reasoning, todo plans, polls)
stay inside, and answer parts sitting before a process part stay put so
the reply is never reordered.
@Adam-Dalloul

Copy link
Copy Markdown
Contributor Author

Allow edits is on, and I applied the patch.

@xintaofei
xintaofei merged commit 7980cf3 into xintaofei:main Aug 21, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants