Skip to content
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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Bharath-K-Shetty
Copy link

@Bharath-K-Shetty Bharath-K-Shetty commented Dec 14, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

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

@brandones brandones changed the title (Refactor) O3-4274: Use useRef instead of useState for useVisitFormCallbacks (fix) O3-4274: Use useRef instead of useState for useVisitFormCallbacks Dec 15, 2024
Copy link
Contributor

@brandones brandones left a 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
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Author

@Bharath-K-Shetty Bharath-K-Shetty Jan 3, 2025

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!

Copy link
Contributor

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.

@@ -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"
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

@brandones
Copy link
Contributor

Please add a video demonstrating that everything still behaves as expected.

@Bharath-K-Shetty
Copy link
Author

Hey @brandones,I’ve made the changes as requested and updated the PR. Let me know if any further changes are needed.

Thank you

@@ -580,6 +584,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
return;
}
},
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

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

@@ -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"
Copy link
Contributor

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.

@brandones
Copy link
Contributor

Also, you can use the "Request re-review" button instead of pinging people on Slack.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants