From 7150283d7c2d8dcf48c2e8d315ae8651b40765ca Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Thu, 15 Jan 2026 15:24:32 -0600 Subject: [PATCH] [PM-30287] Archive deletion navigation (#18213) * add `routeAfterDeletion` for edit screen to redirect the user to the correct location after deleting an archived cipher * use `historyGo` to preserve the back invocations * fix duplicate import --- .../add-edit/add-edit-v2.component.spec.ts | 104 +++++++++++++++++- .../add-edit/add-edit-v2.component.ts | 46 +++++++- .../vault-v2/view-v2/view-v2.component.ts | 12 +- .../popup/settings/archive.component.spec.ts | 5 + .../vault/popup/settings/archive.component.ts | 13 ++- 5 files changed, 173 insertions(+), 7 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts index fb58f1e2240..8ea23e7e2b9 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts @@ -1,5 +1,7 @@ +import { Location } from "@angular/common"; import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; +import { provideNoopAnimations } from "@angular/platform-browser/animations"; import { ActivatedRoute, Router } from "@angular/router"; import { mock, MockProxy } from "jest-mock-extended"; import { BehaviorSubject, of } from "rxjs"; @@ -59,6 +61,8 @@ describe("AddEditV2Component", () => { const back = jest.fn().mockResolvedValue(null); const setHistory = jest.fn(); const collect = jest.fn().mockResolvedValue(null); + const history$ = jest.fn(); + const historyGo = jest.fn().mockResolvedValue(null); const openSimpleDialog = jest.fn().mockResolvedValue(true); const cipherArchiveService = mock(); @@ -68,6 +72,8 @@ describe("AddEditV2Component", () => { navigate.mockClear(); back.mockClear(); collect.mockClear(); + history$.mockClear(); + historyGo.mockClear(); openSimpleDialog.mockClear(); cipherArchiveService.hasArchiveFlagEnabled$ = of(true); @@ -81,11 +87,13 @@ describe("AddEditV2Component", () => { await TestBed.configureTestingModule({ imports: [AddEditV2Component], providers: [ + provideNoopAnimations(), { provide: PlatformUtilsService, useValue: mock() }, { provide: ConfigService, useValue: mock() }, - { provide: PopupRouterCacheService, useValue: { back, setHistory } }, + { provide: PopupRouterCacheService, useValue: { back, setHistory, history$ } }, { provide: PopupCloseWarningService, useValue: { disable } }, { provide: Router, useValue: { navigate } }, + { provide: Location, useValue: { historyGo } }, { provide: ActivatedRoute, useValue: { queryParams: queryParams$ } }, { provide: I18nService, useValue: { t: (key: string) => key } }, { provide: CipherService, useValue: cipherServiceMock }, @@ -558,12 +566,104 @@ describe("AddEditV2Component", () => { expect(deleteCipherSpy).toHaveBeenCalled(); }); - it("navigates to vault tab after deletion", async () => { + it("navigates to vault tab after deletion by default", async () => { jest.spyOn(component["dialogService"], "openSimpleDialog").mockResolvedValue(true); await component.delete(); expect(navigate).toHaveBeenCalledWith(["/tabs/vault"]); }); + + it("navigates to custom route when not in history", fakeAsync(() => { + buildConfigResponse.originalCipher = { edit: true, id: "123" } as Cipher; + queryParams$.next({ + cipherId: "123", + routeAfterDeletion: "/archive", + }); + + tick(); + + // Mock history without the target route + history$.mockReturnValue( + of([ + { url: "/tabs/vault" }, + { url: "/view-cipher?cipherId=123" }, + { url: "/add-edit?cipherId=123" }, + ]), + ); + + jest.spyOn(component["dialogService"], "openSimpleDialog").mockResolvedValue(true); + + void component.delete(); + tick(); + + expect(history$).toHaveBeenCalled(); + expect(historyGo).not.toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(["/archive"]); + })); + + it("uses historyGo when custom route exists in history", fakeAsync(() => { + buildConfigResponse.originalCipher = { edit: true, id: "123" } as Cipher; + queryParams$.next({ + cipherId: "123", + routeAfterDeletion: "/archive", + }); + + tick(); + + history$.mockReturnValue( + of([ + { url: "/tabs/vault" }, + { url: "/archive" }, + { url: "/view-cipher?cipherId=123" }, + { url: "/add-edit?cipherId=123" }, + ]), + ); + + jest.spyOn(component["dialogService"], "openSimpleDialog").mockResolvedValue(true); + + void component.delete(); + tick(); + + expect(history$).toHaveBeenCalled(); + expect(historyGo).toHaveBeenCalledWith(-2); + expect(navigate).not.toHaveBeenCalled(); + })); + + it("uses router.navigate for default /tabs/vault route", fakeAsync(() => { + buildConfigResponse.originalCipher = { edit: true, id: "456" } as Cipher; + component.routeAfterDeletion = "/tabs/vault"; + + queryParams$.next({ + cipherId: "456", + }); + + tick(); + + jest.spyOn(component["dialogService"], "openSimpleDialog").mockResolvedValue(true); + + void component.delete(); + tick(); + + expect(history$).not.toHaveBeenCalled(); + expect(historyGo).not.toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(["/tabs/vault"]); + })); + + it("ignores invalid routeAfterDeletion query param and uses default route", fakeAsync(() => { + // Reset the component's routeAfterDeletion to default before this test + component.routeAfterDeletion = "/tabs/vault"; + + buildConfigResponse.originalCipher = { edit: true, id: "456" } as Cipher; + queryParams$.next({ + cipherId: "456", + routeAfterDeletion: "/invalid/route", + }); + + tick(); + + // The invalid route should be ignored, routeAfterDeletion should remain default + expect(component.routeAfterDeletion).toBe("/tabs/vault"); + })); }); describe("reloadAddEditCipherData", () => { diff --git a/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts index 8fa17502d42..895a5fe0cce 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts @@ -1,6 +1,6 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { CommonModule } from "@angular/common"; +import { CommonModule, Location } from "@angular/common"; import { Component, OnInit, OnDestroy, viewChild } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { FormsModule } from "@angular/forms"; @@ -64,6 +64,18 @@ import { import { VaultPopoutType } from "../../../utils/vault-popout-window"; import { OpenAttachmentsComponent } from "../attachments/open-attachments/open-attachments.component"; +/** + * Available routes to navigate to after editing a cipher. + * Useful when the user could be coming from a different view other than the main vault (e.g., archive). + */ +export const ROUTES_AFTER_EDIT_DELETION = Object.freeze({ + tabsVault: "/tabs/vault", + archive: "/archive", +} as const); + +export type ROUTES_AFTER_EDIT_DELETION = + (typeof ROUTES_AFTER_EDIT_DELETION)[keyof typeof ROUTES_AFTER_EDIT_DELETION]; + /** * Helper class to parse query parameters for the AddEdit route. */ @@ -79,6 +91,7 @@ class QueryParams { this.username = params.username; this.name = params.name; this.prefillNameAndURIFromTab = params.prefillNameAndURIFromTab; + this.routeAfterDeletion = params.routeAfterDeletion ?? ROUTES_AFTER_EDIT_DELETION.tabsVault; } /** @@ -131,6 +144,12 @@ class QueryParams { * NOTE: This will override the `uri` and `name` query parameters if set to true. */ prefillNameAndURIFromTab?: true; + + /** + * The view that will be navigated to after deleting the cipher. + * @default "/tabs/vault" + */ + routeAfterDeletion?: ROUTES_AFTER_EDIT_DELETION; } export type AddEditQueryParams = Partial>; @@ -168,6 +187,7 @@ export class AddEditV2Component implements OnInit, OnDestroy { headerText: string; config: CipherFormConfig; canDeleteCipher$: Observable; + routeAfterDeletion: ROUTES_AFTER_EDIT_DELETION = "/tabs/vault"; get loading() { return this.config == null; @@ -221,6 +241,7 @@ export class AddEditV2Component implements OnInit, OnDestroy { private dialogService: DialogService, protected cipherAuthorizationService: CipherAuthorizationService, private accountService: AccountService, + private location: Location, private archiveService: CipherArchiveService, private archiveCipherUtilsService: ArchiveCipherUtilitiesService, ) { @@ -407,6 +428,13 @@ export class AddEditV2Component implements OnInit, OnDestroy { ); } + if ( + params.routeAfterDeletion && + Object.values(ROUTES_AFTER_EDIT_DELETION).includes(params.routeAfterDeletion) + ) { + this.routeAfterDeletion = params.routeAfterDeletion; + } + return config; }), ) @@ -514,7 +542,21 @@ export class AddEditV2Component implements OnInit, OnDestroy { return false; } - await this.router.navigate(["/tabs/vault"]); + if (this.routeAfterDeletion !== ROUTES_AFTER_EDIT_DELETION.tabsVault) { + const history = await firstValueFrom(this.popupRouterCacheService.history$()); + const targetIndex = history.map((h) => h.url).lastIndexOf(this.routeAfterDeletion); + + if (targetIndex !== -1) { + const stepsBack = targetIndex - (history.length - 1); + // Use historyGo to navigate back to the target route in history + // This allows downstream calls to `back()` to continue working as expected + await this.location.historyGo(stepsBack); + } else { + await this.router.navigate([this.routeAfterDeletion]); + } + } else { + await this.router.navigate([this.routeAfterDeletion]); + } this.toastService.showToast({ variant: "success", diff --git a/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts index b5c5de032d6..f57b3e2d7f1 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/view-v2/view-v2.component.ts @@ -61,6 +61,7 @@ import { BrowserPremiumUpgradePromptService } from "../../../services/browser-pr import { BrowserViewPasswordHistoryService } from "../../../services/browser-view-password-history.service"; import { VaultPopupScrollPositionService } from "../../../services/vault-popup-scroll-position.service"; import { closeViewVaultItemPopout, VaultPopoutType } from "../../../utils/vault-popout-window"; +import { ROUTES_AFTER_EDIT_DELETION } from "../add-edit/add-edit-v2.component"; import { PopupFooterComponent } from "./../../../../../platform/popup/layout/popup-footer.component"; import { PopupHeaderComponent } from "./../../../../../platform/popup/layout/popup-header.component"; @@ -116,6 +117,7 @@ export class ViewV2Component { collections$: Observable; loadAction: LoadAction; senderTabId?: number; + routeAfterDeletion?: ROUTES_AFTER_EDIT_DELETION; protected showFooter$: Observable; protected userCanArchive$ = this.accountService.activeAccount$ @@ -151,6 +153,9 @@ export class ViewV2Component { switchMap(async (params) => { this.loadAction = params.action; this.senderTabId = params.senderTabId ? parseInt(params.senderTabId, 10) : undefined; + this.routeAfterDeletion = params.routeAfterDeletion + ? params.routeAfterDeletion + : undefined; this.activeUserId = await firstValueFrom( this.accountService.activeAccount$.pipe(getUserId), @@ -230,7 +235,12 @@ export class ViewV2Component { return false; } void this.router.navigate(["/edit-cipher"], { - queryParams: { cipherId: this.cipher.id, type: this.cipher.type, isNew: false }, + queryParams: { + cipherId: this.cipher.id, + type: this.cipher.type, + isNew: false, + routeAfterDeletion: this.routeAfterDeletion, + }, }); return true; } diff --git a/apps/browser/src/vault/popup/settings/archive.component.spec.ts b/apps/browser/src/vault/popup/settings/archive.component.spec.ts index 6ad5c2c2907..2f5cfb8d824 100644 --- a/apps/browser/src/vault/popup/settings/archive.component.spec.ts +++ b/apps/browser/src/vault/popup/settings/archive.component.spec.ts @@ -18,6 +18,11 @@ import { PasswordRepromptService } from "@bitwarden/vault"; import { ArchiveComponent } from "./archive.component"; +// 'qrcode-parser' is used by `BrowserTotpCaptureService` but is an es6 module that jest can't compile. +// Mock the entire module here to prevent jest from throwing an error. I wasn't able to find a way to mock the +// `BrowserTotpCaptureService` where jest would not load the file in the first place. +jest.mock("qrcode-parser", () => {}); + describe("ArchiveComponent", () => { let component: ArchiveComponent; diff --git a/apps/browser/src/vault/popup/settings/archive.component.ts b/apps/browser/src/vault/popup/settings/archive.component.ts index 2a46ac0c46e..ecf091a7322 100644 --- a/apps/browser/src/vault/popup/settings/archive.component.ts +++ b/apps/browser/src/vault/popup/settings/archive.component.ts @@ -39,6 +39,7 @@ import { import { PopOutComponent } from "../../../platform/popup/components/pop-out.component"; import { PopupHeaderComponent } from "../../../platform/popup/layout/popup-header.component"; import { PopupPageComponent } from "../../../platform/popup/layout/popup-page.component"; +import { ROUTES_AFTER_EDIT_DELETION } from "../components/vault-v2/add-edit/add-edit-v2.component"; // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @@ -120,7 +121,11 @@ export class ArchiveComponent { } await this.router.navigate(["/view-cipher"], { - queryParams: { cipherId: cipher.id, type: cipher.type }, + queryParams: { + cipherId: cipher.id, + type: cipher.type, + routeAfterDeletion: ROUTES_AFTER_EDIT_DELETION.archive, + }, }); } @@ -130,7 +135,11 @@ export class ArchiveComponent { } await this.router.navigate(["/edit-cipher"], { - queryParams: { cipherId: cipher.id, type: cipher.type }, + queryParams: { + cipherId: cipher.id, + type: cipher.type, + routeAfterDeletion: ROUTES_AFTER_EDIT_DELETION.archive, + }, }); }