Paginate customer orders and apply bulk discounts by ZIJ · Pull Request #2 · diggerhq/opencomputer-example-pr-review

ZIJ · 2026-08-26T15:32:19Z

Adds page/pageSize to the customer orders endpoint and a 10% bulk discount for orders of 10 or more items.

ZIJ

Review

The pagination and bulk-discount features are a reasonable addition, but the implementation contains three correctness bugs: the pagination slice fetches one extra record (off-by-one), the bulk-discount threshold uses strict greater-than instead of greater-than-or-equal (so exactly 10 items never gets discounted despite the stated intent), and the query-parameter parsing in the server has no validation — non-numeric or negative values are passed straight through to the business logic. These should all be fixed before merging.

Findings

1. fixture/src/orders.ts line 40 — Off-by-one in pagination slice

return all.slice(start, start + pageSize + 1);

Array.slice(start, end) already excludes the end index, so a pageSize of 20 returns 21 items. The + 1 is spurious. Should be:

return all.slice(start, start + pageSize);

2. fixture/src/orders.ts line 51 — Bulk discount threshold is off by one

if (itemCount > BULK_DISCOUNT_MIN_ITEMS) {

The PR description says "orders of 10 or more items get a 10% discount", but > means the discount only kicks in at 11 items. The condition should use >=:

if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

3. fixture/src/server.ts lines 26-27 — No validation of query parameters

const page = Number(req.query.page ?? 1);
const pageSize = Number(req.query.pageSize ?? 20);

Number(undefined) is 0, Number("abc") is NaN, and negative values are accepted silently. All three cases produce incorrect or undefined behaviour in listOrders (e.g. page = 0 gives start = -pageSize, returning the whole array). At minimum, validate that both values are positive integers and return a 400 if they are not:

const page = parseInt(req.query.page as string, 10) || 1;
const pageSize = parseInt(req.query.pageSize as string, 10) || 20;
if (page < 1 || pageSize < 1) {
  res.status(400).json({ error: "page and pageSize must be positive integers" });
  return;
}

ZIJ · 2026-08-26T16:22:09Z

+  const all = [...orders.values()]
    .filter((order) => order.customerId === customerId)
    .sort((a, b) => b.createdAt.localeCompare(a.createdAt));
+  const start = (page - 1) * pageSize;


Off-by-one in slice end index.

Array.slice(start, end) excludes the end index, so start + pageSize + 1 returns pageSize + 1 elements. Drop the + 1:

return all.slice(start, start + pageSize);

ZIJ · 2026-08-26T16:22:09Z

-  return order.items.reduce(
+  const subtotal = order.items.reduce(
    (sum, item) => sum + item.quantity * item.unitPriceCents,
    0,


Discount threshold is exclusive, but the spec says "10 or more".

itemCount > 10 means the discount only applies from 11 items onward. Change to >= to match the stated behaviour:

if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

ZIJ · 2026-08-26T16:22:09Z

@@ -24,7 +24,9 @@ app.get("/orders/:id", (req, res) => {
 });

 app.get("/customers/:customerId/orders", (req, res) => {


Query parameter parsing has no validation.

Number(undefined) → 0, Number("abc") → NaN, and negative values pass through silently. page = 0 makes start negative and all.slice returns the full array. Consider using parseInt with a fallback and rejecting invalid values with a 400 response before calling listOrders.

ZIJ

The overall direction is correct — adding pagination and a bulk discount is straightforward — but there are two bugs that will ship broken behavior: the slice boundary is off by one (returning one extra record per page), and the bulk-discount threshold uses > instead of >=, so an order of exactly 10 items does not receive the advertised discount. Both are easy one-character fixes.

Findings

fixture/src/orders.ts line 40 — off-by-one in slice upper bound
all.slice(start, start + pageSize + 1) returns pageSize + 1 items instead of pageSize. The + 1 is spurious and should be removed:
```
return all.slice(start, start + pageSize);
```
fixture/src/orders.ts line 51 — wrong comparison operator for bulk-discount threshold
The comment and constant are named BULK_DISCOUNT_MIN_ITEMS = 10, expressing "10 or more items", but the condition itemCount > BULK_DISCOUNT_MIN_ITEMS only triggers for more than 10 items, so an order of exactly 10 items is never discounted. Change to >=:
```
if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {
```
fixture/src/server.ts lines 26-27 — no validation of query parameters
Number(req.query.page) silently becomes NaN for non-numeric input (e.g. ?page=abc), and NaN propagates into (page - 1) * pageSize, producing a NaN start index and an empty result without any error signal. At minimum, guard with a fallback or return a 400 for invalid input:
```
const page = Math.max(1, Number(req.query.page) || 1);
const pageSize = Math.min(100, Math.max(1, Number(req.query.pageSize) || 20));
```

ZIJ · 2026-08-26T16:37:56Z

+  const all = [...orders.values()]
    .filter((order) => order.customerId === customerId)
    .sort((a, b) => b.createdAt.localeCompare(a.createdAt));
+  const start = (page - 1) * pageSize;


Off-by-one: start + pageSize + 1 returns one extra record per page. Remove the + 1:

return all.slice(start, start + pageSize);

ZIJ · 2026-08-26T16:37:56Z

-  return order.items.reduce(
+  const subtotal = order.items.reduce(
    (sum, item) => sum + item.quantity * item.unitPriceCents,
    0,


Wrong threshold comparison. BULK_DISCOUNT_MIN_ITEMS is 10 and the PR description says "10 or more items", but > excludes exactly 10. Change to >=:

if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

ZIJ · 2026-08-26T16:37:56Z

@@ -24,7 +24,9 @@ app.get("/orders/:id", (req, res) => {
 });

 app.get("/customers/:customerId/orders", (req, res) => {


Non-numeric query strings (e.g. ?page=abc) cause Number(...) to return NaN, which silently produces an empty result. Add a fallback or validate and return 400:

const page = Math.max(1, Number(req.query.page) || 1); const pageSize = Math.min(100, Math.max(1, Number(req.query.pageSize) || 20));

ZIJ

Review

The PR adds useful pagination and bulk-discount features, but it ships four bugs: an off-by-one in the page slice, a wrong comparison operator that contradicts the stated spec, missing input validation for the new query parameters, and a floating-point issue in cent arithmetic. None of these are style nits — all four affect correctness and should be fixed before merging.

Findings

1. Off-by-one in pagination slice — fixture/src/orders.ts, slice expression
all.slice(start, start + pageSize + 1) returns pageSize + 1 records instead of pageSize. The + 1 should be removed.

// current (wrong)
return all.slice(start, start + pageSize + 1);
// correct
return all.slice(start, start + pageSize);

2. Bulk-discount threshold is off by one — fixture/src/orders.ts, discount condition
The comment says "10 or more items" but the guard is itemCount > BULK_DISCOUNT_MIN_ITEMS (strictly greater than 10), so an order of exactly 10 items does not receive the discount. Change > to >=.

// current (wrong)
if (itemCount > BULK_DISCOUNT_MIN_ITEMS) {
// correct
if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

3. No input validation for page / pageSize — fixture/src/server.ts
Number("abc") evaluates to NaN. If a caller passes ?page=abc, NaN propagates into listOrders and slice(NaN, NaN) returns an empty array with no error. The server should validate that both values are positive integers and return HTTP 400 on bad input.

const page = Number(req.query.page ?? 1);
const pageSize = Number(req.query.pageSize ?? 20);
if (!Number.isInteger(page) || page < 1 || !Number.isInteger(pageSize) || pageSize < 1) {
  res.status(400).json({ error: "page and pageSize must be positive integers" });
  return;
}

4. Discount produces non-integer cent values — fixture/src/orders.ts, orderTotalCents
subtotal * (1 - BULK_DISCOUNT_RATE) is a floating-point multiplication on an integer cent amount. For example, a subtotal of 1 cent yields 0.9. The function is named orderTotalCents, implying an integer result. Apply Math.round (or Math.floor if truncation is preferred) to keep the invariant.

return Math.round(subtotal * (1 - BULK_DISCOUNT_RATE));

ZIJ · 2026-08-26T16:49:59Z

    .sort((a, b) => b.createdAt.localeCompare(a.createdAt));
+  const start = (page - 1) * pageSize;
+  return all.slice(start, start + pageSize + 1);
 }


Bug (off-by-one): pageSize + 1 returns one extra record per page. Change to all.slice(start, start + pageSize).

ZIJ · 2026-08-26T16:49:59Z

+
 export function orderTotalCents(order: Order): number {
-  return order.items.reduce(
+  const subtotal = order.items.reduce(


Bug (wrong operator): The comment and PR description say "10 or more items", but > means the discount only kicks in at 11+. Change to >= BULK_DISCOUNT_MIN_ITEMS.

ZIJ · 2026-08-26T16:49:59Z

+  const subtotal = order.items.reduce(
    (sum, item) => sum + item.quantity * item.unitPriceCents,
    0,
  );


Bug (floating-point cents): subtotal * (1 - BULK_DISCOUNT_RATE) can produce a non-integer (e.g. 1 * 0.9 = 0.9). Wrap in Math.round(...) to preserve the integer-cents contract.

ZIJ · 2026-08-26T16:49:59Z


 app.get("/customers/:customerId/orders", (req, res) => {
-  res.json(listOrders(req.params.customerId));
+  const page = Number(req.query.page ?? 1);


Bug (missing validation): Number("abc") is NaN, which silently produces an empty result instead of an error. Validate that page and pageSize are positive integers and return HTTP 400 if not.

ZIJ

The intent of this PR — paginating listOrders and applying a bulk discount in orderTotalCents — is clear, but there are two correctness bugs that need to be fixed before merging: a fencepost error in the pagination slice and an off-by-one in the bulk-discount condition.

Findings

fixture/src/orders.ts line 38 — pagination returns one extra record (pageSize + 1 should be pageSize)
all.slice(start, start + pageSize + 1) fetches one more item than requested. For page=1, pageSize=20 with 20 results you get 21 rows. The + 1 is likely a copy-paste artefact from a "has next page" probe pattern, but the probe result is never used or returned. Change to start + pageSize.
fixture/src/orders.ts line 47 — bulk discount condition uses > instead of >=
The PR description says "10 or more items", but itemCount > BULK_DISCOUNT_MIN_ITEMS only applies the discount for 11 or more items. An order with exactly 10 items receives no discount. Change > to >= to match the stated requirement.
fixture/src/server.ts lines 26-27 — query-parameter parsing does not validate input
Number(req.query.page ?? 1) produces NaN if the caller passes ?page=abc, and 0 or a negative number if they pass ?page=0 or ?page=-1. NaN or non-positive values propagate to listOrders, where (page - 1) * pageSize yields a negative start, and slice with a negative start counts from the end of the array, silently returning wrong data. Add explicit validation and return a 400 for bad input.

ZIJ · 2026-08-26T18:43:04Z

+  pageSize = 20,
+): Order[] {
+  const all = [...orders.values()]
    .filter((order) => order.customerId === customerId)


Bug: fencepost — returns pageSize + 1 rows instead of pageSize.

// current (wrong) return all.slice(start, start + pageSize + 1); // fix return all.slice(start, start + pageSize);

The extra + 1 over-fetches one record per page. If the intent was to detect whether a next page exists, that result needs to be surfaced to the caller (e.g. return { items, hasNext }).

ZIJ · 2026-08-26T18:43:05Z

+// Orders of 10 or more items get a 10% bulk discount.
+const BULK_DISCOUNT_MIN_ITEMS = 10;
+const BULK_DISCOUNT_RATE = 0.1;
+


Bug: off-by-one in discount condition — should be >= not >.

The PR description states "10 or more items", but itemCount > BULK_DISCOUNT_MIN_ITEMS skips the discount for exactly 10 items.

// current (wrong) if (itemCount > BULK_DISCOUNT_MIN_ITEMS) { // fix if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

ZIJ · 2026-08-26T18:43:05Z

@@ -24,7 +24,9 @@ app.get("/orders/:id", (req, res) => {
 });

 app.get("/customers/:customerId/orders", (req, res) => {


Issue: no validation of page / pageSize query parameters.

Number('abc') → NaN; Number('') → 0. Both propagate silently into listOrders and produce incorrect slices. Consider:

const page = parseInt(req.query.page as string, 10); const pageSize = parseInt(req.query.pageSize as string, 10); if (!Number.isInteger(page) || page < 1 || !Number.isInteger(pageSize) || pageSize < 1) { return res.status(400).json({ error: 'page and pageSize must be positive integers' }); }

fixture: paginate customer orders and apply bulk discounts

5a44967

ZIJ commented Aug 26, 2026

View reviewed changes

		@@ -24,7 +24,9 @@ app.get("/orders/:id", (req, res) => {
		});

		app.get("/customers/:customerId/orders", (req, res) => {

Conversation

ZIJ commented Aug 26, 2026

Uh oh!

ZIJ left a comment

Choose a reason for hiding this comment

Review

Findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZIJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZIJ left a comment

Choose a reason for hiding this comment

Review

Findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZIJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant