diff --git a/app/src/components/VideoEditMenu.js b/app/src/components/VideoEditMenu.js index 5fa2eee6..4cf981a4 100644 --- a/app/src/components/VideoEditMenu.js +++ b/app/src/components/VideoEditMenu.js @@ -379,14 +379,27 @@ export function EditVideoModal({open, onClose, fileGroupId, videoFile, descripti const [title, setTitle] = useState(''); const [description, setDescription] = useState(''); const [saving, setSaving] = useState(false); + // The description last copied into the textarea; the user has typed when the field differs. + const syncedDescription = useRef(''); useEffect(() => { if (open) { // Start from the current details on every open, discarding an abandoned edit. setTitle(videoFile?.title || ''); setDescription(currentDescription || ''); + syncedDescription.current = currentDescription || ''; } - }, [open, videoFile?.title, currentDescription]); + }, [open, videoFile?.title]); + + useEffect(() => { + // The description may still be loading when the dialog opens (null), and it is refetched + // after a save. Adopt what arrives only while the user has not typed, so a draft written + // over the placeholder is never wiped by the empty or failed value that follows it. + if (open && currentDescription != null && description === syncedDescription.current) { + setDescription(currentDescription); + syncedDescription.current = currentDescription; + } + }, [currentDescription]); const trimmed = title.trim(); const titleChanged = trimmed !== (videoFile?.title || ''); diff --git a/app/src/components/VideoEditMenu.test.js b/app/src/components/VideoEditMenu.test.js index c74c4c59..958785af 100644 --- a/app/src/components/VideoEditMenu.test.js +++ b/app/src/components/VideoEditMenu.test.js @@ -269,6 +269,26 @@ describe('EditVideoModal', () => { await waitFor(() => expect(updateVideo).toHaveBeenCalledWith(7, {description: ''})); }); + test('a draft typed while the description was still loading survives its arrival', () => { + // The page passes null while the description is pending, then '' when the server has none. + const {rerender} = renderWithProviders( + , + ); + fireEvent.change(screen.getByLabelText('Description'), {target: {value: 'my new words'}}); + rerender(); + expect(screen.getByLabelText('Description')).toHaveValue('my new words'); + }); + + test('a description that arrives while the field is untouched is adopted', () => { + const {rerender} = renderWithProviders( + , + ); + expect(screen.getByLabelText('Description')).toHaveValue(''); + rerender(); + expect(screen.getByLabelText('Description')).toHaveValue('From the server'); + }); + test('WROL Mode disables saving', async () => { renderWithProviders( , diff --git a/app/src/components/VideoExtras.test.js b/app/src/components/VideoExtras.test.js index a0836519..72f7d259 100644 --- a/app/src/components/VideoExtras.test.js +++ b/app/src/components/VideoExtras.test.js @@ -12,8 +12,9 @@ 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(); + test('pending: shows a placeholder, not "no comments" or an error', () => { + const {container} = render(); + expect(container.querySelector('.wrolpi-placeholder')).toBeInTheDocument(); expect(screen.queryByText(NO_COMMENTS)).not.toBeInTheDocument(); expect(screen.queryByText(/Could not/)).not.toBeInTheDocument(); expect(screen.queryByRole('button', {name: 'Refresh'})).not.toBeInTheDocument(); @@ -39,8 +40,9 @@ describe('Comments', () => { }); describe('VideoDescription', () => { - test('pending: shows neither "no description" nor an error', () => { - render( null}/>); + test('pending: shows a placeholder, not "no description" or an error', () => { + const {container} = render( null}/>); + expect(container.querySelector('.wrolpi-placeholder')).toBeInTheDocument(); expect(screen.queryByText(NO_DESCRIPTION)).not.toBeInTheDocument(); expect(screen.queryByText(/Could not/)).not.toBeInTheDocument(); }); diff --git a/app/src/hooks/customHooks.js b/app/src/hooks/customHooks.js index f8cbed02..e54ae984 100644 --- a/app/src/hooks/customHooks.js +++ b/app/src/hooks/customHooks.js @@ -773,40 +773,47 @@ export const useVideoExtras = (fileGroupId) => { // 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); + // Every fetch takes the next generation number; only the newest generation may write state. + // Matching on the video id is not enough: leaving and returning to the same video, or a save + // that refetches the description, must both beat an older request still in flight. + const commentsGen = useRef(0); + const descriptionGen = useRef(0); const fetchComments = async () => { + const gen = ++commentsGen.current; try { const result = await getVideoComments(fileGroupId); - if (currentIdRef.current === fileGroupId) { + if (gen === commentsGen.current) { setComments(result.comments || []); } } catch (e) { console.error(e); - if (currentIdRef.current === fileGroupId) { + if (gen === commentsGen.current) { setComments(undefined); } } } const fetchDescription = async () => { + const gen = ++descriptionGen.current; try { const result = await getVideoDescription(fileGroupId); - if (currentIdRef.current === fileGroupId) { + if (gen === descriptionGen.current) { setDescription(result.description || ''); } } catch (e) { console.error(e); - if (currentIdRef.current === fileGroupId) { + if (gen === descriptionGen.current) { setDescription(undefined); } } } useEffect(() => { - // Whether switching to a new video or to none, the previous video's data must not linger. - currentIdRef.current = fileGroupId; + // Whether switching to a new video or to none, the previous video's data must not linger, + // and any request still in flight for it must not land. + commentsGen.current++; + descriptionGen.current++; setComments(null); setDescription(null); if (fileGroupId) { diff --git a/app/src/hooks/customHooks.test.js b/app/src/hooks/customHooks.test.js index 656f50fa..9c010598 100644 --- a/app/src/hooks/customHooks.test.js +++ b/app/src/hooks/customHooks.test.js @@ -334,14 +334,17 @@ describe('useVideoExtras', () => { 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 spy = 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(); + try { + const {result} = renderHook(() => useVideoExtras(7)); + await act(async () => { + }); + expect(result.current.comments).toBeUndefined(); + expect(result.current.description).toBeUndefined(); + } finally { + spy.mockRestore(); + } }); test('switching video resets both to pending before the new responses arrive', async () => { @@ -383,4 +386,58 @@ describe('useVideoExtras', () => { expect(result.current.comments).toEqual([{id: 'c8', parent: 'root'}]); expect(result.current.description).toBe('eighth'); }); + + test('returning to a video does not let its first, still-pending request land', async () => { + // The route clears the video between pages, so the id goes 7 -> undefined -> 8 -> 7 while + // the very first request for 7 is still in flight (the API timeout is a minute). + let rejectComments, rejectDescription; + getVideoComments.mockReturnValueOnce(new Promise((_, rej) => rejectComments = rej)); + getVideoDescription.mockReturnValueOnce(new Promise((_, rej) => rejectDescription = rej)); + const {result, rerender} = renderHook(({id}) => useVideoExtras(id), {initialProps: {id: 7}}); + + rerender({id: undefined}); + getVideoComments.mockResolvedValue({comments: [{id: 'c8', parent: 'root'}]}); + getVideoDescription.mockResolvedValue({description: 'eighth'}); + rerender({id: 8}); + getVideoComments.mockResolvedValue({comments: [{id: 'c7b', parent: 'root'}]}); + getVideoDescription.mockResolvedValue({description: 'seventh again'}); + rerender({id: 7}); + await act(async () => { + }); + expect(result.current.comments).toEqual([{id: 'c7b', parent: 'root'}]); + expect(result.current.description).toBe('seventh again'); + + // The first request finally times out. Same id, but it is not the newest request. + const spy = jest.spyOn(console, 'error').mockImplementation(() => { + }); + try { + await act(async () => { + rejectComments(new Error('timeout')); + rejectDescription(new Error('timeout')); + }); + } finally { + spy.mockRestore(); + } + expect(result.current.comments).toEqual([{id: 'c7b', parent: 'root'}]); + expect(result.current.description).toBe('seventh again'); + }); + + test('a refetch after saving beats the initial description request still in flight', async () => { + let resolveInitial; + getVideoComments.mockResolvedValue({comments: []}); + getVideoDescription.mockReturnValueOnce(new Promise(res => resolveInitial = res)); + const {result} = renderHook(() => useVideoExtras(7)); + + // The user edits and saves before the page's first description request returned. + getVideoDescription.mockResolvedValue({description: 'edited'}); + await act(async () => { + await result.current.fetchDescription(); + }); + expect(result.current.description).toBe('edited'); + + await act(async () => { + resolveInitial({description: 'pre-edit'}); + }); + expect(result.current.description).toBe('edited'); + }); });