feat(admin-ui): revamp Home -> Assets page as per Figma#2677
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge refactor that adds a themed Assets UI (list/add/edit), React Query hooks/mutations, audit logging, validation, file upload support, new styles/components (including freeSolo Autocomplete for GluuSelectRow), i18n additions, tests, and removal of legacy saga/fetch/delete actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AssetForm as Asset Form
participant Services as useAssetServices
participant Validation as Validation Schema
participant Mutation as React Query Mutation
participant API as Asset API
participant Audit as useAssetAudit
participant Query as React Query Cache
participant Toast as Toast
User->>AssetForm: fill fields, select service, attach file
AssetForm->>Services: fetch services
Services-->>AssetForm: return services
User->>AssetForm: submit
AssetForm->>Validation: validate (isEdit affects file check)
Validation-->>AssetForm: validation result
AssetForm->>Mutation: create/update asset (multipart/form-data)
Mutation->>API: send multipart/form-data
API-->>Mutation: success (result.data)
Mutation->>Audit: log action (CREATE/UPDATE)
Audit->>API: post audit record
Mutation->>Query: invalidate assets list
Mutation->>Toast: show success
sequenceDiagram
participant User
participant AssetList as Asset List
participant Query as React Query
participant API as Asset API
participant DeleteHook as useDeleteAssetWithAudit
participant Mutation as Delete Mutation
participant Audit as useAssetAudit
participant Toast as Toast
User->>AssetList: view assets
AssetList->>Query: query assets
Query->>API: fetch assets
API-->>Query: assets data
User->>AssetList: click delete
AssetList->>DeleteHook: deleteAsset(inum)
DeleteHook->>Mutation: run delete
Mutation->>API: delete /assets/{inum}
API-->>Mutation: success
Mutation->>Audit: log DELETION
Audit->>API: post audit record
Mutation->>Query: invalidate assets list
Mutation->>Toast: show deletion success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
admin-ui/plugins/admin/components/Assets/types/AssetApiTypes.ts (1)
73-87:⚠️ Potential issue | 🔴 CriticalFix duplicate interface declarations and remove recursive type reference.
CreateAssetPayloadandUpdateAssetPayloadare declared twice (lines 73–87 and 161–169), causing TypeScript to merge them. Additionally, line 167 contains a recursive type definition:data: UpdateAssetPayloadreferences itself instead ofAssetFormData, breaking the type contract.Remove the first nested payload structure (lines 73–87) and fix line 167:
Fix
-export interface CreateAssetPayload { - payload: { - action: { - action_data: AssetFormData - } - } -} - -export interface UpdateAssetPayload { - payload: { - action: { - action_data: AssetFormData - } - } -} ... export interface UpdateAssetPayload { - data: UpdateAssetPayload + data: AssetFormData token: string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Assets/types/AssetApiTypes.ts` around lines 73 - 87, Remove the duplicated early declarations of CreateAssetPayload and UpdateAssetPayload (the nested payload/action/action_data shape) so there's only one authoritative interface each, and fix the recursive reference in the UpdateAssetPayload declaration so its action_data (or data) field uses AssetFormData instead of referencing UpdateAssetPayload itself; ensure the unique interface names CreateAssetPayload, UpdateAssetPayload and the type AssetFormData are used consistently and only once.admin-ui/plugins/admin/components/Assets/AssetForm.tsx (1)
140-145: 🧹 Nitpick | 🔵 TrivialPotential issue:
closeCommitDialog()is called before dispatch completes.Calling
closeCommitDialog()at the start ofsubmitFormcloses the dialog immediately, but if the Redux action fails, the user has no feedback that submission is in progress. Consider moving the dialog close to after successful submission, or rely on thesaveOperationFlageffect to handle navigation.♻️ Suggested approach
Either:
- Remove
closeCommitDialog()from here and let thesaveOperationFlageffect handle closing/navigation, or- Add loading state feedback before closing the dialog
const submitForm: SubmitFormCallback = useCallback( (userMessage: string): void => { - closeCommitDialog() const payload: AssetFormData = { ...formik.values, service: formik.values.service[0], document: formik.values.document || '', } + closeCommitDialog() // ... rest of logic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Assets/AssetForm.tsx` around lines 140 - 145, The submitForm flow currently calls closeCommitDialog() immediately which hides the dialog before the Redux dispatch finishes; update submitForm so it does not close the dialog up-front—either remove the closeCommitDialog() call from submitForm and let the existing saveOperationFlag effect handle closing/navigation after successful dispatch, or move the closeCommitDialog() call to run only after the dispatch resolves successfully (or after a confirmed success flag), ensuring formik values (service/document) are still gathered and the user sees loading/feedback while the async save completes; refer to submitForm, closeCommitDialog, saveOperationFlag and the dispatch call to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/locales/en/translation.json`:
- Line 789: Update the user-facing validation string for the translation key
"service_required" in translation.json to correct plural agreement by changing
its value from "Related Services is required." to "Related Services are
required." so the message reads properly in the UI.
In `@admin-ui/app/locales/fr/translation.json`:
- Line 737: Update the messages.add_asset translation value to use the same
French terminology as the rest of the feature by replacing "Ajouter un Jans
Asset" with a consistent phrasing using "ressource" (e.g., "Ajouter une
ressource Jans" or simply "Ajouter une ressource") so the key messages.add_asset
matches other keys that use "ressource".
In `@admin-ui/app/locales/pt/translation.json`:
- Line 731: The translation value for the key "add_asset" is inconsistent (it
uses English "Jans Asset"); update the value to use consistent Portuguese
terminology (e.g., replace "Adicionar Jans Asset" with "Adicionar recurso") so
it matches nearby keys' use of "recurso" and ensures consistent UI copy; modify
the string for the "add_asset" key accordingly.
In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx`:
- Around line 129-133: The current options mapping in the useMemo (variable
options) converts SelectOption objects to item.value which discards display
labels; change the mapping to preserve labels by returning typeof item ===
'string' ? item : (item.label ?? item.value) so the Autocomplete can show label
when label !== value; apply the same fix to the other identical mapping around
the Select/Autocomplete usage (the second occurrence noted in the file) and keep
using deduplicateSelectValues(values) and the same dependency array.
- Around line 152-154: The free-solo onChange currently only calls the optional
onValueChange and so skips Formik updates when onValueChange is absent; modify
the onChange handler in GluuSelectRow (the arrow function passed to onChange) to
also update Formik state — e.g., call the Formik field's onChange or use
setFieldValue from useFormikContext with the component's name prop — so that
when onValueChange is undefined the new value is still persisted to Formik.
In `@admin-ui/plugins/admin/__tests__/components/Asset/JansAssetAddPage.test.tsx`:
- Around line 1-82: Extract the duplicated test setup into shared test
utilities: move the store creation (configureStore with reducers
authReducer/assetReducer/webhookReducer/noReducer), the QueryClient
instantiation (queryClient), common jest.mock calls (useCedarling, utility,
resourceScopes, useAssetServices, JansConfigApi), and the Wrapper component into
a new test helper module (e.g., a shared test-utils file) and import them into
JansAssetAddPage.test.tsx and JansAssetEditPage.test.tsx; update tests to use
the shared store, queryClient, mocks and Wrapper (referencing Wrapper,
queryClient, and configureStore/authReducer/assetReducer) so the boilerplate is
centralized and both test files simply import and reuse the utilities.
- Around line 77-81: The test "renders the asset add page with form fields" uses
mixed async and sync queries; change the second assertion to use the async query
so both checks await rendering: in the test for JansAssetAddPage (the it block
named 'renders the asset add page with form fields'), replace
screen.getByText(/Description/i) with await screen.findByText(/Description/i)
while keeping render(..., { wrapper: Wrapper }) and the first await
screen.findByText(/Name/i).
In
`@admin-ui/plugins/admin/__tests__/components/Asset/JansAssetEditPage.test.tsx`:
- Around line 88-92: The test mixes a synchronous query with an async one which
can cause flakiness; update the JansAssetEditPage test to use async queries
consistently by awaiting screen.findByText(/Description/i) instead of
screen.getByText(/Description/i) (so both "Name" and "Description" use
findByText and are awaited) in the it('renders the asset edit page with form
fields', ...) block.
- Around line 75-85: The QueryClient and Redux store are created once at module
scope causing shared cache/state between tests; change to create fresh instances
inside a test lifecycle hook (e.g., beforeEach) so each test gets a new
QueryClient and new store: move the QueryClient construction (new
QueryClient(...)) and the store creation into beforeEach, update Wrapper to
accept the per-test client/store (or construct Wrapper inside tests), and ensure
any teardown uses queryClient.clear() or unmount to avoid leaking; update usages
of QueryClientProvider, QueryClient, Provider, Wrapper, and AppTestWrapper to
reference the per-test instances.
In
`@admin-ui/plugins/admin/__tests__/components/Asset/JansAssetListPage.test.tsx`:
- Around line 93-96: The test currently only covers the "has write permission"
path; add a complementary test that overrides the mocked hook useCedarling to
return hasCedarWritePermission as false and then assert the "Add Jans Asset"
button is not present. Specifically, in
admin-ui/plugins/admin/__tests__/components/Asset/JansAssetListPage.test.tsx
create a new it block that calls
jest.mocked(useCedarling).mockReturnValueOnce(...) with hasCedarReadPermission
true and hasCedarWritePermission false, then render <JansAssetListPage /> with
Wrapper and use expect(screen.queryByText(/Add Jans
Asset/i)).not.toBeInTheDocument() to verify conditional rendering.
In `@admin-ui/plugins/admin/components/Assets/AssetForm.tsx`:
- Around line 124-128: The useEffect in AssetForm.tsx calls formik.validateForm
but omits the formik object from its dependency array, risking stale closures;
update the effect to include the formik instance (e.g., add formik or
formik.validateForm) in the dependency array so the latest validateForm is used,
leaving the existing dependencies (formik.values.document,
formik.touched.document, formik.submitCount, i18n.language) intact and ensuring
the hook references the current Formik methods.
- Around line 161-165: The current conditional that runs
invalidateQueriesByKey(queryClient, getGetAllAssetsQueryKey()),
navigateBack(ROUTES.ASSETS_LIST) and dispatch(resetFlags()) based on
saveOperationFlag and errorInSaveOperationFlag executes during render; move that
entire block into a useEffect in AssetForm.tsx that watches [saveOperationFlag,
errorInSaveOperationFlag, queryClient, navigateBack, dispatch] (or the minimal
stable refs) so the side effects run only after commit, e.g., trigger when
saveOperationFlag becomes true and errorInSaveOperationFlag is false, then call
invalidateQueriesByKey(...), navigateBack(...), and dispatch(resetFlags())
inside the effect. Ensure dependencies include getGetAllAssetsQueryKey if it’s
not stable or memoized.
In `@admin-ui/plugins/admin/components/Assets/hooks/useAssetMutations.ts`:
- Around line 22-38: The deleteAsset flow in useDeleteAssetWithAudit currently
swallows errors from logAction via catch(() => {}); update that catch to surface
audit failures non-silently by invoking the provided error callback
(callbacksRef.current?.onError) and dispatching a visible error/telemetry action
(or at minimum console.error) so audit failures are recorded while keeping the
main deletion non-blocking; modify the catch attached to logAction in
deleteAsset to pass the error to callbacksRef and dispatch (or log) with clear
context including the inum and that logAction failed.
In `@admin-ui/plugins/admin/components/Assets/JansAssetListPage.tsx`:
- Around line 106-117: Replace the ref-based transition logic around
prevIsFetchingRef/useEffect with direct usage of React Query's isFetching to
drive the refresh/loading state: remove prevIsFetchingRef, the useEffect that
reads/sets prevIsFetchingRef and the complex wasFetching check, and instead set
or derive isRefreshing (or the loading indicator) directly from isFetching (or
from a simple derived boolean using isFetching and any existing setIsRefreshing
calls) so components use isFetching from the query hook as the single source of
truth; update references to setIsRefreshing to only update external
user-triggered refresh state if needed and ensure components rely on isFetching
for rendering/loading behavior.
- Around line 142-148: handleRefresh currently invalidates the cache via
invalidateQueriesByKey(getGetAllAssetsQueryKey(...)) but doesn't explicitly
trigger a refetch; update handleRefresh to call the React Query refetch after
invalidation by invoking queryClient.refetchQueries or
queryClient.invalidateQueries(..., { refetchActive: true }) (using the same
getGetAllAssetsQueryKey parameters) so that the assets list is immediately
refetched; ensure you still call setPageNumber(0), setPattern(''), and
setIsRefreshing(true) before refetching to preserve existing UI state.
In `@admin-ui/plugins/admin/helper/assets.ts`:
- Around line 33-40: The returned object in the function currently omits
identity fields required for edits; add inum, dn, and baseDn to the returned
payload by reading them from doc or rec (e.g., doc?.inum ?? rec?.inum, doc?.dn
?? rec?.dn, doc?.baseDn ?? rec?.baseDn) so update submissions retain asset
identity; keep the existing fields (creationDate, document, fileName, enabled,
description, service) as-is and simply append these three properties to the
returned object.
In `@admin-ui/plugins/admin/helper/validations/assetValidation.ts`:
- Around line 13-17: The document field validation duplicates .required(...) in
both branches of the isEdit ternary; refactor by calling
.required(t(T_KEYS.MSG_ASSET_DOCUMENT_ERROR)) once and then conditionally apply
the .test('is-file', t(T_KEYS.MSG_ASSET_DOCUMENT_ERROR), (value) => value
instanceof File) only when !isEdit. Update the validation for the document field
(the Yup.mixed() used with the isEdit flag) so required() is extracted before or
after the conditional test to eliminate the duplicated call while keeping the
same error message and the is-file test when not editing.
- Around line 5-18: Update getAssetValidationSchema so each form field allows
null to match backend DTO: add .nullable() to fileName (before .required()), to
service (make the array schema
.nullable().of(Yup.string().required(...)).min(1,...)) and to document (both
branches include .nullable() and keep the existing .required() and the 'is-file'
test for the non-edit branch). Ensure you reference the existing symbols
fileName, service, document and the isEdit conditional while inserting
.nullable() calls.
In `@admin-ui/plugins/admin/redux/api/AssetApi.ts`:
- Around line 125-134: The code currently only detects File and will re-wrap an
existing Blob unnecessarily; update the asset creation logic around assetFile
and body.document to explicitly handle Blob as well as File (e.g., if
body.document instanceof File use it, else if body.document instanceof Blob use
it directly, otherwise create a new Blob from the data), keeping assetFile typed
as Blob | File and then append assetFile to formData as before.
In `@admin-ui/plugins/admin/redux/sagas/AssetSaga.ts`:
- Around line 46-48: The double type assertion on payload.action.action_data
should be removed and replaced with a proper typed payload; create a specific
interface (e.g., AssetAuditPayload) that extends AdditionalPayload and types
action.action_data as AssetFormData, then call addAdditionalData(audit as
AuditRecord, CREATE, 'asset', { action: { action_data:
payload.action.action_data } } as AssetAuditPayload) or alternatively update
addAdditionalData's signature to accept the AssetFormData shape directly so you
no longer need the unsafe `as unknown as Record<string, unknown>` cast.
---
Outside diff comments:
In `@admin-ui/plugins/admin/components/Assets/AssetForm.tsx`:
- Around line 140-145: The submitForm flow currently calls closeCommitDialog()
immediately which hides the dialog before the Redux dispatch finishes; update
submitForm so it does not close the dialog up-front—either remove the
closeCommitDialog() call from submitForm and let the existing saveOperationFlag
effect handle closing/navigation after successful dispatch, or move the
closeCommitDialog() call to run only after the dispatch resolves successfully
(or after a confirmed success flag), ensuring formik values (service/document)
are still gathered and the user sees loading/feedback while the async save
completes; refer to submitForm, closeCommitDialog, saveOperationFlag and the
dispatch call to implement this change.
In `@admin-ui/plugins/admin/components/Assets/types/AssetApiTypes.ts`:
- Around line 73-87: Remove the duplicated early declarations of
CreateAssetPayload and UpdateAssetPayload (the nested payload/action/action_data
shape) so there's only one authoritative interface each, and fix the recursive
reference in the UpdateAssetPayload declaration so its action_data (or data)
field uses AssetFormData instead of referencing UpdateAssetPayload itself;
ensure the unique interface names CreateAssetPayload, UpdateAssetPayload and the
type AssetFormData are used consistently and only once.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
admin-ui/app/customColors.tsadmin-ui/app/locales/en/translation.jsonadmin-ui/app/locales/es/translation.jsonadmin-ui/app/locales/fr/translation.jsonadmin-ui/app/locales/pt/translation.jsonadmin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsxadmin-ui/app/routes/Apps/Gluu/types/GluuSelectRow.types.tsadmin-ui/plugins/admin/__tests__/components/Asset/JansAssetAddPage.test.tsxadmin-ui/plugins/admin/__tests__/components/Asset/JansAssetEditPage.test.tsxadmin-ui/plugins/admin/__tests__/components/Asset/JansAssetListPage.test.tsxadmin-ui/plugins/admin/components/Assets/AssetForm.tsxadmin-ui/plugins/admin/components/Assets/JansAssetAddPage.tsxadmin-ui/plugins/admin/components/Assets/JansAssetEditPage.tsxadmin-ui/plugins/admin/components/Assets/JansAssetFormPage.style.tsadmin-ui/plugins/admin/components/Assets/JansAssetListPage.style.tsadmin-ui/plugins/admin/components/Assets/JansAssetListPage.tsxadmin-ui/plugins/admin/components/Assets/constants.tsadmin-ui/plugins/admin/components/Assets/hooks/index.tsadmin-ui/plugins/admin/components/Assets/hooks/useAssetAudit.tsadmin-ui/plugins/admin/components/Assets/hooks/useAssetMutations.tsadmin-ui/plugins/admin/components/Assets/hooks/useAssetQueries.tsadmin-ui/plugins/admin/components/Assets/types/AssetApiTypes.tsadmin-ui/plugins/admin/helper/assets.tsadmin-ui/plugins/admin/helper/validations/assetValidation.tsadmin-ui/plugins/admin/redux/api/AssetApi.tsadmin-ui/plugins/admin/redux/features/AssetSlice.tsadmin-ui/plugins/admin/redux/sagas/AssetSaga.ts
faisalsiddique4400
changed the title
faisalsiddique4400
changed the title
faisalsiddique4400
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
admin-ui/plugins/admin/helper/assets.ts (1)
41-43:⚠️ Potential issue | 🟠 MajorDo not coerce missing identity fields to empty strings.
Using
''forinum,dn, andbaseDncan propagate invalid identifiers into update payloads; keep these optional fields asundefinedwhen missing.💡 Suggested fix
+function toOptionalStringValue(val: unknown): string | undefined { + if (typeof val === 'string') return val || undefined + return val != null ? String(val) : undefined +} + export const buildAssetInitialValues = ( asset?: Document | Record<string, unknown> | null, ): AssetFormValues => { @@ - inum: toStringValue(doc?.inum ?? rec?.inum, ''), - dn: toStringValue(doc?.dn ?? rec?.dn, ''), - baseDn: toStringValue(doc?.baseDn ?? rec?.baseDn, ''), + inum: toOptionalStringValue(doc?.inum ?? rec?.inum), + dn: toOptionalStringValue(doc?.dn ?? rec?.dn), + baseDn: toOptionalStringValue(doc?.baseDn ?? rec?.baseDn), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/helper/assets.ts` around lines 41 - 43, The code currently coerces missing identity fields to empty strings by calling toStringValue(doc?.inum ?? rec?.inum, ''), toStringValue(doc?.dn ?? rec?.dn, ''), and toStringValue(doc?.baseDn ?? rec?.baseDn, ''), which can inject invalid identifiers into update payloads; change these calls so that missing values remain undefined (e.g., pass undefined or omit the default third argument) for the inum, dn, and baseDn fields in assets.ts to preserve optionality and avoid sending empty-string identifiers.admin-ui/plugins/admin/redux/api/AssetApi.ts (1)
125-136:⚠️ Potential issue | 🟠 MajorAvoid uploading filename text as
assetFilecontent.When
body.documentis a string (typical in edit flow), this code builds a Blob from that string and appends it asassetFile, which can overwrite the asset with incorrect bytes.💡 Suggested fix
private buildFormData(body: AssetFormData, document: Document): FormData { const formData = new FormData() - const assetFile: Blob | File = - body.document instanceof File - ? body.document - : body.document instanceof Blob - ? body.document - : new Blob([body.document as BlobPart], { type: 'application/octet-stream' }) const documentBlob = new Blob([JSON.stringify({ ...document })], { type: 'application/json', }) formData.append('document', documentBlob) - formData.append('assetFile', assetFile) + if (body.document instanceof File || body.document instanceof Blob) { + formData.append('assetFile', body.document) + } return formData }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/redux/api/AssetApi.ts` around lines 125 - 136, The current logic creates assetFile from body.document even when body.document is a string, causing uploads of filename text as file bytes; update the code in AssetApi.ts so assetFile is only created and appended when body.document is actually a File or a Blob (i.e., check body.document instanceof File || body.document instanceof Blob) and skip appending 'assetFile' otherwise (leave only the JSON document Blob appended via documentBlob). Ensure you remove or guard the formData.append('assetFile', assetFile) call when body.document is a string so the existing asset is not overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx`:
- Around line 78-89: The focus style in GluuSelectRow (selectors '&.Mui-focused'
and '&:hover') removes browser focus indicators and makes keyboard focus
indistinguishable; restore an accessible focus ring by adding a distinct,
high-contrast focus rule (preferably include '&.Mui-focusVisible' alongside
'&.Mui-focused') that sets a visible outline/boxShadow (e.g. a 2–4px focus ring
using a contrast color variable) and remove the "outline: none" for focus states
so keyboard users can perceive focus. Ensure the focus rule is visually
different from hover (use boxShadow or outline rather than matching border
color) and keep specificity consistent with the existing styling.
In `@admin-ui/plugins/admin/__tests__/components/Asset/assetTestUtils.tsx`:
- Line 61: The mock authReducer in tests lacks the nested shape expected by
useAssetAudit, causing crashes; update the authReducer mock (the reducer used in
assetTestUtils.tsx) to return a full state object with keys config, location,
and userinfo (plus permissions) containing the fields your audit-aware hooks
read (e.g., config.*, location.*, userinfo.*) so delete/audit paths can read
safely; keep minimal realistic defaults (empty arrays/strings/flags) to satisfy
useAssetAudit and other auth consumers.
- Around line 69-81: Remove the exported singleton QueryClient named
"queryClient" and ensure createAssetTestWrapper no longer references it; instead
change createAssetTestWrapper to default its client parameter by calling
createAssetTestQueryClient() so each wrapper gets a fresh QueryClient; update
any tests (e.g., JansAssetAddPage.test.tsx) that import the singleton to either
pass a client created via createAssetTestQueryClient() or rely on the wrapper's
new default, and ensure any leftover imports/usages of "queryClient" are
deleted.
In `@admin-ui/plugins/admin/redux/sagas/AssetSaga.ts`:
- Around line 55-56: The audit post (yield call(postUserAction, audit as
UserActionPayload)) is currently inside the same try that handles the
create/update mutation so a postUserAction failure can cause the whole operation
to appear failed (data: null); refactor by moving the postUserAction call out of
the mutation's try/catch or wrap it in its own try/catch so any error from
postUserAction is caught/logged separately and does not overwrite or change the
mutation result returned by the saga (handle both occurrences of postUserAction
in AssetSaga — the create path and the update path).
---
Duplicate comments:
In `@admin-ui/plugins/admin/helper/assets.ts`:
- Around line 41-43: The code currently coerces missing identity fields to empty
strings by calling toStringValue(doc?.inum ?? rec?.inum, ''),
toStringValue(doc?.dn ?? rec?.dn, ''), and toStringValue(doc?.baseDn ??
rec?.baseDn, ''), which can inject invalid identifiers into update payloads;
change these calls so that missing values remain undefined (e.g., pass undefined
or omit the default third argument) for the inum, dn, and baseDn fields in
assets.ts to preserve optionality and avoid sending empty-string identifiers.
In `@admin-ui/plugins/admin/redux/api/AssetApi.ts`:
- Around line 125-136: The current logic creates assetFile from body.document
even when body.document is a string, causing uploads of filename text as file
bytes; update the code in AssetApi.ts so assetFile is only created and appended
when body.document is actually a File or a Blob (i.e., check body.document
instanceof File || body.document instanceof Blob) and skip appending 'assetFile'
otherwise (leave only the JSON document Blob appended via documentBlob). Ensure
you remove or guard the formData.append('assetFile', assetFile) call when
body.document is a string so the existing asset is not overwritten.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
admin-ui/app/locales/en/translation.jsonadmin-ui/app/locales/fr/translation.jsonadmin-ui/app/locales/pt/translation.jsonadmin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsxadmin-ui/plugins/admin/__tests__/components/Asset/JansAssetAddPage.test.tsxadmin-ui/plugins/admin/__tests__/components/Asset/JansAssetEditPage.test.tsxadmin-ui/plugins/admin/__tests__/components/Asset/JansAssetListPage.test.tsxadmin-ui/plugins/admin/__tests__/components/Asset/assetTestUtils.tsxadmin-ui/plugins/admin/components/Assets/AssetForm.tsxadmin-ui/plugins/admin/components/Assets/JansAssetListPage.tsxadmin-ui/plugins/admin/components/Assets/hooks/useAssetMutations.tsadmin-ui/plugins/admin/helper/assets.tsadmin-ui/plugins/admin/helper/validations/assetValidation.tsadmin-ui/plugins/admin/redux/api/AssetApi.tsadmin-ui/plugins/admin/redux/features/types/asset.tsadmin-ui/plugins/admin/redux/sagas/AssetSaga.ts
admin-ui/plugins/admin/__tests__/components/Asset/assetTestUtils.tsx
Outdated
Show resolved
Hide resolved
admin-ui/plugins/admin/__tests__/components/Asset/assetTestUtils.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx (1)
118-126: 🧹 Nitpick | 🔵 TrivialConsider using
formikobject in dependency array.Using
formik.handleChangein the dependency array works but is slightly inconsistent with the recommended Formik pattern of including the entireformikobject to avoid potential stale closure issues.♻️ Proposed change
const handleSelectChange = useCallback( (event: React.ChangeEvent<HTMLSelectElement>) => { formik.handleChange(event) if (handleChange) { handleChange(event) } }, - [formik.handleChange, handleChange], + [formik, handleChange], )Based on learnings: "Keeping the entire formik object in the dependency array is the recommended pattern, as extracting methods separately can cause issues with form focusing and break Formik's internal field registration and focus management."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx` around lines 118 - 126, handleSelectChange currently lists formik.handleChange in its dependency array which can lead to stale closure issues; update the dependency array to include the entire formik object instead. Locate the handleSelectChange callback and change its dependencies from [formik.handleChange, handleChange] to [formik, handleChange], ensuring the body still calls formik.handleChange(event) and optional handleChange(event).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx`:
- Around line 166-176: The freeSolo render path for the Autocomplete is missing
a test identifier: inside the renderInput callback that returns the TextField
(the code using renderInput={(params) => (<TextField ... />)}), add
data-testid={name} to that TextField so it matches the non-freeSolo CustomInput
(which already uses data-testid={name}), ensuring consistent test hooks across
both paths; update any related props on the same JSX element (TextField) only.
---
Outside diff comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx`:
- Around line 118-126: handleSelectChange currently lists formik.handleChange in
its dependency array which can lead to stale closure issues; update the
dependency array to include the entire formik object instead. Locate the
handleSelectChange callback and change its dependencies from
[formik.handleChange, handleChange] to [formik, handleChange], ensuring the body
still calls formik.handleChange(event) and optional handleChange(event).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsxadmin-ui/plugins/admin/__tests__/components/Asset/assetTestUtils.tsxadmin-ui/plugins/admin/redux/sagas/AssetSaga.ts
…ex into admin-ui-issue-2628
Contributor
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/plugins/admin/components/Assets/types/AssetApiTypes.ts (1)
73-87:⚠️ Potential issue | 🔴 CriticalDuplicate interface declarations and recursive self-reference create unsound payload types.
CreateAssetPayloadandUpdateAssetPayloadare declared twice (lines 73-79 & 159-162, and lines 81-87 & 164-167). TypeScript merges duplicate interface declarations, resulting in malformed composite types. Additionally,UpdateAssetPayload.dataat line 165 is typed asUpdateAssetPayload, creating a recursive self-reference that violates the payload contract.The first pair (lines 73–87) define saga envelope structures with
payload.action.action_data, while the second pair (lines 159–167) define API payload structures withdataandtoken. These should be kept separate as distinct types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Assets/types/AssetApiTypes.ts` around lines 73 - 87, The file defines duplicate interfaces CreateAssetPayload and UpdateAssetPayload which TypeScript merges and causes a recursive self-reference (UpdateAssetPayload.data typed as UpdateAssetPayload); split the two concerns by renaming and separating the saga-envelope types (currently using payload.action.action_data) from the API payload types (which have data and token), e.g. keep the saga shapes as CreateAssetSagaPayload and UpdateAssetSagaPayload (with payload.action.action_data: AssetFormData) and rename the API shapes to CreateAssetApiPayload and UpdateAssetApiPayload (with data: AssetFormData and token: string), and remove any recursive typing (ensure UpdateAssetApiPayload.data is AssetFormData, not UpdateAssetPayload) so each interface is unique and correctly represents its contract.
♻️ Duplicate comments (2)
admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx (1)
110-119:⚠️ Potential issue | 🟡 MinorAdd
data-testidto free-soloTextFieldfor test parity.The non-freeSolo path already exposes
data-testid={name}; freeSolo should match to keep tests consistent across both render paths.🧪 Suggested fix
renderInput={(params) => ( <TextField {...params} name={name} + data-testid={name} placeholder={`${t('actions.choose')}...`} error={showError && Boolean(errorMessage)} helperText={showError ? errorMessage : undefined} size="small" fullWidth /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx` around lines 110 - 119, The free-solo render path in GluuSelectRow's renderInput is missing the data-testid attribute on the TextField, causing test inconsistency; update the TextField rendered in the freeSolo branch (inside renderInput) to include data-testid={name} so it matches the non-freeSolo path and preserves test parity (reference: GluuSelectRow component, renderInput and the TextField element using prop name).admin-ui/app/routes/Apps/Gluu/styles/GluuSelectRow.style.ts (1)
78-83:⚠️ Potential issue | 🟠 MajorFocus styling is still not distinguishable enough for keyboard users.
Mui-focused/Mui-focusVisiblecurrently suppress visible indicators and look too similar to unfocused state.♿ Suggested fix
'&.Mui-focused, &.Mui-focusVisible': { border: `1px solid ${inputBorderColor} !important`, backgroundColor: `${formInputBg} !important`, - boxShadow: 'none', - outline: 'none', + boxShadow: `0 0 0 2px ${inputBorderColor}55`, }, + '&.Mui-focused.Mui-focusVisible': { + outline: `2px solid ${inputBorderColor}`, + outlineOffset: 1, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/styles/GluuSelectRow.style.ts` around lines 78 - 83, The current focus selectors '&.Mui-focused, &.Mui-focusVisible' remove visible indicators and make keyboard focus hard to see; update these styles so keyboard focus is clearly distinguishable by restoring a visible focus ring and contrast (e.g., remove the "none" boxShadow/outline suppression and instead apply a high-contrast ring or thicker border and a subtle elevated boxShadow) and avoid using identical backgroundColor to unfocused state; modify the rules for '&.Mui-focused, &.Mui-focusVisible' to set a distinct border (or 2–3px focus ring color), a contrasting background or slight lift via boxShadow, and keep outline/boxShadow present for accessibility so keyboard users can see focus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/plugins/admin/components/Assets/hooks/useAssetAudit.ts`:
- Around line 56-63: The audit currently forwards payload.action_data directly
(in useAssetAudit where addAdditionalData is called and then postUserAction),
which can leak File/Blob objects or oversized data; before calling
addAdditionalData/postUserAction sanitize payload.action_data by
mapping/removing binary/File/Blob instances and trimming large fields (e.g.,
replace File/Blob with a short metadata object {type: 'file', name, size} or a
"[REDACTED]" marker and truncate strings/objects over a size threshold), then
pass the sanitized object into addAdditionalData and postUserAction (update the
code around addAdditionalData(audit, actionType, resource, { action: {
action_message, action_data } }) to use the sanitizedActionData).
In `@admin-ui/plugins/admin/components/Assets/JansAssetFormPage.style.ts`:
- Around line 207-214: The focus rule that matches '& input:focus, &
input:active, & select:focus, & select:active, & .custom-select:focus, &
.custom-select:active' currently clears outline and boxShadow via OUTLINE_NONE
which hides keyboard focus; change that to a visible, high-contrast focus
indicator instead (for example set outline to a visible value like `2px solid
${themeColors.focus}` or `3px auto ${themeColors.focus}` and/or a subtle
boxShadow using the same accessible color) and remove the !important suppression
so keyboard focus is perceivable; apply the same change to the other identical
focus rule later in the file that also uses OUTLINE_NONE.
In `@admin-ui/plugins/admin/components/Assets/JansAssetListPage.style.ts`:
- Around line 22-23: Replace the hardcoded numeric spacing (24) used for
paddingTop in this file with the module's existing spacing token(s); locate the
two paddingTop occurrences in JansAssetListPage.style.ts (the style object
definitions where paddingTop: 24 is set) and swap them to use the project's
spacing token API already imported/defined in this module (e.g., the appropriate
spacing token constant or theme.spacing value) so both places use the same token
rather than a literal number.
In `@admin-ui/plugins/admin/components/Assets/JansAssetListPage.tsx`:
- Line 250: Replace the hardcoded boolean text rendering in
JansAssetListPage.tsx ({isEnabled ? 'true' : 'false'}) with localized strings
using the app's i18n mechanism (e.g. call the translation function t(...) or use
IntlProvider/FormattedMessage) so the enabled/disabled labels come from
translation keys like "common.true"/"common.false" or
"assets.status.enabled"/"assets.status.disabled"; update the JSX to use t('...')
for both branches and import/use the same i18n hook or prop that other
components in the project use (ensure the component imports useTranslation or
receives intl if required).
- Line 300: getRowKey currently returns an empty string when row.inum is missing
which causes duplicate React keys; update the getRowKey useCallback to accept
the index parameter (e.g., useCallback((row: Document, index: number) => ...) )
and return a unique fallback such as index.toString() when row.inum is falsy
(follow the pattern used in WebhookListPage), ensuring each row has a stable
unique key.
In `@admin-ui/plugins/admin/redux/sagas/AssetSaga.ts`:
- Around line 110-116: Replace the hardcoded action type strings in the saga
watchers with the action creator .type properties from the AssetSlice to avoid
brittle string literals: import the relevant action creators (e.g.,
createJansAsset and updateJansAsset) from AssetSlice and use
createJansAsset.type and updateJansAsset.type in the takeLatest calls inside
watchCreateJansAsset and watchUpdateJansAsset so the watchers stay correct if
the slice action names change.
---
Outside diff comments:
In `@admin-ui/plugins/admin/components/Assets/types/AssetApiTypes.ts`:
- Around line 73-87: The file defines duplicate interfaces CreateAssetPayload
and UpdateAssetPayload which TypeScript merges and causes a recursive
self-reference (UpdateAssetPayload.data typed as UpdateAssetPayload); split the
two concerns by renaming and separating the saga-envelope types (currently using
payload.action.action_data) from the API payload types (which have data and
token), e.g. keep the saga shapes as CreateAssetSagaPayload and
UpdateAssetSagaPayload (with payload.action.action_data: AssetFormData) and
rename the API shapes to CreateAssetApiPayload and UpdateAssetApiPayload (with
data: AssetFormData and token: string), and remove any recursive typing (ensure
UpdateAssetApiPayload.data is AssetFormData, not UpdateAssetPayload) so each
interface is unique and correctly represents its contract.
---
Duplicate comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsx`:
- Around line 110-119: The free-solo render path in GluuSelectRow's renderInput
is missing the data-testid attribute on the TextField, causing test
inconsistency; update the TextField rendered in the freeSolo branch (inside
renderInput) to include data-testid={name} so it matches the non-freeSolo path
and preserves test parity (reference: GluuSelectRow component, renderInput and
the TextField element using prop name).
In `@admin-ui/app/routes/Apps/Gluu/styles/GluuSelectRow.style.ts`:
- Around line 78-83: The current focus selectors '&.Mui-focused,
&.Mui-focusVisible' remove visible indicators and make keyboard focus hard to
see; update these styles so keyboard focus is clearly distinguishable by
restoring a visible focus ring and contrast (e.g., remove the "none"
boxShadow/outline suppression and instead apply a high-contrast ring or thicker
border and a subtle elevated boxShadow) and avoid using identical
backgroundColor to unfocused state; modify the rules for '&.Mui-focused,
&.Mui-focusVisible' to set a distinct border (or 2–3px focus ring color), a
contrasting background or slight lift via boxShadow, and keep outline/boxShadow
present for accessibility so keyboard users can see focus.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
admin-ui/app/routes/Apps/Gluu/GluuSelectRow.tsxadmin-ui/app/routes/Apps/Gluu/styles/GluuSelectRow.style.tsadmin-ui/plugins/admin/components/Assets/JansAssetFormPage.style.tsadmin-ui/plugins/admin/components/Assets/JansAssetListPage.style.tsadmin-ui/plugins/admin/components/Assets/JansAssetListPage.tsxadmin-ui/plugins/admin/components/Assets/hooks/useAssetAudit.tsadmin-ui/plugins/admin/components/Assets/types/AssetApiTypes.tsadmin-ui/plugins/admin/components/Assets/types/FormTypes.tsadmin-ui/plugins/admin/helper/assets.tsadmin-ui/plugins/admin/redux/api/AssetApi.tsadmin-ui/plugins/admin/redux/features/types/asset.tsadmin-ui/plugins/admin/redux/sagas/AssetSaga.ts
admin-ui/plugins/admin/components/Assets/JansAssetListPage.style.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
admin-ui/plugins/admin/components/Assets/JansAssetListPage.tsx (1)
148-153: 🧹 Nitpick | 🔵 Trivial
handleRefreshmay not trigger a refetch with correct parameters.After resetting
patternandpageNumber, the function invalidates a query with{ pattern: undefined, startIndex: 0 }. However, the active query may still have the oldpatternvalue until the state updates trigger a re-render. Consider callingrefetch()explicitly or usingqueryClient.refetchQueriesto ensure immediate data refresh.♻️ Suggested improvement
const handleRefresh = useCallback(() => { setPageNumber(0) setPattern('') const refreshParams = { limit, pattern: undefined, startIndex: 0 } invalidateQueriesByKey(queryClient, getGetAllAssetsQueryKey(refreshParams)) + // Trigger refetch after state updates + setTimeout(() => refetch(), 0) - }, [queryClient, limit]) + }, [queryClient, limit, refetch])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/admin/components/Assets/JansAssetListPage.tsx` around lines 148 - 153, handleRefresh resets local state but then only calls invalidateQueriesByKey with a params object that may not match the currently cached query (because setPattern/setPageNumber are async), so the query may not be refetched with the intended parameters; update handleRefresh to both reset state (setPageNumber(0), setPattern('')) and then immediately trigger an immediate refetch on the queryClient using the correct key—e.g. call queryClient.refetchQueries(getGetAllAssetsQueryKey({ limit, pattern: undefined, startIndex: 0 })) or call invalidateQueries followed by queryClient.refetchQueries(...) so the data refresh happens immediately with the intended params (referencing handleRefresh, setPageNumber, setPattern, invalidateQueriesByKey, queryClient, getGetAllAssetsQueryKey).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/plugins/admin/components/Assets/hooks/useAssetAudit.ts`:
- Around line 89-104: logAction currently awaits postUserAction and will
propagate failures; change it to swallow audit errors so the primary operation
doesn't fail: in useAssetAudit's logAction callback (which calls initAudit,
sanitizeActionData, addAdditionalData and postUserAction) wrap the await
postUserAction(audit as UserActionPayload) in a try/catch (or run it as a
detached promise) and on error call a local logger (or console.error) to record
the failure but do not rethrow; ensure the function still returns/awaits before
caller proceeds only if needed, otherwise make it fire-and-forget by not letting
postUserAction rejection bubble up.
In `@admin-ui/plugins/admin/components/Assets/JansAssetListPage.tsx`:
- Around line 333-344: The GluuTable is hardcoded with loading={false} which
conflicts with the outer GluuLoader's blocking state; update the GluuTable call
in JansAssetListPage (the JSX where GluuTable<Document> is rendered) to pass the
actual loading state instead of false (e.g., loading={loading} or
loading={assetsLoading} depending on the component state/prop name) so the table
reflects real load status, or if the hardcoded false is intentional to avoid
double indicators, add a short clarifying comment above the GluuTable explaining
why loading is forced to false.
- Around line 170-184: The catch in submitForm swallowing errors makes debugging
harder; update submitForm to catch the thrown error (e.g., catch (err)) and,
while still relying on deleteAsset's user-facing handling, log the error for
devs—use console.error(err) or a conditional log gated by NODE_ENV !==
'production'—and then preserve the existing refetch/setDeleteData behavior;
reference submitForm, deleteAsset, deleteData, refetch, and setDeleteData when
making the change.
In `@admin-ui/plugins/admin/redux/sagas/AssetSaga.ts`:
- Around line 24-25: The imports Document, AssetFormData and AssetRootState are
only used as types; change their import statements to use TypeScript's type-only
imports (i.e., use "import type" for Document and AssetFormData from
'../../components/Assets/types/AssetApiTypes' and for AssetRootState from
'./types/asset') so the compiler/tree-shaker knows they are type-only imports.
---
Duplicate comments:
In `@admin-ui/plugins/admin/components/Assets/JansAssetListPage.tsx`:
- Around line 148-153: handleRefresh resets local state but then only calls
invalidateQueriesByKey with a params object that may not match the currently
cached query (because setPattern/setPageNumber are async), so the query may not
be refetched with the intended parameters; update handleRefresh to both reset
state (setPageNumber(0), setPattern('')) and then immediately trigger an
immediate refetch on the queryClient using the correct key—e.g. call
queryClient.refetchQueries(getGetAllAssetsQueryKey({ limit, pattern: undefined,
startIndex: 0 })) or call invalidateQueries followed by
queryClient.refetchQueries(...) so the data refresh happens immediately with the
intended params (referencing handleRefresh, setPageNumber, setPattern,
invalidateQueriesByKey, queryClient, getGetAllAssetsQueryKey).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
admin-ui/plugins/admin/components/Assets/JansAssetListPage.style.tsadmin-ui/plugins/admin/components/Assets/JansAssetListPage.tsxadmin-ui/plugins/admin/components/Assets/hooks/useAssetAudit.tsadmin-ui/plugins/admin/redux/sagas/AssetSaga.ts
|
duttarnab
approved these changes
Feb 28, 2026
moabu
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(admin-ui): revamp Home -> Assets page as per Figma (#2628)
Summary
This PR revamps the Home → Assets page to align with the latest Figma designs as part of the ongoing Admin UI redesign initiative.
Changes Included
Result
Parent Initiative
This work is part of the following parent feature:
feat(admin-ui): Revamp the existing design of the admin-ui according to the new design🔗 Ticket
Closes: #2628
Summary by CodeRabbit
New Features
Improvements
Behavior Changes
Tests