fix: resolve empty table dropdown issue with dynamic select options in add new row functionality (#37108)
## Description
**Problem**
When using a Table widget's Select column type with dynamic options, the
computed value binding fails to handle empty table states correctly. If
the table has no data (`processedTableData` is empty), the dynamic
options evaluation still attempts to map over the non-existent table
data, resulting in an empty array instead of the expected options.
**Root Cause**
The issue stems from the `getComputedValue` function always using the
table mapping binding prefix:
```typescript
{{${tableName}.processedTableData.map((currentRow, currentIndex) => (
// dynamic options here
))}}
```
This creates an unnecessary dependency on table data even when the
dynamic options don't reference `currentRow` or `currentIndex`, causing
evaluation to fail when the table is empty.
### Problematic Evaluation
When the table is empty, expressions like this in table widget computed
properties:
```typescript
{{[
{ label: "Released", value: "Released" },
{ label: "Not Released", value: "Not Released" }
]}}
```
Would evaluate to an empty array `[]` because it's wrapped in a `.map()`
over empty table data.
**Solution**
Updated the binding logic to account for scenarios where table does not
have data and return the evaluated string directly in an IIFE
1. Updated the binding prefix and suffix
```typescript
static getBindingPrefix = (tableName: string) => {
return `{{(() => { const tableData = ${tableName}.processedTableData || []; return tableData.length > 0 ? tableData.map((currentRow, currentIndex) => (`;
};
static getBindingSuffix = (stringToEvaluate: string) => {
return `)) : ${stringToEvaluate} })()}}`;
};
```
2. Refactored `getComputedValue` and `getInputComputedValue` to
implement the new bindings
3. Created a migration and migration test for the DSL change
This ensures that:
- Dynamic options not dependent on table context evaluate correctly even
with empty tables
- The component maintains consistent behaviour across all table states
The solution prevents unnecessary table data dependencies while
preserving the ability to use table-specific values when required.
Fixes #23470
## Automation
/ok-to-test tags="@tag.Table, @tag.Binding, @tag.Select, @tag.Sanity,
@tag.Widget"
### 🔍 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/13514895959>
> Commit: 0d2e78a0a7be63d4f70fc3499829621bd1761b3d
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13514895959&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Table, @tag.Binding, @tag.Select, @tag.Sanity,
@tag.Widget`
> Spec:
> <hr>Tue, 25 Feb 2025 07:52:52 UTC
<!-- end of auto-generated comment: Cypress test results -->
## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Enhanced test coverage for adding new rows in the `TableV2` widget,
ensuring proper UI behavior when no data exists.
- **Bug Fixes**
- Improved validation of UI elements based on the "Allow adding a row"
property.
- **Refactor**
- Streamlined logic for handling computed values in the
`ComputeTablePropertyControlV2`, improving readability and
functionality.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
parent
ec39a210e1
commit
002ee78966
|
|
@ -1,5 +1,7 @@
|
|||
import {
|
||||
agHelper,
|
||||
entityExplorer,
|
||||
locators,
|
||||
propPane,
|
||||
table,
|
||||
} from "../../../../../support/Objects/ObjectsCore";
|
||||
|
|
@ -185,5 +187,56 @@ describe(
|
|||
propPane.TogglePropertyState("Allow adding a row", "Off");
|
||||
cy.get(".t--add-new-row").should("not.exist");
|
||||
});
|
||||
|
||||
it("1.8. should show the selectOptions data of a new row select cell when no data in the table", () => {
|
||||
entityExplorer.DragDropWidgetNVerify("tablewidgetv2", 600, 750);
|
||||
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();
|
||||
});
|
||||
},
|
||||
);
|
||||
|
|
|
|||
|
|
@ -91,9 +91,10 @@ import { migrateTableServerSideFiltering } from "./migrations/086-migrate-table-
|
|||
import { migrateChartwidgetCustomEchartConfig } from "./migrations/087-migrate-chart-widget-customechartdata";
|
||||
import { migrateCustomWidgetDynamicHeight } from "./migrations/088-migrate-custom-widget-dynamic-height";
|
||||
import { migrateTableWidgetV2CurrentRowInValidationsBinding } from "./migrations/089-migrage-table-widget-v2-currentRow-binding";
|
||||
import { migrateTableComputeValueBinding } from "./migrations/090-migrate-table-compute-value-binding";
|
||||
import type { DSLWidget } from "./types";
|
||||
|
||||
export const LATEST_DSL_VERSION = 91;
|
||||
export const LATEST_DSL_VERSION = 92;
|
||||
|
||||
export const calculateDynamicHeight = () => {
|
||||
const DEFAULT_GRID_ROW_HEIGHT = 10;
|
||||
|
|
@ -617,6 +618,11 @@ const migrateVersionedDSL = async (currentDSL: DSLWidget, newPage = false) => {
|
|||
}
|
||||
|
||||
if (currentDSL.version === 90) {
|
||||
currentDSL = migrateTableComputeValueBinding(currentDSL);
|
||||
currentDSL.version = 91;
|
||||
}
|
||||
|
||||
if (currentDSL.version === 91) {
|
||||
/**
|
||||
* This is just a version bump without any migration
|
||||
* History: With this PR: https://github.com/appsmithorg/appsmith/pull/38391
|
||||
|
|
|
|||
|
|
@ -0,0 +1,43 @@
|
|||
import type { DSLWidget } from "../types";
|
||||
import { isDynamicValue } from "../utils";
|
||||
|
||||
/**
|
||||
* This migration updates the table compute value bindings to use the new robust fallback mechanism
|
||||
* Old format: {{table.processedTableData.map((currentRow, currentIndex) => ( value ))}}
|
||||
* New format: {{(() => { const tableData = table.processedTableData || []; return tableData.length > 0 ? tableData.map((currentRow, currentIndex) => (value)) : value })()}}
|
||||
*/
|
||||
export const migrateTableComputeValueBinding = (currentDSL: DSLWidget) => {
|
||||
if (currentDSL.type === "TABLE_WIDGET_V2") {
|
||||
// Migrate primary columns compute values
|
||||
if (currentDSL.primaryColumns) {
|
||||
Object.keys(currentDSL.primaryColumns).forEach((columnKey) => {
|
||||
const column = currentDSL.primaryColumns[columnKey];
|
||||
|
||||
if (column.computedValue && isDynamicValue(column.computedValue)) {
|
||||
const oldBindingPrefix = `{{${currentDSL.widgetName}.processedTableData.map((currentRow, currentIndex) => (`;
|
||||
const oldBindingSuffix = `))}}`;
|
||||
|
||||
if (column.computedValue.includes(oldBindingPrefix)) {
|
||||
// Extract the value expression from between the old binding
|
||||
const valueExpression = column.computedValue.substring(
|
||||
oldBindingPrefix.length,
|
||||
column.computedValue.length - oldBindingSuffix.length,
|
||||
);
|
||||
|
||||
// Create new binding with fallback mechanism
|
||||
column.computedValue = `{{(() => { const tableData = ${currentDSL.widgetName}.processedTableData || []; return tableData.length > 0 ? tableData.map((currentRow, currentIndex) => (${valueExpression})) : ${valueExpression} })()}}`;
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Recursively migrate children
|
||||
if (currentDSL.children && currentDSL.children.length > 0) {
|
||||
currentDSL.children = currentDSL.children.map((child: DSLWidget) =>
|
||||
migrateTableComputeValueBinding(child),
|
||||
);
|
||||
}
|
||||
|
||||
return currentDSL;
|
||||
};
|
||||
|
|
@ -90,6 +90,7 @@ import * as m86 from "../migrations/086-migrate-table-server-side-filtering";
|
|||
import * as m87 from "../migrations/087-migrate-chart-widget-customechartdata";
|
||||
import * as m88 from "../migrations/088-migrate-custom-widget-dynamic-height";
|
||||
import * as m89 from "../migrations/089-migrage-table-widget-v2-currentRow-binding";
|
||||
import * as m90 from "../migrations/090-migrate-table-compute-value-binding";
|
||||
|
||||
interface Migration {
|
||||
functionLookup: {
|
||||
|
|
@ -931,9 +932,18 @@ const migrations: Migration[] = [
|
|||
version: 89,
|
||||
},
|
||||
{
|
||||
functionLookup: [],
|
||||
functionLookup: [
|
||||
{
|
||||
moduleObj: m90,
|
||||
functionName: "migrateTableComputeValueBinding",
|
||||
},
|
||||
],
|
||||
version: 90,
|
||||
},
|
||||
{
|
||||
functionLookup: [],
|
||||
version: 91,
|
||||
},
|
||||
];
|
||||
|
||||
const mockFnObj: Record<number, any> = {};
|
||||
|
|
|
|||
|
|
@ -0,0 +1,28 @@
|
|||
export const tableComputeBindingInputDsl = {
|
||||
widgetName: "Table1",
|
||||
type: "TABLE_WIDGET_V2",
|
||||
primaryColumns: {
|
||||
column1: {
|
||||
computedValue:
|
||||
"{{Table1.processedTableData.map((currentRow, currentIndex) => (currentRow.value))}}",
|
||||
},
|
||||
column2: {
|
||||
computedValue: "static value",
|
||||
},
|
||||
},
|
||||
version: 1,
|
||||
children: [],
|
||||
};
|
||||
|
||||
export const tableComputeBindingOutputDsl = {
|
||||
...tableComputeBindingInputDsl,
|
||||
primaryColumns: {
|
||||
column1: {
|
||||
computedValue:
|
||||
"{{(() => { const tableData = Table1.processedTableData || []; return tableData.length > 0 ? tableData.map((currentRow, currentIndex) => (currentRow.value)) : currentRow.value })()}}",
|
||||
},
|
||||
column2: {
|
||||
computedValue: "static value",
|
||||
},
|
||||
},
|
||||
};
|
||||
|
|
@ -48,6 +48,11 @@ import {
|
|||
inputDsl as updateHeaderOptionsInputDsl,
|
||||
outputDsl as updateHeaderOptionsOutputDsl,
|
||||
} from "./DSLs/UpdateHeaderOptionsDSLs";
|
||||
import { migrateTableComputeValueBinding } from "../../migrations/090-migrate-table-compute-value-binding";
|
||||
import {
|
||||
tableComputeBindingInputDsl,
|
||||
tableComputeBindingOutputDsl,
|
||||
} from "./DSLs/TableComputeBindingDSLs";
|
||||
|
||||
describe("Table Widget Property Pane Upgrade", () => {
|
||||
it("To test primaryColumns are created for a simple table", () => {
|
||||
|
|
@ -136,3 +141,11 @@ describe("migrateTableWidgetV2CurrentRowInValidationsBinding", () => {
|
|||
).toEqual(currentRownInValidationsBindingOutput);
|
||||
});
|
||||
});
|
||||
|
||||
describe("migrateTableComputeValueBinding", () => {
|
||||
it("should migrate table compute value bindings to use new fallback mechanism", () => {
|
||||
expect(
|
||||
migrateTableComputeValueBinding(tableComputeBindingInputDsl),
|
||||
).toEqual(tableComputeBindingOutputDsl);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -0,0 +1,61 @@
|
|||
import ComputeTablePropertyControlV2 from "./TableComputeValue";
|
||||
|
||||
describe("ComputeTablePropertyControlV2.getInputComputedValue", () => {
|
||||
const tableName = "Table1";
|
||||
const inputVariations = [
|
||||
"currentRow.price",
|
||||
`
|
||||
[
|
||||
{
|
||||
"value": "male",
|
||||
"label": "male"
|
||||
},
|
||||
{
|
||||
"value": "female",
|
||||
"label": "female"
|
||||
}
|
||||
]
|
||||
`,
|
||||
`["123", "-456", "0.123", "-0.456"]`,
|
||||
`["true", "false"]`,
|
||||
`["null", "undefined"]`,
|
||||
`{
|
||||
"name": "John Doe",
|
||||
"age": 30,
|
||||
"isActive": true,
|
||||
"address": {
|
||||
"street": "123 Main St",
|
||||
"city": "Boston"
|
||||
},
|
||||
"hobbies": ["reading", "gaming"]
|
||||
}`,
|
||||
"() => { return true; }",
|
||||
"(x) => x * 2",
|
||||
"currentRow.price * 2",
|
||||
"currentRow.isValid && true",
|
||||
"!currentRow.isDeleted",
|
||||
];
|
||||
|
||||
it("1. should return the correct computed value", () => {
|
||||
inputVariations.forEach((input) => {
|
||||
const computedValue =
|
||||
ComputeTablePropertyControlV2.getTableComputeBinding(tableName, input);
|
||||
|
||||
expect(
|
||||
ComputeTablePropertyControlV2.getInputComputedValue(computedValue),
|
||||
).toBe(`{{${input}}}`);
|
||||
});
|
||||
});
|
||||
|
||||
it("2. should handle addition values", () => {
|
||||
const input = "currentRow.quantity + 5";
|
||||
const computedValue = ComputeTablePropertyControlV2.getTableComputeBinding(
|
||||
tableName,
|
||||
input,
|
||||
);
|
||||
|
||||
expect(
|
||||
ComputeTablePropertyControlV2.getInputComputedValue(computedValue),
|
||||
).toBe(`{{currentRow.quantity}}{{5}}`);
|
||||
});
|
||||
});
|
||||
|
|
@ -99,11 +99,12 @@ function InputText(props: InputTextProp) {
|
|||
}
|
||||
|
||||
class ComputeTablePropertyControlV2 extends BaseControl<ComputeTablePropertyControlPropsV2> {
|
||||
static getBindingPrefix(tableName: string) {
|
||||
return `{{${tableName}.processedTableData.map((currentRow, currentIndex) => ( `;
|
||||
}
|
||||
|
||||
static bindingSuffix = `))}}`;
|
||||
static getTableComputeBinding = (
|
||||
tableName: string,
|
||||
stringToEvaluate: string,
|
||||
) => {
|
||||
return `{{(() => { const tableData = ${tableName}.processedTableData || []; return tableData.length > 0 ? tableData.map((currentRow, currentIndex) => (${stringToEvaluate})) : ${stringToEvaluate} })()}}`;
|
||||
};
|
||||
|
||||
render() {
|
||||
const {
|
||||
|
|
@ -114,13 +115,9 @@ class ComputeTablePropertyControlV2 extends BaseControl<ComputeTablePropertyCont
|
|||
propertyValue,
|
||||
theme,
|
||||
} = this.props;
|
||||
const tableName = this.props.widgetProperties.widgetName;
|
||||
const value =
|
||||
propertyValue && isDynamicValue(propertyValue)
|
||||
? ComputeTablePropertyControlV2.getInputComputedValue(
|
||||
propertyValue,
|
||||
tableName,
|
||||
)
|
||||
? ComputeTablePropertyControlV2.getInputComputedValue(propertyValue)
|
||||
: propertyValue
|
||||
? propertyValue
|
||||
: defaultValue;
|
||||
|
|
@ -157,24 +154,37 @@ class ComputeTablePropertyControlV2 extends BaseControl<ComputeTablePropertyCont
|
|||
);
|
||||
}
|
||||
|
||||
static getInputComputedValue = (propertyValue: string, tableName: string) => {
|
||||
const bindingPrefix =
|
||||
ComputeTablePropertyControlV2.getBindingPrefix(tableName);
|
||||
static getInputComputedValue = (propertyValue: string) => {
|
||||
const MAP_FUNCTION_SIGNATURE = "map((currentRow, currentIndex) => (";
|
||||
|
||||
if (propertyValue.includes(bindingPrefix)) {
|
||||
const value = `${propertyValue.substring(
|
||||
bindingPrefix.length,
|
||||
propertyValue.length -
|
||||
ComputeTablePropertyControlV2.bindingSuffix.length,
|
||||
)}`;
|
||||
const isComputedValue = propertyValue.includes(MAP_FUNCTION_SIGNATURE);
|
||||
|
||||
return JSToString(value);
|
||||
} else {
|
||||
return propertyValue;
|
||||
}
|
||||
if (!isComputedValue) return propertyValue;
|
||||
|
||||
// Extract the computation logic from the full binding string
|
||||
// Input example: "{{(() => { const tableData = Table1.processedTableData || []; return tableData.length > 0 ? tableData.map((currentRow, currentIndex) => (currentRow.price * 2)) : currentRow.price * 2 })()}}"
|
||||
const mapSignatureIndex = propertyValue.indexOf(MAP_FUNCTION_SIGNATURE);
|
||||
|
||||
// Find the actual computation expression between the map parentheses
|
||||
const computationStart = mapSignatureIndex + MAP_FUNCTION_SIGNATURE.length;
|
||||
const computationEnd = propertyValue.indexOf("))", computationStart);
|
||||
|
||||
// Extract the computation expression between the map parentheses
|
||||
// Note: At this point, we're just extracting the raw expression like "currentRow.price * 2"
|
||||
// The actual removal of "currentRow." prefix happens later in JSToString()
|
||||
const computationExpression = propertyValue.substring(
|
||||
computationStart,
|
||||
computationEnd,
|
||||
);
|
||||
|
||||
return JSToString(computationExpression);
|
||||
};
|
||||
|
||||
getComputedValue = (value: string, tableName: string) => {
|
||||
// Return raw value if:
|
||||
// 1. The value is not a dynamic binding (not wrapped in {{...}})
|
||||
// 2. AND this control is not configured to handle array values via additionalControlData
|
||||
// This allows single values to be returned without table binding computation
|
||||
if (
|
||||
!isDynamicValue(value) &&
|
||||
!this.props.additionalControlData?.isArrayValue
|
||||
|
|
@ -188,9 +198,10 @@ class ComputeTablePropertyControlV2 extends BaseControl<ComputeTablePropertyCont
|
|||
return stringToEvaluate;
|
||||
}
|
||||
|
||||
return `${ComputeTablePropertyControlV2.getBindingPrefix(
|
||||
return ComputeTablePropertyControlV2.getTableComputeBinding(
|
||||
tableName,
|
||||
)}${stringToEvaluate}${ComputeTablePropertyControlV2.bindingSuffix}`;
|
||||
stringToEvaluate,
|
||||
);
|
||||
};
|
||||
|
||||
onTextChange = (event: React.ChangeEvent<HTMLTextAreaElement> | string) => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user