feat: Feature Analytics label grouping (#6067)#7215
Open
talissoncosta wants to merge 18 commits intomainfrom
Open
feat: Feature Analytics label grouping (#6067)#7215talissoncosta wants to merge 18 commits intomainfrom
talissoncosta wants to merge 18 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
bot
added
front-end
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
…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>
github-actions
bot
added
feature
Zaimwa9
requested changes
Apr 13, 2026
Contributor
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Looks good overall but couple of comments:
- The one i'd like being fixed if possible is to use a Map for the chartData, that might reduce computation complexity
- NIT on a couple of single character variables that could be made a bit more explicit
Another one is that while testing, I realized we are not sending the labels. I'm looking into why but it looks like it's coming from the edge-api that is not tracking the labels for POST analytics/flags while it does for the usage. I'm looking into it
Comment on lines
+70
to
+76
| dayEntry[environment_id] = entry.count // Set count for specific environment ID | ||
| dayEntry[environment_id] = entry.count |
Contributor
There was a problem hiding this comment.
Can we move this to the same pattern as in aggregateByLabels and use a Map<string, ChartDataPoint for larger payloads ?
Contributor
Author
There was a problem hiding this comment.
Good catch — switched to a day → bucket Map built alongside preBuiltData, so the per-entry lookup is O(1) instead of the O(days × entries) Array.find dance. Commit 8483792
|
|
||
| type SeriesPayload = Payload<ValueType, NameType> | ||
|
|
||
| const formatNumber = (n: number | string | undefined): string => |
Contributor
There was a problem hiding this comment.
NIT: can we use variable names a bit more readable than single char please. Here and in a couple of places
Contributor
Author
There was a problem hiding this comment.
Renamed across the touched files in 8483792: n/v → value, el → entry in ChartTooltip; v → value in BarChart tickFormatter; v → response in useFeatureAnalytics; e → env (plus a small helper dedupe) in useEnvChartProps; d → day in utils. Kept i where it was a standard forEach index.
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>
github-actions
bot
added
feature
…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>
github-actions
bot
added
feature
talissoncosta
added a commit
that referenced
this pull request
Apr 13, 2026
…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>
talissoncosta
added a commit
that referenced
this pull request
Apr 13, 2026
…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>
This was referenced Apr 13, 2026
Two PR review comments from @Zaimwa9: 1. useFeatureAnalytics queryFn used Array.find() per entry to locate each entry's day bucket in preBuiltData — O(days × entries). Switch to a `day → bucket` Map, same pattern aggregateByLabels uses. O(1) lookups, matters for long periods / high entry counts. 2. Rename single-character callback vars across the PR's touched files: - ChartTooltip: `n`/`v` → `value`, `el` → `entry` - BarChart: `v` → `value` in the tick formatter - useFeatureAnalytics: `v` → `response`, `i` kept (idiomatic index) - useEnvChartProps: `e` → `env`; dedupe the findIndex call into a local helper, rename `a,b` to `idA,idB` for clarity - utils: `d` → `day` in the days.map Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
feature
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
2 participants
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
Closes #6067
Feature Analytics label grouping
When the API returns labelled evaluation buckets (per-SDK, via
user_agent/client_application_name), the chart now stacks bars by label value with matching colours and exposes aMultiSelectfilter to narrow visible SDKs. When no usable labels are present, the chart falls back to the existing environment-grouped view.How to review this PR
The diff is grouped by theme. Suggested reading order:
1. The feature itself
web/components/feature-page/FeatureNavTab/FeatureAnalytics.tsx— the consumerweb/components/feature-page/FeatureNavTab/utils.ts—hasLabelledData,aggregateByLabelsweb/components/feature-page/FeatureNavTab/useEnvChartProps.ts— env-grouped chart config (colour + label + stable ordering), reusable for the legacy chart migration (see follow-up PR refactor: Consolidate charts with BarChart component #7223)common/services/useFeatureAnalytics.ts— now returns{ chartData, rawEntries }(split so labelled-mode can group raw entries without a rules-of-hooks violation)common/types/responses.ts—environmentAnalytics.labelsadded2. Shared chart infra (new)
web/components/charts/BarChart.tsx— reusable stacked bar chart wrapping recharts. SupportsseriesLabels(dataKey → display name), optionalshowLegend, and colour tokens passed as prop values directlyweb/components/charts/ChartTooltip.tsx— typed against recharts'Payload<ValueType, NameType>, uses Bootstrap utilities + semantic token classesweb/components/charts/buildChartColorMap.ts— pure helper for building a label → palette-colourRecord<string, string>, no hook / DOM readweb/components/ColorSwatch.tsx— reusable colour indicator (sm 8px / md 12px / lg 16px)3. Flat token constants pattern
scripts/generate-tokens.mjs+ regeneratedcommon/theme/tokens.ts— emits 74 flat camelCase constants (e.g.colorChart1,colorTextSecondary,radiusMd) asvar(--token, #hex)CSS value strings. Pass directly to recharts props, inline styles, anywhere a CSS value is accepted. Browsers resolvevar()in SVG presentation attributes and in dark mode via the CSS cascade — no hook, no DOM read, no theme-toggle staleness--color-chart-1through--color-chart-10with light/dark variantsweb/styles/3rdParty/_recharts.scss— new partial with legacy tooltip rules; deleted entirely in the follow-up PR once the two remaining raw-recharts consumers migrate4. Storybook stories
WithLabelledBuckets(interactive filter),WithoutLabels,SingleSeriesKey design decisions
var()in SVG presentation attributes works in modern browsers (Chrome 85+, Firefox 80+, Safari 14+), so we no longer need auseChartColorshook readinggetComputedStyle. Passingvar(--color-chart-1, #0aaddf)directly to<Bar fill={...}>lets the browser resolve the variable at render, and CSS cascade updates it live on theme toggle (fixes an implicit bug where the old hook memoised hex values at mount).colorMapasRecord<string, string>everywhere (BarChart, MultiSelect, aggregateByLabels, buildChartColorMap). Previously a mix ofMapandRecord— unified.Utils.getTagColour(indexInProjectEnvList)so bar colours match the env chip colours 1:1; labelled mode usesCHART_COLOURS.seriesLabels?.[dataKey] ?? dataKeyso numeric env ids (22, 1848, …) surface as "Production", "Staging", etc.Follow-ups (separate PRs)
OrganisationUsage,SingleSDKLabelsChart) intoBarChart, drop the legacy_recharts.scsspartialanycluster incommon/services/useFeatureAnalytics.ts(pre-existing, flagged but out of scope here)How did you test this code?
In Storybook
npm run storybookIn the app
ENV=local npm run devAutomated
npx eslint— 0 errors on touched filesnpm run typecheck— 0 new errors (pre-existinganycluster inuseFeatureAnalytics.tsis unchanged)npm run test:unit— 20 passing (17utils, 3buildChartColorMap)🤖 Generated with Claude Code