feat: Allow downloading metadata in the sequence detail view #3003
Conversation
| // This type cast isn't pretty, but if the API would be typed correctly, the union type | ||
| // of the actual details resonse and the potential 'string' would polute the whole API, | ||
| // so I decided to just do this cast here. We know that the return value is a TSV string. | ||
| }).then((result) => result.map((data) => data as unknown as string)); |
There was a problem hiding this comment.
I would appreciate input on this. I have fiddled with the Zodios setup quite a bit, but reached the conclusion that this would be the most sensible way to handle the TSV data. Alternatives I considered:
- Get the JSON data from the API and turn it into TSV in somewhere in the backend server (not necessarily in this function). Pros: no casting required. Cons: duplicating the TSV functionality.
- Tried to modify the Zodios API definition and adjusted the Type so it can return a string as well. Pro: Typesafety possible. Cons: the 'string' edgecase polutes all the other regular usages of the endpoint and makes the code much more convoluted in many places.
I think with Zodios the best solution would be to distinguish json and TSV via endpoint, i.e. use a different endpoint alltogether like (sample/details/tsv - I know, not very RESTy but you get the idea). This way we could have a nicely typed TSV endpoint. But that requires a backend API change.
There was a problem hiding this comment.
I'm not the right person to comment on this - I don't know my zodios. @fengelniederhammer might have ideas
website/src/components/SequenceDetailsPage/SequencesDataTableTitle.astro
Show resolved
Hide resolved
| > | ||
| <span class='hidden sm:inline'>Download FASTA</span> | ||
| <span class='sm:hidden inline text-xl'> | ||
| showDownload && ( |
There was a problem hiding this comment.
Also maybe it would be nice to just reuse the react DownloadButton component here, but they handle responsiveness differently and are also styled slightly differently and for now I couldn't be bothered to "merge" the two buttons into a single component. If desired I can put some more work into this refactoring so we end up with a single component.
website/src/components/SequenceDetailsPage/SequencesDataTableTitle.astro
Show resolved
Hide resolved
fhennig
changed the title
| // We don't know which organism the accessionVersion belongs to, | ||
| // so we just try all of them until we get a success. | ||
| const organisms = getConfiguredOrganisms(); | ||
| const results = await Promise.all( |
There was a problem hiding this comment.
Not blocking because it's an existing issue but while reading I noticed that this Promise.all isn't ideal. If one LAPIS instance for one organism is unhealthy and takes 10 secs to respond it will make all organisms take 10 seconds to respond. We use better logic here:
There was a problem hiding this comment.
Thanks for the pointer, I've made the change: be67e74
corneliusroemer
force-pushed
the
feat/2019-download-individual-metadata
branch
from
be67e74
to
6aad7a7
Compare
fhennig
force-pushed
the
feat/2019-download-individual-metadata
branch
from
6aad7a7
to
24b164d
Compare
resolves #2019
preview URL: https://feat-2019-download-indivi.loculus.org/ [Edited by Theo: fyi this gets truncated so you probably need to look it up in ArgoCD - ping on Slack if you don't have the credentials to access that]
Summary
Turn the download button into a selection dropdown instead, where you can select to either download FASTA or a Metadata TSV.
Screenshot
PR Checklist