Add name field validation with maxLength to registry creation#6869
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces the PatternFly modal composition in CreateModal with a single ContentModal and wires submit/cancel via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security & Quality Issues
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6869 +/- ##
==========================================
- Coverage 64.81% 64.80% -0.02%
==========================================
Files 2441 2441
Lines 75996 75997 +1
Branches 19158 19156 -2
==========================================
- Hits 49257 49249 -8
- Misses 26739 26748 +9
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Philip-Carneiro
force-pushed
the
fix/registry-name-field-validation
branch
from
72d8b88 to
36b07cb
Compare
|
SS of the change: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts (2)
370-383: Test validates length constraint but not the transformed name value.The test types 40 'a' characters as display name, but the K8s name will be derived via
translateDisplayNameForK8s. The assertion only checkslength.at.most(40)but doesn't verify the actual transformed name matches expectations. Consider also asserting the expected name value to catch potential translation logic issues.♻️ Strengthen assertion
cy.wait('@createModelRegistry').then((interception) => { - expect(interception.request.body.modelRegistry.metadata.name).to.have.length.at.most(40); + const name = interception.request.body.modelRegistry.metadata.name; + expect(name).to.have.length.at.most(40); + expect(name).to.equal('a'.repeat(40)); // or the expected translated value });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts` around lines 370 - 383, The test only checks the k8s name length after submitting a 40-char display name but doesn't assert the actual transformed value; update the spec in modelRegistrySettings.cy.ts to compute the expected K8s-safe name using the same logic as translateDisplayNameForK8s (or call that helper if accessible) and assert interception.request.body.modelRegistry.metadata.name equals that expected transformed string in addition to the length check, referencing the interception alias '@createModelRegistry' and the modelRegistry.metadata.name property to locate where to assert.
360-368: Brittle assertion:cy.contains('Cannot exceed 40 characters')relies on exact UI text.If the warning message changes, this test breaks. Consider using a
data-testidon the warning element and asserting visibility, or at minimum documenting why exact text matching is acceptable here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts` around lines 360 - 368, The test "should show character count warning when name approaches 40 limit" uses a brittle text match (cy.contains('Cannot exceed 40 characters')); update the app to add a stable test id on the name-length warning (e.g., data-testid="name-length-warning") and change the spec to assert visibility via that test id (use cy.get('[data-testid="name-length-warning"]').should('be.visible')); keep the existing remaining-count assertion (cy.contains('remaining')) or similarly replace it with a test id if available, and locate changes around modelRegistrySettings.findFormField(FormFieldSelector.NAME) where the warning is triggered.frontend/src/pages/modelRegistrySettings/CreateModal.tsx (1)
390-391: Duplicated k8sName computation logic.The
k8sNamederivation (nameDesc.k8sName.value || translateDisplayNameForK8s(nameDesc.name)) appears identically on line 323 inonSubmit. Consider extracting to a memoized value or helper to avoid logic drift between validation and submission.♻️ Extract shared k8sName computation
+ const effectiveK8sName = nameDesc.k8sName.value || translateDisplayNameForK8s(nameDesc.name); + const canSubmit = () => { - const k8sName = nameDesc.k8sName.value || translateDisplayNameForK8s(nameDesc.name); - const isValidName = isValidK8sName(k8sName) && k8sName.length <= MAX_MODEL_REGISTRY_NAME_LENGTH; + const isValidName = isValidK8sName(effectiveK8sName) && effectiveK8sName.length <= MAX_MODEL_REGISTRY_NAME_LENGTH;Then use
effectiveK8sNameinonSubmitas well (line 323).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/modelRegistrySettings/CreateModal.tsx` around lines 390 - 391, Duplicate k8sName logic: extract the expression nameDesc.k8sName.value || translateDisplayNameForK8s(nameDesc.name) into a single reusable value (e.g., effectiveK8sName) computed once (use useMemo or a small helper function) and replace both uses (the validation isValidName and the onSubmit flow) so validation (isValidName) and submission (onSubmit) both reference effectiveK8sName; keep the same length check with MAX_MODEL_REGISTRY_NAME_LENGTH and ensure names used in isValidK8sName and submit payload reference the new symbol.
🤖 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/pages/modelRegistrySettings/CreateModal.tsx`:
- Around line 390-391: Duplicate k8sName logic: extract the expression
nameDesc.k8sName.value || translateDisplayNameForK8s(nameDesc.name) into a
single reusable value (e.g., effectiveK8sName) computed once (use useMemo or a
small helper function) and replace both uses (the validation isValidName and the
onSubmit flow) so validation (isValidName) and submission (onSubmit) both
reference effectiveK8sName; keep the same length check with
MAX_MODEL_REGISTRY_NAME_LENGTH and ensure names used in isValidK8sName and
submit payload reference the new symbol.
In
`@packages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts`:
- Around line 370-383: The test only checks the k8s name length after submitting
a 40-char display name but doesn't assert the actual transformed value; update
the spec in modelRegistrySettings.cy.ts to compute the expected K8s-safe name
using the same logic as translateDisplayNameForK8s (or call that helper if
accessible) and assert interception.request.body.modelRegistry.metadata.name
equals that expected transformed string in addition to the length check,
referencing the interception alias '@createModelRegistry' and the
modelRegistry.metadata.name property to locate where to assert.
- Around line 360-368: The test "should show character count warning when name
approaches 40 limit" uses a brittle text match (cy.contains('Cannot exceed 40
characters')); update the app to add a stable test id on the name-length warning
(e.g., data-testid="name-length-warning") and change the spec to assert
visibility via that test id (use
cy.get('[data-testid="name-length-warning"]').should('be.visible')); keep the
existing remaining-count assertion (cy.contains('remaining')) or similarly
replace it with a test id if available, and locate changes around
modelRegistrySettings.findFormField(FormFieldSelector.NAME) where the warning is
triggered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3287a93d-3af8-48fa-9038-57084b33e320
📒 Files selected for processing (3)
frontend/src/pages/modelRegistrySettings/CreateModal.tsxfrontend/src/pages/modelRegistrySettings/const.tspackages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/pages/modelRegistrySettings/CreateModal.tsx (2)
420-441: ContentModal integration is correct; consider disabling Cancel during submission.The
buttonActionsarray properly wires submit/cancel handlers withisLoadingandisDisabled. However, Cancel remains clickable whileisSubmitting=true. If clicked mid-flight,onBeforeClose()resets state and closes the modal while the async operation is still pending—leading to state updates on an unmounted component.🛠️ Optional: Disable Cancel during submission
{ label: 'Cancel', onClick: onCancelClose, variant: 'link', + isDisabled: isSubmitting, dataTestId: 'modal-cancel-button', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/modelRegistrySettings/CreateModal.tsx` around lines 420 - 441, Cancel remains clickable during submission; update the Cancel action in the ContentModal's buttonActions so it is disabled while isSubmitting is true (e.g., set the Cancel action's isDisabled: isSubmitting or guard onCancelClose to return early when isSubmitting) to prevent closing/resetting state mid-flight; reference the buttonActions array, the Cancel action entry, the isSubmitting flag and the onCancelClose handler when making the change.
444-449: AddmaxLengthattribute to resource name input for consistent UX.The display name field has
maxLength={MAX_MODEL_REGISTRY_NAME_LENGTH}preventing user input beyond the limit. The resource name field lacks this attribute, allowing users to type beyond the limit; validation only blocks submission viacanSubmit(). AddmaxLengthto the resource name input inResourceNameFieldto match the display name behavior and provide immediate visual feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/modelRegistrySettings/CreateModal.tsx` around lines 444 - 449, The resource name input is missing a maxLength causing inconsistent UX; add a maxLength prop with the same constant used for the display name. Update the ResourceNameField usage to pass maxLength={MAX_MODEL_REGISTRY_NAME_LENGTH} and ensure the ResourceNameField component forwards that prop to the underlying input element (or adds it to the input JSX inside ResourceNameField). Reference symbols: ResourceNameField and MAX_MODEL_REGISTRY_NAME_LENGTH.
🤖 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/pages/modelRegistrySettings/CreateModal.tsx`:
- Around line 420-441: Cancel remains clickable during submission; update the
Cancel action in the ContentModal's buttonActions so it is disabled while
isSubmitting is true (e.g., set the Cancel action's isDisabled: isSubmitting or
guard onCancelClose to return early when isSubmitting) to prevent
closing/resetting state mid-flight; reference the buttonActions array, the
Cancel action entry, the isSubmitting flag and the onCancelClose handler when
making the change.
- Around line 444-449: The resource name input is missing a maxLength causing
inconsistent UX; add a maxLength prop with the same constant used for the
display name. Update the ResourceNameField usage to pass
maxLength={MAX_MODEL_REGISTRY_NAME_LENGTH} and ensure the ResourceNameField
component forwards that prop to the underlying input element (or adds it to the
input JSX inside ResourceNameField). Reference symbols: ResourceNameField and
MAX_MODEL_REGISTRY_NAME_LENGTH.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6ccb9fec-f577-469a-9379-33aee04b31a2
📒 Files selected for processing (2)
frontend/src/pages/modelRegistrySettings/CreateModal.tsxpackages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts
Philip-Carneiro
force-pushed
the
fix/registry-name-field-validation
branch
from
b061b22 to
36f330d
Compare
|
/retest |
Philip-Carneiro
force-pushed
the
fix/registry-name-field-validation
branch
from
bdd80e8 to
52cf7ed
Compare
|
/retest |
Philip-Carneiro
force-pushed
the
fix/registry-name-field-validation
branch
from
a9ae525 to
bb0dd8c
Compare
|
/retest |
Philip-Carneiro
force-pushed
the
fix/registry-name-field-validation
branch
from
7e0c05a to
1c8c5a2
Compare
Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
Philip-Carneiro
force-pushed
the
fix/registry-name-field-validation
branch
from
1c8c5a2 to
bfc4af9
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: YuliaKrimerman 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 e9fa6c1
into
opendatahub-io:main
2 participants
Description
Adds
maxLength=40validation to the Name field in the Model Registry creation modal (CreateModal). When the user types a name longer than 40 characters, inline error feedback is shown and the submit button is disabled.Changes:
K8sNameDescriptionFieldto accept an optionalmaxNameLengthprop and trackinvalidLengthstateHelperTextwithValidatedOptions.errorwhen length exceeds limitCreateModalwhen name length is invalidnameStatethroughRegisterAndStoreFieldsHow Has This Been Tested?
Test Impact
setupDefaults,handleUpdateLogic, error rendering, and form interactionRequest review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests