Skip to content

Commit

Permalink
[PM-15065] Vault Loading on empty tabs (#12059)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
nick-livefront authored Nov 25, 2024
1 parent da6a0cb commit d52da58
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 17 deletions.
33 changes: 28 additions & 5 deletions apps/browser/src/autofill/services/autofill.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ describe("AutofillService", () => {
});

describe("collectPageDetailsFromTab$", () => {
const tab = mock<chrome.tabs.Tab>({ id: 1 });
const tab = mock<chrome.tabs.Tab>({ id: 1, url: "https://www.example.com" });
const messages = new Subject<CollectPageDetailsResponseMessage>();

function mockCollectPageDetailsResponseMessage(
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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", () => {
Expand Down
30 changes: 24 additions & 6 deletions apps/browser/src/autofill/services/autofill.service.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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<PageDetail[]> {
/** 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(
Expand All @@ -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$);
}

/**
Expand Down
6 changes: 4 additions & 2 deletions apps/browser/src/platform/browser/browser-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,15 +200,17 @@ export class BrowserApi {
tab: chrome.tabs.Tab,
obj: T,
options: chrome.tabs.MessageSendOptions = null,
rejectOnError = false,
): Promise<TResponse> {
if (!tab || !tab.id) {
return;
}

return new Promise<TResponse>((resolve) => {
return new Promise<TResponse>((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);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
startWith,
Subject,
switchMap,
timeout,
} from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
Expand Down Expand Up @@ -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 }),
);
Expand Down

0 comments on commit d52da58

Please sign in to comment.