feat(admin): adding uniformity in cancel and back buttons present in SCIM & Jans Lock#2404
feat(admin): adding uniformity in cancel and back buttons present in SCIM & Jans Lock#2404
Conversation
📝 WalkthroughWalkthroughAdds a missing "back" translation across locales; introduces typed storage and form utilities; strengthens GluuTypeAhead typing and Formik integration; adds a reusable GluuFormFooter and converts multiple forms to patch-based dirty/validation checks; adds SCIM schema and various component refactors (Jans Lock, Custom Script handling). Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant GT as GluuTypeAhead
participant F as Formik
participant TA as TypeaheadUI
U->>GT: selects option
GT->>GT: handleChange
alt formik provided
GT->>F: setFieldValue(name, value)
end
GT->>TA: update selected prop
GT->>U: call onChange (if provided)
sequenceDiagram
participant U as User
participant P as Config Page
participant F as Formik
participant Footer as GluuFormFooter
participant API as API
P->>F: initialize with configuration
P->>P: compute patches (createJsonPatchFromDifferences)
P->>P: derive isFormDirty / isFormValid
U->>F: edit fields
F->>P: re-evaluate dirty/valid
alt Click Cancel
U->>Footer: Cancel
Footer->>P: handleCancel -> formik.resetForm()
else Click Apply
U->>Footer: Apply
Footer->>F: trigger submit
F->>P: onSubmit -> API call (with precomputed patches)
API->>P: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Points requiring attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-03T08:47:33.926ZApplied to files:
🧬 Code graph analysis (1)admin-ui/app/routes/Apps/Gluu/GluuFormFooter.tsx (1)
🔇 Additional comments (1)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx (1)
49-71: Avoid undefined options causing runtime errors; sync once.Default options to [] to prevent .map on undefined and keep effect resilient.
- const [properties, setProperties] = useState(options) + const [properties, setProperties] = useState<Property[]>(options || []) @@ - useEffect(() => { - setProperties(options) - }, [options]) + useEffect(() => { + setProperties(options || []) + }, [options])admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (2)
43-51: Script path toggle ignores persisted locationType
scriptPathnow derives solely fromitem.moduleProperties.location_type. Many existing scripts persistlocationTypeonly on the top-level field. In that common case this hook initializes tofalse, so the File path input never renders, validation no longer runs, and the user cannot review or edit the stored path. Please fall back toitem.locationType(or the Formik field) when the module property is absent.
417-424: Location type select no longer reflects saved valueThe select’s
defaultValuenow readsgetModuleProperty('location_type'), ignoringitem.locationType/formik.values.location_type. When the module property is missing (most existing records), the dropdown renders blank and immediately marks the form dirty on first render, enabling Cancel/Apply incorrectly. Bind the control to the actual form value (with a fallback toitem.locationType) so persisted selections remain visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (24)
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/GluuCommitFooter.tsx(2 hunks)admin-ui/app/routes/Apps/Gluu/GluuDialog.tsx(2 hunks)admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx(3 hunks)admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx(5 hunks)admin-ui/app/utils/Util.ts(1 hunks)admin-ui/app/utils/formUtils.ts(1 hunks)admin-ui/app/utils/storageUtils.ts(1 hunks)admin-ui/package.json(1 hunks)admin-ui/plugins/admin/components/CustomScripts/CustomScriptAddPage.tsx(1 hunks)admin-ui/plugins/admin/components/CustomScripts/CustomScriptEditPage.tsx(1 hunks)admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx(10 hunks)admin-ui/plugins/admin/components/CustomScripts/types/forms.ts(1 hunks)admin-ui/plugins/admin/components/Mapping/MappingItem.js(1 hunks)admin-ui/plugins/admin/redux/features/customScriptSlice.ts(2 hunks)admin-ui/plugins/fido/components/DynamicConfiguration.tsx(1 hunks)admin-ui/plugins/fido/components/StaticConfiguration.tsx(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)admin-ui/plugins/scim/helper/validations.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
admin-ui/plugins/fido/components/DynamicConfiguration.tsx (1)
admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.js (1)
isSubmitting(48-48)
admin-ui/plugins/admin/components/Mapping/MappingItem.js (3)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (1)
initialValues(84-87)admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.js (1)
initialValues(55-64)admin-ui/plugins/auth-server/components/AuthN/AuthNForm.js (1)
initialValues(21-42)
admin-ui/plugins/scim/helper/schema.ts (2)
admin-ui/plugins/scim/helper/validations.ts (1)
scimConfigurationSchema(1-1)admin-ui/plugins/scim/helper/constants.ts (1)
PROTECTION_MODES(3-3)
admin-ui/plugins/admin/components/CustomScripts/types/forms.ts (2)
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/plugins/scim/components/ScimConfiguration.tsx (3)
admin-ui/plugins/scim/helper/schema.ts (1)
scimConfigurationSchema(19-66)admin-ui/plugins/scim/helper/utils.ts (1)
createJsonPatchFromDifferences(45-142)admin-ui/plugins/scim/types/index.ts (1)
ScimFormValues(3-24)
admin-ui/app/routes/Apps/Gluu/GluuCommitFooter.tsx (3)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (3)
useTranslation(36-36)theme(38-38)selectedTheme(39-39)admin-ui/plugins/services/Components/Configuration/SqlListPage.js (4)
useTranslation(54-54)theme(66-66)selectedTheme(67-67)navigate(57-57)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (3)
admin-ui/plugins/admin/components/CustomScripts/types/customScript.ts (3)
ModuleProperty(3-10)ConfigurationProperty(12-18)CustomScriptItem(29-49)admin-ui/app/utils/Util.ts (2)
filterEmptyObjects(80-82)mapPropertyToKeyValue(84-94)admin-ui/plugins/admin/components/CustomScripts/types/forms.ts (1)
FormValues(15-29)
🔇 Additional comments (27)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptAddPage.tsx (1)
9-9: LGTM! Clean removal of unused imports.The import cleanup successfully removes
applicationStyleandCustomScriptReducerStatewhich are not referenced in this file. All remaining type imports (CustomScriptItem, RootState, UserAction, SubmitData) are actively used throughout the component.admin-ui/package.json (1)
36-38: New npm scripts are production-ready and properly configured.The three new scripts follow npm conventions and integrate seamlessly with the existing codebase:
lint:check: read-only ESLint validation paired with the existinglintscript's--fixtype-check: TypeScript type validation using the well-configured admin-ui/tsconfig.jsoncheck:all: sequential composition for comprehensive validationAll required dependencies (eslint, typescript, TypeScript ESLint plugins, and typescript-plugin-css-modules) are present, and tsconfig.json properly supports the lint and type-check targets. The scripts align with the PR's emphasis on code quality and form control consistency.
admin-ui/plugins/scim/helper/validations.ts (1)
1-1: LGTM!Clean refactor that maintains backward compatibility while consolidating schema definitions.
admin-ui/plugins/scim/helper/index.ts (1)
3-4: LGTM!Appropriate expansion of the public API to include schema and validation exports.
admin-ui/plugins/scim/components/ScimConfiguration.tsx (2)
31-49: LGTM!The form dirty and validity tracking logic is well-implemented with proper memoization. The dirty check correctly excludes
action_messageand uses JSON patch differences to detect changes.
59-61: LGTM!Cancel handler correctly resets the form to initial values.
admin-ui/plugins/admin/components/CustomScripts/types/forms.ts (2)
31-34: LGTM! Clean utility type definition.The
PropertyOptionMaptype provides a clear contract for transforming property arrays into option arrays. The signature correctly allows for undefined input while guaranteeing an array output (implementations should return an empty array for undefined).
21-21: No verification issues found - the optional field changes are correctly implemented.Making
scriptandlocationPathoptional is not a breaking change because:
Localized scope:
FormValuesis only used internally inCustomScriptForm.tsx— no external code depends on these fields.Proper handling: The code already safely manages these optional fields:
- Conditional rendering at line 519 prevents the script editor from displaying when inappropriate
- Error handling includes proper guards (line 532:
formik.errors.script && formik.touched.script)- Business logic explicitly sets one field to
undefinedbased onlocationType(lines 127-131)Intentional design: The optionality reflects the actual runtime behavior — either
scriptorlocationPathis used depending on whether the script source is database-driven or file-based, not both.All existing accesses already include appropriate safeguards.
admin-ui/app/locales/fr/translation.json (1)
134-134: LGTM! Translation is correct.The French translation "Retour" for "back" is accurate and properly placed under the actions section.
admin-ui/plugins/fido/components/StaticConfiguration.tsx (1)
351-351: Good UX improvement!Changing from
disabledtoisLoadingprovides better visual feedback to users during form submission by showing a loading indicator instead of just disabling the button.admin-ui/app/locales/es/translation.json (1)
23-23: LGTM! Translation is correct.The Spanish translation "Atrás" for "back" is accurate and properly placed under the actions section.
admin-ui/plugins/admin/redux/features/customScriptSlice.ts (2)
57-57: Good state management improvement.Incrementing
totalItemswhen a script is successfully added keeps the count accurate and in sync with the actual data.
97-97: Good defensive programming.Using
Math.max(0, state.totalItems - 1)ensures the count never goes negative, which is a good safeguard.admin-ui/plugins/admin/components/CustomScripts/CustomScriptEditPage.tsx (1)
11-11: Good code cleanup.Removing unused imports keeps the dependencies minimal and the code cleaner.
admin-ui/app/locales/pt/translation.json (1)
132-132: PT actions.back added — OK.Label “Voltar” is appropriate and consistent with EN.
admin-ui/plugins/admin/components/Mapping/MappingItem.js (2)
103-103: Formik initialValues for Typeahead — correct.Initializing mappingAddPermissions as [] aligns with GluuTypeAhead’s Formik sync and multiple selection.
191-207: Removed forced value prop — good integration with Formik.Letting GluuTypeAhead derive selected from formik is correct. The explicit ref clear after resetForm may be redundant but harmless.
Confirm the addPermissionsToRole API accepts an array of strings; if objects are ever used as options, pass labelKey and onChange mappers accordingly.
admin-ui/app/routes/Apps/Gluu/GluuDialog.tsx (2)
44-50: Reset message on open — good UX.Prevents stale text when confirming different items.
75-75: Deletion header fallback — robust.Prefers row.name and appends inum/id when present; reads well.
admin-ui/app/utils/Util.ts (1)
80-82: filterEmptyObjects — concise and safe.Correctly drops nullish/empty objects.
admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx (3)
73-91: Formik synchronization and controlled selected — solid.selected derives from value or formik; onChange updates formik then invokes onChange. This is the right pattern.
Also applies to: 118-126
66-70: Confirm or changemultipledefault to preserve single-select behavior for existing callers.The default
multiple = trueis a breaking change. Of the many GluuTypeAhead usages across the codebase, only one explicitly setsmultiple={false}(AssetForm.tsx:190), while at least 10+ usages in SqlForm.js, LdapForm.js, WebhookForm.js, MappingAddDialogForm.js, StaticConfiguration.tsx, and JansLockConfiguration.tsx don't pass the prop at all. This pattern indicates the original default was likelyfalse(single-select), and existing callers depend on that behavior. Changing it totruewill unexpectedly return arrays for fields that should remain single-select, breaking data handling in these forms.Revert to
multiple = falseunless the change was intentional to update all dependent forms.
92-93: The labelKey fallback is safe and properly implemented.Verification confirms all object-based options in the codebase include a
nameproperty when constructed (e.g.,postScripts,updateTokenScriptDns,consentScripts, all map to{ dn: item.dn, name: item.name }). String array options require no labelKey and render directly. No usage violations found.admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx (2)
43-47: State reset on options change — good.Keeps internal state aligned with Formik resets.
72-87: Formik sync paths — clear and correct.Updating valuesOnly when !isKeys && !multiProperties and full array otherwise matches expected payload shapes.
admin-ui/plugins/fido/components/DynamicConfiguration.tsx (1)
311-312: GluuCommitFooter isLoading adoption confirmed — no action needed.Verification shows: (1) the component interface includes
isLoadingas an optional prop, (2) no other GluuCommitFooter usages have adisabledprop, and (3) the DynamicConfiguration.tsx change correctly usesisLoading={isSubmitting}. The adoption is complete and correct.admin-ui/app/locales/en/translation.json (1)
23-23: Locale coverage verified — all supported locales include actions.back.Verification confirms "actions.back" is present in all 4 supported locales (en, pt, fr, es) with proper translations. Code correctly uses
t('actions.back')in GluuCommitFooter.tsx. No gaps found.
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
admin-ui/app/routes/Apps/Gluu/GluuCommitFooter.tsx(2 hunks)admin-ui/app/routes/Apps/Gluu/GluuProperties.tsx(1 hunks)admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx(5 hunks)admin-ui/app/utils/Util.ts(1 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/scim/components/ScimConfiguration.tsx(4 hunks)admin-ui/plugins/scim/helper/schema.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
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)
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/helper/schema.ts (2)
admin-ui/plugins/scim/helper/validations.ts (1)
scimConfigurationSchema(1-1)admin-ui/plugins/scim/helper/constants.ts (1)
PROTECTION_MODES(3-3)
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(80-82)
admin-ui/app/routes/Apps/Gluu/GluuCommitFooter.tsx (3)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (2)
theme(38-38)selectedTheme(39-39)admin-ui/plugins/services/Components/Configuration/LdapListPage.js (3)
theme(57-57)selectedTheme(58-58)navigate(42-42)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)
🔇 Additional comments (3)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (1)
74-85: Based on the Jans API documentation, moduleProperties examples show only value1, value2, and description fields, with nohidefield. In contrast, configurationProperties includes ahidefield with boolean values (e.g., "hide": true).The
transformPropertyForApifunction applies the same logic to both property types, which creates a problem: it defaultshidetofalsefor moduleProperties, but the API schema showshideshould never appear in moduleProperties.Omit
hidefor moduleProperties and conditionally include it only for configurationProperties.Update the function to distinguish between the two property types:
- For
configurationProperties: conditionally includehideonly when defined- For
moduleProperties: never includehideThe description handling should also preserve empty strings if valid.
admin-ui/plugins/scim/helper/schema.ts (1)
1-61: LGTM! Schema implementation is solid.The validation schema is well-structured with appropriate validations for each field:
- URL validation correctly checks for HTTP/HTTPS protocols while treating empty values as valid (optional field)
- Numeric fields have proper type checking, range limits, and user-friendly error messages
- The isSecureUrl function is correctly used directly in the Yup test at line 19, as Yup passes the value as the first parameter to the test function
The schema provides comprehensive validation coverage for the SCIM configuration form.
admin-ui/plugins/scim/components/ScimConfiguration.tsx (1)
81-92: LGTM! Button states correctly implement PR objectives.The button behavior perfectly aligns with the PR requirements:
- Back (line 87): Always enabled (
disableBack={false}) ✓- Cancel (line 88): Disabled until form is modified (
disableCancel={!isFormDirty}) ✓- Apply (line 89): Enabled only when form is valid and has unsaved changes (
disableApply={!isFormValid || !isFormDirty}) ✓The implementation provides clear user feedback about form state and prevents invalid submissions while allowing users to navigate away or reset changes as needed.
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/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (1)
147-151: Inconsistent data source causes filtered properties to be ignored.The filtering at lines 128-133 modifies
item.modulePropertiesto removelocation_path, but lines 147-151 buildsubmitValues.modulePropertiesfromvalues.moduleProperties. WhenObject.assign(item, submitValues)executes at line 159, the unfilteredvalues.modulePropertiesoverwrites the filtereditem.moduleProperties, causinglocation_pathto be sent to the API even whenlocationType === 'db'.Additionally, since the
GluuPropertiescomponent (line 517) updatesformik.values.moduleProperties, usingitem.modulePropertiesfor filtering means user edits are ignored.Apply this diff to filter
values.modulePropertiesconsistently:if (values.moduleProperties) { + let modulePropsToSubmit = values.moduleProperties + if (item.locationType === 'db') { + modulePropsToSubmit = values.moduleProperties.filter( + (moduleItem: ModuleProperty) => moduleItem?.value1 !== 'location_path', + ) + } - submitValues.moduleProperties = filterEmptyObjects(values.moduleProperties).map( + submitValues.moduleProperties = filterEmptyObjects(modulePropsToSubmit).map( transformPropertyForApi, ) }You can also remove the now-redundant filtering at lines 128-133 since the filtering will happen here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx(5 hunks)admin-ui/app/utils/Util.ts(1 hunks)admin-ui/app/utils/storageUtils.ts(1 hunks)admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx(2 hunks)admin-ui/plugins/scim/components/ScimConfiguration.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
admin-ui/plugins/scim/components/ScimConfiguration.tsx (4)
admin-ui/plugins/scim/helper/schema.ts (1)
scimConfigurationSchema(14-61)admin-ui/plugins/scim/helper/validations.ts (1)
scimConfigurationSchema(1-1)admin-ui/plugins/scim/helper/utils.ts (1)
createJsonPatchFromDifferences(45-142)admin-ui/plugins/scim/types/index.ts (1)
ScimFormValues(3-24)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (2)
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)
🔇 Additional comments (10)
admin-ui/plugins/scim/components/ScimConfiguration.tsx (5)
2-7: LGTM! Import changes align with new form state management.The added hooks (useMemo, useCallback) and utilities (createJsonPatchFromDifferences) are used appropriately throughout the component for optimized dirty-checking and memoized handlers.
31-42: Excellent! Dirty-checking logic is robust and addresses previous feedback.The patch-based dirty detection correctly excludes the UI-only
action_messagefield and efficiently compares form state. The switch toeslint-disable-next-line(line 35) properly addresses the previous review comment about thevoid action_messagepattern.
44-49: LGTM! Form validation check is correctly implemented.Using
isValidSyncfor real-time validation is appropriate and efficiently determines whether the Apply button should be enabled.
59-61: LGTM! Cancel handler correctly resets form to initial values.The implementation properly fulfills the PR objective: "Cancel resets all fields to previous values without navigating away."
81-92: Excellent implementation! Footer button behavior perfectly aligns with PR objectives.The button states correctly implement all requirements:
- Back is always enabled (line 87:
disableBack={false}) ✓- Cancel is disabled until changes are made (line 88:
disableCancel={!isFormDirty}) ✓- Apply is enabled only when form is valid and has unsaved changes (line 89) ✓
This addresses the previous review comment about Back button enablement and implements the complete feature as specified.
admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx (2)
92-97: LGTM! Past critical issue resolved.The
resolvedLabelKeylogic correctly returns the string literal'name'as the fallback (line 96), addressing the previous critical comment about blank typeahead labels caused by returning the variablename(which would be the form field name like"scimProperties").
22-45: Interface is well-structured.The
labelKeytyping asstring | ((option: Option) => string)correctly addresses the past refactoring suggestion to match the react-bootstrap-typeahead library's API. The comprehensive prop definitions provide good type safety for consumers.admin-ui/app/utils/Util.ts (1)
90-92: LGTM! Nullish coalescing correctly preserves empty strings.The implementation now uses
??instead of||, which preserves empty strings as valid values (e.g.,{ key: '', value1: 'fallback' }→ key =''). The.trim()call normalizes whitespace without the need for aString()wrapper.admin-ui/app/utils/storageUtils.ts (2)
3-41: LGTM! Correctly handles empty strings and type coercion.The implementation correctly addresses past review concerns:
- Line 10 uses
stored === nullinstead of!stored, allowing empty strings to round-trip properly- Type coercion based on
defaultValuetype (boolean, number, object) works correctly- The special handling of the literal
"null"string (line 17) ensures proper round-trip for null values- Error handling with fallback to
defaultValueis appropriate
43-60: LGTM! Correctly removes key when value is null.The implementation correctly addresses the past review concern by removing the key (line 50) instead of stringifying
nullto"null". This prevents type confusion on subsequent reads.Note: The type
StorageValueexcludesundefined, which prevents accidental storage of the string"undefined". This is good defensive typing.
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (1)
151-155: Location_path filtering is undone before API submission.When
locationType === 'db', lines 132-136 filterlocation_pathfromitem.moduleProperties. However, lines 151-154 read fromvalues.moduleProperties(which hasn't been filtered) and assign the result tosubmitValues.moduleProperties. Subsequently, line 163'sObject.assign(item, submitValues)overwrites the filtereditem.modulePropertieswith the unfiltered content fromsubmitValues.moduleProperties, solocation_pathis still sent to the API.Apply this diff to filter
values.modulePropertiesbefore transformation whenlocationType === 'db':if (values.moduleProperties) { + let modulePropsToSubmit = values.moduleProperties + if (item.locationType === 'db') { + modulePropsToSubmit = modulePropsToSubmit.filter( + (moduleItem: ModuleProperty) => moduleItem?.value1 !== 'location_path', + ) + } - submitValues.moduleProperties = filterEmptyObjects(values.moduleProperties).map( + submitValues.moduleProperties = filterEmptyObjects(modulePropsToSubmit).map( transformPropertyForApi, ) as ModuleProperty[] }Alternatively, you can remove the redundant filtering at lines 132-136 since the filtering would now happen at lines 151-155.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx(5 hunks)admin-ui/app/utils/storageUtils.ts(1 hunks)admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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(80-82)
🔇 Additional comments (7)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptForm.tsx (2)
75-89: LGTM: Metadata preservation now correctly implemented.The
transformPropertyForApifunction now properly preserves optional fields (hideanddescription) when present, addressing the previous concern about forcinghide: falseand dropping metadata.One minor note:
descriptionis only included when truthy (line 85), so an empty string""would be excluded. This is likely intentional to avoid sending empty descriptions to the API.
146-149: LGTM: Configuration properties transformation is correct.The transformation of
configurationPropertiesproperly filters empty objects and applies the correctedtransformPropertyForApifunction with appropriate type casting.admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx (5)
22-45: Well-structured interface with proper typing.The interface comprehensively covers all component props with appropriate types. The
labelKeytyping now correctly supports both string and function forms, addressing the previous review feedback.
73-79: Properly implemented with correct dependencies and type safety.The memo correctly addresses previous feedback:
- Dependencies now track the actual field value (
formik?.values?.[name]) instead of the stable formik reference- Runtime
Array.isArraycheck before type assertion prevents unsafe castsThe implementation ensures
selectedValuerecomputes when the field changes and safely handles both controlled (valueprop) and Formik-managed state.
81-93: Clean implementation with narrowed dependencies.The callback correctly addresses previous feedback by depending on
formik?.setFieldValuerather than the entire formik object. This makes the dependency precise and clarifies that onlysetFieldValueis used.The logic properly handles both Formik-managed and standalone usage patterns.
95-100: Correctly returns the literal 'name' string.This addresses the previous critical issue. The memo now returns the literal string
'name'(line 99) instead of the form field name variable, ensuring the typeahead properly displays each option'snameproperty rather than rendering blank labels.
102-147: Clean integration of memoized values and callbacks.The render logic correctly uses the memoized
resolvedLabelKey,handleChange, andselectedValue, ensuring optimal performance and correct behavior. The component is properly wrapped withReact.memoto prevent unnecessary re-renders.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/app/utils/storageUtils.ts (1)
76-81: Same empty-string issue as the basegetStorageItem.The parser uses
Number(value)which converts''and whitespace to0instead of falling back todefaultValue. Apply the same empty-string check suggested for lines 20-23 to keep the behavior consistent.Apply this diff:
export const getStorageNumber = (key: string, defaultValue: number): number => { return getStorageItem(key, defaultValue, (value) => { + const trimmed = value.trim() + if (trimmed === '') return defaultValue - const parsed = Number(value) + const parsed = Number(trimmed) return isNaN(parsed) ? defaultValue : parsed }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/utils/storageUtils.ts(1 hunks)
🔇 Additional comments (5)
admin-ui/app/utils/storageUtils.ts (5)
40-64: LGTM!The setter correctly handles all edge cases: null values remove the key, objects are JSON-serialized, and custom serializers are supported. Error handling is robust.
66-74: LGTM!Simple, correct wrapper with appropriate error handling and boolean return.
83-85: LGTM!Boolean parsing is consistent with the base function and uses a simple, predictable rule: only the string
'true'parses totrue.
87-96: LGTM!While
getStorageItemalready handles objects via JSON, this specialized helper provides a cleaner, more explicit API for JSON use cases. The error handling and type constraints are appropriate.
98-106: LGTM!Specialized setter provides a cleaner API for JSON values. The error handling is consistent with other functions.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/utils/storageUtils.ts(1 hunks)
🔇 Additional comments (1)
admin-ui/app/utils/storageUtils.ts (1)
43-67: LGTM! Clean serialization logic.The function correctly handles all the edge cases:
nullvalues trigger key removal- Objects are JSON-serialized when no custom serializer is provided
- Custom serializers are respected
- Errors are caught and logged
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/routes/Apps/Gluu/GluuFormFooter.tsx(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 (1)
admin-ui/app/routes/Apps/Gluu/GluuFormFooter.tsx (1)
admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)
🔇 Additional comments (7)
admin-ui/app/routes/Apps/Gluu/GluuFormFooter.tsx (7)
34-44: Well-implemented memoized button label component.The ButtonLabel component effectively handles loading states with a clean conditional icon render and proper displayName for debugging.
48-68: Solid component initialization with proper defaults.The component setup correctly uses React hooks, provides sensible defaults, and safely handles theme context with optional chaining and fallbacks.
70-76: Back navigation correctly implements dashboard-only navigation.The implementation properly navigates to
/home/dashboardas per project requirements, avoiding history-based navigation. Clean callback structure with appropriate dependencies.Based on learnings
146-157: Back button correctly remains enabled during form submission.The Back button's disabled state properly excludes
isLoading(line 153), allowing users to navigate away during form submission. This addresses the previous review feedback.
159-190: Apply button correctly handles both submit and button variants.Both button types properly disable during loading and include appropriate safety checks. The button variant correctly validates
onApplyexistence at line 180 before allowing interaction.
192-203: Cancel button consolidation successfully completed.The Cancel button render has been properly consolidated into a single element with conditional className logic, addressing previous feedback about duplication.
209-212: Proper component memoization and export.The component is correctly memoized with
memoand includes adisplayNamefor better debugging experience in React DevTools.
|
faisalsiddique4400
requested review from
moabu
and removed request for
syntrydy
3 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
New Features
Improvements
Bug Fixes