Conversation
Implements a typed, LLM-friendly vertical/horizontal bar chart using ECharts with support for target/gradient ghost bars, custom color palette derived from baseColors tokens, and Figma-aligned styling (4px border radius on all corners, solid grid lines, circle legend icons, proper axis label typography).
…with filter bar and grid layout
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
🔍 Visual review for your branch is published 🔍Here are the links to: |
📦 Alpha Package Version PublishedUse Use |
There was a problem hiding this comment.
Pull request overview
Adds a new experimental analytics dashboard component to @factorialco/f0-react, backed by a new experimental ECharts-based chart component (F0DataChart) to render bar/line/funnel visualizations inside dashboard widgets.
Changes:
- Introduces
F0AnalyticsDashboard(config-driven dashboard with filter bar + responsive grid of chart/metric/collection items). - Introduces
F0DataChart(bar/line/funnel charts with shared theming, tooltip/axis builders, and skeletons). - Exposes
F0AnalyticsDashboardfrom the package component exports.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/components/exports.ts | Adds F0AnalyticsDashboard re-export from the components entrypoint. |
| packages/react/src/components/F0DataChart/utils/useEChartsInstance.ts | New hook to manage ECharts init/resize/cleanup. |
| packages/react/src/components/F0DataChart/utils/useContainerWidth.ts | New hook to track container width via ResizeObserver. |
| packages/react/src/components/F0DataChart/utils/useChartTheme.ts | New hook to resolve + reactively refresh chart theme (dark mode via DOM observation). |
| packages/react/src/components/F0DataChart/utils/theme.ts | Defines chart theme model + runtime resolver from CSS tokens. |
| packages/react/src/components/F0DataChart/utils/options.ts | Adds shared ECharts option builders (axes/grid/legend/tooltip/etc.). |
| packages/react/src/components/F0DataChart/utils/colors.ts | Adds token-constrained chart palette utilities and CSS color resolution. |
| packages/react/src/components/F0DataChart/types.ts | Defines the public discriminated-union props/types for F0DataChart. |
| packages/react/src/components/F0DataChart/skeletons.tsx | Adds skeleton UIs for bar/line/funnel charts. |
| packages/react/src/components/F0DataChart/index.ts | Experimental wrapper + public exports for chart types/utilities/skeletons. |
| packages/react/src/components/F0DataChart/components/LineChart/useLineChartOptions.ts | Maps typed line props to ECharts options using shared builders. |
| packages/react/src/components/F0DataChart/components/LineChart/LineChart.tsx | Renders the line chart container and binds ECharts instance/options hooks. |
| packages/react/src/components/F0DataChart/components/FunnelChart/useFunnelChartOptions.ts | Maps typed funnel props to ECharts options (labels, tooltip, layout). |
| packages/react/src/components/F0DataChart/components/FunnelChart/FunnelChart.tsx | Renders the funnel chart container and binds ECharts instance/options hooks. |
| packages/react/src/components/F0DataChart/components/BarChart/useBarChartOptions.ts | Maps typed bar props to ECharts options (including optional target “ghost bars”). |
| packages/react/src/components/F0DataChart/components/BarChart/BarChart.tsx | Renders the bar chart container and binds ECharts instance/options hooks. |
| packages/react/src/components/F0DataChart/stories/index.stories.tsx | Adds Storybook stories demonstrating chart variants. |
| packages/react/src/components/F0DataChart/stories/decorators.tsx | Adds a shared Storybook decorator for chart sizing. |
| packages/react/src/components/F0DataChart/F0DataChart.tsx | Implements the discriminated-union dispatcher for bar/line/funnel charts. |
| packages/react/src/components/F0AnalyticsDashboard/types.ts | Defines the dashboard config model (items, chart configs, fetchers). |
| packages/react/src/components/F0AnalyticsDashboard/index.ts | Experimental wrapper + public type exports for the dashboard. |
| packages/react/src/components/F0AnalyticsDashboard/hooks/useDashboardItemData.ts | Adds a generic async data hook for dashboard items (loading/error/stale response protection). |
| packages/react/src/components/F0AnalyticsDashboard/components/MetricItem/MetricItem.tsx | Implements metric widget rendering + formatting + trend computation. |
| packages/react/src/components/F0AnalyticsDashboard/components/FilterBar/FilterBar.tsx | Implements a dashboard-level filter bar via OneFilterPicker. |
| packages/react/src/components/F0AnalyticsDashboard/components/DashboardItem/DashboardItemSkeleton.tsx | Adds skeleton UI for metric items. |
| packages/react/src/components/F0AnalyticsDashboard/components/DashboardItem/DashboardItem.tsx | Adds a common widget “card” wrapper with loading/error handling. |
| packages/react/src/components/F0AnalyticsDashboard/components/DashboardGrid/DashboardGrid.tsx | Implements responsive grid + dynamic chart row heights via ResizeObserver. |
| packages/react/src/components/F0AnalyticsDashboard/components/CollectionItem/CollectionItem.tsx | Implements collection widget rendering via OneDataCollection and useDataCollectionSource. |
| packages/react/src/components/F0AnalyticsDashboard/components/ChartItem/ChartItem.tsx | Implements chart widget rendering (fetch → merge config+data → F0DataChart). |
| packages/react/src/components/F0AnalyticsDashboard/stories/mockData.ts | Adds mock filters/items/data fetchers for Storybook demos. |
| packages/react/src/components/F0AnalyticsDashboard/stories/index.stories.tsx | Adds Storybook stories demonstrating dashboard configurations. |
| packages/react/src/components/F0AnalyticsDashboard/F0AnalyticsDashboard.tsx | Implements the dashboard root component (filter state + grid rendering). |
| const meta = { | ||
| component: F0DataChart, | ||
| title: "F0DataChart", | ||
| tags: ["autodocs", "experimental"], | ||
| decorators: [ChartDecorator], | ||
| } satisfies Meta<typeof F0DataChart> | ||
|
|
||
| export default meta |
There was a problem hiding this comment.
This new Storybook file doesn’t include the required Chromatic Snapshot story (with withSnapshot({})). The f0-code-review checklist marks missing Snapshot stories as blocking for new components. Please add a Snapshot story that renders the key variants of F0DataChart in a compact layout.
| // Analytics | ||
| export * from "./F0AnalyticsDashboard" |
There was a problem hiding this comment.
F0DataChart is introduced in this PR but it isn’t exported from packages/react/src/components/exports.ts. Per packages/react conventions, new public components under components/ must be re-exported here so consumers can import them from the package entrypoint.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| createSource: (filters: FiltersState<Filters>) => any | ||
| /** | ||
| * Visualization configs for the collection (table, card, list, kanban). | ||
| * Same shape as OneDataCollection's `visualizations` prop. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| visualizations: ReadonlyArray<any> | ||
| } |
There was a problem hiding this comment.
This type introduces any for createSource and visualizations. packages/react/AGENTS.md explicitly disallows any in the React package (even with lint disables). Please type these using the existing OneDataCollection types (e.g. DataCollectionSourceDefinition for createSource and Visualization/OneDataCollectionProps["visualizations"]-compatible types) or use unknown + properly-typed generics instead of any.
| /** | ||
| * Show conversion percentages in labels. | ||
| * Each stage displays its value as a percentage of the first stage. | ||
| * @default true | ||
| */ | ||
| showConversion?: boolean | ||
| /** Format the value displayed in labels and tooltip */ | ||
| valueFormatter?: (value: number) => string |
There was a problem hiding this comment.
The JSDoc says showConversion defaults to true, but the funnel chart implementation defaults it to false (see useFunnelChartOptions destructuring). Please align the documented default with the actual behavior (either update the default value or fix the docs).
| const gridWidth = node.clientWidth | ||
| // On mobile (single column), columnWidth = full width | ||
| // On md+ (3 columns), columnWidth = (gridWidth - gaps) / cols | ||
| const isMd = window.matchMedia("(min-width: 768px)").matches |
There was a problem hiding this comment.
window.matchMedia isn’t available in some environments (e.g. unit tests with jsdom unless polyfilled). Calling it unguarded can crash rendering. Consider falling back to a width check (e.g. node.clientWidth) or guarding with typeof window !== "undefined" && typeof window.matchMedia === "function" and defaulting to false when unavailable.
| const isMd = window.matchMedia("(min-width: 768px)").matches | |
| const isMd = | |
| typeof window !== "undefined" && | |
| typeof window.matchMedia === "function" && | |
| window.matchMedia("(min-width: 768px)").matches |
| export const F0AnalyticsDashboard = experimentalComponent( | ||
| "F0AnalyticsDashboard", | ||
| _F0AnalyticsDashboard | ||
| ) |
There was a problem hiding this comment.
F0AnalyticsDashboard is a generic component (<Filters extends FiltersDefinition>). Wrapping it with experimentalComponent without preserving/casting its call signature can break the generic typing (or even fail the React.ComponentType constraint). Align this wrapper with patterns used in F0Form/index.tsx so the exported component retains its generic prop inference.
| ) | |
| ) as typeof _F0AnalyticsDashboard |
| // Re-fetch whenever filters change (deep comparison via JSON) | ||
| const filtersKey = JSON.stringify(filters) | ||
|
|
||
| useEffect(() => { | ||
| doFetch() | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [filtersKey, doFetch]) |
There was a problem hiding this comment.
When enabled is false, doFetch() always calls fetchData with {} but the effect still re-runs on every dashboard filter change because filtersKey is derived from filters (not from enabled ? filters : {}). This causes unnecessary re-fetches/loading states for items that opted out of dashboard filters. Consider basing filtersKey on the effective filters (or gating the effect) so it stays stable when enabled is false.
| return (params: unknown) => { | ||
| const p = params as { | ||
| marker?: string | ||
| name?: string | ||
| value?: number | ||
| } | ||
| const val = Number(p.value ?? 0) | ||
| const formattedValue = valueFormatter | ||
| ? valueFormatter(val) | ||
| : String(val) | ||
| const name = String(p.name ?? "") | ||
| const info = dataMap.get(name) | ||
|
|
||
| let conversionHtml = "" | ||
| if (showConversion && firstValue > 0 && info) { | ||
| const overallPct = formatPercent(val, firstValue) | ||
| conversionHtml = `<div style="margin-top: 4px; color: ${colors.foregroundTertiary}; font-size: 11px">Overall: ${overallPct}</div>` | ||
|
|
||
| // Step-over-step: compare with previous stage | ||
| const prevIndex = info.index - 1 | ||
| if (prevIndex >= 0) { | ||
| const prevData = series.data[prevIndex] | ||
| if (prevData && prevData.value > 0) { | ||
| const stepPct = formatPercent(val, prevData.value) | ||
| conversionHtml += `<div style="color: ${colors.foregroundTertiary}; font-size: 11px">From ${prevData.name}: ${stepPct}</div>` | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return `<div>${String(p.marker ?? "")} <strong>${name}</strong></div><div style="margin-top: 2px">${formattedValue}</div>${conversionHtml}` | ||
| } |
There was a problem hiding this comment.
The funnel tooltip formatter builds an HTML string using name and prevData.name directly. If stage names come from untrusted sources, this can allow HTML/script injection inside the tooltip. Please HTML-escape all interpolated values (or use a non-HTML tooltip render mode) before returning the tooltip content.
| gap = 4, | ||
| orient = "horizontal", | ||
| labelPosition = "outside", | ||
| showLegend = true, |
There was a problem hiding this comment.
showLegend defaults to true here, but F0DataChartFunnelProps documents showLegend as @default false and funnel charts typically don’t need a legend (labels already contain stage names). Please align the default value with the public prop docs (and with FunnelChartConfig’s documented default).
| showLegend = true, | |
| showLegend = false, |
| * Redesigned for readability: | ||
| * - Outside labels with no leader lines: stage name + value + conversion % | ||
| * - Left-aligned funnel so labels sit cleanly to the right | ||
| * - Rounded corners on funnel stages | ||
| * - Generous gap between stages | ||
| * - No legend by default (stage names are in the labels) | ||
| */ |
There was a problem hiding this comment.
The doc comment says outside labels have “no leader lines”, but the chart options set labelLine.show: isOutside with a non-zero length. Please update the comment or the config so they match (otherwise consumers will be misled about the funnel label styling).
…hart crash - Add F0ChatDashboard, F0ChatChart, F0ChatReportCard components - Add displayDashboard and displayChart copilot actions with canvas panel - Fix funnel chart crash when data arrives in bar/line series shape - Add defensive null check in useFunnelChartOptions - Update AiChatStateProvider to support canvas open/close state - Remove debug console.logs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 54 changed files in this pull request and generated 11 comments.
You can also share your feedback on Copilot code review. Take the survey.
| // Only auto-replace if the canvas is already open — otherwise | ||
| // the user must click "View report" to open it | ||
| if (visualizationMode === "canvas") { | ||
| openCanvas(config) | ||
| } |
There was a problem hiding this comment.
openCanvas(config) is called inside the Copilot action render callback. This triggers a React state update during render and can produce warnings ("Cannot update a component while rendering...") or re-render loops. Move this side effect into a component with a useEffect that runs when the finalized config/status is reached (or only trigger it from a user event).
| <OneEmptyState | ||
| variant="critical" | ||
| title="Error loading data" | ||
| description={error.message} | ||
| actions={[ | ||
| { | ||
| type: "default", | ||
| label: "Retry", | ||
| onClick: onRetry, | ||
| }, | ||
| ]} |
There was a problem hiding this comment.
The error UI hardcodes user-facing strings ("Error loading data", "Retry"). This package generally routes UI copy through useI18n(); please use translations here so dashboards are localizable and consistent with the rest of the design system.
| formatter: (params: unknown) => { | ||
| if (!Array.isArray(params)) return "" | ||
|
|
||
| const filtered = filterSeries | ||
| ? params.filter( | ||
| (p: { seriesName?: string }) => | ||
| !filterSeries(String(p.seriesName ?? "")) | ||
| ) | ||
| : params | ||
|
|
||
| if (filtered.length === 0) return "" | ||
|
|
||
| const header = `<div style="margin-bottom: 4px; font-weight: 500">${String(filtered[0].axisValueLabel ?? filtered[0].name ?? "")}</div>` | ||
| const items = filtered | ||
| .map((p: { marker?: string; seriesName?: string; value?: number }) => { | ||
| const formattedValue = valueFormatter | ||
| ? valueFormatter(Number(p.value)) | ||
| : String(p.value) | ||
| return `<div>${String(p.marker ?? "")} ${String(p.seriesName ?? "")} <strong>${formattedValue}</strong></div>` | ||
| }) | ||
| .join("") | ||
|
|
||
| return `${header}${items}` | ||
| }, |
There was a problem hiding this comment.
buildTooltip().formatter builds an HTML string using seriesName/axisValueLabel directly. If any of these values come from untrusted sources (LLM configs, backend data), this can enable HTML injection/XSS in the tooltip. Escape/sanitize dynamic strings before concatenating them into HTML (or use a non-HTML tooltip mode).
| let conversionHtml = "" | ||
| if (showConversion && firstValue > 0 && info) { | ||
| const overallPct = formatPercent(val, firstValue) | ||
| conversionHtml = `<div style="margin-top: 4px; color: ${colors.foregroundTertiary}; font-size: 11px">Overall: ${overallPct}</div>` | ||
|
|
||
| // Step-over-step: compare with previous stage | ||
| const prevIndex = info.index - 1 | ||
| if (prevIndex >= 0) { | ||
| const prevData = dataPoints[prevIndex] | ||
| if (prevData && prevData.value > 0) { | ||
| const stepPct = formatPercent(val, prevData.value) | ||
| conversionHtml += `<div style="color: ${colors.foregroundTertiary}; font-size: 11px">From ${prevData.name}: ${stepPct}</div>` | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return `<div>${String(p.marker ?? "")} <strong>${name}</strong></div><div style="margin-top: 2px">${formattedValue}</div>${conversionHtml}` | ||
| } |
There was a problem hiding this comment.
The funnel tooltip formatter returns HTML containing unsanitized name/prevData.name values. If stage names can come from untrusted input, this is an HTML injection/XSS risk in the tooltip DOM. Sanitize/escape these strings before interpolating them into HTML.
| /** | ||
| * Compact card shown inline in the AI chat to represent a generated | ||
| * dashboard report. Uses F0Card with an icon avatar, the dashboard | ||
| * title/description, an item summary in metadata, and a "View report" | ||
| * primary action that opens the canvas panel. | ||
| */ |
There was a problem hiding this comment.
The docstring says the card includes a "View report" primary action, but the component currently only wires onClick on the full card and doesn’t render an explicit action/button. Either add the action or adjust the comment so it matches the actual UI behavior.
| // Sort by total value across all series | ||
| if (computation.sortBy === "value") { | ||
| const totals = categories.map((_, i) => | ||
| seriesArray.reduce((sum, s) => sum + s.data[i], 0) | ||
| ) | ||
| const indices = categories.map((_, i) => i) | ||
| indices.sort((a, b) => | ||
| computation.sortOrder === "asc" | ||
| ? totals[a] - totals[b] | ||
| : totals[b] - totals[a] | ||
| ) | ||
| categories = indices.map((i) => categories[i]) | ||
| for (const s of seriesArray) { | ||
| s.data = indices.map((i) => s.data[i]) | ||
| } | ||
| } |
There was a problem hiding this comment.
When computation.series is set (multi-series charts), sortBy: "category" is never applied—only sortBy: "value" is handled. This makes the sortBy option behave inconsistently between single- and multi-series modes. Add category sorting for the multi-series path as well (mirroring the single-series branch).
| // Keep a ref to the latest config so re-clicking "View report" | ||
| // re-opens the same dashboard without needing a closure per render | ||
| const latestConfigRef = useRef<ChatDashboardConfig | null>(null) | ||
|
|
||
| useCopilotAction({ |
There was a problem hiding this comment.
latestConfigRef is written to but never read, so it’s dead code and will typically fail noUnusedLocals/linting. Either remove it or actually use it (e.g., as the source of truth for a stable click handler).
| <F0Button | ||
| variant="ghost" | ||
| icon={Cross} | ||
| size="md" | ||
| hideLabel | ||
| onClick={() => closeCanvas()} | ||
| label="Close dashboard" | ||
| /> |
There was a problem hiding this comment.
label="Close dashboard" is user-facing copy but is hardcoded here, while the rest of the AI chat UI uses useI18n() translations. Please move this (and any other canvas strings) into i18n keys for consistency/localization.
| // --------------------------------------------------------------------------- | ||
|
|
||
| const LIGHT_TOOLTIP: ChartThemeTooltip = { | ||
| // text size 14px, padding 8px eje x 6 en eje y |
There was a problem hiding this comment.
This comment is partially in Spanish ("eje") and is hard to understand for non-Spanish speakers. Consider rewriting it in English so the styling rationale is clear to the whole team.
| // text size 14px, padding 8px eje x 6 en eje y | |
| // font size 14px, padding 6px vertically and 8px horizontally |
| const { valueFormat: _vf, ...chartConfigRest } = | ||
| item.chart as unknown as Record<string, unknown> | ||
| const chart = { | ||
| ...chartConfigRest, | ||
| ...(valueFormatter ? { valueFormatter } : {}), | ||
| } | ||
|
|
||
| return { | ||
| id: item.id, | ||
| title: item.title, | ||
| description: item.description, | ||
| colSpan: item.colSpan, | ||
| type: "chart", | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| chart: chart as any, | ||
| fetchData: (filters: FiltersState<FiltersDefinition>) => { |
There was a problem hiding this comment.
chart: chart as any (and the upstream Record<string, unknown> casting) violates the repo's strict TypeScript policy and can hide real type mismatches between ChatDashboardChartConfig and DashboardChartConfig. Please map each chart config variant explicitly (bar/line/funnel) into a correctly-typed DashboardChartConfig shape so this stays type-safe without any.
Description
Screenshots (if applicable)
[Link to Figma Design](Figma URL here)
Implementation details