Feature/fin 023#103
Merged
Merged
Conversation
…delete confirmation
|
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:
📝 WalkthroughWalkthroughRefactors lookup management (countries/currencies) to use a centralized Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as CountriesLookup<br/>Component
participant Hook as useAdminLookup<br/>Hook
participant Service as LookupsService
participant API as API<br/>Endpoints
participant DB as Database
User->>UI: Load component
UI->>Hook: Initialize with mutations
Hook->>Service: fetchItems (from, to, filters)
Service->>API: POST /api/lookups/countries-and-locales/get-items
API->>DB: Query with admin relation
DB-->>API: Return paginated items + admin
API-->>Service: Returns data
Service-->>Hook: Items loaded
Hook-->>UI: Render table
User->>UI: Click edit row
UI->>Hook: openEditForm(item)
Hook-->>UI: Set isFormOpen=true, editingItem
UI->>UI: Render CountryFormModal with initialData
User->>UI: Submit form
UI->>Hook: onSuccessCallback via mutation
Hook->>Service: submitMutation({id, data})
Service->>API: PUT /api/lookups/countries-and-locales/update/{id}
API->>DB: Update country & locale
DB-->>API: Success
API-->>Service: Returns data
Service-->>Hook: onSuccess triggered
Hook->>Hook: reload(), toast
Hook-->>UI: Close modal, refresh table
UI-->>User: Updated table displayed
User->>UI: Click delete row
UI->>Hook: requestSingleDelete(item)
Hook-->>UI: Open confirm modal
User->>UI: Confirm delete
UI->>Hook: confirmSingleDelete()
Hook->>Service: deleteMutation(id)
Service->>API: DELETE /api/lookups/countries-and-locales/remove/{id}
API->>DB: Soft-delete country
DB-->>API: Success
API-->>Service: Returns data
Service-->>Hook: onSuccess
Hook->>Hook: deselect(id), reload(), toast
Hook-->>UI: Refresh table
UI-->>User: Item removed, table updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes introduce substantial new logic across multiple interconnected areas: a centralized lookup hook managing pagination/selection/deletion/forms, new API endpoints with authentication and validation, ORM relation updates, and UI component refactoring. While the implementation follows consistent patterns, the breadth of affected files and the need to verify the complete flow from UI through mutations to API to database requires careful review. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
src/client/shared/ui/ui-dialog/ui-dialog.tsx (1)
81-83: Close-button utility selectors target<svg>, but the icon renders as<i>.The
_svgutility selectors on Line 81 do not apply toUiSvgIcon(it renders<i>), so those styles are effectively dead here.♻️ Suggested fix
- className="absolute top-4 right-4 rounded-xs opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:ring-2 focus:ring-ring focus:ring-offset-2 focus:outline-hidden disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4" + className="absolute top-4 right-4 rounded-xs opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:ring-2 focus:ring-ring focus:ring-offset-2 focus:outline-hidden disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground [&_i]:pointer-events-none [&_i]:shrink-0 [&_i:not([class*='size-'])]:size-4" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/shared/ui/ui-dialog/ui-dialog.tsx` around lines 81 - 83, The close-button's utility selectors target SVG elements but UiSvgIcon renders an <i>, so those styles never apply; update the close button's className on the close element (where UiSvgIcon is used) to include matching nested selectors for <i> as well as <svg> (e.g. add [&_i]:pointer-events-none [&_i]:shrink-0 [&_i:not([class*='size-'])]:size-4 or similar equivalents) so the pointer-events, sizing and shrink rules apply to UiSvgIcon, or alternatively change UiSvgIcon to render an actual <svg> element if you prefer that component-level fix (referencing UiSvgIcon and the close button className).src/client/shared/components/admin-modal/fin-admin-modal.tsx (1)
13-23: Consider aligning the interface name with the component export.The interface is named
FinAdminModalPropsbut the exported component isUiAdminModal. For consistency with theUi*naming convention used throughout the UI layer, consider renaming toUiAdminModalProps.♻️ Suggested rename
-interface FinAdminModalProps { +interface UiAdminModalProps { isOpen: boolean; onClose: () => void; title: string; description?: string; children: React.ReactNode; formId: string; isLoading?: boolean; } -export function UiAdminModal({ isOpen, onClose, title, description, children, formId, isLoading }: FinAdminModalProps) { +export function UiAdminModal({ isOpen, onClose, title, description, children, formId, isLoading }: UiAdminModalProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/shared/components/admin-modal/fin-admin-modal.tsx` around lines 13 - 23, Rename the props interface to match the exported component's Ui* naming: change FinAdminModalProps to UiAdminModalProps and update its usage in the UiAdminModal function signature so the parameter type references UiAdminModalProps; ensure any other references or imports (e.g., tests, consumers) that used FinAdminModalProps are updated to the new name to keep naming consistent with the UI layer convention.src/client/features/admin/countries-lookup/countries-lookup.tsx (1)
140-152: Hidden modal pattern with ref triggers works but is fragile.The PR description mentions this is intentional to avoid modifying
UiConfirmModal. While functional, this approach has drawbacks: timing-dependent (setTimeout), hidden DOM elements, and accessibility concerns witharia-hiddenon interactive elements.Consider refactoring
UiConfirmModalto support controlled open state in a future iteration for cleaner integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/features/admin/countries-lookup/countries-lookup.tsx` around lines 140 - 152, The hidden-trigger pattern using UiConfirmModal + singleDeleteTriggerRef is fragile and should be replaced with a controlled open prop: update UiConfirmModal to accept an isOpen (boolean) and onClose callback (and keep onConfirm), then in this file replace the hidden button/ref logic with a local state (e.g. isSingleDeleteOpen) and setIsSingleDeleteOpen(true) to open and setIsSingleDeleteOpen(false) in onClose; wire isSingleDeleteOpen to the new isOpen prop and confirmSingleDelete to onConfirm (also close after confirm). Update occurrences of singleDeleteTriggerRef, the hidden button, and any setTimeout click hacks to use this new controlled open state and onClose instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/features/admin/countries-lookup/countries-lookup.tsx`:
- Around line 154-165: The bulk-delete path currently calls confirmBulkDelete
directly and never opens the UiConfirmModal; update the delete handler to
trigger the modal by programmatically clicking the ref instead. Replace the
direct onDelete={confirmBulkDelete} usage with a small handler (e.g.,
onBulkDeleteClick or a lambda) that calls bulkDeleteTriggerRef.current?.click(),
and move the actual deletion logic into the confirmBulkDelete callback (the
onConfirm prop of UiConfirmModal). Ensure you reference bulkDeleteTriggerRef,
confirmBulkDelete, and UiConfirmModal so the modal is triggered before deletion.
- Around line 59-67: confirmBulkDelete currently just shows a success toast and
clears selection with no API call; replace the no-op with real bulk-delete logic
by invoking the backend delete function (or fetch/axios call) for the selected
IDs, await its completion inside confirmBulkDelete, handle errors by showing
showToast({ title: 'Error', ... }) and rethrow or return, and on success call
showToast({ title: 'Success', ... }), call clearSelection(), and refresh the
list/state (e.g., refetch or call the existing load/refresh function) so the UI
reflects deletions; locate confirmBulkDelete, clearSelection, and showToast in
this file and use the same ID array/state used to drive selection.
- Around line 74-87: The delete handler confirmSingleDelete currently patches
the server but doesn't refresh the list; obtain React Query's QueryClient (via
useQueryClient) in this component and call queryClient.invalidateQueries with
the countries list query key (the same key used to fetch the countries list)
after the successful patch inside confirmSingleDelete (before or after
showToast) so the UI refetches and the deleted item disappears; ensure you
import/useQueryClient and reference the same query key string/array used by the
countries fetch.
In `@src/client/features/admin/currencies-lookup/currencies-lookup.tsx`:
- Around line 54-55: The bulk-delete flow currently calls confirmBulkDelete
directly, bypassing the confirmation modal; instead, wire the onDelete handler
to programmatically click the hidden modal trigger so the modal opens (use
bulkDeleteTriggerRef.current?.click()), and keep confirmBulkDelete as the
callback invoked by the modal confirm action; update any places where
onDelete={confirmBulkDelete} to onDelete={() =>
bulkDeleteTriggerRef.current?.click()} and ensure the confirmation modal
component uses confirmBulkDelete as its confirm callback.
- Around line 80-93: confirmSingleDelete performs the soft-delete but doesn't
refresh the currencies list; after the successful fetchClient.patch call in
confirmSingleDelete (and before clearing itemToDelete), call your query cache
updater (e.g., queryClient.invalidateQueries or refetchQueries) for the
currencies lookup query key used by the component's useQuery hook so the list is
refetched and UI updates; keep the existing success toast and error handling
intact.
- Around line 65-73: confirmBulkDelete currently performs no deletion — it only
shows a success toast and clears selection; modify confirmBulkDelete to await
the real bulk-delete API call for the selected currency IDs, handle the
response/errors, then on success call clearSelection() and showToast({ title:
'Success', ... }) and on failure show the error message via showToast. Locate
confirmBulkDelete in currencies-lookup.tsx and replace the empty try block with
an awaited API call (e.g., your existing bulk-delete client method or a loop of
deleteCurrency calls using the selected IDs), update local state to remove
deleted items from the list, and ensure errors are caught and their messages
shown (use error instanceof Error ? error.message : fallback).
In `@src/client/features/admin/lookups/countries/country-form-modal.tsx`:
- Around line 25-28: The form is not reset when switching edit items because
useForm's defaultValues only apply on mount; update the component to call
methods.reset(initialData ?? { countryName: '', localeName: '' }) whenever
initialData changes (useEffect that depends on initialData) so the form state
(methods from useForm<CountryFormData>) is synchronized with the new data
validated by countrySchema; ensure you reference the existing methods.reset(...)
call rather than recreating the form instance.
- Around line 30-40: The onSuccess behavior is missing from useSendDataFetch so
the modal (country-form-modal) never closes and the country list isn't
refreshed; add an onSuccess callback in the options passed to useSendDataFetch
that (1) closes the modal (call the component's close handler or set the modal
open state to false—wherever the modal is controlled in this component) and (2)
invalidates the countries lookup query via your react-query QueryClient (e.g.,
call queryClient.invalidateQueries with the countries list query key used by the
list component). Tie this onSuccess to the existing mutate/isPending usage so
both create (when initialData is falsy) and update (when initialData is present)
trigger the close + invalidate sequence.
In `@src/client/features/admin/lookups/currencies/currency-form-modal.tsx`:
- Around line 25-29: CurrencyFormModal's form doesn't update when initialData
changes because useForm's defaultValues only apply on mount; modify the
component to call methods.reset(initialData || { name: '', code: '', symbol: ''
}) inside a useEffect that watches initialData so the form syncs when switching
edit items. Locate the useForm invocation (methods) and add a useEffect
referencing methods.reset and initialData to reinitialize the form whenever
initialData changes.
- Around line 31-41: The mutation created with useSendDataFetch (the const {
mutate, isPending } = useSendDataFetch(...)) lacks an onSuccess handler, so the
modal never closes and list data isn't refreshed; add an onSuccess callback in
the options passed to useSendDataFetch that (1) calls the modal close function
used in this component (e.g., closeModal or closeCurrencyModal) and (2) calls
your React Query client to refresh the list (e.g., queryClient.invalidateQueries
with the lookups/currencies key or the appropriate query key used elsewhere);
ensure the onSuccess receives the response and still preserves any existing
success handling in the options object.
In `@src/client/shared/ui/ui-dialog/ui-dialog.tsx`:
- Around line 84-85: The hardcoded "Close" screen-reader label inside the shared
UI primitive should be made localizable: replace the literal text in the
DialogPrimitive.Close children (the <span className="sr-only">Close</span>
instances) by sourcing the string from your i18n layer or a passed-in prop
(e.g., add an optional closeLabel prop to the UiDialog component and fall back
to an i18n key like t('dialog.close')); update both occurrences that render
DialogPrimitive.Close to use that prop or t(...) value so the SR-only label is
configurable/localized.
---
Nitpick comments:
In `@src/client/features/admin/countries-lookup/countries-lookup.tsx`:
- Around line 140-152: The hidden-trigger pattern using UiConfirmModal +
singleDeleteTriggerRef is fragile and should be replaced with a controlled open
prop: update UiConfirmModal to accept an isOpen (boolean) and onClose callback
(and keep onConfirm), then in this file replace the hidden button/ref logic with
a local state (e.g. isSingleDeleteOpen) and setIsSingleDeleteOpen(true) to open
and setIsSingleDeleteOpen(false) in onClose; wire isSingleDeleteOpen to the new
isOpen prop and confirmSingleDelete to onConfirm (also close after confirm).
Update occurrences of singleDeleteTriggerRef, the hidden button, and any
setTimeout click hacks to use this new controlled open state and onClose
instead.
In `@src/client/shared/components/admin-modal/fin-admin-modal.tsx`:
- Around line 13-23: Rename the props interface to match the exported
component's Ui* naming: change FinAdminModalProps to UiAdminModalProps and
update its usage in the UiAdminModal function signature so the parameter type
references UiAdminModalProps; ensure any other references or imports (e.g.,
tests, consumers) that used FinAdminModalProps are updated to the new name to
keep naming consistent with the UI layer convention.
In `@src/client/shared/ui/ui-dialog/ui-dialog.tsx`:
- Around line 81-83: The close-button's utility selectors target SVG elements
but UiSvgIcon renders an <i>, so those styles never apply; update the close
button's className on the close element (where UiSvgIcon is used) to include
matching nested selectors for <i> as well as <svg> (e.g. add
[&_i]:pointer-events-none [&_i]:shrink-0 [&_i:not([class*='size-'])]:size-4 or
similar equivalents) so the pointer-events, sizing and shrink rules apply to
UiSvgIcon, or alternatively change UiSvgIcon to render an actual <svg> element
if you prefer that component-level fix (referencing UiSvgIcon and the close
button className).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39552ee9-5008-42a7-897d-b15ab8f434a1
📒 Files selected for processing (6)
src/client/features/admin/countries-lookup/countries-lookup.tsxsrc/client/features/admin/currencies-lookup/currencies-lookup.tsxsrc/client/features/admin/lookups/countries/country-form-modal.tsxsrc/client/features/admin/lookups/currencies/currency-form-modal.tsxsrc/client/shared/components/admin-modal/fin-admin-modal.tsxsrc/client/shared/ui/ui-dialog/ui-dialog.tsx
dmytroreshetylo
requested changes
Apr 29, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/server/database/migrations.ts (1)
3-3: ⚡ Quick winNormalize migration filename/import path (remove leading whitespace).
Line 3 imports from
migrations/ 1746000000000...(note the leading space). This is brittle and easy to break in tooling and future refs.♻️ Suggested change
-import { AddAdminNameToLookups1746000000000 } from '@backend/database/migrations/ 1746000000000-add-admin-name-to-lookups.migration'; +import { AddAdminNameToLookups1746000000000 } from '@backend/database/migrations/1746000000000-add-admin-name-to-lookups.migration';Also rename the migration file to remove the leading space from its filename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/database/migrations.ts` at line 3, The import path for AddAdminNameToLookups1746000000000 in migrations.ts contains a leading space ("migrations/ 1746000000000...") which breaks tooling; update the import to the normalized path without the space and also rename the physical migration file to remove the leading space in its filename so the module path and filename match (ensure the exported class AddAdminNameToLookups1746000000000 remains unchanged).src/common/domains/lookups/schemas/lookups-form.schema.ts (1)
11-13: ⚡ Quick winDisallow empty update payloads for PATCH schemas.
Both update schemas currently accept
{}, which makes no-op updates valid.Proposed fix
+const hasAtLeastOneDefinedField = (data: Record<string, unknown>): boolean => + Object.values(data).some((value) => value !== undefined); + export const UpdateCurrencySchema = CurrencyFormSchema.partial().extend({ softDeleted: z.union([z.literal(0), z.literal(1)]).optional(), -}); +}).refine(hasAtLeastOneDefinedField, { message: 'Потрібно передати хоча б одне поле для оновлення' }); @@ export const UpdateCountrySchema = CountryFormSchema.partial().extend({ softDeleted: z.union([z.literal(0), z.literal(1)]).optional(), -}); +}).refine(hasAtLeastOneDefinedField, { message: 'Потрібно передати хоча б одне поле для оновлення' });Also applies to: 24-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/domains/lookups/schemas/lookups-form.schema.ts` around lines 11 - 13, The UpdateCurrencySchema (built from CurrencyFormSchema.partial().extend(...)) currently allows an empty object for PATCH; update it to forbid empty payloads by adding a post-schema check (e.g., a .refine on the resulting schema) that ensures Object.keys(value).length > 0 and returns a clear error message when empty; apply the same non-empty-payload check to the other update schemas referenced around lines 24-26 so all PATCH schemas reject {}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/features/admin/currencies-lookup/currencies-lookup.tsx`:
- Around line 64-68: The success side-effects (showToast, clearSelection,
reload) are being run once per item because you call
bulkDeleteMutation.mutateAsync for each selected id; change the flow so you call
the mutation once for all IDs (or await all deletions and run side-effects once
afterwards). Specifically, update the code that iterates over selectedIds and
calls mutateAsync to instead call bulkDeleteMutation.mutateAsync(selectedIds)
(or Promise.all on individual mutateAsync calls) and move the onSuccess logic
(showToast, clearSelection, reload) to a single follow-up await or the
mutation’s onSuccess handler so it executes only once after all deletions
complete.
---
Nitpick comments:
In `@src/common/domains/lookups/schemas/lookups-form.schema.ts`:
- Around line 11-13: The UpdateCurrencySchema (built from
CurrencyFormSchema.partial().extend(...)) currently allows an empty object for
PATCH; update it to forbid empty payloads by adding a post-schema check (e.g., a
.refine on the resulting schema) that ensures Object.keys(value).length > 0 and
returns a clear error message when empty; apply the same non-empty-payload check
to the other update schemas referenced around lines 24-26 so all PATCH schemas
reject {}.
In `@src/server/database/migrations.ts`:
- Line 3: The import path for AddAdminNameToLookups1746000000000 in
migrations.ts contains a leading space ("migrations/ 1746000000000...") which
breaks tooling; update the import to the normalized path without the space and
also rename the physical migration file to remove the leading space in its
filename so the module path and filename match (ensure the exported class
AddAdminNameToLookups1746000000000 remains unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fbc2546-bf31-4ded-8510-0707c1e08f74
📒 Files selected for processing (20)
src/app/api/lookups/countries-and-locales/create/route.tssrc/app/api/lookups/countries-and-locales/update/[id]/route.tssrc/app/api/lookups/currencies/create/route.tssrc/app/api/lookups/currencies/update/[id]/route.tssrc/client/entities/lookups/lookup-row-actions/lookup-row-actions.tsxsrc/client/features/admin/countries-lookup/countries-lookup.tsxsrc/client/features/admin/currencies-lookup/currencies-lookup.tsxsrc/client/features/admin/lookups/countries/country-form-modal.tsxsrc/client/features/admin/lookups/currencies/currency-form-modal.tsxsrc/client/features/admin/lookups/hooks/use-country-mutations.hook.tssrc/client/features/admin/lookups/hooks/use-currency-mutations.hook.tssrc/client/shared/components/admin-modal/fin-admin-modal.scsssrc/client/shared/components/admin-modal/fin-admin-modal.tsxsrc/common/domains/lookups/schemas/lookups-form.schema.tssrc/common/records/countries.record.tssrc/common/records/currencies.record.tssrc/server/database/migrations.tssrc/server/database/migrations/ 1746000000000-add-admin-name-to-lookups.migration.tssrc/server/entities/country/infrastructure/country.orm.tssrc/server/entities/currency/infrastructure/currency.orm.ts
✅ Files skipped from review due to trivial changes (5)
- src/client/shared/components/admin-modal/fin-admin-modal.scss
- src/common/records/currencies.record.ts
- src/server/entities/country/infrastructure/country.orm.ts
- src/common/records/countries.record.ts
- src/client/features/admin/lookups/countries/country-form-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/features/admin/lookups/currencies/currency-form-modal.tsx
- src/client/features/admin/countries-lookup/countries-lookup.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/features/admin/countries-lookup/countries-lookup.tsx`:
- Around line 87-90: The confirmSingleDelete function currently calls
singleDeleteMutation.mutateAsync(itemToDelete.id) but doesn't remove that id
from the bulk selection state, leaving stale ids in selected; update
confirmSingleDelete (and/or the singleDeleteMutation success handler) to remove
the deleted item id from the selected collection (e.g., call the setter that
manages selected to filter out itemToDelete.id or delete it from the Set) after
the deletion resolves so bulk actions no longer include the deleted row.
- Around line 75-79: In confirmBulkDelete replace Promise.all(...) with
Promise.allSettled(...) so that all deletion attempts are awaited without
short-circuiting on the first rejection; inspect the returned results array from
Promise.allSettled to optionally log or aggregate failures (using
bulkDeleteMutation.mutateAsync for each id), then always call showToast,
clearSelection(), and reload() regardless of individual failures to keep UI in
sync; reference the confirmBulkDelete function and the selected set plus
bulkDeleteMutation to locate where to change the logic.
In `@src/client/features/admin/currencies-lookup/currencies-lookup.tsx`:
- Around line 90-93: When deleting a single row in confirmSingleDelete, ensure
you also remove that id from the bulk selection state so stale ids remain
selected; after awaiting singleDeleteMutation.mutateAsync(itemToDelete.id)
update the selection state (e.g., call setSelected(prev => prev.filter(id => id
!== itemToDelete.id)) or equivalent) and then clear itemToDelete as currently
done. This uses confirmSingleDelete, singleDeleteMutation and the component's
selected/setSelected state to keep bulk selection in sync after a single-row
delete.
- Around line 78-82: The confirmBulkDelete implementation currently uses
Promise.all which aborts on the first rejection; change it to use
Promise.allSettled to ensure all bulkDeleteMutation.mutateAsync(id) calls are
attempted, then inspect the results to determine if any deletions failed—if any
failed call show a destructive/error toast, otherwise show the success toast;
regardless of outcome always call clearSelection() and reload() afterwards.
Reference confirmBulkDelete, bulkDeleteMutation.mutateAsync, clearSelection,
reload, and showToast when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11517d8e-c62a-469a-9662-c3b92a20081d
📒 Files selected for processing (9)
src/app/api/lookups/countries-and-locales/update/[id]/route.tssrc/app/api/lookups/currencies/update/[id]/route.tssrc/client/entities/lookups/lookup-created-by/lookup-created-by.model.tssrc/client/entities/lookups/lookup-created-by/lookup-created-by.util.tssrc/client/entities/lookups/lookup-status-badge/lookup-status-badge.tsxsrc/client/features/admin/countries-lookup/countries-lookup.tsxsrc/client/features/admin/currencies-lookup/currencies-lookup.tsxsrc/client/features/admin/lookups/hooks/use-country-mutations.hook.tssrc/client/features/admin/lookups/hooks/use-currency-mutations.hook.ts
✅ Files skipped from review due to trivial changes (2)
- src/client/entities/lookups/lookup-created-by/lookup-created-by.model.ts
- src/client/entities/lookups/lookup-created-by/lookup-created-by.util.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/features/admin/lookups/hooks/use-currency-mutations.hook.ts
- src/app/api/lookups/currencies/update/[id]/route.ts
…for countries and currencies
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/lookups/countries-and-locales/remove/`[i]/route.ts:
- Line 10: The paramsFn in remove/[i]/route.ts is reading context.id which is
undefined because Next.js provides the dynamic param as i; update the paramsFn
to use context.i when calling GetIntegerParamPipe (e.g., paramsFn: (context) =>
({ id: GetIntegerParamPipe(context.i, 1) })), or alternatively rename the route
segment from [i] to [id] so context.id exists; apply the same fix to the
currencies remove route to ensure the dynamic param is read correctly.
In `@src/app/api/lookups/currencies/remove/`[i]/route.ts:
- Line 10: The params accessor in the route is using the wrong key: paramsFn
currently reads context.id but the dynamic segment is named [i], so Next.js will
provide params.i; update the paramsFn to use context.i (e.g., paramsFn:
(context) => ({ id: GetIntegerParamPipe(context.i, 1) })) or alternatively
rename the route segment to [id] so context.id is valid; ensure the change is
applied where paramsFn and GetIntegerParamPipe are referenced to avoid undefined
IDs.
In `@src/common/domains/lookups/schemas/lookups-form.schema.ts`:
- Around line 3-24: The current schemas accept whitespace-only strings because
they use z.string().min(1) and length(3) without trimming; update
CurrencyFormSchema and CountryFormSchema to trim input before validating (e.g.,
use z.string().trim().min(1, ...) for name, symbol, countryName, localeName and
z.string().trim().length(3, ...) for code) so whitespace-only values are
rejected; the existing UpdateCurrencySchema and UpdateCountrySchema (which use
.partial()) will inherit the fixed validators automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adb10a9a-2f42-4380-84ee-e3fa801ca0c8
📒 Files selected for processing (12)
src/app/api/lookups/countries-and-locales/remove/[i]/route.tssrc/app/api/lookups/currencies/remove/[i]/route.tssrc/client/features/admin/countries-lookup/countries-lookup.tsxsrc/client/features/admin/currencies-lookup/currencies-lookup.tsxsrc/client/features/admin/hooks/use-lookup-selection.hook.tssrc/client/features/admin/lookups/hooks/use-country-mutations.hook.tssrc/client/features/admin/lookups/hooks/use-currency-mutations.hook.tssrc/common/domains/lookups/schemas/lookups-form.schema.tssrc/server/entities/country/application/exist-country.guard.tssrc/server/entities/country/infrastructure/country.orm.tssrc/server/entities/currency/application/exist-currency.guard.tssrc/server/entities/currency/infrastructure/currency.orm.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/entities/currency/infrastructure/currency.orm.ts
- src/client/features/admin/lookups/hooks/use-country-mutations.hook.ts
- src/server/entities/country/infrastructure/country.orm.ts
- src/client/features/admin/lookups/hooks/use-currency-mutations.hook.ts
- src/client/features/admin/countries-lookup/countries-lookup.tsx
…s for improved tracking
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.ts (1)
3-3: 💤 Low valueMinor: Class timestamp differs from filename timestamp.
The class is named
AddAdminIdToLookups1746100000000but the file is1746000000000-add-admin-id-to-lookups.migration.ts. While this works (the import uses the filename), aligning them improves maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.ts` at line 3, Rename the migration class so its timestamp matches the filename: change the class name AddAdminIdToLookups1746100000000 to AddAdminIdToLookups1746000000000 (or alternatively rename the file to match the class), ensuring the exported class name and any references to it (the class declaration in src/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.ts) are consistent for easier maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/lookups/currencies/update/`[id]/route.ts:
- Around line 18-25: The handler currently allows an empty object (validated by
UpdateCurrencySchema.partial()) and performs a no-op update; modify the execute
function to detect when the request body contains no updatable fields (e.g. no
name, code, or symbol keys or all values are undefined) before calling
currencyRepository.updateItem, and return a 400 Bad Request with an explanatory
message instead of proceeding; add this check at the start of execute (using the
same param names: body, context, params: { id }, and context.currency) so you
only call currencyRepository.updateItem when at least one of body.name,
body.code, or body.symbol is provided.
In `@src/server/entities/currency/infrastructure/currency.orm.ts`:
- Around line 1-24: The file wrongly defines and exports CountryOrm for the
country table; replace it with a CurrencyOrm entity mapped to
`@Entity`('currency') and export CurrencyOrm instead of CountryOrm, implement
DefaultTableColumnsOrm, include the adminId : number | null and
`@ManyToOne`('UserOrm', { nullable: true, onDelete: 'SET NULL' }) with
`@JoinColumn`({ name: 'adminId' }) admin?: UserOrm, and ensure the entity exposes
the currencyCode field expected by UserOrm.currency (and any other
currency-specific columns like code/name as in other repos); keep the
OneToMany/relationships consistent with UserOrm.currency and update any unique
constraints or lengths to match currency requirements.
---
Nitpick comments:
In
`@src/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.ts`:
- Line 3: Rename the migration class so its timestamp matches the filename:
change the class name AddAdminIdToLookups1746100000000 to
AddAdminIdToLookups1746000000000 (or alternatively rename the file to match the
class), ensuring the exported class name and any references to it (the class
declaration in
src/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.ts)
are consistent for easier maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f60e5f3-e9ce-4c5c-b415-729296841a7a
📒 Files selected for processing (11)
src/app/api/lookups/countries-and-locales/create/route.tssrc/app/api/lookups/countries-and-locales/update/[id]/route.tssrc/app/api/lookups/currencies/create/route.tssrc/app/api/lookups/currencies/update/[id]/route.tssrc/common/records/countries.record.tssrc/common/records/currencies.record.tssrc/server/database/migrations.tssrc/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.tssrc/server/entities/country/infrastructure/country.orm.tssrc/server/entities/currency/infrastructure/currency.orm.tssrc/server/entities/currency/infrastructure/currency.repository.ts
✅ Files skipped from review due to trivial changes (5)
- src/common/records/currencies.record.ts
- src/common/records/countries.record.ts
- src/server/entities/currency/infrastructure/currency.repository.ts
- src/server/database/migrations.ts
- src/app/api/lookups/countries-and-locales/create/route.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/api/lookups/currencies/create/route.ts
- src/app/api/lookups/countries-and-locales/update/[id]/route.ts
…encyCode, and currencySymbol
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…ce data retrieval
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/client/features/admin/countries-lookup/countries-lookup.tsx (1)
71-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
Promise.allSettledto prevent partial failure from breaking UI sync.If any deletion in
Promise.allfails, the promise rejects immediately. Earlier successful deletions persist, butclearSelection()andreload()never run, leaving the UI inconsistent with the server. Replace withPromise.allSettledto ensure cleanup always occurs.🐛 Suggested fix
const confirmBulkDelete = async () => { - await Promise.all(Array.from(selected).map((id) => bulkDeleteMutation.mutateAsync(id))); - showToast({ title: 'Успішно', description: 'Вибрані записи видалено', variant: 'default' }); - clearSelection(); - reload(); + const results = await Promise.allSettled( + Array.from(selected).map((id) => deleteMutation.mutateAsync(id)), + ); + const failedCount = results.filter((r) => r.status === 'rejected').length; + clearSelection(); + reload(); + showToast( + failedCount === 0 + ? { title: 'Успішно', description: 'Вибрані записи видалено', variant: 'default' } + : { title: 'Помилка', description: `Не вдалося видалити ${failedCount} запис(ів)`, variant: 'destructive' }, + ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/features/admin/countries-lookup/countries-lookup.tsx` around lines 71 - 76, In confirmBulkDelete replace Promise.all with Promise.allSettled so one failing mutateAsync doesn't prevent cleanup; call Promise.allSettled(Array.from(selected).map(id => bulkDeleteMutation.mutateAsync(id))), then inspect results to optionally show failure toasts, but always run clearSelection() and reload() afterwards; reference confirmBulkDelete, bulkDeleteMutation.mutateAsync, clearSelection, and reload when making the change.
🧹 Nitpick comments (3)
src/client/features/admin/countries-lookup/countries-lookup.tsx (1)
61-63: ⚡ Quick winDuplicate mutation hook instances are unnecessary.
Two separate
useCountryMutations()calls create redundant mutation instances. A single call can provide the samedeleteMutationfor both bulk and single delete operations.♻️ Consolidate to single hook call
-const { deleteMutation: bulkDeleteMutation } = useCountryMutations(); - -const { deleteMutation: singleDeleteMutation } = useCountryMutations(); +const { deleteMutation } = useCountryMutations();Then use
deleteMutationin bothconfirmBulkDeleteandconfirmSingleDelete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/features/admin/countries-lookup/countries-lookup.tsx` around lines 61 - 63, Two calls to useCountryMutations() create duplicate mutation instances; consolidate by replacing both declarations with a single call like const { deleteMutation } = useCountryMutations() and then use that deleteMutation in both confirmBulkDelete and confirmSingleDelete (remove bulkDeleteMutation and singleDeleteMutation variables and update their usages to deleteMutation).src/app/api/lookups/countries-and-locales/remove/[id]/route.ts (1)
11-12: 💤 Low value
userIdis loaded but never used.The
userIdis extracted viaGetUserIdTransformerincontextFnbut isn't referenced inexecuteor passed for audit/logging. If audit tracking isn't needed here, remove it to avoid unnecessary async work. If it should be used (e.g., to record who deleted), wire it into the delete call.♻️ Remove unused context if not needed
contextFn: async (request, params) => ({ - userId: await GetUserIdTransformer(request), country: await countryRepository.getItemById(params.id), }),Or, if audit is intended, pass
userIdto the delete operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/lookups/countries-and-locales/remove/`[id]/route.ts around lines 11 - 12, The contextFn currently awaits GetUserIdTransformer(request) and sets userId but that value is never used; either remove the userId extraction from contextFn to avoid unnecessary async work, or if audit is required, thread the ID into the delete flow by passing the userId from context into the execute handler and into the repository/service delete method (e.g., update the delete call in execute to accept an actor or auditUserId and forward userId from contextFn), and ensure GetUserIdTransformer stays only when the execute/delete paths consume it.src/server/entities/country/infrastructure/country.repository.ts (1)
47-50: ⚡ Quick winType assertion masks a shape mismatch.
The
.map()produces plain objects with anadminNameproperty that doesn't exist onCountryOrm. Theas CountryOrm[]assertion hides this discrepancy and strips the ORM class prototype. If callers rely on ORM instance methods or the actual type contract, this will fail silently.Consider returning a proper response type that includes
adminName, or using a DTO/view model that matches the actual shape.♻️ Suggested approach
Define a return type that accurately reflects the shape:
+type CountryWithAdmin = CountryOrm & { adminName: string | null }; override async getItems( from: number, to: number, filters?: DeepPartial<CountriesAndLocalesFilter>, -): Promise<CountryOrm[]> { +): Promise<CountryWithAdmin[]> { // ... return results.map((item) => ({ ...item, adminName: item.admin?.name ?? null, - })) as CountryOrm[]; + })) as CountryWithAdmin[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/entities/country/infrastructure/country.repository.ts` around lines 47 - 50, The map currently creates plain objects with an extra adminName property and then coerces them to CountryOrm via "as CountryOrm[]", which hides a shape mismatch; instead, remove the unsafe cast and return a correctly typed structure: define a new DTO/interface (e.g., CountryWithAdminName) or view model that includes adminName, change the function return type to CountryWithAdminName[] and map results into that DTO (or construct proper CountryOrm instances and set adminName via a dedicated serializer if ORM methods are required), updating usages to consume the new type rather than forcing CountryOrm.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/lookups/user/get-by-id/`[id]/route.ts:
- Around line 15-24: The route currently only uses AuthGuard and allows any
authenticated user to fetch arbitrary user names in the execute function
(params.id); add an authorization check to prevent enumeration by enforcing
either a self-only policy (compare context.userId or context.user?.id to
params.id inside execute) or require an admin role before returning data (check
context.user?.role === 'admin'), and if the check fails return a 403 response.
Update the guards array or add the authorization logic at the top of execute
(referencing AuthGuard, execute, params.id, and context.userId/context.user) so
cross-user lookups are blocked unless the caller is the same user or has admin
privileges.
In `@src/client/entities/lookups/lookup-created-by/lookup-created-by-cell.tsx`:
- Around line 11-23: The avatar image and the fallback initial are currently
announcing the author's name twice (alt={name}) while the full name is rendered
in text; change the avatar's alt to an empty string (alt="") and add
aria-hidden="true" to the UiGraphic to make it decorative, and add
aria-hidden="true" to the fallback initial span (the inline-flex span) so only
the visible name span is exposed to assistive tech; update in
LookupCreatedByCell (UiGraphic usage and the fallback span) accordingly.
In `@src/client/features/admin/countries-lookup/countries-lookup.tsx`:
- Around line 83-90: The confirmSingleDelete function lacks error handling
around singleDeleteMutation.mutateAsync so an exception will leave itemToDelete
set and no error toast shown; wrap the mutateAsync call in try/catch and in the
catch call showToast with an error title/description (include the caught error
message if available), and ensure cleanup (deselect(itemToDelete.id),
setItemToDelete(null), and reload()) runs in a finally block so modal/state is
cleared regardless of success or failure.
---
Duplicate comments:
In `@src/client/features/admin/countries-lookup/countries-lookup.tsx`:
- Around line 71-76: In confirmBulkDelete replace Promise.all with
Promise.allSettled so one failing mutateAsync doesn't prevent cleanup; call
Promise.allSettled(Array.from(selected).map(id =>
bulkDeleteMutation.mutateAsync(id))), then inspect results to optionally show
failure toasts, but always run clearSelection() and reload() afterwards;
reference confirmBulkDelete, bulkDeleteMutation.mutateAsync, clearSelection, and
reload when making the change.
---
Nitpick comments:
In `@src/app/api/lookups/countries-and-locales/remove/`[id]/route.ts:
- Around line 11-12: The contextFn currently awaits
GetUserIdTransformer(request) and sets userId but that value is never used;
either remove the userId extraction from contextFn to avoid unnecessary async
work, or if audit is required, thread the ID into the delete flow by passing the
userId from context into the execute handler and into the repository/service
delete method (e.g., update the delete call in execute to accept an actor or
auditUserId and forward userId from contextFn), and ensure GetUserIdTransformer
stays only when the execute/delete paths consume it.
In `@src/client/features/admin/countries-lookup/countries-lookup.tsx`:
- Around line 61-63: Two calls to useCountryMutations() create duplicate
mutation instances; consolidate by replacing both declarations with a single
call like const { deleteMutation } = useCountryMutations() and then use that
deleteMutation in both confirmBulkDelete and confirmSingleDelete (remove
bulkDeleteMutation and singleDeleteMutation variables and update their usages to
deleteMutation).
In `@src/server/entities/country/infrastructure/country.repository.ts`:
- Around line 47-50: The map currently creates plain objects with an extra
adminName property and then coerces them to CountryOrm via "as CountryOrm[]",
which hides a shape mismatch; instead, remove the unsafe cast and return a
correctly typed structure: define a new DTO/interface (e.g.,
CountryWithAdminName) or view model that includes adminName, change the function
return type to CountryWithAdminName[] and map results into that DTO (or
construct proper CountryOrm instances and set adminName via a dedicated
serializer if ORM methods are required), updating usages to consume the new type
rather than forcing CountryOrm.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 419e2cbb-736f-48ac-92ca-cb1d3629c916
📒 Files selected for processing (21)
src/app/api/lookups/countries-and-locales/create/route.tssrc/app/api/lookups/countries-and-locales/remove/[id]/route.tssrc/app/api/lookups/countries-and-locales/update/[id]/route.tssrc/app/api/lookups/currencies/create/route.tssrc/app/api/lookups/currencies/remove/[id]/route.tssrc/app/api/lookups/currencies/update/[id]/route.tssrc/app/api/lookups/user/get-by-id/[id]/route.tssrc/client/entities/lookups/lookup-created-by/lookup-created-by-cell.tsxsrc/client/entities/lookups/lookup-created-by/lookup-created-by.model.tssrc/client/entities/lookups/lookup-created-by/lookup-created-by.util.tssrc/client/entities/lookups/lookup-table/lookup-table-row.tsxsrc/client/features/admin/countries-lookup/countries-lookup.tsxsrc/common/records/countries.record.tssrc/common/records/currencies.record.tssrc/server/database/migrations.tssrc/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.tssrc/server/entities/country/infrastructure/country.orm.tssrc/server/entities/country/infrastructure/country.repository.tssrc/server/entities/currency/infrastructure/currency.orm.tssrc/server/entities/currency/infrastructure/currency.repository.tssrc/server/entities/user/infrastructure/user.repository.ts
💤 Files with no reviewable changes (1)
- src/client/entities/lookups/lookup-created-by/lookup-created-by.model.ts
✅ Files skipped from review due to trivial changes (1)
- src/common/records/currencies.record.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/common/records/countries.record.ts
- src/app/api/lookups/currencies/create/route.ts
- src/server/database/migrations.ts
- src/app/api/lookups/countries-and-locales/create/route.ts
- src/server/entities/currency/infrastructure/currency.repository.ts
- src/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.ts
- src/app/api/lookups/countries-and-locales/update/[id]/route.ts
- src/app/api/lookups/currencies/update/[id]/route.ts
- src/server/entities/currency/infrastructure/currency.orm.ts
…ce data retrieval
…ng and mock responses
…ses and error handling
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/client/entities/lookups/lookups.service.ts (1)
60-65: 💤 Low valueMinor:
skipAuth: falseis redundant.Since
skipAuthdefaults tofalseinRequestOptions, this explicit assignment is unnecessary. Consider removing it for cleaner code, keeping onlythrowErrorIfNotAuth: false.{ signal: abortSignal ?? null, - skipAuth: false, throwErrorIfNotAuth: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/lookups/lookups.service.ts` around lines 60 - 65, The options object passed to the request includes a redundant skipAuth: false; remove the skipAuth property and leave the remaining options (signal: abortSignal ?? null and throwErrorIfNotAuth: false) so the RequestOptions default behavior is used. Locate the options object in src/client/entities/lookups/lookups.service.ts (around the request call that references abortSignal and throwErrorIfNotAuth) and delete the skipAuth: false entry only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/lookups/countries-and-locales/remove/`[id]/route.ts:
- Around line 17-24: The route's guards currently only verify authentication via
GetUserIdTransformer and existence via ExistCountryGuard but do not check admin
privileges; update the guards array to include an authorization guard that
enforces admin role before allowing deletion (e.g., add or create an AdminGuard
or RoleGuard and place it before calling countryRepository.deleteItem), ensure
the guard inspects the authenticated user object from GetUserIdTransformer (or
request/context) and returns { status: 403, message: 'Недостатньо прав' } when
the user is not an admin, and keep ExistCountryGuard to validate the target
exists before executing deleteItem.
In `@src/client/features/admin/hooks/use-admin-lookup.hook.ts`:
- Around line 75-88: The confirmSingleDelete function currently swallows errors
in its catch block; update the catch to display a destructive error toast
(matching confirmBulkDelete) when deleteMutation.mutateAsync fails — include a
clear title (e.g., 'Помилка') and description (e.g., 'Не вдалося видалити
запис') and optionally log or include the caught error message; keep the
existing finally behavior (setItemToDelete(null) and pagination.reload())
intact.
In `@src/client/shared/components/screen-handlers/fin-table-screen-handler.tsx`:
- Around line 48-54: FinTableScreenHandler currently delegates the no-data
rendering to FinListScreenHandler which returns a bare <span>, causing invalid
markup inside a <tbody>; update FinTableScreenHandler to override the
empty/no-data state by passing a table-safe emptyState (a <tr><td
colSpan="...">...</td></tr> or similar) into FinListScreenHandler props (or
implement an override in FinTableScreenHandler's render path) so that when
tableSkeleton/tableError are not used the no-data UI is wrapped in a table
row/cell and produces valid tbody markup.
In `@src/server/entities/country/infrastructure/country.orm.ts`:
- Around line 18-20: The relation on CountryOrm using admin?: UserOrm | null
with `@JoinColumn`(name: 'adminId') declares onDelete: 'SET NULL' but the
migration only creates a plain adminId column without a foreign key; update the
migration that creates/updates the countries table to add a proper FOREIGN KEY
constraint on adminId referencing the users (or UserOrm backing) primary key
with ON DELETE SET NULL (and keep adminId nullable), or add an ALTER TABLE ...
ADD CONSTRAINT step to create that FK if the table already exists so the DB
enforces referential integrity and SET NULL behavior when a user is deleted.
In `@src/server/shared/transformers/get-user-role.transformer.ts`:
- Around line 4-15: GetUserRoleTransformer currently returns payload.role cast
to RoleEnum without runtime validation, so malformed strings can leak through;
update GetUserRoleTransformer to validate payload.role at runtime against the
RoleEnum set (e.g., check that typeof payload.role === 'string' and that it
matches one of Object.values(RoleEnum) or use a dedicated isValidRole(type)
guard) and only return the role when it is a known RoleEnum value, otherwise
return null; reference the jwtVerify call and payload.role, JwtSecretConstant,
RoleEnum and isEmpty to locate and modify the transformer logic.
---
Nitpick comments:
In `@src/client/entities/lookups/lookups.service.ts`:
- Around line 60-65: The options object passed to the request includes a
redundant skipAuth: false; remove the skipAuth property and leave the remaining
options (signal: abortSignal ?? null and throwErrorIfNotAuth: false) so the
RequestOptions default behavior is used. Locate the options object in
src/client/entities/lookups/lookups.service.ts (around the request call that
references abortSignal and throwErrorIfNotAuth) and delete the skipAuth: false
entry only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e180dac-3e27-402c-ac33-750cc164b263
📒 Files selected for processing (36)
src/app/(profile)/profile/budget/(regular-operations)/regular-operations/edit/[id]/page.tsxsrc/app/api/lookups/countries-and-locales/get-items/route.tssrc/app/api/lookups/countries-and-locales/remove/[id]/route.tssrc/app/api/lookups/currencies/get-items/route.tssrc/app/api/lookups/currencies/update/[id]/route.tssrc/app/api/lookups/user/get-by-id/[id]/route.tssrc/client/entities/lookups/__tests__/lookups.service.spec.tssrc/client/entities/lookups/lookup-created-by/lookup-created-by-cell.tsxsrc/client/entities/lookups/lookup-created-by/lookup-created-by.util.tssrc/client/entities/lookups/lookup-table/lookup-row-skeleton.tsxsrc/client/entities/lookups/lookup-table/lookup-table.tsxsrc/client/entities/lookups/lookups.service.tssrc/client/entities/tracking-operations/tracking-operations.api.client.tssrc/client/features/admin/countries-lookup/countries-lookup.tsxsrc/client/features/admin/currencies-lookup/currencies-lookup.tsxsrc/client/features/admin/hooks/use-admin-lookup.hook.tssrc/client/features/regular-incomes-expenses/regular-incomes-expenses-screen.tsxsrc/client/shared/components/error/fin-error-table-widget.tsxsrc/client/shared/components/screen-handlers/fin-form-screen-handler.tsxsrc/client/shared/components/screen-handlers/fin-list-screen-handler.tsxsrc/client/shared/components/screen-handlers/fin-table-screen-handler.tsxsrc/client/shared/components/screen-handlers/props/form-screen-handler.props.tssrc/client/shared/components/screen-handlers/props/list-screen-handler.props.tssrc/client/shared/components/screen-handlers/props/table-screen-handler.props.tssrc/client/shared/services/fetch-client/fetch-client.service.tssrc/client/shared/services/fetch-client/models/request-options.model.tssrc/client/shared/ui/ui-table/ui-table.tsxsrc/common/domains/lookups/schemas/lookups-form.schema.tssrc/common/records/countries.record.tssrc/common/records/currencies.record.tssrc/server/database/migrations.tssrc/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.tssrc/server/entities/country/infrastructure/country.orm.tssrc/server/entities/country/infrastructure/country.repository.tssrc/server/entities/currency/infrastructure/currency.repository.tssrc/server/shared/transformers/get-user-role.transformer.ts
💤 Files with no reviewable changes (1)
- src/client/entities/lookups/lookup-table/lookup-row-skeleton.tsx
✅ Files skipped from review due to trivial changes (8)
- src/client/shared/components/screen-handlers/fin-form-screen-handler.tsx
- src/app/(profile)/profile/budget/(regular-operations)/regular-operations/edit/[id]/page.tsx
- src/client/shared/components/screen-handlers/fin-list-screen-handler.tsx
- src/client/shared/components/error/fin-error-table-widget.tsx
- src/server/database/migrations.ts
- src/client/shared/components/screen-handlers/props/table-screen-handler.props.ts
- src/client/features/regular-incomes-expenses/regular-incomes-expenses-screen.tsx
- src/common/domains/lookups/schemas/lookups-form.schema.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/common/records/countries.record.ts
- src/server/entities/country/infrastructure/country.repository.ts
- src/server/database/migrations/1746000000000-add-admin-id-to-lookups.migration.ts
- src/common/records/currencies.record.ts
- src/app/api/lookups/currencies/update/[id]/route.ts
…, implement role guard
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
dmytroreshetylo
requested changes
May 1, 2026
…ty in utility function
… updating getCreatedBy function
dmytroreshetylo
approved these changes
May 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
2 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.
Title
Feature: Implement Lookups Data Management Forms and Soft Delete
Type
Description
This PR implements the UI forms and logic for adding, editing, and deleting lookup data, specifically Countries/Locales and Currencies, while following the project’s FSD architecture and existing code style.
Components Created/Updated
UiAdminModal: Created a reusable base modal wrapper (@frontend/shared/components/admin-modal/fin-admin-modal) that handles the standard admin modal layout, accessible form submission viaformId, and loading states.CountryFormModal&CurrencyFormModal: Implemented lookup-specific form modals usingreact-hook-formand sharedzodvalidation schemas fromcommon/domains/lookups.CountriesLookup&CurrenciesLookup: Replaced page navigation for Add/Edit actions with inline modal forms.useCountryMutations&useCurrencyMutations: Added dedicated mutation hooks for create, update, and delete lookup operations.useAdminLookup: Extracted shared admin lookup table behavior, including pagination, selection, modal state, edit flow, and delete confirmation logic.Key Tasks Completed
initialData.Promise.allSettled.createdBydisplay for lookup tables by resolving the related admin fromadminId.ui-dialogimplementation in favor of the existing modal system.API / Backend Updates
adminIdsupport for lookup records.softDeleted: 1.Media
Check list
Addition
Implementation Note: To maintain the existing
UiConfirmModalcomponent's structure (which expects atriggernode) without modifying its source code, a hiddenuseReftrigger pattern was implemented in the lookup tables. This allows opening the confirmation modals programmatically upon table action clicks.Summary by CodeRabbit
New Features
Improvements