[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.
# Conflicts: # app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt
| 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.
3 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