fix(admin-ui): enable Apply button on field changes in FIDO static co…#2586
fix(admin-ui): enable Apply button on field changes in FIDO static co…#2586
Conversation
…nfiguration form (#2585)
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughFormik initialization/reset and validation were standardized across forms: initialValues were memoized; useEffect hooks trigger formik.resetForm() on prop changes; enableReinitialize removed; cancel now uses formik.resetForm(); modifiedFields handling tightened; SAML validations consolidated into no-spaces and URL helpers; locale validation keys split into three new keys. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as "Formik Form"
participant Validator as "Yup Validator"
participant Backend as "Submit Handler / API"
User->>Form: change fields / click submit
Form->>Validator: validateForm() (await)
alt validation errors
Validator-->>Form: errors
Form->>Form: setNestedObjectValues(touched=true)
Form-->>User: display errors (touched/submitCount logic)
else no errors
Form->>Backend: handleSubmit() (proceed)
Backend-->>Form: response
Form-->>User: success / UI update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
mo-auto
added
comp-admin-ui
![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
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/fido/components/DynamicConfiguration.tsx`:
- Around line 38-47: The useEffect in DynamicConfiguration that calls
formik.resetForm(...) intentionally omits formik from the dependency array to
avoid an infinite reset loop; add an ESLint disable comment to document and
suppress the react-hooks/exhaustive-deps warning (e.g. a single-line //
eslint-disable-next-line react-hooks/exhaustive-deps immediately above the
useEffect or its dependency array) and include a brief inline comment explaining
that formik is intentionally excluded to prevent repeated resets when formik has
a new reference each render; reference the useEffect that checks
fidoConfiguration and calls transformToFormValues(...) and formik.resetForm(...)
when making the change.
- Around line 26-32: The code calls transformToFormValues on every render to
compute initialValues even though useFormik only consumes initialValues once;
wrap the calculation in useMemo to memoize initialValues based on its real
dependencies (e.g., fidoConfiguration and fidoConstants.DYNAMIC) so
transformToFormValues is only recomputed when those inputs change—update the
declaration of initialValues (used with DynamicConfigFormValues) to use useMemo
and keep formik instantiation (useFormik) unchanged.
In `@admin-ui/plugins/fido/components/StaticConfiguration.tsx`:
- Around line 57-66: The effect unconditionally calls formik.resetForm when
fidoConfiguration changes, which can clobber in-progress edits; update the
useEffect that references fidoConfiguration?.fido2Configuration and
formik.resetForm so it first checks formik.dirty (or compares current form
values to the transformed new values) and only calls formik.resetForm when the
user has no unsaved changes or the incoming values actually differ; also include
the full formik object in the dependency array (i.e. depend on formik and
fidoConfiguration) and keep using transformToFormValues, StaticConfigFormValues
and fidoConstants.STATIC to compute the candidate values before deciding to
reset.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/plugins/fido/components/DynamicConfiguration.tsxadmin-ui/plugins/fido/components/StaticConfiguration.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:112-172
Timestamp: 2026-01-07T09:26:24.613Z
Learning: In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management.
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.713Z
Learning: In the GluuFederation/flex admin-ui codebase, `SetTitle` (imported from 'Utils/SetTitle') is a custom React hook that should be called at the component's top level, not inside `useEffect` or other hooks, because it internally manages side effects for setting page titles.
📚 Learning: 2026-01-07T09:26:24.613Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:112-172
Timestamp: 2026-01-07T09:26:24.613Z
Learning: In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management.
Applied to files:
admin-ui/plugins/fido/components/StaticConfiguration.tsxadmin-ui/plugins/fido/components/DynamicConfiguration.tsx
📚 Learning: 2025-12-05T18:24:41.713Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.713Z
Learning: In the GluuFederation/flex admin-ui codebase, `SetTitle` (imported from 'Utils/SetTitle') is a custom React hook that should be called at the component's top level, not inside `useEffect` or other hooks, because it internally manages side effects for setting page titles.
Applied to files:
admin-ui/plugins/fido/components/StaticConfiguration.tsxadmin-ui/plugins/fido/components/DynamicConfiguration.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/fido/components/StaticConfiguration.tsx (3)
admin-ui/plugins/fido/types/fido.ts (1)
StaticConfigFormValues(24-39)admin-ui/plugins/fido/helper/constants.ts (1)
fidoConstants(1-103)admin-ui/plugins/fido/helper/validations.ts (1)
validationSchema(64-70)
🔇 Additional comments (5)
admin-ui/plugins/fido/components/DynamicConfiguration.tsx (2)
1-1: LGTM!The
useEffectimport is correctly added to support the new form state synchronization logic.
60-62: LGTM!The simplified
handleCancelcorrectly relies on the useEffect having updated Formik's internal initial values viaresetForm({ values: ... }). Subsequent calls toresetForm()without arguments will reset to those updated values. Includingformikin the dependency array follows the recommended pattern from learnings.admin-ui/plugins/fido/components/StaticConfiguration.tsx (3)
45-52: Good extraction ofinitialValues.Clearer initialization and keeps Formik config tidy.
79-81: Cancel now reliably resets state.
formik.resetForm()is the right consolidation after removing manual recomputation.
372-373: Touch handling update looks fine.Disabling validation on touch here is consistent with deferring validation to value changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
- SAML>>Identity Brokering,
Applybutton is not activating. - SAML>>Website SSO,
Applybutton is not activating. - Users: The Apply button is not working as expected. When creating a new user, the Apply button becomes active after entering only the first name. It allows submission even when other required fields are not filled.
…ex into admin-ui-issue-2585
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/user-management/components/UserForm.tsx (1)
175-257: Booleanfalsevalues are pruned frommodifiedFields, causing the form to reset when setting a boolean tofalseis the user's last change.
isEmptyValuetreatsfalseas empty (line 179:return value === false), so whenupdateModifiedFieldsis called withfalse, the field is removed frommodifiedFields. If no other fields have pending modifications, the condition at line 189 (Object.keys(rest).length === 0) triggersresetForm, reverting the form to initial values and undoing the user's intentionalfalseassignment.The same issue occurs in
setModifiedFieldsWrapper(line 243), which filters out empty values before resetting the form.Fix: Boolean
falseis a valid value and should never be treated as empty.- if (typeof value === 'boolean') return value === false + if (typeof value === 'boolean') return false
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/user-management/utils/formValidationUtils.ts`:
- Around line 12-14: The submit handler handleApply currently allows submission
when there are modified fields even if the form is invalid; align its
early-return logic with shouldDisableApplyButton by rejecting submission
whenever isSubmitting OR !isFormChanged OR !isValid. Update handleApply to check
all three flags (isSubmitting, isFormChanged, isValid) and return/abort when any
indicate the form should be disabled, mirroring the behavior of
shouldDisableApplyButton to prevent Enter-key bypass.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/user-management/components/UserForm.tsx (1)
116-128: Unnecessary dependency inhandleCancel.
selectedClaimsis included in the dependency array but is not used within the callback body. This causes unnecessary callback recreation when claims change.♻️ Remove unused dependency
- }, [formik, userDetails, memoizedPersonAttributes, selectedClaims, setSelectedClaims]) + }, [formik, userDetails, memoizedPersonAttributes, setSelectedClaims])
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/user-management/components/UserForm.tsx`:
- Around line 171-177: The isEmptyValue helper currently treats boolean false as
empty which causes unchecked checkboxes to be dropped from modifiedFields;
update the useCallback for isEmptyValue so it does NOT treat booleans as empty
(remove or adjust the "if (typeof value === 'boolean') return value === false"
branch) — only null/undefined, empty strings, and empty arrays should be
considered empty; keep the function name isEmptyValue and ensure callers that
rely on it (e.g., the modifiedFields logic) will now detect false as a real
change.
- Around line 225-253: The setModifiedFieldsWrapper function duplicates the
requestAnimationFrame + formik.resetForm reset pattern from updateModifiedFields
and uses a redundant condition; extract the shared reset logic into a helper
(e.g., resetFormToInitialCustomAttributes) that calls
initializeCustomAttributes(userDetails || null, memoizedPersonAttributes) and
invokes requestAnimationFrame(() => formik.resetForm({ values })) to keep
behavior consistent, then simplify setModifiedFieldsWrapper to remove the
redundant check (drop Object.keys(cleanedFields).length === 0 and only check
!hasNonEmptyFields) and call the new helper when no non-empty fields remain;
reference setModifiedFieldsWrapper, updateModifiedFields,
initializeCustomAttributes, isEmptyValue, and formik.resetForm when making the
change.
- Around line 179-204: The setModifiedFields updater in updateModifiedFields
currently schedules a side-effect (requestAnimationFrame -> formik.resetForm)
inside the state updater which can race with rapid edits; move that reset logic
out of the state updater and into a useEffect that watches modifiedFields (e.g.,
when Object.keys(modifiedFields).length === 0) and then calls formik.resetForm
with initializeCustomAttributes(userDetails, memoizedPersonAttributes) as
needed; also remove the requestAnimationFrame and instead perform the reset
synchronously in the effect, and reconsider whether you should reset the entire
form or only clear modified markers when modifiedFields becomes empty.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/user-management/components/UserForm.tsx (1)
146-166: Clarify:selectedClaimsin dependency array may cause extra effect triggers.Including
selectedClaimsin the dependency array means the effect re-runs when claims change, but the ref guard at line 150 blocks re-execution after initialization. This is functionally safe, but you might consider removingselectedClaimsfrom dependencies since the ref check already handles initialization, avoiding unnecessary effect triggers.♻️ Optional simplification
- }, [userDetails?.inum, personAttributesKey, memoizedPersonAttributes, selectedClaims, setSelectedClaims]) + }, [userDetails?.inum, personAttributesKey, memoizedPersonAttributes])
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx`:
- Around line 272-289: The current manual dot-splitting after calling
formik.validateForm() fails to mark nested error fields (like
samlMetadata.entityId) as touched; replace that block by using Formik's
setNestedObjectValues to convert the nested errors into a touched structure and
pass it to formik.setTouched. Concretely, after const errors = await
formik.validateForm(), call formik.setTouched(setNestedObjectValues(errors,
true), false) (referencing validateForm, setTouched, and setNestedObjectValues)
and remove the manual Object.keys/... dot-splitting logic so nested samlMetadata
fields are correctly flagged as touched.
In `@admin-ui/plugins/saml/helper/validations.ts`:
- Around line 106-121: The Yup.when usage on idpEntityId with
metaDataFileImportedFlag must use the callback signature to satisfy Biome lint;
change the current object-form when('metaDataFileImportedFlag', { then: ...,
otherwise: ... }) to the callback form when('metaDataFileImportedFlag',
([value], schema) => value === false ? schema.concat(noSpacesValidation(t,
'fields.idp_entity_id')).required(`${t('fields.idp_entity_id')} is
Required!`).test('not-empty', t('errors.cannot_be_empty', { field:
t('fields.idp_entity_id') }), v => { if (!v) return true; return v.trim().length
> 0; }) : schema) so the first parameter is destructured as ([value], schema)
per Yup API and preserve noSpacesValidation, required and test logic, and leave
the otherwise branch returning schema.
♻️ Duplicate comments (1)
admin-ui/plugins/fido/components/DynamicConfiguration.tsx (1)
39-48: Add ESLint disable comment to suppress the exhaustive-deps warning.The
formikobject is referenced inside the effect but intentionally excluded from the dependency array (including it would cause infinite re-renders sinceformikis a new reference each render). This will trigger anreact-hooks/exhaustive-depsESLint warning.♻️ Proposed fix
useEffect(() => { if (fidoConfiguration) { formik.resetForm({ values: transformToFormValues( fidoConfiguration, fidoConstants.DYNAMIC, ) as DynamicConfigFormValues, }) } + // eslint-disable-next-line react-hooks/exhaustive-deps -- formik intentionally excluded to prevent infinite re-renders }, [fidoConfiguration])
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/user-management/components/UserForm.tsx (1)
217-234: Fix boolean field value tracking in removeSelectedClaimsFromState.Line 230 passes an empty string to
updateModifiedFieldsfor all non-multiValued fields, but for boolean fields (line 225-226), Formik is set tofalse. SinceisEmptyValue('')returnstrue, the field is removed frommodifiedFieldsinstead of being tracked with thefalsevalue. Line 230 should be updated to:updateModifiedFields(id, isMultiValued ? [] : isBoolean ? false : '')This ensures the boolean
falsevalue is properly recorded inmodifiedFieldsto match what's set in Formik.
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/user-management/components/UserForm.tsx`:
- Line 64: The options object is recreated each render which can break
referential equality; change the declaration of options (typed
Partial<GetAttributesParams>) to a memoized value using React's useMemo (e.g.,
useMemo(() => ({} as Partial<GetAttributesParams>), [])) so AvailableClaimsPanel
and any dependency arrays receive a stable reference; update imports if needed
to ensure useMemo is available in UserForm.tsx.
- Around line 166-172: The effect that calls setupCustomAttributes currently
includes selectedClaims in its dependency array which can cause confusing
re-runs; remove selectedClaims from the dependencies and leave the intended
dependencies (userDetails?.inum, personAttributesKey, memoizedPersonAttributes,
setSelectedClaims) so initialization is driven only by those values and
controlled by initializedRef; if your linter complains about the change, add a
focused disable comment for react-hooks/exhaustive-deps right above the
useEffect to document why selectedClaims is intentionally omitted.
♻️ Duplicate comments (1)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
272-291: UsesetNestedObjectValuesto correctly mark nested fields as touched.The manual dot-splitting logic assumes error keys are flattened strings like
"samlMetadata.entityId", but Formik'svalidateForm()returns nested objects (e.g.,{ samlMetadata: { entityId: "error" } }). Thekey.includes('.')check will never match, leaving nestedsamlMetadata.*validation errors invisible.🔧 Suggested fix
Update the import:
-import { useFormik } from 'formik' +import { useFormik, setNestedObjectValues } from 'formik'Replace the manual touched construction:
const errors = await formik.validateForm() if (Object.keys(errors).length > 0) { - const touched: Record<string, boolean | Record<string, boolean>> = {} - Object.keys(errors).forEach((key) => { - if (key.includes('.')) { - const [parent, child] = key.split('.') - if (!touched[parent] || typeof touched[parent] === 'boolean') { - touched[parent] = {} - } - const parentTouched = touched[parent] - if (typeof parentTouched === 'object' && !Array.isArray(parentTouched)) { - parentTouched[child] = true - } - } else { - touched[key] = true - } - }) - formik.setTouched(touched, false) + formik.setTouched(setNestedObjectValues(errors, true), false) return }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
582-593: Validation bypass on Apply button click.The
onApply={toggle}directly opens the commit dialog without callinghandleFormSubmit, which contains the pre-submit validation logic (lines 261-276). ThedisableApply={!formik.isValid || !formik.dirty}relies on Formik's internal validation state.This works because
validateOnChange: truekeepsformik.isValidupdated. However, the file metadata check (lines 261-269) inhandleFormSubmitwon't execute until form submission, potentially allowing the dialog to open when no metadata file is provided.Consider calling validation explicitly in
onApplyor moving the file check to the schema:🔧 Suggested fix
- onApply={toggle} + onApply={async () => { + if (formik.values.spMetaDataSourceType?.toLowerCase() === 'file') { + const hasMetadata = + formik.values.metaDataFile || + formik.values.metaDataFileImportedFlag || + formik.values.spMetaDataFN + if (!hasMetadata) { + formik.setFieldTouched('metaDataFile', true) + return + } + } + const errors = await formik.validateForm() + if (Object.keys(errors).length > 0) { + formik.setTouched(setNestedObjectValues(errors, true), false) + return + } + toggle() + }}admin-ui/plugins/saml/helper/validations.ts (1)
134-140: Remove the$prefix from the field reference inrequiredWhenManual.The
$prefix in Yup is used only for context variables, not field references. In nested objects, sibling or parent fields are referenced by their plain field name.Line 155 correctly uses
when('spMetaDataSourceType', ...)while line 136 incorrectly useswhen('$spMetaDataSourceType', ...). This mismatch prevents the conditional validation from triggering—notice the form manually checksformik.values.spMetaDataSourceType?.toLowerCase() === 'manual'in the UI (lines 474–476, 498–500) to set the required attribute, suggesting the Yup validation isn't working.Fix
const requiredWhenManual = (t: TFunction, fieldKey: string) => - Yup.string().when('$spMetaDataSourceType', { + Yup.string().when('spMetaDataSourceType', { is: (val: string) => val === 'manual', then: (s) => s.required(`${t(fieldKey)} is Required!`), otherwise: (s) => s, })
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx`:
- Around line 297-302: The showError logic for the name field (the showError
prop using formik.errors.name && (formik.touched.name || formik.submitCount > 0
|| (formik.values.name && formik.values.name.length > 0))) is overly eager
because it treats any typed value as a trigger; remove the third clause
((formik.values.name && formik.values.name.length > 0)) so showError becomes
driven only by formik.touched.name or formik.submitCount, and apply the same
change to the other fields that repeat this pattern to keep UX consistent with
other Admin UI forms; keep validateOnChange behavior as-is if you want
change-time validation but without instant error display while typing.
|
4 participants
fix(admin-ui): enable Apply button on field changes in FIDO static configuration form (#2585)
Summary
The Apply button on the FIDO page remained disabled even after modifying form fields under the Static Configuration tab. This prevented users from saving any changes and blocked configuration updates.
Root Cause
Fix Details
Result
Additional Notes
This fix aligns with the ongoing effort to standardize form behavior and button interactions across the Admin UI.
🔗 Ticket
Closes: #2585
Summary by CodeRabbit
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.