From 5efe0d1452ffbba57c6835b9cd44c08fd209e91b Mon Sep 17 00:00:00 2001 From: ilkin-overleaf <100852799+ilkin-overleaf@users.noreply.github.com> Date: Wed, 29 Nov 2023 16:05:59 +0200 Subject: [PATCH] Merge pull request #15999 from overleaf/ii-ide-page-prototype-review-panel-accept-reject-changes [web] React ide page accept/reject changes GitOrigin-RevId: 0bb8e3759c7edbef16be04b2f200ae3686c3a53c --- .../hooks/use-review-panel-state.ts | 257 +++++++++++++++--- .../review-panel-context-adapter.ts | 5 +- .../review-panel/types/review-panel-state.ts | 5 +- .../extensions/changes/change-manager.ts | 2 +- services/web/types/review-panel/entry.ts | 1 + services/web/types/utils.ts | 4 + 6 files changed, 228 insertions(+), 46 deletions(-) diff --git a/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts b/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts index 5ecc780d11..230c06f4f6 100644 --- a/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts +++ b/services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts @@ -6,7 +6,10 @@ import useSocketListener from '@/features/ide-react/hooks/use-socket-listener' import useAsync from '@/shared/hooks/use-async' import useAbortController from '@/shared/hooks/use-abort-controller' import { sendMB } from '../../../../../infrastructure/event-tracking' -import { dispatchReviewPanelLayout as handleLayoutChange } from '@/features/source-editor/extensions/changes/change-manager' +import { + dispatchReviewPanelLayout as handleLayoutChange, + UpdateType, +} from '@/features/source-editor/extensions/changes/change-manager' import { useProjectContext } from '@/shared/context/project-context' import { useLayoutContext } from '@/shared/context/layout-context' import { useUserContext } from '@/shared/context/user-context' @@ -33,12 +36,15 @@ import { PublicAccessLevel } from '../../../../../../../types/public-access-leve import { ReviewPanelStateReactIde } from '../types/review-panel-state' import { DeepReadonly, + Entries, MergeAndOverride, } from '../../../../../../../types/utils' import { ReviewPanelCommentThread } from '../../../../../../../types/review-panel/comment-thread' import { DocId } from '../../../../../../../types/project-settings' import { + ReviewPanelAddCommentEntry, ReviewPanelAggregateChangeEntry, + ReviewPanelBulkActionsEntry, ReviewPanelChangeEntry, ReviewPanelCommentEntry, ReviewPanelEntry, @@ -130,9 +136,13 @@ function useReviewPanelState(): ReviewPanelStateReactIde { const [loading] = useScopeValue>( 'reviewPanel.overview.loading' ) - const [nVisibleSelectedChanges] = useScopeValue< - ReviewPanel.Value<'nVisibleSelectedChanges'> - >('reviewPanel.nVisibleSelectedChanges') + // All selected changes. If an aggregated change (insertion + deletion) is selected, the two ids + // will be present. The length of this array will differ from the count below (see explanation). + const selectedEntryIds = useRef([]) + // A count of user-facing selected changes. An aggregated change (insertion + deletion) will count + // as only one. + const [nVisibleSelectedChanges, setNVisibleSelectedChanges] = + useState>(0) const [collapsed, setCollapsed] = usePersistedState< ReviewPanel.Value<'collapsed'> >(`docs_collapsed_state:${projectId}`, {}, false, true) @@ -333,7 +343,9 @@ function useReviewPanelState(): ReviewPanelStateReactIde { // Assume we'll delete everything until we see it, then we'll remove it from this object const deleteChanges = new Set() - for (const [id, change] of Object.entries(docEntries)) { + for (const [id, change] of Object.entries(docEntries) as Entries< + typeof docEntries + >) { if ( 'entry_ids' in change && id !== 'add-comment' && @@ -344,7 +356,9 @@ function useReviewPanelState(): ReviewPanelStateReactIde { } } } - for (const [, change] of Object.entries(docResolvedComments)) { + for (const [, change] of Object.entries(docResolvedComments) as Entries< + typeof docResolvedComments + >) { if ('entry_ids' in change) { for (const entryId of change.entry_ids) { deleteChanges.add(entryId) @@ -383,10 +397,9 @@ function useReviewPanelState(): ReviewPanelStateReactIde { offset: change.op.p, metadata: change.metadata, } - const newEntryEntries = Object.entries(newEntry) as [ - [keyof typeof newEntry, typeof newEntry[keyof typeof newEntry]] - ] - for (const [key, value] of newEntryEntries) { + for (const [key, value] of Object.entries(newEntry) as Entries< + typeof newEntry + >) { const entriesTyped = docEntries[change.id] as Record entriesTyped[key] = value } @@ -417,9 +430,6 @@ function useReviewPanelState(): ReviewPanelStateReactIde { content: comment.op.c, offset: comment.op.p, } - const newEntryEntries = Object.entries(newEntry) as [ - [keyof typeof newEntry, typeof newEntry[keyof typeof newEntry]] - ] let newComment: any if (localResolvedThreadIds[comment.op.t]) { @@ -432,7 +442,9 @@ function useReviewPanelState(): ReviewPanelStateReactIde { delete docResolvedComments[comment.id] } - for (const [key, value] of newEntryEntries) { + for (const [key, value] of Object.entries(newEntry) as Entries< + typeof newEntry + >) { newComment[key] = value } } @@ -827,6 +839,8 @@ function useReviewPanelState(): ReviewPanelStateReactIde { (entry: { thread_id: ThreadId; replyContent: string }) => void >('submitReply') + const view = reviewPanelOpen ? subView : 'mini' + const toggleReviewPanel = useCallback(() => { if (!trackChangesVisible) { return @@ -870,18 +884,9 @@ function useReviewPanelState(): ReviewPanelStateReactIde { postJSON(`/project/${projectId}/thread/${entry.thread_id}/resolve`) onCommentResolved(entry.thread_id, user) - sendMB('rp-comment-resolve', { - view: reviewPanelOpen ? subView : 'mini', - }) + sendMB('rp-comment-resolve', { view }) }, - [ - getDocEntries, - onCommentResolved, - projectId, - reviewPanelOpen, - subView, - user, - ] + [getDocEntries, onCommentResolved, projectId, user, view] ) const onCommentReopened = useCallback( @@ -979,6 +984,47 @@ function useReviewPanelState(): ReviewPanelStateReactIde { [onCommentDeleted, projectId] ) + const doAcceptChanges = useCallback( + (entryIds: ThreadId[]) => { + const url = `/project/${projectId}/doc/${currentDocumentId}/changes/accept` + postJSON(url, { body: { change_ids: entryIds } }).catch( + debugConsole.error + ) + dispatchReviewPanelEvent('changes:accept', entryIds) + }, + [currentDocumentId, projectId] + ) + + const acceptChanges = useCallback( + (entryIds: ThreadId[]) => { + doAcceptChanges(entryIds) + sendMB('rp-changes-accepted', { view }) + }, + [doAcceptChanges, view] + ) + + const doRejectChanges = useCallback((entryIds: ThreadId[]) => { + dispatchReviewPanelEvent('changes:reject', entryIds) + }, []) + + const rejectChanges = useCallback( + (entryIds: ThreadId[]) => { + doRejectChanges(entryIds) + sendMB('rp-changes-rejected', { view }) + }, + [doRejectChanges, view] + ) + + const bulkAcceptActions = useCallback(() => { + doAcceptChanges(selectedEntryIds.current) + sendMB('rp-bulk-accept', { view, nEntries: nVisibleSelectedChanges }) + }, [doAcceptChanges, nVisibleSelectedChanges, view]) + + const bulkRejectActions = useCallback(() => { + doRejectChanges(selectedEntryIds.current) + sendMB('rp-bulk-reject', { view, nEntries: nVisibleSelectedChanges }) + }, [doRejectChanges, nVisibleSelectedChanges, view]) + const refreshRanges = useCallback(() => { type Doc = { id: DocId @@ -1020,19 +1066,6 @@ function useReviewPanelState(): ReviewPanelStateReactIde { updateEntries, ]) - const [acceptChanges] = - useScopeValue>('acceptChanges') - const [rejectChanges] = - useScopeValue>('rejectChanges') - const [bulkAcceptActions] = - useScopeValue>( - 'bulkAcceptActions' - ) - const [bulkRejectActions] = - useScopeValue>( - 'bulkRejectActions' - ) - const handleSetSubview = useCallback((subView: SubView) => { setSubView(subView) sendMB('rp-subview-change', { subView }) @@ -1075,9 +1108,132 @@ function useReviewPanelState(): ReviewPanelStateReactIde { handleLayoutChange() } + const editorFocusChanged = ( + selectionOffsetStart: number, + selectionOffsetEnd: number, + selection: boolean, + updateType: UpdateType + ) => { + let tempEntries = { + ...getDocEntries(currentDocumentId), + } + // All selected changes will be added to this array. + selectedEntryIds.current = [] + // Count of user-visible changes, i.e. an aggregated change will count as one. + let tempNVisibleSelectedChanges = 0 + + const offset = selectionOffsetStart + const length = selectionOffsetEnd - selectionOffsetStart + + // Recreate the add comment and bulk actions entries only when + // necessary. This is to avoid the UI thinking that these entries have + // changed and getting into an infinite loop. + if (selection) { + const existingAddComment = tempEntries[ + 'add-comment' + ] as ReviewPanelAddCommentEntry + if ( + !existingAddComment || + existingAddComment.offset !== offset || + existingAddComment.length !== length + ) { + tempEntries['add-comment'] = { + type: 'add-comment', + offset, + length, + } as ReviewPanelAddCommentEntry + } + const existingBulkActions = tempEntries[ + 'bulk-actions' + ] as ReviewPanelBulkActionsEntry + if ( + !existingBulkActions || + existingBulkActions.offset !== offset || + existingBulkActions.length !== length + ) { + tempEntries['bulk-actions'] = { + type: 'bulk-actions', + offset, + length, + } as ReviewPanelBulkActionsEntry + } + } else { + delete (tempEntries as Partial)['add-comment'] + delete (tempEntries as Partial)['bulk-actions'] + } + + for (const [key, entry] of Object.entries(tempEntries) as Entries< + typeof tempEntries + >) { + let isChangeEntryAndWithinSelection = false + if (entry.type === 'comment' && !resolvedThreadIds[entry.thread_id]) { + tempEntries = { + ...tempEntries, + [key]: { + ...tempEntries[key], + focused: + entry.offset <= selectionOffsetStart && + selectionOffsetStart <= entry.offset + entry.content.length, + }, + } + } else if ( + entry.type === 'insert' || + entry.type === 'aggregate-change' + ) { + isChangeEntryAndWithinSelection = + entry.offset >= selectionOffsetStart && + entry.offset + entry.content.length <= selectionOffsetEnd + tempEntries = { + ...tempEntries, + [key]: { + ...tempEntries[key], + focused: + entry.offset <= selectionOffsetStart && + selectionOffsetStart <= entry.offset + entry.content.length, + }, + } + } else if (entry.type === 'delete') { + isChangeEntryAndWithinSelection = + selectionOffsetStart <= entry.offset && + entry.offset <= selectionOffsetEnd + tempEntries = { + ...tempEntries, + [key]: { + ...tempEntries[key], + focused: entry.offset === selectionOffsetStart, + }, + } + } else if ( + ['add-comment', 'bulk-actions'].includes(entry.type) && + selection + ) { + tempEntries = { + ...tempEntries, + [key]: { ...tempEntries[key], focused: true }, + } + } + if (isChangeEntryAndWithinSelection) { + const entryIds = 'entry_ids' in entry ? entry.entry_ids : [] + for (const entryId of entryIds) { + selectedEntryIds.current.push(entryId) + } + tempNVisibleSelectedChanges++ + } + } + setEntries(prev => ({ ...prev, [currentDocumentId]: tempEntries })) + setNVisibleSelectedChanges(tempNVisibleSelectedChanges) + dispatchReviewPanelEvent('recalculate-screen-positions', { + entries: tempEntries, + updateType, + }) + // Ensure that watchers, such as the React-based review panel component, + // are informed of the changes to entries + handleLayoutChange() + } + const handleEditorEvents = (e: Event) => { const event = e as CustomEvent - const { type } = event.detail + const { type, payload } = event.detail switch (type) { case 'track-changes:changed': { @@ -1085,6 +1241,12 @@ function useReviewPanelState(): ReviewPanelStateReactIde { break } + case 'focus:changed': { + const { from, to, empty, updateType } = payload + editorFocusChanged(from, to, !empty, updateType) + break + } + case 'toggle-track-changes': { toggleTrackChangesFromKbdShortcut() break @@ -1099,6 +1261,8 @@ function useReviewPanelState(): ReviewPanelStateReactIde { } }, [ currentDocumentId, + getDocEntries, + resolvedThreadIds, toggleTrackChangesForUser, trackChanges, trackChangesState, @@ -1112,6 +1276,21 @@ function useReviewPanelState(): ReviewPanelStateReactIde { useSocketListener(socket, 'resolve-thread', onCommentResolved) useSocketListener(socket, 'edit-message', onCommentEdited) useSocketListener(socket, 'delete-message', onCommentDeleted) + useSocketListener( + socket, + 'accept-changes', + useCallback( + (docId: DocId, entryIds: ThreadId[]) => { + if (docId !== currentDocumentId) { + getChangeTracker(docId).removeChangeIds(entryIds) + } else { + dispatchReviewPanelEvent('changes:accept', entryIds) + } + updateEntries(docId) + }, + [currentDocumentId, getChangeTracker, updateEntries] + ) + ) const values = useMemo( () => ({ diff --git a/services/web/frontend/js/features/ide-react/scope-adapters/review-panel-context-adapter.ts b/services/web/frontend/js/features/ide-react/scope-adapters/review-panel-context-adapter.ts index 656de52409..b2495b477f 100644 --- a/services/web/frontend/js/features/ide-react/scope-adapters/review-panel-context-adapter.ts +++ b/services/web/frontend/js/features/ide-react/scope-adapters/review-panel-context-adapter.ts @@ -8,10 +8,7 @@ export default function populateReviewPanelScope(store: ReactScopeValueStore) { store.set('reviewPanel.rendererData.lineHeight', 0) store.set('submitNewComment', async () => {}) store.set('gotoEntry', () => {}) - store.set('acceptChanges', () => {}) - store.set('rejectChanges', () => {}) - store.set('bulkAcceptActions', () => {}) - store.set('bulkRejectActions', () => {}) + store.set('saveEdit', () => {}) store.set('submitReply', () => {}) store.set('addNewComment', () => {}) } diff --git a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts index 519aa2e40a..1d11beb102 100644 --- a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts +++ b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts @@ -58,8 +58,8 @@ export interface ReviewPanelState { resolveComment: (docId: DocId, entryId: ThreadId) => void deleteComment: (threadId: ThreadId, commentId: CommentId) => void submitReply: (threadId: ThreadId, replyContent: string) => void - acceptChanges: (entryIds: unknown) => void - rejectChanges: (entryIds: unknown) => void + acceptChanges: (entryIds: ThreadId[]) => void + rejectChanges: (entryIds: ThreadId[]) => void toggleTrackChangesForEveryone: (onForEveryone: boolean) => void toggleTrackChangesForUser: (onForUser: boolean, userId: UserId) => void toggleTrackChangesForGuests: (onForGuests: boolean) => void @@ -97,6 +97,7 @@ export interface ReviewPanelState { > } } + /* eslint-enable no-use-before-define */ // Getter for values diff --git a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts index b363f7b8e2..5af521f599 100644 --- a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts +++ b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts @@ -63,7 +63,7 @@ export type ChangeManager = { destroy: () => void } -type UpdateType = +export type UpdateType = | 'edit' | 'selectionChange' | 'geometryChange' diff --git a/services/web/types/review-panel/entry.ts b/services/web/types/review-panel/entry.ts index 803a0e062c..72f4a0b1f4 100644 --- a/services/web/types/review-panel/entry.ts +++ b/services/web/types/review-panel/entry.ts @@ -61,6 +61,7 @@ export interface ReviewPanelAggregateChangeEntry extends ReviewPanelBaseEntry { export interface ReviewPanelAddCommentEntry extends ReviewPanelBaseEntry { type: 'add-comment' + length: number } export interface ReviewPanelBulkActionsEntry extends ReviewPanelBaseEntry { diff --git a/services/web/types/utils.ts b/services/web/types/utils.ts index 4cb4394f30..4bdb852937 100644 --- a/services/web/types/utils.ts +++ b/services/web/types/utils.ts @@ -18,3 +18,7 @@ export type DeepReadonly = T extends (infer R)[] export type DeepPartial = Partial<{ [P in keyof T]: DeepPartial }> export type MergeAndOverride = Own & Omit + +export type Entries = [keyof T, T[keyof T]][] + +export type Keys = (keyof T)[]