From 2cdc3d3fa871355a70960d6449d9072196fea357 Mon Sep 17 00:00:00 2001 From: Aaron Elijah Mars <61592645+aaronjmars@users.noreply.github.com> Date: Mon, 20 Jul 2026 13:06:54 -0400 Subject: [PATCH] refactor: remove the set-state-in-effect suppressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three were disabled with rationale rather than fixed. Each had a real fix that also simplifies the code. ColumnCard opened its search row from two effects. Opening is a state adjustment, so it now happens during render — React re-runs the component before paint, with no cascading commit and no frame where the row is missing. The query-driven open has to stay edge-triggered. Both the "Esc" button and the toolbar toggle close the row without clearing the query, so a plain `searchActive && !searchOpen` test re-opens it on the next render and makes the row impossible to dismiss. Comparing against the previous value keeps it a transition; the mount case is covered by seeding `searchOpen` from `searchActive` instead. The effect keeps the two jobs that belong in one: moving focus and draining the one-shot signal. VersionHistoryDialog stays mounted for the lifetime of the sidebar, which is the only reason it needed to set a loading flag synchronously on open. Split the list into a child mounted per-open and keyed by deck, so `useState`'s initial value is the reset. `snapshots: null` then encodes "not loaded yet" and the separate `loading` boolean goes away, along with a one-render flash of the empty-state copy that the old `loading: false` default allowed. Verified against a live deck: closing search with an active query stays closed, the query survives close/reopen, clearing it mid-type keeps the row up, `/` opens and focuses, and the dialog reloads cleanly on reopen. No React warnings in the console. --- components/column/column-card.tsx | 65 +++---- components/dialogs/version-history-dialog.tsx | 163 ++++++++++-------- 2 files changed, 127 insertions(+), 101 deletions(-) diff --git a/components/column/column-card.tsx b/components/column/column-card.tsx index a47f852..b659a9b 100644 --- a/components/column/column-card.tsx +++ b/components/column/column-card.tsx @@ -83,7 +83,14 @@ export function ColumnCard({ column }: { column: Column }) { s.pendingRefreshIds.has(column.id), ); const clearPendingRefresh = useDeckStore((s) => s.clearPendingRefresh); - const [searchOpen, setSearchOpen] = useState(false); + const searchActive = searchQuery.trim().length > 0; + // Seeded from the query rather than hardcoded false: a column that mounts + // with a query already in the store (collapse/expand, tab switch) shows its + // search row on the first paint instead of flickering it in a frame later. + const [searchOpen, setSearchOpen] = useState(searchActive); + // Mirrors `searchActive` so the adjustment below can tell a transition from + // a steady state. See the comment there for why that distinction matters. + const [prevSearchActive, setPrevSearchActive] = useState(searchActive); const searchInputRef = useRef(null); const [isFetchingRaw, setIsFetching] = useState(false); @@ -140,7 +147,6 @@ export function ColumnCard({ column }: { column: Column }) { // never widens past the persisted-filter results, matching the operator's // mental model: filters decide what the column *contains*, search decides // what they're looking at *right now* inside that subset. - const searchActive = searchQuery.trim().length > 0; const visibleItems = useMemo(() => { if (!searchActive) return filteredItems; return filteredItems.filter((it) => itemMatchesSearchQuery(it, searchQuery)); @@ -156,36 +162,37 @@ export function ColumnCard({ column }: { column: Column }) { }, [alertTerms, visibleItems]); const matchCount = matchedItemIds.size; - // Auto-open the search row when a query already exists on mount — covers - // the case where the operator collapsed/expanded a column or switched tabs - // and the search state was preserved in-session. Closing collapses the row - // visually but never clears the query, so re-opening shows what they last - // typed (until they reload or manually clear). - useEffect(() => { - // set-state-in-effect is suppressed, not fixed: the open state is sticky - // by design, so it can't be derived as `searchOpen || searchActive` — - // that would auto-close the row the moment the query is cleared, which is - // exactly the behaviour the note below rules out. The cascade is one - // bounded extra render on the mount-with-existing-query path. - // eslint-disable-next-line react-hooks/set-state-in-effect - if (searchActive && !searchOpen) setSearchOpen(true); - // intentionally only react to searchActive flipping true — don't auto- - // close when the operator clears the query mid-type; let them keep the - // input visible to keep typing. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [searchActive]); + // Two things force the search row open, and both are state adjustments, so + // they run during render rather than from an effect — React re-runs the + // component immediately, before paint, with no cascading commit. + // + // 1. The query going from empty to non-empty. This must be edge-triggered, + // not condition-triggered: the "Esc" button and the toolbar toggle both + // close the row *without* clearing the query, so a plain + // `searchActive && !searchOpen` test would immediately re-open it and + // make the row impossible to dismiss. Comparing against the previous + // value is what keeps it a transition. Deliberately one-way — clearing + // the query mid-type leaves the row up so the operator can keep typing. + // 2. The `/` shortcut, routed here by the deck-board listener through + // `pendingSearchOpen`. That signal is one-shot and drained by the effect + // below on the same commit, so it is already edge-shaped. + // + // The mount case (a query that survived a collapse/expand or a tab switch) + // is handled by seeding `searchOpen` from `searchActive` above, not here. + if (searchActive !== prevSearchActive) { + setPrevSearchActive(searchActive); + if (searchActive) setSearchOpen(true); + } + if (pendingSearchOpen === column.id && !searchOpen) { + setSearchOpen(true); + } - // React to the `/` keyboard shortcut routed via the store. The deck-board - // listener sets `pendingSearchOpen` to this column's id; we open the search - // row, focus the input, then clear the signal so a future `/` keypress - // re-fires cleanly. + // Left with the two jobs that genuinely belong in an effect: moving DOM + // focus, and draining the one-shot signal so a later `/` re-fires. The row + // is already committed by the time this runs, because the adjustment above + // resolves before React commits the tree. useEffect(() => { if (pendingSearchOpen !== column.id) return; - // Syncing local state from an external store signal is effect-shaped by - // nature — the keypress happens outside this component and is routed here - // through the store, so there is no render-time value to derive from. - // eslint-disable-next-line react-hooks/set-state-in-effect - setSearchOpen(true); requestAnimationFrame(() => searchInputRef.current?.focus()); clearPendingSearchOpen(column.id); }, [pendingSearchOpen, column.id, clearPendingSearchOpen]); diff --git a/components/dialogs/version-history-dialog.tsx b/components/dialogs/version-history-dialog.tsx index dd0b647..f83f11c 100644 --- a/components/dialogs/version-history-dialog.tsx +++ b/components/dialogs/version-history-dialog.tsx @@ -25,36 +25,77 @@ export function VersionHistoryDialog({ deckId, open, onOpenChange }: Props) { const deckName = useDeckStore((s) => deckId ? (s.decks[deckId]?.name ?? "") : "", ); + + return ( + + + + + + Version history + {deckName ? ( + + · {deckName} + + ) : null} + + + +

