Wip stage M4
This commit is contained in:
Nông Đức Huy
@@ -0,0 +1,71 @@
|
||||
# ADR-0016: Option values are retained when variants reference them
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-08-12
|
||||
|
||||
## Context
|
||||
|
||||
A merchandiser removes the "Red" colourway from a product. What should happen to
|
||||
the three Red variants and to the Red option value itself?
|
||||
|
||||
Three things reference them and each has a different claim:
|
||||
|
||||
- **Order history** — an order line points at a variant id, and reading that
|
||||
order later needs "Red / M" to still mean something.
|
||||
- **The variant matrix** — Red must stop generating combinations.
|
||||
- **The storefront** — a shopper must not see a Red swatch that can never be
|
||||
selected.
|
||||
|
||||
The database already takes a position: `ProductVariantOptionValue.optionValue`
|
||||
is `onDelete: Restrict`, precisely so that deleting a value cannot silently
|
||||
orphan or cascade into history.
|
||||
|
||||
## Decision
|
||||
|
||||
**Variants are archived, never deleted.** A removed combination sets
|
||||
`status = ARCHIVED`. The row, its SKU and its option links all survive, so an
|
||||
order placed last month still resolves.
|
||||
|
||||
**Option values are deleted only when nothing references them.** If any variant
|
||||
— active or archived — still links to a value, the value is _retained_. It stops
|
||||
appearing in the matrix and stops being offered, but it continues to exist so the
|
||||
archived variants remain readable.
|
||||
|
||||
**The matrix is built from the operator's submitted option set, not from the
|
||||
database.** Retained values would otherwise regenerate the very variants that
|
||||
were just archived.
|
||||
|
||||
**The storefront filters option values to those offered by at least one active
|
||||
variant.** Without this last step, a retained value renders as a permanently
|
||||
disabled swatch with no explanation.
|
||||
|
||||
## Consequences
|
||||
|
||||
Order history stays intact and readable, which is the constraint that drove
|
||||
everything else. Re-adding a removed colourway later reuses the retained value
|
||||
and its translations rather than creating a duplicate.
|
||||
|
||||
The cost is that `product_option_values` accumulates rows that are invisible to
|
||||
shoppers, and "delete" is not always literally a delete — a merchandiser who
|
||||
inspects the database will find values they thought they removed. The schema
|
||||
comment and this ADR are the mitigation; a future admin screen could surface
|
||||
retained values explicitly.
|
||||
|
||||
This was found the hard way: the first implementation deleted values before
|
||||
archiving variants and hit the `Restrict` constraint as a 500. The failure was
|
||||
the schema doing its job.
|
||||
|
||||
## Alternatives considered
|
||||
|
||||
**Cascade the delete.** Removes the option value and its variant links, which
|
||||
either breaks order lines or cascades into them. Rejected — this is the outcome
|
||||
`Restrict` exists to prevent.
|
||||
|
||||
**Soft-delete option values with an `isActive` column.** Functionally similar to
|
||||
retention, but adds a column and a filter to every catalog query for a case that
|
||||
is already handled by "is any active variant offering this?". Rejected as
|
||||
redundant state.
|
||||
|
||||
**Refuse to remove a value that has variants.** Simple and safe, but forces the
|
||||
merchandiser to archive six variants by hand before they can drop a colourway —
|
||||
pushing bookkeeping onto the person the tool exists to help.
|
||||
@@ -0,0 +1,90 @@
|
||||
# ADR-0017: shadcn/ui for infrastructure, hand-built for brand
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-08-12
|
||||
|
||||
## Context
|
||||
|
||||
The component layer started as four hand-rolled primitives in `@sport/ui`
|
||||
(Button, Input, Badge, Skeleton). That was enough while the surface was static
|
||||
text and links. It stopped being enough the moment real interaction arrived: the
|
||||
mobile filter panel needs focus trapping and scroll locking, the product editor
|
||||
needs tabs, row actions need a menu, and the media picker needs a modal.
|
||||
|
||||
Writing those by hand means writing focus management, `aria-modal` wiring,
|
||||
escape handling, portal placement and collision detection. That work is
|
||||
well-understood, easy to get subtly wrong, and worth nothing to this store's
|
||||
customers — nobody chooses a running jacket because the dialog traps focus
|
||||
correctly. They only notice when it doesn't.
|
||||
|
||||
The opposite is true of the product card, the header, the hero and the PDP.
|
||||
Those _are_ the store. A generic card component would make them look like every
|
||||
other template on the internet, which is precisely the outcome the visual
|
||||
direction exists to avoid.
|
||||
|
||||
## Decision
|
||||
|
||||
**Two tiers, split on whether a component carries brand.**
|
||||
|
||||
`packages/ui/src/components/ui/` holds shadcn/ui components, owned as source in
|
||||
this repo and shared by the storefront and the admin: Button, Input, Dialog,
|
||||
Sheet, DropdownMenu, Tabs, plus Badge and Skeleton. Registry components are
|
||||
copied in essentially unedited so that anything else from shadcn drops in later.
|
||||
|
||||
`apps/storefront/src/components/commerce/` holds everything that carries the
|
||||
brand, built by hand: hero, mega menu, site header, product card, product
|
||||
gallery, product detail, filter sheet. These are storefront-only and are never
|
||||
promoted to `@sport/ui`, per the existing rule in architecture §4.
|
||||
|
||||
**A semantic token layer adapts shadcn to the palette.** shadcn writes against
|
||||
role tokens (`--background`, `--primary`, `--ring`, `--radius`); the store's
|
||||
palette is `ink`/`volt`. `packages/config/tailwind/theme.css` maps one onto the
|
||||
other, so a registry component already looks like this store before it is
|
||||
touched. Note that shadcn's `--accent` is a hover surface, not a brand accent —
|
||||
it maps to `ink-100`, and volt stays applied deliberately.
|
||||
|
||||
**Supporting libraries, each for one job.** Lucide for icons, Motion for the
|
||||
hero's entrance, Embla for the PDP gallery's swipe.
|
||||
|
||||
## Consequences
|
||||
|
||||
The dialog/sheet/menu behaviour we would otherwise have written badly is now
|
||||
correct and not our problem. `asChild` removed a real defect class: `<Link>`
|
||||
wrapping `<Button>` was producing `<a><button>`, which is invalid HTML and
|
||||
announces as two nested controls.
|
||||
|
||||
Registry components import `@/lib/utils` and `@/components/ui/*`. A workspace
|
||||
package cannot rely on an app's tsconfig aliases, so those imports are rewritten
|
||||
to relative paths on the way in. That is a one-line edit per component and is
|
||||
documented in the `@sport/ui` barrel.
|
||||
|
||||
Two registry defaults had to be overridden, both because they assume a light
|
||||
page: `outline` shipped with `bg-background`, which rendered a white button with
|
||||
white text on the black hero, and the base sizes are rounded and lowercase where
|
||||
this brand is square and uppercase. Owning the source is what made those
|
||||
one-line fixes rather than a fight.
|
||||
|
||||
The cost is a larger dependency surface — `radix-ui`, `lucide-react`, `motion`,
|
||||
`embla-carousel-react`, `tw-animate-css` — and a standing obligation to keep
|
||||
copied components in step with upstream by choice rather than by `npm update`.
|
||||
|
||||
## Alternatives considered
|
||||
|
||||
**Keep hand-rolling everything.** Total control, and the existing primitives were
|
||||
good. Rejected once the list of things to build became "focus trap, scroll lock,
|
||||
portal, collision detection" — that is a component library, and writing one is
|
||||
not this project.
|
||||
|
||||
**Adopt a full component library (MUI, Mantine, Chakra).** Faster to start and
|
||||
far harder to make look like anything but itself. The visual direction here is
|
||||
the deliverable, and fighting a theme system to reach it is worse than building
|
||||
the branded parts by hand.
|
||||
|
||||
**Use shadcn for everything, including the product card and header.** This is the
|
||||
common failure mode. It produces a store that works correctly and looks like a
|
||||
demo. The split above exists precisely to prevent it.
|
||||
|
||||
**Per-app copies of the shadcn layer instead of a shared package.** Matches
|
||||
shadcn's single-app default and lets the two apps drift freely. Rejected because
|
||||
Button and Input genuinely are identical in both, and two copies means two
|
||||
places to fix the next `bg-background` problem.
|
||||
@@ -24,6 +24,8 @@ An ADR is immutable once accepted. If a decision changes, add a new ADR that sup
|
||||
| [0013](./0013-content-translations-in-typed-tables-ui-strings-in-message-catalogs.md) | Content translations in typed tables, UI strings in message catalogs | Accepted |
|
||||
| [0014](./0014-denormalised-price-projection-on-product.md) | A denormalised price/stock projection on Product | Accepted |
|
||||
| [0015](./0015-frontends-reach-the-api-through-their-own-origin.md) | Frontends reach the API through their own origin | Accepted |
|
||||
| [0016](./0016-option-values-are-retained-when-variants-reference-them.md) | Option values are retained when variants reference them | Accepted |
|
||||
| [0017](./0017-shadcn-for-infrastructure-hand-built-for-brand.md) | shadcn/ui for infrastructure, hand-built for brand | Accepted |
|
||||
|
||||
## Decisions deliberately NOT recorded yet
|
||||
|
||||
|
||||
+60
-20
@@ -102,7 +102,9 @@ a framework, one of those consumers breaks.
|
||||
- Domain types and API contracts (`@sport/types`)
|
||||
- Input shape/format rules (`@sport/validation`)
|
||||
- API access (`@sport/api-client`)
|
||||
- Design-system primitives: Button, Input, Badge, Skeleton (`@sport/ui`)
|
||||
- UI infrastructure: Button, Input, Dialog, Sheet, DropdownMenu, Tabs, Badge,
|
||||
Skeleton (`@sport/ui`, shadcn/ui owned as source — see
|
||||
[ADR-0017](./adr/0017-shadcn-for-infrastructure-hand-built-for-brand.md))
|
||||
- Build configuration (`@sport/config`, `@sport/eslint-config`)
|
||||
|
||||
**Do not share** — things that only look shareable:
|
||||
@@ -110,7 +112,10 @@ a framework, one of those consumers breaks.
|
||||
- **Domain components.** `<ProductCard>` knows about sale badges, price ranges and colour
|
||||
swatches. It belongs to the storefront. The admin's product row needs status, stock and
|
||||
margin. Merging them produces a component with fourteen props and two consumers who both
|
||||
fight it.
|
||||
fight it. These live in `apps/storefront/src/components/commerce/` — hero, mega menu, site
|
||||
header, product card, product gallery, product detail, filter sheet — and are built by hand
|
||||
because they _are_ the brand. The dividing line: infrastructure comes from the registry,
|
||||
identity is written here.
|
||||
- **Business logic.** It lives in the API. A discount calculated in a shared package is a
|
||||
discount that can disagree with the invoice.
|
||||
- **App state.** Cart, auth session and filter state are app-specific. Shared stores create
|
||||
@@ -418,23 +423,27 @@ Places where the instinct to generalise should be resisted until a second real c
|
||||
|
||||
## 14. Architectural risks to prevent from day one
|
||||
|
||||
| Risk | Why it is fatal later | Prevention in place |
|
||||
| ------------------------------------------ | --------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------ |
|
||||
| Size/colour as product columns | Cannot express per-combination stock or price; requires re-modelling after orders exist | Variant model ([ADR-0003](./adr/0003-product-and-productvariant-as-separate-entities.md)) |
|
||||
| Float money | Silent discrepancies, unfixable retroactively | Integer minor units ([ADR-0011](./adr/0011-money-as-integer-minor-units.md)) |
|
||||
| Role checks scattered in code | Authorization becomes unauditable; new roles need deploys | RBAC + global guard ([ADR-0007](./adr/0007-rbac-permissions-instead-of-role-checks.md)) |
|
||||
| Admin querying the DB directly | A second write path where authorization is forgotten | No DB driver in admin ([ADR-0004](./adr/0004-the-admin-dashboard-has-no-database-access.md)) |
|
||||
| Module boundary erosion | The monolith becomes unsplittable and untestable | ESLint boundary rules + `public/` barrels |
|
||||
| Order lines joined to live catalog | Historical invoices change when prices do | Snapshot fields on order lines (milestone 5) |
|
||||
| Overselling under concurrency | Real money, real customers, real refunds | `reserved` column + transactional reservation |
|
||||
| Unversioned API | Cannot ship a breaking change once a mobile app exists | URI versioning from request one ([ADR-0005](./adr/0005-uri-based-api-versioning.md)) |
|
||||
| Binaries in PostgreSQL | Backups and replication degrade permanently | Object storage ([ADR-0009](./adr/0009-media-in-s3-compatible-storage-metadata-in-postgresql.md)) |
|
||||
| Single-warehouse inventory | Adding a location later means migrating live stock history | `(variant, location)` keys from the start |
|
||||
| Secrets in the repository | One leak compromises production | Validated env, generated dev secrets, `.env` gitignored |
|
||||
| No request correlation | Production incidents become guesswork | `x-request-id` end to end |
|
||||
| Browser-only failures passing every check | Server rendering and curl both succeed while every click fails | Client paths must be exercised in a real browser before a milestone closes |
|
||||
| `onUnauthorized` retrying the refresh call | Unbounded refresh loop hammering the API from the browser | `skipAuthRetry` on all auth endpoints plus a single-flight refresh |
|
||||
| Concurrent 401s each rotating the token | The second rotation reads as token reuse and revokes the family | One shared in-flight refresh promise |
|
||||
| Risk | Why it is fatal later | Prevention in place |
|
||||
| ---------------------------------------------- | --------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| Size/colour as product columns | Cannot express per-combination stock or price; requires re-modelling after orders exist | Variant model ([ADR-0003](./adr/0003-product-and-productvariant-as-separate-entities.md)) |
|
||||
| Float money | Silent discrepancies, unfixable retroactively | Integer minor units ([ADR-0011](./adr/0011-money-as-integer-minor-units.md)) |
|
||||
| Role checks scattered in code | Authorization becomes unauditable; new roles need deploys | RBAC + global guard ([ADR-0007](./adr/0007-rbac-permissions-instead-of-role-checks.md)) |
|
||||
| Admin querying the DB directly | A second write path where authorization is forgotten | No DB driver in admin ([ADR-0004](./adr/0004-the-admin-dashboard-has-no-database-access.md)) |
|
||||
| Module boundary erosion | The monolith becomes unsplittable and untestable | ESLint boundary rules + `public/` barrels |
|
||||
| Order lines joined to live catalog | Historical invoices change when prices do | Snapshot fields on order lines (milestone 5) |
|
||||
| Overselling under concurrency | Real money, real customers, real refunds | `reserved` column + transactional reservation |
|
||||
| Unversioned API | Cannot ship a breaking change once a mobile app exists | URI versioning from request one ([ADR-0005](./adr/0005-uri-based-api-versioning.md)) |
|
||||
| Binaries in PostgreSQL | Backups and replication degrade permanently | Object storage ([ADR-0009](./adr/0009-media-in-s3-compatible-storage-metadata-in-postgresql.md)) |
|
||||
| Single-warehouse inventory | Adding a location later means migrating live stock history | `(variant, location)` keys from the start |
|
||||
| Secrets in the repository | One leak compromises production | Validated env, generated dev secrets, `.env` gitignored |
|
||||
| No request correlation | Production incidents become guesswork | `x-request-id` end to end |
|
||||
| Browser-only failures passing every check | Server rendering and curl both succeed while every click fails | Client paths must be exercised in a real browser before a milestone closes |
|
||||
| `onUnauthorized` retrying the refresh call | Unbounded refresh loop hammering the API from the browser | `skipAuthRetry` on all auth endpoints plus a single-flight refresh |
|
||||
| Concurrent 401s each rotating the token | The second rotation reads as token reuse and revokes the family | One shared in-flight refresh promise |
|
||||
| Editing options wiping variant pricing | A merchandiser's price and stock work vanishes on a cosmetic edit | Order-independent combination signatures; `planVariantMatrix` is pure and tested ([ADR-0016](./adr/0016-option-values-are-retained-when-variants-reference-them.md)) |
|
||||
| Deleting an option value referenced by history | Breaks or cascades into past orders | `Restrict` FK plus retain-if-referenced ([ADR-0016](./adr/0016-option-values-are-retained-when-variants-reference-them.md)) |
|
||||
| A catalog write not invalidating the cache | Edits appear not to work, so operators repeat them | Every write path ends in `afterWrite` → `deleteByPrefix('catalog:')` |
|
||||
| Stock edited outside the ledger | "Why is this number wrong?" becomes unanswerable | Stock is not settable on the variant endpoint; it moves only through inventory |
|
||||
|
||||
---
|
||||
|
||||
@@ -456,19 +465,50 @@ interactions have been clicked in a real browser**, with the console and network
|
||||
Regression tests now cover the first two (`packages/api-client/src/http-client.spec.ts`); the
|
||||
third belongs in an end-to-end test when one exists.
|
||||
|
||||
Authoring the M3 editor added a second rule. Two defects survived a green pipeline and a
|
||||
successful-looking click-through, and both needed the _second_ step of a flow to appear:
|
||||
|
||||
- The variant grid saved correctly, showed "saved", and left every row marked dirty. It compared
|
||||
its draft against a `product` prop the editor shell never refreshed, so a save could not be seen
|
||||
by the thing measuring whether one was needed. One save looked fine; it was saving _twice_ that
|
||||
exposed it. The shell now owns the product and every tab hands its result back.
|
||||
- The inventory screen listed `StockLevel`, which is a projection of the ledger. A variant that has
|
||||
never moved has no row — so freshly created variants were invisible there, and since that screen
|
||||
is the only route to an adjustment, they could never be given stock at all. Every seeded product
|
||||
had stock already, so the whole catalog looked healthy. Only a product created from scratch
|
||||
showed it.
|
||||
|
||||
M4's listing controls added a third variant of the same lesson. Sorting a listing _after_
|
||||
loading more products showed the same product twice — once from the freshly sorted first page
|
||||
and once left over from the products appended under the previous order. `LoadMore` keeps its
|
||||
position in the React tree across a soft navigation, so its `useState` survived a query change
|
||||
the server had already acted on. Neither action alone revealed anything; only the pair did. It
|
||||
is keyed on the listing identity now.
|
||||
|
||||
The same pass caught a link built with a raw `<a href>` instead of the locale-aware `Link`, which
|
||||
sent an English shopper following "load more" to the Vietnamese listing — invisible in the
|
||||
default locale, which is exactly why it survived.
|
||||
|
||||
So: **exercise a flow on data you just created, not only on seeded data, perform each action
|
||||
twice, and do it in a non-default locale.** Seed data has been through every code path already; new data has been through none. The
|
||||
inventory case is pinned in `apps/api/src/modules/inventory/inventory-list.spec.ts`.
|
||||
|
||||
---
|
||||
|
||||
## 16. Deliberate limitations
|
||||
|
||||
Stated plainly so they are choices rather than oversights.
|
||||
|
||||
**Shipped (M0–M2)**
|
||||
**Shipped (M0–M3)**
|
||||
|
||||
- Catalog reads: products, variants, options, categories, collections, brands, navigation —
|
||||
localised, cached, filtered and faceted.
|
||||
- Storefront browsing and PDP in Vietnamese and English, with per-locale slugs and `hreflang`.
|
||||
- Auth: login, refresh rotation with reuse detection, audience separation, login throttling.
|
||||
- RBAC enforcement end to end, plus user administration and a role viewer in the admin.
|
||||
- Admin catalog authoring: product create/edit with per-locale content tabs, an option builder that
|
||||
regenerates the variant matrix, per-variant SKU and pricing, imagery assigned per colourway,
|
||||
media uploaded straight to storage, and stock received through the append-only ledger.
|
||||
|
||||
**Not built yet, and why**
|
||||
|
||||
|
||||
Reference in New Issue
Block a user