Open links in the browser for every click gesture, not just a plain click - #358

## Why

An agent found two rebase breakages on an external contributor's pull
request (#358): trunk had removed two exports the branch still imported.
The repository allows maintainer edits on that branch and each fix was
one line, so the agent pushed a merge and a fix commit to the
contributor's fork. Nothing in `AGENTS.md` said not to.

That is the author's call, not the agent's. A contributor's branch is
theirs: what lands on it, and when it merges, is decided by whoever
opened the pull request. The right move on a pull request the person you
work for did not open is a review with the findings written down.

## What changes

One section in `AGENTS.md`, under "Before opening a pull request": a
pull request you did not open belongs to its author. No pushes, rebases,
description edits or merges on it, whatever the branch permissions say
and however small the fix. Reviewing it and leaving findings, with a
diff where that helps, is what an agent may do. If the author is
unresponsive and the work is urgent, the agent says so and lets the
person decide how to reach them.

## How to test this

Platforms: any. Documentation only, nothing runs.

**Starting state:** any checkout of this branch.

1. Open `AGENTS.md` and find the section "A pull request you did not
open belongs to its author". It sits between the CodeRabbit paragraphs
and "The pull request description follows the template".
2. Read it as an agent about to act on a pull request someone else
opened. Expected: it names every action that is off limits (push,
rebase, update, edit description, merge) and the one that is allowed
(review and leave findings).

**What must not have happened:** no other line of `AGENTS.md` changed.
`git diff trunk --stat` lists one file with six added lines.

## Risks and limitations

None to the app. The rule constrains agents only; a human maintainer
keeps whatever latitude the repository gives them.

## Related

Prompted by #358, where the rule was broken before it existed.

---

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

Documentation change, six added lines in `AGENTS.md`, no code. Read once
against the surrounding sections for tone and placement; `npm run lint`
does not cover Markdown. Nothing to fix, nothing deferred.

</details>

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

Nothing on screen changes.

</details>

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

https://claude.ai/code/session_0123opUnXoxs1CAN7YQ7q8KU

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>