-
Notifications
You must be signed in to change notification settings - Fork 1.5k
PM-22836 Import popout crashes unexpectedly #16928
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.
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
0581ebc
fe765b1
deb75dd
4915c99
d4db7d0
5d426bc
07986ca
f78e6f9
7b53db2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -246,7 +246,7 @@ const routes: Routes = [ | |
| path: "import", | ||
| component: ImportBrowserV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, | ||
| }, | ||
| { | ||
| path: "export", | ||
|
|
@@ -324,7 +324,7 @@ const routes: Routes = [ | |
| path: "add-send", | ||
| component: SendAddEditV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Why are we not saving the url here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 Finding 5: Missing route configuration for import path The Questions:
This addresses the reviewer's question about why we're not saving the URL here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ Finding 8: Import route missing The Why this matters:
Required change: {
path: "import",
component: ImportBrowserV2Component,
canActivate: [authGuard],
data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties,
} |
||
| }, | ||
| { | ||
| path: "edit-send", | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||
| import { CommonModule } from "@angular/common"; | ||||||||
| import { Component } from "@angular/core"; | ||||||||
| import { Router } from "@angular/router"; | ||||||||
|
|
||||||||
| import { JslibModule } from "@bitwarden/angular/jslib.module"; | ||||||||
| import { ButtonModule, DialogModule, DialogService, TypographyModule } from "@bitwarden/components"; | ||||||||
|
|
@@ -14,13 +15,18 @@ import BrowserPopupUtils from "../../../../platform/browser/browser-popup-utils" | |||||||
| imports: [JslibModule, CommonModule, DialogModule, ButtonModule, TypographyModule], | ||||||||
| }) | ||||||||
| export class SendFilePopoutDialogComponent { | ||||||||
| constructor(private dialogService: DialogService) {} | ||||||||
| constructor( | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| private dialogService: DialogService, | ||||||||
| private router: Router, | ||||||||
| ) {} | ||||||||
|
|
||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use inject instead of constructor |
||||||||
| async popOutWindow() { | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| await BrowserPopupUtils.openCurrentPagePopout(window); | ||||||||
| } | ||||||||
|
|
||||||||
| close() { | ||||||||
| async close() { | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.router.navigate(["/tabs/send"]); | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ Finding 3: Inconsistent navigation patterns create technical debt The Send dialog uses Issue: Both components solve the same problem (prevent file selector outside popout) but use different navigation strategies. Recommendation: Standardize on one approach:
|
||||||||
| } | ||||||||
| } | ||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont need closing tag There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New Angular components should include
Suggested change
|
||||||||||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||||||||||
| selector: "import-file-popout-dialog-container", | ||||||||||
| template: "", | ||||||||||
| imports: [JslibModule, CommonModule], | ||||||||||
| }) | ||||||||||
| export class ImportFilePopoutDialogContainerComponent implements OnInit, OnDestroy { | ||||||||||
| private dialogRef: DialogRef | null = null; | ||||||||||
|
|
||||||||||
| constructor( | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per Angular modernization patterns, prefer
Suggested change
|
||||||||||
| private dialogService: DialogService, | ||||||||||
| private filePopoutUtilsService: FilePopoutUtilsService, | ||||||||||
| ) {} | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inject |
||||||||||
|
|
||||||||||
| ngOnInit() { | ||||||||||
| if (this.filePopoutUtilsService.showFilePopoutMessage(window)) { | ||||||||||
| this.dialogRef = this.dialogService.open(ImportFilePopoutDialogComponent); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| ngOnDestroy() { | ||||||||||
| this.dialogRef?.close(); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| 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> | ||||||||||
| <ng-container bitDialogContent> | ||||||||||
| <div bitTypography="h3"> | ||||||||||
| {{ "sendFilePopoutDialogText" | i18n }} | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ 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"
} |
||||||||||
| </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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||||||||||
| selector: "import-file-popout-dialog", | ||||||||||
| templateUrl: "./import-file-popout-dialog.component.html", | ||||||||||
| imports: [JslibModule, CommonModule, DialogModule, ButtonModule, TypographyModule], | ||||||||||
| }) | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. standalone component |
||||||||||
| export class ImportFilePopoutDialogComponent { | ||||||||||
| constructor( | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| private dialogService: DialogService, | ||||||||||
| private popupRouterCacheService: PopupRouterCacheService, | ||||||||||
| ) {} | ||||||||||
|
|
||||||||||
| async popOutWindow() { | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| await BrowserPopupUtils.openCurrentPagePopout(window); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async close() { | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ 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: