Fixed : Disable Submit Button #9526
Conversation
WalkthroughThe pull request introduces updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/Form/Form.tsx (2)
1-1: Consider using deep-import alternativeThe current import
import isEqual from "lodash/isEqual"is good for tree-shaking, but consider using the more modern deep-import syntax:-import isEqual from "lodash/isEqual"; +import { isEqual } from "lodash/isEqual";
130-142: Optimize form change detection performanceThe current implementation creates a new form object on every change. Consider memoizing the comparison or using a more efficient change detection strategy.
onChange: ({ name, value }: FieldChangeEvent<T[keyof T]>) => { - const newForm = { - ...state.form, - [name]: value, - }; - setIsFormModified(!isEqual(newForm, formVals.current)); + const hasChanged = !isEqual(value, formVals.current[name]); + if (hasChanged !== isFormModified) { + setIsFormModified(hasChanged); + } dispatch({ type: "set_field", name, value, error: validate?.(value), }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(2 hunks)src/components/Form/Form.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Form/Form.tsx (1)
162-162: Verify submit button behavior in edge cases
The submit button disabled state implementation looks correct. However, please verify the behavior in the following scenarios:
- Form initialized with non-default values
- Form reset after submission
- Form modified and then restored to initial values
package.json
Outdated
| @@ -70,6 +70,7 @@ | |||
| "@sentry/browser": "^8.45.1", | |||
| "@tanstack/react-query": "^5.62.3", | |||
| "@tanstack/react-query-devtools": "^5.62.7", | |||
| "@types/lodash": "^4.17.13", | |||
There was a problem hiding this comment.
Fix incorrect @types/lodash version
The version ^4.17.13 for @types/lodash appears to be incorrect. The latest version series is 4.14.x, and version 4.17.x doesn't exist in the npm registry.
Update the version to the latest stable:
- "@types/lodash": "^4.17.13",
+ "@types/lodash": "^4.14.202",📝 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.
| "@types/lodash": "^4.17.13", | |
| "@types/lodash": "^4.14.202", |
src/components/Form/Form.tsx
Outdated
| dispatch({ type: "set_form", form: formVals.current }); | ||
| setIsFormModified(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for form submission failures
The form modified state is reset only on successful submission. Consider handling submission failures to maintain consistent state.
} else if (props.resetFormValsOnSubmit) {
dispatch({ type: "set_form", form: formVals.current });
setIsFormModified(false);
+} else {
+ // Keep isFormModified true if submission fails
+ setIsFormModified(true);
}Also applies to: 93-95
Jeffrin2005
force-pushed
the
Disable_Button
branch
from
8d3405f to
7d7b8f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Form/Form.tsx (1)
84-86:⚠️ Potential issueAdd error handling for form submission failures
The form modified state is reset only on successful submission and cancellation. Consider handling submission failures to maintain consistent state.
Also applies to: 91-93
🧹 Nitpick comments (2)
src/components/Form/Form.tsx (2)
128-140: Consider memoizing form state comparison for better performance.While the implementation is functionally correct, creating a new form object and performing deep comparison on every change might impact performance for large forms.
Consider this optimization:
onChange: ({ name, value }: FieldChangeEvent<T[keyof T]>) => { - const newForm = { - ...state.form, - [name]: value, - }; - setIsFormModified(!isEqual(newForm, formVals.current)); + const isModified = !isEqual(value, formVals.current[name]); + if (isModified !== isFormModified) { + setIsFormModified(isModified); + } dispatch({ type: "set_field", name, value, error: validate?.(value), }); },
160-160: Consider adding a tooltip to explain why the submit button is disabled.While the disabled state logic is correct, users might not understand why the button is disabled. Consider adding a tooltip to improve user experience.
<Submit data-testid="submit-button" type="submit" disabled={disabled || !isFormModified} + title={!isFormModified ? "Make changes to enable submission" : disabled ? "Form is currently disabled" : ""} label={props.submitLabel ?? "Submit"} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Form/Form.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Form/Form.tsx (1)
1-1: LGTM! Good choice of dependencies and state management.
The use of lodash/isEqual for deep comparison and the state initialization look correct. Using the specific lodash import is good for bundle size optimization.
Also applies to: 52-53
|
@rithviknishad Implemented without using lodash |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(3 hunks)src/components/Form/Form.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (3)
src/components/Form/Form.tsx (3)
2-2: LGTM: Proper internationalization implementation
The translation integration is well-implemented using react-i18next, following i18n best practices.
Also applies to: 47-47
Line range hint 93-102: Add error handling for form submission failures
The form modified state is reset only on successful submission. Consider handling submission failures to maintain consistent state.
154-169: Verify test coverage for button disabled state
The button disabled state implementation looks good, but let's ensure adequate test coverage exists.
src/components/Form/Form.tsx
Outdated
| const initial = { form: props.defaults, errors: {} }; | ||
| const [isLoading, setIsLoading] = useState(!!asyncGetDefaults); | ||
| const [state, dispatch] = useAutoSaveReducer<T>(formReducer, initial); | ||
| const formVals = useRef(props.defaults); | ||
| const [isModified, setIsModified] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing form state comparison
The current implementation has potential performance and edge case concerns:
- Using
JSON.stringifyfor comparison could be performance-intensive for large forms - Edge case: The form might still show as modified even when values are reset to initial state
Consider this optimization:
- const hasChanges =
- JSON.stringify(state.form) !== JSON.stringify(formVals.current);
- setIsModified(hasChanges);
+ import { isEqual } from 'lodash';
+ const hasChanges = !isEqual(state.form, formVals.current);
+ setIsModified(hasChanges);Also applies to: 63-67
src/components/Form/Form.tsx
Outdated
|
|
||
| useEffect(() => { | ||
| const hasChanges = | ||
| JSON.stringify(state.form) !== JSON.stringify(formVals.current); |
There was a problem hiding this comment.
Let's not do this, comparing like this isn't always reliable.
There was a problem hiding this comment.
like already said in previous review, use shadcn's form
src/components/Form/Form.tsx
Outdated
| @@ -141,17 +151,22 @@ const Form = <T extends FormDetails>({ | |||
| </div> | |||
| <div className="flex flex-col-reverse justify-end gap-3 sm:flex-row"> | |||
| {!hideCancelButton && ( | |||
| <Cancel | |||
| <ButtonV2 | |||
There was a problem hiding this comment.
Use shadcn buttons instead (src/components/ui).
package-lock.json
Outdated
There was a problem hiding this comment.
Shouldn't be in the commit.
How are you committing your merge changes? Cli or GUI 🤔
src/components/Form/Form.tsx
Outdated
| <Submit | ||
| data-testid="submit-button" | ||
| <ButtonV2 | ||
| variant="primary" |
There was a problem hiding this comment.
Make sure the data-testid is not being used in tests first, before removing them.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
src/components/ui/form.tsx (6)
1-2: Provide a more descriptive file header or component-level comment.Since this new file introduces a shared form component architecture, a concise top-level comment describing its purpose and usage would be helpful for future maintainers.
28-30: Avoid using empty object type assertions.
React.createContext<FormFieldContextValue>({} as FormFieldContextValue)can lead to runtime errors if used outside its intended scope. Consider initializing it with a safer default or anullcheck.
32-43: Maintain consistent naming for props in the controller.Currently, you spread all
ControllerPropsinto<Controller {...props} />. Consider destructuring essential props (likename,control) to make it explicit, which can improve readability and clarity.
130-146: Consistent text size in descriptive text.
text-[0.8rem]forFormDescriptionmay differ from typical app design guidelines. Confirm it matches the design system or consider making it themable.
147-169: Add i18n support for error messages.
FormMessagedisplayserror.messageor custom text. If your app uses i18n, ensure these strings are translated or externalized.
171-180: Review default exports vs. named exports for clarity.Exporting many named components is handy, but verify that import paths and usage across your codebase remain consistent. A single default export of an object containing all subcomponents might simplify imports for consumers.
src/components/Form/Form.tsx (7)
1-4: Document the file's purpose.Adding a top-level comment explaining this file’s relationship with
src/components/ui/form.tsx(e.g., form container vs. form fields) will help developers navigate the codebase.
50-54: Avoid naming collisions with "state" and "dispatch".Since you already use
useAutoSaveReducer, naming your React Hook Form’s reducer or local state differently can reduce confusion between the two sets of form-related data.
71-74: Ensure proper cleanup or error handling for asyncGetDefaults.If a user navigates away or unmounts the component before
asyncGetDefaults()completes, you might encounter a memory leak or an error. Consider canceling the request or checkingisMountedbefore callingdispatch.
103-104: Improve error logging.Consider capturing error details or custom fields (e.g., unique request ID) to help with debugging. Right now, only the default browser console logs are used.
113-113: Publicly track form cancellation reasons (optional).When
handleCancelis invoked, you simply reset the state. If analytics or other tracking is beneficial, consider logging why or how the user canceled.
120-127: Rename "form" prop to "onSubmit" for clarity.Your wrapper
<form onSubmit={methods.handleSubmit(...)}>is good, but ensure that fully describing the usage avoids confusion between theFormContainerfromui/form.tsxand standard HTML<form>semantics.
167-189: Confirm or unify button styling with design system.The
primaryandsecondaryvariants might differ from other styling patterns across the app. Ensure your design team or style guidelines are consistent, especially for critical actions like form submission or cancellation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(3 hunks)src/components/Form/Form.tsx(3 hunks)src/components/ui/form.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (7)
src/components/ui/form.tsx (5)
19-20: Confirm usage of FormProvider as a component alias.
Using const Form = FormProvider; is valid, but might be confusing. If you plan to rename it to Form, ensure that developers understand this is merely a re-export of FormProvider from React Hook Form, so there's no confusion with the new custom form components introduced here.
45-66: Validate existence of both contexts.
You check if (!fieldContext) {...}, but you also rely on itemContext. Consider a similar guard for itemContext to avoid potential runtime errors if used outside a <FormItem> scope.
76-88: Test interactions with nested FormItem contexts.
FormItem uses an auto-generated ID and provides it via context. It might be beneficial to ensure that multiple nested FormItem components behave as expected (no collision in IDs, etc.).
90-106: Gracefully handle missing error states in FormLabel.
When error is undefined, there is no issue, but if the label is used outside the intended context, a more explicit fallback or guard could preempt confusion.
107-129: Double-check aria attributes.
aria-describedby concatenates form description and message IDs conditionally. Ensure these elements are rendered consistently, preventing potential runtime issues or confusion for assistive technologies.
src/components/Form/Form.tsx (2)
57-64: Validate consistency with default form state.
useForm uses defaultValues: props.defaults, but you also have formVals.current referencing props.defaults. Confirm these remain in sync if the prop changes after component mount.
76-102: Maintain form modified state if submission fails.
Currently, successful submissions reset isEdited to false, but partially validated submissions or network failures keep it true. This is generally correct, but confirm whether you need additional logic to indicate partial success or other states.
| <Provider | ||
| value={(name: keyof T, validate?: FieldValidator<T[keyof T]>) => { | ||
| return { | ||
| name, | ||
| value, | ||
| error: validate?.(value), | ||
| }), | ||
| value: state.form[name], | ||
| error: state.errors[name], | ||
| disabled, | ||
| }; | ||
| }} | ||
| > | ||
| <div className="my-6"> | ||
| <Consumer>{props.children}</Consumer> | ||
| </div> | ||
| <div className="flex flex-col-reverse justify-end gap-3 sm:flex-row"> | ||
| {!hideCancelButton && ( | ||
| <Cancel | ||
| onClick={handleCancel} | ||
| label={props.cancelLabel ?? "Cancel"} | ||
| /> | ||
| )} | ||
| <Submit | ||
| data-testid="submit-button" | ||
| type="submit" | ||
| disabled={disabled} | ||
| label={props.submitLabel ?? "Submit"} | ||
| /> | ||
| </div> | ||
| </Provider> | ||
| </DraftSection> | ||
| </form> | ||
| id: name, | ||
| onChange: ({ name, value }: FieldChangeEvent<T[keyof T]>) => { | ||
| dispatch({ | ||
| type: "set_field", | ||
| name, | ||
| value, | ||
| error: validate?.(value), | ||
| }); | ||
| setIsEdited(true); | ||
| }, | ||
| value: state.form[name], | ||
| error: state.errors[name], | ||
| disabled, | ||
| }; | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check performance impact of dispatch calls.
onChange triggers dispatch({ type: "set_field", ... }) on every keystroke. While likely fine for small forms, for large forms this may have potential performance overhead.
|
@rithviknishad Used Shadcn Form and added form.tsx |
Proposed Changes
S_Button.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit