Conversation
…fore state clearance. This allows canDeactivate guards to function properly, preventing state changes if the user cancels the logout. Refactor logout logic to improve user experience during logout flow.
…g logout and navigation. Removed unused canDeactivate registration in PoliciesComponent and streamlined the PoliciesDeactivateGuard to directly check user authentication status. This ensures smoother transitions when locking the vault or logging out, preventing unnecessary warnings and enhancing overall functionality.
…dle null active accounts during logout. This change improves user experience by ensuring that the guards disarm correctly when the active account is null, preventing unnecessary warnings and enhancing navigation flow during logout and policy editing.
…. Removed unused initial value signals and streamlined the dirty state checks for form components, enhancing clarity and maintainability of the code.
…gComponent to simplify the component and improve code maintainability.
…ity for dialog interactions. Updated button actions to call close method, ensuring proper dialog closure and integration with the discard-edits dialog when there are unsaved changes. Enhanced MultiStepPolicyEditDialogComponent to reset dirty state on step advancement, improving user experience during policy editing.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR fixes several bugs in the policy edit Discard Edits dialog: the policies route deactivation guard now skips its dirty-check when the user is locked or logged out; the modal/drawer dialog disarms its discard guard during a lock/logout transition; the dialog X button no longer bypasses Code Review Details
|
…lock functionality. Removed unused Injector dependency in AppComponent and streamlined logout process to ensure canDeactivate guards are respected. Updated AccountMenuComponent to utilize LockService for locking the vault, improving user experience during state transitions.
Remove navigate-before-logout change; no net changes to this file should be on this branch. Co-authored-by: Cursor <cursoragent@cursor.com>
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| protected readonly dialogService: DialogService, | ||
| protected readonly cdkDialogRef: CdkDialogRef, | ||
| protected readonly configService: ConfigService, | ||
| private readonly authService: AuthService, |
There was a problem hiding this comment.
AuthService constructor dependency breaks existing tests.
Details and fix
PolicyEditDialogComponent and MultiStepPolicyEditDialogComponent now require AuthService via constructor injection (not @Optional()), but neither policy-edit-dialog.component.spec.ts nor multi-step-policy-edit-dialog.component.spec.ts provides one in TestBed.configureTestingModule. TestBed.createComponent(...) in the existing beforeEach blocks will throw NullInjectorError: No provider for AuthService!, failing every test in both spec files.
Fix: add a mock provider in both spec files, e.g.:
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
// ...
{ provide: AuthService, useValue: mock<AuthService>() },Also consider provisioning authService.authStatusFor$ to return a non-emitting observable (e.g. NEVER or of(AuthenticationStatus.Unlocked)) so the new subscription in setupDiscardGuard doesn't disrupt other tests if ngAfterViewInit runs.
| if (confirmed) { | ||
| // Disarm the beforeunload handler immediately. Angular won't destroy this component | ||
| // until its next change-detection cycle, but a lock/logout can trigger | ||
| // window.location.reload() before that cycle runs. Clearing the flag here ensures | ||
| // the handler doesn't block the reload after the user has already confirmed discard. | ||
| this.discardGuardEnabled.set(false); | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: Comment references a beforeunload handler that no longer exists.
Details
This same PR removes the @HostListener("window:beforeunload") handler, so the rationale "Disarm the beforeunload handler immediately" no longer applies. After this point, discardGuardEnabled is read only by cancel(), which won't be invoked again on this close path since closePredicate is about to resolve true and tear down the dialog. Either drop discardGuardEnabled.set(false) here or update the comment to reflect the actual reason for the assignment (currently it appears load-bearing, but isn't).
The same stale rationale appears at lines 162–166 ("so the browser's beforeunload dialog cannot block the lock/logout reload") for the auth-status subscription — worth aligning that comment too.
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-36512
https://bitwarden.atlassian.net/browse/PM-36516
https://bitwarden.atlassian.net/browse/PM-36517
https://bitwarden.atlassian.net/browse/PM-36518
https://bitwarden.atlassian.net/browse/PM-36532
https://bitwarden.atlassian.net/browse/PM-36533
📔 Objective
Resolves all bugs brought upon by the Discard Edits dialog recently implemented.