[ENG-613] Added expiry date to supply delivery table - #16579
Conversation
|
|
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 (1)
WalkthroughChangesSupply delivery expiry display
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Deploying care-preview with
|
| Latest commit: |
fc52de5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eabcbd3b.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://supply-delivery-expiry-date.care-preview-a7w.pages.dev |
Greptile SummaryThis PR adds an Expiry column to the
Confidence Score: 5/5Safe to merge — the change is a straightforward column addition with no logic side-effects and no impact on existing columns or data mutations. The change only adds a read-only display column. The translation key exists, date parsing uses the correct parseISO helper, column alignment between header and body is correct, and both data sources for the expiry value are properly guarded with optional chaining. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "care-loop: address review feedback (roun..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Adds an Expiry column to the facility inventory Supply Delivery table so users can see batch expiry dates alongside delivered items.
Changes:
- Added an “Expiry” table header to the Supply Delivery table.
- Rendered expiry date per row (preferring
supplied_inventory_item.product.expiration_date, falling back tosupplied_item.expiration_date) and formatted it asdd/MM/yyyywith-when missing.
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 `@src/pages/Facility/services/inventory/SupplyDeliveryTable.tsx`:
- Around line 290-299: Update the expiry rendering in SupplyDeliveryTable to
parse the serialized date-only expiration_date with date-only-safe parsing such
as parseISO before formatDate, avoiding timezone-based day shifts. Add a
regression test covering a date-only expiry value and verify the displayed date
remains unchanged.
- Around line 290-299: Update the expiry selection in the TableCell rendering to
use the same internal-branch condition as the product-name logic, choosing the
inventory product expiration for internal deliveries and the supplied-item
expiration for external deliveries. Preserve the existing date formatting and
"-" fallback.
🪄 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: f43e77b2-63b8-4657-8505-be162c7e4c87
📒 Files selected for processing (1)
src/pages/Facility/services/inventory/SupplyDeliveryTable.tsx
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10556 |
There was a problem hiding this comment.
Fine. 13 lines changed, one column added, parseISO correctly used for ISO string dates, fallback to "-" is appropriate. The logic is sound.
One gripe (see inline): ditch the IIFE, extract expiry to a plain const before the return. That's literally all I ask. Is that too much? Apparently.
Also — the PR description says "tests skipped — trivial change." It is trivial, so fine. At least someone is honest.
Generated by Grumpy PR Reviewer for issue #16579 · 20.8 AIC · ⌖ 6 AIC · ⊞ 6.3K
| return expiry | ||
| ? formatDate(parseISO(expiry), "dd/MM/yyyy") | ||
| : "-"; | ||
| })()} |
There was a problem hiding this comment.
An IIFE inside JSX? Really? In 2026? Just extract expiry to a const before the return statement — you know, like a normal human. The entire IIFE exists to derive one variable; that's what const is for.
const expiry =
delivery.supplied_inventory_item?.product?.expiration_date ||
delivery.supplied_item?.expiration_date;
// ...
<TableCell>
{expiry ? formatDate(parseISO(expiry), "dd/MM/yyyy") : "-"}
</TableCell>Cleaner, readable, no gymnastics required.
Changes
Added expiry date to supply delivery table
Tests skipped — trivial change.
Summary by CodeRabbit