Skip to content

feat(ui): complete the composer draft from prompt history with Tab - #1874

Merged
Astro-Han merged 13 commits into
apache:mainfrom
ARE404:are404/feat-composer-inline-suggestion
Aug 18, 2026
Merged

feat(ui): complete the composer draft from prompt history with Tab#1874
Astro-Han merged 13 commits into
apache:mainfrom
ARE404:are404/feat-composer-inline-suggestion

Conversation

@ARE404

@ARE404 ARE404 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

What

The composer completes the draft you are typing with the rest of a matching past prompt, drawn dim after the caret inside the editor. Tab commits it, Escape dismisses it, every other key ignores it.

帮我把| composer 的样式再收紧一点        ← the dim half is the offer
      ^ caret                            Tab → the whole line is the draft

Where the pieces live

What to offer matchPromptHistory(draft, entries) — the remainder of the newest history entry the draft is a prefix of. Pure, no DOM.
Who owns the history useComposerHistory, the only holder. input-history.ts notifies on both of its writes so the holder re-reads instead of a second copy going stale.
Whether it may be shown, and how it commits ChatComposerInput, via inlineCompletion / inlineCompletionLabel (patched into Astryx — see patches/README.md).

This is history retrieval presented as a completion. It does not generate text and does not recommend a prompt: those are asynchronous, metered, and need not extend a typed prefix, so they are different decision contracts and are deliberately not folded into this one. Only the visual seam would be worth sharing.

composer.tsx contributes 10 lines: it supplies the candidate and nothing else.

Why the offer lives inside the editor

An earlier revision drew it as an overlay mirroring the draft. That cannot be made correct: the preview and the insertion are two layouts agreeing by luck. Measured, a one-row field offering a 116-character completion showed 57 characters and Tab inserted all 116 — 59 characters entered the draft having never been on screen.

Inside the editable the field grows and wraps around a contenteditable=false span that serialize skips, so what is shown and what Tab commits are the same text by construction. The same measurement is now 116 of 116.

The editor also owns the questions only it can answer: the selection, the composition state, whether a trigger menu already holds Tab, and how the text wraps and scrolls. It is the only place that knows when an offer is really on screen, which is what lets it announce one.

One offer, one draft, one transaction

An offer is { base, suffix } — the candidate plus the exact serialized draft it was derived from. Every question about it is really a question about whether the editor still holds that draft, so withdrawOffer / reconcileOffer / acceptOffer own the DOM node, the active record, the dismissal and the announcement together.

  • Only a fully visible candidate is promoted. Laid out, measured against the field's visible box, withdrawn if its tail falls past the bottom.
  • Acceptance is an undoable editing transaction. Through execCommand('insertText'), not Range.insertNode — the latter is not on Chromium's contentEditable undo stack, so Ctrl/Cmd+Z took back the character the user had typed and left the completion standing. Revalidated synchronously against the candidate's base draft first.
  • Reconciliation runs after the controlled-value sync, not before, so a programmatic setText cannot promote an offer against a draft the editor is about to replace.
  • One canonicalization (canonicalizePromptText, NBSP → space) for storage and matching, so a draft carrying token anchors can match the history it was stored as.

Keyboard contract

Tab, offer showing and valid commits it, caret at the end, focus stays
Tab, no offer / clipped / caret moved / composing ordinary focus move
Shift/Alt/Ctrl/Cmd+Tab never a completion
Tab, trigger menu open the menu keeps it
Escape withdraws the offer and falls through, so a streaming turn still stops on the first press

Tests

  • packages/ui/src/__tests__/prompt-history-match.test.ts — the matcher and the canonicalization.
  • packages/ui/src/__tests__/astryx-inline-completion.test.tsx — the patch guard: unpatched, the prop falls into ...rest and React renders a stray attribute.
  • apps/desktop/e2e/composer-inline-completion.spec.ts10 Electron journeys: visible acceptance, a clipped offer leaving Tab untouched, blur/refocus, composition, caret movement, trigger-menu priority, streaming Escape, Tab/Undo/Redo, a programmatic draft replacement invalidating a live offer, and a multi-line completion keeping its line breaks through Tab/Undo/Redo.

The Storybook story is a review driver only — the render smoke opens stories in embedded mode, which disables autoplay, so it proves the composer mounts and nothing more.

Generative tooling

Generative tooling made a substantive contribution to this pull request. Claude Code wrote the implementation, the patch, the tests and this description, and carried out the measurements quoted above; commits carry Generated-by: Claude Code. The human contributor of record reviewed the work, decided to submit it, and remains responsible for its accuracy, provenance and licensing. Upstream ask filed as facebook/astryx#4822.

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

P2. suggestComposerCompletion calls detectMentionTrigger unconditionally and returns null for any boundary-anchored @// draft. In a Composer without mention props (the quote companion panel is one), /review ... history can never be completed. Let the matcher only match history; let the Composer decide when mention owns Tab.

P2. The 500 ms history cache in use-composer-suggestion is not invalidated by clearGlobalInputHistory. A Tab inside that window writes the stale entry back into the draft after clearing. One authoritative snapshot from input-history.ts would remove the second copy.

P3. The new tests call only the pure matcher. Tab routing, the DOM write, and caret placement in composer.tsx are untested.

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

Note: approved but currently unmergeable

This PR already has an approval, but it is currently blocked by a merge conflict with main (merge-state DIRTY). Please rebase onto the latest main; once the conflict is resolved the approval covers the head again and it can be merged.

@ARE404
ARE404 force-pushed the are404/feat-composer-inline-suggestion branch from 4468d2d to 67110e8 Compare August 5, 2026 09:05
@ARE404

ARE404 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main, which had meanwhile landed #1861 slice 3 — so this is now built on ChatComposerInput rather than the textarea. That migration also turned your first point into a structural fix rather than an argument.

P2 — the matcher was deciding mention ownership

Fixed, and at the layer you asked for. suggestComposerCompletion now matches history and nothing else; detectMentionTrigger is gone from it (and from the tree). Whether something else owns the caret and Tab is asked of the editor instead, in two places:

  • paint: use-composer-suggestion.ts reads aria-expanded off the editable before offering;
  • routing: commitsComposerSuggestion(event, triggerMenuOpen) in onInputKeyDown, off the same attribute the existing Enter branch already reads.

That is the editor's live attribute, not a React flag, so the frame-lag worry that put the check in the matcher in the first place is gone too. Verified both directions in Storybook against a seeded history: with the / menu open a /rev draft paints no ghost; with the menu dismissed the same draft completes to /review 这段实现有没有问题. So a Composer without mention sources completes its own /… history — your quote-companion case.

P2 — the 500 ms cache was not invalidated by a clear

Moved into input-history.ts as the module's one snapshot (readCachedGlobalInputHistory), and both writers there drop it. clearGlobalInputHistory drops it before the removeItem that can throw, so there is no ordering in which a cleared prompt survives into a Tab. The TTL is still there but only as a backstop for writes this module did not perform, and says so.

P3 — only the pure matcher was covered

Two answers, since there is no DOM test runner here:

  • The routing rule is now itself pure and unit-tested: bare Tab only, never Shift/Alt/Ctrl/Cmd+Tab, and yields to an open trigger menu.
  • The DOM write and the caret are covered by a play story that runs in the storybook job — product-shell-official-appshell--composer-inline-suggestion, added to the smoke manifest. It types a prefix, waits for the mirror to read exactly the recalled prompt, dispatches Tab, then asserts the draft, the caret offset, that the ghost is gone, and that a second Tab does not touch the draft. A throwing play fails the smoke run (FIDELITY.md), so this is real CI coverage rather than a story that merely renders.

What the migration changed

  • The paint is still a mirror, but the editable is styled by Astryx — its own 4px padding, and line-height: 22px where the wrapper inherits 20px. A product rule restating those would be a copy that drifts, so the hook reads the box and type off the editable and passes them as inline styles; the CSS keeps only what the mirror owes on its own account. Measured after the change: ghost rect and editable rect agree to the pixel (440.5 / 480 / 656 × 30), font and padding identical.
  • The mirror is only mounted for a plain-text draft. A Skill chip is a styled span with its own box and no run of text can stand in for it, so a draft containing one gets no suggestion rather than a misplaced one.
  • Accepting now goes through the editor — execCommand('insertText') at the caret — so it is one undo step and emits the input event the controlled value and the draft store already hang off, instead of writing the value behind their backs.
  • The overlay lost its imperative handle: the input is controlled now, so it is a plain component again.

Checks

@maka/ui 335/335, typecheck, format:check, lint, check-dead-css, check-story-annotations all clean. @maka/desktop is 1701/1707 — the six failures are XaiOAuthService from #1946, byte-identical to main here (git diff upstream/main -- apps/desktop/src/main/oauth is empty), so they are not this branch's.

@ARE404

ARE404 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

CI is green on all of it, including test_workspaces — so a correction to my last comment: those six XaiOAuthService failures were local to my checkout, not main and not #1946, and I should not have pointed at that PR.

The cause is that the test starts a real loopback server on the fixed XAI_CALLBACK_PORT = 56121, and something else on this machine was already holding it (EADDRINUSE), so getAuthorizationUrl returned {ok: false} and every assertion in the suite fell over. Nothing to do with the code under test — though a fixed port does make that suite unrunnable next to anything that has the port open, if that is worth an issue.

@ARE404
ARE404 marked this pull request as draft August 5, 2026 09:31
@ARE404
ARE404 marked this pull request as ready for review August 7, 2026 13:39
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for exploring this. Reusing prompt history is useful, and the pure matching logic is a good starting point. I think the current implementation needs a cleaner product and ownership boundary before it lands.

P1 — Tab can insert completion text that the user never saw.

The mirror is limited to the editable’s current height and clips overflow, while commit() always inserts the full suffix. With a multiline history entry, the ghost can begin on a completely hidden second line, but Tab still inserts it. Long wrapped prompts have the same issue: the visible preview and the committed value are not guaranteed to match.

P1 — Tab changes meaning for screen-reader users without exposing the completion.

The ghost is aria-hidden, with no accessible announcement or acceptance instruction. A screen-reader user can press Tab expecting normal focus navigation and instead have the draft modified.

These look like symptoms of the same architectural problem rather than isolated CSS or ARIA fixes. The external mirror and the 205-line hook effectively build a second editor beside ChatComposerInput: they query private DOM, copy layout, infer trigger state from attributes, maintain another history snapshot/TTL, and write back through editor commands.

Could we reduce this to a small first-class ChatComposerInput capability?

<ChatComposerInput
  value={text}
  inlineCompletion={suffix}
  onChange={setText}
/>

The ownership could then be:

  • useComposerHistory remains the only history owner and exposes a synchronous history matcher;
  • the matcher returns only the suffix;
  • ChatComposerInput renders that suffix inside the real editor;
  • the input accepts it only after the trigger menu has had the first chance to consume Tab;
  • selection, IME composition, wrapping, scrolling, Escape dismissal, and accessible announcement stay inside the input, where those states are authoritative.

Astryx already uses an ephemeral inline span for dictation. A generic non-editable completion span could reuse that model, remain excluded from serialization, participate in the editor’s real layout, and become real text only when accepted. If Astryx does not expose this seam yet, I would prefer adding the small upstream capability first instead of maintaining a parallel editor implementation in Maka.

There is also an important product boundary here: this PR performs history matching, presented through a completion interaction. It does not generate a new completion or recommend a new prompt.

newestPrefixMatch and a future context/model suggestion do not share the same decision contract:

  • history matching is synchronous, deterministic, free, and retrieves existing text;
  • model completion is asynchronous, cancellable, metered, and produces new text;
  • prompt suggestions can appear on an empty draft and need not extend a typed prefix.

I would name the decision function around what it actually does, such as matchPromptHistory, and keep future model completion or prompt suggestions out of this abstraction. The visual inlineCompletion seam may be shared; the decision sources should remain separate.

For this PR, the precise behavior can remain small: when the user types a prefix, complete it from the newest matching history entry; when the draft is empty, ArrowUp continues to own history recall. That avoids presenting the latest historical prompt as if it were a new suggestion.

I would keep the matcher and its tests, but replace the mirror, DOM inference, writeback path, and second history cache with input-owned inline completion.

简体中文

感谢探索这个功能。复用 Prompt 历史是有价值的,纯匹配逻辑也是一个不错的起点。但在合并前,我认为需要把产品概念和前端职责边界收得更清楚。

P1 — Tab 可能插入用户从未看到的补全内容。

mirror 的高度被限制为当前输入框高度,超出部分会被裁掉,但 commit() 始终插入完整 suffix。对于多行历史记录,ghost 可能从完全不可见的第二行开始,Tab 仍会插入它。长 Prompt 自动换行时也一样:用户看到的预览和最终写入的内容没有保证一致。

P1 — 对屏幕阅读器用户,Tab 会在没有暴露补全信息的情况下改变含义。

ghost 被标记为 aria-hidden,没有可访问播报,也没有说明 Tab 会接受补全。屏幕阅读器用户按 Tab 时,本来预期移动焦点,实际却可能修改草稿。

这两个问题更像同一个架构问题的症状,而不是独立的 CSS 或 ARIA 修补项。外部 mirror 和 205 行 hook,实际上是在 ChatComposerInput 旁边又实现了半个编辑器:查询内部 DOM、复制布局、从属性推断菜单状态、维护另一份 history snapshot/TTL,再通过编辑器命令写回。

建议把它收敛成 ChatComposerInput 的一个小型正式能力:

<ChatComposerInput
  value={text}
  inlineCompletion={suffix}
  onChange={setText}
/>

职责可以是:

  • useComposerHistory 继续作为唯一的历史记录 owner,并提供同步的历史匹配;
  • matcher 只返回 suffix;
  • ChatComposerInput 在真实输入框内部显示 suffix;
  • 只有触发菜单没有消费 Tab 时,输入组件才接受补全;
  • selection、IME composition、换行、滚动、Escape 隐藏和无障碍播报,都由输入组件内部处理,因为只有它拥有这些状态的权威信息。

Astryx 已经为语音输入使用了临时 inline span。可以把这个模型泛化为不可编辑的 completion span:序列化时忽略,参与输入框真实布局,只有接受后才变成真实文本。如果 Astryx 尚未提供这个 seam,我更倾向于先给上游增加这个小能力,而不是在 Maka 长期维护一套平行的编辑器实现。

这里还有一个重要的产品边界:当前 PR 做的是历史匹配,只是通过补全交互呈现。它不会生成新的补全内容,也不会推荐新的 Prompt。

newestPrefixMatch 和未来基于上下文或模型的建议,并不共享同一个决策契约:

  • 历史匹配是同步、确定、免费,并且检索已有文本;
  • 模型补全是异步、可取消、有费用,并且生成新文本;
  • Prompt 建议可以在草稿为空时出现,也不要求延续用户已经输入的前缀。

