Repository navigation
Conversation
chibongho
left a comment
chibongho
left a comment
There was a problem hiding this comment.
Most of my comments are performance-related, so fine as a first-pass and fix later. However, I think the PatientActivity table can miss displaying data if the patient has too many encounters.
| * dropdown never offers a type that would return nothing. Read unfiltered, so choosing a type | ||
| * does not narrow the choices left. | ||
| */ | ||
| export function usePatientEncounterTypes(patient: AuditPatient | undefined, includeDeleted: boolean) { |
There was a problem hiding this comment.
Fine for now, but we should consider having a server-side endpoint for this instead of figuring that out client-side.
| * summarised per user and listed in order. | ||
| * | ||
| * The encounter rows carry their own `auditInfo`, but the observations do not come with them — | ||
| * they have to be read per encounter, since that is the only obs search that honours |
There was a problem hiding this comment.
I don't think this is completely right. Obs has auditInfo, but I think it's true that calling the Encounter endpoint with includeAll does not also transitively do includeAll on Obs. Might want to fix to not confuse the AI agent.
Longer term, might not be a bad idea to consider making includeAll transitive (or making a new param that does so). This will allow us to fetch the encounters + obs in one request, instead of making N requests to fetch obs of N encounters.
| isLoading: isLoadingEncounters, | ||
| } = useAllPatientEncounters(patient, includeDeleted, filters); | ||
|
|
||
| const scannedEncounters = useMemo(() => encounters.slice(0, scanLimit), [encounters, scanLimit]); |
There was a problem hiding this comment.
This seems bad. We should not get all the encounters of the patient, and then just use 50 of them.
| * server's configured URI prefix, which is not always reachable from the browser, so this walks | ||
| * `startIndex` itself instead of following the link. | ||
| */ | ||
| async function fetchAllPages<T>(url: string): Promise<Array<T>> { |
There was a problem hiding this comment.
The existing useOpenmrsFetchAll() function should do this already.
| const [pageSize, setPageSize] = useState(config.encountersPageSize ?? 10); | ||
| const pageSizes = useMemo(() => Array.from(new Set([pageSize, 10, 20, 50])).sort((a, b) => a - b), [pageSize]); | ||
|
|
||
| const { encounters, totalCount, error, isLoading } = usePatientEncounters( |
There was a problem hiding this comment.
Since we are displaying these in a paginated table, I recommend making usePatientEncounters use useOpenmrsPaginatiion() to fetch paginated data from the server instead of fetchAllData()
| const config = useConfig<Config>(); | ||
| const [selectedUserUuid, setSelectedUserUuid] = useState<string | null>(null); | ||
| const [page, setPage] = useState(1); | ||
| const { events, userActivity, scannedCount, matchedCount, error, isLoading } = usePatientActivity( |
There was a problem hiding this comment.
I think scanLimit puts a hard limit of how many encounters we fetch data for, which means it's possible we'll never see some of the patient's data if the patient has more than 50 (or activityScanLimit) encounters.
Since an "AuditEvent" could be coming from either an encounter or an obs, and we sort the Array<AudiEvent> by timestamp, I don't think we have a good way to correctly show the data without fetching all of them. This can get fairly expensive on the client side, since we make 1 request per encounter. We should consider having a server endpoint for this.
…viewing patient activity
|
Thanks @chibongho ! I have updated the PR to address your observations. I have removed the scan limit and since this is still a prototype app, I have modified the Patient activity tab to load all encounters and obs, so the numbers are accurate. When we demoed this to the ZL team this afternoon, the only observation/question we got was "Where can we test this?". So, maybe we would just deploy this new app to the test servers and once the users have a chance to interact with this app we will address their feedback and also make any performance improvements that we need to do? First I would like to confirm that these are the type of features that they need, it was not clear to me from the teams meeting call today. |
|
Agree, this can go in for now with the scan limit removal. |
|
Thanks @chibongho ! |
2 participants
Summary
A new O3 app that pretty much mirrors the leagacy UI manage encounters admin pages. Please see the README below for full details of all the changes.
Screenshots