Open
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
📝 WalkthroughWalkthroughThis pull request extends Ray job visualization capabilities by adding structured UI components and supporting infrastructure. It introduces two new Cypress page objects (RayJobDetailsTab, RayJobResourcesTab) with finder methods; creates new React components for displaying Ray job details and resource specifications; adds a new hook (useRayClusterSpec) for resolving cluster specs from inline or external sources; enhances mock factories to support worker group specs and cluster selectors; adds a new API function (getRayCluster) for fetching individual Ray clusters; and expands the test suite with comprehensive UI validation tests. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Actionable Issues1. useRayClusterSpec: Potential null dereference in clusterSpec resolution
2. Clipboard copy state management lacks error handling (CWE-755)
3. Unsafe test data index assumptions
4. Missing null guard in getAllConsumedResources usage
5. Mock factory lacks input validation
6. Type safety gap in RayJobDetailsDrawerProps
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/model-training/src/__mocks__/mockRayJobK8sResource.ts (1)
15-28:⚠️ Potential issue | 🟡 MinorThis fixture cannot model a RayJob with no cluster reference.
When
clusterSelectoris absent, the helper always manufactures an inlinerayClusterSpec.RayJobKindallows both fields to be missing, so tests cannot exercise that branch and will get false confidence arounduseRayClusterSpec. Add an explicit opt-out such asrayClusterSpec: nullorincludeInlineSpec: false.One way to make the state representable
-import { RayJobKind, RayWorkerGroupSpec } from '@odh-dashboard/model-training/k8sTypes'; +import { + RayJobKind, + RayClusterSpec, + RayWorkerGroupSpec, +} from '@odh-dashboard/model-training/k8sTypes'; @@ rayVersion?: string; shutdownAfterJobFinishes?: boolean; + rayClusterSpec?: RayClusterSpec | null; @@ shutdownAfterJobFinishes = true, + rayClusterSpec, @@ ...(clusterSelector ? { clusterSelector } + : rayClusterSpec === null + ? {} : { - rayClusterSpec: { + rayClusterSpec: rayClusterSpec ?? { rayVersion, headGroupSpec: { template: { @@ - }, + }, }),Also applies to: 121-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/model-training/src/__mocks__/mockRayJobK8sResource.ts` around lines 15 - 28, The mock currently always produces an inline rayClusterSpec when clusterSelector is missing, preventing tests from representing a RayJob with no cluster reference; update the fixture generator to accept an explicit opt-out (e.g., a new boolean flag includeInlineSpec or allow rayClusterSpec: null) and when that flag is set (or rayClusterSpec is null) leave both clusterSelector and rayClusterSpec undefined so callers can test the branch where RayJobKind has neither field; update the mock creation logic that references clusterSelector/rayClusterSpec and add tests/examples showing useRayClusterSpec behavior remains testable.
🧹 Nitpick comments (2)
packages/model-training/src/global/rayJobDetailsDrawer/RayJobResourcesTab.tsx (2)
169-203: DescriptionListGroup without DescriptionListTerm.Line 169 opens a
DescriptionListGroupthat contains only aDescriptionListDescription(line 171). PatternFly DescriptionList semantics expect term-description pairs. This may cause accessibility/screen reader issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/model-training/src/global/rayJobDetailsDrawer/RayJobResourcesTab.tsx` around lines 169 - 203, The DescriptionListGroup currently renders only a DescriptionListDescription when clusterQueueDataLoaded is true, which breaks PatternFly term/description semantics; update the JSX inside the top-level DescriptionListGroup so it always includes a paired DescriptionListTerm and DescriptionListDescription (e.g., add a term like "Consumed quota" or a visually-hidden DescriptionListTerm) and move the consumedResources mapping into the DescriptionListDescription block; ensure DescriptionListTerm / DescriptionListDescription pairs are present for both the consumedResources non-empty branch and the '-' fallback, referencing the existing DescriptionListGroup, DescriptionListTerm, DescriptionListDescription and consumedResources map to locate where to add the term.
75-82: Unusederrorvariable from hook destructuring.
errorfromuseClusterQueueFromLocalQueueis destructured but only used on line 163 for quota source display. If an error occurs during local queue fetch, the UI still shows a skeleton indefinitely becauseclusterQueueLoadedmay never become true with a proper error state. Consider displaying an error state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/model-training/src/global/rayJobDetailsDrawer/RayJobResourcesTab.tsx` around lines 75 - 82, The hook useClusterQueueFromLocalQueue currently destructures an error that isn't used to update the UI; in RayJobResourcesTab update the rendering logic to handle that error: when the hook returns an error (error from useClusterQueueFromLocalQueue) show an explicit error state/message (or fallback UI) for the cluster-queue/quota source section instead of leaving the skeleton, or treat the error as a loaded state so the component can render the quota source error UI; modify the conditional checks that rely on clusterQueueLoaded to also consider error (e.g., if (clusterQueueLoaded || error) render quota source/error UI) and surface the error text where quota source is displayed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/model-training/src/global/ModelTraining.tsx`:
- Around line 140-146: The code currently maps a missing/unknown workspace
RayCluster to 0 via getUnifiedJobNodeCount()/nodeCountMap and passes
selectedJobNodeCount into RayJobDetailsDrawer, which causes a false zero to
display while the cluster watch is loading or failed; change the logic so that
node count is nullable/undefined when the cluster is unresolved instead of
defaulting to 0 (i.e., let selectedJobNodeCount be null/undefined when
nodeCountMap lacks selectedJobId), or obtain the node count from the same
resolved cluster spec used by the drawer (use selectedRayJob or its
resolvedClusterSpec to derive a definitive count) and only pass a numeric
nodeCount when the cluster is actually resolved. Ensure references:
getUnifiedJobNodeCount, nodeCountMap, selectedJobNodeCount, selectedRayJob,
RayJobDetailsDrawer, selectedJobDisplayName.
In `@packages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsx`:
- Around line 36-41: The handleCopy callback currently calls
navigator.clipboard.writeText without awaiting or handling rejection, causing
setCopied(true) to be set even if the copy fails; update handleCopy (the
function tied to job.spec.entrypoint and setCopied) to be async, await
navigator.clipboard.writeText(job.spec.entrypoint) inside a try/catch, call
setCopied(true) only on successful await, and in the catch block
setCopied(false) (or leave unchanged) and log or surface the error (e.g.,
console.error or existing logger); also guard for navigator.clipboard
availability and fallback or handle that error path accordingly.
In
`@packages/model-training/src/global/rayJobDetailsDrawer/RayJobResourcesTab.tsx`:
- Around line 98-100: The rendering of node count in RayJobResourcesTab uses a
falsy check (nodeCount || '-') which treats 0 as missing; update the render
logic in the RayJobResourcesTab component (the DescriptionListDescription with
data-testid="nodes-value") to use the nullish coalescing operator (nodeCount ??
'-') so that 0 is displayed while still falling back to '-' only for
null/undefined.
In `@packages/model-training/src/hooks/useRayClusterSpec.ts`:
- Around line 21-35: The fetched RayCluster payload (fetchedCluster from
useFetch/getRayCluster returning RayClusterKind) must be runtime-validated
before exposing clusterSpec: add a runtime shape check that ensures
fetchedCluster is an object with a non-null spec and required nested fields you
read downstream (e.g., spec.headGroupSpec, spec.workerGroupSpecs or whatever
your tabs expect) and if the check fails set/return an error instead of the
spec; update the return values (clusterSpec, loaded, error) so clusterSpec only
uses inlineSpec ?? fetchedCluster.spec when the runtime check passes and
otherwise set error and avoid exposing malformed data (adjust the useFetch
callback/post-process where fetchedCluster is consumed and reference
getRayCluster, fetchedCluster, RayClusterKind, clusterSpec, loaded, and error).
---
Outside diff comments:
In `@packages/model-training/src/__mocks__/mockRayJobK8sResource.ts`:
- Around line 15-28: The mock currently always produces an inline rayClusterSpec
when clusterSelector is missing, preventing tests from representing a RayJob
with no cluster reference; update the fixture generator to accept an explicit
opt-out (e.g., a new boolean flag includeInlineSpec or allow rayClusterSpec:
null) and when that flag is set (or rayClusterSpec is null) leave both
clusterSelector and rayClusterSpec undefined so callers can test the branch
where RayJobKind has neither field; update the mock creation logic that
references clusterSelector/rayClusterSpec and add tests/examples showing
useRayClusterSpec behavior remains testable.
---
Nitpick comments:
In
`@packages/model-training/src/global/rayJobDetailsDrawer/RayJobResourcesTab.tsx`:
- Around line 169-203: The DescriptionListGroup currently renders only a
DescriptionListDescription when clusterQueueDataLoaded is true, which breaks
PatternFly term/description semantics; update the JSX inside the top-level
DescriptionListGroup so it always includes a paired DescriptionListTerm and
DescriptionListDescription (e.g., add a term like "Consumed quota" or a
visually-hidden DescriptionListTerm) and move the consumedResources mapping into
the DescriptionListDescription block; ensure DescriptionListTerm /
DescriptionListDescription pairs are present for both the consumedResources
non-empty branch and the '-' fallback, referencing the existing
DescriptionListGroup, DescriptionListTerm, DescriptionListDescription and
consumedResources map to locate where to add the term.
- Around line 75-82: The hook useClusterQueueFromLocalQueue currently
destructures an error that isn't used to update the UI; in RayJobResourcesTab
update the rendering logic to handle that error: when the hook returns an error
(error from useClusterQueueFromLocalQueue) show an explicit error state/message
(or fallback UI) for the cluster-queue/quota source section instead of leaving
the skeleton, or treat the error as a loaded state so the component can render
the quota source error UI; modify the conditional checks that rely on
clusterQueueLoaded to also consider error (e.g., if (clusterQueueLoaded ||
error) render quota source/error UI) and surface the error text where quota
source is displayed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4b33bd9f-8e68-4fd3-81a2-3f569bd81764
📒 Files selected for processing (12)
packages/cypress/cypress/pages/modelTraining.tspackages/cypress/cypress/tests/mocked/modelTraining/modelTrainingRayJobs.cy.tspackages/model-training/src/__mocks__/mockRayClusterK8sResource.tspackages/model-training/src/__mocks__/mockRayJobK8sResource.tspackages/model-training/src/api/__tests__/rayClusters.spec.tspackages/model-training/src/api/rayClusters.tspackages/model-training/src/global/ModelTraining.tsxpackages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsDrawer.tsxpackages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsxpackages/model-training/src/global/rayJobDetailsDrawer/RayJobResourcesTab.tsxpackages/model-training/src/hooks/__tests__/useRayClusterSpec.spec.tspackages/model-training/src/hooks/useRayClusterSpec.ts
Comment on lines
+140
to
+146
| const selectedJobNodeCount = selectedJobId ? nodeCountMap.get(selectedJobId) ?? 0 : 0; | ||
|
|
||
| const panelContent = selectedRayJob ? ( | ||
| <RayJobDetailsDrawer | ||
| job={selectedRayJob} | ||
| displayName={selectedJobDisplayName} | ||
| nodeCount={selectedJobNodeCount} |
Contributor
There was a problem hiding this comment.
Stop encoding “workspace RayCluster not resolved” as 0 nodes.
getUnifiedJobNodeCount() falls back to 0 when the referenced cluster is missing from the watched list. Passing that straight through here makes the new Resources tab show an actual node count of zero while the cluster watch is still loading, stale, or failed. Keep this nullable/loading, or derive it from the same resolved cluster spec the drawer already uses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/model-training/src/global/ModelTraining.tsx` around lines 140 - 146,
The code currently maps a missing/unknown workspace RayCluster to 0 via
getUnifiedJobNodeCount()/nodeCountMap and passes selectedJobNodeCount into
RayJobDetailsDrawer, which causes a false zero to display while the cluster
watch is loading or failed; change the logic so that node count is
nullable/undefined when the cluster is unresolved instead of defaulting to 0
(i.e., let selectedJobNodeCount be null/undefined when nodeCountMap lacks
selectedJobId), or obtain the node count from the same resolved cluster spec
used by the drawer (use selectedRayJob or its resolvedClusterSpec to derive a
definitive count) and only pass a numeric nodeCount when the cluster is actually
resolved. Ensure references: getUnifiedJobNodeCount, nodeCountMap,
selectedJobNodeCount, selectedRayJob, RayJobDetailsDrawer,
selectedJobDisplayName.
packages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsx
Show resolved
Hide resolved
Comment on lines
+98
to
+100
| <DescriptionListDescription data-testid="nodes-value"> | ||
| {nodeCount || '-'} | ||
| </DescriptionListDescription> |
Contributor
There was a problem hiding this comment.
Falsy check on nodeCount treats 0 as missing.
nodeCount || '-' displays '-' when nodeCount is 0. If 0 nodes is a valid state that should be displayed, use nullish coalescing instead.
Proposed fix
<DescriptionListDescription data-testid="nodes-value">
- {nodeCount || '-'}
+ {nodeCount ?? '-'}
</DescriptionListDescription>📝 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
| <DescriptionListDescription data-testid="nodes-value"> | |
| {nodeCount || '-'} | |
| </DescriptionListDescription> | |
| <DescriptionListDescription data-testid="nodes-value"> | |
| {nodeCount ?? '-'} | |
| </DescriptionListDescription> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/model-training/src/global/rayJobDetailsDrawer/RayJobResourcesTab.tsx`
around lines 98 - 100, The rendering of node count in RayJobResourcesTab uses a
falsy check (nodeCount || '-') which treats 0 as missing; update the render
logic in the RayJobResourcesTab component (the DescriptionListDescription with
data-testid="nodes-value") to use the nullish coalescing operator (nodeCount ??
'-') so that 0 is displayed while still falling back to '-' only for
null/undefined.
Comment on lines
+21
to
+35
| } = useFetch<RayClusterKind | null>( | ||
| React.useCallback(() => { | ||
| if (inlineSpec || !workspaceClusterName) { | ||
| return Promise.reject(new NotReadyError('Inline spec available or no workspace cluster')); | ||
| } | ||
| return getRayCluster(workspaceClusterName, job.metadata.namespace); | ||
| }, [inlineSpec, workspaceClusterName, job.metadata.namespace]), | ||
| null, | ||
| { initialPromisePurity: true }, | ||
| ); | ||
|
|
||
| return { | ||
| clusterSpec: inlineSpec ?? fetchedCluster?.spec, | ||
| loaded: !!inlineSpec || !workspaceClusterName || fetchLoaded, | ||
| error, |
Contributor
There was a problem hiding this comment.
Validate fetched RayCluster payloads before returning clusterSpec.
k8sGetResource() is only type-safe at compile time. If this comes back without a valid spec, the new drawer tabs will receive malformed data and can fail when they read nested resource fields. Gate the response with a runtime shape check and surface an error instead of rendering it. As per coding guidelines, "Validate all API responses before rendering".
Minimal guard
+const isRayClusterSpec = (spec: unknown): spec is RayClusterSpec =>
+ !!spec && typeof spec === 'object' && 'headGroupSpec' in spec;
+
export const useRayClusterSpec = (
job: RayJobKind,
): { clusterSpec: RayClusterSpec | undefined; loaded: boolean; error: Error | undefined } => {
@@
+ const fetchedSpec =
+ fetchedCluster && isRayClusterSpec(fetchedCluster.spec) ? fetchedCluster.spec : undefined;
+
return {
- clusterSpec: inlineSpec ?? fetchedCluster?.spec,
+ clusterSpec: inlineSpec ?? fetchedSpec,
loaded: !!inlineSpec || !workspaceClusterName || fetchLoaded,
- error,
+ error:
+ fetchedCluster && !isRayClusterSpec(fetchedCluster.spec)
+ ? new Error('Invalid RayCluster response')
+ : error,
};
};📝 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
| } = useFetch<RayClusterKind | null>( | |
| React.useCallback(() => { | |
| if (inlineSpec || !workspaceClusterName) { | |
| return Promise.reject(new NotReadyError('Inline spec available or no workspace cluster')); | |
| } | |
| return getRayCluster(workspaceClusterName, job.metadata.namespace); | |
| }, [inlineSpec, workspaceClusterName, job.metadata.namespace]), | |
| null, | |
| { initialPromisePurity: true }, | |
| ); | |
| return { | |
| clusterSpec: inlineSpec ?? fetchedCluster?.spec, | |
| loaded: !!inlineSpec || !workspaceClusterName || fetchLoaded, | |
| error, | |
| const isRayClusterSpec = (spec: unknown): spec is RayClusterSpec => | |
| !!spec && typeof spec === 'object' && 'headGroupSpec' in spec; | |
| export const useRayClusterSpec = ( | |
| job: RayJobKind, | |
| ): { clusterSpec: RayClusterSpec | undefined; loaded: boolean; error: Error | undefined } => { | |
| } = useFetch<RayClusterKind | null>( | |
| React.useCallback(() => { | |
| if (inlineSpec || !workspaceClusterName) { | |
| return Promise.reject(new NotReadyError('Inline spec available or no workspace cluster')); | |
| } | |
| return getRayCluster(workspaceClusterName, job.metadata.namespace); | |
| }, [inlineSpec, workspaceClusterName, job.metadata.namespace]), | |
| null, | |
| { initialPromisePurity: true }, | |
| ); | |
| const fetchedSpec = | |
| fetchedCluster && isRayClusterSpec(fetchedCluster.spec) ? fetchedCluster.spec : undefined; | |
| return { | |
| clusterSpec: inlineSpec ?? fetchedSpec, | |
| loaded: !!inlineSpec || !workspaceClusterName || fetchLoaded, | |
| error: | |
| fetchedCluster && !isRayClusterSpec(fetchedCluster.spec) | |
| ? new Error('Invalid RayCluster response') | |
| : error, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/model-training/src/hooks/useRayClusterSpec.ts` around lines 21 - 35,
The fetched RayCluster payload (fetchedCluster from useFetch/getRayCluster
returning RayClusterKind) must be runtime-validated before exposing clusterSpec:
add a runtime shape check that ensures fetchedCluster is an object with a
non-null spec and required nested fields you read downstream (e.g.,
spec.headGroupSpec, spec.workerGroupSpecs or whatever your tabs expect) and if
the check fails set/return an error instead of the spec; update the return
values (clusterSpec, loaded, error) so clusterSpec only uses inlineSpec ??
fetchedCluster.spec when the runtime check passes and otherwise set error and
avoid exposing malformed data (adjust the useFetch callback/post-process where
fetchedCluster is consumed and reference getRayCluster, fetchedCluster,
RayClusterKind, clusterSpec, loaded, and error).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6694 +/- ##
==========================================
+ Coverage 64.02% 64.14% +0.11%
==========================================
Files 2499 2505 +6
Lines 75200 75735 +535
Branches 18956 19055 +99
==========================================
+ Hits 48149 48579 +430
- Misses 27051 27156 +105
... and 30 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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
1 participant
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.
RHOAIENG-49277
Description
How Has This Been Tested?
Click on ray job and check the resource tab
Test Impact
Added cypress tests
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main@tobiastal
Summary by CodeRabbit
Release Notes