feat(insights): show value labels on hog-charts lifecycle insight#60131
Merged
Conversation
Contributor
|
Reviews (1): Last reviewed commit: "feat(insights): show value labels on hog..." | Re-trigger Greptile |
Contributor
|
Size Change: 0 B Total Size: 80.2 MB ℹ️ View Unchanged
|
Contributor
|
✅ Visual changes approved by @sampennington — baseline updated in 4 changed. |
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
![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 change — wires an existing showValuesOnSeries selector into the lifecycle chart and adds a clipping-flip guard in ValueLabels. No backend, data model, API contract, or CI changes. Tests and snapshot updates are included and correct.
Wires `showValuesOnSeries` from `trendsDataLogic` through `buildTrendsLifecycleConfig` so the `TimeSeriesBarChart` renders per-segment value labels when "Values on series" is toggled on, matching the behavior of `TrendsBarChart`. The hog-charts `ValueLabels` overlay already places labels above positive segments and below negatives, so dormant labels render below the zero baseline in diverging stacks without further work. Adds two Storybook stories (`StackedWithValuesOnSeries`, `UnstackedWithValuesOnSeries`) so the snapshot pipeline captures the rendered labels. Generated-By: PostHog Code Task-Id: a3175207-c8ad-409a-a965-1b16bce4ab7d
Bars that reached the y-axis top tick (e.g. 50 in a [0, 50] domain) had their value labels clipped at the top edge of the chart wrapper, since the label box renders above the bar but the wrapper has `overflow: hidden`. Adds an `extraMargins` option to `ChartConfig` that flows through `useChartMargins` and is applied additively to the auto-computed margins. `TimeSeriesBarChart`, `TimeSeriesLineChart`, and the horizontal aggregated `BarChart` config in `TrendsBarChart` set `extraMargins` based on `VALUE_LABEL_HEIGHT` whenever value labels are enabled, so a label can never be clipped against the plot-area boundary. Bottom-side headroom is intentionally skipped — `d3.nice()` already extends the negative domain past the data, and reserving bottom space would push x-axis ticks into the value-label area. Generated-By: PostHog Code Task-Id: a3175207-c8ad-409a-a965-1b16bce4ab7d
Generated-By: PostHog Code Task-Id: a3175207-c8ad-409a-a965-1b16bce4ab7d
Replaces the earlier extra-margin approach (which shrank every chart vertically to make room for the label box) with a per-candidate placement swap in `ValueLabels`. The default placement renders the label outside the bar (above the top in vertical, right of the end in horizontal); when that would clip against the wrapper's `overflow: hidden` boundary, the overlay swaps the candidate's `above` flag so the label is anchored inside the bar instead. Only swaps when the inside placement actually fits — if neither side fits, the original placement is kept so behaviour outside the visible plot area is unchanged. Generated-By: PostHog Code Task-Id: e69ebe26-7527-4b97-ae59-724e3b82c6ce
Generated-By: PostHog Code Task-Id: e69ebe26-7527-4b97-ae59-724e3b82c6ce
10 updated Run: f5c7df42-3166-4609-96e1-6793e5368cf8 Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
The first test in the file rendered the chart with the default 1000ms waitFor timeout, which intermittently flaked in CI before the chart mounted. Switched to findByRole with an explicit 3000ms timeout, matching the DEBOUNCE_TIMEOUT used by the insight-testing interaction helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sampennington
force-pushed
the
posthog-code/lifecycle-value-labels
branch
from
88fbb7e to
6cdf977
Compare
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
Generated-By: PostHog Code Task-Id: 1cc492ff-a817-49e8-819d-d483c48dde15
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
…m scratch" This reverts commit 67b436c. The reset reintroduced 14 stale entries for stories that no longer exist (hogcharts-sparkline path doesn't exist, StickinessBarChart has no .stories.tsx). Restoring the PR's original snapshots.yml. Generated-By: PostHog Code Task-Id: 1cc492ff-a817-49e8-819d-d483c48dde15
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
Resolved ValueLabels.tsx — both branches added a destructured field from the chart context (lifecycle: dimensions, master: hoverIndex). Kept useChart() with both fields so the lifecycle flip-clipped logic and the master hover-lift coexist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves conflict in frontend/src/lib/hog-charts/overlays/ValueLabels.tsx by adopting the master split of useChart() into useChartLayout() + useChartHover() while keeping the PR's flipClippedCandidates / dimensions usage. Generated-By: PostHog Code Task-Id: be7f7f3e-db9d-4c8f-add8-6469092aa17c
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
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.
Purely additive frontend feature — wires an existing selector into the lifecycle chart and adds a bounds-check flip in ValueLabels to prevent label clipping. No backend, data model, API contract, or CI changes; tests and snapshots are correctly updated.
4 updated Run: 2d203c32-f880-4f6b-acb7-5cb7f420a758 Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
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.
Purely additive frontend feature wiring an existing showValuesOnSeries selector into the lifecycle chart, with a well-implemented clipping-flip guard in ValueLabels. No backend, data model, API contract, or CI changes; tests and snapshots are correctly updated.
DanielVisca
added a commit
that referenced
this pull request
May 29, 2026
…rename posthog.temporal.llm_analytics.* was renamed to posthog.temporal.ai_observability.* in master (#60131). Update the metrics module's ExecutionTimeRecorder / get_metric_meter import and the precedent docstrings accordingly. Generated-By: PostHog Code Task-Id: 973975fd-03b4-460b-b81e-3ee5c636e086
DanielVisca
added a commit
that referenced
this pull request
Jun 4, 2026
…rename posthog.temporal.llm_analytics.* was renamed to posthog.temporal.ai_observability.* in master (#60131). Update the metrics module's ExecutionTimeRecorder / get_metric_meter import and the precedent docstrings accordingly. Generated-By: PostHog Code Task-Id: 973975fd-03b4-460b-b81e-3ee5c636e086
DanielVisca
added a commit
that referenced
this pull request
Jun 4, 2026
…rename posthog.temporal.llm_analytics.* was renamed to posthog.temporal.ai_observability.* in master (#60131). Update the metrics module's ExecutionTimeRecorder / get_metric_meter import and the precedent docstrings accordingly. Generated-By: PostHog Code Task-Id: 973975fd-03b4-460b-b81e-3ee5c636e086
DanielVisca
added a commit
that referenced
this pull request
Jun 4, 2026
…rename posthog.temporal.llm_analytics.* was renamed to posthog.temporal.ai_observability.* in master (#60131). Update the metrics module's ExecutionTimeRecorder / get_metric_meter import and the precedent docstrings accordingly. Generated-By: PostHog Code Task-Id: 973975fd-03b4-460b-b81e-3ee5c636e086
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.
Problem
Toggling "Values on series" on a lifecycle insight had no effect in the new hog-charts version — the chart never rendered per-segment labels. The wiring existed for
TrendsBarChartbut had not been added toTrendsLifecycleChart.Changes
TrendsLifecycleChartnow readsshowValuesOnSeriesfromtrendsDataLogicand forwards aformatAggregationAxisValue-based formatter throughbuildTrendsLifecycleConfigasvalueLabels.BuildTrendsLifecycleConfigOptsexposes an optionalvalueLabelsfield that maps straight ontoTimeSeriesBarChartConfig.valueLabels.ValueLabelsoverlay already handles negative segments (places dormant labels below the zero baseline in diverging stacks), so no overlay-level changes were needed.StackedWithValuesOnSeriesandUnstackedWithValuesOnSeries— so the snapshot pipeline picks up the rendered labels.How did you test this code?
I'm an agent. Automated checks I added/touched:
trendsLifecycleChartTransforms.test.tswith a parameterized case that assertsbuildTrendsLifecycleConfigforwardsvalueLabels(true,false, and a formatter object) onto the config.testOptions: { snapshotBrowsers: ['chromium'] }) on CI.I did not run Jest or
typescript:checklocally —node_modulesis not installed in this environment.Publish to changelog?
no
Docs update
No docs change needed.
🤖 Agent context
Authored by Claude Code. The fix mirrors the existing
TrendsBarChartpattern: pullshowValuesOnSeries, build a memoizedvalueLabelFormatterviaformatAggregationAxisValue(trendsFilter, value, baseCurrency), and passvalueLabels: showValuesOnSeries ? { formatter } : falseinto the config builder. I considered handling negative dormant values specially (e.g. taking abs in the formatter), but the hog-charts overlay already anchors negative labels below the baseline and the legacy chart.js implementation rendered raw negatives — kept that behavior for consistency.