diff --git a/resources/js/components/ui/chart-tooltip-portal.test.tsx b/resources/js/components/ui/chart-tooltip-portal.test.tsx new file mode 100644 index 00000000..0ba6e2eb --- /dev/null +++ b/resources/js/components/ui/chart-tooltip-portal.test.tsx @@ -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 ( +
+ + + tooltip + +
+ ); +} + +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(); + + 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(); + + const afterMount = rectSpy.mock.calls.length; + + rerender(); + + expect(rectSpy.mock.calls.length).toBeGreaterThan(afterMount); + }); +}); diff --git a/resources/js/components/ui/chart.tsx b/resources/js/components/ui/chart.tsx index f1c44a39..886200cd 100644 --- a/resources/js/components/ui/chart.tsx +++ b/resources/js/components/ui/chart.tsx @@ -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, diff --git a/resources/js/lib/chart-tooltip-position.test.ts b/resources/js/lib/chart-tooltip-position.test.ts new file mode 100644 index 00000000..1913640f --- /dev/null +++ b/resources/js/lib/chart-tooltip-position.test.ts @@ -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 }); + }); +}); diff --git a/resources/js/lib/chart-tooltip-position.ts b/resources/js/lib/chart-tooltip-position.ts new file mode 100644 index 00000000..4e674519 --- /dev/null +++ b/resources/js/lib/chart-tooltip-position.ts @@ -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) }; +}