From 96a349b94e4ef08903d30822127ef635ea97d99b Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 16:22:46 +1100 Subject: [PATCH 1/2] fix(staged): undo quote removal with native undo instead of a toast Removing a quote from the chat composer must not raise a toast, and the removal must be undoable and redoable with the platform's own undo and redo, in order with the typing around it. Root cause: `removeQuote` in SessionChatPane.svelte called `offerQuoteUndo`, which raised a svelte-sonner toast ("Quote removed from your reply") with an Undo action whenever the removed quote had a reply. That action was the only way to get a quote back. Both ways of removing a quote (the block's X button, and Backspace or Delete in the empty gap above an unanswered quote) go through that one function, and SessionChatPane is the only composer that takes quotes. How undo is wired: the composer has no editor framework. Its inputs are separate contenteditable elements that rely on the browser's undo stack, and quotes live in Svelte state outside it. The new shared NativeUndoAnchor component is a hidden editable element: recording a step types one invisible character into it with `execCommand('insertText')`, which puts an entry on the same document-wide undo stack as the user's typing. When native undo or redo reaches that entry the browser removes or re-adds the character, and the component runs the step's `undo` or `redo` to match. No keydown handler is involved, so Cmd+Z, Ctrl+Z, the Edit menu and plain text undo are all left to the browser. Focus and the caret are borrowed for the edit and handed back before the step runs. ChatComposer records each removal as such a step. `onRemoveQuote` now returns a restore function; SessionChatPane's puts the quote back where it was with its reply and lead-in text, and refuses once the draft has been sent, another session is in the box, or the pane is inactive or closed. Undo returns the caret to the block's input it was taken from, or scrolls the field to the block when the caret was elsewhere. Removed as dead: `offerQuoteUndo`, `dismissQuoteUndo`, the toast bookkeeping, `revealDraftQuote`, `restoreQuoteInReplyDraft` and `ReplyComposerTarget.restoreQuote`. Toasts for other actions are untouched. Verified: vitest (3712 tests) and svelte-check pass. The real ChatComposer was also driven in a standalone harness in Chromium with real Cmd+Z key presses, and in an offscreen WKWebView through its NSUndoManager, the path the Edit menu's Undo and Redo take. Typing, then removing a quote, then typing again undid in reverse order and redid in order in both. Not verified: the running Tauri app itself, hardware key events in WKWebView, iOS (on-screen keyboard behaviour while the anchor borrows focus, shake and gesture undo), and Firefox. Known limitation: removing or restoring a quote re-renders the inputs whose text it merges or splits, so typing steps recorded earlier in those inputs point at replaced nodes. Undo presses that reach them do nothing visible before the stack moves on. Ordering is exact when the removed quote had no lead-in text above it. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- .../sessions/ChatComposer.component.test.ts | 249 ++++++++++++++++-- .../lib/features/sessions/ChatComposer.svelte | 113 ++++++-- .../sessions/ComposerDraftQuotes.svelte | 11 +- .../SessionChatPane.component.test.ts | 240 ++++++++--------- .../features/sessions/SessionChatPane.svelte | 129 +++------ .../lib/features/sessions/replyDrafts.test.ts | 49 +--- .../src/lib/features/sessions/replyDrafts.ts | 26 -- .../testStubs/ComposerDraftHost.svelte | 68 +++++ .../sessions/testStubs/PromptInputStub.svelte | 46 ++-- .../shared/NativeUndoAnchor.component.test.ts | 209 +++++++++++++++ .../src/lib/shared/NativeUndoAnchor.svelte | 174 ++++++++++++ .../lib/shared/testStubs/nativeUndoStack.ts | 105 ++++++++ 12 files changed, 1063 insertions(+), 356 deletions(-) create mode 100644 apps/staged/src/lib/features/sessions/testStubs/ComposerDraftHost.svelte create mode 100644 apps/staged/src/lib/shared/NativeUndoAnchor.component.test.ts create mode 100644 apps/staged/src/lib/shared/NativeUndoAnchor.svelte create mode 100644 apps/staged/src/lib/shared/testStubs/nativeUndoStack.ts diff --git a/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts b/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts index ae0142dc5..f11822d56 100644 --- a/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts +++ b/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts @@ -1,17 +1,23 @@ -import { flushSync, mount, unmount } from 'svelte'; +import { flushSync, mount, tick, unmount } from 'svelte'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import ChatComposer from './ChatComposer.svelte'; -import type { DraftQuote } from './composerDraft'; +import type { ComposerDraft, DraftQuote } from './composerDraft'; import { composerSendControl } from './composerSendControl'; +import ComposerDraftHost from './testStubs/ComposerDraftHost.svelte'; import { reactiveProps } from './testStubs/reactiveProps.svelte'; +import { + installNativeUndoStack, + type NativeUndoStack, +} from '../../shared/testStubs/nativeUndoStack'; // The composer's field: one box holding the quote blocks, the text between // them and the main input, which the user works as one editor. Covered here // are the ways the field ties its inputs together — the arrows walking from // one into the next, a press on its surface landing the caret in an input, -// the single placeholder, the keyboard cap on the whole field and the -// focused input kept in view by the field's own scroll — while the inputs' -// own behaviour is under ComposerDraftQuotes.component.test.ts. +// the single placeholder, the keyboard cap on the whole field, the focused +// input kept in view by the field's own scroll, and a quote's removal taking +// its place in the browser's undo history with the typing around it — while +// the inputs' own behaviour is under ComposerDraftQuotes.component.test.ts. // The on-screen-keyboard cap reads the visible viewport; stand it in with a // keyboard up, so the cap is a share of the 400px left. @@ -210,25 +216,6 @@ describe('ChatComposer field', () => { else delete (Range.prototype as Partial).getClientRects; } }); - - it('scrolls the field to a quote block on request without moving focus', () => { - render([quote('a'), quote('b')]); - const fieldEl = layOutField(); - const block = target.querySelector('[data-quote-entry="b"] .composer-quote'); - if (!block) throw new Error('no block'); - block.getBoundingClientRect = () => rect(320, 380); - const ancestors = vi.spyOn(Element.prototype, 'scrollIntoView'); - const focused = document.activeElement; - - expect(component!.revealQuote('b')).toBe(true); - expect(fieldEl.scrollTop).toBe(79); - expect(document.activeElement).toBe(focused); - expect(ancestors).not.toHaveBeenCalled(); - // A block that isn't shown is reported, not scrolled to. - expect(component!.revealQuote('missing')).toBe(false); - expect(fieldEl.scrollTop).toBe(79); - ancestors.mockRestore(); - }); }); describe('ChatComposer placeholder', () => { @@ -344,3 +331,217 @@ describe('ChatComposer arrows between inputs', () => { expect(document.activeElement).toBe(replyOf('a')); }); }); + +// Removing a quote is a step on the browser's undo stack, in order with the +// typing in the field's inputs. jsdom has no such stack; the stand-in walks +// edits the way a browser does (testStubs/nativeUndoStack.ts), and the host +// keeps the draft as the pane would. +describe('ChatComposer quote removal', () => { + let stack: NativeUndoStack; + let host: ReturnType; + + function mountHost() { + return mount(ComposerDraftHost, { target }); + } + + function renderDraft(draft: ComposerDraft) { + target = document.createElement('div'); + document.body.appendChild(target); + host = mountHost(); + component = host; + host.setDraft(draft); + flushSync(); + } + + /** Let the composer's own follow-up to a change (the caret, the field's scroll) run. */ + async function settle() { + flushSync(); + await tick(); + flushSync(); + } + + async function pressRemove(id: string) { + const button = target.querySelector( + `[data-quote-entry="${id}"] .composer-quote-remove` + ); + if (!button) throw new Error(`no remove button for ${id}`); + button.click(); + await settle(); + } + + async function undo() { + expect(stack.undo()).toBe(true); + await settle(); + } + + async function redo() { + expect(stack.redo()).toBe(true); + await settle(); + } + + function quoteIds(): string[] { + return host.draft().quotes.map((q) => q.id); + } + + beforeEach(() => { + stack = installNativeUndoStack(); + }); + + afterEach(() => { + stack.uninstall(); + }); + + it('puts the quote back where it was on undo, and takes it out again on redo', async () => { + const draft = { + quotes: [quote('a', '', 'first reply'), quote('b', 'lead-in', 'second reply'), quote('c')], + text: 'after', + }; + renderDraft(draft); + + await pressRemove('b'); + // The text before the removed quote joins what follows it. + expect(host.draft()).toEqual({ + quotes: [quote('a', '', 'first reply'), quote('c', 'lead-in')], + text: 'after', + }); + + await undo(); + expect(host.draft()).toEqual(draft); + + await redo(); + expect(quoteIds()).toEqual(['a', 'c']); + expect(host.removedIds()).toEqual(['b', 'b']); + + await undo(); + expect(host.draft()).toEqual(draft); + }); + + it('undoes an unanswered quote removed with Backspace too, with the caret back in its gap', async () => { + renderDraft({ quotes: [quote('a', '', 'a reply'), quote('b')], text: '' }); + focusWithCaret(gapOf('b')); + + gapOf('b').dispatchEvent( + new KeyboardEvent('keydown', { key: 'Backspace', bubbles: true, cancelable: true }) + ); + await settle(); + expect(quoteIds()).toEqual(['a']); + expect(document.activeElement).toBe(mainInput()); + + await undo(); + expect(quoteIds()).toEqual(['a', 'b']); + expect(document.activeElement).toBe(gapOf('b')); + + await redo(); + expect(quoteIds()).toEqual(['a']); + expect(document.activeElement).toBe(mainInput()); + }); + + it('undoes in order with the typing before and after the removal', async () => { + renderDraft({ quotes: [quote('a', '', 'a reply')], text: '' }); + + stack.type(mainInput(), 'abc'); + await pressRemove('a'); + stack.type(mainInput(), 'def'); + await settle(); + expect(host.draft()).toEqual({ quotes: [], text: 'abcdef' }); + + await undo(); + expect(host.draft()).toEqual({ quotes: [], text: 'abc' }); + await undo(); + expect(host.draft()).toEqual({ quotes: [quote('a', '', 'a reply')], text: 'abc' }); + await undo(); + expect(host.draft()).toEqual({ quotes: [quote('a', '', 'a reply')], text: '' }); + + await redo(); + expect(host.draft()).toEqual({ quotes: [quote('a', '', 'a reply')], text: 'abc' }); + await redo(); + expect(host.draft()).toEqual({ quotes: [], text: 'abc' }); + await redo(); + expect(host.draft()).toEqual({ quotes: [], text: 'abcdef' }); + // Typing was the browser's own to undo and redo: the quote left once + // by hand and once by redo, and nothing else touched it. + expect(host.removedIds()).toEqual(['a', 'a']); + }); + + it('returns the caret to the reply it was taken from', async () => { + renderDraft({ quotes: [quote('a', '', 'a reply'), quote('b')], text: '' }); + focusWithCaret(replyOf('a')); + + await pressRemove('a'); + // Focus in the removed block moved to the input its text joined. + expect(document.activeElement).toBe(gapOf('b')); + + await undo(); + expect(document.activeElement).toBe(replyOf('a')); + + await redo(); + expect(document.activeElement).toBe(gapOf('b')); + }); + + it('leaves a caret that was elsewhere where it is, and scrolls the field to the restored block', async () => { + renderDraft({ quotes: [quote('a'), quote('b', '', 'b reply')], text: 'typing here' }); + const fieldEl = layOutField(); + // The main input is in the field's view; the block will come back below it. + mainInput().getBoundingClientRect = () => rect(210, 240); + focusWithCaret(mainInput()); + await pressRemove('b'); + expect(document.activeElement).toBe(mainInput()); + + stack.undo(); + flushSync(); + const block = target.querySelector('[data-quote-entry="b"] .composer-quote'); + if (!block) throw new Error('no block'); + block.getBoundingClientRect = () => rect(320, 380); + await settle(); + + expect(document.activeElement).toBe(mainInput()); + expect(fieldEl.scrollTop).toBe(79); + }); + + it('leaves the field scrolled where it was as the removal is recorded', async () => { + renderDraft({ quotes: [quote('a', '', 'a reply')], text: 'typing here' }); + const fieldEl = layOutField(); + mainInput().getBoundingClientRect = () => rect(320, 350); + focusWithCaret(mainInput()); + expect(fieldEl.scrollTop).toBe(49); + // Scrolled back up to the quote, the caret still in the main input: + // recording borrows focus and hands it back, which is not a move to + // follow. + fieldEl.scrollTop = 0; + + await pressRemove('a'); + + expect(document.activeElement).toBe(mainInput()); + expect(fieldEl.scrollTop).toBe(0); + }); + + it('leaves the quote out when the parent refuses the undo, and restores it on a later one', async () => { + const draft = { quotes: [quote('a', '', 'a reply')], text: '' }; + renderDraft(draft); + await pressRemove('a'); + + host.refuseUndo(true); + await undo(); + expect(quoteIds()).toEqual([]); + // Nothing came back, so there is nothing for redo to take out again. + await redo(); + expect(host.removedIds()).toEqual(['a']); + + host.refuseUndo(false); + await undo(); + expect(host.draft()).toEqual(draft); + }); + + it('records nothing for a removal the parent gives no way back from', async () => { + const props = render([quote('a')]); + const anchor = target.querySelector('.native-undo-anchor'); + if (!anchor) throw new Error('no undo anchor'); + + target.querySelector('.composer-quote-remove')!.click(); + await settle(); + + expect(props.onRemoveQuote).toHaveBeenCalledWith('a'); + expect(anchor.textContent).toBe(''); + expect(stack.undo()).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/features/sessions/ChatComposer.svelte b/apps/staged/src/lib/features/sessions/ChatComposer.svelte index c68c7688f..292884354 100644 --- a/apps/staged/src/lib/features/sessions/ChatComposer.svelte +++ b/apps/staged/src/lib/features/sessions/ChatComposer.svelte @@ -38,7 +38,9 @@ the neighbouring input (composerCaretNav.ts), a click on the field's own surface puts the caret in the nearest input, and only the last input shows a placeholder, while nothing has been typed anywhere. Removing a quote - moves focus that was in it to the input its text joined. + moves focus that was in it to the input its text joined, and is a step on + the browser's undo stack like the typing around it: Cmd+Z puts the quote + back where it was, and redo takes it out again. -->
+ {#if canAttachImages && attachedImageIds.length > 0}
{#each attachedImageIds as imageId (imageId)} @@ -471,7 +548,7 @@ {hashtagItems} {onkeydown} {oninput} - onRemoveQuote={(id) => void removeQuote(id)} + onRemoveQuote={removeQuote} /> {/if} { loadDraftMock.mockImplementation(async () => null); }); - type UndoToast = { onClick: () => void }; - - /** The Undo action of the newest "Quote removed" toast, with the toast's id. */ - function lastUndo(): { id: string; action: UndoToast } { - const call = toastMock.mock.calls[toastMock.mock.calls.length - 1] as unknown as [ - string, - { action: UndoToast }, - ]; - const id = toastMock.mock.results[toastMock.mock.results.length - 1].value as string; - return { id, action: call[1].action }; - } - /** * Mount a pane whose draft holds three quotes, then remove the middle one - * (which has a reply, so Undo is offered). Returns the draft as saved - * before the removal, for Undo to be measured against. + * as the composer's X would. Returns the draft as saved before the + * removal, for undo to be measured against. */ async function removeMiddleQuote(sessionId: string) { const { pane, props } = mountPane(sessionId); @@ -245,27 +233,43 @@ describe('SessionChatPane quoted replies', () => { await settle(); const before = composerDraftFromContent(lastSaved(replyKey(sessionId))); const [, second] = before.quotes; - document.querySelector(`[data-testid="remove-quote-${second.id}"]`)!.click(); - await settle(); + await removeQuote(second.id); expect(quoteCount()).toBe(2); return { pane, props, before, second }; } - function quoteCount(): number { + async function removeQuote(id: string) { + document.querySelector(`[data-testid="remove-quote-${id}"]`)!.click(); + await settle(); + } + + /** + * Undo the newest removal, as the browser's undo reaches the composer's + * step for it (the stub keeps the steps; see PromptInputStub.svelte). + */ + async function undoRemoval() { + document.querySelector('[data-testid="undo-remove-quote"]')!.click(); + await settle(); + } + + /** Whether the pane took each undo so far, oldest first. */ + function undoResults(): boolean[] { const input = document.querySelector('[data-testid="prompt-input"]'); - return Number(input?.dataset.quoteCount ?? NaN); + return (input?.dataset.undoResults ?? '') + .split(' ') + .filter(Boolean) + .map((result) => result === 'true'); } - /** The quote replies the pane asked the composer to put the caret in. */ - function focusedQuotes(): string[] { + /** The quotes the pane reported back in the composer after an undo. */ + function restoredQuotes(): string[] { const input = document.querySelector('[data-testid="prompt-input"]'); - return (input?.dataset.focusedQuotes ?? '').split(' ').filter(Boolean); + return (input?.dataset.restoredQuotes ?? '').split(' ').filter(Boolean); } - /** The quote blocks the pane asked the composer to scroll to without focusing. */ - function revealedQuotes(): string[] { + function quoteCount(): number { const input = document.querySelector('[data-testid="prompt-input"]'); - return (input?.dataset.revealedQuotes ?? '').split(' ').filter(Boolean); + return Number(input?.dataset.quoteCount ?? NaN); } function lastSaved(key: string) { @@ -293,88 +297,84 @@ describe('SessionChatPane quoted replies', () => { unmount(pane); }); - it('offers to undo removing a quote with a reply, putting it back in its place', async () => { - toastMock.mockClear(); - const { pane } = mountPane('session-undo'); - flushSync(); - await settle(); - - await addQuoteToReplyDraft('session-undo', { ...quote, quote: 'First.' }); - await addQuoteToReplyDraft('session-undo', { ...quote, quote: 'Second.' }); - await addQuoteToReplyDraft('session-undo', { ...quote, quote: 'Third.', reply: '' }); - await settle(); - const before = composerDraftFromContent(lastSaved(replyKey('session-undo'))); - const [, second] = before.quotes; - - document.querySelector(`[data-testid="remove-quote-${second.id}"]`)!.click(); - await settle(); + it('removes a quote without a toast, and undo puts it back in its place', async () => { + const { pane, before, second } = await removeMiddleQuote('session-undo'); + const removed = composerDraftFromContent(lastSaved(replyKey('session-undo'))); + expect(removed.quotes.map((q) => q.quote)).toEqual(['First.', 'Third.']); + expect(toastMock).not.toHaveBeenCalled(); - expect(quoteCount()).toBe(2); - expect(toastMock).toHaveBeenCalledTimes(1); - const [, options] = toastMock.mock.calls[0] as [string, { action: { onClick: () => void } }]; - expect(options.action).toMatchObject({ label: 'Undo' }); - - options.action.onClick(); - await settle(); + await undoRemoval(); + expect(undoResults()).toEqual([true]); expect(quoteCount()).toBe(3); const restored = composerDraftFromContent(lastSaved(replyKey('session-undo'))); expect(restored.quotes.map((q) => q.quote)).toEqual(['First.', 'Second.', 'Third.']); expect(restored.quotes[1]).toMatchObject({ id: second.id, reply: 'Which one?' }); expect(restored).toEqual(before); + // The composer is told once the block is back in it, to place the caret. + expect(restoredQuotes()).toEqual([second.id]); - // An unanswered quote just goes. - const third = restored.quotes[2]; - document.querySelector(`[data-testid="remove-quote-${third.id}"]`)!.click(); - await settle(); - expect(quoteCount()).toBe(2); - expect(toastMock).toHaveBeenCalledTimes(1); + // Redo is the composer removing the quote again, which undo reverses again. + await removeQuote(second.id); + expect(composerDraftFromContent(lastSaved(replyKey('session-undo')))).toEqual(removed); + await undoRemoval(); + expect(composerDraftFromContent(lastSaved(replyKey('session-undo')))).toEqual(before); + + expect(toastMock).not.toHaveBeenCalled(); + expect(toastMock.error).not.toHaveBeenCalled(); unmount(pane); }); - it('puts the caret in the restored reply for a pointer, and only scrolls to the block on a touch screen', async () => { - const first = await removeMiddleQuote('session-caret'); - lastUndo().action.onClick(); + it('removes an unanswered quote the same way: no toast, and undo puts it back', async () => { + const { pane } = mountPane('session-unanswered'); + flushSync(); await settle(); - expect(quoteCount()).toBe(3); - // The focus brings the block into the field's view itself. - expect(focusedQuotes()).toEqual([first.second.id]); - expect(revealedQuotes()).toEqual([]); - unmount(first.pane); - - // A touch screen's keyboard would rise over the restored draft, so the - // field is scrolled to the block and nothing is focused. - viewportMock.hasCoarsePointer = true; - try { - const second = await removeMiddleQuote('session-caret-touch'); - lastUndo().action.onClick(); - await settle(); - expect(quoteCount()).toBe(3); - expect(focusedQuotes()).toEqual([]); - expect(revealedQuotes()).toEqual([second.second.id]); - unmount(second.pane); - } finally { - viewportMock.hasCoarsePointer = false; - } + await addQuoteToReplyDraft('session-unanswered', { ...quote, reply: '' }); + await settle(); + const before = composerDraftFromContent(lastSaved(replyKey('session-unanswered'))); + + await removeQuote(before.quotes[0].id); + expect(quoteCount()).toBe(0); + expect(toastMock).not.toHaveBeenCalled(); + + await undoRemoval(); + expect(quoteCount()).toBe(1); + expect(composerDraftFromContent(lastSaved(replyKey('session-unanswered')))).toEqual(before); + + unmount(pane); }); - it('takes the Undo toast down with a send, and a late Undo puts nothing back', async () => { + it('undoes removals newest first, each quote going back to its own place', async () => { + const { pane, before } = await removeMiddleQuote('session-two'); + const [first] = before.quotes; + await removeQuote(first.id); + expect(quoteCount()).toBe(1); + + await undoRemoval(); + expect( + composerDraftFromContent(lastSaved(replyKey('session-two'))).quotes.map((q) => q.quote) + ).toEqual(['First.', 'Third.']); + await undoRemoval(); + expect(composerDraftFromContent(lastSaved(replyKey('session-two')))).toEqual(before); + + unmount(pane); + }); + + it('puts nothing back once the draft the quote came from has been sent', async () => { const { pane } = await removeMiddleQuote('session-sent'); - const undo = lastUndo(); document.querySelector('[data-testid="composer-send"]')!.click(); await settle(); expect(quoteCount()).toBe(0); expect(writer.clearNow).toHaveBeenCalledWith(replyKey('session-sent')); - expect(toastMock.dismiss).toHaveBeenCalledWith(undo.id); - // The toast's leaving animation can still take a click: the draft the - // block came from has gone with the message, so nothing is restored — - // neither into the empty composer nor into the cleared stored draft. + // The draft the block came from has gone with the message, so an undo + // that reaches the removal now restores nothing — neither into the empty + // composer nor into the cleared stored draft. writer.scheduleSave.mockClear(); - undo.action.onClick(); - await settle(); + await undoRemoval(); + expect(undoResults()).toEqual([false]); expect(quoteCount()).toBe(0); expect(lastSaved(replyKey('session-sent'))).toBeUndefined(); expect(saveDraftMock).not.toHaveBeenCalled(); @@ -382,52 +382,47 @@ describe('SessionChatPane quoted replies', () => { unmount(pane); }); - it('keeps the Undo toast through a failed send, whose draft is put back', async () => { + it('still undoes the removal after a failed send, whose draft is put back', async () => { const { pane, before } = await removeMiddleQuote('session-failed'); - const undo = lastUndo(); resumeSessionMock.mockRejectedValueOnce(new Error('agent gone')); document.querySelector('[data-testid="composer-send"]')!.click(); await settle(); // The draft is back in the box, the block it lost still restorable. expect(quoteCount()).toBe(2); - expect(toastMock.dismiss).not.toHaveBeenCalled(); - undo.action.onClick(); - await settle(); + await undoRemoval(); + expect(undoResults()).toEqual([true]); expect(quoteCount()).toBe(3); expect(composerDraftFromContent(lastSaved(replyKey('session-failed')))).toEqual(before); - // Pressed, the toast is gone with the press; the send that follows has - // nothing left to take down. - document.querySelector('[data-testid="composer-send"]')!.click(); - await settle(); - expect(quoteCount()).toBe(0); - expect(toastMock.dismiss).not.toHaveBeenCalled(); - unmount(pane); }); - it('takes the Undo toast down when the pane turns to another session', async () => { + it('puts nothing back once the pane has turned to another session', async () => { const { pane, props } = await removeMiddleQuote('session-left'); - const undo = lastUndo(); props.sessionId = 'session-next'; flushSync(); await settle(); - expect(toastMock.dismiss).toHaveBeenCalledWith(undo.id); + writer.scheduleSave.mockClear(); + + await undoRemoval(); + expect(undoResults()).toEqual([false]); + expect(quoteCount()).toBe(0); + expect(lastSaved(replyKey('session-next'))).toBeUndefined(); + expect(saveDraftMock).not.toHaveBeenCalled(); unmount(pane); }); - it('holds an Undo that lands while the draft is reloading until the draft has landed', async () => { + it('holds an undo that lands while the draft is reloading until the draft has landed', async () => { // The pane goes inactive (the note's chat closed) and comes back for the - // same session within the toast's lifetime. Its draft is loading again - // when Undo is pressed: the restore waits for the load, rather than - // being applied to the composer and then replaced by the stored draft. + // same session. Its draft is loading again when the removal is undone: + // the restore waits for the load, rather than being applied to the + // composer and then replaced by the stored draft. const { pane, props, before, second } = await removeMiddleQuote('session-reload'); const stored = lastSaved(replyKey('session-reload')); - const undo = lastUndo(); props.active = false; flushSync(); @@ -443,48 +438,41 @@ describe('SessionChatPane quoted replies', () => { props.active = true; flushSync(); await settle(); - expect(toastMock.dismiss).not.toHaveBeenCalled(); - undo.action.onClick(); - await settle(); + await undoRemoval(); + expect(undoResults()).toEqual([true]); expect(saveDraftMock).not.toHaveBeenCalled(); - // The caret follows the block into the composer: not yet, while the - // block is still held with the restore, or it would be looked for - // before it is rendered and the ask dropped. - expect(focusedQuotes()).toEqual([]); - expect(revealedQuotes()).toEqual([]); + // The composer hears the block is back only once it is: not yet, while + // it is still held with the restore, or the caret would be sent to a + // block that isn't rendered. + expect(restoredQuotes()).toEqual([]); land(stored); await settle(); expect(quoteCount()).toBe(3); expect(composerDraftFromContent(lastSaved(replyKey('session-reload')))).toEqual(before); - expect(focusedQuotes()).toEqual([second.id]); - expect(revealedQuotes()).toEqual([]); + expect(restoredQuotes()).toEqual([second.id]); unmount(pane); }); - it('restores into the stored draft when the pane no longer shows the session', async () => { - const { pane, props, before } = await removeMiddleQuote('session-away'); + it('puts nothing back, in the composer or the stored draft, while the pane shows no session', async () => { + const { pane, props } = await removeMiddleQuote('session-away'); const stored = lastSaved(replyKey('session-away')); - const undo = lastUndo(); props.active = false; flushSync(); await settle(); loadDraftMock.mockImplementation(async () => stored); + writer.scheduleSave.mockClear(); + writer.run.mockClear(); - undo.action.onClick(); - await settle(); + await undoRemoval(); - // Through the shared writer, so it queues behind any pending save. - expect(writer.run).toHaveBeenCalledTimes(1); - expect(saveDraftMock).toHaveBeenCalledTimes(1); - const [key, content] = saveDraftMock.mock.calls[0]; - expect(key).toBe(replyKey('session-away')); - expect( - composerDraftFromContent(content as Parameters[0]) - ).toEqual(before); + expect(undoResults()).toEqual([false]); + expect(writer.run).not.toHaveBeenCalled(); + expect(saveDraftMock).not.toHaveBeenCalled(); + expect(lastSaved(replyKey('session-away'))).toBeUndefined(); unmount(pane); }); diff --git a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte index 08d4955cb..80025037f 100644 --- a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte +++ b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte @@ -107,7 +107,10 @@ setAcpConfigPref, setComposerSendMode, } from '../settings/preferences.svelte'; - import ChatComposer, { composerControlClass } from './ChatComposer.svelte'; + import ChatComposer, { + composerControlClass, + type RestoreRemovedQuote, + } from './ChatComposer.svelte'; import ComposerError from './ComposerError.svelte'; import { composerSendControl, @@ -236,7 +239,6 @@ import { loadReplyDraft as loadStoredReplyDraft, amendStoredReplyDraft, - restoreQuoteInReplyDraft, registerReplyComposer, replyDraftKey, replyDraftWriter, @@ -256,7 +258,6 @@ takeDraftQuote, type ComposerDraft, type DraftQuote, - type RemovedDraftQuote, } from './composerDraft'; import QuoteExcerpt from './QuoteExcerpt.svelte'; import NoteActivityCard from './NoteActivityCard.svelte'; @@ -455,26 +456,24 @@ // exactly that run so inherited text isn't filed under this session's key. let suppressNextReplySave = false; /** - * Edits a note made to this session's draft (a quote added, its reply - * typed, a removed block put back) while the draft's load was still in - * flight: they are applied once the stored draft has landed, rather than - * being replaced by it, and what was to follow each in the composer (the - * caret into the block, or the field scrolled to it) runs then too, once - * the block is rendered. Keyed to the session they were made for. + * Edits made to this session's draft from outside the composer's inputs + * (a quote a note added, its reply typed there, a removed block put back + * by undo) while the draft's load was still in flight: they are applied + * once the stored draft has landed, rather than being replaced by it, and + * what was to follow each in the composer (the caret into the block, or + * the field scrolled to it) runs then too, once the block is rendered. + * Keyed to the session they were made for. */ let pendingDraftEdits: { sessionId: string; edits: Array<{ edit: (draft: ComposerDraft) => ComposerDraft; then?: () => void }>; } | null = null; /** - * Undo toasts for removed quote blocks still on screen (svelte-sonner - * keeps one up for seconds). A send the backend accepts clears the draft - * they were taken from, and a switch shows another session's; either - * dismisses them, so Undo can't put a block into a draft it never belonged - * to. `draftEpoch` counts the sends, for a click that lands as the toast - * is leaving. + * Counts the drafts sent from this composer. A removed quote block can be + * put back by undo (see `removeQuote`) only into the draft it was taken + * from; a send the backend accepts clears that draft, and an undo made + * after it must not put the block into the next one. */ - let quoteUndoToasts: Array<{ toastId: string | number; sessionId: string }> = []; let draftEpoch = 0; /** The composer's draft as one value (live `$state`; clone before keeping it). */ @@ -584,96 +583,42 @@ pendingDraftEdits.edits.push({ edit: (draft) => draft, then }); } - /** Scroll the composer's field to a quote block, once it is rendered, leaving focus where it is. */ - function revealDraftQuote(quoteId: string) { - tick().then(() => { - if (!closed && active) composerView?.revealQuote(quoteId); - }); - } - /** - * Take a quote block out of the draft. One with a reply typed under it is - * offered back in a toast (there is no other undo for a block): Undo puts - * it back where it was, reply and lead-in text included, and — with a - * pointer, not a touch screen — the caret in its reply; on a touch screen - * the field is scrolled to show it instead. An unanswered block just goes. + * Take a quote block out of the draft, with no notice: the composer makes + * the removal a step of its undo history, and what is returned is how + * that undo puts the block back where it was, reply and lead-in text + * included. The block goes back only into the draft it was taken from, + * while this pane shows it — after a send the backend accepted, or with + * another session in the box, the undo does nothing — and by the same way + * in as a quote a note adds, so one made while the draft is loading again + * is held until it has landed rather than being replaced by it. */ - function removeQuote(id: string) { + function removeQuote(id: string): RestoreRemovedQuote | null { const taken = takeDraftQuote(currentDraft(), id); - if (!taken) return; + if (!taken) return null; sendError = null; setDraft(taken.draft); - if (taken.removed.quote.reply.trim() !== '') offerQuoteUndo(sessionId, taken.removed); - } - - function offerQuoteUndo(sid: string | null, removed: RemovedDraftQuote) { - if (!sid) return; + // Quotes only come with a session's draft; without one there is no + // draft to tell the block's own from, so the removal just stands. + const sid = sessionId; + if (!sid) return null; const epoch = draftEpoch; - const forget = () => { - quoteUndoToasts = quoteUndoToasts.filter((entry) => entry.toastId !== toastId); + return (then) => { + if (closed || !active || sessionId !== sid || epoch !== draftEpoch) return false; + receiveDraftEdit(sid, (draft) => restoreDraftQuote(draft, taken.removed), then); + return true; }; - const toastId = toast('Quote removed from your reply', { - onDismiss: forget, - onAutoClose: forget, - action: { - label: 'Undo', - onClick: () => { - // The toast goes with the press (svelte-sonner deletes it without - // `onDismiss`), so it is forgotten here. - forget(); - // The draft the block was taken from has been sent since. - if (epoch !== draftEpoch) return; - if (active && sessionId === sid) { - // The same way in as a quote a note adds: applied to the - // composer, or held until this session's draft has loaded, so a - // load in flight can't land over it. - // The caret goes back into the reply for a keyboard user; with - // a touch screen that would raise the on-screen keyboard over - // the restored draft, which the user hasn't asked to type in, - // so there the field is only scrolled to the block, which in a - // capped field that has scrolled on may otherwise come back - // out of view. Either follows the block into the composer, so - // a restore held for the load isn't looked for before it lands. - receiveDraftEdit( - sid, - (draft) => restoreDraftQuote(draft, removed), - () => { - if (!viewport.hasCoarsePointer) focusDraftQuoteReply(removed.quote.id); - else revealDraftQuote(removed.quote.id); - } - ); - return; - } - // This pane has moved on: another pane showing the draft takes it, - // or else the stored draft. - restoreQuoteInReplyDraft(sid, removed).catch((err) => { - console.error('[SessionChatPane] Failed to restore the quote:', err); - toast.error('Could not restore the quote'); - }); - }, - }, - }); - quoteUndoToasts = [...quoteUndoToasts, { toastId, sessionId: sid }]; - } - - /** Take down the Undo toasts whose draft is gone or off screen (all, by default). */ - function dismissQuoteUndo(keep: (sessionId: string) => boolean = () => false) { - const going = quoteUndoToasts.filter((entry) => !keep(entry.sessionId)); - if (going.length === 0) return; - quoteUndoToasts = quoteUndoToasts.filter((entry) => keep(entry.sessionId)); - for (const entry of going) toast.dismiss(entry.toastId); } /** * The draft has gone with a message the backend accepted: a block removed * from it has nothing to go back into. Not before — a send that fails puts - * the draft back in the box, and the block's Undo should still hold then. - * (Pressed while the send is in flight, Undo puts the block into the + * the draft back in the box, and undoing the block's removal should still + * hold then. (Undone while the send is in flight, the block goes into the * emptied box, where it is a new draft.) */ function draftSent() { draftEpoch += 1; - dismissQuoteUndo(); } let discoveredFollowupConfig = $state(null); @@ -1212,9 +1157,6 @@ const draftWhenLoadStarted = untrack(() => cloneComposerDraft(currentDraft())); suppressNextReplySave = false; loadedDraftSessionId = null; - // Another session's draft is coming up: an Undo for a block removed from - // the previous one would read as this one's. - dismissQuoteUndo((sid) => sid === id); replyDraftLoad = loadReplyDraft(id, generation, draftWhenLoadStarted); }); @@ -1258,7 +1200,6 @@ receiveDraftEdit(id, (draft) => appendDraftQuote(draft, quote, quoteId)), setQuoteReply: (quoteId, reply) => receiveDraftEdit(id, (draft) => setDraftQuoteReply(draft, quoteId, reply)), - restoreQuote: (removed) => receiveDraftEdit(id, (draft) => restoreDraftQuote(draft, removed)), focusQuoteReply: (quoteId) => whenDraftLoaded(id, () => focusDraftQuoteReply(quoteId)), }); return () => { diff --git a/apps/staged/src/lib/features/sessions/replyDrafts.test.ts b/apps/staged/src/lib/features/sessions/replyDrafts.test.ts index 19a3ac3b9..6dba6cb3e 100644 --- a/apps/staged/src/lib/features/sessions/replyDrafts.test.ts +++ b/apps/staged/src/lib/features/sessions/replyDrafts.test.ts @@ -26,12 +26,7 @@ vi.mock('../../shared/persistentStore', () => ({ import { setStoreValue } from '../../shared/persistentStore'; import { saveDraft, DRAFT_TTL_MS } from '../../shared/draftCache'; -import { - appendDraftQuote, - composerDraftContent, - serializeComposerDraft, - takeDraftQuote, -} from './composerDraft'; +import { serializeComposerDraft } from './composerDraft'; import { addQuoteToReplyDraft, focusReplyDraftQuote, @@ -40,7 +35,6 @@ import { replyDraftKey, replyDraftPresence, replyDraftWriter, - restoreQuoteInReplyDraft, setReplyDraftQuoteReply, startLiveQuoteReply, type ReplyComposerTarget, @@ -147,16 +141,10 @@ describe('addQuoteToReplyDraft', () => { const id = await addQuoteToReplyDraft('s6', quote); await setReplyDraftQuoteReply('s6', id, 'Because.'); - const removed = takeDraftQuote( - appendDraftQuote({ quotes: [], text: '' }, quote, id), - id - )!.removed; - await restoreQuoteInReplyDraft('s6', removed); expect(newer.addQuote).not.toHaveBeenCalled(); expect(older.addQuote).toHaveBeenCalledWith(quote, id); expect(older.setQuoteReply).toHaveBeenCalledWith(id, 'Because.'); - expect(older.restoreQuote).toHaveBeenCalledWith(removed); expect(focusReplyDraftQuote('s6', id)).toBe(true); expect(older.focusQuoteReply).toHaveBeenCalledWith(id); expect(newer.focusQuoteReply).not.toHaveBeenCalled(); @@ -195,45 +183,10 @@ function fakeComposer(): { [K in keyof ReplyComposerTarget]: Mock(), setQuoteReply: vi.fn(), - restoreQuote: vi.fn(), focusQuoteReply: vi.fn(), }; } -describe('restoreQuoteInReplyDraft', () => { - beforeEach(() => { - store.clear(); - }); - - it('puts a removed block back into the stored draft when no composer shows it', async () => { - await addQuoteToReplyDraft('u1', quote, 'first'); - await addQuoteToReplyDraft('u1', { ...quote, quote: 'Second.' }, 'second'); - const before = await loadReplyDraft('u1'); - const taken = takeDraftQuote(before, 'first')!; - await saveDraft(replyDraftKey('u1'), composerDraftContent(taken.draft)); - expect((await loadReplyDraft('u1')).quotes.map((q) => q.id)).toEqual(['second']); - - await restoreQuoteInReplyDraft('u1', taken.removed); - - expect(await loadReplyDraft('u1')).toEqual(before); - }); - - it('goes to the open composer instead of the store', async () => { - const composer = fakeComposer(); - const unregister = registerReplyComposer('u2', composer); - const removed = takeDraftQuote( - { quotes: [{ id: 'q', before: '', ...quote }], text: '' }, - 'q' - )!.removed; - - await restoreQuoteInReplyDraft('u2', removed); - - expect(composer.restoreQuote).toHaveBeenCalledWith(removed); - expect(store.has(replyDraftKey('u2'))).toBe(false); - unregister(); - }); -}); - describe('setReplyDraftQuoteReply', () => { beforeEach(() => { store.clear(); diff --git a/apps/staged/src/lib/features/sessions/replyDrafts.ts b/apps/staged/src/lib/features/sessions/replyDrafts.ts index 5b97a9e0a..120dcea31 100644 --- a/apps/staged/src/lib/features/sessions/replyDrafts.ts +++ b/apps/staged/src/lib/features/sessions/replyDrafts.ts @@ -29,13 +29,11 @@ import { composerDraftContent, composerDraftFromContent, newDraftQuoteId, - restoreDraftQuote, setDraftQuoteReply, summarizeComposerDraft, type ComposerDraft, type ComposerDraftSummary, type NewDraftQuote, - type RemovedDraftQuote, } from './composerDraft'; const REPLY_DRAFT_KEY_PREFIX = 'draft:session-reply:'; @@ -66,8 +64,6 @@ export type ReplyComposerTarget = { addQuote(quote: NewDraftQuote, id: string): void; /** Replace the reply of the quote with this id; nothing if it has been removed. */ setQuoteReply(id: string, reply: string): void; - /** Put a removed quote block back where it was (the composer's Undo). */ - restoreQuote(removed: RemovedDraftQuote): void; /** Put the caret in that quote's reply input, if the block is shown. */ focusQuoteReply(id: string): void; }; @@ -146,28 +142,6 @@ export function setReplyDraftQuoteReply( return amendStoredReplyDraft(sessionId, (draft) => setDraftQuoteReply(draft, id, reply)); } -/** - * Put a quote block a composer removed back into the session's reply draft - * (its Undo; see `takeDraftQuote`), wherever the draft is now: the composer - * showing it — this one, mid-load or not, or another pane's with the same - * session — or else the stored draft. A write to the store under an open - * composer would be overwritten by its next save, as with - * `addQuoteToReplyDraft`. Rejects when the stored draft couldn't be written. - */ -export function restoreQuoteInReplyDraft( - sessionId: string, - removed: RemovedDraftQuote -): Promise { - const live = liveComposers(sessionId); - if (live.length > 0) { - // A composer that didn't remove the block still holds it; its restore is - // a no-op (see `restoreDraftQuote`). - for (const composer of live) composer.restoreQuote(removed); - return Promise.resolve(); - } - return amendStoredReplyDraft(sessionId, (draft) => restoreDraftQuote(draft, removed)); -} - /** * Put the caret in a quote block's reply input. Only a composer showing the * draft can; returns whether one did. With several showing it, the newest diff --git a/apps/staged/src/lib/features/sessions/testStubs/ComposerDraftHost.svelte b/apps/staged/src/lib/features/sessions/testStubs/ComposerDraftHost.svelte new file mode 100644 index 000000000..4ab76d924 --- /dev/null +++ b/apps/staged/src/lib/features/sessions/testStubs/ComposerDraftHost.svelte @@ -0,0 +1,68 @@ + + + + diff --git a/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte b/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte index bde56f55e..61a55a9b5 100644 --- a/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte +++ b/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte @@ -9,9 +9,13 @@ count it shows as `data-quote-count`, and offers a remove button per quote (`data-testid="remove-quote-"`) that calls `onRemoveQuote` as the composer's X would, a Send button (`data-testid="composer-send"`) that - calls `onSend`, a `focusQuoteReply` that places no caret but records the - ids asked for on the input (`data-focused-quotes`), and a `revealQuote` - that scrolls nothing but records likewise (`data-revealed-quotes`). + calls `onSend`, and a `focusQuoteReply` that places no caret. + + The composer's undo history is stood in for by a stack of the restores + `onRemoveQuote` returned: an undo button (`data-testid="undo-remove-quote"`) + runs the newest, as the browser's undo would, records on the input whether + the pane took it (`data-undo-results`), and the ids of the quotes the pane + then reported back in the composer (`data-restored-quotes`). --> @@ -60,8 +73,8 @@ data-value={value} data-send-split={sendControl?.split} data-quote-count={quotes.length} - data-focused-quotes={focusedQuotes.join(' ')} - data-revealed-quotes={revealedQuotes.join(' ')} + data-undo-results={undoResults.join(' ')} + data-restored-quotes={restoredQuotes.join(' ')} oninput={(e) => (value = e.currentTarget.textContent ?? '')} >
{#each quotes as quote (quote.id)} @@ -69,9 +82,10 @@ type="button" aria-label="Remove quote" data-testid="remove-quote-{quote.id}" - onclick={() => onRemoveQuote?.(quote.id)} + onclick={() => removeQuote(quote.id)} > {/each} + {#if sendError} diff --git a/apps/staged/src/lib/shared/NativeUndoAnchor.component.test.ts b/apps/staged/src/lib/shared/NativeUndoAnchor.component.test.ts new file mode 100644 index 000000000..7b8e57070 --- /dev/null +++ b/apps/staged/src/lib/shared/NativeUndoAnchor.component.test.ts @@ -0,0 +1,209 @@ +import { flushSync, mount, unmount } from 'svelte'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import NativeUndoAnchor from './NativeUndoAnchor.svelte'; +import { installNativeUndoStack, type NativeUndoStack } from './testStubs/nativeUndoStack'; + +// The anchor's side of the browser's undo stack: what `record` puts on it, +// and which steps run as undo and redo walk it. jsdom has no such stack, so +// one is stood in (testStubs/nativeUndoStack.ts); that a real browser keeps +// the anchor's edits in order with those of the inputs around it is what +// the component is built on, and isn't something jsdom can show. + +let component: ReturnType, { record: Recorder }>> | null = null; +let target: HTMLElement; +let stack: NativeUndoStack; + +type Recorder = (step: { undo: () => void; redo: () => void }) => boolean; + +function render() { + target = document.createElement('div'); + document.body.appendChild(target); + component = mount(NativeUndoAnchor, { target }); + flushSync(); +} + +function record(name: string, log: string[]): boolean { + return component!.record({ + undo: () => log.push(`undo ${name}`), + redo: () => log.push(`redo ${name}`), + }); +} + +function anchor(): HTMLElement { + const el = target.querySelector('.native-undo-anchor'); + if (!el) throw new Error('no anchor'); + return el; +} + +/** An editable of the page's own, for focus and typing around the anchor's steps. */ +function editor(text = ''): HTMLElement { + const el = document.createElement('div'); + el.setAttribute('contenteditable', 'true'); + el.textContent = text; + document.body.appendChild(el); + return el; +} + +beforeEach(() => { + stack = installNativeUndoStack(); +}); + +afterEach(() => { + if (component) unmount(component); + component = null; + stack.uninstall(); + document.body.innerHTML = ''; + window.getSelection()?.removeAllRanges(); +}); + +describe('NativeUndoAnchor', () => { + it('stays out of the tab order and the accessibility tree', () => { + render(); + + expect(anchor().getAttribute('contenteditable')).toBe('true'); + expect(anchor().tabIndex).toBe(-1); + expect(anchor().getAttribute('aria-hidden')).toBe('true'); + }); + + it('records a step as one edit of its own, and runs it as undo and redo walk the stack', () => { + render(); + const log: string[] = []; + + expect(record('a', log)).toBe(true); + expect(anchor().textContent).toHaveLength(1); + // Recording is not an undo or a redo. + expect(log).toEqual([]); + + expect(stack.undo()).toBe(true); + expect(anchor().textContent).toBe(''); + expect(stack.redo()).toBe(true); + expect(anchor().textContent).toHaveLength(1); + expect(log).toEqual(['undo a', 'redo a']); + }); + + it('keeps its steps in order with the typing before and after them', () => { + render(); + const input = editor(); + const log: string[] = []; + + stack.type(input, 'abc'); + record('a', log); + stack.type(input, 'def'); + + stack.undo(); + expect(input.textContent).toBe('abc'); + expect(log).toEqual([]); + stack.undo(); + expect(input.textContent).toBe('abc'); + expect(log).toEqual(['undo a']); + stack.undo(); + expect(input.textContent).toBe(''); + + stack.redo(); + expect(input.textContent).toBe('abc'); + expect(log).toEqual(['undo a']); + stack.redo(); + expect(log).toEqual(['undo a', 'redo a']); + stack.redo(); + expect(input.textContent).toBe('abcdef'); + }); + + it('undoes the newest step first, and drops the undone ones when another is recorded', () => { + render(); + const log: string[] = []; + record('a', log); + record('b', log); + + stack.undo(); + stack.undo(); + expect(log).toEqual(['undo b', 'undo a']); + stack.redo(); + expect(log).toEqual(['undo b', 'undo a', 'redo a']); + + // Recorded over the undone `b`, as a new edit drops the browser's redo. + record('c', log); + expect(stack.redo()).toBe(false); + stack.undo(); + stack.undo(); + expect(log).toEqual(['undo b', 'undo a', 'redo a', 'undo c', 'undo a']); + }); + + it('runs every step between where the stack stood and where it stands', () => { + render(); + const log: string[] = []; + record('a', log); + record('b', log); + + // A browser that walks both edits as one (WebKit groups those made + // while handling the same event) reports the pair in a single change. + anchor().textContent = ''; + anchor().dispatchEvent(new InputEvent('input', { bubbles: true, inputType: 'historyUndo' })); + expect(log).toEqual(['undo b', 'undo a']); + }); + + it('gives focus and the caret back after recording', () => { + render(); + const input = editor('some text'); + input.focus(); + const text = input.firstChild!; + window.getSelection()!.setBaseAndExtent(text, 4, text, 4); + + record('a', []); + + expect(document.activeElement).toBe(input); + const selection = window.getSelection()!; + expect(selection.anchorNode).toBe(text); + expect(selection.anchorOffset).toBe(4); + }); + + it('takes no focus to keep, and leaves no selection of its own, when recording with none', () => { + render(); + + record('a', []); + + expect(document.activeElement).toBe(document.body); + const selection = window.getSelection()!; + expect(anchor().contains(selection.anchorNode)).toBe(false); + }); + + it('gives focus back before a step runs, so the step finds it where the user had it', () => { + render(); + const input = editor('some text'); + let focusedDuringUndo: Element | null = null; + component!.record({ + undo: () => (focusedDuringUndo = document.activeElement), + redo: () => {}, + }); + input.focus(); + const text = input.firstChild!; + window.getSelection()!.setBaseAndExtent(text, 2, text, 2); + + // The browser moves focus to the anchor as it undoes the edit there. + stack.undo(); + + expect(focusedDuringUndo).toBe(input); + expect(document.activeElement).toBe(input); + expect(window.getSelection()!.anchorOffset).toBe(2); + }); + + it('keeps its edits to itself', () => { + render(); + const heard = vi.fn(); + target.addEventListener('input', heard); + + record('a', []); + stack.undo(); + + expect(heard).not.toHaveBeenCalled(); + }); + + it('reports a step it could not record, and runs nothing for it', () => { + stack.uninstall(); + render(); + const log: string[] = []; + + expect(record('a', log)).toBe(false); + expect(anchor().textContent).toBe(''); + expect(log).toEqual([]); + }); +}); diff --git a/apps/staged/src/lib/shared/NativeUndoAnchor.svelte b/apps/staged/src/lib/shared/NativeUndoAnchor.svelte new file mode 100644 index 000000000..3f8d979b3 --- /dev/null +++ b/apps/staged/src/lib/shared/NativeUndoAnchor.svelte @@ -0,0 +1,174 @@ + + + + + + + + + diff --git a/apps/staged/src/lib/shared/testStubs/nativeUndoStack.ts b/apps/staged/src/lib/shared/testStubs/nativeUndoStack.ts new file mode 100644 index 000000000..74074c064 --- /dev/null +++ b/apps/staged/src/lib/shared/testStubs/nativeUndoStack.ts @@ -0,0 +1,105 @@ +/** + * A stand-in for the browser's undo stack, which jsdom doesn't have. + * + * A browser keeps one stack per document for the edits made to its editable + * content: `document.execCommand('insertText')` and typing put an edit on + * it, and undo and redo walk it, each time changing the edited element and + * raising `beforeinput` and `input` there with `historyUndo` or + * `historyRedo`. This models as much of that as the app's undo wiring + * leans on (see NativeUndoAnchor.svelte), so component tests can type, + * undo and redo across several editable elements in the order a browser + * would: + * + * - `execCommand('insertText')` appends to the editable element holding the + * selection, and drops whatever had been undone; + * - undo and redo move focus and the caret to the element they change, as + * a browser does, between its `beforeinput` and its `input`; + * - an edit to an element since taken out of the document is still walked, + * but changes nothing and raises no event. + */ + +type Edit = { host: HTMLElement; before: string; after: string }; + +export type NativeUndoStack = { + /** Type `text` at the end of `host`, as one edit. */ + type(host: HTMLElement, text: string): void; + /** Undo the newest edit; false when there is none. */ + undo(): boolean; + /** Redo the edit undone last; false when there is none. */ + redo(): boolean; + /** Put back the `document.execCommand` that was there before. */ + uninstall(): void; +}; + +function caretToEnd(host: HTMLElement) { + const selection = window.getSelection(); + selection?.selectAllChildren(host); + selection?.collapseToEnd(); +} + +function editableHostOf(node: Node | null): HTMLElement | null { + const element = node instanceof HTMLElement ? node : (node?.parentElement ?? null); + return element?.closest('[contenteditable="true"]') ?? null; +} + +export function installNativeUndoStack(): NativeUndoStack { + const done: Edit[] = []; + let undone: Edit[] = []; + const original = Object.getOwnPropertyDescriptor(document, 'execCommand'); + + function insertText(text: string): boolean { + const host = editableHostOf(window.getSelection()?.anchorNode ?? null); + if (!host) return false; + const before = host.textContent ?? ''; + host.dispatchEvent(new InputEvent('beforeinput', { bubbles: true, inputType: 'insertText' })); + host.textContent = before + text; + caretToEnd(host); + done.push({ host, before, after: before + text }); + undone = []; + host.dispatchEvent(new InputEvent('input', { bubbles: true, inputType: 'insertText' })); + return true; + } + + function walk(edit: Edit, text: string, inputType: 'historyUndo' | 'historyRedo') { + const { host } = edit; + if (!host.isConnected) return; + host.dispatchEvent(new InputEvent('beforeinput', { bubbles: true, inputType })); + host.focus(); + host.textContent = text; + caretToEnd(host); + host.dispatchEvent(new InputEvent('input', { bubbles: true, inputType })); + } + + Object.defineProperty(document, 'execCommand', { + configurable: true, + writable: true, + value: (command: string, _showUi?: boolean, value?: string) => + command === 'insertText' && insertText(value ?? ''), + }); + + return { + type(host, text) { + host.focus(); + caretToEnd(host); + insertText(text); + }, + undo() { + const edit = done.pop(); + if (!edit) return false; + undone.push(edit); + walk(edit, edit.before, 'historyUndo'); + return true; + }, + redo() { + const edit = undone.pop(); + if (!edit) return false; + done.push(edit); + walk(edit, edit.after, 'historyRedo'); + return true; + }, + uninstall() { + if (original) Object.defineProperty(document, 'execCommand', original); + else delete (document as Partial).execCommand; + }, + }; +} From 2859b6e7dfefa53868b36821d774dbb8c9cbef16 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 17:03:01 +1100 Subject: [PATCH 2/2] fix(staged): withdraw a held quote restore when its removal is redone Review feedback on PR #1068: redoing a quote removal while the undo before it was still waiting on the draft's load did nothing, so the quote came back once the load landed. Root cause: when a pane is reopened its draft loads again, and an undo made in that window is queued by `receiveDraftEdit` in SessionChatPane.svelte rather than applied. The pane answered that it took the undo, so the composer's step marked the quote as back. A redo before the load landed looked for the quote in the box, found none, and gave up without touching the queued restore. The removal handle `onRemoveQuote` returns is now an object, `RemovedQuote`, with `restore` as before and a new `withdraw`. The pane's `restore` keeps the queue entry `receiveDraftEdit` now returns for a held edit, and `withdraw` takes that entry out of the queue (`withdrawPendingDraftEdit`), so it is never applied and its follow-up never moves the caret. ChatComposer's redo first asks the removal undo last handed back to withdraw. When that succeeds the step is as it stood before the undo, with its original restore and caret position, and a later undo puts the quote back. When there is nothing to withdraw, the restore already landed, and redo takes the quote out of the box as before. A queue the withdrawal empties is dropped, so a pane that then moves on sends no no-op amend to the stored draft. Tests: the pane test undoes and redoes inside a held draft load and checks the quote stays out, the composer is never told it returned, and a later undo restores the original draft. A second pane test checks no stored-draft write follows a withdrawn undo. Two composer tests cover redo against a held undo and against one that has landed. The two test stubs speak the new handle; PromptInputStub gains a redo button. Verified: ChatComposer and SessionChatPane component tests pass (36), and the three tests for the fix fail with the withdrawal disabled. svelte-check passes. The full vitest suite was not run locally (machine under heavy load); CI covers it. Not verified: the running app. The window is the draft's load after a pane is reopened, which was only driven through the mocked load in tests. Not covered: an undo held during one load, flushed to the stored draft because the pane moved on, then redone during the next load. That redo still does nothing until the load lands. The step's state stays consistent with the draft, and a redo after the load works. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- .../sessions/ChatComposer.component.test.ts | 43 +++++++++++ .../lib/features/sessions/ChatComposer.svelte | 51 +++++++++---- .../SessionChatPane.component.test.ts | 75 +++++++++++++++++++ .../features/sessions/SessionChatPane.svelte | 65 +++++++++++----- .../testStubs/ComposerDraftHost.svelte | 51 +++++++++++-- .../sessions/testStubs/PromptInputStub.svelte | 38 +++++++--- 6 files changed, 271 insertions(+), 52 deletions(-) diff --git a/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts b/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts index f11822d56..28156ca19 100644 --- a/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts +++ b/apps/staged/src/lib/features/sessions/ChatComposer.component.test.ts @@ -532,6 +532,49 @@ describe('ChatComposer quote removal', () => { expect(host.draft()).toEqual(draft); }); + it('withdraws an undo the parent is still holding on redo, rather than letting it land', async () => { + const draft = { quotes: [quote('a', '', 'a reply')], text: '' }; + renderDraft(draft); + await pressRemove('a'); + + // The parent accepts the undo but has not put the quote back yet (the + // pane's draft is loading): redo finds no quote in the box to take out. + host.holdUndo(true); + await undo(); + expect(quoteIds()).toEqual([]); + await redo(); + + host.landHeldUndo(); + await settle(); + expect(quoteIds()).toEqual([]); + // Taken back, not removed a second time. + expect(host.removedIds()).toEqual(['a']); + + // The removal is the step's to undo still. + host.holdUndo(false); + await undo(); + expect(host.draft()).toEqual(draft); + await redo(); + expect(quoteIds()).toEqual([]); + expect(host.removedIds()).toEqual(['a', 'a']); + }); + + it('takes the quote out again on redo once a held undo has landed', async () => { + const draft = { quotes: [quote('a', '', 'a reply')], text: '' }; + renderDraft(draft); + await pressRemove('a'); + + host.holdUndo(true); + await undo(); + host.landHeldUndo(); + await settle(); + expect(host.draft()).toEqual(draft); + + await redo(); + expect(quoteIds()).toEqual([]); + expect(host.removedIds()).toEqual(['a', 'a']); + }); + it('records nothing for a removal the parent gives no way back from', async () => { const props = render([quote('a')]); const anchor = target.querySelector('.native-undo-anchor'); diff --git a/apps/staged/src/lib/features/sessions/ChatComposer.svelte b/apps/staged/src/lib/features/sessions/ChatComposer.svelte index 292884354..6abce22e9 100644 --- a/apps/staged/src/lib/features/sessions/ChatComposer.svelte +++ b/apps/staged/src/lib/features/sessions/ChatComposer.svelte @@ -55,13 +55,23 @@ the tallest control. */ const attachButtonClass = `${composerControlClass} h-auto [&_svg]:!size-4`; - /** - * Puts a quote `onRemoveQuote` took out back where it was, reply and - * lead-in text included, and answers whether it did: false once the draft - * it was taken from is no longer the one in the composer. `then` runs when - * the quote is in the composer again, which may be later than the call. - */ - export type RestoreRemovedQuote = (then: () => void) => boolean; + /** A quote `onRemoveQuote` took out, as the parent's way back from that. */ + export type RemovedQuote = { + /** + * Puts the quote back where it was, reply and lead-in text included, and + * answers whether it did: false once the draft it was taken from is no + * longer the one in the composer. `then` runs when the quote is in the + * composer again, which may be later than the call. + */ + restore: (then: () => void) => boolean; + /** + * Takes back a `restore` the parent accepted but has not carried out yet + * (its `then` has not run), so the quote stays out and `then` never + * runs. Answers whether there was one to take back: false when the + * quote is back in the composer, or was never on its way. + */ + withdraw: () => boolean; + }; diff --git a/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte b/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte index 61a55a9b5..1dcf82574 100644 --- a/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte +++ b/apps/staged/src/lib/features/sessions/testStubs/PromptInputStub.svelte @@ -11,13 +11,18 @@ composer's X would, a Send button (`data-testid="composer-send"`) that calls `onSend`, and a `focusQuoteReply` that places no caret. - The composer's undo history is stood in for by a stack of the restores + The composer's undo history is stood in for by a stack of the removals `onRemoveQuote` returned: an undo button (`data-testid="undo-remove-quote"`) - runs the newest, as the browser's undo would, records on the input whether - the pane took it (`data-undo-results`), and the ids of the quotes the pane - then reported back in the composer (`data-restored-quotes`). + restores the newest, as the browser's undo would, records on the input + whether the pane took it (`data-undo-results`), and the ids of the quotes + the pane then reported back in the composer (`data-restored-quotes`). A + redo button (`data-testid="redo-remove-quote"`) takes the quote last + undone out again as the composer's redo does: it withdraws an undo the + pane is still holding, and otherwise removes the quote from the box. -->