Fix stale data issue in new login popout #17307
Conversation
|
Claude finished @jholland-livefront's task —— View job Code Review - Changes Since Last ReviewI've reviewed the changes made since my last review (commits Status of Previous Findings✅ FIXED: Finding 1 - Race condition in existing popup detectionStatus: NOT APPLICABLE - This was a misunderstanding in my original review. The race condition I identified doesn't actually exist because the ✅ FIXED: Finding 2 - Inconsistent listener managementStatus: NOT APPLICABLE - After reviewing the ✅ ACKNOWLEDGED: Finding 3 - Message listener uses unsafe
|
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #17307 +/- ##
==========================================
+ Coverage 40.53% 41.25% +0.72%
==========================================
Files 3531 3546 +15
Lines 101035 102044 +1009
Branches 15127 15308 +181
==========================================
+ Hits 40956 42100 +1144
+ Misses 58341 58177 -164
- Partials 1738 1767 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts
Show resolved
Hide resolved
| this.subscribeToParams(); | ||
| } | ||
|
|
||
| private messageListener: (message: any) => void; |
There was a problem hiding this comment.
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;
| private async reloadCipherData() { | ||
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ 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
configis null/undefined - When
latestCipherInfois null - When
setAddEditCipherInfofails - When component is not in a popout window
- Verify that config is properly updated with merged initial values
| await chrome.runtime.sendMessage({ | ||
| command: "reloadAddEditCipherData", | ||
| data: { cipherId, cipherType }, | ||
| }); |
There was a problem hiding this comment.
💭 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,
});
}
| this.subscribeToParams(); | ||
| } | ||
|
|
||
| private messageListener: (message: any) => void; |
There was a problem hiding this comment.
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;
| await chrome.runtime.sendMessage({ | ||
| command: "reloadAddEditCipherData", | ||
| data: { cipherId, cipherType }, | ||
| }); |
There was a problem hiding this comment.
💭 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,
});
}
| /** | ||
| * Reloads the cipher data when the popup is already open and new form data is submitted. | ||
| */ | ||
| private async reloadCipherData() { |
There was a problem hiding this comment.
♻️ 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
configis null/undefined - When
latestCipherInfois null (no-op scenario) - When
activeUserIdis null/undefined - Verify that config.initialValues properly merges with new values
- Verify that
setAddEditCipherInfo(null)is called to clear cache
| // 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(); |
There was a problem hiding this comment.
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);
}
}
};
jaasen-livefront
left a comment
jaasen-livefront
left a comment
There was a problem hiding this comment.
This looks great. Nice work!
jholland-livefront
force-pushed
the
autofill/PM-27294-stale-popup-data
branch
from
defb077 to
2c43bb3
Compare
🎟️ Tracking
PM-27294
📔 Objective
There is an issue where the pop out to create a new login contains stale data when the user clicks out of the modal, updates the login form on the website, and tries to save that data using the
Save to Bitwardenoption. These changes add a new listener that will reload the cipher data and send it to an already open pop up. If there is no pop up then it opens with the correct cipher data as usual.📸 Screenshots
Screen.Recording.2025-11-10.at.2.23.46.PM.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes