Repository navigation
[Fix] Name each project's own source directory in the hint under the terminal - #491
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
Collaborator
Author
|
@coderabbitai review |
|
…terminal (#490) The "Edited files in src/?" hint under the terminal was Core's on a Gutenberg site too, which has no src/. The directory now comes from the registry's cards block (src/ for Core, packages/ for Gutenberg), where the checklist's equivalent sentence already lives. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…the app shows it Self-review findings on 8b9ef45. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
juanmaguitar
force-pushed
the
fix/gutenberg-terminal-hint-packages
branch
from
September 18, 2026 12:43
1b8b4a1 to
70d9b7d
Compare
This was referenced Sep 19, 2026
juanmaguitar
added a commit
that referenced
this pull request
Sep 19, 2026
…watch (#500) ## Why On a Gutenberg site, **Start dev server** started the build watch first and waited for it to be ready before starting Playground (#489). Gutenberg's watch is `npm run dev`, which cannot skip its initial build: it removes `build/`, builds the workspaces, runs `tsc --build` and then `wp-build --watch`, which builds again before printing *Watching for changes*. The only flag it has is `--skip-types`. So every server start on a built site redid the build the wizard had just finished: about 20 s on macOS, several minutes on Windows plus four console windows (#497). A contributor who had just watched step 3 complete saw "Dev server is starting…" count for minutes before the URL appeared (#499). ## What changes **The rule.** A new pure helper beside the plan, `serveWithoutWatch`, says whether the server may start on the `build/` the site has. Yes only when the watch would first destroy `build/` (Gutenberg's, the registry's `readyPattern` is how it says so), the site is built, and nothing has been tearing `build/` down: no watch is up, and the last watch did not stop or crash while rebuilding. Core's watch is free (no pattern, touches nothing on start), so it keeps starting with the server. When the helper says no, `toggleDevServer` takes the path it took before: watch first, server off its readiness. **"Built" is not trusted blindly.** The status's "built" is one marker file, and a watch rebuild cut short can leave that file with the rest of `build/` missing. The renderer keeps a `buildInterrupted` flag (set when a watch is stopped or exits while `building`, cleared when a watch reaches `watching` or a one-shot `npm run build` exits 0), and `toggleDevServer` reads the status afresh before deciding instead of using the copy from the last poll. **What the contributor sees.** On a built Gutenberg site the URL appears in seconds. The **Start build watch** button and its tab are unchanged; the watch starts when they click it, or when an apply needs a build (applying a pull request or a patch, and updating trunk, already run a one-shot `npm run build` when no watch is running, and pause and resume one that is, #493). A watch that is already watching or building when the server is started is used as before: the server hangs off its readiness. **Deliberately not in it.** Core keeps starting its watch with the server: it costs nothing and `src/` edits compile on save from the first click. A Gutenberg site that has never been built (skip-the-wizard) keeps the current order: one-shot build, watch, server after *Watching for changes*. Also: the watch button's tooltip said `src/` on every target; it now names the project's source directory like the terminal hint does (#491). ## How to test this Platforms: either; Windows is where the difference is visible. Build: the Buildkite artifact for the current head, or `npm start` from the repository root. **Starting state:** a Gutenberg site whose wizard is at step 4, or an initialized Gutenberg site with a completed build and nothing running. 1. Click **Start dev server** (or **Start dev server and finish the wizard**). Expected: the URL and **wp-admin** appear within seconds, the **Build watcher** tab title stays `Build watcher`, the watch button reads **Start build watch**, and the Server tab says the server started without the watch. On Windows: no console windows open. 2. Open wp-admin. Expected: the dashboard, no *requires files to be built* notice. 3. Click **Start build watch**. Expected: tab `(building)`, then `(watching)` after the rebuild; while it rebuilds the site shows the notice, as documented (risk of #489). 4. Click **Stop dev server**. Expected: the watch keeps running (#247). Click **Start dev server** again. Expected: the server starts on the running watch, no second watch. 5. Stop both. Apply a pull request by number with no watch running. Expected: the apply chain runs its own `npm run build` and the banner says so. 6. On a Core site: **Start dev server**. Expected: the watch starts with the server, as before (tab `(watching)`). **What must not have happened:** a Gutenberg server started while `build/` is missing or half-written; a second watch spawned when one was already running; Core's watch no longer starting with the server; the wizard's step 4 button behaving differently from the action bar's. 7. Start build watch, and while the tab reads `(building)` click **Stop build watch**, then **Start dev server**. Expected: the watch starts first and the server waits for *Watching for changes*, as on trunk; no half-built site is served. Tests: `tests/unit/dev-server-command.test.cjs`, five tests on `serveWithoutWatch` (idle and paused serve at once; unbuilt, watching, building, exited and interrupted do not; Core never), the first fails on trunk (the helper does not exist) and passes here. `tests/e2e/real-setup/create-site.spec.js` gets a step for Gutenberg: after the URL, the watch is not running, then it is started by hand so the stop and the process-tree check still exercise it. Lint clean, 1591 unit tests and 33 journeys pass locally. ## Risks and limitations - An edit made by hand under `packages/` is not compiled until the contributor starts the watch. The terminal hint (*Edited files in packages/? Run npm run build*) and the guide say so. - Starting the watch by hand while the server is up reopens the *requires files to be built* window during its rebuild. Unchanged from #489, now the normal way to get a watch on Gutenberg, and documented. - `buildInterrupted` lives in the renderer and does not survive an app restart; after one, a `build/` cut short reads as built until the next successful build, as on trunk. - The real-setup e2e (40 min per target) was not run for this PR; the journeys were. ## Related Fixes #499. Follows #489 and #493. Windows console windows during the build: #497. Part of the v1.2.0-beta.1 pass. --- <details> <summary>Design decisions and alternatives considered</summary> - **Skip the initial build inside `npm run dev`.** Not possible from outside: `tools/build-scripts/dev.mjs` only parses `--skip-types`. An upstream flag would still leave `wp-build --watch`'s own initial build. - **Run `wp-build --watch` directly instead of `npm run dev`.** Skips the clean and `tsc` but not wp-build's initial build, and loses the type watch. Rejected. - **Never start the watch with the server, on every target.** Uniform, but it changes Core, whose watch is instant, for no gain; and the guide's promise that Core edits compile on save from the first click would go. The rule is keyed on the watch having a ready pattern, which is the registry's own way of saying "this watch destroys build/ first". - **Where the rule lives.** In the pure plan helper, beside `needsBuild` and `readyPattern`, so it is unit-tested and the renderer only reads it. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 2 [fix here] · 2 [follow-up] on `7cbd399` — both fix-here fixed in `f53e0a4`. - Fixed: the fast path trusted a `hasBuilt` as old as the last status poll and one marker file, so a watch stopped or crashed while rebuilding could be served as complete (now: fresh status read, `buildInterrupted` flag, `exited` sends the watch first). - Fixed: the composed rule lived inline in `index.jsx` with no test; now `serveWithoutWatch` in `dev-server-command.cjs`, five unit tests. - Fixed (documentation, outside the five dimensions): `docs/guide/editors.md` still said starting the server starts the watch. - Deferred: `startPhpServer`'s reject path never clears `devServerActiveRef`, leaving the button dead for the session; pre-existing, only reachable when the IPC itself rejects, not made more reachable here. Follow-up issue to open. - Fixed in `9e683b1` after CodeRabbit raised it too: `editors.md` said "WordPress source lives under `src/`" on every target. - Rule for the instructions file: documentation accuracy against the code is not one of the five dimensions; the reviewer suggests adding it, given how much of the guide narrates renderer behaviour. - **Review:** completed — `/self-review` (Explore subagent, judgement pass; lint, unit and journeys inline); reviewed head `7cbd399` / base `4c53900` (trunk); evidence: findings above; outcome: 2 fixed, 2 deferred. - **Since review:** `7cbd399 → f53e0a4` checked: the fixes above plus their tests; lint clean, 1593 unit tests and the 33 journeys pass on the new head; no re-review of the fixes by a second pass. - **Review:** completed — CodeRabbit on `f53e0a4`; 2 findings, both documentation (`editors.md` said source lives under `src/` on every target, one of the deferrals above; `running-the-site.md` did not mention the cut-short rebuild exception); both fixed in `9e683b1`, docs only, no code change. - **Manual pass:** Windows 11 ARM (UTM VM), Buildkite artifact of `f53e0a4`, on the built Gutenberg site from the beta.1 pass: Start dev server gave the URL in seconds, no console windows, Build watcher tab unchanged; Start build watch by hand then ran the rebuild. Confirmed by JuanMa, 19 Sep 2026. </details> <details> <summary>Screenshots or recording</summary> No new surface: the buttons, tabs and dots are the ones from #489 and #493; what changes is the timing (URL in seconds) and one line in the Server log. Windows checked on the VM with the artifact of `f53e0a4`, see the review outcome. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Sep 19, 2026
## Why While the setup wizard is still installing (banner *step 2 of 3, Installing dependencies*, `npm ci` streaming in the terminal), the **Install npm dependencies** step flipped to **COMPLETED** and **Run full build** to **READY** with its button enabled. A click there starts a build over a half-written `node_modules`. Seen on Windows during the v1.2.0-beta.1 pass on a Gutenberg site, whose install is long enough to switch windows during; the rule is shared by every site (#495). ## What changes **Root cause.** `computeSetupStepState` took the step as done when `node_modules` exists and is non-empty and the last install did not fail. It ignored `installing`. Any status refresh mid-install (switching back to the window triggers one) reads the partial directory as done. **The fix.** An install still running overrides existence, exactly as a failed one already did (#42): `installOk` also requires `!installing`. One line. When `npm ci` exits 0 the flag clears, the next status read sees the full directory, and the step completes as before. ## How to test this Platforms: either. Build: the Buildkite artifact for the current head, or `npm start` from the repository root. **Starting state:** no site yet, or a site whose wizard is at the download step. 1. Create a Gutenberg site (its install runs for minutes) and wait until the banner reads *step 2 of 3, Installing dependencies*. 2. Switch to another application and back, two or three times, while the terminal still shows `npm http fetch` lines. Expected: **Install npm dependencies** keeps its in-progress state and **Run full build** stays locked. 3. Let the install finish. Expected: the step turns **COMPLETED** and the build starts on its own (the wizard chains it). 4. On an existing, fully installed site, open it. Expected: the step reads **COMPLETED** as before; nothing regresses for a site that is not installing. **What must not have happened:** **Run full build** enabled while `npm ci` is running; a completed site's install step reading as not done. Test: `tests/unit/setup-steps.test.cjs`, *a half-written node_modules during an install does not complete the step*. Fails on trunk, checked in a trunk worktree. ## Risks and limitations None beyond the one-line rule. The `installing` flag is set by the renderer for the wizard's install and for a manual `npm install` from the terminal alike, so both paths get the same hold. ## Related Fixes #495. Part of the v1.2.0-beta.1 pass, follows #489, #491, #493. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> - **Review:** completed in-session on head 44570f4 / base 4c53900. Deterministic layer: eslint clean, `npm test` 1589 pass, 33 journeys. Judgement: the change is one predicate in a pure module with a test that fails on trunk; the only question checked was whether `installing` is set on every install path (it is, for the wizard's chain and for a terminal `npm install`), so the hold applies to both; no security, performance or platform surface. - **Since review:** `44570f4 → 2b6634c` checked: rebase onto trunk `5167e88` (#500) only, no conflict, same diff; eslint clean, `npm test` 1594 pass on the new head. Reproduced again on 19 Sep 2026 on Windows with the trunk artifact, see #495. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Sep 20, 2026
## Why `v1.1.0` shipped on 16 September. Sixteen commits since then reach trunk, and this beta is the first build that carries the Gutenberg contribution target (#251): a site can now be a checkout of `WordPress/gutenberg`, served through Playground as a plugin in a stock WordPress, with a GitHub issue as its work item, pull request checkout from that issue, and a pull request opened against `WordPress/gutenberg` from the app. The manual pass of that work ran on macOS and on Windows over 18 to 20 September and found six issues; the four that affected the first-run path are fixed on trunk (#489, #493, #496, #500, #504, and #491 for the hint), the rest are on the `v1.2.0-beta.2` milestone. `electron-builder` derives artifact names from `package.json`, so the version has to move before the tag and the artifacts exist. A beta is a pre-release, so `releases/latest` stays on `v1.1.0`. ## What changes Only the version. All three package-version fields in `package.json` and `package-lock.json` move together from `1.1.0` to `1.2.0-beta.1`. No dependency versions change, and there is no application behaviour change in this PR. ## How to test this Platforms: any for the suite; macOS and Windows for the artifact checks that follow the merge. 1. `npm run lint` is clean and `npm test` passes. 2. `node -e "console.log(require('./package.json').version)"` prints `1.2.0-beta.1`. 3. `python3 -c "import json;d=json.load(open('package-lock.json'));print(d['version'], d['packages']['']['version'])"` prints `1.2.0-beta.1 1.2.0-beta.1`. **What must not have happened:** no dependency version may change. `git diff` must show exactly three changed lines, all of them the root package version. After merge, the release build must produce artifacts named `wordpress-contributor-toolkit-1.2.0-beta.1-*`; the asar root must be exactly `node_modules`, `package.json`, `src`, with npm 11 under `node_modules/npm` and no `.codesigning` directory; on macOS `CFBundleShortVersionString` reads `1.2.0-beta.1`. ## Risks and limitations No UI change. The risk is a mis-scoped edit in `package-lock.json`, where dependencies also carry version fields; the replacement was confined to the first 2000 bytes of the file, which hold the two root entries and nothing else, and both files were re-parsed as JSON afterwards. Verified locally on this branch: lint clean, `npm test` 1596 pass / 0 fail. ## Related Part of #251. Release notes follow on the pre-release; docs publish for the tag per #487. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> 0 [fix here] · 0 [follow-up]. Deterministic layer only: lint clean, 1596 tests pass, both files valid JSON, diff confined to the three root version fields. No judgement pass was run: this change has no logic to review. - **Review:** completed — deterministic layer, in session; head is the single bump commit / base `b5ca272` (trunk); evidence above; outcome: nothing to fix. - **Since review:** none. </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
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
Under the Terminal on a Gutenberg site the first hint reads Edited files in
src/? Runnpm run build. A Gutenberg checkout has nosrc/; its code is underpackages/. The line predates the Gutenberg target and was never made per-project, unlike the checklist's equivalent sentence. Found during the v1.2.0-beta.1 manual pass (#490).What changes
The directory moves into the registry's
cardsblock,sourceDir:src/for Core,packages/for Gutenberg. The hint reads it. The invariant test that walks every project type checks the field is a directory name, and a values test pins the two. The terminal guide quotes the hint as the app shows it and notes that the build watch does the first one for you while it runs.Nothing else in the hint changes:
npm run buildis on both targets' allowed-script lists, and the second hint (Added a dependency topackage.json? Runnpm install) is true of both.How to test this
Platforms: either. Build: the Buildkite artifact for the current head, or
npm startfrom the repository root.Starting state: one Gutenberg site and one Core site, both past the setup wizard (the hints appear only once a site is built).
packages/? Run npm run build so the site picks them up.npm run buildlink in that line. Expected: the command is typed into the terminal, not run.src/, as before.What must not have happened: a site with no project type on record (one created before v1.2.0) showing anything but
src/; the second hint changing on either site.Test:
tests/unit/project-type.test.cjs. On trunk the invariant walk fails on every entry (sourceDirundefined), checked in a trunk worktree.Risks and limitations
None beyond wording. The hint still suggests a manual
npm run buildon a Gutenberg site whose watcher would rebuild on save; the guide now says the watch covers it, the hint itself stays one line.Related
Fixes #490. Part of #251. Found alongside #488 (#489).
Review outcome (required — see AGENTS.md)
1 [fix here] · 0 [follow-up] · 2 [note] — fixed, one note taken.
npm test1548 pass. Finding: the new test landed between the built-marker comment and its test, so the comment headed the wrong test → moved below. Notes: the guide bullet quoted the hint with a parenthetical the app never shows → reworded to quote it as shown, watch sentence moved out of the quote; thesourceDirregex forbids a nested directory, fine today, left. Verified clean:projectalways resolves throughgetProjectType, so the field is never undefined in the renderer;npm run buildis on both allowed-script lists; the second hint is true on both (rootnpm installcovers a workspace package's dependency); no othersrc/copy left.8b9ef45 → 1b8b4a1, the findings above only; eslint and the registry suite re-run.Screenshots or recording
Before, on a Gutenberg site (from #490): Edited files in
src/?. After: Edited files inpackages/?. One word changes; no layout change.🤖 Generated with Claude Code