Skip to content

feat(collaboration): emit comments-update event for remotely-added comments#3079

Closed
mattConnHarbour wants to merge 2 commits into
mainfrom
SD-2914
Closed

feat(collaboration): emit comments-update event for remotely-added comments#3079
mattConnHarbour wants to merge 2 commits into
mainfrom
SD-2914

Conversation

@mattConnHarbour

Copy link
Copy Markdown
Contributor

No description provided.

@mattConnHarbour mattConnHarbour requested a review from a team as a code owner May 1, 2026 23:30
@linear

linear Bot commented May 1, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

📦 Preview published: superdoc@1.28.0-pr.3079.1777678388

npm install superdoc@pr-3079

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@caio-pizzol caio-pizzol self-assigned this May 5, 2026

@caio-pizzol caio-pizzol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mattConnHarbour, direction's right :) one bug inline plus a question about event type.

if (currentUser.name === user.name && currentUser.email === user.email) return;

// Capture existing comment IDs before loading new state
const existingIds = new Set(superdoc.commentsStore.commentsList?.map((c) => c.commentId || c.importedId) || []);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the loader and this code disagree on which id wins when a comment has both. loader (line 19) prefers importedId, here commentId wins. so if an imported comment later gets a commentId, or two people import the same docx and pick different commentIds, this code treats it as new and fires add for a comment that's already there. fix: match the loader at lines 104 and 112.

Suggested change
const existingIds = new Set(superdoc.commentsStore.commentsList?.map((c) => c.commentId || c.importedId) || []);
const existingIds = new Set(superdoc.commentsStore.commentsList?.map((c) => c.importedId ?? c.commentId) || []);

newComments.forEach((comment) => {
const commentValues = typeof comment.getValues === 'function' ? comment.getValues() : comment;
superdoc.emit('comments-update', {
type: comments_module_events.ADD,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments-store.js emits 'add', CommentsLayer.vue emits 'new' for UI creation. does the listener that scrolls to remote comments use 'add'? if it keys off 'new', it won't fire.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants