fix(admin-ui): Use typescript generated client for Auth Server Logging pages #2463
Conversation
syntrydy
added
comp-admin-ui
|
Warning Rate limit exceeded@moabu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 27 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)
📝 WalkthroughWalkthroughReplaces Redux-driven logging state with React Query API hooks, adds permission gating and an audit-logging hook, introduces merge/diff utilities and stricter validations, and updates tests to run under a QueryClientProvider with JansConfigApi hook mocks. (28 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Page as LoggingPage
participant Auth as Redux (auth state)
participant Perm as Permission Init
participant Q as React Query
participant API as Config API
participant Audit as useLoggingActions
Page->>Auth: read token, client_id, userinfo
Page->>Perm: init(LOGGING_READ, LOGGING_WRITE)
Perm-->>Page: permissionsInitialized / permissionError
alt permissions OK
Page->>Q: useGetConfigLogging (enabled)
Q->>API: GET /config/logging
API-->>Q: logging data
Q-->>Page: populate localLogging / form
else permissions fail
Page-->>User: render permission error UI
end
User->>Page: submit form
Page->>Page: merged = getMergedValues(orig, updated)
Page->>Page: changed = getChangedFields(orig, updated)
Page->>Q: usePutConfigLogging.mutateAsync(merged)
Q->>API: PUT /config/logging
API-->>Q: updated logging
Q-->>Page: update localLogging
rect rgba(76,175,80,0.08)
Page->>Audit: logLoggingUpdate(msg, changed) [non-blocking]
Audit->>API: POST /audit/action
API-->>Audit: ack
end
Page-->>User: show toast / updated UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.test.tsx(2 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(8 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts(1 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts(1 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/validations.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/validations.ts (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (2)
LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (3)
admin-ui/app/utils/TokenController.ts (1)
addAdditionalData(49-124)admin-ui/app/audit/Resources.ts (1)
API_LOGGING(7-7)admin-ui/app/redux/api/backend-api.ts (1)
postUserAction(51-69)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(31-36)useLoggingActions(58-121)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (4)
LoggingFormValues(15-21)getLoggingInitialValues(38-56)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)
🔇 Additional comments (19)
admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (4)
1-22: LGTM! Well-structured type definitions.The use of
as constfor literal type inference and clear separation between nullable API types (LoggingConfigLike) and non-nullable form types (LoggingFormValues) follows best practices.
23-33: LGTM! Proper type guard implementation.The type guards correctly validate runtime values with both type checking and array inclusion, providing safe narrowing for TypeScript.
35-56: LGTM! Robust initialization with proper validation.The function correctly handles null/undefined inputs, validates logging levels and layouts using the type guards, and explicitly converts nullable booleans. This prevents invalid states in the form.
72-91: LGTM! Type-safe change detection.The function correctly returns
Partial<T>and uses shallow equality comparison, which is appropriate for the primitive values in logging configuration. The null handling is safe.admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
1-36: LGTM! Well-defined types for audit logging.The type definitions clearly structure the audit log payload and expose
ModifiedFieldsfor external use, supporting proper type safety across the module boundary.
70-116: No verification issues found.The code at line 86 correctly constructs the Authorization header. The
access_tokenstored in Redux is a raw token value without the "Bearer " prefix, and the code properly prepends "Bearer " when creating the header. This pattern is consistent across the entire codebase (50+ locations), confirming the implementation is correct.admin-ui/plugins/auth-server/components/Configuration/Defaults/validations.ts (1)
1-14: LGTM! Proper validation schema with type safety.The validation schema correctly imports and uses the logging constants for
oneOfvalidation, ensuring runtime validation matches the TypeScript type definitions. The addition of required boolean validations aligns with the expandedLoggingFormValuesinterface.admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.test.tsx (3)
1-58: LGTM! Proper test setup for React Query integration.The test correctly:
- Mocks the API hooks at module level
- Configures QueryClient with
retry: falseto prevent test flakiness- Expands AUTH_STATE to include all fields required by the new hook-based audit logging
The test infrastructure properly supports the refactored data flow.
60-70: LGTM! Proper provider composition.The
Wrappercomponent correctly composes multiple providers in the right order, ensuring both React Query and Redux state are available to the tested component.
72-99: LGTM! Test correctly verifies React Query integration.The test properly:
- Dynamically imports and mocks the API hooks
- Sets up appropriate return values for both query and mutation hooks
- Verifies that the UI renders the fetched data correctly
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (9)
1-54: LGTM! Clean refactor to React Query with proper type definitions.The imports reflect the migration from Redux-based data management to React Query hooks. The local TypeScript interfaces provide clear type safety for component state.
56-77: LGTM! Proper permission-gated data fetching.The query is correctly disabled until permissions are initialized and the user has the required read permission (line 74). This prevents unnecessary API calls and ensures proper authorization flow.
79-109: LGTM! Robust permission initialization with proper cleanup.The permission initialization:
- Uses parallel authorization checks for better performance (line 85)
- Implements the isMounted pattern to prevent memory leaks
- Gracefully handles errors by still setting
permissionsInitializedto unblock the UI (line 93)- Properly cleans up on unmount
105-118: LGTM! Efficient form initialization.The component properly:
- Syncs server data to local state (lines 106-108)
- Memoizes initial form values with correct dependency (line 112)
- Uses constants directly from utils for dropdown options
120-164: LGTM! Robust form submission with proper error handling.The form submission flow is well-designed:
- handleSubmit: Validates data is loaded (line 122), prepares merged/changed values, and shows confirmation dialog
- handleAccept:
- Updates API first and uses server response as source of truth (lines 144-147)
- Audit logging is non-blocking to prevent submission failure from audit issues (lines 150-152)
- Keeps dialog open on API errors so users can retry (lines 158-161)
Note: The
getMergedValuescall on line 127 is affected by the type safety issue flagged in utils.ts, but runtime behavior should be safe sincelocalLoggingis null-checked.
166-179: LGTM! Proper loading state composition.Line 166 correctly combines the data fetching loading state with the mutation pending state, ensuring the UI is blocked during both operations.
195-197: LGTM! Explicit event handler typing.The event handlers properly use explicit
React.ChangeEvent<HTMLInputElement>typing, providing better type safety and preventing common event-related bugs.Also applies to: 223-225, 240-242, 251-253
262-264: Consider adding explicit event type for consistency.While the handler on line 263 works correctly, it's missing the explicit
React.ChangeEvent<HTMLInputElement>type annotation that other handlers have (lines 195, 223, 240, 251). Consider adding it for consistency:- handler={(e: React.ChangeEvent<HTMLInputElement>) => + handler={(e: React.ChangeEvent<HTMLInputElement>) => formik.setFieldValue('enabledOAuthAuditLogging', e.target.checked) }Wait, looking at the code again, line 262 already has the type annotation. Let me re-examine... Actually, it appears line 263 doesn't have the explicit type shown in the diff, but it might be inferred. Let me check the actual line numbers again.
Actually, reviewing the code more carefully, I see that line 262 shows:
handler={(e: React.ChangeEvent<HTMLInputElement>) =>So it does have the explicit typing. This is already consistent. I should not flag this.
[actions]
Skip this comment[scratchpad_end] -->
180-306: LGTM! Well-structured form with proper permission gating.The form implementation:
- Uses Formik for form management with proper validation
- Implements consistent event handler typing across all fields
- Properly gates the form footer behind write permissions (line 271)
- Correctly passes the composed
isLoadingstate to the footer (line 288)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (1)
31-34: AlignPendingValues.changedFieldswithgetChangedFieldsresult andModifiedFields
PendingValuesdefines:interface PendingValues { mergedValues: Logging changedFields: ModifiedFields }while
changedFieldsis produced via:const mergedValues = getMergedValues(localLogging, values) const changedFields = getChangedFields(localLogging, mergedValues) setPendingValues({ mergedValues, changedFields })Given the current
getChangedFieldsimplementation,changedFieldsisPartial<Logging>(new values only), not{ oldValue; newValue }. This both disagrees with theModifiedFieldsinterface and will fail type‑checking oncegetChangedFieldsis tightened.After updating
getChangedFieldsas suggested inutils.tsto emit{ oldValue, newValue }, this code will naturally line up:changedFieldswill have a structure compatible withModifiedFields, and you won’t need casts. Until then, there is a real risk of either compile‑time errors or incorrect audit payloads.#!/bin/bash # Focus on this call chain to ensure all typings are consistent. rg -n "PendingValues" admin-ui/plugins/auth-server/components/Configuration/Defaults -C3 rg -n "logLoggingUpdate\(" admin-ui/plugins/auth-server/components/Configuration/Defaults -C3Also applies to: 100-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(8 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts(1 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (3)
admin-ui/app/utils/TokenController.ts (1)
addAdditionalData(49-124)admin-ui/app/audit/Resources.ts (1)
API_LOGGING(7-7)admin-ui/app/redux/api/backend-api.ts (1)
postUserAction(51-69)
admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (1)
admin-ui/app/helpers/utility.js (1)
changed(3-3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(15-20)useLoggingActions(46-109)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (4)
LoggingFormValues(15-21)getLoggingInitialValues(38-56)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)
🔇 Additional comments (4)
admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (1)
23-55: Logging level/layout guards and initial values look solidThe type guards and
getLoggingInitialValueslogic are consistent with the declared unions, handlenull/undefinedsafely, and provide deterministic defaults. No issues from a correctness or typing perspective.admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (1)
15-41:ModifiedFieldsinterface is clear and compatible with suggestedgetChangedFieldschangeThe
ModifiedFieldsshape here ({ oldValue; newValue }per key) aligns with the proposed change togetChangedFieldsinutils.ts. OncegetChangedFieldsis updated to emit{ oldValue, newValue }, the structural typing will match and audit logs will carry both before/after values as intended.admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (2)
116-144: Audit logging after mutation is non‑blocking and generally correctThe submit/accept flow looks good:
handleSubmitcomputes merged values and changed fields, then opens the commit dialog.handleAcceptawaitsusePutConfigLogging().mutateAsync, updateslocalLoggingfrom the server response, and only then fireslogLoggingUpdatein a fire‑and‑forget way (.catchlogs any failure).This ensures API updates drive the source of truth and audit logging failures don’t block the user. If you adopt the updated
getChangedFieldsshape,changedFieldshere will also include meaningful old/new pairs for the audit trail.
268-269: Nice touch wiring form footer loading to mutation/query statePassing
isLoadingdown toGluuFormFooterensures the apply button reflects both data fetch and mutation states and aligns visually with the top‑levelGluuLoader. No issues here.
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(8 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts(1 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Defaults/hooks/useLoggingActions.tsadmin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.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/Defaults/LoggingPage.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (3)
admin-ui/app/utils/TokenController.ts (1)
addAdditionalData(49-124)admin-ui/app/audit/Resources.ts (1)
API_LOGGING(7-7)admin-ui/app/redux/api/backend-api.ts (1)
postUserAction(51-69)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(9-14)useLoggingActions(33-86)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (4)
LoggingFormValues(15-21)getLoggingInitialValues(29-47)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)
🔇 Additional comments (4)
admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (1)
23-69: Logging helpers and diff logic look sound and type-safeThe new validators, initialization logic, and merge/diff helpers are consistent with the generated
Loggingtype usage inLoggingPage.tsxand withModifiedFieldsinuseLoggingActions.ts. Shallow diffing with old/new pairs is appropriate for these scalar fields, and requiring a non-nulloriginalingetMergedValuesremoves the earlier unsafe cast.admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (1)
9-31: ModifiedFields and AuditLog shapes align with callersThe
ModifiedFieldsmap andAuditLogstructure line up withgetChangedFieldsandaddAdditionalData/postUserActionusage, giving you a clean, typed surface for audit logging. This is a solid consolidation of the logging-audit concerns.admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (2)
40-51: Permissions gating and loader behavior are well‑structuredInitializing both
LOGGING_READandLOGGING_WRITEviaauthorizewith anisMountedguard, and folding!permissionsInitializedintoisLoading, avoids flashes of unauthorized/empty UI and respects the earlier guidance about keepingconsole.errorin catch blocks for diagnostics. The queryenabledcondition andGluuViewWrapperpermission gate work together cleanly.Also applies to: 52-76, 132-136
88-90: Formik wiring, event typings, and footer loading flag look correctReusing
LOG_LEVELS/LOG_LAYOUTSfor dropdown options, updating the select handlers toChangeEvent<HTMLSelectElement>, keeping checkbox toggles asChangeEvent<HTMLInputElement>, and passing the combinedisLoadinginto bothGluuLoaderandGluuFormFootermakes the form interactions and loading UX consistent and type‑aligned with the underlying components.Also applies to: 155-163, 183-191, 203-209, 215-219, 225-230, 254-255
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(7 hunks)admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Defaults/hooks/useLoggingActions.tsadmin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.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/Defaults/hooks/useLoggingActions.tsadmin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (3)
admin-ui/app/utils/TokenController.ts (1)
addAdditionalData(49-124)admin-ui/app/audit/Resources.ts (1)
API_LOGGING(7-7)admin-ui/app/redux/api/backend-api.ts (1)
postUserAction(51-69)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(9-14)useLoggingActions(33-91)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (6)
LoggingFormValues(15-21)getLoggingInitialValues(29-47)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)getMergedValues(49-54)getChangedFields(56-69)
🔇 Additional comments (4)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (1)
1-91: LGTM! Audit logging hook is well-implemented.The hook correctly:
- Validates authorization token and user information before constructing audit logs.
- Handles the
-1sentinel return value frompostUserActionby throwing an explicit error.- Uses
addAdditionalDatawithomitPayload: trueand includesmodifiedFieldsin the action data, aligning with established audit patterns.- Applies proper error handling with
console.errorand rethrows to allow callers to treat audit as non-blocking.- Memoizes the callback with appropriate dependencies.
The implementation addresses all previous review feedback and follows the codebase patterns. Based on learnings, the
console.errorcalls in catch blocks are appropriate for debugging.admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
138-138: Loading state properly reflects all initialization phases.The
isLoadingcalculation now includes!permissionsInitialized, which addresses the previous review feedback. This ensures the loader blocks the UI during permission checks, data fetching, and mutation operations, providing a consistent loading experience.
167-169: Event handler types are correct.The event handlers use appropriate DOM event types:
React.ChangeEvent<HTMLSelectElement>forCustomInput type="select"(lines 167, 195)React.ChangeEvent<HTMLInputElement>forGluuToogleRowcheckboxes (lines 212, 223, 234)This provides proper type safety and addresses previous feedback about event typing.
Also applies to: 195-197, 212-214, 223-225, 234-236
102-104: No-op guard prevents unnecessary commits.The guard correctly skips the commit dialog and audit logging when no fields have changed, improving UX by avoiding unnecessary operations. This implements the optional refactor suggested in previous review feedback.
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Defaults/LoggingPage.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/Defaults/LoggingPage.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(9-14)useLoggingActions(33-91)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (6)
LoggingFormValues(15-21)getLoggingInitialValues(29-47)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)getMergedValues(49-54)getChangedFields(56-69)
🔇 Additional comments (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
10-17: Good consolidation of logging utilities, types, and pending stateUsing
LOG_LEVELS/LOG_LAYOUTS,getLoggingInitialValues, and the sharedLoggingFormValuestype keeps the form tightly aligned with the API surface, and thePendingValueswrapper cleanly couples the serverLoggingpayload with auditModifiedFields. This removes manual duplication and will help avoid drift between the UI and the generated client types.Also applies to: 27-33, 87-90, 92-93
41-45: Permissions, loading state, and gated rendering are wired coherentlyThe combination of:
- explicit permission initialization via
authorizewith anisMountedguard,permissionsInitializedfolded intoisLoading,enabled: permissionsInitialized && hasCedarPermission(LOGGING_READ)on the query,- a dedicated
permissionErrorUI branch, andGluuViewWrapper+hasCedarPermission(LOGGING_READ/WRITE)gives a clear, fail-closed permissions story and avoids UI flashes while auth/logging config are in flight. Passing
isLoadingthrough to bothGluuLoaderandGluuFormFooteralso keeps the spinner and buttons in sync.Also applies to: 47-52, 54-79, 145-146, 147-159, 161-166, 264-282
188-190: Event typings now match control types and Formik integrationUpdating the handlers to use
React.ChangeEvent<HTMLSelectElement>for theCustomInput type="select"controls andReact.ChangeEvent<HTMLInputElement>for the toggle rows aligns the TS types with the underlying DOM elements while keeping the FormiksetFieldValuecalls straightforward. This should eliminate the earlier typing friction around these handlers without introducingany.Also applies to: 216-218, 233-235, 244-246, 255-257
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Show resolved
Hide resolved
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 (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (1)
191-207: Formik handlers and control typings are consistent; optional minor typing polishThe select and toggle handlers now match their underlying DOM elements:
ChangeEvent<HTMLSelectElement>forCustomInput type="select",ChangeEvent<HTMLInputElement>forGluuToogleRowhandlers,and all updates flow through
formik.setFieldValue, withLOG_LEVELS/LOG_LAYOUTSdriving the options. PassingisLoadingintoGluuFormFooterin addition to the outerGluuLoaderkeeps the footer in sync with permission/query/mutation state.If you want to tighten types further, you could cast
e.target.valueto the specific union (e.g.,as LoggingFormValues['loggingLevel']) to retain strong typing onLoggingFormValues, but that’s strictly optional given Formik’ssetFieldValuesignature.Also applies to: 218-235, 246-273, 280-299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Defaults/LoggingPage.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/Defaults/LoggingPage.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(9-14)useLoggingActions(33-91)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (6)
LoggingFormValues(15-21)getLoggingInitialValues(29-47)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)getMergedValues(49-54)getChangedFields(56-69)
🔇 Additional comments (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
10-33: Type-safe wiring to utils and JansConfigApi aligns with PR goalsImports of
LOG_LEVELS,LOG_LAYOUTS,getLoggingInitialValues,getMergedValues,getChangedFields, andLoggingFormValuesplus theLoggingtype andModifiedFields/PendingValuesgive the page a clean, strongly typed contract with the generated client and audit hook. This is exactly the direction described in the PR objectives; no issues spotted here.
91-98: Submit/commit/audit pipeline is well-structured and robustThe form lifecycle is clean:
initialValuesderived viagetLoggingInitialValues(localLogging)gives safe defaults while still reinitializing when server data arrives.handleSubmitcorrectly guards onlocalLogging, merges viagetMergedValues, computes a precise diff withgetChangedFields, and skips no-op commits.PendingValuescleanly carries bothmergedValuesandchangedFieldsintohandleAccept.handleAcceptawaits the mutation, refresheslocalLoggingfrom the server result, fires non-blockinglogLoggingUpdate(userMessage, changedFields).catch(...), and surfaces success/error via toasts while keeping the commit dialog open on failure.The explicit
console.errorcalls in the permission init and both update/audit error paths are appropriate for unexpected failures and align with previous guidance about keeping full error objects in logs. Based on learnings.Also applies to: 100-118, 120-145
147-175: Combined loading and error branches prevent “fake editable” statesDeriving
isLoadingfrom!permissionsInitialized || isLoadingData || updateLogging.isPendingand then short-circuiting onloggingErrorandpermissionErrorbefore rendering the editable form avoids the earlier issue where users could see a form that could never successfully save. The error UIs mirror the existing permission error pattern and keep the UX consistent.
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Defaults/LoggingPage.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/Defaults/LoggingPage.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(9-14)useLoggingActions(33-91)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (6)
LoggingFormValues(15-21)getLoggingInitialValues(29-47)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)getMergedValues(49-54)getChangedFields(56-69)
🔇 Additional comments (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (1)
47-172: Solid error handling, loading states, and permission flow.The implementation properly addresses all major concerns:
- Error handling: Both
loggingError(lines 146-158) andpermissionError(lines 160-172) are surfaced with clear error UI blocks, preventing the "fake editable" state.- Loading states:
isLoading(line 144) correctly combines permission initialization, data fetch, and mutation states to block the UI appropriately.- Permission gating: The pattern of checking
readResult.isAuthorizedexplicitly (lines 67-70) to block the page while relying onhasCedarPermission(LOGGING_WRITE)(line 277) to gate write actions cleanly supports read-only access.- Submit guards: The defensive check at lines 99-102 prevents submission when data isn't loaded.
- User feedback: Toast notifications (lines 132, 138) provide clear success/error messages.
- Audit logging: The non-blocking pattern with
catch(console.error)(lines 128-130) aligns with the learning about keeping error details for debugging.- Data flow: The merge/diff logic (lines 104-105) and early return for no-op changes (lines 107-109) ensure clean state management and precise audit trails.
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Defaults/LoggingPage.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/Defaults/LoggingPage.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (3)
admin-ui/plugins/auth-server/components/Configuration/Defaults/hooks/useLoggingActions.ts (2)
ModifiedFields(9-14)useLoggingActions(33-91)admin-ui/app/utils/PermChecker.ts (2)
LOGGING_READ(73-73)LOGGING_WRITE(74-74)admin-ui/plugins/auth-server/components/Configuration/Defaults/utils.ts (6)
LoggingFormValues(15-21)getLoggingInitialValues(29-47)LOG_LEVELS(1-1)LOG_LAYOUTS(2-2)getMergedValues(49-54)getChangedFields(56-69)
🔇 Additional comments (6)
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx (6)
10-18: Integration with JansConfigApi hooks and shared logging utils looks correctUsing
LOG_LEVELS/LOG_LAYOUTS,LoggingFormValues,getLoggingInitialValues,getMergedValues,getChangedFields, and the generateduseGetConfigLogging/usePutConfigLogginghooks plususeLoggingActionscleanly replaces the old Redux/manual types and aligns with the PR objectives.Please re-run the existing
LoggingPage.test.tsxsuite to confirm the generated client types stay in sync with the backend contract after this change.Also applies to: 21-22, 27-33
35-45: Component typing and core local state wiring are consistentDeclaring
LoggingPage(): React.ReactElementand introducingpendingValues,localLogging,permissionsInitialized, andpermissionErrorprovides a clear separation of concerns between form state, loaded config, and permission lifecycle. No functional issues spotted here.
82-86: Merge/diff + pendingValues submit flow looks correct and resilientUsing
localLoggingas the source of truth, derivinginitialValuesviagetLoggingInitialValues(localLogging), then computingmergedValues = getMergedValues(localLogging, values)andchangedFields = getChangedFields(localLogging, mergedValues)gives you both a fullLoggingpayload for the API and a precise diff for auditing. The guards for!localLoggingandObject.keys(changedFields).length === 0prevent invalid submissions and no-op commits, andpendingValuescleanly passes data into the commit dialog.Also applies to: 88-91, 93-94, 97-115
117-142: Update + audit logging pipeline correctly separates concerns and user feedback
handleAcceptawaitingupdateLogging.mutateAsync, updatinglocalLoggingfrom the server result, firinglogLoggingUpdate(userMessage, changedFields).catch(...)non-blockingly, and showing success/error toasts gives a clear UX while keeping audit logging out of the critical path. On failures, the commit dialog remains open andpendingValuesis preserved, allowing the user to retry. Theconsole.errorcalls in both the update and audit paths retain full error objects for debugging, which matches prior guidance.Based on learnings
201-203: Formik field handlers for selects and toggles are wired appropriatelyThe explicit
React.ChangeEvent<HTMLSelectElement>/HTMLInputElementtypings and use offormik.setFieldValueforloggingLevel,loggingLayout, and the three boolean toggles correctly bridge DOM events into the strongly-typedLoggingFormValuesshape. This should keep the form state in sync with the API model without surprising coercions.If
CustomInput/GluuToogleRowhave stricter prop typings, re-run TypeScript builds to ensure these handler signatures align with their expected event types.Also applies to: 229-231, 246-248, 257-259, 268-269
47-56: Review comment is incorrect; authorize never rejectsThe
authorizefunction fromuseCedarling(admin-ui/app/cedarling/hooks/useCedarling.ts:123-170) handles all error cases internally with a try/catch and always returns a structuredPromise<AuthorizationResult>. It never rejects or throws.Even
token_authorizeCedarRequestBuildererrors (which can throw for missing tokens) are caught withinauthorize's try/catch block (lines 166-169) and returned as{ isAuthorized: false, error: '...' }.LoggingPage already correctly handles authorization results by checking
readResult.isAuthorizedand settingpermissionErrorwhen needed. The suggested try/catch wrapper is unnecessary because the promise never rejects.Likely an incorrect or invalid review comment.
admin-ui/plugins/auth-server/components/Configuration/Defaults/LoggingPage.tsx
Show resolved
Hide resolved
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
|
|
4 participants
#2462
Closes #2462
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.