fix: unique key prop warnings for all components #6775
Conversation
WalkthroughThis PR adds stable React keys to markdown and message block rendering components by implementing client-side ID generation at the block and child element level. A recursive Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/containers/UIKit/MessageBlock.tsx(1 hunks)app/containers/markdown/components/Inline.tsx(2 hunks)app/containers/markdown/components/emoji/BigEmoji.tsx(1 hunks)app/containers/markdown/components/inline/Bold.tsx(1 hunks)app/containers/markdown/components/list/UnorderedList.tsx(1 hunks)app/containers/markdown/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/containers/markdown/components/Inline.tsx (2)
app/containers/markdown/Markdown.stories.tsx (4)
Image(121-125)Emoji(94-101)Hashtag(88-92)Text(51-59)app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(9-17)
app/containers/markdown/index.tsx (2)
app/containers/markdown/components/LineBreak.tsx (1)
LineBreak(4-6)app/containers/markdown/components/Katex.tsx (1)
KaTeX(16-28)
app/containers/markdown/components/list/UnorderedList.tsx (1)
app/lib/constants/colors.ts (1)
themes(304-304)
app/containers/UIKit/MessageBlock.tsx (1)
app/containers/UIKit/index.tsx (1)
UiKitMessage(211-211)
🔇 Additional comments (7)
app/containers/markdown/components/emoji/BigEmoji.tsx (1)
19-20: LGTM! Good key generation strategy.The key generation appropriately prioritizes meaningful identifiers (shortCode or unicode) before falling back to the index, ensuring both uniqueness and stability.
app/containers/markdown/index.tsx (1)
78-100: Keys applied consistently across all block types.All rendered block components now receive the generated key prop, which resolves the React warning. The switch statement covers all block types appropriately.
app/containers/markdown/components/list/UnorderedList.tsx (1)
18-28: LGTM! Key generation ensures uniqueness.The indexed map with a composite key (
${value}-${index}) ensures uniqueness. The optional chaining ontoString?.()is good defensive coding.app/containers/markdown/components/inline/Bold.tsx (1)
28-45: Keys applied consistently to all block types.All cases in the switch statement now receive the generated key prop, properly resolving React key warnings.
app/containers/markdown/components/Inline.tsx (1)
52-87: Keys applied comprehensively across all inline block types.All block types, including AtMention, now receive unique keys. The key generation strategy ensures stability and uniqueness.
app/containers/UIKit/MessageBlock.tsx (2)
13-19: LGTM! Proper handling of array rendering with stable keys.The implementation correctly:
- Guards against null/undefined blocks
- Checks if the result is an array before mapping
- Uses
blockIdwhen available (stable identifier) before falling back to index- Wraps each element in a Fragment with a unique key
21-26: Context merging looks appropriate.The spread of both
contextanddatainto the KitContext.Provider value ensures all necessary properties are available to the ModalBlock component.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/containers/markdown/components/Inline.tsx(3 hunks)app/containers/markdown/components/inline/Bold.tsx(2 hunks)app/containers/markdown/index.tsx(2 hunks)app/lib/methods/getBlockValueString.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/containers/markdown/components/Inline.tsx
- app/containers/markdown/index.tsx
- app/containers/markdown/components/inline/Bold.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/lib/methods/getBlockValueString.ts (1)
1-19: Excellent implementation—all past feedback addressed!The function now properly handles distinct falsy values, uses a separator for array joins, includes clear JSDoc, and has a reasonable truncation limit. This will generate stable, debuggable keys for React rendering.
One minor consideration:
JSON.stringifyon line 17 can throw on circular references orBigIntvalues. However, given the context (Markdown/UIKit blocks), these edge cases are unlikely. If you want to be defensive, you could wrap it in a try-catch and return a fallback like'[object]':- return JSON.stringify(v).slice(0, 50); + try { + return JSON.stringify(v).slice(0, 50); + } catch { + return '[object]'; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/containers/markdown/components/Inline.tsx(3 hunks)app/containers/markdown/components/inline/Bold.tsx(2 hunks)app/containers/markdown/index.tsx(2 hunks)app/lib/methods/getBlockValueString.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/containers/markdown/components/Inline.tsx
- app/containers/markdown/components/inline/Bold.tsx
- app/containers/markdown/index.tsx
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Using index as key is never a good idea https://www.epicreact.dev/why-react-needs-a-key-prop
|
Updated the PR |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/containers/markdown/components/list/UnorderedList.tsx (1)
20-30: LGTM: Stable keys for list items.Using
item._idas the key for each list item View is the correct approach.Note: The same type assertion concern mentioned in Code.tsx applies here - ensure
_idis populated upstream.app/containers/markdown/components/Quote.tsx (1)
22-25: LGTM: Stable keys for quote paragraphs.Each Paragraph component now has a stable key via
item._id.Note: The same type assertion concern mentioned in Code.tsx applies here.
app/containers/markdown/components/inline/Strike.tsx (1)
22-38: LGTM: All inline elements properly keyed.All rendered components (Link, Plain, Bold, Italic) now have stable keys via
block._id.Note: The same type assertion concern mentioned in Code.tsx applies here.
app/containers/markdown/components/inline/Italic.tsx (2)
22-38: LGTM: All inline elements properly keyed.All rendered components (Link, Plain, Strike, Bold) now have stable keys via
block._id.Note: The same type assertion concern mentioned in Code.tsx applies here.
18-18: Consider simplifying the generic type.Similar to Strike.tsx, the generic parameter in
TItalicWithID<T>is unnecessary. A simpler type would be:type TItalicBlockWithID = ItalicProps['value'][number] & { _id: string };
🧹 Nitpick comments (1)
app/containers/markdown/components/inline/Strike.tsx (1)
18-18: Consider simplifying the generic type.
TStrikeWithID<T>uses a generic parameter, butTis alwaystypeof bfrom the map callback. A simpler non-generic type would suffice:type TStrikeBlockWithID = StrikeProps['value'][number] & { _id: string };Then use:
const block = b as TStrikeBlockWithID;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
app/containers/UIKit/MessageBlock.tsx(1 hunks)app/containers/markdown/components/Inline.tsx(4 hunks)app/containers/markdown/components/Quote.tsx(2 hunks)app/containers/markdown/components/code/Code.tsx(3 hunks)app/containers/markdown/components/emoji/BigEmoji.tsx(2 hunks)app/containers/markdown/components/inline/Bold.tsx(1 hunks)app/containers/markdown/components/inline/Italic.tsx(1 hunks)app/containers/markdown/components/inline/Strike.tsx(1 hunks)app/containers/markdown/components/list/TaskList.tsx(2 hunks)app/containers/markdown/components/list/UnorderedList.tsx(2 hunks)app/containers/markdown/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/containers/markdown/components/emoji/BigEmoji.tsx
- app/containers/markdown/index.tsx
- app/containers/markdown/components/Inline.tsx
- app/containers/markdown/components/inline/Bold.tsx
- app/containers/markdown/components/list/TaskList.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
app/containers/markdown/components/Quote.tsx (2)
app/theme.tsx (1)
useTheme(29-29)app/lib/constants/colors.ts (1)
themes(304-304)
app/containers/markdown/components/code/Code.tsx (1)
app/containers/markdown/components/code/index.ts (1)
CodeLine(4-4)
app/containers/markdown/components/list/UnorderedList.tsx (3)
app/containers/markdown/components/list/index.ts (1)
UnorderedList(5-5)app/theme.tsx (1)
useTheme(29-29)app/lib/constants/colors.ts (1)
themes(304-304)
app/containers/UIKit/MessageBlock.tsx (1)
app/containers/UIKit/index.tsx (1)
UiKitMessage(211-211)
🔇 Additional comments (3)
app/containers/markdown/components/code/Code.tsx (2)
27-31: Good: Stable key usage with_id.The switch from index-based keys to
block._idresolves the React warning and provides stable keys across renders.
13-13: I'm unable to complete the verification as the repository clone operation failed. Without direct access to the codebase, I cannot:
- Examine the Code component implementation at lines 13-28
- Verify how and where
assignIdsis called- Trace data flow to confirm
_idassignment before rendering- Identify all code paths leading to the Code component
To proceed, I would need either:
- Resolution of the repository access issue, or
- You to provide the relevant code sections (Code.tsx, usage locations, assignIds implementation)
Could you provide the code or confirm the repository access can be restored?
app/containers/UIKit/MessageBlock.tsx (1)
28-28: I'm unable to verify the review comment due to repository access limitations and insufficient public documentation about the specific return types of these UIKit rendering functions. Let me mark this for manual verification:Unable to verify if
UiKitModalreturns arrays that would require the suggested defensive handling pattern.The review comment's suggestion depends on whether
UiKitModal(blocks)can return an array of elements (similar toUiKitMessage). Without access to:
- The actual implementation of
UiKitModalandUiKitMessage- The current implementation of
MessageBlockto confirm it uses the suggested pattern- The repository code to trace these function behaviors
I cannot confirm whether this suggestion is necessary or accurate.
To resolve this review comment, manually verify:
- Check if
UiKitModal(blocks)can return an array of React elements- Check if
UiKitMessage(blocks)returns arrays and whetherMessageBlockimplements the suggested defensive handling- If both can return arrays, apply the same pattern to
ModalBlockfor consistency
| return Array.isArray(renderedBlocks) | ||
| ? renderedBlocks.map(element => <React.Fragment key={element.props.blockId}>{element}</React.Fragment>) |
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
#!/bin/bash
# Search for UiKitMessage implementation and blockId assignment
rg -n "blockId" --type ts --type tsx -C 3 | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Find UiKitMessage definition
rg -n "UiKitMessage" --type ts --type tsx -C 3 | head -80Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Look at the actual MessageBlock file to see full context
cat -n app/containers/UIKit/MessageBlock.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
The repository cannot be cloned in the sandbox environment. However, I can still provide a rewritten review comment based on the context provided and common React/TypeScript patterns.
Let me analyze the original review comment:
Key observations from the provided context:
- The code uses
element.props.blockIdas the React key without any fallback - The AI summary mentioned a fallback mechanism, but the actual code doesn't show one
- The concern is valid: if
blockIdis undefined/null, multiple fragments could share the same key
Analysis:
- The original review correctly identifies a potential issue: React will warn if multiple elements share
undefinedas a key - Whether this is actually a problem depends on whether
UiKitMessagealways providesblockIdon rendered elements - Without access to the implementation, the safest approach is to flag this as requiring verification by the developer
Since I cannot verify the codebase directly, the review comment should be rewritten to be more precise about what needs verification while preserving the valid concern:
Consider adding a fallback for blockId if it can be undefined.
The current implementation sets the key to element.props.blockId without a fallback. If any rendered element lacks a blockId property, React will warn about duplicate keys. Verify that UiKitMessage guarantees blockId on all rendered elements; if not, add an index fallback:
- return Array.isArray(renderedBlocks)
- ? renderedBlocks.map(element => <React.Fragment key={element.props.blockId}>{element}</React.Fragment>)
+ return Array.isArray(renderedBlocks)
+ ? renderedBlocks.map((element, index) => (
+ <React.Fragment key={element.props.blockId ?? `block-${index}`}>{element}</React.Fragment>
+ ))📝 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.
| return Array.isArray(renderedBlocks) | |
| ? renderedBlocks.map(element => <React.Fragment key={element.props.blockId}>{element}</React.Fragment>) | |
| return Array.isArray(renderedBlocks) | |
| ? renderedBlocks.map((element, index) => ( | |
| <React.Fragment key={element.props.blockId ?? `block-${index}`}>{element}</React.Fragment> | |
| )) |
🤖 Prompt for AI Agents
In app/containers/UIKit/MessageBlock.tsx around lines 16 to 17, the mapped
Fragment uses element.props.blockId as the React key with no fallback; ensure
keys are always stable by verifying UiKitMessage guarantees a non-empty blockId
or change the mapping to derive a unique key when blockId is missing (for
example use the map index or a deterministic fallback like
`${element.type}-${index}`), and update the code to use that fallback so React
never receives duplicate/undefined keys.
|
updated the PR, every direct or nested block now contains |
| switch (block.type) { | ||
| case 'CODE_LINE': | ||
| return <CodeLine value={block.value} />; | ||
| return <CodeLine key={block._id} value={block.value} />; |
There was a problem hiding this comment.
in app/containers/markdown/index.tsx
after the tokens are parsed we add _id field in it
i also tested in my local _id field always contains unique 16 char value
There was a problem hiding this comment.
ids are assigned to nested blocks also, so even if the block internally maps the tokens again like BigEmoji works perfectly with this keys
2 participants
Proposed changes
This PR resolves the React warning
Each child in a list should have a unique key prop._idto each parsed token inapp/containers/markdown/index.tsxCloses #4943
files changed
app/containers/markdown/components/code/Code.tsxapp/containers/markdown/components/emoji/BigEmoji.tsxapp/containers/markdown/components/inline/Bold.tsxapp/containers/markdown/components/inline/Italic.tsxapp/containers/markdown/components/inline/Strike.tsxapp/containers/markdown/components/list/UnorderedList.tsxapp/containers/markdown/components/list/TaskList.tsxapp/containers/markdown/components/Inline.tsxapp/containers/markdown/index.tsxapp/containers/UIKit/MessageBlock.tsxapp/containers/markdown/index.tsx: generate unique id and assigns them to each parsed token so now every block contains._idfieldHow to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.