deepdrft/product-notes/phase-9-solid-review.md

# Phase 9 SOLID Review — Release Medium Types

Status: design-quality audit of `product-notes/phase-9-release-medium-types.md` + `PLAN.md §9`.
Author: product-designer. Date: 2026-06-12. **Review only — no spec or code edits made.**

Scope: SOLID discipline as this project frames it — Open/Closed at the schema level, SRP at the
HTTP boundary, one mapping table per concern, DRY-by-composition, design-the-seam-up-front.
Findings are grouped by the principle at stake. Each carries a verdict: **accept**, **tighten**,
or **redesign**. Facts marked *(verified)* were read against live source on 2026-06-12.

---

## Findings — Single Responsibility / ownership

### F1 — Mix waveform compute is placed in the wrong tier `[CRITICAL — redesign]`

**Spec says (§3.4, §4, PLAN 9.2.B):** the CMS triggers high-res waveform preprocessing and uploads
the resulting datum: *"upload audio → trigger waveform preprocessing → `POST api/release/mix/waveform`
with the datum → service writes datum to vault."* The endpoint takes a client-computed blob.

**The tension — four problems, one root cause:**

1. **It contradicts the established pattern** *(verified)*. The existing low-res pipeline is
   server-side: `POST api/track/{trackId}/waveform` takes **no body** — `TrackController` pulls the
   audio from its own vault (`GetAudioBinaryAsync`) and computes via
   `WaveformProfileService.ComputeAndStoreAsync`. Upload-time profiling also runs in the API. The
   spec's flow inverts the authority: a network client computes a derived datum the server can
   derive itself from data it already owns.
2. **It is architecturally unreachable as written** *(verified)*. §3.4's own "Reuse point" demands
   the *same code path* as the existing preprocessor — but `WaveformProfileService` lives in
   `DeepDrftContent`, referenced by `DeepDrftAPI`. `DeepDrftManager` has **no in-process data
   layer** by explicit convention (root `CLAUDE.md`: all track operations are HTTP proxies). The
   CMS cannot run the shared preprocessor without breaking that constraint; running a *copy* breaks
   the reuse mandate. The spec's two requirements are mutually exclusive.
3. **Trust boundary.** An ApiKey holder POSTing arbitrary "waveform" bytes gives the server no way
   to validate the datum against the audio it purports to describe. Server-side derivation makes
   the datum correct by construction.
4. **The recovery path is punitive.** The per-row "Generate Waveform" action in `CmsMixBrowser`
   would require the CMS to download the full mix audio (mixes are the *longest* tracks in the
   catalogue), recompute, and re-upload. The existing pattern's recovery is a body-less POST.

**Recommendation — redesign 9.2.B:** make the Mix waveform endpoint a server-side trigger,
mirroring the existing idiom: `POST api/release/{id}/mix/waveform` (ApiKey, **no body**) — API
fetches audio from the vault, computes at high resolution via `WaveformProfileService`
parameterized by resolution, stores the datum in the vault, sets `MixMetadata.WaveformEntryKey`.
CMS upload flow becomes: upload track → call trigger. *(Verified: `ComputeAndStoreAsync(wavBytes,
entryKey)` currently takes resolution from injected `WaveformProfileOptions.BucketCount = 512`; the
refactor is a per-call resolution/profile parameter plus a distinct entry-key/vault target for the
high-res datum — small, and it resolves open question §7.3 in favour of the one-pipeline answer.)*

This single change also repairs F11 (the Wave 4 dependency claim) and removes the largest ApiKey
upload surface from the new controller.

### F2 — Hero-image endpoint route omits the release identity `[minor — tighten]`

**Spec says (§4):** `POST api/release/session/hero-image`. The release id is nowhere in the route;
presumably it rides the multipart form. **Recommendation:** `POST api/release/{id}/session/hero-image`
— resource-addressed, consistent with F1's trigger shape and the existing
`api/track/{trackId}/waveform` idiom. SRP at the endpoint is otherwise right: splitting hero-image
and waveform writes from track upload is correct.

---

## Findings — Open/Closed

### F3 — §8's "what a fourth medium costs" list is extensible-on-paper `[major — tighten]`