建议让决策函数的命名准确反映实际职责,例如 matchPromptHistory,不要把未来的模型补全或 Prompt 建议放进这个抽象。视觉层的 inlineCompletion seam 可以共用,但不同决策来源应保持独立。

对当前 PR,产品行为可以保持简单:用户输入前缀后,从最新的匹配历史记录中补全;草稿为空时,仍由 ArrowUp 负责历史召回。这样不会把最新历史 Prompt 包装成一条新的建议。

建议保留 matcher 和测试,但用输入组件拥有的 inline completion 替换 mirror、DOM 状态推断、写回路径和第二份历史缓存。

@ARE404
ARE404 force-pushed the are404/feat-composer-inline-suggestion branch 2 times, most recently from 430dc4e to 4fb507c Compare August 8, 2026 05:13
@ARE404

ARE404 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Taken, all of it. The mirror is gone; the offer now lives inside ChatComposerInput.

P1 — Tab could insert text the user never saw

Confirmed before changing anything, because it deserved a number rather than agreement. Draft 帮我审查, one history entry of 120 characters:

before after
completion offered 116 chars 116 chars
visible 57 116
inserted by Tab 116 116
field height 30px (clipped, scrollHeight 74) 74px (scrollHeight === clientHeight)

59 characters used to enter the draft having never been on screen. The mirror's height was copied from the editable, which is sized by the draft — so the guard I had (scrollHeight > clientHeight) could only ever see a draft that already scrolled, never the overflow the completion itself caused. Inside the editor there is no such gap: the field grows around the offer because it is the same text in the same box.

P1 — Tab changed meaning for screen-reader users

The editor now carries a polite live region, because it is the thing that knows when an offer is really on screen. inlineCompletionLabel supplies the instruction — "press Tab to accept" is product copy a component library has no business inventing. Measured announcement: the completion text followed by 按 Tab 键补全,按 Esc 键忽略, cleared on Escape and on acceptance. Escape dismisses the offer and leaves the draft untouched.

The seam

patches/@astryxdesign+core+0.3.0.patch, alongside the conversationKey patch already in that file, with the reason, the deletion condition and the guard the README asks for. Guard is packages/ui/src/__tests__/astryx-inline-completion.test.tsx: an upstream that does not know the prop lets it fall into ...rest and renders a stray inlinecompletion attribute, so both cases fail loudly against an unpatched package — verified by unpatching and watching them go red. I will open the upstream issue on facebook/astryx and link it here; if you would rather that land first and this wait on it, say so and I will park the PR.

Deleted with the mirror: use-composer-suggestion.ts (205 lines), composer-suggestion-overlay.tsx, mirrorMetrics / getComputedStyle inference, the aria-expanded sniffing, the execCommand writeback, the second history cache and the 35-line stylesheet. composer.tsx goes from +63 to +10, and its whole contribution is now one line of "what to offer".

Naming and the product boundary

matchPromptHistory(draft, entries), in prompt-history-match.ts. The module says in as many words that this retrieves text the user already sent, that a model completion and a prompt recommendation are different decision contracts, and that only the visual seam is worth sharing. I have dropped the "a model provider slots in behind this later" framing from the PR description — you are right that it was doing work the abstraction could not honestly carry.

useComposerHistory is the only owner: it exposes matchCompletion(draft), and input-history.ts notifies on both of its writes so the owner re-reads instead of a second copy going stale. Empty draft still belongs to ArrowUp, untouched.

Checks

@maka/ui 502/502, @maka/desktop 1812/1812, typecheck, format:check, lint, check-dead-css, check-story-annotations clean. The composer-inline-suggestion play story is rewritten against the new behaviour and now reads the draft with the offer removed rather than through textContent — the offer is a child of the editable, so textContent cannot tell "painted" from "accepted", and every assertion would have passed on a component that committed nothing. That bug was in my previous story; it is the same class of thing you flagged, and it is fixed.

@ARE404

ARE404 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Upstream RFC filed: facebook/astryx#4822ChatComposerInput: a seam for inline completion (ghost text), with the measured clipping case as the argument for why it belongs inside the editor, and an offer to send the implementation as a PR since it is already written behind the patch.

Linked from the patch's README section, and it is now the deletion condition: drop the patch when #4822 ships and astryx-inline-completion.test.tsx passes unpatched. Not waiting on it to land.

@ARE404 ARE404 closed this Aug 8, 2026
@ARE404 ARE404 reopened this Aug 8, 2026
@ARE404
ARE404 force-pushed the are404/feat-composer-inline-suggestion branch from 9007a96 to 70a6db0 Compare August 8, 2026 06:36
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for revisiting this — the product boundary is much clearer now. This is a deterministic prompt-history prefix match, rather than a prompt recommendation or generative completion. Moving history ownership back into useComposerHistory is also a substantial improvement.

I agree that the generic inline-completion behavior belongs inside ChatComposerInput: only the input primitive owns the selection, composition, trigger-menu, serialization, wrapping, and Tab/Escape semantics. Since upstreaming to Astryx is not currently practical, a small downstream patch is a reasonable solution.

I would keep that patch as a minimal, product-agnostic Astryx primitive:

  • Maka decides only which suffix to offer.
  • ChatComposerInput decides whether the offer is valid and visible.
  • The patch owns focus, selection, IME, trigger-menu priority, acceptance, dismissal, serialization, and accessibility.
  • No prompt-history concepts should enter the patch.

There are two lifecycle gaps in the current implementation:

P2 — The completion can become stale after the caret moves

The offer is validated during render, but ArrowLeft or a mouse selection change may not cause another render. Tab then trusts the cached suffix and can insert it in the middle of the draft. Blur can leave a similarly stale offer.

Please clear the offer when focus or selection leaves the valid append-only position, and synchronously re-check focus, collapsed selection, and caret-at-end immediately before accepting it.

P2 — IME composition needs an explicit guard

The patch says the primitive owns composition state, but the current implementation does not track it. The offer may remain active during Chinese, Japanese, or Korean composition, and the Tab path does not re-check isComposing.

Please clear and suppress the offer on composition start, recompute after composition ends, and leave acceptance keys alone while composition is active.

I think the cleanest implementation is a small event-driven offer lifecycle inside ChatComposerInput, rather than removing and rebuilding the span on every render:

candidate → eligible and visible → blur / selection move / composition start / menu open clears it → Tab revalidates and accepts

The current architecture does not need to be replaced. It just needs to fully implement the selection and composition contract it already claims to own.

For tests, I would keep the prompt matcher cases, add one real primitive-level interaction guard covering caret movement, blur, IME, menu priority, Tab/Escape, and serialization, and keep one Maka Story/E2E for the history wiring and visual result. The two SSR cases in astryx-inline-completion.test.tsx appear largely duplicative; one can be removed or replaced with a test of the actual lifecycle contract.

After that, please rebase onto current main, rerun CI, and, if convenient, include a short recording covering caret movement, blur/refocus, and IME input. Thanks!

中文对照

感谢重新调整这一版。现在产品边界已经清楚很多:它是确定性的历史 Prompt 前缀匹配,不是 Prompt 推荐,也不是生成式补全。由 useComposerHistory 统一管理历史也是很明显的改进。

我认同把通用 inline completion 放进 ChatComposerInput:只有输入原语真正拥有选区、输入法组词、触发菜单、序列化、换行,以及 Tab/Escape 的语义。在暂时无法 upstream 到 Astryx 的前提下,维护一个小型下游 patch 是合理方案。

