fix(admin-ui): Scope update issues, missing toasts, and disappearing action buttons #2479
Conversation
faisalsiddique4400
requested review from
duttarnab and
syntrydy
as code owners
📝 WalkthroughWalkthroughIntroduces typed scope defaults (INITIAL_SCOPE, DEFAULT_SCOPE_ATTRIBUTES, ScopeWithMessage), centralizes submit/update flows into memoized handlers using react-query QueryClient for targeted cache invalidation, adds deep-diff change detection (hasActualChanges), refactors list/detail rendering and theming, and adds toasts and audit logging across scope CRUD flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ScopeForm
participant ScopePage
participant API as ScopeAPI
participant QueryClient
participant Toast
participant AuditLog
User->>ScopeForm: submit(values)
ScopeForm->>ScopePage: handleSubmit(values)
rect `#e6f4ea`
ScopePage->>ScopePage: parse JSON, extract action_message, clean payload
end
rect `#e7eefa`
ScopePage->>API: mutateAsync(scopePayload)
API-->>ScopePage: success / error
end
rect `#fff1d6`
ScopePage->>QueryClient: invalidate queries (predicate getGetOauthScopesQueryKey / getOauthScopesByInum)
ScopePage->>Toast: updateToast(success or error)
ScopePage->>AuditLog: log action_message (if present)
ScopePage->>User: navigate to scope list (on success)
end
sequenceDiagram
participant User
participant ScopeList
participant Query
participant Table
User->>ScopeList: open/list
ScopeList->>Query: fetch(scopesQueryParams)
Query-->>Table: rows
User->>ScopeList: type in search
ScopeList->>ScopeList: debounce 500ms
ScopeList->>Query: refetch with updated params
Query-->>Table: filtered rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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/auth-server/components/Scopes/ScopeListPage.tsx (1)
329-342: Audit logging failure blocks modal closure.If
logScopeDeletionthrows an error after successful deletion, the modal won't close (toggle()won't be called) even though the scope was actually deleted. This could confuse users.Consider separating the audit logging from the critical path:
const onDeletionConfirmed = useCallback( async (message: string) => { if (!item?.inum) return try { await deleteScope.mutateAsync({ inum: item.inum }) - await logScopeDeletion(item, message) toggle() + // Fire-and-forget audit logging + logScopeDeletion(item, message).catch((err) => + console.error('Failed to log scope deletion:', err) + ) } catch (error) { console.error('Error deleting scope:', error) } }, [item, deleteScope, logScopeDeletion, toggle], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx(3 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeDetailPage.tsx(2 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx(3 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeForm.tsx(7 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx(7 hunks)admin-ui/plugins/auth-server/components/Scopes/helper/utils.ts(1 hunks)admin-ui/plugins/auth-server/components/Scopes/hooks/index.ts(0 hunks)admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts(0 hunks)admin-ui/plugins/auth-server/components/Scopes/types/index.ts(0 hunks)
💤 Files with no reviewable changes (3)
- admin-ui/plugins/auth-server/components/Scopes/hooks/index.ts
- admin-ui/plugins/auth-server/components/Scopes/types/index.ts
- admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts
🧰 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.
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsxadmin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsxadmin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx
🧬 Code graph analysis (4)
admin-ui/plugins/auth-server/components/Scopes/ScopeDetailPage.tsx (4)
admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/auth-server/components/Scopes/types/index.ts (1)
ScopeDetailPageProps(28-28)admin-ui/plugins/auth-server/components/Scopes/types/componentTypes.ts (1)
ScopeDetailPageProps(4-6)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (7)
admin-ui/plugins/auth-server/components/Scopes/types/index.ts (3)
ScopeTableRow(33-33)Scope(2-2)ScopeWithClients(11-11)admin-ui/plugins/auth-server/components/Scopes/types/componentTypes.ts (1)
ScopeTableRow(24-28)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts (1)
useScopeActions(26-106)admin-ui/plugins/auth-server/components/Scopes/types/scopeTypes.ts (2)
Scope(11-11)ScopeWithClients(40-40)admin-ui/plugins/auth-server/components/Scopes/scopes.test.ts (1)
scopes(3-569)
admin-ui/plugins/auth-server/components/Scopes/ScopeForm.tsx (3)
admin-ui/plugins/auth-server/components/Scopes/types/scopeTypes.ts (1)
ExtendedScope(29-38)admin-ui/plugins/auth-server/components/Scopes/types/formTypes.ts (1)
ScopeFormValues(3-15)admin-ui/plugins/auth-server/components/Scopes/helper/utils.ts (1)
hasActualChanges(93-112)
admin-ui/plugins/auth-server/components/Scopes/helper/utils.ts (1)
admin-ui/plugins/auth-server/components/Scopes/types/formTypes.ts (1)
ScopeFormValues(3-15)
🔇 Additional comments (25)
admin-ui/plugins/auth-server/components/Scopes/helper/utils.ts (1)
93-112: LGTM!The
hasActualChangesfunction correctly compares all relevantScopeFormValuesfields, appropriately excludingaction_messagewhich is only used during submission. The use of helper functions for complex comparisons keeps the logic clean.admin-ui/plugins/auth-server/components/Scopes/ScopeDetailPage.tsx (3)
10-10: LGTM!Extracting
CONTAINER_STYLESas a module-level constant prevents unnecessary object recreation on each render.
17-43: LGTM!The memoization strategy is well-implemented with correct dependency arrays. The
useCallbackforrenderAttributeRowappropriately prevents unnecessary re-renders when passed tomap.
45-103: LGTM!The JSX structure is clean and well-organized. The use of memoized values for badge colors and attribute rendering is appropriate.
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx (3)
166-183: LGTM with minor observation.The conditional fetching logic prevents unnecessary API calls when data already exists. Note that if an API call fails and returns an empty result, this could lead to repeated fetch attempts. Consider adding error state tracking if this becomes an issue.
83-97: LGTM!The
extensibleScopememoization correctly handles both the case whenscopeDataexists and when it's undefined, providing sensible defaults.
135-142: Consider using the generated query key function for consistency, but verify availability first.The hardcoded string
'getOauthScopesByInum'is used across four files (ScopeEditPage, ScopeAddPage, ScopeListPage) whilegetGetOauthScopesQueryKey()is properly imported. TheuseGetOauthScopesByInumhook is actively used and generates a query with this key. However, searching the codebase reveals no correspondinggetGetOauthScopesByInumQueryKeyfunction—despite the naming pattern used for other endpoints likegetGetOauthScopesQueryKey. If the generated API includes this query key function, importing and using it would improve consistency and reduce brittleness. If not, consider whether the OpenAPI generator should be configured to export it.admin-ui/plugins/auth-server/components/Scopes/ScopeForm.tsx (6)
32-38: LGTM!Proper imports added for
ExtendedScopetype andhasActualChangesutility function.
97-97: LGTM!The type change from
Record<string, unknown>toPartial<ExtendedScope>improves type safety.
213-245: LGTM!The scope type change handler correctly uses
setValuesandsetTouchedwith validation disabled (second parameterfalse), then triggers validation viasetTimeoutto ensure state updates have propagated. This is the proper pattern for coordinated Formik state updates.
677-691: LGTM!Using
hasActualChangesinstead of Formik'sdirtystate provides more accurate change detection, as it performs deep comparison of arrays and nested attributes. This correctly addresses the PR objective of ensuring action buttons remain properly enabled/disabled.
189-210: LGTM!The
onSubmithandler correctly merges form values with existing scope data, properly handles attributes, and sets creator information. The modal-based confirmation flow is preserved.
404-411: LGTM!Adding the
requiredattribute to the scope type label correctly indicates this field is mandatory in the UI.admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx (2)
114-131: LGTM!The data fetching logic is consistent with
ScopeEditPage.tsxand appropriately conditionally fetches only when needed.
143-164: LGTM!The render structure is clean with proper loading state, error display, and form initialization using
INITIAL_SCOPE.admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (10)
1-51: LGTM!Imports are well-organized and the
DetailPanelPropsinterface provides proper typing for the detail panel component.
53-79: LGTM!Style constants are properly extracted outside the component to prevent recreation on each render. Using
as constfor type narrowing is a good practice.
144-169: LGTM!Loading state correctly combines initial load and refetch states. Delete handlers properly dispatch toast notifications and invalidate relevant queries. Error handling includes a sensible fallback message.
436-485: LGTM!Table actions are properly conditional based on Cedar permissions. The defensive array checks in click handlers are reasonable given MaterialTable's type signatures allow both single and array selections.
612-613: Verify intentional table remount on page size change.Using
limitas thekeyprop forces a complete remount of the MaterialTable component wheneverlimitchanges. This will reset any internal table state (expanded detail panels, column visibility toggles, etc.).If this is intentional to ensure clean state on page size change, consider adding a comment. If not, remove the key or use a stable identifier.
269-277: LGTM!
handleClearFilterscomprehensively resets all filter state including the newsearchInputfield, ensuring a clean slate when users click the clear button.
290-308: LGTM!Column render functions are properly memoized with correct dependencies. The clients column gracefully handles missing data and provides useful navigation to filtered clients.
513-638: LGTM!The JSX structure is well-organized with proper accessibility attributes (
aria-label,title), conditional rendering for permissions, and a clean separation between the filter UI and data table. TheGluuLoaderwrapper ensures users see feedback during data loading.
421-425: Verifyfield: 'dn'for clients column is intentional.The clients column uses
field: 'dn'but renders client count. If MaterialTable's built-in sorting/filtering is enabled for this column, it would operate on thednfield rather than the actual clients count. This may be intentional (to disable sorting on this column) or an oversight.If sorting by client count is desired, consider using a computed field or disabling sorting for this column explicitly with
sorting: false.
497-499:authorizeHelperis properly memoized and the dependency is correct.The
useCedarlinghook wrapsauthorizeHelperwithuseCallback(lines 186-241 inuseCedarling.ts), ensuring it has a stable reference. WhileauthorizeHelperwill be recreated when its dependencies change (when authorization state or tokens change), this is expected behavior and consistent with the pattern used in other pages likeUserList.tsx. The effect will not run on every render.
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx(3 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx(3 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx(7 hunks)admin-ui/plugins/auth-server/components/Scopes/constants.ts(1 hunks)admin-ui/plugins/auth-server/components/Scopes/helper/utils.ts(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.
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsxadmin-ui/plugins/auth-server/components/Scopes/helper/utils.tsadmin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsxadmin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx
🧬 Code graph analysis (3)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (5)
admin-ui/plugins/auth-server/components/Scopes/types/index.ts (3)
ScopeTableRow(33-33)Scope(2-2)ScopeWithClients(11-11)admin-ui/plugins/auth-server/components/Scopes/types/componentTypes.ts (1)
ScopeTableRow(24-28)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts (1)
useScopeActions(26-106)
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx (3)
admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts (1)
useScopeActions(26-106)admin-ui/plugins/auth-server/components/Scopes/types/scopeTypes.ts (4)
ExtendedScope(29-38)ScopeClient(23-27)EMPTY_SCOPE(69-78)Scope(11-11)admin-ui/app/utils/PermChecker.ts (1)
buildPayload(153-160)
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx (3)
admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts (1)
useScopeActions(26-106)admin-ui/plugins/auth-server/components/Scopes/types/index.ts (2)
ModifiedFields(22-22)Scope(2-2)admin-ui/plugins/auth-server/components/Scopes/constants.ts (2)
ScopeWithMessage(4-6)INITIAL_SCOPE(12-22)
🔇 Additional comments (12)
admin-ui/plugins/auth-server/components/Scopes/helper/utils.ts (3)
74-81: LGTM!compareArraysimplementation is correct for string arrays.The function properly handles null/undefined cases and performs order-insensitive comparison. The explicit
string[]typing addresses the previous concern about object array limitations.
83-94: LGTM!compareAttributesnow includes all relevant fields.The
spontaneousClientIdcomparison (line 91) has been added as previously suggested.
96-115: LGTM! Comprehensive change detection logic.The
hasActualChangesfunction correctly compares all scalar fields directly and delegates array/object comparisons to the appropriate helper functions.admin-ui/plugins/auth-server/components/Scopes/constants.ts (1)
1-22: LGTM! Good extraction of shared constants.This addresses the previous review suggestion to extract
ScopeWithMessage,DEFAULT_SCOPE_ATTRIBUTES, andINITIAL_SCOPEto a shared location, eliminating duplication betweenScopeAddPage.tsxandScopeEditPage.tsx.admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx (2)
105-155: LGTM! Well-structured submit handler with proper error handling.The implementation correctly:
- Parses JSON data with error handling
- Extracts and removes
action_messagebefore mutation- Invalidates related queries after successful update
- Dispatches toast notification
- Logs audit (non-blocking)
- Includes
dispatchin the dependency array (addressing the past review comment)
157-174: Consider adding dependency tracking for initial fetch.The
useEffectfetches attributes and scripts when their lengths are 0, but includesattributes.lengthandscripts.lengthin the dependency array. This works but could refetch if these arrays are emptied and repopulated during the component lifecycle.Verify this is the intended behavior. If not, consider using a ref to track whether the initial fetch has occurred:
+ const hasFetchedRef = useRef(false) useEffect(() => { + if (hasFetchedRef.current) return + hasFetchedRef.current = true + if (attributes.length === 0) { // ... fetch attributes } if (scripts.length === 0) { // ... fetch scripts } - }, [dispatch, attributes.length, scripts.length]) + }, [dispatch, attributes.length, scripts.length])admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx (2)
44-94: LGTM! Well-structured submit handler matching the edit page pattern.The implementation correctly:
- Parses JSON data with error handling
- Extracts and removes
action_messagebefore mutation- Invalidates related queries after successful creation
- Dispatches toast notification with scope name/id
- Logs audit (non-blocking)
- Includes
dispatchin the dependency array (addressing the past review comment)
135-135: Good use of the sharedINITIAL_SCOPEconstant.This addresses the past review suggestion to use a centralized constant instead of constructing the scope object inline.
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (4)
126-136: Good cache settings addressing past review feedback.The
staleTime: 30000(30 seconds) is a reasonable choice that balances freshness with reduced API calls, addressing the previous concern about excessive refetching.
240-253: Good fix for the debounce race condition.Clearing the debounce timer when Enter is pressed (lines 243-246) prevents the race condition between the debounced search and the immediate Enter key handler that was flagged in the past review.
381-381: Good simplification ofDeleteIcon.Removing the unnecessary
useCallbackwrapper addresses the past review comment about overhead for a static component.
505-521: Good implementation of debounced search with cleanup.The debounce effect properly:
- Uses a ref to track the timer
- Clears the timer reference after execution (line 512)
- Cleans up on unmount/re-run
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx (1)
52-64: Aggressive cache settings may cause excessive API calls.The query options still use
staleTime: 0andgcTime: 0withrefetchOnWindowFocus: true, which was flagged in a previous review. This causes data to always be considered stale and garbage collected immediately, potentially triggering API calls on every tab switch.admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (1)
409-420: PaginationComponent pattern unchanged from past review.The
PaginationComponentstill uses theuseCallbackwrapper pattern that was flagged in a previous review. Consider the suggested refactor to useuseMemofor pagination props instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx(3 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx(3 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx(7 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.
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx
🧬 Code graph analysis (2)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (7)
admin-ui/plugins/auth-server/components/Scopes/types/index.ts (3)
ScopeTableRow(33-33)Scope(2-2)ScopeWithClients(11-11)admin-ui/plugins/auth-server/components/Scopes/types/componentTypes.ts (1)
ScopeTableRow(24-28)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts (1)
useScopeActions(26-106)admin-ui/plugins/auth-server/components/Scopes/types/scopeTypes.ts (2)
Scope(11-11)ScopeWithClients(40-40)admin-ui/app/components/Card/Card.tsx (1)
Card(27-27)
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx (3)
admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts (1)
useScopeActions(26-106)admin-ui/plugins/auth-server/components/Scopes/types/formTypes.ts (1)
ModifiedFields(17-19)admin-ui/plugins/auth-server/components/Scopes/constants.ts (2)
ScopeWithMessage(4-6)INITIAL_SCOPE(12-22)
🔇 Additional comments (17)
admin-ui/plugins/auth-server/components/Scopes/ScopeEditPage.tsx (4)
1-23: LGTM! Imports and module setup are well-organized.The imports are properly structured, bringing in necessary hooks from React, React Query, and the API layer. The constants are now imported from a shared
./constantsmodule, which addresses the duplication concern from previous reviews.
74-93: LGTM! Data handling and loading state are well-implemented.The
extensibleScopememo correctly handles the case whenscopeDatais undefined by falling back toEMPTY_SCOPEwith proper defaults. The loading state properly combines both the mutation pending state and the query loading state.
105-155: Well-structured submit handler with comprehensive error handling.The
handleSubmitcallback correctly:
- Parses and validates input data with proper error handling
- Extracts and removes
action_messagebefore the API call- Invalidates relevant queries to ensure fresh data in the list
- Shows a contextual success toast
- Logs audit information with isolated error handling
- Navigates back to the list on success
The dependency array now correctly includes
dispatch, addressing the previous review comment.
157-174: LGTM! Lazy loading of attributes and scripts.The effect correctly fetches attributes and scripts only when they haven't been loaded yet, preventing redundant API calls.
admin-ui/plugins/auth-server/components/Scopes/ScopeAddPage.tsx (4)
1-17: LGTM! Clean imports using shared constants.The file properly imports
ScopeWithMessageandINITIAL_SCOPEfrom the shared./constantsmodule, addressing the duplication concern from previous reviews.
44-94: Well-implemented submit handler that addresses the PR objectives.The
handleSubmitcallback correctly:
- Parses input with proper JSON error handling
- Creates the scope via
mutateAsync- Invalidates queries to refresh the list (fixes the "list not updating" issue)
- Shows success toast (restores missing toast notification)
- Logs audit information with isolated error handling
- Navigates back to the list
The dependency array now correctly includes
dispatch, addressing the previous review comment.
96-123: LGTM! Consistent data loading and search handling.The effect for loading attributes and scripts, and the search handler, follow the same pattern as
ScopeEditPage, maintaining consistency across the module.
125-146: LGTM! Clean render implementation.The component correctly uses
createScope.isPendingfor the loading state and passes the sharedINITIAL_SCOPEconstant to the form, ensuring consistent initial values.admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (9)
1-47: LGTM! Well-organized imports and type definitions.The imports are properly structured with clear separation between React utilities, UI components, routing, state management, and API hooks. The type imports from
@material-table/coreenable proper typing for columns and actions.
49-79: LGTM! Style constants properly extracted.Defining style constants outside the component as frozen objects (
as const) prevents recreation on each render and provides clear documentation of the UI structure.
113-136: Cache settings improved from previous review.The query options now use
staleTime: 30000(30 seconds) andrefetchOnWindowFocus: false, addressing the previous review concern about excessive API calls. This is a good balance between data freshness and performance.
173-210: LGTM! Proper memoization of derived state.Theme colors, permission checks, and data extraction are appropriately memoized to prevent unnecessary recalculations. The permission checks integrate correctly with the Cedar permission system.
240-253: Debounce race condition addressed.The
handlePatternKeyDownnow properly clears the debounce timer when Enter is pressed, preventing the race condition between debounced search and immediate Enter key submission that was flagged in a previous review.
381-388: DeleteIcon simplified as suggested.
DeleteIconis now a plain function instead of being wrapped inuseCallback, and it's correctly excluded from themyActionsdependency array. This addresses the previous review comments.
440-496: LGTM! Actions and table components properly memoized.The
myActionsarray correctly excludesDeleteIconfrom its dependency array as suggested in previous reviews. The actions are conditionally added based on permissions, and the table components are properly memoized.
504-520: LGTM! Debounce effect properly implemented.The debounce effect correctly:
- Uses a ref to track the timer
- Sets the timer reference to
nullafter execution- Properly cleans up the timer on unmount or when dependencies change
This addresses the race condition concern from previous reviews.
522-647: LGTM! Well-structured UI implementation.The render section properly:
- Wraps content with
GluuLoaderfor loading states- Implements accessible filter controls with proper
aria-labelattributes- Uses
GluuViewWrapperfor permission-based rendering- Configures
MaterialTablewith memoized components, columns, and actions- Conditionally renders the delete confirmation dialog
The improved state management ensures action buttons remain visible after navigation, addressing the PR objective.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (1)
146-154: Use translation for the success message.The success message "Scope deleted successfully" is hardcoded in English. For consistency with the rest of the codebase and i18n support, use the translation function.
const handleDeleteSuccess = useCallback(() => { - dispatch(updateToast(true, 'success', 'Scope deleted successfully')) + dispatch(updateToast(true, 'success', t('messages.scope_deleted_successfully'))) queryClient.invalidateQueries({Note: Ensure the translation key
messages.scope_deleted_successfullyexists in your i18n resources.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx(7 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.
📚 Learning: 2025-11-07T12:55:26.241Z
Learnt from: kdhttps
Repo: GluuFederation/flex PR: 2378
File: admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx:93-104
Timestamp: 2025-11-07T12:55:26.241Z
Learning: In the Cedarling configuration feature (admin-ui/plugins/admin/components/Cedarling/CedarlingConfigPage.tsx), the sync role to scopes mappings operation depends on the `auiPolicyStoreUrl`. The backend fetches content from this URL and syncs roles based on the response. Therefore, the audit log for the sync operation should include the `auiPolicyStoreUrl` in its payload to provide proper audit context.
Applied to files:
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx
🧬 Code graph analysis (1)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (4)
admin-ui/plugins/auth-server/components/Scopes/types/index.ts (3)
ScopeTableRow(33-33)Scope(2-2)ScopeWithClients(11-11)admin-ui/plugins/auth-server/components/Scopes/types/componentTypes.ts (1)
ScopeTableRow(24-28)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/auth-server/components/Scopes/hooks/useScopeActions.ts (1)
useScopeActions(26-106)
🔇 Additional comments (7)
admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx (7)
49-79: Well-structured style constants and type definitions.The extraction of style constants with
as constassertions and theDetailPanelPropsinterface improves type safety and maintainability. This is a good pattern for keeping styles DRY.
126-136: Cache settings have been improved.The
staleTime: 30000addresses the previous concern about aggressive cache settings. This provides a reasonable balance between data freshness and avoiding excessive API calls.
202-210: LGTM!The memoized derivations for
scopesandtotalItemsare correctly implemented with proper dependency arrays and safe fallbacks for undefined values.
240-253: Debounce race condition has been addressed.The
handlePatternKeyDownnow properly clears the debounce timer when Enter is pressed, preventing the race condition mentioned in past reviews. Good implementation.
440-488: Actions array is well-structured.The
myActionsmemoization correctly excludesDeleteIconfrom dependencies (addressing the past review comment). The conditional action building based on permissions is clean.
621-631: MaterialTable key may cause unnecessary remounts.Using
key={limit || 0}forces a full remount of MaterialTable whenlimitchanges. While this might be intentional to reset table state, it could cause performance issues and loss of UI state (like expanded detail panels).Consider whether this behavior is intentional. If the goal is to reset pagination state, this is already handled by the handlers. If full remount is not needed:
- <MaterialTable - key={limit || 0} + <MaterialTable
522-646: Implementation correctly addresses PR objectives.The changes properly implement:
- List refresh after create/edit via
queryClient.invalidateQueries()- Success toast notifications via
updateToast()dispatch- Action buttons remain visible through proper state management and memoization
syntrydy
left a comment
syntrydy
left a comment
There was a problem hiding this comment.
I have fetch the branch and test.
The cache invalidation is working as expected and the toast notifications also
5 participants
fix(admin-ui): resolve scope update issues, missing toasts, and disappearing action buttons (#2476)
Summary
The Scopes module is not functioning correctly after creating or editing a scope. Updated scopes are not reflected in the list, toast notifications are not shown, and action buttons disappear when returning to the Scope List page.
Issues Fixed
Steps to Reproduce
Expected Behavior
Fix Summary
Closes
Closes: #2476
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.