fix: calculate country visitors share relative to total #3955
Conversation
as opposed to relative to the top-10 countries shown in the breakdown
|
@dankotov is attempting to deploy a commit to the umami-software Team on Vercel. A member of the Team first needs to authorize it. |
Greptile OverviewGreptile SummaryThis PR fixes percentage calculation in session metrics (Location, Environment panels) to show percentages relative to total visitors rather than the sum of displayed items. The implementation adds a parallel SQL query to fetch total unique sessions and modifies Key Changes:
Approach: Scope: Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client as MetricsTable/WorldMap
participant Hook as useWebsiteMetricsQuery
participant API as /api/websites/[id]/metrics
participant Query as getSessionMetrics
participant DB as Database (Prisma/ClickHouse)
Client->>Hook: Request metrics (type, limit, offset)
Hook->>API: GET /metrics?type=country&limit=10
API->>Query: getSessionMetrics(websiteId, params, filters)
par Fetch paginated data
Query->>DB: SELECT metric, COUNT(DISTINCT session_id)<br/>GROUP BY metric<br/>ORDER BY count DESC<br/>LIMIT 10
DB-->>Query: [{x: 'US', y: 888}, ...]
and Fetch total sessions
Query->>DB: SELECT COUNT(DISTINCT session_id)<br/>(no GROUP BY, no LIMIT)
DB-->>Query: [{total: 4712}]
end
Query-->>API: {data: [...], total: 4712}
API-->>Hook: {data: [...], total: 4712}
Hook-->>Client: {data: [...], total: 4712}
Client->>Client: percentFilter(data, total)<br/>Calculate: 888/4712 = 18.8%
Client->>Client: Render with correct percentages
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (3)
When Papa.unparse receives Fix needed: Extract the Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/api/websites/[websiteId]/export/route.ts
Line: 37:40
Comment:
The export functionality will break for session metrics. `getSessionMetrics` now returns `{ data: [...], total: number }` but `Papa.unparse()` on lines 52-58 expects a flat array structure.
When Papa.unparse receives `{ data: [...], total: 1000 }`, it will not correctly convert this to CSV format. This will break CSV exports for browsers, os, devices, and countries.
**Fix needed:** Extract the `.data` property from session metrics results before passing to `parse()`:
```suggestion
getSessionMetrics(websiteId, { type: 'browser' }, filters).then(r => r.data),
getSessionMetrics(websiteId, { type: 'os' }, filters).then(r => r.data),
getSessionMetrics(websiteId, { type: 'device' }, filters).then(r => r.data),
getSessionMetrics(websiteId, { type: 'country' }, filters).then(r => r.data),
```
How can I resolve this? If you propose a fix, please make it concise.
This means WorldMap will show incorrect percentages (based on sum of displayed countries rather than total visitors), defeating the purpose of this PR. Fix needed: Handle the new format similar to how MetricsTable does it: Prompt To Fix With AIThis is a comment left during a code review.
Path: src/components/metrics/WorldMap.tsx
Line: 35:38
Comment:
WorldMap is not updated to handle the new `{ data, total }` format from `getSessionMetrics`. Since 'country' is in `SESSION_COLUMNS`, the API now returns `{ data: [...], total: number }`, but this code:
1. Casts the entire response as `any[]` which will fail
2. Doesn't extract the `total` field
3. Doesn't pass `total` to `percentFilter`
This means WorldMap will show incorrect percentages (based on sum of displayed countries rather than total visitors), defeating the purpose of this PR.
**Fix needed:** Handle the new format similar to how MetricsTable does it:
```suggestion
const metrics = useMemo(() => {
const source = data || mapData;
if (!source) return [];
// Handle both old format (array) and new format ({ data, total })
const items = Array.isArray(source) ? source : source.data;
const total = Array.isArray(source) ? undefined : source.total;
return percentFilter(items, total);
}, [data, mapData]);
```
How can I resolve this? If you propose a fix, please make it concise.
This creates a type safety issue where TypeScript expects an array but runtime can return an object. Suggested fix: Update the type to handle both formats: Prompt To Fix With AIThis is a comment left during a code review.
Path: src/components/hooks/queries/useWebsiteMetricsQuery.ts
Line: 7:10
Comment:
Type definition is now inaccurate. The API returns different formats depending on the metric type:
- **SESSION_COLUMNS** (browser, os, device, country, city, region, language): `{ data: { x: string; y: number }[], total: number }`
- **EVENT_COLUMNS** (path, entry, exit, referrer, etc.): `{ x: string; y: number }[]`
- **channel**: `{ x: string; y: number }[]`
This creates a type safety issue where TypeScript expects an array but runtime can return an object.
**Suggested fix:** Update the type to handle both formats:
```suggestion
export type WebsiteMetricsData =
| { x: string; y: number }[]
| { data: { x: string; y: number }[]; total: number };
```
How can I resolve this? If you propose a fix, please make it concise. |
getSessionMetrics now returns { data, total } but Papa.unparse()
expects a flat array for CSV export
WorldMap now correctly extracts data and total from the new
{ data, total } response format for accurate percentage calculation.
Type now correctly represents both array format (event/pageview metrics)
and { data, total } format (session metrics).
1 participant
Fix percentage calculation in session metrics
Problem
The Location (and other session metrics) panel calculates percentages based on the sum of displayed items rather than total visitors.
For example, with 4,712 total visitors and 888 from the US:
Changes
getSessionMetricsto return{ data, total }instead of just the data arraypercentFilterto accept an optional total parameterMetricsTableto use the total from the API responseNote
This fix currently only applies to session metrics (Location, Environment panels due to the
getSessionMetricsrefactor). Other metrics (getPageviewMetrics,getEventMetrics,getChannelMetrics) still use the old calculation. Happy to implement the same fix for those if the team approves this approach.Fixes #3912