OINT-1210: Datatable pagination #486
Merged
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Bug Report
|
|
|
1 similar comment
|
|
|
✨ No issues found! Your code is sparkling clean! ✨ 🗒️ View all ignored comments in this repo
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements comprehensive pagination functionality across the application's data tables, with server-side pagination support and consistent UI patterns.
- Added
PaginatedDatatype inpackages/ui-v1/components/DataTable.tsxto standardize pagination structure across tables - Implemented server-side pagination in
DataTablecomponent with manual pagination control and loading states - Fixed potential memory leak in
columnsWithActionsuseMemo dependency array inapps/web/app/console/(authenticated)/connections/page.client.tsx - Added consistent page size of 10 items across all table implementations
- Added loading state handling during page transitions with visual feedback
💡 (4/5) You can add custom instructions or style guidelines for the bot here!
8 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
apps/web/app/console/(authenticated)/connector-config/page.client.tsx
Outdated
Show resolved
Hide resolved
| const PaginatedTable = (props: DataTableProps<Person, string | number>) => { | ||
| const [pageIndex, setPageIndex] = useState(0) | ||
| const [isLoading, setIsLoading] = useState(false) | ||
| const [currentData, setCurrentData] = useState(generateRandomData(10)) |
There was a problem hiding this comment.
style: initial data generation should use props.data.limit instead of hardcoded 10
Comment on lines
104
to
105
| type Story = StoryObj< | ||
| | typeof DataTable<Core['connector_config_select'], unknown> | ||
| | typeof ConnectorConfigTable | ||
| typeof DataTable<Core['connector_config_select'], unknown> | ||
| > |
There was a problem hiding this comment.
logic: Story type no longer includes ConnectorConfigTable but meta still uses it as component
Comment on lines
+7
to
+12
| type PaginatedData<T> = { | ||
| items: T[] | ||
| total: number | ||
| limit: number | ||
| offset: number | ||
| } |
There was a problem hiding this comment.
style: PaginatedData type is duplicated from DataTable.tsx. Consider importing it instead of redefining.
| const pageSize = isPaginated ? data.limit : items.length | ||
| const currentPage = isPaginated ? Math.floor(data.offset / data.limit) : 0 | ||
|
|
||
| const [pageIndex, setPageIndex] = useState(currentPage) |
There was a problem hiding this comment.
logic: pageIndex state may get out of sync with currentPage when data.offset changes externally
Suggested change
| const [pageIndex, setPageIndex] = useState(currentPage) | |
| const [pageIndex, setPageIndex] = useState(currentPage) | |
| React.useEffect(() => { | |
| setPageIndex(currentPage) | |
| }, [currentPage]) |
|
|
||
| export const Default: Story = { | ||
| args: { | ||
| data: customers, | ||
| onPageChange: () => {}, |
There was a problem hiding this comment.
style: Empty onPageChange handler won't demonstrate pagination behavior. Consider adding a mock implementation that logs the page change.
Suggested change
| onPageChange: () => {}, | |
| onPageChange: (page: number) => { console.log('Page changed to:', page) }, |
Comment on lines
+182
to
+207
| ? (updater) => { | ||
| if (typeof updater === 'function') { | ||
| const newState = updater({pageIndex, pageSize}) | ||
| setPageIndex(newState.pageIndex) | ||
| onPageChange?.(newState.pageIndex) | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: onPaginationChange should handle non-function updaters (direct state updates) as well
| type Story = StoryObj< | ||
| typeof DataTable<AppRouterOutput['listCustomers']['items'][number], unknown> | ||
| > | ||
| type Story = StoryObj<typeof CustomersTable> |
There was a problem hiding this comment.
logic: Story type changed from DataTable to CustomersTable, but meta still uses DataTable as component. This mismatch could cause type issues.
Suggested change
| type Story = StoryObj<typeof CustomersTable> | |
| const meta: Meta<typeof CustomersTable> = { | |
| title: 'Domain Components/CustomersTable', | |
| component: CustomersTable, | |
| parameters: { | |
| layout: 'padded', | |
| }, | |
| } |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to f6198c2 in 2 minutes and 3 seconds
More details
- Looked at
808lines of code in8files - Skipped
0files when reviewing. - Skipped posting
19drafted comments based on config settings.
1. apps/web/app/console/(authenticated)/connections/page.client.tsx:63
- Draft comment:
Pagination is implemented correctly using pageIndex; ensure that the offset calculation (pageIndex * 10) aligns with API expected values. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/app/console/(authenticated)/connector-config/page.client.tsx:35
- Draft comment:
Pagination is added by setting limit and offset; ensure that multiple queries (connector configs, connectors) remain in sync if pagination affects both. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/web/app/console/(authenticated)/customers/page.client.tsx:9
- Draft comment:
Ensure the use of pageIndex state properly drives the customer list pagination. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages/ui-v1/components/DataTable.stories.tsx:207
- Draft comment:
The PaginatedTable story simulates a delay; ensure the delay logic (setTimeout) mimics real API pagination without side effects. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. packages/ui-v1/components/DataTable.tsx:180
- Draft comment:
The onPaginationChange callback only handles function updaters. Consider handling cases where updater might be a direct object for broader compatibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. packages/ui-v1/domain-components/tables/ConnectorConfigTable.stories.tsx:137
- Draft comment:
The Default story passes data as connectorConfigs.items while WithPagination uses connectorConfigs. Ensure clarity across stories for paginated and non-paginated props. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. packages/ui-v1/domain-components/tables/CustomerTable.tsx:71
- Draft comment:
The CustomersTable component wraps DataTable correctly. Verify that onPageChange and isLoading props align with paginated data structure expected by DataTable. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. packages/ui-v1/domain-components/tables/CustomersTable.stories.tsx:59
- Draft comment:
The story for CustomersTable provides a paginated data object. Ensure that the shape of data matches what CustomersTable component expects. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. apps/web/app/console/(authenticated)/connections/page.client.tsx:73
- Draft comment:
Consider memoizing the 'handlePageChange' function to avoid unnecessary re-renders. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
10. apps/web/app/console/(authenticated)/connector-config/page.client.tsx:53
- Draft comment:
Pagination logic is integrated well. Consider resetting pageIndex when filters or data conditions change. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
11. apps/web/app/console/(authenticated)/customers/page.client.tsx:17
- Draft comment:
Pagination options for listCustomers are correctly applied via limit and offset. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
12. packages/ui-v1/components/DataTable.stories.tsx:226
- Draft comment:
Consider parameterizing the simulated async delay in PaginatedTable for easier testing and clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
13. packages/ui-v1/components/DataTable.tsx:180
- Draft comment:
Ensure that the onPaginationChange updater is always a function; consider adding fallback handling if a non‐function value is provided. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
14. packages/ui-v1/components/DataTable.tsx:195
- Draft comment:
Revisit memoization of the context value to ensure stable re-renders without performance trade-offs (see commented-out optimization). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
15. packages/ui-v1/domain-components/tables/ConnectorConfigTable.stories.tsx:137
- Draft comment:
Ensure consistent data shapes for connectorConfigs: default stories pass an array while the pagination variant passes a full paginated object. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
16. packages/ui-v1/domain-components/tables/CustomersTable.stories.tsx:59
- Draft comment:
Consider adding additional story variants to simulate different pagination states (e.g. loading, empty page) for CustomersTable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
17. packages/ui-v1/components/DataTable.stories.tsx:71
- Draft comment:
Typo: Remove the trailing comma after the closing tag of the Button in the NoData story's JSX. It may be confusing or lead to formatting issues. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. packages/ui-v1/components/DataTable.stories.tsx:97
- Draft comment:
Typo: Please remove the trailing comma after the closing Button tag in the WithCallToAction story's header. This extra comma is unnecessary and may affect consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. packages/ui-v1/components/DataTable.tsx:248
- Draft comment:
Typo: The class 'size-full' seems out of place. In Tailwind CSS, 'w-full' is typically used for full width. Consider correcting this if it was unintentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_GwGYdqR3e0Vh9RVR
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| const pageSize = isPaginated ? data.limit : items.length | ||
| const currentPage = isPaginated ? Math.floor(data.offset / data.limit) : 0 | ||
|
|
||
| const [pageIndex, setPageIndex] = useState(currentPage) |
There was a problem hiding this comment.
The derived currentPage is computed from data.offset but pageIndex state is initialized only once. Consider adding an effect to resync pageIndex when data.offset changes.
pellicceama
requested changes
Apr 19, 2025
Rodri77
force-pushed
the
datatable-pagination
branch
from
f6198c2 to
2752bcb
Compare
|
Error: Agent timed out after 12 minutes of thinking |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2752bcb in 51 seconds. Click for details.
- Reviewed
93lines of code in2files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/app/console/(authenticated)/events/page.client.tsx:25
- Draft comment:
Good move shifting timestamp formatting into the cell renderer. Consider memoizing the date formatting for performance if the table is large. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. apps/web/app/console/(authenticated)/events/page.client.tsx:50
- Draft comment:
Ensure the TRPC response returns a paginated object with the expected shape (items, total, limit, offset) to match the DataTable's paginated data setup. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. packages/ui-v1/components/DataTable.tsx:375
- Draft comment:
Pagination now also checks for totalCount === 0 to not render pagination controls. Confirm that this behavior is intended, especially when zero items are a valid state. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
4. apps/web/app/console/(authenticated)/events/page.client.tsx:25
- Draft comment:
Good use of a custom cell for timestamp formatting. Consider extracting the repeated date formatting logic into a shared utility for consistency (e.g. used in sheet details too). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. apps/web/app/console/(authenticated)/events/page.client.tsx:50
- Draft comment:
Pagination params (limit and offset) are now passed to the TRPC query. Verify that the backend correctly handles these values. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. apps/web/app/console/(authenticated)/events/page.client.tsx:63
- Draft comment:
handlePageChange updates pageIndex; ensure any dependent state (if needed) is also reset when changing pages. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. packages/ui-v1/components/DataTable.tsx:375
- Draft comment:
The added check for paginationInfo.totalCount === 0 cleanly avoids rendering pagination when no items exist. Ensure this behavior aligns with the overall UX for empty states. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_WSKTfiNBMAfG4Rlf
You can customize 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Important
Adds pagination support to
DataTableand updates various components to utilize paginated data handling.ConnectionsPage,ConnectorConfigList,CustomerList, andEventsListby introducingDATA_PER_PAGEconstant andpageIndexstate.DataTablecomponent to handle paginated data withPaginatedDatatype andonPageChangecallback.processTypedPaginatedResponse()inpagination.tsto calculate total based on result length.DataTableinDataTable.tsxto support pagination withmanualPaginationandpageCount.Paginationcomponent toDataTablefor navigation controls.CustomersTable,ConnectorConfigTable, andDataTablestories to demonstrate pagination.events/page.client.tsx.handlePageChangefunction to manage page index state in multiple components.This description was created by
for 2752bcb. You can customize this summary. It will automatically update as commits are pushed.