Skip to content

Give the composer its own undo stack - #72288

Merged
OutThisLife merged 3 commits into
mainfrom
bb/composer-undo
Jul 26, 2026
Merged

OutThisLife merged 3 commits into
mainfrom
bb/composer-undo

Conversation

@OutThisLife

@OutThisLife OutThisLife commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Supersedes #49746.

Cmd+Z didn't see a paste in either composer. Worse than doing nothing: it skipped past the pasted text and undid whatever edit came before it, leaving the paste stranded and the earlier edit destroyed.

The cause is a bill coming due from #45812. Both composers stopped using execCommand('insertText') because Chromium's editing pipeline is ~O(n²) on large multiline blobs — a 47KB paste froze the composer for 4.5 seconds. Range-based mutation fixed the freeze, but it's invisible to Chromium's undo stack. Confirmed in a real browser: a Range insert fires zero beforeinput/input events, and the next undo reaches straight past it to the previous entry.

Owning the stack outright is the only coherent fix. A half-owned one interleaves our snapshots with Chromium's own typing entries and unwinds them out of order. So every edit path now banks its pre-edit state, and the editor claims the shortcuts itself instead of letting the native command run.

Snapshots are plain text plus a caret offset rather than DOM — the editor already round-trips losslessly through composerPlainText/renderComposerContents, so text is the smallest thing that fully restores a state. Consecutive keystrokes coalesce inside a 600ms window, so undo steps back by a typing burst the way a native editor does rather than one character at a time.

Two things needed handling beyond the keystroke. Electron's Edit menu ships { role: 'undo' }, whose accelerator macOS consumes before the renderer sees it — the same hazard main.ts already documents for Cmd+W — so a capture-phase beforeinput listener claims historyUndo/historyRedo and keeps the menu item and the shortcut in agreement. And in the edit composer, undo is handled ahead of Escape, since a stray Cmd+Z falling through would cancel the entire edit rather than step back one change.

Both composers share the hook. It keys its claim off document.activeElement, so the two mounted instances stay independent — only the focused editor's stack responds.

Relationship to #49746

@DavidMetcalfe diagnosed this first and correctly: the paste bypasses the pipeline, so the native stack never records it. That PR restores native undo for pastes under 4096 chars and keeps the fast path above the threshold.

This one takes the same diagnosis further on two points. A size threshold makes undo behave differently depending on how much you pasted, and the large pastes that are most expensive to lose are exactly the ones that stay un-undoable. And the edit composer pastes through the same helper, so it has the identical bug today — a fix scoped to index.tsx leaves it broken.

Credited via Co-authored-by on the undo-stack commit.

A separate bug this turned up

composerPlainText appends a newline to any block element that isn't the editor slot, and inline-refs.ts serialized the caret's preceding content through a bare <div>. So "text before caret" always looked like it ended in whitespace and the separating space was never inserted: dragging a file in after a word produced review@file:... glued together. Fixed at both call sites, including the new caret helpers that would have inherited it.

That one's verified the hard way — its test fails on main and passes here.

Verification

  • 26 new tests: history and caret-offset behavior, plus hook behavior including two-instance isolation
  • Full desktop suite green (3126 passing), tsc clean, no new lint errors

…pace

`plainTextInRange` serialized the caret's preceding content through a bare
<div>, but `composerPlainText` appends "\n" to any block element that isn't the
editor slot. So `beforeText` always looked like it ended in whitespace and the
separating space was never inserted — dragging a file in after a word produced
`review@file:...` glued together.

Marking the scratch container with RICH_INPUT_SLOT makes it serialize in the
same coordinates as the editor. Same fix lands in the new `caretOffsetInEditor`,
which measures caret offsets the same way.
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

૮ >ﻌ< ა ci review

ran on 92f62be

ℹ️ Info

Desktop E2E visual evidence · View test artifacts · View job

1 visual diff.

inline evidence is publishing...

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation labels Jul 26, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related to #49746 and #49745. Both open PRs repair the post-#45812 paste-undo regression, but they require a policy choice: #49746 restores native undo below a performance threshold, while this PR owns a complete undo stack including large pastes and both composers.

OutThisLife and others added 2 commits July 26, 2026 18:21
The rich editor mutates its DOM through `Range` rather than the browser's
editing commands, because `execCommand('insertText')` is ~O(n²) on large
multiline blobs and froze the composer for seconds on a big paste (#45812).
Those mutations never reach Chromium's undo stack, so a paste was invisible to
it — Cmd+Z skipped straight past the pasted text and undid whatever edit came
before it, leaving the paste stranded and the earlier edit destroyed.

Owning the stack outright is the only coherent fix; a half-owned one interleaves
our snapshots with Chromium's own typing entries and undoes them out of order.
Every edit path now banks its pre-edit state, and the editor claims Cmd+Z /
Cmd+Shift+Z (plus Ctrl+Y) instead of letting the native command run. Snapshots
are plain text + a caret offset rather than DOM, since the editor already
round-trips losslessly through composerPlainText/renderComposerContents.

Consecutive keystrokes coalesce inside a 600ms window, so undo steps back by a
typing burst the way a native editor does rather than one character at a time.
Electron's Edit menu `{ role: 'undo' }` fires the native command without a
keystroke the renderer can see, so a capture-phase `beforeinput` listener claims
historyUndo/historyRedo too and keeps the menu item and the shortcut in
agreement. Switching drafts resets the history — undoing into another
conversation's text is worse than having none.

Co-authored-by: David Metcalfe <80915+DavidMetcalfe@users.noreply.github.com>
…oser

The edit composer pastes through the same `insertComposerContentsAtCaret` the
main composer uses, so it had the identical bug: the Range-based insert never
reaches Chromium's undo stack and Cmd+Z skipped past the paste to destroy the
edit before it. Its inline-ref and trigger-chip inserts mutate the DOM directly
too, with the same result.

Both surfaces now share `useComposerUndo`. The hook already keys its
document-level `beforeinput` claim off `document.activeElement`, so the two
mounted instances stay independent — only the focused editor's stack responds.
Undo/redo is handled ahead of Escape here, since a stray Cmd+Z falling through
would cancel the whole edit rather than step back one change.

`insertRefStrings` banks through `withUndoPoint` rather than recording after the
insert, which would have snapshotted the state it was meant to restore.
@OutThisLife
OutThisLife enabled auto-merge July 26, 2026 23:34
@OutThisLife
OutThisLife merged commit bd6437d into main Jul 26, 2026
34 checks passed
@OutThisLife
OutThisLife deleted the bb/composer-undo branch July 26, 2026 23:38
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
prmartinow pushed a commit to prmartinow/hermes-agent that referenced this pull request Aug 26, 2026
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants