[PM-41929] fix: Update the manage devices screen when a passwordless request push is received - #7280
[PM-41929] fix: Update the manage devices screen when a passwordless request push is received#7280aj-rosado wants to merge 8 commits into
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the head revision, which now merges Code Review DetailsNo code findings. Notes on things checked and cleared:
PR Metadata Assessment
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7280 +/- ##
==========================================
- Coverage 86.25% 85.84% -0.41%
==========================================
Files 891 978 +87
Lines 65294 67541 +2247
Branches 9808 9908 +100
==========================================
+ Hits 56320 57982 +1662
- Misses 5472 6081 +609
+ Partials 3502 3478 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * Whether this request may still be approved or declined, meaning it has not already been | ||
| * approved, not been declined (indicated by it not being approved & having a responseDate), | ||
| * and has not expired (it is under 5 minutes old). | ||
| */ | ||
| private val AuthRequest.isActionable: Boolean | ||
| get() = !requestApproved && | ||
| responseDate == null && | ||
| !creationDate.isOverFiveMinutesOld(clock) |
There was a problem hiding this comment.
♻️ DEBT: isActionable is now a third copy of the approved/declined/expired predicate.
Details and fix
The same three-clause rule already exists in two places:
ManageDevicesViewModel.kt:537—List<AuthRequest>.filterRespondedAndExpired(clock)PendingRequestsViewModel.kt:401— identicalfilterRespondedAndExpired(clock)
Adding a third copy here means the five-minute window and the decline detection now have to be kept in sync across three files.
Since this PR already extracts toAuthRequest into data/auth/manager/util/, consider putting the predicate there too and having both view models' filterRespondedAndExpired delegate to it:
// data/auth/manager/util/AuthRequestExtensions.kt
val AuthRequest.isActionable: Boolean
get() = !requestApproved &&
responseDate == null &&
!creationDate.isOverFiveMinutesOld(clock)(The clock would need to become a parameter, matching how filterRespondedAndExpired already takes one.)
Non-blocking — the current behavior is correct and matches the existing copies.
| publicKey = initialAuthRequest.publicKey, | ||
| fingerprint = initialAuthRequest.fingerprint, | ||
| ) | ||
| .toAuthRequest(fingerprint = initialAuthRequest.fingerprint) |
There was a problem hiding this comment.
Can we just pass these values into the extension method instead of copying it.
fun AuthRequestsResponseJson.AuthRequest.toAuthRequest(
fingerprint: String,
publicKey: String = this.publicKey,
responseDate: Instant = this.responseDate,
isRequestApproved: Boolean = this.requestApproved,
): AuthRequest = AuthRequest(| fun AuthRequestsResponseJson.AuthRequest.toAuthRequest( | ||
| fingerprint: String, | ||
| ): AuthRequest = AuthRequest( | ||
| id = id, |
There was a problem hiding this comment.
For clarity, can you dd the explicit this to all of these.
| // The device list is the only source that reports which device owns a pending request, so | ||
| // it is re-read before the new request can be rendered against its device. | ||
| viewModelScope.launch { | ||
| sendAction( |
There was a problem hiding this comment.
Can we just call a common method from both here and from handlePasswordlessAuthRequestDevicesReceive?
There was a problem hiding this comment.
Am I missing something or is the PasswordlessAuthRequestDevicesReceive action only ever launched from here?
If so, why do we need to actions like this?
There was a problem hiding this comment.
OK, I finally go there, the authRepository.getDevices() is suspending.
All of this is fine 😄
There was a problem hiding this comment.
I have simplified it a bit by moving getDevices into a map on the getPasswordlessAuthRequestFlow
| @@ -399,8 +399,4 @@ sealed class PendingRequestsAction { | |||
| * * The request has expired (it is at least 5 minutes old). | |||
| */ | |||
| private fun List<AuthRequest>.filterRespondedAndExpired(clock: Clock) = | |||
There was a problem hiding this comment.
Looks like this function exists in 2 spots. What do you think about consolidating them in the AuthRequestExtenstions file?
| init { | ||
| updateAuthRequestList() | ||
| fetchAllDevices() | ||
| observePasswordlessAuthRequests() |
There was a problem hiding this comment.
Instead of observing this separately, should getAuthRequestsWithUpdates called in updateAuthRequestList just observer the push notifications directly?
Is there any reason not to do this?
There was a problem hiding this comment.
observePasswordlessAuthRequests Will only get the authRequest returned by the push instead of the whole list
There was a problem hiding this comment.
Is there a reason we would not want to live update the getAuthRequestsWithUpdates flow with data from a push?
There was a problem hiding this comment.
no reason, was simply my choice to try to minimize the network call, although looking at it now we might benefit from getting the whole list as right now it will not update expired requests (or other new requests) and the impact would be minimal
There was a problem hiding this comment.
I have approved and you are free to merge but I do agree there would be benefits to merging the push notifications into the other flow. It would make this ViewModel much simplier and improve the speed at which getAuthRequestsWithUpdates gets updates, which is good for this screen and other places in the app.
Something to consider.
| * * The request has been declined (indicated by it not being approved & having a responseDate). | ||
| * * The request has expired (it is at least 5 minutes old). | ||
| */ | ||
| fun List<AuthRequest>.filterRespondedAndExpired(clock: Clock): List<AuthRequest> = |
| /** | ||
| * An unanswered request created one minute before the [AuthRequestManagerTest] clock, making it | ||
| * neither responded to nor expired. | ||
| */ | ||
| private val PENDING_AUTH_REQUEST_RESPONSE: AuthRequestsResponseJson.AuthRequest = | ||
| AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE.copy( | ||
| creationDate = Instant.parse("2023-10-27T11:59:00Z"), | ||
| requestApproved = false, | ||
| ) | ||
|
|
||
| private val PENDING_AUTH_REQUEST: AuthRequest = AUTH_REQUEST.copy( | ||
| creationDate = Instant.parse("2023-10-27T11:59:00Z"), | ||
| requestApproved = false, | ||
| ) |
There was a problem hiding this comment.
♻️ DEBT: PENDING_AUTH_REQUEST_RESPONSE and PENDING_AUTH_REQUEST are declared but never referenced.
Details and fix
Both constants appear to be leftovers from the getPasswordlessAuthRequestFlow implementation that was collapsed into getAuthRequestsWithUpdates in the latest commit. The only new test in this file (getAuthRequestsWithUpdates should re-read on a push and ignore a non-active user push) uses AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE / AUTH_REQUEST and PASSWORDLESS_REQUEST_DATA.
UnusedPrivateMember is set to active: false in detekt-config.yml, so nothing will flag these automatically. Suggest deleting both declarations (and the KDoc above them).
| mutableStateFlow.update { it.copy(authRequests = filteredRequests.toImmutableList()) } | ||
| // The device list is the only source that reports which device owns a pending request, so | ||
| // it is re-read before the new list can be rendered against it. | ||
| fetchAllDevices() |
There was a problem hiding this comment.
🎨 SUGGESTED: Making the device fetch depend on the auth-request result serializes two network calls that used to run in parallel.
Details and fix
fetchAllDevices() was removed from init, so the only trigger is now this call site. The first emission from getAuthRequestsWithUpdates() arrives only after getAuthRequests() completes (service call plus a fingerprint SDK call per request), so time-to-content on screen open goes from max(authRequests, getDevices) to authRequests + getDevices. It also adds a getDevices() round trip on every 5-minute poll tick, not just on pushes.
One option that keeps the push behavior while restoring the parallel initial load: keep fetchAllDevices() in init, and here only re-fetch when the request id set actually differs from state.authRequests, e.g.
val previousIds = state.authRequests.map { it.id }.toSet()
mutableStateFlow.update { it.copy(authRequests = filteredRequests.toImmutableList()) }
if (filteredRequests.map { it.id }.toSet() != previousIds) fetchAllDevices()Non-blocking — the current behavior is correct, this is about perceived load time and redundant polling traffic.
There was a problem hiding this comment.
@claude We want to get the most recent screen state and also keep the VM simpler
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 18s —— View job
Agreed — keeping the unconditional re-fetch
That rationale holds up, and checking it against the current code my suggestion was actually the worse option.
On "most recent screen state": getDevices() is the only source for lastActivityDate, isTrusted, and newly-registered devices. Gating the fetch on a change in the auth-request id set would mean the device rows only ever refresh when a login request changes — a device that logged in elsewhere since the screen opened would keep showing a stale last-activity label through both the 5-minute poll and a pull-to-refresh. Always re-reading is the behavior that matches what the screen claims to show.
On "keep the VM simpler": my suggested conditional would have broken pull-to-refresh. isRefreshing = false is only ever cleared in handleGetDevicesResultReceived (ManageDevicesViewModel.kt:210 / :220). Skipping fetchAllDevices() when the id set is unchanged — the common case for a pull-to-refresh — would leave the spinner running indefinitely. Fixing that would mean either clearing isRefreshing in two places or threading a "was this user-initiated" flag through, which is exactly the complexity you're avoiding.
The remaining cost is the serialized initial load (authRequests then getDevices instead of in parallel), which is covered by the Loading state and is a fair price for the single linear path the VM has now. Withdrawing the suggestion — no change needed.
For reference, the current shape reads cleanly: one flow in updateAuthRequestList() fed by both the poll and the push merge in AuthRequestManagerImpl.getAuthRequestsWithUpdates(), then a device read, then render.
My separate comment on the silent failed pull-to-refresh at :209 is a different question and still stands on its own — that one is about user feedback, not about which data gets re-read.
• PM-41929/manage-devices-push-update-screen
There was a problem hiding this comment.
Doesn't this mean we will be re-fetching the devices everytime the auth requests emits. Was that part of you're goal?
| mutableStateFlow.update { | ||
| it.copy( | ||
| authRequests = emptyList(), | ||
| viewState = PendingRequestsState.ViewState.Error, | ||
| ) | ||
| // Once something has rendered, a failed refresh leaves it in place rather | ||
| // than replacing it with an error; the next update reconciles it. | ||
| if (it.viewState is PendingRequestsState.ViewState.Loading) { | ||
| it.copy( | ||
| authRequests = emptyList(), | ||
| viewState = PendingRequestsState.ViewState.Error, | ||
| ) | ||
| } else { | ||
| it | ||
| } | ||
| } |
There was a problem hiding this comment.
🎨 SUGGESTED: A user-initiated pull-to-refresh that fails is now silent — no error view and no other feedback.
Details and fix
The rationale for suppressing the error state ("a failed refresh leaves it in place") holds for the background poll, but handleRefreshPull reaches the same code path: it sets isRefreshing = true, restarts the flow, and on an Error emission the state is returned unchanged and isRefreshing is reset at line 214. The user sees the spinner vanish and stale data, with no indication the refresh failed. Before this change they at least got the Error view state.
Consider distinguishing the two, e.g. tracking that the update was user-initiated and emitting PendingRequestsEvent.ShowSnackbar on failure in that case, while keeping the silent behavior for the poll.
Note the same shape exists in ManageDevicesViewModel.handleGetDevicesResultReceived, where a pull-to-refresh device failure also resolves with no feedback.
| viewState = if (it.devicesLoaded) { | ||
| it.viewState | ||
| } else { | ||
| ManageDevicesState.ViewState.Error | ||
| }, |
There was a problem hiding this comment.
🎨 SUGGESTED: A failed pull-to-refresh is silent once devices have loaded — the same case just changed to show an error in PendingRequestsViewModel.
Details
devicesLoaded does not distinguish a background poll/push refresh from a user-initiated one, so:
- Devices load successfully →
devicesLoaded = true,Contentrendered. - Network drops.
- User pulls to refresh →
isRefreshing = true→updateAuthRequestList()→fetchAllDevices()→GetDevicesResult.Error. viewStateis left untouched andisRefreshingis cleared. The spinner disappears, the stale list stays, and nothing tells the user the refresh failed —ManageDevicesScreenonly surfaces errors viaViewState.Error(the snackbar host is wired toSnackbarRelay.LOGIN_APPROVALonly).
The PR description says "polling and pull-to-refresh reconcile it later", but a failed pull-to-refresh is itself swallowed here. Commit f1f45da took the opposite approach in PendingRequestsViewModel for the same failure class, so the two sibling screens now disagree.
If keeping stale content is still preferred for background refreshes, one option is to track whether the in-flight fetch came from RefreshPull and show the error (or a snackbar) only in that case.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41929
📔 Objective
The Manage Devices screen only learned about pending login requests when it polled or when the
user pulled to refresh, so a request initiated while the screen was already open did not appear
until the next poll cycle.
This wires the push notification stream into the screen:
AuthRequestManager.getPasswordlessAuthRequestFlow()observesPushManager.passwordlessRequestFlow,filters to the active user (a push for another user would otherwise be hydrated with the active
user's token), hydrates the request with its fingerprint, and emits only requests that are still
actionable — not already approved, not declined, and under five minutes old. Requests that fail
to hydrate are logged and dropped rather than surfaced as errors.
ManageDevicesViewModelcollects that flow, re-reads the device list (the only source thatreports which device owns a pending request), and merges the request into state, replacing any
earlier copy so it cannot be listed twice. Because the refresh is not user-initiated, a failed
device fetch leaves the screen untouched instead of replacing it with an error — polling and
pull-to-refresh reconcile it later.
Also extracts the repeated
AuthRequestsResponseJson.AuthRequest→AuthRequestmapping into atoAuthRequest(fingerprint)extension, replacing seven hand-written copies inAuthRequestManagerImplwith no behavior change.