fix(admin-ui): Use typescript generated client for Webhooks pages#2488
fix(admin-ui): Use typescript generated client for Webhooks pages#2488
Conversation
syntrydy
added
comp-admin-ui
|
Warning Rate limit exceeded@syntrydy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 52 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 legacy JavaScript webhook UI with TypeScript implementations; adds typed hooks and audit logging for webhook mutations, a Redux slice, URL validation, new locale keys, ShortCodesIcon prop relaxations, centralized webhook exports, and dispatches webhook triggers on scope create/update/delete. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Webhook UI (List/Form/Scopes)
participant Hooks as useCreate/Update/DeleteWithAudit
participant API as JansConfigApi
participant Audit as useWebhookAudit
participant Cache as QueryCache
User->>UI: submit create/update/delete or perform scope action
UI->>Hooks: call mutation(payload)
Hooks->>API: POST / PUT / DELETE request
API-->>Hooks: response (success / error)
Hooks->>Audit: logAction(actionType, resource, payload)
Hooks->>Cache: invalidate getAllWebhooks query
Hooks-->>UI: return result
UI->>User: show toast / navigate / show error
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: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
admin-ui/app/components/SVG/menu/ShortCodesIcon.tsx(1 hunks)admin-ui/app/locales/en/translation.json(2 hunks)admin-ui/app/locales/es/translation.json(2 hunks)admin-ui/app/locales/fr/translation.json(2 hunks)admin-ui/app/locales/pt/translation.json(2 hunks)admin-ui/plugins/admin/components/Webhook/ShortcodePopover.tsx(4 hunks)admin-ui/plugins/admin/components/Webhook/WebhookAddPage.js(0 hunks)admin-ui/plugins/admin/components/Webhook/WebhookAddPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookEditPage.js(0 hunks)admin-ui/plugins/admin/components/Webhook/WebhookEditPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookForm.js(0 hunks)admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookListPage.js(0 hunks)admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookSearch.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts(2 hunks)admin-ui/plugins/admin/components/Webhook/hooks/index.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/index.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/types/index.ts(1 hunks)admin-ui/plugins/admin/redux/features/WebhookSlice.ts(1 hunks)
💤 Files with no reviewable changes (4)
- admin-ui/plugins/admin/components/Webhook/WebhookEditPage.js
- admin-ui/plugins/admin/components/Webhook/WebhookAddPage.js
- admin-ui/plugins/admin/components/Webhook/WebhookListPage.js
- admin-ui/plugins/admin/components/Webhook/WebhookForm.js
🧰 Additional context used
🧠 Learnings (1)
📚 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/admin/components/Webhook/hooks/useWebhookAudit.ts
🧬 Code graph analysis (7)
admin-ui/plugins/admin/components/Webhook/WebhookEditPage.tsx (1)
admin-ui/app/components/Card/Card.tsx (1)
Card(27-27)
admin-ui/plugins/admin/components/Webhook/WebhookAddPage.tsx (1)
admin-ui/app/components/Card/Card.tsx (1)
Card(27-27)
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (2)
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts (4)
useWebhookAudit(21-56)CREATE(58-58)UPDATE(58-58)DELETION(58-58)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (1)
WebhookEntry(5-5)
admin-ui/plugins/admin/redux/features/WebhookSlice.ts (2)
admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (4)
TriggerPayload(87-90)WebhookSliceState(64-85)WebhookEntry(5-5)AuiFeature(5-5)admin-ui/plugins/admin/redux/sagas/types/userAction.ts (1)
ActionPayload(7-9)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (6)
admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (3)
WebhookEntry(5-5)WebhookFormValues(7-15)CursorPosition(59-62)admin-ui/plugins/admin/helper/webhook.ts (1)
buildWebhookInitialValues(29-41)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (2)
useCreateWebhookWithAudit(19-57)useUpdateWebhookWithAudit(59-97)admin-ui/plugins/admin/helper/validations/webhookValidation.ts (1)
getWebhookValidationSchema(4-38)admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
isValid(26-31)admin-ui/plugins/admin/components/Webhook/index.ts (1)
isValid(7-7)
admin-ui/plugins/admin/components/Webhook/ShortcodePopover.tsx (1)
admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (2)
ShortcodeLabelProps(42-46)ShortcodePopoverProps(36-40)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
admin-ui/plugins/admin/components/Webhook/index.ts (2)
isAllowed(7-7)isValid(7-7)
⏰ 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)
🔇 Additional comments (18)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
1-33: LGTM! Clean TypeScript migration with improved structure.The refactoring introduces explicit typing, extracts the
isAllowedhelper for better separation of concerns, and properly handles null/undefined cases. The early-return pattern improves readability while preserving the original validation logic.admin-ui/app/locales/es/translation.json (1)
442-442: LGTM! Spanish translations added for webhook UI.The new translation keys support pagination and webhook management features. The Spanish text appears grammatically correct and aligns with the webhook UI requirements described in the PR objectives.
Also applies to: 740-742
admin-ui/plugins/admin/components/Webhook/types/index.ts (1)
1-1: LGTM! Standard barrel export pattern.This centralizes webhook type exports, providing a clean public API surface for the webhook module. Consumers can now import types via
import { ... } from '.../Webhook/types'.admin-ui/app/locales/pt/translation.json (1)
332-332: LGTM! Portuguese translations added for webhook UI.The translations mirror the structure in other locale files and support the new webhook management features. The Portuguese text appears grammatically correct.
Also applies to: 697-699
admin-ui/plugins/admin/components/Webhook/WebhookEditPage.tsx (1)
6-12: No action required—WebhookFormcorrectly handles edit mode and webhook loading.The component retrieves the webhook ID from URL parameters via
useParams()(line 55) and loads the webhook data from Redux state, which is populated byWebhookListPagebefore navigation. The form detects edit mode by checking if bothidandselectedWebhookexist (line 166), and queries webhook features conditionally based on the presence of an ID (line 66-68). No props are needed because the component relies on routing context and Redux state, which is the intended design pattern.admin-ui/plugins/admin/components/Webhook/WebhookAddPage.tsx (1)
6-12: Routing and form mode differentiation are correctly implemented.The webhook pages are properly distinguished:
WebhookAddPageroutes to/webhook/add(no ID parameter)WebhookEditPageroutes to/webhook/edit/:id(with ID parameter)
WebhookFormusesuseParams()to extract theidfrom the route, enabling it to differentiate between modes: whenidis present, it loads webhook features for editing and usesupdateWebhook; when absent, it usescreateWebhook. The conditional rendering{id && selectedWebhook?.inum && ...}further confirms mode-specific logic.admin-ui/app/locales/fr/translation.json (1)
365-365: Webhook & pagination translations are consistent with EN locale
fields.results_per_pageand the three webhook message keys are correctly namespaced and align with the English originals; no issues from an i18n/usage perspective.Also applies to: 676-678
admin-ui/plugins/admin/components/Webhook/hooks/index.ts (1)
1-6: Barrel exports for webhook hooks look goodCentralizing
useWebhookAuditand the audited mutation hooks here simplifies imports and keeps the public surface clear.admin-ui/app/locales/en/translation.json (1)
442-442: New webhook and pagination messages are well‑integrated
fields.results_per_pageand the three webhook messages read clearly and match howWebhookSearchandWebhookListPageconsume them; no changes needed.Also applies to: 740-742
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts (1)
8-56: Audit hook correctly centralizes webhook audit contextUsing
initAuditto encapsulate token/client/IP/userinfo andlogActionto attachWebhookEntry/{ inum }payloads keeps webhook auditing consistent and reusable. Only consideration: ifpostUserActionfailures should never block the calling operation, you may want to wrap theawait postUserAction(audit)in atry/catchor make it fire‑and‑forget so audit failures don’t influence business logic.admin-ui/plugins/admin/components/Webhook/index.ts (1)
1-9: Webhook barrel module cleanly aggregates the public surfaceExporting pages, form/search components, URL helpers, and
types/hooksfrom a single entry point will simplify consumers and keep the Webhook module cohesive.admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (1)
1-131: Webhook type definitions are cohesive and align with usageCentralizing Webhook‑related types (form values, slice state, audit payloads, table actions, shortcode config) and re‑exporting JansConfigApi types gives a clear, type‑safe contract for all Webhook UI code. The shapes for
WebhookActionPayloadandPagedWebhookResultin particular line up with how auditing and listing are implemented elsewhere.admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
150-167: Server‑side pagination & sorting look correct; confirm API params and delete‑refetch behaviorThe list page wires server‑side pagination and sorting cleanly via
useGetAllWebhooks(limit, pattern, startIndex, sortBy, sortOrder) and a customTablePaginationComponent, and usesuseDeleteWebhookWithAuditfor audited deletions. Two things worth double‑checking:
- The
sortBy(displayName|url|httpMethod|jansEnabled) andsortOrder('ascending' | 'descending') values passed from this page andWebhookSearchshould match exactly what the JansConfigApiGetAllWebhooksendpoint expects.useDeleteWebhookWithAuditshould invalidate or refetch theGetAllWebhooksquery so that the list updates automatically after a deletion without relying solely on the manual Refresh button.Functionally the rest of the table rendering (columns, permission‑gated actions, empty/loading states) looks solid.
Also applies to: 222-241, 348-398
admin-ui/plugins/admin/components/Webhook/ShortcodePopover.tsx (2)
17-42: Consider memoizing theLabelcomponent or moving it outside the module.The
Labelcomponent is recreated on each render of the parent module. While it doesn't cause immediate issues since it's defined at module level (not insideShortcodePopover), if it were ever moved inside the main component, it would cause unnecessary re-renders of list items.The current placement is acceptable, but adding
memowould make it consistent with the parent component.
91-116: LGTM!The list rendering with proper key usage, conditional dividers, and type-safe props is well-implemented. The TypeScript conversion from PropTypes is clean and the component correctly handles empty state.
admin-ui/plugins/admin/redux/features/WebhookSlice.ts (1)
72-173: Slice implementation is well-structured.The Redux slice follows standard patterns with proper initial state, typed payloads, and clear reducer logic. The separation of request/response actions enables saga integration cleanly.
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (2)
112-119: LGTM!The feature selection state management correctly handles the async loading of webhook features. The baseline tracking for change detection is a good pattern for enabling/disabling form controls.
248-506: Form implementation is comprehensive and well-structured.The form correctly handles:
- Lazy loading of the JSON editor with Suspense
- Cursor position tracking for shortcode insertion
- Change detection for enabling/disabling controls
- Conditional body editor based on HTTP method
- Proper cleanup on unmount
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
113-131: Re‑validate payload insubmitFormto avoid uncaughtJSON.parseexceptions.
onSubmitcorrectly usesvalidatePayload(with a guardedJSON.parse) before opening the commit dialog, butsubmitFormlater does:if (formikValues.httpMethod !== 'GET' && formikValues.httpMethod !== 'DELETE') { payload.httpRequestBody = JSON.parse(formikValues.httpRequestBody) }If the user edits
httpRequestBodyafter the initial validation but before confirming the commit dialog, thisJSON.parsecan throw and break the component.You can re‑use
validatePayloadinsidesubmitFormwith current values and Formik’ssetFieldErrorto ensure the body is still valid before parsing, eliminating the unguarded exception risk:- const { values: formikValues, resetForm, setFieldValue, dirty: formikDirty } = formik + const { + values: formikValues, + resetForm, + setFieldValue, + setFieldError, + dirty: formikDirty, + } = formik @@ - const submitForm = useCallback( - async (userMessage: string) => { - closeCommitDialog() - - const payload: WebhookEntry = { + const submitForm = useCallback( + async (userMessage: string) => { + const hasError = validatePayload(formikValues, setFieldError) + closeCommitDialog() + + if (hasError) { + return + } + + const payload: WebhookEntry = { displayName: formikValues.displayName, url: formikValues.url, httpMethod: formikValues.httpMethod, description: formikValues.description, jansEnabled: formikValues.jansEnabled, @@ - if (formikValues.httpMethod !== 'GET' && formikValues.httpMethod !== 'DELETE') { - payload.httpRequestBody = JSON.parse(formikValues.httpRequestBody) - } + if (formikValues.httpMethod !== 'GET' && formikValues.httpMethod !== 'DELETE') { + payload.httpRequestBody = JSON.parse(formikValues.httpRequestBody) + } @@ - [ - closeCommitDialog, - formikValues, - resetForm, - selectedFeatures, - id, - selectedWebhook, - createWebhook, - updateWebhook, - ], + [ + closeCommitDialog, + formikValues, + resetForm, + selectedFeatures, + id, + selectedWebhook, + createWebhook, + updateWebhook, + validatePayload, + setFieldError, + ], )This keeps the single source of truth for validation (
validatePayload), ensures the body is still valid at the moment of submission, and prevents an unhandledJSON.parseexception.Also applies to: 133-178, 153-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
admin-ui/app/components/SVG/menu/ShortCodesIcon.tsx(1 hunks)admin-ui/app/locales/en/translation.json(3 hunks)admin-ui/app/locales/es/translation.json(3 hunks)admin-ui/app/locales/fr/translation.json(3 hunks)admin-ui/app/locales/pt/translation.json(2 hunks)admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookSearch.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts(1 hunks)admin-ui/plugins/admin/redux/features/WebhookSlice.ts(1 hunks)admin-ui/plugins/admin/redux/sagas/types/state.ts(2 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/admin/components/Webhook/hooks/useWebhookMutations.tsadmin-ui/plugins/admin/components/Webhook/WebhookForm.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/admin/components/Webhook/hooks/useWebhookMutations.ts
🧬 Code graph analysis (5)
admin-ui/plugins/admin/redux/sagas/types/state.ts (1)
admin-ui/plugins/admin/redux/features/WebhookSlice.ts (1)
WebhookSliceState(6-27)
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (4)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
extractErrorMessage(25-30)admin-ui/plugins/admin/components/Webhook/hooks/index.ts (2)
useCreateWebhookWithAudit(3-3)useWebhookAudit(1-1)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts (1)
useWebhookAudit(21-56)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (1)
WebhookEntry(5-5)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (8)
admin-ui/plugins/admin/redux/sagas/types/state.ts (1)
RootState(5-27)admin-ui/plugins/admin/helper/webhook.ts (1)
buildWebhookInitialValues(29-41)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (2)
useCreateWebhookWithAudit(25-60)useUpdateWebhookWithAudit(62-97)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (4)
WebhookFormValues(7-15)CursorPosition(59-62)AuiFeature(5-5)WebhookEntry(5-5)admin-ui/plugins/admin/helper/validations/webhookValidation.ts (1)
getWebhookValidationSchema(4-38)admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
isValid(26-31)admin-ui/plugins/admin/components/Webhook/index.ts (1)
isValid(7-7)admin-ui/app/components/Card/Card.tsx (1)
Card(27-27)
admin-ui/plugins/admin/redux/features/WebhookSlice.ts (1)
admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (3)
WebhookEntry(5-5)TriggerPayload(64-68)WebhookActionPayload(70-78)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (6)
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
ThemeState(20-22)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (2)
WebhookEntry(5-5)TableAction(86-93)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (1)
useDeleteWebhookWithAudit(99-134)admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
deleteWebhook(120-150)admin-ui/app/customColors.ts (1)
customColors(4-25)
🔇 Additional comments (16)
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (3)
25-60: LGTM - Previous review concerns have been addressed.The implementation correctly addresses the previous review comments:
- Callback stability is handled via
useRefpattern (lines 30-31)- Audit logging failures are now fire-and-forget with
.catch()(line 40), preventing them from masking successful mutations- Error message extraction uses the shared helper function
The hook properly orchestrates mutation, audit logging, toast notifications, query invalidation, and callbacks.
62-97: LGTM - Consistent implementation.The update hook follows the same pattern as the create hook, maintaining consistency across all webhook mutation operations. All the fixes from previous reviews are applied here as well.
99-134: LGTM - Consistent implementation.The delete hook follows the same reliable pattern as create and update, appropriately accepting an
inumstring parameter instead of a fullWebhookEntryobject. All review fixes are consistently applied.admin-ui/app/locales/fr/translation.json (3)
365-365: Localization key placement consistent with related fields.The
results_per_pagekey is appropriately positioned nearsort_byin the fields section, and the French translation is grammatically correct and consistent with UI pagination patterns.
676-678: Webhook UI localizations properly placed and translated.The three new webhook message keys are correctly positioned within the messages section near existing webhook entries, and the French translations are grammatically accurate and contextually appropriate for the webhook form UI.
724-724: URL validation message appropriately placed with related error messages.The
invalid_url_errorkey is logically positioned adjacent to other URL/validation errors, and the French translation accurately conveys the validation intent with proper grammar and punctuation.admin-ui/app/locales/pt/translation.json (3)
332-332: Consistent pagination localization across locales.The Portuguese
results_per_pagetranslation mirrors the French placement and structure, maintaining consistency in UI localization across language variants. The Brazilian Portuguese translation is grammatically correct.
697-699: Webhook UI localizations consistent with other locales and grammatically correct.The three Portuguese webhook messages are properly positioned and accurately translated, with correct Brazilian Portuguese grammar, verb conjugations, and punctuation. Placement and structure mirror the French localization file, maintaining consistency across language variants.
705-705: URL validation message consistent across locales with correct Portuguese grammar.The
invalid_url_errormessage in Portuguese is correctly positioned adjacent to related validation errors and uses appropriate vocabulary and grammar. Placement and structure mirror the French localization, maintaining cross-locale consistency.admin-ui/app/locales/en/translation.json (1)
442-442: Webhook & pagination i18n additions are consistent and correctly keyedThe new
results_per_pageand webhook-related message keys are well named, grammatically sound, and align with their usages in the new webhook components. No changes needed.Also applies to: 740-743, 756-756
admin-ui/plugins/admin/redux/sagas/types/state.ts (1)
3-3: RootState now correctly reuses WebhookSliceStateSwitching
webhookReducerto the importedWebhookSliceStateremoves duplicated state shape and keeps saga typing in sync with the slice definition. This is the right direction.Also applies to: 26-26
admin-ui/app/locales/es/translation.json (1)
442-442: Spanish webhook & pagination translations are accurateThe new Spanish entries mirror the English intent and fit existing tone (“Agregar…”, “Cree su primer webhook…”). Keys match the usages in the webhook UI. No issues.
Also applies to: 740-743, 756-756
admin-ui/app/components/SVG/menu/ShortCodesIcon.tsx (1)
3-5: ShortCodesIcon now correctly forwards styling props
classNameandstyleare optional and properly applied to the SVG, with a sensible default forstyleand a fixed width. This resolves the earlier “unused props” concern cleanly.Also applies to: 8-8, 16-17
admin-ui/plugins/admin/components/Webhook/WebhookSearch.tsx (1)
17-147: WebhookSearch behavior and typing look solidState sync with
pattern, keyboard handling on Enter, sorting/limit controls, and i18n integration are all correctly wired with appropriate memoization. I don’t see any issues here.admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
69-83: Row actions, icons, and table wiring are correctly implemented
EditIcon/DeleteIconnow forward props to MUI icons, soiconProps(id, color, style) fromrowActionsare honored.rowActionscorrectly gates edit/delete bycanWriteWebhooks/canDeleteWebhooksand usesinum-based IDs for testing and targeting.- MaterialTable is fed with server-side data, custom pagination, and theming in a consistent way.
No further issues spotted in this part of the list page.
Also applies to: 245-278, 360-387
admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (1)
1-109: Centralized webhook types look coherent and match UI usage.The re-exports from
JansConfigApiand the locally defined form/UI types (WebhookFormValues,HttpHeader,TriggerPayload,WebhookActionPayload, etc.) are consistent with how they’re consumed in the form and slice code. Having this as a single source of truth for webhook UI types is a good direction.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
159-161: Still safer to guardJSON.parseinsubmitForm(or avoid double-parsing)
validatePayloadnow does aJSON.parsewith try/catch, which is a solid improvement, but Line 160 insubmitFormstill performs an unguardedJSON.parse(formikValues.httpRequestBody). If validation is ever bypassed or the body changes between validation and accepting the commit dialog, this can still throw synchronously inside the submit handler.To make this robust:
- Either wrap this
JSON.parsein a try/catch and surface a field error / toast andreturnearly on failure, or- Refactor
validatePayloadto return the parsed body (e.g.{ hasError, parsedBody }) and reuse that instead of parsing again here.This keeps the submit path resilient even if future refactors change how validation is wired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
admin-ui/app/context/theme/themeContext.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts(1 hunks)admin-ui/plugins/admin/redux/features/WebhookSlice.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/admin/components/Webhook/hooks/useWebhookMutations.tsadmin-ui/plugins/admin/components/Webhook/WebhookForm.tsx
🧬 Code graph analysis (3)
admin-ui/plugins/admin/redux/features/WebhookSlice.ts (1)
admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (4)
WebhookEntry(5-5)AuiFeature(5-5)TriggerPayload(64-68)WebhookActionPayload(70-78)
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (4)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
extractErrorMessage(25-30)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts (4)
useWebhookAudit(21-56)CREATE(58-58)UPDATE(58-58)DELETION(58-58)admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (3)
createWebhook(89-118)updateWebhook(152-182)deleteWebhook(120-150)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (1)
WebhookEntry(5-5)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (6)
admin-ui/plugins/admin/helper/webhook.ts (1)
buildWebhookInitialValues(29-41)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (2)
useCreateWebhookWithAudit(31-66)useUpdateWebhookWithAudit(68-103)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (3)
WebhookFormValues(7-15)CursorPosition(59-62)WebhookEntry(5-5)admin-ui/plugins/admin/helper/validations/webhookValidation.ts (1)
getWebhookValidationSchema(4-38)admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
isValid(26-31)admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (2)
updateWebhook(152-182)createWebhook(89-118)
⏰ 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)
🔇 Additional comments (7)
admin-ui/app/context/theme/themeContext.tsx (1)
14-17: ExportingThemeContextTypelooks good and is type-safeMaking
ThemeContextTypeexported on Line 14 cleanly exposes the context shape for consumers (e.g., custom hooks or components usinguseContext(ThemeContext)) without changing runtime behavior. The internalThemeState/ThemeActionaliases can safely remain non-exported since consumers only need theThemeContextTypesurface.admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (4)
19-29: LGTM! Well-documented error extraction.The documentation clearly explains why this implementation differs from the general error utility, and the extraction logic correctly handles the webhook API's response structure with appropriate fallbacks.
31-66: LGTM! Solid implementation with proper error handling.The hook correctly implements the mutation flow with:
- Stable callback references using
useRef- Fire-and-forget audit logging to avoid masking successful mutations
- Appropriate error handling with user feedback
- Query invalidation and callback execution
All concerns from previous reviews have been addressed.
68-103: LGTM! Consistent implementation.The update hook follows the same robust pattern as the create hook, with appropriate changes for the UPDATE action type. The structural similarity across hooks improves maintainability and reduces cognitive load.
105-140: LGTM! Consistent deletion implementation.The delete hook correctly adapts the pattern for deletion operations, appropriately handling the different parameter structure (
inumvsdata) and logging only the necessary information ({ inum }) in the audit trail.admin-ui/plugins/admin/redux/features/WebhookSlice.ts (1)
1-61: Good consolidation on webhook slice typingThe slice now pulls
WebhookEntry/AuiFeaturefromJansConfigApiandTriggerPayload/WebhookActionPayloadfrom the shared webhook types module, which removes the earlier duplicated/ divergent interfaces. The initial state and reducers line up with those shapes cleanly; no issues spotted here.admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
321-337: ConfirmGluuInputRowstill updates Formik state whenhandleChangeis providedIn Lines 321–327, the
handleChangepassed toGluuInputRowonly updatescursorPositionand does not callformik.handleChange. This is fine only ifGluuInputRowinternally wires Formik’s change handler and treatshandleChangeas an additional side-effect.If
handleChangeis an override rather than a hook, form values forurlwill no longer update. Please confirm the component’s contract; if it overrides, adjust the callback to:handleChange={(event: React.ChangeEvent<HTMLInputElement>) => { formik.handleChange(event) const currentPosition = event.target.selectionStart || 0 setCursorPosition((prev) => ({ ...prev, url: currentPosition })) }}so both Formik and cursor tracking work correctly.
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts
Outdated
Show resolved
Hide resolved
| render: (rowData: WebhookEntry) => ( | ||
| <Typography | ||
| variant="body2" | ||
| fontWeight={500} | ||
| color={rowData.jansEnabled ? 'black !important' : 'gray !important'} | ||
| > | ||
| {rowData.displayName} | ||
| </Typography> | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the WebhookListPage.tsx file
find . -name "WebhookListPage.tsx" -type fRepository: GluuFederation/flex
Length of output: 126
🏁 Script executed:
# Get an overview of the file size
wc -l admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsxRepository: GluuFederation/flex
Length of output: 128
🏁 Script executed:
# Read the specific lines mentioned (279-287) with context
sed -n '270,300p' admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsxRepository: GluuFederation/flex
Length of output: 953
🏁 Script executed:
# Find MUI type definitions or check package.json for MUI version
find . -name "package.json" -type f | head -5Repository: GluuFederation/flex
Length of output: 86
🏁 Script executed:
# Check the imports and MUI version being used
grep -r "@mui/material" admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsxRepository: GluuFederation/flex
Length of output: 85
🏁 Script executed:
# Look for other usages of Typography in the codebase to see patterns
rg "Typography" admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx -A 3 -B 1Repository: GluuFederation/flex
Length of output: 1054
🏁 Script executed:
# Check MUI version in package.json
cat admin-ui/package.json | grep -A 2 -B 2 "@mui/material"Repository: GluuFederation/flex
Length of output: 241
🏁 Script executed:
# Check if there's TypeScript compilation configured
find admin-ui -name "tsconfig.json" -type fRepository: GluuFederation/flex
Length of output: 85
🏁 Script executed:
# Search for other color prop usages with non-semantic values
rg "color\s*=\s*\{" admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx -A 1Repository: GluuFederation/flex
Length of output: 387
🏁 Script executed:
# Check tsconfig.json to see strictness settings
cat admin-ui/tsconfig.jsonRepository: GluuFederation/flex
Length of output: 1379
🏁 Script executed:
# Verify MUI Typography color prop type definition expectations
web_search
MUI Material-UI Typography color prop TypeScript type v5Repository: GluuFederation/flex
Length of output: 155
Use sx prop for custom colors instead of Typography's typed color prop
On line 283, the Typography component's color prop is set to 'black !important' / 'gray !important', which violates MUI's TypeScript type constraints (the prop expects semantic values like 'primary' or 'textSecondary'). With strict TypeScript enabled in this project, this produces type errors. The URL column already uses sx for styling; apply the same approach here:
- render: (rowData: WebhookEntry) => (
- <Typography
- variant="body2"
- fontWeight={500}
- color={rowData.jansEnabled ? 'black !important' : 'gray !important'}
- >
+ render: (rowData: WebhookEntry) => (
+ <Typography
+ variant="body2"
+ fontWeight={500}
+ sx={{ color: rowData.jansEnabled ? 'black' : 'gray' }}
+ >This aligns with MUI's API, satisfies TypeScript's strict type checking, and removes unnecessary !important.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| render: (rowData: WebhookEntry) => ( | |
| <Typography | |
| variant="body2" | |
| fontWeight={500} | |
| color={rowData.jansEnabled ? 'black !important' : 'gray !important'} | |
| > | |
| {rowData.displayName} | |
| </Typography> | |
| ), | |
| render: (rowData: WebhookEntry) => ( | |
| <Typography | |
| variant="body2" | |
| fontWeight={500} | |
| sx={{ color: rowData.jansEnabled ? 'black' : 'gray' }} | |
| > | |
| {rowData.displayName} | |
| </Typography> | |
| ), |
🤖 Prompt for AI Agents
In admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx around lines
279–287, the Typography uses the typed color prop with raw values ('black
!important' / 'gray !important') which breaks strict TypeScript; remove the
color prop and instead apply the styling via the sx prop (matching the URL
column pattern) — e.g. set sx={{ color: rowData.jansEnabled ? 'text.primary' :
'text.secondary', fontWeight: 500 }} or use literal colors if needed (without
!important) so the type-safe MUI API is respected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
279-306: Use theme-aware colors insxand drop!importantin Typography stylesThe name and URL columns now use
sx(good), but they still hard-code'black !important'/'gray !important':sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important' }} ... sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important', ... }}This bypasses theme palettes and keeps
!importantin play unnecessarily, making it harder to maintain dark/light themes.Consider moving to palette-based colors without
!important, e.g.:- sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important' }} + sx={{ color: rowData.jansEnabled ? 'text.primary' : 'text.secondary' }} @@ - sx={{ - color: rowData.jansEnabled ? 'black !important' : 'gray !important', + sx={{ + color: rowData.jansEnabled ? 'text.primary' : 'text.secondary', wordBreak: 'break-all', maxWidth: '350px', fontFamily: 'monospace', fontSize: '0.8rem', }}This keeps the styling consistent with the active MUI theme and avoids hard-coded CSS overrides.
Please double-check against your MUI v5 typings/docs that
'text.primary'/'text.secondary'are the palette keys you want for enabled vs disabled states, and that no other parts of the design system rely on the previous hard-coded colors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/hooks/index.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/admin/components/Webhook/hooks/useWebhookMutations.ts
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (4)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
extractErrorMessage(25-30)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookAudit.ts (3)
useWebhookAudit(21-56)CREATE(58-58)UPDATE(58-58)admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (2)
createWebhook(89-118)updateWebhook(152-182)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (1)
WebhookEntry(5-5)
🔇 Additional comments (6)
admin-ui/plugins/admin/components/Webhook/hooks/index.ts (1)
1-7: LGTM! Clean barrel export pattern.The centralized exports provide a clean public API for webhook hooks and improve maintainability.
admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (4)
1-17: LGTM! Imports and interface are well-structured.The imports are appropriate, and the
MutationCallbacksinterface is now properly exported, addressing the previous feedback.
19-29: LGTM! Error extraction with clear documentation.The
extractErrorMessagehelper properly addresses previous concerns about duplication by documenting the intentional difference between webhook API responses (responseMessage) and other APIs (message). The implementation correctly handles the webhook-specific error structure.
31-66: LGTM! Well-implemented create hook with proper error handling.The hook correctly implements:
- useRef pattern to prevent callback dependency issues (previously addressed)
- Fire-and-forget audit logging that won't mask successful mutations (previously addressed)
- Proper error extraction and user feedback via toasts
- Query cache invalidation on success
- Error propagation for caller handling
The console.error in the audit catch block is appropriate for debugging as per coding guidelines.
68-140: LGTM! Update and delete hooks follow consistent patterns.Both
useUpdateWebhookWithAuditanduseDeleteWebhookWithAuditcorrectly mirror the create hook's implementation:
- Proper useRef pattern for stable callbacks
- Fire-and-forget audit logging
- Appropriate mutation hooks and parameters
- Consistent error handling and user feedback
- Proper action constants (UPDATE, DELETION)
The consistency across all three hooks enhances maintainability.
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
74-77: Row action icons now correctly forward iconProps
EditIcon/DeleteIconnow accept props and spread them into the underlying MUI icons, soiconProps(includingid,style,color, etc.) fromrowActionswill be honored. This fixes the previous issue where action icon styling and ids were ignored.You may want to quickly verify in the UI that
id="editWebhook{inum}"/id="deleteWebhook{inum}"are present on the rendered<svg>elements and that the customstyle.coloroverrides the default red on delete.Also applies to: 239-272
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (2)
161-170: Pagination clamp logic correctly fixes the empty-last-page issueThe
totalItems-aware clamp:if (totalItems > 0 && pageNumber * limit >= totalItems) { const lastPage = Math.max(0, Math.ceil(totalItems / limit) - 1) setPageNumber(lastPage) }prevents the “stranded on an empty page after deleting the last row” problem and converges safely without loops. Combined with the updated empty-state condition (
totalItems === 0), this addresses the earlier pagination bug.
282-305: Avoid!importantinsxcolors for better theming and override controlYou’re now using
sx(good), but the values still include!important:sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important' }} ... sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important', ... }}
!importanthere is unnecessary and makes future overrides harder. Plain colors (or theme tokens) are enough:- sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important' }} + sx={{ color: rowData.jansEnabled ? 'black' : 'gray' }} @@ - sx={{ - color: rowData.jansEnabled ? 'black !important' : 'gray !important', + sx={{ + color: rowData.jansEnabled ? 'black' : 'gray', wordBreak: 'break-all', maxWidth: '350px', fontFamily: 'monospace', fontSize: '0.8rem', }}If you want to stay fully theme-aware, you could also use
theme.palette.text.primary/secondaryvia a function form ofsx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)
🔇 Additional comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
182-189: Confirm that delete mutation invalidates/refetches the list query
submitFormawaitsdeleteWebhook(deleteData.inum, userMessage)but doesn’t callrefetchor otherwise update the list. IfuseDeleteWebhookWithAuditdoesn’t invalidate theuseGetAllWebhookscache under the hood, the table could show stale data until the user manually hits Refresh.If invalidation isn’t wired, consider:
- if (deleteData?.inum) { - await deleteWebhook(deleteData.inum, userMessage) - } + if (deleteData?.inum) { + await deleteWebhook(deleteData.inum, userMessage) + await refetch() + }(or doing this inside the mutation hook’s
onSuccess).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
166-172: Pagination clamping and empty-state condition now handle last-page deletions correctlyThe
useEffectthat clampspageNumberwhenpageNumber * limit >= totalItems, combined with the!pattern && totalItems === 0empty-state check, resolves the “delete last item on last page shows misleading empty state” issue while there are still earlier pages.Also applies to: 359-361
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookSearch.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/admin/components/Webhook/WebhookForm.tsx
🔇 Additional comments (4)
admin-ui/plugins/admin/components/Webhook/WebhookSearch.tsx (1)
48-62: Pattern/input synchronization is correct and avoids stale UI stateUsing
inputValueplus theuseEffectthat syncs it frompatternensures the textbox reflects external resets (e.g., URL/query changes) while only committing on Enter viaonPatternChange. This is a solid, predictable pattern for a search box.admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
76-82: Icon components now correctly forwardiconPropsto MUI iconsWrapping
Edit/DeleteOutlinedas components that accept and spreadprops(includingstyleandid) fixes the earlier issue where MaterialTable’siconPropswere ignored. Thestyle={{ color: 'red', ...props.style }}merge onDeleteIconalso lets callers override the default color if needed.admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (2)
119-137: Inline payload validation is consistent and localized
validatePayloadcombining JSON validation withisValidURL checks and localized error messages (invalid_json_error,invalid_url_error) is a good complement to the schema. Keeping this in a dedicated callback keepsonSubmitlean and makes the extra checks easy to maintain.
193-208: Feature selection change tracking is correctly separated from Formik dirtinessUsing
selectedFeatures/baselineSelectedFeatureswithisEqualand then combining that withformikDirtyforisFormChangedcleanly captures both field edits and feature selection changes. This ensures Cancel/Apply enablement matches real user changes, not just Formik field state.
| const getHttpMethodColor = ( | ||
| method: string, | ||
| ): 'info' | 'success' | 'error' | 'warning' | 'default' => { | ||
| const colorMap: Record<string, 'info' | 'success' | 'error' | 'warning'> = { | ||
| GET: 'info', | ||
| POST: 'success', | ||
| DELETE: 'error', | ||
| } | ||
| return colorMap[method] || 'warning' | ||
| } |
There was a problem hiding this comment.
Map colors for all HTTP methods you support
getHttpMethodColor only defines colors for GET, POST, and DELETE, defaulting everything else to 'warning'. Since the form exposes PUT and PATCH as valid methods, those will currently render with a warning color. If that’s not intentional, add explicit entries, e.g.:
const colorMap: Record<string, 'info' | 'success' | 'error' | 'warning'> = {
GET: 'info',
POST: 'success',
PUT: 'success',
PATCH: 'success',
DELETE: 'error',
}so visual semantics match your HTTP methods.
🤖 Prompt for AI Agents
In admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx around lines 65
to 74, the getHttpMethodColor function currently maps only GET, POST and DELETE
and falls back to 'warning' for other methods; add explicit mappings for PUT and
PATCH (and any other supported HTTP verbs) to the desired color (e.g., 'success'
for PUT and PATCH) by extending the colorMap so all methods exposed by the form
have an explicit color and avoid unintentionally showing 'warning'.
| const columns = useMemo( | ||
| () => [ | ||
| { | ||
| title: t('fields.name'), | ||
| field: 'displayName', | ||
| render: (rowData: WebhookEntry) => ( | ||
| <Typography | ||
| variant="body2" | ||
| fontWeight={500} | ||
| sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important' }} | ||
| > | ||
| {rowData.displayName} | ||
| </Typography> | ||
| ), | ||
| }, | ||
| { | ||
| title: t('fields.url'), | ||
| field: 'url', | ||
| width: '35%', | ||
| render: (rowData: WebhookEntry) => ( | ||
| <Typography | ||
| variant="body2" | ||
| sx={{ | ||
| color: rowData.jansEnabled ? 'black !important' : 'gray !important', | ||
| wordBreak: 'break-all', | ||
| maxWidth: '350px', | ||
| fontFamily: 'monospace', | ||
| fontSize: '0.8rem', | ||
| }} | ||
| > | ||
| {rowData.url} | ||
| </Typography> | ||
| ), | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid literal 'black !important' / 'gray !important' in sx; prefer theme-aware colors
Using sx={{ color: rowData.jansEnabled ? 'black !important' : 'gray !important' }} (and the same pattern for the URL column) hard-codes colors and relies on !important, which is brittle against theme changes and unnecessary with MUI.
Consider switching to theme-aware tokens without !important, e.g.:
sx={{
color: rowData.jansEnabled ? 'text.primary' : 'text.secondary',
}}and similarly for the URL column. This keeps the table aligned with the active theme and avoids specificity hacks.
🤖 Prompt for AI Agents
In admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx around lines
284 to 317, the cells set literal colors with '!important' (e.g. 'black
!important' / 'gray !important'), which hardcodes colors and breaks theme
compatibility; replace those literal strings with theme-aware color tokens (e.g.
use text.primary for enabled and text.secondary for disabled) and remove the
'!important' usage so MUI's styling and theme can control color; update both the
displayName and url column sx objects to use the theme color tokens and drop
'!important'.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookSearch.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/admin/components/Webhook/WebhookForm.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (6)
admin-ui/plugins/admin/redux/sagas/types/state.ts (1)
RootState(5-27)admin-ui/plugins/admin/helper/webhook.ts (1)
buildWebhookInitialValues(29-41)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (2)
useCreateWebhookWithAudit(31-66)useUpdateWebhookWithAudit(68-103)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (3)
WebhookFormValues(7-15)WebhookEntry(5-5)ShortCodesConfig(105-109)admin-ui/plugins/admin/helper/validations/webhookValidation.ts (1)
getWebhookValidationSchema(4-38)admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
isValid(26-31)
🔇 Additional comments (3)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
1-457: Strong implementation with past review feedback incorporatedThe webhook list page demonstrates solid React and TypeScript practices:
- Permission-gated UI: Cedar permissions properly control read/write/delete actions
- Pagination clamping: The useEffect on lines 169-174 correctly handles the edge case when deleting the last item on a page
- Empty state logic: Now properly checks
totalItems === 0instead ofwebhooks.length === 0(line 361)- Icon prop forwarding: EditIcon and DeleteIcon (lines 78-81) correctly forward props to underlying MUI components
- Translation best practices: Uses i18next
defaultValueoption instead of||fallback pattern (lines 46, 49-50)- Memoization: Appropriate use of useMemo and useCallback prevents unnecessary re-renders
The code is production-ready and addresses the PR objective of migrating to the TypeScript generated client.
admin-ui/plugins/admin/components/Webhook/WebhookSearch.tsx (1)
1-149: Clean, type-safe search componentThis component demonstrates excellent TypeScript practices:
- Type safety: WebhookSortBy union type (line 17) constrains sortBy to valid fields, preventing runtime errors from unsupported field names
- Controlled input pattern: Lines 50-55 properly sync internal input state with external pattern prop
- Proper event handling: Enter-to-search pattern (lines 57-64) provides good UX
- Memoization: All callbacks properly memoized to prevent unnecessary child re-renders
- i18n best practices: Consistently uses
defaultValueoption for translations (lines 88, 121-122)The component is well-structured and addresses the past review feedback on type constraints.
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
259-519: Overall Webhook form structure and TS integration look solid.Data loading, Formik setup (with reinitialization, schema + extra payload validation), commit dialog flow, and shortcode handling are all cohesive and type-safe. The move to
WebhookEntryand the generated hooks fromJansConfigApiis cleanly integrated with the existing Gluu UI components and change-tracking logic.
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
- Can I check webhook implementation on all features when testing this PR.
I have checked for FIDO, it is not calling configured webhook on editing FIDO configuration.
syntrydy
changed the title
|
Conclusion for the 14 Missing Components
Key Finding These 14 components NEVER had webhook integration - it's not a regression from migration. The original webhook implementation (commit cf9eae4) only covered a subset of features:
The other features were simply never updated to include webhook support. |
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/admin/components/Webhook/WebhookForm.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
📚 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/admin/components/Webhook/WebhookForm.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)
🔇 Additional comments (5)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (5)
1-56: LGTM: Imports and component setup are well-structured.The imports are properly organized, types are correctly imported from their respective modules, and the lazy loading of
GluuInputEditoris a good performance optimization. The Redux selector properly uses the importedRootStatetype.
57-102: LGTM: Data fetching and form initialization are properly implemented.The feature fetching is correctly guarded with
enabled: Boolean(id)to prevent unnecessary requests during webhook creation. The use ofuseMemofor derived state and the Formik configuration withenableReinitializeensure the form properly handles both create and edit scenarios.
121-139: LGTM: Validation logic is robust and user-friendly.The validation properly handles edge cases:
- JSON parsing is wrapped in try/catch to prevent uncaught exceptions
- URL validation only runs when a URL is provided, preserving "required" errors
- All error messages use translation keys for internationalization
176-191: LGTM: Mutation error handling is properly implemented.The submission logic correctly wraps the async mutation calls in try/catch, and the
console.erroris appropriately kept for diagnostics when multiple operations are involved. The hooks already surface user-facing errors via toast notifications.Based on learnings, console.error calls should be kept in catch blocks when there are multiple operations for debugging unexpected errors.
266-527: LGTM: Form rendering is comprehensive and well-structured.The UI implementation is solid:
- Proper use of Suspense for lazy-loaded components
- Conditional rendering based on HTTP method for body editor
- All user-facing strings use translation keys (including the Enabled/Disabled chip labels)
- Proper error display and validation feedback
- Good UX with cursor position tracking for shortcode insertion
- Change detection correctly accounts for both form and feature selection changes
| value={selectedFeatures as unknown as Record<string, unknown>[]} | ||
| options={features as unknown as Record<string, unknown>[]} | ||
| onChange={(options) => { | ||
| const typedOptions = options as unknown as AuiFeature[] | ||
| setSelectedFeatures( | ||
| typedOptions && typedOptions.length > 0 ? [typedOptions[0]] : [], | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertions suggest GluuTypeAhead component needs better typing.
The multiple type casts (as unknown as Record<string, unknown>[] and as unknown as AuiFeature[]) indicate that GluuTypeAhead isn't properly generic or typed for this use case. While this works if the shapes are compatible, it bypasses type safety.
Consider either:
- Making
GluuTypeAheada generic component that accepts typed options - Creating a typed wrapper around
GluuTypeAheadfor webhook features
This is technical debt rather than a functional issue.
🤖 Prompt for AI Agents
In admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx around lines 317 to
323, the code uses repeated unsafe casts (as unknown as Record<string,
unknown>[] and as unknown as AuiFeature[]) when passing options/value to
GluuTypeAhead; fix by making GluuTypeAhead a generic component (e.g.,
GluuTypeAhead<T>) so it accepts typed props for options/value/onChange, or
alternatively create a small typed wrapper component around GluuTypeAhead
specialized for AuiFeature that enforces correct types and maps props without
any unknown casts; update the call sites here to use the generic/wrapper with
AuiFeature so setSelectedFeatures and onChange operate with real AuiFeature
types instead of asserting through unknown.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (1)
314-334: Type casts aroundGluuTypeAheadindicate missing generics/typing on that component.The repeated
as unknown as Record<string, unknown>[]/as unknown as AuiFeature[]casts effectively bypass type safety and rely on runtime shape compatibility. Longer term, this would be cleaner ifGluuTypeAheadwere generic or wrapped in a typed adapter forAuiFeaturesooptions,value, andonChangeare strongly typed withoutunknownhops.This is the same technical-debt concern previously raised about GluuTypeAhead’s typing; OK to defer but worth addressing when you next touch that component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
📚 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/admin/components/Webhook/WebhookForm.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (6)
admin-ui/plugins/admin/redux/sagas/types/state.ts (1)
RootState(5-27)admin-ui/plugins/admin/helper/webhook.ts (1)
buildWebhookInitialValues(29-41)admin-ui/plugins/admin/components/Webhook/hooks/useWebhookMutations.ts (2)
useCreateWebhookWithAudit(31-66)useUpdateWebhookWithAudit(68-103)admin-ui/plugins/admin/components/Webhook/types/WebhookTypes.ts (3)
WebhookFormValues(7-15)CursorPosition(59-62)WebhookEntry(5-5)admin-ui/plugins/admin/helper/validations/webhookValidation.ts (1)
getWebhookValidationSchema(4-38)admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
isValid(26-31)
🔇 Additional comments (2)
admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx (2)
149-193: Submit flow is robust and aligned with audit + error-handling patterns.The
submitFormimplementation looks solid: it defensively parseshttpRequestBodywith try/catch and surfaces a field error, preservesinum/dn/baseDnon updates, wrapscreateWebhook/updateWebhookin a try/catch to avoid unhandled rejections, and logs failures withconsole.errorwhile letting the audit hooks + toasts handle user-facing errors. This matches the recommended pattern of keeping console.error in catch blocks for multi-step operations.Based on learnings, ...
60-66: Theenabledoption placement is correct for the generated hook—no changes needed.The code
useGetFeaturesByWebhookId(id ?? 'skip', { query: { enabled: Boolean(id) } })follows the correct pattern for orval-generated React Query hooks. Whenenabled: false, React Query prevents query execution entirely, so the'skip'placeholder is never sent to the backend. Theenabledflag is correctly placed inside thequeryobject, consistent with other generated hooks likeuseGetAllWebhooksin the same codebase.Likely an incorrect or invalid review comment.
| <FormGroup row> | ||
| <GluuLabel | ||
| doc_category={WEBHOOK} | ||
| doc_entry="http_headers" | ||
| label="fields.http_headers" | ||
| size={4} | ||
| /> | ||
| <Col sm={8}> | ||
| <GluuProperties | ||
| compName="httpHeaders" | ||
| isInputLables={true} | ||
| formik={formik} | ||
| multiProperties | ||
| inputSm={10} | ||
| destinationPlaceholder={'placeholders.enter_key_value'} | ||
| sourcePlaceholder={'placeholders.enter_header_key'} | ||
| options={formikValues.httpHeaders || []} | ||
| isKeys={false} | ||
| buttonText="actions.add_header" | ||
| showError={!!(formik.errors.httpHeaders && formik.touched.httpHeaders)} | ||
| errorMessage={formik.errors.httpHeaders as string} | ||
| /> | ||
| </Col> | ||
| </FormGroup> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Narrow the httpHeaders error type instead of casting to string.
errorMessage={formik.errors.httpHeaders as string} forces a string cast even though Formik’s errors.httpHeaders type can be more complex (arrays/nested errors). Given your Yup schema puts a single message on the array, this is safe at runtime but loses type information.
Consider tightening the typing instead of casting, e.g., by refining the Formik type for errors.httpHeaders or using a small helper that normalizes the error to a string (or undefined). This keeps the component type-safe without relying on as string.
🤖 Prompt for AI Agents
In admin-ui/plugins/admin/components/Webhook/WebhookForm.tsx around lines 398 to
421, the code currently force-casts formik.errors.httpHeaders to string when
passing errorMessage which loses type-safety; instead narrow/normalize the error
to a string or undefined before passing it (for example update the Formik
generic types to reflect that httpHeaders errors are string | undefined, or
create a small helper that checks the runtime shape of formik.errors.httpHeaders
and returns a single message or undefined), then pass that normalized value to
GluuProperties.errorMessage so no unsafe as string cast is used.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
17-24: See previous comment regarding SSRF vulnerability.The security issues with this function's approach are covered in the comment on lines 1-13.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts(1 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx(2 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx(2 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
📚 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/Scopes/ScopeListPage.tsxadmin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsxadmin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (1)
admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx (1)
admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
⏰ 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)
🔇 Additional comments (4)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
26-31: Good defensive programming with null/undefined handling.The type signature correctly accepts
string | undefined | null, and the early return prevents unnecessary processing for invalid inputs.admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx (1)
11-11: Wire scope creation into webhook pipeline using API responseUsing
triggerWebhook({ createdFeatureValue: response })after a successfulusePostOauthScopesmutation is consistent with the saga’s expectation and ensures webhooks receive the persisted scope object (including generated fields). The placement after the toast but inside the success path looks correct, and the updated dependency list onhandleSubmitis appropriate.Also applies to: 75-82
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (1)
40-40: Trigger webhooks only after successful scope deletionImporting
triggerWebhookand dispatchingtriggerWebhook({ createdFeatureValue: item })afterdeleteScope.mutateAsyncresolves cleanly ties deletions into the webhook flow while ensuring no webhook is fired on failure. Using the selecteditemas the payload gives webhooks full context about the deleted scope, and the updated dependency array foronDeletionConfirmedis correct.Also applies to: 334-348
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx (1)
12-12: Send updated scope to webhooks on successful editThe new
triggerWebhook({ createdFeatureValue: response })dispatch in the update success path correctly passes the server-returned scope object into the webhook saga, aligning the edit flow with create/delete webhook behavior. Dependency updates onhandleSubmitare also consistent.Also applies to: 136-143
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
1-21: Harden private/local host detection (169.254/16, 0.0.0.0/8, IPv6 hostname mismatch).
isPrivateOrLocalhoststill misses some high‑risk internal ranges and has an IPv6 hostname mismatch:
https://169.254.169.254/...(cloud metadata) is currently allowed: hostname169.254.169.254does not match any of the existing prefixes.0.0.0.0/8and similar special ranges are not blocked.- For
https://[::1]/...,new URL(url).hostnameis"::1", so the"[::1]"check never matches; IPv6 loopback is only incidentally rejected byPATTERNtoday and would become allowed if the regex is relaxed later.Given this is a security boundary for webhook targets, these should be treated as private/internal and rejected here, not just indirectly via
PATTERN.A minimal hardening that stays consistent with your current string‑prefix approach:
const isPrivateOrLocalhost = (hostname: string): boolean => { - if (hostname === 'localhost' || hostname === '[::1]') { + if (hostname === 'localhost' || hostname === '::1') { return true } if ( hostname.startsWith('127.') || hostname.startsWith('10.') || - hostname.startsWith('192.168.') + hostname.startsWith('192.168.') || + // Link-local and special ranges often used in SSRF attacks + hostname.startsWith('169.254.') || + hostname.startsWith('0.') ) { return true } const match = hostname.match(/^172\.(\d+)\./) if (match) { const secondOctet = parseInt(match[1], 10) if (secondOctet >= 16 && secondOctet <= 31) { return true } } return false }You may also want to consider future‑proofing for IPv6 private/ULA/link‑local ranges if you ever relax
PATTERNto accept IPv6 literals or broader host syntax.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
admin-ui/plugins/admin/components/Webhook/index.ts (2)
isAllowed(7-7)isValid(7-7)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
admin-ui/plugins/admin/components/Webhook/index.ts (2)
isAllowed(7-7)isValid(7-7)
🔇 Additional comments (2)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (2)
24-24: Confirm whether blocking query strings and fragments is intentional.The current
PATTERNenforces HTTPS but disallows query parameters (?token=abc) and fragments (#section). Many webhook services require query strings for authentication tokens or tracking parameters.If this restriction is intentional for security reasons, please document it. Otherwise, consider extending the pattern:
-const PATTERN = /^https:\/\/([\w-]+\.)+[\w-]+(:\d+)?(\/([\w\-.]|\$\{[\w]+\})*)*$/i +const PATTERN = /^https:\/\/([\w-]+\.)+[\w-]+(:\d+)?(\/([\w\-.]|\$\{[\w]+\})*)*([\?#].*)?$/iAlternatively, if the restriction is intentional, add a comment:
// Webhook URLs must not contain query strings or fragments for security reasons const PATTERN = /^https:\/\/([\w-]+\.)+[\w-]+(:\d+)?(\/([\w\-.]|\$\{[\w]+\})*)*$/i
44-49: LGTM: Validation flow is correct.The function correctly chains null/undefined checks, the
isAllowedsecurity validation, and thePATTERNformat validation. The logic is sound.Note that the effectiveness of this function depends on the fixes to
isAllowedandisPrivateOrLocalhostmentioned in other comments.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
admin-ui/app/locales/en/translation.json(3 hunks)admin-ui/app/locales/es/translation.json(3 hunks)admin-ui/app/locales/fr/translation.json(3 hunks)admin-ui/app/locales/pt/translation.json(2 hunks)admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts(1 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
📚 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/Scopes/ScopeListPage.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (1)
admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
admin-ui/plugins/admin/components/Webhook/index.ts (2)
isAllowed(7-7)isValid(7-7)
⏰ 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: sonar scan (admin-ui)
🔇 Additional comments (11)
admin-ui/app/locales/es/translation.json (1)
448-448: ✓ Webhook and pagination translation keys added correctly.The Spanish translations for webhook functionality and pagination are semantically appropriate and properly placed in their respective sections. The keys (
results_per_page,webhook_form_description,no_webhooks_found,create_first_webhook,invalid_url_error) align with the PR objectives and follow the existing structure.Also applies to: 746-748, 762-762
admin-ui/app/locales/fr/translation.json (1)
367-367: ✓ Webhook and pagination translation keys added correctly.The French translations mirror the Spanish additions with semantically appropriate phrasing and proper placement. Key consistency across language files (
results_per_page,webhook_form_description,no_webhooks_found,create_first_webhook,invalid_url_error) is maintained, supporting the webhook UI expansion per PR objectives.Also applies to: 682-684, 730-730
admin-ui/app/locales/pt/translation.json (2)
334-334: Translation key placement and content are correct.The "results_per_page" key is appropriately added to the "fields" section and the Portuguese translation is grammatically correct.
Also applies to: 334-334
703-711: Webhook message translations are contextually appropriate and consistent with English locale.All four new webhook-related translation keys are placed in the correct "messages" section and their Portuguese translations are:
- Grammatically correct
- Semantically accurate
- Consistent with the English versions in structure and meaning
admin-ui/app/locales/en/translation.json (3)
448-448: Translation key placement and content are correct.The "results_per_page" key is appropriately added to the "fields" section with clear English text.
Also applies to: 448-448
746-762: Webhook and URL validation message translations are appropriate and well-placed.All four new webhook-related translation keys are correctly placed in the "messages" section with:
- Clear, user-facing English text
- Consistent terminology and tone
- Appropriate context for webhook configuration and error handling
334-334: Locale translations are consistent across Portuguese and English files.The translation keys for webhook functionality and pagination ("results_per_page", "webhook_form_description", "no_webhooks_found", "create_first_webhook", "invalid_url_error") are properly aligned across both locale files:
- Placed in matching sections ("fields" and "messages")
- Translations are semantically equivalent
- JSON syntax is valid in both files
These translations enable the webhook UI feature to be properly localized across languages, aligning with the PR objective to integrate the TypeScript-generated webhook client into the admin UI.
Also applies to: 448-448
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (2)
39-39: triggerWebhook import wiring looks consistent with webhook flowImporting
triggerWebhookfromPlugins/admin/redux/features/WebhookSliceand using it in this component aligns with the centralized webhook handling introduced elsewhere; no issues here.
336-350: Scope delete now correctly participates in webhook + audit flowAwaiting
deleteScope.mutateAsyncbeforedispatch(triggerWebhook({ createdFeatureValue: item }))andlogScopeDeletion(item, message)ensures webhooks/audit only run after a successful delete. Addingdispatchto the dependency array keepsonDeletionConfirmedcompliant with hook rules. This matches the existing pattern on create/update and closes the gap for delete.admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (2)
1-12: LGTM! Comprehensive scheme blocklist.The blocked schemes list now includes all common SSRF vectors, including the additional protocols (
gopher,dict,tftp) requested in previous reviews.
44-60: LGTM! Private IP blocking now applies to all protocols.The function correctly validates URLs by parsing with the URL constructor, blocking disallowed schemes, and rejecting private/localhost hostnames for all protocols (not just HTTPS). The previous critical issue where protocols like
wss://could bypass private IP checks has been fixed.
| export const isValid = (url: string | undefined | null): boolean => { | ||
| if (url === undefined || url === null || !isAllowed(url)) { | ||
| return false | ||
| } | ||
| return PATTERN.test(url) | ||
| } | ||
|
|
||
| export { isAllowed } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Validation logic is correct.
The isValid function properly chains null-safety, allow-list, and pattern checks. The control flow is sound and the type signature appropriately handles optional URLs.
Note: This validation runs client-side/at configuration time and cannot prevent DNS rebinding attacks (where a domain initially resolves to a public IP but later rebinds to a private IP). If webhook URLs are stored and invoked later by backend services, ensure the backend re-validates the resolved IP address before making the request.
🤖 Prompt for AI Agents
In admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts around lines
62-69 the client-side isValid check is fine but does not mitigate DNS rebinding
for server-side webhook delivery; implement server-side re-validation before
invoking stored webhook URLs by resolving the webhook hostname to IP(s),
checking each resolved IP is not in private/reserved ranges (RFC1918, loopback,
link-local, multicast, carrier-grade NAT, etc.), and rejecting requests if any
resolved IP is disallowed; ensure the backend performs this resolution
immediately before the outbound request, logs the resolved IPs and reason for
rejection, and fails the delivery with a clear error rather than relying solely
on client-side validation.
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/admin/components/Webhook/WebhookURLChecker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
🧬 Code graph analysis (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
admin-ui/plugins/admin/components/Webhook/index.ts (2)
isAllowed(7-7)isValid(7-7)
🔇 Additional comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
1-12: BLOCKED_SCHEMES set is conservative and consistent with HTTPS-only policyThe blocked schemes list is sane and, combined with the HTTPS-only
PATTERN, gives a clear and consistent “HTTPS or nothing” story for webhooks. No issues here.
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/admin/components/Webhook/WebhookURLChecker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
🔇 Additional comments (1)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (1)
1-12: Overall URL validation flow looks solid.Aside from the ranges noted above, the combination of:
- scheme blocking via
BLOCKED_SCHEMES(Line [1]–[11]),URLparsing and hostname-based private/reserved checks inisAllowed(Lines [60]–[76]),- and strict HTTPS + path/query/fragment structure in
PATTERN+isValid(Lines [58]–[83])is coherent and significantly reduces SSRF risk at the UI layer. Exporting
isAllowedseparately also gives you flexibility for lighter-weight checks when regex structure isn’t required.Also applies to: 60-83
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/admin/components/Webhook/WebhookURLChecker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: devrimyatar
Repo: GluuFederation/flex PR: 2440
File: flex-linux-setup/flex_linux_setup/flex_setup.py:519-519
Timestamp: 2025-11-11T15:17:34.651Z
Learning: In flex-linux-setup/flex_linux_setup/flex_setup.py, the webhook LDIF (aui_webhook.ldif) should only be imported during fresh Admin UI installations, not during updates. The import is correctly guarded by the check `client_check_result['2001.'] == -1` which ensures it only runs when the Admin UI Web Client doesn't already exist. Docker update workflows differ from Linux update workflows in this regard.
🔇 Additional comments (6)
admin-ui/plugins/admin/components/Webhook/WebhookURLChecker.ts (6)
1-12: LGTM! Comprehensive scheme blocklist.The
BLOCKED_SCHEMESarray covers all major SSRF protocol vectors, including the rare-but-exploitable schemes (gopher, dict, tftp) that were added in response to past reviews. This provides strong defense-in-depth against protocol smuggling attacks.
24-45: LGTM! Complete IPv4 private range coverage.The IPv4 checks correctly cover all RFC1918 private ranges (
10.0.0.0/8,172.16.0.0/12,192.168.0.0/16), link-local (169.254.0.0/16), loopback (127.0.0.0/8), and carrier-grade NAT (100.64.0.0/10). The second-octet range validation for172.16.0.0/12and100.64.0.0/10correctly prevents false positives on public IP ranges.
47-65: LGTM! Comprehensive IPv6 reserved range blocking.The IPv6 checks are correctly scoped to IPv6 literals (via the
includes(':')guard) to prevent false positives on DNS hostnames. The function blocks:
- IPv4-mapped addresses (
::ffff:0:0/96)- Unique local addresses (
fc00::/7covering bothfcandfdprefixes)- Full link-local range (
fe80::/10correctly coveringfe8-febprefixes, not justfe80:)- Documentation range (
2001:db8::/32)- Multicast (
ff00::/8)This provides robust protection against IPv6-based SSRF vectors.
70-71: LGTM! Pattern comprehensively validates HTTPS webhook URLs.The regex correctly enforces HTTPS-only, supports both DNS hostnames (with required domain structure) and IPv6 literals in bracket notation (
[...]), and allows optional port, path, query parameters, and fragments. The inclusion of query parameter support ((\?[^\s#]*)?) aligns with the documented requirement for shortcode usage in query strings (e.g.,?action=${action}&user_id=${userId}).
73-89: LGTM! Strong SSRF protection via proper URL parsing.The
isAllowedfunction correctly:
- Uses the WHATWG URL constructor to parse and normalize the URL, preventing user-info bypass attacks (e.g.,
https://attacker.com@127.0.0.1)- Blocks all disallowed schemes, not just HTTP
- Crucially, applies the private/localhost check to all protocols (line 81), not just HTTPS—this prevents bypasses via
wss://192.168.1.1or other non-blocked schemes- Returns
falsefor malformed URLs via thecatchblockThis addresses the critical SSRF vulnerabilities flagged in earlier reviews.
91-98: LGTM! Validation flow is sound and well-structured.The
isValidfunction properly chains:
- Null/undefined guard (early return for missing input)
- Security validation via
isAllowed(SSRF protection)- Format validation via
PATTERN(structural correctness)The exports provide both granular (
isAllowed) and comprehensive (isValid) validation APIs. Note that this client-side validation cannot prevent DNS rebinding attacks; ensure webhook URLs are re-validated server-side immediately before invocation, as noted in past reviews.
|
3 participants
#2433
Closes #2433
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Internationalization
✏️ Tip: You can customize this high-level summary in your review settings.