refactor: Consolidate charts with BarChart component#7223
Draft
talissoncosta wants to merge 18 commits intofeat/feature-analytics-6067from
Draft
refactor: Consolidate charts with BarChart component#7223talissoncosta wants to merge 18 commits intofeat/feature-analytics-6067from
talissoncosta wants to merge 18 commits intofeat/feature-analytics-6067from
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Zero custom CSS — tooltip uses Bootstrap utilities + semantic tokens. ChartTooltip extracted to own file for single responsibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Data utilities extracted to analyticsUtils.ts for testability. Closes #6067 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Chart tokens were sorted alphabetically (1, 10, 2, 3...) instead of numerically (1, 2, 3...10). Use localeCompare with numeric option. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Update featureAnalytics service queryFn to return rawEntries alongside chartData, so FeatureAnalytics doesn't need to call hooks in a loop - Remove useGetEnvironmentAnalyticsQuery calls inside .map() (React rules-of-hooks violation) - Rename analyticsUtils.ts to utils.ts - Add 14 unit tests for hasLabelledData, aggregateByLabels, buildEnvColorMap - Fix natural sort order in token generator Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add useChartColors() and useChartColorMap() hooks (common/hooks/) - Remove getCSSVars/CHART_COLOURS imports from utils.ts — colors are now passed as a parameter from the hook - Remove buildEnvColorMap — replaced by useChartColorMap hook - Update tests to pass mock colors directly (no jest.mock needed) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace getCSSVars/CHART_COLOURS with useChartColorMap hook - Replace inline color/fontSize/margin styles with Bootstrap utilities Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove common/utils/getCSSVar.ts — only had one consumer - Inline resolveColors() into useChartColors hook with JSDoc explaining why we read from DOM (Recharts needs hex strings, not var() refs) - Add comment on rawEntries in service queryFn explaining the dual return (chartData for environments, rawEntries for labels) - Add comment on label priority in utils.ts - Regenerate tokens.ts with updated comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Create web/styles/3rdParty/_recharts.scss as proper home for recharts global styles (alongside _react-select.scss, _react-datepicker.scss) - Switch axis tick + line colours to var(--color-text-secondary) via recharts' built-in classNames — dark mode handled automatically, no hex strings threaded through props - Drop hardcoded '#656D7B' from BarChart.tsx (4 axis instances + 1 bar fallback) - Delete dead .xAxis/.yAxis rules from _tooltips.scss (no consumers) - TODO on legacy .recharts-tooltip + .dark block — removes when SingleSDKLabelsChart and OrganisationUsage migrate to <BarChart /> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tColors Generator now emits flat camelCase constants for every semantic token (e.g. colorChart1, colorTextSecondary, radiusMd) as CSS value strings — 'var(--token, #hex-fallback)'. Pass directly to recharts props, inline styles, or anywhere a CSS value is accepted; var() resolves at render and theme toggle updates colours via the CSS cascade. Charts: - BarChart uses colorTextSecondary directly for axis tick + line — no CSS classname plumbing in _recharts.scss - buildChartColorMap pure function (web/components/charts/) replaces useChartColors / useChartColorMap hooks — no DOM read, no theme-toggle staleness bug - _recharts.scss reduced to legacy rules + TODO for the two remaining raw-recharts consumers (SingleSDKLabelsChart, OrganisationUsage) Cleanup: - Delete common/hooks/useChartColors.ts (both hooks) - Delete common/theme/index.ts barrel — codebase convention is direct file imports - Drop nested tokens object + 4 unused type exports from tokens.ts; update the one MDX docs example to use a flat constant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix inverted pluralisation in empty-state message ("environment"
takes an 's' when there are multiple selected, not the other way
around)
- Type ChartTooltip against recharts' Payload<ValueType, NameType>
instead of `el: any` — use `el.color` (typed) in place of `el.fill`
(untyped), add numericValue helper to keep the reduce typed
- Add direct unit tests for buildChartColorMap (basic mapping,
wraparound, empty array) — previously covered only transitively
via aggregateByLabels tests
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ull labels
Three bugs caused labelled analytics to look wrong in real data:
1. hasLabelledData was too permissive — passed `{ user_agent: null }`
as "labelled" because the key exists. Every entry then fell through
to the 'Unknown' fallback in aggregation, collapsing all data into
a single series. Tightened to require at least one entry with a
non-empty `user_agent` or `client_application_name` value.
2. chartData from aggregateByLabels had no date ordering guarantee —
dates appeared in whatever order the raw entries landed (visible
in prod as `2026-04-03` rendered AFTER `2026-04-12`). Fixed by
having aggregateByLabels seed its output from a caller-supplied
day axis (chronologically pre-built by useFeatureAnalytics for
the env-path chart).
3. Labelled chart only included days with events, so the x-axis was
sparse — whereas the env path pre-builds all 30 days. Fixed by
reusing the env-path day axis, giving both paths the same complete
date range and the same 'Do MMM' display format.
hasData check updated — labelledChartData now always has one bucket
per day (including empty ones), so `.length > 0` no longer indicates
presence of data. Switched to "any day has a non-zero count for a
label series".
Tests: 20 passing (17 utils + 3 buildChartColorMap), includes new
cases for null-label-values, caller-provided day order, and out-of-
range entries being dropped.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The new BarChart introduced two regressions for the env-grouped path:
1. Legend showed raw env IDs (e.g. "22", "1848") because the new
BarChart unconditionally renders recharts' <Legend /> using each
series' dataKey. The env tags at the top already serve as a
colour legend, so a second one was redundant and broken.
- Add `showLegend?: boolean` to BarChart (default false)
- FeatureAnalytics only enables it for the labelled path (where
the filter above the chart doesn't carry colour per series)
- Stories keep `showLegend` on (no filter UI above them)
2. Bar colours didn't match the env tag chip colours. The new code
used `buildChartColorMap(environmentIds)` — which indexes into
our chart palette by selection order. Old prod used
`Utils.getTagColour(indexInProjectEnvList)` — the same function
that colours the env chip, indexed by the env's position in the
project's env list. Restored that behaviour:
- Fetch envs via `useGetEnvironmentsQuery`
- Sort selected envs by their project-list position (stable)
- Build envColorMap with `Color(Utils.getTagColour(idx)).alpha(0.75)`
- Pass sortedEnvIds as the chart series
Labelled path keeps the new `CHART_COLOURS` palette (intentional — SDK
names aren't in the tag-colour palette, and this is a net-new feature).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two remaining Feature Analytics tooltip regressions: 1. Raw dataKeys surfaced in the tooltip — env mode showed "22: 0" / "1848: 0" instead of "Production: 0" / "Staging: 0". Added a `seriesLabels?: Record<string, string>` prop to BarChart, threaded through to ChartTooltip (and recharts' <Legend formatter> when the optional legend is enabled). FeatureAnalytics builds an env-id → env-name map from useGetEnvironmentsQuery and passes it for the env path; labelled path leaves it undefined (dataKeys are already SDK names). 2. Tooltip label text used `text-secondary` which in the theme's tooltip-on-surface context rendered with poor contrast (muted yellow-ish on white). Switched label + value to `text-default` with a semibold weight on the value — keeps the label/value hierarchy while matching the tooltip header's contrast. Incidental type fixes (both introduced earlier this session): - ChartTooltip.formatNumber now accepts recharts' ValueType (was string | number | undefined — ValueType is broader and can be an array) - FeatureAnalytics passes `projectId: Number(projectId)` to useGetEnvironmentsQuery — the Req type expects number, old code was silently typechecking as string against a loose upstream signature The pre-existing `any` cluster in useFeatureAnalytics.ts is unchanged (out of scope — separate follow-up). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…to Record
Three targeted cleanups after auditing the PR for overengineering:
1. Extract useEnvChartProps({ projectId, environmentIds }) — returns
{ series, colorMap, seriesLabels } for env-grouped charts. Bundles
the env-list query, project-position-based sorting, tag-colour
mapping (Utils.getTagColour + alpha), and env-name lookup into one
hook. Real reuse ahead: the planned legacy chart migration
(SingleSDKLabelsChart, OrganisationUsage) needs the same env
colouring to match their tag chips — this hook is the single
source of that logic. FeatureAnalytics.tsx drops ~30 lines of
derivation in the process.
2. Remove BarChart's `stacked` and `height` props. Both defaulted
sensibly (stacked=true, height=400) and no consumer ever overrode
them (grep'd every <BarChart /> callsite, including the legacy
chart migration targets). YAGNI — cheaper to add them back later
than to carry unused API surface now.
3. Switch every `colorMap: Map<string, string>` to
`Record<string, string>` — BarChart, MultiSelect, aggregateByLabels,
buildChartColorMap, plus all tests and stories. Previous mix of
Map (for colours) and Record (for seriesLabels) wasn't principled,
just historical. Record is more idiomatic React, simpler to
inspect in devtools, and callsites are slightly cleaner (`m[k]`
vs `m.get(k)`). Tree-shakeable, no behavioural change.
Labelled-mode derivations (aggregateByLabels call, labelOptions,
filteredLabels) stay inline as component-local useMemos — extracting
them into a second hook would be packaging for packaging's sake, with
no reuse today.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rts SCSS Replaces the two raw-recharts consumers with the shared <BarChart /> component introduced in #7215. Both pages keep their behaviour and visual contract; the JSX collapses substantially. BarChart API additions to support these consumers: - `barSize?: number` — fixed bar width in pixels (OrganisationUsage uses 14px to fit four series per day comfortably) - `verticalGrid?: boolean` (default true) — toggles CartesianGrid's vertical lines (legacy charts hide them) Migrations: - OrganisationUsage.container.tsx (4 metric series stacked, custom display labels via seriesLabels, selection-driven series filter, pre-formatted day axis) - SingleSDKLabelsChart.tsx (per-SDK stacked, palette colour map passed in from parent, MultiSelect-driven SDK filter) - OrganisationUsageMetrics.container.tsx — pre-formats day to 'D MMM' (matches the new chart-side day-as-display-string contract), switches userAgentColorMap from Map to Record (consistent with BarChart's prop after #7215) Cleanup: - Delete web/styles/3rdParty/_recharts.scss entirely — its rules existed solely to style the two legacy charts' tooltips. With both consumers migrated, no consumer of recharts global classNames remains; the new BarChart's ChartTooltip uses Bootstrap utilities + semantic token classes directly. Drop the @import too. - SingleSDKLabelsChart loses unused props (`metricKey`, `colours`) that the migration made redundant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
bot
added
front-end
4 tasks
talissoncosta
force-pushed
the
feat/feature-analytics-6067
branch
from
8483792 to
45ee267
Compare
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.
docs/if required so people know about the feature.Changes
Follow-up to #7215 — migrates the two raw-recharts consumers in the org-level Usage page to the shared
<BarChart />component, retiring the legacy recharts SCSS overrides.What's migrated
OrganisationUsage(chart section)web/components/organisation-settings/usage/OrganisationUsage.container.tsxenvironment_document→ "Environment Document"), selection-driven series, pre-formatted day axisSingleSDKLabelsChartweb/components/organisation-settings/usage/components/SingleSDKLabelsChart.tsxOrganisationUsageMetrics(parent)web/components/organisation-settings/usage/OrganisationUsageMetrics.container.tsxdayto'D MMM'(matches BarChart's day-as-display-string contract);userAgentColorMapswitched fromMap→Record<string, string>for consistency with the BarChart propBarChartAPI additionsBoth consumers needed two props the existing
BarChartdidn't expose:barSize?: number— fixed bar width in pixels (OrganisationUsageuses 14px to fit four series per day comfortably)verticalGrid?: boolean(defaulttrue) — togglesCartesianGrid's vertical lines (legacy charts hide them)seriesLabels(added in #7215) handles the display-name mapping forOrganisationUsage(environment_document→ "Environment Document").Cleanup
web/styles/3rdParty/_recharts.scssentirely — its rules existed solely to style the two legacy charts' tooltips. With both consumers migrated, no recharts global-className consumer remains; the newBarChart'sChartTooltipuses Bootstrap utilities + semantic token classes directly. Drop the@importtoo.SingleSDKLabelsChartloses unused props (metricKey,colours) that the migration made redundant.Net diff
+101 / -312— six files changed, one deleted.Type strictness
OrganisationUsage'scolorMapandseriesLabelsare typed againstRecord<MetricDataKey, string>whereMetricDataKeyis derived from theMETRICSarray ((typeof METRICS)[number]['dataKey']). Adding a new metric toMETRICSforces both maps to be updated — TS errors on missing keys.How did you test this code?
In the app
ENV=local npm run devsdk_usage_chartsflag enabled) → per-SDK stack renders, MultiSelect filter narrows visible series, tooltip shows SDK namesAutomated
npx eslint— 0 errors on touched filesnpm run typecheck— 0 new errors (pre-existinganycluster inuseFeatureAnalytics.tsunchanged; pre-existing errors inOrganisationUsageSideBar/OrganisationUsagePageare unrelated)🤖 Generated with Claude Code