docs: fold shipped specs into the architecture docs and delete them
A spec was a promise; sixteen of them had become descriptions of code that already shipped, sitting beside four that describe work still outstanding, with nothing in the file telling the two apart. Half the statuses were also wrong — audio-equalizer read "Accepted" with the EQ live on both platforms, the native video spec said the flag stays off after the default was flipped on. The shipped designs move into docs/architecture, which is the maintained description of the build, and the spec files go. Git history keeps the originals; what a future change still needs is carried across: - 01-rust-backend: favourites rewritten (the old section named a file that no longer exists and called shipped buttons "planned"), domain vocabulary owned by Rust (SearchScope, exclusions, the bitrate ladder), background workers - 02-svelte-frontend: app shell and chrome, library mosaic, series/episode navigation, downloaded browse, safe-area insets, native-video store, logging - 03-data-flow: locally-indexed search - 05-platform-backends: audio settings on ExoPlayer, the equalizer's band vocabulary, native video compositing, the background-audio handoff - 06-downloads-and-offline: one storage model, offline catalog visibility - 09-security: path confinement and input binding docs/specs/README.md now says what the directory is for and where each shipped design went. Deferred work the specs recorded is kept beside the code it concerns rather than lost: season-bounded autoplay, the two dead search commands, why indexing is a full crawl. requirements.md had fourteen stale statuses — Android audio parity still read "Linux only", DR-150 still said the native-video default was off, DR-190 was Proposed after DR-196 implemented it, and five tooling requirements were Proposed after landing. Three unbuilt specs suggested requirement ids that have since been allocated to other work; each now carries a warning.
This commit is contained in:
@@ -0,0 +1,79 @@
|
||||
# Specs index
|
||||
|
||||
Feature specs for JellyTau. Start a new one from
|
||||
[SPEC-TEMPLATE.md](SPEC-TEMPLATE.md) and run it past
|
||||
[SPEC-REVIEW-CHECKLIST.md](SPEC-REVIEW-CHECKLIST.md) before accepting it.
|
||||
|
||||
## What lives here
|
||||
|
||||
**Only work that has not shipped.** Once a spec is fully implemented its design
|
||||
is folded into the architecture docs — which are the maintained description of
|
||||
the build — and the spec file is deleted. Git history keeps the original,
|
||||
including its rejected alternatives and acceptance criteria; the architecture
|
||||
docs keep the reasoning that a future change still needs.
|
||||
|
||||
So: a file in this directory is a **promise, not a description**. If you want to
|
||||
know how something *works*, read
|
||||
[docs/architecture/](../architecture/README.md). If you want to know what is
|
||||
*planned*, read here.
|
||||
|
||||
**Status vocabulary**
|
||||
|
||||
| Status | Meaning |
|
||||
|---|---|
|
||||
| Proposed | Written, not accepted. Nothing built. |
|
||||
| Accepted | Agreed as the design; implementation not started or not finished. |
|
||||
| Partially implemented | Some parts shipped; the spec names what is left. |
|
||||
| Design authority | No code of its own — it records a decision later specs act on. |
|
||||
|
||||
**Next free requirement ids** (always re-check
|
||||
[requirements.md](../requirements.md) before allocating): **UR-077**,
|
||||
**IR-033**, **DR-215**. Three specs below suggested ids that have since been
|
||||
taken by other work; each carries a ⚠️ note at the top.
|
||||
|
||||
## Partially implemented
|
||||
|
||||
| Spec | What landed | What is left |
|
||||
|---|---|---|
|
||||
| [frontend-domain-model.md](frontend-domain-model.md) | Catalog surface: `MediaKind`, `from_jellyfin` isolated, ticks → ms | `primaryImageTag` → `imageId` (~30 sites); player/session/reporting tick math; `stream.type` |
|
||||
| [libmpv2-migration.md](libmpv2-migration.md) | `LICENSE` | The `libmpv` → `libmpv2` crate swap |
|
||||
| [read-through-media-cache.md](read-through-media-cache.md) | DR-126…128, DR-133…138 — cache entries *are* download rows; local playback of downloads | DR-121/122/124/125 — the player quality selector and the read-through capture |
|
||||
| [scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md) | Stage 1: `SearchScope` owned by Rust (DR-063…067) | Stage 2: result-side grouping (`GROUP_ITEM_TYPES` still in `searchScope.ts`) |
|
||||
|
||||
## Not started
|
||||
|
||||
| Spec | Blocked on / note |
|
||||
|---|---|
|
||||
| [build-provenance.md](build-provenance.md) | `build.rs` is still bare. ⚠️ suggested id DR-093 is taken. |
|
||||
| [player-facade-enforcement.md](player-facade-enforcement.md) | ~60 `commands.player*` sites still outside the facade; no lint rule. ⚠️ suggested id DR-095 is taken. |
|
||||
| [windows-native-audio-backend.md](windows-native-audio-backend.md) | Blocked on the libmpv2 swap. ⚠️ suggested id IR-030 is taken. |
|
||||
|
||||
## Design authority
|
||||
|
||||
| Spec | Role |
|
||||
|---|---|
|
||||
| [playback-backend-unification.md](playback-backend-unification.md) | Why video cannot unify onto one native engine and audio can. The audio half has since shipped on Android; Windows has not. |
|
||||
| [scoped-search-boundary.md](scoped-search-boundary.md) | The boundary design the `check:boundary` rule came from. Stage 1 built. |
|
||||
| [scoped-search.md](scoped-search.md) | Superseded in part — its "frontend only, no Rust changes" decision is the leak the boundary spec reversed. UX still current. |
|
||||
|
||||
## Where the shipped specs went
|
||||
|
||||
Sixteen specs were folded into the architecture docs and deleted (2026-08-21).
|
||||
Where to look for each:
|
||||
|
||||
| Shipped work | Now documented in |
|
||||
|---|---|
|
||||
| Account menu & global chrome | [02-svelte-frontend.md](../architecture/02-svelte-frontend.md) — App Shell and Chrome |
|
||||
| Library mosaic | [02-svelte-frontend.md](../architecture/02-svelte-frontend.md) — Library Mosaic |
|
||||
| Series current-episode navigation | [02-svelte-frontend.md](../architecture/02-svelte-frontend.md) — Series and Episode Navigation |
|
||||
| Downloads as an offline library | [02-svelte-frontend.md](../architecture/02-svelte-frontend.md) — Downloaded Browse |
|
||||
| Favourites browsing | [01-rust-backend.md](../architecture/01-rust-backend.md) — Favorites System |
|
||||
| Streaming bitrate cap | [01-rust-backend.md](../architecture/01-rust-backend.md) — Streaming quality ladder |
|
||||
| Locally-indexed search | [03-data-flow.md](../architecture/03-data-flow.md) — Search Flow; [01-rust-backend.md](../architecture/01-rust-backend.md) — Background workers |
|
||||
| Offline downloaded-only filter | [06-downloads-and-offline.md](../architecture/06-downloads-and-offline.md) — Offline Catalog Visibility |
|
||||
| Audio equalizer · Android audio settings parity | [05-platform-backends.md](../architecture/05-platform-backends.md) — Audio settings on ExoPlayer |
|
||||
| Android native video spike | [05-platform-backends.md](../architecture/05-platform-backends.md) — Native Video Compositing |
|
||||
| Video background audio | [05-platform-backends.md](../architecture/05-platform-backends.md) — Background Audio Handoff |
|
||||
| Traceability gate repair | [traceability-ci.md](../traceability-ci.md) |
|
||||
| Boundary tripwire hardening | `scripts/check-frontend-boundary.sh` (its header is the spec) |
|
||||
| Playback docs corrections · req-coverage script removal | Nothing to document — both were corrections that have been applied |
|
||||
@@ -1,164 +0,0 @@
|
||||
# Spec: Account menu and global chrome availability
|
||||
|
||||
**Status:** Implemented
|
||||
**Scope:** Frontend only. No Rust changes required.
|
||||
**Requirements:** UR-054 → DR-075, DR-076, DR-077 (see
|
||||
[requirements.md](../requirements.md)).
|
||||
**UX spec:** [ux-flows.md §1.2–1.4](../ux-flows.md).
|
||||
|
||||
## Summary
|
||||
|
||||
Account actions — Settings, Downloads, Display preferences, Sign out — are
|
||||
currently reachable **only from `/library/*`**. Move them into a single shared
|
||||
account menu anchored to the user's name, and make that menu available on every
|
||||
authenticated non-immersive screen.
|
||||
|
||||
## Motivation
|
||||
|
||||
A user sitting on the home screen cannot open Settings or sign out. The bottom
|
||||
nav offers Home / Search / Library only, and the header that hosts those actions
|
||||
belongs to the library layout. The user has to guess that account actions live
|
||||
*inside* Library — an unrelated section — and navigate there first.
|
||||
|
||||
Desktop and mobile also disagree today: desktop shows an unlabeled logout icon
|
||||
with no grouped menu, mobile shows a three-dot overflow with labelled items. The
|
||||
same two actions are found two different ways.
|
||||
|
||||
## Background: verified current state
|
||||
|
||||
1. **The header is not global.** It is defined in
|
||||
[library/+layout.svelte](../../src/routes/library/+layout.svelte). The root
|
||||
layout [+layout.svelte](../../src/routes/+layout.svelte) renders no header at
|
||||
all.
|
||||
|
||||
2. **`routeOwnsLayout`** in
|
||||
[layoutShell.ts](../../src/lib/utils/layoutShell.ts) returns true for
|
||||
`/library`, `/player/`, `/login` — those routes own their own full-height
|
||||
flex column. Everything else renders into the root scroller with the root's
|
||||
`BottomUi` below it.
|
||||
|
||||
3. **Bottom nav is Home / Search / Library only**
|
||||
([BottomNav.svelte](../../src/lib/components/BottomNav.svelte)) — no Settings
|
||||
or account entry.
|
||||
|
||||
4. **Net effect:** on `/`, `/search`, and `/downloads` there is no route to
|
||||
Settings or Sign out.
|
||||
|
||||
5. **Desktop username is inert text** — a `<span>` next to the icons, not a
|
||||
trigger.
|
||||
|
||||
6. **The mobile overflow menu already has the right contents** (Downloads,
|
||||
Settings, divider, Sign out) and the right dismissal behaviour (backdrop
|
||||
click, keyboard handler). **Extract and reuse it rather than rewriting it.**
|
||||
|
||||
7. **`viewMode` is already a persisted store** in
|
||||
[library.ts](../../src/lib/stores/library.ts) (`jellytau-view-mode`,
|
||||
`localStorage`). The Display setting is a second view onto it — **no new
|
||||
state, no migration.**
|
||||
|
||||
## Design
|
||||
|
||||
### `AccountMenu` component (DR-075)
|
||||
|
||||
One component used by both breakpoints. Contents in fixed order:
|
||||
|
||||
```
|
||||
Signed in as <name> ← identity block, not interactive
|
||||
<server host>
|
||||
────────────────────────
|
||||
Downloads
|
||||
Settings
|
||||
Display ← grid/list preference
|
||||
────────────────────────
|
||||
Sign out ← destructive, last, after a divider
|
||||
```
|
||||
|
||||
- **Trigger is the username/avatar**, not a bare three-dot icon. On mobile where
|
||||
horizontal space is tight, the avatar (or initial) alone is acceptable; the
|
||||
name shows inside the open menu regardless.
|
||||
- **Same items, same order, both platforms.**
|
||||
- Preserve the existing dismissal behaviour: click-outside backdrop, `Escape`,
|
||||
and focus return to the trigger on close.
|
||||
- Menu items are real links/buttons — keyboard reachable, correct roles,
|
||||
`aria-expanded` on the trigger.
|
||||
|
||||
"Display" may either navigate to the Settings Display section or expose the
|
||||
grid/list choice inline. Prefer navigating — it keeps one source of truth for
|
||||
preferences and avoids a nested control inside a dropdown.
|
||||
|
||||
### Global chrome (DR-076)
|
||||
|
||||
Make the header — and therefore the account menu — available on `/`, `/search`,
|
||||
and `/downloads`.
|
||||
|
||||
The cleanest route is to lift the header out of the library layout into a shared
|
||||
component rendered by the root layout, with the library layout consuming the
|
||||
same component rather than defining its own. **Do not duplicate the markup into
|
||||
each route.**
|
||||
|
||||
Constraints that must survive the change:
|
||||
|
||||
- `/player/*` and `/login` stay chrome-free.
|
||||
- `/settings` already owns its layout; it needs no account menu (the user is
|
||||
already there), but must not double up on chrome.
|
||||
- The root layout's flex/scroller structure is deliberate — the comments in
|
||||
[layoutShell.ts](../../src/lib/utils/layoutShell.ts) and
|
||||
[+layout.svelte](../../src/routes/+layout.svelte) explain why routes own their
|
||||
own column. Preserve the scroll containment; a regression here reintroduces
|
||||
the "last row hidden behind the nav" bug called out in those comments.
|
||||
- Mini-player and bottom-nav visibility rules (`showGlobalMiniPlayer`,
|
||||
`showBottomNav`) must be unchanged.
|
||||
|
||||
### Display section in Settings (DR-077)
|
||||
|
||||
Add a Display section to [settings/+page.svelte](../../src/routes/settings/+page.svelte)
|
||||
with the grid/list control bound to the existing `viewMode` store via
|
||||
`library.setViewMode(...)`. The page-header toggle in `LibraryGrid` stays — both
|
||||
controls drive the same store, so they stay in sync for free.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Redesigning the Settings page or reorganising its existing sections.
|
||||
- Multi-server / account switching (UR-047) — the identity block displays the
|
||||
active server but offers no switcher.
|
||||
- Changing the bottom nav's three destinations.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] Settings and Sign out are reachable from `/`, `/search`, and `/downloads`
|
||||
without first navigating into Library.
|
||||
- [ ] Desktop and mobile show the same account menu items in the same order.
|
||||
- [ ] The username/avatar opens the menu; it is a real button with
|
||||
`aria-expanded`.
|
||||
- [ ] Sign out is last, after a divider, and still logs out + resets library
|
||||
state + redirects as it does today.
|
||||
- [ ] `/player/*` and `/login` remain chrome-free.
|
||||
- [ ] Settings has a Display section that changes grid/list, and the change is
|
||||
immediately reflected by the library page-header toggle (same store).
|
||||
- [ ] No regression in scroll containment, mini-player visibility, or bottom-nav
|
||||
visibility on any route.
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
|
||||
## Testing
|
||||
|
||||
- Extend the existing `layoutShell` tests: chrome-visibility for `/`, `/search`,
|
||||
`/downloads` (now true) and `/player/*`, `/login` (still false).
|
||||
- `AccountMenu`: renders the documented items in order; trigger toggles
|
||||
`aria-expanded`; `Escape` and backdrop click close it; Sign out invokes the
|
||||
logout handler.
|
||||
- Display setting: writes through to the `viewMode` store and persists.
|
||||
|
||||
New requirement-implementing code needs `TRACES:` comments — see
|
||||
[CLAUDE.md](../../CLAUDE.md). Suggested: `AccountMenu` → `UR-054 | DR-075`,
|
||||
shell/header changes → `UR-054 | DR-076`, Settings Display section →
|
||||
`UR-054, UR-029 | DR-077`.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- Read [ux-flows.md §1.2–1.4](../ux-flows.md) first — behavioural spec; this is
|
||||
the implementation plan.
|
||||
- The layout shell is subtle and the existing comments record real bugs that
|
||||
were fixed there. Read them before restructuring.
|
||||
- Another session may be active in this repo, including in
|
||||
`src/routes/settings/+page.svelte`. Check `git diff` before "repairing"
|
||||
unexpected changes, and expect to coordinate on that file.
|
||||
@@ -1,209 +0,0 @@
|
||||
# Spec: Android audio settings parity (EQ, normalization, gapless)
|
||||
|
||||
**Status:** Proposed
|
||||
**Requirements:** UR-031, UR-032, UR-033, UR-027 → DR-034, DR-035, DR-036, DR-030; IR-004
|
||||
**UX spec:** n/a — no UI change; Settings › Audio already renders these controls
|
||||
**Supersedes / revises:** closes the audio half of the parity gap recorded in [playback-backend-unification.md](playback-backend-unification.md)
|
||||
|
||||
## Summary
|
||||
|
||||
Implement `set_audio_settings` / `audio_settings` on `ExoPlayerBackend` so the
|
||||
equalizer, volume normalization, and gapless playback settings actually take
|
||||
effect on Android. Today the Settings › Audio panel renders these controls on
|
||||
Android and they silently do nothing — `ExoPlayerBackend` is the only backend
|
||||
that does not override the trait's no-op defaults.
|
||||
|
||||
Crossfade is explicitly **not** included; see Out of scope.
|
||||
|
||||
## Motivation
|
||||
|
||||
`PlayerBackend` declares `set_audio_settings` with a default `Ok(())` body.
|
||||
`MpvBackend`, `NullBackend`, and `WebviewAudioBackend` all override it;
|
||||
`ExoPlayerBackend` does not. The settings are persisted, pushed to the backend on
|
||||
every track load, and displayed in the UI — and then dropped on the floor.
|
||||
|
||||
This is the single most user-visible platform divergence in the app: a user who
|
||||
sets a "Rock" EQ preset on Android sees the sliders move and hears no change.
|
||||
|
||||
The backend-unification investigation ruled out fixing this by swapping engines
|
||||
(video cannot be unified; see the sibling spec), so the fix is to implement the
|
||||
trait methods where they are missing.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Band count, centre frequencies, gain range, preset→curve map | Rust (existing) | Already domain-owned in `settings.rs` per [audio-equalizer.md](audio-equalizer.md). Android must consume the same `AudioSettings`, not define its own bands. Duplicating the band layout in Kotlin would be a taxonomy leak of exactly the kind `check:boundary` guards against. |
|
||||
| Mapping `AudioSettings` → Android audio-effect parameters | Rust → JNI boundary | Platform playback detail, the direct analogue of `build_af_filter` in `mpv_backend.rs`. Belongs with the other `set_audio_settings` code. |
|
||||
| Attaching/detaching `Equalizer` and `LoudnessEnhancer` to the ExoPlayer audio session | Kotlin (`JellyTauPlayer.kt`) | Android platform API mechanics; needs the live `audioSessionId`, which only the Kotlin layer holds. |
|
||||
| Normalization preset (Loud/Normal/Quiet) → target gain | Rust (existing) | `VolumeLevel` is domain vocabulary; the same preset must mean the same loudness on every platform. |
|
||||
| Rendering sliders / preset chips | Frontend (existing) | Pure presentation; unchanged by this spec. |
|
||||
|
||||
Borderline row: attaching the effects could arguably be driven entirely from
|
||||
Rust via JNI property calls. It goes to Kotlin because `AudioEffect` construction
|
||||
requires the audio session id and must be re-attached when ExoPlayer rebuilds its
|
||||
audio sink — lifecycle state that lives in `JellyTauPlayer.kt`. Rust still owns
|
||||
*what* the values are; Kotlin owns *when* the effect objects exist.
|
||||
|
||||
## Design
|
||||
|
||||
### Rust — `ExoPlayerBackend` (`src-tauri/src/player/android/mod.rs`)
|
||||
|
||||
Override the two defaulted methods, mirroring the shape of the existing
|
||||
`set_audio_track` JNI call:
|
||||
|
||||
```rust
|
||||
fn set_audio_settings(&mut self, settings: &AudioSettings) -> Result<(), PlayerError> {
|
||||
let s = settings.clone().with_crossfade_clamped().with_equalizer_normalised();
|
||||
// Serialize as JSON — the same pattern load() already uses for subtitles,
|
||||
// avoiding a 6-arg JNI signature that has to change every time a field lands.
|
||||
let json = serde_json::to_string(&s).map_err(|e| PlayerError { message: e.to_string() })?;
|
||||
// Kotlin: fun setAudioSettings(json: String)
|
||||
self.call_player_method_string("setAudioSettings", &json)?;
|
||||
self.shared_state.lock_safe().audio_settings = s;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn audio_settings(&self) -> AudioSettings {
|
||||
self.shared_state.lock_safe().audio_settings.clone()
|
||||
}
|
||||
```
|
||||
|
||||
`ExoPlayerState` gains an `audio_settings: AudioSettings` field. Note
|
||||
`ExoPlayerBackend` currently holds no such state — `position`/`state`/`volume` are
|
||||
all pushed in by JNI callbacks — so this is the first *pull*-side field. That is
|
||||
correct: audio settings are commanded downward, never reported upward.
|
||||
|
||||
### Kotlin — `JellyTauPlayer.kt`
|
||||
|
||||
```kotlin
|
||||
fun setAudioSettings(json: String) {
|
||||
val s = JSONObject(json)
|
||||
applyEqualizer(s.getBoolean("equalizerEnabled"), s.getJSONArray("equalizerBands"))
|
||||
applyNormalization(s.getBoolean("normalizeVolume"), s.getString("volumeLevel"))
|
||||
exoPlayer.pauseAtEndOfMediaItems = !s.getBoolean("gaplessPlayback")
|
||||
}
|
||||
```
|
||||
|
||||
Three independent mechanisms:
|
||||
|
||||
- **Gapless** — nearly free. ExoPlayer is gapless by default for compatible
|
||||
formats; honouring the setting means *disabling* it when the user turns it off,
|
||||
via `pauseAtEndOfMediaItems`. Note this only applies within a loaded playlist;
|
||||
our queue loads one item at a time, so verify behaviour before claiming DR-035
|
||||
on Android (see Testing).
|
||||
- **Equalizer** — `android.media.audiofx.Equalizer` bound to
|
||||
`exoPlayer.audioSessionId`. Android's EQ exposes a device-dependent band count
|
||||
(commonly 5) at fixed centre frequencies, which will **not** match our 10-band
|
||||
ISO layout. Rust owns the canonical 10 bands; Kotlin resamples them onto the
|
||||
device's bands by nearest-centre-frequency interpolation. Gains are in
|
||||
millibels (`setBandLevel` takes mB, we store dB → ×100), clamped to the
|
||||
device's reported `getBandLevelRange()`.
|
||||
- **Normalization** — `android.media.audiofx.LoudnessEnhancer`, also bound to the
|
||||
audio session, `setTargetGain(mB)` derived from `VolumeLevel`. This is a gain
|
||||
booster, not a true EBU R128 normalizer like MPV's `dynaudnorm`; parity is
|
||||
approximate and should be documented as such rather than overclaimed.
|
||||
|
||||
Lifecycle: build the effects lazily on first use, release them in `release()`,
|
||||
and re-attach on `onAudioSessionIdChanged` — ExoPlayer can rebuild its audio sink
|
||||
(e.g. on a format change), which invalidates effects bound to the old session.
|
||||
|
||||
### Make the silent-failure mode impossible
|
||||
|
||||
The trait's default is the root cause of this whole class of bug:
|
||||
|
||||
```rust
|
||||
// backend.rs:85 — reports success while doing nothing
|
||||
fn set_audio_settings(&mut self, _settings: &AudioSettings) -> Result<(), PlayerError> {
|
||||
Ok(())
|
||||
}
|
||||
```
|
||||
|
||||
Android inherits this, so every EQ/normalization change on Android returns `Ok`
|
||||
and silently does nothing — the UI ships and has no effect, with no error anywhere.
|
||||
|
||||
Once `ExoPlayerBackend` implements the methods, **change the trait default to
|
||||
`Err(PlayerError::not_implemented())`**, matching how `set_audio_track` /
|
||||
`set_subtitle_track` already behave. Any future backend that forgets to implement
|
||||
audio settings then fails loudly instead of lying.
|
||||
|
||||
Check the call sites before flipping it: `NullBackend` overrides both methods, so
|
||||
the graceful-degradation path is unaffected, but confirm nothing treats a
|
||||
`set_audio_settings` error as fatal to playback.
|
||||
|
||||
### Re-application on track load
|
||||
|
||||
`PlayerController` already re-pushes `AudioSettings` per track on the platforms
|
||||
that implement it; the Android path inherits that for free once the trait methods
|
||||
exist. No controller change.
|
||||
|
||||
### 🔴 Threading note
|
||||
|
||||
`setAudioSettings` is invoked from Rust on whatever thread the command lands on.
|
||||
`AudioEffect` construction must not happen on the ExoPlayer application thread
|
||||
from inside a player callback — that is the re-entrancy hazard CLAUDE.md warns
|
||||
about, and the same shape as the `AutoplayDecision` deadlock. Post the work to
|
||||
the player's handler rather than doing it inline in a listener.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **Crossfade (UR-031 / DR-034).** Not implemented on *any* platform today, and
|
||||
architecturally blocked on MPV (single-stream audio chain; `acrossfade` needs
|
||||
two inputs). Implementing it on Android alone would invert the parity gap. It
|
||||
needs its own spec and probably two player instances.
|
||||
- True EBU R128 normalization. `LoudnessEnhancer` is a gain stage; matching
|
||||
`dynaudnorm` exactly is out of reach without a custom `AudioProcessor`.
|
||||
- Windows audio settings — see [windows-native-audio-backend.md](windows-native-audio-backend.md).
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] `ExoPlayerBackend` overrides `set_audio_settings` and `audio_settings`.
|
||||
- [ ] EQ preset change on Android audibly changes playback; setting persists across track changes and app restart.
|
||||
- [ ] Normalization toggle audibly changes level; the three presets are ordered Loud > Normal > Quiet.
|
||||
- [ ] Disabling gapless produces a gap between consecutive tracks; enabling it does not.
|
||||
- [ ] Effects are released on `release()` and survive an audio-session rebuild.
|
||||
- [ ] `requirements.md` parity matrix updated: EQ and normalization ✅ Android.
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
- [ ] `cargo fmt` clean, `cargo clippy` clean, `bun run test:rust` passes.
|
||||
- [ ] `bun run check:boundary` passes.
|
||||
- [ ] New requirement-implementing code carries `// TRACES:` comments.
|
||||
- [ ] `bindings.ts` regenerated if Rust types changed.
|
||||
|
||||
## Testing
|
||||
|
||||
**Rust** (`cargo test`): `set_audio_settings` stores the sanitized settings and
|
||||
`audio_settings()` returns them — assert clamping/normalisation is applied
|
||||
(crossfade clamped to 12s, band vector normalised to `EQ_BANDS.len()`). The JNI
|
||||
call itself is not unit-testable; extract the JSON serialization into a pure
|
||||
function and test that its shape matches what the Kotlin parser expects. That
|
||||
serialization contract is the part most likely to silently break.
|
||||
|
||||
**Kotlin**: the band-resampling function (10 canonical bands → N device bands) is
|
||||
pure arithmetic — extract it and unit-test it, including the degenerate cases of
|
||||
a 5-band device and a device reporting 10 bands.
|
||||
|
||||
**Manual, on device** (these are the ones that actually prove it):
|
||||
1. Set Bass Boost, play a track, confirm audible change.
|
||||
2. Toggle normalization mid-track; confirm level change without a playback stall.
|
||||
3. Queue two gapless-encoded tracks, toggle the setting, confirm the gap appears/disappears.
|
||||
4. Force a format change (44.1kHz → 48kHz track) and confirm the EQ still applies afterwards — this exercises the session-rebuild re-attach.
|
||||
|
||||
## TRACES
|
||||
|
||||
- `ExoPlayerBackend::set_audio_settings` → `// TRACES: UR-027, UR-032, UR-033 | DR-030, DR-035, DR-036`
|
||||
- Kotlin `setAudioSettings` / `applyEqualizer` / `applyNormalization` → same IDs
|
||||
- Band-resampling helper + its tests → `DR-030 | UT-xxx`
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- Read [audio-equalizer.md](audio-equalizer.md) first — it defines the canonical
|
||||
band layout and the preset→curve rule this spec consumes. Do not redefine bands
|
||||
in Kotlin.
|
||||
- Android source edits go in `src-tauri/android/src` (canonical tree), then run
|
||||
`scripts/sync-android-sources.sh`. Never edit the `gen/` tree.
|
||||
- There is a **stale duplicate** `JellyTauPlayer.kt` (285 lines) at
|
||||
`src-tauri/android/app/src/main/java/com/dtourolle/jellytau/player/` alongside
|
||||
the real 1103-line file at `src-tauri/android/src/main/java/...`. Edit the
|
||||
latter. Consider deleting the former as a separate change.
|
||||
- A parallel Claude session may be active — `git diff` before "repairing"
|
||||
unexpected changes.
|
||||
@@ -1,306 +0,0 @@
|
||||
# Spec: Android native video — transparent-webview spike
|
||||
|
||||
**Status:** Spike succeeded (2026-08-11); shipped behind `experimentalNativeVideo`,
|
||||
default off. Flipping that default shipped **audio with no picture** and was
|
||||
reverted (DR-172). Three defects behind that have since been fixed — DR-182
|
||||
(nothing on the native path could lift the poster overlay), DR-183 (the JS
|
||||
bridges raced the page load), DR-184 (the SurfaceView was never detached).
|
||||
Branch `fix/android-native-video-visible`. **The default stays off until the
|
||||
device criteria below are green.**
|
||||
|
||||
**The spike's central question is answered: yes.** A `SurfaceView` *can* be
|
||||
composited behind a transparent Tauri WebView on Android. Nothing upstream
|
||||
blocked it and nothing upstream demonstrated it — this is, as far as the issue
|
||||
trackers show, the first working instance. The remaining flag is about test
|
||||
coverage and the unverified cases below, not about viability.
|
||||
**Requirements:** IR-004, UR-003, UR-004, UR-041 → DR-001, DR-004, DR-150, DR-151, DR-152
|
||||
**Note:** the original draft cited DR-023/DR-024 here. Those are the *subtitle*
|
||||
and *audio-track selection UI* requirements — unrelated to this work. The IDs
|
||||
actually implemented are DR-150 (native rendering behind the flag), DR-151 (the
|
||||
severed SurfaceView attach chain) and DR-152 (capabilities reported by Rust).
|
||||
**UX spec:** n/a — no intended visual change; the video surface must land exactly where the `<video>` element is today
|
||||
**Supersedes / revises:** acts on finding 2 of [playback-backend-unification.md](playback-backend-unification.md)
|
||||
|
||||
## Summary
|
||||
|
||||
Test whether ExoPlayer's existing `SurfaceView` video path can be composited
|
||||
behind a transparent Tauri WebView on Android. If it works, Android regains
|
||||
hardware video decoding (MediaCodec) and libass-quality ASS/SSA subtitles, both
|
||||
of which the current webview path lacks. If it does not, we document why and
|
||||
delete the dead code.
|
||||
|
||||
This is a **spike**, not a feature commitment. The deliverable is a yes/no answer
|
||||
with evidence, plus either a working path behind a flag or a removal.
|
||||
|
||||
## Motivation
|
||||
|
||||
`createAdapter()` hardcodes `const effectiveKind = "html5"` and does
|
||||
`void backendKind`, discarding the `use_html5_element` value Rust computes in
|
||||
`get_player_status`. As a result:
|
||||
|
||||
- `NativePlayerAdapter` is dead code.
|
||||
- `JellyTauPlayer.kt`'s `getOrCreateSurfaceView()` — which already calls
|
||||
`setZOrderMediaOverlay(false)` and wires `setVideoSurfaceHolder` — is
|
||||
unreachable.
|
||||
- Android video decodes in the WebView instead of via MediaCodec, despite
|
||||
`CodecDetector.kt` going to the trouble of reporting hardware codec
|
||||
capabilities back to Rust for DeviceProfile generation.
|
||||
|
||||
The code comment in `nativeAdapter.ts:11-14` justifies this by citing
|
||||
tauri#10152 as an upstream blocker. **That justification is stale.**
|
||||
|
||||
### Why the blocker no longer holds
|
||||
|
||||
- tauri#10152 is open but **dead since 2024-07-01**, and it is a *feature
|
||||
request* ("Support transparent webviews on mobile"), not a bug report about
|
||||
compositing.
|
||||
- The capability shipped in tauri commit `27d01834` (2024-09-02) — a clippy
|
||||
cleanup that moved `transparent()` out of the desktop-gated impl block, fencing
|
||||
only the tao call behind `#[cfg(desktop)]`. Because it landed as unrelated
|
||||
cleanup, nobody closed the issue.
|
||||
- The black/white-screen reports (tauri#8381, tauri#9408) were a real but
|
||||
*different* bug: a broken JNI signature for `setBackgroundColor`, fixed in
|
||||
**wry 0.39.4** (PR #1237). We ship wry 0.55.x.
|
||||
- Current wry calls `setBackgroundColor(0)` unconditionally on Android when
|
||||
transparency is requested.
|
||||
|
||||
### The honest caveat
|
||||
|
||||
**Nobody has demonstrated SurfaceView-behind-WebView on Tauri Android.** A search
|
||||
of both `tauri-apps/tauri` and `tauri-apps/wry` issues for `surfaceview` returns
|
||||
zero results, and the one native-video Tauri plugin
|
||||
(`YeonV/tauri-plugin-videoplayer`) sidesteps compositing by launching a separate
|
||||
fullscreen Activity. Nothing upstream blocks this; nothing upstream proves it.
|
||||
Hence: spike, not feature.
|
||||
|
||||
Note this is the *Android* question only. The equivalent Linux compositing
|
||||
problem is maintainer-declared unfixable and is **not** in scope — see the
|
||||
unification spec.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Which video backend this platform uses | Rust (existing) | `get_player_status` already computes `use_html5_element`. The frontend must *consume* it, not decide it. Restoring that is the point of the spike. |
|
||||
| Surface creation, z-ordering, `setVideoSurfaceHolder` lifecycle | Kotlin | Android platform mechanics; already written in `JellyTauPlayer.kt`. |
|
||||
| Seek/audio-track *strategy* | Rust (existing) | Already returned by `player_seek_video` / `player_switch_audio_track`; `NativePlayerAdapter` executes the chosen primitive. Unchanged — this is exactly what the `PlayerAdapter` contract was built for. |
|
||||
| Positioning the surface under the video viewport | Frontend | Pure presentation/layout. **This is the risk area** — see Design. |
|
||||
|
||||
## Design
|
||||
|
||||
### Phase 1 — prove compositing (no app changes)
|
||||
|
||||
Before touching the adapter factory, verify the primitive works at all:
|
||||
|
||||
1. Set `"transparent": true` in `tauri.conf.json` for the Android build, plus
|
||||
`html, body { background: transparent; }`.
|
||||
2. Confirm the WebView is genuinely transparent (a native view behind it is
|
||||
visible) and that the app does not regress to a black/white screen.
|
||||
|
||||
If this fails, stop — everything downstream is moot, and the finding is that
|
||||
Tauri Android transparency is still broken in practice despite the shipped fix.
|
||||
|
||||
### Phase 2 — un-hardcode the factory
|
||||
|
||||
```ts
|
||||
// src/lib/player/adapters/index.ts
|
||||
export function createAdapter({ backendKind, host, bridge }: CreateAdapterArgs): PlayerAdapter {
|
||||
return backendKind === "native"
|
||||
? new NativePlayerAdapter(host)
|
||||
: new Html5PlayerAdapter(host, bridge);
|
||||
}
|
||||
```
|
||||
|
||||
`backendKind` comes from `get_player_status` (`VideoBackend::Native` on Android).
|
||||
Gate behind a setting — `experimentalNativeVideo`, default **off** — so a broken
|
||||
spike cannot ship as a regression. Rust already owns this decision; the flag only
|
||||
suppresses it.
|
||||
|
||||
**Also in scope: remove the user-agent sniffing in
|
||||
`src/lib/services/webviewAudio.ts:30-41`.** It re-derives which audio backend the
|
||||
platform has from `navigator.userAgent` ("matching the Rust cfg gate", per its own
|
||||
comment) — the frontend deciding a backend fact it should be told. Same root cause
|
||||
as the hardcode above, same fix: consume the value Rust already computes. Fold it
|
||||
in here rather than leaving a second, subtler copy of the bug behind. If
|
||||
`get_player_status` does not currently expose enough to cover the audio case, add
|
||||
the field — that is backend work, and correct.
|
||||
|
||||
### Implementation findings (2026-08-11)
|
||||
|
||||
Two blockers existed that this spec did not anticipate. Both were in code the
|
||||
spec assumed was merely *unreachable*; it was also *broken*.
|
||||
|
||||
**1. The Kotlin attach chain was severed.** `JellyTauPlayer.setActivity()` had
|
||||
**zero callers** anywhere in the tree. `currentActivity` was therefore always
|
||||
null, so `autoAttachSurface()` logged "Cannot attach surface - no Activity
|
||||
reference" and returned. The `SurfaceView` was created and wired to ExoPlayer but
|
||||
never added to the view hierarchy — video would have decoded to a surface that
|
||||
was never on screen, *regardless* of webview transparency. Fixed by calling
|
||||
`JellyTauPlayer.setActivity(this)` from `MainActivity.onCreate`.
|
||||
|
||||
Note the knock-on: `PictureInPictureManager.canEnterPip()` gates on
|
||||
`VideoOverlayManager.isVideoSurfaceAttached()`, which was permanently false. PiP
|
||||
on the video path was dead for the same reason.
|
||||
|
||||
**2. `createAdapter()` was not the real gate.** It is never called by production
|
||||
code — `VideoPlayer.svelte` constructs `Html5PlayerAdapter` directly. The actual
|
||||
override was `VideoPlayer.svelte`'s INTERIM block, which read Rust's
|
||||
`useHtml5Element`, forced it to `true`, and called `playerStop()` to kill the
|
||||
native backend `player_play_item` had just started. Both sites are now fixed;
|
||||
`VideoPlayer.svelte` routes through `createAdapter()` so there is one gate.
|
||||
|
||||
**Transparency needs two independent layers cleared,** not one. The spec's
|
||||
Phase 1 named only `html, body`. Clearing just the page leaves the WebView
|
||||
widget's own background opaque, which is a black screen with audio — the exact
|
||||
symptom the INTERIM comment described as "native surface not visible". Both are
|
||||
now toggled together by `$lib/utils/videoSurface.ts`:
|
||||
|
||||
| Layer | Cleared by | Reachable from |
|
||||
|-------|-----------|----------------|
|
||||
| WebView widget background + window drawable | `AndroidVideoSurface.setTransparent()` (MainActivity) | Kotlin only |
|
||||
| `html`/`body` + app-shell `--color-background` | `data-native-video` attribute → app.css | CSS only |
|
||||
|
||||
Transparency is scoped to `tauri.android.conf.json` rather than the base config:
|
||||
a transparent window on Linux is a regression, since nothing renders behind it.
|
||||
It is also toggled per-session rather than set once — a permanently transparent
|
||||
window shows the launcher through the rest of the app.
|
||||
|
||||
### Phase 3 — surface positioning
|
||||
|
||||
The hard part, and where this most likely fails. The webview's `<video>` element
|
||||
occupies a laid-out box; the `SurfaceView` must be positioned to match it, and
|
||||
kept matched through scroll, rotation, and mini-player transitions.
|
||||
|
||||
Approach: the video view reports its `getBoundingClientRect()` to Rust, which
|
||||
forwards the rect to Kotlin to position the `SurfaceView`. This is the same
|
||||
"faking it" technique the ecosystem uses on desktop — acceptable here *only if*
|
||||
the video is effectively fullscreen on Android, which it is in the player route.
|
||||
|
||||
**Explicit failure criterion**: if the surface cannot be kept aligned during
|
||||
rotation or the mini-player transition without visible artefacts, the spike fails
|
||||
and we keep HTML5. Do not ship a janky native path for a codec win.
|
||||
|
||||
**Update: no rect plumbing was needed.** The premise — that the surface must be
|
||||
positioned to match a laid-out `<video>` box — does not hold on the player route,
|
||||
where video is fullscreen. `VideoOverlayManager` adds the SurfaceView at index 0
|
||||
of `android.R.id.content` with `MATCH_PARENT`, and `fitSurfaceToScreen()`
|
||||
(`JellyTauPlayer.kt`) already letterboxes/pillarboxes to the real video aspect
|
||||
ratio and re-centres via a `Gravity.CENTER` `FrameLayout.LayoutParams`. Rotation
|
||||
is handled by an `OnLayoutChangeListener` that re-fits on any bounds change. The
|
||||
frontend's native branch is a bare `flex-1` box, so there is no rect to report
|
||||
and nothing to keep in sync.
|
||||
|
||||
Fullscreen playback is confirmed working on device. But this reasoning rests
|
||||
entirely on the fullscreen assumption, so **the mini-player transition is the
|
||||
known gap** — it is the one case where the surface is *not* fullscreen, and
|
||||
therefore the one case where the "no rect plumbing needed" conclusion could
|
||||
still turn out to be wrong. If artefacts appear there, the fix is the rect
|
||||
reporting this section originally proposed, scoped to that transition alone.
|
||||
|
||||
### A trap for the next implementer
|
||||
|
||||
There is a **stale duplicate player** at
|
||||
`src-tauri/android/app/src/main/java/com/dtourolle/jellytau/player/JellyTauPlayer.kt`
|
||||
(only commit: `cfddc1e` "First working POC"). No `sourceSets` entry points at it,
|
||||
so it is not compiled — but edits made there silently do nothing. The canonical
|
||||
tree is `src-tauri/android/src`, synced into `gen/` by
|
||||
`scripts/sync-android-sources.sh`.
|
||||
|
||||
### What we gain if it works
|
||||
|
||||
- **Hardware decode via MediaCodec** — `CodecDetector.kt` already reports
|
||||
capabilities; the DeviceProfile would finally match what actually plays.
|
||||
- **ASS/SSA subtitles** are *not* automatic. ExoPlayer cannot render them; that
|
||||
would require libmpv, which is a separate and much larger decision (see the
|
||||
unification spec's engine comparison). Scope this spike to hardware decode
|
||||
only, and do not claim subtitle improvements from it.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Linux native video. Maintainer-declared unfixable on WebKitGTK/Wayland.
|
||||
- Replacing ExoPlayer with libmpv on Android.
|
||||
- Windows native video.
|
||||
- Removing the HTML5 path. It stays as the default and the fallback.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
The spike is **complete** when one of these is true:
|
||||
|
||||
**Success path**
|
||||
- [x] Transparent WebView confirmed working on a physical device (reported by the maintainer; the config that enables it is now committed in `tauri.android.conf.json`).
|
||||
- [x] `experimentalNativeVideo` off → behaviour byte-identical to today. Guarded by `adapterSelection.test.ts`, which asserts the flag-off case forces HTML5 even when Rust reports native.
|
||||
- [x] `webviewAudio.ts` no longer inspects `navigator.userAgent`; the platform's audio backend is read from Rust (`player_get_capabilities` → `usesWebviewAudio`).
|
||||
- [x] `experimentalNativeVideo` on → video plays via ExoPlayer, correctly positioned, on a physical device (2026-08-11). The surface reaches the hierarchy and is visible through the transparent WebView — the whole point of the spike.
|
||||
- [x] The poster/title card comes down on the native path. It never could: every `markMediaReady()` call site is a `<video>` element event and the native branch renders no element, so an opaque `bg-black` overlay covered the ExoPlayer surface for the whole session. See DR-182; guarded by `mediaReady.test.ts` (UT-184) and `VideoPlayer.nativeReveal.test.ts` (UT-185), the latter written failing first.
|
||||
- [x] The `AndroidVideoSurface` bridge is installed before the page that calls it loads, via `WryActivity.onWebViewCreate` instead of a 500 ms tree walk, and a missing bridge now logs an error instead of no-oping. See DR-183.
|
||||
- [x] The SurfaceView is detached when video stops, instead of accumulating one leaked view per native video. See DR-184.
|
||||
- [ ] Seek, audio-track switch and subtitle selection exercised through `NativePlayerAdapter`. Playback is confirmed; these individual controls are not yet each verified on the native path.
|
||||
- [ ] No artefacts on rotation, background/foreground, or **mini-player transition** — the last is the one case the fullscreen assumption does not cover, so it is the likeliest place to find a problem.
|
||||
- [ ] `adb shell dumpsys media.metrics` (or logcat) confirms a hardware decoder is in use. Plausible but unmeasured — do not claim the MediaCodec win until this is read.
|
||||
- [ ] Measured battery/thermal or CPU improvement over the HTML5 path on the same clip.
|
||||
|
||||
**Failure path**
|
||||
- [ ] The blocking behaviour is documented in this spec with evidence.
|
||||
- [ ] `NativePlayerAdapter` and the unreachable `SurfaceView` code are deleted, or explicitly retained with a *correct* comment.
|
||||
- [ ] `nativeAdapter.ts:11-14` no longer cites tauri#10152.
|
||||
|
||||
Either way:
|
||||
- [x] `bun run check` (0 errors), `bun run test` (997 passed), `bun run check:boundary` pass.
|
||||
- [x] `cargo fmt` / `cargo clippy` clean (no new warnings); `cargo test` passes (603 lib + 7 doc).
|
||||
|
||||
### Why the 2026-08-11 verification and DR-172 do not contradict each other
|
||||
|
||||
The spike was reported working on device; the same path then shipped as audio
|
||||
with no picture. Both are consistent with DR-182: the poster overlay is drawn
|
||||
only while `isMediaReady` is false, and the native path has no way to set it, so
|
||||
what the surface shows depends entirely on **whether that overlay is on screen**
|
||||
— not on whether compositing works. Any run that reached the player through a
|
||||
path leaving `isMediaReady` already true (a handoff return, a re-render, a
|
||||
session that had previously played on the HTML5 path) shows video; a cold start
|
||||
into the native path never does. That is also why DR-172 read the symptom as a
|
||||
compositing failure: on screen the two are identical, and the one piece of
|
||||
evidence separating them — `WebView transparent = true` never being logged —
|
||||
points at DR-183 rather than at the compositing itself.
|
||||
|
||||
**This reasoning is not yet device-confirmed.** It explains the reports and is
|
||||
backed by the code, but the criteria above are what settle it.
|
||||
|
||||
> Note: this environment has no host WebKitGTK dev packages, no Android SDK and
|
||||
> no `bun`, so all of the above were run inside the CI builder image
|
||||
> (`gitea.tourolle.paris/dtourolle/jellytau-builder:latest`). On Fedora the bind
|
||||
> mount needs `:z` for SELinux, and `scripts/build-android.sh` hardcodes
|
||||
> `ANDROID_HOME="$HOME/Android/Sdk"`, so the image's SDK at `/opt/android-sdk`
|
||||
> must be symlinked there rather than passed by env var.
|
||||
|
||||
## Testing
|
||||
|
||||
Adapter-selection logic is pure and testable without a device: assert
|
||||
`createAdapter` returns `NativePlayerAdapter` for `backendKind: "native"` with
|
||||
the flag on, and `Html5PlayerAdapter` in every other combination — including that
|
||||
the flag off forces HTML5 even when Rust says native. That last case is the
|
||||
regression guard.
|
||||
|
||||
Everything else is manual on-device; there is no meaningful way to unit-test
|
||||
surface compositing. Test on at least two devices — compositing behaviour varies
|
||||
by OEM and Android version.
|
||||
|
||||
Per CLAUDE.md, if the spike turns into a bug fix (e.g. seek breaks under the
|
||||
native adapter), write the failing test first.
|
||||
|
||||
## TRACES
|
||||
|
||||
- `createAdapter` → `// TRACES: UR-003, UR-004 | DR-004, DR-150 | UT-149`
|
||||
- Adapter-selection tests → `UT-xxx`
|
||||
- No new requirement IDs; this spike either satisfies existing IR-004 expectations or documents why it cannot.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- **Do not skip Phase 1.** If transparency does not work, phases 2 and 3 are
|
||||
wasted effort.
|
||||
- `VideoPlayer.svelte` has a documented hazard: no lifecycle calls after an
|
||||
`await` in `onMount` — it flips to HTML5 mode and breaks Android seek. The
|
||||
adapter swap touches exactly this code path.
|
||||
- tauri-specta tagged responses keep Rust field names (`new_url`, not `newUrl`).
|
||||
- Android source edits go in `src-tauri/android/src`, then run
|
||||
`scripts/sync-android-sources.sh`.
|
||||
- A parallel Claude session may be active — `git diff` first.
|
||||
@@ -1,176 +0,0 @@
|
||||
# Spec: Audio equalizer
|
||||
|
||||
**Status:** Accepted
|
||||
**Requirements:** UR-027 → DR-030 (EQ UI), IR-020 (MPV EQ integration).
|
||||
**UX spec:** n/a (extends the Settings › Audio section, ux-flows §8.1 instant-apply).
|
||||
**Supersedes / revises:** —
|
||||
**Revised by:** [android-audio-settings-parity.md](android-audio-settings-parity.md) — lifts the "Android is a no-op" limitation below.
|
||||
|
||||
## Summary
|
||||
|
||||
Add a graphic audio equalizer to playback. Users pick a preset (Flat, Rock,
|
||||
Pop, Jazz, Classical, Bass Boost, Treble Boost, Vocal) or set custom per-band
|
||||
gains, from a new block in Settings › Audio. On Linux the gains apply live via
|
||||
MPV's audio-filter chain; the settings persist and re-apply on the next track
|
||||
and at startup, exactly like crossfade/gapless/normalize do today. Android is a
|
||||
no-op for now (documented parity gap, same as those three features).
|
||||
|
||||
## Motivation
|
||||
|
||||
UR-027 is one of the few still-unbuilt audio features. The audio-settings
|
||||
pipeline it needs already exists — `AudioSettings` + `set_audio_settings` on the
|
||||
`PlayerBackend` trait, the `player_set_audio_settings` command, and the Settings
|
||||
› Audio UI with instant-apply. Crossfade, gapless, and volume normalization all
|
||||
ride that pipeline. The equalizer is the same shape: N more fields on
|
||||
`AudioSettings`, an `af` filter on the MPV backend, one more block in the
|
||||
settings panel. No new command, no new state machine.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| EQ band count, centre frequencies, gain range/clamping | Rust | Domain of the audio engine; the bands must match what the MPV filter expects. Changing the DSP must not require a frontend change. |
|
||||
| Preset name → per-band gain curve | Rust | A preset *is* a domain gain curve, not a label. It changes with the audio engine's band layout, never with the UI. Placing it in the frontend would be the scoped-search taxonomy mistake again (values that look like config but are domain data). |
|
||||
| Translating gains → MPV `af` filter string | Rust | Platform playback detail; lives with the other `set_audio_settings` filter code in `mpv_backend.rs`. |
|
||||
| Persisting the chosen settings, re-pushing on load | Rust/existing | Same path crossfade/etc. already use; the controller re-applies `AudioSettings` per track. |
|
||||
| Rendering band sliders, the preset chips, live readouts | Frontend | Pure presentation; changes only if the settings UI is redesigned. |
|
||||
| Which preset chip is highlighted; instant-apply on change | Frontend | Presentation/input handling (UR-057), the same as the normalize preset picker. |
|
||||
|
||||
Tie-breaker note: the preset→curve map is the one tempting boundary leak. It goes
|
||||
in Rust because a preset is a set of band gains defined *by the band layout*,
|
||||
which is an engine property. The frontend only ever names a preset and renders
|
||||
the resulting gains; it never defines them.
|
||||
|
||||
## Design
|
||||
|
||||
### `AudioSettings` (Rust, `settings.rs`)
|
||||
|
||||
Add two fields (both `#[serde(rename_all = "camelCase")]` via the existing
|
||||
struct attribute):
|
||||
|
||||
```rust
|
||||
/// Equalizer enabled. When false, no `af` EQ filter is applied.
|
||||
pub equalizer_enabled: bool,
|
||||
/// Per-band gains in dB, one per FIXED band (see EQ_BANDS). Length is
|
||||
/// validated/normalised to EQ_BANDS.len(); clamped to [-12, +12] dB.
|
||||
pub equalizer_bands: Vec<f32>,
|
||||
```
|
||||
|
||||
Fixed 10-band ISO layout (domain constant in `settings.rs`):
|
||||
|
||||
```rust
|
||||
pub const EQ_BANDS: [f32; 10] =
|
||||
[31.0, 62.0, 125.0, 250.0, 500.0, 1000.0, 2000.0, 4000.0, 8000.0, 16000.0];
|
||||
pub const EQ_GAIN_MIN: f32 = -12.0;
|
||||
pub const EQ_GAIN_MAX: f32 = 12.0;
|
||||
```
|
||||
|
||||
- `Default`: `equalizer_enabled: false`, `equalizer_bands: vec![0.0; 10]` (flat).
|
||||
- New `with_equalizer_normalised(self)` clamps each gain to `[EQ_GAIN_MIN,
|
||||
EQ_GAIN_MAX]` and pads/truncates the vec to 10 bands. Applied in the command
|
||||
alongside `with_crossfade_clamped` (add that call too — it's currently missing).
|
||||
- Backward compat: both fields `#[serde(default)]` so old persisted JSON loads.
|
||||
|
||||
### Presets (Rust, `settings.rs`)
|
||||
|
||||
```rust
|
||||
#[derive(specta::Type, Serialize, Deserialize, Clone, Copy, PartialEq)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub enum EqPreset { Flat, Rock, Pop, Jazz, Classical, BassBoost, TrebleBoost, Vocal }
|
||||
|
||||
impl EqPreset {
|
||||
/// The 10-band gain curve (dB) for this preset.
|
||||
pub fn gains(&self) -> [f32; 10] { /* table */ }
|
||||
}
|
||||
```
|
||||
|
||||
Preset selection is a *frontend* convenience: tapping a chip sets
|
||||
`equalizer_bands = preset.gains()` and pushes settings. The curve tables live in
|
||||
Rust; the frontend reads them via a tiny `player_get_eq_presets` command
|
||||
returning `Vec<(EqPreset, Vec<f32>)>` (or a map), so the frontend never encodes
|
||||
the numbers. (If exposing the whole table is awkward through specta, expose
|
||||
`player_eq_preset_gains(preset) -> Vec<f32>` instead — pick at implement time.)
|
||||
|
||||
### MPV application (Rust, `mpv_backend.rs::set_audio_settings`)
|
||||
|
||||
Build an `equalizer` / `anequalizer` filter from the bands and set the `af`
|
||||
property. When `equalizer_enabled` is false or all gains are 0, clear the EQ
|
||||
filter (leave any other `af` entries intact). Use `af add`/`af remove` or a
|
||||
rebuilt `af` string; keep it isolated so it doesn't stomp a future crossfade
|
||||
filter. Errors map to `PlayerError` like the gapless code.
|
||||
|
||||
### No new persistence table
|
||||
|
||||
`AudioSettings` is already round-tripped by the frontend settings store and
|
||||
re-pushed via `player_set_audio_settings` on change and on load. The two new
|
||||
fields ride along. `NullBackend`/Android inherit the trait default (no-op).
|
||||
|
||||
### Wire summary
|
||||
|
||||
- Command names unchanged: `player_set_audio_settings`,
|
||||
`player_get_audio_settings` (now carry the EQ fields).
|
||||
- New (optional) read-only command for preset curves — kebab n/a (it's a
|
||||
command): `player_get_eq_presets` (or `player_eq_preset_gains`).
|
||||
- Regenerate `bindings.ts` from the Rust types; never hand-edit.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Android/ExoPlayer EQ (parity gap tracked with crossfade/gapless/normalize).
|
||||
**Now specified in [android-audio-settings-parity.md](android-audio-settings-parity.md)**,
|
||||
which implements `set_audio_settings` on `ExoPlayerBackend`. The canonical band
|
||||
layout and preset→curve map defined here remain authoritative; the Android side
|
||||
resamples those bands onto the device equalizer rather than defining its own.
|
||||
- Per-track or per-library EQ profiles — one global profile only.
|
||||
- Automatic loudness/room correction; only manual bands + presets.
|
||||
- Changing the crossfade/normalize TODOs in `set_audio_settings` beyond wiring
|
||||
the missing `with_crossfade_clamped` call.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] Settings › Audio has an Equalizer block: enable toggle, preset chips, 10
|
||||
band sliders with live dB readouts, instant-apply (no Save button).
|
||||
- [ ] Choosing a preset sets the bands from the Rust-defined curve; editing a
|
||||
band switches the highlighted preset to "Custom" (frontend-only label).
|
||||
- [ ] Gains clamp to [-12, +12] dB; the band vector always normalises to 10.
|
||||
- [ ] On Linux, enabling EQ audibly changes output and persists across tracks
|
||||
and app restart; disabling clears the filter without affecting other audio.
|
||||
- [ ] Old persisted settings (no EQ fields) load without error, defaulting flat.
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
- [ ] `cargo fmt` clean, `cargo clippy` clean, `bun run test:rust` passes.
|
||||
- [ ] `bun run check:boundary` passes (no preset curve numbers in the frontend).
|
||||
- [ ] New requirement-implementing code carries `// TRACES:` comments.
|
||||
- [ ] `bindings.ts` regenerated.
|
||||
|
||||
## Testing
|
||||
|
||||
- Rust (`settings.rs`): default is flat + disabled; `with_equalizer_normalised`
|
||||
clamps out-of-range gains and pads/truncates band length; serialization
|
||||
round-trips the camelCase fields; backward-compat load of pre-EQ JSON; each
|
||||
preset returns a 10-length curve; Flat is all zeros.
|
||||
- Rust IPC param naming for any new command (camelCase rule per CLAUDE.md).
|
||||
- Frontend (`settings` page or an extracted helper): selecting a preset sets the
|
||||
expected band array; editing a band flips the label to Custom; enable toggle
|
||||
gates the sliders. Keep DSP untested on the frontend (it's Rust's).
|
||||
|
||||
## TRACES
|
||||
|
||||
- `AudioSettings` EQ fields + normalise + presets: `UR-027 | DR-030` (+ unit tests)
|
||||
- MPV EQ filter application: `UR-027 | IR-020`
|
||||
- Settings EQ UI block: `UR-027 | DR-030`
|
||||
- Preset-curve command: `UR-027 | DR-030`
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- A parallel Claude session is active in this repo (it has touched
|
||||
`tauri.conf.json`, `Dockerfile`, `package.json`, home components, and added
|
||||
build scripts, and the Rust build is currently broken by its
|
||||
`tauri.conf.json` bundle-target change). `git diff` before "repairing"
|
||||
anything you didn't write; keep EQ changes isolated to `settings.rs`,
|
||||
`mpv_backend.rs`, `backend.rs` (trait default already covers it),
|
||||
`commands/player/settings.rs`, and the settings page.
|
||||
- Mirror the volume-normalization block in the settings page for the toggle +
|
||||
preset-picker pattern; mirror the gapless code in `set_audio_settings` for the
|
||||
MPV property handling.
|
||||
- Confirm the exact MPV filter name available in the linked libmpv
|
||||
(`equalizer` vs `anequalizer`/`superequalizer`) before committing the filter
|
||||
string; gate cleanly if unavailable.
|
||||
@@ -1,215 +0,0 @@
|
||||
# Spec: Harden the frontend boundary tripwire
|
||||
|
||||
**Status:** Implemented
|
||||
**Requirements:** DR-094
|
||||
**UX spec:** n/a — developer tooling.
|
||||
**Supersedes / revises:** revises the detection rule in
|
||||
[scripts/check-frontend-boundary.sh](../../scripts/check-frontend-boundary.sh);
|
||||
the boundary *policy* in [scoped-search-boundary.md](scoped-search-boundary.md)
|
||||
is unchanged.
|
||||
|
||||
## Summary
|
||||
|
||||
`bun run check:boundary` passes on a tree that contains the exact leak it was
|
||||
built to catch. It matches a multi-type array only when written **inline at the
|
||||
query site**, so assigning the same array to a named const evades it entirely —
|
||||
which is how [searchScope.ts](../../src/lib/utils/searchScope.ts) has kept a
|
||||
category→item-type mapping through every green CI run. This spec broadens the
|
||||
match to item-type array literals anywhere in `src/`, and resolves the handful
|
||||
of legitimate hits that broadening surfaces.
|
||||
|
||||
## Motivation
|
||||
|
||||
The current pattern is anchored to the `includeItemTypes:` key:
|
||||
|
||||
```sh
|
||||
PATTERN='includeItemTypes:[[:space:]]*\[[^]]*,[^]]*\]'
|
||||
```
|
||||
|
||||
The live leak is not written that way:
|
||||
|
||||
```ts
|
||||
// src/lib/utils/searchScope.ts:29 — invisible to the tripwire
|
||||
const SCOPE_ITEM_TYPES = { music: ["MusicAlbum", "MusicArtist", "Audio", "Playlist"], … };
|
||||
```
|
||||
|
||||
The taxonomy and the query are one indirection apart, and the grep only sees the
|
||||
query. The script's own header is admirably honest that it is "a TRIPWIRE, NOT A
|
||||
PROOF" — but the gap here is not a subtle judgment call it was designed to
|
||||
defer to human review. It is the *crudest form* of the violation, one `const`
|
||||
away from the shape it does match, in the very file the founding incident was
|
||||
written about.
|
||||
|
||||
Broadening the pattern to any item-type array literal finds it, with a
|
||||
manageable number of other hits (measured, not estimated):
|
||||
|
||||
| Site | Verdict |
|
||||
|------|---------|
|
||||
| `searchScope.ts:30,32` | 🔴 The leak. Removed by [scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md). |
|
||||
| `PersonDetailView.svelte:30` | Already allowlisted, with a recorded reason. |
|
||||
| `DownloadedBrowse.svelte:95` | Borderline — `["MusicAlbum","Series","Season","BoxSet"].includes(item.type)` as an "is this a container?" predicate. |
|
||||
| `GenericMediaListPage.svelte:298` | Borderline — `["MusicAlbum","MusicArtist","Audio","Playlist"].includes(config.itemType)` as a music-styling predicate. |
|
||||
| 6 hits in `*.test.ts` | Excluded; tests legitimately name types. |
|
||||
|
||||
Four non-test sites total. This is a tractable change, not a boil-the-ocean one.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
Tooling only — no application logic, nothing crosses IPC. The two borderline
|
||||
*application* sites do get a layer decision, below.
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Detecting item-type array literals in `src/` | Build tooling (`scripts/`) | Static analysis of repo source; belongs beside the existing check. |
|
||||
| "Is this item a container?" (`DownloadedBrowse`) | **Rust** (recommended) | Containers-vs-leaves is Jellyfin structure, and the set grows when Jellyfin adds a container type — the litmus test's "yes". Prefer a `MediaItem.isContainer` boolean from the backend over a type-set predicate in a component. |
|
||||
| "Is this music content?" (`GenericMediaListPage`) | **Frontend, allowlisted** | Selects a grid *style*. It reads `config.itemType`, a value the page already declares about itself, and changes only if the UI is redesigned — the litmus test's "no". Single-type presentation is explicitly not the target of the rule. |
|
||||
|
||||
`DownloadedBrowse` defaults to Rust per the checklist's borderline rule; see
|
||||
Out of scope for why the migration itself is deferred rather than bundled.
|
||||
|
||||
## Design
|
||||
|
||||
### 1. Broaden the pattern
|
||||
|
||||
Replace the key-anchored pattern with one matching an array literal of two or
|
||||
more known Jellyfin item types, wherever it appears:
|
||||
|
||||
```sh
|
||||
# Two or more adjacent item-type string literals inside a bracket.
|
||||
TYPES='Movie|Series|Episode|Audio|MusicAlbum|MusicArtist|MusicVideo|Season|BoxSet|Playlist|Book|AudioBook|Video|Person|Folder|CollectionFolder|TvChannel|LiveTvChannel'
|
||||
PATTERN="\[[[:space:]]*\"($TYPES)\"[[:space:]]*,[[:space:]]*\"($TYPES)\""
|
||||
```
|
||||
|
||||
Properties worth stating, because each is a deliberate trade:
|
||||
|
||||
- **Not anchored to any key**, so a named const, a function return, a `Record`
|
||||
value, or an inline query all match equally.
|
||||
- **Requires two adjacent type literals**, preserving the existing and correct
|
||||
carve-out that single-type presentation (`itemType: "Movie"`) is legitimate.
|
||||
- **Requires string literals**, so `item.type === "Audio"` (display inspection)
|
||||
still does not match.
|
||||
- **Explicit type list**, not `[A-Z][a-z]+`, so arbitrary string arrays
|
||||
(`["High","Low"]`, `["Songs","Albums"]`) do not produce noise.
|
||||
|
||||
Keep `grep -rInE`, the `*.test.*` exclusion, and the allowlist mechanism as they
|
||||
are — all three work.
|
||||
|
||||
### 2. Resolve the surfaced sites
|
||||
|
||||
- `PersonDetailView.svelte` — already allowlisted; entry unchanged.
|
||||
- `GenericMediaListPage.svelte` — **add to the allowlist** with the reason from
|
||||
the layer table (grid styling over a self-declared `itemType`).
|
||||
- `DownloadedBrowse.svelte` — **add to the allowlist with a `TODO` naming the
|
||||
preferred fix** (backend `isContainer`). An allowlist entry that records a
|
||||
known-borderline decision is honest; silently broadening the pattern to miss
|
||||
it would not be.
|
||||
- `searchScope.ts` — **not allowlisted.** It is the leak, and
|
||||
[scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md)
|
||||
deletes it.
|
||||
|
||||
### 3. 🔴 Sequencing
|
||||
|
||||
**This spec must land after Stage 1 of
|
||||
[scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md).**
|
||||
Hardening the tripwire first turns `master` red on a violation with no fix
|
||||
available, and the only ways out are reverting the hardening or allowlisting the
|
||||
leak — the second of which is exactly how a boundary rule dies.
|
||||
|
||||
### 4. Keep the allowlist honest
|
||||
|
||||
The script already warns that a growing allowlist means the boundary is eroding.
|
||||
This change takes it from 1 entry to 3, which is close to that line. Add a hard
|
||||
cap so drift is caught mechanically rather than by whoever notices:
|
||||
|
||||
```sh
|
||||
MAX_ALLOWLIST=4
|
||||
if [ "${#ALLOWLIST[@]}" -gt "$MAX_ALLOWLIST" ]; then
|
||||
echo "❌ Allowlist has ${#ALLOWLIST[@]} entries (max $MAX_ALLOWLIST)."
|
||||
echo " Push taxonomy into Rust instead of appending here."
|
||||
exit 1
|
||||
fi
|
||||
```
|
||||
|
||||
The cap is deliberately just above the current count: the next exception forces
|
||||
a conversation instead of a one-line append.
|
||||
|
||||
### 5. Restate the limits
|
||||
|
||||
The header's "tripwire, not a proof" caveat stays and gets sharper. The broadened
|
||||
pattern still cannot see:
|
||||
|
||||
- a type set built at run time (`[...musicTypes, "Playlist"]`),
|
||||
- types split across variables (`const A = "Audio"; [A, B]`),
|
||||
- taxonomy expressed as a `switch` or chained `||` rather than an array.
|
||||
|
||||
The spec-review checklist remains the real gate. This raises the floor; it does
|
||||
not close the class.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **Migrating `DownloadedBrowse` to a backend `isContainer` flag.** It touches
|
||||
`MediaItem`, `bindings.ts`, and the offline path — its own spec. Allowlisted
|
||||
with a TODO here so it is recorded, not forgotten.
|
||||
- The scoped-search fix itself — [scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md).
|
||||
- Detecting the run-time-construction cases listed above.
|
||||
- Extending the check to Rust or Kotlin (the rule is about `src/`).
|
||||
- Changing the boundary *policy* in CLAUDE.md.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] With `searchScope.ts` reverted to its leaking form, `bun run check:boundary`
|
||||
**fails** and names `src/lib/utils/searchScope.ts`. This is the criterion
|
||||
that proves the fix — verify it explicitly before landing.
|
||||
- [ ] On the post-fix tree, `bun run check:boundary` passes.
|
||||
- [ ] A newly introduced `const X = ["Movie", "Series"]` in any non-test `src/`
|
||||
file fails the check (regression test for the const-indirection evasion).
|
||||
- [ ] `itemType: "Movie"` and `item.type === "Audio"` do **not** trip the check.
|
||||
- [ ] `["High", "Low"]` and other non-item-type arrays do **not** trip it.
|
||||
- [ ] Test files are still excluded (the 6 known test hits stay silent).
|
||||
- [ ] The allowlist has exactly 3 entries, each with a written reason; a 5th
|
||||
entry fails the check via `MAX_ALLOWLIST`.
|
||||
- [ ] The script header still states it is a tripwire, not a proof, and names the
|
||||
evasions it cannot see.
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
- [ ] `bun run test:all` passes.
|
||||
|
||||
## Testing
|
||||
|
||||
The script is bash and has no test harness. Verify by construction — each is a
|
||||
temporary edit, run, revert:
|
||||
|
||||
1. Reintroduce the `SCOPE_ITEM_TYPES` const → **must fail**.
|
||||
2. Add `const T = ["Movie","Series"]` to a scratch `.svelte` file → **must fail**.
|
||||
3. Add the same to a `.test.ts` file → **must pass** (exclusion holds).
|
||||
4. Add `itemType: "Movie"` → **must pass**.
|
||||
5. Add a 5th allowlist entry → **must fail** on the cap.
|
||||
|
||||
Record the five results in the PR description. A grep-based gate that has never
|
||||
been observed failing is indistinguishable from one that cannot fail — which is
|
||||
the precise condition this whole spec exists to correct.
|
||||
|
||||
## TRACES
|
||||
|
||||
Allocate in `requirements.md`:
|
||||
|
||||
- **DR-094** — "Frontend boundary tripwire detects Jellyfin item-type array
|
||||
literals anywhere in `src/` (not only inline at an `includeItemTypes:` query
|
||||
site), so a category→type mapping cannot evade the check via a named const;
|
||||
allowlist is capped to force taxonomy into Rust rather than accumulating
|
||||
exceptions." Category: Tooling. Status: Done on merge.
|
||||
|
||||
Shell scripts carry no `TRACES:` comment convention in this repo; reference
|
||||
DR-094 in the script header comment instead.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- A parallel Claude session may be active in this repo — `git diff` before
|
||||
"repairing" unexpected changes (CLAUDE.md §Gotchas).
|
||||
- **Land after Stage 1 of the scoped-search fix** — see §3. This is the one
|
||||
ordering constraint that will break `master` if ignored.
|
||||
- Test the regex against the current tree *before* committing:
|
||||
`grep -rInE "$PATTERN" src/ | grep -v '\.test\.'` should return exactly the
|
||||
four sites in the Motivation table.
|
||||
- The `TYPES` list will need occasional extension as Jellyfin adds types.
|
||||
That is acceptable for a tripwire — an unlisted type produces a false
|
||||
negative, never a false positive, so the check degrades safely.
|
||||
@@ -1,7 +1,13 @@
|
||||
# Spec: Build provenance (git describe + build profile)
|
||||
|
||||
**Status:** Proposed
|
||||
**Requirements:** new DR-093 (build provenance surfaced in-app and in logs); no UR — this is a diagnostic capability, not a user feature
|
||||
**Status:** Proposed — not started. `src-tauri/build.rs` still contains only
|
||||
`tauri_build::build()`, and nothing reports a version over IPC. Note that
|
||||
`scripts/set-version.sh` has since landed, which changes the "three hand-bumped
|
||||
files" premise below: versions are now stamped from one place.
|
||||
**Requirements:** ⚠️ the suggested id **DR-093 has since been allocated** to the
|
||||
traceability coverage gate — allocate a fresh id (DR-215 or later) on
|
||||
implementation. Build provenance surfaced in-app and in logs; no UR — this is a
|
||||
diagnostic capability, not a user feature
|
||||
**UX spec:** n/a — adds an About block to Settings; no new flow
|
||||
**Supersedes / revises:** —
|
||||
|
||||
|
||||
@@ -1,334 +0,0 @@
|
||||
# Spec: Locally-indexed search
|
||||
|
||||
**Status:** Implemented
|
||||
**Requirements:** UR-065 → DR-108, DR-109, DR-110, DR-111; IR-030
|
||||
**UX spec:** [ux-flows.md §6.1](../ux-flows.md) (search surface is unchanged)
|
||||
**Revises:** [scoped-search.md](scoped-search.md) and
|
||||
[scoped-search-boundary.md](scoped-search-boundary.md) — scope semantics are
|
||||
untouched; this changes only *which corpus* the cache leg searches.
|
||||
|
||||
## Summary
|
||||
|
||||
Search stops depending on a per-keystroke round trip to Jellyfin. The local
|
||||
SQLite catalog — which is already synced and already FTS5-indexed — becomes the
|
||||
corpus the instant leg of search reads, so results appear as fast as SQLite can
|
||||
answer, online or offline. A background indexer keeps that catalog fresh on a
|
||||
schedule instead of only at app start, prunes content deleted on the server, and
|
||||
covers the item types search groups results by. The server query stays, demoted
|
||||
to a background reconciliation that merges in late results for anything indexed
|
||||
since the last pass.
|
||||
|
||||
## Motivation
|
||||
|
||||
The pieces are already built and simply not wired together:
|
||||
|
||||
- [`sync_full_catalog`](../../src-tauri/src/commands/catalog.rs) already walks
|
||||
every library `Recursive=true` and persists items with `synced_at`.
|
||||
- `items_fts` (schema.rs migration 001) already indexes `name`, `overview`,
|
||||
`album_name`, `album_artist`, `artists`, `series_name` with keep-in-sync
|
||||
triggers.
|
||||
- `repository_search` is already two-phase — synchronous cache result, then a
|
||||
spawned server query merged in via the `search-event`.
|
||||
|
||||
What breaks the chain is that the cache leg is hard-restricted to *downloaded*
|
||||
items. `OfflineRepository::search` wraps its FTS query in a `downloaded_items`
|
||||
CTE requiring `d.status = 'completed'`:
|
||||
|
||||
```sql
|
||||
FROM items i
|
||||
JOIN items_fts fts ON fts.rowid = i.rowid
|
||||
INNER JOIN downloaded_items di ON i.id = di.id
|
||||
WHERE i.server_id = ? AND items_fts MATCH ?
|
||||
```
|
||||
|
||||
So for a user with no downloads, phase 1 returns nothing on every query, and
|
||||
every debounced keystroke falls through to a full `Recursive=true` server
|
||||
request with `Limit=10000`. The populated local index is never read.
|
||||
|
||||
`get_items` does not have this problem — it gates a third `synced_at IS NOT NULL`
|
||||
branch on `include_catalog_browse()` (offline.rs, the "Show all server media"
|
||||
toggle). The asymmetry is the bug: **offline you can already browse the whole
|
||||
catalog but cannot search it.**
|
||||
|
||||
Three further defects found while confirming the above:
|
||||
|
||||
1. **The FTS index grows without bound.** `save_to_cache` uses
|
||||
`INSERT OR REPLACE INTO items`, but `recursive_triggers` is never enabled
|
||||
(`storage/mod.rs` sets only `foreign_keys` and `journal_mode`). SQLite fires
|
||||
`AFTER DELETE` triggers on a REPLACE *only* with recursive triggers on — so
|
||||
`items_ad` never runs, the old FTS row is orphaned, and because `items.id` is
|
||||
a `TEXT PRIMARY KEY` the replacement row takes a **new rowid** and inserts a
|
||||
second FTS entry. Every sync appends a duplicate index. Results stay correct
|
||||
(the `INNER JOIN … ON fts.rowid = i.rowid` hides orphans, and no rowid is ever
|
||||
reused because nothing is deleted) but `MATCH` degrades permanently.
|
||||
2. **Server-side deletions never propagate.** There is no `DELETE FROM items`
|
||||
anywhere in the codebase. The local catalog is append-only, so media removed
|
||||
from the server would stay searchable forever — tolerable when the cache was
|
||||
only a browse accelerator, not acceptable when it is the search corpus.
|
||||
3. **The index omits types search groups by.** `CATALOG_ITEM_TYPES` is
|
||||
`MusicAlbum, Movie, Series, Season, Episode, Audio, BoxSet` — no
|
||||
`MusicArtist`, no `Playlist`, and People live in a separate `people` table
|
||||
with no FTS at all. UR-060 mandates Artists and People result groups, so today
|
||||
those can *only* come from the server.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Which corpus search reads (downloads-only vs full synced catalog) | **Rust** | Sync/availability policy over domain data. Changes if Jellyfin's API or the offline rules change, not if the UI is redesigned. Reuses the existing `include_catalog_browse()` flag so search and browse cannot diverge again. |
|
||||
| Index freshness policy — TTL, when a re-index is due, skip-while-offline | **Rust** | Explicitly named as domain policy in [SPEC-REVIEW-CHECKLIST.md](SPEC-REVIEW-CHECKLIST.md) ("reachability/sync policy"). It is currently frontend-driven in `offlineCatalog.ts`; this spec moves it. |
|
||||
| Which Jellyfin item types get indexed (`CATALOG_ITEM_TYPES`) | **Rust** | Textbook domain taxonomy — a category→item-type set. Must never appear in `src/`. |
|
||||
| Reconciling a crawl against local rows (what to prune) | **Rust** | Operates on domain data and depends on crawl completeness semantics. |
|
||||
| FTS query construction, ranking, scope→type expansion | **Rust** | Already there (`search_rank.rs`, `SearchScope::item_types()`); unchanged by this spec. |
|
||||
| Rendering a "catalog last indexed N ago" hint and any re-index button | **Frontend** | Pure presentation of a backend-supplied timestamp. |
|
||||
| Debounce interval, result group order, scope chips | **Frontend** | Input handling and view preference; changes only if the UI is redesigned. |
|
||||
|
||||
Borderline call, recorded: the **TTL value itself** (how many hours before a
|
||||
re-index is due) could be argued as a user preference and therefore frontend. It
|
||||
is placed in Rust because the frontend must not be able to decide *whether the
|
||||
cache is authoritative* — that is the same class of decision as
|
||||
`include_catalog_browse`, which already lives in Rust. If the TTL later becomes
|
||||
user-configurable it stays a Rust-owned setting the frontend edits through a
|
||||
command, not a frontend constant. Borderline defaults to Rust.
|
||||
|
||||
## Design
|
||||
|
||||
### 1. Search the full synced catalog (DR-108)
|
||||
|
||||
`OfflineRepository::search` mirrors `get_items` exactly: rename the CTE to
|
||||
`available_items` and add the same third branch, gated on the same flag.
|
||||
|
||||
```rust
|
||||
let catalog_branch = if include_catalog_browse() {
|
||||
"UNION
|
||||
|
||||
-- Synced catalog: fast online search, or the offline 'Show all
|
||||
-- server media' view. Mirrors get_items; see set_include_catalog_browse.
|
||||
SELECT DISTINCT i.id
|
||||
FROM items i
|
||||
WHERE i.synced_at IS NOT NULL"
|
||||
} else {
|
||||
""
|
||||
};
|
||||
```
|
||||
|
||||
No new IPC surface and no frontend change: `set_include_catalog_browse` is
|
||||
already called with `true` when online or when the offline toggle is on, and
|
||||
`false` only when offline with the toggle off. Search inherits the correct
|
||||
behaviour in all three states, and the "search is restricted to downloads" case
|
||||
survives for users who deliberately asked for downloads-only.
|
||||
|
||||
Also fix, in the same function, the `type_filter` built by **string
|
||||
interpolation** of `include_item_types` rather than bound parameters. It is
|
||||
currently safe only because callers pass `SearchScope`-derived values, but
|
||||
`SearchOptions.include_item_types` is settable directly from the frontend (as
|
||||
`GenericMediaListPage` does). Bind the values.
|
||||
|
||||
Phase 2 (the server query) is unchanged and still merges via `search-event`, so
|
||||
content added to the server since the last index still surfaces — just late
|
||||
rather than first.
|
||||
|
||||
### 2. Scheduled background indexer (DR-109, IR-030)
|
||||
|
||||
A Rust-owned task replaces the frontend's startup-only trigger.
|
||||
|
||||
```rust
|
||||
/// How long a full-catalog index stays fresh before a re-index is due.
|
||||
const CATALOG_INDEX_TTL: Duration = Duration::from_secs(6 * 60 * 60);
|
||||
```
|
||||
|
||||
Behaviour:
|
||||
|
||||
- On app setup, spawn a tokio task that ticks every 30 min.
|
||||
- Each tick: if a repository is active **and** the server is reachable **and**
|
||||
`now - last_catalog_sync > CATALOG_INDEX_TTL`, run a full index pass.
|
||||
- On the existing `ConnectivityMonitor` reconnect signal, evaluate the same
|
||||
staleness condition immediately rather than waiting for the next tick.
|
||||
- Never run two passes concurrently (the existing `syncInProgress` guard moves
|
||||
into Rust as an `AtomicBool`).
|
||||
|
||||
`last_catalog_sync` is already written to `app_settings` by `sync_full_catalog`
|
||||
and is currently read only for a UI hint; this makes it load-bearing.
|
||||
|
||||
`RepositoryManager` (`commands/repository.rs`) is a `HashMap<String, …>` with no
|
||||
notion of an active handle, so the task has nothing to run against. Add:
|
||||
|
||||
```rust
|
||||
pub struct RepositoryManager {
|
||||
repositories: Arc<Mutex<HashMap<String, Arc<HybridRepository>>>>,
|
||||
active: Arc<Mutex<Option<String>>>, // set in create(), cleared in destroy()
|
||||
}
|
||||
```
|
||||
|
||||
Progress is reported with a **kebab-case** event (per the project convention):
|
||||
|
||||
```rust
|
||||
// event name: "catalog-index-event"
|
||||
#[derive(specta::Type, Serialize, Clone)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct CatalogIndexEvent {
|
||||
pub state: CatalogIndexState, // #[serde(tag = "type")] Idle | Running | Complete | Failed
|
||||
pub libraries_done: usize,
|
||||
pub libraries_total: usize,
|
||||
pub items_indexed: usize,
|
||||
}
|
||||
```
|
||||
|
||||
`sync_full_catalog` stays a command so the UI can still force a pass; it and the
|
||||
scheduler share one internal `run_index_pass()`.
|
||||
|
||||
### 3. Index hygiene — no orphans, and deletions propagate (DR-110)
|
||||
|
||||
**Orphan growth.** Replace `INSERT OR REPLACE INTO items (…)` in `save_to_cache`
|
||||
with a true upsert:
|
||||
|
||||
```sql
|
||||
INSERT INTO items (id, server_id, …) VALUES (…)
|
||||
ON CONFLICT(id) DO UPDATE SET
|
||||
name = excluded.name, overview = excluded.overview, …,
|
||||
synced_at = excluded.synced_at
|
||||
```
|
||||
|
||||
This preserves the rowid (which `items_fts` keys on via `content_rowid`) and
|
||||
fires `items_au` instead of silently orphaning a row. Preferred over
|
||||
`PRAGMA recursive_triggers = ON` because it also stops the rowid churn, and the
|
||||
three FTS triggers are the only triggers in the schema so nothing else depends
|
||||
on REPLACE semantics.
|
||||
|
||||
A new migration `021_rebuild_items_fts` clears the orphans already accumulated on
|
||||
existing installs:
|
||||
|
||||
```sql
|
||||
INSERT INTO items_fts(items_fts) VALUES('rebuild');
|
||||
```
|
||||
|
||||
**Deletions.** After a library crawls *successfully and completely*, reconcile:
|
||||
delete local rows for that library whose `id` was not seen in the crawl. Two
|
||||
constraints the implementation must respect:
|
||||
|
||||
- Skip any item with a completed download — the user has the file; removing the
|
||||
row would orphan it. Prune only synced-but-not-downloaded rows.
|
||||
- Only sweep libraries whose crawl succeeded. `sync_full_catalog` is
|
||||
deliberately best-effort per library, and `items.parent_id` is
|
||||
`ON DELETE CASCADE` — sweeping on a partial crawl would cascade a whole series
|
||||
away because one request timed out.
|
||||
|
||||
### 4. Index the types search groups by (DR-111)
|
||||
|
||||
Add `MusicArtist` and `Playlist` to `CATALOG_ITEM_TYPES`.
|
||||
|
||||
People need a different mechanism: they live in `people` (`id`, `server_id`,
|
||||
`name`, `overview`, `primary_image_tag`, `synced_at`), populated incidentally by
|
||||
item-detail fetches, with no FTS table. Migration `022_people_fts` adds one
|
||||
mirroring the `items_fts` pattern:
|
||||
|
||||
```sql
|
||||
CREATE VIRTUAL TABLE IF NOT EXISTS people_fts USING fts5(
|
||||
name, overview, content='people', content_rowid='rowid'
|
||||
);
|
||||
-- plus people_ai / people_ad / people_au triggers
|
||||
```
|
||||
|
||||
`OfflineRepository::search` UNIONs `people_fts` matches into its result set as
|
||||
`Person`-typed items when the resolved scope permits them (i.e. when
|
||||
`include_item_types` is `None` — `SearchScope::All`). `search_rank.rs` already
|
||||
handles `MediaKind::Person`, so ranking needs no change.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **Incremental indexing** (e.g. Jellyfin's `MinDateLastSaved`). A full crawl is
|
||||
what makes the deletion sweep in §3 sound — it yields the authoritative id set
|
||||
per library. An incremental pass cannot detect deletions, so it would need a
|
||||
separate reconciliation strategy. Worth revisiting if full crawls prove too
|
||||
slow on large libraries; measure first.
|
||||
- **Changing search UX** — scope chips, group order, the debounce, and the
|
||||
`/search` route are untouched.
|
||||
- **Removing the server leg.** Phase 2 stays.
|
||||
- The two dead search implementations (`storage_search_items` in
|
||||
`commands/storage/mod.rs`, `offline_search` in `commands/offline.rs`) — both
|
||||
registered in `lib.rs` and exported to `bindings.ts`, neither called from the
|
||||
frontend. Deleting them is correct but is cleanup, not this feature; file
|
||||
separately so this spec's diff stays reviewable.
|
||||
- `GenericMediaListPage` passing raw `includeItemTypes` and re-implementing the
|
||||
store's request-id/event protocol. A real boundary smell, tracked separately.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] With a synced catalog and **zero downloads**, typing a query returns
|
||||
results from the local index before any server request completes.
|
||||
- [ ] Offline with "Show all server media" **on**, search returns the full
|
||||
catalog (non-downloaded entries greyed out, matching browse).
|
||||
- [ ] Offline with the toggle **off**, search returns downloaded media only —
|
||||
the behaviour that exists today.
|
||||
- [ ] Re-running a full index pass N times does not grow `items_fts` row count
|
||||
beyond the `items` row count.
|
||||
- [ ] An item deleted server-side disappears from local search after one index
|
||||
pass; a **downloaded** item deleted server-side does not.
|
||||
- [ ] A library that fails mid-crawl prunes nothing.
|
||||
- [ ] Searching an artist or actor name returns results with the server
|
||||
unreachable.
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
- [ ] `cargo fmt` clean, `cargo clippy` clean, `bun run test:rust` passes.
|
||||
- [ ] `bun run check:boundary` passes.
|
||||
- [ ] New requirement-implementing code carries `// TRACES:` comments.
|
||||
- [ ] `bindings.ts` regenerated (new `CatalogIndexEvent` type).
|
||||
|
||||
## Testing
|
||||
|
||||
Per CLAUDE.md, each defect gets a **failing test first**.
|
||||
|
||||
Rust (`cargo test`), against an in-memory DB seeded with synced-but-not-
|
||||
downloaded items:
|
||||
|
||||
- `search` returns synced items when `include_catalog_browse()` is true, and
|
||||
only downloaded items when false. *Fails today* — the current CTE returns
|
||||
empty in the first case.
|
||||
- Upserting the same item twice leaves exactly one `items_fts` row. *Fails
|
||||
today.*
|
||||
- The sweep removes a vanished synced item, retains a vanished downloaded item,
|
||||
and no-ops for a library whose crawl errored.
|
||||
- `type_filter` binds parameters — a type string containing a quote does not
|
||||
alter the query.
|
||||
- Staleness: a `last_catalog_sync` inside the TTL does not trigger a pass; one
|
||||
outside it does; offline never does.
|
||||
- `people_fts` matches surface as `Person` items under `SearchScope::All` and
|
||||
are excluded under `Music`/`Movies`/`Tv`.
|
||||
|
||||
Frontend (`vitest`): the catalog-index event maps to the staleness hint; no
|
||||
change to the search store's request-id/stale-response handling, which stays
|
||||
covered by its existing tests.
|
||||
|
||||
## TRACES
|
||||
|
||||
| Piece | Tag |
|
||||
|---|---|
|
||||
| `OfflineRepository::search` availability CTE | `// TRACES: UR-065 \| DR-108` |
|
||||
| Background indexer task + scheduling | `// TRACES: UR-065 \| DR-109, IR-030` |
|
||||
| `save_to_cache` upsert + FTS rebuild migration | `// TRACES: UR-065 \| DR-110` |
|
||||
| Deletion reconciliation | `// TRACES: UR-065 \| DR-110` |
|
||||
| `CATALOG_ITEM_TYPES` widening + `people_fts` | `// TRACES: UR-065, UR-060 \| DR-111` |
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- **A parallel Claude session may be active in this repo.** Run `git diff`
|
||||
before "repairing" changes you did not make (CLAUDE.md gotchas).
|
||||
- The frontend's `offlineCatalog.ts` startup trigger should be **removed**, not
|
||||
left alongside the Rust scheduler — two independent triggers with one
|
||||
`syncInProgress` guard each is how double-crawls happen.
|
||||
- `downloads` has a relaxed FK to `items` (migration 005). Verify the deletion
|
||||
sweep's interaction with it before enabling the sweep, and check whether
|
||||
`parent_id`'s `ON DELETE CASCADE` reaches further than intended.
|
||||
- The existing 100 ms `cache_with_timeout` in `hybrid.rs` returns *empty* on
|
||||
timeout rather than erroring. Once the cache leg is the primary path, that
|
||||
budget may need raising — an FTS query over a large catalog on cold page cache
|
||||
can exceed it, and the failure mode is a silently empty result.
|
||||
- Keep `SearchScope` semantics as-is: `All => None` (no filter), deliberately
|
||||
not a union, so People and folders are not filtered out (DR-063).
|
||||
- Noted but deliberately not fixed here: `pushCatalogVisibility` in
|
||||
`offlineCatalog.ts` derives the flag as `connected || showCatalog` — the
|
||||
frontend computing an availability *policy*, even though the flag itself is
|
||||
Rust-stored. DR-108 depends on that derivation being correct and it is, so
|
||||
this spec leaves it alone. Once DR-109 has moved sync policy into Rust, the
|
||||
derivation belongs there too, with the frontend pushing only the raw user
|
||||
toggle. Folding it into this change would enlarge the diff for no behavioural
|
||||
gain — but do not add *new* policy on the frontend side of that line.
|
||||
@@ -1,166 +0,0 @@
|
||||
# Spec: Downloads as a browsable offline library
|
||||
|
||||
**Status:** Draft — ready to implement
|
||||
**Scope:** Frontend-heavy; one new repository-client browse path. Minimal Rust.
|
||||
**Requirements:** UR-055 → DR-081, DR-082, DR-083, DR-084; UR-056 → DR-085
|
||||
(see [requirements.md](../requirements.md)).
|
||||
**UX spec:** [ux-flows.md §7.2–7.7](../ux-flows.md).
|
||||
|
||||
## Summary
|
||||
|
||||
Replace the flat Active/Completed download list with two views under
|
||||
`/downloads`:
|
||||
|
||||
1. **Downloaded** (default) — the library, filtered to what's on the device,
|
||||
using the *same* browse screens as online (grids, cards, detail pages).
|
||||
2. **Transfers** — the existing progress-row list, demoted to a secondary tab,
|
||||
showing only in-flight transfers.
|
||||
|
||||
Plus per-item disk usage (UR-056) shown in familiar units on cards, detail
|
||||
pages, a device total, and the remove confirmation.
|
||||
|
||||
## Motivation
|
||||
|
||||
A user who downloaded three seasons and two albums sees ~70 individual transfer
|
||||
rows today, with no grouping and no reuse of the library UI. "What do I have
|
||||
offline" and "what is downloading" are different questions crammed into one flat
|
||||
list. Browsing offline should feel exactly like browsing online.
|
||||
|
||||
## Background: verified current state
|
||||
|
||||
1. **The offline repository is already a browsable tree.**
|
||||
[offline.rs](../../src-tauri/src/repository/offline.rs) — `get_items` returns
|
||||
downloaded items **plus** containers (MusicAlbum, Series, Season) that have at
|
||||
least one downloaded child. `get_libraries`, `get_item`, and `search` all
|
||||
filter to downloaded content via CTEs. This is the data source for
|
||||
Downloaded; **do not build a new query layer.**
|
||||
|
||||
2. **The client cannot reach it independently.**
|
||||
[repository-client.ts](../../src/lib/api/repository-client.ts) `getItems` →
|
||||
`repositoryGetItems` always goes through the **hybrid** repository
|
||||
([hybrid.rs](../../src-tauri/src/repository/hybrid.rs)), which merges cache and
|
||||
server. There is no "offline only" browse path exposed. This is the one real
|
||||
backend gap (DR-082).
|
||||
|
||||
3. **Downloads page is a flat two-tab list.**
|
||||
[downloads/+page.svelte](../../src/routes/downloads/+page.svelte) — Active /
|
||||
Completed tabs, one `DownloadItem` row per transfer, no browsing.
|
||||
|
||||
4. **Library browse components are reusable as-is.** `LibraryGrid`, `MediaCard`,
|
||||
the `/library/[id]` detail page (§5A/§5B) render whatever items they are
|
||||
given. Downloaded browse is those components with an offline-scoped source.
|
||||
|
||||
5. **A related fallthrough bug is already tracked** (DR-080, another session):
|
||||
`HybridRepository::get_items` treats an empty offline result as a cache miss
|
||||
and falls through to the server. The offline-only browse path (DR-082) must
|
||||
**not** share that behaviour — an empty result there is authoritative "nothing
|
||||
downloaded here."
|
||||
|
||||
6. **Concurrency, the 3-download cap, and the auto-pump are backend concerns.**
|
||||
Do not surface them as manual controls; do not loop `startDownload` from the
|
||||
frontend (see [CLAUDE.md](../../CLAUDE.md) gotchas).
|
||||
|
||||
## Design
|
||||
|
||||
### View split (DR-081)
|
||||
|
||||
`/downloads` renders a **Downloaded** / **Transfers** switch. Downloaded is the
|
||||
default. Transfers shows a count/badge only while transfers are active.
|
||||
Initiating downloads stays on item/album/series detail pages (§7.1) — this page
|
||||
does not start downloads.
|
||||
|
||||
### Offline-scoped browse source (DR-082, DR-083)
|
||||
|
||||
Add an explicit offline-only browse path so Downloaded never merges server
|
||||
results and never depends on reachability. Two viable shapes — pick per the
|
||||
codebase, do not do both:
|
||||
|
||||
- **(a)** A dedicated command (e.g. `repository_get_downloaded_items` /
|
||||
`_libraries`) that calls the offline repository directly, with a matching
|
||||
client method; or
|
||||
- **(b)** An explicit `offlineOnly`/scope flag on the existing get-items path
|
||||
that bypasses the hybrid merge and the empty→fallthrough behaviour.
|
||||
|
||||
Either way: an empty result is authoritative (do **not** reuse the DR-080
|
||||
fallthrough), and the path is available while the server is reachable (a user
|
||||
online still wants to browse their downloads).
|
||||
|
||||
Downloaded then reuses `LibraryGrid` / `MediaCard` / the detail page against this
|
||||
source. Omit libraries and containers with no downloaded content. Badge
|
||||
partially- vs fully-downloaded containers. Play uses the local file; remove is
|
||||
available at item / album / season / series level and removes a container from
|
||||
the browse when its last downloaded child goes.
|
||||
|
||||
### Transfers view (DR-084)
|
||||
|
||||
The existing list, filtered to in-flight rows only: downloading (with progress),
|
||||
queued, paused, failed, waiting-for-WiFi (the DR-074 state from the other
|
||||
session). Controls: Pause / Resume / Cancel / Retry. Completed transfers leave
|
||||
this view — they appear in Downloaded. Empty state points at the library.
|
||||
|
||||
### Disk usage (DR-085, UR-056)
|
||||
|
||||
- **Source the bytes from the download manager** — it writes the files and can
|
||||
stat them. Aggregate to album/season/series subtotals and a device total.
|
||||
This is display + aggregation, **not** new tracking.
|
||||
- **Format once, consistently.** One shared formatter, human units, 2–3
|
||||
significant figures (`1.2 GB`, `340 MB`). Binary vs decimal — pick one and use
|
||||
it everywhere.
|
||||
- **Surface it in familiar places:** a secondary size label on the card and
|
||||
detail page; a device total at the top of Downloaded (`3.4 GB · 12 items`)
|
||||
that reconciles with the listed sum; a reclaim figure in the remove
|
||||
confirmation ("frees 1.2 GB"). No separate "storage report" screen.
|
||||
- Sort/filter by size is a nice-to-have, not required for v1.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Changing download initiation, the 3-concurrent cap, or the auto-pump.
|
||||
- The catalog-browse / show-server-catalog toggle (UR-052, another session) —
|
||||
that governs the *online offline-fallback* library; this is the dedicated
|
||||
Downloads surface. They should be consistent but are separate work.
|
||||
- Fixing the DR-080 hybrid fallthrough bug (owned elsewhere) — just don't depend
|
||||
on that behaviour here.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] `/downloads` opens on Downloaded and can switch to Transfers.
|
||||
- [ ] Downloaded lists only libraries/containers with downloaded content, using
|
||||
the same grids/cards/detail pages as online browsing.
|
||||
- [ ] Browsing Downloaded never shows non-downloaded server items, online or off.
|
||||
- [ ] An empty Downloaded result reads as "nothing downloaded," never falls
|
||||
through to the server.
|
||||
- [ ] Play from Downloaded plays the local file.
|
||||
- [ ] Remove works at item/album/season/series level and updates the browse.
|
||||
- [ ] Transfers shows only in-flight rows with working controls; finished
|
||||
transfers move to Downloaded.
|
||||
- [ ] Each downloaded item/container shows its on-disk size; a device total is
|
||||
shown and reconciles with the sum; remove states the reclaim amount.
|
||||
- [ ] `bun run check`, `bun run test`, and (if Rust touched) `cargo test` +
|
||||
`cargo clippy` pass.
|
||||
|
||||
## Testing
|
||||
|
||||
- Repository client: the offline-only browse path returns downloaded content and
|
||||
its containers, and an empty result does **not** trigger server fallthrough.
|
||||
- Downloaded view: libraries/containers with no downloads are omitted;
|
||||
partial/full container badging.
|
||||
- Transfers: only in-flight statuses render; a completed transfer disappears.
|
||||
- Size formatter: rounding and unit thresholds; subtotal aggregation; device
|
||||
total reconciles with listed items.
|
||||
- If a Rust command is added, add the tauri IPC param-naming coverage per
|
||||
[CLAUDE.md](../../CLAUDE.md) (camelCase rule).
|
||||
|
||||
New requirement-implementing code needs `TRACES:` comments. Suggested tags:
|
||||
view split `UR-055 | DR-081`; offline browse path `UR-055 | DR-082, DR-083`;
|
||||
Transfers `UR-055 | DR-084`; size display `UR-056 | DR-085`.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- Read [ux-flows.md §7.2–7.7](../ux-flows.md) first — behavioural spec; this is
|
||||
the implementation plan.
|
||||
- The offline repository already does the hard part. The main work is a clean
|
||||
offline-only client path and reusing the library components — resist
|
||||
rebuilding browse UI.
|
||||
- Another session is active in downloads/offline/connectivity code (DR-074,
|
||||
DR-078–080). Coordinate on [downloads/+page.svelte](../../src/routes/downloads/+page.svelte)
|
||||
and the repository layer; check `git diff` before repairing unexpected changes.
|
||||
@@ -1,403 +0,0 @@
|
||||
# Spec: Favourites — marking, browsing, and sync
|
||||
|
||||
**Status:** Implemented
|
||||
**Requirements:** UR-067, UR-068, UR-069 → DR-113 … DR-120; JA-033, JA-034
|
||||
(allocated in [requirements.md](../requirements.md); tests UT-099 … UT-107).
|
||||
Note: UR-066/DR-112/IR-031 were claimed by the concurrent safe-area work while
|
||||
this spec was being written, so the ids here start one higher than first drafted.
|
||||
Existing: UR-017 → DR-021, JA-017, JA-018 (the toggle itself, already built).
|
||||
**UX spec:** [ux-flows.md](../ux-flows.md) §3.2 (full-player favourite), §5.2
|
||||
(album detail favourite), §5B.3 (movie detail hero: *Play / Download / Favorite*)
|
||||
— all three already specify favourite affordances that **do not exist in the
|
||||
build**. This spec closes those, and adds a new §5C for the Favourites browse
|
||||
surface.
|
||||
**Supersedes / revises:** nothing.
|
||||
|
||||
## Summary
|
||||
|
||||
JellyTau can favourite an item but can never show you what you favourited. The
|
||||
heart is mounted in exactly one place (the mini player), no query anywhere asks
|
||||
Jellyfin or the local database for favourites, and favourites marked on any other
|
||||
client are invisible here. This spec adds the read side (a Favourites page, home
|
||||
carousels, an in-library filter), puts the heart on detail pages and media cards,
|
||||
teaches the backend to ingest server-side favourite state, and drains favourite
|
||||
toggles made while offline.
|
||||
|
||||
## Background: what exists today
|
||||
|
||||
Verified in code, 2026-08-04. The **write** path is real and mostly correct; the
|
||||
**read** path does not exist at all.
|
||||
|
||||
1. **Toggling works, from one place only.**
|
||||
[FavoriteButton.svelte](../../src/lib/components/FavoriteButton.svelte) is
|
||||
mounted solely in
|
||||
[MiniPlayer.svelte:381](../../src/lib/components/player/MiniPlayer.svelte#L381).
|
||||
Nothing else in `src/` renders it — so the only favouritable item in the app
|
||||
is the one currently playing.
|
||||
|
||||
2. **The toggle's plumbing is sound.**
|
||||
[favorites.ts](../../src/lib/services/favorites.ts) writes local first
|
||||
(`storage_toggle_favorite`,
|
||||
[storage/mod.rs:908](../../src-tauri/src/commands/storage/mod.rs#L908) — sets
|
||||
`user_data.is_favorite` + `pending_sync = 1`), then POST/DELETEs
|
||||
`/Users/{uid}/FavoriteItems/{id}`
|
||||
([online.rs:1652](../../src-tauri/src/repository/online.rs#L1652)) only when
|
||||
connected. Leave this design intact.
|
||||
|
||||
3. **`MediaItem.user_data` is always `None` from the server.**
|
||||
`JellyfinItem` has no `UserData` field, and `to_media_item` hardcodes
|
||||
[`user_data: None`](../../src-tauri/src/repository/online.rs#L656) with the
|
||||
comment *"User data not included in basic item responses"*. The only
|
||||
populated `user_data` in the app comes from
|
||||
[series_progress.rs](../../src-tauri/src/repository/series_progress.rs#L244)
|
||||
and the local read in
|
||||
[offline.rs:115](../../src-tauri/src/repository/offline.rs#L115). **Nothing
|
||||
ingests server favourite state**, which is why the mini player has to fetch
|
||||
`storageGetPlaybackProgress` per track to colour one heart
|
||||
([MiniPlayer.svelte:75-93](../../src/lib/components/player/MiniPlayer.svelte#L75-L93)).
|
||||
|
||||
4. **No favourites query exists.**
|
||||
[`GetItemsOptions`](../../src-tauri/src/repository/types.rs#L278) has no
|
||||
favourites field; `Filters=IsFavorite` appears nowhere; no SQL selects
|
||||
`is_favorite = 1`; there is no `/library/favorites` route and no favourites
|
||||
carousel in [home.ts](../../src/lib/stores/home.ts).
|
||||
|
||||
5. **Offline favourites are silently lossy.** Offline `mark_favorite` /
|
||||
`unmark_favorite` are no-ops
|
||||
([offline.rs:1620](../../src-tauri/src/repository/offline.rs#L1620)), so an
|
||||
offline toggle survives only as a local row with `pending_sync = 1` — and
|
||||
**nothing ever drains that flag**. `syncService.queueFavorite`
|
||||
([syncService.ts:91](../../src/lib/services/syncService.ts#L91)) exists with
|
||||
no callers.
|
||||
|
||||
## Motivation
|
||||
|
||||
Favouriting is a promise: the app takes the input and shows a "Added to
|
||||
favorites" toast, then discards it as far as the user can tell. Three of the UX
|
||||
flows already specify favourite buttons that were never built, and the one that
|
||||
was built (mini player) writes to a store nothing reads. Either the feature gets
|
||||
its read side or the heart should be removed — this spec takes the first option.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Favourites **scope** → set of Jellyfin item types | Rust | Domain taxonomy. Changes when Jellyfin adds/renames a type, never when the UI is redesigned. Reuses the canonical `SearchScope::item_types()` ([types.rs:324](../../src-tauri/src/repository/types.rs#L324)) — the exact leak class of [scoped-search-boundary.md](scoped-search-boundary.md). |
|
||||
| Cross-library favourites query (`Filters=IsFavorite`, `Recursive`, paging, sort field) | Rust | Query shaping against the Jellyfin API is domain logic; the endpoint's contract changes with the server, not the UI. |
|
||||
| Offline favourites SQL (join `user_data`, downloaded/catalog gating) | Rust | Storage + domain. Must obey the existing catalog-browse gate (DR-080) which the frontend cannot see. |
|
||||
| Deserialising Jellyfin `UserData` into `MediaItem.user_data` | Rust | Provider payload mapping. |
|
||||
| Mirroring server favourite state into the local `user_data` table | Rust | Cache/sync policy. |
|
||||
| Conflict rule: a local row with `pending_sync = 1` beats the server value | Rust | Business rule about which write wins; nothing to do with rendering. |
|
||||
| Draining pending favourite toggles on reconnect | Rust | Sync policy, and it must run whether or not any view is mounted — a frontend-driven drain dies with the component. Consistent with *reachability from real traffic* (DR-055). |
|
||||
| Which surfaces show favourites, tab order, row placement on home | Frontend | Pure presentation; changes only if the UI is redesigned. |
|
||||
| Heart placement, animation, toast, haptics, empty-state copy | Frontend | Presentation. |
|
||||
| In-session optimistic heart state shared across views | Frontend | View state, not persisted truth; the durable write already goes to Rust. |
|
||||
|
||||
**Borderline, and the tie-breaker used:** *which* scopes appear as tabs (All /
|
||||
Movies / Shows / Music) is a presentation choice — the frontend picks which
|
||||
`SearchScope` values to offer. What each scope *means* is Rust's. The frontend
|
||||
sends the enum value and never names an item type in connection with favourites.
|
||||
Single-type pages (`itemType: "Movie"` on the Movies list page) stay as they are;
|
||||
this rule targets category taxonomy, not every mention of a type.
|
||||
|
||||
## Design
|
||||
|
||||
### 1. Server user data reaches `MediaItem` (Rust, DR-113, JA-034)
|
||||
|
||||
Jellyfin returns `UserData` on `/Users/{uid}/Items*` responses. Add the field to
|
||||
`JellyfinItem` and map it in `to_media_item`, replacing the hardcoded `None`:
|
||||
|
||||
```rust
|
||||
// in JellyfinItem
|
||||
#[serde(alias = "UserData")]
|
||||
pub user_data: Option<JellyfinUserData>,
|
||||
```
|
||||
|
||||
`JellyfinUserData` deserialises `IsFavorite`, `Played`, `PlaybackPositionTicks`,
|
||||
`PlayCount`, `LastPlayedDate` into the existing
|
||||
[`UserData`](../../src-tauri/src/repository/types.rs#L44) type (which already
|
||||
carries `is_favorite` and already serialises camelCase, so `bindings.ts` needs no
|
||||
new type — only regeneration). Add `UserData` to the `Fields=` list in `get_items`
|
||||
/ `get_item` so the shape is explicit rather than relying on the default.
|
||||
|
||||
Wire shape, unchanged from today's `UserData`:
|
||||
|
||||
```ts
|
||||
item.userData?.isFavorite // boolean | null | undefined
|
||||
```
|
||||
|
||||
Delete the now-false `// User data not included in basic item responses` comment.
|
||||
|
||||
### 2. Local mirror of server favourites (Rust, DR-114)
|
||||
|
||||
Choke point: `save_to_cache(parent_id, &items)` in
|
||||
[offline.rs](../../src-tauri/src/repository/offline.rs) — every server result
|
||||
that gets cached (including via
|
||||
[`cache_items_from_server`](../../src-tauri/src/repository/hybrid.rs#L124) and
|
||||
the background cache refresh) passes through it.
|
||||
|
||||
For each item carrying `user_data.is_favorite`, upsert:
|
||||
|
||||
```sql
|
||||
INSERT INTO user_data (user_id, item_id, is_favorite, synced_at, pending_sync)
|
||||
VALUES (?, ?, ?, ?, 0)
|
||||
ON CONFLICT(user_id, item_id) DO UPDATE SET
|
||||
is_favorite = excluded.is_favorite,
|
||||
synced_at = excluded.synced_at
|
||||
WHERE user_data.pending_sync = 0; -- local unsynced change wins
|
||||
```
|
||||
|
||||
The `WHERE` on the conflict clause is the whole conflict rule: a toggle made
|
||||
offline is never overwritten by a stale server value before it has been pushed.
|
||||
|
||||
### 3. Favourites queries (Rust, DR-115, DR-116, JA-033)
|
||||
|
||||
**(a) In-library filter** — one new field on `GetItemsOptions`:
|
||||
|
||||
```rust
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub favorites_only: Option<bool>,
|
||||
```
|
||||
|
||||
- online `get_items`: append `&Filters=IsFavorite` when true.
|
||||
- offline `get_items`: add `INNER JOIN user_data ud ON ud.item_id = i.id AND ud.user_id = ? AND ud.is_favorite = 1`, composed with the existing `available_items` CTE so the downloads-only gate still applies.
|
||||
|
||||
Frontend sends `{ favoritesOnly: true }` (camelCase — nested struct field, needs
|
||||
the existing `#[serde(rename_all = "camelCase")]` on `GetItemsOptions`, already
|
||||
present).
|
||||
|
||||
**(b) Cross-library favourites** — a new trait method, because favourites span
|
||||
libraries and `get_items` is `ParentId`-shaped:
|
||||
|
||||
```rust
|
||||
/// TRACES: UR-067 | DR-115 | JA-033
|
||||
async fn get_favorites(
|
||||
&self,
|
||||
scope: SearchScope,
|
||||
options: Option<GetItemsOptions>,
|
||||
) -> Result<SearchResult, RepoError>;
|
||||
```
|
||||
|
||||
```rust
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
pub async fn repository_get_favorites(
|
||||
manager: State<'_, RepositoryManagerWrapper>,
|
||||
handle: String,
|
||||
scope: SearchScope,
|
||||
options: Option<GetItemsOptions>,
|
||||
) -> Result<SearchResult, String>
|
||||
```
|
||||
|
||||
Frontend call (command name matches the Rust fn exactly; top-level params
|
||||
auto-camelCase; `SearchScope` is `#[serde(rename_all = "camelCase")]` so the wire
|
||||
values are `"all" | "music" | "movies" | "tv"`):
|
||||
|
||||
```ts
|
||||
await commands.repositoryGetFavorites(handle, "movies", { limit: 100 });
|
||||
```
|
||||
|
||||
- **online**: `/Users/{uid}/Items?Filters=IsFavorite&Recursive=true&SortBy=SortName&SortOrder=Ascending` + `&IncludeItemTypes=…` from `scope.item_types()` (omit entirely on `None`, per that function's contract) + the standard `Fields=`.
|
||||
- **offline**: `items ⨝ user_data (is_favorite = 1)`, type filter from the same `scope.item_types()`, honouring `include_catalog_browse()`.
|
||||
- **hybrid**: same cache-first race as `get_items`, **including the DR-080 rule** — with the catalog-browse gate off, an empty offline result is authoritative and must not fall through to the server. Getting this wrong reproduces Defect B from [offline-downloaded-only-filter.md](offline-downloaded-only-filter.md).
|
||||
|
||||
**The cache-first result arrives stale, and there is no second payload.** On a
|
||||
cache hit, `hybrid::get_items` returns the local rows and refreshes the cache in
|
||||
a background task whose result the frontend never sees — fine for a library
|
||||
listing that changes daily, wrong for favourites, where the *point* is that
|
||||
another client just changed something. Favourites is the second read path (after
|
||||
search) that needs the deferred update, so the background refresh in
|
||||
`get_favorites` must emit the same `favorites-changed` event as §4 when the
|
||||
server's favourite set differs from what was returned:
|
||||
|
||||
```
|
||||
favorites-changed → { itemIds: string[] } // union of ids whose is_favorite flipped
|
||||
```
|
||||
|
||||
Both producers (background refresh, reconnect drain) emit the identical payload,
|
||||
and the frontend has one handler that refreshes the `favorites` store. Without
|
||||
this, a favourite marked on another client appears in JellyTau only on the
|
||||
*second* visit to the page.
|
||||
|
||||
### 4. Draining offline toggles (Rust, DR-120)
|
||||
|
||||
On the offline→online transition already detected by `ConnectivityMonitor`,
|
||||
select `user_data WHERE pending_sync = 1 AND is_favorite IS NOT NULL`, POST or
|
||||
DELETE `/Users/{uid}/FavoriteItems/{id}` per row, then set `pending_sync = 0` and
|
||||
`synced_at`. Failures leave the row pending for the next transition.
|
||||
|
||||
Emit a kebab-case event when anything changed, so open views refresh without
|
||||
polling:
|
||||
|
||||
```
|
||||
favorites-changed → { itemIds: string[] }
|
||||
```
|
||||
|
||||
`syncService.queueFavorite` is dead code once this lands — delete it or point it
|
||||
at the backend drain; do not leave two competing queues.
|
||||
|
||||
### 5. Frontend surfaces (DR-117, DR-118, DR-119)
|
||||
|
||||
**Favourites page** — new route `/library/favorites`:
|
||||
- Scope tabs *All / Movies / Shows / Music* via the existing `LibraryViewTabs`; each tab sends a `SearchScope` value, nothing more.
|
||||
- Renders through `LibraryGrid` + `MediaCard` (tracklist for Music→tracks if the tab is later split; not in this pass).
|
||||
- Entry points: a card on the library overview ([library/+page.svelte](../../src/routes/library/+page.svelte)) and "See all" on the home rows.
|
||||
- Empty state per tab: "Nothing favourited yet — tap the heart on anything you like."
|
||||
|
||||
**Home carousels** — `favoriteMovies`, `favoriteShows`, `favoriteMusic` added to
|
||||
[home.ts](../../src/lib/stores/home.ts), each `repositoryGetFavorites(scope, { limit: 20 })`,
|
||||
rendered after *Recently Added* and **only when non-empty** (no empty rows on a
|
||||
fresh install).
|
||||
|
||||
**In-library filter** — a favourites toggle in the header of
|
||||
[GenericMediaListPage](../../src/lib/components/library/GenericMediaListPage.svelte)
|
||||
and the Movies/TV landing pages, passing `favoritesOnly: true` into the existing
|
||||
`repo.getItems(...)` options. Session-scoped state; not persisted (a persisted
|
||||
filter that hides most of a library is a support call waiting to happen).
|
||||
|
||||
**Hearts** — mount `FavoriteButton`:
|
||||
- Movie / series detail hero button row, beside the download buttons ([library/[id]/+page.svelte:528-553](../../src/routes/library/%5Bid%5D/+page.svelte#L528-L553)) — closes ux-flows §5B.3.
|
||||
- `EpisodeFocusView`, `ArtistDetailView`, `PlaylistDetailView`, album detail — closes ux-flows §5.2.
|
||||
- `MediaCard` artwork overlay (top-right). Suppressed on `isServerOnly` cards, and must not fight the existing long-press/scroll-guard handlers ([MediaCard.svelte:60-70](../../src/lib/components/library/MediaCard.svelte#L60-L70)) — the heart is its own button and stops propagation.
|
||||
|
||||
**Shared optimistic state** — a small `favorites` store (`Map<string, boolean>`
|
||||
overlay + `favorites.set(id, value)`), so un-hearting an item on the Favourites
|
||||
page removes it from the grid and from any home row without a refetch, and a
|
||||
heart tapped on a card is reflected on the detail page. Resolution order:
|
||||
|
||||
```
|
||||
favorites store override ?? item.userData?.isFavorite ?? false
|
||||
```
|
||||
|
||||
`toggleFavorite()` updates the store alongside its existing local + server
|
||||
writes; the `favorites-changed` event refreshes it. This removes the mini
|
||||
player's per-track `storageGetPlaybackProgress` fetch once items carry
|
||||
`userData`.
|
||||
|
||||
### 6. Offline behaviour
|
||||
|
||||
Toggling offline keeps working exactly as now (local write + `pending_sync`), and
|
||||
now actually reaches the server on reconnect (§4). The Favourites page offline
|
||||
shows favourites among downloaded/cached items, subject to the existing
|
||||
catalog-browse gate. The offline repo's no-op `mark_favorite`/`unmark_favorite`
|
||||
stay no-ops — the local write plus the drain is the offline path.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Favouriting people, genres, or collections; favourite **playlists** are included only insofar as they fall under the Music scope.
|
||||
- Sorting by "date favourited" — Jellyfin does not expose it. Favourites sort by name.
|
||||
- A dedicated bottom-nav tab for favourites (reachable from library overview + home).
|
||||
- Building a playlist or download batch from favourites.
|
||||
- Reconciling favourites for items that no longer exist on the server.
|
||||
- Splitting the Music tab into albums/artists/tracks sub-tabs.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] Favouriting is possible from movie, series, episode, album, artist and playlist detail pages, and from media cards in any grid.
|
||||
- [ ] A favourite marked in another Jellyfin client shows a filled heart in JellyTau without toggling it here.
|
||||
- [ ] `/library/favorites` lists favourites across libraries, filtered by the All/Movies/Shows/Music tabs.
|
||||
- [ ] Home shows favourite rows for movies, shows and music, and shows no row when a category has none.
|
||||
- [ ] Movies/TV/Music list pages can be filtered to favourites only.
|
||||
- [ ] Un-hearting an item on one surface updates the others without a manual refresh.
|
||||
- [ ] A favourite toggled while offline reaches the server after reconnect (verified against a real server or a fake repository).
|
||||
- [ ] Offline, the Favourites page respects the "Show all server media" gate — with it off, an empty result stays empty and does not fall through to the server.
|
||||
- [ ] No item-type set appears in `src/` in connection with favourites; the frontend sends `SearchScope` only.
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
- [ ] `cargo fmt` clean, `cargo clippy` clean, `bun run test:rust` passes.
|
||||
- [ ] `bun run check:boundary` passes (necessary, not sufficient — see CLAUDE.md).
|
||||
- [ ] New requirement-implementing code carries `// TRACES:` comments.
|
||||
- [ ] `bindings.ts` regenerated from Rust, not hand-edited.
|
||||
|
||||
## Testing
|
||||
|
||||
**🔴 §4 (the pending-sync drain) is a bug fix — failing test first.** Write a
|
||||
test that toggles a favourite with the repository offline, transitions to online,
|
||||
and asserts the server call happened; watch it fail before writing the drain.
|
||||
|
||||
Rust (`cd src-tauri && cargo test`):
|
||||
|
||||
| Test | Covers |
|
||||
|------|--------|
|
||||
| UT-099 | A Jellyfin item JSON fixture with `UserData.IsFavorite: true` maps to `MediaItem.user_data.is_favorite == Some(true)` |
|
||||
| UT-100 | `online::get_favorites` builds an endpoint with `Filters=IsFavorite`, `Recursive=true`, and the scope's `IncludeItemTypes`; `SearchScope::All` omits the type filter entirely |
|
||||
| UT-101 | `offline::get_favorites` returns only `is_favorite = 1` rows, respects the scope type filter, and returns nothing extra when the catalog-browse gate is off |
|
||||
| UT-102 | `save_to_cache` mirror does **not** overwrite a row with `pending_sync = 1` |
|
||||
| UT-103 | Drain pushes pending rows, clears `pending_sync`, sets `synced_at`, and leaves failed rows pending |
|
||||
| UT-104 | `get_items` with `favorites_only: true` filters both online (endpoint) and offline (SQL) |
|
||||
| UT-107 | The background refresh in `hybrid::get_favorites` emits `favorites-changed` with the flipped ids, and emits nothing when the server set matches the cache |
|
||||
|
||||
Frontend (`bun run test`):
|
||||
|
||||
| Test | Covers |
|
||||
|------|--------|
|
||||
| UT-105 | `favorites` store override precedence: store value beats `userData.isFavorite` beats `false` |
|
||||
| UT-106 | Un-hearting removes the item from a favourites list view (pure logic extracted to a `.ts` module, per the TrackList/episodeStrip pattern) |
|
||||
| IT-0xx | `repositoryGetFavorites` param naming — add to the IPC param-naming suite under `src/lib/utils/` (`tauriIntegration.test.ts` no longer exists — see the current camelCase guards in `src/lib/stores/`): camelCase top-level params, scope serialised as `"movies"` etc. |
|
||||
|
||||
Any component logic worth testing gets extracted into a plain `.ts` module first
|
||||
(`favoritesView.ts`), rather than tested through the component.
|
||||
|
||||
## TRACES
|
||||
|
||||
| Piece | Tag |
|
||||
|-------|-----|
|
||||
| `JellyfinUserData` + `to_media_item` mapping | `// TRACES: UR-069 \| DR-113, JA-034 \| UT-099` |
|
||||
| `save_to_cache` user_data mirror | `// TRACES: UR-069 \| DR-114 \| UT-102` |
|
||||
| `get_favorites` (trait, online, offline, hybrid) + command | `// TRACES: UR-067 \| DR-115, JA-033 \| UT-100, UT-101` |
|
||||
| `GetItemsOptions.favorites_only` handling | `// TRACES: UR-067 \| DR-116 \| UT-104` |
|
||||
| `/library/favorites` route + tabs | `// TRACES: UR-067 \| DR-117` |
|
||||
| Home favourite carousels | `// TRACES: UR-067 \| DR-118` |
|
||||
| `FavoriteButton` mounts + `favorites` store | `// TRACES: UR-068 \| DR-119 \| UT-105, UT-106` |
|
||||
| Pending-favourite drain + `favorites-changed` | `// TRACES: UR-069 \| DR-120 \| UT-103` |
|
||||
|
||||
New requirement rows to add to [requirements.md](../requirements.md):
|
||||
|
||||
- **UR-067** — Browse favourited media across libraries (page, home rows, in-library filter).
|
||||
- **UR-068** — Mark/unmark favourites from browse and detail surfaces, not only the player.
|
||||
- **UR-069** — Favourite state stays consistent with the server in both directions.
|
||||
- **DR-113 … DR-120** — as tabled above.
|
||||
- **JA-033** — Query favourite items (`Filters=IsFavorite`).
|
||||
- **JA-034** — Read `UserData` from item responses.
|
||||
|
||||
## Implementation notes (as built)
|
||||
|
||||
Two things landed differently from the design above, both forced by where the
|
||||
`AppHandle` lives:
|
||||
|
||||
1. **The `favorites-changed` event is emitted from the command layer, not the
|
||||
repository.** `HybridRepository` has no `AppHandle` — the same reason
|
||||
`search-event` is emitted from `repository_search`. `repository_get_favorites`
|
||||
therefore does the two-phase read itself (cache leg returned, server leg
|
||||
spawned) and diffs the two id sets via `changed_favorite_ids`, which is
|
||||
extracted and unit-tested (UT-107) rather than buried in the spawn.
|
||||
2. **The drain hooks the existing `connectivity:reconnected` event** via
|
||||
`app.listen` in `commands/favorites.rs`, rather than reaching into
|
||||
`ConnectivityMonitor` (which knows nothing about repositories). It drains
|
||||
through a narrow `FavoriteSink` trait so it can be tested against a recording
|
||||
double instead of a forty-method `MediaRepository` mock.
|
||||
|
||||
3. **The command falls back to `HybridRepository::get_favorites` when nothing is
|
||||
cached.** The two-phase read alone paints "Nothing favourited yet" on a fresh
|
||||
install and corrects it a server round trip later, which is a wrong answer
|
||||
shown to the user. An empty cache leg therefore defers to the repository's
|
||||
own cache-first-then-server read. That read was also fixed to *save through*
|
||||
on a server hit — without it the page re-queried the server on every visit
|
||||
and the DR-114 mirror was never filled by this path.
|
||||
|
||||
Also as built: `DatabaseService` is not object-safe (generic methods), so the
|
||||
drain takes `Arc<RusqliteService>` like the rest of the storage code, and
|
||||
`get_items`' endpoint construction was extracted to `build_get_items_endpoint`
|
||||
so the `favorites_only` filter could be asserted without an HTTP server.
|
||||
|
||||
**Not built:** the full-player heart. ux-flows §3.2 lists one among the full
|
||||
player's secondary controls and it remains unbuilt — recorded as a known
|
||||
deviation in ux-flows §5C.5 rather than silently dropped.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- A parallel Claude session may be active in this repo — `git diff` before "repairing" unexpected changes.
|
||||
- Do **not** try to reuse `get_items` with an empty `ParentId` for cross-library favourites; that endpoint is built as `?ParentId={}` ([online.rs:731](../../src-tauri/src/repository/online.rs#L731)) and an empty value is not a reliable "all libraries" request. Use `get_favorites`.
|
||||
- `SearchScope` is reused rather than a new `FavoritesScope` so there is one taxonomy expansion in the codebase, not two that can drift. If the name grates once favourites ship, rename the type across search + favourites in one commit — don't fork it.
|
||||
- `SearchScope::All` returns `None` from `item_types()` **on purpose**; callers must omit `IncludeItemTypes` entirely rather than sending a union (see the doc comment at [types.rs:316](../../src-tauri/src/repository/types.rs#L316)).
|
||||
- Ship order that keeps each step demonstrable: §1+§2 (state becomes visible) → §5 hearts (marking becomes possible) → §3+§5 browse surfaces (finding becomes possible) → §4 drain.
|
||||
- Regenerate `bindings.ts` after the Rust types change; never hand-edit it.
|
||||
@@ -1,6 +1,8 @@
|
||||
# Spec: Migrate to libmpv2 and declare the project licence
|
||||
|
||||
**Status:** Proposed
|
||||
**Status:** Partially implemented — the `LICENSE` file has landed (part 2). The
|
||||
`libmpv` → `libmpv2` swap (part 1) is **not** done: `src-tauri/Cargo.toml` still
|
||||
pins the abandoned crate to a git branch.
|
||||
**Requirements:** UR-003 → IR-003 (revises the MPV integration); no new user-facing behaviour
|
||||
**UX spec:** n/a
|
||||
**Supersedes / revises:** dependency and licensing housekeeping identified in [playback-backend-unification.md](playback-backend-unification.md)
|
||||
|
||||
@@ -1,125 +0,0 @@
|
||||
# Spec: Library mosaic (library overview + home shortcuts)
|
||||
|
||||
**Status:** Implemented
|
||||
**Requirements:** UR-075 → DR-174, DR-175 (with UR-067 → DR-117 extended)
|
||||
**UX spec:** [ux-flows.md](../ux-flows.md) §5C.2 (Favourites)
|
||||
|
||||
## Summary
|
||||
|
||||
The library overview and the home "Your Libraries" strip stop being fixed-shape
|
||||
grids and become a **mosaic**: rows share one height, and each tile is as wide as
|
||||
its own artwork is. A square music cover, a 16:9 library backdrop and a 2:3
|
||||
poster sit in the same row at their own proportions instead of all three being
|
||||
cropped into whichever box the grid picked. Favourites gain a tile per category,
|
||||
placed beside the library that category belongs to, alongside the existing
|
||||
cross-library entry.
|
||||
|
||||
## Motivation
|
||||
|
||||
Every surface here shows artwork of more than one shape. The grid resolved that
|
||||
by choosing one shape and cropping to it — and the home strip said so out loud:
|
||||
|
||||
> Uniform 16:9 artwork so music (square) and video libraries line up at the same
|
||||
> height in this mixed row.
|
||||
|
||||
Lining them up is right; cropping the covers to do it is not. Holding the
|
||||
**height** fixed and letting the **width** vary achieves the same alignment with
|
||||
no crop at all, which is the whole idea of a justified layout.
|
||||
|
||||
Favourites had one entry for everything. With per-category tiles, "my favourite
|
||||
albums" is one tap from the library page rather than a tap plus a tab.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Collection type → favourites category (`movies` → Movies, `livetv` → none) | **Rust** | Jellyfin vocabulary. It changes when Jellyfin renames a collection type, never when this page is redesigned — the same test that put `SearchScope::item_types` in Rust. Shipping it in Svelte would have re-created the leak [scoped-search-boundary.md](scoped-search-boundary.md) exists to document. |
|
||||
| Which scopes exist at all (`SearchScope`) | **Rust** | Already there; unchanged. |
|
||||
| Row packing: heights, widths, justification, clamping | Frontend | Geometry of a rendered page. It changes when the layout is redesigned and never when the API does. |
|
||||
| Assumed artwork shape before the image loads (music = square, else wide) | Frontend | The shape of a *picture*, not a taxonomy — and it is only a starting guess, overruled by the decoded image. |
|
||||
| Tile labels, order, and showing a category's tile once | Frontend | Pure presentation: wording and placement. |
|
||||
|
||||
Borderline row: the "assumed artwork shape" is a per-collection-type default, and
|
||||
any per-collection-type table deserves suspicion. The tie-breaker: it does not
|
||||
decide *what a category means* or what is fetched — it seeds a pixel dimension
|
||||
that the loaded bitmap immediately corrects. Getting it wrong costs one re-pack,
|
||||
not a wrong result. The scope mapping, which does decide what is fetched, went to
|
||||
Rust.
|
||||
|
||||
## Design
|
||||
|
||||
### Wire
|
||||
|
||||
`Library` gains one optional field, derived at construction:
|
||||
|
||||
```rust
|
||||
pub struct Library {
|
||||
pub id: String,
|
||||
pub name: String,
|
||||
pub collection_type: String,
|
||||
pub image_tag: Option<String>,
|
||||
pub favorites_scope: Option<SearchScope>, // ← new
|
||||
}
|
||||
|
||||
impl SearchScope {
|
||||
pub fn for_collection_type(collection_type: &str) -> Option<SearchScope>;
|
||||
}
|
||||
```
|
||||
|
||||
```ts
|
||||
type Library = { …; favoritesScope?: SearchScope | null }
|
||||
```
|
||||
|
||||
`Library::new` derives it, so the four construction sites (online views, two
|
||||
offline cache reads, tests) cannot forget it. `None` is *omitted* from the JSON,
|
||||
not sent as null. No new command, no new event.
|
||||
|
||||
### Layout
|
||||
|
||||
`src/lib/components/library/mosaic.ts` — pure, no DOM:
|
||||
|
||||
- `layoutMosaic(items, { containerWidth, targetHeight, gap })` → rows of tiles
|
||||
with pixel boxes. Tiles join a row until the height needed to fill the width
|
||||
drops to the target; the row closes there and is justified to the container
|
||||
width, the rounding remainder absorbed by its widest tile. The **last row is
|
||||
not justified** (one leftover tile would inflate into a banner) — it sits at
|
||||
the target height, left-aligned.
|
||||
- `layoutMosaicStrip(items, height)` → the same rule as one fixed-height row, for
|
||||
a horizontally scrolling shelf.
|
||||
- `mosaicTargetHeight(containerWidth)` → the row height chosen when the caller
|
||||
doesn't pick one. Bounded so a phone still fits two tiles across and a desktop
|
||||
doesn't turn each library into a billboard.
|
||||
- Ratios are clamped to a band (0.5–2.5) so one panorama can't own a row.
|
||||
|
||||
`MosaicGrid.svelte` supplies the two things only the DOM knows — the measured
|
||||
container width (`bind:clientWidth`) and the artwork's decoded ratio — and
|
||||
renders the caller's `tile` snippet. `CachedImage` gained an `onNaturalSize`
|
||||
callback for the second. Measured ratios are committed in one debounced batch
|
||||
(120 ms): artwork arrives over several hundred milliseconds and re-packing per
|
||||
image would shuffle the grid under the pointer.
|
||||
|
||||
`MosaicTile.svelte` draws one tile at an exact pixel box, with its label written
|
||||
**over** the bottom of the artwork. A caption below the box would add height the
|
||||
layout didn't compute, and a caption that wrapped to two lines would break the
|
||||
row alignment the mosaic exists to provide.
|
||||
|
||||
### Composition
|
||||
|
||||
`libraryMosaic.ts` (pure, tested) builds the tile list: the cross-library
|
||||
favourites entry first, then each library followed by its own category tile. A
|
||||
category appears **once** — two movie libraries share one favourites list, so a
|
||||
tile each would be two tiles to the same place. A library whose `favoritesScope`
|
||||
is absent (Live TV, channels, books) gets no tile rather than one opening an
|
||||
unfiltered list.
|
||||
|
||||
Home uses the same tiles in `layout="strip"` but **without** the favourites tiles:
|
||||
home already carries Favourite Movies / Shows / Music rows of its own, and a
|
||||
second entry point in the strip above them would be redundant.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- The item grids inside a library (`/library/movies`, `/library/music/albums`, …).
|
||||
Those show one item type each, so a uniform grid crops nothing; the mosaic buys
|
||||
them nothing but reflow.
|
||||
- Backdrop/collage artwork for libraries with no image of their own.
|
||||
- Reordering or pinning libraries.
|
||||
@@ -1,235 +0,0 @@
|
||||
# Spec: Offline "downloaded only" filtering (issue #10)
|
||||
|
||||
**Status:** Implemented
|
||||
**Scope:** Frontend (connectivity store) + Rust (hybrid repository). No new
|
||||
commands, no schema changes, no UI additions.
|
||||
**Requirements:** UR-052 → DR-078, DR-079, DR-080
|
||||
(see [requirements.md](../requirements.md)).
|
||||
**Tracking:** issue #10 — *"when offline the filter to show only downloaded
|
||||
media does not work."*
|
||||
|
||||
## Summary
|
||||
|
||||
Offline, a library page is supposed to show **only media on the device**, with a
|
||||
"Show all server media" toggle that additionally reveals the cached server
|
||||
catalog greyed out (queueable for download on reconnect). In practice the toggle
|
||||
does not gate the listing — every server item still appears. This spec fixes
|
||||
that with two independent changes; either one alone leaves the bug visible.
|
||||
|
||||
## Background: what already exists
|
||||
|
||||
Verified in code. **The feature is built and mostly correct — this is a
|
||||
two-point repair, not new infrastructure.** Do not rebuild the toggle, the
|
||||
command, or the SQL gate.
|
||||
|
||||
1. **The SQL gate works and is unit-tested.**
|
||||
[offline.rs](../../src-tauri/src/repository/offline.rs) — `get_items` appends
|
||||
the synced-catalog `UNION` branch only when `include_catalog_browse()` is
|
||||
true; with it false, only downloaded/local rows return. Guarded by
|
||||
`test_get_items_toggle_gates_synced_catalog` (UT-067). **Do not touch the
|
||||
query.**
|
||||
|
||||
2. **The toggle → backend path is wired.** The `showServerCatalog` store and the
|
||||
`set_show_server_catalog` command
|
||||
([catalog.rs](../../src-tauri/src/commands/catalog.rs)) drive the process-wide
|
||||
`INCLUDE_CATALOG_BROWSE` flag. `pushCatalogVisibility` in
|
||||
[offlineCatalog.ts](../../src/lib/services/offlineCatalog.ts) computes
|
||||
`include = connected || showCatalog` and pushes it on every change.
|
||||
|
||||
3. **Home-screen queries are already downloads-only.** `get_latest_items`,
|
||||
`get_resume_items`, `get_recently_played_audio`, `get_resume_movies` all
|
||||
`INNER JOIN downloads ... status = 'completed'`. They are unaffected — leave
|
||||
them.
|
||||
|
||||
4. **`MediaCard` already greys and queues.**
|
||||
[MediaCard.svelte](../../src/lib/components/library/MediaCard.svelte) —
|
||||
`isServerOnly` renders the greyed, inert card with a queue button; the queued
|
||||
row heals its `stream_url` on reconnect via the offlineCatalog service. Leave
|
||||
it.
|
||||
|
||||
## The two defects
|
||||
|
||||
### Defect A — offline is never actually entered (DR-079)
|
||||
|
||||
`pushCatalogVisibility` keys off `isConnected`, but
|
||||
[connectivity.ts](../../src/lib/stores/connectivity.ts) derives:
|
||||
|
||||
```ts
|
||||
isConnected = isOnline && isServerReachable // isOnline = navigator.onLine
|
||||
```
|
||||
|
||||
`navigator.onLine` is documented in that same file as **advisory only** — the
|
||||
Rust `ConnectivityMonitor` is the source of truth (principle: *reachability from
|
||||
real traffic*, DR-055). When the server is unreachable but the device link is
|
||||
up (server down, wrong LAN, VPN dropped), `isOnline` stays true, so `isConnected`
|
||||
stays true, so `include` stays true, so the backend keeps returning the full
|
||||
catalog. The user is "offline" in every meaningful sense but the toggle never
|
||||
gets a chance to gate anything.
|
||||
|
||||
This is the primary cause: it explains why the filter looks dead rather than
|
||||
merely inverted — the gate never closes.
|
||||
|
||||
### Defect B — an intentionally empty result falls through to the server (DR-080)
|
||||
|
||||
With the gate off and nothing downloaded in a library, offline `get_items`
|
||||
correctly returns few or zero rows. But
|
||||
[hybrid.rs](../../src-tauri/src/repository/hybrid.rs) treats a cache result as a
|
||||
hit only `if data.has_content()`. An empty offline result is indistinguishable
|
||||
from a cache miss, so `HybridRepository::get_items` (and `parallel_race`, used by
|
||||
~10 other reads) falls through to the server and returns the full server list —
|
||||
re-defeating the filter even after Defect A is fixed.
|
||||
|
||||
## Design
|
||||
|
||||
### Fix A: `isConnected` follows backend reachability alone (DR-079)
|
||||
|
||||
In [connectivity.ts](../../src/lib/stores/connectivity.ts), redefine the derived
|
||||
store:
|
||||
|
||||
```ts
|
||||
export const isConnected = derived(
|
||||
connectivity,
|
||||
($c) => $c.isServerReachable
|
||||
);
|
||||
```
|
||||
|
||||
`navigator.onLine` stays wired to what it is good for — a *trigger* for an
|
||||
immediate recheck (`online`/`offline` listeners already call
|
||||
`checkServerReachable()`); it must no longer be a *term* in the offline decision.
|
||||
Leave `isOnline` on the state object and the listeners intact.
|
||||
|
||||
Consider whether the optimistic `isServerReachable: true` startup default
|
||||
([connectivity.ts](../../src/lib/stores/connectivity.ts)) should hold until the
|
||||
first real check resolves. Keep it — flipping the app to "offline" on launch is a
|
||||
worse regression than a brief full-catalog flash before the first probe. Note the
|
||||
choice in a comment.
|
||||
|
||||
**Blast radius — this is the reason this is a spec, not a patch.** `isConnected`
|
||||
is consumed beyond this feature (banners, `MediaCard`, mini-player gating,
|
||||
anything importing it). Enumerate consumers first:
|
||||
|
||||
```
|
||||
grep -rn "isConnected" src/ | grep -v node_modules
|
||||
```
|
||||
|
||||
For each, confirm "server unreachable" (not "device link down") is the correct
|
||||
trigger. It almost always is — that is the whole point of the reachability model
|
||||
— but verify rather than assume, and call out anything that genuinely wanted the
|
||||
device link in the PR description.
|
||||
|
||||
### Fix B: an empty offline result is authoritative when the gate is off (DR-080)
|
||||
|
||||
The backend must distinguish "cache is cold, go ask the server" from "user asked
|
||||
for downloads only and there are none here." The gate flag already encodes intent
|
||||
— reuse it.
|
||||
|
||||
Add a getter beside the existing setter in
|
||||
[offline.rs](../../src-tauri/src/repository/offline.rs):
|
||||
|
||||
```rust
|
||||
pub fn include_catalog_browse() -> bool { /* pub, already exists privately */ }
|
||||
```
|
||||
|
||||
In [hybrid.rs](../../src-tauri/src/repository/hybrid.rs) `get_items`: when
|
||||
`!include_catalog_browse()`, treat the offline result as authoritative and return
|
||||
it **as-is even when empty** — do not spawn/await the server fallback for this
|
||||
call. When the flag is on (online fast-path, or offline with the toggle on),
|
||||
behaviour is unchanged: empty cache still falls through to the server.
|
||||
|
||||
Keep it surgical:
|
||||
|
||||
- Scope the change to `get_items`. The gate is a `get_items` concept; do not
|
||||
thread it into `parallel_race` or the other readers, which have no catalog
|
||||
gate and legitimately want the server on an empty cache.
|
||||
- Preserve the online path exactly: with the flag on (its default, and always so
|
||||
while reachable) the method behaves as it does today, including the background
|
||||
cache refresh on a hit.
|
||||
- The flag is process-global `Relaxed`; it is set from the frontend before the
|
||||
query. That ordering already holds for the SQL gate — no new synchronization.
|
||||
|
||||
### Why both
|
||||
|
||||
Fix A closes the gate; Fix B stops the hybrid from re-opening it. A alone: with
|
||||
downloads present the list still gets padded by the server fallback whenever a
|
||||
library's cache is thin. B alone: the gate never closes because `isConnected`
|
||||
never goes false on a live link. Ship them together.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- The SQL gate, the toggle, the command, `INCLUDE_CATALOG_BROWSE` — all correct.
|
||||
- `MediaCard` greying / queue-on-reconnect — correct.
|
||||
- Home-screen and resume queries — already downloads-only.
|
||||
- The Rust `ConnectivityMonitor` reachability logic itself — unchanged; this
|
||||
spec only stops the *frontend* from diluting its verdict with `navigator.onLine`.
|
||||
- Any new IPC command, DB column, or settings entry.
|
||||
- Making the "Show all server media" toggle reachable from Settings (that is a
|
||||
UX-placement question, tracked separately under UR-051's toggle note).
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [~] With the server unreachable on a live device link, a library page lists
|
||||
only downloaded media when the toggle is off (IT-016 — pending e2e; unit
|
||||
coverage via UT-069 + gate tests).
|
||||
- [x] Turning the toggle on reveals the greyed-out cached catalog; turning it off
|
||||
hides it again — without leaving/re-entering the page (SQL gate + toggle
|
||||
wiring unchanged; UT-068 confirms the flag is pushed on toggle change).
|
||||
- [x] A library with downloads and a thin cache does not get padded with
|
||||
non-downloaded server items when offline with the toggle off (Defect B —
|
||||
UT-070: gate off + empty offline result returned as-is, server not queried).
|
||||
- [x] `isConnected` is false whenever the server is unreachable, regardless of
|
||||
`navigator.onLine`; true for a reachable server even if the browser reports
|
||||
offline (UT-069).
|
||||
- [x] Every existing `isConnected` consumer still behaves correctly (banner in
|
||||
`+layout.svelte`, `MediaCard`, `favorites.ts` server-write skip — all want
|
||||
"server unreachable", which is the new semantics; `CastButton`'s local
|
||||
`isConnected` is unrelated). Full frontend suite (616 tests) green.
|
||||
- [x] Online behaviour is unchanged: with the flag on (its default, always so
|
||||
while reachable) `get_items` keeps the offline fast-path and background
|
||||
refresh (UT-067 + gate-on fall-through test).
|
||||
- [~] A download queued from a greyed offline card resolves and starts on
|
||||
reconnect (IT-017 — regression check, no code change; offlineCatalog
|
||||
resume path untouched).
|
||||
- [x] `bun run check`, `bun run test`, and `bun run test:rust` pass;
|
||||
`cd src-tauri && cargo fmt && cargo clippy` clean (no new warnings in the
|
||||
touched files).
|
||||
|
||||
## Testing
|
||||
|
||||
Rust ([offline.rs](../../src-tauri/src/repository/offline.rs) /
|
||||
[hybrid.rs](../../src-tauri/src/repository/hybrid.rs) test modules):
|
||||
|
||||
- **UT-070** — hybrid `get_items` with the gate off returns an empty offline
|
||||
result as-is and does **not** query the server. Assert via a mock online repo
|
||||
whose `get_items` bumps a call counter that must stay at zero.
|
||||
- Gate on + empty cache still falls through to the server (guard the online path).
|
||||
- UT-067 (`test_get_items_toggle_gates_synced_catalog`) must still pass untouched.
|
||||
|
||||
Frontend (vitest, `src/lib/**/*.test.ts`):
|
||||
|
||||
- **UT-069** — `isConnected` follows `isServerReachable` alone: false when
|
||||
unreachable with `navigator.onLine === true`; true when reachable with
|
||||
`navigator.onLine === false`.
|
||||
- **UT-068** — `pushCatalogVisibility` resolves `serverReachable || showCatalog`
|
||||
and pushes to the backend on a change of either input (extend the existing
|
||||
offlineCatalog tests).
|
||||
|
||||
Integration (IT-016, IT-017) are documented as pending in
|
||||
[requirements.md](../requirements.md); wire them if the e2e harness can simulate
|
||||
an unreachable-server-on-live-link state, otherwise leave them pending with a note.
|
||||
|
||||
New/changed requirement code keeps its `TRACES:` comments — see
|
||||
[CLAUDE.md](../../CLAUDE.md). The affected files already carry tags:
|
||||
`connectivity.ts` (`… | DR-079`), `hybrid.rs` (`… | DR-080`), `offline.rs`
|
||||
(`… | DR-078`). Update the getter's tag when you expose it.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- Read [docs/architecture/07-connectivity.md](../architecture/07-connectivity.md)
|
||||
before Fix A — it is the canonical statement of the reachability model this fix
|
||||
restores fidelity to.
|
||||
- Fix B relies on the frontend having pushed the flag before the query runs; that
|
||||
ordering already holds for the SQL gate today. No new locking.
|
||||
- Another session is active in this repo (WiFi-only downloads, account menu
|
||||
landed alongside this work). Check `git diff` before "repairing" unexpected
|
||||
changes, and expect requirement IDs around UR-052 / DR-078 to be adjacent to
|
||||
other new rows.
|
||||
@@ -3,7 +3,10 @@
|
||||
**Status:** Accepted (analysis; no code changes)
|
||||
**Requirements:** IR-004, UR-031, UR-032, UR-033 — revises the "Platform Playback Backend Parity" issue in requirements.md
|
||||
**UX spec:** n/a
|
||||
**Supersedes / revises:** informs [android-native-video-spike.md](android-native-video-spike.md), [android-audio-settings-parity.md](android-audio-settings-parity.md), [windows-native-audio-backend.md](windows-native-audio-backend.md)
|
||||
**Supersedes / revises:** informed the Android native-video and audio-parity
|
||||
work (both since shipped — see
|
||||
[05-platform-backends.md](../architecture/05-platform-backends.md)) and
|
||||
[windows-native-audio-backend.md](windows-native-audio-backend.md), still open
|
||||
|
||||
## Summary
|
||||
|
||||
@@ -167,9 +170,9 @@ rewrite, not the bindings.
|
||||
2. **Android native video is worth a bounded spike anyway** — not for
|
||||
unification, but because ExoPlayer's `SurfaceView` path already exists and
|
||||
would restore hardware decode plus ASS/SSA subtitles. See
|
||||
[android-native-video-spike.md](android-native-video-spike.md).
|
||||
[05-platform-backends.md](../architecture/05-platform-backends.md#native-video-compositing-android).
|
||||
3. **Audio parity is the real gap** and is achievable without touching any of the
|
||||
above. See [android-audio-settings-parity.md](android-audio-settings-parity.md)
|
||||
above. See [05-platform-backends.md](../architecture/05-platform-backends.md)
|
||||
and [windows-native-audio-backend.md](windows-native-audio-backend.md).
|
||||
4. **Migrate the dead libmpv pin** regardless of any of this. See
|
||||
[libmpv2-migration.md](libmpv2-migration.md).
|
||||
|
||||
@@ -1,198 +0,0 @@
|
||||
# Spec: Playback documentation corrections
|
||||
|
||||
**Status:** Proposed
|
||||
**Requirements:** revises the status of DR-034; corrects the parity matrix in [requirements.md](../requirements.md)
|
||||
**UX spec:** n/a
|
||||
**Supersedes / revises:** implements the "Corrections to existing docs" section of [playback-backend-unification.md](playback-backend-unification.md)
|
||||
|
||||
## Summary
|
||||
|
||||
Fix four factual errors in the requirements doc and the player source comments,
|
||||
all found while investigating backend unification. Each claims something the code
|
||||
does not do. Small change, but they are actively misleading: two of them assert a
|
||||
feature is implemented when it is implemented nowhere, and one cites an upstream
|
||||
blocker that no longer exists.
|
||||
|
||||
Documentation and comments only — no behaviour change.
|
||||
|
||||
## Motivation
|
||||
|
||||
These errors compound. DR-034 reads "Done (Linux only)", so a future session
|
||||
planning Android parity would reasonably assume crossfade exists on Linux and
|
||||
only needs porting — when in fact it is unimplemented everywhere *and*
|
||||
architecturally blocked on the engine it supposedly runs on. Likewise the
|
||||
tauri#10152 comment has been discouraging work on Android native video since the
|
||||
upstream capability shipped in September 2024.
|
||||
|
||||
## The corrections
|
||||
|
||||
### 1. DR-034 status is wrong
|
||||
|
||||
`requirements.md` line ~196:
|
||||
|
||||
```
|
||||
| DR-034 | Crossfade engine with configurable duration (0-12s) | Player | UR-031 | Done (Linux only) |
|
||||
```
|
||||
|
||||
The code:
|
||||
|
||||
```rust
|
||||
// src-tauri/src/player/mpv_backend.rs, in set_audio_settings
|
||||
// TODO: Implement crossfade via MPV audio filters if needed
|
||||
```
|
||||
|
||||
That is the entire crossfade implementation. `crossfade_duration` is plumbed
|
||||
through `AudioSettings` and clamped to 0–12s, but no backend ever acts on it.
|
||||
|
||||
**Change to:** `Not implemented (blocked on MPV — see playback-backend-unification.md)`
|
||||
|
||||
Worth stating *why* in the requirements entry, because it is not a scheduling
|
||||
gap: mpv's audio chain is single-stream, and FFmpeg's `acrossfade` is an `N→A`
|
||||
filter needing two inputs. Real crossfade requires two libmpv instances with
|
||||
manually ramped volumes. Upstream declined the feature (mpv#4512).
|
||||
|
||||
### 1b. UR-031 status is wrong for the same reason
|
||||
|
||||
`requirements.md` line ~44:
|
||||
|
||||
```
|
||||
| UR-031 | Crossfade between audio tracks | Low | Done (Linux only) |
|
||||
```
|
||||
|
||||
Same error one level up: the *user* requirement is also marked done. Since no
|
||||
backend implements crossfade, UR-031 is not satisfied on any platform.
|
||||
|
||||
**Change to:** `Not implemented (blocked — see DR-034)`
|
||||
|
||||
Note line ~517 of the same file (`UR-031 (Crossfade), UR-032 (Gapless),
|
||||
UR-033 (Normalization) only work on Linux`) inherits the error — crossfade works
|
||||
nowhere, so it should read UR-032/UR-033 only.
|
||||
|
||||
### 2. Parity matrix crossfade row is wrong
|
||||
|
||||
```
|
||||
| Crossfade | ✅ | ❌ | Gap |
|
||||
```
|
||||
|
||||
**Change to** `| Crossfade | ❌ | ❌ | Not implemented |` — it is not a
|
||||
platform-parity gap, it is an unbuilt feature.
|
||||
|
||||
### 3. Parity matrix is missing the equalizer
|
||||
|
||||
The matrix lists crossfade, gapless, and normalization but omits the EQ, which
|
||||
has the same Linux-only shape and the same root cause (`ExoPlayerBackend` not
|
||||
overriding `set_audio_settings`). `build_af_filter` and `eq_filter_entries` exist
|
||||
only in `mpv_backend.rs`; there is no equalizer code in the Android tree.
|
||||
|
||||
**Add:** `| Equalizer (10-band) | ✅ | ❌ | Gap |`
|
||||
|
||||
### 4. `nativeAdapter.ts` cites a stale blocker
|
||||
|
||||
`src/lib/player/adapters/nativeAdapter.ts:11-14` states native Android video is
|
||||
blocked upstream by tauri#10152 (transparent webview / SurfaceView compositing).
|
||||
|
||||
tauri#10152 is open but **dead since 2024-07-01**, and it is a *feature request*
|
||||
that `WebviewWindowBuilder::transparent` was desktop-only — not a report that
|
||||
compositing is broken. The capability shipped in tauri commit `27d01834`
|
||||
(2024-09-02), which moved `transparent()` into the cross-platform impl block with
|
||||
only the tao call `#[cfg(desktop)]`-fenced. It landed as a clippy cleanup, so the
|
||||
issue was never closed. Separately, the black/white-screen bug (tauri#8381,
|
||||
tauri#9408) was a broken JNI signature for `setBackgroundColor`, fixed in wry
|
||||
0.39.4; we ship wry 0.55.x.
|
||||
|
||||
**Change to:** a comment stating the adapter is currently unreachable because
|
||||
`createAdapter` hardcodes the HTML5 kind, that transparency is no longer an
|
||||
upstream blocker, and that
|
||||
[android-native-video-spike.md](android-native-video-spike.md) tracks whether
|
||||
SurfaceView compositing actually works. Be explicit that *nobody has
|
||||
demonstrated* SurfaceView-behind-WebView on Tauri Android — nothing upstream
|
||||
blocks it, and nothing upstream proves it.
|
||||
|
||||
### 5. Platform capability is signalled three incompatible ways
|
||||
|
||||
Not a doc error — a real inconsistency found during the same investigation, worth
|
||||
recording here even though fixing it needs its own change.
|
||||
|
||||
Which backend a platform uses is currently expressed three ways:
|
||||
|
||||
1. Rust `#[cfg]` gates in `player/mod.rs` and `create_player_backend` — the truth.
|
||||
2. The `useHtml5Element` / `VideoBackend` value from `get_player_status` — which
|
||||
the frontend discards (see the spike spec).
|
||||
3. **Frontend user-agent sniffing** in `src/lib/services/webviewAudio.ts:30-41`:
|
||||
|
||||
```ts
|
||||
const ua = navigator.userAgent.toLowerCase();
|
||||
const isAndroid = ua.includes("android");
|
||||
const isLinux = ua.includes("linux") && !isAndroid;
|
||||
return !isAndroid && !isLinux;
|
||||
```
|
||||
|
||||
The comment says it is "matching the Rust cfg gate" — i.e. the frontend
|
||||
re-derives a backend decision from the user-agent string and hopes it stays in
|
||||
sync. That is the frontend deciding *which backend exists*, which is domain
|
||||
knowledge, not presentation. It also breaks silently the moment a new target is
|
||||
added or a webview's UA changes.
|
||||
|
||||
**This is a boundary leak of the same family the spec-review checklist exists to
|
||||
catch**, even though `check:boundary`'s tripwire (item-type arrays) does not
|
||||
match it. Rust already computes the answer; the frontend should consume it.
|
||||
|
||||
Not fixed by this spec — it is behavioural, not documentation. It should be
|
||||
folded into the spike spec's factory rework, where the same
|
||||
"consume Rust's decision instead of re-deriving it" change is already in scope.
|
||||
|
||||
### Also worth fixing while here
|
||||
|
||||
`requirements.md` IR-004 reads "In Progress (basic playback works, audio settings
|
||||
missing)". That stays accurate until
|
||||
[android-audio-settings-parity.md](android-audio-settings-parity.md) lands, but
|
||||
the "Future Fix" list in the parity issue proposes
|
||||
`ConcatenatingMediaSource` for crossfade — **deprecated in current Media3**. Drop
|
||||
that suggestion; the modern approach is a custom `AudioProcessor`.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
No logic. Documentation and comments only.
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| — | — | No logic introduced or moved by this spec. |
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Implementing crossfade. This spec only stops claiming it exists.
|
||||
- Implementing Android audio settings — see the parity spec.
|
||||
- Running the Android video spike — see that spec.
|
||||
- Rewriting the architecture docs. `docs/architecture/05-platform-backends.md`
|
||||
should be re-read for the same class of error, but that is a larger pass.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] DR-034 status corrected, with the blocking reason stated.
|
||||
- [ ] UR-031 status corrected (line ~44), and the "only work on Linux" line (~517) no longer lists crossfade.
|
||||
- [ ] Parity matrix: crossfade ❌/❌; equalizer row added.
|
||||
- [ ] `ConcatenatingMediaSource` suggestion removed from the "Future Fix" list.
|
||||
- [ ] `nativeAdapter.ts` comment corrected and pointing at the spike spec.
|
||||
- [ ] `bun run check` and `bun run test` pass (a comment change still touches TS).
|
||||
- [ ] `bun run traces:markdown` re-run if requirement text changed.
|
||||
|
||||
No Rust changes, so the `cargo` gates do not apply.
|
||||
|
||||
## Testing
|
||||
|
||||
None beyond the standard gates — no behaviour changes. Confirm
|
||||
`bun run traces:markdown` regenerates cleanly, since DR-034's row is referenced
|
||||
by the traceability matrix.
|
||||
|
||||
## TRACES
|
||||
|
||||
No code implementing requirements changes; no TRACES comments to add or update.
|
||||
The DR-034 row in `docs/traceability.md` will regenerate with the corrected text.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- Do **not** silently delete DR-034. The requirement (UR-031 crossfade) is still
|
||||
wanted; it is the *status* that is wrong. Keeping the row with an honest status
|
||||
and a reason is the point.
|
||||
- A parallel Claude session may be active — `git diff` before "repairing"
|
||||
unexpected changes.
|
||||
@@ -1,7 +1,12 @@
|
||||
# Spec: Enforce the unified player boundary
|
||||
|
||||
**Status:** Proposed
|
||||
**Requirements:** DR-095 (new); relates to UR-005 and 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
|
||||
|
||||
@@ -1,10 +1,22 @@
|
||||
# Spec: Two-path media — selectable playback bitrate, independent whole-file download
|
||||
|
||||
**Status:** Proposed
|
||||
**Status:** Partially implemented. Landed: the cache/download unification
|
||||
(DR-126, DR-127 — a cache entry *is* a `downloads` row with a shorter life, and
|
||||
eviction only reclaims the temporary tier), local playback of downloaded media
|
||||
(DR-128), and the one-path/one-row invariants that followed (DR-133 … DR-138).
|
||||
DR-123 is in progress. Still open: the **player quality selector** and the
|
||||
read-through capture itself — DR-121, DR-122, DR-124, DR-125. The separate
|
||||
settings-level bitrate cap (DR-162, shipped —
|
||||
[01-rust-backend.md](../architecture/01-rust-backend.md#streaming-quality-ladder))
|
||||
covers a *settings-level*
|
||||
ceiling (DR-162), which serves part of UR-070 but is not the per-playback
|
||||
selector specified here.
|
||||
**Requirements:** UR-070, UR-071 → DR-121, DR-122, DR-123, DR-124, DR-125; IR-032
|
||||
**UX spec:** player quality selector — needs a `ux-flows.md` section before build
|
||||
**Related:** [catalog-index-search.md](catalog-index-search.md),
|
||||
[downloads-as-offline-library.md](downloads-as-offline-library.md)
|
||||
**Related:** the locally-indexed search and downloaded-browse work, both
|
||||
shipped — see
|
||||
[03-data-flow.md](../architecture/03-data-flow.md) and
|
||||
[06-downloads-and-offline.md](../architecture/06-downloads-and-offline.md)
|
||||
|
||||
## Summary
|
||||
|
||||
|
||||
@@ -1,161 +0,0 @@
|
||||
# Spec: Remove the broken `check-req-coverage.sh`
|
||||
|
||||
**Status:** Implemented
|
||||
**Requirements:** supports DR-093 (see [traceability-gate-repair.md](traceability-gate-repair.md))
|
||||
**UX spec:** n/a — developer tooling.
|
||||
**Supersedes / revises:** n/a
|
||||
|
||||
## Summary
|
||||
|
||||
`scripts/check-req-coverage.sh` is broken, orphaned, and actively misleading: it
|
||||
reports `Total Requirements: 1`, zeros in every category, and then prints
|
||||
**"✨ All requirements have implementations!"**. Nothing references it — not CI,
|
||||
not `package.json`, not the docs. This spec deletes it, with a narrowly-scoped
|
||||
alternative (repair it) documented and rejected below.
|
||||
|
||||
## Motivation
|
||||
|
||||
Running it today produces:
|
||||
|
||||
```
|
||||
Category Breakdown:
|
||||
UR: 0 requirements
|
||||
IR: 0 requirements
|
||||
DR: 0 requirements
|
||||
JA: 0 requirements
|
||||
|
||||
Summary:
|
||||
Total Requirements: 1
|
||||
✅ Fully Implemented: 0 (0%)
|
||||
|
||||
✨ All requirements have implementations!
|
||||
```
|
||||
|
||||
Every number is wrong (the real totals are UR 61, IR 29, DR 89, JA 32), and the
|
||||
concluding message is the *opposite* of a warning — a developer running this to
|
||||
sanity-check coverage is told everything is fine.
|
||||
|
||||
This is worse than having no script. It is a trap, and it sits in `scripts/`
|
||||
next to tools that do work, with nothing marking it as dead.
|
||||
|
||||
Verification that it is genuinely orphaned:
|
||||
|
||||
```console
|
||||
$ grep -rn "check-req-coverage" . --include='*.yml' --include='*.json' \
|
||||
--include='*.sh' --include='*.md' | grep -v node_modules
|
||||
(no output)
|
||||
```
|
||||
|
||||
## Layer assignment
|
||||
|
||||
Developer tooling only; no application logic and nothing crosses the IPC
|
||||
boundary.
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Requirement-coverage reporting | Build tooling — `extract-traces.ts` | One tool should own coverage analysis. A second, divergent implementation is how the two answers ("1 requirement" vs "211") came to disagree unnoticed. |
|
||||
|
||||
## Design
|
||||
|
||||
**Delete `scripts/check-req-coverage.sh`.**
|
||||
|
||||
Coverage reporting is owned by [scripts/extract-traces.ts](../../scripts/extract-traces.ts),
|
||||
which is correct, is what CI runs, and gains a first-class local coverage mode
|
||||
in [traceability-gate-repair.md](traceability-gate-repair.md):
|
||||
|
||||
```bash
|
||||
bun run traces:coverage # the supported way to check coverage locally
|
||||
```
|
||||
|
||||
Then check the sibling scripts for the same rot. `scripts/` also contains
|
||||
`check-test-coverage.sh` and `find-req-implementations.sh`, neither of which is
|
||||
referenced from `package.json`. An unreferenced script is never run and so rots
|
||||
silently — that is the actual failure mode being fixed here, and fixing only the
|
||||
one instance found by audit leaves the others to be rediscovered later.
|
||||
|
||||
### Findings (investigation, 2026-07)
|
||||
|
||||
All three scripts turned out to share a **single root cause**, and all three are
|
||||
deleted:
|
||||
|
||||
| Script | Defect |
|
||||
|---|---|
|
||||
| `check-req-coverage.sh` | Reads `README.md`, which has held **zero** requirement rows since they moved to `docs/requirements.md` → `total_reqs=1`, every category 0, "✨ All requirements have implementations!" Also greps `src-tauri/` unscoped. |
|
||||
| `check-test-coverage.sh` | Greps `src-tauri/` unscoped — including **40 GB** of `target/` build artifacts. Hangs indefinitely; produces no output at all. |
|
||||
| `find-req-implementations.sh` | Same unscoped `src-tauri/` grep. Same hang. |
|
||||
|
||||
So none of them were subtly wrong — two could never terminate, and the third
|
||||
inverted its own conclusion.
|
||||
|
||||
They were nonetheless *salvageable*: scoping the greps to `src-tauri/src` and
|
||||
repointing at `docs/requirements.md` would be a few lines, and the `@req:` /
|
||||
`@req-test:` tags they read are still present in the tree (**146** and **76**
|
||||
occurrences).
|
||||
|
||||
**Decision: delete all three anyway.** The tags are an undocumented parallel
|
||||
convention — `@req:` appears in no doc, and CLAUDE.md describes only `TRACES:`.
|
||||
Repairing the scripts would re-establish a second traceability system to keep in
|
||||
sync with the first, which is the same two-sources-of-truth condition that let
|
||||
"1 requirement" and "211 requirements" coexist unnoticed. `TRACES:` plus the
|
||||
repaired coverage engine ([traceability-gate-repair.md](traceability-gate-repair.md))
|
||||
already cover this ground.
|
||||
|
||||
The existing `@req:` / `@req-test:` comments are left in place: they are
|
||||
harmless as prose, several encode genuinely useful test intent, and stripping
|
||||
222 comments across the tree is a large diff with no functional gain. They are
|
||||
simply no longer read by any tool.
|
||||
|
||||
### Alternative considered: repair rather than delete
|
||||
|
||||
Rejected. The script's output format duplicates what `traces:markdown` already
|
||||
generates, it has no tests, no caller, and no documented purpose distinct from
|
||||
`extract-traces.ts`. Repairing it recreates the two-sources-of-truth condition
|
||||
that produced the contradiction. If a shell-based coverage check is ever wanted,
|
||||
it should shell out to `traces:json` and `jq` rather than re-parse
|
||||
`requirements.md` independently.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- The CI workflow denominators — [traceability-gate-repair.md](traceability-gate-repair.md).
|
||||
- Any change to `extract-traces.ts`'s output (that spec owns it).
|
||||
- Auditing scripts that *are* referenced from `package.json` — they run
|
||||
regularly and would fail visibly.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] `scripts/check-req-coverage.sh` no longer exists.
|
||||
- [ ] `grep -rn "check-req-coverage" .` (excluding `node_modules` and this spec)
|
||||
returns nothing — no dangling reference in CI, docs, or `package.json`.
|
||||
- [ ] `scripts/check-test-coverage.sh` and `find-req-implementations.sh` have each
|
||||
been run and either wired into `package.json` or deleted; the decision and
|
||||
reason are recorded in `scripts/README.md`. **Outcome: all three deleted —
|
||||
see Findings.**
|
||||
- [ ] `scripts/README.md` documents `bun run traces:coverage` as the supported
|
||||
way to check requirement coverage locally.
|
||||
- [ ] `bun run test:all` passes (confirms nothing invoked the deleted script).
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
- [ ] `bun run check:boundary` passes.
|
||||
|
||||
## Testing
|
||||
|
||||
No unit tests — this is a deletion. Verification is the grep in the acceptance
|
||||
criteria plus a green `bun run test:all`, which exercises the script paths that
|
||||
actually run.
|
||||
|
||||
## TRACES
|
||||
|
||||
No new requirement. The deletion is covered by **DR-093**
|
||||
([traceability-gate-repair.md](traceability-gate-repair.md)), which establishes
|
||||
`extract-traces.ts` as the single owner of coverage reporting. Note the removal
|
||||
in that DR's text when both land.
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- A parallel Claude session may be active in this repo — `git diff` before
|
||||
"repairing" unexpected changes (CLAUDE.md §Gotchas).
|
||||
- Land this **after** or alongside [traceability-gate-repair.md](traceability-gate-repair.md),
|
||||
so `bun run traces:coverage` exists before the broken script is removed and
|
||||
developers are never left without a coverage command.
|
||||
- Check `docs/traceability-ci.md` and `docs/traces-quick-ref.md` for prose
|
||||
references to the deleted script; the grep above covers `.md`, but read the
|
||||
surrounding sentence rather than deleting the line mechanically.
|
||||
@@ -49,7 +49,7 @@ audit:
|
||||
2. **The tripwire cannot see it.** `bun run check:boundary` passes — it greps for
|
||||
a multi-type array literal *at the query site*, and this one is assigned to a
|
||||
named const and dereferenced elsewhere. Broadening the tripwire is specified
|
||||
separately in [boundary-tripwire-hardening.md](boundary-tripwire-hardening.md);
|
||||
separately by the tripwire hardening (DR-094, shipped);
|
||||
note that hardening it **without** landing this fix would turn `master` red.
|
||||
3. **The spec's own acceptance criterion fails today.** "Adding a hypothetical
|
||||
new type to a scope requires editing only Rust" — adding a type to the Music
|
||||
@@ -96,7 +96,7 @@ the `search-event` dual-payload hazard in the middle. Split it:
|
||||
`SCOPE_ITEM_TYPES` and `scopeItemTypes()`. Result grouping stays as it is.
|
||||
|
||||
After Stage 1 the actual boundary violation is gone and
|
||||
[boundary-tripwire-hardening.md](boundary-tripwire-hardening.md) can land safely.
|
||||
the hardened tripwire (DR-094) can pass.
|
||||
|
||||
**Stage 2 — result side.** `SearchGroupId`/`SearchGroup`/`GroupedSearchResult`,
|
||||
Rust bucketing, both payloads converted, `composeSearchGroups()` shrunk,
|
||||
@@ -160,7 +160,7 @@ hand-edit it.
|
||||
- Chip UX, scope persistence, group-order persistence — unchanged.
|
||||
- The two lesser type-set sites in `DownloadedBrowse.svelte` and
|
||||
`GenericMediaListPage.svelte`, handled in
|
||||
[boundary-tripwire-hardening.md](boundary-tripwire-hardening.md).
|
||||
the hardened tripwire (DR-094, see `scripts/check-frontend-boundary.sh`).
|
||||
- Broadening the tripwire itself — same sibling spec.
|
||||
|
||||
## Acceptance criteria
|
||||
@@ -244,7 +244,7 @@ take `@req-test: UT-089` onward (next free UT is **UT-089**).
|
||||
- **Read [scoped-search-boundary.md](scoped-search-boundary.md) first.** This
|
||||
spec is deliberately thin on design; that one is the authority.
|
||||
- Sequence with the sibling specs: **Stage 1 here → then
|
||||
[boundary-tripwire-hardening.md](boundary-tripwire-hardening.md)**. Hardening
|
||||
the hardened tripwire (DR-094)**. Hardening
|
||||
the tripwire first turns `master` red on a known-unfixed violation.
|
||||
- `git log --oneline -- docs/specs/scoped-search-boundary.md` is worth a look
|
||||
before starting — understanding why the fix stalled may surface a constraint
|
||||
|
||||
@@ -1,6 +1,13 @@
|
||||
# Spec: Move search scope taxonomy behind the Rust boundary
|
||||
|
||||
**Status:** Proposed
|
||||
**Status:** Design authority — **Stage 1 implemented**, Stage 2 outstanding.
|
||||
The scope→item-type mapping now lives in Rust (`SearchScope::item_types()` in
|
||||
`repository/types.rs`, DR-063 … DR-067). The *result-side* grouping table
|
||||
(`GROUP_ITEM_TYPES` in `src/lib/utils/searchScope.ts`) is still in the
|
||||
frontend, and `check:boundary` does not match its shape. Delivery status and
|
||||
the remaining work live in
|
||||
[scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md);
|
||||
this spec remains the design authority.
|
||||
**Scope:** Rust + Frontend. **Revises a decision in
|
||||
[scoped-search.md](scoped-search.md).**
|
||||
**Requirements:** UR-049, UR-050 (existing) → new DRs for the boundary move
|
||||
|
||||
@@ -1,239 +0,0 @@
|
||||
# Spec: series navigation lands on the current episode
|
||||
|
||||
**Status:** Accepted
|
||||
**Requirements:** UR-062 → DR-101, DR-102, DR-103, DR-104, DR-107; UR-063 → DR-105; UR-064 → DR-106
|
||||
**UX spec:** [ux-flows.md §5B.1](../ux-flows.md), [§5B.2](../ux-flows.md), [§5B.4](../ux-flows.md), [§5B.5](../ux-flows.md)
|
||||
|
||||
## Summary
|
||||
|
||||
Opening a TV series lands you where you actually are in it: the seasons render
|
||||
as collapsible sections with **only the current season expanded**, the current
|
||||
episode highlighted and scrolled into view, and the hero button opens that
|
||||
episode's focus view (labelled `Resume S2E4` / `Play S1E1`) instead of the first
|
||||
season. A season stops being a destination of its own — every route that used to
|
||||
land on `/library/<seasonId>` now lands on the series with that season in view,
|
||||
so the full cross-season episode list is always reachable in one place. Watch
|
||||
history can be erased per series and per season. Separately, each video library
|
||||
collapses from three routes (landing, all-titles, genres) to one route with
|
||||
in-page tabs.
|
||||
|
||||
## Motivation
|
||||
|
||||
Two problems, reported together.
|
||||
|
||||
**1. Series navigation dead-ends at season 1.** The series detail page's Play
|
||||
button resolved its target as `$libraryItems[0]` — the first *season* child,
|
||||
ordered by `SortName` — and navigated to `/player/<seasonId>`. The player route
|
||||
classifies `season` as a container kind and bounces it back to
|
||||
`/library/<seasonId>`. So Play on a series played nothing; it navigated you to
|
||||
the season-1 page. Opening a series without pressing Play rendered every season
|
||||
stacked but scrolled to the top, so a viewer 4 seasons deep had to scroll past
|
||||
everything they had already watched.
|
||||
|
||||
The backend has been able to answer "where is this viewer in this show" the
|
||||
whole time: `repository_get_next_up_episodes(handle, series_id, limit)` is wired
|
||||
end-to-end to `/Shows/NextUp?SeriesId=`. **Both frontend call sites pass
|
||||
`undefined` for `series_id`** — the per-series capability existed and was never
|
||||
used.
|
||||
|
||||
**2. Seasons are an accidental page.** There is no season route. `/library/
|
||||
<seasonId>` falls through the detail page's `kind` chain into the generic
|
||||
"Contents" poster grid, which contradicts ux-flows §5A.2 (episodes in a season
|
||||
must render as a row list). Worse, clicking an episode from that grid opens a
|
||||
*bare* Episode page, which §5B.1 explicitly forbids. Four call sites fed it: the
|
||||
episode breadcrumb, `handleItemClick case "season"`, the TV landing page, and
|
||||
the broken Play button above.
|
||||
|
||||
**3. Too many video library routes.** Seven routes serve two media types, and the
|
||||
naming does not even agree with itself: `/library/tv` + `/library/tv/shows` +
|
||||
`/library/shows/genres` versus `/library/movies` + `/library/movies/all` +
|
||||
`/library/movies/genres`. The genre routes do not share a prefix, which
|
||||
`searchScope.ts:45` carries an apologetic comment about. The two "all" pages are
|
||||
27-line config wrappers over the same `GenericMediaListPage`.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Which episode is "current" for a series (resume → next-up → first unwatched → first) | **Rust** | Domain policy over Jellyfin user-data semantics. It changes if Jellyfin changes what `UserData.is_played` means, if Next Up's rules change, or if we decide a 98%-watched episode counts as finished. It does not change if the UI is redesigned. |
|
||||
| Gathering a series' episodes across all seasons in broadcast order | **Rust** | Jellyfin's shape (episodes hang off season folders, except when a series is flat and they hang off the series) is provider vocabulary. The frontend already reimplemented this fan-out *and* its flat-series fallback; that is domain knowledge that leaked. |
|
||||
| Ordering rule for "series order" (season index, then episode index, specials last) | **Rust** | Season 0 = specials is a Jellyfin convention, not a layout choice. |
|
||||
| Scrolling the current episode into view; the highlight ring and `Up next` badge | Frontend | Pure presentation. Changes only if the page is redesigned. |
|
||||
| Which seasons start expanded | Frontend | Consumes the backend's answer (`currentEpisode`) to decide layout. The *decision* about where the viewer is stays in Rust; only "and therefore this section opens" is here. |
|
||||
| What "erase watch history" means (played flag + resume position, recursive over a container) | **Rust** | Jellyfin user-data semantics. Changes if the server's mark-unplayed behaviour changes; unaffected by any UI redesign. |
|
||||
| Refusing to clear history while offline | **Rust** | A data-integrity rule, not a disabled button: history cleared only locally would be undone by the next sync. The UI disabling the button is a courtesy on top. |
|
||||
| Play button *label* (`Resume S2E4` vs `Play S1E1`) | Frontend | Rendering a decision the backend already made (the returned episode plus its resume position). |
|
||||
| Which route Play navigates to | Frontend | Navigation is presentation. |
|
||||
| Redirecting `/library/<seasonId>` to the series anchor | Frontend | Route topology. |
|
||||
| Episode-strip window size (3 before / 6 after) | Frontend | A layout constant; §5B.2 owns it. |
|
||||
| Library page tabs and the `?view=` param | Frontend | View preference and route topology. |
|
||||
|
||||
Borderline row — **the strip's cross-season *ordering*** is Rust (it is series
|
||||
order, above), but the *window* taken from that ordered list is frontend. The
|
||||
tie-breaker: the list handed to the frontend is already correct and complete;
|
||||
choosing how much of it fits on screen is layout.
|
||||
|
||||
## Design
|
||||
|
||||
### Rust: the current-episode policy
|
||||
|
||||
Two new pieces, split so the policy is unit-testable without a repository.
|
||||
|
||||
**Pure policy** — `src-tauri/src/repository/series_progress.rs`:
|
||||
|
||||
```rust
|
||||
/// Series order: season index asc, then episode index asc. Specials (season 0)
|
||||
/// sort after every numbered season rather than before season 1.
|
||||
pub fn sort_series_order(episodes: &mut [MediaItem]);
|
||||
|
||||
/// The episode a viewer should land on, given everything already fetched.
|
||||
/// Order: in-progress episode → Next Up → first unwatched → first episode.
|
||||
pub fn pick_current_episode(
|
||||
episodes: &[MediaItem], // series order
|
||||
next_up: &[MediaItem],
|
||||
resume: &[MediaItem],
|
||||
) -> Option<MediaItem>;
|
||||
```
|
||||
|
||||
Why that order:
|
||||
|
||||
- **In-progress wins** because a partially-watched episode is literally where
|
||||
the viewer stopped; Next Up would skip past it. Ties break toward the earliest
|
||||
in series order, so a viewer who dipped into a later episode still resumes the
|
||||
one they are actually working through.
|
||||
- **Next Up second** because it is the server's own answer, and it accounts for
|
||||
history we do not cache.
|
||||
- **First unwatched third** — the offline repository returns an empty vec for
|
||||
Next Up (`offline.rs:1247`), so without this fallback the whole feature would
|
||||
be online-only. This is the offline path, not dead code.
|
||||
- **First episode last** so a never-watched series lands on S1E1 rather than
|
||||
nothing.
|
||||
|
||||
A `resume`/`next_up` entry that is not among `episodes` is still honoured — it
|
||||
comes from the same server and may carry an id the season fan-out missed — but
|
||||
it must belong to this series.
|
||||
|
||||
**Fetch + command** — `src-tauri/src/commands/repository.rs`:
|
||||
|
||||
```rust
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
pub async fn repository_get_series_episodes(
|
||||
manager: State<'_, RepositoryManagerWrapper>,
|
||||
handle: String,
|
||||
series_id: String,
|
||||
) -> Result<Vec<MediaItem>, String>
|
||||
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
pub async fn repository_get_series_current_episode(
|
||||
manager: State<'_, RepositoryManagerWrapper>,
|
||||
handle: String,
|
||||
series_id: String,
|
||||
) -> Result<Option<MediaItem>, String>
|
||||
```
|
||||
|
||||
Frontend params are camelCase (`{ handle, seriesId }`) per the Tauri v2 rule.
|
||||
|
||||
`repository_get_series_episodes` performs the fan-out the frontend used to do:
|
||||
`get_items(series_id)` → seasons → `get_items(season_id)` per season, plus the
|
||||
flat-series fallback (a series whose children are episodes, not seasons), then
|
||||
`sort_series_order`. `repository_get_series_current_episode` calls it, adds
|
||||
`get_next_up_episodes(Some(series_id), Some(1))` and
|
||||
`get_resume_items(Some(series_id), Some(10))`, and applies `pick_current_episode`.
|
||||
Both tolerate a failing Next Up (offline) by treating it as empty rather than
|
||||
failing the whole call.
|
||||
|
||||
### Frontend: series page
|
||||
|
||||
- `loadItem()` calls `repositoryGetSeriesEpisodes` once instead of fanning out
|
||||
over seasons itself, and `repositoryGetSeriesCurrentEpisode` for the anchor.
|
||||
Season *headers* still come from `get_items(seriesId)`; the page groups the
|
||||
returned episodes under them by `parentIndexNumber`.
|
||||
- No `?episode=` param → series view, `SeasonSection` receives
|
||||
`currentEpisodeId`, `EpisodeRow` renders the highlight and scrolls itself into
|
||||
view (`scrollIntoView({ block: "center" })`, the existing `focused` mechanism,
|
||||
now distinguishing *focused* from *current*).
|
||||
- Seasons are collapsible and **only the current season is expanded**
|
||||
(`initialExpandedSeasons`). Without this a ten-season show renders every
|
||||
episode of every season at once and buries the one the viewer came for. A
|
||||
collapsed season still shows its episode count and watched count, so progress
|
||||
is legible without expanding. Toggle state is local and not persisted — it is
|
||||
a reading position, not a preference.
|
||||
- Hero Play → `goto(/library/<seriesId>?episode=<currentId>)`, i.e. the Episode
|
||||
Focus View, where an explicit Play/Resume starts playback. This follows
|
||||
ux-flows §5B.5's "tap opens, never commits" rule: Play on a *container* is
|
||||
navigation; Play on a *leaf* (the focus view, a movie) commits.
|
||||
- Clicking an episode in a season section → `?episode=` swap, not
|
||||
`/player/<id>`. §5B.1.
|
||||
|
||||
### Frontend: seasons are not a destination
|
||||
|
||||
`/library/<seasonId>` resolves the season's `seriesId` and redirects to
|
||||
`/library/<seriesId>#season-<indexNumber>`; `SeasonSection` renders that anchor
|
||||
id. A season with no `seriesId` (deep link into a stale cache) keeps the old
|
||||
generic rendering as a fallback so the user is never stranded. Inbound links
|
||||
updated: episode breadcrumb, `handleItemClick case "season"`, the TV landing
|
||||
page's `case "Season"`, and `DownloadedBrowse`.
|
||||
|
||||
### Erasing watch history
|
||||
|
||||
```rust
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
pub async fn repository_clear_watch_history(
|
||||
manager: State<'_, RepositoryManagerWrapper>,
|
||||
handle: String,
|
||||
item_id: String,
|
||||
) -> Result<(), String>
|
||||
```
|
||||
|
||||
`OnlineRepository` maps it to `DELETE /Users/{userId}/PlayedItems/{itemId}` —
|
||||
Jellyfin's mark-unplayed, which clears the played flag *and* zeroes the resume
|
||||
position, and which the server applies recursively to a folder. One call
|
||||
therefore handles a whole series or a single season; no per-episode fan-out.
|
||||
`OfflineRepository` returns `RepoError::Offline` rather than clearing locally,
|
||||
because divergent local history is undone by the next sync.
|
||||
|
||||
`ClearHistoryButton` is shared by the series hero (`scope="series"`) and each
|
||||
`SeasonSection` header (`scope="season"`). It confirms first — there is no undo —
|
||||
disables itself while the server is unreachable, and reloads the page on success
|
||||
so the recomputed current episode is what the viewer sees. Clearing a whole
|
||||
series therefore returns it to S1E1, which is the same path a never-watched
|
||||
series takes through `pick_current_episode`.
|
||||
|
||||
### Frontend: one route per video library
|
||||
|
||||
`/library/tv` and `/library/movies` each gain `?view=browse|all|genres` tabs,
|
||||
rendering the existing `GenericMediaListPage` / `GenericGenreBrowser` components
|
||||
inline. `?view=` is omitted for `browse` (the default) to keep URLs clean —
|
||||
the same convention `searchRouteUrl` uses for the `all` scope.
|
||||
|
||||
The four legacy routes become redirect-only `+page.ts` loads:
|
||||
|
||||
| Legacy | Redirects to |
|
||||
|--------|--------------|
|
||||
| `/library/tv/shows` | `/library/tv?view=all` |
|
||||
| `/library/shows/genres` | `/library/tv?view=genres` |
|
||||
| `/library/movies/all` | `/library/movies?view=all` |
|
||||
| `/library/movies/genres` | `/library/movies?view=genres` |
|
||||
|
||||
They are kept (rather than deleted) because `GenreTags` builds links to them and
|
||||
users may have them in history. `resolveSearchScope` keeps its `/library/shows`
|
||||
branch for the same reason.
|
||||
|
||||
The "Browse" tile grid at the bottom of both landing pages is removed — the tabs
|
||||
replace it, and the tiles were a second navigation affordance to the same two
|
||||
destinations the carousels' "Show all" links already reach.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **Cross-season autoplay.** `player/mod.rs:fetch_next_episode_for_item` is
|
||||
still season-bounded, so autoplay stops at a season boundary. Fixing it should
|
||||
reuse `repository_get_series_episodes`, but it touches the playback state
|
||||
machine and the Android JNI advance path (see the `AutoplayDecision` deadlock
|
||||
note in CLAUDE.md) and belongs in its own change.
|
||||
- **Music library routes.** `/library/music/*` has five sub-routes with the same
|
||||
shape; the same consolidation applies but is not done here.
|
||||
- **Marking a series' progress** (mark-watched / mark-unwatched from the series
|
||||
page).
|
||||
@@ -1,146 +0,0 @@
|
||||
# Spec: streaming bitrate cap
|
||||
|
||||
**Status:** Implemented
|
||||
**Requirements:** UR-074 → DR-162 (partially serves UR-070)
|
||||
**UX spec:** n/a — the controls reuse existing patterns (Settings → Video Playback, and the player's track menus).
|
||||
|
||||
## Summary
|
||||
|
||||
The viewer picks a bandwidth ceiling for video — from `Original` (no client
|
||||
limit) down to 720 kbps — and every video the app opens is fetched within it,
|
||||
live TV included. The choice is made once in Settings and persists across
|
||||
restarts; a single video can be moved to another ceiling from the player, which
|
||||
re-opens the stream and resumes where it was without changing the saved default.
|
||||
|
||||
## Motivation
|
||||
|
||||
Every video URL the app built carried a fixed allowance —
|
||||
`MaxStreamingBitrate=20000000`, `VideoBitrate=18000000` — the `PlaybackInfo`
|
||||
negotiation asked for 20 Mbps, and the device profile advertised
|
||||
`999999999`, which invites the server to direct-play a source of any size. On a
|
||||
metered or slow connection there was no lever at all short of not watching.
|
||||
|
||||
The related UR-070 asks for something adjacent but different: a list of the
|
||||
renditions *the server can produce for this item*. That needs per-item
|
||||
`MediaSources` negotiation and is still proposed. What was missing first is
|
||||
cruder and more valuable: a device-wide budget that holds regardless of what is
|
||||
playing.
|
||||
|
||||
## Layer assignment
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| What a quality step *is* — total ceiling, audio share, resolution cap | Rust | Jellyfin encoding vocabulary. It changes if Jellyfin's transcoder or parameter binding changes, not if the UI is redesigned. Exactly the shape of `EqPreset::gains()`. |
|
||||
| Splitting the ceiling between video and audio | Rust | A domain rule about what the server is being asked to produce; getting it wrong overshoots the user's cap. |
|
||||
| Choosing `MaxHeight` for a bitrate | Rust | An encoding judgement (how many pixels a budget can carry), not a display preference. |
|
||||
| Where the cap is applied (URL builders, `PlaybackInfo`, live TV, audio handoff) | Rust | All four are backend concerns, and the frontend must not have to know that a cap has more than one enforcement point. |
|
||||
| Whether a mid-playback change needs a stream reload, and performing it | Rust | Same decision the audio-track switch already delegates: the backend knows the playback mode and owns the queue. |
|
||||
| Persisting the default | Rust | Application state in `app_settings`, alongside every other durable setting. |
|
||||
| Rendering the picker, menu placement, which control is highlighted | Frontend | Pure presentation. |
|
||||
|
||||
The frontend holds one string — the serde token for the chosen variant — and
|
||||
labels/details it received from Rust. It never encodes a bitrate, a resolution
|
||||
or a parameter name.
|
||||
|
||||
## Design
|
||||
|
||||
`StreamingQuality` (`src-tauri/src/settings.rs`) is the ladder: `Original`,
|
||||
`Mbps20`, `Mbps10`, `Mbps8`, `Mbps4`, `Mbps2`, `Mbps1`, `Kbps720`, serialised
|
||||
camelCase (`"mbps10"`). Each step answers `max_bitrate()`, `audio_bitrate()`,
|
||||
`video_bitrate()` (= total − audio), `max_height()`, `label()`, `detail()`.
|
||||
|
||||
The active ceiling is a process-wide `RwLock<StreamingQuality>` in
|
||||
`repository/online.rs`, read by every builder. Process-wide rather than a field
|
||||
on `OnlineRepository` because it is a preference about *this device's
|
||||
connection*: it must survive a repository rebuilt on re-login, and the URL
|
||||
builders and the negotiation have to agree on it or the cap leaks. This mirrors
|
||||
`offline::INCLUDE_CATALOG_BROWSE`.
|
||||
|
||||
Enforcement points — all four are required:
|
||||
|
||||
| Point | What the cap sets |
|
||||
|-------|-------------------|
|
||||
| `get_video_stream_url` (HLS transcode) | `MaxStreamingBitrate`, `VideoBitrate`, `AudioBitrate`, `MaxHeight` |
|
||||
| `get_playback_info` | request `MaxStreamingBitrate`, and the device profile's `MaxStreamingBitrate`/`MaxStaticBitrate` |
|
||||
| `open_live_stream` | `MaxStreamingBitrate` |
|
||||
| `build_audio_only_stream_url_for_video` | `min(cap audio, 384 kbps)` |
|
||||
|
||||
The negotiation is the one that matters most. `MaxStaticBitrate` is what makes
|
||||
the server refuse to *direct play* a source fatter than the ceiling; without it
|
||||
a 30 Mbps remux is served untouched and no URL parameter downstream can reduce
|
||||
it.
|
||||
|
||||
IPC:
|
||||
|
||||
```rust
|
||||
player_get_streaming_qualities() -> Vec<(StreamingQuality, String, String)> // variant, label, detail
|
||||
player_set_stream_quality(repository_handle, quality, use_html5,
|
||||
current_position, media_source_id, audio_stream_index)
|
||||
-> StreamQualityResponse // #[serde(tag = "strategy")]: native | reloadStream
|
||||
```
|
||||
|
||||
`VideoSettings` gains `streaming_quality` (`#[serde(default)]`, so settings
|
||||
persisted before the field existed load as uncapped).
|
||||
`player_set_video_settings` applies it and writes it to `app_settings`;
|
||||
`restore_streaming_quality` reads it back in the Tauri `setup` hook via
|
||||
`tauri::async_runtime::spawn`, defaulting to uncapped if anything fails.
|
||||
|
||||
`StreamQualityResponse` keeps its Rust field names on the wire (`new_url`) —
|
||||
tauri-specta only camelCases the `strategy` tag. The facade
|
||||
(`playerController.setStreamQuality`) dispatches `reloadSource` for
|
||||
`reloadStream` and does nothing for `native`, because the backend has already
|
||||
reloaded itself.
|
||||
|
||||
Mid-playback the change applies to the current video **and** becomes the process
|
||||
ceiling for what follows, but it is not persisted: the in-player menu is a "this
|
||||
film, this connection" control and Settings owns the durable default.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Per-item rendition lists from the server's `MediaSources` (UR-070's other half).
|
||||
- Connection-aware caps (separate WiFi/cellular ceilings). One cap, all connections.
|
||||
- Adaptive/automatic selection from measured throughput.
|
||||
- Download quality, which already has its own preset vocabulary (UR-071/DR-123).
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [x] `bun run check` passes.
|
||||
- [x] `cargo fmt` clean, `cargo clippy` clean, Rust tests pass.
|
||||
- [x] `bun run test` passes.
|
||||
- [x] `bun run check:boundary` passes — no bitrate/resolution numbers in `src/`.
|
||||
- [x] New code carries `// TRACES:` comments.
|
||||
- [x] `bindings.ts` regenerated from Rust.
|
||||
- [x] A capped step changes what the URL asks for; the uncapped default is byte-identical to the previous behaviour.
|
||||
|
||||
## Testing
|
||||
|
||||
Rust (`cargo test`):
|
||||
|
||||
- `test_video_stream_url_applies_bitrate_cap` — all four parameters at `Mbps2`.
|
||||
- `test_video_stream_url_uncapped_keeps_legacy_allowance` — `Original` is unchanged and adds no `MaxHeight`.
|
||||
- `test_audio_only_stream_url_takes_the_lower_of_cap_and_default`.
|
||||
- `test_streaming_quality_budget_is_internally_consistent`, `..._ladder_descends`, `..._round_trips_through_json`.
|
||||
|
||||
The ceiling is process-wide, so tests that depend on it serialise on a guard
|
||||
(`QualityFixture`) that restores `Original` on drop — including the two
|
||||
pre-existing stream-URL tests, which would otherwise see another test's cap.
|
||||
|
||||
`get_playback_info` and `open_live_stream` need a live server and are not unit
|
||||
tested; their behaviour is the enum's `max_bitrate()`, which is.
|
||||
|
||||
## TRACES
|
||||
|
||||
- `StreamingQuality`, `VideoSettings.streaming_quality` — `UR-074 | DR-162`
|
||||
- URL builders / negotiation / live TV — `UR-004, UR-074 | DR-140, DR-162`
|
||||
- Audio-only handoff — `UR-040, UR-074 | DR-162`
|
||||
- Commands, facade, Settings UI, player menu — `UR-074 | DR-162`
|
||||
- Tests — `UT-156`, `UT-157`
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- `videoBitRate` with a capital R is the *download* endpoint's binding quirk
|
||||
(DR-123). The streaming endpoint used here binds `VideoBitrate`/
|
||||
`MaxStreamingBitrate` as spelled above — do not "correct" one to the other.
|
||||
- A parallel Claude session may be active in this repo; `git diff` before
|
||||
repairing unexpected changes. DR-160/161 were claimed by such a session while
|
||||
this feature was in flight, which is why it is DR-162.
|
||||
@@ -1,238 +0,0 @@
|
||||
# Spec: Repair the traceability coverage gate
|
||||
|
||||
**Status:** Implemented
|
||||
**Requirements:** DR-093 → supports the traceability practice described in CLAUDE.md
|
||||
**UX spec:** n/a — developer tooling, no user-facing surface.
|
||||
**Supersedes / revises:** n/a
|
||||
|
||||
## Summary
|
||||
|
||||
The CI traceability gate has been passing unconditionally for an unknown length
|
||||
of time because it divides traced-requirement counts by **hardcoded denominators
|
||||
that no longer match [requirements.md](../requirements.md)**. It currently
|
||||
reports **158% overall coverage** (and `JA 24 / 3 = 800%`), so the 50% threshold
|
||||
is mathematically unreachable and the job cannot fail. This spec makes the gate
|
||||
derive its denominators from `requirements.md` at run time, so it reports the
|
||||
real number (**85%** today) and can actually fail again.
|
||||
|
||||
## Motivation
|
||||
|
||||
`.gitea/workflows/traceability-check.yml` hardcodes `UR/39, IR/24, DR/48, JA/3`
|
||||
and `TOTAL_REQS=114`. The real counts are **UR 61, IR 29, DR 89, JA 32 — 211
|
||||
total**. Requirements were added over time; the divisors were never updated.
|
||||
|
||||
The consequence is not a cosmetic reporting bug. The gate is the *only*
|
||||
automated defence for the traceability practice, and it is dead:
|
||||
|
||||
```
|
||||
CI today: 181 / 114 = 158% → threshold 50% can never trip
|
||||
Reality: 181 / 211 = 85% → healthy, but unguarded
|
||||
```
|
||||
|
||||
Coverage could collapse to 30% and CI would still print a green
|
||||
"✅ Coverage is acceptable". An audit of the design principles found that every
|
||||
principle with a *working* automated check is in good shape, and the ones that
|
||||
drifted are exactly the ones whose checks were broken or too narrow — this is
|
||||
the clearest instance.
|
||||
|
||||
A second, related defect is handled in a sibling spec: `scripts/check-req-coverage.sh`
|
||||
is separately broken and orphaned (see
|
||||
[req-coverage-script-removal.md](req-coverage-script-removal.md)).
|
||||
|
||||
## Layer assignment
|
||||
|
||||
This spec touches only CI/build tooling — no application logic crosses the
|
||||
Rust/Svelte boundary. The table is filled in for completeness.
|
||||
|
||||
| Logic / responsibility | Layer | Why it belongs there |
|
||||
|------------------------|-------|----------------------|
|
||||
| Counting requirement IDs defined in `requirements.md` | Build tooling (`scripts/`) | Neither runtime layer; it is repo metadata analysis. Belongs beside `extract-traces.ts`, not in the workflow YAML, so it is runnable and testable locally. |
|
||||
| Counting *traced* requirement IDs | Build tooling — existing `extract-traces.ts` | Already implemented and correct; this spec consumes it rather than duplicating it. |
|
||||
| Threshold policy (the 50% number) | CI workflow | Deployment policy, not analysis. Keeping it in YAML lets it be tuned without touching the script. |
|
||||
|
||||
No frontend or Rust logic is added, so no taxonomy leak is possible.
|
||||
|
||||
## Design
|
||||
|
||||
### 1. Denominators come from `requirements.md`, not literals
|
||||
|
||||
`requirements.md` defines requirements in markdown tables with a stable leading
|
||||
cell, e.g.:
|
||||
|
||||
```
|
||||
| DR-001 | Player state machine (idle, loading, …) | Player | UR-005 | Done |
|
||||
| UR-002 | Access media when online or offline | High | Done |
|
||||
```
|
||||
|
||||
Extend [scripts/extract-traces.ts](../../scripts/extract-traces.ts) to also emit
|
||||
the *defined* counts, so one tool owns both sides of the fraction and CI does no
|
||||
arithmetic on stale literals. Add a `defined` key to the JSON report:
|
||||
|
||||
```jsonc
|
||||
{
|
||||
"byType": { "UR": [...], "IR": [...], "DR": [...], "JA": [...] }, // traced (existing)
|
||||
"defined": { "UR": 61, "IR": 29, "DR": 89, "JA": 32 }, // NEW
|
||||
"coverage": { "covered": 181, "total": 211, "percent": 85 }, // NEW
|
||||
"requirements": { ... }, // existing
|
||||
"totalTraces": 318, "totalFiles": …, "timestamp": "…" // existing
|
||||
}
|
||||
```
|
||||
|
||||
Parsing rule for a *defined* requirement: a line in `docs/requirements.md`
|
||||
matching `^\|\s*(UR|IR|DR|JA)-\d{3}\s*\|` — the ID must be the table's first
|
||||
cell. This deliberately does **not** count IDs mentioned in the `Traces To`
|
||||
column or in prose, which is why a naive `grep -o` over the whole file
|
||||
overcounts.
|
||||
|
||||
`defined` counts IDs that exist in the spec; `byType` counts IDs that appear in
|
||||
a `TRACES:` comment somewhere in the source. Coverage is
|
||||
`|byType ∩ defined| / |defined|`.
|
||||
|
||||
> **Intersection, not raw length.** A `TRACES:` comment naming an ID that
|
||||
> `requirements.md` does not define (a typo, or a requirement later deleted)
|
||||
> must **not** inflate the numerator — that is how a ratio exceeds 100% in the
|
||||
> first place. Such IDs are reported separately as `orphaned` so they get fixed
|
||||
> rather than silently counted or silently dropped.
|
||||
|
||||
```jsonc
|
||||
"orphaned": ["DR-097"] // traced in code but not defined in requirements.md
|
||||
```
|
||||
|
||||
### 2. The workflow consumes the computed number
|
||||
|
||||
Replace the arithmetic in `.gitea/workflows/traceability-check.yml` (lines
|
||||
46–76) with reads of the precomputed fields:
|
||||
|
||||
```sh
|
||||
COVERAGE=$(jq '.coverage.percent' traces-report.json)
|
||||
COVERED=$(jq '.coverage.covered' traces-report.json)
|
||||
TOTAL_REQS=$(jq '.coverage.total' traces-report.json)
|
||||
|
||||
for T in UR IR DR JA; do
|
||||
TRACED=$(jq --arg t "$T" '.byType[$t] | length' traces-report.json)
|
||||
DEFINED=$(jq --arg t "$T" '.defined[$t]' traces-report.json)
|
||||
echo " $T: $TRACED / $DEFINED"
|
||||
done
|
||||
|
||||
MIN_THRESHOLD=50
|
||||
[ "$COVERAGE" -lt "$MIN_THRESHOLD" ] && { echo "❌ …"; exit 1; }
|
||||
```
|
||||
|
||||
No hardcoded denominator survives anywhere in the workflow.
|
||||
|
||||
### 3. A self-check so this cannot silently rot again
|
||||
|
||||
The root cause was a number that drifted with nothing watching it. Add a
|
||||
guard that fails the job on an arithmetically impossible result:
|
||||
|
||||
```sh
|
||||
if [ "$COVERAGE" -gt 100 ]; then
|
||||
echo "❌ Coverage > 100% — the gate is miscomputing; orphaned IDs: $(jq -c '.orphaned' traces-report.json)"
|
||||
exit 1
|
||||
fi
|
||||
```
|
||||
|
||||
A >100% reading is now a hard failure rather than a green tick.
|
||||
|
||||
### 4. Local parity
|
||||
|
||||
Add a script so the gate is runnable outside CI:
|
||||
|
||||
```jsonc
|
||||
"traces:coverage": "bun run scripts/extract-traces.ts --format coverage"
|
||||
```
|
||||
|
||||
Prints the same table CI prints and exits non-zero below threshold.
|
||||
|
||||
### Threshold
|
||||
|
||||
Keep `MIN_THRESHOLD=50` in this spec. Real coverage is 85%, so raising the bar
|
||||
is tempting, but doing it in the same change that repairs the gate conflates
|
||||
"restore the safety net" with "tighten the policy" — if the build then fails, it
|
||||
is ambiguous which change caused it. Ratcheting is deliberately deferred to
|
||||
follow-up work once the honest number has been observed on `master` for a few
|
||||
builds.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Raising `MIN_THRESHOLD` above 50 (see above).
|
||||
- Fixing/removing `scripts/check-req-coverage.sh` — [req-coverage-script-removal.md](req-coverage-script-removal.md).
|
||||
- Adding TRACES comments to raise the actual coverage number.
|
||||
- Changing the `TRACES:` comment format or the extractor's parsing of it.
|
||||
- The PR "modified files missing TRACES" step (lines 78–126), which is advisory
|
||||
by design and stays advisory.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] `bun run traces:json` emits `defined`, `coverage`, and `orphaned` keys.
|
||||
- [ ] `coverage.total` equals the count of requirement IDs defined in
|
||||
`requirements.md` (**211** at time of writing), not a literal.
|
||||
- [ ] `coverage.percent` reports **85** (±1 for rounding) on the current tree —
|
||||
i.e. the honest number, not 158.
|
||||
- [ ] No hardcoded requirement denominator (`39`, `24`, `48`, `3`, `114`) remains
|
||||
in `.gitea/workflows/traceability-check.yml`. Verify:
|
||||
`grep -nE '/ *(39|24|48|3|114)\b' .gitea/workflows/traceability-check.yml`
|
||||
returns nothing.
|
||||
- [ ] Adding a new requirement row to `requirements.md` **lowers** reported
|
||||
coverage until it is traced (proves the denominator is live).
|
||||
- [ ] A `TRACES:` comment naming an undefined ID appears in `orphaned` and does
|
||||
**not** raise `coverage.percent`.
|
||||
- [ ] The job fails if coverage is forced below 50% (test by temporarily raising
|
||||
`MIN_THRESHOLD` to 99 locally) — proving the gate can fail again.
|
||||
- [ ] The job fails if coverage computes >100%.
|
||||
- [ ] `bun run check` and `bun run test` pass.
|
||||
- [ ] `bun run check:boundary` passes.
|
||||
- [ ] New requirement-implementing code carries `// TRACES:` comments.
|
||||
- [ ] No Rust types changed, so no `bindings.ts` regeneration needed.
|
||||
|
||||
## Testing
|
||||
|
||||
`extract-traces.ts` currently has no test coverage. Add
|
||||
`scripts/extract-traces.test.ts` (vitest) over fixture strings rather than the
|
||||
live `requirements.md`, so the tests do not change meaning as requirements are
|
||||
added:
|
||||
|
||||
- **UT:** counts a well-formed table row as a defined requirement.
|
||||
- **UT:** does **not** count an ID appearing only in the `Traces To` column or
|
||||
in prose — the specific overcounting bug this parse rule avoids.
|
||||
- **UT:** coverage is the intersection — a traced-but-undefined ID lands in
|
||||
`orphaned` and does not inflate the numerator.
|
||||
- **UT:** coverage of an empty trace set is 0%, not a divide-by-zero.
|
||||
- **UT:** all-traced fixture reports exactly 100%, never above.
|
||||
|
||||
CI behaviour is verified by the acceptance criteria above (the forced-failure
|
||||
check is the important one — a gate nobody has watched fail is not known to
|
||||
work).
|
||||
|
||||
## TRACES
|
||||
|
||||
Allocate in `requirements.md`:
|
||||
|
||||
- **DR-093** — "Traceability coverage gate derives requirement denominators from
|
||||
`requirements.md` at run time (not hardcoded literals), computes coverage as
|
||||
the intersection of traced and defined IDs, reports IDs traced but undefined
|
||||
as orphaned, and fails on an impossible >100% result." Category: Tooling.
|
||||
Status: Done on merge.
|
||||
|
||||
Tag:
|
||||
|
||||
```typescript
|
||||
// scripts/extract-traces.ts
|
||||
// TRACES: | DR-093
|
||||
```
|
||||
|
||||
Tests carry `@req-test: UT-089 …` onward (next free UT is **UT-089**).
|
||||
|
||||
## Notes for the implementer
|
||||
|
||||
- A parallel Claude session may be active in this repo — run `git diff` before
|
||||
"repairing" unexpected changes (CLAUDE.md §Gotchas).
|
||||
- **Do not add tooling to the CI image for this.** `jq` and `bun` are already in
|
||||
`jellytau-builder`; this spec needs nothing else. Installing a system package
|
||||
in a workflow step violates the hard CI rule in CLAUDE.md.
|
||||
- Keep `traces:json`'s existing keys intact — `release-notes.ts` and
|
||||
`traces:markdown` consume the same report, and the CI workflow uploads it as
|
||||
an artifact. This is an additive change.
|
||||
- The `head -50 docs/traceability.md` and artifact-upload steps are unaffected.
|
||||
- Expect the first green build after this change to print a *lower* number than
|
||||
before (85% vs 158%). That is the fix working, not a regression.
|
||||
@@ -1,233 +0,0 @@
|
||||
# Spec: Background audio for video playback (Android)
|
||||
|
||||
**Status:** Draft
|
||||
**Scope:** Android only (v1). Linux noted as future work.
|
||||
**Branch base:** `android-picture-in-picture`
|
||||
**Requirements:** UR-040 → IR-025, JA-032, DR-051, DR-052 (see
|
||||
[requirements.md](../requirements.md)). Tests: UT-059, UT-060, UT-061, IT-013.
|
||||
|
||||
## Summary
|
||||
|
||||
Add a per-player toggle that lets the **audio** of a video keep playing when the
|
||||
app is backgrounded or the screen is locked, while **video decoding stops**.
|
||||
When the app returns to the foreground, video decoding resumes from the current
|
||||
audio position.
|
||||
|
||||
This is the audio-first counterpart to the existing Picture-in-Picture feature
|
||||
(which keeps the *whole video* decoding in a floating window). The two are
|
||||
mutually exclusive: enabling background audio suppresses auto-PiP.
|
||||
|
||||
## Motivation
|
||||
|
||||
Users watching talk-heavy content (podcasts-as-video, lectures, music videos,
|
||||
concert films) want to lock the phone or switch apps and keep listening without
|
||||
draining battery on video decode or needing a visible floating window.
|
||||
|
||||
## Background: how playback actually works here
|
||||
|
||||
Two facts drive the entire design (verified in code, not assumed):
|
||||
|
||||
1. **Video renders through the HTML5 `<video>` element in the WebView on both
|
||||
platforms.** The native ExoPlayer *video* surface path is disabled — see the
|
||||
INTERIM override in
|
||||
[VideoPlayer.svelte](../../src/lib/components/player/VideoPlayer.svelte)
|
||||
around the `playerPlayItem` response handling (`useHtml5Element` is forced
|
||||
`true`, native backend is stopped). So "video decoding" == the WebView
|
||||
`<video>` element, and the WebView is what Android suspends on background.
|
||||
|
||||
2. **An Android WebView `<video>` element does not keep playing audio when the
|
||||
app is backgrounded / locked.** The system throttles the WebView and media
|
||||
pauses. Keeping audio alive in the background requires a **native foreground
|
||||
media service**, which already exists for music:
|
||||
[`JellyTauPlaybackService`](../../src-tauri/android/src/main/java/com/dtourolle/jellytau/player/JellyTauPlaybackService.kt)
|
||||
+
|
||||
[`JellyTauPlayer`](../../src-tauri/android/src/main/java/com/dtourolle/jellytau/player/JellyTauPlayer.kt)
|
||||
(ExoPlayer) + `MediaSessionCompat`.
|
||||
|
||||
**Therefore the design is a handoff**, not "keep the WebView alive": on
|
||||
background, stop the WebView `<video>` and start audio-only playback of the same
|
||||
item through the existing native ExoPlayer audio service; on foreground, hand
|
||||
back to the WebView `<video>`.
|
||||
|
||||
This also aligns with the project's one-directional playback rule
|
||||
(`CLAUDE.md` → "Playback state is one-directional"): the currently-authoritative
|
||||
player (WebView element **or** native audio service) drives position; the UI and
|
||||
MediaSession consume it. The handoff is a change of *which* player is
|
||||
authoritative, and must transfer position cleanly.
|
||||
|
||||
## User-facing behavior
|
||||
|
||||
### The toggle
|
||||
|
||||
- A toggle button in the video player controls (next to the existing PiP /
|
||||
fullscreen buttons in
|
||||
[VideoPlayer.svelte](../../src/lib/components/player/VideoPlayer.svelte)).
|
||||
- Icon: headphones / "audio-only" glyph. Two visual states (on/off).
|
||||
- **Visible only when** `isPipSupported()`-equivalent conditions hold — i.e.
|
||||
Android with a native audio service available. Hidden on Linux in v1.
|
||||
- State is a UI preference on the player. Consider persisting the last choice
|
||||
per user (see Open Questions) — v1 may default OFF each session.
|
||||
|
||||
### When toggle is ON and the app goes to background / screen locks
|
||||
|
||||
1. Auto-PiP is suppressed (see "Interaction with PiP").
|
||||
2. The WebView `<video>` is paused and its decode stopped (release the media
|
||||
source so the decoder is freed, not merely `pause()`).
|
||||
3. Native audio-only playback of the same item starts at the current position,
|
||||
through `JellyTauPlaybackService` (foreground notification + lockscreen
|
||||
controls via the existing `MediaSessionCompat`).
|
||||
4. Lockscreen / notification shows the item with play/pause/seek, driven by the
|
||||
native player (existing music behavior — reused, not rebuilt).
|
||||
|
||||
### When toggle is ON and the app returns to foreground
|
||||
|
||||
1. Native audio playback stops; its final position is captured.
|
||||
2. WebView `<video>` reloads/resumes at that position and continues as normal
|
||||
audiovisual playback.
|
||||
3. Playback state (playing/paused) is preserved across the handoff.
|
||||
|
||||
### When toggle is OFF (default)
|
||||
|
||||
Current behavior is unchanged: backgrounding video auto-enters PiP
|
||||
(`onUserLeaveHint` → `PictureInPictureManager.enterPip`).
|
||||
|
||||
## Interaction with PiP
|
||||
|
||||
The toggle chooses one behavior or the other:
|
||||
|
||||
- Toggle **ON** → call `AndroidPictureInPicture.setAutoEnterEnabled(false)` (the
|
||||
bridge already exists,
|
||||
[pictureInPicture.ts](../../src/lib/utils/pictureInPicture.ts) →
|
||||
`setAutoEnterEnabled`). Background → audio handoff instead of PiP.
|
||||
- Toggle **OFF** → `setAutoEnterEnabled(true)`. Background → PiP (status quo).
|
||||
|
||||
The frontend must also call `setAutoEnterEnabled(false)` on unmount if it left
|
||||
it enabled, and re-assert the correct value whenever the toggle changes, so a
|
||||
stale setting can't leak into the next player.
|
||||
|
||||
> Note: `canEnterPip()` today requires `isPlayingVideo()` on the *native*
|
||||
> ExoPlayer, but video plays via the WebView, so native `isPlayingVideo()` is
|
||||
> false during normal playback. Confirm during implementation how auto-PiP is
|
||||
> actually triggering today (it may rely on a different signal), because the
|
||||
> background-audio handoff needs the same "is a local video active" signal to
|
||||
> know it should fire. **This is a load-bearing unknown — resolve it first
|
||||
> (Phase 0).**
|
||||
|
||||
## Technical design
|
||||
|
||||
### The audio-only stream
|
||||
|
||||
Jellyfin can transcode/stream a video item as audio-only. Add a repository
|
||||
method (mirroring
|
||||
[`get_video_stream_url`](../../src-tauri/src/repository/online.rs) and
|
||||
[`get_audio_stream_url`](../../src-tauri/src/repository/mod.rs)) that returns an
|
||||
**audio-only stream URL for a video item** at a given audio-stream index — so
|
||||
the currently-selected audio track (`selectedAudioTrackIndex` in the player)
|
||||
carries over. Prefer direct-play of the audio stream where the container/codec
|
||||
allows; transcode to a broadly-supported audio codec otherwise.
|
||||
|
||||
Position semantics must match between the WebView `<video>` timeline and the
|
||||
audio stream (account for the transcoded-HLS `seekOffset` model already in the
|
||||
player — see the `seekOffset` handling in `VideoPlayer.svelte`).
|
||||
|
||||
### Backend command surface (Rust)
|
||||
|
||||
New/extended `#[tauri::command]`s in `src-tauri/src/commands/player/` (follow the
|
||||
camelCase param rule and `Result<T, String>` convention):
|
||||
|
||||
- `player_enter_background_audio(item_id, position_seconds, audio_stream_index)`
|
||||
— stop WebView authority, start native audio-only playback at position; makes
|
||||
the native player authoritative. Emits state via the existing player-event
|
||||
channel so MediaSession/UI stay consumers.
|
||||
- `player_exit_background_audio() -> position_seconds` — stop native audio,
|
||||
return final position for the WebView to resume from; restores WebView
|
||||
authority.
|
||||
|
||||
Reuse existing `player_play_*` / `player_stop` plumbing where possible rather
|
||||
than adding a parallel path.
|
||||
|
||||
### Android native
|
||||
|
||||
- Reuse `JellyTauPlaybackService` + `JellyTauPlayer` audio path
|
||||
(`MediaSessionCompat`, foreground notification, audio-becoming-noisy, etc. —
|
||||
all already implemented for music).
|
||||
- Add a bridge method (alongside `AndroidPictureInPicture`) or reuse an existing
|
||||
one so the frontend can signal "prepare for background audio handoff" tied to
|
||||
the Activity lifecycle (`onPause`/`onStop`/`onUserLeaveHint`).
|
||||
- On `onUserLeaveHint` / screen-off with background-audio enabled: **do not**
|
||||
enter PiP; instead trigger the handoff command.
|
||||
- Respect the deadlock gotchas in `CLAUDE.md` (no sync/blocking calls from
|
||||
player event callbacks; bind locked `AutoplayDecision` to a `let` before
|
||||
matching).
|
||||
|
||||
### Frontend (VideoPlayer.svelte)
|
||||
|
||||
- Add toggle state + button. On change, call `setAutoEnterEnabled(!on)`.
|
||||
- Listen for Android lifecycle background/foreground signals (via a bridge event
|
||||
or existing visibility hooks) and:
|
||||
- background + ON → `player_enter_background_audio(...)`, pause + tear down the
|
||||
`<video>`/HLS decode (reuse the existing HLS teardown sequence to avoid dual
|
||||
audio).
|
||||
- foreground + ON → `player_exit_background_audio()`, reload `<video>` at the
|
||||
returned position, restore play/pause state.
|
||||
- **Follow the native-mode pitfall** (memory:
|
||||
`videoplayer-native-mode-pitfalls`): no lifecycle calls after an `await` in
|
||||
`onMount`. Keep the handoff logic out of that window.
|
||||
- Dual-audio is the key regression risk: at every handoff exactly one of
|
||||
{WebView `<video>`, native ExoPlayer} produces audio. Tear the other down
|
||||
*before* starting the next, mirroring the existing HLS cleanup discipline.
|
||||
|
||||
## Phasing
|
||||
|
||||
- **Phase 0 — De-risk (do first):**
|
||||
- Confirm what actually triggers today's auto-PiP given video is on the
|
||||
WebView (resolve the `canEnterPip`/`isPlayingVideo` question).
|
||||
- Spike: obtain an audio-only stream URL for a video item and play it through
|
||||
the native audio service; measure position accuracy and that WebView audio
|
||||
is fully silenced (no dual audio).
|
||||
- **Phase 1 — Backend:** repository audio-only-URL method + the two player
|
||||
commands + events.
|
||||
- **Phase 2 — Native:** lifecycle wiring, PiP suppression, handoff trigger.
|
||||
- **Phase 3 — Frontend:** toggle UI, lifecycle listeners, handoff calls,
|
||||
teardown discipline.
|
||||
- **Phase 4 — Polish:** persist toggle preference, subtitle/audio-track
|
||||
carry-over, edge cases (calls, headphone unplug, autoplay-next during
|
||||
background audio).
|
||||
|
||||
## Testing
|
||||
|
||||
- Rust: unit tests for the audio-only URL builder and the two commands
|
||||
(`cargo test`, `bun run test:rust`).
|
||||
- IPC param-naming integration tests for any new commands
|
||||
(`bun run test -- tauriIntegration.test.ts`).
|
||||
- Frontend: `bun run check`, `bun run test`, plus a VideoPlayer logic test for
|
||||
the handoff state machine (mirror the existing
|
||||
`VideoPlayer.logic.test.ts`).
|
||||
- Manual on-device matrix:
|
||||
- toggle ON: home button → audio continues, video stops decoding; return →
|
||||
video resumes at position; playing/paused preserved.
|
||||
- toggle ON: screen lock → audio continues; lockscreen controls work; unlock →
|
||||
resumes.
|
||||
- toggle OFF: background → PiP (unchanged).
|
||||
- No dual audio at any transition. No audio leak after leaving the player.
|
||||
- Transcoded (HEVC/10-bit) item — verify position with `seekOffset`.
|
||||
- Autoplay-next fires correctly if an episode ends during background audio.
|
||||
|
||||
## Open questions
|
||||
|
||||
1. **Persist the toggle per user/series, or default OFF each session?**
|
||||
(Recommend: remember last choice; series-level like the audio-track
|
||||
preference is a nice-to-have.)
|
||||
2. **Autoplay-next during background audio** — should the next episode start as
|
||||
audio-only and stay audio until foreground, or pause at episode end? (Recommend:
|
||||
continue as audio-only.)
|
||||
3. **Subtitles** are irrelevant in audio-only mode but must restore on
|
||||
foreground — confirm they survive the `<video>` teardown/reload.
|
||||
4. Exact **Android lifecycle signal** for "screen locked" vs "app backgrounded"
|
||||
— `onUserLeaveHint` covers Home but not lock; may need a screen-off receiver.
|
||||
|
||||
## Non-goals (v1)
|
||||
|
||||
- Linux background audio (desktop windows keep running unfocused; low value).
|
||||
- Replacing or removing PiP — it stays as the toggle-OFF behavior.
|
||||
- Re-enabling the native ExoPlayer *video* surface path.
|
||||
@@ -1,7 +1,11 @@
|
||||
# Spec: Windows native audio backend
|
||||
|
||||
**Status:** Proposed
|
||||
**Requirements:** UR-003, UR-027, UR-032, UR-033 → DR-030, DR-035, DR-036; new IR-030
|
||||
**Status:** Proposed — not started. Windows still runs on
|
||||
`WebviewAudioBackend`. Blocked on [libmpv2-migration.md](libmpv2-migration.md),
|
||||
whose crate swap has not landed either.
|
||||
**Requirements:** UR-003, UR-027, UR-032, UR-033 → DR-030, DR-035, DR-036;
|
||||
⚠️ the suggested id **IR-030 has since been allocated** to the scheduled catalog
|
||||
crawl — allocate a fresh id (IR-033 or later) on implementation
|
||||
**UX spec:** n/a — Settings › Audio already renders the controls
|
||||
**Supersedes / revises:** acts on the "audio can unify, video cannot" conclusion in [playback-backend-unification.md](playback-backend-unification.md)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user