Skip to content

Fix stale data issue in new login popout #17307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+191 −7
Open

Fix stale data issue in new login popout #17307

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1de8d92
Fix stale data issue in new login popout
jholland-livefront Nov 10, 2025
0357842
Update the comments
jholland-livefront Nov 10, 2025
43b3eb2
Address critical claude code bot suggestions
jholland-livefront Nov 10, 2025
2c43bb3
Clean out all stale data from pop up
jholland-livefront Nov 18, 2025
b109bfa
Fix cached cipher issue
jholland-livefront Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { CommonModule } from "@angular/common";
import { Component, OnInit } from "@angular/core";
import { Component, OnInit, OnDestroy } from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { FormsModule } from "@angular/forms";
import { ActivatedRoute, Params, Router } from "@angular/router";
Expand Down Expand Up @@ -158,7 +158,7 @@ export type AddEditQueryParams = Partial<Record<keyof QueryParams, string>>;
IconButtonModule,
],
})
export class AddEditV2Component implements OnInit {
export class AddEditV2Component implements OnInit, OnDestroy {
headerText: string;
config: CipherFormConfig;
canDeleteCipher$: Observable<boolean>;
Expand Down Expand Up @@ -200,12 +200,56 @@ export class AddEditV2Component implements OnInit {
this.subscribeToParams();
}

private messageListener: (message: any) => void;
Copy link

@claude claude bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Finding 3: Message listener uses unsafe any type

The messageListener parameter is typed as any, which bypasses type safety and could lead to runtime errors if the message structure changes.

Suggested fix

Define a message type interface:

interface ReloadCipherDataMessage {
  command: string;
  data?: {
    cipherId?: string;
    cipherType?: CipherType;
  };
}

private messageListener: (message: ReloadCipherDataMessage) => void;

Copy link

@claude claude bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Finding 3: Message listener uses unsafe any type

The messageListener parameter is typed as any, which bypasses type safety and could lead to runtime errors if the message structure changes.

Suggested fix

Define a message type interface:

interface ReloadCipherDataMessage {
  command: string;
  data?: {
    cipherId?: string;
    cipherType?: CipherType;
  };
}

private messageListener: (message: ReloadCipherDataMessage) => void;


async ngOnInit() {
this.fido2PopoutSessionData = await firstValueFrom(this.fido2PopoutSessionData$);

if (BrowserPopupUtils.inPopout(window)) {
this.popupCloseWarningService.enable();
}

// Listen for messages to reload cipher data when the pop up is already open
this.messageListener = async (message: any) => {
if (message?.command === "reloadAddEditCipherData") {
await this.reloadCipherData();
Copy link

@claude claude bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Finding 6: No error handling for reloadCipherData in message listener

If reloadCipherData() throws an error, it will be silently swallowed since the async function isn't awaited with error handling. This could lead to silent failures when the popup tries to refresh data.

Suggested fix

Add try-catch to handle errors gracefully:

this.messageListener = async (message: any) => {
  if (message?.command === "reloadAddEditCipherData") {
    try {
      await this.reloadCipherData();
    } catch (error) {
      this.logService.error("Failed to reload cipher data", error);
    }
  }
};

}
};
BrowserApi.addListener(chrome.runtime.onMessage, this.messageListener);
}

ngOnDestroy() {
if (this.messageListener) {
chrome.runtime.onMessage.removeListener(this.messageListener);
}
}

/**
* Reloads the cipher data when the popup is already open and new form data is submitted.
*/
private async reloadCipherData() {
Copy link

@claude claude bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Finding 4: Missing comprehensive test coverage for reloadCipherData method

Codecov reports 56.52% patch coverage with 10 lines missing coverage. The reloadCipherData method lacks tests for edge cases and error scenarios.

Recommended test cases:

  • When config is null/undefined
  • When latestCipherInfo is null (no-op scenario)
  • When activeUserId is null/undefined
  • Verify that config.initialValues properly merges with new values
  • Verify that setAddEditCipherInfo(null) is called to clear cache

if (!this.config) {
return;
}

const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));

const latestCipherInfo = await firstValueFrom(
this.cipherService.addEditCipherInfo$(activeUserId),
);

if (latestCipherInfo != null) {
this.config = {
...this.config,
initialValues: {
...this.config.initialValues,
...mapAddEditCipherInfoToInitialValues(latestCipherInfo),
},
};

// Be sure to clear the "cached" cipher info, so it doesn't get used again
await this.cipherService.setAddEditCipherInfo(null, activeUserId);
}
}
Comment on lines 231 to 251
Copy link

@claude claude bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Finding 4: Missing test coverage for reloadCipherData method

Codecov reports 56.52% patch coverage with 10 lines missing coverage. The reloadCipherData method lacks comprehensive test coverage for error scenarios and edge cases.

Recommended test cases:

  • When config is null/undefined
  • When latestCipherInfo is null
  • When setAddEditCipherInfo fails
  • When component is not in a popout window
  • Verify that config is properly updated with merged initial values


/**
Expand Down
40 changes: 40 additions & 0 deletions apps/browser/src/vault/popup/utils/vault-popout-window.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { mock } from "jest-mock-extended";

import { CipherType } from "@bitwarden/common/vault/enums";

import { BrowserApi } from "../../../platform/browser/browser-api";
import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils";

import {
Expand All @@ -23,6 +24,19 @@ describe("VaultPopoutWindow", () => {
.spyOn(BrowserPopupUtils, "closeSingleActionPopout")
.mockImplementation();

beforeEach(() => {
jest.spyOn(BrowserApi, "tabsQuery").mockResolvedValue([]);
jest.spyOn(BrowserApi, "updateWindowProperties").mockResolvedValue();
global.chrome = {
...global.chrome,
runtime: {
...global.chrome?.runtime,
sendMessage: jest.fn().mockResolvedValue(undefined),
getURL: jest.fn((path) => `chrome-extension://extension-id/${path}`),
},
};
});

afterEach(() => {
jest.clearAllMocks();
});
Expand Down Expand Up @@ -123,6 +137,32 @@ describe("VaultPopoutWindow", () => {
},
);
});

it("sends a message to refresh data when the popup is already open", async () => {
const existingPopupTab = {
id: 123,
windowId: 456,
url: `chrome-extension://extension-id/popup/index.html#/edit-cipher?singleActionPopout=${VaultPopoutType.addEditVaultItem}_${CipherType.Login}`,
} as chrome.tabs.Tab;

jest.spyOn(BrowserApi, "tabsQuery").mockResolvedValue([existingPopupTab]);
const sendMessageSpy = jest.spyOn(chrome.runtime, "sendMessage");
const updateWindowSpy = jest.spyOn(BrowserApi, "updateWindowProperties");

await openAddEditVaultItemPopout(
mock<chrome.tabs.Tab>({ windowId: 1, url: "https://jest-testing-website.com" }),
{
cipherType: CipherType.Login,
},
);

expect(openPopoutSpy).not.toHaveBeenCalled();
expect(sendMessageSpy).toHaveBeenCalledWith({
command: "reloadAddEditCipherData",
data: { cipherId: undefined, cipherType: CipherType.Login },
});
expect(updateWindowSpy).toHaveBeenCalledWith(456, { focused: true });
});
});

describe("closeAddEditVaultItemPopout", () => {
Expand Down
24 changes: 20 additions & 4 deletions apps/browser/src/vault/popup/utils/vault-popout-window.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ async function openAddEditVaultItemPopout(
let singleActionKey = VaultPopoutType.addEditVaultItem;
let addEditCipherUrl = "popup/index.html#/edit-cipher";
let queryParamToken = "?";
const extensionUrl = chrome.runtime.getURL("popup/index.html");
const existingPopupTabs = await BrowserApi.tabsQuery({ url: `${extensionUrl}*` });
const existingPopup = existingPopupTabs.find((tab) =>
tab.url?.includes(`singleActionPopout=${singleActionKey}`),
);
const formatQueryString = (key: string, value: string) => {
const queryString = `${queryParamToken}${key}=${value}`;
queryParamToken = "&";
Expand All @@ -115,10 +120,21 @@ async function openAddEditVaultItemPopout(
addEditCipherUrl += formatQueryString("uri", url);
}

await BrowserPopupUtils.openPopout(addEditCipherUrl, {
singleActionKey,
senderWindowId: windowId,
});
// Check if the an existing popup is already open
if (existingPopup) {
await chrome.runtime.sendMessage({
command: "reloadAddEditCipherData",
data: { cipherId, cipherType },
});
Comment on lines +125 to +128
Copy link

@claude claude bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Finding 5: No error handling for message sending

If chrome.runtime.sendMessage fails (e.g., receiver not ready, extension context invalidated), there's no error handling or user feedback. The window will be focused but data won't be refreshed, creating a confusing UX.

Consider adding try-catch and logging:

try {
  await chrome.runtime.sendMessage({
    command: "reloadAddEditCipherData",
    data: { cipherId, cipherType },
  });
  await BrowserApi.updateWindowProperties(existingPopup.windowId, {
    focused: true,
  });
} catch (error) {
  // Log error and fall back to opening a new popout
  console.error("Failed to send reload message, opening new popout", error);
  await BrowserPopupUtils.openPopout(addEditCipherUrl, {
    singleActionKey,
    senderWindowId: windowId,
  });
}

Copy link

@claude claude bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Finding 5: No error handling for message sending

If chrome.runtime.sendMessage fails (e.g., receiver not ready, extension context invalidated), there's no error handling or user feedback. The window will be focused but data won't be refreshed, creating a confusing UX.

Consider adding try-catch and logging:

try {
  await chrome.runtime.sendMessage({
    command: "reloadAddEditCipherData",
    data: { cipherId, cipherType },
  });
  await BrowserApi.updateWindowProperties(existingPopup.windowId, {
    focused: true,
  });
} catch (error) {
  // Log error and fall back to opening a new popout
  console.error("Failed to send reload message, opening new popout", error);
  await BrowserPopupUtils.openPopout(addEditCipherUrl, {
    singleActionKey,
    senderWindowId: windowId,
  });
}

await BrowserApi.updateWindowProperties(existingPopup.windowId, {
focused: true,
});
} else {
await BrowserPopupUtils.openPopout(addEditCipherUrl, {
singleActionKey,
senderWindowId: windowId,
});
}
}

/**
Expand Down
Loading