PM-22836 Import popout crashes unexpectedly #16928
There are no files selected for viewing
|
Member
There was a problem hiding this comment. question: Why are these needed when we show a dialog on the pages when opened in a popup.
Contributor
Author
There was a problem hiding this comment. ImportWith Without SendWith Without Since only the Send functionality requires this, why was it added to Import as well?For consistency, it can be safely removed from the import routing logic if this is preferred.
Member
There was a problem hiding this comment. I think I understand why you've resorted to adding Is this accurate? And if so should we resolve the root cause rather than rely on the race condition? @Component({
selector: "send-file-popout-dialog-container",
templateUrl: "./send-file-popout-dialog-container.component.html",
imports: [JslibModule, CommonModule],
})
export class SendFilePopoutDialogContainerComponent {
private readonly dialogService = inject(DialogService);
private readonly filePopoutUtilsService = inject(FilePopoutUtilsService);
readonly config = input.required<SendFormConfig>();
/**
* Tracks if the dialog has already been opened. This prevents multiple dialogs from opening if config is updated.
*/
private readonly dialogOpened = signal(false);
constructor() {
effect(() => {
if (this.dialogOpened()) {
return;
}
if (
this.config().sendType === SendType.File &&
this.config().mode === "add" &&
this.filePopoutUtilsService.showFilePopoutMessage(window)
) {
this.dialogService.open(SendFilePopoutDialogComponent, {
positionStrategy: new CenterPositionStrategy(),
});
this.dialogOpened.set(true);
}
});
}
} |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,8 @@ | |||||
| > | ||||||
| </tools-import> | ||||||
|
|
||||||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> | ||||||
|
Member
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. There was a problem hiding this comment. 💭 Finding 4: Component placement and responsibilities need clarification The Questions for consideration:
Alternative approaches:
This addresses the reviewer's question about architecture. 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:
Alternative: Move dialog logic directly to parent component or implement as canActivate guard. There was a problem hiding this comment. Self-closing components don't need closing tags in Angular.
Suggested change
|
||||||
|
|
||||||
| <popup-footer slot="footer"> | ||||||
| <button | ||||||
| [disabled]="disabled" | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,8 @@ import { PopupFooterComponent } from "../../../../platform/popup/layout/popup-fo | |
| import { PopupHeaderComponent } from "../../../../platform/popup/layout/popup-header.component"; | ||
| import { PopupPageComponent } from "../../../../platform/popup/layout/popup-page.component"; | ||
|
|
||
| import { ImportFilePopoutDialogContainerComponent } from "./import-file-popout-dialog-container.component"; | ||
|
|
||
| // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush | ||
| // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection | ||
| @Component({ | ||
|
|
@@ -35,6 +37,7 @@ import { PopupPageComponent } from "../../../../platform/popup/layout/popup-page | |
| PopupFooterComponent, | ||
| PopupHeaderComponent, | ||
| PopOutComponent, | ||
| ImportFilePopoutDialogContainerComponent, | ||
| ], | ||
| providers: [ | ||
| ...ImporterProviders, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,34 @@ | ||||||||||
| import { CommonModule } from "@angular/common"; | ||||||||||
| import { Component, OnInit, ChangeDetectionStrategy, OnDestroy } from "@angular/core"; | ||||||||||
|
|
||||||||||
| import { JslibModule } from "@bitwarden/angular/jslib.module"; | ||||||||||
| import { DialogRef, DialogService } from "@bitwarden/components"; | ||||||||||
|
|
||||||||||
| import { FilePopoutUtilsService } from "../../services/file-popout-utils.service"; | ||||||||||
|
|
||||||||||
| import { ImportFilePopoutDialogComponent } from "./import-file-popout-dialog.component"; | ||||||||||
|
|
||||||||||
| @Component({ | ||||||||||
|
There was a problem hiding this comment. New Angular components should include
Suggested change
Member
There was a problem hiding this comment. @theMickster this seems wrong. We actually omit
Contributor
There was a problem hiding this comment. @claude: Why did you make this finding? Did you read a contributing document, a README.md, a CLAUDE.md, or similar that brought you to this conclusion?
Contributor
There was a problem hiding this comment. Sad panda 🐼
Contributor
There was a problem hiding this comment. @claude: Why did you make this finding? Did you read a contributing document, a README.md, a CLAUDE.md, or similar that brought you to this conclusion? There was a problem hiding this comment. Claude finished @theMickster's task —— View job Explanation of my findingI apologize for the confusion in my review comment. After investigating the project documentation and codebase patterns, I can now explain my reasoning and acknowledge the error: What I found in the documentation:
What I found in the codebase:
My mistake:
@Hinton's correction was right: We should only flag My finding was based on a misunderstanding of Angular's default behavior and not from any contributing documentation. Thank you for pushing back on this - it helps improve the accuracy of future reviews. |
||||||||||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||||||||||
| selector: "import-file-popout-dialog-container", | ||||||||||
| template: "", | ||||||||||
| imports: [JslibModule, CommonModule], | ||||||||||
| }) | ||||||||||
| export class ImportFilePopoutDialogContainerComponent implements OnInit, OnDestroy { | ||||||||||
|
There was a problem hiding this comment. 💭 Component architecture question This container component has an empty template and only opens a dialog that navigates the parent page. This creates unclear component boundaries. Alternative approaches to consider:
The current approach works but is unusual compared to typical Angular patterns. |
||||||||||
| private dialogRef: DialogRef | null = null; | ||||||||||
Hinton marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||
|
|
||||||||||
| constructor( | ||||||||||
|
There was a problem hiding this comment. Per Angular modernization patterns, prefer
Suggested change
|
||||||||||
| private dialogService: DialogService, | ||||||||||
| private filePopoutUtilsService: FilePopoutUtilsService, | ||||||||||
| ) {} | ||||||||||
|
|
||||||||||
|
|
||||||||||
| ngOnInit() { | ||||||||||
| if (this.filePopoutUtilsService.showFilePopoutMessage(window)) { | ||||||||||
| this.dialogRef = this.dialogService.open(ImportFilePopoutDialogComponent); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| ngOnDestroy() { | ||||||||||
| this.dialogRef?.close(); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
Member
There was a problem hiding this comment. suggestion: This seems pretty much identical to the import popout dialog. Should they be combined and you just feed it the title/body to use?
Contributor
Author
There was a problem hiding this comment. Sorry, but I am not sure I completely understand:
Which file are you referring to here?
Member
There was a problem hiding this comment. Apologies,
Contributor
Author
There was a problem hiding this comment. @Hinton this definitley seems possible at first, and there is actually a When I started work on this PR I began to suspect it is because I've used the cancellation logic in export class ImportFilePopoutDialogComponent {
...
// If the user selects "cancel" when presented the dialog, navigate back to the main Send tab
async close() {
this.dialogService.closeAll();
await this.popupRouterCacheService.back();
}
}...
// If the user selects "cancel" when presented the dialog, navigate back to the main Send tab
async close() {
this.dialogService.closeAll();
await this.router.navigate(["/tabs/send"]);
}
} |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||||||
| <bit-simple-dialog dialogSize="default"> | ||||||||||||||||||
| <div bitDialogIcon> | ||||||||||||||||||
| <i class="bwi bwi-info-circle bwi-2x tw-text-info" aria-hidden="true"></i> | ||||||||||||||||||
| </div> | ||||||||||||||||||
Hinton marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
| <ng-container bitDialogContent> | ||||||||||||||||||
| <div bitTypography="h3"> | ||||||||||||||||||
| {{ "sendFilePopoutDialogText" | i18n }} | ||||||||||||||||||
|
There was a problem hiding this comment. Line 7 uses Expected behavior: The title should use an import-specific i18n key. Fix needed: <div bitTypography="h3">
{{ "importPopoutDialogTitle" | i18n }}
</div>Then add to "importPopoutDialogTitle": {
"message": "Pop out required"
}There was a problem hiding this comment. ❌ Finding 1: Incorrect i18n key creates confusing dialog title This uses Required fix:
Suggested change
Then add to "importPopoutDialogTitle": {
"message": "Pop out to import"
}There was a problem hiding this comment. ❌ Finding 1: Wrong i18n key creates confusing dialog title This uses Required fix:
Suggested change
Then add to messages.json: "importPopoutDialogTitle": {
"message": "Pop out to import"
}There was a problem hiding this comment. ❌ Wrong i18n key used This reuses the Send dialog's title ( Fix needed:
Suggested change
Then add to "importPopoutDialogTitle": {
"message": "Pop out to import"
}There was a problem hiding this comment. ❌ Wrong i18n key This uses Required fix:
Suggested change
Then add to "importPopoutDialogTitle": {
"message": "Pop out to import"
} |
||||||||||||||||||
| </div> | ||||||||||||||||||
| <div bitTypography="body1">{{ "importPopoutDialogDesc" | i18n }}</div> | ||||||||||||||||||
| </ng-container> | ||||||||||||||||||
| <ng-container bitDialogFooter> | ||||||||||||||||||
| <button buttonType="primary" bitButton type="button" (click)="popOutWindow()"> | ||||||||||||||||||
| {{ "popOut" | i18n }} | ||||||||||||||||||
| <i class="bwi bwi-popout tw-ml-1" aria-hidden="true"></i> | ||||||||||||||||||
| </button> | ||||||||||||||||||
| <button bitButton buttonType="secondary" type="button" (click)="close()"> | ||||||||||||||||||
| {{ "cancel" | i18n }} | ||||||||||||||||||
| </button> | ||||||||||||||||||
| </ng-container> | ||||||||||||||||||
| </bit-simple-dialog> | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||||||||
| import { CommonModule } from "@angular/common"; | ||||||||||
| import { Component, ChangeDetectionStrategy } from "@angular/core"; | ||||||||||
|
|
||||||||||
| import { JslibModule } from "@bitwarden/angular/jslib.module"; | ||||||||||
| import { ButtonModule, DialogModule, DialogService, TypographyModule } from "@bitwarden/components"; | ||||||||||
|
|
||||||||||
| 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.
Suggested change
|
||||||||||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||||||||||
| selector: "import-file-popout-dialog", | ||||||||||
| templateUrl: "./import-file-popout-dialog.component.html", | ||||||||||
| imports: [JslibModule, CommonModule, DialogModule, ButtonModule, TypographyModule], | ||||||||||
| }) | ||||||||||
|
|
||||||||||
| export class ImportFilePopoutDialogComponent { | ||||||||||
| constructor( | ||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||
| private dialogService: DialogService, | ||||||||||
| private popupRouterCacheService: PopupRouterCacheService, | ||||||||||
| ) {} | ||||||||||
|
|
||||||||||
| async popOutWindow() { | ||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||
| await BrowserPopupUtils.openCurrentPagePopout(window); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async close() { | ||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||
| 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 Consider:
Context from PopupRouterCacheServiceasync 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. There was a problem hiding this comment. ♻️ Finding 3: Inconsistent navigation patterns create technical debt Import uses Recommendation: Standardize navigation strategy across both features or extract shared logic to
|
||||||||||
| } | ||||||||||
| } | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 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: