feat(ui): complete the composer draft from prompt history with Tab - #1874
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
4468d2d to
67110e8
Compare
|
Rebased onto main, which had meanwhile landed #1861 slice 3 — so this is now built on P2 — the matcher was deciding mention ownershipFixed, and at the layer you asked for.
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 P2 — the 500 ms cache was not invalidated by a clearMoved into P3 — only the pure matcher was coveredTwo answers, since there is no DOM test runner here:
What the migration changed
Checks
|
|
CI is green on all of it, including The cause is that the test starts a real loopback server on the fixed |
|
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 P1 — Tab changes meaning for screen-reader users without exposing the completion. The ghost is 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 Could we reduce this to a small first-class <ChatComposerInput
value={text}
inlineCompletion={suffix}
onChange={setText}
/>The ownership could then be:
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.
I would name the decision function around what it actually does, such as 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 的高度被限制为当前输入框高度,超出部分会被裁掉,但 P1 — 对屏幕阅读器用户,Tab 会在没有暴露补全信息的情况下改变含义。 ghost 被标记为 这两个问题更像同一个架构问题的症状,而不是独立的 CSS 或 ARIA 修补项。外部 mirror 和 205 行 hook,实际上是在 建议把它收敛成 <ChatComposerInput
value={text}
inlineCompletion={suffix}
onChange={setText}
/>职责可以是:
Astryx 已经为语音输入使用了临时 inline span。可以把这个模型泛化为不可编辑的 completion span:序列化时忽略,参与输入框真实布局,只有接受后才变成真实文本。如果 Astryx 尚未提供这个 seam,我更倾向于先给上游增加这个小能力,而不是在 Maka 长期维护一套平行的编辑器实现。 这里还有一个重要的产品边界:当前 PR 做的是历史匹配,只是通过补全交互呈现。它不会生成新的补全内容,也不会推荐新的 Prompt。
建议让决策函数的命名准确反映实际职责,例如 对当前 PR,产品行为可以保持简单:用户输入前缀后,从最新的匹配历史记录中补全;草稿为空时,仍由 ArrowUp 负责历史召回。这样不会把最新历史 Prompt 包装成一条新的建议。 建议保留 matcher 和测试,但用输入组件拥有的 inline completion 替换 mirror、DOM 状态推断、写回路径和第二份历史缓存。 |
430dc4e to
4fb507c
Compare
|
Taken, all of it. The mirror is gone; the offer now lives inside P1 — Tab could insert text the user never sawConfirmed before changing anything, because it deserved a number rather than agreement. Draft
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 ( P1 — Tab changed meaning for screen-reader usersThe editor now carries a polite live region, because it is the thing that knows when an offer is really on screen. The seam
Deleted with the mirror: Naming and the product boundary
Checks
|
4fb507c to
9007a96
Compare
|
Upstream RFC filed: facebook/astryx#4822 — Linked from the patch's README section, and it is now the deletion condition: drop the patch when #4822 ships and |
9007a96 to
70a6db0
Compare
|
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 I agree that the generic inline-completion behavior belongs inside I would keep that patch as a minimal, product-agnostic Astryx primitive:
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 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
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 After that, please rebase onto current 中文对照感谢重新调整这一版。现在产品边界已经清楚很多:它是确定性的历史 Prompt 前缀匹配,不是 Prompt 推荐,也不是生成式补全。由 我认同把通用 inline completion 放进 建议把这个 patch 严格限制为最小、与产品无关的 Astryx 原语:
当前实现还有两个生命周期缺口: P2 — 光标移动后,补全可能继续保留旧状态 当前只在渲染时验证 offer。用户按 ArrowLeft 或用鼠标改变选区时,不一定会触发重新渲染;随后按 Tab,缓存的 suffix 可能被插入草稿中间。失焦后也可能留下类似的过期建议。 建议在焦点或选区离开“仅在末尾追加”的有效位置时清除 offer,并在真正接受前同步重新检查焦点、折叠选区和光标是否位于末尾。 P2 — 需要明确处理 IME composition patch 的设计说明称原语拥有 composition 状态,但当前代码并没有跟踪它。中文、日文或韩文输入法组词期间仍可能保留建议,Tab 路径也没有重新检查 建议在 composition start 时清除并暂停建议,composition end 后重新计算;composition 期间不要拦截补全快捷键。 更干净的实现是让
当前架构不需要推翻,只需真正补全它已经声明要负责的 selection 和 composition 契约。 测试方面,建议保留 Prompt matcher 用例;增加一个真实的原语交互门禁,覆盖光标移动、失焦、IME、菜单优先级、Tab/Escape 和序列化;Maka 侧保留一个 Story/E2E 验证历史接线和视觉结果。 完成后请 rebase 到最新 |
25b6b7a to
b954bb9
Compare
|
Rebased onto main (44 commits); the The 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: |
|
#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:
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 |
|
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
I verified in Chromium that generated content participates in the real editor’s wrapping and height while remaining outside A few details seem important:
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 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
left a comment
There was a problem hiding this comment.
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.
|
Caution Review failedAn error occurred during the review process. Please try again later. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummaryThis PR adds inline composer completion from submitted prompt history. When the draft matches a history-entry prefix, the unmatched suffix appears in The PR extends 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
WalkthroughThe 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. ChangesComposer inline completion
Core chat patch updates
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to 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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
Both P2s fixed in 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 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 Both pinned in the play story: caret moved off the end then Tab (draft unchanged); and On the automated review's third point — the patch carrying ChatLayout / ChatToolCalls / Kbd / List / Popover / useHotkeys: those aren't authored here. The remaining Storybook red on this branch is |
There was a problem hiding this comment.
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 viauseComposerHistory. - Adds same-document subscription for input-history writes/clears so the UI can update immediately when history changes.
- Patches
@astryxdesign/coreChatComposerInputto 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.
There was a problem hiding this comment.
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:
|
Right again — fixed in 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 |
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
ba5a700 to
e648c6f
Compare
There was a problem hiding this comment.
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
applyValueRefis updated via a passiveuseEffect, which means the global-history subscription callback can run (via synchronousnotifyListeners()during a write) before this effect has a chance to update the ref. SincesaveCurrentDraftis re-created every render (seeuseComposerDraft), this can reintroduce a stale-closure window. Assigning the ref during render removes the race and still keeps the subscription wired to the latestapplyValue.
useEffect(() => {
applyValueRef.current = applyValue;
});
Astro-Han
left a comment
There was a problem hiding this comment.
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
|
Thanks — and the P2 is fixed here rather than tracked, in Reproduced first: a 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 One note for the record, since it was raised as a blocking failure earlier: the |
Astro-Han
left a comment
There was a problem hiding this comment.
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,可顺手更新,但不阻塞合并。
|
Thanks for the re-review. Both of your follow-ups are in
Green on Note this head is one commit past the 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. |
|
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 The failure is structural rather than a missed edge case. The patched 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 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 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 |
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.
Where the pieces live
matchPromptHistory(draft, entries)— the remainder of the newest history entry the draft is a prefix of. Pure, no DOM.useComposerHistory, the only holder.input-history.tsnotifies on both of its writes so the holder re-reads instead of a second copy going stale.ChatComposerInput, viainlineCompletion/inlineCompletionLabel(patched into Astryx — seepatches/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.tsxcontributes 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=falsespan thatserializeskips, 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, sowithdrawOffer/reconcileOffer/acceptOfferown the DOM node, the active record, the dismissal and the announcement together.execCommand('insertText'), notRange.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.setTextcannot promote an offer against a draft the editor is about to replace.canonicalizePromptText, NBSP → space) for storage and matching, so a draft carrying token anchors can match the history it was stored as.Keyboard contract
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...restand React renders a stray attribute.apps/desktop/e2e/composer-inline-completion.spec.ts— 10 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.