Add support for adding and removing managing organization for a device #10872
Conversation
WalkthroughThis pull request updates organization-related language entries in the localization file to distinguish between singular and plural forms and introduces support for a new entity type, "device," across several components. The changes extend API definitions and data types to handle managing organizations for devices, integrate the new functionality into device detail views, and update the UI in the devices list with improved pagination and badge displays. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Patient/LinkDepartmentsSheet.tsx (2)
63-97: The getMutationParams function update is well-structured.The function correctly handles the new "device" entity type and returns appropriate mutation parameters. The default case is now for "device" instead of what was likely for "location" before.
Consider using a more DRY approach with a lookup object for the different entity types:
function getMutationParams( entityType: "encounter" | "location" | "device", entityId: string, facilityId: string, isAdd: boolean, ): MutationParams { - if (entityType === "encounter") { - return { - route: isAdd - ? routes.encounter.addOrganization - : routes.encounter.removeOrganization, - pathParams: { encounterId: entityId } as EncounterPathParams, - queryKey: ["encounter", entityId], - }; - } else if (entityType === "location") - return { - route: isAdd - ? locationApi.addOrganization - : locationApi.removeOrganization, - pathParams: { - facility_id: facilityId, - id: entityId, - } as LocationPathParams, - queryKey: ["location", entityId], - }; - - return { - route: isAdd ? deviceApi.addOrganization : deviceApi.removeOrganization, - pathParams: { - facility_external_id: facilityId, - external_id: entityId, - } as DevicePathParams, - queryKey: ["device", entityId], - }; + const config = { + encounter: { + route: isAdd ? routes.encounter.addOrganization : routes.encounter.removeOrganization, + pathParams: { encounterId: entityId } as EncounterPathParams, + queryKey: ["encounter", entityId], + }, + location: { + route: isAdd ? locationApi.addOrganization : locationApi.removeOrganization, + pathParams: { facility_id: facilityId, id: entityId } as LocationPathParams, + queryKey: ["location", entityId], + }, + device: { + route: isAdd ? deviceApi.addOrganization : deviceApi.removeOrganization, + pathParams: { facility_external_id: facilityId, external_id: entityId } as DevicePathParams, + queryKey: ["device", entityId], + } + }; + + return config[entityType]; }
233-264: Improved UI rendering with proper null checking.The UI rendering logic has been enhanced with a null check before mapping over currentOrganizations, and an explicit fallback UI when no organizations are present. This is a good improvement for handling edge cases.
For improved readability, consider using Array.length instead of checking the first element:
-{currentOrganizations[0] != null && +{currentOrganizations.length > 0 && currentOrganizations.map((org) => ( // ... ))} -{currentOrganizations[0] == null && ( +{currentOrganizations.length === 0 && ( <p className="text-sm text-gray-500"> {t("no_organizations_added_yet")} </p> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(3 hunks)src/components/Patient/LinkDepartmentsSheet.tsx(8 hunks)src/pages/Facility/settings/devices/DeviceDetail.tsx(4 hunks)src/types/device/device.ts(2 hunks)src/types/device/deviceApi.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
- GitHub Check: lint
- GitHub Check: cypress-run (1)
🔇 Additional comments (17)
src/types/device/device.ts (2)
3-3: Import added for FacilityOrganization typeGood addition of the import for the FacilityOrganization type that will be used in the DeviceDetail interface.
48-48: Managing organization property added to DeviceDetail interfaceThe addition of the
managing_organizationproperty to the DeviceDetail interface correctly associates an organization with a device. This aligns with the PR objectives of adding support for managing organizations for devices.src/types/device/deviceApi.ts (2)
47-52: API endpoint for adding organization to deviceThe implementation of the
addOrganizationendpoint is correct, using POST method and proper typing. However, I notice this endpoint usesfacility_external_idandexternal_idparameters, while other endpoints in this file usefacility_idandid.Verify that this inconsistency in parameter naming is intentional. If there's a specific reason for using external IDs for these operations, then it's fine, otherwise consider aligning the parameter names with the rest of the API for consistency.
53-58: API endpoint for removing organization from deviceThe implementation of the
removeOrganizationendpoint is correct, with consistent structure to theaddOrganizationendpoint. The same note about parameter naming applies here as well.src/pages/Facility/settings/devices/DeviceDetail.tsx (4)
3-3: Import Lucide icons for organization UI elementsGood addition of Building and ExternalLink icons to enhance the visual representation of organization-related elements.
34-34: Import LinkDepartmentsSheet componentProperly importing the LinkDepartmentsSheet component that will be used to manage organization associations.
148-164: Organization management in header sectionGood implementation of the LinkDepartmentsSheet component in the header section, allowing users to manage the organization directly from the top-level controls. The component is configured with the correct props, including entity type, IDs, and current organization data.
The onUpdate callback correctly invalidates the device query to ensure the UI reflects the latest changes.
243-280: Dedicated organization management sectionWell-structured implementation of the organization management section that:
- Shows the organization name with a link to its details
- Displays a fallback message when no organization is associated
- Provides a button to change or add the organization
- Uses the same LinkDepartmentsSheet component with consistent props
This implementation maintains good UX consistency with the location section above it.
public/locale/en.json (3)
1413-1414: Generalized organization management locale entriesGood implementation of generalized locale entries for organization management. Using
{{entityType}}in the description makes this reusable across different entity types (devices, encounters, locations), which is more maintainable than having entity-specific strings.
1423-1423: Added managing_organization locale entryProper addition of the locale entry for managing organization section heading.
1572-1572: Added no_organization locale entryAdded appropriate fallback text for when no organization is associated with an entity.
src/components/Patient/LinkDepartmentsSheet.tsx (6)
20-20: Appropriate import of deviceApi.The import of deviceApi is correctly placed with other API imports and is necessary for the device-related operations.
25-25: Adding "device" as a valid entity type.Correctly extends the Props interface to support the new device entity type while maintaining the existing structure.
37-39: MutationRoute type update is correct.The MutationRoute type has been properly extended to include the new device API routes for organization management.
99-109: getInvalidateQueries function updated correctly.The function now properly handles the "device" entity type for query invalidation, using a consistent format with entity-specific query keys.
205-205:✅ Verification successful
Good use of dynamic translation.
Using a dynamic translation with entityType parameter allows for more contextual descriptions based on the entity type. This is a good internationalization practice.
🏁 Script executed:
#!/bin/bash # Check if the translation key exists and supports the entityType parameter rg "manage_organization_description" --type json public/locale/en.jsonLength of output: 163
Approval: Dynamic translation implemented correctly
The translation key
"manage_organization_description"exists inpublic/locale/en.jsonand correctly supports theentityTypeparameter. The usage insrc/components/Patient/LinkDepartmentsSheet.tsxis a good internationalization practice, and no changes are needed.
50-53:❓ Verification inconclusive
DevicePathParams interface looks good but uses different naming convention.
The interface correctly defines the parameters needed for device API calls, but note that it uses "external_id" naming convention which differs from the "id" used in LocationPathParams. This is likely intentional to match the API expectations.
🏁 Script executed:
#!/bin/bash # Check if the API routes in deviceApi.ts use the same parameter names rg -A 2 -B 2 "addOrganization|removeOrganization" --type typescript src/types/device/deviceApi.tsLength of output: 134
Below is a revised review comment that highlights the naming convention while noting that additional verification is needed due to an issue with the initial verification command. I've also included a revised shell script command using the correct TypeScript file type flag.
DevicePathParams naming convention – please confirm API matching
The
DevicePathParamsinterface insrc/components/Patient/LinkDepartmentsSheet.tsx(lines 50–53) correctly defines the parameters required for device API calls. Although it usesexternal_id(in contrast toidused inLocationPathParams), this appears to be an intentional decision to align with the API expectations.A verification command was attempted to check the relevant API routes in
src/types/device/deviceApi.ts, but it failed due to improper file type recognition. Please re-run the following command to verify that the API routes (e.g. those involvingaddOrganization/removeOrganization) are consistently usingexternal_id:#!/bin/bash # Re-run verification for API routes in deviceApi.ts using the correct file type flag for TypeScript files. rg -A 2 -B 2 "addOrganization|removeOrganization" -t ts src/types/device/deviceApi.tsOnce the output is confirmed, we can be assured that the naming discrepancy is intentional and corresponds to the API design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Patient/LinkDepartmentsSheet.tsx (1)
230-261: Prefer checking array length for clarity.Instead of
currentOrganizations[0] != null, usingcurrentOrganizations.length > 0more explicitly communicates your intent to check for a non-empty array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/LinkDepartmentsSheet.tsx(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (15)
src/components/Patient/LinkDepartmentsSheet.tsx (15)
20-20: No concerns with the new import.
25-25: ExpandingentityTypeto include"device"is consistent with the new feature requirements.
37-39: Good addition of device API routes to the union type.
50-53: DefiningDevicePathParamsis clear and aligns with device-related data requirements.
55-55: Combining all path parameter interfaces into a singlePathParamstype is a neat approach.
64-64: Extending the signature ofgetMutationParamsto support"device"looks correct.
77-87: Conditional block for handling location parameters remains consistent and readable.
89-96: Device-specific return block properly configures route and queryKey.
100-100: Including"device"ingetInvalidateQueriesensures query invalidation works for devices.
105-109: Query key adjustment for location-based organizations is appropriately extended.
122-127: Fetching mutation parameters during initialization is a logical way to set up the add operation.
130-132: Mutation config for adding an organization is clearly defined.
150-161:⚠️ Potential issueRemove function mismatch for device + duplicated body parameter.
You're always passing
{ organization: organizationId }in thebody, which will fail for"device"since it expects{ managing_organization: ... }. Also, the body is defined both in the mutate options and in a direct call, echoing a past review comment.Apply a fix similar to the following:
return mutate(route, { pathParams, - body: { organization: organizationId }, -})({ organization: organizationId }); + body: entityType === "device" + ? { managing_organization: organizationId } + : { organization: organizationId }, +});
195-195: Using a dynamic description string to reflect different entity types is a good UX practice.
208-217: Conditional logic for adding an organization to a device or other entities is well-structured.
|
|
that was intentionally left out in backend, you can filter by location though; we can skip that here; |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/types/device/deviceApi.ts (1)
53-58: 🛠️ Refactor suggestionMatch path parameter style used elsewhere in the codebase.
Likewise, rename
facilityIdtofacility_idhere to maintain uniform naming conventions.path: "/api/v1/facility/{facilityId}/device/{id}/remove_managing_organization/", - path: "/api/v1/facility/{facilityId}/device/{id}/remove_managing_organization/", + path: "/api/v1/facility/{facility_id}/device/{id}/remove_managing_organization/",
🧹 Nitpick comments (2)
src/components/Patient/LinkDepartmentsSheet.tsx (2)
90-95: Enforce uniform path param usage for device endpoints.
Here,facilityIdshould matchfacility_idif you wish to keep the naming style consistent with the location and other endpoints.- pathParams: { - facilityId, - id: entityId, - } as DevicePathParams, + pathParams: { + facility_id: facilityId, + id: entityId, + } as DevicePathParams,
230-261: Check array length instead of the first element for clarity.
UsingcurrentOrganizations[0] != nullcan be replaced with a more direct condition on array length. This is a minor readability improvement.-{currentOrganizations[0] != null && - currentOrganizations.map((org) => … +{currentOrganizations.length > 0 && + currentOrganizations.map((org) => …
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/LinkDepartmentsSheet.tsx(9 hunks)src/types/device/deviceApi.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (11)
src/components/Patient/LinkDepartmentsSheet.tsx (11)
20-20: Import of deviceApi looks good.
No issues are found with this new import statement.
25-25: Entity type extended to include "device".
This addition is consistent with the new device management feature.
37-39: Including device endpoints in MutationRoute is appropriate.
These new references keep the MutationRoute type in sync with the device API.
55-55: PathParams extension is valid.
The union type includes DevicePathParams seamlessly.
64-64: Updated function signature is coherent.
Adding"device"to theentityTypeparameter aligns with the rest of the logic.
77-87: Location block changes look correct.
The updated block follows a well-structured, consistent approach with the rest of the code.
88-88: No significant change.
Skipping as this line does not introduce functional modifications.
100-100: Added “device” case in getInvalidateQueries is valid.
This matches the new device query key usage.
105-106: Consistent invalidation for "location" entity.
Returning["location", entityId, "organizations"]aligns with the existing pattern.
108-108: Device invalidation strategy looks good.
These queries are properly separated from other entity types.
195-195: I18n usage is correct.
Passing{ entityType }to the translation function keeps copies in sync with the latest entity additions.
location based search implemented for devices ✅
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Facility/settings/devices/DevicesList.tsx (2)
52-67: Consider using translation keys for entity names.The Badge component is a good addition for showing count information, but consider using a translation key for "Device" instead of hardcoding it to ensure proper internationalization.
- entity: "Device", + entity: t("device"),
89-90: Error handling could be improved.While the code updates the variable name correctly, consider adding error handling for the query to show appropriate messages to users when data fetching fails.
- const { data: devices, isLoading } = useQuery({ + const { data: devices, isLoading, isError, error } = useQuery({Then add error UI:
{isError && ( <Card className="col-span-full"> <CardContent className="p-6 text-center text-red-500"> {t("error_fetching_devices")} </CardContent> </Card> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/devices/DevicesList.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/pages/Facility/settings/devices/DevicesList.tsx (5)
29-31: Well-implemented location filtering state.The new state variable for location filtering is correctly typed and initialized as null, allowing for both filtered and unfiltered states.
35-36: Good query invalidation pattern.Including
searchLocation?.idin the queryKey ensures the query will refetch when the location filter changes. The renamed destructured variable improves code readability.
40-40: Properly implemented query parameter for location filtering.The query parameter is correctly added to filter devices by location, matching the state management approach.
69-73: Excellent addition of location filtering.The LocationSearch component integration provides a clean way for users to filter devices by location, improving usability.
101-104: Well-updated pagination logic.The pagination logic correctly uses the renamed
devicesvariable, maintaining proper functionality.
rajku-dev
changed the title
|
@rajku-dev can we drop the location search from this PR? 🤔 Already done in: #10831 |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
Resolve the conflict in en.json 👍 |
|
Are you sure you have replaced all the relevant keys? like "manage_organizations"? |
Yes i replaced with plural forms as suggested, which were used in |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
54-55:⚠️ Potential issueDuplicate search_text parameter found.
There's a duplicate
search_textparameter in the query parameters object. This is likely a copy-paste error and one of them should be removed.queryParams: { search_text: qParams.search || undefined, - search_text: qParams.search || undefined, limit: resultsPerPage, offset: ((qParams.page || 1) - 1) * resultsPerPage, },src/pages/Facility/settings/organizations/FacilityOrganizationView.tsx (1)
82-84:⚠️ Potential issueDuplicate queryParams property found.
There's a duplicate
queryParamsproperty in the query parameters object. This will cause the second one to override the first, which could lead to unexpected behavior.pathParams: { facilityId }, queryParams: { parent: id, - queryParams: { - parent: id, offset: ((qParams.page || 1) - 1) * resultsPerPage, limit: resultsPerPage, name: qParams.search || undefined, },src/components/Patient/PatientDetailsTab/index.tsx (2)
41-43:⚠️ Potential issueFix duplicate line.
The "resource_requests" route is duplicated on lines 41-42. One of these lines should be removed.
{ route: "resource_requests", - route: "resource_requests", component: ResourceRequests, },
14-15:⚠️ Potential issueRemove duplicate prop definition.
The
facilityIdprop is defined twice in thePatientPropsinterface. One of these lines should be removed.export interface PatientProps { facilityId?: string; - facilityId?: string; patientId: string; patientData: Patient, }src/Routers/routes/ConsultationRoutes.tsx (1)
36-42:⚠️ Potential issueFix questionnaireSlug and duplicate props
This section appears to be missing the component declaration and contains duplicated props which would cause overrides.
A complete fix would require seeing the full context of this route handler, but you need to ensure that:
- The component declaration is properly included
- Each prop is only declared once
For example:
"/facility/:facilityId/patient/:patientId/encounter/:encounterId/questionnaire/:slug": + ({ facilityId, encounterId, patientId, slug }) => ( + <Questionnaire facilityId={facilityId} - encounterId={encounterId} - patientId={patientId} encounterId={encounterId} questionnaireSlug={slug} patientId={patientId} subjectType="encounter" /> + ),
🧹 Nitpick comments (17)
src/types/metaAritifact/metaArtifactApi.ts (1)
10-12: Fix incorrect comment description.The comment mentions "Schedule Template related APIs" but this file is defining Meta Artifact related APIs. This appears to be a copy-paste error that should be corrected.
/** - * Schedule Template related APIs + * Meta Artifact related APIs */src/Routers/routes/PatientRoutes.tsx (1)
66-110: Consider refactoring repeated drawing routes logic.Multiple drawing routes, each returning nearly identical components, may increase maintenance overhead. Consider a helper function or a more generalized parameter-driven approach to unify the “new” vs. “existing” drawing routes and reduce duplication.
src/components/Encounter/EncounterInfoCard.tsx (2)
60-62: Optimize React key usageThe key in this Card component is using the entire encounter object (
props.encounter.id). For better performance and to follow React best practices, consider using just the encounter ID as the key.<Card - key={props.encounter.id} + key={encounter.id} className={cn(
109-115: Consider adding translation for "View Details" textThe "View Details" link text appears to be hardcoded rather than using the translation function like other text in the component.
<Link href={`/facility/${facilityId}/patient/${encounter.patient.id}/encounter/${encounter.id}/updates`} className="text-sm text-primary hover:underline text-right flex items-center justify-end group-hover:translate-x-1 transition-transform" > - View Details + {t("view_details")} <CareIcon icon="l-arrow-right" className="ml-1 h-4 w-4" /> </Link>src/pages/Facility/locations/LocationNavbar.tsx (2)
43-54: Consider adding error handling for location children queryThe query for fetching child locations doesn't have explicit error handling. Consider adding error handling to improve user experience when network issues occur.
// Query for this node's children const { data: children, isLoading } = useQuery({ queryKey: ["locations", facilityId, "children", location.id, "kind"], queryFn: query.paginated(locationApi.list, { pathParams: { facility_id: facilityId }, queryParams: { parent: location.id, mode: "kind", }, pageSize: 100, }), enabled: isExpanded, + onError: (error) => { + console.error("Failed to fetch location children:", error); + // Consider using toast for user-friendly error notification + // toast.error(t("error_fetching_location_children")); + }, });
27-41: Consider memoizing LocationTreeNode componentSince
LocationTreeNodeis used recursively and can potentially re-render many child nodes, consider memoizing it with React.memo to prevent unnecessary re-renders of unchanged nodes.-function LocationTreeNode({ +const LocationTreeNode = React.memo(function LocationTreeNode({ location, selectedLocationId, onSelect, expandedLocations, onToggleExpand, level = 0, facilityId, -}: LocationTreeNodeProps) { +}: LocationTreeNodeProps) { const hasChildren = location.has_children; const isExpanded = expandedLocations.has(location.id); const isSelected = location.id === selectedLocationId; const Icon = LocationTypeIcons[location.form as keyof typeof LocationTypeIcons]; - + // Rest of the component... -} +});src/components/Common/Drawings/DrawingTab.tsx (2)
85-87: Consider adjusting or removing the setTimeout delayThe 50ms timeout before generating the SVG might not be necessary and could be removed or reduced if quick rendering is preferred.
- const timeoutId = setTimeout(() => { - generateSvg(); - }, 50); + // Generate SVG immediately or with minimal delay + const timeoutId = setTimeout(() => { + generateSvg(); + }, 10);Alternatively, if there's a specific reason for the delay (like ensuring DOM is ready), consider adding a comment explaining it.
78-82: Enhance error handling for SVG generationThe current error handling for SVG generation only shows a generic message. Consider logging the specific error for debugging purposes.
- } catch (_error) { - toast.error(t("error_generating_svg")); + } catch (error) { + console.error("SVG generation failed:", error); + toast.error(t("error_generating_svg")); } finally { if (isMounted) setIsLoading(false); }src/pages/Facility/locations/LocationList.tsx (2)
21-31: Consider safeguarding against circular references ingetParentChain.
If a location’sparentproperty forms a cycle (e.g., a location that references itself up the chain), thewhile (current)loop could become infinite. You might want to add a fail-safe mechanism, such as keeping track of visited location IDs, to break out if a cycle is detected.
145-155: Confirm handling of updated location details.
WhenlocationDetailis fetched and mapped toLocationListType, the hookhandleLocationSelectis invoked to update the selected location and expand parents. Ensure this logic gracefully handles partial data (e.g., missing parent info) or large parent chains without performance issues.src/components/Common/Drawings/ExcalidrawEditor.tsx (1)
83-108: Ensure correct handling of save mutation and user feedback.
ThehandleSavefunction triggers thesaveDrawingmutation and then calls a success toast. Consider awaiting the mutation’s promise or using the mutation’sonSuccesscallback to guarantee the toast is shown only after a confirmed successful save. This prevents potential race conditions if the user navigates away quickly.const handleSave = async () => { + if (!name.trim()) { ... } + // Wait for mutation to finish before showing success toast try { - saveDrawing({ ... }); - toast.success(t("drawing_saved_successfully")); + await saveDrawing({ ... }); + toast.success(t("drawing_saved_successfully")); } catch (_error) { ... } };src/pages/Facility/locations/LocationContent.tsx (2)
167-197: Prevent potential infinite recursion in breadcrumbs.
Breadcrumbsbuilds a chain from child to parent by walking up theparentpointer. LikegetParentChain, if there's a cyclical reference, this loop might never conclude. Consider adding a visited set or similar check to guard against cycles.
285-342: Improve clarity by extracting the bed vs non-bed grouping logic.
The IIFE used to separate bed and non-bed locations can make the JSX harder to read. Extracting it into a small helper function could improve clarity and make it easier to test or reuse.- {(() => { - const { bedLocations, nonBedLocations } = children.results.reduce(...); - return (...JSX markup...); - })()} + const { bedLocations, nonBedLocations } = groupLocations(children.results); + return ( + <> + {...JSX markup...} + </> + ); // Outside return statement function groupLocations(locations: LocationList[]) { return locations.reduce(...); }src/types/metaAritifact/metaArtifact.ts (4)
5-8: Consider adding JSDoc documentation for better code clarity.The
DrawingValuetype looks well-defined with appropriate typing for Excalidraw elements. Consider adding JSDoc comments to explain the purpose of this type and its properties for better maintainability.+/** + * Represents the value of a drawing created with Excalidraw + */ type DrawingValue = { + /** The application used to create the drawing */ application: "excalidraw"; + /** The elements that make up the drawing */ elements: readonly ExcalidrawElement[]; };
10-13: Consider future extensibility for object types.The current implementation only supports "drawing" as an object type. If there are plans to add more object types in the future, consider using a union type or making this more generic.
-type ObjectTypeValues = { +/** + * Defines the type and value of an object in the system + */ +type ObjectTypeValues<T extends string = "drawing", V = DrawingValue> = { - object_type: "drawing"; - object_value: DrawingValue; + object_type: T; + object_value: V; };
20-25: Extract shared association types to avoid duplication.The association types are duplicated in both request and response types. Consider extracting them to a shared type.
+/** + * Types of entities that can be associated with a meta artifact + */ +type AssociatingType = "patient" | "encounter"; export type MetaArtifactCreateRequest = MetaArtifactBase & ObjectTypeValues & { - associating_type: "patient" | "encounter"; + associating_type: AssociatingType; associating_id: string; };
28-38: Consider more specific date types for improved type safety.The response type is well-structured, but the date properties could benefit from more specific typing.
+/** + * Response type for meta artifact operations + */ export type MetaArtifactResponse = MetaArtifactBase & { id: string; - associating_type: "patient" | "encounter"; + associating_type: AssociatingType; associating_id: string; - created_date: string; - modified_date: string; + created_date: string; // ISO 8601 date string + modified_date: string; // ISO 8601 date string created_by: UserBase; updated_by: UserBase; username: string; } & ObjectTypeValues;
🛑 Comments failed to post (1)
src/Routers/routes/ConsultationRoutes.tsx (1)
29-30:
⚠️ Potential issueFix duplicated facilityId prop
The facilityId prop is duplicated, which could cause unexpected behavior.
<TreatmentSummary facilityId={facilityId} - facilityId={facilityId} encounterId={encounterId} patientId={patientId} subjectType="encounter" />📝 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.<TreatmentSummary facilityId={facilityId} encounterId={encounterId} patientId={patientId} subjectType="encounter" />
They are used in other files are well, you need to replace it there as well. |
…ajku-dev/care_fe into issue/10820/link-device-to-org
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Patient/LinkDepartmentsSheet.tsx (1)
128-140:⚠️ Potential issueFix inconsistent parameter name in the body for device entities.
For device entities, you need to use
managing_organizationinstead oforganizationin the removal logic to match the API expectation.return mutate(route, { pathParams, - body: { organization: organizationId }, + body: entityType === "device" + ? { managing_organization: organizationId } + : { organization: organizationId }, })({ organization: organizationId });
🧹 Nitpick comments (1)
src/components/Patient/LinkDepartmentsSheet.tsx (1)
183-183: Consider making orgType default value conditional on entityType.Since device entities use "managing_organization" while others use "organization", it would be more robust to set the default value based on the entityType.
- orgType = "organization", + orgType = entityType === "device" ? "managing_organization" : "organization",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(5 hunks)src/components/Patient/LinkDepartmentsSheet.tsx(9 hunks)src/pages/Facility/settings/devices/DeviceDetail.tsx(2 hunks)src/types/device/device.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/types/device/device.ts
- src/pages/Facility/settings/devices/DeviceDetail.tsx
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (11)
src/components/Patient/LinkDepartmentsSheet.tsx (11)
20-20: Good addition of device API import.The import for
deviceApiis correctly added to support the new device entity type.
25-25: LGTM: Interface updates support device entity.The Props interface is properly updated to include "device" as a valid entityType and added an optional orgType parameter to distinguish between organization types.
Also applies to: 31-31
38-40: Good extension of MutationRoute type.The MutationRoute type is correctly extended to include device API operations.
51-54: Consider using snake_case for property names to maintain consistency.This interface uses
facilityIdwhereas location path params usefacility_id. Ensure a single style across all path parameters.-interface DevicePathParams { - facilityId: string; - id: string; -} +interface DevicePathParams { + facility_id: string; + id: string; +}
56-56: LGTM: Type extension includes new DevicePathParams.The PathParams type is correctly updated to include DevicePathParams.
65-65: Well-structured entity type handling.The function is properly updated to handle the new "device" entity type with appropriate route and parameter mappings. The refactoring to use if-else if-else improves readability.
Also applies to: 78-89, 91-98
102-103: Good implementation of invalidation queries for device.The getInvalidateQueries function is correctly updated to handle device query invalidation, maintaining the pattern established for other entity types.
Also applies to: 107-110
188-193: LGTM: Proper use of updated getMutationParams.The code correctly uses the updated getMutationParams function with the appropriate parameters.
196-198: LGTM: Simplified mutation function.The mutation function structure is simplified and properly set up to work with the dynamic parameter approach.
221-223: Well-implemented singular/plural text handling for device entities.The text translations are properly updated to handle singular/plural forms based on the entity type, as requested in the previous review comments.
Also applies to: 230-232, 235-238, 263-265, 271-273, 303-305
254-259: Good implementation of dynamic organization property.The code correctly uses the orgType parameter to dynamically set the property name in the request body.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
public/locale/en.json (2)
322-323: Update for "Add Organization" translations added.
The new keys"add_organization_one": "Add Organization"and"add_organization_other": "Add Organizations"clearly separate the singular and plural text, following i18n best practices. Ensure that downstream components correctly use the pluralization functions provided by your localization framework.
1503-1506: Enhanced Management Organization Localization Keys.
The updated keys for managing organizations:
"manage_organization_description_one": "Add or remove organization from this {{entityType}}""manage_organization_description_other": "Add or remove organizations from this {{entityType}}""manage_organization_one": "Manage Organization""manage_organization_other": "Manage Organizations"
provide clear guidance to end-users and support functional context through the placeholder{{entityType}}. Double-check that your components pass the correct entity type (for example, "device") so that the descriptions render as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
public/locale/en.json(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (3)
public/locale/en.json (3)
765-766: Singular/Plural Update for Current Organization Labels.
The keys"current_organization_one": "Current Organization"and"current_organization_other": "Current Organizations"now offer explicit singular and plural labels. This will help in cases where a device or any entity should only show a singular label when one organization is associated. Confirm that the UI components use these keys appropriately.
1514-1514: Consistent Label for Managing Organization.
The"managing_organization": "Managing Organization"key (line 1514) appears to serve as a label and fits well with the other organization-related keys. Verify its usage is consistent with how you refer to the managing organization in other parts of the application.
1671-1673: Refined "No Organization" Messaging.
The changes:
"no_organization": "No organization associated""no_organization_added_yet_one": "No organization added yet""no_organization_added_yet_other": "No organizations added yet"
offer clear and differentiated messages for scenarios when no organization is linked and when none have been added. Make sure that your UI logic correctly selects between the singular and plural keys based on the number of organizations involved.
Proposed Changes
Fixes Devices: Add support for adding and removing managing org. for a device and enhancements to list page #10820
Fixes Inconsistent Text on Managing Organization for Locations #10870
Add support for adding/removing a managing organization.
Improve the list page's cards. We could also show the currently linked location and managing organization in the card.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores