diff --git a/.changeset/y-axis-even-tick-spacing.md b/.changeset/y-axis-even-tick-spacing.md new file mode 100644 index 0000000000..7e6d8bfa1f --- /dev/null +++ b/.changeset/y-axis-even-tick-spacing.md @@ -0,0 +1,7 @@ +--- +'@hyperdx/app': patch +--- + +fix: keep Y-axis ticks evenly spaced and cleanly rounded + +A chart's Y-axis could render unevenly spaced or fractional ticks (e.g. `0, 300, 1k` instead of `0, 250, 500, 750, 1k`), or even show two ticks with the identical label. Ticks now round to clean, evenly-spaced, always-distinct values instead. diff --git a/packages/app/src/HDXMultiSeriesTimeChart.tsx b/packages/app/src/HDXMultiSeriesTimeChart.tsx index 6b86ef905c..d173b0a5b4 100644 --- a/packages/app/src/HDXMultiSeriesTimeChart.tsx +++ b/packages/app/src/HDXMultiSeriesTimeChart.tsx @@ -873,6 +873,407 @@ export function getYAxisTicks( return getTickValuesFixedDomain([min, max], 5, true); } +// Rounds off float dust (e.g. 3 * 1.05 giving 3.1500000000000004). +const cleanNumber = (v: number) => Number(v.toPrecision(12)); + +// A format with no decimals to spend forces every tick to an integer, +// where a 2.5x10^n step (n <= 0) rounds unevenly - see niceStepsNear. +function forcesIntegerTicks(axisNumberFormat?: NumberFormat): boolean { + if (!axisNumberFormat) return true; + if (axisNumberFormat.output === 'duration') return false; + return (axisNumberFormat.mantissa ?? 0) === 0; +} + +// Every 1/2/5 x10^n step, plus 2.5x10^n for n > 0 (integers like 250 are +// always safe; 2.5, 0.25, ... round unevenly with no decimals to spare). +function niceStepsNear( + target: number, + axisNumberFormat?: NumberFormat, +): number[] { + const exp = Math.floor(Math.log10(target)); + const excludeSmall2_5 = forcesIntegerTicks(axisNumberFormat); + return [exp - 1, exp, exp + 1] + .flatMap(e => { + const includeQuarterStep = e > 0 || !excludeSmall2_5; + const multipliers = includeQuarterStep + ? [1, 2, 2.5, 5, 10] + : [1, 2, 5, 10]; + return multipliers.map(m => m * 10 ** e); + }) + .filter(step => step > 0) + .sort((a, b) => a - b); +} + +// null means the step is unusable at this magnitude (see below), which +// getNiceYAxisTicks must treat as a rejection, not as a short tick list. +function ticksWithinRange( + step: number, + min: number, + max: number, +): number[] | null { + const ticks: number[] = []; + let t = cleanNumber(Math.ceil(min / step) * step); + while (t <= max + step * 1e-9) { + ticks.push(t); + const next = cleanNumber(t + step); + // At extreme magnitudes, float precision can make this step a no-op - + // reject it rather than accept a truncated, collapsed tick list. + if (next <= t) { + return null; + } + t = next; + } + return ticks; +} + +// Escalates precision past the configured mantissa, bypassing +// formatAxisTick's own selection (which can force 0 regardless). +function formatTickAtMantissa( + value: number, + axisNumberFormat: NumberFormat, + mantissa: number, +): string { + // Mirrors formatAxisTick's fixed-unit handling - re-adding the suffix it + // drops would make budget checks reject a label that never actually renders that wide. + if (isFixedNumericUnit(axisNumberFormat.numericUnit)) { + return trimTrailingZeros(value.toFixed(mantissa)); + } + return trimTrailingZeros( + formatNumber(value, { + ...axisNumberFormat, + mantissa, + average: true, + unit: undefined, + }), + ); +} + +// Ticks must never carry duplicate labels. +const MAX_TICK_MANTISSA_ESCALATION = 4; + +function isDistinct( + ticks: number[], + formatTick: (value: number) => string, +): boolean { + return new Set(ticks.map(formatTick)).size === ticks.length; +} + +// Reuses formatAxisTick's own per-label budget (axisLabelBudget already +// accounts for a space-separated unit suffix or a negative percent sign). +function fitsLabelBudget( + ticks: number[], + formatTick: (value: number) => string, +): boolean { + return ticks.every(t => { + const label = formatTick(t); + return label.length <= axisLabelBudget(label); + }); +} + +// Prefers formatAxisTick's normal output, but escalates precision past the +// configured mantissa when that's the only way to keep labels distinct. +function resolveDistinctTickLabels( + ticks: number[], + axisNumberFormat: NumberFormat | undefined, +): ((value: number) => string) | null { + const base = (value: number) => formatAxisTick(value, axisNumberFormat); + if (isDistinct(ticks, base)) { + return base; + } + if (!axisNumberFormat) { + // No configured mantissa to escalate - fall back to full, non-compact + // precision, which always distinguishes any two different numbers. + const fullPrecision = (value: number) => + new Intl.NumberFormat('en-US').format(value); + return isDistinct(ticks, fullPrecision) && + fitsLabelBudget(ticks, fullPrecision) + ? fullPrecision + : null; + } + if (axisNumberFormat.output === 'duration') { + // formatDurationMsCompact has no mantissa - escalate its own fixed + // 2-3 significant digits instead, past whichever unit it picks. + const factor = axisNumberFormat.factor ?? 1; + for (let p = 3; p <= 3 + MAX_TICK_MANTISSA_ESCALATION; p++) { + const escalated = (value: number) => + formatDurationMsCompact(value * factor * 1000, p); + if (isDistinct(ticks, escalated) && fitsLabelBudget(ticks, escalated)) { + return escalated; + } + } + return null; + } + // formatAxisTick can force mantissa down to 0 regardless of what's + // configured, so escalation must start from 1, not the configured value. + for (let m = 1; m <= MAX_TICK_MANTISSA_ESCALATION; m++) { + const escalated = (value: number) => + formatTickAtMantissa(value, axisNumberFormat, m); + if (isDistinct(ticks, escalated) && fitsLabelBudget(ticks, escalated)) { + return escalated; + } + } + return null; +} + +export interface NiceYAxisTicks { + ticks: number[]; + tickFormatter?: (value: number) => string; +} + +// Ported from packages/cli/src/termchart/scale.ts's niceTicks: the smallest +// step that fits within [min, max]/maxTicks and formats to distinct labels. +export function getNiceYAxisTicks( + min: number, + max: number, + maxTicks = 5, + axisNumberFormat?: NumberFormat, +): NiceYAxisTicks { + if (!Number.isFinite(min) || !Number.isFinite(max) || max <= min) { + return { ticks: [] }; + } + const steps = niceStepsNear((max - min) / (maxTicks - 1), axisNumberFormat); + for (const step of steps) { + const ticks = ticksWithinRange(step, min, max); + // A single tick conveys no scale at all - never accept it, even + // though its "labels" are trivially distinct from one another. + if (!ticks || ticks.length < 2 || ticks.length > maxTicks) { + continue; + } + const tickFormatter = resolveDistinctTickLabels(ticks, axisNumberFormat); + if (tickFormatter) { + return { ticks, tickFormatter }; + } + } + return { ticks: [] }; +} + +export interface ExpandableYAxisTicks { + max: number; + ticks: number[]; + tickFormatter?: (value: number) => string; +} + +// For the plain default branch (domain may expand, as [0,'auto'] did +// pre-PR): rounds the upper bound up to fill slack under maxTicks. +export function getExpandableYAxisTicks( + min: number, + max: number, + maxTicks = 5, + axisNumberFormat?: NumberFormat, +): ExpandableYAxisTicks { + if (!Number.isFinite(min) || !Number.isFinite(max) || max <= min) { + return { max, ticks: [] }; + } + const steps = niceStepsNear((max - min) / (maxTicks - 1), axisNumberFormat); + for (const step of steps) { + const tightTicks = ticksWithinRange(step, min, max); + // A single tick conveys no scale at all - never accept it, even + // though its "labels" are trivially distinct from one another. + if (!tightTicks || tightTicks.length < 2 || tightTicks.length > maxTicks) { + continue; + } + const tightFormatter = resolveDistinctTickLabels( + tightTicks, + axisNumberFormat, + ); + if (!tightFormatter) { + continue; + } + const expandedMax = cleanNumber(Math.ceil(max / step) * step); + // Filling a step's worth of dead space is fine, but not at the cost + // of a large fraction of the range - keep the tight result instead. + if (expandedMax - max <= (max - min) * 0.25) { + const expandedTicks = ticksWithinRange(step, min, expandedMax); + if (expandedTicks && expandedTicks.length <= maxTicks) { + const expandedFormatter = resolveDistinctTickLabels( + expandedTicks, + axisNumberFormat, + ); + if (expandedFormatter) { + return { + max: expandedMax, + ticks: expandedTicks, + tickFormatter: expandedFormatter, + }; + } + } + } + return { max, ticks: tightTicks, tickFormatter: tightFormatter }; + } + return { max, ticks: [] }; +} + +// Shared by every yAxisDomain branch below. Callers pass only the series +// actually drawn (already selection- and HARD_LINES_LIMIT-filtered). +export function scanYAxisValueRange( + graphResults: any[], + lineData: LineData[], +): { min: number; max: number } { + let min = Infinity; + let max = -Infinity; + graphResults.forEach(dataPoint => { + lineData.forEach(ld => { + const value = dataPoint[ld.dataKey]; + if (typeof value === 'number' && !isNaN(value)) { + min = Math.min(min, value); + max = Math.max(max, value); + } + }); + }); + return { min, max }; +} + +export interface YAxisBounds { + domain: AxisDomain; + ticks: number[] | undefined; + tickFormatter?: (value: number) => string; +} + +// Stable identity for the referenceLineValues default - a fresh `[]` literal +// as a default prop value defeats memoization and can loop React's compiler. +const EMPTY_REFERENCE_LINE_VALUES: number[] = []; + +const DEFAULT_Y_AXIS_BOUNDS: YAxisBounds = { + domain: [0, 'auto'], + ticks: undefined, +}; +const FIT_Y_AXIS_BOUNDS: YAxisBounds = { + domain: ['auto', 'auto'], + ticks: undefined, +}; + +// A stacked bar's rendered height sums its series at each timestamp - leave +// that entirely to Recharts, regardless of selection/fit-to-data state. +export function computeYAxisBounds( + graphResults: any[], + visibleLineData: LineData[], + hasSelection: boolean, + fitYAxisToData: boolean, + displayType: DisplayType, + referenceLineValues: number[], + axisNumberFormat?: NumberFormat, +): YAxisBounds { + if (displayType === DisplayType.StackedBar) { + return DEFAULT_Y_AXIS_BOUNDS; + } + const shouldFitYAxis = fitYAxisToData; + + if (!hasSelection && !shouldFitYAxis) { + // A fully numeric domain skips Recharts' own nice rounding, and a + // reference line can extend it further - defer to Recharts entirely. + if (referenceLineValues.length > 0) { + return DEFAULT_Y_AXIS_BOUNDS; + } + const { min, max } = scanYAxisValueRange(graphResults, visibleLineData); + if (max === -Infinity) { + return DEFAULT_Y_AXIS_BOUNDS; + } + // Recharts widens an explicit domain to fit out-of-range data, so + // negative data must be reflected here, not just pinned at zero. + const lowerBound = cleanNumber(Math.min(0, min)); + // max * 1.05 would shrink the upper bound below max for negative data; + // padding away from zero keeps headroom regardless of max's sign. + const upperBound = cleanNumber(max + Math.abs(max) * 0.05); + if (upperBound <= lowerBound) { + return DEFAULT_Y_AXIS_BOUNDS; + } + const expanded = getExpandableYAxisTicks( + lowerBound, + upperBound, + 5, + axisNumberFormat, + ); + // No nice step fits - fall back to getYAxisTicks' reduce-tick-count + // dedup instead of Recharts' raw, collision-prone default. + if (expanded.ticks.length === 0) { + const baseFormat = (value: number) => + formatAxisTick(value, axisNumberFormat); + return { + domain: [lowerBound, upperBound], + ticks: getYAxisTicks(lowerBound, upperBound, baseFormat), + tickFormatter: baseFormat, + }; + } + return { + domain: [lowerBound, expanded.max], + ticks: expanded.ticks, + tickFormatter: expanded.tickFormatter, + }; + } + + // A selection with fit-to-data off still keeps the zero-pinned fallback, + // not the unpinned fit fallback - only fitting itself opts out of it. + const degenerateFallback = shouldFitYAxis + ? FIT_Y_AXIS_BOUNDS + : DEFAULT_Y_AXIS_BOUNDS; + const { min, max } = scanYAxisValueRange(graphResults, visibleLineData); + if (min === Infinity || max === -Infinity) { + return degenerateFallback; + } + const padding = (max - min) * 0.05; + // Recharts widens the domain to actual negative data regardless of fit + // mode, so the lower bound must follow it whenever min itself is negative. + const lowerBound = cleanNumber( + min < 0 ? min - padding : Math.max(0, min - padding), + ); + const upperBound = cleanNumber(max + padding); + if (upperBound <= lowerBound) { + return degenerateFallback; + } + // A reference line can widen the domain (extendDomain) - extend it up + // front and nice-step the result, instead of ticking a stale domain. + if (referenceLineValues.length > 0) { + const extendedLower = cleanNumber( + Math.min(lowerBound, ...referenceLineValues), + ); + const extendedUpper = cleanNumber( + Math.max(upperBound, ...referenceLineValues), + ); + const expanded = getExpandableYAxisTicks( + extendedLower, + extendedUpper, + 5, + axisNumberFormat, + ); + if (expanded.ticks.length > 0) { + return { + domain: [extendedLower, expanded.max], + ticks: expanded.ticks, + tickFormatter: expanded.tickFormatter, + }; + } + const baseFormat = (value: number) => + formatAxisTick(value, axisNumberFormat); + return { + domain: [extendedLower, extendedUpper], + ticks: getYAxisTicks(extendedLower, extendedUpper, baseFormat), + tickFormatter: baseFormat, + }; + } + const { ticks, tickFormatter } = getNiceYAxisTicks( + lowerBound, + upperBound, + 5, + axisNumberFormat, + ); + // Same fallback as the default branch above - reduce tick count via + // getYAxisTicks rather than leaving this to Recharts' raw default. + if (ticks.length === 0) { + const baseFormat = (value: number) => + formatAxisTick(value, axisNumberFormat); + return { + domain: [lowerBound, upperBound], + ticks: getYAxisTicks(lowerBound, upperBound, baseFormat), + tickFormatter: baseFormat, + }; + } + return { + domain: [lowerBound, upperBound], + ticks, + tickFormatter, + }; +} + export const MemoChart = memo(function MemoChart({ graphResults, setIsClickActive, @@ -881,6 +1282,7 @@ export const MemoChart = memo(function MemoChart({ dateRange, lineData, referenceLines, + referenceLineValues = EMPTY_REFERENCE_LINE_VALUES, annotations, logReferenceTimestamp, displayType = DisplayType.Line, @@ -911,6 +1313,9 @@ export const MemoChart = memo(function MemoChart({ dateRange: [Date, Date] | Readonly<[Date, Date]>; lineData: LineData[]; referenceLines?: React.ReactNode; + // Raw numeric value(s) backing referenceLines (pre-rendered JSX the axis + // math can't read), used to size the Y-axis domain around them. + referenceLineValues?: number[]; /** * Event markers (alerts, releases, …) drawn as dashed vertical lines with a * label above. Passed as data rather than pre-rendered elements so the chart @@ -1051,64 +1456,28 @@ export const MemoChart = memo(function MemoChart({ ); }, [nearestSeriesKey, visibleLineData.length, id]); - const yAxisDomain: AxisDomain = useMemo(() => { - const hasSelection = hasSeriesSelection(selectedSeriesNames); - - // Fitting the y-axis lower bound to the data only applies to line charts. - // Bar charts are always anchored at zero so the bar lengths stay - // proportional to their values. - const shouldFitYAxis = - fitYAxisToData && displayType !== DisplayType.StackedBar; - - // The data min/max is only needed to either zoom into a selection or to - // fit the lower bound to the data. When neither applies, let Recharts - // auto-calculate the upper bound while pinning the lower bound to zero. - if (!hasSelection && !shouldFitYAxis) { - return [0, 'auto']; - } - - // Calculate domain based on visible series (all series when there's no - // explicit selection). - let minValue = Infinity; - let maxValue = -Infinity; - - graphResults.forEach(dataPoint => { - lineData.forEach(ld => { - const seriesName = ld.displayName || ld.dataKey; - // Only consider visible series - if (!hasSelection || selectedSeriesNames.has(seriesName)) { - const value = dataPoint[ld.dataKey]; - if (typeof value === 'number' && !isNaN(value)) { - minValue = Math.min(minValue, value); - maxValue = Math.max(maxValue, value); - } - } - }); - }); - - // If we found valid values, return them with some padding - if (minValue !== Infinity && maxValue !== -Infinity) { - const padding = (maxValue - minValue) * 0.05; // 5% padding - // When fitting to data, allow the lower bound to follow the data - // minimum; otherwise keep it pinned at zero. The 5% padding must not - // drag the axis below zero unless the data itself is negative, so - // clamp at zero whenever the minimum is non-negative. - const lowerBound = - shouldFitYAxis && minValue < 0 - ? minValue - padding - : Math.max(0, minValue - padding); - const upperBound = maxValue + padding; - return [lowerBound, upperBound]; - } - - return ['auto', 'auto']; - }, [ - graphResults, - lineData, - selectedSeriesNames, - fitYAxisToData, - displayType, - ]); + const yAxisBounds = useMemo( + () => + computeYAxisBounds( + graphResults, + visibleLineData, + hasSeriesSelection(selectedSeriesNames), + fitYAxisToData, + displayType, + referenceLineValues, + axisNumberFormat, + ), + [ + graphResults, + visibleLineData, + selectedSeriesNames, + fitYAxisToData, + displayType, + referenceLineValues, + axisNumberFormat, + ], + ); + const yAxisDomain = yAxisBounds.domain; const [containerWidth, setContainerWidth] = useState(0); @@ -1226,15 +1595,9 @@ export const MemoChart = memo(function MemoChart({ [axisNumberFormat], ); - // Only overrides Recharts' own tick generation when the domain is a - // concrete range - a bare 'auto' bound has no fixed interval to work from. - const yAxisTicks = useMemo(() => { - const [min, max] = yAxisDomain; - if (typeof min !== 'number' || typeof max !== 'number') { - return undefined; - } - return getYAxisTicks(min, max, tickFormatter); - }, [yAxisDomain, tickFormatter]); + const yAxisTicks = yAxisBounds.ticks; + // computeYAxisBounds already resolves ticks+formatter together per domain. + const yAxisTickFormatter = yAxisBounds.tickFormatter ?? tickFormatter; const [highlightStart, setHighlightStart] = useState(); const [highlightEnd, setHighlightEnd] = useState(); @@ -1686,7 +2049,7 @@ export const MemoChart = memo(function MemoChart({ { }); }); +describe('getNiceYAxisTicks', () => { + it('rounds to a clean step instead of dividing the raw range', () => { + // Regression: evenly dividing an unrounded range into quarters gave + // fractional labels (341/683/1k) or, worse, duplicate rounded ones. + expect(getNiceYAxisTicks(0, 1000).ticks).toEqual([0, 250, 500, 750, 1000]); + }); + + it('works for a non-zero lower bound', () => { + expect(getNiceYAxisTicks(10, 30).ticks).toEqual([10, 15, 20, 25, 30]); + }); + + it('never expands past the given range, even if it clips ticks short', () => { + // A tight/fitted range shouldn't gain extra ticks beyond its own bounds. + expect(getNiceYAxisTicks(95, 205).ticks).toEqual([100, 125, 150, 175, 200]); + }); + + it('returns no ticks for a degenerate (flat) range', () => { + expect(getNiceYAxisTicks(5, 5).ticks).toEqual([]); + }); + + it('cleans up float dust from a fractional step', () => { + // Regression: without rounding, a 0.1 step gave 0.30000000000000004. + expect(getNiceYAxisTicks(0, 0.42).ticks).toEqual([0, 0.1, 0.2, 0.3, 0.4]); + }); + + it('never returns more than maxTicks ticks', () => { + // Regression: rounding to the nearest step could pick one below the + // raw target, overflowing to 6 ticks here instead of capping at 5. + expect(getNiceYAxisTicks(0, 1480).ticks.length).toBeLessThanOrEqual(5); + }); + + it('rejects a step whose only distinct formatting overflows the label budget', () => { + // This domain needs 17-char labels to distinguish at all - no step + // fits the budget and stays distinct, so this returns no ticks. + expect(getNiceYAxisTicks(999999999999, 1000000000030).ticks).toEqual([]); + }); + + it('avoids a step that formatAxisTick would round unevenly (12.5 -> "13")', () => { + // At this magnitude (exp === 0), a 2.5 step yields non-integer ticks + // that straddle formatAxisTick's forced-integer threshold of 10. + expect(getNiceYAxisTicks(0, 12.6).ticks).toEqual([0, 5, 10]); + }); + + it('never accepts a single tick, even though it is trivially "distinct"', () => { + // Regression: a lone survivor's label can't collide with anything, + // so a naive distinctness check accepted a one-tick, no-scale axis. + const result = getNiceYAxisTicks(999999999999, 1000000000000.001); + expect(result.ticks.length === 0 || result.ticks.length >= 2).toBe(true); + }); + + it('escalates precision past the configured mantissa to keep labels distinct', () => { + // Regression: two ticks must never show the same label - 0 decimals + // collapses a narrow byte range entirely to "3 GB". + const GB = 1024 ** 3; + const result = getNiceYAxisTicks(2.825 * GB, 3.45 * GB, 5, { + output: 'byte', + mantissa: 0, + }); + const labels = result.ticks.map(t => result.tickFormatter!(t)); + expect(result.ticks.length).toBeGreaterThanOrEqual(2); + expect(new Set(labels).size).toBe(labels.length); + }); + + it('falls back to full, non-compact precision when no format is configured', () => { + // Regression: with no mantissa to escalate, close-together magnitudes + // collapsed under Intl's compact notation (e.g. "1T") instead. + const result = getNiceYAxisTicks(999999999999, 1000000000030); + const labels = result.ticks.map(t => result.tickFormatter!(t)); + expect(new Set(labels).size).toBe(labels.length); + }); + + it('escalates from the minimal precision, not past what distinctness needs', () => { + // Regression: starting escalation above 1 (e.g. at "configured+1") + // skipped a sufficient mantissa, producing needlessly wide labels. + const result = getExpandableYAxisTicks(0, 2100, 5, { output: 'number' }); + expect(result.ticks.map(t => result.tickFormatter!(t))).toEqual([ + '0', + '0.5k', + '1k', + '1.5k', + '2k', + ]); + }); + + it('escalates a duration format past its fixed significant-digit count', () => { + // Regression: duration collisions returned null unconditionally, + // discarding the tick list instead of trying more precision. + const result = getNiceYAxisTicks(3600, 3700, 5, { output: 'duration' }); + expect(result.tickFormatter).toBeDefined(); + const labels = result.ticks.map(t => result.tickFormatter!(t)); + expect(new Set(labels).size).toBe(labels.length); + }); + + it('rejects a plain-number escalation that would overflow the axis label budget', () => { + // Regression: mantissa 3 was needed for distinctness but rendered + // "1.0500k" (7 chars) - must be rejected in favor of a coarser step. + const result = getExpandableYAxisTicks(1050, 1050.315, 5, { + output: 'number', + mantissa: 3, + }); + const labels = result.ticks.map(t => result.tickFormatter!(t)); + expect(labels.every(l => l.length <= 5)).toBe(true); + }); + + it('escalates a fixed-unit axis without reintroducing the suffix it drops', () => { + // Regression: escalation called formatNumber directly, restoring the + // "cps" suffix formatAxisTick strips - overflowing the label budget. + const result = getExpandableYAxisTicks(1.15, 1.150115, 5, { + output: 'throughput', + numericUnit: NumericUnit.Cps, + mantissa: 2, + }); + expect(result.ticks.map(t => result.tickFormatter!(t))).toEqual([ + '1.1', + '1.2', + ]); + }); + + it('rejects a full-precision fallback that overflows the label budget', () => { + // Regression: no-format fallback had no budget check, rendering + // 9-char grouped numbers instead of falling through to a coarser step. + const result = getNiceYAxisTicks(1200000, 1250000); + const labels = result.ticks.map(t => result.tickFormatter!(t)); + expect(labels.every(l => l.length <= 5)).toBe(true); + expect(new Set(labels).size).toBe(labels.length); + }); + + it('excludes a 2.5-scaled step below 1 only when the format forces integers', () => { + // Regression: step 0.25 (2.5x10^-1) survived the bare-2.5 filter, + // rounding to an uneven 0.3/0.2/0.3/0.2 progression at mantissa 0. + const forcedInteger = getNiceYAxisTicks(0, 1.1, 5, { + output: 'number', + mantissa: 0, + }); + expect(forcedInteger.ticks).toEqual([0, 0.5, 1]); + + // With decimals to spend, 0.25 renders exactly - keep the denser step. + const withDecimals = getNiceYAxisTicks(0, 1.1, 5, { + output: 'number', + mantissa: 2, + }); + expect(withDecimals.ticks).toEqual([0, 0.25, 0.5, 0.75, 1]); + }); +}); + +describe('scanYAxisValueRange', () => { + const series = (dataKey: string): LineData => ({ + dataKey, + currentPeriodKey: dataKey, + previousPeriodKey: `${dataKey}.prev`, + displayName: dataKey, + valueColumnName: dataKey, + color: '#a', + }); + + it('finds the min/max across every series it is given', () => { + // Callers pass only the drawn series (already selection- and + // HARD_LINES_LIMIT-filtered), so no visibility filtering happens here. + const lineData = [series('a'), series('b')]; + const graphResults = [{ a: 10, b: 40 }, { a: -5 }]; + expect(scanYAxisValueRange(graphResults, lineData)).toEqual({ + min: -5, + max: 40, + }); + }); +}); + +describe('computeYAxisBounds', () => { + const series = (dataKey: string): LineData => ({ + dataKey, + currentPeriodKey: dataKey, + previousPeriodKey: `${dataKey}.prev`, + displayName: dataKey, + valueColumnName: dataKey, + color: '#a', + }); + + it('rounds ticks to clean, non-duplicating values', () => { + // A peak of 3 divided into raw quarters rounds (mantissa 0) to + // 0/1/2/2/3 - two ticks reading "2" at different heights. + const bounds = computeYAxisBounds( + [{ a: 3 }], + [series('a')], + false, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ + domain: [0, 3.15], + ticks: [0, 1, 2, 3], + tickFormatter: expect.any(Function), + }); + }); + + it('never picks a step that formatAxisTick would round unevenly', () => { + // Regression: a 2.5 step at this magnitude gives "12.5", which + // formatAxisTick rounds to "13" - an uneven-looking progression. + const bounds = computeYAxisBounds( + [{ a: 12 }], + [series('a')], + false, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ + domain: [0, 15], + ticks: [0, 5, 10, 15], + tickFormatter: expect.any(Function), + }); + }); + + it('expands past padding to avoid a sparse, dead-space-heavy axis', () => { + // Regression: a step landing between two step levels left only 3 + // ticks (0/500/1000) over domain [0,1260], 20% dead space at the top. + const bounds = computeYAxisBounds( + [{ a: 1200 }], + [series('a')], + false, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ + domain: [0, 1500], + ticks: [0, 500, 1000, 1500], + tickFormatter: expect.any(Function), + }); + }); + + it('extends the zero-pinned domain to cover negative data too', () => { + // Regression: Recharts widens a [0, upperBound] domain to fit negative + // data, stranding a domain-only tick list in a sliver at the top. + const bounds = computeYAxisBounds( + [{ a: -100 }, { a: 1 }], + [series('a')], + false, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ + domain: [-100, 1.05], + ticks: [-100, -75, -50, -25, 0], + tickFormatter: expect.any(Function), + }); + }); + + it('pads an all-negative series away from zero, not toward it', () => { + // Regression: max * 1.05 made the upper bound *more* negative than max + // itself (-900 * 1.05 = -945), excluding the data's own maximum. + const bounds = computeYAxisBounds( + [{ a: -1000 }, { a: -900 }], + [series('a')], + false, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ + domain: [-1000, -850], + ticks: [-1000, -950, -900, -850], + tickFormatter: expect.any(Function), + }); + }); + + it('defers entirely to Recharts when a reference line is present', () => { + // A fully numeric domain reproduces the exact uneven spacing this PR + // fixes, and a reference line can extend it further - fall back to auto. + const bounds = computeYAxisBounds( + [{ a: 1000 }], + [series('a')], + false, + false, + DisplayType.Line, + [1500], + ); + expect(bounds).toEqual({ domain: [0, 'auto'], ticks: undefined }); + }); + + it('leaves a stacked bar`s max to Recharts, selection or not', () => { + // Regression: 60 and 40 individually reach a stack height of 100, not + // 63 - the guard must apply regardless of hasSelection. + const lineData = [series('a'), series('b')]; + const graphResults = [{ a: 60, b: 40 }]; + for (const hasSelection of [false, true]) { + expect( + computeYAxisBounds( + graphResults, + lineData, + hasSelection, + false, + DisplayType.StackedBar, + [], + ), + ).toEqual({ domain: [0, 'auto'], ticks: undefined }); + } + }); + + it('falls back to auto when there is no numeric data', () => { + const bounds = computeYAxisBounds( + [], + [], + false, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ domain: [0, 'auto'], ticks: undefined }); + }); + + it('falls back to auto for flat data instead of a degenerate domain', () => { + // Regression: an all-zero domain ([0, 0]) collapsed the axis to one point. + const bounds = computeYAxisBounds( + [{ a: 0 }, { a: 0 }], + [series('a')], + false, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ domain: [0, 'auto'], ticks: undefined }); + }); + + it('falls back to auto for flat negative data on a fitted axis', () => { + // Regression: the same guard could also invert the domain (e.g. + // [0, -5]) for all-negative data padded past zero. + const bounds = computeYAxisBounds( + [{ a: -50 }, { a: -50 }], + [series('a')], + false, + true, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ domain: ['auto', 'auto'], ticks: undefined }); + }); + + it('keeps the zero-pinned fallback for flat selected data when not fitting', () => { + // Regression: this case used the fit fallback (`['auto','auto']`) + // even though only fit-to-data, not selection, should opt out of it. + const bounds = computeYAxisBounds( + [{ a: 100 }, { a: 100 }], + [series('a')], + true, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ domain: [0, 'auto'], ticks: undefined }); + }); + + it('lets a fitted axis follow a negative minimum without expanding the domain', () => { + const bounds = computeYAxisBounds( + [{ a: -50 }, { a: 200 }], + [series('a')], + false, + true, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ + domain: [-62.5, 212.5], + ticks: [0, 100, 200], + tickFormatter: expect.any(Function), + }); + }); + + it('extends a selection`s lower bound to negative data even without fitting', () => { + // Regression: `Math.max(0, ...)` pinned this at 0 even though min < 0, + // but Recharts widens the domain to the real minimum regardless. + const bounds = computeYAxisBounds( + [{ a: -100 }, { a: 100 }], + [series('a')], + true, + false, + DisplayType.Line, + [], + ); + expect(bounds).toEqual({ + domain: [-110, 110], + ticks: [-100, -50, 0, 50, 100], + tickFormatter: expect.any(Function), + }); + }); + + it('nice-steps normally when a reference line already sits inside the range', () => { + // Regression: bailing to ticks: undefined dropped the duplicate-label + // dedup an alert chart (always has a reference line) used to get. + const bounds = computeYAxisBounds( + [{ a: 100 }, { a: 200 }], + [series('a')], + false, + true, + DisplayType.Line, + [150], + ); + expect(bounds).toEqual({ + domain: [95, 205], + ticks: [100, 125, 150, 175, 200], + tickFormatter: expect.any(Function), + }); + }); + + it('extends the domain to include an out-of-range reference line, then nice-steps it', () => { + // A threshold beyond the data (e.g. Alerts.tsx's ifOverflow="extendDomain") + // widens the rendered domain - ticking it up front avoids stale ticks. + const bounds = computeYAxisBounds( + [{ a: 100 }, { a: 200 }], + [series('a')], + false, + true, + DisplayType.Line, + [300], + ); + expect(bounds).toEqual({ + domain: [95, 300], + ticks: [100, 150, 200, 250, 300], + tickFormatter: expect.any(Function), + }); + }); + + it('escalates precision instead of dropping a step whose labels collide', () => { + // Regression: step 500's "0,500,1k,2k,2k" labels collide - escalating + // precision keeps the denser step instead of falling back to a coarser one. + const bounds = computeYAxisBounds( + [{ a: 2000 }], + [series('a')], + false, + false, + DisplayType.Line, + [], + { output: 'number' }, + ); + expect(bounds.domain).toEqual([0, 2100]); + expect(bounds.ticks).toEqual([0, 500, 1000, 1500, 2000]); + const labels = bounds.ticks!.map(t => bounds.tickFormatter!(t)); + expect(new Set(labels).size).toBe(labels.length); + }); + + it('never collapses to a single tick, escalating precision or a wider step instead', () => { + // Regression: a narrow byte range at 0 decimals (2.9-3.4GB) all rounds + // to "3 GB" - never show duplicate ticks or a no-scale single tick. + const GB = 1024 ** 3; + const bounds = computeYAxisBounds( + [{ a: 2.9 * GB }, { a: 3.4 * GB }], + [series('a')], + false, + true, + DisplayType.Line, + [], + { output: 'byte', mantissa: 0 }, + ); + expect(bounds.ticks).toBeDefined(); + expect(bounds.ticks!.length).toBeGreaterThanOrEqual(2); + const labels = bounds.ticks!.map(t => bounds.tickFormatter!(t)); + expect(new Set(labels).size).toBe(labels.length); + }); + + it('falls back to getYAxisTicks when no nice step fits, instead of a raw duplicate-prone domain', () => { + // Regression: no nice step fit this range, leaving ticks undefined for + // Recharts' raw domain-division default - which collides. + const GB = 1024 ** 3; + const bounds = computeYAxisBounds( + [{ a: 3.269 * GB }, { a: 3.511 * GB }], + [series('a')], + false, + true, + DisplayType.Line, + [], + { output: 'byte', mantissa: 1 }, + ); + expect(bounds.ticks).toBeDefined(); + expect(bounds.ticks!.length).toBeGreaterThanOrEqual(2); + const labels = bounds.ticks!.map(t => bounds.tickFormatter!(t)); + expect(new Set(labels).size).toBe(labels.length); + }); +}); + describe('getYAxisTicks', () => { it('resolves the reported duplicate-tick regression with evenly spaced ticks', () => { // 5 ticks collide ("3.3/3.3/3.4/3.5/3.5 GB"); dropping duplicates would diff --git a/packages/app/src/components/AlertPreviewChart.tsx b/packages/app/src/components/AlertPreviewChart.tsx index 03a96990f2..1040c6f295 100644 --- a/packages/app/src/components/AlertPreviewChart.tsx +++ b/packages/app/src/components/AlertPreviewChart.tsx @@ -23,7 +23,7 @@ import { DBTimeChart } from '@/components/DBTimeChart'; import { useAliasMapFromChartConfig } from '@/hooks/useChartConfig'; import { intervalToDateRange, intervalToGranularity } from '@/utils/alerts'; -import { getAlertReferenceLines } from './Alerts'; +import { getAlertReferenceLines, getAlertReferenceLineValues } from './Alerts'; type AlertPreviewChartProps = { source: TSource; @@ -119,6 +119,11 @@ export const AlertPreviewChart = ({ }), [threshold, thresholdMax, thresholdType], ); + const referenceLineValues = useMemo( + () => + getAlertReferenceLineValues({ threshold, thresholdMax, thresholdType }), + [threshold, thresholdMax, thresholdType], + ); return ( @@ -128,6 +133,7 @@ export const AlertPreviewChart = ({ showMVOptimizationIndicator={false} showDateRangeIndicator={false} referenceLines={referenceLines} + referenceLineValues={referenceLineValues} annotations={annotations} config={config} /> diff --git a/packages/app/src/components/Alerts.tsx b/packages/app/src/components/Alerts.tsx index e0e1a82aa7..360d797fb8 100644 --- a/packages/app/src/components/Alerts.tsx +++ b/packages/app/src/components/Alerts.tsx @@ -256,3 +256,33 @@ export const getAlertReferenceLines = ({ /> ); }; + +// Mirrors getAlertReferenceLines' own branching, so the values used to size +// the Y-axis domain can never drift from what's actually drawn as a line. +export const getAlertReferenceLineValues = ({ + thresholdType, + threshold, + thresholdMax, +}: { + thresholdType: AlertThresholdType; + threshold: number; + thresholdMax?: number; +}): number[] => { + if (threshold == null) { + return []; + } + if ( + (thresholdType === AlertThresholdType.BETWEEN || + thresholdType === AlertThresholdType.NOT_BETWEEN) && + thresholdMax != null + ) { + return [threshold, thresholdMax]; + } + if ( + thresholdType === AlertThresholdType.BELOW || + thresholdType === AlertThresholdType.BELOW_OR_EQUAL + ) { + return [0, threshold]; + } + return [threshold]; +}; diff --git a/packages/app/src/components/DBEditTimeChartForm/ChartPreviewPanel.tsx b/packages/app/src/components/DBEditTimeChartForm/ChartPreviewPanel.tsx index 60e7a7b5fd..2f352a138a 100644 --- a/packages/app/src/components/DBEditTimeChartForm/ChartPreviewPanel.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/ChartPreviewPanel.tsx @@ -15,7 +15,10 @@ import { IconList } from '@tabler/icons-react'; import { SortingState } from '@tanstack/react-table'; import { buildTableRowSearchUrl } from '@/ChartUtils'; -import { getAlertReferenceLines } from '@/components/Alerts'; +import { + getAlertReferenceLines, + getAlertReferenceLineValues, +} from '@/components/Alerts'; import { ChartEditorFormState } from '@/components/ChartEditor/types'; import ChartSQLPreview from '@/components/ChartSQLPreview'; import { DBBarChart } from '@/components/DBBarChart'; @@ -209,6 +212,18 @@ export function ChartPreviewPanel({ [queriedConfig, tableSource, dateRange, queryReady], ); + const referenceLineValues = useMemo( + () => + alert + ? getAlertReferenceLineValues({ + threshold: alert.threshold, + thresholdMax: alert.thresholdMax, + thresholdType: alert.thresholdType, + }) + : undefined, + [alert], + ); + return ( <> {isBlockedByRequiredFilters ? ( @@ -263,6 +278,7 @@ export function ChartPreviewPanel({ thresholdType: alert.thresholdType, }) } + referenceLineValues={referenceLineValues} errorVariant="inline" showMVOptimizationIndicator={false} // Preview doesn't need the MV indicators; disabling both lets diff --git a/packages/app/src/components/DBTimeChart.tsx b/packages/app/src/components/DBTimeChart.tsx index 1ae67bcbb8..6cd84278b9 100644 --- a/packages/app/src/components/DBTimeChart.tsx +++ b/packages/app/src/components/DBTimeChart.tsx @@ -304,6 +304,8 @@ type DBTimeChartComponentProps = { onTimeRangeSelect?: (start: Date, end: Date) => void; queryKeyPrefix?: string; referenceLines?: React.ReactNode; + /** Raw numeric value(s) backing referenceLines, for Y-axis domain sizing. */ + referenceLineValues?: number[]; /** Event markers (e.g. alert firing/recovery) drawn as dashed lines with labels. */ annotations?: ChartAnnotation[]; setDisplayType?: (type: DisplayType) => void; @@ -339,6 +341,7 @@ function DBTimeChartComponent({ onTimeRangeSelect, queryKeyPrefix, referenceLines, + referenceLineValues, annotations, setDisplayType, showDisplaySwitcher = true, @@ -1035,6 +1038,7 @@ function DBTimeChartComponent({ tooltipNumberFormatsByKey={formatByColumn} onTimeRangeSelect={onTimeRangeSelect} referenceLines={referenceLines} + referenceLineValues={referenceLineValues} annotations={annotations} setIsClickActive={setPinnedPayload} refreshClickActive={refreshPinnedPayload} diff --git a/packages/app/src/components/__tests__/Alerts.test.ts b/packages/app/src/components/__tests__/Alerts.test.ts new file mode 100644 index 0000000000..5ce1772ce5 --- /dev/null +++ b/packages/app/src/components/__tests__/Alerts.test.ts @@ -0,0 +1,80 @@ +import { AlertThresholdType } from '@hyperdx/common-utils/dist/types'; + +import { getAlertReferenceLineValues } from '@/components/Alerts'; + +describe('getAlertReferenceLineValues', () => { + it('returns only threshold for a simple (non-range) alert type', () => { + expect( + getAlertReferenceLineValues({ + threshold: 50, + thresholdMax: undefined, + thresholdType: AlertThresholdType.ABOVE, + }), + ).toEqual([50]); + }); + + it('ignores a stale thresholdMax left over from a since-changed alert type', () => { + // Regression: getAlertReferenceLines only draws thresholdMax for + // BETWEEN/NOT_BETWEEN - a leftover value here would silently widen the + // Y-axis domain around a line that's never actually rendered. + expect( + getAlertReferenceLineValues({ + threshold: 50, + thresholdMax: 500, + thresholdType: AlertThresholdType.ABOVE, + }), + ).toEqual([50]); + }); + + it('includes both values for BETWEEN, matching the rendered reference area', () => { + expect( + getAlertReferenceLineValues({ + threshold: 10, + thresholdMax: 20, + thresholdType: AlertThresholdType.BETWEEN, + }), + ).toEqual([10, 20]); + }); + + it('includes both values for NOT_BETWEEN, matching the rendered reference areas', () => { + expect( + getAlertReferenceLineValues({ + threshold: 10, + thresholdMax: 20, + thresholdType: AlertThresholdType.NOT_BETWEEN, + }), + ).toEqual([10, 20]); + }); + + it('drops thresholdMax for BETWEEN when it is unset', () => { + expect( + getAlertReferenceLineValues({ + threshold: 10, + thresholdMax: undefined, + thresholdType: AlertThresholdType.BETWEEN, + }), + ).toEqual([10]); + }); + + it("includes 0 for BELOW, matching the reference area's fixed bottom edge", () => { + // Regression: getAlertReferenceLines renders BELOW/BELOW_OR_EQUAL with + // y1={0} - omitting 0 here let Recharts silently widen the domain later. + expect( + getAlertReferenceLineValues({ + threshold: 550, + thresholdMax: undefined, + thresholdType: AlertThresholdType.BELOW, + }), + ).toEqual([0, 550]); + }); + + it("includes 0 for BELOW_OR_EQUAL, matching the reference area's fixed bottom edge", () => { + expect( + getAlertReferenceLineValues({ + threshold: 550, + thresholdMax: undefined, + thresholdType: AlertThresholdType.BELOW_OR_EQUAL, + }), + ).toEqual([0, 550]); + }); +}); diff --git a/packages/app/src/components/alerts/AlertDetailChart.tsx b/packages/app/src/components/alerts/AlertDetailChart.tsx index 31e9ef48ce..60608d154e 100644 --- a/packages/app/src/components/alerts/AlertDetailChart.tsx +++ b/packages/app/src/components/alerts/AlertDetailChart.tsx @@ -25,7 +25,10 @@ import { import { Anchor, Center, Paper, Skeleton, Text } from '@mantine/core'; import { AlertPreviewChart } from '@/components/AlertPreviewChart'; -import { getAlertReferenceLines } from '@/components/Alerts'; +import { + getAlertReferenceLines, + getAlertReferenceLineValues, +} from '@/components/Alerts'; import { DBTimeChart } from '@/components/DBTimeChart'; import { useDashboards } from '@/dashboard'; import { useAlertAnnotations } from '@/hooks/useAlertAnnotations'; @@ -244,7 +247,7 @@ const SINGLE_VALUE_RAW_SQL_MESSAGE = 'This alert runs a raw SQL query that returns one value per window, so it has no chart over time.'; function useAlertReferenceLines(alert: AlertsPageItem) { - return React.useMemo( + const referenceLines = React.useMemo( () => getAlertReferenceLines({ threshold: alert.threshold, @@ -253,6 +256,16 @@ function useAlertReferenceLines(alert: AlertsPageItem) { }), [alert.threshold, alert.thresholdMax, alert.thresholdType], ); + const referenceLineValues = React.useMemo( + () => + getAlertReferenceLineValues({ + threshold: alert.threshold, + thresholdMax: alert.thresholdMax, + thresholdType: alert.thresholdType, + }), + [alert.threshold, alert.thresholdMax, alert.thresholdType], + ); + return { referenceLines, referenceLineValues }; } function TileAlertChart({ @@ -294,7 +307,7 @@ function TileAlertChart({ [tile, source, dashboard?.filters, dateRange, granularity], ); - const referenceLines = useAlertReferenceLines(alert); + const { referenceLines, referenceLineValues } = useAlertReferenceLines(alert); if (isDashboardsLoading || (tileSourceId != null && isSourceLoading)) { return ; @@ -321,6 +334,7 @@ function TileAlertChart({ showMVOptimizationIndicator={false} showDateRangeIndicator={false} referenceLines={referenceLines} + referenceLineValues={referenceLineValues} annotations={annotations} config={config} /> @@ -364,7 +378,7 @@ function InlineAlertChart({ [chartConfig, source, dateRange, granularity], ); - const referenceLines = useAlertReferenceLines(alert); + const { referenceLines, referenceLineValues } = useAlertReferenceLines(alert); if (configSourceId != null && isSourceLoading) { return ; @@ -391,6 +405,7 @@ function InlineAlertChart({ showMVOptimizationIndicator={false} showDateRangeIndicator={false} referenceLines={referenceLines} + referenceLineValues={referenceLineValues} annotations={annotations} config={config} /> diff --git a/packages/app/src/utils.ts b/packages/app/src/utils.ts index 6315d561b2..35386b87e2 100644 --- a/packages/app/src/utils.ts +++ b/packages/app/src/utils.ts @@ -1104,21 +1104,27 @@ export function formatDurationMs(ms: number): string { return `${parseFloat((ms / 3_600_000).toFixed(2))}h`; } -/** Compact duration labels for axis ticks — fewer decimals, shorter units. */ -export function formatDurationMsCompact(ms: number): string { - if (ms < 0) return `-${formatDurationMsCompact(-ms)}`; +// Compact duration labels for axis ticks. An explicit `precision` overrides +// each branch's own default significant-digit count. +export function formatDurationMsCompact( + ms: number, + precision?: number, +): string { + if (ms < 0) return `-${formatDurationMsCompact(-ms, precision)}`; if (ms === 0) return '0'; - if (ms < 0.001) return `${+(ms * 1e6).toPrecision(2)}ns`; + if (ms < 0.001) return `${+(ms * 1e6).toPrecision(precision ?? 2)}ns`; if (ms < 1) { const µs = ms * 1000; + if (precision != null) return `${+µs.toPrecision(precision)}µs`; return µs < 10 ? `${+µs.toPrecision(2)}µs` : `${Math.round(µs)}µs`; } if (ms < 1000) { + if (precision != null) return `${+ms.toPrecision(precision)}ms`; return ms < 10 ? `${+ms.toPrecision(2)}ms` : `${Math.round(ms)}ms`; } - if (ms < 120_000) return `${+(ms / 1000).toPrecision(3)}s`; - if (ms < 3_600_000) return `${+(ms / 60_000).toPrecision(2)}m`; - return `${+(ms / 3_600_000).toPrecision(2)}h`; + if (ms < 120_000) return `${+(ms / 1000).toPrecision(precision ?? 3)}s`; + if (ms < 3_600_000) return `${+(ms / 60_000).toPrecision(precision ?? 2)}m`; + return `${+(ms / 3_600_000).toPrecision(precision ?? 2)}h`; } // format uptime as days, hours, minutes or seconds