Memoize Fluent chart token resolution
This commit is contained in:
@@ -168,7 +168,11 @@ Issues are grouped by category and ordered roughly by severity. Each entry descr
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
### TASK-009 — `resolveFluentToken` calls `getComputedStyle` on every render
|
### TASK-009 — `resolveFluentToken` calls `getComputedStyle` on every render (done)
|
||||||
|
|
||||||
|
**Where fixed:** `frontend/src/components/BanTrendChart.tsx`, `frontend/src/components/TopCountriesPieChart.tsx`, `frontend/src/components/TopCountriesBarChart.tsx`, `frontend/src/components/JailDistributionChart.tsx`
|
||||||
|
|
||||||
|
**Summary:** Wrapped Fluent token resolution calls in `useMemo([])` in each chart component and added tests verifying token resolution is memoized across rerenders.
|
||||||
|
|
||||||
**Where found:** `frontend/src/utils/chartTheme.ts`, `resolveFluentToken` function. Called 2–3 times per render in `BanTrendChart`, `TopCountriesPieChart`, `TopCountriesBarChart`, and `JailDistributionChart`.
|
**Where found:** `frontend/src/utils/chartTheme.ts`, `resolveFluentToken` function. Called 2–3 times per render in `BanTrendChart`, `TopCountriesPieChart`, `TopCountriesBarChart`, and `JailDistributionChart`.
|
||||||
|
|
||||||
|
|||||||
@@ -4,6 +4,7 @@
|
|||||||
* Calls `useBanTrend` internally and handles loading, error, and empty states.
|
* Calls `useBanTrend` internally and handles loading, error, and empty states.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
import { useMemo } from "react";
|
||||||
import {
|
import {
|
||||||
Area,
|
Area,
|
||||||
AreaChart,
|
AreaChart,
|
||||||
@@ -134,8 +135,14 @@ function buildEntries(
|
|||||||
// Custom tooltip
|
// Custom tooltip
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
function TrendTooltip(props: TooltipContentProps): React.JSX.Element | null {
|
interface TrendTooltipProps extends Partial<TooltipContentProps> {
|
||||||
const { active, payload } = props;
|
backgroundColor: string;
|
||||||
|
borderColor: string;
|
||||||
|
textColor: string;
|
||||||
|
}
|
||||||
|
|
||||||
|
function TrendTooltip(props: TrendTooltipProps): React.JSX.Element | null {
|
||||||
|
const { active, payload = [], backgroundColor, borderColor, textColor } = props;
|
||||||
if (!active || payload.length === 0) return null;
|
if (!active || payload.length === 0) return null;
|
||||||
const entry = payload[0];
|
const entry = payload[0];
|
||||||
if (entry == null) return null;
|
if (entry == null) return null;
|
||||||
@@ -154,11 +161,11 @@ function TrendTooltip(props: TooltipContentProps): React.JSX.Element | null {
|
|||||||
return (
|
return (
|
||||||
<div
|
<div
|
||||||
style={{
|
style={{
|
||||||
backgroundColor: resolveFluentToken(tokens.colorNeutralBackground1),
|
backgroundColor,
|
||||||
border: `1px solid ${resolveFluentToken(tokens.colorNeutralStroke2)}`,
|
border: `1px solid ${borderColor}`,
|
||||||
borderRadius: "4px",
|
borderRadius: "4px",
|
||||||
padding: "8px 12px",
|
padding: "8px 12px",
|
||||||
color: resolveFluentToken(tokens.colorNeutralForeground1),
|
color: textColor,
|
||||||
fontSize: "13px",
|
fontSize: "13px",
|
||||||
}}
|
}}
|
||||||
>
|
>
|
||||||
@@ -197,11 +204,27 @@ export function BanTrendChart({
|
|||||||
|
|
||||||
const isEmpty = buckets.every((b) => b.count === 0);
|
const isEmpty = buckets.every((b) => b.count === 0);
|
||||||
const entries = buildEntries(buckets, timeRange);
|
const entries = buildEntries(buckets, timeRange);
|
||||||
const primaryColour = resolveFluentToken(CHART_PALETTE[0] ?? "");
|
const { primaryColour, axisColour, gridColour } = useMemo(
|
||||||
const axisColour = resolveFluentToken(CHART_AXIS_TEXT_TOKEN);
|
() => ({
|
||||||
const gridColour = resolveFluentToken(CHART_GRID_LINE_TOKEN);
|
primaryColour: resolveFluentToken(CHART_PALETTE[0] ?? ""),
|
||||||
|
axisColour: resolveFluentToken(CHART_AXIS_TEXT_TOKEN),
|
||||||
|
gridColour: resolveFluentToken(CHART_GRID_LINE_TOKEN),
|
||||||
|
}),
|
||||||
|
[],
|
||||||
|
);
|
||||||
const tickInterval = TICK_INTERVAL[timeRange];
|
const tickInterval = TICK_INTERVAL[timeRange];
|
||||||
|
|
||||||
|
const tooltipContent = useMemo(
|
||||||
|
() => (
|
||||||
|
<TrendTooltip
|
||||||
|
backgroundColor={resolveFluentToken(tokens.colorNeutralBackground1)}
|
||||||
|
borderColor={resolveFluentToken(tokens.colorNeutralStroke2)}
|
||||||
|
textColor={resolveFluentToken(tokens.colorNeutralForeground1)}
|
||||||
|
/>
|
||||||
|
),
|
||||||
|
[],
|
||||||
|
);
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<ChartStateWrapper
|
<ChartStateWrapper
|
||||||
isLoading={isLoading}
|
isLoading={isLoading}
|
||||||
@@ -233,7 +256,7 @@ export function BanTrendChart({
|
|||||||
tickLine={false}
|
tickLine={false}
|
||||||
axisLine={false}
|
axisLine={false}
|
||||||
/>
|
/>
|
||||||
<Tooltip content={TrendTooltip} />
|
<Tooltip content={tooltipContent} />
|
||||||
<Area
|
<Area
|
||||||
type="monotone"
|
type="monotone"
|
||||||
dataKey="count"
|
dataKey="count"
|
||||||
|
|||||||
@@ -6,6 +6,7 @@
|
|||||||
* empty states so the parent only needs to pass filter props.
|
* empty states so the parent only needs to pass filter props.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
import { useMemo } from "react";
|
||||||
import {
|
import {
|
||||||
Bar,
|
Bar,
|
||||||
BarChart,
|
BarChart,
|
||||||
@@ -137,9 +138,14 @@ export function JailDistributionChart({
|
|||||||
|
|
||||||
const entries = buildEntries(jails);
|
const entries = buildEntries(jails);
|
||||||
const chartHeight = Math.max(entries.length * BAR_HEIGHT_PX, MIN_CHART_HEIGHT);
|
const chartHeight = Math.max(entries.length * BAR_HEIGHT_PX, MIN_CHART_HEIGHT);
|
||||||
const primaryColour = resolveFluentToken(CHART_PALETTE[0] ?? "");
|
const { primaryColour, axisColour, gridColour } = useMemo(
|
||||||
const axisColour = resolveFluentToken(CHART_AXIS_TEXT_TOKEN);
|
() => ({
|
||||||
const gridColour = resolveFluentToken(CHART_GRID_LINE_TOKEN);
|
primaryColour: resolveFluentToken(CHART_PALETTE[0] ?? ""),
|
||||||
|
axisColour: resolveFluentToken(CHART_AXIS_TEXT_TOKEN),
|
||||||
|
gridColour: resolveFluentToken(CHART_GRID_LINE_TOKEN),
|
||||||
|
}),
|
||||||
|
[],
|
||||||
|
);
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<ChartStateWrapper
|
<ChartStateWrapper
|
||||||
|
|||||||
@@ -3,6 +3,7 @@
|
|||||||
* by ban count, sorted descending.
|
* by ban count, sorted descending.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
import { useMemo } from "react";
|
||||||
import {
|
import {
|
||||||
Bar,
|
Bar,
|
||||||
BarChart,
|
BarChart,
|
||||||
@@ -144,6 +145,15 @@ export function TopCountriesBarChart({
|
|||||||
|
|
||||||
const entries = buildEntries(countries, countryNames);
|
const entries = buildEntries(countries, countryNames);
|
||||||
|
|
||||||
|
const { primaryColour, axisColour, gridColour } = useMemo(
|
||||||
|
() => ({
|
||||||
|
primaryColour: resolveFluentToken(CHART_PALETTE[0] ?? ""),
|
||||||
|
axisColour: resolveFluentToken(CHART_AXIS_TEXT_TOKEN),
|
||||||
|
gridColour: resolveFluentToken(CHART_GRID_LINE_TOKEN),
|
||||||
|
}),
|
||||||
|
[],
|
||||||
|
);
|
||||||
|
|
||||||
if (entries.length === 0) {
|
if (entries.length === 0) {
|
||||||
return (
|
return (
|
||||||
<div className={styles.emptyWrapper}>
|
<div className={styles.emptyWrapper}>
|
||||||
@@ -154,10 +164,6 @@ export function TopCountriesBarChart({
|
|||||||
|
|
||||||
const chartHeight = Math.max(entries.length * BAR_HEIGHT_PX, MIN_CHART_HEIGHT);
|
const chartHeight = Math.max(entries.length * BAR_HEIGHT_PX, MIN_CHART_HEIGHT);
|
||||||
|
|
||||||
const primaryColour = resolveFluentToken(CHART_PALETTE[0] ?? "");
|
|
||||||
const axisColour = resolveFluentToken(CHART_AXIS_TEXT_TOKEN);
|
|
||||||
const gridColour = resolveFluentToken(CHART_GRID_LINE_TOKEN);
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div className={styles.wrapper} style={{ height: chartHeight }}>
|
<div className={styles.wrapper} style={{ height: chartHeight }}>
|
||||||
<ResponsiveContainer width="100%" height="100%">
|
<ResponsiveContainer width="100%" height="100%">
|
||||||
|
|||||||
@@ -3,6 +3,7 @@
|
|||||||
* an "Other" slice aggregating all remaining countries.
|
* an "Other" slice aggregating all remaining countries.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
import { useMemo } from "react";
|
||||||
import {
|
import {
|
||||||
Cell,
|
Cell,
|
||||||
Legend,
|
Legend,
|
||||||
@@ -134,7 +135,10 @@ export function TopCountriesPieChart({
|
|||||||
}: TopCountriesPieChartProps): React.JSX.Element {
|
}: TopCountriesPieChartProps): React.JSX.Element {
|
||||||
const styles = useStyles();
|
const styles = useStyles();
|
||||||
|
|
||||||
const resolvedPalette = CHART_PALETTE.map(resolveFluentToken);
|
const resolvedPalette = useMemo(
|
||||||
|
() => CHART_PALETTE.map(resolveFluentToken),
|
||||||
|
[],
|
||||||
|
);
|
||||||
const slices = buildSlices(countries, countryNames, resolvedPalette);
|
const slices = buildSlices(countries, countryNames, resolvedPalette);
|
||||||
const total = slices.reduce((sum, s) => sum + s.value, 0);
|
const total = slices.reduce((sum, s) => sum + s.value, 0);
|
||||||
|
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ import { render, screen } from "@testing-library/react";
|
|||||||
import userEvent from "@testing-library/user-event";
|
import userEvent from "@testing-library/user-event";
|
||||||
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
||||||
import { BanTrendChart } from "../BanTrendChart";
|
import { BanTrendChart } from "../BanTrendChart";
|
||||||
|
import * as chartTheme from "../../utils/chartTheme";
|
||||||
import * as useBanTrendModule from "../../hooks/useBanTrend";
|
import * as useBanTrendModule from "../../hooks/useBanTrend";
|
||||||
import type { UseBanTrendResult } from "../../hooks/useBanTrend";
|
import type { UseBanTrendResult } from "../../hooks/useBanTrend";
|
||||||
import type { BanTrendBucket } from "../../types/ban";
|
import type { BanTrendBucket } from "../../types/ban";
|
||||||
@@ -87,4 +88,22 @@ describe("BanTrendChart", () => {
|
|||||||
wrap(<BanTrendChart timeRange="24h" origin="all" />);
|
wrap(<BanTrendChart timeRange="24h" origin="all" />);
|
||||||
expect(screen.getByTestId("area-chart")).toBeInTheDocument();
|
expect(screen.getByTestId("area-chart")).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("memoizes token resolution across rerenders", () => {
|
||||||
|
const spy = vi.spyOn(chartTheme, "resolveFluentToken");
|
||||||
|
const props = { timeRange: "24h" as const, origin: "all" as const };
|
||||||
|
mockHook({ buckets: [{ timestamp: "2024-01-01T00:00:00Z", count: 5 }] });
|
||||||
|
|
||||||
|
const { rerender } = wrap(<BanTrendChart {...props} />);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(6);
|
||||||
|
|
||||||
|
rerender(
|
||||||
|
<FluentProvider theme={webLightTheme}>
|
||||||
|
<BanTrendChart {...props} />
|
||||||
|
</FluentProvider>,
|
||||||
|
);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(6);
|
||||||
|
|
||||||
|
spy.mockRestore();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ import { render, screen } from "@testing-library/react";
|
|||||||
import userEvent from "@testing-library/user-event";
|
import userEvent from "@testing-library/user-event";
|
||||||
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
||||||
import { JailDistributionChart } from "../JailDistributionChart";
|
import { JailDistributionChart } from "../JailDistributionChart";
|
||||||
|
import * as chartTheme from "../../utils/chartTheme";
|
||||||
import * as useJailDistributionModule from "../../hooks/useJailDistribution";
|
import * as useJailDistributionModule from "../../hooks/useJailDistribution";
|
||||||
import type { UseJailDistributionResult } from "../../hooks/useJailDistribution";
|
import type { UseJailDistributionResult } from "../../hooks/useJailDistribution";
|
||||||
|
|
||||||
@@ -84,4 +85,21 @@ describe("JailDistributionChart", () => {
|
|||||||
wrap(<JailDistributionChart timeRange="24h" origin="all" />);
|
wrap(<JailDistributionChart timeRange="24h" origin="all" />);
|
||||||
expect(screen.getByTestId("bar-chart")).toBeInTheDocument();
|
expect(screen.getByTestId("bar-chart")).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("memoizes token resolution across rerenders", () => {
|
||||||
|
const spy = vi.spyOn(chartTheme, "resolveFluentToken");
|
||||||
|
const props = { timeRange: "24h" as const, origin: "all" as const };
|
||||||
|
|
||||||
|
const { rerender } = wrap(<JailDistributionChart {...props} />);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(3);
|
||||||
|
|
||||||
|
rerender(
|
||||||
|
<FluentProvider theme={webLightTheme}>
|
||||||
|
<JailDistributionChart {...props} />
|
||||||
|
</FluentProvider>,
|
||||||
|
);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(3);
|
||||||
|
|
||||||
|
spy.mockRestore();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import { describe, it, expect, vi } from "vitest";
|
|||||||
import { render, screen } from "@testing-library/react";
|
import { render, screen } from "@testing-library/react";
|
||||||
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
||||||
import { TopCountriesBarChart } from "../TopCountriesBarChart";
|
import { TopCountriesBarChart } from "../TopCountriesBarChart";
|
||||||
|
import * as chartTheme from "../../utils/chartTheme";
|
||||||
|
|
||||||
vi.mock("recharts", () => ({
|
vi.mock("recharts", () => ({
|
||||||
ResponsiveContainer: ({ children }: { children: React.ReactNode }) => (
|
ResponsiveContainer: ({ children }: { children: React.ReactNode }) => (
|
||||||
@@ -41,6 +42,26 @@ describe("TopCountriesBarChart", () => {
|
|||||||
expect(screen.getByTestId("bar-chart")).toBeInTheDocument();
|
expect(screen.getByTestId("bar-chart")).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("memoizes token resolution across rerenders", () => {
|
||||||
|
const spy = vi.spyOn(chartTheme, "resolveFluentToken");
|
||||||
|
const props = {
|
||||||
|
countries: { DE: 50, US: 30, FR: 15 },
|
||||||
|
countryNames: { DE: "Germany", US: "United States", FR: "France" },
|
||||||
|
};
|
||||||
|
|
||||||
|
const { rerender } = wrap(<TopCountriesBarChart {...props} />);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(3);
|
||||||
|
|
||||||
|
rerender(
|
||||||
|
<FluentProvider theme={webLightTheme}>
|
||||||
|
<TopCountriesBarChart {...props} />
|
||||||
|
</FluentProvider>,
|
||||||
|
);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(3);
|
||||||
|
|
||||||
|
spy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
it("does not render more than 20 bars (TOP_N limit)", () => {
|
it("does not render more than 20 bars (TOP_N limit)", () => {
|
||||||
// Build 30 countries — only top 20 should appear in the chart
|
// Build 30 countries — only top 20 should appear in the chart
|
||||||
const countries: Record<string, number> = {};
|
const countries: Record<string, number> = {};
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import { describe, it, expect, vi } from "vitest";
|
|||||||
import { render, screen } from "@testing-library/react";
|
import { render, screen } from "@testing-library/react";
|
||||||
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
import { FluentProvider, webLightTheme } from "@fluentui/react-components";
|
||||||
import { TopCountriesPieChart } from "../TopCountriesPieChart";
|
import { TopCountriesPieChart } from "../TopCountriesPieChart";
|
||||||
|
import * as chartTheme from "../../utils/chartTheme";
|
||||||
|
|
||||||
vi.mock("recharts", () => ({
|
vi.mock("recharts", () => ({
|
||||||
ResponsiveContainer: ({ children }: { children: React.ReactNode }) => (
|
ResponsiveContainer: ({ children }: { children: React.ReactNode }) => (
|
||||||
@@ -38,4 +39,24 @@ describe("TopCountriesPieChart", () => {
|
|||||||
);
|
);
|
||||||
expect(screen.getByTestId("pie-chart")).toBeInTheDocument();
|
expect(screen.getByTestId("pie-chart")).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("memoizes palette token resolution across rerenders", () => {
|
||||||
|
const spy = vi.spyOn(chartTheme, "resolveFluentToken");
|
||||||
|
const props = {
|
||||||
|
countries: { DE: 30, US: 20, CN: 10 },
|
||||||
|
countryNames: { DE: "Germany", US: "United States", CN: "China" },
|
||||||
|
};
|
||||||
|
|
||||||
|
const { rerender } = wrap(<TopCountriesPieChart {...props} />);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(5);
|
||||||
|
|
||||||
|
rerender(
|
||||||
|
<FluentProvider theme={webLightTheme}>
|
||||||
|
<TopCountriesPieChart {...props} />
|
||||||
|
</FluentProvider>,
|
||||||
|
);
|
||||||
|
expect(spy).toHaveBeenCalledTimes(5);
|
||||||
|
|
||||||
|
spy.mockRestore();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user