Skip to content

Conversation

@MatthewBemis
Copy link
Member

@MatthewBemis MatthewBemis commented Jun 9, 2025

Jira Ticket: https://broadworkbench.atlassian.net/browse/TSPS-512 and https://broadworkbench.atlassian.net/browse/TSPS-514

Summary of changes:

What

  • Adds the "Deletion Date" column
  • Implements the View Error and View Outputs modals for the "Actions" column

screencapture-localhost-3000-2025-06-09-20_24_23

screencapture-localhost-3000-2025-06-09-20_24_39

screencapture-localhost-3000-2025-06-09-20_24_46

Why

Testing strategy

<div style={{ fontWeight: 'bold', marginTop: '2rem' }}>Our current offerings:</div>
<div>
<span style={{ fontStyle: 'italic' }}>All of Us</span> + AnVIL Imputation Service
<a
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an unrelated drive-by change to link to the marketing page from the landing page

@MatthewBemis MatthewBemis requested a review from Copilot June 9, 2025 22:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds modal components to display pipeline outputs and errors, updates the job history table to include a deletion date column, and adjusts styles and API models to support these features. Key changes include:

  • Introducing ViewOutputsModal and ViewErrorModal components for displaying pipeline outputs and error details.
  • Updating JobHistory to include a new column for data deletion date and new action buttons that trigger the appropriate modals.
  • Adding new API interfaces and a method in Teaspoons for retrieving pipeline run results.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/pages/scientificServices/pipelines/views/modals/ViewOutputsModal.tsx New modal for showing pipeline output files with download support.
src/pages/scientificServices/pipelines/views/modals/ViewErrorModal.tsx New modal for displaying detailed pipeline error messages and causes.
src/pages/scientificServices/pipelines/views/JobHistory.tsx Updated job history table with a deletion date column and action buttons triggering modals.
src/pages/scientificServices/landingPage/ScientificServicesDescription.tsx Modified link element for external service with proper accessibility attributes.
src/libs/ajax/teaspoons/teaspoons-models.ts Added new interfaces to support detailed pipeline run reporting.
src/libs/ajax/teaspoons/Teaspoons.ts Introduced getPipelineRunResult API method for fetching pipeline run results.

Comment on lines +266 to +271
const outputsModal = useModalHandler(() => {
return <ViewOutputsModal jobId={pipelineRun.jobId} onDismiss={outputsModal.close} />;
});

const errorModal = useModalHandler(() => {
return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />;
Copy link

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider defining the onDismiss callback outside of the modal handler to avoid a self-referential closure, which can improve code clarity.

Suggested change
const outputsModal = useModalHandler(() => {
return <ViewOutputsModal jobId={pipelineRun.jobId} onDismiss={outputsModal.close} />;
});
const errorModal = useModalHandler(() => {
return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />;
const onOutputsModalDismiss = () => outputsModal.close();
const outputsModal = useModalHandler(() => {
return <ViewOutputsModal jobId={pipelineRun.jobId} onDismiss={onOutputsModalDismiss} />;
});
const onErrorModalDismiss = () => errorModal.close();
const errorModal = useModalHandler(() => {
return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={onErrorModalDismiss} />;

Copilot uses AI. Check for mistakes.
Comment on lines +270 to +271
const errorModal = useModalHandler(() => {
return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />;
Copy link

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider refactoring the onDismiss callback so that it does not reference errorModal directly within its own initialization, enhancing readability.

Suggested change
const errorModal = useModalHandler(() => {
return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />;
const handleErrorModalDismiss = () => {
errorModal.close();
};
const errorModal = useModalHandler(() => {
return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={handleErrorModalDismiss} />;

Copilot uses AI. Check for mistakes.
@MatthewBemis MatthewBemis marked this pull request as ready for review June 10, 2025 00:23
Copy link

@jsotobroad jsotobroad left a comment

Choose a reason for hiding this comment

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

i really like the download outputs modal

<h3>Job History</h3>
<div>All files associated with jobs will be automatically deleted after 2 weeks from completed.</div>
<div style={{ marginBottom: '0.25rem' }}>
All files associated with jobs will be automatically deleted after 2 weeks from completed.

Choose a reason for hiding this comment

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

after 2 weeks from completed -> 2 weeks after completion ?


const completionDate = new Date(pipelineRun?.timeCompleted);
const deletionDate = new Date(completionDate);
deletionDate.setDate(deletionDate.getDate() + 14);

Choose a reason for hiding this comment

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

this is something we already return as part of the PipelineRunReport which you would get from a call to /result on a successful pipeline run. I guess we should decide if we want to only show them this date after they click on the download button in the overlay or do this math here and the response's time in the overlay or show them the response's time as part of the list pipeline runs call

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yeah, i went back and forth on this. I think since it's related to permanently deleting data that they paid to generate, it's good UX to have it here and in the download outputs modal. I also think we should provide an additional visual cue in the cell if the data is facing imminent deletion (perhaps <72 hours away from deletion).

I didn't want to scope-explode so I just added the hardcoded +14 days here, but i guess we could take one of these approaches in a follow-up ticket:

  1. add the deletion date to the listPipelineRuns call, like you suggested (I'm leaning towards this)
  2. return the TTL config value somewhere so we can do the math here based on that, and we could also programmatically generate the "All files associated with jobs will be automatically deleted after X days from completion" text based on that value

I guess it also depends on if we ever think we'd change the global TTL, or perhaps have different TTLs per pipeline

Choose a reason for hiding this comment

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

yea i woudl say lets make a followup ticket to discuss what UX we want around this and the rest of this looks good to me

</ButtonPrimary>
</div>
))}
{result.pipelineRunReport.outputExpirationDate && (
Copy link
Member Author

Choose a reason for hiding this comment

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

note to self: it would probably be good to add some logic here to check if the expiration date has passed, and disable the download buttons + show different text

Copy link
Member Author

Choose a reason for hiding this comment

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

(will also cover this in a followup UX ticket, since it requires further discussion)

@sonarqubecloud
Copy link

@MatthewBemis MatthewBemis merged commit f30aab7 into tsps_imputation_ui_feature_branch Jun 12, 2025
8 of 9 checks passed
@MatthewBemis MatthewBemis deleted the mb-tsps-512 branch June 12, 2025 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants