[PM-5950] Fix Context Menu Update Race Condition and Refactor Implementation (#7724)

* [PM-5949] Refactor typing information for notification bar

* [PM-5949] Fix jest tests for overlay background

* [PM-5949] Removing unnused typing data

* [PM-5949] Fixing lint error

* [PM-5949] Adding jest tests for convertAddLoginQueueMessageToCipherView method

* [PM-5949] Fixing jest test for overlay

* [PM-5950] Fix Context Menu Update Race Condition and Refactor Implementation

* [PM-5950] Adding jest test for cipherContextMenu.update method

* [PM-5950] Adding documentation for method within MainContextMenuHandler

* [PM-5950] Adding jest tests for the mainContextMenuHandler

* [PM-5950] Removing unnecessary return value from MainContextMenuHandler.create method

* [PM-5950] Fixing unawaited context menu promise

apps/browser/src/autofill/browser/abstractions/main-context-menu-handler.ts

@@ -0,0 +1,5 @@
+type InitContextMenuItems = Omit<chrome.contextMenus.CreateProperties, "contexts"> & {
+  checkPremiumAccess?: boolean;
+};
+
+export { InitContextMenuItems };

apps/browser/src/autofill/browser/cipher-context-menu-handler.spec.ts

@@ -18,6 +18,7 @@ describe("CipherContextMenuHandler", () => {
 
   beforeEach(() => {
     mainContextMenuHandler = mock();
+    mainContextMenuHandler.initRunning = false;
     authService = mock();
     cipherService = mock();
 
@@ -29,6 +30,14 @@ describe("CipherContextMenuHandler", () => {
   afterEach(() => jest.resetAllMocks());
 
   describe("update", () => {
+    it("skips updating if the init process for the mainContextMenuHandler is running", async () => {
+      mainContextMenuHandler.initRunning = true;
+
+      await sut.update("https://test.com");
+
+      expect(authService.getAuthStatus).not.toHaveBeenCalled();
+    });
+
     it("locked, updates for no access", async () => {
       authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked);
 

apps/browser/src/autofill/browser/cipher-context-menu-handler.ts

@@ -146,6 +146,10 @@ export class CipherContextMenuHandler {
   }
 
   async update(url: string) {
+    if (this.mainContextMenuHandler.initRunning) {
+      return;
+    }
+
     const authStatus = await this.authService.getAuthStatus();
     await MainContextMenuHandler.removeAll();
     if (authStatus !== AuthenticationStatus.Unlocked) {

apps/browser/src/autofill/browser/main-context-menu-handler.spec.ts

@@ -7,6 +7,7 @@ import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
 import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
 
 import { BrowserStateService } from "../../platform/services/abstractions/browser-state.service";
+import { NOOP_COMMAND_SUFFIX } from "../constants";
 
 import { MainContextMenuHandler } from "./main-context-menu-handler";
 
@@ -39,6 +40,7 @@ describe("context-menu", () => {
       return props.id;
     });
 
+    i18nService.t.mockImplementation((key) => key);
     sut = new MainContextMenuHandler(stateService, i18nService, logService);
   });
 
@@ -136,4 +138,75 @@ describe("context-menu", () => {
       expect(createSpy).toHaveBeenCalledTimes(6);
     });
   });
+
+  describe("creating noAccess context menu items", () => {
+    let loadOptionsSpy: jest.SpyInstance;
+    beforeEach(() => {
+      loadOptionsSpy = jest.spyOn(sut, "loadOptions").mockResolvedValue();
+    });
+
+    it("Loads context menu items that ask the user to unlock their vault if they are authed", async () => {
+      stateService.getIsAuthenticated.mockResolvedValue(true);
+
+      await sut.noAccess();
+
+      expect(loadOptionsSpy).toHaveBeenCalledWith("unlockVaultMenu", NOOP_COMMAND_SUFFIX);
+    });
+
+    it("Loads context menu items that ask the user to login to their vault if they are not authed", async () => {
+      stateService.getIsAuthenticated.mockResolvedValue(false);
+
+      await sut.noAccess();
+
+      expect(loadOptionsSpy).toHaveBeenCalledWith("loginToVaultMenu", NOOP_COMMAND_SUFFIX);
+    });
+  });
+
+  describe("creating noCards context menu items", () => {
+    it("Loads a noCards context menu item and an addCardMenu context item", async () => {
+      const noCardsContextMenuItems = sut["noCardsContextMenuItems"];
+
+      await sut.noCards();
+
+      expect(createSpy).toHaveBeenCalledTimes(3);
+      expect(createSpy).toHaveBeenCalledWith(noCardsContextMenuItems[0], expect.any(Function));
+      expect(createSpy).toHaveBeenCalledWith(noCardsContextMenuItems[1], expect.any(Function));
+      expect(createSpy).toHaveBeenCalledWith(noCardsContextMenuItems[2], expect.any(Function));
+    });
+  });
+
+  describe("creating noIdentities context menu items", () => {
+    it("Loads a noIdentities context menu item and an addIdentityMenu context item", async () => {
+      const noIdentitiesContextMenuItems = sut["noIdentitiesContextMenuItems"];
+
+      await sut.noIdentities();
+
+      expect(createSpy).toHaveBeenCalledTimes(3);
+      expect(createSpy).toHaveBeenCalledWith(noIdentitiesContextMenuItems[0], expect.any(Function));
+      expect(createSpy).toHaveBeenCalledWith(noIdentitiesContextMenuItems[1], expect.any(Function));
+      expect(createSpy).toHaveBeenCalledWith(noIdentitiesContextMenuItems[2], expect.any(Function));
+    });
+  });
+
+  describe("creating noLogins context menu items", () => {
+    it("Loads a noLogins context menu item and an addLoginMenu context item", async () => {
+      const noLoginsContextMenuItems = sut["noLoginsContextMenuItems"];
+
+      await sut.noLogins();
+
+      expect(createSpy).toHaveBeenCalledTimes(5);
+      expect(createSpy).toHaveBeenCalledWith(noLoginsContextMenuItems[0], expect.any(Function));
+      expect(createSpy).toHaveBeenCalledWith(noLoginsContextMenuItems[1], expect.any(Function));
+      expect(createSpy).toHaveBeenCalledWith(
+        {
+          enabled: false,
+          id: "autofill_NOTICE",
+          parentId: "autofill",
+          title: "noMatchingLogins",
+          type: "normal",
+        },
+        expect.any(Function),
+      );
+    });
+  });
 });

apps/browser/src/autofill/browser/main-context-menu-handler.ts

@@ -38,32 +38,127 @@ import {
   SEPARATOR_ID,
 } from "../constants";
 
-export class MainContextMenuHandler {
+import { InitContextMenuItems } from "./abstractions/main-context-menu-handler";
-  private initRunning = false;
 
-  create: (options: chrome.contextMenus.CreateProperties) => Promise<void>;
+export class MainContextMenuHandler {
+  initRunning = false;
+  private initContextMenuItems: InitContextMenuItems[] = [
+    {
+      id: ROOT_ID,
+      title: "Bitwarden",
+    },
+    {
+      id: AUTOFILL_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("autoFillLogin"),
+    },
+    {
+      id: COPY_USERNAME_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("copyUsername"),
+    },
+    {
+      id: COPY_PASSWORD_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("copyPassword"),
+    },
+    {
+      id: COPY_VERIFICATION_CODE_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("copyVerificationCode"),
+      checkPremiumAccess: true,
+    },
+    {
+      id: SEPARATOR_ID + 1,
+      type: "separator",
+      parentId: ROOT_ID,
+    },
+    {
+      id: AUTOFILL_IDENTITY_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("autoFillIdentity"),
+    },
+    {
+      id: AUTOFILL_CARD_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("autoFillCard"),
+    },
+    {
+      id: SEPARATOR_ID + 2,
+      type: "separator",
+      parentId: ROOT_ID,
+    },
+    {
+      id: GENERATE_PASSWORD_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("generatePasswordCopied"),
+    },
+    {
+      id: COPY_IDENTIFIER_ID,
+      parentId: ROOT_ID,
+      title: this.i18nService.t("copyElementIdentifier"),
+    },
+  ];
+  private noCardsContextMenuItems: chrome.contextMenus.CreateProperties[] = [
+    {
+      id: `${AUTOFILL_CARD_ID}_NOTICE`,
+      enabled: false,
+      parentId: AUTOFILL_CARD_ID,
+      title: this.i18nService.t("noCards"),
+      type: "normal",
+    },
+    {
+      id: `${AUTOFILL_CARD_ID}_${SEPARATOR_ID}`,
+      parentId: AUTOFILL_CARD_ID,
+      type: "separator",
+    },
+    {
+      id: `${AUTOFILL_CARD_ID}_${CREATE_CARD_ID}`,
+      parentId: AUTOFILL_CARD_ID,
+      title: this.i18nService.t("addCardMenu"),
+      type: "normal",
+    },
+  ];
+  private noIdentitiesContextMenuItems: chrome.contextMenus.CreateProperties[] = [
+    {
+      id: `${AUTOFILL_IDENTITY_ID}_NOTICE`,
+      enabled: false,
+      parentId: AUTOFILL_IDENTITY_ID,
+      title: this.i18nService.t("noIdentities"),
+      type: "normal",
+    },
+    {
+      id: `${AUTOFILL_IDENTITY_ID}_${SEPARATOR_ID}`,
+      parentId: AUTOFILL_IDENTITY_ID,
+      type: "separator",
+    },
+    {
+      id: `${AUTOFILL_IDENTITY_ID}_${CREATE_IDENTITY_ID}`,
+      parentId: AUTOFILL_IDENTITY_ID,
+      title: this.i18nService.t("addIdentityMenu"),
+      type: "normal",
+    },
+  ];
+  private noLoginsContextMenuItems: chrome.contextMenus.CreateProperties[] = [
+    {
+      id: `${AUTOFILL_ID}_NOTICE`,
+      enabled: false,
+      parentId: AUTOFILL_ID,
+      title: this.i18nService.t("noMatchingLogins"),
+      type: "normal",
+    },
+    {
+      id: `${AUTOFILL_ID}_${SEPARATOR_ID}1`,
+      parentId: AUTOFILL_ID,
+      type: "separator",
+    },
+  ];
 
   constructor(
     private stateService: BrowserStateService,
     private i18nService: I18nService,
     private logService: LogService,
-  ) {
+  ) {}
-    if (chrome.contextMenus) {
-      this.create = (options) => {
-        return new Promise<void>((resolve, reject) => {
-          chrome.contextMenus.create(options, () => {
-            if (chrome.runtime.lastError) {
-              reject(chrome.runtime.lastError);
-              return;
-            }
-            resolve();
-          });
-        });
-      };
-    } else {
-      this.create = (_options) => Promise.resolve();
-    }
-  }
 
   static async mv3Create(cachedServices: CachedServices) {
     const stateFactory = new StateFactory(GlobalState, Account);
@@ -110,76 +205,14 @@ export class MainContextMenuHandler {
     this.initRunning = true;
 
     try {
-      const create = async (options: Omit<chrome.contextMenus.CreateProperties, "contexts">) => {
+      for (const options of this.initContextMenuItems) {
-        await this.create({ ...options, contexts: ["all"] });
+        if (options.checkPremiumAccess && !(await this.stateService.getCanAccessPremium())) {
-      };
+          continue;
+        }
 
-      await create({
+        delete options.checkPremiumAccess;
-        id: ROOT_ID,
+        await MainContextMenuHandler.create({ ...options, contexts: ["all"] });
-        title: "Bitwarden",
-      });
-
-      await create({
-        id: AUTOFILL_ID,
-        parentId: ROOT_ID,
-        title: this.i18nService.t("autoFillLogin"),
-      });
-
-      await create({
-        id: COPY_USERNAME_ID,
-        parentId: ROOT_ID,
-        title: this.i18nService.t("copyUsername"),
-      });
-
-      await create({
-        id: COPY_PASSWORD_ID,
-        parentId: ROOT_ID,
-        title: this.i18nService.t("copyPassword"),
-      });
-
-      if (await this.stateService.getCanAccessPremium()) {
-        await create({
-          id: COPY_VERIFICATION_CODE_ID,
-          parentId: ROOT_ID,
-          title: this.i18nService.t("copyVerificationCode"),
-        });
       }
-
-      await create({
-        id: SEPARATOR_ID + 1,
-        type: "separator",
-        parentId: ROOT_ID,
-      });
-
-      await create({
-        id: AUTOFILL_IDENTITY_ID,
-        parentId: ROOT_ID,
-        title: this.i18nService.t("autoFillIdentity"),
-      });
-
-      await create({
-        id: AUTOFILL_CARD_ID,
-        parentId: ROOT_ID,
-        title: this.i18nService.t("autoFillCard"),
-      });
-
-      await create({
-        id: SEPARATOR_ID + 2,
-        type: "separator",
-        parentId: ROOT_ID,
-      });
-
-      await create({
-        id: GENERATE_PASSWORD_ID,
-        parentId: ROOT_ID,
-        title: this.i18nService.t("generatePasswordCopied"),
-      });
-
-      await create({
-        id: COPY_IDENTIFIER_ID,
-        parentId: ROOT_ID,
-        title: this.i18nService.t("copyElementIdentifier"),
-      });
     } catch (error) {
       this.logService.warning(error.message);
     } finally {
@@ -188,6 +221,26 @@ export class MainContextMenuHandler {
     return true;
   }
 
+  /**
+   * Creates a context menu item
+   *
+   * @param options - the options for the context menu item
+   */
+  private static create = async (options: chrome.contextMenus.CreateProperties) => {
+    if (!chrome.contextMenus) {
+      return;
+    }
+
+    return new Promise<void>((resolve, reject) => {
+      chrome.contextMenus.create(options, () => {
+        if (chrome.runtime.lastError) {
+          return reject(chrome.runtime.lastError);
+        }
+        resolve();
+      });
+    });
+  };
+
   static async removeAll() {
     return new Promise<void>((resolve, reject) => {
       chrome.contextMenus.removeAll(() => {
@@ -221,7 +274,7 @@ export class MainContextMenuHandler {
       const createChildItem = async (parentId: string) => {
         const menuItemId = `${parentId}_${optionId}`;
 
-        return await this.create({
+        return await MainContextMenuHandler.create({
           type: "normal",
           id: menuItemId,
           parentId,
@@ -272,74 +325,42 @@ export class MainContextMenuHandler {
   async noAccess() {
     if (await this.init()) {
       const authed = await this.stateService.getIsAuthenticated();
-      await this.loadOptions(
+      this.loadOptions(
         this.i18nService.t(authed ? "unlockVaultMenu" : "loginToVaultMenu"),
         NOOP_COMMAND_SUFFIX,
-      );
+      ).catch((error) => this.logService.warning(error.message));
     }
   }
 
   async noCards() {
-    await this.create({
+    try {
-      id: `${AUTOFILL_CARD_ID}_NOTICE`,
+      for (const option of this.noCardsContextMenuItems) {
-      enabled: false,
+        await MainContextMenuHandler.create(option);
-      parentId: AUTOFILL_CARD_ID,
+      }
-      title: this.i18nService.t("noCards"),
+    } catch (error) {
-      type: "normal",
+      this.logService.warning(error.message);
-    });
+    }
-
-    await this.create({
-      id: `${AUTOFILL_CARD_ID}_${SEPARATOR_ID}`,
-      parentId: AUTOFILL_CARD_ID,
-      type: "separator",
-    });
-
-    await this.create({
-      id: `${AUTOFILL_CARD_ID}_${CREATE_CARD_ID}`,
-      parentId: AUTOFILL_CARD_ID,
-      title: this.i18nService.t("addCardMenu"),
-      type: "normal",
-    });
   }
 
   async noIdentities() {
-    await this.create({
+    try {
-      id: `${AUTOFILL_IDENTITY_ID}_NOTICE`,
+      for (const option of this.noIdentitiesContextMenuItems) {
-      enabled: false,
+        await MainContextMenuHandler.create(option);
-      parentId: AUTOFILL_IDENTITY_ID,
+      }
-      title: this.i18nService.t("noIdentities"),
+    } catch (error) {
-      type: "normal",
+      this.logService.warning(error.message);
-    });
+    }
-
-    await this.create({
-      id: `${AUTOFILL_IDENTITY_ID}_${SEPARATOR_ID}`,
-      parentId: AUTOFILL_IDENTITY_ID,
-      type: "separator",
-    });
-
-    await this.create({
-      id: `${AUTOFILL_IDENTITY_ID}_${CREATE_IDENTITY_ID}`,
-      parentId: AUTOFILL_IDENTITY_ID,
-      title: this.i18nService.t("addIdentityMenu"),
-      type: "normal",
-    });
   }
 
   async noLogins() {
-    await this.create({
+    try {
-      id: `${AUTOFILL_ID}_NOTICE`,
+      for (const option of this.noLoginsContextMenuItems) {
-      enabled: false,
+        await MainContextMenuHandler.create(option);
-      parentId: AUTOFILL_ID,
+      }
-      title: this.i18nService.t("noMatchingLogins"),
-      type: "normal",
-    });
 
-    await this.create({
+      await this.loadOptions(this.i18nService.t("addLoginMenu"), CREATE_LOGIN_ID);
-      id: `${AUTOFILL_ID}_${SEPARATOR_ID}` + 1,
+    } catch (error) {
-      parentId: AUTOFILL_ID,
+      this.logService.warning(error.message);
-      type: "separator",
+    }
-    });
-
-    await this.loadOptions(this.i18nService.t("addLoginMenu"), CREATE_LOGIN_ID);
   }
 }