Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
WalkthroughAdds supply delivery viewing functionality to the inventory system through a new drawer component, integrates delivery links into the inventory list, updates the supply delivery table to support product linking via order type identification, and extends the supply delivery type definition to include order information. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Deploying care-preview with
|
| Latest commit: |
ba93eda
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9f3d12c8.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://care-78-when-a-user-click-on.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve navigation from the Inventory summary view into the relevant Delivery Order context by introducing a deliveries drawer and updating how supply-delivery items link out to related pages.
Changes:
- Extend
SupplyDeliveryReadto include the associatedorderobject. - Add a reusable
SupplyDeliveriesDrawerand wire it intoInventoryList(open drawer by clicking a product name). - Update
SupplyDeliveryTableto link the product name to a delivery-order route and move the product-settings link to an external-link icon; add i18n for the drawer title.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/inventory/supplyDelivery/supplyDelivery.ts | Adds order: DeliveryOrderRetrieve to supply delivery read model. |
| src/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsx | Simplifies rendering by removing an unnecessary fragment wrapper. |
| src/pages/Facility/services/inventory/SupplyDeliveryTable.tsx | Adds delivery-order navigation link(s) from delivery rows and helper to infer order type. |
| src/pages/Facility/services/inventory/SupplyDeliveriesDrawer.tsx | New drawer component that fetches and displays supply deliveries in a table. |
| src/pages/Facility/services/inventory/InventoryList.tsx | Adds “deliveries for product” drawer entry point from inventory list rows. |
| public/locale/en.json | Adds deliveries_for i18n string and reorders an existing confirmation string. |
| <SupplyDeliveriesDrawer | ||
| open={!!deliveriesDrawerProduct} | ||
| onOpenChange={(open) => { | ||
| if (!open) setDeliveriesDrawerProduct(undefined); | ||
| }} | ||
| title={ | ||
| deliveriesDrawerProduct | ||
| ? t("deliveries_for", { product: deliveriesDrawerProduct.name }) | ||
| : undefined | ||
| } | ||
| facilityId={facilityId} | ||
| queryParams={{ | ||
| destination: locationId, | ||
| supplied_inventory_item_product_knowledge: | ||
| deliveriesDrawerProduct?.id, | ||
| ordering: "-created_date", | ||
| }} | ||
| linkToProduct | ||
| /> |
There was a problem hiding this comment.
This adds a new user-facing navigation flow (Inventory summary -> open deliveries drawer -> navigate into a delivery order). There are existing Playwright specs covering inventory receive/dispatch flows, but none appear to cover this new drawer + navigation path. Please add/update a Playwright spec to exercise opening the drawer from the inventory list and following the delivery-order link (at least one internal and one external case if applicable).
| </Link> | ||
| <div className="flex items-center gap-2"> | ||
| <Link | ||
| href={`/facility/${facilityId}/locations/${delivery.order.destination.id}/inventory/${identifyOrderType(delivery)}/deliveries/incoming/${delivery.order.id}`} |
There was a problem hiding this comment.
The generated delivery-order link path is incorrect for internal deliveries. Location routes expect /inventory/internal/:type/deliveries/:id (e.g. .../internal/receive/deliveries/{id}), but this builds /inventory/internal/deliveries/incoming/{id}, which won’t match any route and will 404 for internal orders. Build the href using the existing inventory route helper (or include the required :type segment based on receive/dispatch) so internal navigation resolves correctly.
| href={`/facility/${facilityId}/locations/${delivery.order.destination.id}/inventory/${identifyOrderType(delivery)}/deliveries/incoming/${delivery.order.id}`} | |
| href={ | |
| internal | |
| ? `/facility/${facilityId}/locations/${delivery.order.destination.id}/inventory/internal/receive/deliveries/${delivery.order.id}` | |
| : `/facility/${facilityId}/locations/${delivery.order.destination.id}/inventory/${identifyOrderType(delivery)}/deliveries/incoming/${delivery.order.id}` | |
| } |
| <Link | ||
| href={`/facility/${facilityId}/locations/${delivery.order.destination.id}/inventory/${identifyOrderType(delivery)}/deliveries/incoming/${delivery.order.id}`} | ||
| className="font-medium text-primary-600 hover:underline" | ||
| basePath="/" | ||
| > | ||
| {productName} | ||
| </Link> |
There was a problem hiding this comment.
This <Link> is inside a <TableCell> that has an onClick handler; unlike the settings-link, it does not stop event propagation. Clicking the product name can therefore also trigger onDeliveryClick (and potentially navigate twice / to the wrong place). Add e.stopPropagation() on the product-name link click as well when onDeliveryClick is in use.
| if (linkToProduct && facilityId && productId) { | ||
| return ( | ||
| <Link | ||
| href={`/facility/${facilityId}/settings/product/${productId}`} | ||
| className="font-medium text-primary-600 hover:underline" | ||
| onClick={(e) => e.stopPropagation()} | ||
| basePath="/" | ||
| > | ||
| {productName} | ||
| </Link> | ||
| <div className="flex items-center gap-2"> | ||
| <Link | ||
| href={`/facility/${facilityId}/locations/${delivery.order.destination.id}/inventory/${identifyOrderType(delivery)}/deliveries/incoming/${delivery.order.id}`} | ||
| className="font-medium text-primary-600 hover:underline" | ||
| basePath="/" | ||
| > | ||
| {productName} | ||
| </Link> | ||
| <Link | ||
| href={`/facility/${facilityId}/settings/product/${productId}`} |
There was a problem hiding this comment.
linkToProduct is now effectively controlling whether the product-name navigates to the delivery order (with a separate icon linking to the product settings). This makes the prop name misleading for callers and future maintenance. Consider renaming/splitting this prop so its name reflects the current behavior (e.g. one prop for “link product name to order”, another for “show product settings link”).
| return ( | ||
| <div className="font-medium text-wrap wrap-break-word"> | ||
| <span className="font-medium text-primary-600 hover:underline"> | ||
| {productName} | ||
| </div> | ||
| </span> |
There was a problem hiding this comment.
When linkToProduct is false, the product name is rendered as a styled <span> that looks like a link (primary color + hover underline) but is not interactive unless the cell itself is clickable. This is misleading for users and a11y tools. Consider rendering link-like styling only when there is an actual navigation target/click handler, or use a real <Link>/<button> with proper semantics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/locale/en.json`:
- Line 1736: The new i18n key "deliveries_for" was inserted in-place instead of
being appended; remove the in-place entry ("deliveries_for": "Deliveries for
{{product}}") and append that key/value pair to the very end of
public/locale/en.json (respecting JSON syntax — add a comma to the previous last
entry if needed and ensure no trailing comma at EOF). Locate the key by
searching for "deliveries_for" and move it to the file's EOF so the repository's
append-only i18n policy is preserved.
In `@src/pages/Facility/services/inventory/SupplyDeliveriesDrawer.tsx`:
- Around line 47-53: The current useQuery call using
query.paginated(supplyDeliveryApi.listSupplyDelivery) only fetches one page and
the UI renders supplyDeliveries.results, truncating history; change to a
paginated/infinite pattern: replace useQuery with useInfiniteQuery (or implement
a helper that fetches all pages) using query.paginated as the queryFn, provide a
getNextPageParam that returns the next page cursor/page number, and in the
render logic replace supplyDeliveries.results with the aggregated list (e.g.,
supplyDeliveries.pages.flatMap(p => p.results) or the helper's combined array)
and add either a "load more" control or an intersection-observer-triggered
fetchNextPage call to load subsequent pages (update any calls to
isLoading/isFetching accordingly and reference useInfiniteQuery,
query.paginated, supplyDeliveryApi.listSupplyDelivery, getNextPageParam,
fetchNextPage, and pages.flatMap to find where to modify).
- Around line 64-80: The current render treats fetch errors as an empty list
because it only checks isLoading and supplyDeliveries.results; update the
rendering logic in SupplyDeliveriesDrawer to explicitly handle an error state:
after isLoading, check for the fetch error (e.g., supplyDeliveriesError or error
returned by the hook that provides supplyDeliveries) and render an ErrorState
(or an EmptyState variant showing the error) with a retry button wired to the
hook's refetch/mutate function; only then check supplyDeliveries?.results.length
to render SupplyDeliveryTable, otherwise render the existing EmptyState;
reference the existing symbols isLoading, supplyDeliveries (results),
SupplyDeliveryTable, EmptyState/TableSkeleton/Truck and the hook's error/refetch
identifiers to locate and implement the change.
🪄 Autofix (Beta)
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: f1be217f-9f1f-48f0-843e-a0fbd11b2b20
📒 Files selected for processing (6)
public/locale/en.jsonsrc/pages/Facility/services/inventory/InventoryList.tsxsrc/pages/Facility/services/inventory/SupplyDeliveriesDrawer.tsxsrc/pages/Facility/services/inventory/SupplyDeliveryTable.tsxsrc/pages/Facility/services/inventory/externalSupply/deliveryOrder/DeliveryOrderShow.tsxsrc/types/inventory/supplyDelivery/supplyDelivery.ts
| "deliveries": "Deliveries", | ||
| "deliveries_approved": "Deliveries approved successfully", | ||
| "deliveries_created": "Deliveries Created", | ||
| "deliveries_for": "Deliveries for {{product}}", |
There was a problem hiding this comment.
Move new i18n key to the end of en.json (append-only policy).
Line 1736 adds a new key in-place. In this repo, new keys in public/locale/en.json should be appended at EOF.
Proposed fix
- "deliveries_for": "Deliveries for {{product}}", "zoom_in": "Zoom In",
"zoom_out": "Zoom Out",
+ "deliveries_for": "Deliveries for {{product}}",
"℞": "℞"As per coding guidelines public/locale/en.json: Append missing i18n keys directly to the end of the public/locale/en.json file without reading i18n files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/locale/en.json` at line 1736, The new i18n key "deliveries_for" was
inserted in-place instead of being appended; remove the in-place entry
("deliveries_for": "Deliveries for {{product}}") and append that key/value pair
to the very end of public/locale/en.json (respecting JSON syntax — add a comma
to the previous last entry if needed and ensure no trailing comma at EOF).
Locate the key by searching for "deliveries_for" and move it to the file's EOF
so the repository's append-only i18n policy is preserved.
| const { data: supplyDeliveries, isLoading } = useQuery({ | ||
| queryKey: ["supplyDeliveriesDrawer", facilityId, queryParams], | ||
| queryFn: query.paginated(supplyDeliveryApi.listSupplyDelivery, { | ||
| queryParams: { facility: facilityId, ...queryParams }, | ||
| }), | ||
| enabled: open && enabled, | ||
| }); |
There was a problem hiding this comment.
Only the first paginated page is rendered; “all deliveries” is not guaranteed.
At Line 49, the API call is paginated, but at Lines 68-74 only the current results page is shown and there is no page state/control or infinite loading. This can truncate delivery history when records exceed page size.
💡 Suggested fix direction
+ const [page, setPage] = useState(1);
const { data: supplyDeliveries, isLoading } = useQuery({
queryKey: ["supplyDeliveriesDrawer", facilityId, queryParams],
queryFn: query.paginated(supplyDeliveryApi.listSupplyDelivery, {
- queryParams: { facility: facilityId, ...queryParams },
+ queryParams: { facility: facilityId, page, ...queryParams },
}),
enabled: open && enabled,
});
+ // Render pagination controls based on supplyDeliveries.count / next / previous
+ // and update `page` via setPage(...)As per coding guidelines: “Handle pagination and infinite scroll patterns appropriately in page components.”
Also applies to: 68-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/services/inventory/SupplyDeliveriesDrawer.tsx` around
lines 47 - 53, The current useQuery call using
query.paginated(supplyDeliveryApi.listSupplyDelivery) only fetches one page and
the UI renders supplyDeliveries.results, truncating history; change to a
paginated/infinite pattern: replace useQuery with useInfiniteQuery (or implement
a helper that fetches all pages) using query.paginated as the queryFn, provide a
getNextPageParam that returns the next page cursor/page number, and in the
render logic replace supplyDeliveries.results with the aggregated list (e.g.,
supplyDeliveries.pages.flatMap(p => p.results) or the helper's combined array)
and add either a "load more" control or an intersection-observer-triggered
fetchNextPage call to load subsequent pages (update any calls to
isLoading/isFetching accordingly and reference useInfiniteQuery,
query.paginated, supplyDeliveryApi.listSupplyDelivery, getNextPageParam,
fetchNextPage, and pages.flatMap to find where to modify).
| {isLoading ? ( | ||
| <TableSkeleton count={3} /> | ||
| ) : supplyDeliveries?.results && | ||
| supplyDeliveries.results.length > 0 ? ( | ||
| <SupplyDeliveryTable | ||
| deliveries={supplyDeliveries.results} | ||
| internal={internal} | ||
| isRequester={isRequester} | ||
| facilityId={facilityId} | ||
| linkToProduct={linkToProduct} | ||
| /> | ||
| ) : ( | ||
| <EmptyState | ||
| icon={<Truck className="size-5 text-primary-600" />} | ||
| title={t("no_deliveries_found")} | ||
| description={t("no_deliveries_found_description")} | ||
| /> |
There was a problem hiding this comment.
API errors are currently shown as “no deliveries found.”
At Line 64 onward, the branch handles loading and empty data, but errors fall through to the empty-state message. That masks backend/network failures and misleads users.
💡 Suggested fix direction
- const { data: supplyDeliveries, isLoading } = useQuery({
+ const { data: supplyDeliveries, isLoading, isError } = useQuery({
...
});
{isLoading ? (
<TableSkeleton count={3} />
+ ) : isError ? (
+ <EmptyState
+ icon={<Truck className="size-5 text-primary-600" />}
+ title={t("deliveries_load_failed")}
+ description={t("deliveries_load_failed_description")}
+ />
) : supplyDeliveries?.results && supplyDeliveries.results.length > 0 ? (
<SupplyDeliveryTable ... />
) : (
<EmptyState ... />
)}As per coding guidelines: “Implement proper loading, error, and success states for data fetching.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/services/inventory/SupplyDeliveriesDrawer.tsx` around
lines 64 - 80, The current render treats fetch errors as an empty list because
it only checks isLoading and supplyDeliveries.results; update the rendering
logic in SupplyDeliveriesDrawer to explicitly handle an error state: after
isLoading, check for the fetch error (e.g., supplyDeliveriesError or error
returned by the hook that provides supplyDeliveries) and render an ErrorState
(or an EmptyState variant showing the error) with a retry button wired to the
hook's refetch/mutate function; only then check supplyDeliveries?.results.length
to render SupplyDeliveryTable, otherwise render the existing EmptyState;
reference the existing symbols isLoading, supplyDeliveries (results),
SupplyDeliveryTable, EmptyState/TableSkeleton/Truck and the hook's error/refetch
identifiers to locate and implement the change.
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #8607 |
Proposed Changes
Fixes #issue_number
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements