-
Notifications
You must be signed in to change notification settings - Fork 32
[PB-5465] Implement file version history actions #1771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/file-version-history
Are you sure you want to change the base?
[PB-5465] Implement file version history actions #1771
Conversation
…wnload functionality Adds complete file version management system with API integration for viewing, restoring, deleting, and downloading file versions. Updates name collision dialog to use replaceFile endpoint instead of delete-and-reupload for file replacements.
0fc701b to
1612aff
Compare
…itional messages and error handling
| "restoreVersion": "Restore version", | ||
| "downloadVersion": "Download version", | ||
| "deleteVersion": "Delete version" | ||
| "deleteVersion": "Delete version", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The remaining translations are included in this PR: #1773
48361a6 to
f59d219
Compare
| const user = localStorageService.getUser(); | ||
| if (!user) throw new Error('User not found'); | ||
|
|
||
| const fileStream = await downloadFile({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the multipart download for this? Imagine the file has 5GB or more. It would be better to download it in parallel-small chunks instead of the full file at once. You can do it by using the DownloadManager.downloadItem if I'm not mistaken.
Check that as we need to download the items using the actual method since it solves a lot of speed/memory issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to add tests to this service.
| }, | ||
| }); | ||
|
|
||
| const blob = await binaryStreamToBlob(fileStream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Downloading it as blob and then save it will causes memory issues, specially for Firefox. Also, it won't work for big files in Safari - the file will be downloaded but only 700 bytes (or even 0) will be saved to the disk as Safari does not allow saving big blobs.
Try the method I mentioned above as it already handles all these things, such as saving the downloaded + decrypted chunk directly to the user disk instead of saving it in memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this function in this service? It is a part of file versioning, isn't it? Also, remember to add tests to this service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This service is used in the name-collision modal, which belongs to Drive and isn’t necessarily part of the file-version history flow. That’s why I placed it inside the Drive module. It could live under Version History as well, but keeping it in Drive makes more sense in this context. What do you think?
The tests will be added in a separate PR.
…etter UX - Replace direct download with DownloadManager for efficient multipart downloads - Add workspace credentials support for version downloads - Position TaskLogger at bottom-right - Refactor: rename service files to camelCase convention (file-version.service.ts → fileVersion.service.ts, replace-file.service.ts → replaceFile.service.ts) - Pass file item and workspace context to downloadVersion for proper credential handling
Remove try-catch block and error handling from useVersionItemActions since DownloadManager already handles error reporting, notifications, and task status updates internally. This prevents duplicate error notifications from being shown to the user.
[PB-5465]: enhance internationalization for version management
|
| integrity sha512-91iEUvZizlwX6KBEFJ3JdFiGrhMBQ9R54sTc3Pei9QtV2FYTU8nTVEPYAg39tLOGzT/kVuplYOtBxfk6wFtSDA== | ||
| "@internxt/sdk@1.11.19": | ||
| version "1.11.19" | ||
| resolved "https://npm.pkg.github.com/download/@internxt/sdk/1.11.19/f9033c8bf0849d414a3865806bacd79026a38769#f9033c8bf0849d414a3865806bacd79026a38769" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is breaking your cloudflare build, and it also breaking the PR's that depend on this one. You are using an .npmrc file so you are changing the internxt registry from yarnpkg to github.
You should use this one:
registry=https://registry.yarnpkg.com/
@internxt:registry=https://registry.yarnpkg.com/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Since this is an old PR, I made the change in this one instead: #1779


Description
Related Issues
Related Pull Requests
Checklist
Testing Process
Additional Notes