Feature: order grid revamp#262
Open
santipalenque wants to merge 11 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughRefactors SponsorOrderGrid into an order-based ledger with per-item running balances, new transaction-type UI (BalanceValue, TransactionType), reconciliation UI (ReconciliationBox, TotalFooter), cancelled-items header, updated extra-row components to show balances, tests, i18n updates, and a package version bump to 5.0.33-beta.2. ChangesSponsor Order Grid Ledger Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 7
🧹 Nitpick comments (6)
src/i18n/en.json (1)
143-150: ⚡ Quick winInconsistent spelling: "canceled" vs "Cancelled".
Line 143 uses American spelling ("canceled") twice:
- "Active order items can be canceled"
- "Canceled items show an undo action"
However, lines 144-150 use British spelling ("Cancelled"):
- Line 144: "Cancelled {date} by {user}"
- Line 145: "Cancelled items ({count})"
- Line 147: "Cancelled"
Choose one spelling convention (either American "canceled" or British "cancelled") and apply it consistently across all
sponsor_order_gridtranslation keys.🔤 Suggested fix for American English consistency
- "cancel_info_note": "Active order items can be canceled. Canceled items show an undo action to restore them. Refund and payment rows are display-only.", - "cancelled_by": "Cancelled {date} by {user}", - "cancelled_items": "Cancelled items ({count}):", + "cancel_info_note": "Active order items can be canceled. Canceled items show an undo action to restore them. Refund and payment rows are display-only.", + "cancelled_by": "Canceled {date} by {user}", + "cancelled_items": "Canceled items ({count}):", "reconciliation": "Reconciliation", - "cancelled": "Cancelled", + "cancelled": "Canceled",Alternatively, if British English is preferred, update line 143 to use "cancelled" throughout.
🤖 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/i18n/en.json` around lines 143 - 150, The translation entries under sponsor_order_grid use mixed American and British spellings; update the keys/values for consistency by choosing one spelling and applying it across the affected keys: change the "cancel_info_note" value to use the same spelling as "cancelled_by", "cancelled_items", and "cancelled" (or vice versa), and ensure the displayed phrase in "cancel_info_note" and the label in "cancelled_items" both use the chosen spelling; specifically review and update the values for "cancel_info_note", "cancelled_by", "cancelled_items", and "cancelled" so they all consistently use either "canceled"/"Canceled" or "cancelled"/"Cancelled".src/components/mui/InfoNote/index.jsx (1)
11-11: 💤 Low valuePrefer theme-based or numeric fontSize values.
The hardcoded
"13px"string bypasses MUI's theme sizing system and uses absolute pixel units, which can reduce accessibility for users with custom font-size preferences. Consider using a theme-relative value or a numeric pixel value for better integration with MUI's design system.♻️ Suggested alternatives
Option 1 (preferred): Use MUI theme typography sizing:
- <Typography variant="body1" color="text.secondary" sx={{fontSize: "13px"}}> + <Typography variant="body2" color="text.secondary">Option 2: Use numeric value (still absolute, but more idiomatic):
- <Typography variant="body1" color="text.secondary" sx={{fontSize: "13px"}}> + <Typography variant="body1" color="text.secondary" sx={{fontSize: 13}}>Option 3: Use theme spacing with rem units:
- <Typography variant="body1" color="text.secondary" sx={{fontSize: "13px"}}> + <Typography variant="body1" color="text.secondary" sx={{fontSize: "0.8125rem"}}>🤖 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/mui/InfoNote/index.jsx` at line 11, Replace the hardcoded string font size "13px" in the InfoNote component's Typography sx with a theme-aware or numeric value; e.g., use a theme callback like sx={(theme) => ({ fontSize: theme.typography.pxToRem(13) })} or switch to a built-in size/variant (e.g., variant="body2") or use numeric pixels (fontSize: 13) so the Typography (in InfoNote) integrates with MUI's theme system and accessibility settings.src/components/mui/SponsorOrderGrid/index.js (3)
227-243: 💤 Low valueMisleading
key="balance"on the action cell.The action
TableCellreuseskey="balance"(copy-pasted from the balance column). It doesn't collide within the row, but it's confusing and brittle; key it on the row id/action instead.🔧 Proposed fix
cols.push( <TableCell - key="balance" + key={`grid-col-${row.id}-action`} align="right" >🤖 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/mui/SponsorOrderGrid/index.js` around lines 227 - 243, The action TableCell added inside the canCancel branch is using a misleading static key "balance"; update the key to be unique per row/action (e.g. use row.id or a composite like `action-${row.id}`) so the element identity is stable and clear; modify the cols.push call that creates the TableCell (the block referencing canCancel, TableCell, row.cancelled, onUndoCancelForm and onCancelForm) to set its key to a row-specific value instead of "balance".
96-101: ⚖️ Poor tradeoff
calculateBalancemutates closure state during render.The running
balanceis mutated as a side effect while building JSX. It works under React 17's synchronous single pass, but it's order-dependent and would break under StrictMode double-invocation or concurrent rendering. Consider precomputing balances inmapOrderData/a memoized pass and passing values down, keeping render pure.Also applies to: 184-263
🤖 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/mui/SponsorOrderGrid/index.js` around lines 96 - 101, The calculateBalance function mutates the outer balance variable during render (balance and calculateBalance), which is unsafe under StrictMode; instead compute running balances ahead of rendering—e.g., in mapOrderData or a useMemo that iterates the original data once, producing a new array with a computed balance field on each row (attach as row.runningBalance or similar). Replace calls to calculateBalance in the JSX with the precomputed property and remove the closure mutation and calculateBalance side effects so rendering becomes pure and order-independent.
80-91: 💤 Low valueDestructuring
orderwithout a default crashes when the prop is absent.
const {...} = order;throws a TypeError iforderis undefined/null. Sinceorderis now required (replacing the prior individual props), add a default to fail soft during the migration.🛡️ Proposed fix
title = T.translate("sponsor_order_grid.title"), - order, + order = {}, withReconciliation = false,🤖 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/mui/SponsorOrderGrid/index.js` around lines 80 - 91, The destructuring of the prop "order" in SponsorOrderGrid (the const { forms, fees, payments, refunds, notes, total, retained, credited_to_payment_method: credited, cancelled_total: cancelledTotal, refunds_total: refundsTotal } = order;) will throw if order is undefined; change the destructuring to provide a safe default object (or set a default param for the component) so the destructure reads from {} when order is missing and avoid runtime TypeError—keep the same property names/aliases so downstream code continues to work.src/components/mui/SponsorOrderGrid/components/CancelledItems.jsx (1)
21-23: 💤 Low valueGuard against an undefined
cancelledItems.
cancelledItems.lengththrows if the prop is omitted. A default empty array keeps the component safe when reused outside the current call site.🛡️ Proposed fix
-const CancelledItems = ({cancelledItems, sx = {}}) => { +const CancelledItems = ({cancelledItems = [], sx = {}}) => {🤖 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/mui/SponsorOrderGrid/components/CancelledItems.jsx` around lines 21 - 23, The component CancelledItems currently assumes cancelledItems is defined and calls cancelledItems.length; update its props handling to guard against undefined by defaulting cancelledItems to an empty array or validating it before use (e.g., change the function signature to use cancelledItems = [] or add an Array.isArray check) so checks like cancelledItems.length won't throw when the prop is omitted.
🤖 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/mui/SponsorOrderGrid/components/CancelledItems.jsx`:
- Around line 31-39: The mapped Link elements in the CancelledItems component
are missing a React key which causes warnings and unstable reconciliation;
update the cancelledItems.map callback (in CancelledItems.jsx) to add a stable
key prop to each <Link> using item.id (or another unique identifier) so each
child has a unique key.
In `@src/components/mui/SponsorOrderGrid/components/ReconciliationBox.jsx`:
- Around line 21-54: Reconcile the displayed amount with the label in
ReconciliationBox: compute a totalValue (e.g., const totalValue = retained > 0 ?
retained : (credited > 0 ? credited : retained)) alongside totalLabel and
replace the hard-coded currencyAmountFromCents(retained) in the bottom-right
cell with currencyAmountFromCents(totalValue) so that when totalLabel is
"credited" the credited amount is shown; update the variables near where
totalLabel is defined (in the ReconciliationBox component) and render
currencyAmountFromCents(totalValue) instead of retained.
In `@src/components/mui/SponsorOrderGrid/components/TransactionType.js`:
- Around line 30-38: TransactionType currently dereferences iconMap[type]
without checking existence which crashes when type is missing; update the
component to guard access to iconMap by reading a safe entry (e.g. const meta =
iconMap[type] || {icon: DefaultIcon, color: 'inherit'}), use meta.icon as Icon
and meta.color for the sx color, and ensure children/type text still renders
when meta is the fallback; reference the TransactionType component, iconMap,
Icon and children when applying this change.
- Around line 22-28: The module crashes because SPONSOR_ORDER_GRID_ITEM_TYPES is
undefined when Jest mocks "../../../../utils/constants" in tests, so update the
tests' mocks to return SPONSOR_ORDER_GRID_ITEM_TYPES (or replace the mock with
jest.requireActual("../../../../utils/constants") spread) so TransactionType can
import it, and also harden TransactionType by guarding access to iconMap entries
(in TransactionType.js: check iconMap[type] exists before accessing
iconMap[type].icon / .color and provide a sensible default icon/color) to
prevent module-eval failures when the constant is missing.
In `@src/components/mui/SponsorOrderGrid/index.js`:
- Around line 252-260: The DiscountRow is passing mismatched fields
(discountCents={form.discount_total}) while calculateBalance uses
form.discount_in_cents, which can be undefined and produces NaN; update the
DiscountRow to use a single canonical cents value (e.g., const discountCents =
form.discount_in_cents ?? form.discount_total ?? 0) and pass that same
discountCents into both the DiscountRow prop and calculateBalance(discountCents,
-1), ensuring a default of 0 to avoid NaN and crashes (also ensure any
downstream callers like BalanceValue/currencyAmountFromCents receive an
integer). Additionally, rename the action column TableCell key from "balance" to
a clearer key like "action" to avoid the misleading identifier.
In `@src/components/mui/table/extra-rows/DiscountRow.jsx`:
- Around line 25-27: The guard in DiscountRow currently checks `discountCents
=== 0`, which misses null/undefined and allows
`currencyAmountFromCents(discountCents)` to print NaN; change the early-return
to a falsy check (e.g., `if (!discountCents) return null;`) so the row doesn't
render for 0, null, or undefined values and ensure `currencyAmountFromCents`
only receives a valid number.
In `@src/components/mui/table/extra-rows/TotalRow.jsx`:
- Around line 23-26: The TotalRow/TotalFooter crash happens because
currencyAmountFromCents is called with Math.abs(total) when total can be
non-integer/NaN/nullish/or a preformatted string; update the rendering logic
around total (variables isNegative, sign, totalStr) to sanitize/guard before
calling currencyAmountFromCents: set isNegative = (typeof total === "number" &&
total < 0), if typeof total === "number" and Number.isFinite(total) compute
sanitizedCents = Math.round(Math.abs(total)) and pass that to
currencyAmountFromCents, if typeof total === "string" treat it as a preformatted
string and render it directly (skip currencyAmountFromCents), and for
null/NaN/other types fallback sanitizedCents = 0; apply the same fix in
TotalFooter (same currencyAmountFromCents(Math.abs(total)) usage).
---
Nitpick comments:
In `@src/components/mui/InfoNote/index.jsx`:
- Line 11: Replace the hardcoded string font size "13px" in the InfoNote
component's Typography sx with a theme-aware or numeric value; e.g., use a theme
callback like sx={(theme) => ({ fontSize: theme.typography.pxToRem(13) })} or
switch to a built-in size/variant (e.g., variant="body2") or use numeric pixels
(fontSize: 13) so the Typography (in InfoNote) integrates with MUI's theme
system and accessibility settings.
In `@src/components/mui/SponsorOrderGrid/components/CancelledItems.jsx`:
- Around line 21-23: The component CancelledItems currently assumes
cancelledItems is defined and calls cancelledItems.length; update its props
handling to guard against undefined by defaulting cancelledItems to an empty
array or validating it before use (e.g., change the function signature to use
cancelledItems = [] or add an Array.isArray check) so checks like
cancelledItems.length won't throw when the prop is omitted.
In `@src/components/mui/SponsorOrderGrid/index.js`:
- Around line 227-243: The action TableCell added inside the canCancel branch is
using a misleading static key "balance"; update the key to be unique per
row/action (e.g. use row.id or a composite like `action-${row.id}`) so the
element identity is stable and clear; modify the cols.push call that creates the
TableCell (the block referencing canCancel, TableCell, row.cancelled,
onUndoCancelForm and onCancelForm) to set its key to a row-specific value
instead of "balance".
- Around line 96-101: The calculateBalance function mutates the outer balance
variable during render (balance and calculateBalance), which is unsafe under
StrictMode; instead compute running balances ahead of rendering—e.g., in
mapOrderData or a useMemo that iterates the original data once, producing a new
array with a computed balance field on each row (attach as row.runningBalance or
similar). Replace calls to calculateBalance in the JSX with the precomputed
property and remove the closure mutation and calculateBalance side effects so
rendering becomes pure and order-independent.
- Around line 80-91: The destructuring of the prop "order" in SponsorOrderGrid
(the const { forms, fees, payments, refunds, notes, total, retained,
credited_to_payment_method: credited, cancelled_total: cancelledTotal,
refunds_total: refundsTotal } = order;) will throw if order is undefined; change
the destructuring to provide a safe default object (or set a default param for
the component) so the destructure reads from {} when order is missing and avoid
runtime TypeError—keep the same property names/aliases so downstream code
continues to work.
In `@src/i18n/en.json`:
- Around line 143-150: The translation entries under sponsor_order_grid use
mixed American and British spellings; update the keys/values for consistency by
choosing one spelling and applying it across the affected keys: change the
"cancel_info_note" value to use the same spelling as "cancelled_by",
"cancelled_items", and "cancelled" (or vice versa), and ensure the displayed
phrase in "cancel_info_note" and the label in "cancelled_items" both use the
chosen spelling; specifically review and update the values for
"cancel_info_note", "cancelled_by", "cancelled_items", and "cancelled" so they
all consistently use either "canceled"/"Canceled" or "cancelled"/"Cancelled".
🪄 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: 39ee3ca9-abe1-4254-b608-4642fd6138d6
📒 Files selected for processing (16)
package.jsonsrc/components/mui/InfoNote/index.jsxsrc/components/mui/SponsorOrderGrid/components/BalanceValue.jsxsrc/components/mui/SponsorOrderGrid/components/CancelledItems.jsxsrc/components/mui/SponsorOrderGrid/components/ReconciliationBox.jsxsrc/components/mui/SponsorOrderGrid/components/TotalFooter.jsxsrc/components/mui/SponsorOrderGrid/components/TransactionType.jssrc/components/mui/SponsorOrderGrid/index.jssrc/components/mui/table/extra-rows/DiscountRow.jsxsrc/components/mui/table/extra-rows/FeeRow.jsxsrc/components/mui/table/extra-rows/NotesRow.jsxsrc/components/mui/table/extra-rows/PaymentRow.jsxsrc/components/mui/table/extra-rows/RefundRow.jsxsrc/components/mui/table/extra-rows/TotalRow.jsxsrc/i18n/en.jsonsrc/utils/constants.js
smarcet
reviewed
Jun 4, 2026
smarcet
reviewed
Jun 4, 2026
smarcet
requested changes
Jun 4, 2026
Collaborator
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@santipalenque please review
santipalenque
force-pushed
the
feature/order-grid-revamp
branch
from
afdac1c to
616b857
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/mui/__tests__/payment-row.test.js (1)
40-68: ⚡ Quick winConsider adding test coverage for the balance prop.
The refactored
PaymentRownow renders a balance column usingBalanceValue, but the test suite doesn't verify this new functionality.🧪 Suggested test case
test("renders the balance value when provided", () => { renderInTable({ payment: { method: "Visa", amount: 2000, created: PAYMENT_TIMESTAMP }, balance: 5000 }); // Add assertion to verify BalanceValue renders with balance prop });🤖 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/mui/__tests__/payment-row.test.js` around lines 40 - 68, Add a test that covers the new balance prop: call renderInTable with a payment object and a balance value, then assert that the BalanceValue output appears (e.g., the formatted balance string or a DOM node rendered by BalanceValue is present). Target the helpers/fixtures used in this file by updating the existing test block for PaymentRow and use renderInTable and screen/querySelector to locate the BalanceValue output for the given balance prop.src/components/mui/__tests__/total-row.test.js (1)
14-22: ⚡ Quick winMock
currencyAmountFromCentsfor consistency.Unlike
payment-row.test.jsandrefund-row.test.js, this test file doesn't mock thecurrencyAmountFromCentsutility. This inconsistency could cause brittleness if the utility's implementation changes.💡 Suggested mock
Add after the i18n mock:
jest.mock("../../../utils/money", () => ({ currencyAmountFromCents: (amount) => `$${(amount / 100).toFixed(2)}` }));🤖 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/mui/__tests__/total-row.test.js` around lines 14 - 22, The test lacks a mock for the currency utility which can cause brittleness; add a Jest mock for currencyAmountFromCents (the exported function from the module used by TotalRow) immediately after the existing i18n mock so tests return a stable formatted string (e.g., mapping cents to a dollar string like `$<amount>` with two decimals) — target the module that exports currencyAmountFromCents (the same import used by TotalRow) so the TotalRow tests use the mocked formatter consistently with payment-row.test.js and refund-row.test.js.src/components/mui/__tests__/refund-row.test.js (1)
37-70: ⚡ Quick winConsider adding test coverage for the balance prop.
The refactored
RefundRownow renders a balance column usingBalanceValue, but the test suite doesn't verify this new functionality.🧪 Suggested test case
test("renders the balance value when provided", () => { renderInTable({ refund: { reason: "Duplicate", status: "completed", amount: 3000 }, balance: 8000 }); // Add assertion to verify BalanceValue renders with balance prop });🤖 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/mui/__tests__/refund-row.test.js` around lines 37 - 70, Add a new test that verifies the balance prop is rendered by BalanceValue: in refund-row.test.js create a test named like "renders the balance value when provided" that calls renderInTable({ refund: { reason: "Duplicate", status: "completed", amount: 3000 }, balance: 8000 }) and assert that the expected formatted balance (e.g. "$80.00") produced by BalanceValue is present via screen.getByText or container queries; reference RefundRow, BalanceValue and renderInTable to locate where to add the test.
🤖 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.
Nitpick comments:
In `@src/components/mui/__tests__/payment-row.test.js`:
- Around line 40-68: Add a test that covers the new balance prop: call
renderInTable with a payment object and a balance value, then assert that the
BalanceValue output appears (e.g., the formatted balance string or a DOM node
rendered by BalanceValue is present). Target the helpers/fixtures used in this
file by updating the existing test block for PaymentRow and use renderInTable
and screen/querySelector to locate the BalanceValue output for the given balance
prop.
In `@src/components/mui/__tests__/refund-row.test.js`:
- Around line 37-70: Add a new test that verifies the balance prop is rendered
by BalanceValue: in refund-row.test.js create a test named like "renders the
balance value when provided" that calls renderInTable({ refund: { reason:
"Duplicate", status: "completed", amount: 3000 }, balance: 8000 }) and assert
that the expected formatted balance (e.g. "$80.00") produced by BalanceValue is
present via screen.getByText or container queries; reference RefundRow,
BalanceValue and renderInTable to locate where to add the test.
In `@src/components/mui/__tests__/total-row.test.js`:
- Around line 14-22: The test lacks a mock for the currency utility which can
cause brittleness; add a Jest mock for currencyAmountFromCents (the exported
function from the module used by TotalRow) immediately after the existing i18n
mock so tests return a stable formatted string (e.g., mapping cents to a dollar
string like `$<amount>` with two decimals) — target the module that exports
currencyAmountFromCents (the same import used by TotalRow) so the TotalRow tests
use the mocked formatter consistently with payment-row.test.js and
refund-row.test.js.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33c7dc5d-35d6-4d0e-a9ff-a9c8c0828275
📒 Files selected for processing (16)
src/components/mui/SponsorOrderGrid/__tests__/SponsorOrderGrid.test.jssrc/components/mui/SponsorOrderGrid/components/CancelledItems.jsxsrc/components/mui/SponsorOrderGrid/components/ReconciliationBox.jsxsrc/components/mui/SponsorOrderGrid/components/TotalFooter.jsxsrc/components/mui/SponsorOrderGrid/components/TransactionType.jssrc/components/mui/SponsorOrderGrid/components/__tests__/BalanceValue.test.jsxsrc/components/mui/SponsorOrderGrid/components/__tests__/CancelledItems.test.jsxsrc/components/mui/SponsorOrderGrid/components/__tests__/ReconciliationBox.test.jsxsrc/components/mui/SponsorOrderGrid/components/__tests__/TotalFooter.test.jsxsrc/components/mui/SponsorOrderGrid/components/__tests__/TransactionType.test.jsxsrc/components/mui/SponsorOrderGrid/index.jssrc/components/mui/__tests__/payment-row.test.jssrc/components/mui/__tests__/refund-row.test.jssrc/components/mui/__tests__/total-row.test.jssrc/components/mui/table/extra-rows/DiscountRow.jsxsrc/components/mui/table/extra-rows/TotalRow.jsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/components/mui/table/extra-rows/TotalRow.jsx
- src/components/mui/SponsorOrderGrid/components/TransactionType.js
- src/components/mui/SponsorOrderGrid/components/TotalFooter.jsx
- src/components/mui/SponsorOrderGrid/components/ReconciliationBox.jsx
- src/components/mui/SponsorOrderGrid/index.js
- src/components/mui/SponsorOrderGrid/components/CancelledItems.jsx
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/utils/__tests__/money.test.js (1)
122-158: 💤 Low valueConsider adding test coverage for unsafe integer edge case.
While unlikely in practice,
Number.isInteger()returnstruefor values beyondNumber.MAX_SAFE_INTEGER, which would pass the early validation but throw whenamountFromCentschecks for safe integers. Adding a test case ensures the try/catch properly handles this edge case:🧪 Suggested test case
it("returns error string for unsafe integer numbers", () => { expect(currencyAmountFromCents(Number.MAX_SAFE_INTEGER + 1)).toBe("!ERROR"); });🤖 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/utils/__tests__/money.test.js` around lines 122 - 158, The tests lack coverage for the unsafe-integer edge case where Number.isInteger returns true but the value exceeds Number.MAX_SAFE_INTEGER and will fail inside amountFromCents; add a test in the currencyAmountFromCents suite that calls currencyAmountFromCents(Number.MAX_SAFE_INTEGER + 1) and expects "!ERROR" so the try/catch path in amountFromCents is exercised and the function returns the error string for unsafe integers.src/utils/money.js (3)
164-167: ⚡ Quick winConsider validating negative cents in the early check for consistency.
The early validation checks for type and integer, but relies on the
try/catchto handle negative cents (whichamountFromCentsrejects at line 145-147). For consistency and clarity, consider adding the negative check here:♻️ Proposed refactor
- if (typeof cents !== "number" || !Number.isInteger(cents)) { - console.error("ERROR - currencyAmountFromCents: cents must be an integer number"); + if (typeof cents !== "number" || !Number.isInteger(cents) || cents < 0) { + console.error("ERROR - currencyAmountFromCents: cents must be a non-negative integer number"); return result; }🤖 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/utils/money.js` around lines 164 - 167, The early validation in currencyAmountFromCents only checks type and integer but not negativity; add a negative check alongside the existing guards so that currencyAmountFromCents verifies cents is >= 0, logs an error (similar message style) and returns the default result when cents is negative, keeping behavior consistent with amountFromCents which rejects negative values.
165-165: ⚡ Quick winInclude the invalid cents value in the error log for easier debugging.
The error message logs the validation failure but doesn't include the actual
centsvalue that was passed, making it harder to debug the source of invalid data.♻️ Proposed enhancement
- console.error("ERROR - currencyAmountFromCents: cents must be an integer number"); + console.error(`ERROR - currencyAmountFromCents: cents must be an integer number, received: ${typeof cents === 'object' ? JSON.stringify(cents) : cents} (type: ${typeof cents})`);🤖 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/utils/money.js` at line 165, Update the error log in currencyAmountFromCents to include the invalid cents value so debugging shows the actual input; change the console.error call that currently emits "ERROR - currencyAmountFromCents: cents must be an integer number" to log a message containing the cents variable (and optionally its type) and context (e.g., "ERROR - currencyAmountFromCents: invalid cents value: ", cents) so the invalid input is visible when the validation fails.
162-177: ⚖️ Poor tradeoffThe "!ERROR" sentinel will be displayed directly to users.
Based on the relevant code snippet from
BalanceValue.jsx:18-28, the return value is rendered directly in aTypographycomponent without checking for error sentinels. While "!ERROR" is useful for debugging, it may not provide the best user experience in production.Consider one of these alternatives:
- Use a more user-friendly error string like
"--"or"N/A"- Return an object
{ value: string, isError: boolean }so UI components can handle errors appropriately- Document that upstream components should validate inputs before calling this function
The current approach prevents crashes (which aligns with the PR objective), but the error presentation could be refined in a follow-up.
🤖 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/utils/money.js` around lines 162 - 177, The function currencyAmountFromCents currently returns the literal sentinel "!ERROR" which may be rendered to users; change its API to return an object { value: string, isError: boolean } instead: on success set value to `${symbol}${amount}` and isError=false, on validation failure or exception set value to a user-friendly placeholder like "—" (or "N/A") and isError=true; update callers (e.g., BalanceValue.jsx) to read the .value for display and use .isError to decide alternate UI behavior; ensure references to amountFromCents and CURRENCY_SYMBOL remain but adapt error branches to return the object rather than "!ERROR".
🤖 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.
Nitpick comments:
In `@src/utils/__tests__/money.test.js`:
- Around line 122-158: The tests lack coverage for the unsafe-integer edge case
where Number.isInteger returns true but the value exceeds
Number.MAX_SAFE_INTEGER and will fail inside amountFromCents; add a test in the
currencyAmountFromCents suite that calls
currencyAmountFromCents(Number.MAX_SAFE_INTEGER + 1) and expects "!ERROR" so the
try/catch path in amountFromCents is exercised and the function returns the
error string for unsafe integers.
In `@src/utils/money.js`:
- Around line 164-167: The early validation in currencyAmountFromCents only
checks type and integer but not negativity; add a negative check alongside the
existing guards so that currencyAmountFromCents verifies cents is >= 0, logs an
error (similar message style) and returns the default result when cents is
negative, keeping behavior consistent with amountFromCents which rejects
negative values.
- Line 165: Update the error log in currencyAmountFromCents to include the
invalid cents value so debugging shows the actual input; change the
console.error call that currently emits "ERROR - currencyAmountFromCents: cents
must be an integer number" to log a message containing the cents variable (and
optionally its type) and context (e.g., "ERROR - currencyAmountFromCents:
invalid cents value: ", cents) so the invalid input is visible when the
validation fails.
- Around line 162-177: The function currencyAmountFromCents currently returns
the literal sentinel "!ERROR" which may be rendered to users; change its API to
return an object { value: string, isError: boolean } instead: on success set
value to `${symbol}${amount}` and isError=false, on validation failure or
exception set value to a user-friendly placeholder like "—" (or "N/A") and
isError=true; update callers (e.g., BalanceValue.jsx) to read the .value for
display and use .isError to decide alternate UI behavior; ensure references to
amountFromCents and CURRENCY_SYMBOL remain but adapt error branches to return
the object rather than "!ERROR".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b687768-eeb5-477e-b607-c12e59a4ea4b
📒 Files selected for processing (5)
package.jsonsrc/components/mui/SnackbarNotification/index.jssrc/components/mui/SponsorOrderGrid/index.jssrc/utils/__tests__/money.test.jssrc/utils/money.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/components/mui/SponsorOrderGrid/index.js
…nd payments by created
santipalenque
force-pushed
the
feature/order-grid-revamp
branch
from
29ba7d7 to
bc10316
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/86ba505mg
Summary by CodeRabbit
New Features
Refactor
Style
Tests
Chores