Skip to content

Commit f21eb26

Browse files
refactor(OUT-4012): split confirm-modal effects per review
Replace the onCancelRef workaround with two focused effects: one keyed on `open` for focus-in/restore (runs once), one keyed on `open`+`onCancel` for the Escape + Tab-trap listener. Clearer, honest dependency arrays. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 10fda63 commit f21eb26

1 file changed

Lines changed: 18 additions & 17 deletions

File tree

src/components/ui/ConfirmModal.tsx

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,24 +25,28 @@ export default function ConfirmModal({
2525
const titleId = useId()
2626
const descId = useId()
2727
const dialogRef = useRef<HTMLDivElement>(null)
28-
// Keep the latest onCancel without re-running the focus effect each render.
29-
const onCancelRef = useRef(onCancel)
30-
onCancelRef.current = onCancel
3128

32-
// On open: focus into the dialog, trap Tab, and restore focus on close.
29+
// On open, focus into the dialog; on close, restore focus to the opener.
3330
useEffect(() => {
3431
if (!open) return
3532
const previouslyFocused = document.activeElement as HTMLElement | null
36-
const focusables = Array.from(
37-
dialogRef.current?.querySelectorAll<HTMLElement>('button') ?? [],
38-
)
39-
focusables[0]?.focus()
33+
const buttons = dialogRef.current?.querySelectorAll<HTMLElement>('button')
34+
buttons?.[0]?.focus()
35+
return () => previouslyFocused?.focus()
36+
}, [open])
4037

38+
// Escape cancels; Tab is trapped between the dialog's buttons.
39+
useEffect(() => {
40+
if (!open) return
4141
const onKeyDown = (e: KeyboardEvent) => {
42-
if (e.key === 'Escape') return onCancelRef.current()
43-
if (e.key !== 'Tab' || focusables.length === 0) return
44-
const first = focusables[0]
45-
const last = focusables[focusables.length - 1]
42+
if (e.key === 'Escape') return onCancel()
43+
if (e.key !== 'Tab') return
44+
const buttons = Array.from(
45+
dialogRef.current?.querySelectorAll<HTMLElement>('button') ?? [],
46+
)
47+
if (buttons.length === 0) return
48+
const first = buttons[0]
49+
const last = buttons[buttons.length - 1]
4650
if (e.shiftKey && document.activeElement === first) {
4751
e.preventDefault()
4852
last.focus()
@@ -52,11 +56,8 @@ export default function ConfirmModal({
5256
}
5357
}
5458
document.addEventListener('keydown', onKeyDown)
55-
return () => {
56-
document.removeEventListener('keydown', onKeyDown)
57-
previouslyFocused?.focus()
58-
}
59-
}, [open])
59+
return () => document.removeEventListener('keydown', onKeyDown)
60+
}, [open, onCancel])
6061

6162
if (!open) return null
6263

0 commit comments

Comments
 (0)