TUS inspired multi-upload support - #4188
Open
LukasMalyszko wants to merge 73 commits into
Open
Conversation
- Introduced TUS resumable uploads functionality in the application helper. - Updated multi-upload controller to handle TUS uploads, including progress tracking and error handling. - Modified file upload form to conditionally include TUS client script and adjust upload settings based on TUS configuration. - Added new settings for TUS uploads in settings.yml, including max file size and TTL for incomplete uploads. - Configured recurring job for cleaning up TUS uploads. - Updated routes to include uploads management. This enhances the file upload experience by allowing resumable uploads for larger files.
- Updated error handling in `TusUploadsController` to specifically rescue `ArgumentError` during Base64 decoding. - Refactored `TusUploadStore` to use a block for file opening, improving resource management. - Added test support for TUS uploads in various test files, including integration and system tests. - Enhanced existing tests to validate TUS upload behavior, including metadata handling and response assertions. - Ensured proper setup and teardown of TUS-related settings in tests to maintain isolation and reliability.
- Added `valid_id?` method in `TusUploadStore` to validate client-supplied IDs, preventing path traversal vulnerabilities. - Updated `TusUploadsController` to return a 404 response for invalid upload IDs. - Refactored `append_chunk!` method to include file locking for better concurrency handling. - Enhanced test coverage for ID validation and error responses in both controller and service tests.
- Added a test to verify that a 404 response is returned when file pushes are disabled in the TUS uploads controller. - Ensured proper formatting by adding a newline at the end of the `TusUploadsController` file.
…gement - Updated the `require_tus_enabled` method in `TusUploadsController` to use the helper method for checking TUS uploads status. - Removed the obsolete `tus_enabled?` method to streamline the controller. - Modified the `multi_upload_controller.js` to ensure the single file input is only appended to the first row, preventing it from moving with each addition of selected files.
- Updated the `upload_url_for` method in `TusUploadsController` to enhance host and port handling, ensuring correct URL formation. - Removed the obsolete `tus_enabled?` method and replaced its usage with a helper method for checking TUS uploads status. - Adjusted the test for creating uploads to use the `Host` header, clarifying the request context and avoiding ambiguity in host/port resolution.
- Updated the `tus_uploads_enabled?` method in `ApplicationHelper` to directly check login and file push settings, removing the dependency on the `use_tus_uploads` setting. - Modified the `CleanupTusUploadsJob` to rely solely on the `enable_file_pushes` setting for execution. - Removed references to `use_tus_uploads` from tests and settings, streamlining the configuration for file uploads. - Enhanced tests to reflect the new logic for TUS uploads, ensuring clarity and correctness in behavior when file pushes are enabled or disabled.
- Updated the multi-upload controller to improve error handling by displaying error messages visibly in the UI. - Changed the configuration to use `maxTusSize` instead of `maxDirectSize` for file size limits. - Modified the file upload form to reflect the new TUS size limit in the footer message. - Adjusted tests to ensure the footer correctly displays the maximum file size when TUS is enabled.
- Added duration formatting for upload times to display how long uploads take. - Updated the multi-upload controller to handle progress details and finalizing states for TUS uploads. - Modified the file upload form to include a finalizing label and adjusted size display for TUS uploads. - Enhanced the selected file row template to show upload time alongside file names. - Improved the TUS upload row template to include size information during uploads.
…le when TUS is enabled - Changed test assertion to verify that the footer shows the maximum size per file when TUS uploads are active. - Adjusted the test description for clarity regarding the footer's behavior under TUS conditions.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements TUS-inspired chunked file upload support to enable large file uploads (up to tens of GB) for local storage without hitting HTTP body size limits. The implementation uses a resumable upload protocol with server-side chunk storage and cleanup.
Changes:
- Added
TusUploadStoreservice for managing temporary upload state and chunk storage with path traversal protection - Created
TusUploadsControllerto handle TUS protocol endpoints (POST create, PATCH append, HEAD status) - Implemented
CleanupTusUploadsJobrecurring job to remove stale incomplete uploads - Enhanced
multi_upload_controller.jswith TUS client integration, pause/resume functionality, and progress tracking - Added comprehensive test coverage including unit, integration, and system tests
- Configured new settings for TUS upload limits, TTL, and direct upload fallback
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/services/tus_upload_store.rb | Core service managing upload state, chunks, file locking, and stale upload cleanup |
| app/controllers/tus_uploads_controller.rb | HTTP endpoint implementing TUS protocol for chunked uploads |
| app/jobs/cleanup_tus_uploads_job.rb | Recurring job to remove incomplete uploads past TTL |
| app/javascript/controllers/multi_upload_controller.js | Frontend controller integrating TUS client with pause/resume and progress UI |
| app/helpers/application_helper.rb | Helper methods to check TUS availability |
| app/views/pushes/_files_form.html.erb | Updated file upload form with TUS support and CDN script loading |
| app/views/pushes/_tus_upload_row_template.html.erb | Progress bar template for resumable uploads |
| app/views/pushes/_direct_upload_row_template.html.erb | Progress bar template for direct uploads |
| app/views/pushes/_selected_file_row_template.html.erb | Template for completed file list items |
| config/settings.yml | New settings for TUS upload size limits and TTL |
| config/defaults/settings.yml | Default configuration values |
| config/routes.rb | Routes for TUS upload endpoints |
| config/routes/uploads.rb | TUS-specific route definitions |
| config/recurring.yml | Scheduled cleanup job configuration |
| test/unit/tus_upload_store_test.rb | Comprehensive unit tests for upload store |
| test/controllers/tus_uploads_controller_test.rb | Integration tests for TUS controller |
| test/unit/cleanup_tus_uploads_job_test.rb | Tests for cleanup job |
| test/helpers/application_helper_test.rb | Tests for TUS helper methods |
| test/integration/file_push/file_push_upload_ui_test.rb | Integration tests for file push UI |
| test/system/file_push_upload_ui_test.rb | End-to-end system tests |
| test/support/tus_upload_test_settings.rb | Shared test support module for settings management |
| test/test_helper.rb | Test helper updates to load support modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added a new `sanitize_upload_filename` method in `TusUploadsController` to ensure uploaded filenames are safe and properly formatted. - Updated the multi-upload controller to conditionally set the maximum file size based on TUS settings. - Modified the file upload form to include the TUS client script as a module for better compatibility. - Adjusted asset precompilation to include the new `tus.js` file. - Improved error handling in the `TusUploadsController` to return appropriate responses for missing uploads.
- Introduced tests in `TusUploadsControllerTest` to verify 404 and 410 responses when `finalize_to_blob!` raises `NotFound` and `ArgumentError`, respectively. - Added tests in `FilePushUploadUiTest` to ensure the file upload form includes the maximum direct upload size data attribute and that this size is configured correctly. - Implemented a test in `CleanupTusUploadsJobTest` to confirm that the cleanup job is scheduled in the recurring configuration for both production and development environments. - Enhanced `TusUploadStoreTest` with a test for concurrent calls to `append_chunk!`, ensuring proper handling of `OffsetMismatch` errors.
- Updated `TusUploadsController` to use relative URLs for the `Location` header, ensuring clients send PATCH requests to the same origin. - Implemented chunk size validation in the controller to return a 413 response for oversized chunks. - Introduced caching for finalized uploads to allow retries after finalization, returning appropriate headers. - Added a new helper method `parse_human_size` to convert human-readable sizes to bytes, and updated settings to allow configuration of TUS chunk size. - Enhanced the multi-upload controller and file upload form to support dynamic chunk size configuration. - Updated tests to verify new functionality, including chunk size parsing and retry behavior after finalization.
- Updated `TusUploadsController` to validate the `Upload-Offset` header, returning a 400 response for empty or invalid values. - Refactored the multi-upload controller to use bound methods for event listeners, enhancing readability and maintainability. - Added tests to ensure proper handling of empty `Upload-Offset` headers and validate response behavior for TUS uploads.
- Updated `TusUploadsController` to improve validation of the `Upload-Offset` header, ensuring it checks for blank or non-numeric values before processing. - Enhanced error handling to return a 400 response for invalid `Upload-Offset` inputs, maintaining robust upload functionality.
- Introduced a test to verify that creating uploads returns a 404 when logins are disabled. - Added multiple tests to ensure proper sanitization of filenames in Upload-Metadata, including handling of path traversal, special characters, spaces, and blank filenames. - Ensured that filenames are correctly processed and stored in ActiveStorage blobs, maintaining expected behavior across various scenarios.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Changed the file count variable from a global scope to an instance variable within the controller, improving encapsulation and preventing potential conflicts in multi-instance scenarios.
- Updated the wait time for TUS upload completion in both `file_push_upload_ui_test.rb` and `push_creation_workflows_test.rb` to ensure that the file row appears before assertions are made. - Adjusted the comments to clarify the purpose of the wait statements in relation to TUS uploads.
- Introduced a method to manually trigger the change event for file inputs in both `file_push_upload_ui_test.rb` and `push_creation_workflows_test.rb`. - This change ensures that the Stimulus controller handling file uploads correctly processes the attached files, addressing potential issues with the Capybara `attach_file` method not firing the change event. - Updated relevant test cases to call the new method after attaching files, improving the reliability of the tests.
- Updated the file count management to consistently use the instance variable `this.fileCount` instead of a local variable, ensuring accurate tracking of selected files across various methods. - This change enhances the encapsulation of file count logic within the controller, improving reliability in multi-instance scenarios.
- Updated the `TusUploadsController` to enforce maximum chunk size during uploads when Content-Length is absent, ensuring that only a limited number of bytes are processed. - Modified the `append_chunk!` method in `TusUploadStore` to respect both the upload length and an optional maximum byte limit, preventing excessive data writes during chunked transfers. - Added tests to verify that the controller and store correctly handle scenarios where the Content-Length header is missing, ensuring robust upload behavior.
- Moved the check for ongoing TUS uploads in the `update` action of `PushesController` to ensure proper error messaging when a user attempts to update a push with files still uploading. - Added a new test case to verify that users cannot update another user's push while a TUS upload is in progress, ensuring correct redirection and error handling.
- Added new packages: `buffer-from@1.1.2`, `combine-errors@3.0.3`, and `custom-error-instance@2.1.1`. - Updated lodash dependencies with new versions and added necessary integrity checks. - Ensured all new dependencies are resolved correctly from the Yarn registry.
Owner
|
- Removed the `tus_entry.js` file and updated the asset precompilation list to exclude `tus.js`. - Adjusted the inclusion of the TUS client in the `_files_form.html.erb` to use import maps for better module management. - Added the TUS client to the import map configuration for streamlined loading. These changes enhance the modularity of the TUS upload feature and improve asset handling in the application.
…ng behavior - Added a comment to the TUS client entry in the import map to clarify that the module should be loaded from the specified URL without integrity checks. - This change enhances the documentation within the configuration for better understanding of the loading process.
- Removed outdated comments and improved cache key formatting in the TUS uploads controller for clarity. - Consolidated helper functions related to upload progress and error handling by importing them from dedicated helper modules in the multi-upload controller. - Streamlined the creation of upload rows by utilizing a helper function to enhance code readability and maintainability. These changes enhance the organization of the upload logic and improve the overall structure of the codebase.
- Eliminated tests and settings related to TUS uploads to streamline the test suite. - This cleanup enhances the focus of the tests on relevant functionality and improves maintainability.
- Deleted the FORCE_SSL environment variable in the test setup to ensure a clean testing environment. - This change improves the reliability of tests by preventing unintended SSL enforcement during test execution.
…mits - Implemented user ownership validation for TUS uploads to ensure that only the creator can access or modify their uploads. - Added a limit on the number of concurrent uploads tracked in a session, returning a conflict response when the limit is exceeded. - Updated the TUS upload store to include user ID in metadata and adjusted related methods for consistency. - Enhanced tests to cover ownership checks and session limits, ensuring robust error handling and user-specific upload management. These changes improve security and user experience in the TUS upload process.
- Simplified the retrieval of the user ID in the TusUploadStore by using safe navigation to handle nil values. - Improved the teardown process in the TusUploadsControllerTest to ensure best-effort cleanup, enhancing test reliability. - Updated file removal logic to use safe navigation, preventing potential errors when checking for file existence. These changes contribute to cleaner code and more robust test cases.
Collaborator
Author
|
Enhance TUS upload handling with user ownership checks and increase readability of TusUploadController |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated the multi-upload controller to include a new message for temporarily disabled uploads. - Modified the associated HTML form to expose the new error message as a data attribute. - Added a test to verify that both the session full message and the new temporarily disabled message are correctly exposed in the multi-upload data attributes. These changes improve user feedback during the upload process.
- Added `aria-label` and `role` attributes to the trash link for improved accessibility. - These changes ensure that the remove file action is more user-friendly for assistive technologies.
- Eliminated the tusServerUploadId variable and its associated checks from the multi-upload controller. - This cleanup simplifies the code and removes unnecessary data handling, contributing to improved readability and maintainability.
…d responses and simplify finalized upload cache writing
…de clarity and maintainability.
Signed-off-by: Lukasz <108612299+LukasMalyszko@users.noreply.github.com>
|
|
||
| require "application_system_test_case" | ||
|
|
||
| class FilePushUploadUiTest < ApplicationSystemTestCase |
salimmolla7401-svg
suggested changes
May 6, 2026
|
|
||
| require "application_system_test_case" | ||
|
|
||
| class FilePushUploadUiTest < ApplicationSystemTestCase |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Support large file uploads (e.g. tens of GB) for local storage without hitting HTTP body limits. Use chunking so the server never receives the whole file in one request.
Related Issue
https://github.com/apnotic/pwpush-pro/issues/1474
Type of Change
Checklist
rake test)