[PM-26363] Add one time setup dialog for auto confirm

[BTreston]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-26363

📔 Objective

This PR initializes the policy dialog as a one time setup dialog for owners/admins/manage users custom permission. Dialog reads from app state to show only one time when the vault loads for a given client. When the dialog opens, it updates local state to ensure it is not showed again.

📸 Screenshots

DESIGN REVIEW (updated):

Screen.Recording.2025-11-03.at.12.34.44.PM.mov

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[codecov]

Codecov Report

❌ Patch coverage is 17.94872% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.54%. Comparing base (97aef75) to head (33a8a79).
⚠️ Report is 44 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../src/app/vault/individual-vault/vault.component.ts	0.00%	26 Missing ⚠️
...icies/auto-confirm-edit-policy-dialog.component.ts	14.28%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17104      +/-   ##
==========================================
- Coverage   40.55%   40.54%   -0.01%     
==========================================
  Files        3525     3525              
  Lines      100808   100843      +35     
  Branches    15085    15088       +3     
==========================================
+ Hits        40881    40886       +5     
- Misses      58206    58236      +30     
  Partials     1721     1721              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Checkmarx One – Scan Summary & Details – b24d1a64-6a6a-4e69-b58d-46033705f7c6

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CVE-2025-59288	Npm-playwright-1.53.1	details Recommended version: 1.55.1 Description: In versions prior to 1.55.1, improper verification of the cryptographic signature in Playwright allows an unauthorized attacker to perform spoofin... Attack Vector: ADJACENT_NETWORK Attack Complexity: HIGH ID: 8MVGEl%2BlhhBL83VeYNyUlUdLS00%2BhIePCz4c5v%2BhmlI%3D Vulnerable Package

[claude]

Claude finished @BTreston's task —— View job

---

Code Review Complete

Summary of Changes Since Last Review

This PR adds a one-time setup dialog for the auto-confirm policy feature. The implementation includes:

1. Vault Component Enhancement (vault.component.ts): Added setupAutoConfirm() method that displays a policy dialog once per user on vault load
2. Dialog Component Updates (auto-confirm-edit-policy-dialog.component.ts): Enhanced with multi-step submission and first-time dialog flag
3. State Management: Modified AutoConfirmState model to track showSetupDialog boolean
4. Service Registration: Added AutomaticUserConfirmationService to core.module.ts
5. I18n Updates: Renamed message keys for consistency

---

Critical Issues

1. ⚠️ Race Condition Risk in setupAutoConfirm()

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1583-1630

The setupAutoConfirm() method has a potential race condition:

private setupAutoConfirm() {
  // Multiple observables combined
  zip([organization$, featureFlag$, autoConfirmState$, policyEnabled$, this.userId$])
    .pipe(
      first(),
      switchMap(async ([organization, flagEnabled, autoConfirmState, policyEnabled, userId]) => {
        // ... conditional logic
        if (showDialog) {
          await this.openAutoConfirmFeatureDialog(organization);  // Opens dialog
          
          await this.autoConfirmService.upsert(userId, {          // Updates state
            ...autoConfirmState,
            showSetupDialog: false,
          });
        }
      }),
      takeUntil(this.destroy$),
    )
    .subscribe({
      error: (err: unknown) => this.logService.error("Failed to update auto-confirm state", err),
    });
}

Problem:

• If the user navigates away or the component is destroyed while the dialog is open, the state update (showSetupDialog: false) will still execute
• The takeUntil(this.destroy$) only stops the outer observable subscription, but won't cancel the async operation inside switchMap
• This means the dialog could be shown again if the component re-initializes before the state update completes

Recommendation:

• Check if the component is still active before updating state
• Move state update to dialog close handling, or
• Add a check: if (this.destroy$.closed) return; before the upsert call

2. ❌ Missing Subscription Cleanup for autoConfirmDialogRef

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1566-1581

private async openAutoConfirmFeatureDialog(organization: Organization) {
  if (this.autoConfirmDialogRef) {
    return;
  }

  this.autoConfirmDialogRef = AutoConfirmPolicyDialogComponent.open(this.dialogService, {
    data: {
      policy: new AutoConfirmPolicy(),
      organizationId: organization.id,
      firstTimeDialog: true,
    },
  });

  await lastValueFrom(this.autoConfirmDialogRef.closed);  // ⚠️ No error handling
  this.autoConfirmDialogRef = undefined;
}

Problem: According to Angular best practices (ADR-0003), subscriptions/observables should be properly managed. The lastValueFrom will throw if the observable completes without emitting a value.

Recommendation: Add error handling:

await lastValueFrom(this.autoConfirmDialogRef.closed).catch(() => {
  // Dialog was closed without a result
});

3. ⚠️ Memory Leak: Missing takeUntilDestroyed() Pattern

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1605-1630

While the code uses takeUntil(this.destroy$), the codebase preference (per ADR-0003) is to use takeUntilDestroyed() in constructors for better automatic cleanup:

// Current approach - requires manual destroy$ management
.pipe(takeUntil(this.destroy$))

// Recommended approach per ADR-0003
constructor() {
  this.observable$.pipe(takeUntilDestroyed()).subscribe(...);
}

However, since setupAutoConfirm() is called in ngOnInit() and not the constructor, the current pattern is acceptable. Just ensure destroy$.complete() is always called in ngOnDestroy() (which it is at line 652).

4. ⚠️ Unsafe State Update After Async Dialog

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1617-1622

if (showDialog) {
  await this.openAutoConfirmFeatureDialog(organization);
  
  await this.autoConfirmService.upsert(userId, {  // ⚠️ State update happens regardless of dialog result
    ...autoConfirmState,
    showSetupDialog: false,
  });
}

Problem: The state is updated to showSetupDialog: false even if:

• User cancels the dialog
• User dismisses without taking action
• Dialog encounters an error

Recommendation: Only update state if the dialog completes successfully:

const result = await this.openAutoConfirmFeatureDialog(organization);
if (result === 'saved') {
  await this.autoConfirmService.upsert(userId, {
    ...autoConfirmState,
    showSetupDialog: false,
  });
}

Note: The dialog component already updates state in submitAutoConfirm() (line 220-223 of auto-confirm-edit-policy-dialog.component.ts), so this creates duplicate state updates.

---

Suggested Improvements

5. 🎨 Code Duplication: State Update Logic

Location:

• apps/web/src/app/vault/individual-vault/vault.component.ts:1618-1622
• apps/web/src/app/admin-console/organizations/policies/auto-confirm-edit-policy-dialog.component.ts:220-223

Both the vault component AND the dialog component update the showSetupDialog state. This violates the Single Responsibility Principle.

Recommendation: Let the dialog handle ALL state updates. Remove the update from vault.component.ts.

6. 🎨 Complex RxJS Chain Could Be Simplified

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1583-1630

The setupAutoConfirm() method has deeply nested observables with multiple switchMap and combineLatest operations. Consider breaking this into smaller, testable methods:

private shouldShowAutoConfirmDialog$(userId: UserId): Observable<boolean> {
  // Extract the complex logic
}

private setupAutoConfirm() {
  this.userId$.pipe(
    switchMap(userId => this.shouldShowAutoConfirmDialog$(userId)),
    // ... rest of logic
  )
}

7. 🎨 Magic Boolean Values - Consider Enum

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1609-1614

const showDialog =
  flagEnabled &&
  !policyEnabled &&
  autoConfirmState.showSetupDialog &&
  !!organization &&
  (organization.canManageUsers || organization.canManagePolicies);

This complex boolean expression would benefit from comments explaining each condition, or extracting to a named function like canShowAutoConfirmDialog().

8. 🎨 Inconsistent Error Handling

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1627-1629

The error handler only logs errors without notifying the user:

.subscribe({
  error: (err: unknown) => this.logService.error("Failed to update auto-confirm state", err),
});

Recommendation: Consider showing a toast notification to the user if the setup fails.

9. 🎨 Missing JSDoc Comments

