Skip to content

Fix a site's terminal showing stretched text when the site was not open at launch - #592

Merged
zaerl merged 2 commits into
trunkfrom
fix/terminal-first-shown
Oct 1, 2026
Merged

Fix a site's terminal showing stretched text when the site was not open at launch#592
zaerl merged 2 commits into
trunkfrom
fix/terminal-first-shown

Conversation

@zaerl

@zaerl zaerl commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Why

A site's terminal shows its text stretched, every letter a whole cell apart and each line cut off half way, for any site that was not the one on screen when the app opened. For a contributor with two sites, that is every site but one, and it stays that way: the lines printed at startup never recover.

What changes

The root cause is where the terminal was opened, not how. Every site's view mounts inside a display: none wrapper, the selected one included, because the selection is made by an effect that runs after the views have mounted. The hook created the terminal and called term.open() in that hidden container. xterm's DOM renderer sets the spacing between characters from a glyph it measures in the document, and in a hidden container the glyph measures zero, so the spacing came out one cell wide. A row drawn then kept it.

The terminal is still created when the view mounts, so nothing written to it is lost: what is written before it opens stays in its buffer and is drawn when it does. Opening it moves to a second effect, which puts it on the page the first time its site is the one on screen. The hook is told which site that is, through a new isActive argument.

How to test this

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

A journey carries this. From the repository root, npx playwright test --project=journeys tests/e2e/journeys/terminal.spec.js; the second test in that file is new. It starts the app with two sites, opens the one that was hidden at launch, and measures how far the longest line of the terminal's help runs past the end of its row: nothing, for the site open at launch, for the one opened afterwards, and for the first again. On trunk it is red at the second measurement, with the line running 532 pixels past its row.

What it cannot reach is the look of it, which takes a minute by hand:

Starting state: the app with two or more sites.

  1. Click, in the sidebar, a site other than the one the app opened on, and scroll to its Terminal. The help text reads normally: "WordPress npm helper terminal." and the list of commands, each line whole.

What must not have happened:

  • Nothing the terminal was sent before its site was first shown is missing: a site whose setup ran while another site was on screen still shows that output when it is opened.

Risks and limitations

  • A terminal that is never shown is never opened. It still receives everything written to it, in its buffer.
  • The second effect runs after every render and returns at once when the terminal already has an element. It has no dependency list because the effect that creates the terminal can, in principle, create it again, and a new one has to be opened too.
  • The journey reads xterm's own markup, the rows it draws, as the first journey in that file already does and for the reason its header gives: nothing with a role holds what a terminal shows.
  • The manual step above was not run by a person.
  • Checked on macOS only. Windows is covered by CI.
  • Pre-PR review: nothing to fix here, three follow-ups, two of them bugs that were here before and are not this one. They are in the review outcome below.

Related

Found while building the new shell for #555, where switching sites is the main way around the app. After this merges, trunk is merged into redesign/wpds.

Review outcome (required — see AGENTS.md)

Run as the repository's review procedure, with the judgement pass in a fresh context that took no part in the change, on 848f596 against e1b17dd. The head, 7e76e31, differs from it by two comments in the journey, which that review asked for.

No finding to fix here; three follow-ups, all low. The reviewer read xterm 5.5's source for everything the app does with a terminal before it is opened and found each of it safe: writes are buffered and drawn by open(), scrollToBottom and focus do nothing without a viewport or a textarea, dispose of a terminal never opened removes nothing that is not there, and element is set by open() alone, which returns at once when called again. It also confirmed that a site's view and its isActive are decided by the same expression in the same render, so the terminal cannot be opened while its container is hidden, and that the site the app opens on gets isActive only after the selection effect, when its container is already shown.

Left as follow-ups:

  • The terminal does not let go of the bottom while a command runs. The check that decides whether to follow new output is always true, so scrolling up to read an error is undone by the next line. It was so before this change.
  • A log pane of a site that was hidden while its server or watch kept logging is not at the last line when the site is opened, until the next line arrives. Milder than the terminal's bug and the same kind.
  • The journey's last step reads rows drawn before the site was hidden. Rows drawn while an opened terminal is hidden are not exercised; the reviewer judged that path narrow, since xterm stops drawing a terminal that is out of view and keeps the widths it measured while it was in view. The step is marked as a characterisation and says what it reads.

Checks, on macOS, on the head of this branch:

Check Result
npx eslint . --max-warnings=0 clean
npm test 1733 pass, 2 skipped
npm run test:electron 1734 pass, 1 skipped
npm run test:e2e (journeys) 56 of 56 on 848f596; terminal.spec.js again on the head

