[PM-26061] Consolidate Debug Menu to BitwardenKit

[KatherineInCode]

🎟️ Tracking

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

📔 Objective

This consolidates the Debug Menu and related objects to BitwardenKit, as part of a broader effort to consolidate things to BitwardenKit.

I had to also bring in our safeIndex array extension as part of this, but I also think we should expand usage of it (particularly in tests).

I also split out the "Generate SDK Error" from "Generate Error", to provide more granular control. As well, I changed which kind of error is generated.

The close button of the Debug menu has also been updated to match the rest of the app.

📸 Screenshots

⏰ 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 – fda06e92-eb93-44e2-b778-f83a2e3fbcdb

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

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.12%. Comparing base (8739967) to head (33fd0bd).

Files with missing lines	Patch %	Lines
...hared/UI/Platform/Application/AppCoordinator.swift	12.50%	7 Missing ⚠️
GlobalTestHelpers-bwa/MockAppModule.swift	0.00%	2 Missing ⚠️
...Kit/UI/Platform/DebugMenu/DebugMenuProcessor.swift	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2177      +/-   ##
==========================================
- Coverage   85.42%   84.12%   -1.30%     
==========================================
  Files        1733     1973     +240     
  Lines      146246   161418   +15172     
==========================================
+ Hits       124926   135800   +10874     
- Misses      21320    25618    +4298     

☔ 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 reviewed this PR for architecture compliance, code style, testing guidelines, and security considerations. Overall, this is a solid consolidation effort that successfully moves the Debug Menu to BitwardenKit. Here are my findings:

---

🎯 Summary

This PR successfully consolidates the Debug Menu from AuthenticatorShared to BitwardenKit, making it accessible to both apps. The consolidation is well-executed with proper architecture patterns, good test coverage, and appropriate security considerations for debug-only functionality.

---

✅ Strengths

1. Architecture Compliance ✓

   • Follows the established coordinator/processor/view pattern correctly
   • Uses dependency injection through ServiceContainer properly
   • Module pattern implemented correctly in both apps' AppModule extensions
   • Proper delegate pattern for coordinator communication (BitwardenKit/UI/Platform/DebugMenu/DebugMenuCoordinator.swift:5-9)

2. Code Organization ✓

   • Files properly moved to BitwardenKit/UI/Platform/ structure
   • Tests co-located with implementation files
   • Proper target membership updates in project files

3. Testing ✓

   • Good test coverage maintained (90.47% patch coverage per Codecov)
   • Tests properly use mocks through ServiceContainer.withMocks()
   • Tests follow naming conventions and are well-structured
   • Good use of safeIndex in tests (BitwardenKit/UI/Platform/DebugMenu/DebugMenuProcessorTests.swift:98, 116)

4. Feature Enhancement ✓

   • Splitting "Generate SDK Error" from "Generate Error" provides better granularity
   • Proper localization added for new button (BitwardenResources/Localizations/en.lproj/Localizable.strings)
   • Close button updated to match app-wide patterns using closeToolbarButton

5. Security ✓

   • Debug menu properly protected by DEBUG_MENU compilation flag
   • Shake gesture detection only active in debug builds (BitwardenKit/UI/Platform/Application/Views/ShakeWindow.swift:52-56)
   • Error generation uses safe test data without exposing sensitive information

---

📝 Observations & Suggestions

1. Code Coverage Gaps (Minor)

Codecov reports 10 lines missing coverage. The main gaps appear to be:

• AuthenticatorShared/UI/Platform/Application/AppCoordinator.swift:170-186 - The showDebugMenu() method lacks test coverage

Suggestion: Consider adding a test to AppCoordinatorTests to verify the debug menu presentation logic, especially the isShowingDebugMenu flag behavior:

func test_showDebugMenu_presentsDebugMenu() {
    subject.navigate(to: .debugMenu)
    XCTAssertTrue(subject.isShowingDebugMenu)
    XCTAssertTrue(module.makeDebugMenuCoordinatorCalled)
}

2. Array Extension Documentation (Very Minor)

The safeIndex extension (BitwardenKit/Core/Platform/Extensions/Array+Extensions.swift:1-22) is well-documented and useful. Great addition!

Suggestion from PR description: The author mentions wanting to expand usage of safeIndex in tests. This is a good idea - consider gradually replacing manual bounds checking with this safer pattern across the codebase.

3. SDK Error Type Change (Question)

The PR changes which error type is generated for SDK errors (BitwardenKit/UI/Platform/DebugMenu/DebugMenuProcessor.swift:74-76):

services.errorReporter.log(error: BitwardenSdk.BitwardenError.Api(ApiError.ResponseContent(
    message: "Generated SDK error report from debug view.",
)))

Question: What was the previous SDK error type, and why was this specific error type chosen? This change isn't problematic, but understanding the rationale would be helpful for documentation purposes.

4. Access Control Consistency (Very Minor)

The DebugMenuView is marked public (BitwardenKit/UI/Platform/DebugMenu/DebugMenuView.swift:8), but DebugMenuProcessor is internal (default). This is likely intentional since the processor shouldn't be created directly outside the coordinator.

Note: This is correct - just confirming the access control is intentional and appropriate.

---

🔒 Security Considerations

✅ All security requirements met:

1. Zero-knowledge architecture - Debug menu doesn't access or expose any encrypted user data
2. Proper compilation flags - Debug functionality properly gated behind DEBUG_MENU flag
3. Input validation - Not applicable (debug menu doesn't accept user input for processing)
4. Secure storage - Debug menu doesn't persist any sensitive data
5. Threat model - Debug functionality only available in debug builds, cannot be enabled in production

The debug menu is appropriately secured for its purpose.

---

📊 Code Style Compliance

✅ Follows Bitwarden Swift style guidelines:

• Proper DocC documentation on all public types and methods
• Protocol implementations don't duplicate documentation (correctly)
• Proper use of MARK comments for organization
• Consistent naming conventions throughout
• Proper use of guard clauses and Swift best practices
• Extensions used appropriately for module conformance

---

🧪 Testing Guidelines Compliance

✅ Follows testing guidelines:

• Unit tests for processors cover actions, effects, and state mutations
• Tests properly named with test_<function>_<behavior> pattern
• Tests use proper setup/teardown
• Mocks generated and used correctly via Sourcery
• Tests co-located with implementation
• Snapshot tests updated with new button (visible in screenshot)

---

🎨 Recommendations Summary

Priority: Low - No blocking issues found

1. Consider: Add test coverage for AppCoordinator.showDebugMenu() to reach closer to 100% coverage
2. Consider: Document the rationale for the SDK error type change in commit history
3. Future: Gradually expand safeIndex usage in tests as mentioned in PR description

---

✅ Final Verdict

APPROVED - This PR is ready to merge.

The consolidation is well-executed with:

• ✅ Proper architecture compliance
• ✅ Good code style adherence
• ✅ Appropriate testing coverage
• ✅ Security considerations properly addressed
• ✅ Clear feature enhancement with SDK error separation

The minor coverage gap is non-critical and the overall quality is high. Great work on the consolidation! 🎉

---

CI Status: All checks passing ✓

• Lint workflow: ✅
• Test Authenticator: ✅
• Test Password Manager: ✅
• Scan: ✅
• Code Review: 🔄 (in progress - this review)

[fedemkr]

Looks good, just one small question.

[fedemkr]

🤔 I'm wondering if the 464161978EAC5FCE changes with new releases of the SDK. If so, we should make this more flexible or the test will fail every time the SDK gets updated.

[KatherineInCode]

That's a very good question, I don't actually know 🤔 I'll dig into it, see where that's coming from and how I can update the snapshot test to handle it better.