Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make core API call non-blocking #454

Merged

Conversation

jfkonecn
Copy link

@jfkonecn jfkonecn commented Oct 3, 2024

closes oncokb/oncokb-pipeline#569

I added the feature flag so I wouldn't miss a spot when I revert this change.

closes oncokb/oncokb-pipeline#570

I added a loading indicator as well.

zhx828 and others added 27 commits August 22, 2024 08:56
The completedDate in the DB could be ISO 8601 string, or json object. I think this has to do with the recent spring boot upgrade, but I can't find the specific version of the change. I will handle both scenarios for now
Handle when Instant json element is a json object
The getSession is an async method which updates the loading status.
Avoid fetching management info repeatedly
* Added Heap

* Updated CSP
* allow comma in mutation name when transcripts present

* add missing colon

* rename transcripts to reference genomes
@jfkonecn jfkonecn requested a review from zhx828 October 3, 2024 17:41
@@ -206,6 +206,7 @@ const ReviewPage: React.FunctionComponent<IReviewPageProps> = (props: IReviewPag
</Row>
</>
)}
{(isAcceptingAll || isAccepting) && <LoadingIndicator key={'curation-page-loading'} size={LoaderSize.LARGE} center isLoading />}
Copy link
Member

Choose a reason for hiding this comment

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

This is new to me. The loader is on top of the review page. What's your thought on this?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it's at the top. I have no real preference on this. You have a better eye for UI than I do.

Copy link
Member

Choose a reason for hiding this comment

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

We could hide all review content when accepting all, and show a loader on the white background. Could you try that? Or without.

@@ -0,0 +1 @@
export const STOP_REVIEW_IF_CORE_SUBMISSION_FAILS: boolean = false;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in appConfig.ts?

Copy link
Author

Choose a reason for hiding this comment

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

I think you could make an argument either way for this. If you feel like you'll want to switch the behavior without creating a new build, then yes. If you feel like we'll make a code change first then rolls this out, then it doesn't make a different. This is entirely for myself to track the places I need to rollback.

Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to AppConfig to consolidate.

@jfkonecn
Copy link
Author

jfkonecn commented Oct 14, 2024

@zhx828 I addressed your comments.
I also decided to fix the merge conflicts here as well since this is probably going to be the last PR before this goes into RC.

Put feature flag in app config
298f9c8

Now showing loading icon on accept all
5ebc9a8

@jfkonecn jfkonecn requested a review from zhx828 October 14, 2024 14:27
Copy link
Member

@zhx828 zhx828 left a comment

Choose a reason for hiding this comment

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

LGTM

@jfkonecn jfkonecn merged commit d314a92 into oncokb:feature/submit-to-core Oct 14, 2024
2 checks passed
@jfkonecn jfkonecn deleted the feature/fix-submit-to-core branch October 14, 2024 14:33
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