fix: unique key prop warnings for all components#6775
fix: unique key prop warnings for all components#6775divyanshu-patil wants to merge 12 commits intoRocketChat:developfrom
Conversation
WalkthroughAdds client-side ID assignment and stable React keys across markdown rendering and message/block components; refactors iteration to cast blocks with an added Changes
Sequence Diagram(s)(omitted — changes are internal rendering/stable-key additions and do not introduce multi-component sequential flows requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
hello maintainers, |
|
I have some questions
|
ans to que 1
ex: scrolling in same chat up and down changes id cause value of token changes eventually creating new id ans to que 2
|
divyanshu-patil
force-pushed
the
fix/unique_key_prop_clean
branch
from
7101c1b to
810b56d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/markdown/components/Inline.tsx (1)
79-79: Critical: Missing key prop on TIMESTAMP case.The
<Timestamp>component is rendered within the mapped array but lacks akeyprop, which will trigger the React warning this PR aims to fix.🔑 Proposed fix
case 'TIMESTAMP': - return <Timestamp value={block.value} />; + return <Timestamp key={block._id} value={block.value} />;
🤖 Fix all issues with AI agents
In @app/containers/markdown/components/Inline.tsx:
- Around line 21-28: The Inline component is using a transient _id on each token
(TInlineWithID / value in Inline) that is regenerated on every render causing
unstable React keys; instead, compute a stable key before render from intrinsic
token properties (e.g., token type + position/index + content hash or a stable
token id) or ensure the _id is assigned once when tokens are parsed (upstream)
rather than inside the render path; update the map call in Inline to use that
stable key (token.type/tokenPosition/tokenTextHash) for React children keys and
remove reliance on ephemeral _id generation so keys remain stable across
re-renders.
- Around line 27-28: The code unsafely asserts each mapped item as TInlineWithID
and directly uses block._id; add a runtime type guard (e.g., hasId(block): block
is TInlineWithID that checks '_id' in block and typeof _id === 'string') inside
the value.map callback, use that guard to narrow the type before referencing
block._id, and in the else branch either generate/assign a stable id or
skip/render a fallback to avoid undefined keys; update the mapping logic around
value.map and the b/block handling to use this guard.
- Around line 76-77: The INLINE_KATEX branch returns a JSX element without a key
(return <Text>{block.value}</Text>), causing the "Each child in a list should
have a unique key prop" warning; fix by returning a keyed element in the
INLINE_KATEX case (preferably use the intended InlineKaTeX component) and set a
stable key derived from the block (e.g., block.id) or fallback to the loop index
(e.g., key={block.id ?? idx}) so the returned element from the INLINE_KATEX case
has a unique key.
📜 Review details
Configuration used: Organization 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 (1)
app/containers/markdown/components/Inline.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/markdown/components/Inline.tsx (3)
app/containers/markdown/components/inline/index.ts (4)
Bold(6-6)Strike(6-6)Italic(6-6)Link(6-6)app/containers/markdown/components/mentions/index.tsx (2)
AtMention(4-4)Hashtag(4-4)app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(9-17)
🔇 Additional comments (2)
app/containers/markdown/components/Inline.tsx (2)
3-3: LGTM!The type imports and
TInlineWithIDalias correctly model inline blocks with the added_idfield.Also applies to: 21-22
47-47: LGTM! Consistent key props added.All these inline block types now receive stable
key={block._id}props, resolving the React warning for list rendering.Also applies to: 49-49, 51-51, 53-53, 55-55, 57-57, 61-61, 70-70, 72-72, 74-74
|
hello sir, |
3 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
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.