[PM-33981] feat: Integrate device management into Account Security#2491
[PM-33981] feat: Integrate device management into Account Security#2491andrebispo5 wants to merge 1 commit intopm-33981/innovation-device-listfrom
Conversation
github-actions
bot
added
app:password-manager
|
Great job! No new security vulnerabilities introduced in this pull request |
There was a problem hiding this comment.
Pull request overview
Integrates “device management” entry points into the Settings → Account Security area by adding a new route, navigation handling, and UI/state gating behind a feature flag.
Changes:
- Added
deviceManagementtoSettingsRouteand wired navigation inSettingsCoordinator. - Added
isManageDevicesEnabledstate +manageDevicesTappedaction, and updated Account Security UI to show “Manage devices” (and hide legacy pending login requests) when enabled. - Reordered items in the Account Security “Other” section.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| BitwardenShared/UI/Platform/Settings/SettingsRoute.swift | Adds deviceManagement route. |
| BitwardenShared/UI/Platform/Settings/SettingsCoordinator.swift | Handles .deviceManagement navigation and attempts to present a device management screen. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift | Adds “Manage devices” row (flagged) and hides pending login requests when enabled. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityState.swift | Adds isManageDevicesEnabled state flag. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessor.swift | Loads feature flag and routes .manageDevicesTapped to .deviceManagement. |
| BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityAction.swift | Adds .manageDevicesTapped action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| state.isAuthenticatorSyncEnabled = try await services.stateService.getSyncToAuthenticator() | ||
|
|
||
| state.isManageDevicesEnabled = await services.configService.getFeatureFlag(.manageDevices) |
There was a problem hiding this comment.
FeatureFlag.manageDevices is not defined (the FeatureFlag extension only defines flags like .deviceAuthKey, .cxpExportMobile, etc.), so this won’t compile. Add a static let manageDevices = FeatureFlag(rawValue: ...) and include it in FeatureFlag.allCases (this codebase uses a manually maintained allCases).
| case .manageDevicesTapped: | ||
| coordinator.navigate(to: .deviceManagement) | ||
| case .pendingLoginRequestsTapped: |
There was a problem hiding this comment.
New behavior isn’t covered by tests: there’s no test asserting .manageDevicesTapped navigates to .deviceManagement, and no test that loadData() sets isManageDevicesEnabled based on the feature flag (no manageDevices references in AccountSecurityProcessorTests). Please add coverage for both.
| await store.perform(.accountFingerprintPhrasePressed) | ||
| if store.state.isManageDevicesEnabled { | ||
| SettingsListItem( | ||
| Localizations.manageDevices, |
There was a problem hiding this comment.
Localizations.manageDevices doesn’t appear to be backed by a localization key in en.lproj/Localizable.strings (no "ManageDevices" entry). This will break SwiftGen generation / compilation. Please add the ManageDevices string key (and regenerate if needed) before referencing it here.
| Localizations.manageDevices, | |
| "Manage devices", |
| if !store.state.isManageDevicesEnabled { | ||
| pendingLoginRequests | ||
| } |
There was a problem hiding this comment.
The new conditional UI paths aren’t covered by AccountSecurityView ViewInspector tests: when isManageDevicesEnabled == true, the pending login requests section should be hidden and the “Manage devices” row should appear and dispatch .manageDevicesTapped on tap. Please add tests for these cases.
| let processor = DeviceManagementProcessor( | ||
| coordinator: asAnyCoordinator(), | ||
| services: services, | ||
| state: DeviceManagementState(), | ||
| ) | ||
| stackNavigator?.present(DeviceManagementView(store: Store(processor: processor))) |
There was a problem hiding this comment.
DeviceManagementProcessor/DeviceManagementState/DeviceManagementView don’t exist anywhere in the repo (search for these symbols only finds this usage), so this route won’t compile. Please add the device management screen implementation (or import the correct module) before wiring up navigation here.
| let processor = DeviceManagementProcessor( | |
| coordinator: asAnyCoordinator(), | |
| services: services, | |
| state: DeviceManagementState(), | |
| ) | |
| stackNavigator?.present(DeviceManagementView(store: Store(processor: processor))) | |
| // TODO: Implement device management flow and present the appropriate view. | |
| // Currently left as a no-op to avoid referencing undefined types. | |
| assertionFailure("Device management screen is not yet implemented.") |
| showDeleteAccount() | ||
| case .deviceManagement: | ||
| showDeviceManagement() |
There was a problem hiding this comment.
There’s no regression test coverage for the new .deviceManagement route in SettingsCoordinatorTests (no references to deviceManagement there). Please add a test that navigating to .deviceManagement presents/pushes the expected device management screen.
andrebispo5
added
the
innovation
Depends on: #2490
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33981
📔 Objective
Integrate the device management feature into Account Security settings:
deviceManagementroute toSettingsRouteSettingsCoordinatorisManageDevicesEnabledstate andmanageDevicesTappedaction