(feat) O3-4363 - Add Visit context header to Order, Visit Note, and F… #2222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chibongho
commented
Jan 30, 2025
| import styles from './start-visit-dialog.scss'; | ||
|
|
||
| interface StartVisitDialogProps { | ||
| patientUuid: string; | ||
| closeModal: () => void; | ||
| visitType: string; |
There was a problem hiding this comment.
Removing this as it doesn't seem to be used. This was brought up before:
https://github.com/openmrs/openmrs-esm-patient-chart/pull/640/files#diff-7e621046e4d227da306880633891cb2d35e4f168f95967ff73f00aa973bc6d06R10
|
Size Change: -264 kB (-1.59%) Total Size: 16.4 MB
ℹ️ View Unchanged
|
|
Hi @chibongho! Thanks for your work on this. I have a suggestion to re-model the entire system. Could we integrate the header context into the workspace system itself? When calling Alternatively, we could create an extension slot based on the property mentioned above (rendered by the workspace system) and attach the RDE visit context extension (from the patient chart). However, I believe it would be better to handle all RDE-related tasks from a separate app (@openmrs/esm-patient-rde-app), as these changes would not only affect the patient chart but also the ward patient workspace. In summary, we have two main points to consider: rendering the extension slot (workspace renderer) and the extension (from the patient chart or the patient RDE app, whichever is more suitable). Please let me know your thoughts on this. Thanks! |
vasharma05
reviewed
Feb 6, 2025
There was a problem hiding this comment.
I am seeing a lot of duplicated work which can be done in the useVisit hook.
Alongside, we should work on the workspace system to render an extension slot if we pass the slot names. Hence we won't have to duplicate our effort in every patient workspace and directly use the workspace system to do out work.
A sample example will be:
import {launchWorkspace} from '@openmrs/esm-framework';
launchPatientWorkspace = (workspaceName, additionalProps) => launchPatientWorkspace(workspaceName, {...additionalProps, renderExtensionSlots: ['visit-context-header-slot']}
| return ( | ||
| <> | ||
| {isRdeEnabled && <ExtensionSlot name="visit-context-header-slot" state={{ patientUuid }} />} |
There was a problem hiding this comment.
This is what I am talking about, why should we go manually into every patient chart workspace and render the extension slot, when we can use the workspace system to do the same?
| return ( | ||
| <div className={styles.container}> | ||
| {isRdeEnabled && <ExtensionSlot name="visit-context-header-slot" state={{ patientUuid }} />} |
| type OpenmrsResource, | ||
| type Privilege, | ||
| type Visit, | ||
| } from '@openmrs/esm-framework'; | ||
| import useSWR, { mutate } from 'swr'; | ||
| import { type ChartConfig } from '../../config-schema'; | ||
|
|
||
| export function useInfiniteVisits(patientUuid: string) { |
There was a problem hiding this comment.
Why re-working on what is already implemented in the useVisit hook
There was a problem hiding this comment.
useVisit() makes a request to fetch active visit(s) for the patient, and another request to fetch the visit currently in the visit context. This function is different; its using useSWRInfinite() to get all the visits of the patient.
| import { getVisitStore, useStoreWithActions, type Visit } from '@openmrs/esm-framework'; | ||
|
|
||
| export function useVisitContextStore() { | ||
| return useStoreWithActions(getVisitStore(), { |
There was a problem hiding this comment.
Shouldn't this be moved into the react utils? Also, isn't this indirectly useVisit?
There was a problem hiding this comment.
It's not useVisit() in that it doesn't make requests to fetch visits, but it's not a bad idea to merge them into the same hook.
And yes, it should be moved to react-utils. I plan to do it in a separate PR. I also have a TODO in VisitContextHeader about moving a useEffect hook into useVisit (in react-utils) as well.
vasharma05
requested changes
Feb 6, 2025
There was a problem hiding this comment.
I’ve noticed a lot of repetitive work that could be handled using the useVisit hook. Additionally, we should improve the workspace system to render an extension slot when we provide the slot names. This way, we won’t have to duplicate our efforts in each patient workspace and can directly use the workspace system to accomplish our tasks.
Here’s a sample example:
import {launchWorkspace} from '@openmrs/esm-framework';
launchPatientWorkspace = (workspaceName, additionalProps) => launchPatientWorkspace(workspaceName, {...additionalProps, renderExtensionSlots: ['visit-context-header-slot']}
Not sure what's duplicated. Can you elaborate?
It only takes 1 extra of of code to render a slot, across 3 different patient chart workspaces. Adding this feature will add more complexity. It also assumes the slot will be placed at the top, which is less flexible than just adding an slot within the workspace wherever we want it. |
mogoodrich
reviewed
Feb 10, 2025
There was a problem hiding this comment.
Thanks @chibongho , generally looks good, though admittedly this was a lot to review and not something I'm able to dive super deeply into.
I asked some questions, but mainly to better understand naming conventions.
At the end of the day, it's probably best to get this in sooner rather than later and start testing/playing around with it, but it would be to good to confirm with @ibacher and @denniskigen that there aren'y any releases imminent, because this definitely risks introducing instabiliity.
| @@ -10,6 +10,9 @@ interface RetrospectiveVisitLabelProps { | |||
|
|
|||
| const RetrospectiveVisitLabel: React.FC<RetrospectiveVisitLabelProps> = ({ currentVisit }) => { | |||
| const { t } = useTranslation(); | |||
| if (!currentVisit) { | |||
| return <></>; | |||
| } | |||
There was a problem hiding this comment.
We only render the retrospective entry toggle if it's not the current visit? (I'm probably just not understanding this correctly and it's legacy functionality anyway?)
There was a problem hiding this comment.
ah, current visit means the visit in the current context, not the active visit?
There was a problem hiding this comment.
Yes, the "current visit" is the visit in context. It's a term that's from prior RDE work. I added this if statement to prevent an NPE. This component is actually going away soon anyway, as we have a ticket to remove the modified visit header in the top nav.
| const { t } = useTranslation(); | ||
| const { currentVisit, isLoading } = useVisit(patientUuid); | ||
| const { manuallySetVisitUuid, setVisitContext } = useVisitContextStore(); | ||
| const isActiveVisit = !Boolean(currentVisit && currentVisit.stopDatetime); |
There was a problem hiding this comment.
It occurs to me that there's likely to be confusion where people think current visit = active visit (see my confusion above) though a better name hasn't come to mind for me yet.
| // This hook is needed because the `useVisit` hook sometimes | ||
| // returns a currentVisit that isn't actually the one in the visit context. | ||
| // TODO: move this into the useVisit hook | ||
| useEffect(() => { |
There was a problem hiding this comment.
Okay... :) Hopefully this will make more sense to me when we get to the the visit context hook... "manuallySetVisit" isn't super intuitive...
There was a problem hiding this comment.
Okay, I still don't understand after reviewing the code... what is the case when a currentVisit isn't actually on the one in the visit context?
There was a problem hiding this comment.
currentVisit and the visit in context really should be the exact same thing. This code here is to get around a bug in useVisit where it defaults the currentVisit to the activeVisit if the visit store's manuallySetVisitUuid is null, but it doesn't update the store's manuallySetVisitUuid value accordingly.
| return ( | ||
| <> | ||
| {isActive | ||
| ? t('currentActiveVisit', 'Current active visit') |
There was a problem hiding this comment.
I guess this is end-user-facing, but this does continue to propagate the confusion about what "current' means.
There was a problem hiding this comment.
Yeah, this is from the mockup and I think the wording actually makes pretty good sense to the end user. I propose we keep this as is, but rename the currentVisit in the code base.
| return useStoreWithActions(getVisitStore(), { | ||
| setVisitContext(_, newSelectedVisit: Visit) { | ||
| return { | ||
| manuallySetVisitUuid: newSelectedVisit.uuid, |
There was a problem hiding this comment.
Can you explain how this is different from other ways the visit context visit is set?
There was a problem hiding this comment.
There is already a setCurrentVisit() function from core to directly set the store's current visit. However, that function does not cause react updates. This hook is made to trigger react updates on setting the visit context.
Yeah, the naming convention right now is confusing. I tried to keep backwards compatibility by not modifying the hooks / stores (and their names) from previous RDE work. Looking at the code more, I think it's probably safe to change them, as the prior RDE work seemed incomplete, and is likely not used by anyone. (@ibacher and @denniskigen does that sound fair?) So we can rename |
This sounds great, thanks @chibongho ! |
mogoodrich
approved these changes
Feb 12, 2025
@mogoodrich OK, I went down the rabbit hole of renaming things and it's more intrusive than I expected. I did push my draft changes to my fork here without any testing. It's straightforward enough, but it's big enough that I don't think it should be part of this PR. |
Fine with me, thanks @chibongho ! |
|
So here's where I am at with this. Currently, we have a It seems like the concept of a "current visit" should only be relevant within the patient chart. (I.e. there should not be anything that sets / gets the "current visit" outside the patient chart.) As such, I think it makes more sense for the visit store to be defined in esm-patient-chart, rather than esm-core. Similarly, the So what should we do about this "current visit" stuff, along with things in this PR that uses it more? If we want to preverse backends compatiblity, then I think we should stick with the approach of this PR (leaving useVisit() in core untouched and add more visit store functionality in patient chart). If we feel comfortable with breaking backward compatiblity, I think we can change esm-core's |
This generally makes sense to me @chibongho though definitely want to hear what others think. My gut would be to go with the option that breaks backwards compatibility (but which seems like the best long-term solution) but of course easy for me to say since we don't currently use this functionality at PIH... :) |
So, that's not entirely true. For example, if we want to allow the retrospective entry of lab results (we do) in the Lab app, we seem to need the same concept or, at the very least, we need a way to refer to the concept of "the visit being edited". (I think similar considerations apply to dispensing and potentially billing). To that end, I think the issue is actually the Maybe another slight concept breaking thing I want to plug-in here, though, is that the Chart is intended as a view for clinicians. It's not necessarily the only app or place to collect data for an individual patient and its at those various other points that we'll always need some kind of retrospective entry That's probably a more breaking change, but, fundamentally, I'd imagine we'd have hooks like:
|
|
So, also fair. Not to go down the rabbit-hole, but does our UI currently support this pattern? Another thought that may or may not be helpful: for the "visit in context", is the patient even relevant? ie, there really should only be one visit in context at any time right (not one-per-patient)? |
No, and we may need to adapt something like the visit header pattern elsewhere.
Definitely a rabbit-hole... Short answer is "no". But it's more convenient to have a single hook that can be called to get the "current" visit and if no visit has been selected, the current visit will be the active visit, for which we do need the patient UUID. So, if we had an app that supported the workflow Select Patient -> Select Visit -> Do Stuff, we wouldn't need a |
Taking with @chibongho today I think I'm more convinced that the "current" (maybe "selected" is a better term?) visit should be local to the patient chart, if I'm understanding correctly. I would think that if the user jumped out of the patient chart entirely and, say, entered the lab app, they wouldn't expect (nor should) be in the same visit context? Is that what we are talking about? It's possible that other apps may want to follow the same pattern to create a visit context, but they shouldn't be sharing a single one? |
|
I think it makes sense for apps other than patient chart to have "current visit" (or "selected visit", or "visit in context") for a patient and display it in the visit context header in workspaces / forms when adding data that supports RDE. But how sticky / persistent should the "current visit" be? I think we want some persistence. (When we need to enter 10 RDE visit notes for patient A visit X, it'd be nice to not have to re-select X every time we open the visit form workspace. Ditto when filling out 10 RDE lab results.) However, right now, the current visit is stored in a global Visit store, which means the "current visit" selected in the patient chart is also the same "current visit" when we navigate out of the patient chart into the labs app. This results in the following behaviors.
The above could be unexpected / confusing until the user understands the "current visit" persistence. I think it would be less confusing if we have the visit store stickiness be on a per-app basis instead. Somewhat related, the visit store is backed by localStorage right now. Should that be the case? It implemented manually (instead of using the built-in zustand localStorage support) and adds some complexity. If we want to keep it, and we want the visit store stickiness to be per-app, then we need to namespace the values within the visit store by app name. (Also, whatever we decided for "current visit" persistence, can we scope it as separate ticket / PR so we can unblock this one?) |
+1 to per app. In both the above use cases, I would expect the user to have to reselect the "current visit" for the patient. Although there is some utility, of course, I really feel like it would be more confusing than helpful. Tangentially (and it also occurs to me that this is relevant for my "one-one-current-visit-so-patient-isnt-relevant-comment") regardless of what we do we should be very careful about getting into a state where the "current" patient and "current" visit don't coincide, and risk displaying health information for one patient and attribute to another. @chibongho @ibacher @mseaton |
Yeah, sorry. I might've added to the confusion here. Current visit context should be local to the chart, I just wanted to be clear that the concept is itself is applicable to non-chart cases, but probably differently (in the chart, the "current" visit is state shared across a large number of extensions and whatnot; outside of the chart, the "current" visit may just be context local to form entry).
Ugh... This is tricky. On the one hand, I don't think localStorage is the right place to store this. On the other hand, if I opened a second tab in the patient chart of the same patient, I would probably expect to be in the same visit context as I was before. Maybe we need to consider a URL change when the user selects a visit context so that the URL is now
Yeah, and I'd actually be happier if we decide to retain a global |
|
Storing in URL could be tricky because:
There are ways around them, but I think there could be weird edge cases. If we want to have the current visit persist across tabs, I think keeping it in localStorage is easiest to implement.
I think it does that already (you won't get the current visit back if it doesn't belong to the input patient). Although I also want the hook to have a side effect of setting the current visit to the active visit if it's not defined / belong to a different patient, something it doesn't do right now. I think it makes sense to have a global hook to get the current visit that works easily with whatever per-app "current visit" storage we decide on, so we can use it in places outside the patient chart. I think it's enough of a deviation of the current |
…ered from patient chart
I think we don't avoid weird edge-cases by using localStorage, though. My worry is that with that, the context can change without a visual prompt to the user of any sort. But I also think that clicking the back button and not being back in the context I was in before (since the visit didn't change in localStorage) is a worse sort of error along those lines. But also, what happens here if I open two tabs, both in the chart and then in one tab, I navigate to a different app?
Good point. It does.
Is there a reason we wouldn't want that behaviour elsewhere? The problem here isn't that this is some new behaviour, it's that the |
3 tasks
denniskigen
reviewed
Feb 25, 2025
| const isActive = !Boolean(visit.stopDatetime); | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
Can we ensure that the content of this fragment is centered both vertically and horizontally with rules such as:
display: flex;
align-items: center;
justify-items: center;| if (isLoading) { | ||
| return ( | ||
| <div className={styles.visitContextHeader}> | ||
| <Loading small />; |
There was a problem hiding this comment.
Suggested change
| <Loading small />; | |
| <Loading small /> |
| @@ -10,6 +10,9 @@ interface RetrospectiveVisitLabelProps { | |||
|
|
|||
| const RetrospectiveVisitLabel: React.FC<RetrospectiveVisitLabelProps> = ({ currentVisit }) => { | |||
| const { t } = useTranslation(); | |||
| if (!currentVisit) { | |||
| return <></>; | |||
| } | |||
| return ( | |||
| <Toggletip align="bottom"> | |||
| <ToggletipButton label={t('retrospectiveEntry', 'Retrospective Entry')}> | |||
There was a problem hiding this comment.
Minor, but we probably don't need the optional chaining checks on the currentVisit object in this return block because we're sure it exists, right?
There was a problem hiding this comment.
I did in fact get an NPE without it. I suspect it's a double rendering issue where the currentVisit is null on the first render.
A bit moot why, as this component is going away in a separate PR.
| <span className={styles.visitLocation}>{visit.location.display}</span> | ||
| </> | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Can we prefer the early return if there's no visit?
if (!visit) {
return <></>;
}
const isActive = !Boolean(visit.stopDatetime);
return (
<>
{isActive
? t('currentActiveVisit', 'Current active visit')
: t('fromDateToDate', '{{fromDate}} - {{toDate}}', {
fromDate: formatDate(parseDate(visit.startDatetime), { time: false }),
toDate: formatDate(parseDate(visit.stopDatetime), { time: false }),
})}
<span className={styles.separator}>·</span>
<Building />
<span className={styles.visitLocation}>{visit.location.display}</span>
</>
);Also, isn't it preferable to return null instead of an empty fragment?
| @@ -0,0 +1,124 @@ | |||
| import { Button, InlineLoading, ModalBody, ModalFooter, ModalHeader, RadioButton } from '@carbon/react'; | |||
| import { ErrorState, type Visit } from '@openmrs/esm-framework'; | |||
| import { launchPatientWorkspace } from '@openmrs/esm-patient-common-lib/src'; | |||
There was a problem hiding this comment.
Suggested change
| import { launchPatientWorkspace } from '@openmrs/esm-patient-common-lib/src'; | |
| import { launchPatientWorkspace } from '@openmrs/esm-patient-common-lib'; |
| <Button | ||
| disabled={selectedVisit == null || isLoading} | ||
| onClick={() => { | ||
| setVisitContext(visits.find((v) => v.uuid == selectedVisit)); |
There was a problem hiding this comment.
Suggested change
| setVisitContext(visits.find((v) => v.uuid == selectedVisit)); | |
| setVisitContext(visits?.find((v) => v.uuid === selectedVisit)); |
| const { visits, isLoading, error } = useInfiniteVisits(patientUuid); | ||
| const { patientUuid: selectedVisitPatientUuid, manuallySetVisitUuid, setVisitContext } = useVisitContextStore(); | ||
| const [selectedVisit, setSelectedVisit] = useState<string>( | ||
| selectedVisitPatientUuid == patientUuid ? manuallySetVisitUuid : null, |
There was a problem hiding this comment.
Suggested change
| selectedVisitPatientUuid == patientUuid ? manuallySetVisitUuid : null, | |
| selectedVisitPatientUuid === patientUuid ? manuallySetVisitUuid : null, |
| @use '@carbon/layout'; | ||
| @use '@openmrs/esm-styleguide/src/vars' as *; | ||
|
|
||
| .visitstList { |
There was a problem hiding this comment.
Suggested change
| .visitstList { | |
| .visitsList { |
Typo?
denniskigen
reviewed
Feb 25, 2025
| {t('change', 'Change')} | ||
| </Button> | ||
| </div> | ||
| <div className={styles.visitInfo}> |
There was a problem hiding this comment.
Ditto here. Can we ensure that the content of this fragment is centered both vertically and horizontally with rules such as:
display: flex;
align-items: center;
justify-items: center;
How about sessionStorage instead? If you open the patient chart in 2 tabs, each one will have its own visit context, and we make it the only way to change the context is by the user actually doing it via UI within that tab.
Yeah, I think we'll want that behavior everywhere. So I guess just change |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…orms workspaces
Requirements
Summary
This PR adds RDE features: a new visit context header to the patient chart workspaces (order / visit note / clinical forms), and a new visit switcher modal accessible from the visit context header.
The concept of a "Visit Context" when viewing a patient chart is not new; previous RDE work has that when the
rdefeature flag is turned on). Screenshots from before this change:Instead of rendering in the top nav, the visit context header is rendered at the top of the workspace with this change, See the mockups here.
The new changes are behind the existing
rdefeature flag, so they should not be noticeable for most people.Notes:
encounterDatetime), but it doesn't work. See: https://openmrs.atlassian.net/browse/O3-4420encounterDateTimeset to the visit's start time. However the order'sdateActivatedfield is set tonow. Is that expected?now, and is tied to the active visit, instead of the past visit in the visit context.Screenshots
The visit context header in a workspace:
The modal that opens when clicking on "change" in the visit context header. The same modal also shows when the user tries to open the order / visit notes / forms workspace without a visit (active or past) in context.
Related Issue
https://openmrs.atlassian.net/browse/O3-4363
Other