Repository navigation
Add the PHP version and the debug flags to the settings, applied at the next server start - #640
Merged
Merged
Conversation
…at the next server start The third of the settings (#559): a Sites tab with what every site's development server runs with. The PHP version is one of those the bundled Playground has, 8.3 unless another is chosen, and WP_DEBUG and SCRIPT_DEBUG can be turned off; the rest of the constants a server is booted with stay as they were and are nobody's to change. Main reads the three at each start and hands them to the runner in its config, so a change applies to the next server and a running one keeps what it started with; a version the bundled Playground no longer has, left by a bump, is passed over for the fallback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…, and put the server settings in the details From the review. WP_DEBUG off was said to empty debug.log and the browser of errors; it takes away notices and deprecations, and warnings, errors and error_log() calls still reach both, since Playground's own PHP keeps logging and display on. The switch, the guide and the constants' module say so. Which versions the control offers, and which it shows as chosen, are decided in the view module: a version a bump has taken from the bundle shows the fallback as chosen, with a note, and the start logs why. The open site's details name the PHP a server starts on beside the checkout and, under Debugging, which of the two constants are on, as the prototype has them. The guide's pictures are retaken: the details changed here, and the footer's cog since #638 had not been taken yet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the details whole in the guide's pictures From the second review. The details named the PHP version set, where the dialog and the start used the fallback for one the bundle no longer has; all three now decide it in one place, and the versions are read once by the hook, which says when they could not be, where the control offered nothing. error_log() writes to the log and not the page, and the four sentences say so. The site views are taken in a window tall enough for the details' Build watch section, which the Debugging row had pushed under the footer. The require of the PHP versions says whose dependency it rides on, and the start handler builds its log scope once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
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 |
… on the runners' screens The facts gained a Debugging row (#559), and with every row the facts alone no longer fit over the tray at its smallest on the runners' screens, which the journey's first half needs them to: it failed there by 36 to 49 pixels and passed on a taller screen. The site it seeds now has no creation date and no trunk date. Run at windows 700, 680 and 640 high, and red at 640 with the app's rule broken. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
zaerl
added a commit
that referenced
this pull request
Oct 6, 2026
…what a quit stopped (#641) ## Why Fourth part of #559, on the dialog of #638. Every site started cold: its development server and its build watch waited for a press, every time the site was opened and every time the app was. A contributor who works on one site all afternoon, and a mentor who opens the app to find the sites they left running, each had a press to make first. ## What changes **Opening and quitting, on the General tab.** - **Start the server when I open a site** and **Start the build watch when I open a site**, both off unless turned on. On, opening a site that is set up starts its server, its watch or both, as a press of Start would; on WordPress Core the server's start brings the watch with it. Nothing starts for a site whose setup, update or deletion is under way, and nothing already running or starting is started again. - **When I quit, running servers and build watches**: **Stop them**, which is what the app has always done, or **Stop them, and start them again next time**. Under the second, main writes down at quit which sites had a server or their project's watch running, and the next launch starts them again, whichever site it opens on, then forgets the list. The prototype's third answer, leaving them running past the quit, is deliberately not offered: the quit sweep exists to end every child the app started (#83). **How a site decides.** Opening a site is an edge, and so is the list a quit left: each is consumed once, when the site is ready for it, in a pure module (`src/renderer/auto-start.cjs`) the row asks. The row keeps the gates and the calls: the server's start first, awaited, then the watch where that start did not bring it up. A watch the server's start was refused, because the terminal is held, is not asked for twice. **How the quit remembers.** `before-quit` runs synchronously and is not awaited, so the write goes through the store's synchronous accessor, which the first read at startup has made; a store that cannot be written is logged and does not stop the sweep. A site with both its server and its watch running is listed under both. **Not in this pull request:** - Leaving servers running past the quit. See above. - Starting a site that is not set up: the setup checklist is what starts things the first time. - The theme (#560), the last of #559's rows. ## How to test this Platforms: any. The journeys drive this on macOS and Windows. From the repository root, after `npm ci` and `npm run build:once`: ``` npx playwright test --project=journeys auto-start.spec settings.spec i18n.spec site-header.spec ``` - `auto-start.spec.js`: with the server set to start, the site the window opens on asks for its server once, and on Core the watch with it; opening another site asks for that site's; coming back to one whose server runs asks for nothing and does not stop it. With the watch set to start, the watch is asked for and no server; turned off, a relaunch starts nothing. With a list a quit left, the served site's server and the watched site's watch are asked for, whichever site the window opens on, the served site's watch once; the list is forgotten as it is read; a relaunch with nothing left starts nothing, and a list left under 'restart' is not followed under 'stop'. The quit setting is chosen and kept. - `settings.spec.js`, `i18n.spec.js`: the dialog, in English and in the pseudo-locale. - `site-header.spec.js`: the details' stickiness journey now sizes its window itself. Nothing is run: the handlers that start the server and a script are stood in, and the journeys read what they were asked. The quit's own write is `ipc-wiring.test.cjs`'s subject: under 'restart' it writes the sites with a server and those running their project's watch, under 'stop' nothing, and a store that cannot be written is logged and the sweep still reaches every child. I broke the open-site gate (every row started) and the quit's write, and each was caught. **Worth a look by hand** (any platform, the current head, a site that is set up): Settings → General, turn on **Start the server when I open a site**, close, open another site and come back: the server starts, and the Logs tray shows its lines. Set **When I quit** to **Stop them, and start them again next time**, leave the server running, quit, open the app: the server starts again on that site, whichever site the window opens on. Quit again with it running and set the quit back to **Stop them** before you open the app: nothing starts. **What must not have happened:** a server started for a site whose setup or update is under way; a server stopped by the setting (it only starts); a second watch started on a site whose server's start started one; a server or watch running past the quit; the list followed twice after a relaunch that was not a quit. **What I could not test:** the real quit path end to end, since the journeys stand in for the server; the wiring test drives the handler with recorded children. ## Risks and limitations - Review: 2 passes, 12 findings fixed here, none left. Details below. - **A start asked for cannot be taken back:** a site deactivated, or an update pressed, in the moment between the plan and the server's first answer still gets its start. The window is one request to main. - **Two servers starting at once** (on a relaunch with two sites listed) can both find Playground's first port free and one fall back to another port; pressing Start on two sites quickly has the same race today. - **On Gutenberg a relaunch with both listed starts the watch**, which removes `build/` under the just-started server for its rebuild, as both settings on do. - The stickiness journey's second half held only on screens that leave the tray at its largest too little room; on a tall screen it did not, so the journey sizes its window to the smallest the suite runs on. ## Related Part of #559, which is part of #542. Follows #640. The quit sweep: #83. --- <details> <summary>Design decisions and alternatives considered</summary> - **Edges, consumed once, in a pure module.** The first version decided inline in the component, which the suite cannot load, and predicted whether the server's start would bring the watch from a copy of "built" that the start re-reads from disk. Now the server's start is awaited and the watch's state read after it. - **The quit writes through a synchronous accessor.** `before-quit` is not awaited by Electron; the store is an ESM import made at startup for the locale, so it is there by the quit, and the sweep must not wait on it. - **Two answers to the quit question, not three.** Leaving children past the quit undoes #83. - **The list is read once and forgotten**, and only while the setting still says 'restart': a launch that ends badly does not start everything twice, and a list left under 'restart' is not followed under 'stop'. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 5 [fix here] · 2 [follow-up]. Pass 2 (on the fix-up): 3 [fix here] · 2 [follow-up]. All 12 applied here.** - **Review:** completed — a separate agent context, read-only; head `e5a0ba4` / base `c34aa45`; the quit path read against the store rule and the sweep, the effect's edges and gates, Playground's port choice for two servers at once, the journeys against the standard; ESLint, the unit files and the pot extraction run, Playwright not; 5 [fix here], 2 [follow-up]. - Fixed: an error writing the list at quit would have skipped the sweep; a site with both its server and its watch was remembered under the servers only, and on Gutenberg with a build the watch was not started again; the start decision lived in the component, with a prediction from a stale copy of "built"; a #557 comment sat over the wrong code; the journeys slept. - Follow-ups, taken here: the stickiness journey's comment and its wait for the resize; a dead check in the effect. - **Review:** completed — a second separate agent context, read-only; head `c75017f` / base `c34aa45`; the awaited server start traced through the hook's two paths for whether the watch's state is set before it resolves, the throwing-store test checked to be red on the first commit; ESLint and the unit files run; 3 [fix here], 2 [follow-up]. - Fixed: a round trip to main was not a sound sign that nothing was started, since a start asks main only after awaits of its own, so the journeys now read the controls after the settings are in the page and the effects have run; the resume journey did not seed the list a quit now writes; the effect's catch was silent. - Follow-ups, taken here: a watch the server's start was refused was asked for a second time; the throwing-store test did not read the log. - **Since review:** `c75017f → 5117052` is those five, not reviewed again. On the head: `npm run lint`, `npm test` (2018), `npm run test:electron` (2019), all 123 journeys, the packaged smoke and the docs build pass. </details> <details> <summary>Implementation notes</summary> - `src/settings.cjs`: `autoStartServer`, `autoStartWatch`, `quitBehavior`. `src/resume-sites.cjs`: `sitesToResume`, `readResume`. `src/settings-store.js`: `peekStore`. `src/main.js`: `scriptByRunId`, `rememberRunningSites` in `before-quit`, `sites:resume`. `src/preload.js`: `takeResumeList`. - `src/renderer/auto-start.cjs`: `autoStartPlan`. `src/renderer/settings-view.cjs`: `quitItems`, `resumeFor`. `src/renderer/index.jsx`: the row's effect; App reads the list once. `src/renderer/components/settings-dialog.jsx`: `OpeningAndQuitting`. - Tests: `tests/unit/auto-start.test.cjs`, `resume-sites.test.cjs`, `settings.test.cjs`, `settings-view.test.cjs`, `ipc-wiring.test.cjs`; `tests/e2e/journeys/auto-start.spec.js`. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The journeys drive everything that changed, and the guide page says what each control does. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <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
1 participant
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
Third part of #559, on the dialog of #638. Every site's development server ran on the PHP the bundled Playground chooses and with WordPress's debug constants fixed on; a contributor whose ticket is about PHP 7.4, or about what a site does with
WP_DEBUGoff, had no way to see it. This adds the PHP version and the two debug constants that can be turned off to the settings, and shows what a server will start with in the open site's details.What changes
A Sites tab: Development server. The PHP version, one of the seven the bundled Playground has (7.4 to 8.5), 8.3 unless another is chosen; and two switches, Report notices and deprecations (WP_DEBUG) and Use unminified scripts (SCRIPT_DEBUG). The rest of the constants a server is booted with (the log, the display, the disabled fatal handler and updater) stay as they were and are not anyone's to change.
Applied at the next start. Main reads the three at every server start and hands them to the runner in its config; a server that is running keeps what it started with until it is stopped and started, and the tab says so. A version a release has taken out of the bundle is passed over for 8.3, with a line in the app log, and the control shows 8.3 as chosen with a note saying why.
The details say what a server will start with, as the prototype has them: "WordPress Core · PHP 8.3" on the Checkout row, and a Debugging row with the constants that are on, or "Off".
What WP_DEBUG off does, said for what it is. It takes away notices and deprecations. Warnings and errors still reach
debug.logand the browser, anderror_log()calls still reachdebug.log, because Playground's own PHP keeps logging and display on whatever WP_DEBUG says. The switch, the guide and the constants' module say so.The guide's pictures are retaken. The details changed here, and the footer's cog from #638 had never been retaken. The three pictures of a site's page are taken in a window 900 high, so the details' Build watch section is whole.
Not in this pull request:
siteMetaif one is ever wanted.WP_DEBUG_LOG,WP_DEBUG_DISPLAY,WP_DISABLE_FATAL_ERROR_HANDLER,AUTOMATIC_UPDATER_DISABLED), on purpose:src/wp-debug-constants.jssays why each is on.How to test this
Platforms: any for the dialog and the details; macOS for the real server, below. The journeys drive the dialog and the details on macOS and Windows. From the repository root, after
npm ciandnpm run build:once:settings.spec.js, the Sites journey: the fallbacks (8.3, both constants on); 8.4 chosen is kept, WP_DEBUG off is kept and SCRIPT_DEBUG left as it was; the open site's details then say "WordPress Core · PHP 8.4" and "SCRIPT_DEBUG"; restarted, the tab shows what was kept.site-header.spec.js: the details name the project with the PHP a server starts on and which constants are on.i18n.spec.js: the tab in the pseudo-locale.I broke two claims one at a time (main not reading the settings at a start; the switch not writing) and each was caught, the first by
ipc-wiring.test.cjs, the second by the journey.tests/unit/runner-wiring.test.cjsproves the runner hands Playground the version and turns the two constants off in the blueprint;wp-debug-constants.test.cjsthat nothing else in the set moves;settings.test.cjsandsettings-view.test.cjswhat is accepted and what the control shows.The real server, run by me on this Mac (Apple Silicon, the current head), as TESTING.md asks for a change to
src/server-runner.js. The app was driven through Playwright with a throwaway profile listing a builtwordpress-developcheckout; Start development server was pressed; the served WordPress was logged in to with the app's own credentials and asked, on Site Health's Info tab, which PHP it runs on:Worth a look by hand (any platform, the current head): Settings → Sites, choose 7.4, close: the details say "PHP 7.4". Start the server: wp-admin → Tools → Site Health → Info → Server says PHP 7.4. Turn WP_DEBUG off, stop and start the server: a page with a deprecated call shows nothing for it, and a page that calls a missing function still shows the fatal.
What must not have happened: a running server changing PHP or constants under the contributor; a change applied to a site other than the next start;
WP_DEBUG_LOGor the fatal handler's constant moving with the switches; the details saying a version the server will not start on.What I could not test: Windows, and a real server with WP_DEBUG off (I checked what the code sends, not a page's output).
Risks and limitations
@php-wasm/universalfor the list of versions. It is not a declared dependency; it is@wp-playground/cli's, at whatever version that pins, and the comment says so. Declaring it pinned is apackage.jsondecision I have left to the maintainer.WP_DEBUGswitch that is off still leavesWP_DEBUG_LOGon, by design, so the debug.log tab keeps showing warnings and errors.Related
Part of #559, which is part of #542. Follows #639.
Design decisions and alternatives considered
SupportedPHPVersionsfrom@php-wasm/universalis what the runner can serve; a list of our own would drift.phpVersionChoiceinsettings-view.cjs: the dialog, the details and the start handler all agree on the fallback.Review outcome (required — see AGENTS.md)
Pass 1: 3 [fix here] · 3 [follow-up]. Pass 2 (on the fix-up and the details): 5 [fix here] · 1 [follow-up]. All 8 fixed, the real-boot follow-up taken here, 2 left.
9131ed7/ base77751a1; the runner's options checked against the CLI'srunCLIsignature and its persistentstartcommand, WordPress'swp_debug_mode()and Playground's php.ini and mu-plugin read for what WP_DEBUG off does, the design system's switch and toggle props confirmed; ESLint and the unit files run; 3 [fix here], 3 [follow-up].ca84255/ base77751a1; the four rewordings checked against WordPress and Playground again, the details' new rows and their journeys, the retaken pictures opened against their alt texts; ESLint and the unit files run; 5 [fix here], 1 [follow-up].error_log()was said to reach the browser; three pictures cut the details' Build watch section off under the footer; the require had no comment on whose dependency it rides; a failed read of the versions left an enabled, empty control.ca84255 → ba66103is those five fixes and two style notes, not reviewed again. The commit after it changes one journey:site-header.spec.js's stickiness test failed on both runners, since the Debugging row left the details too tall to fit over the tray at its smallest on their screens; its seeded site now has no creation date and no trunk date, two rows fewer, and the journey was run at windows 700, 680 and 640 high and is red at 640 with the app's rule broken. On the head:npm run lint,npm test(2002),npm run test:electron(2003), all 119 journeys, the packaged smoke and the docs build pass. The twelve pictures were taken four times before the last change and two of them moved by 12 and 30 pixels on a corner between takes, the known noise; the three retaken after it were taken once.Implementation notes
src/settings.cjs:phpVersion,wpDebug,scriptDebug,acceptSwitch.src/wp-debug-constants.js:debugConstants(debug).src/server-runner.js:phpand the constants from the config.src/main.js:phpVersions(),playground:php-versions,playground:startreads the store at each start.src/preload.js:listPhpVersions.src/renderer/settings-view.cjs:phpVersionChoice.src/renderer/hooks/use-settings.jsx:php.src/renderer/components/settings-dialog.jsx:SitesTab.src/renderer/site-details.cjs:phpVersionanddebugin the rows.scripts/screenshots/shots.cjs: the three site views at 1200×900.Screenshots or recording
The retaken images are in the diff:
docs/public/screenshots/shows the details before and after.🤖 Generated with Claude Code