fix(admin-ui): scopes not displayed for clients in client search resu… #2533
Conversation
faisalsiddique4400
requested review from
duttarnab and
syntrydy
as code owners
📝 WalkthroughWalkthroughRemoved the legacy Redux-connected ClientShowScopes JS component and added a new TypeScript ClientShowScopes TSX component that maps scope DNs to inums, queries OAuth scopes when opened, shows a loading state, renders themed scope badges or a localized "no scopes" message, and exposes a close handler via props. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
comp-admin-ui
faisalsiddique4400
requested review from
moabu
and removed request for
syntrydy
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js (3)
admin-ui/plugins/auth-server/components/Clients/ClientScopeUtils.js (1)
scopeDn(15-15)admin-ui/app/utils/Util.ts (1)
getClientScopeByInum(34-38)admin-ui/api-client.ts (1)
AXIOS_INSTANCE(9-9)
🔇 Additional comments (2)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js (2)
42-43: Verify the API response structure and scope object properties are correct.The code assumes the API response at
/api/v1/scopesreturns eitherdata.entriesordatadirectly as an array, and that each scope object hasid,displayName, andinumproperties. Confirm these assumptions match the actual API specification to prevent runtime errors if the response structure differs.
36-41: The/api/v1/scopesendpoint parameters are correctly implemented. Thepatternparameter accepts comma-separated scope inums andlimitspecifies the result set size—this matches the API contract used consistently across ClientBasicPanel.js, OAuthScopeSaga.js, and the underlying ScopeApi wrapper. The response structure (entries or data field) is properly handled.
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js (1)
16-54: Add cleanup to prevent race conditions and memory leaks.The
useEffectdoes not cancel in-flight API requests when the component unmounts or whenisOpen/datachanges. This can cause:
- React warnings about setting state on unmounted components
- Race conditions where an older request resolves after a newer one, showing stale data
- Memory leaks
🔎 Apply this diff to add request cancellation:
useEffect(() => { if (!isOpen || !data || data.length === 0) { setFetchedScopes([]) setLoading(false) return } const fetchScopesForClient = async () => { + const controller = new AbortController() setLoading(true) const scopeInums = data .map((scopeDn) => getClientScopeByInum(scopeDn)) .filter((inum) => inum !== null && inum !== undefined && inum !== '') if (scopeInums.length === 0) { setFetchedScopes([]) setLoading(false) return } try { const response = await AXIOS_INSTANCE.get('/api/v1/scopes', { params: { pattern: scopeInums.join(','), limit: scopeInums.length, }, + signal: controller.signal, }) const scopeResults = response.data?.entries || response.data || [] setFetchedScopes(Array.isArray(scopeResults) ? scopeResults : []) } catch (error) { + if (error.name === 'CanceledError' || error.name === 'AbortError') { + return + } console.error('Error fetching scopes:', error) setFetchedScopes([]) } setLoading(false) + return controller } - fetchScopesForClient() + const controllerPromise = fetchScopesForClient() + return () => { + controllerPromise.then((controller) => controller?.abort()) + } }, [isOpen, data])
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js (4)
admin-ui/plugins/auth-server/components/Clients/ClientScopeUtils.js (1)
scopeDn(15-15)admin-ui/app/utils/Util.ts (1)
getClientScopeByInum(34-38)admin-ui/api-client.ts (1)
AXIOS_INSTANCE(9-9)admin-ui/app/components/index.tsx (3)
Modal(81-81)ModalHeader(84-84)ModalBody(82-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: sonar scan (flex-linux-setup)
🔇 Additional comments (2)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js (2)
1-14: LGTM: Imports and state setup are appropriate.The addition of
useEffect,useState,Spinner,AXIOS_INSTANCE, andgetClientScopeByInumcorrectly supports the new async data-fetching approach. Local state forfetchedScopesandloadingis well-structured for managing the fetch lifecycle.
37-42: The API parameter usage follows established patterns in the codebase. The comma-separatedinumformat for thepatternparameter is consistent acrossClientShowScopes.jsandClientBasicPanel.js, and the response handling already includes defensive fallback logic (response.data?.entries || response.data || []) to accommodate both expected and alternative response formats. No issues identified.
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js
Outdated
Show resolved
Hide resolved
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
We should use orval to call API.
| } | ||
|
|
||
| try { | ||
| const response = await AXIOS_INSTANCE.get('/api/v1/scopes', { |
There was a problem hiding this comment.
we should use orval to call API
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js(0 hunks)admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.js
🔇 Additional comments (5)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx (5)
1-8: LGTM!The imports are well-organized and include all necessary dependencies for the component functionality.
10-14: LGTM!The interface clearly defines the component props with appropriate TypeScript types.
21-28: LGTM!The memoized conversion from scope DNs to inums is well-implemented with appropriate filtering to remove invalid values.
76-76: LGTM!Standard default export following React component conventions.
30-44: Verify the pattern parameter format in the OAuth scopes API.The
patternparameter is being set to comma-separated scope inums (line 36), but the API may expect a search pattern or filter rather than multiple identifiers. Consider verifying:
- Whether the API's pattern parameter actually supports comma-separated identifier lists
- Whether using
useGetOauthScopesByInumin a loop or an alternative batch-fetch method would be more appropriate
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx(1 hunks)
🔇 Additional comments (4)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx (4)
1-8: LGTM! Well-structured imports.The imports are appropriate and well-organized, covering React hooks, UI components, theming, translations, and API integration.
10-14: LGTM! Clear prop interface.The props interface is well-defined with appropriate types for the modal handler, scope data, and visibility state.
16-19: LGTM! Proper hook initialization.The component correctly initializes translation and theme hooks with a sensible fallback for the theme.
49-73: No issues found. All translation keys used in the component (fields.scopes,messages.no_scope_in_client,actions.close) are properly defined in the locale files. The modal rendering logic is correct with appropriate loading states and fallback messages.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T14:18:58.310Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:107-109
Timestamp: 2025-11-10T14:18:58.310Z
Learning: In the Cedarling configuration page (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), console.error calls should be kept in catch blocks when there are multiple operations in the try block, as they help print the whole error object with details for debugging unexpected errors.
Applied to files:
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx (4)
admin-ui/plugins/auth-server/components/Clients/ClientScopeUtils.js (1)
scopeDn(15-15)admin-ui/app/utils/Util.ts (1)
getClientScopeByInum(34-38)admin-ui/app/components/index.tsx (6)
Modal(81-81)ModalHeader(84-84)ModalBody(82-82)Badge(37-37)ModalFooter(83-83)Button(40-40)admin-ui/plugins/auth-server/components/Clients/ClientAdvancedPanel.js (1)
handler(34-36)
🔇 Additional comments (3)
admin-ui/plugins/auth-server/components/Clients/ClientShowScopes.tsx (3)
1-14: LGTM! Clean TypeScript setup.The imports are well-organized, and the props interface is properly typed with clear, descriptive names.
21-26: LGTM! Memoization is properly implemented.The conversion from scope DNs to inums is correctly memoized with appropriate edge case handling. The
.filter(Boolean)simplification from the previous review has been successfully applied.
43-70: LGTM! Rendering logic is well-structured.The modal correctly handles loading and empty states, and all user-facing strings are properly translated. The fixes from the previous review have been successfully applied:
- Modal header now uses
t('fields.scopes')instead of hardcoded text- Badge displays
scope.displayName || scope.id, prioritizing the user-friendly name
fix(admin-ui): scopes not displayed for clients in client search results (#2529)
Summary
On the Clients search page, scope records were not being displayed for clients that already had scopes assigned. This caused confusion and made it appear as if clients had no associated scopes.
Fix Summary
Result
Clients with assigned scopes now correctly display their scope records in the Clients search page.
Closes
Closes: #2529
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.