close
Skip to content

fix: seed artifact companion composer on first open - #2529

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
fanhongy:fix/artifact-comment-agent-empty-prompt
Aug 10, 2026
Merged

fix: seed artifact companion composer on first open#2529
iamwhatever merged 1 commit into
kirodotdev:mainfrom
fanhongy:fix/artifact-comment-agent-empty-prompt

Conversation

@fanhongy

Copy link
Copy Markdown
Contributor

What

Fixes the artifact companion chat opening with an empty composer the first time a comment is sent to an agent (the staged prompt only appears on the second open).

Closes #2524.

Root cause

Two React.StrictMode-only races combined (the app mounts under StrictMode, and the artifact companion panel mounts ChatPage fresh every first open, so it hits mount-effect double-invokes the always-mounted /chat page never sees):

  1. Slot never activated. createBoundSession (ArtifactDetailPage.tsx) wrote the prefill via writePrefill but delegated slot activation to ArtifactChatPanel's useEffect. That switchSlot therefore lands in the same commit that mounts the embedded ChatPage, where ChatPage's mount-only slot re-fetch effect — StrictMode double-invoked, empty deps so it captures a stale activeSlot — re-asserts the prior active slot as the last write. activeSlot never becomes the bound slot, so the prefill is never matched.
  2. Prefill wiped on the second mount invoke. Once activation is fixed, ChatPage's per-slot draft-restore effect consumes and removes PREFILL_STORAGE_KEY on StrictMode's first mount invoke, then the second invoke finds nothing and resets the composer to the empty incoming draft.

The follow-up worktree handler in ChatPage.tsx already documents the correct sequencing (writePrefill then switchSlot back-to-back) — but it works there because /chat's ChatPage is already mounted, so its consumer effect runs once on a dep-change, never a mount double-invoke.

Fix

  • ArtifactDetailPage.tsxcreateBoundSession now dispatches switchSlot(res.key) immediately after writePrefill + addSlotOptimistic, so activeSlot is already the bound slot before the panel mounts ChatPage (idempotent once equal). Covers both the "Ask agent to address" flow and "New chat" (both route through createBoundSession).
  • ChatPage.tsx — a consumedPrefillRef guards the draft-restore consumer against the mount double-invoke: when the effect re-runs for the same active slot after already consuming the prefill, it leaves the composer as-is instead of wiping it to the empty draft. A genuine slot switch changes activeSlot, so the guard can never mask a real draft restore.

Tests

  • New src/test/ChatPageEmbeddedPrefill.test.tsx mounts the real ChatPage the way ArtifactChatPanel does, under StrictMode, and asserts the composer is seeded with the staged prompt on the first open. It fails on main (empty composer) and passes with this change.
  • cd website && npx tsc -b clean; eslint 0 errors; ran the ChatPage + Artifact + Sidebar test surface (65 files / 496 tests) — all green, confirming the shared draft-restore effect change is regression-free.

Known related edge (not fixed here, flagged for maintainers)

An audit surfaced a narrower sibling: re-staging an "Ask agent to address" prompt while the companion panel is already open on that same slot (openCompanionChat resume path, ArtifactDetailPage.tsx:1147) can strand the prompt, because ArtifactChatPanel's ref-guarded effect no-ops switchSlot when the slot is unchanged and nothing re-fires the consumer. It's a distinct trigger from the reported first-open bug and touches hotter shared code; happy to address in a follow-up if you'd like it in scope.

Sending an artifact comment to an agent opened the companion chat with an
empty composer on the first open; the staged prompt only appeared after
closing and reopening the agent.

Two StrictMode-only races combined:
1. createBoundSession wrote the prefill but delegated slot activation to
   ArtifactChatPanel's effect, so switchSlot landed in the same commit that
   mounts the embedded ChatPage. There ChatPage's mount-only slot re-fetch
   effect (double-invoked, stale activeSlot capture) re-asserted the prior
   active slot as the last write, so activeSlot never became the bound slot
   and the prefill was never consumed.
