From 8087fd04ce9150745e8d898c03284b3f27f297b2 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 12 Jun 2026 21:00:04 -0400 Subject: [PATCH] =?UTF-8?q?docs:=20SOLID=20review=20of=20Phase=209=20spec?= =?UTF-8?q?=20=E2=80=94=20waveform=20compute=20tier=20flagged=20critical?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- product-notes/phase-9-solid-review.md | 350 ++++++++++++++++++++++++++ 1 file changed, 350 insertions(+) create mode 100644 product-notes/phase-9-solid-review.md diff --git a/product-notes/phase-9-solid-review.md b/product-notes/phase-9-solid-review.md new file mode 100644 index 0000000..128ce5b --- /dev/null +++ b/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 `` 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.