[PM-26060] Consolidate TabNavigator to BitwardenKit

[KatherineInCode]

🎟️ Tracking

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

📔 Objective

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

Two things of note on this one:

1. The TabNavigatorTests were not actually testing TabNavigator (which is just a protocol), but were instead testing the BitwardenTabBarController as a specific implementation of the protocol. I've shifted those tests to more clearly be with the object under test.

2. BWA did not yet have BitwardenTabBarController, but instead relied on an extension on UITabBarController. I've shifted it to use a copy of BitwardenTabBarController, in the hopes that it could also be consolidated eventually. I did break that out into PM-27853, as it would require thinking more about how we handle tabs (TabRoute being application-specific), and that seemed like it was definitely outside of the scope of this ticket, especially since the scope has already been expanded so much, relatively speaking.

⏰ 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 – 067472cc-4242-4c4f-aa02-23a7434403ad

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

[codecov]

Codecov Report

❌ Patch coverage is 31.91489% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.67%. Comparing base (e030fbf) to head (7a4cafc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../Application/Views/BitwardenTabBarController.swift	0.00%	27 Missing ⚠️
...Application/Utilities/Mocks/MockTabNavigator.swift	75.00%	4 Missing ⚠️
...hared/UI/Platform/Application/AppCoordinator.swift	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2105      +/-   ##
==========================================
- Coverage   85.23%   83.67%   -1.56%     
==========================================
  Files        1709     1978     +269     
  Lines      145501   161317   +15816     
==========================================
+ Hits       124019   134989   +10970     
- Misses      21482    26328    +4846     

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

[claude]

Claude finished @KatherineInCode's task —— View job

---

Code Review Complete

I've completed a comprehensive review of this PR that consolidates TabNavigator and related components to BitwardenKit. Here are my findings:

---

Summary of Changes

This PR successfully moves shared tab navigation infrastructure from BitwardenShared/AuthenticatorShared to BitwardenKit:

• Moved to BitwardenKit: TabNavigator protocol, TabRepresentable protocol, MockTabNavigator
• Added: BitwardenTabBarController implementation for AuthenticatorShared (previously used UITabBarController extension)
• Fixed: Typo in BitwardenShared's BitwardenTabBarController DocC comment ("TabBavigator" → "TabNavigator")
• Renamed: Test file from TabNavigatorTests to BitwardenTabBarControllerTests to match actual subject under test
• Updated: Import statements to reference BitwardenKit for tab-related types

---

Critical Issues

⚠️ Test Coverage Gap - AuthenticatorShared

Location: AuthenticatorShared/UI/Platform/Application/Views/BitwardenTabBarController.swift

The new BitwardenTabBarController implementation for AuthenticatorShared has 0% test coverage (27 lines missing coverage per Codecov). This violates the project's testing philosophy that "every type containing logic must be tested."

Details

The AuthenticatorShared version is a duplicate of the BitwardenShared implementation but lacks corresponding tests. While BitwardenShared has comprehensive tests (BitwardenTabBarControllerTests.swift), AuthenticatorShared does not.

Recommendation: Add test file AuthenticatorShared/UI/Platform/Application/Views/BitwardenTabBarControllerTests.swift covering:

• setNavigators(_:) correctly sets view controllers in sorted order
• Tab bar items are configured with correct title, image, and selectedImage
• navigator(for:) returns correct navigator for tab index
• traitCollectionDidChange(_:) refreshes tab icons on light/dark mode changes
• rootViewController returns self

You can adapt the existing tests from BitwardenShared's implementation, adjusting for AuthenticatorShared's TabRoute enum.

---

Suggested Improvements

🎨 Potential Type Constraint Issue in setNavigators

Location:

• BitwardenShared/UI/Platform/Application/Views/BitwardenTabBarController.swift:28
• AuthenticatorShared/UI/Platform/Application/Views/BitwardenTabBarController.swift:28

func setNavigators<Tab: Hashable & TabRepresentable>(_ tabs: [Tab: Navigator]) {
    tabsAndNavigators = tabs as? [TabRoute: Navigator] ?? [:]
    // ...
}

The downcast tabs as? [TabRoute: Navigator] will fail silently when Tab is not TabRoute, falling back to an empty dictionary. This means traitCollectionDidChange won't be able to refresh tabs properly for non-TabRoute tab types.

Analysis & Recommendation

Current behavior: If you call setNavigators with a different Tab type (like the TestRoute in tests), the cast fails and tabsAndNavigators becomes empty. Subsequently, traitCollectionDidChange has nothing to refresh.

Impact:

• Tests using TestRoute won't properly validate the trait collection behavior
• Future code using different tab types won't get proper dark mode refresh

Options:

1. Generic storage (preferred if other tab types are expected):

private var tabsAndNavigatorsStorage: [AnyHashable: any Navigator] = [:]

func setNavigators<Tab: Hashable & TabRepresentable>(_ tabs: [Tab: Navigator]) {
    tabsAndNavigatorsStorage = Dictionary(uniqueKeysWithValues: tabs.map { ($0.key as AnyHashable, $0.value) })
    
    viewControllers = tabs
        .sorted { $0.key.index < $1.key.index }
        .compactMap { /* ... */ }
}

override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
    super.traitCollectionDidChange(previousTraitCollection)
    
    if traitCollection.hasDifferentColorAppearance(comparedTo: previousTraitCollection) {
        // Reconstruct typed dictionary from storage
        let tabs = Dictionary(uniqueKeysWithValues: tabsAndNavigatorsStorage.compactMap { key, value in
            guard let typedKey = key.base as? Tab else { return nil }
            return (typedKey, value)
        })
        setNavigators(tabs)
    }
}

2. Constrain to TabRoute (simpler if only TabRoute is used):

// Change protocol conformance to be TabRoute-specific
class BitwardenTabBarController: UITabBarController {
    private var tabsAndNavigators: [TabRoute: any Navigator] = [:]
    
    func setNavigators(_ tabs: [TabRoute: Navigator]) {
        tabsAndNavigators = tabs
        // ... rest of implementation
    }
}

// Extend to conform to TabNavigator for any generic Tab type
extension BitwardenTabBarController: TabNavigator {
    func setNavigators<Tab: Hashable & TabRepresentable>(_ tabs: [Tab: Navigator]) {
        guard let tabRoutes = tabs as? [TabRoute: Navigator] else {
            assertionFailure("BitwardenTabBarController only supports TabRoute")
            return
        }
        setNavigators(tabRoutes)
    }
    
    func navigator<Tab: TabRepresentable>(for tab: Tab) -> Navigator? {
        viewControllers?[tab.index] as? Navigator
    }
}

Given that both BitwardenShared and AuthenticatorShared have their own TabRoute enums, option 2 is likely more appropriate and would make the constraint explicit.

---

🤔 MockTabNavigator Placement

Location: BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockTabNavigator.swift

The mock is now in BitwardenKit but imports itself (import BitwardenKit). While this works, it's unconventional for a mock to be in the same module as the protocol it mocks.

Discussion

Current state: MockTabNavigator lives in BitwardenKit alongside TabNavigator.

Considerations:

• Mocks typically go in separate test-helper targets (BitwardenKitMocks, GlobalTestHelpers)
• The architecture doc mentions BitwardenKitMocks as a test helper target
• However, MockTabNavigator has public visibility and is used by GlobalTestHelpers/MockAppModule.swift

Recommendation:

• If BitwardenKitMocks exists and is designed for this purpose, consider moving MockTabNavigator there
• Otherwise, current placement is acceptable given it's already marked public and used across test targets
• The self-import (import BitwardenKit) is fine—it just makes the module boundary explicit

This is a low-priority organizational question, not a functional issue.

---

📝 Inconsistent Access Modifiers in MockTabNavigator

Location: BitwardenKit/UI/Platform/Application/Utilities/Mocks/MockTabNavigator.swift:22-27

The present(_:animated:overFullscreen:onCompletion:) method has public visibility but is an empty stub implementation. While this satisfies the Navigator protocol conformance, it's unclear if this is intentional test behavior or incomplete.

Details

public func present(
    _ viewController: UIViewController,
    animated: Bool,
    overFullscreen: Bool,
    onCompletion: (() -> Void)?,
) {}

Observations:

• The original GlobalTestHelpers/MockTabNavigator.swift had the same empty implementation
• No test verification properties (like presentCalled or presentedViewController) are provided
• Tests using this mock don't appear to verify presentation behavior

Recommendation:

• If presentation is never tested on TabNavigator, this is fine as-is
• If presentation testing is needed in the future, add tracking properties:

public var presentCalled = false
public var presentedViewController: UIViewController?
public var presentAnimated: Bool?
public var presentOverFullscreen: Bool?
public var presentCompletion: (() -> Void)?

This is a minor observation—current implementation matches the original.

---

Good Practices Observed

• ✅ Proper module consolidation following the repository's BitwardenKit migration effort
• ✅ Fixed typo in DocC comment ("TabBavigator" → "TabNavigator")
• ✅ Correctly renamed test file to match actual subject under test
• ✅ Maintained existing test coverage for BitwardenShared's BitwardenTabBarController
• ✅ Added necessary import BitwardenKit statements to maintain module boundaries
• ✅ Preserved DocC documentation throughout the move
• ✅ Both BitwardenTabBarController implementations handle dynamic appearance (light/dark mode) changes

---

Action Items

Before merging:

1. ⚠️ High Priority: Add comprehensive tests for AuthenticatorShared/UI/Platform/Application/Views/BitwardenTabBarController.swift to address the 0% coverage reported by Codecov

Consider for future PRs:
2. 🎨 Medium Priority: Evaluate the type constraint issue in setNavigators and either make it generic-friendly or explicitly constrain to TabRoute
3. 📝 Low Priority: Consider whether MockTabNavigator should move to BitwardenKitMocks for consistency with architecture guidelines

---

[fedemkr]

Looks good, just a minor question.