refactor: eliminate useEffect anti-patterns (audit pass 2) (#665)

* refactor: eliminate useEffect anti-patterns across 5 components

- MonacoEditor, RichMarkdownEditor: assign callback/prop refs directly
  during render instead of via useEffect, so ProseMirror and Monaco
  handlers always read the current closure rather than a one-render-
  stale copy.

- SettingsFormControls (FontAutocomplete): replace useEffect that reset
  highlightedIndex on filteredSuggestions/open changes with the
  "adjusting state during render" pattern (track prevOpen /
  prevFilteredSuggestions), so the correct item is highlighted on the
  same paint as the dropdown opens or the filter updates.

- ChecksPanel: replace useEffect that reset 5 local state fields on
  activeWorktreeId change with the "adjusting during render" pattern
  (track prevActiveWorktreeId), so stale title/loading UI from the
  previous worktree is cleared before the first paint of the new one.

- SshPassphraseDialog: split one useEffect that both reset form state
  and focused the input into (a) during-render state reset and (b) a
  focused DOM-only useEffect, so the cleared input is visible
  immediately rather than after a post-paint effect flush.

* fix: resync font autocomplete highlight on value change
This commit is contained in:
Neil 2026-04-14 23:28:58 -07:00 committed by GitHub
parent 81144f29db
commit 306118dbee
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 59 additions and 26 deletions

View file

@ -54,10 +54,11 @@ export default function MonacoEditor({
// cleanup and overwrite the correct value with a stale one.
const scrollThrottleTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null)
const propsRef = useRef({ relativePath, language, onSave })
useEffect(() => {
propsRef.current = { relativePath, language, onSave }
}, [relativePath, language, onSave])
// Why: assigning during render keeps the ref current before any event handler
// or effect reads it, avoiding the one-render stale window that a useEffect
// would introduce. Refs are mutable and don't trigger re-renders, so this is
// safe to do unconditionally every render.
propsRef.current = { relativePath, language, onSave }
const settings = useAppStore((s) => s.settings)
const editorFontZoomLevel = useAppStore((s) => s.editorFontZoomLevel)

View file

@ -72,18 +72,13 @@ export default function RichMarkdownEditor({
const [isEditingLink, setIsEditingLink] = useState(false)
const isEditingLinkRef = useRef(false)
useEffect(() => {
onContentChangeRef.current = onContentChange
}, [onContentChange])
useEffect(() => {
onDirtyStateHintRef.current = onDirtyStateHint
}, [onDirtyStateHint])
useEffect(() => {
onSaveRef.current = onSave
}, [onSave])
useEffect(() => {
isEditingLinkRef.current = isEditingLink
}, [isEditingLink])
// Why: assigning callback refs during render keeps them current before any
// ProseMirror handler reads them, avoiding the one-render stale window that
// useEffect would introduce. Refs are mutable and never trigger re-renders.
onContentChangeRef.current = onContentChange
onDirtyStateHintRef.current = onDirtyStateHint
onSaveRef.current = onSave
isEditingLinkRef.current = isEditingLink
const flushPendingSerialization = useCallback(() => {
if (serializeTimerRef.current === null) {

View file

@ -54,14 +54,19 @@ export default function ChecksPanel(): React.JSX.Element {
// remount on worktree switch (that caused an IPC storm on Windows).
// Reset worktree-specific local state so stale UI from the previous
// worktree doesn't leak (e.g. mid-edit title, stale loading indicators).
useEffect(() => {
// Done during render (not useEffect) so the reset takes effect on the same
// paint as the worktree change — useEffect would leave one render with the
// previous worktree's stale title/loading state visible.
const [prevActiveWorktreeId, setPrevActiveWorktreeId] = useState(activeWorktreeId)
if (activeWorktreeId !== prevActiveWorktreeId) {
setPrevActiveWorktreeId(activeWorktreeId)
setEditingTitle(false)
setTitleDraft('')
setTitleSaving(false)
setIsRefreshing(false)
setEmptyRefreshing(false)
conflictSummaryRefreshKeyRef.current = null
}, [activeWorktreeId])
}
// Find active worktree and repo
const { worktree, repo } = useMemo(() => {

View file

@ -1,3 +1,6 @@
/* eslint-disable max-lines -- Why: these small settings form primitives and controls
co-locate shared layout and keyboard interaction logic, which keeps the settings
panel wiring simple even though the file exceeds the default line limit. */
import { useEffect, useId, useMemo, useRef, useState } from 'react'
import { ScrollArea } from '../ui/scroll-area'
import { Input } from '../ui/input'
@ -253,15 +256,27 @@ export function FontAutocomplete({
return normalizedQuery ? [...startsWith, ...includes] : suggestions
}, [suggestions, normalizedQuery])
useEffect(() => {
// Why: sync the highlighted index during render rather than via useEffect so
// the correct item is highlighted on the very first paint after open/filter
// changes — useEffect would leave one render with the stale index visible.
const [prevFilteredSuggestions, setPrevFilteredSuggestions] = useState(filteredSuggestions)
const [prevOpen, setPrevOpen] = useState(open)
const [prevHighlightedValue, setPrevHighlightedValue] = useState(value)
if (
filteredSuggestions !== prevFilteredSuggestions ||
open !== prevOpen ||
value !== prevHighlightedValue
) {
setPrevFilteredSuggestions(filteredSuggestions)
setPrevOpen(open)
setPrevHighlightedValue(value)
if (!open || filteredSuggestions.length === 0) {
setHighlightedIndex(-1)
return
} else {
const selectedIndex = filteredSuggestions.findIndex((font) => font === value)
setHighlightedIndex(Math.max(selectedIndex, 0))
}
const selectedIndex = filteredSuggestions.findIndex((font) => font === value)
setHighlightedIndex(Math.max(selectedIndex, 0))
}, [filteredSuggestions, open, value])
}
useEffect(() => {
if (!open || highlightedIndex < 0) {

View file

@ -23,10 +23,22 @@ export function SshPassphraseDialog(): React.JSX.Element | null {
const open = request !== null
const requestId = request?.requestId
useEffect(() => {
// Why: reset form state during render (not useEffect) so the cleared input is
// visible on the same paint as the new request arriving — useEffect would
// leave one render showing the previous passphrase value.
const [prevRequestId, setPrevRequestId] = useState(requestId)
if (requestId !== prevRequestId) {
setPrevRequestId(requestId)
if (requestId) {
setValue('')
setSubmitting(false)
}
}
// DOM focus is a side effect that must remain in useEffect.
useEffect(() => {
if (requestId) {
requestAnimationFrame(() => inputRef.current?.focus())
}
}, [requestId])
@ -109,7 +121,12 @@ export function SshPassphraseDialog(): React.JSX.Element | null {
/>
</div>
<DialogFooter className="mt-1">
<Button variant="outline" size="sm" onClick={() => void handleCancel()} disabled={submitting}>
<Button
variant="outline"
size="sm"
onClick={() => void handleCancel()}
disabled={submitting}
>
Cancel
</Button>
<Button size="sm" onClick={() => void handleSubmit()} disabled={!value || submitting}>