- Notifications
You must be signed in to change notification settings - Fork3k
[WEB-5482] fix: propel package types#8158
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:preview
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Linked to Plane Work Item(s) This comment was auto-generated byPlane |
coderabbitaibot commentedNov 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces runtime Object.assign-based compound-component exports with explicit root aliases and separate named subcomponent exports across multiple Propel components; adds displayName assignments and tighter React.ComponentProps/memo typings; updates stories and consumers to import and use the new named subcomponents. Changes
Sequence Diagram(s)(omitted — changes are export/typing refactors and story updates; no new control-flow to visualize) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for usingCodeRabbit! 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.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit theCursor dashboard to activate Pro and start your 14-day free trial.
Uh oh!
There was an error while loading.Please reload this page.
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.
Pull Request Overview
This PR refactors the compound component creation pattern across the propel package to fix TypeScript type inference issues. The changes replaceObject.assign() with explicit type casting and manual property assignment, which provides better type safety and IDE support for compound components.
Key changes:
- Replaced
Object.assign()pattern with type casting (as typeof Component & {...}) followed by manual property assignments - Added explicit type parameters to
React.memocalls for better type inference (in popover and dialog) - Exported previously internal type definitions (SkeletonProps, SkeletonItemProps in skeleton)
- Added displayName properties to component subparts where they were missing (tabs)
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/propel/src/toolbar/toolbar.tsx | Refactored Toolbar compound component from Object.assign to type casting pattern |
| packages/propel/src/tabs/tabs.tsx | Removed complex TabsCompound type definition, added displayNames, and applied new compound component pattern |
| packages/propel/src/skeleton/root.tsx | Exported type definitions, applied new compound component pattern, and exported individual components |
| packages/propel/src/popover/root.tsx | Added explicit type parameters to React.memo calls and applied new compound component pattern |
| packages/propel/src/dialog/root.tsx | Added explicit type parameters to React.memo calls and applied new compound component pattern |
| packages/propel/src/context-menu/context-menu.tsx | Refactored ContextMenu compound component to use type casting pattern |
| packages/propel/src/command/command.tsx | Applied new compound component pattern to Command component |
| packages/propel/src/combobox/combobox.tsx | Applied new compound component pattern to Combobox component |
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/propel/src/combobox/combobox.tsx(1 hunks)packages/propel/src/command/command.tsx(1 hunks)packages/propel/src/context-menu/context-menu.tsx(1 hunks)packages/propel/src/dialog/root.tsx(5 hunks)packages/propel/src/popover/root.tsx(3 hunks)packages/propel/src/skeleton/root.tsx(3 hunks)packages/propel/src/tabs/tabs.tsx(1 hunks)packages/propel/src/toolbar/toolbar.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7888File: packages/propel/src/avatar/avatar.stories.tsx:2-3Timestamp: 2025-10-01T15:30:17.605ZLearning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.Learnt from: lifeiscontentRepo: makeplane/plane PR: 7989File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46Timestamp: 2025-10-21T17:22:05.204ZLearning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T20:42:31.843ZLearning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.Applied to files:
packages/propel/src/tabs/tabs.tsx
🧬 Code graph analysis (1)
packages/propel/src/dialog/root.tsx (1)
packages/propel/src/utils/classname.tsx (1)
cn(5-5)
⏰ 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: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (11)
packages/propel/src/combobox/combobox.tsx (1)
223-231:Typed compound Combobox wiring looks correctThe
Comboboxintersection type and subsequent static assignments (Button,Options,Option) are consistent and preserve the previous runtime behavior while improving typings.packages/propel/src/toolbar/toolbar.tsx (1)
168-177:Toolbar compound-component typing is soundCasting
ToolbarRootto an intersection with its subcomponents and then assigningGroup,Item,Separator, andSubmitButtonyields a well-typed compoundToolbarwithout changing runtime behavior.packages/propel/src/skeleton/root.tsx (3)
5-15:Public Skeleton prop types are well-shapedExporting
SkeletonPropsandSkeletonItemPropsmatches the existing usage and makes the component API easier to consume in TS without altering behavior.
25-33:SkeletonItem props refactor is consistentSwitching
SkeletonItemto acceptSkeletonItemPropswith the same defaults (height,width,className) preserves behavior while exposing a clearer public type.
38-43:Skeleton compound export and additional named exports look goodThe
Skeletonintersection type plusSkeleton.Itemassignment is correct, and exportingSkeletonRootandSkeletonItemalongsideSkeletonis a safe, additive API improvement.packages/propel/src/command/command.tsx (1)
34-43:Command compound-component typing is correct
Commandis now a properly typed compound component (CommandComponentplusInput,List,Empty,Itemstatics), eliminating the looseObject.assigntyping while keeping the same runtime structure.packages/propel/src/tabs/tabs.tsx (3)
90-94:Display names improve debuggabilityAssigning
displayNameon all Tabs primitives (TabsRoot,TabsList,TabsTrigger,TabsContent,TabsIndicator) is harmless and helps with React DevTools/debugging.
96-105:Tabs compound-component typing matches the established patternDefining
TabsasTabsRootintersected with its subcomponents and then settingTabs.List,Tabs.Trigger,Tabs.Content, andTabs.Indicatorpreserves the compound API with accurate static member types.
107-107:No breaking change identified—subcomponents were never exported as named exportsThe subcomponents (
TabsList,TabsTrigger,TabsContent,TabsIndicator) are only available through theTabscompound component pattern (accessed asTabs.List,Tabs.Trigger, etc.), not as separate named exports. The codebase never exported these as individual named imports, so exporting onlyTabsis not a breaking change.Likely an incorrect or invalid review comment.
packages/propel/src/context-menu/context-menu.tsx (1)
132-148:ContextMenu compound-component refactor is consistent and type-safeThe new
ContextMenuintersection type plus explicit assignments forTrigger,Portal,Content,Item,Separator,Submenu, andSubmenuTriggeraligns with the pattern used elsewhere and keeps the runtime API unchanged while tightening typings.packages/propel/src/popover/root.tsx (1)
65-70:Typed compound wiring looks solid.The explicit static assignments preserve the compound API while improving inference. Nice cleanup.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/propel/src/popover/root.tsx (2)
48-67:Critical: Incomplete definition and duplicate components block compilation.This section has multiple blocking syntax errors:
- Line 56 is incomplete — the
PopoverPositionerdefinition starts but has no closing brace or return statement, causing the parse error reported by static analysis.- Duplicate component definitions —
PopoverTrigger,PopoverPortal, andPopoverPositionerare each defined twice (lines 48-50/57-59, 52-54/61-63, 56/65-67).React.memoused without importingReact— lines 48, 52, 56, 70 referenceReact.memobut onlymemois imported at line 1.Apply this diff to remove duplicates, fix the incomplete definition, and use the correct import:
// wrapper components-const PopoverTrigger = React.memo(function PopoverTrigger(props: React.ComponentProps<typeof BasePopover.Trigger>) {- return <BasePopover.Trigger data-slot="popover-trigger" {...props} />;-});--const PopoverPortal = React.memo(function PopoverPortal(props: React.ComponentProps<typeof BasePopover.Portal>) {- return <BasePopover.Portal data-slot="popover-portal" {...props} />;-});--const PopoverPositioner = React.memo(function PopoverPositioner(props: React.ComponentProps<typeof BasePopover.Positioner>) { const PopoverTrigger = memo(function PopoverTrigger(props: React.ComponentProps<typeof BasePopover.Trigger>) { return <BasePopover.Trigger data-slot="popover-trigger" {...props} />; }); const PopoverPortal = memo(function PopoverPortal(props: React.ComponentProps<typeof BasePopover.Portal>) { return <BasePopover.Portal data-slot="popover-portal" {...props} />; }); const PopoverPositioner = memo(function PopoverPositioner(props: React.ComponentProps<typeof BasePopover.Positioner>) { return <BasePopover.Positioner data-slot="popover-positioner" {...props} />; });
70-81:Critical: Remove old Object.assign pattern and fix React.memo usage.Two issues here:
- Old Object.assign pattern (lines 73-81) not removed — per the PR objectives, this refactor replaces the compound
Object.assignpattern with separate named exports, but the old code is still present alongside the newPopoverRoot.React.memoused without importingReact(line 70) — onlymemois imported.Apply this diff to remove the old pattern and fix the import:
// compound components-const PopoverRoot = React.memo<React.ComponentProps<typeof BasePopover.Root>>(function Popover(props) {+const PopoverRoot = memo<React.ComponentProps<typeof BasePopover.Root>>(function Popover(props) { return <BasePopover.Root data-slot="popover" {...props} />; });-const Popover = Object.assign(- memo(function Popover(props: React.ComponentProps<typeof BasePopover.Root>) {- return <BasePopover.Root data-slot="popover" {...props} />;- }),- {- Button: PopoverTrigger,- Panel: PopoverContent,- }-);packages/propel/src/skeleton/root.tsx (1)
35-38:UndefinedSkeletonexport causes the reported build/lint failure
Skeletonis exported but never defined in this module, which matches the pipeline error ('Skeleton' is not defined). Given the PR’s goal to keepSkeletonas the main export while addingSkeletonRootandSkeletonItem, you can aliasSkeletonRoot:SkeletonRoot.displayName = "plane-ui-skeleton"; SkeletonItem.displayName = "plane-ui-skeleton-item";-export { Skeleton, SkeletonRoot, SkeletonItem };+const Skeleton = SkeletonRoot;++export { Skeleton, SkeletonRoot, SkeletonItem };This preserves the existing
{ Skeleton }API and fixes the build.
🧹 Nitpick comments (1)
packages/propel/src/skeleton/root.tsx (1)
5-33:Public props typing is solid; consider aligningheight/widthwith CSSPropertiesExporting
SkeletonPropsandSkeletonItemPropsis a nice improvement for the public API. To better match how these are used instyle={{ height, width }}, you might want to allow both strings and numbers (and stay in sync with React’s CSS typing) by keying offReact.CSSProperties:-export type SkeletonItemProps = {- height?: string;- width?: string;- className?: string;-};+export type SkeletonItemProps = {+ height?: React.CSSProperties["height"];+ width?: React.CSSProperties["width"];+ className?: string;+};This keeps the runtime behavior the same while making the TS surface a bit more flexible and accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/propel/src/dialog/root.tsx(2 hunks)packages/propel/src/popover/root.tsx(3 hunks)packages/propel/src/skeleton/root.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/propel/src/dialog/root.tsx
🧰 Additional context used
🪛 Biome (2.1.2)
packages/propel/src/popover/root.tsx
[error] 89-90: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 90-90: expected} but instead the file ends
the file ends here
(parse)
🪛 GitHub Actions: Build and lint web apps
packages/propel/src/popover/root.tsx
[error] 91-91: ESLint: Parsing error: '}' expected
packages/propel/src/skeleton/root.tsx
[error] 38-38: 'Skeleton' is not defined
⏰ 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: Analyze (javascript)
- GitHub Check: Cursor Bugbot
| // display names | ||
| PopoverContent.displayName="PopoverContent"; | ||
| Popover.displayName="Popover"; | ||
| PopoverRoot.displayName="Popover"; | ||
| PopoverPortal.displayName="PopoverPortal"; | ||
| PopoverTrigger.displayName="PopoverTrigger"; | ||
| PopoverPositioner.displayName="PopoverPositioner"; | ||
| export{Popover}; | ||
| export{PopoverRootasPopover,PopoverTrigger,PopoverContent}; |
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.
Parse errors cascade from earlier syntax issues.
The static analysis and pipeline errors reporting missing closing braces and unexpected end-of-file at lines 89-91 are consequences of the incompletePopoverPositioner definition at line 56 and the duplicate component definitions above. Once those critical syntax errors are resolved, these parse errors will be fixed automatically.
🧰 Tools
🪛 Biome (2.1.2)
[error] 89-90: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 90-90: expected} but instead the file ends
the file ends here
(parse)
🤖 Prompt for AI Agents
In packages/propel/src/popover/root.tsx around lines 83 to 90, the file hascascading parse errors caused by an incomplete PopoverPositioner definition(around line 56) and duplicate component definitions earlier; finish thePopoverPositioner implementation (close any open braces, parentheses and JSXtags) and remove or consolidate the duplicate component definitions so eachcomponent is defined once, then verify the displayName assignments and theexport line match the actual defined identifiers (ensure semicolons/commas arecorrect) to eliminate the syntax errors.| ComboboxRoot.displayName="Combobox"; | ||
| ComboboxButton.displayName="ComboboxButton"; | ||
| ComboboxOptions.displayName="ComboboxOptions"; | ||
| ComboboxOption.displayName="ComboboxOption"; |
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.
if they're function declarations you do not need to define a displayName it will be inferred by the functions.name
| CommandInput.displayName="CommandInput"; | ||
| CommandList.displayName="CommandList"; | ||
| CommandEmpty.displayName="CommandEmpty"; | ||
| CommandItem.displayName="CommandItem"; |
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.
same as above
| export{ContextMenu}; | ||
| export{ | ||
| ContextMenuRootasContextMenu, |
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.
just rename the component, no need for indirection
| children:React.ReactNode; | ||
| } | ||
| exportinterfaceDialogPortalPropsextendsReact.ComponentProps<typeofBaseDialog.Portal>{ |
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.
prefer types over interfaces for prop types, you don't need declaration merging for props
| }; | ||
| export{Dialog,DialogTitle,DialogPanel}; | ||
| export{DialogComponentasDialog,DialogTitle,DialogPanel,DialogTrigger}; |
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.
rename the component, no need for indirection
| }); | ||
| DialogTitle.displayName="DialogTitle"; | ||
| DialogComponent.displayName="Dialog"; |
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.
same as above
| ); | ||
| // display names | ||
| PopoverContent.displayName="PopoverContent"; |
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.
same as above
Uh oh!
There was an error while loading.Please reload this page.
Description
This PR includes fixs for propel package types error
Type of Change
Note
Shifts multiple UI components from compound-object subcomponents to named exports, adds display names/types, and updates stories/usages accordingly.
Object.assigncompound subcomponents with named exports forCombobox,Command,ContextMenu,Dialog,Popover,Tabs,Toolbar, andSkeleton....Button/Trigger/Content/List/Item/Panel/Title/...named subcomponents.displayNames across components; broaden re-exports inindex.ts(e.g.,context-menu,toolbar).EmojiPickerandEmojiReactionPickertoPopoverTrigger/PopoverContent.ContextMenuandDialoginternals/exports (portal/overlay/trigger) and re-export granular parts.Written byCursor Bugbot for commit85ca86b. This will update automatically on new commits. Configurehere.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.