feat: add unmount field validation logic to JSON form widget (#37220) #37259
Conversation
Implement `useUnmountFieldValidation` for better field validation during unmounting in JSON form widget
WalkthroughThe changes introduce a new import statement for 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
github-actions
bot
added
Enhancement
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11703417058. |
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
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/JSONFormWidget/fields/FieldRenderer.tsx (2)
Line range hint
11-14: Address TODO comment during this editSince we're editing the file now, we should address the eslint-disable comment by properly typing the options.
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - options?: Record<string, any>; + options?: Record<string, unknown>;
Line range hint
34-52: Consider documenting the interaction between unmount validation and visibility handlingThe existing comment about visibility and unmounting should be updated to clarify how the new unmount validation interacts with the visibility-based field removal.
Add a note about the unmount validation handling to the existing comment:
/** * When a component is hidden, the field is removed from the form and the component unmounts. * So formData update is triggered so that the particular field can be removed from the `formData`. + * The useUnmountFieldValidation hook ensures proper cleanup of validation state during this process. * * Alternative solution: enable `shouldUnregister` in useController which will * de-register in the RHF's internal formData state. There's a caveat which needs to be * solved before this is enabled i.e the ArrayField does not play well with this configuration. * Removing of one Array field item will reset all the items below it. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/widgets/JSONFormWidget/fields/FieldRenderer.tsx(2 hunks)
🔇 Additional comments (2)
app/client/src/widgets/JSONFormWidget/fields/FieldRenderer.tsx (2)
8-8: LGTM: Clean import statement
The import follows the project's conventions and is appropriately placed with other hook imports.
34-34: Verify unmount cleanup behavior
The hook placement is correct, but let's verify its cleanup behavior with array fields.
✅ Verification successful
The previous pattern might have been too specific. Let's try to find the hook implementation first and then analyze its cleanup behavior.
Cleanup behavior is properly implemented and tested ✅
The hook correctly handles unmount cleanup:
- Implements cleanup function in useEffect
- Triggers field validation on unmount
- Properly updates when fieldName changes
- Comprehensive test coverage including field name changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cleanup handling in the hook implementation
ast-grep --pattern 'useUnmountFieldValidation = function($_) {
$$$
useEffect(() => {
return () => {
$$$
}
}, $$$)
}'
Length of output: 138
Script:
#!/bin/bash
# Let's try a different pattern to find the hook implementation
rg -A 10 "export const useUnmountFieldValidation"
# Also search for any file containing this hook name
fd "useUnmountFieldValidation"
Length of output: 236
Script:
#!/bin/bash
# Let's examine the hook implementation
cat app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts
# Also check the test file to understand the expected behavior
cat app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.test.tsx
Length of output: 2820
|
Deploy-Preview-URL: https://ce-37259.dp.appsmith.com |
| @@ -30,6 +31,7 @@ function FieldRenderer({ | |||
|
|
|||
| const FieldComponent = FIELD_MAP[fieldType]; | |||
|
|
|||
| useUnmountFieldValidation({ fieldName }); | |||
There was a problem hiding this comment.
If you are adding it here; then doesn't it mean we might not need to explicitly do it for every field?
There was a problem hiding this comment.
I thought so as well, but the conditionally rendered inputs which are not inside ObjectField or Array don't trigger this.
There was a problem hiding this comment.
Basically this problem #28018 appears again if we remove it from all the other places.
…horg#37220) (appsmithorg#37259) ## Description <ins>Problem</ins> When deleting all fields of array item, submit became disabled. <ins>Root cause</ins> The JSON form widget did not properly handle field validation during unmounting of array items, leading to inconsistencies in the form's error state. <ins>Solution</ins> This PR implements `useUnmountFieldValidation` to `FieldRenderer.tsx`, enhancing field validation for array items in the JSON form widget. This PR handles... - Ensuring proper cleanup of field validation when an array field is removed from the form, resolving visibility issues in list view mode. - Maintaining a consistent and accurate form state even after field removal. Fixes appsmithorg#18752 _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/11703351095> > Commit: 521fd25 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11703351095&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JSONForm` > Spec: > <hr>Wed, 06 Nov 2024 12:45:42 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** - Introduced a new mechanism for handling field validation upon unmounting in the form widget. - **Bug Fixes** - Improved field validation logic to enhance form data management during component unmounting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Problem
When deleting all fields of array item, submit became disabled.
Root cause
The JSON form widget did not properly handle field validation during unmounting of array items, leading to inconsistencies in the form's error state.
Solution
This PR implements
useUnmountFieldValidationtoFieldRenderer.tsx, enhancing field validation for array items in the JSON form widget. This PR handles...Fixes #18752
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/11703351095
Commit: 521fd25
Cypress dashboard.
Tags:
@tag.JSONFormSpec:
Wed, 06 Nov 2024 12:45:42 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes