refactor(website): unify modals onto BaseDialog, drop MUI modal and react-confirm-alert#6780
refactor(website): unify modals onto BaseDialog, drop MUI modal and react-confirm-alert#6780corneliusroemer wants to merge 8 commits into
Conversation
…eact-confirm-alert
Consolidate all modal/dialog mechanisms onto a single component, `BaseDialog`,
and remove the two extra dependencies they relied on.
- Add a `dismissible` prop to `BaseDialog` (default true). When false, backdrop
clicks and Escape no longer close the dialog (it can still be closed via the
close button or an explicit action). This replaces the separate "sticky"
Modal behavior. Documented when to use each mode.
- Replace the MUI-based `Modal` (only used by the SeqSet create form) with
`<BaseDialog dismissible={false}>` and delete `common/Modal.tsx`.
- Migrate `EditDataUseTermsButton` (hand-rolled native <dialog> + ModalBox) and
`RevokeButton` (confirmAlert + ModalBox) to `<BaseDialog dismissible={false}>`.
- Replace the imperative `displayConfirmationDialog` / react-confirm-alert with a
declarative `useConfirmDialog()` hook built on BaseDialog, and migrate all
call sites (ReviewPage, GroupPage, EditPage, DataUploadForm, SeqSetItem,
SeqSetItemActions).
- Delete now-unused `common/ModalBox.tsx`, remove the `react-confirm-alert`
dependency and its overlay CSS.
Button labels, dialog texts and the revocation reason input are unchanged so the
existing Playwright integration tests keep passing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When `title=''`, BaseDialog still rendered an empty `<h3>` (leading-6 + mb-4), adding ~2.5rem of dead vertical space that could push the panel past the viewport and cause a small scroll. Apply `sr-only` instead of `mb-4` when there is no title, so it takes no layout space while keeping the DialogTitle element for accessibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…matting
Address review feedback: revert the confirmation dialog call sites to the
original imperative `displayConfirmationDialog({...})` pattern instead of the
`useConfirmDialog()` hook, so callers don't have to render a `{confirmDialog}`
element. The dialog is still consolidated onto `BaseDialog` — it is now mounted
imperatively in a transient React root (replacing react-confirm-alert) rather
than via a hook.
Also fix Prettier formatting in RevokeButton.tsx.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ackground Go back to the declarative `useConfirmDialog()` hook (rendering BaseDialog within the component tree) instead of mounting it imperatively in a transient React root. The imperative `createRoot`-on-body approach lost Headless UI's background inerting, focus trapping and scroll lock: that behavior is coordinated through React context, so the Dialog must be part of the app's React tree. Mounting it in a detached root left the rest of the page interactive while a confirmation was open. Rendering the dialog in-tree restores correct inert/focus behavior. The RevokeButton Prettier formatting fix from the previous commit is kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The create and edit SeqSet modals wrap the same SeqSetForm but looked different:
the create modal used `max-w-3xl` (~768px) while the edit/export/citations modals
use `min-h-[60vh]` plus a `min-w-[1000px]` spacer (~1000px). Make the create modal
use the same wrapper so they match.
Also set `dismissible={false}` on both form modals: they hold significant
user-entered state, so a backdrop click should not discard it. The edit modal was
previously dismissible, which was inconsistent with the create modal.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # website/src/components/Edit/EditPage.tsx
corneliusroemer
force-pushed
the
unify-modal
branch
from
5d99ce9 to
b39ef1e
Compare
|
Claude finished @corneliusroemer's task in 2m 27s —— View job Code Review
This is a clean, well-motivated consolidation. Removing two modal mechanisms and two dependencies while preserving correct Headless UI inert/focus-trap behaviour (by keeping dialogs in-tree) is the right call, and the discussion thread confirms the trade-off was thought through. Two inline comments posted — both minor:
Everything else looks good: the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| <Button | ||
| variant='primary' | ||
| onClick={() => | ||
| void (async () => { |
There was a problem hiding this comment.
The void (async () => { ... })() pattern discards the promise entirely. If onConfirmation() rejects, the rejection becomes an unhandled promise rejection (potentially logged as an uncaught error in console), and close() is never called — the dialog stays open with no visible feedback.
Callers that handle errors internally (e.g. via toast in onSuccess/onError) are fine, but a caller that throws from onConfirmation will leave the dialog hanging open silently. Worth either documenting this contract or adding a minimal catch:
| void (async () => { | |
| onClick={() => | |
| void (async () => { | |
| try { | |
| await options.onConfirmation(); | |
| } finally { | |
| close(); | |
| } | |
| })() |
Though "always close on confirm click regardless of error" vs "keep open if action failed" is a design decision — just ensure it's intentional.
| <DialogTitle as='h3' className='text-2xl font-bold leading-6 text-gray-900 mb-4'> | ||
| <DialogTitle | ||
| as='h3' | ||
| className={`text-2xl font-bold leading-6 text-gray-900 ${title ? 'mb-4' : 'sr-only'}`} |
There was a problem hiding this comment.
When title='', Headless UI still wires up aria-labelledby on the <Dialog> to point to this DialogTitle element, which will have empty text content. Screen readers will announce the dialog with no name, even though there may be a visible <h3> heading inside.
Dialogs that pass title='' and supply their own heading (like RevokeButton, ConfirmationDialog, EditDataUseTermsButton) would benefit from either:
- Passing the actual title text to
BaseDialogso it appears inDialogTitle, or - Keeping
title=''but addingaria-labelto the wrapping<Dialog>as a fallback.
Not blocking, but worth a follow-up for the dialogs with non-trivial content.
There was a problem hiding this comment.
Pull request overview
This PR consolidates the website’s various modal/confirmation implementations onto a single Headless UI-based BaseDialog, removing react-confirm-alert and replacing the one remaining MUI Modal usage. This supports the linked accessibility goal of making background content inert to keyboard navigation while a modal is open.
Changes:
- Introduces a
dismissibleoption onBaseDialogand migrates remaining modal implementations (MUIModal,react-confirm-alert, and native<dialog>) toBaseDialog. - Replaces imperative confirmation dialogs with a declarative
useConfirmDialog()hook and updates call sites accordingly. - Removes now-unused modal components/styles and drops the
react-confirm-alertdependency.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/src/styles/base.css | Removes react-confirm-alert overlay CSS. |
| website/src/components/User/GroupPage.tsx | Migrates group membership confirmations to useConfirmDialog(). |
| website/src/components/Submission/FileUpload/FolderUploadComponent.tsx | Migrates discard/replace confirmations to useConfirmDialog() and renders the dialog declaratively. |
| website/src/components/Submission/DataUploadForm.tsx | Migrates “Open terms” confirmation to useConfirmDialog(). |
| website/src/components/SequenceDetailsPage/RevokeButton.tsx | Replaces react-confirm-alert revoke dialog with BaseDialog + local state. |
| website/src/components/SeqSetCitations/SeqSetListActions.tsx | Replaces MUI modal wrapper with BaseDialog for SeqSet creation. |
| website/src/components/SeqSetCitations/SeqSetItemActions.tsx | Migrates delete confirmation to useConfirmDialog() and sets edit dialog as non-dismissible. |
| website/src/components/SeqSetCitations/SeqSetItem.tsx | Migrates DOI creation confirmation to useConfirmDialog(). |
| website/src/components/ReviewPage/ReviewPage.tsx | Migrates approve/discard confirmations to useConfirmDialog(). |
| website/src/components/Edit/EditPage.tsx | Migrates submit confirmation to useConfirmDialog(). |
| website/src/components/DataUseTerms/EditDataUseTermsButton.tsx | Replaces native <dialog> usage with BaseDialog + state. |
| website/src/components/ConfirmationDialog.tsx | Replaces imperative confirm API with useConfirmDialog() hook built on BaseDialog. |
| website/src/components/common/ModalBox.tsx | Deletes unused ModalBox component. |
| website/src/components/common/Modal.tsx | Deletes unused MUI-based modal wrapper. |
| website/src/components/common/BaseDialog.tsx | Adds dismissible prop and improves title spacing behavior. |
| website/package.json | Removes react-confirm-alert dependency. |
| website/package-lock.json | Removes react-confirm-alert from lockfile (and related lockfile normalization). |
Files not reviewed (1)
- website/package-lock.json: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <DialogTitle | ||
| as='h3' | ||
| className={`text-2xl font-bold leading-6 text-gray-900 ${title ? 'mb-4' : 'sr-only'}`} | ||
| > | ||
| {title} | ||
| </DialogTitle> |
| className='min-h-[60vh]' | ||
| > | ||
| <div className='min-w-[1000px]'></div> | ||
| <SeqSetForm clientConfig={clientConfig} accessToken={accessToken} /> |
| <Button | ||
| variant='primary' | ||
| onClick={() => | ||
| void (async () => { | ||
| await options.onConfirmation(); | ||
| close(); | ||
| })() | ||
| } | ||
| > |
PR to Cornelius's PR #6780 Replace the useConfirmDialog() hook (which returned { confirm, confirmDialog } and required every call site to both call confirm() and render {confirmDialog}) with a module-level store and a single ConfirmDialogContainer host mounted once in BaseLayout. Call sites now just import and call confirm() — no element to render, and no risk of an early return omitting the dialog. A module-level store rather than React context is required because each Astro client:* island is an isolated React tree that context cannot cross. The host renders in-tree (not a detached createRoot), so Headless UI still makes the background inert and traps focus. 🚀 Preview: https://confirmation.loculus.org --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| } else void addAdditionalFiles(); | ||
| }; | ||
|
|
||
| return fileUploadState === undefined || fileUploadState.type === 'awaitingUrls' ? ( |
There was a problem hiding this comment.
I think the large diff in this file is not really needed? Its only a few lines that actually required changing right
(otherwise looks good to me)
Relates to #1548 (removing a MUI component)
Fixes #6744 (make background behind modal inert to keyboard navigation)
Summary
The website had four different modal/dialog mechanisms and three dependencies powering them:
BaseDialog(Headless UI)Modal(@mui/material/Modal)confirmAlert(react-confirm-alert) +ModalBox<dialog>+ModalBoxThis PR consolidates everything onto a single modal component,
BaseDialog, and removes the two extra dependencies (@mui/material/Modalusage andreact-confirm-alert).Testing
Clicked through the various changed modals. All seem to behave well. The keyboard navigation works great with background inert.
Detailed Claude generated description
What changed
One unified modal:
BaseDialogdismissibleprop (defaulttrue). Whenfalse, backdrop clicks and Escape no longer close the dialog — it can only be closed via the close button or an explicit action. This folds the old "stickyModal" behaviour into a single prop, and the component documents when to use each mode:dismissible={false}— confirmations and involved modals (forms) where the user might lose significant state to an accidental backdrop click.Migrated every other mechanism onto it
Modal(only used by the SeqSet create form) →<BaseDialog dismissible={false}>; deletedcommon/Modal.tsx.EditDataUseTermsButton(hand-rolled native<dialog>+ModalBox) →<BaseDialog dismissible={false}>.RevokeButton(confirmAlert+ModalBox, has a reason input) → declarative<BaseDialog dismissible={false}>with local state.Confirmations are now declarative
displayConfirmationDialog/react-confirm-alertwith auseConfirmDialog()hook built onBaseDialog. It returns{ confirm, confirmDialog }: renderconfirmDialogonce and callconfirm({ dialogText, onConfirmation, ... })from handlers. One hook instance per component covers any number of confirm actions (only one shows at a time).ReviewPage,GroupPage,EditPage,DataUploadForm,SeqSetItem,SeqSetItemActions.Cleanup
common/Modal.tsxandcommon/ModalBox.tsx.react-confirm-alertdependency and its.react-confirm-alert-overlayCSS.Scroll fix
title='',BaseDialogpreviously rendered an empty<h3>(leading-6+mb-4), adding ~2.5rem of dead vertical space that could push the panel past the viewport and cause a small spurious scroll. It now usessr-onlyinstead ofmb-4when there is no title, so it takes no layout space while keeping theDialogTitleelement for accessibility.Behaviour change worth flagging
The generic confirmation dialog is now not dismissible by backdrop click (it was
closeOnClickOutside: truebefore), matching the principle that confirmations shouldn't be lost accidentally. Users still cancel via the Cancel or close (✕) button.Testing
npm run check-types: clean (inwebsite/src).eslintonsrc: clean.prettier: clean.mainpreview backend on localhost (SeqSet create modal, revoke flow, confirmations) — including that the scroll glitch is gone.Confirm,Discard,Approve,Continue under Open terms,Revoke this sequence, "Are you sure you want to revoke this sequence?", "Enter reason for revocation"), all of which are preserved.🤖 Generated with Claude Code
🚀 Preview: Add
previewlabel to enable