[lexical-playground] Bug Fix: autocomplete format before and after insertion

[bedre7]

Description

This PR aims to fix the formatting of the suggested text before and after insertion. The AutocompleteNode was previously implemented by extending the DecoratorNode, which has no format property. As a result, it has been reimplemented by extending the TextNode instead to fix the format of the text.

Closes #6816

Test plan

Existing test has been modified to meet the new requirements and a new test has been added.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 20, 2024 5:47pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 20, 2024 5:47pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	30.85 KB (0%)
lexical - esm	30.73 KB (0%)
@lexical/rich-text - cjs	39.58 KB (0%)
@lexical/rich-text - esm	32.67 KB (0%)
@lexical/plain-text - cjs	38.22 KB (0%)
@lexical/plain-text - esm	29.93 KB (0%)
@lexical/react - cjs	41.35 KB (0%)
@lexical/react - esm	34.03 KB (0%)

[etrepum]

This does seem like it's an improvement but there are some strange interactions with undo/redo. These don't appear to be regressions though, since there are also similar problems in the current playground, but if you want to go deeper that's probably worth exploring in another PR. I think the root cause is that the plugin requires transforms to run for correct operation but transforms don't run after undo/redo so something else has to happen here.

[ivailop7]

I believe we should have exportDOM exporting nothing, because if we don't have exportDOM, it will trigger the createDOM logic for clipboard and so on and we should never export the autocomplete suggestion in the exported html.

[etrepum]

Yeah there's also that exportDOM issue, which would not fix the history interactions but would help with other bugs.

[etrepum]

It looks like in the full test suite it's failing some e2e tests, let's sort that out and the exportDOM issue before merging

[bedre7]

It looks like in the full test suite it's failing some e2e tests, let's sort that out and the exportDOM issue before merging

Hi @etrepum, @ivailop7,
Thanks for your review and suggestions. I fixed most of the failing tests and the exportDOM issue, but there are still some failing flaky tests and 2 other tests which I couldn't figure out how they're related to my changes. Am I missing something?

[etrepum]

I'm still seeing some issues that makes it seem like the node transform and/or the node itself isn't implemented correctly. The repro steps for this are:

1. Type the start of a word in the autocomplete dictionary
2. Press return
3. Undo (cmd-z on mac or ctrl-z elsewhere)

The autocomplete shows back up, in the wrong place, and you can add text to it (it should probably be in token mode, but also it shouldn't be there at all).

autocomplete-undo.mov

[etrepum]

I think perhaps a better strategy here would be to insert the text directly at the end of the previous node? What would happen if you just did something like:

autocompleteNode.selectPrevious().insertText(lastSuggestion);

Then you wouldn't need to worry about any of the style because it should instead modify the previous node instead of create a new node with the hope that they get merged together later.

[etrepum]

I can't comment on the line because it's not modified in the PR but above where it has if (node.__uuid === uuid && key !== autocompleteNodeKey) { that should probably use || instead?

[bedre7]

I can't comment on the line because it's not modified in the PR but above where it has if (node.__uuid === uuid && key !== autocompleteNodeKey) { that should probably use || instead?

I just tried this, it clears the suggestion for all sessions in collab mode including the composing session, so it is never visible to anyone.

[bedre7]

I think perhaps a better strategy here would be to insert the text directly at the end of the previous node? What would happen if you just did something like:

autocompleteNode.selectPrevious().insertText(lastSuggestion);

Then you wouldn't need to worry about any of the style because it should instead modify the previous node instead of create a new node with the hope that they get merged together later.

That's a good point, It seems to fix the undo/redo bug, but it causes another bug: After undoing suggestion insertion, pressing tab won't insert the suggestion back; it inserts a tab and pushes the suggestion forward.

[etrepum]

I think the solution to that is probably to have this plug-in listen to history commands directly, because its transforms are not going to run on undo but there might still be something that needs to happen.

[bedre7]

I'm still seeing some issues that makes it seem like the node transform and/or the node itself isn't implemented correctly. The repro steps for this are:

1. Type the start of a word in the autocomplete dictionary
2. Press return
3. Undo (cmd-z on mac or ctrl-z elsewhere)

The autocomplete shows back up, in the wrong place, and you can add text to it (it should probably be in token mode, but also it shouldn't be there at all).

autocomplete-undo.mov

Thanks for pointing this out. I can reproduce a similar bug on the main branch, so I don't think it's a regression. What if I refactor this plugin and try to address the undo/redo bugs in another PR?

[etrepum]

I think addressing the history related issues would be fine in another PR. As you've mentioned, they are not regressions.

[bedre7]

Hi @etrepum , is this ready to be merged?