[lexical-markdown] Fix: Prevent Link Transformer from consuming preceding text#8130
[lexical-markdown] Fix: Prevent Link Transformer from consuming preceding text#8130Sa-Te wants to merge 4 commits intofacebook:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
d7d4e1a to
a22d61d
Compare
| const matchText = match[0]; | ||
|
|
||
| const textContent = textNode.getTextContent(); | ||
| const localMatchIndex = textContent.indexOf(matchText); | ||
| if (localMatchIndex === -1) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
In what scenario would this be any different from this code?
| const matchText = match[0]; | |
| const textContent = textNode.getTextContent(); | |
| const localMatchIndex = textContent.indexOf(matchText); | |
| if (localMatchIndex === -1) { | |
| return; | |
| } | |
| const localMatchIndex = match.index || 0; |
There was a problem hiding this comment.
I tried the optimization, but it caused regressions in the LexicalMarkdown import tests. It seems match.index can become stale if other transformers modify the node content during the pass. Reverting to indexOf fixes the regression.
There was a problem hiding this comment.
It seems that the importer already splits the text based on the match before calling the replace function so localMatchIndex is always 0
etrepum
left a comment
There was a problem hiding this comment.
This test passes even if the MarkdownTransformers changes are reverted, so it's not clear that it's fixing anything
@etrepum Thanks for the review! I investigated your comment about localMatchIndex always being 0. While that is true for simple cases, I found that relying on match.index (or assuming 0) actually causes 11 regressions in the lexical-markdown unit test suite. The Issue: Evidence: The Fix: |
|
I don't see any test failures when reverting your changes, even in the new test, so if this fixes anything it needs additional tests that will fail without your changes. Relying on match.index indeed does not work (without other refactoring) because the importer splits the TextNode before calling the replace function so the string is already split, at least in the import cases. |
f1c27f3 to
5dd2162
Compare
@etrepum Thanks for the feedback! I've added a new unit test (LINK.replace handles stale match indices) that isolates the specific race condition where match.index becomes stale. The Issue: Without this fix (using match.index): The new test fails, and I also see regressions in 11 existing tests (specifically regarding nested formatting). With this fix (using indexOf): The new test passes. This confirms that searching the current text node is necessary to handle these nested cases correctly. |
|
I think these unit tests are a bit too artificial to be useful, they really should be called by the transformer infrastructure (shortcuts/importer) to show that they are working as expected in the environments that they are used from. I don't think these tests are a good simulation of how that works. |
The Markdown link transformer was using the regex match index relative to the entire text content, rather than relative to the specific TextNode being transformed. This caused text preceding the link (e.g., Start link) to be incorrectly sliced out if the match index was non-zero.
Fix:
Test Plan: