[PM-27619] assign tasks component

[AlexRubik]

🎟️ Tracking

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

📔 Objective

mvp for assign tasks component which is part of the new applications review flow

📸 Screenshots

27619.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

[github-actions]

Checkmarx One – Scan Summary & Details – 9ed9cdc3-d491-4e8c-bdf1-b1ac37e20a37

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	Missing_HSTS_Header	/libs/common/src/services/api.service.ts: 198	details The web-application does not define an HSTS header, leaving it vulnerable to attack. ID: 6sbMocbf6FBC03W0xbXQgzbeSJk%3D Attack Vector

[codecov]

Codecov Report

❌ Patch coverage is 0.61728% with 161 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.44%. Comparing base (8e1a6a3) to head (32dd082).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...review-dialog/new-applications-dialog.component.ts	0.00%	76 Missing ⚠️
...vices/domain/risk-insights-orchestrator.service.ts	2.38%	41 Missing ⚠️
...eview-dialog/review-applications-view.component.ts	0.00%	23 Missing ⚠️
...ation-review-dialog/assign-tasks-view.component.ts	0.00%	11 Missing ⚠️
...ss-intelligence/activity/all-activity.component.ts	0.00%	7 Missing ⚠️
...sights/services/view/risk-insights-data.service.ts	0.00%	1 Missing ⚠️
.../access-intelligence/access-intelligence.module.ts	0.00%	1 Missing ⚠️
...nce/all-applications/all-applications.component.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17125      +/-   ##
==========================================
- Coverage   40.48%   40.44%   -0.05%     
==========================================
  Files        3516     3519       +3     
  Lines      100416   100551     +135     
  Branches    15040    15053      +13     
==========================================
+ Hits        40657    40664       +7     
- Misses      58046    58174     +128     
  Partials     1713     1713              

☔ 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.

[claude]

Code Review: PM-27619 Assign Tasks Component

---

Summary of Changes Since Last Review

The PR has progressed significantly with commit 1dd92f8f52 addressing previous Claude comments. Key improvements include:

• ✅ Fixed: Set manipulation logic now uses native Set.delete() method (new-applications-dialog.component.ts:137-147)
• ✅ Fixed: Improved distinctUntilChanged comparison logic for applications array (risk-insights-orchestrator.service.ts:114-123)
• ✅ Removed: Dead/commented code cleanup
• Refactored dialog into multi-view component structure with embedded views
• Added comprehensive task assignment flow with video demonstrations
• Enhanced UI with search, bulk select, and improved UX

---

Critical Issues Found

1. ❌ Missing Unit Tests (risk-insights-orchestrator.service.ts, all dialog components)

Severity: High
Location: All new components in application-review-dialog/

The PR adds 162 lines of untested code (per Codecov report: 0.61728% patch coverage). Specifically:

• new-applications-dialog.component.ts: 76 lines missing coverage
• risk-insights-orchestrator.service.ts: 41 lines missing coverage
• review-applications-view.component.ts: 23 lines missing coverage
• assign-tasks-view.component.ts: 11 lines missing coverage

Impact: Complex state management logic, multi-step workflows, and RxJS operations are completely untested. This is particularly concerning for:

• Dialog state transitions (SelectApplications ↔ AssignTasks)
• Error handling in handleAssignTasks() (new-applications-dialog.component.ts:184-251)
• Application data merging logic in _updateApplicationData() (risk-insights-orchestrator.service.ts:815-849)

Recommendation: Add unit tests covering:

• Component state transitions and user interactions
• Error scenarios in the assign tasks flow
• Edge cases in selection toggling (empty sets, all selected, partial selection)
• Service method behavior in _updateApplicationData()

---

2. ⚠️ Async Race Condition Risk (all-activity.component.ts:132)

Location: all-activity.component.ts:132-148

async onReviewNewApplications() {
  const organizationId = this.activatedRoute.snapshot.paramMap.get("organizationId");
  
  if (!organizationId) {
    return;  // ❌ Silent failure - no user feedback
  }
  
  const dialogRef = NewApplicationsDialogComponent.open(this.dialogService, {
    newApplications: this.newApplications,  // ⚠️ Could be stale if observable emits during this method
    organizationId: organizationId as OrganizationId,
  });
  
  await lastValueFrom(dialogRef.closed);
}

Issues:

1. Silent failure: When organizationId is missing, the method returns without notifying the user or logging the error
2. Stale data risk: this.newApplications is populated via subscription (line 82-88). If the observable emits while this method executes, the dialog receives stale data
3. Type assertion: Casting organizationId to OrganizationId bypasses type safety

Recommendations:

