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.

Sign up for GitHub

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 9 commits into
base: main
Choose a base branch
from
+21 −22
Open

(fix) O3-4274: Use useRef instead of useState for useVisitFormCallbacks #2157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ee5f077
O3-4274: Refactor useVisitFormCallbacks to use useRef instead of useS…
Bharath-K-Shetty Dec 13, 2024
e917561
Merge branch 'main' into fix/O3-4274
Bharath-K-Shetty Dec 15, 2024
6905952
Update packages/esm-patient-chart-app/src/visit/visit-form/visit-form…
Bharath-K-Shetty Dec 16, 2024
5db6c90
Update packages/esm-patient-chart-app/src/visit/visit-form/visit-form…
Bharath-K-Shetty Dec 16, 2024
783de5b
Merge branch 'main' into fix/O3-4274
Bharath-K-Shetty Dec 16, 2024
7945ebd
O3-4274: Refactor useVisitFormCallbacks to use useRef instead of useS…
Bharath-K-Shetty Dec 13, 2024
4578546
Removed white spaces and unwanted comments
Bharath-K-Shetty Dec 17, 2024
5740c48
Merge remote-tracking branch 'origin/fix/O3-4274' into fix/O3-4274
Bharath-K-Shetty Dec 20, 2024
c70c01d
Merge branch 'main' into fix/O3-4274
Bharath-K-Shetty Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
const { visitAttributeTypes } = useVisitAttributeTypes();
const [extraVisitInfo, setExtraVisitInfo] = useState(null);

const [visitFormCallbacks, setVisitFormCallbacks] = useVisitFormCallbacks();
const visitFormCallbacksRef = useVisitFormCallbacks();
const displayVisitStopDateTimeFields = useMemo(
() => Boolean(visitToEdit?.uuid || showVisitEndDateTimeFields),
[visitToEdit?.uuid, showVisitEndDateTimeFields],
Expand Down Expand Up @@ -243,7 +243,6 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
reset,
} = methods;

// default values are cached so form needs to be reset when they change (e.g. when default visit location finishes loading)
Bharath-K-Shetty marked this conversation as resolved.
Show resolved Hide resolved
useEffect(() => {
reset(defaultValues);
}, [defaultValues, reset]);
Expand Down Expand Up @@ -531,7 +530,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
},
);

const onVisitCreatedOrUpdatedRequests = [...visitFormCallbacks.values()].map((callbacks) =>
const onVisitCreatedOrUpdatedRequests = [...visitFormCallbacksRef.current.values()].map((callbacks) =>
callbacks.onVisitCreatedOrUpdated(visit),
);

Expand Down Expand Up @@ -580,6 +579,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 •
edited
Loading

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!

[
closeWorkspace,
config.offlineVisitTypeUuid,
Expand All @@ -591,7 +591,6 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
mutateCurrentVisit,
mutateVisits,
mutateInfiniteVisits,
visitFormCallbacks,
patientUuid,
t,
validateVisitStartStopDatetime,
Expand Down Expand Up @@ -660,10 +659,10 @@ 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.

patientUuid={patientUuid}
visitFormOpenedFrom={openedFrom}
setVisitFormCallbacks={setVisitFormCallbacks}
visitFormCallbacksRef={visitFormCallbacksRef}
/>
</div>
</section>
Expand Down Expand Up @@ -732,7 +731,6 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
{contentSwitcherIndex === 1 && <BaseVisitType visitTypes={allVisitTypes} />}
</>
) : (
// Defaults to showing all possible visit types if recommended visits are not enabled
Bharath-K-Shetty marked this conversation as resolved.
Show resolved Hide resolved
<BaseVisitType visitTypes={allVisitTypes} />
)}
</div>
Expand Down Expand Up @@ -774,7 +772,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
name="visit-form-bottom-slot"
patientUuid={patientUuid}
visitFormOpenedFrom={openedFrom}
setVisitFormCallbacks={setVisitFormCallbacks}
visitFormCallbacksRef={visitFormCallbacksRef}
/>
</div>
</section>
Expand Down Expand Up @@ -813,12 +811,12 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
</FormProvider>
);
};

