Location Popover & History Sheet #10540
Conversation
…encounter-overview-location
…encounter-overview-location
…encounter-overview-location
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request enhances the application’s localization and location tracking functionality by adding new localization keys for location history and introducing two new React components—LocationHistorySheet and LocationTree—to display a scrollable list of location history items and a visual hierarchy of locations. The PatientInfoCard component has been updated to integrate these new components. Additionally, a navigation link in EncounterShow has been removed, and type definitions have been updated to support location history and hierarchical relationships. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/components/Location/LocationHistorySheet.tsx (2)
28-47: Add accessibility attributes to the Sheet component.The Sheet component should have proper ARIA labels and roles for better accessibility.
- <Sheet> + <Sheet aria-label={t("location_history")}> <SheetTrigger asChild>{trigger}</SheetTrigger> - <SheetContent className="w-full sm:max-w-xl"> + <SheetContent className="w-full sm:max-w-xl" role="dialog" aria-modal="true">
34-45: Consider virtualizing the scroll area for large histories.For better performance with large datasets, consider using a virtualized list component.
Consider using
react-windoworreact-virtualizedfor efficient rendering of large lists.src/types/emr/encounter.ts (1)
120-124: Consider using a union type for LocationHistory status.The status field could benefit from being a union type of allowed values for better type safety.
export type LocationHistory = { start_datetime: string; location: LocationList; - status: string; + status: 'current' | 'previous' | 'planned'; };src/components/Patient/PatientInfoCard.tsx (2)
106-117: Consider extracting the patient link to a reusable component.The patient name link is duplicated in both mobile and desktop views. Consider extracting it to a reusable component to maintain DRY principles.
+const PatientNameLink = ({ patient, encounter }: { patient: Patient; encounter: Encounter }) => ( + <Link + href={`/facility/${encounter.facility.id}/patient/${encounter.patient.id}`} + className="text-gray-950 font-semibold flex items-start gap-0.5" + id="patient-details" + data-cy="patient-details-button" + > + {patient.name} + <CareIcon + icon="l-external-link-alt" + className="w-3 h-3 opacity-50 mt-1" + /> + </Link> +); // Usage in mobile view -<Link - href={`/facility/${encounter.facility.id}/patient/${encounter.patient.id}`} - className="text-gray-950 font-semibold flex items-start gap-0.5" - id="patient-details" - data-cy="patient-details-button" -> - {patient.name} - <CareIcon - icon="l-external-link-alt" - className="w-3 h-3 opacity-50 mt-1" - /> -</Link> +<PatientNameLink patient={patient} encounter={encounter} /> // Usage in desktop view -<Link - href={`/facility/${encounter.facility.id}/patient/${encounter.patient.id}`} - className="text-gray-950 font-semibold flex items-start gap-0.5" - id="patient-details" - data-cy="patient-details-button" -> - {patient.name} - <CareIcon - icon="l-external-link-alt" - className="w-3 h-3 opacity-50 mt-1" - /> -</Link> +<PatientNameLink patient={patient} encounter={encounter} />Also applies to: 131-142
248-290: Improve accessibility of the location history button.The location history button should have an aria-label for better accessibility.
<Button variant="link" className="text-gray-950 underline pl-1 pr-0 font-semibold" + aria-label={t("view_location_history")} > {t("history")} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
public/locale/en.json(2 hunks)src/components/Location/LocationHistorySheet.tsx(1 hunks)src/components/Location/LocationTree.tsx(1 hunks)src/components/Patient/PatientInfoCard.tsx(7 hunks)src/pages/Encounters/EncounterShow.tsx(0 hunks)src/types/emr/encounter.ts(2 hunks)src/types/location/location.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/pages/Encounters/EncounterShow.tsx
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/types/location/location.ts (1)
45-49: Verify potential circular reference handling in LocationList interface.The recursive nature of the
parentproperty could lead to infinite loops when serializing/deserializing location data.Consider implementing a maximum depth check in the location hierarchy traversal logic.
src/components/Patient/PatientInfoCard.tsx (2)
33-37: LGTM! Clean import organization.The new imports are logically grouped and well-organized.
58-58: Verify the hook's return type.The hook's return value is used in a map operation at line 429, implying it returns an array of questionnaire options.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the hook's implementation and return type ast-grep --pattern 'function useQuestionnaireOptions($_) { $$$ return $_; }'Length of output: 1321
Hook Return Type Verified
The
useQuestionnaireOptionshook indeed returns an array of questionnaire options by concatenatingDEFAULT_OPTIONSwith the mappeddata?.resultsarray. The return statement:return [...DEFAULT_OPTIONS, ...questionnaireOptions];guarantees an array return type, which is suitable for subsequent array operations like
.map().
| {history.map((item, index) => ( | ||
| <div key={index}> | ||
| <LocationTree | ||
| location={item.location} | ||
| datetime={item.start_datetime} | ||
| isLatest={index === 0} | ||
| showTimeline | ||
| /> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use stable keys for list items instead of array indices.
Using array indices as keys can lead to performance issues and bugs when items are reordered or removed.
- {history.map((item, index) => (
- <div key={index}>
+ {history.map((item) => (
+ <div key={`${item.location.id}_${item.start_datetime}`}>Committable suggestion skipped: line range outside the PR's diff.
| function getLocationHierarchy(location: LocationList): LocationList[] { | ||
| const hierarchy: LocationList[] = []; | ||
| let current: LocationList | undefined = location; | ||
|
|
||
| while (current?.id) { | ||
| hierarchy.unshift(current); | ||
| current = current.parent; | ||
| } | ||
|
|
||
| return hierarchy; | ||
| } |
There was a problem hiding this comment.
Add safety check for circular references in getLocationHierarchy.
The while loop could potentially run indefinitely if there's a circular reference in the location hierarchy.
function getLocationHierarchy(location: LocationList): LocationList[] {
const hierarchy: LocationList[] = [];
let current: LocationList | undefined = location;
+ const visited = new Set<string>();
while (current?.id) {
+ if (visited.has(current.id)) {
+ console.error('Circular reference detected in location hierarchy');
+ break;
+ }
+ visited.add(current.id);
hierarchy.unshift(current);
current = current.parent;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getLocationHierarchy(location: LocationList): LocationList[] { | |
| const hierarchy: LocationList[] = []; | |
| let current: LocationList | undefined = location; | |
| while (current?.id) { | |
| hierarchy.unshift(current); | |
| current = current.parent; | |
| } | |
| return hierarchy; | |
| } | |
| function getLocationHierarchy(location: LocationList): LocationList[] { | |
| const hierarchy: LocationList[] = []; | |
| let current: LocationList | undefined = location; | |
| const visited = new Set<string>(); | |
| while (current?.id) { | |
| if (visited.has(current.id)) { | |
| console.error('Circular reference detected in location hierarchy'); | |
| break; | |
| } | |
| visited.add(current.id); | |
| hierarchy.unshift(current); | |
| current = current.parent; | |
| } | |
| return hierarchy; | |
| } |
| {questionnaireOptions.map((option) => ( | ||
| <DropdownMenuItem key={option.slug} asChild> | ||
| <Link | ||
| href={`/facility/${encounter.facility.id}/patient/${patient.id}/encounter/${encounter.id}/questionnaire/${option.slug}`} | ||
| className="cursor-pointer text-gray-800" | ||
| data-cy="update-encounter-option" | ||
| > | ||
| {t(option.title)} | ||
| </Link> | ||
| </DropdownMenuItem> | ||
| ))} | ||
| <DropdownMenuSeparator /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state for questionnaire options.
The dropdown menu should handle the loading state of questionnaire options to prevent potential UI flickers.
+const questionnaireOptions = useQuestionnaireOptions("encounter_actions");
+const isLoading = !questionnaireOptions?.length;
<DropdownMenuContent align="end">
+ {isLoading ? (
+ <DropdownMenuItem disabled>
+ {t("loading_options")}
+ </DropdownMenuItem>
+ ) : (
{questionnaireOptions.map((option) => (
<DropdownMenuItem key={option.slug} asChild>
<Link
href={`/facility/${encounter.facility.id}/patient/${patient.id}/encounter/${encounter.id}/questionnaire/${option.slug}`}
className="cursor-pointer text-gray-800"
data-cy="update-encounter-option"
>
{t(option.title)}
</Link>
</DropdownMenuItem>
))}
+ )}
<DropdownMenuSeparator />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {questionnaireOptions.map((option) => ( | |
| <DropdownMenuItem key={option.slug} asChild> | |
| <Link | |
| href={`/facility/${encounter.facility.id}/patient/${patient.id}/encounter/${encounter.id}/questionnaire/${option.slug}`} | |
| className="cursor-pointer text-gray-800" | |
| data-cy="update-encounter-option" | |
| > | |
| {t(option.title)} | |
| </Link> | |
| </DropdownMenuItem> | |
| ))} | |
| <DropdownMenuSeparator /> | |
| const questionnaireOptions = useQuestionnaireOptions("encounter_actions"); | |
| const isLoading = !questionnaireOptions?.length; | |
| <DropdownMenuContent align="end"> | |
| {isLoading ? ( | |
| <DropdownMenuItem disabled> | |
| {t("loading_options")} | |
| </DropdownMenuItem> | |
| ) : ( | |
| questionnaireOptions.map((option) => ( | |
| <DropdownMenuItem key={option.slug} asChild> | |
| <Link | |
| href={`/facility/${encounter.facility.id}/patient/${patient.id}/encounter/${encounter.id}/questionnaire/${option.slug}`} | |
| className="cursor-pointer text-gray-800" | |
| data-cy="update-encounter-option" | |
| > | |
| {t(option.title)} | |
| </Link> | |
| </DropdownMenuItem> | |
| )) | |
| )} | |
| <DropdownMenuSeparator /> | |
| </DropdownMenuContent> |
CARE
|
||||||||||||||||||||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
encounter-overview-location
|
| Run status |
|
| Run duration | 06m 27s |
| Commit |
|
| Committer | Amjith Titus |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
3
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
8
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
facility_spec/facility_creation.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Facility Management > Create a new facility using the admin role and verify validation errors |
Test Replay
Screenshots
|
|
users_spec/user_creation.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| User Creation > should create a new user successfully |
Test Replay
Screenshots
|
|
patient_spec/patient_encounter.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Patient Encounter Questionnaire > Create a new ABG questionnaire and verify the values |
Test Replay
Screenshots
|
|
Deploying care-fe with
|
| Latest commit: |
ecaa675
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://41a079cc.care-fe.pages.dev |
| Branch Preview URL: | https://encounter-overview-location.care-fe.pages.dev |
Co-authored-by: Rithvik Nishad <mail@rithviknishad.dev>
Co-authored-by: Rithvik Nishad <mail@rithviknishad.dev>
Co-authored-by: Rithvik Nishad <mail@rithviknishad.dev>
…etwork/care_fe into encounter-overview-location
…encounter-overview-location
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Location/LocationTree.tsx (1)
41-43:⚠️ Potential issueAdd error handling for invalid date formatting.
The date formatting could throw an error if the datetime string is invalid.
- {format(new Date(datetime), "MMM d, yyyy h:mm a")} + {(() => { + try { + return format(new Date(datetime), "MMM d, yyyy h:mm a"); + } catch (error) { + console.error('Invalid date format:', error); + return 'Invalid date'; + } + })()}Also applies to: 65-67
🧹 Nitpick comments (1)
src/components/Location/LocationTree.tsx (1)
40-44: Extract duplicated date formatting UI into a shared component.The date formatting UI is duplicated between the root and child nodes. Consider extracting it into a shared component to improve maintainability.
+const DateTimeDisplay: React.FC<{ datetime: string }> = ({ datetime }) => ( + <div className="pl-6 flex items-center text-sm font-normal text-gray-700 italic"> + {(() => { + try { + return format(new Date(datetime), "MMM d, yyyy h:mm a"); + } catch (error) { + console.error('Invalid date format:', error); + return 'Invalid date'; + } + })()} + </div> +); function LocationNode({...}: LocationNodeProps) { if (!location.parent?.id) { return ( ... - {isLast && datetime && ( - <div className="pl-6 flex items-center text-sm font-normal text-gray-700 italic"> - {format(new Date(datetime), "MMM d, yyyy h:mm a")} - </div> - )} + {isLast && datetime && <DateTimeDisplay datetime={datetime} />} </div> ); } ... - {isLast && datetime && ( - <div className="pl-6 flex items-center text-sm font-normal text-gray-700 italic"> - {format(new Date(datetime), "MMM d, yyyy h:mm a")} - </div> - )} + {isLast && datetime && <DateTimeDisplay datetime={datetime} />}Also applies to: 64-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Location/LocationTree.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Location/LocationTree.tsx (2)
1-20: LGTM! Clean imports and well-defined interfaces.The imports are minimal and necessary, and the interfaces are well-structured with clear prop types.
74-105: LGTM! Clean implementation of the location tree visualization.The component effectively implements the tree visualization with:
- Proper conditional rendering of timeline
- Clear visual hierarchy using icons and styling
- Good use of CSS classes for layout and spacing
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Patient/PatientInfoCard.tsx (1)
348-423: Consider error boundary for location components.The location-related components (
LocationHistorySheetandLocationTree) are rendered without error boundaries, which could cause the entire card to fail if these components encounter errors.Consider wrapping these components with an error boundary:
+import { ErrorBoundary } from "@/components/Common/ErrorBoundary"; {props.encounter.current_location ? ( <Popover> <PopoverTrigger asChild> {/* ... */} </PopoverTrigger> <PopoverContent align={"start"} className="w-auto p-2"> <div className="space-y-2 p-2 items-center"> {/* ... */} + <ErrorBoundary> <LocationTree location={props.encounter.current_location} /> + </ErrorBoundary> {/* ... */} </div> </PopoverContent> </Popover> ) : ( // ... )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(5 hunks)src/components/Patient/PatientInfoCard.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (3)
src/components/Patient/PatientInfoCard.tsx (3)
33-37: Consider adding loading state for questionnaire options.The hook
useQuestionnaireOptionsis used without handling its loading state, which could lead to UI flickers.This was previously suggested in a past review. The loading state should be handled as follows:
+const { data: questionnaireOptions, isLoading } = useQuestionnaireOptions("encounter_actions"); +const options = questionnaireOptions ?? []; <DropdownMenuContent align="end"> + {isLoading ? ( + <DropdownMenuItem disabled> + {t("loading")} + </DropdownMenuItem> + ) : ( - {questionnaireOptions.map((option) => ( + {options.map((option) => (
106-117: LGTM! Patient name link implementation.The patient name is now consistently clickable across both mobile and desktop layouts, with a clear visual indicator (external link icon) showing it's a navigation element.
Also applies to: 131-142
446-457: Verify questionnaire option links.Ensure that all questionnaire option slugs in the dropdown menu correspond to valid routes and have proper translations.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for questionnaire option translations and route handlers # Test 1: Check for translation keys rg -g '*.json' -A 1 '"title":' # Test 2: Check for route handlers for questionnaire options rg -g '*.{ts,tsx}' 'questionnaire/\$'Length of output: 4492
Verification of Questionnaire Option Links
- The route structure (
/facility/{facilityId}/patient/{patientId}/encounter/{encounterId}/questionnaire/{option.slug}) in the dropdown is consistent with similar usages across the codebase (e.g., in QuickAccess and PatientInfoCard).- The pattern for handling questionnaire option links is confirmed by corresponding route handlers in the Questionnaire-related components.
- The translation keys for option titles, referenced by
t(option.title), are present in the locale/config JSON files (as seen by the entries in files likepublic/locale/en.json), ensuring proper translations.
…encounter-overview-location
|
@amjithtitus09 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Removals