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

feat(15128): load and error state for modelArtifact on mr version detail #3515

Conversation

gitdallas
Copy link
Contributor

@gitdallas gitdallas commented Nov 23, 2024

closes: https://issues.redhat.com/browse/RHOAIENG-15128

Description

add loading spinner and error for the loading of modelArtifacts on mr version details page.

show spinner on full page as the loading doesn't take long and determines some of the page format
image

show error in the model location area
image

How Has This Been Tested?

you can see the spinner briefly if you refresh the page. otherwise i was hard coding an error value to see that.

Test Impact

had to add an intercept to make sure the right side of mr version detail loads after selecting a different version #

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@gitdallas gitdallas force-pushed the feat/15128-load-and-error-state-artifact branch from f29e98a to 6b40a7a Compare November 23, 2024 00:10
@gitdallas
Copy link
Contributor Author

there was still some discussion on how this should look, but i am planning to be out next week and wanted to get something in

https://issues.redhat.com/browse/RHOAIENG-15128?focusedId=26139642&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-26139642

@gitdallas gitdallas force-pushed the feat/15128-load-and-error-state-artifact branch 3 times, most recently from 04e400f to 0db253c Compare November 23, 2024 15:28
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.56%. Comparing base (a24aabc) to head (254369c).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx 86.20% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3515      +/-   ##
==========================================
- Coverage   85.57%   85.56%   -0.02%     
==========================================
  Files        1372     1372              
  Lines       31211    31216       +5     
  Branches     8741     8744       +3     
==========================================
- Hits        26710    26709       -1     
- Misses       4501     4507       +6     
Files with missing lines Coverage Δ
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx 86.66% <86.20%> (-8.34%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a24aabc...254369c. Read the comment docs.

@gitdallas gitdallas force-pushed the feat/15128-load-and-error-state-artifact branch 2 times, most recently from c5b8332 to fc396c3 Compare December 2, 2024 19:56
Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

/lgtm

I can see the loading state in MR version details page.. wondering whether we are doing the same for Model Details page 🤔 - that will require a different ticket I guess.. this is good to go.

Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @gitdallas

@mturley
Copy link
Contributor

mturley commented Dec 5, 2024

@manaswinidas

wondering whether we are doing the same for Model Details page 🤔 - that will require a different ticket I guess

I wanted to double check and I think we're good on the model details page. It doesn't need to load any extra data, it just uses the registeredModel passed into it. The loading/error states for that are handled all the way up at the ModelVersions component (despite the name, that's the whole page for a model) before the model is passed into ModelVersionsTabs and then ModelDetailsView.

@mturley
Copy link
Contributor

mturley commented Dec 6, 2024

/retest

Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com>
@gitdallas gitdallas force-pushed the feat/15128-load-and-error-state-artifact branch from fc396c3 to 254369c Compare December 9, 2024 19:04
@openshift-ci openshift-ci bot removed the lgtm label Dec 9, 2024
Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

Thanks @gitdallas

@openshift-ci openshift-ci bot added the lgtm label Dec 10, 2024
Copy link
Contributor

openshift-ci bot commented Dec 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: manaswinidas, mturley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit e441ba1 into opendatahub-io:main Dec 10, 2024
6 checks passed
ConorOM1 pushed a commit to ConorOM1/odh-dashboard that referenced this pull request Dec 12, 2024
…ail (opendatahub-io#3515)

Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com>
ConorOM1 pushed a commit to ConorOM1/odh-dashboard that referenced this pull request Dec 12, 2024
…ail (opendatahub-io#3515)

Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants