History migration: Abort previous GET requests (#12893)

GitOrigin-RevId: ad72d526bf7cdfa91b00025b58205d9a5b4cc62f

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 (
     <aside className="change-list">
       <div className="history-header history-toggle-switch-container">
-        {!error && (
-          <ToggleSwitch labelsOnly={labelsOnly} setLabelsOnly={setLabelsOnly} />
-        )}
+        <ToggleSwitch labelsOnly={labelsOnly} setLabelsOnly={setLabelsOnly} />
+      </div>
+      <div
+        className="history-version-list-container"
+        data-history-version-list-container
+      >
+        {labelsOnly ? <LabelsList /> : <AllHistoryList />}
       </div>
-      {!error && (
-        <div
-          className="history-version-list-container"
-          data-history-version-list-container
-        >
-          {labelsOnly ? <LabelsList /> : <AllHistoryList />}
-        </div>
-      )}
     </aside>
   )
 }

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<DocDiffResponse>()
+  const { isLoading, data, runAsync } = useAsync<DocDiffResponse>()
   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 (
     <div className="doc-panel">
-      {error ? (
-        <ErrorMessage />
-      ) : (
-        <>
-          <div className="history-header toolbar-container">
-            <Toolbar diff={diff} selection={selection} />
-          </div>
-          <div className="doc-container">
-            <Main diff={diff} isLoading={isLoading || loadingFileDiffs} />
-          </div>
-        </>
-      )}
+      <div className="history-header toolbar-container">
+        <Toolbar diff={diff} selection={selection} />
+      </div>
+      <div className="doc-container">
+        <Main diff={diff} isLoading={isLoading || loadingFileDiffs} />
+      </div>
     </div>
   )
 }

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

services/web/frontend/js/features/history/components/error-message.tsx

@@ -1,13 +0,0 @@
-import { useTranslation } from 'react-i18next'
-
-export default function ErrorMessage() {
-  const { t } = useTranslation()
-
-  return (
-    <div className="history-error">
-      <div className="text-danger error">
-        {t('generic_something_went_wrong')}
-      </div>
-    </div>
-  )
-}

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 (
     <HistoryFileTreeFolderList
       folders={mappedFileTree.folders}

services/web/frontend/js/features/history/components/history-root.tsx

@@ -4,18 +4,17 @@ import { HistoryProvider, useHistoryContext } from '../context/history-context'
 import { createPortal } from 'react-dom'
 import HistoryFileTree from './history-file-tree'
 import LoadingSpinner from '../../../shared/components/loading-spinner'
-import ErrorMessage from './error-message'
+import { ErrorBoundaryFallback } from '../../../shared/components/error-boundary-fallback'
+import withErrorBoundary from '../../../infrastructure/error-boundary'
 
 const fileTreeContainer = document.getElementById('history-file-tree')
 
 function Main() {
-  const { updatesInfo, error } = useHistoryContext()
+  const { updatesInfo } = useHistoryContext()
 
   let content
   if (updatesInfo.loadingState === 'loadingInitial') {
     content = <LoadingSpinner />
-  } else if (error) {
-    content = <ErrorMessage />
   } else {
     content = (
       <>
@@ -35,10 +34,12 @@ function Main() {
   )
 }
 
-export default function HistoryRoot() {
+function HistoryRoot() {
   return (
     <HistoryProvider>
       <Main />
     </HistoryProvider>
   )
 }
+
+export default withErrorBoundary(HistoryRoot, ErrorBoundaryFallback)

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<HistoryContextValue['error']>(null)
+
+  const updatesAbortControllerRef = useRef<AbortController | null>(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])
 
-  const resetSelection = useCallback(() => {
-    setSelection(selectionInitialState)
+  // Abort in-flight updates request on unmount
+  useEffect(() => {
+    return () => {
+      if (updatesAbortControllerRef.current) {
+        updatesAbortControllerRef.current.abort()
+      }
+    }
   }, [])
 
-  // Initial load when the History tab is active
+  // Initial load on first render
   const initialFetch = useRef(false)
   useEffect(() => {
     if (view === 'history' && !initialFetch.current) {
-      fetchNextBatchOfUpdates()
       initialFetch.current = true
+      return fetchNextBatchOfUpdates()
     }
   }, [view, fetchNextBatchOfUpdates])
 
-  const { updateRange, comparing, files } = selection
+  const resetSelection = useCallback(() => {
+    setSelection(selectionInitialState)
+  }, [])
+
+  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)
-          }
-
-          return file
-        })
-        setSelection({
-          updateRange,
-          comparing,
-          files: newFiles,
-          selectedFile,
+        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
+          })
+          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<HistoryContextValue>(
     () => ({
-      error,
-      setError,
       loadingFileDiffs,
-      setLoadingFileDiffs,
       updatesInfo,
       setUpdatesInfo,
       labels,
@@ -290,10 +308,7 @@ function useHistory() {
       resetSelection,
     }),
     [
-      error,
-      setError,
       loadingFileDiffs,
-      setLoadingFileDiffs,
       updatesInfo,
       setUpdatesInfo,
       labels,

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)
       )
     }
   }

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<unknown>
-  setError: React.Dispatch<React.SetStateAction<HistoryContextValue['error']>>
   labels: Nullable<LoadedLabel[]>
   setLabels: React.Dispatch<React.SetStateAction<HistoryContextValue['labels']>>
   labelsOnly: boolean
@@ -31,6 +29,6 @@ export type HistoryContextValue = {
   setSelection: React.Dispatch<
     React.SetStateAction<HistoryContextValue['selection']>
   >
-  fetchNextBatchOfUpdates: () => void
+  fetchNextBatchOfUpdates: () => (() => void) | void
   resetSelection: () => void
 }

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<string, string> = {
     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<FetchUpdatesResponse>(updatesURL)
+  return getJSON<FetchUpdatesResponse>(updatesURL, { signal })
 }
 
-export function fetchLabels(projectId: string) {
+export function fetchLabels(projectId: string, signal?: AbortSignal) {
   const labelsURL = `/project/${projectId}/labels`
-  return getJSON<Label[]>(labelsURL)
+  return getJSON<Label[]>(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<string, string> = {
     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<string, string> = {
     from: fromV.toString(),
@@ -69,7 +79,7 @@ export function diffDoc(
   }
   const queryParamsSerialized = new URLSearchParams(queryParams).toString()
   const diffUrl = `/project/${projectId}/diff?${queryParamsSerialized}`
-  return getJSON<DocDiffResponse>(diffUrl)
+  return getJSON<DocDiffResponse>(diffUrl, { signal })
 }
 
 export function restoreFile(projectId: string, selectedFile: FileRemoved) {

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<FileWithOps> = 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<FileDiff> = 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,

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')