fix(admin-ui): Migrate SAML plugin to use orval generated typescript client #2575
Conversation
syntrydy
added
comp-admin-ui
📝 WalkthroughWalkthroughReplaces the SAML Redux/Saga/API layer with an orval-generated React Query hook layer and updates UI, helpers, and types to consume the new hooks and payload shapes. Deleted slices, sagas, and SamlApi; added consolidated hooks, audit logging, toasts, and cache invalidation. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Component (Form/List)
participant Hooks as React Query Hooks
participant API as Orval HTTP Client
participant Audit as AuditLogger
participant Cache as ReactQuery Cache
UI->>Hooks: call mutateAsync(payload) / call query
Hooks->>API: HTTP POST/PUT/DELETE or GET
API-->>Hooks: response (success / error)
Hooks->>Audit: logAuditUserAction(event, payload, ctx)
Hooks->>Cache: invalidate SAML query keys / update cache
Hooks-->>UI: resolve mutation / return query data
Hooks-->>UI: onError -> show toast
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/saml/helper/utils.ts (1)
67-67: Inconsistent fallback pattern forvalidateSignature.Unlike other fields (e.g.,
signingCertificate,principalType),validateSignatureonly reads fromconfig.validateSignaturewithout falling back toconfigs?.validateSignature. This may cause the value to be lost if it exists at the root level instead of within the nestedconfigobject.🔧 Suggested fix
- const validateSignature = config.validateSignature || '' + const validateSignature = config.validateSignature || configs?.validateSignature || ''
🤖 Fix all issues with AI agents
In @admin-ui/plugins/saml/components/hooks/useSamlApi.ts:
- Around line 124-127: The four repeated selectors (token, userinfo, clientId,
ipAddress) in useSamlApi.ts should be extracted into a reusable hook named
useAuditContext; implement a function useAuditContext that calls useSelector for
each of those selectors and returns { token, userinfo, clientId, ipAddress },
then replace the inline selector calls inside each mutation hook in useSamlApi
(and any other files duplicating them) with a single const { token, userinfo,
clientId, ipAddress } = useAuditContext() to remove duplication and centralize
selector logic.
- Around line 151-161: The mutateAsync function inside the
useUpdateSamlConfiguration hook lacks error handling; wrap the await
baseMutation.mutateAsync({ data }) and subsequent steps in a try/catch, on
success keep invalidateQueries(getGetSamlPropertiesQueryKey()),
dispatch(updateToast(true, 'success')) and logAudit(userMessage, data), and in
catch dispatch an error toast (e.g., dispatch(updateToast(true, 'error')) or a
dedicated error action) and rethrow or return the error so callers can handle
it; update the mutateAsync closure to reference the same symbols (mutateAsync,
baseMutation, queryClient.invalidateQueries, getGetSamlPropertiesQueryKey,
dispatch, updateToast, logAudit) so behavior matches other mutation hooks.
In @admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx:
- Around line 271-276: The cleanup effect in WebsiteSsoIdentityProviderForm
currently lists createIdentityProvider and updateIdentityProvider in its
dependency array which can trigger premature cleanup when their internal state
changes; fix this by capturing stable refs for those objects (e.g.,
ipCreateRef.current = createIdentityProvider and ipUpdateRef.current =
updateIdentityProvider) and use an effect with an empty dependency array that
calls ipCreateRef.current.resetSavedForm() and
ipUpdateRef.current.resetSavedForm() on unmount; reference the existing
resetSavedForm method on createIdentityProvider and updateIdentityProvider and
mirror the same ref-based approach used in WebsiteSsoServiceProviderForm.
In @admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx:
- Around line 222-228: The cleanup effect in WebsiteSsoServiceProviderForm is
running on every render because the hook-returned objects
createTrustRelationship and updateTrustRelationship change references; change
the effect to depend only on stable callbacks (their resetSavedForm functions)
or capture those functions into refs and call the ref.current methods in the
cleanup, e.g., obtain stableResetCreate =
useRef(createTrustRelationship.resetSavedForm) / stableResetUpdate =
useRef(updateTrustRelationship.resetSavedForm) (or extract resetSavedForm into
its own hook variable) and then have a useEffect with an empty dependency array
that calls stableResetCreate.current(), stableResetUpdate.current(), and
dispatch(setClientSelectedScopes([])) in the cleanup so the cleanup only runs on
unmount.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
admin-ui/plugins/saml/components/SamlConfigurationForm.tsxadmin-ui/plugins/saml/components/SamlPage.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsxadmin-ui/plugins/saml/components/hooks/index.tsadmin-ui/plugins/saml/components/hooks/useSamlApi.tsadmin-ui/plugins/saml/helper/utils.tsadmin-ui/plugins/saml/plugin-metadata.jsadmin-ui/plugins/saml/redux/api/SamlApi.tsadmin-ui/plugins/saml/redux/features/SamlSlice.tsadmin-ui/plugins/saml/redux/sagas/SamlSaga.tsadmin-ui/plugins/saml/types/api.tsadmin-ui/plugins/saml/types/index.tsadmin-ui/plugins/saml/types/payloads.tsadmin-ui/plugins/saml/types/saga.tsadmin-ui/plugins/saml/types/state.ts
💤 Files with no reviewable changes (7)
- admin-ui/plugins/saml/types/state.ts
- admin-ui/plugins/saml/types/api.ts
- admin-ui/plugins/saml/types/payloads.ts
- admin-ui/plugins/saml/types/saga.ts
- admin-ui/plugins/saml/redux/api/SamlApi.ts
- admin-ui/plugins/saml/redux/features/SamlSlice.ts
- admin-ui/plugins/saml/redux/sagas/SamlSaga.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-05T18:24:41.713Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.713Z
Learning: In the GluuFederation/flex admin-ui codebase, `SetTitle` (imported from 'Utils/SetTitle') is a custom React hook that should be called at the component's top level, not inside `useEffect` or other hooks, because it internally manages side effects for setting page titles.
Applied to files:
admin-ui/plugins/saml/components/SamlPage.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx
📚 Learning: 2026-01-06T13:45:19.278Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts:4-101
Timestamp: 2026-01-06T13:45:19.278Z
Learning: In admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts, ensure Yup schema marks fields as nullable when the backend DTOs permit nulls and do not mark them as required. Align frontend validation with backend validation to prevent conflicts when editing existing configurations with null values; apply this pattern consistently to other Admin UI validation files where the backend is the source of truth.
Applied to files:
admin-ui/plugins/saml/types/index.tsadmin-ui/plugins/saml/components/hooks/index.tsadmin-ui/plugins/saml/helper/utils.tsadmin-ui/plugins/saml/components/hooks/useSamlApi.ts
📚 Learning: 2026-01-07T09:26:24.613Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:112-172
Timestamp: 2026-01-07T09:26:24.613Z
Learning: In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management.
Applied to files:
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx
📚 Learning: 2025-11-06T08:23:20.948Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
🔇 Additional comments (23)
admin-ui/plugins/saml/plugin-metadata.js (1)
69-70: LGTM! Clean removal of Redux infrastructure.The empty arrays for
reducersandsagascorrectly reflect the migration to React Query hooks. The plugin metadata structure is preserved while removing the Redux integration points.admin-ui/plugins/saml/types/index.ts (1)
15-30: LGTM! Well-organized type exports.The type surface is cleanly restructured to support the hook-based API. Payload types from
./payloadsand Orval-generated types fromJansConfigApiare properly re-exported, providing a unified public API for consumers.admin-ui/plugins/saml/helper/utils.ts (3)
18-32: LGTM! Well-structured internal augmentation type.The
IdentityProviderWithMetaDataFNtype properly extends the Orval-generatedIdentityProviderwith additional metadata fields. The index signature[key: string]: unknownin theconfigobject allows for forward compatibility with additional SAML configuration properties.
38-51: LGTM!The function signature appropriately handles multiple configuration sources through a union type, and the implementation safely handles undefined/null values with optional chaining and fallbacks.
141-143: Type assertion is acceptable but verify runtime shape.The cast for
profileConfigurationsis a pragmatic solution for type mismatches between Orval-generated types and form requirements. Ensure the actual runtime data structure matches this expected shape to prevent potential runtime errors.admin-ui/plugins/saml/components/hooks/useSamlApi.ts (3)
53-56: LGTM! Sensible cache configuration.5-minute stale time and 10-minute garbage collection time are reasonable defaults for SAML configuration data that doesn't change frequently.
109-119: LGTM! Consistent query pattern.The query hooks properly gate data fetching on authentication status and apply consistent cache configuration.
212-229: LGTM! Proper error handling pattern.The mutation correctly handles both success and error cases with appropriate toast notifications and state updates.
admin-ui/plugins/saml/components/hooks/index.ts (1)
1-21: LGTM! Clean barrel export.The index file provides a well-organized public API for the SAML hooks module, with proper separation between type and value exports.
admin-ui/plugins/saml/components/SamlPage.tsx (1)
42-48: LGTM! Simplified rendering structure.The removal of the parent-level loading overlay is appropriate since child components now manage their own loading states via React Query hooks. The layout structure is clean and straightforward.
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (2)
61-73: Error toast may be missing in form submission.The
handleSubmitcallback catches errors and logs them, but doesn't show an error toast to the user. However, sinceuseUpdateSamlConfiguration.mutateAsyncshould be updated to dispatch error toasts (as noted in the useSamlApi.ts review), this may become redundant once that fix is applied.If you implement the suggested fix in
useUpdateSamlConfiguration, you can remove the try/catch here since the hook will handle error feedback.💡 Alternative: rely on hook for error handling
const handleSubmit = useCallback( async (values: SamlConfigurationFormValues, messages: string) => { - try { - await updateConfigMutation.mutateAsync({ - data: values, - userMessage: messages, - }) - } catch (error) { - console.error('Failed to update SAML configuration:', error) - } + await updateConfigMutation.mutateAsync({ + data: values, + userMessage: messages, + }) }, [updateConfigMutation], )
27-30: LGTM! Well-structured hook-based data flow.The component correctly uses React Query hooks for data fetching and mutations, with a combined loading state that covers both the initial query and pending mutations.
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (3)
28-34: LGTM!Clean import of hooks and types from the new hooks module, properly aligned with the migration objectives.
80-84: LGTM!Good pattern deriving loading and savedForm states from both mutation hooks. The logical OR for
isPendingcorrectly blocks UI during either operation.
163-199: LGTM!Well-structured async mutation flow with proper try/catch error handling. The conditional create vs update logic based on
configspresence is clear and correct.admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (3)
53-66: LGTM!Clean extraction of query params into a memoized object, and proper destructuring of hook results. The
entriesandtotalEntriesCountfallbacks to empty array and 0 are defensive and correct.
129-150: LGTM!Async deletion properly wrapped in try/catch with error logging. The modal toggle after the try/catch block ensures UI cleanup regardless of outcome.
227-294: LGTM!Table actions correctly typed with
Action<IdentityProvider>[]and handlers properly handle the array case from MaterialTable's onClick signature. The explicit cast on line 293 for the refresh action'sdata-testidis a reasonable workaround.admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (3)
47-53: LGTM!Clean hook integration with proper destructuring. The default empty array for
websiteSsoServiceProvidersensures the table always has valid data.
89-107: LGTM!Consistent async deletion pattern with proper error handling. The flow matches the established pattern in other migrated components.
109-111: LGTM!Using
refetchfrom the query hook is the correct approach for manual refresh with React Query.admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (2)
59-64: LGTM!Consistent hook usage pattern matching the service provider form. Loading and savedForm state derivation is correct.
128-153: LGTM!Well-structured payload construction with proper type casting. The conditional Blob creation for metadata file handles both file upload and non-file scenarios correctly.
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx:
- Around line 178-185: The trustRelationshipFormData construction always sends
metaDataFile as new Blob([]), which ignores the extracted
metaDataFileImportedFlag; change the metaDataFile assignment in
trustRelationshipFormData (used in WebsiteSsoServiceProviderForm) to mirror
WebsiteSsoIdentityProviderForm by conditionally including the actual
metaDataFile only when metaDataFileImportedFlag is true and metaDataFile is a
File (e.g., metaDataFileImportedFlag && metaDataFile && metaDataFile instanceof
File ? metaDataFile : new Blob([])) or alternatively omit metaDataFile entirely
when the flag is false so updates don’t overwrite existing metadata.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/components/hooks/useSamlApi.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-06T08:23:20.948Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
📚 Learning: 2025-12-05T18:24:41.713Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.713Z
Learning: In the GluuFederation/flex admin-ui codebase, `SetTitle` (imported from 'Utils/SetTitle') is a custom React hook that should be called at the component's top level, not inside `useEffect` or other hooks, because it internally manages side effects for setting page titles.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2026-01-07T09:26:24.613Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:112-172
Timestamp: 2026-01-07T09:26:24.613Z
Learning: In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2025-11-03T08:47:33.943Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2404
File: admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx:70-77
Timestamp: 2025-11-03T08:47:33.943Z
Learning: In the admin-ui forms (Gluuformfooter component), the Back button should always navigate to '/home/dashboard' rather than using history-based navigation (navigate(-1)), as clarified by faisalsiddique4400 in PR #2404.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
📚 Learning: 2025-11-10T14:18:58.310Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:107-109
Timestamp: 2025-11-10T14:18:58.310Z
Learning: In the Cedarling configuration page (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), console.error calls should be kept in catch blocks when there are multiple operations in the try block, as they help print the whole error object with details for debugging unexpected errors.
Applied to files:
admin-ui/plugins/saml/components/hooks/useSamlApi.ts
📚 Learning: 2026-01-06T13:45:19.278Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts:4-101
Timestamp: 2026-01-06T13:45:19.278Z
Learning: In admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts, ensure Yup schema marks fields as nullable when the backend DTOs permit nulls and do not mark them as required. Align frontend validation with backend validation to prevent conflicts when editing existing configurations with null values; apply this pattern consistently to other Admin UI validation files where the backend is the source of truth.
Applied to files:
admin-ui/plugins/saml/components/hooks/useSamlApi.ts
🔇 Additional comments (15)
admin-ui/plugins/saml/components/hooks/useSamlApi.ts (8)
1-31: LGTM!Clean imports organization with orval-generated types and hooks, audit utilities, and React Query primitives. The separation of orval types (aliased as
OrvalIdentityProvider,OrvalTrustRelationship) for extension is a good pattern.
33-51: LGTM!The interface extensions add form-specific fields (
idpMetaDataFN,spMetaDataFN,config) while preserving the orval-generated base types. The index signature onconfigprovides flexibility for additional SAML configuration properties.
53-72: LGTM!Cache configuration with 5-minute stale time and 10-minute garbage collection is reasonable for SAML configuration data. Using orval's query key generators ensures consistency with the generated client.
81-114: LGTM!The
useAuditContexthook centralizes auth/user data extraction, andcreateAuditLoggerfactory cleanly encapsulates audit logging logic. Theconsole.errorin the catch block is appropriate for debugging audit failures without disrupting the main flow. Based on learnings, keeping console.error for unexpected errors is acceptable.
163-200: LGTM!The
useUpdateSamlConfigurationhook properly:
- Wraps the orval mutation with audit logging and toast notifications
- Invalidates the query cache on success
- Maintains proper error handling with toast feedback
- Uses correct dependency arrays in
useCallback
214-260: LGTM!The
useCreateIdentityProviderhook properly manages the mutation lifecycle with:
savedFormstate for tracking successful saves (used by forms for navigation)resetSavedFormcallback for cleanup on unmount- Consistent audit logging and cache invalidation patterns
The pattern is repeated across create/update hooks - this duplication is acceptable for maintainability and explicit behavior per mutation type.
310-346: LGTM!The delete mutation hooks properly invalidate the cache and log audit actions. The absence of
savedFormstate for delete operations is appropriate since deletion doesn't require the same navigation flow as create/update.
348-454: LGTM!Trust relationship hooks follow the same consistent patterns established for identity providers - proper cache management, audit logging, and form state tracking. The code is well-structured and maintainable.
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (3)
28-34: LGTM!Clean import migration from Redux slice to the new hook-based API. The type imports (
TrustRelationship,TrustRelationshipForm) align with the orval-generated client.
80-89: LGTM!The ref pattern for
resetSavedFormfunctions avoids stale closures in the cleanup effect. DerivingloadingandsavedFormfrom both create and update hooks ensures proper UI state regardless of which operation is in progress.
221-233: LGTM!The navigation effect correctly triggers on
savedFormchanges. The cleanup effect properly uses refs to call the currentresetSavedFormfunctions and clears selected scopes on unmount. The[dispatch]dependency is stable (from Redux), making this effectively run only on unmount.admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (4)
28-33: LGTM!Clean import migration to hook-based API with proper type imports for
IdentityProviderandBrokerIdentityProviderForm.
60-69: LGTM!Consistent hook initialization pattern with refs for
resetSavedFormfunctions, matching the service provider form approach.
141-157: LGTM!The async submission logic properly handles create vs update based on
configsexistence. Error handling withconsole.erroris appropriate for debugging while the toast notifications (handled in hooks) provide user feedback. Based on learnings, keeping console.error for unexpected errors is acceptable.
276-281: LGTM!The cleanup effect with an empty dependency array correctly runs only on unmount, using refs to call the current
resetSavedFormfunctions without stale closure issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx:
- Around line 143-157: The catch block after createIdentityProvider.mutateAsync
/ updateIdentityProvider.mutateAsync only logs to console and provides no user
feedback; update the catch to surface the error to the user by calling the app's
notification/toast utility (e.g., toast.error) or by setting a local form error
state (e.g., setFormError) and ensure the component displays that error; retain
the console.error for debugging but add a clear, user-friendly message that
includes the error message so users know the save failed.
In @admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx:
- Around line 189-203: The catch block only logs errors to the console, leaving
users unaware when save fails; update the error handling in the try/catch around
createTrustRelationship.mutateAsync and updateTrustRelationship.mutateAsync to
surface the error to the user by invoking the app's notification/toast utility
(e.g., toast.error or showNotification) with a clear message and include the
caught error message, and optionally set a local error state (e.g.,
setSaveError) or disable any loading flag so the UI reflects the failure; ensure
you reference the same trustRelationshipFormData and user_message context when
reporting the error.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-06T08:23:20.948Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
📚 Learning: 2025-12-05T18:24:41.713Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.713Z
Learning: In the GluuFederation/flex admin-ui codebase, `SetTitle` (imported from 'Utils/SetTitle') is a custom React hook that should be called at the component's top level, not inside `useEffect` or other hooks, because it internally manages side effects for setting page titles.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2026-01-07T09:26:24.613Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:112-172
Timestamp: 2026-01-07T09:26:24.613Z
Learning: In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2025-11-03T08:47:33.943Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2404
File: admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx:70-77
Timestamp: 2025-11-03T08:47:33.943Z
Learning: In the admin-ui forms (Gluuformfooter component), the Back button should always navigate to '/home/dashboard' rather than using history-based navigation (navigate(-1)), as clarified by faisalsiddique4400 in PR #2404.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
🔇 Additional comments (7)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (3)
80-89: LGTM! Well-structured hook initialization and ref pattern.The ref pattern for
resetSavedFormfunctions correctly prevents stale closure issues in the cleanup effect. DerivedloadingandsavedFormstates from hook results are appropriate.
229-235: Cleanup effect correctly uses refs to avoid stale closures.The ref-based cleanup pattern ensures the latest
resetSavedFormfunctions are called on unmount, regardless of when they were captured. This is a solid pattern for avoiding dependency issues in cleanup effects.
177-187: Verify backend API expectations formetaDataFileproperty handling.The concern about sending an empty
Blob([])when no file is provided is reasonable—this can cause validation or processing errors on the backend. However, the actual impact depends on the API specification. The proposed fix using conditional spread syntax is a sound pattern, but verification requires checking:
- Whether the backend API spec makes
metaDataFileoptional- Whether the API can handle omitted properties or requires explicit
undefined/nullvalues- Whether the current empty
Blobimplementation causes actual validation failuresReview the
TrustRelationshipFormtype definition and the backend endpoint documentation to confirm whether the fix is necessary.admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (4)
59-69: LGTM! Consistent hook initialization pattern.The ref pattern for stabilizing
resetSavedFormfunctions mirrors the approach inWebsiteSsoServiceProviderForm, maintaining consistency across the codebase.
85-85: Correct usage ofSetTitleat component top level.Per codebase conventions,
SetTitleis called at the component's top level, not insideuseEffect, which is the correct pattern. Based on learnings, this aligns with how the custom hook internally manages side effects.
278-283: The cleanup effect is correct as written. The IdentityProvider form does not managesetClientSelectedScopesat all—unlike the SP form, this component has no scope management logic. The different cleanup patterns reflect intentional design differences, not an oversight.Likely an incorrect or invalid review comment.
133-141: Verify backend API handling of empty metadata file field.The pattern of sending
new Blob([])when no metadata file is selected appears in both the identity provider form (lines 133-141) and the service provider form (lines 175-195), suggesting this may be intentional. However, without access to backend validation logic, it's unclear whether the backend correctly handles an empty Blob or if it should receive an omitted field instead. Confirm with the backend team whether the API validation fails with an empty Blob, and if so, apply the proposed fix to conditionally includemetaDataFileonly when a valid file exists.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @admin-ui/plugins/saml/components/hooks/useSamlApi.ts:
- Around line 213-258: useCreateIdentityProvider and useUpdateIdentityProvider
(and the trust relationship hooks) duplicate nearly identical mutation logic;
extract a reusable factory like createMutationHook<TData, TParams> that accepts
the base mutation (e.g., usePostSamlIdentityProvider /
usePutSamlIdentityProvider), the queryKey getter
(getGetSamlIdentityProviderQueryKey or trust-related key), audit options (action
CREATE/UPDATE and AUDIT_RESOURCE_NAMES.IDENTITY_BROKERING), a payloadMapper for
audit payload, and a flag to track savedForm; implement the shared
mutateAsync/error
handling/invalidations/toast/setSavedForm/logAudit/resetSavedForm inside the
factory and replace the individual hooks (useCreateIdentityProvider,
useUpdateIdentityProvider, trust hooks) to call createMutationHook with the
proper arguments and return the same shape (...baseMutation, mutateAsync,
savedForm, resetSavedForm).
In @admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:
- Around line 63-75: The catch block in handleSubmit swallows the caught error
and only dispatches updateToast; log the caught error before dispatching the
toast by adding a console.error with context (e.g., "Error updating SAML
config:") referencing the caught error variable in the catch, so that
updateConfigMutation.mutateAsync failures are printed for debugging while
keeping the existing dispatch(updateToast(...)) behavior.
In @admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx:
- Around line 158-160: In the catch block inside the
WebsiteSsoIdentityProviderForm component (where updateToast is dispatched), log
the caught error before calling dispatch(updateToast(...)); specifically,
capture the error object in the catch and call the application's logger or
console (e.g., console.error or the component's logging utility) with a clear
message and the error, then proceed to dispatch(updateToast(true, 'error')) so
you maintain existing behavior while providing diagnostic details.
In @admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx:
- Around line 202-204: In the catch block inside WebsiteSsoServiceProviderForm
(the code that currently calls dispatch(updateToast(true, 'error'))), log the
caught error before dispatching the toast so debugging info isn’t lost; add a
console.error(error) (or use the existing logger if available) immediately prior
to dispatch(updateToast(...)) to include stack/message details.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
admin-ui/plugins/saml/components/SamlConfigurationForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/components/hooks/useSamlApi.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-01-07T09:26:24.613Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsx:112-172
Timestamp: 2026-01-07T09:26:24.613Z
Learning: In React components using Formik, do not extract individual Formik methods (such as setValues, setTouched, validateForm) or properties (such as touched, errors) into separate variables/refs to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management.
Applied to files:
admin-ui/plugins/saml/components/SamlConfigurationForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2025-12-05T18:24:41.713Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.713Z
Learning: In the GluuFederation/flex admin-ui codebase, `SetTitle` (imported from 'Utils/SetTitle') is a custom React hook that should be called at the component's top level, not inside `useEffect` or other hooks, because it internally manages side effects for setting page titles.
Applied to files:
admin-ui/plugins/saml/components/SamlConfigurationForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2025-11-06T08:23:20.948Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
📚 Learning: 2025-11-10T14:18:58.310Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:107-109
Timestamp: 2025-11-10T14:18:58.310Z
Learning: In the Cedarling configuration page (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), console.error calls should be kept in catch blocks when there are multiple operations in the try block, as they help print the whole error object with details for debugging unexpected errors.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/components/hooks/useSamlApi.ts
📚 Learning: 2025-11-03T08:47:33.943Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2404
File: admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx:70-77
Timestamp: 2025-11-03T08:47:33.943Z
Learning: In the admin-ui forms (Gluuformfooter component), the Back button should always navigate to '/home/dashboard' rather than using history-based navigation (navigate(-1)), as clarified by faisalsiddique4400 in PR #2404.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
📚 Learning: 2026-01-06T13:45:19.278Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2561
File: admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts:4-101
Timestamp: 2026-01-06T13:45:19.278Z
Learning: In admin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/validations.ts, ensure Yup schema marks fields as nullable when the backend DTOs permit nulls and do not mark them as required. Align frontend validation with backend validation to prevent conflicts when editing existing configurations with null values; apply this pattern consistently to other Admin UI validation files where the backend is the source of truth.
Applied to files:
admin-ui/plugins/saml/components/hooks/useSamlApi.ts
🧬 Code graph analysis (2)
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (3)
admin-ui/plugins/saml/components/hooks/useSamlApi.ts (2)
useSamlConfiguration(151-161)useUpdateSamlConfiguration(163-199)admin-ui/plugins/saml/components/hooks/index.ts (2)
useSamlConfiguration(2-2)useUpdateSamlConfiguration(3-3)admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)
admin-ui/plugins/saml/components/hooks/useSamlApi.ts (3)
admin-ui/plugins/saml/types/index.ts (5)
IdentityProvider(25-25)TrustRelationship(27-27)SamlAppConfiguration(24-24)BrokerIdentityProviderForm(26-26)TrustRelationshipForm(28-28)admin-ui/app/utils/AuditLogger.ts (3)
BasicUserInfo(5-8)logAuditUserAction(25-66)logAudit(68-68)admin-ui/plugins/saml/helper/constants.ts (1)
AUDIT_RESOURCE_NAMES(30-34)
🔇 Additional comments (13)
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (1)
30-34: LGTM on hook migration and loading state derivation.The migration from Redux selectors to React Query hooks is well-implemented. Loading state correctly combines both query loading and mutation pending states.
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (3)
81-91: LGTM on hook initialization and state derivation.The refs pattern for capturing reset functions is correctly implemented to ensure stable references in the cleanup effect. Loading and savedForm states are properly derived from both create and update hooks.
230-236: LGTM on cleanup effect implementation.Using refs to capture the latest reset functions avoids stale closure issues. The empty dependency array (except
dispatch) is correct since the refs are updated on each render.
178-188: Verify if empty Blob field in metaDataFile causes API issues.The code sends an empty
new Blob([])when no metadata file is provided (lines 184-186). Without access to the API schema, confirm whether the backend expects this field to be omitted entirely rather than present with an empty Blob, or whether it handles empty Blobs correctly.admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (3)
62-71: LGTM on hook initialization pattern.Consistent with the service provider form implementation. The refs pattern correctly handles the reset functions.
136-144: Same concern: Empty Blob when no metadata file is provided.Same pattern as the service provider form. Verify the API handles empty blobs correctly or consider making
metaDataFileoptional/undefined when not provided.
281-286: LGTM on cleanup effect.Properly resets both hook states on unmount using the refs pattern.
admin-ui/plugins/saml/components/hooks/useSamlApi.ts (6)
33-51: LGTM on type extensions.Extending the Orval-generated types with additional fields (
idpMetaDataFN,spMetaDataFN,config) is a clean approach to add missing properties while maintaining type safety.
53-62: LGTM on cache configuration and query keys.Centralizing cache timings (5 min stale, 10 min GC) and query key functions promotes consistency across all SAML hooks.
92-114: Well-designed audit logger factory with proper error handling.The
createAuditLoggerfunction is a good abstraction that:
- Encapsulates audit context and action type
- Handles errors gracefully with console logging
- Returns a typed async function for reuse
This aligns with the learning that console.error calls should be kept for debugging unexpected errors.
179-193: Audit logging is fire-and-forget - verify this is intentional.The
logAuditcall on line 186 is not awaited. This means:
- The mutation returns before audit logging completes
- If the component unmounts quickly, the audit log might fail silently
- The audit error is caught inside
createAuditLogger(line 110-112), so it won't affect the mutation flowIf audit logging is critical for compliance, consider awaiting it. If it's best-effort telemetry, the current approach is acceptable.
Also,
dispatchis missing from the dependency array on line 192, thoughuseDispatchreturns a stable reference so this is technically safe.
307-342: LGTM on delete mutation implementation.Delete operations correctly:
- Invalidate the relevant query cache
- Show success toast
- Log audit action
- Propagate errors to callers
The simpler structure (no savedForm tracking) is appropriate for delete operations.
450-485: LGTM on delete trust relationship mutation.Consistent implementation with the identity provider delete mutation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (1)
165-171: Consider awaiting the asynchandleSubmitcall.
handleSubmitis an async function but is invoked withoutawait. While errors are caught internally, this could lead to unexpected behavior if the caller needs to know when submission completes, and may trigger linter warnings about floating promises.♻️ Suggested fix
const submitForm = useCallback( - (messages: string) => { + async (messages: string) => { toggle() - handleSubmit(formik.values, messages) + await handleSubmit(formik.values, messages) }, [toggle, formik.values, handleSubmit], )
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx`:
- Line 206: The component depends on the whole mutation hook objects
(createTrustRelationship, updateTrustRelationship) in the dependency array which
is unstable; instead, destructure the mutateAsync functions at the hook
declaration (e.g., const { mutateAsync: createTrustRelationshipAsync } =
useCreateTrustRelationship(...) and const { mutateAsync:
updateTrustRelationshipAsync } = useUpdateTrustRelationship(...)) and use those
stable references in handleSubmit and the dependency array; if those mutateAsync
functions are used in callbacks, wrap the handlers that call them (e.g.,
handleSubmit) in useCallback and include only the destructured functions and
stable values (like configs) in the deps; also audit other usages (lines ~84–90)
and similarly access any other needed properties directly from the hook return
to avoid depending on the whole hook object.
♻️ Duplicate comments (3)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (2)
158-160: Add console.error for debugging purposes.The catch block dispatches an error toast but doesn't log the error details, making debugging difficult when failures occur.
🔧 Suggested improvement
} catch (error) { + console.error('Failed to save identity provider:', error) dispatch(updateToast(true, 'error')) }
136-144: Type assertion masks potential type mismatch.The
as IdentityProvidercast on line 139 bypasses TypeScript's type checking betweenIdentityProviderPayload(returned bybuildIdentityProviderPayload) andIdentityProvider(expected byBrokerIdentityProviderForm). Consider aligning these types or creating an explicit mapper function.admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
202-204: Add error logging before dispatching the toast.The error details are discarded, which makes debugging production issues difficult. Based on learnings from this codebase,
console.errorcalls should be kept in catch blocks when there are multiple operations in the try block.♻️ Suggested improvement
} catch (error) { + console.error('Failed to save trust relationship:', error) dispatch(updateToast(true, 'error')) }
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/saml/components/hooks/useSamlApi.ts`:
- Around line 230-246: The useCallback for mutateAsync omits dispatch from its
dependency array and is inconsistent with other hooks; update the dependency
array for the mutateAsync callback to include dispatch (alongside baseMutation,
queryClient, logAudit) and audit other mutation hooks in this file to
consistently depend on the same symbols (either baseMutation or
baseMutation.mutateAsync across hooks) so all callbacks reference the same
dependency form and include dispatch.
- Around line 323-336: The delete mutation handlers (mutateAsync in
useDeleteIdentityProvider and the analogous function in
useDeleteTrustRelationshipMutation) currently catch errors and re-throw without
notifying the user; update each catch block to dispatch an error toast via
dispatch(updateToast(true, 'error', ...)) including a helpful message or the
caught error message if present, then re-throw (or return) as before; ensure you
reference the existing symbols mutateAsync, useDeleteIdentityProvider,
useDeleteTrustRelationshipMutation, dispatch, and updateToast when making the
change so the error toast is consistently shown on delete failures.
In `@admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx`:
- Around line 136-144: The code is unsafely casting identityProviderData to
IdentityProvider when building brokerIdentityProviderFormData; replace the cast
with an explicit mapper that converts IdentityProviderPayload (returned by
buildIdentityProviderPayload) into the exact IdentityProvider shape (including
OrvalIdentityProvider fields plus idpMetaDataFN and config) and use that mapped
object for the identityProvider field in BrokerIdentityProviderForm; implement a
function (e.g., mapPayloadToIdentityProvider(payload: IdentityProviderPayload):
IdentityProvider) and call it instead of using the as IdentityProvider
assertion, preserving the existing metaDataFile/metaDataFileImportedFlag logic.
- Around line 62-72: The form uses the full hook objects
(useCreateIdentityProvider/useUpdateIdentityProvider) instead of destructuring
mutateAsync, causing unstable references in the handleSubmit dependency array;
refactor to destructure mutateAsync from createIdentityProvider and
updateIdentityProvider (e.g., const { mutateAsync: createMutateAsync } =
useCreateIdentityProvider()) and use those functions in handleSubmit, then
update the dependency array to reference only those stable mutateAsync variables
(and any form values) instead of the whole hook objects; ensure
createResetRef/updateResetRef still read resetSavedForm from the original hook
return values if needed.
♻️ Duplicate comments (3)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
212-214: Consider logging the error before dispatching toast.Per repository learnings,
console.errorcalls help debug unexpected errors when there are multiple operations in the try block. Adding logging would aid troubleshooting.♻️ Suggested improvement
} catch (error) { + console.error('Failed to save trust relationship:', error) dispatch(updateToast(true, 'error')) }admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (1)
158-160: Consider logging the error before dispatching toast.Per repository learnings,
console.errorcalls help debug unexpected errors when there are multiple operations in the try block.♻️ Suggested improvement
} catch (error) { + console.error('Failed to save identity provider:', error) dispatch(updateToast(true, 'error')) }admin-ui/plugins/saml/components/hooks/useSamlApi.ts (1)
179-193: Missing error toast dispatch on failure inuseUpdateSamlConfiguration.The catch block re-throws the error but doesn't dispatch an error toast like other mutation hooks (e.g., the form components dispatch
updateToast(true, 'error')in their catch blocks). This creates inconsistent user feedback—successful updates show a toast, but failures are silent.🔧 Suggested fix
try { const result = await baseMutation.mutateAsync({ data }) await queryClient.invalidateQueries({ queryKey: getGetSamlPropertiesQueryKey() }) dispatch(updateToast(true, 'success')) logAudit(userMessage, data) return result } catch (error) { + dispatch(updateToast(true, 'error')) throw error }
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@admin-ui/plugins/saml/components/hooks/useSamlApi.ts`:
- Around line 226-236: Update useSamlApi.ts so all mutation hooks handle errors
consistently by dispatching error toasts on catch like the delete hooks; for the
create/update hooks (useCreateIdentityProvider, useUpdateIdentityProvider,
useCreateTrustRelationship, useUpdateTrustRelationship) add a
dispatch(updateToast(true, 'error', <message>)) in their catch blocks
(preserving existing behavior: setSavedForm(false), rethrow or return as
appropriate), and ensure the same pattern is used around
baseMutation.mutateAsync, queryClient.invalidateQueries and logAudit calls to
mirror how useDeleteIdentityProvider and useDeleteTrustRelationshipMutation
report failures to users.
♻️ Duplicate comments (3)
admin-ui/plugins/saml/components/hooks/useSamlApi.ts (1)
176-186: Missing error handling inuseUpdateSamlConfiguration.Unlike other mutation hooks (
useCreateIdentityProvider,useDeleteIdentityProvider, etc.), this hook doesn't wrap the mutation in try/catch and doesn't dispatch an error toast on failure. Users won't see feedback if the SAML configuration update fails at the hook level.🔧 Suggested fix
const mutateAsync = useCallback( async (params: UpdateSamlConfigurationParams): Promise<SamlAppConfiguration> => { const { data, userMessage } = params - const result = await baseMutation.mutateAsync({ data }) - await queryClient.invalidateQueries({ queryKey: getGetSamlPropertiesQueryKey() }) - dispatch(updateToast(true, 'success')) - logAudit(userMessage, data) - return result + try { + const result = await baseMutation.mutateAsync({ data }) + await queryClient.invalidateQueries({ queryKey: getGetSamlPropertiesQueryKey() }) + dispatch(updateToast(true, 'success')) + logAudit(userMessage, data) + return result + } catch (error) { + const errorMessage = + error instanceof Error ? error.message : 'Failed to update SAML configuration' + dispatch(updateToast(true, 'error', errorMessage)) + throw error + } }, [baseMutation.mutateAsync, queryClient, dispatch, logAudit], )admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (1)
146-154: Type assertion bypasses TypeScript checking.The
as IdentityProvidercast (line 149) masks a potential type mismatch betweenbuildIdentityProviderPayloadreturn type and the expectedIdentityProviderinterface. While functional, this bypasses compile-time safety.Consider creating an explicit mapper function that transforms the payload into the expected
IdentityProviderstructure for better type safety.admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (1)
70-76: Consider logging the error before dispatching toast.The error details are extracted for the toast message but not logged to console. Based on learnings,
console.errorcalls help print detailed error information for debugging unexpected errors.♻️ Suggested improvement
} catch (error) { + console.error('Failed to update SAML configuration:', error) const errorMessage = error instanceof Error ? error.message : (error as { response?: { data?: { message?: string } } })?.response?.data?.message || t('messages.error_in_saving') dispatch(updateToast(true, 'error', errorMessage)) }
#2574
Closes #2574
Summary by CodeRabbit
Improvements
Refactor
UX
✏️ Tip: You can customize this high-level summary in your review settings.