Move "Open a pull request" out of SiteRow into a hook and a component - #577

…nent (#578)

## Why

"Review & submit changes" was the last dialog still inline in
`index.jsx` (#553), and it has been leaving a part at a time so the
redesign in #542 can restyle each: the diff pane in #574, "Hand it to a
mentor" in #575, "Attach to Trac" in #576 and "Open a pull request" in
#577. This is the fifth and last part: the dialog itself, the frame
around the other four.

## What changes

`src/renderer/components/review-dialog.jsx` is new, and holds no state.

- **It is the frame**: the modal, what it says before the diff (an old
trunk, a diff that could not be read, nothing to send), the two columns,
and the heading and the warning that stand above the destinations.
- **The diff pane goes in as a prop and the destinations as children.**
They are the four components already moved, and `SiteRow` still hands
each what it needs.
- **The warning about someone else's work moved with it.** It speaks for
every destination at once, so it belongs to the dialog and not to any
one of them.
- **The diff, whether it is loading and whether it could be read stay in
`SiteRow`.** Discarding and saving are offered outside the dialog as
well and move the same values.

With this, `index.jsx` renders no modal of its own, which is what #553
asked for.

**No journey opened the dialog on a checkout that carries someone else's
work**, so one does now for an applied patch, as a fourth test in
`review-changes.spec.js`, and the pull request checkout journey asks the
same of a checked-out pull request. Both were written against the inline
markup first and pass on both.

## How to test this

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

The journeys carry this, from the repository root:

- `npx playwright test --project=journeys -g "someone else"` applies a
patch file through the app, edits another file and opens the dialog. It
asserts that the warning naming the patch stands between the heading and
the first destination and says a copy can still be kept; that the pull
request card says to revert the patch and offers no form, signed in
though the contributor is; that "Attach to Trac" and the mentor hand-off
will not save; that Save still asks where to put the file; and that
Escape closes the dialog.
- `npx playwright test --project=journeys -g "keeps ticket work"` now
also opens the dialog with a pull request checked out and an edit on
top, and asserts the warning and the card's refusal for that case, then
closes it with its close button.

The rest of the dialog was already covered: the diff, saving, copying
and discarding, a diff that could not be read, nothing to send, and each
destination.

What no journey reaches is the layout, which is in the stylesheet and
not in this diff:

**Starting state:** a site that has finished setup, with one file
edited.

1. Click "Review & submit changes". The diff is on the left and "Where
this patch goes" on the right, with the three destinations under it.
2. Narrow the window below about 900 pixels. The destinations move under
the diff, and the two scroll together.
3. Click outside the dialog. It closes.

**What must not have happened:**

- The destinations are not pushed off the dialog by a long line in the
diff: the diff scrolls sideways instead.

## Risks and limitations

- **Which warning wins when a checkout has both a pull request and an
applied patch is decided in two places**: here for the warning, as it
was, and in `prSubmissionBlocked` for the pull request card, which has a
unit test. The app cannot reach that state, so no journey can hold the
two together. Moving the warning's choice into the same module is a
follow-up.
- **The warning for a trunk more than a fortnight old has no journey**,
as before this PR. The app reads the age from the commit's date and the
fixtures commit now, so a journey would need a dated fixture. It is
moved as it was.
- **Closing by clicking outside the dialog has no journey**; Escape and
the close button do. The manual steps above were not run.
- **The pull request checkout journey is a few seconds longer**, and the
new journey costs one app launch on every pull request, on both
platforms: about three seconds locally on macOS.
- **Checked on macOS only.** Windows is covered by CI.
- Review, two passes: 1 finding to fix, fixed here; 2 follow-ups, both
above.

## Related

Part of #553: the last of five parts of its last dialog, and so the last
of #553. Part of #542. Follows #577. The layout is #186 and the
destinations #166.

---

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

**A frame with two slots, not a dialog that renders its own contents.**
The pane and the three destinations between them take about thirty
props, most of them `SiteRow`'s state. A dialog that rendered them
itself would take all thirty to pass them straight through. As slots,
the call site in `SiteRow` reads as the dialog's contents, and the
component is only what the redesign will restyle as one thing.

**No state moved.** The mentor hand-off owns its form and the pull
request has its hook, because nothing else needs those values. The diff
is different: "Discard all changes" on the site's own page reloads it
when the dialog is open, and "Save a copy of your work" on the
applied-patch panel writes the same saved-to line and the same error.
Moving that is splitting `SiteRow`'s state by domain, which is #554.

**The ownership warning keeps its own two branches.** The pull request
card's refusal is chosen by `prSubmissionBlocked`, which has a unit
test. The warning carries markup the card's sentence does not, so it was
moved as written, and the two journeys pin each branch.

</details>

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

**First pass: 1 [fix here] · 2 [follow-up].**

- Tests, low, [fix here], fixed in 6de82be: both journeys asserted that
the pull request card offers no form in a run where the app is signed
out, where it has none to offer whatever else is true. They now tell the
app it is signed in, wait until the card has heard, and then read that
the form is not there.
- Architecture, low, [follow-up]: the ownership warning chooses between
its two sentences inline, the same order `prSubmissionBlocked` decides
with a test. Moved as written; in Risks above.
- Tests, low, [follow-up]: the old-trunk warning in the dialog and
closing by a click outside have no journey. In Risks above.

The first pass diffed the old markup against the new file with
indentation ignored and found every element, attribute, style and string
identical. It confirmed that the stylesheet's `.patch-columns >
.patch-diff` and `.patch-columns > .patch-destinations` still match with
the pane passed as a prop, that the destinations mount and unmount at
the same moments as children, so the mentor hand-off's form still starts
empty on each opening, and that the screenshots still find what they
look for.

**Second pass, on 6de82be alone: 0 [fix here] · 0 [follow-up].** It
traced the wait and found it sound up to one thing it could not prove by
reading: that React has committed the account before the next query,
which nothing strictly orders. The mutation below is the evidence that
it has in practice.

- **Review:** completed — fresh agent context, local, before the PR
existed, read-only; head 9ae257c / base
20e7d11; committed work only, working
tree clean; then a second fresh context on
6de82be alone, which is the head;
outcomes above. CodeRabbit: not run yet.

Checks, macOS, local:

| Check | On | Result |
|---|---|---|
| `eslint . --max-warnings=0` | 6de82be | clean |
| unit suite on Node | 9ae257c | 1697 pass, 2 skipped, 0 fail |
| unit suite on Electron's Node | 9ae257c | 1699 pass, 0 fail |
| `npm run test:e2e` | 6de82be | 43 of 43 pass |
| `npm run test:e2e:packaged` | 9ae257c | 15 of 15 pass |

6de82be changes two journey files only.

Audit of the new assertions on 9ae257c, one break at a time, each
restored before the next. Every one goes red on the line that makes the
claim. On 6de82be, with the card changed to show the refusal as a banner
and the rest of its body under it, both journeys go red on the line that
reads the form's absence.

| Broken | Journey | Goes red at |
|---|---|---|
| No warning for an applied patch | review-changes | the warning |
| The patch warning does not say a copy can be kept | review-changes |
that sentence |
| The warning comes after the destinations | review-changes | the order
|
| The pull request card does not refuse | review-changes | the card's
sentence |
| The pull request card does not refuse | pr-checkout | the card's
sentence |
| "Attach to Trac" will save | review-changes | the disabled button |
| The mentor hand-off will save | review-changes | the disabled button |
| Save saves nothing | review-changes | the save dialog being asked |
| The dialog cannot be closed | review-changes | the dialog being gone
after Escape |
| The dialog cannot be closed | pr-checkout | the dialog being gone
after Close |
| No warning for a checked-out pull request | pr-checkout | the warning
|
| The pull request warning does not say a copy can be kept | pr-checkout
| that sentence |

The new journey first asserted the old-trunk warning too, with the
trunk's date seeded in the settings. It failed against the inline
markup: the app reads that date from Git. The assertion was removed
rather than faked, and the gap is in Risks.

</details>

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

---------

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

## Why

`SiteRow` holds the state of eight separate things in one function, and
#554 splits it into hooks by domain so the redesign in #542 can rebuild
the screen around pieces it can hold. The pull request's state already
left in #577. This is the first of the rest, and the one with the fewest
ties to anything else: the mail a site's WordPress sends.

## What changes

`src/renderer/hooks/use-site-mail.jsx` is new. It holds the list of
caught mail, the port the mail server listens on, the mail open in the
dialog, and the two listeners that keep the list live.

- **The dev server still says when the list is live**, because the mail
server runs with it. `SiteRow` calls `listen` before it starts the
server, so no early mail is missed, `load` once the server is up, and
`stopListening` when it stops. That is what the inline code did, in the
same places.
- **Those three functions keep their identity between renders.** The
callbacks that start and stop the dev server list them as dependencies,
so they are as stable as they were.
- **The markup stays in `SiteRow`.** This issue moves state; the mail
panel's markup moves when the redesign reaches that part of the screen.

**No journey had a mail arrive while the server ran, stopped the server,
or cleared the list**, so one does now, as a second test in
`mail.spec.js`. It was written against the inline state first and passes
on both.

**It is the first journey to speak for the main process**, with
`webContents.send`, so `TESTING.md` gains a convention for that: match
the payload against both ends, say what the stand-in leaves out, and
wait until the page has heard before reading an absence. `TESTING.md`
and `AGENTS.md` also said one journey clicks "Start dev server"; two do
now, both with the server's start answered by a stub.

## 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
tests/e2e/journeys/mail.spec.js`, from the repository root. The new test
says what the main process says when it catches a mail, and asserts that
the list is empty until a server has run; that the line above it names
the port once the mail server is up; that a mail caught for this site
joins the list and one for another site does not; that the list is
ordered by when a mail was sent and not by when it arrived; that
stopping the server puts the line back and a mail caught afterwards does
not join; that the next start loads the list from the store; that a mail
caught after a restart appears once; and that "Clear emails" empties
both the list and the store. The first test in the file, which opens and
closes a mail, is unchanged and passes.

What it cannot reach is a real mail from a real WordPress:

**Starting state:** a Core site that has finished setup, with no dev
server running.

1. Click "Start dev server" and wait for the site to open in the
browser. Under "Mail", the line reads "SMTP listening on 127.0.0.1:" and
a port.
2. In WordPress, go to the login page, click "Lost your password?" and
ask for a reset for `admin`. A "Password Reset" mail appears at the top
of the list in the app without anything being clicked.
3. Click the mail. It opens in a dialog; close it.
4. Click "Stop dev server". The line reads "SMTP will start with the dev
server." The mail is still in the list.
5. Click "Start dev server" again, wait for the site, and ask for
another reset. One new mail appears, above the first.

**What must not have happened:**

- The second reset does not appear twice. That would be a listener left
over from the first run.

## Risks and limitations

- **This changes the dependency lists of the two callbacks that start
and stop the dev server.** They are no less stable than before: three
stable functions replace three state setters, two refs and a stable
sorter. Nothing depends on either callback except the one on the other.
- **The journey announces two things the app cannot produce.** It says a
mail was caught without writing it to the store, which the real mail
server does first, and it says one was caught while the dev server was
stopped, when the real mail server is stopped too. Both are the only way
to see what they show: that a start reloads the list from the store, and
that nothing is listening after a stop.
- **Not changed, and older than this PR: the mail listeners are not
removed when a site's view is unmounted with the server running.** The
debug log's listener has that cleanup and mail's never did. The hook is
the place for it; this PR moves the code as it was.
- **The packaged smoke test was not run locally**: the app was running
on the same desktop and holds the single-instance lock. Nothing here
touches packaging beyond one new `.jsx` file under `src/renderer/`,
which the build excludes by rule; CI runs the test.
- **The manual steps above were not run.**
- **The journey costs one app launch** on every pull request, on both
platforms: about a second and a half locally on macOS.
- **Checked on macOS only.** Windows is covered by CI.
- Review, two passes: 1 finding to fix, fixed here; 2 follow-ups, both
older than this PR: the newest-first order is written once here and once
in the main process with no unit test, and a failed read or clear of the
list says nothing.

## Related

Part of #554: the first of its domains, after the pull request's in
#577. Part of #542. Follows #578.

---

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

**Mail first, not the dev server, though the issue lists the server
first.** The eight domains are not equally separable. The dev server,
the build watch, the terminal, the update and the apply chains read each
other's state through refs, and the first of them to move sets the shape
for the rest. Mail touches the others in exactly three calls, so it
shows the shape with the least at stake.

**Three verbs, not an effect.** The hook could have taken `running` and
subscribed in an effect. That would change when the listeners go on:
today they go on before the server is asked to start, which is the point
of the comment "to avoid missing early events", and an effect on
`running` would subscribe only after the server reported its address.

**The three functions are taken out of the hook's value by name.**
`mail` is a new object on every render. A callback that used
`mail.listen` would have to list `mail` as a dependency and would change
identity every render with it. Destructured, each function is a
dependency on its own and is stable.

**`sortEmails` is a plain function in the module.** It was a
`useCallback` with no dependencies, which is a constant with extra
steps.

</details>

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

**First pass: 1 [fix here] · 2 [follow-up].**

- Tests, low, [fix here], fixed in e500d5f: "Stop dev server" was
located inline in the new journey and in
`tests/e2e/real-setup/create-site.spec.js`. It is `stopDevServerButton`
in `tests/e2e/helpers/ui.cjs`, used by both.
- Architecture, low, [follow-up]: the newest-first comparator is a
decision with fallbacks, in a hook where the suite cannot reach it, and
the `smtp:get` handler in `src/main.js` has the same one. One shared
module with a unit test would hold both. Moved as it was.
- Architecture, low, [follow-up]: `load` swallows a failed read with an
empty `catch`, and `clear` has none, so a failed "Clear emails" is an
unhandled rejection with nothing on screen. Moved as it was.

Notes from the same pass, applied in e500d5f: the two documentation
sentences, the convention in `TESTING.md` (the reviewer named the
missing rule), and what the journey's stand-in leaves out, in its
comments and above.

The first pass compared every state variable, setter and listener with
`trunk`, and the order of what happens when the dev server starts and
stops. It confirmed that the setter order changing inside one batched
render has no visible effect, that `sitePath` cannot change for a
mounted `SiteRow`, that the listeners behave on unmount as they did, and
that nothing else in `SiteRow` touched the moved state. It checked the
journey's payloads against `src/preload.js` and `src/main.js`, and that
no real process can be reached with the stubs in place.

**Second pass, on e500d5f alone: 0 [fix here] · 0 [follow-up].** It
checked each statement in the new convention and in the journey's new
comments against the code.

Both passes left one thing unproven by reading: that React has committed
a state change before the next query when a journey waits for the page
to have heard. The same wait is in `review-changes.spec.js` and
`pr-checkout.spec.js`, and the breaks below are the evidence that it
holds in practice.

- **Review:** completed — fresh agent context, local, before the PR
existed, read-only; head 9308be7 / base
a5f95ff; committed work only, working
tree clean; then a second fresh context on
e500d5f alone, which is the head;
outcomes above. CodeRabbit: not run yet.

Checks, macOS, local:

| Check | On | Result |
|---|---|---|
| `eslint . --max-warnings=0` | e500d5f | clean |
| unit suite on Node | 9308be7 | 1697 pass, 2 skipped, 0 fail |
| unit suite on Electron's Node | 9308be7 | 1699 pass, 0 fail |
| `npm run test:e2e` | 9308be7 | 44 of 44 pass |
| the two journeys in `mail.spec.js` | e500d5f | 2 of 2 pass |
| `npm run test:e2e:packaged` | 9308be7 | not run: the app was running
on the same desktop and holds the single-instance lock |
| `tests/e2e/real-setup/create-site.spec.js` | e500d5f | not run: manual
workflow only; lint is the only check on its one changed line |

e500d5f changes two documents, the journey helper and two spec files.

Audit of the journeys in `mail.spec.js` on 9308be7, 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 |
|---|---|
| A mail is put on top whenever it arrives | the order of the three
mails |
| The port is not listened for | the line naming the port |
| New mail is not listened for | the new mail in the list |
| Stopping leaves the mail listener on | the mail caught while stopped |
| Stopping leaves the port on the line | the line after stopping |
| The dev server stopping does not stop the listening | the line after
stopping |
| The dev server starting does not start the listening | the line naming
the port |
| The list is not loaded when the server starts | the stored mail, in
both tests |
| Clearing leaves the store as it was | the store |
| Clearing leaves the list as it was | "No emails yet." |
| Opening a mail does nothing | the dialog, in the first test |
| Closing the mail does nothing | the dialog being gone, in the first
test |

"A mail caught after a restart appears once" has no break of its own:
within the app the only way to a second listener is a stop that leaves
the first one on, and that break goes red earlier, at the mail caught
while stopped.

</details>

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

---------

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

## Why

`SiteRow` holds the state of eight separate things in one function, and
#554 splits it into hooks by domain so the redesign in #542 can rebuild
the screen around pieces it can hold. The pull request's state left in
#577 and mail's in #579. This is the next: the Logs panel.

## What changes

`src/renderer/hooks/use-site-logs.jsx` is new. It holds what the Logs
panel shows: the dev server's output, the build watch's, and WordPress's
debug.log, with which tab is open, how many debug.log lines arrived
unseen, whether each pane is following its last line, and the tail of
the file.

- **The hook holds the text and not the processes.** Whoever runs one
still appends to its pane: the dev server to the Server tab, the build
watch to its own. The functions they call keep their names in `SiteRow`
and their identity between renders, so no dependency list there is less
stable than it was.
- **The dev server still says when debug.log is tailed.** WordPress
writes the file only while it runs, so `SiteRow` calls `startDebugTail`
where the server starts and `stopDebugTail` where it stops, in the
places the inline code did both.
- **The pane refs and the effects that keep a pane at its last line
moved with the state**, since they are one mechanism. The markup stays
in `SiteRow`.
- **Which tabs there are stays in `SiteRow`**: the build watch's tab is
titled by the watch's state, which is another domain's.

**Not in this PR: removing the install log's buffer.** `SiteRow` kept a
fourth text, for an install log that once had a pane. Nothing has shown
it since the install's output went to the terminal, yet every chunk of
install or script output is still appended to it. It moves here as it
was; taking it and its callers out is a change of its own.

**No journey touched the Logs panel**, so two do now, in a new
`logs.spec.js`. Both were written against the inline state first and
pass on both.

## How to test this

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

The journeys carry this: `npx playwright test --project=journeys
tests/e2e/journeys/logs.spec.js`, from the repository root.

- The first writes to the site's real `build/wp-content/debug.log`, the
way WordPress does, and reads it through the app's own tail. It asserts
that the tab holds nothing before a server has run; that lines which
arrive while the other tab is open are counted on this one and the count
goes when it is opened; that a line arriving while it is open is not
counted; that Copy copies what the pane shows and says so; that "Show in
folder" reveals the file the panel names; that Clear empties the file as
well as the pane and the tail goes on; that stopping the server leaves
the pane as it was; and that the next start replays the file into an
emptied pane, so each line is there once.
- The second says what the dev server would print, and asserts that the
Server pane stays at its last line as output arrives, stays where it was
put once scrolled up, follows again from the bottom, and opens at the
last line when its tab is come back to.

What they cannot reach is a real server and a real WordPress:

**Starting state:** a Core site that has finished setup, with no dev
server running.

1. Click "Start dev server" and wait for the site to open in the
browser. Under "Logs", the Server tab fills with the server's output and
stays at its last line.
2. Scroll the Server pane up. More output does not pull it back down.
3. Click the "debug.log" tab. The line under the pane is the path of a
file under `build/wp-content/`. Click "Server" again.
4. Make WordPress log something, for example by loading a page that
raises a PHP notice. The "debug.log" tab shows a count.
5. Click "debug.log". The line is there and the count is gone.
6. Click "Clear". The pane empties. Stop the dev server and start it
again: the cleared lines do not come back.

**What must not have happened:**

- After the restart in step 6, no line is in the pane twice.

## Risks and limitations

- **The largest move of state so far in #554**: nine pieces of state,
seven refs, six effects and eleven callbacks, called from the callbacks
in `SiteRow` that run the dev server, the build watch and the installs.
The names those callbacks use did not change, which is why the diff
outside the hook is mostly deletions.
- **The install log's buffer is still written and never read**, as
above. It costs a render of `SiteRow` per chunk of install output, as it
did before.
- **The build watch's pane has no journey of its own.** Its text comes
from the watch, which is another domain of #554 and will bring its
journey with it. Not covered either, as before this PR: what Clear and
"Show in folder" say when they fail (including "Show in folder" before
WordPress has written the file, which a contributor can reach), and a
copy that fails.
- **Moved as they were, and worth a change of their own:** Clear and
"Show in folder" word a refusal inline, so one from the main process
shows its raw code, where `open-failure.cjs` already has the sentences;
a tail that fails to start says nothing; and the Server pane's text
grows without a bound, as the install buffer does.
- **The first journey depends on the operating system reporting a change
to a file**, which is how the app's tail works. It passed every local
run on macOS; Windows is CI's to show.
- **The manual steps above were not run.**
- **The journeys cost two app launches** on every pull request, on both
platforms: about three seconds locally on macOS.
- Review, two passes: 4 findings to fix, all in the new journey's
comments and labels and all fixed here; the follow-ups are all older
than this PR and listed above.

## Related

Part of #554: the Logs domain, after the pull request's in #577 and
mail's in #579. Part of #542.

---

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

**One hook for the panel, not one per pane.** The three panes share the
tab that is open, the unread count depends on it, and the effects that
keep a pane at its last line watch it. Split by pane, each hook would
need the others' state.

**The build watch's text is here, though the watch is not.** A pane's
text, its ref and its follow state are one thing; who writes to it is
another. The watch calls `appendWatch` as the dev server calls
`appendRuntime`.

**The functions keep the names `SiteRow` already used.**
`appendRuntime`, `appendWatch`, `ensureStick` and `selectLogTab` are in
the dependency lists of the callbacks that run the dev server, the build
watch and the installs. Taking them out of the hook's value under the
same names leaves those callbacks and their lists untouched, so the
review of this change is the hook and four call sites.

**Refs made in the hook, attached in `SiteRow`.** The pane elements are
`SiteRow`'s markup and the scrolling is the hook's. A ref is the one
object both can hold.

**The install log's buffer was not deleted here.** It is dead, but
deleting it changes the callbacks that write to it and an option that
exists only to feed it, in the install and script runners. That is a
change to another domain's code and would make this more than a move.

</details>

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

**First pass: 4 [fix here] · 5 [follow-up].** It found no change in
behaviour; the four to fix are in the new journey.

- Tests, low, [fix here], fixed in d10af99: the block about a restart
said a listener left behind would show a line twice. It would not,
because a start takes off any it finds. The comment now names what the
block pins: the stop ended the tail in the main process, and the start
emptied the pane. The hook's comment that made the same claim is
corrected.
- Tests, low, [fix here], fixed in d10af99: the journey did not say what
its stand-in for the server leaves out, which `TESTING.md` asks for. The
file's header does now.
- Tests, low, [fix here], fixed in d10af99: the count of two after the
first start includes the app's own marker line, which is how things are
today and not something that must hold. That part is marked a
characterisation.
- Tests, low, [fix here], fixed in d10af99: an absence after Clear could
not fail on its own. Removed.
- Performance, medium, [follow-up]: the install log's buffer, written on
every chunk and read by nothing. In "What changes" above.
- Architecture, low, [follow-up]: Clear and "Show in folder" word their
refusals inline.
- Architecture, low, [follow-up]: a tail that fails to start is silent,
and a rejected stop is unhandled.
- Tests, low, [follow-up]: the paths of the panel with no journey.
- The headline counted five follow-ups and the list has these four; the
Server pane's unbounded text is named inside the first, and may be the
fifth.

Also in d10af99, from the same pass's unverified list: the scroll step
waited for two animation frames, which a hidden or covered window does
not draw. It sends the scroll event itself now, and the three breaks
that depend on it were run again.

The first pass compared every state variable, ref, effect and dependency
list with `trunk`, and the order of what happens when the dev server
starts and stops. It confirmed that no callback in `SiteRow` is less
stable than before, that the effects moving earlier in the component
changes nothing they depend on, that a ref made in the hook attaches as
one made in the component does, and that nothing renders the install
buffer. It traced the first journey against the real tail and found its
counts deterministic and a duplicate change event unable to double a
line.

**Second pass, on d10af99 alone: 0 [fix here] · 0 [follow-up]**, with
two notes on comment wording, both applied in 40d83de. It confirmed that
React delivers a dispatched scroll event to the pane's handler and that
the browser's own later event cannot change the outcome.

- **Review:** completed — fresh agent context, local, before the PR
existed, read-only; head 74d13fe / base
5898c04; committed work only, working
tree clean; then a second fresh context on
d10af99 alone; outcomes above.
CodeRabbit: not run yet.
- **Since review:** `d10af99 → 40d83de` checked. The difference is two
comments. It was not reviewed again by a fresh context.

Checks, macOS, local:

| Check | On | Result |
|---|---|---|
| `eslint . --max-warnings=0` | 40d83de | clean |
| unit suite on Node | 74d13fe | 1697 pass, 2 skipped, 0 fail |
| unit suite on Electron's Node | 74d13fe | 1699 pass, 0 fail |
| `npm run test:e2e` | d10af99 | 46 of 46 pass |
| `npm run test:e2e:packaged` | 74d13fe | 15 of 15 pass |

After 74d13fe the only change outside the journey is comments in the
hook. 40d83de changes comments only.

Audit of the journeys in `logs.spec.js` on 74d13fe, 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 |
|---|---|
| Lines that arrive unseen are not counted | the count after starting |
| Opening the tab leaves the count | the tab without a count |
| A line that arrives while the tab is open is counted | the tab without
a count |
| The path of the file is not kept | the path |
| Copy copies nothing | what was copied |
| Copy says nothing | "Copied" |
| "Show in folder" does nothing | what was revealed |
| Clear leaves the file | the file's size |
| Clear leaves the pane | the empty pane |
| A start does not empty the pane first | the cleared line appearing
once |
| No listener is ever removed, at a stop or at a start | the line
written while stopped, shown twice |
| Stopping does not stop the tail in the main process | the line written
while stopped, or the marker, depending on when the file's change
arrives |
| The dev server stopping does not stop the tail | the marker after the
restart |
| The dev server starting does not start the tail | the count after
starting |
| The server's output is not kept | the first line said |
| The Server pane does not follow | the pane at its last line |
| The Server pane is not the one the hook scrolls | the pane at its last
line |
| Scrolling away does not stop the following | the pane still at the top
|
| Scrolling back does not resume the following | the pane at its last
line |
| A tab come back to opens at the top | the pane at its last line |

Two things the second journey needed before it passed against the inline
code, both about the test and not the app: it spoke for the server
before the page was listening, so it now says one line until that line
is shown; and it read the pane's position before the page had been told
of the scroll, so it now tells the pane itself.

</details>

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

---------

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

## Why

`SiteRow` holds the state of eight separate things in one function, and
#554 splits it into hooks by domain so the redesign in #542 can rebuild
the screen around pieces it can hold. The pull request's state left in
#577, mail's in #579 and the logs' in #580. This is the next, and the
first of the five that are tied to each other: the terminal.

## What changes

`src/renderer/hooks/use-site-terminal.jsx` is new. It holds the xterm
instance and the effect that creates it, the line being typed and its
history, the handful of commands the terminal knows, and the lock that
says a command is running.

- **The lock is handed out, not hidden.** Every chain that runs an
install or a build (the setup, a trunk update, applying a patch,
switching tickets) takes the terminal's lock, writes its progress there,
and leaves there the function Ctrl+C should call. The hook returns the
lock's ref, the function that moves it, the kill handler's ref and the
writer, and `SiteRow` takes them out under the names it always used. No
chain changed.
- **The install and script runners stay in `SiteRow` and go in as
arguments.** They belong with the processes they start, which is the
next domain.
- **Three dependency lists gained the two refs.** The linter knows a ref
made in the component is stable and cannot know it of one that comes
from a hook. The refs are the same objects for the life of the
component, so those lists are as stable as they were.
- **The markup stays in `SiteRow`**, with the element the terminal is
drawn in.

**Not in this PR: a narrower interface to the lock.** Reading
`terminalStateRef.current.running` and writing `terminalKillRef.current`
from twenty places is what the code did and still does. Once the chains
are hooks of their own, they can be given `isBusy()` and
`holdLock(kill)` instead of two refs; doing it now would change every
chain in a PR that is meant to move one thing.

**One journey typed a command into the terminal and nothing more**, so a
new `terminal.spec.js` covers what it accepts, refuses, runs and stops.
It was written against the inline code first and passes on both.

## 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
tests/e2e/journeys/terminal.spec.js`, from the repository root. With the
three handlers that start and stop a run answered by stubs, it asserts
that the terminal opens on its help, naming the project's scripts; that
an unknown command and a script the project does not allow are refused
by name and nothing is run; that a character taken back with Backspace
is not part of the command; that an allowed script is run in the site's
directory; that while it runs its output is shown, the hints under the
terminal stop being links, and keys pressed start nothing; that Ctrl+C
asks for that run to be stopped and, with nothing running, stops
nothing; that a command clicked in the hints is put at the prompt and
not run; that a build ending does not remake the terminal; that the up
arrow steps back through what was run; and that `npm install` and `npm
i` run the install and start no script. `pr-checkout.spec.js` still
starts and stops a real command.

What it cannot reach is a real build:

**Starting state:** a Core site that has finished setup, so the hints
under the terminal are shown.

1. In the terminal, type `npm run build` and press Enter. The build's
output streams into the terminal, and the two commands in the hints
under it become plain text.
2. While it runs, type anything. Nothing appears.
3. Press Ctrl+C. The build stops, the prompt returns, and the hints are
links again.
4. Click "npm install" in the hints. The command appears at the prompt
and nothing runs until you press Enter.
5. Click "Update to latest trunk" in the site's "More" menu, on a site
where an update is available. Its progress is written to the same
terminal, and keys pressed in the terminal go nowhere while it runs.

**What must not have happened:**

- The terminal's earlier output is not lost at any point. A terminal
that empties itself has been recreated, which is the one failure this
move could introduce without any test noticing.

## Risks and limitations

- **The terminal must be created once and never again.** The effect that
creates it depends on three functions, and if any of them changed
identity the terminal would be disposed and made anew, scrollback and
all. They are as stable as they were, and none depends on the runners
that now arrive as arguments. The journey catches a terminal remade as a
command starts or ends, or while a build ends; one remade on any other
drawing of the view it does not catch, and the manual check above would.
- **Step 5 of the manual check is the part no journey reaches**: a chain
holding the terminal's lock through the refs the hook hands out. The
journeys for ticket switching, applying a patch and the trunk update all
run through those chains and pass, which is evidence the hand-out works,
though none of them asserts on the terminal.
- **The journey reads the terminal's screen from the markup of the
library that draws it**, because nothing with a role holds it. An
upgrade of that library could break the journey without breaking the
app.
- **The manual steps above were not run.**
- **The journey costs one app launch** on every pull request, on both
platforms: about two and a half seconds locally on macOS.
- **Checked on macOS only.** Windows is covered by CI.
- **Moved as they were, and worth a change of their own:** the
terminal's decisions (which commands it knows, the script check, the
history's rules) are in the hook, where no unit test reaches them. A
module with tests would hold them.
- Review, two passes: 7 findings to fix, all in the new journey or in
comments, all fixed here; 2 follow-ups, the one above and the paths with
no journey.

## Related

Part of #554: the terminal, after the pull request's state in #577,
mail's in #579 and the logs' in #580. Part of #542.

---

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

**The terminal first of the five that remain.** The dev server, the
build watch, the update, the ticket switch and the apply all write to
the terminal and take its lock; it takes nothing from them. Moving it
first means each of the others, when it moves, depends on a hook and not
on a block of `SiteRow`.

**The runners as arguments, not inside the hook.** `runInstall` and
`runScript` set the install and build flags the setup checklist reads,
record the run Ctrl+C stops, and reload the site's status when a build
ends. That is process state. The terminal only asks for a run and prints
what comes back.

**Refs handed out under their old names.** The alternative is an
interface the chains call. That is better, and it is a change to about
twenty call sites across four chains, each with its own ordering of
lock, kill handler and write. Kept as it was, the diff outside the hook
is one line of destructuring and three dependency lists.

**The stylesheet's import stayed in `index.jsx`.** Moving it with the
`Terminal` import would change where it lands in the bundled CSS, and so
what it overrides.

</details>

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

**First pass: 4 [fix here] · 2 [follow-up].** It diffed the old block
against the hook and found them identical apart from one renamed local
and the parameter that replaces `projectBuild.allowedScripts`.

- Tests, low, [fix here], fixed in add7415: the header did not name
everything the stand-in leaves out. It does now, and the log events
carry `type` as the real ones do.
- Tests, low, [fix here], fixed in add7415: the hints going plain while
a command runs had no positive control. They are shown to be links
first.
- Tests, low, [fix here], fixed in add7415: two reads of "nothing was
started" skipped the wait the others use.
- Tests, low, [fix here], fixed in add7415: the install step said "under
either of its names" and typed one of three. It types `npm install` and
`npm i`, and says the bare `install` is not typed.
- Architecture, medium, [follow-up]: the terminal's decisions are in the
hook, out of the unit suite's reach. In Risks above.
- Tests, low, [follow-up]: paths with no journey, among them Ctrl+C
during an install, the down arrow, upper-case commands, a script name
left out, and Gutenberg's list of scripts.

Notes from the same pass, applied in add7415: a comment about the
terminal's font left behind in `index.jsx`, a directory the journey
created that the fixture already has, and marks and wording on four
comments.

**Second pass, on add7415 alone: 3 [fix here] · 0 [follow-up]**, all
three about comments saying more than the code supports, all fixed in
75a3116.

- The journey's header said one terminal throughout needed no assertion
of its own. The reviewer showed that this fails for a terminal remade
while a build or an install ends: the exit line is printed after the
view is drawn again, so it lands in the new terminal and every later
step passes. The step where a build ends now looks for the banner, and
the header says what is pinned and what is not.
- "Here the store is not written" was too wide: an install that ends
well still marks the site initialised. It is the install's failure that
is not recorded.
- The hook's header left `normalizeForTerminal` out of what the effect
that creates the terminal depends on.

- **Review:** completed — fresh agent context, local, before the PR
existed, read-only; head 490216a / base
cb206e1; committed work only, working
tree clean; then a second fresh context on
add7415 alone; outcomes above.
CodeRabbit: not run yet.
- **Since review:** `add7415 → 75a3116` checked. The difference is one
assertion in the journey and three comments. It was not reviewed again
by a fresh context; the break in the audit below is the evidence for the
assertion.

Checks, macOS, local:

| Check | On | Result |
|---|---|---|
| `eslint . --max-warnings=0` | 75a3116 | clean |
| unit suite on Node | 490216a | 1697 pass, 2 skipped, 0 fail |
| unit suite on Electron's Node | 490216a | 1699 pass, 0 fail |
| `npm run test:e2e` | add7415 | 47 of 47 pass |
| the journey in `terminal.spec.js` | 75a3116 | passes, three runs in a
row |
| `npm run test:e2e:packaged` | 490216a | 15 of 15 pass |

After 490216a the only changes outside the journey are comments.

Audit of the journey in `terminal.spec.js`, on the commit each line was
written for, 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 |
|---|---|
| The help does not name the project's scripts | the help |
| An unknown command is met with silence | the refusal |
| A script the project does not allow is run | the refusal naming it |
| Backspace takes nothing back | the command starting |
| An allowed script is not started | what was asked for |
| What a script prints is not shown | the output |
| The screen is not told a command is running | the hints being plain
text |
| Keys pressed while a command runs are typed | the screen |
| A second command is started while one runs | what was asked for |
| Ctrl+C stops nothing | the stop asked for |
| A command ending does not give the prompt back | the hints being links
|
| Ctrl+C with nothing running stops the last command again | the stops
asked for |
| A suggested command is not put at the prompt | the prompt |
| A suggested command is run as soon as it is clicked | what was asked
for |
| The terminal keeps no history | the third run |
| `npm i` is not `npm install` | the second install asked for |
| `npm install` is not a command | the install starting |
| The install is not started | what was asked for |
| The hints are never links | the hints being links, before any command
|
| A refused script is run all the same | nothing having been started |
| An install that ends starts a build | no script having been started |
| The terminal is made anew whenever a command starts or ends | the
first command's first line |
| The terminal is made anew whenever a build starts or ends | the banner
being absent after the build |

The first audit found one assertion that could not fail: the hints going
plain while a build runs, which the build's own flag does whatever the
terminal's lock says. The journey now runs `npm run test` there, so the
lock is the only thing that can take the links away, and the break lands
on that line. It also showed that `$ npm run build` was already on the
screen from an earlier step when the journey looked for it at the
prompt; that check is anchored to the end of the screen now.

</details>

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

---------

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