interface VisitFormExtensionSlotProps {
name: string;
patientUuid: string;
visitFormOpenedFrom: string;
setVisitFormCallbacks: React.Dispatch<React.SetStateAction<Map<string, VisitFormCallbacks>>>;

visitFormCallbacksRef: React.MutableRefObject<Map<string, VisitFormCallbacks>>;
}

type VisitFormExtensionState = {
Expand All @@ -838,22 +836,21 @@ type VisitFormExtensionState = {
};

const VisitFormExtensionSlot: React.FC<VisitFormExtensionSlotProps> = React.memo(
({ name, patientUuid, visitFormOpenedFrom, setVisitFormCallbacks }) => {
({ name, patientUuid, visitFormOpenedFrom, visitFormCallbacksRef }) => {
const config = useConfig<ChartConfig>();

return (
<ExtensionSlot name={name}>
{(extension: AssignedExtension) => {
const state: VisitFormExtensionState = {
const state = {
patientUuid,
setVisitFormCallbacks: (callbacks) => {
setVisitFormCallbacks((old) => {
return new Map(old).set(extension.id, callbacks);
});
setVisitFormCallbacks: (callbacks: VisitFormCallbacks) => {
visitFormCallbacksRef.current.set(extension.id, callbacks);
},
visitFormOpenedFrom,
patientChartConfig: config,
};

return <Extension state={state} />;
}}
</ExtensionSlot>
Expand Down
5 changes: 3 additions & 2 deletions packages/esm-patient-chart-app/src/visit/visit-form/visit-form.resource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { openmrsFetch, restBaseUrl, useConnectivity, useVisitTypes, type Visit } from '@openmrs/esm-framework';
import { type amPm } from '@openmrs/esm-patient-common-lib';
import { useOfflineVisitType } from '../hooks/useOfflineVisitType';
import { useState } from 'react';
import { useRef } from 'react';

export type VisitFormData = {
visitStartDate: Date;
Expand All @@ -28,12 +28,13 @@ export function useConditionalVisitTypes() {

return visitTypesHook();
}

export interface VisitFormCallbacks {
onVisitCreatedOrUpdated: (visit: Visit) => Promise<any>;
}

export function useVisitFormCallbacks() {
return useState<Map<string, VisitFormCallbacks>>(new Map());
return useRef<Map<string, VisitFormCallbacks>>(new Map());
}

export function createVisitAttribute(visitUuid: string, attributeType: string, value: string) {
Expand Down
9 changes: 5 additions & 4 deletions packages/esm-patient-chart-app/src/visit/visit-form/visit-form.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,11 @@ const mockUseEmrConfiguration = jest.mocked(useEmrConfiguration);

// from ./visit-form.resource
const mockOnVisitCreatedOrUpdatedCallback = jest.fn();
jest.mocked(useVisitFormCallbacks).mockReturnValue([
new Map([['test-extension-id', { onVisitCreatedOrUpdated: mockOnVisitCreatedOrUpdatedCallback }]]), // visitFormCallbacks
jest.fn(), // setVisitFormCallbacks
]);

jest.mocked(useVisitFormCallbacks).mockReturnValue({
current: new Map([['test-extension-id', { onVisitCreatedOrUpdated: mockOnVisitCreatedOrUpdatedCallback }]]),
});

const mockCreateVisitAttribute = jest.mocked(createVisitAttribute).mockResolvedValue({} as unknown as FetchResponse);
const mockUpdateVisitAttribute = jest.mocked(updateVisitAttribute).mockResolvedValue({} as unknown as FetchResponse);
const mockDeleteVisitAttribute = jest.mocked(deleteVisitAttribute).mockResolvedValue({} as unknown as FetchResponse);
Expand Down
Loading