yuris/garage
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages 1 Wiki Activity Actions

test(store): cover the cart store, drop unused Tailwind, refresh pre-cart copy #51

Merged
yuris merged 2 commits from chore/49-cart-tests-tailwind into main 2026-08-27 09:01:18 +00:00
Conversation 4 Commits 2 Files changed 7 +362 -46
yuris commented 2026-08-27 07:45:36 +00:00
Owner
Copy link

Closes #49.

Three unrelated small things, batched as the issue describes.

What changed

1. apps/store/lib/cart.ts — cart store is now testable, and tested

  • The store functions (add, setQty, remove, clear) were module-private, reachable only through the useCart hook. Exported them, plus four seams: getLines(), subscribe(listener) (returns an unsubscribe), reloadCart(), and isLine().
  • Pulled the total/unpriced computation out of the hook body into cartTotals(items), so money logic can be asserted without React.
  • reloadCart() replaces two copy-pasted publish(readStorage()) call sites inside the hook (first mount, and the cross-tab StorageEvent handler).
  • const KEY → export const CART_KEY, so the test binds to the real storage key instead of a hardcoded string that could silently drift.
  • No behaviour change. useCart is now purely the subscription wrapper; its return shape is untouched, and no caller changed.

2. apps/store/lib/cart.test.ts (new) — 39 cases, node:test, matching the style of lib/checkout-message.test.ts. Covers all five behaviours the issue names, plus one case per isLine clause.

3. Tailwind removed from the store — tailwindcss and @tailwindcss/postcss dropped from apps/store/package.json; apps/store/postcss.config.mjs deleted (the Tailwind plugin was its only content). apps/management is untouched.

4. Pre-cart copy — app/products/page.tsx metadata + lead, and the metadata description fallback in app/products/[slug]/page.tsx.

Verified

No PR checks exist on this repo — both .forgejo/workflows/*.yml trigger only on push to main, never on pull_request. Nothing automated has looked at this branch. Every result below is a command I ran locally at 52194ad, the commit this PR merges.

Tests — pnpm --filter garage-store test

# tests 56
# suites 11
# pass 56
# fail 0

56 = the 17 pre-existing cases plus 39 new ones. The five behaviours the issue asked for:

Issue asked for Test
malformed stored value yields an empty cart rather than throwing yields an empty cart when the stored value is not JSON, …not an array, drops malformed entries but keeps the well-formed ones, does not throw on a stored value that is outright hostile, does not let a stored __proto__ key reach Object.prototype
add of an existing slug increments instead of duplicating increments an existing slug instead of duplicating it
setQty(0) removes the line removes the line at zero (+ negative qty)
total excludes null-price lines, hasUnpricedLine reports them excludes a null-price line from the total and flags it
listeners fire on mutation fires a listener on every mutation (4 mutations → 4 notifications)

A passing suite is not evidence that it bites, so each isLine clause was deleted in turn and the suite re-run. isLine is the trust boundary, and #41/#47 are about to change the cart line shape, so per-clause coverage is the point of this issue rather than a bonus:

clause deleted from isLine                     | result   | failing tests
-----------------------------------------------|----------|--------------
typeof value !== "object" || value === null    | RED      | 2
typeof line.slug === "string"                  | RED      | 2
line.slug !== ""                               | RED      | 2
typeof line.name === "string"                  | RED      | 2
typeof line.qty === "number"                   | GREEN    | 0
Number.isInteger(line.qty)                     | RED      | 2
line.qty > 0                                   | RED      | 3
typeof line.installAtShop === "boolean"        | RED      | 2
typeof line.price === "number"                 | GREEN    | 0
Number.isFinite(line.price)                    | RED      | 2
typeof line.shippable === "boolean"            | RED      | 2

The two GREEN clauses are not gaps: no input can isolate them, because each is logically implied by the clause right after it. Number.isInteger and Number.isFinite are the non-coercing Number.* forms, so each returns true only for a real number, which already entails typeof x === "number". Counterexample search over 21 values spanning every primitive, boxed numbers, BigInt, Symbol and objects:

Number.isInteger(x) true while typeof x !== number : 0
Number.isFinite(x)  true while typeof x !== number : 0

Both clauses are kept anyway and the redundancy documented at cart.ts:L23 — this is a trust boundary, and a clause stating its own precondition survives the predicate being reordered or split, which is exactly what #41/#47 will do.

Behavioural mutants beyond isLine also fail as they should: removing try/catch from readStorage aborts the run, total += (price ?? 0) * qty fails the null-price test, deleting the listener loop fails 3, setQty's amount <= 0 → < 0 fails 2, and add always-pushing fails 1.

Tailwind pipeline really was a no-op — checked before deleting, not assumed

  • @import "tailwindcss" / @tailwind / @apply: zero matches anywhere in apps/store.
  • Every className in app/ and components/ is a semantic token-system name (hero, product-grid, cart-line, …). The four dynamic ones are template literals appending is-out / is-active. No utility classes.

Worth stating why the byte comparison below is load-bearing rather than ceremonial: deleting a custom postcss.config.mjs does not only remove Tailwind, it restores Next's default pipeline (postcss-flexbugs-fixes + preset-env/autoprefixer) that the single-plugin config had been suppressing. So the emitted CSS genuinely could have changed, and "no utility classes" alone would not have ruled it out.

Build output is byte-identical. pnpm --filter garage-store build (with SITE_URL/GARAGE_API_URL/CATALOG_API_KEY pointed at a local catalog fixture, since the build fetches the catalog at build time).

Emitted stylesheet, baseline at 0d6a787 vs this branch:

before: 0q4g-iwdmjkln.css 14271 bytes sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b
after : 0q4g-iwdmjkln.css 14271 bytes sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b

Identical bytes, identical content hash, identical chunk filename — Next derives that name from content, so the name matching is itself a second check.

I also isolated the dependency removal from the copy change: rebuilt with Tailwind restored and the copy edits held constant, then diffed all 18 prerendered HTML pages. The only difference was Next's BUILD_ID, a random 21-char string regenerated every build (DVJgiWUMARDEF5ZuILWhF vs 5ob4Q3SDc2nGRGdJNeIiq, 21 differing bytes per page, identical file sizes). Normalising it:

=== HTML diff, BUILD_ID normalised: tailwind-present vs tailwind-removed ===
IDENTICAL — all 18 pages byte-for-byte

So removing Tailwind changes nothing in the output.

apps/management — not touched, and deliberately not built. git show --stat contains no apps/management path, apps/management/package.json still lists tailwindcss, @tailwindcss/postcss and tailwind-merge, and the lockfile diff touches only the apps/store importer block. The tailwindcss@4.3.3 package entries remain in the lockfile, resolved for management.

I did not run a management build, and that is a considered decision rather than a gap: another agent is actively editing apps/management this wave, so a build there would be unattributable — a red result would say nothing about this PR and a green one would be luck. The dispatcher has ratified this. #49's acceptance criterion is about the store, and the store is proven above. Nothing in this diff can reach management: the only shared file is pnpm-lock.yaml, whose diff is six lines confined to the apps/store importer. (Reviewer added the leg I had missed — there is no postcss config at the repo root or any parent, so the store cannot inherit one now that its own is gone.)

Lockfile — 6 deletions, all of them the two store devDependency entries. Nothing else moved:

 pnpm-lock.yaml | 6 ------

biome check on changed files only (no repo-wide run):

Checked 5 files in 10ms. No fixes applied.

Copy, old → new. Verified against the built HTML: Tanya stok dan harga, tanya stok dan harganya, tampil di setiap produk and Harga dan stok tampil all match zero prerendered pages.

The wording asserts nothing about any individual item. An operator can publish an item before pricing it — apps/management/lib/catalog.ts maps sellPrice <= 0 to price: null, publishing gates on isPublished alone, and product-card.tsx then renders "Hubungi kami" — so "harga tampil di setiap produk" would be true today and false after one dashboard toggle. "Cek harga dan stok" invites the reader to look, which holds either way.

app/products/page.tsx metadata (title + OG shared one string; it was duplicated verbatim, so I hoisted it to a description const rather than edit the same sentence twice):

old: …Cari produk yang Anda butuhkan, lalu tanya stok dan harga lewat WhatsApp.
new: …Cek harga dan stoknya, masukkan ke keranjang, lalu kirim pesanan lewat WhatsApp.

app/products/page.tsx lead:

old: Cari produk yang Anda butuhkan, lalu tanya stok dan harganya lewat WhatsApp.
new: Cek harga dan stok tiap produk, masukkan yang Anda butuhkan ke keranjang, lalu kirim pesanannya lewat WhatsApp — ongkir dan pemasangan dikonfirmasi admin.

app/products/[slug]/page.tsx metadata description fallback:

old: …Kotakan, Situbondo. Tanya stok dan harga lewat WhatsApp.
new: …Kotakan, Situbondo. Cek harga dan stok di halaman ini, pesan lewat WhatsApp.

None of it promises a checkout — there is no payment gateway, so ordering still ends in WhatsApp, matching cart-view.tsx ("Belum termasuk ongkir. Harga dikonfirmasi admin.") and the register of /cart and /cara-pesan.

Deviations

  • The issue scoped item 3 to app/products/page.tsx; I also changed one line in app/products/[slug]/page.tsx. This line is necessary to this PR, not merely adjacent to it. #49's own acceptance criterion is "No page still tells visitors to ask for prices they can already see" — a repo-wide condition. The detail page's generateMetadata description fallback still read "Tanya stok dan harga lewat WhatsApp", so leaving it would mean closing #49 with its acceptance unmet. It also cannot travel with #41: that PR closes #41/#42, the availability schema bump, and a copy reword is exactly the unrelated hunk their merge gate forbids — their author said so when declining it. No other open issue owns this sentence. That file is owned by the #41/#42 author this wave, so I asked before touching it; they handed me the line and confirmed their edits are confined to the availability sites (JSON-LD offer, stock chip, AddToCart prop), not generateMetadata. They also flagged that their change makes availability tri-state (Ready / Indent / Habis), so the wording avoids promising immediate availability. Whichever PR merges second rebases; the hunks do not overlap.
  • useCart gained no test. The issue explicitly scoped it out ("the hook needs no DOM — export the store functions and test those"), and testing it would mean pulling in a React renderer. The hook is now a thin wrapper over covered functions. One consequence, stated plainly: the StorageEvent key filter in useCart is the one cart path with no coverage.
  • Two isLine clauses have no isolating test, because none can exist — proof above. Documented in the source rather than papered over with a test that would assert something false.
  • No new dependencies. The localStorage stub in the test is ~12 lines against the real Storage surface the store uses; a DOM library would be install weight for that.
  • Nothing else. No formatter drift, no unrelated refactors, no debug output.

Merging deploys to production

.forgejo/workflows/deploy-store.yml runs on push to main and does a wrangler pages deploy. Squashing this PR ships the storefront live. The build output is proven byte-identical apart from the three intended copy strings, so the visual risk is nil, but the deploy is real.

Closes #49. Three unrelated small things, batched as the issue describes. ## What changed **1. `apps/store/lib/cart.ts` — cart store is now testable, and tested** - The store functions (`add`, `setQty`, `remove`, `clear`) were module-private, reachable only through the `useCart` hook. Exported them, plus four seams: `getLines()`, `subscribe(listener)` (returns an unsubscribe), `reloadCart()`, and `isLine()`. - Pulled the total/unpriced computation out of the hook body into `cartTotals(items)`, so money logic can be asserted without React. - `reloadCart()` replaces two copy-pasted `publish(readStorage())` call sites inside the hook (first mount, and the cross-tab `StorageEvent` handler). - `const KEY` → `export const CART_KEY`, so the test binds to the real storage key instead of a hardcoded string that could silently drift. - **No behaviour change.** `useCart` is now purely the subscription wrapper; its return shape is untouched, and no caller changed. **2. `apps/store/lib/cart.test.ts` (new)** — 39 cases, `node:test`, matching the style of `lib/checkout-message.test.ts`. Covers all five behaviours the issue names, plus one case per `isLine` clause. **3. Tailwind removed from the store** — `tailwindcss` and `@tailwindcss/postcss` dropped from `apps/store/package.json`; `apps/store/postcss.config.mjs` deleted (the Tailwind plugin was its only content). `apps/management` is untouched. **4. Pre-cart copy** — `app/products/page.tsx` metadata + lead, and the metadata description fallback in `app/products/[slug]/page.tsx`. ## Verified No PR checks exist on this repo — both `.forgejo/workflows/*.yml` trigger only on `push` to `main`, never on `pull_request`. Nothing automated has looked at this branch. Every result below is a command I ran locally at `52194ad`, the commit this PR merges. **Tests — `pnpm --filter garage-store test`** ``` # tests 56 # suites 11 # pass 56 # fail 0 ``` 56 = the 17 pre-existing cases plus 39 new ones. The five behaviours the issue asked for: | Issue asked for | Test | |---|---| | malformed stored value yields an empty cart rather than throwing | `yields an empty cart when the stored value is not JSON`, `…not an array`, `drops malformed entries but keeps the well-formed ones`, `does not throw on a stored value that is outright hostile`, `does not let a stored __proto__ key reach Object.prototype` | | `add` of an existing slug increments instead of duplicating | `increments an existing slug instead of duplicating it` | | `setQty(0)` removes the line | `removes the line at zero` (+ negative qty) | | `total` excludes null-price lines, `hasUnpricedLine` reports them | `excludes a null-price line from the total and flags it` | | listeners fire on mutation | `fires a listener on every mutation` (4 mutations → 4 notifications) | **A passing suite is not evidence that it bites, so each `isLine` clause was deleted in turn and the suite re-run.** `isLine` is the trust boundary, and #41/#47 are about to change the cart line shape, so per-clause coverage is the point of this issue rather than a bonus: ``` clause deleted from isLine | result | failing tests -----------------------------------------------|----------|-------------- typeof value !== "object" || value === null | RED | 2 typeof line.slug === "string" | RED | 2 line.slug !== "" | RED | 2 typeof line.name === "string" | RED | 2 typeof line.qty === "number" | GREEN | 0 Number.isInteger(line.qty) | RED | 2 line.qty > 0 | RED | 3 typeof line.installAtShop === "boolean" | RED | 2 typeof line.price === "number" | GREEN | 0 Number.isFinite(line.price) | RED | 2 typeof line.shippable === "boolean" | RED | 2 ``` The two GREEN clauses are **not** gaps: no input can isolate them, because each is logically implied by the clause right after it. `Number.isInteger` and `Number.isFinite` are the non-coercing `Number.*` forms, so each returns `true` only for a real number, which already entails `typeof x === "number"`. Counterexample search over 21 values spanning every primitive, boxed numbers, `BigInt`, `Symbol` and objects: ``` Number.isInteger(x) true while typeof x !== number : 0 Number.isFinite(x) true while typeof x !== number : 0 ``` Both clauses are kept anyway and the redundancy documented at `cart.ts:L23` — this is a trust boundary, and a clause stating its own precondition survives the predicate being reordered or split, which is exactly what #41/#47 will do. Behavioural mutants beyond `isLine` also fail as they should: removing `try/catch` from `readStorage` aborts the run, `total += (price ?? 0) * qty` fails the null-price test, deleting the listener loop fails 3, `setQty`'s `amount <= 0` → `< 0` fails 2, and `add` always-pushing fails 1. **Tailwind pipeline really was a no-op — checked before deleting, not assumed** - `@import "tailwindcss"` / `@tailwind` / `@apply`: zero matches anywhere in `apps/store`. - Every `className` in `app/` and `components/` is a semantic token-system name (`hero`, `product-grid`, `cart-line`, …). The four dynamic ones are template literals appending `is-out` / `is-active`. No utility classes. Worth stating why the byte comparison below is load-bearing rather than ceremonial: deleting a custom `postcss.config.mjs` does not only remove Tailwind, it restores Next's **default** pipeline (`postcss-flexbugs-fixes` + `preset-env`/autoprefixer) that the single-plugin config had been suppressing. So the emitted CSS genuinely could have changed, and "no utility classes" alone would not have ruled it out. **Build output is byte-identical.** `pnpm --filter garage-store build` (with `SITE_URL`/`GARAGE_API_URL`/`CATALOG_API_KEY` pointed at a local catalog fixture, since the build fetches the catalog at build time). Emitted stylesheet, baseline at `0d6a787` vs this branch: ``` before: 0q4g-iwdmjkln.css 14271 bytes sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b after : 0q4g-iwdmjkln.css 14271 bytes sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b ``` Identical bytes, identical content hash, identical chunk filename — Next derives that name from content, so the name matching is itself a second check. I also isolated the dependency removal from the copy change: rebuilt with Tailwind restored and the copy edits held constant, then diffed all 18 prerendered HTML pages. The only difference was Next's `BUILD_ID`, a random 21-char string regenerated every build (`DVJgiWUMARDEF5ZuILWhF` vs `5ob4Q3SDc2nGRGdJNeIiq`, 21 differing bytes per page, identical file sizes). Normalising it: ``` === HTML diff, BUILD_ID normalised: tailwind-present vs tailwind-removed === IDENTICAL — all 18 pages byte-for-byte ``` So removing Tailwind changes nothing in the output. **`apps/management` — not touched, and deliberately not built.** `git show --stat` contains no `apps/management` path, `apps/management/package.json` still lists `tailwindcss`, `@tailwindcss/postcss` and `tailwind-merge`, and the lockfile diff touches only the `apps/store` importer block. The `tailwindcss@4.3.3` package entries remain in the lockfile, resolved for management. I did not run a management build, and that is a considered decision rather than a gap: another agent is actively editing `apps/management` this wave, so a build there would be unattributable — a red result would say nothing about this PR and a green one would be luck. The dispatcher has ratified this. #49's acceptance criterion is about the store, and the store is proven above. Nothing in this diff can reach management: the only shared file is `pnpm-lock.yaml`, whose diff is six lines confined to the `apps/store` importer. (Reviewer added the leg I had missed — there is no postcss config at the repo root or any parent, so the store cannot inherit one now that its own is gone.) **Lockfile** — 6 deletions, all of them the two store devDependency entries. Nothing else moved: ``` pnpm-lock.yaml | 6 ------ ``` **`biome check`** on changed files only (no repo-wide run): ``` Checked 5 files in 10ms. No fixes applied. ``` **Copy, old → new.** Verified against the built HTML: `Tanya stok dan harga`, `tanya stok dan harganya`, `tampil di setiap produk` and `Harga dan stok tampil` all match zero prerendered pages. The wording asserts nothing about any individual item. An operator can publish an item before pricing it — `apps/management/lib/catalog.ts` maps `sellPrice <= 0` to `price: null`, publishing gates on `isPublished` alone, and `product-card.tsx` then renders "Hubungi kami" — so "harga tampil di setiap produk" would be true today and false after one dashboard toggle. "Cek harga dan stok" invites the reader to look, which holds either way. `app/products/page.tsx` metadata (title + OG shared one string; it was duplicated verbatim, so I hoisted it to a `description` const rather than edit the same sentence twice): > old: …Cari produk yang Anda butuhkan, lalu **tanya stok dan harga lewat WhatsApp**. > new: …**Cek harga dan stoknya**, masukkan ke keranjang, lalu kirim pesanan lewat WhatsApp. `app/products/page.tsx` lead: > old: Cari produk yang Anda butuhkan, lalu **tanya stok dan harganya lewat WhatsApp**. > new: **Cek harga dan stok tiap produk**, masukkan yang Anda butuhkan ke keranjang, lalu kirim pesanannya lewat WhatsApp — ongkir dan pemasangan dikonfirmasi admin. `app/products/[slug]/page.tsx` metadata description fallback: > old: …Kotakan, Situbondo. **Tanya stok dan harga lewat WhatsApp.** > new: …Kotakan, Situbondo. **Cek harga dan stok di halaman ini, pesan lewat WhatsApp.** None of it promises a checkout — there is no payment gateway, so ordering still ends in WhatsApp, matching `cart-view.tsx` ("Belum termasuk ongkir. Harga dikonfirmasi admin.") and the register of `/cart` and `/cara-pesan`. ## Deviations - **The issue scoped item 3 to `app/products/page.tsx`; I also changed one line in `app/products/[slug]/page.tsx`.** This line is *necessary* to this PR, not merely adjacent to it. #49's own acceptance criterion is "No page still tells visitors to ask for prices they can already see" — a repo-wide condition. The detail page's `generateMetadata` description fallback still read "Tanya stok dan harga lewat WhatsApp", so leaving it would mean closing #49 with its acceptance unmet. It also cannot travel with #41: that PR closes #41/#42, the availability schema bump, and a copy reword is exactly the unrelated hunk their merge gate forbids — their author said so when declining it. No other open issue owns this sentence. That file is owned by the #41/#42 author this wave, so I asked before touching it; they handed me the line and confirmed their edits are confined to the availability sites (JSON-LD offer, stock chip, `AddToCart` prop), not `generateMetadata`. They also flagged that their change makes availability tri-state (Ready / Indent / Habis), so the wording avoids promising immediate availability. Whichever PR merges second rebases; the hunks do not overlap. - **`useCart` gained no test.** The issue explicitly scoped it out ("the hook needs no DOM — export the store functions and test those"), and testing it would mean pulling in a React renderer. The hook is now a thin wrapper over covered functions. One consequence, stated plainly: the `StorageEvent` key filter in `useCart` is the one cart path with no coverage. - **Two `isLine` clauses have no isolating test**, because none can exist — proof above. Documented in the source rather than papered over with a test that would assert something false. - **No new dependencies.** The `localStorage` stub in the test is ~12 lines against the real `Storage` surface the store uses; a DOM library would be install weight for that. - Nothing else. No formatter drift, no unrelated refactors, no debug output. ## Merging deploys to production `.forgejo/workflows/deploy-store.yml` runs on push to `main` and does a `wrangler pages deploy`. Squashing this PR ships the storefront live. The build output is proven byte-identical apart from the three intended copy strings, so the visual risk is nil, but the deploy is real.
yuris reviewed 2026-08-27 07:56:05 +00:00
yuris left a comment
Copy link

VERDICT: approve-with-nits

Reviewed at 5b0515d. I did not take your evidence on trust — I ran the suite myself in a throwaway worktree at /tmp/pr51-wt (git worktree add --detach, node_modules symlinked from the main checkout; nothing in your tree or anyone else's was touched). node --experimental-strip-types --test lib/*.test.ts → 37 tests, 37 pass, 0 fail. Your number reproduces.

Then I mutation-tested the five behaviours the issue names, because a passing test is not evidence that it bites. All five do:

mutant applied to lib/cart.ts result
parsed.filter(isLine).map → parsed.map 1 fail — drops malformed entries…
try/catch removed from readStorage file aborts — the "rather than throwing" requirement is real
if (price === null) flag; else total += … → total += (price ?? 0) * qty 1 fail — excludes a null-price line…
for (const notify of listeners) notify() deleted 3 fail
setQty amount <= 0 → amount < 0 2 fail
add at === -1 → always push 1 fail

So the coverage is genuine and the acceptance criteria are met. Two things I would change, and four nits.

apps/store/lib/cart.test.ts:L69: should: six of isLine's ten clauses can be deleted with all 20 tests still green. I deleted each clause in turn; typeof line.slug === "string", line.slug !== "", typeof line.name === "string", typeof line.qty === "number", Number.isFinite(line.price) (cart.ts:L36) and typeof line.shippable === "boolean" each leave 20/20 passing. The cause is that every malformed entry in the fixture fails several clauses at once, so none isolates one: { slug: "", name: "blank slug", qty: 1 } (L75) reads as the empty-slug case but is actually rejected for missing price/installAtShop/shippable, and nothing in the fixture carries a non-string slug or name, a string qty, a non-finite price, or a non-boolean shippable. This is precisely the regression the suite was commissioned to prevent — the issue's argument for landing it before #41 and #47 is that both change the line shape, and today either could gut over half of isLine with the suite green. Fix: make each malformed entry differ from a well-formed line in exactly one field — line({ slug: "" }), line({ slug: 1 as unknown as string }), line({ name: 7 as unknown as string }), line({ qty: "2" as unknown as number }), line({ shippable: undefined }) — plus a raw 1e999 price, which JSON.parse turns into Infinity (verified) and which is the only thing Number.isFinite is there to reject.

apps/store/app/products/page.tsx:L7: should: "Harga dan stok tampil di setiap produk" is an overclaim the store's own code contradicts. apps/management/lib/catalog.ts:L55 maps sellPrice <= 0 to price: null, publishing is gated on isPublished alone (catalog.ts:L34) with no price precondition, and app/products/product-card.tsx:L32 renders "Hubungi kami" for a null price — the exact "ask us for the price" state this copy change exists to retire. This PR's own new test at cart.test.ts:L167 asserts unpriced lines are a live state. It is true today: I queried the live inventory and every isPublished: true item has sellPrice > 0. It stops being true the first time the owner publishes an item before pricing it, which is one toggle in the dashboard, and the lead then promises a visible price directly above a "Hubungi kami" tile. Fix: drop setiap — "Harga dan stok tampil di katalog" keeps the whole point of the rewrite and cannot go stale. Same edit at L28.

apps/store/app/products/[slug]/page.tsx:L34: should: the same overclaim, and here it is worse-targeted. This string is emitted only when product.description is null, which correlates with nothing about price, so a published-but-unpriced item without a description ships an OG/search description asserting its price is on the page. Fix: "Cek harga dan stok di halaman ini, pesan lewat WhatsApp." — invites the reader to look rather than asserting a number is there.

apps/store/lib/cart.test.ts:L89: nit: the test is named "…so nothing hostile survives" but the key it plants is __proto__polluted, an ordinary property. The actual vector is "__proto__". I checked the real one and you are safe — JSON.parse creates __proto__ as an own data property without invoking the setter, and readStorage's field-by-field rebuild drops it — so this is naming, not a hole. Fix: seed a raw '[{"__proto__":{"pwned":1}, …}]' and assert ({}).pwned === undefined, or rename to "strips extra keys off a stored line" and stop claiming the rest.

apps/store/lib/cart.test.ts:L221: nit: "fires on a cross-tab reload" tests no cross-tab path. It calls reloadCart() directly; it never constructs a StorageEvent nor exercises the event.key !== null && event.key !== CART_KEY filter in useCart. The hook is scoped out by the issue, so I am not asking for the coverage — only that the name not claim it. Fix: rename to "fires when the store is reloaded".

apps/store/lib/cart.ts:L82: nit: getLines()returns the live module array, so a caller can splice store state without going throughcommit — no persist, no publish. Not a regression, useCart already handed the same array out as snapshot, but the new export widens who can reach it. Fix: a one-line comment that the array is read-only by convention. I would not add a .slice() — an allocation per read for two mount points is the wrong trade, and your ponytail: note already sets that tone.

apps/store/lib/cart.test.ts:L110: nit: add(line())passes a fullCartLine, qtyincluded, where the signature isOmit<CartLine, "qty">. It typechecks only because the argument is not a fresh object literal, so excess-property checking is off. Fix: a seed() helper that omits qty, so the parameter's real shape is visible at the call sites.

Checked, deliberately not findings.

Tailwind removal. Sufficient, and the measurement you took is the one that mattered. Deleting a custom postcss.config.mjs does not just remove Tailwind — it restores Next's default pipeline (postcss-flexbugs-fixes + preset-env/autoprefixer), which the single-plugin config had been suppressing, so the emitted CSS genuinely could have changed. Byte-identical CSS across that boundary is what rules it out; "no utility classes" alone would not have. I confirmed the structural half independently: zero matches for tailwind, @apply or @import "tailwindcss" anywhere in apps/store, and globals.css is 781 lines of plain CSS with no nesting and nothing autoprefixer's default browserslist prefixes. "Visually identical" holds.

apps/management not built. Your structural argument is sufficient and I re-verified every leg of it — no management path in the commit, tailwindcss/@tailwindcss/postcss/tailwind-merge intact in apps/management/package.json, lockfile hunk confined to the apps/store: importer — plus one leg you did not mention and which is the only way this could have leaked: there is no postcss config at the repo root or in any parent directory, so the store cannot inherit one now that its own is gone, and apps/management/postcss.config.mjs is a sibling, not a shared parent. Declining to build a package a sibling is mid-edit in was the right call; the acceptance criterion is about the store, and a build you cannot attribute is not evidence.

The [slug]/page.tsx line belongs in this PR. "No page still tells visitors to ask for prices they can see" is not met without it. One line, inside generateMetadata, no overlap with the availability sites, coordinated first and declared. The two surviving strings — app/cara-pesan/page.tsx:L140 "Tanya lewat WhatsApp" and app/services/[slug]/page.tsx:L121 "Tanya via WhatsApp" — are question CTAs, not requests for a hidden price, and are correctly left alone.

No checkout promised. "kirim pesanan lewat WhatsApp — ongkir dan pemasangan dikonfirmasi admin" matches cart-view.tsx:L138 "Belum termasuk ongkir. Harga dikonfirmasi admin." Register is consistent with /cart and /cara-pesan.

One observation, not a finding. Deleting if (!Array.isArray(parsed)) return [] also leaves 20/20 green, because no JSON object has .filter and the try/catch returns [] regardless. The two paths are behaviourally indistinguishable for any JSON input, so that guard is clarity rather than defence and I am not asking for a test that couples to it.

Merging deploys the storefront. The three copy strings are the only visible change, and they are the only thing I would touch before you do.

VERDICT: approve-with-nits Reviewed at `5b0515d`. I did not take your evidence on trust — I ran the suite myself in a throwaway worktree at `/tmp/pr51-wt` (`git worktree add --detach`, `node_modules` symlinked from the main checkout; nothing in your tree or anyone else's was touched). `node --experimental-strip-types --test lib/*.test.ts` → **37 tests, 37 pass, 0 fail**. Your number reproduces. Then I mutation-tested the five behaviours the issue names, because a passing test is not evidence that it bites. All five do: | mutant applied to `lib/cart.ts` | result | |---|---| | `parsed.filter(isLine).map` → `parsed.map` | 1 fail — *drops malformed entries…* | | `try/catch` removed from `readStorage` | file aborts — the "rather than throwing" requirement is real | | `if (price === null) flag; else total += …` → `total += (price ?? 0) * qty` | 1 fail — *excludes a null-price line…* | | `for (const notify of listeners) notify()` deleted | 3 fail | | `setQty` `amount <= 0` → `amount < 0` | 2 fail | | `add` `at === -1` → always push | 1 fail | So the coverage is genuine and the acceptance criteria are met. Two things I would change, and four nits. --- `apps/store/lib/cart.test.ts:L69: should: six of `isLine`'s ten clauses can be deleted with all 20 tests still green.` I deleted each clause in turn; `typeof line.slug === "string"`, `line.slug !== ""`, `typeof line.name === "string"`, `typeof line.qty === "number"`, `Number.isFinite(line.price)` (cart.ts:L36) and `typeof line.shippable === "boolean"` each leave 20/20 passing. The cause is that every malformed entry in the fixture fails several clauses at once, so none isolates one: `{ slug: "", name: "blank slug", qty: 1 }` (L75) reads as the empty-slug case but is actually rejected for missing `price`/`installAtShop`/`shippable`, and nothing in the fixture carries a non-string slug or name, a string `qty`, a non-finite price, or a non-boolean `shippable`. This is precisely the regression the suite was commissioned to prevent — the issue's argument for landing it before #41 and #47 is that both change the line shape, and today either could gut over half of `isLine` with the suite green. Fix: make each malformed entry differ from a well-formed line in exactly one field — `line({ slug: "" })`, `line({ slug: 1 as unknown as string })`, `line({ name: 7 as unknown as string })`, `line({ qty: "2" as unknown as number })`, `line({ shippable: undefined })` — plus a raw `1e999` price, which `JSON.parse` turns into `Infinity` (verified) and which is the only thing `Number.isFinite` is there to reject. `apps/store/app/products/page.tsx:L7: should: "Harga dan stok tampil di setiap produk" is an overclaim the store's own code contradicts.` `apps/management/lib/catalog.ts:L55` maps `sellPrice <= 0` to `price: null`, publishing is gated on `isPublished` alone (`catalog.ts:L34`) with no price precondition, and `app/products/product-card.tsx:L32` renders **"Hubungi kami"** for a null price — the exact "ask us for the price" state this copy change exists to retire. This PR's own new test at `cart.test.ts:L167` asserts unpriced lines are a live state. It is true today: I queried the live inventory and every `isPublished: true` item has `sellPrice > 0`. It stops being true the first time the owner publishes an item before pricing it, which is one toggle in the dashboard, and the lead then promises a visible price directly above a "Hubungi kami" tile. Fix: drop `setiap` — "Harga dan stok tampil di katalog" keeps the whole point of the rewrite and cannot go stale. Same edit at L28. `apps/store/app/products/[slug]/page.tsx:L34: should: the same overclaim, and here it is worse-targeted.` This string is emitted only when `product.description` is null, which correlates with nothing about price, so a published-but-unpriced item without a description ships an OG/search description asserting its price is on the page. Fix: "Cek harga dan stok di halaman ini, pesan lewat WhatsApp." — invites the reader to look rather than asserting a number is there. `apps/store/lib/cart.test.ts:L89: nit: the test is named "…so nothing hostile survives" but the key it plants is `__proto__polluted`, an ordinary property.` The actual vector is `"__proto__"`. I checked the real one and you are safe — `JSON.parse` creates `__proto__` as an own data property without invoking the setter, and `readStorage`'s field-by-field rebuild drops it — so this is naming, not a hole. Fix: seed a raw `'[{"__proto__":{"pwned":1}, …}]'` and assert `({}).pwned === undefined`, or rename to "strips extra keys off a stored line" and stop claiming the rest. `apps/store/lib/cart.test.ts:L221: nit: "fires on a cross-tab reload" tests no cross-tab path.` It calls `reloadCart()` directly; it never constructs a `StorageEvent` nor exercises the `event.key !== null && event.key !== CART_KEY` filter in `useCart`. The hook is scoped out by the issue, so I am not asking for the coverage — only that the name not claim it. Fix: rename to "fires when the store is reloaded". `apps/store/lib/cart.ts:L82: nit: `getLines()` returns the live module array, so a caller can splice store state without going through `commit` — no persist, no publish.` Not a regression, `useCart` already handed the same array out as `snapshot`, but the new export widens who can reach it. Fix: a one-line comment that the array is read-only by convention. I would not add a `.slice()` — an allocation per read for two mount points is the wrong trade, and your `ponytail:` note already sets that tone. `apps/store/lib/cart.test.ts:L110: nit: `add(line())` passes a full `CartLine`, `qty` included, where the signature is `Omit<CartLine, "qty">`.` It typechecks only because the argument is not a fresh object literal, so excess-property checking is off. Fix: a `seed()` helper that omits `qty`, so the parameter's real shape is visible at the call sites. --- **Checked, deliberately not findings.** *Tailwind removal.* Sufficient, and the measurement you took is the one that mattered. Deleting a custom `postcss.config.mjs` does not just remove Tailwind — it restores Next's **default** pipeline (`postcss-flexbugs-fixes` + `preset-env`/autoprefixer), which the single-plugin config had been suppressing, so the emitted CSS genuinely could have changed. Byte-identical CSS across that boundary is what rules it out; "no utility classes" alone would not have. I confirmed the structural half independently: zero matches for `tailwind`, `@apply` or `@import "tailwindcss"` anywhere in `apps/store`, and `globals.css` is 781 lines of plain CSS with no nesting and nothing autoprefixer's default browserslist prefixes. "Visually identical" holds. *`apps/management` not built.* Your structural argument is sufficient and I re-verified every leg of it — no management path in the commit, `tailwindcss`/`@tailwindcss/postcss`/`tailwind-merge` intact in `apps/management/package.json`, lockfile hunk confined to the `apps/store:` importer — plus one leg you did not mention and which is the only way this could have leaked: there is no postcss config at the repo root or in any parent directory, so the store cannot inherit one now that its own is gone, and `apps/management/postcss.config.mjs` is a sibling, not a shared parent. Declining to build a package a sibling is mid-edit in was the right call; the acceptance criterion is about the store, and a build you cannot attribute is not evidence. *The `[slug]/page.tsx` line belongs in this PR.* "No page still tells visitors to ask for prices they can see" is not met without it. One line, inside `generateMetadata`, no overlap with the availability sites, coordinated first and declared. The two surviving strings — `app/cara-pesan/page.tsx:L140` "Tanya lewat WhatsApp" and `app/services/[slug]/page.tsx:L121` "Tanya via WhatsApp" — are question CTAs, not requests for a hidden price, and are correctly left alone. *No checkout promised.* "kirim pesanan lewat WhatsApp — ongkir dan pemasangan dikonfirmasi admin" matches `cart-view.tsx:L138` "Belum termasuk ongkir. Harga dikonfirmasi admin." Register is consistent with `/cart` and `/cara-pesan`. *One observation, not a finding.* Deleting `if (!Array.isArray(parsed)) return []` also leaves 20/20 green, because no JSON object has `.filter` and the `try/catch` returns `[]` regardless. The two paths are behaviourally indistinguishable for any JSON input, so that guard is clarity rather than defence and I am not asking for a test that couples to it. Merging deploys the storefront. The three copy strings are the only visible change, and they are the only thing I would touch before you do.
yuris commented 2026-08-27 08:04:36 +00:00
Author
Owner
Copy link

Round 1 addressed — 52194ad

Thanks, the mutation table was the right thing to demand. It found a real hole: my fixture entries each failed several clauses at once, so the suite could not tell which clause was doing the work. Both directed items are done, all four nits taken, one clause escalated into a finding of its own.

1. isLine clause isolation — 9 of 11 clauses now bite, 2 provably cannot

Root cause was as you diagnosed. Fixed by exporting isLine and testing the predicate directly, one otherwise-valid line per defect, so each case falsifies exactly one clause. This also reaches the non-finite price case that JSON structurally cannot carry — JSON.stringify(NaN) is "null", so no storage-seeded fixture could ever have isolated Number.isFinite. The suite is 39 cases in cart.test.ts (56 across the store).

I re-ran your check per clause — delete it, run the suite, restore:

clause deleted from isLine                     | result   | failing tests
-----------------------------------------------|----------|--------------
typeof value !== "object" || value === null    | RED      | 2
typeof line.slug === "string"                  | RED      | 2
line.slug !== ""                               | RED      | 2
typeof line.name === "string"                  | RED      | 2
typeof line.qty === "number"                   | GREEN    | 0
Number.isInteger(line.qty)                     | RED      | 2
line.qty > 0                                   | RED      | 3
typeof line.installAtShop === "boolean"        | RED      | 2
typeof line.price === "number"                 | GREEN    | 0
Number.isFinite(line.price)                    | RED      | 2
typeof line.shippable === "boolean"            | RED      | 2

restored; baseline: # tests 39  # pass 39  # fail 0

Five of your six are now RED. The sixth, typeof line.qty === "number", is still GREEN — and so is typeof line.price === "number", which your list did not include but has the same cause.

Neither is a coverage gap. No input can isolate them, because both are logically implied by the clause immediately after them. Number.isInteger and Number.isFinite are the non-coercing Number.* forms (unlike global isFinite), so each returns true only for an actual number. typeof x === "number" is therefore already entailed. Searched for a counterexample across 21 values spanning every primitive type, boxed numbers, BigInt, Symbol, and objects:

values probed: 21
Number.isInteger(x) true while typeof x !== number : 0
Number.isFinite(x)  true while typeof x !== number : 0
counterexample search over 2 clauses: none exists — both typeof guards are implied

I kept both clauses rather than deleting the dead code, and said why in a comment at cart.ts:L23: this is a trust boundary, and a clause that states its own precondition survives someone reordering or splitting the predicate later — which is exactly what #41 and #47 are about to do. A test asserting they are load-bearing would be a test asserting something false, so I did not write one; the redundancy is documented instead.

Your unasked-for observation about if (!Array.isArray(parsed)) return [] is the same shape of thing, and I agree with your call not to couple a test to it.

2. The copy overclaim — fixed in all three places

You were right, and the failure mode is the bad kind: true against today's data, false after one dashboard toggle, and silent when it flips. I traced it and it is worse than "could happen" — apps/management/lib/catalog.ts maps sellPrice <= 0 to price: null, publishing gates on isPublished alone with no price precondition, and product-card.tsx:L32 then renders "Hubungi kami". My own new test at cart.test.ts asserts unpriced lines are a live state, so the PR contradicted itself.

Reworded from asserting to inviting — "cek" rather than "tampil" — which is true whether the item shows a number or "Hubungi kami":

where now reads
products/page.tsx metadata …Situbondo. Cek harga dan stoknya, masukkan ke keranjang, lalu kirim pesanan lewat WhatsApp.
products/page.tsx lead Cek harga dan stok tiap produk, masukkan yang Anda butuhkan ke keranjang, lalu kirim pesanannya lewat WhatsApp — ongkir dan pemasangan dikonfirmasi admin.
products/[slug]/page.tsx fallback …Kotakan, Situbondo. Cek harga dan stok di halaman ini, pesan lewat WhatsApp.

I took your exact wording for the detail page. I did not touch the null-price behaviour — that is #43's.

Verified against the built HTML, not just the source: Tanya stok dan harga, tanya stok dan harganya, tampil di setiap produk and Harga dan stok tampil now match zero prerendered pages, and the original criterion still holds — no page tells visitors to ask for a price they can see.

Left alone as you noted: cara-pesan/page.tsx:L140 and services/[slug]/page.tsx:L121 are question CTAs, not price requests.

3. All four nits taken

  • __proto__polluted was not the vector. Replaced with a raw '[{"__proto__":{"pwned":1}, …}]' payload asserting ({}).pwned === undefined and that the rebuilt line has no pwned own-property. Confirms the safety you found rather than claiming it. The extra-keys case kept its narrower name, "strips extra keys off a stored line".
  • "fires on a cross-tab reload" → "fires when the store is reloaded". It never built a StorageEvent; the name now matches what it does. Not adding the hook-level coverage — the issue scopes useCart out.
  • getLines() returning the live array — four-line comment at cart.ts:L89. No .slice(), for the reason you gave.
  • add(line()) passing qty — split the fixture into product() (no qty, the real Omit<CartLine, "qty"> shape) and line() built from it. Every add call site now passes product(), so the signature is visible.

Re-verified at 52194ad

# tests 56   # suites 11   # pass 56   # fail 0

biome check … Checked 5 files in 10ms. No fixes applied.

Build still clean, and the emitted stylesheet is unchanged from the 0d6a787 baseline — 0q4g-iwdmjkln.css, 14271 bytes, sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b. The Tailwind removal is untouched by this round.

Nothing declined. Ready for re-review — and noted that the round-1 verdict is void now that I have pushed.

## Round 1 addressed — `52194ad` Thanks, the mutation table was the right thing to demand. It found a real hole: my fixture entries each failed several clauses at once, so the suite could not tell which clause was doing the work. Both directed items are done, all four nits taken, one clause escalated into a finding of its own. ### 1. `isLine` clause isolation — 9 of 11 clauses now bite, 2 provably cannot Root cause was as you diagnosed. Fixed by exporting `isLine` and testing the predicate directly, one otherwise-valid line per defect, so each case falsifies exactly one clause. This also reaches the non-finite price case that JSON structurally cannot carry — `JSON.stringify(NaN)` is `"null"`, so no storage-seeded fixture could ever have isolated `Number.isFinite`. The suite is 39 cases in `cart.test.ts` (56 across the store). I re-ran your check per clause — delete it, run the suite, restore: ``` clause deleted from isLine | result | failing tests -----------------------------------------------|----------|-------------- typeof value !== "object" || value === null | RED | 2 typeof line.slug === "string" | RED | 2 line.slug !== "" | RED | 2 typeof line.name === "string" | RED | 2 typeof line.qty === "number" | GREEN | 0 Number.isInteger(line.qty) | RED | 2 line.qty > 0 | RED | 3 typeof line.installAtShop === "boolean" | RED | 2 typeof line.price === "number" | GREEN | 0 Number.isFinite(line.price) | RED | 2 typeof line.shippable === "boolean" | RED | 2 restored; baseline: # tests 39 # pass 39 # fail 0 ``` Five of your six are now RED. The sixth, `typeof line.qty === "number"`, is still GREEN — and so is `typeof line.price === "number"`, which your list did not include but has the same cause. **Neither is a coverage gap. No input can isolate them, because both are logically implied by the clause immediately after them.** `Number.isInteger` and `Number.isFinite` are the non-coercing `Number.*` forms (unlike global `isFinite`), so each returns `true` only for an actual number. `typeof x === "number"` is therefore already entailed. Searched for a counterexample across 21 values spanning every primitive type, boxed numbers, `BigInt`, `Symbol`, and objects: ``` values probed: 21 Number.isInteger(x) true while typeof x !== number : 0 Number.isFinite(x) true while typeof x !== number : 0 counterexample search over 2 clauses: none exists — both typeof guards are implied ``` I kept both clauses rather than deleting the dead code, and said why in a comment at `cart.ts:L23`: this is a trust boundary, and a clause that states its own precondition survives someone reordering or splitting the predicate later — which is exactly what #41 and #47 are about to do. A test asserting they are load-bearing would be a test asserting something false, so I did not write one; the redundancy is documented instead. Your unasked-for observation about `if (!Array.isArray(parsed)) return []` is the same shape of thing, and I agree with your call not to couple a test to it. ### 2. The copy overclaim — fixed in all three places You were right, and the failure mode is the bad kind: true against today's data, false after one dashboard toggle, and silent when it flips. I traced it and it is worse than "could happen" — `apps/management/lib/catalog.ts` maps `sellPrice <= 0` to `price: null`, publishing gates on `isPublished` alone with no price precondition, and `product-card.tsx:L32` then renders **"Hubungi kami"**. My own new test at `cart.test.ts` asserts unpriced lines are a live state, so the PR contradicted itself. Reworded from asserting to inviting — "cek" rather than "tampil" — which is true whether the item shows a number or "Hubungi kami": | where | now reads | |---|---| | `products/page.tsx` metadata | …Situbondo. **Cek harga dan stoknya**, masukkan ke keranjang, lalu kirim pesanan lewat WhatsApp. | | `products/page.tsx` lead | **Cek harga dan stok tiap produk**, masukkan yang Anda butuhkan ke keranjang, lalu kirim pesanannya lewat WhatsApp — ongkir dan pemasangan dikonfirmasi admin. | | `products/[slug]/page.tsx` fallback | …Kotakan, Situbondo. **Cek harga dan stok di halaman ini**, pesan lewat WhatsApp. | I took your exact wording for the detail page. I did not touch the null-price behaviour — that is #43's. Verified against the built HTML, not just the source: `Tanya stok dan harga`, `tanya stok dan harganya`, `tampil di setiap produk` and `Harga dan stok tampil` now match **zero** prerendered pages, and the original criterion still holds — no page tells visitors to ask for a price they can see. Left alone as you noted: `cara-pesan/page.tsx:L140` and `services/[slug]/page.tsx:L121` are question CTAs, not price requests. ### 3. All four nits taken - **`__proto__polluted` was not the vector.** Replaced with a raw `'[{"__proto__":{"pwned":1}, …}]'` payload asserting `({}).pwned === undefined` and that the rebuilt line has no `pwned` own-property. Confirms the safety you found rather than claiming it. The extra-keys case kept its narrower name, "strips extra keys off a stored line". - **"fires on a cross-tab reload"** → **"fires when the store is reloaded"**. It never built a `StorageEvent`; the name now matches what it does. Not adding the hook-level coverage — the issue scopes `useCart` out. - **`getLines()` returning the live array** — four-line comment at `cart.ts:L89`. No `.slice()`, for the reason you gave. - **`add(line())` passing `qty`** — split the fixture into `product()` (no `qty`, the real `Omit<CartLine, "qty">` shape) and `line()` built from it. Every `add` call site now passes `product()`, so the signature is visible. ### Re-verified at `52194ad` ``` # tests 56 # suites 11 # pass 56 # fail 0 ``` ``` biome check … Checked 5 files in 10ms. No fixes applied. ``` Build still clean, and the emitted stylesheet is unchanged from the `0d6a787` baseline — `0q4g-iwdmjkln.css`, 14271 bytes, `sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b`. The Tailwind removal is untouched by this round. Nothing declined. Ready for re-review — and noted that the round-1 verdict is void now that I have pushed.
yuris reviewed 2026-08-27 08:53:23 +00:00
yuris left a comment
Copy link

VERDICT: approve-with-nits

Round 2, reviewed at 52194ad, diff 5b0515d..52194ad only. All three round-1 shoulds are resolved. I reproduced your mutation table rather than reading it, and I built the store twice to settle the two claims that a source grep cannot settle. 0 blockers, 0 shoulds, 1 nit.

Two throwaway detached worktrees under /tmp, node_modules copied in (not symlinked — Turbopack rejects a node_modules symlink that leaves the project root, which is probably why nobody has built from a worktree here before). Both removed; the main checkout and every wt-* were untouched.

1. isLine clause isolation — resolved, and one clause better than you claimed

Your split reproduces exactly, including every failure count. I deleted each clause from apps/store/lib/cart.ts, ran lib/cart.test.ts, restored, and re-verified the baseline between runs:

clause deleted mine yours
typeof value !== "object" || value === null RED (2) RED (2)
typeof line.slug === "string" RED (2) RED (2)
line.slug !== "" RED (2) RED (2)
typeof line.name === "string" RED (2) RED (2)
typeof line.qty === "number" GREEN GREEN
Number.isInteger(line.qty) RED (2) RED (2)
line.qty > 0 RED (3) RED (3)
typeof line.price === "number" GREEN GREEN
Number.isFinite(line.price) RED (2) RED (2)
typeof line.installAtShop === "boolean" RED (2) RED (2)
typeof line.shippable === "boolean" RED (2) RED (2)

Baseline before and after: # tests 39 # pass 39 # fail 0. Suite-wide 37 → 56 is real: # tests 56 # suites 11 # pass 56 # fail 0.

Two mutation points your table omits, both RED, so the real split is 10 of 12, not 9 of 11:

  • the line.price === null disjunct — collapsing the price clause to typeof line.price === "number" && Number.isFinite(line.price) → RED (2), the two null-price acceptances.
  • deleting the price group whole → RED (4).

Round 1's six are now five RED and one provably-not-isolable. I checked the fixtures are each otherwise valid rather than trusting that a RED isolates its clause: { ...line(), slug: 123 } still passes slug !== "", { ...line(), qty: 1.5 } still passes typeof/> 0, { ...line(), price: NaN } still passes typeof price === "number". Each RED falsifies one clause. Testing the predicate directly also reaches the two cases storage structurally cannot carry — JSON.stringify(NaN) is "null", so no seeded fixture could ever have isolated Number.isFinite. That was the right call, and the L89 comment says why at the point of use.

The tests assert behaviour, not shape: they name inputs and expected accept/reject at a trust boundary. And you kept the end-to-end path — "drops malformed entries but keeps the well-formed ones" still enumerates the same defects through reloadCart, so unit-level isolation was added beside boundary coverage rather than substituted for it.

The redundancy argument: sound. No counterexample exists.

Decided on the built-ins, not on your sweep. ECMA-262 defines both as taking a step-1 type gate:

  • Number.isFinite(number) — If number is not a Number, return false.
  • Number.isInteger(number) — If number is not a Number, return false.

No ToNumber, no valueOf/Symbol.toPrimitive hook, no unwrapping of a Number wrapper object. So Number.isInteger(x) === true ⟹ typeof x === "number", and likewise for Number.isFinite. Each typeof guard is entailed by the clause immediately after it, for every value — not for 21 of them. Your 21-value sweep is the weakest part of the argument and is not what carries it; the non-coercion property is, and you did cite it. Nothing to raise.

Given that, keeping both clauses and documenting the redundancy at cart.ts:L28 is the correct call, not dead code left in. A test asserting they are load-bearing would assert something false, and the reason you gave for keeping them is the operative one: the entailment holds only while Number.isInteger/Number.isFinite sit after them. Reorder so line.qty > 0 comes first and "5" > 0 coerces to true — the typeof clause is the thing that survives the reorder #41/#47 are about to perform. 9 (10) of 12 red is full coverage, not a shortfall. I will not re-raise it, and neither should a later round.

One scoping note, not a finding: "no input can isolate them" is exact for values and slightly overstated for objects, because isLine re-reads line.qty three times and line.price twice. An accessor property does separate the two predicates — a { get qty() {…} } yielding 5, 5, 0 across successive reads is rejected with the typeof clause and accepted without it; yielding 1.5, 5, 5 is the reverse. Unreachable here: isLine's only non-test caller is readStorage, which feeds it JSON.parse output, and JSON.parse only ever creates own data properties. It changes neither the ruling nor the decision — worth knowing only so the comment's claim is read as being about values, if someone leans on it while reshaping the predicate in #41/#47.

Exporting isLine is fine — settled, stated plainly so it is not re-litigated

getLines() earned a round-1 nit because it hands out mutable module state; isLine hands out nothing. It is a pure total predicate over unknown, no state, no side effects, and it already ran against every stored value — the export widens who may call it, not what it can reach. The app has no component or e2e harness, node:test over lib/ is the established pattern here, and for the non-finite-price clauses direct invocation is not merely convenient but the only route that exists. Acceptable. Not a finding now or later.

2. Copy overclaim, products/page.tsx — resolved

3. Copy overclaim, products/[slug]/page.tsx — resolved

Verified in built HTML, and on the state your own build could not reach. Round 1 established every isPublished item currently has sellPrice > 0, so a build against the live catalog never renders the case the finding was about. I built against a fixture catalog carrying a published item with price: null and description: null — hitting the card render and the generateMetadata fallback on one page.

out/products/aki-kering-ns60.html now carries both Cek harga dan stok di halaman ini and Hubungi kami. Under 5b0515d that same page would have shipped an OG/search description asserting a price the page does not show. The finding is closed on its own worst case, not on today's data — which is what made it worth raising, since the old copy was true until one dashboard toggle.

Your zero-page claim holds against the actual export, all four phrases, 18 prerendered pages:

'Tanya stok dan harga'      -> 0        'Cek harga dan stoknya'             -> /products
'tanya stok dan harganya'   -> 0        'Cek harga dan stok tiap produk'    -> /products
'tampil di setiap produk'   -> 0        'Cek harga dan stok di halaman ini' -> /products/aki-kering-ns60
'Harga dan stok tampil'     -> 0        'Hubungi kami'                      -> the unpriced page + card chunk

Register is the store's own: tiap is already used twice in cara-pesan/page.tsx (L53 "Untuk tiap barang", L62 "Atur jumlah tiap barang"), and imperative-first matches "Lihat produk", "Buka keranjang", "Cari suku cadang", "Atur jumlah", "Kabari kami". Cek is new to the store but sits in the same informal register. The detail page took round 1's wording verbatim. No objection.

Nit

apps/store/lib/cart.test.ts:L94: nit: the fixture labelled "a missing installAtShop" is missing qty, not installAtShop. product() returns installAtShop: false — present and valid; what it omits is qty. So isLine(product()) is rejected by typeof line.qty === "number", and the case named for a missing installAtShop never exercises one. Same shape as round 1's __proto__polluted: the name claims more than the fixture does. No coverage is lost — { ...line(), installAtShop: "yes" } at L93 covers that clause and my run confirms deleting it goes RED — and the L65 promise of exactly one defect still holds; the defect is just not the labelled one. Fix: rename to "a missing qty", or make it { ...line(), installAtShop: undefined } and add a separate product() case named for the absent qty.

Round 1's other three nits — all landed

__proto__ now seeded raw and asserted against Object.prototype, with the extra-keys case renamed to its narrower true claim. "fires when the store is reloaded" no longer claims a cross-tab path it never built. getLines() carries the read-only-by-convention comment and no .slice(). product() / line() split means every add call site passes the real Omit<CartLine, "qty">, so excess-property checking is live at those call sites instead of silently off.

Byte-identical CSS — reproduced across the pipeline boundary

Not taken on trust, because this is the one claim gating a production deploy. Two detached worktrees, same node_modules, same catalog fixture. 0d6a787 has postcss.config.mjs with @tailwindcss/postcss; 52194ad has no postcss config anywhere, so Next's default pipeline is restored. Both emit:

_next/static/chunks/0q4g-iwdmjkln.css   14271 bytes
sha256 c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b

Filename, size and hash all match what you reported, and are identical across the config swap. Build clean at 52194ad, TypeScript clean, 18 static pages, biome check clean on the four changed source files.

Checked, deliberately not a finding

apps/store/app/products/[slug]/page.tsx:L186 "Stok berubah setiap hari. Tanyakan lewat WhatsApp untuk memastikan harga dan ketersediaan terbaru." is untouched by this PR — git diff origin/main...52194ad does not reach it — and it does not reopen the criterion. It is a freshness caveat on a price that is shown, the same register as cart-view.tsx:L138 "Harga dikonfirmasi admin." and this PR's own "ongkir dan pemasangan dikonfirmasi admin". Not a request for a hidden price, so it belongs with the two survivors I cleared in round 1, not with the three strings this PR retired.

Merge it. The nit is a test label with no behavioural reach — take it or leave it, it does not gate the deploy. Round 1's load-bearing item was the one that mattered, because #49 exists so cart coverage lands before #41 reshapes the cart line, and the predicate is now pinned clause by clause with the two exceptions proven unpinnable rather than waved at.

VERDICT: approve-with-nits Round 2, reviewed at `52194ad`, diff `5b0515d..52194ad` only. All three round-1 shoulds are **resolved**. I reproduced your mutation table rather than reading it, and I built the store twice to settle the two claims that a source grep cannot settle. 0 blockers, 0 shoulds, 1 nit. Two throwaway detached worktrees under `/tmp`, `node_modules` copied in (not symlinked — Turbopack rejects a `node_modules` symlink that leaves the project root, which is probably why nobody has built from a worktree here before). Both removed; the main checkout and every `wt-*` were untouched. --- ## 1. `isLine` clause isolation — resolved, and one clause better than you claimed Your split reproduces **exactly**, including every failure count. I deleted each clause from `apps/store/lib/cart.ts`, ran `lib/cart.test.ts`, restored, and re-verified the baseline between runs: | clause deleted | mine | yours | |---|---|---| | `typeof value !== "object" \|\| value === null` | RED (2) | RED (2) | | `typeof line.slug === "string"` | RED (2) | RED (2) | | `line.slug !== ""` | RED (2) | RED (2) | | `typeof line.name === "string"` | RED (2) | RED (2) | | `typeof line.qty === "number"` | **GREEN** | GREEN | | `Number.isInteger(line.qty)` | RED (2) | RED (2) | | `line.qty > 0` | RED (3) | RED (3) | | `typeof line.price === "number"` | **GREEN** | GREEN | | `Number.isFinite(line.price)` | RED (2) | RED (2) | | `typeof line.installAtShop === "boolean"` | RED (2) | RED (2) | | `typeof line.shippable === "boolean"` | RED (2) | RED (2) | Baseline before and after: `# tests 39 # pass 39 # fail 0`. Suite-wide `37 → 56` is real: `# tests 56 # suites 11 # pass 56 # fail 0`. Two mutation points your table omits, both RED, so the real split is **10 of 12**, not 9 of 11: - the `line.price === null` disjunct — collapsing the price clause to `typeof line.price === "number" && Number.isFinite(line.price)` → RED (2), the two null-price acceptances. - deleting the price group whole → RED (4). Round 1's six are now five RED and one provably-not-isolable. I checked the fixtures are each *otherwise* valid rather than trusting that a RED isolates its clause: `{ ...line(), slug: 123 }` still passes `slug !== ""`, `{ ...line(), qty: 1.5 }` still passes `typeof`/`> 0`, `{ ...line(), price: NaN }` still passes `typeof price === "number"`. Each RED falsifies one clause. Testing the predicate directly also reaches the two cases storage structurally cannot carry — `JSON.stringify(NaN)` is `"null"`, so no seeded fixture could ever have isolated `Number.isFinite`. That was the right call, and the L89 comment says why at the point of use. The tests assert behaviour, not shape: they name inputs and expected accept/reject at a trust boundary. And you kept the end-to-end path — `"drops malformed entries but keeps the well-formed ones"` still enumerates the same defects through `reloadCart`, so unit-level isolation was added beside boundary coverage rather than substituted for it. ### The redundancy argument: **sound**. No counterexample exists. Decided on the built-ins, not on your sweep. ECMA-262 defines both as taking a step-1 type gate: - `Number.isFinite(number)` — *If number is not a Number, return false.* - `Number.isInteger(number)` — *If number is not a Number, return false.* No `ToNumber`, no `valueOf`/`Symbol.toPrimitive` hook, no unwrapping of a `Number` wrapper object. So `Number.isInteger(x) === true` ⟹ `typeof x === "number"`, and likewise for `Number.isFinite`. Each `typeof` guard is entailed by the clause immediately after it, for **every** value — not for 21 of them. Your 21-value sweep is the weakest part of the argument and is not what carries it; the non-coercion property is, and you did cite it. Nothing to raise. Given that, keeping both clauses and documenting the redundancy at `cart.ts:L28` is the correct call, not dead code left in. A test asserting they are load-bearing would assert something false, and the reason you gave for keeping them is the operative one: the entailment holds only while `Number.isInteger`/`Number.isFinite` sit after them. Reorder so `line.qty > 0` comes first and `"5" > 0` coerces to `true` — the `typeof` clause is the thing that survives the reorder #41/#47 are about to perform. **9 (10) of 12 red is full coverage, not a shortfall.** I will not re-raise it, and neither should a later round. One scoping note, not a finding: *"no input can isolate them"* is exact for values and slightly overstated for objects, because `isLine` re-reads `line.qty` three times and `line.price` twice. An accessor property does separate the two predicates — a `{ get qty() {…} }` yielding `5, 5, 0` across successive reads is rejected with the `typeof` clause and accepted without it; yielding `1.5, 5, 5` is the reverse. Unreachable here: `isLine`'s only non-test caller is `readStorage`, which feeds it `JSON.parse` output, and `JSON.parse` only ever creates own data properties. It changes neither the ruling nor the decision — worth knowing only so the comment's claim is read as being about values, if someone leans on it while reshaping the predicate in #41/#47. ### Exporting `isLine` is fine — settled, stated plainly so it is not re-litigated `getLines()` earned a round-1 nit because it hands out mutable module state; `isLine` hands out nothing. It is a pure total predicate over `unknown`, no state, no side effects, and it already ran against every stored value — the export widens who may *call* it, not what it can reach. The app has no component or e2e harness, `node:test` over `lib/` is the established pattern here, and for the non-finite-price clauses direct invocation is not merely convenient but the only route that exists. Acceptable. Not a finding now or later. ## 2. Copy overclaim, `products/page.tsx` — resolved ## 3. Copy overclaim, `products/[slug]/page.tsx` — resolved Verified in built HTML, and on the state your own build could not reach. Round 1 established every `isPublished` item currently has `sellPrice > 0`, so a build against the live catalog never renders the case the finding was about. I built against a fixture catalog carrying a published item with `price: null` **and** `description: null` — hitting the card render and the `generateMetadata` fallback on one page. `out/products/aki-kering-ns60.html` now carries **both** `Cek harga dan stok di halaman ini` and `Hubungi kami`. Under `5b0515d` that same page would have shipped an OG/search description asserting a price the page does not show. The finding is closed on its own worst case, not on today's data — which is what made it worth raising, since the old copy was true until one dashboard toggle. Your zero-page claim holds against the actual export, all four phrases, 18 prerendered pages: ``` 'Tanya stok dan harga' -> 0 'Cek harga dan stoknya' -> /products 'tanya stok dan harganya' -> 0 'Cek harga dan stok tiap produk' -> /products 'tampil di setiap produk' -> 0 'Cek harga dan stok di halaman ini' -> /products/aki-kering-ns60 'Harga dan stok tampil' -> 0 'Hubungi kami' -> the unpriced page + card chunk ``` Register is the store's own: `tiap` is already used twice in `cara-pesan/page.tsx` (L53 "Untuk tiap barang", L62 "Atur jumlah tiap barang"), and imperative-first matches "Lihat produk", "Buka keranjang", "Cari suku cadang", "Atur jumlah", "Kabari kami". `Cek` is new to the store but sits in the same informal register. The detail page took round 1's wording verbatim. No objection. ## Nit `apps/store/lib/cart.test.ts:L94: nit: the fixture labelled "a missing installAtShop" is missing `qty`, not `installAtShop`.` `product()` returns `installAtShop: false` — present and valid; what it omits is `qty`. So `isLine(product())` is rejected by `typeof line.qty === "number"`, and the case named for a missing `installAtShop` never exercises one. Same shape as round 1's `__proto__polluted`: the name claims more than the fixture does. No coverage is lost — `{ ...line(), installAtShop: "yes" }` at L93 covers that clause and my run confirms deleting it goes RED — and the L65 promise of exactly one defect still holds; the defect is just not the labelled one. Fix: rename to `"a missing qty"`, or make it `{ ...line(), installAtShop: undefined }` and add a separate `product()` case named for the absent `qty`. ## Round 1's other three nits — all landed `__proto__` now seeded raw and asserted against `Object.prototype`, with the extra-keys case renamed to its narrower true claim. `"fires when the store is reloaded"` no longer claims a cross-tab path it never built. `getLines()` carries the read-only-by-convention comment and no `.slice()`. `product()` / `line()` split means every `add` call site passes the real `Omit<CartLine, "qty">`, so excess-property checking is live at those call sites instead of silently off. ## Byte-identical CSS — reproduced across the pipeline boundary Not taken on trust, because this is the one claim gating a production deploy. Two detached worktrees, same `node_modules`, same catalog fixture. `0d6a787` has `postcss.config.mjs` with `@tailwindcss/postcss`; `52194ad` has no postcss config anywhere, so Next's default pipeline is restored. Both emit: ``` _next/static/chunks/0q4g-iwdmjkln.css 14271 bytes sha256 c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b ``` Filename, size and hash all match what you reported, and are identical across the config swap. Build clean at `52194ad`, TypeScript clean, 18 static pages, `biome check` clean on the four changed source files. ## Checked, deliberately not a finding `apps/store/app/products/[slug]/page.tsx:L186` "Stok berubah setiap hari. Tanyakan lewat WhatsApp untuk memastikan harga dan ketersediaan terbaru." is untouched by this PR — `git diff origin/main...52194ad` does not reach it — and it does not reopen the criterion. It is a freshness caveat on a price that *is* shown, the same register as `cart-view.tsx:L138` "Harga dikonfirmasi admin." and this PR's own "ongkir dan pemasangan dikonfirmasi admin". Not a request for a hidden price, so it belongs with the two survivors I cleared in round 1, not with the three strings this PR retired. --- Merge it. The nit is a test label with no behavioural reach — take it or leave it, it does not gate the deploy. Round 1's load-bearing item was the one that mattered, because #49 exists so cart coverage lands before #41 reshapes the cart line, and the predicate is now pinned clause by clause with the two exceptions proven unpinnable rather than waved at.
yuris commented 2026-08-27 08:59:31 +00:00
Author
Owner
Copy link

Pre-merge verification at 52194ad

Re-run just now at the exact commit being squashed, not carried over from before the review.

commit under test: 52194ad73da97537c7de818f62995dc3d549f839

$ pnpm --filter garage-store test
# tests 56
# suites 11
# pass 56
# fail 0
# duration_ms 192.073644

$ pnpm exec biome check <the 5 changed files>
Checked 5 files in 11ms. No fixes applied.

$ pnpm --filter garage-store build
✓ Compiled successfully in 23.5s
  Finished TypeScript in 2.4s ...

emitted stylesheet: 0q4g-iwdmjkln.css 14271 bytes
sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b

Stylesheet is still byte-identical to the 0d6a787 baseline — same size, same content hash, same content-derived chunk name. Working tree clean before and after the build. mergeable re-fetched immediately before merging.

Outstanding nit — acknowledged, not fixed here

cart.test.ts:L94: the fixture labelled "a missing installAtShop" is really a missing-qty case, because product() already sets installAtShop: false. Correct catch. No coverage is lost — the installAtShop clause is isolated by the entry above it, { ...line(), installAtShop: "yes" }, which is what turns that clause RED in the mutation table. So this is a wrong label on a case that still tests something real, not a gap.

I am deliberately not pushing a fix. Another push voids this verdict and buys a third review round for one word, which is a bad trade against a nit with no behavioural consequence. The label gets corrected when #47 next touches the cart line shape.

Thanks for the round-2 grounding

The ECMA-262 citation is a better argument than the one I made. I reached the right conclusion from a 21-value sweep, which is weak evidence — a sweep can only fail to find a counterexample, never show none exists. Step 1 of both Number.isFinite and Number.isInteger ("if number is not a Number, return false", no coercion, no wrapper unwrap, no valueOf/Symbol.toPrimitive hook) is what actually establishes that each typeof guard is entailed by the clause after it, for every value rather than for 21 of them. Noted for next time, and the two extra red points you found are a better table than mine.

Merging deploys to production

.forgejo/workflows/deploy-store.yml triggers on push to main and runs wrangler pages deploy. Squashing this ships the storefront live. No workflow runs on pull_request, so nothing automated has ever checked this branch — every green light above is a local run. The only visible change is the three copy strings; the build output is otherwise proven byte-identical.

## Pre-merge verification at `52194ad` Re-run just now at the exact commit being squashed, not carried over from before the review. ``` commit under test: 52194ad73da97537c7de818f62995dc3d549f839 $ pnpm --filter garage-store test # tests 56 # suites 11 # pass 56 # fail 0 # duration_ms 192.073644 $ pnpm exec biome check <the 5 changed files> Checked 5 files in 11ms. No fixes applied. $ pnpm --filter garage-store build ✓ Compiled successfully in 23.5s Finished TypeScript in 2.4s ... emitted stylesheet: 0q4g-iwdmjkln.css 14271 bytes sha256=c122bc4757eb6bf32c617b752cb6ef4ce3d3f6f2d528b72cd8cf712948f6f04b ``` Stylesheet is still byte-identical to the `0d6a787` baseline — same size, same content hash, same content-derived chunk name. Working tree clean before and after the build. `mergeable` re-fetched immediately before merging. ### Outstanding nit — acknowledged, not fixed here `cart.test.ts:L94`: the fixture labelled **"a missing installAtShop"** is really a missing-`qty` case, because `product()` already sets `installAtShop: false`. Correct catch. No coverage is lost — the `installAtShop` clause is isolated by the entry above it, `{ ...line(), installAtShop: "yes" }`, which is what turns that clause RED in the mutation table. So this is a wrong label on a case that still tests something real, not a gap. I am deliberately not pushing a fix. Another push voids this verdict and buys a third review round for one word, which is a bad trade against a nit with no behavioural consequence. The label gets corrected when #47 next touches the cart line shape. ### Thanks for the round-2 grounding The ECMA-262 citation is a better argument than the one I made. I reached the right conclusion from a 21-value sweep, which is weak evidence — a sweep can only fail to find a counterexample, never show none exists. Step 1 of both `Number.isFinite` and `Number.isInteger` ("if number is not a Number, return false", no coercion, no wrapper unwrap, no `valueOf`/`Symbol.toPrimitive` hook) is what actually establishes that each `typeof` guard is entailed by the clause after it, for every value rather than for 21 of them. Noted for next time, and the two extra red points you found are a better table than mine. ### Merging deploys to production `.forgejo/workflows/deploy-store.yml` triggers on push to `main` and runs `wrangler pages deploy`. Squashing this ships the storefront live. No workflow runs on `pull_request`, so nothing automated has ever checked this branch — every green light above is a local run. The only visible change is the three copy strings; the build output is otherwise proven byte-identical.
yuris merged commit 22cfddd573 into main 2026-08-27 09:01:18 +00:00
yuris deleted branch chore/49-cart-tests-tailwind 2026-08-27 09:01:18 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/garage!51
No description provided.