feat(admin): adding uniformity in cancel and back buttons present along with TS migration, Tabs navigation throttling issue and type safe implementation in SAML #2498
Conversation
…i-issue-2361-auth-server
…i-issue-2361-auth-server
faisalsiddique4400
requested review from
duttarnab and
syntrydy
as code owners
📝 WalkthroughWalkthroughRefactors the SAML plugin from JavaScript to TypeScript: adds typed components, validations, helpers, API wrapper, saga generators, Redux typing; replaces several JS components with TSX counterparts and updates localization and tab navigation guards. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Browser UI (Formik forms)
participant Saga as Redux-Saga
participant API as SamlApi (TS)
participant Store as Redux Store
participant Webhook as Webhook/Audit
UI->>Saga: dispatch(createSamlIdentity { action_data: FormData })
activate Saga
Saga->>API: postSamlIdentity({ formdata, token })
activate API
API-->>Saga: response (created identity / error)
deactivate API
alt success
Saga->>Store: dispatch(createSamlIdentityResponse)
Saga->>Webhook: postUserAction / trigger webhook
Saga->>UI: dispatch(toggleSavedFormFlag(true))
else error
Saga->>UI: dispatch(updateToast(error))
Saga->>Store: dispatch(toggleSavedFormFlag(false))
end
deactivate Saga
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
…i-issue-2361-saml-1
mo-auto
added
area-documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
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/WebsiteSsoTrustRelationshipForm.tsx (1)
348-366: Key with JSON.stringify may impact performance.Using
JSON.stringify(formik.values.releasedAttributes)as part of the component key could cause unnecessary re-renders if the array is large. Consider using a more stable key strategy.<GluuTypeAheadForDn - key={`releasedAttributes-${JSON.stringify(formik.values.releasedAttributes)}`} + key={`releasedAttributes-${(formik.values.releasedAttributes || []).length}`} name="releasedAttributes"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (38)
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/app/routes/Apps/Gluu/GluuTabs.tsx(1 hunks)admin-ui/app/routes/Apps/Gluu/GluuUploadFile.tsx(3 hunks)admin-ui/plugins/saml/components/IdpConfigTab.js(0 hunks)admin-ui/plugins/saml/components/SamlConfigurationForm.js(0 hunks)admin-ui/plugins/saml/components/SamlConfigurationForm.tsx(1 hunks)admin-ui/plugins/saml/components/SamlIdpAddPage.js(0 hunks)admin-ui/plugins/saml/components/SamlIdpEditPage.js(0 hunks)admin-ui/plugins/saml/components/SamlIdpForm.js(0 hunks)admin-ui/plugins/saml/components/SamlPage.js(0 hunks)admin-ui/plugins/saml/components/SamlPage.tsx(1 hunks)admin-ui/plugins/saml/components/TrustRelationshipAddPage.js(0 hunks)admin-ui/plugins/saml/components/TrustRelationshipEditPage.js(0 hunks)admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx(6 hunks)admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx(8 hunks)admin-ui/plugins/saml/components/WebsiteSsoTrustRelationshipForm.tsx(14 hunks)admin-ui/plugins/saml/helper/constants.ts(2 hunks)admin-ui/plugins/saml/helper/index.ts(1 hunks)admin-ui/plugins/saml/helper/tableUtils.tsx(1 hunks)admin-ui/plugins/saml/helper/utils.ts(1 hunks)admin-ui/plugins/saml/helper/validations.ts(1 hunks)admin-ui/plugins/saml/plugin-metadata.js(2 hunks)admin-ui/plugins/saml/redux/api/SamlApi.js(0 hunks)admin-ui/plugins/saml/redux/api/SamlApi.ts(1 hunks)admin-ui/plugins/saml/redux/features/SamlSlice.ts(3 hunks)admin-ui/plugins/saml/redux/sagas/SamlSaga.js(0 hunks)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts(1 hunks)admin-ui/plugins/saml/types/api.ts(1 hunks)admin-ui/plugins/saml/types/formValues.ts(1 hunks)admin-ui/plugins/saml/types/index.ts(1 hunks)admin-ui/plugins/saml/types/redux.ts(1 hunks)admin-ui/plugins/saml/types/state.ts(1 hunks)admin-ui/plugins/scim/components/ScimConfiguration.tsx(1 hunks)admin-ui/plugins/scim/components/ScimPage.tsx(1 hunks)
💤 Files with no reviewable changes (10)
- admin-ui/plugins/saml/components/TrustRelationshipEditPage.js
- admin-ui/plugins/saml/components/IdpConfigTab.js
- admin-ui/plugins/saml/components/SamlConfigurationForm.js
- admin-ui/plugins/saml/components/SamlPage.js
- admin-ui/plugins/saml/components/SamlIdpForm.js
- admin-ui/plugins/saml/redux/api/SamlApi.js
- admin-ui/plugins/saml/components/SamlIdpEditPage.js
- admin-ui/plugins/saml/components/SamlIdpAddPage.js
- admin-ui/plugins/saml/components/TrustRelationshipAddPage.js
- admin-ui/plugins/saml/redux/sagas/SamlSaga.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2404
File: admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx:70-77
Timestamp: 2025-11-03T08:47:33.943Z
Learning: In the admin-ui forms (Gluuformfooter component), the Back button should always navigate to '/home/dashboard' rather than using history-based navigation (navigate(-1)), as clarified by faisalsiddique4400 in PR #2404.
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
📚 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/app/routes/Apps/Gluu/GluuTabs.tsx
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx
🧬 Code graph analysis (13)
admin-ui/plugins/saml/types/formValues.ts (1)
admin-ui/plugins/saml/types/index.ts (2)
SamlConfigurationFormValues(5-5)LocationState(5-5)
admin-ui/plugins/saml/types/state.ts (2)
admin-ui/plugins/saml/types/index.ts (2)
SamlRootState(3-3)SamlReduxState(1-1)admin-ui/plugins/saml/types/redux.ts (1)
SamlReduxState(94-105)
admin-ui/plugins/saml/plugin-metadata.js (1)
admin-ui/app/utils/PermChecker.ts (1)
SAML_WRITE(52-52)
admin-ui/plugins/saml/helper/constants.ts (1)
admin-ui/plugins/saml/helper/index.ts (2)
NameIdPolicyFormatOption(2-2)nameIDPolicyFormat(1-1)
admin-ui/plugins/saml/types/redux.ts (1)
admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (6)
admin-ui/plugins/saml/types/redux.ts (1)
SamlIdentity(30-57)admin-ui/plugins/saml/types/index.ts (3)
SamlIdentity(1-1)LocationState(5-5)SamlRootState(3-3)admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/helper/index.ts (7)
WebsiteSsoIdentityProviderFormValues(9-9)transformToIdentityProviderFormValues(14-14)websiteSsoIdentityProviderValidationSchema(5-5)cleanOptionalFields(16-16)separateConfigFields(17-17)buildIdentityProviderPayload(18-18)nameIDPolicyFormat(1-1)admin-ui/plugins/saml/helper/validations.ts (2)
WebsiteSsoIdentityProviderFormValues(5-22)websiteSsoIdentityProviderValidationSchema(47-118)admin-ui/plugins/saml/helper/constants.ts (1)
nameIDPolicyFormat(6-27)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (4)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/types/redux.ts (1)
TrustRelationship(75-92)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(153-160)admin-ui/plugins/saml/helper/tableUtils.tsx (2)
getServiceProviderTableCols(25-38)PaperContainer(40-42)
admin-ui/plugins/saml/helper/validations.ts (1)
admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)
admin-ui/plugins/saml/components/SamlPage.tsx (2)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx (1)
GluuTabs(78-196)
admin-ui/plugins/saml/helper/utils.ts (3)
admin-ui/plugins/saml/types/redux.ts (3)
SamlConfiguration(3-8)SamlIdentity(30-57)TrustRelationship(75-92)admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)admin-ui/plugins/saml/helper/validations.ts (2)
WebsiteSsoIdentityProviderFormValues(5-22)WebsiteSsoTrustRelationshipFormValues(120-138)
admin-ui/plugins/saml/helper/tableUtils.tsx (1)
admin-ui/plugins/saml/helper/index.ts (4)
TableColumn(25-25)getIdentityProviderTableCols(22-22)getServiceProviderTableCols(23-23)PaperContainer(21-21)
admin-ui/plugins/saml/redux/features/SamlSlice.ts (1)
admin-ui/plugins/saml/types/redux.ts (6)
SamlReduxState(94-105)SamlConfiguration(3-8)GetSamlIdentityProviderPayload(114-119)SamlIdentityListResponse(59-63)PutSamlPropertiesPayload(107-112)TrustRelationship(75-92)
admin-ui/plugins/saml/redux/api/SamlApi.ts (3)
admin-ui/plugins/saml/types/redux.ts (10)
SamlConfiguration(3-8)GetSamlIdentityProviderPayload(114-119)SamlIdentityProviderResponse(65-69)TrustRelationshipListResponse(71-73)TrustRelationship(75-92)PutSamlPropertiesPayload(107-112)CreateSamlIdentityPayload(121-124)UpdateSamlIdentityPayload(126-130)CreateTrustRelationshipPayload(136-139)UpdateTrustRelationshipPayload(141-145)admin-ui/plugins/saml/types/api.ts (3)
SamlIdentityProviderResponse(8-12)SamlApiResponse(31-34)SamlIdentityCreateResponse(14-25)admin-ui/app/utils/ApiUtils.ts (1)
handleTypedResponse(23-40)
🪛 Biome (2.1.2)
admin-ui/plugins/saml/helper/validations.ts
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 43-43: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 62-62: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 66-66: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 94-94: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 144-144: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 158-158: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 162-162: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (45)
admin-ui/plugins/scim/components/ScimPage.tsx (1)
100-101: LGTM! Clean linting fix.The
void omittedstatement correctly marks the intentionally unused variable, which must be destructured to remove the key from the object. This is a standard TypeScript pattern for handling variables that exist only for their side effects.admin-ui/plugins/scim/components/ScimConfiguration.tsx (1)
36-37: Valid approach for handling the intentionally unused variable.Using
void action_messageis a clean TypeScript pattern to satisfy the linter without disabling rules. The variable is correctly excluded from the diff calculation while remaining available for form submission.If you prefer a more conventional approach, consider renaming the destructured variable:
- const { action_message, ...valuesWithoutAction } = formik.values - void action_message + const { action_message: _action_message, ...valuesWithoutAction } = formik.valuesOr using the underscore prefix convention:
- const { action_message, ...valuesWithoutAction } = formik.values - void action_message + const { action_message: _, ...valuesWithoutAction } = formik.valuesadmin-ui/plugins/saml/helper/constants.ts (1)
1-27: LGTM! Type-safe implementation of nameID policy format options.The addition of the
NameIdPolicyFormatOptioninterface and theas constassertion provides strong type safety for the nameID policy format options. This ensures:
- Compile-time validation of option structure
- Readonly array prevents accidental modifications
- Literal type inference for better autocomplete and type checking
The implementation is clean and follows TypeScript best practices.
admin-ui/plugins/saml/types/api.ts (1)
1-71: Well-structured API type definitions with comprehensive error handling.The API interfaces are well-defined with proper TypeScript typing. A few observations:
Callback pattern: The use of callback-based signatures is clear and includes proper error handling with
Error | null.Response duplication: Some methods (e.g.,
getSamlIdentityProvider,getTrustRelationships) include bothdataandresponseparameters in callbacks. This pattern appears intentional for providing both processed data and raw response access.Type safety: All interfaces properly type their inputs and outputs, ensuring compile-time validation.
The implementation follows consistent conventions and provides a solid foundation for type-safe API interactions.
admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx (1)
162-162: Effective fix for navigation throttling issue.The added condition
activeTab.path !== path.pathnameprevents redundant navigation when the active tab already matches the current location. This aligns with:
- Similar checks at lines 99 and 151
- The PR objective to fix "Tabs navigation throttling issue"
- Best practices for avoiding unnecessary navigation cycles
The fix is consistent with the existing code patterns and should resolve navigation performance issues.
admin-ui/plugins/saml/types/state.ts (1)
1-11: LGTM! Well-structured root state interface.The
SamlRootStateinterface clearly defines the shape of the SAML-related Redux state with:
- Proper typing for the
idpSamlReducerusing the importedSamlReduxState- Standard auth reducer structure with nested token access pattern
- Clean separation of concerns between SAML and auth state
This provides type safety for selectors and components accessing the Redux store.
admin-ui/app/locales/pt/translation.json (2)
687-692: Portuguese message translations added for provider operations.The message translations for add_identity_provider, edit_identity_provider, view_identity_provider, delete_identity_provider, edit_service_provider, view_service_provider, and delete_service_provider are properly added and follow the existing message structure conventions.
897-900: Portuguese translations added for SAML service and identity provider operations.The new translation keys for service provider and identity provider operations are well-structured and follow the existing naming conventions. The Portuguese translations are grammatically correct and consistent with the application's terminology.
Key additions include:
- Service Provider: view_sp, create_service_provider, edit_service_provider, view_service_provider
- Identity Provider: identity_provider, edit_identity_provider, create_identity_provider, view_identity_provider, delete_identity_provider
All keys are present and consistent across all locale files (en, es, fr, pt) with appropriate translations for each language.
admin-ui/plugins/saml/types/formValues.ts (1)
1-11: Clean and reusable form value type definitions.Both interfaces are well-designed:
SamlConfigurationFormValues: Provides clear typing for SAML configuration form fields with appropriate boolean and string types.
LocationState: Generic utility type for type-safe navigation state. The optional properties make it flexible for different routing scenarios where row data or view-only mode may not always be needed.
The types follow TypeScript best practices and will improve type safety across SAML forms and navigation.
admin-ui/plugins/saml/types/index.ts (1)
1-5: Well-organized barrel file for SAML type exports.The barrel file provides a clean, centralized export point for SAML-related types:
- Uses type-only exports (
export type) to ensure no runtime impact- Logical grouping: redux types, state types, and form value types
- Simplifies imports for consumers who can now import from a single location
This follows TypeScript best practices for module organization and will improve developer experience.
admin-ui/plugins/saml/helper/tableUtils.tsx (1)
40-44: LGTM!The
PaperContaineris well-implemented withReact.memofor performance optimization and has a properdisplayNameset for debugging purposes.admin-ui/plugins/saml/components/SamlPage.tsx (1)
45-54: LGTM!The render structure with
GluuLoader,Card, andGluuTabsis well-organized. ThewithNavigation={true}prop correctly enables URL-based tab navigation.admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (1)
164-180: LGTM! Button behavior aligns with PR objectives.The form footer correctly implements the button behavior specified in the PR:
- Back: Always enabled (
disableBack={false})- Cancel: Disabled until form is dirty (
disableCancel={!formik.dirty})- Apply: Disabled until form is valid AND dirty (
disableApply={!formik.isValid || !formik.dirty})admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (2)
111-121: LGTM! Good defensive coding with array guards.The
onClickhandlers correctly guard against theMaterialTablepotentially passing an array of row data when selection is enabled, even though selection is currently disabled. This is good defensive coding.Also applies to: 127-133, 148-154
168-212: LGTM!The table rendering and component structure are well-implemented. Good use of
useMemofor table columns and properdisplayNameassignment.admin-ui/app/locales/en/translation.json (1)
1063-1067: LGTM!The new identity provider translation keys are well-structured and follow a consistent naming pattern.
admin-ui/app/locales/es/translation.json (1)
972-975: New SP/IdP title keys look consistentThe added
view_sp,create_service_provider,edit_service_provider,view_service_providerand*_identity_providertitles are consistent with other locales and existing SAML terminology. JSON structure is intact.Also applies to: 1055-1060
admin-ui/app/locales/fr/translation.json (1)
905-910: French SP/IdP titles align with other localesThe added
view_sp,create_service_provider,edit_service_provider,view_service_providerand*_identity_providertitle entries are consistent with the English/Spanish keys and existing message strings. JSON remains valid.Also applies to: 955-960
admin-ui/plugins/saml/helper/index.ts (1)
1-25: Clean barrel export structure.The index file correctly aggregates and re-exports the SAML helper public API with proper separation of value and type exports.
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (4)
45-59: Well-structured component initialization with proper type safety.The component correctly derives
configsandviewOnlyfrom both props and location state with appropriate fallbacks and memoization.
109-156: Correct form submission logic with proper payload construction.The submission handler properly separates metadata file handling, cleans optional fields, and dispatches the appropriate create/update action based on whether
configsexists.
234-256: Comprehensive Apply button disable logic.The
isApplyDisabledmemo correctly handles the various metadata file states including new File uploads, file-like objects with path properties, and existing server-side metadata files.
278-512: Well-structured form with proper conditional rendering.The form correctly toggles between file upload and manual metadata entry modes, with appropriate validation, error display, and disabled states for view-only mode.
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (2)
25-29: Appropriate DeleteItem interface definition.The interface correctly types the deletion item with optional
displayNameandtableDatafields to handle MaterialTable row data.
262-293: Well-organized table configuration with proper memoization.The table columns, header styles, options, and components are appropriately memoized to prevent unnecessary re-renders.
admin-ui/plugins/saml/components/WebsiteSsoTrustRelationshipForm.tsx (5)
36-53: Well-defined local types for component-specific needs.The
TrustRelationshipPayloadOmit type andScopeOptioninterface provide clear type contracts for the form's specific requirements.
166-206: Correct form submission with proper create/update differentiation.The submission handler properly constructs the FormData payload with optional metadata file and dispatches the appropriate action based on whether editing an existing trust relationship.
234-258: Proper viewOnly guards on file handlers.The
handleDropandhandleClearFilescallbacks correctly checkviewOnlybefore modifying form state, preventing accidental modifications in view mode.
545-560: Correct Back button navigation with explicit fallback route.The
onBackhandler correctly passesROUTES.SAML_SP_LISTas the fallback route tonavigateBack, aligning with the PR requirements and retrieved learnings for context-aware navigation.
561-566: Feature flag for SAML trust relationships does not exist.The
GluuCommitDialogfeatureprop is optional and used for webhook triggering, not permission checking. WhileWebsiteSsoIdentityProviderFormincludesfeature={adminUiFeatures.saml_idp_write}, the corresponding feature flag for trust relationships (e.g.,saml_tr_write) is not defined inadminUiFeatures. Adding the prop toWebsiteSsoTrustRelationshipFormwould require first defining a new feature flag in the admin UI features list, webhook configuration, and resource scopes mapping.Likely an incorrect or invalid review comment.
admin-ui/plugins/saml/types/redux.ts (1)
121-130: No action required. The token field inCreateSamlIdentityPayloadandUpdateSamlIdentityPayloadfollows the established pattern used throughout the codebase. Redux sagas retrieve the token from the auth state and intentionally include it in action payloads, which is then used by API methods to set Bearer authorization headers. This is consistent with how other modules (e.g., Assets) handle authentication tokens and is not an anti-pattern in this architecture.Likely an incorrect or invalid review comment.
admin-ui/plugins/saml/redux/api/SamlApi.ts (5)
1-15: LGTM!Imports are well-organized with proper type imports using the
typekeyword for types-only imports, which helps with tree-shaking and build optimization.
17-54: LGTM!Clean interface segregation for different SAML API capabilities. The callback-based signatures properly wrap the underlying JansConfigApi patterns, and the union type enables the runtime capability checks used throughout the class.
56-103: LGTM!The runtime capability checks using
'methodName' in this.apiare appropriate given the union type, and the Promise wrapping withhandleTypedResponseprovides consistent error handling. The fallback pattern at line 96 correctly handles both response shapes.
199-216: Same concern:inumnot used in update operation.Similar to
updateSamlIdentityProvider, theinumfield fromUpdateTrustRelationshipPayloadis not destructured or used. Ensure the backend API contract is being satisfied for identifying the resource to update.
153-228: LGTM!The delete operations correctly pass
inumto identify the resource. ThegetTrustRelationshipmethod properly normalizes the response structure. Post operations for trust relationships follow the same pattern as identity providers.admin-ui/plugins/saml/redux/features/SamlSlice.ts (4)
1-10: LGTM!Good use of
typekeyword for type-only imports and proper typing withPayloadActionfrom Redux Toolkit.
12-28: LGTM!The initial state is properly typed as
SamlReduxStateand all fields are initialized with appropriate default values matching the interface definition.
62-145: LGTM!The reducers demonstrate good practices:
- Nullish coalescing (
??) for safe default values- Inline action types with the
actionwrapper structure for audit purposes (distinct from API payload types)- Proper use of
_actionprefix to indicate unused-but-typed parameters- Consistent pattern for response handlers with null payload fallbacks
149-174: LGTM!All actions are properly exported, and the reducer is registered with the registry using the correct slice name.
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (5)
1-47: LGTM!Comprehensive imports covering saga effects, action creators, API types, and utility functions. Good separation of type imports using the
typekeyword.
61-80: LGTM!Good factory pattern for creating authenticated API instances. The generator return types are properly specified, and the pattern of selecting auth state then creating the wrapped API is clean and reusable.
361-364: LGTM on list refresh after delete.Dispatching
getSamlIdentiteswith emptyinumto refresh the list after deletion is appropriate. The same webhook naming consideration fromdeleteTrustRelationshipapplies here at line 363.
374-391: LGTM!Good centralized error message extraction that handles various API error response shapes with appropriate fallbacks. The type assertion for the extended error shape is reasonable for axios errors.
402-455: LGTM!Standard saga watcher pattern with
takeEveryfor action handling. TherootSagaproperly forks all watchers to run concurrently, and all CRUD operations have corresponding watchers.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
admin-ui/plugins/saml/redux/api/SamlApi.ts (1)
90-106: Verifyinumhandling in update operation.The
updateSamlIdentityProvidermethod doesn't includeinumin the URL path or explicitly in the payload type. If the backend requiresinumto identify the resource being updated, it should either be in the URL (e.g.,/kc/saml/idp/upload/{inum}) or appended to the FormData.Run this script to check how inum is used in update flows:
#!/bin/bash # Check how inum is passed when calling updateSamlIdentityProvider rg -n "updateSamlIdentityProvider|updateSamlIdentity" --type=ts -B 5 -A 10 | head -100admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (3)
24-24: Remove redundantmemowrapping.The component is already wrapped with
memo()at export (line 194), making theReact.FCtyping at line 24 sufficient.Also applies to: 194-194
30-30: MoveSetTitlecall touseEffect.
SetTitleis a side effect and should be called within auseEffecthook to avoid executing during render.
82-106: Avoid usingformikobject directly inuseCallbackdependencies.Including the entire
formikobject causes these callbacks to be recreated on every render. Extract only the specific methods needed.admin-ui/plugins/saml/types/redux.ts (1)
3-8: Consider derivingSamlConfigurationfrom form values type.
SamlConfigurationduplicates the exact fields ofSamlConfigurationFormValues. Using a type alias would prevent drift.admin-ui/plugins/saml/redux/features/SamlSlice.ts (1)
48-48: Typo in action name:getSamlIdentitesshould begetSamlIdentities.The action is misspelled (missing 'i'), while the response handler uses the correct spelling. This creates inconsistency.
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (3)
175-177: ReplacesamlIdentityResponse()withupdateTrustRelationshipResponse().The
postTrustRelationshipsaga dispatches the wrong action in its finally block. It should dispatchupdateTrustRelationshipResponse()to properly resetloadingTrustRelationship, consistent with other trust relationship sagas.
225-225: Consider using semantically appropriate key for delete webhook payload.The webhook payload for a delete operation uses
createdFeatureValue, which is semantically misleading. Consider usingdeletedFeatureValueordeletedResourcefor clarity.Same issue appears at lines 311 and 225 for delete operations.
83-88: Returning error as response type can cause runtime type errors.Multiple catch blocks return the caught error cast as the saga's declared response type (e.g.,
return e as SamlIdentityProviderResponse). This is problematic because consuming code may attempt to access response properties (like.entries) on what is actually an Error object.Consider either:
- Re-throwing the error after handling:
throw e- Not returning from the catch block (let the generator complete)
Apply consistently across all affected catch blocks.
Also applies to: 103-108, 125-130, 143-148, 169-175, 198-204, 227-232, 255-261, 286-292, 313-318
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx(1 hunks)admin-ui/plugins/saml/redux/api/SamlApi.ts(1 hunks)admin-ui/plugins/saml/redux/features/SamlSlice.ts(3 hunks)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts(1 hunks)admin-ui/plugins/saml/redux/sagas/types/saml.ts(1 hunks)admin-ui/plugins/saml/types/api.ts(1 hunks)admin-ui/plugins/saml/types/redux.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2404
File: admin-ui/app/routes/Apps/Gluu/Gluuformfooter.tsx:70-77
Timestamp: 2025-11-03T08:47:33.943Z
Learning: In the admin-ui forms (Gluuformfooter component), the Back button should always navigate to '/home/dashboard' rather than using history-based navigation (navigate(-1)), as clarified by faisalsiddique4400 in PR #2404.
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2418
File: admin-ui/plugins/user-management/components/UserForm.tsx:290-297
Timestamp: 2025-11-06T08:23:20.948Z
Learning: In the UserForm component (admin-ui/plugins/user-management/components/UserForm.tsx), the Back button should fall back to '/user/usersmanagement' when browser history is unavailable, not '/home/dashboard', as this keeps users in the user management context. This aligns with the new requirement that users should be redirected to the respective listing screen of the component rather than the dashboard.
📚 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/redux/sagas/SamlSaga.ts
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts
🧬 Code graph analysis (3)
admin-ui/plugins/saml/types/api.ts (1)
admin-ui/plugins/saml/types/redux.ts (4)
SamlIdentity(30-56)SamlConfiguration(3-8)TrustRelationshipListResponse(58-60)TrustRelationship(62-79)
admin-ui/plugins/saml/types/redux.ts (1)
admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)
admin-ui/plugins/saml/redux/features/SamlSlice.ts (3)
admin-ui/plugins/saml/types/redux.ts (5)
SamlReduxState(81-92)SamlConfiguration(3-8)GetSamlIdentityProviderPayload(101-106)PutSamlPropertiesPayload(94-99)TrustRelationship(62-79)admin-ui/plugins/saml/types/index.ts (3)
SamlReduxState(1-1)SamlConfiguration(1-1)TrustRelationship(1-1)admin-ui/plugins/saml/types/api.ts (1)
SamlIdentityProviderResponse(8-12)
🔇 Additional comments (6)
admin-ui/plugins/saml/types/api.ts (1)
1-72: LGTM!The API client interfaces are well-structured with consistent callback patterns and proper typing for error handling.
admin-ui/plugins/saml/types/redux.ts (1)
81-116: LGTM!The Redux state and payload interfaces are well-structured and comprehensive.
admin-ui/plugins/saml/redux/features/SamlSlice.ts (1)
77-145: LGTM!The CRUD operation reducers are consistently typed with
PayloadActionand follow a clear pattern.admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (2)
170-170: Keepconsole.logfor debugging unexpected errors.Based on learnings, console.log calls in catch blocks help print the whole error object with details for debugging unexpected errors when multiple operations are in the try block.
Also applies to: 199-199, 256-256, 287-287
321-392: LGTM!The error handling functions and saga watchers are well-structured and follow Redux-Saga best practices.
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (1)
143-143: Use translation key for "Choose" text.The text "Choose" is hardcoded and should use a translation key for consistency with the i18n pattern used throughout the component.
Apply this diff:
- <option value="">{t('Choose')}...</option> + <option value="">{t('actions.choose')}...</option>⛔ Skipped due to learnings
Learnt from: faisalsiddique4400 Repo: GluuFederation/flex PR: 2415 File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153 Timestamp: 2025-11-05T19:57:35.143Z Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
| export interface SamlRootState { | ||
| authReducer: SamlAuthState | ||
| } |
There was a problem hiding this comment.
SamlRootState appears incomplete.
This interface only includes authReducer, but the sagas also access state.idpSamlReducer (e.g., in SamlSaga.ts line 29). If this root state is used for saga selectors, it should include the SAML slice reducer.
Consider whether this should be:
export interface SamlRootState {
authReducer: SamlAuthState
+ idpSamlReducer: /* import the correct state type */
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In admin-ui/plugins/saml/redux/sagas/types/saml.ts around lines 9 to 11, the
SamlRootState interface only declares authReducer but sagas also read
state.idpSamlReducer; update SamlRootState to include idpSamlReducer with the
correct type (e.g., IdpSamlState) by importing that type and adding
idpSamlReducer: IdpSamlState to the interface so saga selectors have the correct
shape; also add any other missing SAML slice types used by sagas.
2 participants
feat(admin): adding uniformity in cancel and back buttons present along with TS migration, Tabs navigation throttling issue and type safe implementation in SAML
Summary
This update introduces consistent behavior for Cancel, Back, and Apply buttons across Admin UI forms, including correct enabling and disabling based on form state.
This PR involves the Typescript migration, Tabs navigation throttling issue and type safe implementation
Improvements
Additional Enhancements
Affected Modules
This change is applied across all forms where configuration changes are possible, including but not limited to:
Parent issue: #2268
Testing Steps
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.