diff --git a/__tests__/components/editor/DeleteImpactAnalysis.test.tsx b/__tests__/components/editor/DeleteImpactAnalysis.test.tsx index afda71da..1d91576f 100644 --- a/__tests__/components/editor/DeleteImpactAnalysis.test.tsx +++ b/__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( { /> ); 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( + + ); + 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( + + ); + 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); }); }); diff --git a/__tests__/components/editor/PropertyDetailPanel.test.tsx b/__tests__/components/editor/PropertyDetailPanel.test.tsx index f62312f3..6f904f46 100644 --- a/__tests__/components/editor/PropertyDetailPanel.test.tsx +++ b/__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( + + ); + expect(screen.queryByTestId("auto-save-bar")).toBeNull(); + + rerender( + + ); + + 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 () => { diff --git a/__tests__/lib/hooks/useEntityAutoSave.test.ts b/__tests__/lib/hooks/useEntityAutoSave.test.ts index d065a5cd..6ebd2b8b 100644 --- a/__tests__/lib/hooks/useEntityAutoSave.test.ts +++ b/__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(); diff --git a/app/page.tsx b/app/page.tsx index a2817afb..b05c22da 100644 --- a/app/page.tsx +++ b/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(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 ( <> diff --git a/app/projects/[id]/editor/page.tsx b/app/projects/[id]/editor/page.tsx index 186e3773..0e94ffaa 100644 --- a/app/projects/[id]/editor/page.tsx +++ b/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(null); const [deleteTargetLabel, setDeleteTargetLabel] = useState(""); - 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} > + + + + + ); +} + +type AddEntityFormProps = Omit; + +function AddEntityForm({ + onOpenChange, + onConfirm, + iriPattern, + nextNumeric, + ontologyNamespace, + parentIri, + parentLabel, +}: AddEntityFormProps) { const [label, setLabel] = useState(""); const [entityType, setEntityType] = useState("class"); - const [iri, setIri] = useState(""); const [showAdvanced, setShowAdvanced] = useState(false); + /** Set only when the user hand-edits the IRI field; otherwise the IRI is derived. */ + const [iriOverride, setIriOverride] = useState(null); const inputRef = useRef(null); - const iriManuallyEdited = useRef(false); - - // Generate a stable UUID IRI once when the dialog opens - const stableUuidIriRef = useRef(""); - - // Generate IRI based on current state - const generateIri = useCallback( - (currentLabel: string) => { - switch (iriPattern) { - case "named": - if (currentLabel.trim()) { - return ontologyNamespace + labelToLocalName(currentLabel); - } - return ontologyNamespace + "..."; - case "numeric": - return ontologyNamespace + String(nextNumeric ?? 1); - case "uuid": - default: - return stableUuidIriRef.current; - } - }, - [iriPattern, nextNumeric, ontologyNamespace], - ); - - // Reset state when dialog opens - useEffect(() => { - if (open) { - setLabel(""); - setEntityType(parentIri ? "class" : "class"); - setShowAdvanced(false); - iriManuallyEdited.current = false; - // Generate a fresh UUID IRI for this dialog session - stableUuidIriRef.current = ontologyNamespace + uuidToBase62(); + // One stable UUID per dialog session. The lazy initializer runs once per + // mount, and a mount is exactly one opening of the dialog. + const [stableUuidIri] = useState(() => ontologyNamespace + uuidToBase62()); - // Set initial IRI - const initialIri = iriPattern === "uuid" - ? stableUuidIriRef.current - : iriPattern === "numeric" - ? ontologyNamespace + String(nextNumeric ?? 1) + const generatedIri = useMemo(() => { + switch (iriPattern) { + case "named": + return label.trim() + ? ontologyNamespace + labelToLocalName(label) : ontologyNamespace + "..."; - setIri(initialIri); - - setTimeout(() => inputRef.current?.focus(), 50); + case "numeric": + return ontologyNamespace + String(nextNumeric ?? 1); + case "uuid": + default: + return stableUuidIri; } - }, [open, iriPattern, nextNumeric, ontologyNamespace, parentIri]); + }, [iriPattern, label, nextNumeric, ontologyNamespace, stableUuidIri]); + + const iri = iriOverride ?? generatedIri; - // Update IRI reactively when label changes (named pattern only) + // Focus the label input once the dialog content is on screen. This is a DOM + // side effect, not derived state, so an effect is the right tool. useEffect(() => { - if (!open) return; - if (iriPattern === "named" && !iriManuallyEdited.current) { - setIri(generateIri(label)); - } - }, [label, open, iriPattern, generateIri]); + const timer = setTimeout(() => inputRef.current?.focus(), 50); + return () => clearTimeout(timer); + }, []); const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); @@ -150,136 +158,129 @@ export function AddEntityDialog({ : null); return ( - - -
- - - - Add Entity - - -

- {parentIri ? ( - <> - Create a new subclass of{" "} - - {parentDisplayName} - - - ) : ( - "Create a new entity in this ontology" - )} -

- - + + + + + Add Entity + + +

+ {parentIri ? ( + <> + Create a new subclass of{" "} + + {parentDisplayName} + + + ) : ( + "Create a new entity in this ontology" + )} +

+ + -
- {/* Label input */} -
+
+ {/* Label input */} +
+ + setLabel(e.target.value)} + onKeyDown={handleKeyDown} + placeholder="e.g., Privileged Altar" + className="w-full rounded-md border border-slate-300 bg-white px-3 py-2 text-sm placeholder:text-slate-400 focus:border-primary-500 focus:outline-hidden focus:ring-1 focus:ring-primary-500 dark:border-slate-600 dark:bg-slate-700 dark:text-slate-100 dark:placeholder:text-slate-500" + autoComplete="off" + /> +
+ + {/* Entity type select */} +
+ + + {parentIri && ( +

+ Type is locked to Class when creating a subclass. +

+ )} +
+ + {/* Advanced: IRI */} +
+ + {showAdvanced && ( +
setLabel(e.target.value)} - onKeyDown={handleKeyDown} - placeholder="e.g., Privileged Altar" - className="w-full rounded-md border border-slate-300 bg-white px-3 py-2 text-sm placeholder:text-slate-400 focus:border-primary-500 focus:outline-hidden focus:ring-1 focus:ring-primary-500 dark:border-slate-600 dark:bg-slate-700 dark:text-slate-100 dark:placeholder:text-slate-500" - autoComplete="off" + value={iri} + onChange={(e) => setIriOverride(e.target.value)} + className="w-full rounded-md border border-slate-300 bg-white px-3 py-2 font-mono text-xs placeholder:text-slate-400 focus:border-primary-500 focus:outline-hidden focus:ring-1 focus:ring-primary-500 dark:border-slate-600 dark:bg-slate-700 dark:text-slate-100 dark:placeholder:text-slate-500" /> +

+ {iriPattern === "uuid" && "Auto-generated UUID-based IRI"} + {iriPattern === "numeric" && `Sequential numeric IRI (next: ${nextNumeric ?? 1})`} + {iriPattern === "named" && "Derived from label"} +

+ )} +
+
- {/* Entity type select */} -
- - - {parentIri && ( -

- Type is locked to Class when creating a subclass. -

- )} -
- - {/* Advanced: IRI */} -
- - {showAdvanced && ( -
- - { - iriManuallyEdited.current = true; - setIri(e.target.value); - }} - className="w-full rounded-md border border-slate-300 bg-white px-3 py-2 font-mono text-xs placeholder:text-slate-400 focus:border-primary-500 focus:outline-hidden focus:ring-1 focus:ring-primary-500 dark:border-slate-600 dark:bg-slate-700 dark:text-slate-100 dark:placeholder:text-slate-500" - /> -

- {iriPattern === "uuid" && "Auto-generated UUID-based IRI"} - {iriPattern === "numeric" && `Sequential numeric IRI (next: ${nextNumeric ?? 1})`} - {iriPattern === "named" && "Derived from label"} -

-
- )} -
-
- - - - - - - -
+ + + + + ); } diff --git a/components/editor/DeleteImpactAnalysis.tsx b/components/editor/DeleteImpactAnalysis.tsx index 6f0442bb..36561a4e 100644 --- a/components/editor/DeleteImpactAnalysis.tsx +++ b/components/editor/DeleteImpactAnalysis.tsx @@ -1,10 +1,10 @@ "use client"; -import { useState, useEffect } from "react"; +import { useState } from "react"; import { AlertTriangle } from "lucide-react"; -import { qualityApi } from "@/lib/api/quality"; +import { useCrossReferences } from "@/lib/hooks/useCrossReferences"; import { getLocalName } from "@/lib/utils"; -import type { CrossReferencesResponse, CrossReferenceGroup } from "@/lib/ontology/qualityTypes"; +import type { CrossReferenceGroup } from "@/lib/ontology/qualityTypes"; interface DeleteImpactAnalysisProps { projectId: string; @@ -21,36 +21,19 @@ export function DeleteImpactAnalysis({ branch, onAcknowledge, }: DeleteImpactAnalysisProps) { - const [data, setData] = useState(null); - const [isLoading, setIsLoading] = useState(false); - const [fetchError, setFetchError] = useState(false); const [isExpanded, setIsExpanded] = useState(false); const [acknowledged, setAcknowledged] = useState(false); - useEffect(() => { - if (!entityIri || !projectId) { - setData(null); - setIsExpanded(false); - setAcknowledged(false); - onAcknowledge(false); - setIsLoading(false); - setFetchError(false); - return; - } - setIsLoading(true); - setAcknowledged(false); - onAcknowledge(false); - setFetchError(false); - qualityApi - .getCrossReferences(projectId, entityIri, accessToken, branch) - .then(setData) - .catch(() => setFetchError(true)) - .finally(() => setIsLoading(false)); - }, [entityIri, projectId, accessToken, branch]); // eslint-disable-line react-hooks/exhaustive-deps + // React Query owns the fetch and its loading/error state, so nothing needs + // resetting on prop change. The parent additionally keys this component by + // entityIri, so `isExpanded` / `acknowledged` start fresh for each target. + const { data, isPending, isError } = useCrossReferences(projectId, entityIri, accessToken, branch); const total = data?.total ?? 0; - if (isLoading) { + if (!entityIri || !projectId) return null; + + if (isPending) { return (
@@ -59,7 +42,7 @@ export function DeleteImpactAnalysis({ ); } - if (fetchError) { + if (isError) { return (
diff --git a/components/editor/PropertyDetailPanel.tsx b/components/editor/PropertyDetailPanel.tsx index d67eabeb..c0a3dea1 100644 --- a/components/editor/PropertyDetailPanel.tsx +++ b/components/editor/PropertyDetailPanel.tsx @@ -121,7 +121,10 @@ export function PropertyDetailPanel({ const [editRelationships, setEditRelationships] = useState([]); const [editPropertyType, setEditPropertyType] = useState("object"); - const editInitializedRef = useRef(false); + // Whether the panel has already seeded its edit state for this mount. State + // rather than a ref because it is both read and written during render. + const [autoEnterDone, setAutoEnterDone] = useState(false); + const [restoredDraftApplied, setRestoredDraftApplied] = useState(false); const toast = useToast(); // Build draft entry for auto-save @@ -267,7 +270,7 @@ export function PropertyDetailPanel({ const enterEditMode = useCallback(() => { if (!detail) return; initEditState(detail); - editInitializedRef.current = true; + setAutoEnterDone(true); setIsEditing(true); }, [detail, initEditState]); @@ -283,13 +286,23 @@ export function PropertyDetailPanel({ await flushToGit(); }, [triggerSave, flushToGit]); - // Auto-enter edit mode - useEffect(() => { - if (isEditing || editInitializedRef.current) return; - if (!canEdit || !onUpdateProperty || !detail) return; - - if (restoredDraft && restoredDraft.entityType === "property" && propertyIri) { - const d = restoredDraft as PropertyDraftEntry; + // Auto-enter edit mode, seeding from a restored draft when there is one. + // + // This is done during render rather than in an effect. `detail` is parsed + // synchronously from `sourceContent` and `restoredDraft` is read + // synchronously from the draft store, so both values are already known on the + // render that first satisfies the conditions — there is nothing to wait for. + // Adjusting state here rather than after the commit means the panel never + // paints an empty editor before the values arrive. + // See https://react.dev/learn/you-might-not-need-an-effect + const draftToRestore = + restoredDraft && restoredDraft.entityType === "property" && propertyIri + ? (restoredDraft as PropertyDraftEntry) + : null; + if (!isEditing && !autoEnterDone && canEdit && onUpdateProperty && detail) { + if (draftToRestore) { + setAutoEnterDone(true); + const d = draftToRestore; setEditPropertyType(d.propertyType); setEditLabels(d.labels); setEditComments(ensureTrailingEmpty(d.comments)); @@ -301,14 +314,20 @@ export function PropertyDetailPanel({ setEditCharacteristics(d.characteristics); setEditAnnotations(d.annotations); setEditRelationships(d.relationships); - editInitializedRef.current = true; + // `clearRestoredDraft` updates state owned by useEntityAutoSave, so it + // cannot run during render — deferred to the effect below. + setRestoredDraftApplied(true); setIsEditing(true); - clearRestoredDraft(); - return; + } else { + enterEditMode(); } + } - enterEditMode(); - }, [detail, canEdit, restoredDraft, propertyIri, clearRestoredDraft, onUpdateProperty, isEditing, enterEditMode]); + // Tell the draft store the restored draft has been taken up. This only + // synchronises with the store; it derives nothing. + useEffect(() => { + if (restoredDraftApplied) clearRestoredDraft(); + }, [restoredDraftApplied, clearRestoredDraft]); // ── Edit helpers ── const updateLabel = useCallback((index: number, field: "value" | "lang", val: string) => { diff --git a/lib/hooks/useEntityAutoSave.ts b/lib/hooks/useEntityAutoSave.ts index 0945e14f..1add8ba2 100644 --- a/lib/hooks/useEntityAutoSave.ts +++ b/lib/hooks/useEntityAutoSave.ts @@ -57,7 +57,16 @@ export function useEntityAutoSave({ const [saveStatus, setSaveStatus] = useState("idle"); const [saveError, setSaveError] = useState(null); const [validationError, setValidationError] = useState(null); - const [restoredDraft, setRestoredDraft] = useState(null); + // Seeded synchronously so the draft is available on the very first render. + // The store is backed by localStorage, so the lookup is a plain read — there + // is nothing to wait for, and consumers that initialise their edit state from + // a restored draft would otherwise miss it on the render where their own data + // first becomes available. + const [restoredDraft, setRestoredDraft] = useState(() => + entityIri && branch + ? useDraftStore.getState().getDraft(draftKey(projectId, branch, entityIri)) ?? null + : null, + ); const flushingRef = useRef(false); const savedTimerRef = useRef | null>(null);