Ask and report in the app's own dialogs, not the window's confirm() and alert()

TESTING.md

@@ -223,7 +223,7 @@ Every matrix uses `fail-fast: false`, so a Windows failure never hides the macOS
 - **`npm test` is separated from the end-to-end suite by filename, not by directory.** `node --test` is run with no path at all, so it walks the whole repository and collects by name: `*.test.cjs` and four other shapes Node treats as test files — `*-test.cjs`, `*_test.cjs`, `test-*.cjs`, `test.cjs`. The journeys are `*.spec.js`, which matches none of them. Now that both suites live under `tests/`, that naming is the only thing keeping them apart, and a file under `tests/e2e/` named any of the five would silently join the fast suite and cost it an app launch. **End-to-end files are `.spec.js`.** The separation is worth defending: the fast suite has to stay something you run without thinking.
 - End-to-end selectors read the text and roles already on screen. No `data-testid` — the visible copy is the contract, and renaming a button is a change worth noticing.
 - Journeys run in the `en-US` locale, the source language, so no translation catalog loads and they read English text as written. The exception is `tests/e2e/journeys/i18n.spec.js`, which launches with `lang: 'en-XA'` (Chromium's pseudo-locale) and fails on any visible text that did not go through `__()`. See [Translatable strings](CONTRIBUTING.md#translatable-strings).
-- What is on screen is named once, in `tests/e2e/helpers/ui.cjs`, for the journeys, the other end-to-end specs and `scripts/screenshots/` alike, so renaming a button or rebuilding a card is one change. It holds every control that more than one of those files reaches, and the locators that find a part of the screen rather than something to press: the rendered app, an entry of the sites list, what a window with no site says, the open site's heading, a card, the work-item card with its heading and its ticket's number, a ticket's row, the question asked before a deletion, the window's confirmations, and the tray along the bottom of the window with the footer's button for it. A sentence one journey asserts, a button only one journey presses, and a selector only one file needs stay in that file: they are what it is about.
+- What is on screen is named once, in `tests/e2e/helpers/ui.cjs`, for the journeys, the other end-to-end specs and `scripts/screenshots/` alike, so renaming a button or rebuilding a card is one change. It holds every control that more than one of those files reaches, and the locators that find a part of the screen rather than something to press: the rendered app, an entry of the sites list, what a window with no site says, the open site's heading, a card, the work-item card with its heading and its ticket's number, a ticket's row, the question asked before a deletion or a discard, the window's confirmations, and the tray along the bottom of the window with the footer's button for it. A sentence one journey asserts, a button only one journey presses, and a selector only one file needs stay in that file: they are what it is about.
 - No test reads a component's source, the `.jsx` under `src/renderer/`, as text. A test that greps the component for a call, a literal or the order of two strings pins the file rather than the screen: it goes red when the code moves and stays green when the screen is wrong. What is on the page and where, a journey asserts, with order read as document order rather than from bounding boxes (the page scrolls as cards appear). How a component is allowed to be written, a lint rule asserts: `react-hooks/refs` is on for the renderer for that reason, and so are this repository's own two, `toolkit/no-hardcoded-colors` and `toolkit/no-inline-styles`, now that the window is painted from classes in `src/renderer/shell.css` with the design system's tokens. The first reports a hex colour or a function that takes a colour's channels, `rgb()` and the like, written in a string, and the second a `style` attribute on a JSX element. Neither sees a colour's name, or a style that reaches an element some other way; each rule's header says what it does not read. The repository's rules are in `eslint-rules/`, and `tests/unit/eslint-rules.test.cjs` runs each over code that must pass and code that must not, since a rule that stopped reporting would turn nothing else red.
 - Journeys mark each assertion as an **invariant** (must hold under any model of how work is stored) or a **characterisation** (true because of how the app stores things today). A red invariant is a bug; a red characterisation is a prompt to read it and update it deliberately.
 - A journey that needs something only a running process would say, a mail caught or a server's address, says it itself: `webContents.send` from `app.evaluate`, on the channel and in the shape the real sender uses. Check the payload against both ends, the handler in `src/preload.js` and the place `src/main.js` sends it, and say in the test what the stand-in leaves out, such as the store write the real sender makes first. To assert that the page did *not* react, wait until it has heard: after the telling, ask the main process one more question from the page (`window.api.getSitesWithMeta()` is a plain read) and assert once that answer is back. An absence read before then passes whatever the app does.

docs/guide/submitting-changes.md

@@ -21,7 +21,7 @@ If there are no changes to send, the screen says so. If the checkout is old, a w
 
 ## Discarding it all
 
-Next to the heading is **Discard all changes**. It asks first — *Discard all local changes? This cannot be undone* — and then throws away exactly what the diff above it shows, which on a linked ticket or issue means the whole of that work: your uncommitted edits *and* anything parked in a commit the last time you switched away from it.
+Next to the heading is **Discard all changes**. It asks first — *Discard all local changes? This can’t be undone.* — and **Discard changes** then throws away exactly what the diff above it shows, which on a linked ticket or issue means the whole of that work: your uncommitted edits *and* anything parked in a commit the last time you switched away from it.
 
 The work item itself survives. Its branch stays, the link stays, and you carry on working on it from a clean base. Throwing that work away along with its branch is a different gesture — **Delete this ticket's work** or **Delete this issue's work**, on the work-items card. See [Deleting a work item's changes](ticket-branches#deleting-a-work-item-s-changes).
 

src/renderer/changes-note.cjs

@@ -14,10 +14,19 @@
 
 const { __, _n, sprintf } = require('@wordpress/i18n');
 
-// Byte-identical to the confirm the dirty-update modal has always used, so
-// the same action reads the same everywhere it can be triggered.
-function discardConfirmMessage() {
-	return __('Discard all local changes? This cannot be undone.');
+/**
+ * What is asked before local changes are discarded, from the note, the review
+ * or the question an update asks over edits: one action, so one question
+ * wherever it is triggered from.
+ *
+ * @return {{title: string, description: string, confirm: string}} The question, and what its button says.
+ */
+function discardQuestion() {
+	return {
+		title: __('Discard all local changes?'),
+		description: __('This can’t be undone.'),
+		confirm: __('Discard changes')
+	};
 }
 
 // The branch namespaces a work item gets (#251): `ticket/` on a Core site,
@@ -310,4 +319,4 @@ function patchReviewContext({ pullRequest, tracTicket, workItemNoun = 'ticket' }
 	};
 }
 
-module.exports = { patchReviewContext, changesNoteParts, discardOutcome, applyFeedbackAfterDiscard, noteAfterDiscard, noteAfterProbe, discardBlocked, discardDisabledReason, discardConfirmMessage };
+module.exports = { patchReviewContext, changesNoteParts, discardOutcome, applyFeedbackAfterDiscard, noteAfterDiscard, noteAfterProbe, discardBlocked, discardDisabledReason, discardQuestion };

src/renderer/components/confirm-dialog.jsx

@@ -12,7 +12,9 @@ import { AlertDialog } from '@wordpress/ui';
  * is left, a yes included, and is where the question is put away.
  *
  * The design system's alert dialog is not closed by a press outside it, and
- * opens on its Cancel button.
+ * opens on its Cancel button. A discard can be asked from inside the older
+ * library's modals, the review and the question before an update, so the
+ * dialog's layer is lifted to theirs (see shell.css).
  *
  * @param {Object}   props
  * @param {?Object}  props.question  `{ title, description, confirm }`, or null for no question.
@@ -32,6 +34,7 @@ export function ConfirmDialog({ question, onConfirm, onClose }) {
     >
       {shown ? (
         <AlertDialog.Popup
+          portal={<AlertDialog.Portal className="confirm-dialog-layer" />}
           className="confirm-dialog"
           intent="irreversible"
           title={shown.title}

src/renderer/hooks/use-dev-server.jsx

@@ -27,13 +27,14 @@ import { serveWithoutWatch } from '../dev-server-command.cjs';
 // `terminalKillRef`, `markTerminalRunning` and `currentRunIdRef` are the
 // terminal's lock and the script runner's current run, which a stop clears.
 // `hasBuilt`, `setHasBuilt` and `skipInit` are what the site's status says,
-// and `projectBuild` is the project's build plan.
+// and `projectBuild` is the project's build plan. `confirm` says, in the
+// window's notices, that a start was refused for want of a build.
 //
 // `toggleDevServer` is what each of them does. `isServerStarting`,
 // `isDevProcessActive` and `serverFailure` are what the page's words about
 // the server are decided from, in site-processes.cjs, and `startElapsed` is
 // how long a start has been going.
-export function useDevServer({ sitePath, projectBuild, hasBuilt, setHasBuilt, skipInit, appendRuntime, revealServerLog, ensureStick, startDebugTail, stopDebugTail, listenForMail, stopListeningForMail, loadMail, startBuildWatch, watchStateRef, buildInterruptedRef, currentRunIdRef, terminalKillRef, markTerminalRunning }) {
+export function useDevServer({ sitePath, confirm, projectBuild, hasBuilt, setHasBuilt, skipInit, appendRuntime, revealServerLog, ensureStick, startDebugTail, stopDebugTail, listenForMail, stopListeningForMail, loadMail, startBuildWatch, watchStateRef, buildInterruptedRef, currentRunIdRef, terminalKillRef, markTerminalRunning }) {
   const [serverUrl, setServerUrl] = useState('');
   const [starting, setStarting] = useState(false);
   const [running, setRunning] = useState(false);
@@ -173,8 +174,7 @@ export function useDevServer({ sitePath, projectBuild, hasBuilt, setHasBuilt, sk
       // A start is already queued behind the watch (or in flight): a second
       // click must not queue a second server start (#488).
       if (devServerActiveRef.current) return;
-      // eslint-disable-next-line no-alert -- see the note above confirmAnd in index.jsx.
-      if (!skipInit && !hasBuilt) { alert(__('Please complete the full build before starting the dev server. You can also skip the wizard.')); return; }
+      if (!skipInit && !hasBuilt) { confirm(__('Please complete the full build before starting the dev server. You can also skip the wizard.'), { tone: 'error' }); return; }
       serverStartRequestedRef.current = false;
       devServerActiveRef.current = true;
       setServerFailure('');

src/renderer/hooks/use-trunk-update.jsx

@@ -4,7 +4,7 @@ import { savedAndResetMessage } from '../confirmations.cjs';
 import { planUpdateSteps, updateStepStatuses, skipInstallMessage, planWatchImpact } from '../update-plan.cjs';
 import { planUpdateHandOff } from '../update-handoff.cjs';
 import { watchOccupiesBuild } from '../watch-waiters.cjs';
-import { discardOutcome, discardConfirmMessage } from '../changes-note.cjs';
+import { discardOutcome, discardQuestion } from '../changes-note.cjs';
 import { pathBasename } from '../path-basename.cjs';
 import { updateHeldReason } from '../site-menu.cjs';
 
@@ -29,16 +29,16 @@ import { updateHeldReason } from '../site-menu.cjs';
 // some projects does the update's build (#507). `loadStatus` and
 // `refreshDirty` re-read the site once the tree has changed, and
 // `applyDiscardToNote` tells the unsubmitted-changes note that edits were
-// thrown away. `confirm` announces an outcome, and `confirmAnd` asks before
-// one that loses work.
+// thrown away. `confirm` announces an outcome, and `askFirst` asks before
+// one that loses work, in the app's own dialog.
 //
 // None of the functions here is memoised, as none was: they are called from
 // handlers and from each other, never listed as a dependency.
 //
 // Not here: the date of the site's trunk and the marker that an update is
 // incomplete. Both are read from the site's status with everything else the
 // status says, and this hook only asks for the status to be read again.
-export function useTrunkUpdate({ sitePath, confirm, confirmAnd, installing, building, runInstall, runScript, killCurrent, terminalStateRef, terminalKillRef, markTerminalRunning, writeToTerminal, refuseInTerminal, revealTerminal, watchStateRef, watchWaitersRef, pauseWatcher, resumeWatcher, watchRebuildsOnStart, loadStatus, refreshDirty, applyDiscardToNote }) {
+export function useTrunkUpdate({ sitePath, confirm, askFirst, installing, building, runInstall, runScript, killCurrent, terminalStateRef, terminalKillRef, markTerminalRunning, writeToTerminal, refuseInTerminal, revealTerminal, watchStateRef, watchWaitersRef, pauseWatcher, resumeWatcher, watchRebuildsOnStart, loadStatus, refreshDirty, applyDiscardToNote }) {
   const [updateState, setUpdateState] = useState('idle'); // idle | fetching | installing | building
   // Who runs the update's build: null for the chain itself, 'resumed-watch'
   // when the watch paused for the reset rebuilds from scratch as it resumes and
@@ -284,7 +284,7 @@ export function useTrunkUpdate({ sitePath, confirm, confirmAnd, installing, buil
     }
   };
 
-  const dirtyDiscardAndUpdate = () => confirmAnd(discardConfirmMessage(), async () => {
+  const dirtyDiscardAndUpdate = () => askFirst(discardQuestion(), async () => {
     setDirtyError(null);
     const d = await window.api.discardChanges(sitePath);
     if (!d || !d.ok) {