Conversation
WalkthroughThis pull request renames KPI-related UI components and variant names to more general Metric equivalents across multiple applications. Specifically, component names are updated (e.g., Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/shade/src/components/ui/card.stories.tsx (1)
120-126: Use the built-incolorprop in these stories.Both stories re-create the color indicator with inline markup even though
MetricCardHeaderLabelalready exposescolor. That makes Storybook document the implementation detail instead of the public API.As per coding guidelines "In story content, prefer CVA variants/props over ad-hoc class overrides; avoid obvious technical implementation details and prefer one-line guidance per story explaining when to use that variant/size/state".
Also applies to: 170-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/ui/card.stories.tsx` around lines 120 - 126, The story recreates the color indicator markup instead of using the public API: replace the manual inline span and style with the MetricCardHeaderLabel's color prop (e.g., pass color="purple" or the appropriate variant name) in the MetricCardHeaderLabel component used at lines ~120 and ~170; remove the extra <span className='inline-block ...' style={{backgroundColor: ...}}/> markup and update the story text to a one-line note about when to use that color variant.apps/shade/src/components/ui/tabs.tsx (1)
11-25: ExporttabsVariantswith deprecated variant support for API consistency.While the
Tabscomponent correctly resolves deprecated'navbar'→'navigation'and'kpis'→'metrics'mappings, the exportedtabsVariantshelper only recognizes the new variant names. This creates an inconsistency in the public API surface: direct callers of the helper will fail if they pass the deprecated names, even though the component itself accepts them. SincetabsVariantsis exported as part of the package, maintaining parity between the component and helper is a best practice.Consider wrapping
tabsVariantsto apply the same deprecation mapping:Suggested approach
-const tabsVariants = cva( +const tabsVariantsBase = cva( '', { variants: { variant: { segmented: '', 'segmented-sm': '', button: '', 'button-sm': '', underline: '', navigation: '', pill: '', metrics: '' } }, defaultVariants: { variant: 'segmented' } } ); + +const tabsVariants = (props?: {variant?: TabsVariant; [key: string]: any}) => { + if (!props) return tabsVariantsBase(); + const {variant, ...rest} = props; + return tabsVariantsBase({ + ...rest, + variant: variant ? resolveTabsVariant(variant) : undefined + }); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/ui/tabs.tsx` around lines 11 - 25, Exported helper tabsVariants currently only accepts PreferredTabsVariant names, causing callers using deprecated names ('navbar'/'kpis') to fail; wrap or adapt tabsVariants to normalize its input using the existing resolveTabsVariant(variant: TabsVariant) mapping so it accepts TabsVariant (including DeprecatedTabsVariant) and internally converts deprecated values to PreferredTabsVariant before performing its logic; update the tabsVariants signature/type to accept TabsVariant and call resolveTabsVariant at the start of the function so behavior matches the Tabs component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/shade/src/components/ui/tabs.tsx`:
- Around line 188-199: MetricTabTrigger currently allows incoming
props.className to overwrite the base 'h-auto' class because props are spread
after the literal className; fix by destructuring className from
MetricTabTriggerProps (e.g., const MetricTabTrigger = ({ children, className,
...rest }) => ...) and merge it with the base class using the project's utility
(cn('h-auto', className)), then pass the merged className to TabsTrigger along
with ...rest; keep MetricTabTrigger.displayName unchanged.
---
Nitpick comments:
In `@apps/shade/src/components/ui/card.stories.tsx`:
- Around line 120-126: The story recreates the color indicator markup instead of
using the public API: replace the manual inline span and style with the
MetricCardHeaderLabel's color prop (e.g., pass color="purple" or the appropriate
variant name) in the MetricCardHeaderLabel component used at lines ~120 and
~170; remove the extra <span className='inline-block ...'
style={{backgroundColor: ...}}/> markup and update the story text to a one-line
note about when to use that color variant.
In `@apps/shade/src/components/ui/tabs.tsx`:
- Around line 11-25: Exported helper tabsVariants currently only accepts
PreferredTabsVariant names, causing callers using deprecated names
('navbar'/'kpis') to fail; wrap or adapt tabsVariants to normalize its input
using the existing resolveTabsVariant(variant: TabsVariant) mapping so it
accepts TabsVariant (including DeprecatedTabsVariant) and internally converts
deprecated values to PreferredTabsVariant before performing its logic; update
the tabsVariants signature/type to accept TabsVariant and call
resolveTabsVariant at the start of the function so behavior matches the Tabs
component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae643128-73da-47e3-8ae6-02a146862cdf
📒 Files selected for processing (14)
apps/posts/src/views/PostAnalytics/Overview/components/newsletter-overview.tsxapps/posts/src/views/PostAnalytics/Overview/components/web-overview.tsxapps/posts/src/views/PostAnalytics/Web/components/kpis.tsxapps/shade/src/components/ui/card.stories.tsxapps/shade/src/components/ui/card.tsxapps/shade/src/components/ui/table.stories.tsxapps/shade/src/components/ui/table.tsxapps/shade/src/components/ui/tabs.stories.tsxapps/shade/src/components/ui/tabs.tsxapps/stats/src/views/Stats/Growth/components/growth-kpis.tsxapps/stats/src/views/Stats/Newsletters/components/newsletters-kpis.tsxapps/stats/src/views/Stats/Newsletters/newsletters.tsxapps/stats/src/views/Stats/Overview/components/overview-kpis.tsxapps/stats/src/views/Stats/Web/components/web-kpis.tsx
| interface MetricTabTriggerProps extends React.ComponentProps<typeof TabsTrigger> { | ||
| children: React.ReactNode; | ||
| } | ||
|
|
||
| const KpiTabTrigger: React.FC<KpiTabTriggerProps> = ({children, ...props}) => { | ||
| const MetricTabTrigger: React.FC<MetricTabTriggerProps> = ({children, ...props}) => { | ||
| return ( | ||
| <TabsTrigger className='h-auto' {...props}> | ||
| {children} | ||
| </TabsTrigger> | ||
| ); | ||
| }; | ||
| MetricTabTrigger.displayName = 'MetricTabTrigger'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any current call sites already pass className to Metric/KPI tab triggers.
rg -nP --type=tsx '<(MetricTabTrigger|KpiTabTrigger)\b[^>]*\bclassName=' -C2Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# First, read the file to confirm the current code state and imports
cat -n apps/shade/src/components/ui/tabs.tsx | head -210 | tail -50Repository: TryGhost/Ghost
Length of output: 1781
🏁 Script executed:
# Check if cn() is imported in the file
rg -n "^import.*cn" apps/shade/src/components/ui/tabs.tsxRepository: TryGhost/Ghost
Length of output: 91
🏁 Script executed:
# Search for MetricTabTrigger and KpiTabTrigger definitions and usages
rg -n "MetricTabTrigger|KpiTabTrigger" apps/shade/src/components/ui/tabs.tsxRepository: TryGhost/Ghost
Length of output: 437
🏁 Script executed:
# Search for MetricTabTrigger usage across the codebase
rg -l "MetricTabTrigger" --type=ts --type=js --type=tsx --type=jsxRepository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Search for MetricTabTrigger usage across the codebase
rg "MetricTabTrigger|KpiTabTrigger" -B2 -A2 | head -100Repository: TryGhost/Ghost
Length of output: 10577
🏁 Script executed:
# Search specifically for className being passed to these components
rg "<(MetricTabTrigger|KpiTabTrigger)" -B1 -A3 | grep -A3 "className"Repository: TryGhost/Ghost
Length of output: 2804
Merge className in MetricTabTrigger instead of letting props overwrite it.
Callers in newsletters-kpis.tsx and growth-kpis.tsx are passing className to add conditional styling (e.g., cursor-auto after:hidden). Currently, these custom classes overwrite the base h-auto styling because props spreads after the literal prop. Destructure and merge with cn() to preserve both.
Suggested change
-const MetricTabTrigger: React.FC<MetricTabTriggerProps> = ({children, ...props}) => {
+const MetricTabTrigger: React.FC<MetricTabTriggerProps> = ({children, className, ...props}) => {
return (
- <TabsTrigger className='h-auto' {...props}>
+ <TabsTrigger className={cn('h-auto', className)} {...props}>
{children}
</TabsTrigger>
);
};📝 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.
| interface MetricTabTriggerProps extends React.ComponentProps<typeof TabsTrigger> { | |
| children: React.ReactNode; | |
| } | |
| const KpiTabTrigger: React.FC<KpiTabTriggerProps> = ({children, ...props}) => { | |
| const MetricTabTrigger: React.FC<MetricTabTriggerProps> = ({children, ...props}) => { | |
| return ( | |
| <TabsTrigger className='h-auto' {...props}> | |
| {children} | |
| </TabsTrigger> | |
| ); | |
| }; | |
| MetricTabTrigger.displayName = 'MetricTabTrigger'; | |
| interface MetricTabTriggerProps extends React.ComponentProps<typeof TabsTrigger> { | |
| children: React.ReactNode; | |
| } | |
| const MetricTabTrigger: React.FC<MetricTabTriggerProps> = ({children, className, ...props}) => { | |
| return ( | |
| <TabsTrigger className={cn('h-auto', className)} {...props}> | |
| {children} | |
| </TabsTrigger> | |
| ); | |
| }; | |
| MetricTabTrigger.displayName = 'MetricTabTrigger'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/shade/src/components/ui/tabs.tsx` around lines 188 - 199,
MetricTabTrigger currently allows incoming props.className to overwrite the base
'h-auto' class because props are spread after the literal className; fix by
destructuring className from MetricTabTriggerProps (e.g., const MetricTabTrigger
= ({ children, className, ...rest }) => ...) and merge it with the base class
using the project's utility (cn('h-auto', className)), then pass the merged
className to TabsTrigger along with ...rest; keep MetricTabTrigger.displayName
unchanged.
1 participant
ref https://linear.app/ghost/issue/DES-1343/component-core-consolidation