From acb8f02bca59dafcea43911b4c2fc5b826dffb60 Mon Sep 17 00:00:00 2001 From: saravanakumardb1 Date: Wed, 27 May 2026 14:45:55 -0700 Subject: [PATCH] fix(data-viz): SSR-safe gradient id + NaN-safe ProgressRing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs in @bytelyst/data-viz@0.1.0 surfaced during a cross-repo audit: 1. Sparkline used `useMemo(() => \`bl-spark-${Math.random()...}\`)` for its id. Math.random() produces different values during Next.js SSR vs client hydration, triggering React hydration mismatches (and broken gradient refs on first paint). Swap for React's `useId()` — deterministic across server + client. Also: with a single-element series, `data.length > 0` was true but the early-return branch left `lastX=lastY=0`, painting a stale dot at the SVG origin. Tighten the guard to `data.length >= 2`. 2. ProgressRing's `Math.max(0, Math.min(1, value))` propagated NaN when callers passed `NaN` or `Infinity` (e.g. division-by-zero metrics) — producing "NaN percent" in the aria-label. Guard with `Number.isFinite` first. Regression tests cover all three cases — 17/17 passing. Tests: pnpm -F @bytelyst/data-viz test → 17 passed --- packages/data-viz/src/ProgressRing.tsx | 3 +- packages/data-viz/src/Sparkline.tsx | 15 +++--- .../data-viz/src/__tests__/data-viz.test.tsx | 47 +++++++++++++++---- 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/packages/data-viz/src/ProgressRing.tsx b/packages/data-viz/src/ProgressRing.tsx index d09ee59a..0bc9bc53 100644 --- a/packages/data-viz/src/ProgressRing.tsx +++ b/packages/data-viz/src/ProgressRing.tsx @@ -37,7 +37,8 @@ export function ProgressRing({ className, style, }: ProgressRingProps) { - const clamped = Math.max(0, Math.min(1, value)); + const safeValue = Number.isFinite(value) ? value : 0; + const clamped = Math.max(0, Math.min(1, safeValue)); const { radius, circumference, dashOffset } = useMemo(() => { const r = (size - thickness) / 2; const c = 2 * Math.PI * r; diff --git a/packages/data-viz/src/Sparkline.tsx b/packages/data-viz/src/Sparkline.tsx index bc89552e..df86381f 100644 --- a/packages/data-viz/src/Sparkline.tsx +++ b/packages/data-viz/src/Sparkline.tsx @@ -1,4 +1,4 @@ -import { useMemo, type CSSProperties } from 'react'; +import { useId, useMemo, type CSSProperties } from 'react'; export interface SparklineProps { /** Series values. Length 2+. */ @@ -54,9 +54,7 @@ export function Sparkline({ const y = pad + innerH - ((v - min) / range) * innerH; return [x, y] as const; }); - const path = pts - .map(([x, y], i) => (i === 0 ? `M${x},${y}` : `L${x},${y}`)) - .join(' '); + const path = pts.map(([x, y], i) => (i === 0 ? `M${x},${y}` : `L${x},${y}`)).join(' '); const areaPath = pts.length > 1 ? `${path} L${pts[pts.length - 1]![0]},${height} L${pts[0]![0]},${height} Z` @@ -65,10 +63,9 @@ export function Sparkline({ return { path, areaPath, lastX: lx, lastY: ly }; }, [data, width, height, strokeWidth]); - const gradId = useMemo( - () => `bl-spark-${Math.random().toString(36).slice(2, 8)}`, - [], - ); + // useId() is SSR-stable; using Math.random() here would mismatch on hydration. + const reactId = useId(); + const gradId = `bl-spark-${reactId.replace(/[^a-zA-Z0-9_-]/g, '')}`; return ( - {showLastPoint && data.length > 0 && ( + {showLastPoint && data.length >= 2 && ( { const path = screen.getByTestId('bl-sparkline').querySelector('path'); expect(path?.getAttribute('d')).toBe(''); }); + it('derives gradient id from React useId, not Math.random (SSR-safe)', () => { + // Regression: previously used Math.random() which caused Next.js + // hydration mismatches. React's useId emits ids shaped like ":r3:" / + // "_r_3_" — deterministic per render-position. We assert the id + // matches that shape (only the React-id chars), with no 6-hex random + // suffix. + const { container } = render(); + const id = container.querySelector('linearGradient')?.getAttribute('id') ?? ''; + expect(id).toMatch(/^bl-spark-[a-zA-Z0-9_-]+$/); + // Math.random().toString(36).slice(2, 8) always produced exactly 6 + // chars after `bl-spark-`. useId produces something different. + const suffix = id.replace(/^bl-spark-/, ''); + expect(/^[0-9a-f]{6}$/.test(suffix)).toBe(false); + }); + it('omits the last-point circle when given a single datum', () => { + render(); + // Single-point series has no meaningful trend, so no stale (0,0) circle. + expect(screen.getByTestId('bl-sparkline').querySelector('circle')).toBeNull(); + }); }); describe('BarSparkline', () => { @@ -61,15 +80,11 @@ describe('KpiCard', () => { it('goodWhen=lower inverts the success/danger interpretation', () => { // For latency: a negative delta (faster) is good. const { rerender } = render( - , + ); - expect( - screen.getByTestId('bl-kpi-delta').getAttribute('data-good'), - ).toBe('true'); + expect(screen.getByTestId('bl-kpi-delta').getAttribute('data-good')).toBe('true'); rerender(); - expect( - screen.getByTestId('bl-kpi-delta').getAttribute('data-good'), - ).toBe('false'); + expect(screen.getByTestId('bl-kpi-delta').getAttribute('data-good')).toBe('false'); }); it('renders sparkline when trend provided', () => { render(); @@ -85,11 +100,18 @@ describe('ProgressRing', () => { rerender(); expect(screen.getByTestId('bl-progress-ring').getAttribute('aria-label')).toBe('0 percent'); }); + it('treats NaN / non-finite value as 0 percent (no `NaN percent` aria-label)', () => { + render(); + expect(screen.getByTestId('bl-progress-ring').getAttribute('aria-label')).toBe('0 percent'); + cleanup(); + render(); + expect(screen.getByTestId('bl-progress-ring').getAttribute('aria-label')).toBe('0 percent'); + }); it('renders inner content slot', () => { render( 50% - , + ); expect(screen.getByTestId('center').textContent).toBe('50%'); }); @@ -103,7 +125,14 @@ describe('Heatmap', () => { expect(screen.getAllByTestId(/bl-heatmap-cell-/).length).toBe(14); }); it('zero-value cells render with empty intensity', () => { - render(); + render( + + ); const a = screen.getByTestId('bl-heatmap-cell-0'); const b = screen.getByTestId('bl-heatmap-cell-1'); expect(a.getAttribute('data-intensity')).toBe('0.00');