ECOPROJECT-4330 | refactor: extract EnvironmentForm component and add network config helpers#569
Closed
rh-gvincent wants to merge 1 commit intokubev2v:masterfrom
Closed
ECOPROJECT-4330 | refactor: extract EnvironmentForm component and add network config helpers#569rh-gvincent wants to merge 1 commit intokubev2v:masterfrom
rh-gvincent wants to merge 1 commit intokubev2v:masterfrom
Conversation
… network config helpers Extract EnvironmentForm from DiscoverySourceSetupModal to enable reuse across the application. Add networkConfig helper following proxyConfig pattern for type-safe network configuration extraction. Use react hook form and yup for form and validation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Guillaume Vincent <gvincent@redhat.com>
|
[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 |
📝 WalkthroughWalkthroughIntroduced Changes
Sequence DiagramsequenceDiagram
participant User
participant DiscoverySourceSetupModal
participant EnvironmentForm
participant EnvironmentPageViewModel
participant SourcesStore
rect rgb(200, 150, 255, 0.5)
Note over User,SourcesStore: Create New Source Flow (Refactored)
end
User->>DiscoverySourceSetupModal: Opens modal (sourceSelected is null)
DiscoverySourceSetupModal->>EnvironmentForm: Renders form with empty defaults
User->>EnvironmentForm: Fills form (name, proxy, network config)
User->>EnvironmentForm: Clicks Submit
EnvironmentForm->>EnvironmentForm: Validates against schema
alt Validation passes
EnvironmentForm->>DiscoverySourceSetupModal: onSubmit(values)
DiscoverySourceSetupModal->>EnvironmentPageViewModel: createDownloadSource({...values, enableProxy})
EnvironmentPageViewModel->>SourcesStore: create source with payload
SourcesStore-->>EnvironmentPageViewModel: newSource.id
EnvironmentPageViewModel->>EnvironmentPageViewModel: selectSourceById(newSource.id)
EnvironmentPageViewModel-->>DiscoverySourceSetupModal: sourceSelected updated
DiscoverySourceSetupModal->>DiscoverySourceSetupModal: Show download URL
User->>DiscoverySourceSetupModal: Downloads OVA
else Validation fails
EnvironmentForm->>EnvironmentForm: Show field errors
EnvironmentForm-->>User: Display error messages
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/assessment/view-models/useCreateFromOvaViewModel.ts (1)
267-274:⚠️ Potential issue | 🟡 MinorAdd
sourceCreatedIdto the dependency arrays for bothuseAsyncFncallbacks.Both
doRefreshAfterCloseanddoRefreshAfterDownloadclose oversourceCreatedId, which is derived fromenvVm.sourceSelected?.id. WhensourceCreatedIdchanges independently of theenvVmreference, the callbacks capture a stale value, causing the preselection logic (setUseExisting,setSelectedEnvironmentId) to be skipped.🧩 Minimal fix
const [refreshAfterCloseState, doRefreshAfterClose] = useAsyncFn(async () => { const newId = sourceCreatedId; await envVm.listSources(); if (newId) { setUseExisting(true); setSelectedEnvironmentId(newId); } - }, [envVm]); + }, [envVm, sourceCreatedId]); const [, doRefreshAfterDownload] = useAsyncFn(async () => { const newId = sourceCreatedId; await envVm.listSources(); if (newId) { setUseExisting(true); setSelectedEnvironmentId(newId); } - }, [envVm]); + }, [envVm, sourceCreatedId]);Also applies to: 281-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/assessment/view-models/useCreateFromOvaViewModel.ts` around lines 267 - 274, The two useAsyncFn callbacks (doRefreshAfterClose and doRefreshAfterDownload) close over sourceCreatedId and can capture a stale value; update their dependency arrays to include sourceCreatedId in addition to envVm so the hooks re-create when sourceCreatedId changes, ensuring the logic that uses sourceCreatedId (newId, setUseExisting, setSelectedEnvironmentId) runs with the current value.
🧹 Nitpick comments (4)
src/ui/environment/views/DiscoverySourceSetupModal.tsx (1)
193-206: SSH key is normalized twice.
normalizeSshKey(sshKey)is called here, butEnvironmentFormalready normalizes the SSH key in itshandleFormSubmit(seesrc/ui/environment/components/EnvironmentForm.tsxlines 245-249). While calling normalize twice is safe (idempotent), it's redundant.♻️ Proposed fix - remove redundant normalization
Since
EnvironmentForm.handleFormSubmitalready normalizes, you can pass the value directly:void vm .createDownloadSource({ name, - sshPublicKey: normalizeSshKey(sshKey), + sshPublicKey: sshKey, enableProxy,Apply similarly to the update path at line 212.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/environment/views/DiscoverySourceSetupModal.tsx` around lines 193 - 206, The call to normalizeSshKey is redundant because EnvironmentForm.handleFormSubmit already normalizes the SSH key; update the create and update calls in DiscoverySourceSetupModal (the .createDownloadSource and .updateDownloadSource invocations) to pass sshKey directly (sshKey) instead of normalizeSshKey(sshKey), removing the duplicate normalization while leaving normalizeSshKey usage in EnvironmentForm.handleFormSubmit intact.src/ui/core/components/form/CheckboxFormGroup.tsx (1)
12-13: Remove unusedplaceholderprop from CheckboxFormGroup.PatternFly's
Checkboxcomponent doesn't support aplaceholderprop. The prop is destructured fromFormGroupPropsbut passing it toCheckboxhas no effect.♻️ Proposed fix
export default function CheckboxFormGroup({ id, label, name, isRequired = false, - placeholder = "", ...props }: FormGroupProps) {And remove line 33:
<Checkbox label={label} id={id} - placeholder={placeholder} isRequired={isRequired}Also applies to: 30-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/core/components/form/CheckboxFormGroup.tsx` around lines 12 - 13, The CheckboxFormGroup component is destructuring and declaring a placeholder prop (from FormGroupProps) but PatternFly's Checkbox does not accept it; remove the placeholder from the props list in the CheckboxFormGroup definition and stop passing it into the Checkbox so the unused prop is not forwarded—update the component where placeholder is declared/used (reference: CheckboxFormGroup, placeholder, FormGroupProps, Checkbox) and remove the placeholder-related code (including the stray placeholder in the destructuring and the prop passed into <Checkbox />).src/ui/environment/components/EnvironmentForm.tsx (1)
74-163: Extract duplicated IP address validation logic.The IP address validation logic is duplicated three times (for
ipAddress,defaultGateway, anddns). Consider extracting it into a reusable validation helper.♻️ Proposed refactor
+const isValidIpAddress = (value: string | undefined): boolean => { + if (!value) return false; + const ipPattern = /^((25[0-5]|(2[0-4]|1\d|[1-9]|)\d)\.?\b){4}$/; + if (!ipPattern.test(value.trim())) return false; + const parts = value.trim().split("."); + if (parts.length !== 4) return false; + return parts.every((part) => { + const num = parseInt(part, 10); + return !isNaN(num) && num >= 0 && num <= 255; + }); +}; + +const ipAddressValidation = (schema: yup.StringSchema) => + schema + .required("IP address is required") + .test( + "ip-address", + "Invalid IP address format. Please use format like 192.168.1.100", + isValidIpAddress, + ); // Then use in the schema: ipAddress: yup .string() .default("") .when("networkConfigType", { is: "static", - then: (schema) => - schema - .required("IP address is required") - .test( - "ip-address", - "Invalid IP address format. Please use format like 192.168.1.100", - (value) => { - if (!value) return false; - const ipPattern = /^((25[0-5]|(2[0-4]|1\d|[1-9]|)\d)\.?\b){4}$/; - if (!ipPattern.test(value.trim())) return false; - const parts = value.trim().split("."); - if (parts.length !== 4) return false; - return parts.every((part) => { - const num = parseInt(part, 10); - return !isNaN(num) && num >= 0 && num <= 255; - }); - }, - ), + then: ipAddressValidation, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/environment/components/EnvironmentForm.tsx` around lines 74 - 163, The IP validation logic is duplicated for ipAddress, defaultGateway, and dns; extract it into a single reusable helper (e.g., isValidIPv4(value): boolean) and replace the inline test functions in the yup .test calls for ipAddress, defaultGateway, and dns to call that helper; keep the same error messages and ensure the helper trims input, checks the regex and splits into four octets validating 0–255 so existing schema methods (ipAddress, defaultGateway, dns) behave identically but without duplicated code.src/ui/environment/view-models/useEnvironmentPageViewModel.ts (1)
196-210: MissingselectSourceByIdinuseAsyncFndependency array.
selectSourceByIdis called insidedoCreateDownloadSourcebut isn't listed in the dependency array. WhileselectSourceByIdis stable due touseCallback, including it follows best practices and avoids potential stale closure issues if the implementation changes.♻️ Proposed fix
const [createDownloadState, doCreateDownloadSource] = useAsyncFn( async (input: SourceCreateInput): Promise<void> => { setDismissDownloadError(false); try { const newSource = await sourcesStore.create(input); await imagesStore.headImage(newSource.id); const url = await imagesStore.getDownloadUrl(newSource.id); setDownloadSourceUrlRaw(url); selectSourceById(newSource.id); } catch (err) { throw await parseApiError(err, "Failed to create environment"); } }, - [sourcesStore, imagesStore], + [sourcesStore, imagesStore, selectSourceById], );Apply the same to
doUpdateSourceat line 226:- [sourcesStore, imagesStore], + [sourcesStore, imagesStore, selectSourceById],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/environment/view-models/useEnvironmentPageViewModel.ts` around lines 196 - 210, The async callback passed to useAsyncFn for doCreateDownloadSource (and similarly for doUpdateSource) references selectSourceById but the hook dependency array only lists sourcesStore and imagesStore; add selectSourceById to the dependency arrays of both useAsyncFn calls so the closures stay correct (update the dependency arrays for the functions creating doCreateDownloadSource and doUpdateSource to include selectSourceById).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/assessment/view-models/useCreateFromOvaViewModel.ts`:
- Around line 113-116: The code is incorrectly using envVm.sourceSelected to
populate sourceCreatedId and createdSource, which leaks preselected existing
sources into the "create new environment" flow; change the logic to use the
actual newly created source identifier/state from this creation flow (e.g., the
result of the create source call or a dedicated state like newSource or
sourceCreated returned by the createSource function) instead of
envVm.sourceSelected?.id, and ensure createdSource is derived from
envVm.getSourceById(newlyCreatedSourceId) only when a source was actually
created in this session; apply the same replacement for the other occurrences
(the blocks referenced around the other lines that set
sourceCreatedId/createdSource and the spots used by handleSubmit) so
handleSubmit only submits against the newly created source when present.
In `@src/ui/core/components/form/RadioButtonFormGroup.tsx`:
- Line 42: The div in RadioButtonFormGroup.tsx uses an inline style (display:
"flex", gap: "16px") which violates the CSS-in-JS guideline; replace it by
importing the css helper from `@emotion/css`, define a named class (e.g.,
container or radioGroupContainer) with "display: flex; gap: 16px;" and apply it
via className on the div in the RadioButtonFormGroup component; remove the
inline style and ensure the newly created class is exported/kept local to the
module as needed.
- Around line 34-57: The component calculates showError as "error && isTouched"
which hides validation errors after a submit if the field was never touched;
update the logic to show errors when the field is touched OR the form has been
submitted (e.g. use methods.formState.isSubmitted), so change the showError
determination in RadioButtonFormGroup to "error && (isTouched ||
methods.formState.isSubmitted)" (or equivalent) so the Controller render and
FormErrorMessage receive the correct value and submit-time validation messages
are displayed.
In `@src/ui/core/components/form/TextAreaFormGroup.tsx`:
- Around line 21-35: The current showError logic (showError = error &&
isTouched) suppresses validation messages on submit for untouched fields; change
showError to show when there is an error AND (the field was touched OR the form
has been submitted) by using methods.formState.isSubmitted (e.g., showError =
error && (isTouched || methods.formState.isSubmitted)); update the references to
showError used for the TextArea validated prop and FormErrorMessage so
submit-time errors are displayed while preserving blur-based behavior.
In `@src/ui/environment/components/__tests__/EnvironmentForm.test.tsx`:
- Around line 285-336: The test mixes the global userEvent.click and the setup
instance user; replace the direct call to userEvent.click(enableProxyCheckbox)
with await user.click(enableProxyCheckbox) so the test consistently uses the
userEvent.setup() instance (created as user) and preserves proper event
sequencing for EnvironmentForm interactions.
In `@src/ui/environment/components/EnvironmentForm.tsx`:
- Around line 362-377: The subnet mask TextInput (id="subnet-mask-form-control",
registered via methods.register("subnetMask") in EnvironmentForm.tsx) currently
uses aria-describedby="ip-address-helper-text" but no element has that ID;
either update the helper text element(s) (the HelperTextItem(s) rendered for the
IP/subnet helper) to include id="ip-address-helper-text" so the input references
a real element, or change/remove the aria-describedby on both the IP address and
subnet mask TextInput controls to either reference the correct helper ID you add
or omit the attribute if no explicit relationship is needed.
In `@src/ui/environment/views/Environment.tsx`:
- Around line 56-59: The modal is reusing vm.sourceSelected so the "Add
environment" flow can show previous edits; ensure a clear create/edit boundary
by clearing or resetting vm.sourceSelected when opening/closing the
DiscoverySourceSetupModal for a create flow: update the open/close handlers
(e.g., the function closeModalAndLoadSources and whatever opens the modal) to
call a vm.clearSelectedSource or set vm.sourceSelected = undefined (or pass an
explicit prop like mode="create" to DiscoverySourceSetupModal) before
setShouldShowDiscoverySetupModal(true/false) and before calling vm.listSources()
so the modal initializes with blank values for creates and retains selection
only for edits; also apply same reset in other related handlers referenced at
lines around 100-107 and 154-158.
In `@src/ui/environment/views/SourcesTable.tsx`:
- Around line 297-300: handleDelete currently calls vm.deleteAndRefresh but
doesn't clear the shared selection, leaving vm.sourceSelected pointing at a
deleted id; update handleDelete so that before calling vm.deleteAndRefresh it
checks if vm.sourceSelected?.id === source.id and clears the selection (e.g.
call vm.clearSelection() or set vm.sourceSelected = null), then proceed with
setDeleteTarget(null) and void vm.deleteAndRefresh(source.id) so the view-model
no longer references the deleted SourceModel.
---
Outside diff comments:
In `@src/ui/assessment/view-models/useCreateFromOvaViewModel.ts`:
- Around line 267-274: The two useAsyncFn callbacks (doRefreshAfterClose and
doRefreshAfterDownload) close over sourceCreatedId and can capture a stale
value; update their dependency arrays to include sourceCreatedId in addition to
envVm so the hooks re-create when sourceCreatedId changes, ensuring the logic
that uses sourceCreatedId (newId, setUseExisting, setSelectedEnvironmentId) runs
with the current value.
---
Nitpick comments:
In `@src/ui/core/components/form/CheckboxFormGroup.tsx`:
- Around line 12-13: The CheckboxFormGroup component is destructuring and
declaring a placeholder prop (from FormGroupProps) but PatternFly's Checkbox
does not accept it; remove the placeholder from the props list in the
CheckboxFormGroup definition and stop passing it into the Checkbox so the unused
prop is not forwarded—update the component where placeholder is declared/used
(reference: CheckboxFormGroup, placeholder, FormGroupProps, Checkbox) and remove
the placeholder-related code (including the stray placeholder in the
destructuring and the prop passed into <Checkbox />).
In `@src/ui/environment/components/EnvironmentForm.tsx`:
- Around line 74-163: The IP validation logic is duplicated for ipAddress,
defaultGateway, and dns; extract it into a single reusable helper (e.g.,
isValidIPv4(value): boolean) and replace the inline test functions in the yup
.test calls for ipAddress, defaultGateway, and dns to call that helper; keep the
same error messages and ensure the helper trims input, checks the regex and
splits into four octets validating 0–255 so existing schema methods (ipAddress,
defaultGateway, dns) behave identically but without duplicated code.
In `@src/ui/environment/view-models/useEnvironmentPageViewModel.ts`:
- Around line 196-210: The async callback passed to useAsyncFn for
doCreateDownloadSource (and similarly for doUpdateSource) references
selectSourceById but the hook dependency array only lists sourcesStore and
imagesStore; add selectSourceById to the dependency arrays of both useAsyncFn
calls so the closures stay correct (update the dependency arrays for the
functions creating doCreateDownloadSource and doUpdateSource to include
selectSourceById).
In `@src/ui/environment/views/DiscoverySourceSetupModal.tsx`:
- Around line 193-206: The call to normalizeSshKey is redundant because
EnvironmentForm.handleFormSubmit already normalizes the SSH key; update the
create and update calls in DiscoverySourceSetupModal (the .createDownloadSource
and .updateDownloadSource invocations) to pass sshKey directly (sshKey) instead
of normalizeSshKey(sshKey), removing the duplicate normalization while leaving
normalizeSshKey usage in EnvironmentForm.handleFormSubmit intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de24452d-b720-4082-b0a4-4ff19dc3af9b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
package.jsonsrc/data/stores/SourcesStore.tssrc/ui/assessment/view-models/__tests__/useCreateFromOvaViewModel.test.tssrc/ui/assessment/view-models/useCreateFromOvaViewModel.tssrc/ui/core/components/form/CheckboxFormGroup.tsxsrc/ui/core/components/form/FormErrorMessage.tsxsrc/ui/core/components/form/RadioButtonFormGroup.tsxsrc/ui/core/components/form/TextAreaFormGroup.tsxsrc/ui/core/components/form/TextInputFormGroup.tsxsrc/ui/core/components/form/index.tssrc/ui/core/components/form/types.tssrc/ui/environment/components/EnvironmentForm.tsxsrc/ui/environment/components/__tests__/EnvironmentForm.test.tsxsrc/ui/environment/helpers/__tests__/networkConfig.test.tssrc/ui/environment/helpers/__tests__/proxyConfig.test.tssrc/ui/environment/helpers/networkConfig.tssrc/ui/environment/helpers/proxyConfig.tssrc/ui/environment/view-models/__tests__/useEnvironmentPageViewModel.test.tssrc/ui/environment/view-models/useEnvironmentPageViewModel.tssrc/ui/environment/views/DiscoverySourceSetupModal.tsxsrc/ui/environment/views/Environment.tsxsrc/ui/environment/views/SourcesTable.tsxsrc/ui/environment/views/__tests__/SourcesTable.test.tsx
💤 Files with no reviewable changes (1)
- src/ui/environment/views/tests/SourcesTable.test.tsx
Comment on lines
+113
to
116
| const sourceCreatedId = envVm.sourceSelected?.id || null; | ||
| const createdSource = sourceCreatedId | ||
| ? envVm.getSourceById(sourceCreatedId) | ||
| : undefined; |
There was a problem hiding this comment.
Don't use sourceSelected as sourceCreatedId.
envVm.sourceSelected also represents existing and preselected environments. Reusing it here means the "create new environment" path can become enabled—and handleSubmit() can submit—against whatever source happened to be selected before this flow, even if no new source was created in this session.
Also applies to: 136-137, 225-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/assessment/view-models/useCreateFromOvaViewModel.ts` around lines 113
- 116, The code is incorrectly using envVm.sourceSelected to populate
sourceCreatedId and createdSource, which leaks preselected existing sources into
the "create new environment" flow; change the logic to use the actual newly
created source identifier/state from this creation flow (e.g., the result of the
create source call or a dedicated state like newSource or sourceCreated returned
by the createSource function) instead of envVm.sourceSelected?.id, and ensure
createdSource is derived from envVm.getSourceById(newlyCreatedSourceId) only
when a source was actually created in this session; apply the same replacement
for the other occurrences (the blocks referenced around the other lines that set
sourceCreatedId/createdSource and the spots used by handleSubmit) so
handleSubmit only submits against the newly created source when present.
Comment on lines
+34
to
+57
| const showError = error && isTouched; | ||
|
|
||
| return ( | ||
| <FormGroup label={label} isRequired={isRequired} fieldId={id} {...props}> | ||
| <Controller | ||
| name={name} | ||
| control={methods.control} | ||
| render={({ field }) => ( | ||
| <div style={{ display: "flex", gap: "16px" }}> | ||
| {options.map((option) => ( | ||
| <Radio | ||
| key={option.value} | ||
| id={option.id} | ||
| name={name} | ||
| label={option.label} | ||
| isChecked={field.value === option.value} | ||
| onChange={() => field.onChange(option.value)} | ||
| /> | ||
| ))} | ||
| </div> | ||
| )} | ||
| /> | ||
| <FormErrorMessage error={showError ? error : undefined} /> | ||
| </FormGroup> |
There was a problem hiding this comment.
Submit-time validation feedback is currently suppressed.
This component only shows errors when touched, so existing errors can stay hidden after submit.
Suggested fix
const isTouched = methods.formState.touchedFields[name] as
| boolean
| undefined;
+ const isSubmitted = methods.formState.isSubmitted;
- const showError = error && isTouched;
+ const showError = !!error && (isTouched || isSubmitted);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/core/components/form/RadioButtonFormGroup.tsx` around lines 34 - 57,
The component calculates showError as "error && isTouched" which hides
validation errors after a submit if the field was never touched; update the
logic to show errors when the field is touched OR the form has been submitted
(e.g. use methods.formState.isSubmitted), so change the showError determination
in RadioButtonFormGroup to "error && (isTouched ||
methods.formState.isSubmitted)" (or equivalent) so the Controller render and
FormErrorMessage receive the correct value and submit-time validation messages
are displayed.
| name={name} | ||
| control={methods.control} | ||
| render={({ field }) => ( | ||
| <div style={{ display: "flex", gap: "16px" }}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace inline style with @emotion/css class.
Inline style here breaks the repository styling rule for TS/TSX components.
Suggested fix
+import { css } from "@emotion/css";
import { FormGroup, Radio } from "@patternfly/react-core";
import { Controller, type FieldError, useFormContext } from "react-hook-form";
@@
+const radioGroupClass = css({
+ display: "flex",
+ gap: "16px",
+});
@@
- <div style={{ display: "flex", gap: "16px" }}>
+ <div className={radioGroupClass}>📝 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
| <div style={{ display: "flex", gap: "16px" }}> | |
| import { css } from "@emotion/css"; | |
| import { FormGroup, Radio } from "@patternfly/react-core"; | |
| import { Controller, type FieldError, useFormContext } from "react-hook-form"; | |
| const radioGroupClass = css({ | |
| display: "flex", | |
| gap: "16px", | |
| }); | |
| // ... rest of component code ... | |
| <div className={radioGroupClass}> | |
| {/* content */} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/core/components/form/RadioButtonFormGroup.tsx` at line 42, The div in
RadioButtonFormGroup.tsx uses an inline style (display: "flex", gap: "16px")
which violates the CSS-in-JS guideline; replace it by importing the css helper
from `@emotion/css`, define a named class (e.g., container or radioGroupContainer)
with "display: flex; gap: 16px;" and apply it via className on the div in the
RadioButtonFormGroup component; remove the inline style and ensure the newly
created class is exported/kept local to the module as needed.
Comment on lines
+21
to
+35
| const showError = error && isTouched; | ||
|
|
||
| return ( | ||
| <FormGroup label={label} isRequired={isRequired} fieldId={id} {...props}> | ||
| <TextArea | ||
| id={id} | ||
| placeholder={placeholder} | ||
| isRequired={isRequired} | ||
| validated={ | ||
| showError ? ValidatedOptions.error : ValidatedOptions.default | ||
| } | ||
| {...methods.register(name)} | ||
| /> | ||
| <FormErrorMessage error={showError ? error : undefined} /> | ||
| </FormGroup> |
There was a problem hiding this comment.
Show submit-time errors for untouched fields.
showError = error && isTouched hides existing errors until blur. This can leave submit failures with no visible field feedback.
Suggested fix
const isTouched = methods.formState.touchedFields[name] as
| boolean
| undefined;
+ const isSubmitted = methods.formState.isSubmitted;
- const showError = error && isTouched;
+ const showError = !!error && (isTouched || isSubmitted);📝 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
| const showError = error && isTouched; | |
| return ( | |
| <FormGroup label={label} isRequired={isRequired} fieldId={id} {...props}> | |
| <TextArea | |
| id={id} | |
| placeholder={placeholder} | |
| isRequired={isRequired} | |
| validated={ | |
| showError ? ValidatedOptions.error : ValidatedOptions.default | |
| } | |
| {...methods.register(name)} | |
| /> | |
| <FormErrorMessage error={showError ? error : undefined} /> | |
| </FormGroup> | |
| const isTouched = methods.formState.touchedFields[name] as | |
| | boolean | |
| | undefined; | |
| const isSubmitted = methods.formState.isSubmitted; | |
| const showError = !!error && (isTouched || isSubmitted); | |
| return ( | |
| <FormGroup label={label} isRequired={isRequired} fieldId={id} {...props}> | |
| <TextArea | |
| id={id} | |
| placeholder={placeholder} | |
| isRequired={isRequired} | |
| validated={ | |
| showError ? ValidatedOptions.error : ValidatedOptions.default | |
| } | |
| {...methods.register(name)} | |
| /> | |
| <FormErrorMessage error={showError ? error : undefined} /> | |
| </FormGroup> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/core/components/form/TextAreaFormGroup.tsx` around lines 21 - 35, The
current showError logic (showError = error && isTouched) suppresses validation
messages on submit for untouched fields; change showError to show when there is
an error AND (the field was touched OR the form has been submitted) by using
methods.formState.isSubmitted (e.g., showError = error && (isTouched ||
methods.formState.isSubmitted)); update the references to showError used for the
TextArea validated prop and FormErrorMessage so submit-time errors are displayed
while preserving blur-based behavior.
Comment on lines
+285
to
+336
| it("enableProxy is set to false but proxy value are conserved", async () => { | ||
| const mockOnSubmit = vi.fn(); | ||
| const user = userEvent.setup(); | ||
|
|
||
| const environment: Environment = { | ||
| name: "existing-environment", | ||
| defaultGateway: "192.168.1.1", | ||
| dns: "8.8.8.8", | ||
| enableProxy: true, | ||
| httpProxy: "http://proxy.example.com:8080", | ||
| httpsProxy: "https://proxy.example.com:8443", | ||
| noProxy: "localhost,127.0.0.1", | ||
| ipAddress: "192.168.1.100", | ||
| networkConfigType: "static", | ||
| sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC user@host", | ||
| subnetMask: "22", | ||
| }; | ||
|
|
||
| const { getByRole, getByLabelText } = render( | ||
| <> | ||
| <EnvironmentForm | ||
| id="edit-environment-form" | ||
| environment={environment} | ||
| onSubmit={mockOnSubmit} | ||
| /> | ||
| <Button variant="primary" type="submit" form="edit-environment-form"> | ||
| Update | ||
| </Button> | ||
| </>, | ||
| ); | ||
|
|
||
| const name = getByRole("textbox", { name: /Name/i }); | ||
| expect(name).toHaveValue("existing-environment"); | ||
|
|
||
| const enableProxyCheckbox = getByLabelText(/Enable proxy/i); | ||
| expect(enableProxyCheckbox).toBeChecked(); | ||
|
|
||
| await userEvent.click(enableProxyCheckbox); | ||
|
|
||
| const updateButton = getByRole("button", { name: /Update/i }); | ||
| await user.click(updateButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockOnSubmit.mock.calls.length).toBe(1); | ||
| expect(mockOnSubmit.mock.calls[0][0]).toMatchObject({ | ||
| enableProxy: false, | ||
| httpProxy: "http://proxy.example.com:8080", | ||
| httpsProxy: "https://proxy.example.com:8443", | ||
| noProxy: "localhost,127.0.0.1", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Inconsistent userEvent usage - use the setup instance consistently.
Line 322 uses userEvent.click() directly while line 325 uses user.click() from the setup instance. This inconsistency could cause timing issues since the setup instance provides proper event sequencing.
🐛 Proposed fix
- await userEvent.click(enableProxyCheckbox);
+ await user.click(enableProxyCheckbox);📝 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
| it("enableProxy is set to false but proxy value are conserved", async () => { | |
| const mockOnSubmit = vi.fn(); | |
| const user = userEvent.setup(); | |
| const environment: Environment = { | |
| name: "existing-environment", | |
| defaultGateway: "192.168.1.1", | |
| dns: "8.8.8.8", | |
| enableProxy: true, | |
| httpProxy: "http://proxy.example.com:8080", | |
| httpsProxy: "https://proxy.example.com:8443", | |
| noProxy: "localhost,127.0.0.1", | |
| ipAddress: "192.168.1.100", | |
| networkConfigType: "static", | |
| sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC user@host", | |
| subnetMask: "22", | |
| }; | |
| const { getByRole, getByLabelText } = render( | |
| <> | |
| <EnvironmentForm | |
| id="edit-environment-form" | |
| environment={environment} | |
| onSubmit={mockOnSubmit} | |
| /> | |
| <Button variant="primary" type="submit" form="edit-environment-form"> | |
| Update | |
| </Button> | |
| </>, | |
| ); | |
| const name = getByRole("textbox", { name: /Name/i }); | |
| expect(name).toHaveValue("existing-environment"); | |
| const enableProxyCheckbox = getByLabelText(/Enable proxy/i); | |
| expect(enableProxyCheckbox).toBeChecked(); | |
| await userEvent.click(enableProxyCheckbox); | |
| const updateButton = getByRole("button", { name: /Update/i }); | |
| await user.click(updateButton); | |
| await waitFor(() => { | |
| expect(mockOnSubmit.mock.calls.length).toBe(1); | |
| expect(mockOnSubmit.mock.calls[0][0]).toMatchObject({ | |
| enableProxy: false, | |
| httpProxy: "http://proxy.example.com:8080", | |
| httpsProxy: "https://proxy.example.com:8443", | |
| noProxy: "localhost,127.0.0.1", | |
| }); | |
| }); | |
| }); | |
| it("enableProxy is set to false but proxy value are conserved", async () => { | |
| const mockOnSubmit = vi.fn(); | |
| const user = userEvent.setup(); | |
| const environment: Environment = { | |
| name: "existing-environment", | |
| defaultGateway: "192.168.1.1", | |
| dns: "8.8.8.8", | |
| enableProxy: true, | |
| httpProxy: "http://proxy.example.com:8080", | |
| httpsProxy: "https://proxy.example.com:8443", | |
| noProxy: "localhost,127.0.0.1", | |
| ipAddress: "192.168.1.100", | |
| networkConfigType: "static", | |
| sshKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC user@host", | |
| subnetMask: "22", | |
| }; | |
| const { getByRole, getByLabelText } = render( | |
| <> | |
| <EnvironmentForm | |
| id="edit-environment-form" | |
| environment={environment} | |
| onSubmit={mockOnSubmit} | |
| /> | |
| <Button variant="primary" type="submit" form="edit-environment-form"> | |
| Update | |
| </Button> | |
| </>, | |
| ); | |
| const name = getByRole("textbox", { name: /Name/i }); | |
| expect(name).toHaveValue("existing-environment"); | |
| const enableProxyCheckbox = getByLabelText(/Enable proxy/i); | |
| expect(enableProxyCheckbox).toBeChecked(); | |
| await user.click(enableProxyCheckbox); | |
| const updateButton = getByRole("button", { name: /Update/i }); | |
| await user.click(updateButton); | |
| await waitFor(() => { | |
| expect(mockOnSubmit.mock.calls.length).toBe(1); | |
| expect(mockOnSubmit.mock.calls[0][0]).toMatchObject({ | |
| enableProxy: false, | |
| httpProxy: "http://proxy.example.com:8080", | |
| httpsProxy: "https://proxy.example.com:8443", | |
| noProxy: "localhost,127.0.0.1", | |
| }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/environment/components/__tests__/EnvironmentForm.test.tsx` around
lines 285 - 336, The test mixes the global userEvent.click and the setup
instance user; replace the direct call to userEvent.click(enableProxyCheckbox)
with await user.click(enableProxyCheckbox) so the test consistently uses the
userEvent.setup() instance (created as user) and preserves proper event
sequencing for EnvironmentForm interactions.
Comment on lines
+362
to
+377
| aria-describedby="ip-address-helper-text" | ||
| style={{ flex: 1 }} | ||
| /> | ||
| <span>/</span> | ||
| <TextInput | ||
| id="subnet-mask-form-control" | ||
| type="text" | ||
| {...methods.register("subnetMask")} | ||
| placeholder="24" | ||
| isRequired | ||
| validated={ | ||
| methods.formState.errors.subnetMask ? "error" : "default" | ||
| } | ||
| style={{ width: "60px" }} | ||
| aria-describedby="ip-address-helper-text" | ||
| /> |
There was a problem hiding this comment.
Fix aria-describedby reference for subnet mask field.
Both the IP address and subnet mask inputs reference aria-describedby="ip-address-helper-text", but there's no element with that ID. The helper text is rendered conditionally with HelperTextItem components that don't have matching IDs.
♻️ Proposed fix
Either add the referenced ID to the helper text container:
{(methods.formState.errors.ipAddress ||
methods.formState.errors.subnetMask) &&
(methods.formState.isSubmitted ||
methods.formState.touchedFields.ipAddress ||
methods.formState.touchedFields.subnetMask) && (
- <FormHelperText>
+ <FormHelperText id="ip-address-helper-text">
<HelperText>Or remove the aria-describedby attributes if the relationship isn't needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/environment/components/EnvironmentForm.tsx` around lines 362 - 377,
The subnet mask TextInput (id="subnet-mask-form-control", registered via
methods.register("subnetMask") in EnvironmentForm.tsx) currently uses
aria-describedby="ip-address-helper-text" but no element has that ID; either
update the helper text element(s) (the HelperTextItem(s) rendered for the
IP/subnet helper) to include id="ip-address-helper-text" so the input references
a real element, or change/remove the aria-describedby on both the IP address and
subnet mask TextInput controls to either reference the correct helper ID you add
or omit the attribute if no explicit relationship is needed.
Comment on lines
+56
to
+59
| const closeModalAndLoadSources = () => { | ||
| setShouldShowDiscoverySetupModal(false); | ||
| void vm.listSources(); | ||
| }; |
There was a problem hiding this comment.
Keep an explicit create/edit boundary for DiscoverySourceSetupModal.
The modal now derives its initial form values from vm.sourceSelected (src/ui/environment/views/DiscoverySourceSetupModal.tsx:67-78), but these changes never clear that selection for the add flow. After editing any environment, clicking Add environment can reopen the modal prefilled with the previous source instead of a blank create state.
Also applies to: 100-107, 154-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/environment/views/Environment.tsx` around lines 56 - 59, The modal is
reusing vm.sourceSelected so the "Add environment" flow can show previous edits;
ensure a clear create/edit boundary by clearing or resetting vm.sourceSelected
when opening/closing the DiscoverySourceSetupModal for a create flow: update the
open/close handlers (e.g., the function closeModalAndLoadSources and whatever
opens the modal) to call a vm.clearSelectedSource or set vm.sourceSelected =
undefined (or pass an explicit prop like mode="create" to
DiscoverySourceSetupModal) before setShouldShowDiscoverySetupModal(true/false)
and before calling vm.listSources() so the modal initializes with blank values
for creates and retains selection only for edits; also apply same reset in other
related handlers referenced at lines around 100-107 and 154-158.
Comment on lines
297
to
300
| const handleDelete = (source: SourceModel): void => { | ||
| setDeleteTarget(null); | ||
| void vm.deleteAndRefresh(source.id).then((sources) => { | ||
| if (sources?.length) { | ||
| vm.selectSource(sources[0]); | ||
| } | ||
| }); | ||
| void vm.deleteAndRefresh(source.id); | ||
| }; |
There was a problem hiding this comment.
Handle deletion of the currently selected environment.
deleteAndRefresh() in src/ui/environment/view-models/useEnvironmentPageViewModel.ts:336-342 only deletes and reloads. If this row was also vm.sourceSelected, the VM keeps pointing at a deleted model, and later flows that read the shared selection can still reuse that dead id.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/environment/views/SourcesTable.tsx` around lines 297 - 300,
handleDelete currently calls vm.deleteAndRefresh but doesn't clear the shared
selection, leaving vm.sourceSelected pointing at a deleted id; update
handleDelete so that before calling vm.deleteAndRefresh it checks if
vm.sourceSelected?.id === source.id and clears the selection (e.g. call
vm.clearSelection() or set vm.sourceSelected = null), then proceed with
setDeleteTarget(null) and void vm.deleteAndRefresh(source.id) so the view-model
no longer references the deleted SourceModel.
|
PR needs rebase. DetailsInstructions 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 kubernetes-sigs/prow repository. |
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.
Extract EnvironmentForm from DiscoverySourceSetupModal to enable reuse across the application. Add networkConfig helper following proxyConfig pattern for type-safe network configuration extraction. Use react hook form and yup for form and validation.
Summary by CodeRabbit
New Features
Refactor