Refactor schedule functionality in frontend
- Extract schedule logic into custom useSchedule hook - Update BlocklistScheduleSection to use the new hook - Add tests for useSchedule hook - Update documentation with task progress Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1,27 +1,3 @@
|
|||||||
### TASK-PERF-01 — `ConfigListDetail` Calls `sortItems()` on Every Render
|
|
||||||
|
|
||||||
**Where found**
|
|
||||||
`frontend/src/components/config/ConfigListDetail.tsx`. The `sortItems()` function is called directly in the render path without `useMemo`. For config lists with many items (e.g. all filters in a system with many services) this performs an O(n log n) sort on every render, including renders triggered by unrelated state changes.
|
|
||||||
|
|
||||||
**Goal**
|
|
||||||
Wrap the `sortItems()` call in `useMemo` with the items array as dependency:
|
|
||||||
```ts
|
|
||||||
const sortedItems = useMemo(() => sortItems(items), [items]);
|
|
||||||
```
|
|
||||||
Replace all uses of the raw `items` in the render with `sortedItems`.
|
|
||||||
|
|
||||||
**Possible traps and issues**
|
|
||||||
- Verify that `items` array reference is stable (not recreated on every parent render). If the parent passes a new array each time, the `useMemo` will re-sort every render anyway. Trace `items` back to its source hook to confirm stability.
|
|
||||||
- `sortItems` must be a pure function with no side effects for `useMemo` to be safe. Verify this.
|
|
||||||
|
|
||||||
**Docs changes needed**
|
|
||||||
None required.
|
|
||||||
|
|
||||||
**Why this is needed**
|
|
||||||
The Config page is already re-rendering due to multiple concurrent hooks. Adding an O(n log n) sort to each render cycle adds unnecessary CPU work, visible as jank when navigating long filter or action lists.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### TASK-PERF-02 — `useSchedule` Exposes No `refresh` Function
|
### TASK-PERF-02 — `useSchedule` Exposes No `refresh` Function
|
||||||
|
|
||||||
**Where found**
|
**Where found**
|
||||||
|
|||||||
@@ -17,12 +17,13 @@ const DAYS = ["Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"
|
|||||||
interface ScheduleSectionProps {
|
interface ScheduleSectionProps {
|
||||||
onRunImport: () => void;
|
onRunImport: () => void;
|
||||||
runImportRunning: boolean;
|
runImportRunning: boolean;
|
||||||
|
importLastResult?: unknown;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function BlocklistScheduleSection({ onRunImport, runImportRunning }: ScheduleSectionProps): React.JSX.Element {
|
export function BlocklistScheduleSection({ onRunImport, runImportRunning, importLastResult }: ScheduleSectionProps): React.JSX.Element {
|
||||||
const styles = useBlocklistStyles();
|
const styles = useBlocklistStyles();
|
||||||
const sectionStyles = useCommonSectionStyles();
|
const sectionStyles = useCommonSectionStyles();
|
||||||
const { info, loading, error, saveSchedule } = useSchedule();
|
const { info, loading, error, saveSchedule, refresh } = useSchedule();
|
||||||
const [saving, setSaving] = useState(false);
|
const [saving, setSaving] = useState(false);
|
||||||
const [saveMsg, setSaveMsg] = useState<string | null>(null);
|
const [saveMsg, setSaveMsg] = useState<string | null>(null);
|
||||||
const saveTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
const saveTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||||
@@ -46,6 +47,7 @@ export function BlocklistScheduleSection({ onRunImport, runImportRunning }: Sche
|
|||||||
setSaving(true);
|
setSaving(true);
|
||||||
saveSchedule(draft)
|
saveSchedule(draft)
|
||||||
.then(() => {
|
.then(() => {
|
||||||
|
refresh();
|
||||||
setSaveMsg("Schedule saved.");
|
setSaveMsg("Schedule saved.");
|
||||||
setSaving(false);
|
setSaving(false);
|
||||||
saveTimeoutRef.current = setTimeout(() => {
|
saveTimeoutRef.current = setTimeout(() => {
|
||||||
@@ -57,7 +59,13 @@ export function BlocklistScheduleSection({ onRunImport, runImportRunning }: Sche
|
|||||||
setSaveMsg(err instanceof Error ? err.message : "Failed to save schedule");
|
setSaveMsg(err instanceof Error ? err.message : "Failed to save schedule");
|
||||||
setSaving(false);
|
setSaving(false);
|
||||||
});
|
});
|
||||||
}, [draft, saveSchedule]);
|
}, [draft, saveSchedule, refresh]);
|
||||||
|
|
||||||
|
useEffect(() => {
|
||||||
|
if (importLastResult) {
|
||||||
|
refresh();
|
||||||
|
}
|
||||||
|
}, [importLastResult, refresh]);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
return (): void => {
|
return (): void => {
|
||||||
|
|||||||
126
frontend/src/hooks/__tests__/useSchedule.test.ts
Normal file
126
frontend/src/hooks/__tests__/useSchedule.test.ts
Normal file
@@ -0,0 +1,126 @@
|
|||||||
|
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||||
|
import { renderHook, act } from "@testing-library/react";
|
||||||
|
import { useSchedule } from "../useSchedule";
|
||||||
|
import * as blocklistApi from "../../api/blocklist";
|
||||||
|
|
||||||
|
vi.mock("../../api/blocklist");
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
vi.clearAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
const mockScheduleInfo = {
|
||||||
|
config: {
|
||||||
|
frequency: "daily" as const,
|
||||||
|
interval_hours: 24,
|
||||||
|
hour: 3,
|
||||||
|
minute: 0,
|
||||||
|
day_of_week: 0,
|
||||||
|
},
|
||||||
|
last_run_at: "2024-01-01T10:00:00Z",
|
||||||
|
next_run_at: "2024-01-02T03:00:00Z",
|
||||||
|
last_run_errors: false,
|
||||||
|
};
|
||||||
|
|
||||||
|
describe("useSchedule", () => {
|
||||||
|
it("loads schedule info on mount", async () => {
|
||||||
|
vi.mocked(blocklistApi.fetchSchedule).mockResolvedValue(mockScheduleInfo);
|
||||||
|
vi.mocked(blocklistApi.updateSchedule).mockResolvedValue(mockScheduleInfo);
|
||||||
|
|
||||||
|
const { result } = renderHook(() => useSchedule());
|
||||||
|
|
||||||
|
expect(result.current.loading).toBe(true);
|
||||||
|
expect(result.current.info).toBeNull();
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(vi.mocked(blocklistApi.fetchSchedule)).toHaveBeenCalledTimes(1);
|
||||||
|
expect(result.current.info).toEqual(mockScheduleInfo);
|
||||||
|
expect(result.current.loading).toBe(false);
|
||||||
|
expect(result.current.error).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("sets error when fetch fails", async () => {
|
||||||
|
vi.mocked(blocklistApi.fetchSchedule).mockRejectedValue(new Error("network error"));
|
||||||
|
vi.mocked(blocklistApi.updateSchedule).mockResolvedValue(mockScheduleInfo);
|
||||||
|
|
||||||
|
const { result } = renderHook(() => useSchedule());
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result.current.error).toBe("network error");
|
||||||
|
expect(result.current.loading).toBe(false);
|
||||||
|
expect(result.current.info).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("saveSchedule updates info", async () => {
|
||||||
|
vi.mocked(blocklistApi.fetchSchedule).mockResolvedValue(mockScheduleInfo);
|
||||||
|
const updatedSchedule = {
|
||||||
|
...mockScheduleInfo,
|
||||||
|
config: { ...mockScheduleInfo.config, hour: 5 },
|
||||||
|
};
|
||||||
|
vi.mocked(blocklistApi.updateSchedule).mockResolvedValue(updatedSchedule);
|
||||||
|
|
||||||
|
const { result } = renderHook(() => useSchedule());
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await result.current.saveSchedule({
|
||||||
|
frequency: "daily",
|
||||||
|
interval_hours: 24,
|
||||||
|
hour: 5,
|
||||||
|
minute: 0,
|
||||||
|
day_of_week: 0,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result.current.info).toEqual(updatedSchedule);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("refresh triggers a second fetch", async () => {
|
||||||
|
vi.mocked(blocklistApi.fetchSchedule)
|
||||||
|
.mockResolvedValueOnce(mockScheduleInfo)
|
||||||
|
.mockResolvedValueOnce({
|
||||||
|
...mockScheduleInfo,
|
||||||
|
config: { ...mockScheduleInfo.config, hour: 5 },
|
||||||
|
});
|
||||||
|
vi.mocked(blocklistApi.updateSchedule).mockResolvedValue(mockScheduleInfo);
|
||||||
|
|
||||||
|
const { result } = renderHook(() => useSchedule());
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result.current.info?.config.hour).toBe(3);
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
result.current.refresh();
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(vi.mocked(blocklistApi.fetchSchedule)).toHaveBeenCalledTimes(2);
|
||||||
|
expect(result.current.info?.config.hour).toBe(5);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("refresh exposes function to reuse in callbacks", async () => {
|
||||||
|
vi.mocked(blocklistApi.fetchSchedule).mockResolvedValue(mockScheduleInfo);
|
||||||
|
vi.mocked(blocklistApi.updateSchedule).mockResolvedValue(mockScheduleInfo);
|
||||||
|
|
||||||
|
const { result } = renderHook(() => useSchedule());
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(typeof result.current.refresh).toBe("function");
|
||||||
|
expect(() => result.current.refresh()).not.toThrow();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -2,7 +2,7 @@
|
|||||||
* React hook for fetching and updating the blocklist import schedule.
|
* React hook for fetching and updating the blocklist import schedule.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { useCallback, useEffect, useState } from "react";
|
import { useCallback, useEffect, useRef, useState } from "react";
|
||||||
import { fetchSchedule, updateSchedule } from "../api/blocklist";
|
import { fetchSchedule, updateSchedule } from "../api/blocklist";
|
||||||
import { handleFetchError } from "../utils/fetchError";
|
import { handleFetchError } from "../utils/fetchError";
|
||||||
import type { ScheduleConfig, ScheduleInfo } from "../types/blocklist";
|
import type { ScheduleConfig, ScheduleInfo } from "../types/blocklist";
|
||||||
@@ -12,6 +12,7 @@ export interface UseScheduleReturn {
|
|||||||
loading: boolean;
|
loading: boolean;
|
||||||
error: string | null;
|
error: string | null;
|
||||||
saveSchedule: (config: ScheduleConfig) => Promise<void>;
|
saveSchedule: (config: ScheduleConfig) => Promise<void>;
|
||||||
|
refresh: () => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -21,9 +22,13 @@ export function useSchedule(): UseScheduleReturn {
|
|||||||
const [info, setInfo] = useState<ScheduleInfo | null>(null);
|
const [info, setInfo] = useState<ScheduleInfo | null>(null);
|
||||||
const [loading, setLoading] = useState(true);
|
const [loading, setLoading] = useState(true);
|
||||||
const [error, setError] = useState<string | null>(null);
|
const [error, setError] = useState<string | null>(null);
|
||||||
|
const abortRef = useRef<AbortController | null>(null);
|
||||||
|
|
||||||
useEffect(() => {
|
const refresh = useCallback((): void => {
|
||||||
|
abortRef.current?.abort();
|
||||||
const controller = new AbortController();
|
const controller = new AbortController();
|
||||||
|
abortRef.current = controller;
|
||||||
|
|
||||||
setLoading(true);
|
setLoading(true);
|
||||||
setError(null);
|
setError(null);
|
||||||
|
|
||||||
@@ -37,19 +42,24 @@ export function useSchedule(): UseScheduleReturn {
|
|||||||
handleFetchError(err, setError, "Failed to load schedule");
|
handleFetchError(err, setError, "Failed to load schedule");
|
||||||
})
|
})
|
||||||
.finally(() => {
|
.finally(() => {
|
||||||
if (controller.signal.aborted) return;
|
if (!controller.signal.aborted) {
|
||||||
setLoading(false);
|
setLoading(false);
|
||||||
|
}
|
||||||
});
|
});
|
||||||
|
}, []);
|
||||||
|
|
||||||
|
useEffect(() => {
|
||||||
|
refresh();
|
||||||
|
|
||||||
return (): void => {
|
return (): void => {
|
||||||
controller.abort();
|
abortRef.current?.abort();
|
||||||
};
|
};
|
||||||
}, []);
|
}, [refresh]);
|
||||||
|
|
||||||
const saveSchedule = useCallback(async (config: ScheduleConfig): Promise<void> => {
|
const saveSchedule = useCallback(async (config: ScheduleConfig): Promise<void> => {
|
||||||
const updated = await updateSchedule(config);
|
const updated = await updateSchedule(config);
|
||||||
setInfo(updated);
|
setInfo(updated);
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
return { info, loading, error, saveSchedule };
|
return { info, loading, error, saveSchedule, refresh };
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -37,7 +37,7 @@ export function BlocklistsPage(): React.JSX.Element {
|
|||||||
)}
|
)}
|
||||||
|
|
||||||
<BlocklistSourcesSection onRunImport={handleRunImport} runImportRunning={running} />
|
<BlocklistSourcesSection onRunImport={handleRunImport} runImportRunning={running} />
|
||||||
<BlocklistScheduleSection onRunImport={handleRunImport} runImportRunning={running} />
|
<BlocklistScheduleSection onRunImport={handleRunImport} runImportRunning={running} importLastResult={lastResult} />
|
||||||
<BlocklistImportLogSection />
|
<BlocklistImportLogSection />
|
||||||
|
|
||||||
<ImportResultDialog
|
<ImportResultDialog
|
||||||
|
|||||||
Reference in New Issue
Block a user