jellytau/docs/specs/player-facade-enforcement.md

# Spec: Enforce the unified player boundary

**Status:** Proposed — not started. The count below has not improved: ~60
`commands.player*` call sites still live outside `src/lib/player/`, and no lint
rule enforces the boundary. This remains the one stated design principle with no
automated check.
**Requirements:** ⚠️ the suggested id **DR-095 has since been allocated** to seek
clamping — allocate a fresh id (DR-215 or later) on implementation. Relates to
UR-005 and the unified-player-boundary
principle in CLAUDE.md and [02-svelte-frontend.md](../architecture/02-svelte-frontend.md)
**UX spec:** n/a — refactor, no user-visible change.
**Supersedes / revises:** n/a

## Summary

The stated principle is that UI controls playback **only** through
`playerController` ([src/lib/player/index.ts](../../src/lib/player/index.ts)),
never by calling `commands.player*` directly. There are **52 direct call sites
outside** that facade. This spec routes the genuine playback-control calls
through the facade, narrows the principle's wording so it stops forbidding
things it never meant to forbid, and adds the lint rule that keeps it true —
because this rule is the one design principle in the audit with **no automated
check at all**, and it is also the one that drifted furthest.

## Motivation

Direct `commands.player*` usage outside `src/lib/player/`, by file:

| File | Sites |
|---|---|
| [queue.ts](../../src/lib/stores/queue.ts) | 10 |
| [player/[id]/+page.svelte](../../src/routes/player/[id]/+page.svelte) | 9 |
| [VideoPlayer.svelte](../../src/lib/components/player/VideoPlayer.svelte) | 8 |
| [settings/+page.svelte](../../src/routes/settings/+page.svelte) | 5 |
| [sleepTimer.ts](../../src/lib/stores/sleepTimer.ts) / [auth.ts](../../src/lib/stores/auth.ts) / [autoplay.ts](../../src/lib/api/autoplay.ts) | 4 each |
| [preload.ts](../../src/lib/services/preload.ts) | 3 |
| [library/[id]](../../src/routes/library/[id]/+page.svelte), [playerEvents.ts](../../src/lib/services/playerEvents.ts), [playbackMode.ts](../../src/lib/stores/playbackMode.ts) | 1–2 each |

These are **not** equivalent violations, and treating them as one number is why
the rule has been easy to ignore. Three distinct groups:

**(a) Genuine violations — playback control with a facade method that already
exists.** `playerStop` ×6, `playerPlayTracks` ×4, `playerSeek` ×2,
`playerPlayAlbumTrack` ×2, `playerNext`, `playerPrevious`, `playerSkipTo`,
`playerToggleShuffle`, `playerCycleRepeat`, `playerRemoveFromQueue`,
`playerMoveInQueue`, `playerAddTrackById`, `playerAddTracksByIds`,
`playerSetSubtitleTrack`, `playerPlayItem`. The facade exposes `stop()`,
`seek()`, `next()`, `previous()`, `skipTo()`, `toggleShuffle()`,
`cycleRepeat()`, `removeFromQueue()`, `moveInQueue()`, `addTrackById()`,
`addTracksByIds()`, `setSubtitleTrack()`, `playTracks()`, `playAlbumTrack()`,
`playItem()` — every one of these has a facade equivalent that is simply not
being called. `queue.ts` is the starkest case: it imports `commands` directly
and re-implements ten methods the facade already provides.

**(b) Playback control with no facade method.** `playerPlayQueue`,
`playerGetQueue`, `playerGetStatus`, `playerEnterBackgroundAudio`,
`playerExitBackgroundAudio`, `playerSetSleepTimer`, `playerCancelSleepTimer`,
`playerPlayNextEpisode`, `playerCancelAutoplayCountdown`. In scope for the
principle, but currently *impossible* to comply with — the facade has no surface
for them. A rule that cannot be followed is not being broken so much as it is
unfinished.

**(c) Not playback control.** `playerConfigureJellyfin` ×3,
`playerDisableJellyfin`, `playerGet/SetAudioSettings`,
`playerGet/SetVideoSettings`, `playerGetEqPresets`,
`playerGet/SetAutoplaySettings`, `playerGet/SetCacheConfig`,
`playerPreloadUpcoming`. These are configuration and lifecycle calls that happen
to live under the `player_` command prefix. The principle is about *who is
authoritative for playback state* — settings CRUD isn't that.

The audit's read: the rule as written is violated 52 times, which makes real
drift indistinguishable from acceptable usage, and that ambiguity is what lets
group (a) persist. Note also that the principle **is** well-honoured where it
matters most — the read side is clean, with UI reading state exclusively from
the facade's re-exported stores. The write side is what drifted.

## Layer assignment

Frontend-internal refactor. No domain logic moves and nothing new crosses IPC —
the same Rust commands are called, through one module instead of many.

