From 598e2b6638a0625d9303a0d6c909f1d46628f20f Mon Sep 17 00:00:00 2001 From: Jai Date: Wed, 7 Oct 2026 15:14:26 +1100 Subject: [PATCH] fix(editor): let Undo revert every write of a multi-request gesture Deleting a node is one gesture of two requests (App.tsx, removeNode): the removal, then a set-frontmatter that prunes the imports nothing reads any more. The undo step was owed one write when the gesture was queued (addGesture), so the first reply settled it to `applied` and recordApplied dropped the second write's inverse. Undo then put the nodes back without their imports, and Astro failed the page with " is not defined". sendGesture now owes the step one more write right before each later request goes out. Owing lazily keeps a retried gesture honest: a request that never went out (a retry stops at the first) is never waited for. A record no step reads (a preview's, dropped) stays dropped. Reproduced on 0.1.37 by deleting a page's wrapper element and pressing Undo: the wrapper returned, its nine component imports did not. Tests: the fake-send outcome table pins the step holding every write of a two-request gesture and nothing extra owed on a retry; an end-to-end case through main's real handlers deletes a wrapper, lets the prune follow, reverts the step, and expects the file byte for byte. Co-Authored-By: Claude Fable 5.1 --- src/editor/pageEdits.ts | 34 +++++++++- test/renderer/editor/pageEdits.test.js | 87 +++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/src/editor/pageEdits.ts b/src/editor/pageEdits.ts index c1ecba7c..74ada8ea 100644 --- a/src/editor/pageEdits.ts +++ b/src/editor/pageEdits.ts @@ -357,6 +357,28 @@ function owe(record: EditsRecord): void { } } +// One more write of a gesture the step is already waiting on, or has just +// settled (sendGesture, 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. */ @@ -431,7 +453,17 @@ export async function sendGesture(input: { return { tag: 'refused', reason, diskChecksum: origin.checksum, replies: [] }; } const replies: PageEdited[] = []; - for (const edit of requests) { + for (const [index, edit] of requests.entries()) { + // The step was owed one write when its gesture was queued (addGesture). A + // gesture of several requests — a removal and the frontmatter prune it + // leaves (`sequence`) — writes once per request, and the step must wait for + // every one: settled by the first reply, it would take no later inverse + // (recordApplied), and Undo would put the nodes back without their + // imports. Owed right before the request goes out, so one that never + // goes (a retry stops at the first) is never waited for. + if (index > 0) { + oweAnother(input.record); + } const request = { pagePath: input.path, authoredChecksum: 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 e512f634..08666316 100644 --- a/test/renderer/editor/pageEdits.test.js +++ b/test/renderer/editor/pageEdits.test.js @@ -6,7 +6,8 @@ // cannot be stated is refused — nothing is 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 @@ -27,6 +28,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, @@ -36,6 +38,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 } }; @@ -164,6 +167,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( @@ -181,6 +197,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)), @@ -341,6 +362,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( 'insertGesture stands the new node beside the ' + 'one at its place, or inside an empty parent', () => {