feat(hog-charts): add useAnimatedNumber and resolveDelta helpers#60206
Merged
Conversation
Two small, dependency-free helpers that the upcoming Metric component relies on, shipped on their own with focused unit tests. - `useAnimatedNumber(target, duration)` — cubic-ease tween between successive targets. Snaps when the duration is non-positive or the target is non-finite; restarts from the currently-displayed value when the target changes mid-animation; cancels its pending frame on unmount. - `resolveDelta(...)` — pure derivation of the change pill: respects `showChange`, treats `change: null` as suppression, prefers a supplied `change` over the hover-derived percent, and rejects non-finite fallbacks. PR 1 of 2; the Metric component that consumes both lands in #59851. Generated-By: PostHog Code Task-Id: 5e93ca14-f811-4b7e-8536-00f8e676ea4f
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. |
sampennington
added a commit
that referenced
this pull request
May 27, 2026
A small block that pairs a headline number with an optional sparkline. Hovering a point swaps the headline; the change pill follows the same index. Number-only mode renders without the chart. Stacked on #60206, which lands the `useAnimatedNumber` hook and the `resolveDelta` derivation with their unit tests. This commit consumes both, adds the visual component + storybook stories + integration tests, and exports `Metric` from the package barrel. Generated-By: PostHog Code Task-Id: 5e93ca14-f811-4b7e-8536-00f8e676ea4f
Contributor
|
Size Change: 0 B Total Size: 80.2 MB ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/lib/hog-charts/blocks/Metric/resolveDelta.ts:1-11
The only thing used from this import is `React.ReactNode` as a type, which pulls the entire React runtime into a module that otherwise has zero runtime dependencies. The `useLatest` sibling in this same codebase (and TypeScript idiom generally) uses `import type` for this pattern.
```suggestion
import type { ReactNode } from 'react'
export interface MetricChange {
value: number
label?: ReactNode
}
export interface ResolvedDelta {
value: number
label: ReactNode
}
```
### Issue 2 of 2
frontend/src/lib/hog-charts/blocks/Metric/resolveDelta.ts:27-34
**Non-finite `change.value` unguarded while the fallback path is**
The fallback path at line 36 explicitly rejects `NaN` and `Infinity`, but the explicit-`change` path does not. If a caller passes `change: { value: NaN }` (no `label`), the function returns `{ value: NaN, label: 'NaN%' }` instead of `null`, which would display "NaN%" in the pill. The asymmetry is also untested — none of the `NullCase` entries cover `change: { value: NaN }` or `change: { value: Infinity }`.
Reviews (1): Last reviewed commit: "feat(hog-charts): add useAnimatedNumber ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+27
to
+34
| if (change !== undefined) { | ||
| if (change === null) { | ||
| return null | ||
| } | ||
| return { | ||
| value: change.value, | ||
| label: change.label ?? formatChange(change.value), | ||
| } |
Contributor
There was a problem hiding this comment.
change.value unguarded while the fallback path is
The fallback path at line 36 explicitly rejects NaN and Infinity, but the explicit-change path does not. If a caller passes change: { value: NaN } (no label), the function returns { value: NaN, label: 'NaN%' } instead of null, which would display "NaN%" in the pill. The asymmetry is also untested — none of the NullCase entries cover change: { value: NaN } or change: { value: Infinity }.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/hog-charts/blocks/Metric/resolveDelta.ts
Line: 27-34
Comment:
**Non-finite `change.value` unguarded while the fallback path is**
The fallback path at line 36 explicitly rejects `NaN` and `Infinity`, but the explicit-`change` path does not. If a caller passes `change: { value: NaN }` (no `label`), the function returns `{ value: NaN, label: 'NaN%' }` instead of `null`, which would display "NaN%" in the pill. The asymmetry is also untested — none of the `NullCase` entries cover `change: { value: NaN }` or `change: { value: Infinity }`.
How can I resolve this? If you propose a fix, please make it concise.![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
The bot flagged a valid, unresolved behavioral asymmetry in resolveDelta.ts: the explicit-change path does not guard against non-finite values (NaN, Infinity) while the fallback path explicitly does — meaning a caller passing { value: NaN } gets back { value: NaN, label: 'NaN%' } instead of null. This is a real bug (not just style), and the comment remains unaddressed.
There was a problem hiding this comment.
There is an unresolved substantive bug flagged by a reviewer: the explicit-change path in resolveDelta.ts does not guard against non-finite values (NaN, Infinity), while the fallback path does — meaning callers can get "NaN%" rendered in the UI instead of null. This behavioral asymmetry is real, untested, and the comment remains unaddressed.
- Switch `import React` to `import type { ReactNode }`; the module has
no runtime React dependency and the rest of the package uses the
type-only import for this pattern.
- Reject non-finite `change.value` in the explicit-change path so it
matches the fallback path's guard. Without it, `change: { value: NaN }`
rendered as "NaN%" and the >= 0 sign check flipped the pill color
the wrong way.
- Add three parameterized null-case rows covering `change.value` of
NaN, Infinity, and -Infinity (with a label, to confirm the guard
fires before the label is used).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sampennington
force-pushed
the
posthog-code/hog-charts-metric-logic
branch
from
bd84b86 to
aa47c12
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All flagged issues are resolved in the current diff — the non-finite guard is present in the explicit change path and covered by parameterized tests, and import type is used correctly. Purely additive frontend utility code with no API, data model, or production risk.
Deploy status
|
sampennington
added a commit
that referenced
this pull request
Jun 1, 2026
A small block that pairs a headline number with an optional sparkline. Hovering a point swaps the headline; the change pill follows the same index. Number-only mode renders without the chart. Builds on the `useAnimatedNumber` hook and the `resolveDelta` derivation (merged in #60206). This commit adds the visual component, storybook stories, and integration tests, and exports `Metric` from the package barrel. Generated-By: PostHog Code Task-Id: 6d7bfdc8-dd44-4934-a0de-5d2cdda59dbd
2 tasks
sampennington
added a commit
that referenced
this pull request
Jun 1, 2026
A small block that pairs a headline number with an optional sparkline. Hovering a point swaps the headline; the change pill follows the same index. Number-only mode renders without the chart. Builds on the `useAnimatedNumber` hook and the `resolveDelta` derivation (merged in #60206). This commit adds the visual component, storybook stories, and integration tests, and exports `Metric` from the package barrel. Generated-By: PostHog Code Task-Id: 6d7bfdc8-dd44-4934-a0de-5d2cdda59dbd
sampennington
added a commit
that referenced
this pull request
Jun 1, 2026
A small block that pairs a headline number with an optional sparkline. Hovering a point swaps the headline; the change pill follows the same index. Number-only mode renders without the chart. Builds on the `useAnimatedNumber` hook and the `resolveDelta` derivation (merged in #60206). This commit adds the visual component, storybook stories, and integration tests, and exports `Metric` from the package barrel. Generated-By: PostHog Code Task-Id: 6d7bfdc8-dd44-4934-a0de-5d2cdda59dbd
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
Two small helpers for the upcoming
Metricblock: an animated-number hook for the headline tween, and a pure derivation that decides what the change pill should say. They land here on their own with focused unit tests so reviewers can sign off on the logic without paging in the fullMetricUI.PR 1 of 2. The
Metriccomponent that consumes both is #59851 (now stacked on this branch).Changes
useAnimatedNumber(target, duration)— cubic-ease tween between successive targets. Snaps when the duration is non-positive or the target is non-finite; restarts from the currently-displayed value when the target changes mid-animation; cancels its pending frame on unmount.resolveDelta(...)— derives the change pill: respectsshowChange, treatschange: nullas suppression, prefers a suppliedchangeover the hover-derived percent, and rejects non-finite fallbacks.Both files live under
frontend/src/lib/hog-charts/blocks/Metric/; they have no consumer in this PR (theMetriccomponent lands in #59851).How did you test this code?
I'm an agent — no manual testing. Added the two test files in this PR:
useAnimatedNumber.test.tsxcovers initial render, snap onduration <= 0, snap on non-finite target, no-op on unchanged target, frame-by-frame interpolation, mid-animation retarget, and unmount cleanup. Uses a controllable RAF +performance.now()mock to step through frames deterministically.resolveDelta.test.tscovers the null-returning paths (parameterized), the supplied-change label/format paths, and the fallback-percent path.Publish to changelog?
no
🤖 Agent context
Authored by Claude Code (Sonnet) at the request of @sampennington while splitting the original
Metriccomponent PR (#59851) into two reviewable pieces. The split was proposed and approved in chat: PR 1 (this one) for the pure logic + unit tests; PR 2 (#59851, rebased to stack on this branch) for theMetricUI component, stories, snapshots, andindex.tsexports.resolveDeltawas originally a private helper insideMetric.tsx; extracted to its own file so it could be unit-tested without rendering the component.useAnimatedNumberwas already its own file in the original PR but had no dedicated tests — added here.