Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix suggestion collaboration bug #4380

Merged

Conversation

fsvergara
Copy link
Contributor

Please describe your changes

Fix a bug where the suggestion re-renders when someone is typing above the cursor while collaborating.
Here's a screencast explaining the bug and the code:
https://www.loom.com/share/bbb7e73feae44a8c8ebfcbae77ad3eb5?sid=8fd24818-0b3b-4cea-9a54-b7bcb017d682

How did you accomplish your changes

Change when onStart and onExit are called in the suggestion extension, so that they won't be called if we are just moving the suggestion but it will remain active.

How have you tested your changes

Used the changed version of the suggestion extension in our local environment, here's how it looks after the changes:
https://www.loom.com/share/facfc0b58a7a496199f2be07a8306969

How can we verify your changes

  1. Create an editor with the Suggestion and Collaboration extensions
  2. Open two editors, with one of them initiate the suggestion and with the other one type above the cursor where the suggestion is
  3. The suggestion should not re-render

Remarks

[add any additional remarks here]

Checklist

  • The changes are not breaking the editor
  • Added tests where possible
  • Followed the guidelines
  • Fixed linting issues

Related issues

@netlify
Copy link

netlify bot commented Aug 25, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 0069ac6
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/66692b8e4906180008ddadf6
😎 Deploy Preview https://deploy-preview-4380--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@fsvergara fsvergara marked this pull request as ready for review August 25, 2023 16:16
@fsvergara fsvergara changed the title Fix suggestion collab bug Fix suggestion collaboration bug Aug 25, 2023
@nperez0111 nperez0111 force-pushed the develop branch 2 times, most recently from bcaef5c to 7e7ae19 Compare June 12, 2024 04:33
@nperez0111
Copy link

Thank you for your contribution @fsvergara

Sorry it took so long to get a review on this, I'm going to add to the beta

@nperez0111 nperez0111 merged commit 8d93070 into ueberdosis:develop Jun 12, 2024
14 checks passed
@nperez0111 nperez0111 mentioned this pull request Jul 13, 2024
5 tasks
@rfgamaral
Copy link
Contributor

@nperez0111 We found out that this fix introduced a bug that didn't exist before when using the suggestion utility with allowSpaces: true, any chance this can be reverted until a proper a fix that doesn't introduce new bugs is implemented?

The newly introduced bug by this can be reproduced like this:

  • Add allowSpaces: true to demos/src/Nodes/Mention/React/suggestion.js
  • Open the Nodes/Mention/React demo (i.e. npm run start)
  • Type the following in this order: @lea, Esc, Space, and @
    • Observe that the suggestion dropdown does not appear (this didn't happen before)

@nperez0111
Copy link

@rfgamaral I'd prefer to roll forward, will look into it tmrw

There are too many changes queued up to be doing a patch release right now

@nperez0111
Copy link

@fsvergara I wonder if this change (to latest tiptap version on develop) works for what you were running into?

diff --git a/demos/src/Nodes/Mention/React/suggestion.js b/demos/src/Nodes/Mention/React/suggestion.js
index 554d9ee11..2695199d2 100644
--- a/demos/src/Nodes/Mention/React/suggestion.js
+++ b/demos/src/Nodes/Mention/React/suggestion.js
@@ -4,6 +4,7 @@ import tippy from 'tippy.js'
 import MentionList from './MentionList.jsx'
 
 export default {
+  allowSpaces: true,
   items: ({ query }) => {
     return [
       'Lea Thompson',
diff --git a/packages/suggestion/src/suggestion.ts b/packages/suggestion/src/suggestion.ts
index 80744aaac..c0dd97808 100644
--- a/packages/suggestion/src/suggestion.ts
+++ b/packages/suggestion/src/suggestion.ts
@@ -194,10 +194,9 @@ export function Suggestion<I = any, TSelected = any>({
           const started = !prev.active && next.active
           const stopped = prev.active && !next.active
           const changed = !started && !stopped && prev.query !== next.query
-
-          const handleStart = started
+          const handleStart = started || moved
           const handleChange = changed || moved
-          const handleExit = stopped
+          const handleExit = stopped || moved
 
           // Cancel when suggestion isn't active
           if (!handleStart && !handleChange && !handleExit) {

@rfgamaral
Copy link
Contributor

@nperez0111 Yes, that will fix it. Actually, just changing handleStart to started || moved is enough to fix it. But won't that reintroduce the issue at hand? 🤔

@nperez0111
Copy link

My understanding is that it would be okay to trigger more updates than needed (which would re-render a slash command), rather than forcing it to re-render and recreate from scratch by flagging the start and stop.
@rfgamaral I'm fairly confident in this, but I will let it soak over night as it is late over here

@rfgamaral
Copy link
Contributor

@nperez0111 That's okay, rest up, and come back tomorrow energized 💪

Hopefully, you can figure it out tomorrow morning, and if you could cut a patch release, I'd really appreciate it. 🙏 This is blocking us from updating Tiptap to the latest version, which brings a couple of important fixes for us.

@nperez0111
Copy link

@rfgamaral consider it done 😄

@nperez0111
Copy link

@rfgamaral 2.6.5 is out

@rfgamaral
Copy link
Contributor

@nperez0111 This is much appreciated, thank you!

@rfgamaral
Copy link
Contributor

rfgamaral commented Aug 23, 2024

@nperez0111 Sorry to bring this up again, but while doing tests on our side with the new version, we realized that it's still not working as it was before this PR.

The remaining issue can be reproduced like this:

  • Add allowSpaces: true to demos/src/Nodes/Mention/React/suggestion.js
  • Open the Nodes/Mention/React demo (i.e. npm run start)
  • Type the following in this order: @, Space, @, and Esc
    • Observe that the suggestion dropdown does not close (this didn't happen before this PR)

And while I was defining this test plan, I found yet another issue caused by this PR:

firefox_mMo2z7HkRX.mp4

When I selected all the text, and deleted the selection, the dropdown does not close.

And yet another bug (they are all related to the same change):

firefox_jmD6ENqxv8.mp4

I always found the suggestion utility code a bit confusing, so I'm unsure if the best solution is to revert the code to what it was before this PR (and try to find a better solution for that issue), or try to keep rolling forward, and find a solution for all these issues.

@nperez0111
Copy link

nperez0111 commented Aug 23, 2024

@rfgamaral I have too many things to do today so I won't be able to get to it until next week.

My suggestion would be to attempt to add the same condition that I put at the handleStart event onto the handleExit event & see if that resolves it. I tried the minimal change because I couldn't come up with a scenario where that would happen, but it seems like you found them.

Would be willing to take a PR for a fix sometime within the day

@rfgamaral
Copy link
Contributor

My suggestion would be to attempt to add the same condition that I put at the handleStart event onto the handleExit event & see if that resolves it.

@nperez0111 Yes, that solves all those issues I posted, and keeps the issue at hand also fixed 👍

Will open a quick PR.

@nperez0111
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants