diff --git a/product-notes/player-minimize-sync.md b/product-notes/player-minimize-sync.md new file mode 100644 index 0000000..7e9f4f0 --- /dev/null +++ b/product-notes/player-minimize-sync.md @@ -0,0 +1,202 @@ +# Design Brief: Player Minimize/Spacer Sync + +**Status:** Proposed — awaiting direction +**Surface:** `DeepDrftPublic.Client` audio player dock + `MainLayout` spacer +**Author:** product-designer + +--- + +## 1. Root cause + +`AudioPlayerBar` owns `_isMinimized` as private component state. `MainLayout` renders a +sibling spacer `
` whose class +(`minimized` / `expanded`) reserves vertical space for the fixed dock so page content +does not overlap it. The two are kept in sync by a single bridge: the +`EventCallback OnMinimized` parameter. + +`_isMinimized` is mutated on **three** paths. Only one of them fires the bridge: + +| Path | Method | Mutates `_isMinimized` | Fires `OnMinimized` | Spacer stays correct? | +|------|--------|:---:|:---:|:---:| +| Manual minimize/restore button | `ToggleMinimized()` (line 179) | yes | **yes** (line 182) | yes | +| Track selected → auto-expand | `Expand()` (line 118) via `PlayerService.OnTrackSelected` | yes (`true`→`false`) | **no** | **no** | +| Close (X) button | `Close()` (line 186) | yes (`false`→`true`) | **no** | **no** | + +Resulting defects: + +- **Select a track:** bar expands, spacer stays `minimized` → content overlaps the now-visible dock. +- **Close the player:** bar collapses to the FAB, spacer stays `expanded` → a dead gap is left below content. + +The `ResizeObserver` hook in `OnAfterRenderAsync` (line 83) already reacts to +`_isMinimized` correctly on every path — it toggles `observe`/`unobserve` purely off +`!_isMinimized && !Fixed` and guards re-entry with `_spacerObserved`. So the CSS-var +height publishing is **not** part of this bug; only the spacer's *class* (which reserves +the box at all) drifts. Any fix must preserve the observer hook's correctness. + +### Secondary observation (matters for Option B) + +`Close()` is not a pure "minimize" — it also calls `PlayerService.Unload()` when a track +is loaded (line 188-190). The close path therefore couples *two* concerns: tearing down +playback and collapsing the chrome. A fix should keep "minimized state" as the thing the +spacer tracks, independent of the unload side effect. + +--- + +## 2. Design options + +### Option A — Fix at the call sites + +Fire the existing callback from the two paths that currently skip it: add +`OnMinimized.InvokeAsync(false)` inside `Expand()` and `OnMinimized.InvokeAsync(true)` +inside `Close()`, guarded the same way `ToggleMinimized` already guards +(`if (OnMinimized.HasDelegate)`), and only when the state actually flips. + +**Change:** Two added invocations inside `AudioPlayerBar.razor.cs`. No new types, no DI, +no signature changes. `MainLayout` and the `OnMinimized` contract are untouched. + +**Scope:** Smallest possible. One file, ~4 lines. + +**Edge cases / risks:** +- `Expand()` and `Close()` are `async` but currently never `await` — adding + `await OnMinimized.InvokeAsync(...)` makes them genuinely async; confirm callers + (`PlayerService.OnTrackSelected = new EventCallback(this, Expand)` at line 75, and the + `@onclick` on the X button) await/route the returned task. `EventCallback` handles this + correctly, so this is low risk but worth a glance. +- Must fire **only on an actual transition** (mirror the existing `if (_isMinimized)` / + `if (!_isMinimized)` guards) to avoid redundant `StateHasChanged` churn in `MainLayout`. +- Keeps the bug class alive structurally: any *future* fourth path that sets + `_isMinimized` will reintroduce the same drift. This is a "patch each leak" fix, not a + "remove the leak class" fix. +- The contract stays push-based and one-directional (child → layout), which matches the + current mental model and the existing `ToggleMinimized` precedent. + +### Option B — Lift minimize state into a shared service / cascade + +Make `_isMinimized` a derived view of authoritative state held outside the bar — e.g. a +small `PlayerChromeState` service (scoped, cascaded by `AudioPlayerProvider` alongside the +player service) exposing `IsMinimized` + a `StateChanged` event. `AudioPlayerBar` reads +and writes through it; `MainLayout` subscribes (or cascades it) and derives +`_audioPlayerClass` reactively. The `OnMinimized` parameter is retired. + +**Change:** New service type + DI registration; `AudioPlayerBar` rewires its three +mutation points to the service; `MainLayout` subscribes instead of taking a callback. +Touches 3-4 files plus `Startup.ConfigureDomainServices`. + +**Scope:** Medium. Structural — introduces a new piece of shared state. + +**Edge cases / risks:** +- Eliminates the bug *class*: any path that flips minimize state flows through one + setter, so the spacer can never drift regardless of how many triggers exist later. +- Aligns with the project's stated direction — **one source, multiple views**: chrome + state becomes a single authority that both the bar and the layout render from, rather + than the bar owning private state and pushing notifications. (See memory: + one-source-multiple-views.) +- The `ResizeObserver` hook still keys off the bar's local read of `IsMinimized`, so it + stays correct — but the bar must `StateHasChanged` (and thus re-run + `OnAfterRenderAsync`) when the service fires, exactly as it already does for + `StateChanged` from the player service (line 76-81). The wiring pattern already exists; + this reuses it. +- More moving parts for a defect that is, today, a missed callback. Risk of over-building + if minimize state never grows beyond this one consumer. +- Must decide ownership lifetime: the cascade is `IsFixed` (line: provider stores player + with `IsFixed="true"`), so a cascaded chrome-state value would need the same + subscribe-to-event escape hatch the bar already uses for the player service. Reuses a + known pattern, but it is the fiddly part. + +### Option C — Single internal mutator (middle path) — *recommended* + +Keep the `OnMinimized` callback contract and `MainLayout` exactly as they are, but funnel +**all three** trigger paths through one private method inside `AudioPlayerBar` that is the +*only* place `_isMinimized` is assigned: + +``` +SetMinimized(bool value): + if (_isMinimized == value) return; // transition guard + _isMinimized = value; + if (OnMinimized.HasDelegate) await OnMinimized.InvokeAsync(value); + StateHasChanged(); +``` + +`ToggleMinimized` becomes `SetMinimized(!_isMinimized)`. `Expand()` becomes +`SetMinimized(false)`. `Close()` keeps its `Unload()` side effect, then calls +`SetMinimized(true)`. `OnParametersSet`'s `Fixed` branch (line 59-62) can also route +through it for consistency. + +**Change:** One new private method; the three existing mutation points delegate to it. +Single file (`AudioPlayerBar.razor.cs`), no new types, no DI, no contract change. + +**Scope:** As small as Option A, but **structurally closes the bug class** the way +Option B does — by guaranteeing there is exactly one assignment site that always fires the +bridge. + +**Why this is the sweet spot:** It gets Option B's invariant ("every state change +propagates") without Option B's new shared-state machinery. The drift is impossible +because the callback firing is co-located with the only assignment, not duplicated across +call sites where the next contributor can forget it. + +--- + +## 3. Recommendation + +**Option C.** It satisfies all three evaluation criteria with the smallest durable footprint: + +- **(a) Blast radius:** one file, no new types, no DI, no contract or `MainLayout` change — + identical reach to Option A. +- **(b) Correctness across all three paths:** by construction, every assignment of + `_isMinimized` goes through the single mutator that fires the bridge and guards the + transition. Option A fixes today's two missed paths but leaves the pattern that produced + them; Option C removes the pattern. +- **(c) ResizeObserver stays correct:** the mutator calls `StateHasChanged()`, which + triggers `OnAfterRenderAsync`, which re-evaluates `shouldObserve` off the now-updated + `_isMinimized` exactly as today. No change to the observer logic. + +Option B is the right move *only if* minimize/chrome state grows additional consumers +(e.g. a second surface that needs to read or drive it, or deep-link/restore-on-load +behaviour). It is over-scoped for the current single-consumer reality. Note the path +forward is clean: if that need arrives, Option C's single mutator is the natural seam to +later back with a service — `SetMinimized` becomes the one call site to redirect. Choosing +C now does not foreclose B later. + +**Trade-off being accepted:** C keeps minimize state private to the bar and the +sync push-based. If a future feature needs the layout (or anything else) to *drive* +minimize state rather than just react to it, that is the trigger to revisit Option B. + +--- + +## 4. Acceptance criteria + +A correct fix satisfies all of the following observable conditions: + +1. **Track-select expand:** selecting a track while minimized expands the bar **and** the + spacer element's class changes from `minimized` to `expanded` (content reflows below the + dock, no overlap). +2. **Manual toggle sync:** clicking minimize collapses the bar to the FAB and the spacer + class returns to `minimized`; clicking restore expands the bar and the spacer class + becomes `expanded`. Bar and spacer never disagree. +3. **Close sync:** clicking the X minimizes the bar **and** the spacer class reverts to + `minimized` (no residual empty gap), and — unchanged — unloads the track when one is + loaded. +4. **No double-invocation:** the manual toggle path fires `OnMinimized` exactly once per + click (no regression from the existing single-fire behaviour). Re-triggering a path + that does not change state (e.g. `Expand()` when already expanded) fires nothing. +5. **ResizeObserver lifecycle:** across all three paths, the spacer `ResizeObserver` is + `observe`d when (and only when) the bar is expanded and not `Fixed`, and `unobserve`d + otherwise — i.e. `_spacerObserved` ends up consistent with `!_isMinimized && !Fixed` + after each transition. +6. **No spacer height strand:** after close/minimize, no stale `--player-height` reserves + phantom space (the existing `unobserve` path already clears this; the fix must not + bypass it). + +--- + +## 5. Notes for the implementer (staff-engineer) + +- The `Fixed` embed path (line 59-62) sets `_isMinimized = false` directly in + `OnParametersSet` and intentionally renders no `PlayerWindowControls` (line 42-45) and no + spacer observation (the `!Fixed` guard). Whatever mutator is introduced must not fire + `OnMinimized` for the `Fixed` embed in a way that makes a host page's layout reserve dock + space it does not have. Simplest: the `Fixed` branch may bypass the callback, or + `MainLayout` is simply not present on embed surfaces. Confirm before routing the `Fixed` + branch through the shared mutator. +- This brief specifies behaviour and structure only. No implementation is included by + design; dispatch staff-engineer to land it.