feat: include hashes in paired collections sent to galaxy (#470) #538
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds MD5 hashes to paired FASTQ collections sent to Galaxy and refactors configuration inputs to use a typed key/value system across the stepper and side-panel UI.
- Introduce
ConfiguredInputwith explicit fields and updateOnConfigureto accept typed tuples - Map side-panel entries via
STEPSandrenderValueinstead of storing labels inConfiguredInput - Enhance Galaxy API payloads to include file hashes and update all step components to use the new
STEP.keyandrenderValuepatterns
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
UseConfigureInputs/types.ts |
Define explicit ConfiguredInput and OnConfigureParams |
viewModelBuilders.ts |
Replace generic loop with STEPS+renderValue mapping |
galaxy-api.ts |
Accept EnaPairedReads[], include hashes in payload |
entities.ts |
Add EnaPairedReads & EnaFileInfo, define WorkflowDatasetHash |
stepper.tsx |
Switch to STEPS props, use STEP.key, drop entryKey |
Multiple Step/*/step.ts and related components |
Refactor each step to use typed STEP config and renderValue |
Numerous Step/*/* UI components |
Remove old entryKey/entryLabel props, use onConfigure(STEP.key, …) |
Comments suppressed due to low confidence (1)
app/utils/galaxy-api/galaxy-api.ts:146
- [nitpick] The error message could include the actual lengths to aid debugging, e.g.
Hash list length ${splitMd5Hashes.length} does not match URL list length ${splitUrls.length}.
if (splitMd5Hashes.length !== splitUrls.length)
throw new Error("Hash list has different length than URL list");
| return ( | ||
| <Step | ||
| key={key} | ||
| key={i} |
There was a problem hiding this comment.
Using the array index as a React key can lead to rendering issues if the steps array ever changes order; consider using a stable identifier such as STEP.key or label.
| key={i} | |
| key={label} |
There was a problem hiding this comment.
I think i should work fine here since it's an index in a constant and indices are established as a way to identify the steps elsewhere in the code
| * Possible tuples containing a key and its associated value type. | ||
| */ | ||
| export type OnConfigureParams = { | ||
| [K in keyof Required<ConfiguredInput>]: [K, ConfiguredInput[K]]; |
There was a problem hiding this comment.
[nitpick] The Required<ConfiguredInput> wrapper is unnecessary since it has the same keys as ConfiguredInput; consider simplifying to keyof ConfiguredInput for clarity.
| [K in keyof Required<ConfiguredInput>]: [K, ConfiguredInput[K]]; | |
| [K in keyof ConfiguredInput]: [K, ConfiguredInput[K]]; |
There was a problem hiding this comment.
These are not in fact equivalent
| }>; | ||
| } | ||
|
|
||
| interface WorkflowDatasetHash { |
There was a problem hiding this comment.
This interface is referenced in public types but not exported; consider adding export so it can be used externally if needed.
| interface WorkflowDatasetHash { | |
| export interface WorkflowDatasetHash { |
| const splitUrls = enaUrls.split(";"); | ||
| const splitMd5Hashes = enaMd5Hashes.split(";"); | ||
| if (splitMd5Hashes.length !== splitUrls.length) | ||
| throw new Error("Hash list has different length than URL list"); |
There was a problem hiding this comment.
Is that gracefully handled somewhere in the UI ? Will a user have an alternative ? We could also just render a warning but allow using the files anyway, it's not a critical requirement for functionality, but a nice to have that allows us to do some optimizations on the galaxy backend.
There was a problem hiding this comment.
There's no special handling for the different errors that might be thrown, but the page content will be replaced with an error message if one occurs, so the user will at least know that something has happened and be able to reload the page to try something else
| function getPairedRunUrlsInfo(enaUrls: string): { | ||
| forwardUrl: string; | ||
| reverseUrl: string; | ||
| function getPairedRunUrlsInfo( |
There was a problem hiding this comment.
generally speaking it might be good to place ENA parsing in a separate file ? I guess I wouldn't look for this in galaxy-api.ts ?
There was a problem hiding this comment.
Yeah, I agree -- I didn't do that in this PR because we already had getPairedRunUrlsInfo in galaxy-api.ts and moving it seemed out-of-scope, so I've just opened #547 to cover separating out the ENA stuff
frano-m
left a comment
frano-m
left a comment
There was a problem hiding this comment.
LGTM thank you @hunterckx 🌭
Closes #470
Passing the hashes through from the stepper to the Galaxy API requires some changes to how the steps work:
ConfiguredInputnow contains a specific set of fields which may have different types, rather than containing arbitrary entries all of the same type.ConfiguredInput-- instead, the view model builder for the side panel maps over theSTEPSarray, and for each step uses thelabelfield to determine the entry label, and the function in therenderValuefield to determine how the value is displayed. (Note: I believe this is the one place where the new system is less powerful than the old one -- with the new system, there's a one-to-one correspondence between steps and side panel entries.)onConfigurejust takes a key and a value. Since the values may be different types, this requires the type system to know what the key is, so keys are referenced via the step config constant rather than being passed to the step component as a prop.