Skip to content

Add pricing and checkout helpers for the store - #9

Merged
sergical merged 1 commit into
storefrom
pricing-helpers
Oct 2, 2026
Merged

sergical merged 1 commit into
storefrom
pricing-helpers

Conversation

@sergical

@sergical sergical commented Oct 2, 2026

Copy link
Copy Markdown
Member

Adds the helpers the checkout page needs: cart total, discounts, loyalty points, the refund window, shipping, currency formatting, order ids and the confirmation email.

Each helper is a small pure function with no I/O so it can be unit tested on its own.

Created with Claude Code

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Verdict: ❌ Request changes — multiple real bugs across these new demo files (off-by-one crash, unit-conversion error, silent NaN, and a likely percent/fraction mismatch).

Correctness (blocking)

  1. cart-total.ts — off-by-one, will throw at runtime.
    for (let i = 0; i <= items.length; i++) iterates one past the end of the array. On the last iteration items[items.length] is undefined, so .price/.qty throws a TypeError. Should be i < items.length (or better, items.reduce(...)).

  2. refund-window.ts — wrong unit conversion, breaks the refund window entirely.
    (now.getTime() - deliveredAt.getTime()) / 86400 divides milliseconds by seconds-per-day. getTime() is in ms, so the divisor must be 86400000 (ms/day). As written, days is ~1000x too large, so canRefund will return false almost immediately after delivery instead of honoring a real 30-day window. This is a significant functional bug, not cosmetic.

  3. discount.ts — ambiguous/likely-wrong percent semantics.
    applyDiscount(total, percent) computes total - total * percent, which only works if percent is a fraction (0.2 for 20%). The parameter name percent strongly suggests a whole number (20), and if called that way the result goes deeply negative (e.g. applyDiscount(100, 20) → -1900). Needs a clear contract (doc comment and/or rename) and ideally input validation/clamping.

  4. order-id.ts — silent failure instead of error handling.
    parseInt(last) has no radix and no validation. If last isn't numeric (e.g. "ORD-001"), this silently returns the string "NaN" rather than failing loudly. It also doesn't preserve zero-padding/prefixes ("00042" → "43", losing the format). Add a radix (parseInt(last, 10)) and decide/validate the expected ID format.

  5. loyalty.ts — rounds too early and returns fractional points.
    Math.round(totalCents / 100) * 1.5 rounds the dollar amount before applying the multiplier, introducing avoidable rounding error, and the final result is not rounded, so callers can receive fractional "points" (e.g. 1.5), which is surprising for a points system.

  6. currency.ts — unformatted currency output.
    "$" + cents / 100 yields values like $10.5 instead of $10.50, and can produce long floating-point artifacts for some inputs. Use .toFixed(2).

  7. email.ts — overly weak validation given the function's implied contract.
    value.includes("@") accepts clearly invalid strings ("@", "a@", "@@@"). Low severity since it's a tiny demo helper, but the name isEmail oversells what it actually checks.

shipping.ts looks correct (boundary and express-multiplier logic both behave as intended).

Style / clarity

  • Manual index loops (cart-total.ts) should be replaced with reduce/for...of to sidestep index bugs and improve readability.
  • Pervasive unexplained magic numbers across the set: 100 (cents/dollar), 1.5 (points rate), 86400/30 (refund window), 5/12/6/2 (shipping tiers). Extract named constants and add short doc comments stating units (cents vs dollars, ms vs days, fraction vs whole-number percent).
  • parseInt without a radix in order-id.ts is an easy lint fix (parseInt(last, 10)).
  • Naming: percent in discount.ts and isEmail in email.ts both invite misuse — rename or document the exact contract (e.g., "fraction, 0–1" / "minimal heuristic, not full validation").
  • None of the files carry JSDoc describing expected units/ranges, which matters a lot here since several bugs stem directly from unit/format ambiguity.

Summary

This is a small fixture set but it has several concrete logic bugs (array OOB access, ms/s unit mix-up, NaN propagation) plus a design ambiguity in discount.ts that could cause large behavioral errors downstream. Recommend fixing items 1–5 before merging; items 6–7 and the style notes are good follow-ups but not blockers on their own.

