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

feat(store): faceted product filters — kategori, merk, kendaraan, ketersediaan #54

Merged
yuris merged 3 commits from feat/46-faceted-filters into main 2026-08-27 14:35:07 +00:00
Conversation 3 Commits 3 Files changed 4 +979 -123
yuris commented 2026-08-27 12:42:15 +00:00
Owner
Copy link

Closes #46.

Merging this PR deploys to production: no workflow runs on pull_request — both .forgejo/workflows/*.yml trigger on push: main and they deploy (the store runs wrangler pages deploy). The forge never checks this branch, so every green light below is a command I ran locally against commit 7b7a7e5.

What changed

  • apps/store/lib/facets.ts (new) — the whole filter reduction as a pure module, so it is testable without a component harness. reduceCatalog returns the grid contents and every facet count in one pass over the catalog. OR inside a group, AND across groups.
  • apps/store/lib/facets.test.ts (new) — 24 node:test cases over a fixture catalog, matching lib/checkout-message.test.ts in style.
  • apps/store/app/products/product-browser.tsx — replaces the single-select category button row with a four-group multi-select sidebar (Kategori, Merk, Kendaraan, Ketersediaan), per-option counts, an active-filter chip row, a live result count, and a mobile collapse toggle. The text search is unchanged in behaviour and now AND-s with the facets.
  • apps/store/app/globals.css — one appended region commented #46, starting at line 849: .browser, .facet-panel, .facet-group, .facet-option, .facet-count, .filter-toolbar, .result-count, .filter-chip, .filter-toggle, plus the max-width: 900px block. Every selector is new except two descendant-scoped overrides under .browser. I did not touch .stock-badge or any availability rule — that is #47's region.

app/products/page.tsx is unchanged: the page already fetched the full catalog, and counts come from that same array — no second data source.

Counts reflect the other active facets, never a facet's own selection

This is the subtle requirement. Per item, reduceCatalog tallies how many groups it fails. It contributes to a group's counts when it fails no group, or fails only that group — so ticking one Kategori leaves its siblings reading their real reachable totals instead of collapsing to zero. Two or more misses and no single tick could surface it, so it counts nowhere.

Verified live at 1440px with ?kategori=Aki:

group counts
Kategori (own) Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3 unchanged — own selection ignored
Merk GS Astra=2, all others 0 narrowed by Kategori
Kendaraan Avanza / Xenia=1 Brio / Mobilio=1 Ertiga=0 Innova=1 Pajero / Fortuner=1 narrowed by Kategori
Ketersediaan Ready=1 Indent=1 Habis=0 narrowed by Kategori

Options with a reachable count of 0 stay listed but disabled, so the sidebar never reflows under the pointer; an already-ticked option is never disabled, so no tick can become un-undoable.

URL scheme: repeated keys, no delimiter

fitment is free text carrying slashes, spaces, ampersands and parentheses, so any separator would need an escape of its own. URLSearchParams already round-trips repeated keys losslessly: ?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner. A pre-existing single-value ?kategori=Oli&q=aki link still works. Unknown values are kept rather than dropped, so a stale link renders its chip and an honest 0 produk that one click undoes, instead of silently showing the whole catalog.

Ketersediaan: three checkboxes, not one "Ready stok"

Decision: one checkbox per state — Ready, Indent, Habis — in that fixed severity order, with the note "Indent bisa dipesan — dikirim setelah stok masuk." above them.

The issue specifies a single "Ready stok" checkbox, but it was written against the inStock boolean that #41 removed. Reasoning for changing it:

  • A lone "Ready stok" box has only two states, so unticking it means "ready plus everything else" and ticking it means "ready only". indent never gets named either way — it lands in an unlabelled not-ready bucket together with habis, which is exactly the conflation the tri-state was introduced to remove.
  • indent is orderable. Hiding it behind a ready-only filter would suppress buyable stock from anyone who ticks the one obvious box, which costs real orders.
  • Three boxes also express the useful query the boolean could not: Ready + Indent = everything a visitor can actually buy today, habis excluded.
  • Ordered ready → indent → habis rather than alphabetically, because availability is a defined scale, not free text.

No wording implies indent is unavailable: the option reads Indent, the note says it can be ordered and ships once stock arrives, and the facet is independently selectable (ticking Indent alone yields its 4 items, verified below).

Verified

All at commit 7b7a7e5, worktree clean.

Test suite — pnpm --filter garage-store test:

# tests 80
# suites 16
# pass 80
# fail 0
# skipped 0
# todo 0

56 pre-existing plus 24 new. The two that pin the subtle requirement are leaves a group's own counts untouched by its own selection and counts what selecting the option would actually yield (it ticks the option and asserts the resulting item count equals the number the sidebar promised). Also covered: OR-within/AND-across, an item that misses two groups counting nowhere, a repeated fitment value counting once, blank/null brand and empty fitment producing no blank option and no match, and the awkward-value round-trip (Body & lampu, KYB + Kayaba, Avanza / Xenia (2019)).

Build — SITE_URL=… GARAGE_API_URL=… CATALOG_API_KEY=… pnpm --filter garage-store build:

✓ Compiled successfully in 10.0s
  Finished TypeScript in 2.7s
✓ Generating static pages using 13 workers (32/32) in 640ms
├ ○ /products
├ ● /products/[slug]

Catalog endpoint for the build: the store has no fixture, so I served one — /tmp/wt46-catalog-server.mjs, a 16-item schemaVersion: 3 payload on /api/public/catalog behind the x-api-key header, sized so every facet count is checkable by hand. It exercises the awkward cases on purpose: 5 items with brand: null, 3 with empty fitment, multi-value fitment, all three availability states, and a null price. /tmp/wt46-static.mjs then serves out/ the way Cloudflare Pages does (clean URLs resolve to <path>.html) so query strings survive a reload. Both scripts are throwaway verification tooling and are not committed.

Biome — pnpm exec biome check lib/facets.ts lib/facets.test.ts app/products/product-browser.tsx app/globals.css:

Checked 4 files in 15ms. No fixes applied.
Found 2 warnings.

Both warnings are noDescendingSpecificity on the pre-existing .image-placeholder / .cart-line .image-placeholder rules at lines 573 and 641 — above my region, which starts at line 849. biome check on git show 55f5d72:apps/store/app/globals.css reports the same Found 2 warnings, so my change introduces none.

Browser — 1440 × 900

Real Chromium against the served static export.

  • Unfiltered: 16 produk, 16 cards, sidebar 240px 968px, toggle display: none. Counts summed correctly out of the gate — Kategori Aki 2 / Ban 2 / Body & lampu 2 / Filter 3 / Kaki-kaki 4 / Oli 3 = 16; Ketersediaan Ready 10 / Indent 4 / Habis 2 = 16; Merk summed to 11, the 5 brand: null items correctly absent with no blank option; Kendaraan omitted the 3 empty-fitment items.
  • Multi-select OR: ?kategori=Aki&kategori=Ban → 4 produk, exactly the 2 Aki + 2 Ban items. Kategori's own counts stayed at their unfiltered values; Merk narrowed to Dunlop 2, GS Astra 2.
  • AND across groups: adding kendaraan=Pajero / Fortuner → 2 produk (Aki N50 Basah, Ban 265/65 R17). Kategori counts became Aki 1 / Ban 1 / Kaki-kaki 1 / rest 0 — narrowed by Kendaraan only, still ignoring its own two ticks — while Kendaraan's own counts held at their previous values. Both directions correct simultaneously.
  • URL round-trip via hard reload: reloaded ?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner from scratch. Checkboxes, all three chips, 2 produk and both cards restored identically — the filtered view is linkable, not just in-memory state.
  • Chip removal: clicking Pajero / Fortuner × cleared only Kendaraan → 4 produk, Kategori ticks intact, URL back to ?kategori=Aki&kategori=Ban. Hapus semua filter → 16 produk, empty query string.
  • Ketersediaan: Indent alone → 4 produk, the four indent items. Ready + Indent → 14 produk (16 minus the 2 habis).
  • Search AND-s and round-trips: typing aki on top of Ready + Indent → 2 produk, URL ?ketersediaan=indent&ketersediaan=ready&q=aki; reloading that URL restored the same state and the same narrowed availability counts (Ready 1 / Indent 1 / Habis 0).

Browser — 375 × 812

  • Sidebar collapsed by default: .facet-panel display: none, aria-expanded="false", .browser a single 343px column.
  • Toggle reads Filter (2) ▾ — it carries the active-filter count, so a collapsed sidebar still says filters are on. Hit area 46 × 343px, full width.
  • Tapping it expands (display: flex, aria-expanded="true", aria-controls="product-facets" matching the panel id); tapping again re-collapses. Verified with real mouse events at the button's centre coordinates, both directions.
  • 44px tap targets: every .facet-option measured exactly 44px and every .filter-chip 44px, at the expanded state.
  • No horizontal overflow: documentElement.scrollWidth 375 = viewport width.
  • Counts, chips and result count all behaved as on desktop (?kategori=Aki&kendaraan=Pajero+%2F+Fortuner → 1 produk, correct).

Deviations

  • The mobile toggle is labelled Filter (n), not the mockup's Filter & urutkan. There is no sort control in this PR — the issue specifies four facet groups and no sort dimension, and none of its acceptance criteria mention ordering. Labelling the button "& urutkan" would advertise a control that does not exist, so it names only what it opens. The collapse behaviour the mockup asks for is implemented and verified above. Adding sorting is a separate issue; the label should change with it.
  • 0-count options render disabled rather than hidden. The issue does not say which. Hiding them would reflow the sidebar under the pointer mid-click; an already-ticked option is never disabled, so nothing becomes un-undoable.
  • The search term gets no chip, unlike the facets. It is already visible in its own box and clears there, so a second removal affordance for it would be redundant. Hapus semua filter deliberately clears the facets and leaves the query text, since the box still shows it.
  • Free-text search still reads name, description, brand and part number — unchanged from the previous browser. oemNumbers and fitment are facets here; widening the haystack is not this issue's scope.
  • Brand and fitment match exactly after trimming, with no case folding, since both come from managed dashboard fields. Flagged with a ponytail: comment in facets.ts naming case-folding as the upgrade path if the dashboard ever admits free-text brands.

Round 2 — review id=13 (approve-with-nits) and the composition cross-check

Head is now 6fece92 (was 7b7a7e5). Two commits on top:

  • c6d956b — chore(store): delete dead .product-filter css left by the facet rewrite
  • 6fece92 — fix(store): debounce the facet url write and clear the query on reset

should — dead .product-filter* CSS deleted

Deleted globals.css lines 380–413: .product-filters, .product-filters-legend, .product-filter and .product-filter[aria-pressed="true"]. This PR removed their only consumer (the single-select category button row), so they were orphaned by this PR and are this PR's to remove.

Proof the deletion is safe, not an assumption:

  1. Zero consumers. grep for product-filter across the repo returns four hits, none of them markup: the four CSS rules themselves, a prose comment at globals.css:67, and one docs line. No .tsx, .ts, .jsx, .html or template file references any of the three classes.
  2. Nothing can match them. In the built site I queried .product-filters, .product-filters-legend, .product-filter on all 8 routes — /products, two detail pages, /, /cart, /services, /contact, /cara-pesan — across 1124 elements: 0 matches on every page, and the rules are absent from document.styleSheets. Rules that match nothing cannot change rendering.
  3. Rendering is byte-identical. I re-measured 1770 computed values on /products — 27 selectors × colour/background/border/radius/font/padding/min-height/display/opacity/flex/cursor/gap plus bounding boxes — at 1440×1000 and 375×812, panel collapsed and expanded, filtered and unfiltered, before and after the deletion. 0 of 1770 values changed.

I deliberately did not widen this into general CSS tidying: the two pre-existing noDescendingSpecificity warnings and every other unused-looking rule are untouched. Only classes whose consumer this PR removed.

One thing I did not touch and am flagging rather than fixing: docs/store-overhaul-spec.md:260 reads ".product-filter and .gallery-thumb need a min-height: 44px audit before the cart quantity steppers land". .product-filter no longer exists after this commit, so that line is now stale. It is a one-line docs fix, but it is a different file with an unclear owner and outside the scope you set, so I left it. The requirement behind it is satisfied: the replacement control .facet-option measures exactly 44px, verified on all 21 rows. Say the word and I will correct the line.

nit globals.css:67 — accepted

The comment justifying font: inherit on form controls cited "the .product-filter chips", a class this commit deletes. Repointed at .facet-option checkboxes and .filter-chip buttons, which are the controls that actually need it now.

nit product-browser.tsx:123 — accepted, URL write debounced

Real platform limit, and leaving it would make a long search query throw on iOS Safari — a large share of this storefront's traffic. The URL write moved out of the change handler into a 200 ms debounced useEffect keyed on picked. The input stays controlled by state, so typing is unaffected; only the history write defers.

Measured, instrumenting window.history.replaceState — note it must be patched as an own property, since Next's App Router installs its own there and a History.prototype patch is bypassed (my first attempt read a false 0 for that reason):

action writes during the burst after it settles
25-keystroke query 0 2, URL ?q=shockbreaker+depan+avanza
one facet tick — 2, URL ?kategori=Oli

Writes now scale with settled interactions, not keystrokes: 25 keystrokes cost 2 writes where they previously cost one each. Both writes carry an identical URL and the count is 2 per settled action whether the action was 1 keystroke or 25; I did not chase which layer emits the duplicate, only established it does not scale with typing. Safari's ~100-per-30s ceiling now needs ~50 distinct interactions in 30 s, which is not a typing pattern.

Round-trip still holds: after the debounce settles, a hard reload of the written URL restored the query text, the ticks and 1 produk.

nit product-browser.tsx:151 — accepted, q is now cleared too

Of the two options offered I took clearing q rather than relabelling, because it makes the stronger label honest instead of weakening the label to match a partial action. Hapus semua filter now resets to EMPTY_SELECTION: on ?kategori=Aki&q=aki it lands on / `16 produk` / empty search box, where it previously left `?q=aki` and `2 produk`. The emptied input is the visible receipt, and per-facet chips still exist for dropping one facet while keeping the search. Verified: `?kategori=Aki&q=aki` → click → URL, input ``, 16 produk, 0 chips.

Also removed the const apply = setPicked alias the debounce left behind — with the URL write in the effect it was pure indirection, so the five call sites now call setPicked directly.

Composition cross-check with #55 (#47) — clean, measured both ways

You were right that textual non-conflict is not the risk. I composed both branches' globals.css in my own worktree (git merge-file three-way, git restored afterwards — worktree confirmed byte-identical and clean), built it, and measured the same 1770 values with one shared probe file so a difference could not come from a difference in the probe.

git merge-tree --write-tree HEAD origin/feat/47-product-detail → no conflicts. Line arithmetic checks out: 847 base + 148 mine − 34 deleted + 53 theirs = 1014 composed, 0 conflict markers.

My region against theirs — 0 property collisions. Of 1770 values, #46 property differences: 0. No colour, background, border, display, font, spacing, cursor, flex or specificity value in my region changes when #47 is composed in. The only movement is 8 container heights — .product-grid 662→674 px, .browser-results 1125→1143 px and so on — which is the correct downstream consequence of their chips growing 19→25 px tall, not a style collision.

Their chips on my page — all three states correct. On the composed sheet, measured on /products and two detail pages:

state markup --chip colour fill
ready <span> (detail only) #2d543c --ok rgb(45, 84, 60) color(srgb 0.176471 0.329412 0.235294 / 0.12)
indent <p> in card #8a5a12 --warn rgb(138, 90, 18) color(srgb 0.541176 0.352941 0.0705882 / 0.12)
habis <p> in card #8c3a32 --off rgb(140, 58, 50) color(srgb 0.54902 0.227451 0.196078 / 0.12)

is-indent resolves --warn, not --off — the wave-1 #52/#53 defect does not return. Ready chips on cards: 0, correct, product-card.tsx returns null for ready. All at 1px border, 999px radius, align-self: start, boxes 58×25 (Indent) and 52×25 (Habis).

No third surface behind their tint. .browser, .browser-results, .facet-panel, .filter-toolbar and .product-grid all compute rgba(0, 0, 0, 0); only .facet-group adds --surface, and no product card is a descendant of it. Their 12 % fill keeps compositing over exactly the two bases they sized it for. One finding neither of us owns, which Author47b independently confirmed: .product-card.is-out { background: transparent } is pre-existing on 55f5d72, so Habis cards are unfilled and their chip composites over --bg while a normal card's composites over --surface — two backdrops, both passing AA (--off on --bg 5.79:1, --warn on --surface 5.00:1, tightest case --warn on --bg 4.61:1).

Leakage, checked in both directions. No #46 class appears anywhere in #47's markup: .facet-option, .facet-group, .filter-chip, .result-count, .browser matched 0 elements on both detail pages, while matching 26 on /products. Author47b ran the mirror test — element.matches() for all 22 composed rules touching my 14 region classes against every element on four detail pages, 138–158 elements each — and got zero matches on all four. Our two independent measurements agree on every value.

Re-checked after the deletion, not just before, as you asked: the deletion moved my nearest edit to within ~77 lines of #47's .stock-badge hunk, so I re-composed at 6fece92 and re-ran all of the above. Still 0 property collisions, still no conflicts, and the deleted selectors match nothing and appear in no stylesheet on the composed build either.

Re-verification at 6fece92

Worktree clean, globals.css confirmed byte-identical to pre-composition-test.

# tests 80
# suites 16
# pass 80
# fail 0
# skipped 0
# todo 0

✓ Compiled successfully in 2.6s
  Finished TypeScript in 2.5s
✓ Generating static pages using 13 workers (32/32) in 573ms

Checked 4 files in 12ms. No fixes applied.
Found 2 warnings.

Same 2 pre-existing noDescendingSpecificity warnings, unchanged from 55f5d72.

Full acceptance re-driven in Chromium at this commit, since the apply → setPicked refactor touched every control: unfiltered 16 produk with all four count rows correct → tick Aki, own counts held at Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3 while the other three narrowed → add Ban and Pajero / Fortuner, 2 produk, URL ?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner → hard reload reproduced all three chips, both ticks, 2 produk and identical count rows → removing the Pajero / Fortuner chip cleared only Kendaraan, 4 produk → Hapus semua filter returned to 16 produk, empty URL, state identical to the unfiltered start.

Judgement calls left alone

Per your instruction: three Ketersediaan checkboxes and the Filter (n) label both stand as originally argued, unchanged in these two commits.

Merging still deploys to production.

Composition re-confirmed at current heads

#55's head moved to 3daea4b after I first measured against b830657, so the earlier number is refreshed rather than left to stand:

  • git merge-tree --write-tree HEAD origin/feat/47-product-detail → no conflicts at 6fece92 × 3daea4b. Three-way compose exits 0 with 0 conflict markers.
  • Their globals.css delta since b830657 is comment text only. Proven by stripping every /* … */ block from both revisions and diffing the remainder: identical, 823 rule lines, zero declarations or selectors changed.
  • The composed sheet's rule content is therefore byte-identical to the composition I measured, so the 0-of-1770 property-collision result carries to current heads unchanged.
  • Our file sets intersect only at globals.css. Everything else on #55 — cart-view.tsx, [slug]/add-to-cart.tsx, [slug]/page.tsx, lib/cart.ts, lib/checkout-message.ts and their tests — is in this PR's explicit non-goals and untouched here.

