feat(react-ui-base): add graph compound component#2269
feat(react-ui-base): add graph compound component#2269lachieh wants to merge 2 commits intolachieh/tam-1057-message-suggestionsfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The new Graph primitives are a good separation, but validateGraphData currently makes unsupported-type largely unreachable and risks silent data corruption when dataset labels collide (overwriting keys in chartData). The base GraphErrorBoundary unconditionally console.errors, which is noisy/leaky for a shared library. In ui-registry, the styled Graph never handles the unsupported-type state, resulting in an empty component for that case.
Summary of changes
Added Graph compound component primitives to @tambo-ai/react-ui-base
- Introduced a new
graphentrypoint export inpackages/react-ui-base/package.json("./graph") to publish ESM/CJS builds. - Added base primitives under
packages/react-ui-base/src/graph/*:Graph.Rootwith context + validation (validateGraphData).Graph.Title,Graph.Chartrender-prop primitives.Graph.Loadingprimitive for pre-valid states.Graph.ErrorBoundaryclass-based boundary.- Test coverage for
validateGraphData.
- Re-exported graph API from
packages/react-ui-base/src/index.tsand from the newpackages/react-ui-base/src/graph/index.tsx.
Refactored styled registry Graph to compose base primitives
- Updated
packages/ui-registry/src/components/graph/graph.tsxto:- Import
Graphprimitives from@tambo-ai/react-ui-base/graph. - Move loading UI into a
GraphLoadingIndicatorbound toGraphBase.Loadingstatus. - Move chart-type branching into
ChartRenderer+ per-chart renderer helpers. - Wrap the styled graph with
GraphBase.ErrorBoundaryand useGraphBase.Root/Title/Chartfor composition. - Removed the bespoke local
GraphErrorBoundaryand inline validation/transformation logic.
- Import
| const hasValidStructure = | ||
| data.type && | ||
| data.labels && | ||
| data.datasets && | ||
| Array.isArray(data.labels) && | ||
| Array.isArray(data.datasets) && | ||
| data.labels.length > 0 && | ||
| data.datasets.length > 0; | ||
|
|
||
| if (!hasValidStructure) { | ||
| return { status: "invalid-structure" }; | ||
| } | ||
|
|
||
| const validDatasets = data.datasets.filter( | ||
| (dataset) => | ||
| dataset.label && | ||
| dataset.data && | ||
| Array.isArray(dataset.data) && | ||
| dataset.data.length > 0, | ||
| ); | ||
|
|
||
| if (validDatasets.length === 0) { | ||
| return { status: "no-valid-datasets" }; | ||
| } | ||
|
|
||
| if (!["bar", "line", "pie"].includes(data.type)) { | ||
| return { status: "unsupported-type", type: data.type }; | ||
| } |
There was a problem hiding this comment.
validateGraphData checks hasValidStructure (including data.type) before it checks for unsupported chart types. That means unsupported-type is effectively unreachable for values that still satisfy the structure check but have an unknown type—except via unsafe casts in tests. If consumers pass runtime data (e.g., from an API) and it’s typed loosely, you likely want unsupported-type to be reported rather than treating it as invalid-structure or relying on type-level constraints.
Also, data.type && is redundant since type is a required field in GraphData; the only realistic invalidity here is runtime shape mismatch, which should be handled deterministically.
Suggestion
Reorder validation so unsupported-type is checked immediately after the !data guard, and simplify the structure check to focus on arrays/lengths. For example:
export function validateGraphData(data: GraphData | undefined): GraphDataState {
if (!data) return { status: "no-data" };
if (!(["bar", "line", "pie"] as const).includes(data.type)) {
return { status: "unsupported-type", type: data.type };
}
const hasValidStructure =
Array.isArray(data.labels) &&
Array.isArray(data.datasets) &&
data.labels.length > 0 &&
data.datasets.length > 0;
if (!hasValidStructure) return { status: "invalid-structure" };
// ...rest unchanged
}This makes unsupported-type meaningful for runtime inputs while keeping the “streaming/partial” behavior. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| const validDatasets = data.datasets.filter( | ||
| (dataset) => | ||
| dataset.label && | ||
| dataset.data && | ||
| Array.isArray(dataset.data) && | ||
| dataset.data.length > 0, | ||
| ); | ||
|
|
||
| if (validDatasets.length === 0) { | ||
| return { status: "no-valid-datasets" }; | ||
| } | ||
|
|
||
| if (!["bar", "line", "pie"].includes(data.type)) { | ||
| return { status: "unsupported-type", type: data.type }; | ||
| } | ||
|
|
||
| const maxDataPoints = Math.min( | ||
| data.labels.length, | ||
| Math.min(...validDatasets.map((d) => d.data.length)), | ||
| ); | ||
|
|
||
| const chartData = data.labels.slice(0, maxDataPoints).map((label, index) => ({ | ||
| name: label, | ||
| ...Object.fromEntries( | ||
| validDatasets.map((dataset) => [dataset.label, dataset.data[index] ?? 0]), | ||
| ), | ||
| })); |
There was a problem hiding this comment.
The charting data uses dataset.label as an object key via Object.fromEntries. If labels collide (two datasets with the same label) or if labels contain characters that the charting library treats specially, you’ll silently overwrite earlier datasets in chartData while still rendering multiple series that reference the same dataKey. That’s a correctness bug that will surface as duplicated/incorrect series.
Suggestion
Enforce uniqueness (and ideally non-whitespace) for dataset.label during validation and either:
- return a dedicated invalid state (e.g.
{ status: "duplicate-dataset-labels"; labels: string[] }), or - normalize keys by generating stable unique keys (e.g.
label,label__2, …) and expose those keys invalidDatasets(add akeyfield) so renderers usedataKey={dataset.key}.
Minimal approach (validation failure):
const trimmed = validDatasets.map(d => d.label.trim()).filter(Boolean);
const duplicates = trimmed.filter((l, i) => trimmed.indexOf(l) !== i);
if (duplicates.length) {
return { status: "invalid-structure" }; // or a new status
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing a safer label/key strategy.
| componentDidCatch(error: Error, errorInfo: React.ErrorInfo): void { | ||
| console.error("Error rendering chart:", error, errorInfo); | ||
| } |
There was a problem hiding this comment.
GraphErrorBoundary logs errors unconditionally via console.error. In libraries, this can be noisy and may leak sensitive data into logs in production (e.g., chart data embedded in error messages). Prefer delegating logging to the host app or gating logs behind an environment check.
Suggestion
Consider one of:
- Only log in non-production environments:
if (process.env.NODE_ENV !== "production") {
console.error("Error rendering chart:", error, errorInfo);
}- Or accept an optional
onError?: (error, errorInfo) => voidprop and call that instead of logging.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with an onError prop + production-safe default.
| return ( | ||
| <GraphBase.ErrorBoundary | ||
| renderError={() => ( | ||
| <div className={cn(graphVariants({ variant, size }), className)}> | ||
| <div className="p-4 flex items-center justify-center h-full"> | ||
| <div className="text-destructive text-center"> | ||
| <p className="font-medium">Error loading chart</p> | ||
| <p className="text-sm mt-1"> | ||
| An error occurred while rendering. Please try again. | ||
| </p> | ||
| </div> | ||
| <span className="text-sm">Awaiting data...</span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Check if we have the minimum viable data structure | ||
| const hasValidStructure = | ||
| data.type && | ||
| data.labels && | ||
| data.datasets && | ||
| Array.isArray(data.labels) && | ||
| Array.isArray(data.datasets) && | ||
| data.labels.length > 0 && | ||
| data.datasets.length > 0; | ||
|
|
||
| if (!hasValidStructure) { | ||
| return ( | ||
| <div | ||
| ref={ref} | ||
| className={cn(graphVariants({ variant, size }), className)} | ||
| {...props} | ||
| > | ||
| <div className="p-4 h-full flex items-center justify-center"> | ||
| <div className="text-muted-foreground text-center"> | ||
| <p className="text-sm">Building chart...</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Filter datasets to only include those with valid data | ||
| const validDatasets = data.datasets.filter( | ||
| (dataset) => | ||
| dataset.label && | ||
| dataset.data && | ||
| Array.isArray(dataset.data) && | ||
| dataset.data.length > 0, | ||
| ); | ||
|
|
||
| if (validDatasets.length === 0) { | ||
| return ( | ||
| <div | ||
| ref={ref} | ||
| className={cn(graphVariants({ variant, size }), className)} | ||
| {...props} | ||
| > | ||
| <div className="p-4 h-full flex items-center justify-center"> | ||
| <div className="text-muted-foreground text-center"> | ||
| <p className="text-sm">Preparing datasets...</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Use the minimum length between labels and the shortest dataset | ||
| const maxDataPoints = Math.min( | ||
| data.labels.length, | ||
| Math.min(...validDatasets.map((d) => d.data.length)), | ||
| ); | ||
|
|
||
| // Transform data for Recharts using only available data points | ||
| const chartData = data.labels | ||
| .slice(0, maxDataPoints) | ||
| .map((label, index) => ({ | ||
| name: label, | ||
| ...Object.fromEntries( | ||
| validDatasets.map((dataset) => [ | ||
| dataset.label, | ||
| dataset.data[index] ?? 0, | ||
| ]), | ||
| ), | ||
| })); | ||
|
|
||
| const renderChart = () => { | ||
| if (!["bar", "line", "pie"].includes(data.type)) { | ||
| return ( | ||
| <div className="h-full flex items-center justify-center"> | ||
| <div className="text-muted-foreground text-center"> | ||
| <p className="text-sm">Unsupported chart type: {data.type}</p> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| switch (data.type) { | ||
| case "bar": | ||
| return ( | ||
| <RechartsCore.BarChart data={chartData}> | ||
| <RechartsCore.CartesianGrid | ||
| strokeDasharray="3 3" | ||
| vertical={false} | ||
| stroke="var(--border)" | ||
| /> | ||
| <RechartsCore.XAxis | ||
| dataKey="name" | ||
| stroke="var(--muted-foreground)" | ||
| axisLine={false} | ||
| tickLine={false} | ||
| /> | ||
| <RechartsCore.YAxis | ||
| stroke="var(--muted-foreground)" | ||
| axisLine={false} | ||
| tickLine={false} | ||
| /> | ||
| <RechartsCore.Tooltip | ||
| cursor={{ | ||
| fill: "var(--muted-foreground)", | ||
| fillOpacity: 0.1, | ||
| radius: 4, | ||
| }} | ||
| contentStyle={{ | ||
| backgroundColor: "white", | ||
| border: "1px solid #e5e7eb", | ||
| borderRadius: "var(--radius)", | ||
| color: "var(--foreground)", | ||
| }} | ||
| /> | ||
| {showLegend && ( | ||
| <RechartsCore.Legend | ||
| wrapperStyle={{ | ||
| color: "var(--foreground)", | ||
| }} | ||
| /> | ||
| )} | ||
| {validDatasets.map((dataset, index) => ( | ||
| <RechartsCore.Bar | ||
| key={dataset.label} | ||
| dataKey={dataset.label} | ||
| fill={ | ||
| dataset.color ?? defaultColors[index % defaultColors.length] | ||
| } | ||
| radius={[4, 4, 0, 0]} | ||
| /> | ||
| ))} | ||
| </RechartsCore.BarChart> | ||
| ); | ||
|
|
||
| case "line": | ||
| return ( | ||
| <RechartsCore.LineChart data={chartData}> | ||
| <RechartsCore.CartesianGrid | ||
| strokeDasharray="3 3" | ||
| vertical={false} | ||
| stroke="var(--border)" | ||
| /> | ||
| <RechartsCore.XAxis | ||
| dataKey="name" | ||
| stroke="var(--muted-foreground)" | ||
| axisLine={false} | ||
| tickLine={false} | ||
| /> | ||
| <RechartsCore.YAxis | ||
| stroke="var(--muted-foreground)" | ||
| axisLine={false} | ||
| tickLine={false} | ||
| /> | ||
| <RechartsCore.Tooltip | ||
| cursor={{ | ||
| stroke: "var(--muted)", | ||
| strokeWidth: 2, | ||
| strokeOpacity: 0.3, | ||
| }} | ||
| contentStyle={{ | ||
| backgroundColor: "white", | ||
| border: "1px solid #e5e7eb", | ||
| borderRadius: "var(--radius)", | ||
| color: "var(--foreground)", | ||
| }} | ||
| /> | ||
| {showLegend && ( | ||
| <RechartsCore.Legend | ||
| wrapperStyle={{ | ||
| color: "var(--foreground)", | ||
| }} | ||
| /> | ||
| )} | ||
| {validDatasets.map((dataset, index) => ( | ||
| <RechartsCore.Line | ||
| key={dataset.label} | ||
| type="monotone" | ||
| dataKey={dataset.label} | ||
| stroke={ | ||
| dataset.color ?? defaultColors[index % defaultColors.length] | ||
| } | ||
| dot={false} | ||
| /> | ||
| ))} | ||
| </RechartsCore.LineChart> | ||
| ); | ||
|
|
||
| case "pie": { | ||
| // For pie charts, use the first valid dataset | ||
| const pieDataset = validDatasets[0]; | ||
| if (!pieDataset) { | ||
| return ( | ||
| <div className="h-full flex items-center justify-center"> | ||
| <div className="text-muted-foreground text-center"> | ||
| <p className="text-sm">No valid dataset for pie chart</p> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <RechartsCore.PieChart> | ||
| <RechartsCore.Pie | ||
| data={pieDataset.data | ||
| .slice(0, maxDataPoints) | ||
| .map((value, index) => ({ | ||
| name: data.labels[index], | ||
| value, | ||
| fill: defaultColors[index % defaultColors.length], | ||
| }))} | ||
| dataKey="value" | ||
| nameKey="name" | ||
| cx="50%" | ||
| cy="50%" | ||
| labelLine={false} | ||
| outerRadius={80} | ||
| fill="#8884d8" | ||
| /> | ||
| <RechartsCore.Tooltip | ||
| contentStyle={{ | ||
| backgroundColor: "white", | ||
| border: "1px solid #e5e7eb", | ||
| borderRadius: "var(--radius)", | ||
| color: "var(--foreground)", | ||
| boxShadow: "0 2px 4px rgba(0,0,0,0.1)", | ||
| }} | ||
| itemStyle={{ | ||
| color: "var(--foreground)", | ||
| }} | ||
| labelStyle={{ | ||
| color: "var(--foreground)", | ||
| }} | ||
| /> | ||
| {showLegend && ( | ||
| <RechartsCore.Legend | ||
| wrapperStyle={{ | ||
| color: "var(--foreground)", | ||
| }} | ||
| /> | ||
| )} | ||
| </RechartsCore.PieChart> | ||
| ); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <GraphErrorBoundary className={className} variant={variant} size={size}> | ||
| <div | ||
| )} | ||
| > | ||
| <GraphBase.Root | ||
| ref={ref} | ||
| className={cn(graphVariants({ variant, size }), className)} | ||
| data={data} | ||
| title={title} | ||
| showLegend={showLegend} | ||
| {...props} | ||
| > | ||
| <GraphLoadingIndicator /> | ||
| <div className="p-4 h-full"> | ||
| {title && ( | ||
| <h3 className="text-lg font-medium mb-4 text-foreground"> | ||
| {title} | ||
| </h3> | ||
| )} | ||
| <div className="w-full h-[calc(100%-2rem)]"> | ||
| <RechartsCore.ResponsiveContainer width="100%" height="100%"> | ||
| {renderChart()} | ||
| </RechartsCore.ResponsiveContainer> | ||
| </div> | ||
| <GraphBase.Title className="text-lg font-medium mb-4 text-foreground" /> | ||
| <GraphBase.Chart | ||
| className="w-full h-[calc(100%-2rem)]" | ||
| render={(chartProps) => ( | ||
| <RechartsCore.ResponsiveContainer width="100%" height="100%"> | ||
| <ChartRenderer {...chartProps} /> | ||
| </RechartsCore.ResponsiveContainer> | ||
| )} | ||
| /> | ||
| </div> | ||
| </div> | ||
| </GraphErrorBoundary> | ||
| </GraphBase.Root> | ||
| </GraphBase.ErrorBoundary> |
There was a problem hiding this comment.
The new base layer introduces an explicit unsupported-type state, but the styled ui-registry component never renders anything for that state. GraphBase.Loading only covers no-data | invalid-structure | no-valid-datasets, and GraphBase.Chart renders only for valid, so users will see an empty graph container when unsupported-type occurs.
Even if your graphDataSchema currently restricts types, runtime data can still violate it (and GraphBase.Root accepts data: GraphData | undefined, which may be constructed/loosened elsewhere). This is a user-facing correctness gap.
Suggestion
Add an explicit unsupported-type UI path by exposing dataState from the base context (either via a new primitive like GraphBase.Status or by adding a GraphBase.Error primitive that renders for non-valid terminal states).
A minimal change in ui-registry is to add a new base primitive (recommended) like:
// in react-ui-base
export const GraphStatus = ({ render }: { render: (s: GraphDataState) => React.ReactNode }) => {
const { dataState } = useGraphRootContext();
return <>{render(dataState)}</>;
};Then in ui-registry:
<GraphBase.Status render={(s) =>
s.status === "unsupported-type" ? (
<div className="p-4 text-sm text-muted-foreground">Unsupported chart type: {s.type}</div>
) : null
} />Reply with "@CharlieHelps yes please" if you'd like me to add a commit adding a base Graph.Status primitive and wiring this UI in the registry component.
| function getLoadingMessage(status: GraphLoadingStatus): string { | ||
| switch (status) { | ||
| case "no-data": | ||
| return "Awaiting data..."; | ||
| case "invalid-structure": | ||
| return "Building chart..."; | ||
| case "no-valid-datasets": | ||
| return "Preparing datasets..."; | ||
| } | ||
| } |
There was a problem hiding this comment.
GraphLoadingIndicator calls getLoadingMessage(status) in all branches, but the helper lacks a default case. Today it’s exhaustive for GraphLoadingStatus, but the moment a new status is added in the base package, the registry UI will silently return undefined (and render nothing) without an obvious failure.
This is a cross-package boundary: it’s safer to make the mapping total.
Suggestion
Add a default branch (or an exhaustive assertNever) to avoid silent UI gaps:
function getLoadingMessage(status: GraphLoadingStatus): string {
switch (status) {
case "no-data": return "Awaiting data...";
case "invalid-structure": return "Building chart...";
case "no-valid-datasets": return "Preparing datasets...";
default: return "Loading...";
}
}Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this change.
| it("returns invalid-structure when data has no labels", () => { | ||
| const data = { | ||
| type: "bar", | ||
| labels: [], | ||
| datasets: [{ label: "A", data: [1] }], | ||
| } as GraphData; | ||
| expect(validateGraphData(data)).toEqual({ status: "invalid-structure" }); | ||
| }); | ||
|
|
||
| it("returns invalid-structure when data has no datasets", () => { | ||
| const data = { | ||
| type: "bar", | ||
| labels: ["Jan"], | ||
| datasets: [], | ||
| } as GraphData; | ||
| expect(validateGraphData(data)).toEqual({ status: "invalid-structure" }); | ||
| }); | ||
|
|
||
| it("returns no-valid-datasets when datasets have no data", () => { | ||
| const data: GraphData = { | ||
| type: "bar", | ||
| labels: ["Jan"], | ||
| datasets: [{ label: "A", data: [] }], | ||
| }; | ||
| expect(validateGraphData(data)).toEqual({ status: "no-valid-datasets" }); | ||
| }); | ||
|
|
||
| it("returns no-valid-datasets when datasets have no label", () => { | ||
| const data: GraphData = { | ||
| type: "bar", | ||
| labels: ["Jan"], | ||
| datasets: [{ label: "", data: [1] }], | ||
| }; | ||
| expect(validateGraphData(data)).toEqual({ status: "no-valid-datasets" }); | ||
| }); | ||
|
|
||
| it("returns unsupported-type for unknown chart types", () => { | ||
| const data = { | ||
| type: "radar" as "bar", | ||
| labels: ["Jan"], | ||
| datasets: [{ label: "A", data: [1] }], | ||
| }; | ||
| expect(validateGraphData(data)).toEqual({ | ||
| status: "unsupported-type", | ||
| type: "radar", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
validateGraphData.test.ts uses as GraphData to construct invalid inputs (e.g. labels: [], type: "radar" as "bar"). This undermines the purpose of the tests: you’re testing runtime validation, but the inputs are forced through the type system in a way production code wouldn’t.
Given the function’s stated goal (“partial/streaming data”), the tests would be more valuable if they used a looser input type and exercised truly partial shapes without type assertions.
Suggestion
If you keep validateGraphData(data: GraphData | undefined), then these tests are necessarily artificial. Prefer changing the function signature to accept unknown or Partial<GraphData> and validate properly; then update tests to pass plain objects without as casts.
Reply with "@CharlieHelps yes please" if you want me to add a commit that updates the signature + tests accordingly (and adjusts call sites).
3d02498 to
b753c1c
Compare
853eeae to
1e8a9f1
Compare
b753c1c to
c07cab3
Compare
Fixes TAM-1052 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… render prop in graph
1e8a9f1 to
8f17df8
Compare
Summary
Adds
graphcompound component base primitives and styled wrapper.Base: Root, Chart, Title, Loading, ErrorBoundary + data validation
Styled: Composes base components with Tailwind styling
Fixes TAM-1052
Test Plan
npm run lint && npm run check-types && npm test🤖 Generated with Claude Code