[lexical-rich-text] Bug Fix: use writable node in HeadingNode.setTag#8235
[lexical-rich-text] Bug Fix: use writable node in HeadingNode.setTag#8235etrepum merged 5 commits intofacebook:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @karesansui-u! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
etrepum
left a comment
There was a problem hiding this comment.
The test is useless, it would pass without the fix. You can either fix the test to actually test the condition (requires two updates with an await or using the discrete option) or delete the test altogether.
Split the single-update test into two discrete updates so the test actually exercises the getWritable() code path. In a single update, both this.__tag and self.__tag write to the same object, making the test pass regardless of the fix. With discrete updates, the second update gets a cloned node from getWritable(), so only self.__tag correctly mutates the writable copy.
|
@etrepum Thank you for the review — you're absolutely right. The original test ran everything in a single I've updated the test to use two separate
With this structure, |
etrepum
left a comment
There was a problem hiding this comment.
A similar fix needs to happen in getTag to use getLatest
getFirstChildOrThrow() returns a read-only node. setTag() internally calls getWritable() which returns a new clone, so the original variable still points to the old read-only node with tag 'h1'. Use the return value of setTag() to correctly assert the writable clone's tag.
etrepum
left a comment
There was a problem hiding this comment.
This fix isn’t complete, it’s not required to use the result of setTag, so you can revert that change. getTag needs to fixed to use getLatest.
HeadingNode.getTag() was reading this.__tag directly, which returns stale values when called on a non-latest node reference. Use getLatest().__tag to match the pattern used by ListNode.getTag() and other node getters. Also revert the test to use heading.getTag() directly instead of capturing the setTag() return value, since getLatest() now ensures the correct value is returned from any node reference.
|
@etrepum Sorry about the back and forth — my test didn't properly validate the fix, and I missed the I've pushed a new commit that:
Appreciate the thorough review — learned a lot about Lexical's node versioning model. |
There was a problem hiding this comment.
This file should not change, remove this from the PR.
There was a problem hiding this comment.
I'm really sorry about that — reverted in the latest push.
Summary
HeadingNode.setTag()callsgetWritable()but writes tothis.__taginstead of the returned writable clone (self). This bypasses Lexical's immutability model — the original (possibly frozen) node is mutated directly.The fix changes
this.__tag = tagtoself.__tag = tag, matching the pattern used elsewhere (e.g.CodeNode.setLanguage()).Test plan
HeadingNode.setTag()verifying the tag is updated correctlypnpm test-unit: 2112 passed)