[PM-5778] [PM-1401] Refactor Sync Service

[shane-melton]

Type of change

- [ ] Bug fix
- [ ] New feature development
- [X] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Introduce syncEvent$ and ~syncError$~ observables to the Sync service to allow other services/components to observe full syncs and any potential errors that may occur. This is precursor work that will allow clients to effectively show an error message in the Vault instead of a confusing empty vault.

Its partially based on the existing, but currently unused, SyncNotifierService and re-uses the types it introduced and moves its logic into the SyncService instead.

Code changes

Sync Service

Introduce subjects for the syncEvent$ observable ~and a convenience syncError$ observable. The syncError$ observable filters and maps the syncEvent$ observable to emit an error when one occurs.~

The syncEventSubject emits at the start of a fullSync and at the end of any branch of the method with a Completed status. The Completed events will also have a successful flag to indicate if new sync data was successfully retrieved. If new data was retrieved it will be available on the data field. Otherwise, data will be undefined and if there was an unexpected exception the error field will be populated ~and the syncError$ observable will emit the error object.~

Api Service - refreshIdentityToken()

Update the refreshIdentityToken() method to throw any errors to upstream callers. I was previously consuming any caught errors, but was still throwing a null error which prevents any callers from handing different types of errors. (e.g. If there is a network error that occurs during sync we'd like to bubble up that exception to the UI to be displayed accordingly).

I did a quick scan of the existing consumers of refreshIdentityToken and couldn't find any that were explicitly expecting a null error so this was likely a dependency of behavior that no longer exists in the code base.

Remove SyncNotiferService

The SyncNotiferService is no longer necessary as the syncEvent$ observable is available directly within the SyncService and can be removed. This includes removing from the various points of registration in bw.ts, main.background.ts, etc.

Before you submit

• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team
• Ensure that all UI additions follow WCAG AA requirements

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – 22d110cc-89bf-4794-9c53-959a73491192

No New Or Fixed Issues Found

[shane-melton]

Thanks for the suggestion of removing the internal subscription @justindbaur! I like it much better as a piped version of the syncEvent$ observable.

I would be curious to see how these will be consumed as well, it feels slightly weird for errors to have a buffer and it could be nicer to start subscribing to errors, do the sync action, and then see if any errors happened but that all depends on how you use it.

Here's an example how I planned on using it with regards to showing a warning in the Vault on failed syncs. https://github.com/bitwarden/clients/blob/061d89d0133d80eca0b65333edf466a2e2d7e0da/apps/web/src/app/vault/individual-vault/vault.component.ts#L152-L161

With that in mind, I needed to change the syncEventSubject to a behavior/replay subject. This is because the component subscribes too late to the syncEvent$ and misses the failed sync event and since there are no other subscribers, the syncError$ pipe never executes and "caches" the error.

With a behavior subject for syncEventSubject when the component finally gets around to subscribing to the syncError$, it can grab the most recent syncEvent and process it accordingly. Do you see any issues with this?

It works pretty well on the vault/pm-1401/failed-sync-warning-component branch if you want to take a look at an example usage. (You can easily simulate errors by either failing to start the API or disabling the network for the tab from dev tools)

[justindbaur]

The startWith(null), ..., shareReplay({ bufferSize: 1 }) sandwich was me attempting to make that observable act very similarly to something that is backed by a BehaviorSubject. shareReplay is creating a ReplaySubject for you with the bufferSize you give it and then the startWith was saying start off with this value which would immediately be the buffer of the underlying ReplaySubject but there is a way better way to do it. You can do share({ connector: () => new BehaviorSubject(null) }) which will make it act exactly like a behavior subject without the need for the startWith dance.

[shane-melton]

After revisiting this again I no longer saw the benefit of the BehaviorSubject for the syncEvent$ and found that the required initial null state could possibly cause more confusion about the status of a sync on application startup. I opted to use a ReplaySubject so that nothing is emitted until the first sync is underway; that way consumers do not have to worry about a null event and any "late" subscribers are still able to get the most recent sync event/error.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 27.68%. Comparing base (dba910d) to head (4ebffde).

Files	Patch %	Lines
...ibs/common/src/vault/services/sync/sync.service.ts	0.00%	11 Missing :warning:
libs/common/src/vault/types/sync-event-args.ts	0.00%	5 Missing :warning:
libs/common/src/services/api.service.ts	0.00%	1 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6944   +/-   ##
=======================================
  Coverage   27.68%   27.68%           
=======================================
  Files        2381     2378    -3     
  Lines       69137    69133    -4     
  Branches    12882    12883    +1     
=======================================
  Hits        19139    19139           
+ Misses      48564    48560    -4     
  Partials     1434     1434           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[shane-melton]

This will need to be re-evaluated once the SyncService is using the new State Provider for its LastSyncDate data. Another concern, is the current implementation uses a BehaviorSubject for the LastSyncEvent$ which could potentially keep sync data in memory after logout and will need to be mitigated/accounted for.