Skip to content

[quality] useFocusTrap's previousFocus cleanup fallback has no end-to-end coverage: no spec closes a lightbox by unmounting its trigger #1177

Description

@hivecommons-hive

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 at 03cfcfe reports src 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, id 11518581518, from run
    37702408788,
    workflow Validate repository, job End-to-end coverage, head 678f79d
    (03cfcfe plus one test file). report.json:
    {"file":"src/components/hooks/useFocusTrap.js","regions":21,"coveredRegions":19,"regionPercent":90.48,"uncoveredRegions":[35,51]}.
    Reproduced locally at 03cfcfe with 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-search through 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
    pageerror fires, document.body.style.overflow returns 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

  1. added
    qualityApproved by a Hive merger/owner for auto-merge on green CI
    testingApproved by a Hive merger/owner for auto-merge on green CI
    agent/qualityApproved by a Hive merger/owner for auto-merge on green CI
    hive/covered-by-prHive verified that an open PR references or claims this issue; still actionable until confirmed
    on Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent/qualityApproved by a Hive merger/owner for auto-merge on green CIhive/covered-by-prHive verified that an open PR references or claims this issue; still actionable until confirmedhive/hosted-available-lke648397-260827-5n31Approved by a Hive merger/owner for auto-merge on green CIqualityApproved by a Hive merger/owner for auto-merge on green CItestingApproved by a Hive merger/owner for auto-merge on green CI

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions