From 7ab21290a0688c9e95b39cd3816fbee99be6e862 Mon Sep 17 00:00:00 2001 From: Zaldaryon <273555259+Zaldaryon@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:05:04 -0300 Subject: [PATCH 1/3] fix(backups): prevent duplicate backup deletes --- .../pages/ManageInstallationBackups.tsx | 6 +- .../installationsRestoreBackup.test.tsx | 77 +++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx b/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx index 7f975f66..5b0b440e 100644 --- a/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx +++ b/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx @@ -96,6 +96,7 @@ function ManageInstallationBackups(): JSX.Element { if (!installation) return addNotification(t("features.installations.noInstallationFound"), "error") if (!backup) return addNotification(t("features.backups.cantDeleteWhileinUse"), "error") + configDispatch({ type: CONFIG_ACTIONS.EDIT_INSTALLATION_BACKUP, payload: { id: installation.id, backupId: backup.id, updates: { _deleting: true } } }) const result = await deleteInstallationBackup(createBackupDeletionPorts(), { backup: toBackupSnapshot(backup) }) if (result.ok) { @@ -103,6 +104,7 @@ function ManageInstallationBackups(): JSX.Element { return addNotification(t("features.backups.backupDeletedSuccesfully"), "success") } + configDispatch({ type: CONFIG_ACTIONS.EDIT_INSTALLATION_BACKUP, payload: { id: installation.id, backupId: backup.id, updates: { _deleting: false } } }) const { messageKey, logged } = describeBackupDeletionFailure(result.reason) if (logged) { @@ -153,10 +155,10 @@ function ManageInstallationBackups(): JSX.Element {
- setBackupToRestore(backup)}> + setBackupToRestore(backup)} disabled={backup._deleting || backup._restoring}> - setBackupToDelete(backup)} title={t("generic.delete")} className="p-1"> + setBackupToDelete(backup)} title={t("generic.delete")} className="p-1" disabled={backup._deleting || backup._restoring}> openPathInExplorer(backup.path, { parentOfFile: true })} title={`${t("generic.openOnFileExplorer")} ยท ${backup.path}`} className="p-1"> diff --git a/tests/renderer-dom/installationsRestoreBackup.test.tsx b/tests/renderer-dom/installationsRestoreBackup.test.tsx index 31f2da09..0c0c967b 100644 --- a/tests/renderer-dom/installationsRestoreBackup.test.tsx +++ b/tests/renderer-dom/installationsRestoreBackup.test.tsx @@ -96,4 +96,81 @@ describe("ManageInstallationBackups", () => { // The confirm dialog closes as part of the same click. await waitFor(() => expect(screen.queryByText("Are you sure you want to restore this Backup?")).toBeNull()) }) + + it("deletes the backup archive after the delete confirmation", async () => { + const user = userEvent.setup() + const deletePath = vi.fn(async () => true) + installMockWindowApi({ + configManager: { getConfig: vi.fn(async () => createMockConfig({ installations: [anInstallationWithBackup()] })) }, + pathsManager: { + deletePath, + extractOnPath: vi.fn(async () => true) + } + }) + + renderManageBackups("install-a") + + await user.click(await screen.findByTitle("Delete")) + await screen.findByText("Are you sure you want to delete this Backup?") + await user.click(screen.getAllByTitle("Delete")[1]!) + + await waitFor(() => expect(deletePath).toHaveBeenCalledTimes(1)) + expect(deletePath.mock.calls[0]?.[0]).toBe("/backups/a/backup-1.zip") + await waitFor(() => expect(screen.queryByTitle("Delete")).toBeNull()) + }) + + it("does not start a second deletion while the first one is in flight", async () => { + const user = userEvent.setup() + let resolveDelete: (result: boolean) => void = () => {} + const deletePath = vi.fn( + () => + new Promise((resolve) => { + resolveDelete = resolve + }) + ) + installMockWindowApi({ + configManager: { getConfig: vi.fn(async () => createMockConfig({ installations: [anInstallationWithBackup()] })) }, + pathsManager: { + deletePath, + extractOnPath: vi.fn(async () => true) + } + }) + + renderManageBackups("install-a") + + await user.click(await screen.findByTitle("Delete")) + await screen.findByText("Are you sure you want to delete this Backup?") + await user.click(screen.getAllByTitle("Delete")[1]!) + + await waitFor(() => expect(deletePath).toHaveBeenCalledTimes(1)) + await waitFor(() => expect(screen.queryByTitle("Delete")).toBeNull()) + + resolveDelete(true) + await waitFor(() => expect(screen.queryByText("Are you sure you want to delete this Backup?")).toBeNull()) + }) + + it("clears the deleting state when the archive deletion fails", async () => { + const user = userEvent.setup() + const deletePath = vi.fn().mockResolvedValueOnce(false).mockResolvedValueOnce(true) + installMockWindowApi({ + configManager: { getConfig: vi.fn(async () => createMockConfig({ installations: [anInstallationWithBackup()] })) }, + pathsManager: { + deletePath, + extractOnPath: vi.fn(async () => true) + } + }) + + renderManageBackups("install-a") + + await user.click(await screen.findByTitle("Delete")) + await screen.findByText("Are you sure you want to delete this Backup?") + await user.click(screen.getAllByTitle("Delete")[1]!) + await waitFor(() => expect(deletePath).toHaveBeenCalledTimes(1)) + await waitFor(() => expect(screen.getByTitle("Delete")).toBeTruthy()) + + await user.click(screen.getByTitle("Delete")) + await screen.findByText("Are you sure you want to delete this Backup?") + await user.click(screen.getAllByTitle("Delete")[1]!) + await waitFor(() => expect(deletePath).toHaveBeenCalledTimes(2)) + }) }) From a1d894fafad37df71e95b0913a1f084cb52e641a Mon Sep 17 00:00:00 2001 From: Zaldaryon <273555259+Zaldaryon@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:45:16 -0300 Subject: [PATCH 2/3] fix(backups): guard _deleting clear on backup-in-use, assert deletePath call count The failure path at ManageInstallationBackups:107 cleared _deleting unconditionally. When the domain layer refuses with backup-in-use (because another delete is in flight), the clear re-enabled the button and defeated the per-row guard. Skip the clear when reason is backup-in-use, matching the started-flag pattern RestoreBackupHandler uses above. The middle test asserted a tooltip attribute that NormalButton blanks when disabled (Buttons.tsx:43). Anyone fixing disabled-button tooltips for accessibility would silently break the test without changing behavior. Replace the assertion with a second click on the trash button followed by expect(deletePath).toHaveBeenCalledTimes(1), which pins the property the PR exists to guarantee. Filed #145 for the auto-prune and deleteInstallation bypass routes, and #149 for the dead domain guard at backupDeletion.ts:28. --- .../installations/pages/ManageInstallationBackups.tsx | 4 +++- tests/renderer-dom/installationsRestoreBackup.test.tsx | 7 ++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx b/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx index 5b0b440e..6662bcd8 100644 --- a/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx +++ b/src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx @@ -104,7 +104,9 @@ function ManageInstallationBackups(): JSX.Element { return addNotification(t("features.backups.backupDeletedSuccesfully"), "success") } - configDispatch({ type: CONFIG_ACTIONS.EDIT_INSTALLATION_BACKUP, payload: { id: installation.id, backupId: backup.id, updates: { _deleting: false } } }) + if (result.reason !== "backup-in-use") { + configDispatch({ type: CONFIG_ACTIONS.EDIT_INSTALLATION_BACKUP, payload: { id: installation.id, backupId: backup.id, updates: { _deleting: false } } }) + } const { messageKey, logged } = describeBackupDeletionFailure(result.reason) if (logged) { diff --git a/tests/renderer-dom/installationsRestoreBackup.test.tsx b/tests/renderer-dom/installationsRestoreBackup.test.tsx index 0c0c967b..a7634273 100644 --- a/tests/renderer-dom/installationsRestoreBackup.test.tsx +++ b/tests/renderer-dom/installationsRestoreBackup.test.tsx @@ -143,7 +143,12 @@ describe("ManageInstallationBackups", () => { await user.click(screen.getAllByTitle("Delete")[1]!) await waitFor(() => expect(deletePath).toHaveBeenCalledTimes(1)) - await waitFor(() => expect(screen.queryByTitle("Delete")).toBeNull()) + + const trashButtons = screen.getAllByRole("button").filter((btn) => btn.querySelector("svg")) + const trashButton = trashButtons.find((btn) => btn.hasAttribute("disabled") || btn.getAttribute("title") === "") + if (trashButton) await user.click(trashButton) + + expect(deletePath).toHaveBeenCalledTimes(1) resolveDelete(true) await waitFor(() => expect(screen.queryByText("Are you sure you want to delete this Backup?")).toBeNull()) From 189299dd60420dd80463cbbd0f88cc81b20f58bf Mon Sep 17 00:00:00 2001 From: Zaldaryon <273555259+Zaldaryon@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:28:39 -0300 Subject: [PATCH 3/3] test(backups): rewrite the double-delete guard test to fail pre-fix The previous test was vacuous: it found the trash button conditionally and skipped the click when no match existed, so it passed on dev where the button stays enabled. This version acquires the button unconditionally, confirms the deletion, then asserts that disabled buttons appear while the delete is in flight. That assertion fails on dev (no _deleting dispatch) and passes on this branch. --- .../installationsRestoreBackup.test.tsx | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/tests/renderer-dom/installationsRestoreBackup.test.tsx b/tests/renderer-dom/installationsRestoreBackup.test.tsx index a7634273..27c182dd 100644 --- a/tests/renderer-dom/installationsRestoreBackup.test.tsx +++ b/tests/renderer-dom/installationsRestoreBackup.test.tsx @@ -119,7 +119,7 @@ describe("ManageInstallationBackups", () => { await waitFor(() => expect(screen.queryByTitle("Delete")).toBeNull()) }) - it("does not start a second deletion while the first one is in flight", async () => { + it("disables the row trash button while a deletion is in flight", async () => { const user = userEvent.setup() let resolveDelete: (result: boolean) => void = () => {} const deletePath = vi.fn( @@ -138,20 +138,29 @@ describe("ManageInstallationBackups", () => { renderManageBackups("install-a") - await user.click(await screen.findByTitle("Delete")) + // Acquire the row's trash button. Before deletion it must be enabled. + const trashButton = await screen.findByTitle("Delete") + expect(trashButton.hasAttribute("disabled")).toBe(false) + + // Start the first deletion through the confirm dialog. + await user.click(trashButton) await screen.findByText("Are you sure you want to delete this Backup?") - await user.click(screen.getAllByTitle("Delete")[1]!) + const confirmButtons = screen.getAllByTitle("Delete") + await user.click(confirmButtons[confirmButtons.length - 1]!) await waitFor(() => expect(deletePath).toHaveBeenCalledTimes(1)) - const trashButtons = screen.getAllByRole("button").filter((btn) => btn.querySelector("svg")) - const trashButton = trashButtons.find((btn) => btn.hasAttribute("disabled") || btn.getAttribute("title") === "") - if (trashButton) await user.click(trashButton) - - expect(deletePath).toHaveBeenCalledTimes(1) + // While the deletion is in flight, the row trash button must be disabled. + // This assertion fails on dev (where _deleting is never set) and passes on + // this branch (where configDispatch sets _deleting: true before the call). + await waitFor(() => { + const buttons = screen.getAllByRole("button") + const disabledButtons = buttons.filter((btn) => btn.hasAttribute("disabled")) + expect(disabledButtons.length).toBeGreaterThan(0) + }) resolveDelete(true) - await waitFor(() => expect(screen.queryByText("Are you sure you want to delete this Backup?")).toBeNull()) + await waitFor(() => expect(deletePath).toHaveBeenCalledTimes(1)) }) it("clears the deleting state when the archive deletion fails", async () => {