| Logic / responsibility | Layer | Why it belongs there |
|------------------------|-------|----------------------|
| Playback command dispatch (adapter routing: native vs HTML5) | Frontend — `src/lib/player/` **only** | Presentation-layer plumbing, but must be centralised: the facade picks between the native backend and the HTML5 `<video>` adapter. A caller bypassing it silently skips that routing. |
| Playback *authority* (position, pause, rate, track changes) | **Rust / the player** | Unchanged. The player is authoritative; UI is a consumer. This spec does not touch that direction. |
| Queue mutation commands | Frontend facade → Rust | Rust owns queue state; the facade is the single call path to it. |
| Player settings CRUD (EQ, video, autoplay, cache) | Frontend, **outside** the facade | Configuration, not playback control — read/written on a settings page with no adapter routing. Explicitly carved out below. |
| Backend→frontend event handling | `playerEvents.ts` | Already correct. It is the facade's own plumbing, not a bypassing consumer. |

No Jellyfin taxonomy is involved, so no boundary-leak risk.

## Design

### 1. Narrow the principle to what it actually means

Amend CLAUDE.md and [02-svelte-frontend.md](../architecture/02-svelte-frontend.md):

> **Unified player boundary.** UI controls **playback** — transport, queue
> mutation, track selection, playback initiation — *only* through
> `playerController`. Player **configuration** commands (`player_*_settings`,
> `player_configure_jellyfin`, `player_*_cache_config`, `player_preload_upcoming`)
> are ordinary IPC and may be called directly from settings surfaces.

This is a clarification, not a relaxation: it makes group (c) explicitly fine so
that a violation count means something. A rule with 52 nominal violations, most
of them acceptable, provides no signal.

### 2. Fill the facade gaps (group b)

Add to `playerController`, each a thin pass-through preserving current
behaviour:

```ts
playQueue, getQueue, getStatus,
enterBackgroundAudio, exitBackgroundAudio,
setSleepTimer, cancelSleepTimer,
playNextEpisode, cancelAutoplayCountdown,
```

Do this **first** — group (a) cannot be fully migrated while callers still need
a direct import for a neighbouring call, and a file that imports `commands` for
one reason will keep using it for others.

### 3. Migrate group (a)

Mechanical: replace `commands.playerX(...)` with `playerController.x(...)`.
Highest-value first: `queue.ts` (10 sites, all direct facade equivalents), then
`player/[id]/+page.svelte`, `VideoPlayer.svelte`, `sleepTimer.ts`,
`playbackMode.ts`, `library/[id]/+page.svelte`.

Two sites need care rather than substitution:

- **`playerEvents.ts`** (`playerOnPlaybackEnded`, `playerStop` in the error
  path). This module *is* the facade's event plumbing — the counterpart to
  `index.ts`, inside the boundary conceptually though not by directory. Treat
  `src/lib/services/playerEvents.ts` as **inside** the boundary and exempt it,
  rather than making it call the facade that calls back into it. Record this in
  the lint config with the reason.
- **`VideoPlayer.svelte`** — registers its own adapter via `setActiveAdapter`.
  Its `playerStop`/`playerPlayItem` calls interact with adapter lifecycle, and
  CLAUDE.md's gotcha ("no lifecycle calls after an `await` in `onMount`") applies.
  Migrate this file **last and on its own**, so an Android seek regression is
  bisectable to one commit.

### 4. Add the lint rule (the part that makes it stick)

The audit's finding was that principles with working checks held up and
principles without them drifted. This principle has no check. Add
`scripts/check-player-boundary.sh`, wired as `bun run check:player-boundary` and
into `test-all.sh`:

```sh
# Playback-control commands that MUST go through the facade.
CONTROL='player(Play|Pause|Toggle|Stop|Seek|Next|Previous|SkipTo|ToggleShuffle|CycleRepeat|RemoveFromQueue|MoveInQueue|SetVolume|ToggleMute|SetSubtitleTrack|SeekVideo|SwitchAudioTrack|PlayTracks|PlayAlbumTrack|PlayItem|PlayQueue|AddTrackById|AddTracksByIds|GetQueue|GetStatus|EnterBackgroundAudio|ExitBackgroundAudio|SetSleepTimer|CancelSleepTimer|PlayNextEpisode|CancelAutoplayCountdown|OnPlaybackEnded)'

# Inside the boundary: the facade and its event plumbing.
EXEMPT='^src/lib/player/|^src/lib/services/playerEvents\.ts$'
```

Flag `commands.$CONTROL` in non-test `src/` files outside `EXEMPT`. Config
commands are deliberately absent from the list, matching §1 — so the check
encodes the narrowed rule rather than the aspirational one.

An ESLint `no-restricted-syntax` rule would give better editor feedback, but the
project has no ESLint config; a shell check matches the existing
`check:boundary` precedent and adds no dependency.

