From 106596f9345c4d7aad4677b8cdac96ec7db82c71 Mon Sep 17 00:00:00 2001 From: Roland Date: Fri, 25 Sep 2026 14:13:44 -0600 Subject: [PATCH] app: video comments and description distinguish pending from empty useVideoExtras initialised both values to null and the page treated null the same as "none", so every video open (and every prev/next) showed "No comments have been downloaded. Refresh the video?" with a live Refresh button until the comments arrived. The Description tab showed "No description available." the same way. A failed fetch was also rendered as "none", and the previous video's data lingered until the next video's responses arrived. Invariants now enforced and tested: * null = pending, undefined = fetch failed, [] / '' = loaded and empty. The API serialises a missing info_json field as null, so the hook normalises that to empty; consumers never see null for "none". * Switching video resets both to pending, and a slow response for the previous video is dropped. * The page renders a placeholder while pending, an error message on failure, and offers Refresh only once the server confirmed there are no comments. Co-Authored-By: Claude Fable 5.1 --- app/src/components/VideoExtras.test.js | 64 +++++++++++++++++++++ app/src/components/VideoPlayer.js | 35 ++++++++---- app/src/hooks/customHooks.js | 27 +++++++-- app/src/hooks/customHooks.test.js | 77 ++++++++++++++++++++++++++ 4 files changed, 186 insertions(+), 17 deletions(-) create mode 100644 app/src/components/VideoExtras.test.js diff --git a/app/src/components/VideoExtras.test.js b/app/src/components/VideoExtras.test.js new file mode 100644 index 00000000..a0836519 --- /dev/null +++ b/app/src/components/VideoExtras.test.js @@ -0,0 +1,64 @@ +import React from 'react'; +import {render, screen} from '../test-utils'; +import {Comments, VideoDescription} from './VideoPlayer'; + +// What the video page shows for each state of useVideoExtras. The states are null (pending), +// undefined (fetch failed), and an empty value (loaded, nothing to show). "No comments have been +// downloaded" carries a Refresh button that re-downloads the video, so it must only appear once +// the server has confirmed there are none. + +const video = {id: 7, url: 'https://example.com/v'}; +const NO_COMMENTS = /No comments have been/; +const NO_DESCRIPTION = /No description available/; + +describe('Comments', () => { + test('pending: shows neither "no comments" nor an error', () => { + render(); + expect(screen.queryByText(NO_COMMENTS)).not.toBeInTheDocument(); + expect(screen.queryByText(/Could not/)).not.toBeInTheDocument(); + expect(screen.queryByRole('button', {name: 'Refresh'})).not.toBeInTheDocument(); + }); + + test('failed: shows an error, not "no comments"', () => { + render(); + expect(screen.getByText(/Could not fetch the comments/)).toBeInTheDocument(); + expect(screen.queryByText(NO_COMMENTS)).not.toBeInTheDocument(); + }); + + test('empty: shows "no comments" with the Refresh button', () => { + render(); + expect(screen.getByText(NO_COMMENTS)).toBeInTheDocument(); + expect(screen.getByRole('button', {name: 'Refresh'})).toBeInTheDocument(); + }); + + test('loaded: renders the comments', () => { + render(); + expect(screen.getByText('first!')).toBeInTheDocument(); + expect(screen.queryByText(NO_COMMENTS)).not.toBeInTheDocument(); + }); +}); + +describe('VideoDescription', () => { + test('pending: shows neither "no description" nor an error', () => { + render( null}/>); + expect(screen.queryByText(NO_DESCRIPTION)).not.toBeInTheDocument(); + expect(screen.queryByText(/Could not/)).not.toBeInTheDocument(); + }); + + test('failed: shows an error, not "no description"', () => { + render( null}/>); + expect(screen.getByText(/Could not fetch the description/)).toBeInTheDocument(); + expect(screen.queryByText(NO_DESCRIPTION)).not.toBeInTheDocument(); + }); + + test('empty: shows "no description"', () => { + render( null}/>); + expect(screen.getByText(NO_DESCRIPTION)).toBeInTheDocument(); + }); + + test('loaded: renders the description', () => { + render( null}/>); + expect(screen.getByText('A fine video.')).toBeInTheDocument(); + expect(screen.queryByText(NO_DESCRIPTION)).not.toBeInTheDocument(); + }); +}); diff --git a/app/src/components/VideoPlayer.js b/app/src/components/VideoPlayer.js index d957f773..106296f2 100644 --- a/app/src/components/VideoPlayer.js +++ b/app/src/components/VideoPlayer.js @@ -8,6 +8,7 @@ import { BackButton, DirectoryLink, encodeMediaPath, + ErrorMessage, humanFileSize, humanNumber, isoDatetimeToAgoPopup, @@ -19,7 +20,7 @@ import { } from "./Common"; import {VideoPlaceholder} from "./Placeholder"; import {useChannel, useVideoCaptions, useVideoExtras} from "../hooks/customHooks"; -import {Button, Grid, Header, Icon, Label, Loading, Panel, Tabs, TextInput} from "./ui"; +import {Button, Grid, Header, Icon, Label, Loading, Panel, Placeholder, Tabs, TextInput} from "./ui"; import {VideoCard} from "./Videos"; import {TagsSelector} from "../Tags"; import {AddToPlaylistButton} from "./AddToPlaylist"; @@ -170,8 +171,14 @@ const VideoComment = ({comment, children}) => { } -const Comments = ({comments, video}) => { - if (!comments || _.isEmpty(comments)) { +// `comments` follows useVideoExtras: null = pending, undefined = fetch failed, [] = none downloaded. +// Only the last of those may offer the Refresh button. +export const Comments = ({comments, video}) => { + if (comments === null) { + return + } else if (comments === undefined) { + return Could not fetch the comments. + } else if (_.isEmpty(comments)) { return } @@ -343,12 +350,6 @@ function VideoPage({videoFile, prevFile, nextFile, fetchVideo, ...props}) { caption_files.filter(i => i['mimetype'] === 'text/vtt').map(i => `${MEDIA_PATH}/${encodeMediaPath(i['path'])}`) : []; - let description = 'No description available.'; - if (videoDescription) { - // Only replace empty description if there is one available. - description = formatVideoDescription(videoDescription, setVideoTime) - } - const getFile = (suffix) => videoFile.files.find(i => i.path.toLowerCase().endsWith(suffix)); const fileSize = (file) => file && file.size ? ({humanFileSize(file.size)}) @@ -550,9 +551,7 @@ function VideoPage({videoFile, prevFile, nextFile, fetchVideo, ...props}) { - 
-                        {description}
-
+ @@ -717,6 +716,18 @@ export default VideoPage; const chapterRegex = new RegExp('^(\\(?(?:((\\d?\\d):)?(?:(\\d?\\d):(\\d\\d)))\\)?)\.?\\s+(.*)$', 'i'); +// `description` follows useVideoExtras: null = pending, undefined = fetch failed, '' = none. +export const VideoDescription = ({description, setVideoTime}) => { + if (description === null) { + return + } else if (description === undefined) { + return Could not fetch the description. + } + return 
+        {description ? formatVideoDescription(description, setVideoTime) : 'No description available.'}
+
+} + function formatVideoDescription(description, setVideoTime) { // Convert timestamps to links which change the video's playback location. Change hashtags to search links. if (description && description.length > 0) { diff --git a/app/src/hooks/customHooks.js b/app/src/hooks/customHooks.js index b0e93298..f8cbed02 100644 --- a/app/src/hooks/customHooks.js +++ b/app/src/hooks/customHooks.js @@ -767,34 +767,51 @@ export const useVideo = (fileGroupId) => { export const useVideoExtras = (fileGroupId) => { // Fetches extra data to display on a Video's page. These are served separately from the // Video itself so list responses stay small and never read files from disk. + // + // Each value is null while pending, undefined when the fetch failed, and empty ([] or '') when + // the server confirmed there is nothing. The API serializes a missing info_json field as null, + // so a null response is normalized to empty here; consumers must never see null for "none". const [comments, setComments] = useState(null); const [description, setDescription] = useState(null); + // The video currently shown. A response for any other video is stale and is dropped. + const currentIdRef = useRef(fileGroupId); const fetchComments = async () => { try { const result = await getVideoComments(fileGroupId); - setComments(result.comments); + if (currentIdRef.current === fileGroupId) { + setComments(result.comments || []); + } } catch (e) { console.error(e); + if (currentIdRef.current === fileGroupId) { + setComments(undefined); + } } } const fetchDescription = async () => { try { const result = await getVideoDescription(fileGroupId); - setDescription(result.description); + if (currentIdRef.current === fileGroupId) { + setDescription(result.description || ''); + } } catch (e) { console.error(e); + if (currentIdRef.current === fileGroupId) { + setDescription(undefined); + } } } useEffect(() => { + // Whether switching to a new video or to none, the previous video's data must not linger. + currentIdRef.current = fileGroupId; + setComments(null); + setDescription(null); if (fileGroupId) { fetchComments(); fetchDescription(); - } else { - setComments(null); - setDescription(null); } }, [fileGroupId]); diff --git a/app/src/hooks/customHooks.test.js b/app/src/hooks/customHooks.test.js index 7e8f8183..656f50fa 100644 --- a/app/src/hooks/customHooks.test.js +++ b/app/src/hooks/customHooks.test.js @@ -306,4 +306,81 @@ describe('useVideoExtras', () => { expect(result.current.comments).toBeNull(); expect(result.current.description).toBeNull(); }); + + // State contract, matching the rest of the hooks: null = pending, undefined = error, + // [] / '' = loaded and empty. The page branches on these to choose between a placeholder, + // an error, and "No comments have been downloaded". + test('both are pending (null) until the responses arrive', () => { + getVideoComments.mockReturnValue(new Promise(() => { + })); + getVideoDescription.mockReturnValue(new Promise(() => { + })); + const {result} = renderHook(() => useVideoExtras(7)); + expect(result.current.comments).toBeNull(); + expect(result.current.description).toBeNull(); + }); + + test('a video with no comments or description loads as empty, not pending', async () => { + // The API serializes a missing info_json field as null. + getVideoComments.mockResolvedValue({comments: null}); + getVideoDescription.mockResolvedValue({description: null}); + const {result} = renderHook(() => useVideoExtras(7)); + await act(async () => { + }); + expect(result.current.comments).toEqual([]); + expect(result.current.description).toBe(''); + }); + + test('a failed fetch yields undefined so the page can show an error instead of "no comments"', async () => { + getVideoComments.mockRejectedValue(new Error('boom')); + getVideoDescription.mockRejectedValue(new Error('boom')); + jest.spyOn(console, 'error').mockImplementation(() => { + }); + const {result} = renderHook(() => useVideoExtras(7)); + await act(async () => { + }); + expect(result.current.comments).toBeUndefined(); + expect(result.current.description).toBeUndefined(); + console.error.mockRestore(); + }); + + test('switching video resets both to pending before the new responses arrive', async () => { + getVideoComments.mockResolvedValue({comments: [{id: 'c1', parent: 'root'}]}); + getVideoDescription.mockResolvedValue({description: 'first'}); + const {result, rerender} = renderHook(({id}) => useVideoExtras(id), {initialProps: {id: 7}}); + await act(async () => { + }); + expect(result.current.description).toBe('first'); + + // The next video's responses are slow: the previous video's data must not linger. + getVideoComments.mockReturnValue(new Promise(() => { + })); + getVideoDescription.mockReturnValue(new Promise(() => { + })); + rerender({id: 8}); + expect(result.current.comments).toBeNull(); + expect(result.current.description).toBeNull(); + }); + + test('a slow response for the previous video does not overwrite the current one', async () => { + // Video 7's responses are slow; the user moves on to video 8 before they arrive. + let resolveComments, resolveDescription; + getVideoComments.mockReturnValueOnce(new Promise(res => resolveComments = res)); + getVideoDescription.mockReturnValueOnce(new Promise(res => resolveDescription = res)); + const {result, rerender} = renderHook(({id}) => useVideoExtras(id), {initialProps: {id: 7}}); + + getVideoComments.mockResolvedValue({comments: [{id: 'c8', parent: 'root'}]}); + getVideoDescription.mockResolvedValue({description: 'eighth'}); + rerender({id: 8}); + await act(async () => { + }); + expect(result.current.description).toBe('eighth'); + + await act(async () => { + resolveComments({comments: [{id: 'c7', parent: 'root'}]}); + resolveDescription({description: 'seventh'}); + }); + expect(result.current.comments).toEqual([{id: 'c8', parent: 'root'}]); + expect(result.current.description).toBe('eighth'); + }); });