fix: Enhance error handling in ComputeTablePropertyControlV2 binding methods #38205
Conversation
…methods Updated the binding methods in `ComputeTablePropertyControlV2` to improve error handling. The binding prefix now includes a try-catch block to prevent runtime errors when processing table data, ensuring that null values are returned gracefully in case of exceptions.
WalkthroughThe pull request introduces enhancements to error handling in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12368869711. |
![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/components/propertyControls/TableComputeValue.tsx (1)
106-106: Consider logging errors before returning nullWhile returning null is a good fallback, silently swallowing errors might make debugging harder.
Consider adding error logging:
- static bindingSuffix = `); } catch (e) { return null; }})}}`; + static bindingSuffix = `); } catch (e) { console.error('Table computation error:', e); return null; }})}}`;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/components/propertyControls/TableComputeValue.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/widget/reactTableUtils/transformDataPureFn.tsx(1 hunks)
🔇 Additional comments (3)
app/client/src/widgets/TableWidgetV2/widget/reactTableUtils/transformDataPureFn.tsx (1)
19-19: Good type safety improvement!
Changing from any to unknown enforces better type checking and reduces potential runtime errors.
app/client/src/components/propertyControls/TableComputeValue.tsx (2)
103-103: Good addition of try block for error isolation
The try block ensures that errors in one row's computation don't affect other rows.
103-106: Verify impact on existing error handling
The new error handling affects all computed values. Let's verify there are no duplicate error handlers or logging.
✅ Verification successful
Let me analyze the error handling patterns in the codebase to verify if there are any duplicate handlers or logging conflicts.
Error handling implementation is appropriate
The error handling in TableComputeValue.tsx is correctly implemented and doesn't conflict with other error handlers. The catch block returns null for computation errors, while other handlers in the table widget ecosystem handle specific scenarios (date parsing, local data reading, etc.) with appropriate logging.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing error handling in table computation code
rg -A 2 "catch \(e\)" --type ts
Length of output: 65866
Script:
#!/bin/bash
# Search for error handling in table computation and related files
rg -A 2 "catch \(e\)" --type ts app/client/src/components/propertyControls/TableComputeValue.tsx app/client/src/widgets/TableWidgetV2/widget/utilities.ts app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts app/client/src/widgets/TableWidgetV2/widget/reactTableUtils/transformDataPureFn.tsx
Length of output: 2546
|
Deploy-Preview-URL: https://ce-38205.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12373445425. |
|
Deploy-Preview-URL: https://ce-38205.dp.appsmith.com |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
…o rahulbarwal/issue38157/Bug-JSON-values-are-shown-in-the-table-cell-when-one-of-the-rows-does-not-contain-corresponding-value
- Introduced a new migration (090) to update computed value bindings in TABLE_WIDGET_V2 to include try-catch blocks for error handling. - Updated LATEST_DSL_VERSION to 91 to reflect the new migration. - Added tests to ensure the migration correctly modifies the computed values as expected.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12503447032. |
|
Deploy-Preview-URL: https://ce-38205.dp.appsmith.com |
…perties - Introduced a new test case to verify that computed values in TableV2 correctly handle missing nested properties using try-catch blocks. - The test checks the behavior of the table when provided with data containing nested objects and missing properties, ensuring that it returns expected values or empty strings as appropriate.
rahulbarwal
requested review from
ApekshaBhosale,
sagar-qa007 and
a team
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/dsl/src/migrate/tests/TableWidgetV2/DSLs/ValidationTryCatchDSLs.ts (2)
16-31: Consider removingas anytype casting.
Using a more specific type definition forvalidationTryCatchInputwill help improve type safety.-} as any as DSLWidget; +} as DSLWidget;
33-48: Similarly, refine type definition forvalidationTryCatchOutput.
Maintaining strict typing helps catch errors earlier.-} as any as DSLWidget; +} as DSLWidget;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Derived_Column_Data_validation_spec.js(1 hunks)app/client/packages/dsl/src/migrate/index.ts(2 hunks)app/client/packages/dsl/src/migrate/migrations/090-migrate-table-widget-v2-validation-try-catch.ts(1 hunks)app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts(2 hunks)app/client/packages/dsl/src/migrate/tests/TableWidgetV2/DSLs/ValidationTryCatchDSLs.ts(1 hunks)app/client/packages/dsl/src/migrate/tests/TableWidgetV2/ValidationTryCatch.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Derived_Column_Data_validation_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 (8)
app/client/packages/dsl/src/migrate/tests/TableWidgetV2/ValidationTryCatch.test.ts (1)
1-15: LGTM!
The test effectively verifies that try-catch blocks are inserted into the computed values.
app/client/packages/dsl/src/migrate/migrations/090-migrate-table-widget-v2-validation-try-catch.ts (1)
15-46: Overall approach is aligned with migration best practices.
The code properly verifies matching prefixes before updating to the new error-handling variant.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Derived_Column_Data_validation_spec.js (1)
125-128:
Fix possible mustache brace mismatch in table data.
It appears there's a missing closing curly brace. This might cause the expression to fail.
-propPane.UpdatePropertyFieldValue(
- "Table data",
- `{{[{"name": "Rahul", age: {value: 31}}, {"name": "Jacq", age: {}}, {"name": "John"}]}`
-);
+propPane.UpdatePropertyFieldValue(
+ "Table data",
+ `{{[{"name": "Rahul", age: {value: 31}}, {"name": "Jacq", age: {}}, {"name": "John"}]}}`
+);Likely invalid or redundant comment.
app/client/packages/dsl/src/migrate/index.ts (3)
94-94: New migration import is consistent.
This import correctly integrates the new migration function into the existing migration framework.
97-97: DSL version increment aligns with new migration.
Updating LATEST_DSL_VERSION to 91 properly reflects the addition of the new migration step.
617-621: Ensures version 90 is handled with new try-catch logic.
These lines correctly insert the new migration step into the versioned DSL flow. This adds robust error handling for table widget validations and ensures a seamless jump to the latest version.
app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts (2)
93-93: Accurate import of new migration module.
Importing m90 ensures the test suite can reference the new migration function.
934-942: Properly adds the new migration function to the test sequence.
Including the migrateTableWidgetV2ValidationTryCatch entry at version 90 maintains synchronization with the production migration logic.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Description
Updated the binding methods in ComputeTablePropertyControlV2 to improve error handling. The binding prefix now includes a try-catch block to prevent runtime errors when processing table data, ensuring that null values are returned gracefully in case of exceptions.
Fixes #38157
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Table"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12515152114
Commit: 36d974f
Cypress dashboard.
Tags: @tag.Table
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Derived_Column_Data_validation_spec.js
List of identified flaky tests.Fri, 27 Dec 2024 11:45:09 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation