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, ADR-011, Seller-Management-Diagrams.md, Seller-Management-Domain-Models.md, Seller-Management-UX-Review.md, Seller-Management-Backoffice-Readiness-Audit.md, Seller-Management-Storefront-Audit.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.