Skip to content

web: stage partial input — replay tool-input-partial before final tool input#1659

Open
cliffhall wants to merge 3 commits into
v2/1570-message-logsfrom
v2/1571-stage-partial-input
Open

web: stage partial input — replay tool-input-partial before final tool input#1659
cliffhall wants to merge 3 commits into
v2/1570-message-logsfrom
v2/1571-stage-partial-input

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1571

Adds a "Stage partial input" control so progressive-render widgets can be exercised in the inspector (re-implementation of the relevant slice of PR #1510).

Changes

AppRenderer

  • New partialInputs prop (ordered fragments), snapshotted into pendingPartialsRef at bridge-build time (read via a ref so prop churn never rebuilds the iframe) and cleared on dispose. flushPending replays them via bridge.sendToolInputPartial before the complete tool-input, per spec.

AppsScreen

  • New partialStages state + handleStagePartialInput (snapshots current form values). resetAppChannels gains a keepPartials option so Open App preserves the staged fragments (they're consumed by the renderer) while select/close/back clear them.
  • Adds a "Stage partial input" control (fielded apps only) with a staged count and "Clear staged", wired to AppRenderer's partialInputs. .withProps() constants, no inline styles.

Tests

  • AppRenderer: ordered replay before tool-input; none when omitted.
  • AppsScreen: stage/count/clear gated on fielded apps; staged partials survive Open and are replayed in order before the final input.
  • Per-file coverage ≥90 on all four dimensions; full npm run ci passes locally.

Stacking

Stacked on #1570 (v2/1570-message-logs). Merge order: #1565#1566#1567#1568#1569#1570 → this.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

…l input

AppRenderer:
- New `partialInputs` prop (ordered fragments), snapshotted into pendingPartials
  at bridge-build time (read via a ref so prop churn never rebuilds the iframe)
  and cleared on dispose. flushPending replays them via
  bridge.sendToolInputPartial BEFORE the complete tool-input, per spec.

AppsScreen:
- New `partialStages` state + `handleStagePartialInput` (snapshots current form
  values). resetAppChannels gains a `keepPartials` option so Open App preserves
  the staged fragments (they're consumed by the renderer) while select/close/
  back clear them.
- Adds a "Stage partial input" control (fielded apps only) with a staged count
  and "Clear staged", wired to AppRenderer's `partialInputs`.

Adds AppRenderer tests (ordered replay before tool-input; none when omitted) and
AppsScreen tests (stage/count/clear gated on fields; staged partials survive
Open and are replayed).

Closes #1571

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 11, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 42s —— View job


Review: PR #1659 — stage partial input

  • Read changed files and diff against base
  • Review AppRenderer changes
  • Review AppsScreen changes
  • Review tests
  • Post review

Reviewed against origin/v2/1570-message-logs. Overall: LGTM — clean, well-scoped, and thoroughly documented. The design correctly solves the two tricky parts (ordering guarantee and avoiding iframe rebuilds on fragment churn), and the tests assert the behaviors that matter. No blocking issues. A few minor observations below.


What's done well

  • Ref-based snapshot to avoid rebuilds. Reading partialInputsRef.current at bridge-build time (AppRenderer.tsx:285) rather than adding partialInputs to the effect deps is exactly right — staging fragments would otherwise tear down and rebuild the iframe. The inline comment explaining this (and why the StrictMode reuse path at :260 doesn't re-snapshot) is excellent.
  • Ordering. flushPending sends all staged partials, clears the queue, then sends the final tool-input (AppRenderer.tsx:196-206), matching the spec requirement that partials precede the complete input. The AppRenderer test asserts this via invocationCallOrder, and the AppsScreen test asserts it live via expect(sawInput).toBe(false) inside sendToolInputPartial.
  • keepPartials reset semantics are correct: preserved on handleOpen (the renderer is about to consume them) and cleared on select/close/back. Both branches are exercised by tests.
  • Styling follows the repo conventions — .withProps() constants (PartialStageControls, StagePartialButton, PartialStageCount), no inline styles, control gated on selectedHasFields.

Minor observations (non-blocking)

  1. Shallow snapshot of formValueshandleStagePartialInput captures { ...formValues } (AppsScreen.tsx:399). This is a shallow copy, so any nested object/array in a form value is shared by reference with the live form. In practice onFormChange replaces formValues wholesale with a new object, so a staged snapshot won't observe later edits to top-level keys — but a fragment that captured a nested structure could alias a mutated one. Given the current form model this is almost certainly fine; just noting it in case form values ever grow nested mutable state.

  2. partialStages lives on after Open. On handleOpen the fragments stay in state (only AppRenderer's internal pendingPartialsRef is drained). That's intentional and the reset paths cover cleanup, but since the detail panel (and thus the count/clear UI) only renders while not running, the surviving state is invisible until the next reset — worth a one-line comment at the handleOpen call site noting the state is intentionally not cleared here, to complement the good comment already on resetAppChannels.

  3. Fire-and-forget ordering assumption. The partial sends are void bridge.sendToolInputPartial(...) in a loop, not awaited, so cross-message ordering relies on the bridge posting to the iframe synchronously (or with uniform async structure) — the same assumption the existing input→result sequence already makes, so this introduces no new risk. Just flagging that the tests validate call order, not delivery order (consistent with the rest of the file).

One thing to double-check

  • bridge.sendToolInputPartial({ arguments: args }) is typed against AppBridge from @modelcontextprotocol/ext-apps@^1.7.1. I couldn't verify the method/signature locally (no node_modules in this checkout), but since you report npm run ci (incl. typecheck) passes, the method and its { arguments } shape must exist on the type — mirroring the existing sendToolInput({ arguments }) call. Good.

Nice work — nothing here needs to change to merge.
· v2/1571-stage-partial-input

Address the claude[bot] review's observation #2: add a comment at the
handleOpen call site clarifying that partialStages is intentionally NOT cleared
there — AppRenderer snapshots the fragments into its own pendingPartialsRef at
build time, and the staging UI only renders while not running, so the surviving
state is invisible until the next reset drains it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — LGTM appreciated. Responses:

1. Shallow snapshot of formValues — declined, with reason. handleStagePartialInput captures { ...formValues }, a shallow copy. As you note, the form model replaces formValues wholesale via onFormChange (never mutates in place), so a staged snapshot can't observe later top-level edits. A deep clone would guard only against a hypothetical future nested-mutable form model that doesn't exist today; I'd rather not pay structuredClone on every stage for a case the current model precludes. Left as-is; noted for if the form model ever grows nested mutable state.

2. partialStages survives Open — applied (comment). Added a comment at the handleOpen call site noting the state is intentionally not cleared there (AppRenderer snapshots the fragments at build time; the staging UI only renders while not running, so the surviving state is invisible until the next reset drains it), complementing the existing resetAppChannels comment.

3. Fire-and-forget ordering — no change. Agreed it introduces no new risk — the void bridge.sendToolInputPartial(...) loop makes the same synchronous-post assumption the existing input→result sequence already relies on, and the tests validate call order (consistent with the rest of the file).

Double-check (sendToolInputPartial signature) — confirmed. AppBridge from @modelcontextprotocol/ext-apps@^1.7.1 exposes sendToolInputPartial(params: McpUiToolInputPartialNotification["params"]): Promise<void>, and { arguments } matches — mirrors the existing sendToolInput({ arguments }). npm run ci (incl. typecheck) is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

web: stage partial input — replay tool-input-partial before final tool input

1 participant