feat: add recharts library and implement various chart components wit… - #61
Merged
Conversation
…h stories (#60) - Added recharts dependency to package.json - Created Chart component with ChartContainer, ChartTooltip, ChartTooltipContent, ChartLegend, and ChartLegendContent - Implemented multiple chart types (Area, Bar, Line, Pie, Radar, Radial) in Chart.stories.tsx - Updated Card component styles for better layout consistency - Exported new chart components in index.ts files
📝 WalkthroughWalkthroughAdds Recharts as a dependency, a new charting UI module (ChartContainer, ChartStyle, tooltip/legend helpers, context), Storybook stories demonstrating many chart types, and public exports for chart components; minor UI and story tweaks elsewhere. Changes
Sequence DiagramsequenceDiagram
participant App
participant ChartContainer
participant ChartContext
participant ChartStyle
participant Recharts
participant Tooltip as ChartTooltipContent
participant Legend as ChartLegendContent
App->>ChartContainer: render with ChartConfig and children
ChartContainer->>ChartContext: provide config
ChartContainer->>ChartStyle: generate scoped CSS vars (id, config)
ChartStyle-->>ChartContainer: inject theme CSS
ChartContainer->>Recharts: wrap ResponsiveContainer and chart primitives
App->>Recharts: user interaction (hover/click)
Recharts->>Tooltip: request tooltip (payload)
Tooltip->>ChartContext: useChart() to resolve labels/icons
Tooltip->>Tooltip: call getPayloadConfigFromPayload(payload, key)
Tooltip-->>Recharts: render formatted tooltip
Recharts->>Legend: request legend payload
Legend->>ChartContext: useChart() to resolve legend entries
Legend-->>Recharts: render custom legend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/index.ts (1)
237-241: ExposeChartConfigat the package root for TS consumers.
ChartConfigis exported insrc/components/ui/index.tsbut not re-exported here, so root consumers lose the type unless they deep-import.💡 Suggested diff
// Rich Text Editor RichTextEditor, // Types type RichTextEditorProps, + type ChartConfig, type CircularProgressProps,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 237 - 241, The package root index is missing a re-export of the ChartConfig type (currently exported from the UI barrel), so add a type-only re-export for ChartConfig alongside the existing exports (where RichTextEditor and RichTextEditorProps are exported) so TypeScript consumers can import ChartConfig from the package root; re-export the ChartConfig symbol from the UI barrel using a type export.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/chart.tsx`:
- Around line 235-239: The tooltip currently hides zero values because it uses a
truthy check on item.value; change the conditional around the span that renders
{item.value.toLocaleString()} to explicitly check for null/undefined (e.g.,
item.value !== null && item.value !== undefined) so that 0 is rendered; update
the conditional near the span inside the tooltip rendering (the block that
references item.value and the className "text-foreground font-mono font-medium
tabular-nums") to use this explicit existence check.
- Around line 301-302: The legend rendering currently shows only
itemConfig?.label which leaves the legend blank when config lookup fails; update
the JSX in the Chart component where {itemConfig?.label} is rendered to provide
a fallback (e.g., use the series key/id or a humanized version like itemKey ||
item.id or a formatted fallback string) by replacing the single-access with a
nullish-coalescing or logical-or fallback that uses the unique identifier
(itemKey, item.id, or similar) so the legend always displays meaningful text
when itemConfig is undefined.
- Around line 35-59: ChartContainer currently reads id from props but never
applies it to the root DOM node; add id={id} to the root <div> in the
ChartContainer component (keep the existing data-chart={chartId} attribute) so
the id prop passed into ChartContainer is forwarded to the root element and
available for anchors/tests; locate the ChartContainer function and add id={id}
on the div that currently has data-slot="chart" and data-chart={chartId}.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 237-241: The package root index is missing a re-export of the
ChartConfig type (currently exported from the UI barrel), so add a type-only
re-export for ChartConfig alongside the existing exports (where RichTextEditor
and RichTextEditorProps are exported) so TypeScript consumers can import
ChartConfig from the package root; re-export the ChartConfig symbol from the UI
barrel using a type export.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78ecb85d-0de2-4c41-8dd2-768aab97efb4
📒 Files selected for processing (6)
package.jsonsrc/components/ui/Chart.stories.tsxsrc/components/ui/card.tsxsrc/components/ui/chart.tsxsrc/components/ui/index.tssrc/index.ts
Comment on lines
+35
to
+59
| function ChartContainer({ | ||
| id, | ||
| className, | ||
| children, | ||
| config, | ||
| ...props | ||
| }: React.ComponentProps<"div"> & { | ||
| config: ChartConfig | ||
| children: React.ComponentProps< | ||
| typeof RechartsPrimitive.ResponsiveContainer | ||
| >["children"] | ||
| }) { | ||
| const uniqueId = React.useId() | ||
| const chartId = `chart-${id || uniqueId.replace(/:/g, "")}` | ||
|
|
||
| return ( | ||
| <ChartContext.Provider value={{ config }}> | ||
| <div | ||
| data-slot="chart" | ||
| data-chart={chartId} | ||
| className={cn( | ||
| "[&_.recharts-cartesian-axis-tick_text]:fill-muted-foreground [&_.recharts-cartesian-grid_line[stroke='#ccc']]:stroke-border/50 [&_.recharts-curve.recharts-tooltip-cursor]:stroke-border [&_.recharts-polar-grid_[stroke='#ccc']]:stroke-border [&_.recharts-radial-bar-background-sector]:fill-muted [&_.recharts-rectangle.recharts-tooltip-cursor]:fill-muted [&_.recharts-reference-line_[stroke='#ccc']]:stroke-border flex aspect-video justify-center text-xs [&_.recharts-dot[stroke='#fff']]:stroke-transparent [&_.recharts-layer]:outline-hidden [&_.recharts-sector]:outline-hidden [&_.recharts-sector[stroke='#fff']]:stroke-transparent [&_.recharts-surface]:outline-hidden", | ||
| className | ||
| )} | ||
| {...props} |
There was a problem hiding this comment.
Forward the id prop to the root element.
Line 36 reads id from ComponentProps<"div">, but it is not applied to the DOM node. That breaks expected anchor/test selectors.
💡 Suggested diff
<div
+ id={id}
data-slot="chart"
data-chart={chartId}
className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/chart.tsx` around lines 35 - 59, ChartContainer currently
reads id from props but never applies it to the root DOM node; add id={id} to
the root <div> in the ChartContainer component (keep the existing
data-chart={chartId} attribute) so the id prop passed into ChartContainer is
forwarded to the root element and available for anchors/tests; locate the
ChartContainer function and add id={id} on the div that currently has
data-slot="chart" and data-chart={chartId}.
Comment on lines
+80
to
+99
| <style | ||
| dangerouslySetInnerHTML={{ | ||
| __html: Object.entries(THEMES) | ||
| .map( | ||
| ([theme, prefix]) => ` | ||
| ${prefix} [data-chart=${id}] { | ||
| ${colorConfig | ||
| .map(([key, itemConfig]) => { | ||
| const color = | ||
| itemConfig.theme?.[theme as keyof typeof itemConfig.theme] || | ||
| itemConfig.color | ||
| return color ? ` --color-${key}: ${color};` : null | ||
| }) | ||
| .join("\n")} | ||
| } | ||
| ` | ||
| ) | ||
| .join("\n"), | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Harden dynamic style generation; avoid raw dangerouslySetInnerHTML injection.
Line 81 interpolates dynamic id/keys/colors directly into HTML/CSS text. This is an avoidable injection surface and can also break selectors with malformed values.
🔒 Suggested diff
const ChartStyle = ({ id, config }: { id: string; config: ChartConfig }) => {
+ const safeChartId = CSS.escape(id)
+ const toVarName = (value: string) => value.replace(/[^a-zA-Z0-9_-]/g, "")
+ const toCssValue = (value: string) => value.replace(/[;"{}<>]/g, "")
+
const colorConfig = Object.entries(config).filter(
([, config]) => config.theme || config.color
)
if (!colorConfig.length) {
return null
}
+ const cssText = Object.entries(THEMES)
+ .map(
+ ([theme, prefix]) => `
+${prefix} [data-chart="${safeChartId}"] {
+${colorConfig
+ .map(([key, itemConfig]) => {
+ const color =
+ itemConfig.theme?.[theme as keyof typeof itemConfig.theme] ||
+ itemConfig.color
+ return color
+ ? ` --color-${toVarName(key)}: ${toCssValue(String(color))};`
+ : null
+ })
+ .filter(Boolean)
+ .join("\n")}
+}
+`
+ )
+ .join("\n")
+
- return (
- <style
- dangerouslySetInnerHTML={{
- __html: Object.entries(THEMES)
- .map(
- ([theme, prefix]) => `
-${prefix} [data-chart=${id}] {
-${colorConfig
- .map(([key, itemConfig]) => {
- const color =
- itemConfig.theme?.[theme as keyof typeof itemConfig.theme] ||
- itemConfig.color
- return color ? ` --color-${key}: ${color};` : null
- })
- .join("\n")}
-}
-`
- )
- .join("\n"),
- }}
- />
- )
+ return <style>{cssText}</style>
}#!/bin/bash
# Verify raw dynamic HTML/CSS interpolation points in chart style generation.
sed -n '70,110p' src/components/ui/chart.tsx
rg -n 'dangerouslySetInnerHTML|data-chart=\\$\\{id\\}|--color-\\$\\{key\\}' src/components/ui/chart.tsx
rg -n 'CSS\\.escape|toVarName|toCssValue' src/components/ui/chart.tsx🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 80-80: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.4.4)
[error] 81-81: Avoid passing content using the dangerouslySetInnerHTML prop.
(lint/security/noDangerouslySetInnerHtml)
Comment on lines
+235
to
+239
| {item.value && ( | ||
| <span className="text-foreground font-mono font-medium tabular-nums"> | ||
| {item.value.toLocaleString()} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
Render zero values in tooltip output.
Line 235 uses a truthy check, so 0 values are hidden.
💡 Suggested diff
- {item.value && (
+ {item.value !== undefined && item.value !== null && (
<span className="text-foreground font-mono font-medium tabular-nums">
{item.value.toLocaleString()}
</span>
)}📝 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
| {item.value && ( | |
| <span className="text-foreground font-mono font-medium tabular-nums"> | |
| {item.value.toLocaleString()} | |
| </span> | |
| )} | |
| {item.value !== undefined && item.value !== null && ( | |
| <span className="text-foreground font-mono font-medium tabular-nums"> | |
| {item.value.toLocaleString()} | |
| </span> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/chart.tsx` around lines 235 - 239, The tooltip currently
hides zero values because it uses a truthy check on item.value; change the
conditional around the span that renders {item.value.toLocaleString()} to
explicitly check for null/undefined (e.g., item.value !== null && item.value !==
undefined) so that 0 is rendered; update the conditional near the span inside
the tooltip rendering (the block that references item.value and the className
"text-foreground font-mono font-medium tabular-nums") to use this explicit
existence check.
Comment on lines
+301
to
+302
| {itemConfig?.label} | ||
| </div> |
There was a problem hiding this comment.
Add a legend label fallback when config lookup misses.
Line 301 renders only itemConfig?.label; if config resolution fails, legend text is blank.
💡 Suggested diff
- {itemConfig?.label}
+ {itemConfig?.label ?? item.value}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/chart.tsx` around lines 301 - 302, The legend rendering
currently shows only itemConfig?.label which leaves the legend blank when config
lookup fails; update the JSX in the Chart component where {itemConfig?.label} is
rendered to provide a fallback (e.g., use the series key/id or a humanized
version like itemKey || item.id or a formatted fallback string) by replacing the
single-access with a nullish-coalescing or logical-or fallback that uses the
unique identifier (itemKey, item.id, or similar) so the legend always displays
meaningful text when itemConfig is undefined.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/index.ts (2)
50-51: ExportChartConfigfrom the root barrel to complete the chart API.
ChartContainer(Line 50) requires aconfigprop typed asChartConfig, butsrc/index.tsdoes not re-export that type. Root consumers lose an ergonomic way to type chart configs.Proposed fix
// Types + type ChartConfig, type RichTextEditorProps,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 50 - 51, Export the ChartConfig type from the root barrel so consumers can import it alongside ChartContainer and related components: add ChartConfig to the exported list in the root index module (the same place ChartContainer, ChartTooltip, ChartLegend, ChartStyle are exported) so ChartContainer's config prop is publicly typed; ensure you import ChartConfig from its defining module and include it in the existing export statement.
329-329: Document the recharts namespace re-export and consider tree-shaking implications.The
export * as recharts from 'recharts're-export couples your package API directly to upstream recharts surface/versioning. Additionally, namespace re-exports defeat recharts 2.15.4's tree-shaking optimization—bundlers must assume the entire namespace could be used, retaining unused code.Since you have an internal Chart component that uses recharts, this re-export may be intentional. If so, document it explicitly in your README (e.g., mentioning that consumers can access
rechartsexports through this package or should import named components directly fromrecharts). Recommend guiding consumers toward named imports (import { LineChart } from 'recharts') to preserve tree-shaking benefits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` at line 329, The export line export * as recharts from 'recharts' exposes the entire recharts namespace (and can defeat tree-shaking) and couples your package API to recharts; either remove this namespace re-export or clearly document the intention: if you need to keep it (because internal Chart component exposes recharts types/props), add a README note explaining that consumers can access recharts via this package (and recommend preferred named imports like import { LineChart } from 'recharts' to preserve tree-shaking) and mention the specific exported namespace export * as recharts; otherwise delete that export and update internal uses (e.g., Chart) to import recharts directly from 'recharts' so consumers are not forced to pull the whole namespace.
🤖 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/index.ts`:
- Around line 50-51: Export the ChartConfig type from the root barrel so
consumers can import it alongside ChartContainer and related components: add
ChartConfig to the exported list in the root index module (the same place
ChartContainer, ChartTooltip, ChartLegend, ChartStyle are exported) so
ChartContainer's config prop is publicly typed; ensure you import ChartConfig
from its defining module and include it in the existing export statement.
- Line 329: The export line export * as recharts from 'recharts' exposes the
entire recharts namespace (and can defeat tree-shaking) and couples your package
API to recharts; either remove this namespace re-export or clearly document the
intention: if you need to keep it (because internal Chart component exposes
recharts types/props), add a README note explaining that consumers can access
recharts via this package (and recommend preferred named imports like import {
LineChart } from 'recharts' to preserve tree-shaking) and mention the specific
exported namespace export * as recharts; otherwise delete that export and update
internal uses (e.g., Chart) to import recharts directly from 'recharts' so
consumers are not forced to pull the whole namespace.
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
1 participant
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.
…h stories (#60)
Summary by CodeRabbit
New Features
Documentation / Examples
Chores