建议把这个 patch 严格限制为最小、与产品无关的 Astryx 原语:

  • Maka 只决定提供哪个 suffix。
  • ChatComposerInput 决定建议是否有效、是否显示。
  • patch 负责焦点、选区、IME、菜单优先级、接受、隐藏、序列化和可访问性。
  • patch 内不应出现 Prompt history 的产品概念。

当前实现还有两个生命周期缺口:

P2 — 光标移动后,补全可能继续保留旧状态

当前只在渲染时验证 offer。用户按 ArrowLeft 或用鼠标改变选区时,不一定会触发重新渲染;随后按 Tab,缓存的 suffix 可能被插入草稿中间。失焦后也可能留下类似的过期建议。

建议在焦点或选区离开“仅在末尾追加”的有效位置时清除 offer,并在真正接受前同步重新检查焦点、折叠选区和光标是否位于末尾。

P2 — 需要明确处理 IME composition

patch 的设计说明称原语拥有 composition 状态,但当前代码并没有跟踪它。中文、日文或韩文输入法组词期间仍可能保留建议,Tab 路径也没有重新检查 isComposing

建议在 composition start 时清除并暂停建议,composition end 后重新计算;composition 期间不要拦截补全快捷键。

更干净的实现是让 ChatComposerInput 内部维护一个很小的事件驱动生命周期,而不是每次 render 都删除并重建 span:

候选建议 → 满足条件并显示 → 失焦 / 选区移动 / 开始组词 / 菜单打开时清除 → Tab 重新校验后接受

当前架构不需要推翻,只需真正补全它已经声明要负责的 selection 和 composition 契约。

测试方面,建议保留 Prompt matcher 用例;增加一个真实的原语交互门禁,覆盖光标移动、失焦、IME、菜单优先级、Tab/Escape 和序列化;Maka 侧保留一个 Story/E2E 验证历史接线和视觉结果。astryx-inline-completion.test.tsx 中两个 SSR 用例基本重复,可以删除一个,或改成验证真正的生命周期契约。

完成后请 rebase 到最新 main、重新运行 CI;如果方便,也可以附一段简短录屏,覆盖光标移动、失焦/重新聚焦和 IME 输入。谢谢!

@ARE404
ARE404 force-pushed the are404/feat-composer-inline-suggestion branch 2 times, most recently from 25b6b7a to b954bb9 Compare August 9, 2026 04:32
@ARE404

ARE404 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main (44 commits); the patches/README.md conflict was additive — your astryx-layer-surface section kept, mine appended. Checked the shared patch file for a silent revert while I was there: git diff upstream/main --numstat on it is 254 / 0, and the merged patch applies clean to a pristine @astryxdesign/core (all three sections ✔), so nothing of yours was dropped.

The storybook job is red for a reason that is not this PR. Design System/Icons throws on mount: the gallery enumerates icons.tsx and mounts every export, and #2538 added ICON_SIZE — enumerated at runtime it is the only non-component of 112 exports, so React rejects it (#130) and the story takes the smoke run with it. Neither icons.tsx nor that story is in this diff. It is invisible on main because the storybook job is path-gated and was skipped on those runs.

Split out as #2559 rather than folded in here, since it has nothing to do with this feature. With it applied the smoke passes end to end locally (48 manifest checks, 101 catalog renders); this branch's own story passes either way.

Local on this head: @maka/ui 273/273, @maka/desktop 1131/1131, typecheck / format:check / lint / check-dead-css / check-story-annotations clean.

@ARE404

ARE404 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

#2574 landed while this was in review, and it changes the ground this PR stands on. I'd like your call before rebasing onto it.

The patches directory now says: keep it small, prefer product code against the published API, only patch for bugs that block shipping and cannot be worked around at the call site. The Astryx patch went 490 lines → 32, and the per-patch guard tests went with the sections they covered.

This PR adds an Astryx patch for a capability, not a blocking bug. Under that rule it does not qualify — so I stopped rather than rebase a ~250-line patch onto the file you just trimmed.

How it got here, briefly: your round-2 review asked for the completion to be input-owned rather than a sibling of the editor, and noted you'd prefer the upstream capability first if Astryx didn't expose the seam. It doesn't. I filed facebook/astryx#4822 and, rather than block on Meta, patched locally — that was my call, and #2574 is a reasonable answer to it.

Three ways forward as I see them:

  1. Park this until #4822 ships. Consistent with both your original preference and the new rule. Cost is unbounded — it's Meta's queue.
  2. Keep the patch as a stated exception. I'd re-cut it onto the 32-line file and rewrite the README section in the new, terser voice. Tell me whether you want a guard test back for it, or whether that machinery is gone on purpose.
  3. A published-API route I haven't found. I looked: the offer has to sit inside the editable to wrap and scroll with the draft, and serialize would carry it into the value; ChatComposerToken is part of the value by design. If there's a seam I missed, that's the best outcome and I'll take it.

My preference is (2) if you're comfortable with the exception, (1) if not — but it's your directory and your policy, so I'd rather ask than assume.

