From ef1211401e21ea132c1966dd4fb51ebbf51934fc Mon Sep 17 00:00:00 2001 From: Tim Down <158919+timdown@users.noreply.github.com> Date: Wed, 24 May 2023 11:02:21 +0100 Subject: [PATCH] History migration: Abort previous GET requests (#12893) GitOrigin-RevId: ad72d526bf7cdfa91b00025b58205d9a5b4cc62f --- .../components/change-list/change-list.tsx | 20 ++- .../components/diff-view/diff-view.tsx | 54 +++++--- .../diff-view/document-diff-viewer.tsx | 4 +- .../history/components/error-message.tsx | 13 -- .../history/components/history-file-tree.tsx | 6 +- .../history/components/history-root.tsx | 11 +- .../history/context/history-context.tsx | 129 ++++++++++-------- .../context/hooks/use-restore-deleted-file.ts | 6 +- .../context/types/history-context-value.ts | 4 +- .../js/features/history/services/api.ts | 26 ++-- .../history/utils/auto-select-file.ts | 14 +- .../history/utils/auto-select-file.test.ts | 18 ++- 12 files changed, 166 insertions(+), 139 deletions(-) delete mode 100644 services/web/frontend/js/features/history/components/error-message.tsx diff --git a/services/web/frontend/js/features/history/components/change-list/change-list.tsx b/services/web/frontend/js/features/history/components/change-list/change-list.tsx index 93613ce4e2..cb0e93fae4 100644 --- a/services/web/frontend/js/features/history/components/change-list/change-list.tsx +++ b/services/web/frontend/js/features/history/components/change-list/change-list.tsx @@ -4,23 +4,19 @@ import LabelsList from './labels-list' import { useHistoryContext } from '../../context/history-context' function ChangeList() { - const { error, labelsOnly, setLabelsOnly } = useHistoryContext() + const { labelsOnly, setLabelsOnly } = useHistoryContext() return ( ) } diff --git a/services/web/frontend/js/features/history/components/diff-view/diff-view.tsx b/services/web/frontend/js/features/history/components/diff-view/diff-view.tsx index 4d29613a28..02e0045423 100644 --- a/services/web/frontend/js/features/history/components/diff-view/diff-view.tsx +++ b/services/web/frontend/js/features/history/components/diff-view/diff-view.tsx @@ -5,15 +5,16 @@ import { Diff, DocDiffResponse } from '../../services/types/doc' import { useHistoryContext } from '../../context/history-context' import { diffDoc } from '../../services/api' import { highlightsFromDiffResponse } from '../../utils/highlights-from-diff-response' +import { useErrorHandler } from 'react-error-boundary' import useAsync from '../../../../shared/hooks/use-async' -import ErrorMessage from '../error-message' import { useTranslation } from 'react-i18next' function DiffView() { const { selection, projectId, loadingFileDiffs } = useHistoryContext() - const { isLoading, data, runAsync, error } = useAsync() + const { isLoading, data, runAsync } = useAsync() const { t } = useTranslation() const { updateRange, selectedFile } = selection + const handleError = useErrorHandler() useEffect(() => { if (!updateRange || !selectedFile?.pathname || loadingFileDiffs) { @@ -21,11 +22,36 @@ function DiffView() { } const { fromV, toV } = updateRange + let abortController: AbortController | null = new AbortController() - runAsync(diffDoc(projectId, fromV, toV, selectedFile.pathname)).catch( - console.error + runAsync( + diffDoc( + projectId, + fromV, + toV, + selectedFile.pathname, + abortController.signal + ) ) - }, [projectId, runAsync, updateRange, selectedFile, loadingFileDiffs]) + .catch(handleError) + .finally(() => { + abortController = null + }) + + // Abort an existing request before starting a new one or on unmount + return () => { + if (abortController) { + abortController.abort() + } + } + }, [ + projectId, + runAsync, + updateRange, + selectedFile, + loadingFileDiffs, + handleError, + ]) let diff: Diff | null @@ -42,18 +68,12 @@ function DiffView() { return (
- {error ? ( - - ) : ( - <> -
- -
-
-
-
- - )} +
+ +
+
+
+
) } diff --git a/services/web/frontend/js/features/history/components/diff-view/document-diff-viewer.tsx b/services/web/frontend/js/features/history/components/diff-view/document-diff-viewer.tsx index 274a7bfd52..f5d3c3062d 100644 --- a/services/web/frontend/js/features/history/components/diff-view/document-diff-viewer.tsx +++ b/services/web/frontend/js/features/history/components/diff-view/document-diff-viewer.tsx @@ -1,6 +1,4 @@ import { useCallback, useEffect, useState } from 'react' -import withErrorBoundary from '../../../../infrastructure/error-boundary' -import { ErrorBoundaryFallback } from '../../../../shared/components/error-boundary-fallback' import { EditorSelection, EditorState, @@ -135,4 +133,4 @@ function DocumentDiffViewer({ ) } -export default withErrorBoundary(DocumentDiffViewer, ErrorBoundaryFallback) +export default DocumentDiffViewer diff --git a/services/web/frontend/js/features/history/components/error-message.tsx b/services/web/frontend/js/features/history/components/error-message.tsx deleted file mode 100644 index dda8cb4c99..0000000000 --- a/services/web/frontend/js/features/history/components/error-message.tsx +++ /dev/null @@ -1,13 +0,0 @@ -import { useTranslation } from 'react-i18next' - -export default function ErrorMessage() { - const { t } = useTranslation() - - return ( -
-
- {t('generic_something_went_wrong')} -
-
- ) -} diff --git a/services/web/frontend/js/features/history/components/history-file-tree.tsx b/services/web/frontend/js/features/history/components/history-file-tree.tsx index 296e15115f..39cf775936 100644 --- a/services/web/frontend/js/features/history/components/history-file-tree.tsx +++ b/services/web/frontend/js/features/history/components/history-file-tree.tsx @@ -8,7 +8,7 @@ import { import HistoryFileTreeFolderList from './file-tree/history-file-tree-folder-list' export default function HistoryFileTree() { - const { selection, error } = useHistoryContext() + const { selection } = useHistoryContext() const fileTree = useMemo( () => reduce(selection.files, reducePathsToTree, []), @@ -25,10 +25,6 @@ export default function HistoryFileTree() { [sortedFileTree] ) - if (error) { - return null - } - return ( - } else if (error) { - content = } else { content = ( <> @@ -35,10 +34,12 @@ function Main() { ) } -export default function HistoryRoot() { +function HistoryRoot() { return (
) } + +export default withErrorBoundary(HistoryRoot, ErrorBoundaryFallback) diff --git a/services/web/frontend/js/features/history/context/history-context.tsx b/services/web/frontend/js/features/history/context/history-context.tsx index 17339cfa18..d82a26d8b0 100644 --- a/services/web/frontend/js/features/history/context/history-context.tsx +++ b/services/web/frontend/js/features/history/context/history-context.tsx @@ -26,6 +26,8 @@ import { Update, } from '../services/types/update' import { Selection } from '../services/types/selection' +import { useErrorHandler } from 'react-error-boundary' +import { getUpdateForVersion } from '../utils/history-details' // Allow testing of infinite scrolling by providing query string parameters to // limit the number of updates returned in a batch and apply a delay @@ -92,10 +94,17 @@ function useHistory() { `history.userPrefs.showOnlyLabels.${projectId}`, false ) - const [loadingFileDiffs, setLoadingFileDiffs] = useState(false) - const [error, setError] = useState(null) + + const updatesAbortControllerRef = useRef(null) + const handleError = useErrorHandler() const fetchNextBatchOfUpdates = useCallback(() => { + // If there is an in-flight request for updates, just let it complete, by + // bailing out + if (updatesAbortControllerRef.current) { + return + } + const updatesLoadingState = updatesInfo.loadingState const loadUpdates = (updatesData: Update[]) => { @@ -150,16 +159,20 @@ function useHistory() { return } + updatesAbortControllerRef.current = new AbortController() + const signal = updatesAbortControllerRef.current.signal + const updatesPromise = limitUpdates( - fetchUpdates(projectId, updatesInfo.nextBeforeTimestamp) + fetchUpdates(projectId, updatesInfo.nextBeforeTimestamp, signal) ) - const labelsPromise = labels == null ? fetchLabels(projectId) : null + const labelsPromise = labels == null ? fetchLabels(projectId, signal) : null setUpdatesInfo({ ...updatesInfo, loadingState: updatesLoadingState === 'ready' ? 'loadingUpdates' : 'loadingInitial', }) + Promise.all([updatesPromise, labelsPromise]) .then(([{ updates: updatesData, nextBeforeTimestamp }, labels]) => { const lastUpdateToV = updatesData.length ? updatesData[0].toV : null @@ -183,75 +196,83 @@ function useHistory() { loadingState: 'ready', }) }) - .catch(error => { - setError(error) - setUpdatesInfo({ ...updatesInfo, atEnd: true, loadingState: 'ready' }) + .catch(handleError) + .finally(() => { + updatesAbortControllerRef.current = null }) - }, [updatesInfo, projectId, labels, userHasFullFeature]) + }, [updatesInfo, projectId, labels, handleError, userHasFullFeature]) + + // Abort in-flight updates request on unmount + useEffect(() => { + return () => { + if (updatesAbortControllerRef.current) { + updatesAbortControllerRef.current.abort() + } + } + }, []) + + // Initial load on first render + const initialFetch = useRef(false) + useEffect(() => { + if (view === 'history' && !initialFetch.current) { + initialFetch.current = true + return fetchNextBatchOfUpdates() + } + }, [view, fetchNextBatchOfUpdates]) const resetSelection = useCallback(() => { setSelection(selectionInitialState) }, []) - // Initial load when the History tab is active - const initialFetch = useRef(false) - useEffect(() => { - if (view === 'history' && !initialFetch.current) { - fetchNextBatchOfUpdates() - initialFetch.current = true - } - }, [view, fetchNextBatchOfUpdates]) - - const { updateRange, comparing, files } = selection + const { updateRange } = selection + const { fromV, toV } = updateRange || {} const { updates } = updatesInfo - const filesEmpty = files.length === 0 + + const updateForToV = + toV === undefined ? undefined : getUpdateForVersion(toV, updates) // Load files when the update selection changes + const [loadingFileDiffs, setLoadingFileDiffs] = useState(false) + useEffect(() => { - if (!updateRange || updatesInfo.loadingState !== 'ready' || !filesEmpty) { + if (fromV === undefined || toV === undefined) { return } - const { fromV, toV } = updateRange + let abortController: AbortController | null = new AbortController() setLoadingFileDiffs(true) - diffFiles(projectId, fromV, toV) + diffFiles(projectId, fromV, toV, abortController.signal) .then(({ diff: files }) => { - const selectedFile = autoSelectFile( - files, - updateRange.toV, - comparing, - updates - ) - const newFiles = files.map(file => { - if (isFileRenamed(file) && file.newPathname) { - return renamePathnameKey(file) - } + setSelection(previousSelection => { + const selectedFile = autoSelectFile( + files, + toV, + previousSelection.comparing, + updateForToV + ) + const newFiles = files.map(file => { + if (isFileRenamed(file) && file.newPathname) { + return renamePathnameKey(file) + } - return file - }) - setSelection({ - updateRange, - comparing, - files: newFiles, - selectedFile, + return file + }) + return { ...previousSelection, files: newFiles, selectedFile } }) }) - .catch(error => { - setError(error) - }) + .catch(handleError) .finally(() => { setLoadingFileDiffs(false) + abortController = null }) - }, [ - updateRange, - projectId, - updates, - comparing, - setError, - updatesInfo.loadingState, - filesEmpty, - ]) + + return () => { + if (abortController) { + abortController.abort() + } + } + }, [projectId, fromV, toV, updateForToV, handleError]) useEffect(() => { // Set update range if there isn't one and updates have loaded @@ -271,10 +292,7 @@ function useHistory() { const value = useMemo( () => ({ - error, - setError, loadingFileDiffs, - setLoadingFileDiffs, updatesInfo, setUpdatesInfo, labels, @@ -290,10 +308,7 @@ function useHistory() { resetSelection, }), [ - error, - setError, loadingFileDiffs, - setLoadingFileDiffs, updatesInfo, setUpdatesInfo, labels, diff --git a/services/web/frontend/js/features/history/context/hooks/use-restore-deleted-file.ts b/services/web/frontend/js/features/history/context/hooks/use-restore-deleted-file.ts index 2382ed66d7..983f82f671 100644 --- a/services/web/frontend/js/features/history/context/hooks/use-restore-deleted-file.ts +++ b/services/web/frontend/js/features/history/context/hooks/use-restore-deleted-file.ts @@ -7,12 +7,14 @@ import { isFileRemoved } from '../../utils/file-diff' import { waitFor } from '../../utils/wait-for' import { useHistoryContext } from '../history-context' import type { HistoryContextValue } from '../types/history-context-value' +import { useErrorHandler } from 'react-error-boundary' export function useRestoreDeletedFile() { const { isLoading, runAsync } = useAsync() - const { projectId, setError } = useHistoryContext() + const { projectId } = useHistoryContext() const ide = useIdeContext() const { setView } = useLayoutContext() + const handleError = useErrorHandler() const restoreDeletedFile = async ( selection: HistoryContextValue['selection'] @@ -40,7 +42,7 @@ export function useRestoreDeletedFile() { setView('editor') }) - .catch(error => setError(error)) + .catch(handleError) ) } } diff --git a/services/web/frontend/js/features/history/context/types/history-context-value.ts b/services/web/frontend/js/features/history/context/types/history-context-value.ts index fca47cba3c..b68b22357b 100644 --- a/services/web/frontend/js/features/history/context/types/history-context-value.ts +++ b/services/web/frontend/js/features/history/context/types/history-context-value.ts @@ -20,8 +20,6 @@ export type HistoryContextValue = { userHasFullFeature: boolean currentUserIsOwner: boolean loadingFileDiffs: boolean - error: Nullable - setError: React.Dispatch> labels: Nullable setLabels: React.Dispatch> labelsOnly: boolean @@ -31,6 +29,6 @@ export type HistoryContextValue = { setSelection: React.Dispatch< React.SetStateAction > - fetchNextBatchOfUpdates: () => void + fetchNextBatchOfUpdates: () => (() => void) | void resetSelection: () => void } diff --git a/services/web/frontend/js/features/history/services/api.ts b/services/web/frontend/js/features/history/services/api.ts index a568ac5a17..22711b2492 100644 --- a/services/web/frontend/js/features/history/services/api.ts +++ b/services/web/frontend/js/features/history/services/api.ts @@ -11,7 +11,11 @@ import { RestoreFileResponse } from './types/restore-file' const BATCH_SIZE = 10 -export function fetchUpdates(projectId: string, before?: number) { +export function fetchUpdates( + projectId: string, + before?: number, + signal?: AbortSignal +) { const queryParams: Record = { min_count: BATCH_SIZE.toString(), } @@ -22,12 +26,12 @@ export function fetchUpdates(projectId: string, before?: number) { const queryParamsSerialized = new URLSearchParams(queryParams).toString() const updatesURL = `/project/${projectId}/updates?${queryParamsSerialized}` - return getJSON(updatesURL) + return getJSON(updatesURL, { signal }) } -export function fetchLabels(projectId: string) { +export function fetchLabels(projectId: string, signal?: AbortSignal) { const labelsURL = `/project/${projectId}/labels` - return getJSON(labelsURL) + return getJSON(labelsURL, { signal }) } export function addLabel( @@ -46,21 +50,27 @@ export function deleteLabel( return deleteJSON(`/project/${projectId}/labels/${labelId}`, { signal }) } -export function diffFiles(projectId: string, fromV: number, toV: number) { +export function diffFiles( + projectId: string, + fromV: number, + toV: number, + signal?: AbortSignal +) { const queryParams: Record = { from: fromV.toString(), to: toV.toString(), } const queryParamsSerialized = new URLSearchParams(queryParams).toString() const diffUrl = `/project/${projectId}/filetree/diff?${queryParamsSerialized}` - return getJSON<{ diff: FileDiff[] }>(diffUrl) + return getJSON<{ diff: FileDiff[] }>(diffUrl, { signal }) } export function diffDoc( projectId: string, fromV: number, toV: number, - pathname: string + pathname: string, + signal?: AbortSignal ) { const queryParams: Record = { from: fromV.toString(), @@ -69,7 +79,7 @@ export function diffDoc( } const queryParamsSerialized = new URLSearchParams(queryParams).toString() const diffUrl = `/project/${projectId}/diff?${queryParamsSerialized}` - return getJSON(diffUrl) + return getJSON(diffUrl, { signal }) } export function restoreFile(projectId: string, selectedFile: FileRemoved) { diff --git a/services/web/frontend/js/features/history/utils/auto-select-file.ts b/services/web/frontend/js/features/history/utils/auto-select-file.ts index 54c65e25cf..b23eaa7607 100644 --- a/services/web/frontend/js/features/history/utils/auto-select-file.ts +++ b/services/web/frontend/js/features/history/utils/auto-select-file.ts @@ -1,5 +1,4 @@ import _ from 'lodash' -import { getUpdateForVersion } from './history-details' import type { Nullable } from '../../../../../types/utils' import type { FileDiff } from '../services/types/file' import type { FileOperation } from '../services/types/file-operation' @@ -15,21 +14,20 @@ function getFilesWithOps( files: FileDiff[], toV: Version, comparing: boolean, - updates: LoadedUpdate[] + updateForToV: LoadedUpdate | undefined ): FileWithOps[] { if (toV && !comparing) { const filesWithOps: FileWithOps[] = [] - const currentUpdate = getUpdateForVersion(toV, updates) - if (currentUpdate) { - for (const pathname of currentUpdate.pathnames) { + if (updateForToV) { + for (const pathname of updateForToV.pathnames) { filesWithOps.push({ pathname, operation: 'edited', }) } - for (const op of currentUpdate.project_ops) { + for (const op of updateForToV.project_ops) { let fileWithOps: Nullable = null if (op.add) { @@ -86,11 +84,11 @@ export function autoSelectFile( files: FileDiff[], toV: Version, comparing: boolean, - updates: LoadedUpdate[] + updateForToV: LoadedUpdate | undefined ): FileDiff { let fileToSelect: Nullable = null - const filesWithOps = getFilesWithOps(files, toV, comparing, updates) + const filesWithOps = getFilesWithOps(files, toV, comparing, updateForToV) for (const opType of orderedOpTypes) { const fileWithMatchingOpType = _.find(filesWithOps, { operation: opType, diff --git a/services/web/test/frontend/features/history/utils/auto-select-file.test.ts b/services/web/test/frontend/features/history/utils/auto-select-file.test.ts index bb53e91dea..2293c339e7 100644 --- a/services/web/test/frontend/features/history/utils/auto-select-file.test.ts +++ b/services/web/test/frontend/features/history/utils/auto-select-file.test.ts @@ -4,6 +4,7 @@ import { autoSelectFile } from '../../../../../frontend/js/features/history/util import type { User } from '../../../../../frontend/js/features/history/services/types/shared' import { LoadedUpdate } from '../../../../../frontend/js/features/history/services/types/update' import { fileFinalPathname } from '../../../../../frontend/js/features/history/utils/file-diff' +import { getUpdateForVersion } from '../../../../../frontend/js/features/history/utils/history-details' describe('autoSelectFile', function () { const historyUsers: User[] = [ @@ -264,7 +265,7 @@ describe('autoSelectFile', function () { files, updates[0].toV, comparing, - updates + getUpdateForVersion(updates[0].toV, updates) ) expect(pathname).to.equal('newfolder1/newfile10.tex') @@ -334,7 +335,7 @@ describe('autoSelectFile', function () { files, updates[0].toV, comparing, - updates + getUpdateForVersion(updates[0].toV, updates) ) expect(pathname).to.equal('newfile1.tex') @@ -435,7 +436,7 @@ describe('autoSelectFile', function () { files, updates[0].toV, comparing, - updates + getUpdateForVersion(updates[0].toV, updates) ) expect(pathname).to.equal('main3.tex') @@ -606,7 +607,7 @@ describe('autoSelectFile', function () { files, updates[0].toV, comparing, - updates + getUpdateForVersion(updates[0].toV, updates) ) expect(pathname).to.equal('main.tex') @@ -714,7 +715,7 @@ describe('autoSelectFile', function () { files, updates[0].toV, comparing, - updates + getUpdateForVersion(updates[0].toV, updates) ) expect(pathname).to.equal('certainly_not_main.tex') @@ -781,7 +782,12 @@ describe('autoSelectFile', function () { ] const pathname = fileFinalPathname( - autoSelectFile(files, updates[0].toV, comparing, updates) + autoSelectFile( + files, + updates[0].toV, + comparing, + getUpdateForVersion(updates[0].toV, updates) + ) ) expect(pathname).to.equal('new.bib')