Skip to content

Conversation

@chaoran-chen
Copy link
Member

@chaoran-chen chaoran-chen commented Jan 11, 2026

resolves #5818

This PR adds the upload of extra files for revisions, supporting both bulk and the form-based, single-sequence revision.

Screenshot

Bulk revision screenshot
Single revision screenshot2

PR Checklist

  • [ ] All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
    • Two integration tests have been added to test single and bulk uploads.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)
    • Tested a bulk revision and a single-sequence revision in the preview.

🚀 Preview: https://files-bulk-revision.loculus.org

@claude
Copy link
Contributor

claude bot commented Jan 11, 2026

Claude finished @chaoran-chen's task in 2m 31s —— View job


Code Review Complete ✅

I've completed a comprehensive review of this PR. The implementation successfully adds file upload functionality to revisions, which is a valuable feature. The code is generally well-structured with good test coverage.

Summary

This PR adds file upload support to the revision workflow, allowing users to attach files when revising sequences. The implementation extends existing file upload infrastructure to work with both single and bulk revisions.

Key Changes:

  • Added file upload support to EditPage for single sequence revisions
  • Extended bulk revision workflow to handle file uploads
  • Refactored stringifyFileMapping into a reusable function
  • Added comprehensive integration tests covering single and bulk revision scenarios with files
  • Fixed parameter passing by changing group prop to groupId in ExtraFilesUpload

Issues Found

Code Quality - Minor Issues:

  1. Validation Gap (EditPage.tsx:93, DataUploadForm.tsx:105): The code accesses Object.values(fileMapping)[0] without validating that the object isn't empty. While fileMapping !== undefined is checked, an empty object {} would pass this check and cause Object.values(fileMapping)[0] to return undefined. Consider adding Object.keys(fileMapping).length > 0 check.

  2. Error Handling (file-upload-helpers.ts:55): Using void to discard the promise makes error handling implicit. While technically correct, explicit promise handling would be clearer and more maintainable.

Positive Observations

Excellent Test Coverage: The new tests in file-sharing.spec.ts are comprehensive:

  • Single sequence upload with files
  • Bulk upload with different file counts per sequence
  • Discard and re-add workflow
  • Large file multipart upload (35 MB)
  • Bulk revisions with files
  • Single revision via edit page

Good Refactoring: Extracting stringifyFileMapping reduces code duplication and makes the transform logic reusable and testable.

Proper TypeScript Usage: Added the missing UploadFiles type export which improves type safety.

Clean Architecture: The ExtraFilesUpload component is properly exported and reused in EditPage, following DRY principles.

Edge Case Testing: The test that verifies discarding and re-adding files is excellent for catching regression bugs.

Recommendations

While the issues found are minor and the PR is solid overall, I recommend:

  1. Add the empty object validation before accessing Object.values(fileMapping)[0] in both EditPage.tsx and DataUploadForm.tsx
  2. Consider extracting the multipart upload chunk size (10 MB) as a shared constant to avoid magic numbers in tests
  3. The PR checklist mentions documentation and manual testing - ensure these are completed before merging

Security & Performance

✅ No security vulnerabilities identified
✅ Performance appears appropriate for the use case
✅ Multipart upload implementation correctly handles large files

@theosanderson
Copy link
Member

Apologies for the unexpected review - will figure out why

@chaoran-chen chaoran-chen force-pushed the files-bulk-revision branch 3 times, most recently from 020c989 to 245a6b3 Compare January 11, 2026 18:43
@chaoran-chen chaoran-chen added the preview Triggers a deployment to argocd label Jan 11, 2026
@chaoran-chen chaoran-chen marked this pull request as ready for review January 11, 2026 22:00
@claude
Copy link
Contributor

claude bot commented Jan 11, 2026

Claude finished @chaoran-chen's task in 2m 38s —— View job


Code Review Complete ✅

I've completed a comprehensive review of this PR which adds file upload functionality to revisions, supporting both bulk and single-sequence workflows.

Summary

This PR successfully extends the file upload infrastructure to support revisions. The implementation is well-structured with good test coverage. The code quality is generally good, though there are a few minor issues worth addressing.

Issues Found

Code Quality - Minor Issues

1. Validation Gap (EditPage.tsx:93, DataUploadForm.tsx:105)
The code accesses Object.values(fileMapping)[0] without validating that the object isn't empty. While fileMapping !== undefined is checked, an empty object {} would pass this check and cause Object.values(fileMapping)[0] to return undefined. See inline comments for suggested fixes.

2. Error Handling (file-upload-helpers.ts:55)
Using void to discard the promise makes error handling implicit. While technically correct, explicit promise handling would be clearer. See inline comment for suggested improvement.

Positive Observations

Excellent Test Coverage: The integration tests in file-sharing.spec.ts are comprehensive:

  • Single sequence upload with files
  • Bulk upload with different file counts per sequence
  • Discard and re-add workflow
  • Large file multipart upload (35 MB)
  • Bulk revisions with files
  • Single revision via edit page

Good Refactoring: Extracting stringifyFileMapping reduces code duplication and makes the transform logic reusable and testable (backendApi.ts:23-26).

Proper TypeScript Usage: The UploadFiles type export was added, improving type safety across the codebase.

Clean Architecture: The ExtraFilesUpload component is properly exported and reused in EditPage, following DRY principles.

Edge Case Testing: The test that verifies discarding and re-adding files is excellent for catching regression bugs.

Multipart Upload Support: The 35 MB file test validates that large file uploads work correctly with the multipart upload implementation.

Security & Performance

✅ No security vulnerabilities identified
✅ Performance appears appropriate for the use case
✅ Multipart upload implementation correctly handles large files

Recommendations

While the issues found are minor and the PR is solid overall, I recommend:

  1. Add empty object validation before accessing Object.values(fileMapping)[0] in both EditPage.tsx and DataUploadForm.tsx
  2. Consider using explicit Promise.all() instead of void for the file upload promise in file-upload-helpers.ts for better error visibility
  3. The PR checklist mentions documentation - ensure any relevant user-facing documentation is updated if needed

Overall Assessment

This is a well-implemented feature with strong test coverage. The code follows existing patterns and integrates cleanly with the current architecture. The minor issues identified don't block the PR but would improve code robustness if addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Revise with uploading new files on website

3 participants