-
Notifications
You must be signed in to change notification settings - Fork 246
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
(fix) O3-4274: Use useRef instead of useState for useVisitFormCallbacks #2157
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for this change, @Bharath-K-Shetty . Please clean up your code before submitting it. Remove the extra whitespace and strange comments.
@@ -580,6 +584,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
return; | |||
} | |||
}, | |||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
I think it is preferable simply to include the ref in the dependency array, even though it will not change.
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.
@Bharath-K-Shetty you actually need to respond to reviewer feedback, you can't just hit the "resolve conversation" button to make things go away
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.
Thank you for the feedback, @brandones ! I understand your concern and have updated the code to include the visitFormCallbacksRef.current in the dependency array of the useEffect hook, even though it is unlikely to change.
But React and ESLint warn against using mutable refs (MutableRefObject) in dependency arrays. This is because changes to .current do not trigger re-renders or effect executions, which can lead to misleading behavior or unnecessary lint suppressions.
For this reason, I’ve previously excluded visitFormCallbacksRef.current from the dependency array
I appreciate your thorough review, and I’ll make sure to properly address all feedback moving forward instead of prematurely resolving conversations. Please let me know if there’s anything else you'd like me to adjust.
Please let me know if you feel strongly about including it, and we can discuss further!
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.
Ok if adding the ref in the dependency array causes a different eslint error that we would need to suppress anyway, then yes this is fine. Very strange though, that there is no dependency array that will not produce an ESLint error when a ref is used in the useEffect.
Do not click the "resolve conversation" button unless you yourself opened the conversation, otherwise the person you are talking with might not see your response. I opened this conversation with a request, so it's resolved either when I click the "resolved" button or when I respond with something that suggests that I've acknowledged what you've written and think my question is resolved. Hope this is clear and helpful for your future GitHub use.
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
@@ -660,10 +665,12 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
<div className={styles.sectionTitle}></div> | |||
<div className={styles.sectionField}> | |||
<VisitFormExtensionSlot | |||
name="visit-form-top-slot" | |||
name="visit-form-bottom-slot" |
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.
Why did you do this?
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.
Please don't resolve conversations without addressing the question being asked.
Why did you change the extension slot name?
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.
I changed the slot name from visit-form-top-slot to visit-form-bottom-slot because the original name (visit-form-top-slot) was causing a 404 error, indicating that the server could not find the requested resource. After troubleshooting, I updated the name to visit-form-bottom-slot, which resolved the issue and allowed the form to function as intended. This suggests that the visit-form-bottom-slot aligns with the correct configuration or extension setup expected by the system.
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.
That's very surprising, and a separate bug from this ticket. I'm fine with you addressing it in this PR, but you need to document the bug. Please provide screenshots showing the error.
Whatever the problem is, this is pretty certainly not the solution, since there is already a visit-form-bottom-slot
in existence, on line 774 of this file. If you're not sure how the extension system works, read the docs.
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
Please add a video demonstrating that everything still behaves as expected. |
….component.tsx Co-authored-by: Brandon Istenes <brandonesbox@gmail.com>
….component.tsx Co-authored-by: Brandon Istenes <brandonesbox@gmail.com>
783de5b
to
7945ebd
Compare
Hey @brandones,I’ve made the changes as requested and updated the PR. Let me know if any further changes are needed. Thank you |
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
@@ -580,6 +584,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
return; | |||
} | |||
}, | |||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
@Bharath-K-Shetty you actually need to respond to reviewer feedback, you can't just hit the "resolve conversation" button to make things go away
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
@@ -660,10 +665,12 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
<div className={styles.sectionTitle}></div> | |||
<div className={styles.sectionField}> | |||
<VisitFormExtensionSlot | |||
name="visit-form-top-slot" | |||
name="visit-form-bottom-slot" |
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.
That's very surprising, and a separate bug from this ticket. I'm fine with you addressing it in this PR, but you need to document the bug. Please provide screenshots showing the error.
Whatever the problem is, this is pretty certainly not the solution, since there is already a visit-form-bottom-slot
in existence, on line 774 of this file. If you're not sure how the extension system works, read the docs.
Requirements
Summary
This PR refactors the useVisitFormCallbacks hook by replacing the usage of useState with useRef. The primary goal of this change is to improve performance by avoiding unnecessary re-renders triggered by state updates. useRef is used to persist values across renders without causing re-renders, which is particularly beneficial in the case of form callbacks where state persistence is needed but without affecting component lifecycle.
This change ensures that the form callbacks are maintained correctly across renders, while reducing the overhead associated with state management in React.
Jira Issue: O3-4274
Screenshots
O3-4274.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-4274
Other