[PM-33981] feat: Add device management UI components#2490
[PM-33981] feat: Add device management UI components#2490andrebispo5 wants to merge 1 commit intopm-33981/innovation-device-listfrom
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
There was a problem hiding this comment.
Pull request overview
Adds the initial SwiftUI and state-management building blocks for a new Device Management screen under Account Security, including a list row UI and a processor to load/sort devices and associate pending login requests.
Changes:
- Introduces
DeviceManagementViewwith loading/empty/list states and pull-to-refresh. - Adds
DeviceManagementProcessor+State/Action/Effectfor loading devices, matching pending requests, and sorting. - Adds
DeviceRowSwiftUI component with status badges and activity/first-login metadata.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/DeviceManagement/DeviceRow.swift | New device list cell UI with badges, metadata rows, and tap handling for pending requests. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/DeviceManagement/DeviceManagementView.swift | New screen UI using LoadingView, empty state, and a list of DeviceRows. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/DeviceManagement/DeviceManagementState.swift | New state container for loading state/toast/current device ID. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/DeviceManagement/DeviceManagementProcessor.swift | New processor that fetches devices/current device/pending requests, matches/sorts results, and handles navigation/toast. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/DeviceManagement/DeviceManagementEffect.swift | Defines .loadData effect for the processor. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/DeviceManagement/DeviceManagementAction.swift | Defines user actions (tap, dismiss, toast lifecycle). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// Formats a date for display with date and time. | ||
| private func formattedDateTime(_ date: Date?) -> String { | ||
| guard let date else { return Localizations.unknown } |
There was a problem hiding this comment.
Localizations.unknown doesn’t appear to exist in BitwardenResources/Localizations/en.lproj/Localizable.strings (no Unknown key). Add the missing localization key (and regenerate SwiftGen) or use an existing Localizations.* value (e.g., UnknownXErrorMessage is not a direct replacement).
| guard let date else { return Localizations.unknown } | |
| guard let date else { | |
| return NSLocalizedString("Unknown", comment: "Fallback text for unknown date") | |
| } |
| /// Formats a date for display with date and time. | ||
| private func formattedDateTime(_ date: Date?) -> String { | ||
| guard let date else { return Localizations.unknown } | ||
| let formatter = DateFormatter() | ||
| formatter.dateStyle = .medium | ||
| formatter.timeStyle = .short | ||
| return formatter.string(from: date) |
There was a problem hiding this comment.
formattedDateTime creates a new DateFormatter every time it’s called, which is relatively expensive in a scrolling list. Consider using date.formatted(...) (as done elsewhere) or a cached/static formatter to avoid repeated allocations.
| /// Formats a date for display with date and time. | |
| private func formattedDateTime(_ date: Date?) -> String { | |
| guard let date else { return Localizations.unknown } | |
| let formatter = DateFormatter() | |
| formatter.dateStyle = .medium | |
| formatter.timeStyle = .short | |
| return formatter.string(from: date) | |
| /// A cached formatter for displaying dates with medium date and short time styles. | |
| private static let dateTimeFormatter: DateFormatter = { | |
| let formatter = DateFormatter() | |
| formatter.dateStyle = .medium | |
| formatter.timeStyle = .short | |
| return formatter | |
| }() | |
| /// Formats a date for display with date and time. | |
| private func formattedDateTime(_ date: Date?) -> String { | |
| guard let date else { return Localizations.unknown } | |
| return Self.dateTimeFormatter.string(from: date) |
| HStack(alignment: .center) { | ||
| VStack(alignment: .leading, spacing: 4) { | ||
| VStack(alignment: .leading, spacing: 0) { | ||
| // Device name | ||
| Text(device.displayName) | ||
| .foregroundStyle(SharedAsset.Colors.textPrimary.swiftUIColor) | ||
| .styleGuide(.bodySemibold) | ||
| .accessibilityIdentifier("DeviceNameLabel") | ||
|
|
There was a problem hiding this comment.
For VoiceOver, it’s typically better if the row reads as a single element (name + status + dates) instead of many separate labels. Consider applying .accessibilityElement(children: .combine) on the row’s main content container (similar to PendingRequestsView’s row implementation).
| deviceItems = matchPendingRequestsToDevices(deviceItems, pendingRequests: pendingRequests) | ||
|
|
||
| // Sort devices: current session first, then pending requests, then by activity. | ||
| deviceItems.sort { lhs, rhs in | ||
| // Current session always first. |
There was a problem hiding this comment.
This processor adds non-trivial logic (pending-request matching + multi-criteria sorting). Since similar flows (e.g. PendingRequestsProcessor) have unit tests, please add DeviceManagementProcessorTests covering matching/sorting and error handling to prevent regressions.
| .accessibilityIdentifier("DeviceNameLabel") | ||
|
|
||
| if device.isTrusted { | ||
| Text(Localizations.trusted) |
There was a problem hiding this comment.
Localizations.trusted doesn’t appear to exist in BitwardenResources/Localizations/en.lproj/Localizable.strings (no Trusted key). Add the missing localization key (and regenerate SwiftGen) or update to an existing localization constant.
| Text(Localizations.trusted) | |
| Text("Trusted") |
| Spacer() | ||
|
|
||
| Image(asset: SharedAsset.Icons.chevronRight16) | ||
| .imageStyle(.accessoryIcon16) |
There was a problem hiding this comment.
The chevron is purely decorative/affordance; consider marking it as non-accessible (e.g., Image(decorative:) or .accessibilityHidden(true)) so VoiceOver doesn’t announce it separately from the row content.
| .imageStyle(.accessoryIcon16) | |
| .imageStyle(.accessoryIcon16) | |
| .accessibilityHidden(true) |
| var body: some View { | ||
| LoadingView(state: store.state.loadingState) { devices in | ||
| if devices.isEmpty { | ||
| empty | ||
| .scrollView(centerContentVertically: true) | ||
| } else { | ||
| devicesList(devices) | ||
| .scrollView() | ||
| } |
There was a problem hiding this comment.
There are snapshot tests for similar screens (e.g. PendingRequestsView+SnapshotTests.swift), but this new view doesn’t add any. Please consider adding snapshot coverage for the empty and populated states to catch UI regressions (including AX text size variants).
| .scrollView() | ||
| } | ||
| } | ||
| .navigationBar(title: Localizations.manageDevices, titleDisplayMode: .inline) |
There was a problem hiding this comment.
Localizations.manageDevices doesn’t appear to exist in BitwardenResources/Localizations/en.lproj/Localizable.strings (no ManageDevices key). This will fail to compile unless the string is added (and SwiftGen updated).
| .navigationBar(title: Localizations.manageDevices, titleDisplayMode: .inline) | |
| .navigationBar(title: "Manage devices", titleDisplayMode: .inline) |
| .resizable() | ||
| .frame(width: 100, height: 100) | ||
|
|
||
| Text(Localizations.noDevicesFound) |
There was a problem hiding this comment.
Localizations.noDevicesFound doesn’t appear to exist in BitwardenResources/Localizations/en.lproj/Localizable.strings (no NoDevicesFound key). Add the missing localization key (and regenerate SwiftGen) to avoid build failures.
| Text(Localizations.noDevicesFound) | |
| Text("No devices found") |
| Text(Localizations.firstLoginLabel) | ||
| .foregroundStyle(SharedAsset.Colors.textSecondary.swiftUIColor) | ||
| .styleGuide(.subheadlineSemibold) | ||
|
|
||
| Text(formattedDateTime(device.firstLogin)) |
There was a problem hiding this comment.
Localizations.firstLoginLabel doesn’t appear to exist in BitwardenResources/Localizations/en.lproj/Localizable.strings (no FirstLoginLabel key). Add the missing localization key (and regenerate SwiftGen) or use an existing localization constant.

Depends on: #2489
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33981
📔 Objective
Add the Device Management screen UI components including:
DeviceManagementView- Main view with device list and empty stateDeviceManagementProcessor- Handles loading devices, matching pending requests, and sortingDeviceManagementState,DeviceManagementAction,DeviceManagementEffect- State managementDeviceRow- Individual device cell with status indicators