From e3f4eea1324f71dda35eecc2e11bf81426934590 Mon Sep 17 00:00:00 2001 From: Gauvain Date: Wed, 27 May 2026 20:08:21 +0200 Subject: [PATCH] fix(chapters): address review findings + trickplay polish Copilot + CodeRabbit review findings: - React.memo ChapterTicks and ChapterList (project guideline: hot-path components must use React.memo to cut redraw work during control updates). - chapterNameAt now sorts the chapter array once instead of twice per call. The previous version went through currentChapterIndex (chapterStartsMs + sort) then sortedChapters (sort again). Runs on every playback tick, so the duplicate work added up. - Import getUserLibraryApi from the public barrel (@jellyfin/sdk/lib/utils/api) instead of the deep internal path (@jellyfin/sdk/lib/utils/api/user-library-api) to match the rest of the codebase and avoid coupling to SDK file layout. TrickplayBubble polish: - Sit just above the slider (bottom: 0) so the bubble no longer overlaps the progress bar. - Move the chapter-name + timestamp overlay to the bottom-left of the preview frame, smaller font, in front of the surrounding overlays (zIndex + elevation). BottomControls cleanup: - Drop dev-only "pick one to test" comment in favour of a one-line note on TICK_HEIGHT. - Inline scrubMs into its useMemo callback so the scrub-chapter-name lookup only recomputes while a slide is active. --- components/DownloadItem.tsx | 2 +- components/chapters/ChapterList.tsx | 6 ++++-- components/chapters/ChapterTicks.tsx | 6 ++++-- components/video-player/controls/BottomControls.tsx | 1 - components/video-player/controls/TrickplayBubble.tsx | 5 +++-- utils/chapters.ts | 11 +++++++++-- 6 files changed, 21 insertions(+), 10 deletions(-) diff --git a/components/DownloadItem.tsx b/components/DownloadItem.tsx index 4adfabc0..0d2b8ffb 100644 --- a/components/DownloadItem.tsx +++ b/components/DownloadItem.tsx @@ -9,7 +9,7 @@ import type { BaseItemDto, MediaSourceInfo, } from "@jellyfin/sdk/lib/generated-client/models"; -import { getUserLibraryApi } from "@jellyfin/sdk/lib/utils/api/user-library-api"; +import { getUserLibraryApi } from "@jellyfin/sdk/lib/utils/api"; import { type Href } from "expo-router"; import { t } from "i18next"; import { useAtom } from "jotai"; diff --git a/components/chapters/ChapterList.tsx b/components/chapters/ChapterList.tsx index e4927e6b..42a90b89 100644 --- a/components/chapters/ChapterList.tsx +++ b/components/chapters/ChapterList.tsx @@ -6,7 +6,7 @@ import { Ionicons } from "@expo/vector-icons"; import type { ChapterInfo } from "@jellyfin/sdk/lib/generated-client/models"; -import { useEffect, useMemo, useRef } from "react"; +import { memo, useEffect, useMemo, useRef } from "react"; import { useTranslation } from "react-i18next"; import { FlatList, Modal, Pressable, StyleSheet, View } from "react-native"; import { Text } from "@/components/common/Text"; @@ -30,7 +30,7 @@ interface ChapterListProps { const ROW_HEIGHT = 48; -export function ChapterList({ +function ChapterListComponent({ visible, chapters, currentPositionMs, @@ -151,6 +151,8 @@ export function ChapterList({ ); } +export const ChapterList = memo(ChapterListComponent); + const styles = StyleSheet.create({ backdrop: { flex: 1, diff --git a/components/chapters/ChapterTicks.tsx b/components/chapters/ChapterTicks.tsx index e4ec5ca6..850c63bf 100644 --- a/components/chapters/ChapterTicks.tsx +++ b/components/chapters/ChapterTicks.tsx @@ -4,7 +4,7 @@ * so the slider underneath still receives touches. */ -import { useState } from "react"; +import { memo, useState } from "react"; import { type LayoutChangeEvent, PixelRatio, View } from "react-native"; import type { ChapterMarker } from "@/utils/chapters"; @@ -19,7 +19,7 @@ interface ChapterTicksProps { width?: number; } -export function ChapterTicks({ +function ChapterTicksComponent({ markers, // Semi-transparent black contrasts against both the filled progress // (#fff) and the unfilled track (rgba(255,255,255,0.2)) so the ticks @@ -83,3 +83,5 @@ export function ChapterTicks({ ); } + +export const ChapterTicks = memo(ChapterTicksComponent); diff --git a/components/video-player/controls/BottomControls.tsx b/components/video-player/controls/BottomControls.tsx index 85e35fc8..af445d37 100644 --- a/components/video-player/controls/BottomControls.tsx +++ b/components/video-player/controls/BottomControls.tsx @@ -189,7 +189,6 @@ export const BottomControls: FC = ({ onPress={() => setChapterListVisible(true)} hitSlop={10} className='justify-center mr-4' - style={{ marginTop: 10 }} accessibilityRole='button' accessibilityLabel={t("chapters.open")} > diff --git a/components/video-player/controls/TrickplayBubble.tsx b/components/video-player/controls/TrickplayBubble.tsx index 11c702a0..ea126a12 100644 --- a/components/video-player/controls/TrickplayBubble.tsx +++ b/components/video-player/controls/TrickplayBubble.tsx @@ -52,8 +52,9 @@ export const TrickplayBubble: FC = ({ style={{ position: "absolute", left: -62, - // Drop the bubble closer to the slider — less floating-high feel. - bottom: -20, + // Sit just above the slider — high enough not to overlap the + // progress bar, low enough to feel anchored to the thumb. + bottom: 0, paddingTop: 12, paddingBottom: 5, width: tileWidth * 1.5, diff --git a/utils/chapters.ts b/utils/chapters.ts index d76b5521..8b0e0e7b 100644 --- a/utils/chapters.ts +++ b/utils/chapters.ts @@ -70,9 +70,16 @@ export const chapterNameAt = ( positionMs: number, chapters: ChapterInfo[] | null | undefined, ): string | null => { - const idx = currentChapterIndex(positionMs, chapters); - if (idx < 0) return null; + // Sort once, derive both the active index and the entry from the same array + // — `chapterNameAt` runs on every playback tick, so paying for one `sort()` + // instead of two is worth the duplication of the index loop here. const sorted = sortedChapters(chapters); + let idx = -1; + for (let i = 0; i < sorted.length; i++) { + if (positionMs >= sorted[i].positionMs) idx = i; + else break; + } + if (idx < 0) return null; const name = sorted[idx]?.chapter.Name; return name && name.length > 0 ? name : null; };