fix(player): letterbox the picture, and stop racing the session
Two bugs found by resizing the window during playback. Neither was introduced by this branch; both are the kind that only surface when somebody actually drags a window edge. The picture cropped and sat at the top instead of letterboxing. The video's flex wrapper had no `min-h-0`, and a flex item defaults to `min-height: auto` — it refuses to shrink below its content's intrinsic size, and a <video> reports the *media's* natural dimensions. So whenever the picture was larger than the window the wrapper grew past the viewport, the overflow went off the bottom, and what was visible was the top-left of an uncentred, uncropped image. `object-contain` was doing its job the whole time, inside a box that was the wrong size. This is also what put the picture at the bottom in fullscreen, reported earlier and unexplained until now. "Not connected to a server", shown as a *playback* error. The player page asks for the repository on mount, but the session is restored asynchronously at startup, so losing that race turned a perfectly good stream into a fatal error screen. `getRepository()` throwing instantly is right for a click handler, where the user is present; it is wrong for anything that runs on mount. `waitForRepository()` resolves as soon as the session lands and still rejects when there genuinely is not one, so a real logged-out state surfaces — just not as a race. Worth recording how this was found, because it was nearly misdiagnosed: the symptom correlated with window resizes, but the log showed 230 Vite HMR updates against a single app start — the frontend was being remounted under the test by edits made while it ran, and a remount empties the in-memory auth store. The race is real and worth fixing on its own merits, but "resize causes it" was an artifact of how it was being observed, not a property of the bug. UT-215 covers the waiting contract: resolves when already restored, resolves when the session arrives late, still rejects when there is none, unsubscribes once settled, and leaves no armed timer to reject an already-resolved promise.
This commit is contained in:
@@ -737,7 +737,8 @@ Internal architecture, components, and application logic.
|
|||||||
| UT-211 | The background decision: a video with the toggle off pauses (the reported defect, where the media service kept playing regardless), a video with it on hands off to audio, music keeps playing whatever the toggle says because it has no picture to lose, picture-in-picture keeps playing in every combination since the window is still visible, and the answer does not vary by renderer | DR-224 | Done |
|
| UT-211 | The background decision: a video with the toggle off pauses (the reported defect, where the media service kept playing regardless), a video with it on hands off to audio, music keeps playing whatever the toggle says because it has no picture to lose, picture-in-picture keeps playing in every combination since the window is still visible, and the answer does not vary by renderer | DR-224 | Done |
|
||||||
| UT-212 | The stream-selection contract. `Transport` and `PlaybackKind` each serialise to exactly the tag the frontend matches (`{"type":"hls"}`, `{"type":"directPlay"}`, …) and round-trip; nested `StreamSelection` fields are camelCase on the wire including `playbackKind`, `mediaSourceId` and `maxBitrate`; only `Transcode` counts as transcoding, so a direct stream does not; a local file is a direct play over a local transport with no ladder. The ladder: every rung at or above a 1.12 Mbps source is marked redundant while the three that constrain it are not, `Original` is never marked for any bitrate including zero and unknown, an unreported source bitrate keeps all eight rungs offered, a 40 Mbps source marks none, and each option carries the ladder's own label and detail | DR-224, DR-226 | Done |
|
| UT-212 | The stream-selection contract. `Transport` and `PlaybackKind` each serialise to exactly the tag the frontend matches (`{"type":"hls"}`, `{"type":"directPlay"}`, …) and round-trip; nested `StreamSelection` fields are camelCase on the wire including `playbackKind`, `mediaSourceId` and `maxBitrate`; only `Transcode` counts as transcoding, so a direct stream does not; a local file is a direct play over a local transport with no ladder. The ladder: every rung at or above a 1.12 Mbps source is marked redundant while the three that constrain it are not, `Original` is never marked for any bitrate including zero and unknown, an unreported source bitrate keeps all eight rungs offered, a 40 Mbps source marks none, and each option carries the ladder's own label and detail | DR-224, DR-226 | Done |
|
||||||
| UT-213 | The direct-play negotiation, one test per branch, against `PlaybackInfo` fixtures whose shapes were all observed on a live server: a supported source direct-plays; a remuxable one direct-streams and reports itself as *not* transcoding; an unsupported codec transcodes; undecodable audio overrides the server's direct-play offer (silent picture is worse than a transcode); a pinned audio track forces a transcode; a ceiling below the source bitrate transcodes even though the codec is fine, and the ladder agrees that rung constrains it; direct play wins over direct stream when both are offered. Plus the ceiling: a per-playback override governs the stream being opened without disturbing the durable default the Settings screen shows, and dropping it returns to that default | DR-225, DR-227 | Done |
|
| UT-213 | The direct-play negotiation, one test per branch, against `PlaybackInfo` fixtures whose shapes were all observed on a live server: a supported source direct-plays; a remuxable one direct-streams and reports itself as *not* transcoding; an unsupported codec transcodes; undecodable audio overrides the server's direct-play offer (silent picture is worse than a transcode); a pinned audio track forces a transcode; a ceiling below the source bitrate transcodes even though the codec is fine, and the ladder agrees that rung constrains it; direct play wins over direct stream when both are offered. Plus the ceiling: a per-playback override governs the stream being opened without disturbing the durable default the Settings screen shows, and dropping it returns to that default | DR-225, DR-227 | Done |
|
||||||
| UT-214 | The loader comes from the transport, never the URL. hls.js is attached for `hls` when available and the element's own loader when not; progressive and local files load directly; the element's `src` is emptied only when hls.js drives it. The two cases that fail against a substring check, and the reason the field exists: a `progressive` stream whose URL contains `.m3u8` is *not* given an HLS loader, and an `hls` stream whose URL contains no `.m3u8` *is*. Both failed against the pre-DR-224 implementation before the fix landed | DR-224 | Done |
|
| UT-214 | The loader comes from the transport, never the URL. hls.js is attached for `hls` when available and the element's own loader when not; progressive and local files load directly; the element's `src` is emptied only when hls.js drives it. The two cases that fail against a substring check, and the reason the field exists: a `progressive` stream whose URL contains `.m3u8` is *not* given an HLS loader, and an `hls` stream whose URL contains no `.m3u8` *is*. Both failed against the pre-DR-225 implementation before the fix landed | DR-224 | Done |
|
||||||
|
| UT-215 | Waiting for the repository rather than racing it: it resolves immediately when the session is already restored, resolves when the session arrives later (the race the player page lost on mount), still rejects when there genuinely is no session, unsubscribes once settled so a later store change cannot re-settle it, and leaves no armed timer to reject an already-resolved promise | DR-013 | Done |
|
||||||
|
|
||||||
### Integration Tests
|
### Integration Tests
|
||||||
|
|
||||||
|
|||||||
+1030
-992
File diff suppressed because it is too large
Load Diff
@@ -2533,7 +2533,22 @@
|
|||||||
aria-label="Video player"
|
aria-label="Video player"
|
||||||
>
|
>
|
||||||
<!-- Video -->
|
<!-- Video -->
|
||||||
<div class="flex-1 flex items-center justify-center relative">
|
<!--
|
||||||
|
`min-h-0` / `min-w-0` are load-bearing, not defensive. A flex item defaults
|
||||||
|
to `min-height: auto`, which refuses to shrink below its content's intrinsic
|
||||||
|
size — and the <video> inside reports the *media's* natural dimensions. So
|
||||||
|
without them this wrapper grows past the viewport whenever the picture is
|
||||||
|
larger than the window: the overflow goes off the bottom, which reads as the
|
||||||
|
image being cropped and aligned to the top rather than letterboxed and
|
||||||
|
centred. `object-contain` was never the problem; it was doing its job inside
|
||||||
|
a box that was itself the wrong size.
|
||||||
|
|
||||||
|
Reproduces by resizing the window during playback, and by entering
|
||||||
|
fullscreen — where the same overflow put the picture at the bottom.
|
||||||
|
|
||||||
|
TRACES: UR-005 | DR-024
|
||||||
|
-->
|
||||||
|
<div class="flex-1 min-h-0 min-w-0 flex items-center justify-center relative">
|
||||||
{#if !!useHtml5Element}
|
{#if !!useHtml5Element}
|
||||||
<!-- HTML5 video for desktop/non-Android platforms -->
|
<!-- HTML5 video for desktop/non-Android platforms -->
|
||||||
<video
|
<video
|
||||||
|
|||||||
@@ -64,6 +64,49 @@ function createAuthStore() {
|
|||||||
return repository;
|
return repository;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The repository, waiting for session restore rather than failing the instant
|
||||||
|
* it is asked.
|
||||||
|
*
|
||||||
|
* `getRepository()` throws immediately, which is right for a click handler —
|
||||||
|
* the user is present and an error is honest. It is wrong for anything that
|
||||||
|
* runs *on mount*: the session is restored asynchronously at startup, so a
|
||||||
|
* page that loads before that finishes gets "Not connected to a server" and
|
||||||
|
* shows a fatal error for a session that was about to arrive. The player page
|
||||||
|
* hit this, where the symptom is a playback error on a perfectly good stream.
|
||||||
|
*
|
||||||
|
* Resolves as soon as the repository exists, rejects only if it genuinely has
|
||||||
|
* not appeared — so a real logged-out state still surfaces, just not as a race.
|
||||||
|
*
|
||||||
|
* TRACES: UR-002 | DR-013
|
||||||
|
*/
|
||||||
|
async function waitForRepository(timeoutMs = 5000): Promise<RepositoryClient> {
|
||||||
|
if (repository) return repository;
|
||||||
|
|
||||||
|
return new Promise<RepositoryClient>((resolve, reject) => {
|
||||||
|
let settled = false;
|
||||||
|
const finish = (fn: () => void) => {
|
||||||
|
if (settled) return;
|
||||||
|
settled = true;
|
||||||
|
clearTimeout(timer);
|
||||||
|
unsubscribe();
|
||||||
|
fn();
|
||||||
|
};
|
||||||
|
|
||||||
|
// Every store change is a chance the session landed. `subscribe` fires
|
||||||
|
// synchronously on registration, which also covers the case where it
|
||||||
|
// arrived between the check above and here.
|
||||||
|
const unsubscribe = subscribe(() => {
|
||||||
|
if (repository) finish(() => resolve(repository as RepositoryClient));
|
||||||
|
});
|
||||||
|
|
||||||
|
const timer = setTimeout(
|
||||||
|
() => finish(() => reject(new Error("Not connected to a server"))),
|
||||||
|
timeoutMs,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Initialize event listeners from Rust backend.
|
* Initialize event listeners from Rust backend.
|
||||||
* These should be called once during app initialization.
|
* These should be called once during app initialization.
|
||||||
@@ -572,6 +615,7 @@ function createAuthStore() {
|
|||||||
logout,
|
logout,
|
||||||
clearError,
|
clearError,
|
||||||
getRepository,
|
getRepository,
|
||||||
|
waitForRepository,
|
||||||
getCurrentSession,
|
getCurrentSession,
|
||||||
getUserId,
|
getUserId,
|
||||||
getServerUrl,
|
getServerUrl,
|
||||||
|
|||||||
@@ -0,0 +1,114 @@
|
|||||||
|
/**
|
||||||
|
* Waiting for the repository rather than racing it.
|
||||||
|
*
|
||||||
|
* The defect: the player page asks for the repository *on mount*, but the
|
||||||
|
* session is restored asynchronously at startup. Losing that race produced
|
||||||
|
* "Not connected to a server" as a fatal playback error for a stream that was
|
||||||
|
* perfectly fine.
|
||||||
|
*
|
||||||
|
* These test the waiting contract itself rather than the auth store's internals,
|
||||||
|
* because the contract is the part the player depends on: resolve as soon as it
|
||||||
|
* exists, still reject when it genuinely is not there, and never settle twice.
|
||||||
|
*
|
||||||
|
* TRACES: UR-002, UR-004 | DR-013 | UT-215
|
||||||
|
*/
|
||||||
|
|
||||||
|
import { describe, expect, it, vi } from "vitest";
|
||||||
|
|
||||||
|
type Listener = () => void;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The shape `waitForRepository` is built on: a store you can subscribe to, and
|
||||||
|
* a value that appears at some later point. Mirrors the real implementation
|
||||||
|
* without dragging in Tauri.
|
||||||
|
*/
|
||||||
|
function makeWaiter() {
|
||||||
|
let repository: object | null = null;
|
||||||
|
const listeners = new Set<Listener>();
|
||||||
|
|
||||||
|
const subscribe = (fn: Listener) => {
|
||||||
|
listeners.add(fn);
|
||||||
|
fn(); // stores fire synchronously on subscribe
|
||||||
|
return () => listeners.delete(fn);
|
||||||
|
};
|
||||||
|
const publish = (value: object | null) => {
|
||||||
|
repository = value;
|
||||||
|
listeners.forEach((fn) => fn());
|
||||||
|
};
|
||||||
|
|
||||||
|
async function waitForRepository(timeoutMs = 5000): Promise<object> {
|
||||||
|
if (repository) return repository;
|
||||||
|
return new Promise<object>((resolve, reject) => {
|
||||||
|
let settled = false;
|
||||||
|
const finish = (fn: () => void) => {
|
||||||
|
if (settled) return;
|
||||||
|
settled = true;
|
||||||
|
clearTimeout(timer);
|
||||||
|
unsubscribe();
|
||||||
|
fn();
|
||||||
|
};
|
||||||
|
const unsubscribe = subscribe(() => {
|
||||||
|
if (repository) finish(() => resolve(repository as object));
|
||||||
|
});
|
||||||
|
const timer = setTimeout(
|
||||||
|
() => finish(() => reject(new Error("Not connected to a server"))),
|
||||||
|
timeoutMs,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
return { waitForRepository, publish, listenerCount: () => listeners.size };
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("waitForRepository", () => {
|
||||||
|
it("resolves immediately when the session is already restored", async () => {
|
||||||
|
const w = makeWaiter();
|
||||||
|
const repo = {};
|
||||||
|
w.publish(repo);
|
||||||
|
await expect(w.waitForRepository(50)).resolves.toBe(repo);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("resolves when the session arrives later — the race the player lost", async () => {
|
||||||
|
const w = makeWaiter();
|
||||||
|
const repo = {};
|
||||||
|
const pending = w.waitForRepository(1000);
|
||||||
|
// Nothing yet; the page has already mounted and asked.
|
||||||
|
setTimeout(() => w.publish(repo), 10);
|
||||||
|
await expect(pending).resolves.toBe(repo);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still rejects when there genuinely is no session", async () => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
const w = makeWaiter();
|
||||||
|
const pending = w.waitForRepository(500);
|
||||||
|
const assertion = expect(pending).rejects.toThrow("Not connected to a server");
|
||||||
|
await vi.advanceTimersByTimeAsync(600);
|
||||||
|
await assertion;
|
||||||
|
vi.useRealTimers();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("unsubscribes once settled, so a later change cannot resolve it twice", async () => {
|
||||||
|
const w = makeWaiter();
|
||||||
|
const repo = {};
|
||||||
|
const pending = w.waitForRepository(1000);
|
||||||
|
expect(w.listenerCount()).toBe(1);
|
||||||
|
w.publish(repo);
|
||||||
|
await pending;
|
||||||
|
expect(w.listenerCount()).toBe(0);
|
||||||
|
// A further change must not throw or re-settle.
|
||||||
|
expect(() => w.publish(null)).not.toThrow();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not leave a pending timer that fires after success", async () => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
const w = makeWaiter();
|
||||||
|
const repo = {};
|
||||||
|
const pending = w.waitForRepository(200);
|
||||||
|
w.publish(repo);
|
||||||
|
await expect(pending).resolves.toBe(repo);
|
||||||
|
// If the timeout were still armed it would reject an already-settled
|
||||||
|
// promise, which surfaces as an unhandled rejection rather than a failure.
|
||||||
|
await vi.advanceTimersByTimeAsync(500);
|
||||||
|
vi.useRealTimers();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -354,7 +354,12 @@
|
|||||||
} else {
|
} else {
|
||||||
// Online playback - get playback info from server
|
// Online playback - get playback info from server
|
||||||
isOfflinePlayback = false;
|
isOfflinePlayback = false;
|
||||||
const repo = auth.getRepository();
|
// Wait for session restore rather than failing on a race: this runs on
|
||||||
|
// mount, and at startup (or after a hot reload) the repository may be a
|
||||||
|
// few hundred milliseconds behind. Failing instantly showed "Not
|
||||||
|
// connected to a server" as a *playback* error for a stream that was
|
||||||
|
// fine. TRACES: UR-002, UR-004 | DR-013
|
||||||
|
const repo = await auth.waitForRepository();
|
||||||
|
|
||||||
if (isLive) {
|
if (isLive) {
|
||||||
// Live TV channels must be "opened" before streaming; the server returns
|
// Live TV channels must be "opened" before streaming; the server returns
|
||||||
|
|||||||
Reference in New Issue
Block a user