## Out of scope

- Changing playback *behaviour* — pure refactor.
- The one-directional state principle (audited clean; UI reads from facade
  stores only).
- Moving settings CRUD behind the facade (§1 explicitly carves it out).
- Introducing ESLint.
- Refactoring `VideoPlayer.svelte`'s 2079 lines generally, beyond its facade
  call sites.
- The `commands.player*` calls **inside** `src/lib/player/` — that is the
  facade doing its job.

## Acceptance criteria

- [ ] `playerController` exposes the group-(b) methods listed in §2.
- [ ] `grep -rn "commands\.player" src/ --include='*.ts' --include='*.svelte' | grep -v '^src/lib/player/' | grep -v 'playerEvents\.ts' | grep -v '\.test\.' | grep -v bindings.ts`
      returns **only** configuration commands per §1 — no transport, queue, or
      playback-initiation call.
- [ ] `queue.ts` no longer imports `commands` from bindings.
- [ ] `bun run check:player-boundary` exists, is wired into `test-all.sh`, and
      passes.
- [ ] The check **fails** when a `commands.playerStop()` is added to a non-exempt
      file — verify explicitly, as with the other gates in this batch.
- [ ] The check does **not** fail on `commands.playerSetAudioSettings()` in
      `settings/+page.svelte` (the §1 carve-out works).
- [ ] CLAUDE.md and `02-svelte-frontend.md` carry the narrowed wording, including
      the config carve-out and the `playerEvents.ts` exemption with its reason.
- [ ] **No behavioural change**: audio and video playback, queue reorder,
      shuffle/repeat, sleep timer, background audio, and autoplay all behave as
      before on **both Linux and Android**.
- [ ] Android seek and `onMount` lifecycle still correct after the
      `VideoPlayer.svelte` migration (the known-fragile path).
- [ ] `bun run check` and `bun run test` pass.
- [ ] `bun run check:boundary` passes.
- [ ] Changed code carries `// TRACES:` comments.
- [ ] No Rust change, so no `bindings.ts` regeneration.

## Testing

**Frontend** (`bun run test`):
- Extend the existing facade tests to cover each new group-(b) method: it
  forwards to the right command with the right arguments, and routes to the
  active adapter where applicable.
- `queue.ts` tests: assert calls land on `playerController`, not `commands`. Mock
  the facade — a test that mocks `commands` would pass either way and guard
  nothing.
- Keep `tauriIntegration.test.ts` and the other IPC param-naming tests green;
  they cover the camelCase rule this refactor must not disturb.

**Manual** (no automated coverage for these paths):
- Linux: play/pause/seek/next/prev, queue reorder, shuffle, repeat, sleep timer,
  transcoded video (HLS), background audio enter/exit.
- Android: the same, plus lockscreen/MediaSession controls, and **seek after
  entering the player** — the specific regression CLAUDE.md warns about.

Because this is a pure refactor, the strongest signal is that no test *changes
expectation*. A test needing its assertions rewritten means behaviour moved —
investigate rather than update it.

## TRACES

Allocate in `requirements.md`:

- **DR-095** — "UI playback control is routed exclusively through the
  `playerController` facade (`src/lib/player/`), with `playerEvents.ts` inside
  the boundary as its event plumbing and player *configuration* commands
  explicitly outside it; enforced by `scripts/check-player-boundary.sh`."
  Category: Player. Traces to UR-005. Status: Done on merge.

```typescript
// src/lib/player/index.ts
// TRACES: UR-005 | DR-095
```

New facade tests take `@req-test: UT-089` onward (next free UT is **UT-089**;
coordinate if landing alongside the sibling specs, which draw from the same
pool).

## Notes for the implementer

- A parallel Claude session may be active in this repo — `git diff` before
  "repairing" unexpected changes (CLAUDE.md §Gotchas).
- **Order matters**: §2 (fill gaps) → §3 (migrate, `VideoPlayer.svelte` last and
  alone) → §4 (add the check). Adding the check first turns `master` red.
- 🔴 **`VideoPlayer.svelte`**: no lifecycle calls after an `await` in `onMount` —
  it flips to HTML5 mode and breaks Android seek. Do not let a mechanical
  substitution introduce an `await` before a lifecycle call.
- The facade's `requireHandle()` may throw where a raw `commands` call did not.
  Check each migrated call site's error handling rather than assuming the
  try/catch still covers the same cases.
- `playbackMode.ts` interacts with remote-mode routing (`play_on_session` vs
  local MPV). Verify remote casting still works after migrating its
  `playerPlayTracks` call.
- This spec is deliberately the *lowest* priority of the audit batch: it is the
  largest diff and the only one carrying real regression risk, while the
  traceability gate is a few lines and restores a dead safety net.