[TSPS-156] Imputation Service UI [feature branch] #5325
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MatthewBemis
changed the title
|
MatthewBemis
changed the title
MatthewBemis
changed the title
| </div> | ||
| <ButtonPrimary | ||
| style={{ marginTop: '1.5rem' }} | ||
| href='https://allofus-anvil-imputation.terra.bio/' |
There was a problem hiding this comment.
I suspect we ultimately want this to link to documentation, but it's not public yet. What do you think about linking to pypi https://pypi.org/project/terralab-cli/ instead of the marketing page until the documentation is public?
There was a problem hiding this comment.
great idea, done in c73dfa7
|
|
||
| <h2 style={{ marginTop: '2rem' }}>User Documentation</h2> | ||
| <div style={{ marginTop: '1rem' }}> | ||
| <a href='services/pipelines' style={{ color: '#46A3E9', textDecoration: 'underline', fontWeight: 'bold' }}> |
There was a problem hiding this comment.
this is currently a dead page (same link in row 58)
There was a problem hiding this comment.
good catch, just updated these to be real paths. eventually we'll link out to documentation here once that's public
|
|
||
| export const navPaths = [ | ||
| { | ||
| name: 'pipelines-about', | ||
| path: '/pipelines/imputation', |
There was a problem hiding this comment.
it's a little confusing that '/pipelines' is not a page. could that redirect to '/pipelines/imputation' for now?
There was a problem hiding this comment.
good call! added this route in c73dfa7
| // If a pipeline doesn't have any tips, this widget won't be rendered at all. | ||
| // This supports ReactNodes in case you want to include links or other non-string elements in the tips. | ||
| array_imputation: [ | ||
| { id: 'vcf-valid', content: 'Ensure that your file is a valid vcf file' }, |
There was a problem hiding this comment.
| { id: 'vcf-valid', content: 'Ensure that your file is a valid vcf file' }, | |
| { id: 'vcf-valid', content: 'Ensure that your file is a valid VCF file containing array data (fewer than 10 million sites)' }, |
| { id: 'vcf-valid', content: 'Ensure that your file is a valid vcf file' }, | ||
| { id: 'grch38', content: 'VCFs must be generated from GRCh38/hg38' }, | ||
| { id: 'quota-check', content: 'Ensure your multi-sample file contains no more than your remaining quota.' }, | ||
| { id: 'format-guidelines', content: 'More guidelines for data formatting' }, |
There was a problem hiding this comment.
is this supposed to link to documentation?
There was a problem hiding this comment.
eventually! for context, these are all just placeholder tips from Lou's mocks. there's a ticket in the backlog to make these tips more useful https://broadworkbench.atlassian.net/browse/TSPS-525
Mind if I fold your suggested changes here into that ticket?
There was a problem hiding this comment.
sure, it makes sense to add to that ticket, thanks!
| array_imputation: [ | ||
| { id: 'vcf-valid', content: 'Ensure that your file is a valid vcf file' }, | ||
| { id: 'grch38', content: 'VCFs must be generated from GRCh38/hg38' }, | ||
| { id: 'quota-check', content: 'Ensure your multi-sample file contains no more than your remaining quota.' }, |
There was a problem hiding this comment.
| { id: 'quota-check', content: 'Ensure your multi-sample file contains no more than your remaining quota.' }, | |
| { id: 'quota-check', content: 'Ensure your multi-sample file contains no more samples than your remaining quota.' }, |
| // Add tips for new pipelines here, and they'll automatically be displayed. | ||
| // If a pipeline doesn't have any tips, this widget won't be rendered at all. | ||
| // This supports ReactNodes in case you want to include links or other non-string elements in the tips. | ||
| array_imputation: [ |
There was a problem hiding this comment.
can we add a tip that's like
{ id: 'all-chr', content: 'Your VCF should include data for all autosomes (chr1-chr22). Sex chromosomes are not processed.' },
| <div style={{ display: 'flex', flexDirection: 'column', gap: '0.25rem' }}> | ||
| <h3>Job History</h3> | ||
| <div style={{ marginBottom: '0.25rem' }}> | ||
| All files associated with jobs will be automatically deleted after 2 weeks from completion. |
There was a problem hiding this comment.
can we also (here or elsewhere) note that this includes error information? i.e. jobs older than 2 weeks won't display an error message. feel free to put this in a new ticket.
There was a problem hiding this comment.
Just filed https://broadworkbench.atlassian.net/browse/TSPS-581 for this!
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive UI for the Imputation Service within Terra, adding views and custom branding for Scientific Services. The imputation service allows users to run imputation jobs, monitor their status, and download outputs through a new Scientific Services interface.
Key changes include:
- Complete UI implementation for the Imputation Service with job submission, history, and output management
- New Scientific Services branding with custom styling, logos, and compact navigation
- Feature preview gate system for controlled access to the imputation UI
Reviewed Changes
Copilot reviewed 41 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/registration/Register.tsx | Conditionally hides Terra-specific registration sections for Scientific Services branding |
| src/pages/scientificServices/ | Complete imputation service UI with job submission, history, modals, and components |
| src/libs/brands.tsx | New Scientific Services brand configuration with custom styling and compact TopBar |
| src/components/TopBar.tsx | Support for compact navigation mode that hides non-essential menu items |
| src/components/FooterWrapper.js | Hide Terra-specific footer links for Scientific Services branding |
| src/libs/ajax/teaspoons/ | Complete API client implementation for Teaspoons service |
| config/ | Teaspoons service URL configuration for dev and prod environments |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| selectedUserInputs: Record<string, any>, | ||
| description: string, | ||
| pipelineInputs: PipelineInput[], | ||
| setUploadProgress: (percentage: Record<string, any>) => void = () => {} |
There was a problem hiding this comment.
The parameter name 'percentage' is misleading since the type is Record<string, any> which represents upload progress for multiple files, not a single percentage value. Consider renaming to 'uploadProgress' or 'progressMap' to better reflect its purpose.
| setUploadProgress: (percentage: Record<string, any>) => void = () => {} | |
| setUploadProgress: (uploadProgress: Record<string, any>) => void = () => {} |
Copilot uses AI. Check for mistakes.
| setPageNumber(v); | ||
| }} | ||
| itemsPerPage={itemsPerPage} | ||
| setItemsPerPage={(v) => { |
There was a problem hiding this comment.
Using @ts-ignore suppresses TypeScript errors without addressing the underlying issue. This should be replaced with proper type definitions or explicit type assertions to maintain type safety.
| setItemsPerPage={(v) => { | |
| <Paginator | |
| filteredDataLength={pipelineRunsResponse?.totalResults ?? 0 as number} | |
| unfilteredDataLength={pipelineRunsResponse?.totalResults ?? 0 as number} | |
| pageNumber={pageNumber as number} | |
| setPageNumber={(v: number) => { | |
| setPageNumber(v); | |
| }} | |
| itemsPerPage={itemsPerPage as number} | |
| setItemsPerPage={(v: number) => { |
Copilot uses AI. Check for mistakes.
| const { inputs } = await Teaspoons(signal).getPipelineDetails(pipelineName, pipelineVersion); | ||
| setPipelineInputs(inputs); | ||
| setIsLoading(false); | ||
| }); |
There was a problem hiding this comment.
Using map() with async functions creates promises that are not awaited, potentially causing race conditions. The pipeline details are fetched for all pipelines simultaneously but only the last one sets the state. Use Promise.all() or process pipelines sequentially to ensure proper state management.
| }); | |
| const pipelineDetails = await Promise.all( | |
| response.results.map(async (pipeline) => { | |
| const pipelineName = pipeline.pipelineName; | |
| const pipelineVersion = pipeline.pipelineVersion; | |
| const { inputs } = await Teaspoons(signal).getPipelineDetails(pipelineName, pipelineVersion); | |
| return { pipelineName, pipelineVersion, inputs }; | |
| }) | |
| ); | |
| // If you want to set inputs for all pipelines, you could store them in an object or array. | |
| // For now, set inputs for the first pipeline (or handle as needed). | |
| if (pipelineDetails.length > 0) { | |
| setPipelineInputs(pipelineDetails[0].inputs); | |
| } | |
| setIsLoading(false); |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| export const PipelineStringInput: React.FC<PipelineStringInputProps> = ({ input, value, onChange }) => { | ||
| const { label, placeholder, helpText } = INPUT_DESCRIPTIONS[input.name]; |
There was a problem hiding this comment.
Destructuring INPUT_DESCRIPTIONS[input.name] will throw an error if the input name doesn't exist in the mapping. Add a fallback or optional chaining: INPUT_DESCRIPTIONS[input.name] || {} to prevent runtime errors.
| const { label, placeholder, helpText } = INPUT_DESCRIPTIONS[input.name]; | |
| const { label, placeholder, helpText } = INPUT_DESCRIPTIONS[input.name] || {}; |
Copilot uses AI. Check for mistakes.
Co-authored-by: Matt Bemis <[email protected]>
…ry table styling (#5340)
… tooltip for running jobs (#5381)
MatthewBemis
force-pushed
the
tsps_imputation_ui_feature_branch
branch
from
d4396ad to
220bfb2
Compare
|
Jira Ticket: https://broadworkbench.atlassian.net/browse/TSPS-156
Adds views and custom branding within Terra UI to interact with the Imputation Service.
NOTE: These changes have almost all been reviewed in individual PRs, with the exception of the last 3 commits which were just small fixes for lint rules and configs.
Summary of changes:
What
Why
Testing strategy