fix(subtitles): address CodeRabbit review

- 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).
2026-06-30 09:32:50 +01:00 · 2026-06-30 01:08:50 +02:00
parent 1c158dea4e
commit 90ea934548
4 changed files with 76 additions and 52 deletions
--- 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", () => {
--- 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<SubtitleSelection> => {
  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;
 };

 /**