Repository navigation
Fix: Fixes dev server retry after start failure - #603
Open
singhakanshu00 wants to merge 2 commits into
Open
singhakanshu00 wants to merge 2 commits into
singhakanshu00 wants to merge 2 commits into
Conversation
|
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 |
1 task done
zaerl
added a commit
that referenced
this pull request
Oct 5, 2026
## Why #542 replaces the app's interface with the one in the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-prototype), drawn with the WordPress Design System. It was built in slices on `redesign/wpds`, so that trunk, which releases are cut from, never held a half-redesigned window. The window is whole now: the shell, the page header and details, every card and dialog, and the tray. This pull request is how the branch reaches trunk. It lands before the last three sub-issues, on purpose. The settings dialog (#559) and dark mode (#560) add to a finished window and do not need a branch to hide in, and the guide (#561) is easier to rewrite against what is on trunk. They follow as pull requests against trunk. ## What changes Everything `redesign/wpds` holds; this pull request adds nothing of its own. Each slice was reviewed in its own pull request against that branch, and says there how to test it and what it leaves out. - **The shell** (#555): a sites list, the page area and a footer, in a larger window. #590 - **The page header and the details** (#556): the site's menu in the header, and what the checkout is in a sidebar that can be put away. #594 - **The site's page** (#557): the server and the build watch in the header and the details (#595); the ticket card, the apply card with a preview dialog, and the list of a site's tickets (#596, #597, #599); the create-site, rename and delete dialogs (#600, #601); confirmations as notices in the window's corner (#602); the page's notices, update card and setup checklist (#605); the terminal, logs, mail and the review dialogs painted with the design system's tokens (#606, #608); and lint rules that keep it so (#609, #610). - **The tray** (#558): along the bottom of the window, holding the terminal, the logs or the mail, with a count of unseen debug.log lines on the footer (#612, #613, #614, #615). - Along the way: #611 (the site menu's update item says why it is held), and three that tidy up before landing: #616 (a journey's race with the page's clock), #617 (comments the finished redesign made untrue) and #618 (the server's button by its present name in `AGENTS.md`). **What a contributor will notice first:** the terminal, the logs and the mail are no longer on the page but behind three buttons at the foot of the window; the terminal is dark text on light; the server and the build watch are started from the header; a patch is previewed in a dialog before it is applied; and what worked is said in the window's corner. **Not in this pull request:** - The settings dialog (#559). Until it, nothing about how the window is arranged is remembered across restarts: the details, the tray's height, the apply card's fold. - Dark mode (#560). The terminal reads its colours once, when it is made, and will need to follow the theme. - The user guide and its screenshots (#561), which describe the old interface. They are published from release tags, so they are not wrong until a release ships this; #561 is next, ahead of #559 and #560, for that reason. - How a mail is drawn, and how the window handles a link. Both are unchanged and tracked separately. ## How to test this Platforms: macOS and Windows, both. Buildkite builds signed artifacts for this pull request; use the build for its current head, and check that it is: a push to the branch makes earlier builds stale. The suite, from the repository root, after `npm ci`: ``` npm run lint npm test npm run test:e2e ``` 113 journeys walk every slice on macOS and Windows on each push. What they do not do is run a real site: a clone of `wordpress-develop`, an install, a build and a server are stood in for. The one test that does, `real-setup.yml`, is run by hand and was run on this branch at `548c020` (https://github.com/WordPress/contributor-toolkit/actions/runs/37298667056): a Core site on macOS and on Windows and a Gutenberg site on macOS were created, installed, built and served, and passed. **The Gutenberg site on Windows did not finish.** Its first attempt was cancelled after 74 minutes inside the journey, a minute short of the limit that would have failed it with evidence, so it says nothing about where it was; a second attempt was still running when this was written and its result is not in this description. The commits since `548c020` change a journey, comments and prose. **A pass by hand, on a real machine** (any platform; Apple Silicon for the server, which no test on a pull request starts for real). It follows the order a contributor meets things in: 1. Open the app with no sites. **Create new site**, a Core site, in a folder with a space in its path. The setup checklist runs its steps; the terminal's button at the foot of the window shows what each is doing. 2. Let the checklist's last step start the server, and finish WordPress's own setup in the browser. Back in the app the header says "Server running" and "Build watching". Stop the server and start it again: from **Server running** in the header, or with **Stop** and **Start** beside "Server" in the details. 3. Press the three buttons at the left of the footer in turn. Terminal: type `help`. Logs: the server's lines, the build watch's, and Debug.log. Email: where SMTP is listening. Drag the tray's top edge. 4. Link a ticket by its number. The card shows the ticket and its pull requests and patches. 5. Apply one of them. The preview dialog names the files; **Apply and rebuild** runs the steps on the card. 6. Edit a file under `src/`, then **Review & submit changes**. The diff is the edit and nothing else. 7. Create a second site, or open one from before, and move between the two from the sites list. Each has its own terminal, logs and mail. 8. **Update to latest trunk** from the site's menu. **What must not have happened:** work lost in a link, an apply, a switch or an update that did not say so; `node_modules` rebuilt where nothing changed; a failure said only in a tray that stayed closed; one site's output in another's tray; anything on the page under the tray. **What cannot be tested by hand, and what was not:** none of the slices' manual steps were run by a person. I looked at screenshots and drove the app through the journeys; everything local was on macOS, with Windows left to CI. This pass is the first one by hand. ## Risks and limitations - **It is one commit on trunk**, since the repository allows only squash merges. The slices' pull requests are its record. `redesign/wpds` should not be branched from after this: it will have diverged from trunk. - **Reviewed in slices, not as a whole.** Twenty-two pull requests, all but a two-word change to `AGENTS.md` reviewed by a separate reviewer context before they were opened, most in two passes; see below. The branch was then swept once for what falls between slices, which found comments and one unread field (#617) and the three points that follow. - **Three questions and two failures still use the system's own dialogs**: the discards ask through its confirm box, and a path that could not be copied and a server started before there is a build are said in its alert. #557 meant to replace them and did so for deletions only. - **Some dialogs are repainted, not rebuilt.** The review dialog and its destinations, the mail dialog and the question an update asks about loose edits have the design system's colours and type on the old component library's parts. - **A real Gutenberg setup on Windows is not shown to pass on this branch.** See "How to test this". It passed on trunk before the redesign, in 35 minutes; here it was past 46 on its second attempt. Whether that is the runner or the branch is not known. - **The guide describes the old interface** until #561. Do not cut a release before it. - **The bundle roughly doubled** (#590): the sites list brings `DataViews` and what it needs. Launch time was not measured. - **Less is always in view.** A build's output, a new line in debug.log and a caught mail are in a tray that may be closed. A failure opens the terminal, or the logs if nothing else is open; unseen debug.log lines are counted on the footer; a mail that arrives shows nowhere until its tray is opened, which was decided and is not an oversight. - **Translation is about half done.** The shell, the cards, the tray and most dialogs go through the translator. Roughly 200 strings in components and hooks do not yet (the review dialog and its destinations, the setup checklist, the logs, most of what the hooks say), nor about 150 in the modules they take sentences from. Strings that are new are in English in the shipped translations until they are translated. - **Focus is lost in a few places**: when the app swaps what the tray holds from under it, when a notice with a focused button goes, when a row's own action removes the row. - **Some failures are still worded by the main process** ("Error invoking remote method…"): a failed rename, a failed setup. - **Things carried over, not made by this:** a start that throws leaves the server's button dead until a restart; erasing typed input in the terminal breaks once the line wraps; a line written to debug.log while it is being attached to can be shown twice. - **#571 and #603 will conflict with trunk after this**, in the mail's journey and helpers and in the dev server's hook. - Each slice's own list has more, and says which of its limits were chosen. ## Related Closes #557. Closes #558. Part of #542, which stays open for #559, #560 and #561. #555 and #556 are closed already. --- <details> <summary>Design decisions and alternatives considered</summary> - **An integration branch, and one landing.** A window drawn half by the old view and half by the new one could not be released, and the slices were too many to hold back one by one behind a flag. Fixes and changes with no visible effect went to trunk first and were merged across, so this diff is the redesign and little else. - **Landing before #559 to #561.** The plan was to land when all seven sub-issues were in the branch. The remaining three do not change what is there: two add to it and one documents it. Holding a finished window on a branch for them only lengthens the time trunk and the branch can drift apart. - **What the prototype lacks is kept and redrawn**, and what it has somewhere else moves to where it puts it. Its labels are adopted. Where a slice departs from it, the slice says so. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **No review of the whole diff was run. The record is the slices': twenty-two pull requests into `redesign/wpds`, each but one reviewed before it was opened by a separate, read-only reviewer context following `.github/instructions/code-review.instructions.md`, most in two passes (the change, then its fix-up), with the counts and the findings in each pull request's own review outcome. The one is #618, two words in a line of prose.** - **Review:** completed, per slice — #590, #594 to #602, #605, #606, #608 to #617 (no review was run for #618); everything marked to fix in a slice was fixed in that slice, and what was left is in its Risks. - **Review:** completed, for leftovers only — a separate agent context, read-only; `redesign/wpds` at `548c020` against trunk; not a review of correctness, which the slices had: a search for dead CSS and code, comments that waited for a slice that has landed, what is left of the old component library and of the system's dialogs, strings not yet translatable, and repository docs that name the branch. Nothing found in the stylesheet, the dependencies or the docs; thirteen comments and one unread field, fixed in #617; the rest is in Risks above. - **CodeRabbit:** not run. Its check reads "Review skipped: automatic reviews are disabled" on this pull request and on the slices, so the slices' pre-PR reviews are the reviews of record, each for the head it names. - **Since review:** nothing was added to the branch outside a reviewed slice, apart from three merges of trunk (`da19ac6`, `3344172`, `23d46e7`). </details> <details> <summary>Implementation notes</summary> - `git diff origin/trunk...origin/redesign/wpds`: 148 files, about 14,800 lines added and 3,000 removed; about 6,300 of the added lines are tests. - The site's view, which was one component of five thousand lines, is spread over the four regions the layout has: the header, the cards, the details and the tray. Decisions are in `src/renderer/*.cjs` with unit tests; components draw them. - New for the suite: journeys per region under `tests/e2e/journeys/`, lint rules of the repository's own (`eslint-rules/`), and Stylelint over the window's stylesheet. - The main process changed in one place for the redesign's sake: the debug.log tail says which lines the file already held (#614). </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The prototype shows the design; the signed build shows the app. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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>
Collaborator
|
Thanks for this, @singhakanshu00. Could you rebase onto For context, #663 fixes the listener leak you reported in #604 (the |
Author
Thanks, @zaerl have rebased my current branch onto the trunk. |
This branch has not been deployed
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
When
window.api.startServer()rejects rather than resolving{ ok: false }- a real main-process exception in theplayground:starthandler, not just a reported failure -startPhpServer'scatchblock resets most of its retry guards but leavesdevServerActiveRef.currentstuck attrue. The "Start dev server" button then renders normally, buttoggleDevServer'sif (devServerActiveRef.current) return;guard makes every subsequent click a silent no-op: no new log line, no status check, nothing. The only visible symptom is the one failure message already in the Logs panel; the dead button looks identical to a working one.What changes
Adds the one missing reset -
devServerActiveRef.current = false;- to thecatchblock insrc/renderer/hooks/use-dev-server.jsx, restoring the exact pre-click state so a retry is a genuine retry rather than a guarded no-op.Deliberately minimal: the two sibling abort paths in this hook (a resolved
{ ok: false }, and an unsolicited exit) both callstopDevServer()for their cleanup, but this fix does not route through it. It isn't established thatstopDevServer()'s broader cleanup - stopping the server process, the debug-log tail, and mail listening - is necessary on a path where no server process may have actually started, so the smaller, explicit reset was chosen instead. See Design decisions below.How to test this
Covered by a journey -
tests/e2e/journeys/dev-server.spec.js, the test named "a start that rejects (rather than answering ok: false) goes back to offering to start, and a second click tries again instead of going dead":Mutation-audited by hand against a freshly built renderer bundle (
npm run build:once- a stale bundle silently hides renderer-source changes, so this was rebuilt immediately before each run): fails on the pre-fix hook, specifically on the assertion that a second click produces a secondplayground:startrequest, and passes on the fix.What must not have happened: the button must not require quitting and reopening the site to recover from a single failed start - before the fix it would, silently, with nothing in the UI indicating the button itself had stopped responding (as opposed to the visible, and correct, failure message already logged).
What this doesn't reach: the journey simulates the rejection through a stubbed
playground:startIPC handler; it doesn't drive an actual spawn or permission failure insidespawnRunner(src/main.js). I checked the real path works too, with a temporary one-shot forcedthrowadded to the live handler and reverted before committing: a genuine handler-level rejection is logged, the button recovers, and a real retry succeeds end to end.Platform: any - nothing in this change is platform-specific.
Risks and limitations
Review outcome: 0
[fix here]· 1[follow-up]- see collapsed section below.Two pre-existing cleanup gaps are deliberately left out of this PR, both found while working on this fix rather than introduced by it:
listenForMail()fires before thetryinstartPhpServerand is never balanced bystopListeningForMail()on the rejection path this PR touches.src/preload.js'sstartServeronly removes itsplayground:log/url/stoppedlisteners insidestoppedHandler, which never fires ifinvoke()rejects - so each failed attempt leaks a listener triple, and a later successful attempt fires the stale ones too (duplicateopenExternalcalls, duplicate log lines). This PR's fix is what makes that leak newly reachable repeatedly: previously, the stale-ref bug capped it at one occurrence per site, because nobody could retry.Both are real but pre-existing, scoped as follow-ups rather than fixed here to keep this change to the one bug it targets.
npm testshows 2 pre-existing failures unrelated to this change -kill-tree.integration.test.cjsandorphan-pipe.integration.test.cjs- confirmed present on bare trunk by stashing this branch's two changed files and re-running both tests directly; both fail identically there.Related
Fixes: #501
Design decisions and alternatives considered
Considered reusing
stopDevServer()in thecatch, matching the two sibling abort paths in this file exactly. Rejected for this PR: it would also callwindow.api.stopServer(sitePath), stop the debug-log tail, and stop mail listening on a path where no server process may have actually started, and that broader cleanup's necessity here hasn't been established. Kept the fix to the single ref reset the bug actually needs; consolidating ontostopDevServer()is a reasonable follow-up if the mail-listener leak above gets fixed at the same time.Review outcome (required - see AGENTS.md)
0
[fix here]· 1[follow-up]522b6ea(working-tree diff, uncommitted at review time, diffed against trunk).npm run lintclean;npm test- 2 pre-existing, trunk-confirmed-unrelated failures (above). Finding:src/preload.js'sstartServerlistener-cleanup gap (medium,[follow-up]) - see Risks above. No findings on security, performance, or cross-platform.npm run lintand the regression journey against a rebuilt bundle; both still green. No re-review needed - reviewed behavior is unchanged.Implementation notes
Electron wraps a thrown
ipcMain.handlelistener's error before it reaches the renderer -error.messageon the invoking side isn't the bare message butError invoking remote method '<channel>': <original>. The journey's expected log text accounts for this; worth knowing if this test's assertion string ever looks wrong at a glance.Screenshots or recording
Not applicable - no new UI surface. The button's existing label states ("Start dev server" / "Starting dev server…" / "Stop dev server") are unchanged; what changes is only whether a click after a failure does anything.
🤖 Generated with Claude Code