test(ui): keep the composer's inline-completion seam closed - #4208
Conversation
) The renderer crash reported in apache#4117 (React error apache#185, Maximum update depth exceeded) resolves to the Astryx 0.4.0 ChatComposerInput inline-offer engine that the 0.1.11 composer fed a prompt-history candidate: a dependency-less useEffect re-decided the offer after every render, both of its exits wrote the same announcement state, and termination depended on a layout measurement agreeing across passes. When real layout disagreed, the writes flip-flopped until React hit the nested-update limit and the crash dialog appeared. The unstable wiring was already removed (apache#3292) and the engine is gone with Astryx 0.5.0 (apache#3755); what was missing is the fence. Add regression coverage that drives the composer through the reported scenario (prompt history seeded, draft a strict prefix of the newest entry), asserts the offer machinery never reappears, and pins the seam shut at the source, plus a changelog entry documenting the root cause.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the unusually thorough reconstruction of the 0.1.11 renderer crash. The diagnostic work is valuable, and the historical problem is real: the old prompt-history completion wiring could feed the Astryx inline-offer engine into a layout-dependent React update loop.
I reviewed exact head 156d0794b943c0a6026dd3e1f983ffd02ef1e7e3. Current main no longer contains that production path: #3292 removed the completion wiring and #3755 removed the vendor engine. This PR therefore adds only a changelog entry and a 294-line test file.
I found one P2 test-quality issue. The behavioral settle test does not exercise the conditions that made the old failure reachable: typeDraft() writes textContent and dispatches input, but never focuses the editor, establishes a caret/selection at the end, or runs real browser layout measurements. The PR verification itself confirms that this test still passes on the 0.1.11 tree where the faulty wiring exists. Only the source/API string fences fail there. The harness also emits React act(...) warnings.
As written, the largest part of the change presents itself as behavioral crash coverage without detecting the old behavior, adding more maintenance than protection. The smallest coherent version is either:
- remove the ineffective DOM harness and retain a compact contract at the actual composer/history seam; or
- replace it with a real Chromium regression that exercises focus, caret, layout, and the inline-offer lifecycle.
Please also eliminate the act(...) warnings in whichever test remains. I would not keep both a large non-reproducing harness and source-regex fences for a production path that has already been deleted.
Only the label check exists for this exact head; there is no successful required test check yet.
Review analysis was assisted by Codex and an independent @reviewer agent. Astro-Han verified the historical and current production paths, exact head, old-tree test behavior, focused local evidence, and severity judgment, and owns this review.
中文对照
谢谢你非常细致地还原 0.1.11 的 renderer crash。诊断工作本身很有价值,历史问题也是真实的:旧的 prompt-history completion 接线会把 Astryx inline-offer engine 带入依赖布局测量的 React 更新循环。
我审查了精确 head 156d0794b943c0a6026dd3e1f983ffd02ef1e7e3。当前 main 已经没有这条生产路径:#3292 删除了 completion 接线,#3755 删除了 vendor engine。因此本 PR 实际只增加 changelog 和一个 294 行测试文件。
当前有一个 P2 测试质量问题。所谓 behavioral settle test 没有覆盖旧问题真正可达的条件:typeDraft() 只是写入 textContent 并派发 input,没有聚焦 editor、没有建立末尾 caret/selection,也没有真实浏览器布局测量。PR 自己的验证也确认,这条测试在仍包含故障接线的 0.1.11 tree 上依然通过;真正失败的只有 source/API 字符串围栏。这套 harness 还会输出 React act(...) 警告。
当前最大的代码块看起来像行为级崩溃覆盖,但实际上不能检测旧行为,维护成本大于保护价值。最小完整方案应二选一:
- 删除无效 DOM harness,只在真实 composer/history seam 保留一个精简契约;或
- 改为真实 Chromium 回归测试,覆盖 focus、caret、layout 和 inline-offer lifecycle。
无论保留哪种测试,也请清理 act(...) 警告。对于一条已经从生产代码删除的路径,不建议同时保留大型但无法复现问题的 harness 和 source regex 围栏。
当前 exact head 只有 label check,没有成功的 required test。
本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了历史与当前生产路径、精确 head、旧 tree 测试行为、聚焦本地证据和问题分级,并对本次 Review 负责。
Review on apache#4208: the linkedom settle harness could not reach the old failure — without real layout the offer engine never activated, so the harness passed on the 0.1.11 tree it was written against, while the React act(...) warnings added noise. Replace it with the compact contract at the seam every reintroduction must touch: composer.tsx passes no inlineCompletion prop to ChatComposerInput, and use-composer-history.ts carries no prompt-completion source. Both assertions fail on the 0.1.11 sources and pass on main, with no React rendering left in the file.
|
Thanks for the review — agreed on all points. I took option 1: Why not the Chromium regression: the flip-flop itself needs real layout to disagree between the insertion pass and the post-commit re-measurement (a wrap boundary inside the ±0.5 px tolerance at the scroll cap, scrollbar appearance, zoom/rounding), and nothing in a replay can force that deterministically — even in Chromium the old wiring would usually settle after one pass. With the production path deleted, the meaningful protection is the contract at what every reintroduction must touch, which is what remains: no Both assertions verified to fail against the actual 0.1.11 sources and pass on current Also fixed the PR body: bare |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the focused follow-up. I rereviewed exact head 8843694c79bd495e5c8190aa9eb26c368ab24778.
The earlier P2 is fully closed: the 294-line linkedom/React harness that could not reach the historical failure has been deleted, the act(...) warnings are gone, and the remaining contract is limited to the two seams any reintroduction must touch:
- the composer does not feed
inlineCompletion*intoChatComposerInput; - the history hook does not expose the old prompt-completion source.
Both assertions fail on the actual 0.1.11 sources and pass on current code. The cumulative PR is now 107 additions across two files: the compact seam contract and the changelog record. That is the smallest coherent protection for a production path already removed by #3292 and #3755; recreating a nondeterministic Chromium layout failure would add cost without a reliable signal.
I found no P0–P3 issues on this head. Focused UI tests, Biome, ASF headers, and git diff --check pass. The hosted required test check is currently running on this exact SHA, so merge should wait for that delivery gate, but no further code change is requested.
中文对照
谢谢你快速而且准确地收敛了这次修改。我重新审查了当前精确 head 8843694c79bd495e5c8190aa9eb26c368ab24778。
之前的 P2 已经完整解决:无法触达历史故障的 294 行 linkedom/React harness 已删除,act(...) 警告也消失了。当前只保留任何重新引入都必须经过的两个 seam:Composer 不再向 ChatComposerInput 传入 inlineCompletion*,history hook 不再提供旧的 prompt-completion source。两条断言在真实 0.1.11 源码上失败,在当前代码上通过。
累计改动现在只有两个文件、107 行:精简 seam contract 和 changelog 记录。这已经是一条被 #3292 与 #3755 删除的生产路径所需的最小完整保护,没有必要为了不可稳定复现的布局竞态重新加入 Chromium harness。
当前 head 没有 P0–P3。聚焦 UI 测试、Biome、ASF header 和 git diff --check 均通过。托管的 required test 仍在当前 SHA 上运行,所以应等待其绿色再合并,但不需要作者继续修改代码。
AI-assisted review notice: Codex coordinated two independent incremental review lanes; Astro-Han verified the cumulative diff, old-tree sharpness, current production seam, focused validation, and severity judgment before approval.
Fixes #4117
Summary
The crash in #4117 is React error
#185— "Maximum update depth exceeded" — thrown by the prompt-history inline-completion engine inside the AstryxChatComposerInput, which the 0.1.11 composer fed a completion candidate on every keystroke. The unstable wiring was already removed frommainby #3292, and the vendor engine no longer exists in the Astryx 0.5.0 upgrade (#3755), so currentmaincannot crash through this path. Whatmainwas missing is proof and a fence: this PR documents the root cause in the changelog and adds a compact contract test that pins the composer/history seam shut, so the engine cannot be fed again without a deliberate, reviewable change.How the diagnostic stack was traced
The reporter could not reproduce the crash, so I reconstructed their exact build instead. The diagnostic lists five renderer chunk names with hashes (
index-Dswu7pEs.js,src-DS7ZWxYS.js,module-page-B9KtHvkK.js,use-roving-row-focus-Akw7s7le.js,Toolbar-D_Y1A8uy.js). Building the renderer at commit4a54ee25c(the commit 0.1.11 was packaged from, immediately before thechore(release): prepare 0.1.11bump) reproduces all five hashes byte-for-byte, which makes the minified stack resolvable: rebuilding that commit with--sourcemapand mapping everyline:columnof the diagnostic gives the original files, functions and call sites.The React component stack (innermost first), de-minified:
ki← the crashing component@astryxdesign/core/dist/Chat/ChatComposerInput.js:168—ChatComposerInputFiChat/ChatComposer.js:109—ChatComposerpackages/ui/src/composer.tsx:367— the MakaComposer's<form>Moapps/desktop/src/renderer/chat-composer-region.tsx:68ma/ldChat/ChatLayout.js:188/packages/ui/src/chat-surface-layout.tsx:36W/J/CeLayout/LayoutContent(the module-page shell)iepackages/ui/src/locale-context.tsxprovider chainnfapps/desktop/src/renderer/app.tsx:9(root)And the two decisive JS frames on the crash stack itself:
src-DS7ZWxYS.js:10:2426→ChatComposerInput.js:383:5—setInlineCompletionAnnouncement('')insidewithdrawOffersrc-DS7ZWxYS.js:10:2756→ChatComposerInput.js:418:5—withdrawOffer()called fromreconcileOfferThe throw itself lands in
getRootForUpdatedFiber(react-dom-client.production.js:2410), which fires the minified error#185oncenestedUpdateCount > 50— i.e. fifty commit-phase updates were flushed inside one nested chain before the crash was captured.Root cause
In 0.1.11,
packages/ui/src/composer.tsxpassed a live completion candidate into the vendor engine:matchCompletion(thematchPromptHistorymatcher removed by #3292) returned the remainder of the newest history entry whenever the draft was a strict prefix of it — so for any user with prompt history, offers were standing while retyping a previous prompt, which is exactly the reported precondition.Astryx 0.4.0 implemented the offer inside
ChatComposerInputwith three properties that together allow an unbounded update loop:useEffect(reconcileOffer);— deliberately re-run after every render (its comment says the decision reads the live selection, which a render does not announce).withdrawOffer()callssetInlineCompletionAnnouncement('')unconditionally (even when the announcement is already empty), while the re-offer path inserts the offer span and callssetInlineCompletionAnnouncement('<suffix> <label>'). If the effect keeps reaching different exits across passes, the state flip-flops'' ↔ suffix, and each flip schedules another commit-phase update in the same nested chain.offerFullyVisible(editable, span)— agetBoundingClientRectcomparison with a ±0.5 px tolerance — and eligibility also readscaretAtContentEndanddocument.activeElement. The pass that inserts the span measures it immediately afterappendChildinside the same effect; the pass after the announcement commit re-measures it post-reflow. When real layout answers differently between those two passes — a draft at themaxRowsscroll cap where the offer's tail wraps the field's bottom edge inside the tolerance, scrollbar appearance, zoom/DPI/font-metric rounding — the fast path fails,withdrawOfferfires, the next pass re-offers, and the loop runs until React's 50-nested-update ceiling throws#185.That dependency on exact viewport geometry is also why it could not be reproduced: in a dev environment the two measurements agree, the effect settles after one pass, and nothing looks wrong. The crash dialog in the report is the app's own renderer-crash boundary catching the throw — the alert itself is working as designed; the loop behind it was the bug.
What this PR changes
packages/ui/src/__tests__/composer-inline-completion-seam.test.tsx— a compact contract at the seam every reintroduction must touch (the file header documents the full root cause for whoever lands here next):composer.tsxpasses noinlineCompletion*prop toChatComposerInput— the prop was the engine's only host-side input; the assertion message points at Bug:alert error dialog #4117, fix(ui): remove unstable composer inline completion #3292, and what a safe reintroduction would require.use-composer-history.tscarries no prompt-completion source (matchCompletion/matchPromptHistory/ theprompt-history-matchmodule) — the matcher that fed the prop.No React rendering is involved: the flip-flop itself cannot be pinned by a unit harness (it needs real Chromium layout to disagree between two measurement passes, which no DOM emulator performs — the first version of this PR carried a linkedom harness that could not detect the old wiring, removed in
8843694c7after review), so the contract asserts the deleted production path stays deleted at its source.CHANGELOG.md— a### Fixedentry under0.2.0 - Unreleaseddocumenting the crash, its root cause, and the two changes that closed it (#3292, #3755).Verification
main, so the fence detects exactly the deleted path.npm run testinpackages/ui— 281/281 pass,tscbuild clean,biome checkclean. No Reactact(...)warnings — the test file renders nothing.Test plan
npm run testinpackages/ui(includes the new seam contract)npx biome checkon the new file