-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: JSONForm infinite re-rendering when field validation is set #35994
Conversation
…en a field has error
WalkthroughThe changes involve enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant Hook
participant ErrorHandler
User->>Form: Submit invalid data
Form->>Hook: Call useRegisterFieldValidity(isValid: false)
Hook->>ErrorHandler: Check for existing error
alt No existing error
ErrorHandler-->>Hook: setError() called
else Existing error present
ErrorHandler-->>Hook: No action taken
end
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.test.tsx (1 hunks)
- app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts (1 hunks)
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10628129558. |
Deploy-Preview-URL: https://ce-35994.dp.appsmith.com |
…5994) ## Description The setError function updates the state and the effect is also dependant on the state which triggers re-render. This is a classic case of re-rendering due to self state dependency and is fixed with a check if the error is present for a particular field then it doesn't need to set again Fixes #35995 ## 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/10627924838> > Commit: 1d5dec6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10627924838&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JSONForm` > Spec: > <hr>Fri, 30 Aug 2024 06:50:40 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** - Enhanced error handling logic in the form validation process to improve efficiency and correctness. - **Tests** - Added a new test case to validate the behavior of the form validation hook when encountering invalid field states. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…psmithorg#35994) ## Description The setError function updates the state and the effect is also dependant on the state which triggers re-render. This is a classic case of re-rendering due to self state dependency and is fixed with a check if the error is present for a particular field then it doesn't need to set again Fixes appsmithorg#35995 ## 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/10627924838> > Commit: 1d5dec6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10627924838&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JSONForm` > Spec: > <hr>Fri, 30 Aug 2024 06:50:40 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** - Enhanced error handling logic in the form validation process to improve efficiency and correctness. - **Tests** - Added a new test case to validate the behavior of the form validation hook when encountering invalid field states. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
The setError function updates the state and the effect is also dependant on the state which triggers re-render. This is a classic case of re-rendering due to self state dependency and is fixed with a check if the error is present for a particular field then it doesn't need to set again
Fixes #35995
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/10627924838
Commit: 1d5dec6
Cypress dashboard.
Tags:
@tag.JSONForm
Spec:
Fri, 30 Aug 2024 06:50:40 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests