feat(backup): enhance backup progress tracking and state management#280
feat(backup): enhance backup progress tracking and state management#280
Conversation
|
| const backups = await backupsConfig.obtainBackupsInfo(); | ||
| const container = await BackupsDependencyContainerFactory.build(); | ||
| const backupService = container.get(BackupService); | ||
| try { |
There was a problem hiding this comment.
Why this needs to be wrapped inside a try?, the idea is to reduce the amount of try catch to the max and wrap and handle where we know 100% that the exception will be thrown, this way we reduce complexity
| errors: BackupErrorsTracker, | ||
| signal: AbortSignal, | ||
| ): Promise<void> { | ||
| ) { |
There was a problem hiding this comment.
I think its better to keep the funtion signature
| const itemCounts = new Map<string, number>(); | ||
|
|
||
| for (const backup of backups) { | ||
| if (signal.aborted) { | ||
| logger.debug({ tag: 'BACKUPS', msg: 'Precalculation aborted' }); | ||
| break; | ||
| } | ||
|
|
||
| const count = await precalculateBackupItemCount(backup, container); | ||
| itemCounts.set(backup.folderUuid, count); | ||
| } | ||
|
|
||
| // eslint-disable-next-line no-await-in-loop | ||
| const result = await backupService.runWithRetry(backupInfo, signal, tracker); | ||
| if (result.isLeft()) { | ||
| const error = result.getLeft(); | ||
| logger.debug({ tag: 'BACKUPS', msg: 'failed', error: error.cause }); | ||
| // TODO: Make retryError extend DriveDesktopError to avoid this check | ||
| if (error instanceof DriveDesktopError && 'cause' in error && error.cause && isSyncError(error.cause)) { | ||
| errors.add(backupInfo.folderId, { name: backupInfo.name, error: error.cause }); | ||
| const backupIds = backups.map((b) => b.folderUuid); |
| })); | ||
|
|
||
| describe('BackupProgressTracker', () => { | ||
| describe('BackupProgressTracker - Functional approach', () => { |
There was a problem hiding this comment.
I dont think this comment adds something meaningful
| this.processedItems = 0; | ||
| this.totalItems = 0; | ||
| initializeWeights(backupIds: string[], fileCounts: ReadonlyMap<string, number>): void { | ||
| this.state = initializeWeights(this.state, backupIds, fileCounts); |
There was a problem hiding this comment.
This method could be renamed to something more meaningful, right? such as initializeBackupProgressWeights
| export const precalculateBackupItemCount = async (backupInfo: BackupInfo, container: Container) => { | ||
| try { | ||
| const localTreeBuilder = container.get(LocalTreeBuilder); | ||
| const remoteTreeBuilder = container.get(RemoteTreeBuilder); |
There was a problem hiding this comment.
its better if you use function instead
also, its better if you pass down laready the LocalTreebuild and the remoteTreeBuilder as param instead of the container, this way its one resposability less that the function has
| import { Container } from 'diod'; | ||
|
|
||
| export const precalculateBackupItemCount = async (backupInfo: BackupInfo, container: Container) => { | ||
| try { |
There was a problem hiding this comment.
The same as i comented before, is it better if we just narrow down the tryCatch to where we 100% now it can throw, so that we arent constantly wrapping everything in tryCatch, and return a result type there, so that we know that the function has a potential error and must be handled
| const localTreeBuilder = container.get(LocalTreeBuilder); | ||
| const remoteTreeBuilder = container.get(RemoteTreeBuilder); | ||
|
|
||
| const localTreeEither = await localTreeBuilder.run(backupInfo.pathname as AbsolutePath); |
There was a problem hiding this comment.
if we are 100% sure that this is the case, then maybe we should change completly the property to AbsolutePath instead of string
| batch: Array<LocalFile>, | ||
| signal: AbortSignal, | ||
| onFileProcessed: () => void, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
this is going to be deleted on #283
maybe we can replace the callback for a return on the equivalent
| @@ -22,6 +22,7 @@ export class FileBatchUploader { | |||
| remoteTree: RemoteTree, | |||
There was a problem hiding this comment.
same, this is going to be deleted in #283
2 participants
What is Changed / Added
We've refactored the backup progress tracking system to provide a linear, consistent progress display, eliminating unexpected jumps and visual inconsistencies. Previously, each folder added to the backup was tracked independently, causing progress bar fluctuations and apparent setbacks. The new implementation treats the backup as a unified process with weighted progress calculation, significantly improving the user experience when monitoring backup operations.