Question: is `report.metric` intentionally ignored by the Metric chart, or was the hardcoded `count` a stopgap?

[gkapatia]

Discussions aren't enabled on the repo, so opening this as a question issue rather than a bug report — I'd rather check the intent before sending a PR that might be arguing against a decision you already made deliberately.

What I was looking at

A report has a metric field (sum | average | min | max, default sum) and IChartMetric/zMetric additionally allow count. For the Metric chart type, that field appears to have no effect: apps/start/src/components/report-chart/metric/chart.tsx renders

<MetricCard key={serie.id} serie={serie} metric={'count'} unit={unit} />

MetricCard itself reads serie.metrics[metric] correctly, and serie.metrics already carries { count, sum, average, min, max } for every series (packages/db/src/engine/format.ts). There's also no UI anywhere to change a report's metric — no changeMetric action in reportSlice.ts, and no metric case in sidebar/ReportSettings.tsx.

My first read was "wiring bug". Then I found 84fd5ce ("fix: metric chart total count"), which changed it in exactly that direction on purpose:

-    report: { metric, unit },
+    report: { unit },
-            metric={metric}
+            metric={'count'}

…and in the same commit added count to the metrics constant, added uniq(profile_id) as total_count to the chart SQL, and added the count → sum coercion in report.ts. So the hardcoded count looks like the point of that commit, not an oversight.

The actual question

1. Is report.metric intentionally inert for chartType: 'metric' — i.e. a Metric card should always be the total unique count, and the field is only meaningful to other chart types?
2. Or was hardcoding count a stopgap for the total-count fix, with a per-report aggregation picker still wanted?

Happy to go either way; I just don't want to guess.

A separate thing that looks unambiguously wrong

Independent of the above: count is accepted by the public schema but cannot be stored.

• zMetric (packages/validation/src/index.ts) includes count, and zReportInput.metric defaults to 'sum'.
• The Postgres enum has no count: CREATE TYPE "Metric" AS ENUM ('sum', 'average', 'min', 'max') (migrations/20240131212106_add_metric/migration.sql).
• So report.create / report.update silently rewrite it: metric: report.metric === 'count' ? 'sum' : report.metric (packages/trpc/src/routers/report.ts:68 and :115).

A caller that posts metric: 'count' gets a 200 and a report persisted as sum — no zod error, no warning. This is reachable from the agent/MCP generate_report tool, whose schema explicitly allows it: metric: z.enum(['sum', 'count', 'average']) (apps/api/src/agents/tools/base.ts:261). Also worth noting report.ts:188 (duplicate) doesn't have the coercion the other two paths do; it's safe today only because the value comes from the DB.

Whatever the answer to the design question, the schema advertising a value the storage can't hold seems worth closing — either by adding count to the enum, or by narrowing the persisted type so it's rejected instead of silently downgraded.

What I'd like to do (and the caveat)

If you're open to the picker, I have a change ready that:

1. adds a changeMetric action to reportSlice.ts (mirroring changeUnit);
2. adds an aggregation picker to ReportSettings.tsx for chartType === 'metric', reusing the labels already used by the report table (Unique / Sum / Average / Min / Max);
3. passes report.metric through in metric/chart.tsx;
4. adds count to the Metric enum (ALTER TYPE "Metric" ADD VALUE 'count') and drops the count → sum coercion, so a picked count actually persists.

Being upfront: step 3 reverses the metric={'count'} line from 84fd5ce. My argument for why that's safe given steps 1, 2 and 4 — and why it needs them — is that count stops being a hardcoded constant and becomes a stored, user-visible choice, so the total-unique-count behaviour that commit was fixing remains reachable and becomes explicit rather than implicit.

The part that needs your judgement is existing data. reports.metric is NOT NULL DEFAULT 'sum', so every existing Metric-type report stores 'sum', and there's no way to distinguish "user chose sum" from "never touched". Passing report.metric straight through would therefore flip every existing Metric card from count to sum — and those aren't near-neighbours: count is uniq(profile_id) for the whole range, while sum is the sum of per-bucket values, where the bucket is count(*) for a default event series. A tile currently reading ~1.2k unique users could read ~45k events instead (and for property_sum series they aren't even the same unit).

So I'd pair it with a backfill — UPDATE reports SET metric = 'count' WHERE "chartType" = 'metric' — so no existing dashboard number changes. That looks lossless because for Metric reports the stored value is currently read by nothing: the SQL builders ignore metric (your own comment in chart-sql.test.ts notes it's a display-only field), and the render hardcodes count. But it is still a data migration on user data, so I'd rather you sign off than surprise you with it in a PR.

(One implementation note if you do want it: the ALTER TYPE ... ADD VALUE and the UPDATE have to be in two separate migration files — Postgres won't let a new enum value be used in the transaction that added it.)

Some smaller things I noticed while tracing this, happy to split into their own issues if useful:

• getAggregateChartSql never selects total_count, so bar/pie series always have count: undefined and the report table's "Unique" column is permanently blank for them.
• format.ts uses .find(item => !!item.total_count), so a genuine 0 is indistinguishable from missing and renders as N/A.
• Series are ranked by metrics.sum in format.ts while the Metric card displays metrics.count, so which cards survive the 4-card cap is decided by a number that isn't the one shown.

Thanks — happy to close this if the answer is "working as intended".

[gkapatia]

I went ahead and built the change so the question isn't abstract — branch: https://github.com/gkapatia/openpanel/tree/fix/metric-chart-aggregation

Deliberately not opening a PR yet. It reverses a line you wrote on purpose, so it seems wrong to put it in your review queue before you've said whether you want it. Say the word and I'll open it, or close this if it's working as intended.

What I verified locally (Postgres 14 + ClickHouse via docker-compose.yml, 40 events across 3 days from 5 distinct profiles):

The Linear data table for that dataset reads Unique 5 · Sum 40 · Average 13.33 · Min 0 · Max 20. With the picker, the Metric card matches every column:

Aggregation	Linear column	Metric card
Unique	5	5
Sum	40	40
Average	13.33	13 (number.short)
Min	0	0
Max	20	20

Min needed one extra fix: MetricCard's renderValue starts with if (!value), so a real 0 rendered as N/A. That's pre-existing, but the picker is what makes it reachable (min is 0 whenever the range has an empty bucket), so I changed it to a === undefined || === null check. Genuinely-absent values still show N/A, which matters because getAggregateChartSql never selects total_count — so count really is undefined for bar/pie.

Persistence, which was the part I most wanted to confirm: saving with "Unique" selected now stores metric = count and the report reloads showing 5. Before the enum change the same save silently wrote sum.

Backfill, checked against a report seeded to sum to imitate an existing one: chartType = 'metric' flips to count, and a linear control row keeps sum untouched.

Two things worth flagging on the migration:

1. ALTER TYPE ... ADD VALUE and the UPDATE cannot share a migration file. Postgres 14 gives ERROR: unsafe use of new value "count" of enum type Metric / HINT: New enum values must be committed before they can be used. Hence two files. (I confirmed this both ways on a scratch enum.)
2. count is appended last in the Prisma enum to match what ADD VALUE does in the DB, so prisma migrate reports no drift.

One thing I left alone for you to decide: reportSlice.ts initialState.metric is still 'sum', so a newly created Metric report defaults to Sum while backfilled existing ones show Unique. Changing the default would touch every chart type, so I didn't — but it does mean the default differs between old and new Metric reports. Happy to set it either way.

Also report-chart/map/chart.tsx already reads report.metric (s.metrics[metric] ?? 0) with no UI to set it, so map has the same gap. Adding chartType === 'map' to the picker condition would be a one-word change and, unlike metric, purely additive — it can't change any existing rendering since map already honours the stored value. Left it out to keep this scoped; happy to include it.

pnpm typecheck passes across the monorepo. I didn't run pnpm fix per CLAUDE.md — for the record the diff adds 2 Biome diagnostics, both in ReportSettings.tsx (useSortedClasses + useSortedAttributes) and both identical in kind to the 8 existing instances on the sibling blocks in that same file, since I copied the unit field's pattern.

Unrelated, but noticed while setting up: apps/worker doesn't build on main for me — rolldown panics with no entry found for key at crates/rolldown/src/module_finalizers/mod.rs:1312. Reproduces with a clean tree, so it's not from my change. Want a separate issue?