async onReviewNewApplications() {
  const organizationId = this.activatedRoute.snapshot.paramMap.get("organizationId");
  
  if (!organizationId) {
    this.logService.error("[AllActivityComponent] Missing organizationId");
    this.toastService.showToast({
      variant: "error",
      title: this.i18nService.t("error"),
      message: this.i18nService.t("organizationIdMissing"),
    });
    return;
  }
  
  // Get latest data from observable instead of relying on component property
  const newApplications = await firstValueFrom(this.dataService.newApplications$);
  
  const dialogRef = NewApplicationsDialogComponent.open(this.dialogService, {
    newApplications,
    organizationId: organizationId as OrganizationId,
  });
  
  await lastValueFrom(dialogRef.closed);
}

---

3. 🤔 Inconsistent Array Comparison Logic (risk-insights-orchestrator.service.ts:114-123)

Location: risk-insights-orchestrator.service.ts:114-123

distinctUntilChanged((prev, curr) => {
  if (prev.length !== curr.length) {
    return false;
  }
  return prev.every(
    (app, i) =>
      app.applicationName === curr[i].applicationName &&
      app.atRiskPasswordCount === curr[i].atRiskPasswordCount,  // Only checks 2 fields
  );
})

Issue: The comparison only checks applicationName and atRiskPasswordCount, but ApplicationHealthReportDetail contains many more fields:

• passwordCount
• atRiskMemberCount
• atRiskMemberDetails[]
• etc.

Impact: If other fields change (e.g., passwordCount, atRiskMemberCount), the observable won't emit, potentially showing stale data in the UI.

Recommendation: Either:

1. Document why only these two fields matter for change detection
2. Or expand comparison to include other relevant fields:

distinctUntilChanged((prev, curr) => {
  if (prev.length !== curr.length) return false;
  return prev.every((app, i) =>
    app.applicationName === curr[i].applicationName &&
    app.atRiskPasswordCount === curr[i].atRiskPasswordCount &&
    app.passwordCount === curr[i].passwordCount &&
    app.atRiskMemberCount === curr[i].atRiskMemberCount
  );
})

---

4. ⚠️ Nullable Coercion in Update Logic (risk-insights-orchestrator.service.ts:835-836)

Location: risk-insights-orchestrator.service.ts:835-836

return {
  applicationName: app.applicationName,
  isCritical: foundApp?.isCritical || app.isCritical,  // ⚠️ Problematic for falsy values
  reviewedDate: foundApp?.reviewedDate || app.reviewedDate,  // ⚠️ Same issue
};

Issue: Using || operator means:

• If foundApp.isCritical is explicitly false, it falls back to app.isCritical (wrong!)
• If foundApp.reviewedDate is null or undefined, it falls back to app.reviewedDate (may be intended, but unclear)

Example scenario:

// User unchecks "critical" for an app
foundApp = { applicationName: "App1", isCritical: false, reviewedDate: new Date() }
app = { applicationName: "App1", isCritical: true, reviewedDate: null }

// Result: isCritical becomes TRUE (wrong! should be FALSE)

Recommendation: Use nullish coalescing to preserve explicit false values:

return {
  applicationName: app.applicationName,
  isCritical: foundApp?.isCritical ?? app.isCritical,
  reviewedDate: foundApp?.reviewedDate ?? app.reviewedDate,
};

---

Suggested Improvements

5. 🎨 Code Duplication in Template (new-applications-dialog.component.html:43-96)

Location: new-applications-dialog.component.html

Button definitions are nearly identical between the two view states. Consider extracting to a shared variable:

// In component
protected readonly footerButtons = computed(() => {
  const view = this.currentView();
  if (view === DialogView.SelectApplications) {
    return {
      primary: { label: 'markAsCritical', action: () => this.handleMarkAsCritical() },
      secondary: { label: 'cancel', action: () => this.handleCancel() }
    };
  }
  return {
    primary: { label: 'assignTasks', action: () => this.handleAssignTasks() },
    secondary: { label: 'back', action: () => this.onBack() }
  };
});

---

6. 🎨 Component Responsibility Violation (assign-tasks-view.component.ts:39)

Location: assign-tasks-view.component.ts:39

providers: [AccessIntelligenceSecurityTasksService, DefaultAdminTaskService],

Issue: This presentational component provides its own service instances, violating single responsibility principle. The component comment acknowledges this is a workaround for DI issues.

Why this is problematic:

• Service state is isolated to this component instance
• Makes testing harder (must mock providers at component level)
• Violates Angular best practices (services should be provided at module/root level)

Recommendation:

• Investigate the root cause of the NullInjectorError
• Provide services at the appropriate level (likely the AccessIntelligenceModule)
• If component-level providers are truly needed, document the architectural decision

---

7. 📝 TODO Comment (new-applications-dialog.component.ts:210)

Location: new-applications-dialog.component.ts:210-211

// Manual enrich for type matching
// TODO Consolidate in model updates

This manual enrichment is a code smell suggesting type inconsistencies. Consider:

• Creating a helper function if this pattern is reused
• Or consolidating types as the TODO suggests
• Link to a tracking ticket if this is deferred work

---

8. ⚠️ Inconsistent Method Declaration Style (all-activity.component.ts:132-156)

Location: all-activity.component.ts:132-156

The commit changed from arrow functions to method declarations inconsistently:

// Changed TO method declaration
async onReviewNewApplications() { }
async onViewAtRiskMembers() { }
async onViewAtRiskApplications() { }

// But elsewhere in file, still uses arrow functions (lines exist elsewhere)

Why this matters: Arrow functions in class fields have different this binding. If some methods use arrow functions and others don't, it creates confusion about which pattern to follow.

Recommendation: Be consistent - either use arrow functions throughout (for template event handlers) or use method declarations. The codebase seems to prefer arrow functions for template-bound methods.

---

9. 🎨 Magic Number in Template (new-applications-dialog.component.html:36)

Location: new-applications-dialog.component.html:36

[totalApplicationsCount]="this.dialogParams.newApplications.length"

This recalculates length on every change detection cycle. Consider:

protected readonly totalApplicationsCount = computed(() => this.dialogParams.newApplications.length);

---

10. 📝 Missing Accessibility Labels (review-applications-view.component.html:17,48-51)

Location: review-applications-view.component.html

Good: You have [attr.aria-label] on buttons.
Missing: No aria-describedby or visual text explaining what the star icon means before users interact with the table.

Recommendation: Add a visually-hidden description or aria-describedby linking to the helper text at line 19-21.

---

Good Practices Observed

• ✅ ADR-0025 compliance: Using const objects instead of enums (DialogView, NewApplicationsDialogResultType)
• ✅ Signal adoption: Appropriate use of Angular Signals for component state
• ✅ OnPush change detection: Main dialog component uses OnPush strategy
• ✅ Internationalization: All UI strings use i18n pipe
• ✅ RxJS best practices: Uses takeUntilDestroyed() for subscription management
• ✅ Error handling: Comprehensive try-catch with user-facing toast notifications
• ✅ Type safety: Good use of TypeScript types for dialog data
• ✅ Accessibility: aria-labels on interactive elements

---

Action Items for Author

Must Address (Blocking Issues):

1. Add unit tests for all new components (codecov shows 161 lines missing coverage)
2. Fix nullable coercion bug in _updateApplicationData() (use ?? instead of ||)
3. Handle missing organizationId error with user feedback instead of silent return

Should Address (Quality Issues):
4. Review and document distinctUntilChanged field selection logic
5. Resolve component-level service provider workaround
6. Fetch latest newApplications data from observable instead of stale component property

Nice to Have (Improvements):
7. Extract duplicate button logic in template
8. Resolve TODO comment or link to tracking ticket
9. Add computed property for totalApplicationsCount
10. Enhance accessibility with aria-describedby

---

Questions for Discussion

1. Video files: As @Banrion noted, the dark theme video (3.5MB) is significantly larger than the light theme video (1.8MB). Was this intentional, or should both be compressed equally?

2. Service providers: The assign-tasks-view.component provides its own service instances. Is there a plan to resolve this architectural issue?

3. Test coverage: The PR has <1% patch coverage. What's the testing strategy for this feature? Integration tests? E2E?

---

Review Context

Previous Issues Tracked:

• ✅ Set manipulation inefficiency - FIXED in commit 1dd92f8f52
• ✅ Commented dead code - FIXED (removed)
• ✅ distinctUntilChanged shallow comparison - PARTIALLY FIXED (now checks specific fields, but see Issue Overlay Popup #3 above)

Commits Since Last Review: 20 commits, most notably:

• 1dd92f8f52 - Address claude comments
• 831cf5349c - Add columns, description, search, and bulk select
• b9c9737601 - Refactor assign tasks dialog
• eb74aa243a - Add multi-view state management

---

View job run

[Banrion]

🤔 The size difference in these two video files surprise me. Is one compressed or built upon the other somehow?

[maxkpower]

Great catch, one was attached as .mov to the ticket so I converted with ffmpeg. Will check / optimise

[Banrion]

📝 Update both application and summary with metrics in review status saving

[Banrion]

📝 Updated to accept new object and update those values specifically. This does not handle removing elements from the array if that were needed, but it does add applications if they didn't exist in the array already.

[Banrion]

📝 Not required. Can be removed during further code cleanup, but not required for accepting