Replace native alert()/confirm() with tpen-alert/tpen-confirm modal dialogs#496
Replace native alert()/confirm() with tpen-alert/tpen-confirm modal dialogs#496
Conversation
Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
cubap
left a comment
cubap
left a comment
There was a problem hiding this comment.
spot checks and readthrough pass.
There was a problem hiding this comment.
Pull request overview
This pull request modernizes user interaction by replacing native browser alert() and confirm() dialogs with custom modal dialogs using the existing TPEN event dispatcher system. Native dialogs produce inconsistent, browser-controlled blocking UI that varies across browsers. The new implementation provides a consistent, styled modal experience across the application.
Changes:
- Replaced all
alert()calls withTPEN.eventDispatcher.dispatch('tpen-alert', { message })for non-blocking custom alert modals - Replaced all
confirm()calls with an async event-dispatch pattern using cross-removingone()listener pairs to handle positive/negative responses - Removed unnecessary
async/awaitkeywords from methods that were only async due toconfirm()usage
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| interfaces/manage-project/index.js | Replaced confirm dialog for manifest export and alert for permission check |
| components/roles-handler/index.js | Replaced alert for errors and confirms for unsaved changes, ownership transfer, and member removal |
| components/quicktype-manager/index.js | Replaced confirm dialog for clearing shortcuts |
| components/projects/project-list-write.js | Replaced alert for delete not implemented message |
| components/projects/index.js | Replaced alert for delete not implemented message |
| components/navigation-manager/index.js | Replaced confirm dialog for resetting navigation URLs |
| components/manifest-import/index.js | Replaced confirm dialog for multi-manifest import |
| components/manage-pages/index.js | Replaced confirm dialog for page deletion |
| components/manage-layers/index.js | Replaced confirm dialog for layer deletion |
| components/leave-project/index.js | Replaced confirm dialog for leaving project |
| components/decline-project/index.js | Replaced confirm dialog for declining invitation |
| components/annotorious-annotator/plain.js | Replaced alert for missing pageID and confirm for annotation removal |
| components/annotorious-annotator/line-parser.js | Replaced confirms for navigation with unsaved changes, annotation removal, and delete all annotations; removed async/await |
| api/Project.js | Replaced alert for role update failure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
The 'Yes' and 'No' do not always fit the wording, we will have to go through some of these. When active I can never tab-select yes or no. These should be accessible via keyboard-only means where tab selects the buttons, there is some selected/focused css, and 'enter' works to make the selection.
I can button mash and cause multiple dialogues. They are not cleared when you click 'no', so consecutive calls continue to show old dialogues.
This one is not actionable.
Fixed a small UX effect where activating the dialogue caused the page size to visually bump. This is because it would cause the vertical scrollbar to hide.
Static Review Comments
Branch: copilot/refactor-alert-confirm-dialogs
Review Date: 2026-02-24
Reviewer: Claude Code assisted by Bryan - @thehabes
Bryan and Claude make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.
Summary
The replacement pattern is mechanically correct and applied consistently. The primary concern is a race condition inherent to using a global event bus for dialog responses.
| Category | Issues Found |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 0 |
| 🟡 Minor | 2 |
| 🔵 Suggestions | 1 |
Critical Issues 🔴
Issue 1: Race condition — global event bus confirm listeners collide
File: All files using the tpen-confirm pattern
Category: Logic Error
Problem:
The confirm pattern uses the global eventDispatcher singleton for both dispatching the dialog and listening for the response. If two components fire tpen-confirm in quick succession (or a user triggers a confirm while one is already showing), the one() listeners from the first confirm are still registered when the second confirm's buttons fire. The cross-removal (off()) only cleans up the paired handler — it doesn't prevent a different component's stale handlers from also firing.
Consider this sequence:
- Component A dispatches
tpen-confirmand registersonPositive_A/onNegative_A - Before the user responds, Component B dispatches
tpen-confirmand registersonPositive_B/onNegative_B - User clicks "Yes" on Component B's dialog
tpen-confirm-positivefires — bothonPositive_AandonPositive_Bexecute (both registered viaone())
This means Component A's destructive action (delete, leave project, transfer ownership, etc.) executes without user intent.
How severe this is in practice: The ConfirmContainer does not queue dialogs — calling addConfirm() appends a new <tpen-confirm> element to the container. But the buttons for both dialogs dispatch the same tpen-confirm-positive/tpen-confirm-negative events, so any button click resolves all pending listeners. This is especially dangerous for irreversible operations like deleteAllAnnotations, transferOwnership, and removeMember.
Suggested Fix:
This is an architectural concern that may be out of scope for this PR, since the tpen-confirm event system predates these changes. However, the migration from synchronous confirm() (which inherently blocked and prevented races) to async events introduces this risk. A dialog ID or correlation token would solve it:
// Dispatch with a unique ID
const confirmId = crypto.randomUUID()
const onPositive = (ev) => {
if (ev.detail?.confirmId !== confirmId) return
TPEN.eventDispatcher.off('tpen-confirm-negative', onNegative)
// perform action
}
const onNegative = (ev) => {
if (ev.detail?.confirmId !== confirmId) return
TPEN.eventDispatcher.off('tpen-confirm-positive', onPositive)
}
TPEN.eventDispatcher.on('tpen-confirm-positive', onPositive)
TPEN.eventDispatcher.on('tpen-confirm-negative', onNegative)
TPEN.eventDispatcher.dispatch('tpen-confirm', { message: '...', confirmId })The ConfirmContainer would need to pass the confirmId through to the button handlers and include it in the dispatched events. This could be a follow-up issue.
How to Verify:
- Open a page that has two confirm-triggering buttons visible
- Use browser console to rapidly dispatch two
tpen-confirmevents - Click "Yes" on the second dialog and observe whether the first handler also fires
Major Issues 🟠
None
Minor Issues 🟡
Issue 1: confirm() dialog message reads like a statement, not a question
File: components/roles-handler/index.js:716
Category: UX / Copy
Problem:
The confirm message "This action will remove ${memberName} from your project. Click 'OK' to continue?" references "OK" but the tpen-confirm dialog uses "Yes"/"No" buttons:
TPEN.eventDispatcher.dispatch('tpen-confirm', {
message: `This action will remove ${memberName} from your project. Click 'OK' to continue?`
})Suggested Fix:
TPEN.eventDispatcher.dispatch('tpen-confirm', {
message: `This action will remove ${memberName} from your project. Do you want to continue?`
})Issue 2: manage-project/index.js alert fires then immediately redirects
File: interfaces/manage-project/index.js:74-75
Category: UX / Logic
Problem:
When the user lacks permission, the code dispatches a tpen-alert and then immediately sets document.location.href. The alert is non-blocking now, so the redirect will fire before the user sees the dialog:
TPEN.eventDispatcher.dispatch('tpen-alert', { message: "You do not have permissions to use this page." })
document.location.href = `/project?projectID=${TPEN.screen.projectInQuery}`With native alert() this worked because alert() blocked execution. With the event-based approach, the redirect fires immediately.
Suggested Fix:
Use tpen-alert-acknowledged to defer the redirect:
TPEN.eventDispatcher.dispatch('tpen-alert', { message: "You do not have permissions to use this page." })
TPEN.eventDispatcher.one('tpen-alert-acknowledged', () => {
document.location.href = `/project?projectID=${TPEN.screen.projectInQuery}`
})How to Verify:
- Navigate to
/project/manage?projectID=...as a user without edit permissions - Observe whether the alert message is visible before the redirect occurs
Suggestions 🔵
Suggestion 1: Extract a confirmAction helper to reduce boilerplate
Category: DRY / Maintainability
The 10-line confirm pattern is repeated identically ~14 times. A helper would reduce duplication and make the race condition fix (Issue 🔴 1) a single-point change:
// e.g., in api/events.js or a utility file
export function confirmAction(message, onConfirm, onCancel) {
const onPositive = () => {
eventDispatcher.off('tpen-confirm-negative', onNegative)
onConfirm()
}
const onNegative = () => {
eventDispatcher.off('tpen-confirm-positive', onPositive)
onCancel?.()
}
eventDispatcher.one('tpen-confirm-positive', onPositive)
eventDispatcher.one('tpen-confirm-negative', onNegative)
eventDispatcher.dispatch('tpen-confirm', { message })
}Usage becomes:
confirmAction("Delete all annotations?", () => {
// destructive action here
})This would also be the natural place to add the confirmId correlation token from Issue 🔴 1.
|
@copilot Review comments... There may be some places we might consider an OK/Cancel option instead of just Yes/No: |
…rm dialogs Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
Addressed in commit cfce025:
Done in commit cfce025. All 14 confirm call sites now pass explicit |
Introduce confirmAction in api/events.js to centralize confirm dialog handling with unique confirmId to avoid race conditions. Revamp ConfirmContainer to queue dialogs, handle keyboard navigation, manage focus, and dispatch confirm events with confirmId. Improve AlertContainer with keyboard handling and focus management. Replace many ad-hoc TPEN.eventDispatcher one/off/dispatch confirm patterns across components (annotorious-annotator, plain, decline-project, leave-project, manage-layers, manage-pages, manifest-import, navigation-manager, quicktype-manager, roles-handler, manage-project interface) to use confirmAction, simplifying code and reducing listener collisions. Also adjust redirect behavior after alerts in manage-project to wait for acknowledgement.
|
Hey @static-reviewer please take a look at this |
|
Claude Code Review -- Complete Review report posted below. |
|
Now I have all the context needed for a thorough review. Let me compile the report. Static Review CommentsBranch:
SummaryThis PR replaces native
Critical Issues 🔴Issue 1: XSS via unsanitized user message injected into innerHTMLFile: Problem: Current Code: // decline-project/index.js:108
this.shadowRoot.innerHTML = `
<h3> ${userMessage} </h3>
`
// leave-project/index.js:174 (error path)
this.shadowRoot.innerHTML = `
...
<code>${userMessage}<code>
`Suggested Fix: // Import escapeHtml from utils.js
import { escapeHtml } from '/js/utils.js'
// Then sanitize:
this.shadowRoot.innerHTML = `
<h3>${escapeHtml(userMessage)}</h3>
`How to Verify:
Major Issues 🟠Issue 1: Unclosed
|
…ub.com/CenterForDigitalHumanities/TPEN-interfaces into copilot/refactor-alert-confirm-dialogs
|
@copilot, have all these changes been addressed? |
Yes, all the issues raised in the review have been addressed. Here's a quick status summary:
|
Convert various modal/overlay containers to use the native <dialog> element and its showModal()/close() API. Update Alert and Confirm components to call showModal when showing and to toggle classes safely. Replace section/div overlays with dialog elements in AlertContainer, ConfirmContainer, ProjectTools, QuickTypeEditorDialog, and UpdateMetadata; add backdrop styles, sizing, and [open] display rules. Add cancel and backdrop click handlers to close dialogs (and prevent default cancel behavior). Ensure screen-locking sections are closed/cleaned up after dialogs dismiss (including retry logic in AlertContainer and delayed close in ConfirmContainer). Misc CSS tweaks to support full-viewport dialogs and backdrop behavior.
Introduce utilities/modalHost.js (openModalHost, closeModalHost, closeModalHostWhenEmpty) to centralize dialog host open/close logic and timing retries. Replace scattered showModal/close logic in Alert.js and Confirm.js to use openModalHost; update AlertContainer.js and ConfirmContainer.js to use closeModalHostWhenEmpty for coordinated closing after animations. Add addCustomAlert and a shared keyboard handler in AlertContainer, and simplify transcription code to call addCustomAlert directly. Files changed: components/gui/alert/Alert.js, components/gui/alert/AlertContainer.js, components/gui/confirm/Confirm.js, components/gui/confirm/ConfirmContainer.js, interfaces/transcription/index.js, utilities/modalHost.js.
|
Ok real world scenario AFK at this time. @static-reviewer review this and I will take a closer look when I am back at the keyboard |
Static Review CommentsBranch:
Critical Issues 🔴Issue 1: Duplicate import of
|
Introduce alert queuing and improve modal/confirm behavior and styling. Alerts are now queued (only one shown at a time) with addAlert/addCustomAlert pushing entries to a queue; show/dismiss logic uses a #showCurrent/dismissCurrent flow, native dialog cancel events for Escape, and Enter handling for activation. Confirm dialogs now use role="alertdialog", default focus is set to the negative/cancel button, keyboard navigation is improved (Tab/Shift+Tab cycling, Enter activation through shadow DOM), and positive/negative handlers validate confirmId against the active dialog. Both containers now append their singleton to document.body, and cancel handlers are removed when dialogs are dismissed to avoid leaks. Shared modal CSS was updated (transparent backdrop, color variables, selector changes to alertdialog, button layout and responsive tweaks). Also fixed an import in line-parser to use the utilities confirmAction path.
|
@thehabes Feel free to poke at this and see if you can find anything broken. |
cubap
left a comment
cubap
left a comment
There was a problem hiding this comment.
Some styling is still crunchy, but I think the layering, tabbing, dismissing etc all works
thehabes
left a comment
thehabes
left a comment
There was a problem hiding this comment.
Repeated the same test as the first review and could not reproduce any reported error.
Further tested by causing both success and error scenarios behind the positive selection. Errors occurred and were reported to the user correctly, and the dialogues dismiss in both scenarios.
No noticeable gaps in "keyboard only" functionality. Tab selection, selection indicators, focus, esc, and enter all work as expected.
The dialogues were never blocked by other element and were always actionable.
Static Review and Copilot Review also seem happy with it.
Native
alert()andconfirm()calls produce inconsistent, browser-controlled blocking dialogs. This replaces all instances across the codebase with the existingtpen-alert/tpen-confirmevent-dispatch system.Alert replacements
alert()→TPEN.eventDispatcher.dispatch('tpen-alert', { message })Affected:
annotorious-annotator/plain.js,roles-handler,projects/index.js,project-list-write.js,api/Project.js,manage-project/index.jsThe permissions-denied alert in
manage-project/index.jsnow defers the redirect viatpen-alert-acknowledgedso the user sees the dialog before being navigated away.Confirm replacements
All
confirm()calls are replaced via a newutilities/confirmAction.jshelper:The helper generates a unique
confirmId(crypto.randomUUID()with a fallback) for each dialog.ConfirmContainerechoes the ID through button-click events; listeners useon()with an ID guard so stale callbacks from earlier dialogs never fire on a later one.Affected:
decline-project,leave-project,manage-layers,manage-pages,manifest-import,navigation-manager,quicktype-manager,roles-handler(3 instances),line-parser.js(3 instances),manage-project/index.jsConfirmContainer improvements
tpen-confirmrequests while a dialog is already visible, preventing button-mash stacking.:focus-visibleCSS so the focused button has a visible outline, making tab/enter navigation work correctly.positiveButtonText/negativeButtonText. Destructive or statement-style confirms use specific labels (e.g., Delete/Cancel, Leave/Cancel, Publish/Cancel, Transfer/Cancel, Discard/Keep Editing); question-style confirms keep the Yes/No defaults.Methods that were
asyncsolely due toawait confirm(…)patterns have hadasync/awaitremoved from both the method and its call sites.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.