Repository navigation
[quality] useFocusTrap's previousFocus cleanup fallback has no end-to-end coverage: no spec closes a lightbox by unmounting its trigger #1177
Description
Finding
src/components/hooks/useFocusTrap.js restores focus when a lightbox closes:
48 return () => {
49 document.body.style.overflow = previousOverflow;
50 document.removeEventListener('keydown', onKeyDown);
51 (triggerRef.current || previousFocus)?.focus?.();
52 };Every existing lightbox spec closes the dialog with Escape, the close button or
a backdrop click. In all three the trigger is still mounted, triggerRef.current
is truthy, and the previousFocus operand at line 51 is never evaluated.
There is a browser path that nulls the ref, and it is ordinary use rather
than a contrivance. Both lightboxes are rendered by the card that triggers
them:
72 {open && (
73 <MemberProfile
74 member={member}
75 onClose={() => setOpen(false)}
76 triggerRef={triggerRef}
77 />
78 )}— src/components/MemberDirectory/MemberCard.js:72-78
The directory's search field stays live behind the backdrop, so a visitor who
opens a profile and keeps typing to narrow the list filters that card out. The
trigger and the dialog unmount in the same commit; React detaches refs during
the mutation phase and runs passive-effect cleanups afterwards, so the cleanup
observes triggerRef.current === null and falls through to previousFocus.
Nothing asserts what happens on that path. Written as triggerRef.current.focus()
— the obvious simplification, since in every other close path the trigger is
mounted — the cleanup throws a TypeError mid-commit while the visitor is
typing, and a reordered cleanup would also skip the body scroll-lock release at
line 49, leaving body { overflow: hidden } with no dialog on screen.
Evidence and provenance
- Unit — covered.
npm run test:unit:coverage
(TZ=UTC node tests/tools/coverage-report.mjs, node v26.10.0) run locally on
2026-10-08 at03cfcfereportssrc files | 100.00 | 99.92 | 8984/8984 lines | 2545/2547 regions, with no sub-100% entry for
src/components/hooks/useFocusTrap.js. The whole unit suite is green at
2025 passed. - E2E — not covered. CI artifact
e2e-coverage, id11518581518, from run
37702408788,
workflowValidate repository, jobEnd-to-end coverage, head678f79d
(03cfcfeplus one test file).report.json:
{"file":"src/components/hooks/useFocusTrap.js","regions":21,"coveredRegions":19,"regionPercent":90.48,"uncoveredRegions":[35,51]}.
Reproduced locally at03cfcfewith the two-build coverage run
(npm run build:e2e:coverage+test:e2e:coverage, 341 passed,
src files | 100.00 | 91.63 | 438/478 regions). - Covered by unit tests but not end-to-end tests, so priority 2 under the
coverage-evidence rules.
The region will not leave the uncovered list, and that is #1066 / #1079
Measured, not assumed. A control run of the new spec alone at 03cfcfe, against
the real build only, reports:
src/components/hooks/useFocusTrap.js | 83.93 | 42.86 | 30 36 37 38 39 40 41 42 43 | 29 34 35
Line 51 is absent — the interaction demonstrably executes it. Re-running the
full two-build suite with the spec included puts it back:
src/components/hooks/useFocusTrap.js | 100.00 | 90.48 | | 35 51
src files | 100.00 | 91.72 | 2099/2099 lines | 443/483 regions
This is the same region-union attribution behaviour tracked by
#1066 and
#1079: the variant build emits
the cleanup under coordinates that do not match the real build's, so the union
keeps the zero. The spec is therefore stated as behaviour rather than as a
coverage claim, exactly as tests/e2e/dialog-non-dismissing.spec.js already is.
Line 35 (if (!focusable?.length) return;) is a separate case and is not
in scope here: both lightboxes always render a close button, so the dialog's
button, a[href] list is never empty and the guard's taken arm has no browser
path at all. It is a phantom region of the kind #1051 folds, not a missing test.
Recommendation
-
tests/e2e/focus-trap-trigger-unmounted.spec.js(new) opens a member
profile, focuses#member-searchthrough the DOM (a click would dismiss
via the backdrop while the trigger is still mounted), types a query no
organization matches, and asserts the dialog unmounts with its card, no
pageerrorfires,document.body.style.overflowreturns to the value
captured before the dialog opened, and focus stays in the search field
with its typed value intact
Coordination
The change adds one new file, tests/e2e/focus-trap-trigger-unmounted.spec.js,
and edits nothing. It is disjoint from every open hold-gated PR: not
scripts/audit-gate.mjs (#1161), not tests/tools/e2e-coverage-report.mjs
(#1163), not scripts/lib/svg-active-content.mjs (#1168, #1176), not
CONTRIBUTING.md (#1166), not tests/architecture-content-mirror.test.mjs
(#1165), not tests/tools/e2e-data-fixtures.cjs / the metrics.json overlay /
the metrics specs (#1171), not tests/e2e-coverage-run.test.mjs (#1174). It
needs no fixture overlay, so it does not touch the two-build mechanism whose
ceiling #1172 documents. No workflow file is involved: the e2e gate is
--check-source-regions 91 (.github/workflows/ci.yml:256), and the measured
figure rises from 91.63% to 91.72%.
Priority
- Impact: medium (the only close path that nulls the trigger ref; its failure
mode is a TypeError thrown into a commit while the visitor is typing, plus a
stuck body scroll lock) - Effort: low
🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5n31 | SHA: 03cfcfe
— hive: agent=quality backend=copilot model=claude-opus-5 copilot=1.0.88
Activity