From 21b303ba7c1444140a95f40213cefe42e831395c Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 02:33:11 -0400 Subject: [PATCH] fix(ui): stop a closing dialog answering the next question MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `confirm-dialog` is one singleton for every confirmation in the app, and `wa-dialog` reports its close asynchronously: `open = false` starts an animation and `wa-hide` arrives after it. So a hide belonging to a question already answered can land after the *next* question has opened, and cancel it — the user is asked something, the dialog vanishes on its own, and the call site is told they said no. Each ask now carries an id. `close` ignores an id that no longer names the question on screen, the button handlers pass none (they always mean the current one), and only the `wa-hide` handler carries one, because only `wa-hide` can arrive late. Found by writing two `confirmAction()` tests in one file: the second could not be accepted at all, because the first one's hide had cancelled it before the click landed. Reaching it in the app needs two confirmations close together, which the album page's "Apply tags" makes possible. --- .../confirm-dialog/confirm-dialog.ts | 42 +++++++++++++++++-- .../test/components/confirm-dialog.test.ts | 39 +++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/frontend/src/components/confirm-dialog/confirm-dialog.ts b/frontend/src/components/confirm-dialog/confirm-dialog.ts index 220f5c8..6c138d5 100644 --- a/frontend/src/components/confirm-dialog/confirm-dialog.ts +++ b/frontend/src/components/confirm-dialog/confirm-dialog.ts @@ -81,23 +81,55 @@ export class ConfirmDialog extends LitElement { `, ]; + /** + * Which question is on screen. + * + * This is a singleton reused for every confirmation in the app, + * and `wa-dialog` reports its close *asynchronously* — `open = + * false` starts an animation and `wa-hide` arrives after it. So a + * hide belonging to a question that has already been answered can + * land after the *next* question has opened, and cancel it: the + * user is asked something, the dialog vanishes on its own, and the + * call site is told they said no. + * + * The counter is what tells one question from the next. Every + * close bumps it, and the `wa-hide` handler carries the id its + * template was rendered with. + */ + private askSeq = 0; + /** Ask. Resolves true if the user went ahead. */ ask(request: ConfirmRequest): Promise { this.close(false); + + const id = ++this.askSeq; + this.request = request; return new Promise((resolve) => { this.settle = resolve; void this.updateComplete.then(() => { - if (this.dialog) this.dialog.open = true; + // A third question could have arrived while this one + // was waiting for its own render. + if (this.askSeq === id && this.dialog) this.dialog.open = true; }); }); } - private close(ok: boolean): void { + /** + * Settle the current question, if `id` still names it. + * + * The button handlers pass nothing and always mean the question on + * screen; only `wa-hide` carries an id, because only `wa-hide` can + * arrive late. + */ + private close(ok: boolean, id = this.askSeq): void { + if (id !== this.askSeq) return; + const settle = this.settle; this.settle = null; + this.askSeq++; if (this.dialog) this.dialog.open = false; this.request = null; @@ -118,11 +150,15 @@ export class ConfirmDialog extends LitElement { if (!request) return nothing; + // Captured at render time, so the handler answers the question + // it was drawn for and not whichever one is up when it fires. + const id = this.askSeq; + return html` this.close(false)} + @wa-hide=${() => this.close(false, id)} >

${request.message}

${request.impact diff --git a/frontend/test/components/confirm-dialog.test.ts b/frontend/test/components/confirm-dialog.test.ts index ac3683c..9ab447d 100644 --- a/frontend/test/components/confirm-dialog.test.ts +++ b/frontend/test/components/confirm-dialog.test.ts @@ -80,3 +80,42 @@ describe('confirmAction', () => { await expect(Promise.all([first, second])).resolves.toEqual([false, true]); }); }); + +/** + * A late `wa-hide` must not answer the next question. + * + * This is one singleton for every confirmation in the app, and + * `wa-dialog` reports its close *asynchronously* — `open = false` + * starts an animation and `wa-hide` arrives after it. So a hide + * belonging to a question already answered can land after the next one + * has opened: the user is asked something, the dialog vanishes on its + * own, and the call site is told they said no. + * + * Found by writing two `confirmAction()` tests in one file — the + * second could not be accepted at all, because the first one's hide + * had cancelled it before the click landed. In the app it needs two + * confirmations close together, which "apply these tags" now makes + * reachable. + */ +describe('two questions in a row', () => { + it('does not let the first one answer the second', async () => { + const first = confirmAction({ title: 'First?', message: 'One.' }); + + await press('confirm-cancel'); + await expect(first).resolves.toBe(false); + + const second = confirmAction({ title: 'Second?', message: 'Two.' }); + + // Whatever the first dialog's hide animation is still doing, the + // second question is on screen and unanswered. + await new Promise((r) => setTimeout(r, 0)); + + // The title is a `label` on `wa-dialog` and lands in *its* shadow + // root; the message is the part this component renders. + expect(host().shadowRoot?.textContent ?? '').toContain('Two.'); + + await press('confirm-accept'); + + await expect(second).resolves.toBe(true); + }); +});