**Spec says (§8):** a fourth medium costs five items (enum value, optional metadata table + DTO,
display-metadata entry, projection entry, hero-slot renderer) and zero changes to anything existing.

**The tension:** the list omits real, recurring costs that the spec itself describes elsewhere:

- **Form field sections** — §3.3 puts medium-conditional `@if` blocks in *five* files (`TrackNew`,
  `TrackEdit`, `BatchUpload`, `BatchEdit`, `AlbumHeaderFields`). A new medium with authoring fields
  touches all five (see F4).
- **A CMS browser** — Session and Mix each get a new browser component (§3.2). Medium #4 gets one
  too, or at least a grid-parameterization entry.
- **A public gallery page + detail page + routes** — `/sessions`, `/sessions/{id}` etc. are
  per-medium pages; Blazor routes are attribute-based, so a new medium is new routable components.
- **Per-host mapping tables** — the "display-metadata entry" is not one entry: the CMS card lookup
  lives in `DeepDrftManager`, the nav `Children` list lives in `DeepDrftPublic.Client/Layout/Pages.cs`,
  and the medium→route mapping is a third. One table per concern is the right discipline, but §8
  counts three tables as one entry.

The *meaningful* OCP claim survives: a new medium modifies **zero existing tables, zero existing
media's components, zero existing endpoints**. That is the claim worth defending. The "five items"
framing undersells the additive surface and is exactly the "extensible on paper" failure §8 warns
against — by the spec's own standard.

**Recommendation:** rewrite §8 as two honest lists: (a) what is *never modified* (the true OCP
payoff), and (b) the full set of *additive* artifacts, each with its single known location. A
roadmap that understates extension cost will be trusted less the second time it's read.

### F4 — The form conditional is fine once; the spec scatters it five times `[major — tighten]`

**Spec says (§3.3):** "a small `@if` per medium in the form is acceptable and clearer than
over-abstracting… SOLID discipline matters most at the data/service layer; a form is allowed to be
explicit."

**The tension:** the principle is sound *for one form*. But the same medium-conditional logic is
specified across five form files. That is precisely the "scattered `switch`/`if` chains" the spec's
own §8 forbids — the concern (which fields does medium X author?) gets five copies that must be kept
in sync. The resolved single-track invariant (§7.8) raises the stakes: the conditionals now also
gate structural form shape (multi-track master list present/absent), not just a field or two.

**Recommendation — middle path, not over-abstraction:** extract per-medium field-section components
(`CutFields`, `SessionFields`, `MixFields` — explicit markup inside, no clever generics) and one
dispatch point (a single mapping or a `MediumFields` component with the one `@switch`) embedded by
all five forms. Inside each section, plain explicit markup — the spec's anti-over-abstraction
instinct is right *within* a section. This keeps "one table per concern" without inventing a form
framework. A new medium then adds one section component + one dispatch entry, and F3's list can
cite it honestly.

### F5 — The projection map's extensibility guarantee should be stated honestly `[moderate — tighten]`

**Spec says (§4):** include medium metadata "via a small per-medium projection map — not a hardcoded
`if session … else if mix …` chain. A future medium adds a projection entry, not a new endpoint
branch."

**The tension:** a dictionary keyed by enum *is* a switch, data-structured. The honest guarantee is
not "zero changes to the controller" but "the change is one declaration in one known place, and the
read *logic* is untouched." That is still worth having — but two refinements:

- For `GET api/release/{id}` the medium is unknown until the row is fetched, so a per-medium
  projection map either double-queries or is bypassed. With exactly two 1:1 navs behind unique FK
  indexes, **always-`Include` both** on single-release reads is simpler and naturally yields nulls
  for non-matching media — no per-medium branching at all. The map earns its keep on the *list*
  endpoint (`?medium=`), where joining every metadata table on every page query is the thing to
  avoid.
- The map is also the natural single point that enforces the F8 correlation (metadata DTO populated
  iff medium matches). Say so in the spec — that is the map's second responsibility and the reason
  it should not be inlined.

**Recommendation:** keep the map for the list read; permit always-`Include` for the by-id read;
restate the guarantee as "one entry, one file" rather than "zero changes."

### F6 — Nav `Children` collection: right call, two semantics to pin down `[accept, with conditions]`

