[PM-33982] feat: Add device management screen#6754
Conversation
aj-rosado
added
the
innovation
|
Great job! No new security vulnerabilities introduced in this pull request |
# Conflicts: # app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlockednavbar/VaultUnlockedNavBarNavigation.kt # app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlockednavbar/VaultUnlockedNavBarScreen.kt # app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlockednavbar/VaultUnlockedNavBarScreenTest.kt # core/src/main/kotlin/com/bitwarden/core/data/manager/model/FlagKey.kt # ui/src/main/kotlin/com/bitwarden/ui/platform/components/debug/FeatureFlagListItems.kt # ui/src/main/res/values/strings.xml
github-actions
Bot
added
app:password-manager
aj-rosado
added
the
ai-review-vnext
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6754 +/- ##
==========================================
- Coverage 85.80% 85.74% -0.07%
==========================================
Files 835 845 +10
Lines 59391 60027 +636
Branches 8668 8734 +66
==========================================
+ Hits 50963 51471 +508
- Misses 5441 5546 +105
- Partials 2987 3010 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| daysAgo < 7 -> BitwardenString.active_this_week | ||
| daysAgo < 14 -> BitwardenString.active_last_week | ||
| daysAgo < 30 -> BitwardenString.active_this_month | ||
| else -> BitwardenString.active_over_thirty_days_ago |
There was a problem hiding this comment.
So this is all based on number of days in a week/month. Should it be based on the day of the week?
If today is Monday, 3 days ago is last week, not this week, right?
There was a problem hiding this comment.
I have mirrored the other clients logic but that makes sense to me 🤔
There was a problem hiding this comment.
We have updated the texts so that it is clearer 🙌
There was a problem hiding this comment.
The language does match the functionality now. But don't we want it to say this week, not last seven days?
There was a problem hiding this comment.
It was decided to keep it simpler as this change will affect all other clients. So we are keeping the logic and adapting the texts.
| * requiring rounding (unlike the JavaScript equivalent). | ||
| */ | ||
| @Suppress("MagicNumber") | ||
| fun Instant?.toLastActivityLabel(clock: Clock, resourceManager: ResourceManager): String? { |
There was a problem hiding this comment.
Since this is a UI layer component, can we just return a Text?
| 24 -> DeviceTypeEntry(BitwardenString.cli, "MacOs") | ||
| 25 -> DeviceTypeEntry(BitwardenString.cli, "Linux") | ||
| 26 -> DeviceTypeEntry(BitwardenString.extension, "DuckDuckGo") | ||
| else -> return resourceManager.getString(BitwardenString.unknown_device) |
There was a problem hiding this comment.
Same thing here, can we just return a Text
| 19 -> DeviceTypeEntry(BitwardenString.extension, "Vivaldi") | ||
| 20 -> DeviceTypeEntry(BitwardenString.extension, "Safari") | ||
| 21 -> DeviceTypeEntry(BitwardenString.sdk, "") | ||
| 22 -> DeviceTypeEntry(BitwardenString.server, "") |
There was a problem hiding this comment.
Should these say something?
Also, should these platforms be translatable?
There was a problem hiding this comment.
Other clients are returning the same data.
Regarding translations, nothing is translatable on other clients 🤔
It makes sense to me to be able to translate words like "server" or "extension"
|
|
||
| ManageDevicesState.ViewState.Error -> BitwardenErrorContent( | ||
| message = stringResource( | ||
| id = BitwardenString.generic_error_message, |
There was a problem hiding this comment.
Do we have nothing more specific than this?
There was a problem hiding this comment.
We do not, mobile was based roughly on existing Pending Auth screen and Manage Devices extension screen.
Probably will see some more design work before being approved
| ) | ||
| } | ||
|
|
||
| else -> SessionItem( |
There was a problem hiding this comment.
Can we make this exhaustive
Change return type from ui extensions from String to Text
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds a Manage Devices screen that lists registered devices with their session status (current, pending auth request, or active session), gated behind the Code Review DetailsNo new high-confidence findings. Iterative review by david-livefront and prior automated reviews drove substantive improvements:
The |
| daysAgo < 30 -> BitwardenString.past_thirty_days | ||
| else -> BitwardenString.over_thirty_days_ago | ||
| } | ||
| return resourceManager.getString(resId).asText() |
There was a problem hiding this comment.
Now that you are returning a Text you do not need the ResourceManager here anymore.
| * Returns e.g. "Mobile - Android", "Extension - Chrome", "Desktop - Windows". | ||
| */ | ||
| @Suppress("CyclomaticComplexMethod", "MagicNumber") | ||
| fun Int.toReadableDeviceTypeName(resourceManager: ResourceManager): Text { |
There was a problem hiding this comment.
Same here, the ResourceManager is unnecessary
# Conflicts: # core/src/main/kotlin/com/bitwarden/core/data/manager/model/FlagKey.kt
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| val hideBottomSheet: Boolean | ||
| get() = internalHideBottomSheet && | ||
| !isFdroid && | ||
| isBuildVersionAtLeast(Build.VERSION_CODES.TIRAMISU) |
There was a problem hiding this comment.
hideBottomSheet always returns false, so the "Skip for now" button cannot dismiss the notification bottom sheet.
Details and fix
The current AND logic requires all three conditions to be true simultaneously. When isFdroid is true, the expression !isFdroid evaluates to false, making the entire result false regardless of internalHideBottomSheet. This means:
- On F-Droid with API 33+: the bottom sheet shows (asking to enable push notifications that don't work on F-Droid) and cannot be dismissed.
- On pre-TIRAMISU: same undismissable state, though the permission check in the Screen may mask this in some cases.
The fix is to use OR logic so that any single reason to hide the sheet is sufficient:
| val hideBottomSheet: Boolean | |
| get() = internalHideBottomSheet && | |
| !isFdroid && | |
| isBuildVersionAtLeast(Build.VERSION_CODES.TIRAMISU) | |
| val hideBottomSheet: Boolean | |
| get() = internalHideBottomSheet || | |
| isFdroid || | |
| !isBuildVersionAtLeast(Build.VERSION_CODES.TIRAMISU) |
The @ChecksSdkIntAtLeast annotation on this property should also be removed since the corrected logic no longer solely checks for a minimum SDK level.
| return if (entry.platform.isNotEmpty()) { | ||
| entry.categoryResId.asText() | ||
| .concat( | ||
| " - ${entry.platform}".asText(), |
There was a problem hiding this comment.
Instead of concat, can we make these format args?
| <string name="continue_without_syncing">Continue without syncing</string> | ||
| <string name="external_link">External link</string> | ||
| <string name="external_link_format" comment="Used for accessibility to indicate that tapping this item will leave the app">%1$s, External link</string> | ||
| <string name="manage_devices">Manage devices</string> |
There was a problem hiding this comment.
This managed devices string is being used in both prod code and for feature flags.
The feature flag name should be in the unlocalized file.
| 20 -> DeviceTypeEntry(BitwardenString.extension_platform, "Safari") | ||
| 21 -> DeviceTypeEntry(BitwardenString.sdk, "") | ||
| 22 -> DeviceTypeEntry(BitwardenString.server, "") | ||
| 23 -> DeviceTypeEntry(BitwardenString.cli_platform, "Windows") |
There was a problem hiding this comment.
Do we need this intermediary step?
Can we just return the correct value here?
| ) { | ||
| val filteredRequests = when (val result = action.authRequestsUpdatesResult) { | ||
| is AuthRequestsUpdatesResult.Update -> | ||
| result.authRequests.filterRespondedAndExpired(clock = clock) |
There was a problem hiding this comment.
Can we wrap this in curly braces.
| assertEquals(DeviceSessionStatus.Pending, content.items[1].status) | ||
| assertEquals(DeviceSessionStatus.None, content.items[2].status) | ||
| assertEquals(currentDevice.id, content.items[0].id) | ||
| assertEquals(pendingDevice.id, content.items[1].id) |
There was a problem hiding this comment.
You should be comparing again the entire ManageDevicesState, not individual ids and statuses
| val viewModel = createViewModel() | ||
| assertEquals( | ||
| ManageDevicesState.ViewState.Error, | ||
| viewModel.stateFlow.value.viewState, |
There was a problem hiding this comment.
You should compare the entire thing, not just the viewSTate
| timeStyle = FormatStyle.MEDIUM, | ||
| clock = fixedClock, | ||
| ) | ||
| } returns "Oct 27, 2023, 12:00:00 PM" |
There was a problem hiding this comment.
Why is this needed? The instant is fixed, so it should always return the same string
| fun `type 1 should return Mobile - iOS`() { | ||
| assertEquals( | ||
| "Mobile - iOS", | ||
| 1.readableDeviceTypeName.toString(resources), |
There was a problem hiding this comment.
None of resource mocking is needed, you can just compare the outputs:
assertEquals(
BitwardenString.mobile_platform.asText("iOS"),
1.readableDeviceTypeName,
)# Conflicts: # app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt
| val device = result.devices.first() | ||
| assertEquals("deviceId", device.id) | ||
| // identifier "deviceIdentifier" != uniqueAppId "testUniqueAppId", so not current device | ||
| assertFalse(device.isCurrentDevice) |
There was a problem hiding this comment.
We should be comparing the entire result
| val id: String, | ||
| val name: String, | ||
| val identifier: String, | ||
| val type: Int, |
There was a problem hiding this comment.
Can this type be enumerated?
| name = name, | ||
| identifier = identifier, | ||
| type = type, | ||
| type = DeviceType.entries.firstOrNull { it.value == type } ?: DeviceType.UNKNOWN, |
There was a problem hiding this comment.
Can we add the type for DeviceResponseJson too
| id = "0d31b6fb-d282-43c7-b614-b13e0129dbd7", | ||
| name = "Pixel 8", | ||
| identifier = "ea7c0a13-5ce4-4f96-8e17-4fc7fa54f464", | ||
| type = 0, |
There was a problem hiding this comment.
❌ CRITICAL: type = 0 no longer compiles after DeviceResponseJson.type was changed to DeviceType enum.
Details and fix
In commit dad449ef the type property on DeviceResponseJson was changed from Int to the new DeviceType enum, but this test fixture still passes the integer literal 0. Kotlin will not implicitly convert Int to enum, so the network module test source no longer compiles and DevicesServiceTest cannot run.
AuthRepositoryTest.kt was updated in the same commit (now uses type = DeviceType.ANDROID); this site was missed.
| type = 0, | |
| type = DeviceType.ANDROID, |
You'll also need to add import com.bitwarden.network.model.DeviceType at the top of the file.
|
Howdy @aj-rosado et. al 👋🏼 I am field testing the new multi-agent code review, I gave it a local run against the PR. The findings are below for your analysis. Please incorporate the findings that are true signals. We are also looking closely into the difference in the way that Claude Code runs the review given different models for pros, cons, false-positive rates, instructions not being followed, etc. so please don't hesitate to let me know your thoughts. Many Thanks! Opus Code Review: [PM-33982] feat: Add device management screen (#6754)**Date:** 2026-05-07 | **Reviewed by:** Claude Code | **Model:** OpusSummary
This PR adds a well-structured device management screen that follows the established architectural patterns of the codebase. The multi-source async loading pattern ( FindingsNo findings found. Reviewed and Dismissed🔍 3 initial findings dismissed after validationSessionItem test tag is hardcoded "CurrentItemCell" but renders for both Current and non-current devices
|
| Severity | Count |
|---|---|
| 🛑 Blocker | 0 |
| 3 | |
| ♻️ Refactor | 2 |
This PR adds a well-structured Device Management screen following Bitwarden's MVVM/UDF and State/Action/Event patterns, with appropriate feature-flag gating and broad test coverage. The primary bug is an inverted boolean condition in ManageDevicesState.hideBottomSheet that permanently breaks the "Skip for now" dismissal for F-Droid and pre-Android-13 users — a scenario with no test coverage. The success-path test for AuthRepositoryImpl.getDevices does not exercise the isCurrentDevice=true branch despite that branch being the sole reason currentDeviceIdentifier is threaded through. One auth-request filter function is duplicated verbatim from a sibling ViewModel.
Findings
⚠️ Important
hideBottomSheet dismiss is permanently ineffective on F-Droid and pre-Android-13 builds
app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModel.kt:293
Caught by: Bug analysis agent
Details
The hideBottomSheet computed property is:
val hideBottomSheet: Boolean
get() = internalHideBottomSheet &&
!isFdroid &&
isBuildVersionAtLeast(Build.VERSION_CODES.TIRAMISU)Because all three conditions are joined with &&, the property can only return true when the user dismissed it AND the build is not F-Droid AND the OS is Android 13+.
F-Droid (isFdroid = true): !isFdroid is false, so hideBottomSheet is permanently false regardless of internalHideBottomSheet. Tapping "Skip for now" sets internalHideBottomSheet = true but the sheet reappears on every visit. F-Droid users also cannot receive FCM push notifications, so the notification-permission prompt is irrelevant to them.
Android < 13: isBuildVersionAtLeast(TIRAMISU) is false, so hideBottomSheet is permanently false — dismiss has no effect here either.
Correct semantics (hide when on F-Droid, OR pre-Tiramisu, OR user dismissed):
val hideBottomSheet: Boolean
get() = isFdroid || !isBuildVersionAtLeast(Build.VERSION_CODES.TIRAMISU) || internalHideBottomSheetgetDevices success test only covers isCurrentDevice=false; isCurrentDevice=true case is untested
app/src/test/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt:6888
Caught by: Code quality agent
Details
The success test constructs a DeviceResponseJson with identifier = "deviceIdentifier", which differs from the stub uniqueAppId = "testUniqueAppId", so only the isCurrentDevice = false branch of toDeviceInfo(currentDeviceIdentifier) is exercised.
The isCurrentDevice = true path — which determines which device is sorted to the top of the list and labeled "Current session" — has no coverage at the repository or extension layer. The sole reason currentDeviceIdentifier is threaded through AuthRepositoryImpl → toDeviceInfo is to produce this flag, yet the true branch is never asserted.
A second test case where identifier == authDiskSource.uniqueAppId should verify that isCurrentDevice = true is produced.
HideBottomSheet test does not cover isFdroid=true; the dismissal bug goes undetected
app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModelTest.kt:113
Caught by: Validation agent (collateral)
Details
The HideBottomSheet should set hideBottomSheet to true test hardcodes buildInfoManager.isFdroid = false in the class-level mock and mocks isBuildVersionAtLeast(any()) to true. Under those conditions hideBottomSheet evaluates correctly and the test passes.
There is no test exercising isFdroid = true, which is the exact configuration where hideBottomSheet is permanently false regardless of internalHideBottomSheet. A complementary test such as:
@Test
fun `HideBottomSheet has no effect on F-Droid builds`() {
every { buildInfoManager.isFdroid } returns true
val viewModel = createViewModel()
viewModel.trySendAction(ManageDevicesAction.HideBottomSheet)
assertFalse(viewModel.stateFlow.value.hideBottomSheet)
}would have caught the logic inversion before this PR reached review.
♻️ Refactor
filterRespondedAndExpired duplicated verbatim from PendingRequestsViewModel
app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModel.kt:467
Caught by: Architecture agent
Details
The private extension function filterRespondedAndExpired(clock: Clock) at line 467 is character-for-character identical to the same function in PendingRequestsViewModel.kt. Both live in the accountsecurity feature tree and share the same auth-request filtering contract.
The project anti-pattern list explicitly lists "Create new patterns when established ones exist" as a DON'T. If the 5-minute expiry threshold changes or the filter predicate evolves, both copies must be updated in lockstep or they silently diverge.
Suggested fix: move the function to a shared utility such as app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestFilterExtensions.kt and import it at both call sites.
testTag("CurrentItemCell") applied to both None and Current status branches
app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesScreen.kt:229
Caught by: Code quality agent
Details
.testTag("CurrentItemCell") is applied to the SessionItem composable in the when branch that covers both DeviceSessionStatus.None and DeviceSessionStatus.Current. The tag name implies it identifies only the current-session device, but every inactive session also receives it.
Any future test querying onNodeWithTag("CurrentItemCell") in a list with mixed statuses will get multiple nodes and fail with an ambiguous-node error. Renaming to "SessionItemCell" (with a separate tag for Current-only if needed) would align with the project convention where tags describe content rather than selection state.
Reviewed and Dismissed
🔍 2 initial findings dismissed after validation
Inside fingerprintPhrase?.let block, passes item.fingerprintPhrase instead of it
app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesScreen.kt:201
Caught by: Code quality agent
Original severity: ♻️ Refactor
Original confidence: 85/100
Dismissed at: Step 4 validation
Dismissed because: Inside a ?.let block, Kotlin smart-casts the nullable receiver to non-null in the block body, making item.fingerprintPhrase and it semantically identical; a senior engineer would not flag this in review.
hideBottomSheet logic inverted for F-Droid builds (security framing)
app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModel.kt:293
Caught by: Security & logic agent
Original severity:
Original confidence: 82/100
Dismissed at: Step 4 validation
Dismissed because: Exact duplicate of bug-1 — same property, same line, same root cause; the security-angle framing adds no new finding beyond what bug-1 already covers.
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33982
📔 Objective
Add a new screen for device management on Android.
This new screen should display the current device, followed by the pending authorization requests and finally all devices with active sessions ordered by "last active date".
📸 Screenshots