Move the mail dialog out of SiteRow into a component of its own - #569

…o a component (#570)

## Why

The dialogs in `SiteRow` are leaving `index.jsx` one per pull request
(#553), so the redesign in #542 can restyle each as a component. This is
the third, after #568 and #569: the question an update to trunk asks
when it would reset edits that are not committed.

## What changes

`src/renderer/components/dirty-tree-modal.jsx` is new. It holds the
markup that was inline in `SiteRow`, with `dirtyFiles`, `dirtySaving`
and `dirtyError` arriving as the props `files`, `saving` and `error`.

**It owns one thing: which answer is selected.** It is mounted while the
question is open and not otherwise, so it starts on saving each time it
is asked, which is what `setDirtyChoice('save')` used to arrange by hand
on every opening.

**What each answer does stays in `SiteRow`**, with the state those
handlers write. `dirtySaveAndUpdate` and `dirtyDiscardAndUpdate` reach
the terminal, the ticket note and the update chain, and moving them is a
different change from extracting a dialog.

**No journey asked this question**, so one does now, in
`trunk-update.spec.js`, written against the inline dialog first and
passing on both. It gives both answers against a real reset, two updates
in one launch.

Deliberately not in this PR: the two dialogs still to move, and the gaps
in this one listed under Risks, which are how `trunk` behaves.

## How to test this

Platforms: any. Use the Buildkite build for the current head of this
branch.

The journey carries this: `npx playwright test --project=journeys -g "an
update asks before it resets"`, from the repository root. It asserts
that the dialog comes up and names the file, that it starts on saving,
that cancelling does nothing, that it cannot be dismissed while a save
is under way, that backing out of the save dialog resets nothing, that
saving writes the edit to the chosen file before the reset, and that
discarding asks once more and loses that edit only.

What it cannot reach is the operating system's save dialog, which it
answers for:

**Starting state:** a site that has finished setup and is behind trunk,
with one file edited and not committed.

1. Open the menu at the top right of the site and click "Update to
latest trunk". A dialog titled "Update to latest trunk?" lists the file,
with "Save them as a patch first (as a local file)" selected.
2. Click "Save patch & update". The system's save dialog opens; cancel
it. The question is still up and the file still has your edit.
3. Click "Save patch & update" again and choose a location. The dialog
closes, the update runs, and the file you chose holds your edit as a
diff.

**What must not have happened:**

- The edit is not lost in step 2: nothing is reset for a patch that was
never written.
- In step 3 the patch file exists before the working tree is reset, not
after.

## Risks and limitations

- **Discarding does not mark the dialog busy.** `dirtyDiscardAndUpdate`
never sets `dirtySaving`, so while the discard runs the dialog can be
closed and the discard and the update still go ahead, a failure is then
written to a dialog that is no longer there, and a second click asks a
second time and starts a second discard. This is `trunk`'s behaviour;
the move keeps it.
- **The close button and Escape stay live while saving.** The guard
makes them do nothing, but the dialog fades out and comes back, and
either pressed just before "Save patch & update" can close the dialog
once the save has started. Also `trunk`'s behaviour.
`@wordpress/components` has `isDismissible` and `shouldCloseOnEsc` for
it.
- **The sentence's plural and the table of answers are still in a `.jsx`
file**, where the unit suite cannot reach them. Moved, not added;
`dirtyTrunkQuestion` in `ticket-actions.cjs` is the shape they would
take.
- **The journey is the slowest of the dialog journeys**: about five
seconds locally on macOS, for two real updates.
- **One selector in it reads markup**: the components' own class for a
dialog that is fading out, because nothing on screen says "the fade is
over".
- Review: 2 [fix here], fixed; 3 [follow-up], the first three items
above.

## Related

Part of #553, which has two more dialogs. Part of #542. Follows #569.

---

<details>
<summary>Design decisions and alternatives considered</summary>

**The handlers stay where they are.** Moving `dirtySaving` and
`dirtyError` into the component would mean the handlers returning a
result for the component to show, which rewrites two functions that
discard work. The component takes them as props, and nothing about what
they do changes.

**Named for what it asks about, not for its title.** The title is
"Update to latest trunk?", but the code has always called this the
dirty-tree modal, and the update has other surfaces.

**Both answers in one journey.** A second launch would halve the run
time of each and double the launches. Both need a site behind its origin
with an edit in the tree, which is most of the cost.

**The save dialog is held open by the test.** The app builds the patch
before it asks where to put it, so the dialog comes up a moment after
the click. The test waits for it, looks at the app while the save is
pending, and answers when it chooses, which is also how it knows the app
has finished acting on the answer before it reads the tree.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

**2 [fix here] · 3 [follow-up]** — the two fixed, the three recorded
under Risks.

- Tests, medium, [fix here], fixed: the check that the question cannot
be dismissed mid-save looked at once after clicking the close button,
which fades the dialog out before asking to close it, so the dialog was
on screen either way. It waits for the fade to be over.
- Tests, low, [fix here], fixed: "cancelling does nothing" read the tree
the instant the dialog was gone. The update is asked for again first,
which goes through the main process's own look at the tree.
- Architecture, low, [follow-up], deferred: the discard path sets no
busy flag. Predates this change; a behaviour fix.
- Architecture, low, [follow-up], deferred: the close button and Escape
stay live while saving. Predates this change; a behaviour fix.
- Architecture, low, [follow-up], deferred: decisions moved from one
`.jsx` to another.

The review compared the inline markup with the component prop by prop
and found it the same apart from the renames and the callbacks, and
found the dialog closes the same four ways as on `trunk`.

- **Review:** completed — fresh agent context, local, before the PR
existed, read-only; head 559fed2 / base
3270f30; committed work only, working
tree clean; outcome above. CodeRabbit: not run yet.
- **Since review:** `559fed2 → 8cb6d09` checked. The difference is the
two fixes and one comment, all in the journey. It was not reviewed again
by a fresh context.

Checks, macOS, local:

| Check | On | Result |
|---|---|---|
| `eslint . --max-warnings=0` | 8cb6d09 | clean |
| unit suite on Node | 559fed2 | 1691 pass, 2 skipped, 0 fail |
| unit suite on Electron's Node | 559fed2 | 1693 pass, 0 fail |
| `npm run test:e2e` | 8cb6d09 | 37 of 37 pass |
| `npm run test:e2e:packaged` | 559fed2 | 15 of 15 pass |

Lint and the unit suites were run on this checkout's files only: another
local session's worktree sits under `.claude/worktrees/`, and the bare
commands collect it.

Audit, one break at a time, each restored before the next. Every one
goes red on the line that makes the claim:

| Broken | Goes red at |
|---|---|
| Cancel discards | no confirmation was asked |
| Cancel starts the update | the question coming up a second time |
| Cancel discards without asking | the question coming up a second time
|
| The dialog opens on "Discard" | the selected answer |
| The button says "Save" whatever is chosen | the button's name |
| The button saves even when "Discard" is chosen | the confirmation
count |
| The chosen answer is remembered for the next time | the selected
answer on reopening |
| The files are not listed | the file's name |
| The dialog can be dismissed while saving | the dialog still being up
after the fade |
| A cancelled save dialog still resets the tree | the edit still in the
file |

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

…e of its own (#571)

## Why

Two things about the app's main window were left open, and this closes
both as a security hardening.

The main window was not held to the page the app loads into it: it could
be navigated or redirected to another page, and it could open a second
window. The Trac window has been pinned to its origin since it was
written (`pinToTrac` in `src/trac-view.js`); the main window had nothing
of the kind.

And a mail's HTML part was put into the app's own document, so markup
the app did not write became part of the page: its styles applied to the
whole window, and its links and forms acted on the window itself.

## What changes

**The main window stays on its own page.** `pinToOwnPage` in the new
`src/window-navigation.js` cancels every navigation away from the page,
cancels a main-frame redirect, and denies every new window. An address
the window refuses is handed to `src/external-url.js`, the gate
`url:open` already uses: http and https open in the system browser,
anything else is refused and logged. A page reloading itself is still
allowed. `createWindow()` calls it, and the hand-off to the browser is
one function in `src/main.js` shared with `url:open`.

**A mail's HTML part is drawn in a frame of its own**, an `<iframe
srcdoc>` with an empty `sandbox`: no scripts, no forms, no new windows,
no navigating the window around it. The frame inherits the window's
content security policy, which needed no change for the frame to render
and keeps scripts and remote loads blocked inside it. A link in a mail
does nothing, and the mail stays on screen. The frame asks for the light
colour scheme, so a mail is drawn on a white page with dark text in
either theme, the way a mail client draws one, and fills its box to the
edge.

**The rule is written down where it is checked.** `react/no-danger` is
on for the renderer, so a new `dangerouslySetInnerHTML` fails lint, and
the review standard has a paragraph on both halves.

Deliberately not in this PR: the content security policy is unchanged,
links in a mail are not sent to the browser, and no other window is
touched.

## How to test this

Platforms: any. Use the Buildkite build for the current head of this
branch.

Three journeys carry this, run from the repository root:

- `npx playwright test --project=journeys -g "a middle click on a link
to the site"`
- `npx playwright test --project=journeys -g "a mail is read inside the
app"`
- `npx playwright test --project=journeys -g "a mail the site sent"`

All three are red on `trunk`'s code. The first is red without the pin (a
second app window opens and the address never reaches the browser). The
second is red when the mail is drawn in the dialog's own document, when
the frame is given more than the empty `sandbox`, and when a link
replaces the mail in its frame. The third now reads the rendered mail
through its frame.

What they cannot reach is a real dev server and a mail a real site sent,
since they stub the first and seed the second. The middle click needs a
mouse with a clickable wheel; a trackpad has no middle button.

**Starting state:** a site that has finished setup.

1. Click **Start development server** and wait for **View site**,
**wp-admin** and **Database** to appear in the site's details. The app
opens the site in your browser by itself at this point; close that tab.
2. Middle-click each of the three links. Each opens in your browser, and
Adminer keeps its full query string.
3. Plain-click each of the three. Same as before: each opens in your
browser.
4. In wp-admin, send a mail: on the login screen, **Lost your
password?** for the `admin` user.
5. Back in the app, click **Toggle Email** at the bottom of the window
and click the mail in the tray. It reads as it did before, **Rendered**
and **Raw** both.
6. If you have a plugin that sends HTML mail, open one of its mails. It
reads as a mail, in the app's font unless it sets its own. Click a link
in it: nothing happens, and the mail is still there.
7. In the settings, switch to the dark theme and open the same HTML
mail. It is on a white page with dark text that fills the bordered box,
with no dark band around it.

**What must not have happened:**

- No window of the app shows the WordPress site, after any click.
- The app window still shows the app, never WordPress and never an empty
page.
- The dialog's heading and tabs look the same whatever the mail's own
styles say.

## Risks and limitations

- **A link in a mail does nothing.** That is deliberate and recorded as
a characterisation in the journey. Sending it to the browser is a
separate decision.
- **Escape does not close the mail dialog after a click inside an HTML
mail**, because the keyboard focus is then in the frame. The Close
button works, and so does Escape before clicking in the mail.
- **An HTML mail scrolls inside its frame**, which is 60vh tall, instead
of stretching the dialog. It no longer inherits the dialog's styles; it
starts from the app's font and size on a white page, in either theme,
and its own markup wins from there.
- **Overlap with #358**, which addresses #284 by taking http and https
navigation and new windows to the browser. This pin does that and also
refuses every other scheme. The two touch the same lines in
`createWindow()` and `url:open`, so they need to be reconciled rather
than both merged as they are.
- I ran the journeys and both unit passes on macOS only. Windows is
CI's. The dark-theme drawing was checked in a standalone Electron page
reproducing the frame, not in the app: the journeys run in the light
theme only.
- Rebased onto `trunk` at 9e48b4d (the redesign, translations and themes
landed meanwhile). The rebased head had a fresh-context review; see the
review outcome.
- Review: 3 [fix here], all fixed, and 3 [follow-up], listed in the
collapsed block. A second review of the rebased head found 2 more [fix
here], both fixed, and no new follow-up.

## Related

Related to #284 and #358. Follows #569, which moved the mail dialog into
its own component.

---

<details>
<summary>Design decisions and alternatives considered</summary>

**The page the window may stay on is the one it is on**, read from the
window when the event arrives, not an address fixed in `main.js`. Both
strings come from Chromium, so there is no path to normalise across
platforms and no second copy of where the app's page lives.

**The pin takes one function, `openInBrowser`, and decides nothing about
schemes.** Which addresses may reach the OS stays in `external-url.js`,
in one place, and a refused one is logged there the same way as for
`url:open`.

**`will-redirect` acts on the main frame only.** Electron sends it for
frames inside the page too. A frame redirecting is not the window
leaving, and its address is not one to hand to the browser.

**An empty `sandbox`, and no sanitiser.** A sanitiser is a list of what
is dangerous that has to stay complete; the frame is a boundary that
does not depend on what the mail contains. It also needs no new
dependency.

**A `<base target="_blank">` in front of the mail.** Without it a link
with no target loads in the frame itself, the frame may not load it, and
the mail is replaced by an empty page. Aimed at a new window, which the
frame may not open either, the link does nothing and the mail stays. A
`srcdoc` document is always in standards mode, so putting it in front of
a doctype changes nothing about how the mail is laid out.

**The content security policy was checked, not changed.** A `srcdoc`
frame renders under `default-src 'self'` as it stands and inherits the
policy.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

3 [fix here] · 3 [follow-up]. All 3 fixed.

- **Review:** completed. Reviewer: a separate agent context with no
access to the authoring session, given the diff and
`.github/instructions/code-review.instructions.md`. Reviewed head
90098aa, base 3270f30 (`trunk`, no pull request existed yet). `npm run
lint` clean; `npm test` and `npm run test:electron` pass; all journeys
pass on macOS.
- **Since review:** `90098aa → bf568e8` is the three fixes below and
nothing else. Lint, both unit passes and all journeys re-run on bf568e8.
- **Rebase, 2026-10-08:** the three commits were replayed onto `trunk`
at 9e48b4d (now fb7674c, 74eeff6, 6f2d7d8). Conflicts were in
`AGENTS.md`, `TESTING.md`, `tests/e2e/helpers/ui.cjs` and
`mail.spec.js`, resolved to trunk's wording plus this PR's additions;
the frame journey now opens the Email tray first. d286d96 follows: the
frame asks for the light scheme and carries its padding, its size moves
to `shell.css` for the redesign's lint, and its title is translated. On
d286d96: lint clean, `npm test` and `npm run test:electron` pass, the
mail and site-link journeys pass on macOS, and the site-link journey is
red with the pin disabled.
- **Review of the rebase:** completed. Reviewer: a separate agent
context given only the diff and
`.github/instructions/code-review.instructions.md`. Reviewed head
d286d96, base 9e48b4d (`trunk`). `npm run lint` clean, `npm test`
passes. Outcome: 2 [fix here] · 1 [follow-up], all 🔵; both fixed in
4f8edb0:
- Tests, low: in `mail.spec.js`, the check that the second mail shows
nothing of the first searched the dialog only, which does not enter the
frame the first one's HTML part is drawn in. It now also checks the
second mail has no frame. Red with a stale frame drawn, where the old
line stayed green.
- Tests, low: the frame's translated title was read by no journey, since
`unwrapped()` collects no `title`. `i18n.spec.js` now checks it, and is
red with the title left untranslated.
- Follow-up, low: a hyperlink in the terminal. Already listed below; the
reviewer confirmed that on `trunk` it opened a second window with the
preload bridge, and that with the pin it does nothing after its
confirmation.
- Also in 4f8edb0: the comment over `MAIL_DEFAULTS` (a mail's styles
win, its `<base>` does not), `AGENTS.md` and `TESTING.md` naming every
journey that stubs the server's start, and a sentence in the review
standard's §5 on content moved into a frame.
- On 4f8edb0: lint clean; the mail, site-link and i18n mail journeys
pass on macOS. 4f8edb0 changes only tests, a comment and documentation,
so it was checked against the findings and not reviewed again.

Fixed:

- Tests, low: the wp-admin link's locator was written in two end-to-end
specs. It is named once in `tests/e2e/helpers/ui.cjs` now.
- Tests, low: a wiring test asserted that the pin ran before `loadFile`,
an order nothing can observe. Removed; the test with the real modules
already fails when the main window is not pinned.
- Architecture, low: `will-redirect` also fires for frames, and the pin
would have treated one as the window leaving. It acts on the main frame
only now, with a test.

Follow-ups, each a decision rather than an omission:

- Medium: links in a mail do nothing. Kept on purpose for this PR; see
Risks.
- Low: a hyperlink printed in the in-app terminal uses the terminal
library's default handler, which asks for a new window. With the pin
that window is denied, so the link does nothing after its confirmation.
To be routed through `url:open` separately.
- Low: one finding in a window this PR does not touch, which predates
the branch and is being handled on its own.

CodeRabbit: not requested yet.

</details>

<details>
<summary>Implementation notes</summary>

- `src/window-navigation.js`: `pinToOwnPage(webContents, { openInBrowser
})`.
- `src/main.js`: `openInBrowser(url)` wraps `openExternalUrl`;
`createWindow()` pins the window and logs a browser that could not be
opened; `url:open` calls the same function.
- `src/renderer/components/email-modal.jsx`: the frame and what goes in
front of every mail. `src/renderer/shell.css`: the frame's size, and a
framed mail's box without padding.
- `eslint.config.mjs`: `react/no-danger` for `src/renderer/`.
- `tests/unit/window-navigation.test.cjs` (layer 1),
`tests/unit/ipc-wiring.test.cjs` (layer 3: the stub window now records
its navigation listeners and its new-window handler),
`tests/e2e/journeys/mail.spec.js` and
`tests/e2e/journeys/site-links.spec.js` (layer 4).
- `AGENTS.md` and `TESTING.md` add `site-links.spec.js` (and the two
already missing, `auto-start.spec.js` and `i18n.spec.js`) to the
journeys that press "Start development server" with the server stubbed;
the Electron-pin guidance is unchanged.
- Checked on Electron 43.6.0: a reload from the menu and Playwright's
`page.reload()` do not raise `will-navigate`, so neither is affected;
`location.reload()` raises it with the page's own address, which is the
one navigation the pin lets through.

</details>

<details>
<summary>Screenshots or recording</summary>

Nothing new appears on screen. A plain-text mail is drawn exactly as
before, and an HTML mail is drawn in the same place in the dialog, on a
white page in either theme.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>