Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughIntroduces a new PreviewCard component with Trigger/Content/Viewport subcomponents, portal-based positioning, optional arrow rendering, CSS animations, tests, documentation, playground examples, and exports the component from the public library surface. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger as PreviewCard.Trigger
participant Content as PreviewCard.Content
participant Portal
participant Positioner
participant Popup as BaseUI.Popup
participant Arrow
User->>Trigger: Hover / Focus
Trigger->>Content: request open (onOpenChange)
Content->>Portal: render content in body (Portal)
Portal->>Positioner: compute side/align/offset
Positioner->>Popup: position and mount
Popup->>Arrow: render arrow if showArrow=true
Popup->>Popup: apply entrance animation
User->>Trigger: leave / blur
Content->>Popup: apply exit animation
Popup->>Portal: unmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/add-new-component/SKILL.md:
- Around line 118-176: The Component.Content component merges Positioner and
Popup props via ComponentContentProps but currently spreads all remaining props
into ComponentPrimitive.Positioner inside ComponentContent, causing Popup-only
props (e.g., initialFocus, finalFocus, onOpenAutoFocus) to be dropped or
misrouted; fix by explicitly destructuring Popup-specific props (at least the
known Popup props like render, initialFocus, finalFocus, onOpenAutoFocus,
onCloseAutoFocus, etc.) from the function args and pass those to
ComponentPrimitive.Popup, while spreading the remaining props into
ComponentPrimitive.Positioner so Positioner receives only its props and Popup
receives its own; update ComponentContent signature and the destructure in
ComponentContent to reflect this split.
In `@apps/www/src/content/docs/components/preview-card/props.ts`:
- Around line 1-33: PreviewCardRootProps is missing the shared handle prop and
PreviewCardTriggerProps is missing both handle and payload; add an optional
handle prop to PreviewCardRootProps and add optional handle and payload props to
PreviewCardTriggerProps so triggers and root can be linked via createHandle();
type the handle using the same Handle/ReturnType used by createHandle() in your
project (or generically/with generics if createHandle is generic) and set
payload to the appropriate payload type used by the demo (or any/unknown/generic
if payload can vary), and ensure prop names exactly match "handle" and "payload"
so the documented usage works with createHandle() and the trigger-root
association.
In `@packages/raystack/components/preview-card/__tests__/preview-card.test.tsx`:
- Around line 49-55: The test "renders in portal" in preview-card.test.tsx
currently asserts content.closest('body') which is tautological; change the test
to capture the render container (from render(<BasicPreviewCard open={true} />)
e.g., const { container } = ...) and then assert that the portal content (found
via screen.getByText(CARD_CONTENT)) is not a descendant of that container (e.g.,
expect(container).not.toContainElement(content}) or otherwise assert the
content's parent is document.body but not the render container; update the test
around the BasicPreviewCard render and screen.getByText usage accordingly.
In `@packages/raystack/components/preview-card/preview-card.module.css`:
- Around line 28-33: The .popup CSS block contains unexpected blank lines before
declarations (e.g., the lines before width, height, and transition-* properties)
causing a declaration-empty-line-before Stylelint error; edit the .popup
selector in preview-card.module.css (the block containing width, height,
transition-property, transition-timing-function, transition-duration) and remove
the extra empty lines so declarations appear consecutively with no blank lines
between them.
In `@packages/raystack/components/preview-card/preview-card.tsx`:
- Around line 8-49: The component forwards all remaining props to
PreviewCardPrimitive.Positioner but its type includes Popup props, causing
popup-specific attributes to be dropped; in PreviewCardContent destructure the
positioning props (side, align, sideOffset, alignOffset, collisionPadding) out
of the incoming props and pass only those to PreviewCardPrimitive.Positioner,
while forwarding the rest (including aria-*, id, role, event handlers, etc.) to
PreviewCardPrimitive.Popup (keep render, className, style, children handling
as-is) so Positioner only receives layout props and Popup receives the
accessibility/DOM props.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.claude/skills/add-new-component/SKILL.mdapps/www/src/components/playground/index.tsapps/www/src/components/playground/preview-card-examples.tsxapps/www/src/content/docs/components/preview-card/demo.tsapps/www/src/content/docs/components/preview-card/index.mdxapps/www/src/content/docs/components/preview-card/props.tspackages/raystack/components/preview-card/__tests__/preview-card.test.tsxpackages/raystack/components/preview-card/index.tsxpackages/raystack/components/preview-card/preview-card.module.csspackages/raystack/components/preview-card/preview-card.tsxpackages/raystack/index.tsx
Comment on lines
+118
to
+176
| **Content Props Interface:** Merge Positioner props with Popup/Panel props so positioning config (`side`, `align`, `sideOffset`, etc.) is passed directly on `<Component.Content>`. Separate them internally via rest spread: | ||
|
|
||
| ```tsx | ||
| export interface ComponentContentProps | ||
| extends Omit< | ||
| ComponentPrimitive.Positioner.Props, | ||
| 'render' | 'className' | 'style' | ||
| >, | ||
| ComponentPrimitive.Popup.Props { | ||
| /** @default false */ | ||
| showArrow?: boolean; | ||
| } | ||
| ``` | ||
|
|
||
| **Content Component Template:** | ||
|
|
||
| ```tsx | ||
| const ComponentContent = forwardRef< | ||
| ElementRef<typeof ComponentPrimitive.Popup>, | ||
| ComponentContentProps | ||
| >( | ||
| ( | ||
| { | ||
| className, | ||
| children, | ||
| showArrow = false, | ||
| style, | ||
| render, | ||
| ...positionerProps | ||
| }, | ||
| ref | ||
| ) => { | ||
| return ( | ||
| <ComponentPrimitive.Portal> | ||
| <ComponentPrimitive.Positioner | ||
| sideOffset={showArrow ? 10 : 4} | ||
| collisionPadding={3} | ||
| className={styles.positioner} | ||
| {...positionerProps} | ||
| > | ||
| <ComponentPrimitive.Popup | ||
| ref={ref} | ||
| className={cx(styles.popup, className)} | ||
| style={style} | ||
| render={render} | ||
| > | ||
| {children} | ||
| {showArrow && ( | ||
| <ComponentPrimitive.Arrow className={styles.arrow}> | ||
| {/* arrow SVG */} | ||
| </ComponentPrimitive.Arrow> | ||
| )} | ||
| </ComponentPrimitive.Popup> | ||
| </ComponentPrimitive.Positioner> | ||
| </ComponentPrimitive.Portal> | ||
| ); | ||
| } | ||
| ); | ||
| ComponentContent.displayName = 'Component.Content'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locating referenced example components ==="
fd 'popover\.tsx|tooltip-content\.tsx|preview-card\.tsx' || echo "No exact matches found"
echo
echo "=== Broader search for popover/tooltip/preview patterns ==="
fd -e tsx | grep -i -E '(popover|tooltip|preview)' | head -20
echo
echo "=== Check if .claude/skills directory exists ==="
ls -la .claude/skills/add-new-component/ 2>/dev/null | head -20 || echo "Directory not found"Repository: raystack/apsara
Length of output: 1524
🏁 Script executed:
#!/bin/bash
echo "=== popover.tsx ==="
cat -n packages/raystack/components/popover/popover.tsx | head -100
echo
echo "=== tooltip-content.tsx ==="
cat -n packages/raystack/components/tooltip/tooltip-content.tsx | head -100
echo
echo "=== preview-card.tsx ==="
cat -n packages/raystack/components/preview-card/preview-card.tsx | head -100Repository: raystack/apsara
Length of output: 7843
🏁 Script executed:
#!/bin/bash
echo "=== Check test files for usage patterns ==="
grep -n "Popup\|Popup.Props" packages/raystack/components/popover/__tests__/popover.test.tsx | head -30
echo
echo "=== Check tooltip tests ==="
grep -n "Popup\|TooltipContent" packages/raystack/components/tooltip/__tests__/tooltip.test.tsx | head -30
echo
echo "=== Search for any additional Popup-specific props being used ==="
rg "TooltipContent|PopoverContent|PreviewCardContent" -A 8 packages/ | grep -E "(initialFocus|finalFocus|onOpenChange|onEscapeKeyDown|aria-|role=)" | head -20Repository: raystack/apsara
Length of output: 334
🏁 Script executed:
#!/bin/bash
echo "=== SKILL.md lines 118-176 ==="
sed -n '118,176p' .claude/skills/add-new-component/SKILL.md | cat -n
echo
echo "=== Compare with actual popover.tsx destructuring ==="
echo "popover.tsx destructures:"
sed -n '19,28p' packages/raystack/components/popover/popover.tsx
echo
echo "=== Compare with actual tooltip-content.tsx destructuring ==="
echo "tooltip-content.tsx destructures:"
sed -n '26,34p' packages/raystack/components/tooltip/tooltip-content.tsxRepository: raystack/apsara
Length of output: 2513
Routing bug in Content component template: merged Popup props are incorrectly spread into Positioner.
Lines 118–176 merge Positioner and Popup props at the interface level but fail to route them correctly. The template destructures only className, style, render explicitly and spreads all remaining props via ...positionerProps into Positioner. This means any additional Popup-specific props (like initialFocus, finalFocus, or others) either get dropped or incorrectly routed to Positioner.
Real implementations (popover.tsx, tooltip-content.tsx, preview-card.tsx) follow this pattern. popover.tsx works around it by explicitly destructuring Popup-specific props, but tooltip-content.tsx and preview-card.tsx silently drop unhandled Popup props. The guidance should separate prop concerns:
-export interface ComponentContentProps
- extends Omit<
- ComponentPrimitive.Positioner.Props,
- 'render' | 'className' | 'style'
- >,
- ComponentPrimitive.Popup.Props {
+type PositionerProps = Omit<
+ ComponentPrimitive.Positioner.Props,
+ 'render' | 'className' | 'style'
+>;
+
+export interface ComponentContentProps extends ComponentPrimitive.Popup.Props {
+ positioner?: PositionerProps;
/** `@default` false */
showArrow?: boolean;
}
@@
- ...positionerProps
+ positioner,
+ ...popupProps
@@
<ComponentPrimitive.Positioner
sideOffset={showArrow ? 10 : 4}
collisionPadding={3}
className={styles.positioner}
- {...positionerProps}
+ {...positioner}
>
<ComponentPrimitive.Popup
ref={ref}
className={cx(styles.popup, className)}
style={style}
render={render}
+ {...popupProps}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/add-new-component/SKILL.md around lines 118 - 176, The
Component.Content component merges Positioner and Popup props via
ComponentContentProps but currently spreads all remaining props into
ComponentPrimitive.Positioner inside ComponentContent, causing Popup-only props
(e.g., initialFocus, finalFocus, onOpenAutoFocus) to be dropped or misrouted;
fix by explicitly destructuring Popup-specific props (at least the known Popup
props like render, initialFocus, finalFocus, onOpenAutoFocus, onCloseAutoFocus,
etc.) from the function args and pass those to ComponentPrimitive.Popup, while
spreading the remaining props into ComponentPrimitive.Positioner so Positioner
receives only its props and Popup receives its own; update ComponentContent
signature and the destructure in ComponentContent to reflect this split.
Comment on lines
+1
to
+33
| export interface PreviewCardRootProps { | ||
| /** Whether the preview card is open (controlled). */ | ||
| open?: boolean; | ||
|
|
||
| /** | ||
| * Whether the preview card is initially open (uncontrolled). | ||
| * @defaultValue false | ||
| */ | ||
| defaultOpen?: boolean; | ||
|
|
||
| /** Callback when the open state changes. */ | ||
| onOpenChange?: (open: boolean) => void; | ||
| } | ||
|
|
||
| export interface PreviewCardTriggerProps { | ||
| /** The URL that the link points to. */ | ||
| href?: string; | ||
|
|
||
| /** | ||
| * Wait time in milliseconds before the preview card opens. | ||
| * @defaultValue 600 | ||
| */ | ||
| delay?: number; | ||
|
|
||
| /** | ||
| * Wait time in milliseconds before the preview card closes. | ||
| * @defaultValue 300 | ||
| */ | ||
| closeDelay?: number; | ||
|
|
||
| /** Additional CSS class name. */ | ||
| className?: string; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether PreviewCard root/trigger runtime props include handle/payload,
# and compare with docs interface definitions and demo usage.
echo "== Runtime/component definitions =="
fd "preview-card.tsx$" | xargs -I{} sh -c '
echo "-- {} --"
rg -n -C3 "interface PreviewCard(Root|Trigger)Props|createHandle|handle|payload" "{}"
'
echo
echo "== Docs interfaces =="
rg -n -C3 "interface PreviewCardRootProps|interface PreviewCardTriggerProps|handle|payload" apps/www/src/content/docs/components/preview-card/props.ts
echo
echo "== Demo usage =="
rg -n -C2 "createHandle|PreviewCard\\.Trigger handle|payload=|<PreviewCard handle" apps/www/src/content/docs/components/preview-card/demo.tsRepository: raystack/apsara
Length of output: 1354
🏁 Script executed:
#!/bin/bash
# Find the actual type definitions for handle and payload
echo "== Check primitive source for handle/payload types =="
fd "preview-card" -t d | head -5 | xargs -I{} find {} -name "*.tsx" -o -name "*.ts" | head -10
echo
echo "== Search for handle type definition =="
rg -n "createHandle|typeof.*createHandle|type.*Handle" packages/raystack/components/preview-card/ -A2
echo
echo "== Check if handle/payload are in trigger/root props in component =="
rg -n "PreviewCardRootProps|PreviewCardTriggerProps" packages/raystack/components/preview-card/ -B2 -A5Repository: raystack/apsara
Length of output: 703
🏁 Script executed:
#!/bin/bash
# Examine the component source to understand the actual API
echo "== Component source structure =="
cat -n packages/raystack/components/preview-card/preview-card.tsx | head -100
echo
echo "== Check index.tsx for exports =="
cat -n packages/raystack/components/preview-card/index.tsx
echo
echo "== Search for createHandle in primitives =="
rg -n "createHandle" packages/raystack/ --type tsx --type tsRepository: raystack/apsara
Length of output: 3494
🏁 Script executed:
#!/bin/bash
# Verify Base UI primitive types to understand what handle/payload are
echo "== Search for `@base-ui` references and types =="
rg "@base-ui/react" packages/raystack/components/preview-card/ -A3
echo
echo "== Check demo more carefully for how handle/payload are used =="
cat -n apps/www/src/content/docs/components/preview-card/demo.ts | sed -n '59,95p'Repository: raystack/apsara
Length of output: 2698
Add missing handle and payload props to documented interfaces.
PreviewCardRootProps (line 1) is missing the handle prop, and PreviewCardTriggerProps (line 15) is missing both handle and payload props. These are required for the documented usage pattern shown in the official demo, where createHandle() returns a handle that must be passed to both the root and trigger components, and trigger components accept a payload prop to associate triggers with content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/preview-card/props.ts` around lines 1 -
33, PreviewCardRootProps is missing the shared handle prop and
PreviewCardTriggerProps is missing both handle and payload; add an optional
handle prop to PreviewCardRootProps and add optional handle and payload props to
PreviewCardTriggerProps so triggers and root can be linked via createHandle();
type the handle using the same Handle/ReturnType used by createHandle() in your
project (or generically/with generics if createHandle is generic) and set
payload to the appropriate payload type used by the demo (or any/unknown/generic
if payload can vary), and ensure prop names exactly match "handle" and "payload"
so the documented usage works with createHandle() and the trigger-root
association.
Comment on lines
+49
to
+55
| it('renders in portal', async () => { | ||
| render(<BasicPreviewCard open={true} />); | ||
|
|
||
| await waitFor(() => { | ||
| const content = screen.getByText(CARD_CONTENT); | ||
| expect(content.closest('body')).toBe(document.body); | ||
| }); |
Contributor
There was a problem hiding this comment.
Portal test assertion is tautological and can’t catch regressions.
Checking closest('body') does not prove portal mounting. Assert that content is outside the render container instead.
Proposed fix
- it('renders in portal', async () => {
- render(<BasicPreviewCard open={true} />);
+ it('renders in portal', async () => {
+ const { container } = render(<BasicPreviewCard open={true} />);
await waitFor(() => {
const content = screen.getByText(CARD_CONTENT);
- expect(content.closest('body')).toBe(document.body);
+ expect(document.body).toContainElement(content);
+ expect(container).not.toContainElement(content);
});
});📝 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.
Suggested change
| it('renders in portal', async () => { | |
| render(<BasicPreviewCard open={true} />); | |
| await waitFor(() => { | |
| const content = screen.getByText(CARD_CONTENT); | |
| expect(content.closest('body')).toBe(document.body); | |
| }); | |
| it('renders in portal', async () => { | |
| const { container } = render(<BasicPreviewCard open={true} />); | |
| await waitFor(() => { | |
| const content = screen.getByText(CARD_CONTENT); | |
| expect(document.body).toContainElement(content); | |
| expect(container).not.toContainElement(content); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/preview-card/__tests__/preview-card.test.tsx`
around lines 49 - 55, The test "renders in portal" in preview-card.test.tsx
currently asserts content.closest('body') which is tautological; change the test
to capture the render container (from render(<BasicPreviewCard open={true} />)
e.g., const { container } = ...) and then assert that the portal content (found
via screen.getByText(CARD_CONTENT)) is not a descendant of that container (e.g.,
expect(container).not.toContainElement(content}) or otherwise assert the
content's parent is document.body but not the render container; update the test
around the BasicPreviewCard render and screen.getByText usage accordingly.
Comment on lines
+28
to
+33
| width: var(--popup-width, auto); | ||
| height: var(--popup-height, auto); | ||
|
|
||
| transition-property: width, height, opacity, transform; | ||
| transition-timing-function: var(--easing); | ||
| transition-duration: var(--animation-duration); |
Contributor
There was a problem hiding this comment.
Stylelint violation: unexpected empty lines before declarations.
This currently fails declaration-empty-line-before; remove the extra blank lines in .popup.
Proposed fix
.popup {
position: relative;
box-sizing: border-box;
padding: var(--rs-space-3);
background-color: var(--rs-color-background-base-primary);
border-radius: var(--rs-radius-2);
border: 0.5px solid var(--rs-color-border-base-primary);
box-shadow:
0 1px 1px 0 rgba(0, 0, 0, 0.07),
0 2px 5px 0 rgba(0, 0, 0, 0.07),
0 3px 8px 0 rgba(0, 0, 0, 0.07);
color: var(--rs-color-foreground-base-primary);
transform-origin: var(--transform-origin);
-
width: var(--popup-width, auto);
height: var(--popup-height, auto);
-
transition-property: width, height, opacity, transform;
transition-timing-function: var(--easing);
transition-duration: var(--animation-duration);
}📝 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.
Suggested change
| width: var(--popup-width, auto); | |
| height: var(--popup-height, auto); | |
| transition-property: width, height, opacity, transform; | |
| transition-timing-function: var(--easing); | |
| transition-duration: var(--animation-duration); | |
| .popup { | |
| position: relative; | |
| box-sizing: border-box; | |
| padding: var(--rs-space-3); | |
| background-color: var(--rs-color-background-base-primary); | |
| border-radius: var(--rs-radius-2); | |
| border: 0.5px solid var(--rs-color-border-base-primary); | |
| box-shadow: | |
| 0 1px 1px 0 rgba(0, 0, 0, 0.07), | |
| 0 2px 5px 0 rgba(0, 0, 0, 0.07), | |
| 0 3px 8px 0 rgba(0, 0, 0, 0.07); | |
| color: var(--rs-color-foreground-base-primary); | |
| transform-origin: var(--transform-origin); | |
| width: var(--popup-width, auto); | |
| height: var(--popup-height, auto); | |
| transition-property: width, height, opacity, transform; | |
| transition-timing-function: var(--easing); | |
| transition-duration: var(--animation-duration); | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 28-28: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 31-31: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/preview-card/preview-card.module.css` around
lines 28 - 33, The .popup CSS block contains unexpected blank lines before
declarations (e.g., the lines before width, height, and transition-* properties)
causing a declaration-empty-line-before Stylelint error; edit the .popup
selector in preview-card.module.css (the block containing width, height,
transition-property, transition-timing-function, transition-duration) and remove
the extra empty lines so declarations appear consecutively with no blank lines
between them.
Comment on lines
+8
to
+49
| export interface PreviewCardContentProps | ||
| extends Omit< | ||
| PreviewCardPrimitive.Positioner.Props, | ||
| 'render' | 'className' | 'style' | ||
| >, | ||
| PreviewCardPrimitive.Popup.Props { | ||
| /** | ||
| * Controls whether to show the arrow. | ||
| * @default false | ||
| */ | ||
| showArrow?: boolean; | ||
| } | ||
|
|
||
| const PreviewCardContent = forwardRef< | ||
| ElementRef<typeof PreviewCardPrimitive.Popup>, | ||
| PreviewCardContentProps | ||
| >( | ||
| ( | ||
| { | ||
| className, | ||
| children, | ||
| showArrow = false, | ||
| style, | ||
| render, | ||
| ...positionerProps | ||
| }, | ||
| ref | ||
| ) => { | ||
| return ( | ||
| <PreviewCardPrimitive.Portal> | ||
| <PreviewCardPrimitive.Positioner | ||
| sideOffset={showArrow ? 10 : 4} | ||
| collisionPadding={3} | ||
| className={styles.positioner} | ||
| {...positionerProps} | ||
| > | ||
| <PreviewCardPrimitive.Popup | ||
| ref={ref} | ||
| className={cx(styles.popup, className)} | ||
| style={style} | ||
| render={render} | ||
| > |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/raystack/components/preview-card/preview-card.tsxRepository: raystack/apsara
Length of output: 3283
🏁 Script executed:
# Find where PreviewCardPrimitive is defined to understand Positioner and Popup interfaces
rg -n 'export.*PreviewCardPrimitive|from.*PreviewCardPrimitive' packages/raystack/components/preview-card/preview-card.tsxRepository: raystack/apsara
Length of output: 131
🏁 Script executed:
# Check if there are any usages of PreviewCard.Content in the codebase
fd --type f -e tsx -e ts . | xargs rg -l '<PreviewCard\.Content|PreviewCard\.Content\s*<' 2>/dev/null | head -20Repository: raystack/apsara
Length of output: 251
🏁 Script executed:
cat packages/raystack/components/preview-card/__tests__/preview-card.test.tsxRepository: raystack/apsara
Length of output: 5855
🏁 Script executed:
cat apps/www/src/content/docs/components/preview-card/demo.tsRepository: raystack/apsara
Length of output: 5180
🏁 Script executed:
cat apps/www/src/components/playground/preview-card-examples.tsxRepository: raystack/apsara
Length of output: 1756
Popup props are misdeclared and misrouted to Positioner.
The type signature accepts Popup.Props, but the implementation forwards all remaining props to Positioner instead. This creates a silent failure where consumers trying to pass aria-label, id, role, event handlers, or other popup-specific props would have them ignored at runtime.
Split prop forwarding: explicitly forward only the positioning props (side, align, sideOffset, alignOffset, collisionPadding) to Positioner, and forward remaining props to Popup.
Proposed fix
export interface PreviewCardContentProps
- extends Omit<
- PreviewCardPrimitive.Positioner.Props,
- 'render' | 'className' | 'style'
- >,
+ extends Pick<
+ PreviewCardPrimitive.Positioner.Props,
+ 'side' | 'align' | 'sideOffset' | 'alignOffset' | 'collisionPadding'
+ >,
PreviewCardPrimitive.Popup.Props {
@@
{
className,
children,
showArrow = false,
style,
render,
- ...positionerProps
+ side,
+ align,
+ sideOffset,
+ alignOffset,
+ collisionPadding,
+ ...popupProps
},
ref
) => {
return (
<PreviewCardPrimitive.Portal>
<PreviewCardPrimitive.Positioner
- sideOffset={showArrow ? 10 : 4}
- collisionPadding={3}
+ side={side}
+ align={align}
+ sideOffset={sideOffset ?? (showArrow ? 10 : 4)}
+ alignOffset={alignOffset}
+ collisionPadding={collisionPadding ?? 3}
className={styles.positioner}
- {...positionerProps}
>
<PreviewCardPrimitive.Popup
ref={ref}
className={cx(styles.popup, className)}
style={style}
render={render}
+ {...popupProps}
>📝 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.
Suggested change
| export interface PreviewCardContentProps | |
| extends Omit< | |
| PreviewCardPrimitive.Positioner.Props, | |
| 'render' | 'className' | 'style' | |
| >, | |
| PreviewCardPrimitive.Popup.Props { | |
| /** | |
| * Controls whether to show the arrow. | |
| * @default false | |
| */ | |
| showArrow?: boolean; | |
| } | |
| const PreviewCardContent = forwardRef< | |
| ElementRef<typeof PreviewCardPrimitive.Popup>, | |
| PreviewCardContentProps | |
| >( | |
| ( | |
| { | |
| className, | |
| children, | |
| showArrow = false, | |
| style, | |
| render, | |
| ...positionerProps | |
| }, | |
| ref | |
| ) => { | |
| return ( | |
| <PreviewCardPrimitive.Portal> | |
| <PreviewCardPrimitive.Positioner | |
| sideOffset={showArrow ? 10 : 4} | |
| collisionPadding={3} | |
| className={styles.positioner} | |
| {...positionerProps} | |
| > | |
| <PreviewCardPrimitive.Popup | |
| ref={ref} | |
| className={cx(styles.popup, className)} | |
| style={style} | |
| render={render} | |
| > | |
| export interface PreviewCardContentProps | |
| extends Pick< | |
| PreviewCardPrimitive.Positioner.Props, | |
| 'side' | 'align' | 'sideOffset' | 'alignOffset' | 'collisionPadding' | |
| >, | |
| PreviewCardPrimitive.Popup.Props { | |
| /** | |
| * Controls whether to show the arrow. | |
| * `@default` false | |
| */ | |
| showArrow?: boolean; | |
| } | |
| const PreviewCardContent = forwardRef< | |
| ElementRef<typeof PreviewCardPrimitive.Popup>, | |
| PreviewCardContentProps | |
| >( | |
| ( | |
| { | |
| className, | |
| children, | |
| showArrow = false, | |
| style, | |
| render, | |
| side, | |
| align, | |
| sideOffset, | |
| alignOffset, | |
| collisionPadding, | |
| ...popupProps | |
| }, | |
| ref | |
| ) => { | |
| return ( | |
| <PreviewCardPrimitive.Portal> | |
| <PreviewCardPrimitive.Positioner | |
| side={side} | |
| align={align} | |
| sideOffset={sideOffset ?? (showArrow ? 10 : 4)} | |
| alignOffset={alignOffset} | |
| collisionPadding={collisionPadding ?? 3} | |
| className={styles.positioner} | |
| > | |
| <PreviewCardPrimitive.Popup | |
| ref={ref} | |
| className={cx(styles.popup, className)} | |
| style={style} | |
| render={render} | |
| {...popupProps} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/preview-card/preview-card.tsx` around lines 8 -
49, The component forwards all remaining props to
PreviewCardPrimitive.Positioner but its type includes Popup props, causing
popup-specific attributes to be dropped; in PreviewCardContent destructure the
positioning props (side, align, sideOffset, alignOffset, collisionPadding) out
of the incoming props and pass only those to PreviewCardPrimitive.Positioner,
while forwarding the rest (including aria-*, id, role, event handlers, etc.) to
PreviewCardPrimitive.Popup (keep render, className, style, children handling
as-is) so Positioner only receives layout props and Popup receives the
accessibility/DOM props.
paanSinghCoder
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds PreviewCard component based on BaseUI primitives
Summary by CodeRabbit
New Features
Documentation
Style
Tests