**Spec says (§5.1):** extend the nav model with an optional `Children` collection rather than
hardcoding an ARCHIVE popover. *(Verified: `PageRoute` is a flat POCO — `Name`, `Route`, `Icon` —
rendered as a flat `<a>` list; the Sessions/Mixes entries are literal `"#"` dead links today.)*

**Assessment:** genuinely more extensible, and the complexity does *not* merely move to the model —
the model change is a nullable list; the render branch (has-children vs. leaf) exists under either
option, and option (a) keeps desktop popover and mobile indented-sublist driven by the same data.
Two things to pin down in the spec, or the model will grow warts:

1. **Dual-role nodes.** ARCHIVE has both a `Route` (`/archive`) *and* `Children`. Define the
   semantics once: desktop hover opens children, desktop click navigates to the overview; mobile
   renders the parent as a link with children indented. Don't leave it to the implementer.
2. **Depth cap.** One level only. If a future need wants nesting, that's a redesign, not a recursion.

---

## Findings — Liskov Substitution

### F7 — Rejecting the type hierarchy was the right LSP move `[good — accept]`

Option (B) in §1 (EF TPH subclasses) would have created `SessionRelease`/`MixRelease` types whose
substitutability is a lie — most code paths ("give me releases") don't care about medium, and the
ones that do would downcast. The discriminator-enum design has **no inheritance to violate**; medium
variance rides data (metadata rows) and composition (hero slots), not subtypes. This is the spec's
quiet best decision and worth keeping in §8 as the LSP rationale.

---

## Findings — Interface Segregation / contract honesty

### F8 — Optional nested metadata DTOs: acceptable, with the discipline named `[accept]`

**Spec says (§2.4):** `ReleaseDto` gains `SessionMetadata?` / `MixMetadata?`, populated only for the
matching medium; no denormalized flat fields.

**Assessment:** this is the right call, not an ISP violation, for three reasons: (1) it mirrors the
Phase 8 nested-`Release` precedent on `TrackDto` — consistency beats purity; (2) consumers that
don't care (gallery cards, grids) never touch the fields — null-checking burden lands only on the
medium-specific detail pages, which are medium-aware by definition; (3) the polymorphic-DTO
alternative (discriminated JSON hierarchy) collides with the documented BlazorBlocks `new()`
constraint on DTOs *(verified in `ReleaseDto.cs` header)* and buys nothing the enum doesn't.

Invalid states are representable (Medium == Cut with a populated `MixMetadata`) — acceptable for a
serialization contract **iff** exactly one producer maintains the correlation: the projection map
(F5). Name that in the spec.

**ISP done right downstream — preserve it:** §5.4 has `MixWaveformVisualizer` take *the datum (or a
URL) + optional position binding*, not a `ReleaseDto`. That is the segregation that matters — the
reusable component depends only on what it consumes. Hold that line for the hero slots generally:
slots receive what they render, not the whole release.

### F9 — `ReleaseDto.ReleaseType` lies for non-Cut releases `[moderate — tighten]`

**Spec says (§1):** "readers of a non-Cut release should not surface it [`ReleaseType`]."

**The tension** *(verified)*: `ReleaseDto.ReleaseType` is non-nullable, defaulting `Single`. A
Session release therefore serializes as `ReleaseType: Single` — a confidently wrong value — and the
"readers should not surface it" rule becomes a discipline imposed on **every consumer** instead of
a property of the contract. That is the DTO-level version of the scattered-switch smell.

**Recommendation:** make `ReleaseDto.ReleaseType` nullable (`ReleaseType?`), nulled at the single
entity→DTO mapping point when `Medium != Cut`. One producer enforces; zero consumers need to know
the rule. (The *entity* column staying non-nullable is fine per F10 — this is a read-model fix.)

---

## Findings — the `ReleaseType` invariant (schema-level OCP consistency)

### F10 — The spec's own §1 argument indicts its invariant handling — name the exception `[major — tighten, then accept]`

**Spec says (§1):** Cut "needs nothing extra" so it gets no metadata table; `ReleaseType` stays on
`ReleaseEntity`, unused for non-Cut media, enforced by service discipline.