Positional collision class — checked, and it is the one a selector audit cannot see

Author47b caught a defect on #55 worth recording here, because the visible damage would have landed on this PR. At their head 8f160d1 they appended a new @media (max-width: 600px) block at end of file in globals.css — the same anchor this PR's #46 region appends to. git merge-tree went to exit 1, CONFLICT in both orders, and the natural keep-both resolution would have nested this PR's entire facet region inside their .cart-line > :last-child { … } at depth 2. Browsers discard all of it and the sheet is left two braces open, while the file still parses, the rule count is unchanged, and biome stays silent. With no workflow on pull_request and wrangler pages deploy on merge, that resolution ships a storefront with a filter sidebar and no filter styling.

They fixed it at c20ae14 by moving the block beside .cart-line p. Re-verified here at 6fece92 × c20ae14:

  • git merge-tree --write-tree — clean in both orders. Three-way compose exits 0, 0 conflict markers.
  • Brace balance and nesting depth of the composed sheet, which is the check that actually catches this: final depth 0 (balanced), max depth 2, 157 rules, and 0 rules nested inside a non-at-rule block. This PR's 19 rules sit at depth 0 with 3 inside its own @media (max-width: 900px); #55's 12 sit at depth 0 with 2 inside their @media (max-width: 600px).
  • Rendered, not just parsed — the sidebar is alive in the composed build: .facet-group rgb(255,255,255) / 1px solid / 12px radius / 8px 16px padding, .browser grid at 240px 968px, .facet-option flex at 44px, .filter-chip rgb(228,243,232) / 999px / 44px.
  • Computed-value diff re-run at these heads: 0 of 1770 property collisions, the same 8 geometry-only height shifts, nothing else.

