[PM-27190] Improve performance on Autofill Passwords + Fido2 list

[fedemkr]

🎟️ Tracking

PM-27190

📔 Objective

Refactors the ciphers autofill publisher to use the new approach for the .combinedMultipleSections flow (autofilling Fido2 + Passwords credentials shown in different sections).
This is being done in order to improve the overall performance of the list loading, which will load faster and consume less memory given the credentials are being decrypted and filtered in bulk. This is similar to #1939.

⛏️ Also fixed some warnings in the project.

Important

Added a WORKAROUND when getting the URI on this flow as Apple doesn't seem to be sending the service identifiers thus we can't get the URI from them to filter the vault. Therefore the relying party identifier is taken as the URI to filter in order to fix this scenario until Apple fixes the issue. Check CredentialProviderContext -> uri (it was moved there in order to add tests)

⏰ 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

[claude]

Claude finished @fedemkr's task —— View job

---

Code Review Complete

Summary of Changes

This PR refactors the autofill credentials publisher for the .combinedMultipleSections flow (Fido2 + Passwords displayed in separate sections) to significantly improve performance. The changes align with a similar refactor in #1939 by implementing bulk decryption and filtering to reduce memory usage and improve loading times.

Key changes:

• New Strategy: Added CombinedMultipleAutofillVaultListDirectorStrategy to handle the Fido2 + Passwords flow with multiple sections
• New Data Preparation Method: Added prepareAutofillCombinedMultipleData() to VaultListDataPreparator for batch processing
• Repository Simplification: Removed inline decryption/filtering logic from VaultRepository.ciphersAutofillPublisher(), delegating to the new strategy pattern
• Workaround for iOS Bug: Added fallback logic in CredentialProviderContext.uri to use relyingPartyIdentifier when serviceIdentifiers are not sent by iOS
• Section Ordering: Reversed section order to show "Passkeys for X" first, followed by "Passwords for X"
• New Helper Extension: Added canBeUsedInBasicLoginAutofill computed property to CipherListView
• String Utility: Added httpsNormalized() extension to normalize URLs with HTTPS prefix

---

Critical Issues

None identified. The code appears to be well-implemented with proper error handling, security considerations, and comprehensive test coverage.

---

Suggested Improvements

⚠️ [Architecture] Section Ordering Change Not Documented

Location: BitwardenShared/Core/Vault/Helpers/VaultListSectionsBuilder.swift:118-139

Issue: The PR changes the section order from "Passwords first" to "Passkeys first" based on review comment feedback. However, this behavioral change is not documented in the PR description or commit messages.

Suggestion: While this change was discussed in review comments, consider whether this ordering is the final intended design. The order directly affects user experience in the autofill UI. If this is intentional and final, the current implementation is good. If there's uncertainty, consider:

• Adding a comment explaining the UX reasoning for "Passkeys first"
• Or making the order configurable if requirements may change

Reference: Review comment from matt-livefront notes this reversal at CredentialProviderContextTests.swift

---

⚠️ [Code Quality] Workaround Comment Could Be More Actionable

Location: BitwardenShared/Core/Autofill/Utilities/CredentialProviderContext/CredentialProviderContext.swift:96-103

Issue: The WORKAROUND comment explains the iOS bug well, but the team discussion in review comments suggested keeping this as a permanent fallback rather than tracking removal.

Current comment:

// WORKAROUND: iOS does not consistently send `serviceIdentifiers` in the Fido2 + Passwords
// vault list flow (.autofillFido2VaultList). As a fallback, we use the `relyingPartyIdentifier`
// as the URI for filtering, which provides similar functionality.
//
// This fallback should be retained even if Apple fixes the primary issue...

Suggestion: The comment is excellent and comprehensive. The team decision from review comments (fedemkr, KatherineInCode) is to keep this permanently as a defensive fallback. The current comment correctly reflects this decision. No change needed, but wanted to highlight this as good documentation of a platform workaround.

---

⚠️ [Testing] Missing Edge Case Test for httpsNormalized()

Location: BitwardenKit/Core/Platform/Extensions/StringTests.swift

Issue: While the new httpsNormalized() method has basic test coverage, it's missing an edge case test for URLs that already have http:// (non-secure) with a trailing slash.

Suggestion: Add test case:

func test_httpsNormalized_httpWithTrailingSlash() {
    let result = "http://example.com/".httpsNormalized()
    XCTAssertEqual(result, "http://example.com")
}

This ensures the trailing slash removal works correctly for both http and https schemes.

---

⚠️ [Code Style] Implicit Return Style Inconsistency

Location: BitwardenShared/Core/Vault/Helpers/VaultListSectionsBuilder.swift:128-132

Issue: The code uses Swift 5.9's implicit return for if expressions, which is modern and correct. However, within the same function at line 119, there's direct property access without if wrapping:

// Line 119 - direct access
if !preparedData.fido2Items.isEmpty, let rpID {

// Line 128 - expression-based if
let passwordsSectionName = if let rpID {
    Localizations.passwordsForX(rpID)
} else {
    Localizations.passwords
}

Suggestion: This is actually fine and idiomatic Swift - using if expressions where they improve readability and clarity. The current code is good as-is. This is just noting the pattern for consistency across the codebase.

---

⚠️ [Performance] Consider Documenting Batch Processing Performance Gains

Location: BitwardenShared/Core/Vault/Helpers/VaultListDataPreparator.swift:132-175

Issue: The new prepareAutofillCombinedMultipleData() method uses decryptAndProcessCiphersInBatch() for performance, but there's no documentation quantifying the improvement or explaining why batching helps.

Suggestion: Add a brief comment explaining the performance benefit:

/// Prepares autofill's data on passwords + Fido2 combined in multiple sections for the vault list builder.
///
/// This method uses batch decryption to improve performance and reduce memory usage compared to
/// decrypting all ciphers at once, especially beneficial for large vaults.
///
/// - Parameters:
///   - ciphers: An array of `Cipher` objects to be processed.
///   - filter: A `VaultListFilter` object that defines the filtering criteria for the vault list.
///   - withFido2Credentials: Available Fido2 credentials to build the vault list section.
/// - Returns: An optional `VaultListPreparedData` object containing the prepared data for the vault list.
/// Returns `nil` if the vault is empty.

---

⚠️ [Code Quality] Potential Duplicate Cipher in CombinedMultiple Flow

Location: BitwardenShared/Core/Vault/Helpers/VaultListDataPreparator.swift:160-171

Issue: A cipher with Fido2 credentials that also qualifies for basic login autofill will be added twice:

1. Line 163: Added to fido2Items if it has Fido2 and is in available credentials
2. Line 167-170: Added to groupItems if canBeUsedInBasicLoginAutofill is true

Current behavior: A login item with both Fido2 credentials and password will appear in BOTH sections (Passkeys section AND Passwords section).

Question: Is this intentional? The single section flow (prepareAutofillCombinedSingleData) explicitly prevents duplicates by using guard/return (lines 204-210).

Suggestion: If duplicates are intentional (showing the same item in both sections), add a comment explaining why:

// Note: A cipher may appear in both sections if it has Fido2 credentials AND basic login fields.
// This is intentional to provide users multiple ways to access the same credential.

If duplicates are NOT intentional, consider adding similar guard logic as the single section flow.

Update from commit history: Commit 53cfd33bf mentions "Fixed combined single section data preparator to add each cipher once", suggesting duplicates may have been a previous issue. Please verify if the multiple sections flow intentionally allows duplicates or if this needs the same fix.

---

Good Practices Observed

✅ Comprehensive test coverage for all new components (director strategy, data preparator, sections builder)
✅ Proper use of protocol-based dependency injection throughout
✅ Excellent documentation on the iOS service identifiers workaround
✅ Security-conscious filtering with passesRestrictItemTypesPolicy checks
✅ Proper use of batch decryption pattern for performance
✅ Well-structured separation of concerns (strategy, preparator, builder)
✅ Consistent error handling and nil-safety throughout
✅ Good use of Swift modern features (if expressions, async/await)
✅ Test files properly co-located with implementation
✅ Comprehensive mock setup in tests for isolated testing

---

Action Items

1. [Optional] Verify and document the section ordering decision (Passkeys first vs Passwords first) - see review comment discussion
2. [Recommended] Add edge case test for httpsNormalized() with http:// + trailing slash
3. [Recommended] Clarify whether ciphers appearing in both sections (Fido2 AND Passwords) is intentional behavior in prepareAutofillCombinedMultipleData()
4. [Optional] Add performance documentation to batch processing methods explaining memory/speed benefits

---

Overall Assessment

This is a well-executed performance optimization that follows established architectural patterns. The code is clean, well-tested, and maintains security best practices. The handling of the iOS service identifiers bug is particularly well-documented and defensively coded.

The main question to resolve is whether the duplicate cipher behavior in the combined multiple flow is intentional (see improvement #6 above).

Recommendation: Approve after clarifying the duplicate cipher question and considering the optional test additions.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 1b4258b6-5619-4927-909f-34d75a40215c

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

[codecov]

Codecov Report

❌ Patch coverage is 99.70356% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.04%. Comparing base (2e4b325) to head (a784a4d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ared/Core/Platform/Services/ServiceContainer.swift	0.00%	1 Missing ⚠️
...llExtension/CredentialProviderViewController.swift	0.00%	1 Missing ⚠️
...ialProviderContext/CredentialProviderContext.swift	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2091      +/-   ##
==========================================
- Coverage   85.28%   83.04%   -2.25%     
==========================================
  Files        1695     1975     +280     
  Lines      144628   161630   +17002     
==========================================
+ Hits       123344   134219   +10875     
- Misses      21284    27411    +6127     

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

[fedemkr]

🤔 Even though Claude has a point in here about adding a ticket TODO to clear this; I may be inclined to leave this code there in case Apple fixes the issue and then breaks it again in a future release. It seems like a good fallback when the uri is not provided from the service identifiers. What do you think? @KatherineInCode @matt-livefront

[KatherineInCode]

I think it's fine not having an explicit TODO for this sort of thing. I feel like we've had "check if this is still broken if we're on a future version of iOS" comments before 🤔