feat: implement RayJob drawer Details tab (RHOAIENG-49276)#6693
feat: implement RayJob drawer Details tab (RHOAIENG-49276)#6693dpanshug wants to merge 2 commits intoopendatahub-io:mainfrom
Conversation
|
[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 |
📝 WalkthroughWalkthroughThis PR introduces UI and API support for Ray job details display. It adds a new RayJobDetailsTab component rendering job summary (with Ray version), executions (entrypoint and submission mode), and management sections (shutdown policy and cluster name). Supporting infrastructure includes a getRayCluster API function, a useRayClusterSpec hook that fetches or uses inline cluster specs, a mockRayClusterK8sResource factory, and updates to mockRayJobK8sResource to support rayVersion, shutdownAfterJobFinishes, and clusterSelector fields. Test coverage validates hook behavior and component rendering with mocked data. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security & Implementation Review PointsClipboard API handling: The copy-to-clipboard implementation in RayJobDetailsTab requires verification. Ensure it uses the modern Clipboard API with proper error handling rather than deprecated Async error boundaries: The useRayClusterSpec hook's NotReadyError handling and useFetch integration need validation. Confirm error propagation doesn't silently fail—callers must handle the error field. If this feeds into UI rendering, unhandled promise rejections could trigger Unhandled Promise Rejection (related to CWE-391). clusterSelector propagation: The mockRayJobK8sResource now conditionally includes clusterSelector vs. rayClusterSpec. Verify the actual component and API correctly handle both paths—mixing inline specs with cluster selectors could cause undefined behavior if both are present or neither is present. Fetch error state: The hook returns Test data assumptions: Mock tests increase row counts (4→6, 5→7) based on new RayJob entries. Verify filters/sorting logic still functions correctly with the expanded dataset—filtering bugs can emerge at boundaries. 🚥 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 |
dpanshug
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/__mocks__/mockRayJobK8sResource.ts`:
- Line 27: The mock factory in mockRayJobK8sResource currently defaults
status.rayClusterName to `${name}-raycluster` even when clusterSelector is
provided; update the factory that builds the mock (mockRayJobK8sResource / the
workspace-cluster mock block) so that if clusterSelector is present and no
explicit rayClusterName is provided, you derive status.rayClusterName from
clusterSelector (for example use the selected cluster name from clusterSelector,
e.g. clusterSelector["ray.ibm.com/cluster"] or equivalent key used in your code)
instead of `${name}-raycluster`; ensure you apply this change wherever
rayClusterName is set (references to status.rayClusterName, rayClusterName,
clusterSelector) including the other instances noted (lines ~53 and ~119-126).
In `@packages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsx`:
- Around line 35-40: The handleCopy callback should await
navigator.clipboard.writeText and handle rejections: update handleCopy to be
async, wrap the await navigator.clipboard.writeText(job.spec.entrypoint) in a
try/catch, call setCopied(true) only after a successful await, and handle errors
in the catch (e.g., setCopied(false) and log or surface the error) to avoid
unhandled promise rejections; references: handleCopy, job.spec.entrypoint, and
setCopied.
In `@packages/model-training/src/hooks/useRayVersion.ts`:
- Around line 12-18: The hook fails to reset derived state when the job changes,
causing stale fetchedVersion or a stuck loaded; update the relevant
React.useEffect blocks in useRayVersion to depend on the job (or a stable job
identifier) and reset state before starting a new fetch by calling
setFetchedVersion(undefined) and setLoaded(!!inlineVersion ||
!workspaceClusterName) (and also ensure pending fetches are cancelled/ignored in
the fetchRayVersion flow); apply the same reset logic to the other affected
effects (the ones around lines showing initialization and the fetch completion
handlers) so fetchedVersion and loaded are reinitialized whenever the
job/workspaceClusterName/inlineVersion context changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: facb4627-e93d-4165-899d-589dd1812148
📒 Files selected for processing (7)
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/global/rayJobDetailsDrawer/RayJobDetailsDrawer.tsxpackages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsxpackages/model-training/src/hooks/useRayVersion.ts
packages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api/rayClusters.ts`:
- Around line 8-12: getRayCluster currently returns the k8sGetResource result
typed as RayClusterKind but lacks runtime validation, so malformed payloads can
reach useRayClusterSpec and the drawer; add a runtime guard in getRayCluster
that checks required fields (e.g., spec, metadata,
spec.headGroupSpec/workerGroupSpecs as applicable) on the fetched object and
reject (throw or return a rejected Promise) if missing or invalid, using the
RayClusterKind/RayClusterModel types to guide which properties to validate and
ensure callers only receive validated RayClusterKind instances.
In `@packages/model-training/src/hooks/__tests__/useRayClusterSpec.spec.ts`:
- Around line 111-124: The test is hitting the inline-spec path because
mockRayJobK8sResource injects spec.rayClusterSpec by default; update the test to
construct a job that truly lacks rayClusterSpec (so the "no inline spec" branch
is exercised). Modify the test to call mockRayJobK8sResource with an override
that removes or sets rayClusterSpec to undefined (or use a dedicated factory
helper) so the job returned has no spec.rayClusterSpec, then keep the existing
assertions against useRayClusterSpec (and mocked useFetch via useFetchMock) to
verify loaded === true and clusterSpec is defined from the no-selector path.
In `@packages/model-training/src/hooks/useRayClusterSpec.ts`:
- Around line 17-35: The hook returns a stale error when it short-circuits;
update useRayClusterSpec so that the returned error is cleared whenever the hook
is not actively fetching (i.e., when inlineSpec is present or
workspaceClusterName is falsy). Concretely, keep using the useFetch result
(fetchedCluster, fetchLoaded, error) but change the returned error to be
masked/reset when short-circuited (reference inlineSpec, workspaceClusterName,
fetchedCluster, fetchLoaded, and error) so the hook returns undefined (or null)
for error unless the fetch path is actually active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9caf0f3a-4a0c-4cfd-958c-f7f296090d78
📒 Files selected for processing (6)
packages/model-training/src/__mocks__/mockRayJobK8sResource.tspackages/model-training/src/api/__tests__/rayClusters.spec.tspackages/model-training/src/api/rayClusters.tspackages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsxpackages/model-training/src/hooks/__tests__/useRayClusterSpec.spec.tspackages/model-training/src/hooks/useRayClusterSpec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/model-training/src/api/tests/rayClusters.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/model-training/src/global/rayJobDetailsDrawer/RayJobDetailsTab.tsx
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6693 +/- ##
==========================================
- Coverage 64.02% 64.00% -0.03%
==========================================
Files 2499 2504 +5
Lines 75200 75535 +335
Branches 18956 19028 +72
==========================================
+ Hits 48149 48346 +197
- Misses 27051 27189 +138
... and 22 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
RHOAIENG-49276
Description
How Has This Been Tested?
Check the ray jobs drawer.
Test Impact
Added cypress test
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
New Features
Tests