feat: add order invoice component#282
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a new ChangesOrder PDF invoice feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant generateInvoicePDF
participant OrderPdf
participant BrowserDOM
Caller->>generateInvoicePDF: order, summit, {logoSrc, theme}
generateInvoicePDF->>OrderPdf: render component to blob
OrderPdf-->>generateInvoicePDF: PDF blob
generateInvoicePDF->>BrowserDOM: createObjectURL, create <a>, trigger download
BrowserDOM-->>generateInvoicePDF: revoke object URL
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/components/__tests__/order-pdf-.test.js`:
- Line 16: The test import is wrong in order-pdf-.test.js: it only brings in
buildOrderPdfRows and OrderPdf from the components barrel, but the suite
actually calls buildRows, and importing via the barrel also pulls in react-dnd
through SortableTable. Update the test to import the order-pdf module directly
and use the matching exported helper name (or import the alias used by the
tests) so buildRows is defined and the react-dnd dependency chain is avoided.
In `@src/components/order-pdf/index.js`:
- Around line 82-101: The item filtering in the order PDF is inconsistent with
the displayed default quantity: the current filter in the item iteration drops
rows when quantity is missing, but the row rendering in the same block (`qty` in
the `form.items` processing) treats missing quantity as 1. Update the
`form.items` filter logic so optional/missing quantities are still included when
they should render as default quantity 1, and keep the balance calculation and
PDF rows aligned with the `rows.push` item mapping.
- Around line 804-805: The previewPDF flow is creating a blob URL and opening it
in a new tab without releasing it afterward, which can leak memory on repeated
previews. Update the URL handling around the object URL creation in previewPDF
so the URL is revoked after the new window/tab has been opened, while keeping
the current open-in-new-tab behavior intact.
- Around line 32-33: The formatDate helper currently calls .format()
unconditionally on the result of epochToMomentTimeZone, which can be falsy for
missing or 0 epochs and crash invoice rendering. Update formatDate to guard the
value returned by epochToMomentTimeZone before formatting, and return a safe
fallback when no valid moment-like value exists. Keep the fix localized to
formatDate so callers of the order PDF rendering path continue working without
changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0cf3870-2447-4bcc-95d2-77eafc9856b4
📒 Files selected for processing (5)
package.jsonsrc/components/__tests__/order-pdf-.test.jssrc/components/index.jssrc/components/order-pdf/index.jswebpack.common.js
There was a problem hiding this comment.
Pull request overview
Adds a reusable “order invoice PDF” component to uicore, enabling apps that consume this library to generate and preview an order invoice PDF (with line items, fees, discounts, payments/refunds, notes, reconciliation, and amount due).
Changes:
- Introduces
OrderPdf+ helpers (buildRows,generateInvoicePDF,previewPDF) undersrc/components/order-invoice-pdf/. - Exposes the new component/helpers via the central
src/components/index.jsexport surface and adds a webpack entrypoint. - Updates
@react-pdf/rendererto v4.x and adds unit tests for row-building and rendering.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
webpack.common.js |
Adds a new webpack entry for the invoice PDF component bundle. |
src/components/order-invoice-pdf/index.js |
Implements PDF layout, row building, and generate/preview helpers. |
src/components/order-invoice-pdf/__tests__/order-invoice-pdf.test.js |
Adds tests for row formatting and basic render behavior. |
src/components/index.js |
Exports the new invoice PDF APIs from the library’s main component index. |
package.json |
Bumps @react-pdf/renderer devDependency and broadens peer dependency to allow v4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/order-invoice-pdf/helpers.js (1)
88-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winQuantity filter still drops items that the row renderer treats as qty=1 — previously flagged, now regressed.
Line 90 filters out any item with a falsy
quantity(coversundefined/null/0), yet Line 108 renders missing quantity as1viaitem.quantity ?? 1. Since the filter runs first, items withundefined/nullquantity never reach the render step, so this fallback is dead code and those legacy/optional-quantity items silently disappear from both the invoice table and the running balance (Line 99balanceCents += item.amount) instead of showing with default qty 1. This is the exact issue previously raised on an earlier revision of this component (then atsrc/components/order-pdf/index.js) and marked resolved — it appears to have regressed during the file split intohelpers.js/styles.js/components.js/index.js.🐛 Proposed fix
(form.items || []) - .filter((item) => item.quantity) + .filter((item) => (item.quantity ?? 1) > 0) .forEach((item) => {🤖 Prompt for 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. In `@src/components/order-invoice-pdf/helpers.js` around lines 88 - 114, The item iteration in the order invoice row builder is filtering out any entry with a falsy quantity before rendering, which causes legacy items with missing quantity to disappear even though the row logic already defaults qty to 1. Update the item-processing path in the form/items loop so that undefined or null quantity values are allowed through while still excluding only truly invalid cases if needed, and keep the existing qty fallback used when building each row. Ensure the fix preserves the balance update and row creation for those items in the helpers.js item row construction.
🤖 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/components/order-invoice-pdf/index.js`:
- Around line 36-108: The OrderPdf component is only partially guarded: the
totals destructure uses order || {}, but later reads still access order.client,
order.address, order.number, order.purchased_date, summit.time_zone_id, and
summit.name directly. Update OrderPdf to consistently handle missing order and
summit data by defaulting the props up front or using optional
chaining/fallbacks at each access so rendering does not throw when either prop
is absent.
---
Duplicate comments:
In `@src/components/order-invoice-pdf/helpers.js`:
- Around line 88-114: The item iteration in the order invoice row builder is
filtering out any entry with a falsy quantity before rendering, which causes
legacy items with missing quantity to disappear even though the row logic
already defaults qty to 1. Update the item-processing path in the form/items
loop so that undefined or null quantity values are allowed through while still
excluding only truly invalid cases if needed, and keep the existing qty fallback
used when building each row. Ensure the fix preserves the balance update and row
creation for those items in the helpers.js item row construction.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc0a5572-6fe6-46c0-a3ca-9e4a0688b776
📒 Files selected for processing (8)
package.jsonsrc/components/index.jssrc/components/order-invoice-pdf/__tests__/order-invoice-pdf.test.jssrc/components/order-invoice-pdf/components.jssrc/components/order-invoice-pdf/helpers.jssrc/components/order-invoice-pdf/index.jssrc/components/order-invoice-pdf/styles.jswebpack.common.js
| export const OrderPdf = ({ order, summit, logoSrc, theme }) => { | ||
| const fontFamily = getThemeFontFamily(theme); | ||
| const styles = createStyles(fontFamily); | ||
| const rowStyles = createRowStyles(styles); | ||
| const rows = buildRows(order, summit); | ||
|
|
||
| const { | ||
| amount_due: total = 0, | ||
| cancelled_total: cancelledTotal = 0, | ||
| refunds_total: refundsTotal = 0, | ||
| retained = 0, | ||
| credited_to_payment_method: credited = 0 | ||
| } = order || {}; | ||
|
|
||
| const clientName = | ||
| order.client?.contact_name || order.purchased_by_full_name || ""; | ||
| const clientCompany = order.client?.company_name || ""; | ||
| const clientAddress = formatAddress(order.address) || "N/A"; | ||
|
|
||
| return ( | ||
| <Document> | ||
| <Page size="A4" orientation="portrait" style={styles.page}> | ||
| {/* Row 1: Logo | Invoice title + Order / Date */} | ||
| <View style={styles.headerRow} wrap={false}> | ||
| <View style={styles.cellLeft}> | ||
| {logoSrc ? <Image src={logoSrc} style={styles.logo} /> : null} | ||
| </View> | ||
| <View style={styles.cellRight}> | ||
| <Text style={styles.invoiceTitle}>Invoice</Text> | ||
| <FieldRow styles={styles} label="Order" value={order.number} /> | ||
| <FieldRow | ||
| styles={styles} | ||
| label="Date" | ||
| value={formatDate( | ||
| order.purchased_date, | ||
| summit.time_zone_id, | ||
| "YYYY/MM/DD hh:mm a" | ||
| )} | ||
| noBorder | ||
| /> | ||
| </View> | ||
| </View> | ||
|
|
||
| {/* Row 2: Client + Address | Event + Venue + Address */} | ||
| <View style={styles.headerRow} wrap={false}> | ||
| <View style={styles.cellLeft}> | ||
| <FieldRow | ||
| styles={styles} | ||
| label="Client" | ||
| value={`${clientCompany}\n${clientName}`} | ||
| /> | ||
| <FieldRow | ||
| styles={styles} | ||
| label="Address" | ||
| value={clientAddress} | ||
| noBorder | ||
| /> | ||
| </View> | ||
| <View style={styles.cellRight}> | ||
| <FieldRow styles={styles} label="Event" value={summit.name ?? ""} /> | ||
| <FieldRow | ||
| styles={styles} | ||
| label="Venue" | ||
| value={formatVenueName(summit?.main_locations?.[0]) ?? ""} | ||
| /> | ||
| <FieldRow | ||
| styles={styles} | ||
| label="Address" | ||
| value={formatAddress(summit?.main_locations?.[0]) ?? ""} | ||
| noBorder | ||
| /> | ||
| </View> | ||
| </View> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline src/components/order-invoice-pdf/index.js --view expanded || true
printf '\n== related symbols/usages ==\n'
rg -n "OrderPdf|generateInvoicePDF|previewPDF|PropTypes\.isRequired|order-invoice-pdf" src -g '!**/dist/**' -g '!**/build/**' || true
printf '\n== file around lines 1-220 ==\n'
nl -ba src/components/order-invoice-pdf/index.js | sed -n '1,220p'Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 2291
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== component file lines 1-240 ==\n'
sed -n '1,240p' src/components/order-invoice-pdf/index.js | cat -n
printf '\n== tests around render cases ==\n'
sed -n '300,430p' src/components/order-invoice-pdf/__tests__/order-invoice-pdf.test.js | cat -n
printf '\n== other callsites in repository ==\n'
rg -n "previewPDF\(|generateInvoicePDF\(|<OrderPdf|OrderPdf order=" src -g '!**/dist/**' -g '!**/build/**' || trueRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 12925
Guard order and summit consistently — order || {} only protects the totals destructure; order.client, order.address, order.number, order.purchased_date, summit.time_zone_id, and summit.name still throw if either prop is missing. Default the props up front or switch these reads to optional chaining.
🤖 Prompt for 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.
In `@src/components/order-invoice-pdf/index.js` around lines 36 - 108, The
OrderPdf component is only partially guarded: the totals destructure uses order
|| {}, but later reads still access order.client, order.address, order.number,
order.purchased_date, summit.time_zone_id, and summit.name directly. Update
OrderPdf to consistently handle missing order and summit data by defaulting the
props up front or using optional chaining/fallbacks at each access so rendering
does not throw when either prop is absent.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
| fieldRow: { | ||
| fontSize: 8, | ||
| lineHeight: 1.8, | ||
| flexDirection: "row", | ||
| color: "rgba(0, 0, 0, 0.87)", | ||
| paddingBottom: 3, | ||
| paddingTop: 6, | ||
| borderBottom: "1px solid rgba(224, 224, 224, 1)", | ||
| alignContent: "flex-start" | ||
| }, |
|
|
||
| (order.forms || []).forEach((form) => { | ||
| (form.items || []) | ||
| .filter((item) => item.quantity) |
| link.click(); | ||
| URL.revokeObjectURL(url); | ||
| document.body.removeChild(link); |
| "@mui/material": "^6.4.3", | ||
| "@mui/x-date-pickers": "^7.26.0", | ||
| "@react-pdf/renderer": "^3.1.11", | ||
| "@react-pdf/renderer": "^4.4.1", |
There was a problem hiding this comment.
@tomrndom @react-pdf/renderer is bumped from ^3.1.11 to ^4.4.1 here (and at line 129, peerDependencies), but yarn.lock wasn't updated — it still resolves 3.1.12. This is a major, breaking upstream release (v4.0.0 dropped CJS support per the official changelog), and it's a peer dependency already used by schedule-print-button.js/schedule-print-view.js, which have no test coverage. Since this PR's own test suite fully mocks @react-pdf/renderer, the v3→v4 upgrade is currently unverified anywhere in the repo. Could we regenerate the lockfile and do a manual smoke test of schedule-print (and this new component) rendering under v4 before merging?
| <OrderPdf order={order} summit={summit} logoSrc={logoSrc} theme={theme} /> | ||
| ).toBlob(); | ||
| const url = URL.createObjectURL(blob); | ||
| window.open(url, "_blank", "noopener,noreferrer"); |
There was a problem hiding this comment.
@tomrndom window.open here runs after await pdf(...).toBlob(), so the popup blocker no longer sees it as tied to the original click. Safari in particular will reliably block this (same pattern reported here: langgenius/dify#29389). A common fix is to open a blank tab synchronously in the click handler (window.open('', '_blank')) and set .location.href once the blob URL is ready.
| } | ||
| }; | ||
|
|
||
| export const previewPDF = async (order, summit, { logoSrc, theme } = {}) => { |
There was a problem hiding this comment.
@tomrndom Two small inconsistencies vs. generateInvoicePDF right above: this function has no try/catch, so a missing order/summit (the OrderPdf: ... is required throw) becomes an unhandled promise rejection here instead of a caught/logged error. Also, generateInvoicePDF revokes the object URL synchronously right after link.click(), while this one waits 60s — the immediate-revoke pattern has documented cases of producing empty/failed downloads on Safari and some Firefox versions (Scrince/YellowSphere#30). Worth aligning both on the same timing/error handling.
|
|
||
| import React from "react"; | ||
| import { render } from "@testing-library/react"; | ||
| import { buildRows, OrderPdf } from "../index"; |
There was a problem hiding this comment.
@tomrndom The suite covers buildRows and <OrderPdf>'s render output well, but never exercises generateInvoicePDF/previewPDF — the actual generate/preview feature this PR adds, and where the two issues above live. A test mocking document.createElement/URL.createObjectURL/window.open would catch both.
|
|
||
| (order.forms || []).forEach((form) => { | ||
| (form.items || []) | ||
| .filter((item) => item.quantity) |
There was a problem hiding this comment.
@tomrndom This was flagged by CodeRabbit on Jul 3 ("Keep the quantity filter consistent with the default quantity") and marked resolved/addressed, but the underlying issue is still here: .filter((item) => item.quantity) drops items where quantity is null/undefined, yet line 108 defaults qty to item.quantity ?? 1 — a default that can now never trigger, since anything reaching that point already passed the truthy filter. Items with a missing (not zero) quantity silently disappear from the invoice instead of rendering with an assumed qty of 1. A fresh Copilot review comment today flagged the same line independently, which confirms this regressed (likely during the file-split refactor) rather than being fixed. Suggest filtering with an explicit check like (item.quantity ?? 1) > 0 so the default and the filter agree.
| color: "rgba(0, 0, 0, 0.87)", | ||
| paddingBottom: 3, | ||
| paddingTop: 6, | ||
| borderBottom: "1px solid rgba(224, 224, 224, 1)", |
There was a problem hiding this comment.
@tomrndom Re: the Copilot comment on this line claiming fieldRowNoBorder's borderBottomWidth: 0 can't override this borderBottom shorthand — I traced @react-pdf/stylesheet's actual resolution pipeline (mergeStyles → expand) in both v3.1.12 (installed locally) and v4.2.0 (same major line as this PR's dependency bump), and the code is identical in both. mergeStyles flattens the style array in order, and since borderBottomWidth doesn't exist in fieldRow, it gets appended as a new key after borderBottom. expand() then iterates keys in that same insertion order — so borderBottom's shorthand expansion runs first, and the explicit borderBottomWidth: 0 from fieldRowNoBorder is processed afterward, overwriting it. The override does work correctly here; no change needed on this line.
| ? `${row.description} - Total: ${row.qty}` | ||
| : row.description} | ||
| </Text> | ||
| {row.cancelledBy ? ( |
There was a problem hiding this comment.
can't you just do rox.cancelledBy && .... ? I think is cleaner than a ternary
| import { currencyAmountFromCents } from "../../utils/money"; | ||
| import { MUI_ICON_PATHS, PDF_ICON_SIZE, fmtBalance } from "./helpers"; | ||
|
|
||
| export const PdfIcon = ({ name, color, size = PDF_ICON_SIZE }) => ( |
| </Svg> | ||
| ); | ||
|
|
||
| export const FieldRow = ({ styles, label, value, noBorder = false }) => ( |
| </View> | ||
| ); | ||
|
|
||
| export const PdfTableRow = ({ row, styles, rowStyles }) => { |
| // refundsTotal ← order.refunds_total | ||
| // retained ← order.retained | ||
| // credited ← order.credited_to_payment_method | ||
| export const ReconciliationBlock = ({ |
There was a problem hiding this comment.
etc etc. should be its own file.
Best Practices & Community ConsensusModern React development largely favors a pragmatic balance over rigid adherence to the rule:1. Define One "Public" Exported ComponentThe primary, default exported component should dominate the file. The file name should ideally match this component name (e.g., UserProfile.tsx).2. Allow Small "Private" Sub-ComponentsIf a component needs a tiny helper component that won't be used anywhere else in the application, it is acceptable to write it in the same file. Keep it below the main component and do not export it.
|
|
||
| export const DEFAULT_FONT_FAMILY = "Helvetica"; | ||
|
|
||
| export const MUI_ICON_PATHS = { |
There was a problem hiding this comment.
this would go in PdfIcon file
ref: https://app.clickup.com/t/9014802374/86baq9key
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit