fix(ui): remove unstable composer inline completion - #3292
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
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.
| ## `@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: |
There was a problem hiding this comment.
[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.
|
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: |
Astro-Han
left a comment
There was a problem hiding this comment.
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.
Summary
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@astryxdesign/core@0.4.0patch applies cleanly to the original npm package withpatch --dry-runThe 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.