From 3e11985e59d921d8cdf858d2ee4ed5c4a9fa4346 Mon Sep 17 00:00:00 2001 From: Kristina <7614497+khjrtbrg@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:45:40 +0200 Subject: [PATCH] [web] prevent dangling comments by using the thread's doc id in operations (#36423) GitOrigin-RevId: 9f3ddf282a058e92419e772b583169ac8bfe4af9 --- .../components/review-panel-comment.tsx | 8 +- .../review-panel-resolved-thread.tsx | 11 +- .../review-panel-resolved-threads-menu.tsx | 34 +- .../review-panel/context/threads-context.tsx | 47 ++- .../review-panel/review-panel.spec.tsx | 365 ++++++++++++++++++ .../source-editor/helpers/mock-doc.ts | 38 +- 6 files changed, 463 insertions(+), 40 deletions(-) diff --git a/services/web/frontend/js/features/review-panel/components/review-panel-comment.tsx b/services/web/frontend/js/features/review-panel/components/review-panel-comment.tsx index c7f60aac35..6df7814fb6 100644 --- a/services/web/frontend/js/features/review-panel/components/review-panel-comment.tsx +++ b/services/web/frontend/js/features/review-panel/components/review-panel-comment.tsx @@ -44,7 +44,7 @@ export const ReviewPanelComment = memo<{ const handleResolveComment = useCallback(async () => { setProcessing(true) try { - await resolveThread(comment.op.t) + await resolveThread(comment.op.t, docId) } catch (err) { debugConsole.error(err) showGenericMessageModal( @@ -54,7 +54,7 @@ export const ReviewPanelComment = memo<{ } finally { setProcessing(false) } - }, [comment.op.t, resolveThread, showGenericMessageModal, t]) + }, [comment.op.t, docId, resolveThread, showGenericMessageModal, t]) const handleEditMessage = useCallback( async (commentId: CommentId, content: string) => { @@ -110,7 +110,7 @@ export const ReviewPanelComment = memo<{ async (commentId: ThreadId) => { setProcessing(true) try { - await deleteThread(commentId) + await deleteThread(commentId, docId) } catch (err) { debugConsole.error(err) showGenericMessageModal( @@ -121,7 +121,7 @@ export const ReviewPanelComment = memo<{ setProcessing(false) } }, - [deleteThread, showGenericMessageModal, t] + [docId, deleteThread, showGenericMessageModal, t] ) const handleSubmitReply = useCallback( diff --git a/services/web/frontend/js/features/review-panel/components/review-panel-resolved-thread.tsx b/services/web/frontend/js/features/review-panel/components/review-panel-resolved-thread.tsx index e5b4b372b8..9bb7d3fdd8 100644 --- a/services/web/frontend/js/features/review-panel/components/review-panel-resolved-thread.tsx +++ b/services/web/frontend/js/features/review-panel/components/review-panel-resolved-thread.tsx @@ -16,8 +16,9 @@ import { useUserContext } from '@/shared/context/user-context' export const ReviewPanelResolvedThread: FC<{ id: ThreadId comment: Change + docId: string docName: string -}> = ({ id, comment, docName }) => { +}> = ({ id, comment, docId, docName }) => { const { t } = useTranslation() const { reopenThread, deleteThread } = useThreadsActionsContext() const [processing, setProcessing] = useState(false) @@ -33,7 +34,7 @@ export const ReviewPanelResolvedThread: FC<{ const handleReopenThread = useCallback(async () => { setProcessing(true) try { - await reopenThread(id) + await reopenThread(id, docId) } catch (err) { debugConsole.error(err) showGenericMessageModal( @@ -43,12 +44,12 @@ export const ReviewPanelResolvedThread: FC<{ } finally { setProcessing(false) } - }, [id, reopenThread, showGenericMessageModal, t]) + }, [id, docId, reopenThread, showGenericMessageModal, t]) const handleDeleteThread = useCallback(async () => { setProcessing(true) try { - await deleteThread(id) + await deleteThread(id, docId) } catch (err) { debugConsole.error(err) showGenericMessageModal( @@ -58,7 +59,7 @@ export const ReviewPanelResolvedThread: FC<{ } finally { setProcessing(false) } - }, [id, deleteThread, showGenericMessageModal, t]) + }, [id, docId, deleteThread, showGenericMessageModal, t]) return (
{ const { projectRanges, loading } = useProjectRanges() - const docNameForThread = useMemo(() => { - const docNameForThread = new Map() + const docForThread = useMemo(() => { + const docForThread = new Map() const otMigrationStage = getMeta('ol-otMigrationStage') - for (const [docId, ranges] of projectRanges?.entries() ?? []) { - const docName = docs?.find( - doc => (otMigrationStage === 1 ? doc.path : doc.doc.id) === docId - )?.doc.name - if (docName !== undefined) { + for (const [key, ranges] of projectRanges?.entries() ?? []) { + // history-ot keys project ranges by pathname, sharejs-text-ot by doc id + const matchingDoc = docs?.find( + doc => (otMigrationStage === 1 ? doc.path : doc.doc.id) === key + ) + if (matchingDoc !== undefined) { for (const comment of ranges.comments) { const threadId = comment.op.t - docNameForThread.set(threadId, docName) + docForThread.set(threadId, { + id: matchingDoc.doc.id, + name: matchingDoc.doc.name, + }) } } } - return docNameForThread + return docForThread }, [docs, projectRanges]) const allComments = useMemo(() => { @@ -72,8 +76,10 @@ export const ReviewPanelResolvedThreadsMenu: FC = () => { return Date.parse(b.thread.resolved_at) - Date.parse(a.thread.resolved_at) }) - return allResolvedThreads.filter(thread => allComments.has(thread.id)) - }, [threads, allComments]) + return allResolvedThreads.filter( + thread => allComments.has(thread.id) && docForThread.has(thread.id) + ) + }, [threads, allComments, docForThread]) if (loading) { return @@ -103,7 +109,8 @@ export const ReviewPanelResolvedThreadsMenu: FC = () => {
{resolvedThreads.map(thread => { const comment = allComments.get(thread.id) - if (!comment) { + const doc = docForThread.get(thread.id) + if (!comment || !doc) { return null } @@ -112,7 +119,8 @@ export const ReviewPanelResolvedThreadsMenu: FC = () => { key={thread.id} id={thread.id as ThreadId} comment={comment} - docName={docNameForThread.get(thread.id) ?? t('unknown')} + docId={doc.id} + docName={doc.name} /> ) })} diff --git a/services/web/frontend/js/features/review-panel/context/threads-context.tsx b/services/web/frontend/js/features/review-panel/context/threads-context.tsx index a12f740110..8e4ed57626 100644 --- a/services/web/frontend/js/features/review-panel/context/threads-context.tsx +++ b/services/web/frontend/js/features/review-panel/context/threads-context.tsx @@ -43,9 +43,9 @@ export const ThreadsContext = createContext(undefined) type ThreadsActions = { addComment: (pos: number, text: string, content: string) => Promise - resolveThread: (threadId: ThreadId) => Promise - reopenThread: (threadId: ThreadId) => Promise - deleteThread: (threadId: ThreadId) => Promise + resolveThread: (threadId: ThreadId, docId: string) => Promise + reopenThread: (threadId: ThreadId, docId: string) => Promise + deleteThread: (threadId: ThreadId, docId: string) => Promise addMessage: (threadId: ThreadId, content: string) => Promise editMessage: ( threadId: ThreadId, @@ -293,23 +293,25 @@ export const ThreadsProvider: FC = ({ children }) => { currentDocument.submitOp(op) }, - async resolveThread(threadId: string) { + async resolveThread(threadId: string, docId: string) { await postJSON( - `/project/${projectId}/doc/${currentDocument.doc_id}/thread/${threadId}/resolve` + `/project/${projectId}/doc/${docId}/thread/${threadId}/resolve` ) sendEvent('rp-comment-resolve', { view: reviewPanelView }) }, - async reopenThread(threadId: string) { + async reopenThread(threadId: string, docId: string) { await postJSON( - `/project/${projectId}/doc/${currentDocument.doc_id}/thread/${threadId}/reopen` + `/project/${projectId}/doc/${docId}/thread/${threadId}/reopen` ) sendEvent('rp-comment-reopen') }, - async deleteThread(threadId: string) { + async deleteThread(threadId: string, docId: string) { await deleteJSON( - `/project/${projectId}/doc/${currentDocument.doc_id}/thread/${threadId}` + `/project/${projectId}/doc/${docId}/thread/${threadId}` ) - currentDocument.ranges?.removeCommentId(threadId) + if (docId === currentDocument.doc_id) { + currentDocument.ranges?.removeCommentId(threadId) + } sendEvent('rp-comment-delete') }, async addMessage(threadId: ThreadId, content: string) { @@ -347,6 +349,10 @@ export const ThreadsProvider: FC = ({ children }) => { } if (isHistoryOT) { + // ops can only be submitted for the open document, so threads in other + // documents go through the HTTP API instead + const httpActions = { ...actions } + // TODO: dispatch on view instead? Object.assign(actions, { async addComment(pos: number, text: string, content: string) { @@ -370,7 +376,12 @@ export const ThreadsProvider: FC = ({ children }) => { effects: rangesUpdatedEffect.of(null), }) }, - async resolveThread(threadId: string) { + async resolveThread(threadId: string, docId: string) { + if (docId !== currentDocument.doc_id) { + await httpActions.resolveThread(threadId, docId) + return + } + const op = new SetCommentStateOperation(threadId, true) currentDocument.historyOTShareDoc.submitOp([op]) sendEvent('rp-comment-resolve', { view: reviewPanelView }) @@ -378,7 +389,12 @@ export const ThreadsProvider: FC = ({ children }) => { effects: rangesUpdatedEffect.of(null), }) }, - async reopenThread(threadId: string) { + async reopenThread(threadId: string, docId: string) { + if (docId !== currentDocument.doc_id) { + await httpActions.reopenThread(threadId, docId) + return + } + const op = new SetCommentStateOperation(threadId, false) currentDocument.historyOTShareDoc.submitOp([op]) sendEvent('rp-comment-reopen') @@ -386,7 +402,12 @@ export const ThreadsProvider: FC = ({ children }) => { effects: rangesUpdatedEffect.of(null), }) }, - async deleteThread(threadId: string) { + async deleteThread(threadId: string, docId: string) { + if (docId !== currentDocument.doc_id) { + await httpActions.deleteThread(threadId, docId) + return + } + const op = new DeleteCommentOperation(threadId) currentDocument.historyOTShareDoc.submitOp([op]) sendEvent('rp-comment-delete') diff --git a/services/web/test/frontend/features/review-panel/review-panel.spec.tsx b/services/web/test/frontend/features/review-panel/review-panel.spec.tsx index 6190887fb5..535537c30c 100644 --- a/services/web/test/frontend/features/review-panel/review-panel.spec.tsx +++ b/services/web/test/frontend/features/review-panel/review-panel.spec.tsx @@ -702,6 +702,371 @@ describe('', function () { }) }) +describe(' resolved comment in another file', function () { + const otherDocId = 'fake-nested-doc-id' + const otherDocThreadId = 'other-doc-thread-id' + + beforeEach(function () { + window.metaAttributesCache.set('ol-preventCompileOnLoad', true) + cy.interceptEvents() + cy.intercept('GET', '/project/*/changes/users', []) + + cy.intercept('GET', '/project/*/threads', { + [otherDocThreadId]: { + messages: [ + { + content: 'comment in another file', + id: `${otherDocThreadId}-1`, + timestamp: new Date('2025-01-01T00:00:00.000Z'), + user: userData, + user_id: USER_ID, + }, + ], + resolved: true, + resolved_at: new Date('2025-01-02T00:00:00.000Z').toISOString(), + resolved_by_user_id: USER_ID, + resolved_by_user: userData, + }, + }) + + cy.intercept('GET', '/project/*/ranges', [ + { + id: otherDocId, + ranges: { + changes: [], + comments: [ + { + id: 'other-doc-op-id', + op: { p: 161, c: 'Your introduction', t: otherDocThreadId }, + }, + ], + docId: otherDocId, + }, + }, + ]) + + cy.intercept( + 'POST', + `/project/*/doc/${otherDocId}/thread/${otherDocThreadId}/reopen`, + {} + ).as('reopenThreadInOtherDoc') + + cy.intercept( + 'DELETE', + `/project/*/doc/${otherDocId}/thread/${otherDocThreadId}`, + {} + ).as('deleteThreadInOtherDoc') + + const scope = mockScope(undefined, { + docOptions: { + rangesOptions: { removeCommentId: cy.stub().as('removeCommentId') }, + }, + }) + const project = mockProject({ + projectOwner: { _id: USER_ID }, + projectFeatures: { trackChanges: false, trackChangesVisible: true }, + }) + + cy.mount( + + + + + + ) + + cy.get('.cm-content').should('have.css', 'opacity', '1') + + // Open the review panel with keyboard shortcut + cy.findByText('contentLine 0').type('{command}j', { scrollBehavior: false }) + cy.findByText('contentLine 1').type('{ctrl}j', { scrollBehavior: false }) + + cy.findByTestId('review-panel').should('exist') + }) + + it('names the other file in the resolved comments dropdown', function () { + cy.findByLabelText('Resolved comments').click() + cy.findByRole('tooltip').within(() => { + cy.findByText('foo.tex').should('exist') + cy.findByText('comment in another file').should('exist') + }) + }) + + it('deletes the comment using the doc id of the thread', function () { + cy.findByLabelText('Resolved comments').click() + cy.findByRole('tooltip').within(() => { + cy.findByText('Delete').click({ force: true }) + }) + cy.wait('@deleteThreadInOtherDoc') + .its('request.url') + .should('contain', `/doc/${otherDocId}/thread/${otherDocThreadId}`) + }) + + it('leaves the ranges of the open document alone when deleting', function () { + cy.findByLabelText('Resolved comments').click() + cy.findByRole('tooltip').within(() => { + cy.findByText('Delete').click({ force: true }) + }) + cy.wait('@deleteThreadInOtherDoc') + cy.get('@removeCommentId').should('not.have.been.called') + }) + + it('reopens the comment using the doc id of the thread', function () { + cy.findByLabelText('Resolved comments').click() + cy.findByRole('tooltip').within(() => { + cy.findByText('Re-open').click({ force: true }) + }) + cy.wait('@reopenThreadInOtherDoc') + .its('request.url') + .should('contain', `/doc/${otherDocId}/thread/${otherDocThreadId}/reopen`) + }) +}) + +describe(' unresolved comment in another file', function () { + const otherDocId = 'fake-nested-doc-id' + const otherDocThreadId = 'other-doc-thread-id' + + beforeEach(function () { + window.metaAttributesCache.set('ol-preventCompileOnLoad', true) + cy.interceptEvents() + cy.intercept('GET', '/project/*/changes/users', []) + + cy.intercept('GET', '/project/*/threads', { + [otherDocThreadId]: { + messages: [ + { + content: 'comment in another file', + id: `${otherDocThreadId}-1`, + timestamp: new Date('2025-01-01T00:00:00.000Z'), + user: userData, + user_id: USER_ID, + }, + ], + }, + }) + + cy.intercept('GET', '/project/*/ranges', [ + { + id: otherDocId, + ranges: { + changes: [], + comments: [ + { + id: 'other-doc-op-id', + op: { p: 161, c: 'Your introduction', t: otherDocThreadId }, + }, + ], + docId: otherDocId, + }, + }, + ]) + + cy.intercept( + 'POST', + `/project/*/doc/${otherDocId}/thread/${otherDocThreadId}/resolve`, + {} + ).as('resolveThreadInOtherDoc') + + cy.intercept( + 'DELETE', + `/project/*/doc/${otherDocId}/thread/${otherDocThreadId}`, + {} + ).as('deleteThreadInOtherDoc') + + const scope = mockScope() + const project = mockProject({ + projectOwner: { _id: USER_ID }, + projectFeatures: { trackChanges: false, trackChangesVisible: true }, + }) + + cy.mount( + + + + + + ) + + cy.get('.cm-content').should('have.css', 'opacity', '1') + + // Open the review panel with keyboard shortcut + cy.findByText('contentLine 0').type('{command}j', { scrollBehavior: false }) + cy.findByText('contentLine 1').type('{ctrl}j', { scrollBehavior: false }) + + cy.findByTestId('review-panel').should('exist') + + // Overview mode lists the comments of every file, not just the open one + cy.findByRole('tab', { name: /overview/i }).click() + cy.findByText('comment in another file').should('exist') + }) + + it('resolves the comment using the doc id of the thread', function () { + // Find the resolve icon button using the hidden label + cy.findByText('Resolve comment').click({ force: true }) + cy.wait('@resolveThreadInOtherDoc') + .its('request.url') + .should( + 'contain', + `/doc/${otherDocId}/thread/${otherDocThreadId}/resolve` + ) + }) + + it('deletes the comment using the doc id of the thread', function () { + // Find the options icon button using the hidden label + cy.findByText('More options') + .first() + .click({ force: true, scrollBehavior: false }) + cy.findByRole('menu').within(() => { + cy.findByText('Delete').click({ scrollBehavior: false }) + }) + cy.findByRole('dialog').within(() => { + cy.findByRole('button', { name: 'Delete' }).click() + }) + cy.wait('@deleteThreadInOtherDoc') + .its('request.url') + .should('contain', `/doc/${otherDocId}/thread/${otherDocThreadId}`) + }) +}) + +describe(' resolved comment in another file (history OT)', function () { + const otherDocId = 'fake-nested-doc-id' + const otherDocPath = 'figures/foo.tex' + const otherDocThreadId = 'other-doc-thread-id' + const quotedText = 'Your introduction' + + beforeEach(function () { + window.metaAttributesCache.set('ol-preventCompileOnLoad', true) + window.metaAttributesCache.set('ol-otMigrationStage', 1) + cy.interceptEvents() + cy.intercept('GET', '/project/*/changes/users', []) + + cy.intercept('GET', '/project/*/threads', { + [otherDocThreadId]: { + messages: [ + { + content: 'comment in another file', + id: `${otherDocThreadId}-1`, + timestamp: new Date('2025-01-01T00:00:00.000Z'), + user: userData, + user_id: USER_ID, + }, + ], + resolved: true, + resolved_at: new Date('2025-01-02T00:00:00.000Z').toISOString(), + resolved_by_user_id: USER_ID, + resolved_by_user: userData, + }, + }) + + // In history OT the resolved comments menu reads project ranges from the + // project snapshot rather than from /project/:id/ranges. Inline the file + // content in the chunk so no blobs need fetching. + cy.intercept('POST', '/project/*/flush', { statusCode: 204 }) + cy.intercept('GET', '/project/*/changes?*', { body: [] }) + cy.intercept('GET', '/project/*/latest/history', { + body: { + chunk: { + history: { + snapshot: { + files: { + [otherDocPath]: { + content: quotedText, + comments: [ + { + id: otherDocThreadId, + ranges: [{ pos: 0, length: quotedText.length }], + resolved: true, + }, + ], + }, + }, + }, + changes: [], + }, + startVersion: 0, + }, + }, + }) + + cy.intercept( + 'POST', + `/project/*/doc/${otherDocId}/thread/${otherDocThreadId}/reopen`, + {} + ).as('reopenThreadInOtherDoc') + + cy.intercept( + 'DELETE', + `/project/*/doc/${otherDocId}/thread/${otherDocThreadId}`, + {} + ).as('deleteThreadInOtherDoc') + + const scope = mockScope(undefined, { docOptions: { historyOT: true } }) + const project = mockProject({ + projectOwner: { _id: USER_ID }, + projectFeatures: { trackChanges: false, trackChangesVisible: true }, + }) + + cy.mount( + + + + + + ) + + cy.get('.cm-content').should('have.css', 'opacity', '1') + + // Open the review panel with keyboard shortcut + cy.findByText('contentLine 0').type('{command}j', { scrollBehavior: false }) + cy.findByText('contentLine 1').type('{ctrl}j', { scrollBehavior: false }) + + cy.findByTestId('review-panel').should('exist') + }) + + it('names the other file in the resolved comments dropdown', function () { + cy.findByLabelText('Resolved comments').click() + cy.findByRole('tooltip').within(() => { + cy.findByText('foo.tex').should('exist') + cy.findByText('comment in another file').should('exist') + }) + }) + + it('deletes the comment over HTTP rather than submitting an op', function () { + // Opening the panel edits the doc, which submits ops of its own + cy.get('@historyOTSubmitOp').invoke('resetHistory') + cy.findByLabelText('Resolved comments').click() + cy.findByRole('tooltip').within(() => { + cy.findByText('Delete').click({ force: true }) + }) + cy.wait('@deleteThreadInOtherDoc') + .its('request.url') + .should('contain', `/doc/${otherDocId}/thread/${otherDocThreadId}`) + cy.get('@historyOTSubmitOp').should('not.have.been.called') + }) + + it('reopens the comment over HTTP rather than submitting an op', function () { + // Opening the panel edits the doc, which submits ops of its own + cy.get('@historyOTSubmitOp').invoke('resetHistory') + cy.findByLabelText('Resolved comments').click() + cy.findByRole('tooltip').within(() => { + cy.findByText('Re-open').click({ force: true }) + }) + cy.wait('@reopenThreadInOtherDoc') + .its('request.url') + .should('contain', `/doc/${otherDocId}/thread/${otherDocThreadId}/reopen`) + cy.get('@historyOTSubmitOp').should('not.have.been.called') + }) +}) + describe(' review tooltip without write permission', function () { beforeEach(function () { window.metaAttributesCache.set('ol-preventCompileOnLoad', true) diff --git a/services/web/test/frontend/features/source-editor/helpers/mock-doc.ts b/services/web/test/frontend/features/source-editor/helpers/mock-doc.ts index 9fccb91f37..06dfe3de8e 100644 --- a/services/web/test/frontend/features/source-editor/helpers/mock-doc.ts +++ b/services/web/test/frontend/features/source-editor/helpers/mock-doc.ts @@ -1,5 +1,9 @@ -import { ShareLatexOTShareDoc } from '../../../../../types/share-doc' +import { + HistoryOTShareDoc, + ShareLatexOTShareDoc, +} from '../../../../../types/share-doc' import { EventEmitter } from 'events' +import { StringFileData } from 'overleaf-editor-core' export const docId = 'test-doc' @@ -60,15 +64,39 @@ class MockShareDoc extends EventEmitter { } } +class MockHistoryOTShareDoc extends EventEmitter { + otType = 'history-ot' as const + snapshot: StringFileData + submitOp: (op: any[]) => void + + constructor(public text: string) { + super() + this.snapshot = new StringFileData(text) + // Stubbed so tests can assert whether an op was submitted for the open doc + // instead of going through the HTTP API. + this.submitOp = cy.stub().as('historyOTSubmitOp') + } + + getText() { + return this.text + } +} + export const mockDoc = ( content = defaultContent, - { rangesOptions = {} } = {} + { rangesOptions = {}, historyOT = false } = {} ) => { - const mockShareJSDoc: ShareLatexOTShareDoc = new MockShareDoc(content) + const mockShareJSDoc: ShareLatexOTShareDoc | HistoryOTShareDoc = historyOT + ? new MockHistoryOTShareDoc(content) + : new MockShareDoc(content) return { doc_id: docId, - getType: () => 'sharejs-text-ot', + getType: () => (historyOT ? 'history-ot' : 'sharejs-text-ot'), + historyOTShareDoc: mockShareJSDoc, + // The history-ot CodeMirror extension reads the share doc through + // `doc._doc` rather than through `historyOTShareDoc`. + doc: historyOT ? { _doc: mockShareJSDoc } : undefined, getSnapshot: () => { return content }, @@ -115,6 +143,6 @@ export const mockDoc = ( getPendingOp: () => null, hasBufferedOps: () => false, leaveAndCleanUpPromise: () => false, - isHistoryOT: () => false, + isHistoryOT: () => historyOT, } }