Repository navigation
Ask and report in the app's own dialogs, not the window's confirm() and alert() - #665
Conversation
…nd alert() Five places still used the window's confirm() and alert(). Their message was translated, but Electron draws the dialog and its OK and Cancel buttons in English whatever the language. The three discards (all changes, the edits on trunk before a switch, and the edits before a trunk update) now ask through askFirst, in the same ConfirmDialog a deletion uses, with a button that says what it does. The two failures, a path that could not be copied and a dev server started before a build, are said in the window's error notices. confirmAnd and the no-alert exceptions are gone. Two of the discards are asked from inside the older library's modals, whose overlay is at 100000. The confirm dialog's portal sets --wp-ui-dialog-z-index to the same height, so it opens over them; the notices stay above it. The journeys that answered window.confirm now press the dialog, and the en-XA journey checks each question's title and buttons are translated. Fixes #658
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (17)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe described discard flows now use the application confirmation dialog with localized title, description, and button text. Selected error notices use the application confirmation queue instead of native alerts. End-to-end and unit tests now check the dialog content and discard behavior. Assessment against linked issues
Priority: ➖ Normal Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The reviewed dialog changes are mergeable after normal checks; the investigated discard failure is reported rather than escaping the workflow. 🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Description checkExplanation The description covers the required sections and provides detailed testing, risks, review outcome, and implementation notes. The required Screenshots or recording section is incomplete and still says “To add,” although this change modifies visible dialogs and notices.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Why
Five places still used the window's
confirm()andalert(). The message was translated, but Electron draws those dialogs and their OK and Cancel buttons in English whatever the language. Found in the en-XA walk (#658).What changes
ConfirmDialoga deletion already uses throughaskFirst.confirmAndis gone.confirm(message, { tone: 'error' }), which stay until dismissed.{ title, description, confirm }object, like the deletion questions:discardQuestion()inchanges-note.cjsreplacesdiscardConfirmMessage(), anddiscardTrunkEditsQuestion()is new inticket-actions.cjs. The yes button says what it does: Discard changes or Discard edits.--wp-ui-dialog-z-index: 100000inshell.css. That puts it level with those modals, and because its portal is added to the page later, it shows on top. The toast stack stays above it.confirm()oralert()is left insrc/renderer, and theno-alertexceptions are gone.How to test this
Journeys cover the whole flow and are red without the change:
i18n.spec.js("the ticket card's questions…" and "the trunk banners, the question before an update…") opens each of the three questions in en-XA and checks that the title, the yes button and Cancel are all bracketed. I checked it goes red when the button label skips__().trunk-update.spec.js("an update asks before it resets edits…") answers Cancel in the new dialog over the update question, then checks that the edit and the update question are both still there. It then answers yes and checks that the update runs. Before the stacking fix this was red, because the update modal's overlay intercepted the click.review-changes.spec.jsandpr-checkout.spec.jscheck that the discard asks first and touches nothing while it asks, then answer yes.The journeys cannot tell you how the dialog looks over another modal, so look at that by hand. Platform: either; build: the current head.
Starting state: a site with an uncommitted edit to any tracked file, opened with
npx electron . --lang=en-XAfrom the repository root (or the Buildkite build for the current head with the system language set to something translated).navigator.clipboard.writeText = () => Promise.reject(new Error('blocked')), then click Copy beside the path in the site's details (or Copy path in the site menu). Expected: a red notice in the bottom corner says Unable to copy path: blocked and stays until dismissed. Before this change it was a system alert.What must not have happened:
Risks and limitations
confirm()used to block the renderer, so the states that disable a discard could not change while it was up. The new dialog is modal, so nothing the contributor can press starts an install, build or server behind it. Something already running could still finish under it, but that only makes a discard less blocked. The comment abovediscardAllChangessays this. I added no re-check.ConfirmDialog, through its own portal. Other@wordpress/uidialogs keep their stacking.Related
Fixes #658. Part of #622.
Design decisions and alternatives considered
askFirstandConfirmDialograther than adding a new prompt component or a promise-returningconfirm. Every discard caller already had the "ask, then run this" shape thataskFirsttakes.:root. A global--wp-ui-dialog-z-indexwould lift the create-site, settings and rename dialogs as well, and would put them above any@wordpress/uimenu or popover opened inside them, which have no z-index of their own.docs/guide/submitting-changes.mdquotes it, so I updated that line.Review outcome (required — see AGENTS.md)
0 [fix here] · 1 [follow-up]: nothing fixed, one deferred.
ConfirmDialog'sonConfirmdrops the promise its action returns, so ifwindow.api.discardChangesrejects insidedirtyDiscardAndUpdatethe rejection goes unhandled and the contributor sees nothing happen. Deferred because it is not new:confirmAndhad the same gap, and fixing it means catching the invoke the waydiscardAllChangesalready does, which changes a failure path this PR does not touch.docs/guide/ticket-branches.md:61quotes the delete-work question as "This cannot be undone." while the app says "This can’t be undone." That predates this PR..github/instructions/code-review.instructions.mdstep 3, reviewed the working tree against base4835fba(identical to heada46d316); evidence:npm run lintclean,npm test2091 pass / 0 fail / 2 skipped,npm run test:electron2092 pass / 0 fail / 1 skipped, the 7 journeys inreview-changes.spec.js+trunk-update.spec.jsand the 48 ini18n.spec.js+pr-checkout.spec.js+delete-site.spec.js+ticket-list.spec.js+ticket-branches.spec.jspass; outcome: no blocking findings.a46d316unchanged.Implementation notes
useTrunkUpdatetakesaskFirstwhere it tookconfirmAnd.useDevServertakesconfirm, the same wayuseTrunkUpdateanduseApplyPatchalready do.acceptConfirms()is removed fromtests/e2e/helpers/app.cjsbecause nothing raises awindow.confirmany more. The journeys that used it now click the dialog throughui.confirmDialog/confirmYesButton/confirmNoButton.src/renderer/index.jsis not tracked, so there is no bundle in the diff.Screenshots or recording
To add: the discard question in en-XA over the review dialog, before (the system dialog with English OK and Cancel) and after.
🤖 Generated with Claude Code