Fixed React development mode warnings in admin-x-design-system#25650
Fixed React development mode warnings in admin-x-design-system#25650kevinansfield wants to merge 1 commit intomainfrom
Conversation
no issue Some aspects of `admin-x-design-system` and its usage within `admin-x-settings` caused a lot of errors to be output in the console in development mode making it difficult to find real problems. - Added missing `key` props to list items in `TabView`, `ButtonGroup`, `CheckboxGroup`, and `TiersList` to fix "unique key" warnings - Added `asChild` to `Tooltip`'s `TooltipPrimitive.Trigger` to fix nested button warning when Button components are wrapped in Tooltip - Destructured unused `separator` prop in NewsletterItemContainer to prevent it being passed to `DragIndicator` causing errors
WalkthroughThis pull request improves React rendering stability and component structure across design system and settings components. Key-related changes are made to list-rendered elements in button-group, checkbox-group, tab-view, and tiers-list to ensure deterministic React keys with fallback synthesis. The button-group introduces centralized key management through a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (4)
apps/admin-x-design-system/src/global/tab-view.tsx (1)
92-103: Tab keys are now stable; consider avoiding the extra wrapper elementUsing
tab.idas the key in both the tab list andTabsPrimitive.Contentis correct and should eliminate the key warnings while keeping state aligned with the Radixvalue.You likely don’t need the extra
<div>aroundTabButtonpurely for the key, though. If layout allows, you could instead key the button itself (or use a keyedReact.Fragment) to avoid an extra DOM node:{tabs.map(tab => ( <TabButton key={tab.id} border={buttonBorder} counter={tab.counter} icon={tab.icon} id={tab.id} title={tab.title} onClick={handleTabChange} /> ))}This is a minor, optional cleanup; behavior-wise the current change is fine.
Please verify that no CSS relies specifically on that wrapper
<div>(e.g., targeting children count or direct descendants) before inlining the key as above.Also applies to: 163-169
apps/admin-x-settings/src/components/settings/email/newsletters/newsletters-list.tsx (1)
14-24: Separator handling inNewsletterItemContaineris safe and clearDestructuring
separatorand thenvoid‑ing it before spreading...propsintoDragIndicatorneatly prevents the prop from leaking into the drag handle while still allowing it on the container type.If your lint setup supports the conventional “unused underscore” pattern, an alternative is:
const NewsletterItemContainer: React.FC<Partial<SortableItemContainerProps>> = ({ separator: _separator, ...props }) => { /* ... */ }but what you have is perfectly valid.
apps/admin-x-design-system/src/global/form/checkbox-group.tsx (1)
23-25: Deterministic key fallback is good; be mindful if the list is reorderableUsing
key ?? \checkbox-${index}`` will remove key warnings when callers don’t provide a key and is a reasonable fallback for mostly static checkbox lists.If you ever expect
checkboxesto be dynamically reordered/inserted/removed, it’s worth encouraging callers to pass a stable semantic key (e.g., an id) instead of relying on the index, to avoid unnecessary remounts and state loss inside individualCheckboxinstances.It may be worth scanning current
CheckboxGroupcall sites to confirm they’re using stable keys (or static arrays) and won’t rely on stateful checkboxes that could be impacted by index‑based keys.apps/admin-x-design-system/src/global/button-group.tsx (1)
42-65: CentralizedbuttonKeyhandling is good; just be cautious with index-based identityUsing
{key, ...props}plusconst buttonKey = key ?? \button-${index}`` nicely fixes React key warnings and unifies the key used for:
- the React list key (
key={buttonKey}onTooltip/Button), and- the active state check (
buttonKey === activeKey).For buttons that do specify a
key, behavior is preserved and more robust.A couple of small considerations:
- For buttons without an explicit
key, their identity is now effectively “button-${index}”. That’s fine for static arrays, but if a caller ever wants to make such a button active viaactiveKey, they’d have to know that synthesized value—and any reordering would change it.- If there’s any chance the
buttonsarray becomes dynamic, it may be worth nudging consumers towards always providing a stable semantic key (e.g.,id) and treating the index fallback as a last resort only.The added
border-transparentfor inactive buttons is also a nice touch to keep border behavior explicit.Consider auditing
ButtonGroupcall sites that useactiveKeyto ensure they always pair it with an explicitkeyon the corresponding button config, avoiding reliance on the synthesizedbutton-${index}form.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/admin-x-design-system/src/global/button-group.tsx(2 hunks)apps/admin-x-design-system/src/global/form/checkbox-group.tsx(1 hunks)apps/admin-x-design-system/src/global/tab-view.tsx(2 hunks)apps/admin-x-design-system/src/global/tooltip.tsx(1 hunks)apps/admin-x-settings/src/components/settings/email/newsletters/newsletters-list.tsx(1 hunks)apps/admin-x-settings/src/components/settings/membership/tiers/tiers-list.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Applied to files:
apps/admin-x-design-system/src/global/tab-view.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Applied to files:
apps/admin-x-design-system/src/global/tab-view.tsx
🧬 Code graph analysis (1)
apps/admin-x-design-system/src/global/button-group.tsx (1)
apps/shade/src/components/ui/tooltip.tsx (1)
Tooltip(33-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (2)
apps/admin-x-settings/src/components/settings/membership/tiers/tiers-list.tsx (1)
72-74: Stable key onTierCardlooks goodUsing
tier.idas the React key is a solid choice here and will clean up the dev warnings, assumingidis always defined and unique within the list.Please just double‑check that
Tier[]can’t contain duplicate/undefinedidvalues for this view.apps/admin-x-design-system/src/global/tooltip.tsx (1)
26-32:asChildusage correctly avoids nested interactive elementsSwitching the Radix
TooltipPrimitive.TriggertoasChildwhile keepingonClick={event => event.preventDefault()}is a solid way to avoid nested<button>warnings when wrappingButtoncomponents, without changing existing click behavior.One implication is that
Tooltipnow effectively requires a single valid React element as its child (no strings or multiple siblings), which matches typical usages likeTooltip><Button /></Tooltip>.Please confirm your Radix version fully supports
asChildonTooltipPrimitive.Triggerand that allTooltipusages pass exactly one React element child.
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20030850384 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 similar comment
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20030850384 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
2 participants
no issue
Some aspects of
admin-x-design-systemand its usage withinadmin-x-settingscaused a lot of errors to be output in the console in development mode making it difficult to find real problems.keyprops to list items inTabView,ButtonGroup,CheckboxGroup, andTiersListto fix "unique key" warningsasChildtoTooltip'sTooltipPrimitive.Triggerto fix nested button warning when Button components are wrapped in Tooltipseparatorprop in NewsletterItemContainer to prevent it being passed toDragIndicatorcausing errors