fix: JSON form validation trigger on child component update#37128
fix: JSON form validation trigger on child component update#37128
Conversation
This commit fixes an edge case where form validation was not triggered when the child component was updated. The issue was reported in the GitHub issue #28018. This commit adds a trigger to ensure that form validation is performed correctly. Refactor the `Form` component in `app/client/src/widgets/JSONFormWidget/component/Form.tsx` to include the `trigger` function from the `methods` object. This ensures that form validation is triggered when the child component is updated. Closes #28018
WalkthroughThe changes in this pull request introduce a new test suite for the Changes
Assessment against linked issues
Possibly related PRs
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
rahulbarwal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/JSONFormWidget/component/Form.tsx (1)
Line range hint 182-196: Consider clearing debounced callbacks
The debounced updateFormData callback might retain sensitive form data in closure. Consider clearing the debounced callback in the cleanup function.
useEffect(() => {
const debouncedUpdateFormData = debounce(updateFormData, DEBOUNCE_TIMEOUT);
const formData = getFormData();
// ... rest of the effect
return () => {
subscription.unsubscribe();
unregisterResetObserver();
+ debouncedUpdateFormData.cancel();
};
}, []);📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/JSONFormWidget/component/Form.test.tsx (1 hunks)
- app/client/src/widgets/JSONFormWidget/component/Form.tsx (4 hunks)
🔇 Additional comments (3)
app/client/src/widgets/JSONFormWidget/component/Form.test.tsx (1)
1-199:
Add missing test coverage for hidden field validation.
The PR objectives mention fixing validation for hidden fields, but there are no tests covering this scenario.
Add test cases for hidden field validation:
describe("hidden field validation", () => {
it("should not validate hidden required fields", () => {
const mockTrigger = jest.fn();
const mockWatch = jest.fn();
(useForm as jest.Mock).mockReturnValue({
...defaultMockFormValues,
trigger: mockTrigger,
watch: mockWatch
});
render(
<Form {...defaultProps}>
<input data-hidden="true" required />
</Form>
);
expect(mockTrigger).toHaveBeenCalled();
// Add assertions for hidden field validation
});
});app/client/src/widgets/JSONFormWidget/component/Form.tsx (2)
155-155: LGTM: Good addition of trigger method
The addition of the trigger method from useForm is essential for the validation fix.
308-308: LGTM: Good test improvements
The addition of data-testid attributes improves test reliability and follows testing best practices.
Also applies to: 318-318
| jest.mock("react-hook-form", () => ({ | ||
| __esModule: true, | ||
| useForm: jest.fn(), | ||
| FormProvider: jest.fn(({ children }) => <div>{children}</div>), | ||
| })); | ||
|
|
||
| jest.mock("./useFixedFooter", () => ({ | ||
| __esModule: true, | ||
| default: jest.fn(), | ||
| })); | ||
|
|
||
| jest.mock("widgets/ButtonWidget/component", () => ({ | ||
| __esModule: true, | ||
| BaseButton: jest.fn(({ children, ...props }) => ( | ||
| <button {...props}>{children}</button> | ||
| )), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating mock implementations.
The mock setup for multiple components could be moved to a separate test utils file for better reusability across test suites.
+ // Move to __mocks__/testUtils.ts
+ export const mockComponents = () => {
+ jest.mock("react-hook-form", () => ({
+ __esModule: true,
+ useForm: jest.fn(),
+ FormProvider: jest.fn(({ children }) => <div>{children}</div>),
+ }));
+
+ // ... other mocks
+ };Committable suggestion was skipped due to low confidence.
| useEffect(() => { | ||
| // This effect is to handle an edge-case: | ||
| // https://github.com/appsmithorg/appsmith/issues/28018 | ||
| trigger(); |
There was a problem hiding this comment.
I didn't quite understood the solution here, why are we re-validating the whole form when children changes? Isn't it a problem of when reset button is pressed the validation persists?
There was a problem hiding this comment.
@ashit-rath I don't think so, the problem is the validation not getting triggered when children changes. When reset button is pressed, then the validation change is triggered.
There was a problem hiding this comment.
@ashit-rath
Let me take you with a dry run of whats happening in the original issue #28018
- User select red and get one input which bound to red. It is empty hence, errors has
{red: required:true}. - user adds value, errors are cleared. and user can submit.
- Now they click reset, the input is cleared and errorState has this:
{red: required:true} - Now user switches to blue, and gets input bound to blue, which adds
{blue: required:true} - Over all errorState looks liks:
{red: required:true, blue: required:true} - Now even if you type in blue input, the red error never goes away.
I am open to ideas if this call to trigger can be optimized. Looking forward to your ideas.
There was a problem hiding this comment.
Thanks @rahulbarwal and @jacquesikot for explaining this in detail, I understood the problem. I believe this is a reset problem; if you look at the RESET_OPTIONS which gets passed to onReset has keepErrors: true. Have we checked if this was set to false for the case when button is clicked; solves the problem?
There was a problem hiding this comment.
Thanks @ashit-rath for this input.
keepErrors: false did not work, and I think I know why:
In step 3 above, when user resets the form, the red input is visible and Here, whether we keep or discard errors does not matter.
IT is on step 4 that multiple errors enter the formState(where we are not doing the reset).
There was a problem hiding this comment.
@rahulbarwal Understood! Can we profile and understand the performance implication of having children as a dependency of an effect? esp. with Array data types in jsonform where the form size increases with user interaction?
Also this method trigger accepts a field name to re-trigger validation for it. So when the validation of red changes in step 4, basically we need to re-validate. I'm assuming the validation it gets from the required property is correctly set and we just need to trigger for re-validation. So there is a hook called useRegisterFieldValidity where we deal with field level error state setting/unsetting; can we use this hook to actually call trigger()?
There was a problem hiding this comment.
Thanks @ashit-rath, for this feedback, I've incorporated these changes. please review again.
jacquesikot
left a comment
jacquesikot
left a comment
There was a problem hiding this comment.
Can we find some state that changes when formData changes, and then add that as a dependency in the useEffect
@jacquesikot Actually, the errors are dependant on whats visible on the UI rather than whats the value of state. |
…o rahulbarwal/issue28018
…37155) - Implemented visibility tracking for form fields based on schema. - Enhanced reactivity to visibility changes with useRef for tracking
…o rahulbarwal/issue28018
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/JSONFormWidget/component/Form.tsx (2)
154-154: LGTM with a suggestion for optimization.The visibility tracking ref and form trigger method are correctly implemented. Consider memoizing the trigger method to prevent unnecessary re-renders.
- const { formState, reset, trigger, watch } = methods; + const { formState, reset, trigger: rawTrigger, watch } = methods; + const trigger = useCallback(rawTrigger, [rawTrigger]);Also applies to: 156-156
277-335: Comprehensive visibility tracking implementation with room for optimization.The implementation correctly handles the visibility validation issue described in #28018. However, consider these improvements:
- Memoize the computeFieldVisibilityMap function to prevent unnecessary recomputations
- Add JSDoc comments to explain the complex recursive logic
+ /** Recursively computes visibility state for all fields in the form + * @param schemaItem - The schema item to process + * @param parentPath - The dot-notation path to the current item + * @returns Record of field paths and their visibility states + */ const computeFieldVisibilityMap = useCallback( (schemaItem: SchemaItem, parentPath = ""): Record<string, boolean> => { // ... existing implementation }, [] );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/widgets/JSONFormWidget/component/Form.tsx(5 hunks)
🔇 Additional comments (2)
app/client/src/widgets/JSONFormWidget/component/Form.tsx (2)
14-14: LGTM!
The SchemaItem type import is necessary for the new visibility tracking implementation.
363-363: LGTM!
The test IDs are well-named and consistent with testing best practices.
Also applies to: 373-373
- Cleans up test props for better clarity and focus on functionality
| describe("happy render path", () => { | ||
| beforeEach(() => mockUseForm()); | ||
| it("renders the form title", () => { | ||
| render(<Form {...defaultProps}>Form Content</Form>); | ||
| expect(screen.getByText("Test Form")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("renders children inside the form body", () => { | ||
| render(<Form {...defaultProps}>Form Content</Form>); | ||
| expect(screen.getByText("Form Content")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("renders the submit button with correct label", () => { | ||
| const { getByTestId } = render( | ||
| <Form {...defaultProps}>Form Content</Form>, | ||
| ); | ||
| const submitBtn = getByTestId(TEST_IDS.SUBMIT_BTN); | ||
|
|
||
| expect(submitBtn).toBeInTheDocument(); | ||
| expect(submitBtn).toHaveAttribute("text", defaultProps.submitButtonLabel); | ||
|
|
||
| fireEvent.click(submitBtn); | ||
| expect(defaultProps.onSubmit).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("renders the reset button with correct label when showReset is true", () => { | ||
| const { getByTestId } = render( | ||
| <Form {...defaultProps}>Form Content</Form>, | ||
| ); | ||
| const resetBtn = getByTestId(TEST_IDS.RESET_BTN); | ||
|
|
||
| expect(resetBtn).toBeInTheDocument(); | ||
| expect(resetBtn).toHaveAttribute("text", defaultProps.resetButtonLabel); | ||
| expect(defaultProps.registerResetObserver).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // Form updates data correctly when input values change | ||
| it("should update data when input values change", () => { | ||
| const mockUpdateFormData = jest.fn(); | ||
| const mockGetFormData = jest.fn().mockReturnValue({}); | ||
| const mockRegisterResetObserver = jest.fn(); | ||
| const mockUnregisterResetObserver = jest.fn(); | ||
| const mockOnSubmit = jest.fn(); | ||
| const mockOnFormValidityUpdate = jest.fn(); | ||
| const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem }; | ||
| const props = klona({ | ||
| ...defaultProps, | ||
| updateFormData: mockUpdateFormData, | ||
| getFormData: mockGetFormData, | ||
| registerResetObserver: mockRegisterResetObserver, | ||
| unregisterResetObserver: mockUnregisterResetObserver, | ||
| onSubmit: mockOnSubmit, | ||
| onFormValidityUpdate: mockOnFormValidityUpdate, | ||
| schema: mockSchema, | ||
| }); | ||
| const { getByTestId } = render(<Form {...props} />); | ||
|
|
||
| fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN)); | ||
| expect(mockUpdateFormData).toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add missing test coverage for form validation triggers.
The "happy render path" suite is missing tests for form validation triggers on child component updates.
Add the following test:
it("should trigger validation on child component updates", async () => {
const mockTrigger = jest.fn().mockResolvedValue(true);
(useForm as jest.Mock).mockReturnValue({
...defaultMockFormValues,
trigger: mockTrigger,
formState: { errors: {} },
});
const { rerender } = render(
<Form {...defaultProps}>
<input value="initial" />
</Form>
);
rerender(
<Form {...defaultProps}>
<input value="updated" />
</Form>
);
expect(mockTrigger).toHaveBeenCalled();
});| function mockUseForm(withErrors = false) { | ||
| (useForm as jest.Mock).mockReturnValue({ | ||
| formState: { | ||
| errors: withErrors | ||
| ? { | ||
| fieldName: { | ||
| type: "required", | ||
| message: "This field is required", | ||
| }, | ||
| } | ||
| : null, | ||
| }, | ||
| reset: jest.fn(), | ||
| trigger: jest.fn(), | ||
| watch: jest.fn(() => ({ | ||
| subscribe: jest.fn(), | ||
| unsubscribe: jest.fn(), | ||
| })), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance mockUseForm to support configurable trigger behavior.
The mockUseForm function should allow configuring the trigger function's return value to properly test validation scenarios.
function mockUseForm(withErrors = false) {
+ const mockTrigger = jest.fn().mockResolvedValue(!withErrors);
(useForm as jest.Mock).mockReturnValue({
formState: {
errors: withErrors
? {
fieldName: {
type: "required",
message: "This field is required",
},
}
: null,
},
reset: jest.fn(),
- trigger: jest.fn(),
+ trigger: mockTrigger,
watch: jest.fn(() => ({
subscribe: jest.fn(),
unsubscribe: jest.fn(),
})),
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function mockUseForm(withErrors = false) { | |
| (useForm as jest.Mock).mockReturnValue({ | |
| formState: { | |
| errors: withErrors | |
| ? { | |
| fieldName: { | |
| type: "required", | |
| message: "This field is required", | |
| }, | |
| } | |
| : null, | |
| }, | |
| reset: jest.fn(), | |
| trigger: jest.fn(), | |
| watch: jest.fn(() => ({ | |
| subscribe: jest.fn(), | |
| unsubscribe: jest.fn(), | |
| })), | |
| }); | |
| } | |
| function mockUseForm(withErrors = false) { | |
| const mockTrigger = jest.fn().mockResolvedValue(!withErrors); | |
| (useForm as jest.Mock).mockReturnValue({ | |
| formState: { | |
| errors: withErrors | |
| ? { | |
| fieldName: { | |
| type: "required", | |
| message: "This field is required", | |
| }, | |
| } | |
| : null, | |
| }, | |
| reset: jest.fn(), | |
| trigger: mockTrigger, | |
| watch: jest.fn(() => ({ | |
| subscribe: jest.fn(), | |
| unsubscribe: jest.fn(), | |
| })), | |
| }); | |
| } |
| it("triggers a check, necessitating a re-render, when the children in form are updated", () => { | ||
| mockUseForm(true); | ||
| const propsToUpdate = klona({ | ||
| ...defaultProps, | ||
| disabledWhenInvalid: true, | ||
| }); | ||
| const { getByTestId } = render( | ||
| <Form {...propsToUpdate}>Form Content</Form>, | ||
| ); | ||
|
|
||
| expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled(); | ||
| }); |
There was a problem hiding this comment.
Improve test assertions for child component updates.
The test lacks proper assertions to verify the trigger behavior when children are updated.
-it("triggers a check, necessitating a re-render, when the children in form are updated", () => {
+it("should trigger validation when children are updated", () => {
mockUseForm(true);
const propsToUpdate = klona({
...defaultProps,
disabledWhenInvalid: true,
});
- const { getByTestId } = render(
+ const { getByTestId, rerender } = render(
<Form {...propsToUpdate}>Form Content</Form>,
);
+
+ rerender(<Form {...propsToUpdate}>Updated Content</Form>);
+
+ const mockTrigger = (useForm as jest.Mock).mock.results[0].value.trigger;
+ expect(mockTrigger).toHaveBeenCalled();
expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("triggers a check, necessitating a re-render, when the children in form are updated", () => { | |
| mockUseForm(true); | |
| const propsToUpdate = klona({ | |
| ...defaultProps, | |
| disabledWhenInvalid: true, | |
| }); | |
| const { getByTestId } = render( | |
| <Form {...propsToUpdate}>Form Content</Form>, | |
| ); | |
| expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled(); | |
| }); | |
| it("should trigger validation when children are updated", () => { | |
| mockUseForm(true); | |
| const propsToUpdate = klona({ | |
| ...defaultProps, | |
| disabledWhenInvalid: true, | |
| }); | |
| const { getByTestId, rerender } = render( | |
| <Form {...propsToUpdate}>Form Content</Form>, | |
| ); | |
| rerender(<Form {...propsToUpdate}>Updated Content</Form>); | |
| const mockTrigger = (useForm as jest.Mock).mock.results[0].value.trigger; | |
| expect(mockTrigger).toHaveBeenCalled(); | |
| expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled(); | |
| }); |
| it("should update data when input values change", () => { | ||
| const mockUpdateFormData = jest.fn(); | ||
| const mockGetFormData = jest.fn().mockReturnValue({}); | ||
| const mockRegisterResetObserver = jest.fn(); | ||
| const mockUnregisterResetObserver = jest.fn(); | ||
| const mockOnSubmit = jest.fn(); | ||
| const mockOnFormValidityUpdate = jest.fn(); | ||
| const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem }; | ||
| const props = klona({ | ||
| ...defaultProps, | ||
| updateFormData: mockUpdateFormData, | ||
| getFormData: mockGetFormData, | ||
| registerResetObserver: mockRegisterResetObserver, | ||
| unregisterResetObserver: mockUnregisterResetObserver, | ||
| onSubmit: mockOnSubmit, | ||
| onFormValidityUpdate: mockOnFormValidityUpdate, | ||
| schema: mockSchema, | ||
| }); | ||
| const { getByTestId } = render(<Form {...props} />); | ||
|
|
||
| fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN)); | ||
| expect(mockUpdateFormData).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Test implementation doesn't match its description.
The test claims to verify input value changes but only tests form submission. Either rename the test or implement proper input change verification.
-it("should update data when input values change", () => {
+it("should update form data on submit", () => {
const mockUpdateFormData = jest.fn();
const mockGetFormData = jest.fn().mockReturnValue({});
const mockRegisterResetObserver = jest.fn();
const mockUnregisterResetObserver = jest.fn();
const mockOnSubmit = jest.fn();
const mockOnFormValidityUpdate = jest.fn();
const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
const props = klona({
...defaultProps,
updateFormData: mockUpdateFormData,
getFormData: mockGetFormData,
registerResetObserver: mockRegisterResetObserver,
unregisterResetObserver: mockUnregisterResetObserver,
onSubmit: mockOnSubmit,
onFormValidityUpdate: mockOnFormValidityUpdate,
schema: mockSchema,
});
const { getByTestId } = render(<Form {...props} />);
fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
expect(mockUpdateFormData).toHaveBeenCalled();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should update data when input values change", () => { | |
| const mockUpdateFormData = jest.fn(); | |
| const mockGetFormData = jest.fn().mockReturnValue({}); | |
| const mockRegisterResetObserver = jest.fn(); | |
| const mockUnregisterResetObserver = jest.fn(); | |
| const mockOnSubmit = jest.fn(); | |
| const mockOnFormValidityUpdate = jest.fn(); | |
| const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem }; | |
| const props = klona({ | |
| ...defaultProps, | |
| updateFormData: mockUpdateFormData, | |
| getFormData: mockGetFormData, | |
| registerResetObserver: mockRegisterResetObserver, | |
| unregisterResetObserver: mockUnregisterResetObserver, | |
| onSubmit: mockOnSubmit, | |
| onFormValidityUpdate: mockOnFormValidityUpdate, | |
| schema: mockSchema, | |
| }); | |
| const { getByTestId } = render(<Form {...props} />); | |
| fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN)); | |
| expect(mockUpdateFormData).toHaveBeenCalled(); | |
| }); | |
| it("should update form data on submit", () => { | |
| const mockUpdateFormData = jest.fn(); | |
| const mockGetFormData = jest.fn().mockReturnValue({}); | |
| const mockRegisterResetObserver = jest.fn(); | |
| const mockUnregisterResetObserver = jest.fn(); | |
| const mockOnSubmit = jest.fn(); | |
| const mockOnFormValidityUpdate = jest.fn(); | |
| const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem }; | |
| const props = klona({ | |
| ...defaultProps, | |
| updateFormData: mockUpdateFormData, | |
| getFormData: mockGetFormData, | |
| registerResetObserver: mockRegisterResetObserver, | |
| unregisterResetObserver: mockUnregisterResetObserver, | |
| onSubmit: mockOnSubmit, | |
| onFormValidityUpdate: mockOnFormValidityUpdate, | |
| schema: mockSchema, | |
| }); | |
| const { getByTestId } = render(<Form {...props} />); | |
| fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN)); | |
| expect(mockUpdateFormData).toHaveBeenCalled(); | |
| }); |
- Removed unnecessary parent path parameter for cleaner code
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/JSONFormWidget/component/Form.tsx (2)
277-327: Consider optimizing visibility computation performanceWhile the implementation correctly handles visibility changes, consider these optimizations:
- Memoize the computeFieldVisibilityMap function to prevent recreation on every render
- Consider implementing partial schema traversal by tracking changed paths
+ const computeFieldVisibilityMap = useCallback( (schemaItem: SchemaItem): Record<string, boolean> => { // ... existing implementation }, + [] + );
312-324: Add error handling for schema computationConsider adding error handling for edge cases in the visibility computation:
const currentVisibility = computeFieldVisibilityMap( - schema[ROOT_SCHEMA_KEY], + schema[ROOT_SCHEMA_KEY] ?? { fieldType: "Object", children: {} } ); + if (!currentVisibility) { + console.warn("Failed to compute field visibility map"); + return; + } const fieldsWithVisibilityChange = Object.entries(currentVisibility)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/widgets/JSONFormWidget/component/Form.tsx(5 hunks)
🔇 Additional comments (3)
app/client/src/widgets/JSONFormWidget/component/Form.tsx (3)
14-14: LGTM: Type import addition
The SchemaItem type import is necessary for the new visibility computation logic.
154-156: LGTM: State management setup
The previousVisibilityRef and trigger method additions are well-implemented for tracking field visibility changes and triggering validation.
355-355: LGTM: Test attributes addition
The data-testid attributes are well-named and follow testing best practices.
Also applies to: 365-365
…o rahulbarwal/issue28018
rahulbarwal
added
ok-to-test
…or cleaner code and better performance
…ion on unmount in JSONFormWidget
…on is triggered correctly on unmount and field name changes
…mproved validation handling on unmounting events
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (15)
app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts (3)
6-8: Consider using a more specific type for fieldNameWhile deriving from
ControllerProps["name"]works, consider usingPath<TFieldValues>from react-hook-form for more precise typing.+import type { Path } from "react-hook-form"; export interface UseUnmountFieldValidationProps { - fieldName: ControllerProps["name"]; + fieldName: Path<any>; // Or Path<YourFormType> for better type safety }
10-14: Add usage example in documentationThe documentation clearly explains the purpose, but adding a usage example would make it more developer-friendly.
/** * This hook triggers validation for a field when it unmounts * Useful for validating fields that become hidden/removed * https://github.com/appsmithorg/appsmith/issues/28018 + * + * @example + * function MyField() { + * useUnmountFieldValidation({ fieldName: "myField" }); + * return <input />; + * } */
29-29: Consider using named exportFor hooks, named exports are often preferred as they make imports more explicit and help with tree-shaking.
-export default useUnmountFieldValidation; +export { useUnmountFieldValidation };app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.test.tsx (3)
28-48: Add error handling test caseThe test verifies successful validation, but we should also test error scenarios. What happens if the trigger function throws an error?
Consider adding:
it("should handle trigger errors gracefully", () => { const errorTrigger = jest.fn().mockRejectedValue(new Error("Validation failed")); (useFormContext as jest.Mock).mockReturnValue({ trigger: errorTrigger }); const { unmount } = renderHook(() => useUnmountFieldValidation({ fieldName: "testField" }), ); unmount(); expect(errorTrigger).toHaveBeenCalled(); });
50-64: Consider testing rapid field name changesThe test verifies basic field name changes, but consider testing rapid changes to ensure proper cleanup of previous effects.
Consider adding:
it("should handle rapid field name changes", () => { const { rerender, unmount } = renderHook( ({ fieldName }) => useUnmountFieldValidation({ fieldName }), { initialProps: { fieldName: "field1" } } ); // Rapid changes rerender({ fieldName: "field2" }); rerender({ fieldName: "field3" }); rerender({ fieldName: "field4" }); unmount(); expect(mockTrigger).toHaveBeenCalledTimes(1); expect(mockTrigger).toHaveBeenLastCalledWith("field4"); });
1-65: Consider adding integration test for hidden fieldsWhile the unit tests are good, the PR objectives mention issues with hidden field validation. Consider adding an integration test that verifies the hook's behavior with actual hidden/shown fields.
This would help ensure the fix addresses the original issue #28018 where required validation wasn't updating correctly for hidden fields.
app/client/src/widgets/JSONFormWidget/fields/RadioGroupField.tsx (1)
Line range hint
19-70: Consider consolidating validation hooksThe component now uses two validation hooks:
useRegisterFieldValidityanduseUnmountFieldValidation. Consider consolidating these into a single hook to manage all validation scenarios.A unified hook could look like:
const useFieldValidation = ({ fieldName, fieldType, isValid, unmountBehavior, }: ValidationConfig) => { // Combined logic from both hooks };app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx (1)
Line range hint
69-74: Consider adding visibility check in validation logicGiven the PR objective to fix validation issues with hidden fields, consider adding a visibility check in the validation logic. The current implementation might not handle the case when the switch field is hidden.
useRegisterFieldValidity({ fieldName: name, fieldType: schemaItem.fieldType, - isValid: isValueValid, + isValid: schemaItem.isVisible ? isValueValid : true, });app/client/src/widgets/JSONFormWidget/fields/CheckboxField.tsx (2)
Line range hint
82-86: Consider documenting validation flowThe component now has multiple validation mechanisms:
- useController for form state
- useRegisterFieldValidity for field validity
- useUnmountFieldValidation for unmount validation
Consider adding a comment explaining how these validations work together to prevent future maintenance issues.
Line range hint
44-48: Consider enhancing isValid functionThe current isValid function might need to account for field visibility when determining validity, especially for required fields that are hidden.
const isValid = ( value: boolean, schemaItem: CheckboxFieldProps["schemaItem"], -) => !schemaItem.isRequired || Boolean(value); +) => { + // Skip validation if field is hidden + if (!schemaItem.isVisible) return true; + return !schemaItem.isRequired || Boolean(value); +};app/client/src/widgets/JSONFormWidget/fields/SelectField.tsx (1)
Line range hint
63-69: Consider enhancing validation for hidden fields.The current
isValidimplementation only checks for required status and value presence. Based on issue #28018, we should also consider field visibility in validation logic.Consider updating the validation logic:
export const isValid = ( schemaItem: SelectFieldProps["schemaItem"], value?: unknown, -) => !schemaItem.isRequired || (value !== "" && !isNil(value)); +) => { + if (!schemaItem.isVisible) return true; + return !schemaItem.isRequired || (value !== "" && !isNil(value)); +};app/client/src/widgets/JSONFormWidget/fields/DateField.tsx (1)
Line range hint
132-138: Consider documenting validation flowThe component now has multiple validation mechanisms:
- Field validity registration
- Unmount validation
- Required field validation
Consider adding a comment block explaining the validation lifecycle and the role of each mechanism.
app/client/src/widgets/JSONFormWidget/fields/MultiSelectField.tsx (1)
Line range hint
64-85: Consider extracting value transformation logicThe
fieldValuesToComponentValuesandcomponentValuesToFieldValuesfunctions could be moved to a separate utility file for better maintainability and potential reuse.-const fieldValuesToComponentValues = ( - values: LabelInValueType["value"][], - options: LabelInValueType[] = [], -) => { - return values.map((value) => { - const option = options.find((option) => option.value === value); - return option ? option : { value, label: value }; - }); -}; - -const componentValuesToFieldValues = ( - componentValues: LabelInValueType[] = [], -) => componentValues.map(({ value }) => value);Create a new file
utils/fieldTransformations.ts:export const fieldValuesToComponentValues = ( values: LabelInValueType["value"][], options: LabelInValueType[] = [], ) => { return values.map((value) => { const option = options.find((option) => option.value === value); return option ? option : { value, label: value }; }); }; export const componentValuesToFieldValues = ( componentValues: LabelInValueType[] = [], ) => componentValues.map(({ value }) => value);app/client/src/widgets/JSONFormWidget/fields/BaseInputField.tsx (2)
244-244: Consider documenting the validation lifecycleWhile the implementation is correct, adding a brief comment explaining the validation lifecycle (registration, updates, and unmounting) would help future maintainers understand the complete validation flow.
Line range hint
328-367: Consider refactoring validation logicThe
conditionalPropslogic handles multiple validation scenarios. Consider extracting the validation logic into a separate hook for better maintainability and testability.+ const useValidationProps = (schemaItem, isDirty, isValueValid, inputText, inputType, inputDefaultValue) => { + return useMemo(() => { + // ... existing conditionalProps logic + }, [schemaItem, isDirty, isValueValid, inputText]); + }; const conditionalProps = useValidationProps(schemaItem, isDirty, isValueValid, inputText, inputType, inputDefaultValue);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
app/client/src/widgets/JSONFormWidget/component/Form.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/BaseInputField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/CheckboxField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/DateField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/MultiSelectField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/RadioGroupField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/SelectField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.test.tsx(1 hunks)app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/JSONFormWidget/component/Form.tsx
🔇 Additional comments (16)
app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts (1)
1-29: Verify integration with Form component
The hook implementation looks solid for addressing issue #28018. Let's verify its integration:
✅ Verification successful
Implementation and integration verified successfully ✅
The hook is:
- Properly tested with comprehensive coverage including unmount and field name changes
- Correctly integrated in form fields (verified in CheckboxField)
- Used alongside other form validation hooks (useRegisterFieldValidity)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Form component integration
echo "Checking Form component integration..."
rg -l "useUnmountFieldValidation" app/client/src/widgets/JSONFormWidget/
# Verify test coverage
echo "Checking test coverage..."
rg -l "useUnmountFieldValidation.*test" app/client/src/widgets/JSONFormWidget/
Length of output: 894
Script:
#!/bin/bash
# Let's check the test implementation to verify proper coverage
cat app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.test.tsx
# Let's also check one of the field implementations to see how the hook is used
cat app/client/src/widgets/JSONFormWidget/fields/CheckboxField.tsx
Length of output: 6312
app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.test.tsx (2)
15-27: LGTM! Clean test setup
Good practice using beforeEach for mock setup and afterEach for cleanup.
11-13: Consider async behavior in telemetry mock
The current mock immediately executes the function, which might not catch potential async timing issues. Consider adding tests with async scenarios.
app/client/src/widgets/JSONFormWidget/fields/RadioGroupField.tsx (2)
19-19: LGTM: Clean import addition
The import statement is correctly placed with other hook imports.
70-70: Verify validation behavior with hidden fields
The hook implementation looks correct, but we should ensure it properly handles the specific case mentioned in issue #28018 where fields are hidden/shown based on dropdown selection.
app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx (2)
20-20: LGTM - Import statement is properly placed
74-74: Verify integration with existing validation flow
The hook is correctly placed, but let's verify that it doesn't conflict with the existing useRegisterFieldValidity hook.
app/client/src/widgets/JSONFormWidget/fields/CheckboxField.tsx (2)
21-21: LGTM: Clean import addition
The import statement follows the project's conventions and is appropriately placed with related imports.
86-86: Verify hidden field validation behavior
The hook implementation looks correct, but let's verify it properly handles the core issue of field validation when fields are hidden.
app/client/src/widgets/JSONFormWidget/fields/SelectField.tsx (1)
21-21: LGTM: Clean integration of unmount validation hook.
The addition of useUnmountFieldValidation hook is well-placed and consistent with other form field components.
Let's verify the hook implementation across other form fields:
Also applies to: 110-110
✅ Verification successful
Hook integration verified and consistent across form fields
The useUnmountFieldValidation hook is properly integrated in all form field components with consistent usage pattern, passing the fieldName parameter. The implementation in SelectField.tsx aligns with the established pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent hook usage across form fields
rg "useUnmountFieldValidation.*fieldName: \w+" "app/client/src/widgets/JSONFormWidget/fields/"
Length of output: 883
app/client/src/widgets/JSONFormWidget/fields/DateField.tsx (2)
23-23: LGTM: Clean import addition
The import follows the established pattern and is correctly placed with other hook imports.
137-137: Verify unmount validation behavior
The hook implementation aligns with the PR objective to fix validation triggers on child component updates.
✅ Verification successful
Hook implementation is correct and properly handles cleanup
The useUnmountFieldValidation hook is:
- Consistently used across all form field components
- Properly implements cleanup using useEffect's return function
- Correctly triggers field validation on unmount
- Well documented with the related issue reference
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of unmount validation across other field components
ast-grep --pattern 'useUnmountFieldValidation({ fieldName: $_ })'
# Check for any cleanup handling in the hook implementation
rg -l "useEffect.*return.*" --type ts | xargs rg "useUnmountFieldValidation"
Length of output: 958
Script:
#!/bin/bash
# Let's check the implementation of useUnmountFieldValidation hook
rg -A 10 "export.*useUnmountFieldValidation" --type ts
# Check if there are any cleanup effects in the hook
ast-grep --pattern 'useUnmountFieldValidation = ({ $_ }) => {
useEffect(() => {
$$$
return $$$
}, $$$)
}'
Length of output: 292
Script:
#!/bin/bash
# Let's check the actual content of the hook file
cat app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts
# Check if there are any imports that might give us more context
rg -B 3 "useUnmountFieldValidation" app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts
Length of output: 1271
app/client/src/widgets/JSONFormWidget/fields/MultiSelectField.tsx (2)
25-25: LGTM: Clean hook integration
The addition of useUnmountFieldValidation hook is well-placed and properly integrated with the field name.
Also applies to: 133-133
Line range hint 89-93: Verify validation behavior with hidden fields
The validation logic needs verification to ensure it correctly handles the scenario described in issue #28018 where fields are conditionally hidden.
Also applies to: 117-132
✅ Verification successful
Validation behavior with hidden fields is properly implemented
The MultiSelectField correctly handles validation for hidden fields through:
- Proper integration with
useRegisterFieldValidityhook to track field validity state - Validation state is only shown when field is dirty (
isValid={isDirty ? isValueValid : true}) - Field validity is cleaned up on unmount via
useUnmountFieldValidation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for visibility-related validation handling
rg -A 5 "isVisible.*isRequired"
# Check for other field components' validation implementation
ast-grep --pattern 'useRegisterFieldValidity({ $$$, fieldName: $_, fieldType: $_ })'
Length of output: 81282
Script:
#!/bin/bash
# Search for validation logic in MultiSelectField
ast-grep --pattern 'function MultiSelectField($$$) {
$$$
const isValid = $$$
$$$
}'
# Check for useRegisterFieldValidity usage in MultiSelectField
ast-grep --pattern 'useRegisterFieldValidity({
isValid: $$$,
fieldName: $$$,
fieldType: $$$
})'
# Look for visibility-related validation handling in JSONFormWidget
rg -A 5 "isVisible.*isValid" app/client/src/widgets/JSONFormWidget/
Length of output: 1170
Script:
#!/bin/bash
# Search for MultiSelectField implementation and validation logic
rg -A 10 "function MultiSelectField" app/client/src/widgets/JSONFormWidget/fields/MultiSelectField.tsx
# Check for visibility handling in field validation
rg -A 5 "isVisible.*fieldState" app/client/src/widgets/JSONFormWidget/fields/
# Look for validation state updates in JSONFormWidget
rg -A 5 "updateWidgetMetaProperty.*isValid" app/client/src/widgets/JSONFormWidget/
Length of output: 988
Script:
#!/bin/bash
# Check for field state validation implementation
rg -A 15 "isValueValid" app/client/src/widgets/JSONFormWidget/fields/MultiSelectField.tsx
# Look for visibility-related validation in widget helper
rg -A 10 "computeFormValidationState" app/client/src/widgets/JSONFormWidget/widget/helper.ts
# Search for field state handling in JSONFormWidget
rg -A 5 "fieldState.*isVisible" app/client/src/widgets/JSONFormWidget/
Length of output: 1751
app/client/src/widgets/JSONFormWidget/fields/BaseInputField.tsx (2)
40-40: LGTM: Clean integration of unmount validation
The addition of useUnmountFieldValidation hook aligns well with the PR's objective of fixing form validation triggers. This change ensures proper validation state management when fields are hidden or shown.
Also applies to: 244-244
Line range hint 232-241: Verify validation state synchronization
The component uses multiple validation mechanisms:
useRegisterFieldValidityfor ongoing validationuseUnmountFieldValidationfor unmount handling- Local
isValueValidstate
Let's ensure these states remain synchronized.
app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts
Show resolved
Hide resolved
…org#37128) ## Description <ins>Problem</ins> Form validation was not triggered when the child component was updated, resulting in inconsistent data consistency. <ins>Root cause</ins> The `Form` component in `app/client/src/widgets/JSONFormWidget/component/Form.tsx` did not include the `trigger` function from the `methods` object, preventing form validation from being triggered on child component updates. <ins>Solution</ins> This PR adds the `trigger` function from the `methods` object to the `Form` component, ensuring form validation is triggered correctly when the child component is updated. * Adds unit tests for `Form` component as well Fixes appsmithorg#28018 _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/11697880527> > Commit: 1c38b05 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11697880527&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JSONForm` > Spec: > <hr>Wed, 06 Nov 2024 06:06:41 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 form validation lifecycle management with the introduction of the `useUnmountFieldValidation` hook for better handling of field validation upon unmounting. - Improved testability of the form component through the inclusion of `data-testid` attributes for the submit and reset buttons. - **Bug Fixes** - Resolved edge cases in form validation, ensuring components function correctly with changing props and handle empty schemas gracefully. - **Tests** - Introduced a comprehensive suite of unit tests for the `Form` component, covering various scenarios including validation and visibility management. - Added tests for the new `useUnmountFieldValidation` hook to ensure correct validation behavior during unmounting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
Description
Problem
Form validation was not triggered when the child component was updated, resulting in inconsistent data consistency.
Root cause
The
Formcomponent inapp/client/src/widgets/JSONFormWidget/component/Form.tsxdid not include thetriggerfunction from themethodsobject, preventing form validation from being triggered on child component updates.Solution
This PR adds the
triggerfunction from themethodsobject to theFormcomponent, ensuring form validation is triggered correctly when the child component is updated.Formcomponent as wellFixes #28018
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/11697880527
Commit: 1c38b05
Cypress dashboard.
Tags:
@tag.JSONFormSpec:
Wed, 06 Nov 2024 06:06:41 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
useUnmountFieldValidationhook for better handling of field validation upon unmounting.data-testidattributes for the submit and reset buttons.Bug Fixes
Tests
Formcomponent, covering various scenarios including validation and visibility management.useUnmountFieldValidationhook to ensure correct validation behavior during unmounting.