feat(admin-ui): update Config API configuration screen with recent changes (#2550) #2551
Conversation
…ead of marking Not present as Down (#2500)
|
Warning Rate limit exceeded@faisalsiddique4400 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds typed props and disabled/read-only support, refactors ConfigApiConfiguration components to use React hooks/memoization and shared type-guard utilities, restructures utils exports, updates GluuInlineInput with memoized handlers and JsonPatch-based patching, and adjusts GluuTabs null-check logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as ConfigApiPage
participant PatchSvc as patchConfigMutation
participant Audit as logConfigApiUpdate
participant Toast
Note over Page,PatchSvc: Submit flow (useCallback)
User->>Page: click "Submit"
Page->>PatchSvc: mutateAsync(patches)
alt mutation succeeds
PatchSvc-->>Page: result
Page->>Audit: logConfigApiUpdate(result)
alt audit succeeds
Audit-->>Page: audit ok
Page->>Toast: show success
else audit warns/fails
Audit-->>Page: warning/error
Page->>Toast: show warning
end
else mutation fails
PatchSvc-->>Page: throw error
Page->>Toast: show error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
mo-auto
added
comp-admin-ui
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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/ConfigApiConfiguration/ConfigApiPage.tsx (1)
54-66: Consider extracting error message logic to a utility.The error message extraction (lines 55-58) is correct but verbose. This pattern likely exists elsewhere in the codebase and could be centralized.
🔎 Suggested utility extraction
// In a shared utils file function getErrorMessage(error: unknown): string { if (typeof error === 'object' && error !== null && 'message' in error) { return String((error as { message: unknown }).message) } return String(error) }Then in the component:
if (error !== null && error !== undefined) { - const errorText = - typeof error === 'object' && error !== null && 'message' in error - ? String((error as { message: string | number | boolean | null }).message || error) - : String(error) + const errorText = getErrorMessage(error)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx (1)
21-29: Incorrect type for schema properties.The
propertiestypeRecord<string, string | number | boolean | null | undefined>doesn't match the expected OpenAPI schema structure. Schema properties are objects describing field metadata (e.g.,{ type: 'string', items: {...} }), not primitive values.🔎 Proposed fix
interface SpecSchema { components: { schemas: { ApiAppConfiguration: { - properties: Record<string, string | number | boolean | null | undefined> + properties: Record<string, { type?: string; items?: { type?: string; enum?: string[] } }> } } } }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsxadmin-ui/app/routes/Apps/Gluu/GluuTabs.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ConfigApiPage.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/hooks/index.tsadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/componentTypes.tsadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/constants.tsadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/index.tsadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.tsadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/useConfigApiActions.ts
💤 Files with no reviewable changes (1)
- admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/hooks/index.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:43-149
Timestamp: 2025-11-07T12:17:39.857Z
Learning: In the Cedarling configuration UI PR (#2378), the `configApiPolicyStoreUrl` field is intentionally out of scope. It relates to config API configuration and will be covered in a separate PR. The current PR focuses on the Admin UI policy store URL (`auiPolicyStoreUrl`).
📚 Learning: 2025-11-07T12:17:39.857Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:43-149
Timestamp: 2025-11-07T12:17:39.857Z
Learning: In the Cedarling configuration UI PR (#2378), the `configApiPolicyStoreUrl` field is intentionally out of scope. It relates to config API configuration and will be covered in a separate PR. The current PR focuses on the Admin UI policy store URL (`auiPolicyStoreUrl`).
Applied to files:
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.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/Configuration/ConfigApiConfiguration/ConfigApiPage.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/ConfigApiConfiguration/ConfigApiPage.tsx
🧬 Code graph analysis (5)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/componentTypes.ts (1)
admin-ui/app/components/index.tsx (1)
Accordion(107-107)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (1)
JsonPatch(3-3)admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.js (4)
correctValue(27-27)onValueChanged(34-46)onAccept(47-58)onCancel(59-62)admin-ui/app/components/index.tsx (3)
Col(60-60)FormGroup(70-70)Button(40-40)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (2)
admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (1)
schema(58-58)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/componentTypes.ts (1)
SchemaProperty(4-10)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/componentTypes.ts (2)
JsonPropertyBuilderConfigApiProps(12-24)AccordionWithSubComponents(26-29)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (1)
JsonPatch(3-3)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (3)
isBoolean(7-9)isString(11-13)isNumber(3-5)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ConfigApiPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/useConfigApiActions.ts (1)
useConfigApiActions(11-36)admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (2)
handleSubmit(115-128)patches(53-53)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (1)
JsonPatch(3-3)
🔇 Additional comments (14)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (1)
3-5: LGTM!The
isNumberguard is correctly implemented as a pure runtime check.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/componentTypes.ts (1)
23-28: LGTM!The addition of the
disabledprop andAccordionWithSubComponentstype properly supports the new read-only field functionality. The compound component typing pattern is appropriate for Accordion with subcomponents.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/constants.ts (1)
1-14: LGTM!The read-only fields constant is well-structured with proper
readonlytyping. These security-sensitive fields (API credentials, authentication URLs) are appropriate candidates for read-only protection in the UI.admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx (1)
56-65: LGTM!The type guard improvements are sound. The
tab &&check at the beginning correctly short-circuits beforetypeof tab === 'object'is evaluated, so null values are properly handled. The additional validation fortab.pathbeing a non-empty string adds robustness.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/index.ts (1)
1-3: LGTM!Clean barrel export pattern that consolidates the utils module's public API. This simplifies imports for consumers of the ConfigApiConfiguration utilities.
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (1)
142-220: LGTM!The render logic properly handles disabled states across all input variants (text, boolean, array). The memoized filtered values and conditional action button rendering are well-implemented.
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ConfigApiPage.tsx (2)
25-52: LGTM!The
handleSubmitimplementation is well-structured with:
- Proper separation of mutation and audit logging concerns
- Graceful handling of audit failures (warning toast instead of error)
- Comprehensive error handling with localized messages
- Correct
useCallbackdependencies
13-23: LGTM!The component structure is clean with proper hook placement. Based on learnings,
SetTitleis correctly called at the component's top level. The combinedloadingstate simplifies the loader condition.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx (2)
90-106: LGTM - Read-only field handling is well implemented.The iteration over configuration keys with disabled state derived from
READ_ONLY_FIELDSis clean. TheisDisabledcheck and propagation toJsonPropertyBuilderConfigApivia thedisabledprop follows a good pattern.
49-56: The type assertion onpatch.valuecannot be removed; onlypatch.pathassertion is unnecessary.
JsonPatchoperations definevalueasunknown(per RFC 6902), whileGluuCommitDialogOperationrequires it to beJsonValue. The assertion onpatch.pathis redundant since both are typed asstring, but the assertion onpatch.valueis necessary to narrowunknownto the specificJsonValuetype.If removing the
pathassertion:const operations: GluuCommitDialogOperation[] = useMemo( () => patches.map((patch) => ({ - path: patch.path as string, + path: patch.path, value: patch.value as JsonValue, })), [patches], )Likely an incorrect or invalid review comment.
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx (4)
12-24: LGTM - Clean component signature with proper default fordisabled.The destructured props with
disabled = falsedefault and the explicit return type annotation improve readability and type safety.
28-35: LGTM - Good use ofuseMemofor derived values.The memoization of
pathanduniqueIdis appropriate since these are computed from props and used in multiple places including the dependency arrays of other hooks.
50-66: LGTM - Consistentdisabledprop propagation across all input types.The
disabledprop is correctly passed toGluuInlineInputin all four primitive/array type branches (boolean, string, number, string array), ensuring read-only fields are properly disabled throughout the form.Also applies to: 69-85, 87-104, 106-124
146-161: LGTM - Recursivedisabledpropagation to nested components.The
disabledprop is correctly passed down to nestedJsonPropertyBuilderConfigApiinstances in both object array (line 158) and object (line 233) branches, ensuring the read-only state is preserved through the entire component tree.Also applies to: 213-236
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx
Show resolved
Hide resolved
...auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx
Outdated
Show resolved
Hide resolved
...auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx
Outdated
Show resolved
Hide resolved
...auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx
Outdated
Show resolved
Hide resolved
...n-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts
Outdated
Show resolved
Hide resolved
...n-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (1)
166-175: Add type safety for boolean values whenisBooleanis true.Line 172 casts
value as boolean, but the prop type allowsstring | number | boolean | string[]. While most call sites respect the implicit contract of passing boolean values whenisBoolean={true}, this isn't enforced by the type system and could lead to unexpected behavior (e.g., if falsy non-boolean values like0or""are passed toGluuToogle, which usesvalue || false).Consider a discriminated union type to enforce this relationship at compile time:
type GluuInlineInputProps = | { isBoolean: true; value: boolean; /* other props */ } | { isBoolean: false; value: string | number | string[]; /* other props */ }Alternatively, add runtime validation:
{isBoolean && ( <GluuToogle id={name} data-testid={name} name={name} handler={onValueChanged} value={typeof value === 'boolean' ? value : false} disabled={disabled} /> )}
♻️ Duplicate comments (6)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx (2)
34-36: Plain constant for configApiScopes is correct.The code correctly uses a plain constant instead of
useMemosinceCONFIG_API_RESOURCE_IDis a module-level constant that never changes.
63-70: Patch deduplication correctly implemented.The
patchHandlernow filters out existing patches with the same path before appending the new patch, preventing duplicate or conflicting patches. This addresses the previous concern about patch accumulation.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx (2)
25-27: Accordion aliases eliminate duplication.Extracting
AccordionWithSub,AccordionHeader, andAccordionBodyat the module level eliminates the repeated casting and destructuring in theisObjectArrayandisObjectbranches, improving code clarity.
178-254: Remove button correctly handles disabled state.The remove button now includes:
- Visual styling changes when
disabled(gray background, not-allowed cursor, reduced opacity)disabledattribute to prevent clicksaria-disabledattribute for accessibilityThe
disabledprop is also correctly propagated to childJsonPropertyBuilderConfigApiinstances.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (1)
3-17: Type guards correctly separated from schema-based checks.The type guards now perform pure runtime type checks without mixing in schema-based logic, which addresses previous concerns about incorrect type assertions. Specifically:
isStringArrayuses.every()which correctly handles empty arrays (returnstruefor[])- Schema-based rendering decisions are delegated to separate
shouldRenderAs*functionsThis separation of concerns is the correct pattern for type-safe code.
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (1)
110-121: Emptypathfallback still allows unintended root-level patches.This issue from the previous review remains unresolved. Line 116 defaults
pathto an empty string when undefined. In JSON Patch specification, an empty path""targets the document root, which would replace the entire configuration instead of a specific field, potentially causing data corruption.🔎 Proposed fix
const onAccept = useCallback(() => { if (disabled) { return } + if (!path) { + console.error('Cannot apply patch: path is required') + return + } const patch: JsonPatch = { op: 'replace', - path: path || '', + path: path, value: isArray ? correctValue : data, } as JsonPatch handler(patch) setShow((prev) => !prev) - }, [disabled, path, isArray, correctValue, data, handler]) + }, [disabled, path, isArray, correctValue, data, handler])
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsxadmin-ui/package.jsonadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx
📚 Learning: 2025-11-07T12:17:39.857Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:43-149
Timestamp: 2025-11-07T12:17:39.857Z
Learning: In the Cedarling configuration UI PR (#2378), the `configApiPolicyStoreUrl` field is intentionally out of scope. It relates to config API configuration and will be covered in a separate PR. The current PR focuses on the Admin UI policy store URL (`auiPolicyStoreUrl`).
Applied to files:
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.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/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx
🧬 Code graph analysis (4)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (5)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (1)
JsonPatch(3-3)admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.js (4)
correctValue(27-27)onValueChanged(34-46)onAccept(47-58)onCancel(59-62)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (1)
isBoolean(7-9)admin-ui/plugins/admin/__tests__/components/CustomScripts/CustomScriptDetailPage.test.js (1)
name(18-18)admin-ui/app/components/index.tsx (3)
Col(60-60)FormGroup(70-70)Button(40-40)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx (4)
admin-ui/plugins/auth-server/components/Configuration/ConfigPage.js (4)
useAppNavigation(29-29)modal(52-52)operations(54-54)configuration(30-30)admin-ui/app/helpers/navigation.ts (2)
useAppNavigation(173-202)ROUTES(204-204)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (2)
JsonPatch(3-3)ApiAppConfiguration(3-3)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/constants.ts (1)
READ_ONLY_FIELDS(1-14)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/componentTypes.ts (1)
AccordionWithSubComponents(26-29)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (1)
JsonPatch(3-3)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (12)
isBoolean(7-9)shouldRenderAsBoolean(19-21)getBooleanValue(35-43)isString(11-13)shouldRenderAsString(23-25)getStringValue(45-53)isNumber(3-5)shouldRenderAsNumber(31-33)getNumberValue(55-63)isStringArray(15-17)shouldRenderAsStringArray(27-29)getStringArrayValue(65-73)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (1)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/componentTypes.ts (1)
SchemaProperty(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: label PR
🔇 Additional comments (24)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx (5)
13-14: LGTM!The new imports support the disabled field logic and typed operations array, which are essential for the Config API configuration updates.
25-25: LGTM!The widened properties type appropriately reflects the dynamic nature of API configuration properties parsed from the YAML spec.
43-46: LGTM!The memoization of
canWriteConfigApiandoperationsis appropriate for optimizing re-renders and deriving the commit dialog operations from patches.Also applies to: 48-54
88-90: LGTM!Memoizing
handleBackprevents unnecessary re-creation of the callback on each render.
94-110: LGTM!The disabled field logic correctly identifies read-only fields from
READ_ONLY_FIELDSand propagates thedisabledprop toJsonPropertyBuilderConfigApi, ensuring read-only fields cannot be modified in the UI.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/JsonPropertyBuilderConfigApi.tsx (9)
1-1: LGTM!The expanded imports for
useMemo,useCallback, and the utility functions (isNumber,isBoolean, etc.) support the refactored type-checking and value-extraction logic throughout the component.Also applies to: 10-23
29-41: LGTM!The function signature correctly includes the
disabledprop with a default value offalse, enabling read-only field support while maintaining backward compatibility.
45-50: LGTM!Memoizing
pathanduniqueIdavoids unnecessary recomputation of these derived values on every render.Also applies to: 52-52
54-62: LGTM!The memoized
removeHandlercorrectly creates the remove patch and updates the show state. The disabled state is handled via the button'sdisabledattribute rather than in the handler, which is the correct approach.
64-80: LGTM!The boolean rendering logic correctly uses
isBoolean/shouldRenderAsBooleanfor type detection andgetBooleanValuefor value extraction, while properly propagating thedisabledprop to the input.
83-99: LGTM!The string rendering logic correctly uses the type guards and value extractor, with proper
disabledpropagation.
101-118: LGTM!The number rendering logic correctly handles both runtime type checks and schema-based rendering decisions. The
disabledprop is properly propagated to the input.
120-138: LGTM!The string array rendering correctly uses
getStringArrayValuefor both the value and options (when no enum is defined in the schema), with properdisabledpropagation.
140-176: LGTM!The object array rendering uses the extracted Accordion aliases and correctly propagates the
disabledprop to childJsonPropertyBuilderConfigApiinstances, ensuring consistent read-only behavior throughout nested structures.admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (2)
19-33: LGTM!The
shouldRenderAs*helper functions correctly handle schema-based rendering decisions, properly separated from runtime type guards. The null-safe optional chaining ensures safe access to schema properties.
65-73: LGTM!
getStringArrayValueappropriately returns an empty array[]as the default instead ofundefined, which aligns with the expected usage in array-handling contexts.admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (7)
1-9: LGTM: Imports properly support the component refactor.The addition of React hooks (useCallback, useEffect, useMemo) and the JsonPatch type import align well with the component's refactoring to improve performance and type safety.
11-34: LGTM: Well-defined TypeScript interfaces.The interfaces provide strong typing for component props and context, improving type safety and developer experience. The
valueprop union type correctly accommodates both primitive and array values for different input modes.
52-62: LGTM: Array guard resolves previous type mismatch concern.The
useEffectnow includes!Array.isArray(value)guard (line 59), which properly prevents array values from being assigned to thedatastate. This addresses the previous review comment about type mismatch between thedatastate type and thevalueprop type.
64-77: LGTM: Properly memoized and disabled-aware callback.The
onValueChangedcallback correctly usesuseCallbackfor optimization, includes a disabled guard, and properly handles both boolean and text input changes with appropriate typing.
79-108: LGTM: Memoized callback with proper type guards.The callback correctly uses
useCallback, includes disabled guard, and properly filters and processes both string and object selections. The special handling fortokenEndpointAuthMethodsSupported(lines 91-102) accommodates specific API field structure requirements.
123-141: LGTM: Clean callback and memoized helper values.The
onCancelcallback,disabledStyle,filteredValue, andfilteredOptionsare all properly implemented with correct memoization and dependencies. The filtering logic prevents potential rendering issues with null/undefined values.
176-220: LGTM: Well-structured JSX with proper disabled state handling.The Typeahead configuration correctly uses filtered values and options, and the action buttons are properly hidden when disabled (line 194). The self-closing icon tag (line 202) is a minor code quality improvement.
admin-ui/package.json (1)
81-82: No action required. The @typescript-eslint upgrade to v8.51.0 is fully compatible with TypeScript 5.3.3 and ESLint 8.37.0. The project's .eslintrc.js configuration is already compliant and does not use any deprecated rules from the v7→v8 migration (prefer-ts-expect-error, no-var-requires, no-throw-literal).
...n-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (1)
110-121: Unresolved: Empty path fallback could cause root-level patches.Line 116 still defaults
pathto an empty string when undefined. In JSON Patch, an empty path targets the document root, which would replace the entire configuration instead of a specific field. This was flagged in a previous review but hasn't been addressed.🔎 Proposed fix
const onAccept = useCallback(() => { if (disabled) { return } + if (!path) { + console.error('GluuInlineInput: Cannot apply patch without a valid path') + return + } const patch: JsonPatch = { op: 'replace', - path: path || '', + path: path, value: isArray ? correctValue : data, } as JsonPatch handler(patch) setShow((prev) => !prev) - }, [disabled, path, isArray, correctValue, data, handler]) + }, [disabled, path, isArray, correctValue, data, handler])
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx
🧬 Code graph analysis (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (1)
JsonPatch(3-3)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (1)
isBoolean(7-9)admin-ui/app/components/index.tsx (3)
Col(60-60)FormGroup(70-70)Button(40-40)
🔇 Additional comments (5)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (5)
1-1: Good addition of hooks and strongly-typed interfaces.The imports of React hooks and JsonPatch type, along with the new GluuInlineInputProps and ThemeContextValue interfaces, improve type safety and enable proper memoization. The narrowing of the
typeprop (line 14) to a specific union addresses the previous review concern about accepting any string.Also applies to: 9-9, 11-34
58-62: Good array guard in useEffect.The array guard on line 59 properly prevents array values from being assigned to the
datastate, addressing part of the previous review concern. This ensures type safety for updates after the initial render.
64-77: Well-implemented event handlers with consistent disabled guards.Both
onValueChangedandhandleTypeAheadChangeproperly useuseCallbackfor memoization and consistently check thedisabledstate before processing changes. The logic correctly handles boolean toggles, string/number inputs, and complex typeahead selections with mixed object/string inputs.Also applies to: 79-108
128-141: Effective use of useMemo for performance and data sanitization.The memoized
disabledStyle,filteredValue, andfilteredOptionsimprove performance and prevent null/undefined values from causing issues in the Typeahead component. Dependencies are correctly specified for all three memos.
154-220: Render logic properly implements disabled state and uses memoized values.The render logic consistently applies the
disabledstate and styling across all input types (regular inputs, boolean toggles, and typeahead arrays). The use of memoizedfilteredValueandfilteredOptionsprevents rendering issues with null/undefined values, and action buttons are correctly hidden when disabled.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (1)
112-123: Emptypathfallback could cause unintended root-level patches.Line 118 defaults
pathto an empty string when undefined. In JSON Patch, an empty path targets the document root, which would replace the entire configuration instead of a specific field. This issue was previously flagged and remains unresolved.🔎 Proposed fix
const onAccept = useCallback(() => { if (disabled) { return } + if (!path) { + console.error('Cannot apply patch: path is required') + return + } const patch: JsonPatch = { op: 'replace', - path: path || '', + path: path, value: isArray ? correctValue : data, } as JsonPatch handler(patch) setShow((prev) => !prev) -}, [disabled, path, isArray, correctValue, data, handler]) +}, [disabled, path, isArray, correctValue, data, handler])
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:43-149
Timestamp: 2025-11-07T12:17:39.857Z
Learning: In the Cedarling configuration UI PR (#2378), the `configApiPolicyStoreUrl` field is intentionally out of scope. It relates to config API configuration and will be covered in a separate PR. The current PR focuses on the Admin UI policy store URL (`auiPolicyStoreUrl`).
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:43-149
Timestamp: 2025-11-07T12:17:39.857Z
Learning: In the Cedarling configuration UI PR (#2378), the `configApiPolicyStoreUrl` field is intentionally out of scope. It relates to config API configuration and will be covered in a separate PR. The current PR focuses on the Admin UI policy store URL (`auiPolicyStoreUrl`).
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx
🧬 Code graph analysis (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (2)
admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.js (4)
correctValue(27-27)onValueChanged(34-46)onAccept(47-58)onCancel(59-62)admin-ui/app/components/index.tsx (3)
Col(60-60)FormGroup(70-70)Button(40-40)
🔇 Additional comments (9)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (9)
11-28: LGTM! Well-typed props interface.The interface properly defines all component props with appropriate types. The
typeprop is now correctly restricted to valid HTML input types, and thehandleris properly typed to acceptJsonPatch, addressing previous type safety concerns.
56-58: Good fix for falsy value handling.The state initialization now correctly handles falsy primitives (like
falseor0) by checkingvalue !== undefinedinstead of the previous truthy check. This ensures the actual value is preserved rather than defaulting to an empty string.
60-64: Proper value synchronization.The effect correctly synchronizes the internal state with prop changes while maintaining type safety by excluding array values.
66-79: Clean event handler with proper disabled guard.The callback correctly prevents updates when disabled and appropriately handles both boolean and text input changes.
81-110: Type-safe Typeahead handler with proper guards.The callback correctly:
- Guards against disabled state
- Uses type predicates to separate object and string selections
- Validates string types before aggregating values
- Handles the
tokenEndpointAuthMethodsSupportedfield's special structureThe implementation is appropriate for this reusable component.
125-143: Good use of React optimization hooks.The memoized values efficiently filter null/undefined entries and compute disabled styling only when dependencies change. The cancel callback is clean and properly resets state.
156-167: Correct input handling with disabled support.The
String(data)conversion safely handles both string and number values, and the disabled state is properly applied to both the input's native disabled prop and visual styling.
168-191: Boolean and array branches properly handle disabled state.The type casts are safe given the
isBooleanandisArrayguards, and both GluuToogle and Typeahead correctly receive thedisabledprop along with filtered values to prevent null/undefined issues.
196-218: Action buttons correctly hidden when disabled.The condition properly prevents the save/cancel UI from rendering when the component is disabled, maintaining consistent disabled behavior throughout.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (2)
204-215: Inconsistent icon tag style.Line 204 uses a self-closing
<i />tag while line 215 uses<i></i>. Both are functionally identical, but consistency would improve code style.🔎 Suggested fix
- <i className="fa fa-times me-2"></i> + <i className="fa fa-times me-2" />
156-167: Change Input to usevalueprop instead ofdefaultValuefor proper re-render handling.The Input component at line 162 uses
defaultValue={String(data)}, which only applies the initial value. When thevalueprop changes from the parent, theuseEffect(lines 60-64) correctly updates thedatastate, but the Input won't visually reflect this change becausedefaultValueis ignored on re-renders. This creates a mismatch where the internal state and DOM become out of sync.Use
value={String(data)}instead to make this a fully controlled component that updates whendatachanges.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:43-149
Timestamp: 2025-11-07T12:17:39.857Z
Learning: In the Cedarling configuration UI PR (#2378), the `configApiPolicyStoreUrl` field is intentionally out of scope. It relates to config API configuration and will be covered in a separate PR. The current PR focuses on the Admin UI policy store URL (`auiPolicyStoreUrl`).
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.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/app/routes/Apps/Gluu/GluuInlineInput.tsx
🧬 Code graph analysis (1)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/types/configApiTypes.ts (1)
JsonPatch(3-3)admin-ui/plugins/auth-server/components/Configuration/DefaultAcrInput.js (4)
correctValue(27-27)onValueChanged(34-46)onAccept(47-58)onCancel(59-62)admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/utils/typeGuards.ts (1)
isBoolean(7-9)
🔇 Additional comments (6)
admin-ui/app/routes/Apps/Gluu/GluuInlineInput.tsx (6)
1-9: LGTM: TypeScript and React hooks properly imported.The additions of
useCallback,useEffect,useMemohooks and theJsonPatchtype align well with the refactoring to add type safety and performance optimizations.
11-34: LGTM: Strong typing with proper prop interfaces.The
GluuInlineInputPropsinterface provides comprehensive type safety with appropriate types for each prop. Thetypeprop is correctly restricted to valid HTML input types, and the newdisabledprop supports the enable/disable semantics mentioned in the PR objectives.
56-64: LGTM: Proper state initialization and synchronization.The state initialization correctly guards against array values and handles falsy primitives (
false,0) appropriately. The useEffect properly synchronizes the localdatastate with externalvalueprop changes.
66-79: LGTM: Proper event handler with disabled guard.The
onValueChangedcallback is correctly memoized and includes appropriate disabled state handling. The logic properly distinguishes between boolean (checkbox) and text input value extraction.
81-110: LGTM: Robust Typeahead handler with type safety.The callback properly handles mixed arrays of strings and objects using type guards. The disabled guard prevents changes when the component is disabled. The special handling for
tokenEndpointAuthMethodsSupportedis appropriate for the field's unique structure.
125-143: LGTM: Proper memoization for callbacks and computed values.The
onCancelcallback and memoized values (disabledStyle,filteredValue,filteredOptions) are correctly implemented. The null filtering infilteredValueandfilteredOptionsprevents potential runtime issues with null/undefined array items.
|
4 participants
feat(admin-ui): update Config API configuration screen with recent changes (#2550)
Summary
This PR updates the Config API configuration screen in the Admin UI to align with the latest backend changes.
Fix Summary
Reference
Closes
Closes: #2550
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.