From d52da5869b9782a2ca61d0c4bd0aa6a75cbc2e71 Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Mon, 25 Nov 2024 08:37:01 -0600 Subject: [PATCH] [PM-15065] Vault Loading on empty tabs (#12059) * catch errors from `tabSendMessage` - Removes the need for a timeout when fetching page details * Add parameter to reject on error - allows each implementation to decide if they want to handle the error or not --- .../services/autofill.service.spec.ts | 33 ++++++++++++++++--- .../src/autofill/services/autofill.service.ts | 30 +++++++++++++---- .../src/platform/browser/browser-api.ts | 6 ++-- .../services/vault-popup-autofill.service.ts | 5 +-- 4 files changed, 57 insertions(+), 17 deletions(-) diff --git a/apps/browser/src/autofill/services/autofill.service.spec.ts b/apps/browser/src/autofill/services/autofill.service.spec.ts index 0b036e17a303..19b4135bcfec 100644 --- a/apps/browser/src/autofill/services/autofill.service.spec.ts +++ b/apps/browser/src/autofill/services/autofill.service.spec.ts @@ -143,7 +143,7 @@ describe("AutofillService", () => { }); describe("collectPageDetailsFromTab$", () => { - const tab = mock({ id: 1 }); + const tab = mock({ id: 1, url: "https://www.example.com" }); const messages = new Subject(); function mockCollectPageDetailsResponseMessage( @@ -165,11 +165,16 @@ describe("AutofillService", () => { it("sends a `collectPageDetails` message to the passed tab", () => { autofillService.collectPageDetailsFromTab$(tab); - expect(BrowserApi.tabSendMessage).toHaveBeenCalledWith(tab, { - command: AutofillMessageCommand.collectPageDetails, - sender: AutofillMessageSender.collectPageDetailsFromTabObservable, + expect(BrowserApi.tabSendMessage).toHaveBeenCalledWith( tab, - }); + { + command: AutofillMessageCommand.collectPageDetails, + sender: AutofillMessageSender.collectPageDetailsFromTabObservable, + tab, + }, + null, + true, + ); }); it("builds an array of page details from received `collectPageDetailsResponse` messages", async () => { @@ -218,6 +223,24 @@ describe("AutofillService", () => { expect(tracker.emissions[1]).toBeUndefined(); }); + + it("returns an empty array when the tab.url is empty", async () => { + const tracker = subscribeTo(autofillService.collectPageDetailsFromTab$({ ...tab, url: "" })); + + await tracker.pauseUntilReceived(1); + + expect(tracker.emissions[0]).toEqual([]); + }); + + it("returns an empty array when the `BrowserApi.tabSendMessage` throws an error", async () => { + (BrowserApi.tabSendMessage as jest.Mock).mockRejectedValueOnce(undefined); + + const tracker = subscribeTo(autofillService.collectPageDetailsFromTab$(tab)); + + await tracker.pauseUntilReceived(1); + + expect(tracker.emissions[0]).toEqual([]); + }); }); describe("loadAutofillScriptsOnInstall", () => { diff --git a/apps/browser/src/autofill/services/autofill.service.ts b/apps/browser/src/autofill/services/autofill.service.ts index 24054794b62c..3580a90f7694 100644 --- a/apps/browser/src/autofill/services/autofill.service.ts +++ b/apps/browser/src/autofill/services/autofill.service.ts @@ -1,4 +1,4 @@ -import { filter, firstValueFrom, Observable, scan, startWith } from "rxjs"; +import { filter, firstValueFrom, merge, Observable, ReplaySubject, scan, startWith } from "rxjs"; import { pairwise } from "rxjs/operators"; import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service"; @@ -91,6 +91,9 @@ export default class AutofillService implements AutofillServiceInterface { * @param tab The tab to collect page details from */ collectPageDetailsFromTab$(tab: chrome.tabs.Tab): Observable { + /** Replay Subject that can be utilized when `messages$` may not emit the page details. */ + const pageDetailsFallback$ = new ReplaySubject<[]>(1); + const pageDetailsFromTab$ = this.messageListener .messages$(COLLECT_PAGE_DETAILS_RESPONSE_COMMAND) .pipe( @@ -112,13 +115,28 @@ export default class AutofillService implements AutofillServiceInterface { ), ); - void BrowserApi.tabSendMessage(tab, { - tab: tab, - command: AutofillMessageCommand.collectPageDetails, - sender: AutofillMessageSender.collectPageDetailsFromTabObservable, + void BrowserApi.tabSendMessage( + tab, + { + tab: tab, + command: AutofillMessageCommand.collectPageDetails, + sender: AutofillMessageSender.collectPageDetailsFromTabObservable, + }, + null, + true, + ).catch(() => { + // When `tabSendMessage` throws an error the `pageDetailsFromTab$` will not emit, + // fallback to an empty array + pageDetailsFallback$.next([]); }); - return pageDetailsFromTab$; + // Empty/New tabs do not have a URL. + // In Safari, `tabSendMessage` doesn't throw an error for this case. Fallback to the empty array to handle. + if (!tab.url) { + pageDetailsFallback$.next([]); + } + + return merge(pageDetailsFromTab$, pageDetailsFallback$); } /** diff --git a/apps/browser/src/platform/browser/browser-api.ts b/apps/browser/src/platform/browser/browser-api.ts index 072ef74004f3..850e14302d4d 100644 --- a/apps/browser/src/platform/browser/browser-api.ts +++ b/apps/browser/src/platform/browser/browser-api.ts @@ -200,15 +200,17 @@ export class BrowserApi { tab: chrome.tabs.Tab, obj: T, options: chrome.tabs.MessageSendOptions = null, + rejectOnError = false, ): Promise { if (!tab || !tab.id) { return; } - return new Promise((resolve) => { + return new Promise((resolve, reject) => { chrome.tabs.sendMessage(tab.id, obj, options, (response) => { - if (chrome.runtime.lastError) { + if (chrome.runtime.lastError && rejectOnError) { // Some error happened + reject(); } resolve(response); }); diff --git a/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts b/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts index 0f76c2f2cdbe..7620bd4950b0 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts @@ -10,7 +10,6 @@ import { startWith, Subject, switchMap, - timeout, } from "rxjs"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -75,9 +74,7 @@ export class VaultPopupAutofillService { if (!tab) { return of([]); } - return this.autofillService - .collectPageDetailsFromTab$(tab) - .pipe(timeout({ first: 1500, with: () => of([]) })); + return this.autofillService.collectPageDetailsFromTab$(tab); }), shareReplay({ refCount: false, bufferSize: 1 }), );