From 99e1ff08048e38dd633f99f072e060f5dc0ab858 Mon Sep 17 00:00:00 2001 From: Tim Down <158919+timdown@users.noreply.github.com> Date: Tue, 18 Apr 2023 16:15:01 +0200 Subject: [PATCH] Merge pull request #12644 from overleaf/td-history-compare Initial implementation of comparing history versions GitOrigin-RevId: 890e270d6e41856a79689ab41ccfbde25c4703ba --- .../components/change-list/change-list.tsx | 2 +- .../change-list/history-version.tsx | 47 +++++++++++++++++- .../history/components/diff-view/main.tsx | 12 +++-- .../history/components/history-file-tree.tsx | 8 +-- .../history/context/history-context.tsx | 49 ++++++++++--------- .../hooks/use-file-tree-item-selection.tsx | 20 +++----- .../context/types/history-context-value.ts | 10 ++-- .../history/extensions/highlight-locations.ts | 5 -- .../js/features/history/services/api.ts | 7 +-- .../features/history/services/types/file.ts | 5 -- .../history/services/types/selection.ts | 9 ++++ .../features/history/services/types/update.ts | 10 ++-- .../history/utils/auto-select-file.ts | 24 ++++----- .../features/history/utils/history-details.ts | 11 ++++- .../stylesheets/app/editor/history-react.less | 11 +++++ .../history/utils/auto-select-file.test.ts | 36 ++------------ 16 files changed, 147 insertions(+), 119 deletions(-) create mode 100644 services/web/frontend/js/features/history/services/types/selection.ts 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 4aad40c733..f585b07669 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 @@ -19,7 +19,7 @@ function ChangeList() { )} {!error && ( -
+
{labelsOnly ? : }
)} diff --git a/services/web/frontend/js/features/history/components/change-list/history-version.tsx b/services/web/frontend/js/features/history/components/change-list/history-version.tsx index 0cbf457e50..6eda2b7f17 100644 --- a/services/web/frontend/js/features/history/components/change-list/history-version.tsx +++ b/services/web/frontend/js/features/history/components/change-list/history-version.tsx @@ -6,6 +6,9 @@ import { useUserContext } from '../../../../shared/context/user-context' import { relativeDate, formatTime } from '../../../utils/format-date' import { orderBy } from 'lodash' import { LoadedUpdate } from '../../services/types/update' +import { useHistoryContext } from '../../context/history-context' +import classNames from 'classnames' +import { updateIsSelected } from '../../utils/history-details' type HistoryEntryProps = { update: LoadedUpdate @@ -15,16 +18,46 @@ function HistoryVersion({ update }: HistoryEntryProps) { const { id: currentUserId } = useUserContext() const orderedLabels = orderBy(update.labels, ['created_at'], ['desc']) + const { selection, setSelection } = useHistoryContext() + + const selected = updateIsSelected(update, selection) + + function compare() { + const { updateRange } = selection + if (!updateRange) { + return + } + const fromV = Math.min(update.fromV, updateRange.fromV) + const toV = Math.max(update.toV, updateRange.toV) + + setSelection({ + updateRange: { fromV, toV }, + comparing: true, + files: [], + pathname: null, + }) + } + return ( -
+
{update.meta.first_in_day && ( )} + {/* TODO: Sort out accessibility for this */} + {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions */}
+ setSelection({ + updateRange: update, + comparing: false, + files: [], + pathname: null, + }) + } >
) diff --git a/services/web/frontend/js/features/history/components/diff-view/main.tsx b/services/web/frontend/js/features/history/components/diff-view/main.tsx index a0c8599d78..21a80ee304 100644 --- a/services/web/frontend/js/features/history/components/diff-view/main.tsx +++ b/services/web/frontend/js/features/history/components/diff-view/main.tsx @@ -17,7 +17,7 @@ type Diff = { function Main() { const { t } = useTranslation() - const { projectId, updateSelection, fileSelection } = useHistoryContext() + const { projectId, selection } = useHistoryContext() const { isLoading, runAsync, data } = useAsync() let diff: Diff | undefined if (data?.diff) { @@ -28,16 +28,18 @@ function Main() { } } + const { updateRange, pathname } = selection + useEffect(() => { - if (!updateSelection || !fileSelection || !fileSelection.pathname) { + if (!updateRange || !pathname) { return } - const { fromV, toV } = updateSelection.update + const { fromV, toV } = updateRange // TODO: Error handling - runAsync(diffDoc(projectId, fromV, toV, fileSelection.pathname)) - }, [fileSelection, projectId, runAsync, updateSelection]) + runAsync(diffDoc(projectId, fromV, toV, pathname)) + }, [projectId, runAsync, pathname, updateRange]) if (isLoading) { return ( 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 623926472f..d8aaead216 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 @@ -7,13 +7,9 @@ import { import HistoryFileTreeFolderList from './file-tree/history-file-tree-folder-list' export default function HistoryFileTree() { - const { fileSelection } = useHistoryContext() + const { files } = useHistoryContext().selection - if (!fileSelection) { - return null - } - - const fileTree = _.reduce(fileSelection.files, reducePathsToTree, []) + const fileTree = _.reduce(files, reducePathsToTree, []) const mappedFileTree = fileTreeDiffToFileTreeData(fileTree) 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 0a6c23e554..84cf02755d 100644 --- a/services/web/frontend/js/features/history/context/history-context.tsx +++ b/services/web/frontend/js/features/history/context/history-context.tsx @@ -19,9 +19,9 @@ import ColorManager from '../../../ide/colors/ColorManager' import moment from 'moment' import * as eventTracking from '../../../infrastructure/event-tracking' import { cloneDeep } from 'lodash' -import { LoadedUpdate, Update, UpdateSelection } from '../services/types/update' -import { FileSelection } from '../services/types/file' +import { LoadedUpdate, Update } from '../services/types/update' import { Nullable } from '../../../../../types/utils' +import { Selection } from '../services/types/selection' function useHistory() { const { view } = useLayoutContext() @@ -30,9 +30,14 @@ function useHistory() { const userId = user.id const projectId = project._id const projectOwnerId = project.owner?._id - const [updateSelection, setUpdateSelection] = - useState(null) - const [fileSelection, setFileSelection] = useState(null) + + const [selection, setSelection] = useState({ + updateRange: null, + comparing: false, + files: [], + pathname: null, + }) + const [updates, setUpdates] = useState([]) const [loadingFileTree, setLoadingFileTree] = useState(true) @@ -158,15 +163,17 @@ function useHistory() { } }, [view, fetchNextBatchOfUpdates]) + const { updateRange, comparing } = selection + // Load files when the update selection changes useEffect(() => { - if (!updateSelection) { + if (!updateRange) { return } - const { fromV, toV } = updateSelection.update + const { fromV, toV } = updateRange diffFiles(projectId, fromV, toV).then(({ diff: files }) => { - const pathname = autoSelectFile(files, updateSelection, updates) + const pathname = autoSelectFile(files, updateRange, comparing, updates) const newFiles = files.map(file => { if (isFileRenamed(file) && file.newPathname) { return renamePathnameKey(file) @@ -174,19 +181,21 @@ function useHistory() { return file }) - setFileSelection({ files: newFiles, pathname }) + setSelection({ updateRange, comparing, files: newFiles, pathname }) }) - }, [updateSelection, projectId, updates]) + }, [updateRange, projectId, updates, comparing]) useEffect(() => { // Set update selection if there isn't one - if (updates.length && !updateSelection) { - setUpdateSelection({ - update: updates[0], + if (updates.length && !updateRange) { + setSelection({ + updateRange: updates[0], comparing: false, + files: [], + pathname: null, }) } - }, [setUpdateSelection, updateSelection, updates]) + }, [updateRange, updates]) const value = useMemo( () => ({ @@ -202,10 +211,8 @@ function useHistory() { setUpdates, userHasFullFeature, projectId, - fileSelection, - setFileSelection, - updateSelection, - setUpdateSelection, + selection, + setSelection, }), [ atEnd, @@ -220,10 +227,8 @@ function useHistory() { setUpdates, userHasFullFeature, projectId, - fileSelection, - setFileSelection, - updateSelection, - setUpdateSelection, + selection, + setSelection, ] ) diff --git a/services/web/frontend/js/features/history/context/hooks/use-file-tree-item-selection.tsx b/services/web/frontend/js/features/history/context/hooks/use-file-tree-item-selection.tsx index 43eeb1c119..804cfa492a 100644 --- a/services/web/frontend/js/features/history/context/hooks/use-file-tree-item-selection.tsx +++ b/services/web/frontend/js/features/history/context/hooks/use-file-tree-item-selection.tsx @@ -1,25 +1,19 @@ -import { useCallback, useMemo } from 'react' +import { useCallback } from 'react' import { useHistoryContext } from '../history-context' export function useFileTreeItemSelection(pathname: string) { - const { fileSelection, setFileSelection } = useHistoryContext() + const { selection, setSelection } = useHistoryContext() const handleClick = useCallback(() => { - if (!fileSelection) { - return - } - if (pathname !== fileSelection.pathname) { - setFileSelection({ - files: fileSelection.files, + if (pathname !== selection.pathname) { + setSelection({ + ...selection, pathname, }) } - }, [fileSelection, pathname, setFileSelection]) + }, [pathname, selection, setSelection]) - const isSelected = useMemo( - () => fileSelection?.pathname === pathname, - [fileSelection, pathname] - ) + const isSelected = selection.pathname === pathname return { isSelected, onClick: handleClick } } 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 9917d85d8b..1ff6bedb78 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 @@ -1,7 +1,7 @@ import { Nullable } from '../../../../../../types/utils' -import { LoadedUpdate, UpdateSelection } from '../../services/types/update' +import { LoadedUpdate } from '../../services/types/update' import { LoadedLabel } from '../../services/types/label' -import { FileSelection } from '../../services/types/file' +import { Selection } from '../../services/types/selection' export type HistoryContextValue = { updates: LoadedUpdate[] @@ -18,8 +18,6 @@ export type HistoryContextValue = { setLabels: React.Dispatch> loadingFileTree: boolean projectId: string - fileSelection: FileSelection | null - setFileSelection: (fileSelection: FileSelection) => void - updateSelection: UpdateSelection | null - setUpdateSelection: (updateSelection: UpdateSelection) => void + selection: Selection + setSelection: (selection: Selection) => void } diff --git a/services/web/frontend/js/features/history/extensions/highlight-locations.ts b/services/web/frontend/js/features/history/extensions/highlight-locations.ts index 4b12127c4d..0a5d4fa732 100644 --- a/services/web/frontend/js/features/history/extensions/highlight-locations.ts +++ b/services/web/frontend/js/features/history/extensions/highlight-locations.ts @@ -73,11 +73,6 @@ const plugin = ViewPlugin.fromClass( const oldLocations = this.view.state.field(highlightLocationsField) const newLocations = calculateHighlightLocations(this.view) - console.log( - 'dispatchIfChanged, changed is', - !isEqual(oldLocations, newLocations) - ) - if (!isEqual(oldLocations, newLocations)) { this.view.dispatch({ effects: setHighlightLocationsEffect.of(newLocations), diff --git a/services/web/frontend/js/features/history/services/api.ts b/services/web/frontend/js/features/history/services/api.ts index 3f91577195..3e55ad602f 100644 --- a/services/web/frontend/js/features/history/services/api.ts +++ b/services/web/frontend/js/features/history/services/api.ts @@ -17,9 +17,10 @@ export function fetchUpdates(projectId: string, before?: number) { const queryParamsSerialized = new URLSearchParams(queryParams).toString() const updatesURL = `/project/${projectId}/updates?${queryParamsSerialized}` - return getJSON<{ updates: Update[]; nextBeforeTimestamp?: number }>( - updatesURL - ) + return getJSON<{ + updates: Update[] + nextBeforeTimestamp?: number + }>(updatesURL) } export function fetchLabels(projectId: string) { diff --git a/services/web/frontend/js/features/history/services/types/file.ts b/services/web/frontend/js/features/history/services/types/file.ts index c507bbb278..a0b2ebb8c8 100644 --- a/services/web/frontend/js/features/history/services/types/file.ts +++ b/services/web/frontend/js/features/history/services/types/file.ts @@ -30,8 +30,3 @@ export type FileDiff = | FileEdited | FileRenamed | FileUnchanged - -export interface FileSelection { - files: FileDiff[] - pathname: string | null -} diff --git a/services/web/frontend/js/features/history/services/types/selection.ts b/services/web/frontend/js/features/history/services/types/selection.ts new file mode 100644 index 0000000000..1f8f722121 --- /dev/null +++ b/services/web/frontend/js/features/history/services/types/selection.ts @@ -0,0 +1,9 @@ +import { FileDiff } from './file' +import { UpdateRange } from './update' + +export interface Selection { + updateRange: UpdateRange | null + comparing: boolean + files: FileDiff[] + pathname: string | null +} diff --git a/services/web/frontend/js/features/history/services/types/update.ts b/services/web/frontend/js/features/history/services/types/update.ts index 0df18620df..2284d69f8b 100644 --- a/services/web/frontend/js/features/history/services/types/update.ts +++ b/services/web/frontend/js/features/history/services/types/update.ts @@ -9,9 +9,12 @@ export interface ProjectOp { atV: number } -export interface Update { +export interface UpdateRange { fromV: number toV: number +} + +export interface Update extends UpdateRange { meta: Meta labels: Label[] pathnames: string[] @@ -32,8 +35,3 @@ interface LoadedUpdateMeta extends Meta { export interface LoadedUpdate extends Update { meta: LoadedUpdateMeta } - -export interface UpdateSelection { - update: Update - comparing: boolean -} 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 3537c4c5c6..750e7ecc67 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 @@ -3,12 +3,12 @@ import type { Nullable } from '../../../../../types/utils' import type { HistoryContextValue } from '../context/types/history-context-value' import type { FileDiff } from '../services/types/file' import type { DiffOperation } from '../services/types/diff-operation' -import type { Update } from '../services/types/update' +import type { LoadedUpdate, UpdateRange } from '../services/types/update' function getUpdateForVersion( - version: Update['toV'], + version: LoadedUpdate['toV'], updates: HistoryContextValue['updates'] -): Nullable { +): Nullable { return updates.filter(update => update.toV === version)?.[0] ?? null } @@ -19,18 +19,13 @@ type FileWithOps = { function getFilesWithOps( files: FileDiff[], - updateSelection: HistoryContextValue['updateSelection'], + updateRange: UpdateRange, + comparing: boolean, updates: HistoryContextValue['updates'] ): FileWithOps[] { - if (!updateSelection) { - return [] - } - if (updateSelection.update.toV && !updateSelection.comparing) { + if (updateRange.toV && !comparing) { const filesWithOps: FileWithOps[] = [] - const currentUpdate = getUpdateForVersion( - updateSelection.update.toV, - updates - ) + const currentUpdate = getUpdateForVersion(updateRange.toV, updates) if (currentUpdate !== null) { for (const pathname of currentUpdate.pathnames) { @@ -95,12 +90,13 @@ const orderedOpTypes: DiffOperation[] = [ export function autoSelectFile( files: FileDiff[], - updateSelection: HistoryContextValue['updateSelection'], + updateRange: UpdateRange, + comparing: boolean, updates: HistoryContextValue['updates'] ) { let fileToSelect: Nullable = null - const filesWithOps = getFilesWithOps(files, updateSelection, updates) + const filesWithOps = getFilesWithOps(files, updateRange, comparing, updates) for (const opType of orderedOpTypes) { const fileWithMatchingOpType = _.find(filesWithOps, { operation: opType, diff --git a/services/web/frontend/js/features/history/utils/history-details.ts b/services/web/frontend/js/features/history/utils/history-details.ts index 3ae78aa5c4..09f1c1fd25 100644 --- a/services/web/frontend/js/features/history/utils/history-details.ts +++ b/services/web/frontend/js/features/history/utils/history-details.ts @@ -1,7 +1,8 @@ import ColorManager from '../../../ide/colors/ColorManager' import { Nullable } from '../../../../../types/utils' import { User } from '../services/types/shared' -import { ProjectOp } from '../services/types/update' +import { ProjectOp, UpdateRange } from '../services/types/update' +import { Selection } from '../services/types/selection' export const getUserColor = (user?: Nullable<{ id: string }>) => { const hue = ColorManager.getHueForUserId(user?.id) || 100 @@ -35,3 +36,11 @@ export const getProjectOpDoc = (projectOp: ProjectOp) => { } return '' } + +export const updateIsSelected = (update: UpdateRange, selection: Selection) => { + return ( + selection.updateRange && + update.fromV >= selection.updateRange.fromV && + update.toV <= selection.updateRange.toV + ) +} diff --git a/services/web/frontend/stylesheets/app/editor/history-react.less b/services/web/frontend/stylesheets/app/editor/history-react.less index cc4f463cb1..f582c560dc 100644 --- a/services/web/frontend/stylesheets/app/editor/history-react.less +++ b/services/web/frontend/stylesheets/app/editor/history-react.less @@ -39,6 +39,8 @@ history-root { } .change-list { + display: flex; + flex-direction: column; width: @versions-list-width; font-size: @font-size-small; border-left: 1px solid @history-react-separator-color; @@ -53,6 +55,11 @@ history-root { } } + .history-version-list-container { + flex: 1; + overflow-y: auto; + } + .history-toggle-switch-container, .history-version-day, .history-version-details { @@ -79,6 +86,10 @@ history-root { } } + .history-version-selected { + background-color: @green-10; + } + .history-version-metadata-time { display: block; margin-bottom: 4px; 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 670a75fee3..4930c78d2f 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 @@ -3,7 +3,6 @@ import type { HistoryContextValue } from '../../../../../frontend/js/features/hi import type { FileDiff } from '../../../../../frontend/js/features/history/services/types/file' import { autoSelectFile } from '../../../../../frontend/js/features/history/utils/auto-select-file' import type { User } from '../../../../../frontend/js/features/history/services/types/shared' -import { UpdateSelection } from '../../../../../frontend/js/features/history/services/types/update' describe('autoSelectFile', function () { const historyUsers: User[] = [ @@ -260,12 +259,7 @@ describe('autoSelectFile', function () { }, ] - const updateSelection: UpdateSelection = { - update: updates[0], - comparing, - } - - const pathname = autoSelectFile(files, updateSelection, updates) + const pathname = autoSelectFile(files, updates[0], comparing, updates) expect(pathname).to.equal('newfolder1/newfile10.tex') }) @@ -330,12 +324,7 @@ describe('autoSelectFile', function () { }, ] - const updateSelection: UpdateSelection = { - update: updates[0], - comparing, - } - - const pathname = autoSelectFile(files, updateSelection, updates) + const pathname = autoSelectFile(files, updates[0], comparing, updates) expect(pathname).to.equal('newfile1.tex') }) @@ -431,12 +420,7 @@ describe('autoSelectFile', function () { }, ] - const updateSelection: UpdateSelection = { - update: updates[0], - comparing, - } - - const pathname = autoSelectFile(files, updateSelection, updates) + const pathname = autoSelectFile(files, updates[0], comparing, updates) expect(pathname).to.equal('main3.tex') }) @@ -602,12 +586,7 @@ describe('autoSelectFile', function () { }, ] - const updateSelection: UpdateSelection = { - update: updates[0], - comparing, - } - - const pathname = autoSelectFile(files, updateSelection, updates) + const pathname = autoSelectFile(files, updates[0], comparing, updates) expect(pathname).to.equal('main.tex') }) @@ -710,12 +689,7 @@ describe('autoSelectFile', function () { }, ] - const updateSelection: UpdateSelection = { - update: updates[0], - comparing, - } - - const pathname = autoSelectFile(files, updateSelection, updates) + const pathname = autoSelectFile(files, updates[0], comparing, updates) expect(pathname).to.equal('certainly_not_main.tex') })