Skip to content

[lexical-playground] Bug Fix: restore a deleted comment thread when its deletion is undone - #9171

Open
sahiee-dev wants to merge 1 commit into
facebook:mainfrom
sahiee-dev:fix/undo-restores-deleted-comment-mark
Open

sahiee-dev wants to merge 1 commit into
facebook:mainfrom
sahiee-dev:fix/undo-restores-deleted-comment-mark

Conversation

@sahiee-dev

@sahiee-dev sahiee-dev commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Description

Deleting a comment thread from the playground's comments panel removes it from CommentStore (plain JS state outside Lexical's EditorState) and, separately, strips the thread id from its MarkNode in an ordinary, undoable editor.update. Undo only reverts the second half: the MarkNode and its highlight reappear, but the thread was never actually restored, so the mark ends up referencing a deleted comment, and the comments panel also gets stuck popping open empty for that span.

This adds a small tombstone to CommentStore (capturing a deleted thread and its original position) and a MarkNode mutation listener, scoped to updates tagged HISTORIC_TAG, that reconciles marks and threads in both directions: an id that reappears via undo restores its thread from the tombstone, and an id that disappears via redo retires (re-tombstones) its thread. An id with no tombstone (content this CommentStore never tracked) is pruned from the document instead, so a mark can never permanently outlive every trace of its thread.

Closes #9166

Test plan

Before

Screen.Recording.2026-09-15.at.7.19.40.PM.mov

After

Screen.Recording.2026-09-15.at.7.18.52.PM.mov

…ts deletion is undone

## Description

Deleting a comment thread from the playground's comments panel removes
it from CommentStore (plain JS state outside Lexical's EditorState) and,
separately, strips the thread id from its MarkNode in an ordinary,
undoable editor.update. Undo only reverts the second half: the MarkNode
and its highlight reappear, but the thread was never actually restored,
so the mark ends up referencing a deleted comment and the comments panel
also gets stuck popping open empty for that span.

This adds a small tombstone to CommentStore (capturing a deleted
thread and its original position) and a MarkNode mutation listener,
scoped to updates tagged HISTORIC_TAG, that reconciles marks and
threads in both directions: an id that reappears via undo restores its
thread from the tombstone, and an id that disappears via redo retires
(re-tombstones) its thread. An id with no tombstone (content this
CommentStore never tracked) is pruned from the document instead, so a
mark can never permanently outlive every trace of its thread.

Closes facebook#9166

## Test plan

### Before

`npx vitest run --project unit packages/lexical-playground/src/commenting/__tests__/unit/commentMarkHistorySync.test.ts`
(run with registerCommentMarkHistorySync's registration replaced by a no-op)

    ❯ registerCommentMarkHistorySync > undo after deleting a thread restores both the mark and the comment
      AssertionError: expected false to be true
    ❯ registerCommentMarkHistorySync > the mark and comment stay in sync across repeated undo/redo cycles
      AssertionError: expected false to be true
    ❯ registerCommentMarkHistorySync > a dangling id with no tombstone is pruned instead of restored
      AssertionError: expected { stillMark: true, text: 'hello' } to deeply equal { stillMark: false, text: 'hello' }

    Test Files  1 failed (1)
         Tests  3 failed | 14 passed (17)

Manually reproduced in the playground: add an inline comment, delete
the thread, press Ctrl+Z — the highlighted mark comes back but the
comments panel stays empty ("No Comments"), and clicking back into the
mark force-reopens the empty panel.

### After

`npx vitest run --project unit packages/lexical-playground/src/commenting/__tests__/unit/commentMarkHistorySync.test.ts`

    Test Files  1 passed (1)
         Tests  17 passed (17)

`pnpm run tsc`, `pnpm run lint`, `pnpm run prettier` all clean.

Manually re-verified in the playground: same repro, but Ctrl+Z now
restores both the mark and the comment in the panel; redo removes both
again; repeated undo/redo cycles stay in sync.
@vercel

vercel Bot commented Sep 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
lexical Ready Ready Preview Sep 15, 2026 11:19pm UTC
lexical-playground Ready Ready Preview Sep 15, 2026 11:19pm UTC

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 15, 2026
Comment on lines +98 to +99
setTimeout(() => {
editor.update(

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.

I'm not sure, but I think you can simply replace editor.read with editor.update to avoid writing a workaround like that. If the function exits before this line and there are no state changes, no entry will be made in the history. So I assume that update here is equivalent to a read operation. If I'm wrong, the maintainers can correct me

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way setTimeout is not generally an acceptable solution

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@levensta @etrepum, good catch on both, thanks! I dug into this a bit more rather than just swapping the timer.

It looks like setTimeout wasn’t actually needed for re-entrancy here. By the time the mutation listener fires, Lexical has already reset _updating to false, so a plain editor.update() is fine- it just gets batched to the next microtask, which is where the delay comes from.

The actual fix seems to be {discrete: true}, which forces the update to commit immediately. That’s also the same mechanism Lexical uses internally for setEditorState, so it feels like the right pattern here.

Does that reasoning make sense before I push this?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd have to take a close read of the PR to see what the best solution is, generally discrete updates are reserved for exceptional situations because it forces a synchronous render to DOM. That's probably not needed here. If you need to just read the data from an update before it's committed you can do that from an editor.update(…) or from editor.read('pending', …).

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.

Discrete updates aren't necessary. What I meant was that if, inside the mutationListener, you need to not only read the latest state but also modify the state based on some condition, you can handle all of that within update

-      editor.read('latest', () => {
+      editor.update(() => {
        for (const [key, mutation] of mutations) {

@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Sep 16, 2026

@etrepum etrepum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After having taken a closer look at the code in context (not on my phone), the approach here is probably not good. Mutation listeners that trigger editor updates or commands should be avoided whenever possible, because it causes an update cascade (two or more DOM renders for each matching update). Mutation listeners are primarily for managing things related to the DOM, not dealing with the model. Usually there's a better way.

@sahiee-dev

Copy link
Copy Markdown
Contributor Author

That makes sense, and I don't want to just re-guess at scheduling again. Before I try another approach: I dug into why a transform (the usual no-cascade way to react to node changes) isn't an option here, $applyAllTransforms only processes nodes in editor._dirtyLeaves/_dirtyElements, and setEditorState (what undo()/redo() call) sets _dirtyType = FULL_RECONCILE without populating either dirty set (_dirtyElements.set('root', false), and the transform loop skips non-intentionally-dirty entries), so transforms genuinely never fire for a historic restore.

Given that, what would you reach for instead? Two things I can think of, but wanted your read before I commit to one:

Intercept UNDO_COMMAND/REDO_COMMAND directly rather than reacting to the resulting mutation, though I'm not sure how to run after history's own handler within the same dispatch without it already having stopped propagation.

Store the deleted-thread tombstone as part of Lexical's own undo-tracked state (e.g. via createState/NodeState, on the node) instead of in the standalone CommentStore: so undo/redo reverts it for free, no reactive correction needed at all. Bigger change than this PR's current scope, but it removes the mismatch at the source instead of patching around it.

Or is there a third option I'm missing?

@sahiee-dev
sahiee-dev requested a review from etrepum September 22, 2026 02:58

This branch was successfully deployed

2 active deployments
Preview – lexical a205b8da Deployed Sep 15, 2026 by vercel[bot]
Preview – lexical-playground a205b8da Deployed Sep 15, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Undo returns the mark of the deleted comment

3 participants