Render loading/error states on catalog and details pages #3770

YuliaKrimerman · 2025-02-18T15:38:59Z

Closes https://issues.redhat.com/browse/RHOAIENG-18960

Description

Added empty and loading states for both Model Catalog and Model Catalog Details page.

Screen.Recording.2025-02-14.at.11.34.25.AM.mov

How Has This Been Tested?

Test Impact

Added test cases to test all scenarios :

if the configmap is missing (intercept responds 404, you can see other examples we talked about earlier for that), we show the empty state
-if there is a non-404 error loading the configmap, we show an error message
-if the configmap is present but has empty data (data: { modelCatalogSource: '[]' }), we show the empty state
-if the configmap is present but has malformed data / bad JSON (data: { modelCatalogSource: 'invalid JSON here' } , we show the empty state
-if the configmap is present and has good data (the intercept you have now), we show the actual model details / cards

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

codecov · 2025-02-19T16:18:59Z

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.70%. Comparing base (7fbab31) to head (f015fec).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/concepts/modelCatalog/useModelCatalogSources.ts	88.88%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3770      +/-   ##
==========================================
+ Coverage   84.34%   84.70%   +0.36%     
==========================================
  Files        1487     1512      +25     
  Lines       34078    34970     +892     
  Branches     9440     9796     +356     
==========================================
+ Hits        28744    29623     +879     
- Misses       5334     5347      +13

Files with missing lines	Coverage Δ
...nd/src/pages/modelCatalog/screens/ModelCatalog.tsx	`100.00% <100.00%> (ø)`
...rc/pages/modelCatalog/screens/ModelDetailsPage.tsx	`100.00% <100.00%> (ø)`
...rc/concepts/modelCatalog/useModelCatalogSources.ts	`88.46% <88.88%> (+9.89%)`	⬆️

... and 133 files 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 0aab29e...f015fec. Read the comment docs.

alexcreasy

Great work! I'd remove the linter overrides though :)

I haven't tested this yet, I'll run it against a cluster this afternoon.

alexcreasy · 2025-02-19T16:37:41Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalog.cy.ts

+    modelCatalog.findModelCatalogEmptyState().should('exist');
+  });
+
+  it('should show empty state when configmap has malformed data', () => {


I'm curious why we show the empty state rather than the error state when the configmap is garbage?

@mturley I think he can answer this better

Hmm. Honestly it probably does make sense to render an error if there is malformed JSON... I'm not sure what I was thinking there 👀 maybe that it might be an ugly error? I think I'm just used to always making sure to catch JSON.parse exceptions, but in this case since it's in a fetch callback letting that error be thrown is probably a good idea so the admin messing with the configmap can see what they did wrong.

Sorry @YuliaKrimerman .... can you just remove the innermost try/catch around the JSON.parse? We can proceed as if it succeeded and let an exception there cause the fetch promise to be rejected. We should also update that test case to look for the rendered error.

alexcreasy · 2025-02-20T11:22:31Z

frontend/src/concepts/modelCatalog/useModelCatalogSources.ts

-      // Swallow JSON parse errors and return empty array for temporary summit implementation
-      return [];
+      const configMap = await getConfigMap(dashboardNamespace, MODEL_CATALOG_SOURCE_CONFIGMAP);
+      /* eslint-enable @typescript-eslint/no-unsafe-member-access */


Curious as to why this linter override is necessary?

alexcreasy · 2025-02-20T11:23:19Z

frontend/src/concepts/modelCatalog/useModelCatalogSources.ts

+        // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+        const source = JSON.parse(configMap.data.modelCatalogSource) as ModelCatalogSource;


You don't need this linter override here if you specify the type of source.

Suggested change

// eslint-disable-next-line @typescript-eslint/consistent-type-assertions

const source = JSON.parse(configMap.data.modelCatalogSource) as ModelCatalogSource;

const source: ModelCatalogSource = JSON.parse(configMap.data.modelCatalogSource);

alexcreasy · 2025-02-20T11:39:23Z

frontend/src/concepts/modelCatalog/useModelCatalogSources.ts

+      if (
+        // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+        'statusObject' in (e as object) &&
+        // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+        isK8sStatus((e as { statusObject: unknown }).statusObject)
+      ) {
+        return [];
+      }


You don't need the linter overrides here either, as long as you have enough guards to narrow the type down tightly enough you won't need the as keyword, which we generally want to avoid like the plague as that really has the effect of saying to the type system: "I've got this, kick back and chill for a bit", so we're losing compile time type safety.

If you actually narrow the type through conditional statements rather than using as, you're proving to the compiler the code is type safe, so any unexpected runtime values will be handled.

Try something like this, it's not pretty, but it's type safe and avoids the linter overrides:

Suggested change

if (

// eslint-disable-next-line @typescript-eslint/consistent-type-assertions

'statusObject' in (e as object) &&

// eslint-disable-next-line @typescript-eslint/consistent-type-assertions

isK8sStatus((e as { statusObject: unknown }).statusObject)

) {

return [];

}

// Check if error is a 404

if (

typeof e === 'object' &&

e != null &&

'statusObject' in e &&

isK8sStatus(e.statusObject)

) {

return [];

}

openshift-ci · 2025-02-20T17:44:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alexcreasy. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

frontend/OWNERS

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

mturley

@YuliaKrimerman this is great work - just a few things we need to tweak. You've also got some merge conflicts here.

mturley · 2025-02-20T23:56:40Z

frontend/src/concepts/modelCatalog/useModelCatalogSources.ts

+        typeof e === 'object' &&
+        e != null &&
+        'statusObject' in e &&
+        isK8sStatus(e.statusObject)


It's not enough to just check that we have a k8s Status object - we also need to check that e.statusObject.code === 404 here. Non-404 errors at the k8s API level will also have a statusObject with a different code (e.g. 403 if our rolebinding is messed up and the user doesn't have access to the configmap).

@YuliaKrimerman can you also update your non-404 error test case so it has a Status like the 404 case, but the status code is 500 or 401 or something else?

mturley · 2025-02-20T23:57:42Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalog.cy.ts

+        ns: 'opendatahub',
+        name: 'model-catalog-source-redhat',
+      },
+      { statusCode: 500 },


This is the line I'm referring to - this error response should have a Status object too.

mturley · 2025-02-21T00:09:51Z

frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelDetailsPage.cy.ts

+  it('should show error state when configmap fetch fails (non-404)', () => {
+    cy.interceptK8s(
+      {
+        model: ConfigMapModel,
+        ns: 'opendatahub',
+        name: 'model-catalog-source-redhat',
+      },
+      {
+        statusCode: 500,
+        body: {
+          kind: 'Status',
+          apiVersion: 'v1',
+          status: 'Failure',
+          message: 'Internal server error',
+          reason: 'InternalError',
+          code: 500,
+        },
+      },
+    );
+
+    modelDetailsPage.visit();
+    cy.contains('Unable to load model details').should('exist');
+  });


EDIT --- we may not need any changes here. I think this test is passing because of the incorrect contents being rendered in emptyStatePage. See comments below before you try to change anything here.

Hang on... I'm confused about why this test is passing. You have the non-404 Status object like we need to add to the other non-404 test here, but (see my earlier comment) your current code is eating errors and returning [] if they have a statusObject even if it's not 404. This test is written correctly but it should be failing (and adding the && e.statusObject.code === 404 in useModelCatalogSources like I mentioned should fix it).

I wonder if the problem is that you have two cy.interceptK8s calls for this configmap (one in initIntercepts and one in the test) and Cypress is using the first intercept for some reason? I wonder, does this test fail if you comment out the configmap intercept in initIntercepts so this is the only intercept?

Maybe we should add a param to initIntercepts like includeConfigMap = true, and then in these describe blocks around the tests that have their own configmap intercepts we should change the beforeEach so it passes true there and there is only ever one configmap intercept per test. We probably want to do that in the test files for both of these pages.

If that's not the issue, I'm not sure why this test would be passing...

