Closed gpuente closed 1 week ago
Looking good! Already see some potential simplifications for when we move to the new document-centric paradigm.
Hey @acaldas! I've addressed your comments in the latest commits. Here's a summary of the changes:
pull
and push
optional in SyncUnitStatusObject
syncUnitStatus
is only set with push/pull
if the drive has listeners/triggers
. This prevents creating a push status if the drive has no listeners or a pull status if the drive has no triggers.initializeDriveSyncStatus
: I've added a specific function to initialize the sync units of a drive. I found it clearer to set this map inside the _initializeDrive
method rather than inside shouldSyncRemoteDrive
. Here, I set both push and pull (if needed), before starting to get or push any updates. Also, instead of determining whether to update to INITIAL_SYNCING
or SYNCING
by checking the current status of sync unit, I delegated this responsibility to updateSyncUnitStatus
. Since both INITIAL_SYNCING
and SYNCING
are syncing statuses (with INITIAL_SYNCING
used only to identify the first sync), we should not replace INITIAL_SYNCING
with SYNCING
. This is exactly what updateSyncUnitStatus
handles. So, if an attempt is made to set a SYNCING
status for a sync unit that is currently in INITIAL_SYNCING
, the status will remain in INITIAL_SYNCING
until it's updated to either SUCCESS
or ERROR
. This also allows us to control this behavior in one place rather than in every location where SYNCING
might be set for a sync unit.Although this PR addresses some of the current issues with the document sync status, I have identified additional problems that fall outside the scope of this PR and should be tackled in future ones. These issues will likely require some planning before they can be resolved (perhaps through a workshop discussion?).
Status per listener and trigger: As we discussed yesterday, since a document may push and receive updates from multiple triggers and listeners, the status of a document should be linked to its syncUnitId + listener/trigger (push = syncUnitId + listener
, pull = syncUnitId + trigger
). The resulting status for a document in this scenario would be combinedStatus(combinedStatus(...listenerStatus), combinedStatus(...triggerStatus))
. This approach would provide specific statuses for individual or multiple listeners and triggers.
Folder status: Since folders are not documents, no sync unit is created for them, meaning no status is tracked for folders. In this case, the folder’s status should be the combined status of the files within it, including those in subfolders. This will require processing to gather all child files (including those in subfolders), calculate each file's combined status, and then determine the folder's status based on the combined statuses of all the files. We could potentially optimize this process by implementing some sort of caching system, but this is something that needs further consideration.
Persisting error status for sync units: Another issue occurs when we receive a strand containing operations that cause an error after being applied. Although the error status is stored for that sync unit, later strands, whether different or empty, may be processed successfully, which updates the error status to success, even when no new operations have been performed on the document with the original error. One possible solution would be to update the status only for documents that receive new operations, and to retain the previous status for untouched documents. While this might work during an initial load, it fails if browser is refreshed: since the sync unit's status is stored in memory, refreshing the browser causes the previous status (e.g., "error") to be lost. If no new strands are received for the document with the error, we cannot transition from syncing to either success or error. To address this, we could persist the latest sync status (possibly by extending the storage layer to support this) and restore the previous status from the storage layer.
Migrating sync status management to a new class: Due to the complexity of handling these issues, I believe it would be beneficial to migrate sync status management to a dedicated class. This would isolate the related code and make it more maintainable and readable.
Description: