feat: disable table features when infinite scroll is enabled#40020
feat: disable table features when infinite scroll is enabled#40020jacquesikot merged 37 commits intoreleasefrom
Conversation
…dd-disabled-property-to-property-pane-config
…enabled" This reverts commit 9e6417d.
…x cyclic dependency
…dd-disabled-property-to-property-pane-config
…nitions for disabled and hidden properties in property controls
…in PropertyPaneSectionConfig to use unknown type
…in PropertyPaneSectionConfig to use Record<string, unknown> type
…in PropertySection to use Record<string, unknown> type
…in PropertySection to use generic type T
…pertyPaneSectionConfig and PropertySection to use any type
…dd-disabled-property-to-property-pane-config
…WidgetV2 configuration
…isable-features-for-infinite-scroll
jacquesikot
changed the title
jacquesikot
requested review from
AmanAgarwal041,
rahulbarwal and
vivek-appsmith
and removed request for
a team
![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 (3)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts (2)
42-42: Consider using optional chaining.Replace the conditional check with optional chaining for cleaner code.
- if (paginationSection && paginationSection.children) { + if (paginationSection?.children) {🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
83-83: Consider using optional chaining.Similar to the previous comment, replace the conditional check with optional chaining.
- if (paginationSection && paginationSection.children) { + if (paginationSection?.children) {🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (1)
1517-1558: Comprehensive cell editability update with potential improvement.The function correctly manages cell editability across all columns when infinite scroll is toggled. However, there's a potential issue in how columns are accessed.
The function uses column.alias to construct property paths, but it's not clear if the alias is guaranteed to match the key in primaryColumns. Consider using the column.id instead, which seems to be the primary identifier:
- Object.entries(props.primaryColumns).forEach(([_, column]) => { + Object.entries(props.primaryColumns).forEach(([columnId, column]) => { - const columnName = column.alias; + // Use column.id which appears to be the primaryColumns key updates.push({ - propertyPath: `primaryColumns.${columnName}.isCellEditable`, + propertyPath: `primaryColumns.${columnId}.isCellEditable`, propertyValue: false, }); updates.push({ - propertyPath: `primaryColumns.${columnName}.isEditable`, + propertyPath: `primaryColumns.${columnId}.isEditable`, propertyValue: false, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts(6 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts (1)
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (3)
updateAllowAddNewRowOnInfiniteScrollChange(1452-1474)updateSearchSortFilterOnInfiniteScrollChange(1477-1515)updateCellEditabilityOnInfiniteScrollChange(1518-1558)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts (2)
app/client/src/widgets/TableWidgetV2/constants.ts (1)
TableWidgetProps(51-116)app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (3)
updateAllowAddNewRowOnInfiniteScrollChange(1452-1474)updateCellEditabilityOnInfiniteScrollChange(1518-1558)updateSearchSortFilterOnInfiniteScrollChange(1477-1515)
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (15)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts (4)
9-11: Good use of consistent help text constant.The disabledHelpText is defined as a constant, which ensures consistency throughout the test and matches what's used in the implementation.
12-34: Well-structured test with comprehensive verification.The test properly checks all required aspects of the section configuration when infinite scroll is enabled:
- Section existence
- Presence of shouldDisableSection function
- Correct disabledHelpText
- infiniteScrollEnabled in dependencies
- Function behavior with both enabled and disabled props
36-73: Good test for Pagination section with proper validation.This test properly verifies the server-side pagination functionality within the Pagination section, ensuring it follows the same infinite scroll rules as other sections.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
76-94: Appropriate test for update hooks configuration.This test ensures the correct configuration of update hooks for the infiniteScrollEnabled property, which is essential for the feature to work properly.
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (2)
1450-1474: Clear implementation to manage row addition with infinite scroll.This function correctly toggles the allowAddNewRow property based on the infiniteScrollEnabled state. The implementation handles the true/false cases explicitly and returns undefined for other values, which is a good defensive programming practice.
1476-1515: Complete implementation for toggling search, filter, and sort features.This function properly disables search, filters, and sorting when infinite scroll is enabled and enables them when disabled. The function follows the same pattern as the previous one, with explicit handling of true/false cases.
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts (3)
8-49: Comprehensive test for add row update feature.This test properly verifies all scenarios:
- Enabling infinite scroll disables add row
- Disabling infinite scroll enables add row
- Non-boolean values return undefined
Good coverage of the expected behavior.
51-106: Well-structured test for search, filter, and sort updates.The test covers all expected behaviors for the updateSearchSortFilterOnInfiniteScrollChange function, with clear assertions and good organization.
108-197: Complete test suite for cell editability updates.This test properly verifies:
- Disabling editability when infinite scroll is enabled
- Enabling editability when infinite scroll is disabled
- Handling the case when primaryColumns is undefined
- Handling non-boolean property values
The mock data structure is appropriate and the assertions are thorough.
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/contentConfig.ts (6)
34-35: Good use of constant for help text.Defining the help text as a constant ensures consistency across all disabled sections and makes future updates easier.
185-188: Clear section disabling configuration for server-side pagination.This properly implements the disabling of server-side pagination when infinite scroll is enabled, with appropriate help text and dependency.
198-204: Well-structured update hook composition.The update hook correctly composes all three utility functions to update related properties when infinite scroll is toggled. Adding primaryColumns as a dependency ensures the hook runs when columns change.
255-258: Consistent implementation for Search & filters section.The section disabling configuration follows the same pattern as other sections, ensuring consistent behavior across the UI.
408-411: Appropriate disabling for Sorting section.The implementation is consistent with other sections and properly disables sorting when infinite scroll is enabled.
465-468: Consistent implementation for Adding a row section.The section follows the same pattern as other sections, ensuring consistency in the UI behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts (2)
42-42: Consider using optional chainingThe static analysis tool suggests using optional chaining here to simplify the code.
- if (paginationSection && paginationSection.children) { + if (paginationSection?.children) {🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
92-92: Consider using optional chainingSimilar to the previous suggestion, use optional chaining to simplify the code.
- if (paginationSection && paginationSection.children) { + if (paginationSection?.children) {🧰 Tools
🪛 Biome (1.9.4)
[error] 92-92: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts (1)
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (3)
updateAllowAddNewRowOnInfiniteScrollChange(1452-1474)updateSearchSortFilterOnInfiniteScrollChange(1477-1515)updateCellEditabilityOnInfiniteScrollChange(1518-1558)
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 92-92: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
🔇 Additional comments (7)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts (1)
5-109: Tests look thorough and well-structuredThe tests comprehensively validate the behavior of table sections when infinite scroll is enabled/disabled. You're checking both the configuration (disabled sections, help text) and the actual functionality (shouldDisableSection behavior).
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 92-92: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (3)
1451-1474: Well-implemented toggle for add new row featureThis function properly disables the add new row feature when infinite scrolling is enabled and restores it when infinite scrolling is disabled. The code handles edge cases appropriately by returning undefined for invalid inputs.
1476-1515: Good implementation for search, sort, and filter controlsThe function correctly manages multiple properties that need to be toggled when infinite scrolling changes state. The implementation is consistent with the other toggle functions and handles edge cases properly.
1517-1558: Cell editability toggle implementation looks correctThis function iterates through all columns to update both cell-level and column-level editability properties. The null check for primaryColumns is important for preventing errors. The approach of collecting updates in an array first is clean and ensures we only return updates when needed.
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts (3)
8-49: Comprehensive tests for add new row toggleThe test suite thoroughly validates the behavior of
updateAllowAddNewRowOnInfiniteScrollChange, including the expected property updates when infinite scroll is enabled/disabled and the undefined return for invalid inputs.
51-106: Good coverage for search, sort, and filter featuresThese tests properly verify that all three properties (search, filter, sort) are toggled correctly when infinite scroll changes state. The test also validates handling of invalid input values.
108-198: Thorough testing of cell editability togglingThe test setup with mock columns allows proper verification of column property updates. All paths are tested, including the undefined return cases for when there are no primary columns or invalid property values are provided.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts (3)
44-44: Consider using optional chaining for cleaner conditionals.The current null checks could be simplified using optional chaining.
- if (paginationSection && paginationSection.children) { + if (paginationSection?.children) {Same pattern appears again at line 95.
Also applies to: 95-95
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
25-30: Extract duplicated test fixtures.The
enabledPropsanddisabledPropsobjects are duplicated in the test file. Consider extracting these to reusable variables.+ const createProps = (infiniteScrollEnabled: boolean) => ({ + infiniteScrollEnabled, + } as TableWidgetProps); + // Then in the test: - const enabledProps = { - infiniteScrollEnabled: true, - } as TableWidgetProps; - const disabledProps = { - infiniteScrollEnabled: false, - } as TableWidgetProps; + const enabledProps = createProps(true); + const disabledProps = createProps(false);Also applies to: 70-75
12-34: Consider flattening nested conditionals.The nested if-block after the expect statement could be flattened for improved readability.
expect(section).toBeDefined(); + if (!section) return; // Early return if undefined - if (section) { expect(typeof section.shouldDisableSection).toBe("function"); expect(section.disabledHelpText).toBe(disabledHelpText); expect(section.dependencies).toContain("infiniteScrollEnabled"); const enabledProps = { infiniteScrollEnabled: true, } as TableWidgetProps; const disabledProps = { infiniteScrollEnabled: false, } as TableWidgetProps; expect(section.shouldDisableSection!(enabledProps, "")).toBe(true); expect(section.shouldDisableSection!(disabledProps, "")).toBe(false); - }Same pattern appears in other conditionals throughout the file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- 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-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (3)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/__tests__/contentConfig.test.ts (3)
5-85: Strong test coverage for infinite scroll section disabling.This test case effectively validates that appropriate sections are disabled when infinite scroll is enabled. It checks the essential configuration properties and behavior.
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
87-113: Appropriate update hook verification.The test verifies that the infinite scroll property is correctly configured with update hooks and dependencies, which is crucial for the automatic management feature described in the PR objectives.
🧰 Tools
🪛 Biome (1.9.4)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1-4: Imports look appropriate for the test context.The imports correctly reference the configuration being tested and necessary type definitions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts (2)
1103-1121: Consider maintaining consistent property order in test assertionsThe order of properties in the enable test (lines 1131-1143) is different from the order in the disable test (lines 1109-1121). For better readability and maintainability, consider keeping the same order in both places.
// When infinite scroll is disabled expect( updateSearchSortFilterOnInfiniteScrollChange( props, "infiniteScrollEnabled", false, ), ).toEqual([ { - propertyPath: "isVisibleFilters", - propertyValue: true, - }, - { propertyPath: "isVisibleSearch", propertyValue: true, }, { + propertyPath: "isVisibleFilters", + propertyValue: true, + }, + { propertyPath: "isSortable", propertyValue: true, }, ]);Also applies to: 1131-1143
1057-1096: Consider adding initial state verificationThe tests could be improved by verifying the initial state of properties before applying changes. This would ensure the tests start from a known state and make them more robust against future changes.
it("updateAllowAddNewRowOnInfiniteScrollChange - should disable/enable add new row when infinite scroll is toggled", () => { - const props = {} as TableWidgetProps; + const props = { allowAddNewRow: true } as TableWidgetProps; + + // Verify initial state + expect(props.allowAddNewRow).toBe(true); // When infinite scroll is enabled expect( updateAllowAddNewRowOnInfiniteScrollChange(Also applies to: 1098-1153, 1155-1245
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts (1)
1056-1246: LGTM! The tests for infinite scroll hooks are comprehensive.The newly added test suite thoroughly verifies that:
- Adding rows is automatically disabled with infinite scroll
- Search, filter, and sort features are disabled with infinite scroll
- Cell editability is disabled with infinite scroll
- Features are properly restored when infinite scroll is disabled
- Edge cases are handled correctly
All tests match the requirements from the PR objectives.
jacquesikot
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14211612203. |
rahulbarwal
left a comment
rahulbarwal
left a comment
There was a problem hiding this comment.
Screen.Recording.2025-04-02.at.10.10.44.AM.mov
|
Deploy-Preview-URL: https://ce-40020.dp.appsmith.com |
…onent to account for infinite scroll
2 participants
📌 Problem
The TableWidgetV2's infinite scroll feature has not been implemented for several table capabilities. The affected features include:
✅ Solution
This PR implements automatic feature compatibility management when infinite scroll is enabled:
🛠️ Implementation
propertyUtils.tsto manage feature state transitions🎯 Why This Approach?
🚀 How to Test
Fixes #39850
Automation
/ok-to-test tags="@tag.Table, @tag.Widget, @tag.Sanity, @tag.PropertyPane"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14216055087
Commit: 89df4d6
Cypress dashboard.
Tags:
@tag.Table, @tag.Widget, @tag.Sanity, @tag.PropertyPaneSpec:
Wed, 02 Apr 2025 10:49:47 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Improvements