Feature/fin 031#74
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a complete pagination system with a custom React hook ( Changes
Sequence Diagram(s)sequenceDiagram
participant Page as RegularIncomesExpenses
participant Hook as usePaginationResource
participant RQ as React Query
participant Mocks as Mock Data
Page->>Hook: Initialize with config (queryKey, getOptionsFn, getTotalCountFn)
Hook->>Hook: Set selectedPage = 1
Hook->>RQ: Create query for total count (getTotalCountFn)
Hook->>RQ: Create query for page options (getOptionsFn, page 1)
RQ->>Mocks: Fetch total count
RQ->>Mocks: Fetch options for page 1
Mocks-->>RQ: Return totalCount
Mocks-->>RQ: Return options
RQ-->>Hook: Success state + data
Hook-->>Page: Return PaginationResource (state, options, totalCount)
Page->>Page: Render cards + FinPagination
Page->>FinPagination: Pass selectedPage, setPage, totalCount, pageSize
FinPagination->>FinPagination: Compute showPages range
User->>Page: Click page number
Page->>Hook: Call setPage(newPage)
Hook->>RQ: Update queries with new page
RQ->>Mocks: Fetch new page options
Mocks-->>RQ: Return new options
RQ-->>Hook: Success state + new data
Hook-->>Page: Rerender with updated options
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
Member
Author
|
@CodeRabbit 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: 4
🧹 Nitpick comments (3)
src/client/shared/hooks/pagination-resource/pagination-resource.hook.ts (1)
49-59: Error aggregation can be simplified.Current logic works, but
find((error) => !!error)plusisEmptyis redundant.Simpler equivalent
const errorMessage = useMemo(() => { - const errors = [getOptionsQuery.error, getTotalCountQuery.error]; - - const error = errors.find((error) => !!error); - - if (isEmpty(error)) { - return undefined; - } - - return getErrorMessage(error); + const error = getOptionsQuery.error ?? getTotalCountQuery.error; + return isEmpty(error) ? undefined : getErrorMessage(error); }, [getOptionsQuery.error, getTotalCountQuery.error]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/shared/hooks/pagination-resource/pagination-resource.hook.ts` around lines 49 - 59, The error aggregation in the useMemo for errorMessage is redundant: remove the find + isEmpty pattern and instead directly select the first non-null/undefined error (e.g., via nullish coalescing between getOptionsQuery.error and getTotalCountQuery.error) and, if present, pass it to getErrorMessage; update the useMemo body for errorMessage to compute const error = getOptionsQuery.error ?? getTotalCountQuery.error; return error ? getErrorMessage(error) : undefined, keeping the same dependencies.src/client/shared/components/pagination/fin-pagination.tsx (1)
40-43: Guard page bounds in click handlers.Even with disabled UI, clamping in callbacks prevents accidental
setPage(0)/ overflow if action components change behavior later.Hardening change
<UiPaginationPrevious disabled={selectedPage <= 1} - onClick={() => setPage(selectedPage - 1)} + onClick={() => setPage(Math.max(1, selectedPage - 1))} /> ... <UiPaginationNext disabled={selectedPage >= totalPages} - onClick={() => setPage(selectedPage + 1)} + onClick={() => setPage(Math.min(totalPages, selectedPage + 1))} />Also applies to: 58-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/shared/components/pagination/fin-pagination.tsx` around lines 40 - 43, Guard the page bounds inside the click handlers for pagination: instead of calling setPage(selectedPage - 1) or setPage(selectedPage + 1) directly in UiPaginationPrevious and UiPaginationNext handlers, compute a clamped value (e.g., newPage = Math.max(1, Math.min(totalPages, selectedPage - 1)) and similarly for +1) and pass that to setPage; alternatively add a small clampPage helper used by the onClick for UiPaginationPrevious (current handler) and UiPaginationNext (lines ~58-61) so setPage never receives 0 or >totalPages even if the button behavior changes.src/client/features/regular-incomes-expenses/regular-incomes-expenses-page-copy.tsx (1)
7-7: Track the TODO with an issue before merge.If this page is temporary, please link a cleanup task so it doesn’t linger unnoticed.
I can draft a follow-up issue template for removal if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/features/regular-incomes-expenses/regular-incomes-expenses-page-copy.tsx` at line 7, This file contains a TODO comment indicating the component is temporary; before merging, create a tracking issue (GitHub/GitLab/JIRA) to remove the component and add the issue reference back into the source by replacing the TODO comment in regular-incomes-expenses-page-copy.tsx with a short note that includes the new issue ID/URL and expected cleanup conditions, and also add the issue link to the PR description so reviewers can verify the planned follow-up removal.
🤖 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/shared/components/pagination/fin-pagination.tsx`:
- Around line 47-52: UiPaginationItem is being rendered with the disabled prop
unconditionally, making all numeric page buttons non-clickable; update the
render in fin-pagination.tsx so that disabled is only set when appropriate (for
example when page === selectedPage or when a prop explicitly indicates the
control should be disabled) and ensure onClick uses setPage(page) for enabled
items; look for UiPaginationItem, selectedPage, setPage and page in the
component to apply the conditional disabled logic so active/current page or
global disabled state is handled correctly.
In `@src/client/shared/hooks/pagination-resource/pagination-resource.hook.ts`:
- Around line 31-33: The effect currently resets page on any reference change
because useEffect depends on the filters object reference; change the dependency
to match the query key logic by depending on the same serialized value used in
the query keys (e.g., the JSON.stringify(filters) string) so setPage(1) only
runs when filter content actually changes; update the useEffect dependency array
to use the serialized filters value (keeping references to setPage and filters)
and ensure this mirrors the serialization used where query keys are built.
In `@src/client/shared/ui/ui-pagination/ui-pagination-ellipsis.tsx`:
- Around line 8-17: The pagination ellipsis element is currently aria-hidden so
screen readers get no context; update the UiPaginationEllipsis span to remove
aria-hidden and instead provide an accessible label (e.g., add aria-label="More
pages" or aria-label="Ellipsis, more pages") and keep the decorative icon hidden
by adding aria-hidden to the UiSvgIcon prop; modify the span in
ui-pagination-ellipsis.tsx (the element rendering the <span ...
data-slot="pagination-ellipsis">) to include the aria-label and remove the
global aria-hidden while ensuring the UiSvgIcon remains aria-hidden.
In `@src/client/shared/ui/ui-pagination/ui-pagination-item.tsx`:
- Around line 10-15: The active page button only conveys visual state; update
the UiButton element in ui-pagination-item (where onClick, disabled, size,
variant are set) to include an aria-current attribute when isActive is true
(e.g., aria-current="page" or aria-current={isActive ? 'page' : undefined}) so
assistive technology receives current-page semantics; ensure you only set
aria-current for the active item and do not pass it when isActive is false.
---
Nitpick comments:
In
`@src/client/features/regular-incomes-expenses/regular-incomes-expenses-page-copy.tsx`:
- Line 7: This file contains a TODO comment indicating the component is
temporary; before merging, create a tracking issue (GitHub/GitLab/JIRA) to
remove the component and add the issue reference back into the source by
replacing the TODO comment in regular-incomes-expenses-page-copy.tsx with a
short note that includes the new issue ID/URL and expected cleanup conditions,
and also add the issue link to the PR description so reviewers can verify the
planned follow-up removal.
In `@src/client/shared/components/pagination/fin-pagination.tsx`:
- Around line 40-43: Guard the page bounds inside the click handlers for
pagination: instead of calling setPage(selectedPage - 1) or setPage(selectedPage
+ 1) directly in UiPaginationPrevious and UiPaginationNext handlers, compute a
clamped value (e.g., newPage = Math.max(1, Math.min(totalPages, selectedPage -
1)) and similarly for +1) and pass that to setPage; alternatively add a small
clampPage helper used by the onClick for UiPaginationPrevious (current handler)
and UiPaginationNext (lines ~58-61) so setPage never receives 0 or >totalPages
even if the button behavior changes.
In `@src/client/shared/hooks/pagination-resource/pagination-resource.hook.ts`:
- Around line 49-59: The error aggregation in the useMemo for errorMessage is
redundant: remove the find + isEmpty pattern and instead directly select the
first non-null/undefined error (e.g., via nullish coalescing between
getOptionsQuery.error and getTotalCountQuery.error) and, if present, pass it to
getErrorMessage; update the useMemo body for errorMessage to compute const error
= getOptionsQuery.error ?? getTotalCountQuery.error; return error ?
getErrorMessage(error) : undefined, keeping the same dependencies.
🪄 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: 38187d8c-9075-4e33-b188-603908f5558e
📒 Files selected for processing (13)
src/client/features/regular-incomes-expenses/regular-incomes-expenses-page-copy.tsxsrc/client/shared/components/pagination/fin-pagination.tsxsrc/client/shared/components/pagination/props/pagination.props.tssrc/client/shared/hooks/pagination-resource/models/pagination-resource.model.tssrc/client/shared/hooks/pagination-resource/pagination-resource.hook.tssrc/client/shared/ui/ui-icon-button/props/icon-button.props.tssrc/client/shared/ui/ui-icon-button/styles/icon-button-variant.scsssrc/client/shared/ui/ui-pagination/props/pagination-item.props.tssrc/client/shared/ui/ui-pagination/ui-pagination-actions.tsxsrc/client/shared/ui/ui-pagination/ui-pagination-content.tsxsrc/client/shared/ui/ui-pagination/ui-pagination-ellipsis.tsxsrc/client/shared/ui/ui-pagination/ui-pagination-item.tsxsrc/client/shared/ui/ui-pagination/ui-pagination.tsx
…ixed ui-pagination-ellipsis.tsx
Qunitee
approved these changes
Apr 10, 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
Add pagination component
Type
Description
Added pagination component and usePaginatedResource hook
Check list
Summary by CodeRabbit