AGENT-1466: fix validations for NTP and Proxy in above sea level ABI flow#3464
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ElayAharoni: This pull request references AGENT-1466 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new translation key and label for "Additional NTP sources"; tightens NTP validation (trim + optional allowEmpty); makes proxy and NTP validations conditional on toggles, clears fields/errors when toggles disable; forwards TextArea onChange to consumer; adjusts payload building to include proxy/NTP only if enabled and non-empty. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as OptionalConfigurationsForm
participant Val as ntpValidationSchema
participant Builder as buildInfraEnvParams
participant API as InfraEnv API
User->>UI: toggle enableProxy/enableNtpSources
UI->>UI: clear fields/errors/touched if toggled off
User->>UI: enter proxy / additionalNtpSources
UI->>Val: validate additionalNtpSources (allowEmpty depends on toggle)
Val-->>UI: validation result
User->>UI: submit form
UI->>Builder: build payload (include proxy/NTP only if enabled & trimmed non-empty)
Builder->>API: send infraEnv params
API-->>Builder: response
Builder-->>UI: submission result
UI-->>User: show success/error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
openshift-ci
bot
added
the
size/L
|
@ElayAharoni: This pull request references AGENT-1466 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
311-318:⚠️ Potential issue | 🔴 Critical
onNextdoes not trigger Formik form submission — the InfraEnv create/update will be bypassed.The
onNext={moveNext}prop passes only a navigation function to the footer, skipping the FormikonSubmithandler that creates or updates the InfraEnv (lines 204–253). WrapmoveNextto callsubmitForm()first, or use the form'sid="wizard-cluster-optional-configurations__form"to submit the form before navigating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx` around lines 311 - 318, The ClusterWizardFooter currently receives onNext={moveNext}, which navigates without triggering the Formik onSubmit (so InfraEnv create/update is bypassed); change the prop to call the form submission before navigating — either wrap moveNext in a function that calls Formik's submitForm() (or triggers the form with id "wizard-cluster-optional-configurations__form") and awaits completion/validation, then calls moveNext, or replace onNext with a handler that programmatically submits that form and only calls moveNext after submit succeeds; update references around ClusterWizardFooter, moveNext, and the form id to implement this.
🧹 Nitpick comments (1)
libs/locales/lib/en/translation.json (1)
75-76: Consider consolidating duplicate translation keys with casing differences.There are now two translation keys that differ only in casing:
- Line 75:
ai:Additional NTP sources(lowercase 's')- Line 76:
ai:Additional NTP Sources(uppercase 'S')Both map to the same value. While this works, it creates potential confusion and maintenance overhead. Consider standardizing on one key across the codebase to avoid drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/locales/lib/en/translation.json` around lines 75 - 76, Two duplicate translation keys differ only by casing ("ai:Additional NTP sources" vs "ai:Additional NTP Sources") which creates maintenance overhead; pick a single canonical key (e.g., "ai:Additional NTP sources") and remove the other entry, then update any code references that use the removed key to the chosen canonical key by searching for both symbols in the repository and replacing occurrences to keep translations consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 311-318: The ClusterWizardFooter currently receives
onNext={moveNext}, which navigates without triggering the Formik onSubmit (so
InfraEnv create/update is bypassed); change the prop to call the form submission
before navigating — either wrap moveNext in a function that calls Formik's
submitForm() (or triggers the form with id
"wizard-cluster-optional-configurations__form") and awaits
completion/validation, then calls moveNext, or replace onNext with a handler
that programmatically submits that form and only calls moveNext after submit
succeeds; update references around ClusterWizardFooter, moveNext, and the form
id to implement this.
---
Nitpick comments:
In `@libs/locales/lib/en/translation.json`:
- Around line 75-76: Two duplicate translation keys differ only by casing
("ai:Additional NTP sources" vs "ai:Additional NTP Sources") which creates
maintenance overhead; pick a single canonical key (e.g., "ai:Additional NTP
sources") and remove the other entry, then update any code references that use
the removed key to the chosen canonical key by searching for both symbols in the
repository and replacing occurrences to keep translations consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3180ecb9-8465-4f10-99a3-0624d01309ba
📒 Files selected for processing (8)
libs/locales/lib/en/translation.jsonlibs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsxlibs/ui-lib/lib/common/components/hosts/AdditionalNTPSourcesDialog.tsxlibs/ui-lib/lib/common/components/ui/formik/AdditionalNTPSourcesField.tsxlibs/ui-lib/lib/common/components/ui/formik/TextAreaField.tsxlibs/ui-lib/lib/common/config/constants.tslibs/ui-lib/lib/common/validationSchemas/ntpValidation.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
libs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/common/components/ui/formik/AdditionalNTPSourcesField.tsx
Outdated
Show resolved
Hide resolved
openshift-ci
bot
added
the
needs-rebase
ElayAharoni
force-pushed
the
Implement-Conditional-Mandatory-Validation-in-SaaS-UI-for-Proxy-and-NTP-Configurations
branch
from
f099967 to
357aa65
Compare
|
@ElayAharoni: This pull request references AGENT-1466 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/locales/lib/en/translation.json (1)
75-76: Consolidate duplicated i18n key casing for “Additional NTP sources”.Having both
ai:Additional NTP sourcesandai:Additional NTP Sourcescreates key drift across call sites/locales. Prefer one canonical key, migrate usages, then remove the duplicate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/locales/lib/en/translation.json` around lines 75 - 76, Duplicate i18n keys exist: "ai:Additional NTP sources" and "ai:Additional NTP Sources"; choose one canonical key (use "ai:Additional NTP sources"), update all call sites to use that exact key, remove the duplicate entry from libs/locales/lib/en/translation.json, and mirror the same consolidation across other locale files and tests (run i18n lint or search/replace to ensure no remaining usages of "ai:Additional NTP Sources").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 268-278: The Next button currently calls moveNext() directly via
WizardFooter, bypassing Formik submission in OptionalConfigurationsStep and
skipping the onSubmit logic that persists infra-env changes; fix by restoring
Formik's submit mechanism: include submitForm in the render props destructure
(add submitForm to the list returned by Formik) and either (a) call await
submitForm() before calling moveNext() in the WizardFooter onClick handler, or
(b) change the WizardFooter Next control to be a form submit (type="submit") and
ensure the step content is wrapped in Formik's <form> so onSubmit (the infra-env
create/update logic) runs before navigation. Ensure moveNext is only invoked
after a successful submit/validation.
---
Nitpick comments:
In `@libs/locales/lib/en/translation.json`:
- Around line 75-76: Duplicate i18n keys exist: "ai:Additional NTP sources" and
"ai:Additional NTP Sources"; choose one canonical key (use "ai:Additional NTP
sources"), update all call sites to use that exact key, remove the duplicate
entry from libs/locales/lib/en/translation.json, and mirror the same
consolidation across other locale files and tests (run i18n lint or
search/replace to ensure no remaining usages of "ai:Additional NTP Sources").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 153e691c-c4e6-4d40-826c-3268bdfa5052
📒 Files selected for processing (7)
libs/locales/lib/en/translation.jsonlibs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsxlibs/ui-lib/lib/common/components/hosts/AdditionalNTPSourcesDialog.tsxlibs/ui-lib/lib/common/components/ui/formik/TextAreaField.tsxlibs/ui-lib/lib/common/config/constants.tslibs/ui-lib/lib/common/validationSchemas/ntpValidation.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
✅ Files skipped from review due to trivial changes (1)
- libs/ui-lib/lib/common/config/constants.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsx
- libs/ui-lib/lib/common/components/ui/formik/TextAreaField.tsx
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Implement-Conditional-Mandatory-Validation-in-SaaS-UI-for-Proxy-and-NTP-Configurations
branch
from
357aa65 to
25f5c12
Compare
openshift-ci
bot
removed
the
needs-rebase
|
@ElayAharoni: This pull request references AGENT-1466 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/ui-lib/lib/common/validationSchemas/ntpValidation.tsx (1)
6-15:⚠️ Potential issue | 🟠 MajorPotential silent behavior change for legacy two-arg callers
Line 14 removes the explicit second parameter, so any legacy call like
ntpSourceValidationSchema(t, false)will ignorefalseat runtime and default toallowEmpty=true. That can unintentionally disable required validation.💡 Suggested compatibility-safe change
-type NtpSourceValidationOptions = { t: TFunction; allowEmpty?: boolean } | TFunction; +type NtpSourceValidationOptions = { t: TFunction; allowEmpty?: boolean } | TFunction; -const parseOptions = (options: NtpSourceValidationOptions) => { - const t = typeof options === 'function' ? options : options.t; - const allowEmpty = typeof options === 'function' ? true : options.allowEmpty ?? true; +const parseOptions = (optionsOrT: NtpSourceValidationOptions, allowEmptyArg?: boolean) => { + const t = typeof optionsOrT === 'function' ? optionsOrT : optionsOrT.t; + const allowEmpty = + typeof optionsOrT === 'function' ? allowEmptyArg ?? true : optionsOrT.allowEmpty ?? true; return { t, allowEmpty }; }; -export const ntpSourceValidationSchema = (options: NtpSourceValidationOptions) => { - const { t, allowEmpty } = parseOptions(options); +export const ntpSourceValidationSchema = ( + optionsOrT: NtpSourceValidationOptions, + allowEmptyArg?: boolean, +) => { + const { t, allowEmpty } = parseOptions(optionsOrT, allowEmptyArg);#!/bin/bash # Verify that no legacy two-argument calls remain. # Expected: no matches. ast-grep --pattern 'ntpSourceValidationSchema($A, $B)' # Fallback contextual grep for manual inspection. rg -nP -C2 '\bntpSourceValidationSchema\s*\([^)]*,[^)]*\)' -g '!**/node_modules/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/common/validationSchemas/ntpValidation.tsx` around lines 6 - 15, The refactor removed the explicit second parameter, breaking legacy calls like ntpSourceValidationSchema(t, false); restore compatibility by updating ntpSourceValidationSchema to accept a second optional boolean (e.g., ntpSourceValidationSchema(options: NtpSourceValidationOptions | TFunction, allowEmpty?: boolean)), or add a function overload, and change parseOptions to prefer an explicitly passed allowEmpty when defined (use the second arg or arguments[1] if needed) before falling back to options.allowEmpty or true; update type NtpSourceValidationOptions or overloads accordingly so calls to ntpSourceValidationSchema(t, false) honor false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/ui-lib/lib/common/validationSchemas/ntpValidation.tsx`:
- Around line 6-15: The refactor removed the explicit second parameter, breaking
legacy calls like ntpSourceValidationSchema(t, false); restore compatibility by
updating ntpSourceValidationSchema to accept a second optional boolean (e.g.,
ntpSourceValidationSchema(options: NtpSourceValidationOptions | TFunction,
allowEmpty?: boolean)), or add a function overload, and change parseOptions to
prefer an explicitly passed allowEmpty when defined (use the second arg or
arguments[1] if needed) before falling back to options.allowEmpty or true;
update type NtpSourceValidationOptions or overloads accordingly so calls to
ntpSourceValidationSchema(t, false) honor false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e48a3b76-dc44-4771-b3b3-3b2068fa1452
📒 Files selected for processing (7)
libs/locales/lib/en/translation.jsonlibs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsxlibs/ui-lib/lib/common/components/hosts/AdditionalNTPSourcesDialog.tsxlibs/ui-lib/lib/common/components/ui/formik/TextAreaField.tsxlibs/ui-lib/lib/common/config/constants.tslibs/ui-lib/lib/common/validationSchemas/ntpValidation.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
✅ Files skipped from review due to trivial changes (2)
- libs/ui-lib/lib/common/components/ui/formik/TextAreaField.tsx
- libs/ui-lib/lib/common/config/constants.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsx
- libs/locales/lib/en/translation.json
- libs/ui-lib/lib/common/components/hosts/AdditionalNTPSourcesDialog.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
ElayAharoni
force-pushed
the
Implement-Conditional-Mandatory-Validation-in-SaaS-UI-for-Proxy-and-NTP-Configurations
branch
from
25f5c12 to
bb0a789
Compare
openshift-ci
bot
added
lgtm
|
@ElayAharoni: This pull request references AGENT-1466 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
Implement-Conditional-Mandatory-Validation-in-SaaS-UI-for-Proxy-and-NTP-Configurations
branch
from
bb0a789 to
a05a626
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/ui-lib/lib/common/config/constants.ts`:
- Line 74: The translation key for the "Additional NTP Sources" label is
inconsistent between constants.ts (additionalNtpSources: t('ai:Additional NTP
sources')) and AdditionalNTPSourcesDialog.tsx (t('ai:Additional NTP Sources'));
pick one canonical key and make both use it—either change the string in
libs/ui-lib/lib/common/config/constants.ts to t('ai:Additional NTP Sources') or
update AdditionalNTPSourcesDialog.tsx to consume the
constants.additionalNtpSources value so both reference the exact same i18n key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23813519-babf-480b-9572-3dacec6a1162
📒 Files selected for processing (7)
libs/locales/lib/en/translation.jsonlibs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsxlibs/ui-lib/lib/common/components/hosts/AdditionalNTPSourcesDialog.tsxlibs/ui-lib/lib/common/components/ui/formik/TextAreaField.tsxlibs/ui-lib/lib/common/config/constants.tslibs/ui-lib/lib/common/validationSchemas/ntpValidation.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
✅ Files skipped from review due to trivial changes (2)
- libs/ui-lib/lib/common/components/clusterConfiguration/ProxyFields.tsx
- libs/locales/lib/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/common/components/ui/formik/TextAreaField.tsx
- libs/ui-lib/lib/common/validationSchemas/ntpValidation.tsx
|
@ElayAharoni: This pull request references AGENT-1466 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni, rawagner 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 e189939
into
openshift-assisted:master
3 participants
https://redhat.atlassian.net/browse/AGENT-1466
Summary by CodeRabbit
New Features
Improvements