fix: KVEditor effect dependency uses stable JSON serialization
Replace the flawed join-based comparison (entryKeys.join(',')) with
JSON.stringify() to properly handle keys containing commas. The previous
implementation could produce false equality when different key sets
shared the same comma-separated representation (e.g., 'a,b' key vs
separate 'a' and 'b' keys).
This ensures the effect fires correctly when keys change, fixing silent
failures to update derived state.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1,23 +1,3 @@
|
|||||||
### TASK-PERF-02 — `useSchedule` Exposes No `refresh` Function
|
|
||||||
|
|
||||||
**Where found**
|
|
||||||
`frontend/src/hooks/useSchedule.ts`. The hook fetches the import schedule on mount and exposes no way to re-fetch. After a `PUT /api/blocklists/schedule` from a different component or tab, the displayed schedule data stays stale until the user navigates away and back.
|
|
||||||
|
|
||||||
**Goal**
|
|
||||||
Expose a `refresh` callback from `useSchedule`, following the same pattern as `useListData` and other hooks. The `BlocklistsPage` (or whichever component saves schedule changes) should call `refresh()` after a successful save, and after `runImportNow()` completes.
|
|
||||||
|
|
||||||
**Possible traps and issues**
|
|
||||||
- `useSchedule` currently uses a simple `useEffect` on mount. Adding `refresh` means converting the internal fetch into a `useCallback` and calling it from the effect.
|
|
||||||
- Ensure the `AbortController` pattern is applied correctly when adding `refresh`.
|
|
||||||
|
|
||||||
**Docs changes needed**
|
|
||||||
None required.
|
|
||||||
|
|
||||||
**Why this is needed**
|
|
||||||
After saving a new schedule the user sees the old schedule until they reload the page. This makes the save feel broken even when it succeeded.
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### TASK-QUALITY-01 — `KVEditor` Uses `entryKeys.join(",")` as Effect Dependency
|
### TASK-QUALITY-01 — `KVEditor` Uses `entryKeys.join(",")` as Effect Dependency
|
||||||
|
|
||||||
**Where found**
|
**Where found**
|
||||||
|
|||||||
@@ -12,7 +12,7 @@ export function KVEditor({ entries, onChange }: KVEditorProps): React.JSX.Elemen
|
|||||||
const styles = useConfigStyles();
|
const styles = useConfigStyles();
|
||||||
const rows = useMemo(() => Object.entries(entries), [entries]);
|
const rows = useMemo(() => Object.entries(entries), [entries]);
|
||||||
const entryKeys = useMemo(() => Object.keys(entries), [entries]);
|
const entryKeys = useMemo(() => Object.keys(entries), [entries]);
|
||||||
const entryKeyList = entryKeys.join(",");
|
const entryKeysJson = useMemo(() => JSON.stringify(entryKeys), [entryKeys]);
|
||||||
const [editedKeys, setEditedKeys] = useState<Record<string, string>>(
|
const [editedKeys, setEditedKeys] = useState<Record<string, string>>(
|
||||||
Object.fromEntries(rows.map(([key]) => [key, key])),
|
Object.fromEntries(rows.map(([key]) => [key, key])),
|
||||||
);
|
);
|
||||||
@@ -27,7 +27,7 @@ export function KVEditor({ entries, onChange }: KVEditorProps): React.JSX.Elemen
|
|||||||
.map((key) => [key, previousErrors[key] ?? ""]),
|
.map((key) => [key, previousErrors[key] ?? ""]),
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
}, [entryKeyList, rows, entryKeys]);
|
}, [entryKeysJson, rows, entryKeys]);
|
||||||
|
|
||||||
const validateKey = (oldKey: string, newKey: string): string | null => {
|
const validateKey = (oldKey: string, newKey: string): string | null => {
|
||||||
const trimmedKey = newKey.trim();
|
const trimmedKey = newKey.trim();
|
||||||
|
|||||||
@@ -34,4 +34,27 @@ describe("KVEditor", () => {
|
|||||||
|
|
||||||
expect(handleChange).toHaveBeenCalledWith({ primary: "1", second: "2" });
|
expect(handleChange).toHaveBeenCalledWith({ primary: "1", second: "2" });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("correctly distinguishes keys with commas from separate keys", () => {
|
||||||
|
const handleChange = vi.fn();
|
||||||
|
const { rerender } = render(
|
||||||
|
<FluentProvider theme={webLightTheme}>
|
||||||
|
<KVEditor entries={{ "a,b": "1", c: "2" }} onChange={handleChange} />
|
||||||
|
</FluentProvider>,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(screen.getByLabelText(/Setting name: a,b/i)).toBeInTheDocument();
|
||||||
|
expect(screen.getByLabelText(/Setting name: c/i)).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Verify that changing to separate keys is recognized as a different state
|
||||||
|
rerender(
|
||||||
|
<FluentProvider theme={webLightTheme}>
|
||||||
|
<KVEditor entries={{ a: "1", b: "1", c: "2" }} onChange={handleChange} />
|
||||||
|
</FluentProvider>,
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(screen.getByLabelText(/Setting name: a/i)).toBeInTheDocument();
|
||||||
|
expect(screen.getByLabelText(/Setting name: b/i)).toBeInTheDocument();
|
||||||
|
expect(screen.getByLabelText(/Setting name: c/i)).toBeInTheDocument();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user