feat(admin): adding uniformity in cancel and back buttons present along with TS migration, Tabs navigation throttling issue and type safe implementation in SAML #2499
Conversation
…i-issue-2361-auth-server
…i-issue-2361-auth-server
…i-issue-2361-saml-1
faisalsiddique4400
requested review from
duttarnab and
syntrydy
as code owners
📝 WalkthroughWalkthroughReplace legacy SAML JS UI with TypeScript implementations: add typed Redux slice, sagas, SamlApi wrapper, helpers/validations, new SAML pages/forms/lists, update routes/navigation and translations, and refine several Gluu UI primitives and types. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant List as WebsiteSsoIdentityBrokeringList
participant Router
participant Form as WebsiteSsoIdentityProviderForm
participant Formik
participant Saga as SamlSaga
participant API as SamlApi
participant Store as Redux
User->>List: Click "Add"
List->>Router: navigate to /saml/identity-providers/add
Router->>Form: render (viewOnly=false)
Form->>Formik: initialize form
User->>Form: upload metadata / fill fields
Form->>Formik: submit
Formik->>Form: validation OK
Form->>Form: open commit dialog
User->>Form: confirm
Form->>Saga: dispatch postSamlIdentity(FormData)
Saga->>API: postSamlIdentity(formdata, token)
API-->>Saga: success
Saga->>Store: dispatch create response
Saga->>List: trigger refresh / navigate back
Store->>List: updated identities
List->>User: show updated list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-11-03T08:47:33.943ZApplied to files:
📚 Learning: 2025-11-06T08:23:20.948ZApplied to files:
📚 Learning: 2025-11-05T19:57:35.143ZApplied to files:
📚 Learning: 2025-12-05T18:24:41.713ZApplied to files:
🧬 Code graph analysis (1)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (5)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
comp-admin-ui
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (1)
74-77: Avoid double-fetching SAML identities on pagination/search changesRight now, data is fetched in multiple places:
useEffecton[dispatch, canReadIdentities, limit, pattern](Line 74–77).onRowCountChangeClickandonPageChangeClickdispatchgetSamlIdentitesexplicitly (Lines 110–117, 119–132).handleOptionsChangedispatches on Enter (Line 145–147), while thepatternchange itself also triggers the effect.This can easily lead to redundant API calls (same parameters dispatched twice) whenever limit, pattern, or page change. It also makes the data-loading behavior harder to reason about.
Consider consolidating the fetching logic into a single place, e.g.:
- Keep only the effect and let handlers just update
limit,pattern, andpageNumber, or- Remove the effect and have handlers compute and dispatch the correct
startIndex/limit/patternexplicitly.This will simplify the flow and avoid unnecessary network chatter.
Also applies to: 110-117, 119-132, 138-151
admin-ui/app/routes/Apps/Gluu/GluuTabs.tsx (1)
94-105: Navigation throttling logic is correct and avoids redundant route pushesThe added
tab.path !== path.pathnamechecks inhandleChangeand in theuseEffect(Lines 94–105, 161–163) correctly prevent navigation when the active tab already corresponds to the current route. In theactiveIndex !== -1branch the condition is logically redundant (the path is already equal by construction), but it’s harmless and keeps the pattern consistent with the other branches.Overall, this should reduce unnecessary router activity without changing visible behavior.
Also applies to: 138-166
admin-ui/plugins/saml/redux/features/SamlSlice.ts (1)
12-28: Extract duplicate default configuration to a constant.The default configuration object
{ enabled: false, selectedIdp: '', ignoreValidation: false, applicationName: '' }is repeated in three places:initialState,getSamlConfigurationResponse, andputSamlPropertiesResponse. This violates DRY and risks inconsistencies if updated in one place but not others.+const DEFAULT_CONFIGURATION: SamlConfiguration = { + enabled: false, + selectedIdp: '', + ignoreValidation: false, + applicationName: '', +} + const initialState: SamlReduxState = { - configuration: { - enabled: false, - selectedIdp: '', - ignoreValidation: false, - applicationName: '', - }, + configuration: DEFAULT_CONFIGURATION, loading: false, ...Then use
DEFAULT_CONFIGURATIONin the reducers:getSamlConfigurationResponse: (state, action: PayloadAction<SamlConfiguration | null>) => { - state.configuration = action.payload - ? action.payload - : { - enabled: false, - selectedIdp: '', - ignoreValidation: false, - applicationName: '', - } + state.configuration = action.payload ?? DEFAULT_CONFIGURATIONAlso applies to: 37-46, 68-76
admin-ui/plugins/saml/components/WebsiteSsoTrustRelationshipForm.tsx (1)
348-366:JSON.stringifyin key prop may impact performance.Using
JSON.stringify(formik.values.releasedAttributes)as part of the key recalculates on every render and can be expensive for large arrays. Consider using a more stable key derivation, such as array length or a hash.<GluuTypeAheadForDn - key={`releasedAttributes-${JSON.stringify(formik.values.releasedAttributes)}`} + key={`releasedAttributes-${formik.values.releasedAttributes?.length ?? 0}`}Alternatively, if the array contents matter for re-rendering, consider memoizing the key computation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (39)
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/redux/sagas/types/saml.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/SamlIdpAddPage.js
- admin-ui/plugins/saml/components/TrustRelationshipAddPage.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/components/IdpConfigTab.js
- admin-ui/plugins/saml/components/TrustRelationshipEditPage.js
- admin-ui/plugins/saml/redux/sagas/SamlSaga.js
- admin-ui/plugins/saml/redux/api/SamlApi.js
- admin-ui/plugins/saml/components/SamlIdpEditPage.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-12-05T18:24:41.681Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.681Z
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/WebsiteSsoTrustRelationshipForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.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/app/routes/Apps/Gluu/GluuTabs.tsx
🧬 Code graph analysis (13)
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/app/routes/Apps/Gluu/GluuTabs.tsx (1)
admin-ui/fix-orval-enums.js (1)
path(3-3)
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(81-92)
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/plugin-metadata.js (1)
admin-ui/app/utils/PermChecker.ts (1)
SAML_WRITE(52-52)
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/helper/validations.ts (1)
admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)
admin-ui/plugins/saml/types/redux.ts (1)
admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)
admin-ui/plugins/saml/components/WebsiteSsoTrustRelationshipForm.tsx (5)
admin-ui/plugins/saml/helper/validations.ts (2)
WebsiteSsoTrustRelationshipFormValues(120-138)websiteSsoTrustRelationshipValidationSchema(148-187)admin-ui/plugins/saml/types/redux.ts (1)
TrustRelationship(62-79)admin-ui/plugins/saml/types/formValues.ts (1)
LocationState(8-11)admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/helper/utils.ts (1)
transformToTrustRelationshipFormValues(65-93)
admin-ui/plugins/saml/helper/utils.ts (3)
admin-ui/plugins/saml/types/redux.ts (3)
SamlConfiguration(3-8)SamlIdentity(30-56)TrustRelationship(62-79)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/redux/api/SamlApi.ts (3)
admin-ui/plugins/saml/types/api.ts (6)
JansConfigApiClient(32-38)JansIdentityBrokerApiClient(40-58)JansTrustRelationshipApiClient(60-72)SamlIdentityProviderResponse(8-12)SamlIdentityCreateResponse(14-25)SamlApiResponse(27-30)admin-ui/plugins/saml/types/redux.ts (5)
SamlConfiguration(3-8)GetSamlIdentityProviderPayload(101-106)CreateSamlIdentityPayload(108-111)TrustRelationshipListResponse(58-60)CreateTrustRelationshipPayload(113-116)admin-ui/app/utils/ApiUtils.ts (1)
handleResponse(5-20)
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (4)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/types/redux.ts (1)
SamlIdentity(30-56)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(153-160)admin-ui/plugins/saml/helper/tableUtils.tsx (2)
getIdentityProviderTableCols(10-23)PaperContainer(40-42)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (6)
admin-ui/plugins/saml/types/redux.ts (1)
SamlIdentity(30-56)admin-ui/plugins/saml/types/formValues.ts (1)
LocationState(8-11)admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/helper/validations.ts (2)
WebsiteSsoIdentityProviderFormValues(5-22)websiteSsoIdentityProviderValidationSchema(47-118)admin-ui/plugins/saml/helper/utils.ts (4)
transformToIdentityProviderFormValues(19-52)cleanOptionalFields(123-128)separateConfigFields(130-146)buildIdentityProviderPayload(148-191)admin-ui/plugins/saml/helper/constants.ts (1)
nameIDPolicyFormat(6-27)
🪛 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 (35)
admin-ui/plugins/scim/components/ScimPage.tsx (1)
95-103:removecase change is behaviorally safeThe destructuring with
omittedandvoid omittedcorrectly drops the keyed property while keeping lint happy; logic and optimistic update behavior remain unchanged.admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (1)
187-260: MaterialTable configuration and memoization look solidThe
tableActions,tableColumns,headerStyle,tableOptions, andtableComponentsare all properly memoized and typed (Action<SamlIdentity>[]), and the row handlers defensively handle the array vs single-row case and striptableDatabefore navigation. This should reduce needless re-renders and keep the listing behavior predictable.Also applies to: 262-292, 298-305
admin-ui/app/locales/pt/translation.json (1)
895-897: Service/identity provider title keys and PT translations look consistentRenaming the service provider keys and adding
view_service_providerplus the identity provider CRUD titles aligns with the new SAML UI usage (titles.*_service_provider/titles.*_identity_provider). The Portuguese strings themselves are coherent and consistent with existing terminology.Also applies to: 947-951
admin-ui/app/locales/es/translation.json (1)
970-972: Updated service/identity provider title keys and ES translations are appropriateThe new
*_service_providerand*_identity_providertitle keys align with the SAML components’ translation lookups, and the Spanish texts are accurate and stylistically consistent with the rest of the file (including the updatedfollowingPermissionRequiredToBeAddedmessage).Also applies to: 1052-1057
admin-ui/app/locales/fr/translation.json (1)
905-907: Service/Identity Provider title keys align with updated SAML terminologyThe new
*_service_providerand*_identity_providertitle keys and translations look consistent with the English source and the rest of the SAML UI.Also applies to: 953-957
admin-ui/plugins/saml/types/formValues.ts (1)
1-11: Form value and location state typings are consistent with redux types
SamlConfigurationFormValuesmirrorsSamlConfiguration, andLocationState<T>cleanly captures the rowData/viewOnly pattern for navigation state; no issues spotted.admin-ui/plugins/saml/components/SamlPage.tsx (1)
14-53: SAML tab container, title handling, and loader wiring look correct
SetTitle(t('titles.saml_management'))is called at the component top level as intended, avoiding extra hook wrappers (based on learnings).tabNamesandtabToShowuse the same translated menu labels as inplugin-metadata.js, so navigation and content switching should stay in sync.GluuLoaderis correctly bound toloadingSamlIdp, so the whole page blocks while SAML IDP data is loading.admin-ui/plugins/saml/types/api.ts (1)
1-72: API and client typings align with redux shapesThe
SamlIdentityProviderResponse,SamlIdentityCreateResponse, andSamlApiResponseinterfaces, plus the threeJans*ApiClientinterfaces, are consistent with the redux types in./reduxand with typical(error, data, response)callback signatures; nothing obviously problematic here.It would be good to double‑check these signatures against the actual generated client or OpenAPI spec to ensure option and response shapes (especially
TrustRelationshipListResponsevsresponse.body) match the backend.admin-ui/plugins/saml/types/index.ts (1)
1-5: Type barrel cleanly centralizes SAML typingsRe‑exporting redux, root state, and form value types from a single index simplifies imports for consumers and keeps the SAML type surface organized.
admin-ui/plugins/saml/plugin-metadata.js (1)
2-2: Routing metadata correctly targets new SAML forms with appropriate permissions
WebsiteSsoIdentityProviderFormandWebsiteSsoTrustRelationshipFormare now wired for the/identity-providers/*and/service-providers/*add/edit routes, matching the new form-based UX.- List and config routes still use
SamlPageand are gated by the appropriate read/config permissions, while add/edit routes requireSAML_WRITE, which is consistent with write operations.Also applies to: 6-6, 47-69
admin-ui/plugins/saml/helper/constants.ts (1)
1-4: Fix incompatible combination ofas constwith non‑readonly array typeAssigning an
as constassertion (which creates a readonly type) to a variable typed asNameIdPolicyFormatOption[](mutable array) violates TypeScript's type compatibility rules and causes a compilation error.Resolve this by either removing
as constor changing the type annotation toreadonly NameIdPolicyFormatOption[].admin-ui/plugins/saml/helper/index.ts (1)
1-25: LGTM!Clean barrel file with well-organized exports. Using
export typefor type-only exports is the correct approach for TypeScript.admin-ui/plugins/saml/helper/tableUtils.tsx (1)
40-44: LGTM!
PaperContaineris correctly memoized and has a properdisplayNameset for React DevTools debugging.admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (1)
31-210: LGTM on overall component structure.Good use of memoization with
React.memo, properdisplayNameassignment, and correctly typed Redux selectors and actions. The component follows React best practices for performance optimization.admin-ui/app/routes/Apps/Gluu/GluuUploadFile.tsx (3)
8-96: LGTM on TypeScript migration.Good conversion to typed React.FC with proper interface definition. The component correctly handles file state, theme context, and provides clear button functionality.
31-32: No issues found. The default theme value'darkBlack'is a valid and intentional theme choice throughout the application. It is properly defined in the theme configuration and consistently used as a fallback across all theme-aware components.
83-88: No action required. This button color format is correct and widely established throughout the codebase. The custom CSS inadmin-ui/app/styles/custom/layout.scssdefines the button color classes (.btn-primary-darkBlack,.btn-primary-darkBlue,.btn-primary-lightBlue,.btn-primary-lightGreen), and this pattern is used consistently across 50+ component instances with Badges, Buttons, and other elements usingcolor={primary-${selectedTheme}}.Likely an incorrect or invalid review comment.
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (2)
165-181: LGTM on footer button behavior.The
GluuFormFooterconfiguration correctly implements the PR objectives:
- Back is 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})
30-30: Consider movingSetTitlecall to auseEffect.
SetTitleis called directly in the component body. IfSetTitlehas side effects, it should be wrapped in auseEffectto follow React best practices:+ useEffect(() => { + SetTitle(t('titles.saml_management')) + }, [t]) - SetTitle(t('titles.saml_management'))⛔ Skipped due to learnings
Learnt from: faisalsiddique4400 Repo: GluuFederation/flex PR: 2498 File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30 Timestamp: 2025-12-05T18:24:41.681Z 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.admin-ui/plugins/saml/helper/validations.ts (4)
24-37: Ignore Biome'snoThenPropertywarnings.The static analysis warnings about
thenproperties are false positives. Yup's.when()API usesthenandotherwiseas standard method names for conditional validation, not as Promise-like thenables.
60-86: Complex nested.when()validation is correctly structured.The metadata file validation logic handles multiple scenarios:
- When metadata import is enabled and no existing file, require a new file
- When existing file is present (
idpMetaDataFN), allow null- When metadata import is disabled, allow null
This correctly handles both create and edit scenarios.
99-113: Good use of custom URL validation.The custom
url-formattest forsingleLogoutServiceUrlcorrectly handles optional fields by returning true for empty strings, while validating actual URLs using the nativeURLconstructor.
140-146: Remove the$prefix from$spMetaDataSourceType— it should reference the form field directly.The
$prefix in Yup is used for context variables, butspMetaDataSourceTypeis a regular form field (line 125). It should be referenced as'spMetaDataSourceType'without the$, consistent with the pattern used at line 156:when('spMetaDataSourceType', ...).Likely an incorrect or invalid review comment.
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (3)
515-527: LGTM on footer button configuration.The
GluuFormFootercorrectly implements PR objectives:
- Back always enabled with custom
onBack={navigateBack}handler- Cancel/Apply hidden in viewOnly mode
- Cancel disabled until dirty
- Apply uses custom
isApplyDisabledlogic for metadata validationBased on learnings, the
navigateBackhandler should navigate toROUTES.SAML_IDP_LISTwhich aligns with the pattern of redirecting to the respective listing screen.
234-256: Complex but necessaryisApplyDisabledlogic.The validation for metadata file presence handles several edge cases correctly:
- Checks for File instances
- Accepts file-like objects with path/relativePath properties
- Falls back to existing
idpMetaDataFNfor edit scenariosThis aligns well with the validation schema logic.
78-78: Consider movingSetTitlecall to auseEffect.Same as in
SamlConfigurationForm.tsx,SetTitleshould ideally be in auseEffecthook:+ useEffect(() => { + SetTitle(title) + }, [title]) - SetTitle(title)⛔ Skipped due to learnings
Learnt from: faisalsiddique4400 Repo: GluuFederation/flex PR: 2498 File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30 Timestamp: 2025-12-05T18:24:41.681Z 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.admin-ui/plugins/saml/redux/sagas/types/saml.ts (1)
1-67: Type definitions are well-structured.The saga payload interfaces are cleanly organized with appropriate typing for FormData and string payloads. The separation between create/update/delete operations with their respective data shapes is clear.
admin-ui/plugins/saml/helper/utils.ts (1)
148-191: Well-structured payload builder.The
buildIdentityProviderPayloadfunction properly handles both metadata file imports and manual configuration scenarios with appropriate field initialization and trimming.admin-ui/plugins/saml/redux/api/SamlApi.ts (1)
25-30: Class design handles API polymorphism well.The runtime method checks with
'methodName' in this.apipattern effectively handles the union type, providing clear error messages when methods aren't available on the specific API instance.admin-ui/plugins/saml/redux/features/SamlSlice.ts (1)
2-10: TypeScript migration with proper PayloadAction typings looks good.The explicit typing of Redux actions with
PayloadAction<T>provides excellent type safety for the saga and component consumers.admin-ui/plugins/saml/components/WebsiteSsoTrustRelationshipForm.tsx (3)
545-560: Form footer correctly implements PR requirements.The
GluuFormFooterconfiguration properly implements the PR objectives:
- Cancel disabled when form is not dirty (
!formik.dirty)- Apply disabled when form is not valid or not dirty
- Back always enabled and navigates to the service provider list
Based on learnings, the Back button correctly navigates to the component's listing screen (
ROUTES.SAML_SP_LIST) rather than the dashboard.
234-258: Good handling of file drop/clear touch states.The asymmetric touch state handling is intentional and correct:
handleDropsets touched tofalseto prevent immediate error display after successful drophandleClearFilessets touched totrueto trigger validation when user clears the fileThis provides a better UX for file upload validation.
57-71: Props fallback to location state is well-implemented.The memoized resolution of
configsandviewOnlyfrom either props or location state provides flexibility for both direct prop passing and route-based navigation patterns.admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (2)
75-392: Overall saga structure follows Redux best practices.The saga implementation demonstrates several good patterns:
- Consistent audit trail initialization and logging
- Proper error handling with try-catch blocks
- Token refresh on 401 errors
- Webhook integration for state changes
- User feedback via toast notifications
- Clean separation between business logic and side effects
223-223: Add missing function invocation parentheses.The saga
getTrustRelationshipsSagais passed without parentheses, which means the generator function itself is passed instead of being invoked. This will cause incorrect behavior.Apply this diff to fix the invocation:
- yield call(getTrustRelationshipsSaga) + yield call(getTrustRelationshipsSaga)Actually, since
getTrustRelationshipsSagatakes no parameters, use either:- yield call(getTrustRelationshipsSaga) + yield* getTrustRelationshipsSaga()or keep
callwith parentheses:- yield call(getTrustRelationshipsSaga) + yield call(getTrustRelationshipsSaga)The current code references the function without calling it.
Likely an incorrect or invalid review comment.
…i-issue-2361-saml-1
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx (1)
270-281: Consider removing redundant PropTypes after TypeScript migration.The component now has full TypeScript type coverage via
GluuCommitDialogProps. The PropTypes validation is redundant and adds maintenance overhead. Standard practice when migrating to TypeScript is to remove PropTypes.-export default GluuCommitDialog -GluuCommitDialog.propTypes = { - feature: PropTypes.string, - operations: PropTypes.any, - handler: PropTypes.func, - modal: PropTypes.bool.isRequired, - onAccept: PropTypes.func, - placeholderLabel: PropTypes.string, - inputType: PropTypes.string, - label: PropTypes.string, - formik: PropTypes.object, - isLicenseLabel: PropTypes.bool, -} +export default GluuCommitDialog
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
admin-ui/app/locales/en/translation.json(2 hunks)admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx(4 hunks)admin-ui/app/routes/Apps/Gluu/GluuUploadFile.tsx(3 hunks)admin-ui/app/routes/Apps/Gluu/types.ts(1 hunks)admin-ui/plugins/saml/components/SamlConfigurationForm.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx(8 hunks)admin-ui/plugins/saml/helper/utils.ts(1 hunks)admin-ui/plugins/saml/redux/features/SamlSlice.ts(1 hunks)admin-ui/plugins/saml/redux/sagas/types/saml.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
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.
🧬 Code graph analysis (4)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (3)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/types/redux.ts (1)
TrustRelationship(62-79)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(154-161)
admin-ui/plugins/saml/components/SamlConfigurationForm.tsx (5)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/types/index.ts (2)
SamlRootState(3-3)SamlConfigurationFormValues(5-5)admin-ui/plugins/saml/helper/utils.ts (1)
transformToFormValues(8-17)admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)admin-ui/plugins/saml/helper/validations.ts (1)
samlConfigurationValidationSchema(24-37)
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)
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx (1)
admin-ui/app/routes/Apps/Gluu/types.ts (1)
GluuCommitDialogOperation(1-4)
🔇 Additional comments (19)
admin-ui/app/routes/Apps/Gluu/GluuUploadFile.tsx (5)
1-20: LGTM: Well-structured TypeScript migration.The interface definition, type imports, and constants are properly implemented. The
GluuUploadFilePropsinterface provides clear type safety for all component props.
41-49: Past review feedback addressed.The
handleDropimplementation now correctly passes only the first file ([file]) to the parent'sonDropcallback, which aligns with the single-file upload design and addresses the previous review comment.
61-69: LGTM: Proper event handling and state cleanup.The
clearFilesfunction is well-implemented with proper TypeScript typing, event propagation prevention, and complete state cleanup. The dependency array is correct.
78-78: LGTM: Safe file name display logic.The optional chaining for
selectedFile?.namewith a fallback topreDefinedFileNameis properly implemented and type-safe.
31-32: No issues found. The theme fallback value'darkBlack'is a valid and well-established theme in the theming system. It is properly defined inadmin-ui/app/context/theme/config.tswith complete theme configuration, listed as an available option in ThemeSettings, and used consistently as the default theme throughout the application.admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (4)
25-31: LGTM on interface and icon placement.The
DeleteIteminterface is appropriately typed withRecord<string, unknown>for the optionaltableDatafield. TheDeleteOutlinedIconcomponent is correctly placed at module scope, addressing the previous review feedback.
38-45: LGTM on state and memoization.The
bgThemeColormemo andtogglecallback are properly memoized with correct dependencies. The state typing withDeleteItemis appropriate.
64-73: LGTM on effects.The authorization and data fetching effects have proper guards and dependency arrays.
175-219: LGTM on render and exports.The table columns are properly memoized, the
idSynonymcorrectly identifies rows byinum, and the conditional dialog rendering based on permissions is appropriate. ThedisplayNameassignment and default export follow React best practices for memoized components.admin-ui/app/locales/en/translation.json (1)
977-979: Translation keys and message updates look good.The renamed service provider keys (
create_service_provider,edit_service_provider,view_service_provider) and new identity provider keys are consistent with the SAML feature restructuring. The awkward permission message has been improved to clearer English.Also applies to: 1059-1064
admin-ui/plugins/saml/helper/utils.ts (3)
47-47: Simplified boolean conversion - good.Using
!!configs?.idpMetaDataFNis cleaner than the ternary. This addresses the previous review feedback.
148-191: Well-implemented object cleaning utility.The
cleanOptionalFieldsfunction properly handles:
- Null/undefined removal
- Optional empty string filtering
- Recursive nested object cleaning
- File object preservation
- Non-empty array retention
The generic typing with
Partial<T>return type is appropriate.
226-229: Verify intended behavior for field merging.When
metaDataFileImportedFlagis false,finalConfigDatais spread afterrootFields, which means config fields would override any same-named root fields. Ensure this is the intended API contract.If
rootFieldsandfinalConfigDatacould have overlapping keys, consider being explicit about precedence or restructuring as nested objects:const payload = { ...rootFields, config: finalConfigData, // nested instead of merged } as IdentityProviderPayloadadmin-ui/plugins/saml/components/SamlConfigurationForm.tsx (1)
144-147: Hardcoded IdP option may need future expansion.The
selectedIdpdropdown has only "keycloak" as an option. If additional identity providers are planned, consider making this dynamic.Is this intentionally limited to Keycloak, or should this be populated from available SAML identity providers?
admin-ui/plugins/saml/redux/features/SamlSlice.ts (1)
12-28: Well-structured initial state matching the interface.The initial state properly implements
SamlReduxStatewith sensible defaults. The structure supports the various loading states needed for different SAML operations.admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx (2)
26-26: LGTM! Type import enhances type safety.The addition of
GluuCommitDialogOperationto the imports is appropriate and necessary for the explicit type annotations used in the map callback at line 184.
184-184: LGTM! Explicit typing improves code clarity.The explicit type annotations for the map callback parameters enhance type safety and align with the TypeScript migration objectives.
admin-ui/app/routes/Apps/Gluu/types.ts (2)
20-20: LGTM! NarrowedinputTypeimproves type safety.The refinement of
inputTypefrom a genericstringto a specific union of valid input types is a solid improvement that prevents invalid values and enhances IDE autocomplete support.
10-16: Customformikshape restricts value parameter to strings, but actual Formik accepts any type.The custom
formikshape definessetFieldValuewithvalue: string, but the actual Formik library acceptsvalue: any. Evidence: UserForm.tsx callsformik.setFieldValue()with arrays ([]) and booleans (false), yet castsformik as anywhen passing to GluuCommitDialog to work around the type mismatch.While GluuCommitDialog itself only calls
setFieldValue('action_message', userMessage)with strings, the type definition should match Formik's actual signature for consistency. Consider updating the type to accept any value:formik?: { setFieldValue: ( field: string, value: any, shouldValidate?: boolean, ) => void | Promise<unknown> } | nullAlternatively, use Formik's actual type:
Pick<FormikProps<any>, 'setFieldValue'>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (3)
203-203: Remove debug console.log statement.Debug
console.logstatements should not be present in production code as they can expose sensitive error details in the browser console and clutter logs.Apply this diff to remove the debug statement:
} catch (e) { - console.log('Error: ', e) yield put(toggleSavedFormFlag(false))Also applies to lines 236, 292, and 325.
140-162: Add missingtoggleSavedFormFlagdispatch to maintain consistency.The
putSamlPropertiessaga is missing thetoggleSavedFormFlagdispatch that all other mutation sagas include (lines 197, 230, 287, 319). This creates inconsistent form state management across SAML operations.Apply this diff to add the missing flag dispatches:
}) + yield put(toggleSavedFormFlag(true)) yield* triggerWebhook({ payload: { createdFeatureValue: data } }) yield put(putSamlPropertiesResponse(data)) yield call(postUserAction, audit) yield put(updateToast(true, 'success', 'Data updated successfully')) return data } catch (e) { + yield put(toggleSavedFormFlag(false)) yield put(putSamlPropertiesResponse(null))
385-421: Replace hardcoded action strings with typed action creators.All watcher functions use hardcoded string literals for action types (e.g.,
'idpSaml/getSamlConfiguration'). This is fragile and bypasses TypeScript's type safety. If action names are renamed in the slice, these watchers will silently fail at runtime.Import the action creators from the slice and use their
.typeproperty:+import { + getSamlConfiguration, + getSamlIdentities, + getTrustRelationship, + putSamlProperties, + createSamlIdentity, + createTrustRelationship, + deleteSamlIdentity, + updateSamlIdentity, + deleteTrustRelationship, + updateTrustRelationship, +} from '../features/SamlSlice' export function* watchGetSamlConfig(): Generator { - yield takeEvery('idpSaml/getSamlConfiguration', getSamlConfigSaga) + yield takeEvery(getSamlConfiguration.type, getSamlConfigSaga) } export function* watchGetSamlIdentityProvider(): Generator { - yield takeEvery('idpSaml/getSamlIdentities', getSamlIdentityProvider) + yield takeEvery(getSamlIdentities.type, getSamlIdentityProvider) }Apply similar changes to all other watchers (lines 392-421).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx(3 hunks)admin-ui/app/routes/Apps/Gluu/types.ts(1 hunks)admin-ui/plugins/saml/components/SamlConfigurationForm.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx(8 hunks)admin-ui/plugins/saml/helper/utils.ts(1 hunks)admin-ui/plugins/saml/redux/features/SamlSlice.ts(1 hunks)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts(1 hunks)admin-ui/plugins/saml/redux/sagas/types/saml.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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.
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.
📚 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/SamlConfigurationForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/saml/components/SamlConfigurationForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsxadmin-ui/plugins/saml/redux/sagas/SamlSaga.ts
📚 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/SamlConfigurationForm.tsxadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx
📚 Learning: 2025-12-05T18:24:41.681Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.681Z
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/WebsiteSsoServiceProviderList.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/redux/sagas/SamlSaga.ts
🧬 Code graph analysis (4)
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx (1)
admin-ui/app/routes/Apps/Gluu/types.ts (2)
JsonValue(1-7)GluuCommitDialogOperation(9-12)
admin-ui/plugins/saml/helper/utils.ts (3)
admin-ui/plugins/saml/types/redux.ts (3)
SamlConfiguration(3-8)SamlIdentity(30-56)TrustRelationship(62-79)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/redux/sagas/SamlSaga.ts (7)
admin-ui/app/utils/TokenController.ts (3)
AdditionalPayload(10-21)addAdditionalData(49-124)isFourZeroOneError(23-25)admin-ui/plugins/saml/redux/api/SamlApi.ts (1)
SamlApi(25-179)admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/app/redux/api/base.ts (1)
getClient(23-44)admin-ui/plugins/saml/types/redux.ts (3)
SamlConfiguration(3-8)GetSamlIdentityProviderPayload(101-106)TrustRelationshipListResponse(58-60)admin-ui/app/redux/api/backend-api.ts (1)
postUserAction(51-69)admin-ui/plugins/saml/types/api.ts (3)
SamlIdentityProviderResponse(8-12)SamlApiResponse(27-30)SamlIdentityCreateResponse(14-25)
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 (13)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (4)
25-29: LGTM on the updatedDeleteIteminterface.The change from
Record<string, never>toRecord<string, unknown>fortableDataproperly addresses the previous review comment and provides a more accurate representation of MaterialTable's internal data structure.
94-112: Improved type safety withUserActionimport.Good adoption of the
UserActiontype fromPermCheckeras suggested in past reviews. The deletion payload now includes bothaction_messageandaction_data, which preserves audit details properly.
121-129: Clean destructuring pattern for strippingtableData.Good implementation using destructuring with
void tableDatato suppress the unused variable warning. This is a cleaner approach than the previous manual delete operation.
175-176: Well-structured component with proper memoization and display name.The component follows React best practices with
React.memo, memoized table columns, and an explicitdisplayNamefor better debugging experience in React DevTools.Also applies to: 215-219
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx (3)
30-32: Good use of type guard for array narrowing.The
isJsonValueArraytype guard provides proper type narrowing when checking ifitem.valueis an array, improving type safety over a plainArray.isArraycheck.
204-217: Comprehensive value type handling with proper type narrowing.The rendering logic correctly handles all
JsonValuetypes in order: arrays (via type guard), booleans, empty/null/undefined values, objects, and finally falls back to string conversion. This aligns well with the new typed interface.
225-227: CorrectinputTypehandling without type assertion.The incorrect
as 'textarea'assertion flagged in previous reviews has been removed. TheinputTypeprop now correctly accepts the union type fromGluuCommitDialogProps.admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (3)
25-29: Proper handling of optionalinumin deletion flow.Good fix from previous review:
DeleteItem.inumis now correctly typed as optional (inum?: string), andonDeletionConfirmedproperly guards against missinginumat line 102-104 before proceeding with the deletion. The fulluserActionobject (bothaction_messageandaction_data) is now forwarded todeleteSamlIdentity.Also applies to: 100-121
168-198: Properly memoized component factories.Both
PaginationWrapperandGluuSearchare correctly memoized withuseCallbackand have appropriate dependency arrays. This ensures stable references for MaterialTable'scomponentsandactionsprops.
275-305: Well-organized memoization of table configuration.The separation of
tableColumns,headerStyle,tableOptions, andtableComponentsinto individualuseMemoblocks with appropriate dependencies is a good pattern for optimizing MaterialTable renders.admin-ui/app/routes/Apps/Gluu/types.ts (3)
1-12: Well-defined recursiveJsonValuetype.The
JsonValuetype properly represents all valid JSON values including nested objects and arrays through recursive type references. TheGluuCommitDialogOperationinterface now uses this type forvalue, providing better type safety than the previousunknown[]approach. This addresses the concern raised in past reviews.
18-24: Flexibleformikprop typing.Good decision to narrow the
formikprop to only thesetFieldValuemethod that's actually used by the component, rather than requiring the fullFormikPropsgeneric. This improves reusability across different form contexts.
28-28: ExplicitinputTypeunion provides better type safety.The explicit union type for
inputTypeclearly documents the supported input types and provides better autocomplete support compared to the previousInputProps['type']approach.
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/helpers/navigation.ts (1)
86-95: Silent no-op whenfallbackRouteis not provided and history is empty.When
window.history.length <= 1and nofallbackRouteis supplied,navigateBacksilently does nothing. This could confuse callers expecting navigation to occur. Based on learnings from PR #2404, the Back button should navigate to/home/dashboardas a default fallback rather than doing nothing.Consider defaulting to
ROUTES.HOME_DASHBOARDwhen no fallback is provided:const navigateBack = useCallback( - (fallbackRoute?: string): void => { + (fallbackRoute: string = ROUTES.HOME_DASHBOARD): void => { if (window.history.length > 1) { navigate(-1) - } else if (fallbackRoute) { + } else { navigateToRoute(fallbackRoute) } }, [navigate, navigateToRoute], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/app/helpers/navigation.ts(1 hunks)admin-ui/plugins/saml/components/SamlPage.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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: 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.
📚 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/helpers/navigation.ts
📚 Learning: 2025-12-05T18:24:41.681Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.681Z
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.tsx
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx
🔇 Additional comments (5)
admin-ui/app/helpers/navigation.ts (1)
52-52: LGTM!The
SAML_CONFIGroute constant is well-placed and follows the existing naming conventions for SAML routes.admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (3)
1-24: Well-structured imports and component setup.The imports are logically organized, and the component correctly uses memoization, hooks, and TypeScript types. The Cedar permission system integration and Redux state management follow the established patterns.
304-330: Clean component composition and conditional rendering.The return statement properly wraps the table in
GluuViewWrapperfor permission gating and conditionally renders the delete dialog. ThedisplayNameassignment aids debugging.
102-123: Review comment is incorrect. Theinumis properly passed todeleteSamlIdentitythroughaction_data. The flow is:buildPayload(userAction, message, item.inum)setsuserAction.action_data = item.inum, which is then dispatched in the action and received by the saga aspayload.action.action_data. The saga passes this directly toapi.deleteSamlIdentityProvider(payload.action.action_data), which expects theinumstring as its first parameter.Likely an incorrect or invalid review comment.
admin-ui/plugins/saml/components/SamlPage.tsx (1)
17-19:SetTitleusage is correct.Based on learnings,
SetTitleis a custom React hook that should be called at the component's top level, which is correctly done here.
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
admin-ui/plugins/saml/components/SamlPage.tsx (1)
32-46: Switch on translated strings is fragile (previously flagged).This issue was already identified in a previous review: using translated strings (
t('menus.configuration'), etc.) as switch case values couples rendering logic to translations. If translations change or differ across locales, the switch may fail to match.The previous suggestion was to use constant keys (e.g.,
TAB_KEYS.CONFIGURATION) instead of relying on translated strings for control flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
admin-ui/app/routes/Apps/Gluu/GluuAdvancedSearch.tsx(3 hunks)admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx(3 hunks)admin-ui/app/routes/Apps/Gluu/types/GluuAdvancedSearch.ts(1 hunks)admin-ui/app/routes/Apps/Gluu/types/GluuCommitDialog.ts(1 hunks)admin-ui/app/routes/Apps/Gluu/types/common.ts(1 hunks)admin-ui/app/routes/Apps/Gluu/types/index.ts(1 hunks)admin-ui/plugins/saml/components/SamlPage.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx(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: 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.
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx
📚 Learning: 2025-12-05T18:24:41.681Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2498
File: admin-ui/plugins/saml/components/SamlConfigurationForm.tsx:30-30
Timestamp: 2025-12-05T18:24:41.681Z
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.tsx
🧬 Code graph analysis (4)
admin-ui/app/routes/Apps/Gluu/types/common.ts (1)
admin-ui/app/routes/Apps/Gluu/types/index.ts (1)
JsonValue(1-1)
admin-ui/app/routes/Apps/Gluu/types/GluuAdvancedSearch.ts (1)
admin-ui/app/routes/Apps/Gluu/types/index.ts (1)
GluuAdvancedSearchProps(3-3)
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx (2)
admin-ui/app/routes/Apps/Gluu/types/common.ts (1)
JsonValue(1-7)admin-ui/app/routes/Apps/Gluu/types/GluuCommitDialog.ts (1)
GluuCommitDialogOperation(3-6)
admin-ui/app/routes/Apps/Gluu/GluuAdvancedSearch.tsx (2)
admin-ui/app/routes/Apps/Gluu/types/GluuAdvancedSearch.ts (1)
GluuAdvancedSearchProps(3-15)admin-ui/app/routes/Apps/Gluu/types/index.ts (1)
GluuAdvancedSearchProps(3-3)
🔇 Additional comments (17)
admin-ui/app/routes/Apps/Gluu/types/common.ts (1)
1-7: LGTM!Well-defined recursive type for JSON-compatible values. This properly covers all JSON primitives and composite types, and is a standard pattern for typed JSON handling.
admin-ui/app/routes/Apps/Gluu/types/GluuAdvancedSearch.ts (1)
1-15: LGTM!Well-structured props interface with appropriate use of type-only imports and correct React event types for input handlers.
admin-ui/app/routes/Apps/Gluu/types/index.ts (1)
1-3: LGTM!Clean barrel file with type-only re-exports, following best practices for centralized type management.
admin-ui/app/routes/Apps/Gluu/GluuAdvancedSearch.tsx (1)
40-50: LGTM!The controlled input pattern using
patternPropsspread is clean and correctly handles both controlled (value) and uncontrolled (defaultValue) modes.admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx (2)
30-32: LGTM!Clean type guard that properly narrows
JsonValuetoJsonValue[]. This is preferable to rawArray.isArrayin typed contexts.
185-220: LGTM!Good use of explicit typing in the map callback and proper use of the
isJsonValueArraytype guard. The rendering logic correctly handles all JsonValue variants.admin-ui/app/routes/Apps/Gluu/types/GluuCommitDialog.ts (1)
1-25: LGTM!Well-structured type definitions. Using
JsonValueforGluuCommitDialogOperation.valueaddresses the previous review concern about type safety withunknown[]. TheinputTypeunion provides good type safety for supported input types.admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (5)
26-32: LGTM!Previous review concerns addressed:
DeleteItem.inumis now optional to handle cases whereSamlIdentity.inummight be undefined, andDeleteOutlinedIconis correctly defined at module level.
118-139: LGTM!Good defensive check for
item.inumbefore proceeding with deletion. Bothaction_messageandaction_dataare now forwarded todeleteSamlIdentity, addressing the previous review concern about discarded audit details.
169-192: LGTM!Event handlers are now properly separated:
handleOptionsChangehandlesChangeEventandhandleOptionsKeyDownhandlesKeyboardEvent. This addresses the previous review concern about incorrect event typing.
150-163: LGTM!The redundant
parseInt(String(startCount), 10)conversion has been removed.startCountis now passed directly as a number, which is cleaner.
332-358: LGTM!Component rendering is clean with proper permission checks for view wrapper and delete dialog. The MaterialTable integration is well-structured with memoized columns, options, and components.
admin-ui/plugins/saml/components/SamlPage.tsx (5)
1-14: LGTM!All imports are appropriate and necessary for the component's functionality. The type import for
SamlRootStateensures type safety when accessing Redux state.
21-21: LGTM!Correct usage of
SetTitleat the component's top level. This follows the proper pattern for this custom hook.
48-56: LGTM!The
GluuLoadercorrectly blocks on bothloadingSamlIdpandloadingTrustRelationship, ensuring users see a loading indicator when either Identity Provider or Service Provider data is being fetched. This addresses the concern from the previous review.
59-61: LGTM!Setting
displayNameon the memoized component improves debugging experience in React DevTools.
17-19: The reducer structure is correct. BothloadingSamlIdpandloadingTrustRelationshipare properly defined inSamlReduxStateand managed inSamlSlice.ts. However, the reducer nameidpSamlReduceris semantically misleading—it suggests Identity Provider-only functionality but actually handles both IDP and Service Provider (Trust Relationship) state. Consider renaming tosamlReducerorsamlStateReducerfor clarity, though the current usage is functionally correct.
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 (2)
admin-ui/app/routes/Apps/Gluu/GluuAdvancedSearch.tsx(3 hunks)admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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: 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.
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx
🧬 Code graph analysis (1)
admin-ui/app/routes/Apps/Gluu/GluuAdvancedSearch.tsx (1)
admin-ui/app/routes/Apps/Gluu/types/GluuAdvancedSearch.ts (1)
GluuAdvancedSearchProps(3-15)
🔇 Additional comments (6)
admin-ui/app/routes/Apps/Gluu/GluuAdvancedSearch.tsx (4)
4-16: TypeScript migration is clean and well-typed.The explicit import and usage of
GluuAdvancedSearchPropsproperly types the component. Adding separateonChangeandonKeyDownprops alongside the fallbackhandlergives callers fine-grained control.
19-34: Excellent improvement: wrapper functions replace unsafe type assertions.The new wrapper approach addresses the past review concern about type assertions masking mismatches. By creating explicit wrapper functions that call
handler(event)with the concrete event type, TypeScript can verify thathandler(typed as acceptingChangeEvent | KeyboardEvent) correctly handles both cases. This is much safer than the previous assertions.
36-36: Controlled mode implementation is correct.The
patternPropslogic correctly implements the controlled vs. uncontrolled pattern:valuefor controlled mode (parent-managed state) anddefaultValuefor uncontrolled mode (component-managed state).
41-51: Verify: Should the limit input support controlled mode?The
limitinput usesdefaultValue={limit}withonChange={handleChange}, making it always uncontrolled. In contrast, thepatterninput supports both controlled and uncontrolled modes via thecontrolledprop.This asymmetry might be intentional if the limit doesn't need to be re-synchronized with parent state, but please verify this is the desired behavior. If the parent updates
limit, the input won't reflect the new value since it usesdefaultValue.admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (2)
26-33: Delete flow typing and payload wiring look solidMaking
DeleteItem.inumoptional and guarding inonDeletionConfirmedbefore dispatching, while forwarding bothaction_messageandaction_datafrombuildPayload, gives you a safe and consistent delete path that matches the runtime data shape fromSamlIdentity. No further changes needed here.
228-287: Actions, permissions, and row-shaping are well structuredThe MaterialTable actions are nicely gated by
canReadIdentities/canWriteIdentities/canDeleteIdentities, the edit handler stripstableDatabefore navigation, delete delegates cleanly tohandleDelete, and the advanced search + refresh free actions are clear (with an explicit no-op click handler on the search icon). This matches the rest of the admin UI patterns.
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/saml/components/WebsiteSsoIdentityBrokeringList.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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: 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.
📚 Learning: 2025-11-26T12:25:34.923Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2475
File: admin-ui/plugins/auth-server/components/Clients/ClientDetailPage.js:111-216
Timestamp: 2025-11-26T12:25:34.923Z
Learning: In the admin-ui codebase, avoid suggesting comments for small functionalities or straightforward layout decisions in smaller components. Comments should be reserved for more complex or non-obvious logic.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (5)
admin-ui/app/helpers/navigation.ts (1)
useAppNavigation(76-105)admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/types/redux.ts (1)
SamlIdentity(30-56)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(154-161)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (1)
deleteSamlIdentity(336-360)
🔇 Additional comments (2)
admin-ui/plugins/saml/components/WebsiteSsoIdentityBrokeringList.tsx (2)
26-30: Deletion flow and audit payload wiring look correct and type‑safeThe
DeleteItemshape, guardeditem.inumcheck, andonDeletionConfirmednow align well with the saga andbuildPayloadhelper: you only dispatch wheninumis present, and you forward bothaction_messageandaction_datatodeleteSamlIdentity, preserving audit context. This resolves the earlier typing/payload mismatch cleanly.Also applies to: 125-153, 347-356
231-300: Table actions, permission gating, and compositional setup are consistentThe way you gate edit/add/view/delete actions on
can*Identities, striptableDatavia destructuring before navigation, and composecomponents,options, andcolumnsviauseMemoyields a clear, predictable MaterialTable configuration and respects Cedarling permissions.Also applies to: 314-332, 334-345
…i-issue-2361-saml-1
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
- Edit operation not working in
Identity BrokeringandWebsite SSOform - for
Identity Brokeringaudit payload the resource name should beIdentity-Brokering
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (16)
admin-ui/plugins/saml/redux/sagas/types/saml.ts (1)
1-7: ClarifySamlRootStatenaming to avoid conflict with other root state type.There is another
SamlRootStateinadmin-ui/plugins/saml/types/state.tswith a different shape, which can be confusing and error‑prone when importing. Consider renaming this one to something likeSamlAuthRootState(and updating saga usages) to make the scope explicit.Also applies to: 9-11
admin-ui/plugins/saml/components/SamlPage.tsx (1)
32-46: Switch on translated strings remains fragile.This was noted in a previous review. Using
t('menus.configuration')as switch case values couples rendering logic to translation strings. If translations change or differ across locales, the switch may fail to match, renderingnull.Consider using a constant key-based approach where each tab has a stable
keyproperty, andtabToShowswitches on that key instead of the translated name. This would require coordination with theGluuTabscomponent.admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (4)
111-113: Replacevoidstatements with underscore prefix convention.Using
voidto suppress unused variable warnings is unconventional. Consider using underscore-prefixed destructuring instead.Apply this diff:
- const { metaDataFileImportedFlag, manualMetadata, metaDataFile, ...formValues } = values - void metaDataFileImportedFlag - void manualMetadata + const { metaDataFileImportedFlag: _metaDataFileImportedFlag, manualMetadata: _manualMetadata, metaDataFile, ...formValues } = values
166-190: Extract stable Formik methods to avoid stale closures.Using
formikas a dependency can cause stale closure issues. Extract the stable methods you need.Apply this diff:
+ const { setFieldValue, setFieldTouched, validateForm } = formik + const handleToggleMetadataImport = useCallback( async (event: React.ChangeEvent<HTMLInputElement>) => { if (event.target.checked) { setShowUploadBtn(true) - formik.setFieldValue('metaDataFileImportedFlag', true) - formik.setFieldValue('idpEntityId', '') + setFieldValue('metaDataFileImportedFlag', true) + setFieldValue('idpEntityId', '') // ... rest of the implementation - await formik.validateForm() + await validateForm() } else { // ... } }, - [formik], + [setFieldValue, setFieldTouched, validateForm], )
192-205: ExtractsetValuesandsetFieldTouchedforhandleDrop.Similar to the previous comment, extract Formik methods to avoid including the entire
formikobject in dependencies.Apply this diff:
+ const { setValues, setFieldTouched } = formik + const handleDrop = useCallback( (files: File[]) => { const file = files[0] if (file) { - formik.setValues({ - ...formik.values, + setValues((prev) => ({ + ...prev, metaDataFileImportedFlag: true, metaDataFile: file, - }) - formik.setFieldTouched('metaDataFile', false, false) + })) + setFieldTouched('metaDataFile', false, false) } }, - [formik], + [setValues, setFieldTouched], )
213-216:handleCancelshould respect initial metadata import state.Setting
showUploadBtntofalseunconditionally may cause inconsistency if the initial data had metadata imported. Consider syncing withinitialValues.Apply this diff:
const handleCancel = useCallback(() => { formik.resetForm({ values: initialValues }) - setShowUploadBtn(false) + setShowUploadBtn(initialValues.metaDataFileImportedFlag ?? false) }, [formik, initialValues])admin-ui/plugins/saml/types/redux.ts (3)
3-8: Consider reusingSamlConfigurationFormValuestype.
SamlConfigurationhas identical fields toSamlConfigurationFormValuesfrom./formValues. Consider reusing the type to maintain a single source of truth.Apply this diff:
-export interface SamlConfiguration { - enabled: boolean - selectedIdp: string - ignoreValidation: boolean - applicationName: string -} +export type SamlConfiguration = SamlConfigurationFormValues
62-96: MakesamlMetadataoptional in theWebsiteSsoServiceProvidertype definition.API responses may omit this nested object. While the code already handles this defensively with optional chaining in
utils.tsand provides defaults when transforming data for forms, the type definition should reflect thatsamlMetadatais not guaranteed to be present in API responses.Update line 72 to:
- samlMetadata: { + samlMetadata?: { entityId: string singleLogoutServiceUrl: string nameIDPolicyFormat: string jansAssertionConsumerServiceGetURL: string jansAssertionConsumerServicePostURL: string }This makes the type consistent with the defensive programming patterns already in place throughout the codebase.
125-133: Consider consolidating identical payload interfaces.
CreateSamlIdentityPayloadandCreateWebsiteSsoServiceProviderPayloadhave identical structures. Consider a shared generic type.Apply this diff:
+export interface FormDataPayload { + formdata: FormData + token: string +} + -export interface CreateSamlIdentityPayload { - formdata: FormData - token: string -} - -export interface CreateWebsiteSsoServiceProviderPayload { - formdata: FormData - token: string -} +export type CreateSamlIdentityPayload = FormDataPayload +export type CreateWebsiteSsoServiceProviderPayload = FormDataPayloadadmin-ui/plugins/saml/redux/api/SamlApi.ts (3)
75-109: Add explicit request timeouts to axios calls to prevent indefinite hangs.The axios POST and PUT calls (lines 80-83, 98-101) lack explicit timeout configuration. Since axios defaults to no timeout (0), requests can hang indefinitely if the API is slow or unresponsive.
Consider adding a
timeoutoption to the axios config:axios - .post<SamlIdentityCreateResponse>('/kc/saml/idp/upload', formdata, { + .post<SamlIdentityCreateResponse>('/kc/saml/idp/upload', formdata, { headers: { Authorization: `Bearer ${token}` }, + timeout: 30000, // 30 seconds })Apply the same pattern to the PUT call on lines 98-101.
123-133: Inconsistent response handling ingetWebsiteSsoServiceProvider.This method passes
responsedirectly tohandleResponse, whilegetSamlIdentityProvider(line 67) passesresponse?.body || data. Based on the typeWebsiteSsoServiceProviderListResponsewhich has abodyproperty, this may return the wrong shape.Apply this diff:
if ('getTrustRelationships' in this.api) { this.api.getTrustRelationships((error, _data, response) => { - handleResponse(error, reject, resolve as (data: unknown) => void, response) + handleResponse(error, reject, resolve as (data: unknown) => void, response?.body || _data) })
135-169: Add explicit request timeouts to service provider axios calls.Similar to the identity provider methods, these axios POST and PUT calls (lines 140-143, 158-161) lack timeout configuration and can hang indefinitely.
Add timeout to both axios calls:
axios - .post<SamlApiResponse>('/kc/saml/trust-relationship/upload', formdata, { + .post<SamlApiResponse>('/kc/saml/trust-relationship/upload', formdata, { headers: { Authorization: `Bearer ${token}` }, + timeout: 30000, })Apply the same pattern to the PUT call on lines 158-161.
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (3)
141-163: Add missingtoggleSavedFormFlagdispatch toputSamlPropertiessaga.The
putSamlPropertiessaga is missing thetoggleSavedFormFlagdispatch that all other mutation sagas in this file include. This inconsistency means the saved form flag won't be properly managed for SAML configuration updates.Add
yield put(toggleSamlFormFlag(true))after line 153 andyield put(toggleSavedFormFlag(false))in the catch block before the return statement, following the pattern inpostSamlIdentity(line 310) and other mutation sagas.
184-258: Remove debug console.log statements.Debug
console.logstatements are present in error handlers (lines 211, 249). These should be removed before production deployment as they can expose sensitive error details in the browser console and clutter logs.Apply this diff at lines 211 and 249:
} catch (e) { - console.log('Error: ', e) yield put(toggleSavedFormFlag(false))
290-362: Remove debug console.log statements from identity sagas.Debug
console.logstatements are present in error handlers (lines 315, 353) similar to the service provider sagas. These should be removed for the same reasons.Apply this diff:
} catch (e) { - console.log('Error: ', e) yield put(toggleSavedFormFlag(false))admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
160-200: Clarify unused variable suppression.Line 163 uses
void metaDataFileImportedFlagto suppress the unused variable warning after destructuring. Consider using an underscore prefix in the destructuring pattern itself (e.g.,metaDataFileImportedFlag: _unused) to make the intent clearer without needing the void statement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
admin-ui/app/routes/Apps/Gluu/GluuDialog.tsx(0 hunks)admin-ui/plugins/saml/components/SamlPage.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx(14 hunks)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx(1 hunks)admin-ui/plugins/saml/helper/index.ts(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.ts(1 hunks)admin-ui/plugins/saml/redux/features/SamlSlice.ts(1 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/formValues.ts(1 hunks)admin-ui/plugins/saml/types/index.ts(1 hunks)admin-ui/plugins/saml/types/redux.ts(1 hunks)
💤 Files with no reviewable changes (1)
- admin-ui/app/routes/Apps/Gluu/GluuDialog.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 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.
📚 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/WebsiteSsoServiceProviderList.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/WebsiteSsoServiceProviderList.tsx
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsxadmin-ui/plugins/saml/redux/sagas/SamlSaga.ts
📚 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.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/redux/sagas/SamlSaga.ts
🧬 Code graph analysis (9)
admin-ui/plugins/saml/components/SamlPage.tsx (2)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/app/helpers/navigation.ts (1)
ROUTES(107-107)
admin-ui/plugins/saml/plugin-metadata.js (1)
admin-ui/app/utils/PermChecker.ts (1)
SAML_WRITE(52-52)
admin-ui/plugins/saml/redux/api/SamlApi.ts (3)
admin-ui/plugins/saml/types/api.ts (5)
JansConfigApiClient(32-38)JansIdentityBrokerApiClient(40-58)JansTrustRelationshipApiClient(60-72)SamlIdentityProviderResponse(8-12)SamlApiResponse(27-30)admin-ui/plugins/saml/types/redux.ts (5)
SamlConfiguration(3-8)GetSamlIdentityProviderPayload(118-123)CreateSamlIdentityPayload(125-128)WebsiteSsoServiceProviderListResponse(58-60)CreateWebsiteSsoServiceProviderPayload(130-133)admin-ui/app/utils/ApiUtils.ts (1)
handleResponse(5-20)
admin-ui/plugins/saml/types/redux.ts (2)
admin-ui/plugins/saml/types/index.ts (5)
SamlConfiguration(3-3)SamlIdentity(4-4)WebsiteSsoServiceProvider(5-5)SamlReduxState(2-2)SamlConfigurationFormValues(11-11)admin-ui/plugins/saml/types/formValues.ts (1)
SamlConfigurationFormValues(1-6)
admin-ui/plugins/saml/redux/features/SamlSlice.ts (2)
admin-ui/plugins/saml/types/redux.ts (5)
SamlReduxState(98-109)SamlConfiguration(3-8)GetSamlIdentityProviderPayload(118-123)PutSamlPropertiesPayload(111-116)WebsiteSsoServiceProvider(62-96)admin-ui/plugins/saml/types/api.ts (1)
SamlIdentityProviderResponse(8-12)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (5)
admin-ui/plugins/saml/types/index.ts (4)
WebsiteSsoServiceProvider(5-5)LocationState(12-12)SamlRootState(8-8)WebsiteSsoServiceProviderFormValues(21-21)admin-ui/plugins/saml/types/formValues.ts (2)
LocationState(8-11)WebsiteSsoServiceProviderFormValues(77-112)admin-ui/plugins/saml/helper/validations.ts (2)
websiteSsoServiceProviderValidationSchema(120-159)WebsiteSsoServiceProviderFormValues(14-14)admin-ui/plugins/saml/helper/utils.ts (2)
transformToWebsiteSsoServiceProviderFormValues(89-134)buildWebsiteSsoServiceProviderPayload(326-375)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (1)
updateWebsiteSsoServiceProvider(222-258)
admin-ui/plugins/saml/types/api.ts (1)
admin-ui/plugins/saml/types/redux.ts (4)
SamlIdentity(30-56)SamlConfiguration(3-8)WebsiteSsoServiceProviderListResponse(58-60)WebsiteSsoServiceProvider(62-96)
admin-ui/plugins/saml/helper/utils.ts (4)
admin-ui/plugins/saml/types/redux.ts (3)
SamlConfiguration(3-8)SamlIdentity(30-56)WebsiteSsoServiceProvider(62-96)admin-ui/plugins/saml/types/formValues.ts (8)
SamlConfigurationFormValues(1-6)WebsiteSsoIdentityProviderFormValues(31-69)WebsiteSsoServiceProviderFormValues(77-112)FormValue(14-14)FormValues(15-15)RootFields(17-17)ConfigFields(16-16)CleanableValue(20-28)admin-ui/plugins/saml/helper/constants.ts (1)
nameIDPolicyFormat(6-27)admin-ui/plugins/user-management/utils/userFormUtils.ts (1)
getStringValue(18-26)
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (10)
admin-ui/plugins/saml/redux/sagas/types/saml.ts (3)
PutSamlPropertiesSagaPayload(20-25)CreateSamlIdentitySagaPayload(27-32)SamlRootState(9-11)admin-ui/app/utils/TokenController.ts (3)
AdditionalPayload(10-21)addAdditionalData(49-124)isFourZeroOneError(23-25)admin-ui/plugins/saml/redux/api/SamlApi.ts (1)
SamlApi(28-182)admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/app/redux/api/base.ts (1)
getClient(23-44)admin-ui/plugins/saml/types/redux.ts (3)
SamlConfiguration(3-8)GetSamlIdentityProviderPayload(118-123)WebsiteSsoServiceProviderListResponse(58-60)admin-ui/app/redux/api/backend-api.ts (1)
postUserAction(51-69)admin-ui/plugins/saml/types/api.ts (3)
SamlIdentityProviderResponse(8-12)SamlApiResponse(27-30)SamlIdentityCreateResponse(14-25)admin-ui/plugins/saml/helper/constants.ts (1)
AUDIT_RESOURCE_NAMES(30-34)admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
🪛 Biome (2.1.2)
admin-ui/plugins/saml/helper/validations.ts
[error] 21-21: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 35-35: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 58-58: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 86-86: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 116-116: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 130-130: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 134-134: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (22)
admin-ui/plugins/saml/redux/sagas/types/saml.ts (1)
27-47: Consistent delete payload typing vs. create/update (identity).Create/Update/Delete identity payloads now all carry
action_messageand a clearly typedaction_data/action_inum, which matches the other saga payloads and makes the delete flow type‑safe. This looks coherent and should work well with your existing sagas.admin-ui/plugins/saml/plugin-metadata.js (2)
2-6: LGTM!The new component imports for
WebsiteSsoIdentityProviderFormandWebsiteSsoServiceProviderFormare correctly added and align with the TypeScript migration and unified form component approach described in the PR objectives.
46-69: Route configuration updates look correct.The routes for identity-providers and service-providers (add/edit) are properly updated to use the new unified form components. The permission (
SAML_WRITE) and resource key assignments are consistent with the existing route patterns.admin-ui/plugins/saml/types/index.ts (1)
1-22: LGTM!Clean barrel file implementation that centralizes type exports from the SAML plugin's type modules. This follows TypeScript best practices for module organization and provides a single entry point for type imports.
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (4)
25-29: LGTM!The
DeleteIteminterface properly usesRecord<string, unknown>fortableData, which is more appropriate than the previousRecord<string, never>typing for MaterialTable's internal data.
94-112: LGTM!The
onDeletionConfirmedcallback properly uses theUserActiontype fromPermChecker, addressing the previous review feedback about type alignment.
125-136: LGTM!The Edit action properly strips
tableDatausing destructuring before passing tohandleGoToEditPage, which is cleaner than the previous manual deletion approach.
192-229: LGTM!The render implementation properly wraps the MaterialTable with
GluuViewWrapperfor permission-based visibility and includes theGluuDialogfor deletion confirmation. The component structure follows established patterns in the codebase.admin-ui/plugins/saml/helper/index.ts (1)
1-26: LGTM!Well-organized barrel file that provides a clean public API for the SAML helper module. The exports are logically grouped by source module (constants, validations, types, utilities, table helpers).
admin-ui/plugins/saml/components/SamlPage.tsx (2)
17-19: LGTM!The loading state now correctly includes both
loadingSamlIdpandloadingWebsiteSsoServiceProvider, addressing the previous review feedback about missing the Service Provider loading state.
48-56: LGTM!The
GluuLoadernow properly blocks on both IdP and Service Provider loading states, providing consistent loading feedback across all tabs.admin-ui/plugins/saml/helper/validations.ts (3)
16-29: LGTM! Biome warnings are false positives.The validation schema is correctly structured. The Biome warnings about
.thenproperty are false positives—this is Yup's standard conditional validation API (.when().then()), not Promise-related code.
39-110: Identity provider validation schema is well-structured.The conditional validation logic correctly handles:
- Metadata file requirement based on import flag and existing file presence
- File-like object acceptance for drag-and-drop scenarios
- URL validation for SSO endpoints
- Required field enforcement based on manual vs. metadata-import mode
120-159: Service provider validation schema is well-structured.The schema correctly implements:
- Conditional metadata file validation based on source type
- Nested samlMetadata validation for manual configuration mode
- Proper handling of existing file presence (spMetaDataFN)
admin-ui/plugins/saml/types/formValues.ts (1)
1-112: LGTM! Type definitions are comprehensive and well-structured.The form value types provide a solid foundation for the SAML form handling:
- Clear separation of configuration, identity provider, and service provider forms
- Appropriate use of optional fields
- Utility types for form data transformation
- FileLikeObject type supports flexible file handling
admin-ui/plugins/saml/redux/features/SamlSlice.ts (1)
30-158: LGTM! Redux slice is well-structured with consistent patterns.The slice implementation demonstrates good practices:
- Consistent loading flag management (general + operation-specific)
- Proper fallback values for null responses
- Clean separation of request and response reducers
- All CRUD operations properly covered
The past issues (typo in getSamlIdentities, inconsistent loading flags, missing response handler) have been addressed.
admin-ui/plugins/saml/types/api.ts (1)
1-72: LGTM! API type definitions are well-structured.The API interface definitions provide clear contracts:
- Error-first callback pattern for Node.js style APIs
- Comprehensive response types for identity providers and service providers
- Clean separation between config, identity broker, and trust relationship API clients
- Proper optional types where API responses may vary
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
539-554: Verify Back button navigation behavior.Line 545 navigates to
ROUTES.SAML_SP_LISTon Back. Based on learnings, the Back button in admin-ui forms should navigate to '/home/dashboard'. However, the PR objectives indicate "Back: navigates to the previous page or returns to Dashboard if no history", which suggests context-dependent behavior.Please verify whether:
- The navigation to
ROUTES.SAML_SP_LISTis the intended behavior for this specific form- This represents a change from the previous pattern of always navigating to '/home/dashboard'
- The
navigateBackhelper implements the logic described in the PR objectivesBased on learnings, the previous pattern was to always navigate to dashboard.
admin-ui/plugins/saml/helper/utils.ts (4)
19-28: LGTM: Clean type-safe transformation.The function properly coerces values to expected types and handles undefined/null cases.
89-134: LGTM: Explicit field mapping ensures type safety.The function uses explicit property mapping (lines 93-132) rather than spreading the entire config object. This prevents unexpected properties from leaking into form values and maintains type safety.
225-246: LGTM: Value extraction helpers provide safe type coercion.The helper functions handle various FormValue types and provide appropriate fallbacks. The String() coercion in
getStringValue(line 228) safely handles edge cases where non-string values might be present.
326-375: LGTM: Payload builder follows consistent pattern.The function properly constructs the payload using type-safe helpers and provides sensible defaults for
validationStatus(line 358) andstatus(line 360). The conditionalinumassignment (lines 370-372) correctly handles both create and update scenarios.
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (1)
213-216: Fix UI inconsistency when canceling with imported metadata.The
handleCancelfunction unconditionally setsshowUploadBtntofalse, which causes an inconsistency when the initial data had metadata imported. After canceling, the form will havemetaDataFileImportedFlag=true(frominitialValues) but the upload UI will be hidden.Apply this diff to preserve the initial upload button state:
const handleCancel = useCallback(() => { formik.resetForm({ values: initialValues }) - setShowUploadBtn(false) + setShowUploadBtn(initialValues.metaDataFileImportedFlag ?? false) }, [formik, initialValues])admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (1)
172-178: Optional: Remove unused type assertion.The type assertion
as Action<WebsiteSsoServiceProvider> & { 'data-testid'?: string }is unnecessary since'data-testid'is not being used in this action object. TypeScript can infer the correct type without the assertion.Apply this diff if you want to simplify:
actions.push({ icon: 'refresh', tooltip: `${t('messages.refresh')}`, iconProps: { color: 'primary' }, isFreeAction: true, onClick: handleRefresh, - } as Action<WebsiteSsoServiceProvider> & { 'data-testid'?: string }) + })admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
161-162: Clarify unused variable suppression with underscore prefix.The
void metaDataFileImportedFlagpattern suppresses the unused variable warning but is unclear. Using an underscore prefix makes the intent explicit.const handleSubmit = useCallback( (values: WebsiteSsoServiceProviderFormValues, user_message: string) => { - const { metaDataFileImportedFlag, metaDataFile, ...websiteSsoServiceProviderData } = values - void metaDataFileImportedFlag + const { metaDataFileImportedFlag: _unused, metaDataFile, ...websiteSsoServiceProviderData } = values const formdata = new FormData()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx(1 hunks)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx(14 hunks)admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx(1 hunks)admin-ui/plugins/saml/redux/sagas/types/saml.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
📚 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/WebsiteSsoServiceProviderList.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/WebsiteSsoServiceProviderList.tsx
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.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/WebsiteSsoServiceProviderForm.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (5)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/types/redux.ts (1)
WebsiteSsoServiceProvider(62-96)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(154-161)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (1)
deleteWebsiteSsoServiceProvider(260-288)admin-ui/plugins/saml/helper/tableUtils.tsx (2)
getServiceProviderTableCols(25-38)PaperContainer(40-42)
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (6)
admin-ui/plugins/saml/types/redux.ts (1)
SamlIdentity(30-56)admin-ui/plugins/saml/types/formValues.ts (2)
LocationState(8-11)WebsiteSsoIdentityProviderFormValues(31-69)admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)admin-ui/plugins/saml/helper/validations.ts (2)
WebsiteSsoIdentityProviderFormValues(12-12)websiteSsoIdentityProviderValidationSchema(39-110)admin-ui/plugins/saml/helper/utils.ts (3)
transformToIdentityProviderFormValues(30-76)separateConfigFields(162-178)buildIdentityProviderPayload(248-315)admin-ui/plugins/saml/helper/constants.ts (1)
nameIDPolicyFormat(6-27)
🔇 Additional comments (18)
admin-ui/plugins/saml/redux/sagas/types/saml.ts (4)
1-7: LGTM! Clear auth state structure.The
SamlAuthStateinterface is well-defined with explicit required fields for authentication data.
18-23: LGTM! Elegant generic pattern for saga actions.The
SamlAction<TData, TInum>generic with conditional typing cleanly handles optionalaction_inumwithout resorting to permissive index signatures. This provides strong type safety while keeping the code DRY.
25-30: FormData is the browser API and is already properly handled.
FormDatarefers to the browser's built-in Fetch API class, not a custom type. The saga handlers at lines 70–71 ofSamlSaga.tscorrectly detect and process multipart form data usinginstanceof FormData, wrapping it appropriately for audit logging. No changes needed.
13-16: The imported types are correctly defined in the source module.Both
SamlConfiguration(line 3 of redux.ts) andPutSamlPropertiesPayload(line 111 of redux.ts) are properly exported fromadmin-ui/plugins/saml/types/redux.ts. The re-export aliases are valid and follow good naming conventions for saga-specific types.admin-ui/plugins/saml/components/WebsiteSsoServiceProviderList.tsx (5)
25-31: LGTM! Past issues successfully addressed.The
DeleteIteminterface now correctly typestableDataasRecord<string, unknown>, andDeleteOutlinedIconhas been properly moved outside the component to avoid unnecessary recreation on each render.
33-73: LGTM! Permission-gated data fetching is well-structured.The component correctly:
- Authorizes Cedar scopes on mount
- Checks read permissions before fetching data
- Memoizes permission checks to prevent unnecessary re-computation
- Follows the established Cedar authorization pattern
94-112: LGTM! Deletion flow correctly implements UserAction type.The
onDeletionConfirmedcallback now properly uses the importedUserActiontype, aligning with thebuildPayloadfunction signature and improving type safety.
125-136: LGTM! Edit action correctly strips MaterialTable's internal data.The edit action now properly uses destructuring to remove
tableDatabefore passing the cleaned object tohandleGoToEditPage, preventing MaterialTable's internal properties from leaking into navigation state.
192-229: LGTM! Component rendering is properly permission-gated and theme-aware.The MaterialTable and deletion dialog are correctly wrapped with permission checks, ensuring users only see UI elements they're authorized to use. Theme styling is properly applied to the table header.
admin-ui/plugins/saml/components/WebsiteSsoIdentityProviderForm.tsx (7)
45-107: LGTM! Component initialization is well-structured.The component correctly:
- Merges props from both direct props and router state
- Derives the appropriate title based on view mode
- Memoizes initial values and validation schema
- Sets up Formik with proper configuration including
enableReinitialize
109-156: LGTM! Form submission logic correctly handles create and update flows.The
handleSubmitfunction properly:
- Constructs a
FormDatapayload with both the identity provider JSON and optional metadata file- Uses helper utilities to clean, separate, and build the payload
- Dispatches the appropriate Redux action based on whether editing existing config
Note: Lines 111-113 use
voidstatements to suppress unused variable warnings. While this works, the underscore prefix convention (e.g.,_metaDataFileImportedFlag) is more idiomatic, but this is purely a style preference.
166-211: LGTM! Metadata import handlers are correctly implemented.The handlers properly:
- Toggle between metadata import and manual entry modes
- Handle file drop with appropriate Formik state updates
- Clear files and reset related form state
- Validate the form after state changes
The use of the
formikobject in dependency arrays is acceptable; Formik's methods are stable references and this pattern is commonly used in Formik forms.
218-266: LGTM! Form validation and submission logic is robust.The form correctly:
- Validates metadata file presence before submission when in import mode
- Disables the Apply button based on form validity, dirty state, and file requirements
- Shows field errors appropriately when touched or populated
- Handles both new file uploads and existing metadata files
268-276: LGTM! Navigation correctly returns to the SAML IDP list.The effect properly navigates back to
ROUTES.SAML_IDP_LISTafter a successful save, keeping users in the SAML management context. This aligns with the established pattern of returning to the respective listing screen rather than the dashboard.Based on learnings: This follows the requirement that users should be redirected to the respective listing screen of the component.
513-528: LGTM! Form footer correctly implements button behavior.The footer properly:
- Shows/hides buttons based on
viewOnlymode- Disables Cancel when form is not dirty (no unsaved changes)
- Disables Apply when form is invalid or not dirty
- Navigates back to the SAML IDP list, maintaining context
Based on learnings: The Back button correctly falls back to the SAML IDP listing screen, not the dashboard, keeping users in the SAML management context.
278-541: LGTM! Form rendering correctly adapts to metadata import mode.The form JSX properly:
- Conditionally renders either the metadata file upload or manual entry fields based on the toggle
- Adjusts field requirements dynamically (fields are required only when not importing metadata)
- Uses a unique key on
GluuUploadFileto force remount when file changes- Displays validation errors inline with appropriate conditions
- Disables all inputs in
viewOnlymodeadmin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (2)
567-569: LGTM!Good use of
displayNamefor debugging andmemo()for performance optimization. The component is well-structured with proper TypeScript typing.
545-550: The validation schema atadmin-ui/plugins/saml/helper/validations.tslines 128-145 already enforces the file requirement through conditional validation whenspMetaDataSourceTypeis 'file'. Additionally, thedisableApply={!formik.isValid || !formik.dirty}button state prevents the Apply button from being clickable when the form is invalid, which blocks thetoggle()call from executing with validation errors.The custom validation in
handleFormSubmit(lines 261-265) is redundant to the schema validation but doesn't create a security issue since the button itself prevents invalid submissions. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (4)
141-163: MissingtoggleSavedFormFlagdispatch inputSamlPropertiessaga.This saga is missing the
toggleSavedFormFlagdispatch that other mutation sagas include. Other sagas likepostWebsiteSsoServiceProvider,updateSamlIdentity, etc., dispatchtoggleSavedFormFlag(true)on success andtoggleSavedFormFlag(false)in catch blocks. This inconsistency could cause form dirty-state issues.yield put(putSamlPropertiesResponse(data)) yield call(postUserAction, audit) yield put(updateToast(true, 'success', 'Data updated successfully')) + yield put(toggleSavedFormFlag(true)) return data } catch (e) { yield put(putSamlPropertiesResponse(null)) + yield put(toggleSavedFormFlag(false)) const error = e instanceof Error ? e : new Error(String(e))
211-211: Remove debugconsole.logstatements from error handlers.These debug statements should be removed before production deployment as they can expose sensitive error details in the browser console.
} catch (e) { - console.log('Error: ', e) yield put(toggleSavedFormFlag(false))Apply similar removal at lines 249, 317, and 355.
Also applies to: 249-249, 317-317, 355-355
417-455: Replace hardcoded action strings with typed action creators.All watcher functions use hardcoded string literals (e.g.,
'idpSaml/getSamlConfiguration'). This bypasses TypeScript's type safety and will silently fail at runtime if action names are renamed in the slice.Import action creators from the slice and use their
.typeproperty:+import { + getSamlConfiguration, + getSamlIdentities, + putSamlProperties as putSamlPropertiesAction, + createSamlIdentity, + deleteSamlIdentity as deleteSamlIdentityAction, + updateSamlIdentity as updateSamlIdentityAction, + getWebsiteSsoServiceProvider, + createWebsiteSsoServiceProvider, + updateWebsiteSsoServiceProvider as updateWebsiteSsoServiceProviderAction, + deleteWebsiteSsoServiceProvider as deleteWebsiteSsoServiceProviderAction, +} from '../features/SamlSlice' export function* watchGetSamlConfig(): Generator { - yield takeEvery('idpSaml/getSamlConfiguration', getSamlConfigSaga) + yield takeEvery(getSamlConfiguration.type, getSamlConfigSaga) }
395-402: Consider simplifying error message extraction.The nested optional chaining with type assertions is verbose. A simpler approach improves readability.
function* errorToast({ error }: { error: Error }): Generator<PutEffect, void, void> { + const errorResponse = (error as any)?.response?.data const errorMessage = - (error as { response?: { data?: { description?: string; message?: string } } })?.response?.data - ?.description || - (error as { response?: { data?: { message?: string } } })?.response?.data?.message || - error.message + errorResponse?.description || errorResponse?.message || error.message yield put(updateToast(true, 'error', errorMessage)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx(14 hunks)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts(1 hunks)admin-ui/plugins/saml/types/index.ts(1 hunks)admin-ui/plugins/saml/types/saga.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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-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/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/types/saga.ts
📚 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/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/types/saga.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/components/WebsiteSsoServiceProviderForm.tsxadmin-ui/plugins/saml/types/saga.tsadmin-ui/plugins/saml/redux/sagas/SamlSaga.ts
📚 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/WebsiteSsoServiceProviderForm.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/redux/sagas/SamlSaga.ts
🧬 Code graph analysis (1)
admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (1)
admin-ui/plugins/saml/types/state.ts (1)
SamlRootState(3-11)
🔇 Additional comments (8)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (3)
51-65: LGTM! Well-structured props resolution with location state fallback.The component correctly derives
configsandviewOnlyfrom either props or location state, providing flexibility for both direct prop passing and route-based navigation patterns. The memoization ensures stable references.
134-143: Good Formik configuration with proper validation settings.The form is configured with
enableReinitialize: truewhich correctly handles external data updates, and validation is properly enabled on both change and blur events. This aligns well with the PR's goal of dirty-state detection for Apply/Cancel button enablement.
538-553: Button behavior aligns with PR objectives and navigation learnings.The GluuFormFooter configuration correctly implements:
- Back button always enabled, navigating to
ROUTES.SAML_SP_LIST(consistent with the learning that users should return to the respective listing screen)- Cancel disabled until form is dirty (
!formik.dirty)- Apply disabled until form is both valid and dirty (
!formik.isValid || !formik.dirty)Based on learnings, this navigation pattern keeps users in the SAML context rather than redirecting to the dashboard.
admin-ui/plugins/saml/types/index.ts (1)
1-34: LGTM! Clean and well-organized type barrel file.The barrel file properly centralizes all SAML-related type exports, making imports cleaner for consumers. The separation into redux, state, formValues, and saga modules is logical.
admin-ui/plugins/saml/types/saga.ts (1)
18-31: Good use of generic action types to reduce duplication.The
SamlAction<TData>andSamlActionWithInum<TData>generics provide a clean, reusable pattern for saga payloads. This reduces boilerplate and ensures consistency across create/update/delete operations.admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (3)
65-80: Good solution for the double-cast type compatibility issue.The
toAdditionalPayloadhelper properly converts saga payloads to theAdditionalPayloadtype expected by audit functions, avoiding the unsafeas unknown aspattern. This addresses the structural mismatch between saga and audit types cleanly.
103-118: LGTM! Clean saga implementation with proper error handling.The
getSamlConfigSagafollows good patterns: initializes audit, uses the API factory, dispatches response actions, handles errors with proper type narrowing, and triggers appropriate error handling. This pattern is consistent across the file.
381-381: No changes needed. The empty object{}is a validGetSamlIdentityProviderPayloadsince all properties (inum,limit,pattern,startIndex) are optional. The saga correctly handles this payload and passes it to the API endpoint.Likely an incorrect or invalid review comment.
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (1)
223-233: Apply currently navigates away after save; verify against PR behaviorThe
useEffectnavigates toROUTES.SAML_SP_LISTwheneversavedFormbecomes true, which means a successful Apply will return the user to the SAML SP list instead of keeping them on the current page. That appears to conflict with the PR summary stating “Apply: saves changes without leaving the page”.If the new global behavior really requires Apply to stay on the form, consider either:
- Removing the
navigateBack(ROUTES.SAML_SP_LIST)from this effect, or- Gating navigation behind an explicit “Save & Back” path rather than any save.
Please confirm expected UX for SAML SP forms and adjust accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx(14 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
📚 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/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/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/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/WebsiteSsoServiceProviderForm.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (6)
admin-ui/plugins/saml/types/redux.ts (1)
WebsiteSsoServiceProvider(62-96)admin-ui/plugins/saml/types/formValues.ts (2)
LocationState(8-11)WebsiteSsoServiceProviderFormValues(77-112)admin-ui/plugins/saml/helper/index.ts (4)
websiteSsoServiceProviderValidationSchema(6-6)WebsiteSsoServiceProviderFormValues(10-10)transformToWebsiteSsoServiceProviderFormValues(15-15)buildWebsiteSsoServiceProviderPayload(19-19)admin-ui/plugins/saml/helper/validations.ts (2)
websiteSsoServiceProviderValidationSchema(120-159)WebsiteSsoServiceProviderFormValues(14-14)admin-ui/plugins/saml/helper/utils.ts (2)
transformToWebsiteSsoServiceProviderFormValues(89-134)buildWebsiteSsoServiceProviderPayload(326-375)admin-ui/plugins/saml/redux/sagas/SamlSaga.ts (1)
updateWebsiteSsoServiceProvider(222-258)
🔇 Additional comments (10)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (10)
51-79: Configs/viewOnly derivation and title/SetTitle usage look correctDeriving
configs/viewOnlyfrom props orlocation.stateviauseMemoand callingSetTitle(title)at the top level matches the intended routing/title behavior and the SetTitle usage learning. No issues here.Based on learnings, this aligns with previous navigation/title conventions.
92-123: Attributes loading and default scope mapping are robustThe
useGetAttributescall with a hard limit, theattributesListmapping plus type guard, anddefaultScopeValuebased onconfigs.releasedAttributesare defensive and type-safe. The validation schema memoization is also appropriate.
129-144: Formik initialization with transformed initial values is appropriateUsing
transformToWebsiteSsoServiceProviderFormValues(configs)withenableReinitializeand an external validation schema cleanly ties Redux/config state into the form. The submit handler delegating totoggle()to open the commit dialog keeps concerns separated.
145-165: Scope value derivation and stable key for released attributes look good
scopeFieldValuecorrectly derives display values fromformik.values.releasedAttributesand falls back to Redux/default scopes when needed. The newreleasedAttributesKeybased onconfigs.inumis a much more stable key than the previous JSON.stringify strategy and avoids unnecessary remounts.
166-204: FormData + payload construction logic is soundStripping out
metaDataFileImportedFlag, appendingmetaDataFileonly when present, and delegating object creation tobuildWebsiteSsoServiceProviderPayloadbefore appending as a JSON blob matches the helper contracts and SAML saga expectations. The create vs update branching onconfigsalso looks correct.
208-221: Commit dialog submit flow and scope persistence are consistent
submitFormcleanly bridges the commit dialog back intohandleSubmitusing currentformik.values, andsaveSelectedScopespersisting selections viasetClientSelectedScopeskeeps Redux in sync with the typeahead. This is straightforward and coherent.
349-375: Typeahead configuration and attribute error handling look solidUsing
key={releasedAttributesKey}, wiringvalue/defaultSelectedtoscopeFieldValue, and passingisLoading,placeholder, anddisabled={viewOnly || attributesLoading}give good UX. RenderingGluuStatusMessageonly whenattributesErroris truthy is also appropriate.
397-429: Metadata upload UI is consistent with form stateThe
GluuUploadFilekey,fileNamederivation combiningmetaDataFileandconfigs.spMetaDataFN,showClearButton={!viewOnly}, and conditionalGluuStatusMessagebased onformik.errors.metaDataFile && formik.touched.metaDataFileall line up with the form state and validation rules. Once the submit guard is fixed as noted above, this section should behave correctly for both new and existing SPs.
545-558: Footer props match uniform Back/Cancel/Apply semantics (navigation caveat noted above)
GluuFormFooteris configured as:
- Back always visible, wired to
navigateBack(ROUTES.SAML_SP_LIST), matching the “back to listing screen” pattern from other modules.- Cancel/Apply hidden in view-only mode, and disabled based on
formik.dirty/formik.isValid, which matches the PR’s dirty-state requirements.applyButtonType="button"ensures Apply doesn’t submit the form directly and instead goes through the commit dialog.This footer wiring looks correct; just ensure the
savedFormeffect behavior matches the intended Apply navigation, as raised earlier.Based on learnings, this aligns with the module-specific listing fallback for Back.
574-576: Memoized default export with displayName is appropriateSetting
displayNameand exportingmemo(WebsiteSsoServiceProviderForm)is a good pattern for debugging and avoiding unnecessary re-renders of this fairly heavy form component.
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/WebsiteSsoServiceProviderForm.tsx (1)
438-545: Manual metadata fields are correctly conditioned on source typeThe manual SAML metadata fields are only rendered when
spMetaDataSourceType === 'manual', withrequiredtied to that same condition andviewOnlyrespected across inputs. Error bindings anddoc_entrytags look consistent with other SAML forms. The extraBoolean(spMetaDataSourceType === 'manual')checks onrequiredare slightly redundant given the surrounding conditional block, but they don’t cause any issues and can stay for explicitness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx(14 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
📚 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/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/WebsiteSsoServiceProviderForm.tsx
📚 Learning: 2025-11-05T19:57:35.143Z
Learnt from: faisalsiddique4400
Repo: GluuFederation/flex PR: 2415
File: admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx:148-153
Timestamp: 2025-11-05T19:57:35.143Z
Learning: In the SSA Add page (admin-ui/plugins/auth-server/components/Ssa/SsaAddPage.tsx), the Back button should fall back to '/auth-server/config/ssa' when browser history is unavailable, not '/home/dashboard', as this keeps users in the SSA management context.
Applied to files:
admin-ui/plugins/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/WebsiteSsoServiceProviderForm.tsx
🔇 Additional comments (6)
admin-ui/plugins/saml/components/WebsiteSsoServiceProviderForm.tsx (6)
51-82: Location-derived configs, viewOnly, and title handling are clean and consistentThe precedence
propsConfigs → location.state.rowData → nullandpropsViewOnly → location.state.viewOnly → falseis clear and keeps the component reusable from routes and direct usage. The titleview/edit/createbranching plusSetTitle(title)at the top level is also in line with the documented SetTitle usage pattern. Based on learnings, this is the intended way to manage titles from within admin‑ui forms.
80-82: SAML Redux wiring and navigation on save look correctUsing
SamlRootStatefor SAML slice selectors anduseAppNavigation().navigateBack(ROUTES.SAML_SP_LIST)for post‑save navigation matches the newer pattern of returning users to the relevant listing screen instead of a generic dashboard. The cleanup that resetssavedFormand clearsselectedClientScopeson unmount prevents stale flags and scope selections from leaking into subsequent visits. Based on learnings, the fallback to the SAML SP list here is aligned with the updated Back behavior across domain‑specific sections.Also applies to: 118-123, 223-233
123-144: Formik lifecycle, payload construction, and metadata guard align with SAML flowsThe Formik setup with
websiteSsoServiceProviderValidationSchema,enableReinitialize: true, and theinitialValuesderived fromtransformToWebsiteSsoServiceProviderFormValues(configs)looks correct for both create and edit scenarios.handleSubmit’s construction of aFormDatapayload (optionalmetaDataFile+ JSON blob frombuildWebsiteSsoServiceProviderPayloadand conditional create/update actions) matches the backend expectations and keeps the TS surface type‑safe. The updatedhandleFormSubmitguard that only blocks whenspMetaDataSourceType === 'file'and nometaDataFile,metaDataFileImportedFlag, orspMetaDataFNis present addresses the earlier edge case where edits with an already‑imported metadata file were incorrectly blocked. WithGluuFormFooterenabling Apply only whenformik.dirty && formik.isValid, and the confirm dialog delegating tosubmitForm → handleSubmit, the flow should prevent submitting genuinely missing metadata for the file case while still allowing valid edits. Please just double‑check that the validation schema’s rules for the file/metadata fields stay in sync with this guard so the UI/validation behavior doesn’t diverge over time.Also applies to: 166-215, 264-281, 565-570
234-263: File upload handlers correctly manage metadata flags and validation state
handleDropandhandleClearFilesupdate bothmetaDataFileImportedFlagandmetaDataFilewhile adjustingtouchedformetaDataFile, and they bail out cleanly inviewOnlymode. TheGluuUploadFileinstance’s key (based on current file name, existingspMetaDataFN, and the imported flag) plus thefileNameandshowClearButtonprops give predictable behavior for switching between newly uploaded and previously imported metadata. This all fits well with the guard inhandleFormSubmitand the form‑level validation.Also applies to: 401-437, 407-423
549-563: GluuFormFooter wiring matches the new Back/Cancel/Apply semantics
GluuFormFooteris configured so:
- Back is always shown and enabled, and routes via
navigateBack(ROUTES.SAML_SP_LIST)to keep the user in the SAML context.- Cancel and Apply are hidden in
viewOnlymode.- Cancel calls
handleCancel, which resets the form toinitialValues, and is disabled until the form is dirty.- Apply is a plain button (
applyButtonType="button") that just opens the commit dialog and is disabled unless the form is both dirty and valid.This matches the PR objective of consistent Back/Cancel/Apply behavior across forms and the newer pattern of falling back to the relevant listing page rather than a global dashboard. Based on learnings, this Back behavior is aligned with how other admin‑ui sections were updated.
Also applies to: 260-262
578-580: memo export and displayName improve rendering and DevTools claritySetting
displayNameand exporting a memoizedWebsiteSsoServiceProviderFormis a nice touch for both performance and debugging, especially given the number of props and hooks in play.
|
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
Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.