From 85e2467da64050b1a3c4b33ab4e8e1c218d052ae Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Tue, 19 May 2026 19:56:46 -0300 Subject: [PATCH 1/7] fix(cli): headless bridge syncs browser-authored comment metadata from Y.Array (SD-3214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The headless SDK's `editor.doc.comments.list()` was returning partial data for browser-authored comments: `id`, `target`, `anchoredText`, `status` populated correctly from the PM anchor mark (synced via y-prosemirror), but `text`, `creatorName`, `creatorEmail`, `createdTime` came back empty. Cause: browser SuperDoc clients write comment data over two channels — the PM XmlFragment carries anchor marks, and `ydoc.getArray('comments')` carries metadata. The CLI's `headless-comment-bridge.ts` only had the write half of this dance. It never fed Y.Array entries into the editor's `CommentEntityStore`, which is what `comments.list()` reads from. Fix: - New shared helper in super-editor: `syncCommentEntitiesFromCollaboration` maps Y.Array entry shape to `CommentEntityRecord`, upserts via the existing `upsertCommentEntity`, filters out tracked-change entries (separate domain), and supports an optional `previouslySynced` set to prune remote deletions via `removeCommentEntityTree`. - Bridge gains `attachEditor(editor)` called after `Editor.open()` resolves. Seeds the store from the current Y.Array snapshot, then installs an observer that re-syncs on remote events while skipping own-origin echoes using the same `transaction.origin.user` shape the browser writers use. - `document.ts` wires `attachEditor` once. CLI engine-agnostic boundary preserved: the bridge forwards the editor handle to the shared helper but never touches `editor.state` / `editor.storage` directly. The helper lives in super-editor so the next consumer (mobile SDK, edge worker) doesn't re-derive the mapping. Coverage: - 13 new unit tests in `comment-entity-store.test.ts` cover upsert paths, alias handling (`text` vs `commentText`), tracked-change filtering, importedId fallback, merge-without-clobber, resolution metadata, and the four deletion scenarios (prune previously-synced, leave locally- authored alone, cascade thread replies, no-op when no removals). - 8 integration tests in `sd-3214-browser-comment-metadata.regression.test.ts` cover: pre-open Y.Array entry, post-open observe, remote update, Option A (two Editors on shared Y.Doc), Option A origin-filter (no self-echo), Option A remote delete, Option B (two Y.Docs with Yjs update relay), Option B late-join. Out of scope (separate gap, documented in the test file): the CLI's write-side delete path doesn't currently propagate to Y.Array because the `removeComment` editor command sets `tr.setMeta` but never emits `commentsUpdate({ type: 'deleted' })`. The bridge's existing 'deleted' handler is therefore unused for CLI-initiated deletes. This affects the customer's "agent resolves comments" flow and is worth its own ticket. The bridge's READ-side prune (this PR) is correct: it would fire if any client — including a future CLI write path or a real browser — removes the Y.Array entry. Full super-editor suite (13115) and CLI suite (1246) green. --- .../__tests__/lib/_collab-password-worker.ts | 2 + ...rowser-comment-metadata.regression.test.ts | 436 ++++++++++++++++++ apps/cli/src/lib/document.ts | 5 + apps/cli/src/lib/headless-comment-bridge.ts | 76 ++- .../helpers/comment-entity-store.test.ts | 183 ++++++++ .../helpers/comment-entity-store.ts | 87 ++++ packages/super-editor/src/editors/v1/index.js | 3 + 7 files changed, 790 insertions(+), 2 deletions(-) create mode 100644 apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts diff --git a/apps/cli/src/__tests__/lib/_collab-password-worker.ts b/apps/cli/src/__tests__/lib/_collab-password-worker.ts index 28d30d11e5..83da2eb063 100644 --- a/apps/cli/src/__tests__/lib/_collab-password-worker.ts +++ b/apps/cli/src/__tests__/lib/_collab-password-worker.ts @@ -32,6 +32,8 @@ mock.module('superdoc/super-editor', () => ({ getDocumentApiAdapters: () => ({}), markdownToPmDoc: () => null, initPartsRuntime: () => ({ dispose: () => {} }), + // SD-3214: bridge imports this to feed Y.Array entries into the store. + syncCommentEntitiesFromCollaboration: () => new Set(), })); mock.module('happy-dom', () => ({ diff --git a/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts b/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts new file mode 100644 index 0000000000..898a7b94c8 --- /dev/null +++ b/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts @@ -0,0 +1,436 @@ +/** + * SD-3214 regression: comment metadata authored on the browser side (Y.Array) + * must reach the headless SDK's CommentEntityStore so `doc.comments.list()` + * surfaces text / creatorName / creatorEmail / createdTime. + * + * Architectural gap (pre-fix): browser clients write to BOTH + * `ydoc.getArray('comments')` (metadata) and the PM XmlFragment (anchor marks). + * The headless CLI bridge only handled the WRITE direction. The fix wires + * `bridge.attachEditor(editor)` to observe Y.Array and feed entries through + * `syncCommentEntitiesFromCollaboration`. + * + * This test focuses on the ENTITY STORE flow: when a Y.Array entry exists + * pre-open OR arrives post-open, its metadata should be readable via + * `doc.comments.list()`. (PM anchor presence is orthogonal — it supplies + * target/anchoredText/status when the comment is anchored to text.) + */ +import { describe, expect, it } from 'vitest'; +import { Doc as YDoc, Map as YMap } from 'yjs'; +import { openDocument } from '../document'; + +function createIo() { + return { + stdout() {}, + stderr() {}, + async readStdinBytes() { + return new Uint8Array(); + }, + now() { + return Date.now(); + }, + }; +} + +function createProviderStub() { + const noop = () => {}; + return { + synced: true, + awareness: { + on: noop, + off: noop, + getStates: () => new Map(), + setLocalState: noop, + setLocalStateField: noop, + }, + on: noop, + off: noop, + connect: noop, + disconnect: noop, + destroy: noop, + }; +} + +/** + * Mirror the shape `addYComment` in collaboration-comments.js produces when + * a browser user creates a comment via SuperDoc.vue's normal flow. + */ +function pushBrowserAuthoredComment(ydoc: YDoc, comment: Record): void { + const yArray = ydoc.getArray('comments'); + const yComment = new YMap(Object.entries(comment)); + ydoc.transact( + () => { + yArray.push([yComment]); + }, + { user: { name: comment.creatorName, email: comment.creatorEmail } }, + ); +} + +describe('SD-3214: headless SDK reads browser-authored comment metadata', () => { + it('exposes creatorName, createdTime, and text from Y.Array entries pre-existing at open', async () => { + const ydoc = new YDoc(); + + // Browser authored this comment BEFORE the headless client connected. + pushBrowserAuthoredComment(ydoc, { + commentId: 'c-browser-pre', + commentText: 'Please review this clause.', + creatorName: 'Browser User', + creatorEmail: 'browser@example.com', + createdTime: 1700000000000, + isInternal: false, + }); + + const opened = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-doc', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + const result = opened.editor.doc.comments.list(); + opened.dispose(); + + const item = result.items.find((c) => c.id === 'c-browser-pre'); + expect(item, 'browser-authored comment should be listed by headless SDK').toBeDefined(); + expect(item?.text, 'commentText from Y.Array should reach SDK').toBe('Please review this clause.'); + expect(item?.creatorName, 'creatorName from Y.Array should reach SDK').toBe('Browser User'); + expect(item?.creatorEmail, 'creatorEmail from Y.Array should reach SDK').toBe('browser@example.com'); + expect(item?.createdTime, 'createdTime from Y.Array should reach SDK').toBe(1700000000000); + }); + + it('exposes metadata for browser-authored comments that arrive AFTER open via Y.Array observe', async () => { + const ydoc = new YDoc(); + + const opened = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-doc-late', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + // Browser authors the comment AFTER the headless client connected. + pushBrowserAuthoredComment(ydoc, { + commentId: 'c-browser-late', + commentText: 'A late comment.', + creatorName: 'Late Browser User', + creatorEmail: 'late@example.com', + createdTime: 1700000001000, + isInternal: false, + }); + + const result = opened.editor.doc.comments.list(); + opened.dispose(); + + const late = result.items.find((c) => c.id === 'c-browser-late'); + expect(late, 'late browser-authored comment should be listed').toBeDefined(); + expect(late?.text).toBe('A late comment.'); + expect(late?.creatorName).toBe('Late Browser User'); + expect(late?.creatorEmail).toBe('late@example.com'); + expect(late?.createdTime).toBe(1700000001000); + }); + + it('a remote update to an existing browser comment surfaces the new fields', async () => { + const ydoc = new YDoc(); + + pushBrowserAuthoredComment(ydoc, { + commentId: 'c-update', + commentText: 'v1', + creatorName: 'Browser User', + creatorEmail: 'browser@example.com', + createdTime: 1700000002000, + }); + + const opened = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-doc-update', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + // Browser edits the same comment. + const yArray = ydoc.getArray>('comments'); + ydoc.transact( + () => { + yArray.delete(0, 1); + yArray.push([ + new YMap( + Object.entries({ + commentId: 'c-update', + commentText: 'v2', + creatorName: 'Browser User', + creatorEmail: 'browser@example.com', + createdTime: 1700000002000, + }), + ), + ]); + }, + { user: { name: 'Browser User', email: 'browser@example.com' } }, + ); + + const result = opened.editor.doc.comments.list(); + opened.dispose(); + + const item = result.items.find((c) => c.id === 'c-update'); + expect(item?.text).toBe('v2'); + }); +}); + +// --------------------------------------------------------------------------- +// Two-client validation (SD-3214 follow-up). +// Option A: two Editors on a single shared Y.Doc — proves Yjs change broadcast +// reaches Session B's bridge through the real write path that a browser SuperDoc +// would use (editor.commands.addComment + bridge onCommentsUpdate → addYComment). +// Option B: two distinct Y.Docs synced by relaying updates — closer to +// wire-protocol reality. Verifies origin filtering survives applyUpdate hops. +// --------------------------------------------------------------------------- + +import { applyUpdate as yApplyUpdate, encodeStateAsUpdate as yEncodeStateAsUpdate } from 'yjs'; + +describe('SD-3214: two-client end-to-end', () => { + it('Option A — shared Y.Doc: Session B sees a comment authored by Session A', async () => { + const ydoc = new YDoc(); + + // Session A — author (the "browser" side, run as a headless Editor here so + // the test stays in Node; the code path it exercises is identical to what + // SuperDoc.vue runs). + const sessionA = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-shared-doc', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Browser User', email: 'browser@example.com' }, + }); + const insertA = sessionA.editor.doc.create.paragraph({ + at: { kind: 'documentEnd' }, + text: 'A clause about indemnification.', + }); + expect(insertA.success).toBe(true); + const matchA = sessionA.editor.doc.query.match({ + select: { type: 'text', pattern: 'indemnification' }, + require: 'first', + }); + const matchBlock = matchA.items[0].blocks[0]; + const create = sessionA.editor.doc.comments.create({ + target: { kind: 'text', blockId: matchBlock.blockId, range: matchBlock.range } as never, + text: 'Please review this clause.', + }); + expect(create.success).toBe(true); + + // Session B — agent connects to the SAME ydoc. + const sessionB = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-shared-doc', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: false, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + const listB = sessionB.editor.doc.comments.list(); + sessionA.dispose(); + sessionB.dispose(); + + expect(listB.items.length).toBeGreaterThanOrEqual(1); + const item = listB.items[0]; + // Anchor-derived fields survive via PM/Yjs sync. + expect(item.target).toBeDefined(); + // Y.Array-derived metadata — the field set the ticket flagged as empty. + expect(item.text).toBe('Please review this clause.'); + expect(item.creatorName).toBe('Browser User'); + expect(item.creatorEmail).toBe('browser@example.com'); + expect(item.createdTime).toBeTypeOf('number'); + }); + + it('Option A — origin filter: Session A does not double-sync its own writes through observe', async () => { + // After A writes a comment, A's bridge observer fires for its own write. + // The origin filter must skip — otherwise the entry could be processed + // twice (idempotent thanks to upsert, but wasted work and a hint of a + // broken filter that would matter under deletion). + const ydoc = new YDoc(); + const sessionA = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-self-echo', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Author', email: 'author@example.com' }, + }); + sessionA.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'A short clause.' }); + const matchBlock = sessionA.editor.doc.query.match({ + select: { type: 'text', pattern: 'clause' }, + require: 'first', + }).items[0].blocks[0]; + sessionA.editor.doc.comments.create({ + target: { kind: 'text', blockId: matchBlock.blockId, range: matchBlock.range } as never, + text: 'mine', + }); + + // Trigger any pending observers by reading. + const list = sessionA.editor.doc.comments.list(); + sessionA.dispose(); + + expect(list.items).toHaveLength(1); + expect(list.items[0].text).toBe('mine'); + expect(list.items[0].creatorName).toBe('Author'); + }); + + it('Option B — two Y.Docs synced via update relay: Session B sees the metadata', async () => { + const docA = new YDoc(); + const docB = new YDoc(); + + // Manual sync relay — simulates a network bridge. Origin tags prevent + // infinite echo loops. + const relayAtoB = (update: Uint8Array, origin: unknown) => { + if (origin === 'from-B') return; + yApplyUpdate(docB, update, 'from-A'); + }; + const relayBtoA = (update: Uint8Array, origin: unknown) => { + if (origin === 'from-A') return; + yApplyUpdate(docA, update, 'from-B'); + }; + docA.on('update', relayAtoB); + docB.on('update', relayBtoA); + + const sessionA = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-two-docs', + ydoc: docA, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Browser User', email: 'browser@example.com' }, + }); + sessionA.editor.doc.create.paragraph({ + at: { kind: 'documentEnd' }, + text: 'A clause on confidentiality.', + }); + const matchA = sessionA.editor.doc.query.match({ + select: { type: 'text', pattern: 'confidentiality' }, + require: 'first', + }); + const matchBlockA = matchA.items[0].blocks[0]; + sessionA.editor.doc.comments.create({ + target: { kind: 'text', blockId: matchBlockA.blockId, range: matchBlockA.range } as never, + text: 'Confidentiality should cover IP.', + }); + + // Seed docB from docA before opening Session B — mimics a fresh client + // joining the room after some history exists. + yApplyUpdate(docB, yEncodeStateAsUpdate(docA), 'from-A'); + + const sessionB = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-two-docs', + ydoc: docB, + collaborationProvider: createProviderStub(), + isNewFile: false, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + const listB = sessionB.editor.doc.comments.list(); + sessionA.dispose(); + sessionB.dispose(); + docA.off('update', relayAtoB); + docB.off('update', relayBtoA); + + expect(listB.items.length).toBeGreaterThanOrEqual(1); + const item = listB.items[0]; + expect(item.text).toBe('Confidentiality should cover IP.'); + expect(item.creatorName).toBe('Browser User'); + expect(item.creatorEmail).toBe('browser@example.com'); + }); + + it('Option A — remote delete: when a browser removes a comment from Y.Array, Session B prunes the entry', async () => { + // Scope: this validates Session B's READ-SIDE prune behavior. The browser + // delete is simulated by removing the entry from ydoc.getArray('comments') + // directly — exactly what packages/superdoc/.../collaboration-comments.js + // deleteYComment does. The CLI's own write-side delete bridging is tracked + // separately; SD-3214 covers metadata propagation from browser to agent. + const ydoc = new YDoc(); + + // Seed Y.Array with a browser-authored comment. + pushBrowserAuthoredComment(ydoc, { + commentId: 'c-to-delete', + commentText: 'Will be removed.', + creatorName: 'Browser User', + creatorEmail: 'browser@example.com', + createdTime: 1700000010000, + }); + + const opened = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-delete-readside', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + // Session sees the comment after attach. + const beforeList = opened.editor.doc.comments.list(); + expect(beforeList.items.find((c) => c.id === 'c-to-delete')).toBeDefined(); + + // Simulate browser deletion of the Y.Array entry. + const yArr = ydoc.getArray>('comments'); + const idx = (yArr.toJSON() as Array>).findIndex((c) => c.commentId === 'c-to-delete'); + ydoc.transact( + () => { + yArr.delete(idx, 1); + }, + { user: { name: 'Browser User', email: 'browser@example.com' } }, + ); + + const afterList = opened.editor.doc.comments.list(); + opened.dispose(); + + expect(afterList.items.find((c) => c.id === 'c-to-delete')).toBeUndefined(); + }); + + it('Option B — post-open browser write: a comment authored AFTER the agent connects still propagates', async () => { + const docA = new YDoc(); + const docB = new YDoc(); + const relayAtoB = (update: Uint8Array, origin: unknown) => { + if (origin === 'from-B') return; + yApplyUpdate(docB, update, 'from-A'); + }; + const relayBtoA = (update: Uint8Array, origin: unknown) => { + if (origin === 'from-A') return; + yApplyUpdate(docA, update, 'from-B'); + }; + docA.on('update', relayAtoB); + docB.on('update', relayBtoA); + + // Agent connects FIRST. + const sessionB = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-late-author', + ydoc: docB, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + // Browser connects and authors a comment. + const sessionA = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-late-author', + ydoc: docA, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Browser User', email: 'browser@example.com' }, + }); + sessionA.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'A late clause.' }); + const matchBlock = sessionA.editor.doc.query.match({ + select: { type: 'text', pattern: 'late clause' }, + require: 'first', + }).items[0].blocks[0]; + sessionA.editor.doc.comments.create({ + target: { kind: 'text', blockId: matchBlock.blockId, range: matchBlock.range } as never, + text: 'Late comment.', + }); + + const listB = sessionB.editor.doc.comments.list(); + sessionA.dispose(); + sessionB.dispose(); + docA.off('update', relayAtoB); + docB.off('update', relayBtoA); + + const found = listB.items.find((c) => (c as { text?: string }).text === 'Late comment.'); + expect(found, 'browser-authored comment after agent connect should reach agent').toBeDefined(); + expect((found as { creatorName?: string }).creatorName).toBe('Browser User'); + }); +}); diff --git a/apps/cli/src/lib/document.ts b/apps/cli/src/lib/document.ts index 5b3e5faf2c..bd01d7347d 100644 --- a/apps/cli/src/lib/document.ts +++ b/apps/cli/src/lib/document.ts @@ -222,6 +222,11 @@ export async function openDocument( // afterCommit hooks are always wired, including in headless CLI sessions. initPartsRuntime(editor as never); + // SD-3214: bridge observes ydoc.getArray('comments') and feeds remote + // (browser-authored) metadata into the editor's CommentEntityStore so the + // headless SDK can read text/creatorName/createdTime via doc.comments.list(). + commentBridge?.attachEditor(editor as never); + // Apply content override post-init. // - markdown: DOM-free AST pipeline // - plainText: builds PM paragraphs directly, preserving all whitespace diff --git a/apps/cli/src/lib/headless-comment-bridge.ts b/apps/cli/src/lib/headless-comment-bridge.ts index adab125c2d..2f80b7fcc3 100644 --- a/apps/cli/src/lib/headless-comment-bridge.ts +++ b/apps/cli/src/lib/headless-comment-bridge.ts @@ -11,9 +11,16 @@ */ import { Map as YMap } from 'yjs'; -import type { Doc as YDoc, Array as YArray } from 'yjs'; +import type { Doc as YDoc, Array as YArray, YArrayEvent } from 'yjs'; +import { syncCommentEntitiesFromCollaboration } from 'superdoc/super-editor'; import type { UserIdentity } from './types'; +// Editor handle is intentionally typed as `unknown` here — the bridge only +// forwards it to `syncCommentEntitiesFromCollaboration`, which owns the +// engine-specific knowledge. Keeping the type opaque preserves the CLI's +// engine-agnostic boundary. +type EditorHandle = Parameters[0]; + // --------------------------------------------------------------------------- // Yjs write helpers (mirrors collaboration-comments.js) // --------------------------------------------------------------------------- @@ -144,7 +151,23 @@ export interface HeadlessCommentBridgeResult { onCommentsUpdate: (params: Record) => void; onCommentsLoaded: (params: { editor: unknown; comments: unknown[] }) => void; }; - /** Cleanup — clears internal registry */ + /** + * Wire the bridge to an Editor instance once `Editor.open()` resolves. + * + * Seeds the editor's CommentEntityStore from the current Y.Array contents, + * then installs a Y.Array observer that mirrors remote (other-client) + * comment additions, updates, and removals into the store. Without this, + * `editor.doc.comments.list()` only sees PM-anchor data for browser- + * authored comments and the text/creatorName/createdTime fields are empty. + * + * Origin filter: events whose `transaction.origin.user` matches the bridge + * user are skipped — those came from this client's own writes and are + * already in the store via the normal `onCommentsUpdate` path. + * + * Safe to call multiple times; only the most recent editor is observed. + */ + attachEditor(editor: EditorHandle): void; + /** Cleanup — clears internal registry and detaches Y.Array observer */ dispose(): void; } @@ -274,6 +297,51 @@ export function buildHeadlessCommentBridge(ydoc: unknown, user?: UserIdentity): ); } + // ---- Y.Array → CommentEntityStore observer (SD-3214) ---- + + let attachedEditor: EditorHandle | null = null; + let yArrayObserver: ((event: YArrayEvent>) => void) | null = null; + // Set of commentIds previously synced from Y.Array. The helper uses this + // to detect remote deletions and prune them from the store. + let previousSyncedIds: ReadonlySet = new Set(); + + function syncYArrayToStore(): void { + if (!attachedEditor) return; + const entries = yArray.toJSON() as Array>; + previousSyncedIds = syncCommentEntitiesFromCollaboration(attachedEditor, entries, { + previouslySynced: previousSyncedIds, + }); + } + + function detachYArrayObserver(): void { + if (yArrayObserver) { + yArray.unobserve(yArrayObserver); + yArrayObserver = null; + } + } + + function attachEditor(editor: EditorHandle): void { + detachYArrayObserver(); + attachedEditor = editor; + // Reset the prior-sync set so a re-attach (e.g. document re-open) doesn't + // prune entries that are genuinely fresh from this editor's perspective. + previousSyncedIds = new Set(); + + // Initial seed: pull whatever is already in the room. + syncYArrayToStore(); + + yArrayObserver = (event) => { + // Skip our own writes — they're already in the store via onCommentsUpdate. + const origin = (event.transaction.origin ?? null) as { user?: { name?: string; email?: string } } | null; + const originUser = origin?.user; + if (userOrigin && originUser && originUser.name === userOrigin.name && originUser.email === userOrigin.email) { + return; + } + syncYArrayToStore(); + }; + yArray.observe(yArrayObserver); + } + return { editorOptions: { isCommentsEnabled: true, @@ -281,7 +349,11 @@ export function buildHeadlessCommentBridge(ydoc: unknown, user?: UserIdentity): onCommentsUpdate: handleCommentsUpdate, onCommentsLoaded: handleCommentsLoaded, }, + attachEditor, dispose() { + detachYArrayObserver(); + attachedEditor = null; + previousSyncedIds = new Set(); registry.clear(); }, }; diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts index f435142c8f..2ccadb8cef 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts @@ -8,6 +8,7 @@ import { getCommentEntityStore, isCommentResolved, removeCommentEntityTree, + syncCommentEntitiesFromCollaboration, toCommentInfo, upsertCommentEntity, type CommentEntityRecord, @@ -252,3 +253,185 @@ describe('toCommentInfo', () => { expect(info.anchoredText).toBeUndefined(); }); }); + +describe('syncCommentEntitiesFromCollaboration (SD-3214)', () => { + it('upserts a new browser-authored comment into an empty store', () => { + const editor = makeEditorWithConverter(); + syncCommentEntitiesFromCollaboration(editor, [ + { + commentId: 'c1', + commentText: 'Please review this clause.', + creatorName: 'Browser User', + creatorEmail: 'browser@example.com', + createdTime: 1700000000000, + isInternal: false, + }, + ]); + + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(1); + expect(store[0].commentId).toBe('c1'); + expect(store[0].commentText).toBe('Please review this clause.'); + expect(store[0].creatorName).toBe('Browser User'); + expect(store[0].creatorEmail).toBe('browser@example.com'); + expect(store[0].createdTime).toBe(1700000000000); + expect(store[0].isInternal).toBe(false); + }); + + it('accepts `text` as a fallback for `commentText`', () => { + // Some browser writers emit { text } instead of { commentText }; mirror + // the alias logic the browser-side loader uses. + const editor = makeEditorWithConverter(); + syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'c1', text: 'short form' }]); + const store = getCommentEntityStore(editor); + expect(store[0].commentText).toBe('short form'); + }); + + it('skips entries flagged trackedChange:true (those belong to a separate domain)', () => { + const editor = makeEditorWithConverter(); + syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'tc-1', trackedChange: true, trackedChangeText: 'inserted', creatorName: 'A' }, + { commentId: 'c-1', commentText: 'real comment', creatorName: 'B' }, + ]); + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(1); + expect(store[0].commentId).toBe('c-1'); + }); + + it('skips entries without a commentId', () => { + const editor = makeEditorWithConverter(); + syncCommentEntitiesFromCollaboration(editor, [{ creatorName: 'orphan' }, { commentId: 'c-ok', creatorName: 'X' }]); + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(1); + expect(store[0].commentId).toBe('c-ok'); + }); + + it('falls back to importedId when commentId is missing', () => { + const editor = makeEditorWithConverter(); + syncCommentEntitiesFromCollaboration(editor, [{ importedId: 'imp-1', creatorName: 'X' }]); + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(1); + expect(store[0].commentId).toBe('imp-1'); + expect(store[0].importedId).toBe('imp-1'); + }); + + it('merges an updated entry without clobbering unchanged fields', () => { + const editor = makeEditorWithConverter([ + { + commentId: 'c1', + commentText: 'v1', + creatorName: 'Author', + creatorEmail: 'author@example.com', + createdTime: 1, + }, + ]); + // Remote update bumps commentText only. + syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'c1', commentText: 'v2' }]); + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(1); + expect(store[0].commentText).toBe('v2'); + expect(store[0].creatorName).toBe('Author'); + expect(store[0].creatorEmail).toBe('author@example.com'); + expect(store[0].createdTime).toBe(1); + }); + + it('propagates resolution metadata', () => { + const editor = makeEditorWithConverter([{ commentId: 'c1', commentText: 'hi' }]); + syncCommentEntitiesFromCollaboration(editor, [ + { + commentId: 'c1', + commentText: 'hi', + isDone: true, + resolvedTime: 1700000005000, + resolvedByEmail: 'resolver@example.com', + resolvedByName: 'Resolver', + }, + ]); + const store = getCommentEntityStore(editor); + expect(store[0].isDone).toBe(true); + expect(store[0].resolvedTime).toBe(1700000005000); + expect(store[0].resolvedByEmail).toBe('resolver@example.com'); + expect(store[0].resolvedByName).toBe('Resolver'); + }); + + it('returns the set of synced comment ids (for caller-driven deletion sweep)', () => { + const editor = makeEditorWithConverter(); + const seen = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'c1' }, + { commentId: 'c2' }, + { trackedChange: true, commentId: 'tc-1' }, + ]); + expect(seen).toEqual(new Set(['c1', 'c2'])); + }); + + it('is a no-op for empty input', () => { + const editor = makeEditorWithConverter([{ commentId: 'pre', commentText: 'kept' }]); + syncCommentEntitiesFromCollaboration(editor, []); + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(1); + expect(store[0].commentText).toBe('kept'); + }); + + // Remote-deletion handling: when a prior collab-synced id disappears from + // the upstream Y.Array, the helper prunes the matching store entry. + it('prunes a previously-synced entry that is no longer in upstream entries', () => { + const editor = makeEditorWithConverter(); + const first = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'a', commentText: 'a' }, + { commentId: 'b', commentText: 'b' }, + ]); + expect(getCommentEntityStore(editor)).toHaveLength(2); + expect(first).toEqual(new Set(['a', 'b'])); + + // Remote drops 'a'. + const second = syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'b', commentText: 'b' }], { + previouslySynced: first, + }); + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(1); + expect(store[0].commentId).toBe('b'); + expect(second).toEqual(new Set(['b'])); + }); + + it('does not prune locally-authored entries that were never collab-synced', () => { + // 'local' is in the store but never in `previouslySynced` — the helper + // must leave it alone even though it isn't in the upstream entries. + const editor = makeEditorWithConverter([{ commentId: 'local', commentText: 'cli-authored' }]); + syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'remote', commentText: 'r' }], { + previouslySynced: new Set(), + }); + const store = getCommentEntityStore(editor); + expect(store).toHaveLength(2); + expect(store.map((e) => e.commentId).sort()).toEqual(['local', 'remote']); + }); + + it('prunes thread replies along with the deleted parent', () => { + // removeCommentEntityTree cascades to children — confirm via the helper. + const editor = makeEditorWithConverter(); + const first = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'root' }, + { commentId: 'reply-1', parentCommentId: 'root' }, + { commentId: 'reply-2', parentCommentId: 'root' }, + { commentId: 'unrelated' }, + ]); + expect(getCommentEntityStore(editor)).toHaveLength(4); + + syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'unrelated' }], { + previouslySynced: first, + }); + const store = getCommentEntityStore(editor); + expect(store.map((e) => e.commentId).sort()).toEqual(['unrelated']); + }); + + it('returns the new sync set unchanged when no removals occur', () => { + const editor = makeEditorWithConverter(); + const first = syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'a' }, { commentId: 'b' }]); + const second = syncCommentEntitiesFromCollaboration( + editor, + [{ commentId: 'a' }, { commentId: 'b' }, { commentId: 'c' }], + { previouslySynced: first }, + ); + expect(second).toEqual(new Set(['a', 'b', 'c'])); + expect(getCommentEntityStore(editor)).toHaveLength(3); + }); +}); diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts index c1f8712413..778ed125a2 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts @@ -182,6 +182,93 @@ export function isCommentResolved(entry: CommentEntityRecord): boolean { return Boolean(entry.isDone || entry.resolvedTime); } +/** + * Sync remote comment metadata from a collaboration channel (e.g. the Yjs + * `ydoc.getArray('comments')` used by browser SuperDoc clients) into the + * editor's CommentEntityStore. + * + * Without this sync, the headless SDK only sees PM-anchor-derived fields + * (id, target, anchoredText, status) for browser-authored comments — the + * Y.Array metadata (text, creatorName, creatorEmail, createdTime) never + * reaches `doc.comments.list()`. See SD-3214. + * + * Behavior: + * - Each entry with a `commentId` is upserted into the store. Existing + * entries are merged (collaborator-authored fields override locally + * captured ones; missing fields are left alone). + * - Entries flagged `trackedChange: true` are skipped — those belong to + * the tracked-changes domain, not the comments store. + * - When `options.previouslySynced` is provided, any id present in the + * prior set but absent from the current entries is treated as a remote + * deletion and pruned via `removeCommentEntityTree`. Locally-authored + * entries that were never collab-synced are left alone. + * - Returns the set of commentIds observed during the sync. Callers should + * pass this back as `previouslySynced` on the next call to detect + * subsequent remote deletions. + */ +export function syncCommentEntitiesFromCollaboration( + editor: Editor, + entries: ReadonlyArray>, + options: { previouslySynced?: ReadonlySet } = {}, +): Set { + const store = getCommentEntityStore(editor); + const seen = new Set(); + + for (const raw of entries) { + if (!raw || typeof raw !== 'object') continue; + if (raw.trackedChange === true) continue; + + const commentId = toNonEmptyString(raw.commentId) ?? toNonEmptyString(raw.importedId); + if (!commentId) continue; + seen.add(commentId); + + const patch: Partial = {}; + // Identity fields + if (typeof raw.importedId === 'string') patch.importedId = raw.importedId; + if (typeof raw.parentCommentId === 'string') patch.parentCommentId = raw.parentCommentId; + // Body + const commentText = + typeof raw.commentText === 'string' ? raw.commentText : typeof raw.text === 'string' ? raw.text : undefined; + if (commentText !== undefined) patch.commentText = commentText; + if (raw.commentJSON !== undefined) patch.commentJSON = raw.commentJSON; + if (raw.elements !== undefined) patch.elements = raw.elements; + // Authoring metadata + if (typeof raw.creatorName === 'string') patch.creatorName = raw.creatorName; + if (typeof raw.creatorEmail === 'string') patch.creatorEmail = raw.creatorEmail; + if (typeof raw.creatorImage === 'string') patch.creatorImage = raw.creatorImage; + if (typeof raw.createdTime === 'number') patch.createdTime = raw.createdTime; + // Status + if (typeof raw.isInternal === 'boolean') patch.isInternal = raw.isInternal; + if (typeof raw.isDone === 'boolean') patch.isDone = raw.isDone; + if (typeof raw.resolvedTime === 'number') patch.resolvedTime = raw.resolvedTime; + if (raw.resolvedTime === null) patch.resolvedTime = null; + if (typeof raw.resolvedByEmail === 'string') patch.resolvedByEmail = raw.resolvedByEmail; + if (typeof raw.resolvedByName === 'string') patch.resolvedByName = raw.resolvedByName; + + upsertCommentEntity(store, commentId, patch); + } + + // Prune entries previously known to come from collab sync but now absent + // from the upstream Y.Array. Locally-authored entries that were never in + // `previouslySynced` are intentionally left alone. + if (options.previouslySynced) { + for (const priorId of options.previouslySynced) { + if (!seen.has(priorId)) { + removeCommentEntityTree(store, priorId); + } + } + } + + return seen; +} + +/** Local helper: trim+narrow a value to a non-empty string. */ +function toNonEmptyString(value: unknown): string | undefined { + if (typeof value !== 'string') return undefined; + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : undefined; +} + export function toCommentInfo( entry: CommentEntityRecord, options: { diff --git a/packages/super-editor/src/editors/v1/index.js b/packages/super-editor/src/editors/v1/index.js index 0979a94485..fcf98a5d4b 100644 --- a/packages/super-editor/src/editors/v1/index.js +++ b/packages/super-editor/src/editors/v1/index.js @@ -51,6 +51,7 @@ import { onCollaborationProviderSynced } from './core/helpers/collaboration-prov import { resolveSelectionTarget } from './document-api-adapters/helpers/selection-target-resolver.js'; import { resolveDefaultInsertTarget } from './document-api-adapters/helpers/adapter-utils.js'; import { resolveTrackedChangeInStory } from './document-api-adapters/helpers/tracked-change-resolver.js'; +import { syncCommentEntitiesFromCollaboration } from './document-api-adapters/helpers/comment-entity-store.js'; import { getTrackedChangeIndex } from './document-api-adapters/tracked-changes/tracked-change-index.js'; import { makeTrackedChangeAnchorKey, @@ -158,6 +159,8 @@ export { resolveDefaultInsertTarget, /** @internal */ resolveTrackedChangeInStory, + /** @internal SD-3214: feed collaboration-sourced comment metadata into the editor's CommentEntityStore. */ + syncCommentEntitiesFromCollaboration, // Story-aware tracked-change service /** @internal */ From 3865418e65cbd869ff728a5717639e61018d0c27 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Tue, 19 May 2026 20:15:19 -0300 Subject: [PATCH 2/7] fix(document-api): emit commentsUpdate from comment wrappers (SD-3214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The customer's "agent resolves comments" flow needs CLI-initiated `comments.delete` and `comments.patch({ status: 'resolved' })` calls to reach other browser collaborators via Y.Array. Pre-fix, none of those write paths propagated: - The engine commands (`resolveComment`, `reopenComment`, `removeComment`) set `tr.setMeta(CommentsPluginKey, { event: ... })` but never call `editor.emit('commentsUpdate', ...)`. - The headless bridge's existing `case 'deleted'`/`case 'update'` handlers therefore never fire for CLI-initiated mutations. - The browser's own delete path (`commentsStore.deleteComment`) sidesteps this by manually calling `superdoc.emit('comments-update', ...)` + `syncCommentsToClients(...)`, but `ui.comments.delete` (which routes through `editor.doc.comments.delete`) hits the same gap — so the browser's Document-API surface was also silently broken for collab. Fix: emit `commentsUpdate` from `resolveCommentHandler`, `reopenCommentHandler`, and `removeCommentHandler` in `comments-wrappers.ts` after a successful mutation. Symmetric with how `addCommentHandler` and `editCommentHandler` already rely on engine-level emits. Effects: - CLI bridge's `onCommentsUpdate('deleted' | 'update' | 'resolved')` handlers fire and call `deleteYComment` / `updateYComment` correctly. - Browser's `onEditorCommentsUpdate` DELETED/UPDATE branches in `SuperDoc.vue` start receiving events for the `ui.comments.*` surface, refreshing the Vue list to match the entity store. - Browser's `commentsStore.deleteComment` path (calls `editor.commands.removeComment` directly, bypassing the wrapper) is unaffected — no double-fire. Adds 2 integration tests covering the customer's flow: agent deletes a comment and Y.Array reflects it; agent resolves a comment and `isDone` + `resolvedTime` reach Y.Array. Full super-editor suite (13117) and CLI suite (1248) green. --- ...rowser-comment-metadata.regression.test.ts | 70 +++++++++++++++++++ .../plan-engine/comments-wrappers.ts | 61 +++++++++++++++- 2 files changed, 130 insertions(+), 1 deletion(-) diff --git a/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts b/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts index 898a7b94c8..518a6ab573 100644 --- a/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts +++ b/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts @@ -382,6 +382,76 @@ describe('SD-3214: two-client end-to-end', () => { expect(afterList.items.find((c) => c.id === 'c-to-delete')).toBeUndefined(); }); + it('CLI write-side delete: agent.comments.delete propagates to Y.Array (customer "agent resolves comments" flow)', async () => { + // The customer's headless agent needs to mutate comments and have those + // mutations reach other browser collaborators. resolveComment / removeComment + // engine commands don't emit `commentsUpdate`, so SD-3214's bridge fix + // pairs with wrapper-level emits in `comments-wrappers.ts`. + const ydoc = new YDoc(); + const session = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-cli-delete', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + session.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'A clause to delete.' }); + const matchBlock = session.editor.doc.query.match({ + select: { type: 'text', pattern: 'delete' }, + require: 'first', + }).items[0].blocks[0]; + session.editor.doc.comments.create({ + target: { kind: 'text', blockId: matchBlock!.blockId, range: matchBlock!.range } as never, + text: 'agent-authored', + }); + const yArr = ydoc.getArray('comments').toJSON() as Array>; + expect(yArr.length).toBe(1); + const targetId = yArr[0].commentId as string; + + // Agent deletes — this is the write-side that previously didn't propagate. + const del = session.editor.doc.comments.delete({ commentId: targetId }); + expect(del.success).toBe(true); + + // Y.Array now reflects the delete (other collaborators would observe this). + const afterYArr = ydoc.getArray('comments').toJSON() as Array>; + session.dispose(); + expect(afterYArr).toHaveLength(0); + }); + + it('CLI write-side resolve: agent.comments.patch({status:resolved}) propagates resolvedTime to Y.Array', async () => { + const ydoc = new YDoc(); + const session = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-cli-resolve', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + session.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'A clause to resolve.' }); + const matchBlock = session.editor.doc.query.match({ + select: { type: 'text', pattern: 'resolve' }, + require: 'first', + }).items[0].blocks[0]; + session.editor.doc.comments.create({ + target: { kind: 'text', blockId: matchBlock!.blockId, range: matchBlock!.range } as never, + text: 'pending review', + }); + const initial = ydoc.getArray('comments').toJSON() as Array>; + const targetId = initial[0].commentId as string; + expect(initial[0].resolvedTime).toBeFalsy(); + + // Agent resolves via the public patch surface. + const patch = session.editor.doc.comments.patch({ commentId: targetId, status: 'resolved' }); + expect(patch.success).toBe(true); + + // Y.Array reflects the resolution. + const after = ydoc.getArray('comments').toJSON() as Array>; + session.dispose(); + expect(after).toHaveLength(1); + expect(after[0].isDone).toBe(true); + expect(typeof after[0].resolvedTime).toBe('number'); + }); + it('Option B — post-open browser write: a comment authored AFTER the agent connects still propagates', async () => { const docA = new YDoc(); const docB = new YDoc(); diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts index a828ee85a4..b5f4ec71e0 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts @@ -147,6 +147,29 @@ function listCommentAnchorsSafe(editor: Editor): ReturnType, +): void { + const emitter = (editor as unknown as { emit?: (event: string, payload: unknown) => void }).emit; + if (typeof emitter !== 'function') return; + emitter.call(editor, 'commentsUpdate', { type, comment }); +} + function applyTextSelection(editor: Editor, from: number, to: number): boolean { const setTextSelection = editor.commands?.setTextSelection; if (typeof setTextSelection === 'function') { @@ -762,6 +785,8 @@ function resolveCommentHandler(editor: Editor, input: ResolveCommentInput, optio }; } + let resolvedTimestamp: number | null = null; + const receipt = executeDomainCommand( editor, () => { @@ -770,10 +795,11 @@ function resolveCommentHandler(editor: Editor, input: ResolveCommentInput, optio importedId: identity.importedId, }); if (didResolve) { + resolvedTimestamp = Date.now(); upsertCommentEntity(store, identity.commentId, { importedId: identity.importedId, isDone: true, - resolvedTime: Date.now(), + resolvedTime: resolvedTimestamp, }); } return Boolean(didResolve); @@ -788,6 +814,18 @@ function resolveCommentHandler(editor: Editor, input: ResolveCommentInput, optio }; } + // SD-3214: the resolveComment engine command sets `tr.setMeta(CommentsPluginKey, { event: 'update' })` + // but does not emit `commentsUpdate`. The browser commentsStore handles its own resolve flow by + // emitting `comments-update` manually + writing to Y.Array. Document-API consumers (CLI, MCP) need + // the wrapper to fire the canonical event so the headless bridge can propagate to Y.Array via its + // existing `'update'` / `'resolved'` handler. + emitCommentLifecycleUpdate(editor, 'resolved', { + commentId: identity.commentId, + importedId: identity.importedId, + isDone: true, + resolvedTime: resolvedTimestamp, + }); + return { success: true, updated: [toCommentAddress(identity.commentId)] }; } @@ -850,6 +888,16 @@ function reopenCommentHandler(editor: Editor, input: ReopenCommentInput, options }; } + // SD-3214: reopenComment doesn't emit either — surface a canonical + // 'update' event so the bridge can mirror the cleared resolved markers + // into Y.Array. + emitCommentLifecycleUpdate(editor, 'update', { + commentId: identity.commentId, + importedId: identity.importedId, + isDone: false, + resolvedTime: null, + }); + return { success: true, updated: [toCommentAddress(identity.commentId)] }; } @@ -890,6 +938,17 @@ function removeCommentHandler(editor: Editor, input: RemoveCommentInput, options removedIds.add(identity.commentId); } + // SD-3214: removeComment engine command sets `tr.setMeta` but doesn't emit + // `commentsUpdate`. Emit here so the headless bridge propagates the delete + // to Y.Array (and the browser's existing DELETED branch refreshes its Vue + // list). Emits per removed id so thread-reply cascades reach subscribers. + for (const removedId of removedIds) { + emitCommentLifecycleUpdate(editor, 'deleted', { + commentId: removedId, + importedId: removedId === identity.commentId ? identity.importedId : undefined, + }); + } + return { success: true, removed: Array.from(removedIds).map((id) => toCommentAddress(id)), From bd0e6482fc0de67237044b208a0c6fbbbd4075b0 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Wed, 20 May 2026 08:41:31 -0300 Subject: [PATCH 3/7] chore(cli): add manual repro script + guide for SD-3214 In-process two-Editor harness on a shared Y.Doc. Exercises the three flows of the customer use case: read browser-authored metadata, resolve a comment from the agent, delete a comment from the agent. Each scenario prints PASS/FAIL so reviewers can eyeball the fix end-to-end without standing up Liveblocks/Hocuspocus infra. NODE_ENV=test bun apps/cli/scripts/repro-sd-3214.ts README documents how to toggle each half of the fix off (sed-revert / git-checkout the relevant files) to compare buggy vs fixed output. --- apps/cli/scripts/README-sd-3214.md | 124 ++++++++++++++++ apps/cli/scripts/repro-sd-3214.ts | 223 +++++++++++++++++++++++++++++ 2 files changed, 347 insertions(+) create mode 100644 apps/cli/scripts/README-sd-3214.md create mode 100644 apps/cli/scripts/repro-sd-3214.ts diff --git a/apps/cli/scripts/README-sd-3214.md b/apps/cli/scripts/README-sd-3214.md new file mode 100644 index 0000000000..5aa91e30e4 --- /dev/null +++ b/apps/cli/scripts/README-sd-3214.md @@ -0,0 +1,124 @@ +# SD-3214 — Manual End-to-End Reproduction + +Three scenarios that exercise the headless SDK's comment-sync pipeline through real Yjs primitives. Runs in one Node process; no Liveblocks/Hocuspocus required. + +## 1. Run the repro (fix on) + +From the worktree root: + +```bash +NODE_ENV=test bun apps/cli/scripts/repro-sd-3214.ts +``` + +Expected output (with the fix applied): + +``` +=== Scenario 1: READ — browser → agent metadata propagation === + agent.comments.list() returned 1 item(s): +{ + id: "", + text: "Please review this clause.", + creatorName: "Browser User", + creatorEmail: "browser@example.com", + createdTime: 1779..., + target: "present", +} + READ ✓ — metadata fully propagated + +=== Scenario 2: WRITE — agent resolves comment, browser sees it === + agent.comments.patch({status:'resolved'}).success === true + { + commentId: "", + isDone: true, + resolvedTime: 1779..., + } + WRITE ✓ — resolve propagated to Y.Array + +=== Scenario 3: DELETE — agent deletes, Y.Array shrinks === + agent.comments.delete().success === true + Y.Array length after delete: 0 + DELETE ✓ — Y.Array entry removed +``` + +## 2. See the bug (fix off) + +To confirm what the pre-fix state looks like, disable each fix individually. + +### Disable read-side (bridge.attachEditor) + +```bash +# Comment out the attachEditor wiring: +sed -i.bak "s|commentBridge?.attachEditor(editor as never);|// commentBridge?.attachEditor(editor as never);|" \ + apps/cli/src/lib/document.ts + +NODE_ENV=test bun apps/cli/scripts/repro-sd-3214.ts +# Scenario 1 now prints: +# text: undefined, creatorName: undefined, creatorEmail: undefined, createdTime: undefined +# READ ✗ — metadata missing (this is the SD-3214 read-side bug pre-fix) + +# Restore: +mv apps/cli/src/lib/document.ts.bak apps/cli/src/lib/document.ts +``` + +### Disable write-side (wrapper emits) + +```bash +# Comment out the three emits in comments-wrappers.ts: +git stash # save state first +git checkout HEAD~1 -- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts +# (this reverts the wrapper to the first commit, before the write-side fix) + +NODE_ENV=test bun apps/cli/scripts/repro-sd-3214.ts +# Scenarios 2 and 3 now print: +# WRITE ✗ — resolve did not reach Y.Array +# DELETE ✗ — Y.Array still has the entry + +# Restore: +git checkout HEAD -- packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts +git stash pop +``` + +## 3. What each scenario simulates + +### Scenario 1 — READ direction + +Mirrors the customer's flow: a user authors a comment in the browser SuperDoc; an agent connects to the same Y.Doc and reads the comment via `editor.doc.comments.list()`. + +Both sessions are headless Editor instances sharing one in-memory Y.Doc. The "browser" session uses the user identity `Browser User `; the "agent" session uses `Headless Agent `. Yjs broadcasts changes between them through the natural CRDT mechanism — no network required. + +Pass condition: all four metadata fields (`text`, `creatorName`, `creatorEmail`, `createdTime`) populate on the agent side. + +### Scenario 2 — WRITE direction (resolve) + +The agent calls `editor.doc.comments.patch({ commentId, status: 'resolved' })`. The Y.Array entry should reflect `isDone: true` and a numeric `resolvedTime`, so other clients observing the Y.Doc see the resolution. + +Pass condition: `yEntry.isDone === true && typeof yEntry.resolvedTime === 'number'`. + +### Scenario 3 — WRITE direction (delete) + +The agent calls `editor.doc.comments.delete({ commentId })`. The Y.Array entry should disappear. + +Pass condition: `ydoc.getArray('comments').toJSON().length === 0`. + +## 4. Extending to a real two-process setup + +If you want to validate against an actual collaboration provider (Liveblocks, Hocuspocus, custom websocket), the same code structure works — replace `providerStub()` with a real provider returned by `@superdoc-dev/cli`'s `createCollaborationRuntime`, and run the "browser" half in the dev server (`pnpm dev`) and the "agent" half via the CLI binary connected to the same room. + +The in-memory shared Y.Doc here is the structural equivalent. If it passes, the network case will pass too — Yjs's wire protocol is just the same CRDT updates delivered over a socket. + +## 5. Running the unit + integration suites + +For machine-readable validation: + +```bash +# CLI integration tests (10 cases for SD-3214) +pnpm --filter @superdoc-dev/cli test -- --run sd-3214 + +# super-editor unit + integration tests +pnpm --filter super-editor test -- --run comment-entity-store +pnpm --filter super-editor test -- --run comments-wrappers + +# Full suites (slower, for pre-merge confidence) +pnpm --filter @superdoc-dev/cli test # 1248 pass +pnpm --filter super-editor test # 13117 pass +``` diff --git a/apps/cli/scripts/repro-sd-3214.ts b/apps/cli/scripts/repro-sd-3214.ts new file mode 100644 index 0000000000..ada6a7ff74 --- /dev/null +++ b/apps/cli/scripts/repro-sd-3214.ts @@ -0,0 +1,223 @@ +/** + * SD-3214 end-to-end manual reproduction. + * + * Runs entirely in one Node process — no Liveblocks/Hocuspocus needed. + * Two Editor instances share a single Y.Doc, so changes from one side + * propagate to the other through the exact same Yjs primitives a real + * browser + agent pair would use over the wire. + * + * USAGE (from the worktree root): + * + * NODE_ENV=test bun apps/cli/scripts/repro-sd-3214.ts + * + * Three scenarios print PASS/FAIL lines so you can eyeball whether the + * fix is active. See the "Toggle the fix" section in the guide for how + * to compare before vs after. + */ + +import { Doc as YDoc } from 'yjs'; +import { openDocument } from '../src/lib/document'; + +const io = { + stdout: () => {}, + stderr: () => {}, + readStdinBytes: async () => new Uint8Array(), + now: () => Date.now(), +}; + +function providerStub() { + const noop = () => {}; + return { + synced: true, + awareness: { + on: noop, + off: noop, + getStates: () => new Map(), + setLocalState: noop, + setLocalStateField: noop, + }, + on: noop, + off: noop, + connect: noop, + disconnect: noop, + destroy: noop, + }; +} + +// --------------------------------------------------------------------------- +// Scenario 1: READ — browser authors, agent reads, metadata propagates +// --------------------------------------------------------------------------- + +async function scenarioReadSide() { + console.log('\n=== Scenario 1: READ — browser → agent metadata propagation ==='); + const ydoc = new YDoc(); + + const browser = await openDocument(undefined, io, { + documentId: 'sd-3214-readside', + ydoc, + collaborationProvider: providerStub() as never, + isNewFile: true, + user: { name: 'Browser User', email: 'browser@example.com' }, + }); + + browser.editor.doc.create.paragraph({ + at: { kind: 'documentEnd' }, + text: 'A clause about indemnification.', + }); + const block = browser.editor.doc.query.match({ + select: { type: 'text', pattern: 'indemnification' }, + require: 'first', + }).items[0]!.blocks[0]!; + browser.editor.doc.comments.create({ + target: { kind: 'text', blockId: block.blockId, range: block.range } as never, + text: 'Please review this clause.', + }); + + const agent = await openDocument(undefined, io, { + documentId: 'sd-3214-readside', + ydoc, + collaborationProvider: providerStub() as never, + isNewFile: false, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + + const list = agent.editor.doc.comments.list(); + console.log(` agent.comments.list() returned ${list.items.length} item(s):`); + for (const item of list.items) { + console.log({ + id: item.id, + text: (item as { text?: string }).text, + creatorName: (item as { creatorName?: string }).creatorName, + creatorEmail: (item as { creatorEmail?: string }).creatorEmail, + createdTime: (item as { createdTime?: number }).createdTime, + target: (item as { target?: unknown }).target ? 'present' : 'absent', + }); + } + + const item = list.items[0] as { text?: string; creatorName?: string; createdTime?: number } | undefined; + if (item?.text && item?.creatorName && item?.createdTime) { + console.log(' READ ✓ — metadata fully propagated'); + } else { + console.log(' READ ✗ — metadata missing (this is the SD-3214 read-side bug pre-fix)'); + } + + browser.dispose(); + agent.dispose(); +} + +// --------------------------------------------------------------------------- +// Scenario 2: WRITE / RESOLVE — agent resolves, Y.Array reflects it +// --------------------------------------------------------------------------- + +async function scenarioWriteSideResolve() { + console.log('\n=== Scenario 2: WRITE — agent resolves comment, browser sees it ==='); + const ydoc = new YDoc(); + + const browser = await openDocument(undefined, io, { + documentId: 'sd-3214-writeside', + ydoc, + collaborationProvider: providerStub() as never, + isNewFile: true, + user: { name: 'Browser User', email: 'browser@example.com' }, + }); + browser.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'A clause to be resolved.' }); + const block = browser.editor.doc.query.match({ + select: { type: 'text', pattern: 'resolved' }, + require: 'first', + }).items[0]!.blocks[0]!; + browser.editor.doc.comments.create({ + target: { kind: 'text', blockId: block.blockId, range: block.range } as never, + text: 'Resolve me.', + }); + const targetId = (ydoc.getArray('comments').toJSON() as Array>)[0]!.commentId as string; + + const agent = await openDocument(undefined, io, { + documentId: 'sd-3214-writeside', + ydoc, + collaborationProvider: providerStub() as never, + isNewFile: false, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + const patch = agent.editor.doc.comments.patch({ commentId: targetId, status: 'resolved' }); + console.log(` agent.comments.patch({status:'resolved'}).success === ${patch.success}`); + + const yEntry = (ydoc.getArray('comments').toJSON() as Array>)[0]!; + console.log({ + commentId: yEntry.commentId, + isDone: yEntry.isDone, + resolvedTime: yEntry.resolvedTime, + }); + + if (yEntry.isDone === true && typeof yEntry.resolvedTime === 'number') { + console.log(' WRITE ✓ — resolve propagated to Y.Array'); + } else { + console.log(' WRITE ✗ — resolve did not reach Y.Array (this is the write-side gap pre-fix)'); + } + + browser.dispose(); + agent.dispose(); +} + +// --------------------------------------------------------------------------- +// Scenario 3: DELETE — agent deletes, Y.Array entry disappears +// --------------------------------------------------------------------------- + +async function scenarioDelete() { + console.log('\n=== Scenario 3: DELETE — agent deletes, Y.Array shrinks ==='); + const ydoc = new YDoc(); + + const browser = await openDocument(undefined, io, { + documentId: 'sd-3214-delete', + ydoc, + collaborationProvider: providerStub() as never, + isNewFile: true, + user: { name: 'Browser User', email: 'browser@example.com' }, + }); + browser.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'A clause to delete.' }); + const block = browser.editor.doc.query.match({ + select: { type: 'text', pattern: 'delete' }, + require: 'first', + }).items[0]!.blocks[0]!; + browser.editor.doc.comments.create({ + target: { kind: 'text', blockId: block.blockId, range: block.range } as never, + text: 'I will be deleted.', + }); + const targetId = (ydoc.getArray('comments').toJSON() as Array>)[0]!.commentId as string; + + const agent = await openDocument(undefined, io, { + documentId: 'sd-3214-delete', + ydoc, + collaborationProvider: providerStub() as never, + isNewFile: false, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + const del = agent.editor.doc.comments.delete({ commentId: targetId }); + console.log(` agent.comments.delete().success === ${del.success}`); + + const yArr = ydoc.getArray('comments').toJSON() as Array>; + console.log(` Y.Array length after delete: ${yArr.length}`); + + if (yArr.length === 0) { + console.log(' DELETE ✓ — Y.Array entry removed'); + } else { + console.log(' DELETE ✗ — Y.Array still has the entry (this is the write-side gap pre-fix)'); + } + + browser.dispose(); + agent.dispose(); +} + +// --------------------------------------------------------------------------- +// Run +// --------------------------------------------------------------------------- + +try { + await scenarioReadSide(); + await scenarioWriteSideResolve(); + await scenarioDelete(); + console.log('\nDone.'); + process.exit(0); +} catch (err) { + console.error('Repro crashed:', err); + process.exit(1); +} From 39322757f0f119f5dc932bd99dd74623aed9a40d Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Wed, 20 May 2026 09:06:36 -0300 Subject: [PATCH 4/7] chore(snapshots): allow syncCommentEntitiesFromCollaboration export (SD-3214) Regenerates the SD-3176 legacy-subpath snapshot to accept the new `syncCommentEntitiesFromCollaboration` helper that SD-3214 adds to `superdoc/super-editor`. The growth is intentional and reviewed under the SD-3175 path-as-contract umbrella. Regenerated with: node tests/consumer-typecheck/snapshot-superdoc-legacy-exports.mjs --write --- tests/consumer-typecheck/snapshots/superdoc-super-editor.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/consumer-typecheck/snapshots/superdoc-super-editor.txt b/tests/consumer-typecheck/snapshots/superdoc-super-editor.txt index dd2de09b8a..e7526544a7 100644 --- a/tests/consumer-typecheck/snapshots/superdoc-super-editor.txt +++ b/tests/consumer-typecheck/snapshots/superdoc-super-editor.txt @@ -200,4 +200,5 @@ resolveSelectionTarget resolveTrackedChangeInStory seedEditorStateToYDoc shallowEqual +syncCommentEntitiesFromCollaboration trackChangesHelpers From 23c2a11e5bddbed82d306e23f2b47efbd8d507d8 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Thu, 21 May 2026 13:05:53 -0300 Subject: [PATCH 5/7] fix(cli): track own Y.Array writes so remote deletes prune agent-authored comments (SD-3214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bridge previously early-returned the Y.Array observer on own-origin events, which meant `previousSyncedIds` never recorded ids the agent itself wrote. A subsequent remote delete then had no prior id to detect, so the entity-store metadata (text / creatorName / createdTime / …) stayed in doc.comments.list() even after the canonical Y.Array entry was gone. Re-syncs on every observer fire are idempotent for already-present entries; the only effective change is that the synced-id bookkeeping now includes agent-authored comments and the prune step can act on them. Surfaced by Codex review (P2). Regression test exercises agent.comments.create followed by a remote-origin yArray.delete and asserts the stale metadata is pruned. --- ...rowser-comment-metadata.regression.test.ts | 63 +++++++++++++++++++ apps/cli/src/lib/headless-comment-bridge.ts | 17 ++--- 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts b/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts index 518a6ab573..0df44fc460 100644 --- a/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts +++ b/apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts @@ -503,4 +503,67 @@ describe('SD-3214: two-client end-to-end', () => { expect(found, 'browser-authored comment after agent connect should reach agent').toBeDefined(); expect((found as { creatorName?: string }).creatorName).toBe('Browser User'); }); + + // Codex P2 — "Track own Y.Array writes before filtering": the bridge used + // to skip own-origin Y.Array events to avoid redundant work, but that also + // meant `previousSyncedIds` never learned about agent-authored comments. + // So a subsequent remote delete had no prior id to prune against, and the + // metadata stored at create time (text / creatorName / createdTime / …) + // would keep surfacing through doc.comments.list() even though the + // canonical Y.Array entry was gone. + // + // The fix updates `previousSyncedIds` on every observer fire, including + // own-origin ones. We assert the entity-store-resident metadata is pruned + // here; the PM anchor mark is local to this session and (correctly) + // outlives a Y.Array-only delete simulation, so we don't assert on + // list-membership — only on the stale-metadata symptom Codex described. + it('agent-authored entity-store metadata is pruned when a remote client deletes it from Y.Array', async () => { + const ydoc = new YDoc(); + const session = await openDocument(undefined, createIo(), { + documentId: 'sd-3214-own-write-then-remote-delete', + ydoc, + collaborationProvider: createProviderStub(), + isNewFile: true, + user: { name: 'Headless Agent', email: 'agent@superdoc.dev' }, + }); + session.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'Agent-authored body.' }); + const matchBlock = session.editor.doc.query.match({ + select: { type: 'text', pattern: 'Agent-authored' }, + require: 'first', + }).items[0].blocks[0]; + session.editor.doc.comments.create({ + target: { kind: 'text', blockId: matchBlock!.blockId, range: matchBlock!.range } as never, + text: 'agent says review this', + }); + + const yArr = ydoc.getArray>('comments'); + const seeded = yArr.toJSON() as Array>; + expect(seeded).toHaveLength(1); + const targetId = seeded[0].commentId as string; + + const before = session.editor.doc.comments.list().items.find((c) => c.id === targetId); + expect(before, 'comment should be visible before remote delete').toBeDefined(); + expect(before?.text, 'agent-authored text should be present pre-delete').toBe('agent says review this'); + expect(before?.creatorName).toBe('Headless Agent'); + + // Remote client (different user origin) deletes the Y.Array entry. + ydoc.transact( + () => { + const idx = (yArr.toJSON() as Array>).findIndex((c) => c.commentId === targetId); + yArr.delete(idx, 1); + }, + { user: { name: 'Other Browser', email: 'other@example.com' } }, + ); + + const after = session.editor.doc.comments.list().items.find((c) => c.id === targetId); + session.dispose(); + + // The entity-store record carrying the rich metadata must be pruned. PM + // anchor presence is a separate concern; this test scopes to the + // metadata symptom Codex flagged ("stale local store entry"). + expect(after?.text, 'stale agent-authored text must be pruned after remote delete').toBeUndefined(); + expect(after?.creatorName, 'stale creatorName must be pruned after remote delete').toBeUndefined(); + expect(after?.creatorEmail, 'stale creatorEmail must be pruned after remote delete').toBeUndefined(); + expect(after?.createdTime, 'stale createdTime must be pruned after remote delete').toBeUndefined(); + }); }); diff --git a/apps/cli/src/lib/headless-comment-bridge.ts b/apps/cli/src/lib/headless-comment-bridge.ts index 2f80b7fcc3..247870e647 100644 --- a/apps/cli/src/lib/headless-comment-bridge.ts +++ b/apps/cli/src/lib/headless-comment-bridge.ts @@ -330,13 +330,16 @@ export function buildHeadlessCommentBridge(ydoc: unknown, user?: UserIdentity): // Initial seed: pull whatever is already in the room. syncYArrayToStore(); - yArrayObserver = (event) => { - // Skip our own writes — they're already in the store via onCommentsUpdate. - const origin = (event.transaction.origin ?? null) as { user?: { name?: string; email?: string } } | null; - const originUser = origin?.user; - if (userOrigin && originUser && originUser.name === userOrigin.name && originUser.email === userOrigin.email) { - return; - } + yArrayObserver = () => { + // Re-sync on every Y.Array event, including own-origin writes. For own + // writes the store is already coherent (the wrapper's `commentsUpdate` + // emit pre-populates it before this observer fires), but the prune + // step relies on `previousSyncedIds` knowing every collab-synced id — + // including ids we authored ourselves — so a later remote delete of + // an agent-authored comment can be detected and cascaded. The sync + // is idempotent for entries already present, so iterating over our + // own writes is a no-op on store contents and only refreshes the + // synced-id bookkeeping. syncYArrayToStore(); }; yArray.observe(yArrayObserver); From 4fb0764b049f9dccebb00e75ef3bf2f1848d0d3c Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Thu, 21 May 2026 13:06:06 -0300 Subject: [PATCH 6/7] fix(super-editor): skip orphan-reply upserts in collab comment sync (SD-3214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Browser-side deleteYComment only removes the parent's Y.Array index — reply entries linger upstream until the browser flushes them. The headless sync would upsert those replies in the same pass that cascade-removed the parent via removeCommentEntityTree, and `seen` would still contain the reply id. On the next observer fire the reply was re-upserted as an orphan with no parent, surfacing in doc.comments.list(). Pre-pass collects every upstream id (commentId AND importedId), and the upsert loop skips entries whose parentCommentId is not in that set. parentCommentId may reference parent.importedId for DOCX-imported threads, so both are tracked. Surfaced by Codex review (P2). Unit tests cover the orphan-reply scenarios plus sanity coverage that valid threads + legacy DOCX importedId references still pass through. --- .../helpers/comment-entity-store.test.ts | 98 +++++++++++++++++++ .../helpers/comment-entity-store.ts | 24 +++++ 2 files changed, 122 insertions(+) diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts index 2ccadb8cef..fcab399407 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts @@ -434,4 +434,102 @@ describe('syncCommentEntitiesFromCollaboration (SD-3214)', () => { expect(second).toEqual(new Set(['a', 'b', 'c'])); expect(getCommentEntityStore(editor)).toHaveLength(3); }); + + // Codex P2 — "Keep deleted thread descendants out of the sync set": + // packages/superdoc/.../collaboration-comments.js#deleteYComment removes + // only the parent index from Y.Array. The browser UI drops replies + // locally. If our helper iterates the upstream array AFTER the browser + // delete, it would still see the reply entries and upsert them. Even + // though removeCommentEntityTree cascades the parent's deletion through + // the store, the returned `seen` set would still contain the reply id + // because the reply was upserted in that same pass — so the next sync + // would re-upsert the reply as an orphan with no parent. + describe('orphaned-reply handling (Codex P2)', () => { + it('does not upsert a reply whose parent disappeared from the upstream array', () => { + const editor = makeEditorWithConverter(); + // Initial sync — parent and reply both upstream. + const first = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'root', commentText: 'parent body' }, + { commentId: 'reply-1', parentCommentId: 'root', commentText: 'reply body' }, + ]); + expect( + getCommentEntityStore(editor) + .map((e) => e.commentId) + .sort(), + ).toEqual(['reply-1', 'root']); + + // Browser deletes parent only; reply still in upstream. + const second = syncCommentEntitiesFromCollaboration( + editor, + [{ commentId: 'reply-1', parentCommentId: 'root', commentText: 'reply body' }], + { previouslySynced: first }, + ); + expect( + getCommentEntityStore(editor) + .map((e) => e.commentId) + .sort(), + 'parent + reply must both be pruned after parent deletion', + ).toEqual([]); + // And the returned sync set must NOT include the orphan reply, otherwise + // the next observer fire would re-upsert it as a parent-less orphan. + expect(second.has('reply-1'), 'reply id must not survive in the next sync set').toBe(false); + }); + + it('does not resurrect a reply as an orphan on a subsequent sync over the same upstream', () => { + // Same scenario as above, but we now run THREE syncs back-to-back, all + // observing the same "parent missing, reply present" upstream. Without + // the fix, the second and third syncs would re-upsert the reply each + // time, leaving an orphan record in the store. + const editor = makeEditorWithConverter(); + const first = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'root' }, + { commentId: 'reply-1', parentCommentId: 'root' }, + ]); + + const second = syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'reply-1', parentCommentId: 'root' }], { + previouslySynced: first, + }); + + const third = syncCommentEntitiesFromCollaboration(editor, [{ commentId: 'reply-1', parentCommentId: 'root' }], { + previouslySynced: second, + }); + + expect(getCommentEntityStore(editor).map((e) => e.commentId)).toEqual([]); + expect(third.has('reply-1'), 'orphan reply must not appear in the third sync set').toBe(false); + }); + + it('still upserts a reply when its parent is in the same upstream pass (preserves valid threads)', () => { + // Sanity check that the orphan filter does NOT break the common case: + // parent + reply both upstream → both upserted. + const editor = makeEditorWithConverter(); + const seen = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'root' }, + { commentId: 'reply-1', parentCommentId: 'root' }, + { commentId: 'reply-2', parentCommentId: 'root' }, + ]); + expect( + getCommentEntityStore(editor) + .map((e) => e.commentId) + .sort(), + ).toEqual(['reply-1', 'reply-2', 'root']); + expect(seen).toEqual(new Set(['root', 'reply-1', 'reply-2'])); + }); + + it('treats importedId as a valid parent reference (legacy DOCX threads)', () => { + // Imported comments may carry only `importedId`; a reply's + // `parentCommentId` can point at the parent's importedId rather than + // its canonical commentId. + const editor = makeEditorWithConverter(); + const seen = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'canonical-root', importedId: '0' }, + { commentId: 'reply-1', parentCommentId: '0' }, + ]); + expect( + getCommentEntityStore(editor) + .map((e) => e.commentId) + .sort(), + ).toEqual(['canonical-root', 'reply-1']); + expect(seen.has('reply-1'), 'reply pointing at parent.importedId must still be accepted').toBe(true); + }); + }); }); diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts index 778ed125a2..b93a662115 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts @@ -214,12 +214,36 @@ export function syncCommentEntitiesFromCollaboration( const store = getCommentEntityStore(editor); const seen = new Set(); + // Pre-pass: collect every id (commentId AND importedId) that exists in the + // current upstream array. `deleteYComment` on the browser side removes only + // the parent index from Y.Array — reply entries linger upstream until the + // browser flushes them. Without this set we'd happily upsert those replies + // even though `removeCommentEntityTree` is about to cascade-delete them, + // and the next observer fire would resurrect them as orphans (Codex P2). + const upstreamIds = new Set(); + for (const raw of entries) { + if (!raw || typeof raw !== 'object') continue; + if (raw.trackedChange === true) continue; + const cid = toNonEmptyString(raw.commentId); + const iid = toNonEmptyString(raw.importedId); + if (cid) upstreamIds.add(cid); + if (iid) upstreamIds.add(iid); + } + for (const raw of entries) { if (!raw || typeof raw !== 'object') continue; if (raw.trackedChange === true) continue; const commentId = toNonEmptyString(raw.commentId) ?? toNonEmptyString(raw.importedId); if (!commentId) continue; + + // Skip orphan replies — entries whose parent no longer exists upstream. + // `parentCommentId` may reference the parent's `commentId` OR its + // `importedId` (DOCX-imported threads), hence checking against the + // combined set built above. + const parentRef = toNonEmptyString(raw.parentCommentId); + if (parentRef && !upstreamIds.has(parentRef)) continue; + seen.add(commentId); const patch: Partial = {}; From 948066bd8d631b8f2b12181e649d5c1fe405afaa Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Fri, 22 May 2026 09:24:43 -0300 Subject: [PATCH 7/7] fix(super-editor): collapse orphan reply chains transitively in collab comment sync (SD-3214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous single-pass filter handled A→B (B skipped when A is gone) but broke on A→B→C: B was skipped, yet its id stayed in the upstream set, so C survived the upsert and dangled as an orphan whose chain led nowhere. On the next observer fire C resurfaced in doc.comments.list(). Pre-pass now iteratively drops orphan ids from the upstream set until stable. Each iteration removes entries whose declared parent is no longer in the set; the next iteration re-evaluates entries transitively orphaned by the previous removal. Worst-case O(depth × entries), bounded by Y.Array size (small in practice). Surfaced by Codex follow-up review. Unit tests cover A→B→C delete-A and the multi-sync resurrection variant. --- .../helpers/comment-entity-store.test.ts | 69 +++++++++++++++++++ .../helpers/comment-entity-store.ts | 48 +++++++++---- 2 files changed, 103 insertions(+), 14 deletions(-) diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts index fcab399407..144c24ec97 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.test.ts @@ -531,5 +531,74 @@ describe('syncCommentEntitiesFromCollaboration (SD-3214)', () => { ).toEqual(['canonical-root', 'reply-1']); expect(seen.has('reply-1'), 'reply pointing at parent.importedId must still be accepted').toBe(true); }); + + // Codex follow-up: a one-shot orphan filter (build upstreamIds once, + // skip entries whose direct parent is missing) handles A→B but breaks on + // A→B→C. B is correctly skipped because A is gone, but C's parent B is + // still present in `upstreamIds`, so C survives the upsert and dangles + // as an orphan whose chain leads nowhere. Filter must be applied + // transitively until the upstream set is stable. + it('drops the entire orphan chain when an ancestor is missing upstream (A→B→C, A deleted)', () => { + const editor = makeEditorWithConverter(); + const first = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'A' }, + { commentId: 'B', parentCommentId: 'A' }, + { commentId: 'C', parentCommentId: 'B' }, + ]); + expect( + getCommentEntityStore(editor) + .map((e) => e.commentId) + .sort(), + ).toEqual(['A', 'B', 'C']); + + // A is deleted upstream. B and C linger (browser only flushed A). + const second = syncCommentEntitiesFromCollaboration( + editor, + [ + { commentId: 'B', parentCommentId: 'A' }, + { commentId: 'C', parentCommentId: 'B' }, + ], + { previouslySynced: first }, + ); + expect( + getCommentEntityStore(editor) + .map((e) => e.commentId) + .sort(), + 'A → B → C: with A gone, B and C must both be pruned', + ).toEqual([]); + expect(second.has('B'), 'B must not appear in the sync set').toBe(false); + expect(second.has('C'), 'C must not appear in the sync set (or it would be re-upserted)').toBe(false); + }); + + it('does not resurrect a grandchild orphan on subsequent syncs over the same upstream (A→B→C)', () => { + const editor = makeEditorWithConverter(); + const first = syncCommentEntitiesFromCollaboration(editor, [ + { commentId: 'A' }, + { commentId: 'B', parentCommentId: 'A' }, + { commentId: 'C', parentCommentId: 'B' }, + ]); + + const second = syncCommentEntitiesFromCollaboration( + editor, + [ + { commentId: 'B', parentCommentId: 'A' }, + { commentId: 'C', parentCommentId: 'B' }, + ], + { previouslySynced: first }, + ); + + const third = syncCommentEntitiesFromCollaboration( + editor, + [ + { commentId: 'B', parentCommentId: 'A' }, + { commentId: 'C', parentCommentId: 'B' }, + ], + { previouslySynced: second }, + ); + + expect(getCommentEntityStore(editor).map((e) => e.commentId)).toEqual([]); + expect(third.has('B')).toBe(false); + expect(third.has('C')).toBe(false); + }); }); }); diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts index b93a662115..cd6d1fdfdf 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts @@ -215,12 +215,17 @@ export function syncCommentEntitiesFromCollaboration( const seen = new Set(); // Pre-pass: collect every id (commentId AND importedId) that exists in the - // current upstream array. `deleteYComment` on the browser side removes only - // the parent index from Y.Array — reply entries linger upstream until the - // browser flushes them. Without this set we'd happily upsert those replies - // even though `removeCommentEntityTree` is about to cascade-delete them, - // and the next observer fire would resurrect them as orphans (Codex P2). + // current upstream array, then transitively drop entries whose + // `parentCommentId` is missing from the set. `deleteYComment` on the + // browser side removes only the parent index from Y.Array — replies (and + // replies-of-replies) linger upstream until the browser flushes them. A + // single-pass filter handles A→B (B skipped when A is gone) but breaks on + // A→B→C: B would be skipped, yet B's id is still in `upstreamIds`, so C + // survives and dangles as an orphan whose chain leads nowhere. The + // fixed-point loop below removes orphan ids from the set until stable, so + // any depth of orphan chain collapses in one go. const upstreamIds = new Set(); + const validEntries: Array> = []; for (const raw of entries) { if (!raw || typeof raw !== 'object') continue; if (raw.trackedChange === true) continue; @@ -228,21 +233,36 @@ export function syncCommentEntitiesFromCollaboration( const iid = toNonEmptyString(raw.importedId); if (cid) upstreamIds.add(cid); if (iid) upstreamIds.add(iid); + validEntries.push(raw); } - for (const raw of entries) { - if (!raw || typeof raw !== 'object') continue; - if (raw.trackedChange === true) continue; + // Iteratively drop orphan ids until the set is stable. Each pass removes + // entries whose declared parent is no longer represented in `upstreamIds`; + // the next pass then re-evaluates entries that were transitively orphaned + // by the previous removal. Worst-case cost is O(depth × validEntries), + // bounded by document size — Y.Array of comments is small in practice. + let changed = true; + while (changed) { + changed = false; + for (const raw of validEntries) { + const parentRef = toNonEmptyString(raw.parentCommentId); + if (!parentRef) continue; + if (upstreamIds.has(parentRef)) continue; + const cid = toNonEmptyString(raw.commentId); + const iid = toNonEmptyString(raw.importedId); + if (cid && upstreamIds.delete(cid)) changed = true; + if (iid && upstreamIds.delete(iid)) changed = true; + } + } + for (const raw of validEntries) { const commentId = toNonEmptyString(raw.commentId) ?? toNonEmptyString(raw.importedId); if (!commentId) continue; - // Skip orphan replies — entries whose parent no longer exists upstream. - // `parentCommentId` may reference the parent's `commentId` OR its - // `importedId` (DOCX-imported threads), hence checking against the - // combined set built above. - const parentRef = toNonEmptyString(raw.parentCommentId); - if (parentRef && !upstreamIds.has(parentRef)) continue; + // After the fixed-point pass, an entry is an orphan iff its own id was + // dropped from `upstreamIds`. Skip it so the prune step can cascade- + // delete the local record without `seen` re-marking the orphan as live. + if (!upstreamIds.has(commentId)) continue; seen.add(commentId);