The new methods setupAutoConfirm() and openAutoConfirmFeatureDialog() lack documentation. Given the complexity of the setup logic, JSDoc comments would improve maintainability.

10. ℹ️ State Definition Location

Location: libs/state/src/core/state-definitions.ts:39

The AUTO_CONFIRM state definition uses disk-local storage for web, which is appropriate for this use case. This is correctly configured.

11. 🤔 Missing Unit Tests

Test Coverage: The codecov report shows 20% patch coverage with 32 lines missing coverage. Specifically:

• vault.component.ts: 0% coverage for the new setupAutoConfirm() logic (26 missing lines)
• auto-confirm-edit-policy-dialog.component.ts: 14.28% coverage (6 missing lines)

Recommendation: Add unit tests for:

• setupAutoConfirm() method with various permission scenarios
• Dialog state transitions
• Error handling paths
• Race condition scenarios

12. 🎨 Type Safety: any Type Used

Location: apps/web/src/app/admin-console/organizations/policies/auto-confirm-edit-policy-dialog.component.ts:276

} catch (error: any) {  // ⚠️ Using 'any'
  this.toastService.showToast({
    variant: "error",
    message: error.message,
  });
}

Per the file header // @ts-strict-ignore, the file is excluded from strict type checking. However, you should use unknown instead:

} catch (error: unknown) {
  const message = error instanceof Error ? error.message : 'An error occurred';
  this.toastService.showToast({
    variant: "error",
    message,
  });
}

---

Security Considerations

✅ No Security Issues Found

• No unencrypted vault data is being sent to APIs
• No sensitive data is being logged
• Policy permissions are properly checked (canManageUsers, canManagePolicies)
• Feature flag gating is correctly implemented
• State is stored in disk-local storage (appropriate for this use case)

---

Performance Considerations

13. 💡 Observable Chain Efficiency

Location: apps/web/src/app/vault/individual-vault/vault.component.ts:1583-1630

The setupAutoConfirm() creates a complex observable chain that:

• Uses zip() which waits for ALL observables to emit (good)
• Uses first() so it only runs once (good)
• Properly cleans up with takeUntil() (good)

Minor Optimization: Consider using take(1) instead of first() since you're using zip() which guarantees a single emission.

14. 💡 Dialog Instance Check

The guard if (this.autoConfirmDialogRef) prevents multiple dialog instances, which is good. However, if the user somehow triggers this twice, the second call silently returns. Consider logging this scenario for debugging.

---

Good Practices Observed

✅ Proper dependency injection for new services
✅ Feature flag gating for gradual rollout
✅ State persistence using StateProvider
✅ Multi-step dialog workflow with clear user guidance
✅ Permission-based conditional logic
✅ Proper use of observables with cleanup
✅ I18n used for all user-facing strings

---

Action Items for Author

High Priority

1. ❌ Fix race condition in setupAutoConfirm() - add component lifecycle check before state update
2. ❌ Remove duplicate state update (vault component should not update state, let dialog handle it)
3. ❌ Add error handling to lastValueFrom() call
4. ⚠️ Add unit tests to improve coverage from 20% to at least 70%

Medium Priority

5. 🎨 Extract complex boolean logic to named method with documentation
6. 🎨 Add JSDoc comments to new methods
7. 🎨 Change any to unknown in error handling
8. 🎨 Add user-facing error notification for setup failures

Low Priority

9. 💡 Consider extracting observable chain to testable sub-methods
10. ℹ️ Add debug logging when dialog is prevented from opening twice

---

Architectural Alignment

✅ ADR-0003 (Observable Data Services): Properly uses RxJS observables
⚠️ ADR-0003 (Subscription Management): Uses takeUntil() correctly, but has async race condition
✅ ADR-0025 (No TypeScript Enums): No new enums added
✅ ADR-0027 (Signals): Not using Signals (appropriate for cross-platform service)

---

Branch: ac/pm-26363-one-time-setup-dialog-web

[coroiu]

Platform changes are good, have not reviewed the rest

The merge-base changed after approval.

[coroiu]

No changes to platform code since last review

[gbubemismith]

Looks good