Conversation
📝 WalkthroughWalkthroughAdds a new composite Popover UI (Root/Trigger/Content/Arrow) with Floating UI positioning, context, portal support, focus/escape handling, docs (page, anatomy, API tables, examples), Storybook stories, tests, styles, SEO metadata, and a docs navigation entry. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as Popover.Trigger
participant R as Popover.Root (Context)
participant F as Floating UI
participant C as Popover.Content
participant A as Popover.Arrow
U->>T: Click
T->>R: toggle open
R->>F: compute position (offset/flip/shift/arrow)
F-->>R: floating styles & placement
R-->>C: provide context (open, data, interactions)
Note over C: Content renders (optionally portalled)
C->>A: use arrowRef & placement
U->>C: Press Escape
C->>R: request close
R-->>T: restore focus
C-->>U: unmount content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 5
🧹 Nitpick comments (29)
docs/app/docs/docsNavigationSections.tsx (1)
134-137: Add “is_new” badge for Popover (optional)If you want Popover to show as newly added (like other entries), add the flag.
{ title:"Popover", path:"/docs/components/popover" + ,is_new:true },docs/app/docs/components/popover/seo.ts (1)
1-8: Type the SEO metadata for safer Next.js integrationAnnotate with
Metadataso type errors surface at build time.+import type { Metadata } from 'next'; import generateSeoMetadata from "@/utils/seo/generateSeoMetadata"; -const popoverMetadata = generateSeoMetadata({ +const popoverMetadata: Metadata = generateSeoMetadata({ title: 'Popover - Rad UI', description: 'A headless React Popover component for floating panels triggered by user interaction.' }); export default popoverMetadata;styles/themes/components/popover.scss (1)
1-10: Use design tokens (with fallbacks) and set a z-indexAlign padding/radius with spacing/radius tokens and ensure layering above content.
.rad-ui-popover { - background-color: var(--rad-ui-color-gray-50); - padding: 12px; - box-shadow: 0 10px 38px -10px #0e121659, 0 10px 20px -15px #0e121633; - border-radius: 8px; + background-color: var(--rad-ui-color-gray-50); + padding: var(--rad-ui-space-3, 12px); + box-shadow: 0 10px 38px -10px #0e121659, 0 10px 20px -15px #0e121633; + border-radius: var(--rad-ui-radius-2, 8px); + z-index: var(--rad-ui-z-index-popover, 50); .rad-ui-popover-arrow { fill: var(--rad-ui-color-gray-50); } }Note: Please verify the exact token names (
--rad-ui-space-3,--rad-ui-radius-2,--rad-ui-z-index-popover) exist in your design system; keep the fallbacks if uncertain.docs/app/docs/components/popover/docs/anatomy.tsx (1)
1-15: If rendered live, add a minimal trigger childIn case this snippet is executed (not just shown as code), include a focusable trigger to avoid an empty target.
return ( <Popover.Root> <Popover.Trigger> - {/* Your trigger element */} + <button>Open</button> </Popover.Trigger> <Popover.Content> <Popover.Arrow /> {/* Your popover content */} </Popover.Content> </Popover.Root> );src/components/ui/Popover/stories/Popover.stories.tsx (1)
6-9: Point Storybook to a concrete component for better docs/controlsUse
Popover.Rootas thecomponentso Storybook can infer props; the module objectPopoverhas no renderable props.-const meta: Meta<typeof Popover> = { - title: 'Components/Popover', - component: Popover -}; +const meta: Meta<typeof Popover.Root> = { + title: 'Components/Popover', + component: Popover.Root +};src/components/ui/Popover/tests/Popover.test.tsx (4)
1-3: Use userEvent.setup() for reliable async interactionsInitialize a user instance; it avoids flakiness with timers and mirrors current @testing-library/user-event guidance.
-import { render, screen } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; @@ - await userEvent.click(trigger); + const user = userEvent.setup(); + await user.click(trigger); @@ - await userEvent.click(trigger); + await user.click(trigger); @@ - await userEvent.click(screen.getByText('Trigger')); + const user = userEvent.setup(); + await user.click(screen.getByText('Trigger'));Also applies to: 19-22, 42-42
2-2: Avoid timing flakes when asserting dismissalIf the Content unmounts asynchronously (animations, portals), prefer waitForElementToBeRemoved.
-import { render, screen } from '@testing-library/react'; +import { render, screen, waitForElementToBeRemoved } from '@testing-library/react'; @@ - await user.click(trigger); - expect(screen.queryByText('Content')).toBeNull(); + await user.click(trigger); + await waitForElementToBeRemoved(() => screen.queryByText('Content'));Also applies to: 21-23
17-21: Prefer accessible role-based queriesMinor: query by role to better reflect the user-facing accessibility contract.
- const trigger = screen.getByText('Trigger'); + const trigger = screen.getByRole('button', { name: /trigger/i }); @@ - await user.click(screen.getByText('Trigger')); + await user.click(screen.getByRole('button', { name: /trigger/i }));Also applies to: 42-42
25-41: Verify Root ref contract (DOM vs. provider-only)If Popover.Root is a provider that renders no DOM node (common pattern), asserting an HTMLDivElement here will fail. Either ensure Root forwards a DOM ref by design, or drop this assertion.
- const rootRef = React.createRef<HTMLDivElement>(); + // If Root is provider-only, no DOM ref is expected: + // const rootRef = React.createRef<HTMLDivElement>(); @@ - <Popover.Root ref={rootRef}> + <Popover.Root> @@ - expect(rootRef.current).toBeInstanceOf(HTMLDivElement); + // If Root does not render a DOM node, remove this expectation.Would you like me to align tests with the intended Root contract across the suite?
src/components/ui/Popover/context/PopoverContext.tsx (1)
3-10: Replace any with precise types (Floating UI) when availableIf you’re using @floating-ui/react, consider exact types, e.g., ReturnType and FloatingContext, to regain type safety.
I can wire exact typings once I see PopoverRoot/Content implementations. Want me to follow up?
docs/app/docs/components/popover/docs/component_api/trigger.tsx (2)
11-14: Copy fix: article usage“as child element” → “as a child element”.
- prop: { name: 'asChild', info_tooltips: 'Render the trigger as child element.' }, + prop: { name: 'asChild', info_tooltips: 'Render the trigger as a child element.' },
1-16: Optionally enforce the shape with a shared typeIf you have a shared API table type, assert with satisfies for drift protection; also consider .ts since there’s no JSX.
-const data = { +const data = { name: 'Trigger', @@ -}; +} as const; + +export default data;And, if available:
- import the type (e.g., ApiTableData) and change to:
const data = { ... } satisfies ApiTableData;Rename to trigger.ts if consistent with the folder.
docs/app/docs/components/popover/docs/component_api/arrow.tsx (1)
1-10: LGTM; optional: type assertion and extension pointStructure looks consistent. If/when Arrow exposes props (size, offset), append rows here. Optionally add as const / satisfies to catch shape drift.
docs/app/docs/components/popover/docs/component_api/root.tsx (1)
1-31: Type-check the data object; clarify placement type if applicableOptionally assert with satisfies to keep docs in sync. If placement is a union (e.g., 'top'|'bottom'|...), consider reflecting that in the Type column for clarity.
-const data = { +const data = { name: 'Root', @@ -}; +} as const; export default data;src/components/ui/Popover/tests/Popover.behavior.test.tsx (2)
60-63: Stabilize a11y test by waiting for open content before running axeAvoid a race by ensuring the popover content is in the DOM first.
- await user.click(screen.getByText('Trigger')); + await user.click(screen.getByText('Trigger')); + await screen.findByText('Accessible'); const results = await axe(container);
21-23: Prefer role-based queries over text for the triggerUse accessible roles to reduce brittleness and improve semantics.
- const trigger = screen.getByText('Trigger'); + const trigger = screen.getByRole('button', { name: /trigger/i }); @@ - const trigger = getByText('Trigger'); + const trigger = screen.getByRole('button', { name: /trigger/i }); @@ - await user.click(screen.getByText('Trigger')); + await user.click(screen.getByRole('button', { name: /trigger/i }));Also applies to: 43-45, 61-61
docs/app/docs/components/popover/page.mdx (2)
3-3: Remove unused import
Popoverisn’t referenced in this page.-import Popover from "@radui/ui/Popover";
17-23: Fix wording: “Dismissible”Use the standard spelling.
<Documentation.ComponentFeatures features={[ "Accessible and keyboard-friendly", - "Dismissable with Escape key", + "Dismissible with Escape key", "Supports portal rendering" ]} />src/components/ui/Popover/fragments/PopoverRoot.tsx (2)
3-4: Memoize provider value to avoid needless subtree re-rendersContext value is recreated every render; memoize it.
-import React, { useRef } from 'react'; +import React, { useRef, useMemo } from 'react'; @@ - return ( - <PopoverContext.Provider value={{ open: isOpen, setOpen: setIsOpen, data, interactions, context, arrowRef }}> + const contextValue = useMemo( + () => ({ open: isOpen, setOpen: setIsOpen, data, interactions, context, arrowRef }), + [isOpen, setIsOpen, data, interactions, context] + ); + + return ( + <PopoverContext.Provider value={contextValue}> <div ref={ref} data-state={isOpen ? 'open' : 'closed'} {...dataAttributes()} {...props}> {children} </div> </PopoverContext.Provider> );Also applies to: 59-64
8-8: Drop unused data-attribute hook (it’s a no-op here)You call
useCreateDataAttributewithnull, creating an empty spread each render. Remove it for simplicity.-import { useCreateDataAttribute } from '~/core/hooks/createDataAttribute'; @@ - const dataAttributes = useCreateDataAttribute('popover', null); @@ - <div ref={ref} data-state={isOpen ? 'open' : 'closed'} {...dataAttributes()} {...props}> + <div ref={ref} data-state={isOpen ? 'open' : 'closed'} {...props}>Also applies to: 57-57, 61-61
src/components/ui/Popover/fragments/PopoverArrow.tsx (1)
12-12: Copy: align error text with public API nameSay “Popover.Root” (public API) instead of “PopoverRoot component”.
- throw new Error('PopoverArrow must be used within a PopoverRoot component'); + throw new Error('PopoverArrow must be used within a Popover.Root component');src/components/ui/Popover/Popover.tsx (1)
16-19: Silence console in production buildsGate the warning to dev to avoid noisy prod logs.
-const Popover = React.forwardRef<React.ElementRef<'div'>, React.ComponentPropsWithoutRef<'div'>>((_, __) => { - console.warn('Direct usage of Popover is not supported. Use Popover.Root etc.'); - return null; -}) as PopoverComponent; +const Popover = React.forwardRef<React.ElementRef<'div'>, React.ComponentPropsWithoutRef<'div'>>((_, __) => { + if (process.env.NODE_ENV !== 'production') { + console.warn('Direct usage of Popover is not supported. Use Popover.Root etc.'); + } + return null; +}) as PopoverComponent;src/components/ui/Popover/fragments/PopoverTrigger.tsx (2)
16-17: Copy: align error text with public API name- throw new Error('PopoverTrigger must be used within a PopoverRoot component'); + throw new Error('PopoverTrigger must be used within a Popover.Root component');
23-30: A11y: default aria-haspopup for triggersExpose popover intent to AT. Default to
dialog, allow override via props.<ButtonPrimitive asChild={asChild} ref={mergedRef} data-state={open ? 'open' : 'closed'} - {...getReferenceProps(props)} + {...getReferenceProps(props)} + aria-haspopup={(props as any)['aria-haspopup'] ?? 'dialog'} >docs/app/docs/components/popover/docs/codeUsage.js (2)
24-29: Remove unused localcolumnsconstantDead code in docs bundle.
-const columns = [ - { name: 'Prop', id: 'prop' }, - { name: 'Type', id: 'type' }, - { name: 'Default', id: 'default' }, - { name: 'Description', id: 'description' } -];
5-7: Harden source loading to avoid docs crash on missing filesWrap with a small helper and fall back to a comment if read fails.
-import { getSourceCodeFromPath } from '@/utils/parseSourceCode'; +import { getSourceCodeFromPath } from '@/utils/parseSourceCode'; @@ -const example_1_SourceCode = await getSourceCodeFromPath('docs/app/docs/components/popover/docs/examples/popover_example1.tsx'); -const anatomy_SourceCode = await getSourceCodeFromPath('docs/app/docs/components/popover/docs/anatomy.tsx'); +const safeGet = async (p) => { + try { return await getSourceCodeFromPath(p); } + catch (err) { return `// Source unavailable (${p}): ${String(err)}`; } +}; +const example_1_SourceCode = await safeGet('docs/app/docs/components/popover/docs/examples/popover_example1.tsx'); +const anatomy_SourceCode = await safeGet('docs/app/docs/components/popover/docs/anatomy.tsx');src/components/ui/Popover/fragments/PopoverContent.tsx (3)
19-20: Copy: align error text with public API name- throw new Error('PopoverContent must be used within a PopoverRoot component'); + throw new Error('PopoverContent must be used within a Popover.Root component');
45-65: A11y: add role and non-modal hint; keep user overrideNon-modal popover content should default to
role="dialog"witharia-modal={false}; let consumers overrideroleif needed.<Primitive.div ref={allRefs} tabIndex={-1} data-state={open ? 'open' : 'closed'} data-side={side} data-align={align} style={{ ...data.floatingStyles, ...style }} {...interactions.getFloatingProps({ ...props, onKeyDown: composeEventHandlers(onKeyDown, (e: React.KeyboardEvent) => { if (e.key === 'Escape') { setOpen(false); e.stopPropagation(); } }) })} + role={props.role ?? 'dialog'} + aria-modal={false} >
67-67: Nit: avoid passing null to FloatingPortal rootCoalesce to
undefinedfor default-root behavior.- return portalled ? <FloatingPortal root={container}>{element}</FloatingPortal> : element; + return portalled ? <FloatingPortal root={container ?? undefined}>{element}</FloatingPortal> : element;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
docs/app/docs/components/popover/docs/anatomy.tsx(1 hunks)docs/app/docs/components/popover/docs/codeUsage.js(1 hunks)docs/app/docs/components/popover/docs/component_api/arrow.tsx(1 hunks)docs/app/docs/components/popover/docs/component_api/content.tsx(1 hunks)docs/app/docs/components/popover/docs/component_api/root.tsx(1 hunks)docs/app/docs/components/popover/docs/component_api/trigger.tsx(1 hunks)docs/app/docs/components/popover/docs/examples/popover_example1.tsx(1 hunks)docs/app/docs/components/popover/page.mdx(1 hunks)docs/app/docs/components/popover/seo.ts(1 hunks)docs/app/docs/docsNavigationSections.tsx(1 hunks)src/components/ui/Popover/Popover.tsx(1 hunks)src/components/ui/Popover/context/PopoverContext.tsx(1 hunks)src/components/ui/Popover/fragments/PopoverArrow.tsx(1 hunks)src/components/ui/Popover/fragments/PopoverContent.tsx(1 hunks)src/components/ui/Popover/fragments/PopoverRoot.tsx(1 hunks)src/components/ui/Popover/fragments/PopoverTrigger.tsx(1 hunks)src/components/ui/Popover/stories/Popover.stories.tsx(1 hunks)src/components/ui/Popover/tests/Popover.behavior.test.tsx(1 hunks)src/components/ui/Popover/tests/Popover.test.tsx(1 hunks)styles/themes/components/popover.scss(1 hunks)styles/themes/default.scss(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.{ts,tsx}
📄 CodeRabbit inference engine (docs/.cursor/rules/posthog-integration.mdc)
In TypeScript, store feature flag names in an enum
Files:
docs/app/docs/components/popover/docs/anatomy.tsxdocs/app/docs/components/popover/seo.tsdocs/app/docs/components/popover/docs/examples/popover_example1.tsxdocs/app/docs/docsNavigationSections.tsxdocs/app/docs/components/popover/docs/component_api/trigger.tsxdocs/app/docs/components/popover/docs/component_api/root.tsxdocs/app/docs/components/popover/docs/component_api/arrow.tsxdocs/app/docs/components/popover/docs/component_api/content.tsx
docs/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (docs/.cursor/rules/posthog-integration.mdc)
docs/**/*.{ts,tsx,js,jsx}: Write enum/const object members for feature flag names in UPPERCASE_WITH_UNDERSCORE
Gate flag-dependent code with a check that verifies the flag’s values are valid and expected before use
If a custom person/event property is referenced in two or more places, define it in a central enum (TS) or const object (JS) and reuse that reference
Files:
docs/app/docs/components/popover/docs/anatomy.tsxdocs/app/docs/components/popover/seo.tsdocs/app/docs/components/popover/docs/examples/popover_example1.tsxdocs/app/docs/docsNavigationSections.tsxdocs/app/docs/components/popover/docs/component_api/trigger.tsxdocs/app/docs/components/popover/docs/component_api/root.tsxdocs/app/docs/components/popover/docs/component_api/arrow.tsxdocs/app/docs/components/popover/docs/component_api/content.tsxdocs/app/docs/components/popover/docs/codeUsage.js
docs/**/*.{js,jsx}
📄 CodeRabbit inference engine (docs/.cursor/rules/posthog-integration.mdc)
In JavaScript, store feature flag names as strings in a constant object to simulate an enum, and use a consistent naming convention
Files:
docs/app/docs/components/popover/docs/codeUsage.js
🧠 Learnings (2)
📚 Learning: 2024-12-12T08:22:59.375Z
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
Applied to files:
docs/app/docs/components/popover/docs/anatomy.tsxsrc/components/ui/Popover/fragments/PopoverTrigger.tsxsrc/components/ui/Popover/Popover.tsx
📚 Learning: 2024-12-12T08:34:33.079Z
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
src/components/ui/Popover/fragments/PopoverTrigger.tsx
🧬 Code graph analysis (9)
docs/app/docs/components/popover/docs/examples/popover_example1.tsx (1)
docs/app/docs/components/popover/docs/anatomy.tsx (1)
Popover(3-15)
docs/app/docs/components/popover/docs/component_api/trigger.tsx (1)
docs/components/layout/Documentation/helpers/ComponentHero/ComponentHero.js (1)
data(32-32)
src/components/ui/Popover/tests/Popover.behavior.test.tsx (2)
src/test-utils/portal.ts (2)
renderWithPortal(4-17)assertFocusReturn(41-43)test-utils/index.ts (2)
keyboard(64-64)axe(56-62)
docs/app/docs/components/popover/docs/component_api/arrow.tsx (1)
docs/components/layout/Documentation/helpers/ComponentHero/ComponentHero.js (1)
data(32-32)
src/components/ui/Popover/tests/Popover.test.tsx (1)
docs/app/docs/components/popover/docs/anatomy.tsx (1)
Popover(3-15)
docs/app/docs/components/popover/docs/codeUsage.js (1)
docs/utils/parseSourceCode.tsx (1)
getSourceCodeFromPath(16-62)
src/components/ui/Popover/fragments/PopoverRoot.tsx (2)
src/core/hooks/useControllableState/index.tsx (1)
useControllableState(15-78)src/core/hooks/createDataAttribute/index.ts (1)
useCreateDataAttribute(10-25)
src/components/ui/Popover/stories/Popover.stories.tsx (1)
docs/app/docs/components/popover/docs/anatomy.tsx (1)
Popover(3-15)
src/components/ui/Popover/Popover.tsx (1)
docs/app/docs/components/popover/docs/anatomy.tsx (1)
Popover(3-15)
⏰ 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). (2)
- GitHub Check: coverage
- GitHub Check: build
🔇 Additional comments (5)
styles/themes/default.scss (1)
43-43: Importing popover theme tokens — LGTMThe new
@use "components/popover";correctly wires the popover styles into the default theme.docs/app/docs/components/popover/docs/component_api/content.tsx (1)
1-19: Update description text to use “popover”name: 'Content', - description: 'The popup content.', + description: 'The popover content.',No changes needed for
info_tooltipsor theportalledprop—they match the schema and runtime implementation.docs/app/docs/components/popover/docs/examples/popover_example1.tsx (1)
10-14: Use an interactive element for Popover.Trigger child
Popover.Trigger asChild must wrap a semantic interactive element to ensure keyboard operability and correct ARIA. Replace the Card (div) wrapper with a button:- <Popover.Trigger asChild> - <Card className="bg-gray-50" style={{ display: 'inline-flex', gap: 20 }}> - <Text className="text-gray-950">Open Popover</Text> - </Card> - </Popover.Trigger> + <Popover.Trigger asChild> + <button className="bg-gray-50 inline-flex gap-5 px-3 py-2 text-gray-950"> + Open Popover + </button> + </Popover.Trigger>Remove the now-unused
CardandTextimports. Manually verify keyboard focus management, Enter/Space activation, and update or add behavior tests accordingly.src/components/ui/Popover/tests/Popover.test.tsx (1)
1-1: jest-dom matchers already configured globally
The Jest config’ssetupFilesAfterEnvpoints tosrc/setupTests.ts, which imports@testing-library/jest-dom, sotoBeInTheDocument()is available in all tests.src/components/ui/Popover/context/PopoverContext.tsx (1)
12-12: Remove null-check audit note for PopoverContext usage
No direct calls touseContext(PopoverContext)were found in the codebase; theusePopoverContext()helper suffices.
🦋 Changeset detectedLatest commit: 3a83a35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage decreased for at least one metric. Please add or update tests to improve coverage. Run |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/ui/Popover/stories/Popover.stories.tsx (1)
22-22: Addtype="button"to prevent implicit form submission.Buttons without an explicit
typeattribute default totype="submit"inside forms, which can cause unexpected behavior.♻️ Proposed fix
- <button className='bg-gray-200 p-2'>Open</button> + <button type="button" className='bg-gray-200 p-2'>Open</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Popover/stories/Popover.stories.tsx` at line 22, The button element used as the Popover trigger (the <button className='bg-gray-200 p-2'>Open</button> in Popover.stories.tsx) lacks an explicit type and can implicitly act as type="submit" inside forms; update that button to include type="button" so it won't trigger form submission (modify the JSX for the Popover trigger button to add type="button").src/components/tools/SandboxEditor/SandboxEditor.tsx (1)
76-91: Usecolorinstead ofidxfor the key prop.Using array index as a key can cause issues with React's reconciliation if the list order changes. Since
color(the color name) is unique and stable, it's a better key.♻️ Proposed fix
- <button - key={idx} + <button + key={color}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tools/SandboxEditor/SandboxEditor.tsx` around lines 76 - 91, The map callback in SandboxEditor uses the array index (idx) as the React key which can break reconciliation; change the key on the button to use the stable unique color identifier instead (the local variable color / accentColorName) so replace key={idx} with key={color} (or key={accentColorName}) in the loop that renders <button> with onClick={() => setColorName(accentColorName)} and <ColorSelect>.src/components/ui/Popover/fragments/PopoverArrow.tsx (1)
13-13: Consider adding accessibility attributes for the decorative arrow.The arrow is purely decorative and should be hidden from assistive technologies.
♻️ Optional improvement
- return <FloatingArrow ref={mergedRef} context={popover.context} {...rest} className={clsx('rad-ui-popover-arrow', className)} />; + return <FloatingArrow ref={mergedRef} context={popover.context} {...rest} className={clsx('rad-ui-popover-arrow', className)} aria-hidden="true" />;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Popover/fragments/PopoverArrow.tsx` at line 13, The arrow is decorative and must be hidden from assistive tech; update the JSX for the FloatingArrow element in PopoverArrow.tsx to include accessibility attributes (e.g., aria-hidden="true" and role="presentation") so screen readers ignore it, making sure these explicit attributes are applied after the {...rest} spread on the FloatingArrow element (refer to the FloatingArrow JSX, mergedRef, popover.context and className usage) to prevent them from being accidentally overridden.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/tools/SandboxEditor/SandboxEditor.tsx`:
- Around line 76-91: The map callback in SandboxEditor uses the array index
(idx) as the React key which can break reconciliation; change the key on the
button to use the stable unique color identifier instead (the local variable
color / accentColorName) so replace key={idx} with key={color} (or
key={accentColorName}) in the loop that renders <button> with onClick={() =>
setColorName(accentColorName)} and <ColorSelect>.
In `@src/components/ui/Popover/fragments/PopoverArrow.tsx`:
- Line 13: The arrow is decorative and must be hidden from assistive tech;
update the JSX for the FloatingArrow element in PopoverArrow.tsx to include
accessibility attributes (e.g., aria-hidden="true" and role="presentation") so
screen readers ignore it, making sure these explicit attributes are applied
after the {...rest} spread on the FloatingArrow element (refer to the
FloatingArrow JSX, mergedRef, popover.context and className usage) to prevent
them from being accidentally overridden.
In `@src/components/ui/Popover/stories/Popover.stories.tsx`:
- Line 22: The button element used as the Popover trigger (the <button
className='bg-gray-200 p-2'>Open</button> in Popover.stories.tsx) lacks an
explicit type and can implicitly act as type="submit" inside forms; update that
button to include type="button" so it won't trigger form submission (modify the
JSX for the Popover trigger button to add type="button").
1 participant
Summary
Testing
npm test src/components/ui/Popover/tests/Popover.test.tsx src/components/ui/Popover/tests/Popover.behavior.test.tsxSummary by CodeRabbit