Draft PR: Multi-select for departments in locations #10809
Conversation
WalkthroughThis pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as LinkDepartmentsSheet
participant Selector as MultiFacilityOrganizationSelector
participant API as Organization API
UI->>Selector: Render multi-select dropdown(s)
Selector-->>UI: Return selected organizations (array)
UI->>API: Trigger addOrganization (Promise.all for each org)
API-->>UI: Return success for each organization
UI->>UI: Reset selectedOrg state to empty array
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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. |
![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 (4)
src/pages/Facility/settings/organizations/components/MultiFacilityOrganizationSelector.tsx (3)
1-17: Clean up commented and unused importsI notice there are commented out imports on lines 2 and 6 that should be removed for cleaner code.
-// import { Building } from "lucide-react"; -// import CareIcon from "@/CAREUI/icons/CareIcon";
79-79: Remove console.log statement before productionThere's a console.log statement that should be removed before merging to production.
-console.log(selectedLevels);
53-70: Handle edge case when no value is selectedThe
handleLevelChangefunction has an early return ifselectedOrgs.lengthis 0, but it doesn't update the state or notify the parent component. This could lead to a mismatch between the parent's value and the component's internal state when all selections are cleared.const handleLevelChange = (values: string[], level: number) => { let orgList: FacilityOrganization[] | undefined; if (level === 0) { orgList = rootOrganizations?.results; } else if (level - 1 < organizationQueries.length) { orgList = organizationQueries[level - 1].data?.results; } const selectedOrgs = orgList?.filter((org) => values.includes(org.id)) || []; - if (!selectedOrgs.length) return; + const newLevels = selectedLevels.slice(0, level); - newLevels.push(...selectedOrgs); + if (selectedOrgs.length) { + newLevels.push(...selectedOrgs); + } setSelectedLevels(newLevels); onChange(newLevels.map((org) => org.id)); };src/components/Patient/LinkDepartmentsSheet.tsx (1)
100-118: Optimize multiple organization API calls with error handlingThe current implementation uses
Promise.allwhich will fail completely if any single organization addition fails. Consider usingPromise.allSettledfor more robust error handling.Also, there appears to be redundant data being sent in the mutation call.
mutationFn: async () => { if (!selectedOrg.length) return; const { route, pathParams } = getMutationParams( entityType, entityId, facilityId, true, ); // Send each organization separately - await Promise.all( + const results = await Promise.allSettled( selectedOrg.map((orgId) => mutate(route, { pathParams, body: { organization: orgId }, - })({ organization: orgId }), + })(), ), ); + + // Check for any failed requests + const failures = results.filter(r => r.status === 'rejected'); + if (failures.length > 0) { + throw new Error(`Failed to add ${failures.length} organizations`); + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/LinkDepartmentsSheet.tsx(3 hunks)src/pages/Facility/settings/organizations/components/MultiFacilityOrganizationSelector.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/pages/Facility/settings/organizations/components/MultiFacilityOrganizationSelector.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10104
File: src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx:118-118
Timestamp: 2025-02-04T07:18:45.806Z
Learning: The project uses flat translation keys in locale files (e.g., "select_department", "has_sub_departments") without nesting (e.g., "facility.organization.select_department").
⏰ 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: CodeQL-Build
- GitHub Check: cypress-run (1)
- GitHub Check: lint
- GitHub Check: OSSAR-Scan
🔇 Additional comments (6)
src/pages/Facility/settings/organizations/components/MultiFacilityOrganizationSelector.tsx (2)
88-92: LGTM: Root organizations selector implementationThe MultiSelect component is well-implemented with proper data binding and event handling for the root level organizations.
93-106: LGTM: Sub-department selection implementationThe implementation for selecting sub-departments is elegant and handles the hierarchical nature of the data well. The conditional rendering based on
has_childrenis a good approach.src/components/Patient/LinkDepartmentsSheet.tsx (4)
96-96: State updated to support multi-selectionGood update to change the state from a string to an array of strings to support multiple organization selections.
122-122: Success message updated for multiple organizationsGood update to the success message to reflect that multiple organizations can now be added.
188-192: Component updated to use the new multi-select implementationGood replacement of the previous selector with the new
MultiFacilityOrganizationSelectorcomponent that supports multi-selection.
196-197: Button state correctly handles multi-selectionThe button's click handler and disabled state have been properly updated to work with the array-based selection state.
| export default function FacilityOrganizationSelector( | ||
| props: FacilityOrganizationSelectorProps, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Component export name doesn't match imported name
The component is exported as FacilityOrganizationSelector but imported as MultiFacilityOrganizationSelector in LinkDepartmentsSheet.tsx. This mismatch could cause confusion for other developers.
-export default function FacilityOrganizationSelector(
+export default function MultiFacilityOrganizationSelector(
props: FacilityOrganizationSelectorProps,
)📝 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.
| export default function FacilityOrganizationSelector( | |
| props: FacilityOrganizationSelectorProps, | |
| ) { | |
| export default function MultiFacilityOrganizationSelector( | |
| props: FacilityOrganizationSelectorProps, | |
| ) { |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit