fix(cli): headless bridge syncs browser-authored comment metadata from Y.Array (SD-3214)

[tupizz]

Summary

Fixes SD-3214. Two commits in this PR:

1. Read-side: headless SDK's editor.doc.comments.list() now surfaces full metadata (text, creatorName, creatorEmail, createdTime) for browser-authored comments. Pre-fix it only had anchor-derived fields.
2. Write-side: CLI-initiated comments.delete / comments.patch({status:'resolved'}) / comments.patch({status:'active'}) now propagate to other collaborators via Y.Array. Without this, the customer's "agent resolves/responds to comments" workflow would have read correctly but written into a black hole.

Customer use case (Viven Puthenpurayil, via Lighthouse): an agent reads comments from the document, addresses them, then resolves/responds. Both halves of the workflow work end-to-end after this PR.

Root causes

Read-side gap (commit 1)

Browser and CLI share most of the core (Editor, comment mark, CommentEntityStore, Document API adapters). The per-environment orchestration was asymmetric:

        BROWSER                              CLI
   ┌──────────────────────┐          ┌──────────────────────┐
   │ initCollaboration-   │          │ headless-comment-    │
   │ Comments + loadCom-  │          │ bridge.ts            │
   │ mentsFromYdoc        │          │                      │
   │   read  ✓ / write ✓  │          │   read  ✗  ← bug     │
   │                      │          │   write ✓            │
   └──────────────────────┘          └──────────────────────┘
                            ↓ shared ↓
                CommentEntityStore + Document API
                Yjs + y-prosemirror substrate

The browser had both halves of the Y.Array dance. The CLI bridge had only the write half — so nothing pushed Y.Array entries into the editor's CommentEntityStore, which is what comments.list() reads from.

Write-side gap (commit 2)

The engine commands resolveComment, reopenComment, and removeComment set tr.setMeta(CommentsPluginKey, { event: ... }) but never call editor.emit('commentsUpdate', ...). So:

• The bridge's existing case 'deleted' | 'update' | 'resolved' handlers (which call deleteYComment / updateYComment) were unreachable from CLI-initiated mutations.
• The browser's ui.comments.* surface (which routes through editor.doc.comments.*) was silently broken for collab — fine for local Vue list, but Y.Array never updated.
• The browser-internal commentsStore.deleteComment path stays unaffected because it calls editor.commands.removeComment directly and manually handles Y.Array sync via syncCommentsToClients.

Fix

Commit 1: read-side

• packages/super-editor: new shared helper syncCommentEntitiesFromCollaboration(editor, entries, { previouslySynced? }) in comment-entity-store.ts. Maps Y.Array entry shape to CommentEntityRecord, upserts via upsertCommentEntity, filters out trackedChange: true entries (different domain), and prunes remote deletions when given the prior sync set (cascades thread replies via removeCommentEntityTree).
• apps/cli: bridge gains attachEditor(editor) called after Editor.open(). Seeds the store from the current Y.Array, then installs an observer that re-syncs on remote events while skipping own-origin echoes using the transaction.origin.user shape browser writers use. Tracks previousSyncedIds across observer fires for prune correctness.

Commit 2: write-side

• packages/super-editor: resolveCommentHandler, reopenCommentHandler, and removeCommentHandler in comments-wrappers.ts now emit commentsUpdate after a successful mutation. Symmetric with how addCommentHandler and editCommentHandler already rely on engine-level emits.

CLI engine-agnostic boundary preserved: the bridge forwards an opaque editor handle to the 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.

Audit notes (why the write-side fix went in the wrapper, not the engine command)

Initial scoping put the write-side gap as a follow-up because changing engine commands risks double-fire in the browser. The audit found:

• Browser path A: ui.comments.delete → editor.doc.comments.delete → wrapper handler → engine command. No emit anywhere. Vue list silently goes stale; Y.Array unchanged.
• Browser path B: commentsStore.deleteComment (sidebar trash click, etc.) → engine command directly. Manual superdoc.emit + syncCommentsToClients. Bypasses the wrapper entirely.

Emitting from the wrapper (this PR) fixes path A for both browser and CLI without affecting path B. Engine commands stay untouched — no double-fire risk.

How to reproduce the original bugs

Read-side (pre-fix, fails; post-fix, passes)

const ydoc = new YDoc();
const yComment = new YMap(Object.entries({
  commentId: 'c1',
  commentText: 'Please review.',
  creatorName: 'Browser User',
  creatorEmail: 'browser@example.com',
  createdTime: 1700000000000,
}));
ydoc.transact(() => { ydoc.getArray('comments').push([yComment]); }, { user: { name: 'Browser User', email: 'browser@example.com' } });

const opened = await openDocument(undefined, io, { ydoc, collaborationProvider, user, isNewFile: true });
const list = opened.editor.doc.comments.list();
// PRE-FIX:  list.items.find(c => c.id === 'c1') === undefined
// POST-FIX: { text: 'Please review.', creatorName: 'Browser User', ... }

