From a651d4d2dac9dbc1eb2c19aa7c27b9397519e9c3 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Tue, 8 Sep 2026 12:40:17 +0300 Subject: [PATCH] app: a variable row's own delete button can survive a sibling row's blur Round 3 of adversarial review, reproduced live: type an uncommitted edit into one Variables row, then click a DIFFERENT row's own delete button without blurring the first. The click's mousedown blurs the edited field, which commits and triggers a full non-keyed redraw of the whole list (inspector.js draw() -> node.replaceChildren()) -- destroying the delete button mid-gesture, so the click that started on it never arrives. No error, nothing removed, no sign anything happened. Fixed with the same mousedown+preventDefault idiom this session's own autocomplete.js already uses for the same reason: stop the browser's default focus-move (and the blur/commit/redraw it would trigger) before it can pull the target out from under its own pending click. Confirmed with a Playwright test reproducing the exact sequence. --- app/shell/inspector.js | 24 ++++++++++++++++++++++-- app/tests-layout/shortcuts.spec.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/app/shell/inspector.js b/app/shell/inspector.js index dc2fc5cc..f0b70912 100644 --- a/app/shell/inspector.js +++ b/app/shell/inspector.js @@ -16,6 +16,27 @@ const RATINGS = [ { value: "disliked", label: "Disliked" }, ]; +/** + * A "✕" that removes something the moment it is pressed, not the moment a + * browser would normally call it clicked. + * + * Between a text field's blur-commit and its own click, a browser blurs the + * field FIRST -- and a commit here triggers a full, non-keyed redraw of this + * whole list (draw() -> node.replaceChildren()). Wire this to `onClick` and + * pressing ✕ right after typing in ANOTHER row's field destroys this exact + * button mid-gesture: the field's blur/commit/redraw runs before the click + * the mousedown started ever reaches it, so the press does nothing and says + * nothing. `mousedown` fires first and `preventDefault()` on it stops the + * browser's own focus-move (and so the blur it would otherwise cause) before + * this handler's own redraw gets the chance to pull the rug out from under + * itself. + */ +function removeButton(onRemove) { + const btn = composer.button.sm({ label: "✕", tone: "danger" }); + btn.node.addEventListener("mousedown", (e) => { e.preventDefault(); onRemove(); }); + return btn; +} + export function createInspector({ project, onRename } = {}) { const host = composer.region.stack({ gap: "sm", fill: true }); let tab = "scene"; @@ -168,8 +189,7 @@ export function createInspector({ project, onRename } = {}) { onCommit: (value) => project.setVariables(project.variables.map( (x, j) => (j === i ? { ...x, value } : x))), }), - composer.button.sm({ label: "✕", tone: "danger", - onClick: () => project.setVariables(project.variables.filter((_, j) => j !== i)) }), + removeButton(() => project.setVariables(project.variables.filter((_, j) => j !== i))), ] }), })), composer.button.md({ label: "+ Add variable", diff --git a/app/tests-layout/shortcuts.spec.js b/app/tests-layout/shortcuts.spec.js index c7283e05..06b069b4 100644 --- a/app/tests-layout/shortcuts.spec.js +++ b/app/tests-layout/shortcuts.spec.js @@ -147,3 +147,33 @@ test("each Generate draws a fresh seed for shortcut expansion, not a fixed hash expect(seeds.every((s) => s > 0)).toBe(true); expect(new Set(seeds).size, "the same seed was sent twice in a row").toBeGreaterThan(1); }); + +test("removing a variable works even mid-edit on a different row", async ({ page }) => { + // A real race: row0's blur-commit triggers a full, non-keyed redraw of + // this whole list (inspector.js's draw() -> node.replaceChildren()) -- + // wired to a plain click, pressing row1's ✕ right after typing in row0 + // (without ever blurring it first) destroys row1's own button mid- + // gesture, so the click that started on it never arrives and nothing is + // removed. mousedown+preventDefault on the ✕ (inspector.js's + // removeButton()) is what this proves still works. + await app(page); + await page.getByRole("tab", { name: "Project" }).click(); + + const add = page.getByRole("button", { name: "+ Add variable" }); + await add.click(); + await page.locator('input[placeholder="name"]').last().fill("row0"); + await add.click(); + await page.locator('input[placeholder="name"]').last().fill("row1"); + + const rowCount = await page.locator('input[placeholder="name"]').count(); + // Type into row0's name field WITHOUT blurring it, then press row1's own + // ✕ (the last one -- rows are appended, never reordered) in one motion -- + // the exact sequence the race needs. + const row0Name = page.locator('input[placeholder="name"]').nth(rowCount - 2); + await row0Name.pressSequentially("-edited", { delay: 20 }); + const removeButtons = page.getByRole("button", { name: "✕" }); + await removeButtons.last().click(); + + await expect(page.locator('input[placeholder="name"]')).toHaveCount(rowCount - 1); + await expect(page.locator("body")).not.toContainText("$row1"); +});