Refactor allowed attributes field to use claim autocomplete #9535
Conversation
Replaces the text input for allowed attributes with an Autocomplete component that fetches and displays claim URIs and display names. Improves usability by allowing selection from available claims, adds tooltips, and updates state management for selected claims. Also updates form submission logic to handle the new selection method.
WalkthroughReplaces a plain text input for allowed claim attributes with a multi-select Autocomplete that fetches local claims, maps URIs to display names, shows chips with tooltips, and submits joined claim URIs under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (UI)
participant Form as MultiAttributeLoginForm
participant ClaimsAPI as LocalClaimsService
participant Server as ConfigurationAPI
Note over Form,ClaimsAPI: On mount / open
User->>Form: Opens form
Form->>ClaimsAPI: Fetch local claims (useGetAllLocalClaims)
ClaimsAPI-->>Form: Return claims list (uris + display names)
Form->>Form: build claimMap & claimURIs (render options)
Note over User,Form: User selects claims
User->>Form: Selects/Deselects claims (Autocomplete)
Form-->>Form: update selectedClaims (handleClaimChange)
Note over Form,Server: On submit
User->>Form: Submit
Form->>Server: POST updated config (account.multiattributelogin.handler.allowedattributes = joined selectedClaims)
Server-->>Form: Acknowledge / persist
Form->>User: Show success / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @features/admin.server-configurations.v1/forms/multi-attribute-login.tsx:
- Around line 265-276: The Chip inside renderTags is receiving a duplicate key
because getTagProps({ index }) already injects a key; remove the explicit key={
option } from the Chip component (keep the spread {...getTagProps({ index })}
and the Tooltip's key if needed) so only one key prop is present; locate the
renderTags function and edit the Chip JSX to drop key={ option } while
preserving label={getDisplayName(option)} and size="small".
🧹 Nitpick comments (5)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx (5)
83-99: Consider extracting styles outside the component.These style objects are recreated on every render. Moving them outside the component as module-level constants would avoid unnecessary object allocations.
Proposed refactor
+const containerStyle: React.CSSProperties = { marginBottom: "1rem" }; +const labelStyle: React.CSSProperties = { + display: "block", + fontSize: "0.875rem", + fontWeight: 500, + marginBottom: "0.5rem" +}; +const spanStyle: React.CSSProperties = { + overflow: "hidden", + textOverflow: "ellipsis", + whiteSpace: "nowrap" +}; +const helperTextStyle: React.CSSProperties = { + color: "rgba(0, 0, 0, 0.6)", + fontSize: "0.75rem", + marginTop: "0.25rem" +}; + export const MultiAttributeLoginForm: FunctionComponent<MultiAttributeLoginFormPropsInterface> = ( props: MultiAttributeLoginFormPropsInterface ): ReactElement => { // ... component code, remove style definitions from inside
107-116: Potential issues with claims fetching configuration.
claimsParamsis recreated on every render. If the hook uses object reference equality, this could cause unnecessary re-fetches. Consider memoizing withuseMemoor extracting as a constant.Hard-coded
limit: 1000may be insufficient for deployments with many claims.The hook's
errorstate isn't destructured or handled. Consider providing user feedback if claim loading fails.Proposed improvements
+ const claimsParams: ClaimsGetParams = useMemo(() => ({ + filter: "", + limit: 1000, + offset: 0, + sort: "" + }), []); + const { data: claimsData, - isLoading: isClaimsLoading + isLoading: isClaimsLoading, + error: claimsError } = useGetAllLocalClaims<Claim[]>(claimsParams, isConnectorEnabled);Then handle
claimsErrorappropriately in the UI (e.g., show an error message).
152-154: Minor formatting issue and consider memoization.Missing spaces in the type annotation and after
||. Also, sincegetDisplayNamedepends onclaimMap, consider wrapping it withuseCallbackto prevent unnecessary re-creations.Proposed fix
- const getDisplayName = (uri:string): string => { - return claimMap.get(uri) ||uri; - }; + const getDisplayName = useCallback((uri: string): string => { + return claimMap.get(uri) || uri; + }, [claimMap]);
214-216: Unused event parameter.The
eventparameter is required by the Autocomplete onChange signature but isn't used. Consider prefixing with underscore for clarity.- const handleClaimChange = (event: React.SyntheticEvent, newValue: string[]): void => { + const handleClaimChange = (_event: React.SyntheticEvent, newValue: string[]): void => {
248-255: Hard-coded placeholder strings should use i18n.The placeholder texts "Loading claims..." and "Select claim URIs" are hard-coded. For consistency with the rest of the codebase, use the
t()translation function.<TextField { ...params } - placeholder={ isClaimsLoading ? "Loading claims..." : "Select claim URIs" } + placeholder={ isClaimsLoading + ? t("common:loading") + : t("governanceConnectors:form.placeholder.selectClaimURIs") + } size="small" data-componentid={ `${componentId}-allowed-attribute-list` } />Note: Adjust the translation keys to match your i18n configuration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx (1)
modules/core/src/models/claim.ts (2)
ClaimsGetParams(123-132)Claim(22-48)
🔇 Additional comments (2)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx (2)
160-190: LGTM!The initial values parsing logic correctly extracts and processes the comma-separated allowed attributes into the
selectedClaimsstate.
198-207: LGTM!The submission logic correctly joins selected claims into a comma-separated string for the expected property key.
| renderTags={ (value: string[], getTagProps: (args: { index: number }) => any) => | ||
| value.map((option: string, index: number) => ( | ||
| <Tooltip title={ option } key={ option } placement="top"> | ||
| <Chip | ||
| { ...getTagProps({ index }) } | ||
| label={ getDisplayName(option) } | ||
| size="small" | ||
| key={ option } | ||
| /> | ||
| </Tooltip> | ||
| )) | ||
| } |
There was a problem hiding this comment.
Duplicate key prop on Chip component.
getTagProps({ index }) already provides a key prop. Adding an explicit key={ option } on the Chip causes duplicate key assignment. Remove the explicit key from Chip to avoid potential React warnings.
Proposed fix
renderTags={ (value: string[], getTagProps: (args: { index: number }) => any) =>
value.map((option: string, index: number) => (
<Tooltip title={ option } key={ option } placement="top">
<Chip
{ ...getTagProps({ index }) }
label={ getDisplayName(option) }
size="small"
- key={ option }
/>
</Tooltip>
))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| renderTags={ (value: string[], getTagProps: (args: { index: number }) => any) => | |
| value.map((option: string, index: number) => ( | |
| <Tooltip title={ option } key={ option } placement="top"> | |
| <Chip | |
| { ...getTagProps({ index }) } | |
| label={ getDisplayName(option) } | |
| size="small" | |
| key={ option } | |
| /> | |
| </Tooltip> | |
| )) | |
| } | |
| renderTags={ (value: string[], getTagProps: (args: { index: number }) => any) => | |
| value.map((option: string, index: number) => ( | |
| <Tooltip title={ option } key={ option } placement="top"> | |
| <Chip | |
| { ...getTagProps({ index }) } | |
| label={ getDisplayName(option) } | |
| size="small" | |
| /> | |
| </Tooltip> | |
| )) | |
| } |
🤖 Prompt for AI Agents
In @features/admin.server-configurations.v1/forms/multi-attribute-login.tsx
around lines 265-276, The Chip inside renderTags is receiving a duplicate key
because getTagProps({ index }) already injects a key; remove the explicit key={
option } from the Chip component (keep the spread {...getTagProps({ index })}
and the Tooltip's key if needed) so only one key prop is present; locate the
renderTags function and edit the Chip JSX to drop key={ option } while
preserving label={getDisplayName(option)} and size="small".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9535 +/- ##
=======================================
Coverage 55.88% 55.88%
=======================================
Files 42 42
Lines 1020 1020
Branches 254 254
=======================================
Hits 570 570
- Misses 439 450 +11
+ Partials 11 0 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| /** | ||
| * Flattens and resolved form initial values and field metadata. | ||
| */ | ||
|
|
Is there any specific reason to only show email and username attributes in the list of attributes? |
|
For a quick local test, I added mock partial claims at the top of the file to check how the UI would display the attributes. |
| [ property.name ]: property.value | ||
| [property.name]: property.value |
There was a problem hiding this comment.
no , i accidentally added space
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Pavindu Lakshan <pavindulakshan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx (5)
83-99: Consider extracting inline styles to a stylesheet or styled-components.Defining styles as inline objects works, but for consistency with the codebase and easier theming, consider using CSS modules, styled-components, or the project's existing styling pattern.
107-116: MemoizeclaimsParamsto prevent potential refetch loops.The
claimsParamsobject is recreated on every render. IfuseGetAllLocalClaimsuses referential equality checks, this could cause unnecessary refetches or infinite loops.Proposed fix
+ const claimsParams: ClaimsGetParams = useMemo(() => ({ + filter: "", + limit: 1000, + offset: 0, + sort: "" + }), []); - const claimsParams: ClaimsGetParams = { - filter: "", - limit: 1000, - offset: 0, - sort: "" - };Also add
useMemoto the imports on line 25:-import React, { FunctionComponent, ReactElement, useEffect, useState } from "react"; +import React, { FunctionComponent, ReactElement, useEffect, useMemo, useState } from "react";
128-150: Move pure helper functions outside the component.
getClaimURIsandcreateClaimMapdon't depend on component state or props. Moving them outside the component avoids unnecessary recreation on each render and improves clarity.Proposed refactor
Move these functions above the component definition:
+const getClaimURIs = (claims: Claim[]): string[] => { + if (!claims || claims.length === 0) { + return []; + } + return claims + .map((claim: Claim) => claim?.claimURI) + .filter((uri: string) => typeof uri === "string" && uri.length > 0); +}; + +const createClaimMap = (claims: Claim[]): Map<string, string> => { + const map: Map<string, string> = new Map<string, string>(); + if (!claims || claims.length === 0) { + return map; + } + claims.forEach((claim: Claim) => { + if (claim?.claimURI && claim?.displayName) { + map.set(claim.claimURI, claim.displayName); + } + }); + return map; +}; + export const MultiAttributeLoginForm: FunctionComponent<MultiAttributeLoginFormPropsInterface> = (Then remove them from inside the component body.
152-154: Minor formatting: missing space after||.Proposed fix
- const getDisplayName = (uri:string): string => { - return claimMap.get(uri) ||uri; + const getDisplayName = (uri: string): string => { + return claimMap.get(uri) || uri; };
171-178: UseforEachinstead ofmapfor side effects.
.map()is intended for transformations that return values. Since the return value is ignored here and you're only performing side effects (populatingresolvedInitialValuesandresolvedInitialFormValues), use.forEach()instead.Proposed fix
- initialValues.properties.map((property: ConnectorPropertyInterface) => { - + initialValues.properties.forEach((property: ConnectorPropertyInterface) => { resolvedInitialValues.set(property.name, property); resolvedInitialFormValues = { ...resolvedInitialFormValues, [property.name]: property.value }; });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx (1)
modules/core/src/models/claim.ts (2)
ClaimsGetParams(123-132)Claim(22-48)
🔇 Additional comments (4)
features/admin.server-configurations.v1/forms/multi-attribute-login.tsx (4)
18-32: Imports look good.The new imports for MUI components (Autocomplete, Chip, TextField, Tooltip) and the claims-related types/hook are appropriate for the refactored functionality.
198-216: Form submission logic looks correct.The
getUpdatedConfigurationsfunction properly joins selected claims and returns the expected data structure. ThehandleClaimChangehandler is appropriately simple.
253-260: Potential duplicate key inrenderOption.The
propsargument from MUI's AutocompleterenderOptionmay already include akeyprop. Spreadingpropsafter setting an explicitkeyis correct here, but verify there are no console warnings about duplicate keys.
287-298: Submit button configuration looks correct.The button properly handles disabled state, loading state, and visibility based on connector and read-only states.
2 participants
Purpose
Replaces the text input for allowed attributes with an Autocomplete component that fetches and displays claim URIs and display names. Improves usability by allowing selection from available claims, adds tooltips, and updates state management for selected claims. Also updates form submission logic to handle the new selection method.
WSO2.Identity.Server.Console.-.Multi.Attribute.Login.mp4
Related Issues
wso2/product-is#22655
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Summary by CodeRabbit
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.