Open
Conversation
Contributor
WalkthroughThis pull request introduces permission-based access control throughout the application by adding service, supply, and inventory permission constants to the core permissions system, extending type definitions to include permissions, and applying permission checks across navigation components and pages to conditionally render UI elements. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🚀 Preview Deployment Ready!🔗 Preview URL: https://pr-16038.care-preview-a7w.pages.dev 📱 Mobile Access: This preview will be automatically updated when you push new commits to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds permission-aware rendering and access control for service/location navigation and related pages in the CARE frontend, aligning UI navigation with RBAC capabilities for inventory/pharmacy/service workflows.
Changes:
- Extend
LocationDetailandHealthcareServiceReadSpectypes to includepermissions. - Add new permission slugs + derived booleans via
getPermissions. - Apply
visibility-based filtering for sidebar/nav items and inventory tabs; add a pharmacist-only guard for the prescription queue page.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/location/location.ts | Adds permissions: string[] to LocationDetail to support location-scoped permission checks. |
| src/types/healthcareService/healthcareService.ts | Adds permissions: string[] to service read type for service-scoped permission checks. |
| src/pages/Facility/services/pharmacy/MedicationRequestList.tsx | Adds pharmacist permission gating with toast + redirect. |
| src/pages/Facility/services/inventory/ToReceive.tsx | Adds permission-gated “Raise stock request” action and tab visibility controls. |
| src/pages/Facility/services/inventory/ToDispatch.tsx | Adds permission-gated tab visibility controls and “Create delivery” action. |
| src/pages/Facility/services/HealthcareServiceShow.tsx | Filters service shortcuts based on derived permissions (service vs facility-root fallback). |
| src/components/ui/sidebar/nav-main.tsx | Prevents rendering empty child menus by filtering children using visibility. |
| src/components/ui/sidebar/facility/service/service-nav.tsx | Adds permission-based visibility for service sidebar links. |
| src/components/ui/sidebar/facility/location/location-nav.tsx | Adds permission-based visibility for location sidebar links and nested items. |
| src/common/Permissions.tsx | Introduces new permission constants and exposes them via getPermissions. |
Comment on lines
72
to
97
| <NavTabs | ||
| className="w-full mt-2" | ||
| tabContentClassName="mt-2" | ||
| tabs={{ | ||
| orders: { | ||
| label: t("requests_to_dispatch"), | ||
| component: ( | ||
| <IncomingOrdersTab | ||
| facilityId={facilityId} | ||
| locationId={locationId} | ||
| internal={internal} | ||
| /> | ||
| ), | ||
| visible: canReadSupplyRequest, | ||
| }, | ||
| deliveries: { | ||
| label: t("outgoing_deliveries"), | ||
| component: ( | ||
| <OutgoingDeliveriesTab | ||
| facilityId={facilityId} | ||
| locationId={locationId} | ||
| internal={internal} | ||
| /> | ||
| ), | ||
| visible: canReadSupplyDelivery, | ||
| }, |
There was a problem hiding this comment.
Same issue as ToReceive: if both canReadSupplyRequest and canReadSupplyDelivery are false, NavTabs will have zero visible tabs (it falls back to allTabKeys[0]), which can lead to an undefined tab value and runtime issues. Add a guard/redirect/empty state for the “no readable tabs” scenario before rendering NavTabs.
Comment on lines
+114
to
+118
| if (!canViewAsPharmacist) { | ||
| toast.error(t("no_permission_to_view_page")); | ||
| goBack(`/facility/${facilityId}/locations/${locationId}`); | ||
| } | ||
| }, [canViewAsPharmacist]); |
There was a problem hiding this comment.
The permission guard runs even while facility is still loading. Since facility is initially undefined, facility?.permissions ?? [] becomes [], making canViewAsPharmacist false and redirecting/toasting for users who actually have access. Gate this effect on facility being loaded (e.g., !isFacilityLoading / facility != null) before redirecting, and include the referenced values (t, goBack, facilityId, locationId) in the effect deps to avoid stale closures and hook-lint warnings.
Suggested change
| if (!canViewAsPharmacist) { | |
| toast.error(t("no_permission_to_view_page")); | |
| goBack(`/facility/${facilityId}/locations/${locationId}`); | |
| } | |
| }, [canViewAsPharmacist]); | |
| if (!facility) { | |
| return; | |
| } | |
| if (!canViewAsPharmacist) { | |
| toast.error(t("no_permission_to_view_page")); | |
| goBack(`/facility/${facilityId}/locations/${locationId}`); | |
| } | |
| }, [canViewAsPharmacist, facility, t, goBack, facilityId, locationId]); |
Comment on lines
93
to
+117
| <NavTabs | ||
| className="w-full mt-2" | ||
| tabContentClassName="mt-2" | ||
| tabs={{ | ||
| orders: { | ||
| label: t("requests_raised"), | ||
| component: ( | ||
| <OutgoingOrdersTab | ||
| facilityId={facilityId} | ||
| locationId={locationId} | ||
| internal={internal} | ||
| /> | ||
| ), | ||
| visible: canReadSupplyRequest, | ||
| }, | ||
| deliveries: { | ||
| label: t("incoming_deliveries"), | ||
| component: ( | ||
| <IncomingDeliveriesTab | ||
| facilityId={facilityId} | ||
| locationId={locationId} | ||
| internal={internal} | ||
| /> | ||
| ), | ||
| visible: canReadSupplyDelivery, |
There was a problem hiding this comment.
NavTabs assumes there is at least one visible tab (it uses allTabKeys[0] as a fallback). With the new visible flags, a user who lacks both canReadSupplyRequest and canReadSupplyDelivery will end up with zero visible tabs, which can produce an invalid/undefined Tabs value and break navigation/rendering. Add an explicit guard for the “no readable tabs” case (redirect, show a no-permission empty state, or ensure at least one tab remains visible).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/sidebar/facility/location/location-nav.tsx`:
- Line 61: The visibility flags currently allow pharmacy workflows to be shown
when either canViewAsPharmacist or canReadSupplyDelivery is true; tighten this
by changing those visibility conditions to depend only on canViewAsPharmacist.
Locate the visibility properties in the LocationNav sidebar items (where
visibility: canViewAsPharmacist || canReadSupplyDelivery is used) and remove the
canReadSupplyDelivery operand so the expression is just canViewAsPharmacist;
ensure the same change is applied to the other two occurrences that reference
canReadSupplyDelivery alongside canViewAsPharmacist.
- Around line 29-32: The navigation uses inconsistent permission fields: update
the call to getPermissions in location-nav (where canViewAsPharmacist is
derived) to use the same permission source as other nav components—prefer
facility?.permissions—so replace any use of facility?.root_org_permissions with
facility?.permissions (or ensure a canonical selector) when calling
getPermissions in the location-nav component (referencing getPermissions and
canViewAsPharmacist) so all sidebar/service/location/nav components check the
same permission array.
In `@src/pages/Facility/services/HealthcareServiceShow.tsx`:
- Line 136: The temporary facility-level visibility comes from computing
hasManagingOrganization before the service is resolved; change the logic so
hasManagingOrganization (and any permission fallbacks at the shortcut visibility
checks around the earlier conditional block) is only evaluated after the service
is loaded/defined—e.g., guard with service !== undefined or a serviceLoaded flag
and move the const hasManagingOrganization =
!!service?.managing_organization?.id to run only when service exists, then
update the visibility conditionals that currently fall back to facility-level
(the checks around lines 153-174) to first confirm service is resolved before
applying the fallback.
In `@src/pages/Facility/services/inventory/ToDispatch.tsx`:
- Around line 216-221: OutgoingDeliveriesTab is re-calling
useCurrentLocation/usePermissions/getPermissions to derive
canWriteSupplyDelivery even though ToDispatch already computes it; instead,
compute canWriteSupplyDelivery in ToDispatch (using useCurrentLocation,
usePermissions, getPermissions) and pass it into the OutgoingDeliveriesTab
component as a prop (e.g., canWriteSupplyDelivery={canWriteSupplyDelivery});
then remove the redundant hook calls and permission derivation from
OutgoingDeliveriesTab so it reads the prop and uses it directly.
In `@src/pages/Facility/services/pharmacy/MedicationRequestList.tsx`:
- Around line 113-118: The effect that redirects non-pharmacists in
MedicationRequestList.tsx is firing too early because canViewAsPharmacist is
derived from facility?.permissions and may be false while facility is still
loading; update the useEffect to first guard for facility presence (e.g., if
(facility === undefined) return) before showing toast or calling goBack, and
include all external dependencies in the dependency array (canViewAsPharmacist,
facility, t, goBack, facilityId, locationId) so the check runs again when
facility or translations change; ensure you reference the existing identifiers
canViewAsPharmacist, facility, toast, goBack, t, facilityId, and locationId when
making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec2cbea0-eb7e-4180-800b-493f704d3aa9
📒 Files selected for processing (10)
src/common/Permissions.tsxsrc/components/ui/sidebar/facility/location/location-nav.tsxsrc/components/ui/sidebar/facility/service/service-nav.tsxsrc/components/ui/sidebar/nav-main.tsxsrc/pages/Facility/services/HealthcareServiceShow.tsxsrc/pages/Facility/services/inventory/ToDispatch.tsxsrc/pages/Facility/services/inventory/ToReceive.tsxsrc/pages/Facility/services/pharmacy/MedicationRequestList.tsxsrc/types/healthcareService/healthcareService.tssrc/types/location/location.ts
Comment on lines
+29
to
+32
| const { canViewAsPharmacist } = getPermissions( | ||
| hasPermission, | ||
| facility?.permissions ?? [], | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Facility permission field usage across repo =="
rg -nP 'facility\?\.(permissions|root_org_permissions)|facility\.(permissions|root_org_permissions)' --type ts --type tsx src
echo
echo "== Facility model/type declarations =="
rg -nP 'interface\s+\w*Facility|type\s+\w*Facility' --type ts --type tsx src/types src/pages src/components
echo
echo "== Direct declarations of permission fields in type/model files =="
rg -nP '\b(permissions|root_org_permissions)\b' --type ts --type tsx src/typesRepository: ohcnetwork/care_fe
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Facility permission field usage across repo =="
rg -nP 'facility\?\.(permissions|root_org_permissions)|facility\.(permissions|root_org_permissions)' src --glob '*.ts' --glob '*.tsx'
echo
echo "== Facility model/type declarations =="
rg -nP 'interface\s+\w*Facility|type\s+\w*Facility' src/types src/pages src/components --glob '*.ts' --glob '*.tsx'
echo
echo "== Check location-nav.tsx file =="
cat -n src/components/ui/sidebar/facility/location/location-nav.tsx | head -40
echo
echo "== Check HealthcareServiceShow.tsx file =="
cat -n src/pages/Facility/services/HealthcareServiceShow.tsx | sed -n '125,145p'
echo
echo "== Facility type definition =="
rg -nP 'permissions|root_org_permissions' src/types --glob '*.ts' --glob '*.tsx' -B 2 -A 2Repository: ohcnetwork/care_fe
Length of output: 10215
Use consistent facility.permissions across navigation components for uniform permission checking.
Line 31 uses facility?.permissions, but src/pages/Facility/services/HealthcareServiceShow.tsx Line 134 and src/components/ui/sidebar/facility/service/service-nav.tsx Line 29 use facility?.root_org_permissions. All other usages in the codebase (22+ locations) follow the facility?.permissions pattern. This inconsistency can cause permission-based navigation visibility to diverge if only one permission field is populated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/sidebar/facility/location/location-nav.tsx` around lines 29
- 32, The navigation uses inconsistent permission fields: update the call to
getPermissions in location-nav (where canViewAsPharmacist is derived) to use the
same permission source as other nav components—prefer facility?.permissions—so
replace any use of facility?.root_org_permissions with facility?.permissions (or
ensure a canonical selector) when calling getPermissions in the location-nav
component (referencing getPermissions and canViewAsPharmacist) so all
sidebar/service/location/nav components check the same permission array.
| name: t("pharmacy"), | ||
| url: `${baseUrl}/pharmacy`, | ||
| icon: <CareIcon icon="l-medical-drip" />, | ||
| visibility: canViewAsPharmacist || canReadSupplyDelivery, |
Contributor
There was a problem hiding this comment.
Tighten pharmacy workflow visibility to pharmacy-specific permission.
Lines 61, 71, and 76 allow canReadSupplyDelivery to expose pharmacy workflows. This broadens access beyond canViewAsPharmacist and can reveal medication-facing routes to non-pharmacy roles.
🔒 Proposed fix
- visibility: canViewAsPharmacist || canReadSupplyDelivery,
+ visibility: canViewAsPharmacist,
@@
- visibility: canViewAsPharmacist || canReadSupplyDelivery,
+ visibility: canViewAsPharmacist,
@@
- visibility: canReadSupplyDelivery,
+ visibility: canViewAsPharmacist,Also applies to: 71-71, 76-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/sidebar/facility/location/location-nav.tsx` at line 61, The
visibility flags currently allow pharmacy workflows to be shown when either
canViewAsPharmacist or canReadSupplyDelivery is true; tighten this by changing
those visibility conditions to depend only on canViewAsPharmacist. Locate the
visibility properties in the LocationNav sidebar items (where visibility:
canViewAsPharmacist || canReadSupplyDelivery is used) and remove the
canReadSupplyDelivery operand so the expression is just canViewAsPharmacist;
ensure the same change is applied to the other two occurrences that reference
canReadSupplyDelivery alongside canViewAsPharmacist.
| canListTokens: canListTokensForFacility, | ||
| } = getPermissions(hasPermission, facility?.root_org_permissions ?? []); | ||
|
|
||
| const hasManagingOrganization = !!service?.managing_organization?.id; |
Contributor
There was a problem hiding this comment.
Avoid evaluating fallback permissions before service scope is resolved.
On Line 136, hasManagingOrganization is false while service is still loading, so Lines 153-174 temporarily apply facility-level visibility. That can briefly render shortcuts under the wrong permission scope.
🔧 Proposed fix
- const hasManagingOrganization = !!service?.managing_organization?.id;
+ const hasManagingOrganization = Boolean(service?.managing_organization?.id);
+ const isServiceResolved = !isLoading && Boolean(service);
+ const effectiveShortcutPermissions = isServiceResolved
+ ? hasManagingOrganization
+ ? {
+ canViewSchedule,
+ canViewAppointments,
+ canListTokens,
+ }
+ : {
+ canViewSchedule: canViewScheduleForFacility,
+ canViewAppointments: canViewAppointmentsForFacility,
+ canListTokens: canListTokensForFacility,
+ }
+ : {
+ canViewSchedule: false,
+ canViewAppointments: false,
+ canListTokens: false,
+ };- visibility: hasManagingOrganization
- ? canViewSchedule
- : canViewScheduleForFacility,
+ visibility: effectiveShortcutPermissions.canViewSchedule,
@@
- visibility: hasManagingOrganization
- ? canViewAppointments
- : canViewAppointmentsForFacility,
+ visibility: effectiveShortcutPermissions.canViewAppointments,
@@
- visibility: hasManagingOrganization
- ? canListTokens
- : canListTokensForFacility,
+ visibility: effectiveShortcutPermissions.canListTokens,Also applies to: 153-155, 162-164, 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/services/HealthcareServiceShow.tsx` at line 136, The
temporary facility-level visibility comes from computing hasManagingOrganization
before the service is resolved; change the logic so hasManagingOrganization (and
any permission fallbacks at the shortcut visibility checks around the earlier
conditional block) is only evaluated after the service is loaded/defined—e.g.,
guard with service !== undefined or a serviceLoaded flag and move the const
hasManagingOrganization = !!service?.managing_organization?.id to run only when
service exists, then update the visibility conditionals that currently fall back
to facility-level (the checks around lines 153-174) to first confirm service is
resolved before applying the fallback.
Comment on lines
+216
to
+221
| const { location } = useCurrentLocation(); | ||
| const { hasPermission } = usePermissions(); | ||
| const { canWriteSupplyDelivery } = getPermissions( | ||
| hasPermission, | ||
| location?.permissions ?? [], | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider passing canWriteSupplyDelivery as a prop to avoid redundant hook calls.
OutgoingDeliveriesTab calls useCurrentLocation() and derives permissions again, duplicating the work done in the parent ToDispatch component. This is functional but adds minor overhead.
♻️ Optional: Pass permission as prop
function OutgoingDeliveriesTab({
facilityId,
locationId,
internal,
+ canWriteSupplyDelivery,
}: {
facilityId: string;
locationId: string;
internal: boolean;
+ canWriteSupplyDelivery: boolean;
}) {
const { t } = useTranslation();
- const { location } = useCurrentLocation();
- const { hasPermission } = usePermissions();
- const { canWriteSupplyDelivery } = getPermissions(
- hasPermission,
- location?.permissions ?? [],
- );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/services/inventory/ToDispatch.tsx` around lines 216 - 221,
OutgoingDeliveriesTab is re-calling
useCurrentLocation/usePermissions/getPermissions to derive
canWriteSupplyDelivery even though ToDispatch already computes it; instead,
compute canWriteSupplyDelivery in ToDispatch (using useCurrentLocation,
usePermissions, getPermissions) and pass it into the OutgoingDeliveriesTab
component as a prop (e.g., canWriteSupplyDelivery={canWriteSupplyDelivery});
then remove the redundant hook calls and permission derivation from
OutgoingDeliveriesTab so it reads the prop and uses it directly.
Comment on lines
+113
to
+118
| useEffect(() => { | ||
| if (!canViewAsPharmacist) { | ||
| toast.error(t("no_permission_to_view_page")); | ||
| goBack(`/facility/${facilityId}/locations/${locationId}`); | ||
| } | ||
| }, [canViewAsPharmacist]); |
Contributor
There was a problem hiding this comment.
Race condition and incomplete dependency array in permission check.
Two issues:
-
Premature redirect: When
facilityis still loading (undefined),facility?.permissions ?? []yields an empty array, causingcanViewAsPharmacistto befalse. This triggers an immediate redirect before the facility data loads, even for users with proper permissions. -
Missing dependencies: The effect depends on
t,goBack,facilityId, andlocationId, but onlycanViewAsPharmacistis listed.
🐛 Proposed fix
+ const { facility, isFacilityLoading } = useCurrentFacility();
- const { facility } = useCurrentFacility();
// ...
useEffect(() => {
+ if (isFacilityLoading) return;
if (!canViewAsPharmacist) {
toast.error(t("no_permission_to_view_page"));
goBack(`/facility/${facilityId}/locations/${locationId}`);
}
- }, [canViewAsPharmacist]);
+ }, [canViewAsPharmacist, isFacilityLoading, t, goBack, facilityId, locationId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/services/pharmacy/MedicationRequestList.tsx` around lines
113 - 118, The effect that redirects non-pharmacists in
MedicationRequestList.tsx is firing too early because canViewAsPharmacist is
derived from facility?.permissions and may be false while facility is still
loading; update the useEffect to first guard for facility presence (e.g., if
(facility === undefined) return) before showing toast or calling goBack, and
include all external dependencies in the dependency array (canViewAsPharmacist,
facility, t, goBack, facilityId, locationId) so the check runs again when
facility or translations change; ensure you reference the existing identifiers
canViewAsPharmacist, facility, toast, goBack, t, facilityId, and locationId when
making these changes.
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #7024 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Fixes #16037
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features