Refactor RegisterCatlogModel and Remove useEffect #4420
Conversation
WalkthroughThe changes refactor the catalog model registration feature by replacing the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RegisterCatalogModelPage
participant RegisterCatalogModelForm
participant ModelRegistryService
participant Router
User->>RegisterCatalogModelPage: Navigates to registration page
RegisterCatalogModelPage->>RegisterCatalogModelForm: Passes model, registry, and data
User->>RegisterCatalogModelForm: Fills out form fields
RegisterCatalogModelForm->>ModelRegistryService: Submits registration
ModelRegistryService-->>RegisterCatalogModelForm: Returns success or error
RegisterCatalogModelForm->>Router: Navigates to model detail or shows error
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/pages/modelCatalog/screens/RegisterCatalogModelPage.tsx (1)
41-46: Consider enhancing the loading state UI.While the loading logic is correct, the generic "Loading..." text could be more informative.
- <div>Loading...</div> + <div>Loading model registration form...</div>Also applies to: 84-85
frontend/src/pages/modelCatalog/screens/RegisterCatalogModelForm.tsx (1)
119-123: Use optional chaining for cleaner code.The current approach with
&&can be simplified using optional chaining.// eslint-disable-next-line camelcase string_value: - model.artifacts?.map((artifact) => artifact.tags && artifact.tags[0])[0] ?? '', + model.artifacts?.map((artifact) => artifact.tags?.[0])[0] ?? '', metadataType: ModelRegistryMetadataType.STRING,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/registerCatalogModel.cy.ts(1 hunks)frontend/src/pages/modelCatalog/ModelCatalogRoutes.tsx(2 hunks)frontend/src/pages/modelCatalog/screens/RegisterCatalogModel.tsx(0 hunks)frontend/src/pages/modelCatalog/screens/RegisterCatalogModelForm.tsx(1 hunks)frontend/src/pages/modelCatalog/screens/RegisterCatalogModelPage.tsx(1 hunks)frontend/src/pages/modelRegistry/screens/RegisterModel/useRegisterModelData.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/pages/modelCatalog/screens/RegisterCatalogModel.tsx
🧰 Additional context used
🧠 Learnings (1)
frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/registerCatalogModel.cy.ts (1)
Learnt from: christianvogt
PR: opendatahub-io/odh-dashboard#4381
File: frontend/src/__tests__/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the `frontend/src/__tests__/cypress/tsconfig.json` file intentionally has an empty `files` array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
🧬 Code Graph Analysis (2)
frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/registerCatalogModel.cy.ts (2)
frontend/src/__tests__/cypress/cypress/pages/modelCatalog/registerCatalogModel.ts (1)
registerCatalogModelPage(23-23)frontend/src/__tests__/cypress/cypress/pages/modelRegistry/registerModelPage.ts (1)
registerModelPage(89-89)
frontend/src/pages/modelCatalog/screens/RegisterCatalogModelPage.tsx (6)
frontend/src/pages/modelCatalog/types.ts (1)
CatalogModelDetailsParams(1-6)frontend/src/pages/modelCatalog/utils.ts (2)
decodeParams(42-47)findModelFromModelCatalogSources(11-32)frontend/src/concepts/modelRegistry/context/ModelRegistriesContext.tsx (1)
ModelRegistriesContext(20-27)frontend/src/concepts/modelCatalog/context/ModelCatalogContext.tsx (1)
ModelCatalogContext(16-18)frontend/src/concepts/modelCatalog/types.ts (1)
CatalogModel(18-37)frontend/src/routes/modelCatalog/catalogModelDetails.ts (1)
getCatalogModelDetailsRoute(7-13)
🪛 Biome (1.9.4)
frontend/src/pages/modelCatalog/screens/RegisterCatalogModelForm.tsx
[error] 121-121: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
frontend/src/pages/modelCatalog/ModelCatalogRoutes.tsx (1)
8-8: LGTM! Clean refactoring of the registration component.The import and usage have been correctly updated to use the new
RegisterCatalogModelPagecomponent, aligning with the architectural improvement of separating page logic from form UI.Also applies to: 24-24
frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/registerCatalogModel.cy.ts (1)
342-377: Excellent test coverage for the form input stability fix!This test directly verifies that the user input issue mentioned in the PR objectives has been resolved. The 30-second timeout assertions ensure that form values remain stable and don't revert to prefilled defaults.
frontend/src/pages/modelRegistry/screens/RegisterModel/useRegisterModelData.ts (1)
66-69: Good modularization - exporting defaults for reuse.Making
registerModelFormDataDefaultsForModelCatalogexportable enables its use in the newRegisterCatalogModelFormcomponent, promoting code reuse and consistency.frontend/src/pages/modelCatalog/screens/RegisterCatalogModelPage.tsx (1)
15-90: Clean separation of page logic from form UI - well structured!The component properly handles data fetching, loading states, and breadcrumb navigation. The conditional rendering based on
isDataReadyensures the form only renders when all required data is available.frontend/src/pages/modelCatalog/screens/RegisterCatalogModelForm.tsx (2)
44-69: Excellent solution to prevent form value resets!The
useRegisterCatalogModelDataWithDefaultshook with theisInitializedref is a clean solution to the core issue. This ensures form values are initialized only once, preventing the problematic resets that users were experiencing.
71-238: Well-structured form component with comprehensive error handling.The form properly handles:
- Model registry selection
- Form validation including duplicate name checking
- Submission with proper error handling and state management
- Analytics tracking for user interactions
- Navigation on success/cancel
The separation from the page component improves maintainability and testability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4420 +/- ##
=======================================
Coverage ? 82.62%
=======================================
Files ? 1752
Lines ? 36570
Branches ? 10804
=======================================
Hits ? 30216
Misses ? 6354
Partials ? 0
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
For: https://issues.redhat.com/browse/RHOAIENG-21678
Description
The original RegisterCatalogModel.tsx component was split into two separate components: RegisterCatalogModelPage.tsx (handling data fetching, loading states, and routing) and RegisterCatalogModelForm.tsx (rendering the form UI and handling form logic). The previous useEffect workaround for prefilling form data was replaced with using a custom hook (useRegisterCatalogModelDataWithDefaults). This is a followup tech debt work for #3921
How Has This Been Tested?
Test Impact
Cypress test was added to verify that user input in form fields remains stable and doesn't reset to prefilled values over time.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit