From cac9b71709eb5a02401634a0bbe182868ab7fa24 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 07:32:24 +0000 Subject: [PATCH] fix: re-enable Confirm Delete after a delete, so a second one needs no reopen (closes #335) The password route disabled its Confirm Delete button before the decrypt and never re-enabled it on success, so a second delete in the same popup session found a dead button until the popup was closed and reopened. The lost-password route re-enabled its own button in its leave hook, so the two screens on the one screen behaved differently. Both routes now reset the button through the shared finishDelete(), the one path they both take, and the lost-password leave hook no longer handles it separately. Tests drive a password-route delete and a second delete in the same session; they fail against the prior head, where the button stays disabled after the first delete. Model: opus-4-8 --- TODO.md | 11 ++++ src/popup/views/deleteWallet.js | 21 +++++--- tests/deleteWalletLostPassword.test.js | 70 ++++++++++++++++++++++++-- 3 files changed, 90 insertions(+), 12 deletions(-) diff --git a/TODO.md b/TODO.md index e53423b..8df4a10 100644 --- a/TODO.md +++ b/TODO.md @@ -132,6 +132,17 @@ but the review is broader than any of them. constant rather than `isDebug()`, so it survives only in a debug build; a testnet or the runtime debug toggle still raises the banner but without the view id. + +- 2026-09-21: The Confirm Delete button on the delete-wallet screen no longer + stays dead after a successful delete + ([#335](https://git.eeqj.de/sneak/AutistMask/issues/335)). The password route + disabled the button before the decrypt and never re-enabled it, so a second + delete in the same popup session needed a reopen; the lost-password route + re-enabled its own button in its leave hook, so the two screens behaved + differently. Both now reset through the shared `finishDelete()`, the one path + both routes take, so they behave the same and the button is live for the next + delete. + - 2026-08-30: An address no longer wraps, or is shortened to fit, in any of the common views ([#380](https://git.eeqj.de/sneak/AutistMask/issues/380)). The wallet list was the reported case: the address shared one row with the diff --git a/src/popup/views/deleteWallet.js b/src/popup/views/deleteWallet.js index e4c6609..896d7fe 100644 --- a/src/popup/views/deleteWallet.js +++ b/src/popup/views/deleteWallet.js @@ -51,16 +51,12 @@ function clear() { // The lost-password screen holds no secret — a wallet name is not one — // but it is wiped on leave for the neighbouring reason: a typed // confirmation left standing in a hidden view is one click away from -// destroying a wallet the user has since navigated off. The button is -// re-enabled here too, so a screen left mid-delete is usable on re-entry. +// destroying a wallet the user has since navigated off. function clearLostPassword() { lostPasswordIndex = null; $("delete-wallet-lost-name-input").value = ""; $("delete-wallet-lost-flash").textContent = ""; $("delete-wallet-lost-flash").style.visibility = "hidden"; - const btn = $("btn-delete-wallet-lost-confirm"); - btn.disabled = false; - btn.classList.remove("text-muted"); } function show(walletIdx) { @@ -98,6 +94,17 @@ function showLostPassword() { // cleanup and the accountsChanged broadcast cannot drift apart between // them. async function finishDelete(walletIdx) { + // Each route's confirm button was disabled by its own click handler + // before the delete ran. Re-enable both here, on the one path they + // share, so the two routes reset the same way and a second delete in + // the same popup session finds a live button instead of a dead one. + const passwordBtn = $("btn-delete-wallet-confirm"); + passwordBtn.disabled = false; + passwordBtn.classList.remove("text-muted"); + const lostPasswordBtn = $("btn-delete-wallet-lost-confirm"); + lostPasswordBtn.disabled = false; + lostPasswordBtn.classList.remove("text-muted"); + const { activeAddressChanged } = removeWalletFromState(state, walletIdx); deleteWalletIndex = null; @@ -187,8 +194,8 @@ function init(_ctx) { btn.disabled = true; btn.classList.add("text-muted"); - // finishDelete() navigates, and the leave hook re-enables the - // button and wipes the typed name on the way out. + // finishDelete() re-enables the button; navigating away then runs + // the leave hook that wipes the typed name. await finishDelete(lostPasswordIndex); }); diff --git a/tests/deleteWalletLostPassword.test.js b/tests/deleteWalletLostPassword.test.js index 88cc05f..6f2915a 100644 --- a/tests/deleteWalletLostPassword.test.js +++ b/tests/deleteWalletLostPassword.test.js @@ -172,6 +172,15 @@ async function openLostPassword(deleteWallet, walletIdx) { await click("btn-delete-wallet-lost-password"); } +// Delete a wallet through the password route: open its confirm screen, +// enter the password, and confirm. The vault is mocked, so the password +// text itself is irrelevant — decryptWithPassword decides pass or fail. +async function deleteWithPassword(deleteWallet, walletIdx) { + deleteWallet.show(walletIdx); + node("delete-wallet-password").value = "any password"; + await click("btn-delete-wallet-confirm"); +} + // ------------------------------------------------------------ tests // The stub is what every persistence assertion below rests on, so its one @@ -456,15 +465,21 @@ describe("what the screen leaves behind", () => { ); }); - // Left mid-delete, the screen has to come back usable. - test("the confirm button is re-enabled on the way out", async () => { - const { helpers, deleteWallet } = load(); + // Both routes now re-enable through finishDelete(), not their leave + // hooks, so the button comes back live once a delete completes. + test("the confirm button is re-enabled after a delete", async () => { + const { deleteWallet } = load(); await openLostPassword(deleteWallet, 1); - node("btn-delete-wallet-lost-confirm").disabled = true; - helpers.showView("settings"); + node("delete-wallet-lost-name-input").value = "Wallet 2"; + await click("btn-delete-wallet-lost-confirm"); expect(node("btn-delete-wallet-lost-confirm").disabled).toBe(false); + expect( + node("btn-delete-wallet-lost-confirm").classList.contains( + "text-muted", + ), + ).toBe(false); }); // A wallet name is not a secret, so the screen is excluded for the @@ -475,3 +490,48 @@ describe("what the screen leaves behind", () => { expect(RESTORABLE_VIEWS.has("delete-wallet-confirm")).toBe(false); }); }); + +// The password route is the pre-existing bug this file's fix addresses: +// its Confirm Delete button was disabled before the decrypt and never +// re-enabled on success, so a second delete in the same popup session +// found a dead button. Now both routes re-enable through finishDelete(). +// +// Against head these tests fail: with the re-enable absent, the button +// stays disabled after the first delete, so the disabled assertions read +// true where they expect false. +describe("the password route's confirm button", () => { + test("is re-enabled after a successful delete", async () => { + const { deleteWallet, vault } = load(); + vault.decryptWithPassword.mockResolvedValue(); + + await deleteWithPassword(deleteWallet, 1); + + expect(node("btn-delete-wallet-confirm").disabled).toBe(false); + expect( + node("btn-delete-wallet-confirm").classList.contains("text-muted"), + ).toBe(false); + }); + + // The reported symptom: delete one wallet, then open Delete Wallet for + // a second one without reopening the popup. The button must be live on + // that second visit, and the second delete must actually persist. + test("a second delete works in the same popup session", async () => { + const { deleteWallet, vault, storage } = load(); + vault.decryptWithPassword.mockResolvedValue(); + + await deleteWithPassword(deleteWallet, 1); + + // Wallet 2 is gone; the list is now [Wallet 1, Wallet 3]. Opening + // the confirm screen for the wallet now at index 1 (Wallet 3) must + // find its button live, not the dead one the first delete left. + deleteWallet.show(1); + expect(node("btn-delete-wallet-confirm").disabled).toBe(false); + + node("delete-wallet-password").value = "any password"; + await click("btn-delete-wallet-confirm"); + + expect((await persistedWallets(storage)).map((w) => w.name)).toEqual([ + "Wallet 1", + ]); + }); +});