From 685bf989b9043b8efe08668c74f105b21c3a5f74 Mon Sep 17 00:00:00 2001 From: Jai Date: Sun, 11 Oct 2026 10:47:31 +1100 Subject: [PATCH] fix(editor): let Undo revert every write of a multi-request gesture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A gesture that writes more than once — removing a node whose unused imports are then pruned, say — owed its undo step one answer, so the second write's inverse was dropped and Undo put the markup back without the imports, leaving the page with a component it no longer declared. Every send now goes through one loop (sendRequests), and before each request after the first that loop owes the step one more answer (oweAnother): a pending step waits for one more, a step that had just settled is pending again with what it already holds. The replacement fallback sends several node rewrites through the same loop, so it is covered the same way. Tests pin a two-write gesture's undo and redo, and a refusal mid-way. Co-Authored-By: Claude Fable 5.1 --- src/editor/pageEdits.ts | 29 ++++++++- test/renderer/editor/pageEdits.test.js | 87 +++++++++++++++++++++++++- 2 files changed, 114 insertions(+), 2 deletions(-) diff --git a/src/editor/pageEdits.ts b/src/editor/pageEdits.ts index 13dceb97..e559f060 100644 --- a/src/editor/pageEdits.ts +++ b/src/editor/pageEdits.ts @@ -358,6 +358,28 @@ function owe(record: EditsRecord): void { } } +// One more write of a gesture the step is already waiting on, or has just +// settled (sendRequests, a later request). A record no step reads — a preview's, +// dropped — stays dropped: it takes no answers. +function oweAnother(record: EditsRecord): void { + const outcome = record.outcome; + switch (outcome.tag) { + case 'pending': + writeOutcome(record, { ...outcome, waiting: outcome.waiting + 1 }); + return; + case 'applied': + writeOutcome(record, { tag: 'pending', waiting: 1, applied: outcome.applied }); + return; + case 'folded': + case 'dropped': + return; + default: { + const exhaustive: never = outcome; + return exhaustive; + } + } +} + /** The reference main needs for a node of the origin, or undefined when the * node is not one main can name in the page's own bytes: not in this parse, * inside a chunk file, or without a source range. */ @@ -469,7 +491,12 @@ async function sendRequests( requests: readonly Edit[], ): Promise { const replies: PageEdited[] = []; - for (const edit of requests) { + for (const [index, edit] of requests.entries()) { + if (index > 0) { + // A later request of the same step: the step is owed its answer too, or + // Undo would hold the inverse of the first write only. + oweAnother(input.record); + } const request = { pagePath: input.path, authoredChecksum: input.origin.checksum, edit }; const answer = await input.send(request); if (answer.ok) { diff --git a/test/renderer/editor/pageEdits.test.js b/test/renderer/editor/pageEdits.test.js index 30fe331b..51f947ac 100644 --- a/test/renderer/editor/pageEdits.test.js +++ b/test/renderer/editor/pageEdits.test.js @@ -7,7 +7,8 @@ // ever saved as a whole model; a transient failure sends it again, a write that // may have landed never blind; // and every applied write leaves its undo step the inverse that restores the -// file (several steps in one write: the newest, the rest folded). +// file (several steps in one write: the newest, the rest folded; several +// writes of one gesture: the step waits for every one of them). // Method: the real modules, bundled with esbuild, driven directly; the send // function is a fake for the outcome table, then main's real handlers in the // windowless harness for the end-to-end run, undo included, on a temporary @@ -28,6 +29,7 @@ esbuild.buildSync({ entryPoints: { pageEdits: repoPath('src/editor/pageEdits.ts'), editGestures: repoPath('src/editor/editGestures.ts'), + insertGestures: repoPath('src/editor/insertGestures.ts'), }, outdir: buildDirectory, bundle: true, @@ -37,6 +39,7 @@ esbuild.buildSync({ }); const edits = require(path.join(buildDirectory, 'pageEdits.js')); const gestures = require(path.join(buildDirectory, 'editGestures.js')); +const insertGestures = require(path.join(buildDirectory, 'insertGestures.js')); const sum = (digit) => String(digit).repeat(64); const REF = { path: [0], kind: 'element', span: { start: 0, end: 4 } }; @@ -165,6 +168,19 @@ test('sending a gesture: every outcome, and what the answers mean', async () => applied.outcome.replies.map((reply) => reply.checksum), [sum(2), sum(3)], ); + // A gesture of two requests writes twice, and its step undoes both: the + // first reply settled a step owed one write, and the second was lost. + assert.deepEqual( + applied.step.outcome, + { + tag: 'applied', + applied: [ + { checksum: sum(2), inverse: [] }, + { checksum: sum(3), inverse: [] }, + ], + }, + 'the undo step holds every write of the gesture', + ); const unstated = await sent([], 1, false); assert.deepEqual( @@ -182,6 +198,11 @@ test('sending a gesture: every outcome, and what the answers mean', async () => const busy = await sent([{ ok: false, error: { code: 'backpressured', message: 'busy' } }], 2); assert.deepEqual(busy.outcome, { tag: 'retry', message: 'busy' }, 'never accepted: sent again'); + assert.deepEqual( + busy.step.outcome, + { tag: 'pending', waiting: 1, applied: [] }, + 'a second request that never went out is not waited for: the retry owes it', + ); const maybe = await sent([ PAGE_OK(sum(2)), @@ -443,6 +464,70 @@ test('requests reach the page as splices; the undo step restores every byte', as assert.equal(fs.readFileSync(file, 'utf8'), text, 'undone on the engine, byte for byte'); }); +test('a delete and the prune it leaves undo together: every import comes back', async (context) => { + // The shape of a real loss: deleting the wrapper that held every component + // (App.tsx, removeNode) removes the nodes and then, as a second request of the + // same gesture, the imports nothing reads any more. Undo reverts the step's + // writes newest first; a step that learnt only the first write's inverse put + // the components back and left their imports out. + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'stacki-page-edits-')); + fs.mkdirSync(path.join(root, 'src/pages'), { recursive: true }); + fs.mkdirSync(path.join(root, 'user')); + fs.writeFileSync(path.join(root, 'package.json'), '{"dependencies":{"astro":"*"}}'); + const { mainHarness } = await import('../../helpers/mainHarness.ts'); + const harness = mainHarness(path.join(root, 'user')); + context.after(() => { + harness.dispose(); + fs.rmSync(root, { recursive: true, force: true }); + }); + const file = path.join(root, 'src/pages/index.astro'); + const text = + '---\n' + + 'import Hero from "../components/Hero.astro";\n' + + 'import Note from "../components/Note.astro";\n' + + '---\n' + + '
\n \n
\n\n'; + fs.writeFileSync(file, text); + const read = parsePageDiskRead(await harness.invoke('page:read', file)); + assert.ok(read.editable); + const origin = { checksum: read.checksum, source: read.source, model: read.model }; + const wrapper = read.model.nodes.find((node) => node.kind === 'element' && node.name === 'main'); + assert.ok(wrapper !== undefined); + const removal = insertGestures.removalGesture([wrapper.id], { urgency: true }); + const afterRemoval = removal.apply(read.model); + // What the delete leaves unused: Hero's import, read by nothing now. + const prune = (model) => ({ + ...model, + imports: model.imports.filter((member) => member.name !== 'Hero'), + }); + const options = { coalesceKey: undefined, urgency: true }; + const pruning = gestures.frontmatterGesture(afterRemoval, options, prune); + const gesture = gestures.sequence(removal, pruning); + const store = new edits.EditDrafts(); + const step = record(); + assert.equal(store.addGesture(file, gesture, step), 'queued'); + const entry = store.shift(file); + assert.equal(entry?.tag, 'gesture'); + const send = async (request) => parsePageEditResult(await harness.invoke('page:edit', request)); + const outcome = await edits.sendGesture({ path: file, origin, gesture, record: step, send }); + assert.equal(outcome.tag, 'applied'); + assert.equal(outcome.replies.length, 2, 'the removal, then the prune'); + const written = fs.readFileSync(file, 'utf8'); + assert.ok(!written.includes('Hero'), 'the wrapper and the import only it needed are gone'); + assert.ok(written.includes('import Note'), 'the import still read stays'); + assert.equal(step.outcome.tag, 'applied'); + assert.equal(step.outcome.applied.length, 2, 'the step waited for both writes'); + for (const applied of [...step.outcome.applied].reverse()) { + const undone = await send({ + pagePath: file, + authoredChecksum: applied.checksum, + edit: { tag: 'revert', hunks: applied.inverse }, + }); + assert.ok(undone.ok); + } + assert.equal(fs.readFileSync(file, 'utf8'), text, 'the nodes and their imports, byte for byte'); +}); + test( 'visual attributes stay editable on ' + 'markup with runtime-provided children', async (context) => {