Add support for Location Assignment Question #10441
Conversation
WalkthroughThe changes introduce a new local storage key for login preferences and update the authentication flow to store and retrieve this preference. Additionally, the pull request adds full support for a new "location_association" question type in questionnaires. This includes new React components (LocationSearch and LocationQuestion), extended API endpoints and TypeScript interfaces for location associations, and updates throughout questionnaire-related components and structured handling logic. Minor improvements to logging and query filtering are also included. Changes
Possibly related PRs
Suggested labels
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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. |
Deploying care-fe with
|
| Latest commit: |
fe386ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://02edbca9.care-fe.pages.dev |
| Branch Preview URL: | https://location-question.care-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/components/Common/LoginHeader.tsx (1)
77-84: LGTM! Consider adding error handling for localStorage.The implementation correctly retrieves the login preference from localStorage with a sensible default. However, localStorage operations can fail in certain scenarios (e.g., private browsing mode).
Consider wrapping the localStorage access in a try-catch:
- onClick={() => - navigate( - `/login?mode=${ - localStorage.getItem(LocalStorageKeys.loginPreference) ?? - "patient" - }`, - ) - } + onClick={() => { + let mode = "patient"; + try { + mode = localStorage.getItem(LocalStorageKeys.loginPreference) ?? "patient"; + } catch (error) { + console.error("Failed to retrieve login preference:", error); + } + navigate(`/login?mode=${mode}`); + }}src/components/Auth/Login.tsx (1)
102-105: LGTM! Consider adding error handling for localStorage.The useEffect hook correctly persists the login mode preference. However, like in LoginHeader.tsx, localStorage operations should be protected against potential failures.
Consider adding error handling:
// Remember the last login mode useEffect(() => { - localStorage.setItem(LocalStorageKeys.loginPreference, loginMode); + try { + localStorage.setItem(LocalStorageKeys.loginPreference, loginMode); + } catch (error) { + console.error("Failed to save login preference:", error); + } }, [loginMode]);src/types/location/association.ts (1)
12-14: Consider adding location type validation.The
locationfield inLocationAssociationQuestionmight benefit from a more specific type than juststringto ensure valid location references.Consider using a union type or enum to restrict location values:
- location: string; + location: LocationReference | LocationId;src/types/questionnaire/form.ts (1)
13-26: Consider grouping related types.The type union is growing large. Consider grouping related types (e.g., medical, scheduling, location) for better maintainability.
Example approach:
type MedicalResponseType = | "allergy_intolerance" | "medication_request" | "medication_statement" | "symptom" | "diagnosis"; type SchedulingResponseType = | "appointment" | "location_association"; type ResponseValueType = | "string" | "number" | "boolean" | "dateTime" | MedicalResponseType | SchedulingResponseType;src/components/Location/LocationSearch.tsx (1)
39-45: Consider adding cache configuration for better performance.While the query implementation is correct, consider adding cache configuration to optimize performance and reduce unnecessary API calls.
const { data: locations } = useQuery({ queryKey: ["locations", facilityId, mode, search], queryFn: query(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { mode, search }, }), + staleTime: 5 * 60 * 1000, // 5 minutes + cacheTime: 30 * 60 * 1000, // 30 minutes });src/components/Questionnaire/QuestionTypes/LocationQuestion.tsx (1)
90-112: Add loading state for better UX.The datetime input appears immediately after location selection without considering the API state. Add a loading indicator while the association is being created.
- {selectedLocation && ( + {selectedLocation && (isLoading ? ( + <div className="flex items-center justify-center p-4"> + <span className="loading loading-spinner" /> + </div> + ) : ( <div> <label className="text-sm font-medium mb-1 block">Start Time</label> <Input type="datetime-local" value={/* ... */} onChange={/* ... */} disabled={disabled} className="h-9" /> </div> - )} + ))}src/components/Questionnaire/QuestionnaireForm.tsx (1)
270-277: Enhance logging implementation.The current debug logging implementation could be improved by using proper log levels and potentially a logging framework.
Consider applying this diff to improve the logging:
- console.log( - "Processing structured response", - response.structured_type, - ); + console.debug( + `Processing structured response of type: ${response.structured_type}`, + ); const structuredData = response.values?.[0]?.value; if (Array.isArray(structuredData) && structuredData.length > 0) { - console.log("Structured data found", structuredData); + console.debug( + `Found ${structuredData.length} structured data items`, + { type: response.structured_type }, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/common/constants.tsx(1 hunks)src/components/Auth/Login.tsx(3 hunks)src/components/Common/LoginHeader.tsx(1 hunks)src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx(3 hunks)src/components/Location/LocationSearch.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/LocationQuestion.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/QuestionInput.tsx(2 hunks)src/components/Questionnaire/QuestionnaireEditor.tsx(2 hunks)src/components/Questionnaire/QuestionnaireForm.tsx(1 hunks)src/components/Questionnaire/QuestionnaireSearch.tsx(1 hunks)src/components/Questionnaire/structured/handlers.ts(3 hunks)src/components/Questionnaire/structured/types.ts(3 hunks)src/types/location/association.ts(1 hunks)src/types/location/locationApi.ts(2 hunks)src/types/questionnaire/form.ts(3 hunks)src/types/questionnaire/question.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (22)
src/common/constants.tsx (1)
12-12: LGTM! The constant follows the established naming pattern.The new loginPreference constant is well-named and consistent with the existing LocalStorageKeys pattern.
src/types/location/association.ts (2)
1-10: LGTM! Well-structured base interface.The
LocationAssociationinterface provides a solid foundation with proper null handling and type safety.
16-22: Verify datetime string format.The interface correctly makes
end_datetimeandmetaoptional. However, ensure thatstart_datetimeandend_datetimefollow a consistent datetime format.✅ Verification successful
🌐 Web query:
What is the recommended datetime string format in TypeScript?💡 Result:
The recommended datetime string format in TypeScript is **ISO 8601** (`YYYY-MM-DDTHH:mm:ss.sssZ`), which provides standardization and avoids parsing ambiguities[1][2][7]. This format includes: - **Date components**: `YYYY-MM-DD` - **Time components**: `HH:mm:ss.sss` - **Timezone indicator**: `Z` (denotes UTC) or `±HH:mm` offset[2][4] ### Key advantages 1. **Universal support** - Built-in methods like `Date.toISOString()` output ISO 8601[1][7] ```typescript const isoString = new Date().toISOString(); // "2025-02-06T07:10:22.259Z" ``` 2. **Time zone clarity** - Explicit UTC indication (`Z`) prevents local timezone misinterpretation[2][9] 3. **Native parsing** - Works directly with `Date` constructor[1][2] ```typescript const parsedDate = new Date("2025-02-06T07:10:22.259Z"); ``` 4. **Sortability** - Lexicographical order matches chronological order[9] ### Best practices - Use `toISOString()` for serialization instead of locale-dependent methods like `toLocaleString()`[1][5] - For complex operations (time zones, formatting), consider libraries like `moment.js` or `date-fns`[1][6] - Avoid ambiguous formats like `"December 17, 1995 03:24:00"` which lack timezone info[2] For validation, use regular expressions matching ISO 8601 patterns[4][6]: ```typescript const isoRegex = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z$/; ``` This combination ensures type safety, cross-browser compatibility, and unambiguous datetime handling in TypeScript applications[2][6][8]. Citations: - 1: https://www.squash.io/tutorial-working-with-datetime-type-in-typescript/ - 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date - 3: https://community.palantir.com/t/typescript-using-irange-timestamp-and-timezone-format/1621 - 4: https://gist.github.com/MrChocolatine/367fb2a35d02f6175cc8ccb3d3a20054 - 5: https://www.tutorialspoint.com/how-to-convert-date-to-string-in-typescript - 6: https://dev.to/pffigueiredo/fixing-typescript-dates-3fjf - 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString - 8: https://blog.logrocket.com/handling-date-strings-typescript/ - 9: https://forum.freecodecamp.org/t/how-to-format-these-dates-and-sort/453354
Ensure consistent ISO 8601 usage for datetime strings.
The web query confirms that the recommended datetime string format in TypeScript is ISO 8601 (
YYYY-MM-DDTHH:mm:ss.sssZ). The interface currently declaresstart_datetimeandend_datetimesimply as strings, so while their types are consistent, there's no enforcement of the ISO 8601 format at runtime. Consider adding runtime validation (or document the expected format) to guarantee that the datetime strings adhere to ISO 8601.src/types/questionnaire/form.ts (2)
7-7: LGTM! Clean import addition.The import is properly placed with other type imports.
28-40: LGTM! Consistent value type extension.The value union type is properly extended to include the new location association type.
src/components/Questionnaire/structured/types.ts (3)
7-10: LGTM! Clean import grouping.The imports are properly grouped using destructuring.
17-27: LGTM! Consistent data mapping.The
location_associationmapping follows the established pattern.
29-39: Verify request type consistency.While other types use a
datapointsarray wrapper,location_associationdirectly maps toLocationAssociationWrite. Verify if this difference is intentional.- location_association: LocationAssociationWrite; + location_association: { datapoints: LocationAssociationWrite[] };✅ Verification successful
I'll start by gathering additional details to determine if the single mapping for
location_associationis intentional. For example, I want to see how and where theLocationAssociationWritetype is defined and used compared to the other structured types.
🏁 Script executed:
#!/bin/bash # Search for all occurrences of "LocationAssociationWrite" in the repo rg "LocationAssociationWrite" -n .Length of output: 587
Based on the results from our search for
LocationAssociationWrite, the type is defined insrc/types/location/association.tsand used directly in both the API definitions (insrc/types/location/locationApi.ts) and in the structured request map. Other structured request types that hold collections (likeallergy_intolerance,medication_request, etc.) are wrapped in a{ datapoints: …[] }pattern. However, bothencounterandappointment(which are also direct mappings) are single objects rather than an array wrapper.This suggests that the pattern in the
StructuredRequestMapdifferentiates between request types that expect multiple items (wrapped in a datapoints array) and those handling a single item. SinceLocationAssociationWriteis used singularly in the API (viaTBody: Type(LocationAssociationWrite)), the direct mapping in the request map is consistent with the intended design.Action:
• No change is needed forlocation_association. Its mapping as a direct type (and not a{ datapoints: …[] }wrapper) is intentional and consistent with other single-item types such asencounterandappointment.src/types/questionnaire/question.ts (1)
19-27: LGTM! Clean type extension.The
StructuredQuestionTypeis properly extended to include the new location association type.src/components/Location/LocationSearch.tsx (2)
21-27: LGTM! Well-structured props interface.The props interface is well-defined with TypeScript, providing clear type safety and documentation.
47-83: LGTM! Accessible UI implementation.The component follows accessibility best practices:
- Uses semantic HTML with proper ARIA attributes
- Implements keyboard navigation support
- Provides clear visual feedback
src/types/location/locationApi.ts (1)
48-74: LGTM! Well-structured API endpoints with proper typing.The new location association endpoints:
- Follow RESTful conventions
- Maintain consistent path structure
- Implement proper type safety for request/response
src/components/Questionnaire/QuestionnaireSearch.tsx (1)
50-50: LGTM! Added status filter as required.The addition of the status filter to show only active questionnaires aligns with the PR objectives.
src/components/Questionnaire/structured/handlers.ts (2)
6-7: LGTM! Imports are well organized.The new imports for location association types and API are properly organized and follow the project's import structure.
28-30: LGTM! Export is now explicit.The explicit export of
structuredHandlersimproves code clarity and follows TypeScript best practices.src/components/Questionnaire/QuestionTypes/QuestionInput.tsx (1)
23-23: LGTM! Import follows existing pattern.The import for
LocationQuestionis properly placed alongside other question type imports.src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx (2)
26-26: LGTM! Import is properly organized.The import for
LocationAssociationQuestionis correctly placed with other type imports.
36-50: Address potential type duplication.The TODO comment indicates that this type might be defined elsewhere. This could lead to maintenance issues and inconsistencies.
Let's search for potential type duplicates:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for ResponseValueType definitions rg -A 5 "type\s+ResponseValueType\s*="Length of output: 578
Type Duplication Verification Complete
I verified that the
ResponseValueTypeis defined only insrc/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsxand no other duplicate definitions exist in the codebase.src/components/Questionnaire/QuestionnaireForm.tsx (1)
265-265: LGTM! Context object properly updated.The addition of
facilityIdto the context object is necessary for location association requests and follows the existing pattern.src/components/Questionnaire/QuestionnaireEditor.tsx (3)
66-75: LGTM! Well-structured constant for question types.The new
STRUCTURED_QUESTION_TYPESconstant improves maintainability by centralizing the structured question types in one location. The use ofas constensures type safety.
846-850: LGTM! Efficient rendering of structured question types.Good use of the
STRUCTURED_QUESTION_TYPESconstant to map over and generateSelectItemcomponents, making the code more maintainable.
838-838: LGTM! Safe default value for structured_type.Good use of the nullish coalescing operator to provide a default value of "allergy_intolerance" when
structured_typeis undefined.
| const values = | ||
| (questionnaireResponse.values?.[0] | ||
| ?.value as unknown as LocationAssociationQuestion[]) || []; |
There was a problem hiding this comment.
Add type assertion safety check.
The type assertion to LocationAssociationQuestion[] could fail at runtime. Add a type guard for safety.
- const values =
- (questionnaireResponse.values?.[0]
- ?.value as unknown as LocationAssociationQuestion[]) || [];
+ const values = (() => {
+ const value = questionnaireResponse.values?.[0]?.value;
+ if (Array.isArray(value) && value.every(v =>
+ typeof v === 'object' && v !== null && 'location' in v
+ )) {
+ return value as LocationAssociationQuestion[];
+ }
+ return [];
+ })();📝 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.
| const values = | |
| (questionnaireResponse.values?.[0] | |
| ?.value as unknown as LocationAssociationQuestion[]) || []; | |
| const values = (() => { | |
| const value = questionnaireResponse.values?.[0]?.value; | |
| if ( | |
| Array.isArray(value) && | |
| value.every( | |
| v => typeof v === 'object' && v !== null && 'location' in v | |
| ) | |
| ) { | |
| return value as LocationAssociationQuestion[]; | |
| } | |
| return []; | |
| })(); |
| location_association: { | ||
| getRequests: ( | ||
| locationAssociations: LocationAssociationQuestion[], | ||
| { facilityId, encounterId }, | ||
| ) => { | ||
| if (!locationAssociations.length) { | ||
| console.log("No location associations found"); | ||
| return []; | ||
| } | ||
|
|
||
| if (!facilityId) { | ||
| console.log("No facility ID found"); | ||
| return []; | ||
| } | ||
|
|
||
| return locationAssociations.map((locationAssociation) => { | ||
| console.log( | ||
| "Creating location association request", | ||
| locationAssociation, | ||
| ); | ||
| return { | ||
| url: locationApi.createAssociation.path | ||
| .replace("{facility_external_id}", facilityId) | ||
| .replace("{location_external_id}", locationAssociation.location), | ||
| method: locationApi.createAssociation.method, | ||
| body: { | ||
| encounter: encounterId, | ||
| start_datetime: locationAssociation.start_datetime, | ||
| end_datetime: locationAssociation.end_datetime, | ||
| status: locationAssociation.status, | ||
| meta: locationAssociation.meta, | ||
| }, | ||
| reference_id: `location_association_${locationAssociation}`, | ||
| }; | ||
| }); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix reference_id construction and enhance error handling.
The implementation has two issues that need attention:
- The
reference_idconstruction is using the entirelocationAssociationobject which could lead to invalid or overly long IDs. - Error handling relies solely on console.log which might not be sufficient for production use.
Apply this diff to fix the issues:
- reference_id: `location_association_${locationAssociation}`,
+ reference_id: `location_association_${locationAssociation.location}`,Consider enhancing error handling:
if (!locationAssociations.length) {
- console.log("No location associations found");
+ console.warn("No location associations found for processing");
return [];
}
if (!facilityId) {
- console.log("No facility ID found");
+ console.error("Facility ID is required for location associations");
return [];
}
- return locationAssociations.map((locationAssociation) => {
- console.log(
- "Creating location association request",
- locationAssociation,
- );
+ return locationAssociations.map((locationAssociation) => {
+ if (!locationAssociation.location) {
+ console.error("Location ID is required for location association");
+ return null;
+ }📝 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.
| location_association: { | |
| getRequests: ( | |
| locationAssociations: LocationAssociationQuestion[], | |
| { facilityId, encounterId }, | |
| ) => { | |
| if (!locationAssociations.length) { | |
| console.log("No location associations found"); | |
| return []; | |
| } | |
| if (!facilityId) { | |
| console.log("No facility ID found"); | |
| return []; | |
| } | |
| return locationAssociations.map((locationAssociation) => { | |
| console.log( | |
| "Creating location association request", | |
| locationAssociation, | |
| ); | |
| return { | |
| url: locationApi.createAssociation.path | |
| .replace("{facility_external_id}", facilityId) | |
| .replace("{location_external_id}", locationAssociation.location), | |
| method: locationApi.createAssociation.method, | |
| body: { | |
| encounter: encounterId, | |
| start_datetime: locationAssociation.start_datetime, | |
| end_datetime: locationAssociation.end_datetime, | |
| status: locationAssociation.status, | |
| meta: locationAssociation.meta, | |
| }, | |
| reference_id: `location_association_${locationAssociation}`, | |
| }; | |
| }); | |
| }, | |
| }, | |
| location_association: { | |
| getRequests: ( | |
| locationAssociations: LocationAssociationQuestion[], | |
| { facilityId, encounterId }, | |
| ) => { | |
| if (!locationAssociations.length) { | |
| console.warn("No location associations found for processing"); | |
| return []; | |
| } | |
| if (!facilityId) { | |
| console.error("Facility ID is required for location associations"); | |
| return []; | |
| } | |
| return locationAssociations.map((locationAssociation) => { | |
| if (!locationAssociation.location) { | |
| console.error("Location ID is required for location association"); | |
| return null; | |
| } | |
| return { | |
| url: locationApi.createAssociation.path | |
| .replace("{facility_external_id}", facilityId) | |
| .replace("{location_external_id}", locationAssociation.location), | |
| method: locationApi.createAssociation.method, | |
| body: { | |
| encounter: encounterId, | |
| start_datetime: locationAssociation.start_datetime, | |
| end_datetime: locationAssociation.end_datetime, | |
| status: locationAssociation.status, | |
| meta: locationAssociation.meta, | |
| }, | |
| reference_id: `location_association_${locationAssociation.location}`, | |
| }; | |
| }); | |
| }, | |
| }, |
| case "location_association": | ||
| if (encounterId) { | ||
| return ( | ||
| <LocationQuestion | ||
| {...commonProps} | ||
| facilityId={facilityId} | ||
| locationId={patientId} | ||
| encounterId={encounterId} | ||
| /> | ||
| ); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding facilityId validation.
While the implementation follows the pattern of other structured question types, it should validate facilityId since it's required for location associations.
Apply this diff to add the validation:
case "location_association":
- if (encounterId) {
+ if (encounterId && facilityId) {
return (
<LocationQuestion
{...commonProps}
facilityId={facilityId}
locationId={patientId}
encounterId={encounterId}
/>
);
}
+ console.error("Both encounterId and facilityId are required for location associations");
return null;📝 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.
| case "location_association": | |
| if (encounterId) { | |
| return ( | |
| <LocationQuestion | |
| {...commonProps} | |
| facilityId={facilityId} | |
| locationId={patientId} | |
| encounterId={encounterId} | |
| /> | |
| ); | |
| } | |
| return null; | |
| case "location_association": | |
| if (encounterId && facilityId) { | |
| return ( | |
| <LocationQuestion | |
| {...commonProps} | |
| facilityId={facilityId} | |
| locationId={patientId} | |
| encounterId={encounterId} | |
| /> | |
| ); | |
| } | |
| console.error("Both encounterId and facilityId are required for location associations"); | |
| return null; |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
location_question
|
| Run status |
|
| Run duration | 06m 34s |
| Commit |
|
| Committer | Gigin George |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
11
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/Location/LocationSearch.tsx (1)
48-84: Add loading state and debounce search input.The component could benefit from:
- Loading state UI to indicate when locations are being fetched
- Debounced search to reduce API calls
+import { useDebouncedCallback } from 'use-debounce'; export function LocationSearch({...}: LocationSearchProps) { const [open, setOpen] = useState(false); const [search, setSearch] = useState(""); + const [debouncedSearch, setDebouncedSearch] = useState(""); + + const debouncedSetSearch = useDebouncedCallback( + (value: string) => setDebouncedSearch(value), + 300 + ); - const { data: locations } = useQuery({ + const { data: locations, isLoading } = useQuery({ queryKey: ["locations", facilityId, mode, search], ... }); return ( <Popover open={open} onOpenChange={setOpen}> ... <PopoverContent className="w-[400px] p-0"> <Command> <CommandInput placeholder="Search locations..." value={search} - onValueChange={setSearch} + onValueChange={(value) => { + setSearch(value); + debouncedSetSearch(value); + }} /> <CommandEmpty> + {isLoading ? ( + <div className="p-2 text-sm text-muted-foreground">Loading...</div> + ) : ( No locations found. + )} </CommandEmpty> ... </Command> </PopoverContent> </Popover> ); }src/components/Questionnaire/data/StructuredFormData.tsx (1)
123-141: Add i18n support for questionnaire text.Consider using translation keys for the questionnaire title and text to support internationalization.
const location_association_questionnaire: QuestionnaireDetail = { id: "location_association", slug: "location_association", version: "0.0.1", - title: "Location Association", + title: t("questionnaire.location_association.title"), status: "active", subject_type: "patient", questions: [ { id: "location_association", - text: "Location Association", + text: t("questionnaire.location_association.text"), type: "structured", structured_type: "location_association", link_id: "1.1", required: true, }, ], tags: [], };src/components/Patient/PatientInfoCard.tsx (2)
204-240: Extract location badge to a separate component.Consider extracting the location badge logic into a reusable component to improve maintainability and reusability.
+interface LocationBadgeProps { + location: LocationList; + facilityId: string; + patientId: string; + encounterId: string; +} + +function LocationBadge({ location, facilityId, patientId, encounterId }: LocationBadgeProps) { + return ( + <Popover> + <PopoverTrigger asChild> + <div> + <Badge + className="capitalize gap-1 py-1 px-2 cursor-pointer hover:bg-secondary-100" + variant="outline" + title={`Current Location: ${location.name}`} + > + <Building className="w-4 h-4 text-blue-400" /> + {location.name} + <ChevronDown className="h-3 w-3 opacity-50" /> + </Badge> + </div> + </PopoverTrigger> + <PopoverContent align={"start"} className="w-auto p-2"> + <div className="space-y-2"> + <h4 className="font-medium text-sm">Current Location</h4> + <p className="text-sm text-gray-700">{location.name}</p> + <p className="text-sm text-gray-500">{location.description}</p> + </div> + <Button variant="outline"> + <Link + href={`/facility/${facilityId}/patient/${patientId}/encounter/${encounterId}/questionnaire/location_association`} + > + Move Patient + </Link> + </Button> + </PopoverContent> + </Popover> + ); +}Then use it in the component:
-{props.encounter.current_location && ( - <Popover> - ... - </Popover> -)} +{props.encounter.current_location && ( + <LocationBadge + location={props.encounter.current_location} + facilityId={props.encounter.facility.id} + patientId={props.patient.id} + encounterId={props.encounter.id} + /> +)}
231-237: Add loading state to the Move Patient button.Consider adding a loading state to the Move Patient button to prevent multiple clicks while navigating.
-<Button variant="outline"> +<Button + variant="outline" + onClick={() => setIsNavigating(true)} + disabled={isNavigating} +> + {isNavigating ? ( + <span className="flex items-center gap-2"> + <Loader2 className="h-4 w-4 animate-spin" /> + Moving... + </span> + ) : ( <Link href={`/facility/${props.encounter.facility.id}/patient/${props.patient.id}/encounter/${props.encounter.id}/questionnaire/location_association`} > Move Patient </Link> + )} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Location/LocationSearch.tsx(1 hunks)src/components/Patient/PatientInfoCard.tsx(1 hunks)src/components/Questionnaire/data/StructuredFormData.tsx(1 hunks)src/components/Questionnaire/show.tsx(1 hunks)src/types/emr/encounter.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/types/emr/encounter.ts (1)
140-140: LGTM!The addition of
current_locationto the Encounter interface is well-typed and properly integrated.src/components/Questionnaire/show.tsx (1)
307-310: LGTM! Standardized preview mode identifiers.The changes consistently use "preview" as the identifier for all preview-mode props, making the code more maintainable and easier to understand.
Please verify that the
QuestionnaireFormcomponent correctly handles these preview identifiers, particularly for the new location association feature. Run this script to check the implementation:
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Questionnaire/structured/handlers.ts (1)
162-198:⚠️ Potential issueFix reference_id construction and enhance error handling.
The implementation has several issues that need attention:
- The
reference_iduses the entire object which could lead to invalid IDs- Console.log statements in production code
- No validation for required location field
Apply this diff to fix the issues:
- reference_id: `location_association_${locationAssociation}`, + reference_id: `location_association_${locationAssociation.location}`,Consider enhancing error handling:
if (!locationAssociations.length) { - return []; + console.warn("No location associations found for processing"); + return []; } if (!facilityId) { - throw new Error( - "Cannot create location association without a facility", - ); + console.error("Facility ID is required for location associations"); + return []; } - return locationAssociations.map((locationAssociation) => { - console.log( - "Creating location association request", - locationAssociation, - ); + return locationAssociations.map((locationAssociation) => { + if (!locationAssociation.location) { + console.error("Location ID is required for location association"); + return null; + }
🧹 Nitpick comments (2)
src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx (2)
103-110: Consider adding error handling for malformed form state.The type casting of
questionnaireResponse.values[0]?.value as anycould be unsafe. Consider adding type guards or error handling for malformed data.- const formStateValue = (questionnaireResponse.values[0]?.value as any)?.[0]; + const formStateValue = questionnaireResponse.values[0]?.value; + if (!Array.isArray(formStateValue) || !formStateValue[0]) { + console.warn('Malformed form state value'); + return; + } if (formStateValue) { setEncounter(() => ({ - ...formStateValue, + ...formStateValue[0], })); }
76-94: Consider using a reducer for complex state management.The encounter state contains multiple nested objects and is updated in multiple places. Consider using
useReducerfor more predictable state updates.Example reducer implementation:
type EncounterAction = | { type: 'UPDATE_STATUS'; payload: EncounterStatus } | { type: 'UPDATE_CLASS'; payload: EncounterClass } | { type: 'UPDATE_HOSPITALIZATION'; payload: Partial<typeof initialState.hospitalization> }; function encounterReducer(state: EncounterEditRequest, action: EncounterAction): EncounterEditRequest { switch (action.type) { case 'UPDATE_STATUS': return { ...state, status: action.payload }; case 'UPDATE_CLASS': return { ...state, encounter_class: action.payload }; case 'UPDATE_HOSPITALIZATION': return { ...state, hospitalization: { ...state.hospitalization, ...action.payload } }; default: return state; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/Questionnaire/QuestionTypes/AppointmentQuestion.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/DateTimeQuestion.tsx(2 hunks)src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx(1 hunks)src/components/Questionnaire/structured/handlers.ts(5 hunks)src/components/Questionnaire/structured/types.ts(3 hunks)src/types/questionnaire/form.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Questionnaire/structured/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (5)
src/types/questionnaire/form.ts (2)
13-16: LGTM! Well-structured type hierarchy.The introduction of
ResponseValueBaseas a common base type with specific response types extending it improves type safety and maintainability.
53-56: LGTM! New location association type follows the established pattern.The
LocationAssociationResponseValuetype correctly follows the same pattern as other response types, maintaining consistency in the type system.src/components/Questionnaire/QuestionTypes/DateTimeQuestion.tsx (1)
58-58: LGTM! Improved date handling.Passing
Dateobjects directly instead of ISO strings is a better approach as it:
- Maintains type consistency with
DateTimeResponseValue- Prevents potential timezone issues
- Delegates date formatting responsibility to the consumer
Also applies to: 78-78
src/components/Questionnaire/QuestionTypes/AppointmentQuestion.tsx (1)
65-65: LGTM! Improved type safety.Removing the type casting and relying on the new discriminated union types improves type safety.
src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx (1)
128-128: LGTM! Type handling improvement.The direct assignment of
[encounterRequest]tovalueimproves type safety by removing unnecessary type casting.
| getRequests: (encounters, { facilityId, patientId, encounterId }) => { | ||
| if (!encounterId) return []; | ||
| if (!facilityId) { | ||
| throw new Error("Cannot create encounter without a facility"); | ||
| } | ||
| return encounters.map((encounter) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error handling between handlers.
The encounter handler throws an error while other handlers return empty arrays for missing facilityId. Consider making the error handling consistent across all handlers.
Apply this diff to make error handling consistent:
if (!encounterId) return [];
if (!facilityId) {
- throw new Error("Cannot create encounter without a facility");
+ console.error("Facility ID is required for encounter");
+ return [];
}📝 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.
| getRequests: (encounters, { facilityId, patientId, encounterId }) => { | |
| if (!encounterId) return []; | |
| if (!facilityId) { | |
| throw new Error("Cannot create encounter without a facility"); | |
| } | |
| return encounters.map((encounter) => { | |
| getRequests: (encounters, { facilityId, patientId, encounterId }) => { | |
| if (!encounterId) return []; | |
| if (!facilityId) { | |
| console.error("Facility ID is required for encounter"); | |
| return []; | |
| } | |
| return encounters.map((encounter) => { |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit