Enhance Revenue report component with metric selection and filtering#4103
Conversation
- Added FilterButtons component for metric selection in the Revenue report. - Introduced new metric definitions and updated chart data handling to support dynamic metric selection. - Updated SQL queries to return detailed revenue data including sum, average, count, and unique count. - Improved type definitions for revenue data structures to ensure type safety.
|
@Ingole712521 is attempting to deploy a commit to the Umami Software Team on Vercel. A member of the Team first needs to authorize it. |
Ingole712521
changed the title
Greptile SummaryThis PR enhances the Revenue report by adding a Key points:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Revenue as Revenue.tsx
participant FilterButtons
participant BarChart
participant API as getRevenue (SQL)
User->>Revenue: selects currency / date range
Revenue->>API: useResultQuery({ websiteId, startDate, endDate, currency })
API-->>Revenue: { chart: [{t, sum, avg, count, unique_count}], total, country }
Note over Revenue: Computes metricDefinitions, chartData, metrics
Revenue->>FilterButtons: items=metricOptions, value=metric
Revenue->>BarChart: chartData (single dataset for selected metric)
User->>FilterButtons: clicks different metric (e.g. "Transactions")
FilterButtons->>Revenue: onChange("count")
Revenue->>Revenue: setMetric("count") → re-maps data.chart[].count as y
Revenue->>BarChart: updated chartData (no new API call)
BarChart-->>User: re-renders chart with count data
Reviews (1): Last reviewed commit: "Enhance Revenue report component with me..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -64,12 +65,16 @@ async function relationalQuery( | |||
| and revenue.created_at between {{startDate}} and {{endDate}} | |||
| and revenue.currency = upper({{currency}}) | |||
| ${filterQuery} | |||
| group by x, t | |||
| group by t | |||
| order by t | |||
There was a problem hiding this comment.
The original SQL query grouped by both event_name and t, producing one row per (event_name, time_bucket). This let the frontend render one dataset per revenue event type (e.g., "purchase", "subscription") as separate stacked bars. The new query drops the event_name column entirely and groups only by t, merging all revenue event types into a single aggregate line.
Users who relied on the chart to compare revenue contributions from multiple event types will lose that visibility. For example, a business with both "checkout" and "upsell" events will now only see a combined bar instead of the two separate series.
The PR description mentions "Preserved existing behavior and UI consistency", but this is a material behavioral regression for any site with more than one revenue event name.
Consider either:
- preserving the per-event-type breakdowns as a selectable view, or
- explicitly documenting this as a deliberate simplification
| import { useMemo } from 'react'; | ||
| import { useNavigation } from './useNavigation'; | ||
|
|
||
| const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; |
There was a problem hiding this comment.
The version nibble [1-5] only matches UUID versions 1 through 5. UUID versions 6, 7, and 8 are standardized (RFC 9562) and increasingly used — UUID v7 in particular is now commonly chosen for sortable IDs in modern databases and ORMs.
A valid UUID v7 segment/cohort ID (version nibble = 7) would fail this regex and be silently replaced with undefined, causing the filter to be dropped without any error. This could confuse users whose segments aren't applied even though they appear selected.
| const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; | |
| const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-9a-f][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; |
| })} | ||
| </MetricsBar> | ||
| <Panel> | ||
| <FilterButtons items={metricOptions} value={metric} onChange={handleMetricChange} /> |
There was a problem hiding this comment.
FilterButtons holds uncontrolled internal state
FilterButtons initialises its own selected state from the value prop via useState(value), which means it only reads value once on mount. After that, the parent's metric state and the button component's display state are decoupled. If metric is ever changed externally — e.g., a future URL-driven filter, a "reset" action, or any navigation that remounts the parent — the highlighted button will stay on the old selection while the chart renders the new one.
The component should be converted to a controlled component:
| <FilterButtons items={metricOptions} value={metric} onChange={handleMetricChange} /> | |
| export function FilterButtons({ items, value, onChange }: FilterButtonsProps) { | |
| const handleChange = (value: string) => { | |
| onChange?.(value); | |
| }; | |
| return ( | |
| <Box> | |
| <ToggleGroup | |
| value={[value]} | |
| onChange={e => handleChange(e[0])} | |
| disallowEmptySelection={true} | |
| > |
(Change would be in FilterButtons.tsx; the Revenue usage already passes value={metric} correctly.)
| stacked={true} | ||
| currency={currency} | ||
| stacked={false} | ||
| currency={selectedMetric.isCurrency ? currency : undefined} |
There was a problem hiding this comment.
BarChart formats its hover tooltip using:
value: currency
? formatLongCurrency(dataPoints[0].raw.y, currency)
: `${formatLongNumber(dataPoints[0].raw.y)} ${dataPoints[0].dataset.label}`The currency prop is correctly set to undefined for count/unique_count metrics, so the tooltip will show e.g. "5 Transactions". However, the dataset label shown in the tooltip comes from selectedMetric.label which is re-resolved in the same chartData memo, so this should be consistent.
One edge case: when switching from a currency metric back to a non-currency metric, the currency prop is undefined but the previous chart tooltip (if still visible) could flash stale data before the memo re-runs. This is a minor UX issue, but worth being aware of during testing.
1 participant
Feature: Add Metric Selector to Revenue Chart
Summary
This PR enhances the Revenue page by introducing a metric selector for the chart, allowing users to switch between different key metrics instead of being limited to Total revenue.
Problem
Currently, the Revenue chart only displays Total revenue over time, while other important metrics such as Average, Transactions, and Unique Customers are shown only as aggregated values.
This makes it difficult to analyze how these metrics evolve over time and limits the usefulness of the chart for deeper insights. Additionally, it does not support visualization of custom server-side metrics.
Changes Implemented
Added a metric selector (tabs/dropdown) to the Revenue page
Enabled switching between:
Updated chart to dynamically render based on selected metric
Reused existing data structures where possible
Preserved existing behavior and UI consistency
Improvements
Technical Details
Fix #4097