[ENG-887] Fix internal and external delivery batch failure and refine supply delivery limit UI - #16661
[ENG-887] Fix internal and external delivery batch failure and refine supply delivery limit UI#16661NikhilA8606 wants to merge 19 commits into
Conversation
Entire-Checkpoint: 574e6f6865af
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a configurable maximum of 100 datapoints per upsert request by default. Delivery-order queries and forms enforce the limit. Warning callouts and item-count labels use updated shared presentation and localization. ChangesConfigurable upsert limit and warning UI
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moving setSelectedDeliveries([]) into the mutation's onSuccess — fine, actually an improvement, clears selection only after confirmed success instead of optimistically. The maxDatapointsPerUpsert config plumbing (env var, careConfig, validate-env schema) is clean and consistent with the existing patterns.
But the actual enforcement is half-baked: exceedsUpsertLimit only disables the "mark_as_completed" button. The "receive_update_stock" button and the "mark_as_abandoned"/"mark_as_damaged" dropdown items call the same upsertSupplyDeliveries mutation and aren't gated at all — so the whole point of adding this limit is trivially bypassed. Also no user-facing feedback when the limit is hit; a disabled button with no explanation is a UX dead end. Not blocking, but please fix before this ships — otherwise what was the point?> Generated by Grumpy PR Reviewer for #16661 · auto · 25.6 AIC · ⌖ 2.21 AIC · ⊞ 8.7K
| return; | ||
| } | ||
|
|
||
| upsertSupplyDeliveries({ |
There was a problem hiding this comment.
handleMarkAsAbandoned/handleMarkAsDamaged/handleSubmitDialog fire the upsert with zero regard for maxDatapointsPerUpsert. You built a limit check but only wired it into one disabled prop — the actual guard belongs in these handlers too (early-return with a toast), otherwise it's just decoration on a single button.
There was a problem hiding this comment.
Pull request overview
This PR introduces a configurable cap for the maximum number of datapoints sent in a single “upsert” request and starts wiring that limit into the internal inventory delivery-order UI to avoid backend batch failures (ENG-887).
Changes:
- Adds
REACT_MAX_DATAPOINTS_PER_UPSERTenv support (typing, validation, example env) and exposes it viacareConfig.maxDatapointsPerUpsert. - Updates
DeliveryOrderShowto use the new config and adjusts selection clearing to happen on successful upsert. - Adds a UI flag (
exceedsUpsertLimit) intended to prevent invalid oversized operations.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/vite-env.d.ts |
Adds TypeScript typing for REACT_MAX_DATAPOINTS_PER_UPSERT. |
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx |
Introduces upsert-limit flag usage and changes when selection state is cleared. |
scripts/validate-env.ts |
Adds env schema entry for REACT_MAX_DATAPOINTS_PER_UPSERT. |
care.config.ts |
Adds maxDatapointsPerUpsert config with default 100. |
.example.env |
Documents and provides example value for the new env var. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/validate-env.ts`:
- Line 173: Replace the numberAsString schema for
REACT_MAX_DATAPOINTS_PER_UPSERT with validation that accepts only positive
integer values and rejects decimals, exponent notation, trailing characters, and
other malformed strings; preserve its optional behavior.
In
`@src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx`:
- Around line 257-259: Enforce the configured datapoint limit at every
supply-delivery upsert boundary, not only in the completion-button condition.
Add the limit check to the receive, abandon, damaged, and confirmation controls,
and re-check it immediately before calling upsertSupplyDeliveries in
handleMarkAsAbandoned, handleMarkAsDamaged, and handleSubmitDialog; preserve the
existing behavior for valid selections within the limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89d1c966-b385-490e-a58c-fa341b09ed23
📒 Files selected for processing (5)
.example.envcare.config.tsscripts/validate-env.tssrc/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsxsrc/vite-env.d.ts
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10983 |
rithviknishad
left a comment
rithviknishad
left a comment
There was a problem hiding this comment.
now that we have a baseline value of what we limit, we could set a a limit of 100 in the query, so that we don't need like 7 API calls to fetch ~100 deliveries. we could optimize our query to return in one API call instead of waiting to loop through all pages.
ohc-project-manager
Bot
added
Changes Required
and removed
needs review
needs testing
labels
rithviknishad
left a comment
rithviknishad
left a comment
There was a problem hiding this comment.
this still fails... why don't we disable the button now that we know the upsert limit?
should we think of pre-selcting first 100 items only?
should we give a hint that this order contains more than 100 items?
… allowed limit Entire-Checkpoint: 719ada336ce6
Deploying care-preview with
|
| Latest commit: |
dcc43a2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5db34ffa.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-887-fix-internal-deliver.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
This is a draft PR ("Changes Required"/"Work in Progress" labels), so I'm not surprised there are loose ends. The env var plumbing and i18n additions are fine — that part's competent. But the frontend limit-checking logic is inconsistent across three places (page-length vs. total-count vs. count+in-form-fields), which will cause the button-disabled state and the warning banner to disagree in edge cases. Tighten that up into one shared derivation before this leaves draft. Also worth double-checking: does the backend actually reject requests over the limit with a clear error, or does the frontend cap silently mask a server 400? Not blocking, just tidy it up.> Generated by Grumpy PR Reviewer for #16661 · auto · 20.1 AIC · ⌖ 2.18 AIC · ⊞ 8.7K
The Mark as Complete button was enabled even when the upsert request failed. Based on the discussion with @amjithtitus09, we initially decided to disable the button in that case. However, after the recent discussion with you and @amjithtitus09, I restricted users from creating requests more than limit by adding a callout card on the request creation page. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/CAREUI/display/Callout.tsx:33
Calloutwrapschildrenin a<span>, which creates invalid HTML when callers pass block elements (e.g., a<p>inEditScheduleTemplateSheet). This can break layout and accessibility; render children in a block container instead of an inline span.
<span className="font-medium">{props.badge}</span>
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx:57
- Import order:
lucide-reactis a 3rd‑party import but it is currently placed after local@/types/...imports. This deviates from the repo import ordering convention (AGENTS.md:35) and may get auto-reordered by formatting/linting.
import scheduleApis from "@/types/scheduling/scheduleApi";
import { Info } from "lucide-react";
public/locale/en.json:3182
load_from_order_with_itemswill render as "1 items" when count is 1. The code already passescount, so this should use i18next plural forms (_one/_other) like other keys in this file (e.g.age_days_one/other).
"load_from_order_with_items": "Load from order ({{count}} items)",
public/locale/en.json:3062
item_limit_reachedis added to the English locale but is not referenced anywhere in the codebase. Unused i18n keys add noise and make it harder to manage translations.
"item_condition": "Item Condition?",
"item_limit_reached": "Item limit reached",
"item_location": "Item Location",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/CAREUI/display/Callout.tsx:33
- The Callout badge change is described as a type update (string → ReactNode), but this change also removes the badge container styling (rounded/bordered pill). That will alter the appearance of all existing callouts and pushes styling responsibility to each caller. Consider keeping the pill wrapper and rendering the ReactNode inside it so the visual design stays consistent while still supporting icons/custom nodes.
<span className="font-medium">{props.badge}</span>
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx:57
- The
lucide-reactimport is currently placed after internal@/imports. In nearby files (e.g.,EditScheduleTemplateSheet.tsx:1-10), third-party imports (includinglucide-react) are grouped at the top before local module imports; keeping the same ordering avoids churn and potential lint/format noise.
SchedulableResourceType,
ScheduleAvailabilityCreateRequest,
} from "@/types/scheduling/schedule";
import scheduleApis from "@/types/scheduling/scheduleApi";
import { Info } from "lucide-react";
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/AddSupplyDeliveryForm.tsx:916
- The upsert-limit UI prevents adding rows one-by-one, but it can still be bypassed by “Load from order”:
handleSelectRequestssets the full selected request list intoitemswithout enforcing the remainingmaxDatapointsPerUpsertcapacity. That allows the form to end up with more than the configured maximum items and defeats the intended guardrail.
{supplyDeliveriesCount + fields.length >=
careConfig.maxDatapointsPerUpsert ? (
<Callout
variant="warning"
className="border border-amber-300 bg-amber-50 text-amber-800"
Entire-Checkpoint: 00ca5a250fd0
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/CAREUI/display/Callout.tsx:33
Calloutwrapschildrenin a<span className="font-medium">, but several new call sites pass block-level content (e.g.<p>and<Trans>). This produces invalid HTML (<span><p>…</p></span>) and also forces medium font-weight on all content, which is hard to override from call sites.
<span className="font-medium">{props.badge}</span>
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/AddSupplyDeliveryForm.tsx:250
disableAddItemonly considers existingsupplyDeliveriesCount, so the UI can still allow adding items beyond the upsert limit within the current form session (viafields.length). This defeats the intended guardrail when users keep adding rows before saving.
const hasReachedUpsertLimit =
supplyDeliveriesCount >= careConfig.maxDatapointsPerUpsert;
const disableAddItem =
isProcessing || hasReachedUpsertLimit || isFetchingSupplyDeliveries;
public/locale/en.json:3182
load_from_order_with_itemsalways renders “{{count}} items”, which leads to incorrect grammar forcount = 1("1 items"). Use i18next plural forms (_one/_other) so the UI stays correct for all counts.
"load_from_order_with_items": "Load from order ({{count}} items)",
care.config.ts:412
- Minor grammar in the new config doc: “set with whatever backend sets” reads incorrectly and is a bit unclear.
* This should be set with whatever backend sets.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/AddSupplyDeliveryForm.tsx:948
- The visible button label now includes the item count (e.g., "Load from order (N items)"). Existing Playwright tests in
tests/facility/services/locations/inventory/toDispatch.spec.tsusegetByRole(..., { name: "Load from order" }), which will no longer match and will fail CI unless the tests/selectors are updated (or a stable accessible name is provided).
>
{t("load_from_order_with_items", {
count: supplyRequests.count,
})}
<ShortcutBadge actionId="load-from-order" />
src/CAREUI/display/Callout.tsx:33
Calloutwrapschildrenin a<span>, which produces invalid HTML when callers pass block-level elements (e.g.,<p>in the scheduling sheets). Rendering block content inside a span can break layout and accessibility; renderchildrenwithout forcing an inline wrapper.
<span className="font-medium">{props.badge}</span>
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
care.config.ts:413
- The config comment is unclear/grammatically incorrect ("set with whatever backend sets"). Please clarify that this value should match the backend limit so it’s obvious how to configure it safely.
/**
* Maximum number of datapoints allowed in a single upsert request.
* This should be set with whatever backend sets.
*/
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx:881
- This warning is shown when
selectedDeliveries.lengthexceeds the upsert limit (i.e., the user is trying to process too many deliveries at once), but the i18n string says "You cannot add more than {{count}} items in a single delivery." This can confuse users because they are not adding items here. Consider using a separate i18n key/message for selection/upsert limits (or making the wording generic enough for both contexts).
{!isWithinUpsertLimit && (
<div className="px-4">
<Callout
variant="warning"
className="border border-amber-300 bg-amber-50 text-amber-800"
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>
Entire-Checkpoint: eab7388455ac
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx:491
hasReachedUpsertLimitis derived from the total number of existingsupplyDeliveries(results.length). However,careConfig.maxDatapointsPerUpsertis documented as a per-request limit (seecare.config.ts), and the UI copy says the user cannot add/update more than N in a single request. Using the total existing deliveries to block/hide create UI will incorrectly prevent users from adding deliveries in multiple batches once an order exceeds the limit.
Consider removing the hasReachedUpsertLimit checks that are based on supplyDeliveries.results.length and enforce the limit only on the request payload size (e.g. the number of items being submitted / the number of selected deliveries being upserted).
const hasReachedUpsertLimit =
supplyDeliveries &&
supplyDeliveries.results.length >= careConfig.maxDatapointsPerUpsert;
const isWithinUpsertLimit =
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/AddSupplyDeliveryForm.tsx:914
- This limit check adds
supplyDeliveriesCount(existing deliveries in the order) tofields.length(items in the current create request). The backend limit being enforced here is per request payload size, so existing deliveries should not affect whether the user can add another item to the current request.
{supplyDeliveriesCount + fields.length >=
careConfig.maxDatapointsPerUpsert ? (
<Callout
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx:284
- The supplyDeliveries query runs unconditionally. If
deliveryOrderIdis temporarily undefined (or falsy during routing transitions), this will issue a request with an invalidorderquery param and can produce avoidable 4xx/network noise. The previousenabled: !!deliveryOrderIdguard should be kept.
This issue also appears on line 487 of the same file.
const {
data: supplyDeliveries,
isLoading: isLoadingSupplyDeliveries,
isFetching: isFetchingSupplyDeliveries,
} = useQuery({
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/AddSupplyDeliveryForm.tsx:250
hasReachedUpsertLimitis computed fromsupplyDeliveriesCount(deliveries already present in the order). That will disable adding items to a new delivery request as soon as the order has >= N deliveries, even if the current request only contains a few items. This should be based on the number of items being submitted in the current request (fields.length/data.items.length).
This issue also appears on line 912 of the same file.
const hasReachedUpsertLimit =
supplyDeliveriesCount >= careConfig.maxDatapointsPerUpsert;
const disableAddItem =
isProcessing || hasReachedUpsertLimit || isFetchingSupplyDeliveries;
public/locale/en.json:3409
- This message says "in a single delivery", but the key is used for enforcing a per-request limit (both for creating/upserting). The wording should reflect a request/action limit rather than a delivery entity.
"max_datapoints_per_upsert_limit": "You cannot add more than {{count}} items in a single delivery.",
src/CAREUI/display/Callout.tsx:33
- Wrapping
childrenin a<span>can produce invalid HTML when callers pass block elements (e.g.<p>), which can affect semantics and accessibility. Renderchildrendirectly in the block container instead.
(Separately: removing the old badge wrapper changes the visual styling for string badges across the app; if that wasn’t intentional, consider reintroducing the wrapper with a ReactNode-friendly implementation.)
<span className="font-medium">{props.badge}</span>
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
Entire-Checkpoint: 5c90ecdb25bd
There was a problem hiding this comment.
Sighs, puts down coffee.
The intent here is fine — gating "Mark as Complete" and stopping users from blowing past the backend upsert limit is genuinely worth doing. The execution has a few holes that will bite in production:
every()on an empty array returnstrue. A delivery order with zero deliveries will happily report "all completed or abandoned" and re-enable the very button you were trying to gate.enabled: !!deliveryOrderIdwas silently dropped from thesupplyDeliveriesquery while addingpageSize.isWithinUpsertLimitis dead logic. You capped the page size at the limit, then check whether a selection derived from that page exceeds the limit. It can't. The Callout it guards will never render.- Off-by-one on the limit checks in both
AddItemsFormandAddSupplyDeliveryForm— users are blocked one item early. - Copy-paste. The identical amber
Callout+TriangleAlertblock appears three times, with hardcoded colour classes that arguably belong inside thewarningvariant you just gutted.
Also: the i18n string says "in a single delivery" but is reused for supply requests, and there are no tests covering the new gating logic despite the checklist item sitting unticked.
Fix 1–3 before merging; the rest is housekeeping. Not blocking, but don't pretend you didn't read this.> Generated by Grumpy PR Reviewer for #16661 · opus50 · 54.4 AIC · ⌖ 2.14 AIC · ⊞ 8.7K
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/CAREUI/display/Callout.tsx:33
Calloutnow allowsbadgeto be anyReactNode, but it renders bothbadgeandchildreninside<span>elements. This can produce invalid DOM nesting (e.g., ifbadge/childrencontains a<div>/<p>/other block element) and can triggervalidateDOMNestingwarnings. Prefer rendering these nodes inside<div>containers and avoid wrappingchildrenin a<span>.
<span className="font-medium">{props.badge}</span>
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
There was a problem hiding this comment.
CARE Review — cap supply-delivery/request batches at a configurable upsert limit
What it does: adds maxDatapointsPerUpsert (env-configurable, default 100), uses it as the query.paginated page size for supply deliveries, and gates the add/load controls plus the bulk status actions on it. Separately: "Mark as Completed" now also requires every delivery to be completed or abandoned, and Callout's badge becomes a ReactNode so call sites can pass icons.
The direction is right — one config value, derived in the places that need it, rather than a magic number. Three things are worth fixing before merge.
Correctness
allSupplyDeliveriesCompletedOrAbandoneduseseveryon a possibly-empty array, so an order with zero deliveries enables "Mark as Completed" — the inverse of the stated intent.isWithinUpsertLimit(<=) andhasReachedUpsertLimit(>=) disagree at exactlymax. One of them is off by one; the copy ("cannot add more than N") says which.enabled: !!deliveryOrderIdwas dropped from the deliveries query, seemingly incidentally.
Legibility / single source of truth
AddSupplyDeliveryFormcomputes the limit two different ways in one component. Because the parent already unmounts the form at the limit, the version wired intodisableAddItemcan never be true — so the buttons and the in-form Callout can disagree. Derive it once.
Approach
handleSelectRequests("select all" →setValue("items", ...)) is the bulk path that caused the original batch failure and is the one place with no cap. Everything else added here guards single-row entry.- The
Calloutchange removes variant-derived badge styling from a shared CAREUI primitive, and every call site in this PR now hardcodes a colour to compensate. Aniconprop that maps colour fromvariantwould keep the abstraction doing its job.
The amber classes on the callouts (border-amber-300 bg-amber-50 text-amber-800) are redundant with variant="warning", which already resolves to the same amber ramp via --color-warning-*. Dropping the overrides would let the token do the work — minor, and I've not commented inline on it.
Note: several earlier bot threads on this PR claimed the page-size/total-count mismatch was a bug. query.paginated loops until all pages are fetched, so results.length === count here; those look moot and I've not repeated them.
Generated by CARE PR Reviewer for #16661 · opus50 · 269.4 AIC · ⌖ 3.19 AIC · ⊞ 19.4K
Comments that could not be inline-anchored
src/CAREUI/display/Callout.tsx:30
Deleting the badge pill removes the only variant-aware styling the badge had, for every Callout in the app — and it's a shared CAREUI primitive. The knock-on effect is visible in this same PR: each call site now hardcodes the colour the component used to derive (text-purple-500, text-warning-700, text-amber-600), which is exactly what the variant prop existed to avoid.
A smaller change that keeps the contract: keep the variant map and render the badge unwrapped only when it isn't a s…
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx:492
isWithinUpsertLimit uses <=, so at exactly maxDatapointsPerUpsert selected deliveries the buttons stay enabled and the request goes out with count === max. Meanwhile the sibling gate two lines up uses >= (hasReachedUpsertLimit), and the copy says "You cannot add more than {{count}} items". Two boundaries in the same block that disagree about whether max is allowed.
If the backend cap is "at most N per request" then <= is right and hasReachedUpsertLimit should be > — pick …
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx:474
Array.prototype.every returns true for an empty array, so when the order has zero supply deliveries this evaluates to true and "Mark as Completed" is enabled — the opposite of the PR's stated intent ("enabled only when all supply deliveries are either completed or abandoned").
The ?? false only guards supplyDeliveries being nullish, not results being empty. Needs a length check as well:
const allSupplyDeliveriesCompletedOrAbandoned =
!!supplyDeliveries?.results.length …
</details>
<details><summary>src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx:281</summary>
`enabled: !!deliveryOrderId` was dropped along with the `pageSize` addition. That looks incidental rather than intended — the query will now fire even when `deliveryOrderId` is falsy, issuing a request with `order: undefined`. If it was removed deliberately (because the route guarantees the id), worth saying so; otherwise put it back, it's unrelated to the pageSize change.
</details>
<details><summary>src/pages/Facility/services/inventory/externalSupply/deliveryOrder/AddSupplyDeliveryForm.tsx:245</summary>
Two formulas for the same limit in one component: `hasReachedUpsertLimit` (drives `disableAddItem` on all four add/load buttons) counts only `supplyDeliveriesCount`, while the render gate at ~line 912 counts `supplyDeliveriesCount + fields.length`. Since the parent only mounts this form when `supplyDeliveriesCount < max`, `hasReachedUpsertLimit` is effectively always `false` here — so "Add another" and "Load from order" stay enabled right up until the row-count gate swaps them out.
Derive one …
</details>
<details><summary>src/pages/Facility/services/inventory/externalSupply/deliveryOrder/AddSupplyDeliveryForm.tsx:306</summary>
`handleSelectRequests` does `form.setValue("items", itemsFromRequests)` with the full selection — this is the bulk path that produced the original batch failure, and it's the one path with no limit check. "Select all" on an order with more than `maxDatapointsPerUpsert` requests loads them all, and `onSubmit` then fires them. The disabled props you added guard entering the dialog, not what comes out of it.
A cap here (truncate + toast, or block confirm with the count) would make the limit actua…
</details>
<details><summary>src/pages/Facility/services/inventory/externalSupply/requestOrder/AddItemsForm.tsx:222</summary>
When the limit is hit you replace `ProductKnowledgeSelect` with the Callout, and `-{t("or")}-` gets `hidden` — but the block below still offers the alternate entry path, so the user sees a "cannot add more" warning next to controls that still add items. Disabling the select (`disabled={hasReachedUpsertLimit}`) and rendering the Callout *alongside* it keeps the layout stable and the state honest, rather than swapping controls out and then patching the leftover separator with `hidden`.
</details>
<details><summary>public/locale/en.json:3061</summary>
`item_limit_reached` doesn't appear to be referenced anywhere in the PR's changed files — the callouts all use `max_datapoints_per_upsert_limit`. If it's leftover from an earlier iteration, drop it; `npm run knip` in CI flags unused keys.
</details>Entire-Checkpoint: f0933eca5b3d
There was a problem hiding this comment.
Generated by Grumpy PR Reviewer for #16661 · opus50 · 62.3 AIC · ⌖ 2.12 AIC · ⊞ 8.7K
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/pages/Facility/services/inventory/externalSupply/requestOrder/RequestOrderShow.tsx:760
supplyRequestsCountis currently derived fromsupplyRequests.results.length, which only reflects the current page size of the paginated response. This can undercount existing items and allow users to exceedmaxDatapointsPerUpsertonce there are multiple pages of supply requests.
supplyRequestsCount={
supplyRequests?.results.length || 0
}
public/locale/en.json:3409
- The message for
max_datapoints_per_upsert_limitmentions "in a single delivery", but this key is used in multiple contexts (adding request items, adding deliveries, bulk status updates). The wording should be generic so it remains accurate everywhere it’s shown.
"max_datapoints_per_upsert_limit": "You cannot add more than {{count}} items in a single delivery.",
care.config.ts:413
- The doc comment for
maxDatapointsPerUpsertsays it is a limit for a "single upsert request", but it’s also used to cap total items/deliveries shown/added in UI flows (and as a page size). This is misleading for operators configuring the env var.
/**
* Maximum number of datapoints allowed in a single upsert request.
* This should be set with whatever backend sets.
*/
There was a problem hiding this comment.
CARE Review — gating "Mark as Completed" on delivery status, plus upsert-limit UI in the request-order item form
Re-review. Scope is the 7 commits since my last pass on 2026-08-14 (0ba9beac, 5392e1a7, a1b51bcd, 66c7c291, 78188bef, 9445a5c3, e09e007d).
Prior findings — both confirmed fixed. The pageSize vs limit issue on the deliveries query (9432850f) and the disableAddItem hoist (2692ceb7) are resolved; both threads closed.
Intent
Two distinct changes in this delta, both legible:
Mark as Completedon a delivery order is now gated on every delivery having reachedcompletedorabandoned, so an order can no longer be closed while deliveries are still in flight.e09e007dcorrectly tightened this so an empty list does not satisfyevery(). The Playwright update in78188beffollows from the gate rather than papering over it — a real receive step now precedes the completion assertion. Good.- The upsert cap is surfaced in
AddItemsFormthe way it already is inAddSupplyDeliveryForm, and5392e1a7extends the existing cap check tomark_as_abandoned/mark_as_damaged— those bulk-mutate the same way the Receive button does, so the rename toisWithinUpsertLimitand the wider application both read correctly.
Open items
DeliveryOrderShow.tsx:479—entered_in_erroris a fourthSupplyDeliveryStatusand is not accounted for, so one errored delivery can permanently block completing the order. This is the one I would not merge without an answer.AddItemsForm.tsx:91/:222— the>=boundary blocks the last allowed item, and hiding the selector rather than disabling it leaves no way to back out once the limit trips. Also worth confirmingmaxDatapointsPerUpsertis even the right cap here, since this form POSTs onecreateSupplyRequestper row rather than using the upsert endpoint.
Retraction
I left a comment on DeliveryOrderShow.tsx:493 claiming isWithinUpsertLimit is always true because the query is capped at pageSize. That is wrong — please disregard it. query.paginated loops until items.length >= res.count (src/Utils/request/query.ts:170), so pageSize only sets the per-request size; results is the full set and can exceed the cap. The guard is live and the rename is correct.
Nothing to raise on the approach lens — the changes reuse the existing limit/callout pattern rather than inventing anything, and the diff stays proportionate.
Not re-raising the Callout badge/amber-token and boundary discussions already settled in other threads.
Generated by CARE PR Reviewer for #16661 · opus50 · 414.5 AIC · ⌖ 4.62 AIC · ⊞ 19.4K
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx:480
allSupplyDeliveriesCompletedOrAbandonedis forced tofalsewhen there are 0 supply deliveries due to the!!...lengthguard, which keeps “Mark as Completed” disabled even though the PR description says it should be enabled when all deliveries are completed or abandoned (an empty list should satisfy that condition).
const allSupplyDeliveriesCompletedOrAbandoned =
!!supplyDeliveries?.results?.length &&
supplyDeliveries.results.every(
(delivery) =>
delivery.status === SupplyDeliveryStatus.completed ||
src/CAREUI/display/Callout.tsx:33
- The Callout badge styling (bordered pill with variant-specific border color) was removed. If the intent was only to change the badge type to
ReactNode, consider preserving the existing badge container so current Callout layouts don’t regress visually.
<span className="font-medium">{props.badge}</span>
<div className="flex-1">
<span className="font-medium">{props.children}</span>
</div>
public/locale/en.json:3461
- This message is shown in both request-order item addition and delivery item addition flows; referencing “single delivery” is misleading in the request-order context. Consider making it generic to the upsert/request limit.
"max_datapoints_per_upsert_limit": "You cannot add more than {{count}} items in a single delivery.",
Entire-Checkpoint: 574e6f6865af
Changes
Fix: ENG-887
Screen.Recording.2026-08-10.at.6.07.34.PM.mov
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation