From 9375430e02dd78612de237374d3c8824c2a2d120 Mon Sep 17 00:00:00 2001 From: Lukas Date: Thu, 23 Apr 2026 09:40:19 +0200 Subject: [PATCH] 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> --- Docs/Tasks.md | 24 ---- .../blocklist/BlocklistScheduleSection.tsx | 14 +- .../src/hooks/__tests__/useSchedule.test.ts | 126 ++++++++++++++++++ frontend/src/hooks/useSchedule.ts | 24 +++- frontend/src/pages/BlocklistsPage.tsx | 2 +- 5 files changed, 155 insertions(+), 35 deletions(-) create mode 100644 frontend/src/hooks/__tests__/useSchedule.test.ts diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 5613fc0..3c8bc97 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -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 **Where found** diff --git a/frontend/src/components/blocklist/BlocklistScheduleSection.tsx b/frontend/src/components/blocklist/BlocklistScheduleSection.tsx index b1f6671..c2e3a9f 100644 --- a/frontend/src/components/blocklist/BlocklistScheduleSection.tsx +++ b/frontend/src/components/blocklist/BlocklistScheduleSection.tsx @@ -17,12 +17,13 @@ const DAYS = ["Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday" interface ScheduleSectionProps { onRunImport: () => void; 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 sectionStyles = useCommonSectionStyles(); - const { info, loading, error, saveSchedule } = useSchedule(); + const { info, loading, error, saveSchedule, refresh } = useSchedule(); const [saving, setSaving] = useState(false); const [saveMsg, setSaveMsg] = useState(null); const saveTimeoutRef = useRef | null>(null); @@ -46,6 +47,7 @@ export function BlocklistScheduleSection({ onRunImport, runImportRunning }: Sche setSaving(true); saveSchedule(draft) .then(() => { + refresh(); setSaveMsg("Schedule saved."); setSaving(false); saveTimeoutRef.current = setTimeout(() => { @@ -57,7 +59,13 @@ export function BlocklistScheduleSection({ onRunImport, runImportRunning }: Sche setSaveMsg(err instanceof Error ? err.message : "Failed to save schedule"); setSaving(false); }); - }, [draft, saveSchedule]); + }, [draft, saveSchedule, refresh]); + + useEffect(() => { + if (importLastResult) { + refresh(); + } + }, [importLastResult, refresh]); useEffect(() => { return (): void => { diff --git a/frontend/src/hooks/__tests__/useSchedule.test.ts b/frontend/src/hooks/__tests__/useSchedule.test.ts new file mode 100644 index 0000000..916bd2b --- /dev/null +++ b/frontend/src/hooks/__tests__/useSchedule.test.ts @@ -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(); + }); +}); diff --git a/frontend/src/hooks/useSchedule.ts b/frontend/src/hooks/useSchedule.ts index d6316e2..aa1c5e6 100644 --- a/frontend/src/hooks/useSchedule.ts +++ b/frontend/src/hooks/useSchedule.ts @@ -2,7 +2,7 @@ * 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 { handleFetchError } from "../utils/fetchError"; import type { ScheduleConfig, ScheduleInfo } from "../types/blocklist"; @@ -12,6 +12,7 @@ export interface UseScheduleReturn { loading: boolean; error: string | null; saveSchedule: (config: ScheduleConfig) => Promise; + refresh: () => void; } /** @@ -21,9 +22,13 @@ export function useSchedule(): UseScheduleReturn { const [info, setInfo] = useState(null); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); + const abortRef = useRef(null); - useEffect(() => { + const refresh = useCallback((): void => { + abortRef.current?.abort(); const controller = new AbortController(); + abortRef.current = controller; + setLoading(true); setError(null); @@ -37,19 +42,24 @@ export function useSchedule(): UseScheduleReturn { handleFetchError(err, setError, "Failed to load schedule"); }) .finally(() => { - if (controller.signal.aborted) return; - setLoading(false); + if (!controller.signal.aborted) { + setLoading(false); + } }); + }, []); + + useEffect(() => { + refresh(); return (): void => { - controller.abort(); + abortRef.current?.abort(); }; - }, []); + }, [refresh]); const saveSchedule = useCallback(async (config: ScheduleConfig): Promise => { const updated = await updateSchedule(config); setInfo(updated); }, []); - return { info, loading, error, saveSchedule }; + return { info, loading, error, saveSchedule, refresh }; } diff --git a/frontend/src/pages/BlocklistsPage.tsx b/frontend/src/pages/BlocklistsPage.tsx index e1bbdfc..1255579 100644 --- a/frontend/src/pages/BlocklistsPage.tsx +++ b/frontend/src/pages/BlocklistsPage.tsx @@ -37,7 +37,7 @@ export function BlocklistsPage(): React.JSX.Element { )} - +