[PM-6991] Improve reactivity of cipherViews$ observable in cipher service #11141
Conversation
…ciphers and localData
…pted cipher cache is cleared
|
New Issues
Fixed Issues
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #11141 +/- ##
==========================================
- Coverage 33.18% 33.16% -0.03%
==========================================
Files 2779 2779
Lines 86226 86223 -3
Branches 16419 16420 +1
==========================================
- Hits 28618 28597 -21
- Misses 55341 55360 +19
+ Partials 2267 2266 -1 ☔ View full report in Codecov by Sentry. |
|
Thanks for working on this, @shane-melton; this is a significant improvement. The last time I did something similar, I noticed the |
@gbubemismith Yep! This works as expected here, because the source observable is It also uses |
| // Decrypted ciphers depend on both ciphers and local data and need to be updated when either changes | ||
| this.cipherViews$ = merge(this.ciphers$, this.localData$).pipe( | ||
| switchMap(() => merge(this.forceCipherViews$, this.getAllDecrypted())), | ||
| share(), |
There was a problem hiding this comment.
❓ Why aren't we using a shareReplay(1)? My understanding is using shareReplay would mean VaultFilterComponent, VaultComponent and any other subscriber would get the most recent ciphers even if they subscribe at different times
There was a problem hiding this comment.
Ah, good point!
My thinking was to use share to avoid caching the decrypted ciphers in another spot that needed to be cleared from memory on lock/logout (it was before I had forceCipherViews$). And repeated calls to getAllDecrypted(), even by late subscribers, will still return a copy of the decrypted ciphers stored in state (internally it checks the decryptedCipherState first).
But, thinking about it again, the share operator will never allow getAllDecrypted() to be called again unless ciphers$ or localData$ emit again, so any late subscribers will just be stuck waiting.
I'll update to use shareReplay() like you suggest and we already have the clear issue taken care of with forceCipherViews$.
…1/refactor-cipher-service-to-rxjs
…r instead for cipherViews$
…mpty vault after modifying ciphers
|
|
||
| this.localData$ = this.localDataState.state$.pipe(map((data) => data ?? {})); | ||
| // First wait for ciphersExpectingUpdate to be false before emitting ciphers | ||
| this.ciphers$ = this.ciphersExpectingUpdate.state$.pipe( | ||
| skipWhile((expectingUpdate) => expectingUpdate), |
There was a problem hiding this comment.
ℹ️ This was not having any impact on behavior because ciphersExpectingUpdate starts with a default value of false (see the removed DeriveDefinition above). And because it started with false the skipWhile passes immediately and is never called again.
| @@ -279,7 +279,7 @@ export class VaultComponent implements OnInit, OnDestroy { | |||
| this.currentSearchText$ = this.route.queryParams.pipe(map((queryParams) => queryParams.search)); | |||
|
|
|||
| const ciphers$ = combineLatest([ | |||
| this.cipherService.cipherViews$, | |||
| this.cipherService.cipherViews$.pipe(filter((c) => c !== null)), | |||
There was a problem hiding this comment.
ℹ️ Filtering out null values here prevents flashing the Empty vault screen while ciphers are being decrypted. If there are truthfully no ciphers in the vault (e.g. a new account) the cipherViews$ will emit an empty array instead and then we'll see the empty vault.
There was a problem hiding this comment.
👍🏼 Thanks for this fix. I noticed that weird delay
| @@ -279,7 +279,7 @@ export class VaultComponent implements OnInit, OnDestroy { | |||
| this.currentSearchText$ = this.route.queryParams.pipe(map((queryParams) => queryParams.search)); | |||
|
|
|||
| const ciphers$ = combineLatest([ | |||
| this.cipherService.cipherViews$, | |||
| this.cipherService.cipherViews$.pipe(filter((c) => c !== null)), | |||
There was a problem hiding this comment.
👍🏼 Thanks for this fix. I noticed that weird delay
shane-melton
deleted the
vault/pm-6991/refactor-cipher-service-to-rxjs
branch
🎟️ Tracking
PM-6991
📔 Objective
We have run into race conditions where the vault would attempt to call
cipherService.getAllDecrypted()before the sync had completed. This would lead to an empty list being returned and showing as an empty vault to the user until they refreshed.This PR updates the existing
cipherViews$observable to be reactive tociphers$andlocalData$so that whenever either of those streams update, it will internally call and return the value ofgetAllDecrypted()for any subscribers tocipherViews$.ciphers$will always emit after a sync completes so this ensures and subscribers tocipherViews$will be notified of an update list of decrypted ciphers.With that change, the individual web vault and vault filter service now utilize the
cipherViews$observable instead of wrappinggetAllDecrypted()in ourasyncToObservable()helper.Note
There is still plenty of room for improvement here. Namely, requiring a consumers to provide a
userIdwhen fetching decrypted ciphers instead of relying on theactiveUserIdinternally. But, for now, this gets us one step closer to making the cipher service observable based.Also, I left the Browser's Vault filter component alone as it already has some safe guards to avoid the race condition and it is being deprecated with the Browser refresh.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes