-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix: Change the value of pageOffset when enabling server side pagination #36056
fix: Change the value of pageOffset when enabling server side pagination #36056
Conversation
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes made to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (1 hunks)
Additional comments not posted (1)
app/client/src/widgets/TableWidgetV2/widget/derived.js (1)
794-794
: SimplifiedpageSize
calculation logic. Verify impact on pagination behavior.The change simplifies the logic by directly assigning
props.pageSize
topageSize
, removing the conditional check based onprops.tableData
.Please ensure that this change does not introduce any unintended side effects or break existing functionality, especially in scenarios involving server-side pagination.
To verify the impact, you can run the following script:
Thoroughly test the pagination functionality to confirm that the behavior remains consistent and as expected after this change.
Verification successful
Run existing tests to verify the impact of the
pageSize
logic change.The
getPageOffset
function is well-covered by tests in bothWDSTableWidget
andTableWidgetV2
. These tests include various scenarios, such as null, undefined, and numeric values forpageNo
andpageSize
. Running these tests will help ensure that the simplifiedpageSize
calculation logic does not introduce any unintended side effects or break existing functionality, particularly in server-side pagination scenarios.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "Searching for usages of the getPageOffset function..." # Search for the function usage rg --type js -A 5 $'getPageOffset\\(' echo "Manually review the search results to ensure that the simplified pageSize calculation logic does not break any existing functionality or introduce unintended side effects, particularly in server-side pagination scenarios."Length of output: 8244
/ok-to-test |
This comment was marked as duplicate.
This comment was marked as duplicate.
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
@phennapa can you please remove changes in cypress/scripts/.yarn. There should only be 1 file change in this PR |
Thanks. I have deleted the files as per your suggestion. |
…ion (appsmithorg#36056) ## Description Fixed issue appsmithorg#36022 where incorrect data is displayed when navigating to the previous page after reaching the last page with fewer items in the table during server-side pagination enabled by using `props.pageSize` instead of `props.tableData?.length`. Fix: Replaced `props.tableData?.length` with `props.pageSize` to ensure consistent data display when navigating between pages, especially when dealing with the last page that has fewer items than the pageSize. Fixes appsmithorg#36022 ## Automation /ok-to-test tags="@tag.Table" ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Improved pagination logic for the table widget, simplifying the handling of page size. - **Bug Fixes** - Resolved potential inconsistencies in pagination when server-side data is utilized. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Phennapa Saeliw <psaeliw@gmail.com>
Description
Fixed issue #36022 where incorrect data is displayed when navigating to the previous page after reaching the last page with fewer items in the table during server-side pagination enabled by using
props.pageSize
instead ofprops.tableData?.length
.Fix:
Replaced
props.tableData?.length
withprops.pageSize
to ensure consistent data display when navigating between pages, especially when dealing with the last page that has fewer items than the pageSize.Fixes #36022
Automation
/ok-to-test tags="@tag.Table"
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes