[PM-26060] Consolidate Alert to BitwardenKit #2081

KatherineInCode · 2025-10-29T14:38:55Z

🎟️ Tracking

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

📔 Objective

This moves the Alert to BitwardenKit, along with attendant objects, as part of an effort to migrate shared objects to BitwardenKit. Future PRs will bring some other core objects over, which will enable bringing the Select Language UI over.

⏰ 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 · 2025-10-29T14:50:04Z

Checkmarx One – Scan Summary & Details – 8e765be1-9660-4dab-9a3f-32d7a1cbb60a

Great job! No new security vulnerabilities introduced in this pull request

github-actions · 2025-10-29T14:54:06Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

github-actions · 2025-10-29T15:13:56Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

claude · 2025-10-29T15:44:36Z

Claude finished @KatherineInCode's task —— View job

Code Review: Alert Consolidation to BitwardenKit

Review PR changes and recent commits
Check architecture compliance
Verify test coverage and documentation
Compile comprehensive review findings
Post final review feedback

Summary of Changes Since Last Review

The PR author has addressed previous review feedback:

✅ Removed unused method: The setKeyWindowRoot(view:) method was removed from BitwardenTestCase (commit 23369d6) as identified by @matt-livefront
✅ Documentation added: All public APIs in MockUIViewController.swift now have proper DocC documentation
✅ Method ordering: Methods are correctly ordered (public before private)
✅ Pluralization corrected: "Mocks" directory naming fixed (commit 65f39ad)

Architecture & Design Review

✅ Excellent: Follows Target Architecture

The consolidation properly moves Alert and related types to BitwardenKit, which aligns with the project's goal of migrating shared objects to the shared framework. This is the correct architectural direction per the repository structure documented in Docs/Architecture.md.

Key architectural decisions that are correct:

Proper module structure: Alert moved from app-specific modules to BitwardenKit/UI/Platform/Application/Utilities/Alert/
Type disambiguation: Uses BitwardenKit.Alert in places where both modules are imported (e.g., Coordinator.swift)
Test infrastructure: Properly moved to BitwardenKit with appropriate test helpers
Mock objects: MockUIViewController correctly placed in BitwardenKit/UI/Platform/Application/Utilities/Mocks/

✅ Good Practices Observed

Comprehensive DocC documentation on all public APIs
Proper use of Swift protocols (AlertPresentable, Equatable, Hashable)
Test fixtures properly gated with #if DEBUG
Consistent code style throughout

Critical Issues

None Found ✅

All previously identified issues have been resolved.

Suggested Improvements

⛏️ Minor: AlertTextField initialization parameter documentation

In AlertTextField.swift:44, the initializer has a parameter text: String? = nil but the DocC comment says "An optional initial value to pre-fill the text field with."

Current (line 42):

///   - text: An optional initial value to pre-fill the text field with.

Consider clarifying:

///   - text: An optional initial value to pre-fill the text field with. Defaults to `nil`, which results in an empty string.

This makes it clearer that nil becomes an empty string internally (line 59).

Impact: Documentation clarity
File: BitwardenKit/UI/Platform/Application/Utilities/Alert/AlertTextField.swift:42

ℹ️ Info: Test Coverage Gaps

Per codecov report, the following have missing coverage:

MockUIViewController.swift: 32 lines missing coverage (66.66% covered)
- Many mock tracking properties and methods aren't exercised by current tests
- Recommendation: This is acceptable for mock objects as they're utilities for testing
BitwardenTestCase.swift: 6 lines missing (66.66% covered)
- Base test infrastructure - acceptable
Several Processor files: 1 line missing each
- These appear to be the updated showAlert() calls with the new type
- Recommendation: Verify existing processor tests cover the alert display paths

Impact: Test coverage is at 82.91% (within acceptable range)
Action: Consider adding tests that exercise MockUIViewController methods if this mock is intended to be heavily used

🤔 Question: OSLog import in AlertPresentable

AlertPresentable.swift:1 imports OSLog but only uses Logger.application on line 48.

Question: Is Logger.application defined elsewhere in BitwardenKit, or does this require a specific logger setup?

File: BitwardenKit/UI/Platform/Application/Utilities/Alert/AlertPresentable.swift:1

Security Considerations

✅ No Security Issues Identified

The Alert system properly:

Handles user input through AlertTextField
Uses standard UIKit presentation mechanisms
Doesn't store sensitive data
Properly cleans up references in MockUIViewController.reset()

Testing Assessment

✅ Comprehensive Test Coverage

AlertTests.swift: Tests alert creation, controller generation, dismissal callbacks, equality
AlertPresentableTests.swift: Tests presentation logic and dismissal callbacks
AlertErrorTests.swift: Tests error alert creation helpers
Test infrastructure: Properly uses BaseBitwardenTestCase.setKeyWindowRoot(viewController:) method

All critical paths are tested. The 82.91% coverage is acceptable for this type of consolidation work.

Action Items for Author

✅ All Previous Items Completed

No blocking issues remain. The PR is ready for merge pending:

⏳ CI completion: Test and scan workflows are currently in progress
📝 Optional: Consider addressing the suggestions above (all are minor/informational)

Files Reviewed

Key files examined (click to expand)

Core Alert Implementation:

BitwardenKit/UI/Platform/Application/Utilities/Alert/Alert.swift
BitwardenKit/UI/Platform/Application/Utilities/Alert/AlertAction.swift
BitwardenKit/UI/Platform/Application/Utilities/Alert/AlertTextField.swift
BitwardenKit/UI/Platform/Application/Utilities/Alert/AlertPresentable.swift
BitwardenKit/UI/Platform/Application/Utilities/Alert/Alert+Error.swift

Test Infrastructure:

BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockUIViewController.swift
BitwardenKit/Application/TestHelpers/Support/BitwardenTestCase.swift
BitwardenKit/UI/Platform/Application/Utilities/Alert/AlertTests.swift
BitwardenKit/UI/Platform/Application/Utilities/Alert/AlertPresentableTests.swift

Integration Points:

BitwardenShared/UI/Platform/Application/Utilities/Coordinator.swift
AuthenticatorShared/UI/Platform/Application/Utilities/Coordinator.swift
Multiple mock navigators and coordinators

Final Assessment

Status: ✅ Approved - Ready for Merge (pending CI completion)

This is a well-executed consolidation that properly moves shared Alert infrastructure to BitwardenKit. The code follows project guidelines, has comprehensive documentation, and maintains good test coverage. All previous review feedback has been addressed.

Great work on this consolidation effort! 👍

fedemkr

Looks good, just missing some ⛏️ docs

BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockUIViewController.swift

github-actions · 2025-10-29T16:15:37Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

KatherineInCode · 2025-10-29T16:18:24Z

Looks good, just missing some ⛏️ docs

Good catches. Though now I wonder if there's a way we could change the Claude configuration so that in its review it generates text that points out the missing docs as well 🤔

BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockUIViewController.swift

github-actions · 2025-10-29T16:33:20Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

github-actions · 2025-10-29T16:42:33Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

github-actions · 2025-10-29T16:44:54Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

github-actions · 2025-10-29T16:45:32Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

github-actions · 2025-10-29T16:57:14Z

Warning

@KatherineInCode Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details.

codecov · 2025-10-29T17:28:33Z

Codecov Report

❌ Patch coverage is 84.23237% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.93%. Comparing base (87e3a22) to head (23369d6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ication/Utilities/Mocks/MockUIViewController.swift	66.66%	32 Missing ⚠️
...I/Platform/Application/Utilities/Alert/Alert.swift	83.33%	1 Missing ⚠️
...ardenShared/UI/Auth/Landing/LandingProcessor.swift	0.00%	1 Missing ⚠️
...red/UI/Auth/VaultUnlock/VaultUnlockProcessor.swift	0.00%	1 Missing ⚠️
...tem/AddEditSendItem/AddEditSendItemProcessor.swift	0.00%	1 Missing ⚠️
.../UI/Vault/Vault/VaultList/VaultListProcessor.swift	0.00%	1 Missing ⚠️
GlobalTestHelpers/MockStackNavigator.swift	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2081      +/-   ##
==========================================
- Coverage   85.31%   82.93%   -2.39%     
==========================================
  Files        1693     1981     +288     
  Lines      144450   160987   +16537     
==========================================
+ Hits       123238   133512   +10274     
- Misses      21212    27475    +6263

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

KatherineInCode · 2025-10-29T18:13:39Z

@claude The setKeyWindowRoot(viewController: UIViewController) function is in BitwardenBaseTestCase.

claude · 2025-10-29T18:14:31Z

Claude finished @KatherineInCode's task —— View job

Correction to Previous Review

Locate and read BitwardenBaseTestCase.swift
Verify setKeyWindowRoot(viewController:) method exists
Check inheritance relationship between BitwardenTestCase and BaseBitwardenTestCase
Update review with corrected information

Apology and Correction

You're absolutely correct, and I apologize for the error in my previous review.

The setKeyWindowRoot(viewController: UIViewController) method does exist and is located in:

File: TestHelpers/Support/BaseBitwardenTestCase.swift:122

Inheritance Chain Confirmed:

BitwardenTestCase (in BitwardenKit) 
  ↓ extends
BaseBitwardenTestCase (in TestHelpers)
  ↓ contains
setKeyWindowRoot(viewController: UIViewController) at line 122

The tests in AlertTests.swift and AlertPresentableTests.swift correctly call this method and will compile without issues.

Corrected Assessment

Previous Review Status: ❌ Critical Issue - INCORRECT
Updated Status: ✅ No compilation issues

The "critical issue" I flagged in my previous review was incorrect. The method exists in the parent class BaseBitwardenTestCase, which BitwardenTestCase properly inherits from. All test files have appropriate access to this method.

Updated PR Assessment

With this correction, the PR has:

✅ No critical issues
✅ Proper test infrastructure with correct inheritance
✅ All required methods available through the inheritance chain
⛏️ Minor documentation suggestions (as noted by @fedemkr)

The code consolidation work is sound and ready for merge pending any stylistic preferences from the team.

KatherineInCode · 2025-10-29T19:08:43Z

@claude Regarding action item 1, those three things already have DocC comments.

Regarding action item 2, they are now in the correct order.

claude · 2025-10-29T19:09:48Z

Claude finished @KatherineInCode's task —— View job

Verifying Action Items

Read current state of MockUIViewController.swift
Verify DocC documentation for the three items
Verify method ordering
Provide updated assessment

Verification Results

You're absolutely correct on both points! I apologize for the confusion in my previous review. After reading the current state of MockUIViewController.swift, I can confirm:

✅ Action Item 1: DocC Documentation

All three items now have proper DocC comments:

view property (lines 76-85): ✅ Has DocC

/// Returns the mock view or the default view if no mock view is set.
override public var view: UIView! {

isViewLoaded property (lines 87-90): ✅ Has DocC

/// Returns whether the mock view or default view is loaded.
override public var isViewLoaded: Bool {

loadView() method (lines 131-138): ✅ Has DocC

/// Creates the view controller's view programmatically.
/// Sets up a mock view with the predefined mock window size.
override public func loadView() {

✅ Action Item 2: Method Ordering

The methods are now in the correct order per Swift style conventions:

reset() (public method) at line 190
setUpMockHierarchy() (private method) at line 208

Public methods correctly appear before private methods.

Updated Assessment

Both action items from my previous review have been properly addressed. The file now follows all project guidelines:

✅ All public APIs have DocC documentation
✅ Methods are ordered according to Swift conventions (public before private)
✅ Code follows Bitwarden project standards

Thank you for addressing the feedback, and I apologize for any confusion caused by my initial review comments!

View job

BitwardenKit/Application/TestHelpers/Support/BitwardenTestCase.swift

KatherineInCode added 16 commits October 29, 2025 09:24

Move tests

2e5fdd0

Run tests on BWK

2ea0b57

Fix some test things

a9a91cc

BWA to build

139e6bd

PM build no test

18438e6

PM to build

72eff1f

Public

cdc0fa0

Restore old tests for demo

433b3e7

Move mock

d301c54

Mock

e7ec214

More mock

b4e6a30

Remove file

ed0f600

Complete move

0130b10

Remove mock navigation controller

9f39a4e

Adjust commens

e257dfe

Clean warnings

f97485c

KatherineInCode marked this pull request as ready for review October 29, 2025 15:43

KatherineInCode requested review from a team and matt-livefront as code owners October 29, 2025 15:43

KatherineInCode added 2 commits October 29, 2025 10:58

Fix variable

6639f57

Add public

d758f48

fedemkr reviewed Oct 29, 2025

View reviewed changes

BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockUIViewController.swift Outdated Show resolved Hide resolved

BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockUIViewController.swift Show resolved Hide resolved

KatherineInCode added 2 commits October 29, 2025 11:23

Update comments and order

217e690

Merge branch 'main' into pm-26060/consolidate-alert

c19bc1f

claude bot reviewed Oct 29, 2025

View reviewed changes

BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockUIViewController.swift Show resolved Hide resolved

claude bot reviewed Oct 29, 2025

View reviewed changes

BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockUIViewController.swift Show resolved Hide resolved

KatherineInCode requested a review from fedemkr October 29, 2025 17:04

Merge branch 'main' into pm-26060/consolidate-alert

f0f7cdc

KatherineInCode added 2 commits October 29, 2025 12:30

Add more comments per Claude suggestions

3b9f8fd

Remove confusing comments per Claude

349937b

KatherineInCode added 2 commits October 29, 2025 13:51

Resolve warnings

f5312b4

Correct pluralization

65f39ad

matt-livefront reviewed Oct 29, 2025

View reviewed changes

BitwardenKit/Application/TestHelpers/Support/BitwardenTestCase.swift Outdated Show resolved Hide resolved

Removed unused

23369d6

KatherineInCode requested a review from matt-livefront October 30, 2025 13:34

matt-livefront approved these changes Oct 30, 2025

View reviewed changes

KatherineInCode merged commit f90ffd8 into main Oct 30, 2025
16 checks passed

KatherineInCode deleted the pm-26060/consolidate-alert branch October 30, 2025 14:43

[PM-26060] Consolidate Alert to BitwardenKit #2081

[PM-26060] Consolidate Alert to BitwardenKit #2081

Uh oh!

Conversation

KatherineInCode commented Oct 29, 2025

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

claude bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review: Alert Consolidation to BitwardenKit

Summary of Changes Since Last Review

Architecture & Design Review

✅ Excellent: Follows Target Architecture

✅ Good Practices Observed

Critical Issues

None Found ✅

Suggested Improvements

Security Considerations

✅ No Security Issues Identified

Testing Assessment

✅ Comprehensive Test Coverage

Action Items for Author

✅ All Previous Items Completed

Files Reviewed

Final Assessment

Uh oh!

fedemkr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

KatherineInCode commented Oct 29, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

github-actions bot commented Oct 29, 2025

Uh oh!

codecov bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

KatherineInCode commented Oct 29, 2025

Uh oh!

claude bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Correction to Previous Review

Apology and Correction

Corrected Assessment

Updated PR Assessment

Uh oh!

KatherineInCode commented Oct 29, 2025

Uh oh!

claude bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verifying Action Items

Verification Results

✅ Action Item 1: DocC Documentation

✅ Action Item 2: Method Ordering

Updated Assessment

github-actions bot commented Oct 29, 2025 •

edited

Loading

claude bot commented Oct 29, 2025 •

edited

Loading

codecov bot commented Oct 29, 2025 •

edited

Loading

claude bot commented Oct 29, 2025 •

edited

Loading

claude bot commented Oct 29, 2025 •

edited

Loading