fix(chart): stop tooltip render loop crashing the dashboard (PHP-LARAVEL-3B) (#657)
## Problem Sentry **PHP-LARAVEL-3B** — React `Maximum update depth exceeded` crashing `/dashboard`. Regressed, 5 occurrences / 4 users (mobile Chrome/Android and Safari), last seen today. ## Root cause `ChartTooltipPortal` in `resources/js/components/ui/chart.tsx` positioned the portaled tooltip in a `useLayoutEffect` with **no dependency array**, so it ran after every render and called `setPos`. An equality guard was the only thing preventing a `render → effect → setPos → render` feedback loop, and it failed once the computed position oscillated by a sub-pixel (fractional `getBoundingClientRect` / integer `offsetWidth`). React then aborted after 50 nested updates, taking down the dashboard. ## Fix 1. **`651c7d72`** — Depend on the primitive `coordinate.x/.y` (and `offset`) so the effect runs only when the cursor moves, never as a result of its own `setPos`. `coordinate` itself is excluded on purpose: Recharts hands a fresh object every render, so depending on the object would reopen the loop. Extract the flip/clamp math into a pure `computeTooltipPosition` helper (`resources/js/lib/`) rounded to whole pixels, and unit-test it. 2. **`1c1602db`** — Component regression test: asserts the positioning effect does **not** re-run on a re-render with an unchanged coordinate, and **does** on a coordinate change. Verified it fails against the pre-fix dependency-less effect. 3. **`0354a031`** — Strengthen the helper test to assert sub-pixel positions collapse to one pixel, and correct the doc note (the dependency array removes the loop; rounding only narrows the oscillation). ## Review Two independent review agents (architecture/duplication/tests + user-facing correctness) examined the diff: - **Correctness**: no must-fix issues. First paint is correct (measured at `-9999`/opacity 0 before paint, capped by a viewport-relative `max-w`, so size is accurate and there is no flash). The loop cannot recur through the `setHidden` effect or a repeated identical coordinate. No mobile/edge regression — the flip/clamp logic is the extracted original. - **Architecture**: helper placement/naming/types match conventions; no duplicate positioning logic elsewhere to fold in; both agents' top request (regression-test the actual fix mechanism) is addressed by commit 2. `ChartTooltipPortal` is exported for the test, matching how `StackedBarShape` is exported for its own test. ## Known minor residual (acceptable) If the tooltip's **content size** changes while the cursor stays on the exact same point *and* it sits at a viewport edge, the flip/clamp won't recompute until the next mouse move. In practice content size changes together with the coordinate (Recharts hover). Worst case is a one-frame cosmetic overflow that self-corrects — strictly better than the crash it replaces. ## Testing `chart-tooltip-position.test.ts` (5) + `chart-tooltip-portal.test.tsx` (2) pass. Note: the infinite loop itself can't be reproduced in jsdom (zero-size rects), so the component test guards the mechanism (effect does not self-retrigger) rather than the throw. Fixes PHP-LARAVEL-3B
This commit is contained in:
Víctor Falcón
GitHub
parent
7d750fa1a8
commit
5b3400909e
|
|
@ -0,0 +1,58 @@
|
|||
import { fireEvent, render } from '@testing-library/react';
|
||||
import * as React from 'react';
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import { ChartTooltipPortal } from './chart';
|
||||
|
||||
/**
|
||||
* Renders the portal inside a `.recharts-wrapper` (the ancestor its layout
|
||||
* effect looks up) and exposes a button that bumps unrelated state to force a
|
||||
* re-render without touching `coordinate`.
|
||||
*/
|
||||
function Harness({ coordinate }: { coordinate: { x: number; y: number } }) {
|
||||
const [, setTick] = React.useState(0);
|
||||
|
||||
return (
|
||||
<div className="recharts-wrapper">
|
||||
<button onClick={() => setTick((t) => t + 1)}>rerender</button>
|
||||
<ChartTooltipPortal coordinate={coordinate}>
|
||||
<span>tooltip</span>
|
||||
</ChartTooltipPortal>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
describe('ChartTooltipPortal', () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
// The positioning effect reads the wrapper's rect, so a call to
|
||||
// getBoundingClientRect is a proxy for "the effect ran".
|
||||
it('does not recompute its position on a re-render with an unchanged coordinate (regression: PHP-LARAVEL-3B render loop)', () => {
|
||||
const rectSpy = vi.spyOn(Element.prototype, 'getBoundingClientRect');
|
||||
const { getByText } = render(<Harness coordinate={{ x: 10, y: 20 }} />);
|
||||
|
||||
const afterMount = rectSpy.mock.calls.length;
|
||||
expect(afterMount).toBeGreaterThan(0);
|
||||
|
||||
fireEvent.click(getByText('rerender'));
|
||||
fireEvent.click(getByText('rerender'));
|
||||
|
||||
// With the pre-fix dependency-less effect this count would climb on
|
||||
// every render and the setPos feedback loop would eventually throw
|
||||
// "Maximum update depth exceeded".
|
||||
expect(rectSpy.mock.calls.length).toBe(afterMount);
|
||||
});
|
||||
|
||||
it('recomputes its position when the coordinate changes', () => {
|
||||
const rectSpy = vi.spyOn(Element.prototype, 'getBoundingClientRect');
|
||||
const { rerender } = render(<Harness coordinate={{ x: 10, y: 20 }} />);
|
||||
|
||||
const afterMount = rectSpy.mock.calls.length;
|
||||
|
||||
rerender(<Harness coordinate={{ x: 200, y: 300 }} />);
|
||||
|
||||
expect(rectSpy.mock.calls.length).toBeGreaterThan(afterMount);
|
||||
});
|
||||
});
|
||||
|
|
@ -2,6 +2,7 @@ import * as React from 'react';
|
|||
import { createPortal } from 'react-dom';
|
||||
import * as RechartsPrimitive from 'recharts';
|
||||
|
||||
import { computeTooltipPosition } from '@/lib/chart-tooltip-position';
|
||||
import { usePrivacyMode } from '@/contexts/privacy-mode-context';
|
||||
import { useLocale } from '@/hooks/use-locale';
|
||||
import { cn } from '@/lib/utils';
|
||||
|
|
@ -142,6 +143,11 @@ function ChartTooltipPortal({
|
|||
};
|
||||
}, []);
|
||||
|
||||
// Only recompute when the cursor coordinate (or offset) changes. Depending
|
||||
// on nothing re-ran this effect after its own setPos, and the equality
|
||||
// guard alone could not stop the render→effect→setPos loop once positions
|
||||
// oscillated by a sub-pixel — React aborted with "Maximum update depth
|
||||
// exceeded". `pos` is deliberately not a dependency.
|
||||
React.useLayoutEffect(() => {
|
||||
if (!anchorRef.current || !coordinate) {
|
||||
return;
|
||||
|
|
@ -151,37 +157,26 @@ function ChartTooltipPortal({
|
|||
return;
|
||||
}
|
||||
const rect = wrapper.getBoundingClientRect();
|
||||
const cx = (coordinate.x ?? 0) + rect.left;
|
||||
const cy = (coordinate.y ?? 0) + rect.top;
|
||||
|
||||
const tipEl = tooltipRef.current;
|
||||
const tipW = tipEl?.offsetWidth ?? 0;
|
||||
const tipH = tipEl?.offsetHeight ?? 0;
|
||||
|
||||
let x = cx + offset;
|
||||
let y = cy + offset;
|
||||
|
||||
// Flip if overflowing viewport
|
||||
if (x + tipW > window.innerWidth - 8) {
|
||||
x = cx - tipW - offset;
|
||||
}
|
||||
if (y + tipH > window.innerHeight - 8) {
|
||||
y = cy - tipH - offset;
|
||||
}
|
||||
if (x < 8) {
|
||||
x = 8;
|
||||
}
|
||||
if (y < 8) {
|
||||
y = 8;
|
||||
}
|
||||
|
||||
setPos((prev) => {
|
||||
if (prev && prev.x === x && prev.y === y) {
|
||||
return prev;
|
||||
}
|
||||
return { x, y };
|
||||
const next = computeTooltipPosition({
|
||||
cx: (coordinate.x ?? 0) + rect.left,
|
||||
cy: (coordinate.y ?? 0) + rect.top,
|
||||
tipW: tipEl?.offsetWidth ?? 0,
|
||||
tipH: tipEl?.offsetHeight ?? 0,
|
||||
offset,
|
||||
viewportW: window.innerWidth,
|
||||
viewportH: window.innerHeight,
|
||||
});
|
||||
});
|
||||
|
||||
setPos((prev) =>
|
||||
prev && prev.x === next.x && prev.y === next.y ? prev : next,
|
||||
);
|
||||
// Depend on the primitive x/y, not the `coordinate` object: Recharts
|
||||
// passes a fresh object every render, so depending on it would run this
|
||||
// effect on every render again and reopen the loop.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [coordinate?.x, coordinate?.y, offset]);
|
||||
|
||||
return (
|
||||
<>
|
||||
|
|
@ -691,6 +686,7 @@ export {
|
|||
ChartContainer,
|
||||
ChartTooltip,
|
||||
ChartTooltipContent,
|
||||
ChartTooltipPortal,
|
||||
ChartLegend,
|
||||
ChartLegendContent,
|
||||
ChartStyle,
|
||||
|
|
|
|||
|
|
@ -0,0 +1,77 @@
|
|||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
import { computeTooltipPosition } from './chart-tooltip-position';
|
||||
|
||||
const viewport = { viewportW: 1000, viewportH: 800 };
|
||||
|
||||
describe('computeTooltipPosition', () => {
|
||||
it('places the tooltip below-right of the cursor when it fits', () => {
|
||||
const pos = computeTooltipPosition({
|
||||
cx: 100,
|
||||
cy: 100,
|
||||
tipW: 120,
|
||||
tipH: 60,
|
||||
offset: 12,
|
||||
...viewport,
|
||||
});
|
||||
|
||||
expect(pos).toEqual({ x: 112, y: 112 });
|
||||
});
|
||||
|
||||
it('flips to the left when it would overflow the right edge', () => {
|
||||
const pos = computeTooltipPosition({
|
||||
cx: 950,
|
||||
cy: 100,
|
||||
tipW: 120,
|
||||
tipH: 60,
|
||||
offset: 12,
|
||||
...viewport,
|
||||
});
|
||||
|
||||
// 950 + 12 + 120 = 1082 > 1000 - 8, so it flips: 950 - 120 - 12
|
||||
expect(pos.x).toBe(818);
|
||||
});
|
||||
|
||||
it('flips upward when it would overflow the bottom edge', () => {
|
||||
const pos = computeTooltipPosition({
|
||||
cx: 100,
|
||||
cy: 780,
|
||||
tipW: 120,
|
||||
tipH: 60,
|
||||
offset: 12,
|
||||
...viewport,
|
||||
});
|
||||
|
||||
// 780 + 12 + 60 = 852 > 800 - 8, so it flips: 780 - 60 - 12
|
||||
expect(pos.y).toBe(708);
|
||||
});
|
||||
|
||||
it('clamps to the 8px margin when even the flipped position is off-screen', () => {
|
||||
// Tooltip larger than the space on both sides of the cursor: the flip
|
||||
// pushes the coordinate negative, so it must clamp to 8 rather than
|
||||
// render partially off-screen.
|
||||
const pos = computeTooltipPosition({
|
||||
cx: 950,
|
||||
cy: 780,
|
||||
tipW: 980,
|
||||
tipH: 790,
|
||||
offset: 12,
|
||||
...viewport,
|
||||
});
|
||||
|
||||
expect(pos).toEqual({ x: 8, y: 8 });
|
||||
});
|
||||
|
||||
it('rounds to whole pixels so sub-pixel jitter collapses to one coordinate', () => {
|
||||
const base = { tipW: 120, tipH: 60, offset: 12, ...viewport };
|
||||
const a = computeTooltipPosition({ cx: 100.1, cy: 100.1, ...base });
|
||||
const b = computeTooltipPosition({ cx: 100.4, cy: 100.4, ...base });
|
||||
|
||||
expect(Number.isInteger(a.x)).toBe(true);
|
||||
expect(Number.isInteger(a.y)).toBe(true);
|
||||
// 100.1 and 100.4 both land on the same pixel, so the equality guard
|
||||
// sees an unchanged position instead of thrashing on the difference.
|
||||
expect(a).toEqual(b);
|
||||
expect(a).toEqual({ x: 112, y: 112 });
|
||||
});
|
||||
});
|
||||
|
|
@ -0,0 +1,53 @@
|
|||
export interface TooltipPositionInput {
|
||||
/** Cursor x in viewport coordinates. */
|
||||
cx: number;
|
||||
/** Cursor y in viewport coordinates. */
|
||||
cy: number;
|
||||
/** Measured tooltip width. */
|
||||
tipW: number;
|
||||
/** Measured tooltip height. */
|
||||
tipH: number;
|
||||
/** Gap between the cursor and the tooltip. */
|
||||
offset: number;
|
||||
/** Viewport width (window.innerWidth). */
|
||||
viewportW: number;
|
||||
/** Viewport height (window.innerHeight). */
|
||||
viewportH: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Computes the fixed-position coordinates for the chart tooltip, flipping it to
|
||||
* the opposite side of the cursor when it would overflow the viewport and
|
||||
* clamping it to an 8px margin. The result is rounded to whole pixels so that
|
||||
* sub-pixel jitter from `getBoundingClientRect` collapses to the same
|
||||
* coordinate. (The effect's dependency array in chart.tsx is what actually
|
||||
* stops the render loop; rounding only reduces how often the equality guard is
|
||||
* defeated by a fractional difference.)
|
||||
*/
|
||||
export function computeTooltipPosition({
|
||||
cx,
|
||||
cy,
|
||||
tipW,
|
||||
tipH,
|
||||
offset,
|
||||
viewportW,
|
||||
viewportH,
|
||||
}: TooltipPositionInput): { x: number; y: number } {
|
||||
let x = cx + offset;
|
||||
let y = cy + offset;
|
||||
|
||||
if (x + tipW > viewportW - 8) {
|
||||
x = cx - tipW - offset;
|
||||
}
|
||||
if (y + tipH > viewportH - 8) {
|
||||
y = cy - tipH - offset;
|
||||
}
|
||||
if (x < 8) {
|
||||
x = 8;
|
||||
}
|
||||
if (y < 8) {
|
||||
y = 8;
|
||||
}
|
||||
|
||||
return { x: Math.round(x), y: Math.round(y) };
|
||||
}
|
||||
Loading…
Reference in New Issue