[WIKI-575] fix: add unique keys to add nodeview wrappers #7520
Conversation
WalkthroughUnique and stable React Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/editor/src/core/extensions/callout/block.tsx(2 hunks)packages/editor/src/core/extensions/code/code-block-node-view.tsx(3 hunks)packages/editor/src/core/extensions/custom-image/components/node-view.tsx(2 hunks)packages/editor/src/core/extensions/mentions/mention-node-view.tsx(1 hunks)packages/editor/src/core/extensions/work-item-embed/extension.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
Learnt from: SatishGandham
PR: #5864
File: packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts:60-60
Timestamp: 2024-10-22T08:03:04.373Z
Learning: In packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts, the getImageSource command returns a string directly, not a function.
🔇 Additional comments (3)
packages/editor/src/core/extensions/code/code-block-node-view.tsx (1)
8-9: LGTM! Excellent implementation of stable unique keys.The use of
useMemowithuuidv4ensures each code block instance gets a unique, stable key that persists across re-renders. This is the ideal approach for components that don't have natural unique identifiers.Also applies to: 26-27, 41-41
packages/editor/src/core/extensions/callout/block.tsx (1)
2-3: LGTM! Consistent implementation with stable unique keys.The implementation mirrors the approach used in the code block component, using
useMemowithuuidv4to generate stable, unique keys. This ensures proper React reconciliation for callout block instances.Also applies to: 26-27, 33-33
packages/editor/src/core/extensions/custom-image/components/node-view.tsx (1)
25-25: UsingimageEntityIdas the React key is safe—no fallback neededWe verified that every CustomImage node is inserted via the extension’s
insertImageComponentcommand, which always generates and assigns a UUID (uuidv4()) tonode.attrs.id. This guarantees that:
idis neverundefinedornullwhen the node is rendered- IDs are universally unique thanks to UUID v4
The default
nullvalue inaddAttributes()is only a placeholder untiladdCommandsruns and sets a real UUID. You can continue using:<NodeViewWrapper key={`custom-image-node-view-${imageEntityId}`}>without additional fallback logic.
| <NodeViewWrapper | ||
| key={`mention-node-view-${attrs[EMentionComponentAttributeNames.ENTITY_IDENTIFIER]}`} | ||
| className="mention-component inline w-fit" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling empty entity_identifier to prevent key collisions.
The key generation uses entity_identifier directly, but if this value is undefined or empty (as suggested by the fallback on line 25), multiple mention nodes could end up with the same key mention-node-view-, causing React reconciliation issues.
Consider adding a fallback UUID or index-based key:
+ import { v4 as uuidv4 } from "uuid";
+ import { useMemo } from "react";
export const MentionNodeView: React.FC<MentionNodeViewProps> = (props) => {
const {
extension,
node: { attrs },
} = props;
+ const fallbackId = useMemo(() => uuidv4(), []);
+ const entityId = attrs[EMentionComponentAttributeNames.ENTITY_IDENTIFIER] || fallbackId;
return (
<NodeViewWrapper
- key={`mention-node-view-${attrs[EMentionComponentAttributeNames.ENTITY_IDENTIFIER]}`}
+ key={`mention-node-view-${entityId}`}
className="mention-component inline w-fit"
>🤖 Prompt for AI Agents
In packages/editor/src/core/extensions/mentions/mention-node-view.tsx around
lines 20 to 23, the key for NodeViewWrapper uses the entity_identifier directly,
which can be undefined or empty, leading to duplicate keys and React
reconciliation problems. To fix this, modify the key generation to include a
fallback value such as a UUID or a unique index when entity_identifier is
missing or empty, ensuring all keys are unique and stable.
| @@ -18,7 +18,7 @@ export const WorkItemEmbedExtension = (props: Props) => | |||
| WorkItemEmbedExtensionConfig.extend({ | |||
| addNodeView() { | |||
| return ReactNodeViewRenderer((issueProps: any) => ( | |||
| <NodeViewWrapper> | |||
| <NodeViewWrapper key={`work-item-embed-node-view-${issueProps.node.attrs.entity_identifier}`}> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling undefined entity_identifier to prevent key collisions.
Similar to the mentions component, if entity_identifier is undefined or empty, multiple work item embed nodes could end up with the same key, causing React reconciliation issues.
Consider adding a fallback mechanism:
export const WorkItemEmbedExtension = (props: Props) =>
WorkItemEmbedExtensionConfig.extend({
addNodeView() {
+ let nodeCounter = 0;
return ReactNodeViewRenderer((issueProps: any) => {
+ const entityId = issueProps.node.attrs.entity_identifier || `fallback-${++nodeCounter}`;
return (
- <NodeViewWrapper key={`work-item-embed-node-view-${issueProps.node.attrs.entity_identifier}`}>
+ <NodeViewWrapper key={`work-item-embed-node-view-${entityId}`}>
{props.widgetCallback({
issueId: issueProps.node.attrs.entity_identifier,
projectId: issueProps.node.attrs.project_identifier,
workspaceSlug: issueProps.node.attrs.workspace_identifier,
})}
</NodeViewWrapper>
);
});
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <NodeViewWrapper key={`work-item-embed-node-view-${issueProps.node.attrs.entity_identifier}`}> | |
| export const WorkItemEmbedExtension = (props: Props) => | |
| WorkItemEmbedExtensionConfig.extend({ | |
| addNodeView() { | |
| let nodeCounter = 0; | |
| return ReactNodeViewRenderer((issueProps: any) => { | |
| const entityId = | |
| issueProps.node.attrs.entity_identifier || | |
| `fallback-${++nodeCounter}`; | |
| return ( | |
| <NodeViewWrapper | |
| key={`work-item-embed-node-view-${entityId}`} | |
| > | |
| {props.widgetCallback({ | |
| issueId: issueProps.node.attrs.entity_identifier, | |
| projectId: issueProps.node.attrs.project_identifier, | |
| workspaceSlug: issueProps.node.attrs.workspace_identifier, | |
| })} | |
| </NodeViewWrapper> | |
| ); | |
| }); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In packages/editor/src/core/extensions/work-item-embed/extension.tsx at line 21,
the key for NodeViewWrapper uses entity_identifier directly, which may be
undefined or empty, leading to key collisions in React. To fix this, add a
fallback value when entity_identifier is missing, such as a unique placeholder
string or a generated ID, ensuring each key remains unique and prevents
reconciliation issues.
Description
Fixes weird bugs that cause issues while multiple node views are rendered in the editor of the same type!
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit