feat: remove release_table_cell_label_value_enabled feature flag#37110
Conversation
github-actions
bot
added
Table Widget V2
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
rahulbarwal
left a comment
rahulbarwal
left a comment
There was a problem hiding this comment.
Changes are not complete! all of these files(main file: FeatureFlag.ts and all specs as well)
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
@rahulbarwal These changes have been made |
![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
🧹 Outside diff range and nitpick comments (8)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (3)
Line range hint
6-45: Consider moving test data to a fixture file.The table test data should be moved to the fixtures directory for better maintainability and reusability across test files.
Move the
demoTableDatatocypress/fixtures/tableTestData.json:{ "demoTableData": [ { "role": 1, "id": 1, "name": "Alice Johnson", // ... rest of the data } ] }
Line range hint
89-91: Replace cy.wait with proper assertions.Using cy.wait() is discouraged. Instead, wait for specific conditions or state changes.
Replace with:
cy.get(_.locators._dropdownText) .should('be.visible') .children() .contains("Select") .click(); cy.get('@updateLayout').should('exist');
Line range hint
77-78: Avoid using force: true in click events.Using force: true bypasses Cypress's built-in retry-ability and actionability checks.
Replace with proper element targeting:
cy.get(_.propPane._paneTitle) .should('be.visible') .should('be.enabled') .click();app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (2)
Line range hint
157-185: Consider optimizing select column type test setupThe test is well-structured but could be improved:
- Move the select options data to a fixture file
- Consider using
beforeEachfor table data setup- Add negative test cases for select column filtering
+ // Move to fixtures/selectOptions.json + const selectOptions = [ + {"label": "Software Engineer", "value": 10}, + {"label": "Product Manager", "value": 20}, + {"label": "UX Designer", "value": 30} + ];
Replace non-deterministic waits with proper Cypress assertions
Based on the findings in
Table.ts, multiple instances ofagHelper.Sleep()are used which can make tests flaky and non-deterministic. As per the learnings, these should be replaced with Cypress's built-in waiting mechanisms:
- Replace
agHelper.Sleep()calls in table operations with proper Cypress assertions:
WaitUntilTableLoad: Usecy.get(selector).should('exist')orcy.get(selector).should('be.visible')- Table data operations: Use
cy.intercept()to wait for API responses instead of fixed timeouts- Pagination operations: Use
cy.get(selector).should('have.text', expectedText)instead of Sleep(1000)Example:
// Instead of: this.agHelper.Sleep(500); // Use: cy.get(this._tableRowColumnData(rowNum, colNum, tableVersion)) .should('exist') .should('be.visible');🔗 Analysis chain
Line range hint
21-156: Improve test reliability by following Cypress best practicesThe test implementation has several areas that could be improved:
- Replace implicit waits with proper assertions:
- table.WaitUntilTableLoad(0, 0, "v2"); + cy.get(table.selectors.tableRow).should('exist');
- Use data-* attributes for selectors:
- cy.get(".t--property-control-allowfiltering input"); + cy.get("[data-cy=allow-filtering-input]");
- Consider using API calls for table data setup instead of UI interactions.
The previous search didn't find the table helper methods because they are likely in a different location. Let's check the Table.ts file which is more likely to contain these methods.
Let's verify the actual implementation of these methods to provide accurate recommendations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of cy.wait and sleep in table helper methods rg -l "cy\.wait|\.sleep" "app/client/cypress/support/Objects/ObjectsCore.ts"Length of output: 76
Script:
#!/bin/bash # Check for usage of cy.wait and sleep in table helper methods rg -l "cy\.wait|\.sleep" "app/client/cypress/support/Pages/Table.ts" # Check for actual implementation of table helper methods rg "WaitUntilTableLoad|RemoveFilterNVerify|SearchTable" "app/client/cypress/support/Pages/Table.ts" -A 5Length of output: 898
Script:
#!/bin/bash # Let's check the actual implementation of WaitUntilTableLoad to see if it uses cy.wait or sleep rg "WaitUntilTableLoad.*\{" -A 10 "app/client/cypress/support/Pages/Table.ts" # Also check for any agHelper.Sleep usage rg "agHelper\.Sleep" "app/client/cypress/support/Pages/Table.ts" -A 2Length of output: 1044
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (3)
Line range hint
73-96: Replace cy.wait with better assertionsUsing
cy.wait("@updateLayout")is discouraged. Instead, use explicit assertions to wait for the expected state.-cy.wait("@updateLayout"); +cy.get(".t--property-control-editable").should("exist");
Line range hint
250-270: Avoid using force: true in click actionsUsing
force: truein click actions can mask underlying issues with element visibility or positioning.-cy.get(".t--add-new-row").click({ force: true }); +cy.get(".t--add-new-row") + .should("be.visible") + .click();
Line range hint
432-500: Consider adding error scenario testsThe server-side filtering tests could benefit from additional coverage for error scenarios, such as:
- Network failures during filtering
- Invalid response formats
- Rate limiting scenarios
Example test case:
it("should handle server errors during filtering", () => { cy.intercept("POST", "/api/v1/actions/execute", { statusCode: 500, body: { error: "Internal Server Error" } }).as("queryError"); cy.editTableSelectCell(0, 0); cy.get(".select-popover-wrapper .bp3-input-group input") .type("test", { force: true }); cy.wait("@queryError"); cy.get(".error-message").should("be.visible"); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (1 hunks)
- app/client/src/ce/entities/FeatureFlag.ts (0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/ce/entities/FeatureFlag.ts
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (5)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (2)
Line range hint
46-52: Well-structured test suite with proper isolation.The test suite follows good practices with:
- Clear test descriptions
- Proper test isolation using beforeEach
- Comprehensive assertions for sorting validation
1-2: Confirm feature flag removal is complete.The removal of featureFlagIntercept import aligns with the PR objective.
✅ Verification successful
Feature flag removal is complete
The search results confirm that while there are other feature flags in use across the codebase, there are no remaining references to the specific
release_table_cell_label_value_enabledfeature flag.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no remaining references to the feature flag rg -l "release_table_cell_label_value_enabled" app/client/cypress/Length of output: 66
Script:
#!/bin/bash # Let's search more broadly for any feature flag references in the test files rg -i "featureflag|feature.flag" app/client/cypress/ # Also check for any intercept patterns that might be related rg "cy\.intercept.*flag" app/client/cypress/Length of output: 192130
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)
3-8: LGTM: Import changes are well-organizedThe imports are properly structured and include necessary test helper objects.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (2)
Line range hint
1-500: Changes align with feature flag removalThe test file has been successfully updated to remove feature flag dependencies while maintaining comprehensive test coverage for the Select column functionality.
Line range hint
1-11: Consider improving test isolationThe test suite relies on a shared table widget state across multiple test cases. This could lead to flaky tests if the state is not properly reset between tests.
jacquesikot
deleted the
feat/remove-release-table-cell-label-value-enabled-feature-flag
branch
…smithorg#37110) ## Description Removes the `release_table_cell_label_value_enabled` feature flag from the codebase. This feature flag helped us launch the tables ability to show labels instead of values for select column types. It has been in production for over 2 months with no bug or unexpected behaviours reported, warranting a removal of the feature flag from the codebase. Fixes appsmithorg#37109 ## Automation /ok-to-test tags="@tag.Table, @tag.Sanity, @tag.Select" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11550654098> > Commit: d225470 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11550654098&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Table, @tag.Sanity, @tag.Select` > Spec: > <hr>Mon, 28 Oct 2024 09:57:21 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced the `SelectCell` component for improved label value determination. - **Bug Fixes** - Removed unnecessary complexity related to feature flag checks, resulting in a more reliable display of cell labels. - **Tests** - Updated test cases for table filtering and sorting functionalities, removing feature flag dependencies while maintaining core logic. - Enhanced test coverage for select column functionality, including dynamic bindings and server-side filtering checks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Removes the
release_table_cell_label_value_enabledfeature flag from the codebase.This feature flag helped us launch the tables ability to show labels instead of values for select column types. It has been in production for over 2 months with no bug or unexpected behaviours reported, warranting a removal of the feature flag from the codebase.
Fixes #37109
Automation
/ok-to-test tags="@tag.Table, @tag.Sanity, @tag.Select"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11550654098
Commit: d225470
Cypress dashboard.
Tags:
@tag.Table, @tag.Sanity, @tag.SelectSpec:
Mon, 28 Oct 2024 09:57:21 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
SelectCellcomponent for improved label value determination.Bug Fixes
Tests