fix: prevent partial rows in table pagination#42008
Conversation
WalkthroughTableV2 now uses updated header heights and always rounds calculated page sizes down. Jest and Cypress tests cover the resulting page-size values for default, short, and tall row-height configurations. ChangesTableV2 page-size behavior
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Pagesize_spec.ts`:
- Line 27: Update the page-size assertions in the relevant Cypress test to
convert each extracted text value to a number before comparison, covering all
three expectations near the existing $text assertions. Keep the expected
page-size values numeric rather than string literals.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8ddf497-0fd8-4e24-be97-2e0ed4f95d14
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Pagesize_spec.tsapp/client/src/widgets/TableWidgetV2/widget/derived.js
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29593233548. |
|
Deploy-Preview-URL: https://ce-42008.dp.appsmith.com |
|
@jiuyige you can test your changes here https://ce-42008.dp.appsmith.com/ |
Thanks! I checked the deploy preview and noticed there’s still a 2px vertical overflow. The pageSize calculation uses a 38px header height, but the actual height is 40px. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/29605676490. |
|
Deploy-Preview-URL: https://ce-42008.dp.appsmith.com |
|
@jiuyige you can test it again |
Thanks for rebuilding the preview! I tested it again, and the scrollbar issue is now fixed for all three row-height options: Default: 4 rows I reloaded the page after each change to make sure the setting persisted. All rows were fully visible, with no extra vertical scrollbar. I switched the table back to Default when I was done. I also reran the related Jest tests, Prettier, ESLint, and type check locally, and they all passed. The CodeRabbit comment has already been addressed. I haven’t run the full Cypress E2E suite because it requires test credentials, so I left that item unchecked. Looks good from my side. Could you take another look when you get a chance? |
Description
The Table widget could round a fractional
pageSizeup, which allowed a partially visible row and caused a vertical scrollbar even when the table was intended to fit one page.This change always floors the calculated
pageSizeso that only complete fixed-height rows are counted. It also updates the existing Cypress regression coverage for the default, short, and tall row-height modes.Dynamic page-size calculation for wrapped cells is intentionally out of scope, as discussed in the issue.
Fixes #42004
Testing
yarn check-typesAutomation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
getPageSizeacross default, short, and tall scenarios.