fix: resolve empty table dropdown issue with dynamic select options in add new row functionality #37108
Conversation
jacquesikot
requested review from
ApekshaBhosale,
sagar-qa007 and
a team
as code owners
WalkthroughThe changes in this pull request enhance the testing and functionality of 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
|
github-actions
bot
added
Bug
jacquesikot
added
ok-to-test
github-actions
bot
added
Bug
github-actions
bot
added
Production
Table Widget
Widgets & Accelerators Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (2 hunks)
- app/client/src/components/propertyControls/TableComputeValue.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_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.
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (1)
3-4: LGTM: Import statements are properly structured.The addition of
entityExplorerandlocatorsimports aligns with the test requirements.app/client/src/components/propertyControls/TableComputeValue.tsx (1)
178-195:getComputedValuemethod implementation looks good.The logic correctly handles various cases and ensures proper binding formats.
| it("1.8. should show the selectOptions data of a new row select cell when no data in the table", () => { | ||
| entityExplorer.DragDropWidgetNVerify("tablewidgetv2", 350, 500); | ||
| EditorNavigation.SelectEntityByName("Table2", EntityType.Widget); | ||
|
|
||
| // add base data | ||
| propPane.ToggleJSMode("Table data", "On"); | ||
| propPane.UpdatePropertyFieldValue( | ||
| "Table data", | ||
| `{{[ | ||
| { | ||
| role: "", | ||
| } | ||
| ]}}`, | ||
| ); | ||
|
|
||
| // remove all table data | ||
| propPane.UpdatePropertyFieldValue("Table data", `[]`); | ||
|
|
||
| // allow adding a row | ||
| propPane.TogglePropertyState("Allow adding a row", "On"); | ||
|
|
||
| // Edit role column to select type | ||
| table.ChangeColumnType("role", "Select", "v2"); | ||
| table.EditColumn("role", "v2"); | ||
| propPane.TogglePropertyState("Editable", "On"); | ||
|
|
||
| // Add data to select options | ||
| agHelper.UpdateCodeInput( | ||
| locators._controlOption, | ||
| ` | ||
| {{ | ||
| [ | ||
| {"label": "Software Engineer", | ||
| "value": 1,}, | ||
| {"label": "Product Manager", | ||
| "value": 2,}, | ||
| {"label": "UX Designer", | ||
| "value": 3,} | ||
| ] | ||
| }} | ||
| `, | ||
| ); | ||
|
|
||
| table.AddNewRow(); | ||
| agHelper.GetNClick(commonlocators.singleSelectWidgetButtonControl); | ||
| agHelper | ||
| .GetElement(commonlocators.singleSelectWidgetMenuItem) | ||
| .contains("Software Engineer") | ||
| .click(); | ||
| }); |
There was a problem hiding this comment.
Test implementation needs refinement.
While the test case correctly validates the empty table dropdown functionality, there are several areas that need attention to align with the coding guidelines:
- Remove the cy.wait call and replace with proper assertions
- Extract string literals into constants
- Use data-* attributes for selectors
Apply these improvements:
- table.ChangeColumnType("role", "Select", "v2");
+ const COLUMN_TYPE = {
+ SELECT: "Select"
+ };
+ const ROLE_OPTIONS = {
+ SOFTWARE_ENGINEER: "Software Engineer"
+ };
+ table.ChangeColumnType("role", COLUMN_TYPE.SELECT, "v2");
- agHelper.GetElement(commonlocators.singleSelectWidgetMenuItem)
- .contains("Software Engineer")
- .click();
+ agHelper.GetElement(commonlocators.singleSelectWidgetMenuItem)
+ .contains(ROLE_OPTIONS.SOFTWARE_ENGINEER)
+ .should("exist")
+ .click();Committable suggestion was skipped due to low confidence.
| containsTableSpecificBindings = (stringToEvaluate: string) => { | ||
| return ( | ||
| stringToEvaluate.includes("currentIndex") || | ||
| stringToEvaluate.includes("currentRow") | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Improve string matching in containsTableSpecificBindings.
Using string.includes(...) may lead to false positives if currentIndex or currentRow appear within other identifiers (e.g., currentIndexValue). Consider using word boundaries to ensure accurate matching.
Apply this diff to enhance the accuracy:
containsTableSpecificBindings = (stringToEvaluate: string) => {
- return (
- stringToEvaluate.includes("currentIndex") ||
- stringToEvaluate.includes("currentRow")
- );
+ const regex = /\b(currentIndex|currentRow)\b/;
+ return regex.test(stringToEvaluate);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| containsTableSpecificBindings = (stringToEvaluate: string) => { | |
| return ( | |
| stringToEvaluate.includes("currentIndex") || | |
| stringToEvaluate.includes("currentRow") | |
| ); | |
| }; | |
| containsTableSpecificBindings = (stringToEvaluate: string) => { | |
| const regex = /\b(currentIndex|currentRow)\b/; | |
| return regex.test(stringToEvaluate); | |
| }; |
| ); | ||
| }; | ||
|
|
||
| containsTableSpecificBindings = (stringToEvaluate: string) => { |
There was a problem hiding this comment.
The functions that are extracted are just too shallow. I would do such abstractions only if they are used at atleast 3 places or doing something complex. But in this case, it is very straight forward, so extracting thesr won't help much, insteads, makes the code a little complex.
There was a problem hiding this comment.
@jsartisan I want to understand this better. Can you tell in detail, or share a link that explains this concept?
There was a problem hiding this comment.
The main idea is that modules should be deep. Methods like wrapInDoubleCurlyBraces and shouldReturnValueDirectly are too shallow - they don't hide enough complexity. These shallow methods actually increase cognitive load rather than reducing it.
I definitely recommend reading "Philosphy of software design".
| if (stringToEvaluate === "") { | ||
| return stringToEvaluate; | ||
| } | ||
|
|
||
| // Handle specific cases with "currentIndex" or "currentRow" |
There was a problem hiding this comment.
The comment is vey vague. Can you add a little more context why we need to handle this specific case?
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11550725809. |
|
Deploy-Preview-URL: https://ce-37108.dp.appsmith.com |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Problem
When a user attempts to add a new row to a table with a column of type "Select," the dropdown fails to display options if the table is initially empty, and the
selectOptionscontain dynamic data (e.g., expressions in curly braces). This behavior deviates from the expected outcome, where the dropdown should always show options, except when the dynamic data explicitly depends on values like currentRow or currentIndex.Root Cause
The issue lies in the
getComputedValuefunction, which computes table data for both evaluated and non-evaluated fields in the property pane. When the table is empty, and dynamic options are present, the function incorrectly generates an evaluation expression that unnecessarily maps over the table's data using this binding prefix:{{${tableName}.processedTableData.map((currentRow, currentIndex) => (Solution
To resolve this, the
getComputedValuefunction has been improved to detect when dynamic data does not reference currentRow or currentIndex. In such cases, the function no longer includes the mapping logic in its evaluation. This change ensures that select dropdown options are correctly populated even when the table is empty and the dynamic data does not rely on table-specific values.Fixes #23470
Automation
/ok-to-test tags="@tag.Table, @tag.Binding, @tag.Select, @tag.Sanity, @tag.Widget"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11547422311
Commit: a2b638f
Cypress dashboard.
Tags: @tag.Table, @tag.Binding, @tag.Select, @tag.Sanity, @tag.Widget
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Derived_Column_Data_validation_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_PropertyPane_1_spec.js
List of identified flaky tests.Mon, 28 Oct 2024 06:21:01 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
TableV2widget, ensuring proper UI behavior when no data exists.ComputeTablePropertyControlV2, improving readability and functionality.