fix: reset File Ingest Log pagination when rows per page changes BED-8109#2754
Merged
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesPagination Reset Implementation and Testing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx (2)
202-204: 💤 Low value
req.urlis already aURLinstance in MSW v1 —new URL(req.url.toString())is a redundant round-trip.MSW v1 exposes
req.urlas aURLinstance whosesearchParamsproperty is aURLSearchParamsinstance, soreq.urlcan be used directly without reconstructing it.♻️ Optional simplification
- const url = new URL(req.url.toString()); + const url = req.url; fileUploadRequests.push(url);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx` around lines 202 - 204, The test handler is recreating a URL unnecessarily; inside the rest.get callback replace the round-trip new URL(req.url.toString()) with the existing req.url (which is already a URL instance in MSW v1) and push that directly into fileUploadRequests; update the rest.get handler reference to use req.url and any code that accesses searchParams to use req.url.searchParams so you don't reconstruct the URL object.
202-204: 💤 Low value
req.urlis already aURLinstance in MSW v1 — remove the redundantnew URL()constructor call.♻️ Simplification
- const url = new URL(req.url.toString()); - fileUploadRequests.push(url); + fileUploadRequests.push(req.url);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx` around lines 202 - 204, The test handler in the rest.get callback constructs a redundant URL with new URL(req.url.toString()); instead use the existing URL instance on req.url directly—replace the new URL(...) usage with req.url when pushing to fileUploadRequests in the rest.get handler to avoid unnecessary conversion and rely on MSW v1's native URL object.packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.tsx (1)
104-107: 💤 Low valueLGTM — correct and consistent with
handleOnConfirm.Both state updates are batched in React 18+ event handlers, so a single re-render (and single query refetch) will occur. One optional micro-improvement: pass an explicit radix to
parseInt.♻️ Optional: explicit radix
- setRowsPerPage(parseInt(event.target.value)); + setRowsPerPage(parseInt(event.target.value, 10));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.tsx` around lines 104 - 107, Update the onRowsPerPageChange handler to use an explicit radix when parsing the rows-per-page value (e.g., replace parseInt(event.target.value) with parseInt(event.target.value, 10) or Number(event.target.value)) so the setRowsPerPage call in the onRowsPerPageChange callback is unambiguous; this mirrors the parsing approach used in handleOnConfirm and keeps behavior consistent across locales and environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx`:
- Around line 202-204: The test handler is recreating a URL unnecessarily;
inside the rest.get callback replace the round-trip new URL(req.url.toString())
with the existing req.url (which is already a URL instance in MSW v1) and push
that directly into fileUploadRequests; update the rest.get handler reference to
use req.url and any code that accesses searchParams to use req.url.searchParams
so you don't reconstruct the URL object.
- Around line 202-204: The test handler in the rest.get callback constructs a
redundant URL with new URL(req.url.toString()); instead use the existing URL
instance on req.url directly—replace the new URL(...) usage with req.url when
pushing to fileUploadRequests in the rest.get handler to avoid unnecessary
conversion and rely on MSW v1's native URL object.
In
`@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.tsx`:
- Around line 104-107: Update the onRowsPerPageChange handler to use an explicit
radix when parsing the rows-per-page value (e.g., replace
parseInt(event.target.value) with parseInt(event.target.value, 10) or
Number(event.target.value)) so the setRowsPerPage call in the
onRowsPerPageChange callback is unambiguous; this mirrors the parsing approach
used in handleOnConfirm and keeps behavior consistent across locales and
environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 414ab48f-0bff-44cd-8d26-9a5f7887e237
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.tsx
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
FileIngestTableinbh-shared-uitrackspageindependently fromrowsPerPage. When the user paginates to a non-zero page and then changes Rows per page, the API call keeps the staleskip = rowsPerPage * pagevalue. If the new (skip,limit) window falls past the end of the result set, the table renders empty even though matching jobs exist.Motivation and Context
Resolves BED-8109
How Has This Been Tested?
pagination resetdescribe block inFileIngestTable.test.tsxTypes of changes
Summary by CodeRabbit
Bug Fixes
Tests