daniel/deepdrft
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 13 Wiki Activity
Files
master
deepdrft/product-notes/phase-9-solid-review.md
T

22 KiB
Raw Permalink Blame History

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.

Reference in New Issue View Git Blame Copy Permalink