From 90ea93454825bae56b89380d3604d4aa2c73847f Mon Sep 17 00:00:00 2001 From: Gauvain Date: Tue, 30 Jun 2026 01:08:50 +0200 Subject: [PATCH] fix(subtitles): address CodeRabbit review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Unify external detection: isExternalSubtitle drops the bare-DeliveryUrl case (an Hls-delivered sub has a DeliveryUrl but isn't sub-add-ed) so sorting, loading and resolution agree; compareTracksForMenu now uses it. - applyMpvSubtitleSelection wraps player calls in try/catch — fire-and-forget call sites no longer risk unhandled rejections. - VideoContext offline-transcoded branch: treat missing IsTextSubtitleStream as text (use !isImageBasedSubtitle), matching the shared helper. - ItemContent.tv refreshSubtitleTracks: apply compareTracksForMenu like the initial list. - Tests: use the @/ alias; rework the embedded cases to actually exercise identity (reversed player order) and the ordinal fallback (same-language, no title). --- components/ItemContent.tv.tsx | 10 ++- .../controls/contexts/VideoContext.tsx | 4 +- utils/jellyfin/subtitleUtils.test.ts | 37 +++++---- utils/jellyfin/subtitleUtils.ts | 77 +++++++++++-------- 4 files changed, 76 insertions(+), 52 deletions(-) diff --git a/components/ItemContent.tv.tsx b/components/ItemContent.tv.tsx index 7a667e18..7f922190 100644 --- a/components/ItemContent.tv.tsx +++ b/components/ItemContent.tv.tsx @@ -413,11 +413,13 @@ export const ItemContentTV: React.FC = React.memo( ) : freshItem.MediaSources?.[0]; - // Get subtitle streams from the fresh data - const streams = - mediaSource?.MediaStreams?.filter( + // Get subtitle streams from the fresh data, ordered like jellyfin-web + // (embedded first, externals last) — same as the initial list. + const streams = [ + ...(mediaSource?.MediaStreams?.filter( (s: MediaStream) => s.Type === "Subtitle", - ) ?? []; + ) ?? []), + ].sort(compareTracksForMenu); // Convert to Track[] with setTrack callbacks const tracks: Track[] = streams.map((stream) => ({ diff --git a/components/video-player/controls/contexts/VideoContext.tsx b/components/video-player/controls/contexts/VideoContext.tsx index 612da5aa..d43feb70 100644 --- a/components/video-player/controls/contexts/VideoContext.tsx +++ b/components/video-player/controls/contexts/VideoContext.tsx @@ -198,7 +198,9 @@ export const VideoProvider: React.FC<{ children: ReactNode }> = ({ // track list (same as online) — robust to the transcoded file's track // structure differing from the original MediaStreams. Order matches web. for (const sub of [...allSubs].sort(compareTracksForMenu)) { - if (sub.IsTextSubtitleStream) { + // Treat missing IsTextSubtitleStream as text (image-based only when + // explicitly false — matches isImageBasedSubtitle). + if (!isImageBasedSubtitle(sub)) { subs.push({ name: sub.DisplayTitle || "Unknown", index: sub.Index ?? -1, diff --git a/utils/jellyfin/subtitleUtils.test.ts b/utils/jellyfin/subtitleUtils.test.ts index dab53326..53fc061b 100644 --- a/utils/jellyfin/subtitleUtils.test.ts +++ b/utils/jellyfin/subtitleUtils.test.ts @@ -8,7 +8,7 @@ import { isExternalSubtitle, type PlayerSubtitleTrack, resolveSubtitleTrack, -} from "./subtitleUtils"; +} from "@/utils/jellyfin/subtitleUtils"; // String-enum values as typed literals — avoids a runtime SDK import (see subtitleUtils.ts). const External = "External" as SubtitleDeliveryMethod; @@ -55,12 +55,14 @@ const resolve = ( // --- tests ----------------------------------------------------------------- describe("isExternalSubtitle", () => { - test("true for External delivery, IsExternal flag, or a DeliveryUrl", () => { + test("true for External delivery or the IsExternal flag, not a bare DeliveryUrl", () => { expect(isExternalSubtitle(ext(0))).toBe(true); - expect(isExternalSubtitle(sub({ Index: 1, DeliveryUrl: "/x.srt" }))).toBe( - true, - ); + expect(isExternalSubtitle(sub({ Index: 1, IsExternal: true }))).toBe(true); expect(isExternalSubtitle(emb(2))).toBe(false); + // A DeliveryUrl alone (e.g. an Hls-delivered sub) is NOT a sub-added sidecar. + expect(isExternalSubtitle(sub({ Index: 3, DeliveryUrl: "/x.srt" }))).toBe( + false, + ); }); }); @@ -154,25 +156,26 @@ describe("resolveSubtitleTrack — external without DeliveryUrl (#1763 CodeRabbi }); describe("resolveSubtitleTrack — embedded matching", () => { - test("unique language match wins regardless of order", () => { + test("unique language match wins even when player order differs (not positional)", () => { const streams = [emb(0, { Language: "eng" }), emb(1, { Language: "jpn" })]; + // Player lists them in the OPPOSITE order — a positional map would mis-pick. const player = [ - track({ id: 1, external: false, language: "eng" }), - track({ id: 2, external: false, language: "jpn" }), + track({ id: 1, external: false, language: "jpn" }), + track({ id: 2, external: false, language: "eng" }), ]; - expect(resolve(streams, 1, player)).toEqual({ kind: "select", trackId: 2 }); + expect(resolve(streams, 0, player)).toEqual({ kind: "select", trackId: 2 }); // eng + expect(resolve(streams, 1, player)).toEqual({ kind: "select", trackId: 1 }); // jpn }); - test("same-language tracks disambiguate by ordinal among matches", () => { - const streams = [ - emb(0, { Language: "eng", Title: "Full" }), - emb(1, { Language: "eng", Title: "SDH" }), - ]; + test("same-language tracks with no distinguishing title fall back to ordinal among matches", () => { + const streams = [emb(0, { Language: "eng" }), emb(1, { Language: "eng" })]; + // Both eng, no title → identity can't disambiguate → ordinal among matches. const player = [ - track({ id: 1, external: false, language: "eng", title: "Full" }), - track({ id: 2, external: false, language: "eng", title: "SDH" }), + track({ id: 5, external: false, language: "eng" }), + track({ id: 6, external: false, language: "eng" }), ]; - expect(resolve(streams, 1, player)).toEqual({ kind: "select", trackId: 2 }); + expect(resolve(streams, 0, player)).toEqual({ kind: "select", trackId: 5 }); + expect(resolve(streams, 1, player)).toEqual({ kind: "select", trackId: 6 }); }); test("falls back to embedded ordinal when no language/title info", () => { diff --git a/utils/jellyfin/subtitleUtils.ts b/utils/jellyfin/subtitleUtils.ts index 7cb23e84..bce3ffc2 100644 --- a/utils/jellyfin/subtitleUtils.ts +++ b/utils/jellyfin/subtitleUtils.ts @@ -27,24 +27,34 @@ const EXTERNAL_DELIVERY = "External" as SubtitleDeliveryMethod; export const isImageBasedSubtitle = (sub: MediaStream): boolean => sub.IsTextSubtitleStream === false; -/** A Jellyfin subtitle stream is "external" when the server delivers it as a sidecar file. */ +/** + * A Jellyfin subtitle stream is "external" when the server delivers it as a + * sub-added sidecar — i.e. `DeliveryMethod === External` (or the `IsExternal` + * flag before a device-specific delivery method is assigned). + * + * Deliberately NOT keyed on `DeliveryUrl`: an Hls-delivered sub also carries a + * `DeliveryUrl` but lives inside the player's track list (not `sub-add`-ed), so + * it must resolve through the embedded path. Keeping this in lockstep with the + * load sites (which only `sub-add` `DeliveryMethod === External`) and with the + * menu comparator below avoids a sub being sorted as embedded yet resolved as + * external (→ `notFound`). + */ export const isExternalSubtitle = (sub: MediaStream): boolean => - sub.DeliveryMethod === EXTERNAL_DELIVERY || - sub.IsExternal === true || - Boolean(sub.DeliveryUrl); + sub.DeliveryMethod === EXTERNAL_DELIVERY || sub.IsExternal === true; /** - * Order subtitle/audio MediaStreams for the selection menu exactly like - * jellyfin-web's `itemHelper.sortTracks`: in-container tracks first then - * external, and within each group forced first, then default, then `Index` - * ascending. Callers prepend their own "None/Off" entry separately. + * Order subtitle MediaStreams for the selection menu exactly like jellyfin-web's + * `itemHelper.sortTracks`: in-container tracks first then external, and within + * each group forced first, then default, then `Index` ascending. Callers prepend + * their own "None/Off" entry separately. * * The Jellyfin server inserts external (sidecar) streams at the FRONT of * `MediaStreams` (low indices), so raw Index order shows externals first — this - * comparator flips that to match web (externals last). + * comparator flips that to match web (externals last). Uses {@link isExternalSubtitle} + * (not the raw `IsExternal` flag) so ordering and resolution agree. */ export const compareTracksForMenu = (a: MediaStream, b: MediaStream): number => - Number(a.IsExternal ?? false) - Number(b.IsExternal ?? false) || + Number(isExternalSubtitle(a)) - Number(isExternalSubtitle(b)) || Number(b.IsForced ?? false) - Number(a.IsForced ?? false) || Number(b.IsDefault ?? false) - Number(a.IsDefault ?? false) || (a.Index ?? 0) - (b.Index ?? 0); @@ -243,28 +253,35 @@ export const applyMpvSubtitleSelection = async ( ): Promise => { if (!player) return { kind: "notFound" }; - const tracks = (await player.getSubtitleTracks()) ?? []; - const selection = resolveSubtitleTrack({ - subtitleStreams: params.subtitleStreams, - jellyfinSubtitleIndex: params.jellyfinSubtitleIndex, - playerTracks: tracks.map((t) => ({ - id: t.id, - external: t.external, - externalFilename: t.externalFilename, - language: t.lang, - title: t.title, - codec: t.codec, - })), - getExpectedExternalUrl: params.getExpectedExternalUrl, - }); + // Called fire-and-forget (`void applyMpvSubtitleSelection(...)`), so any native + // rejection from getSubtitleTracks/setSubtitleTrack/disableSubtitles must be + // swallowed here instead of escaping as an unhandled promise rejection. + try { + const tracks = (await player.getSubtitleTracks()) ?? []; + const selection = resolveSubtitleTrack({ + subtitleStreams: params.subtitleStreams, + jellyfinSubtitleIndex: params.jellyfinSubtitleIndex, + playerTracks: tracks.map((t) => ({ + id: t.id, + external: t.external, + externalFilename: t.externalFilename, + language: t.lang, + title: t.title, + codec: t.codec, + })), + getExpectedExternalUrl: params.getExpectedExternalUrl, + }); - if (selection.kind === "select") { - await player.setSubtitleTrack(selection.trackId); - } else if (selection.kind === "disable") { - await player.disableSubtitles(); + if (selection.kind === "select") { + await player.setSubtitleTrack(selection.trackId); + } else if (selection.kind === "disable") { + await player.disableSubtitles(); + } + // notFound → leave current selection (e.g. image subs burned in while transcoding) + return selection; + } catch { + return { kind: "notFound" }; } - // notFound → leave current selection (e.g. image subs burned in while transcoding) - return selection; }; /**