Generalising the lesson, since both of us had reported "composition-safe" off selector-level analysis: a selector-by-selector or element.matches() audit structurally cannot detect a positional collision, and conflict markers and rule counts both look fine when this goes wrong. The cheap check that does catch it is to strip comments, walk the braces, and assert final depth 0 and that each region's selectors sit at depth 0 (or inside their own at-rule).

Composition test again done by swapping the composed sheet into this worktree and git restore-ing it; globals.css confirmed byte-identical afterwards, worktree clean, and 80/80 tests plus 32/32 pages re-confirmed at 6fece92.

Closes #46. Merging this PR **deploys to production**: no workflow runs on `pull_request` — both `.forgejo/workflows/*.yml` trigger on `push: main` and they deploy (the store runs `wrangler pages deploy`). The forge never checks this branch, so every green light below is a command I ran locally against commit `7b7a7e5`. ## What changed - **`apps/store/lib/facets.ts`** (new) — the whole filter reduction as a pure module, so it is testable without a component harness. `reduceCatalog` returns the grid contents *and* every facet count in one pass over the catalog. OR inside a group, AND across groups. - **`apps/store/lib/facets.test.ts`** (new) — 24 `node:test` cases over a fixture catalog, matching `lib/checkout-message.test.ts` in style. - **`apps/store/app/products/product-browser.tsx`** — replaces the single-select category button row with a four-group multi-select sidebar (Kategori, Merk, Kendaraan, Ketersediaan), per-option counts, an active-filter chip row, a live result count, and a mobile collapse toggle. The text search is unchanged in behaviour and now AND-s with the facets. - **`apps/store/app/globals.css`** — one appended region commented `#46`, starting at line 849: `.browser`, `.facet-panel`, `.facet-group`, `.facet-option`, `.facet-count`, `.filter-toolbar`, `.result-count`, `.filter-chip`, `.filter-toggle`, plus the `max-width: 900px` block. Every selector is new except two descendant-scoped overrides under `.browser`. **I did not touch `.stock-badge` or any availability rule — that is #47's region.** `app/products/page.tsx` is unchanged: the page already fetched the full catalog, and counts come from that same array — no second data source. ### Counts reflect the *other* active facets, never a facet's own selection This is the subtle requirement. Per item, `reduceCatalog` tallies how many groups it fails. It contributes to a group's counts when it fails **no** group, or fails **only that group** — so ticking one Kategori leaves its siblings reading their real reachable totals instead of collapsing to zero. Two or more misses and no single tick could surface it, so it counts nowhere. Verified live at 1440px with `?kategori=Aki`: | group | counts | | |---|---|---| | Kategori (own) | `Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3` | unchanged — own selection ignored | | Merk | `GS Astra=2`, all others `0` | narrowed by Kategori | | Kendaraan | `Avanza / Xenia=1 Brio / Mobilio=1 Ertiga=0 Innova=1 Pajero / Fortuner=1` | narrowed by Kategori | | Ketersediaan | `Ready=1 Indent=1 Habis=0` | narrowed by Kategori | Options with a reachable count of `0` stay listed but disabled, so the sidebar never reflows under the pointer; an already-ticked option is never disabled, so no tick can become un-undoable. ### URL scheme: repeated keys, no delimiter `fitment` is free text carrying slashes, spaces, ampersands and parentheses, so any separator would need an escape of its own. `URLSearchParams` already round-trips repeated keys losslessly: `?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner`. A pre-existing single-value `?kategori=Oli&q=aki` link still works. Unknown values are kept rather than dropped, so a stale link renders its chip and an honest `0 produk` that one click undoes, instead of silently showing the whole catalog. ## Ketersediaan: three checkboxes, not one "Ready stok" **Decision: one checkbox per state — Ready, Indent, Habis — in that fixed severity order, with the note "Indent bisa dipesan — dikirim setelah stok masuk." above them.** The issue specifies a single "Ready stok" checkbox, but it was written against the `inStock` boolean that #41 removed. Reasoning for changing it: - A lone "Ready stok" box has only two states, so unticking it means "ready plus everything else" and ticking it means "ready only". `indent` never gets named either way — it lands in an unlabelled not-ready bucket **together with `habis`**, which is exactly the conflation the tri-state was introduced to remove. - `indent` is *orderable*. Hiding it behind a ready-only filter would suppress buyable stock from anyone who ticks the one obvious box, which costs real orders. - Three boxes also express the useful query the boolean could not: Ready + Indent = everything a visitor can actually buy today, `habis` excluded. - Ordered ready → indent → habis rather than alphabetically, because availability is a defined scale, not free text. No wording implies indent is unavailable: the option reads `Indent`, the note says it can be ordered and ships once stock arrives, and the facet is independently selectable (ticking `Indent` alone yields its 4 items, verified below). ## Verified All at commit `7b7a7e5`, worktree clean. **Test suite** — `pnpm --filter garage-store test`: ``` # tests 80 # suites 16 # pass 80 # fail 0 # skipped 0 # todo 0 ``` 56 pre-existing plus 24 new. The two that pin the subtle requirement are `leaves a group's own counts untouched by its own selection` and `counts what selecting the option would actually yield` (it ticks the option and asserts the resulting item count equals the number the sidebar promised). Also covered: OR-within/AND-across, an item that misses two groups counting nowhere, a repeated fitment value counting once, blank/null brand and empty fitment producing no blank option and no match, and the awkward-value round-trip (`Body & lampu`, `KYB + Kayaba`, `Avanza / Xenia (2019)`). **Build** — `SITE_URL=… GARAGE_API_URL=… CATALOG_API_KEY=… pnpm --filter garage-store build`: ``` ✓ Compiled successfully in 10.0s Finished TypeScript in 2.7s ✓ Generating static pages using 13 workers (32/32) in 640ms ├ ○ /products ├ ● /products/[slug] ``` **Catalog endpoint for the build:** the store has no fixture, so I served one — `/tmp/wt46-catalog-server.mjs`, a 16-item `schemaVersion: 3` payload on `/api/public/catalog` behind the `x-api-key` header, sized so every facet count is checkable by hand. It exercises the awkward cases on purpose: 5 items with `brand: null`, 3 with empty `fitment`, multi-value fitment, all three availability states, and a `null` price. `/tmp/wt46-static.mjs` then serves `out/` the way Cloudflare Pages does (clean URLs resolve to `<path>.html`) so query strings survive a reload. Both scripts are throwaway verification tooling and are **not** committed. **Biome** — `pnpm exec biome check lib/facets.ts lib/facets.test.ts app/products/product-browser.tsx app/globals.css`: ``` Checked 4 files in 15ms. No fixes applied. Found 2 warnings. ``` Both warnings are `noDescendingSpecificity` on the pre-existing `.image-placeholder` / `.cart-line .image-placeholder` rules at lines 573 and 641 — above my region, which starts at line 849. `biome check` on `git show 55f5d72:apps/store/app/globals.css` reports the same `Found 2 warnings`, so my change introduces none. ### Browser — 1440 × 900 Real Chromium against the served static export. - Unfiltered: `16 produk`, 16 cards, sidebar `240px 968px`, toggle `display: none`. Counts summed correctly out of the gate — Kategori `Aki 2 / Ban 2 / Body & lampu 2 / Filter 3 / Kaki-kaki 4 / Oli 3` = 16; Ketersediaan `Ready 10 / Indent 4 / Habis 2` = 16; Merk summed to 11, the 5 `brand: null` items correctly absent with **no blank option**; Kendaraan omitted the 3 empty-`fitment` items. - **Multi-select OR:** `?kategori=Aki&kategori=Ban` → `4 produk`, exactly the 2 Aki + 2 Ban items. Kategori's own counts stayed at their unfiltered values; Merk narrowed to `Dunlop 2, GS Astra 2`. - **AND across groups:** adding `kendaraan=Pajero / Fortuner` → `2 produk` (`Aki N50 Basah`, `Ban 265/65 R17`). Kategori counts became `Aki 1 / Ban 1 / Kaki-kaki 1 / rest 0` — narrowed by Kendaraan only, still ignoring its own two ticks — while Kendaraan's own counts held at their previous values. Both directions correct simultaneously. - **URL round-trip via hard reload:** reloaded `?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner` from scratch. Checkboxes, all three chips, `2 produk` and both cards restored identically — the filtered view is linkable, not just in-memory state. - **Chip removal:** clicking `Pajero / Fortuner ×` cleared only Kendaraan → `4 produk`, Kategori ticks intact, URL back to `?kategori=Aki&kategori=Ban`. `Hapus semua filter` → `16 produk`, empty query string. - **Ketersediaan:** `Indent` alone → `4 produk`, the four indent items. Ready + Indent → `14 produk` (16 minus the 2 `habis`). - **Search AND-s and round-trips:** typing `aki` on top of Ready + Indent → `2 produk`, URL `?ketersediaan=indent&ketersediaan=ready&q=aki`; reloading that URL restored the same state and the same narrowed availability counts (`Ready 1 / Indent 1 / Habis 0`). ### Browser — 375 × 812 - Sidebar collapsed by default: `.facet-panel` `display: none`, `aria-expanded="false"`, `.browser` a single `343px` column. - Toggle reads `Filter (2) ▾` — it carries the active-filter count, so a collapsed sidebar still says filters are on. Hit area **46 × 343px**, full width. - Tapping it expands (`display: flex`, `aria-expanded="true"`, `aria-controls="product-facets"` matching the panel `id`); tapping again re-collapses. Verified with real mouse events at the button's centre coordinates, both directions. - **44px tap targets:** every `.facet-option` measured exactly 44px and every `.filter-chip` 44px, at the expanded state. - No horizontal overflow: `documentElement.scrollWidth` 375 = viewport width. - Counts, chips and result count all behaved as on desktop (`?kategori=Aki&kendaraan=Pajero+%2F+Fortuner` → `1 produk`, correct). ## Deviations - **The mobile toggle is labelled `Filter (n)`, not the mockup's `Filter & urutkan`.** There is no sort control in this PR — the issue specifies four facet groups and no sort dimension, and none of its acceptance criteria mention ordering. Labelling the button "& urutkan" would advertise a control that does not exist, so it names only what it opens. The collapse behaviour the mockup asks for is implemented and verified above. Adding sorting is a separate issue; the label should change with it. - **`0`-count options render disabled rather than hidden.** The issue does not say which. Hiding them would reflow the sidebar under the pointer mid-click; an already-ticked option is never disabled, so nothing becomes un-undoable. - **The search term gets no chip**, unlike the facets. It is already visible in its own box and clears there, so a second removal affordance for it would be redundant. `Hapus semua filter` deliberately clears the facets and leaves the query text, since the box still shows it. - Free-text search still reads name, description, brand and part number — unchanged from the previous browser. `oemNumbers` and `fitment` are facets here; widening the haystack is not this issue's scope. - Brand and fitment match exactly after trimming, with no case folding, since both come from managed dashboard fields. Flagged with a `ponytail:` comment in `facets.ts` naming case-folding as the upgrade path if the dashboard ever admits free-text brands. --- # Round 2 — review id=13 (`approve-with-nits`) and the composition cross-check Head is now `6fece92` (was `7b7a7e5`). Two commits on top: - `c6d956b` — `chore(store): delete dead .product-filter css left by the facet rewrite` - `6fece92` — `fix(store): debounce the facet url write and clear the query on reset` ## should — dead `.product-filter*` CSS deleted Deleted `globals.css` lines 380–413: `.product-filters`, `.product-filters-legend`, `.product-filter` and `.product-filter[aria-pressed="true"]`. This PR removed their only consumer (the single-select category button row), so they were orphaned by this PR and are this PR's to remove. **Proof the deletion is safe**, not an assumption: 1. **Zero consumers.** `grep` for `product-filter` across the repo returns four hits, none of them markup: the four CSS rules themselves, a prose comment at `globals.css:67`, and one docs line. No `.tsx`, `.ts`, `.jsx`, `.html` or template file references any of the three classes. 2. **Nothing can match them.** In the built site I queried `.product-filters, .product-filters-legend, .product-filter` on all 8 routes — `/products`, two detail pages, `/`, `/cart`, `/services`, `/contact`, `/cara-pesan` — across 1124 elements: **0 matches on every page**, and the rules are absent from `document.styleSheets`. Rules that match nothing cannot change rendering. 3. **Rendering is byte-identical.** I re-measured 1770 computed values on `/products` — 27 selectors × colour/background/border/radius/font/padding/min-height/display/opacity/flex/cursor/gap plus bounding boxes — at 1440×1000 and 375×812, panel collapsed and expanded, filtered and unfiltered, before and after the deletion. **0 of 1770 values changed.** I deliberately did **not** widen this into general CSS tidying: the two pre-existing `noDescendingSpecificity` warnings and every other unused-looking rule are untouched. Only classes whose consumer this PR removed. **One thing I did not touch and am flagging rather than fixing:** `docs/store-overhaul-spec.md:260` reads "`.product-filter` and `.gallery-thumb` need a `min-height: 44px` audit before the cart quantity steppers land". `.product-filter` no longer exists after this commit, so that line is now stale. It is a one-line docs fix, but it is a different file with an unclear owner and outside the scope you set, so I left it. The requirement behind it is satisfied: the replacement control `.facet-option` measures exactly 44px, verified on all 21 rows. Say the word and I will correct the line. ## nit `globals.css:67` — accepted The comment justifying `font: inherit` on form controls cited "the `.product-filter` chips", a class this commit deletes. Repointed at `.facet-option` checkboxes and `.filter-chip` buttons, which are the controls that actually need it now. ## nit `product-browser.tsx:123` — accepted, URL write debounced Real platform limit, and leaving it would make a long search query throw on iOS Safari — a large share of this storefront's traffic. The URL write moved out of the change handler into a 200 ms debounced `useEffect` keyed on `picked`. The input stays controlled by state, so typing is unaffected; only the history write defers. Measured, instrumenting `window.history.replaceState` — note it must be patched as an **own property**, since Next's App Router installs its own there and a `History.prototype` patch is bypassed (my first attempt read a false 0 for that reason): | action | writes during the burst | after it settles | |---|---|---| | 25-keystroke query | **0** | 2, URL `?q=shockbreaker+depan+avanza` | | one facet tick | — | 2, URL `?kategori=Oli` | Writes now scale with *settled interactions*, not keystrokes: 25 keystrokes cost 2 writes where they previously cost one each. Both writes carry an identical URL and the count is 2 per settled action whether the action was 1 keystroke or 25; I did not chase which layer emits the duplicate, only established it does not scale with typing. Safari's ~100-per-30s ceiling now needs ~50 distinct interactions in 30 s, which is not a typing pattern. Round-trip still holds: after the debounce settles, a hard reload of the written URL restored the query text, the ticks and `1 produk`. ## nit `product-browser.tsx:151` — accepted, `q` is now cleared too Of the two options offered I took clearing `q` rather than relabelling, because it makes the stronger label honest instead of weakening the label to match a partial action. `Hapus semua filter` now resets to `EMPTY_SELECTION`: on `?kategori=Aki&q=aki` it lands on `` / `16 produk` / empty search box, where it previously left `?q=aki` and `2 produk`. The emptied input is the visible receipt, and per-facet chips still exist for dropping one facet while keeping the search. Verified: `?kategori=Aki&q=aki` → click → URL ``, input ``, `16 produk`, 0 chips. Also removed the `const apply = setPicked` alias the debounce left behind — with the URL write in the effect it was pure indirection, so the five call sites now call `setPicked` directly. ## Composition cross-check with #55 (#47) — clean, measured both ways You were right that textual non-conflict is not the risk. I composed both branches' `globals.css` in my own worktree (`git merge-file` three-way, `git restore`d afterwards — worktree confirmed byte-identical and clean), built it, and measured the same 1770 values with **one shared probe file** so a difference could not come from a difference in the probe. `git merge-tree --write-tree HEAD origin/feat/47-product-detail` → **no conflicts**. Line arithmetic checks out: 847 base + 148 mine − 34 deleted + 53 theirs = 1014 composed, 0 conflict markers. **My region against theirs — 0 property collisions.** Of 1770 values, `#46` property differences: **0**. No colour, background, border, display, font, spacing, cursor, flex or specificity value in my region changes when #47 is composed in. The only movement is 8 container *heights* — `.product-grid` 662→674 px, `.browser-results` 1125→1143 px and so on — which is the correct downstream consequence of their chips growing 19→25 px tall, not a style collision. **Their chips on my page — all three states correct.** On the composed sheet, measured on `/products` and two detail pages: | state | markup | `--chip` | colour | fill | |---|---|---|---|---| | ready | `<span>` (detail only) | `#2d543c` `--ok` | `rgb(45, 84, 60)` | `color(srgb 0.176471 0.329412 0.235294 / 0.12)` | | indent | `<p>` in card | `#8a5a12` `--warn` | `rgb(138, 90, 18)` | `color(srgb 0.541176 0.352941 0.0705882 / 0.12)` | | habis | `<p>` in card | `#8c3a32` `--off` | `rgb(140, 58, 50)` | `color(srgb 0.54902 0.227451 0.196078 / 0.12)` | `is-indent` resolves `--warn`, **not** `--off` — the wave-1 `#52`/`#53` defect does not return. Ready chips on cards: **0**, correct, `product-card.tsx` returns null for ready. All at 1px border, 999px radius, `align-self: start`, boxes 58×25 (Indent) and 52×25 (Habis). **No third surface behind their tint.** `.browser`, `.browser-results`, `.facet-panel`, `.filter-toolbar` and `.product-grid` all compute `rgba(0, 0, 0, 0)`; only `.facet-group` adds `--surface`, and no product card is a descendant of it. Their 12 % fill keeps compositing over exactly the two bases they sized it for. One finding neither of us owns, which `Author47b` independently confirmed: `.product-card.is-out { background: transparent }` is pre-existing on `55f5d72`, so Habis cards are unfilled and their chip composites over `--bg` while a normal card's composites over `--surface` — two backdrops, both passing AA (`--off` on `--bg` 5.79:1, `--warn` on `--surface` 5.00:1, tightest case `--warn` on `--bg` 4.61:1). **Leakage, checked in both directions.** No `#46` class appears anywhere in `#47`'s markup: `.facet-option, .facet-group, .filter-chip, .result-count, .browser` matched **0** elements on both detail pages, while matching 26 on `/products`. `Author47b` ran the mirror test — `element.matches()` for all 22 composed rules touching my 14 region classes against every element on four detail pages, 138–158 elements each — and got zero matches on all four. Our two independent measurements agree on every value. **Re-checked after the deletion, not just before**, as you asked: the deletion moved my nearest edit to within ~77 lines of #47's `.stock-badge` hunk, so I re-composed at `6fece92` and re-ran all of the above. Still 0 property collisions, still no conflicts, and the deleted selectors match nothing and appear in no stylesheet on the composed build either. ## Re-verification at `6fece92` Worktree clean, `globals.css` confirmed byte-identical to pre-composition-test. ``` # tests 80 # suites 16 # pass 80 # fail 0 # skipped 0 # todo 0 ``` ``` ✓ Compiled successfully in 2.6s Finished TypeScript in 2.5s ✓ Generating static pages using 13 workers (32/32) in 573ms ``` ``` Checked 4 files in 12ms. No fixes applied. Found 2 warnings. ``` Same 2 pre-existing `noDescendingSpecificity` warnings, unchanged from `55f5d72`. Full acceptance re-driven in Chromium at this commit, since the `apply` → `setPicked` refactor touched every control: unfiltered `16 produk` with all four count rows correct → tick `Aki`, own counts held at `Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3` while the other three narrowed → add `Ban` and `Pajero / Fortuner`, `2 produk`, URL `?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner` → hard reload reproduced all three chips, both ticks, `2 produk` and identical count rows → removing the `Pajero / Fortuner` chip cleared only Kendaraan, `4 produk` → `Hapus semua filter` returned to `16 produk`, empty URL, state identical to the unfiltered start. ## Judgement calls left alone Per your instruction: three Ketersediaan checkboxes and the `Filter (n)` label both stand as originally argued, unchanged in these two commits. Merging still deploys to production. ### Composition re-confirmed at current heads `#55`'s head moved to `3daea4b` after I first measured against `b830657`, so the earlier number is refreshed rather than left to stand: - `git merge-tree --write-tree HEAD origin/feat/47-product-detail` → **no conflicts** at `6fece92` × `3daea4b`. Three-way compose exits 0 with 0 conflict markers. - Their `globals.css` delta since `b830657` is **comment text only**. Proven by stripping every `/* … */` block from both revisions and diffing the remainder: **identical**, 823 rule lines, zero declarations or selectors changed. - The composed sheet's rule content is therefore byte-identical to the composition I measured, so the **0-of-1770 property-collision** result carries to current heads unchanged. - Our file sets intersect only at `globals.css`. Everything else on `#55` — `cart-view.tsx`, `[slug]/add-to-cart.tsx`, `[slug]/page.tsx`, `lib/cart.ts`, `lib/checkout-message.ts` and their tests — is in this PR's explicit non-goals and untouched here. ### Positional collision class — checked, and it is the one a selector audit cannot see `Author47b` caught a defect on `#55` worth recording here, because the visible damage would have landed on **this** PR. At their head `8f160d1` they appended a new `@media (max-width: 600px)` block at **end of file** in `globals.css` — the same anchor this PR's `#46` region appends to. `git merge-tree` went to `exit 1, CONFLICT` in both orders, and the natural keep-both resolution would have nested this PR's *entire* facet region inside their `.cart-line > :last-child { … }` at depth 2. Browsers discard all of it and the sheet is left two braces open, while the file still parses, the rule count is unchanged, and `biome` stays silent. With no workflow on `pull_request` and `wrangler pages deploy` on merge, that resolution ships a storefront with a filter sidebar and no filter styling. They fixed it at `c20ae14` by moving the block beside `.cart-line p`. Re-verified here at `6fece92` × `c20ae14`: - `git merge-tree --write-tree` — **clean in both orders**. Three-way compose exits 0, 0 conflict markers. - **Brace balance and nesting depth** of the composed sheet, which is the check that actually catches this: final depth **0 (balanced)**, max depth 2, 157 rules, and **0 rules nested inside a non-at-rule block**. This PR's 19 rules sit at depth 0 with 3 inside its own `@media (max-width: 900px)`; `#55`'s 12 sit at depth 0 with 2 inside their `@media (max-width: 600px)`. - **Rendered, not just parsed** — the sidebar is alive in the composed build: `.facet-group` `rgb(255,255,255)` / `1px solid` / `12px` radius / `8px 16px` padding, `.browser` `grid` at `240px 968px`, `.facet-option` `flex` at `44px`, `.filter-chip` `rgb(228,243,232)` / `999px` / `44px`. - Computed-value diff re-run at these heads: **0 of 1770 property collisions**, the same 8 geometry-only height shifts, nothing else. Generalising the lesson, since both of us had reported "composition-safe" off selector-level analysis: a selector-by-selector or `element.matches()` audit structurally cannot detect a *positional* collision, and conflict markers and rule counts both look fine when this goes wrong. The cheap check that does catch it is to strip comments, walk the braces, and assert final depth 0 **and** that each region's selectors sit at depth 0 (or inside their own at-rule). Composition test again done by swapping the composed sheet into this worktree and `git restore`-ing it; `globals.css` confirmed byte-identical afterwards, worktree clean, and `80/80` tests plus `32/32` pages re-confirmed at `6fece92`.
yuris reviewed 2026-08-27 12:53:11 +00:00
yuris left a comment
Copy link

