daniel-c-harvey
203 lines
11 KiB
Markdown
203 lines
11 KiB
Markdown
# 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 `<div class="player-spacer @_audioPlayerClass">` 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<bool> 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.
|