From 3ee37b396d4a41e04495c09f7f321bc36709f677 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Wed, 19 Aug 2026 00:52:37 +0200 Subject: [PATCH 1/2] fix: eliminate setState-in-effect anti-patterns (Category A, #200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of the six remaining Category A sites. Each replaces an effect that reached back in to reset or mirror state with the React-canonical alternative — remount, or derive during render. AddEntityDialog (2 sites) Form state moves into an inner AddEntityForm rendered inside DialogContent. Radix unmounts that content on close, so every opening mounts a fresh form and the reset-on-open effect disappears. The IRI is now derived (`iriOverride ?? generatedIri`) instead of being mirrored into state by a second effect watching `label`, and the per-session UUID comes from a useState lazy initializer. Focus-on-open stays an effect — it is a DOM side effect, not derived state. DeleteImpactAnalysis (1 site) Switched to the existing useCrossReferences React Query hook, which owns loading/error state, and the parent now keys the component by entityIri so `isExpanded` / `acknowledged` reset on their own. The component reports acknowledgement only from the checkbox. This also fixes a live bug. The old effect called `onAcknowledge(false)` on mount and nothing ever set it back to true, so for an entity with **zero** references the Delete button stayed permanently disabled — the common, safe case was unreachable. The gate is now derived in the editor page from the same query (React Query dedupes both observers onto one fetch), so it only engages when the check is still running, errored, or actually found references. Behaviour on pending/error is unchanged. app/page.tsx (1 site) The "load more" error is tagged with the identity of the list it belongs to, so a filter or search change discards it during render instead of an effect clearing it afterwards. `react-hooks/set-state-in-effect`: 15 warnings → 11. Not included, both deliberately: - PropertyDetailPanel.tsx:293 (restored-draft init) — PR #273 edits lines inside this same effect; sequence after it. - editor/page.tsx:311 (prefix-map detection) — removing the effect means choosing between the current one-shot latch and live derivation, which changes when IRIs are minted. Needs a maintainer call, and it is really a one-shot init (Category D shape) rather than a reset-on-prop-change. Co-Authored-By: Claude Opus 5 (1M context) --- .../editor/DeleteImpactAnalysis.test.tsx | 57 ++- app/page.tsx | 20 +- app/projects/[id]/editor/page.tsx | 23 +- components/editor/AddEntityDialog.tsx | 343 +++++++++--------- components/editor/DeleteImpactAnalysis.tsx | 39 +- 5 files changed, 269 insertions(+), 213 deletions(-) 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/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 (
From bf7889c58df95c35c9d89b712e92727ff19b42d0 Mon Sep 17 00:00:00 2001 From: "John R. D'Orazio" Date: Wed, 19 Aug 2026 01:10:49 +0200 Subject: [PATCH 2/2] fix: seed PropertyDetailPanel edit state during render, not in an effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes Category A of #200 (PropertyDetailPanel.tsx:293). The auto-enter effect initialised eleven pieces of edit state after the commit, either from a restored draft or from the parsed detail. Both inputs are in fact synchronous — `detail` is a useMemo over sourceContent, and the draft store is localStorage-backed — so the seeding can happen during the render that first satisfies the conditions. Doing it there also removes the window where the panel paints an empty editor before the values land. This required two supporting changes: - useEntityAutoSave now seeds `restoredDraft` with a useState lazy initialiser instead of only filling it from an effect. The store read is synchronous, and a consumer initialising during render would otherwise see null on the first render and miss the draft entirely. The effect stays for subsequent entity changes. - `editInitializedRef` becomes `autoEnterDone` state and the draft-consumed flag becomes `restoredDraftApplied` state, because react-hooks/refs forbids touching refs during render. `clearRestoredDraft` writes state owned by useEntityAutoSave, so it cannot be called during render; it stays in a small effect keyed on `restoredDraftApplied`. That effect synchronises the draft store and derives nothing, so it is not the pattern #200 is targeting. `react-hooks/set-state-in-effect`: 11 warnings → 10. Tests: the existing 100 PropertyDetailPanel tests pass unchanged, plus a new one covering the real-world ordering where sourceContent arrives after mount and the draft must survive to the render where detail first appears. Two new useEntityAutoSave tests assert the draft is visible on the first render. The draft-store mock gained `getState` to match zustand's shape. Co-Authored-By: Claude Opus 5 (1M context) --- .../editor/PropertyDetailPanel.test.tsx | 48 +++++++++++++++++++ __tests__/lib/hooks/useEntityAutoSave.test.ts | 42 ++++++++++++---- components/editor/PropertyDetailPanel.tsx | 47 ++++++++++++------ lib/hooks/useEntityAutoSave.ts | 11 ++++- 4 files changed, 124 insertions(+), 24 deletions(-) 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/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);