Serving runtime list fixes #4644
Conversation
|
Skipping CI for Draft Pull Request. |
WalkthroughUpdates refine loading and state handling across model-serving UI: introduce a unified loadingState in platform selection/reset flows, adjust template-loading logic in DeployButton, add EmptyProjectWatcher to mark projects without watchers as loaded, tweak deployments table/row presentation, gate scoped labels by area availability, harden service error handling, and revise Cypress tests to Model Mesh-first. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Hook as useProjectServingPlatform
participant API as Backend
UI->>Hook: selectPlatform(platform)
Hook->>Hook: loadingState = { type: 'platform', platform }
Hook->>API: apply platform
API-->>Hook: success or error
alt success
Hook->>Hook: loadingState = { type: null }
Hook-->>UI: updated projectPlatform
else error
Hook->>Hook: loadingState = { type: null }
Hook-->>UI: error
end
UI->>Hook: resetPlatform()
Hook->>Hook: loadingState = { type: 'reset' }
Hook->>API: reset platform
API-->>Hook: success or error
Hook->>Hook: loadingState = { type: null }
Hook-->>UI: updated state/error
sequenceDiagram
participant Ctx as ModelDeploymentsContext
participant Proj as Project
participant Watch as ProjectDeploymentWatcher
Ctx->>Proj: check platform/watcher
alt watcher exists
Ctx->>Watch: mount watcher(project)
Watch-->>Ctx: onStateChange({ deployments, loaded:true, error? })
Ctx->>Ctx: store per-project state
Watch-->>Ctx: cleanup -> unload project
else no watcher/platform
Ctx->>Ctx: mount EmptyProjectWatcher
Ctx->>Ctx: onStateChange({ deployments:[], loaded:true })
Ctx-->>Ctx: on unmount -> unload project
end
sequenceDiagram
participant UI as DeployButton
participant Hook as useServingRuntimeTemplates
UI->>Hook: [templates, loaded] = useServingRuntimeTemplates()
UI->>UI: isMissingTemplates = templates.length===0 && loaded
UI-->>UI: Disable only when isMissingTemplates true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
emilys314
force-pushed
the
serving-runtime-list-fixes
branch
from
3644967 to
994a8e8
Compare
emilys314
force-pushed
the
serving-runtime-list-fixes
branch
from
994a8e8 to
7f59dda
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
frontend/packages/modelServing/src/concepts/ModelDeploymentsContext.tsx (1)
123-125: allLoaded can turn true prematurely when projects change; compute against current projectsallLoaded uses Object.values(projectDeployments).every(...). If a project is added to props but hasn’t reported state yet, it won’t be in projectDeployments and the check can incorrectly pass. Compute allLoaded against the projects prop to ensure correctness.
Apply this diff:
- const allLoaded = - deploymentWatchersLoaded && Object.values(projectDeployments).every((state) => state.loaded); + const allLoaded = + deploymentWatchersLoaded && + projects.every((p) => projectDeployments[p.metadata.name]?.loaded === true);Optionally, also reconcile state on project list changes to keep entries in sync:
// place below useState React.useEffect(() => { setProjectDeployments((prev) => { const next = { ...prev }; // ensure all current projects have an entry for (const p of projects) { const name = p.metadata.name; if (!(name in next)) next[name] = { loaded: false }; } // remove entries for projects no longer present for (const name of Object.keys(next)) { if (!projects.some((p) => p.metadata.name === name)) { delete next[name]; } } return next; }); }, [projects]);frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (1)
727-735: Typo in statusPredictor field: use restUrl (not restUtl)The mock uses restUtl; UI code expects restUrl. This will hide the REST endpoint in the popover and fail assertions.
Apply this diff:
- statusPredictor: { - grpcUrl: 'grpc://modelmesh-serving.app:8033', - restUtl: 'http:///modelmesh-serving.app:8000', - }, + statusPredictor: { + grpcUrl: 'grpc://modelmesh-serving.app:8033', + restUrl: 'http:///modelmesh-serving.app:8000', + },And similarly below:
- statusPredictor: { - grpcUrl: 'grpc://modelmesh-serving.app:8033', - restUtl: 'http:///modelmesh-serving.app:8000', - }, + statusPredictor: { + grpcUrl: 'grpc://modelmesh-serving.app:8033', + restUrl: 'http:///modelmesh-serving.app:8000', + },Also applies to: 807-815
🧹 Nitpick comments (7)
frontend/packages/modelServing/src/components/deployments/DeploymentsTableRow.tsx (1)
90-96: Make data-testid unique per row to avoid ambiguous selectorsUsing only the platformId will duplicate data-testids across rows (e.g., multiple rows under “kserve”). Consider including the model name to ensure uniqueness.
- <Td - data-testid={`${deployment.modelServingPlatformId}-model-row-item`} + <Td + data-testid={`${deployment.modelServingPlatformId}-${deployment.model.metadata.name}-model-row-item`} expand={{ rowIndex, expandId: `${deployment.modelServingPlatformId}-model-row-item`, isExpanded, onToggle: () => setExpanded(!isExpanded), }} />Note: expandId can remain as-is; rowIndex differentiates expansion wiring.
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts (1)
141-141: Use a more explicit alias name to reduce ambiguity“templates” is generic and may be reused elsewhere. Prefer a specific alias name for clarity and to prevent future collisions.
- ).as('templates'); + ).as('servingRuntimeTemplates');And update the wait accordingly:
- cy.wait('@templates'); + cy.wait('@servingRuntimeTemplates');Also applies to: 483-483
frontend/src/services/componentsServices.ts (1)
15-16: Safer error extraction is good; consider preserving the original error as cause and adding status contextOptional chaining with a fallback to e.message prevents undefined-access crashes. To aid debugging, preserve the original error as the cause and add HTTP status text when available.
- throw new Error(e.response?.data?.message || e.message); + const status = (e as any)?.response?.status; + const statusText = (e as any)?.response?.statusText; + const serverMsg = (e as any)?.response?.data?.message; + const fallback = (e as any)?.message ?? 'Request failed'; + const msg = serverMsg ?? (status ? `${status} ${statusText || ''}`.trim() : fallback); + // Preserve original error for debugging + throw new Error(msg, { cause: e as Error });- throw new Error(e.response?.data?.error || e.message); + const status = (e as any)?.response?.status; + const statusText = (e as any)?.response?.statusText; + const serverErr = (e as any)?.response?.data?.error; + const fallback = (e as any)?.message ?? 'Request failed'; + const msg = serverErr ?? (status ? `${status} ${statusText || ''}`.trim() : fallback); + throw new Error(msg, { cause: e as Error });If the project targets environments without Error.cause support, we can gate that with a small helper; happy to provide one.
Also applies to: 30-31
frontend/packages/modelServing/src/components/deploy/DeployButton.tsx (1)
15-21: Avoid duplicate watches of serving runtime templates by lifting state and passing into the modal.Both DeployButton and DeployButtonModal call
useServingRuntimeTemplates()which results in two watchers and redundant work. Lift the templates to the parent and pass them as a prop to the modal to reduce API load and re-renders.Apply this diff within this file:
@@ -const DeployButtonModal: React.FC<{ - platform?: ServingRuntimePlatform; - currentProject: ProjectKind; - onClose: (submit: boolean) => void; -}> = ({ platform, currentProject, onClose }) => { - const [templates] = useServingRuntimeTemplates(); - const connections = useConnections(currentProject.metadata.name); +const DeployButtonModal: React.FC<{ + platform?: ServingRuntimePlatform; + currentProject: ProjectKind; + onClose: (submit: boolean) => void; + templates: ReturnType<typeof useServingRuntimeTemplates>[0]; +}> = ({ platform, currentProject, onClose, templates }) => { + const connections = useConnections(currentProject.metadata.name); @@ - const [globalTemplates, globalTemplatesLoaded] = useServingRuntimeTemplates(); + const [globalTemplates, globalTemplatesLoaded] = useServingRuntimeTemplates(); const isMissingTemplates = globalTemplates.length === 0 && globalTemplatesLoaded; @@ <DeployButtonModal platform={legacyServingRuntimePlatform} currentProject={project} + templates={globalTemplates} onClose={onSubmit} />Note: Using
ReturnType<typeof useServingRuntimeTemplates>[0]avoids introducing a new import for the template type while keeping type safety.Also applies to: 76-79, 112-116
frontend/src/pages/modelServing/screens/global/InferenceServiceServingRuntime.tsx (1)
25-27: Good shift from prop-driven gating to area-availability gating.Using
useIsAreaAvailable(SupportedArea.DS_PROJECT_SCOPED).statusremoves the need for theisProjectScopedprop and centralizes feature gating logic.Optional micro-cleanup for readability/perf (avoid repeating
getServingRuntimeVersion):// near the top of the component body const srvVersion = servingRuntime ? getServingRuntimeVersion(servingRuntime) : undefined; // later in JSX {srvVersion && <ServingRuntimeVersionLabel version={srvVersion} isCompact />} {versionStatus && ( <ServingRuntimeVersionStatus isOutdated={versionStatus === ServingRuntimeVersionStatusLabel.OUTDATED} version={srvVersion || ''} templateVersion={getServingRuntimeVersion(template) || ''} /> )}Also applies to: 63-69
frontend/packages/modelServing/src/concepts/ModelDeploymentsContext.tsx (1)
90-90: Avoid rebuilding the initial state on every render; use a lazy initializerObject.fromEntries(projects.map(...)) is computed on every render but only used on the first. Switch to a lazy initializer to prevent unnecessary work.
Apply this diff:
- }>(Object.fromEntries(projects.map((p) => [p.metadata.name, { loaded: false }]))); + }>(() => + Object.fromEntries(projects.map((p) => [p.metadata.name, { loaded: false }])) + );frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (1)
175-186: ODH/NIM intercepts: drop the null parameter for clarityThe interceptOdh overload doesn’t require an explicit null options argument. Prefer the 2-arg form for readability and consistency with other calls in this file.
Apply this diff:
- cy.interceptOdh('GET /api/components', null, [mockOdhApplication({})]); + cy.interceptOdh('GET /api/components', [mockOdhApplication({})]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/packages/modelServing/extensions/odh.ts(3 hunks)frontend/packages/modelServing/src/components/deploy/DeployButton.tsx(1 hunks)frontend/packages/modelServing/src/components/deployments/DeploymentsTable.tsx(1 hunks)frontend/packages/modelServing/src/components/deployments/DeploymentsTableRow.tsx(3 hunks)frontend/packages/modelServing/src/components/deployments/DeploymentsTableRowExpandedSection.tsx(1 hunks)frontend/packages/modelServing/src/components/projectDetails/ProjectDeploymentsTable.tsx(1 hunks)frontend/packages/modelServing/src/concepts/ModelDeploymentsContext.tsx(3 hunks)frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts(20 hunks)frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts(2 hunks)frontend/src/pages/modelServing/screens/global/InferenceServiceServingRuntime.tsx(2 hunks)frontend/src/pages/modelServing/screens/global/InferenceServiceTableRow.tsx(1 hunks)frontend/src/plugins/extensions/navigation.ts(1 hunks)frontend/src/plugins/extensions/routes.ts(1 hunks)frontend/src/services/componentsServices.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/cypress-e2e.mdc)
frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx}: Don't create TypeScript interfaces for test data; use direct object access patterns.
All linting errors must be fixed: use object destructuring, proper formatting, no unused variables/imports, no any types unless necessary.
Never hardcode namespaces in tests or utilities; always derive namespaces from test variables or environment variables.
Files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.tsfrontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts
🧠 Learnings (7)
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx} : Don't create TypeScript interfaces for test data; use direct object access patterns.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx} : Never hardcode namespaces in tests or utilities; always derive namespaces from test variables or environment variables.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : Delete test projects, PVCs, and other resources after tests using after() hooks.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : Use .navigate() methods in page objects for navigation instead of cy.visit() in tests.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : Use cy.step('Description') instead of cy.log() for step documentation in tests.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts
📚 Learning: 2025-07-21T11:49:57.416Z
Learnt from: CR
PR: opendatahub-io/odh-dashboard#0
File: .cursor/rules/cypress-e2e.mdc:0-0
Timestamp: 2025-07-21T11:49:57.416Z
Learning: Applies to frontend/src/__tests__/cypress/cypress/tests/e2e/**/*.cy.ts : Use descriptive file names matching feature area, e.g., testFeatureName.cy.ts for main functionality.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts
📚 Learning: 2025-06-19T20:38:32.485Z
Learnt from: christianvogt
PR: opendatahub-io/odh-dashboard#4381
File: frontend/src/__tests__/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the `frontend/src/__tests__/cypress/tsconfig.json` file intentionally has an empty `files` array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
Applied to files:
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts
🧬 Code Graph Analysis (6)
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts (1)
frontend/src/__tests__/cypress/cypress/pages/projects.ts (1)
projectDetails(420-420)
frontend/src/pages/modelServing/screens/global/InferenceServiceServingRuntime.tsx (2)
frontend/src/k8sTypes.ts (1)
ServingRuntimeKind(464-485)frontend/src/concepts/areas/index.ts (1)
SupportedArea(11-11)
frontend/packages/modelServing/extensions/odh.ts (1)
frontend/src/concepts/areas/index.ts (1)
SupportedArea(11-11)
frontend/packages/modelServing/src/concepts/ModelDeploymentsContext.tsx (1)
frontend/packages/modelServing/extension-points/index.ts (1)
Deployment(52-62)
frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (5)
frontend/src/__mocks__/mockOdhApplication.ts (1)
mockOdhApplication(48-112)frontend/src/api/models/kserve.ts (2)
InferenceServiceModel(10-15)ServingRuntimeModel(3-8)frontend/src/__mocks__/mockK8sResourceList.ts (1)
mockK8sResourceList(3-17)frontend/src/__tests__/cypress/cypress/pages/servingRuntimes.ts (1)
servingRuntimes(155-155)frontend/src/__mocks__/mockInferenceServiceK8sResource.ts (1)
mockInferenceServiceK8sResource(85-238)
frontend/packages/modelServing/src/components/deploy/DeployButton.tsx (1)
frontend/packages/modelServing/src/concepts/servingRuntimeTemplates/useServingRuntimeTemplates.ts (1)
useServingRuntimeTemplates(46-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress-Setup
- GitHub Check: Lint
🔇 Additional comments (14)
frontend/packages/modelServing/src/components/deployments/DeploymentsTableRowExpandedSection.tsx (1)
84-86: Clearer label text — looks goodRenaming to “Model server replicas” improves clarity without affecting logic.
frontend/packages/modelServing/src/components/deployments/DeploymentsTableRow.tsx (2)
2-5: Switch to ResourceActionsColumn is appropriateReplacing PatternFly’s ActionsColumn with the internal ResourceActionsColumn (with resource={deployment.model}) aligns actions with RBAC/context and keeps behavior intact.
156-179: Actions column wiring looks correctProviding resource={deployment.model} plus action items mirrors the previous UX and correctly disables actions while starting/stopping.
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectDetails.cy.ts (1)
480-481: Good synchronization additionInitializing empty model-serving intercepts before navigation reduces flakiness where the UI requests runtime lists early.
frontend/packages/modelServing/src/components/deploy/DeployButton.tsx (1)
76-79: Correctly gates “missing templates” on the loaded flag — avoids false-disable while loading.The new check
globalTemplates.length === 0 && globalTemplatesLoadedprevents disabling the Deploy button during template load. This fixes the premature disablement UX.frontend/packages/modelServing/extensions/odh.ts (1)
37-38: Migration to enum-based gating (SupportedArea.MODEL_SERVING) looks correct and consistent.Switching to
SupportedArea.MODEL_SERVINGin flags aligns with the newer area-availability approach and keeps the plugin’s extensions coherently gated.Also applies to: 48-49, 54-55, 71-72
frontend/src/plugins/extensions/routes.ts (1)
81-90: /modelServing routes remain registered by the ModelServing extension
- Verified that
frontend/packages/modelServing/extensions/odh.tsdefines a catch-all route at
path: '/modelServing/:namespace?/*'(lines 61–69), ensuring deep links (e.g./modelServing/<ns>/metrics/<name>) continue to resolve through the plugin.- The commented-out host routes in
frontend/src/plugins/extensions/navigation.tsandroutes.tscan safely be removed without breaking/modelServing/*.frontend/src/pages/modelServing/screens/global/InferenceServiceTableRow.tsx (1)
110-114: LGTM: Prop cleanup aligns with the updated InferenceServiceServingRuntime API.Dropping
isProjectScopedhere matches the component’s new signature and centralizes gating logic.frontend/packages/modelServing/src/concepts/ModelDeploymentsContext.tsx (2)
55-71: EmptyProjectWatcher: good approach to mark non-platform projects as loadedIntroducing EmptyProjectWatcher avoids “dangling” loaded states and fits the existing onStateChange/unload pattern.
145-153: Rendering EmptyProjectWatcher for non-platform projects is correct and consistentThis ensures per-project loaded state is set deterministically even when no watcher exists.
frontend/packages/modelServing/src/components/deployments/DeploymentsTable.tsx (1)
65-71: defaultSortColumn prop confirmed
TheTablecomponent in frontend/src/components/table/Table.tsx declares and destructures adefaultSortColumnprop with a default value of 0—confirming it’s supported and zero-based.No changes required.
frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (3)
102-102: NIM mocks import is appropriateImporting mockOdhApplication to support expanded error detection flows in plugin code is appropriate.
509-509: Test toggles for platform flags look consistentSwitching to explicitly enable Model Mesh/KServe per scenario aligns with the PR goal and the updated flows.
Also applies to: 609-609, 641-641, 727-727, 803-803, 1280-1280, 1396-1396, 1491-1491, 1689-1689, 1719-1726
200-203: Avoid hardcoding namespaces in tests; derive from env/test varsPer repository test guidelines, don’t hardcode namespaces. These new namespaced list intercepts embed 'test-project'. Use a derived namespace variable (e.g., Cypress.env) and apply it across intercepts.
Apply this diff:
- cy.interceptK8sList( - { model: InferenceServiceModel, ns: 'test-project' }, - mockK8sResourceList(inferenceServices), - ); + const ns = Cypress.env('TEST_NAMESPACE') ?? 'test-project'; + cy.interceptK8sList({ model: InferenceServiceModel, ns }, mockK8sResourceList(inferenceServices));- cy.interceptK8sList( - { model: ServingRuntimeModel, ns: 'test-project' }, - mockK8sResourceList(servingRuntimes), - ); + cy.interceptK8sList({ model: ServingRuntimeModel, ns }, mockK8sResourceList(servingRuntimes));If you want, I can generate a follow-up patch to replace remaining hardcoded 'test-project' usages throughout this file to comply fully with the guideline.
Also applies to: 326-329
⛔ Skipped due to learnings
Learnt from: CR PR: opendatahub-io/odh-dashboard#0 File: .cursor/rules/cypress-e2e.mdc:0-0 Timestamp: 2025-07-21T11:49:57.416Z Learning: Applies to frontend/src/__tests__/cypress/cypress/**/*.{ts,tsx} : Never hardcode namespaces in tests or utilities; always derive namespaces from test variables or environment variables.Learnt from: dpanshug PR: opendatahub-io/odh-dashboard#4571 File: frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureViews.cy.ts:16-18 Timestamp: 2025-07-29T15:44:19.024Z Learning: In the ODH Dashboard project, the correct pattern for handling namespaces in Cypress tests is to define them as constants at the top of test files (e.g., `const k8sNamespace = 'default';`), not to use environment variables or other dynamic approaches.Learnt from: dpanshug PR: opendatahub-io/odh-dashboard#4571 File: frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureServices.cy.ts:16-18 Timestamp: 2025-07-29T15:43:30.907Z Learning: In the ODH Dashboard project, `Cypress.env()` is not the correct approach for configuring namespace values in Cypress tests.Learnt from: dpanshug PR: opendatahub-io/odh-dashboard#4571 File: frontend/src/__tests__/cypress/cypress/tests/mocked/featureStore/featureViews.cy.ts:16-18 Timestamp: 2025-07-29T15:44:19.024Z Learning: In the ODH Dashboard project, environment variables are not used in Cypress tests for namespace configuration, as clarified by dpanshug.
| @@ -29,6 +29,7 @@ export const ProjectDeploymentsTable: React.FC<{ | |||
| loaded={loaded && tableExtensionLoaded} | |||
| showExpandedInfo={!!expandedInfoExtension} | |||
| platformColumns={tableExtension?.properties.columns()} | |||
| defaultSortColumn={1} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default sort column should depend on expanded view
With showExpandedInfo=false, column 0 is “Model deployment name”. With showExpandedInfo=true, an expand column is prepended, and the “name” column shifts to index 1. Passing 1 unconditionally sorts by the wrong column (and may hit a non-sortable column when expanded view is off).
Apply this diff:
- defaultSortColumn={1}
+ defaultSortColumn={expandedInfoExtension ? 1 : 0}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaultSortColumn={1} | |
| defaultSortColumn={expandedInfoExtension ? 1 : 0} |
🤖 Prompt for AI Agents
frontend/packages/modelServing/src/components/projectDetails/ProjectDeploymentsTable.tsx
around line 32: the defaultSortColumn is hardcoded to 1 which is correct only
when showExpandedInfo is true (name column at index 1) but wrong when
showExpandedInfo is false (name at index 0); update the prop to compute the
index based on showExpandedInfo (use showExpandedInfo ? 1 : 0) so the table
sorts by the "Model deployment name" column in both modes.
| // { | ||
| // type: 'app.navigation/href', | ||
| // flags: { | ||
| // required: [SupportedArea.MODEL_SERVING], | ||
| // disallowed: [SupportedArea.PLUGIN_MODEL_SERVING], | ||
| // }, | ||
| // properties: { | ||
| // id: 'modelServing', | ||
| // title: 'Model deployments', | ||
| // href: '/modelServing', | ||
| // section: 'models', | ||
| // path: '/modelServing/*', | ||
| // }, | ||
| // }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commented-out navigation: add follow-up context and verify no lingering references
If this route is intentionally disabled, add a TODO with the tracking issue to avoid bitrot. Also verify no code/tests still reference “/modelServing”.
Note: If re-enabling later, replace disallowed: [SupportedArea.PLUGIN_MODEL_SERVING] with the proper enum-based gating consistent with the rest of the PR.
🏁 Script executed:
#!/bin/bash
# Check for lingering references to the removed /modelServing nav/route
set -euo pipefail
echo "Searching for '/modelServing' references..."
rg -n --no-heading "/modelServing(\\b|/)" -S || true
echo
echo "Searching for 'modelServing' nav id or route usage near extensions..."
rg -n --no-heading "id:\\s*'modelServing'|modelServing" -S -A3 -B3 frontend || true
echo
echo "Searching Cypress tests for navigation to /modelServing..."
rg -n --no-heading "visit\\([^)]*modelServing" frontend/src/__tests__/cypress -S || trueLength of output: 271869
Commented-out Model Serving nav: add TODO & clean up lingering “/modelServing” references
Even though you’ve disabled the in-app nav entry for Model Serving, there are hundreds of imports, links, tests and routes still pointing to /modelServing. To avoid breakage or bitrot:
- In
frontend/src/plugins/extensions/navigation.ts(lines 69–82), add a// TODO (track in ISSUE-NNN):…above the commented-out nav item with the issue URL so we don’t lose context. - Remove or update all surviving
/modelServingreferences, for example:- In
frontend/src/plugins/extensions/routes.ts(lines 84–85, 195). - In unit tests and Cypress specs (search under
frontend/src/__tests__/…). - In page components and hooks that call
navigate('/modelServing…')or render<Link to="/modelServing">.
- In
- If you intend to re-enable this later, replace the old
disallowed: [SupportedArea.PLUGIN_MODEL_SERVING]flag with the proper enum-based gating (as in this PR) at both the nav and routes extensions.
Please clean up or guard these references so the codebase continues to build and test green.
🤖 Prompt for AI Agents
In frontend/src/plugins/extensions/navigation.ts around lines 69–82, add a
single-line TODO comment above the commented-out Model Serving nav item that
references the tracking issue (e.g. // TODO (track in ISSUE-NNN): link-to-issue)
so the rationale is preserved; then find and either remove or guard all
remaining "/modelServing" references (notably
frontend/src/plugins/extensions/routes.ts lines ~84–85 and ~195, unit tests
under frontend/src/__tests__, Cypress specs, page components, and hooks that
call navigate('/modelServing...') or <Link to="/modelServing">) — update those
to the new guarded enum-based gating or delete/redirect them as appropriate; if
you plan to re-enable the nav later, replace the old disallowed:
[SupportedArea.PLUGIN_MODEL_SERVING] flag with the proper enum-based gating used
elsewhere before re-adding the nav entry.
There was a problem hiding this comment.
Ignore, I will remove the toggle after review. I just figure this is easier to review with it on
| const [globalTemplates, globalTemplatesLoaded] = useServingRuntimeTemplates(); | ||
| const isMissingTemplates = globalTemplates.length === 0 && globalTemplatesLoaded; |
There was a problem hiding this comment.
99% of the time a user will have runtime templates installed, so this reducing a loading flicker state by only disabling once the check is completed
| @@ -69,7 +87,7 @@ export const ModelDeploymentsProvider: React.FC<ModelDeploymentsProviderProps> = | |||
|
|
|||
| const [projectDeployments, setProjectDeployments] = React.useState<{ | |||
| [key: string]: { deployments?: Deployment[]; loaded: boolean; error?: Error }; | |||
| }>({}); | |||
| }>(Object.fromEntries(projects.map((p) => [p.metadata.name, { loaded: false }]))); | |||
There was a problem hiding this comment.
Initializing the projects as loaded: false, it will prevent loading flicker on first render
| @@ -184,6 +197,10 @@ const initIntercepts = ({ | |||
| mockProjectK8sResource({ enableModelMesh: projectEnableModelMesh }), | |||
| ); | |||
| cy.interceptK8sList(InferenceServiceModel, mockK8sResourceList(inferenceServices)); | |||
| cy.interceptK8sList( | |||
| { model: InferenceServiceModel, ns: 'test-project' }, | |||
There was a problem hiding this comment.
The refactor has namespaced api calls, the old code did a cluster wide one
| components: undefined, | ||
| installedComponents: { kserve: false, 'model-mesh': false }, | ||
| }), | ||
| ); | ||
|
|
||
| cy.reload(); | ||
| projectDetails.findResetPlatformButton().should('exist'); |
There was a problem hiding this comment.
This didn't really make sense, if there are no model platforms, it should be showing the "No serving platforms talk to an admin" screen
There was a problem hiding this comment.
oh yeah that was from this PR
the "No serving platforms talk to an admin" does show up, but lets say your project is kserve and then you go disable kserve, the button to change platforms should exist but you'll be on a page saying "kserve is no longer enabled" then when you hit the change button it lets you know there aren't any platforms enabled
There was a problem hiding this comment.
Gotcha, I guess this isn't needed for the refactor? Because it will go straight to the no platforms page already
Because with the refactor, if a platform isn't installed, there is no way to know that your project no longer has a platform that is removed. Because the extensions are disabled
| @@ -1646,6 +1643,7 @@ describe('Serving Runtime List', () => { | |||
| mockK8sResourceList([]), | |||
| ).as('getPods'); | |||
|
|
|||
| cy.reload(); | |||
There was a problem hiding this comment.
I can't find any other way to make the start / stop button update. Clicking the button doesn't call for a refresh anymore and the webhook will just get the new data. but cypress isn't able to force a webhook update with the cy.wsk8s function
There was a problem hiding this comment.
yeah it's a pain to try and mock this. Haven't looked into it with webhooks implemented now but before there was no way to mock it because the app was only calling for pods one time. I think it's okay for us to rely on e2e testing here more (which we have already)
There was a problem hiding this comment.
Everything LGTM. There's only one more failure coming from modelServingGlobal. Should we merge this and have @katieperry4 rebase and fix it in #4686?
| disableModelMeshConfig: false, | ||
| disableProjectScoped: false, | ||
| inferenceServices: [ | ||
| mockInferenceServiceK8sResource({ name: 'test-inference', isModelMesh: true }), |
There was a problem hiding this comment.
why were so many of these wrong?
|
@Griffin-Sullivan |
emilys314
force-pushed
the
serving-runtime-list-fixes
branch
from
31dff4d to
c7de79b
Compare
|
/unhold @Griffin-Sullivan @katieperry4 i re-disabled the refactor plugin so it's ready for approving |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Griffin-Sullivan 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4644 +/- ##
==========================================
+ Coverage 29.34% 29.56% +0.22%
==========================================
Files 1846 1921 +75
Lines 38288 39386 +1098
Branches 11766 12130 +364
==========================================
+ Hits 11234 11643 +409
- Misses 27054 27743 +689
... and 80 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
openshift-merge-bot
bot
merged commit be7854e
into
opendatahub-io:main
* Fixes * Most things fixed * Better initial loading state * move defaultSortColumn * Fix more feature flag toggles for test inits * Force update start stop view * Add loading state to reset platform button plus consistency
Towards https://issues.redhat.com/browse/RHOAIENG-31943
Description
This should fix all tests except the start stop
How Has This Been Tested?
Enable the flag and run the servingRuntimeList.cy.ts cypress mock test
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
New Features
Bug Fixes
Style
Refactor
Tests