fix(admin-ui): Use typescript generated client for ACR and Authn pages #2553
Conversation
syntrydy
added
comp-admin-ui
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaces legacy Redux/saga/API for AuthN/ACR with TypeScript-generated Orval React Query hooks; adds Jotai atoms and TypeScript typings across AuthN components; introduces audit logging and query invalidation; removes related Redux slices, sagas, and API wrapper; adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as AuthN UI (React)
participant Jotai as Jotai Atom
participant Orval as Orval / React Query
participant QC as QueryClient
participant API
rect rgb(240,255,240)
Note over UI,API: NEW flow — Orval (React Query) + Jotai + audit
User->>UI: Open AuthN page
UI->>Orval: useGetAcrs() -> fetch ACRs
Orval->>QC: check cache
Orval->>API: fetch ACRs (if needed)
API-->>Orval: return ACRs
Orval-->>UI: data / isLoading
User->>UI: Select item -> Edit
UI->>Jotai: set currentAuthNItemAtom
User->>UI: Submit changes
UI->>Orval: mutate via usePutAcrs().mutateAsync(payload)
Orval->>API: PUT acrs
API-->>Orval: success / error
Orval->>QC: invalidate getAcrs query
Orval-->>UI: onSuccess/onError callbacks
UI->>User: show toast / navigate
UI->>QC: call audit hook (logAcrUpdate)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
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/auth-server/components/Configuration/ConfigPage.js (1)
141-162: Add error logging to catch block for debugging.The try-catch block catches errors from both
putAcrsMutation.mutateAsyncandlogAcrUpdate, but the comment is incomplete. TheonErrorcallback (line 64) only handlesputAcrsMutationerrors and displays a toast, whereaslogAcrUpdateerrors are silently swallowed. Addconsole.error(error)in the catch block to log unexpected errors from either operation, which is essential for debugging when multiple async operations are chained.Note:
patchJsonConfigis intentionally called outside theif(put)block, so it executes regardless of ACR update success. If this separation is intentional (ACR update is independent from config patching), the current behavior is acceptable.admin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsx (1)
306-391: LDAP form fields useitem.*instead offormik.values.*, breaking form edits.Multiple fields (
bindDN,maxConnections,remotePrimaryKey,localPrimaryKey,bindPassword) useitem.*for theirvalueprop instead offormik.values.*. User edits will be displayed but not captured informik.values, so submitted data will contain stale initial values.🔎 Proposed fix
<FormGroup row> <GluuLabel label="fields.bind_dn" size={4} /> <Col sm={8}> <Input id="bindDN" name="bindDN" - value={item?.bindDN || ''} + value={formik.values.bindDN || ''} onChange={formik.handleChange} /> </Col> </FormGroup> <FormGroup row> <GluuLabel label="fields.max_connections" size={4} /> <Col sm={8}> <Input id="maxConnections" name="maxConnections" - value={item?.maxConnections || ''} + value={formik.values.maxConnections || ''} onChange={formik.handleChange} /> </Col> </FormGroup> <FormGroup row> <GluuLabel label="fields.remote_primary_key" size={4} /> <Col sm={8}> <Input id="remotePrimaryKey" name="remotePrimaryKey" - value={item?.localPrimaryKey || ''} + value={formik.values.remotePrimaryKey || ''} onChange={formik.handleChange} /> </Col> </FormGroup> <FormGroup row> <GluuLabel label="fields.local_primary_key" size={4} /> <Col sm={8}> <Input id="localPrimaryKey" name="localPrimaryKey" - value={item?.localPrimaryKey || ''} + value={formik.values.localPrimaryKey || ''} onChange={formik.handleChange} /> </Col> </FormGroup> ... <FormGroup row> <GluuLabel label="fields.bind_password" size={4} /> <Col sm={8}> <Input id="bindPassword" name="bindPassword" - value={item?.bindPassword || ''} + value={formik.values.bindPassword || ''} onChange={formik.handleChange} /> </Col> </FormGroup>
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
admin-ui/package.jsonadmin-ui/plugins/auth-server/components/AuthN/AuthNDetailPage.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.jsadmin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNListPage.jsadmin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsxadmin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsxadmin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.tsadmin-ui/plugins/auth-server/components/AuthN/atoms/index.tsadmin-ui/plugins/auth-server/components/AuthN/constants.tsadmin-ui/plugins/auth-server/components/AuthN/hooks/index.tsadmin-ui/plugins/auth-server/components/AuthN/hooks/useAcrAudit.tsadmin-ui/plugins/auth-server/components/AuthN/index.tsxadmin-ui/plugins/auth-server/components/AuthN/styles.tsadmin-ui/plugins/auth-server/components/Configuration/ConfigPage.jsadmin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsxadmin-ui/plugins/auth-server/plugin-metadata.jsadmin-ui/plugins/auth-server/redux/api/AcrApi.jsadmin-ui/plugins/auth-server/redux/features/acrSlice.jsadmin-ui/plugins/auth-server/redux/features/authNSlice.jsadmin-ui/plugins/auth-server/redux/sagas/AcrsSaga.jsadmin-ui/plugins/auth-server/redux/sagas/AuthnSaga.js
💤 Files with no reviewable changes (8)
- admin-ui/plugins/auth-server/redux/sagas/AcrsSaga.js
- admin-ui/plugins/auth-server/plugin-metadata.js
- admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.js
- admin-ui/plugins/auth-server/redux/sagas/AuthnSaga.js
- admin-ui/plugins/auth-server/redux/features/authNSlice.js
- admin-ui/plugins/auth-server/redux/api/AcrApi.js
- admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.js
- admin-ui/plugins/auth-server/redux/features/acrSlice.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.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/auth-server/components/Configuration/ConfigPage.jsadmin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsxadmin-ui/plugins/auth-server/components/AuthN/index.tsxadmin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/hooks/useAcrAudit.ts
🧬 Code graph analysis (5)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (5)
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (2)
currentAuthNItemAtom(34-34)AuthNItem(3-32)admin-ui/plugins/auth-server/components/AuthN/atoms/index.ts (2)
currentAuthNItemAtom(1-1)AuthNItem(1-1)admin-ui/app/utils/ApiUtils.ts (2)
handleSuccess(51-53)handleError(42-49)admin-ui/app/helpers/navigation.ts (1)
ROUTES(204-204)admin-ui/app/components/index.tsx (1)
CardBody(44-44)
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (1)
admin-ui/plugins/auth-server/components/AuthN/atoms/index.ts (2)
AuthNItem(1-1)currentAuthNItemAtom(1-1)
admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsx (1)
admin-ui/plugins/auth-server/components/AuthN/helper/acrUtils.ts (1)
DropdownOption(4-7)
admin-ui/plugins/auth-server/components/AuthN/AuthNDetailPage.tsx (2)
admin-ui/app/components/index.tsx (1)
Container(62-62)admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsx (2)
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (1)
AuthNItem(3-32)admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (1)
acrs(42-46)
🔇 Additional comments (23)
admin-ui/plugins/auth-server/components/AuthN/atoms/index.ts (1)
1-1: LGTM!The barrel export pattern is clean and idiomatic, properly re-exporting the atom and its associated type.
admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (4)
1-1: LGTM!The new imports are well-organized and necessary for the React Query migration. Good use of barrel exports for the JansConfigApi hooks.
Also applies to: 9-9, 23-23, 27-28
38-46: LGTM!The React Query hooks are properly initialized with sensible configuration. The 30-second
staleTimeis appropriate for ACR configuration data.
48-66: LGTM!The mutation handlers follow React Query best practices with proper memoization via
useCallback, query invalidation on success, and user-friendly error handling with toast notifications.
177-183: LGTM!The loading states are comprehensive and correctly account for:
- Initial configuration loading
- ACR data fetching
- ACR mutation in progress
This provides good UX by preventing user interaction during async operations.
admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsx (3)
1-1: LGTM!The TypeScript migration is well-executed:
- Proper type imports for React events and elements
- Explicit function signature with typed props and return type
- Defensive fallback for theme selection (line 48) prevents potential runtime errors
Also applies to: 45-45, 48-48
52-53: LGTM!State variables are properly typed to match their usage throughout the component. The
string | undefinedtype fordatacorrectly reflects the optionalvalueprop.
59-87: LGTM!All function implementations are properly typed with:
- Explicit parameter types
- Correct event handler typing (
ChangeEvent<HTMLSelectElement>)- Appropriate void return types where applicable
- Safe handling of optional values with fallbacks
Also applies to: 109-111
admin-ui/plugins/auth-server/components/AuthN/hooks/index.ts (1)
1-1: LGTM!The barrel export pattern is clean and follows the project's module organization conventions.
admin-ui/package.json (1)
167-167: jotai version is current and secure. Version ^2.16.1 is the latest stable release and has no known security vulnerabilities.admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (1)
34-34: LGTM!The Jotai atom is properly defined with correct typing and initialization.
admin-ui/plugins/auth-server/components/AuthN/AuthNDetailPage.tsx (2)
7-19: LGTM!The interfaces are well-defined with proper typing that aligns with
AuthNItemfrom the atoms file. Thelevelfield correctly usesnumbertype.
21-24: LGTM!The explicit
ReactElementreturn type improves type safety. The color reference correction fromsmokeWhitetowhiteSmokealigns with the defined custom colors.admin-ui/plugins/auth-server/components/AuthN/index.tsx (3)
12-15: LGTM!The
TabNameinterface provides clear typing for tab configuration.
17-20: LGTM!The explicit
ReactElementreturn type and typedtabNamesarray improve type safety.
37-51: LGTM!The explicit return type
ReactElement | undefinedand the addition of adefaultcase returningundefinedare good defensive programming practices that improve type safety and handle unexpected tab names gracefully.admin-ui/plugins/auth-server/components/AuthN/hooks/useAcrAudit.ts (3)
8-23: LGTM!The
AuthStateandRootStateinterfaces are well-defined for Redux state access with appropriate optional fields.
25-30: LGTM!The hook properly selects authentication state from Redux with safe optional chaining.
31-50: No issues found. ThedefaultAcrproperty is a valid property of theAuthenticationMethodtype and is used consistently throughout the codebase (DefaultAcr.tsx, AuthNEditPage.tsx, and other files). The code at line 44 is correct.admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (1)
109-117: No-op branch silently succeeds without user feedback.When
defaultAuthNMethodis falsy forsimple_password_auth,handleSuccess()is called directly, which shows a success toast and navigates away. This may confuse users who expected to save other changes. Consider if this is the intended behavior or if you should skip navigation when no changes were made.admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx (1)
217-220: Button disabled logic is correct but could be clearer.The array handling for
put.valueis necessary but adds complexity. Consider extracting to a computed value for readability.admin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsx (1)
243-251:primaryKeyfield usesitem.primaryKeybut is disabled anyway.Since this field is disabled for
simple_password_auth, the stale value issue doesn't affect functionality. However, for consistency, consider usingformik.values.primaryKeyif you plan to enable this field in the future.admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx (1)
232-261: Good implementation of the list page with proper permission checks and loading states.The MaterialTable integration with Cedar permissions, detail panel, and themed styling is well-structured.
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Fix all issues with AI Agents 🤖
In @admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx:
- Around line 108-131: The LDAP + ACR update sequence can leave partial state if
putLdapMutation succeeds and putAcrsMutation fails; modify the handler around
putLdapMutation and putAcrsMutation so you either (a) call a backend endpoint
that performs both changes in a single transaction if available, or (b)
implement compensating rollback: fetch and cache the existing LDAP config before
calling putLdapMutation (use the same configId used to build ldapPayload), call
putLdapMutation.mutateAsync, then attempt putAcrsMutation.mutateAsync; if the
ACR update fails, call a revert mutation (e.g., putLdapMutation or a
delete/restore endpoint) to restore the cached previous LDAP configuration and
surface a clear partial-success/error message to the user; ensure you check
data.defaultAuthNMethod (string or boolean) before attempting the ACR mutation
and handle errors from both mutations with try/catch so failures trigger
rollback and user notification.
- Around line 49-50: The code assumes currentAuthNItemAtom (typed atom<AuthNItem
| null>) is always present by coercing atomItem to an empty object (const item =
atomItem || {}), which leads to unsafe accesses like item.name, item.configId
and assigning item.acrName into scriptPayload.name; instead, add explicit
null/undefined guards: use optional chaining and sensible fallbacks (e.g.,
item?.acrName || '' for scriptPayload.name) at each property access (references:
currentAuthNItemAtom, atomItem, item, scriptPayload.name) and, importantly,
guard the form submission path so it short-circuits or validates when atomItem
is null (e.g., disable submit or return early from the submit handler) to ensure
you never read properties from a missing AuthNItem.
In @admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx:
- Line 57: The component defines a fixed pagination limit using useState (const
[limit] = useState(10)) even though it never changes; replace this with a plain
constant (e.g., const LIMIT = 10 or const limit = 10) inside the AuthNListPage
component or module and update any references to use the constant instead of the
state variable so you remove the unnecessary useState import and state overhead.
- Around line 115-130: The action pushed into newActions inside AuthNListPage is
wrapped by if (canWriteAuthN) so the disabled: !canWriteAuthN property is
redundant; remove the disabled property from the edit action object (the
function adding an action for AuthNItem) and keep the rest (icon, iconProps,
tooltip, onClick handler calling handleGoToAuthNEditPage) unchanged.
- Around line 79-82: The variable customScriptloading is a redundant, misspelled
alias of scriptsLoading returned from
useCustomScriptsByType(DEFAULT_SCRIPT_TYPE); remove the customScriptloading
declaration and replace its usages (e.g., at the spots currently referencing
customScriptloading around lines where the component uses loading state) with
scriptsLoading. Ensure no other references to customScriptloading remain and
that scriptsLoading is used directly for loading logic/rendering.
In @admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx:
- Around line 175-192: The audit logging catch in submitForm currently only
calls console.error which doesn't notify the user; update the inner
catch(auditError) inside submitForm (after await logAcrUpdate(...)) to also
surface a user-facing warning toast/notification (e.g., call your app's
notify/Toast API such as showToast/enqueueSnackbar/toast.warn) with a clear
message like "Audit logging failed" plus the auditError message, while
preserving the existing console.error and allowing the successful
putAcrsMutation path to complete.
- Around line 125-127: The dispatch in DefaultAcr.tsx calls getScripts with an
incomplete payload ({ action: {} }) so InitSaga.ts expects
payload.action.action_data but receives undefined; fix by dispatching a properly
structured action object that includes action_data (e.g., use the same
buildPayload() pattern used in ScopeEditPage.tsx/ScopeAddPage.tsx to construct
action.action_data before calling dispatch(getScripts(...))), or alternatively
update the getScripts handler in InitSaga.ts to null-check and default
payload.action.action_data like getClients does; modify either DefaultAcr.tsx to
pass action.action_data or InitSaga.ts to guard against undefined.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
admin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsx (5)
88-91: Validation schema type mismatch forlevel.The schema validates
levelasYup.string(), butAuthNFormValues.levelis typed asnumber. This inconsistency can cause unexpected validation behavior.🔎 Proposed fix
validationSchema: Yup.object({ acr: Yup.string().required('ACR name is required.'), - level: Yup.string().required('Level is required.'), + level: Yup.number().required('Level is required.'), }),
278-322: Form fields bound toitemprop instead offormik.values— edits won't be captured.The fields
bindDN(line 282),maxConnections(line 294),remotePrimaryKey(line 306), andlocalPrimaryKey(line 318) usevalue={item?.propName || ''}. Since these are bound to the original prop rather than Formik state, user changes are lost on submit.🔎 Proposed fix
<Input id="bindDN" name="bindDN" - value={item?.bindDN || ''} + value={formik.values.bindDN || ''} onChange={formik.handleChange} /> ... <Input id="maxConnections" name="maxConnections" - value={item?.maxConnections || ''} + value={formik.values.maxConnections || ''} onChange={formik.handleChange} /> ... <Input id="remotePrimaryKey" name="remotePrimaryKey" - value={item?.localPrimaryKey || ''} + value={formik.values.remotePrimaryKey || ''} onChange={formik.handleChange} /> ... <Input id="localPrimaryKey" name="localPrimaryKey" - value={item?.localPrimaryKey || ''} + value={formik.values.localPrimaryKey || ''} onChange={formik.handleChange} />
349-359:bindPasswordfield bound toiteminstead offormik.values.Same issue as above — line 355 uses
value={item?.bindPassword || ''}, so user edits won't be captured.🔎 Proposed fix
<Input id="bindPassword" name="bindPassword" - value={item?.bindPassword || ''} + value={formik.values.bindPassword || ''} onChange={formik.handleChange} />
211-217:primaryKeyfield bound toiteminstead offormik.values.Line 214 uses
value={item?.primaryKey || ''}. Edits won't be tracked.🔎 Proposed fix
<Input id="primaryKey" name="primaryKey" - value={item?.primaryKey || ''} + value={formik.values.primaryKey || ''} onChange={formik.handleChange} disabled={item.name === 'simple_password_auth'} />Note:
primaryKeyis missing fromAuthNFormValuesinterface. You'll need to add it:interface AuthNFormValues { // ... existing fields primaryKey: string }And initialize it in
initialValues:primaryKey: item?.primaryKey || '',
361-389: Checkbox fields usedefaultChecked— changes won't sync with Formik.The
useSSL(line 369) andenabled(line 384) checkboxes usedefaultChecked={item.prop}, making them uncontrolled. Formik'shandleChangewon't properly track checkbox state. Usecheckedwithformik.valuesinstead.🔎 Proposed fix
<Input placeholder={t('placeholders.use_ssl')} id="useSSL" type="checkbox" - defaultChecked={item.useSSL} + checked={formik.values.useSSL} onChange={formik.handleChange} /> ... <Input placeholder={t('placeholders.enabled')} id="enabled" type="checkbox" - defaultChecked={item.enabled} + checked={formik.values.enabled} onChange={formik.handleChange} />
♻️ Duplicate comments (3)
admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx (1)
41-43: Interface simplified appropriately.The
PutDatainterface now contains only thevaluefield, addressing the previous concern about unusedpathandopfields. The union typestring | string[]properly handles both single and array values.admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (1)
33-33: Consider typingtableDatamore specifically.The
tableDatafield remains typed asunknown, which bypasses type safety. While this may be intentional for flexibility with table library internals, consider usingobjectorRecord<string, unknown>for slightly better type safety, or define a specific interface if the structure is known.💡 Possible refinement
- tableData?: unknown + tableData?: objectOr if the structure is known:
interface TableMetadata { id?: number checked?: boolean // other known fields } // Then use: tableData?: TableMetadataadmin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx (1)
186-196: Type mismatch withBUILT_IN_ACRSremains unresolved.The type assertion
BUILT_IN_ACRS as AuthNItem[]masks the fact thatBuiltInAcr.levelis astringwhileAuthNItem.levelisnumber. The numeric sort on lines 193-195 will work correctly for custom items but the built-in path bypasses sorting entirely. If built-in items ever need sorting, this will produce incorrect results.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsxadmin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsxadmin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.tsadmin-ui/plugins/auth-server/components/AuthN/atoms/index.tsadmin-ui/plugins/auth-server/components/AuthN/constants.tsadmin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
📚 Learning: 2025-11-06T08:23:20.948Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
📚 Learning: 2025-11-10T14:18:58.310Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:107-109
Timestamp: 2025-11-10T14:18:58.310Z
Learning: In the Cedarling configuration page (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), console.error calls should be kept in catch blocks when there are multiple operations in the try block, as they help print the whole error object with details for debugging unexpected errors.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsxadmin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.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/auth-server/components/AuthN/DefaultAcr.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsx
📚 Learning: 2025-11-11T15:17:34.651Z
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx
🧬 Code graph analysis (5)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (4)
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (3)
ConfigurationProperty(3-9)currentAuthNItemAtom(36-36)AuthNItem(11-34)admin-ui/app/utils/ApiUtils.ts (2)
handleSuccess(51-53)handleError(42-49)admin-ui/app/helpers/navigation.ts (1)
ROUTES(204-204)admin-ui/app/components/index.tsx (1)
CardBody(44-44)
admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsx (1)
admin-ui/plugins/auth-server/components/AuthN/helper/acrUtils.ts (1)
DropdownOption(4-7)
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (1)
admin-ui/plugins/auth-server/components/AuthN/atoms/index.ts (3)
ConfigurationProperty(1-1)AuthNItem(1-1)currentAuthNItemAtom(1-1)
admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx (1)
admin-ui/plugins/auth-server/components/AuthN/hooks/useAcrAudit.ts (1)
useAcrAudit(25-53)
admin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsx (1)
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (1)
AuthNItem(11-34)
🔇 Additional comments (15)
admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx (4)
1-25: LGTM! Imports properly support React Query migration.The new imports for React Query (
useQueryClient), Orval-generated hooks (useGetAcrs,usePutAcrs), and the audit hook (useAcrAudit) are appropriate for migrating from Redux to React Query-based data fetching.
47-86: Well-structured React Query migration.The component properly initializes React Query hooks with appropriate configurations:
useGetAcrswith reasonablestaleTimeof 30 seconds- Mutation callbacks wrapped in
useCallbackwith correct dependencies- Cache invalidation on success via
queryClient.invalidateQueries- User-friendly error/success toast notifications
129-135: Proper error surfacing for ACR fetch failures.The error handling effect correctly extracts error messages and displays them to users via toast notifications, consistent with the existing pattern for Agama project fetch failures.
195-232: Comprehensive loading state management and button logic.The
GluuLoaderblocking condition correctly aggregates all async states (loadingScripts,agamaLoading,acrLoading,putAcrsMutation.isPending), ensuring a smooth loading experience. The Save button's disabled logic properly handles both string and array values, preventing unnecessary updates when the value hasn't changed.admin-ui/plugins/auth-server/components/AuthN/constants.ts (1)
1-25: LGTM! Previous type mismatches resolved.The
BuiltInAcrinterface now correctly typeslevelasnumber(line 3), and theBUILT_IN_ACRSconstant uses the numeric value-1(line 16) instead of the string'-1'. This ensures type consistency withAuthNItem.level.admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.tsx (1)
1-141: LGTM! TypeScript migration complete with proper type safety.The component has been successfully migrated to TypeScript with:
DropdownOptionnow imported from the shared location (line 7), eliminating duplication- Well-defined interfaces for
PutDataandDefaultAcrInputProps- Explicit typing throughout (state, handlers, event callbacks)
- Defensive theme fallback (line 44)
All previous review concerns have been addressed.
admin-ui/plugins/auth-server/components/AuthN/atoms/index.ts (1)
1-1: LGTM! Clean barrel export.The index file correctly re-exports the atom and types from
authNAtoms, providing a clean public API surface.admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (4)
21-43: Well-structured form values interface.The
AuthNFormValuesinterface comprehensively covers all authentication configuration fields with appropriate types (including union types for flexible string/number/array fields).
53-59: LGTM! Previous issue resolved.
handleSuccessnow correctly callssetIsSubmitting(false)(line 54) before scheduling navigation, ensuring the loading state is cleared immediately on success.
61-68: Robust error handler with fallback message.The error handler extracts the error message with a fallback to the translated error string, ensuring users always see meaningful feedback.
160-166: Improved error handling with proper logging.The catch block now:
- Checks if the error is unexpected (not from a mutation's
onError)- Logs unexpected errors for debugging
- Shows a toast notification
- Always resets the submitting state
This addresses the previous empty catch block concern.
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (1)
1-36: Well-defined atom and interfaces for AuthN state.The module provides clear TypeScript definitions for authentication configuration. The
ConfigurationPropertyandAuthNIteminterfaces cover all necessary fields for managing authentication methods.admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx (1)
198-227: LGTM!The MaterialTable configuration correctly integrates permission-based actions, theme-aware styling, and the detail panel. The loading state aggregation properly reflects all data sources.
admin-ui/plugins/auth-server/components/AuthN/AuthNForm.tsx (2)
16-37: LGTM on typed form values interface.The
AuthNFormValuesinterface provides clear typing for the form. The union types forserversandbaseDNs(string | string[]) accommodate both initial empty state and populated array state.
330-347: Good fix:GluuTypeAheadnow correctly usesformik.values.The
serversandbaseDNsfields now properly bind toformik.valueswith array type checking, addressing the previous review feedback.
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx:
- Line 107: The repeated boolean/string check for data.defaultAuthNMethod should
be extracted into a small helper to remove duplication and clarify intent; add a
function (e.g., isDefaultAuthMethod(value: boolean | string | undefined):
boolean) near the top of the file and replace the repeated expressions like
data.defaultAuthNMethod === 'true' || data.defaultAuthNMethod === true (found in
the AuthNEditPage component around the checks at the current conditional
locations) with calls to isDefaultAuthMethod(data.defaultAuthNMethod), updating
all occurrences (lines where you currently check at ~107, ~133, ~175).
- Around line 204-212: The component currently spreads {...item} into
<AuthNForm>, which can be unsafe when atomItem is null; update AuthNEditPage so
you guard against null before spreading—either compute a safe props object
(e.g., const safeItem = item ?? {}) and pass item={safeItem} to AuthNForm, or
only render <AuthNForm handleSubmit={handleSubmit} item={{...item}}> when
atomItem is non-null (conditional render). Locate the spread usage of item in
the JSX (the AuthNForm call) and the atomItem initialization earlier (related to
lines 49-50) and ensure a null guard or default is applied so AuthNForm never
receives null/invalid props.
- Around line 162-170: Extract the inline transformation of
data.configurationProperties into a small pure helper named e.g.
transformConfigurationProperties that accepts ConfigurationProperty[] |
undefined and returns the mapped array or undefined; replace the current block
that sets scriptPayload.configurationProperties with a call to
transformConfigurationProperties(data.configurationProperties). Ensure the
helper preserves the existing logic: return undefined for empty/undefined input,
filter out null/empty objects, and map each entry to { value1: e.key || e.value1
|| '', value2: e.value || e.value2 || '', hide: false } so scriptPayload and
existing callers continue to work unchanged.
- Around line 53-59: The handleSuccess callback schedules a setTimeout that can
fire after the component unmounts; change it to store the timeout id (e.g., in a
ref like timeoutIdRef) when calling setTimeout inside handleSuccess and add a
useEffect cleanup that clears that timeout (clearTimeout(timeoutIdRef.current))
on unmount; ensure handleSuccess still calls setIsSubmitting(false),
dispatch(updateToast(...)) and navigateToRoute(...) but that the scheduled
timeout is cleared to avoid navigation after unmount.
♻️ Duplicate comments (3)
admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx (1)
125-127: Verify thegetScriptspayload structure.The past review flagged
dispatch(getScripts({ action: {} }))as incomplete, noting thatInitSaga.tsexpectspayload.action.action_datato contain specific fields. The current code passes{ action: { action_data: {} } }with an empty object. Verify whether an emptyaction_dataobject is sufficient or if specific fields (like those built bybuildPayload()in other components) are required.#!/bin/bash # Verify what fields InitSaga.ts expects in action_data for getScripts echo "=== Checking InitSaga getScripts handler ===" cat admin-ui/app/redux/sagas/InitSaga.ts | grep -A 30 "function\* getScripts" echo -e "\n=== Checking other getScripts dispatch calls for comparison ===" rg "dispatch\(getScripts" --type ts --type tsx -B 2 -A 2admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx (1)
185-195: Type mismatch persists:BuiltInAcr.levelisstringbut sort expectsnumber.The type assertion on line 190 (
BUILT_IN_ACRS as AuthNItem[]) masks a structural incompatibility previously flagged. According to the constants file,BuiltInAcr.levelis astring(e.g.,'-1'), whileAuthNItem.levelexpectsnumber. The numeric sort at line 193 will produce incorrect results when operating on string values.🔎 Proposed fix
Convert
leveltonumberwhen buildingtableData:const tableData = useMemo(() => { if (loading || scriptsLoading || acrsLoading) { return [] } if (isBuiltIn) { - return BUILT_IN_ACRS as AuthNItem[] + return BUILT_IN_ACRS.map(acr => ({ + ...acr, + level: typeof acr.level === 'string' ? parseInt(acr.level, 10) : acr.level + })) as AuthNItem[] } return [...list.ldap, ...list.scripts].sort( (item1, item2) => (item1.level || 0) - (item2.level || 0), ) }, [loading, scriptsLoading, acrsLoading, isBuiltIn, list.ldap, list.scripts])admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (1)
49-50: Unsafe type coercion when atomItem is null.When
atomItemisnull, line 50 coerces it to an empty object{}and asserts it asAuthNItem. This bypasses TypeScript's type safety since all properties ofAuthNItemare optional, making{}structurally compatible but semantically incorrect. While the guard at line 98 prevents submission, the component still rendersAuthNFormwith an invalid item (line 208), which may display empty or undefined fields.Consider redirecting or showing a fallback UI when
atomItemis null:🔎 Proposed fix to guard against null atom
const atomItem = useAtomValue(currentAuthNItemAtom) - const item: AuthNItem = atomItem || {} const [isSubmitting, setIsSubmitting] = useState(false) + + // Redirect if no item is available + React.useEffect(() => { + if (!atomItem) { + dispatch(updateToast(true, 'error', t('messages.no_item_selected'))) + navigateToRoute(ROUTES.AUTH_SERVER_AUTHN) + } + }, [atomItem, dispatch, navigateToRoute, t]) + + if (!atomItem) { + return <GluuLoader blocking /> + } + + const item: AuthNItem = atomItemBased on learnings, the Back button should fallback to the respective listing screen. Similarly, when direct navigation occurs without a selected item, redirect to the AuthN list page.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsxadmin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsxadmin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsxadmin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx
📚 Learning: 2025-11-11T15:17:34.651Z
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNListPage.tsx
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
📚 Learning: 2025-11-06T08:23:20.948Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
📚 Learning: 2025-11-10T14:18:58.310Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:107-109
Timestamp: 2025-11-10T14:18:58.310Z
Learning: In the Cedarling configuration page (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), console.error calls should be kept in catch blocks when there are multiple operations in the try block, as they help print the whole error object with details for debugging unexpected errors.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsxadmin-ui/plugins/auth-server/components/AuthN/DefaultAcr.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/auth-server/components/AuthN/DefaultAcr.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (4)
admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (3)
ConfigurationProperty(3-9)currentAuthNItemAtom(36-36)AuthNItem(11-34)admin-ui/app/utils/ApiUtils.ts (2)
handleSuccess(51-53)handleError(42-49)admin-ui/app/helpers/navigation.ts (1)
ROUTES(204-204)admin-ui/app/components/index.tsx (1)
CardBody(44-44)
admin-ui/plugins/auth-server/components/AuthN/DefaultAcr.tsx (1)
admin-ui/plugins/auth-server/components/AuthN/hooks/useAcrAudit.ts (1)
useAcrAudit(25-53)
🔇 Additional comments (5)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (5)
1-19: Imports look good; ConfigurationProperty duplication resolved.The imports are well-organized. Importing
ConfigurationPropertyfrom'./atoms'correctly addresses the previous duplicate interface definition.
70-95: Mutation setup and loading state are well-structured.The three mutation hooks are correctly configured with shared success and error handlers, and the loading state appropriately combines all pending states.
97-101: Guard check is present but relies on earlier atomItem safety.The guard correctly checks
atomItem?.namebefore proceeding with submission. However, this check only prevents the submission logic from executing, not the initial render with potentially null/empty item data (see earlier comment on lines 49-50).
106-112: Sequential mutations include partial-failure mitigation.The code now handles partial failure scenarios where LDAP/Script updates succeed but the subsequent ACR update fails (lines 133-150, 175-192). The try-catch blocks display warning toasts to inform users of partial success, which addresses the previous concern about inconsistent state.
While the mutations are still not atomic (backend changes to LDAP/Script persist even if ACR fails), the user feedback clearly communicates what happened. If full atomicity is required, consider requesting a backend endpoint that performs both operations in a single transaction.
Also applies to: 113-151, 152-193
195-201: Verify if double error toasts are intentional for unexpected errors.The error handling at line 196 checks
error instanceof Error && !('response' in error)to distinguish between HTTP mutation errors (withresponseproperty, handled byonErrorcallback) and unexpected errors (withoutresponseproperty). However, sincehandleError(lines 61-68) shows a toast for all error types regardless of theresponseproperty, unexpected errors without aresponseproperty will trigger two toast dispatches: once from the mutation'sonErrorcallback and again from the catch block.If handling mutation errors separately from unexpected errors is intentional, either:
- Modify
handleErrorto skip unexpected errors, OR- Modify the catch block to prevent re-handling mutation errors
Otherwise, consider consolidating error handling to show a single toast per error.
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx:
- Around line 97-116: The three mutation hooks putAcrsMutation, putLdapMutation
and putScriptMutation currently all use the same onSuccess:
handleSuccess/onError: handleError, which causes duplicate success toasts and
duplicate navigation timeouts when multiple sequential mutations run; fix this
by removing the shared mutation callbacks and invoking
handleSuccess()/handleError(err) manually once inside the try/catch that
performs the chained updates OR create a second usePutAcrs instance (e.g.,
putAcrsMutationPrimary with callbacks for the simple_password_auth flow and
putAcrsMutationSecondary with no callbacks for nested ACR updates triggered by
putLdapMutation/putScriptMutation) and ensure only the primary mutation calls
handleSuccess so only one toast and one timeout is scheduled.
- Around line 161-177: Nested ACR update failures cause duplicate toasts because
the mutation's onError (handleError) and the inner catch both fire; remove the
onError handlers from the relevant mutations (e.g., putMutation,
putScriptMutation, putLdapMutation) so they don't auto-show errors, and instead
handle errors explicitly inside the try/catch where putAcrsMutation.mutateAsync
is invoked: in the inner catch only dispatch the warning via updateToast about
"LDAP/config saved, but failed to set default ACR" and call
setIsSubmitting(false)/return, ensuring handleError is not invoked for these
nested failures.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
📚 Learning: 2025-11-10T14:18:58.310Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:107-109
Timestamp: 2025-11-10T14:18:58.310Z
Learning: In the Cedarling configuration page (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), console.error calls should be kept in catch blocks when there are multiple operations in the try block, as they help print the whole error object with details for debugging unexpected errors.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (5)
admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (2)
properties(90-90)dispatch(37-37)admin-ui/plugins/auth-server/components/AuthN/atoms/authNAtoms.ts (2)
ConfigurationProperty(3-9)currentAuthNItemAtom(36-36)admin-ui/app/utils/ApiUtils.ts (2)
handleSuccess(51-53)handleError(42-49)admin-ui/app/helpers/navigation.ts (1)
ROUTES(204-204)admin-ui/app/components/index.tsx (1)
CardBody(44-44)
🔇 Additional comments (4)
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx (4)
221-229: Good defensive rendering for missing atom state.The early return with a helpful message when
atomItemis null provides clear user feedback and prevents runtime errors from missing data.
80-86: ✓ Timeout cleanup properly implemented.The navigation timeout is now stored in a ref and cleaned up in the
useEffecthook, preventing navigation attempts after component unmount.
21-38: ✓ Well-structured helper functions reduce duplication.The extracted
isDefaultAuthNMethodandtransformConfigurationPropertieshelpers improve code clarity and maintainability.
142-142: Empty string defaults may cause backend API validation failures for required fields.The code uses empty string fallbacks for optional atomItem properties:
- Line 142:
configId: atomItem.configId || ''(sent in GluuLdapConfiguration)- Line 183:
name: atomItem.acrName || ''(sent in CustomScript)- Line 194:
defaultAcr: atomItem.acrName || ''(sent as authentication default)While a guard at line 125 (
if (!atomItem?.name)) prevents submission ifnameis missing, it does not validate thatconfigIdandacrNameare populated. If the backend API requires these fields to be non-empty strings, the mutations will fail with validation errors. Either validate that these required fields exist before allowing submission, or ensure atomItem is always fully populated before rendering the form.Also applies to: 183-183, 194-194
admin-ui/plugins/auth-server/components/AuthN/AuthNEditPage.tsx
Outdated
Show resolved
Hide resolved
|
#2552
Closes #2552
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.