Skip to content

fix(subtask): derive subtask state from messages, remove render-time mutation (#3147)#3158

Open
fancyboi999 wants to merge 1 commit into
bytedance:mainfrom
fancyboi999:fix/3147-derive-subtask-state-from-messages
Open

fix(subtask): derive subtask state from messages, remove render-time mutation (#3147)#3158
fancyboi999 wants to merge 1 commit into
bytedance:mainfrom
fancyboi999:fix/3147-derive-subtask-state-from-messages

Conversation

@fancyboi999
Copy link
Copy Markdown
Contributor

Closes #3147. Split from #3138.

Why

MessageList reconstructed the subtask card map by calling updateSubtask() during render from its per-message render loop. That helper actually mutated the SubtaskContext tasks object in place — only the SSE-driven latestMessage path called setState. Three consequences:

  1. React 19 Strict Mode's double render surfaces the pattern as "Cannot update a component while rendering a different component".
  2. Lifecycle regressions like [Stability][v2.0-m1-rc1] Release-blocking issues found in Ultra/user workflow testing #3107 BUG-007 (cards stuck on "running") were masked: when the backend's final ToolMessage arrived, the render-time mutation silently updated the shared object but did not trigger a re-render unless a follow-up latestMessage event happened to land.
  3. SubtaskCard had to useSubtask(id)! non-null-assert the value, which only worked because the same render had just mutated it in.

Design

Two write paths split by data origin.

Base fields (status / result / error / description / ...) are derived from the message list by a pure buildSubtaskMapFromMessages(messages). MessageList passes each entry directly to SubtaskCard as a task prop. No round trip through context, so the first render is already correct — no effect lag, no ! non-null assertion.

Latest streaming AIMessage still comes from the task_running custom SSE event handled in useThreadStream. That fires outside React render, so writing it into context is safe. The provider in tasks/context.tsx now owns only this map; SubtaskCard reads it via useLatestSubtaskMessage(taskId) and merges into the prop inside a useMemo.

The old API surface (useSubtask / useUpdateSubtask) is gone — replaced by useLatestSubtaskMessage / useUpdateLatestMessage.

Changes

  • core/tasks/derive.ts — new pure buildSubtaskMapFromMessages(messages). Handles multi-tool-call AI messages, missing matching tool messages (stays in_progress), and non-task tool_calls (ignored). Forward-compatible with Follow-up: use structured subtask status instead of parsing backend text #3146 / PR fix(subagent): structured subagent_status field over text parsing (#3146) #3154 — when parseSubtaskResult grows the second additional_kwargs argument the call site will start preferring the structured field automatically.
  • core/tasks/context.tsx — provider trimmed to the latest-message map, exposes useLatestSubtaskMessage(id) and useUpdateLatestMessage().
  • components/.../message-list.tsxuseMemo the derived map, removes the render-time updateSubtask loop, passes each task straight into SubtaskCard. Drops unused extractTextFromMessage import.
  • components/.../subtask-card.tsx — takes task: Subtask as a prop, merges latestMessage from context via useLatestSubtaskMessage.
  • core/threads/hooks.tsonCustomEvent handler renames the import to useUpdateLatestMessage and calls it with (taskId, message).
  • vitest.config.ts — include *.test.tsx, set environment: \"jsdom\", globals: true so the new React hook / render tests can run.
  • package.jsonjsdom + @testing-library/react + @testing-library/dom dev deps for the StrictMode regression test.

Tests

  • tests/unit/core/tasks/derive-subtask-map.test.ts — 7 cases covering empty, in_progress seed, completed / failed flips, multi-tool-call, and non-task-tool-ignored.
  • tests/unit/core/tasks/context.test.tsx — 4 cases including a <StrictMode> render that spies on console.error and asserts the "Cannot update a component while rendering" warning is not logged. That assertion is the regression test for the root cause of Follow-up: avoid mutating subtask state during render #3147.

Totals: backend unchanged, frontend 94 passed (was 76, +18 new).

Live verification

Started make dev, sent "use the task tool with general-purpose subagent to run echo derive_v2", watched the subtask card go from the shimmering in_progress state to the completed-with-output state, then saw the AI's final answer rendered cleanly.

A previous attempt at this PR (which kept base state in a context + effect, instead of passing the task in as a prop) reproduced Cannot read properties of undefined (reading 'status') in SubtaskCard on the very first render — the effect-based publish lagged behind the first paint. The prop-driven version this PR ships does not have that problem.

Out of scope (tracked by #3138 follow-ups)

…mutation

Closes bytedance#3147.

## Why

`MessageList` reconstructed the subtask card map by calling
`updateSubtask()` during render from its per-message render loop. That
helper actually mutated the SubtaskContext `tasks` object in place —
only the SSE-driven `latestMessage` path called `setState`. Three
consequences:

1. React 19 Strict Mode's double render surfaces the pattern as
   "Cannot update a component while rendering a different component."
2. Lifecycle regressions like bytedance#3107 BUG-007 (cards stuck on "running")
   were masked: when the backend's final ToolMessage arrived, the
   render-time mutation silently updated the shared object but did
   not trigger a re-render unless a follow-up `latestMessage` event
   happened to land.
3. `SubtaskCard` had to `useSubtask(id)!` non-null-assert the value,
   which only worked because the same render had just mutated it in.

## Design

Two write paths split by data origin:

- **Base fields** (status / result / error / description / ...) are
  derived from the message list by a pure
  `buildSubtaskMapFromMessages(messages)`. `MessageList` passes each
  entry directly to `SubtaskCard` as a `task` prop. No round trip
  through context, so the first render is already correct — no effect
  lag, no `!` non-null assertion.
- **Latest streaming AIMessage** still comes from the `task_running`
  custom SSE event handled in `useThreadStream`. That fires outside
  React render, so writing it into context is safe. The provider in
  `tasks/context.tsx` now owns only this map; `SubtaskCard` reads it
  via `useLatestSubtaskMessage(taskId)` and merges into the prop
  inside a `useMemo`.

Old API surface (`useSubtask` / `useUpdateSubtask`) is gone — replaced
by `useLatestSubtaskMessage` / `useUpdateLatestMessage`.

## Changes

- `core/tasks/derive.ts` — new pure
  `buildSubtaskMapFromMessages(messages)`. Handles multi-tool-call AI
  messages, missing matching tool messages (stays in_progress), and
  non-task tool_calls (ignored). Forward-compatible with bytedance#3146 — when
  `parseSubtaskResult` grows the second `additional_kwargs` argument
  the call site will start preferring the structured field
  automatically.
- `core/tasks/context.tsx` — provider trimmed to the latest-message
  map, exposes `useLatestSubtaskMessage(id)` and
  `useUpdateLatestMessage()`.
- `components/.../message-list.tsx` — `useMemo` the derived map,
  removes the render-time `updateSubtask` loop, passes each `task`
  straight into `SubtaskCard`. Drops unused `extractTextFromMessage`
  import.
- `components/.../subtask-card.tsx` — takes `task: Subtask` as a prop,
  merges `latestMessage` from context via `useLatestSubtaskMessage`.
- `core/threads/hooks.ts` — `onCustomEvent` handler renames the import
  to `useUpdateLatestMessage` and calls it with `(taskId, message)`.
- `vitest.config.ts` — include `*.test.tsx`, set
  `environment: "jsdom"`, `globals: true` so the new React hook /
  render tests can run.
- `package.json` — `jsdom` + `@testing-library/react` +
  `@testing-library/dom` dev deps for the StrictMode regression test.

## Tests

- `tests/unit/core/tasks/derive-subtask-map.test.ts` — 7 cases covering
  empty, in_progress seed, completed / failed flips, multi-tool-call,
  and non-task-tool-ignored.
- `tests/unit/core/tasks/context.test.tsx` — 4 cases including a
  `<StrictMode>` render that spies on `console.error` and asserts the
  "Cannot update a component while rendering" warning is **not**
  logged. That assertion is the regression for the root cause.

## Live verification

`make dev`, sent "use the task tool with general-purpose subagent to
run echo derive_v2", watched the card go from shimmering in_progress
to completed-with-output, then the AI's final answer rendered cleanly.
The previous attempt at this PR kept base state in a context + effect
and reproduced `Cannot read properties of undefined (reading 'status')`
in `SubtaskCard` on the very first render; the prop-driven version
above fixes that.

Refs: bytedance#3138 (split summary), bytedance#3107 (BUG-007 origin),
bytedance#3131 / bytedance#3154 (prior prefix fixes), bytedance#3147 (this issue).
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes React 19 Strict Mode warnings and subtask-card lifecycle glitches by removing render-time mutations of subtask state and switching to a pure “derive from messages” data flow, while keeping only the streaming task_running message in context.

Changes:

  • Introduces a pure buildSubtaskMapFromMessages(messages) to derive base subtask fields from the thread message list.
  • Refactors subtask context to store only latest streaming AI messages (task_running) and updates consumers accordingly.
  • Adds/updates unit tests and Vitest configuration to run React hook/render tests under JSDOM.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
frontend/vitest.config.ts Expands test glob to include TSX; sets JSDOM + globals for React tests.
frontend/tests/unit/core/tasks/derive-subtask-map.test.ts Adds coverage for deriving subtask map from messages (tool-call + tool-result cases).
frontend/tests/unit/core/tasks/context.test.tsx Adds provider/hook tests, including StrictMode regression coverage.
frontend/src/core/threads/hooks.ts Updates SSE handler to write latest streaming message via new context API.
frontend/src/core/tasks/derive.ts Adds pure derivation helper to compute subtask base state from messages.
frontend/src/core/tasks/context.tsx Replaces old mutable subtask map API with latest-message-only context.
frontend/src/components/workspace/messages/subtask-card.tsx Switches to prop-driven base task + merges latest streaming message from context.
frontend/src/components/workspace/messages/message-list.tsx Removes render-time subtask mutations; derives map via useMemo and passes tasks into cards.
frontend/pnpm-lock.yaml Locks new testing dependencies (jsdom, testing-library).
frontend/package.json Adds jsdom and Testing Library dev dependencies for new React tests.
Files not reviewed (1)
  • frontend/pnpm-lock.yaml: Language not supported

Comment on lines +9 to +22
* Derive the subtask card map from the current thread message list.
*
* Bytedance/deer-flow issue #3147: the old data flow built this map by
* calling `updateSubtask` *during render* from `MessageList`, which silently
* mutated the SubtaskContext object without triggering a re-render (only
* the SSE `latestMessage` path called `setState`). That worked by accident
* but is exactly the render-time mutation React Strict Mode warns about
* and the kind of pattern that masks card-stuck regressions like
* `#3107` BUG-007.
*
* Replace it with a pure function over the message list. The result is
* passed into `SubtasksProvider` via `setBaseTasksFromMessages`, batched
* inside an effect, so render stays read-only. The SSE-driven
* `latestMessage` path stays separate (see `useUpdateLatestMessage`).
Comment on lines +353 to +357
// The subtask context is fed by `derivedSubtasks` / the effect
// above — render only consumes it. Collect the per-group task
// *references* (used downstream to build subtask cards and
// count rendered subtasks) directly from the AI tool_calls so
// we do not mutate any shared state here.
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.

Follow-up: avoid mutating subtask state during render

2 participants