marketplaces/docs/architecture/foundation/Seller-Management-Final-Design-Review.md

# Seller Management — Final Design Review

Principal-architect-level review of everything built and documented for
Seller Management so far. One real bug found and fixed (below, in scope per
this mission's "unless absolutely required" carve-out); everything else is
findings only, no further code changed. Reviewed against source directly
(fresh `tsc --noEmit` and `arch:check` run for this review, not recalled
from memory) plus every doc in the series:
[Seller-Management.md](Seller-Management.md),
[ADR-011](adr/ADR-011-optional-seller-management-module.md),
[Seller-Management-Diagrams.md](Seller-Management-Diagrams.md),
[Seller-Management-Domain-Models.md](Seller-Management-Domain-Models.md),
[Seller-Management-UX-Review.md](Seller-Management-UX-Review.md),
[Seller-Management-Backoffice-Readiness-Audit.md](Seller-Management-Backoffice-Readiness-Audit.md),
[Seller-Management-Storefront-Audit.md](Seller-Management-Storefront-Audit.md),
[Seller-Management-Backend-Migration-Plan.md](Seller-Management-Backend-Migration-Plan.md),
and `BACKEND.md` §11.

## Checklist verification (evidence-based, not asserted)

| Item | Status | Evidence |
|---|---|---|
| No existing marketplace breaks | ✓ Verified | Fresh `tsc --noEmit` clean, fresh `arch:check` clean (this review); live browser tests at 3 separate checkpoints across storefront home + backoffice dashboard/route |
| Feature is optional | ✓ Verified | `DEFAULT_PLATFORM_MODULES_CONFIG.sellerManagement.enabled = false`; no backend anywhere sets it true |
| Bootstrap remains backward compatible | ✓ Verified | `modules?`/`seller?` both optional on `BootstrapConfig`; `tsc` stayed clean the moment they were added, no consumer touched |
| No API breaking changes | ✓ Verified (trivially) | No API exists to break — documentation-only for the backend side |
| Existing frontend continues working | ✓ Verified | Live-tested at every UI-touching commit, zero regressions found |
| Existing backend continues working | N/A | No backend exists; not applicable until implementation begins |
| Dependency direction (ADR-002) | ✓ Verified | `arch:check:boundaries` clean, run fresh for this review |
| Import boundaries (ADR-003) | ✓ Verified | Same tool, same clean result |
| No tenant-specific conditions | ✓ Verified | Reviewed every new file directly — zero `if (tenant...)`/`if (seller===...)` conditionals exist anywhere |
| Seller scope is additive | ✓ Verified | Every new field on every touched type is optional; nothing required changed |
| UI consistency | ✓ Verified, with 2 fixes already applied | Dedicated UX-review pass found and fixed a label/a11y gap and a native-bullet inconsistency |
| Translation readiness | ✓ Verified | en/ru/hy all carry every new key, confirmed by exact-count grep |
| Accessibility readiness | ✓ Verified structurally, **one caveat** | Confirmed via accessibility-tree inspection (`role=dialog`, `aria-modal`, focus trap, `aria-label`) — **no real screen-reader software (NVDA/VoiceOver) pass was ever done**, only automated tree inspection. Flagged below (Low). |
| Performance considerations | ✓ Verified | New route is its own lazy chunk (confirmed in build output), doesn't touch the initial bundle |
| Future scalability | ✓ Addressed at design level | `Seller-Management-Backend-Migration-Plan.md` covers indexes, caching, phased rollout |

## Findings

### 1. `SellerConfig` vs. `Seller`/`SellerBranding` — two unreconciled type hierarchies — **Medium**

`shared/models/config/seller.model.ts` (`SellerConfig`, the bootstrap wire
shape: `id, marketplaceId, slug, name, defaultLocale, supportedLocales`) and
`core/sellers/models/seller.model.ts` (`Seller`, the domain entity:
`id, marketplace: MarketplaceRef, name, slug, status, branding?,
createdAt, updatedAt`) describe overlapping concepts with different shapes
and no conversion function between them. This was **self-identified during
this same body of work** (`BACKEND.md` §11.6 already flags it as an open
question) — restating it here as an independently-confirmed architectural
finding, not a new discovery, because a final design review should not
let a self-flagged gap quietly become "someone else's problem later."
**Recommendation:** resolve before real backend work starts — either
`SellerConfig` becomes a strict projection of `Seller` (documented mapping),
or they're merged into one type with bootstrap-specific fields marked
optional. Either is fine; leaving it unreconciled through implementation
risks two competing "seller" shapes drifting further apart.

### 2. No reusable capability-guard abstraction exists — **Medium**

ADR-011 (and ADR-009 before it) both prescribe checking a capability flag
"in one place, not scattered conditionals." In practice, **no such
reusable guard exists anywhere in this codebase** — not for
`sellerManagement.enabled`, and not for any existing feature flag either.
ADR-009 itself describes a `FeatureFlagService` that was never actually
built (confirmed: no file of that name exists in `src/app/core`). The one
current consumer (`AdminSellerManagementPageComponent`) hand-rolls the
optional-chain read directly. With one consumer this is harmless; the
moment a second consumer needs the same check, it will either duplicate
the same expression (drift risk: `?? false` vs `=== true` vs missing a
null-check) or someone will need to build the guard ADR-009 already
promised. **Recommendation:** build one small `SellerManagementGuardService`
(or equivalent) the first time a second consumer needs the flag — don't
let a third or fourth hand-rolled copy accumulate first.

### 3. Reactive-signal bug in the Phase 1 page — **Fixed during this review**

`AdminSellerManagementPageComponent.sellerManagementEnabled` read
`configService.getBootstrapSnapshot()` once via a plain `signal()` at
construction time — not reactively tied to `configService.bootstrapRevision()`
the way `UiRuntimeFacade` and `SeoService` both correctly do. If bootstrap
ever reloaded after initial page load (tenant context switch, revalidation)
with the flag now `true`, this signal would never update — a real
staleness bug, currently invisible because the flag is always `false` and
the signal was never even read in the template. **Fixed in this review**
(changed to `computed()` keyed on `bootstrapRevision()`, matching the
established codebase pattern exactly) — a one-line correctness fix to
already-committed code, not new feature work, so it fell inside this
mission's "unless absolutely required" carve-out. Verified `tsc --noEmit`
clean after the change.

### 4. `sellerId` typed as bare `string`, not `UUID` — **Low / Nice to have**

`Item.sellerId?`, `AdminProduct.sellerId?`, `AdminOrder.sellerId?` are all
typed `string`, while every ID in the new `core/sellers/models/` uses the
`UUID` type alias (`type UUID = string` — functionally identical, purely a
signaling convention used consistently elsewhere in this codebase, e.g.
`TenantConfig.id: UUID`). Zero functional impact since `UUID` is a bare
alias, but a future reader will reasonably wonder why the new sellerId
fields didn't follow the convention the sellers domain itself established
one file away. **Recommendation:** trivial fix, do it opportunistically
next time any of these three files is touched — not worth a dedicated pass.

### 5. `MarketplaceRef` vs. `TenantConfig` — acceptable but worth flagging — **Low**

`MarketplaceRef {id, slug, name}` and the existing `TenantConfig` (id, slug,
code, host, name, locales, currencies, timezone, base URLs) both represent
"a marketplace," from two different vantage points (seller-record reference
vs. full runtime tenant contract). This is a deliberate, documented
distinction (`Seller-Management-Domain-Models.md` explains it), not an
accidental duplication — but it's the kind of decision that reads clearly
today and could easily read as "why are there two Marketplace types" to
someone joining later without the context. **Recommendation:** no action
needed now; if a third marketplace-shaped type is ever proposed, that's the
signal to consolidate, not before.

### 6. The flag's "true" branch has never been exercised, even manually — **Medium**

Every verification claim in this document's checklist table (and every
prior audit) was tested with `modules.sellerManagement.enabled` at its
real-world value: `false` (or absent). **Nobody has ever manually set it to
`true`** — not in a browser dev-tools override, not in a mock fixture — to
confirm the flag-reading code path actually behaves as intended when the
condition it exists to detect is met. Today that's low-stakes (there's no
enabled-state UI to differ), but the review checklist item "Seller scope is
additive" was verified by reading the code, not by observing the `true`
branch execute. **Recommendation:** the first time any enabled-state UI is
built, that's also the moment to add one manual (or fixture-based) test
confirming the `true` path — don't let a second feature get built on top of
an assumption that was never actually observed.

### 7. Documentation-to-code ratio is unusually high — **Low / Nice to have**

Eight documents (this one included) exist for a capability that has zero
backend bytes and one placeholder frontend page. That's not inherently
wrong — the mission explicitly asked for staged documentation-first work —
but it carries two real risks worth naming: (a) maintenance burden keeping
eight cross-linked documents consistent if any single decision changes
(e.g., if Unified-vs-Split-Orders resolves one way, at least three of these
docs reference it and would need a coordinated update), and (b) the more
times an undecided item ("Future," "not designed") is repeated across
documents, the more it can start to feel settled by sheer repetition even
though nothing has actually been decided. **Recommendation:** before
backend implementation begins, do one consolidation pass collapsing
overlapping content (the Unified/Split-Orders question in particular
appears in `Seller-Management.md`, the Storefront audit, `BACKEND.md` §11,
and the Migration Plan) into a single canonical statement the others link
to, rather than four independent restatements.

### 8. No automated test coverage — **Low / Nice to have, not new**

Zero unit or integration tests cover any file introduced in this work —
consistent with the rest of the codebase (`PROJECT_STATUS.md` already
documents "no automated test suite exists," a pre-existing, repo-wide gap,
not something this work introduced or made worse). Noting it here for
completeness, not as a Seller-Management-specific defect.

## What I did NOT find

No architectural weaknesses beyond the above. Specifically checked for and
did **not** find: circular dependencies (verified fresh, clean), scattered
tenant/seller conditionals (none exist anywhere), over-engineering relative
to the "typed models only" mandate (the six new model files map 1:1 to the
six concepts explicitly requested, nothing extra), hidden coupling between
`core/sellers/models` and any `features/` folder (the new domain models
import only from `shared/types`, nothing reaches into a feature module),
or any security-sensitive code path touched (no auth, no payment code was
modified anywhere in this entire body of work).

## Verdict

**Not an unqualified "ready for implementation."** Two Medium findings
(#1, the unreconciled `SellerConfig`/`Seller` type split, and #2, the
missing capability-guard abstraction) are genuine architectural loose ends
that should be resolved by decision or by a small build, respectively,
before real backend/CRUD work begins — not because either blocks anything
today, but because both compound in cost the longer they're left
unresolved (more consumers = more places to reconcile later; the type
duality especially, since a real `SellerRepository`/`SellerGateway` would
otherwise have to pick one shape or invent a mapping ad hoc under time
pressure). Finding #6 (the flag's true-branch never observed) is a
process gap to close at the next milestone, not before it.

**Everything that has actually been built — the typed foundation, the
disabled-by-default feature flag, the Phase 1 UI, and the one real bug
this review found and fixed — is solid and ready to stay exactly as it
is.** The design as a *whole plan* is sound and internally consistent; the
two Medium findings are refinements to make before the next phase starts,
not defects in what exists today. No Critical or High-severity issue was
found anywhere in this review.