Convert Auth Server Properties into Forms along with Back button uniformity#2565
Convert Auth Server Properties into Forms along with Back button uniformity#2565
Conversation
…ead of marking Not present as Down (#2500)
Signed-off-by: Faisal Siddique <71010439+faisalsiddique4400@users.noreply.github.com>
…ex into admin-ui-issue-2560
…ex into admin-ui-issue-2560
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/auth-server/components/Configuration/JsonPropertyBuilder.tsx (1)
54-87: JSON-Patch path handling lacks JSON Pointer escaping; verify thatpropKeyvalues never contain/or~characters.The code splits the path by
/(line 75:trimmed.split('/')) without handling JSON Pointer escape sequences (~→~0,/→~1). If anypropKeycontains these characters, the path will be incorrectly segmented andgetInwill fail to locate the field. Confirm that the schema and input data guarantee keys are safe, or implement proper JSON Pointer unescaping.Additionally, the
errorsandtouchedtyping asRecord<string, unknown>andRecord<string, boolean | undefined>is narrow for deeply nested form structures; consider usingFormikErrors<T>/FormikTouched<T>with the actual form value type if Formik's type inference is needed for complex nesting.
🤖 Fix all issues with AI agents
In
@admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx:
- Around line 31-53: Replace the fragile negative-margin inline styles in
JsonPropertyBuilder.tsx (notably ERROR_CONTAINER_STYLE.marginTop = '-1.75rem')
with a Bootstrap-compatible approach: remove the negative margin and apply
Bootstrap's "invalid-feedback d-block" (or a shared CSS token/class) to the
error container and text instead of using ERROR_CONTAINER_STYLE/ERROR_TEXT_STYLE
hacks; update usages of ERROR_CONTAINER_STYLE, ERROR_TEXT_STYLE, ERROR_COL_STYLE
and FORM_GROUP_STYLE to rely on the class/token and remove layout pull-up logic
so errors render consistently across breakpoints/themes.
- Around line 88-123: renderError currently hides errors for composite fields
and misaligns the grid: change the strict touched check (where fieldTouched is
compared to true) to a truthy check (e.g., use Boolean(fieldTouched) or
!!fieldTouched) so arrays/objects count as touched, and simplify the layout by
removing the extra <Col sm={10}> wrapper and use proper grid math for the two
columns (use lSize for the label spacer and 12 - lSize for the error column
instead of using lSize twice) while keeping the ERROR_CONTAINER_STYLE /
ERROR_COL_STYLE placements intact so the error message renders and aligns
correctly.
In @admin-ui/plugins/services/Components/Configuration/sqlApiMocks.ts:
- Around line 90-114: The hook useGetConfigDatabaseSql currently accepts only a
limited options object; to support future migration to the real API, extend its
signature to accept and pass through React Query options (e.g., enabled,
refetchInterval, onSuccess) into the useQuery call so consumers can control
behavior; update the parameter type to include a generic ReactQueryOptions /
UseQueryOptions and merge options?.query?.staleTime defaulting to
DEFAULT_STALE_TIME_MS while keeping retry: false and preserving queryKey via
getGetConfigDatabaseSqlQueryKey and the existing queryFn.
In @admin-ui/plugins/services/Components/Configuration/utils/validations.ts:
- Around line 93-95: The exported schema lacks an explicit type annotation which
limits IDE/type inference; annotate sqlConfigurationSchema with an appropriate
Yup type (e.g., Yup.SchemaOf<SqlConfiguration> or
Yup.ObjectSchema<SqlConfiguration>) and update the declaration of
sqlConfigurationSchema to include that type while keeping the existing
initializer (Yup.object<SqlConfiguration>().shape(sqlConfigurationSchemaShape));
also ensure the Yup type is imported if not already.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsxadmin-ui/plugins/services/Components/Configuration/sqlApiMocks.tsadmin-ui/plugins/services/Components/Configuration/utils/validations.ts
🧰 Additional context used
🧠 Learnings (6)
📓 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.943Z
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.
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.
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts:4-101
Timestamp: 2026-01-06T13:45:28.303Z
Learning: In the admin-ui Config API Properties forms (ApiConfigForm component), frontend Yup validation uses `.nullable()` for all fields without `.required()` because the backend DTOs are the source of truth and enforce validation. The frontend validation should match what the backend accepts to avoid conflicts when editing existing configurations that may have null values.
📚 Learning: 2026-01-06T13:45:19.278Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts:4-101
Timestamp: 2026-01-06T13:45:19.278Z
Learning: In admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts, ensure Yup schema marks fields as nullable when the backend DTOs permit nulls and do not mark them as required. Align frontend validation with backend validation to prevent conflicts when editing existing configurations with null values; apply this pattern consistently to other Admin UI validation files where the backend is the source of truth.
Applied to files:
admin-ui/plugins/services/Components/Configuration/utils/validations.tsadmin-ui/plugins/services/Components/Configuration/sqlApiMocks.ts
📚 Learning: 2026-01-07T09:56:06.244Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:320-322
Timestamp: 2026-01-07T09:56:06.244Z
Learning: In the admin-ui Config API Properties forms (ApiConfigForm component in admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx), the error display pattern using `String(fieldError)` for top-level properties is acceptable because the DTO structure and validation schema design ensure that top-level errors are string messages, not nested error objects. Nested field validation is handled within JsonPropertyBuilderConfigApi components themselves.
Applied to files:
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx
📚 Learning: 2026-01-07T09:26:24.613Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:112-172
Timestamp: 2026-01-07T09:26:24.613Z
Learning: In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management.
Applied to files:
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.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/Configuration/JsonPropertyBuilder.tsxadmin-ui/plugins/services/Components/Configuration/sqlApiMocks.ts
📚 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/JsonPropertyBuilder.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/services/Components/Configuration/utils/validations.ts (1)
admin-ui/plugins/services/Components/Configuration/sqlApiMocks.ts (1)
SqlConfiguration(59-70)
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/types/index.ts (1)
JsonPropertyBuilderProps(38-49)admin-ui/app/components/index.tsx (2)
FormGroup(70-70)Col(60-60)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (7)
migratingTextIfRenamed(95-100)isString(11-13)shouldRenderAsString(23-25)isNumber(3-5)isStringArray(15-17)isEmptyArray(75-77)shouldRenderAsStringArray(27-29)
🔇 Additional comments (15)
admin-ui/plugins/services/Components/Configuration/utils/validations.ts (3)
1-26: Well-documented URI patterns with appropriate scope.The regex patterns are clearly documented with supported formats and explicit limitations (e.g., embedded DB paths not supported). The validation approach is appropriately lenient for frontend validation where the backend enforces stricter rules. Based on learnings, this aligns with the pattern that frontend validation should match what the backend accepts.
28-71: LGTM - Consistent validation pattern for string fields.All string fields follow a uniform pattern with trim, max length, empty-to-null transformation, and nullable handling. The approach of omitting
.required()correctly aligns with the backend DTO structure where all fields are optional and nullable.
44-56: LGTM - Array validation with proper null handling.The connectionUri validation correctly:
- Transforms empty strings to null
- Validates non-null values against JDBC/HTTP patterns
- Uses
.compact()to filter out null entries from the arrayThe test function appropriately returns
truefor null/undefined values since empty entries are handled by the compact step.admin-ui/plugins/services/Components/Configuration/sqlApiMocks.ts (10)
1-3: LGTM! Clean imports with proper dependencies.The imports are well-organized and all dependencies are used appropriately throughout the file.
5-22: Excellent documentation and feature flag design.The comprehensive documentation clearly outlines the migration path from mock to real API implementation, making future integration straightforward. The feature flag pattern is well-implemented.
24-38: Excellent production safety guard.The
throwIfMocksInProductionfunction is a critical safeguard that prevents silent failures and data masking in production environments. This fail-fast approach is exactly the right pattern for mock implementations.
44-57: Excellent use of factory pattern for error handling.The
createMutationErrorHandlerfactory effectively reduces duplication while maintaining flexibility through optional callbacks. The consistent error handling approach (toast + logging + optional callback) is well-designed.
59-70: Well-structured interface with appropriate nullability.The interface correctly marks all fields as optional and nullable, aligning with the pattern where backend DTOs are the source of truth for validation. This prevents frontend validation conflicts when editing existing configurations that may have null values.
Based on learnings, this approach is consistent with other Admin UI forms.
77-79: LGTM! Proper query key management.The query key helper with namespace and
as constensures type safety and consistency across all hooks. The explicit note about avoiding Couchbase collision is helpful for maintainability.
124-163: LGTM! Properly structured mutation hook.The POST hook correctly implements cache invalidation on success and uses the centralized error handler. The pattern of returning the input data as mock response is reasonable for development purposes.
173-212: LGTM! Consistent PUT mutation implementation.The PUT hook follows the same well-established pattern as the POST hook, with appropriate query invalidation and error handling. The consistency across mutation hooks makes the codebase maintainable.
222-261: Verify DELETE operation return type with real API.The hook structure and error handling are correct. However, the mock returns
{ name: string }by echoing the input. When implementing the real API, verify the actual return type—many DELETE endpoints returnvoid, a boolean, or the deleted entity rather than just the identifier.Suggested verification when real API is available:
- Check if the DELETE endpoint returns just the name, void, or the full deleted configuration
- Update the return type accordingly
271-308: LGTM! Correct test hook implementation.The test hook correctly omits query invalidation since it's a read-only test operation that doesn't mutate data. The
voidreturn type is appropriate for a connection test, and the error handling is consistent with other mutations.admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx (2)
219-295: Good: errors/touched are threaded through recursion so nested properties can render field-level errors.
135-217: This concern is not valid.
GluuInlineInputdoes not integrate with Formik directly—it doesn't callsetFieldValueorsetFieldTouched. The component'snameprop serves only for HTML attributes and documentation, while the full nested path is correctly passed via thepathprop (computed at line 69 inJsonPropertyBuilder). The handler receives aJsonPatchwith the complete path, whichApiConfigForm'spatchHandlerconverts to Formik dot notation and uses to updateformik.setFieldValueandformik.setTouchedat the correct location (lines 230-231 inApiConfigForm). Touched/errors tracking is computed correctly using the full path segments, so nested fields will mark touched at the correct location.Likely an incorrect or invalid review comment.
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx
Show resolved
Hide resolved
admin-ui/plugins/services/Components/Configuration/sqlApiMocks.ts
Outdated
Show resolved
Hide resolved
admin-ui/plugins/services/Components/Configuration/utils/validations.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @admin-ui/plugins/services/Components/Configuration/types.ts:
- Around line 6-7: The file imports unused types SqlConfiguration and
CouchbaseConfiguration; remove the import lines that reference these symbols
from types.ts so only necessary types remain imported (i.e., delete "import type
{ SqlConfiguration } ..." and "import type { CouchbaseConfiguration } ...") and
run a quick type-check to ensure no remaining references to those types.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/plugins/auth-server/plugin-metadata.jsadmin-ui/plugins/services/Components/Configuration/types.ts
🧰 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.943Z
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.
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.
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts:4-101
Timestamp: 2026-01-06T13:45:28.303Z
Learning: In the admin-ui Config API Properties forms (ApiConfigForm component), frontend Yup validation uses `.nullable()` for all fields without `.required()` because the backend DTOs are the source of truth and enforce validation. The frontend validation should match what the backend accepts to avoid conflicts when editing existing configurations that may have null values.
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.
📚 Learning: 2026-01-06T13:45:19.278Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts:4-101
Timestamp: 2026-01-06T13:45:19.278Z
Learning: In admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts, ensure Yup schema marks fields as nullable when the backend DTOs permit nulls and do not mark them as required. Align frontend validation with backend validation to prevent conflicts when editing existing configurations with null values; apply this pattern consistently to other Admin UI validation files where the backend is the source of truth.
Applied to files:
admin-ui/plugins/services/Components/Configuration/types.ts
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/plugin-metadata.js (1)
admin-ui/app/redux/sagas/SessionSaga.ts (1)
sessionSaga(44-46)
🔇 Additional comments (2)
admin-ui/plugins/auth-server/plugin-metadata.js (2)
254-254: LGTM! Sagas array reformatted.The single-line formatting is functionally equivalent. All saga generators are correctly invoked.
12-12: LGTM! Import updated to new TypeScript component.The import path correctly points to the new
AuthPagecomponent atadmin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx, which replaces the removedConfigPage.jsas part of the TypeScript migration. AuthPage exports correctly with a default export and includes test coverage.
…ex into admin-ui-issue-2560
…ex into admin-ui-issue-2560
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx`:
- Around line 384-389: The RefreshIcon currently requires an extra click to
apply the search; replace this UX by updating the search filter as the user
types (debounced) or swap the icon to a Search/MagnifyingGlass for an explicit
action. Concretely, remove the onClick on RefreshIcon (or replace the icon
component) and instead wire setFinalSearch to a debounced handler that reads the
changing search state (use a debounced effect or a debounce util called from the
input change handler), referencing the existing RefreshIcon, setFinalSearch, and
search variables so typing automatically (after debounce) updates the filter or
the icon reflects a search action.
In
`@admin-ui/plugins/auth-server/components/Configuration/Properties/utils/validations.ts`:
- Around line 195-201: The boolean field
authorizationChallengeShouldGenerateSession is mistakenly placed under the "//
Strings" section; move the declaration
authorizationChallengeShouldGenerateSession: Yup.boolean().nullable() out of the
strings block and place it under the existing "// Booleans" section near the
other boolean validations (alongside fields like any other Yup.boolean()
entries) so the comments correctly reflect the types.
♻️ Duplicate comments (3)
admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx (3)
313-321: Swallowed error allows success toast after ACR update failure.The error from
putAcrsMutation.mutateAsyncis caught and logged, but execution continues to show a success toast (lines 335-339). WhilehandleAcrUpdateErrordispatches an error toast, the subsequent success toast creates conflicting messages.🐛 Suggested fix
if (put && put.value) { const newAcr = { defaultAcr: put.value || acrs?.defaultAcr } try { await putAcrsMutation.mutateAsync({ data: newAcr }) await logAcrUpdate(newAcr, message, { defaultAcr: newAcr.defaultAcr }) } catch (error) { console.error('Error updating ACR:', error) + // Don't show success toast if ACR update failed + return } }
191-196: Avoid array mutation pattern.
authScripts.push(SIMPLE_PASSWORD_AUTH)mutates the array after creation. While functionally correct since it's a fresh array each render, consider includingSIMPLE_PASSWORD_AUTHin the initial array creation for cleaner code.♻️ Suggested improvement
- const authScripts: string[] = scripts + const authScripts: string[] = [ + ...scripts .filter((item: Script) => item.scriptType === 'person_authentication') .filter((item: Script) => item.enabled) - .map((item: Script) => item.name) - - authScripts.push(SIMPLE_PASSWORD_AUTH) + .map((item: Script) => item.name), + SIMPLE_PASSWORD_AUTH, + ]
168-189: Consider using structuredClone for deep cloning.The
JSON.parse(JSON.stringify(...))pattern for deep cloning (lines 175-180, 286-288) works but is expensive for large configuration objects. Consider usingstructuredClonefor better performance and clarity.♻️ Suggested improvement
if (configuration && Object.keys(configuration).length > 0 && configChanged) { if (patches.length === 0) { - baselineConfigurationRef.current = JSON.parse( - JSON.stringify(configuration), - ) as AppConfiguration - previousConfigurationRef.current = JSON.parse( - JSON.stringify(configuration), - ) as AppConfiguration + baselineConfigurationRef.current = structuredClone(configuration) + previousConfigurationRef.current = structuredClone(configuration)
admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Properties/utils/validations.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx`:
- Around line 345-354: The input's placeholder is hardcoded as "Search..." in
AuthPage.tsx; change it to use the app's translation function (e.g., t or
useTranslation) instead of the literal string, e.g., replace
placeholder="Search..." with placeholder={t('placeholders.search')} and ensure
the translation key placeholders.search is added to the locale files; update
imports in AuthPage.tsx if needed and keep existing handlers (setSearch,
debouncedSetFinalSearch) untouched.
In
`@admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx`:
- Around line 107-122: The nested grid in JsonPropertyBuilder (return block
using ERROR_CONTAINER_STYLE, FormGroup, Col and inner FormGroup with Col
sm={lSize}) is overly complex and fragile; simplify by removing the inner
FormGroup and collapsing to a single FormGroup row that places the error message
in one Col, using Bootstrap offset or a single Col with appropriate sm/offset
values (instead of two Col sm={lSize}) and keep ERROR_COL_STYLE/ERROR_TEXT_STYLE
for styling so layout is consistent across viewports.
In
`@admin-ui/plugins/auth-server/components/Configuration/Properties/utils/validations.ts`:
- Around line 193-198: The two schema fields are under the wrong comment block:
move grantTypesSupportedByDynamicRegistration (currently defined as
Yup.array().of(Yup.string()).nullable()) out of the "// Numbers" section into
the "// Arrays" section, and move authorizationChallengeDefaultAcr (currently
Yup.string().nullable()) into the "// Strings" section; leave
discoveryCacheLifetimeInMinutes (nonNegativeNumber(t)) in the Numbers block and
keep softwareStatementValidationType in the Strings block to maintain consistent
grouping by type.
♻️ Duplicate comments (6)
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx (2)
31-34: Negative margin styling is fragile.The
marginTop: '-1.75rem'approach can cause content overlap across different themes or font sizes. Consider using Bootstrap'sinvalid-feedback d-blockclass pattern for more robust error positioning.
83-86: Strict equality check may hide errors for composite fields.
getIn(touched, formikPathSegments) === truewill returnfalsewhen Formik stores touched as an object/array for composite fields, suppressing error display even when the user has interacted with nested fields.🐛 Proposed fix
const fieldTouched = useMemo(() => { if (!touched) return false - return getIn(touched, formikPathSegments) === true + return Boolean(getIn(touched, formikPathSegments)) }, [touched, formikPathSegments])admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx (4)
152-172: JSON stringify/parse for deep comparison and cloning is expensive.Using
JSON.stringifyfor comparison (line 155) andJSON.parse(JSON.stringify(...))for cloning (lines 158-163) on every configuration change can be costly for large config objects. Consider usingstructuredCloneor a deep-equal utility likelodash.isEqualfor better performance.
173-186: Missing dependency:userActionis created outside useEffect.The
userActionobject (lines 59-62) is referenced inside this effect but not included in the dependency array. SincebuildPayloadmutatesuserAction, this pattern works but is fragile. MoveuserActioncreation inside the effect for clarity.♻️ Suggested fix
useEffect(() => { + const userAction: UserAction = { + action_message: '', + action_data: null, + } const actionPayload: ActionData = {} buildPayload(userAction, FETCHING_JSON_PROPERTIES, actionPayload) dispatch(getJsonConfig()) dispatch( getScripts({ action: (userAction.action_data as Record< string, string | number | boolean | string[] | number[] | boolean[] | null >) || {}, }), ) }, [dispatch])
199-206: Array mutation inside useMemo.
filteredScripts.push(SIMPLE_PASSWORD_AUTH)mutates the array created within the memo. While functionally correct, this is an anti-pattern that could cause subtle bugs if the array were ever memoized differently.♻️ Suggested fix
const authScripts = useMemo(() => { - const filteredScripts: string[] = scripts + return [ + ...scripts .filter((item: Script) => item.scriptType === 'person_authentication') .filter((item: Script) => item.enabled) - .map((item: Script) => item.name) - filteredScripts.push(SIMPLE_PASSWORD_AUTH) - return filteredScripts + .map((item: Script) => item.name), + SIMPLE_PASSWORD_AUTH, + ] }, [scripts])
289-297: Swallowed error in ACR update can lead to conflicting toasts.When
putAcrsMutation.mutateAsyncfails, the error is caught and logged (line 295), but execution continues. Meanwhile,handleAcrUpdateError(lines 100-106) dispatches an error toast. If the audit subsequently succeeds, the user sees both an error toast and a success toast, creating a confusing UX.🐛 Suggested fix
if (put && put.value) { const newAcr = { defaultAcr: put.value || acrs?.defaultAcr } try { await putAcrsMutation.mutateAsync({ data: newAcr }) await logAcrUpdate(newAcr, message, { defaultAcr: newAcr.defaultAcr }) } catch (error) { console.error('Error updating ACR:', error) + throw error // Re-throw to prevent success toast } }
admin-ui/plugins/auth-server/components/Configuration/JsonPropertyBuilder.tsx
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Properties/utils/validations.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx`:
- Around line 334-341: The blocking expression passed to GluuLoader is using
confusing double negation; simplify by computing a clear boolean for empty
configuration (e.g., isConfigEmpty = !configuration ||
Object.keys(configuration).length === 0) and then use blocking={isConfigEmpty ||
acrLoading || putAcrsMutation.isPending} inside the AuthPage component to
replace the current !(!!configuration && Object.keys(configuration).length > 0)
|| acrLoading || putAcrsMutation.isPending expression.
♻️ Duplicate comments (7)
admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx (7)
173-186: MissinguserActionin dependency array creates stale closure.The effect references
userActiondefined at component scope (lines 59-62), butuserActionisn't in the dependency array. SincebuildPayloadmutatesuserActionin place and it's recreated each render, this works but is fragile.See earlier comment about moving
userActioncreation inside this effect.
199-206: Array mutation insideuseMemois fragile.While wrapped in
useMemo, the pattern of creating an array and then mutating it withpush(line 204) is not idiomatic. Consider includingSIMPLE_PASSWORD_AUTHin the initial array construction for cleaner immutable code.♻️ Suggested improvement
const authScripts = useMemo(() => { - const filteredScripts: string[] = scripts + return [ + ...scripts .filter((item: Script) => item.scriptType === 'person_authentication') .filter((item: Script) => item.enabled) - .map((item: Script) => item.name) - filteredScripts.push(SIMPLE_PASSWORD_AUTH) - return filteredScripts + .map((item: Script) => item.name), + SIMPLE_PASSWORD_AUTH, + ] }, [scripts])
292-300: Swallowed error allows success toast on ACR update failure.The error from
putAcrsMutation.mutateAsyncis caught and logged (line 298) but execution continues to lines 312-316 where a success toast may be shown. ThehandleAcrUpdateErrorcallback (lines 100-106) dispatches an error toast via Redux, but this conflicts with the success toast path.Consider re-throwing the error to prevent the success toast:
} catch (error) { console.error('Error updating ACR:', error) + throw error }
281-325: Non-atomic operations: patches and ACR updates can partially succeed.The
handleSubmitfunction dispatchespatchJsonConfig(line 290) and separately awaitsputAcrsMutation.mutateAsync(line 295). If one succeeds and the other fails:
- The Redux dispatch doesn't await completion or check for errors
- The audit log (lines 301-311) runs regardless of patch dispatch result
- User may see success toast when state is partially updated
This was flagged in prior reviews. For a complete fix, consider:
- Awaiting the Redux action result (if using async thunks)
- Rolling back or clearly communicating which operation failed
348-358: Search placeholder should be translated.The placeholder
"Search..."(line 351) is hardcoded in English. For i18n consistency with the rest of the application, use the translation function.<input type="text" className="form-control" - placeholder="Search..." + placeholder={t('placeholders.search')} onChange={(e: React.ChangeEvent<HTMLInputElement>) => {Ensure
placeholders.searchexists in your translation files.
152-172:formikin dependency array causes unnecessary effect executions.The
formikobject is recreated on every render, so including it in the dependency array (line 172) causes this effect to run more frequently than intended. The effect only needs to respond toconfigurationandpatches.lengthchanges.🐛 Suggested fix
- }, [configuration, patches.length, formik]) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [configuration, patches.length])The
formik.resetFormcall is safe because the effect only runs whenconfigurationorpatches.lengthchanges, not based onformikidentity. The eslint disable comment documents this intentional omission.Additionally, as noted in prior reviews, consider replacing
JSON.stringifycomparisons withlodash.isEqualandstructuredClonefor cloning.
59-62: MoveuserActioncreation inside the effect that uses it.The
userActionobject is created on every render but only used inside the mount effect (lines 173-186). This creates a stale closure risk sincebuildPayloadmutatesuserActionin place.♻️ Suggested fix
Move the
userActioncreation inside the effect:useEffect(() => { + const userAction: UserAction = { + action_message: '', + action_data: null, + } const actionPayload: ActionData = {} buildPayload(userAction, FETCHING_JSON_PROPERTIES, actionPayload) dispatch(getJsonConfig()) dispatch( getScripts({ action: (userAction.action_data as Record< string, string | number | boolean | string[] | number[] | boolean[] | null >) || {}, }), ) }, [dispatch])Then remove the top-level declaration (lines 59-62) or keep it only for
handleSubmitusage.
admin-ui/plugins/auth-server/components/Configuration/AuthPage.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/Configuration/AuthPage.tsx`:
- Around line 89-95: The debounced setter created in the useMemo
(debouncedSetFinalSearch using debounce and calling setFinalSearch) is never
cancelled on unmount; add a useEffect that returns a cleanup function which
calls debouncedSetFinalSearch.cancel() to stop the pending debounce callback
when the component unmounts so the timer doesn't fire against an unmounted
component.
- Around line 262-264: The handleBack currently always navigates to
ROUTES.HOME_DASHBOARD; change it to use the history-aware helper by calling
navigateBack(ROUTES.AUTH_SERVER_CONFIG_PROPERTIES) instead of navigateToRoute so
back navigation returns to the previous page when possible and falls back to the
auth-server configuration properties page; update the handleBack callback
(referencing handleBack, navigateToRoute -> navigateBack, and
ROUTES.AUTH_SERVER_CONFIG_PROPERTIES) accordingly.
♻️ Duplicate comments (1)
admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx (1)
283-323: Patch + ACR update can still report success on partial failure.
dispatch(patchJsonConfig(...))isn’t awaited and ACR errors are swallowed, yet audit/success toasts can still fire. This can leave config and audit out of sync. Gate the success/audit path on both operations completing successfully.
admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@admin-ui/app/locales/es/translation.json`:
- Around line 921-929: The new "validation_messages" block duplicates
translations already present under the "validations" namespace; consolidate by
removing or merging "validation_messages" and reusing "validations" keys (e.g.,
map "required_field" → "field_required", keep "invalid_number", "invalid_email",
"invalid_json", and "invalid_url_format" aligned to "invalid_number",
"invalid_email", "invalid_json", and "invalid_url" respectively) and update any
consumers to reference "validations" (or document why both namespaces must
remain). Ensure you update references that read the old "validation_messages"
keys to the canonical "validations" keys or add clear aliases in one place to
avoid duplicate maintenance.
| "validation_messages": { | ||
| "invalid_url_format": "Formato de URL inválido", | ||
| "must_be_non_negative": "Debe ser no negativo", | ||
| "required_field": "Este campo es obligatorio", | ||
| "invalid_number": "Debe ser un número válido", | ||
| "invalid_email": "Debe ser una dirección de correo válida", | ||
| "invalid_json": "Debe ser JSON válido", | ||
| "invalid_pattern": "Formato inválido" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating with existing validations section.
There's already a validations section (lines 1865-1889) in this file with overlapping keys like invalid_email, invalid_number, invalid_json, and field_required. This creates duplication:
New key (validation_messages) |
Existing key (validations) |
|---|---|
required_field |
field_required |
invalid_number |
invalid_number |
invalid_email |
invalid_email |
invalid_json |
invalid_json |
invalid_url_format |
invalid_url |
Consider reusing the existing validations namespace to avoid maintaining duplicate translations, or document why both namespaces are needed (e.g., different consumers).
🤖 Prompt for AI Agents
In `@admin-ui/app/locales/es/translation.json` around lines 921 - 929, The new
"validation_messages" block duplicates translations already present under the
"validations" namespace; consolidate by removing or merging
"validation_messages" and reusing "validations" keys (e.g., map "required_field"
→ "field_required", keep "invalid_number", "invalid_email", "invalid_json", and
"invalid_url_format" aligned to "invalid_number", "invalid_email",
"invalid_json", and "invalid_url" respectively) and update any consumers to
reference "validations" (or document why both namespaces must remain). Ensure
you update references that read the old "validation_messages" keys to the
canonical "validations" keys or add clear aliases in one place to avoid
duplicate maintenance.
|
|
3 participants
feat(admin-ui): convert Auth Server Properties to forms with back button uniformity (#2560)
Summary
The Auth Server Properties screen currently renders backend responses as raw JSON-like data. This prevents proper form validation and also makes it difficult to apply the new Back button uniformity rules across configuration pages.
This PR converts Auth Server Properties into fully structured, validated forms and aligns navigation behavior with the updated Admin UI standards.
Implementation Details
Result
Additional Notes
This work aligns with the parent initiative:
feat(admin-ui): Convert Auth Server Properties and Config API Properties into Forms along with Back button uniformity🔗 Ticket
Closes: #2560
Summary by CodeRabbit
New Features
Refactor
Utilities
Validation
UI
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.