diff --git a/.changeset/dataset-percent-scale.md b/.changeset/dataset-percent-scale.md new file mode 100644 index 000000000..ffc0fd850 --- /dev/null +++ b/.changeset/dataset-percent-scale.md @@ -0,0 +1,35 @@ +--- +"@object-ui/core": patch +"@object-ui/plugin-dashboard": patch +"@object-ui/plugin-report": patch +"@object-ui/app-shell": patch +--- + +Honor the server's declared percent scale, so a ratio of exactly 1 renders as 100.0% (#3136) + +A dataset measure declared `format: '0.0%'` rendered every ratio below 1 +correctly and got the single most consequential one wrong: a rate of exactly +`1` printed as **`1.0%`**. On an SLA / pass-rate dashboard that turns +"everything met the SLA" into "1% met the SLA", on both surfaces the issue +names — the KPI card and the dataset-bound table (they share `formatMeasure`). + +The cause was never a bad multiplier; it was a missing fact. `formatMeasure` +scaled by magnitude — `percentDisplayValue` multiplies by 100 only strictly +inside `(-1, 1)` — because the column arrived with a `%` format string and +nothing saying what scale its numbers were on. That guess is undecidable at +exactly 1, which is both a full-compliance ratio ("100%") and one percentage +point ("1%"), and it resolved to the reading almost nobody means. + +The server now answers the question instead (framework: `percentScaleOf` + +`AnalyticsResult.fields[].percentScale`, the sibling of the ADR-0053 currency +chain): a `derived: { op: 'ratio' }` measure is a `fraction` by definition, and +a measure over a `percent` field inherits that field's scale. `formatMeasure` +takes the declared scale as a fourth argument and, when present, scales by it — +`fraction` ×100, `whole` verbatim — instead of inspecting the value. Every +dataset-bound call site passes the column's `percentScale`: the dashboard +metric/table/pivot cells, the report renderer's cells, totals and KPI, and the +dataset preview. + +`percentDisplayValue` is untouched and still the fallback for a column that +arrives without the annotation (an older server, or a non-dataset percent cell +in a list view), so nothing that renders correctly today changes. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx index a72a9f760..dea37a25b 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.tsx @@ -176,7 +176,9 @@ function MeasureFormatField({ measure, onPatch, disabled }: { measure: Measure; const { kind, decimals } = parseMeasureFormat(measure.format, measure.currency); const currency = measure.currency || 'USD'; const apply = (k: string, d: number, c: string) => onPatch(buildMeasureFormat(k, d, c)); - const sample = formatMeasure(kind === 'percent' ? 0.1234 : 1234.5, measure.format, measure.currency); + // The percent sample is a hand-picked 0–1 FRACTION, so it says so rather than + // leaving the formatter to infer a scale from the sample's magnitude. + const sample = formatMeasure(kind === 'percent' ? 0.1234 : 1234.5, measure.format, measure.currency, kind === 'percent' ? 'fraction' : undefined); return (
diff --git a/packages/app-shell/src/views/metadata-admin/previews/DatasetPreview.tsx b/packages/app-shell/src/views/metadata-admin/previews/DatasetPreview.tsx index 85afc1128..c60aeadc6 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/DatasetPreview.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/DatasetPreview.tsx @@ -225,7 +225,7 @@ export function DatasetPreview({ draft }: MetadataPreviewProps) { {columns.map((c) => ( {measureNames.includes(c) - ? formatMeasure(row[c], measureField(c)?.format, measureField(c)?.currency) + ? formatMeasure(row[c], measureField(c)?.format, measureField(c)?.currency, measureField(c)?.percentScale) : formatDimensionValue(row[c])} ))} diff --git a/packages/core/src/utils/__tests__/dataset-format.test.ts b/packages/core/src/utils/__tests__/dataset-format.test.ts index 036b241f2..0ccaeef15 100644 --- a/packages/core/src/utils/__tests__/dataset-format.test.ts +++ b/packages/core/src/utils/__tests__/dataset-format.test.ts @@ -42,13 +42,33 @@ describe('formatMeasure', () => { // metric card shows "0.6%" for an avg of 0.608 instead of "60.8%" (the bug). expect(formatMeasure(0.75, '0%')).toBe('75%'); expect(formatMeasure(0.608_333_333, '0.0%')).toBe('60.8%'); - // Boundary: exactly 0 and exactly 1 (100% stored as 1.0) — 1.0 passes - // through, mirroring the list renderer's strict `< 1` heuristic so the two - // surfaces stay in lockstep (a known shared limitation, not a new one). + // Boundary: exactly 0 and exactly 1 (100% stored as 1.0) — with no declared + // scale, 1.0 passes through, mirroring the list renderer's strict `< 1` + // heuristic so the two unannotated surfaces stay in lockstep. This is the + // ambiguity `percentScale` exists to remove (see below). expect(formatMeasure(0, '0%')).toBe('0%'); expect(formatMeasure(1, '0%')).toBe('1%'); }); + it('honors a DECLARED percent scale over the value-magnitude heuristic (#3136)', () => { + // The bug: a ratio of exactly 1 (full compliance) is indistinguishable from + // 1 percentage point by magnitude alone, and the heuristic resolves it the + // wrong way — "everything met the SLA" reads as "1% met the SLA". The + // server now says which scale the column is on, and that wins. + expect(formatMeasure(1, '0.0%', undefined, 'fraction')).toBe('100.0%'); + expect(formatMeasure(0.6667, '0.0%', undefined, 'fraction')).toBe('66.7%'); + expect(formatMeasure(0, '0.0%', undefined, 'fraction')).toBe('0.0%'); + // Whole-percent storage is the other half of the same ambiguity: a declared + // `whole` column renders 1 as "1%" and is NOT scaled up, even though the + // heuristic would have multiplied a sub-1 value by 100. + expect(formatMeasure(1, '0.0%', undefined, 'whole')).toBe('1.0%'); + expect(formatMeasure(0.5, '0.0%', undefined, 'whole')).toBe('0.5%'); + expect(formatMeasure(80, '0.0%', undefined, 'whole')).toBe('80.0%'); + // Non-percent formats ignore the annotation entirely — it describes the + // percentage scale, not a general multiplier. + expect(formatMeasure(1, '0.0', undefined, 'fraction')).toBe('1.0'); + }); + it('falls back to plain formatting for an unknown currency code', () => { expect(formatMeasure(1234, '0,0', 'NOTACODE')).toBe('1,234'); }); diff --git a/packages/core/src/utils/dataset-format.ts b/packages/core/src/utils/dataset-format.ts index aabf8bc87..9c043cf5b 100644 --- a/packages/core/src/utils/dataset-format.ts +++ b/packages/core/src/utils/dataset-format.ts @@ -29,8 +29,21 @@ export interface DatasetResultField { label?: string; format?: string; currency?: string; + percentScale?: PercentScale; } +/** + * How a percentage column's stored number relates to its displayed percentage — + * the server's answer to a question a `%` format string cannot express: + * `fraction` is a 0–1 ratio (`1` ⇒ "100%"), `whole` is already percentage + * points (`1` ⇒ "1%"). Resolved from metadata server-side (a `ratio` measure is + * a fraction by definition; a measure over a `percent` field inherits that + * field's scale) and carried on the result column, so display never has to + * infer it from the value's magnitude — the inference that printed a ratio of + * exactly 1 as "1.0%" (#3136). + */ +export type PercentScale = 'fraction' | 'whole'; + /** * Scale a stored `percent`-field value to its DISPLAY magnitude. * @@ -55,7 +68,7 @@ export function percentDisplayValue(value: number): number { * controls grouping / decimals / percent; it can't be baked into the row value * server-side (the same number feeds charts), so it is applied here. */ -export function formatMeasure(v: unknown, format?: string, currency?: string): string { +export function formatMeasure(v: unknown, format?: string, currency?: string, percentScale?: PercentScale): string { if (v == null) return '—'; if (typeof v !== 'number') return String(v); @@ -86,7 +99,15 @@ export function formatMeasure(v: unknown, format?: string, currency?: string): s // (0.75 ⇒ 75%). Scale to display magnitude the SAME way the list-view cell // renderer does — otherwise an avg of 0.608 renders as "0.6%" instead of // "60.8%", disagreeing with the per-row "75%" the list already shows. - const display = isPercent ? percentDisplayValue(v) : v; + // + // A DECLARED `percentScale` from the server wins outright: the value-magnitude + // heuristic below cannot tell a 0–1 ratio of exactly 1 from 1 percentage point + // (both are the number 1) and resolves it as "1%", so an SLA rate of full + // compliance rendered as "1.0%" instead of "100.0%" (#3136). The heuristic + // stays as the fallback for columns that arrive without the annotation. + const display = isPercent + ? (percentScale ? (percentScale === 'fraction' ? v * 100 : v) : percentDisplayValue(v)) + : v; const body = display.toLocaleString(undefined, { minimumFractionDigits: decimals ?? 0, maximumFractionDigits: decimals ?? 0 }); return `${legacyDollar}${body}${isPercent ? '%' : ''}`; } diff --git a/packages/plugin-dashboard/src/DatasetWidget.tsx b/packages/plugin-dashboard/src/DatasetWidget.tsx index 6156b4a9c..47910b0c5 100644 --- a/packages/plugin-dashboard/src/DatasetWidget.tsx +++ b/packages/plugin-dashboard/src/DatasetWidget.tsx @@ -400,7 +400,7 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: const value = state.rows[0]?.[values[0]] ?? 0; return (
- {formatMeasure(value, f?.format, f?.currency)} + {formatMeasure(value, f?.format, f?.currency, f?.percentScale)} {headerLabel(values[0])}
); @@ -447,7 +447,7 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: const cellCols = pivot.colHeaders.flatMap((col) => values.map((m) => ({ col, measure: m, header: values.length === 1 ? col.label : `${col.label} · ${headerLabel(m)}` })), ); - const fmtMeasure = (v: unknown, m: string) => formatMeasure(v, measureField(m)?.format, measureField(m)?.currency); + const fmtMeasure = (v: unknown, m: string) => formatMeasure(v, measureField(m)?.format, measureField(m)?.currency, measureField(m)?.percentScale); // Server-supplied marginal totals (ADR-0021): match each grouping by its // dimension array, then its rows to the pivot headers via the same bucket // ids. Absent (older server) → maps stay empty and no totals UI renders. @@ -554,7 +554,7 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: > {columns.map((c) => ( - {values.includes(c) ? formatMeasure(row[c], measureField(c)?.format, measureField(c)?.currency) : formatDimensionValue(row[c])} + {values.includes(c) ? formatMeasure(row[c], measureField(c)?.format, measureField(c)?.currency, measureField(c)?.percentScale) : formatDimensionValue(row[c])} ))} diff --git a/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx b/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx index d2d9a9251..e9c65d253 100644 --- a/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx +++ b/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx @@ -42,6 +42,32 @@ describe('DatasetWidget', () => { expect(screen.queryByText('0.6%')).not.toBeInTheDocument(); }); + it('renders a declared-fraction ratio of exactly 1 as "100.0%" on BOTH the metric card and the table (#3136)', async () => { + // A ratio of exactly 1 is the one value the magnitude heuristic cannot + // resolve — 1 is both "100%" (a full-compliance ratio) and "1%" (a single + // percentage point) — and it resolved it as "1.0%": "everything met the + // SLA" reported to management as "1% met the SLA". The server now declares + // the column's scale, and both dataset-bound surfaces honour it. + const fields = [ + { name: 'status', type: 'string', label: 'Status' }, + { name: 'sla_rate', type: 'number', label: 'SLA rate', format: '0.0%', percentScale: 'fraction' as const }, + ]; + const metricSrc = { queryDataset: vi.fn(async () => ({ rows: [{ sla_rate: 1 }], fields })) }; + render(); + expect(await screen.findByText('100.0%')).toBeInTheDocument(); + expect(screen.queryByText('1.0%')).not.toBeInTheDocument(); + + // Same column in the grouped table: the "met" bucket is 100%, and a + // measured ZERO stays "0.0%" — a real 0% is not missing data. + const tableSrc = { queryDataset: vi.fn(async () => ({ + rows: [{ status: 'met', sla_rate: 1 }, { status: 'breached', sla_rate: 0 }], + fields, + })) }; + render(); + await waitFor(() => expect(screen.getAllByText('100.0%').length).toBe(2)); + expect(screen.getByText('0.0%')).toBeInTheDocument(); + }); + it('runs the dataset query for a dimensioned (chart) widget — rows→dimensions, values→measures', async () => { const src = makeSource(async () => ({ rows: [{ stage: 'won', revenue: 100 }, { stage: 'lost', revenue: 20 }] })); render(); diff --git a/packages/plugin-report/src/DatasetReportRenderer.tsx b/packages/plugin-report/src/DatasetReportRenderer.tsx index 4ada05f2b..4d31616e8 100644 --- a/packages/plugin-report/src/DatasetReportRenderer.tsx +++ b/packages/plugin-report/src/DatasetReportRenderer.tsx @@ -465,7 +465,7 @@ function DatasetReportTable({ {columns.map((c) => ( {values.includes(c) - ? formatMeasure(row[c], measureField(c)?.format, measureField(c)?.currency) + ? formatMeasure(row[c], measureField(c)?.format, measureField(c)?.currency, measureField(c)?.percentScale) : formatDimensionValue(row[c])} ))} @@ -480,7 +480,7 @@ function DatasetReportTable({ )} {values.map((measure) => ( - {formatMeasure(grandTotal[measure], measureField(measure)?.format, measureField(measure)?.currency)} + {formatMeasure(grandTotal[measure], measureField(measure)?.format, measureField(measure)?.currency, measureField(measure)?.percentScale)} ))} @@ -699,7 +699,7 @@ function DatasetReportChart({ {title ?

{title}

: null}
- {formatMeasure(state.rows[0]?.[yAxis], mf?.format, mf?.currency)} + {formatMeasure(state.rows[0]?.[yAxis], mf?.format, mf?.currency, mf?.percentScale)} {headerLabel(yAxis)}
@@ -907,7 +907,7 @@ function DatasetMatrixTable({ data-testid={clickable ? 'dataset-drill-cell' : undefined} onClick={clickable ? () => drillCell(rh.key, cc.col.key, entry!.index) : undefined} > - {formatMeasure(value, measureField(cc.measure)?.format, measureField(cc.measure)?.currency)} + {formatMeasure(value, measureField(cc.measure)?.format, measureField(cc.measure)?.currency, measureField(cc.measure)?.percentScale)} ); })} @@ -918,7 +918,7 @@ function DatasetMatrixTable({ className="px-2 py-1 text-right tabular-nums whitespace-nowrap font-medium" data-testid="matrix-row-total" > - {formatMeasure(rowTotalById.get(rh.id)?.[measure], measureField(measure)?.format, measureField(measure)?.currency)} + {formatMeasure(rowTotalById.get(rh.id)?.[measure], measureField(measure)?.format, measureField(measure)?.currency, measureField(measure)?.percentScale)} ))} @@ -932,7 +932,7 @@ function DatasetMatrixTable({ )} {cellCols.map((cc) => ( - {formatMeasure(colTotalById.get(cc.col.id)?.[cc.measure], measureField(cc.measure)?.format, measureField(cc.measure)?.currency)} + {formatMeasure(colTotalById.get(cc.col.id)?.[cc.measure], measureField(cc.measure)?.format, measureField(cc.measure)?.currency, measureField(cc.measure)?.percentScale)} ))} {showTotalCol && @@ -942,7 +942,7 @@ function DatasetMatrixTable({ className="px-2 py-1 text-right tabular-nums whitespace-nowrap" data-testid="matrix-grand-total" > - {formatMeasure(grandTotal?.[measure], measureField(measure)?.format, measureField(measure)?.currency)} + {formatMeasure(grandTotal?.[measure], measureField(measure)?.format, measureField(measure)?.currency, measureField(measure)?.percentScale)} ))}