Merge pull request #13039 from overleaf/td-history-auto-select-file-ignore-binary

History migration: Ignore binary files when auto-selecting file to display in history view

GitOrigin-RevId: 7d2a314cdb3d54e4e2292a95e7633e3829ea931f

services/project-history/app/js/FileTreeDiffGenerator.js

@@ -54,21 +54,20 @@ function _getInitialDiffSnapshot(chunk, fromVersion) {
   // with nothing in the diff marked as changed.
   // Use a bare object to protect against reserved names.
   const diff = Object.create(null)
-  const pathnames = _getInitialPathnames(chunk, fromVersion)
-  for (const pathname of Array.from(pathnames)) {
-    diff[pathname] = { pathname }
+  const files = _getInitialFiles(chunk, fromVersion)
+  for (const [pathname, file] of Object.entries(files)) {
+    diff[pathname] = { pathname, editable: file.isEditable() }
   }
   return diff
 }
 
-function _getInitialPathnames(chunk, fromVersion) {
+function _getInitialFiles(chunk, fromVersion) {
   const snapshot = chunk.getSnapshot()
   const changes = chunk
     .getChanges()
     .slice(0, fromVersion - chunk.getStartVersion())
   snapshot.applyAll(changes)
-  const pathnames = snapshot.getFilePathnames()
-  return pathnames
+  return snapshot.fileMap.files
 }
 
 function _applyAddFileToDiff(diff, operation) {
@@ -88,6 +87,7 @@ function _applyAddFileToDiff(diff, operation) {
     return (diff[operation.pathname] = {
       pathname: operation.pathname,
       operation: 'added',
+      editable: operation.file.isEditable(),
     })
   }
 }

services/project-history/test/acceptance/js/FileTreeDiffTests.js

@@ -123,6 +123,7 @@ describe('FileTree Diffs', function () {
                   {
                     file: {
                       hash: sha('new-sha'),
+                      stringLength: 42,
                     },
                     pathname: 'added.tex',
                   },
@@ -155,15 +156,18 @@ describe('FileTree Diffs', function () {
               pathname: 'deleted.tex',
               operation: 'removed',
               deletedAtV: 5,
+              editable: true,
             },
             {
               newPathname: 'newName.tex',
               pathname: 'renamed.tex',
               operation: 'renamed',
+              editable: true,
             },
             {
               pathname: 'added.tex',
               operation: 'added',
+              editable: true,
             },
           ],
         })
@@ -269,6 +273,7 @@ describe('FileTree Diffs', function () {
                   {
                     file: {
                       hash: sha('new-sha'),
+                      stringLength: 42,
                     },
                     pathname: 'added.tex',
                   },
@@ -313,20 +318,24 @@ describe('FileTree Diffs', function () {
             },
             {
               pathname: 'baz.tex',
+              editable: true,
             },
             {
               pathname: 'deleted.tex',
               operation: 'removed',
               deletedAtV: 4,
+              editable: true,
             },
             {
               newPathname: 'newName.tex',
               pathname: 'renamed.tex',
               operation: 'renamed',
+              editable: true,
             },
             {
               pathname: 'added.tex',
               operation: 'added',
+              editable: true,
             },
           ],
         })
@@ -391,6 +400,7 @@ describe('FileTree Diffs', function () {
               newPathname: 'three.tex',
               pathname: 'one.tex',
               operation: 'renamed',
+              editable: true,
             },
           ],
         })
@@ -455,6 +465,7 @@ describe('FileTree Diffs', function () {
           diff: [
             {
               pathname: 'one.tex',
+              editable: true,
             },
           ],
         })
@@ -523,6 +534,7 @@ describe('FileTree Diffs', function () {
               pathname: 'two.tex',
               newPathname: 'one.tex',
               operation: 'renamed',
+              editable: true,
             },
           ],
         })
@@ -547,6 +559,7 @@ describe('FileTree Diffs', function () {
                     pathname: 'one.tex',
                     file: {
                       hash: sha('mock-sha'),
+                      stringLength: 42,
                     },
                   },
                 ],
@@ -583,6 +596,7 @@ describe('FileTree Diffs', function () {
             {
               pathname: 'two.tex',
               operation: 'added',
+              editable: true,
             },
           ],
         })

services/web/frontend/js/features/history/services/types/file.ts

@@ -1,24 +1,30 @@
 import { FileOperation } from './file-operation'
 
-export interface FileUnchanged {
+interface File {
   pathname: string
 }
 
-export interface FileAdded extends FileUnchanged {
+export interface FileWithEditable extends File {
+  editable: boolean
+}
+
+export type FileUnchanged = FileWithEditable
+
+export interface FileAdded extends FileWithEditable {
   operation: Extract<FileOperation, 'added'>
 }
 
-export interface FileRemoved extends FileUnchanged {
+export interface FileRemoved extends FileWithEditable {
   operation: Extract<FileOperation, 'removed'>
   newPathname?: string
   deletedAtV: number
 }
 
-export interface FileEdited extends FileUnchanged {
+export interface FileEdited extends File {
   operation: Extract<FileOperation, 'edited'>
 }
 
-export interface FileRenamed extends FileUnchanged {
+export interface FileRenamed extends FileWithEditable {
   newPathname?: string
   oldPathname?: string
   operation: Extract<FileOperation, 'renamed'>

services/web/frontend/js/features/history/utils/auto-select-file.ts

@@ -1,12 +1,12 @@
-import _ from 'lodash'
 import type { Nullable } from '../../../../../types/utils'
 import type { FileDiff } from '../services/types/file'
 import type { FileOperation } from '../services/types/file-operation'
 import type { LoadedUpdate, Version } from '../services/types/update'
-import { fileFinalPathname } from './file-diff'
+import { fileFinalPathname, isFileEditable } from './file-diff'
 
 type FileWithOps = {
   pathname: FileDiff['pathname']
+  editable: boolean
   operation: FileOperation
 }
 
@@ -20,54 +20,70 @@ function getFilesWithOps(
     const filesWithOps: FileWithOps[] = []
 
     if (updateForToV) {
+      const filesByPathname = new Map<string, FileDiff>()
+      for (const file of files) {
+        const pathname = fileFinalPathname(file)
+        filesByPathname.set(pathname, file)
+      }
+
+      const isEditable = (pathname: string) => {
+        const fileDiff = filesByPathname.get(pathname)
+        if (!fileDiff) {
+          return false
+        }
+        return isFileEditable(fileDiff)
+      }
+
       for (const pathname of updateForToV.pathnames) {
         filesWithOps.push({
           pathname,
+          editable: isEditable(pathname),
           operation: 'edited',
         })
       }
 
       for (const op of updateForToV.project_ops) {
-        let fileWithOps: Nullable<FileWithOps> = null
+        let pathAndOp: Nullable<Pick<FileWithOps, 'pathname' | 'operation'>> =
+          null
 
         if (op.add) {
-          fileWithOps = {
+          pathAndOp = {
             pathname: op.add.pathname,
             operation: 'added',
           }
         } else if (op.remove) {
-          fileWithOps = {
+          pathAndOp = {
             pathname: op.remove.pathname,
             operation: 'removed',
           }
         } else if (op.rename) {
-          fileWithOps = {
+          pathAndOp = {
             pathname: op.rename.newPathname,
             operation: 'renamed',
           }
         }
 
-        if (fileWithOps !== null) {
-          filesWithOps.push(fileWithOps)
+        if (pathAndOp !== null) {
+          filesWithOps.push({
+            editable: isEditable(pathAndOp.pathname),
+            ...pathAndOp,
+          })
         }
       }
     }
 
     return filesWithOps
   } else {
-    const filesWithOps = _.reduce(
-      files,
-      (curFilesWithOps, file) => {
-        if ('operation' in file) {
-          curFilesWithOps.push({
-            pathname: file.pathname,
-            operation: file.operation,
-          })
-        }
-        return curFilesWithOps
-      },
-      <FileWithOps[]>[]
-    )
+    const filesWithOps = files.reduce((curFilesWithOps, file) => {
+      if ('operation' in file) {
+        curFilesWithOps.push({
+          pathname: file.pathname,
+          editable: isFileEditable(file),
+          operation: file.operation,
+        })
+      }
+      return curFilesWithOps
+    }, <FileWithOps[]>[])
 
     return filesWithOps
   }
@@ -86,44 +102,25 @@ export function autoSelectFile(
   comparing: boolean,
   updateForToV: LoadedUpdate | undefined
 ): FileDiff {
-  let fileToSelect: Nullable<FileDiff> = null
-
   const filesWithOps = getFilesWithOps(files, toV, comparing, updateForToV)
   for (const opType of orderedOpTypes) {
-    const fileWithMatchingOpType = _.find(filesWithOps, {
-      operation: opType,
-    })
-
-    if (fileWithMatchingOpType != null) {
-      fileToSelect =
-        _.find(
-          files,
-          file => fileFinalPathname(file) === fileWithMatchingOpType.pathname
-        ) ?? null
-
-      break
-    }
-  }
-
-  if (!fileToSelect) {
-    const mainFile = _.find(files, function (file) {
-      return /main\.tex$/.test(file.pathname)
-    })
-
-    if (mainFile) {
-      fileToSelect = mainFile
-    } else {
-      const anyTeXFile = _.find(files, function (file) {
-        return /\.tex$/.test(file.pathname)
-      })
+    const fileWithMatchingOpType = filesWithOps.find(
+      file => file.operation === opType && file.editable
+    )
 
-      if (anyTeXFile) {
-        fileToSelect = anyTeXFile
-      } else {
-        fileToSelect = files[0]
+    if (fileWithMatchingOpType) {
+      const fileToSelect = files.find(
+        file => fileFinalPathname(file) === fileWithMatchingOpType.pathname
+      )
+      if (fileToSelect) {
+        return fileToSelect
       }
     }
   }
 
-  return fileToSelect
+  return (
+    files.find(file => /main\.tex$/.test(file.pathname)) ||
+    files.find(file => /\.tex$/.test(file.pathname)) ||
+    files[0]
+  )
 }

services/web/frontend/js/features/history/utils/file-diff.ts

@@ -1,4 +1,9 @@
-import type { FileDiff, FileRemoved, FileRenamed } from '../services/types/file'
+import type {
+  FileDiff,
+  FileRemoved,
+  FileRenamed,
+  FileWithEditable,
+} from '../services/types/file'
 
 export function isFileRenamed(fileDiff: FileDiff): fileDiff is FileRenamed {
   return (fileDiff as FileRenamed).operation === 'renamed'
@@ -8,6 +13,18 @@ export function isFileRemoved(fileDiff: FileDiff): fileDiff is FileRemoved {
   return (fileDiff as FileRemoved).operation === 'removed'
 }
 
+function isFileWithEditable(fileDiff: FileDiff): fileDiff is FileWithEditable {
+  return 'editable' in (fileDiff as FileWithEditable)
+}
+
+export function isFileEditable(fileDiff: FileDiff) {
+  return isFileWithEditable(fileDiff)
+    ? fileDiff.editable
+    : fileDiff.operation === 'edited'
+}
+
 export function fileFinalPathname(fileDiff: FileDiff) {
-  return isFileRenamed(fileDiff) ? fileDiff.newPathname : fileDiff.pathname
+  return (
+    (isFileRenamed(fileDiff) ? fileDiff.newPathname : null) || fileDiff.pathname
+  )
 }

services/web/frontend/js/features/history/utils/file-tree.ts

@@ -1,6 +1,6 @@
 import _ from 'lodash'
 import type { FileDiff, FileRenamed } from '../services/types/file'
-import { isFileRemoved } from './file-diff'
+import { isFileEditable, isFileRemoved } from './file-diff'
 
 export type FileTreeEntity = {
   name?: string
@@ -37,6 +37,7 @@ export function reducePathsToTree(
           type: 'folder',
           children: <FileTreeEntity[]>[],
           pathname: pathPart,
+          editable: false,
         }
 
         currentFileTreeLocation.push(fileTreeEntity)
@@ -68,17 +69,12 @@ export function fileTreeDiffToFileTreeData(
     if (file.type === 'file') {
       const deletedAtV = isFileRemoved(file) ? file.deletedAtV : undefined
 
-      let newDoc: HistoryDoc = {
+      const newDoc: HistoryDoc = {
         pathname: file.pathname ?? '',
         name: file.name ?? '',
         deletedAtV,
-      }
-
-      if ('operation' in file) {
-        newDoc = {
-          ...newDoc,
-          operation: file.operation,
-        }
+        editable: isFileEditable(file),
+        operation: 'operation' in file ? file.operation : undefined,
       }
 
       docs.push(newDoc)
@@ -115,5 +111,6 @@ export function renamePathnameKey(file: FileRenamed): FileRenamed {
     oldPathname: file.pathname,
     pathname: file.newPathname as string,
     operation: file.operation,
+    editable: file.editable,
   }
 }

services/web/test/frontend/features/history/components/toolbar.spec.tsx

@@ -43,13 +43,16 @@ describe('history toolbar', function () {
         },
         {
           pathname: 'sample.bib',
+          editable: true,
         },
         {
           pathname: 'frog.jpg',
+          editable: false,
         },
       ],
       selectedFile: {
         pathname: 'main.tex',
+        editable: true,
       },
     }
 
@@ -83,18 +86,22 @@ describe('history toolbar', function () {
         {
           pathname: 'main.tex',
           operation: 'added',
+          editable: true,
         },
         {
           pathname: 'sample.bib',
           operation: 'added',
+          editable: true,
         },
         {
           pathname: 'frog.jpg',
           operation: 'added',
+          editable: false,
         },
       ],
       selectedFile: {
         pathname: 'main.tex',
+        editable: true,
       },
     }
 

services/web/test/frontend/features/history/utils/auto-select-file.test.ts

@@ -23,18 +23,23 @@ describe('autoSelectFile', function () {
       const files: FileDiff[] = [
         {
           pathname: 'main.tex',
+          editable: true,
         },
         {
           pathname: 'sample.bib',
+          editable: true,
         },
         {
           pathname: 'frog.jpg',
+          editable: false,
         },
         {
           pathname: 'newfile5.tex',
+          editable: true,
         },
         {
           pathname: 'newfolder1/newfolder2/newfile2.tex',
+          editable: true,
         },
         {
           pathname: 'newfolder1/newfile10.tex',
@@ -276,18 +281,22 @@ describe('autoSelectFile', function () {
         {
           pathname: 'main.tex',
           operation: 'added',
+          editable: true,
         },
         {
           pathname: 'sample.bib',
           operation: 'added',
+          editable: true,
         },
         {
           pathname: 'frog.jpg',
           operation: 'added',
+          editable: false,
         },
         {
           pathname: 'newfile1.tex',
           operation: 'added',
+          editable: true,
         },
       ]
 
@@ -347,17 +356,21 @@ describe('autoSelectFile', function () {
           pathname: 'main.tex',
           operation: 'removed',
           deletedAtV: 6,
+          editable: true,
         },
         {
           pathname: 'sample.bib',
+          editable: true,
         },
         {
           pathname: 'main2.tex',
           operation: 'added',
+          editable: true,
         },
         {
           pathname: 'main3.tex',
           operation: 'added',
+          editable: true,
         },
       ]
 
@@ -446,18 +459,22 @@ describe('autoSelectFile', function () {
       const files: FileDiff[] = [
         {
           pathname: 'main.tex',
+          editable: true,
         },
         {
           pathname: 'sample.bib',
+          editable: true,
         },
         {
           pathname: 'frog.jpg',
+          editable: false,
         },
         {
           pathname: 'newfolder/maybewillbedeleted.tex',
           newPathname: 'newfolder2/maybewillbedeleted.tex',
           operation: 'removed',
           deletedAtV: 10,
+          editable: true,
         },
       ]
 
@@ -617,12 +634,15 @@ describe('autoSelectFile', function () {
       const files: FileDiff[] = [
         {
           pathname: 'certainly_not_main.tex',
+          editable: true,
         },
         {
           pathname: 'newfile.tex',
+          editable: true,
         },
         {
           pathname: 'file2.tex',
+          editable: true,
         },
       ]
 
@@ -725,11 +745,13 @@ describe('autoSelectFile', function () {
       const files: FileDiff[] = [
         {
           pathname: 'main.tex',
+          editable: true,
         },
         {
           pathname: 'original.bib',
           newPathname: 'new.bib',
           operation: 'renamed',
+          editable: true,
         },
       ]
 
@@ -792,5 +814,79 @@ describe('autoSelectFile', function () {
 
       expect(pathname).to.equal('new.bib')
     })
+
+    it('ignores binary file', function () {
+      const files: FileDiff[] = [
+        {
+          pathname: 'frog.jpg',
+          editable: false,
+          operation: 'added',
+        },
+        {
+          pathname: 'main.tex',
+          editable: true,
+        },
+        {
+          pathname: 'sample.bib',
+          editable: true,
+        },
+      ]
+
+      const updates: LoadedUpdate[] = [
+        {
+          fromV: 4,
+          toV: 7,
+          meta: {
+            users: historyUsers,
+            start_ts: 1680874742389,
+            end_ts: 1680874755552,
+          },
+          labels: [],
+          pathnames: [],
+          project_ops: [
+            {
+              add: {
+                pathname: 'frog.jpg',
+              },
+              atV: 5,
+            },
+          ],
+        },
+        {
+          fromV: 0,
+          toV: 4,
+          meta: {
+            users: historyUsers,
+            start_ts: 1680861975947,
+            end_ts: 1680861988442,
+          },
+          labels: [],
+          pathnames: [],
+          project_ops: [
+            {
+              add: {
+                pathname: 'sample.bib',
+              },
+              atV: 1,
+            },
+            {
+              add: {
+                pathname: 'main.tex',
+              },
+              atV: 0,
+            },
+          ],
+        },
+      ]
+
+      const { pathname } = autoSelectFile(
+        files,
+        updates[0].toV,
+        comparing,
+        getUpdateForVersion(updates[0].toV, updates)
+      )
+
+      expect(pathname).to.equal('main.tex')
+    })
   })
 })