Skip to content

[lexical-link] Bug Fix: Enable autolink matching when it unlinked#8165

Open
levensta wants to merge 4 commits intofacebook:mainfrom
levensta:check-valid-unlinked-autolink
Open

[lexical-link] Bug Fix: Enable autolink matching when it unlinked#8165
levensta wants to merge 4 commits intofacebook:mainfrom
levensta:check-valid-unlinked-autolink

Conversation

@levensta
Copy link
Contributor

Description

Previously, it was possible to enter invalid characters in the text of an unlinked AutoLinkNode, which made the link invalid. The fix activates parsing of valid links in AutoLinkNode if they are marked as unlinked

Test plan

Before

Screen.Recording.2026-02-24.at.02.14.48.mov

After

Screen.Recording.2026-02-24.at.02.16.35.mov

@vercel
Copy link

vercel bot commented Feb 23, 2026

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

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment Feb 25, 2026 10:31pm
lexical-playground Ready Ready Preview, Comment Feb 25, 2026 10:31pm

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 Feb 23, 2026
);
});

test('Unlinked the autolink should not destruct if add non-spacing text in front or right after it', async ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if you insert non-space characters before or after the link, it also invalidates it if you consider the text as a whole (visually, it is a whole). I don't have a strong opinion on this case, so for now, I've fixed the current behavior with this test

Screen.Recording.2026-02-24.at.02.43.40.mov

The original PR #6306 was sent with the aim of resetting the link styles, but I don't know if users can have a link and text in a single string without a space between them, and whether this makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other autolink implementations (e.g. google docs, github flavored markdown) will still autolink when the punctuation goes after the link, e.g. www.google.com! will autolink www.google.com. I don't have a super strong opinion about it, since you can always manually link text, but the GFM style autolink behavior is probably what more people are used to. Either way this PR's scope doesn't need to cover that sort of change.

Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

I'm not sure this fixes the whole problem, e.g. if you have an unlinked AutoLinkNode and add a decorator like date or emoji it will split the AutoLinkNode instead of destroying any of them

@levensta
Copy link
Contributor Author

I'm not sure this fixes the whole problem, e.g. if you have an unlinked AutoLinkNode and add a decorator like date or emoji it will split the AutoLinkNode instead of destroying any of them

If during transformation one of the child nodes is not a TextNode or is not simple, the link will be destroyed. I added a test with emoji

@levensta
Copy link
Contributor Author

The tests are failing for the second time in a specific job e2e-tests / windows (22.x, chromium, rich-text, legacy-events) / e2e-test. I don't understand what the problem might be yet, maybe it's worth marking the tests as flaky or rewriting them as unit tests in LexicalAutoLinkExtension

@etrepum
Copy link
Collaborator

etrepum commented Feb 25, 2026

I'm not sure either without looking a lot more closely, which is hard for me since I don't have a windows computer to test with. unit tests would be fine, it's unclear if marking them as flaky would be a sufficient workaround. Really depends on what kind of timing issue it is.

@levensta
Copy link
Contributor Author

I have no idea what's wrong with this test. I couldn't find any information about what exactly changes the legacy events job and what browser version is used, but I tried running the production build on Win 10 Chrome 86 through an online emulator and reproducing the scenario, and I get the expected result

_Screen.Recording.2026-02-26.at.01.58.59.mov

I would suggest skipping this test specifically for legacy events + chromium + windows

@levensta
Copy link
Contributor Author

By the way, the video shows v0.40.dev, although I ran the build and preview on 0.41 🤷‍♂️

image

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.

2 participants