Repository navigation
Skip the unit and E2E suites on pull requests that change only Markdown - #660
Merged
Merged
Conversation
The six suite checks are required, so paths-ignore would leave a docs-only PR pending forever. A changes job reads the PR's files instead, and stub jobs report the same check names as passed on Linux when only .md files changed.
Moving a stacked Markdown-only PR onto trunk adds the code from the branch beneath it, and nothing re-ran, so the stub passes stood. Listen for edited, act only when the base changed, and keep title edits out of the cancelling concurrency group.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
zaerl
self-requested a review
October 8, 2026 08:39
zaerl
added a commit
that referenced
this pull request
Oct 8, 2026
## Why
`window.api.startServer` subscribes to `playground:log`,
`playground:url` and `playground:stopped` before it invokes
`playground:start`, and removes them only when the server announces it
has stopped. A start that **throws** (rather than answering `{ ok: false
}`) has no server, so nothing ever announces a stop and the three
listeners stay for the life of the window. The site's next server would
then open the site in the browser once more for each of them, and print
its log twice (#604).
Today a contributor cannot reach that next server: after a thrown start
the button refuses every further click until the app restarts. #603
fixes that dead button, and once it lands this leak becomes visible.
This PR is the half that makes the retry clean, and does not depend on
#603.
## What changes
- `startServer` in `src/preload.js` takes the shape `runPullRequest` in
the same file already has: one `cleanup`, called by the stop handler as
before, and now also when the invoke throws, before the error is
re-thrown.
- `startPhpServer`'s `catch` in `use-dev-server.jsx` also drops the mail
subscription, as `stopDevServer` does on every other way out. That
subscription is guarded against stacking (`use-site-mail.jsx`), so this
is tidying rather than a second leak: before, one subscription just
stayed open while no server ran.
Root cause: the only removal path ran on an event that a start which
throws before spawning can never produce. The `{ ok: false }` failures
(a server that exits before reporting an address, the 120 s timeout)
were never affected: each has a child whose `close` sends
`playground:stopped`.
Deliberately not here: the dead button after a thrown start. That is
#603, by another author.
## How to test this
**Platforms:** any. Nothing here touches paths, spawning or line
endings.
**Covered by the unit suite**, which runs on both platforms on every PR:
```bash
node --test tests/unit/preload-listeners.test.cjs
```
Run from the repository root. `startServer leaves no listener behind
when the start throws, and the next server is heard once` fails on trunk
(all three listener counts stay at 1, and the failed start's callbacks
receive the next server's log line and address) and passes here. A
second test pins the existing removal on `playground:stopped`, including
that another site's stop does not remove this site's listeners.
**By hand**, only once #603 is also applied, since without it there is
no second start to watch. Build from a branch with both, or from source:
**Starting state:** a built site, server stopped. In `src/main.js`, make
the `playground:start` handler throw once (the issue's step 1: `let once
= true;` above it, and `if (once) { once = false; throw new
Error('forced failure'); }` as its first line), then `npm start` from
the repository root.
1. Click **Start development server**. The Server log says `Failed to
start PHP server: … forced failure`, and the button goes back to **Start
development server**.
2. Click **Start development server** again. The site opens in the
browser **once**.
**What must not have happened:** the site opening in two browser tabs at
step 2, or each line of the Server log at step 2 appearing twice. Revert
the `main.js` edit afterwards.
## Risks and limitations
- Low. The change only runs on a path that ends in a thrown
`playground:start`, which needs `readSiteMeta`, the store,
`readSettings` or the spawn itself to throw. The normal start and stop
paths are unchanged, and the existing removal on `playground:stopped` is
now pinned by a test.
- The hook change has no test. It is one unconditional call inside a
`.jsx` hook, which the suite cannot load (TESTING.md, layer 1).
- Review: 0 [fix here] · 1 [follow-up], the follow-up being #603's fix.
## Related
Fixes #604. Independent of #603, and either can merge first. They touch
the same `catch` in `use-dev-server.jsx` on neighbouring lines, so the
second to merge may need a trivial rebase.
---
<details>
<summary>Design decisions and alternatives considered</summary>
- **Copy `runPullRequest` rather than invent a new shape.** It is the
bridge in the same file that already solved this, and the issue
suggested it.
- **Not a journey.** The leak lives entirely in the preload closure,
which `tests/unit/preload-listeners.test.cjs` already loads against a
fake `ipcRenderer` for exactly this class of bug (#86, #149, #167). That
is the highest layer that sees it. A journey would also need #603 to
reach a second start.
- **Not `stopDevServer()` in the hook's `catch`.** That would also reset
`devServerActiveRef` and fix the dead button, which is #603's change, by
its author.
</details>
<details>
<summary>Review outcome (required — see AGENTS.md)</summary>
**0 [fix here] · 1 [follow-up].**
- 🟡 [follow-up], architecture (failure paths), `use-dev-server.jsx`
`startPhpServer` `catch`: `devServerActiveRef` stays `true` after a
thrown start, so the Start button is a silent no-op until restart. It
predates this change and is exactly what #603 fixes, so it is deferred
to #603 rather than taken over here. The new test's header comment was
reworded after the review to say that the leak's effect is only
reachable once a retry is possible.
Lint and tests: `npm test` passes 2095, 0 fail. ESLint on the changed
files is clean. Repo-wide `npm run lint` fails locally only inside
gitignored `.claude/worktrees` copies, not in tracked files.
- **Review:** completed. Separate agent context (Claude Code Explore
subagent) against `.github/instructions/code-review.instructions.md`,
step 3. Covered the uncommitted change on base `f5b3b81`; outcome as
above.
- **Since review:** committed as `77246ea`. The only difference from the
reviewed diff is the test header comment reworded as described above; no
code changed. Trunk has since moved to `b17ca54` (#660, CI workflow
paths only), which does not touch these files.
</details>
<details>
<summary>Screenshots or recording</summary>
Nothing on screen changes.
</details>
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
zaerl
added a commit
that referenced
this pull request
Oct 9, 2026
…dited (#676) ## Why Since #660, editing a pull request's title or description turns its six required checks (`unit`, `Journeys` and `Packaged smoke` on macOS and Windows) into "Expected — Waiting for status to be reported", even though they already passed on that commit. The merge box shows them as pending until someone pushes or re-runs. Seen on #675: the runs at 07:59 passed, and the edits at 08:18 and 08:45 started runs 37904200109/37904200065 and 37906918098/37906918097 with every job skipped. ## What changes **Root cause:** #660 made `unit-tests.yml` and `e2e.yml` listen for `edited`, so that a base move would re-run their `changes` job. For any other edit, `changes` is skipped and every job after it is skipped too. GitHub treats the newest run of a workflow on a commit as that commit's checks. A skipped matrix job is reported with its name unexpanded (`unit (${{ matrix.os }})`), so the required `unit (macos-latest)` and the rest disappear from the newest run. **Fix:** both workflows stop listening for `edited`, so a title or description edit starts nothing in them and the earlier results stand. A new `.github/workflows/base-change.yml` handles a base move instead. It finds the latest Unit tests and E2E runs for the PR's head and re-runs them with `gh run rerun`; a run that is still in progress is cancelled first and re-run once it stops. A re-run is a new attempt of the same run rather than a newer run, so it replaces those checks directly. The `changes` job reads the PR's file list from the API at run time, so the re-run sees the new base: a Markdown-only stacked PR moved onto trunk is evaluated as code again, which is the guarantee #660 added. Deliberately not in it: re-running anything for a fork PR. A fork's token is read-only, so the workflow posts a warning instead (see Risks). ## How to test this Platforms: none. This is CI only, and it runs on GitHub's Linux runners. The workflows only take effect once this is on trunk: `edited` runs use the workflow files from the PR's merge commit, so a PR opened after this merges, or one that is pushed to after it merges, picks them up. **Starting state:** this branch merged to trunk. You need a pull request against trunk whose head was pushed after the merge, with all six required checks passed. 1. Edit that PR's description and save. Then edit its title. Expected: no new run of **Unit tests** or **E2E** appears in the Actions tab. **Re-run on base change** shows a run in which the `rerun` and `fork` jobs are both skipped. In the merge box, all six required checks are still ✓, not "Expected — Waiting for status to be reported". 2. Make a stacked pair: PR A (any code change) against trunk, and PR B with one Markdown-only commit on top of A's branch, opened against A's branch. Wait for B's checks. Expected: B passes the six checks through the stub jobs ("Only Markdown changed"). 3. On PR B, choose **Edit** next to the title and change the base from A's branch to `trunk`. Expected: a **Re-run on base change** run whose `rerun` job prints `re-running run <id>` for `unit-tests.yml` and `e2e.yml`. Unit tests and E2E each show a second attempt of their earlier run. In it, `changes` reports `code=true` (B now carries A's code), and the real macOS and Windows jobs run in place of the stubs. 4. Optional: repeat step 3 while B's Unit tests run is still in progress (push a Markdown commit to B, then retarget it straight away). Expected: the in-progress run is cancelled and then re-run, and the job log shows both. **What must not have happened:** - The six required checks going to "Expected" after step 1. This is the bug: on trunk today, step 1 reproduces it. - The stub passes surviving step 3. That would mean B could merge A's code without the suites having run on it. - A push made right after a retarget having its new run cancelled. The job compares the PR's live head with the event's head immediately before each re-run, and stops if they differ. **Cannot be tested here:** none of this runs before merge (see Platforms), and there is no unit test for workflow files. In particular, it is unverified that `GITHUB_TOKEN` with `actions: write` may cancel and re-run a `pull_request` run in this repository. Steps 3 and 4 are the check for that, and the first stacked retarget after merge should be watched. The workflows pass `actionlint` (with shellcheck), and the `gh run list` lookup was run against #675's real runs. ## Risks and limitations - **A re-run tests the old merge commit.** A re-run keeps the original event, so the suites check out the head merged into the *old* base. Only the Markdown-only decision uses the new base. That is what #660's guarantee needs, and it matches every PR run once its base branch has moved on. #660's `edited` runs did check out a fresh merge with the new base. - **Fork PRs:** a fork PR whose base moves is not re-run. A `::warning::` asks for a manual re-run or a push. `pull_request_target` would have a write token, but re-running a fork's run with it might bypass the approval that a first-time contributor's run waits for, so I did not use it. It is rare: a fork PR based on an upstream branch other than trunk. - **Runs older than 30 days:** GitHub refuses to re-run them. The job fails with an error that says to push, or to close and reopen the PR. That check is not required, so it does not block the merge on its own. - **PRs already open when this merges:** where the newest run on the head is one of the old skipped `edited` runs (as on #675 now), re-running it replays that skip. A push, or re-running the earlier passing run from the Actions tab, clears it once. - Review: 6 [fix here] · 1 [follow-up]. Five fixed; the remaining [fix here] (no live run yet) is covered by How to test steps 3–4. The follow-up is the open-PR transition above, noted rather than coded. ## Related Follow-up to #660. Seen on #675. --- <details> <summary>Design decisions and alternatives considered</summary> - **Keep `edited`, but give the matrix jobs expanded names when skipped:** not possible. GitHub does not expand a matrix for a job that is skipped at the job level, which is why #660 added stub jobs in the first place. - **Keep `edited` and run the stubs/real jobs again on every edit:** every title edit would cost two Linux jobs at best and the full macOS/Windows suites at worst, and it would cancel or duplicate runs. - **`workflow_dispatch` from the edit workflow:** this would start a fresh run on the head with the new base. But it is a different event, so the `pull_request`-only logic in `changes` would need a second branch, and the checks would come from a run nobody pushed. A re-run reuses everything that already exists. - **Concurrency:** base moves share the group `base-change-<PR>`, so a second move cancels the first and re-runs whatever the first left behind. Any other edit gets a group per run (`-<run_id>`), so it cannot cancel a base move. The concurrency group is evaluated before the job's `if:`, so without this a description edit made during a base move would have cancelled it. </details> <details> <summary>Review outcome</summary> 6 [fix here] · 1 [follow-up]: 5 fixed, 1 covered by How to test, 1 follow-up recorded in Risks. `npm run lint` clean; unit suite 2096 pass, 0 fail, 2 skipped (local, macOS). `actionlint` clean on all workflows. - **Review:** completed. Reviewer: a separate agent context, read-only. Head `da2dbfa`, base `7ee3ead`. Findings: - 🟡 A description edit could cancel a base-move run, because the concurrency group is evaluated before `if:`. Fixed. - 🟡 Re-running the old head's run after a push would cancel the new head's run through the shared concurrency group. Fixed. - 🟡 The 30-day re-run limit failed silently and aborted before the second workflow. Fixed. - 🔵 The fork regression was unannounced. Fixed with a warning job. - 🔵 [follow-up] PRs open across the merge. Recorded in Risks. - 🔵 The GitHub semantics are unverified in practice. Covered by How to test steps 3–4. - **Since review:** `da2dbfa → 667bd95` re-reviewed by the same separate context. #1–#4 resolved. One new 🔵: a push during the cancel-and-wait loop could still be cancelled. Fixed in `667bd95 → 8b5ea25`, which moves the head check to just before each re-run. I checked that last change myself; it was not reviewed again. - **CodeRabbit:** not requested yet. </details> <details> <summary>Implementation notes</summary> - The run lookup is `gh run list --workflow <file> --event pull_request --commit <head sha> --branch <head ref> --limit 1`. `--commit` matches the run's head SHA, not the merge commit's, and `--event` leaves out trunk push runs. - The job needs `actions: write` (cancel, re-run) and `pull-requests: read` (the head check). The workflow's default permissions are `{}`. Every value from the event reaches the script through `env:`, never interpolated into it. - `if ! gh run rerun` keeps a failure for one workflow from skipping the other under `bash -e`. The job then exits non-zero. - The `changes` jobs lose their `if:`, and the concurrency groups lose their `-edit` suffix, since neither workflow sees `edited` any more. </details> Screenshots: nothing on screen changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Every pull request runs the unit suite and the E2E journeys and packaged smoke on macOS and Windows, and all six checks are required to merge. A pull request that only changes Markdown (README, RELEASING.md, AGENTS.md and the like) waits on all of them even though nothing they test has changed.
What changes
A pull request whose changed files all end in
.mdskips the unit suite, the journeys and the packaged smoke, and still merges.paths-ignorecannot do this. A workflow it skips never reports its checks, so the required ones would sit at "pending" and block the merge. Instead, each workflow gets a smallchangesjob on Linux that reads the PR's file list from the API. When only Markdown changed,*-skippedstub jobs report the same required check names as passed, and the real jobs are skipped. Pushes to trunk always run everything.Deliberately not in it: anything under
docs/that is not Markdown (tests/unit/download-button-platform.test.cjsreadsdocs/.vitepress/theme), screenshots, and Lint, which is quick and also required.How to test this
Platforms: none, this is CI only. The checks on this PR and the next Markdown-only PR are the test.
Starting state: this PR, which changes two workflow files.
changesreportscode=true, and the realunit,JourneysandPackaged smokejobs run on macOS and Windows.What must not have happened:
.js, a VitePress config change, an empty list and a.jsfile renamed to.md, and against the real file lists of Document the release process in RELEASING.md #546 (Markdown only, skips) and Translate the line a screen reader says before each announcement #654 (code, runs).Cannot be tested here: the base-move case below needs a stacked pair of PRs to retarget, which I did not stage.
Risks and limitations
changesjob fails (for example the API call errors), both the real and the stub jobs skip, so the required checks stay pending. That blocks the merge rather than letting it through, and re-running the workflow clears it.changes. Without that, a Markdown-only PR stacked on a code PR and moved onto trunk by hand would keep its stub passes while its diff now contains the lower PR's code. The workflows now listen foreditedand act only when the base changed. One unverified detail: whether the PR's file list already reflects the new base when theeditedrun starts.changesscript is copied into both workflows rather than shared. Sharing it would mean a composite action for twenty lines.Related
None.
Design decisions and alternatives considered
paths-ignore/paths: rejected, the required checks would never report.if:on the existing matrix jobs alone: rejected. A skipped matrix job is not expanded, so it reports one check named literallyJourneys (${{ matrix.os }}), and the requiredJourneys (macos-latest)never appears. This is already what happens to drafts today (see [Fix] Stop a Gutenberg build from spawning processes without bound (#275) #283's checks).if:on every step of the real jobs, withruns-onfalling back to Linux: works, but every new step would need the guard, and a missed one would runnpm cion the wrong runner. The stub jobs keep the real jobs unchanged apart fromneedsandif.dorny/paths-filter: a third-party action holding a token, for somethinggh apidoes in one call.Review outcome
1 [fix here] after promotion · 0 left open. Lint clean; unit suite 2081 pass, 0 fail, 2 skipped (local, macOS).
ee41b6d/ base6d5f334. One 🟡 finding, raised as follow-up and fixed here at the author's request: a Markdown-only stacked PR moved onto trunk by hand kept its stub passes, since neither workflow listened foredited. Also noted, left as is: thechangesscript is duplicated across the two workflows.ee41b6d → 251885fre-reviewed by a separate agent context: finding resolved, no new findings. Checked title-only edits (earlier passes stand), base-move cancellation, the concurrency group expression for every event, and drafts.Implementation notes
gh api --paginate repos/…/pulls/N/files(up to 3000 files), takingprevious_filenametoo, so a.jsrenamed to.mdcounts as code.grep -v '\.md$' > /dev/null, notgrep -qv. Not everygrephandles-qwith-vthe same way.changesjob swapscontents: readforpull-requests: read; it never checks out. Fork PRs get that permission too.-editsuffix only for a title or body edit, so those runs cancel only each other. A base move joins the normal group and cancels the stale run.Screenshots: nothing on screen changed.
🤖 Generated with Claude Code