Skip to content

Round cart line totals to whole cents - #20

Closed
sergical wants to merge 1 commit into
storefrom
demo-pr/cart-line-rounding
Closed

sergical wants to merge 1 commit into
storefrom
demo-pr/cart-line-rounding

Conversation

@sergical

@sergical sergical commented Oct 6, 2026

Copy link
Copy Markdown
Member

Each cart line is now rounded to whole cents before it is added to the total, so fractional prices no longer leave sub-cent remainders in the checkout total. A test covers a cart with fractional line totals.

Created with Claude Code

throw new RangeError(`Negative quantity for item ${i}`);
}
total += items[i].price * items[i].qty;
total += Math.round(items[i].price * items[i].qty);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The code rounds each line item's subtotal individually. Subtotals less than 0.5 will be rounded to 0, leading to an incorrect final cart total and potential under-charging.
Severity: MEDIUM

Suggested Fix

To prevent precision loss, sum the exact subtotals of all line items first, and then round the final total only once at the very end. For example: return Math.round(items.reduce((acc, item) => acc + (item.price * item.qty), 0));. This ensures fractional values are not discarded prematurely.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: apps/pr-reviewer/fixtures/demo-pr/cart-total.ts#L7

Potential issue: The function calculates the total cart value by rounding the subtotal
for each line item before summing them. The use of `Math.round(items[i].price *
items[i].qty)` can lead to accumulated rounding errors. If an item's subtotal is a
fractional value less than 0.5, it will be rounded down to 0. If a cart contains
multiple such items, their cost will be completely lost from the final total, resulting
in the customer being under-charged. This edge case is not covered by existing tests but
is a plausible scenario, especially if discounts create fractional prices.

Did we get this right? 👍 / 👎 to inform future reviews.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4b9abd1. Configure here.

throw new RangeError(`Negative quantity for item ${i}`);
}
total += items[i].price * items[i].qty;
total += Math.round(items[i].price * items[i].qty);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsafe floating-point line rounding

Medium Severity

Math.round on the raw price * qty product can pick the wrong whole cent when binary floating point lands just below a half-cent. Checkout then undercharges those lines by one cent. The new coverage stays far enough from a .5 boundary that this miss is not caught.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4b9abd1. Configure here.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

Verdict: Needs changes — per-line rounding introduces unvalidated/ambiguous behavior and the new test doesn't actually pin down the intended semantics.

Correctness

  • cart-total.ts: Math.round is applied per line item, not to the final total. This is a real semantic choice (rounding each line vs. rounding the sum) that changes results for carts with fractional prices across multiple lines, since per-line rounding errors don't cancel out the way they would if the total were rounded once. There's no indication in the PR of which behavior is actually desired by the business.
  • No validation or handling for negative/NaN/Infinity price. The function already validates qty < 0 with a RangeError, but a negative or non-finite price is silently rounded and summed, which is an inconsistency in input validation.
  • Math.round's asymmetric half-rounding (ties round toward +∞, so -2.5 → -2 but 2.5 → 3) is unaddressed; if prices can ever be negative (discounts/refunds), this could cause subtle miscalculations.
  • The new test (rounds each line to whole cents) only checks the aggregate result 110. For the chosen fixture values, per-line rounding and rounding-the-final-sum-once both happen to produce 110 (99.9→100, 10.4→10, sum=110 vs. unrounded sum 110.3→110). The test therefore does not actually distinguish/guard the specific per-line-rounding implementation from the alternative, so it gives weaker regression protection than its name implies.
  • run_tests result: all 17 tests pass (2 suites, 0 failures), so nothing is broken today — but the gap above is about untested edge cases/ambiguous intent, not a failing test.

Style

  • The business rule being encoded (round each line to the nearest cent before summing) deserves a short comment at the call site, since it's not self-evident why rounding happens per line rather than on the total.
  • Minor/pre-existing: the classic indexed for loop (items[i].price, items[i].qty) could be simplified with for (const { price, qty } of items), improving readability of the new Math.round line too. Optional, not introduced by this diff.
  • Consider a slightly more descriptive test name or an inline comment showing the expected per-line arithmetic (99.9→100, 10.4→10, total 110), since the math isn't obvious at a glance and, as noted above, doesn't uniquely confirm per-line rounding.

Summary

Tests pass, but the core logic change (per-line vs. total rounding) is under-specified and under-tested, and input validation for price is inconsistent with the existing qty validation. Recommend clarifying the intended rounding semantics, adding a test that actually distinguishes per-line from total rounding, and considering price validation before merging.

@sergical

sergical commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

The store branch work is on main now (973ed2e). The cache-lifetime demo pull request replaces this one.

@sergical sergical closed this Oct 7, 2026
@sergical
sergical deleted the demo-pr/cart-line-rounding branch October 7, 2026 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant