fix: eliminate setState-in-effect anti-patterns (Category A complete, #200) — fixes #364

__tests__/components/editor/DeleteImpactAnalysis.test.tsx

@@ -1,5 +1,6 @@
 import { describe, expect, it, vi, beforeEach } from "vitest";
-import { render, screen, waitFor } from "@testing-library/react";
+import { screen, waitFor } from "@testing-library/react";
+import { renderWithQueryClient as render } from "../../helpers/renderWithProviders";
 import userEvent from "@testing-library/user-event";
 
 vi.mock("@/lib/api/quality", () => ({
@@ -120,7 +121,7 @@ describe("DeleteImpactAnalysis", () => {
     expect(onAcknowledge).toHaveBeenCalledWith(true);
   });
 
-  it("resets state when entityIri is null", () => {
+  it("renders nothing and stays silent when entityIri is null", () => {
     mockGetCrossReferences.mockResolvedValue({ target_iri: "http://example.org/A", total: 0, groups: [] });
     const { container } = render(
       <DeleteImpactAnalysis
@@ -130,6 +131,56 @@ describe("DeleteImpactAnalysis", () => {
       />
     );
     expect(container.textContent).toBe("");
-    expect(onAcknowledge).toHaveBeenCalledWith(false);
+    // The parent owns the delete gate now, so the component no longer pushes
+    // state upward on mount or on prop change.
+    expect(onAcknowledge).not.toHaveBeenCalled();
+    expect(mockGetCrossReferences).not.toHaveBeenCalled();
+  });
+
+  it("does not ask for acknowledgement when the entity has no references", async () => {
+    mockGetCrossReferences.mockResolvedValue({ target_iri: "http://example.org/A", total: 0, groups: [] });
+    const { container } = render(
+      <DeleteImpactAnalysis
+        projectId="p1"
+        entityIri="http://example.org/A"
+        onAcknowledge={onAcknowledge}
+      />
+    );
+    await waitFor(() => {
+      expect(container.querySelector(".animate-spin")).toBeNull();
+    });
+    // Regression guard: the old effect fired onAcknowledge(false) on mount and
+    // nothing ever set it back to true, so Delete stayed disabled for an
+    // unreferenced entity. Nothing should be reported for the zero-ref case.
+    expect(onAcknowledge).not.toHaveBeenCalled();
+  });
+
+  it("reports acknowledgement only from the checkbox", async () => {
+    mockGetCrossReferences.mockResolvedValue({
+      target_iri: "http://example.org/A",
+      total: 1,
+      groups: [
+        {
+          context: "parent_iris",
+          context_label: "Parent",
+          references: [{ source_iri: "http://example.org/B", source_label: "B", source_type: "class", reference_context: "parent_iris" }],
+        },
+      ],
+    });
+    render(
+      <DeleteImpactAnalysis
+        projectId="p1"
+        entityIri="http://example.org/A"
+        onAcknowledge={onAcknowledge}
+      />
+    );
+    await waitFor(() => expect(screen.getByRole("checkbox")).toBeDefined());
+    expect(onAcknowledge).not.toHaveBeenCalled();
+
+    await userEvent.click(screen.getByRole("checkbox"));
+    expect(onAcknowledge).toHaveBeenLastCalledWith(true);
+
+    await userEvent.click(screen.getByRole("checkbox"));
+    expect(onAcknowledge).toHaveBeenLastCalledWith(false);
   });
 });

__tests__/components/editor/PropertyDetailPanel.test.tsx

@@ -735,6 +735,54 @@ describe("PropertyDetailPanel", () => {
     expect(mockClearRestoredDraft).toHaveBeenCalled();
   });
 
+  it("still applies a restored draft when the source arrives on a later render", () => {
+    autoSaveOverrides = {
+      restoredDraft: {
+        entityType: "property",
+        propertyType: "object",
+        labels: [{ value: "Late Restored Label", lang: "en" }],
+        comments: [],
+        definitions: [],
+        domainIris: [],
+        rangeIris: [],
+        parentIris: [],
+        inverseOf: null,
+        characteristics: [],
+        annotations: [],
+        relationships: [],
+        deprecated: false,
+        equivalentIris: [],
+        disjointIris: [],
+        updatedAt: Date.now(),
+      },
+    };
+    const onUpdateProperty = vi.fn();
+    // First render has no source, so `detail` is null and the panel cannot seed
+    // anything yet — this is the real-world ordering, where sourceContent loads
+    // after mount. The draft must survive to the render where detail appears.
+    const { rerender } = render(
+      <PropertyDetailPanel
+        {...DEFAULT_PROPS}
+        sourceContent=""
+        canEdit={true}
+        onUpdateProperty={onUpdateProperty}
+      />
+    );
+    expect(screen.queryByTestId("auto-save-bar")).toBeNull();
+
+    rerender(
+      <PropertyDetailPanel
+        {...DEFAULT_PROPS}
+        canEdit={true}
+        onUpdateProperty={onUpdateProperty}
+      />
+    );
+
+    expect(screen.getByTestId("auto-save-bar")).not.toBeNull();
+    expect(screen.getByDisplayValue("Late Restored Label")).toBeDefined();
+    expect(mockClearRestoredDraft).toHaveBeenCalled();
+  });
+
   // ── flushToGit on unmount ──
 
   it("flushes pending draft to git on unmount", async () => {

__tests__/lib/hooks/useEntityAutoSave.test.ts

@@ -14,15 +14,22 @@ const stableClearDraft = (key: string) => {
 };
 const stableGetDraft = (key: string) => mockDrafts[key];
 
-vi.mock("@/lib/stores/draftStore", () => ({
-  draftKey: (projectId: string, branch: string, iri: string) =>
-    `${projectId}:${branch}:${iri}`,
-  useDraftStore: () => ({
-    setDraft: stableSetDraft,
-    clearDraft: stableClearDraft,
-    getDraft: stableGetDraft,
-  }),
-}));
+const storeApi = {
+  setDraft: stableSetDraft,
+  clearDraft: stableClearDraft,
+  getDraft: stableGetDraft,
+};
+
+vi.mock("@/lib/stores/draftStore", () => {
+  // Mirrors zustand's shape: callable as a hook, plus a `getState` escape hatch
+  // for reads outside the React lifecycle (used to seed `restoredDraft`).
+  const useDraftStore = Object.assign(() => storeApi, { getState: () => storeApi });
+  return {
+    draftKey: (projectId: string, branch: string, iri: string) =>
+      `${projectId}:${branch}:${iri}`,
+    useDraftStore,
+  };
+});
 
 beforeEach(() => {
   for (const key of Object.keys(mockDrafts)) {
@@ -232,6 +239,23 @@ describe("useEntityAutoSave", () => {
     expect(result.current.restoredDraft).toBeNull();
   });
 
+  it("exposes the restored draft on the very first render, not after an effect", () => {
+    const key = "proj-1:main:http://example.org/myProp";
+    mockDrafts[key] = makeDraftEntry();
+
+    // renderHook's initial `result.current` is the value from the first render
+    // pass. Before the draft was seeded synchronously this was null and only
+    // became populated once the restore effect had run, which meant a consumer
+    // initialising its edit state on that first render silently missed it.
+    const { result } = renderHook(() => useEntityAutoSave(BASE_OPTIONS));
+    expect(result.current.restoredDraft).toEqual(mockDrafts[key]);
+  });
+
+  it("reports no restored draft on first render when the store is empty", () => {
+    const { result } = renderHook(() => useEntityAutoSave(BASE_OPTIONS));
+    expect(result.current.restoredDraft).toBeNull();
+  });
+
   it("restores draft on mount when one exists in the store", () => {
     const key = "proj-1:main:http://example.org/myProp";
     mockDrafts[key] = makeDraftEntry();

app/page.tsx

@@ -69,20 +69,24 @@ export default function HomePage() {
   const unfilteredTotal = data?.pages.at(-1)?.unfiltered_total ?? 0;
   const isFiltered = (!!debouncedSearch || filter !== "all") && unfilteredTotal > total;
 
-  const [nextPageError, setNextPageError] = useState<string | null>(null);
-
-  useEffect(() => {
-    setNextPageError(null);
-  }, [filter, debouncedSearch]);
+  // The "load more" error belongs to one particular list. Tagging it with that
+  // list's identity and deriving visibility lets a filter or search change
+  // discard it during render, instead of an effect clearing it afterwards.
+  const listKey = `${filter}\u0000${debouncedSearch}`;
+  const [pageError, setPageError] = useState<{ listKey: string; message: string } | null>(null);
+  const nextPageError = pageError?.listKey === listKey ? pageError.message : null;
 
   const handleLoadMore = useCallback(() => {
     if (hasNextPage && !isFetchingNextPage) {
-      setNextPageError(null);
+      setPageError(null);
       fetchNextPage().catch((err) => {
-        setNextPageError(err instanceof Error ? err.message : "Failed to load more projects");
+        setPageError({
+          listKey,
+          message: err instanceof Error ? err.message : "Failed to load more projects",
+        });
       });
     }
-  }, [hasNextPage, isFetchingNextPage, fetchNextPage]);
+  }, [hasNextPage, isFetchingNextPage, fetchNextPage, listKey]);
 
   return (
     <>

app/projects/[id]/editor/page.tsx

@@ -39,6 +39,7 @@ import { KeyboardShortcutDialog } from "@/components/editor/KeyboardShortcutDial
 import { SuggestionSubmitDialog } from "@/components/editor/SuggestionSubmitDialog";
 import { useSuggestionSession } from "@/lib/hooks/useSuggestionSession";
 import { useSuggestionBeacon } from "@/lib/hooks/useSuggestionBeacon";
+import { useCrossReferences } from "@/lib/hooks/useCrossReferences";
 import { DeleteImpactAnalysis } from "@/components/editor/DeleteImpactAnalysis";
 import { RemoteSyncIndicator } from "@/components/editor/RemoteSyncIndicator";
 import { ShareButton } from "@/components/editor/ShareButton";
@@ -178,7 +179,22 @@ export default function EditorPage() {
   const [deleteDialogOpen, setDeleteDialogOpen] = useState(false);
   const [deleteTargetIri, setDeleteTargetIri] = useState<string | null>(null);
   const [deleteTargetLabel, setDeleteTargetLabel] = useState<string>("");
-  const [deleteImpactAcknowledged, setDeleteImpactAcknowledged] = useState(true);
+  const [deleteImpactAcknowledged, setDeleteImpactAcknowledged] = useState(false);
+
+  // Delete gating. The impact check is read here as well as inside
+  // DeleteImpactAnalysis; React Query dedupes both observers onto one fetch.
+  // Deriving the gate here means the child never has to push state upward from
+  // an effect, and an entity with zero references is no longer blocked.
+  const deleteImpact = useCrossReferences(
+    projectId,
+    deleteDialogOpen ? deleteTargetIri : null,
+    session?.accessToken,
+    activeBranch,
+  );
+  const deleteNeedsAcknowledgement =
+    !!deleteTargetIri &&
+    deleteDialogOpen &&
+    (deleteImpact.isPending || deleteImpact.isError || (deleteImpact.data?.total ?? 0) > 0);
 
   // Toast
   const toast = useToast();
@@ -1205,16 +1221,17 @@ export default function EditorPage() {
         open={deleteDialogOpen}
         onOpenChange={(open) => {
           setDeleteDialogOpen(open);
-          if (!open) setDeleteImpactAcknowledged(true);
+          if (!open) setDeleteImpactAcknowledged(false);
         }}
         onConfirm={handleDeleteConfirm}
         title="Delete Class"
         description={`Are you sure you want to delete "${deleteTargetLabel}"? This action cannot be undone.`}
         confirmLabel="Delete"
         variant="danger"
-        confirmDisabled={!deleteImpactAcknowledged}
+        confirmDisabled={deleteNeedsAcknowledgement && !deleteImpactAcknowledged}
       >
         <DeleteImpactAnalysis
+          key={deleteTargetIri ?? "none"}
           projectId={projectId}
           entityIri={deleteTargetIri}
           accessToken={session?.accessToken}