chore: Set max height for Table Data property control #39409
Conversation
WalkthroughA new property Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
|
github-actions
bot
added
IDE Pod
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts (1)
38-38: Consider making the maxHeight value configurable.Instead of hardcoding "400px", consider making this value configurable through a property control to allow customization based on different use cases.
- maxHeight: "400px", + propertyName: "tableDataMaxHeight", + label: "Max height", + controlType: "INPUT_TEXT", + defaultValue: "400px", + isBindProperty: true, + isTriggerProperty: false, + validation: { + type: ValidationTypes.TEXT, + params: { + regex: /^\d+px$/, + }, + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts (1)
36-39: LGTM! The maxHeight addition aligns with the PR objective.The implementation will help prevent system overload when users input large amounts of static JSON data.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13492436485. |
|
Hi @hetunandu will lots of static data within that field, can you check the dom nodes are limited by inspecting that field. I want to check if virtualisation is working for that field or not. |
|
Deploy-Preview-URL: https://ce-39409.dp.appsmith.com |
@vsvamsi1 Virtualization Right now is enabled only when table is server side paginated and cell wrapping is turned on/off. (https://github.com/appsmithorg/appsmith/blob/release/app/client/src/widgets/TableWidgetV2/component/Table.tsx#L338) For static data, We paginate the table using the page size which is calculated based on table ID. So technically the problem of too many nodes never arises for static data. |
|
@hetunandu |
|
@vsvamsi1 @rahulbarwal Please check the video. This can be tested on the DP as well. It is seen that even when 500+ records are pasted inside the TableData field, only a few rows are actually available in the dom, and they update when codemirror is scrolled hence proving the presence of virtualisation Screen.Recording.2025-02-24.at.12.45.04.PM.mov |
|
Oh thanks, got it. This makes sense. I assumed you were talking about the actual table widget, my bad. |
|
Although this begs the question, shouldn't this control type(& similar) have a default max height? |
|
@hetunandu thanks for verifying it , what i meant by the excess dom nodes issue is at the "Table Data" field not at the actual table widget. From the screen recording it looks it has been resolved, thanks for resolving this issue. |
Description
Updates the Table Widget Property config to use the maxHeight property. This ensures that when users paste in huge amounts of static json, the content DOM elements dont overload the system
Fixes #39405
Automation
/ok-to-test tags="@tag.Table"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13492213082
Commit: b67e2f9
Cypress dashboard.
Tags:
@tag.TableSpec:
Mon, 24 Feb 2025 07:14:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit