From 46198cd7806ec0385951f907d6490380a9a59aa7 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Wed, 2 Oct 2024 12:58:39 +0100 Subject: [PATCH] Merge pull request #20688 from overleaf/dk-fix-wrong-rangesdata Fix "RangeError" when switching files using new review panel GitOrigin-RevId: 399c8722584053c62acf5c413572fbbcdd1896cc --- .../source-editor/extensions/index.ts | 2 +- .../source-editor/extensions/ranges.ts | 241 ++++++++++-------- .../hooks/use-codemirror-scope.ts | 4 - 3 files changed, 132 insertions(+), 115 deletions(-) diff --git a/services/web/frontend/js/features/source-editor/extensions/index.ts b/services/web/frontend/js/features/source-editor/extensions/index.ts index e0c724a54f..a053a116ea 100644 --- a/services/web/frontend/js/features/source-editor/extensions/index.ts +++ b/services/web/frontend/js/features/source-editor/extensions/index.ts @@ -130,7 +130,7 @@ export const createExtensions = (options: Record): Extension[] => [ // so the decorations are added in the correct order. emptyLineFiller(), isSplitTestEnabled('review-panel-redesign') - ? ranges(options.currentDoc) + ? ranges() : trackChanges(options.currentDoc, options.changeManager), trackDetachedComments(options.currentDoc), visual(options.visual), diff --git a/services/web/frontend/js/features/source-editor/extensions/ranges.ts b/services/web/frontend/js/features/source-editor/extensions/ranges.ts index 7ee26f0746..31c951a40a 100644 --- a/services/web/frontend/js/features/source-editor/extensions/ranges.ts +++ b/services/web/frontend/js/features/source-editor/extensions/ranges.ts @@ -1,4 +1,4 @@ -import { StateEffect, TransactionSpec } from '@codemirror/state' +import { StateEffect, StateField, TransactionSpec } from '@codemirror/state' import { Decoration, type DecorationSet, @@ -18,7 +18,6 @@ import { isDeleteOperation, isInsertOperation, } from '@/utils/operations' -import { DocumentContainer } from '@/features/ide-react/editor/document-container' import { Ranges } from '@/features/review-panel-new/context/ranges-context' import { Threads } from '@/features/review-panel-new/context/threads-context' import { isSelectionWithinOp } from '@/features/review-panel-new/utils/is-selection-within-op' @@ -43,134 +42,156 @@ export const highlightRanges = (op?: AnyOperation): TransactionSpec => { } } -type Options = { - currentDoc: DocumentContainer - loadingThreads?: boolean - ranges?: Ranges - threads?: Threads -} +export const rangesDataField = StateField.define({ + create() { + return null + }, + update(rangesData, tr) { + for (const effect of tr.effects) { + if (effect.is(updateRangesEffect)) { + return effect.value + } + } + return rangesData + }, +}) /** * A custom extension that initialises the change manager, passes any updates to it, * and produces decorations for tracked changes and comments. */ -export const ranges = ({ ranges, threads }: Options) => { - return [ - // handle viewportChanged updates - ViewPlugin.define(view => { - let timer: number +export const ranges = () => [ + rangesDataField, + // handle viewportChanged updates + ViewPlugin.define(view => { + let timer: number + return { + update(update) { + if (update.viewportChanged) { + if (timer) { + window.clearTimeout(timer) + } + + timer = window.setTimeout(() => { + dispatchEvent(new Event('editor:viewport-changed')) + }, 25) + } + }, + } + }), + + // draw change decorations + ViewPlugin.define< + PluginValue & { + decorations: DecorationSet + } + >( + () => { return { + decorations: Decoration.none, update(update) { - if (update.viewportChanged) { - if (timer) { - window.clearTimeout(timer) - } + for (const transaction of update.transactions) { + this.decorations = this.decorations.map(transaction.changes) - timer = window.setTimeout(() => { - dispatchEvent(new Event('editor:viewport-changed')) - }, 25) + for (const effect of transaction.effects) { + if (effect.is(updateRangesEffect)) { + this.decorations = buildChangeDecorations(effect.value) + } + } } }, } - }), + }, + { + decorations: value => value.decorations, + } + ), - // draw change decorations - ViewPlugin.define< - PluginValue & { - decorations: DecorationSet - } - >( - () => { - return { - decorations: - ranges && threads - ? buildChangeDecorations({ ranges, threads }) - : Decoration.none, - update(update) { - for (const transaction of update.transactions) { - this.decorations = this.decorations.map(transaction.changes) + // draw highlight decorations + ViewPlugin.define< + PluginValue & { + decorations: DecorationSet + } + >( + () => { + return { + decorations: Decoration.none, + update(update) { + for (const transaction of update.transactions) { + this.decorations = this.decorations.map(transaction.changes) - for (const effect of transaction.effects) { - if (effect.is(updateRangesEffect)) { - this.decorations = buildChangeDecorations(effect.value) - } - } - } - }, - } - }, - { - decorations: value => value.decorations, - } - ), - - // draw highlight decorations - ViewPlugin.define< - PluginValue & { - decorations: DecorationSet - } - >( - () => { - return { - decorations: Decoration.none, - update(update) { - for (const transaction of update.transactions) { - this.decorations = this.decorations.map(transaction.changes) - - for (const effect of transaction.effects) { - if (effect.is(highlightRangesEffect)) { - this.decorations = buildHighlightDecorations( - 'ol-cm-change-highlight', - effect.value - ) - } - } - } - }, - } - }, - { - decorations: value => value.decorations, - } - ), - - // draw focus decorations - ViewPlugin.define< - PluginValue & { - decorations: DecorationSet - } - >( - () => { - return { - decorations: Decoration.none, - update(update) { - this.decorations = Decoration.none - - if (!ranges) { - return - } - - for (const range of [...ranges.changes, ...ranges.comments]) { - if (isSelectionWithinOp(range.op, update.state.selection.main)) { + for (const effect of transaction.effects) { + if (effect.is(highlightRangesEffect)) { this.decorations = buildHighlightDecorations( - 'ol-cm-change-focus', - range.op + 'ol-cm-change-highlight', + effect.value ) } } - }, - } - }, - { - decorations: value => value.decorations, + } + }, } - ), + }, + { + decorations: value => value.decorations, + } + ), - // styles for change decorations - trackChangesTheme, - ] -} + // draw focus decorations + ViewPlugin.define< + PluginValue & { + decorations: DecorationSet + } + >( + view => { + return { + decorations: Decoration.none, + update(update) { + if ( + !update.transactions.some( + tr => + tr.selection || + tr.effects.some(effect => effect.is(updateRangesEffect)) + ) + ) { + return + } + + this.decorations = Decoration.none + const rangesData = view.state.field(rangesDataField) + + if (!rangesData?.ranges) { + return + } + const { changes, comments } = rangesData.ranges + const unresolvedComments = rangesData.threads + ? comments.filter( + comment => + comment.op.t && !rangesData.threads[comment.op.t].resolved + ) + : [] + + for (const range of [...changes, ...unresolvedComments]) { + if (isSelectionWithinOp(range.op, update.state.selection.main)) { + this.decorations = buildHighlightDecorations( + 'ol-cm-change-focus', + range.op + ) + break + } + } + }, + } + }, + { + decorations: value => value.decorations, + } + ), + + // styles for change decorations + trackChangesTheme, +] const buildChangeDecorations = (data: RangesData) => { if (!data.ranges) { diff --git a/services/web/frontend/js/features/source-editor/hooks/use-codemirror-scope.ts b/services/web/frontend/js/features/source-editor/hooks/use-codemirror-scope.ts index 84b97c3769..c93f92d11e 100644 --- a/services/web/frontend/js/features/source-editor/hooks/use-codemirror-scope.ts +++ b/services/web/frontend/js/features/source-editor/hooks/use-codemirror-scope.ts @@ -172,8 +172,6 @@ function useCodeMirrorScope(view: EditorView) { currentDoc, trackChanges, loadingThreads, - threads, - ranges, }) useEffect(() => { @@ -183,8 +181,6 @@ function useCodeMirrorScope(view: EditorView) { }, [view, currentDoc]) useEffect(() => { - currentDocRef.current.ranges = ranges - currentDocRef.current.threads = threads if (ranges && threads) { window.setTimeout(() => { view.dispatch(updateRanges({ ranges, threads }))