feat(insights): show legend on lifecycle chart#60459
Merged
Merged
Conversation
Contributor
|
Size Change: +7.22 kB (+0.01%) Total Size: 80.8 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/product_analytics/frontend/insights/trends/TrendsLifecycleChart/TrendsLifecycleChart.tsx:195-200
`legendItemsFromSeries(series, theme)` is called inline in the render, creating a new array reference on every render even when `showLegend` is `false`. Both `series` (already `useMemo`'d) and `theme` (`useMemo(() => buildTheme(), [])`) are stable references, so the work is avoidable — and the resulting array is discarded immediately when `ChartLegend` short-circuits on `show=false`.
```suggestion
<ChartLegend
show={!!showLegend}
items={useMemo(() => legendItemsFromSeries(series, theme), [series, theme])}
position="top"
legendDataAttr="trend-lifecycle-legend"
>
```
Reviews (1): Last reviewed commit: "chore(hog-charts): address ChartLegend r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
195
to
200
| <ChartLegend | ||
| show={!!showLegend} | ||
| items={legendItemsFromSeries(series, theme)} | ||
| position="top" | ||
| legendDataAttr="trend-lifecycle-legend" | ||
| > |
Contributor
There was a problem hiding this comment.
legendItemsFromSeries(series, theme) is called inline in the render, creating a new array reference on every render even when showLegend is false. Both series (already useMemo'd) and theme (useMemo(() => buildTheme(), [])) are stable references, so the work is avoidable — and the resulting array is discarded immediately when ChartLegend short-circuits on show=false.
Suggested change
| <ChartLegend | |
| show={!!showLegend} | |
| items={legendItemsFromSeries(series, theme)} | |
| position="top" | |
| legendDataAttr="trend-lifecycle-legend" | |
| > | |
| <ChartLegend | |
| show={!!showLegend} | |
| items={useMemo(() => legendItemsFromSeries(series, theme), [series, theme])} | |
| position="top" | |
| legendDataAttr="trend-lifecycle-legend" | |
| > |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/product_analytics/frontend/insights/trends/TrendsLifecycleChart/TrendsLifecycleChart.tsx
Line: 195-200
Comment:
`legendItemsFromSeries(series, theme)` is called inline in the render, creating a new array reference on every render even when `showLegend` is `false`. Both `series` (already `useMemo`'d) and `theme` (`useMemo(() => buildTheme(), [])`) are stable references, so the work is avoidable — and the resulting array is discarded immediately when `ChartLegend` short-circuits on `show=false`.
```suggestion
<ChartLegend
show={!!showLegend}
items={useMemo(() => legendItemsFromSeries(series, theme), [series, theme])}
position="top"
legendDataAttr="trend-lifecycle-legend"
>
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
There was a problem hiding this comment.
Fixed in c52e050 — memoized as a legendItems variable so the dependency closure is explicit and the JSX stays tidy.
sampennington
commented
May 28, 2026
Contributor
Author
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Ship it
One performance nit worth picking up pre-merge — legendItemsFromSeries(series, theme) is called inline on every render in TrendsLifecycleChart.tsx:197 (flagged by code-reviewer, react, hog-charts, and efficiency). A one-line useMemo keeps the items reference stable. Everything else is fast-follow.
Non-anchored findings
- Story coverage Add an
UnstackedWithLegendstory so the layout fix (self-stretch+ nestedflex flex-col) is locked in across both stacked and unstacked variants. [code-reviewer + hog-charts] - Test hardening Consider asserting the legend swatch order matches the rendered series order (
New, Returning, Resurrecting, Dormant) — the lifecycle transform sorts by status and a regression would silently desync legend from bars. [hog-charts + code-reviewer]
Reviewers: code-reviewer, react, hog-charts, personal, reuse, quality, efficiency. Inline comments above are tagged with the reviewer that raised them and the level of concern.
| onError={handleChartError} | ||
| <ChartLegend | ||
| show={!!showLegend} | ||
| items={legendItemsFromSeries(series, theme)} |
Contributor
Author
There was a problem hiding this comment.
🤖 [code-reviewer+react+hog-charts+efficiency] · Performance
legendItemsFromSeries(series, theme) runs inline on every render and returns a fresh LegendItem[], so <Legend> reconciles even when nothing changed. series (line 74) and theme (line 47) are already memoized, so the fix is one line — extract above the JSX:
const legendItems = useMemo(() => legendItemsFromSeries(series, theme), [series, theme])Cheap at 4 series but the pattern reads as "forgot the useMemo" — worth fixing pre-merge so it doesn't get cargo-culted by the next adapter.
| dataAttr="trend-lifecycle-graph" | ||
| onError={handleChartError} | ||
| <ChartLegend | ||
| show={!!showLegend} |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality] · Nit
!!showLegend works (selector is boolean | null | undefined) but showLegend ?? false reads more directly for a nullable boolean. Either is fine — flagging for consistency.
Contributor
Author
There was a problem hiding this comment.
Won't fix — !! and ?? false both correctly coerce boolean | null | undefined to boolean. The legacy lifecycle adapter also uses !!showLegend (ActionsLineGraph.tsx:74), and !! is the more common idiom in this codebase for nullable-bool gating. Keeping for consistency.
| legendDataAttr, | ||
| children, | ||
| }: ChartLegendProps): React.ReactElement { | ||
| if (!show || items.length === 0) { |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality] · Nit
if (!show || items.length === 0) conflates two distinct cases (consumer opted out vs. nothing to show). Either split into two early returns, or name it:
const hasLegend = show && items.length > 0
if (!hasLegend) {
return <>{children}</>
}
Contributor
Author
There was a problem hiding this comment.
Won't fix — the early return reads as "there's nothing to render in legend form", which is one concept with two sources. Naming it hasLegend adds a line without making the intent clearer. Legend itself does the same items.length === 0 → null check internally; this wrapper just hoists it so we don't render the layout wrapper either. Keeping.
| if (!show || items.length === 0) { | ||
| return <>{children}</> | ||
| } | ||
| const orientation = position === 'left' || position === 'right' ? 'vertical' : 'horizontal' |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality] · Nit
position === 'left' || position === 'right' ? 'vertical' : 'horizontal' duplicates the isRow predicate already inside ChartLegendLayout. Tiny — extract a positionToOrientation(position) helper if it spreads to a third consumer.
Contributor
Author
There was a problem hiding this comment.
Won't fix — one duplication isn't enough to justify a helper, and ChartLegendLayout.isRow is a local const not exported. Will extract when a third consumer needs it.
| } | ||
| const orientation = position === 'left' || position === 'right' ? 'vertical' : 'horizontal' | ||
| // Bakes `flex-1 min-h-0` so consumers in a flex-col parent don't have to remember it. | ||
| const wrapperClassName = `flex-1 min-h-0 ${className ?? ''}`.trim() |
Contributor
Author
There was a problem hiding this comment.
🤖 [reuse+quality] · Suggestion
`flex-1 min-h-0 ${className ?? ''}`.trim() reinvents cn(). The repo already has cn in frontend/src/lib/utils/css-classes.ts (clsx + tailwind-merge) which handles the ?? '' and (more importantly) merges conflicting tailwind utilities if a future consumer passes flex-none:
const wrapperClassName = cn('flex-1 min-h-0', className)Sibling files (ChartLegendLayout.tsx, Legend.tsx) use the same raw-template pattern — so this is a pre-existing house style inside lib/hog-charts/, not just a new sin. If the dir deliberately avoids cn, ignore this; if not, adopting it here is a good entry point.
Contributor
Author
There was a problem hiding this comment.
Won't fix — lib/hog-charts/ is a self-contained pure zone (no imports from lib/utils, scenes/, kea, or PostHog-specific code) so the library stays portable and easy to extract. Sibling files (ChartLegendLayout.tsx, Legend.tsx) use the same raw template-literal pattern deliberately. Adopting cn here would be the first crack in that wall. If we want cn-style merging inside hog-charts, the right move is to inline a small clsx equivalent in the lib's utils/, not import from lib/utils.
| hiddenKeys?: string[] | ||
| className?: string | ||
| /** data-attr on the inner `<Legend>`. The outer layout wrapper has no data-attr. */ | ||
| legendDataAttr?: string |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts+quality] · Suggestion
legendDataAttr lands on the inner <Legend>, but ChartLegendLayout.dataAttr lands on the outer wrapper — so this component has no way to tag the outer block. Symmetry would help: forward an optional dataAttr (or wrapperDataAttr) to ChartLegendLayout too, so consumers can anchor on either layer.
Contributor
Author
There was a problem hiding this comment.
Deferring — no consumer needs the outer-wrapper anchor today, and adding both legendDataAttr and wrapperDataAttr makes the API surface lumpy. Will add when a real test/inspection case shows up.
| <div data-attr="chart">C</div> | ||
| </ChartLegend> | ||
| ) | ||
| // No wrapper div — chart is the only top-level child. |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal] · Nit
// No wrapper div — chart is the only top-level child. narrates the next assertion (expect(container.children).toHaveLength(1)). Drop.
| expect(container.textContent).toContain('B') | ||
| expect(container.querySelector('[data-attr="chart"]')).not.toBeNull() | ||
| expect(container.querySelector('[data-attr="my-legend"]')).not.toBeNull() | ||
| // Outer wrapper carries the height-fill class so the chart inherits parent height. |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal] · Nit
// Outer wrapper carries the height-fill class so the chart inherits parent height. restates what the toContain('flex-1') / toContain('min-h-0') assertions already check. Drop.
|
|
||
| await screen.findByTestId('trend-lifecycle-graph') | ||
| const legend = await screen.findByTestId('trend-lifecycle-legend') | ||
| // Lifecycle status names appear capitalized in the legend. |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal+code-reviewer] · Nit
Comment narrates the next four toMatch calls. Drop the comment, and consider folding the four assertions into a single it.each(['New', 'Returning', 'Resurrecting', 'Dormant'])(...) per repo's parameterized-test convention — would also give individual failures on regression.
Contributor
Author
There was a problem hiding this comment.
Fixed in c52e050 — went further than parameterizing. Replaced the four presence assertions with a single expect(labels).toEqual(['Dormant', 'Returning', 'Resurrecting', 'New']) so the test now also locks in the legend ↔ rendered-series order (which the hog-charts reviewer also wanted).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Purely additive frontend UI change that wires an existing showLegend filter flag to a new ChartLegend wrapper component. No backend, data model, API, or security impact. All review comments are either marked addressed or are author won't-fix calls with reasonable justifications.
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
Contributor
There was a problem hiding this comment.
Pure frontend feature addition wiring an existing filter flag to a new well-tested wrapper component. No backend, API, data model, or security impact. All review comments were addressed or intentionally won't-fixed with clear rationale. The ChartLegendLayout export replacement has no external consumers.
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
Contributor
There was a problem hiding this comment.
Pure frontend feature wiring an existing filter flag to a new well-tested ChartLegend wrapper component. The ChartLegendLayout → ChartLegend public export swap has no external consumers. All review comments are resolved or have clear won't-fix rationale.
gesh
approved these changes
Jun 1, 2026
Wires the hog-charts Legend into TrendsLifecycleChart so toggling "Show legend" in the editor renders status swatches (New / Returning / Resurrecting / Dormant) above the chart, matching the legacy Chart.js behaviour. Also fixes ChartLegendLayout so the chart slot fills both axes inside the flex wrapper — it's now a flex container itself (so a nested chart's `flex: 1` resolves) and uses `self-stretch` so the chart fills the cross axis regardless of the `align` prop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps ChartLegendLayout + Legend into a single component that takes `show` and `items` props, hiding the layout/orientation plumbing from adapters. When `show=false` or `items` is empty it returns children unwrapped, so toggling the legend off doesn't introduce an extra flex container. Bakes the parent-fill classes (`flex-1 min-h-0`) into the wrapper so adapters in a flex-col parent don't need to remember them. Refactors TrendsLifecycleChart to use ChartLegend — drops the manual `if (!showLegend) return chart` branch and the intermediate `chart` variable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Rename ChartLegend.dataAttr → legendDataAttr — the prop lands on the inner <Legend>, not the wrapper, so the old name was a footgun for the next consumer who'd expect it to mirror ChartLegendLayout.dataAttr. - Trim multi-line comments to single lines per repo style. - Strengthen the "renders children unwrapped" tests to assert no stray wrapper div sneaks in (container.children.length === 1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Memoize legendItemsFromSeries in TrendsLifecycleChart so the items reference stays stable across renders (flagged by Greptile and 4 of the /r reviewers). - Strengthen the legend-present test into an order assertion so the legend stays in sync with the rendered series sort (dormant → returning → resurrecting → new). - Add UnstackedWithLegend story so the layout fix is locked in across both stacked and unstacked variants. - Drop three narrative comments that just restated the next line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops the index.ts export of ChartLegendLayout — ChartLegend is now the only public way to add a legend to a chart. The Layout* stories in Legend.stories.tsx switched to ChartLegend too; the old ChartWithLegend helper used ChartLegendLayout directly and skipped the `flex-1 min-h-0` baking, so its chart slot collapsed when position=left/right. ChartLegendLayout.tsx and its test stay in place — they're still used internally by ChartLegend. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
15 updated Run: 37a7600a-688c-4820-80bb-ff1e2028176c Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
sampennington
force-pushed
the
sam/lifecycle-chart-legend
branch
from
62744dc to
de46cdf
Compare
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
Contributor
There was a problem hiding this comment.
Pure frontend UI addition — no backend, data model, API, or security impact. The ChartLegendLayout public export replacement has no external consumers, legendItems is properly memoized, and the new component is well-tested. All review comments are resolved or have clear won't-fix rationale, and the PR has a human approval.
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
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.
Problem
The new hog-charts based lifecycle chart (behind
PRODUCT_ANALYTICS_HOG_CHARTS_LIFECYCLE) didn't render a legend, even when the user enabled "Show legend" in the editor. The legacy Chart.js version rendered the standard top legend with shortened status labels — this PR restores that on the hog-charts path.Changes
TrendsLifecycleChartreadsshowLegendfromtrendsDataLogicand wraps the chart in a new<ChartLegend>withNew / Returning / Resurrecting / Dormantswatches above the bars when on. Status order, colors and labels match the bars (useslegendItemsFromSeries).<ChartLegend show items position>convenience inlib/hog-charts— wrapsChartLegendLayout+Legend, returns children unwrapped whenshow=falseoritems=[], and bakes the parent-fill flex classes in so adapters in a flex-col parent don't have to.ChartLegendLayoutchart slot is now a flex container (flex flex-col) withself-stretchso the inner chart'sflex: 1resolves and the chart fills the cross axis regardless of thealignprop — required to make the first real consumer (lifecycle) work in.TrendsInsight. Existing positions (top/bottom/left/right) still pass their tests.How did you test this code?
I'm an agent. Manually verified the layout in the running dev server (chart fills the slot under the legend; toggling "Show legend" off restores the original full-height chart with no wrapper). Added automated coverage:
TrendsLifecycleChart.test.tsx— new cases forshowLegend=true(legend rendered with all four status labels) and unset (no legend in DOM).ChartLegend.test.tsx— show=false / empty items / show=true / all four positions.Did not run a project-wide type check (OOMs locally); CI will pick that up.
Publish to changelog?
🤖 Agent context
Claude Code (Opus 4.7, 1M ctx). Initial pass put the legend on lifecycle and only fixed the
ChartLegendLayoutchart slot to stretch on the cross axis — the chart then collapsed to zero height because the slot wasdisplay: blockand the inner chart'sflex: 1had no flex parent to resolve against. Second fix made the slot aflex flex-colcontainer too. After that the call site still had the awkwardif (!showLegend) return chart; else wrap in ChartLegendLayoutshape, so the second commit extracted<ChartLegend show items position>as a high-level wrapper that hides the layout/orientation plumbing and the parent-fill classes from adapters.Considered alternatives: a
legendprop directly on each chart (rejected — couples legend rendering into every chart wrapper) and inspecting children to derive items from the chart's series (rejected — fragile). Settled on an explicititemsprop so consumers control how items are derived/filtered, and the caller can reuselegendItemsFromSeries(series, theme)or build a custom list.Code-reviewer agent flagged a
dataAttrnaming footgun (the prop landed on the inner<Legend>whileChartLegendLayout.dataAttrlands on the outer wrapper) — renamed tolegendDataAttrin the third commit before any second consumer adopts it. Also trimmed multi-line comments and tightened the "renders children unwrapped" tests to assert exactly one top-level child.