PM-22836 Import popout crashes unexpectedly #16928
Conversation
• added popout dialog for file imports inspired by file type send popout dialog • TODO: fix bug whereby a user may dismiss popout dialog and attempt to select a file outside of popout in Send and Import
harr1424
changed the title
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16928 +/- ##
==========================================
- Coverage 40.70% 40.68% -0.03%
==========================================
Files 3541 3543 +2
Lines 101383 101419 +36
Branches 15181 15183 +2
==========================================
- Hits 41271 41264 -7
- Misses 58369 58412 +43
Partials 1743 1743 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…en-after-closing-extension-pop-up-and-selecting-choose-file
|
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
question: Why is this a separate component rather than just living inside the import-browser component? It seems quite weird to have a child component popup a dialog that redirects the parent page.
| component: SendAddEditV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, |
There was a problem hiding this comment.
question: Why are we not saving the url here?
There was a problem hiding this comment.
Copying from Claude's comment (below) which correctly notes this logic wasn't applied to the import path:
Without this flag, the import page URL is saved in popup history
If a user navigates away and the popup restores this URL, they could still access the file selector outside the popout
This defeats the purpose of the Firefox crash fix
…en-after-closing-extension-pop-up-and-selecting-choose-file
|
Claude finished @harr1424's task —— View job Code Review CompleteGood work addressing the However, several issues remain from previous reviews and new concerns were identified: FindingsFinding 1: ❌ Wrong i18n key creates confusing dialog title apps/browser/src/tools/popup/settings/import/import-file-popout-dialog.component.html:7 uses Required: Add dedicated import title key to messages.json and use it: "importPopoutDialogTitle": {
"message": "Pop out to import"
}Finding 2: ❌ Zero test coverage for new components Codecov reports 0% coverage for all new files. No unit tests exist for:
At minimum, test that the dialog opens conditionally based on Finding 3: apps/browser/src/tools/popup/settings/import/import-file-popout-dialog-container.component.ts:11 and import-file-popout-dialog.component.ts:10 are not standalone components. Per project modernization efforts, new Angular components should include Finding 4: Multiple files (import-file-popout-dialog-container.component.ts:20-23, import-file-popout-dialog.component.ts:17-20, send-file-popout-dialog.component.ts:18-21) use constructor injection. Per Angular modernization patterns, use export class ImportFilePopoutDialogComponent {
private dialogService = inject(DialogService);
private popupRouterCacheService = inject(PopupRouterCacheService);
// ... methods
}Finding 5: Methods in import-file-popout-dialog.component.ts:22 and :26, send-file-popout-dialog.component.ts:23 and :27 lack explicit return types. Add Finding 6: apps/browser/src/tools/popup/settings/import/import-browser-v2.component.html:15 has Finding 7: ♻️ Inconsistent navigation patterns create technical debt Import dialog uses Consider standardizing on one approach or extracting shared navigation logic to Finding 8: 💭 Component architecture rationale unclear Why is Alternative: Move dialog-opening logic directly to parent component or implement as a route guard since it checks conditions on route entry. Finding 9: 💭 Unrelated bootstrap configuration changes apps/browser/src/popup/main.ts:23-31 adds explicit bootstrap configuration and error handling. While not harmful, this appears unrelated to the Firefox file selector crash fix. Was this intentional or debugging code that should be removed? Summary: Core fix (doNotSaveUrl + popout dialog) is sound, but code quality issues and missing tests need attention before merge. Most critical: Fix the i18n key and add basic test coverage. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async close() { | ||
| this.dialogService.closeAll(); | ||
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.popupRouterCacheService.back(); |
There was a problem hiding this comment.
The popupRouterCacheService.back() method will navigate to the root vault page if history is empty. This could be confusing UX if the user arrived at the import page through a direct link or bookmark rather than navigation.
Consider:
- Explicitly navigating to a known safe route (e.g.,
/vault-settings) instead of relying on back() - Or add error handling if back() returns false or fails
Context from PopupRouterCacheService
async back() {
// ...
if (this.hasNavigated && history.length) {
this.location.back();
return;
}
// if no history is present, fallback to vault page
await this.router.navigate([""]);
}This fallback behavior may not match user expectations when canceling an import.
|
|
||
| ngOnInit() { | ||
| if (this.filePopoutUtilsService.showFilePopoutMessage(window)) { | ||
| this.dialogService.open(ImportFilePopoutDialogComponent); |
There was a problem hiding this comment.
❌ Finding 2: Dialog opened in ngOnInit without cleanup handling
Opening a dialog in ngOnInit without storing a reference prevents proper cleanup and can cause issues:
- Memory leak: If component is destroyed while dialog is open, no cleanup occurs
- Multiple dialogs: If component is re-initialized, multiple dialogs could open
- No unsubscribe: Dialog subscription isn't tracked for cleanup
Recommended fix
import { Component, OnDestroy, OnInit } from "@angular/core";
import { DialogRef } from "@bitwarden/components";
export class ImportFilePopoutDialogContainerComponent implements OnInit, OnDestroy {
private dialogRef: DialogRef | null = null;
ngOnInit() {
if (this.filePopoutUtilsService.showFilePopoutMessage(window)) {
this.dialogRef = this.dialogService.open(ImportFilePopoutDialogComponent);
}
}
ngOnDestroy() {
this.dialogRef?.close();
}
}
| async close() { | ||
| this.dialogService.closeAll(); | ||
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.router.navigate(["/tabs/send"]); |
There was a problem hiding this comment.
♻️ Finding 3: Inconsistent navigation patterns create technical debt
The Send dialog uses Router.navigate() to an absolute path while the Import dialog uses popupRouterCacheService.back(). This inconsistency creates confusion and maintenance burden.
Issue: Both components solve the same problem (prevent file selector outside popout) but use different navigation strategies.
Recommendation: Standardize on one approach:
- Option A: Both use explicit navigation to a known route
- Option B: Both use the router cache service (requires refactoring Send)
- Option C: Extract shared navigation logic to FilePopoutUtilsService
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
💭 Finding 4: Component placement and responsibilities need clarification
The <import-file-popout-dialog-container> is embedded in the template but has no visual representation (empty template). This creates unclear component boundaries.
Questions for consideration:
- Why is this a separate component rather than logic in the parent
ImportBrowserV2Component? - The container component opens a dialog that navigates the parent - this violates typical component encapsulation
- Should this logic live in a route guard instead, since it's checking conditions on route entry?
Alternative approaches:
- Move the dialog-opening logic to
ImportBrowserV2Component.ngOnInit() - Or use a route guard that opens the dialog before route activation
- Or keep as child component but have it emit events rather than directly navigating
This addresses the reviewer's question about architecture.
| component: SendAddEditV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, |
There was a problem hiding this comment.
💭 Finding 5: Missing route configuration for import path
The add-send route has doNotSaveUrl: true to prevent saving the URL when the popout dialog might be shown. However, there's no corresponding route configuration change for the import page.
Questions:
- What is the route path for
ImportBrowserV2Component? (Not visible in the diff) - Does that route also need
doNotSaveUrl: truefor consistency? - Without
doNotSaveUrl, users who close the popout dialog will have the import page saved in history, which could lead to the file selector issue recurring
This addresses the reviewer's question about why we're not saving the URL here.
| </div> | ||
| <ng-container bitDialogContent> | ||
| <div bitTypography="h3"> | ||
| {{ "sendFilePopoutDialogText" | i18n }} |
There was a problem hiding this comment.
Line 7 uses "sendFilePopoutDialogText" (Send-related) while line 9 correctly uses "importPopoutDialogDesc" (Import-related). This creates a confusing message that says "Send file" in the title but talks about importing in the description.
Expected behavior: The title should use an import-specific i18n key.
Fix needed:
<div bitTypography="h3">
{{ "importPopoutDialogTitle" | i18n }}
</div>Then add to messages.json:
"importPopoutDialogTitle": {
"message": "Pop out required"
}
| "message": "This setting is disabled by your organization's policy.", | ||
| "description": "This hint text is displayed when a user setting is disabled due to an organization policy." | ||
| }, | ||
| "importPopoutDialogDesc": { |
There was a problem hiding this comment.
🎨 Finding 7: Missing i18n key for dialog title
Only the description key was added, but the template reuses the Send dialog's title (sendFilePopoutDialogText). This should have a dedicated import title key.
Suggested addition:
"importPopoutDialogTitle": {
"message": "Pop out required"
},
"importPopoutDialogDesc": {
"message": "To import from a file, you need to pop out the extension to a new window."
}
| component: SendAddEditV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, |
There was a problem hiding this comment.
❌ Finding 8: Import route missing doNotSaveUrl: true flag
The import route (line 246-249) should also have data: { elevation: 1, doNotSaveUrl: true } for consistency with the Send route change.
Why this matters:
- Without this flag, the import page URL is saved in popup history
- If a user navigates away and the popup restores this URL, they could still access the file selector outside the popout
- This defeats the purpose of the Firefox crash fix
Required change:
{
path: "import",
component: ImportBrowserV2Component,
canActivate: [authGuard],
data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties,
}
| </div> | ||
| <ng-container bitDialogContent> | ||
| <div bitTypography="h3"> | ||
| {{ "sendFilePopoutDialogText" | i18n }} |
There was a problem hiding this comment.
❌ Finding 1: Incorrect i18n key creates confusing dialog title
This uses sendFilePopoutDialogText which displays "Pop out extension?" with a description about Send. However, line 9 correctly uses importPopoutDialogDesc for the description, creating inconsistent messaging.
Required fix:
| {{ "sendFilePopoutDialogText" | i18n }} | |
| {{ "importPopoutDialogTitle" | i18n }} |
Then add to messages.json:
"importPopoutDialogTitle": {
"message": "Pop out to import"
}
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
💭 Finding 2: Component architecture violates encapsulation principles
This container component has no visual output (empty template) but opens a dialog that navigates the parent page, creating unclear boundaries.
Questions:
- Why is this a separate component rather than logic in
ImportBrowserV2Component.ngOnInit()? - Should this be a route guard instead since it checks conditions on route entry?
Alternative: Move dialog logic directly to parent component or implement as canActivate guard.
| async close() { | ||
| this.dialogService.closeAll(); | ||
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.popupRouterCacheService.back(); |
There was a problem hiding this comment.
♻️ Finding 3: Inconsistent navigation patterns create technical debt
Import uses popupRouterCacheService.back() while Send uses Router.navigate(["/tabs/send"]) (send-file-popout-dialog.component.ts:30). Both solve the same problem with different approaches.
Recommendation: Standardize navigation strategy across both features or extract shared logic to FilePopoutUtilsService.
| private dialogService: DialogService, | ||
| private router: Router, | ||
| ) {} | ||
|
|
| } | ||
|
|
||
| close() { | ||
| async close() { |
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
| selector: "import-file-popout-dialog", | ||
| templateUrl: "./import-file-popout-dialog.component.html", | ||
| imports: [JslibModule, CommonModule, DialogModule, ButtonModule, TypographyModule], | ||
| }) |
…en-after-closing-extension-pop-up-and-selecting-choose-file
| </div> | ||
| <ng-container bitDialogContent> | ||
| <div bitTypography="h3"> | ||
| {{ "sendFilePopoutDialogText" | i18n }} |
There was a problem hiding this comment.
❌ Finding 1: Wrong i18n key creates confusing dialog title
This uses sendFilePopoutDialogText which displays "Pop out extension?" (a Send-specific message), but line 9 correctly uses importPopoutDialogDesc for the description. This creates inconsistent messaging.
Required fix:
| {{ "sendFilePopoutDialogText" | i18n }} | |
| {{ "importPopoutDialogTitle" | i18n }} |
Then add to messages.json:
"importPopoutDialogTitle": {
"message": "Pop out to import"
}
|
|
||
| import { ImportFilePopoutDialogComponent } from "./import-file-popout-dialog.component"; | ||
|
|
||
| @Component({ |
There was a problem hiding this comment.
New Angular components should include standalone: true per project modernization efforts.
| @Component({ | |
| @Component({ | |
| standalone: true, | |
| changeDetection: ChangeDetectionStrategy.OnPush, |
| export class ImportFilePopoutDialogContainerComponent implements OnInit, OnDestroy { | ||
| private dialogRef: DialogRef | null = null; | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
Per Angular modernization patterns, prefer inject():
| constructor( | |
| private dialogService = inject(DialogService); | |
| private filePopoutUtilsService = inject(FilePopoutUtilsService); |
| import BrowserPopupUtils from "../../../../platform/browser/browser-popup-utils"; | ||
| import { PopupRouterCacheService } from "../../../../platform/popup/view-cache/popup-router-cache.service"; | ||
|
|
||
| @Component({ |
There was a problem hiding this comment.
| @Component({ | |
| @Component({ | |
| standalone: true, | |
| changeDetection: ChangeDetectionStrategy.OnPush, |
| imports: [JslibModule, CommonModule, DialogModule, ButtonModule, TypographyModule], | ||
| }) | ||
| export class ImportFilePopoutDialogComponent { | ||
| constructor( |
There was a problem hiding this comment.
| constructor( | |
| private dialogService = inject(DialogService); | |
| private popupRouterCacheService = inject(PopupRouterCacheService); |
| private popupRouterCacheService: PopupRouterCacheService, | ||
| ) {} | ||
|
|
||
| async popOutWindow() { |
There was a problem hiding this comment.
| async popOutWindow() { | |
| async popOutWindow(): Promise<void> { |
| await BrowserPopupUtils.openCurrentPagePopout(window); | ||
| } | ||
|
|
||
| async close() { |
There was a problem hiding this comment.
| async close() { | |
| async close(): Promise<void> { |
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
Self-closing components don't need closing tags in Angular.
| <import-file-popout-dialog-container></import-file-popout-dialog-container> | |
| <import-file-popout-dialog-container /> |
| }) | ||
| export class SendFilePopoutDialogComponent { | ||
| constructor(private dialogService: DialogService) {} | ||
| constructor( |
There was a problem hiding this comment.
| constructor( | |
| private dialogService = inject(DialogService); | |
| private router = inject(Router); |
| private router: Router, | ||
| ) {} | ||
|
|
||
| async popOutWindow() { |
There was a problem hiding this comment.
| async popOutWindow() { | |
| async popOutWindow(): Promise<void> { |
| } | ||
|
|
||
| close() { | ||
| async close() { |
There was a problem hiding this comment.
| async close() { | |
| async close(): Promise<void> { |
|
|
||
| function init() { | ||
| void platformBrowserDynamic().bootstrapModule(AppModule); | ||
| platformBrowserDynamic() |
There was a problem hiding this comment.
💭 Finding 9: Unrelated bootstrap configuration changes
This explicit bootstrap configuration and error handling appears unrelated to the Firefox file selector crash fix. Was this intentional or debugging code that should be removed?
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22836?atlOrigin=eyJpIjoiNTUwZTI2NmZiMzRhNDIzNTlmMTAzMGJkZWZjZDhjOGMiLCJwIjoiaiJ9
📔 Objective
• Determine reason why extension crashes and fix this
Fix:
• Firefox has a known issue that is relevant here
• To avoid the crash it is necessary to only open modal dialogs (file selectors) from within a popout window
• The extension UI involving file type Sends already implemented this logic (mostly)
• File type Send logic was extended to the Import UI
• Both Send and Import UI logic were extended such that a user can't close the popout and still open a file selector in the original extension panel (file selector will NEVER show outside of popout)
📸 Screenshots
⏰ 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