-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dataset pagination #307
Dataset pagination #307
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 6d1b04e in 1 minute and 54 seconds
More details
- Looked at
98
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/components/datasets/datasets.tsx:33
- Draft comment:
useSearchParams
andusePathname
should not be called as functions. They are hooks that return objects, so you should use them directly without calling them. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/components/datasets/datasets.tsx:81
- Draft comment:
Avoid callinggetDatasets
twice inhandleDeleteDatasets
. Consider moving it to afinally
block to ensure it's called once after the try-catch block. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The current implementation only refreshes the dataset list if the deletion was successful (res.ok). This is actually the correct behavior - we don't want to refresh the list if the deletion failed. Moving getDatasets() to a finally block would mean we refresh even on failure, which is wrong. The comment's suggestion would introduce a bug.
Maybe there's a race condition I'm not seeing? Or maybe refreshing on failure would be helpful to ensure the UI is in sync?
No, refreshing on failure would be incorrect - if the deletion failed, the datasets still exist in the backend, so there's no need to refresh. The current implementation correctly only refreshes after confirmed deletion.
The comment should be deleted because its suggestion would make the code worse by refreshing the dataset list even when deletion fails.
Workflow ID: wflow_Ouadd2vje7jUXXLR
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.
setDatasets(undefined); | ||
const url = `/api/projects/${projectId}/datasets/?pageNumber=${pageNumber}&pageSize=${pageSize}`; | ||
|
||
const res = await fetch(url, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for the fetch call to prevent unhandled promise rejections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
components/traces/traces-table.tsx
for implementation.Changes Made
Additional Context
Screen.Recording.2025-01-06.at.10.06.11.PM.mov
Testing
@dinmukhamedm please let me know for any changes required.
Important
Implements manual pagination in
Datasets
component with custom data fetching and state management.Datasets
component, replacing SWR with custom fetch logic.getDatasets()
function to fetch paginated data based onpageNumber
andpageSize
.getDatasets()
.datasets
,totalCount
,pageNumber
, andpageSize
state variables.pageCount
based ontotalCount
andpageSize
.DataTable
to support manual pagination withonPageChange
handler.This description was created by for 6d1b04e. It will automatically update as commits are pushed.