[web] prevent dangling comments by using the thread's doc id in operations (#36423)

GitOrigin-RevId: 9f3ddf282a058e92419e772b583169ac8bfe4af9
This commit is contained in:
Kristina
2026-09-17 11:54:06 +00:00
committed by Copybot
parent 2cb515ac2f
commit 3e11985e59
6 changed files with 463 additions and 40 deletions
@@ -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(
@@ -16,8 +16,9 @@ import { useUserContext } from '@/shared/context/user-context'
export const ReviewPanelResolvedThread: FC<{
id: ThreadId
comment: Change<CommentOperation>
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 (
<div
@@ -17,23 +17,27 @@ export const ReviewPanelResolvedThreadsMenu: FC = () => {
const { projectRanges, loading } = useProjectRanges()
const docNameForThread = useMemo(() => {
const docNameForThread = new Map<string, string>()
const docForThread = useMemo(() => {
const docForThread = new Map<string, { id: string; name: string }>()
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 <LoadingSpinner className="ms-auto me-auto" />
@@ -103,7 +109,8 @@ export const ReviewPanelResolvedThreadsMenu: FC = () => {
</div>
{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}
/>
)
})}
@@ -43,9 +43,9 @@ export const ThreadsContext = createContext<Threads | undefined>(undefined)
type ThreadsActions = {
addComment: (pos: number, text: string, content: string) => Promise<void>
resolveThread: (threadId: ThreadId) => Promise<void>
reopenThread: (threadId: ThreadId) => Promise<void>
deleteThread: (threadId: ThreadId) => Promise<void>
resolveThread: (threadId: ThreadId, docId: string) => Promise<void>
reopenThread: (threadId: ThreadId, docId: string) => Promise<void>
deleteThread: (threadId: ThreadId, docId: string) => Promise<void>
addMessage: (threadId: ThreadId, content: string) => Promise<void>
editMessage: (
threadId: ThreadId,
@@ -293,23 +293,25 @@ export const ThreadsProvider: FC<React.PropsWithChildren> = ({ 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<React.PropsWithChildren> = ({ 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<React.PropsWithChildren> = ({ 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<React.PropsWithChildren> = ({ 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<React.PropsWithChildren> = ({ 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')
@@ -702,6 +702,371 @@ describe('<ReviewPanel />', function () {
})
})
describe('<ReviewPanel /> 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(
<TestContainer className="rp-size-expanded">
<EditorProviders
scope={scope}
providers={{ ProjectProvider: makeProjectProvider(project) }}
>
<CodeMirrorEditor />
</EditorProviders>
</TestContainer>
)
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('<ReviewPanel /> 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(
<TestContainer className="rp-size-expanded">
<EditorProviders
scope={scope}
providers={{ ProjectProvider: makeProjectProvider(project) }}
>
<CodeMirrorEditor />
</EditorProviders>
</TestContainer>
)
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('<ReviewPanel /> 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(
<TestContainer className="rp-size-expanded">
<EditorProviders
scope={scope}
providers={{ ProjectProvider: makeProjectProvider(project) }}
>
<CodeMirrorEditor />
</EditorProviders>
</TestContainer>
)
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('<ReviewPanel /> review tooltip without write permission', function () {
beforeEach(function () {
window.metaAttributesCache.set('ol-preventCompileOnLoad', true)
@@ -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,
}
}