Skip to content

fix(website): Fix applying file mappings for compressed metadata files - #7119

Draft
tombch wants to merge 10 commits into
mainfrom
fix-file-mapping-compressed-uploads
Draft

fix(website): Fix applying file mappings for compressed metadata files#7119
tombch wants to merge 10 commits into
mainfrom
fix-file-mapping-compressed-uploads

Conversation

@tombch

@tombch tombch commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

resolves #7120

Summary

  • Adjacent issue found from reviewing fix(website): report unreadable metadata files instead of hanging on "still being processed" #7108.
  • For organisms with files enabled, and for compressed metadata files with file uploads, the decompressed file was not passed by the file factory, leading to applyFileMappings to fail at writing to the file (as it cannot see the metadata headers) and the submission unable to continue.
  • This PR updates the file factory logic to also check if files are enabled, and if so, decompresses the file for applyFileMappings to later read correctly.
  • I also added an integration test for compressed metadata with files.

Notes

  • This is a simple fix but I feel like these functions need further refactoring. Ideally, the preparation of the metadata and sequence files should be in one place, rather than have some processing occur in the files factory in FormOrUploadWrapper, and further processing occur in the DataUploadForm's handleSubmit function, which silently depends on actions like making sure the file is decompressed in the files factory. So I'd be happy to expand on this if we don't think this is a sufficient fix for now.

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

@claude claude Bot added the website Tasks related to the web application label Aug 20, 2026
Comment thread integration-tests/tests/specs/features/file-sharing.spec.ts Outdated
@tombch
tombch marked this pull request as ready for review August 26, 2026 14:48
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@tombch

tombch commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

@claude review (please)

@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tombch's task in 2m 59s —— View job


Code review

The fix is correct: with files enabled and no column mapping, the factory previously handed the still-compressed .gz/.zst file to applyFileMappings, which reads it with File.text() and sees gzip bytes instead of TSV headers. Materialising the decompressed text solves that, and it lines up with what ColumnMapping.applyTo already does (returns a plain remapped.tsv). Restructuring the metadataFile === undefined check to come first and wrapping the read in try/catch is a genuine improvement — text() can throw (CompressedFile.text() throws on unknown extensions) and that rejection was previously unhandled.

Four points, all inline:

  • The new branch fires for every bulk upload when files are enabled, not just compressed ones — that means a redundant full copy for plain .tsv/Excel input, and an uncompressed upload to the backend when the file mapping turns out empty (applyFileMappings short-circuits at fileMapping.ts:474). Gating on metadataFile instanceof CompressedFile keeps the fix and avoids both.
  • The metadata file is now decompressed twice per submission (once in the mapping effect, once in the factory), synchronously on the main thread.
  • The compressed variant of the linkage-error test doesn't exercise the fixed path — it passes with or without the fix, for ~3 min of CI.
  • A unit test in the existing FormOrUploadWrapper.spec.tsx harness would pin this regression in milliseconds rather than minutes.

Nothing security-relevant here; no new input reaches the backend unvalidated, and submission.page.ts is test-only. I agree with your note that the split of metadata preparation between FormOrUploadWrapper and DataUploadForm.handleSubmit is the underlying problem — the implicit "the factory must have decompressed this already" contract is exactly what broke. Threading the ProcessedFile (rather than a materialised File) into handleSubmit would remove the class of bug, but that's fine as a follow-up.

CI on this branch is currently mostly startup_failure/infra noise, so I couldn't confirm type checks or tests from the run results.

  • Gather PR context / diff
  • Review FormOrUploadWrapper.tsx fix
  • Review integration test changes
  • Post review

No code changes were made — review only.
· fix-file-mapping-compressed-uploads

Comment on lines +149 to +155
} else if (extraFilesEnabled) {
// Ensure the metadata file is decompressed so that any file mappings
// can be applied correctly
mFile = new File([await metadataFile.text()], 'metadata.tsv');
} else {
mFile = metadataFile.inner();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch fires for every bulk upload when files are enabled, not just compressed ones, so it has two side effects beyond the bug being fixed:

  • An already-plain .tsv (or an ExcelFile, whose inner() is already a converted TSV) gets fully buffered into a JS string and copied into a new File for no reason.
  • If the user uploaded a compressed metadata file but no file mappings end up resolved, applyFileMappings returns the file unchanged (fileMapping.ts:474), so we now upload the metadata decompressed to the backend where it used to go over the wire gzipped.

CompressedFile is the only ProcessedFile whose inner() differs from text(), so gating on it keeps the fix and avoids both:

Suggested change
} else if (extraFilesEnabled) {
// Ensure the metadata file is decompressed so that any file mappings
// can be applied correctly
mFile = new File([await metadataFile.text()], 'metadata.tsv');
} else {
mFile = metadataFile.inner();
}
} else if (extraFilesEnabled && metadataFile instanceof CompressedFile) {
// Ensure the metadata file is decompressed so that any file mappings
// can be applied correctly
mFile = new File([await metadataFile.text()], 'metadata.tsv');
} else {
mFile = metadataFile.inner();
}

(needs CompressedFile added to the import from ./FileUpload/fileProcessing).

} else if (extraFilesEnabled) {
// Ensure the metadata file is decompressed so that any file mappings
// can be applied correctly
mFile = new File([await metadataFile.text()], 'metadata.tsv');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the same file is decompressed twice per submission: the effect above (line 90-92) already calls metadataFile.text() to build the submission file mapping, and this runs it again at submit time. gzip/zstd decompression here is synchronous (fflate.decompressSync / fzstd.decompress in fileProcessing.ts), so for a large metadata file that's two main-thread stalls. Caching the decompressed text next to the mapping state (or threading the ProcessedFile through to DataUploadForm instead of materialising a File here, as your PR description suggests) would fix this along with the layering issue.

Comment on lines +248 to +251
METADATA_COMPRESSION_CASES.forEach(({ compressMetadata, description }) => {
test(
'bulk submit blocks a submission with errors in file linkage or parsing' + description,
async ({ page, groupId, tmpDir }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test never reaches the code path the PR fixes: all four cases bail out in DataUploadForm.handleSubmit at the mapping/linkage error checks before applyFileMappings writes anything, and the mapping parse path already used metadataFile.text() (so it decompressed correctly before this fix too). The compressed variant here therefore passes both with and without the fix, while costing another ~3 min of CI on top of the extra run of the 240 s test above.

Consider parameterising only the success-path test at line 136 and leaving this one single-cased.

Comment on lines +41 to +45
const METADATA_COMPRESSION_CASES = [
{ compressMetadata: false, description: '' },
{ compressMetadata: true, description: ' with compressed metadata file' },
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regression itself is entirely client-side (the file factory in FormOrUploadWrapper), so it can be pinned much more cheaply than by re-running a 240 s end-to-end submission. FormOrUploadWrapper.spec.tsx already has a MockSaveWrapper harness that calls the file factory directly; a bulk-mode case that uploads a gzipSynced TSV with files.enabled and asserts the returned metadataFile.text() contains the headers would fail before this fix and run in milliseconds. Worth adding regardless of whether you keep the Playwright parameterisation.

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

Labels

website Tasks related to the web application

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If file sharing is enabled, bulk submission of compressed metadata with files is broken

2 participants