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

#6768 fix to avoid infinite markdown shortcut matchers run #6778

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

fantactuka
Copy link
Contributor

@fantactuka fantactuka commented Oct 30, 2024

Issue #6768 happens because of emoji markdown shortcut. It has conditional replacement which was not really supported for shortcuts:

const emoji = emojiList.find((e) => e.aliases.includes(name))?.emoji;
if (emoji) {
textNode.replace($createTextNode(emoji));
}

  1. Quick option is to disable it from list of shortcut transformers:

return <MarkdownShortcutPlugin transformers={PLAYGROUND_TRANSFORMERS} />;

  1. Another alternative would be to update emoji RegExp to match exact emoji names and instead of having :([a-Z0-9]): it'd be :(smile|sad|happy|...|joy):. The problem is that with current emoji-list that regex is 23kb long.

  2. Enable conditional replacement (this PR). The reason it ran infinitely is because once we find match we call textNode.splitText(matchOffset) to extract text node that should be replaced:

if (startIndex === 0) {
[replaceNode] = anchorNode.splitText(endIndex);
} else {
[, replaceNode] = anchorNode.splitText(startIndex, endIndex);
}
replaceNode.selectNext(0, 0);
transformer.replace(replaceNode, match);

Even if we don't replace anything, it still causes full update cycle which triggers another transformers run. One thing is whether we should even call update cycle for unused result of .splitText (cc @zurfyx) given split nodes will reconcile back to a single text node. But on top of it, we don't really need to run any shortcuts logic if selection itself hasn't changed as we only suppose to do it when user types (i.e. selection moves).

Another (more minor) issue was in emoji regex that should ensure we only attempt to run matcher against end of node's text

Test plan

Tests + sanity check

npm run test-e2e-chromium -- Markdown
✓ 52 passed (28.5s)

Verified

This commit was signed with the committer’s verified signature.
crazy-max CrazyMax
Copy link

vercel bot commented Oct 30, 2024

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 Oct 30, 2024 3:34am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 3:34am

@facebook-github-bot facebook-github-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 Oct 30, 2024
Copy link

size-limit report 📦

Path Size
lexical - cjs 29.94 KB (0%)
lexical - esm 29.78 KB (0%)
@lexical/rich-text - cjs 38.59 KB (0%)
@lexical/rich-text - esm 31.65 KB (0%)
@lexical/plain-text - cjs 37.25 KB (0%)
@lexical/plain-text - esm 28.94 KB (0%)
@lexical/react - cjs 40.32 KB (0%)
@lexical/react - esm 33.01 KB (0%)

Copy link
Contributor

@potatowagon potatowagon left a comment

Choose a reason for hiding this comment

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

thanks for the quick fix!

@potatowagon
Copy link
Contributor

potatowagon commented Oct 30, 2024

closes: #6768

@potatowagon potatowagon added this pull request to the merge queue Oct 30, 2024
Merged via the queue into main with commit 07e300c Oct 30, 2024
42 of 44 checks passed
@potatowagon potatowagon linked an issue Oct 30, 2024 that may be closed by this pull request
@etrepum etrepum mentioned this pull request Nov 7, 2024
@zurfyx zurfyx deleted the shortcuts-infinite-loop branch February 28, 2025 18:17
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: When I type : in the editor, it becomes unresponsive.
4 participants