Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders Open Preview |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (elem !== null) { | ||
| elem.classList.add("selected"); | ||
| changedElems.push(elem); | ||
| setShowComments(true); | ||
| showCommentsTimeoutId = window.setTimeout(() => { | ||
| setShowComments(true); | ||
| }, 0); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return () => { | ||
| if (showCommentsTimeoutId !== undefined) { | ||
| window.clearTimeout(showCommentsTimeoutId); |
There was a problem hiding this comment.
🟡 Timeout IDs leak in loop — only last timeout is cleared on cleanup
When multiple active IDs and keys exist, showCommentsTimeoutId is overwritten on each loop iteration at packages/platform/src/marginal/comments/CommentPlugin.tsx:819. The cleanup at line 828 only clears the last timeout ID stored; all previously-created timeouts leak and will fire after the effect's cleanup runs. This can cause setShowComments(true) to execute after the effect has been torn down (e.g., when activeIDs changes rapidly or the component unmounts), leading to stale state updates.
Example: 3 matching elements → 2 leaked timeouts
If the nested loops find 3 elements, 3 setTimeout calls are made, but only the 3rd ID is saved. The cleanup clears timeout #3 but timeouts #1 and #2 still fire.
(Refers to lines 809-829)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
👍 however the CommentPlugin is deprecated and no longer used. Updates were only made to satisfy the new react-hooks/refs rule.
| const seconds = Math.round( | ||
| (comment.timeStamp - (performance.timeOrigin + performance.now())) / 1000, | ||
| ); | ||
| const [currentTimeMs, setCurrentTimeMs] = useState<number>(0); |
There was a problem hiding this comment.
🟡 Initial currentTimeMs of 0 causes all comments to flash "Just now" on first render
currentTimeMs is initialized to 0 at packages/platform/src/marginal/comments/CommentPlugin.tsx:460. Before the useEffect on line 462 fires (after the first paint), the render computes seconds = Math.round((comment.timeStamp - 0) / 1000) at line 475. Since comment.timeStamp is a large positive number (epoch-relative ms), seconds is a huge positive value, making seconds > -10 always true, so every comment displays "Just now" on the initial render regardless of its actual age. The effect corrects this after the first paint, but users see a brief flash of incorrect timestamps.
| const [currentTimeMs, setCurrentTimeMs] = useState<number>(0); | |
| const [currentTimeMs, setCurrentTimeMs] = useState<number>( | |
| () => performance.timeOrigin + performance.now(), | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
👍 however the CommentPlugin is deprecated and no longer used. Updates were only made to satisfy the new react-hooks/refs rule.
- update Lexical v0.41.0 - tighten workspace overrides - fix linting after upgraded `eslint-plugin-react-hooks` - fix API extraction
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson made 2 comments.
Reviewable status: 0 of 25 files reviewed, 2 unresolved discussions (waiting on devin-ai-integration[bot]).
| const seconds = Math.round( | ||
| (comment.timeStamp - (performance.timeOrigin + performance.now())) / 1000, | ||
| ); | ||
| const [currentTimeMs, setCurrentTimeMs] = useState<number>(0); |
There was a problem hiding this comment.
👍 however the CommentPlugin is deprecated and no longer used. Updates were only made to satisfy the new react-hooks/refs rule.
| if (elem !== null) { | ||
| elem.classList.add("selected"); | ||
| changedElems.push(elem); | ||
| setShowComments(true); | ||
| showCommentsTimeoutId = window.setTimeout(() => { | ||
| setShowComments(true); | ||
| }, 0); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return () => { | ||
| if (showCommentsTimeoutId !== undefined) { | ||
| window.clearTimeout(showCommentsTimeoutId); |
There was a problem hiding this comment.
👍 however the CommentPlugin is deprecated and no longer used. Updates were only made to satisfy the new react-hooks/refs rule.
tjcouch-sil
left a comment
tjcouch-sil
left a comment
There was a problem hiding this comment.
@tjcouch-sil reviewed 25 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on devin-ai-integration[bot]).
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved.
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
I made a PR to improve gitattributes.
@irahopkinson made 1 comment.
Reviewable status:
2 participants
eslint-plugin-react-hooksThis change is