fix(admin-ui): Use typescript generated client for SCHEMA plugin #2396
fix(admin-ui): Use typescript generated client for SCHEMA plugin #2396
Conversation
syntrydy
added
comp-admin-ui
📝 WalkthroughWalkthroughReplace handwritten Redux/Saga attribute implementation and types with the TypeScript-generated JansConfigApi and React Query hooks; add audit and webhook hooks, form helpers and error utilities, translation keys, test updates for React Query, a WebhookSlice field rename, and a new GluuStatusMessage component. Changes
Sequence Diagram(s)sequenceDiagram
participant Add as AttributeAddPage
participant Mut as usePostAttributes
participant API as JansConfigApi
participant Cache as ReactQueryCache
participant Audit as useSchemaAuditLogger
participant Web as useSchemaWebhook
Add->>Mut: mutate(values)
Mut->>API: POST /attributes
API-->>Mut: success(data)
Mut->>Add: onSuccess(data)
Add->>Cache: invalidate(getAttributesQueryKey)
Add->>Audit: logAudit(CREATE, API_ATTRIBUTE, payload)
Add->>Web: triggerAttributeWebhook(data)
Add->>Add: navigate('/attributes') & showSuccessToast()
rect rgb(245,245,255)
Note over API,Add: Error path
API-->>Mut: error
Mut->>Add: onError(error)
Add->>Add: showErrorToast(getErrorMessage(error))
end
sequenceDiagram
participant Old as Redux+Saga
participant New as ReactQuery
rect rgb(255,235,235)
Old->>Old: dispatch(getAttributes)
Old->>Old: saga -> AttributeApi -> dispatch(response)
Old->>Component: component selects state
end
rect rgb(235,255,235)
New->>New: useGetAttributes(options) triggers fetch
New->>New: React Query caches response
New->>Component: component reads cached data
New->>New: mutation onSuccess -> invalidate queries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (2)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (5)
🔇 Additional comments (2)
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
admin-ui/app/locales/en/translation.json(6 hunks)admin-ui/app/locales/es/translation.json(3 hunks)admin-ui/app/locales/fr/translation.json(3 hunks)admin-ui/app/locales/pt/translation.json(3 hunks)admin-ui/plugins/saml/components/TrustRelationForm.js(3 hunks)admin-ui/plugins/schema/__tests__/api/Person.test.js(1 hunks)admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js(2 hunks)admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js(2 hunks)admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js(2 hunks)admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeForm.tsx(2 hunks)admin-ui/plugins/schema/components/Person/AttributeListPage.tsx(5 hunks)admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx(2 hunks)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts(1 hunks)admin-ui/plugins/schema/constants/index.ts(1 hunks)admin-ui/plugins/schema/helper/index.ts(1 hunks)admin-ui/plugins/schema/helper/utils.ts(1 hunks)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts(1 hunks)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts(1 hunks)admin-ui/plugins/schema/plugin-metadata.ts(2 hunks)admin-ui/plugins/schema/redux/api/AttributeApi.ts(0 hunks)admin-ui/plugins/schema/redux/features/attributeSlice.ts(0 hunks)admin-ui/plugins/schema/redux/sagas/AttributeSaga.ts(0 hunks)admin-ui/plugins/schema/redux/types/AttributeSagaTypes.ts(0 hunks)admin-ui/plugins/schema/types/AttributeApiTypes.ts(0 hunks)admin-ui/plugins/schema/types/index.ts(0 hunks)admin-ui/plugins/schema/types/shared.ts(1 hunks)admin-ui/plugins/schema/utils/errorHandler.ts(1 hunks)
💤 Files with no reviewable changes (6)
- admin-ui/plugins/schema/types/index.ts
- admin-ui/plugins/schema/redux/api/AttributeApi.ts
- admin-ui/plugins/schema/redux/types/AttributeSagaTypes.ts
- admin-ui/plugins/schema/redux/features/attributeSlice.ts
- admin-ui/plugins/schema/types/AttributeApiTypes.ts
- admin-ui/plugins/schema/redux/sagas/AttributeSaga.ts
🧰 Additional context used
🧬 Code graph analysis (9)
admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(18-48)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(10-25)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
SubmitData(10-13)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(5-5)
admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js (2)
admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js (3)
queryClient(50-56)Wrapper(58-64)store(43-48)admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js (3)
queryClient(46-52)Wrapper(54-60)store(38-44)
admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js (2)
admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js (3)
queryClient(35-41)Wrapper(43-49)store(28-33)admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js (3)
queryClient(50-56)Wrapper(58-64)store(43-48)
admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js (2)
admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js (3)
queryClient(35-41)Wrapper(43-49)store(28-33)admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js (3)
queryClient(46-52)Wrapper(54-60)store(38-44)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(18-48)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(10-25)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
SubmitData(10-13)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(5-5)
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
AttributeItem(53-73)
admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (6)
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
StyledBadgeProps(43-45)RootState(5-7)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(18-48)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(10-25)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(5-5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (2)
admin-ui/plugins/schema/types/shared.ts (1)
SchemaPluginRootState(8-24)admin-ui/app/utils/AuditLogger.ts (2)
logAudit(68-68)logAuditUserAction(25-66)
🔇 Additional comments (14)
admin-ui/plugins/schema/plugin-metadata.ts (2)
5-5: LGTM! Import path corrected.The double slash in the import path has been fixed, ensuring proper module resolution.
38-39: Redux migration for schema plugin is complete.The verification confirms that schema plugin Redux infrastructure (attributeReducer, attributeSaga) has been successfully removed. The remaining Redux usage in components is application-wide Redux for global concerns:
updateToast: Global toast notification system (admin-ui/app/redux/features/toastSlice)cedarPermissions: Global authorization state (admin-ui/app/redux/features/cedarPermissionsSlice)These are not part of the schema plugin Redux that was scoped for removal. The plugin now relies on React Query for data management while delegating global UI and authorization concerns to application-level Redux.
admin-ui/app/locales/fr/translation.json (1)
362-362: LGTM! French translations for sorting UI are correct.The French translations for the sorting controls have been properly added:
- "Trier Par" for sort_by
- "Tout", "Croissant", "Décroissant", and "Aucun" for the sorting options
These are accurate translations and appropriately placed in the JSON structure.
Also applies to: 914-915, 923-923, 934-934
admin-ui/plugins/schema/helper/index.ts (1)
1-1: LGTM! Standard barrel export pattern.This follows the common barrel export pattern to re-export utilities from the helper module, making them accessible to consumers via the index file.
admin-ui/plugins/schema/constants/index.ts (1)
1-5: LGTM! Clean constant definition.The
API_ATTRIBUTEconstant is well-defined with clear module documentation. This provides a centralized, reusable identifier for API attribute operations across the schema plugin.admin-ui/app/locales/pt/translation.json (1)
329-329: LGTM! Portuguese translations for sorting UI are correct.The Portuguese translations for the sorting controls have been properly added:
- "Ordenar Por" for sort_by
- "Todos", "Ascendente", "Descendente", and "Nenhum" for the sorting options
These are accurate translations and appropriately placed in the JSON structure, consistent with the other locale files.
Also applies to: 911-912, 920-920, 931-931
admin-ui/plugins/saml/components/TrustRelationForm.js (1)
51-59: Add loading and error state handling for the React Query hook to match codebase patterns.The
useGetAttributeshook returnsisLoadinganderrorstates that aren't being destructured here. While other files in the codebase (e.g.,AttributeListPage.tsx) explicitly handle these states, this component ignores them and relies solely on Redux loading. During the attributes fetch, users see an empty attributes list with no indication of loading status, making it indistinguishable from having no available attributes.Consider destructuring and displaying these states:
- const { data: attributesData } = useGetAttributes({ limit: 70 }) + const { data: attributesData, isLoading: attributesLoading, error: attributesError } = useGetAttributes({ limit: 70 })Then display appropriate feedback in the UI (e.g., spinner or disabled state during load, error message on failure).
admin-ui/app/locales/es/translation.json (1)
439-439: LGTM! Spanish translations added correctly.The new sorting and filtering translations (
sort_by,all,ascending,descending,none) are accurate and consistent with the UI requirements mentioned in the PR objectives.Also applies to: 1016-1017, 1025-1026
admin-ui/plugins/schema/types/shared.ts (1)
1-37: LGTM! Well-structured type definitions.The new interfaces provide clean type safety for the Schema plugin's auth state access. The optional fields and clear documentation make the contract explicit for consumers like the audit logger hook.
admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js (1)
5-5: LGTM! Test successfully migrated to React Query.The test updates properly support the component's migration from Redux to React Query:
- QueryClient setup with retry disabled prevents test flakiness
- JansConfigApi mocks provide deterministic behavior
- Wrapper correctly nests QueryClientProvider around Redux Provider
- Consistent with the migration pattern across other test files
Also applies to: 11-32, 50-62
admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js (1)
5-5: LGTM! Consistent React Query test migration.The test updates align perfectly with the AttributeEditPage test migration:
- Proper QueryClient configuration
- Appropriate JansConfigApi mocks for the add page mutation
- Clean removal of Redux-based attribute state
Also applies to: 10-17, 35-47
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
1-7: LGTM! Type definitions align with Redux removal.The changes correctly reflect the migration from Redux-based state to React Query:
JansAttributenow sourced from the TypeScript-generated client (achieving PR objective)RootStatesimplified to removeattributeReducer, keeping onlycedarPermissions- The "minimal, no Redux" comment accurately documents the reduced state surface
This is a breaking change that properly supports the broader architectural shift described in the PR.
admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js (1)
5-5: LGTM! Complete test migration with cedarPermissions support.The test updates are comprehensive and consistent:
- JansConfigApi mocks cover both listing and deletion operations
- Store configuration now includes
cedarPermissionsreducer (line 41), aligning with the updatedRootStateinterface- QueryClient and provider setup follows the established pattern
Also applies to: 11-27, 41-41, 46-58
admin-ui/plugins/schema/components/Person/AttributeForm.tsx (1)
62-91: Excellent refactor! Immutability improvements prevent mutation bugs.The changes eliminate direct mutation of the
itemobject and ensure safe, immutable updates:Key improvements:
- Line 63: Uses spread operator instead of
Object.assignto create a new object- Lines 66-70: Defensively ensures
attributeValidationexists and is cloned before modification- Lines 73, 76, 79: Updates nested properties via
result.attributeValidationinstead of top-level mutation- Lines 83-90: Correctly removes both nested and top-level validation properties when disabled
This pattern prevents unintended side effects and makes the data flow more predictable.
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
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/TrustRelationForm.js (1)
51-59: Add error and loading state handling for the attributes query.The
useGetAttributeshook is called without destructuringisLoadingorerrorstates. This can lead to several issues:
- Silent failures: If the query fails, the user receives no feedback and
attributesListremains empty, preventing attribute selection.- Race condition: If
configs.releasedAttributesexists butattributesDatahasn't loaded yet,defaultScopeValue(lines 61-65) will incorrectly resolve to an empty array, causing the form to lose pre-selected attributes.- Hardcoded limit: The limit of 70 may be insufficient for installations with many attributes.
Consider this approach:
// Fetch attributes using React Query - const { data: attributesData } = useGetAttributes({ limit: 70 }) + const { data: attributesData, isLoading: isLoadingAttributes, error: attributesError } = useGetAttributes({ limit: 200 }) + + if (attributesError) { + // Handle error appropriately (toast notification, error message, etc.) + console.error('Failed to fetch attributes:', attributesError) + } const attributesList = attributesData?.entries ? attributesData.entries.map((item) => ({ dn: item?.dn, name: item?.displayName, })) : []Additionally, consider:
- Passing
isLoadingAttributesto theGluuLoadercomponent (line 236) alongside the existingloadingstate- Increasing the limit or implementing pagination if the attribute count can exceed the limit
- Displaying a user-friendly error message if the query fails
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
admin-ui/app/locales/en/translation.json(6 hunks)admin-ui/app/locales/es/translation.json(3 hunks)admin-ui/app/locales/fr/translation.json(3 hunks)admin-ui/app/locales/pt/translation.json(3 hunks)admin-ui/plugins/saml/components/TrustRelationForm.js(3 hunks)admin-ui/plugins/schema/__tests__/api/Person.test.js(1 hunks)admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js(2 hunks)admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js(2 hunks)admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js(2 hunks)admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeForm.tsx(2 hunks)admin-ui/plugins/schema/components/Person/AttributeListPage.tsx(5 hunks)admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx(2 hunks)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts(1 hunks)admin-ui/plugins/schema/constants/index.ts(1 hunks)admin-ui/plugins/schema/helper/index.ts(1 hunks)admin-ui/plugins/schema/helper/utils.ts(1 hunks)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts(1 hunks)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts(1 hunks)admin-ui/plugins/schema/plugin-metadata.ts(2 hunks)admin-ui/plugins/schema/redux/api/AttributeApi.ts(0 hunks)admin-ui/plugins/schema/redux/features/attributeSlice.ts(0 hunks)admin-ui/plugins/schema/redux/sagas/AttributeSaga.ts(0 hunks)admin-ui/plugins/schema/redux/types/AttributeSagaTypes.ts(0 hunks)admin-ui/plugins/schema/types/AttributeApiTypes.ts(0 hunks)admin-ui/plugins/schema/types/index.ts(0 hunks)admin-ui/plugins/schema/types/shared.ts(1 hunks)admin-ui/plugins/schema/utils/errorHandler.ts(1 hunks)
💤 Files with no reviewable changes (6)
- admin-ui/plugins/schema/types/index.ts
- admin-ui/plugins/schema/redux/features/attributeSlice.ts
- admin-ui/plugins/schema/redux/api/AttributeApi.ts
- admin-ui/plugins/schema/redux/types/AttributeSagaTypes.ts
- admin-ui/plugins/schema/redux/sagas/AttributeSaga.ts
- admin-ui/plugins/schema/types/AttributeApiTypes.ts
🧰 Additional context used
🧬 Code graph analysis (9)
admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js (2)
admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js (3)
queryClient(35-41)Wrapper(43-49)store(28-33)admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js (3)
queryClient(50-56)Wrapper(58-64)store(43-48)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (2)
admin-ui/plugins/schema/types/shared.ts (1)
SchemaPluginRootState(8-24)admin-ui/app/utils/AuditLogger.ts (2)
logAudit(68-68)logAuditUserAction(25-66)
admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js (2)
admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js (3)
queryClient(50-56)Wrapper(58-64)store(43-48)admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js (3)
queryClient(46-52)Wrapper(54-60)store(38-44)
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (6)
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
StyledBadgeProps(43-45)RootState(5-7)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(18-48)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(10-25)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(5-5)
admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(18-48)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(10-25)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
SubmitData(10-13)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(5-5)
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
AttributeItem(53-73)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(18-48)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(10-25)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
SubmitData(10-13)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(5-5)
admin-ui/plugins/schema/__tests__/components/AttributeEditPage.test.js (2)
admin-ui/plugins/schema/__tests__/components/AttributeAddPage.test.js (3)
queryClient(35-41)Wrapper(43-49)store(28-33)admin-ui/plugins/schema/__tests__/components/AttributeListPage.test.js (3)
queryClient(46-52)Wrapper(54-60)store(38-44)
🔇 Additional comments (13)
admin-ui/plugins/schema/types/shared.ts (2)
8-24: All nested fields should be required or use stricter typing.The SchemaPluginRootState makes every field optional through all nesting levels, but the codebase shows that auth fields are expected to be present when authenticated. For example, in
admin-ui/plugins/fido/components/Fido.tsx, fields likestate.authReducer.token.access_tokenandstate.authReducer.config.clientIdare accessed directly without optional chaining. Similarly, Redux sagas throughout the codebase (e.g.,SagaUtils.ts,InitSaga.ts) directly accessstate.authReducer.token.access_tokenwithout defensive chaining.The deep optional structure requires verbose null-checking and inconsistently mirrors other Redux state definitions like
AuthRootStateandSmtpRootState, which define these fields as required. Consider aligningSchemaPluginRootStatewith the actual auth state structure or using utility types to enforce stricter guarantees for authenticated scenarios.
8-37: The review comment incorrectly assumes AuthState is used to transform data from SchemaPluginRootState.The code in
useSchemaAuditLogger.tsusesSchemaPluginRootStatedirectly (line 21) and accesses its nested structure via optional chaining without any flattening or transformation. TheAuthStateinterface defined inshared.tsis not imported or used in this file, so the structural differences between the two interfaces are irrelevant to this code. No transformation logic is needed or exists in the schema plugin.Likely an incorrect or invalid review comment.
admin-ui/plugins/schema/helper/index.ts (1)
1-1: LGTM! Clean barrel export pattern.The re-export from
./utilsprovides a clean public interface for the helper module.admin-ui/plugins/schema/constants/index.ts (1)
1-5: LGTM! Clean constant definition.The
API_ATTRIBUTEconstant provides a consistent identifier for API attribute operations throughout the Schema plugin.admin-ui/plugins/schema/plugin-metadata.ts (2)
5-5: LGTM! Import path corrected.The double slash typo has been fixed in the import path.
38-39: LGTM! Redux infrastructure successfully removed.The empty arrays for
reducersandsagascorrectly reflect the migration away from Redux state management to React Query hooks, as documented in the PR objectives.admin-ui/plugins/schema/helper/utils.ts (1)
7-33: LGTM! Clean transformation with proper defaults.The function correctly initializes all required fields with appropriate default values and preserves existing attribute data when provided.
admin-ui/app/locales/pt/translation.json (1)
329-329: LGTM! Portuguese translations are accurate.The new sorting and filtering UI translations are correctly translated for the Portuguese locale and consistent with similar additions in other locale files.
Also applies to: 911-913, 920-920, 931-931
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
1-7: LGTM! Type definitions correctly updated.The changes align with the PR objectives:
JansAttributenow imported from the TypeScript generated client (JansConfigApi)RootStatesimplified by removingattributeReducer, reflecting the migration from Redux to React Queryadmin-ui/app/locales/es/translation.json (1)
439-439: LGTM! Spanish translations are accurate.The new sorting and filtering UI translations are correctly translated for the Spanish locale and consistent with similar additions in other locale files.
Also applies to: 1016-1017, 1025-1025, 1036-1036
admin-ui/app/locales/fr/translation.json (1)
362-365: French translations look solid.Keys and phrasing align with existing convention; no further changes needed.
Also applies to: 914-935
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
18-47: Hook implementation looks correct.Selector usage and audit payload wiring are consistent with the shared state shape; memoization scope is appropriate.
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (1)
13-89: View page refactor looks good.Query wiring, error/loader handling, and read-only form rendering are coherent with the new client hooks.
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
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/TrustRelationForm.js (1)
54-62: Add error and loading state handling for attribute fetching.The
useGetAttributeshook likely returnserrorandisLoadingstates that are not being handled. Without this:
- Users won't see a loading indicator while attributes are being fetched
- Silent failures will result in an empty attribute dropdown with no error message
- Users may be confused why no attributes are available
Apply this diff to add proper state handling:
- // Fetch attributes using React Query - const { data: attributesData } = useGetAttributes({ limit: MAX_ATTRIBUTES_FOR_TRUST_RELATION }) + // Fetch attributes using React Query + const { + data: attributesData, + error: attributesError, + isLoading: isLoadingAttributes + } = useGetAttributes({ limit: MAX_ATTRIBUTES_FOR_TRUST_RELATION })Then display the error state in the UI near the released attributes field (around line 318-334) or show a loading state. For example, you could add an error message before the
GluuTypeAheadForDncomponent:{attributesError && ( <Col sm={10}> <div style={{ color: customColors.accentRed }}> {t('messages.failed_to_load_attributes')} </div> </Col> )}Also consider disabling the attribute field or showing a loading state when
isLoadingAttributesis true.
♻️ Duplicate comments (5)
admin-ui/plugins/schema/helper/utils.ts (1)
51-56: Use destructuring instead of delete operations.While the deep cloning at lines 47-48 correctly prevents mutation, the
deleteoperations are less idiomatic and don't handle the empty object case. If all validation fields are removed,attributeValidationshould be set toundefinedrather than an empty object.Apply this diff to use destructuring and properly handle the empty object:
if (!validation && result.attributeValidation) { // Remove validation fields if validation is disabled - delete result.attributeValidation.regexp - delete result.attributeValidation.maxLength - delete result.attributeValidation.minLength + const { regexp, maxLength, minLength, ...rest } = result.attributeValidation + result.attributeValidation = Object.keys(rest).length > 0 ? rest : undefined }This approach is cleaner, more type-safe, and ensures that an empty
attributeValidationobject becomesundefined.admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
13-22: LGTM! Type safety concern from past reviews has been resolved.The hook correctly dispatches the
triggerWebhookaction with a properly typed payload. The defensive type casting mentioned in previous reviews has been removed, and the Redux action now properly accepts the payload (as confirmed by the updatedWebhookSlice.jsat line 110).admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (3)
50-57: LGTM! Badge color logic is now case-insensitive.The styled component correctly normalizes the status with
toLowerCase()(line 53) before comparison, which resolves the case sensitivity issue flagged in previous reviews. Defining the component outside the function body also prevents hook ordering issues.
151-157: LGTM! Status filter now correctly uses API-expected casing.The handler properly converts status values to uppercase (line 154) before storing in state, which resolves the API compatibility issue flagged in previous reviews. The explanatory comment is helpful.
191-203: Fix navigation paths - remove literal colons.Both navigation handlers build URLs with literal colons (
:${row.inum}), producing paths like/attribute/edit/:123. React Router expects the colon only in route patterns, not in concrete URLs. These links will fail to match the registered routes, making edit and view pages unreachable.Apply this diff to fix the navigation paths:
const handleGoToAttributeEditPage = useCallback( (row: JansAttribute): void => { - navigate(`/attribute/edit/:${row.inum}`) + navigate(`/attribute/edit/${row.inum}`) }, [navigate], ) const handleGoToAttributeViewPage = useCallback( (row: JansAttribute): void => { - navigate(`/attribute/view/:${row.inum}`) + navigate(`/attribute/view/${row.inum}`) }, [navigate], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
admin-ui/plugins/admin/redux/features/WebhookSlice.js(1 hunks)admin-ui/plugins/saml/components/TrustRelationForm.js(3 hunks)admin-ui/plugins/schema/__tests__/api/Person.test.js(0 hunks)admin-ui/plugins/schema/components/Person/AttributeListPage.tsx(5 hunks)admin-ui/plugins/schema/helper/utils.ts(1 hunks)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts(1 hunks)
💤 Files with no reviewable changes (1)
- admin-ui/plugins/schema/tests/api/Person.test.js
🧰 Additional context used
🧬 Code graph analysis (2)
admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (6)
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
StyledBadgeProps(43-45)RootState(5-7)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(18-48)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(10-25)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(28-37)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(5-5)
🔇 Additional comments (7)
admin-ui/plugins/schema/helper/utils.ts (1)
8-37: Deep cloning successfully prevents cache mutation.The
transformToFormValuesfunction now properly clonesattributeValidation(lines 29-30), addressing the previous concern about mutating cached query data. The shallow copy using the spread operator is sufficient sinceattributeValidationcontains only primitive fields.admin-ui/plugins/saml/components/TrustRelationForm.js (2)
27-27: LGTM: Migration to React Query hook.The replacement of Redux-based attribute fetching with the
useGetAttributeshook from JansConfigApi aligns with the PR objectives and modernizes the data fetching approach.
30-31: No action required—the limit is justified and consistent with the codebase.The constant name and documentation comment address the previous review feedback. The value of 100 is not arbitrary; it's the standard limit used throughout the application for similar attribute-fetching operations in UserList, UserEditPage, ScopeAddPage, and SessionListPage components. The change aligns with established patterns rather than being an unexplained increase.
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (4)
72-75: Good optimization with useMemo for localStorage access.Caching the page size prevents redundant localStorage reads on every render.
325-341: Good use of memoization for columns definition.Prevents unnecessary re-creation of the columns array on every render. The status normalization (line 335) is also correctly implemented.
344-358: Good use of memoization for table options.Prevents unnecessary re-creation with appropriate dependencies on
limitandbgThemeColor.
364-470: Well-implemented filter and search UI.The filter controls provide a good user experience with search, status filtering, sorting, and cache invalidation. The status display correctly converts to lowercase (line 410) for the UI while the state maintains uppercase for API compatibility. The refresh button properly invalidates the React Query cache (line 461).
There was a problem hiding this comment.
Actionable comments posted: 2
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/TrustRelationForm.js (1)
63-67: Race condition:defaultScopeValuecomputed before attributes load.The
defaultScopeValuefiltersattributesList, which depends on the asynchronoususeGetAttributeshook. During initial render,attributesListwill be empty, causingdefaultScopeValueto be empty even whenconfigs?.releasedAttributescontains values. This means released attributes won't be pre-selected when editing an existing trust relationship.Consider computing
defaultScopeValueinside auseMemothat depends on bothattributesListandconfigs?.releasedAttributes:const defaultScopeValue = useMemo(() => { if (!configs?.releasedAttributes?.length || !attributesList?.length) { return [] } return attributesList .filter((item) => configs.releasedAttributes.includes(item.dn)) .map((item) => ({ dn: item.dn, name: item.name })) }, [attributesList, configs?.releasedAttributes])You'll also need to import
useMemofrom React at the top of the file.
♻️ Duplicate comments (3)
admin-ui/plugins/saml/components/TrustRelationForm.js (1)
221-221: Remove redundant comment.This comment was previously flagged as redundant. The automatic fetching behavior is already clear from the
useGetAttributeshook usage at line 54.admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (1)
38-38: Trim only the leading colon fromgid.
replace(':', '')removes the first colon anywhere in the string, so legitimate inums containing colons are mangled and the wrong attribute is fetched. Strip only a prefixed colon.- const inum = gid?.replace(':', '') || '' + const inum = gid?.replace(/^:/, '') || ''admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (1)
189-199: Drop the colon when building edit/view URLs.These paths render as
/attribute/edit/:123, so the configured routes never match and both pages stay unreachable. Interpolate the raw inum instead.- navigate(`/attribute/edit/:${row.inum}`) + navigate(`/attribute/edit/${row.inum}`) @@ - navigate(`/attribute/view/:${row.inum}`) + navigate(`/attribute/view/${row.inum}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
admin-ui/plugins/admin/redux/features/WebhookSlice.js(3 hunks)admin-ui/plugins/saml/components/TrustRelationForm.js(3 hunks)admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeForm.tsx(2 hunks)admin-ui/plugins/schema/components/Person/AttributeListPage.tsx(5 hunks)admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx(2 hunks)admin-ui/plugins/schema/constants/index.ts(1 hunks)admin-ui/plugins/schema/helper/utils.ts(1 hunks)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts(1 hunks)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts(1 hunks)admin-ui/plugins/schema/types/shared.ts(1 hunks)admin-ui/plugins/schema/utils/errorHandler.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(14-43)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
SubmitData(10-13)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (7)
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
StyledBadgeProps(43-45)RootState(5-7)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(14-43)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
AttributeItem(53-73)
admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(14-43)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
SubmitData(10-13)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (2)
admin-ui/plugins/schema/types/shared.ts (1)
SchemaPluginRootState(1-17)admin-ui/app/utils/AuditLogger.ts (2)
logAudit(68-68)logAuditUserAction(25-66)
admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
admin-ui/plugins/admin/redux/sagas/WebhookSaga.ts (1)
triggerWebhook(279-368)
🔇 Additional comments (15)
admin-ui/plugins/saml/components/TrustRelationForm.js (2)
56-61: LGTM: Data transformation is safe and correct.The optional chaining and fallback to an empty array prevent runtime errors, and the mapping to
{ dn, name }format usingdisplayNameis appropriate.Note: The usefulness of this transformation depends on proper error and loading state handling (see comment on line 54).
30-31: Verification complete: Named constant for attribute limit is appropriate.The introduction of
MAX_ATTRIBUTES_FOR_TRUST_RELATIONas a named constant is good practice and addresses the previous concern about magic numbers. The limit of 100 is verified as appropriate—it aligns with Config API's documented limit parameter and remains well below the maxCount setting default of 200. The increase from 70 to 100 is intentional and consistent with other components in the codebase (e.g., ScopeAddPage).admin-ui/plugins/schema/constants/index.ts (1)
1-1: LGTM!The constant is clearly defined and properly exported.
admin-ui/plugins/admin/redux/features/WebhookSlice.js (3)
22-25: Typo fix applied correctly.The state property has been correctly renamed from
tiggerPayloadtotriggerPayloadin the initial state.
110-113: Reducer correctly updated.The
triggerWebhookreducer now properly accepts the payload and assigns it tostate.triggerPayload.
121-123: Payload assignment updated correctly.The
setTriggerPayloadreducer now writes to the correctly namedstate.triggerPayloadproperty.admin-ui/plugins/schema/helper/utils.ts (2)
3-31: Well-implemented transformation utility.The function properly initializes default values and deep-clones
attributeValidationto prevent mutations of the source data.
33-46: Mutation issue resolved correctly.The function now creates a deep copy of
attributeValidationbefore performing delete operations, preventing mutations to the original data. The conditional deletion of validation fields whenvalidationis false is correct.admin-ui/plugins/schema/types/shared.ts (1)
1-27: Clean type definitions.Both interfaces are well-structured with appropriate optional properties. The
SchemaPluginRootStateaccurately represents the Redux state shape, andAuthStateprovides a convenient flattened interface for consuming components.admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
14-43: Well-implemented audit logging hook.The hook properly extracts auth state, handles optional values defensively, and memoizes the callback appropriately. Error handling with console logging is reasonable for an audit utility.
admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
6-21: Clean webhook hook implementation.The type casting issues from previous reviews have been resolved. The hook now uses
Partial<JansAttribute>appropriately and dispatches the webhook action with proper typing.admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
29-42: Good use of memoization for data normalization.The
useMemohook properly normalizes the attribute by ensuringattributeValidationis always initialized. The deep clone prevents mutations to the cached query data.
46-55: Proper error handling.Error state is handled cleanly with a localized message using the
getErrorMessageutility.admin-ui/plugins/schema/utils/errorHandler.ts (2)
1-12: Appropriate type guard for error handling.The
ApiErrorinterface andisApiErrortype guard provide flexible error detection for unknown errors. While the type guard is permissive (accepting any object withresponseormessage), this is appropriate for defensive error handling with unknown types.
14-30: Well-designed error message extraction utilities.Both functions follow a consistent pattern with proper fallback chains using optional chaining and nullish coalescing. The separation between i18n-aware (
getErrorMessage) and plain (extractErrorMessage) versions provides flexibility for different use cases.
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
admin-ui/plugins/saml/components/TrustRelationForm.js (1)
225-225: Redundant comment persists despite previous review.This comment was already flagged in a previous review (marked as addressed in commit 0cdd9ae) as redundant since the
useGetAttributeshook usage at lines 54-58 makes the auto-fetch behavior self-evident. Consider removing it to reduce code noise.admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (1)
38-38: Prevent inum corruption by trimming only the leading colon
replace(':', '')drops the first colon anywhere ingid, so values likeinum:bc50c4c4-64c8-45b3-9f38-e8527ddbf833lose the namespace colon and we request the wrong record. Jans Config APIs expect those embedded colons (--url-suffix inum:bc50c4c4-64c8-45b3-9f38-e8527ddbf833). Trim only an optional leading colon instead. (docs.jans.io)- const inum = gid?.replace(':', '') || '' + const inum = gid?.replace(/^:/, '') || ''
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
admin-ui/app/locales/en/translation.json(6 hunks)admin-ui/app/locales/es/translation.json(4 hunks)admin-ui/app/locales/fr/translation.json(4 hunks)admin-ui/app/locales/pt/translation.json(4 hunks)admin-ui/plugins/saml/components/TrustRelationForm.js(4 hunks)admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(14-43)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
SubmitData(10-13)AttributeItem(53-73)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/plugins/saml/components/TrustRelationForm.js (1)
admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
AttributeItem(53-73)
🔇 Additional comments (10)
admin-ui/app/locales/pt/translation.json (1)
329-329: Localization additions for SCHEMA plugin look good.The new translation keys (
sort_by,loading_attributes, and options forall,ascending,descending,none) are correctly placed in their respective sections (fields, messages, options) and use appropriate Portuguese translations. The structure is consistent with the existing file format.Also applies to: 736-736, 912-932
admin-ui/app/locales/es/translation.json (2)
439-439: Localization additions for SCHEMA plugin look good.The new translation keys (
sort_by,loading_attributes, and options forall,ascending,descending,none) are correctly placed in their respective sections (fields, messages, options) with appropriate Spanish translations that match the Portuguese equivalents structurally. JSON formatting is valid.Also applies to: 802-802, 1017-1037
439-439: All locale files have been updated consistently.Verification confirms that all supported locale files (English, Spanish, French, and Portuguese) contain the required translation keys:
sort_by,loading_attributes, and locale-appropriateallvariants. No missing translations detected.admin-ui/app/locales/en/translation.json (2)
802-803: Inconsistency: singular vs. pluralloading_attributekey.Line 802 introduces
loading_attribute(singular), but line 803 introducesloading_attributes(plural). Verify whether the singular form is intentional or should be removed. The French locale only has the plural variant, suggesting this may be a typo.
1030-1031: Approve new sorting and filtering option keys.The addition of
all,ascending,descending, andnoneto theoptionssection aligns well with the PR's goal of refactoring attribute UI flows with improved sorting and filtering capabilities. These entries are consistent with corresponding French translations and follow established naming conventions.Also applies to: 1039-1039, 1050-1050
admin-ui/plugins/saml/components/TrustRelationForm.js (5)
27-27: LGTM! Import migration aligns with PR objectives.The switch from Redux slice to the React Query hook from the generated TypeScript client is consistent with the goal of replacing manually created types with the TypeScript-generated client.
30-31: Clarify the rationale for increasing the limit from 70 to 100.While extracting the magic number into a named constant addresses the previous review feedback, the limit has been increased from 70 to 100 without explanation. Please document why 100 attributes is the appropriate limit for trust relationship configuration, or confirm that this change was intentional.
54-58: Excellent! All states properly handled.The hook correctly destructures
data,error, andisLoadingstates, fully addressing the previous review feedback about handling loading and error conditions.
60-65: LGTM! Safe data transformation with proper fallbacks.The optional chaining and empty array fallback prevent crashes when data is unavailable. The mapping from
displayNametonamemaintains compatibility with the existing component expectations.
330-330: LGTM! Proper disabled state logic.Disabling the attributes field during loading prevents user confusion and interaction with incomplete data.
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
admin-ui/plugins/saml/components/TrustRelationForm.js (1)
225-225: Redundant comment should be removed.This comment was previously flagged as unnecessary since the behavior is self-evident from the
useGetAttributeshook usage. Although marked as addressed in commit 0cdd9ae, it remains in the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
admin-ui/app/locales/fr/translation.json(6 hunks)admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx(1 hunks)admin-ui/plugins/saml/components/TrustRelationForm.js(5 hunks)admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx (1)
admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
AttributeItem(53-73)
🔇 Additional comments (9)
admin-ui/app/locales/fr/translation.json (1)
362-362: Localization additions are well-structured and appropriately placed.All new French translation keys follow consistent naming conventions and are placed in semantically correct sections:
- Sorting/filtering UI terms in
tooltipsandoptionssections- Attribute operation messages in
messagesand newerrorssections- Field labels in
fieldssectionThe translations appear contextually appropriate and maintain consistency with existing French terminology in the file.
Also applies to: 741-741, 794-797, 924-925, 933-933, 944-944
admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx (1)
14-57: Well-structured component that addresses code duplication.The implementation is clean and reusable, with proper TypeScript typing, sensible defaults, and flexible rendering modes (inline vs. wrapped). This component successfully extracts the duplicated status message patterns identified in the previous review of TrustRelationForm.js.
admin-ui/plugins/saml/components/TrustRelationForm.js (7)
27-28: LGTM: Clean migration to React Query and new UI component.The imports correctly bring in the TypeScript-generated client hook and the new reusable status message component.
30-31: Verify the increased attribute limit.The constant appropriately addresses the previous feedback about documenting the hardcoded limit. However, the value was increased from 70 (mentioned in earlier reviews) to 100 without explanation. Please verify that:
- 100 attributes is sufficient for typical trust relationship configurations
- The backend/API can handle this limit without performance degradation
- This aligns with any product or architectural constraints
54-58: LGTM: Proper React Query hook usage.The hook correctly destructures all necessary states (data, error, isLoading) for comprehensive UI feedback, addressing previous review feedback.
60-65: LGTM: Safe data transformation with appropriate fallback.The optional chaining and fallback to an empty array ensure the component handles loading and error states gracefully without crashes.
330-330: LGTM: Proper disabled state during loading.Correctly disables the attribute selector while data is fetching, preventing user interaction and potential race conditions.
337-352: LGTM: Clean UI feedback implementation.The loading and error states provide excellent user experience by using the newly extracted
GluuStatusMessagecomponent, successfully addressing the previous review feedback about duplicated inline styles. The layout parameters maintain consistency with surrounding form fields.Note: Ensure the import issue in
GluuStatusMessage.tsx(line 4) is fixed for this to work correctly.
393-397: LGTM: Appropriate inline usage for file error.Correctly uses the
inlineprop to render the error message without the Row/Col wrapper, which is appropriate for this nested context.
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx
Outdated
Show resolved
Hide resolved
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
- In User Claims audit logs, on saving the change, is it a good idea to show only the modified-fields in the payload field?
- The
Usage Typefield is not saving in User Claims add/edit form.
There was a problem hiding this comment.
Actionable comments posted: 6
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/locales/pt/translation.json (1)
768-799: Add missing tooltip entries for sorting to maintain parity with French translations.The Portuguese translation file is missing tooltip entries for sorting functionality that are present in the French file (lines 796-798). These entries are needed to support the new sorting UI:
sort_ascendingsort_descendingclear_filtersAdd the following entries to the Portuguese
tooltipssection to maintain consistency across all supported locales:"followingPermissionRequiredToBeAdded": "Precisamos adicionar as seguintes permissões essenciais à função para garantir que os usuários com essa função possam acessar a interface do usuário do administrador." + "sort_ascending": "Classificar em ordem crescente", + "sort_descending": "Classificar em ordem decrescente", + "clear_filters": "Limpar todos os filtros" },
♻️ Duplicate comments (1)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (1)
38-38: Don't strip the actual inum content
Routes hand you the literal inum (inum:…). Replacing the first colon turns it intoinum…, souseGetAttributesByInumasks the API for a non-existent identifier and the edit page breaks for any attribute whose inum contains a colon. This was raised in the earlier review and is still unresolved—please preserve the identifier and only trim the leading parameter delimiter if it exists.Apply this diff to keep the embedded delimiter intact:
- const inum = gid?.replace(':', '') || '' + const inum = gid?.replace(/^:/, '') || ''
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
admin-ui/app/locales/en/translation.json(6 hunks)admin-ui/app/locales/es/translation.json(5 hunks)admin-ui/app/locales/fr/translation.json(6 hunks)admin-ui/app/locales/pt/translation.json(5 hunks)admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx(2 hunks)admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeForm.tsx(5 hunks)admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx(2 hunks)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts(2 hunks)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (2)
admin-ui/plugins/schema/types/shared.ts (1)
SchemaPluginRootState(1-17)admin-ui/app/utils/AuditLogger.ts (2)
logAudit(68-68)logAuditUserAction(25-66)
admin-ui/plugins/schema/components/Person/AttributeAddPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(19-52)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
SubmitData(10-18)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx (1)
admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(19-52)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
SubmitData(10-18)AttributeItem(58-78)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
AttributeItem(58-78)
admin-ui/plugins/schema/components/Person/AttributeForm.tsx (1)
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (3)
AttributeFormValues(80-99)HandleAttributeSubmitParams(115-128)AttributeItem(58-78)
🔇 Additional comments (4)
admin-ui/app/locales/fr/translation.json (2)
925-945: Sorting options properly localized across all locales.The sorting options (
all,ascending,descending,none) have been added to the options section in French, maintaining consistency with Portuguese and English translations. The translations are appropriate and follow established French localization patterns.
766-772: The attribute error translation keys are not used in the codebase.The five new error keys (
attribute_create_failed,attribute_update_failed,attribute_delete_failed,attribute_load_failed,attribute_not_found) have been added to the French translation file, but there is no corresponding implementation:
- No create, update, delete, or load methods exist in
AttributeApi.ts- No saga functions handle these operations in
AttributesSaga.ts- No error state management or error handlers reference these keys
- Current implementation only supports fetching/reading attributes
Verify whether this is intentional (pre-adding translations for future feature development) or if the implementation is incomplete and these operations need to be added.
admin-ui/app/locales/es/translation.json (1)
835-856: ****The review comment references non-existent changes. The actual git diff shows:
- English change: Adds only
"attribute_not_found": "Attribute not found"to theerrorssection- Spanish change: Adds the complete
errorssection with proper Spanish translations, including"attribute_not_found": "Atributo no encontrado"The review claims missing keys (
sort_ascending,sort_descending,clear_filters) and shows a fabricated diff withadd_user,edit_user,delete_user—none of which appear in the actual PR changes. The Spanish locale already maintains parity with English for the changes being made. This review should be disregarded.Likely an incorrect or invalid review comment.
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
13-17: LGTM! Well-structured audit metadata fields.The addition of optional
modifiedFieldsandperformedOnfields appropriately supports audit logging functionality. The types are flexible and won't break existing usage.
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts
Outdated
Show resolved
Hide resolved
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (1)
188-200: Remove colon from navigation paths.Line 190 and Line 198 build URLs like
/attribute/edit/:123, which React Router treats as literal strings, so the edit/view pages never match and navigation breaks. Interpolate theinumwithout the colon.- navigate(`/attribute/edit/:${row.inum}`) + navigate(`/attribute/edit/${row.inum}`) ... - navigate(`/attribute/view/:${row.inum}`) + navigate(`/attribute/view/${row.inum}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx(1 hunks)admin-ui/plugins/schema/components/Person/AttributeListPage.tsx(5 hunks)admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx(2 hunks)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
admin-ui/plugins/schema/components/Person/AttributeListPage.tsx (7)
admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
StyledBadgeProps(48-50)AttributeListPageState(5-7)admin-ui/app/customColors.ts (1)
customColors(4-25)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(19-52)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/user-management/types/CommonTypes.ts (1)
ThemeContext(13-17)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx (1)
admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (2)
admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (1)
AttributeItem(58-78)
🔇 Additional comments (6)
admin-ui/app/routes/Apps/Gluu/GluuStatusMessage.tsx (5)
1-4: LGTM! Imports are correct.The named import for
customColorsis correct and matches the export from@/customColors. All other imports are standard React and component imports.
6-12: LGTM! Well-defined interface.The interface clearly defines required and optional props with appropriate types. The type union for message types provides good type safety.
14-26: LGTM! Function correctly placed outside component.The
getColorForTypefunction is appropriately defined at module scope, which prevents recreation on every render. The color mapping logic is clear and includes a sensible default.
49-60: LGTM! Message content rendering is correct.The message content correctly applies color styling and ARIA attributes. The implementation properly spreads the ARIA attributes and renders the message.
66-71: Verify the empty label is intentional for layout spacing.The non-inline mode renders a
GluuLabelwith an empty string. This appears to be for grid layout alignment, but please confirm this is the intended pattern for maintaining consistent spacing in your form layouts.admin-ui/plugins/schema/components/Person/AttributeViewPage.tsx (1)
54-68: Clear state handling looks solid.
Splitting the loader from the missing-attribute branch removes the previous ambiguity and gives users accurate feedback in each scenario. Nicely tightened up.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (1)
38-38: Strip only the leading colon fromgid.
replace(':', '')removes the first colon anywhere in the string. If an attribute inum legitimately contains a colon (common in Jans identifiers), this will corrupt the identifier. Since the route parameter is prefixed with a colon, strip only that leading character.Apply this diff:
- const inum = gid?.replace(':', '') || '' + const inum = gid?.replace(/^:/, '') || ''
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx(1 hunks)admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (5)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (1)
useSchemaAuditLogger(20-57)admin-ui/plugins/schema/hooks/useSchemaWebhook.ts (1)
useSchemaWebhook(6-21)admin-ui/plugins/schema/utils/errorHandler.ts (1)
getErrorMessage(14-23)admin-ui/plugins/schema/components/types/AttributeListPage.types.ts (2)
SubmitData(10-18)AttributeItem(58-78)admin-ui/plugins/schema/constants/index.ts (1)
API_ATTRIBUTE(1-1)
admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts (2)
admin-ui/plugins/schema/types/shared.ts (1)
SchemaPluginRootState(1-17)admin-ui/app/utils/AuditLogger.ts (2)
logAudit(68-68)logAuditUserAction(25-66)
⏰ 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: Analyze (javascript)
🔇 Additional comments (2)
admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx (2)
65-78: Well-structured defensive initialization.The
useMemocorrectly deep-clones the attribute and ensuresattributeValidationexists with safe defaults, preventing undefined access errors in the form.
103-112: Good error state handling.The error card renders a localized message using the
getErrorMessageutility and provides clear feedback when attribute loading fails.
| onSuccess: () => { | ||
| dispatch(updateToast(true, 'success')) | ||
| queryClient.invalidateQueries({ queryKey: getGetAttributesQueryKey() }) | ||
| queryClient.invalidateQueries({ queryKey: getGetAttributesByInumQueryKey(inum) }) | ||
| navigate('/attributes') | ||
| }, | ||
| onError: (error: unknown) => { | ||
| const errorMessage = getErrorMessage(error, 'errors.attribute_update_failed', t) | ||
| dispatch(updateToast(true, 'error', errorMessage)) | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| const extensibleItems = useMemo(() => { | ||
| if (!attribute) return null | ||
| const cloned = cloneDeep(attribute) as JansAttribute | ||
|
|
||
| if (!cloned.attributeValidation) { | ||
| cloned.attributeValidation = { | ||
| maxLength: undefined, | ||
| regexp: undefined, | ||
| minLength: undefined, | ||
| } | ||
| } | ||
|
|
||
| return cloned | ||
| }, [attribute]) | ||
|
|
||
| const customHandleSubmit = useCallback( | ||
| ({ data, userMessage, modifiedFields, performedOn }: SubmitData): void => { | ||
| if (data) { | ||
| putAttributeMutation.mutate( | ||
| { data: data as JansAttribute }, | ||
| { | ||
| onSuccess: (updatedAttribute: JansAttribute) => { | ||
| logAudit({ | ||
| action: UPDATE, | ||
| resource: API_ATTRIBUTE, | ||
| message: userMessage || '', | ||
| modifiedFields, | ||
| }) | ||
|
|
||
| triggerAttributeWebhook(updatedAttribute) | ||
| }, |
There was a problem hiding this comment.
Ensure audit and webhook complete before navigation.
The mutation's onSuccess handler (line 52) navigates away at line 56, but the mutate's onSuccess callback (lines 86-95) logs the audit and triggers the webhook after navigation. Because logAudit is async, the navigation may complete before the audit record is written, risking lost audit trails on slow networks or if the user closes the tab.
Refactor so audit and webhook fire before navigation:
const putAttributeMutation = usePutAttributes({
mutation: {
- onSuccess: () => {
- dispatch(updateToast(true, 'success'))
- queryClient.invalidateQueries({ queryKey: getGetAttributesQueryKey() })
- queryClient.invalidateQueries({ queryKey: getGetAttributesByInumQueryKey(inum) })
- navigate('/attributes')
- },
+ onSuccess: () => {
+ dispatch(updateToast(true, 'success'))
+ queryClient.invalidateQueries({ queryKey: getGetAttributesQueryKey() })
+ queryClient.invalidateQueries({ queryKey: getGetAttributesByInumQueryKey(inum) })
+ },
onError: (error: unknown) => {
const errorMessage = getErrorMessage(error, 'errors.attribute_update_failed', t)
dispatch(updateToast(true, 'error', errorMessage))
},
},
})
const customHandleSubmit = useCallback(
({ data, userMessage, modifiedFields, performedOn }: SubmitData): void => {
if (data) {
putAttributeMutation.mutate(
{ data: data as JansAttribute },
{
- onSuccess: (updatedAttribute: JansAttribute) => {
+ onSuccess: async (updatedAttribute: JansAttribute) => {
- logAudit({
+ await logAudit({
action: UPDATE,
resource: API_ATTRIBUTE,
message: userMessage || '',
modifiedFields,
})
triggerAttributeWebhook(updatedAttribute)
+ navigate('/attributes')
},
},
)
}
},
- [putAttributeMutation, logAudit, triggerAttributeWebhook],
+ [putAttributeMutation, logAudit, triggerAttributeWebhook, navigate],
)📝 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.
| onSuccess: () => { | |
| dispatch(updateToast(true, 'success')) | |
| queryClient.invalidateQueries({ queryKey: getGetAttributesQueryKey() }) | |
| queryClient.invalidateQueries({ queryKey: getGetAttributesByInumQueryKey(inum) }) | |
| navigate('/attributes') | |
| }, | |
| onError: (error: unknown) => { | |
| const errorMessage = getErrorMessage(error, 'errors.attribute_update_failed', t) | |
| dispatch(updateToast(true, 'error', errorMessage)) | |
| }, | |
| }, | |
| }) | |
| const extensibleItems = useMemo(() => { | |
| if (!attribute) return null | |
| const cloned = cloneDeep(attribute) as JansAttribute | |
| if (!cloned.attributeValidation) { | |
| cloned.attributeValidation = { | |
| maxLength: undefined, | |
| regexp: undefined, | |
| minLength: undefined, | |
| } | |
| } | |
| return cloned | |
| }, [attribute]) | |
| const customHandleSubmit = useCallback( | |
| ({ data, userMessage, modifiedFields, performedOn }: SubmitData): void => { | |
| if (data) { | |
| putAttributeMutation.mutate( | |
| { data: data as JansAttribute }, | |
| { | |
| onSuccess: (updatedAttribute: JansAttribute) => { | |
| logAudit({ | |
| action: UPDATE, | |
| resource: API_ATTRIBUTE, | |
| message: userMessage || '', | |
| modifiedFields, | |
| }) | |
| triggerAttributeWebhook(updatedAttribute) | |
| }, | |
| onSuccess: () => { | |
| dispatch(updateToast(true, 'success')) | |
| queryClient.invalidateQueries({ queryKey: getGetAttributesQueryKey() }) | |
| queryClient.invalidateQueries({ queryKey: getGetAttributesByInumQueryKey(inum) }) | |
| }, | |
| onError: (error: unknown) => { | |
| const errorMessage = getErrorMessage(error, 'errors.attribute_update_failed', t) | |
| dispatch(updateToast(true, 'error', errorMessage)) | |
| }, | |
| }, | |
| }) | |
| const extensibleItems = useMemo(() => { | |
| if (!attribute) return null | |
| const cloned = cloneDeep(attribute) as JansAttribute | |
| if (!cloned.attributeValidation) { | |
| cloned.attributeValidation = { | |
| maxLength: undefined, | |
| regexp: undefined, | |
| minLength: undefined, | |
| } | |
| } | |
| return cloned | |
| }, [attribute]) | |
| const customHandleSubmit = useCallback( | |
| ({ data, userMessage, modifiedFields, performedOn }: SubmitData): void => { | |
| if (data) { | |
| putAttributeMutation.mutate( | |
| { data: data as JansAttribute }, | |
| { | |
| onSuccess: async (updatedAttribute: JansAttribute) => { | |
| await logAudit({ | |
| action: UPDATE, | |
| resource: API_ATTRIBUTE, | |
| message: userMessage || '', | |
| modifiedFields, | |
| }) | |
| triggerAttributeWebhook(updatedAttribute) | |
| navigate('/attributes') | |
| }, | |
| }, | |
| ) | |
| } | |
| }, | |
| [putAttributeMutation, logAudit, triggerAttributeWebhook, navigate], | |
| ) |
🤖 Prompt for AI Agents
In admin-ui/plugins/schema/components/Person/AttributeEditPage.tsx around lines
52 to 95, the mutation onSuccess navigates away before the inner mutate
onSuccess finishes logging the audit and triggering the webhook, so refactor so
audit and webhook complete prior to navigation: update the mutate onSuccess to
await (or return/promisify) logAudit and triggerAttributeWebhook (ensure both
return Promises), only call navigate('/attributes') after both have resolved
(handle/retry/report errors as needed, e.g., show error toast on failure), and
remove the earlier immediate navigate so navigation happens after audit/webhook
complete.
| token: token || undefined, | ||
| userinfo: userinfo ?? undefined, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant nullish coercions.
Line 34: token is already '' (from line 25), so token || undefined converts empty string to undefined but adds no safety. Line 35: userinfo ?? undefined is a no-op when userinfo is already undefined.
Simplify:
- token: token || undefined,
- userinfo: userinfo ?? undefined,
+ token,
+ userinfo,📝 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.
| token: token || undefined, | |
| userinfo: userinfo ?? undefined, | |
| token, | |
| userinfo, |
🤖 Prompt for AI Agents
In admin-ui/plugins/schema/hooks/useSchemaAuditLogger.ts around lines 34-35,
remove the redundant nullish/coercion ops: replace `token: token || undefined`
with `token: token` (or simply `token` in the object shorthand) and replace
`userinfo: userinfo ?? undefined` with `userinfo: userinfo` (or `userinfo`
shorthand); keep types/behavior unchanged but avoid unnecessary conversions.
|
4 participants
#2391
closes #2391
Summary by CodeRabbit
New Features
Refactor
Internationalization
Bug Fixes
Tests