refactoring-backend #3
@@ -1,30 +1,3 @@
|
||||
### TASK-STATE-02 — `withRefresh` in `useJailList` Creates New Function References Every Render
|
||||
|
||||
**Where found**
|
||||
`frontend/src/hooks/useJailList.ts`. The `withRefresh` helper is defined inside the hook body without `useCallback`. It wraps an operation and calls `refresh()` after it completes. Because it is recreated every render, `startJail`, `stopJail`, `setIdle`, and `reloadJail` — which are all produced by `withRefresh(...)` — are also new references every render. Any child component receiving these as props will re-render even when nothing has changed.
|
||||
|
||||
**Goal**
|
||||
Wrap `withRefresh` in `useCallback` with `[refresh]` as dependency, or inline `useCallback` directly for each operation:
|
||||
```ts
|
||||
const startJail = useCallback(async (name: string) => {
|
||||
await apiStartJail(name);
|
||||
refresh();
|
||||
}, [refresh]);
|
||||
```
|
||||
This ensures the function references are stable between renders.
|
||||
|
||||
**Possible traps and issues**
|
||||
- If `withRefresh` is used to produce many operations, it may be cleaner to keep `withRefresh` as a `useCallback` that captures `refresh`, rather than wrapping each call individually.
|
||||
- After stabilising these references, `JailOverviewSection` or any child wrapped in `React.memo` will correctly skip re-renders when only unrelated state changes.
|
||||
|
||||
**Docs changes needed**
|
||||
None required.
|
||||
|
||||
**Why this is needed**
|
||||
Unstable function references passed as props defeat `React.memo` optimisations and cause unnecessary child re-renders. On the Jails page, which renders a list of potentially many jails, this compounds with every parent state change.
|
||||
|
||||
---
|
||||
|
||||
### TASK-STATE-03 — `DashboardFilterBar` Has Dual State Source
|
||||
|
||||
**Where found**
|
||||
|
||||
@@ -5,7 +5,6 @@
|
||||
*/
|
||||
|
||||
import { memo, useMemo } from "react";
|
||||
import { useDashboardFilters } from "../providers/DashboardFilterProvider";
|
||||
import {
|
||||
Area,
|
||||
AreaChart,
|
||||
@@ -53,11 +52,11 @@ const TICK_INTERVAL: Record<TimeRange, number> = {
|
||||
/** Props for {@link BanTrendChart}. */
|
||||
interface BanTrendChartProps {
|
||||
/** Time-range preset controlling the query window. */
|
||||
timeRange?: TimeRange;
|
||||
timeRange: TimeRange;
|
||||
/** Origin filter controlling which bans are included. */
|
||||
origin?: BanOriginFilter;
|
||||
origin: BanOriginFilter;
|
||||
/** Data source used for the chart. */
|
||||
source?: "fail2ban" | "archive";
|
||||
source: "fail2ban" | "archive";
|
||||
}
|
||||
|
||||
/** Internal chart data point shape. */
|
||||
@@ -182,19 +181,13 @@ function TrendTooltip(props: TrendTooltipProps): React.JSX.Element | null {
|
||||
// Component
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Area chart showing ban counts over time.
|
||||
*
|
||||
* Fetches data via `useBanTrend` and handles loading, error, and empty states
|
||||
* inline so the parent only needs to pass filter props.
|
||||
*
|
||||
/**
|
||||
* Area chart showing ban counts over time.
|
||||
*
|
||||
* Fetches data via `useBanTrend` and delegates loading, error, and empty states
|
||||
* to `ChartStateWrapper`.
|
||||
*
|
||||
* @param props - `timeRange` and `origin` filter props.
|
||||
* @param props - `timeRange`, `origin`, and `source` filter props.
|
||||
*/
|
||||
export const BanTrendChart = memo(function BanTrendChart({
|
||||
timeRange,
|
||||
@@ -202,15 +195,11 @@ export const BanTrendChart = memo(function BanTrendChart({
|
||||
source,
|
||||
}: BanTrendChartProps): React.JSX.Element {
|
||||
const styles = useStyles();
|
||||
const context = useDashboardFilters();
|
||||
const effectiveTimeRange = timeRange ?? context.timeRange;
|
||||
const effectiveOrigin = origin ?? context.originFilter;
|
||||
const effectiveSource = source ?? context.source;
|
||||
const { colorMode } = useThemeMode();
|
||||
const { buckets, loading, error, reload } = useBanTrend(effectiveTimeRange, effectiveOrigin, effectiveSource);
|
||||
const { buckets, loading, error, reload } = useBanTrend(timeRange, origin, source);
|
||||
|
||||
const isEmpty = buckets.every((b) => b.count === 0);
|
||||
const entries = buildEntries(buckets, effectiveTimeRange);
|
||||
const entries = buildEntries(buckets, timeRange);
|
||||
const { primaryColour, axisColour, gridColour } = useMemo(
|
||||
() => {
|
||||
void colorMode;
|
||||
@@ -222,7 +211,7 @@ export const BanTrendChart = memo(function BanTrendChart({
|
||||
},
|
||||
[colorMode],
|
||||
);
|
||||
const tickInterval = TICK_INTERVAL[effectiveTimeRange];
|
||||
const tickInterval = TICK_INTERVAL[timeRange];
|
||||
|
||||
const tooltipContent = useMemo(
|
||||
() => {
|
||||
|
||||
@@ -16,7 +16,6 @@ import {
|
||||
tokens,
|
||||
} from "@fluentui/react-components";
|
||||
import { useCardStyles } from "../components/commonStyles";
|
||||
import { useDashboardFilters } from "../providers/DashboardFilterProvider";
|
||||
import type { BanOriginFilter, TimeRange } from "../types/ban";
|
||||
import {
|
||||
BAN_ORIGIN_FILTER_LABELS,
|
||||
@@ -30,13 +29,13 @@ import {
|
||||
/** Props for {@link DashboardFilterBar}. */
|
||||
export interface DashboardFilterBarProps {
|
||||
/** Currently selected time-range preset. */
|
||||
timeRange?: TimeRange;
|
||||
timeRange: TimeRange;
|
||||
/** Called when the user selects a different time-range preset. */
|
||||
onTimeRangeChange?: (value: TimeRange) => void;
|
||||
onTimeRangeChange: (value: TimeRange) => void;
|
||||
/** Currently selected origin filter. */
|
||||
originFilter?: BanOriginFilter;
|
||||
originFilter: BanOriginFilter;
|
||||
/** Called when the user selects a different origin filter. */
|
||||
onOriginFilterChange?: (value: BanOriginFilter) => void;
|
||||
onOriginFilterChange: (value: BanOriginFilter) => void;
|
||||
/** Jail filter value (optional). */
|
||||
jail?: string;
|
||||
/** Called when the jail filter text changes (optional). */
|
||||
@@ -93,7 +92,8 @@ const useStyles = makeStyles({
|
||||
/**
|
||||
* Renders a global filter bar with time-range and origin-filter toggle buttons.
|
||||
*
|
||||
* The bar is fully controlled — it does not maintain its own state.
|
||||
* The bar is fully controlled — all state is passed in via props and changes
|
||||
* are communicated through callbacks. The component does not read from context.
|
||||
*
|
||||
* @param props - See {@link DashboardFilterBarProps}.
|
||||
*/
|
||||
@@ -107,15 +107,9 @@ export function DashboardFilterBar({
|
||||
ip,
|
||||
onIpChange,
|
||||
}: DashboardFilterBarProps): React.JSX.Element {
|
||||
const context = useDashboardFilters();
|
||||
const styles = useStyles();
|
||||
const cardStyles = useCardStyles();
|
||||
|
||||
const currentTimeRange = timeRange ?? context.timeRange;
|
||||
const handleTimeRangeChange = onTimeRangeChange ?? context.setTimeRange;
|
||||
const currentOriginFilter = originFilter ?? context.originFilter;
|
||||
const handleOriginFilterChange = onOriginFilterChange ?? context.setOriginFilter;
|
||||
|
||||
return (
|
||||
<div className={`${styles.container} ${cardStyles.card}`}>
|
||||
{/* Time-range group */}
|
||||
@@ -128,10 +122,10 @@ export function DashboardFilterBar({
|
||||
<ToggleButton
|
||||
key={r}
|
||||
size="small"
|
||||
checked={currentTimeRange === r}
|
||||
aria-pressed={currentTimeRange === r}
|
||||
checked={timeRange === r}
|
||||
aria-pressed={timeRange === r}
|
||||
onClick={() => {
|
||||
handleTimeRangeChange(r);
|
||||
onTimeRangeChange(r);
|
||||
}}
|
||||
>
|
||||
{TIME_RANGE_LABELS[r]}
|
||||
@@ -155,10 +149,10 @@ export function DashboardFilterBar({
|
||||
<ToggleButton
|
||||
key={f}
|
||||
size="small"
|
||||
checked={currentOriginFilter === f}
|
||||
aria-pressed={currentOriginFilter === f}
|
||||
checked={originFilter === f}
|
||||
aria-pressed={originFilter === f}
|
||||
onClick={() => {
|
||||
handleOriginFilterChange(f);
|
||||
onOriginFilterChange(f);
|
||||
}}
|
||||
>
|
||||
{BAN_ORIGIN_FILTER_LABELS[f]}
|
||||
|
||||
@@ -24,7 +24,6 @@ import {
|
||||
CHART_PALETTE,
|
||||
resolveFluentToken,
|
||||
} from "../utils/chartTheme";
|
||||
import { useDashboardFilters } from "../providers/DashboardFilterProvider";
|
||||
import { useThemeMode } from "../providers/ThemeProvider";
|
||||
import { ChartStateWrapper } from "./ChartStateWrapper";
|
||||
import { ChartTooltip } from "./ChartTooltip";
|
||||
@@ -51,9 +50,9 @@ const MIN_CHART_HEIGHT = 180;
|
||||
/** Props for {@link JailDistributionChart}. */
|
||||
interface JailDistributionChartProps {
|
||||
/** Time-range preset controlling the query window. */
|
||||
timeRange?: TimeRange;
|
||||
timeRange: TimeRange;
|
||||
/** Origin filter controlling which bans are included. */
|
||||
origin?: BanOriginFilter;
|
||||
origin: BanOriginFilter;
|
||||
}
|
||||
|
||||
/** Internal chart data point shape. */
|
||||
@@ -136,11 +135,8 @@ export const JailDistributionChart = memo(function JailDistributionChart({
|
||||
origin,
|
||||
}: JailDistributionChartProps): React.JSX.Element {
|
||||
const styles = useStyles();
|
||||
const context = useDashboardFilters();
|
||||
const effectiveTimeRange = timeRange ?? context.timeRange;
|
||||
const effectiveOrigin = origin ?? context.originFilter;
|
||||
const { colorMode } = useThemeMode();
|
||||
const { jails, loading, error, reload } = useJailDistribution(effectiveTimeRange, effectiveOrigin);
|
||||
const { jails, loading, error, reload } = useJailDistribution(timeRange, origin);
|
||||
|
||||
const entries = buildEntries(jails);
|
||||
const chartHeight = Math.max(entries.length * BAR_HEIGHT_PX, MIN_CHART_HEIGHT);
|
||||
|
||||
@@ -55,7 +55,7 @@ beforeEach(() => {
|
||||
describe("BanTrendChart", () => {
|
||||
it("shows a spinner while loading", () => {
|
||||
mockHook({ loading: true });
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" />);
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" source="fail2ban" />);
|
||||
expect(screen.getByRole("progressbar")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
@@ -63,7 +63,7 @@ describe("BanTrendChart", () => {
|
||||
const reload = vi.fn();
|
||||
mockHook({ error: "Failed to fetch trend", reload });
|
||||
const user = userEvent.setup();
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" />);
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" source="fail2ban" />);
|
||||
expect(screen.getByText("Failed to fetch trend")).toBeInTheDocument();
|
||||
await user.click(screen.getByRole("button", { name: /retry/i }));
|
||||
expect(reload).toHaveBeenCalledOnce();
|
||||
@@ -75,7 +75,7 @@ describe("BanTrendChart", () => {
|
||||
{ timestamp: "2024-01-01T01:00:00Z", count: 0 },
|
||||
];
|
||||
mockHook({ buckets: emptyBuckets });
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" />);
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" source="fail2ban" />);
|
||||
expect(screen.getByText("No bans in this time range.")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
@@ -85,13 +85,13 @@ describe("BanTrendChart", () => {
|
||||
{ timestamp: "2024-01-01T01:00:00Z", count: 12 },
|
||||
];
|
||||
mockHook({ buckets });
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" />);
|
||||
wrap(<BanTrendChart timeRange="24h" origin="all" source="fail2ban" />);
|
||||
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 };
|
||||
const props = { timeRange: "24h" as const, origin: "all" as const, source: "fail2ban" as const };
|
||||
mockHook({ buckets: [{ timestamp: "2024-01-01T00:00:00Z", count: 5 }] });
|
||||
|
||||
const { rerender } = wrap(<BanTrendChart {...props} />);
|
||||
|
||||
@@ -73,7 +73,7 @@ const useStyles = makeStyles({
|
||||
*/
|
||||
function DashboardPageContent(): React.JSX.Element {
|
||||
const styles = useStyles();
|
||||
const { timeRange, originFilter, source } = useDashboardFilters();
|
||||
const { timeRange, originFilter, source, setTimeRange, setOriginFilter } = useDashboardFilters();
|
||||
|
||||
const { countries, countryNames, loading: countryLoading, error: countryError, reload: reloadCountry } =
|
||||
useDashboardCountryData(timeRange, originFilter, source);
|
||||
@@ -91,7 +91,12 @@ function DashboardPageContent(): React.JSX.Element {
|
||||
{/* Global filter bar */}
|
||||
{/* ------------------------------------------------------------------ */}
|
||||
<div className={styles.filterRow}>
|
||||
<DashboardFilterBar />
|
||||
<DashboardFilterBar
|
||||
timeRange={timeRange}
|
||||
onTimeRangeChange={setTimeRange}
|
||||
originFilter={originFilter}
|
||||
onOriginFilterChange={setOriginFilter}
|
||||
/>
|
||||
</div>
|
||||
|
||||
{/* ------------------------------------------------------------------ */}
|
||||
@@ -104,7 +109,11 @@ function DashboardPageContent(): React.JSX.Element {
|
||||
</Text>
|
||||
</div>
|
||||
<div className={styles.tabContent}>
|
||||
<BanTrendChart />
|
||||
<BanTrendChart
|
||||
timeRange={timeRange}
|
||||
origin={originFilter}
|
||||
source={source}
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -155,7 +164,11 @@ function DashboardPageContent(): React.JSX.Element {
|
||||
|
||||
{/* Ban table */}
|
||||
<div className={styles.tabContent}>
|
||||
<BanTable />
|
||||
<BanTable
|
||||
timeRange={timeRange}
|
||||
origin={originFilter}
|
||||
source={source}
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -4,9 +4,19 @@ import userEvent from "@testing-library/user-event";
|
||||
import { DashboardFilterBar } from "../../components/DashboardFilterBar";
|
||||
import { DashboardFilterProvider, useDashboardFilters } from "../DashboardFilterProvider";
|
||||
|
||||
function DashboardFilterSummary(): React.JSX.Element {
|
||||
const { timeRange, originFilter } = useDashboardFilters();
|
||||
return <div data-testid="dashboard-filters">{`${timeRange}-${originFilter}`}</div>;
|
||||
function DashboardFilterContent(): React.JSX.Element {
|
||||
const { timeRange, originFilter, setTimeRange, setOriginFilter } = useDashboardFilters();
|
||||
return (
|
||||
<>
|
||||
<DashboardFilterBar
|
||||
timeRange={timeRange}
|
||||
onTimeRangeChange={setTimeRange}
|
||||
originFilter={originFilter}
|
||||
onOriginFilterChange={setOriginFilter}
|
||||
/>
|
||||
<div data-testid="dashboard-filters">{`${timeRange}-${originFilter}`}</div>
|
||||
</>
|
||||
);
|
||||
}
|
||||
|
||||
describe("DashboardFilterProvider", () => {
|
||||
@@ -15,8 +25,7 @@ describe("DashboardFilterProvider", () => {
|
||||
|
||||
render(
|
||||
<DashboardFilterProvider>
|
||||
<DashboardFilterBar />
|
||||
<DashboardFilterSummary />
|
||||
<DashboardFilterContent />
|
||||
</DashboardFilterProvider>,
|
||||
);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user