The journey was audited by breaking the fix and running it: with the terminal opened as it is made, it is red on the measurement for the site opened later; with the terminal never opened, the first journey in the file is red at its first read of the screen. A third break, opening the terminal again each time its site is shown, turned nothing red: xterm returns at once from a second open(), so there is nothing for a test to see, and the journey does not claim there is.

🤖 Generated with Claude Code

zaerl and others added 2 commits October 1, 2026 23:50
@zaerl @claude
Open a site's terminal the first time the site is shown
848f596 
Every site's view mounts behind `display: none`, the selected one
included: the selection is made by an effect that runs after the views
have mounted. The terminal was opened in that hidden container. xterm's
DOM renderer derives its letter-spacing from a glyph it measures in the
document, and there the glyph measures zero, so the spacing came out one
whole cell wide. A row drawn while the site was still hidden inherited
it and kept it once the site was opened: every letter a cell apart, each
line cut off half way.

That is every site but the one the app opens on, for anyone with two
sites.

The terminal is still created when the view mounts, so nothing written
to it is lost, and is put on the page by a second effect the first time
its site is the one on screen. The hook is told which site that is.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@zaerl @claude
Say what the terminal journey's last step shows, and count its line r…
7e76e31 
…ight

The step that goes back to the first site reads rows drawn before the
site was hidden, so it is marked as a characterisation and says so. The
help line it measures is 74 characters, not 75.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 44f208ec-a3ad-41f5-bec0-eb4813baed18

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zaerl
zaerl merged commit 77f4cb7 into trunk Oct 1, 2026
8 checks passed
@zaerl
zaerl deleted the fix/terminal-first-shown branch October 1, 2026 22:20
zaerl added a commit that referenced this pull request Oct 1, 2026
@zaerl @claude
Merge trunk into redesign/wpds
da19ac6 
Brings in the terminal fix (#592): a site's terminal is opened the first
time the site is shown.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@zaerl zaerl mentioned this pull request Oct 2, 2026
Merged
zaerl added a commit that referenced this pull request Oct 2, 2026
@zaerl @claude
Move a site's status out of SiteRow into a hook (#593)
522b6ea 
## Why

#554 took eight domains out of `SiteRow`, and the largest thing it left
behind is the one nearly every hook and card reads: what the main
process says about the site. Thirteen pieces of state, set by one
function, sat in the middle of the component. The redesign's page header
and sites list (#556) need that status from outside the site's view, and
a hook is the first step to handing it to them.

## What changes

- **`src/renderer/hooks/use-site-status.jsx` is the site's status.**
Whether it is installed and built, the ticket it is working on, what is
applied to it, the age of its trunk, and the two states the app reads
and never writes over: a site the old engine made, and a merge left
unfinished outside the app. With them, `loadStatus`, which reads them
all in one answer, and the effect that calls it when the site's view
mounts.
- **Five setters are handed out**, as they were used before: linking or
leaving a ticket and moving one onto trunk, the dev server's start,
skipping the first-run checklist, and discarding every change. The other
eight values change only through `loadStatus`.
- **It is a move.** The lines are `SiteRow`'s own.
- **One new journey, `site-status.spec.js`**, for the one wiring the
move reshaped: the status reports to the sites list through a ref that
is now an argument, and nothing read that the report arrives.

This goes to trunk and not to `redesign/wpds` because nothing on screen
changes; trunk is merged into the redesign branch afterwards.

## How to test this

Platforms: any. This has no surface of its own: the screen is the same
before and after.

From the repository root, `npm run test:e2e` runs the journeys, all of
which read a site's status one way or another. The ones that would
notice a status not read, or read wrong, are `legacy-site`,
`merge-in-progress`, `pr-checkout`, `patch-apply`, `trunk-update-watch`,
`store-persistence`, `build-watch` and the new `site-status`; the audit
in the review outcome says which.

`site-status.spec.js` seeds the store with a trunk thirty days old for a
checkout whose trunk is minutes old, and expects the site's entry in the
list to end up with the name of a site that has nothing to report. It is
red when the report is dropped, when the hook is handed another ref, and
when the date is left out of the report.

**What must not have happened:**

- A site's view does not read its status more often than before:
`loadStatus` keeps its identity for as long as the site's path does, so
the effect that calls it on mount runs once.

## Risks and limitations

- **Two dependency lists gain a value** that was already stable:
`loadStatus` lists the ref it reports through, and the callback that
skips the checklist lists a setter that now comes from the hook.
- **One effect changes place.** The read on mount now runs before the
effect that clears the copy button's timer, which shares nothing with
it.
- **Six of the things the audit broke are not noticed by any journey**,
and were not before the move. They are listed in the review outcome;
none is new.
- **`loadStatus` swallows a failed read**, as it did in `SiteRow`: the
site then looks not installed and not built, nothing is logged, and the
setup chain does not start. Fixing it would stop this being a move, so
it is left for its own change.
- **Discarding all changes with a patch applied has no journey.** That
path does not read the status again, so one setter is all that clears
the applied-patch banner.
- **What is reported to the sites list is decided inside the hook**, out
of the unit tests' reach. It should move to a module before #556 reads
the status from outside the view.
- **Checked on macOS only.** Windows is covered by CI.
- **Pre-PR review, two passes: the move holds line for line; the hook's
header comment was wrong in several places and is rewritten; four
findings are left as follow-ups.**

## Related

Follow-up to #554. Prepares #556, part of #542.

---

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

Run as the repository's review procedure, with the judgement pass in a
fresh context that took no part in the change. First pass on the move
alone; second pass on the fix-up. The branch was rebased onto trunk
after #592 merged, and the checks below are on the rebased head.

**First pass: one finding to fix here, four follow-ups.** It diffed the
hook's body against `SiteRow`'s old lines and found two differences,
both intended; checked that every returned name is read only after the
hook's call and that the eight setters not returned were only ever used
inside `loadStatus`; and found the one effect that changed place, the
copy button's timer, sharing nothing with the status. To fix: the hook's
header said a finished build sets "built" (it goes through `loadStatus`;
the dev server's start is what sets it), that everything comes from one
read (the handed-out setters mean it does not always), and that the
status is read when a site's view is opened (every listed site's view
mounts as the app opens). Rewritten. Follow-ups, all in Risks above: the
silent `catch`, the missing journey for a discard with a patch applied,
the report to the sites list being untested, and that report's decision
living in a hook. The third is closed here by `site-status.spec.js`.

**Second pass: four findings, one medium, all fixed.** The medium one
asked that the new journey be shown to go red, since it never sees the
state it starts from: it does, three ways, as the table below records.
The other three were sentences in the two headers that left a case out.

**Checks, on macOS, on the head of this branch:**

| Check | Result |
| --- | --- |
| `npx eslint . --max-warnings=0` | clean |
| `npm test` | 1733 pass, 2 skipped |
| `npm run test:electron` | 1734 pass, 1 skipped |
| `npm run test:e2e` (journeys) | 57 of 57 on `62c5ab5`; the head
differs from it by comments, and `site-status.spec.js` was run again on
it |

**That it is a move.** Comparing the lines removed from `index.jsx` with
the hook's, every removed line is in the hook except two dependency
lists and a comment that said the trunk update was "below". No name the
hook returns is read in `SiteRow` above the hook's call.

**The wiring was audited by breaking it**, one thing at a time, and
running the journeys: 17 breaks, 11 caught.

| Break | Caught by |
| --- | --- |
| The status is not read when the view mounts | `build-watch` |
| A legacy site is not told apart | `legacy-site` |
| An unfinished merge is not seen | `merge-in-progress` |
| The checked-out pull request is not known | `pr-checkout` |
| An unfinished update is not known | `trunk-update-watch` |
| The applied patch is not known | `patch-apply` |
| The linked ticket is not read back | `store-persistence` |
| Whether the site is built is not known | `build-watch` |
| The dev server's start does not set "built" itself | **nothing**: it
decides from its own read, and the state only changes what is shown
until the next status read |
| Skipping the checklist does not set it itself | **nothing**: every
journey starts with the checklist already skipped |
| Linking a ticket does not set it itself | **nothing**: the link reads
the status straight after |
| Discarding does not clear the applied patch itself | **nothing**: no
journey discards with a patch applied |
| The status never stops loading | **nothing**: it only disables the
first-run checklist's buttons, which no journey shows; `setup-steps.cjs`
has unit tests for what it does with the flag |
| The status does not reach the sites list | `site-status` |
| The hook is handed another ref to report through | `site-status` |
| The trunk's date is left out of the report | `site-status` |
| `loadStatus` returns nothing to its caller | **nothing**: only the
setup chain reads it, and only the opt-in real-setup suite runs that |

</details>

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

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@zaerl zaerl added this to the v2.0.0 milestone Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

1 participant

@zaerl