Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .example.env
Original file line number Diff line number Diff line change
Expand Up @@ -190,5 +190,9 @@ REACT_DECIMAL_ROUNDING_METHOD=ROUND_HALF_UP
# Maximum number of forms that can be favorited in the forms dialog (default: 5)
REACT_MAX_FORM_DIALOG_FAVORITES=5

# Maximum number of datapoints allowed in a single upsert request (default: 100)
# Must not exceed the backend limit
REACT_MAX_DATAPOINTS_PER_UPSERT=100

# Default tab for medication selector. Valid values: product, valueset
REACT_MEDICATION_VALUE_SET_SELECT_DEFAULT_TAB=product
11 changes: 9 additions & 2 deletions care.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ const careConfig = {
: undefined),

defaultDischargeDisposition: env.REACT_DEFAULT_DISCHARGE_DISPOSITION as
| EncounterDischargeDisposition
| undefined,
EncounterDischargeDisposition | undefined,

mapFallbackUrlTemplate:
env.REACT_MAPS_FALLBACK_URL_TEMPLATE ||
Expand Down Expand Up @@ -407,6 +406,14 @@ const careConfig = {
maxFormDialogFavorites: env.REACT_MAX_FORM_DIALOG_FAVORITES
? parseInt(env.REACT_MAX_FORM_DIALOG_FAVORITES, 10)
: 5,

/**
* Maximum number of datapoints allowed in a single upsert request.
* Must not exceed the backend limit (defaults to 100).
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
*/
maxDatapointsPerUpsert: env.REACT_MAX_DATAPOINTS_PER_UPSERT
Comment thread
rithviknishad marked this conversation as resolved.
? parseInt(env.REACT_MAX_DATAPOINTS_PER_UPSERT, 10)
Comment thread
NikhilA8606 marked this conversation as resolved.
: 100,
Comment thread
rithviknishad marked this conversation as resolved.
Comment thread
rithviknishad marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
} as const;

