[WEB-4748] chore: propel utils#7662
Conversation
WalkthroughReplaced imports of the Changes
Sequence Diagram(s)sequenceDiagram
participant Component as UI Component
participant CN as cn (utils/classname)
participant CLSX as clsx
participant TM as tailwind-merge
Component->>CN: cn(...classValues)
Note right of CN #E6F4EA: New implementation uses<br/>twMerge(clsx(...))
CN->>CLSX: clsx(...classValues)
CLSX-->>CN: "combined class string"
CN->>TM: twMerge("combined class string")
TM-->>CN: "merged class string"
CN-->>Component: final className
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (5)
packages/propel/src/charts/tree-map/map-content.tsx (1)
80-82: Bug: falsy value/label handling hides zero and mis-sizes text; font-size mismatch.
0 should render; and sizing should match the rendered “text-sm”.- const valueWidth = value ? calculateContentWidth(value, LAYOUT.TEXT.FONT_SIZES.XS) : 0; - const labelWidth = label ? calculateContentWidth(label, LAYOUT.TEXT.FONT_SIZES.XS) + 4 : 0; // 4px for spacing + const valueWidth = + value !== undefined ? calculateContentWidth(value, LAYOUT.TEXT.FONT_SIZES.SM) : 0; + const labelWidth = + label !== undefined ? calculateContentWidth(label, LAYOUT.TEXT.FONT_SIZES.SM) + 4 : 0; // 4px for spacingpackages/propel/src/dialog/root.tsx (1)
44-51: Invalid hook usage: useCallback at module scope.
Hooks must not be called outside React components or custom hooks.-const getPositionClassNames = React.useCallback( - (position: DialogPosition) => - cn("isolate fixed z-modal", { - "top-1/2 left-1/2 -translate-x-1/2 -translate-y-1/2": position === "center", - "top-8 left-1/2 -translate-x-1/2": position === "top", - }), - [] -); +const getPositionClassNames = (position: DialogPosition) => + cn("isolate fixed z-modal", { + "top-1/2 left-1/2 -translate-x-1/2 -translate-y-1/2": position === "center", + "top-8 left-1/2 -translate-x-1/2": position === "top", + });packages/propel/src/command/command.tsx (1)
22-32: className props are ignored on List, Empty, and Item.
Consumers can’t style these slots; this is a regression in ergonomics.-function CommandList({ className, ...props }: React.ComponentProps<typeof CommandPrimitive.List>) { - return <CommandPrimitive.List data-slot="command-list" {...props} />; -} +function CommandList({ className, ...props }: React.ComponentProps<typeof CommandPrimitive.List>) { + return <CommandPrimitive.List data-slot="command-list" className={cn(className)} {...props} />; +} -function CommandEmpty({ ...props }: React.ComponentProps<typeof CommandPrimitive.Empty>) { - return <CommandPrimitive.Empty data-slot="command-empty" {...props} />; -} +function CommandEmpty({ className, ...props }: React.ComponentProps<typeof CommandPrimitive.Empty>) { + return <CommandPrimitive.Empty data-slot="command-empty" className={cn(className)} {...props} />; +} -function CommandItem({ className, ...props }: React.ComponentProps<typeof CommandPrimitive.Item>) { - return <CommandPrimitive.Item data-slot="command-item" {...props} />; -} +function CommandItem({ className, ...props }: React.ComponentProps<typeof CommandPrimitive.Item>) { + return <CommandPrimitive.Item data-slot="command-item" className={cn(className)} {...props} />; +}packages/propel/src/menu/menu.tsx (2)
52-56: Bug: calling global window.close().
close() here refers to the global window API, not the menu. This may attempt to close the tab and will fail silently or prompt the user. Remove it and rely on the menu’s default close-on-select behavior or wire the library’s close helper.- onClick={(e) => { - close(); - onClick?.(e); - submenuContext?.closeSubmenu(); - }} + onClick={(e) => { + onClick?.(e); + submenuContext?.closeSubmenu(); + }}
125-126: Keep local isOpen in sync with BaseMenu’s open state.
Otherwise the trigger’s visual “open” state can desync (e.g., click outside or hover-open).- return ( - <BaseMenu.Root openOnHover={openOnHover} onOpenChange={handleOpenChange}> + return ( + <BaseMenu.Root + openOnHover={openOnHover} + onOpenChange={(open) => { + setIsOpen(open); + if (!open) { + closeAllSubmenus(); + onMenuClose?.(); + } + handleOpenChange(open as any); + }} + >
🧹 Nitpick comments (10)
packages/propel/src/utils/classname.tsx (2)
4-4: Minor: spread inputs for clsx and annotate return typeSlightly cleaner and avoids wrapping the rest args in an array.
-export const cn = (...inputs: ClassValue[]) => twMerge(clsx(inputs)); +export const cn = (...inputs: ClassValue[]): string => twMerge(clsx(...inputs));
1-4: Optional: rename file to .tsNo JSX here; consider classname.ts for consistency with TS-only utils.
packages/propel/src/combobox/combobox.tsx (1)
39-41: Rename “rg” to a conventional size token (“xs”) and adjust mapping.
Improves clarity and consistency with Tailwind sizing.- maxHeight?: "lg" | "md" | "rg" | "sm"; + maxHeight?: "xs" | "sm" | "md" | "lg";- "max-h-60": maxHeight === "lg", - "max-h-48": maxHeight === "md", - "max-h-36": maxHeight === "rg", - "max-h-28": maxHeight === "sm", + "max-h-60": maxHeight === "lg", + "max-h-48": maxHeight === "md", + "max-h-36": maxHeight === "sm", + "max-h-28": maxHeight === "xs",Also applies to: 248-253
packages/propel/src/charts/tree-map/map-content.tsx (1)
170-173: Reduce unnecessary recalcs from icon identity.
Only depend on its truthiness to avoid memo churn when icon element identity changes.- const visibility = useMemo( - () => calculateVisibility(width, height, !!icon, name, value, label), - [width, height, icon, name, value, label] - ); + const visibility = useMemo( + () => calculateVisibility(width, height, !!icon, name, value, label), + [width, height, !!icon, name, value, label] + );packages/propel/src/tooltip/root.tsx (1)
22-37: Confirm intent: position default shadows side/align props.
With position defaulting to "top", side/align are never used unless position is unset. If that’s unintentional, drop the default or prefer side/align when provided.packages/propel/src/charts/components/legend.tsx (1)
51-55: Stabilize keys to avoid duplicates.
item.value may repeat; include index to ensure uniqueness.- {payload.map((item, index) => ( + {payload.map((item, index) => ( <div - key={item.value} + key={`${String(item.value)}-${index}`}packages/propel/src/command/command.tsx (2)
6-8: Drop the redundant empty string in cn().
No functional impact, but it’s noise.- return <CommandPrimitive data-slot="command" className={cn("", className)} {...props} />; + return <CommandPrimitive data-slot="command" className={cn(className)} {...props} />;
10-19: Consider a wrapperClassName for the input container.
Right now className only styles the <CommandPrimitive.Input/>, not the wrapper. If consumers need to style the wrapper, provide a dedicated prop.-function CommandInput({ className, ...props }: React.ComponentProps<typeof CommandPrimitive.Input>) { +function CommandInput( + { className, wrapperClassName, ...props }: React.ComponentProps<typeof CommandPrimitive.Input> & { wrapperClassName?: string } +) { return ( <div data-slot="command-input-wrapper" - className="flex items-center gap-1.5 rounded border border-custom-border-100 bg-custom-background-90 px-2" + className={cn( + "flex items-center gap-1.5 rounded border border-custom-border-100 bg-custom-background-90 px-2", + wrapperClassName + )} > <SearchIcon className="size-3.5 flex-shrink-0 text-custom-text-400" strokeWidth={1.5} /> <CommandPrimitive.Input data-slot="command-input" className={cn(className)} {...props} /> </div> ); }packages/propel/src/menu/menu.tsx (1)
24-33: Expose styling hooks for submenu trigger/positioner.
className="" on SubmenuTrigger/Positioner discards customization. Either remove the empty className props or add dedicated triggerClassName/positionerClassName to SubMenu.- <BaseMenu.SubmenuTrigger className={""}> + <BaseMenu.SubmenuTrigger> <span className="flex-1">{trigger}</span> <ChevronRight /> </BaseMenu.SubmenuTrigger> <BaseMenu.Portal> - <BaseMenu.Positioner className={""} alignOffset={-4} sideOffset={-4}> + <BaseMenu.Positioner alignOffset={-4} sideOffset={-4}> <BaseMenu.Popup className={className}>{children} </BaseMenu.Popup> </BaseMenu.Positioner> </BaseMenu.Portal>Optionally introduce:
-const SubMenu: React.FC<TSubMenuProps> = (props) => { - const { children, trigger, disabled = false, className = "" } = props; +const SubMenu: React.FC<TSubMenuProps & { triggerClassName?: string; positionerClassName?: string }> = (props) => { + const { children, trigger, disabled = false, className = "", triggerClassName, positionerClassName } = props; ... - <BaseMenu.SubmenuTrigger> + <BaseMenu.SubmenuTrigger className={cn(triggerClassName)}> ... - <BaseMenu.Positioner alignOffset={-4} sideOffset={-4}> + <BaseMenu.Positioner className={cn(positionerClassName)} alignOffset={-4} sideOffset={-4}>packages/propel/src/tabs/tabs.tsx (1)
4-4: Consider a local utils barrel to reduce import churnRe-export cn from packages/propel/src/utils/index.ts and import from "../utils" to simplify paths across files and future moves.
Example:
+// packages/propel/src/utils/index.ts +export { cn } from "./classname"; - import { cn } from "../utils/classname"; + import { cn } from "../utils";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
packages/propel/package.json(1 hunks)packages/propel/src/avatar/avatar.tsx(1 hunks)packages/propel/src/card/card.tsx(1 hunks)packages/propel/src/charts/bar-chart/bar.tsx(1 hunks)packages/propel/src/charts/components/legend.tsx(1 hunks)packages/propel/src/charts/components/tooltip.tsx(1 hunks)packages/propel/src/charts/tree-map/map-content.tsx(1 hunks)packages/propel/src/charts/tree-map/root.tsx(1 hunks)packages/propel/src/combobox/combobox.tsx(1 hunks)packages/propel/src/command/command.tsx(1 hunks)packages/propel/src/dialog/root.tsx(1 hunks)packages/propel/src/menu/menu.tsx(1 hunks)packages/propel/src/switch/root.tsx(1 hunks)packages/propel/src/table/core.tsx(1 hunks)packages/propel/src/tabs/list.tsx(1 hunks)packages/propel/src/tabs/tabs.tsx(1 hunks)packages/propel/src/tooltip/root.tsx(1 hunks)packages/propel/src/utils/classname.tsx(1 hunks)
⏰ 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). (1)
- GitHub Check: Build and lint web apps
🔇 Additional comments (20)
packages/propel/package.json (2)
42-43: LGTM: add tailwind-merge as runtime depDependency placement and version look fine.
42-43: Refactor completeness verified: no @plane/utils imports remain
Ran the provided ripgrep command against packages/propel/src and confirmed there are no imports or requires of@plane/utils.packages/propel/src/table/core.tsx (1)
3-3: LGTM: switch to local cn utilityImport path change aligns with the new utils/classname.
packages/propel/src/charts/bar-chart/bar.tsx (1)
5-5: LGTM: cn import path updatedMatches the shared refactor; no behavioral changes in this file.
packages/propel/src/charts/tree-map/root.tsx (1)
6-6: LGTM: cn import path updatedConsistent with the new local classname utility.
packages/propel/src/combobox/combobox.tsx (1)
4-4: LGTM: import path refactor is correct.packages/propel/src/charts/tree-map/map-content.tsx (1)
4-4: LGTM: import path refactor is correct.packages/propel/src/tooltip/root.tsx (1)
3-3: LGTM: import path refactor is correct.packages/propel/src/charts/components/legend.tsx (1)
5-5: LGTM: import path refactor is correct.packages/propel/src/dialog/root.tsx (1)
5-5: LGTM: import path refactor is correct.packages/propel/src/command/command.tsx (1)
4-4: Import path change looks good.
Localizing cn improves package boundaries and reduces cross-workspace coupling.packages/propel/src/switch/root.tsx (1)
3-3: Import path change is correct.
No API/behavior change; cn remains compatible with tailwind-merge.packages/propel/src/charts/components/tooltip.tsx (1)
5-5: Import path update LGTM.
Component behavior unchanged; cn with tailwind-merge is a safe improvement for class conflict resolution.packages/propel/src/menu/menu.tsx (1)
4-4: Import path change is fine.
Local cn usage aligns with the refactor.packages/propel/src/card/card.tsx (1)
2-2: Import path update acknowledged.
Card continues to merge base + external classes correctly via cn(twMerge+clsx).packages/propel/src/avatar/avatar.tsx (2)
3-3: Import switch to local cn looks correctThe relative path is accurate for this location and the named import matches expectations.
3-3: Verified: cn export and types –cnis a named export with signature(...inputs: ClassValue[]), no lingering @plane/utils imports, and all imports are consistent.packages/propel/src/tabs/tabs.tsx (1)
4-4: Local cn import aligns with refactorConsistent with moving off @plane/utils; no API changes visible here.
packages/propel/src/tabs/list.tsx (2)
5-5: cn path update LGTMMatches the new local utility; usage with conditional class objects remains compatible with clsx + tailwind-merge.
5-5: Double-check tailwind-merge effects on precedenceWith twMerge, later classes win. Since tabClassName is last in cn(), consumer overrides will intentionally take precedence—good. Verify no unintended overrides in size-related classes during QA.
3 participants
Description
This PR refactors the Propel utility functions
Type of Change
Summary by CodeRabbit