2. Once activated, ChatPage's per-slot draft-restore effect consumed and
   removed the prefill on the first mount invoke, then the second invoke
   found nothing and reset the composer to the empty draft.

Activate the bound slot in createBoundSession back-to-back with writePrefill
(matching the follow-up worktree handler), and guard the draft-restore
consumer against the mount double-invoke with a consumed-prefill ref. Add a
StrictMode regression test.
@fanhongy
fanhongy requested a review from a team August 10, 2026 11:30
@fanhongy
fanhongy requested a review from a team as a code owner August 10, 2026 11:30
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 7360ea2887c2c3b71268af0787092257afc9ff93 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings on the changed lines. The diff is a well-reasoned React StrictMode timing fix: switchSlot is dispatched back-to-back with writePrefill in createBoundSession, and the new consumedPrefillRef guard only suppresses the setInput(draftFallback) when prevSlotVal === activeSlot (a StrictMode re-invoke, not a genuine slot switch, which always changes activeSlot). The ref is never cleared, but the only re-entry with prevSlotVal === activeSlot is a StrictMode/flushDrafts re-run where the composer already holds the correct content, so skipping the fallback restore is benign. No AUTOSDE frontend rule (security, icons, a11y, layout) is touched; the new file is a test, exempt from most rules.

No findings.

[OPUS-REVIEWED] 7360ea2

@github-actions

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — ✅ PASS

Advisory UX-level review of 7360ea2887c2c3b71268af0787092257afc9ff93 via the fork AI-review pipeline — updated in place on each push; does not block merge.

UX-Verdict: PASS

Pure timing fix — the staged prompt now appears on first open instead of second; no strings, layout, or flows change, and the repaired behavior matches the label's promise.

[UX-REVIEWED] 7360ea2

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 7360ea2887c2c3b71268af0787092257afc9ff93 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 7360ea2

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Advisory design-level review of 7360ea2887c2c3b71268af0787092257afc9ff93 via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: PASS

Root-cause fix at the right layer — activation ordering moved to the session owner, consumption made idempotent, with a real-ChatPage StrictMode regression test.

Suggestions

  • Slot activation now has two writers for the create path (createBoundSession's new dispatch(switchSlot) plus ArtifactChatPanel's effect, which the resume path still depends on); update the panel's "this component activates whatever bound slot it is handed" contract comment so a future edit doesn't remove the effect and silently break resume.
  • Your own "known related edge" (re-staged prompt stranded when the panel is already open on the same slot) is evidence the one-shot PREFILL_STORAGE_KEY handoff is the fragile primitive — the follow-up worth doing is making the channel consume-on-send (or per-slot durable staging) rather than adding a third coordinated writer/guard; worth filing now so the flagged edge lands as a channel fix, not another patch.

[DESIGN-REVIEWED] 7360ea2

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 10, 2026
@iamwhatever
iamwhatever merged commit 6758e4e into kirodotdev:main Aug 10, 2026
51 of 52 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 10, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
Sending an artifact comment to an agent opened the companion chat with an
empty composer on the first open; the staged prompt only appeared after
closing and reopening the agent.

Two StrictMode-only races combined:
1. createBoundSession wrote the prefill but delegated slot activation to
   ArtifactChatPanel's effect, so switchSlot landed in the same commit that
   mounts the embedded ChatPage. There ChatPage's mount-only slot re-fetch
   effect (double-invoked, stale activeSlot capture) re-asserted the prior
   active slot as the last write, so activeSlot never became the bound slot
   and the prefill was never consumed.
2. Once activated, ChatPage's per-slot draft-restore effect consumed and
   removed the prefill on the first mount invoke, then the second invoke
   found nothing and reset the composer to the empty draft.

Activate the bound slot in createBoundSession back-to-back with writePrefill
(matching the follow-up worktree handler), and guard the draft-restore
consumer against the mount double-invoke with a consumed-prefill ref. Add a
StrictMode regression test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Artifact comment → "send to agent" opens agent with empty prompt on first launch

2 participants