feat(admin): adding uniformity in cancel and back buttons present in SCIM & Jans Lock#2408
feat(admin): adding uniformity in cancel and back buttons present in SCIM & Jans Lock#2408faisalsiddique4400 wants to merge 32 commits intomainfrom
Conversation
…i-issue-2361-scim
…Jans Lock (#2405) * feat(admin): adding uniformity in cancel and back buttons present in Jans Lock * feat(admin): adding uniformity in cancel and back buttons present in Jans Lock * Code Rabbit fixes
faisalsiddique4400
requested review from
duttarnab and
syntrydy
as code owners
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request introduces multi-language translation strings for a "back" action, adds utility functions for form and storage management, creates a new form footer component with configurable action buttons, and refactors existing form components to integrate enhanced form state tracking and submission handling across the admin UI. Changes
Sequence DiagramsequenceDiagram
participant User
participant JansLockConfiguration
participant GluuFormFooter
participant formik as Formik
participant API as patchMutation
User->>JansLockConfiguration: Render form (isSubmitting prop)
JansLockConfiguration->>JansLockConfiguration: useMemo: compute trimmedValuesAndPatches<br/>(trimmed values + patch ops)
JansLockConfiguration->>JansLockConfiguration: Derive isFormDirty, isFormValid
JansLockConfiguration->>GluuFormFooter: Render footer<br/>(showBack, showCancel, showApply,<br/>isLoading=isSubmitting)
alt User clicks Apply
User->>GluuFormFooter: Click Apply button
GluuFormFooter->>JansLockConfiguration: onApply callback
JansLockConfiguration->>formik: submitForm()
JansLockConfiguration->>JansLockConfiguration: Submission flow using<br/>precomputed patches
JansLockConfiguration->>API: Send PATCH request
API-->>JansLockConfiguration: Response (isPending=true during)
JansLockConfiguration->>JansLockConfiguration: Update isSubmitting state
GluuFormFooter->>GluuFormFooter: Display loading in buttons
else User clicks Cancel
User->>GluuFormFooter: Click Cancel button
GluuFormFooter->>JansLockConfiguration: onCancel callback
JansLockConfiguration->>formik: Reset form
else User clicks Back
User->>GluuFormFooter: Click Back button
GluuFormFooter->>GluuFormFooter: Navigate to /home/dashboard
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
Comment |
mo-auto
added
comp-admin-ui
Signed-off-by: Faisal Siddique <71010439+faisalsiddique4400@users.noreply.github.com>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx (1)
81-93: Fix dependency array for handleChange callback.The dependency
formik?.setFieldValueis problematic in a dependency array. Optional chaining doesn't work as expected in React dependencies—React compares the expression reference, not the resolved value. Ifformikchanges identity butsetFieldValueremains the same, the callback may not update correctly.Apply this diff:
const handleChange = useCallback( (selected: Option[]) => { if (formik) { formik.setFieldValue(name, selected) if (onChange) { onChange(selected) } } else if (onChange) { onChange(selected) } }, - [formik?.setFieldValue, name, onChange], + [formik, name, onChange], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
admin-ui/app/locales/en/translation.json(1 hunks)admin-ui/app/locales/es/translation.json(1 hunks)admin-ui/app/locales/fr/translation.json(1 hunks)admin-ui/app/locales/pt/translation.json(1 hunks)admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx(1 hunks)admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx(3 hunks)admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx(1 hunks)admin-ui/app/utils/Util.ts(2 hunks)admin-ui/app/utils/formUtils.ts(1 hunks)admin-ui/app/utils/storageUtils.ts(1 hunks)admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx(2 hunks)admin-ui/plugins/jans-lock/components/JansLock.tsx(1 hunks)admin-ui/plugins/jans-lock/components/JansLockConfiguration.tsx(6 hunks)admin-ui/plugins/jans-lock/helper/utils.ts(1 hunks)admin-ui/plugins/scim/components/ScimConfiguration.tsx(4 hunks)admin-ui/plugins/scim/helper/index.ts(1 hunks)admin-ui/plugins/scim/helper/schema.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2404
File: admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx:70-77
Timestamp: 2025-11-03T08:47:33.926Z
Learning: In the admin-ui forms (Gluuformfooter component), the Back button should always navigate to '/home/dashboard' rather than using history-based navigation (navigate(-1)), as clarified by faisalsiddique4400 in PR #2404.
📚 Learning: 2025-11-03T08:47:33.926Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2404
File: admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx:70-77
Timestamp: 2025-11-03T08:47:33.926Z
Learning: In the admin-ui forms (Gluuformfooter component), the Back button should always navigate to '/home/dashboard' rather than using history-based navigation (navigate(-1)), as clarified by faisalsiddique4400 in PR #2404.
Applied to files:
admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx
🧬 Code graph analysis (5)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (3)
admin-ui/plugins/admin/redux/features/types/customScript.ts (2)
ModuleProperty(3-8)ConfigurationProperty(10-16)admin-ui/plugins/admin/components/CustomScripts/types/customScript.ts (2)
ModuleProperty(3-10)ConfigurationProperty(12-18)admin-ui/app/utils/Util.ts (1)
filterEmptyObjects(82-84)
admin-ui/plugins/jans-lock/components/JansLockConfiguration.tsx (4)
admin-ui/plugins/jans-lock/types/jans-lock-types.ts (2)
JansLockConfigFormValues(4-26)PatchOperation(38-38)admin-ui/app/utils/Util.ts (1)
trimObjectStrings(66-80)admin-ui/plugins/jans-lock/helper/utils.ts (1)
createPatchOperations(62-126)admin-ui/plugins/jans-lock/helper/validations.ts (1)
validationSchema(3-16)
admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx (1)
admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)
admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx (1)
admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (1)
properties(45-45)
admin-ui/plugins/scim/components/ScimConfiguration.tsx (3)
admin-ui/plugins/scim/helper/schema.ts (1)
scimConfigurationSchema(14-61)admin-ui/plugins/scim/helper/utils.ts (1)
createJsonPatchFromDifferences(45-142)admin-ui/plugins/scim/types/index.ts (1)
ScimFormValues(3-24)
🔇 Additional comments (9)
admin-ui/plugins/jans-lock/helper/utils.ts (1)
107-121: Good defensive guards against empty collections in new fields.The logic correctly prevents adding patch operations when introducing new fields with empty array or empty object values. The implementation properly handles edge cases (null check at line 111, array check at line 112 before object inspection), and correctly preserves empty values in "replace" operations for existing fields where you may legitimately want to clear data.
However, no tests exist in the jans-lock plugin directory to validate this behavior, and there's no available backend API documentation specifying expectations for empty patch operations. While the code change is sound from a JSON Patch perspective, verify with your backend team that the API correctly handles patch operations that omit "add" operations for empty new fields.
admin-ui/app/locales/pt/translation.json (1)
132-132: ✓ Correct locale addition for Portuguese "Back" action.The new
actions.backentry with value "Voltar" is correctly placed, semantically accurate, and consistent with related Portuguese translations like "back_home". This mirrors the same addition across other locales (en, es, fr) as expected.admin-ui/app/locales/fr/translation.json (1)
134-134: ✓ Correct locale addition for French "Back" action.The new
actions.backentry with value "Retour" is correctly positioned, semantically accurate, and consistent with related French translations like "back_home". This mirrors the same addition across other locales (pt, en, es) as expected.admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx (2)
24-24: LGTM: Enhanced type flexibility.The expanded type for
labelKeyappropriately supports both string keys and custom label extraction functions.
95-100: LGTM: Clean fallback logic.The
resolvedLabelKeymemoization correctly handles function/string labelKey types and provides a sensible default.admin-ui/plugins/jans-lock/components/JansLockConfiguration.tsx (4)
2-2: LGTM: Clean prop and import updates.The addition of
useMemo,GluuFormFooterimport, andisSubmittingprop appropriately support the new form state management.Also applies to: 8-8, 23-23, 26-30
74-76: LGTM: Accurate dirty state tracking.Deriving dirty state from patch operations is more accurate than using Formik's built-in
dirtyproperty, as it accounts for trimming and type normalization.
85-102: LGTM: Clean submission flow.Using precomputed patches ensures consistency and avoids recalculation. The silent no-op for unchanged forms is good UX.
432-443: Back button navigation verified as correct.The
GluuFormFootercomponent correctly implements the default navigation behavior. Since noonBackprop is provided, the Back button will navigate to/home/dashboardas expected, consistent with the established pattern.
| const selectedValue = useMemo(() => { | ||
| if (value !== undefined) { | ||
| return value | ||
| } | ||
| const fieldValue = formik?.values?.[name] | ||
| return Array.isArray(fieldValue) ? (fieldValue as Option[]) : [] | ||
| }, [value, formik?.values?.[name], name]) |
There was a problem hiding this comment.
Fix dependency array to ensure proper memoization.
The dependency formik?.values?.[name] won't trigger re-computation correctly because React's dependency comparison doesn't evaluate the optional chain expression—it compares the reference to formik?.values?.[name] itself, not the value. When formik.values changes, this memo may return stale data.
Apply this diff:
const selectedValue = useMemo(() => {
if (value !== undefined) {
return value
}
const fieldValue = formik?.values?.[name]
return Array.isArray(fieldValue) ? (fieldValue as Option[]) : []
- }, [value, formik?.values?.[name], name])
+ }, [value, formik?.values, name])📝 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.
| const selectedValue = useMemo(() => { | |
| if (value !== undefined) { | |
| return value | |
| } | |
| const fieldValue = formik?.values?.[name] | |
| return Array.isArray(fieldValue) ? (fieldValue as Option[]) : [] | |
| }, [value, formik?.values?.[name], name]) | |
| const selectedValue = useMemo(() => { | |
| if (value !== undefined) { | |
| return value | |
| } | |
| const fieldValue = formik?.values?.[name] | |
| return Array.isArray(fieldValue) ? (fieldValue as Option[]) : [] | |
| }, [value, formik?.values, name]) |
🤖 Prompt for AI Agents
In admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx around lines 73–79, the
useMemo dependency list uses the optional-chained expression
formik?.values?.[name], which can miss updates; change the dependencies to
[value, formik?.values, name] (i.e., depend on the whole formik.values object
plus name and value) so the memo recalculates when form values change, and
ensure types remain correct when reading formik.values[name] inside the memo.
| const trimmedValuesAndPatches = useMemo(() => { | ||
| if (!lockConfig || !formik.values) { | ||
| return { | ||
| trimmedValues: null as JansLockConfigFormValues | null, | ||
| patches: [] as PatchOperation[], | ||
| } | ||
| } | ||
| const trimmedValues = trimObjectStrings(formik.values) | ||
| const patches = createPatchOperations(trimmedValues, lockConfig) | ||
| return { trimmedValues, patches } | ||
| }, [lockConfig, formik.values]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider performance impact of computing patches on every change.
The trimmedValuesAndPatches memo recomputes on every formik.values change (i.e., every keystroke). For large forms, createPatchOperations iterates all fields and compares them with the original config, which may impact performance during typing.
If performance degrades, consider:
- Debouncing the patch computation
- Computing patches only when needed (e.g., on blur or submit)
- Using Formik's built-in
dirtystate tracking
For small to medium forms, the current approach is acceptable and provides real-time dirty state tracking.
🤖 Prompt for AI Agents
In admin-ui/plugins/jans-lock/components/JansLockConfiguration.tsx around lines
62 to 72, the memoized trimmedValuesAndPatches recomputes patches on every
formik.values change (every keystroke), which can be expensive; change the
implementation to avoid computing createPatchOperations on every keypress by
either debouncing the patch computation (wrap patch creation in a debounced
effect and update patches state after a short delay), or compute patches only on
demand (on blur or on submit) and rely on Formik.dirty for immediate UI state,
ensuring the memo dependencies are updated to use the debounced value or the
on-demand trigger instead of raw formik.values.
| const isFormValid = useMemo(() => { | ||
| if (!formik.values) { | ||
| return false | ||
| } | ||
| return validationSchema.isValidSync(formik.values) | ||
| }, [formik.values]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using Formik's built-in validation state.
The manual isValidSync call duplicates Formik's built-in validation, which runs automatically when validateOnChange is enabled (the default). Formik already tracks validity in formik.isValid.
Consider replacing with:
- const isFormValid = useMemo(() => {
- if (!formik.values) {
- return false
- }
- return validationSchema.isValidSync(formik.values)
- }, [formik.values])
+ const isFormValid = formik.isValidThis eliminates redundant validation and improves performance. If you need synchronous validation for some reason, the current approach is fine.
🤖 Prompt for AI Agents
In admin-ui/plugins/jans-lock/components/JansLockConfiguration.tsx around lines
78 to 83, the code manually computes form validity with
validationSchema.isValidSync(formik.values), duplicating Formik's built-in
validation; replace this useMemo block with Formik's formik.isValid (or directly
reference formik.isValid where needed) so you rely on Formik's internal
validation lifecycle, remove the synchronous isValidSync call and its useMemo,
and ensure any consumers subscribe to formik.isValid (and formik.dirty/touched
if appropriate) instead.
| formik.setFieldValue('tokenChannels', values) | ||
| }} | ||
| options={lockConfig?.tokenChannels || []} | ||
| options={(lockConfig?.tokenChannels as string[]) || []} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertion assumes API contract.
The cast to string[] assumes lockConfig.tokenChannels is always a string array. If the API response could have a different shape, add a runtime check:
options={Array.isArray(lockConfig?.tokenChannels) ? (lockConfig.tokenChannels as string[]) : []}If the API contract is guaranteed, the current code is fine.
🤖 Prompt for AI Agents
In admin-ui/plugins/jans-lock/components/JansLockConfiguration.tsx around line
132, the code casts lockConfig?.tokenChannels to string[] which assumes the API
always returns that shape; change to a runtime check that uses
Array.isArray(lockConfig?.tokenChannels) and only pass it as options when true,
otherwise pass an empty array, so you avoid unsafe type assertions and handle
unexpected API shapes safely.
| showBack={true} | ||
| showCancel={true} | ||
| showApply={true} | ||
| onApply={toggle} | ||
| onCancel={handleCancel} | ||
| disableBack={false} | ||
| disableCancel={!isFormDirty} | ||
| disableApply={!isFormValid || !isFormDirty} | ||
| applyButtonType="submit" | ||
| isLoading={isSubmitting} |
There was a problem hiding this comment.
Prevent Apply modal from immediately closing
onApply calls toggle() while the button is also type="submit". A click fires the click handler (opening the dialog) and then the form submit, which triggers formik.onSubmit (also toggle). The modal flips twice and ends up closed, so users can’t confirm changes. Please invoke formik.submitForm() from onApply and use a non-submit button to keep the dialog open exactly once.
- onApply={toggle}
- applyButtonType="submit"
+ onApply={formik.submitForm}
+ applyButtonType="button"📝 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.
| showBack={true} | |
| showCancel={true} | |
| showApply={true} | |
| onApply={toggle} | |
| onCancel={handleCancel} | |
| disableBack={false} | |
| disableCancel={!isFormDirty} | |
| disableApply={!isFormValid || !isFormDirty} | |
| applyButtonType="submit" | |
| isLoading={isSubmitting} | |
| showBack={true} | |
| showCancel={true} | |
| showApply={true} | |
| onApply={formik.submitForm} | |
| onCancel={handleCancel} | |
| disableBack={false} | |
| disableCancel={!isFormDirty} | |
| disableApply={!isFormValid || !isFormDirty} | |
| applyButtonType="button" | |
| isLoading={isSubmitting} |
🤖 Prompt for AI Agents
In admin-ui/plugins/scim/components/ScimConfiguration.tsx around lines 82 to 91,
the Apply button is both type="submit" and calling toggle() via onApply which
causes the modal to flip twice; change onApply to call formik.submitForm()
instead of toggle(), and set applyButtonType to a non-submit value (e.g.
"button") so the click only triggers formik.submitForm() and the modal is closed
once by the formik onSubmit/toggle flow; ensure you do not remove the existing
isLoading/disableApply props and keep the actual toggle() call in the form
submit/confirmation handler rather than the button click.
2 participants
feat(admin): adding uniformity in cancel and back buttons present in SCIM & Jank Lock
Summary
This update introduces consistent behavior for Cancel, Back, and Apply buttons across Admin UI forms, including correct enabling and disabling based on form state.
Improvements
Additional Enhancements
Affected Modules
This change is applied across all forms where configuration changes are possible, including but not limited to:
Parent issue: #2268
Testing Steps
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements