fix(collaboration): sync comments to second collab client on export#3593
fix(collaboration): sync comments to second collab client on export#3593mattConnHarbour wants to merge 2 commits into
Conversation
Second collaboration client was missing comment metadata (text, author) because comments were not being pushed to Y.Array before the client exported. Three fixes: CLI: - Set shouldLoadComments: true so #initComments() emits commentsLoaded - Fix isNewFile logic: when seeding from a document, isNewFile must be false so #initComments() runs immediately (not deferred) Browser: - Queue comment events arriving before collaboration is ready - Flush queued events when provider syncs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63f5d60069
ℹ️ 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".
| isNewFile: shouldSeed, | ||
| // When seeding from a document, we need isNewFile: false so that | ||
| // #initComments() runs and emits commentsLoaded, pushing comments to Y.Array. | ||
| isNewFile: shouldSeed && !docForEditor, |
There was a problem hiding this comment.
Keep isNewFile true when seeding from a doc
When decision.action === 'seed' and a document path is provided, docForEditor is the same truthy doc, so this now passes isNewFile: false for the normal seed-from-file path. The editor's collaboration bootstrap only inserts the parsed document into the shared Yjs fragment when options.isNewFile is true (initializeCollaborationData() returns early otherwise), so an empty room seeded from a real DOCX can be marked bootstrapped without actually publishing the document content to other clients.
Useful? React with 👍 / 👎.
| * Queue for comment events that arrive before collaboration is ready. | ||
| * Flushed when provider syncs via flushPendingCommentEvents(). | ||
| */ | ||
| let pendingCommentEvents = []; |
There was a problem hiding this comment.
Scope queued comment events per SuperDoc instance
This module-level queue is shared by every SuperDoc instance loaded in the page/process. If one non-collaborative or not-yet-collaborative editor emits a comment event, then another editor later reaches provider sync, flushPendingCommentEvents(superdoc) replays all queued events into that other editor's Y.Array, leaking or corrupting comments across documents; the queued events need to be tied to the originating instance or discarded when that instance is not actually being upgraded to collaboration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
cubic analysis
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/superdoc/src/core/collaboration/helpers.js">
<violation number="1" location="packages/superdoc/src/core/collaboration/helpers.js:11">
P1: Module-level `pendingCommentEvents` queue is shared across all SuperDoc instances. If multiple instances exist (e.g., multi-document workspace, or CLI processing multiple files), events queued by one instance will be flushed into a different instance's Y.Array, corrupting comment sync. The queue should be scoped per SuperDoc instance (e.g., stored on `superdoc` itself or in a WeakMap keyed by instance).</violation>
</file>
Linked issue analysis
Linked issue: SD-3338: Bug: Collaborative comment export fails for second client - Y.Array not seeded
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Queue comment events that arrive before collaboration is ready so they aren't dropped | helpers.js introduces a pendingCommentEvents queue and syncCommentsToClients now pushes events to the queue when superdoc.isCollaborative is false. |
| ✅ | Flush queued comment events into the Y.Array when provider syncs (so Y.Array is seeded before loading comments) | helpers.js adds flushPendingCommentEvents(), calls it in the provider 'synced' handler and if provider is already synced, ensuring queued events are applied before loadCommentsFromYdoc runs. |
| ✅ | Ensure CLI opens editor so initComments runs when seeding from a document (isNewFile false) so commentsLoaded is emitted | apps/cli change adjusts isNewFile to be false when seeding from a document (shouldSeed && !docForEditor), which causes #initComments() to run immediately during open. |
| ✅ | Headless/CLI editorOptions set to load comments so commentsLoaded/onCommentsLoaded callbacks run | The headless comment bridge now sets shouldLoadComments: true in editorOptions (two places), which enables comment loading flow used to seed Y.Array/store. |
| ✅ | Overall outcome: second collaboration clients receive comment definitions (so exports have comment metadata) | The PR implements queuing of early events, flushing them on provider sync, and ensures comment-loading is triggered in CLI/headless paths, which together seed Y.Array before loadCommentsFromYdoc runs — addressing the root cause described in the issue. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| * Queue for comment events that arrive before collaboration is ready. | ||
| * Flushed when provider syncs via flushPendingCommentEvents(). | ||
| */ | ||
| let pendingCommentEvents = []; |
There was a problem hiding this comment.
P1: Module-level pendingCommentEvents queue is shared across all SuperDoc instances. If multiple instances exist (e.g., multi-document workspace, or CLI processing multiple files), events queued by one instance will be flushed into a different instance's Y.Array, corrupting comment sync. The queue should be scoped per SuperDoc instance (e.g., stored on superdoc itself or in a WeakMap keyed by instance).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/superdoc/src/core/collaboration/helpers.js, line 11:
<comment>Module-level `pendingCommentEvents` queue is shared across all SuperDoc instances. If multiple instances exist (e.g., multi-document workspace, or CLI processing multiple files), events queued by one instance will be flushed into a different instance's Y.Array, corrupting comment sync. The queue should be scoped per SuperDoc instance (e.g., stored on `superdoc` itself or in a WeakMap keyed by instance).</comment>
<file context>
@@ -4,6 +4,25 @@ import { actorIdentitiesMatch } from '@superdoc/common';
+ * Queue for comment events that arrive before collaboration is ready.
+ * Flushed when provider syncs via flushPendingCommentEvents().
+ */
+let pendingCommentEvents = [];
+
+/**
</file context>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Second collaboration client was missing comment metadata (text, author) because comments were not being pushed to Y.Array before the client exported.
CLI (SDK):
Browser: