Skip to content

Commit 387b11a

Browse files
committed
fix: sound metric-view row/time typing, currency + cache correctness, review cleanup
Address adversarial-review findings on the useMetricView / metric-route branch: - Type soundness: infer rows from the selected measure/dimension tuples (PickMetricRow) and correlate timeDimension/timeGrain to temporal dims only. - Formatting: preserve every currency symbol the generator emits end-to-end and keep bigint precision (no Number() rounding). - Cache correctness: stamp fresh per-column metadata AFTER the cached execute() so a cache hit never serves stale labels/formats after a redeploy. - Charts: guard selected="" as a no-op, split [x,y] click tuples into x/y, and memoize onEvents on handler presence (no listener thrash per SSE tick). - Typegen: sweep a stale sibling metric-views.d.ts on upgrade and reject a .d.ts mvOutFile. - Drop the unused public notify export and the fake "Write back" demo; remove the dead autoStart option; align AnalyticsStreamMessage; tighten the biome ignore; add tests + comment cleanup. Co-authored-by: Isaac Signed-off-by: Atila Fassina <atila@fassina.eu>
1 parent 613d5cb commit 387b11a

27 files changed

Lines changed: 928 additions & 278 deletions

File tree

apps/dev-playground/client/src/routes/metric-views.route.tsx

Lines changed: 4 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
CardTitle,
1717
DonutChart,
1818
LineChart,
19-
notify,
2019
Select,
2120
SelectContent,
2221
SelectItem,
@@ -54,10 +53,10 @@ const TREND_MEASURES = ["arr", "mrr"] as const;
5453
const TABLE_MEASURES = ["arr", "mrr", "new_arr", "churned_arr"] as const;
5554
const TABLE_COLUMNS = ["region", ...TABLE_MEASURES] as const;
5655

57-
// The dimensions the page lets you slice by. Both the filter bar (dropdowns)
58-
// and the detail table (row click) write selections keyed by these names, and
59-
// every visual composes them into a `MetricFilter` the same way — so a future
60-
// chart-click cross-filter drops into the same `selection` state unchanged.
56+
// The dimensions the page lets you slice by. The filter-bar dropdowns, the
57+
// detail-table row click, AND the chart clicks (region bar / segment donut)
58+
// all write selections keyed by these names, and every visual composes them
59+
// into a `MetricFilter` the same way — one shared `selection` state drives them.
6160
const FILTER_DIMENSIONS = ["region", "segment"] as const;
6261
type FilterDimension = (typeof FILTER_DIMENSIONS)[number];
6362
type Selection = Partial<Record<FilterDimension, string>>;
@@ -450,45 +449,6 @@ function MetricViewsRoute() {
450449
yKey={[...TREND_MEASURES]}
451450
height={320}
452451
showLegend
453-
// Render point symbols + trigger clicks along the whole line
454-
// (SDK's triggerLineEvent, on because onDataClick is set) so
455-
// the hairline isn't the only hit target.
456-
showSymbol
457-
// Clicking a point fires a transient "write-back" toast (the
458-
// "Apps = action layer" gesture) through the same Toaster the
459-
// warehouse-status indicator uses.
460-
onDataClick={(d) => {
461-
// Resolve the measure by series INDEX — yKey order ===
462-
// TREND_MEASURES order, and sorting only reorders points
463-
// WITHIN a series, not the series array. (Parsing the "Arr"
464-
// label wouldn't round-trip to keys like "new_arr", and
465-
// indexing trend.data by dataIndex is wrong because the
466-
// time series is sorted before rendering.)
467-
const measureKey = TREND_MEASURES[d.seriesIndex];
468-
if (!measureKey) return;
469-
const meta = trend.metadata?.[measureKey];
470-
const label = meta?.display_name ?? measureKey;
471-
// A time-series point is a [epochMs, value] tuple on
472-
// ECharts' params.value (d.value is null for tuples); read
473-
// it off the raw params so both the date and amount survive.
474-
const tuple = (d.raw as { value?: unknown } | undefined)
475-
?.value;
476-
const [ts, amount] = Array.isArray(tuple)
477-
? tuple
478-
: [undefined, undefined];
479-
const month =
480-
typeof ts === "number"
481-
? new Date(ts).toLocaleDateString(undefined, {
482-
year: "numeric",
483-
month: "short",
484-
})
485-
: String(d.name || "");
486-
const formatted = formatValue(amount, meta?.format);
487-
notify.message(
488-
`Write back: ${label} · ${month} · ${formatted}`,
489-
{ description: "Point selected for write-back (demo)." },
490-
);
491-
}}
492452
/>
493453
)}
494454
</CardContent>

biome.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@
2121
"!**/*.gen.ts",
2222
"!**/typedoc-sidebar.ts",
2323
"!**/template",
24-
"!**/metric-views.ts",
25-
"!**/metric-views.d.ts"
24+
"!**/appkit-types/metric-views.ts",
25+
"!**/appkit-types/metric-views.d.ts"
2626
]
2727
},
2828
"formatter": {

docs/docs/plugins/analytics.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -535,7 +535,7 @@ When `"revenue"` is a key in the generated `MetricRegistry` (see [Metric-view ty
535535
}
536536
```
537537

538-
Like `useAnalyticsQuery`, the option object is serialized internally, so object/array literals passed fresh each render stay referentially stable — you do **not** need to `useMemo` the options. (Hoisting `measures`/`dimensions` to module scope or memoizing is still fine, and keeps the arrays type-narrowed to their literal tuple.)
538+
Like `useAnalyticsQuery`, the option object is serialized (`JSON.stringify`) internally, so object/array literals passed fresh each render do **not** trigger a refetch as long as they serialize to the same string — you do **not** need to `useMemo` the options. (This is same-serialization, not deep structural equality: reordering keys within `filter` changes the string and does re-query. Hoisting `measures`/`dimensions` to module scope or memoizing is still fine, and keeps the arrays type-narrowed to their literal tuple.)
539539

540540
`metadata` is the per-column display metadata for **only the columns you queried**, scoped and carried in the SSE `result` payload. It is `undefined` when the server injected no metadata (the metric key is unknown, or `analytics({ metricViewsMetadata })` was not wired) — so always treat it as optional.
541541

packages/appkit-ui/src/js/format/format.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,20 @@ describe("js/format formatValue", () => {
3434
expect(formatValue(1234567n, "#,##0")).toBe("1,234,567");
3535
});
3636

37+
test("preserves bigint precision beyond 2^53 (no Number() rounding)", () => {
38+
// 9_007_199_254_740_993n = 2^53 + 1, which is NOT representable as a JS
39+
// number — Number(bigint) would round it to 9_007_199_254_740_992.
40+
expect(formatValue(9_007_199_254_740_993n, "#,##0")).toBe(
41+
"9,007,199,254,740,993",
42+
);
43+
expect(formatValue(9_007_199_254_740_993n, "$#,##0")).toBe(
44+
"$9,007,199,254,740,993",
45+
);
46+
expect(formatValue(-9_007_199_254_740_993n, "$#,##0")).toBe(
47+
"-$9,007,199,254,740,993",
48+
);
49+
});
50+
3751
test("no format falls back to toLocaleString for numbers", () => {
3852
expect(formatValue(1234.5)).toBe((1234.5).toLocaleString());
3953
});
@@ -51,6 +65,28 @@ describe("js/format formatValue", () => {
5165
test("non-numeric value with numeric spec falls back to String()", () => {
5266
expect(formatValue("N/A", "#,##0")).toBe("N/A");
5367
});
68+
69+
// End-to-end over the currency symbols the metric-view generator emits
70+
// (mv-registry/describe.ts CURRENCY_SYMBOLS + the unknown-code fallback).
71+
// Each spec here is exactly what the generator produces for that symbol.
72+
describe("preserves every currency symbol the generator emits", () => {
73+
test.each([
74+
["$#,##0.00", 1234.5, "$1,234.50"], // USD
75+
["€#,##0.00", 1234.5, "€1,234.50"], // EUR
76+
["£#,##0.00", 1234.5, "£1,234.50"], // GBP
77+
["¥#,##0", 1234, "¥1,234"], // JPY / CNY
78+
["₹#,##0.00", 1234.5, "₹1,234.50"], // INR
79+
["R$#,##0.00", 1234.5, "R$1,234.50"], // BRL (multi-char symbol)
80+
["XYZ #,##0.00", 1234.5, "XYZ 1,234.50"], // unknown ISO code + space
81+
])("formatValue(%s) preserves the symbol", (spec, value, expected) => {
82+
expect(formatValue(value, spec)).toBe(expected);
83+
});
84+
85+
test("negative currency keeps the sign before the symbol for every prefix", () => {
86+
expect(formatValue(-1234.5, "€#,##0.00")).toBe("-€1,234.50");
87+
expect(formatValue(-1234.5, "R$#,##0.00")).toBe("-R$1,234.50");
88+
});
89+
});
5490
});
5591

5692
describe("js/format formatLabel", () => {
@@ -93,4 +129,19 @@ describe("js/format toD3Format", () => {
93129
expect(toD3Format("yyyy-MM-dd")).toBeUndefined();
94130
expect(toD3Format("abc")).toBeUndefined();
95131
});
132+
133+
// Every currency spec the generator emits maps to d3's `$` currency type
134+
// (the actual glyph is supplied by the consuming d3 locale, not the
135+
// specifier) — none of them are dropped as unrecognized.
136+
test.each([
137+
["$#,##0.00", "$,.2f"],
138+
["€#,##0.00", "$,.2f"],
139+
["£#,##0.00", "$,.2f"],
140+
["¥#,##0", "$,.0f"],
141+
["₹#,##0.00", "$,.2f"],
142+
["R$#,##0.00", "$,.2f"],
143+
["XYZ #,##0.00", "$,.2f"],
144+
])("maps currency spec %s to a $-typed d3 specifier", (spec, expected) => {
145+
expect(toD3Format(spec)).toBe(expected);
146+
});
96147
});

packages/appkit-ui/src/js/format/format.ts

Lines changed: 57 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,24 @@ function formatNumber(
4949
});
5050
}
5151

52+
/**
53+
* The currency symbol a spec carries — everything before the first digit
54+
* placeholder (`#`/`0`). The metric-view generator emits `$`, `€`, `£`, `¥`,
55+
* `₹`, `R$`, or an unknown ISO code + space (e.g. `"XYZ "`); this recovers any
56+
* of them verbatim. Returns `""` for a bare numeric spec (`"#,##0"`) or a
57+
* percent spec (`"0.0%"`), neither of which has a leading symbol.
58+
*/
59+
function currencyPrefix(format: string): string {
60+
const match = format.match(/^[^#0]+/);
61+
return match ? match[0] : "";
62+
}
63+
5264
/**
5365
* Format a raw value using a UC/YAML printf-style format spec.
5466
*
5567
* Recognizes the common spreadsheet-style specs:
56-
* - currency prefix, e.g. `"$#,##0.00"` (1234.5 -> "$1,234.50")
68+
* - currency prefix, e.g. `"$#,##0.00"` (1234.5 -> "$1,234.50"); the prefix is
69+
* emitted verbatim, so `"€#,##0"`, `"R$#,##0.00"`, etc. survive end-to-end
5770
* - thousands grouping + N decimals, e.g. `"#,##0"` (1234567 -> "1,234,567")
5871
* or `"#,##0.00"` (1234.5 -> "1,234.50")
5972
* - percent, e.g. `"0.0%"` (0.1234 -> "12.3%") — the value is multiplied by 100
@@ -73,22 +86,42 @@ export function formatValue(value: unknown, format?: string): string {
7386
return String(value);
7487
}
7588

76-
const num = coerceNumber(value);
77-
// Non-numeric value with a numeric-ish spec: nothing sensible to format.
78-
if (num === null) return String(value);
79-
8089
const isPercent = format.includes("%");
81-
const isCurrency = format.includes("$");
8290
const grouping = format.includes(",");
8391
const decimals = countDecimals(format);
92+
// Any leading symbol (before the first digit placeholder) is a currency
93+
// prefix — emit it verbatim so non-USD symbols the generator produces are
94+
// preserved instead of collapsing to "$".
95+
const prefix = currencyPrefix(format);
96+
97+
// bigint fast path. A bigint is an exact integer, so `BigInt.toLocaleString`
98+
// formats it losslessly — `Number(bigint)` would corrupt values beyond ±2^53
99+
// (int64 counts / cents). The percent path multiplies by 100 (float math a
100+
// large bigint can't survive), so refuse it rather than emit a wrong number.
101+
if (typeof value === "bigint") {
102+
if (isPercent) return String(value);
103+
const sign = value < 0n ? "-" : "";
104+
// `Intl.NumberFormat` accepts a bigint directly and formats it exactly (no
105+
// float coercion), unlike `Number(value)`.
106+
const body = new Intl.NumberFormat("en-US", {
107+
minimumFractionDigits: decimals,
108+
maximumFractionDigits: decimals,
109+
useGrouping: grouping,
110+
}).format(value < 0n ? -value : value);
111+
return `${sign}${prefix}${body}`;
112+
}
113+
114+
const num = coerceNumber(value);
115+
// Non-numeric value with a numeric-ish spec: nothing sensible to format.
116+
if (num === null) return String(value);
84117

85118
if (isPercent) {
86119
return `${formatNumber(num * 100, decimals, grouping)}%`;
87120
}
88121

89-
if (isCurrency) {
122+
if (prefix) {
90123
const sign = num < 0 ? "-" : "";
91-
return `${sign}$${formatNumber(Math.abs(num), decimals, grouping)}`;
124+
return `${sign}${prefix}${formatNumber(Math.abs(num), decimals, grouping)}`;
92125
}
93126

94127
return formatNumber(num, decimals, grouping);
@@ -136,19 +169,25 @@ export function formatLabel(
136169
*
137170
* Best-effort mapping for the common specs:
138171
* - `"$#,##0.00"` -> `"$,.2f"`
139-
* - `"#,##0"` -> `",.0f"`
140-
* - `"#,##0.00"` -> `",.2f"`
141-
* - `"0.0%"` -> `".1%"`
172+
* - `"€#,##0.00"` -> `"$,.2f"` (currency), `"#,##0"` -> `",.0f"`, `"0.0%"` -> `".1%"`
173+
*
174+
* A d3 specifier's currency marker is the single `$` symbol; the actual glyph
175+
* ($, €, R$, …) is supplied by the d3 *locale*, not the specifier string — so a
176+
* non-USD currency spec still maps to the `$` currency type here (it is not
177+
* rejected as unrecognized), and the caller's d3 locale renders the right glyph.
142178
*
143179
* No spec, or a spec that is not a recognizable numeric pattern -> `undefined`.
144180
*/
145181
export function toD3Format(format?: string): string | undefined {
146182
if (!format) return undefined;
147183

148-
// Only map specs built purely from numeric-format characters; anything else
149-
// (date patterns, free text, ...) is left unrecognized.
150-
if (format.replace(/[#0,.$%\s]/g, "") !== "") return undefined;
151-
if (!/[0#]/.test(format)) return undefined;
184+
// Strip any leading currency prefix first, then require the remainder to be
185+
// built purely from numeric-format characters; anything else (date patterns,
186+
// free text, ...) is left unrecognized.
187+
const prefix = currencyPrefix(format);
188+
const numeric = format.slice(prefix.length);
189+
if (numeric.replace(/[#0,.%\s]/g, "") !== "") return undefined;
190+
if (!/[0#]/.test(numeric)) return undefined;
152191

153192
const group = format.includes(",") ? "," : "";
154193
const decimals = countDecimals(format);
@@ -157,6 +196,7 @@ export function toD3Format(format?: string): string | undefined {
157196
return `${group}.${decimals}%`;
158197
}
159198

160-
const prefix = format.includes("$") ? "$" : "";
161-
return `${prefix}${group}.${decimals}f`;
199+
// `$` is d3's currency marker (glyph comes from the locale); emit it for any
200+
// currency prefix, USD or otherwise.
201+
return `${prefix ? "$" : ""}${group}.${decimals}f`;
162202
}

0 commit comments

Comments
 (0)