fix(comments): prevent flicker when clicking comment with tracked changes#2986
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @gpardhivvarma! good call going after this, but the live demo still flickered with the original guard.
clicking a tracked-change comment fires two selection changes back to back: cursor first, then a range over the whole tracked span. your guard only kicks in for a cursor, so the range part still cleared the active comment and the host kept re-setting it.
i pushed a one-line change to your branch (range selections no longer count as "user moved off"), plus unit and behavior tests. flicker went from ~700 class flips per 2s to 2.
left two inline.
lgtm once you decide on the flag.
|
Both comments addressed:
Ready for re-review. |
…nges Add explicitlySetThreadId flag to CommentsPlugin state to prevent position-based active comment detection from overriding sidebar-initiated comment activation. When a comment is explicitly activated (e.g., sidebar click), subsequent DOM-sync selection transactions preserve the active comment as long as the cursor remains within its range. The flag clears naturally when the cursor moves outside the comment range or when the document is edited, allowing position-based detection to resume for normal user navigation. Closes superdoc-dev#2861
…superdoc-dev#2861) getActiveCommentId returns undefined for any non-collapsed selection. The plugin coerced that into commentsUpdate({activeCommentId: null}), which the host listener echoed back as a fresh setActiveComment dispatch. For tracked changes inside an inline SDT, presentation.navigateTo dispatches a collapsed cursor placement followed by a non-collapsed NodeSelection on the SDT wrapper, so the second tx blanked the just-activated comment and triggered a flicker loop (~400 toggles/sec on .track-change-focused). Skip the emit when getActiveCommentId returned no information for a non-collapsed selection. A non-collapsed selection is not a reliable signal that the user moved off the comment.
Four tests pinning the new contract: - preserves activeThreadId when a non-collapsed selection follows explicit activation - preserves activeThreadId when a follow-up non-collapsed selection sits entirely within the comment range (the WYSIWYG repro shape) - still clears activeThreadId when a collapsed cursor moves outside the comment range (existing behavior preserved) - does not enter a dispatch loop when a host listener re-asserts after each commentsUpdate (models the SuperDoc.vue store round-trip) Three of the four fail without the plugin fix.
…uperdoc-dev#2861) Programmatically reproduces the two-transaction pattern from presentation.navigateTo at the live editor level: setActiveComment on a comment-marked range, then a non-collapsed selection over the same range. Counts dispatches and class toggles on .track-change-focused. Without the plugin fix, dispatchCount climbs into the hundreds within a second as the host re-asserts on every commentsUpdate emit. With the fix, both counts stay bounded and the active comment survives.
Address review feedback — avoid mutating pluginState directly before spreading. Use conditional copy pattern consistent with Caio's suggestion.
Drop the flag in favor of Caio's isNonCollapsedClear guard, which handles the actual flicker trigger (range selection from SDT wrapper). Keeps the dead code removal and condition simplification from the original commit.
gpardhivvarma
force-pushed
the
fix/comment-flicker-tracked-changes
branch
from
5f6b8d9 to
d6463a2
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @gpardhivvarma! thanks for cleaning that up.
re-ran the click test, no flicker. lgtm.
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.5 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.52 |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.50 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.30.0-next.10 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.25 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.10 |
|
🎉 This PR is included in superdoc v1.30.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.8.0 |
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0 |
|
🎉 This PR is included in @superdoc-dev/react v1.3.0 The release is available on GitHub release |
3 participants
Summary
Closes #2861
Fixes rapid flickering when clicking a comment that contains a tracked change inside inline structured content.
Root Cause
When a comment is activated via sidebar click,
comments-plugin.jscorrectly setsactiveThreadIdand returns early. However, the resulting DOM update triggers a ProseMirror selection-sync transaction. This secondary transaction hits position-based active comment detection (getActiveCommentId), which fails to resolve the comment inside tracked-change/structured-content nodes and returnsnull— clearing the active comment, triggering another DOM rebuild, and repeating in a loop.Fix
Add
explicitlySetThreadIdflag to CommentsPlugin state. When a comment is activated via explicit action (setActiveCommentmeta), the flag is set. On subsequent selection-only transactions:This extends the same principle already used at line 426-428 where
setCursorByIdskipsview.focus()to prevent DOM selection sync from overriding the active thread.Also removes dead code:
trChangedActiveCommentvariable was alwaysfalseat the detection block becausesetActiveCommentreturns early at line 488.Changes
comments-plugin.jsexplicitlySetThreadIdto plugin state, guard position-based detection, clear on cursor-out and doc-change, remove dead codecomments-plugin.test.jsTests
Test plan
comments-plugin.test.js— 66/66 passing