fix: when value of array is empty, no need for data type recalculation #38794
Conversation
- Changed parameter type from `any` to `unknown` in `dataTypeFor` and `subDataTypeFor` functions for improved type safety. - Added a check to prevent unnecessary recalculation of sub data types when arrays are empty in `checkIfArrayAndSubDataTypeChanged`. This refactor enhances type safety and optimizes performance in the schema parsing logic.
- Updated the `.fieldTypeFor` tests to improve coverage and accuracy. - Added specific test cases for primitive values, email detection, date formats, array handling, and object types. - Refactored existing tests for clarity and consistency, ensuring all edge cases are addressed.
WalkthroughThe pull request focuses on enhancing the test suite and type safety for the JSON Form Widget's schema parsing functionality. The changes involve updating test cases in Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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
![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/JSONFormWidget/schemaParser.test.ts (3)
1890-1902: Consider adding more edge cases for email validation.While the current test cases cover basic email formats, consider adding tests for:
- Emails with multiple dots in domain
- Emails with special characters in local part
- International email addresses (IDN)
const testCases = [ { input: "[email protected]", expected: FieldType.EMAIL_INPUT }, { input: "[email protected]", expected: FieldType.EMAIL_INPUT }, + { input: "[email protected]", expected: FieldType.EMAIL_INPUT }, + { input: "[email protected]", expected: FieldType.EMAIL_INPUT }, + { input: "valid@domain.みんな", expected: FieldType.EMAIL_INPUT }, { input: "invalid.email@", expected: FieldType.TEXT_INPUT }, { input: "@invalid.com", expected: FieldType.TEXT_INPUT }, { input: "not-an-email", expected: FieldType.TEXT_INPUT }, ];
1930-1960: Consider adding validation for mixed-type arrays.The test cases handle homogeneous arrays well, but consider adding tests for arrays with mixed types to ensure robust handling.
const testCases = [ // ... existing cases ... + { + input: ["string", 42, true, { mixed: "types" }], + expected: FieldType.ARRAY, + }, ];
1962-1981: Consider adding tests for objects with array values.The object test cases could be enhanced by testing objects containing array values.
const testCases = [ // ... existing cases ... + { + input: { arrayField: [1, 2, 3], nested: { arrayField: ["a", "b"] } }, + expected: FieldType.OBJECT, + }, ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/JSONFormWidget/schemaParser.test.ts(1 hunks)
⏰ 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-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (3)
app/client/src/widgets/JSONFormWidget/schemaParser.test.ts (3)
1876-1888: Well-structured test cases for primitive values.The test cases cover essential primitive types and edge cases like null and undefined.
1904-1928: Comprehensive date format test coverage.Good coverage of various date formats including ISO, regional formats, and natural language dates.
1983-1993: Good coverage of special cases and edge values.The test cases handle non-standard JavaScript types appropriately.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12903364209. |
rahulbarwal
changed the title
rahulbarwal
added
ok-to-test
| * as it would be an empty array, which will always be `undefined`. | ||
| * which leads to unnecessary re calculation of a type(https://github.com/appsmithorg/appsmith/issues/37246) | ||
| */ | ||
| if (currentData.length === 0 || prevData.length === 0) return false; |
There was a problem hiding this comment.
Nit: Do you think we should move this condition above the subDataTypeFor and exit early?
There was a problem hiding this comment.
@ashit-rath In the scenario when it does not work, the source data for JSON from looks like this:
- Here if I select row at index 0, then the value it receives in this line is [].
- From the previous evaluation check JSONForm knows that value type is
Multiselectand subtype isstring - For the new row, value is
Multiselectbut subtype isundefined - This needs to revaluation of the type for the JSON form(which subsequently leads to resetting of data in property pane)
What we want to prevent is the check diff between prev and current when one of them turns out to be empty. I don't think that making subDataTypeFor exit early will help us in ☝️ .
rahulbarwal
deleted the
rahulbarwal/issue37246/Bug-Lose-binding-to-a-script-or-api-with-no-errors
branch
appsmithorg#38794) ## Description <ins>Problem</ins> When we have a `select/multi-select` field type and the source data gives empty array to the `JSONFormWidget`, the widget tries to gauge the sub type for value inside the array, and since it is empty it got `undefined` This led to re-evaluation of property config for the field. <ins>Solution</ins> - Added a check to prevent unnecessary recalculation of sub data types when arrays are empty in `checkIfArrayAndSubDataTypeChanged`. - Changed parameter type from `any` to `unknown` in `dataTypeFor` and `subDataTypeFor` functions for improved type safety. - Added and refactored unit tests. This refactor enhances type safety and optimizes performance in the schema parsing logic. Fixes appsmithorg#37246 _or_ Fixes `Issue URL` > [!WARNING] > _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.JSONForm" ### 🔍 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/12905055896> > Commit: cad7015 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12905055896&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JSONForm` > Spec: > <hr>Wed, 22 Jan 2025 09:50:23 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 - **Tests** - Enhanced test suite for determining field types. - Added comprehensive test cases covering primitive values, email formats, date formats, array types, object types, and edge cases. - **Refactor** - Updated type annotations in schema parser functions to improve type safety. - Changed parameter types from `any` to `unknown`. - Added clarifying comment for handling empty arrays in type checking. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Problem
When we have a
select/multi-selectfield type and the source data gives empty array to theJSONFormWidget, the widget tries to gauge the sub type for value inside the array, and since it is empty it gotundefinedThis led to re-evaluation of property config for the field.
Solution
Added a check to prevent unnecessary recalculation of sub data types when arrays are empty in
checkIfArrayAndSubDataTypeChanged.Changed parameter type from
anytounknownindataTypeForandsubDataTypeForfunctions for improved type safety.Added and refactored unit tests.
This refactor enhances type safety and optimizes performance in the schema parsing logic.
Fixes #37246
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.JSONForm"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12905055896
Commit: cad7015
Cypress dashboard.
Tags:
@tag.JSONFormSpec:
Wed, 22 Jan 2025 09:50:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Tests
Refactor
anytounknown.