Bug: async updates cause unwanted history

[montlebalm]

There's a class of bugs involving async actions that leaves the history stack in an unwanted state. The problem occurs when a future update needs to replace a past update such as when uploading an image. Typically, an image node is inserted with a loading state because we don't know the src until it's uploaded. Eventually, we'll set the src as the result of an async action. If the async update is not merged then it's possible to "undo" back to the image's initial loading state which the user would never want.

Updates that use discrete: true and the history-merge tag will properly squash the update, but only if there have been no other updates in between. There's no way to merge a new update into a previous update of arbitrary position in the history stack.

The current way to solve this is to track data in a separate map keyed by node key. It's cumbersome and feels like something that the library should provide a built-in solution for.

Lexical version: 0.9.0 (and earlier)

Steps To Reproduce

1. Insert an image node with a loading indicator. Asynchronously upload the desired image to your backend of choice.
2. Make random edits unrelated to the image
3. When the async upload finishes, update the placeholder image node with the new src attribute.
4. Trigger "undo" to reset the editor to state (2) where the image is still loading

Link to code example: https://codesandbox.io/s/lexical-plain-text-example-forked-usgq8e?file=/src/Editor.js

The current behavior

The user can "undo" back to an unwanted loading state.

The expected behavior

Previous history should be overwritten so that the user cannot return to unwanted loading states.

[thegreatercurve]

We don't want to let users access stack array indexes via the core library, because these positions are liable to change frequently, as we use history-merge within some of the core packages.

However, we allow uses to create their own history states with createEmptyHistoryState for these sort of edge cases. You can then access it via context using something like the below which is on the playground:

  const {historyState} = useSharedHistoryContext();

This will give you access to the current history state, undo and redo stacks, which you can then manually manipulate.

[montlebalm]

we allow uses to create their own history states with createEmptyHistoryState for these sort of edge cases

In my opinion, updating the editor as a result of an async action is not an edge case. It's anecdotal, but I've seen several people with similar issues in the Lexical Discord.

[montlebalm]

What would "manually manipulating" the history stack look like? Given the same image scenario:

1. Insert an image node with a loading indicator
2. Make random edits unrelated to the image
3. Update the placeholder image node with the new src attribute

Here's my guess: In step 3, look back through history collecting states until you find the state from step 1. Then, manually update the image node in each of the collected states with the new src attribute. Is that what you mean?

[kevinansfield]

@montlebalm did you get any further with handling this? I have similar questions around the handling of preview+loading progress states within nodes without causing a bunch of unwanted and problematic history states

[montlebalm]

Unfortunately no. I was hoping to get a response to my last comment regarding the expected way to handle these async updates.

Our current workaround is to keep this kind of data out of lexical as much as possible. For example, it feels natural to store an image's src on its node, but we have to keep it as React state since it's modified once the upload completes. It's inconvenient and makes serialization more difficult.

[acywatson]

I see that we didn't get a full resolution here, so I'm reopening so we can keep track of it

[acywatson]

Our current workaround is to keep this kind of data out of lexical as much as possible. For example, it feels natural to store >an image's src on its node, but we have to keep it as React state since it's modified once the upload completes. It's >inconvenient and makes serialization more difficult.

I don't think this is bad. One strategy we make use of internally that bypasses this problem completely is storing some sort of ID on the node, then using that to fetch or generate a URL at runtime and encapsulating the logic for doing that in the React component. @fantactuka is working on documenting this pattern.

I realize that it suggests a change in architecture, and I'd love to get you a solution that doesn't force that, but I did want to call this one out. It's not without it's own complexities, though.

[acywatson]

Maybe a different history tag with slightly different semantics (like history-ignore or history-discard) could solve this

[hipstersmoothie]

Throwing in my use case:

1. I have special element nodes that get numbered
2. these nodes only exist at the root
3. I use registerMutationListener to find when the nodes are added and deleted
4. I update the numbers

With no way to "merge" undo states all of my updates to the numbers create their own entry in the undo stack

[hipstersmoothie]

Oh history merge worked well for me. Should be documented

[fantactuka]

Not really sure how this can be built-in, given async action can have any delay you'd have to go through whole history undo/redo stacks, find that node and patch it. So it has to be some sort of patch callback that will alter all historical snapshots exposed through API? Instead I'd suggest to go with what @thegreatercurve suggested above and use createEmptyHistoryState:

1. Create own history stack so that you have pointer to it
2. When async action has finished you run editor.update with historic tag (a.k.a. history-ignore)
3. Run through history stack (since now you have pointer to it) and alter node data there. You might need to clone editor state inside each history entry given its considered immutable, so watch out for any perf penalties

I'd also consider caching async request results for few reasons: you might copy-paste node while it's still running async action and you'd likely want to reuse existing running request instead of starting a new one.

[montlebalm]

I think rewriting history could be a fine solution provided there are examples. Even better if lexical exports a helper utility.

Here's what I was able to come up with based on the natural language description of the process:

//
// Utility
//

const updateNode = <TNode extends LexicalNode>(
  stack: HistoryStateEntry,
  nodeKey: NodeKey,
  updater: (node: TNode) => void
): boolean => {
  //  Find the node in history. If we can't find it then we're past the point
  //  at which it was created.
  const node = stack.editorState.read(() => $getNodeByKey<TNode>(nodeKey));
  if (!node) return false;

  stack.editor.update(
    () => {
      updater(node);
      $addUpdateTag('history-merge');
    },
    { discrete: true }
  );

  return true;
};

const updateHistoricalNode = <TNode extends LexicalNode>(
  history: HistoryState,
  nodeKey: NodeKey,
  updater: (node: TNode) => void
) => {
  // Update current state
  if (history.current) {
    updateNode(history.current, nodeKey, updater);
  }

  // Update undo history
  for (const stack of history.undoStack) {
    const updated = updateNode(stack, nodeKey, updater);
    if (!updated) break;
  }

  // Update redo history
  for (const stack of history.redoStack) {
    const updated = updateNode(stack, nodeKey, updater);
    if (!updated) break;
  }
};

//
// Usage
//

uploadImage(url).then((serverId) => {
  updateHistoricalNode<ImageNode>(history, nodeKey, (node) => {
    node.setServerId(serverId);
  });
});

Does that look right? It could potentially cause a lot of updates based on the size of the undo/redo stack. Any suggestions on how to make it more efficient?

[montlebalm]

2. When async action has finished you run editor.update with historic tag (a.k.a. history-ignore)

Should "history-ignore" be "history-merge"? I don't see the ignore tag in the codebase.

[OriginalEXE]

I've arrived here because I have a similar, but not same use case.

In the editor a user can type /, which shows a menu, from which they can select an action. Actions are async, so upon choosing the action I insert a temporary "loading indicator" node in place of /. When a server responds, I insert the response as a text node, replacing the "loading indicator" node. I would not want ctrl + z to get user back to a state where the "loading indicator" node is visible, it should either be a final state, or the state before they typed /.

[montlebalm]

@fantactuka @thegreatercurve does my example look like what you'd expect? Any suggestions for improvement?

[montlebalm]

@fantactuka @thegreatercurve just following-up to see if my proposed approach matches your expectations

[montlebalm]

@fantactuka @thegreatercurve checking in again to see if y'all have any guidance on how to do the proposed history rewrite