mturley · 2025-02-21T00:24:32Z

frontend/src/concepts/modelCatalog/useModelCatalogSources.ts

+        if (isModelCatalogSource(parsed)) {
+          return [parsed];
+        }


I think we either need to make isModelCatalogSource actually inspect the properties inside the object to make sure they match the type, or we need to remove it and not bother checking. I'd lean towards the latter.

A lot of the dashboard code makes the assumption that the API will return data that matches our TypeScript types - actually checking all the types at run time isn't practical. We are curating the contents of the configmap and not supporting manual edits to it, so it's essentially our API contract - it's editable by a cluster admin in case they really need to squeeze another model into the Summit demo, but normal users won't be editing it. I think it's safe to assume it's correctly formatted as long as it's valid JSON (which it is if JSON.parse didn't throw an exception).

I think it would be fine to get rid of isModelCatalogSource and just do this:

return parsed ? [parsed as ModelCatalogSource] : [];

I'm curious if @alexcreasy disagrees.

Alternatively, we could have isModelCatalogSource also check that obj.models.length > 0?

mturley · 2025-02-21T00:30:33Z

frontend/src/pages/modelCatalog/screens/ModelCatalog.tsx

@@ -15,16 +15,35 @@ const ModelCatalog: React.FC = conditionalArea(
 )(() => {
  const { modelCatalogSources } = React.useContext(ModelCatalogContext);
  const renderStateProps = {
-    empty: modelCatalogSources.data.length === 0,
+    empty: modelCatalogSources.data.length === 0 || !modelCatalogSources.data[0]?.models?.length,


This is kinda funky - ideally we shouldn't write anything outside of useModelCatalogSources that assumes we have only one source (since we'll be adding a second source soon for Neural Magic), so we don't want to do anything based on only looking at data[0]. I think we can assume that if we have sources they have models, so we can probably remove this line.

(Note: this is why I added the end of my comment above about isModelCatalogSource - if you do want to check that a source has models before trying to use it, we should do that check there instead).

mturley · 2025-02-21T00:32:38Z

frontend/src/pages/modelCatalog/screens/ModelCatalog.tsx

+        title={
+          modelCatalogSources.error
+            ? 'Unable to load model catalog'
+            : 'Request access to model catalog'
+        }
+        description={
+          modelCatalogSources.error
+            ? 'Refresh the page or try again later'
+            : 'To request access to model catalog, contact your administrator.'
+        }


We don't need to check anything related to modelCatalogSources.error in the emptyStatePage -- if there's an error, the loadError prop below will be true and we won't be rendering this empty state at all. Instead, ApplicationsPage will use its internal error rendering which will use your errorMessage prop below.

We can put the whole EmptyModelCatalogState back the way it was here - the loadError and errorMessage props of ApplicationsPage are sufficient for rendering errors on these pages.

mturley · 2025-02-21T00:34:28Z

frontend/src/pages/modelCatalog/screens/ModelCatalog.tsx

@@ -33,8 +52,6 @@ const ModelCatalog: React.FC = conditionalArea(
        <TitleWithIcon title="Model Catalog" objectType={ProjectObjectType.registeredModels} />
      }
      {...renderStateProps}
-      loaded={modelCatalogSources.loaded}
-      provideChildrenPadding


Why remove the provideChildrenPadding prop?

mturley · 2025-02-21T00:38:13Z

frontend/src/pages/modelCatalog/screens/ModelCatalog.tsx

@@ -15,16 +15,35 @@ const ModelCatalog: React.FC = conditionalArea(
 )(() => {
  const { modelCatalogSources } = React.useContext(ModelCatalogContext);
  const renderStateProps = {


I think we can take this whole renderStateProps object and move its stuff as directly passing props to ApplicationsPage below, like we have for ModelDetailsPage. It was lifted out in other pages because it was conditional / it would vary, but there's no reason for that here.

mturley · 2025-02-21T00:40:16Z

frontend/src/pages/modelCatalog/screens/ModelDetailsPage.tsx

@@ -42,18 +42,20 @@ const ModelDetailsPage: React.FC = conditionalArea(
          objectType={ProjectObjectType.registeredModels}
        />
      }
-      empty={model === null}
+      empty={Boolean(modelCatalogSources.error) || model === null}


Similar to the other page - ApplicationsPage renders errors before it renders empty. We're passing loadError={modelCatalogSources.error}, so if modelCatalogSources.error is defined, emptyStatePage is not going to be rendered regardless of the value of empty. So we can change this back to model === null.

mturley · 2025-02-21T00:45:10Z

frontend/src/pages/modelCatalog/screens/ModelDetailsPage.tsx

      emptyStatePage={
        <EmptyModelCatalogState
          testid="empty-model-catalog-state"
-          title="Details not found"
-          description="To request access to model catalog, contact your administrator."
+          title={modelCatalogSources.error ? 'Details not found' : 'Unable to load model details'}
+          description={modelCatalogSources.error?.message || 'Refresh the page or try again later'}
          headerIcon={() => (
-            <img src={typedEmptyImage(ProjectObjectType.registeredModels)} alt="" />
+            <img src={typedEmptyImage(ProjectObjectType.registeredModels, 'Error')} alt="" />
          )}
        />


Similarly here, we don't need to do any of this error rendering in emptyStatePage. You can put this all back as it was - it is only used if there are no model catalog sources.

Aha -- I know why your mysterious non-404 cypress test was passing when it shouldn't. You're rendering 'Unable to load model details' here in the empty state even though there is no error. If you revert this stuff, that test will fail as it should. I'll edit the earlier comment.

saving

2e6f403

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Feb 18, 2025

openshift-ci bot requested review from christianvogt and ppadti February 18, 2025 15:39

YuliaKrimerman added 2 commits February 18, 2025 13:33

saving

d974251

final

fc59440

YuliaKrimerman changed the title ~~WIP Render loading/error states on catalog and details pages~~ Render loading/error states on catalog and details pages Feb 19, 2025

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Feb 19, 2025

alexcreasy requested changes Feb 20, 2025

View reviewed changes

openshift-ci bot assigned alexcreasy Feb 20, 2025

removed lint ignores

f015fec

mturley requested changes Feb 21, 2025

View reviewed changes

openshift-ci bot assigned mturley Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render loading/error states on catalog and details pages #3770

Render loading/error states on catalog and details pages #3770

YuliaKrimerman commented Feb 18, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

alexcreasy left a comment

alexcreasy Feb 19, 2025

YuliaKrimerman Feb 20, 2025

mturley Feb 20, 2025

alexcreasy Feb 20, 2025 •

edited

Loading

alexcreasy Feb 20, 2025

alexcreasy Feb 20, 2025

openshift-ci bot commented Feb 20, 2025

mturley left a comment •

edited

Loading

mturley Feb 20, 2025

mturley Feb 20, 2025

mturley Feb 21, 2025 •

edited

Loading

mturley Feb 21, 2025

mturley Feb 21, 2025

mturley Feb 21, 2025

mturley Feb 21, 2025

mturley Feb 21, 2025

mturley Feb 21, 2025

mturley Feb 21, 2025

		// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
		const source = JSON.parse(configMap.data.modelCatalogSource) as ModelCatalogSource;

	// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
	const source = JSON.parse(configMap.data.modelCatalogSource) as ModelCatalogSource;
	const source: ModelCatalogSource = JSON.parse(configMap.data.modelCatalogSource);

Render loading/error states on catalog and details pages #3770

Are you sure you want to change the base?

Render loading/error states on catalog and details pages #3770

Conversation

YuliaKrimerman commented Feb 18, 2025 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

codecov bot commented Feb 19, 2025 • edited Loading

Codecov Report

alexcreasy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcreasy Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 20, 2025

mturley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliaKrimerman commented Feb 18, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

alexcreasy Feb 20, 2025 •

edited

Loading

mturley left a comment •

edited

Loading

mturley Feb 21, 2025 •

edited

Loading