+ Snapshots are captured automatically before you add, remove, or + reorder columns. Restoring creates a new deck with “(restored)” + appended — your current deck isn’t touched. +

+ + {/* + The dialog itself stays mounted for the lifetime of the sidebar, so + the list is split out and mounted per-open, keyed by deck. That makes + `useState`'s initial value the reset: reopening, or switching decks + while open, starts from the loading state without an effect having to + synchronously roll it back. + */} + {open && deckId ? ( + onOpenChange(false)} + /> + ) : null} + + + ); +} + +function SnapshotList({ + deckId, + onRestored, +}: { + deckId: string; + onRestored: () => void; +}) { const loadDeckSnapshots = useDeckStore((s) => s.loadDeckSnapshots); const restoreDeckSnapshot = useDeckStore((s) => s.restoreDeckSnapshot); - const [snapshots, setSnapshots] = useState([]); - const [loading, setLoading] = useState(false); + // `null` means "not loaded yet" — one state instead of a separate `loading` + // flag that had to be raised and lowered around the fetch. It also removes a + // flash of the empty-state copy on first paint, which the old `loading` + // default of `false` allowed for one render. + const [snapshots, setSnapshots] = useState(null); const [restoringId, setRestoringId] = useState(null); useEffect(() => { - if (!open || !deckId) return; let cancelled = false; - // Entering the loading state as the fetch is kicked off is the point of - // this effect — the data lives outside React and only the dialog opening - // can trigger the read. Removing the cascade would mean moving snapshot - // loading behind Suspense, which is out of scope here. - // eslint-disable-next-line react-hooks/set-state-in-effect - setLoading(true); loadDeckSnapshots(deckId) .then((rows) => { if (!cancelled) setSnapshots(rows); }) .catch(() => { if (!cancelled) setSnapshots([]); - }) - .finally(() => { - if (!cancelled) setLoading(false); }); return () => { cancelled = true; }; - }, [open, deckId, loadDeckSnapshots]); + }, [deckId, loadDeckSnapshots]); async function handleRestore(id: number) { if (restoringId !== null) return; @@ -64,7 +105,7 @@ export function VersionHistoryDialog({ deckId, open, onOpenChange }: Props) { toast.success(`Restored "${result.deckName}"`, { description: `${result.columns.length} column${result.columns.length === 1 ? "" : "s"}`, }); - onOpenChange(false); + onRestored(); } catch (err) { const msg = err instanceof Error ? err.message : "Restore failed"; toast.error("Restore failed", { description: msg }); @@ -74,65 +115,43 @@ export function VersionHistoryDialog({ deckId, open, onOpenChange }: Props) { } return ( - - - - - - Version history - {deckName ? ( - - · {deckName} - - ) : null} - - - -

- Snapshots are captured automatically before you add, remove, or - reorder columns. Restoring creates a new deck with “(restored)” - appended — your current deck isn’t touched. +

+ {snapshots === null ? ( +

+ Loading…

- -
- {loading ? ( -

- Loading… -

- ) : snapshots.length === 0 ? ( -

- No version history yet. Make a change to this deck and a snapshot - will appear here. -

- ) : ( - snapshots.map((snap) => ( -
-
-
- -
-
- {snap.columnCount} column{snap.columnCount === 1 ? "" : "s"} -
-
- + ) : snapshots.length === 0 ? ( +

+ No version history yet. Make a change to this deck and a snapshot will + appear here. +

+ ) : ( + snapshots.map((snap) => ( +
+
+
+
- )) - )} -
- -
+
+ {snap.columnCount} column{snap.columnCount === 1 ? "" : "s"} +
+ + + + )) + )} + ); }