export default careConfig;
4 changes: 3 additions & 1 deletion public/locale/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -3063,6 +3063,7 @@
"issuer_type": "Issuer Type",
"item": "Item",
"item_condition": "Item Condition?",
"item_limit_reached": "Item limit reached",
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
"item_location": "Item Location",
"item_marked_as_abandoned": "Item marked as abandoned successfully",
"item_marked_as_entered_in_error": "Item marked as entered in error successfully",
Expand Down Expand Up @@ -3183,7 +3184,7 @@
"live": "Live",
"live_monitoring": "Live Monitoring",
"live_patients_total_beds": "Live Patients / Total beds",
"load_from_order": "Load from order",
"load_from_order_with_items": "Load from order ({{count}} items)",
"load_more": "Load More",
Comment thread
NikhilA8606 marked this conversation as resolved.
"loading": "Loading...",
"loading_appointment_details": "Loading appointment details...",
Expand Down Expand Up @@ -3410,6 +3411,7 @@
"max": "Max",
"max_applicable_discounts": "Maximum Applicable Discounts",
"max_applicable_discounts_description": "The maximum number of discount components that can be applied to a single invoice. Set to 0 for no discount.",
"max_datapoints_per_upsert_limit": "You cannot add more than {{count}} items in a single delivery.",
Comment thread
NikhilA8606 marked this conversation as resolved.
"max_dosage_24_hrs": "Max. dosage in 24 hrs.",
"max_dosage_in_24hrs_gte_base_dosage_error": "Max. dosage in 24 hours must be greater than or equal to base dosage",
"max_favorites_reached": "You've reached the limit. Only {{count}} forms can be favourited.",
Expand Down
1 change: 1 addition & 0 deletions scripts/validate-env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ const envSchema = z
})
.optional(),
REACT_MAX_FORM_DIALOG_FAVORITES: numberAsString.optional(),
REACT_MAX_DATAPOINTS_PER_UPSERT: numberAsString.optional(),
Comment thread
rithviknishad marked this conversation as resolved.
Comment thread
rithviknishad marked this conversation as resolved.
})
.superRefine(async (data, ctx) => {
// Ensure at least one API URL configuration is provided
Expand Down
19 changes: 3 additions & 16 deletions src/CAREUI/display/Callout.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import React from "react";
import React, { ReactNode } from "react";

import { cn } from "@/lib/utils";

interface CalloutProps {
variant?: "primary" | "secondary" | "warning" | "alert" | "danger";
className?: string;
badge: string;
badge: ReactNode;
children: React.ReactNode;
}

Expand All @@ -27,20 +27,7 @@ export default function Callout({
props.className,
Comment thread
NikhilA8606 marked this conversation as resolved.
)}
>
<div
className={cn(
"h-min rounded-full border bg-white px-2",
{
primary: "border-primary-200",
secondary: "border-secondary-300",
warning: "border-warning-300",
alert: "border-purple-300",
danger: "border-danger-300",
}[variant],
)}
>
<span className="font-medium">{props.badge}</span>
</div>
<span className="font-medium">{props.badge}</span>
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { zodResolver } from "@hookform/resolvers/zod";
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
import { PlusCircle, Trash2 } from "lucide-react";
import { PlusCircle, Trash2, TriangleAlert } from "lucide-react";
import { useQueryParams } from "raviger";
import { useCallback, useMemo, useState } from "react";
import { useFieldArray, useForm } from "react-hook-form";
Expand All @@ -12,6 +12,7 @@ import careConfig from "@/../care.config";
import { cn } from "@/lib/utils";

import { DisablingCover } from "@/components/Common/DisablingCover";
import { Alert, AlertDescription, AlertTitle } from "@/components/ui/alert";
import Autocomplete from "@/components/ui/autocomplete";
import { Button } from "@/components/ui/button";
import { Card, CardContent } from "@/components/ui/card";
Expand Down Expand Up @@ -129,6 +130,9 @@ interface Props {
origin?: string;
destination: string;
onSuccess: () => void;

Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
supplyDeliveriesCount: number;
isFetchingSupplyDeliveries: boolean;
}

export function AddSupplyDeliveryForm({
Expand All @@ -137,6 +141,8 @@ export function AddSupplyDeliveryForm({
origin,
destination,
onSuccess,
supplyDeliveriesCount,
isFetchingSupplyDeliveries,
}: Props) {
const { t } = useTranslation();
const queryClient = useQueryClient();
Expand Down Expand Up @@ -237,6 +243,9 @@ export function AddSupplyDeliveryForm({
name: "items",
});

const hasReachedUpsertLimit =
Comment thread
NikhilA8606 marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two different formulas for "hit the limit" in the same component: hasReachedUpsertLimit only checks supplyDeliveriesCount, while the alert banner further down checks supplyDeliveriesCount + fields.length. So the "Add Item"/"Load from Order" buttons at the bottom stay enabled even after the in-form alert says you're capped. Pick one source of truth and use it everywhere, or this'll bite you in a "why can I still click add" bug report.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two formulas for the same limit in one component: hasReachedUpsertLimit here uses supplyDeliveriesCount >= max, while the alert at ~912 uses supplyDeliveriesCount + fields.length >= max. Collapse to one derived value (remainingCapacity = max - supplyDeliveriesCount - fields.length) and drive both the disabled props and the alert from it — otherwise the "Add Item"/"Load from order" buttons and the alert can disagree about whether you're at the cap.

supplyDeliveriesCount >= careConfig.maxDatapointsPerUpsert;

const loadFromSupplyRequests = () => {
setIsSelectDialogOpen(true);
handleSelectAll(true);
Expand Down Expand Up @@ -897,29 +906,42 @@ export function AddSupplyDeliveryForm({
</div>
</div>

<div className="flex flex-row gap-2 mt-4 items-end">
<Button
type="button"
variant="outline"
onClick={handleAddAnotherItem}
>
<PlusCircle className="mr-2 size-4" />
{t("add_another")}
</Button>
{supplyRequests?.results?.length &&
supplyRequests?.results?.length > 0 && (
<Button
type="button"
variant="secondary"
onClick={loadFromSupplyRequests}
>
{t("load_from_order")} ({supplyRequests?.count}{" "}
{t("items")}
)
<ShortcutBadge actionId="load-from-order" />
</Button>
)}
</div>
{supplyDeliveriesCount + fields.length >=
Comment thread
NikhilA8606 marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole limit thing is enforced purely client-side. Nothing stops someone from hammering the API directly past maxDatapointsPerUpsert. Fine if the backend has its own real cap (the .env comment implies it does), but make sure that's actually true and not just wishful thinking — a UI-only limit is not a limit.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiding the whole button row behind the limit means when the cap is hit the user also loses "Load from order" and any way to interact — and the identical supplyDeliveriesCount >= limit logic already lives in hasReachedUpsertLimit two hundred lines up. Reuse it (hasReachedUpsertLimit || supplyDeliveriesCount + fields.length >= limit) instead of re-deriving the condition inline.

Comment thread
NikhilA8606 marked this conversation as resolved.
careConfig.maxDatapointsPerUpsert ? (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the limit is hit you replace the entire button row with a warning — including the "Load from order" button and, in the block below, nothing stops the already-added fields from being submitted. Hiding the button is not enforcement; the submit path should reject it too. Also, hasReachedUpsertLimit (line 245) uses a different formula than this one (count + fields.length); two competing definitions of the same rule is how bugs are born.

Comment thread
NikhilA8606 marked this conversation as resolved.
<Alert className="border-amber-300 bg-amber-50 text-amber-800 [&>svg]:text-amber-600 *:data-[slot=alert-description]:text-amber-700">
<TriangleAlert />
<AlertTitle>{t("item_limit_reached")}</AlertTitle>
<AlertDescription>
{t("max_datapoints_per_upsert_limit", {
count: careConfig.maxDatapointsPerUpsert,
})}
</AlertDescription>
</Alert>
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
) : (
<div className="flex flex-row gap-2 mt-4 items-end">
<Button
type="button"
variant="outline"
onClick={handleAddAnotherItem}
Comment thread
NikhilA8606 marked this conversation as resolved.
>
<PlusCircle className="mr-2 size-4" />
{t("add_another")}
</Button>
{supplyRequests?.results?.length &&
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
supplyRequests?.results?.length > 0 && (
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
<Button
type="button"
variant="secondary"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasReachedUpsertLimit ignores fields.length while this render branch uses supplyDeliveriesCount + fields.length. Two different definitions of "at the limit" in one component, so the buttons below stay enabled well past the point the warning fires. Compute it once and use it everywhere.

onClick={loadFromSupplyRequests}
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
>
{t("load_from_order_with_items", {
count: supplyRequests?.count,
})}
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
<ShortcutBadge actionId="load-from-order" />
</Button>
)}
</div>
)}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

<div className="flex justify-between">
<Button
Expand Down Expand Up @@ -952,10 +974,15 @@ export function AddSupplyDeliveryForm({
type="button"
variant="outline_primary"
onClick={loadFromSupplyRequests}
disabled={
isProcessing ||
hasReachedUpsertLimit ||
isFetchingSupplyDeliveries
}
>
{t("load_from_order")} ({supplyRequests?.count}{" "}
{t("items")}
)
{t("load_from_order_with_items", {
count: supplyRequests?.count,
})}
<ShortcutBadge actionId="load-from-order" />
</Button>
<p>- {t("or")} -</p>
Expand All @@ -971,6 +998,11 @@ export function AddSupplyDeliveryForm({
type="button"
variant="outline_primary"
onClick={() => handleAddAnotherItem()}
disabled={
isProcessing ||
hasReachedUpsertLimit ||
isFetchingSupplyDeliveries
}
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
>
<PlusCircle className="mr-2 size-4" />
{t("add_item")}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import {
Hash,
MoreVertical,
Printer,
TriangleAlert,
Truck,
} from "lucide-react";
import { Link, navigate } from "raviger";
import { useMemo, useState } from "react";
import { Trans, useTranslation } from "react-i18next";
import { toast } from "sonner";

import Callout from "@/CAREUI/display/Callout";
import CareIcon from "@/CAREUI/icons/CareIcon";
import ConfirmActionDialog from "@/components/Common/ConfirmActionDialog";
import Page from "@/components/Common/Page";
Expand Down Expand Up @@ -76,6 +78,7 @@ import mutate from "@/Utils/request/mutate";
import query from "@/Utils/request/query";
import { ExtensionContexts } from "@/Utils/schema/types";
import { formatDateTime, formatName } from "@/Utils/utils";
import careConfig from "@careConfig";

interface Props {
facilityId: string;
Expand Down Expand Up @@ -264,18 +267,22 @@ export function DeliveryOrderShow({

const isRequester = locationId === deliveryOrder?.destination.id;

const { data: supplyDeliveries, isLoading: isLoadingSupplyDeliveries } =
useQuery({
queryKey: ["supplyDeliveries", deliveryOrderId],
queryFn: query.paginated(supplyDeliveryApi.listSupplyDelivery, {
queryParams: {
order: deliveryOrderId,
facility: facilityId,
ordering: "created_date",
},
}),
enabled: !!deliveryOrderId,
});
const {
data: supplyDeliveries,
isLoading: isLoadingSupplyDeliveries,
isFetching: isFetchingSupplyDeliveries,
} = useQuery({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You deleted enabled: !!deliveryOrderId. Congratulations, the query now fires with an undefined order id and cheerfully asks the backend for somebody else’s deliveries. Put the guard back.

queryKey: ["supplyDeliveries", deliveryOrderId],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enabled: !!deliveryOrderId was dropped from this query. If deliveryOrderId can ever be empty on first render this now fires a request with order="". If it's guaranteed non-empty by the route, fine — but the removal looks incidental to the limit change rather than intentional. Was it deliberate?

queryFn: query.paginated(supplyDeliveryApi.listSupplyDelivery, {
queryParams: {
order: deliveryOrderId,
facility: facilityId,
ordering: "created_date",
},
Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
rithviknishad marked this conversation as resolved.
pageSize: careConfig.maxDatapointsPerUpsert,
Comment thread
NikhilA8606 marked this conversation as resolved.
}),
Comment thread
NikhilA8606 marked this conversation as resolved.
enabled: !!deliveryOrderId,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You deleted enabled: !!deliveryOrderId while refactoring. Now the query fires even when there's no order id, which is exactly the kind of pointless failing request that people spend an afternoon debugging later. Put it back.

Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.

const supplyOrderId = supplyDeliveries?.results?.find(
(delivery) => delivery.supply_request && delivery.supply_request.id,
Expand Down Expand Up @@ -471,6 +478,10 @@ export function DeliveryOrderShow({
anyCompletedSupplyDeliveries,
);

const hasReachedUpsertLimit =
supplyDeliveries &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing results.length to the limit, when you just set limit to that exact same number? Of course it will never exceed it — the page size caps it. Use the paginated count, which is the actual total, instead of a self-fulfilling prophecy.

Comment thread
NikhilA8606 marked this conversation as resolved.
supplyDeliveries.results.length >= careConfig.maxDatapointsPerUpsert;
Comment thread
rithviknishad marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.

Comment thread
NikhilA8606 marked this conversation as resolved.
return (
<Page
title={t("delivery_order_details")}
Expand Down Expand Up @@ -841,7 +852,7 @@ export function DeliveryOrderShow({
</div>
</CardHeader>
<CardContent className="p-2">
{isLoadingSupplyDeliveries ? (
{isLoadingSupplyDeliveries || isFetchingSupplyDeliveries ? (
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
<div className="space-y-2">
{Array.from({ length: 3 }).map((_, i) => (
<div key={i} className="animate-pulse">
Expand Down Expand Up @@ -911,14 +922,33 @@ export function DeliveryOrderShow({
<></>
)}

{hasReachedUpsertLimit && (
Comment thread
NikhilA8606 marked this conversation as resolved.
Outdated
<Callout

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You switched to Callout (good), but then override its variant styling with className="border border-amber-300 bg-amber-50 text-amber-800" — hardcoded amber utilities rather than the design tokens, and the warning variant already provides bg-warning-50 text-warning-700 (tailwind.config.js / react-components.instructions.md: use color tokens, don't hardcode). The in-form Alert does the same amber override. Drop the className and let variant="warning" do its job, so both callouts look identical without duplicating Tailwind soup.

Also: the inner <span className="flex items-center gap-2"> wraps a single text node — Callout already flexes; it can go.

variant="warning"
className="border border-amber-300 bg-amber-50 text-amber-800"
Comment thread
NikhilA8606 marked this conversation as resolved.
badge={
<TriangleAlert className="size-4 shrink-0 text-amber-600" />
}
>
<span className="flex items-center gap-2">
{t("max_datapoints_per_upsert_limit", {
count: careConfig.maxDatapointsPerUpsert,
})}
</span>
</Callout>
Comment thread
NikhilA8606 marked this conversation as resolved.
)}

{/* Add New Supply Delivery Form - Always show when in draft mode */}
{canAddSupplyDeliveries && (
{canAddSupplyDeliveries && !hasReachedUpsertLimit && (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the parent unmounts the form entirely once the limit is hit... which means that shiny new item_limit_reached Alert you added inside AddSupplyDeliveryForm can never render. Two limit warnings, one of which is unreachable dead code. Pick one owner of this state.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When hasReachedUpsertLimit is true the parent unmounts AddSupplyDeliveryForm entirely, so the item_limit_reached Alert inside that form (AddSupplyDeliveryForm.tsx ~912) is unreachable in the supplyDeliveriesCount >= max case — it can only ever fire on the + fields.length overshoot. Two limit warnings with two different formulas, one mostly dead.

Simpler: keep the form mounted and let it own the limit UI (it already has supplyDeliveriesCount), or drop the in-form Alert and keep only this Callout. Right now a user who hits the cap also loses the ability to see/edit rows they had in progress.

Comment thread
NikhilA8606 marked this conversation as resolved.
Comment thread
NikhilA8606 marked this conversation as resolved.
<AddSupplyDeliveryForm
deliveryOrderId={deliveryOrderId}
facilityId={facilityId}
origin={deliveryOrder.origin?.id}
destination={deliveryOrder.destination.id}
onSuccess={handleSupplyDeliverySuccess}
supplyDeliveriesCount={
Comment thread
NikhilA8606 marked this conversation as resolved.
supplyDeliveries?.results.length || 0
Comment thread
NikhilA8606 marked this conversation as resolved.
}
/>
)}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -467,8 +467,9 @@ export function AddMedicationReturnItemForm({
variant="secondary"
onClick={loadFromMedicationDispenses}
>
{t("load_from_order")} ({medicationDispenses.length}{" "}
{t("items")})
{t("load_from_order_with_items", {
count: medicationDispenses.length,
})}
<ShortcutBadge actionId="load-from-order" />
</Button>
)}
Expand Down Expand Up @@ -506,8 +507,9 @@ export function AddMedicationReturnItemForm({
variant="outline_primary"
onClick={loadFromMedicationDispenses}
>
{t("load_from_order")} ({medicationDispenses.length}{" "}
{t("items")})
{t("load_from_order_with_items", {
count: medicationDispenses.length,
})}
<ShortcutBadge actionId="load-from-order" />
</Button>
<p>- {t("or")} -</p>
Expand Down
Loading
Loading