fix(insights): align hog-charts retention bar with legacy visual#60129
Closed
sampennington wants to merge 8 commits into
Closed
fix(insights): align hog-charts retention bar with legacy visual#60129sampennington wants to merge 8 commits into
sampennington wants to merge 8 commits into
Conversation
- Drop the hatched in-progress bars in `RetentionBarChart` (the partial-period indicator now lives only in the line view, matching legacy chart.js behavior). - Add a `closePlotArea` opt-in to the hog-charts bar chart that draws a closing axis line on the far edge of the value axis, so the retention bar grid reads as a full rectangle instead of an L-shape. Generated-By: PostHog Code Task-Id: 453d24da-b271-4e12-93f6-f606c5e5019b
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
frontend/src/lib/hog-charts/core/canvas-renderer.test.ts:556-564
The test name says "does not draw a closing edge when closePlotArea is false (default)" but the body only verifies a relative call-count difference rather than directly asserting that the open case's calls lack the closing-edge coordinates. The positive-direction assertion (`toBeGreaterThan`) accidentally passes even if the open case drew extra lines for unrelated reasons, making it a weaker safety net than the description implies. A direct check that the closing coordinates are absent from `open.moveTo` makes the intent explicit.
```suggestion
it('does not draw a closing edge when closePlotArea is false (default)', () => {
const open = mockCanvasContext()
drawGrid(makeDrawContext(open, ['a', 'b']))
const closingX = dimensions.plotLeft + dimensions.plotWidth - 0.5
expect(open.moveTo.mock.calls).not.toContainEqual([closingX, dimensions.plotTop])
})
```
Reviews (1): Last reviewed commit: "fix(insights): align hog-charts retentio..." | Re-trigger Greptile |
Contributor
|
Size Change: 0 B Total Size: 80.1 MB ℹ️ View Unchanged
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sampennington
commented
May 26, 2026
Contributor
Author
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Ship it
Small, surgical fix. Well-tested at the right layer (pure transform + library), library stays generic, no React/perf regressions. Optional polish only.
Non-anchored findings
- Reuse (canvas-renderer.ts:353-358 / 388-393) — the two closing-edge blocks duplicate the immediately-preceding axis-baseline pattern and are structurally identical to
drawCrosshair. A smalldrawAxisLine(ctx, dims, side, color)helper could cover near-edge baseline, far-edge close, and crosshair, and centralize the+0.5/-0.5pixel-snap. Not blocking — the file already inlines this pattern elsewhere. - Suggestion (hog-charts) —
drawGridacceptsclosePlotAreagenerically but onlyBarChartwires it through. If a futureLineChartconsumer needs the same closed rectangle, they'll need to mirror the three-line wiring. Worth one line in theDrawGridOptionsdoc-comment noting the flag is bar-only by wiring, not by design. - Suggestion (quality + personal) —
TimeSeriesBarChartConfig.closePlotAreaJSDoc saysOnly takes effect when 'yAxis.showGrid' is true, whileBarChartConfig.closePlotAreaJSDoc saysshowGrid. Verify field name and unify, or have the wrapper reference the underlying type instead of repeating verbatim across three locations.
Reviewers: code-reviewer, react-reviewer, personal, reuse, quality, efficiency, hog-charts. Inline comments above are tagged with the reviewer that raised them and the level of concern.
| ctx.lineTo(dimensions.plotLeft + dimensions.plotWidth, axisY) | ||
| ctx.stroke() | ||
| if (options.closePlotArea) { | ||
| const closingY = Math.round(dimensions.plotTop + dimensions.plotHeight) - 0.5 |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality+hog-charts] · Suggestion
This uses Math.round(...) - 0.5 while the rest of the file uses Math.round(...) + 0.5 for half-pixel alignment (gridlines at L329/L339/L354, baselines at L345/L372, crosshair at L387 — with an explanatory comment at L386). The asymmetry is intentional and correct: +0.5 anchors the near edge just inside the plot, -0.5 anchors the far edge just inside the plot, so both edges sit within plotWidth/plotHeight. But it's subtle — a one-liner would save the next reader a minute.
Suggested change
| const closingY = Math.round(dimensions.plotTop + dimensions.plotHeight) - 0.5 | |
| const closingY = Math.round(dimensions.plotTop + dimensions.plotHeight) - 0.5 // -0.5 (vs +0.5 elsewhere) keeps the far edge inside the plot rect |
Contributor
Author
There was a problem hiding this comment.
Fixed in 1305bbb — added an inline comment on the horizontal-mode closing-Y stroke explaining the - 0.5 mirror of the + 0.5 near-edge snap.
| ctx.stroke() | ||
|
|
||
| if (options.closePlotArea) { | ||
| const closingX = Math.round(dimensions.plotLeft + dimensions.plotWidth) - 0.5 |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality+hog-charts] · Suggestion
Same -0.5 (vs the file's prevailing +0.5) as the horizontal-mode case above. If you add the one-line explanation there, this mirror site is fine without one — or extract a pixelEdge(coord, side) helper to centralize both.
Contributor
Author
There was a problem hiding this comment.
Fixed in 1305bbb — added a one-liner pointing back at the horizontal-mode block for the rationale (rather than duplicating it).
Comment on lines
+76
to
+77
| // Bar chart skips the in-progress hatch — matches the legacy chart.js retention bar visual, | ||
| // where the partial-period indicator is reserved for the line view. |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal+code-reviewer] · Nit
This comment narrates the call-site change rather than a load-bearing invariant — the absent incompletenessOffsetFromEnd is already visible in the call. Per the project's comment guidance (only explain the non-obvious), suggest trimming or deleting.
Suggested change
| // Bar chart skips the in-progress hatch — matches the legacy chart.js retention bar visual, | |
| // where the partial-period indicator is reserved for the line view. | |
| // Legacy parity: in-progress stroke is line-only. |
Contributor
Author
There was a problem hiding this comment.
Fixed in 1305bbb — trimmed to // Legacy parity: in-progress stroke is line-only.
| }, | ||
| // Bars side-by-side so the retention trend per cohort stays distinguishable. | ||
| barLayout: 'grouped', | ||
| // Close the right edge so the grid reads as a full rectangle, matching the legacy chart. |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal+quality] · Nit
This duplicates the JSDoc on the closePlotArea option itself, and says "right edge" — which is misleading for horizontal-orientation charts (the type-level JSDoc correctly says "far edge"). Suggest deletion: the field name + JSDoc carry the meaning.
Suggested change
| // Close the right edge so the grid reads as a full rectangle, matching the legacy chart. | |
| closePlotArea: true, |
Contributor
Author
There was a problem hiding this comment.
Already obsolete — that line was removed in 3bb3f34 when closePlotArea became the default behavior (the call no longer exists). Agreed the original wording was wrong for horizontal mode.
| drawGrid(makeDrawContext(closed, ['a', 'b']), { closePlotArea: true }) | ||
|
|
||
| const open = mockCanvasContext() | ||
| drawGrid(makeDrawContext(open, ['a', 'b'])) |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality] · Nit
closed / open for canvas contexts is slightly ambiguous — canvas contexts don't have a "closed" state. closedCtx / openCtx (or withClose / withoutClose) reads more obviously as "with vs without the closePlotArea flag".
Contributor
Author
There was a problem hiding this comment.
Obsolete — the closed/open block was removed in 3bb3f34 when closePlotArea became always-on. Agreed closed for a canvas context was ambiguous.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No showstoppers. This is a small, well-tested visual alignment fix — purely additive with a safe default of false, no API/data model changes, and the only behavioral delta (dropping incompletenessOffsetFromEnd for the bar view) is intentional and covered by tests. All inline review comments are style/quality nits.
Drops the `closePlotArea` opt-in introduced in the previous commit. `drawGrid` now always frames the plot area — left + right axes in vertical orientation, top + bottom in horizontal — so every hog-charts consumer (bar, line, etc.) picks up the box-style grid for free. Generated-By: PostHog Code Task-Id: 453d24da-b271-4e12-93f6-f606c5e5019b
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.
Small, well-tested visual alignment fix — additive canvas edge line and intentional removal of an incompleteness indicator from the bar view to match legacy behavior. No API, data model, or security concerns; all inline review comments are nits or already outdated.
- Explain the `- 0.5` (vs `+ 0.5`) far-edge pixel snap on the closing-axis strokes in `drawGrid` so the asymmetry isn't a riddle for the next reader. - Trim the call-site comment in `RetentionBarChart` to the one load-bearing fact (legacy line-only behavior) — the missing offset is visible in the call itself. Generated-By: PostHog Code Task-Id: 453d24da-b271-4e12-93f6-f606c5e5019b
Contributor
Author
|
Responses to the non-anchored findings:
Pushed 1305bbb with the inline-comment nits (pixel-snap explanation, trimmed retention call-site comment). |
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.
Small, well-tested visual fix — additive canvas edge line and intentional removal of incompleteness indicator from the bar view to match legacy behavior. All inline review comments are resolved or outdated and explicitly addressed by the author in follow-up commits. No API, data model, security, or infra concerns.
…fmt) Generated-By: PostHog Code Task-Id: 453d24da-b271-4e12-93f6-f606c5e5019b
|
Retaining stamphog approval — delta since last review classified as |
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. |
22 updated Run: d12b89ad-53f4-4e86-89f7-f083cd625429 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.
Small, well-tested visual alignment fix — additive canvas closing-edge strokes and an intentional removal of the incompleteness indicator from the retention bar view to match legacy behavior. No API, data model, security, or infra concerns; all inline review comments are resolved or outdated and explicitly addressed by the author in follow-up commits.
CI Jest shard surfaced an `Unable to find role="img" and name 'chart with 1 data series'` failure on this PR, but the failing test is unrelated to the bar-chart visual fix — it was the only test in the file that used `waitFor` with the default 1s timeout, while every sibling test reads the same canvas via `chart.hoverTooltip` / `chart.clickAtIndex`, both of which call `findByRole` with the project's 3s `DEBOUNCE_TIMEOUT`. Under CI load the chart mount + query resolve can exceed 1s, which made this the canary for the file. Aligns the render-existence assertion with the sibling pattern: `findByRole` with a 3s timeout. No behavioral change, just resilience against the slow-cold-start shape of CI. Generated-By: PostHog Code Task-Id: 453d24da-b271-4e12-93f6-f606c5e5019b
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates denied this PR: it touches CI/CD workflows, migrations, and dependencies — all on the deny-list. The PR is also massively larger than auto-review thresholds (10,425 lines, 170 files) and is cross-cutting across multiple teams whose codebase the author doesn't own.
Contributor
MCP UI Apps size report
|
This reverts commit 3a8e277. The previous commit was authored from a stale local checkout that didn't include the merge of master (191a312). The signed-commit tool created the commit on top of the remote tip but with content from the stale local state, which effectively reverted every master change brought in by the merge — 168 unrelated files, 7700+ lines. Reverting restores the post-merge state. The original de-flake intent (`findByRole` with a 3s timeout on the stickiness render test) can be reapplied in a follow-up if the test still flakes, but it should be a one-file change, not a 168-file change. Generated-By: PostHog Code Task-Id: 453d24da-b271-4e12-93f6-f606c5e5019b
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. |
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
The retention bar chart looks different depending on whether the
product-analytics-hog-charts-retentionflag is on. Compared to the legacychart.js version, the hog-charts version:
in-progress (partial) period tail. The legacy chart only uses the
incompleteness indicator on the line view — the bar view shows nothing
special.
"L-shape" rather than a closed rectangle.
Changes
RetentionBarChartstops forwardingincompletenessOffsetFromEndtobuildRetentionSeries, so nostroke.partialis set and the bar chart nolonger hatches in-progress bars. The line chart still receives the offset
and continues to dash the in-progress tail.
buildRetentionBarChartConfignow setsclosePlotArea: trueon theresulting
TimeSeriesBarChartConfig.closePlotAreaflag onBarChartConfig/TimeSeriesBarChartConfig, plumbed intodrawGrid. When enabled it drawsa closing axis line on the far edge of the value axis (right edge in
vertical orientation, bottom in horizontal), matching the legacy chart.js
box-style grid. Default is
false, so other charts are unaffected.Visual coverage is provided by the existing
Scenes-App/Insights/Retention - RetentionBarstory (gated on the hog-charts retention flag) — thestorybook snapshot diff will pick up both the missing hatch and the new
right-side grid line.
How did you test this code?
I'm an agent (Claude Code). Manual UI verification was not performed.
Automated tests run locally:
pnpm exec jest products/product_analytics/frontend/insights/retention/shared/retentionChartTransforms.test.tspnpm exec jest src/lib/hog-charts/core/canvas-renderer.test.tspnpm exec jest src/lib/hog-charts/charts(full hog-charts chart suites)New tests:
retentionChartTransforms.test.tsasserts the bar config setsclosePlotArea: true.canvas-renderer.test.tsassertsdrawGridemits a closing right line(vertical) / bottom line (horizontal) when
closePlotArea: true, and thatthe default leaves the plot area open.
Publish to changelog?
no
Docs update
No docs change needed.
🤖 Agent context
Authored by Claude Code (Opus 4.7) at the request of @SammyP. The user
provided two screenshots comparing the retention bar chart with the
product-analytics-hog-charts-retentionflag off vs on. The two visualdeltas (hatched bars and open right edge) were the explicit asks.
Decisions worth flagging for review:
closePlotAreaas an opt-inBarChartConfigflag rather thanchanging
drawGriddefaults. That keeps every other hog-chart consumer(trends bar, funnel steps bar, line charts) on its current visual; only
retention picks up the new closing edge. Happy to widen the scope if
reviewers think the box-style grid should be the default everywhere.
buildRetentionSeriesto suppress thepartial stroke, but found the simpler change is for the bar chart to just
not pass
incompletenessOffsetFromEndat all — same outcome, smallersurface area, and the function still works the same for the line chart.
RetentionBaralready exists and isgated on the retention hog-charts flag; the existing snapshot covers
both changes.