Skip to content

[Refactor] Extract useFocusTrap hook from DrawerLayout and AgentSelector#190

Merged
samzong merged 2 commits intomainfrom
fix/focus-trap-hook
Mar 27, 2026
Merged

[Refactor] Extract useFocusTrap hook from DrawerLayout and AgentSelector#190
samzong merged 2 commits intomainfrom
fix/focus-trap-hook

Conversation

@samzong
Copy link
Copy Markdown
Collaborator

@samzong samzong commented Mar 27, 2026

Summary

Extract duplicated focus-trap logic from DrawerLayout and AgentSelector into a single useFocusTrap hook. Eliminates ~40 lines of duplicated keyboard/focus management code and fixes an inconsistency where DrawerLayout did not properly save/restore previous focus.

Type of change

  • [Refactor] internal cleanup

Why is this needed?

Both DrawerLayout and AgentSelector contained nearly identical inline focus-trap implementations: Tab wrapping, Escape handling, save/restore of previous focus, and initial focus targeting. The duplication violated DRY and the two implementations had subtle inconsistencies — DrawerLayout relied on a hardcoded menuButtonRef for focus restoration instead of saving document.activeElement on open, which meant edge-swipe opens could lose focus context.

What changed?

  • New packages/pwa/src/hooks/useFocusTrap.ts — reusable hook with signature useFocusTrap(containerRef, open, onClose, initialFocusSelector?)
  • DrawerLayout.tsx — removed 38-line inline focus-trap useEffect, removed menuButtonRef, simplified closeDrawer to one line, replaced with single useFocusTrap call
  • AgentSelector.tsx — removed handleKeyDown useCallback (26 lines), focus-management useEffect (14 lines), and prevFocusRef; replaced with single useFocusTrap call using initialFocusSelector to skip the close button

Architecture impact

  • Owning layer: renderer
  • Cross-layer impact: none
  • Invariants touched from docs/architecture-invariants.md: none
  • Why those invariants remain protected: change is renderer-only, no state management or persistence logic affected

Linked issues

Closes #

Validation

  • pnpm check (typecheck + lint + format + dead-code)
  • pnpm build
  • Manual smoke test
  • Not run

Commands, screenshots, or notes:

pnpm check passes (only pre-existing todo.md format warning)
tsc --noEmit on useFocusTrap.ts passes in isolation

Screenshots or recordings

No visual changes — behavior is identical.

Release note

  • No user-facing change. Release note is NONE.
  • User-facing change. Release note is included below.
NONE

Checklist

  • The PR title uses at least one approved prefix: [Feat], [Fix], [UI], [Docs], [Refactor], [Build], or [Chore]
  • The summary explains both what changed and why
  • Validation reflects the commands actually run for this PR
  • Architecture impact is described and references any touched invariants
  • Cross-layer changes are explicitly justified
  • The release note block is accurate

@github-actions
Copy link
Copy Markdown
Contributor

Hi @samzong,
Thanks for your pull request!
If the PR is ready, use the /auto-cc command to assign Reviewer to Review.
We will review it shortly.

Details

Instructions for interacting with me using comments are available here.
If you have questions or suggestions related to my behavior, please file an issue against the gh-ci-bot repository.

@samzong samzong merged commit 24371fd into main Mar 27, 2026
7 checks passed
@samzong samzong deleted the fix/focus-trap-hook branch March 29, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant