PM-27756: Create common ExitManager #6117

david-livefront · 2025-11-03T21:00:44Z

🎟️ Tracking

PM-27756

📔 Objective

This PR moves the ExitManager to the UI module, making it common code.

⏰ 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 · 2025-11-03T21:01:55Z

Claude finished @david-livefront's task —— View job

PR Review: PM-27756 - Create common ExitManager

This PR successfully refactors ExitManager into shared code in the :ui module, eliminating duplication between the Password Manager and Authenticator apps. The changes follow established architectural patterns and improve code quality.

Summary

Overall Assessment: ✅ Approved - This is a well-executed refactoring that improves code organization and maintainability.

Key Strengths:

Follows established module organization patterns (moving to :ui module alongside IntentManager and QrCodeAnalyzer)
Removes code duplication between apps (deleted 28 lines of duplicate code)
Improves test coverage by removing @OmitFromCoverage and adding comprehensive unit tests
Maintains consistent behavior across both applications
All imports and references updated correctly

Changes:

✅ Moved ExitManager interface from app → ui module (app:ui/src/main/kotlin/com/bitwarden/ui/platform/manager/exit/ExitManager.kt:1)
✅ Moved ExitManagerImpl from app → ui module with improved implementation (app:ui/src/main/kotlin/com/bitwarden/ui/platform/manager/exit/ExitManagerImpl.kt:1)
✅ Added LocalExitManager to common UI providers (app:ui/src/main/kotlin/com/bitwarden/ui/platform/composition/LocalProviders.kt:12)
✅ Deleted duplicate authenticator implementations (2 files, 28 lines)
✅ Added unit test for ExitManagerImpl (app:ui/src/test/kotlin/com/bitwarden/ui/platform/manager/exit/ExitManagerTest.kt:1)
✅ Updated all imports across 10 files in both apps

Code Quality & Architecture

Architectural Compliance: ✅

Correctly places manager in :ui module per docs/ARCHITECTURE.md guidelines
Follows established pattern for UI-layer managers exposed via CompositionLocal
Maintains proper module dependencies and separation of concerns

Code Style: ✅

Follows Kotlin coding conventions
Proper KDoc documentation on all public APIs
Consistent with existing code patterns

Testing: ✅

Added comprehensive unit test covering exitApplication() behavior
Properly uses MockK for mocking Activity
Test verifies finishAndRemoveTask() is called (correct Android lifecycle method)

Specific Findings

Test Coverage Note:
The Codecov report shows 50% patch coverage with 1 missing line. This is the error() call in LocalExitManager (app:ui/src/main/kotlin/com/bitwarden/ui/platform/composition/LocalProviders.kt:13), which is expected and acceptable. This error only triggers when the CompositionLocal is accessed without being provided—a programming error caught during development, not a runtime scenario requiring test coverage. Similar patterns exist for LocalIntentManager and LocalQrCodeAnalyzer.

Implementation Improvement:
The common implementation correctly uses activity.finishAndRemoveTask() instead of just activity.finish(), which properly removes the task from the recents screen for a cleaner exit experience. This matches the app module's original implementation.

Recommendations

No blocking issues found. The PR is ready to merge.

Optional Future Enhancement:
Consider documenting in docs/ARCHITECTURE.md that UI-layer managers like ExitManager, IntentManager, and QrCodeAnalyzer should be placed in the :ui module when they're shared between applications. This would help guide future similar refactorings.

claude · 2025-11-03T21:04:00Z

ui/src/test/kotlin/com/bitwarden/ui/platform/manager/exit/ExitManagerTest.kt

+            activity.finishAndRemoveTask()
+        }
+    }
+}


👍 Great addition! This test provides coverage for the ExitManagerImpl that was previously annotated with @OmitFromCoverage. The test correctly verifies that exitApplication() calls activity.finishAndRemoveTask().

claude · 2025-11-03T21:04:01Z

ui/src/main/kotlin/com/bitwarden/ui/platform/composition/LocalProviders.kt

+/**
+ * Provides access to the exit manager throughout the app.
+ */
+val LocalExitManager: ProvidableCompositionLocal<ExitManager> = compositionLocalOf {


👍 Good location for the common LocalExitManager provider. This follows the established pattern of other shared CompositionLocals like LocalIntentManager and LocalQrCodeAnalyzer, making it easily accessible to both the Password Manager and Authenticator apps.

claude · 2025-11-03T21:04:11Z

ui/src/main/kotlin/com/bitwarden/ui/platform/manager/exit/ExitManagerImpl.kt

 * The default implementation of the [ExitManager] for managing the various ways to exit the app.
 */
-@OmitFromCoverage
 class ExitManagerImpl(


👍 Excellent improvement! The @OmitFromCoverage annotation was removed from the original app module implementation, and proper unit tests were added in ExitManagerTest.kt. This increases code coverage and confidence in the implementation.

claude · 2025-11-03T21:04:13Z

app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt

 import com.bitwarden.cxf.ui.composition.LocalCredentialExchangeRequestValidator
 import com.bitwarden.cxf.validator.CredentialExchangeRequestValidator
 import com.bitwarden.cxf.validator.dsl.credentialExchangeRequestValidator
+import com.bitwarden.ui.platform.composition.LocalExitManager


👍 Clean refactoring. The imports have been correctly updated to use the common ExitManager from the :ui module instead of the app-specific implementation.

claude · 2025-11-03T21:04:19Z

.../src/main/kotlin/com/bitwarden/authenticator/ui/platform/composition/LocalManagerProvider.kt

 import com.bitwarden.authenticator.ui.platform.manager.permissions.PermissionsManager
 import com.bitwarden.authenticator.ui.platform.manager.permissions.PermissionsManagerImpl
 import com.bitwarden.core.data.manager.BuildInfoManager
+import com.bitwarden.ui.platform.composition.LocalExitManager


👍 Excellent consolidation! The Authenticator app now properly imports the common LocalExitManager from the :ui module, eliminating code duplication between the two apps.

github-actions · 2025-11-03T21:08:11Z

Checkmarx One – Scan Summary & Details – 01c4d23f-c06e-4293-9527-85c21b43d353

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

codecov · 2025-11-03T21:31:36Z

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.90%. Comparing base (0959284) to head (3f07b6e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...itwarden/ui/platform/composition/LocalProviders.kt	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6117   +/-   ##
=======================================
  Coverage   84.89%   84.90%           
=======================================
  Files         724      724           
  Lines       52778    52780    +2     
  Branches     7672     7672           
=======================================
+ Hits        44807    44811    +4     
+ Misses       5279     5277    -2     
  Partials     2692     2692

☔ 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 · 2025-11-03T22:27:07Z

ui/src/main/kotlin/com/bitwarden/ui/platform/composition/LocalProviders.kt

+ * Provides access to the exit manager throughout the app.
+ */
+val LocalExitManager: ProvidableCompositionLocal<ExitManager> = compositionLocalOf {
+    error("CompositionLocal ExitManager not present")


📝 The missing test coverage reported by Codecov (line 13, the error() call) is expected and acceptable. This error block is only executed when LocalExitManager is accessed without being provided via CompositionLocalProvider, which represents a programming error that would be caught during development, not a runtime scenario that needs test coverage.

claude · 2025-11-03T22:27:22Z

ui/src/test/kotlin/com/bitwarden/ui/platform/manager/exit/ExitManagerTest.kt

+            activity.finishAndRemoveTask()
+        }
+    }
+}


👍 Excellent test implementation! This test properly verifies that exitApplication() calls activity.finishAndRemoveTask(), providing coverage for the previously untested ExitManagerImpl. The use of MockK with proper verification ensures the correct Android lifecycle method is invoked.

claude · 2025-11-03T22:27:28Z

app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/composition/LocalManagerProvider.kt

 import com.bitwarden.cxf.ui.composition.LocalCredentialExchangeRequestValidator
 import com.bitwarden.cxf.validator.CredentialExchangeRequestValidator
 import com.bitwarden.cxf.validator.dsl.credentialExchangeRequestValidator
+import com.bitwarden.ui.platform.composition.LocalExitManager


👍 Perfect consolidation! The imports have been correctly updated to reference the common LocalExitManager from the :ui module, eliminating code duplication. This follows the established pattern of other shared CompositionLocals like LocalIntentManager and LocalQrCodeAnalyzer.

claude · 2025-11-03T22:27:35Z

.../src/main/kotlin/com/bitwarden/authenticator/ui/platform/composition/LocalManagerProvider.kt

 import com.bitwarden.authenticator.ui.platform.manager.permissions.PermissionsManager
 import com.bitwarden.authenticator.ui.platform.manager.permissions.PermissionsManagerImpl
 import com.bitwarden.core.data.manager.BuildInfoManager
+import com.bitwarden.ui.platform.composition.LocalExitManager


👍 Great code reuse! The Authenticator app now properly imports the common LocalExitManager from the :ui module instead of maintaining its own duplicate implementation. This eliminates the duplicate ExitManager.kt and ExitManagerImpl.kt files that were previously in the authenticator module, reducing maintenance burden and ensuring consistent behavior across both apps.

Problem: PR #6117 review produced ~800 tokens of excessive praise and unnecessary sections when no issues were found. Expected output should have been ~30 tokens: "APPROVE - Clean refactoring". Changes: - SKILL.md: Add "Clean PRs" section with explicit 2-3 line format - SKILL.md: Make "minimal reviews" first core principle - review-psychology.md: Elevate brevity guidance to first directive - review-outputs.md: Add Example 7 showing PR #6117 as anti-pattern - refactoring.md: Replace verbose example with clean vs issues examples - Add IMPLEMENTATION_PLAN.md documenting rationale and approach Expected Impact: - 70-90% token reduction for clean PRs (800 → 30-100 tokens) - Maintains comprehensive reviews for PRs with actual issues - Reduces noise in PR conversations - Faster approvals for good work 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Implements suggestions from PR #6121 review: 1. SKILL.md: Consolidate overlapping brevity principles - Merged 4 principles about brevity into 2 clearer ones - Reduced redundancy while preserving all guidance - Added forward reference to Special Case section 2. review-outputs.md: Add context note to Example 7 - Explains why example was added (PR #6117 verbosity) - Provides historical context for readers - References anti-pattern section below Changes reduce repetition and improve documentation clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

david-livefront · 2025-11-04T18:31:54Z

Thanks @SaintPatrck

david-livefront requested review from a team and brian-livefront as code owners November 3, 2025 21:00

claude bot reviewed Nov 3, 2025

View reviewed changes

PM-27756: Create common ExitManager

3f07b6e

david-livefront force-pushed the PM-27756-common-exit-manager branch from b56cb8d to 3f07b6e Compare November 3, 2025 22:24

claude bot reviewed Nov 3, 2025

View reviewed changes

SaintPatrck mentioned this pull request Nov 4, 2025

Reduce verbosity in reviewing-changes skill for clean PRs #6121

Merged

david-livefront enabled auto-merge November 4, 2025 17:50

SaintPatrck approved these changes Nov 4, 2025

View reviewed changes

david-livefront added this pull request to the merge queue Nov 4, 2025

Merged via the queue into main with commit 4b7fcdb Nov 4, 2025
19 of 29 checks passed

david-livefront deleted the PM-27756-common-exit-manager branch November 4, 2025 18:45

PM-27756: Create common ExitManager #6117

PM-27756: Create common ExitManager #6117

Uh oh!

Conversation

david-livefront commented Nov 3, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

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

Uh oh!

PR Review: PM-27756 - Create common ExitManager

Summary

Code Quality & Architecture

Specific Findings

Recommendations

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 3, 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!

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

Uh oh!

Codecov Report

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

david-livefront commented Nov 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

david-livefront commented Nov 3, 2025 •

edited by atlassian bot

Loading

claude bot commented Nov 3, 2025 •

edited

Loading

github-actions bot commented Nov 3, 2025 •

edited

Loading

codecov bot commented Nov 3, 2025 •

edited

Loading