(Unrelated: the storybook red here was design-system-icons--lucide-icons, fixed on main by #2544 — I closed my duplicate #2559. A second failure, daily-review-model-selector-open-narrow @ floor, is Linux-only and may be covered by #2556; I'll confirm from the next run.)

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks again for taking the earlier feedback seriously. I revisited this after #2574 tightened the patch policy, and I found a smaller path. My earlier comment correctly identified that the suggestion should use the real editor’s layout, but I was too quick to conclude that this required a new Astryx capability—sorry for sending you toward a dependency patch.

I think we can preserve the current UX with the stock ChatComposerInput:

  • keep useComposerHistory.matchCompletion() as the history owner;
  • render the suffix through ::after on the actual nested contenteditable, passing it from the input root through an inherited CSS custom property;
  • reuse Composer’s existing focus, selection and composition tracking to decide when it is eligible;
  • handle bare Tab through Astryx’s public onKeyDown, which already runs after the trigger menu;
  • keep the accessible instruction in a product-owned aria-live region.

I verified in Chromium that generated content participates in the real editor’s wrapping and height while remaining outside textContent, serialization and the caret. This avoids both the external mirror and an ephemeral DOM node inside the editor.

A few details seem important:

  • CSS content needs a small tested encoder for newlines, quotes and backslashes.
  • After painting, measure the editable in a layout effect and suppress the offer if it exceeds the visible maxRows area. The current patched span otherwise still appears able to show only part of a very long completion while Tab accepts all of it.
  • Tab should revalidate against the live draft before accepting.
  • One correction to my earlier sketch: handleRef.insertText() currently performs a Range insertion without emitting input/onChange, so it can leave the controlled draft stale and does not provide reliable undo. The existing execCommand('insertText') path in Composer is the safer acceptance path here; it already feeds the controlled change flow and handles multiline insertion through the existing beforeinput logic.

This keeps one real editor and the same visible interaction, while removing the roughly 250-line Astryx patch. The structural selector is a narrow dependency, but Composer and its E2E fixtures already rely on that exact contenteditable, so it does not introduce a new coupling level.

Would you be open to rebasing and reshaping the PR this way? The upstream RFC can remain open as a future first-class seam, but I don’t think this PR needs to wait for it.

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

Codex automated review

I reviewed exact head b954bb9c87d0517b8376ca96b9407cbb89b18c9f against the prior review ledger, following history authority, trigger-menu ownership, DOM painting/serialization, caret/selection/focus, IME composition, Tab/Escape routing, and the Desktop story. The earlier matcher/cache issues are addressed, but two input-lifecycle bugs remain; see the P2 inline findings.

This PR is currently conflicting, and Storybook is red on unrelated existing stories. Please rebase and rerun the full checks. The embedded Astryx patch also contains substantial ChatLayout/ChatToolCalls/Kbd/List/Popover/useHotkeys changes unrelated to composer completion; those should be split/rebased out so this PR contains only the completion primitive plus its Maka integration/tests. The line count itself is not the issue—the mixed upstream patch owner is.

Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.

Comment thread patches/@astryxdesign+core+0.3.0.patch Outdated
Comment thread patches/@astryxdesign+core+0.3.0.patch Outdated
@github-actions
github-actions Bot requested a lite review from Copilot August 16, 2026 16:52
@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

An error occurred during the review process. Please try again later.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Summary

This PR adds inline composer completion from submitted prompt history. When the draft matches a history-entry prefix, the unmatched suffix appears in ChatComposerInput. Bare Tab accepts the suffix. Escape dismisses it. Empty drafts retain ArrowUp history recall.

The PR extends useComposerHistory as the source of truth through matchPromptHistory and matchCompletion. It does not create a separate completion cache or editor writeback path. Global history subscriptions support updates from other same-document consumers. The patched ChatComposerInput provides rendering, accessibility, serialization, focus, selection, trigger-menu, and IME behavior.

The solution is the smallest coherent implementation shown by the current changes. The global subscription and downstream Astryx patch add necessary complexity. No feature code or regression coverage can be safely deleted based on the supplied changes. The unrelated chat-layout, streaming-Markdown, tool-call, and platform-detection changes in the Astryx patch should be removed or split if they are not required by this feature.

Matcher tests cover prefix rules, case handling, minimum draft length, trigger-prefixed prompts, and multiline drafts. UI and end-to-end tests cover rendering, serialization, acceptance, clipping, dismissal, caret movement, focus changes, accessibility, trigger-menu priority, streaming Escape, and IME behavior. Required checks remain unverified because direct validation evidence was not provided.

Review-relevant risks

  • New public APIs include matchPromptHistory, matchCompletion, subscribeGlobalInputHistory, and ConversationCopy.composer.inlineCompletionHint. These contract changes require independent human review under repository policy.
  • Patched ChatComposerInput props change the package contract. This change requires independent human review under repository policy.
  • Composer Tab, Escape, accessibility, focus, selection, and IME behavior change user-visible behavior. These changes require independent human review under repository policy.
  • The Astryx patch changes the package release from 0.3.0 to 0.4.0 and adds unrelated public declarations. Release and governance effects require independent human review under repository policy.
  • No security or licensing effect was identified in the current diff.
  • Required checks are unverified. The person performing the merge reviews the final diff. A maintainer makes the final determination.

Walkthrough

The change adds prompt-history inline completion to the composer, including rendering, accessibility, Tab acceptance, dismissal, and interaction validation. It also updates patched chat layout, streaming text, platform detection, and list accessibility behavior.

Changes

Composer inline completion

Layer / File(s) Summary
History matching and subscriptions
packages/ui/src/prompt-history-match.ts, packages/ui/src/input-history.ts, packages/ui/src/use-composer-history.ts, packages/ui/src/__tests__/prompt-history-match.test.ts
Prompt history matching returns completion suffixes. Global history changes refresh the composer history hook. Tests cover matching and rejection cases.
Composer wiring and accessibility copy
packages/ui/src/composer.tsx, packages/ui/src/conversation-copy.ts
The composer passes matching completion text and localized accessibility copy to ChatComposerInput.
Inline input behavior and validation
patches/@astryxdesign+core+0.4.0.patch, packages/ui/src/__tests__/astryx-inline-completion.test.tsx
ChatComposerInput renders non-serialized suggestions, supports Tab and Escape, handles caret and IME state, and announces suggestions.
Inline completion interaction validation
apps/desktop/e2e/composer-inline-completion.spec.ts, apps/desktop/stories/app-shell.stories.tsx, patches/README.md
End-to-end tests and Storybook cover display, acceptance, dismissal, focus, caret, IME, menu, overflow, and streaming interactions. Patch documentation describes the new input seams.

Core chat patch updates

Layer / File(s) Summary
Conversation scroll and message state
patches/@astryxdesign+core+0.4.0.patch
Chat layout resets conversation and unread state when the conversation identity changes and exposes auto-follow controls.
Settled streaming text
patches/@astryxdesign+core+0.4.0.patch
Markdown and streaming text accept settled content and reconcile prefixes, Unicode boundaries, stream state, and fade boundaries.
Platform and surface behavior
patches/@astryxdesign+core+0.4.0.patch
The patch adds tool-call row metadata, forwards list labels, and hardens Apple-platform detection.

Estimated code review effort: 4 (Complex) | ~60 minutes

Merge Risk: 🟡 Moderate · up to 8401f

Inline completion can consume the first Escape key while a suggestion is visible, delaying cancellation of an active streaming turn until a second press; this interaction issue should be resolved or explicitly accepted before merging.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Composer
  participant useComposerHistory
  participant ChatComposerInput
  User->>Composer: enter draft
  Composer->>useComposerHistory: matchCompletion(draft)
  useComposerHistory-->>Composer: return completion suffix
  Composer->>ChatComposerInput: render inlineCompletion
  User->>ChatComposerInput: press Tab
  ChatComposerInput->>Composer: commit completion
Loading

Suggested reviewers: astro-han

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Ai Use Disclosure ⚠️ Warning The PR description selects neither AI-use declaration, and all five PR commits have no Generated-by trailer. Select exactly one AI-use option. If generative tooling made a substantive contribution, name the tool and scope; follow CONTRIBUTING.md’s “Human ownership and AI attribution” section.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly states that the change completes composer drafts from prompt history with Tab, matching the PR's primary behavior.
Description check ✅ Passed The description explains the problem, implementation, behavior, tests, AI contribution, and review context, despite using custom headings.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404

ARE404 commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Both P2s fixed in 3e91a6d38 (still on the un-rebased head — the re-graft onto the trimmed patch file is what I'm holding for your answer above).

P2 selection revalidation. Root cause was narrower than "check on commit": painting was driven only by React commits, and moving a caret is not a state change, so nothing ever withdrew a standing offer. Now re-decided on selectionchange and focusout, and revalidated synchronously inside the accept path — a failed revalidation withdraws the offer and returns false, so preventDefault runs only on a real commit and Tab otherwise keeps its focus move. caretAtContentEnd also had to stop counting a standing offer as content after the caret, or the revalidation could never pass.

Writing the regression surfaced the mirror of it, which the finding didn't name: a caret returning to the end brought no offer back until the next keystroke. So the listener re-decides rather than only withdraws.

P2 IME composition. Tracked inside the primitive, for the reason you gave — Maka guards its own handlers, but this is a published capability and a direct consumer has none. Offer withdrawn at compositionstart, never painted while composing, and isComposing / keyCode 229 / Process all leave the key untouched.

Both pinned in the play story: caret moved off the end then Tab (draft unchanged); and compositionstart then a composing Tab, asserting it was not preventDefaulted — i.e. that it reaches the IME at all. Storybook smoke passes on this story; @maka/ui 273/273; format / lint clean.

On the automated review's third point — the patch carrying ChatLayout / ChatToolCalls / Kbd / List / Popover / useHotkeys: those aren't authored here. patch-package emits one file per package, so it necessarily contains whatever the repo already patched; git diff against the base on that file was +254 / −0. It also resolves itself on rebase, since #2574 has since trimmed that file to the UA-CH section alone.

The remaining Storybook red on this branch is design-system-icons--lucide-icons, already fixed on main by #2544 — this branch just predates it.

Copilot AI 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.

Pull request overview

Adds prompt-history inline completion to the chat composer: when the current draft prefixes a previous prompt, the remainder is offered as dim “ghost” text after the caret and can be committed with Tab, using the same persisted maka-input-history source already used for ArrowUp recall.

Changes:

  • Introduces a pure history-matching function (matchPromptHistory) and wires it into the composer via useComposerHistory.
  • Adds same-document subscription for input-history writes/clears so the UI can update immediately when history changes.
  • Patches @astryxdesign/core ChatComposerInput to render/announce/commit an inline completion inside the editor; adds copy, tests, and Storybook smoke coverage.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
patches/README.md Documents the new Astryx core patch rationale and removal conditions.
patches/@astryxdesign+core+0.3.0.patch Adds inlineCompletion/inlineCompletionLabel support in ChatComposerInput (rendered inside the editor, Tab/Escape behavior, a11y announcement).
packages/ui/src/use-composer-history.ts Exposes matchCompletion() and subscribes to global input-history updates to keep suggestions in sync.
packages/ui/src/prompt-history-match.ts Implements pure “newest prefix match” logic returning only the suffix.
packages/ui/src/input-history.ts Adds subscribeGlobalInputHistory() and notifies listeners on save/clear.
packages/ui/src/conversation-copy.ts Adds localized inlineCompletionHint copy for screen reader announcements.
packages/ui/src/composer.tsx Wires inlineCompletion props into ChatComposerInput using history matching.
packages/ui/src/tests/prompt-history-match.test.ts Unit tests for matching behavior (newest match, suffix-only, case pass, min-length, multiline).
packages/ui/src/tests/astryx-inline-completion.test.tsx Guard tests ensuring the patched props are consumed and not leaked to DOM/SSR value.
apps/desktop/stories/product-smoke-manifest.json Adds a smoke story entry for inline suggestion behavior.
apps/desktop/stories/app-shell.stories.tsx Adds a play-driven Storybook test story covering offer rendering and Tab/Escape behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread patches/@astryxdesign+core+0.3.0.patch Outdated
Comment thread apps/desktop/stories/app-shell.stories.tsx Outdated

Copilot AI 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.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

patches/README.md:31

  • The section says "Four published component seams" but the list now contains five bullets (including the newly added ChatComposerInput inline completion). Update the count to match the list to avoid misleading future patch maintenance.
Four published component seams drop host-owned state or semantics:

@hqhq1025
hqhq1025 requested a lite review from Copilot August 17, 2026 11:26
@ARE404

ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Right again — fixed in ba5a70006. Adding my bullet made the list five while the sentence above it still said four; I updated the list and not the sentence that counts it.

Small, but it sits in the file you read to decide which hunks may be dropped, so a count that disagrees with its own list is worse there than no count at all.

That is four consecutive rounds where the visible verdict was "no new comments" and the actual finding was in the suppressed list — and every one of them was a real inconsistency of mine. If that list is as easy to skip on your side as it looks, it is worth knowing that the useful output has been hiding in it.

Green on ba5a70006: e2e 9/9, @maka/ui 179/179, build, typecheck, format, lint.

Copilot AI 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.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

ARE404 added 12 commits August 17, 2026 19:57
The composer had no way to reuse a prompt short of ArrowUp-ing through the
whole history list. This offers the rest of a matching past prompt as dim
text after the caret; Tab commits it, Escape dismisses it, anything else
ignores it.

What is offered is history, retrieved: `matchPromptHistory` returns the
remainder of the newest entry the draft is a prefix of. It is not a
generated completion and not a prompt recommendation — those are
asynchronous, metered, and need not extend a typed prefix — so it is named
and scoped as the retrieval it is, and `useComposerHistory` stays the one
owner of the entries. A second cached copy of that list is what would let a
prompt cleared from Settings · 数据 be completed back into a draft, so there
isn't one: the module notifies, the owner re-reads.

Where it is offered is the editor's business, and the offer now lives inside
it. `ChatComposerInput` gains `inlineCompletion` (patched into Astryx, see
patches/README.md): a contenteditable=false span in the real flow, skipped by
`serialize` so it never reaches the value. Drawn outside as an overlay, the
preview and the insertion were two layouts agreeing by luck — measured, a
one-row field offering a 116-character completion showed 57 characters and
Tab inserted all 116. Inside, the field grows and wraps around the offer, so
what is shown and what Tab commits are the same text by construction; the
same measurement now shows 116 of 116.

Ownership follows: the editor knows when an offer is really on screen, so it
carries the polite live region that announces it and how to accept it —
Tab no longer changes meaning for a screen-reader user without saying so.
Selection, IME composition, wrapping, scrolling and whether an open trigger
menu already owns Tab are all decided where those states are authoritative,
which leaves the composer with one line: what to offer.

Generated-by: Claude Code
… the last render

Two ways the offer could act on state it no longer described, both from the
same root: painting was driven only by React commits, and moving a caret is
not a state change.

**Tab could splice the offer mid-draft.** After an offer was shown, clicking
or arrowing into the middle left it standing — no render, so nothing
withdrew it — and Tab still committed, with `insertTextAtCursor` putting it
wherever the caret now was: `abc` + `def` became `abdefc`. The selection is
now re-decided on `selectionchange` and `focusout`, and revalidated
synchronously inside the accept path; a failed revalidation withdraws the
offer and returns false, so `preventDefault` runs only on a real commit and
Tab otherwise keeps its ordinary focus move.

**A composing Tab could commit into the half-built character.** Composition
is the editor's own state, so it is tracked here rather than in whatever a
consumer happens to keep — Maka guards its own handlers, but this is a
published capability and a direct consumer has none. The offer is withdrawn
at `compositionstart`, never painted while composing, and every composing
key (`isComposing`, keyCode 229, `Process`) is left untouched.

Re-deciding rather than only withdrawing also fixes the mirror of the first
bug, which the review did not name and the regression found: a caret
returning to the end brought no offer back until the next keystroke.
`caretAtContentEnd` now excludes a standing offer from what counts as
content after the caret, so the same question can be asked before the offer
is painted and while it stands.

Both are pinned in the play story: caret moved off the end, then Tab; and a
`compositionstart` followed by a composing Tab, asserting it was not
`preventDefault`ed — that it reaches the IME at all.

Generated-by: Claude Code
… say

Two rendering defects in the inline-completion patch, both found by review.

The announcement was very likely never spoken. The live region was rendered
only while it had text, so the region and its content arrived in the same
commit — screen readers generally announce changes to regions that already
exist in the accessibility tree, and one that appears with its text is
silent. That is the whole of the accessibility fix this patch exists for, so
it is now mounted unconditionally and only its text changes; the story
asserts the region is present before any offer is made.

The offer could also land on the wrong line. A contentEditable keeps a
trailing filler `<br>`, which contributes no text — so the caret still reads
as being at the end of the content, but `appendChild` put the offer after the
break and drew it on the next line. Insert before the filler when there is
one.

Escape keeping the offer ahead of a consumer's own Escape is left as is, and
said so in the code: the offer is the most local thing on screen and the one
the user just summoned by typing, which is the order Escape unwinds
everywhere else.

Generated-by: Claude Code
`syncInlineCompletion` runs after every render with no dependency array —
the decision reads the live selection, which a render does not announce — and
it tore the offer down and rebuilt it unconditionally. The composer's input is
controlled, so that was one span destroyed and recreated per character typed.

Return early when the offer already on screen is the one this pass would
draw and every condition that put it there still holds. The teardown stays
for every case that actually changes something, including the controlled
rewrite that wipes the span out from under us.

Also collapses a duplicated JSDoc block left on `waitForComposer` by an
earlier rebase.

Generated-by: Claude Code
…it where CI runs

Four review findings, and the first two are the same invariant twice.

**A candidate is not an offer until it fits.** Moving the completion inside
the editable made the preview and the insertion one layout, but the field is
still capped by `maxRows` and scrolls: a long candidate lays out past the
bottom, unreadable, while Tab committed all of it — 26,000 characters against
roughly 214px of visible offer. The candidate is now laid out, measured, and
only promoted to an active offer if the whole span sits inside the field's
visible box; otherwise it is withdrawn and Tab keeps its ordinary meaning.
Acceptance revalidates the same condition. The judgement stays in the editor,
which is the only thing that knows the layout — the matcher still returns
whole history entries.

**Focus returning is an input to the same decision.** `focusout` withdrew but
nothing re-asked, and refocusing fires neither `selectionchange` nor a render,
so a valid candidate stayed withdrawn while the caret sat still. `focusin`
now routes through the one reconciliation owner rather than adding a flag.

**Escape belongs to the host.** The input withdraws what it drew and
remembers the dismissal, then lets the key continue — no `preventDefault`, no
early return — so a composer that stops a streaming turn on Escape still does
it on the first press.

**The contract now lives where it executes.** The Storybook smoke opens
stories in embedded mode, which disables autoplay, so the play function
proved only that the composer mounts. `composer-inline-completion.spec.ts`
covers visible acceptance, a clipped offer leaving Tab untouched,
blur/refocus, composition, caret movement, trigger-menu priority and
streaming Escape; the story is reduced to the review driver it actually is.

Generated-by: Claude Code
Tab acceptance was covered; its complement was not. The premise of the
feature is that Tab commits and every other key ignores the offer, so the
keys that must NOT commit are half the contract: Shift+Tab is back-tab and
leaves the field, and a printable key keeps typing — in both cases nothing of
the offered suffix may reach the value.

Generated-by: Claude Code
A document-wide lookup can pick up a composer from another mounted story,
which would drive the wrong one. Query the canvas the story owns.

Generated-by: Claude Code
The previous rounds fixed the symptoms one at a time; this collapses what was
producing them. An offer was several parallel facts — a candidate prop, a DOM
span, an active ref, a dismissal, an announcement, a stored history spelling —
none bound to the draft they described, so any two of them could disagree.

An offer is now `{ base, suffix }`: the candidate together with the exact
serialized draft it was derived from. Every question worth asking about it is
really a question about whether the editor still holds that draft, and
`withdrawOffer` / `reconcileOffer` / `acceptOffer` own the node, the record,
the dismissal and the announcement together.

Three defects fall out of that, all reproduced:

Accepting was not undoable. `Range.insertNode` is not on Chromium's
contentEditable undo stack, so Ctrl/Cmd+Z after Tab took back the character
the user had typed and left the completion standing. Acceptance now goes
through `execCommand('insertText')` — the browser's own editing command, which
undo and redo already understand — after synchronously confirming the editor
still serializes to the candidate's base draft.

Reconciliation ran before the controlled-value sync, because it was a layout
effect and that sync is a passive one. A programmatic `setText` could promote
and announce an offer against a draft the editor was about to replace, after
which the rewrite removed the span and left the record claiming it. It is a
passive effect now, declared after, so it reconciles against the DOM the value
actually produced.

Dismissal was keyed by suffix alone, so it outlived the draft it was about;
and history stored ASCII spaces where the editor holds U+00A0 token anchors,
so a prompt could fail to match itself. Both sides now canonicalize through
one `canonicalizePromptText`, a character-for-character swap that preserves
offsets, so the suffix is still sliced from the stored entry.

Covered by two new Electron journeys — Tab/Undo/Redo, and a programmatic draft
replacement invalidating a live offer — bringing the spec to 9.

Generated-by: Claude Code
Four consistency defects, all mine, all left behind by earlier rounds.

The reconcile function still carried a block comment calling itself a layout
effect, while the comment below it explained why it must be passive — the same
function documented two contradictory ways, which is worse than either. The
`useLayoutEffect` import it needed went unused with it, and a dead import in a
patch against someone else's library reads as if something still depends on it.

The patch guard's own header pointed at `0.3.0`; the artifact it guards is
`0.4.0`. A guard naming the wrong thing is a guard nobody can check.

`notifyListeners` called subscribers bare, so the first one to throw skipped
the rest and surfaced out of a `save` that had already written — a subscriber
bug reading as a storage failure while other holders stayed stale. Each call
is isolated now. One subscriber exists today; the cost is four lines and the
symptom would have been hard to place.

No behaviour change.

Generated-by: Claude Code
…ntries

The subscription swapped `entries` and left `index` and `savedDraft` alone,
which is wrong exactly when it matters. Clearing history from Settings · 数据
mid-navigation left the index pointing into a list that no longer holds those
entries: the composer went on showing a prompt the user had just deleted, the
hook still believed it was navigating, and the user's own draft sat stranded
in `savedDraft` until an arrow key happened to reconcile it.

`reconcileHistorySync` already decides all of this — clamp or reset the index,
and say when the saved draft is owed back — and is already unit-tested. This
deletes the ad-hoc swap and calls it, restoring the draft on the spot when it
says so.

Generated-by: Claude Code
…ender

The subscription registers once, so the restore path was calling an
`applyValue` captured from the first render. It reaches the current one
through a ref now.

Nothing observable changes today, and the reported failure modes do not
actually occur: the text port is created once and held in a ref, and
`saveCurrentDraft` reads the active draft key from a ref at call time, so a
stale closure could not restore into the wrong port or persist under a stale
key. What it could capture is the `persistence` object handed in that render.

Fixing it anyway, because the guarantee that made it safe belongs to the draft
hook rather than to this subscription — this file cannot see that property
change, and a listener registered once should not depend on one.

Generated-by: Claude Code
Adding the inline-completion bullet made it five; the sentence above it still
said four. A count that disagrees with its own list is worse than no count,
and this one sits in the file a maintainer reads to decide what may be
dropped.

Generated-by: Claude Code
@ARE404
ARE404 force-pushed the are404/feat-composer-inline-suggestion branch from ba5a700 to e648c6f Compare August 17, 2026 11:58
@hqhq1025
hqhq1025 requested a lite review from Copilot August 17, 2026 11:58

Copilot AI 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.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/ui/src/use-composer-history.ts:94

  • applyValueRef is updated via a passive useEffect, which means the global-history subscription callback can run (via synchronous notifyListeners() during a write) before this effect has a chance to update the ref. Since saveCurrentDraft is re-created every render (see useComposerDraft), this can reintroduce a stale-closure window. Assigning the ref during render removes the race and still keeps the subscription wired to the latest applyValue.
  useEffect(() => {
    applyValueRef.current = applyValue;
  });

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

The previous lifecycle findings are substantially fixed on e648c6f2: the offer is now draft-bound, controlled rewrites reconcile through one owner, acceptance is undoable, history clearing reconciles the whole state, and NBSP matching uses one canonical representation. The architecture is now coherent and I’m approving the PR.

One concrete P2 remains and should be fixed here if practical, or tracked as an immediate follow-up:

[P2] Preserve line breaks when accepting a multi-line completion

acceptOffer() passes the complete suffix to execCommand('insertText'). In Chromium, inserting " line1\nline2" produces base line1<div>line2</div>, while Astryx’s serializer recursively reads the div without restoring the newline, yielding base line1line2. This command also emits no beforeinput event, so the Composer’s existing multi-line insertion handler cannot correct it.

The contained fix is to split the suffix on \n and issue insertLineBreak between text segments within the same acceptance handler; Chromium retains that sequence as one undoable transaction. A focused multi-line Tab → Undo → Redo Electron journey should pin the exact-value contract.

This does not require another architectural change or a PR split. Apart from this bounded case, I found no remaining P0–P2 lifecycle regression.

AI-assisted review: Codex reconstructed the previous review ledger, inspected the latest diff and CI evidence, and independently reproduced the Chromium editing/serialization behavior. The human reviewer reviewed this conclusion, chose to approve, and remains responsible for the final review and merge decision.

中文版

前序生命周期问题已经基本修复:offer 已绑定精确草稿,controlled rewrite、撤销事务、history clear 和 NBSP 规范化都已收敛到正确 owner。整体架构已经一致,因此本次给出 Approve。

仍保留一个明确的 P2:多行 suffix 经 execCommand('insertText') 接受后会生成嵌套 div,而现有 serializer 不会恢复换行,最终可能把多行 Prompt 拼成一行;该命令也不会触发现有的 beforeinput 补救逻辑。

最小修复是在同一次接受处理中按换行拆分文本,以 insertLineBreak 连接各段,并补一条多行 Tab → Undo → Redo Electron 测试。这个问题不需要再次调整架构或拆 PR。

`insertText` given a `\n` produces a wrapping `<div>` in Chromium, and the
serializer walks that without restoring the break — so accepting a two-line
prompt committed it joined into one. The command emits no `beforeinput`
either, so the composer's own multi-line replay never saw it.

The acceptance handler now splits the suffix and issues `insertLineBreak`
between segments, which produces the `<br>` the serializer does understand.
Chromium keeps the sequence on one undo entry, so acceptance is still a single
transaction — pinned by a multi-line Tab → Undo → Redo journey asserting the
exact value, bringing the spec to 10.

Generated-by: Claude Code
@ARE404

ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — and the P2 is fixed here rather than tracked, in 335483681, since it was contained exactly as you described.

Reproduced first: a \n handed to insertText becomes a wrapping <div>, serialize walks it without restoring the break, and a two-line prompt committed joined into one. The acceptance handler now splits the suffix and issues insertLineBreak between segments — the same replay the composer already does in its beforeinput handler, which is the shape that produces the <br> the serializer understands. Chromium keeps the sequence on one undo entry, so acceptance is still a single transaction.

Pinned by a multi-line Tab → Undo → Redo journey asserting the exact value including the newline; the spec is at 10 and green locally, with @maka/ui 179/179.

One note for the record, since it was raised as a blocking failure earlier: the settings.spec.ts titlebar-rename e2e failure on the previous head was environmental. It passed alone, the full 36-test suite passed locally in one process, my diff touches nothing in that path, and the same tests went green on the rebase with no code change. I mention it only so a future reader does not attribute it to this branch.

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

Re-reviewed on 335483681. The remaining multi-line acceptance P2 is closed.

acceptOffer() now preserves the suffix exactly by issuing insertLineBreak between text segments within the same editing transaction. This produces the <br> representation Astryx serializes as a newline while retaining one-step Undo and Redo.

The new Electron journey exercises the production Composer and verifies the complete contract: multi-line history recall, Tab acceptance, the exact value including the newline, Undo back to the typed prefix, and Redo to the complete prompt. The current CI is green.

This is the smallest complete correction at the existing acceptance owner; it adds no parallel state or lifecycle path. I found no new P0–P2 issues and consider the previous lifecycle review closed. No PR split or further architectural change is needed.

Optional documentation cleanup: the PR description still says the Electron spec contains nine journeys; it now contains ten.

AI-assisted re-review: Codex reconstructed the prior review ledger, inspected the incremental diff and current CI, and independently verified the Chromium serialization and Undo/Redo behavior. The human reviewer remains responsible for the final review and merge decision.

中文版

最新提交已经关闭多行补全 P2:接受逻辑在同一次编辑事务中以 insertLineBreak 连接文本段,Astryx 可以正确序列化换行,同时保持一次 Undo/Redo。

新增的真实 Electron journey 覆盖了多行历史、Tab 接受、精确换行、撤销和重做。当前没有新的 P0–P2,修复位于既有 acceptance owner,不需要继续调整架构或拆 PR。

PR 正文中的测试数量仍写 9,实际已为 10,可顺手更新,但不阻塞合并。

@Astro-Han
Astro-Han merged commit 18d15cb into apache:main Aug 18, 2026
12 checks passed
@ARE404

ARE404 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. Both of your follow-ups are in be876be6a, and one more that had been sitting unaddressed in a suppressed Copilot list.

  • Description count — fixed, it says ten journeys now.
  • The suppressed one: applyValueRef was being updated from a passive effect, but notifyListeners runs synchronously inside a write, so a write landing between a render and its effect flush read the previous render's closure. That is precisely the window the ref was added to close, so the ref is assigned during render now. Safe in this specific case because it is only ever read from a subscription callback and never during render, so a discarded render cannot be observed through it.

Green on be876be6a: e2e 10/10 locally, @maka/ui 179/179, format, lint, typecheck.

Note this head is one commit past the 335483681 you approved, so the approval may need re-applying — the change is the four-line ref assignment above plus the description count.

Ready to merge from my side whenever you are. Thank you for the six rounds; the ones that changed the shape of this most were the input-owned framing and the draft-bound lifecycle — neither was something I would have arrived at from the symptoms.

@Astro-Han

Copy link
Copy Markdown
Contributor

Follow-up: this feature was removed in #3292

The inline completion shipped in 0.1.11 and turned out to crash the renderer with React error #185 (Maximum update depth exceeded), component stack rooted at ChatComposerInput. It was removed in #3292 before it reached a second release.

The failure is structural rather than a missed edge case. The patched reconcileOffer ran as a dependency-free effect — deliberately, since the caret and selection state it reads are not announced by a render — while also writing React state and mutating the controlled contenteditable DOM. Its early-return depends on three live DOM readings (the offer span still present, caretAtContentEnd, offerFullyVisible). When any of those flips between two measurements, withdraw → reinsert → set state → render → effect becomes self-sustaining.

Worth recording that the review here was thorough on the dimension it examined — Tab ownership, IME composition, caret placement, undo/redo atomicity, multi-line serialization were all found and fixed. What no round asked was whether an effect that writes state on every render converges. Our automated defenses could not have caught it either: the unit test used renderToStaticMarkup (effects never run), and the ten Electron journeys all assert behavior in a settled state, none assert that rendering terminates.

The conclusion in #3292 was to remove the feature rather than add another guard, because the circular authority was the problem: React state and hand-reconciled contenteditable DOM cannot both own the offer. A published seam on the component would collapse that into one authority, which is what facebook/astryx#4822 asks for. That RFC is still open with no upstream response, so redoing this should wait on it rather than on another local patch.

Prompt-history recall (ArrowUp/ArrowDown) and history clearing are untouched and still shipping.

AI disclosure: Claude Code performed the crash analysis (symbolicating the 0.1.11 renderer bundle from the shipped app.asar) and drafted this comment; Astro-Han is the human contributor of record.

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