From 0c749c6a4b6f3b7d9ce68c0562e1ba4dd7e1a986 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Wed, 5 Aug 2026 08:24:41 +0000 Subject: [PATCH 1/2] Extract a shared Modal and migrate all five dialogs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five dialogs each hand-rolled the same backdrop. One of them handled Escape, none trapped focus, none locked body scroll, none carried dialog semantics for a screen reader, and two drifted onto a different border and corner radius than the rest. Behaviour every dialog needs belongs in one component. Three details in components/ui/Modal.tsx are load-bearing rather than boilerplate: Escape is handled in the capture phase. App.tsx and ChatPage.tsx install document-level shortcut handlers that also claim Escape — to clear a search box, to stop generation. A bubble-phase listener would fire after them, so dismissing a dialog could also stop a generation running behind it. ShortcutsModal already knew this and had a capture-phase handler with a comment explaining why; that reasoning now applies to all five. Only the topmost dialog reacts. Every open Modal installs a listener, so without a stack one Escape would close a confirmation and the dialog that raised it together. The same refcount keeps an inner dialog from restoring body scroll while an outer one is still open. The backdrop closes on mousedown, not click. A click fires on the backdrop when a drag that *started* inside the panel — selecting text, say — is released outside it, which would throw the dialog away mid-interaction. Also: focus moves into the panel on open and returns to its origin on close, so keyboard navigation doesn't restart from the top of the document; and the two dialogs that hold real typing (new task, new skill) no longer discard it on a stray backdrop click. No behaviour change beyond those fixes. The footer slot takes a layout override because TaskStatusManager's footer is a form, not a button row. Motion follows the existing @keyframes convention and is dropped under prefers-reduced-motion, along with the transform-based animations already in index.css — the first reduced-motion handling in the app. Verified: tsc -b clean, npm run build clean, eslint unchanged (the 1 error + 5 warnings in these files are all pre-existing). Co-Authored-By: Claude Opus 5 --- web/src/components/ShortcutsModal.tsx | 81 ++---- web/src/components/Tasks/TaskCreateDialog.tsx | 108 ++++---- .../components/Tasks/TaskStatusManager.tsx | 155 ++++++----- web/src/components/ui/Modal.tsx | 243 ++++++++++++++++++ web/src/index.css | 31 +++ web/src/pages/SkillDetailPage.tsx | 47 ++-- web/src/pages/SkillsPage.tsx | 73 +++--- 7 files changed, 496 insertions(+), 242 deletions(-) create mode 100644 web/src/components/ui/Modal.tsx diff --git a/web/src/components/ShortcutsModal.tsx b/web/src/components/ShortcutsModal.tsx index ecd7b831..50697b0b 100644 --- a/web/src/components/ShortcutsModal.tsx +++ b/web/src/components/ShortcutsModal.tsx @@ -1,7 +1,6 @@ -import { useEffect } from 'react'; -import { X } from 'lucide-react'; import { useUIStore } from '../stores/uiStore'; import { formatCombo, type ShortcutCombo } from '../utils/keyboard'; +import { Modal } from './ui/Modal'; interface DisplayShortcut { combo: ShortcutCombo; @@ -51,65 +50,31 @@ export function ShortcutsModal() { const open = useUIStore((s) => s.shortcutsModalOpen); const close = useUIStore((s) => s.closeShortcutsModal); - // Local Esc handler — runs *before* the document-level shortcut listeners - // because modal mount captures it first when focus is inside. - useEffect(() => { - if (!open) return; - const onKey = (e: KeyboardEvent) => { - if (e.key === 'Escape') { - e.preventDefault(); - e.stopPropagation(); - close(); - } - }; - document.addEventListener('keydown', onKey, true); // capture phase = wins - return () => document.removeEventListener('keydown', onKey, true); - }, [open, close]); - - if (!open) return null; - + // The capture-phase Escape handling this component used to own is now + // the Modal's job, along with the focus trap it never had. return ( -
-
e.stopPropagation()} - > -
-

Keyboard shortcuts

- -
- -
- {SECTIONS.map((section) => ( -
-

- {section.title} -

-
- {section.items.map((item, idx) => ( -
- {item.description} - -
- ))} -
+ +
+ {SECTIONS.map((section) => ( +
+

+ {section.title} +

+
+ {section.items.map((item, idx) => ( +
+ {item.description} + +
+ ))}
- ))} -
+
+ ))}
-
+ ); } diff --git a/web/src/components/Tasks/TaskCreateDialog.tsx b/web/src/components/Tasks/TaskCreateDialog.tsx index 37de8333..e560d225 100644 --- a/web/src/components/Tasks/TaskCreateDialog.tsx +++ b/web/src/components/Tasks/TaskCreateDialog.tsx @@ -1,5 +1,7 @@ import { useState } from 'react'; -import { X } from 'lucide-react'; +import { Modal } from '../ui/Modal'; + +const FORM_ID = 'task-create-form'; export function TaskCreateDialog({ onClose, onCreate }: { onClose: () => void; @@ -16,55 +18,63 @@ export function TaskCreateDialog({ onClose, onCreate }: { }; return ( -
-
e.stopPropagation()}> -
-

New Task

- + {/* Outside the
, associated by id — keeps the button in the + modal's footer slot while Enter-to-submit still works. */} + + + } + > + +
+ + setTitle(e.target.value)} + autoFocus + className="w-full px-3 py-2 bg-surface-raised border border-border-subtle rounded-lg text-[14px] text-text outline-none focus:border-accent/50" + /> +
+
+ +