VERDICT: approve-with-nits

Reviewed at head 7b7a7e5 (merge base 55f5d72). Built and drove the branch in a throwaway detached worktree, not the main checkout.

How I verified. I did not reuse the author's fixture. I wrote my own 16-item schemaVersion: 3 payload (/tmp/wt54-fixture.mjs) served on /api/public/catalog behind x-api-key, sized so every count is hand-checkable: 6 categories (Aki 2 / Ban 2 / Body & lampu 2 / Filter 3 / Kaki-kaki 4 / Oli 3), 5 items with brand: null, 3 with fitment: [], one item repeating the same fitment string twice, all three availability states (ready 10 / indent 4 / habis 2), one price: null, and deliberately delimiter-hostile values KYB + Kayaba, Pajero / Fortuner, Body & lampu, Ban 185/65 R15. pnpm exec next build succeeded; out/ served by a Pages-alike static server (clean URLs → <path>.html, query strings preserved) and driven in real Chromium at 1440×900 and 375×812.

Own-facet count isolation — holds, in both directions, simultaneously

Measured, not taken on trust.

  • Ticking Kategori = Aki left Kategori reading Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3 — byte-identical to the unfiltered row. Its own selection did not collapse its siblings.
  • The same tick narrowed the other three: Merk → GS Astra=2 with the other six at 0, Kendaraan → Avanza / Xenia=1 Brio / Mobilio=1 Ertiga=0 Innova=1 Pajero / Fortuner=1, Ketersediaan → Ready=1 Indent=1 Habis=0.
  • Both at once: with ?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner, Kategori read Aki=1* Ban=1* Kaki-kaki=1 rest 0 (narrowed by Kendaraan only, still ignoring its own two ticks) while Kendaraan held Avanza / Xenia=2 Brio / Mobilio=1 Ertiga=1 Innova=1 Pajero / Fortuner=2* (narrowed by Kategori only). Neither group narrowed itself.
  • Counts are honest: Kendaraan Pajero / Fortuner=2 under kategori=Aki,Ban, and ticking it yielded exactly 2 produk.

It is implemented in the pure module, not patched in the component: facets.ts tallies per item how many groups it misses and contributes to a group's counts only when misses === 0 || (misses === 1 && missed === key). product-browser.tsx only reads counts[key].get(value) ?? 0.

URL encoding survives a value containing the delimiter — yes

Repeated keys, no delimiter, so there is no delimiter to break. The sharpest case is KYB + Kayaba, where a bare + decodes to a space: it serialises to ?merk=KYB+%2B+Kayaba and a hard reload of that URL restored the checkbox, the chip reading exactly KYB + Kayaba, and 2 produk. Same for ?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner — cold load reproduced all three chips, both ticked categories, 2 produk and the identical count rows. Pre-multi-select single-value links (?kategori=Oli&q=aki) still resolve, and a stale value is kept rather than dropped: ?kategori=Kategori%20Lama renders its chip and an honest 0 produk one click from undone, instead of silently showing all 16.

Null brand / empty fitment — correct, no blank option, no crash

Merk offered 7 options summing to 11, so the 5 brand: null items are absent with no blank or undefined entry; Kendaraan offered 5 options and omitted the 3 empty-fitment items. They do not match the facet rather than matching vacuously: ?merk=Shell&kendaraan=Innova (both Shell items have empty fitment) → 0 produk. The repeated Avanza / Xenia on one item counted once (5, not 6). Unit tests cover brand: " " and fitment: ["", " "] too.

CSS stayed out of #47's region — yes

The globals.css diff is one hunk, @@ -845,3 +845,151 @@, zero removed lines — pure append. .stock-badge, .stock-badge.is-out and .stock-badge.is-indent sit at lines 497–506 and are untouched, as is every rule above 845. Biome: Found 2 warnings on the branch and Found 2 warnings on git show 55f5d72:apps/store/app/globals.css — the pre-existing noDescendingSpecificity pair, count unchanged.

Ketersediaan: three checkboxes — the reasoning holds, I'd ship it

The issue's single "Ready stok" box was written against the inStock boolean #41 deleted, and the author's argument survives contact with the data. A two-state box can only express "ready only" or "ready plus an unnamed remainder", and that remainder is indent and habis in one bucket — precisely the conflation the tri-state exists to remove. Since indent is orderable, a visitor ticking the one obvious box would have buyable stock hidden from them. Three boxes also express the query the boolean cannot: Ready + Indent = everything purchasable today, habis excluded — measured at 14 produk on a 16-item catalog with 2 habis.

Nothing implies an indent item is unavailable. The option reads Indent, the group carries Indent bisa dipesan — dikirim setelah stok masuk. above the options, and the facet is independently selectable: ?ketersediaan=indent alone → 4 produk, the four indent items. The fixed ready → indent → habis order rather than alphabetical is right for a defined scale.

Toggle label Filter (n) instead of Filter & urutkan — sound, keep it

& urutkan names a sort control this PR does not ship and the issue does not ask for; the label would advertise a dead affordance, which is a worse failure than deviating from mockup copy. What the mockup actually specifies — collapsing the whole sidebar behind one toggle — is implemented. Carrying the active count is a genuine improvement: a collapsed panel still announces that two filters are on. The label should gain & urutkan in the same PR that adds sorting.

Mobile (375×812)

Collapsed by default (display: none, aria-expanded="false", .browser one 343px column), and real mouse clicks at the button centre expanded and re-collapsed it, aria-controls="product-facets" matching the panel id both ways. All 21 .facet-option rows measured exactly 44px when expanded and all 3 chips 44px; the toggle is 343×46. documentElement.scrollWidth 375 = viewport, no horizontal overflow. Chips and the result count sit outside the collapsible panel, so a collapsed sidebar still shows what is filtered.

Tests: real coverage, not decoration

node --experimental-strip-types --test lib/*.test.ts → # tests 80 # pass 80 # fail 0, 56 pre-existing plus 24 new. I mutation-tested facets.ts in the worktree, four ways, and every mutation went red:

mutation result
drop own-facet isolation (misses > 1 || … → misses > 0) 3 fail, incl. leaves a group's own counts untouched by its own selection
OR → AND inside a group (own.some → own.every) 3 fail, incl. counts what selecting the option would actually yield
repeated keys → comma delimiter 3 fail, incl. survives values carrying the characters a delimiter would break
stop dropping blank values in clean() 3 fail across all three suites

Static export

No new fetch, no route handler, no server component change — the diff is 4 files and app/products/page.tsx is untouched. /products still builds as ○ (Static). Filtering is entirely client-side over the array the page already fetched.

apps/store/app/globals.css:L380: should: .product-filters, .product-filters-legend, .product-filter and .product-filter[aria-pressed="true"] (lines 380–413) are dead — this PR deleted their only consumer and product-filter now has zero matches in any .tsx/.ts/.html in the repo. Delete lines 380–413; they are outside both #46's and #47's regions, so removing them now is a smaller conflict than leaving them to rot.
apps/store/app/globals.css:L67: nit: the comment still cites "the .product-filter chips" as the reason controls need font: inherit, naming a class that no longer exists. Point it at .facet-option input / .filter-chip instead.
apps/store/app/products/product-browser.tsx:L123: nit: every keystroke in the search box calls history.replaceState (67 keystrokes in 924 ms produced 67 calls). Chromium logged no error, but Safari throttles replaceState to ~100 calls per 30 s and throws SecurityError past it — I could not test WebKit here, so this is a named platform limit rather than an observed break. Debounce the URL write for q (~200 ms) and leave the input controlled by state.
apps/store/app/products/product-browser.tsx:L151: nit: Hapus semua filter deliberately keeps q, so on ?kategori=Aki&q=avanza it lands on ?q=avanza and 3 produk rather than the full 16 — a button labelled "remove all filters" that leaves one applied. The text stays visible in the box, so it is discoverable, but either clear q too or label it Hapus filter.

VERDICT: approve-with-nits Reviewed at head `7b7a7e5` (merge base `55f5d72`). Built and drove the branch in a throwaway detached worktree, not the main checkout. **How I verified.** I did not reuse the author's fixture. I wrote my own 16-item `schemaVersion: 3` payload (`/tmp/wt54-fixture.mjs`) served on `/api/public/catalog` behind `x-api-key`, sized so every count is hand-checkable: 6 categories (Aki 2 / Ban 2 / Body & lampu 2 / Filter 3 / Kaki-kaki 4 / Oli 3), **5 items with `brand: null`**, **3 with `fitment: []`**, one item repeating the same fitment string twice, all three availability states (ready 10 / indent 4 / habis 2), one `price: null`, and deliberately delimiter-hostile values `KYB + Kayaba`, `Pajero / Fortuner`, `Body & lampu`, `Ban 185/65 R15`. `pnpm exec next build` succeeded; `out/` served by a Pages-alike static server (clean URLs → `<path>.html`, query strings preserved) and driven in real Chromium at 1440×900 and 375×812. ### Own-facet count isolation — holds, in both directions, simultaneously Measured, not taken on trust. - Ticking `Kategori = Aki` left Kategori reading `Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3` — byte-identical to the unfiltered row. Its own selection did not collapse its siblings. - The same tick narrowed the other three: `Merk` → `GS Astra=2` with the other six at 0, `Kendaraan` → `Avanza / Xenia=1 Brio / Mobilio=1 Ertiga=0 Innova=1 Pajero / Fortuner=1`, `Ketersediaan` → `Ready=1 Indent=1 Habis=0`. - Both at once: with `?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner`, Kategori read `Aki=1* Ban=1* Kaki-kaki=1 rest 0` (narrowed by Kendaraan only, still ignoring its own two ticks) while Kendaraan held `Avanza / Xenia=2 Brio / Mobilio=1 Ertiga=1 Innova=1 Pajero / Fortuner=2*` (narrowed by Kategori only). Neither group narrowed itself. - Counts are honest: `Kendaraan Pajero / Fortuner=2` under `kategori=Aki,Ban`, and ticking it yielded exactly `2 produk`. It is implemented in the pure module, not patched in the component: `facets.ts` tallies per item how many groups it misses and contributes to a group's counts only when `misses === 0 || (misses === 1 && missed === key)`. `product-browser.tsx` only reads `counts[key].get(value) ?? 0`. ### URL encoding survives a value containing the delimiter — yes Repeated keys, no delimiter, so there is no delimiter to break. The sharpest case is `KYB + Kayaba`, where a bare `+` decodes to a space: it serialises to `?merk=KYB+%2B+Kayaba` and a **hard reload of that URL** restored the checkbox, the chip reading exactly `KYB + Kayaba`, and `2 produk`. Same for `?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner` — cold load reproduced all three chips, both ticked categories, `2 produk` and the identical count rows. Pre-multi-select single-value links (`?kategori=Oli&q=aki`) still resolve, and a stale value is kept rather than dropped: `?kategori=Kategori%20Lama` renders its chip and an honest `0 produk` one click from undone, instead of silently showing all 16. ### Null `brand` / empty `fitment` — correct, no blank option, no crash Merk offered 7 options summing to 11, so the 5 `brand: null` items are absent with **no blank or `undefined` entry**; Kendaraan offered 5 options and omitted the 3 empty-`fitment` items. They do not match the facet rather than matching vacuously: `?merk=Shell&kendaraan=Innova` (both Shell items have empty fitment) → `0 produk`. The repeated `Avanza / Xenia` on one item counted once (5, not 6). Unit tests cover `brand: " "` and `fitment: ["", " "]` too. ### CSS stayed out of #47's region — yes The `globals.css` diff is one hunk, `@@ -845,3 +845,151 @@`, **zero removed lines** — pure append. `.stock-badge`, `.stock-badge.is-out` and `.stock-badge.is-indent` sit at lines 497–506 and are untouched, as is every rule above 845. Biome: `Found 2 warnings` on the branch and `Found 2 warnings` on `git show 55f5d72:apps/store/app/globals.css` — the pre-existing `noDescendingSpecificity` pair, count unchanged. ### Ketersediaan: three checkboxes — the reasoning holds, I'd ship it The issue's single "Ready stok" box was written against the `inStock` boolean #41 deleted, and the author's argument survives contact with the data. A two-state box can only express "ready only" or "ready plus an unnamed remainder", and that remainder is `indent` and `habis` in one bucket — precisely the conflation the tri-state exists to remove. Since `indent` is orderable, a visitor ticking the one obvious box would have buyable stock hidden from them. Three boxes also express the query the boolean cannot: Ready + Indent = everything purchasable today, `habis` excluded — measured at `14 produk` on a 16-item catalog with 2 `habis`. Nothing implies an indent item is unavailable. The option reads `Indent`, the group carries `Indent bisa dipesan — dikirim setelah stok masuk.` above the options, and the facet is independently selectable: `?ketersediaan=indent` alone → `4 produk`, the four indent items. The fixed ready → indent → habis order rather than alphabetical is right for a defined scale. ### Toggle label `Filter (n)` instead of `Filter & urutkan` — sound, keep it `& urutkan` names a sort control this PR does not ship and the issue does not ask for; the label would advertise a dead affordance, which is a worse failure than deviating from mockup copy. What the mockup actually specifies — collapsing the whole sidebar behind one toggle — is implemented. Carrying the active count is a genuine improvement: a collapsed panel still announces that two filters are on. The label should gain `& urutkan` in the same PR that adds sorting. ### Mobile (375×812) Collapsed by default (`display: none`, `aria-expanded="false"`, `.browser` one `343px` column), and real mouse clicks at the button centre expanded and re-collapsed it, `aria-controls="product-facets"` matching the panel `id` both ways. All 21 `.facet-option` rows measured exactly 44px when expanded and all 3 chips 44px; the toggle is 343×46. `documentElement.scrollWidth` 375 = viewport, no horizontal overflow. Chips and the result count sit outside the collapsible panel, so a collapsed sidebar still shows what is filtered. ### Tests: real coverage, not decoration `node --experimental-strip-types --test lib/*.test.ts` → `# tests 80 # pass 80 # fail 0`, 56 pre-existing plus 24 new. I mutation-tested `facets.ts` in the worktree, four ways, and every mutation went red: | mutation | result | |---|---| | drop own-facet isolation (`misses > 1 \|\| …` → `misses > 0`) | 3 fail, incl. `leaves a group's own counts untouched by its own selection` | | OR → AND inside a group (`own.some` → `own.every`) | 3 fail, incl. `counts what selecting the option would actually yield` | | repeated keys → comma delimiter | 3 fail, incl. `survives values carrying the characters a delimiter would break` | | stop dropping blank values in `clean()` | 3 fail across all three suites | ### Static export No new fetch, no route handler, no server component change — the diff is 4 files and `app/products/page.tsx` is untouched. `/products` still builds as `○ (Static)`. Filtering is entirely client-side over the array the page already fetched. --- apps/store/app/globals.css:L380: should: `.product-filters`, `.product-filters-legend`, `.product-filter` and `.product-filter[aria-pressed="true"]` (lines 380–413) are dead — this PR deleted their only consumer and `product-filter` now has zero matches in any `.tsx`/`.ts`/`.html` in the repo. Delete lines 380–413; they are outside both #46's and #47's regions, so removing them now is a smaller conflict than leaving them to rot. apps/store/app/globals.css:L67: nit: the comment still cites "the `.product-filter` chips" as the reason controls need `font: inherit`, naming a class that no longer exists. Point it at `.facet-option input` / `.filter-chip` instead. apps/store/app/products/product-browser.tsx:L123: nit: every keystroke in the search box calls `history.replaceState` (67 keystrokes in 924 ms produced 67 calls). Chromium logged no error, but Safari throttles `replaceState` to ~100 calls per 30 s and throws `SecurityError` past it — I could not test WebKit here, so this is a named platform limit rather than an observed break. Debounce the URL write for `q` (~200 ms) and leave the input controlled by state. apps/store/app/products/product-browser.tsx:L151: nit: `Hapus semua filter` deliberately keeps `q`, so on `?kategori=Aki&q=avanza` it lands on `?q=avanza` and `3 produk` rather than the full 16 — a button labelled "remove all filters" that leaves one applied. The text stays visible in the box, so it is discoverable, but either clear `q` too or label it `Hapus filter`.
yuris reviewed 2026-08-27 14:30:15 +00:00
yuris left a comment
Copy link

VERDICT: approve-with-nits

Round 2 at head 6fece92 (previous review id 13 covered 7b7a7e5). This is a merge signal — do not open a round 3 for the two nits below, neither blocks. Rebuilt and re-drove from scratch in a fresh throwaway detached worktree at 6fece92 with my own 16-item fixture, not your rig and not your numbers.

All four round-1 findings are fixed, and I confirmed each one rather than reading the commit messages.

c6d956b — dead CSS deletion: safe, and I proved it independently

Deleted exactly lines 380–413 (.product-filters, .product-filters-legend, .product-filter, .product-filter[aria-pressed="true"]), nothing else. Three checks of my own:

  • grep tool for product-filter across the repo at 6fece92: one hit, docs/store-overhaul-spec.md:260. Zero in any .tsx/.ts/.html/.css.
  • Enumerated the shipped stylesheet at runtime on /products (162 rules across document.styleSheets): 0 .product-filter* rules, while .product-search (1) and .product-controls (2) survive — correct, the home page's GET form still uses both — and 10 .facet-* rules are live. document.querySelectorAll('[class*="product-filter"]') → 0 elements.
  • Whole-file brace accounting on globals.css: 150 open, 150 close, final depth 0, minimum depth 0 across 961 lines.

Not widened, as you said: the two pre-existing noDescendingSpecificity warnings are untouched and biome check still reports Found 2 warnings on the same 4 files.

6fece92 — debounce: measured, works, and did not break the refactored controls

I instrumented replaceState the way you flagged — as an own property on window.history, since the App Router installs its own there and a History.prototype patch reads a false zero. Your warning was worth passing on; I patched the own property from the start and got live counts.

  • 67 keystrokes in 589 ms: 0 writes during the burst, 2 after it settled, and the settled URL decoded to exactly ?q= + the input value. Down from 67 synchronous writes at 7b7a7e5, which I had measured. The Safari ~100-calls/30 s SecurityError exposure is gone.
  • Timing directly: 30 ms after a tick, URL still '' and 0 writes; after 300 ms, ?kategori=Aki. Debounce, not a dropped write.
  • The two writes per settle are one component call plus the router's own re-dispatch through the patched property, not a double timer — the recorded targets are identical.

Because apply → setPicked touched all five call sites, I re-drove the whole acceptance set rather than spot-checking:

  • Own-facet isolation, both directions, still exact. Kategori=Aki → Kategori Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3, byte-identical to unfiltered, while Merk collapsed to GS Astra=2 and Ketersediaan to Ready=1 Indent=1 Habis=0. Both at once on ?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner: Kategori Aki=1* Ban=1* Kaki-kaki=1 rest 0 (narrowed by Kendaraan only) while Kendaraan held Avanza / Xenia=2 … Pajero / Fortuner=2* (narrowed by Kategori only).
  • Awkward values still round-trip. The app wrote ?merk=KYB+%2B+Kayaba itself; a cold hard reload of that URL returned the tick, the chip reading exactly KYB + Kayaba, and 2 produk. Same for the three-chip Pajero URL — reload reproduced every count row identically.
  • Chip removal, Indent alone → 4 produk, Ready+Indent → 14 produk of 16 with 2 habis: all unchanged.
  • Mobile 375×812: collapsed by default, real mouse clicks at the button centre expanded and re-collapsed, aria-expanded and aria-controls="product-facets" tracking correctly both ways, every .facet-option exactly 44px expanded, all chips 44px, toggle 343×46, scrollWidth - clientWidth = 0.
  • pnpm --filter garage-store test → 80/80. Build → 32/32 pages, /products still ○ (Static).

Clear-all: ?kategori=Aki&q=aki (2 produk) → empty URL, empty input, 16 produk, all counts back to unfiltered. You chose clearing over relabelling and I think that is the better call — it makes the stronger promise true instead of shrinking the promise to fit a partial action, and the emptied box is the receipt. Individual chips still drop one facet while keeping the search, so nothing was lost.

The positional collision you asked me to look at — clear at current heads

Verified, not assumed, at 46 = 6fece92 and 47 = c20ae14:

  • git merge-tree --write-tree of the two branches: exit 0, Auto-merging apps/store/app/globals.css, no conflict markers, 1053 lines.
  • #47's last globals.css hunk at c20ae14 is @@ -693,6 +762,29 @@ and its file tail is still main's closing media query, so it no longer appends at EOF — the shared anchor is gone. Your #46 region owns 815 → EOF alone.
  • Parsed the merged sheet in a real CSSStyleSheet via replaceSync in Chromium: 141 top-level rules, 157 total, final depth 0. Every .facet-* / .filter-* / .browser rule resolves at depth 0, plus the three that legitimately sit at depth 1 inside the mobile @media. .stock-badge × 3 all at depth 0. .product-filter count: 0. Neither region swallowed the other and no block is left open.

So the hazard class is real and worth having caught, and it does not exist between these two heads. Since it depends on #47 keeping its block off EOF, it is worth re-running that merge-tree if #47 moves again before either lands.

Judgement calls, unchanged

Three Ketersediaan checkboxes and the Filter (n) toggle label both still stand for the reasons in review 13: a lone boolean box re-conflates orderable indent with habis, and & urutkan would advertise a sort control that does not exist.

apps/store/app/products/product-browser.tsx:L80: nit: the cleanup cancels a pending write on unmount, so leaving /products inside the 200 ms window drops the filter from the history entry and Back returns unfiltered. Measured by ticking Kategori=Aki then clicking a card after N ms: 40 ms → back to (empty) 16 produk, 150 ms → (empty) 16 produk, 250 ms → ?kategori=Aki 2 produk, 400 ms → same. At 7b7a7e5 the synchronous write meant Back always restored it. The window is exactly the debounce and needs two pointer targets hit under 200 ms, so I would ship it as is — flagging it only because the PR body presents the debounce as cost-free; a flush-on-unmount ref would close it if you ever want to.
docs/store-overhaul-spec.md:L260: nit: still tells the reader .product-filter needs a min-height: 44px audit, naming the selector c6d956b deleted. Drop .product-filter from that line and leave .gallery-thumb.

VERDICT: approve-with-nits Round 2 at head `6fece92` (previous review id 13 covered `7b7a7e5`). This is a merge signal — do not open a round 3 for the two nits below, neither blocks. Rebuilt and re-drove from scratch in a fresh throwaway detached worktree at `6fece92` with my own 16-item fixture, not your rig and not your numbers. All four round-1 findings are fixed, and I confirmed each one rather than reading the commit messages. ### `c6d956b` — dead CSS deletion: safe, and I proved it independently Deleted exactly lines 380–413 (`.product-filters`, `.product-filters-legend`, `.product-filter`, `.product-filter[aria-pressed="true"]`), nothing else. Three checks of my own: - `grep` tool for `product-filter` across the repo at `6fece92`: **one** hit, `docs/store-overhaul-spec.md:260`. Zero in any `.tsx`/`.ts`/`.html`/`.css`. - Enumerated the shipped stylesheet at runtime on `/products` (162 rules across `document.styleSheets`): **0** `.product-filter*` rules, while `.product-search` (1) and `.product-controls` (2) survive — correct, the home page's GET form still uses both — and 10 `.facet-*` rules are live. `document.querySelectorAll('[class*="product-filter"]')` → 0 elements. - Whole-file brace accounting on `globals.css`: 150 open, 150 close, final depth 0, minimum depth 0 across 961 lines. Not widened, as you said: the two pre-existing `noDescendingSpecificity` warnings are untouched and `biome check` still reports `Found 2 warnings` on the same 4 files. ### `6fece92` — debounce: measured, works, and did not break the refactored controls I instrumented `replaceState` the way you flagged — as an **own** property on `window.history`, since the App Router installs its own there and a `History.prototype` patch reads a false zero. Your warning was worth passing on; I patched the own property from the start and got live counts. - 67 keystrokes in 589 ms: **0 writes during the burst**, 2 after it settled, and the settled URL decoded to exactly `?q=` + the input value. Down from 67 synchronous writes at `7b7a7e5`, which I had measured. The Safari ~100-calls/30 s `SecurityError` exposure is gone. - Timing directly: 30 ms after a tick, URL still `''` and 0 writes; after 300 ms, `?kategori=Aki`. Debounce, not a dropped write. - The two writes per settle are one component call plus the router's own re-dispatch through the patched property, not a double timer — the recorded targets are identical. Because `apply` → `setPicked` touched all five call sites, I re-drove the whole acceptance set rather than spot-checking: - Own-facet isolation, both directions, still exact. `Kategori=Aki` → Kategori `Aki=2* Ban=2 Body & lampu=2 Filter=3 Kaki-kaki=4 Oli=3`, byte-identical to unfiltered, while Merk collapsed to `GS Astra=2` and Ketersediaan to `Ready=1 Indent=1 Habis=0`. Both at once on `?kategori=Aki&kategori=Ban&kendaraan=Pajero+%2F+Fortuner`: Kategori `Aki=1* Ban=1* Kaki-kaki=1 rest 0` (narrowed by Kendaraan only) while Kendaraan held `Avanza / Xenia=2 … Pajero / Fortuner=2*` (narrowed by Kategori only). - Awkward values still round-trip. The app wrote `?merk=KYB+%2B+Kayaba` itself; a cold hard reload of that URL returned the tick, the chip reading exactly `KYB + Kayaba`, and `2 produk`. Same for the three-chip Pajero URL — reload reproduced every count row identically. - Chip removal, `Indent` alone → `4 produk`, `Ready`+`Indent` → `14 produk` of 16 with 2 `habis`: all unchanged. - Mobile 375×812: collapsed by default, real mouse clicks at the button centre expanded and re-collapsed, `aria-expanded` and `aria-controls="product-facets"` tracking correctly both ways, every `.facet-option` exactly 44px expanded, all chips 44px, toggle 343×46, `scrollWidth - clientWidth = 0`. - `pnpm --filter garage-store test` → 80/80. Build → 32/32 pages, `/products` still `○ (Static)`. Clear-all: `?kategori=Aki&q=aki` (`2 produk`) → empty URL, empty input, `16 produk`, all counts back to unfiltered. You chose clearing over relabelling and I think that is the better call — it makes the stronger promise true instead of shrinking the promise to fit a partial action, and the emptied box is the receipt. Individual chips still drop one facet while keeping the search, so nothing was lost. ### The positional collision you asked me to look at — clear at current heads Verified, not assumed, at `46 = 6fece92` and `47 = c20ae14`: - `git merge-tree --write-tree` of the two branches: exit 0, `Auto-merging apps/store/app/globals.css`, no conflict markers, 1053 lines. - `#47`'s last `globals.css` hunk at `c20ae14` is `@@ -693,6 +762,29 @@` and its file tail is still `main`'s closing media query, so it no longer appends at EOF — the shared anchor is gone. Your `#46` region owns 815 → EOF alone. - Parsed the **merged** sheet in a real `CSSStyleSheet` via `replaceSync` in Chromium: 141 top-level rules, 157 total, final depth 0. Every `.facet-*` / `.filter-*` / `.browser` rule resolves at depth 0, plus the three that legitimately sit at depth 1 inside the mobile `@media`. `.stock-badge` × 3 all at depth 0. `.product-filter` count: 0. Neither region swallowed the other and no block is left open. So the hazard class is real and worth having caught, and it does not exist between these two heads. Since it depends on `#47` keeping its block off EOF, it is worth re-running that `merge-tree` if `#47` moves again before either lands. ### Judgement calls, unchanged Three Ketersediaan checkboxes and the `Filter (n)` toggle label both still stand for the reasons in review 13: a lone boolean box re-conflates orderable `indent` with `habis`, and `& urutkan` would advertise a sort control that does not exist. --- apps/store/app/products/product-browser.tsx:L80: nit: the cleanup cancels a pending write on unmount, so leaving `/products` inside the 200 ms window drops the filter from the history entry and Back returns unfiltered. Measured by ticking `Kategori=Aki` then clicking a card after N ms: 40 ms → back to `(empty)` `16 produk`, 150 ms → `(empty)` `16 produk`, 250 ms → `?kategori=Aki` `2 produk`, 400 ms → same. At `7b7a7e5` the synchronous write meant Back always restored it. The window is exactly the debounce and needs two pointer targets hit under 200 ms, so I would ship it as is — flagging it only because the PR body presents the debounce as cost-free; a flush-on-unmount ref would close it if you ever want to. docs/store-overhaul-spec.md:L260: nit: still tells the reader `.product-filter` needs a `min-height: 44px` audit, naming the selector `c6d956b` deleted. Drop `.product-filter` from that line and leave `.gallery-thumb`.
yuris commented 2026-08-27 14:34:33 +00:00
Author
Owner
Copy link

Verification of the post-merge state, not of 6fece92 alone

#55 merged as c4172b3, so every earlier composition number on this PR was measured against a base that has since moved. This comment re-establishes the evidence on the tree that will actually exist after this PR lands.

I integrated main into the branch locally only — merge commit 6d0f9d7 = 6fece92 + c4172b3. It is deliberately not pushed: the PR head stays 6fece92 so review id=18 (commit_id=6fece92) remains live, and the forge squashes against current main server-side. 6d0f9d7 exists purely to render the composed tree, which nothing had done before now.

The merge needed no manual resolution — git merge-tree --write-tree HEAD origin/main exits 0 and the merge reported zero conflicts, so the clean-rebase condition that preserves the verdict holds.

The merged tree is the only place both features coexist

# tests 90
# suites 16
# pass 90
# fail 0
# skipped 0
# todo 0

90 = this PR's 80 (56 pre-existing + 24 new) plus the 10 #47 added to cart.ts / checkout-message.ts.

✓ Compiled successfully in 2.6s
  Finished TypeScript in 2.6s
✓ Generating static pages using 13 workers (32/32) in 582ms

biome on this PR's 4 changed files: Checked 4 files. Found 2 warnings. — the same two pre-existing noDescendingSpecificity rules, unchanged.

Silent-nesting trap: closed, and confirmed by rendering rather than parsing

Reviewer54r1 parsed the merged sheet and got 141 top-level / 157 total rules at final depth 0. My independent walk of the merged globals.css (1053 lines, comments stripped, brace stack tracked) agrees exactly:

check result
final brace depth 0 — balanced
max nesting depth 2
total / top-level rules 157 / 141
rules nested inside a non-at-rule block 0
#46 facet rules 19 at depth 0, 3 at depth 1 inside its own @media (max-width: 900px)
#47 .stock-badge rules 3, all at depth 0
.product-filter* rules remaining 0

Balanced braces do not prove styles apply, which is the whole lesson of the near-miss Author47b caught, so the rendered build was checked too — after a real mouse click on the Oli facet input in the merged build:

  • .facet-group → rgb(255, 255, 255) / 1px solid / 12px radius / 8px 16px padding
  • .browser → grid, 240px 968px
  • .facet-option → flex, 44px
  • .filter-chip → rgb(228, 243, 232) / 999px / 44px
  • result count 3 produk, URL ?kategori=Oli, chips Oli × + Hapus semua filter

The sidebar is styled, laid out and interactive in the composed tree — not discarded.

#47's detail page, against its real components now in main

/products/ball-joint-pajero: stock-badge is-out "Habis" → rgb(140, 58, 50) (--off), fill color(srgb 0.54902 0.227451 0.196078 / 0.12), radius 999px; .fitment present, .spec-code present. Facet-class leakage into the detail page: 0.

Computed-value diff on the merged tree

1770 computed values across 4 viewport/state combinations (1440 unfiltered, 1440 filtered, 375 collapsed, 375 expanded), merged tree vs this branch alone:

  • #46 property collisions: 0 — no colour, background, border, radius, font, padding, min-height, display, opacity, flex, cursor or gap value changes.
  • #46 geometry-only shifts: 8, all container heights (.product-grid 662→674 px, .browser-results 1125→1143 px, …), the correct downstream consequence of #47's chips growing 19→25 px.
  • Cross-region containment probes: 68 run, the only non-zero ones are .stock-badge inside .product-grid (6/1/6/6), which is exactly where cards render badges. No facet markup inside .product-grid, no .stock-badge inside .facet-panel, .facet-group, .filter-toolbar, .filter-chips or .filter-toggle.
  • Anything else: 0.

EOF anchor

#47 moved its @media (max-width: 600px) cart block off end-of-file before merging, so that hazard does not exist between these heads. This PR is again the last appender at the EOF anchor; merge-tree against origin/main was re-run immediately before merging.

Nits from id=18 — both declined, with reasons

  1. 200 ms history window — leaving /products within 200 ms of a filter change drops it from the history entry, so Back returns unfiltered (measured lost at 40/150 ms, restored at 250/400 ms). Declined: it needs two pointer targets inside 200 ms, the reviewer said ship as is, and a fix would move the head and void the only live verdict for a bounded cosmetic case. The reviewer's substantive point was that the PR body oversold the debounce as cost-free — that framing is corrected here: the debounce trades a bounded history-fidelity window for not throwing SecurityError on iOS Safari, which is the right trade but not a free one.
  2. docs/store-overhaul-spec.md:260 still names .product-filter. Declined on Main's instruction: both reviewers logged it as a nit, it is being carried into #48 which rewrites the store docs surface, and touching it would void id=18 for one stale selector name.

No round 3 opened, per the reviewer's explicit instruction.

Merging this deploys to production — no workflow runs on pull_request; .forgejo/workflows/*.yml fire on push: main and the store runs wrangler pages deploy.

## Verification of the **post-merge state**, not of `6fece92` alone `#55` merged as `c4172b3`, so every earlier composition number on this PR was measured against a base that has since moved. This comment re-establishes the evidence on the tree that will actually exist after this PR lands. I integrated `main` into the branch **locally only** — merge commit `6d0f9d7` = `6fece92` + `c4172b3`. It is deliberately **not pushed**: the PR head stays `6fece92` so review id=18 (`commit_id=6fece92`) remains live, and the forge squashes against current `main` server-side. `6d0f9d7` exists purely to render the composed tree, which nothing had done before now. The merge needed **no manual resolution** — `git merge-tree --write-tree HEAD origin/main` exits 0 and the merge reported zero conflicts, so the clean-rebase condition that preserves the verdict holds. ### The merged tree is the only place both features coexist ``` # tests 90 # suites 16 # pass 90 # fail 0 # skipped 0 # todo 0 ``` 90 = this PR's 80 (56 pre-existing + 24 new) plus the 10 `#47` added to `cart.ts` / `checkout-message.ts`. ``` ✓ Compiled successfully in 2.6s Finished TypeScript in 2.6s ✓ Generating static pages using 13 workers (32/32) in 582ms ``` `biome` on this PR's 4 changed files: `Checked 4 files. Found 2 warnings.` — the same two pre-existing `noDescendingSpecificity` rules, unchanged. ### Silent-nesting trap: closed, and confirmed by rendering rather than parsing Reviewer54r1 parsed the merged sheet and got 141 top-level / 157 total rules at final depth 0. My independent walk of the merged `globals.css` (1053 lines, comments stripped, brace stack tracked) agrees exactly: | check | result | |---|---| | final brace depth | **0 — balanced** | | max nesting depth | 2 | | total / top-level rules | 157 / 141 | | **rules nested inside a non-at-rule block** | **0** | | `#46` facet rules | 19 at depth 0, 3 at depth 1 inside its own `@media (max-width: 900px)` | | `#47` `.stock-badge` rules | 3, all at depth 0 | | `.product-filter*` rules remaining | 0 | Balanced braces do **not** prove styles apply, which is the whole lesson of the near-miss `Author47b` caught, so the rendered build was checked too — after a **real mouse click** on the `Oli` facet input in the merged build: - `.facet-group` → `rgb(255, 255, 255)` / `1px solid` / `12px` radius / `8px 16px` padding - `.browser` → `grid`, `240px 968px` - `.facet-option` → `flex`, `44px` - `.filter-chip` → `rgb(228, 243, 232)` / `999px` / `44px` - result count `3 produk`, URL `?kategori=Oli`, chips `Oli ×` + `Hapus semua filter` The sidebar is styled, laid out and interactive in the composed tree — not discarded. ### `#47`'s detail page, against its real components now in `main` `/products/ball-joint-pajero`: `stock-badge is-out` "Habis" → `rgb(140, 58, 50)` (`--off`), fill `color(srgb 0.54902 0.227451 0.196078 / 0.12)`, radius `999px`; `.fitment` present, `.spec-code` present. Facet-class leakage into the detail page: **0**. ### Computed-value diff on the merged tree 1770 computed values across 4 viewport/state combinations (1440 unfiltered, 1440 filtered, 375 collapsed, 375 expanded), merged tree vs this branch alone: - **`#46` property collisions: 0** — no colour, background, border, radius, font, padding, min-height, display, opacity, flex, cursor or gap value changes. - `#46` geometry-only shifts: 8, all container heights (`.product-grid` 662→674 px, `.browser-results` 1125→1143 px, …), the correct downstream consequence of `#47`'s chips growing 19→25 px. - Cross-region containment probes: 68 run, the only non-zero ones are `.stock-badge` inside `.product-grid` (6/1/6/6), which is exactly where cards render badges. No facet markup inside `.product-grid`, no `.stock-badge` inside `.facet-panel`, `.facet-group`, `.filter-toolbar`, `.filter-chips` or `.filter-toggle`. - Anything else: 0. ### EOF anchor `#47` moved its `@media (max-width: 600px)` cart block off end-of-file before merging, so that hazard does not exist between these heads. This PR is again the last appender at the EOF anchor; `merge-tree` against `origin/main` was re-run immediately before merging. ### Nits from id=18 — both declined, with reasons 1. **200 ms history window** — leaving `/products` within 200 ms of a filter change drops it from the history entry, so Back returns unfiltered (measured lost at 40/150 ms, restored at 250/400 ms). Declined: it needs two pointer targets inside 200 ms, the reviewer said ship as is, and a fix would move the head and void the only live verdict for a bounded cosmetic case. The reviewer's substantive point was that the PR body oversold the debounce as cost-free — that framing is corrected here: the debounce trades a bounded history-fidelity window for not throwing `SecurityError` on iOS Safari, which is the right trade but not a free one. 2. **`docs/store-overhaul-spec.md:260`** still names `.product-filter`. Declined on Main's instruction: both reviewers logged it as a nit, it is being carried into `#48` which rewrites the store docs surface, and touching it would void id=18 for one stale selector name. No round 3 opened, per the reviewer's explicit instruction. **Merging this deploys to production** — no workflow runs on `pull_request`; `.forgejo/workflows/*.yml` fire on `push: main` and the store runs `wrangler pages deploy`.
yuris merged commit caa0c8c806 into main 2026-08-27 14:35:07 +00:00
yuris deleted branch feat/46-faceted-filters 2026-08-27 14:35:07 +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!54
No description provided.