Skip to content

refactor: remove the set-state-in-effect suppressions #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aaronjmars merged 1 commit into from
Jul 20, 2026
Merged

refactor: remove the set-state-in-effect suppressions #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 36 additions & 29 deletions components/column/column-card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<HTMLInputElement | null>(null);

const [isFetchingRaw, setIsFetching] = useState(false);
Expand Down Expand Up @@ -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));
Expand All @@ -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]);
Expand Down
163 changes: 91 additions & 72 deletions components/dialogs/version-history-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,36 +25,77 @@ export function VersionHistoryDialog({ deckId, open, onOpenChange }: Props) {
const deckName = useDeckStore((s) =>
deckId ? (s.decks[deckId]?.name ?? "") : "",
);

return (
<Dialog open={open} onOpenChange={onOpenChange}>
<DialogContent className="max-w-md">
<DialogHeader>
<DialogTitle className="flex items-center gap-2">
<History className="size-4" />
Version history
{deckName ? (
<span className="truncate font-normal text-muted-foreground">
· {deckName}
</span>
) : null}
</DialogTitle>
</DialogHeader>

<p className="text-xs text-muted-foreground">
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.
</p>

{/*
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 ? (
<SnapshotList
key={deckId}
deckId={deckId}
onRestored={() => onOpenChange(false)}
/>
) : null}
</DialogContent>
</Dialog>
);
}

function SnapshotList({
deckId,
onRestored,
}: {
deckId: string;
onRestored: () => void;
}) {
const loadDeckSnapshots = useDeckStore((s) => s.loadDeckSnapshots);
const restoreDeckSnapshot = useDeckStore((s) => s.restoreDeckSnapshot);

const [snapshots, setSnapshots] = useState<DeckSnapshotMeta[]>([]);
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<DeckSnapshotMeta[] | null>(null);
const [restoringId, setRestoringId] = useState<number | null>(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;
Expand All @@ -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 });
Expand All @@ -74,65 +115,43 @@ export function VersionHistoryDialog({ deckId, open, onOpenChange }: Props) {
}

return (
<Dialog open={open} onOpenChange={onOpenChange}>
<DialogContent className="max-w-md">
<DialogHeader>
<DialogTitle className="flex items-center gap-2">
<History className="size-4" />
Version history
{deckName ? (
<span className="truncate font-normal text-muted-foreground">
· {deckName}
</span>
) : null}
</DialogTitle>
</DialogHeader>

<p className="text-xs text-muted-foreground">
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.
<div className="grid gap-1.5">
{snapshots === null ? (
<p className="py-6 text-center text-sm text-muted-foreground">
Loading…
</p>

<div className="grid gap-1.5">
{loading ? (
<p className="py-6 text-center text-sm text-muted-foreground">
Loading…
</p>
) : snapshots.length === 0 ? (
<p className="py-6 text-center text-sm text-muted-foreground">
No version history yet. Make a change to this deck and a snapshot
will appear here.
</p>
) : (
snapshots.map((snap) => (
<div
key={snap.id}
className="flex items-center justify-between gap-3 rounded-md border border-border px-3 py-2"
>
<div className="min-w-0">
<div className="text-sm">
<RelativeTime date={snap.capturedAt} addSuffix />
</div>
<div className="text-xs text-muted-foreground tabular-nums">
{snap.columnCount} column{snap.columnCount === 1 ? "" : "s"}
</div>
</div>
<Button
type="button"
size="sm"
variant="outline"
disabled={restoringId !== null}
onClick={() => handleRestore(snap.id)}
>
<RotateCcw className="mr-1.5 size-3.5" />
{restoringId === snap.id ? "Restoring…" : "Restore"}
</Button>
) : snapshots.length === 0 ? (
<p className="py-6 text-center text-sm text-muted-foreground">
No version history yet. Make a change to this deck and a snapshot will
appear here.
</p>
) : (
snapshots.map((snap) => (
<div
key={snap.id}
className="flex items-center justify-between gap-3 rounded-md border border-border px-3 py-2"
>
<div className="min-w-0">
<div className="text-sm">
<RelativeTime date={snap.capturedAt} addSuffix />
</div>
))
)}
</div>
</DialogContent>
</Dialog>
<div className="text-xs text-muted-foreground tabular-nums">
{snap.columnCount} column{snap.columnCount === 1 ? "" : "s"}
</div>
</div>
<Button
type="button"
size="sm"
variant="outline"
disabled={restoringId !== null}
onClick={() => handleRestore(snap.id)}
>
<RotateCcw className="mr-1.5 size-3.5" />
{restoringId === snap.id ? "Restoring…" : "Restore"}
</Button>
</div>
))
)}
</div>
);
}