Write-side (pre-fix, fails; post-fix, passes)

const session = await openDocument(undefined, io, { ydoc, collaborationProvider, user: { name: 'Agent', email: 'agent@x' }, isNewFile: true });
session.editor.doc.create.paragraph({ at: { kind: 'documentEnd' }, text: 'Clause.' });
const block = session.editor.doc.query.match({ select: { type: 'text', pattern: 'Clause' }, require: 'first' }).items[0].blocks[0];
session.editor.doc.comments.create({ target: { kind: 'text', blockId: block.blockId, range: block.range }, text: 'pending' });

const id = (ydoc.getArray('comments').toJSON())[0].commentId;
session.editor.doc.comments.patch({ commentId: id, status: 'resolved' });

const after = ydoc.getArray('comments').toJSON()[0];
// PRE-FIX:  after.isDone === undefined, after.resolvedTime === undefined
// POST-FIX: after.isDone === true,      after.resolvedTime === <number>

Test plan

Unit (13 new cases in comment-entity-store.test.ts)

• Upsert paths: new entry, text alias for commentText, importedId fallback, missing id skip.
• Merge-without-clobber.
• Tracked-change filter.
• Resolution metadata propagation.
• Deletion path: prune previously-synced, leave locally-authored alone, cascade thread replies, no-op on no removals.

Integration (10 cases in sd-3214-browser-comment-metadata.regression.test.ts)

• Pre-open Y.Array entry; post-open observe; remote update.
• Option A (shared Y.Doc): two-Editor scenario; full metadata propagation; origin filter (no self-echo); remote delete prunes.
• Option B (two Y.Docs + Yjs update relay): closer to wire-protocol reality; late-join author.
• CLI write-side delete: agent comments.delete → Y.Array reflects removal.
• CLI write-side resolve: agent comments.patch({status:'resolved'}) → Y.Array gets isDone: true + resolvedTime.

Suites

• pnpm --filter @superdoc-dev/cli test — 1248 pass, 0 fail, 8269 expect(), 52 files.
• pnpm --filter super-editor test — 1035 files / 13117 pass / 13 skip.
• Existing headless-comment-bridge.test.ts (27 cases) untouched and green.
• Existing comments-wrappers.test.ts green — no double-emit regressions.

Acceptance criteria (from ticket)

• doc.comments.list() returns full metadata for browser-authored comments.
• Initial sync covers pre-existing Y.Array entries.
• Observer picks up entries authored after connect.
• Remote deletions prune the headless store.
• Agent resolve/delete/reopen propagate to other collaborators via Y.Array.
• Existing browser comment flow untouched (super-editor + bridge suites green).
• Regression tests prevent re-introduction.

Files changed

File	Role
packages/super-editor/.../helpers/comment-entity-store.ts	New syncCommentEntitiesFromCollaboration helper.
packages/super-editor/.../plan-engine/comments-wrappers.ts	Wrapper handlers emit commentsUpdate after successful mutation.
packages/super-editor/src/editors/v1/index.js	Re-export the helper.
apps/cli/src/lib/headless-comment-bridge.ts	New attachEditor, Y.Array observer, prune-aware re-sync.
apps/cli/src/lib/document.ts	Wire commentBridge.attachEditor(editor) after Editor.open.
apps/cli/src/__tests__/lib/_collab-password-worker.ts	Add the new export to the subprocess mock.
apps/cli/src/lib/__tests__/sd-3214-browser-comment-metadata.regression.test.ts	New 10-case integration suite.
packages/super-editor/.../helpers/comment-entity-store.test.ts	13 new unit cases.

Related

• Customer ask: Viven Puthenpurayil (Lighthouse) — agent workflow over user comments.
• Blocks: IT-1066 (Investigate reading browser comments via headless SDK client).

[linear-code]

SD-3214

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a1210c6633

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[luccas-harbour]

hey @tupizz!
I had a look at this one and codex found these issues below. Can you have a look? Other than that, everything looks good.

• [P2] Track own Y.Array writes before filtering — apps/cli/src/lib/headless-comment-bridge.ts:337-338
  When this client creates a comment after attachEditor, the Y.Array observer returns for the matching userOrigin, so previousSyncedIds never learns about that comment even though it was written to collaboration. If another client later deletes that headless-authored comment, syncCommentEntitiesFromCollaboration(..., { previouslySynced: previousSyncedIds }) has no prior id to prune, and doc.comments.list() can keep returning the stale local store entry.

• [P2] Keep deleted thread descendants out of the sync set — packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts:262-262
  When a browser deletes a parent thread, its existing collaboration helper removes only the parent Y.Array entry while the browser UI also drops replies locally. This sync pass prunes the reply via removeCommentEntityTree, but then returns seen, which still contains the reply because it remained in the upstream array; on the next Y.Array event that reply is considered valid again and is upserted as an orphan in doc.comments.list().

[tupizz]

Both Codex P2s addressed — verified each against the codebase + browser-side prior art before touching code, then TDD per fix.

P2 #1 — Track own Y.Array writes (23c2a11)
Confirmed the bug: with the own-origin early-return in place, previousSyncedIds never recorded ids the agent itself wrote, so a remote delete of an agent-authored comment left the entity-store metadata (text / creatorName / createdTime / …) stale in doc.comments.list(). Fix drops the early-return — syncCommentEntitiesFromCollaboration is idempotent for already-present entries, so the only effective change is that the synced-id bookkeeping now includes agent-authored comments and the prune step acts on them. New regression test in sd-3214-browser-comment-metadata.regression.test.ts exercises agent.comments.create followed by a remote-origin yArray.delete and asserts the stale metadata is pruned.

P2 #2 — Keep deleted thread descendants out of the sync set (4fb0764)
Confirmed against packages/superdoc/src/core/collaboration/collaboration-comments.js:30 — deleteYComment removes only the parent's Y.Array index; replies linger upstream. Without the fix our sync would upsert those replies in the same pass that cascade-removed the parent, seen would still contain the reply id, and the next observer fire would resurrect it as an orphan in doc.comments.list(). Fix adds a pre-pass to collect every upstream id (commentId AND importedId — parentCommentId may reference the parent's importedId for DOCX-imported threads) and skips entries whose parentCommentId is not in that set. Unit tests cover both orphan scenarios, plus sanity coverage that valid threads + legacy DOCX importedId references still pass through.

Suites (re-run after each fix):

• pnpm --filter @superdoc-dev/cli test — 1249 pass / 0 fail / 8277 expect(), 52 files (+1 case vs prior run).
• pnpm --filter super-editor test — 1035 files / 13121 pass / 13 skipped (+3 cases vs prior run).

Note on the Codex P1 (inline) on comment-entity-store.ts:223 about importedId ?? commentId dedupe: not addressed in these commits — flagging here for visibility. I'm happy to roll it in next if you want it before merge.

[luccas-harbour]

hey @tupizz! thanks for addressing the issues that codex had found

it looks like there's still one remaining, related to the second P2 that codex had flagged:

packages/super-editor/src/editors/v1/document-api-adapters/helpers/comment-entity-store.ts:223-245 — multi-level orphan reply gets resurrected
Verdict: real_issue (narrow)
Reason: upstreamIds is built once from every surviving entry. For A→B→C where A is deleted upstream but B/C linger, B is skipped (parent A missing) but C is upserted because B is still in upstreamIds. C lingers as a dangling orphan after the next observer fire.

can you check this one?

[tupizz]

Addressed in 948066b — Codex's follow-up was a real chain bug, not a false positive.

The hole in the previous fix
upstreamIds was built once and never updated as orphans were detected. For A → B → C where A is deleted upstream but B and C linger: B is correctly skipped (parent A missing), but C's check upstreamIds.has('B') is true, so C survives and dangles as an orphan whose chain leads nowhere. On the next observer fire doc.comments.list() re-surfaces C.

Fix
Pre-pass keeps iterating over the valid entries and removes orphan ids from upstreamIds until the set stops changing. Each iteration removes entries whose declared parent is no longer in the set; the next iteration re-evaluates entries transitively orphaned by that removal. After fixed-point, an entry is an orphan iff its own id was dropped from the set, so the upsert loop's gate is a single membership check.

Worst-case O(depth × validEntries), but Y.Array of comments is small in practice — the depth term is the thread nesting depth, typically 1–3.

Coverage
Two new unit cases in comment-entity-store.test.ts:

• drops the entire orphan chain when an ancestor is missing upstream (A→B→C, A deleted) — assert store ends empty and seen excludes both B and C.
• does not resurrect a grandchild orphan on subsequent syncs over the same upstream (A→B→C) — runs three sync passes back-to-back over the same orphaned state to catch resurrection-on-next-observer.

The existing parent-with-direct-reply case (A→B) and the legacy-importedId case still pass — fixed-point reduces to single-pass when there's no chain.

Suites (re-run after fix + post-rebase against the latest tip of the branch):

• pnpm --filter super-editor test — 1036 files / 13136 pass / 13 skipped (+2 new cases).
• pnpm --filter @superdoc-dev/cli test — 1249 pass / 0 fail / 8277 expect(), 52 files (unchanged).

PTAL.

[luccas-harbour]

LGTM

[superdoc-bot]

🎉 This PR is included in superdoc-cli v0.12.0

The release is available on GitHub release

[superdoc-bot]

🎉 This PR is included in superdoc-sdk v1.11.0

[superdoc-bot]

🎉 This PR is included in @superdoc-dev/mcp v0.7.0

The release is available on GitHub release

[superdoc-bot]

🎉 This PR is included in superdoc v1.35.0

The release is available on GitHub release

[superdoc-bot]

🎉 This PR is included in @superdoc-dev/react v1.6.0

The release is available on GitHub release

[superdoc-bot]

🎉 This PR is included in vscode-ext v2.7.0