**The tension:** `ReleaseType` *is* Cut-specific data living on the base table — "a column that is
meaningless for every other medium," which is verbatim the sin for which §1 rejected option (A).
The rigorous application of the spec's own pattern is a `CutMetadata { ReleaseType }` table, making
Cut symmetric with Session and Mix.

**Why the spec's pragmatic call still wins — but must say so out loud:**

- **The hot path needs it.** `/cuts` (the existing `AlbumsView`) displays Single/EP/Album on every
  card — the highest-traffic browse read would pay a join that Session/Mix detail reads (rare,
  single-row) never mind paying. The spec's "metadata joins are paid only on medium-specific
  surfaces" argument cuts *against* moving `ReleaseType` out.
- **Churn cost.** Phase 8 §8.0 just landed `ReleaseEntity` with `ReleaseType`; relocating it is a
  data migration weeks after a breaking normalization, for structural purity with no behavioral win.
- **The invariant is "meaningless," not "invalid."** A stale `Single` on a Session row harms nothing
  if the read model nulls it (F9). Low-stakes invariants warrant low-ceremony enforcement.

**Two corrections required before accepting:**

1. **The spec's factual claim is wrong.** "EF Core cannot express a conditional constraint cleanly /
   a raw SQL CHECK would be migration-fragile" — EF Core has first-class check-constraint support
   (`ToTable(t => t.HasCheckConstraint(...))`), generated and versioned in migrations, fully
   supported by Npgsql. The honest position is "we *choose* not to constrain, because the invariant
   is advisory and the read model enforces it at the mapping point" — a defensible choice. "EF
   can't" is not true and will erode trust when an implementer checks.
2. **Document the asymmetry as a deliberate exception** in §1 and `ReleaseConfiguration`: name the
   rejected `CutMetadata` option and the hot-path reason, so the next medium designer doesn't
   either (a) cargo-cult a base-table column for *their* medium's data, or (b) "fix" `ReleaseType`
   into a table without seeing the read-path cost.

---

## Findings — DRY-by-composition

### F11 — `ReleaseDetailScaffold` is the right shape; its contract is undefined and one elision hides a fork `[moderate — tighten]`

**Spec says (§5.3):** extract common detail scaffolding into `ReleaseDetailScaffold` taking a
`RenderFragment HeroContent`; `CutDetail`/`SessionDetail`/`MixDetail` compose it.

**Assessment:** composition over both duplication (option i) and a branching god-page (option ii)
is correct. Two gaps:

1. **The contract is unstated, and that's how god-components are born.** Variance already exceeds
   the hero: a Cut/Album detail has a multi-track listing; Session/Mix are single-track (resolved
   §7.8). If that variance arrives as scaffold parameters (`ShowTrackList`, `HeroAboveMeta`…), the
   scaffold becomes option (ii) wearing a composition costume. Define the contract in the spec:
   the scaffold owns exactly the invariant trio — **metadata block, play affordance, player
   wiring** — and *every* per-medium variance rides a slot supplied by the page. Rule of thumb to
   write down: **a boolean layout parameter on the scaffold is a design failure; it belongs in a
   slot.** If a second slot is genuinely needed (e.g., `BodyContent` for the Cut track list), a
   named slot is fine; a flag is not.
2. **Where does `CutDetail` live, and what happens to `TrackDetail`?** §5.2 routes `/cuts` through
   the existing `AlbumsView`, whose cards land on the existing track-cardinal `TrackDetail` — but
   §5.3 names `CutDetail` as a scaffold consumer. If `TrackDetail` stays as-is while
   Session/Mix details use the scaffold, the product has **two sources of detail-page truth** —
   exactly what *One source, multiple views* forbids. Decide explicitly: either `TrackDetail` is
   refactored onto the scaffold in Wave 4 (recommended — it is the scaffold's extraction source,
   so this is nearly free), or the fork is accepted and recorded as deliberate debt with a
   retirement note. Silence here guarantees the fork happens by accident.

---

## Findings — sequencing honesty

### F12 — "Wave 4 does not wait on Wave 3" is true for build, false for acceptance — as specced `[major — tighten via F1]`

**Spec says (PLAN §9 dependency summary):** Wave 4 can begin once Wave 2's `api/release` family is
stable; it does not wait on Wave 3.

**The tension:** Wave 4's 9.4.D acceptance criterion is "`/mixes/{id}` renders the waveform
visualizer **fed by real datum**." Under the spec's current 9.2.B design, the only producer of
that datum is the Wave 3C CMS pipeline (the endpoint receives a client-computed blob, and no other
tool computes one). Sessions are fine — `POST …/hero-image` is self-contained, so hero images can
be seeded with a script against Wave 2B. Mixes are not. As written, Wave 4's Mix acceptance has a
hidden dependency on Wave 3C.

**Recommendation:** adopt F1's redesign (server-side trigger) and the claim becomes *true* — a
single ApiKey'd POST against Wave 2 generates real high-res data with no CMS in existence. If F1 is
rejected, amend the dependency summary instead: "Wave 4 *build* is parallel with Wave 3; 9.4.D
acceptance is gated on 9.3.C (or a manual datum-seeding script)." Either fix is acceptable;
shipping the claim as-is is not — a parallelization promise that fails at acceptance time is the
kind of roadmap debt that gets discovered mid-wave.

---

## Findings — minor

### F13 — `HeroImagePath` is an entry key wearing a path's name `[minor — tighten]`

§2.3's comment says it: *"entry key in the image vault."* `MixMetadata` calls its key
`WaveformEntryKey`; `ReleaseEntity.ImagePath` is a genuine path-ish field. Recommend
`HeroImageEntryKey` — the inconsistency will otherwise propagate into the DTO, the endpoint, and
the CMS form labels, and someone will eventually pass a URL into it.

---

## What the spec gets right (keep these)

- **The spine (§1, option C).** Discriminator enum + optional 1:1 metadata tables is the correct
  shape, and the §1 comparison against the wide table and the type hierarchy is genuine analysis,
  not decoration. Base reads never join; medium reads pay one targeted join. Sound.
- **Enum-driven CMS cards (§3.1)** with an explicit prohibition on the three-arm markup switch —
  the right discipline stated at the right layer.
- **`api/release` as a new family (§4)** rather than overloading `api/track/page` — correct SRP at
  the HTTP boundary, and the release-cardinality argument is the real reason, not just taste.
- **Additive-only migration with `Cut = 0` default (§2.1)** — zero data migration of existing rows.
- **The visualizer's position-binding seam designed now, seek deferred (§5.4)** — textbook
  *design for adaptability up front*.
- **Resolved §7.8 (hard single-track constraint)** — kills a whole class of half-valid states the
  forms would otherwise have to police.

---

## Verdict

**The spine is SOLID-sound; the spec ships with one architectural defect and two honesty gaps.**
The discriminator-plus-optional-table design, the new API family, the scaffold composition, and the
enum-driven CMS surfaces are all the right shapes and genuinely extension-friendly. But:

**Must change before implementation begins (blocks Wave 2 design freeze):**
1. **F1** — redesign `POST api/release/mix/waveform` as a body-less server-side trigger
   (`POST api/release/{id}/mix/waveform`); the CMS-computes flow contradicts the verified existing
   pattern and is unreachable under the CMS's no-data-layer constraint. Also resolves the
   preprocessor-reuse open question (§7.3) and repairs the Wave 4 dependency claim (F12).
2. **F9** — `ReleaseDto.ReleaseType` becomes nullable, nulled at the mapping point for non-Cut.
3. **F10** — correct the false EF check-constraint claim; document the `ReleaseType`-on-base-table
   asymmetry as a named, reasoned exception (rejected `CutMetadata`, hot-path join cost).

**Should change before the relevant wave:**
4. **F4** — per-medium form-section components with one dispatch point, replacing five scattered
   conditional blocks (Wave 3).
5. **F11** — write the scaffold's contract (owns the invariant trio; variance rides slots; no
   boolean layout parameters) and decide `TrackDetail`'s fate explicitly (Wave 4).
6. **F3** — rewrite §8 with the honest two-list extension cost.
7. **F2, F6 semantics, F13** — small tightenings, cheap now, annoying later.

None of these findings disturb Daniel's four resolved decisions (§7.2, §7.5, §7.8, and the genre
tab) — they all survive intact. F1 effectively *answers* open question §7.3 (one parameterized
server-side pipeline) and strengthens §7.1's vault-blob recommendation; both still need Daniel's
sign-off before the spec is amended.