-
Notifications
You must be signed in to change notification settings - Fork 19
Fix unstable key usage in protocol card list #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughRemoved commented header blocks on the Protocols page, replaced index-based Card keys with Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/protocols/page.tsx (1)
102-102: Remove the unusedindexprop from the Card component.The
indexprop is declared in the Card interface and destructured but never used in the component. Now that the key is based onprotocol.title, theindexprop can be safely removed from both the Card component definition (components/card.tsx) and the prop passed in app/protocols/page.tsx.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/protocols/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/protocols/page.tsx (1)
components/container.tsx (1)
Container(1-15)
🔇 Additional comments (3)
app/protocols/page.tsx (3)
101-101: Excellent fix for React key stability!Using
protocol.titleas the key instead of the array index ensures stable component identity across re-renders. All protocol titles in theprotocols1array are unique, making this a reliable and stable key.
86-120: Note: Mismatch between PR description and linked issue.The PR description references issue #28 ("Add footer"), but the actual changes in this PR are focused on fixing unstable key usage in the protocol card list. The linked issue objectives don't appear to relate to the key stability fix implemented here.
Consider verifying that the correct issue is linked or updating the PR description to accurately reflect the changes.
112-117: No action required. The CTA component properly accepts and implements thelinkandlinkTextprops as shown in its definition (components/cta.tsx lines 3-27). The component signature includes both props in the Props interface, and they are correctly used in the Link component and span rendering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/card.tsx (1)
33-40: Fix: Same unstable key pattern found inside Card component.While this PR fixes unstable keys at the page level, the same anti-pattern remains here. The map function uses array index (
key) as the React key for description line items. If description lines are reordered or modified, React reconciliation will be unstable.🔎 Proposed fix using content-based key
- {description.split("\n").map((item, key) => { + {description.split("\n").map((item, index) => { return ( - <span key={key}> + <span key={`${item}-${index}`}> {item} <br /> </span> ) })}Note: Using a composite key
${item}-${index}provides better stability when content changes, though if line content is unique,key={item}alone would suffice.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/protocols/page.tsxcomponents/card.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/protocols/page.tsx (1)
components/container.tsx (1)
Container(1-15)
🔇 Additional comments (3)
components/card.tsx (1)
4-12: LGTM! Props interface and signature correctly updated.The removal of the unused
indexprop from both the Props interface and component signature is correct and aligns with the PR objective of using stable keys instead of array indices.app/protocols/page.tsx (2)
112-117: The CTA component correctly accepts thelinkandlinkTextprops. The component's Props interface (lines 3-8 incomponents/cta.tsx) defines both properties, and they are properly used within the component—linkis passed to the Next.js Link href attribute, andlinkTextrenders as the button label. No issues found.
99-107: Good fix—using stable, unique values as React keys.The change from
key={index}tokey={protocol.title}is correct. All protocol titles in the array are unique and hardcoded, making them suitable as stable keys for proper React reconciliation.If protocol objects are ever extended with additional unique identifiers (like IDs fetched from an API or generated dynamically), consider using those as keys instead to future-proof against title changes or duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/card.tsx (1)
33-41: Consider reverting to index-only key for description lines.The composite key
${item}-${index}introduces risk without solving a real problem:
- Duplicate key risk: If the description contains duplicate lines (e.g., repeated text or multiple blank lines), you'll get duplicate keys and React warnings.
- No instability issue here: These are static description lines derived from splitting a string—they won't dynamically reorder or change independently.
- Index is appropriate: React recommends using index as key for static lists that won't reorder, which is exactly this use case.
The main stable key fix was correctly applied at the parent level (using
protocol.titlefor Card components). For these internal description lines, reverting tokey={index}would be more appropriate and eliminate the duplicate key risk.🔎 Suggested fix
- {description.split("\n").map((item, index) => { + {description.split("\n").map((item, idx) => { return ( - <span key={`${item}-${index}`}> + <span key={idx}> {item} <br /> </span> ) })}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/card.tsx
🔇 Additional comments (1)
components/card.tsx (1)
4-11: LGTM! Clean removal of unused index prop.The removal of the
indexprop from the component interface and signature is consistent and aligns with the parent page now usingprotocol.titleas a stable key for Card components in the list.
This PR fixes unstable React key usage in the protocol card list.
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.