fix(ui): preserve composer paste undo history - #3787
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking issues.
The paste now uses execCommand('insertHTML') on a textContent-escaped <br> conversion to make plain-text paste a separate undo transaction, correctly leaving file/reference/IME paths unchanged.
Minor note: execCommand is deprecated but the only remaining transactional insertion primitive in this composer — consider annotating a future migration path.
No P0-P2. Checks on 7afde75d95 are blocked by base execution-host-queue failure unrelated to this change.
简体中文
该头未发现阻断,execCommand 废弃为轻度可维护性观察。7a130ef to
f28a37f
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
f28a37f to
50ab1d7
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Replaces the paste path's DOM insertion with document.execCommand('insertHTML', …) in packages/ui/src/composer.tsx, so a paste becomes its own browser undo transaction instead of a Range mutation the undo stack never sees.
The diagnosis and the seam are both right, and I want to say that plainly before the findings. The real authority for "this edit is undoable" is the browser's own editing transaction, and chatComposerSelection.ts's insertTextAtCursor writes through the Range API, which bypasses it entirely. Reaching for execCommand rather than building a parallel undo stack is the correct instinct, and it matches what this same file already does at composer.tsx:1249 with insertLineBreak. Escaping through textContent → innerHTML before the <br> substitution is also right, and I confirmed it leaves no injection path.
P2 — the paste now opens the @ / / trigger menu and swallows the next Enter.
This is the one thing I would like fixed. insertHTML fires a real input event; the Range-based path it replaces did not. Astryx's handleInput (ChatComposerInput.tsx:504-507) does not just emitChange() — it also calls triggerMenu.handleInput(). findActiveTrigger (useTriggerMenu.tsx:200-217) then scans backwards from the caret and activates on any trigger character that sits at the start of the text or after a space or newline. This composer registers exactly two: @ at composer.tsx:892 and / at :926.
Concrete: caret in an empty composer, paste /Users/me/notes.txt. The backward scan hits / at index 0, prevChar is null, the slash-command menu opens. The user presses Enter to send; composer.tsx:1209 sees aria-expanded === 'true', calls preventDefault(), and dismisses the menu instead. The message is not sent and Enter has to be pressed twice. Pasting text that ends in @name does the same and additionally fires a file-search IPC.
Reachability class 1 — pasting an absolute path into a coding agent's composer is an everyday action. Fully recoverable (press Enter again), which is why this is P2 and not higher. But the PR trades one paste annoyance for another, so it should not land as-is. A triggerMenu.reset() after the insert would do it; if the only correct fix is upstream in Astryx, say so in the body and land it knowingly — this file already sets that precedent at :908-916.
P3 — the fix is applied at one consumer, not at the authority. insertTextAtCursor is what lacks undo bookkeeping, and Astryx calls it from two places: the trigger menu's onInsertText (ChatComposerInput.tsx:487) and handlePaste's own fallback (:678). Type x, then use a slash command to insert text, then Cmd+Z — the inserted text is still not undone. Not this PR's job to fix upstream, but worth one sentence in the body acknowledging that paste is the only path covered.
P3 — the props.disabled || isReferenceSizedPaste(pasted) guard is a second copy of a decision already made. pasteAsToken.onPaste (composer.tsx:1074) evaluates the same condition upstream. The only way to reach this line with the guard true is for pasteAsToken to have declined despite the same condition — in which case the paste falls through to insertTextAtCursor, the exact undo-less path this PR exists to fix. So the guard is either unreachable or it switches the fix off. props.disabled is dead besides: the editable is contentEditable={false} in that state. I would delete the line.
Ungraded, worth knowing: insertHTML's input event makes Astryx call emitChange() twice for one paste, so onInputChange → applyText + saveCurrentDraft each run twice. No visible effect today because React batches within the event and saveCurrentDraft is idempotent — but it stops being harmless the moment that function grows a counter or a remote write.
Also ungraded: the body says the change preserves the IME paste path. It does, but because the capture-phase guard at composer.tsx:1689-1694 stops Astryx's handler from running at all, so the new code is simply unreachable there — not because the new code preserves anything. And the body does not mention the input-event side effect above.
Happy to approve once the trigger-menu interaction is either handled or explicitly accepted in the body.
AI use: Claude Code (Opus) produced the initial findings, including live Playwright measurements in Chromium showing that insertHTML fires input and creates a distinct undo transaction while the Range path does neither, that whitespace and newlines round-trip losslessly through Astryx's serialize(), and that no injection or send-then-undo resurrection is possible. I then independently re-verified the graded P2 chain against the PR head — handleInput → triggerMenu.handleInput, findActiveTrigger's backward scan, the two registered trigger characters, and the Enter branch at :1209. The menu was not observed opening in a running app. The reviewer of record reviewed and accepted this.
简体中文
把粘贴路径的 DOM 插入换成 document.execCommand('insertHTML', …),使一次粘贴成为独立的浏览器 undo 事务,而不是 undo 栈根本看不见的 Range 变更。
先说清楚:诊断和接缝都是对的。"这次编辑可撤销"的真正权威是浏览器自身的编辑事务,而 chatComposerSelection.ts 的 insertTextAtCursor 通过 Range API 写入,完全绕开了它。选择 execCommand 而不是自建一套 undo 栈是正确的直觉,也与本文件 composer.tsx:1249 已有的 insertLineBreak 一致。先经 textContent → innerHTML 转义再替换 <br> 也是对的,我确认没有注入路径。
P2 —— 粘贴现在会打开 @ / / 触发菜单,并吞掉下一次 Enter。
这是我希望修掉的一条。insertHTML 会触发真实的 input 事件,而它替换掉的 Range 路径不会。Astryx 的 handleInput(ChatComposerInput.tsx:504-507)除了 emitChange() 还会调用 triggerMenu.handleInput();findActiveTrigger(useTriggerMenu.tsx:200-217)随后从光标向前扫描,只要触发字符位于文本开头或空格/换行之后就激活。本 composer 恰好注册了两个:composer.tsx:892 的 @ 与 :926 的 /。
具体场景:光标在空 composer,粘贴 /Users/me/notes.txt。反向扫描在 index 0 命中 /,prevChar 为 null,slash 命令菜单弹开。用户按 Enter 发送,composer.tsx:1209 看到 aria-expanded === 'true',执行 preventDefault() 并关闭菜单。消息没有发出,Enter 必须按两次。粘贴以 @name 结尾的文本同理,还会额外触发一次文件搜索 IPC。
可达类别 1——把绝对路径粘进编码 agent 的输入框是日常操作。完全可恢复(再按一次 Enter),所以定 P2 而非更高。但本 PR 等于用一种粘贴烦恼换了另一种,不宜原样合入。插入后调用一次 triggerMenu.reset() 即可;若唯一正确的修法在 Astryx 上游,请在正文说明并有意识地合入——本文件 :908-916 已有此先例。
P3 —— 修在消费端而非权威处。 缺少 undo 记账的是 insertTextAtCursor,Astryx 从两处调用它:触发菜单的 onInsertText(ChatComposerInput.tsx:487)与 handlePaste 自身的兜底(:678)。输入 x,再用 slash 命令插入文本,然后 Cmd+Z——插入的文本仍不会被撤销。修上游不是本 PR 的职责,但正文值得加一句,说明本次只覆盖粘贴。
P3 —— props.disabled || isReferenceSizedPaste(pasted) 是已作决定的第二份副本。 pasteAsToken.onPaste(composer.tsx:1074)上游已判过同一条件。要在此处让守卫为真,唯一可能是 pasteAsToken 在同一条件下拒绝接管——而那时粘贴恰好落到 insertTextAtCursor,正是本 PR 要修的无 undo 路径。所以该守卫要么不可达,要么恰好把修复关掉。props.disabled 另属死代码:该状态下 editable 是 contentEditable={false}。建议整行删除。
未定级但值得知道:insertHTML 触发的 input 事件会让 Astryx 对一次粘贴调用两次 emitChange(),于是 onInputChange → applyText + saveCurrentDraft 各跑两遍。今天无可见后果,因为 React 在同一事件内批处理且 saveCurrentDraft 幂等——但只要该函数将来加上计数或远端写入,它就不再无害。
同样未定级:正文称本改动保留了 IME 粘贴路径。确实没坏,但原因是 composer.tsx:1689-1694 的捕获阶段守卫让 Astryx 的 handler 根本不运行,新代码在那里不可达——不是新代码保留了什么。正文也未提及上面那条 input 事件副作用。
触发菜单的交互被处理掉、或在正文中明确接受之后,我即可 Approve。
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
The trigger-menu P2 is fixed, and fixed at the right granularity. insertHTML fires a real input event that the Range path never did, and that event is what reached triggerMenu.handleInput(). Suppressing it during the paste via onInputCapture + plainTextPasteInputActiveRef removes the side effect without removing the change notification: Astryx's handlePaste calls emitChange() itself once the consumer's onPaste returns true (ChatComposerInput.js:449), and that call does not travel through the input event. So onChange → applyText → setText still runs exactly once — which also resolves the double-emitChange I noted last round.
The menuWasOpen branch covers the other direction (pasting while a menu is already open), and the three new E2E cases pin all three scenarios I described. The redundant props.disabled || isReferenceSizedPaste(pasted) guard is gone, and the execCommand deprecation now carries a migration note.
Two things I checked and am satisfied with: the finally restores the flag on the execCommand throw path, and the synthetic Escape is dispatched after the flag is cleared, so it is not swallowed by the same capture handler.
Ungraded, unchanged from last round: insertTextAtCursor is still the authority that lacks undo bookkeeping, and Astryx calls it from two other places (ChatComposerInput.js:487 and the paste fallback), so a slash-command insertion is still not undoable. Upstream rather than this PR — worth a sentence in the body noting that paste is the only path covered.
Also ungraded: dismissing the menu by dispatching a synthetic Escape keydown couples this to Astryx's current key handling; if the dismiss key ever changes, it fails silently. The E2E covers it, so the risk is bounded.
AI-assisted review: I traced both emitChange paths in @astryxdesign/core/dist/Chat/ChatComposerInput.js to confirm the suppressed input event does not drop the change notification — I initially suspected it did, and disproved that myself. No tests run. AI review is not independent human review.
简体中文
trigger menu 那条 P2 已修,且修在正确的粒度上:被抑制的只有 insertHTML 产生的 input 事件(它才是触达 triggerMenu.handleInput() 的路径),而 Astryx 在 consumer onPaste 返回 true 后会自行调用一次 emitChange()(ChatComposerInput.js:449),不走 input 事件。因此 onChange → applyText → setText 仍恰好执行一次,顺带也解决了我上轮提到的 emitChange 重复调用。
menuWasOpen 分支覆盖了"粘贴时菜单已开"的反向情形,三个新增 E2E 覆盖了我描述的全部三个场景;冗余守卫已删,execCommand 的迁移说明已加。
未定级、与上轮相同:insertTextAtCursor 仍是缺少 undo 记账的权威,Astryx 另有两处调用它,因此斜杠命令插入的文本仍不可撤销——属上游问题,建议在 body 里说明本 PR 只覆盖粘贴路径。
Summary
ChatComposerInput.onPasteseaminputevent produced by that transaction, preventing pasted@//text from opening trigger menus and preventing duplicate draft synchronizationFixes #3786
Review follow-up
The review at
pullrequestreview-5037111788has been checked item by item:inputevent during the browser paste transaction. Regression coverage includes an absolute path, mention-looking text, and a menu that was already open before paste.props.disabled || isReferenceSizedPaste(pasted)guard and relying on Astryx's existing routing.insertTextAtCursorlimitation: documented here rather than expanded into this implementation. Other Astryx callers, including slash-command text insertion, still use its Range-based insertion behavior and are outside this PR's ordinary inline plain-text paste scope.execCommanddeprecation:execCommand('insertHTML')is deprecated, but it is currently the only composer insertion primitive that creates the required browser undo transaction. The intended migration point is a future Astryx transactional plain-text insertion API.Before / after evidence
Both recordings run the same real Electron flow with the same minimal input: type
x→ paste ordinary multiline text → press undo three times. Each step displays the current composer text.Before —
upstream/main@7235069adThe first undo removes
xwhile leaving the pasted text. The second and third undo still leave the pasted text, reproducing #3786.page@c2ccde11c524db300d1090fd7f645c92.webm
After
The first undo removes only the pasted text and leaves
x; the second removesx; the third remains empty without reinsertion or duplication.page@7da85ab9fe494f70134e34b1ff7e7118.webm
Verification
Passed locally on the final head:
npm --workspace @maka/desktop run e2e -- composer-undo.spec.ts(5/5)npm run lintnpm run format:checknpm run typechecknpm run check:asf-headersnpx knip --workspace packages/uinpx knip --workspace apps/desktopnpm run buildgit diff --checkThe full local
npm testcommand completed the affected Desktop suite successfully, but the repository-wide command still exited non-zero on unrelated/environment-sensitive tests in unchanged workspaces:@maka/runtime: expects/usr/localat executable-root index 1 although this Node installation deduplicates it to index 0.@maka/mcp: macOS resolves the temporary working directory as/private/var/...while the assertion expects/var/....maka-agentCLI: the same/private/var/...versus/var/...canonical-path difference appears in the global-installation fixture.@maka/runtime-host: the parallel workspace run transiently lost a shared cache registration directory; the exact failed test passed immediately when rerun in isolation.Current CI status
CI for head
3270202c2passed, including the affected standard workspace tests, Desktop E2E, Browser WebContentsView semantic smoke, alignment audit, and Storybook smoke:AI use
Select exactly one:
Tool(s) and scope: Codex analyzed the review, implemented the focused composer changes, added Electron regression coverage, ran verification, performed an independent code review, and updated this PR description. The commit includes the required
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?