Skip to content

Fix the refund window units and loyalty rounding - #10

Merged
sergical merged 1 commit into
storefrom
fix/refund-window-units
Oct 6, 2026
Merged

sergical merged 1 commit into
storefrom
fix/refund-window-units

Conversation

@sergical

@sergical sergical commented Oct 2, 2026

Copy link
Copy Markdown
Member

Follow-up to #9. The refund window divided milliseconds by seconds per day, so the 30-day window was effectively open forever. Loyalty points now round once, after the multiplier. The cart total loop no longer reads one past the end and rejects negative quantities.

Created with Claude Code

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Verdict: Approve — the three fixes are correct and the diff is low-risk; only minor polish suggestions remain.

Correctness

  • cart-total.ts: loop bound fixed from <= to <, eliminating the out-of-bounds access on items[items.length]. ✅
  • cart-total.ts: new negative-quantity guard is a behavior change (previously a negative qty silently reduced the total; now it throws). This looks intentional and reasonable, but worth calling out explicitly in the PR description/tests since it's a new user-facing contract (a RangeError is now possible where none was before).
  • loyalty.ts: changing Math.round(totalCents / 100) * 1.5 to Math.round((totalCents / 100) * 1.5) matches the new doc comment ("rounded once after the multiplier") and fixes double-rounding/truncation drift for non-integer dollar amounts. ✅
  • refund-window.ts: divisor corrected from 86400 (seconds/day) to 86_400_000 (ms/day), matching getTime()'s millisecond units. This was a real bug (previously days was ~1000x too large, making canRefund return false almost always). ✅

No remaining logic errors or regressions found.

Style

  • cart-total.ts: the thrown message uses the raw loop index i; consider clarifying it's a positional index (e.g., "at index ${i}") or using a stable item identifier if one exists, to avoid ambiguity for callers.
  • loyalty.ts: the new comment is helpful, but the magic numbers 100 and 1.5 could be pulled into named constants (e.g., CENTS_PER_DOLLAR, POINTS_PER_DOLLAR) to make the code self-documenting and reduce reliance on the comment staying in sync.
  • refund-window.ts: good use of numeric separators (86_400_000); consider extracting to a MS_PER_DAY constant for clarity and reuse.

These are non-blocking suggestions; the core fixes are sound.

@linear-code

linear-code Bot commented Oct 5, 2026

Copy link
Copy Markdown

MAR-1676

@sergical
sergical merged commit 2eb46c3 into store Oct 6, 2026
6 checks passed
@sergical
sergical deleted the fix/refund-window-units branch October 6, 2026 02:53
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