Feature/fin 022 parts/sidebar and lookups#98
Merged
dmytroreshetylo merged 37 commits intoApr 29, 2026
Merged
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds admin lookups UI: sidebar, lookup pages for countries and currencies, reusable lookup table components, selection hooks, role-based guard, SVG assets, and minor tooling/type adjustments (async user refresh, lint script quoting). Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant AdminPage as Lookup Page (Countries/Currencies)
participant LookupService as API / Service
participant LookupTable as Table + UI Components
User->>Browser: Navigate to /admin/lookups/countries
Browser->>AdminPage: mount page component
AdminPage->>LookupService: request page data (items)
AdminPage->>LookupService: request total count
LookupService-->>AdminPage: return items & count
AdminPage->>LookupTable: provide data, pagination, selection handlers
LookupTable->>LookupTable: render header, rows, status, created-by, actions
User->>LookupTable: click row checkbox / action
LookupTable->>AdminPage: toggle selection or navigate to edit route
AdminPage->>Browser: navigate to /admin/lookups/countries/edit/[id]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (19)
src/server/entities/user/infrastructure/user.orm.ts (1)
54-55: Minor: inconsistent optionality with sibling relation.
regularEntries!: RegularEntryOrm[]uses definite-assignment whiletrackingOperations?: TrackingOperationOrm[]is optional. Both are inverse-sideOneToManyrelations and behave identically at runtime (only populated when explicitly relation-loaded), so consider aligning them for consistency. The?form is arguably more honest since these are not eagerly loaded — that would suggest also updatingregularEntriesto optional rather than the other way around. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/entities/user/infrastructure/user.orm.ts` around lines 54 - 55, The two inverse OneToMany relations are inconsistent: trackingOperations is optional while regularEntries uses definite-assignment; change regularEntries to be optional as well so both relations reflect that they are not eagerly loaded. Locate the RegularEntryOrm relation named regularEntries in the User ORM class and replace the definite-assignment declaration (regularEntries!) with an optional declaration (regularEntries?: RegularEntryOrm[]), leaving the decorator (`@OneToMany`('RegularEntryOrm', 'user')) and type unchanged.src/common/enums/categories.enum.ts (1)
25-27: Optional:Setdeduplication is unnecessary.Values across
ExpenseCategoriesandIncomeCategoriesare unique by construction (expense-miscvsincome-misc), so theSetonly adds runtime cost without behavioral effect. A direct concatenation would suffice:♻️ Proposed simplification
-export const AllCategoryValues = Array.from( - new Set([...Object.values(ExpenseCategories), ...Object.values(IncomeCategories)]), -) as [AllCategories, ...AllCategories[]]; +export const AllCategoryValues = [ + ...Object.values(ExpenseCategories), + ...Object.values(IncomeCategories), +] as [AllCategories, ...AllCategories[]];That said, keeping
Setas a defensive guard against future name collisions is also reasonable — feel free to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/enums/categories.enum.ts` around lines 25 - 27, The current AllCategoryValues builds a Set from Object.values(ExpenseCategories) and Object.values(IncomeCategories) to deduplicate, which is unnecessary given the distinct naming; replace the Set construction with a direct concatenation of the two arrays to avoid the extra runtime cost (i.e., construct AllCategoryValues from [...Object.values(ExpenseCategories), ...Object.values(IncomeCategories)] and keep the existing type assertion to [AllCategories, ...AllCategories[]]); if you prefer to retain defensive deduplication, leave as-is—use ExpenseCategories, IncomeCategories, and AllCategoryValues to locate the change.src/server/entities/tracking-operation/application/owns-tracking-operation.guard.ts (1)
4-12: Defensive: distinguish missing op (404) from forbidden (403).When
opisnull, this returns 403 rather than 404. It's safe today because all routes invokeExistTrackingOperationGuardfirst, but if a future caller forgets to chain it, a missing resource will leak as "forbidden". Consider an explicit null check so the guard remains correct in isolation.🛡️ Proposed defensive tweak
export function OwnsTrackingOperationGuard( userId: number, op: TrackingOperationOrm | null, ): ApiResultOperationError | null { - if (op?.userId !== userId) { + if (!op) { + return { status: 404, message: 'Операцію не знайдено' }; + } + if (op.userId !== userId) { return { status: 403, message: 'Ця операція вам не належить!' }; } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/entities/tracking-operation/application/owns-tracking-operation.guard.ts` around lines 4 - 12, OwnsTrackingOperationGuard should explicitly treat a missing operation as "not found" instead of "forbidden": add a null check for the op parameter at the start of OwnsTrackingOperationGuard and return a 404 ApiResultOperationError (e.g. { status: 404, message: 'Операція не знайдена' } or similar) when op === null; keep the existing userId mismatch check that returns 403 when op.userId !== userId so the guard is correct both when paired with ExistTrackingOperationGuard and when used standalone.src/app/api/tracking-operation/get-items/route.ts (1)
14-18: RedundantsoftDeletedreassignment.
...filtersalready includessoftDeleted; line 16 re-assigns the exact same value. Drop it for clarity (the explicituserIdoverride after the spread is the only line that's actually needed).♻️ Proposed cleanup
const data = await trackingOperationRepository.getItems(from, to, { ...filters, - softDeleted: filters?.softDeleted, userId, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/tracking-operation/get-items/route.ts` around lines 14 - 18, The spread of filters already includes softDeleted, so remove the redundant explicit softDeleted: filters?.softDeleted in the call to trackingOperationRepository.getItems; keep the object as { ...filters, userId } so userId still overrides any value from filters and the call remains trackingOperationRepository.getItems(from, to, { ...filters, userId }).src/app/api/tracking-operation/get-by-id/[id]/route.ts (1)
15-19: UnusedoperationIdin context.
operationIdis set on the context but never read — guards andexecuteuseparams.idandcontext.opdirectly. The Ukrainian comment is misleading. Drop it to keep the context surface minimal.contextFn: async (request, params) => ({ userId: await GetUserIdTransformer(request), - operationId: params.id, // Передаємо id через контекст op: null as TrackingOperationOrm | null, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/tracking-operation/get-by-id/`[id]/route.ts around lines 15 - 19, Remove the unused operationId from the route context: in the contextFn that currently sets operationId, delete the operationId property (and the Ukrainian comment) so context only contains userId and op; confirm guards and the execute logic continue to use params.id and context.op (and no other code references operationId) to keep the context surface minimal and avoid dead fields tied to GetUserIdTransformer/TrackingOperationOrm.src/app/api/tracking-operation/remove/[id]/route.ts (2)
23-23: Redundant cast.
userIdis already typed asnumberat line 17, souserId as numberadds no value.- const ownsError = OwnsTrackingOperationGuard(userId as number, op); + const ownsError = OwnsTrackingOperationGuard(userId, op);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/tracking-operation/remove/`[id]/route.ts at line 23, Remove the redundant TypeScript cast on userId when calling OwnsTrackingOperationGuard: replace the call OwnsTrackingOperationGuard(userId as number, op) with OwnsTrackingOperationGuard(userId, op) so the already-typed userId (declared earlier) is passed directly; update any surrounding code/comments if they reference the cast.
14-28: Inconsistent with canonical guard/contextFn pattern in this repo.The sibling
get-by-idroute (andregular-entryroutes) fetch bothuserIdand the entity insidecontextFn, then runExistGuard/OwnsGuardas actualguards. Here you instead fetch and validate inline inexecute. Functionally equivalent, but inconsistent and harder to extend (e.g., adding more guards). Consider aligning:♻️ Proposed refactor
- contextFn: GetUserIdTransformer, - guards: [AuthGuard], - execute: async ({ context, params: { id } }) => { - const userId = context as number; - const op = await trackingOperationRepository.getItemById(id); - - const existError = ExistTrackingOperationGuard(op); - if (existError) return existError; - - const ownsError = OwnsTrackingOperationGuard(userId as number, op); - if (ownsError) return ownsError; - - await trackingOperationRepository.deleteItem(id, true); + contextFn: async (request, params) => ({ + userId: await GetUserIdTransformer(request), + op: null as TrackingOperationOrm | null, + }), + guards: [ + AuthGuard, + async ({ context, params }) => { + context.op = await trackingOperationRepository.getItemById(params.id); + return null; + }, + ({ context }) => ExistTrackingOperationGuard(context.op), + ({ context }) => OwnsTrackingOperationGuard(context.userId as number, context.op), + ], + execute: async ({ params: { id } }) => { + await trackingOperationRepository.deleteItem(id, true); return { status: 200, data: true }; },Based on learnings: in the finman repository, the established pattern for routes that require ownership checks is to fetch both
userIdand the entity together insidecontextFn, before guards run, with the regular-entry routes as the canonical reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/tracking-operation/remove/`[id]/route.ts around lines 14 - 28, This route implements user/entity fetching and ownership/existence checks inside execute rather than following the repo's canonical pattern; move entity lookup into the context transformer so guards run before execute: update GetUserIdTransformer (or create a new contextFn used here) to return { userId, op } by calling trackingOperationRepository.getItemById(id), replace the inline ExistTrackingOperationGuard and OwnsTrackingOperationGuard checks with those guards in the guards array (keeping AuthGuard), and keep execute only for the final delete call (trackingOperationRepository.deleteItem) and response; ensure symbol names GetUserIdTransformer, AuthGuard, ExistTrackingOperationGuard, OwnsTrackingOperationGuard, trackingOperationRepository.getItemById, deleteItem and execute are used to locate and update the code.src/app/api/tracking-operation/get-total-count/route.ts (1)
14-18: RedundantsoftDeletedproperty.
softDeletedis already included by...filters; restating it here adds no behavior and can be removed for clarity.♻️ Proposed refactor
const count = await trackingOperationRepository.getTotalCount({ ...filters, - softDeleted: filters?.softDeleted, userId, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/tracking-operation/get-total-count/route.ts` around lines 14 - 18, The call to trackingOperationRepository.getTotalCount builds its filter object by spreading filters and then redundantly re-specifying softDeleted; remove the duplicate property so the argument becomes the spread of filters plus userId (i.e., stop restating softDeleted after ...filters). Update the call site where getTotalCount is invoked to pass {...filters, userId} (or ensure filters already contains the intended softDeleted value) to keep the code clear and avoid redundant keys.src/server/database/migrations/1745000000000-add-tracking-operation.migration.ts (2)
51-54: Add an index onuserId(and consider(userId, softDeleted)).Every read path through
trackingOperationRepositoryfilters byuserId(list/count/get-by-id ownership checks), so a sequential scan on this table will degrade quickly as data grows. A composite index on(userId, softDeleted)would also accelerate the common "active operations for user X" path. You can declare it viaTable.indicesin this migration.♻️ Suggested addition
new Table({ name: 'tracking-operation', columns: [ // ... ], + indices: [ + { name: 'IDX_tracking_operation_user', columnNames: ['userId'] }, + { name: 'IDX_tracking_operation_user_softDeleted', columnNames: ['userId', 'softDeleted'] }, + ], }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/database/migrations/1745000000000-add-tracking-operation.migration.ts` around lines 51 - 54, The migration creates the tracking operation table but lacks an index on userId (and a composite one for userId+softDeleted), which will cause slow reads; update the migration in src/server/database/migrations/1745000000000-add-tracking-operation.migration.ts (inside the up migration that builds the Table) to add Table.indices entries for a single-column index on userId and a composite index on (userId, softDeleted) — i.e., modify the Table constructor or the createTable call in the migration's up method to include indices for 'userId' and ['userId','softDeleted'] so the trackingOperationRepository queries are indexed.
29-29: Consider specifying precision/scale for thenumericcolumn.
type: 'numeric'without precision results in PostgreSQL's arbitrary-precision numeric, which is correct but impractical for monetary values. Forsum, define it asnumeric(14,2)or similar to ensure consistent rounding and storage behavior, which aligns with standard practice for monetary columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/database/migrations/1745000000000-add-tracking-operation.migration.ts` at line 29, The migration currently creates a column with { name: 'sum', type: 'numeric' } which leaves PostgreSQL numeric unbounded; change the column definition to include precision/scale (e.g., { name: 'sum', type: 'numeric(14,2)' } or equivalent) so monetary values have fixed precision, and ensure the corresponding rollback/down logic (if it references the column type) remains consistent with the new definition; locate the column creation in the migration named 1745000000000-add-tracking-operation.migration and update it accordingly.src/common/domains/tracking-operation/schema/tracking-operation.schema.ts (1)
47-54: Note:itemsSchemabecomes aZodEffectsaftersuperRefineand loses object composition methods.Wrapping
basePaginatedSchema.itemsSchemawith.superRefine()produces aZodEffects<ZodObject<...>>, which prevents consumers from calling.extend(),.pick(),.omit(), etc. If future needs require extending the items schema (e.g., adding pagination parameters), consumers will have to referencebasePaginatedSchema.itemsSchemadirectly instead. Consider exporting both the raw schema and the refined variant, or applying the cross-field refinement through a wrappingz.object()to preserve composability for downstream extensions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/domains/tracking-operation/schema/tracking-operation.schema.ts` around lines 47 - 54, The current use of basePaginatedSchema.itemsSchema.superRefine in TrackingOperationPaginationSchema turns itemsSchema into a ZodEffects which blocks downstream composition; change this by keeping the original composable schema and applying the cross-field check via a wrapper or by exporting both forms: leave basePaginatedSchema.itemsSchema unchanged, create a separate refinedItemsSchema that performs the to>=from check (e.g., wrap basePaginatedSchema.itemsSchema inside a z.object(...) and apply .superRefine or create refinedItemsSchema = basePaginatedSchema.itemsSchema.withRefinement(...) ), then reference refinedItemsSchema in TrackingOperationPaginationSchema or export both basePaginatedSchema.itemsSchema and refinedItemsSchema so consumers can still call .extend/.pick/.omit on the raw itemsSchema while using the refined variant where needed.src/client/features/admin/admin-sidebar.tsx (1)
28-44: Minor a11y improvements: addaria-labelto<nav>andaria-currenton active link.Screen readers will announce the navigation landmark unlabeled, and the active route is currently only conveyed via color. Consider:
♻️ Proposed refactor
- <nav className="flex flex-col gap-0.5 p-3 flex-1"> + <nav aria-label="Довідники" className="flex flex-col gap-0.5 p-3 flex-1"> <p className="px-2 py-2 text-xs font-semibold uppercase tracking-wider text-muted-foreground">Довідники</p> - {LOOKUP_NAV_ITEMS.map((item) => ( - <Link - key={item.href} - href={item.href} + {LOOKUP_NAV_ITEMS.map((item) => { + const isActive = pathname === item.href; + return ( + <Link + key={item.href} + href={item.href} + aria-current={isActive ? 'page' : undefined} className={cn( 'flex items-center gap-2 rounded-md px-3 py-2 text-sm font-medium transition-colors', - pathname === item.href + isActive ? 'bg-sidebar-accent text-sidebar-accent-foreground' : 'text-sidebar-foreground hover:bg-sidebar-accent/60 hover:text-sidebar-accent-foreground', )} > {item.label} </Link> - ))} + ); + })} </nav>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/features/admin/admin-sidebar.tsx` around lines 28 - 44, Add an accessible label to the navigation and expose the active item to assistive tech: update the <nav> around LOOKUP_NAV_ITEMS to include an appropriate aria-label (e.g., "Довідники" or "Lookup navigation"), and when rendering each Link from LOOKUP_NAV_ITEMS, set aria-current="page" on the Link where pathname === item.href (keep the existing className logic intact so only the active item receives aria-current).src/client/entities/lookups/lookup-table/lookup-table.tsx (1)
70-78: Cell key collision when columns repeat — but more importantly, prefer a stable per-row + per-col key.
key={String(col.key)}inside a row is fine because columns are unique within a row. However, if the API later allows duplicatekeys (e.g. computed/derived columns), this silently produces React key warnings. Consider keying off the column index as a fallback or on a stable column id field. Optional nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/lookups/lookup-table/lookup-table.tsx` around lines 70 - 78, The per-cell key currently uses only String(col.key) which can collide when columns repeat; update the key on UiTableCell inside the items.map render (where UiTableRow, UiTableCell, columns, items and renderCell are used) to a stable per-row+per-col value — e.g. combine the row identifier (item.id) with either col.key or, if col.key is missing/duplicated, fall back to the column index (use the columns.map index) so each cell key becomes something like `${item.id}-${col.key ?? index}` to guarantee uniqueness and stability.src/client/features/admin/hooks/use-lookup-selection.hook.ts (1)
8-14: Avoid using a ternary purely for side effects.
next.has(id) ? next.delete(id) : next.add(id);evaluates a ternary expression only for its side effects, which most ESLint configs flag asno-unused-expressions. Anif/elseconveys intent more clearly.♻️ Proposed refactor
function toggleRow(id: number) { setSelected((prev) => { const next = new Set(prev); - next.has(id) ? next.delete(id) : next.add(id); + if (next.has(id)) { + next.delete(id); + } else { + next.add(id); + } return next; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/features/admin/hooks/use-lookup-selection.hook.ts` around lines 8 - 14, The toggleRow function uses a ternary solely for side effects: replace the expression `next.has(id) ? next.delete(id) : next.add(id);` with a clear conditional (if/else) to either call next.delete(id) or next.add(id) so the intent is explicit and avoids no-unused-expressions lint errors; update the body inside setSelected's updater (function toggleRow) to perform the mutation with an if/else and then return next.src/client/entities/lookups/lookup-status-badge/lookup-status-badge.tsx (1)
7-28: LGTM with one small polish.The badge logic is straightforward. The icon glyph (
✓/✕) is decorative since the textual status ("Active"/"Deleted") follows; consider marking the icon span witharia-hidden="true"so screen readers don't try to announce the symbol on top of the label.♿ Proposed fix
<span + aria-hidden="true" className={cn( 'w-4 h-4 rounded-full flex items-center justify-center text-white text-[10px]', isDeleted ? 'bg-red-500' : 'bg-green-500', )} > {isDeleted ? '✕' : '✓'} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/lookups/lookup-status-badge/lookup-status-badge.tsx` around lines 7 - 28, The icon character inside LookupStatusBadge is decorative and should be hidden from screen readers; update the inner span that renders the glyph (the span inside function LookupStatusBadge which conditionally shows '✕' or '✓') to include aria-hidden="true" so assistive tech won't announce the symbol alongside the textual label, keeping the outer text ("Deleted"/"Active") as the accessible status.src/client/entities/lookups/lookup-page-header/lookup-page-header.tsx (1)
11-15: Breadcrumb is a decorative<div>of<span>s.The "Lookups / {title}" sequence is structured as plain text. Consider rendering it as a
<nav aria-label="Breadcrumb">containing an ordered list (or at minimum, link the "Lookups" segment to/admin/lookupsif such an index page exists). Right now the leading "Lookups" segment is non-interactive even though it visually implies navigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/lookups/lookup-page-header/lookup-page-header.tsx` around lines 11 - 15, Replace the decorative div in the LookupPageHeader component with an accessible breadcrumb: change the wrapper to a <nav aria-label="Breadcrumb"> and render an <ol> of <li> items instead of plain spans; make the first segment ("Lookups") an interactive link to /admin/lookups (use your project's Link component or an <a>), keep the current classes for styling, and render the separator ("/") as visually shown but mark it aria-hidden so screen readers ignore it; ensure the current title value (title) is the final list item with font-semibold and not linked.src/client/shared/services/user-information/use-user-information.store.ts (1)
62-62:refreshaddition looks good; consider returning the promise.The new
refreshmember correctly delegates toloadUserInformation, which already manages the loading/success/error transitions. SinceloadUserInformationis async and callers (e.g.login/page.tsx) may want to await completion before navigating, consider returning the promise so awaits actually wait for state to settle:♻️ Proposed tweak
- refresh: () => loadUserInformation(), + refresh: () => loadUserInformation(),(no-op if
refreshis already typed as() => Promise<void>inUserInformationStore; just confirm the interface type matches and thatloadUserInformation's returned promise propagates.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/shared/services/user-information/use-user-information.store.ts` at line 62, The refresh property currently calls loadUserInformation() but should return its promise so callers can await completion; change refresh: () => loadUserInformation() to return the async result and ensure the UserInformationStore type for refresh is () => Promise<void> (or matches loadUserInformation's return type) so the promise propagates to callers like login/page.tsx that await refresh.src/client/entities/user-information/role-guard.tsx (2)
37-39: Render-phase fallback should not redirect; ensure children are not rendered for unauthorized users.The current branch correctly avoids rendering
childrenwhile the redirect is in flight — good. One small note: between the momentuserInfoStatebecomesSuccesswith no admin and the effect firing, this branch shows the "Доступ заборонено" message, which is the desired behavior. Consider extracting the user-facing strings to your i18n layer for consistency with the rest of the app (the codebase uses Ukrainian copy in multiple places — centralizing helps future localization).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/user-information/role-guard.tsx` around lines 37 - 39, Replace the hardcoded Ukrainian fallback string in the role guard with the centralized i18n key and ensure children remain unrendered for unauthorized users: in the component that checks userInformation and isAdmin (symbols: userInformation, isAdmin, children, role-guard), import the app's translation hook/function (e.g., useTranslation or t) and return a localized message like t('accessDenied.redirecting') instead of the literal "Доступ заборонено. Переадресація..."; keep the existing early-return branch so children are not rendered while the redirect/effect runs, and add the new key to the translations resource.
20-27: Redirect can fire on the same path (andpathNameis unused).
pathNameis read but never compared in the effect, yet it's listed as a dependency. If the user lands on/profileitself and is non-admin, this still callsrouter.push('/profile'), causing a redundant navigation. Consider either removingpathNamefrom the deps if it isn't needed, or guarding the push:♻️ Proposed tweak
- if (!userInformation || !isAdmin) { - router.push(routePath); - } + if ((!userInformation || !isAdmin) && pathName !== routePath) { + router.push(routePath); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/user-information/role-guard.tsx` around lines 20 - 27, The effect currently lists pathName in the dependency array but never uses it, and it will call router.push(routePath) even when already on that path causing redundant navigation; update the useEffect in role-guard.tsx to either remove pathName from the deps if it's not needed, or add a guard before calling router.push — e.g. check router.pathname (or pathName) !== routePath and only call router.push(routePath) when different; reference the useEffect, userInfoState, userInformation, isAdmin, router.push, routePath and pathName symbols to locate and apply the 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 `@src/app/`(auth)/login/page.tsx:
- Around line 22-25: The login flow currently calls refreshUser()
fire-and-forget so router.push('/profile') runs before the user's store is
loaded; change the callback passed to useSetupLogin to await refreshUser()
before calling router.push('/profile') (i.e., make the callback async and use
await refreshUser()), and ensure refreshUser (the refresh method on
UserInformationStore) is typed and implemented to return Promise<void> by
delegating to loadUserInformation() so the await actually waits for completion
(see UserInformationStore / loadUserInformation and user-information.model.ts
for the typing change).
In `@src/client/entities/lookups/lookup-page-header/lookup-page-header.tsx`:
- Around line 18-63: The icon-only buttons in the LookupPageHeader component
(the buttons using onDelete and onAdd) need accessible names and explicit types:
add type="button" to both <button> elements, add aria-label="Delete selected" to
the delete button and aria-label="Add new" to the add button, and mark the
contained <svg> elements as aria-hidden="true" so screen readers ignore
decorative SVGs; update the buttons that reference onDelete and onAdd
accordingly to include these attributes.
In `@src/client/entities/lookups/lookup-row-actions/lookup-row-actions.tsx`:
- Around line 13-15: The UiPopoverTrigger currently renders the decorative "···"
and lacks an accessible name; update the UiPopoverTrigger element (component:
UiPopoverTrigger) to include an aria-label (for example aria-label="Row
actions") so screen readers get a meaningful name; ensure the label is added
directly to the UiPopoverTrigger props and, if the component supports it, also
include aria-haspopup="menu" to indicate it opens a popover.
- Around line 21-57: In lookup-row-actions.tsx, update the two buttons (the
elements using onEdit and onDelete handlers) to include type="button", move the
hardcoded Ukrainian labels ("Редагувати"/"Видалити") to your i18n system (e.g.,
use the component's translation helper like t('lookup.edit')/t('lookup.delete')
or accept translated labels via props) so language matches other components, and
mark the decorative SVGs inside each button with aria-hidden="true" to prevent
duplicate screen reader announcements; keep the existing onEdit and onDelete
handlers unchanged.
In `@src/client/entities/lookups/lookup-table/lookup-table.tsx`:
- Around line 24-92: LookupTable is dead code; refactor CountriesLookup and
CurrenciesLookup to use LookupTable to eliminate duplicated
loading/error/empty-state/table logic. Replace the inline UiTable blocks in
CountriesLookup and CurrenciesLookup by importing LookupTable and passing title,
columns, items, isLoading, isError and a renderCell callback that handles
badges, row actions and checkboxes (or move shared date-formatting helpers into
LookupTable and call them from renderCell), remove the duplicate helper
functions and UiTable markup from both files, and ensure LookupTable<T extends
{id:number}> is exported and used by both components; alternatively if you
intentionally won't adopt it, delete lookup-table.tsx and remove any unused
exports.
In `@src/client/entities/user-information/role-guard.tsx`:
- Line 18: The isAdmin flag in RoleGuard is incorrectly hardcoded to true;
change it to derive from userInformation by checking the discriminant and role
(e.g., set isAdmin = userInformation.online === true && userInformation.role ===
'admin') so offline users (userInformation.online === false) are not treated as
admins; update the RoleGuard component to use this derived isAdmin and remove
isAdmin from the useEffect dependency array (leave userInformation as the dep)
so the guard actually restricts access based on the GetUser union's online and
role fields.
In `@src/client/features/admin/currencies-lookup/currencies-lookup.tsx`:
- Around line 102-104: The LookupRowActions component is rendered without
onEdit/onDelete handlers causing its popover menu items to be no-ops; update the
UiTableCell rendering to either pass the proper handlers (e.g., onEdit/onDelete
callbacks from the row/data context) into LookupRowActions or change the usage
to conditionally render LookupRowActions only when handlers exist, or render it
with disabled state when handlers are absent so the Edit/Delete menu items are
not interactive; locate the LookupRowActions usage in the UiTableCell and wire
it to your row-level edit/delete functions or guard its rendering accordingly.
- Around line 64-108: This file duplicates table header/rows/loading/empty logic
already encapsulated by LookupTable (and mirrors CountriesLookup); refactor
currencies-lookup.tsx to use the existing LookupTable component instead of
rendering the full UiTable block: replace the UiTable/UiTableHeader/UiTableBody
block with LookupTable and pass data, formatDate, and the selection handlers
(isSelected, toggleRow) plus slots/props for the checkbox column and row actions
(LookupRowActions) and the status cell (LookupStatusBadge); if LookupTable lacks
checkbox/row-action slots, extend lookup-table.tsx to accept render props like
renderRow or columns/renderCell so LookupTable can render the checkbox (using
isSelected/toggleRow), the LookupStatusBadge, createdAt/updatedAt via
formatDate, and the LookupRowActions, then update CountriesLookup similarly so
both use the shared component and wiring for onAdd/onEdit/onDelete becomes a
one-line prop pass.
- Around line 85-92: The row checkbox in the currencies table (inside
UiTableCell) lacks an accessible name; update the checkbox input used with
isSelected(item.id) and toggleRow(item.id) to include an accessible label by
adding an aria-label using the row data (e.g., aria-label={`Select
${item.currencyName}`}), or use aria-labelledby that references the cell showing
the currency name so screen readers announce the checkbox with its row context.
- Around line 44-47: The LookupPageHeader is rendering add/delete buttons but no
handlers are passed; update the currencies-lookup component to supply onAdd and
onDelete props (at minimum stub handlers like console.warn('TODO') or real
handlers), wire onDelete to call the bulk delete logic and then call the
selection clearing function from useLookupSelection (e.g., clearSelection or
resetSelection) so hasSelection and selected rows are cleared after a successful
delete, and ensure onAdd triggers the create flow or a TODO stub; reference
LookupPageHeader and useLookupSelection when making these changes.
- Around line 33-40: The query currently hardcodes pagination in useQuery
(queryFn calling lookupsService.getItems(LookupsTypeEnum.Currency, 1, 300, {})),
which silently truncates results and doesn't cancel in-flight requests; change
it so lookupsService.getItems is called without a fixed 1..300 window (or with
configurable pagination) and wire React Query's abort signal into the request:
update the queryFn signature to accept the React Query context (e.g., ({ signal
}) => ...) and pass that signal into lookupsService.getItems via its options
parameter; additionally either implement UI pagination/virtualization for the
currencies lookup or display a "showing N of M" count (use the returned total
from getItems) so users know results are truncated.
In `@src/client/shared/services/user-information/models/user-infomation.model.ts`:
- Line 10: Change the refresh type to return a Promise so callers can await it:
update the refresh signature in user-infomation.model.ts from () => void to ()
=> Promise<void>, since the implementation in use-user-information.store.ts sets
refresh to () => loadUserInformation() and loadUserInformation is async; ensure
any callers that rely on awaiting refresh continue to work with the new
Promise<void> return type.
In `@src/client/shared/ui/ui-table/ui-table-row.tsx`:
- Around line 9-12: In ui-table-row.tsx update the Tailwind variant used in the
cn(...) class list: replace the invalid token "has-aria-expanded:bg-muted/50"
with a valid Tailwind has- selector such as
"has-[aria-expanded=true]:bg-muted/50" (or "has-[[aria-expanded]]:bg-muted/50")
so the row highlights when a child is expanded; locate the cn(...) call in the
UI table row render and swap the malformed variant string accordingly (and
update any other occurrences of the same token).
In `@src/server/database/migrate.ts`:
- Around line 3-24: The migrate function currently both calls
DBDataSource.runMigrations() and relies on DBDataSource.initialize() (which runs
migrations via migrationsRun: true), and on failure it only logs the error
without setting a non-zero exit code; update migrate to remove the explicit
await DBDataSource.runMigrations() call to avoid double-running migrations and
in the catch block set process.exitCode = 1 (or call process.exit(1)) after
logging the error so CI sees failures; keep the DBDataSource.isInitialized
checks around initialize() and destroy() as-is to avoid closing an uninitialized
connection.
In
`@src/server/database/migrations/1745000000000-add-tracking-operation.migration.ts`:
- Line 28: The migration defines a column named 'date' with type 'date' which
mismatches JS Date semantics and can cause timezone off-by-one errors; update
the column type to a timestamp type (preferably 'timestamptz' for timezone-aware
storage) in the migration (1745000000000-add-tracking-operation.migration.ts)
and ensure the ORM/entity mapping for the 'date' field uses a Date/timestamp
type consistent with that change (or explicitly document/confirm
calendar-day-only semantics if you really want to keep 'date'); make the change
where the column definition { name: 'date', type: 'date' } appears and adjust
any related entity/transformer code handling that field.
In
`@src/server/entities/tracking-operation/infrastructure/tracking-operation.orm.ts`:
- Around line 19-26: The date column's transformer.write (the to function) is
timezone-sensitive because it uses toISOString().split('T')[0]; update the
transformer on the `@Column` for the date property so the to function builds the
YYYY-MM-DD string using UTC getters (e.g., date.getUTCFullYear(),
date.getUTCMonth()+1, date.getUTCDate()) to avoid local-tz shifts and keep
round-trip parity with the existing from function; modify the transformer.to
implementation only on the transformer object attached to the date property.
- Around line 28-35: The sum column's transformer uses parseFloat which loses
precision for monetary values; update the transformer on the sum property in
tracking-operation.orm (the `@Column` with transformer for sum) to use a Decimal
implementation (e.g., decimal.js or big.js) instead of parseFloat: have the
from() return a Decimal instance (or a string if you prefer keeping Decimal
creation deferred) and the to() convert the Decimal back to a string for
storage, or alternatively change the column to store integer minor units (cents)
and adjust the transformer to convert between number/BigInt and the DB type;
ensure the column remains numeric in Postgres (or use bigint for cents) and add
precision/scale constraints if you opt to keep floating numeric values.
---
Nitpick comments:
In `@src/app/api/tracking-operation/get-by-id/`[id]/route.ts:
- Around line 15-19: Remove the unused operationId from the route context: in
the contextFn that currently sets operationId, delete the operationId property
(and the Ukrainian comment) so context only contains userId and op; confirm
guards and the execute logic continue to use params.id and context.op (and no
other code references operationId) to keep the context surface minimal and avoid
dead fields tied to GetUserIdTransformer/TrackingOperationOrm.
In `@src/app/api/tracking-operation/get-items/route.ts`:
- Around line 14-18: The spread of filters already includes softDeleted, so
remove the redundant explicit softDeleted: filters?.softDeleted in the call to
trackingOperationRepository.getItems; keep the object as { ...filters, userId }
so userId still overrides any value from filters and the call remains
trackingOperationRepository.getItems(from, to, { ...filters, userId }).
In `@src/app/api/tracking-operation/get-total-count/route.ts`:
- Around line 14-18: The call to trackingOperationRepository.getTotalCount
builds its filter object by spreading filters and then redundantly re-specifying
softDeleted; remove the duplicate property so the argument becomes the spread of
filters plus userId (i.e., stop restating softDeleted after ...filters). Update
the call site where getTotalCount is invoked to pass {...filters, userId} (or
ensure filters already contains the intended softDeleted value) to keep the code
clear and avoid redundant keys.
In `@src/app/api/tracking-operation/remove/`[id]/route.ts:
- Line 23: Remove the redundant TypeScript cast on userId when calling
OwnsTrackingOperationGuard: replace the call OwnsTrackingOperationGuard(userId
as number, op) with OwnsTrackingOperationGuard(userId, op) so the already-typed
userId (declared earlier) is passed directly; update any surrounding
code/comments if they reference the cast.
- Around line 14-28: This route implements user/entity fetching and
ownership/existence checks inside execute rather than following the repo's
canonical pattern; move entity lookup into the context transformer so guards run
before execute: update GetUserIdTransformer (or create a new contextFn used
here) to return { userId, op } by calling
trackingOperationRepository.getItemById(id), replace the inline
ExistTrackingOperationGuard and OwnsTrackingOperationGuard checks with those
guards in the guards array (keeping AuthGuard), and keep execute only for the
final delete call (trackingOperationRepository.deleteItem) and response; ensure
symbol names GetUserIdTransformer, AuthGuard, ExistTrackingOperationGuard,
OwnsTrackingOperationGuard, trackingOperationRepository.getItemById, deleteItem
and execute are used to locate and update the code.
In `@src/client/entities/lookups/lookup-page-header/lookup-page-header.tsx`:
- Around line 11-15: Replace the decorative div in the LookupPageHeader
component with an accessible breadcrumb: change the wrapper to a <nav
aria-label="Breadcrumb"> and render an <ol> of <li> items instead of plain
spans; make the first segment ("Lookups") an interactive link to /admin/lookups
(use your project's Link component or an <a>), keep the current classes for
styling, and render the separator ("/") as visually shown but mark it
aria-hidden so screen readers ignore it; ensure the current title value (title)
is the final list item with font-semibold and not linked.
In `@src/client/entities/lookups/lookup-status-badge/lookup-status-badge.tsx`:
- Around line 7-28: The icon character inside LookupStatusBadge is decorative
and should be hidden from screen readers; update the inner span that renders the
glyph (the span inside function LookupStatusBadge which conditionally shows '✕'
or '✓') to include aria-hidden="true" so assistive tech won't announce the
symbol alongside the textual label, keeping the outer text ("Deleted"/"Active")
as the accessible status.
In `@src/client/entities/lookups/lookup-table/lookup-table.tsx`:
- Around line 70-78: The per-cell key currently uses only String(col.key) which
can collide when columns repeat; update the key on UiTableCell inside the
items.map render (where UiTableRow, UiTableCell, columns, items and renderCell
are used) to a stable per-row+per-col value — e.g. combine the row identifier
(item.id) with either col.key or, if col.key is missing/duplicated, fall back to
the column index (use the columns.map index) so each cell key becomes something
like `${item.id}-${col.key ?? index}` to guarantee uniqueness and stability.
In `@src/client/entities/user-information/role-guard.tsx`:
- Around line 37-39: Replace the hardcoded Ukrainian fallback string in the role
guard with the centralized i18n key and ensure children remain unrendered for
unauthorized users: in the component that checks userInformation and isAdmin
(symbols: userInformation, isAdmin, children, role-guard), import the app's
translation hook/function (e.g., useTranslation or t) and return a localized
message like t('accessDenied.redirecting') instead of the literal "Доступ
заборонено. Переадресація..."; keep the existing early-return branch so children
are not rendered while the redirect/effect runs, and add the new key to the
translations resource.
- Around line 20-27: The effect currently lists pathName in the dependency array
but never uses it, and it will call router.push(routePath) even when already on
that path causing redundant navigation; update the useEffect in role-guard.tsx
to either remove pathName from the deps if it's not needed, or add a guard
before calling router.push — e.g. check router.pathname (or pathName) !==
routePath and only call router.push(routePath) when different; reference the
useEffect, userInfoState, userInformation, isAdmin, router.push, routePath and
pathName symbols to locate and apply the change.
In `@src/client/features/admin/admin-sidebar.tsx`:
- Around line 28-44: Add an accessible label to the navigation and expose the
active item to assistive tech: update the <nav> around LOOKUP_NAV_ITEMS to
include an appropriate aria-label (e.g., "Довідники" or "Lookup navigation"),
and when rendering each Link from LOOKUP_NAV_ITEMS, set aria-current="page" on
the Link where pathname === item.href (keep the existing className logic intact
so only the active item receives aria-current).
In `@src/client/features/admin/hooks/use-lookup-selection.hook.ts`:
- Around line 8-14: The toggleRow function uses a ternary solely for side
effects: replace the expression `next.has(id) ? next.delete(id) : next.add(id);`
with a clear conditional (if/else) to either call next.delete(id) or
next.add(id) so the intent is explicit and avoids no-unused-expressions lint
errors; update the body inside setSelected's updater (function toggleRow) to
perform the mutation with an if/else and then return next.
In `@src/client/shared/services/user-information/use-user-information.store.ts`:
- Line 62: The refresh property currently calls loadUserInformation() but should
return its promise so callers can await completion; change refresh: () =>
loadUserInformation() to return the async result and ensure the
UserInformationStore type for refresh is () => Promise<void> (or matches
loadUserInformation's return type) so the promise propagates to callers like
login/page.tsx that await refresh.
In `@src/common/domains/tracking-operation/schema/tracking-operation.schema.ts`:
- Around line 47-54: The current use of
basePaginatedSchema.itemsSchema.superRefine in TrackingOperationPaginationSchema
turns itemsSchema into a ZodEffects which blocks downstream composition; change
this by keeping the original composable schema and applying the cross-field
check via a wrapper or by exporting both forms: leave
basePaginatedSchema.itemsSchema unchanged, create a separate refinedItemsSchema
that performs the to>=from check (e.g., wrap basePaginatedSchema.itemsSchema
inside a z.object(...) and apply .superRefine or create refinedItemsSchema =
basePaginatedSchema.itemsSchema.withRefinement(...) ), then reference
refinedItemsSchema in TrackingOperationPaginationSchema or export both
basePaginatedSchema.itemsSchema and refinedItemsSchema so consumers can still
call .extend/.pick/.omit on the raw itemsSchema while using the refined variant
where needed.
In `@src/common/enums/categories.enum.ts`:
- Around line 25-27: The current AllCategoryValues builds a Set from
Object.values(ExpenseCategories) and Object.values(IncomeCategories) to
deduplicate, which is unnecessary given the distinct naming; replace the Set
construction with a direct concatenation of the two arrays to avoid the extra
runtime cost (i.e., construct AllCategoryValues from
[...Object.values(ExpenseCategories), ...Object.values(IncomeCategories)] and
keep the existing type assertion to [AllCategories, ...AllCategories[]]); if you
prefer to retain defensive deduplication, leave as-is—use ExpenseCategories,
IncomeCategories, and AllCategoryValues to locate the change.
In
`@src/server/database/migrations/1745000000000-add-tracking-operation.migration.ts`:
- Around line 51-54: The migration creates the tracking operation table but
lacks an index on userId (and a composite one for userId+softDeleted), which
will cause slow reads; update the migration in
src/server/database/migrations/1745000000000-add-tracking-operation.migration.ts
(inside the up migration that builds the Table) to add Table.indices entries for
a single-column index on userId and a composite index on (userId, softDeleted) —
i.e., modify the Table constructor or the createTable call in the migration's up
method to include indices for 'userId' and ['userId','softDeleted'] so the
trackingOperationRepository queries are indexed.
- Line 29: The migration currently creates a column with { name: 'sum', type:
'numeric' } which leaves PostgreSQL numeric unbounded; change the column
definition to include precision/scale (e.g., { name: 'sum', type:
'numeric(14,2)' } or equivalent) so monetary values have fixed precision, and
ensure the corresponding rollback/down logic (if it references the column type)
remains consistent with the new definition; locate the column creation in the
migration named 1745000000000-add-tracking-operation.migration and update it
accordingly.
In
`@src/server/entities/tracking-operation/application/owns-tracking-operation.guard.ts`:
- Around line 4-12: OwnsTrackingOperationGuard should explicitly treat a missing
operation as "not found" instead of "forbidden": add a null check for the op
parameter at the start of OwnsTrackingOperationGuard and return a 404
ApiResultOperationError (e.g. { status: 404, message: 'Операція не знайдена' }
or similar) when op === null; keep the existing userId mismatch check that
returns 403 when op.userId !== userId so the guard is correct both when paired
with ExistTrackingOperationGuard and when used standalone.
In `@src/server/entities/user/infrastructure/user.orm.ts`:
- Around line 54-55: The two inverse OneToMany relations are inconsistent:
trackingOperations is optional while regularEntries uses definite-assignment;
change regularEntries to be optional as well so both relations reflect that they
are not eagerly loaded. Locate the RegularEntryOrm relation named regularEntries
in the User ORM class and replace the definite-assignment declaration
(regularEntries!) with an optional declaration (regularEntries?:
RegularEntryOrm[]), leaving the decorator (`@OneToMany`('RegularEntryOrm',
'user')) and type 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: a9a0eb20-911d-4c0a-803d-7a3865fae262
📒 Files selected for processing (50)
package.jsonsrc/app/(admin)/admin/lookups/countries/page.tsxsrc/app/(admin)/admin/lookups/currencies/page.tsxsrc/app/(admin)/admin/lookups/page.tsxsrc/app/(admin)/layout.tsxsrc/app/(auth)/login/page.tsxsrc/app/(auth)/login/shared/login-form.tssrc/app/(auth)/signup/shared/signup-form.tssrc/app/api/tracking-operation/create/route.tssrc/app/api/tracking-operation/get-by-id/[id]/route.tssrc/app/api/tracking-operation/get-items/route.tssrc/app/api/tracking-operation/get-total-count/route.tssrc/app/api/tracking-operation/remove/[id]/route.tssrc/app/api/tracking-operation/update/[id]/route.tssrc/client/entities/lookups/lookup-page-header/lookup-page-header.tsxsrc/client/entities/lookups/lookup-row-actions/lookup-row-actions.tsxsrc/client/entities/lookups/lookup-status-badge/lookup-status-badge.tsxsrc/client/entities/lookups/lookup-table/lookup-table.tsxsrc/client/entities/regular-entry/regular-entry.api.client.tssrc/client/entities/user-information/role-guard.tsxsrc/client/features/admin/admin-sidebar.tsxsrc/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/shared/services/user-information/models/user-infomation.model.tssrc/client/shared/services/user-information/use-user-information.store.tssrc/client/shared/ui/ui-table/ui-table-body.tsxsrc/client/shared/ui/ui-table/ui-table-caption.tsxsrc/client/shared/ui/ui-table/ui-table-cell.tsxsrc/client/shared/ui/ui-table/ui-table-footer.tsxsrc/client/shared/ui/ui-table/ui-table-head.tsxsrc/client/shared/ui/ui-table/ui-table-header.tsxsrc/client/shared/ui/ui-table/ui-table-row.tsxsrc/client/shared/ui/ui-table/ui-table.tsxsrc/common/domains/tracking-operation/filter/tracking-operation.filter.tssrc/common/domains/tracking-operation/schema/tracking-operation.schema.tssrc/common/enums/categories.enum.tssrc/common/records/tracking-operation.record.tssrc/server/database/crud.api.repository.tssrc/server/database/entities.tssrc/server/database/init.tssrc/server/database/migrate.tssrc/server/database/migrations.tssrc/server/database/migrations/1745000000000-add-tracking-operation.migration.tssrc/server/entities/tracking-operation/application/exist-tracking-operation.guard.tssrc/server/entities/tracking-operation/application/owns-tracking-operation.guard.tssrc/server/entities/tracking-operation/domain/tracking-operation-api.filter.tssrc/server/entities/tracking-operation/infrastructure/tracking-operation.orm.tssrc/server/entities/tracking-operation/infrastructure/tracking-operation.repository.tssrc/server/entities/user/infrastructure/user.orm.ts
…idebar-and-lookups
…d-lookups' into feature/fin-022-parts/sidebar-and-lookups
Contributor
Author
|
@coderabbitai I updated the threads. Please let me know if this comment is still relevant? |
🧩 Analysis chain🏁 Script executed: Length of output: 162610 🏁 Script executed: Length of output: 4504
🔴 Critical (1)
🟠 Major (9)
🟡 Minor (7)
If you've already addressed some of these in your latest commits, could you resolve the corresponding threads on GitHub so I can track what's done? Alternatively, I can trigger a fresh full review to check the current state of the code. ✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai I updated the code, please check again. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the current state of the updated files. 🏁 Script executed: Length of output: 29548 🏁 Script executed: Length of output: 2567 🏁 Script executed: Length of output: 274
✅ Fixed
❌ Still Open1. 2. } catch (error) {
console.error('Migration error:', error);
process.exit(1); // ← add this
}3. from: (value: string | null) => (value == null ? null : Number(value)), // or use Decimal4. // current
refreshUser();
router.push('/profile');
// safer
await refreshUser(); // requires refresh to return Promise<void>
router.push('/profile');5. refresh: () => Promise<void>; // not () => voidThis is also required to properly 6. 7. const LOOKUP_PAGE_SIZE = 300; // TODO: replace with real pagination |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the current state of all the files with outstanding issues. 🏁 Script executed: Length of output: 25793
✅ Now Fixed
❌ Still Open1. The } catch (error) {
console.error('Migration error:', error);
process.exit(1); // ← still missing
}2.
from: (value: string | null) => (value == null ? null : parseFloat(value)),
// consider: Number(value) or a Decimal library for financial precision3. The store implementation ( // current
refresh: () => void;
// should be
refresh: () => Promise<void>;4.
// current
refreshUser();
router.push('/profile');
// safer (requires refresh: () => Promise<void> in the interface)
await refreshUser();
router.push('/profile');5.
// current (broken)
'has-aria-expanded:bg-muted/50'
// correct
'has-[aria-expanded=true]:bg-muted/50'6.
7. Both still use const LOOKUP_PAGE_LIMIT = 300; // TODO: replace with real pagination |
Contributor
Author
|
@coderabbitai These 4 points are not relevant to my task (FIN-022): migrate.ts and tracking-operation.orm.ts are backend |
|
Tip For best results, initiate chat on the files or code changes.
That leaves 3 open items that are directly within FIN-022: 1. // broken
'has-aria-expanded:bg-muted/50'
// correct
'has-[aria-expanded=true]:bg-muted/50'2. 3. const LOOKUP_PAGE_LIMIT = 300; // TODO: replace with real paginationLet me know once those are addressed and I'll close out the threads! ✏️ Learnings added
🧠 Learnings used |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…idebar-and-lookups
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
dmytroreshetylo
requested changes
Apr 28, 2026
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/client/entities/lookups/lookup-table/lookup-table.tsx (1)
34-37: Remove or wire the unused feature-flag props.
withSelection,withAuditColumns, andwithActionsare declared but currently unused, which adds dead API surface. Either implement conditional rendering with them or drop them fromLookupTablePropsfor now.♻️ Minimal cleanup diff
interface LookupTableProps<T extends DefaultTableColumns> { @@ - withSelection?: boolean; - withAuditColumns?: boolean; - withActions?: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/lookups/lookup-table/lookup-table.tsx` around lines 34 - 37, LookupTableProps declares feature-flag props withSelection, withAuditColumns, and withActions that are not used; either remove them from LookupTableProps or wire them into LookupTable component to conditionally render selection UI, audit columns, and action buttons. Locate the LookupTableProps type and the LookupTable (or LookupTable component render/return) and implement simple conditional checks (e.g., if withSelection) to include the selection checkbox column, (if withAuditColumns) to include created/updated columns, and (if withActions) to render row action buttons, or remove the three props from the props type and any callers to keep the API surface clean.src/client/entities/lookups/lookup-table/lookup-row-skeleton.tsx (2)
5-7: MakecolumnWidthsimmutable in the prop contract.Small maintainability win: the component only reads widths, so
readonlybetter communicates intent.♻️ Proposed typing tweak
interface LookupRowSkeletonProps { - columnWidths: string[]; + columnWidths: readonly string[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/lookups/lookup-table/lookup-row-skeleton.tsx` around lines 5 - 7, The prop contract declares interface LookupRowSkeletonProps with columnWidths: string[] but the component only reads these values; make columnWidths immutable by changing its type to readonly string[] (or ReadonlyArray<string>) in LookupRowSkeletonProps and update any places that construct or pass columnWidths to satisfy the readonly type if needed (e.g., use as const or spread to a new array). Ensure the component and its usages refer to the same symbol (LookupRowSkeletonProps and columnWidths) so the intent is clear.
5-7: Optionally tighten width-class typing for safer call sites.You can prevent accidental non-width class values by typing
columnWidthsas width tokens.🧩 Proposed stricter type
+type WidthClass = `w-${number}`; + interface LookupRowSkeletonProps { - columnWidths: readonly string[]; + columnWidths: readonly WidthClass[]; }Also applies to: 20-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/entities/lookups/lookup-table/lookup-row-skeleton.tsx` around lines 5 - 7, The prop columnWidths in the LookupRowSkeletonProps interface is currently typed as string[]; narrow it to a WidthToken union (e.g., a union of the valid Tailwind width-class tokens you allow) and use WidthToken[] instead of string[]; add a local type alias WidthToken = 'w-1' | 'w-2' | ... (only the tokens you accept) and update all occurrences (including the other interface/prop block referenced around the same file, e.g., the other LookupRowSkeletonProps usage at lines ~20-27) and any call sites or components that construct columnWidths so they produce the narrower tokens. Ensure type imports/exports are updated if the type is consumed externally.src/client/widgets/admin-sidebar/admin-sidebar-content.tsx (1)
57-57: Remove the unnecessary type assertion on tooltip prop.On line 57,
item.name as stringis redundant. Sinceitemcomes fromadminNavRoutesWindow(typed asAdminNavItem[]), and theAdminNavIteminterface definesname: string, the assertion adds no type safety benefit. Remove theas stringcast.Current code
tooltip={item.name as string}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/widgets/admin-sidebar/admin-sidebar-content.tsx` at line 57, Remove the redundant type assertion on the tooltip prop: replace the usage of item.name as string with just item.name. Locate the prop assignment inside the AdminSidebarContent component (where tooltip is set for list items rendered from adminNavRoutesWindow / AdminNavItem[]), and update the tooltip prop to pass item.name directly.src/client/shared/svg/sidebar-toggle-svg.tsx (1)
18-45: Consider usinguseId()for theclipPathid to prevent collisions if this icon is mounted multiple times.The component currently uses a hardcoded id (
sidebarToggleClipat lines 18 and 39). While not an issue for single-instance use, if this icon is rendered multiple times in the same DOM, id duplicates will cause SVG reference collisions. Use React'suseId()hook to generate a unique id per instance.Suggested refactor
-import type { SVGProps } from 'react'; +import { useId, type SVGProps } from 'react'; export function SidebarToggleSvg({ width = 28, height = 28, viewBox = '0 0 28 28', ...props }: SVGProps<SVGSVGElement>) { + const clipPathId = useId(); + return ( <svg width={width} height={height} viewBox={viewBox} fill="none" xmlns="http://www.w3.org/2000/svg" {...props} > - <g clipPath="url(`#sidebarToggleClip`)"> + <g clipPath={`url(#${clipPathId})`}> ... </g> <defs> - <clipPath id="sidebarToggleClip"> + <clipPath id={clipPathId}> <rect width="28" height="28" fill="white" /> </clipPath> </defs> </svg> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/shared/svg/sidebar-toggle-svg.tsx` around lines 18 - 45, The SVG uses a hardcoded clipPath id "sidebarToggleClip" which can collide when multiple instances mount; update the component to call React's useId() (import from react) to generate a unique id and replace both occurrences of "sidebarToggleClip" (the clipPath id and the clipPath="url(#...)" reference) with that generated id (e.g., `${id}-sidebarToggleClip`) so each instance gets a distinct clipPath; ensure the symbol names to change are the clipPath element id and the clipPath="url(`#sidebarToggleClip`)" attribute in this component.
🤖 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/entities/lookups/lookup-table/lookup-table-row.tsx`:
- Line 43: The ID column is still being rendered via the UiTableCell that
outputs item.id; update the LookupTableRow component to exclude the id field
from rendering (remove or conditionally skip the UiTableCell that references
item.id) so lookup tables show all columns except id; locate the UiTableCell
with className "w-16 py-2 text-sm text-muted-foreground" and remove it or wrap
it in a condition that prevents rendering of item.id.
In `@src/client/entities/profile/role-guard.tsx`:
- Line 20: The admin role check is too strict because it requires
userInformation?.online; update the logic in RoleGuard by changing the isAdmin
computation to only verify the user's role (e.g., set isAdmin =
!!(userInformation && userInformation.role === RoleEnum.Admin)) so admin users
are authorized regardless of the online flag; locate and update the isAdmin
variable in role-guard.tsx (and mirror similar role checks for other roles if
present) to remove the online requirement.
In `@src/client/shared/svg/logo-inverse-svg.tsx`:
- Around line 21-29: Remove the invalid "!important" tokens from the SVG path
stroke attributes in logo-inverse-svg.tsx (the two occurrences of
stroke="var(--primary) !important"), replacing them with just
stroke="var(--primary)"; if you need higher precedence, move that rule into a
CSS selector targeting the SVG (or inline style object) rather than embedding
"!important" in the attribute so the stroke value parses correctly.
In `@src/client/shared/utils/lookup-date.util.tsx`:
- Around line 6-7: The current empty-date guard uses the falsy check "if
(!date)" which treats numeric 0 (Unix epoch) as empty; update the guard to only
treat null/undefined (and optionally empty string) as empty so 0 is preserved.
Replace "if (!date) return '—';" with an explicit check such as "if (date ===
null || date === undefined || date === '') return '—';" in the lookup-date util
where the "date" parameter is validated.
In `@src/client/widgets/admin-sidebar/admin-sidebar-content.tsx`:
- Line 49: The active-route check using pathname.startsWith(item.route) can
produce false positives for prefix collisions; update the logic in the
admin-sidebar-content component (the isActive calculation that references
pathname and item.route) to only mark active when pathname === item.route or
when pathname starts with item.route + '/' (or otherwise ensures a path-segment
boundary), so `/admin/lookups/countries-archive` won’t match
`/admin/lookups/countries`; implement the conditional change where isActive is
computed.
---
Nitpick comments:
In `@src/client/entities/lookups/lookup-table/lookup-row-skeleton.tsx`:
- Around line 5-7: The prop contract declares interface LookupRowSkeletonProps
with columnWidths: string[] but the component only reads these values; make
columnWidths immutable by changing its type to readonly string[] (or
ReadonlyArray<string>) in LookupRowSkeletonProps and update any places that
construct or pass columnWidths to satisfy the readonly type if needed (e.g., use
as const or spread to a new array). Ensure the component and its usages refer to
the same symbol (LookupRowSkeletonProps and columnWidths) so the intent is
clear.
- Around line 5-7: The prop columnWidths in the LookupRowSkeletonProps interface
is currently typed as string[]; narrow it to a WidthToken union (e.g., a union
of the valid Tailwind width-class tokens you allow) and use WidthToken[] instead
of string[]; add a local type alias WidthToken = 'w-1' | 'w-2' | ... (only the
tokens you accept) and update all occurrences (including the other
interface/prop block referenced around the same file, e.g., the other
LookupRowSkeletonProps usage at lines ~20-27) and any call sites or components
that construct columnWidths so they produce the narrower tokens. Ensure type
imports/exports are updated if the type is consumed externally.
In `@src/client/entities/lookups/lookup-table/lookup-table.tsx`:
- Around line 34-37: LookupTableProps declares feature-flag props withSelection,
withAuditColumns, and withActions that are not used; either remove them from
LookupTableProps or wire them into LookupTable component to conditionally render
selection UI, audit columns, and action buttons. Locate the LookupTableProps
type and the LookupTable (or LookupTable component render/return) and implement
simple conditional checks (e.g., if withSelection) to include the selection
checkbox column, (if withAuditColumns) to include created/updated columns, and
(if withActions) to render row action buttons, or remove the three props from
the props type and any callers to keep the API surface clean.
In `@src/client/shared/svg/sidebar-toggle-svg.tsx`:
- Around line 18-45: The SVG uses a hardcoded clipPath id "sidebarToggleClip"
which can collide when multiple instances mount; update the component to call
React's useId() (import from react) to generate a unique id and replace both
occurrences of "sidebarToggleClip" (the clipPath id and the clipPath="url(#...)"
reference) with that generated id (e.g., `${id}-sidebarToggleClip`) so each
instance gets a distinct clipPath; ensure the symbol names to change are the
clipPath element id and the clipPath="url(`#sidebarToggleClip`)" attribute in this
component.
In `@src/client/widgets/admin-sidebar/admin-sidebar-content.tsx`:
- Line 57: Remove the redundant type assertion on the tooltip prop: replace the
usage of item.name as string with just item.name. Locate the prop assignment
inside the AdminSidebarContent component (where tooltip is set for list items
rendered from adminNavRoutesWindow / AdminNavItem[]), and update the tooltip
prop to pass item.name directly.
🪄 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: 7c4970d4-888b-47b8-98bc-fb912af70781
📒 Files selected for processing (38)
package.jsonsrc/app/(admin)/layout.tsxsrc/app/(auth)/login/page.tsxsrc/app/(auth)/login/shared/login-form.tssrc/app/(profile)/profile/budget/(regular-operations)/layout.tsxsrc/client/entities/lookups/lookup-column/lookup-column.model.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-page-header/lookup-page-header.tsxsrc/client/entities/lookups/lookup-row-actions/lookup-row-actions.tsxsrc/client/entities/lookups/lookup-status-badge/lookup-status-badge.tsxsrc/client/entities/lookups/lookup-table/lookup-row-skeleton.tsxsrc/client/entities/lookups/lookup-table/lookup-table-head.tsxsrc/client/entities/lookups/lookup-table/lookup-table-row.tsxsrc/client/entities/lookups/lookup-table/lookup-table.tsxsrc/client/entities/profile/role-guard.tsxsrc/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/regular-incomes-expenses/card-creation-form/cards-form-side-block/useful-tips-block/useful-tips-block.tsxsrc/client/shared/models/admin-nav-item.model.tssrc/client/shared/services/user-information/models/user-infomation.model.tssrc/client/shared/svg/logo-inverse-svg.tsxsrc/client/shared/svg/sidebar-countries-svg.tsxsrc/client/shared/svg/sidebar-currencies-svg.tsxsrc/client/shared/svg/sidebar-toggle-svg.tsxsrc/client/shared/ui/ui-sidebar/styles/sidebar-menu-button-variant-mixin.scsssrc/client/shared/ui/ui-sidebar/styles/sidebar-styles.scsssrc/client/shared/ui/ui-sidebar/ui-sidebar-header-title.tsxsrc/client/shared/ui/ui-sidebar/ui-sidebar.tsxsrc/client/shared/utils/lookup-date.util.tsxsrc/client/widgets/admin-sidebar/admin-sidebar-content.tsxsrc/client/widgets/admin-sidebar/admin-sidebar.tsxsrc/client/widgets/profile-sidebar/profile-sidebar.tsxsrc/client/widgets/shared/admin-routes.constant.tssrc/common/domains/regular-entry/schema/regular-entry.schema.tssrc/server/entities/user/infrastructure/user.orm.ts
💤 Files with no reviewable changes (1)
- src/client/features/regular-incomes-expenses/card-creation-form/cards-form-side-block/useful-tips-block/useful-tips-block.tsx
✅ Files skipped from review due to trivial changes (10)
- src/app/(profile)/profile/budget/(regular-operations)/layout.tsx
- src/common/domains/regular-entry/schema/regular-entry.schema.ts
- src/client/shared/ui/ui-sidebar/ui-sidebar-header-title.tsx
- src/client/widgets/profile-sidebar/profile-sidebar.tsx
- src/client/entities/lookups/lookup-column/lookup-column.model.ts
- src/server/entities/user/infrastructure/user.orm.ts
- package.json
- src/app/(auth)/login/shared/login-form.ts
- src/client/shared/ui/ui-sidebar/styles/sidebar-styles.scss
- src/client/entities/lookups/lookup-created-by/lookup-created-by.model.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/app/(auth)/login/page.tsx
- src/client/features/admin/countries-lookup/countries-lookup.tsx
- src/client/entities/lookups/lookup-row-actions/lookup-row-actions.tsx
- src/client/features/admin/hooks/use-lookup-selection.hook.ts
- src/app/(admin)/layout.tsx
- src/client/entities/lookups/lookup-status-badge/lookup-status-badge.tsx
- src/client/entities/lookups/lookup-page-header/lookup-page-header.tsx
- src/client/features/admin/currencies-lookup/currencies-lookup.tsx
- src/client/shared/services/user-information/models/user-infomation.model.ts
…ting and UI elements
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
dmytroreshetylo
approved these changes
Apr 29, 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.
Type
Description
Closes #35
RoleGuard — a component analogous to
AuthGuardthat additionally checksuserInformation.role === RoleEnum.Admin. Redirects to/profileif the user is not an admin.AdminSidebar — sidebar with the Finman logo and app name at the top, and navigation links to lookup pages below. Active link is highlighted via
usePathname().(admin) layout — new route group that wraps pages in
AuthGuard → RoleGuardand renders theAdminSidebaralongside a<main>content area.LookupStatusBadge — shared entity component that renders a colored badge (✓ Active / ✕ Deleted) based on the
softDeletedfield.LookupPageHeader — shared entity component with breadcrumb navigation and action buttons (add
+, delete 🗑️ when rows are selected).LookupRowActions — popover menu on each row (triggered by
···) built withUiPopover*components. Contains Edit and Delete actions.useLookupSelection — reusable hook for checkbox row selection logic, shared between lookup pages.
CountriesLookup — feature component for
/admin/lookups/countries. Displays all columns exceptid:softDeleted,createdAt,updatedAt,country,locale.CurrenciesLookup — feature component for
/admin/lookups/currencies. Displays all columns exceptid:softDeleted,createdAt,updatedAt,currencyName,currencyCode,currencySymbol.Media
Check list
Addition
Summary by CodeRabbit
New Features
Bug Fixes
UI/Style Updates