Update status labels#6746
Merged
openshift-merge-bot[bot] merged 10 commits intoopendatahub-io:mainfrom Apr 2, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames and remaps status semantics and UI props across the frontend: Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
rsun19
force-pushed
the
update-status-labels
branch
from
f9a264b to
2a99a22
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6746 +/- ##
==========================================
+ Coverage 64.76% 64.88% +0.11%
==========================================
Files 2519 2513 -6
Lines 78004 77957 -47
Branches 19381 19375 -6
==========================================
+ Hits 50522 50580 +58
+ Misses 27482 27377 -105
... and 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
needs-rebase
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
rsun19
force-pushed
the
update-status-labels
branch
from
08a13f7 to
269eb25
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx (1)
43-47: 🛠️ Refactor suggestion | 🟠 MajorInconsistent status prop usage.
Lines 44 and 55 still use the
colorprop ("grey","teal") while the refactored cases use thestatusprop. This creates inconsistency within the component. Either complete the migration tostatusfor all states or document why these remain exceptions.Also applies to: 54-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx` around lines 43 - 47, MCPServerStatus is inconsistent: some Label usages still pass color ("grey", "teal") while others use the refactored status prop. Update the two Label instances that use color (the one rendering Unknown with QuestionCircleIcon and the one rendering Healthy with CheckCircleIcon) to use the same status prop name and values as the other cases (replace color="grey"/"teal" with the equivalent status="…" values used elsewhere) so all branches consistently use status; ensure the icon and label text remain unchanged.packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchStatus.cy.ts (1)
89-103:⚠️ Potential issue | 🟡 MinorStale comments and step descriptions reference old status.
Line 89
cy.stepand lines 98-101 comments still reference "Running" but assertions now expectReady.Proposed fix
// Wait for workbench to run - cy.step(`Wait for workbench ${workbenchName} to display a "Running" status`); + cy.step(`Wait for workbench ${workbenchName} to display a "Ready" status`); const notebookRow = workbenchPage.getNotebookRow(workbenchName); notebookRow.findNotebookDescription(projectDescription); notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 120000); // Use dynamic image name verification based on what was actually selected getImageStreamDisplayName(selectedImageStream).then((displayName) => { notebookRow.shouldHaveNotebookImageName(displayName); - // Click on 'Running' status and validate the Progress steps + // Click on 'Ready' status and validate the Progress steps cy.step( - 'Click on Running status, validate the Running status and navigate to the Progress tab', + 'Click on Ready status, validate the Ready status and navigate to the Progress tab', );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchStatus.cy.ts` around lines 89 - 103, The test contains stale step text/comments that say "Running" while the actual assertions use NotebookStatusLabel.Ready; update the descriptive strings and inline comments to reflect "Ready" (or the correct intended status) so they match the assertions: change the cy.step call that currently says `Wait for workbench ${workbenchName} to display a "Running" status` and the later cy.step/comment that says 'Click on Running status...' to reference "Ready" (and any other nearby comments or step descriptions), keeping the assertions using notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready) and workbenchStatusModal.getNotebookStatus(NotebookStatusLabel.Ready) unchanged.packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.ts (1)
138-149:⚠️ Potential issue | 🟡 MinorStale comments reference old status.
Lines 138-139 and 145-146 comments still say "running" but the assertion expects
Ready.Proposed fix
- // Navigate back to the project and verify workbench is still running - cy.step('Navigate to workbenches tab and verify workbench is still running'); + // Navigate back to the project and verify workbench is still ready + cy.step('Navigate to workbenches tab and verify workbench is still ready'); projectListPage.navigate(); projectListPage.filterProjectByName(projectName); projectListPage.findProjectLink(projectName).click(); projectDetails.findSectionTab('workbenches').click(); - // Verify workbench is still running - cy.step('Verify workbench is still running after hardware profile deletion'); + // Verify workbench is still ready + cy.step('Verify workbench is still ready after hardware profile deletion');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.ts` around lines 138 - 149, Update the stale test comments that refer to "running" to accurately reflect the expected Ready state: change the cy.step description texts around the navigation and verification steps in the testDeleteAppliedHardwareProfile.cy.ts (the two cy.step calls before project navigation and before status assertion) so they mention "ready" or "Ready" instead of "running"; this keeps the comments consistent with workbenchPage.getNotebookRow(...), workbenchRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 30000), and the assertion logic.frontend/src/concepts/distributedWorkloads/utils.tsx (1)
123-128:⚠️ Potential issue | 🟡 Minor
Completestill misses the structuredJobFinishedreason.The matcher only looks for
/success|succeeded/, but the completed fixture infrontend/src/__mocks__/mockWorkloadK8sResource.tsusesreason: 'JobFinished'. That means completion currently depends on the free-form message containing"successfully"; an empty or reworded message will misclassify finished workloads. Add a regression test forFinished=Truewithreason: 'JobFinished'and an empty message.Suggested fix
Complete: conditions?.find( ({ type, status, message, reason }) => status === 'True' && type === 'Finished' && - /success|succeeded/.test(`${message} ${reason}`.toLowerCase()), + (reason === 'JobFinished' || + /success|succeeded/.test(`${message} ${reason}`.toLowerCase())), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/concepts/distributedWorkloads/utils.tsx` around lines 123 - 128, The "Complete" condition matcher currently only checks for /success|succeeded/ in message/reason and misses structured reasons like "JobFinished"; update the Complete: conditions?.find(...) predicate to treat status==='True' && type==='Finished' as complete if reason === 'JobFinished' OR the existing regex matches (i.e., add an explicit reason === 'JobFinished' branch alongside the regex check), and add a regression test that creates a Finished=True condition with reason: 'JobFinished' and an empty message (using the existing mockWorkloadK8sResource fixture) to ensure the new logic correctly classifies the workload as complete.frontend/src/concepts/topology/NodeStatusIcon.tsx (1)
15-17:⚠️ Potential issue | 🔴 CriticalConvert incoming
RunStatusenum values to match the switch statement's expected display label strings.The component receives
RunStatusenum values (e.g.,Succeeded,InProgress,Skipped) fromPipelineDefaultTaskGroup.tsxline 49, but the switch statement matchesruntimeStateLabels[...]strings (e.g.,'Complete','Running','Skipped'). These domains do not align—every incoming value falls through to the default case, rendering no icon. Either mapRunStatus → RuntimeStateKFbefore the switch, derive the display label from the enum value explicitly, or accept and directly normalize the incoming type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/concepts/topology/NodeStatusIcon.tsx` around lines 15 - 17, NodeStatusIcon is receiving RunStatus enum values via the runStatus prop (originating from PipelineDefaultTaskGroup) but the switch uses runtimeStateLabels display strings (e.g., 'Complete', 'Running'), so all cases fall through; fix by normalizing the incoming value before the switch: convert runStatus (RunStatus enum string) into the matching RuntimeStateKF/display label (using a small mapping or a helper like mapRunStatusToRuntimeState) or accept enum values directly in the switch by replacing runtimeStateLabels[...] checks with the corresponding RunStatus enum names; update NodeStatusIcon (and any helper you add) to perform this mapping and then use the normalized label variable in the existing switch to select icon/status.
🧹 Nitpick comments (5)
packages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsx (1)
32-32: Switch to semanticstatusprop on theLabelcomponent.Line 32 uses
colorto encode semantic state, but PatternFly v6Labelexpects thestatusprop for success/danger states. This ensures proper icons, color, and accessibility—align with PF6 patterns.Suggested change
- <Label variant="outline" color={apiKey.status === 'active' ? 'green' : 'red'}> + <Label variant="outline" status={apiKey.status === 'active' ? 'success' : 'danger'}> {capitalize(apiKey.status)} </Label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsx` at line 32, The Label in ApiKeysTableRow is using the old color prop; change it to the semantic status prop on the Label component (e.g. replace color={apiKey.status === 'active' ? 'green' : 'red'} with status={apiKey.status === 'active' ? 'success' : 'danger'}) so PatternFly v6 renders correct icons/aria and colors; locate the Label usage in ApiKeysTableRow.tsx and update the prop value to the appropriate semantic status strings while keeping any existing variant prop intact.packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx (1)
89-95: Use semanticstatusprop instead ofcolorfor consistency.This component uses
color="green"andcolor="red", but other status indicators in the same package (MCPServerStatus.tsx) already use the semanticstatusprop with explicit icons. Align these labels with the existing pattern by usingstatus="success"andstatus="danger".♻️ Proposed fix
- <Label variant="outline" color="green" icon={<CheckCircleIcon />}> + <Label variant="outline" status="success" icon={<CheckCircleIcon />}> Active </Label> ) : ( - <Label variant="outline" color="red" icon={<ExclamationCircleIcon />}> + <Label variant="outline" status="danger" icon={<ExclamationCircleIcon />}> Inactive </Label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx` around lines 89 - 95, The Label usage in AIModelTableRow uses color="green"/"red" instead of the package's semantic status prop; update both Label instances in AIModelTableRow to use status="success" for the active case and status="danger" for the inactive case and keep the same icon props (CheckCircleIcon / ExclamationCircleIcon) and variant to match MCPServerStatus.tsx's pattern so styling is consistent across components.frontend/src/concepts/notebooks/NotebookStatusLabel.tsx (1)
44-88: Consider removinguseMemoper project guidelines.
labelSettingsis not passed as a prop, not used as auseEffectdependency, and not returned from a custom hook. React re-renders are cheap; the conditional object creation here doesn't warrant memoization.As per coding guidelines: "Performance: avoid unnecessary useCallback/useMemo/useRef — React is performant by default. Only use useCallback when the function is passed as a prop, used as a useEffect dependency, or returned from a custom hook."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/concepts/notebooks/NotebookStatusLabel.tsx` around lines 44 - 88, Remove the unnecessary React.useMemo around labelSettings in NotebookStatusLabel.tsx: replace the useMemo block that computes labelSettings (typed as StatusLabelSettings) with a plain synchronous computation (keep the same conditional logic using isError, isStopping, kueueStatus with KUEUE_STATUSES_OVERRIDE_WORKBENCH and getKueueStatusInfo, isStarting, isRunning) so labelSettings is a normal const; also update the dependency array removal and ensure any references to labelSettings remain unchanged.packages/automl/frontend/src/app/components/AutomlRunsTable/__tests__/AutomlRunsTableRow.spec.tsx (1)
38-68: Exercise the rendered label once at row level.These cases only pin
getStatusLabelProps(). They will still pass ifAutomlRunsTableRowstops spreading those props onto<Label>or dropsvariant="outline", which is the user-visible contract changed in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/frontend/src/app/components/AutomlRunsTable/__tests__/AutomlRunsTableRow.spec.tsx` around lines 38 - 68, Tests only call getStatusLabelProps directly; update them to render the consumer component AutomlRunsTableRow and assert the actual rendered <Label> receives the expected props (e.g., color or status) and still uses variant="outline". Specifically, replace or augment calls to getStatusLabelProps('RUNNING'|'PENDING'|'INCOMPLETE'|...) with a render of AutomlRunsTableRow (or the small wrapper that renders the Label) and query the Label element to assert it has the expected color/status and variant="outline", so the test fails if AutomlRunsTableRow stops spreading props to Label or drops the outline variant.packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx (1)
41-47: HandlePAUSEDandCANCELLEDexplicitly.Those are known
RUN_STATEvalues, but they currently inherit the unknown-state fallback on Line 47. Keeping explicit branches here makes the neutral styling intentional and prevents future fallback changes from silently retheming known states.Possible change
if (s === RUN_STATE.SKIPPED || s.includes(RUN_STATE.SKIPPED)) { return { status: 'success' }; } if (s === RUN_STATE.INCOMPLETE) { return { status: 'warning' }; } + if (s === RUN_STATE.PAUSED || s === RUN_STATE.CANCELLED) { + return { color: 'grey' }; + } return { color: 'grey' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx` around lines 41 - 47, Add explicit branches for RUN_STATE.PAUSED and RUN_STATE.CANCELLED so they don't fall through to the unknown-state fallback: in the same conditional block that checks RUN_STATE.SKIPPED and RUN_STATE.INCOMPLETE, add checks for s === RUN_STATE.PAUSED and s === RUN_STATE.CANCELLED and return the neutral styling (e.g., { color: 'grey' }) for both; keep the existing returns for SKIPPED and INCOMPLETE and leave the final fallback untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/__mocks__/mockWorkloadK8sResource.ts`:
- Around line 205-208: The empty-workload branch unconditionally returns
mockWorkloadEmptySucceedCondition[WorkloadStatusType.Complete], which overrides
the requested mockStatus; update the conditional so that when
mockStatusEmptyWorkload is true you return
mockWorkloadEmptySucceedCondition[WorkloadStatusType.Complete] only if
mockStatus === WorkloadStatusType.Complete, otherwise fall back to
mockWorkloadStatusConditions[mockStatus] (i.e., preserve non-Complete statuses);
adjust the ternary around mockStatusEmptyWorkload to check mockStatus before
selecting mockWorkloadEmptySucceedCondition.
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchKueue.cy.ts`:
- Around line 88-90: The test step description and assertion disagree: the
cy.step text says "Wait for workbench to reach Running status" but the code
calls workbenchPage.getNotebookRow(workbenchName) and then
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 300000). Either
update the cy.step message to match the assertion (e.g., "Wait for workbench to
reach Ready status") or change the assertion to expect
NotebookStatusLabel.Running; locate the call sites to
workbenchPage.getNotebookRow and notebookRow.expectStatusLabelToBe to make the
text and expected enum value consistent.
In
`@packages/cypress/cypress/tests/e2e/modelRegistry/testRegistryDeployModel.cy.ts`:
- Line 225: The assertion currently uses cy.contains(ModelStateLabel.READY)
which is global and can match unrelated text; change it to scope the check to
the deployed model's row by selecting the row's data-testid (e.g., the
page-object/getter for the deployed model row) and assert the status cell inside
that row contains ModelStateLabel.READY; update the test that currently calls
cy.contains(ModelStateLabel.READY) so it first queries the row-level data-testid
(like the deployed-model-row testid or the page-object method that returns it)
and then .find/.contains the status cell to ensure the READY label is asserted
only for the correct model.
In
`@packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.ts`:
- Around line 95-98: Update the step description to match the asserted status:
change the cy.step message that references testData.workbenchName from "Running"
to "Ready" so it aligns with
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 120000); locate the
block around cy.step, workbenchPage.getNotebookRow(testData.workbenchName) and
notebookRow.findNotebookDescription(projectDescription) and make the text
consistent.
In
`@packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testWorkbenchTolerations.cy.ts`:
- Line 175: The test assumes the workbench is stopped before toggling restart;
make the restart flow deterministic by adding an explicit precondition that
ensures the workbench is in the Stopped state before performing the toggle: use
notebookRow to check the current status (e.g.,
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Stopped, timeout)) and if
it is not Stopped, call the existing stop action on the row (e.g.,
notebookRow.clickStop() or equivalent) and wait for the Stopped label, then
proceed with the restart toggle and final ready assertion; add this setup either
at the start of the test or in a beforeEach so the test is idempotent.
In `@packages/eval-hub/frontend/src/app/components/EvaluationStatusLabel.tsx`:
- Line 49: The component EvaluationStatusLabel currently depends on a global CSS
class 'odh-u-spin' applied to InProgressIcon; create a local CSS module named
EvaluationStatusLabel.module.scss containing a .spin rule (animation:
pf-v6-c-spinner-animation-rotate 3s linear infinite), import it into
EvaluationStatusLabel.tsx (e.g., import styles from
'./EvaluationStatusLabel.module.scss') and replace the global class usage by
setting the icon prop to <InProgressIcon className={styles.spin}/> so the plugin
no longer relies on the host app's global stylesheet.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx`:
- Around line 26-40: The "Unknown" label currently uses color="grey" instead of
the PatternFly v6 status prop; in the component that renders different statuses
(the Label usage within MCPServerStatus), change the Unknown case to use
status="info" (and remove color="grey") so it matches other cases (e.g., the
'success','warning','danger' branches) and aligns with the PatternFly v6 Label
API; keep the existing icon and text but switch to status="info" for
consistency.
---
Outside diff comments:
In `@frontend/src/concepts/distributedWorkloads/utils.tsx`:
- Around line 123-128: The "Complete" condition matcher currently only checks
for /success|succeeded/ in message/reason and misses structured reasons like
"JobFinished"; update the Complete: conditions?.find(...) predicate to treat
status==='True' && type==='Finished' as complete if reason === 'JobFinished' OR
the existing regex matches (i.e., add an explicit reason === 'JobFinished'
branch alongside the regex check), and add a regression test that creates a
Finished=True condition with reason: 'JobFinished' and an empty message (using
the existing mockWorkloadK8sResource fixture) to ensure the new logic correctly
classifies the workload as complete.
In `@frontend/src/concepts/topology/NodeStatusIcon.tsx`:
- Around line 15-17: NodeStatusIcon is receiving RunStatus enum values via the
runStatus prop (originating from PipelineDefaultTaskGroup) but the switch uses
runtimeStateLabels display strings (e.g., 'Complete', 'Running'), so all cases
fall through; fix by normalizing the incoming value before the switch: convert
runStatus (RunStatus enum string) into the matching RuntimeStateKF/display label
(using a small mapping or a helper like mapRunStatusToRuntimeState) or accept
enum values directly in the switch by replacing runtimeStateLabels[...] checks
with the corresponding RunStatus enum names; update NodeStatusIcon (and any
helper you add) to perform this mapping and then use the normalized label
variable in the existing switch to select icon/status.
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchStatus.cy.ts`:
- Around line 89-103: The test contains stale step text/comments that say
"Running" while the actual assertions use NotebookStatusLabel.Ready; update the
descriptive strings and inline comments to reflect "Ready" (or the correct
intended status) so they match the assertions: change the cy.step call that
currently says `Wait for workbench ${workbenchName} to display a "Running"
status` and the later cy.step/comment that says 'Click on Running status...' to
reference "Ready" (and any other nearby comments or step descriptions), keeping
the assertions using
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready) and
workbenchStatusModal.getNotebookStatus(NotebookStatusLabel.Ready) unchanged.
In
`@packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.ts`:
- Around line 138-149: Update the stale test comments that refer to "running" to
accurately reflect the expected Ready state: change the cy.step description
texts around the navigation and verification steps in the
testDeleteAppliedHardwareProfile.cy.ts (the two cy.step calls before project
navigation and before status assertion) so they mention "ready" or "Ready"
instead of "running"; this keeps the comments consistent with
workbenchPage.getNotebookRow(...),
workbenchRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 30000), and the
assertion logic.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx`:
- Around line 43-47: MCPServerStatus is inconsistent: some Label usages still
pass color ("grey", "teal") while others use the refactored status prop. Update
the two Label instances that use color (the one rendering Unknown with
QuestionCircleIcon and the one rendering Healthy with CheckCircleIcon) to use
the same status prop name and values as the other cases (replace
color="grey"/"teal" with the equivalent status="…" values used elsewhere) so all
branches consistently use status; ensure the icon and label text remain
unchanged.
---
Nitpick comments:
In `@frontend/src/concepts/notebooks/NotebookStatusLabel.tsx`:
- Around line 44-88: Remove the unnecessary React.useMemo around labelSettings
in NotebookStatusLabel.tsx: replace the useMemo block that computes
labelSettings (typed as StatusLabelSettings) with a plain synchronous
computation (keep the same conditional logic using isError, isStopping,
kueueStatus with KUEUE_STATUSES_OVERRIDE_WORKBENCH and getKueueStatusInfo,
isStarting, isRunning) so labelSettings is a normal const; also update the
dependency array removal and ensure any references to labelSettings remain
unchanged.
In
`@packages/automl/frontend/src/app/components/AutomlRunsTable/__tests__/AutomlRunsTableRow.spec.tsx`:
- Around line 38-68: Tests only call getStatusLabelProps directly; update them
to render the consumer component AutomlRunsTableRow and assert the actual
rendered <Label> receives the expected props (e.g., color or status) and still
uses variant="outline". Specifically, replace or augment calls to
getStatusLabelProps('RUNNING'|'PENDING'|'INCOMPLETE'|...) with a render of
AutomlRunsTableRow (or the small wrapper that renders the Label) and query the
Label element to assert it has the expected color/status and variant="outline",
so the test fails if AutomlRunsTableRow stops spreading props to Label or drops
the outline variant.
In
`@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx`:
- Around line 41-47: Add explicit branches for RUN_STATE.PAUSED and
RUN_STATE.CANCELLED so they don't fall through to the unknown-state fallback: in
the same conditional block that checks RUN_STATE.SKIPPED and
RUN_STATE.INCOMPLETE, add checks for s === RUN_STATE.PAUSED and s ===
RUN_STATE.CANCELLED and return the neutral styling (e.g., { color: 'grey' }) for
both; keep the existing returns for SKIPPED and INCOMPLETE and leave the final
fallback untouched.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 89-95: The Label usage in AIModelTableRow uses color="green"/"red"
instead of the package's semantic status prop; update both Label instances in
AIModelTableRow to use status="success" for the active case and status="danger"
for the inactive case and keep the same icon props (CheckCircleIcon /
ExclamationCircleIcon) and variant to match MCPServerStatus.tsx's pattern so
styling is consistent across components.
In `@packages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsx`:
- Line 32: The Label in ApiKeysTableRow is using the old color prop; change it
to the semantic status prop on the Label component (e.g. replace
color={apiKey.status === 'active' ? 'green' : 'red'} with status={apiKey.status
=== 'active' ? 'success' : 'danger'}) so PatternFly v6 renders correct
icons/aria and colors; locate the Label usage in ApiKeysTableRow.tsx and update
the prop value to the appropriate semantic status strings while keeping any
existing variant prop intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e8b5faa6-dbdc-4281-a7e7-f48b4cb52f83
📥 Commits
Reviewing files that changed from the base of the PR and between 140a935 and 08a13f7c79bb8c0a4d943d097e9aa4b39800a5d1.
📒 Files selected for processing (56)
frontend/src/__mocks__/mockWorkloadK8sResource.tsfrontend/src/concepts/__tests__/StartNotebookModal.spec.tsxfrontend/src/concepts/distributedWorkloads/__tests__/utils.spec.tsfrontend/src/concepts/distributedWorkloads/utils.tsxfrontend/src/concepts/kueue/__tests__/index.spec.tsfrontend/src/concepts/kueue/index.tsfrontend/src/concepts/modelServing/ModelStatusIcon.tsxfrontend/src/concepts/notebooks/NotebookStatusLabel.tsxfrontend/src/concepts/notebooks/__tests__/NotebookStatusLabel.spec.tsxfrontend/src/concepts/pipelines/content/PipelineRunTypeLabel.tsxfrontend/src/concepts/pipelines/content/__tests__/utils.spec.tsxfrontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsxfrontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsxfrontend/src/concepts/pipelines/content/tables/renderUtils.tsxfrontend/src/concepts/pipelines/content/utils.tsxfrontend/src/concepts/pipelines/kfTypes.tsfrontend/src/concepts/topology/NodeStatusIcon.tsxfrontend/src/pages/distributedWorkloads/components/WorkloadStatusLabel.tsxfrontend/src/pages/distributedWorkloads/global/projectMetrics/sections/TopResourceConsumingWorkloads.tsxfrontend/src/pages/lmEval/global/lmEvalList/lmEvalStatus/LMEvalStatusLabel.tsxfrontend/src/pages/modelServing/screens/ServingRuntimeVersionStatus.tsxfrontend/src/pages/pipelines/global/experiments/ExperimentPipelineRuns.tsxfrontend/src/pages/pipelines/global/experiments/artifacts/ArtifactDetails/ArtifactDetailsTitle.tsxfrontend/src/pages/pipelines/global/experiments/artifacts/ArtifactsTableRow.tsxfrontend/src/pages/pipelines/global/experiments/executions/ExecutionStatus.tsxfrontend/src/pages/projects/screens/detail/storage/StorageTableRow.tsxpackages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsxpackages/automl/frontend/src/app/components/AutomlRunsTable/__tests__/AutomlRunsTableRow.spec.tsxpackages/autorag/frontend/src/app/components/AutoragRunsTable/AutoragRunsTableRow.tsxpackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchControlSuite.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchCreation.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchKueue.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchStatus.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchVariables.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/workbenches.cy.tspackages/cypress/cypress/tests/e2e/modelRegistry/testRegistryDeployModel.cy.tspackages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.tspackages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testWorkbenchTolerations.cy.tspackages/cypress/cypress/tests/mocked/distributedWorkloads/globalDistributedWorkloads.cy.tspackages/cypress/cypress/tests/mocked/modelServing/modelServingLlmd.cy.tspackages/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.tspackages/cypress/cypress/tests/mocked/modelTraining/modelTraining.cy.tspackages/cypress/cypress/tests/mocked/modelTraining/modelTrainingPauseResume.cy.tspackages/cypress/cypress/tests/mocked/projects/tabs/workbench.cy.tspackages/cypress/cypress/types.tspackages/eval-hub/frontend/src/app/components/EvaluationStatusLabel.tsxpackages/eval-hub/frontend/src/app/components/__tests__/EvaluationStatusLabel.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsxpackages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsxpackages/model-serving/src/components/deploymentWizard/types.tspackages/model-serving/src/components/deployments/__tests__/DeploymentsTableRow.spec.tsxpackages/model-training/src/global/trainingJobList/__tests__/utils.spec.tspackages/model-training/src/global/trainingJobList/utils.ts
💤 Files with no reviewable changes (2)
- frontend/src/concepts/kueue/tests/index.spec.ts
- frontend/src/concepts/kueue/index.ts
Comment on lines
88
to
+90
| cy.step('Wait for workbench to reach Running status'); | ||
| const notebookRow = workbenchPage.getNotebookRow(workbenchName); | ||
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Running, 300000); | ||
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 300000); |
Contributor
There was a problem hiding this comment.
Step description inconsistent with assertion.
Line 88 says "Wait for workbench to reach Running status" but line 90 expects NotebookStatusLabel.Ready.
Suggested fix
- cy.step('Wait for workbench to reach Running status');
+ cy.step('Wait for workbench to reach Ready status');
const notebookRow = workbenchPage.getNotebookRow(workbenchName);
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 300000);📝 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.
Suggested change
| cy.step('Wait for workbench to reach Running status'); | |
| const notebookRow = workbenchPage.getNotebookRow(workbenchName); | |
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Running, 300000); | |
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 300000); | |
| cy.step('Wait for workbench to reach Ready status'); | |
| const notebookRow = workbenchPage.getNotebookRow(workbenchName); | |
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 300000); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchKueue.cy.ts`
around lines 88 - 90, The test step description and assertion disagree: the
cy.step text says "Wait for workbench to reach Running status" but the code
calls workbenchPage.getNotebookRow(workbenchName) and then
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 300000). Either
update the cy.step message to match the assertion (e.g., "Wait for workbench to
reach Ready status") or change the assertion to expect
NotebookStatusLabel.Running; locate the call sites to
workbenchPage.getNotebookRow and notebookRow.expectStatusLabelToBe to make the
text and expected enum value consistent.
| @@ -222,7 +222,7 @@ describe('Verify models can be deployed from model registry', () => { | |||
| modelRegistry.visit(); | |||
| cy.contains('1 deployment', { timeout: 30000 }).should('be.visible').click(); | |||
| cy.contains(modelName).should('be.visible'); | |||
| cy.contains(ModelStateLabel.STARTED).should('be.visible'); | |||
| cy.contains(ModelStateLabel.READY).should('be.visible'); | |||
Contributor
There was a problem hiding this comment.
Scope the status assertion to a row-level data-testid selector.
Line 225 uses a global cy.contains(ModelStateLabel.READY), which can pass on unrelated text and cause false positives. Assert the status cell within the deployed model row via a data-testid-backed selector/page-object method.
As per coding guidelines, packages/cypress/**/*.{ts,js}: "Use data-testid selectors, not CSS classes (resilient to style changes)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/e2e/modelRegistry/testRegistryDeployModel.cy.ts`
at line 225, The assertion currently uses cy.contains(ModelStateLabel.READY)
which is global and can match unrelated text; change it to scope the check to
the deployed model's row by selecting the row's data-testid (e.g., the
page-object/getter for the deployed model row) and assert the status cell inside
that row contains ModelStateLabel.READY; update the test that currently calls
cy.contains(ModelStateLabel.READY) so it first queries the row-level data-testid
(like the deployed-model-row testid or the page-object method that returns it)
and then .find/.contains the status cell to ensure the READY label is asserted
only for the correct model.
Comment on lines
95
to
+98
| cy.step(`Wait for workbench ${testData.workbenchName} to display a "Running" status`); | ||
| const notebookRow = workbenchPage.getNotebookRow(testData.workbenchName); | ||
| notebookRow.findNotebookDescription(projectDescription); | ||
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Running, 120000); | ||
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 120000); |
Contributor
There was a problem hiding this comment.
Stale comment references old status label.
Line 95 cy.step message says "Running" but assertion on line 98 expects NotebookStatusLabel.Ready. Update the step description to match.
Proposed fix
- cy.step(`Wait for workbench ${testData.workbenchName} to display a "Running" status`);
+ cy.step(`Wait for workbench ${testData.workbenchName} to display a "Ready" status`);📝 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.
Suggested change
| cy.step(`Wait for workbench ${testData.workbenchName} to display a "Running" status`); | |
| const notebookRow = workbenchPage.getNotebookRow(testData.workbenchName); | |
| notebookRow.findNotebookDescription(projectDescription); | |
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Running, 120000); | |
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 120000); | |
| cy.step(`Wait for workbench ${testData.workbenchName} to display a "Ready" status`); | |
| const notebookRow = workbenchPage.getNotebookRow(testData.workbenchName); | |
| notebookRow.findNotebookDescription(projectDescription); | |
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 120000); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.ts`
around lines 95 - 98, Update the step description to match the asserted status:
change the cy.step message that references testData.workbenchName from "Running"
to "Ready" so it aligns with
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 120000); locate the
block around cy.step, workbenchPage.getNotebookRow(testData.workbenchName) and
notebookRow.findNotebookDescription(projectDescription) and make the text
consistent.
| @@ -172,7 +172,7 @@ describe('Workbenches - tolerations tests', () => { | |||
| cy.step(`Restart workbench ${testData.workbenchName} and validate it has been started`); | |||
| const notebookRow = workbenchPage.getNotebookRow(testData.workbenchName); | |||
| notebookRow.findNotebookStopToggle().click(); | |||
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Running, 120000); | |||
| notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Ready, 120000); | |||
Contributor
There was a problem hiding this comment.
Make restart flow independent of prior test state.
Line 175 assumes the workbench is already stopped before toggling. If this test runs in isolation or with retries, the toggle can execute the wrong action and fail nondeterministically. Add an explicit precondition/setup in this test (or beforeEach) so restart always starts from a known state.
As per coding guidelines, packages/cypress/**/*.{ts,js}: "Tests must be idempotent: runnable in any order without shared state."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testWorkbenchTolerations.cy.ts`
at line 175, The test assumes the workbench is stopped before toggling restart;
make the restart flow deterministic by adding an explicit precondition that
ensures the workbench is in the Stopped state before performing the toggle: use
notebookRow to check the current status (e.g.,
notebookRow.expectStatusLabelToBe(NotebookStatusLabel.Stopped, timeout)) and if
it is not Stopped, call the existing stop action on the row (e.g.,
notebookRow.clickStop() or equivalent) and wait for the Stopped label, then
proceed with the restart toggle and final ready assertion; add this setup either
at the start of the test or in a beforeEach so the test is idempotent.
| color: 'blue', | ||
| icon: <PausedIcon />, | ||
| color: 'grey', | ||
| icon: <InProgressIcon className="odh-u-spin" />, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where odh-u-spin is defined and whether it's a CSS module or global stylesheet
rg -n "odh-u-spin" --type css --type scss
fd -e css -e scss | xargs grep -l "odh-u-spin" 2>/dev/null | head -20Repository: opendatahub-io/odh-dashboard
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Check CSS imports in the eval-hub plugin
echo "=== Checking CSS imports in EvaluationStatusLabel.tsx ==="
head -20 packages/eval-hub/frontend/src/app/components/EvaluationStatusLabel.tsx
echo -e "\n=== Checking CSS module imports in eval-hub plugin ==="
fd -e css -e scss packages/eval-hub/frontend/src/ | head -20
echo -e "\n=== Checking index.tsx for CSS imports ==="
find packages/eval-hub/frontend/src -name "index.tsx" -o -name "index.ts" | xargs grep -l "\.css\|\.scss" 2>/dev/null | head -10Repository: opendatahub-io/odh-dashboard
Length of output: 1161
🏁 Script executed:
#!/bin/bash
# Search for PatternFly spinning/loading animations in dependencies or existing usage
echo "=== Checking if PatternFly has built-in spinner animations ==="
rg "Spinner\|Loading\|Skeleton" packages/eval-hub/frontend/src --type ts --type tsx -A 2 | head -40Repository: opendatahub-io/odh-dashboard
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Verify the context where odh-u-spin is used across the codebase
echo "=== All usages of odh-u-spin class ==="
rg "odh-u-spin" --type ts --type tsx --type jsx -B 2 -A 2Repository: opendatahub-io/odh-dashboard
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Check root index and module federation entry points
echo "=== Looking for CSS imports in eval-hub plugin root files ==="
find packages/eval-hub/frontend/src -maxdepth 3 -type f \( -name "*.tsx" -o -name "*.ts" \) -exec grep -l "\.css\|\.scss" {} \;
echo -e "\n=== Check App.tsx or index entry points ==="
find packages/eval-hub/frontend/src -name "App.tsx" -o -name "index.tsx" -o -name "index.ts" | head -5
echo -e "\n=== Full file content of EvaluationStatusLabel.tsx to see all imports ==="
cat packages/eval-hub/frontend/src/app/components/EvaluationStatusLabel.tsxRepository: opendatahub-io/odh-dashboard
Length of output: 2247
🏁 Script executed:
#!/bin/bash
# Find where odh-u-spin is actually used and its context
echo "=== Grep for odh-u-spin in all TypeScript/TSX files ==="
grep -r "odh-u-spin" packages/eval-hub/frontend/src/ 2>/dev/null || echo "Not found in eval-hub plugin"
echo -e "\n=== Check if odh-u-spin is in the host app's global CSS ==="
grep "odh-u-spin" frontend/src/app/App.scss -B 5 -A 5Repository: opendatahub-io/odh-dashboard
Length of output: 612
Remove dependency on host app's global CSS class; use CSS module instead.
The odh-u-spin class is defined in the host app's global stylesheet (frontend/src/app/App.scss), not imported as a CSS module. This violates plugin isolation requirements: "No global CSS — use PatternFly utility classes or CSS modules only."
Remediation: Create a local CSS module in the eval-hub plugin (e.g., EvaluationStatusLabel.module.scss) with the spin animation, import it, and apply the class locally. This ensures the plugin doesn't depend on host app internals.
/* EvaluationStatusLabel.module.scss */
.spin {
animation: pf-v6-c-spinner-animation-rotate 3s linear infinite;
}Then update the JSX:
import styles from './EvaluationStatusLabel.module.scss';
// ...
icon: <InProgressIcon className={styles.spin} />,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eval-hub/frontend/src/app/components/EvaluationStatusLabel.tsx` at
line 49, The component EvaluationStatusLabel currently depends on a global CSS
class 'odh-u-spin' applied to InProgressIcon; create a local CSS module named
EvaluationStatusLabel.module.scss containing a .spin rule (animation:
pf-v6-c-spinner-animation-rotate 3s linear infinite), import it into
EvaluationStatusLabel.tsx (e.g., import styles from
'./EvaluationStatusLabel.module.scss') and replace the global class usage by
setting the icon prop to <InProgressIcon className={styles.spin}/> so the plugin
no longer relies on the host app's global stylesheet.
Contributor
There was a problem hiding this comment.
@coderabbitai will be addressed as a follow-up in opendatahub-io/mod-arch-library#178 cc @rsun19
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+26
to
40
| <Label status="success" icon={<CheckCircleIcon />}> | ||
| Active | ||
| </Label> | ||
| ); | ||
| case 'auth_required': | ||
| return ( | ||
| <Label color="yellow" icon={<InfoCircleIcon />}> | ||
| <Label status="warning" icon={<InfoCircleIcon />}> | ||
| Token Required | ||
| </Label> | ||
| ); | ||
| case 'unreachable': | ||
| return ( | ||
| <Label color="red" icon={<ExclamationCircleIcon />}> | ||
| <Label status="danger" icon={<ExclamationCircleIcon />}> | ||
| Error | ||
| </Label> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PatternFly v6 Label component status prop documentation
💡 Result:
In PatternFly v6, the Label component has a 'status' prop that sets the status of the label with a respective icon and color. Possible values are 'success', 'warning', 'danger', 'info', or 'custom'. It overrides the 'color' prop if both are set. Status labels are used to communicate states like success, warning, danger, or info, combining appropriate colors and icons. Example usage (React): - status="success": Green background with success icon. - status="danger": Red background with danger icon. This is distinct from the 'color' prop, which uses non-status colors like 'blue', 'teal', 'green', etc., without automatic icons unless specified separately.
Citations:
🏁 Script executed:
cat packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsxRepository: opendatahub-io/odh-dashboard
Length of output: 1797
Migrate "Unknown" status to use status="info" for consistency with PatternFly v6 patterns.
Lines 44–46 use color="grey" while other status states use the status prop. Migrate the "Unknown" case to status="info" to maintain consistency and align with PatternFly v6 Label API. The "Loading" state (lines 54–61) correctly uses color="teal" with variant="outline" for the distinct loading indicator pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx`
around lines 26 - 40, The "Unknown" label currently uses color="grey" instead of
the PatternFly v6 status prop; in the component that renders different statuses
(the Label usage within MCPServerStatus), change the Unknown case to use
status="info" (and remove color="grey") so it matches other cases (e.g., the
'success','warning','danger' branches) and aligns with the PatternFly v6 Label
API; keep the existing icon and text but switch to status="info" for
consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchControlSuite.cy.ts (2)
102-107:⚠️ Potential issue | 🟡 MinorFix stale status wording in the step log.
Line 103 says
"Running"while Line 106 now assertsNotebookStatusLabel.Ready. This makes CI triage misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchControlSuite.cy.ts` around lines 102 - 107, The test step message is out of sync with the asserted status: update the step log string that references workbenchName to reflect the actual expected status (NotebookStatusLabel.Ready) instead of "Running"; locate the call site using getNotebookRow(workbenchName) / notebookRow and change the cy.step text to mention "Ready" (or a matching phrase) so the step message and the expectStatusLabelToBe(notebookStatusLabel.Ready, 120000) assertion are consistent.
179-191:⚠️ Potential issue | 🟡 MinorAlign step text with the new Ready status.
Line 180 says
"Click on Running status"but the flow now validatesNotebookStatusLabel.Readyat Line 191. Update the step text to avoid misleading execution logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchControlSuite.cy.ts` around lines 179 - 191, Update the cy.step text to reflect the new Ready status instead of "Click on Running status": change the step invocation near the stop/restart sequence (the cy.step call before notebookRow.findHaveNotebookStatusText().click()) so it describes clicking the current status and restarting to reach Ready; ensure the related calls (workbenchPage.getNotebookRow(workbenchName), workbenchStatusModal.getNotebookStatus(...), findStopWorkbenchFooterButton, findStopWorkbenchButton, findStartWorkbenchFooterButton) remain unchanged and only the human-readable step string is updated to mention "Ready" or "current status" to match the subsequent getNotebookStatus checks.
🧹 Nitpick comments (3)
packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx (1)
41-43: SKIPPED returningstatus: 'success'may be semantically misleading.A skipped run is not a successful run—it was intentionally bypassed. Using
successstyling could confuse users into thinking the run completed successfully. Consider using a neutral color like{ color: 'grey' }or{ color: 'gold' }to differentiate from actual successes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx` around lines 41 - 43, The code maps RUN_STATE.SKIPPED to { status: 'success' }, which is misleading; in AutomlRunsTableRow (the function that maps run states to UI status) change the SKIPPED branch (the if using RUN_STATE.SKIPPED and s.includes(RUN_STATE.SKIPPED)) to return a neutral styling such as { color: 'grey' } (or { status: 'neutral' } if the UI supports a neutral status) instead of 'success', and update any downstream consumers/tests that expect 'success' for skipped runs to accept the new neutral value.frontend/src/pages/distributedWorkloads/global/projectMetrics/sections/TopResourceConsumingWorkloads.tsx (1)
142-146: Avoid recomputing status in two full scans.
getStatusInfo(wl)is non-trivial; calling it in two separate.some()passes overworkloads.datadoubles work in this render path. Compute statuses once in a single pass and derive both flags from that.Suggested refactor
- const workloadStatuses = []; - if (workloads.data.some((wl) => getStatusInfo(wl).status === WorkloadStatusType.Complete)) { - workloadStatuses.push('completed'); - } - if (workloads.data.some((wl) => getStatusInfo(wl).status === WorkloadStatusType.Failed)) { - workloadStatuses.push('failed'); - } + const seenStatuses = new Set<WorkloadStatusType>(); + for (const wl of workloads.data) { + const { status } = getStatusInfo(wl); + if (status === WorkloadStatusType.Complete || status === WorkloadStatusType.Failed) { + seenStatuses.add(status); + } + if ( + seenStatuses.has(WorkloadStatusType.Complete) && + seenStatuses.has(WorkloadStatusType.Failed) + ) { + break; + } + } + + const workloadStatuses = [ + ...(seenStatuses.has(WorkloadStatusType.Complete) ? ['completed'] : []), + ...(seenStatuses.has(WorkloadStatusType.Failed) ? ['failed'] : []), + ];As per coding guidelines, "
**: REVIEW PRIORITIES: ... 4. Performance problems".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/distributedWorkloads/global/projectMetrics/sections/TopResourceConsumingWorkloads.tsx` around lines 142 - 146, The code calls getStatusInfo(wl) twice via two .some() scans over workloads.data which doubles work; change to a single pass over workloads.data where you call getStatusInfo(wl) once per workload and set flags/collect statuses accordingly: iterate workloads.data, call getStatusInfo(wl) once, if status === WorkloadStatusType.Complete add 'completed' to workloadStatuses (and mark a completed flag) and if status === WorkloadStatusType.Failed add 'failed' (and mark a failed flag); update any downstream logic that relied on the two .some() results to use the booleans or workloadStatuses populated during that single pass (refer to workloadStatuses, getStatusInfo, WorkloadStatusType, and workloads.data).frontend/src/concepts/pipelines/content/__tests__/utils.spec.tsx (1)
98-104: ExtendlabelStatuscoverage toSUCCEEDEDandFAILED.This segment adds the new
labelStatusassertion forSKIPPED, but Line 38 infrontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsxnow renders label styling fromlabelStatusfor all terminal states. The existingSUCCEEDEDandFAILEDspecs still only assertrunStatus.status, so a broken label-color path there would not fail tests. Please mirrorexpect(runStatus.labelStatus).toBe('success')andexpect(runStatus.labelStatus).toBe('danger')in those cases.As per coding guidelines, "Test behavior, not implementation details. Test names must describe the verified behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/concepts/pipelines/content/__tests__/utils.spec.tsx` around lines 98 - 104, The SKIPPED test adds an assertion for label styling but SUCCEEDED and FAILED cases still only check runStatus.status; update the tests that call computeRunStatus(createRun(RuntimeStateKF.SUCCEEDED)) and computeRunStatus(createRun(RuntimeStateKF.FAILED)) to also assert runStatus.labelStatus (expect(...).toBe('success') for SUCCEEDED and expect(...).toBe('danger') for FAILED) so label styling coverage matches runtime status coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/concepts/modelServing/ModelStatusIcon.tsx`:
- Line 53: The InProgressIcon in ModelStatusIcon.tsx is being spun via the
custom CSS class odh-u-spin (defined in App.scss) but lacks
prefers-reduced-motion handling and resides outside the shared utilities; either
update the odh-u-spin rule in App.scss to include a prefers-reduced-motion:
reduce media query that disables the animation, or replace the InProgressIcon
usage in ModelStatusIcon.tsx with PatternFly's Spinner component so motion is
handled by PF utilities and conforms to accessibility guidelines; ensure the CSS
change is made where odh-u-spin is defined and that ModelStatusIcon.tsx (the
place that references InProgressIcon) is updated accordingly.
- Around line 73-79: ModelStatusIcon.tsx currently groups
ModelDeploymentState.STANDBY with LOADED, causing the UI to show "Ready" for a
transitional state; update the mapping so STANDBY is removed from the LOADED
case and gets its own branch (or mapped to the "Starting" label) with an
appropriate icon/message to reflect non-running semantics; locate the
switch/case handling ModelDeploymentState (references:
ModelDeploymentState.STANDBY, ModelDeploymentState.LOADED) and change the case
grouping to match the backend/utils isRunning logic (see isRunning behavior in
packages/model-serving/src/concepts/utils.ts) so STANDBY displays "Starting" (or
distinct transitional state) instead of "Ready".
In `@frontend/src/concepts/pipelines/content/__tests__/utils.spec.tsx`:
- Around line 62-69: Add a test in
frontend/src/concepts/pipelines/content/__tests__/utils.spec.tsx that calls
computeRunStatus(undefined) (instead of createRun('-')) to exercise the run ===
undefined branch and assert the same expected behavior: label === '-'
(UNKNOWN_LABEL), icon strictly equals OutlinedQuestionCircleIcon (UNKNOWN_ICON)
and color === 'grey'; reference computeRunStatus and createRun in the test
name/description to make clear you are verifying the undefined-run fallback
behavior.
In `@frontend/src/concepts/pipelines/content/PipelineRunTypeLabel.tsx`:
- Line 37: The JSX opening tag for the Label component in PipelineRunTypeLabel
(the <Label variant="outline" color="green" isCompact={isCompact}
data-testid="model-registered-label"> line) is misformatted; if Label has no
children, convert the opening tag to a self-closing tag (add a trailing "/>")
and ensure props are properly spaced/wrapped to satisfy the formatter, otherwise
add the matching closing </Label> and format the opening and closing tags on
separate lines to fix the lint/formatter error.
In
`@frontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsx`:
- Around line 38-45: The JSX for the two Label elements (the one with props
color={color} status={labelStatus} icon={icon} and the "Model registered" Label)
violates the formatter; reflow them so each prop is on its own line and the
child text/JSX is on its own line with the closing tag on its own line (e.g.,
split props across lines for the Label in PipelineDetailsTitle and place the
{label} child on its own line, and similarly place "Model registered" as a
separate child line), ensuring consistent indentation inside the SplitItem
components.
In
`@frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsx`:
- Line 41: Reformat the Label component usage in PipelineRunTableRowExperiment
by moving the child text onto its own line to satisfy the formatter: change the
single-line JSX <Label variant="outline" isCompact>Archived</Label> to a
multi-line form so the child text ("Archived") is on a separate line inside the
Label component; update the JSX around the Label instance to match this style
(look for the Label element in PipelineRunTableRowExperiment and adjust its
opening/closing tags and inner text accordingly).
In
`@frontend/src/pages/pipelines/global/experiments/artifacts/ArtifactDetails/ArtifactDetailsTitle.tsx`:
- Line 20: The Label element in the ArtifactDetailsTitle component currently has
its child text on the same line (Label variant="outline"
color="green">Registered</Label) which breaks the project's lint/format rules;
update the JSX so the Label's children are placed on their own line(s) (e.g.,
open <Label ...>, put "Registered" on a separate line, then close </Label>) in
the ArtifactDetailsTitle.tsx file to satisfy the formatter and lint checks.
In
`@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx`:
- Around line 44-46: The check for INCOMPLETE is inconsistent: update the
conditional in the AutomlRunsTableRow logic that currently reads "if (s ===
RUN_STATE.INCOMPLETE)" to also include the substring fallback so it matches the
other cases (i.e., treat INCOMPLETE as true when s === RUN_STATE.INCOMPLETE ||
s.includes(RUN_STATE.INCOMPLETE)); adjust the return block for that branch
(status: 'warning') accordingly so behavior is consistent with
RUNNING/PENDING/SKIPPED/FAILED/SUCCEEDED checks.
---
Outside diff comments:
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchControlSuite.cy.ts`:
- Around line 102-107: The test step message is out of sync with the asserted
status: update the step log string that references workbenchName to reflect the
actual expected status (NotebookStatusLabel.Ready) instead of "Running"; locate
the call site using getNotebookRow(workbenchName) / notebookRow and change the
cy.step text to mention "Ready" (or a matching phrase) so the step message and
the expectStatusLabelToBe(notebookStatusLabel.Ready, 120000) assertion are
consistent.
- Around line 179-191: Update the cy.step text to reflect the new Ready status
instead of "Click on Running status": change the step invocation near the
stop/restart sequence (the cy.step call before
notebookRow.findHaveNotebookStatusText().click()) so it describes clicking the
current status and restarting to reach Ready; ensure the related calls
(workbenchPage.getNotebookRow(workbenchName),
workbenchStatusModal.getNotebookStatus(...), findStopWorkbenchFooterButton,
findStopWorkbenchButton, findStartWorkbenchFooterButton) remain unchanged and
only the human-readable step string is updated to mention "Ready" or "current
status" to match the subsequent getNotebookStatus checks.
---
Nitpick comments:
In `@frontend/src/concepts/pipelines/content/__tests__/utils.spec.tsx`:
- Around line 98-104: The SKIPPED test adds an assertion for label styling but
SUCCEEDED and FAILED cases still only check runStatus.status; update the tests
that call computeRunStatus(createRun(RuntimeStateKF.SUCCEEDED)) and
computeRunStatus(createRun(RuntimeStateKF.FAILED)) to also assert
runStatus.labelStatus (expect(...).toBe('success') for SUCCEEDED and
expect(...).toBe('danger') for FAILED) so label styling coverage matches runtime
status coverage.
In
`@frontend/src/pages/distributedWorkloads/global/projectMetrics/sections/TopResourceConsumingWorkloads.tsx`:
- Around line 142-146: The code calls getStatusInfo(wl) twice via two .some()
scans over workloads.data which doubles work; change to a single pass over
workloads.data where you call getStatusInfo(wl) once per workload and set
flags/collect statuses accordingly: iterate workloads.data, call
getStatusInfo(wl) once, if status === WorkloadStatusType.Complete add
'completed' to workloadStatuses (and mark a completed flag) and if status ===
WorkloadStatusType.Failed add 'failed' (and mark a failed flag); update any
downstream logic that relied on the two .some() results to use the booleans or
workloadStatuses populated during that single pass (refer to workloadStatuses,
getStatusInfo, WorkloadStatusType, and workloads.data).
In
`@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx`:
- Around line 41-43: The code maps RUN_STATE.SKIPPED to { status: 'success' },
which is misleading; in AutomlRunsTableRow (the function that maps run states to
UI status) change the SKIPPED branch (the if using RUN_STATE.SKIPPED and
s.includes(RUN_STATE.SKIPPED)) to return a neutral styling such as { color:
'grey' } (or { status: 'neutral' } if the UI supports a neutral status) instead
of 'success', and update any downstream consumers/tests that expect 'success'
for skipped runs to accept the new neutral value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 59b3af0a-96ea-436f-9d78-18a784b3ae85
📥 Commits
Reviewing files that changed from the base of the PR and between 08a13f7c79bb8c0a4d943d097e9aa4b39800a5d1 and 269eb259ede88b85d8baff0fe07eca0100034495.
📒 Files selected for processing (51)
frontend/src/__mocks__/mockWorkloadK8sResource.tsfrontend/src/concepts/__tests__/StartNotebookModal.spec.tsxfrontend/src/concepts/distributedWorkloads/__tests__/utils.spec.tsfrontend/src/concepts/distributedWorkloads/utils.tsxfrontend/src/concepts/kueue/__tests__/index.spec.tsfrontend/src/concepts/kueue/index.tsfrontend/src/concepts/modelServing/ModelStatusIcon.tsxfrontend/src/concepts/notebooks/NotebookStatusLabel.tsxfrontend/src/concepts/notebooks/__tests__/NotebookStatusLabel.spec.tsxfrontend/src/concepts/pipelines/content/PipelineRunTypeLabel.tsxfrontend/src/concepts/pipelines/content/__tests__/utils.spec.tsxfrontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsxfrontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsxfrontend/src/concepts/pipelines/content/tables/renderUtils.tsxfrontend/src/concepts/pipelines/content/utils.tsxfrontend/src/concepts/pipelines/kfTypes.tsfrontend/src/concepts/topology/NodeStatusIcon.tsxfrontend/src/pages/distributedWorkloads/components/WorkloadStatusLabel.tsxfrontend/src/pages/distributedWorkloads/global/projectMetrics/sections/TopResourceConsumingWorkloads.tsxfrontend/src/pages/lmEval/global/lmEvalList/lmEvalStatus/LMEvalStatusLabel.tsxfrontend/src/pages/modelServing/screens/ServingRuntimeVersionStatus.tsxfrontend/src/pages/pipelines/global/experiments/ExperimentPipelineRuns.tsxfrontend/src/pages/pipelines/global/experiments/artifacts/ArtifactDetails/ArtifactDetailsTitle.tsxfrontend/src/pages/pipelines/global/experiments/artifacts/ArtifactsTableRow.tsxfrontend/src/pages/pipelines/global/experiments/executions/ExecutionStatus.tsxfrontend/src/pages/projects/screens/detail/storage/StorageTableRow.tsxpackages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsxpackages/automl/frontend/src/app/components/AutomlRunsTable/__tests__/AutomlRunsTableRow.spec.tsxpackages/autorag/frontend/src/app/components/AutoragRunsTable/AutoragRunsTableRow.tsxpackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchControlSuite.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchCreation.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchKueue.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchStatus.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchVariables.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/workbenches.cy.tspackages/cypress/cypress/tests/e2e/modelRegistry/testRegistryDeployModel.cy.tspackages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.tspackages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testWorkbenchTolerations.cy.tspackages/cypress/cypress/tests/mocked/distributedWorkloads/globalDistributedWorkloads.cy.tspackages/cypress/cypress/tests/mocked/modelServing/modelServingLlmd.cy.tspackages/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.tspackages/cypress/cypress/tests/mocked/modelTraining/modelTraining.cy.tspackages/cypress/cypress/tests/mocked/modelTraining/modelTrainingPauseResume.cy.tspackages/cypress/cypress/tests/mocked/projects/tabs/workbench.cy.tspackages/cypress/cypress/types.tspackages/eval-hub/frontend/src/app/components/EvaluationStatusLabel.tsxpackages/eval-hub/frontend/src/app/components/__tests__/EvaluationStatusLabel.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsxpackages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsx
💤 Files with no reviewable changes (2)
- frontend/src/concepts/kueue/index.ts
- frontend/src/concepts/kueue/tests/index.spec.ts
✅ Files skipped from review due to trivial changes (19)
- packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.ts
- frontend/src/pages/pipelines/global/experiments/ExperimentPipelineRuns.tsx
- packages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsx
- frontend/src/pages/pipelines/global/experiments/artifacts/ArtifactsTableRow.tsx
- packages/cypress/cypress/tests/e2e/modelRegistry/testRegistryDeployModel.cy.ts
- frontend/src/concepts/notebooks/tests/NotebookStatusLabel.spec.tsx
- packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchCreation.cy.ts
- packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchVariables.cy.ts
- packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchKueue.cy.ts
- frontend/src/pages/modelServing/screens/ServingRuntimeVersionStatus.tsx
- frontend/src/concepts/tests/StartNotebookModal.spec.tsx
- packages/cypress/cypress/tests/mocked/distributedWorkloads/globalDistributedWorkloads.cy.ts
- packages/cypress/cypress/tests/mocked/modelServing/modelServingLlmd.cy.ts
- packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testDeleteAppliedHardwareProfile.cy.ts
- frontend/src/concepts/pipelines/content/tables/renderUtils.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx
- packages/eval-hub/frontend/src/app/components/tests/EvaluationStatusLabel.spec.tsx
- packages/cypress/cypress/tests/mocked/modelTraining/modelTraining.cy.ts
- packages/cypress/cypress/tests/mocked/projects/tabs/workbench.cy.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- frontend/src/pages/projects/screens/detail/storage/StorageTableRow.tsx
- packages/cypress/cypress/types.ts
- frontend/src/concepts/pipelines/kfTypes.ts
- packages/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts
- packages/cypress/cypress/tests/mocked/modelTraining/modelTrainingPauseResume.cy.ts
- frontend/src/pages/distributedWorkloads/components/WorkloadStatusLabel.tsx
- packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/testWorkbenchStatus.cy.ts
- packages/autorag/frontend/src/app/components/AutoragRunsTable/AutoragRunsTableRow.tsx
- frontend/src/concepts/notebooks/NotebookStatusLabel.tsx
- frontend/src/concepts/topology/NodeStatusIcon.tsx
- packages/automl/frontend/src/app/components/AutomlRunsTable/tests/AutomlRunsTableRow.spec.tsx
- frontend/src/concepts/distributedWorkloads/tests/utils.spec.ts
- frontend/src/mocks/mockWorkloadK8sResource.ts
- frontend/src/pages/pipelines/global/experiments/executions/ExecutionStatus.tsx
- packages/cypress/cypress/tests/e2e/dataScienceProjects/workbenches/workbenches.cy.ts
- packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts
- frontend/src/concepts/distributedWorkloads/utils.tsx
- packages/cypress/cypress/tests/e2e/settings/hardwareProfiles/testWorkbenchTolerations.cy.ts
- frontend/src/pages/lmEval/global/lmEvalList/lmEvalStatus/LMEvalStatusLabel.tsx
| @@ -51,7 +50,7 @@ export const ModelStatusIcon: React.FC<ModelStatusIconProps> = ({ | |||
| return { | |||
| label: 'Stopping', | |||
| color: 'grey', | |||
| icon: <SyncAltIcon className="odh-u-spin" />, | |||
| icon: <InProgressIcon className="odh-u-spin" />, | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where odh-u-spin is defined and how it's used.
rg -n --type=ts --type=tsx --type=css --type=scss '\bodh-u-spin\b' -C2
# Check whether the defining stylesheet includes reduced-motion handling.
for f in $(rg -n --type=css --type=scss '\bodh-u-spin\b' | cut -d: -f1 | sort -u); do
echo "---- $f ----"
rg -n 'odh-u-spin|prefers-reduced-motion|animation' "$f"
doneRepository: opendatahub-io/odh-dashboard
Length of output: 128
🏁 Script executed:
# Search for odh-u-spin without restrictive type filters first
rg -n 'odh-u-spin' -C2
# Also search for the file to understand context
cat frontend/src/concepts/modelServing/ModelStatusIcon.tsx | head -60Repository: opendatahub-io/odh-dashboard
Length of output: 7953
Add prefers-reduced-motion to odh-u-spin or use PatternFly's Spinner component.
Line 53 uses InProgressIcon with className="odh-u-spin", which applies a custom spinning animation from frontend/src/app/App.scss. The animation lacks prefers-reduced-motion handling, violating WCAG 2.1 SC 2.3.3 (Motion from Interactions). Additionally, this custom class is not in the guideline-specified shared utilities location (frontend/src/concepts/dashboard).
Either:
- Add
prefers-reduced-motionmedia query to.odh-u-spininApp.scss:
.odh-u-spin {
animation: pf-v6-c-spinner-animation-rotate 3s linear infinite;
}
`@media` (prefers-reduced-motion: reduce) {
.odh-u-spin {
animation: none;
}
}- Or replace
InProgressIconwith PatternFly's nativeSpinnercomponent (PF-first approach).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/concepts/modelServing/ModelStatusIcon.tsx` at line 53, The
InProgressIcon in ModelStatusIcon.tsx is being spun via the custom CSS class
odh-u-spin (defined in App.scss) but lacks prefers-reduced-motion handling and
resides outside the shared utilities; either update the odh-u-spin rule in
App.scss to include a prefers-reduced-motion: reduce media query that disables
the animation, or replace the InProgressIcon usage in ModelStatusIcon.tsx with
PatternFly's Spinner component so motion is handled by PF utilities and conforms
to accessibility guidelines; ensure the CSS change is made where odh-u-spin is
defined and that ModelStatusIcon.tsx (the place that references InProgressIcon)
is updated accordingly.
Comment on lines
62
to
69
| it('should check for run status when run status is undefined', () => { | ||
| const UNKNOWN_ICON = <QuestionCircleIcon />; | ||
| const UNKNOWN_STATUS = 'warning'; | ||
| const UNKNOWN_ICON = <OutlinedQuestionCircleIcon />; | ||
| const UNKNOWN_LABEL = '-'; | ||
| const runStatus = computeRunStatus(createRun('-')); | ||
| expect(runStatus.label).toBe(UNKNOWN_LABEL); | ||
| expect(runStatus.icon).toStrictEqual(UNKNOWN_ICON); | ||
| expect(runStatus.status).toBe(UNKNOWN_STATUS); | ||
| expect(runStatus.color).toBe('grey'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Cover the missing run === undefined branch.
Line 65 passes createRun('-'), so this is just another unknown-state test. It never exercises the if (!run) fallback in computeRunStatus, and Line 139 already covers the default-string branch.
Suggested change
- it('should check for run status when run status is undefined', () => {
+ it('should return the placeholder status when run is undefined', () => {
const UNKNOWN_ICON = <OutlinedQuestionCircleIcon />;
const UNKNOWN_LABEL = '-';
- const runStatus = computeRunStatus(createRun('-'));
+ const runStatus = computeRunStatus(undefined);
expect(runStatus.label).toBe(UNKNOWN_LABEL);
expect(runStatus.icon).toStrictEqual(UNKNOWN_ICON);
expect(runStatus.color).toBe('grey');
});As per coding guidelines, "Test behavior, not implementation details. Test names must describe the verified behavior."
📝 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.
Suggested change
| it('should check for run status when run status is undefined', () => { | |
| const UNKNOWN_ICON = <QuestionCircleIcon />; | |
| const UNKNOWN_STATUS = 'warning'; | |
| const UNKNOWN_ICON = <OutlinedQuestionCircleIcon />; | |
| const UNKNOWN_LABEL = '-'; | |
| const runStatus = computeRunStatus(createRun('-')); | |
| expect(runStatus.label).toBe(UNKNOWN_LABEL); | |
| expect(runStatus.icon).toStrictEqual(UNKNOWN_ICON); | |
| expect(runStatus.status).toBe(UNKNOWN_STATUS); | |
| expect(runStatus.color).toBe('grey'); | |
| }); | |
| it('should return the placeholder status when run is undefined', () => { | |
| const UNKNOWN_ICON = <OutlinedQuestionCircleIcon />; | |
| const UNKNOWN_LABEL = '-'; | |
| const runStatus = computeRunStatus(undefined); | |
| expect(runStatus.label).toBe(UNKNOWN_LABEL); | |
| expect(runStatus.icon).toStrictEqual(UNKNOWN_ICON); | |
| expect(runStatus.color).toBe('grey'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/concepts/pipelines/content/__tests__/utils.spec.tsx` around
lines 62 - 69, Add a test in
frontend/src/concepts/pipelines/content/__tests__/utils.spec.tsx that calls
computeRunStatus(undefined) (instead of createRun('-')) to exercise the run ===
undefined branch and assert the same expected behavior: label === '-'
(UNKNOWN_LABEL), icon strictly equals OutlinedQuestionCircleIcon (UNKNOWN_ICON)
and color === 'grey'; reference computeRunStatus and createRun in the test
name/description to make clear you are verifying the undefined-run fallback
behavior.
Comment on lines
+44
to
46
| if (s === RUN_STATE.INCOMPLETE) { | ||
| return { status: 'warning' }; | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent substring check for INCOMPLETE state.
All other state checks (RUNNING, PENDING, SKIPPED, FAILED, SUCCEEDED) include both exact match and .includes() fallback. INCOMPLETE only checks exact match, breaking the established pattern.
- if (s === RUN_STATE.INCOMPLETE) {
+ if (s === RUN_STATE.INCOMPLETE || s.includes(RUN_STATE.INCOMPLETE)) {
return { status: 'warning' };
}📝 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.
Suggested change
| if (s === RUN_STATE.INCOMPLETE) { | |
| return { status: 'warning' }; | |
| } | |
| if (s === RUN_STATE.INCOMPLETE || s.includes(RUN_STATE.INCOMPLETE)) { | |
| return { status: 'warning' }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx`
around lines 44 - 46, The check for INCOMPLETE is inconsistent: update the
conditional in the AutomlRunsTableRow logic that currently reads "if (s ===
RUN_STATE.INCOMPLETE)" to also include the substring fallback so it matches the
other cases (i.e., treat INCOMPLETE as true when s === RUN_STATE.INCOMPLETE ||
s.includes(RUN_STATE.INCOMPLETE)); adjust the return block for that branch
(status: 'warning') accordingly so behavior is consistent with
RUNNING/PENDING/SKIPPED/FAILED/SUCCEEDED checks.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/components/DocCardBadges.tsx (1)
48-48: Use semanticstatus="info"for consistency with other badges.Line 48 uses
color="blue"while Lines 53 and 58 use thestatusprop. PatternFly's Label component supportsstatus="info"withvariant="outline", eliminating the semantic drift.Proposed change
- <Label variant="outline" color="blue" icon={<InProgressIcon />}> + <Label variant="outline" status="info" icon={<InProgressIcon />}> In Progress </Label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DocCardBadges.tsx` at line 48, The Label using InProgressIcon should use the semantic status prop instead of a color string; replace color="blue" with status="info" on the Label component (the same pattern used in the other Label instances) so the variant="outline" + status="info" is used consistently; look for the Label element that renders the InProgressIcon and update its props accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/DocCardBadges.tsx`:
- Line 48: The Label using InProgressIcon should use the semantic status prop
instead of a color string; replace color="blue" with status="info" on the Label
component (the same pattern used in the other Label instances) so the
variant="outline" + status="info" is used consistently; look for the Label
element that renders the InProgressIcon and update its props accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: edf217bd-d924-4afe-9e28-f130c65234a4
📥 Commits
Reviewing files that changed from the base of the PR and between 269eb259ede88b85d8baff0fe07eca0100034495 and 7a6eb352faa5d8dc746168ca2f32b7bc9ccc7db8.
📒 Files selected for processing (6)
frontend/src/components/DocCardBadges.tsxfrontend/src/pages/projects/screens/detail/storage/ClusterStorageTableRow.tsxfrontend/src/pages/projects/screens/spawner/imageSelector/ImageVersionSelector.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsxpackages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx
- frontend/src/pages/projects/screens/spawner/imageSelector/ImageVersionSelector.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysTableRow.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx
Contributor
Author
|
There are a couple of changes I made that cannot be reflected on the screenshots, unfortunately. Here are the ones: REASON: Written but can't run — need separate dev servers: Gen AI Models (3 statuses) — needs Go BFF + webpack dev server |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsx (1)
51-53: Use semantic PF6 status prop for the registration state label.Line 51 uses
color="green"for a state indicator; this should usestatus="success"to align with PatternFly v6 semantic status-based styling and avoid legacy color coupling.Suggested change
- <Label variant="outline" color="green"> + <Label variant="outline" status="success"> Model registered </Label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsx` around lines 51 - 53, In PipelineDetailsTitle (frontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsx) replace the non-semantic color prop on the Label that reads "Model registered" — locate the Label component instance (Label variant="outline" currently using color="green") and change it to use the PatternFly v6 semantic prop status="success" (keeping variant="outline") so the component reads Label variant="outline" status="success" and adheres to semantic status-based styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsx`:
- Around line 51-53: In PipelineDetailsTitle
(frontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsx)
replace the non-semantic color prop on the Label that reads "Model registered" —
locate the Label component instance (Label variant="outline" currently using
color="green") and change it to use the PatternFly v6 semantic prop
status="success" (keeping variant="outline") so the component reads Label
variant="outline" status="success" and adheres to semantic status-based styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ccfb321e-5b60-4754-8abd-12e982f09087
📥 Commits
Reviewing files that changed from the base of the PR and between 7a6eb352faa5d8dc746168ca2f32b7bc9ccc7db8 and 99816cef93d1c32f79f003393ddabf8bc4db8a5e.
📒 Files selected for processing (4)
frontend/src/concepts/pipelines/content/PipelineRunTypeLabel.tsxfrontend/src/concepts/pipelines/content/pipelinesDetails/PipelineDetailsTitle.tsxfrontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsxfrontend/src/pages/pipelines/global/experiments/artifacts/ArtifactDetails/ArtifactDetailsTitle.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/concepts/pipelines/content/PipelineRunTypeLabel.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/pages/pipelines/global/experiments/artifacts/ArtifactDetails/ArtifactDetailsTitle.tsx
- frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRowExperiment.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
frontend/src/concepts/modelServing/ModelStatusIcon.tsx (1)
53-53:⚠️ Potential issue | 🟠 MajorRespect reduced-motion for the spinning status icon.
Line 53 and Line 84 still use the custom
odh-u-spinanimation class. Ensure that class disables animation underprefers-reduced-motion: reduce; otherwise motion-sensitive users will still get forced animation.As per coding guidelines, "PatternFly v6: use PF6 imports from
@patternfly/react-core, not custom wrappers. Avoid custom CSS — if you need to 'nudge' PF layout, check frontend/src/concepts/dashboard first."Also applies to: 84-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/concepts/modelServing/ModelStatusIcon.tsx` at line 53, ModelStatusIcon currently forces animation by applying the custom odh-u-spin class to InProgressIcon (occurrences in ModelStatusIcon.tsx), which ignores users' prefers-reduced-motion setting; fix by removing the custom spin class and instead use PatternFly v6's built-in Spinner (import from `@patternfly/react-core`) or another PF6 motion-aware component in place of InProgressIcon for the spinning state, and if you must keep odh-u-spin update the CSS so it respects prefers-reduced-motion: reduce (i.e., disable animation under that media query) so motion-sensitive users are not forced to see the spin.
🧹 Nitpick comments (1)
frontend/src/__mocks__/mockWorkloadK8sResource.ts (1)
127-129: Rename staleSucceedidentifier to matchCompletestatusLine 127 still uses
mockWorkloadEmptySucceedConditioneven though the keyed status isComplete. Keep naming aligned to avoid future misreads and bad symbol searches.Proposed diff
-const mockWorkloadEmptySucceedCondition: Record<WorkloadStatusType.Complete, WorkloadCondition[]> = +const mockWorkloadEmptyCompleteCondition: Record<WorkloadStatusType.Complete, WorkloadCondition[]> = { [WorkloadStatusType.Complete]: [ { @@ - ? mockWorkloadEmptySucceedCondition[WorkloadStatusType.Complete] + ? mockWorkloadEmptyCompleteCondition[WorkloadStatusType.Complete] : mockWorkloadStatusConditions[mockStatus] : [],Also applies to: 206-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/__mocks__/mockWorkloadK8sResource.ts` around lines 127 - 129, Rename the stale identifier mockWorkloadEmptySucceedCondition to a name that matches the keyed status (e.g., mockWorkloadEmptyCompleteCondition) so it aligns with WorkloadStatusType.Complete; update the declaration and all other occurrences (including the other instances noted around the second block) and any exports/imports or references to this symbol to use the new name to avoid mismatches during symbol search and future maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/concepts/modelServing/ModelStatusIcon.tsx`:
- Around line 80-85: The STANDBY case in the ModelStatusIcon mapping is missing
the tooltip `message` property, causing inconsistent tooltips; update the case
for ModelDeploymentState.STANDBY in ModelStatusIcon.tsx to include a `message`
string consistent with other starting-state branches (e.g., the same text used
for STARTING/INITIALIZING), so the returned object has label, color, icon, and
message fields like the other branches.
In `@frontend/src/concepts/pipelines/content/tables/renderUtils.tsx`:
- Around line 47-83: The hasNoLabel flag is being ignored when tooltipContent
exists, causing callers that expect icon-only rendering to get a full <Label>;
update the rendering logic in renderUtils (the branch around hasNoLabel,
tooltipContent and the content variable/Label creation) to prioritize
hasNoLabel: if hasNoLabel is true return the icon-only Tooltip variant (using
iconContent and runtimeStateLabels/createdAt) even when tooltipContent is
present, otherwise proceed to create and return the <Label> (and wrap it with
Tooltip only when appropriate). Ensure labelStatus/color/icon/isCompact logic is
preserved for the Label path.
- Line 59: The tooltip currently uses runtimeStateLabels[run.state], which can
yield undefined for unmapped states; replace that usage with the already
computed label from computeRunStatus(run) so the tooltip always shows a valid
string. Locate the JSX line rendering <StackItem>{`Status:
${runtimeStateLabels[run.state]}`}</StackItem> and change it to use the label
returned by computeRunStatus(run) (or the local variable named label if
present), ensuring you reference computeRunStatus and run (or label) instead of
runtimeStateLabels[run.state].
---
Duplicate comments:
In `@frontend/src/concepts/modelServing/ModelStatusIcon.tsx`:
- Line 53: ModelStatusIcon currently forces animation by applying the custom
odh-u-spin class to InProgressIcon (occurrences in ModelStatusIcon.tsx), which
ignores users' prefers-reduced-motion setting; fix by removing the custom spin
class and instead use PatternFly v6's built-in Spinner (import from
`@patternfly/react-core`) or another PF6 motion-aware component in place of
InProgressIcon for the spinning state, and if you must keep odh-u-spin update
the CSS so it respects prefers-reduced-motion: reduce (i.e., disable animation
under that media query) so motion-sensitive users are not forced to see the
spin.
---
Nitpick comments:
In `@frontend/src/__mocks__/mockWorkloadK8sResource.ts`:
- Around line 127-129: Rename the stale identifier
mockWorkloadEmptySucceedCondition to a name that matches the keyed status (e.g.,
mockWorkloadEmptyCompleteCondition) so it aligns with
WorkloadStatusType.Complete; update the declaration and all other occurrences
(including the other instances noted around the second block) and any
exports/imports or references to this symbol to use the new name to avoid
mismatches during symbol search and future maintenance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f855545a-cf9e-4a0f-a7e4-f71afc151e8f
📥 Commits
Reviewing files that changed from the base of the PR and between 99816cef93d1c32f79f003393ddabf8bc4db8a5e and f0377fe3fbb4a2d64fc91cc33c13c0cd28d8ea31.
📒 Files selected for processing (9)
frontend/src/__mocks__/mockWorkloadK8sResource.tsfrontend/src/concepts/kueue/__tests__/index.spec.tsfrontend/src/concepts/kueue/index.tsfrontend/src/concepts/kueue/types.tsfrontend/src/concepts/modelServing/ModelStatusIcon.tsxfrontend/src/concepts/notebooks/__tests__/NotebookStatusLabel.spec.tsxfrontend/src/concepts/pipelines/content/tables/renderUtils.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsxpackages/model-training/src/global/trainingJobList/components/TrainingJobStatus.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/model-training/src/global/trainingJobList/components/TrainingJobStatus.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/concepts/notebooks/tests/NotebookStatusLabel.spec.tsx
- frontend/src/concepts/kueue/tests/index.spec.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/mcp/MCPServerStatus.tsx
- frontend/src/concepts/kueue/index.ts
jenny-s51
reviewed
Mar 26, 2026
Contributor
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
Nice work here @rsun19, screenshots look good and updated labels match the design specs.
Left a comment for the pipelines topology icons which should be a quick fix.
Just a couple things:
-
For the issue CodeRabbit is flagging on the
odh-u-spinI think the suggested change makes sense for now. That class is used in several places though and would be well suited as a shared library style so we could add to https://github.com/opendatahub-io/mod-arch-library as a follow-up. WDYT? -
Nit - there are a few times where we use both the
colorandstatusprop on the labels. Just for clarity and readability maybe we can consolidate and try to use one wherever possible.
jenny-s51
reviewed
Mar 27, 2026
Contributor
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
Code changes look good @rsun19 - looks like just one conflict to fix.
Leaving the remaining CodeRabbit items to your discretion - the Cypress step descriptions for the Running/Ready wording might be an easy lift with the rebase.
rsun19
force-pushed
the
update-status-labels
branch
from
f232032 to
286d405
Compare
Contributor
Author
|
rebased |
Contributor
Author
|
@jenny-s51 @Misjohns follow up here in Kubeflow kubeflow/hub#2523 |
rsun19
force-pushed
the
update-status-labels
branch
from
286d405 to
55a2954
Compare
Standardize status terminology (Running→Ready, Started→Ready, Succeeded→Complete), replace legacy icons (PlayIcon→CheckCircleIcon, SyncAltIcon→InProgressIcon), and use PatternFly label status prop instead of color-only for semantic states. Made-with: Cursor
… and model-registry - Revert model registry settings label changes (Progressing/Available/Degrading) - EvaluationStatusLabel: fix stopping→Stopping, stopped→Stopped (distinct from canceled) - Pipeline runs: use semantic status prop, fix Canceling/Canceled/Pending colors/icons - PipelineDetailsTitle: pass labelStatus to Label - NodeStatusIcon: fix Canceling icon, remove Canceled warning status - Model training: remove redundant color props when status prop is set - Fix tests: StartNotebookModal, DeploymentsTableRow, EvaluationStatusLabel, MR settings Made-with: Cursor
Status labels that do not open a popover or modal when clicked are now styled as outlined labels to visually distinguish them from interactive ones. Made-with: Cursor
Remove redundant color props where status takes precedence (kueue, model-training), replace color="yellow" with status="warning" (StorageTableRow, ServingRuntimeVersionStatus, MCPServerStatus), add missing Skipped handling in automl, and expand computeRunStatus test coverage for Skipped, Paused, Canceling, and color assertions. Made-with: Cursor
- DocCardBadges: replace SyncAltIcon with InProgressIcon, use status prop for Complete/Failed labels - ImageVersionSelector: use status="success" and variant="outline" for Latest label - ClusterStorageTableRow: use status="success" and variant="outline" for Connected label - AIModelTableRow: refactor nested ternary to switch, add variant="outline" to Unknown label - MCPServerStatus: add variant="outline" to all non-interactive status labels - ApiKeysTableRow: use status prop instead of color for active/inactive Made-with: Cursor
- Separate STANDBY from LOADED in ModelStatusIcon (shows Starting instead of Ready) - Remove variant="outline" from labels wrapped in popovers/tooltips - Use filled variant for interactive labels, outline for non-interactive - Rename KueueWorkloadStatus.Succeeded to .Complete for consistency - Guard mockStatusEmptyWorkload with Complete status check in mock - Update test assertions to match Succeeded→Complete rename Made-with: Cursor
- Add missing `message` to STANDBY case in ModelStatusIcon so the Popover shows consistent tooltip text - Prioritize hasNoLabel in RunStatus so icon-only rendering is respected even when tooltipContent exists - Replace runtimeStateLabels[run.state] with the computed label from computeRunStatus to avoid undefined tooltip text - Rename mockWorkloadEmptySucceedCondition to mockWorkloadEmptyCompleteCondition to match WorkloadStatusType.Complete - Remove variant="outline" from Labels wrapped in Tooltip/Popover (OdhExploreCardTypeBadge, BYONImageHardwareProfiles, StorageTableRow) Made-with: Cursor
rsun19
force-pushed
the
update-status-labels
branch
from
55a2954 to
4e0e2e1
Compare
Contributor
Author
|
/retest |
Collaborator
|
/lgtm re-adding lgtm after rebase |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, jenny-s51 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 40e84df
into
opendatahub-io:main
180 of 185 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://redhat.atlassian.net/browse/RHOAIENG-38486
Description
Standardizes PatternFly
Labelusage for status indicators across the entire dashboard to align with RHOAI design guidelines and PatternFly v6 best practices.Changes
Semantic status props — Replaced
color="green"/color="red"/color="yellow"withstatus="success"/status="danger"/status="warning"for labels that represent health/outcome states (e.g., Running, Failed, Succeeded, Connected, Active/Inactive).Outlined variant for non-interactive labels — Added
variant="outline"to all status labels that are display-only (not clickable/popover-triggering), per PF v6 convention where filled labels imply interactivity.Icon standardization — Replaced outdated icons with consistent status icons:
SyncAltIcon→InProgressIconfor in-progress statesPlayIcon→InProgressIconfor running statesLabel text normalization — Aligned display text to use consistent terminology across components:
Files touched (59 files across 6 commits)
NotebookStatusLabel.tsxPipelineRunTypeLabel.tsx,PipelineDetailsTitle.tsx,renderUtils.tsx,utils.tsx,ExecutionStatus.tsx,ExperimentPipelineRuns.tsx,ArtifactDetailsTitle.tsx,ArtifactsTableRow.tsxWorkloadStatusLabel.tsx,utils.tsxLMEvalStatusLabel.tsxModelStatusIcon.tsx,NodeStatusIcon.tsxEvaluationStatusLabel.tsxAutomlRunsTableRow.tsx,AutoragRunsTableRow.tsxAIModelTableRow.tsx,MCPServerStatus.tsxApiKeysTableRow.tsxtrainingJobList/utils.tsClusterStorageTableRow.tsx,StorageTableRow.tsx,ImageVersionSelector.tsxDocCardBadges.tsxHow Has This Been Tested?
lint-stagedpass on all commits (pre-commit hook ran successfully)Test Impact
AutomlRunsTableRow,EvaluationStatusLabel,NotebookStatusLabel,utils.spec.tsx(pipelines),distributedWorkloads/utils.spec.ts, andtrainingJobList/utils.spec.tsRequest 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
UI Improvements
Tests