@@ -0,0 +1,7 @@
export function cartTotal(items: { price: number; qty: number }[]): number {
let total = 0;
for (let i = 0; i <= items.length; i++) {

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 loop condition i <= items.length is an off-by-one error and should be i < items.length. This will cause an error when accessing an out-of-bounds index.
Severity: CRITICAL

Suggested Fix

Change the loop condition from i <= items.length to i < items.length to correctly iterate over the array indices without going out of bounds.

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#L3

Potential issue: The `for` loop iterates one time too many due to the condition `i <=
items.length`. On the final iteration, `i` will be equal to `items.length`, making
`items[i]` `undefined`. Attempting to access the `.price` property on this `undefined`
value will throw a `TypeError`, causing the function to crash for any cart that is not
empty.

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

@@ -0,0 +1,4 @@
export function canRefund(deliveredAt: Date, now = new Date()): boolean {
const days = (now.getTime() - deliveredAt.getTime()) / 86400;

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 millisecond timestamp difference is divided by the number of seconds in a day (86400) instead of milliseconds, making the refund window 1000x shorter than intended.
Severity: HIGH

Suggested Fix

To correctly convert the millisecond difference to days, divide by 86_400_000 (or 1000 * 60 * 60 * 24) instead of 86400.

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/refund-window.ts#L2

Potential issue: The calculation for the number of days since delivery divides a
millisecond timestamp difference by `86400` (the number of seconds in a day) instead of
`86_400_000` (the number of milliseconds in a day). This results in the calculated
`days` value being 1000 times larger than the actual value. Consequently, an intended
30-day refund window is effectively reduced to approximately 43 minutes, causing valid
refund requests to be incorrectly denied.

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

@@ -0,0 +1,3 @@
export function pointsFor(totalCents: number): number {
return Math.round(totalCents / 100) * 1.5;

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 loyalty point calculation can result in fractional points because the final result of ... * 1.5 is not rounded to an integer.
Severity: MEDIUM

Suggested Fix

Wrap the entire calculation in Math.round() to ensure the final result is always an integer. The expression should be Math.round(Math.round(totalCents / 100) * 1.5).

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/loyalty.ts#L2

Potential issue: The loyalty point calculation `Math.round(totalCents / 100) * 1.5` can
produce fractional numbers. Since loyalty point systems typically use integers, this may
lead to unexpected behavior, such as data truncation if stored in an integer database
column or display issues in the UI. For example, a purchase of $2.50 (`totalCents =
250`) would result in 4.5 points.

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

@sergical
sergical merged commit 8b3b0ca into store Oct 2, 2026
6 checks passed
@sergical
sergical deleted the pricing-helpers branch October 2, 2026 15:18

@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 3 potential issues.

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 e17dc4d. Configure here.

export function cartTotal(items: { price: number; qty: number }[]): number {
let total = 0;
for (let i = 0; i <= items.length; i++) {
total += 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.

Cart total loop overflows

High Severity

The loop condition uses <= against items.length, so the last iteration reads past the end of the array and throws when it dereferences undefined. cartTotal therefore crashes for every cart, including an empty one.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e17dc4d. Configure here.

@@ -0,0 +1,4 @@
export function canRefund(deliveredAt: Date, now = new Date()): boolean {
const days = (now.getTime() - deliveredAt.getTime()) / 86400;
return days <= 30;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Refund window uses wrong units

High Severity

canRefund divides a millisecond timestamp delta by 86400 (seconds in a day) instead of milliseconds in a day. The days &lt;= 30 check then treats the refund window as about 43 minutes, so nearly every eligible return is rejected.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e17dc4d. Configure here.

@@ -0,0 +1,3 @@
export function formatUsd(cents: number): string {
return "$" + cents / 100;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Currency drops trailing zeros

Medium Severity

formatUsd concatenates cents / 100 without forcing two decimal places, so whole-dollar and single-digit-cent amounts lose trailing zeros. Checkout, receipts, and confirmation emails would show incomplete money values.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e17dc4d. Configure here.

@linear-code

linear-code Bot commented Oct 5, 2026

Copy link
Copy Markdown

MAR-1676

sergical added a commit that referenced this pull request Oct 7, 2026
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