docs: SOLID review of Phase 9 spec — waveform compute tier flagged critical

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

@@ -0,0 +1,350 @@
+# 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.