fix(admin-ui): incorrect cancel button behaviour in the Application S…#2389
fix(admin-ui): incorrect cancel button behaviour in the Application S…#2389
Conversation
…ettings page in Home module
faisalsiddique4400
requested review from
duttarnab and
syntrydy
as code owners
mo-auto
added
comp-admin-ui
|
The field Custom Parameters (for authentication) is not updating from the backend side. |
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
- the jenkins build is failing could you please check.
📝 WalkthroughWalkthroughReplaces scattered localStorage pagingSize reads with a centralized paging utility; refactors SettingsPage to use Formik with correct cancel/reset and paging persistence; adds disable control for commit footer buttons; renames unused parameters to underscore-prefixed identifiers; exposes putConfigWorker and refines its _meta/toast handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage as SettingsPage (Formik)
participant GluuCommitFooter
participant Redux
participant LocalStorage
User->>SettingsPage: Load page
SettingsPage->>Redux: Fetch config/scripts
SettingsPage->>LocalStorage: getPagingSize()
SettingsPage->>SettingsPage: Initialize Formik (values from config)
User->>SettingsPage: Edit form
SettingsPage->>GluuCommitFooter: Provide disableButtons based on form state
alt Save
User->>GluuCommitFooter: Click Save
GluuCommitFooter->>SettingsPage: Submit
SettingsPage->>SettingsPage: Validate (Yup)
SettingsPage->>Redux: Dispatch putConfigWorker(payload + _meta)
SettingsPage->>LocalStorage: savePagingSize()
Redux->>User: Show toast (uses _meta if provided)
else Cancel
User->>GluuCommitFooter: Click Cancel
GluuCommitFooter->>SettingsPage: Trigger cancel
SettingsPage->>SettingsPage: Reset Formik to config values
SettingsPage->>LocalStorage: restore paging size if needed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)admin-ui/app/redux/sagas/__tests__/AuthSaga.test.ts (2)
admin-ui/app/redux/sagas/AuthSaga.ts (1)
🔇 Additional comments (8)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/app/redux/features/authSlice.ts(3 hunks)admin-ui/app/redux/features/types/authTypes.ts(1 hunks)admin-ui/plugins/admin/components/Settings/SettingsPage.js(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (3)
admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/admin/components/Webhook/ShortcodePopover.js (1)
Label(91-118)admin-ui/app/cedarling/index.ts (1)
CedarlingLogType(5-5)
🔇 Additional comments (8)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (8)
208-223: Formik wiring for cancel/reset looks correct.enableReinitialize with resetForm to transformed config ensures Cancel restores pre-edit values. Nice.
Edge case: confirm additionalParameters deep equality doesn’t retain stale touched/error state after reset; current resetForm({ values }) should clear it.
277-285: Additional parameters mapping is safe; preserves empty values.Options mapping defensively handles blank keys/values; good for UI.
60-67: Cancel behavior for paging size is correct.Local state resets to saved value; submit persists to Redux.
Also applies to: 195-199
44-46: Selector default is good for backward compat.Fallback to 10 covers older persisted states lacking the key.
32-34: Version display via package.json is useful.Immutable and read-only; good UX.
139-139: PropTypes kept in sync—nice.Once you rename to paging, update prop names here accordingly.
31-31: Toast on Cedarling setting change is a good touch.Clear UX feedback.
287-292: The review comment is incorrect—a visible submit action is present.The component renders a submit button when
type="submit"is configured. ThehideButtons={{ save: true }}prop hides the save button (which usesonClick={saveHandler}), not the submit button. These are two different submission mechanisms: the hidden save button is for direct handlers, while the submit button is for form submission. The page uses form submission (wrapped in<Form>), so the submit button with "Apply" text will display and function correctly. No changes are needed.Likely an incorrect or invalid review comment.
…ex into admin-ui-issue-2388
…ex into admin-ui-issue-2388
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 (3)
admin-ui/app/redux/features/authSlice.ts(6 hunks)admin-ui/app/redux/features/types/authTypes.ts(1 hunks)admin-ui/plugins/admin/components/Settings/SettingsPage.js(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (2)
admin-ui/plugins/services/Components/Configuration/LdapForm.js (2)
dispatch(20-20)formik(42-74)admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (1)
ConfigPage(28-163)
🔇 Additional comments (5)
admin-ui/app/redux/features/authSlice.ts (1)
55-55: Minor: unused-arg cleanup is fine.Underscored params avoid TS lint noise without logic changes in getUserInfo/getAPIAccessToken/getUserLocation.
Also applies to: 78-78, 93-93
admin-ui/plugins/admin/components/Settings/SettingsPage.js (4)
48-50: Nice SSR/test guard for window access.Safe fallback prevents crashes outside the browser.
124-126: Correct: use selected value, not selectedIndex.parseInt(e.target.value, 10) avoids order-coupling bugs.
168-173: Good: deduplicated auth script names.Set-based unique list avoids duplicate SIMPLE_PASSWORD_AUTH.
193-197: Cancel now correctly resets to prior values.Resets Formik to current config and restores paging size from store.
Manual check:
- Change multiple fields + paging size.
- Click Cancel → values match Redux config and paging resets to savedPagingSize.
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/GluuCommitFooter.tsx (1)
87-112: Avoid duplicate “Apply” buttons (submit vs click handler).When type==='submit' and hideButtons.save is falsy, two Apply buttons render. Make them mutually exclusive.
Apply:
- {type === 'submit' && ( + {type === 'submit' ? ( <Button type="submit" color={`primary-${selectedTheme}`} style={{ ...applicationStyle.buttonStyle, ...applicationStyle.buttonFlexIconStyles }} className="ms-auto px-4" disabled={disableButtons?.save} > <i className="fa fa-check-circle me-2"></i> {t('actions.apply')} </Button> - )} + ) : ( + (!hideButtons || !hideButtons['save']) && ( + <Button + type="button" + color={`primary-${selectedTheme}`} + style={{ ...applicationStyle.buttonStyle, ...applicationStyle.buttonFlexIconStyles }} + className="ms-auto px-4" + onClick={saveHandler} + disabled={disableButtons?.save} + > + <i className="fa fa-check-circle me-2"></i> + {t('actions.apply')} + </Button> + ) + )} - - {!hideButtons || !hideButtons['save'] ? ( - <Button - type="button" - color={`primary-${selectedTheme}`} - style={{ ...applicationStyle.buttonStyle, ...applicationStyle.buttonFlexIconStyles }} - className="ms-auto px-4" - onClick={saveHandler} - disabled={disableButtons?.save} - > - <i className="fa fa-check-circle me-2"></i> - {t('actions.apply')} - </Button> - ) : null}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
admin-ui/app/redux/features/authSlice.ts(3 hunks)admin-ui/app/routes/Apps/Gluu/GluuCommitFooter.tsx(5 hunks)admin-ui/app/utils/pagingUtils.ts(1 hunks)admin-ui/plugins/admin/components/CustomScripts/ScriptListPage.tsx(5 hunks)admin-ui/plugins/admin/components/Permissions/UiPermListPage.js(2 hunks)admin-ui/plugins/admin/components/Roles/UiRoleListPage.js(1 hunks)admin-ui/plugins/admin/components/Settings/SettingsPage.js(3 hunks)admin-ui/plugins/auth-server/components/Sessions/SessionListPage.tsx(2 hunks)admin-ui/plugins/schema/components/Person/AttributeListPage.tsx(2 hunks)admin-ui/plugins/services/Components/Configuration/LdapListPage.js(2 hunks)admin-ui/plugins/services/Components/Configuration/SqlListPage.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
admin-ui/plugins/services/Components/Configuration/SqlListPage.js (3)
admin-ui/plugins/admin/components/Permissions/UiPermListPage.js (1)
pageSize(53-53)admin-ui/plugins/services/Components/Configuration/LdapListPage.js (1)
pageSize(64-64)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)
admin-ui/plugins/services/Components/Configuration/LdapListPage.js (3)
admin-ui/plugins/admin/components/Permissions/UiPermListPage.js (1)
pageSize(53-53)admin-ui/plugins/services/Components/Configuration/SqlListPage.js (1)
pageSize(60-60)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)
admin-ui/plugins/admin/components/Permissions/UiPermListPage.js (1)
admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (4)
admin-ui/plugins/admin/components/Permissions/UiPermListPage.js (1)
pageSize(53-53)admin-ui/plugins/services/Components/Configuration/LdapListPage.js (1)
pageSize(64-64)admin-ui/plugins/services/Components/Configuration/SqlListPage.js (1)
pageSize(60-60)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)
admin-ui/plugins/admin/components/CustomScripts/ScriptListPage.tsx (4)
admin-ui/plugins/admin/components/Permissions/UiPermListPage.js (1)
pageSize(53-53)admin-ui/plugins/services/Components/Configuration/LdapListPage.js (1)
pageSize(64-64)admin-ui/plugins/services/Components/Configuration/SqlListPage.js (1)
pageSize(60-60)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)
admin-ui/plugins/auth-server/components/Sessions/SessionListPage.tsx (4)
admin-ui/plugins/admin/components/Permissions/UiPermListPage.js (1)
pageSize(53-53)admin-ui/plugins/services/Components/Configuration/LdapListPage.js (1)
pageSize(64-64)admin-ui/plugins/services/Components/Configuration/SqlListPage.js (1)
pageSize(60-60)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)
admin-ui/plugins/admin/components/Roles/UiRoleListPage.js (1)
admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (3)
admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-6)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(153-160)admin-ui/app/cedarling/index.ts (1)
CedarlingLogType(5-5)
🔇 Additional comments (21)
admin-ui/app/redux/features/authSlice.ts (1)
54-92: LGTM! Proper unused parameter naming.Prefixing unused parameters with
_is idiomatic and clearly signals intent. These placeholder reducers correctly ignore the state parameter.admin-ui/plugins/admin/components/Permissions/UiPermListPage.js (2)
30-30: LGTM! Centralized paging utility.The import of
getPagingSizealigns with the PR-wide refactor to centralize paging size management.
53-53: LGTM! Consistent with centralized paging.Replacing direct localStorage access with
getPagingSize()reduces duplication and centralizes paging logic.admin-ui/plugins/services/Components/Configuration/LdapListPage.js (2)
30-30: LGTM! Centralized paging utility.Consistent with the PR-wide refactor to centralize paging size management.
64-64: LGTM! Replaces direct localStorage access.Using
getPagingSize()centralizes paging logic and reduces code duplication.admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (2)
30-30: LGTM! Centralized paging utility.Consistent with the PR-wide refactor to centralize paging size management.
70-71: LGTM! Centralized paging initialization.Using
getPagingSize()to initialize the limit state is consistent with other list pages in this PR.admin-ui/plugins/admin/components/Roles/UiRoleListPage.js (2)
20-20: LGTM! Centralized paging utility.Consistent with the PR-wide refactor to centralize paging size management.
31-31: LGTM! Replaces localStorage access.Using
getPagingSize()centralizes paging logic consistently with other list pages.admin-ui/plugins/services/Components/Configuration/SqlListPage.js (2)
28-28: LGTM! Centralized paging utility.Consistent with the PR-wide refactor to centralize paging size management.
60-60: LGTM! Centralized paging initialization.Using
getPagingSize()reduces direct localStorage dependencies and centralizes logic.admin-ui/plugins/admin/components/CustomScripts/ScriptListPage.tsx (5)
30-30: LGTM! Centralized paging utility.Consistent with the PR-wide refactor to centralize paging size management.
43-44: LGTM! Centralized paging initialization.Using
getPagingSize()to initialize pageSize and limit state is consistent with other list pages.
196-209: LGTM! Proper unused parameter naming.Renaming
rowDatato_rowDatacorrectly signals that the parameter is unused in this action handler (the actual row data comes from theentryparameter in the onClick handler).
213-226: LGTM! Proper unused parameter naming.Renaming
rowDatato_rowDatais appropriate since the parameter is unused in this action handler.
275-288: LGTM! Proper unused parameter naming.Renaming
rowDatato_rowDatacorrectly indicates the parameter is unused in this delete action handler.admin-ui/plugins/auth-server/components/Sessions/SessionListPage.tsx (2)
40-40: LGTM! Centralized paging utility.Consistent with the PR-wide refactor to centralize paging size management.
174-174: LGTM! Simplifies paging size retrieval.Replacing manual parseInt and localStorage access with
getPagingSize()simplifies the code and centralizes paging logic.admin-ui/plugins/admin/components/Settings/SettingsPage.js (3)
116-121: Cancel now correctly resets to previous state.resetForm with config-derived values and restoring savedPagingSize fulfills the issue’s expected behavior.
Please verify that GluuProperties respects formik.resetForm (no internal state persists beyond formik values).
286-291: Good wiring of Save/Cancel/disable states.Save uses formik.handleSubmit; Cancel routes to handleCancel via GluuCommitFooter; disableButtons ties to dirty/validation flags — solid.
70-77: Backend field shape check: additionalParameters.Confirm API expects an array of {key,value} and not a map/object; mismatch would explain “not updating” notes.
Would you like a small adapter to convert between UI array and backend object if needed?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/app/utils/pagingUtils.ts(1 hunks)admin-ui/plugins/admin/components/Settings/SettingsPage.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (3)
admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-24)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(153-160)
🔇 Additional comments (5)
admin-ui/app/utils/pagingUtils.ts (1)
1-45: LGTM! Well-implemented utility with robust error handling.The paging utilities are well-designed with:
- Proper SSR/test environment guards
- Input validation ensuring only positive integers
- Graceful error handling with informative warnings
- Namespaced localStorage key to avoid collisions
The past review concerns about SSR safety and input validation have been thoroughly addressed.
admin-ui/plugins/admin/components/Settings/SettingsPage.js (4)
1-35: Imports and dependencies properly organized.All imports are appropriately used throughout the component. The refactor correctly introduces Formik, Yup validation, and the new paging utilities. No unused imports detected.
36-63: Component initialization properly handles SSR and edge cases.The setup correctly addresses previous review concerns:
- Theme context accessed safely with optional chaining and fallback
- Window object guarded against SSR environments
- Paging size initialization uses the centralized utility with computed defaults
291-296: Excellent! The save button blocker has been resolved.The
GluuCommitFooteris now correctly configured:
saveHandler={formik.handleSubmit}properly wires form submission- Save button disabled only when form hasn't changed or has validation errors
- Cancel handler properly resets form and paging size state
This resolves the critical issue from previous reviews where the save button was hidden and form submission was blocked.
96-115: Formik integration properly implements cancel/reset behavior.The Formik configuration correctly addresses the PR's objective (fixing incorrect cancel button behavior):
enableReinitialize: truekeeps form in sync with config updatesinitialValuesderived from config ensures correct reset statehandleCancelexplicitly resets to config-derived values and paging size- Validation prevents invalid submissions
The form now properly reverts all fields (including custom authentication parameters and paging size) when Cancel is clicked.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (4)
293-298: Save/cancel wiring looks correct now.Save uses formik.handleSubmit; Cancel resets form and paging; buttons disable state respects dirty/errors.
39-41: Good: ThemeContext access is guarded with a safe fallback.Prevents runtime errors when context is absent.
58-62: Good: window access is SSR-safe.Guarded configApiUrl avoids crashes in SSR/tests.
198-201: Good: stable keys for options.Uses option value as key; avoids index-key pitfalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/admin/components/Settings/SettingsPage.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (4)
admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-24)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(153-160)admin-ui/app/cedarling/index.ts (1)
CedarlingLogType(5-5)
🔇 Additional comments (1)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (1)
46-52: Nice: default paging size derived from the options.Keeps defaults in sync with UI choices.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (2)
96-117: Blocker: Baseline paging size updates even if save fails; Cancel can revert to unsaved value.setBaselinePagingSize is called unconditionally right after dispatch. If putConfigWorker fails, isFormChanged becomes false and Cancel resets to the new (unsaved) size. Gate the baseline update on confirmed success (saga success or a store “save succeeded” flag). At minimum, remove the immediate baseline update here and move it to the success path.
Proposed change (UI side):
onSubmit: (values) => { savePagingSizeToStorage(currentPagingSize) const cedarlingLogTypeChanged = values?.cedarlingLogType !== config?.cedarlingLogType dispatch( putConfigWorker({ ...values, _meta: { cedarlingLogTypeChanged, toastMessage: cedarlingLogTypeChanged ? t('fields.reloginToViewCedarlingChanges') : undefined, }, }), ) - setBaselinePagingSize(currentPagingSize) },And update baseline from a success signal (example):
// Pseudocode — adjust names to your store: // const saveSucceeded = useSelector((s) => s.authReducer.putConfig.success) useEffect(() => { if (saveSucceeded) setBaselinePagingSize(currentPagingSize) }, [saveSucceeded, currentPagingSize])Run to locate a usable success flag/action:
#!/bin/bash # Find success/failure signals for config save rg -nP "putConfigWorker|putConfig(Success|Failed|Failure)|save(Config)?(Success|Failure)|TOAST|updateToast" -C2 admin-ui app redux
104-114: Gate cedarlingLogType toast on success, not preemptively.Passing toastMessage via _meta is fine, but ensure the toast fires only when the saga confirms success. If the saga already handles success/failure toasts centrally, avoid duplicating or preempting here.
Suggested verification:
#!/bin/bash # Inspect saga/reducer to confirm success-gated toast behavior rg -nP "putConfigWorker|toastMessage|updateToast|try\s*\{|catch\s*\(" -C2 admin-ui app redux
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/app/redux/sagas/AuthSaga.ts(1 hunks)admin-ui/plugins/admin/components/Settings/SettingsPage.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
admin-ui/plugins/admin/components/Settings/SettingsPage.js (3)
admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/app/utils/pagingUtils.ts (1)
getPagingSize(1-24)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(153-160)
admin-ui/app/redux/sagas/AuthSaga.ts (1)
admin-ui/app/redux/api/backend-api.ts (1)
putServerConfiguration(15-25)
🔇 Additional comments (7)
admin-ui/app/redux/sagas/AuthSaga.ts (1)
61-64: LGTM! Good separation of metadata from config data.The destructuring pattern cleanly separates metadata (
_meta) from the actual configuration data, ensuring that only the relevant config fields are sent to the backend API. This aligns correctly with theputServerConfigurationAPI signature.admin-ui/plugins/admin/components/Settings/SettingsPage.js (6)
38-40: Good: ThemeContext access is safely guarded.Optional chaining with a sane default avoids runtime errors in tests/SSR.
58-62: Good: window access guarded for SSR/tests.Memoized, safe fallback to 'N/A' prevents crashes outside the browser.
45-50: Nice: default paging size derived from options.Keeps defaults aligned with the UI list; avoids drift.
89-95: Nice: deduplicated ACR scripts with Set.Prevents duplicate options while preserving SIMPLE_PASSWORD_AUTH.
199-209: Good: select uses string values and parses to number.Aligns with HTML select semantics; stable option keys used.
301-306: Save wiring looks correct.saveHandler points to formik.handleSubmit; buttons disabled based on dirty/errors; Cancel resets both form and paging size.
Quick manual check:
- Change a field → Save enabled.
- Click Save → verify state updates and Save disables after success.
- Change paging size only → Save enabled; Cancel restores previous saved size.
|
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
For Users page the default rows/page is 10 although I have set it to 10 in settings page.
3 participants
fix(admin-ui): incorrect cancel button behaviour in the Application Settings page in Home module
📝 Description
The Cancel button on the Application Settings page in the Home module is not resetting the field values to their previous state when clicked.
🧩 Steps to Reproduce
✅ Expected Behavior
On clicking the Cancel button, all fields should reset to their previous values before any modification.
🔗 Ticket
Closes: #2388
Summary by CodeRabbit
New Features
Improvements
Tests
Style