Skip to content

test(ui): keep the composer's inline-completion seam closed - #4208

Merged
Astro-Han merged 2 commits into
apache:mainfrom
abhinav-phi:test/composer-inline-completion-regression
Aug 29, 2026
Merged

test(ui): keep the composer's inline-completion seam closed#4208
Astro-Han merged 2 commits into
apache:mainfrom
abhinav-phi:test/composer-inline-completion-regression

Conversation

@abhinav-phi

@abhinav-phi abhinav-phi commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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 Astryx ChatComposerInput, which the 0.1.11 composer fed a completion candidate on every keystroke. The unstable wiring was already removed from main by #3292, and the vendor engine no longer exists in the Astryx 0.5.0 upgrade (#3755), so current main cannot crash through this path. What main was 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 commit 4a54ee25c (the commit 0.1.11 was packaged from, immediately before the chore(release): prepare 0.1.11 bump) reproduces all five hashes byte-for-byte, which makes the minified stack resolvable: rebuilding that commit with --sourcemap and mapping every line:column of the diagnostic gives the original files, functions and call sites.

The React component stack (innermost first), de-minified:

Diagnostic frame Resolved to
kithe crashing component @astryxdesign/core/dist/Chat/ChatComposerInput.js:168ChatComposerInput
Fi Chat/ChatComposer.js:109ChatComposer
(anonymous) packages/ui/src/composer.tsx:367 — the Maka Composer's <form>
Mo apps/desktop/src/renderer/chat-composer-region.tsx:68
ma / ld Chat/ChatLayout.js:188 / packages/ui/src/chat-surface-layout.tsx:36
W / J / Ce Astryx Layout / LayoutContent (the module-page shell)
ie packages/ui/src/locale-context.tsx provider chain
nf apps/desktop/src/renderer/app.tsx:9 (root)

And the two decisive JS frames on the crash stack itself:

  • src-DS7ZWxYS.js:10:2426ChatComposerInput.js:383:5setInlineCompletionAnnouncement('') inside withdrawOffer
  • src-DS7ZWxYS.js:10:2756ChatComposerInput.js:418:5withdrawOffer() called from reconcileOffer

The throw itself lands in getRootForUpdatedFiber (react-dom-client.production.js:2410), which fires the minified error #185 once nestedUpdateCount > 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.tsx passed a live completion candidate into the vendor engine:

<ChatComposerInput
  inlineCompletion={matchCompletion(text) ?? undefined}   // prompt-history prefix completion
  inlineCompletionLabel={copy.inlineCompletionHint}
  ...

matchCompletion (the matchPromptHistory matcher 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 ChatComposerInput with three properties that together allow an unbounded update loop:

  1. A dependency-less passive effectuseEffect(reconcileOffer); — deliberately re-run after every render (its comment says the decision reads the live selection, which a render does not announce).
  2. Both exits write the same state. withdrawOffer() calls setInlineCompletionAnnouncement('') unconditionally (even when the announcement is already empty), while the re-offer path inserts the offer span and calls setInlineCompletionAnnouncement('<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.
  3. Termination depends on a real-layout measurement agreeing across passes. The "already correct" fast path re-measures the standing span with offerFullyVisible(editable, span) — a getBoundingClientRect comparison with a ±0.5 px tolerance — and eligibility also reads caretAtContentEnd and document.activeElement. The pass that inserts the span measures it immediately after appendChild inside 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 the maxRows scroll 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, withdrawOffer fires, 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):

  1. composer.tsx passes no inlineCompletion* prop to ChatComposerInput — 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.
  2. use-composer-history.ts carries no prompt-completion source (matchCompletion / matchPromptHistory / the prompt-history-match module) — 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 8843694c7 after review), so the contract asserts the deleted production path stays deleted at its source.

CHANGELOG.md — a ### Fixed entry under 0.2.0 - Unreleased documenting the crash, its root cause, and the two changes that closed it (#3292, #3755).

Verification

  • Sharpness: both contract assertions fail against the actual 0.1.11 sources (where the wiring existed) and pass on current main, so the fence detects exactly the deleted path.
  • On this branch: npm run test in packages/ui281/281 pass, tsc build clean, biome check clean. No React act(...) warnings — the test file renders nothing.

Test plan

  • npm run test in packages/ui (includes the new seam contract)
  • npx biome check on the new file
  • New contract verified to fail against the pre-fix tree (0.1.11 sources)

)

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.
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 29, 2026

@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 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:

  1. remove the ineffective DOM harness and retain a compact contract at the actual composer/history seam; or
  2. 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(...) 警告。

当前最大的代码块看起来像行为级崩溃覆盖,但实际上不能检测旧行为,维护成本大于保护价值。最小完整方案应二选一:

  1. 删除无效 DOM harness,只在真实 composer/history seam 保留一个精简契约;或
  2. 改为真实 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.
@abhinav-phi

abhinav-phi commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — agreed on all points. I took option 1: 8843694c7 removes the linkedom harness and keeps the compact contract at the seam.

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 inlineCompletion* prop in composer.tsx, no matchCompletion/matchPromptHistory in use-composer-history.ts, each with a failure message pointing at this issue and the removal.

Both assertions verified to fail against the actual 0.1.11 sources and pass on current main; packages/ui is 281/281, and the file no longer renders React, so the act(...) warnings are gone entirely.

Also fixed the PR body: bare #185 was autolinking to an unrelated PR; the error code is now a code span.

@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 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* into ChatComposerInput;
  • 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.

@Astro-Han
Astro-Han merged commit 827b3fd into apache:main Aug 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug:alert error dialog

2 participants