Skip to content

ConfirmDialog's openDialogCount leaks permanently open if the dialog unmounts while open, breaking the "/" shortcut app-wide #318

Description

@Jagadeeshftw

📌 Description

ConfirmDialog (src/components/ConfirmDialog.tsx) tracks whether any dialog is open via a module-level counter in src/components/confirmDialogOpenState.ts (markConfirmDialogOpen/markConfirmDialogClosed/isConfirmDialogOpen), which useFocusShortcut (src/hooks/useFocusShortcut.ts) checks before focusing a search box on "/", specifically so the shortcut doesn't hijack focus while a confirm dialog is open. ConfirmDialog's effect that drives this — useEffect(() => { if (open) { ...; markConfirmDialogOpen(); } else { markConfirmDialogClosed(); } }, [open]); — only reacts to the open prop changing, and returns no cleanup function. If the ConfirmDialog component itself unmounts while open is still true — e.g. a user opens the "Deactivate anchor" dialog on /anchors/[id], then clicks a nav link or the browser Back button without clicking Cancel/Confirm first, unmounting AnchorDetail (and its ConfirmDialog) entirely — markConfirmDialogClosed() is never called for that open. openDialogCount stays incremented for the rest of the SPA session, so isConfirmDialogOpen() permanently reports true and the "/" search-focus shortcut silently stops working on every subsequent page until a full page reload resets the module state.

🧩 Requirements and context

  • ConfirmDialog must decrement the open-dialog counter if it unmounts while still open, not only when its open prop transitions to false.
  • The existing counter-based (rather than boolean) tracking must be preserved, since it's designed to tolerate multiple dialogs opening/closing independently.
  • No change to ConfirmDialog's existing focus-trap, Escape, or backdrop-click behavior.

🛠️ Suggested execution

  • In src/components/ConfirmDialog.tsx, return a cleanup function from the open-tracking useEffect that calls markConfirmDialogClosed() if the dialog is unmounted while open is still true (e.g. track whether this effect run itself opened the dialog and close it in the cleanup, guarding against double-decrementing when open was already flipped to false normally first).
  • Extend a test (in src/components/ConfirmDialog.test.tsx or src/hooks/useFocusShortcut.test.ts) that renders an open ConfirmDialog, unmounts it without clicking Cancel/Confirm, and asserts isConfirmDialogOpen() (from confirmDialogOpenState.ts) returns false afterward and that useFocusShortcut's "/" handler works normally again.

✅ Acceptance criteria

  • Unmounting an open ConfirmDialog (without cancelling/confirming) correctly decrements the open-dialog counter.
  • The "/" search-focus shortcut continues to work normally on subsequent pages after such an unmount.
  • Test coverage for the unmount-while-open case.

🔒 Security notes

No new attack surface; a component-lifecycle hygiene fix preventing a stuck global UI state.

📋 Guidelines

  • Minimum 95% test coverage
  • Clear documentation
  • Timeframe: 96 hours

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26bugSomething isn't workingfrontendFrontend / UI work

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions