Skip to content

fix(ui): remove unstable composer inline completion - #3292

Merged
Astro-Han merged 1 commit into
apache:mainfrom
jackwener:codex/remove-inline-completion-crash
Aug 20, 2026
Merged

fix(ui): remove unstable composer inline completion#3292
Astro-Han merged 1 commit into
apache:mainfrom
jackwener:codex/remove-inline-completion-crash

Conversation

@jackwener

Copy link
Copy Markdown
Member

Summary

  • remove prompt-history inline completion from the composer
  • remove the ChatComposerInput dependency patch that mixed React state with manual contentEditable DOM reconciliation
  • keep prompt-history recall and same-document history clearing behavior unchanged
  • remove feature-specific tests, story, copy, and matching code

Why

The 0.1.11 renderer crash is React error #185 (Maximum update depth exceeded), with the component stack rooted at ChatComposerInput. The inline-completion patch runs a state-writing reconciliation effect after every render while also mutating the controlled contentEditable DOM. Under an unstable DOM/selection transition, the offer span can be withdrawn and reinserted repeatedly, and the live-region state schedules the next render.

This auxiliary completion UX is not worth maintaining a second state machine inside the editor. Removing the feature eliminates the circular authority instead of adding another guard.

Verification

  • git diff --check
  • verified the reduced @astryxdesign/core@0.4.0 patch applies cleanly to the original npm package with patch --dry-run
  • targeted Chromium reproduction confirmed ordinary controlled input behavior is stable; no full test suite run

The standalone UI typecheck was not treated as a signal because this isolated worktree reused stale build artifacts and reported existing workspace module-resolution/dependency-patch errors unrelated to this diff.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks — removing the feature rather than adding a fourth guard is the right call here, and the diagnosis holds. Reviewed exact head 9018b3294108ac621aa524b0de2ba8e37ff03f75.

The loop is visible in the deleted patch: reconcileOffer writes React state through setInlineCompletionAnnouncement, useEffect(reconcileOffer) has no dependency array so it runs after every commit, and the same effect also mutates the controlled contentEditable that the value-sync effect rewrites — so a withdraw-and-reinsert pair is two state writes per commit and the next commit re-runs it. That is React #185 with the component stack the report gives, and no guard placed inside that cycle can be trusted, because the cycle is the authority conflict itself.

The removal reaches a minimal final state, which is the part I checked hardest. ChatComposerInput is now entirely absent from patches/@astryxdesign+core+0.4.0.patch rather than half-reverted; a repository-wide search at this head finds no surviving reference to inlineCompletion, matchPromptHistory, matchCompletion, canonicalizePromptText or data-astryx-inline-completion outside the patch directory; the README seam count is correctly down to five; and prompt-history-match.ts was never re-exported from @maka/ui, so deleting it is not a public API change. Prompt-history recall survives intact — handleArrowKey and the reconcileHistorySync subscription are untouched, and dropping setHistoryRevision is safe now that nothing renders from the entries, since hasHistory is passed a constant false.

No P0/P1/P2 findings. One P3 inline. The required test check passes at this head.

Review disclosure: this review was prepared with Claude Code, which read the diff at this head, ran the residual-reference searches described above against the head tree, and traced the effect cycle in the removed patch. The crash itself was not reproduced. The human contributor reviewed this before posting.

Comment thread patches/README.md
## `@astryxdesign/core@0.4.0`

Six published component seams drop host-owned state or semantics:
Five published component seams drop host-owned state or semantics:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P3] Keep the constraint this crash taught, now that the entry carrying it is gone. patches/README.md is where this repository records what each seam costs and when it may be deleted, and the removed bullet was the only written trace of the failure — that a patch which both writes React state from an unconditional effect and mutates the controlled contentEditable creates a render cycle the host cannot break from outside. The next patch that needs to draw something inside the editor has no reason not to reach for the same shape, and the reviewer of that patch has nothing to point at. One line near the top of this section stating the rule, rather than a bullet for a seam that no longer exists, costs nothing and is the only durable guard a removal-shaped fix can carry. Worth mentioning in the body too: since the fix is a deletion, the confirmation signal is the next release's crash rate, not a test — saying so, and linking the 0.1.11 crash report, is what lets someone verify this landed.

@M4n5ter

M4n5ter commented Aug 20, 2026

Copy link
Copy Markdown
Member

LGTM. I reviewed the removal across the Composer/history path and the Astryx patch. The unstable inline-completion state machine is removed cleanly, while prompt-history recall and clearing remain intact. I found no blocking issues.

Non-blocking follow-up: @maka/ui can narrow its input-history barrel export to clearGlobalInputHistory, since the read, save, and subscribe APIs no longer have cross-package consumers.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving at 9018b3294108ac621aa524b0de2ba8e37ff03f75. My review of this head found nothing above P3, so nothing here should hold the merge — the inline note stands as a suggestion, to take or leave.

The review disclosure on my earlier comment applies: it was prepared with Claude Code, and I reviewed the diff and the findings myself before posting. This approval is my own judgment.

@Astro-Han
Astro-Han merged commit c5d535d into apache:main Aug 20, 2026
1 check 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.

3 participants