diff --git a/.agents/skills/self-review/SKILL.md b/.agents/skills/self-review/SKILL.md index 396b85c..7d526d8 100644 --- a/.agents/skills/self-review/SKILL.md +++ b/.agents/skills/self-review/SKILL.md @@ -7,39 +7,22 @@ description: Review the current branch against this repo's architecture, securit Run the repo's review standard against the working branch, **before** the PR exists. -Nothing runs this in CI. It is the only review a change gets before a human reads it, which is why -it happens now rather than after: a finding fixed here costs one message, the same finding on the PR -costs a review cycle. +Nothing runs this in CI. It is the only review a change gets before a human reads it, which is why it happens now rather than after: a finding fixed here costs one message, the same finding on the PR costs a review cycle. ## Procedure -**Read [`.github/instructions/code-review.instructions.md`](../../../.github/instructions/code-review.instructions.md) -and follow it.** It is the specification for this task, not background reading — it carries the -procedure (establish the diff, run `npm run lint` and `npm test`, then the five dimensions) as well -as the invariants and the reporting bar. Do not restate it here; run it. +**Read [`.github/instructions/code-review.instructions.md`](../../../.github/instructions/code-review.instructions.md) and follow it.** It is the specification for this task, not background reading — it carries the procedure (establish the diff, run `npm run lint` and `npm test`, then the five dimensions) as well as the invariants and the reporting bar. Do not restate it here; run it. -That file is the single copy of the standard. It lives under `.github/instructions/` so Copilot code -review picks it up natively; every other agent, this one included, reaches it from here. Changes to -how reviews work go there, not into this wrapper. +That file is the single copy of the standard. It lives under `.github/instructions/` so Copilot code review picks it up natively; every other agent, this one included, reaches it from here. Changes to how reviews work go there, not into this wrapper. Two things this skill adds on top, both agent-neutral — apply whichever your agent can: -**Run the judgement pass in a fresh context.** The diff and the deterministic layer -(`npm run lint`, `npm test`) run inline. The five dimensions should be reviewed by a context that -did not write the change — a subagent, or a separate pass given only the diff and the instructions -file. If the session that wrote the code also grades it, it reviews its own reasoning and finds it -sound. +**Run the judgement pass in a fresh context.** The diff and the deterministic layer (`npm run lint`, `npm test`) run inline. The five dimensions should be reviewed by a context that did not write the change — a subagent, or a separate pass given only the diff and the instructions file. If the session that wrote the code also grades it, it reviews its own reasoning and finds it sound. -**Report, don't apply.** Surface findings in the chat. No GitHub comments, no files written — the PR -does not exist yet. Then offer; the author decides what is a real finding. +**Report, don't apply.** Surface findings in the chat. No GitHub comments, no files written — the PR does not exist yet. Then offer; the author decides what is a real finding. ## Notes -- This is the same skill as [`.claude/skills/self-review/`](../../../.claude/skills/self-review/), - placed here because agents built on the Agent Skills open standard (Command Code and others) - discover skills under `.agents/skills/` rather than `.claude/skills/`. Both are thin wrappers over - the one instructions file — neither is a second copy of the standard. -- The repo-specific knowledge this adds over a generic review: Electron's bundled Node, - `isomorphic-git`, `electron-store`, loopback binding, the Windows spawn shims. -- If a finding reveals a rule the instructions file does not yet cover, say so. It is meant to - accumulate what the project learns. +- This is the same skill as [`.claude/skills/self-review/`](../../../.claude/skills/self-review/), placed here because agents built on the Agent Skills open standard (Command Code and others) discover skills under `.agents/skills/` rather than `.claude/skills/`. Both are thin wrappers over the one instructions file — neither is a second copy of the standard. +- The repo-specific knowledge this adds over a generic review: Electron's bundled Node, `isomorphic-git`, `electron-store`, loopback binding, the Windows spawn shims. +- If a finding reveals a rule the instructions file does not yet cover, say so. It is meant to accumulate what the project learns. diff --git a/.buildkite/commands/setup_macos_code_signing.sh b/.buildkite/commands/setup_macos_code_signing.sh index 542f34a..34d42b1 100755 --- a/.buildkite/commands/setup_macos_code_signing.sh +++ b/.buildkite/commands/setup_macos_code_signing.sh @@ -11,8 +11,23 @@ bundle exec fastlane setup_code_signing echo "Expose signing config to electron-builder..." # Export necessary env vars for `electron-builder` to find those for its `notarize` option # See https://www.electron.build/mac#notarize -mkdir -p .codesigning/ -echo "$APP_STORE_CONNECT_API_KEY_KEY" >.codesigning/apple_api_key -export APPLE_API_KEY=".codesigning/apple_api_key" +# +# The Buildkite step sources this script, so the shebang's `-eu` never applies. Return explicitly +# so callers without `errexit` still receive materialization failures instead of continuing. +MACOS_NOTARIZATION_TEMP_DIR="$(mktemp -d "${TMPDIR:-/tmp}/wordpress-contributor-toolkit-signing.XXXXXX")" || return 1 +export APPLE_API_KEY="$MACOS_NOTARIZATION_TEMP_DIR/apple_api_key" + +cleanup_macos_notarization_key() { + rm -rf "$MACOS_NOTARIZATION_TEMP_DIR" +} +trap cleanup_macos_notarization_key EXIT + +# `printenv` keeps the key itself out of shell traces, while the private temporary directory and +# restrictive file mode keep it unavailable to other users on the build agent. Unlike `echo` under +# `set -u`, `printenv` exits quietly when the variable is missing — hence the explicit guard. +( umask 077; printenv APP_STORE_CONNECT_API_KEY_KEY >"$APPLE_API_KEY" ) || { + echo "APP_STORE_CONNECT_API_KEY_KEY is unset or could not be written to $APPLE_API_KEY" >&2 + return 1 +} export APPLE_API_KEY_ID="$APP_STORE_CONNECT_API_KEY_KEY_ID" export APPLE_API_ISSUER="$APP_STORE_CONNECT_API_KEY_ISSUER_ID" diff --git a/.claude/skills/self-review/SKILL.md b/.claude/skills/self-review/SKILL.md index 2c003a6..f00a443 100644 --- a/.claude/skills/self-review/SKILL.md +++ b/.claude/skills/self-review/SKILL.md @@ -7,36 +7,21 @@ description: Review the current branch against this repo's architecture, securit Run the repo's review standard against the working branch, **before** the PR exists. -Nothing runs this in CI. It is the only automated pass a change gets before a human reads it, which -is why it happens now rather than after: a finding fixed here costs one message, the same finding on -the PR costs a review cycle. +Nothing runs this in CI. It is the only automated pass a change gets before a human reads it, which is why it happens now rather than after: a finding fixed here costs one message, the same finding on the PR costs a review cycle. ## Procedure -**Read `.github/instructions/code-review.instructions.md` and follow it.** It is the specification -for this task, not background reading — it carries the procedure (establish the diff, run -`npm run lint` and `npm test`, then the five dimensions) as well as the invariants and the -reporting bar. Do not restate it here; run it. +**Read `.github/instructions/code-review.instructions.md` and follow it.** It is the specification for this task, not background reading — it carries the procedure (establish the diff, run `npm run lint` and `npm test`, then the five dimensions) as well as the invariants and the reporting bar. Do not restate it here; run it. -That file sits under `.github/instructions/` so Copilot code review picks it up natively rather -than needing a condensed second copy. It is the only copy of the standard; changes to how reviews -work go there, not here. +That file sits under `.github/instructions/` so Copilot code review picks it up natively rather than needing a condensed second copy. It is the only copy of the standard; changes to how reviews work go there, not here. Two things this skill adds on top: -**Dispatch the judgement pass to a subagent.** Steps 1-2 of the procedure — the diff and the -deterministic layer — run inline. Step 3, the five dimensions, goes to an `Explore` subagent given -the diff and the instructions file, and nothing else from this conversation. If the session that -wrote the code also reviews it, it reviews its own reasoning and finds it sound. Collect the -subagent's findings and report them. +**Dispatch the judgement pass to a subagent.** Steps 1-2 of the procedure — the diff and the deterministic layer — run inline. Step 3, the five dimensions, goes to an `Explore` subagent given the diff and the instructions file, and nothing else from this conversation. If the session that wrote the code also reviews it, it reviews its own reasoning and finds it sound. Collect the subagent's findings and report them. -**Report in the chat and stop there.** No GitHub comments, no files written — the PR does not exist -yet. Then offer; do not apply. The author decides what is a real finding. +**Report in the chat and stop there.** No GitHub comments, no files written — the PR does not exist yet. Then offer; do not apply. The author decides what is a real finding. ## Notes -- Complements the built-in `/code-review` and `/security-review`. What this adds is the - repo-specific knowledge: Electron's bundled Node, `isomorphic-git`, `electron-store`, loopback - binding, the Windows spawn shims. -- If a finding reveals a rule the instructions file does not yet cover, say so. It is meant to - accumulate what the project learns. +- Complements the built-in `/code-review` and `/security-review`. What this adds is the repo-specific knowledge: Electron's bundled Node, `isomorphic-git`, `electron-store`, loopback binding, the Windows spawn shims. +- If a finding reveals a rule the instructions file does not yet cover, say so. It is meant to accumulate what the project learns. diff --git a/.editorconfig b/.editorconfig index 7511ffa..1cb8aae 100644 --- a/.editorconfig +++ b/.editorconfig @@ -26,3 +26,5 @@ indent_size = 2 [*.md] trim_trailing_whitespace = false +# Prose is not wrapped here — one paragraph is one line. See AGENTS.md. +max_line_length = off diff --git a/.github/ISSUE_TEMPLATE/Bug_report.yml b/.github/ISSUE_TEMPLATE/Bug_report.yml index 3c4722a..40053a8 100644 --- a/.github/ISSUE_TEMPLATE/Bug_report.yml +++ b/.github/ISSUE_TEMPLATE/Bug_report.yml @@ -64,7 +64,7 @@ body: id: existing attributes: label: Please confirm that you have searched existing issues in the repo. - description: You can do this by searching https://github.com/WordPress/experimental-wp-dev-env/issues + description: You can do this by searching https://github.com/WordPress/contributor-toolkit/issues options: - label: "Yes" required: true diff --git a/.github/ISSUE_TEMPLATE/installation-bug.yml b/.github/ISSUE_TEMPLATE/installation-bug.yml index 0fbbff8..6a5f2a7 100644 --- a/.github/ISSUE_TEMPLATE/installation-bug.yml +++ b/.github/ISSUE_TEMPLATE/installation-bug.yml @@ -40,7 +40,7 @@ body: attributes: label: Where did you download the app from? description: Please provide the exact URL or describe where you got it (e.g. GitHub Releases page, a blog post link, etc.). - placeholder: "e.g. https://github.com/WordPress/experimental-wp-dev-env/releases/tag/v1.0.0" + placeholder: "e.g. https://github.com/WordPress/contributor-toolkit/releases/tag/v1.0.0" validations: required: true diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 7cdb0e4..67128dc 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -4,23 +4,13 @@ applyTo: "**" # Automated review rules -What an automated reviewer should look for in this repo, and how to run that review. Written to be -read by any agent, not one in particular — this is the single source of truth for the review -standard, and it is deliberately the only copy of it. +What an automated reviewer should look for in this repo, and how to run that review. Written to be read by any agent, not one in particular — this is the single source of truth for the review standard, and it is deliberately the only copy of it. -`AGENTS.md` and `.claude/skills/self-review/SKILL.md` point here rather than restate it. Copilot -needs no pointer: it reads `.github/instructions/*.instructions.md` natively, selecting them by -matching the `applyTo` glob above against the files in a pull request, so `**` means every PR gets -this. That is the whole reason the file lives at this path and not somewhere better-named — a -pointer would not have reached it, and a second condensed copy would have drifted. +`AGENTS.md` and `.claude/skills/self-review/SKILL.md` point here rather than restate it. Copilot needs no pointer: it reads `.github/instructions/*.instructions.md` natively, selecting them by matching the `applyTo` glob above against the files in a pull request, so `**` means every PR gets this. That is the whole reason the file lives at this path and not somewhere better-named — a pointer would not have reached it, and a second condensed copy would have drifted. -The procedure below assumes an agent that can run commands. Copilot cannot; it should skip to -**Scope** and treat the rest as the standard to review against. +The procedure below assumes an agent that can run commands. Copilot cannot; it should skip to **Scope** and treat the rest as the standard to review against. -Nothing runs this automatically. It is the author's pass, before a human reads the diff — which is -the point: a finding fixed now costs one message, the same finding on the PR costs a review cycle. -The producer is responsible for handing over a reviewable change, not the reviewer for -reconstructing the context. +Nothing runs this automatically. It is the author's pass, before a human reads the diff — which is the point: a finding fixed now costs one message, the same finding on the PR costs a review cycle. The producer is responsible for handing over a reviewable change, not the reviewer for reconstructing the context. ## Running the review @@ -32,8 +22,7 @@ git diff --stat origin/trunk...HEAD git diff origin/trunk...HEAD ``` -Include uncommitted work if there is any (`git status --short`, `git diff`) — the author is about -to commit it, so it is in scope. +Include uncommitted work if there is any (`git status --short`, `git diff`) — the author is about to commit it, so it is in scope. **2. Run the deterministic layer first**, so mechanical findings never reach the judgement pass: @@ -42,229 +31,113 @@ npm run lint npm test ``` -Both are repo-wide and both are clean on `trunk` — the lint backlog was cleared in #117, which is -why `lint.yml` runs `eslint .` rather than linting only the changed files. So any failure here -belongs to the branch. Report both results plainly. +Both are repo-wide and both are clean on `trunk` — the lint backlog was cleared in #117, which is why `lint.yml` runs `eslint .` rather than linting only the changed files. So any failure here belongs to the branch. Report both results plainly. -If ESLint fails, `npm run lint:fix` handles the mechanical part. Check what it rewrote before -committing: it is also repo-wide, so a rule that starts flagging untouched files would pull them -into the diff. Do not hand-fix what the fixer handles. +If ESLint fails, `npm run lint:fix` handles the mechanical part. Check what it rewrote before committing: it is also repo-wide, so a rule that starts flagging untouched files would pull them into the diff. Do not hand-fix what the fixer handles. -**3. Review the five dimensions below.** Read the surrounding files, not just the diff — a diff -rarely shows that a helper already handles the case, and the reporting bar requires verifying a -finding before asserting it. +**3. Review the five dimensions below.** Read the surrounding files, not just the diff — a diff rarely shows that a helper already handles the case, and the reporting bar requires verifying a finding before asserting it. -Where the tool allows it, run this pass with fresh context — a subagent given the diff and this -file, rather than the session that wrote the code. Nothing runs this review independently any more, -so a reviewer that already believes the change is correct is the main way it stops working. +Where the tool allows it, run this pass with fresh context — a subagent given the diff and this file, rather than the session that wrote the code. Nothing runs this review independently any more, so a reviewer that already believes the change is correct is the main way it stops working. -**4. Report, then offer.** Format below. Ask before changing anything: the author decides what is a -real finding, which is the whole reason this happens before the PR rather than after. +**4. Report, then offer.** Format below. Ask before changing anything: the author decides what is a real finding, which is the whole reason this happens before the PR rather than after. ## Scope -Review **judgement**, not style. ESLint (`eslint.config.mjs`, `npm run lint`) already covers -formatting, unused variables, JSDoc, React hooks and the rest of the mechanical layer. Repeating -those here buries the findings that matter. When style and process nits share space with -substantive findings, authors learn to skim past them — and the substantive findings go with -them. +Review **judgement**, not style. ESLint (`eslint.config.mjs`, `npm run lint`) already covers formatting, unused variables, JSDoc, React hooks and the rest of the mechanical layer. Repeating those here buries the findings that matter. When style and process nits share space with substantive findings, authors learn to skim past them — and the substantive findings go with them. -Five dimensions, in priority order: **architecture · security · performance · cross-platform · -tests**. +Five dimensions, in priority order: **architecture · security · performance · cross-platform · tests**. ## What this app is -An Electron app that sets up a `wordpress-develop` environment with **zero prerequisites** — no -Git, Node, npm or Docker on the host. Everything runs as JS/WASM inside the Electron process. Its -users are Contributor Day newcomers on macOS and Windows, often on locked-down machines. +An Electron app that sets up a `wordpress-develop` environment with **zero prerequisites** — no Git, Node, npm or Docker on the host. Everything runs as JS/WASM inside the Electron process. Its users are Contributor Day newcomers on macOS and Windows, often on locked-down machines. -That premise is what most of the rules below protect. A change that quietly reintroduces a host -dependency defeats the entire point of the project. +That premise is what most of the rules below protect. A change that quietly reintroduces a host dependency defeats the entire point of the project. --- ## 1. Architecture -Invariants. Breaking one is a `[fix here]` finding even when the code works on the author's -machine. - -**Child processes run on Electron's bundled Node, never the host's.** Spawns go through -`process.execPath` with `ELECTRON_RUN_AS_NODE=1` in the environment (see `runNpmWithEngineRetry` -and the `playground:start` handler in `src/main.js`, and `buildChildEnv`). A bare `spawn('node')` -or `spawn('npm')` assumes a host toolchain that is not there. On Windows child `npm` processes -find a `node` at all only because of the `PATH` shim built by `ensureNodeShimDir` — new spawns -must inherit that environment rather than build their own. - -**Git never shells out.** All Git operations go through `isomorphic-git`. Patch and diff -generation is hand-rolled in `src/main.js` (stage untracked files, diff working tree against -`origin/trunk`) precisely because there is no `git` binary to call. Any `spawn('git')` or -`exec('git ...')` is a regression. - -**`electron-store` is the only persistence layer.** No database, no sidecar JSON. It holds the -site registry and per-site metadata and is the single source of truth for "known sites". A second -store, a cache file, or state parked in a module-level variable that outlives a handler is -architectural drift — flag it. - -**Long-running output streams; it is not returned.** Installs, scripts and the Playground server -push output to the renderer through correlated IDs (`installId`, `runId`, `sitePath`) over -dedicated channels. See the handler pairs in `src/preload.js`. A new long-running operation that -resolves its `invoke()` with accumulated output instead of streaming will look fine on a fast -machine and hang the UI on a slow one. - -**The renderer↔main boundary is fixed.** New surface means a `contextBridge` entry in -`src/preload.js` plus an `ipcMain.handle` in `src/main.js`. `contextIsolation: true` and -`nodeIntegration: false` are set on every `BrowserWindow` — the main window and both patch -windows — and are not negotiable. Flag any window created without them, and any attempt to widen -the bridge by exposing `ipcRenderer` itself rather than named functions. - -**Failure paths are part of the architecture.** The users cannot debug: a swallowed error is -"the button did nothing" at a Contributor Day, with nobody able to diagnose it. Every spawn -handles both `error` and `close` — Node documents that exit events "may or may not" follow a -spawn failure, and `runNpmWithEngineRetry` in `src/main.js` shows the expected shape. No silent -`catch`: an error that never reaches the renderer's log stream did not happen, from the user's -chair. And a setup that dies halfway must leave the site registry consistent — no phantom site -in `electron-store` for a directory that was never finished. - -**Renderer decisions live in modules, not in `index.jsx`.** `src/renderer/index.jsx` mounts itself -at module scope and cannot be loaded without a DOM, so nothing in the suite can reach it: a -decision made there is untestable by construction. Anything with more than one branch — a string -the user reads, a path joined, a status derived, a command parsed — belongs in a -`src/renderer/*.cjs` module with its own test, leaving the component holding JSX, state -assignments and the call. `site-folder.cjs` and `open-failure.cjs` are the shape. - -This is the direction chosen in #216 over building a DOM harness, which was judged too much setup -for the coverage it buys against a 4000-line component. The consequence is that it is enforced -here, by review, and nowhere else — `no-unused-vars` catches a module whose last call site is -deleted, but nothing catches a second code path that answers the same question inline. That is -exactly what #180 was. Reopen the harness question if a bug ever lands in the assignments the -modules cannot absorb. - -**New dependencies are findings by default.** Native compilation or a host binary breaks the -zero-prerequisite promise on user machines. A dependency with lifecycle scripts also needs an -`allowScripts` entry in `package.json` — the mechanism already exists, and a missing entry means -its install scripts silently don't run. - -**Changing the shape of what `electron-store` holds needs a migration path.** Existing users -have site registries on disk; a renamed or restructured key silently orphans their sites. +Invariants. Breaking one is a `[fix here]` finding even when the code works on the author's machine. + +**Child processes run on Electron's bundled Node, never the host's.** Spawns go through `process.execPath` with `ELECTRON_RUN_AS_NODE=1` in the environment (see `runNpmWithEngineRetry` and the `playground:start` handler in `src/main.js`, and `buildChildEnv`). A bare `spawn('node')` or `spawn('npm')` assumes a host toolchain that is not there. On Windows child `npm` processes find a `node` at all only because of the `PATH` shim built by `ensureNodeShimDir` — new spawns must inherit that environment rather than build their own. + +**Git never shells out.** All Git operations go through `isomorphic-git`. Patch and diff generation is hand-rolled in `src/main.js` (stage untracked files, diff working tree against `origin/trunk`) precisely because there is no `git` binary to call. Any `spawn('git')` or `exec('git ...')` is a regression. + +**`electron-store` is the only persistence layer.** No database, no sidecar JSON. It holds the site registry and per-site metadata and is the single source of truth for "known sites". A second store, a cache file, or state parked in a module-level variable that outlives a handler is architectural drift — flag it. + +**Long-running output streams; it is not returned.** Installs, scripts and the Playground server push output to the renderer through correlated IDs (`installId`, `runId`, `sitePath`) over dedicated channels. See the handler pairs in `src/preload.js`. A new long-running operation that resolves its `invoke()` with accumulated output instead of streaming will look fine on a fast machine and hang the UI on a slow one. + +**The renderer↔main boundary is fixed.** New surface means a `contextBridge` entry in `src/preload.js` plus an `ipcMain.handle` in `src/main.js`. `contextIsolation: true` and `nodeIntegration: false` are set on every `BrowserWindow` — the main window and both patch windows — and are not negotiable. Flag any window created without them, and any attempt to widen the bridge by exposing `ipcRenderer` itself rather than named functions. + +**Failure paths are part of the architecture.** The users cannot debug: a swallowed error is "the button did nothing" at a Contributor Day, with nobody able to diagnose it. Every spawn handles both `error` and `close` — Node documents that exit events "may or may not" follow a spawn failure, and `runNpmWithEngineRetry` in `src/main.js` shows the expected shape. No silent `catch`: an error that never reaches the renderer's log stream did not happen, from the user's chair. And a setup that dies halfway must leave the site registry consistent — no phantom site in `electron-store` for a directory that was never finished. + +**Renderer decisions live in modules, not in `index.jsx`.** `src/renderer/index.jsx` mounts itself at module scope and cannot be loaded without a DOM, so nothing in the suite can reach it: a decision made there is untestable by construction. Anything with more than one branch — a string the user reads, a path joined, a status derived, a command parsed — belongs in a `src/renderer/*.cjs` module with its own test, leaving the component holding JSX, state assignments and the call. `site-folder.cjs` and `open-failure.cjs` are the shape. + +This is the direction chosen in #216 over building a DOM harness, which was judged too much setup for the coverage it buys against a 4000-line component. The consequence is that it is enforced here, by review, and nowhere else — `no-unused-vars` catches a module whose last call site is deleted, but nothing catches a second code path that answers the same question inline. That is exactly what #180 was. Reopen the harness question if a bug ever lands in the assignments the modules cannot absorb. + +**New dependencies are findings by default.** Native compilation or a host binary breaks the zero-prerequisite promise on user machines. A dependency with lifecycle scripts also needs an `allowScripts` entry in `package.json` — the mechanism already exists, and a missing entry means its install scripts silently don't run. + +**Changing the shape of what `electron-store` holds needs a migration path.** Existing users have site registries on disk; a renamed or restructured key silently orphans their sites. ## 2. Security -The threat model is not a hostile user — it is a contributor's laptop on a conference or café -network, running a WordPress with `admin`/`admin`. +The threat model is not a hostile user — it is a contributor's laptop on a conference or café network, running a WordPress with `admin`/`admin`. -**Validate what crosses IPC.** Every `ipcMain.handle` argument comes from the renderer and is -untrusted input. Paths get used for file operations, URLs get opened. +**Validate what crosses IPC.** Every `ipcMain.handle` argument comes from the renderer and is untrusted input. Paths get used for file operations, URLs get opened. -> Worked example, kept because it shows the shape rather than a single bug: `url:open` in -> `src/main.js` used to pass its argument straight to `shell.openExternal()`, so `file://` and -> `javascript:` got through to the OS handler. It now goes through `src/external-url.js`, which -> refuses anything outside an http/https allow-list and — the part that is easy to miss — hands -> the OS the *parsed* address rather than the caller's string, because the URL parser strips -> control characters and a validator that checks one string while the caller opens another has -> not checked anything. Look for both halves in any new handler that takes a URL or a path. +> Worked example, kept because it shows the shape rather than a single bug: `url:open` in `src/main.js` used to pass its argument straight to `shell.openExternal()`, so `file://` and `javascript:` got through to the OS handler. It now goes through `src/external-url.js`, which refuses anything outside an http/https allow-list and — the part that is easy to miss — hands the OS the *parsed* address rather than the caller's string, because the URL parser strips control characters and a validator that checks one string while the caller opens another has not checked anything. Look for both halves in any new handler that takes a URL or a path. -**Servers stay on loopback.** `src/bind-loopback.js` patches `net.Server.prototype.listen` so -Playground's servers bind to `127.0.0.1` instead of every interface. Any new listener that is -created before that patch is applied, or that passes an explicit non-loopback host, exposes the -contributor's site to the local network. This is what the file exists to prevent — read its -header comment before deciding a change is safe. +**Servers stay on loopback.** `src/bind-loopback.js` patches `net.Server.prototype.listen` so Playground's servers bind to `127.0.0.1` instead of every interface. Any new listener that is created before that patch is applied, or that passes an explicit non-loopback host, exposes the contributor's site to the local network. This is what the file exists to prevent — read its header comment before deciding a change is safe. -**Never `shell: true`.** Existing spawns pass `shell: false` deliberately. A shell turns any -path containing a space or a quote into a command-injection vector, and contributor directory -names are user-chosen. The Windows `.cmd` shim path in `src/win-spawn-patch.js` is the one -audited exception; new code should not add another. +**Never `shell: true`.** Existing spawns pass `shell: false` deliberately. A shell turns any path containing a space or a quote into a command-injection vector, and contributor directory names are user-chosen. The Windows `.cmd` shim path in `src/win-spawn-patch.js` is the one audited exception; new code should not add another. -**No secrets in the repo, and none in logs.** Pay particular attention to -`scripts/azure-sign.cjs` and `fastlane/`. Signing credentials arrive as environment variables and -must not be echoed into child-process output, which is streamed to the renderer and written to -disk by `electron-log`. +**No secrets in the repo, and none in logs.** Pay particular attention to `scripts/azure-sign.cjs` and `fastlane/`. Signing credentials arrive as environment variables and must not be echoed into child-process output, which is streamed to the renderer and written to disk by `electron-log`. -**Untrusted archives and downloads.** Anything unzipped or fetched into a user directory should -be checked for path traversal (`../` in archive entries) before extraction. +**Untrusted archives and downloads.** Anything unzipped or fetched into a user directory should be checked for path traversal (`../` in archive entries) before extraction. ## 3. Performance -**The main process must not block.** It runs the UI. Synchronous filesystem calls, large -`JSON.parse`, or hashing on a path that a handler can reach will freeze the whole window. `fs` -sync calls during startup or inside an `ipcMain.handle` deserve a flag; the same call in a -one-shot build script does not. +**The main process must not block.** It runs the UI. Synchronous filesystem calls, large `JSON.parse`, or hashing on a path that a handler can reach will freeze the whole window. `fs` sync calls during startup or inside an `ipcMain.handle` deserve a flag; the same call in a one-shot build script does not. -**Log output is unbounded by nature.** `npm install` on `wordpress-develop` produces a lot of it. -Watch for per-chunk work that is quadratic, and for buffers that accumulate the full output with -no ceiling — see `src/log-lines.js` for how line splitting is done today. +**Log output is unbounded by nature.** `npm install` on `wordpress-develop` produces a lot of it. Watch for per-chunk work that is quadratic, and for buffers that accumulate the full output with no ceiling — see `src/log-lines.js` for how line splitting is done today. -**Work should not scale with the site registry.** Anything that walks every known site, or stats -every directory, on each render or each IPC call will degrade as a contributor accumulates sites. +**Work should not scale with the site registry.** Anything that walks every known site, or stats every directory, on each render or each IPC call will degrade as a contributor accumulates sites. -Flag performance only where there is a plausible path to a user noticing it. Speculative -micro-optimisation is noise. +Flag performance only where there is a plausible path to a user noticing it. Speculative micro-optimisation is noise. ## 4. Cross-platform -macOS and Windows are the primary targets; Linux artifacts are published too. CI runs the unit -suite on macOS and Windows, but plenty gets past it. +macOS and Windows are the primary targets; Linux artifacts are published too. CI runs the unit suite on macOS and Windows, but plenty gets past it. -**Paths.** Compose with `path.join` / `path.resolve`, never string concatenation with `/`. Do not -compare paths case-sensitively — Windows and default macOS filesystems are case-insensitive. Do -not assume a path has no spaces; contributors pick their own directories. +**Paths.** Compose with `path.join` / `path.resolve`, never string concatenation with `/`. Do not compare paths case-sensitively — Windows and default macOS filesystems are case-insensitive. Do not assume a path has no spaces; contributors pick their own directories. -**Killing processes is platform-split.** POSIX relies on the child being a process-group leader -(`detached: true`) so the whole tree can be signalled; Windows uses `taskkill /T`. Both live in -`src/kill-tree.js`. A new spawn that does not set `detached` correctly, or that is terminated with -a bare `child.kill()`, will leave orphaned processes on one platform or the other. +**Killing processes is platform-split.** POSIX relies on the child being a process-group leader (`detached: true`) so the whole tree can be signalled; Windows uses `taskkill /T`. Both live in `src/kill-tree.js`. A new spawn that does not set `detached` correctly, or that is terminated with a bare `child.kill()`, will leave orphaned processes on one platform or the other. -**Windows specifics.** Executables need their extension (`.cmd`, `.bat`, `.exe`) resolved; `EPERM` -and `EINVAL` have Windows-specific causes. `src/win-spawn-patch.js` documents the traps that have -already bitten — consult it rather than re-deriving them. +**Windows specifics.** Executables need their extension (`.cmd`, `.bat`, `.exe`) resolved; `EPERM` and `EINVAL` have Windows-specific causes. `src/win-spawn-patch.js` documents the traps that have already bitten — consult it rather than re-deriving them. -**`windowsHide: true` turns on the child's subsystem, not on whether anyone reads its output.** -Node sets `CREATE_NO_WINDOW` and STARTUPINFO's "start hidden" together; which of the two the child -honors is what differs. A console application takes the first and gets a console that is never -displayed — the flashing fix, and why `src/kill-tree.js` sets it on `taskkill` even though nothing -reads that output. A GUI application takes the second, and one that passes it through to its first -window — Chromium does, so VS Code and Cursor both — launches invisible while the spawn still -reports success (#181). So: set it on a console child; leave it off a child whose window is the -point. `src/editor-launch.js` is the only case of the second kind. +**`windowsHide: true` turns on the child's subsystem, not on whether anyone reads its output.** Node sets `CREATE_NO_WINDOW` and STARTUPINFO's "start hidden" together; which of the two the child honors is what differs. A console application takes the first and gets a console that is never displayed — the flashing fix, and why `src/kill-tree.js` sets it on `taskkill` even though nothing reads that output. A GUI application takes the second, and one that passes it through to its first window — Chromium does, so VS Code and Cursor both — launches invisible while the spawn still reports success (#181). So: set it on a console child; leave it off a child whose window is the point. `src/editor-launch.js` is the only case of the second kind. **Line endings** matter when generating patches, which are destined for Trac. -**Electron's Node version is a real constraint.** It is set independently of `.nvmrc` and the two -have drifted before (issues #37 / #46), which is why the suite runs twice. A change that depends -on a newer Node API needs to hold on whichever of the two is older. +**Electron's Node version is a real constraint.** It is set independently of `.nvmrc` and the two have drifted before (issues #37 / #46), which is why the suite runs twice. A change that depends on a newer Node API needs to hold on whichever of the two is older. ## 5. Tests -**A new feature or bugfix without a test is a finding.** Not a suggestion in a footer — a -finding, with the same severity-and-scope labelling as everything else. The suite is -`node --test` over `test/*.test.cjs`; new tests follow the patterns already there. +**A new feature or bugfix without a test is a finding.** Not a suggestion in a footer — a finding, with the same severity-and-scope labelling as everything else. The suite is `node --test` over `tests/unit/*.test.cjs`; new tests follow the patterns already there. -**A bugfix's test must reproduce the bug**: fail on the old code, pass on the new. A test -written after the fix that never saw the bug proves far less — it pins the current behaviour, -whatever that is, rather than the correction. When reviewing a bugfix, check whether the test -would actually have caught it. +**A bugfix's test must reproduce the bug**: fail on the old code, pass on the new. A test written after the fix that never saw the bug proves far less — it pins the current behaviour, whatever that is, rather than the correction. When reviewing a bugfix, check whether the test would actually have caught it. **Watch for tests that are green while proving nothing.** The known shapes in this repo: - Mocking the very thing the test claims to verify. -- Asserting implementation details — exact log strings, call order of internals — instead of - observable behaviour. These break on harmless refactors and survive real bugs. -- Passing on only one of the two Node runtimes. CI runs the suite on `.nvmrc`'s Node *and* on - Electron's bundled Node because the two are set independently and have drifted; a test (or the - code under it) that assumes the newer of the two is broken on the other. - -**Platform-conditional code needs both branches tested — from one machine.** The house pattern -is dependency injection, not skipping: `test/win-spawn-patch.test.cjs` exercises the Windows -paths from macOS by injecting `platform`, lookup and env rather than reading `process.platform`. -A new platform split tested with `it.skip` on the other OS is a coverage hole CI will never -close, since the suite runs on both platforms but each skips the other's branch. - -**Renderer logic is tested through its module, so check it has one.** A PR that puts a branch -inside `src/renderer/index.jsx` has written code the suite cannot reach — see the invariant in §1. -The finding is the missing module, not the missing test. - -**Scope stays proportional.** Missing tests on a touched line of legacy code is `[follow-up]`, -not `[fix here]` — the strong rule applies to what the PR introduces, not to everything it -brushes against. +- Asserting implementation details — exact log strings, call order of internals — instead of observable behaviour. These break on harmless refactors and survive real bugs. +- Passing on only one of the two Node runtimes. CI runs the suite on `.nvmrc`'s Node *and* on Electron's bundled Node because the two are set independently and have drifted; a test (or the code under it) that assumes the newer of the two is broken on the other. + +**Platform-conditional code needs both branches tested — from one machine.** The house pattern is dependency injection, not skipping: `tests/unit/win-spawn-patch.test.cjs` exercises the Windows paths from macOS by injecting `platform`, lookup and env rather than reading `process.platform`. A new platform split tested with `it.skip` on the other OS is a coverage hole CI will never close, since the suite runs on both platforms but each skips the other's branch. + +**Renderer logic is tested through its module, so check it has one.** A PR that puts a branch inside `src/renderer/index.jsx` has written code the suite cannot reach — see the invariant in §1. The finding is the missing module, not the missing test. + +**Scope stays proportional.** Missing tests on a touched line of legacy code is `[follow-up]`, not `[fix here]` — the strong rule applies to what the PR introduces, not to everything it brushes against. --- @@ -272,28 +145,17 @@ brushes against. **Every finding carries a dimension, a severity and a scope.** -- Scope is `[fix here]` or `[follow-up]`. Use `[follow-up]` for cross-cutting refactors and - anything the PR did not introduce. This gives the author a defensible "noted, separate PR" - rather than a finding that looks ignored. +- Scope is `[fix here]` or `[follow-up]`. Use `[follow-up]` for cross-cutting refactors and anything the PR did not introduce. This gives the author a defensible "noted, separate PR" rather than a finding that looks ignored. - Severity: 🔴 high, 🟡 medium, 🔵 low. -**Structure**: counts first (`2 [fix here] · 1 [follow-up]`), then one finding per entry carrying -its dimension, severity, scope, `file:line`, and what actually goes wrong. Style and process -observations go last, grouped and brief — never mixed in among the findings. +**Structure**: counts first (`2 [fix here] · 1 [follow-up]`), then one finding per entry carrying its dimension, severity, scope, `file:line`, and what actually goes wrong. Style and process observations go last, grouped and brief — never mixed in among the findings. -Where this runs before the PR exists, that report goes in the chat: no GitHub comments, no files -written. Where it runs on a PR, `[fix here]` findings become inline comments on the exact lines and -the counts go in a single summary comment, with the style notes in a collapsed `
` block. +Where this runs before the PR exists, that report goes in the chat: no GitHub comments, no files written. Where it runs on a PR, `[fix here]` findings become inline comments on the exact lines and the counts go in a single summary comment, with the style notes in a collapsed `
` block. -**On a re-run, reconcile — do not re-review from scratch.** Mark each earlier finding resolved, -still open, or obsolete. Re-asserting a fixed finding is the fastest way to get the whole review -ignored. +**On a re-run, reconcile — do not re-review from scratch.** Mark each earlier finding resolved, still open, or obsolete. Re-asserting a fixed finding is the fastest way to get the whole review ignored. -**Say when there is nothing.** "No findings across the five dimensions" in one line is a good -review. Do not pad. Do not restate what the PR does — the author knows. +**Say when there is nothing.** "No findings across the five dimensions" in one line is a good review. Do not pad. Do not restate what the PR does — the author knows. -**Verify before claiming.** Read the surrounding file before asserting an invariant is broken; -the diff alone often does not show that a helper already handles the case. A confident wrong -finding costs more than a missed one. +**Verify before claiming.** Read the surrounding file before asserting an invariant is broken; the diff alone often does not show that a helper already handles the case. A confident wrong finding costs more than a missed one. **Never approve, request changes, or merge.** Post comments only. The human decides. diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index c944c70..d65f85c 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -5,12 +5,15 @@ # # Screenshots are committed to the repo, not captured here. Capturing them needs the Electron # app running against a seeded site, which is minutes of flaky work on a runner; a maintainer -# runs `npm run shots` locally when the UI changes instead (see scripts/screenshots/). +# runs `npm run shots` locally when the UI changes instead (see scripts/screenshots/). Four of +# them show states no seeded site can reach — a running dev server, a site setting itself up, +# a ticket read from Trac — so they come from `npm run shots -- --tier=live` against a real +# site, with a maintainer at the keyboard. shots.cjs says which ones and why. # # The site base path is derived from the repository name at run time rather than hardcoded, -# because a project Pages site lives under // and this repository is being renamed -# (experimental-wp-dev-env → contributor-toolkit). Deriving it means the rename cannot break -# every asset URL. +# because a project Pages site lives under // and this repository has already been +# renamed once (experimental-wp-dev-env → contributor-toolkit). Deriving it means another rename +# cannot break every asset URL. # # Pull requests get a build-only job: a broken link or bad markdown fails before merge, and # the deploy job (which holds the OIDC token) never runs on PR code. diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml new file mode 100644 index 0000000..ad4b415 --- /dev/null +++ b/.github/workflows/e2e.yml @@ -0,0 +1,132 @@ +# Launches the app on the two platforms it ships to. +# +# Two jobs, because there are two different apps under test and they cost very +# different amounts: +# +# journeys drives the source tree through the flows a contributor performs by +# hand. No packaging, so it is quick, and it is the one that fails when +# a change loses somebody's work. +# packaged builds an unsigned artifact and asks whether packaging worked at all. +# Several minutes per platform, and it catches failures — asar layout, +# native module rebuilds, bundled CLI resolution — that cannot happen +# from source. +# +# Neither needs a secret: nothing here is signed. Buildkite still owns the signed +# builds (.buildkite/pipeline.yml). +# +# No `playwright install` step in either job. These tests launch Electron; no +# browser is ever downloaded. + +name: E2E + +on: + pull_request: + # `ready_for_review` is not in the default set. Without it, a PR opened as a draft + # and later marked ready never runs this workflow at all — the draft guard below + # would skip the only run it ever gets. + types: [opened, synchronize, reopened, ready_for_review] + push: + branches: [trunk] + +permissions: + contents: read + +# A new push supersedes the previous run on the same ref. +concurrency: + group: e2e-${{ github.ref }} + cancel-in-progress: true + +jobs: + journeys: + name: Journeys (${{ matrix.os }}) + if: github.event_name != 'pull_request' || github.event.pull_request.draft == false + runs-on: ${{ matrix.os }} + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + os: [macos-latest, windows-latest] + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version-file: .nvmrc + cache: npm + + - run: npm ci + + # The journeys read the renderer the app actually loads, so it has to exist. + # `postinstall` builds it too; this is here so a change to that never leaves + # the suite asserting against a stale bundle. + - name: Build the renderer bundle + run: npm run build:once + + - name: Run the journeys + run: npm run test:e2e + + - name: Upload Playwright report + if: failure() + uses: actions/upload-artifact@v4 + with: + name: playwright-journeys-${{ matrix.os }} + path: | + playwright-report + test-results + retention-days: 7 + + packaged: + name: Packaged smoke (${{ matrix.os }}) + # Packaging takes several minutes per platform; don't spend it on drafts. + if: github.event_name != 'pull_request' || github.event.pull_request.draft == false + runs-on: ${{ matrix.os }} + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + os: [macos-latest, windows-latest] + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version-file: .nvmrc + cache: npm + + # `postinstall` runs `electron-builder install-app-deps`, which is what + # rebuilds the native file-locking module against Electron's ABI. + - run: npm ci + + - name: Build the renderer bundle + run: npm run build:once + + # Buildkite creates this directory during signed macOS builds. A canary makes the + # packaged test prove that build-only signing material stays out of every payload. + - name: Stage a build-only signing canary + shell: bash + run: | + mkdir -p .codesigning + printf 'must not ship' > .codesigning/apple_api_key + + - name: Package (unsigned, --dir) + # Mandatory on macOS: electron-builder signs during `--dir` otherwise. + # Harmless elsewhere. Windows signing already no-ops without the Azure + # env vars (scripts/azure-sign.cjs). + env: + CSC_IDENTITY_AUTO_DISCOVERY: 'false' + run: npm run pack:dir + + - name: Run the packaged smoke test + run: npm run test:e2e:packaged + + - name: Upload Playwright report + if: failure() + uses: actions/upload-artifact@v4 + with: + name: playwright-packaged-${{ matrix.os }} + path: | + playwright-report + test-results + retention-days: 7 diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 70c87a5..4bf3193 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -40,7 +40,7 @@ jobs: cache: npm # `npm ci` and not `--ignore-scripts`: the postinstall downloads the Electron binary the - # second pass runs on, and test/npm-install.integration.test.cjs needs node_modules/npm. + # second pass runs on, and tests/unit/npm-install.integration.test.cjs needs node_modules/npm. - name: Install dependencies run: npm ci diff --git a/.gitignore b/.gitignore index 6cbee22..0e17eee 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,11 @@ docs/.vitepress/cache src/renderer/index.js src/renderer/index.css +# Playwright output. Traces, screenshots and the HTML report are written per run and +# uploaded as CI artifacts on failure; none of it is worth committing. +playwright-report +test-results + # Files with potential secrets .env .codesigning/ diff --git a/AGENTS.md b/AGENTS.md index 389de3e..444f178 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,149 +1,98 @@ # AGENTS.md -Instructions for AI coding agents working in this repository. Tool-neutral and canonical — -`CLAUDE.md` points here rather than repeating it. +Instructions for AI coding agents working in this repository. Tool-neutral and canonical — `CLAUDE.md` points here rather than repeating it. ## Where things are, whatever agent you use Two files, and neither is specific to one tool despite what their paths suggest: -**The review standard** — -[`.github/instructions/code-review.instructions.md`](.github/instructions/code-review.instructions.md). -The five dimensions, this project's invariants, the procedure to run them, and the reporting -format. Read it directly if your agent has not already. +**The review standard** — [`.github/instructions/code-review.instructions.md`](.github/instructions/code-review.instructions.md). The five dimensions, this project's invariants, the procedure to run them, and the reporting format. Read it directly if your agent has not already. -It sits under `.github/instructions/` because Copilot code review reads that directory natively and -follows no links out of it. Anywhere better-named would have meant maintaining a condensed second -copy for Copilot, which would drift. Every other agent reaches it from here. +It sits under `.github/instructions/` because Copilot code review reads that directory natively and follows no links out of it. Anywhere better-named would have meant maintaining a condensed second copy for Copilot, which would drift. Every other agent reaches it from here. -**The review as a skill** — a thin `SKILL.md` wrapper over the file above, in two locations because -no single skills directory is read by every agent: +**The review as a skill** — a thin `SKILL.md` wrapper over the file above, in two locations because no single skills directory is read by every agent: -- [`.claude/skills/self-review/`](.claude/skills/self-review/) — Claude Code and Copilot. In Claude - Code it is `/self-review`. -- [`.agents/skills/self-review/`](.agents/skills/self-review/) — the Agent Skills open-standard path - that Command Code and others scan. +- [`.claude/skills/self-review/`](.claude/skills/self-review/) — Claude Code and Copilot. In Claude Code it is `/self-review`. +- [`.agents/skills/self-review/`](.agents/skills/self-review/) — the Agent Skills open-standard path that Command Code and others scan. -Both are pointers to the one instructions file, not copies of the standard. If your agent looks -somewhere else again (`.github/skills/`, `.cursor/skills/`, `.codex/skills/`, or its own -convention), add a wrapper there or skip it entirely and follow the instructions file — that is -where all the content lives. The wrapper only adds two things: run the judgement pass in a fresh -context, and report without touching GitHub. +Both are pointers to the one instructions file, not copies of the standard. If your agent looks somewhere else again (`.github/skills/`, `.cursor/skills/`, `.codex/skills/`, or its own convention), add a wrapper there or skip it entirely and follow the instructions file — that is where all the content lives. The wrapper only adds two things: run the judgement pass in a fresh context, and report without touching GitHub. -There is deliberately no per-tool copy of the *standard*. A wrapper is a few lines that defer to it; -duplicating the standard itself is the thing to avoid — if you find yourself doing that, fix the -pointer, not the number of copies. +In Codex, `/review` is the built-in user-facing review command. Whenever `/review` runs, read and follow [`.github/instructions/code-review.instructions.md`](.github/instructions/code-review.instructions.md) in full as the review standard. `self-review` is the local skill name, not a Codex command; do not tell users to invoke `self-review`. + +There is deliberately no per-tool copy of the *standard*. A wrapper is a few lines that defer to it; duplicating the standard itself is the thing to avoid — if you find yourself doing that, fix the pointer, not the number of copies. ## What this is -An Electron desktop app ("WordPress Contributor Toolkit") that sets up a full WordPress core -(`wordpress-develop`) dev environment with zero prerequisites — no Git, Node, npm, or Docker -required on the host. Everything is bundled and run as JS/WASM inside the Electron process. Built -to fix a Contributor Day problem: newcomers burning the whole session on local setup instead of -contributing. Still labeled "experimental." +An Electron desktop app ("WordPress Contributor Toolkit") that sets up a full WordPress core (`wordpress-develop`) dev environment with zero prerequisites — no Git, Node, npm, or Docker required on the host. Everything is bundled and run as JS/WASM inside the Electron process. Built to fix a Contributor Day problem: newcomers burning the whole session on local setup instead of contributing. Still labeled "experimental." ## Before opening a pull request -Run the review in `.github/instructions/code-review.instructions.md` against the branch, and fix or -consciously defer every finding. Summarise the outcome in the pull request description — counts, -what was fixed, what was left as a follow-up and why. +Run the review in `.github/instructions/code-review.instructions.md` against the branch, and fix or consciously defer every finding. Summarise the outcome in the pull request description — counts, what was fixed, what was left as a follow-up and why. + +Before reporting a GitHub workflow complete, verify every requested final state on GitHub — for example, distinguish a merged pull request from one that is merely closed, and confirm that an issue was closed by the intended pull request rather than only by a comment. -Nothing enforces this. There is no automated review on pull requests, by design: it would mean -storing an AI provider credential as a secret in a public repository. This pass is what stands in -its place, so skipping it means a human reviewer is the first reader of the diff. +Nothing enforces this. There is no automated review on pull requests, by design: it would mean storing an AI provider credential as a secret in a public repository. This pass is what stands in its place, so skipping it means a human reviewer is the first reader of the diff. -That file carries the procedure as well as the standard. Follow it rather than improvising a -review. +That file carries the procedure as well as the standard. Follow it rather than improvising a review. ### The pull request description follows the template -[`.github/pull_request_template.md`](.github/pull_request_template.md) is the shape, and GitHub -loads it into every new pull request automatically — including ones opened with `gh pr create`, as -long as you do not pass a `--body` that replaces it. Fill it in rather than writing your own -structure. +[`.github/pull_request_template.md`](.github/pull_request_template.md) is the shape, and GitHub loads it into every new pull request automatically — including ones opened with `gh pr create`, as long as you do not pass a `--body` that replaces it. Fill it in rather than writing your own structure. -The rule it is built around: **a reviewer understands the change in five minutes.** So what stays -visible is Why, What changes, How to test this, Risks, Related — and everything else goes in a -`
` block, collapsed by default. Depth is not the enemy of a readable PR; depth *in the way* -is. Do not delete detail to hit the five minutes, move it. +The rule it is built around: **a reviewer understands the change in five minutes.** So what stays visible is Why, What changes, How to test this, Risks, Related — and everything else goes in a `
` block, collapsed by default. Depth is not the enemy of a readable PR; depth *in the way* is. Do not delete detail to hit the five minutes, move it. -**One template, not one per kind of change.** GitHub shows no picker when a pull request is opened — -selecting among several requires appending `?template=name.md` to the URL, which nobody remembers, -so the default loads anyway. The template is written to serve a fix, a feature and a process change -equally, and calls out the three places where they genuinely differ: a fix names its root cause and -the test that fails without it, a feature names what it deliberately leaves out and shows its -surface, and either way the testing steps follow the path a contributor actually takes. +**One template, not one per kind of change.** GitHub shows no picker when a pull request is opened — selecting among several requires appending `?template=name.md` to the URL, which nobody remembers, so the default loads anyway. The template is written to serve a fix, a feature and a process change equally, and calls out the three places where they genuinely differ: a fix names its root cause and the test that fails without it, a feature names what it deliberately leaves out and shows its surface, and either way the testing steps follow the path a contributor actually takes. -That includes the review outcome AGENTS.md requires below: it lives in a collapsed block, with the -headline count surfaced in **Risks and limitations** when it changes how the PR should be read. +That includes the review outcome AGENTS.md requires below: it lives in a collapsed block, with the headline count surfaced in **Risks and limitations** when it changes how the PR should be read. -Titles are `[Action] [what] [where or why]` — "Fix the patch panel's empty diff after a trunk -update", not "Fix bug". +Titles are `[Action] [what] [where or why]` — "Fix the patch panel's empty diff after a trunk update", not "Fix bug". -If the diff is over ~800 lines, the first question is whether it should be two pull requests. A -stacked pair reviews faster than one that nobody wants to start. +If the diff is over ~800 lines, the first question is whether it should be two pull requests. A stacked pair reviews faster than one that nobody wants to start. ### Every pull request says how to test it by hand -A **How to test this** section is required, not optional, and a green test suite does not replace -it. This app's failures live where the unit tests cannot go: a real clone of `wordpress-develop`, a -`node_modules` that takes minutes to install, an OS file dialog, a Windows path with a space in it. -The suite proves the logic; only a person driving the app proves the feature. +A **How to test this** section is required, not optional, and a green test suite does not replace it. This app's failures live where the unit tests cannot go: a real clone of `wordpress-develop`, a `node_modules` that takes minutes to install, an OS file dialog, a Windows path with a space in it. The suite proves the logic; only a person driving the app proves the feature. Write it for someone who did not write the change and does not know where the button is. That means: -- **A starting state.** "A site with a linked ticket and an uncommitted edit", not "a site". Say how - to reach it if it is not the state the app opens in. +- **A starting state.** "A site with a linked ticket and an uncommitted edit", not "a site". Say how to reach it if it is not the state the app opens in. - **Numbered steps naming what to click**, in the words on screen. -- **The expected result after each step that has one**, stated so it can come out false. "The patch - contains only `wp-login.php`" — not "the patch looks right". -- **What must NOT have happened.** Most of this project's regressions are silent: work quietly - discarded, `node_modules` quietly rebuilt, a patch quietly missing a file. Name the thing that - would be easy not to notice. -- **The platforms it needs.** Default to "any" and say so; call out macOS or Windows explicitly when - the change touches paths, spawning, line endings, or signing. Buildkite builds signed artifacts - for every branch with an open PR, so a reviewer can test on a real machine without building — - check the build matches the current head commit, since force-pushing invalidates earlier ones. -- **What cannot be tested by hand, and why.** An honest "the mid-switch recovery needs a checkout to - fail part-way, which I could not stage" is worth more than silence. - -If a change genuinely has no user-visible surface — a refactor, a CI fix — say that, and give the -command that demonstrates it instead. The section is never simply absent. - -For the human-facing version of all this — the CI checks each PR runs and the guardrails in prose — -see [`CONTRIBUTING.md`](CONTRIBUTING.md). It points back here; it does not restate the standard. +- **The expected result after each step that has one**, stated so it can come out false. "The patch contains only `wp-login.php`" — not "the patch looks right". +- **What must NOT have happened.** Most of this project's regressions are silent: work quietly discarded, `node_modules` quietly rebuilt, a patch quietly missing a file. Name the thing that would be easy not to notice. +- **The platforms it needs.** Default to "any" and say so; call out macOS or Windows explicitly when the change touches paths, spawning, line endings, or signing. Buildkite builds signed artifacts for every branch with an open PR, so a reviewer can test on a real machine without building — check the build matches the current head commit, since force-pushing invalidates earlier ones. +- **What cannot be tested by hand, and why.** An honest "the mid-switch recovery needs a checkout to fail part-way, which I could not stage" is worth more than silence. + +If a change genuinely has no user-visible surface — a refactor, a CI fix — say that, and give the command that demonstrates it instead. The section is never simply absent. + +For the human-facing version of all this — the CI checks each PR runs and the guardrails in prose — see [`CONTRIBUTING.md`](CONTRIBUTING.md). It points back here; it does not restate the standard. + +## Markdown in this repository + +**Do not hard-wrap prose. One paragraph is one line, however long.** Markdown imposes no line limit — a wrapped paragraph and a single long line render identically — so the wrapping is a convention, and this repository's convention is not to. Every `.md` here follows it: `README.md`, `docs/`, this file, `TESTING.md`, `CONTRIBUTING.md`, the review instructions and the templates. + +The reasons are editing, not rendering. A long line reflows to whatever width the reader's editor or screen has, it pastes into an issue or a chat without arriving pre-chopped, and a one-word edit does not reshuffle the six lines beneath it into a diff that claims they changed. The cost is a diff that marks the whole paragraph as changed; `git diff --word-diff`, and GitHub's own intra-line highlighting, both narrow that back down to the words. + +Line breaks still mean something everywhere else, and none of this touches them: list items, table rows, headings, fenced code, VitePress `:::` containers, YAML frontmatter and the body of an HTML comment all keep the shape they have. ## Commands -See `package.json` scripts. To run a single test file (not exposed as a script): -`node --test test/azure-sign.test.cjs`. +See `package.json` scripts. To run a single test file (not exposed as a script): `node --test tests/unit/azure-sign.test.cjs`. + +**[TESTING.md](TESTING.md) is the canonical description of the suite** — the five layers it is made of, which one a new test belongs in, what each layer is blind to, and how to read a failure. Read it before adding or moving a test; do not restate it elsewhere. + +When writing manual test instructions, inspect the current renderer flow first and distinguish actions that happen automatically after linking a ticket from controls used only to retry or refresh them. Do not tell a tester to click a control when the app already starts that operation. + +When asked to add an existing pull request to an existing stack, preserve its commits and change its base to the head branch of the current top PR. Do not move commits into an earlier PR unless the user explicitly asks to rewrite the stack. ## Architecture notes (non-obvious) -- **Child processes run on Electron's own Node, not the system Node.** `npm install`, - `npm run + + + + diff --git a/docs/.vitepress/theme/Layout.vue b/docs/.vitepress/theme/Layout.vue new file mode 100644 index 0000000..19252d2 --- /dev/null +++ b/docs/.vitepress/theme/Layout.vue @@ -0,0 +1,24 @@ + + + diff --git a/docs/.vitepress/theme/index.js b/docs/.vitepress/theme/index.js new file mode 100644 index 0000000..b0a062a --- /dev/null +++ b/docs/.vitepress/theme/index.js @@ -0,0 +1,14 @@ +// The default theme, extended for : a Layout wrapper that +// puts it in the home hero, and a global registration so markdown pages (the +// install section of the guide) can use it too. +import DefaultTheme from 'vitepress/theme'; +import Layout from './Layout.vue'; +import DownloadButton from './DownloadButton.vue'; + +export default { + extends: DefaultTheme, + Layout, + enhanceApp( { app } ) { + app.component( 'DownloadButton', DownloadButton ); + }, +}; diff --git a/docs/.vitepress/theme/match-platform.mjs b/docs/.vitepress/theme/match-platform.mjs new file mode 100644 index 0000000..823261c --- /dev/null +++ b/docs/.vitepress/theme/match-platform.mjs @@ -0,0 +1,52 @@ +// The user-agent → platform decision behind , kept out of the +// component so the root `node --test` suite can reach it (the docs package has +// no test harness of its own). ESM rather than .cjs because the browser loads +// it as-is through Vite; the test imports it dynamically. +// +// `assetPattern` is matched against the release asset names produced by the +// artifactName pattern in the root package.json. The arch token is matched +// loosely on purpose: electron-builder maps `x64` to `x86_64` for some targets +// (AppImage among them), so an exact `-x64` match is one rebuild away from +// silently missing every Linux asset. macOS stays pinned to `arm64` — the app +// has no Intel build, and the label promises Apple Silicon. +export const PLATFORMS = [ + { + id: 'windows', + test: /Windows/, + assetPattern: /-win-[^.]+\.exe$/, + label: 'Windows', + }, + { + id: 'mac', + // "Macintosh", not "Mac": every iOS user agent contains "like Mac OS X", + // and an iPhone cannot open a .dmg. (iPadOS in desktop mode presents + // itself as a Mac and is genuinely indistinguishable.) + test: /Macintosh/, + assetPattern: /-mac-arm64\.dmg$/, + label: 'macOS (Apple Silicon)', + }, + { + id: 'linux', + test: /Linux|X11/, + assetPattern: /-linux-[^.]+\.AppImage$/, + label: 'Linux', + }, +]; + +/** + * Pick the download platform for a browser user-agent string, or null when + * there is no build for it (mobile, unknown OS) and the caller should fall + * back to the Releases page. + * + * @param {string} ua `navigator.userAgent`. + * @return {?{id: string, test: RegExp, assetPattern: RegExp, label: string}} + * The matched platform entry. + */ +export function matchPlatform( ua ) { + // Android UAs contain "Linux" and iOS UAs contain "like Mac OS X"; there + // is no build for either. + if ( /Android|iPhone|iPad|iPod/.test( ua ) ) { + return null; + } + return PLATFORMS.find( ( platform ) => platform.test.test( ua ) ) ?? null; +} diff --git a/docs/create-patch.png b/docs/create-patch.png deleted file mode 100644 index 5ac4580..0000000 Binary files a/docs/create-patch.png and /dev/null differ diff --git a/docs/guide/applying-patches.md b/docs/guide/applying-patches.md index 8562a63..8f6f165 100644 --- a/docs/guide/applying-patches.md +++ b/docs/guide/applying-patches.md @@ -17,7 +17,7 @@ There are three ways to get a patch into the panel: Nothing is changed yet. The panel first shows what the patch would do: - The list of files it changes. -- A warning if you have your own edits to any of those files. The patch is applied on top of them: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy — see [Submitting your changes](submitting-changes). +- A warning if this ticket already has work in any of those files, measured from the trunk snapshot the ticket started on. The warning names your own edits and changes from an applied patch separately; a file from an applied patch may contain your edits too. The new patch is applied on top of that work: it succeeds if the changes do not overlap, and fails without touching anything if they do. Save a patch of your work first if you want a copy — see [Submitting your changes](submitting-changes). - Which files are binary and will be skipped. - Whether it changes `package-lock.json`, in which case dependencies will be installed before the rebuild. @@ -27,19 +27,54 @@ Click **Apply and rebuild** to go ahead, or **Cancel** to back out. The panel shows each step as it runs: applying the patch, installing dependencies if needed, and rebuilding. When it finishes, the panel reports what is applied — the patch's name, how many files it changed, and when. -If a step fails, the error says what went wrong and the checkout was not changed. +If the [build watch](running-the-site#the-build-watch) is running and the patch does not move `package-lock.json`, there is no build step: the patch is applied and left for the watch to compile, and the checklist says so. A patch that does move the lockfile has to install and build, so it pauses the watch for the duration and resumes it after — the dev server stays up throughout. + +## When a patch will not apply + +The apply is all-or-nothing. If anything fails, nothing is written to your checkout — but the panel now tells you *how much* failed, because one region of twenty missing and all twenty missing are opposite decisions for you. + +![A pull request that does not fit this checkout, with the affected file named and confirmation that the checkout was not changed](/screenshots/apply-patch-conflict.png) + +The headline is a count, not an adjective: *4 of this patch's 20 changes across 3 files no longer fit — the other 16 do.* When every change is already in your checkout — which is what a patch that has since been committed to core looks like — it says that instead, rather than reporting the patch as dead. + +### For a patch file or a Trac attachment + +You get the full breakdown, because you are the only one who can rescue it. Each file lists its failing regions, and each region says **why**: + +- **the code around it has changed** — the patch was written against an older trunk and the lines it expected have moved. +- **looks like it is already in your checkout** — that change is present. The app tells the two apart by testing whether the region's reverse fits. + +Every region carries an **anchor line taken from your own file** to search for. A hunk's line numbers are coordinates in the file as its author had it, so on an old patch they miss by exactly the drift that made it fail; a line you can search for does not. The first few regions of each file also show the lines the patch wanted to add and remove — enough to recognise the change without turning the panel into the diff itself. + +### For a pull request + +The panel first separates two situations that need different next steps. + +If the failures are only in files this ticket has not changed, the pull request was written against an older trunk. The notice names the situation and its scale — *this pull request was written against an older trunk and no longer fits it: 4 of its 20 changes, in 3 files, would need rework* — without the line-level detail. Bringing it up to date is its author's work, so the useful contribution is to leave a comment asking for a rebase or for trunk to be merged in. + +If your ticket already has work in a failing file, the app does not blame the pull request's author. A file-level overlap cannot prove which exact lines caused the failure, so the notice says your work *may* be involved. Save a patch of your work, try the pull request on a clean ticket, and ask its author to update it only if it still fails there. When the file includes changes from a patch you already applied, the notice names that patch too rather than calling all of the file your own writing. + +A **closed** pull request is read differently, because on `wordpress-develop` "closed" is also what landing looks like — core commits go through SVN and the pull request is closed, never merged. If all its changes read back as already in trunk, the panel says it was likely committed to core and there is nothing left to apply. Otherwise it says nobody is coming back to update it, and offers **See why it was closed**. + +### The way out + +When the ticket has other patches on it — another pull request, another attachment — the panel offers them. It only does so when there is genuinely one to try: a way out that lands you back at the same dead end costs a click to discover. ## Applied patches belong to a ticket -What is applied is recorded against the ticket you are on, not against the site. Switch to another ticket and the green "applied" box goes with the first one; switch back and it is there again, describing the patch you actually applied on that ticket. See [Working on several tickets](ticket-branches). +What is applied is recorded as a named layer on the ticket you are on, separate from your own edits. Switch to another ticket and the green "applied" box goes with the first one; switch back and it is there again, naming the patch, how many files it changed, and when it was applied. The preview and failure notices continue to distinguish that layer from your writing. + +A ticket holds one applied patch or pull request at a time. Revert it, or discard the ticket back to its base, before applying another. See [Working on several tickets](ticket-branches). ## Reverting an applied patch -While a patch is applied, the panel shows it in a green box with a **Revert this patch** button. Reverting removes the patch's changes and rebuilds, again leaving your own edits alone. +While the saved patch can still be removed cleanly, the panel shows it in a green box with a **Revert this patch** button. Reverting removes the patch's changes and rebuilds, again leaving your own edits alone. + +If you edit lines the patch brought in, it can no longer be lifted back out without also disturbing your work. A failed Revert changes the explanation accordingly: the patch is part of your changes now. Undo your edits on the named regions to make **Revert this patch** work again, or **Save a copy of your work** and **Discard this ticket to its base**. -Very large patches cannot be undone automatically. The panel says so; use **Update to latest trunk** to reset the checkout instead — see [Staying up to date with trunk](trunk-updates). +For very large patches, the app does not keep the copy it would need for an undo, so they never offer Revert. The amber box says so and offers the same copy-and-discard route. Until the ticket is reverted or discarded, the patch still occupies its one applied-patch slot. -That escape hatch only resets a site sitting on trunk. On a ticket, an update parks your branch before it resets trunk and checks the branch back out afterwards — applied patch and all — so it leaves you where you were. On a ticket, the way to be rid of both the patch and the work under it is [deleting that ticket's work](ticket-branches). +**Update to latest trunk** is not an escape hatch for a patch on a ticket. It parks the ticket branch, updates trunk, and checks the same branch back out afterwards — applied patch and all — so it leaves you where you were. See [Staying up to date with trunk](trunk-updates). ## Your own changes diff --git a/docs/guide/creating-a-site.md b/docs/guide/creating-a-site.md index 8b6ab7e..dc263be 100644 --- a/docs/guide/creating-a-site.md +++ b/docs/guide/creating-a-site.md @@ -25,11 +25,9 @@ The app clones the `wordpress-develop` repository from GitHub. Git is bundled wi While the clone runs, the site view shows a **Setting up new site…** card with the current phase and a terminal panel streaming progress output. The clone downloads the full repository, so expect it to take several minutes depending on your connection. -The clone is the first step of the [initial setup checklist](./setup-wizard); the remaining steps (installing dependencies, building, starting the dev server) stay locked until it finishes, then you drive them yourself. +The clone is the first step of the [initial setup checklist](./setup-wizard); the remaining steps stay locked until it finishes. Then the app carries on by itself — installing the dependencies and running the first build without waiting for you — so the only step left to click is starting the dev server. -If setup fails, the half-created site is removed from the list — the row simply disappears. The -reason goes to the application log rather than to a dialog, so **Help → Open App Log** is where -to look when a site never finishes. +If setup fails, the half-created site is removed from the list — the row simply disappears. The reason goes to the application log rather than to a dialog, so **Help → Open App Log** is where to look when a site never finishes. ## Where sites live on disk diff --git a/docs/guide/database.md b/docs/guide/database.md index ef9e016..3132018 100644 --- a/docs/guide/database.md +++ b/docs/guide/database.md @@ -1,39 +1,25 @@ # Browsing the database -The dev server runs WordPress on SQLite, and the app bundles [Adminer](https://www.adminer.org/) -so you can look inside that database from your browser. +The dev server runs WordPress on SQLite, and the app bundles [Adminer](https://www.adminer.org/) so you can look inside that database from your browser. -## Open Adminer +## Opening Adminer -While the dev server is running, an **Open Adminer** button appears next to the server URL in the -site view. Clicking it opens Adminer in your browser, already logged into the site's SQLite -database — no credentials to enter. From there you can browse tables, inspect rows, and run SQL, -the same way you would against a MySQL-backed install. +While the dev server is running, a **DB inspect (Adminer)** link appears beside the server URL in the site view, after **wp-admin**. Clicking it opens Adminer in your browser, already logged into the site's SQLite database — no credentials to enter. From there you can browse tables, inspect rows, and run SQL, the same way you would against a MySQL-backed install. -The database file itself lives inside the Playground environment at -`/wordpress/wp-content/database/.ht.sqlite`. It is not a file you can open directly on disk; go -through Adminer. +The database file itself lives inside the Playground environment at `/wordpress/wp-content/database/.ht.sqlite`. It is not a file you can open directly on disk; go through Adminer. -Adminer is only reachable while the dev server runs — the button disappears when the server -stops, and so does the page it opened. +Adminer is only reachable while the dev server runs — the link disappears when the server stops, and so does the page it opened. ## Is SQLite enough for core development? -For most new contributors, yes. WordPress's SQLite support has matured considerably: most plugins -and most core unit tests work, and thanks to the query parser, remaining gaps are tracked and -steadily closed. +For most new contributors, yes. WordPress's SQLite support has matured considerably: most plugins and most core unit tests work, and thanks to the query parser, remaining gaps are tracked and steadily closed. It is not a complete substitute for MySQL, though. Be aware of the limits: -- Some queries and features behave differently on SQLite than on MySQL, and a small number do not - work at all. -- If your contribution touches the database layer itself — `wpdb`, schema changes, MySQL-specific - SQL — you should verify it against a real MySQL install before submitting. The toolkit cannot - do that for you: WordPress Playground can work with MySQL, but this app does not ship a MySQL - server. +- Some queries and features behave differently on SQLite than on MySQL, and a small number do not work at all. +- If your contribution touches the database layer itself — `wpdb`, schema changes, MySQL-specific SQL — you should verify it against a real MySQL install before submitting. The toolkit cannot do that for you: WordPress Playground can work with MySQL, but this app does not ship a MySQL server. -For the typical first contribution — a fix in PHP, JavaScript, or CSS that reads and writes -ordinary posts, options, and users — the difference will not affect you. +For the typical first contribution — a fix in PHP, JavaScript, or CSS that reads and writes ordinary posts, options, and users — the difference will not affect you. ## Related pages diff --git a/docs/guide/editors.md b/docs/guide/editors.md index f135c14..a39e99b 100644 --- a/docs/guide/editors.md +++ b/docs/guide/editors.md @@ -37,4 +37,4 @@ If an application fails to open the directory — it was uninstalled, or the lau ## What to edit -WordPress source lives under `src/` in the site directory. While the dev server is running, a watcher rebuilds your edits into `build/` automatically — see [Running the site](./running-the-site). When your change is ready, see [Submitting changes](./submitting-changes). +WordPress source lives under `src/` in the site directory. While the [build watch](./running-the-site#the-build-watch) is running, your edits are rebuilt into `build/` as you save them. It runs on its own control, so it does not need the dev server up — though starting the server starts it too. When your change is ready, see [Submitting changes](./submitting-changes). diff --git a/docs/guide/getting-started.md b/docs/guide/getting-started.md index b7f77ac..57730e4 100644 --- a/docs/guide/getting-started.md +++ b/docs/guide/getting-started.md @@ -10,13 +10,17 @@ You install it, choose a directory for `wordpress-develop`, click a button, and No Git, no Node.js, no npm, no Docker needed. Everything is bundled inside the application as JavaScript/WASM, powered by [WordPress Playground](https://wordpress.github.io/wordpress-playground/). +::: tip Prepare at home before Contributor Day +Creating your first site downloads the full `wordpress-develop` repository, installs its dependencies, and runs the first build. That means downloading a lot of files, so we recommend having at least one site fully set up a day or a few days before the event. + +On Contributor Day, open that site and run [**Update to latest trunk**](./trunk-updates). The app fetches the changes made since your initial setup and only reinstalls dependencies if they changed. Most packages will already be cached, which uses far less of the venue's shared bandwidth than starting from scratch. +::: + ## Install the app -1. Download the latest packaged build for your platform from the [Releases page](https://github.com/WordPress/contributor-toolkit/releases/latest). Pick the file that matches your OS: - - **macOS on Apple Silicon:** the `.dmg` file whose name contains `arm64`. Intel Macs are not currently supported. - - **Windows:** the `.exe` installer. - - **Linux:** the `.AppImage`; `.deb` and `.snap` packages may also be available. -2. Open the app. + + +Download the latest build for your platform with the button above, then open the app. ### If macOS blocks the app @@ -33,20 +37,23 @@ The app is signed and notarized by Automattic, so macOS should open it without i The app is code-signed. The recursive flag (`-r`) tries to strip attributes from files inside the sealed bundle, which macOS rejects with permission errors. Removing the attribute from the top-level bundle is sufficient. ::: -## Your first contribution, in ten steps +## Your first contribution, in seven steps 1. Click **Create WordPress Core site** and choose a destination folder for your site. -2. Wait while the app downloads `wordpress-develop`. -3. Click **Install npm dependencies**. -4. Click **Run full build**. -5. Click **Start dev server**. -6. A browser window opens automatically. If not, open it by clicking the site URL. -7. Make changes to the code — [open the site in your editor](./editors) straight from the app. -8. Click **Review & submit changes** to see a diff of everything you changed. -9. Pick where the patch goes: [a pull request](./submit-github-pr), [a Trac ticket](./submit-trac), or [a file for your mentor](./submit-mentor). -10. That's it — you've contributed to WordPress core. - -![A site ready for work: Start dev server, Review & submit changes, the Trac ticket panel and the patch panel](/screenshots/site-view.png) +2. Wait. The app downloads `wordpress-develop`, then installs the dependencies and runs the first build on its own — go and get a coffee. See [The setup wizard](./setup-wizard). +3. Click **Start dev server**. +4. A browser window opens automatically. If not, open it by clicking the site URL — or the **wp-admin** link beside it, to go straight to the dashboard. +5. Make changes to the code — [open the site in your editor](./editors) straight from the app. +6. Click **Review & submit changes** to see a diff of everything you changed. +7. Pick where the patch goes: [a pull request](./submit-github-pr), [a Trac ticket](./submit-trac), or [a file for your mentor](./submit-mentor). + +That's it — you've contributed to WordPress core. + +::: tip You are not left guessing what to do next +When a site has something pending — a setup step to run, a warning to act on, work in flight — that one block is ringed in amber and scrolls itself into view, and is announced to screen readers as *Next step: …*. A site that is set up, running and clean has nothing pending, so nothing is ringed. And when an action finishes — a patch saved, trunk updated, a pull request opened — a brief notice says so out loud rather than leaving you to check. +::: + +![A site ready for work: Start dev server, Start build watch, Review & submit changes, the Trac ticket panel and the patch panel](/screenshots/site-view.png) The rest of this guide walks through each of these screens in detail, starting with [creating a site](./creating-a-site). diff --git a/docs/guide/logs-and-debugging.md b/docs/guide/logs-and-debugging.md index 899bd69..5c4ae01 100644 --- a/docs/guide/logs-and-debugging.md +++ b/docs/guide/logs-and-debugging.md @@ -1,21 +1,20 @@ # Logs and debugging -The **Logs** section of the site view has two tabs: +The **Logs** section of the site view has three tabs: -- **Server** — the dev server's own output: everything the Playground server prints while - starting and running. +- **Server** — the dev server's own output: everything the Playground server prints while starting and running. +- **Build watcher** — the [build watch](./running-the-site#the-build-watch)'s output. The tab title carries its state while it is doing something: *Build watcher (watching)*, *(building)*, *(paused)*, or *(exited 1)* when it stopped on its own. Stop it yourself and the title goes back to a plain *Build watcher*. - **debug.log** — WordPress's PHP log for this site, streamed live while the dev server runs. Output from `npm install` and `npm run` commands appears in the [Terminal](./terminal), not here. +All three panes read in the terminal's own monospace font, so the columns of a PHP stack trace line up, and each line is coloured by what it is: a fatal, a warning, a deprecation, a notice, a stack trace frame, or the `Ready! WordPress is running on …` line you are actually waiting for. The `[11-Aug-2026 …]` timestamp at the head of a `debug.log` line is dimmed, so 26 identical characters per line recede instead of competing with the message. + ![The debug.log tab showing PHP notices](/screenshots/debug-log.png) ## The debug.log tab -Anything WordPress or your code writes to the PHP error log — `error_log()` calls, notices, -warnings, deprecations, `_doing_it_wrong()`, fatals — appears here while the dev server runs. -This works because every site is booted with WordPress's debug constants already set. They are -not configurable: +Anything WordPress or your code writes to the PHP error log — `error_log()` calls, notices, warnings, deprecations, `_doing_it_wrong()`, fatals — appears here while the dev server runs. This works because every site is booted with WordPress's debug constants already set. They are not configurable: | Constant | Value | Effect | | --- | --- | --- | @@ -26,27 +25,17 @@ not configurable: | `WP_DISABLE_FATAL_ERROR_HANDLER` | `true` | A fatal shows the actual error instead of WordPress's "critical error" recovery screen. | | `AUTOMATIC_UPDATER_DISABLED` | `true` | Core's automatic updater does not run (and does not fill the log with its own messages). | -Note that `WP_DEBUG_DISPLAY` has a known cost: a notice fired during a REST or AJAX request is -printed into the response and can corrupt the JSON it expects. That trade is made deliberately — -seeing the error beats a silent blank page for a newcomer. +Note that `WP_DEBUG_DISPLAY` has a known cost: a notice fired during a REST or AJAX request is printed into the response and can corrupt the JSON it expects. That trade is made deliberately — seeing the error beats a silent blank page for a newcomer. -While you are reading another tab, the **debug.log** tab shows an unread count, for example -**debug.log (3)**, so a notice landing while you watch the server output does not go unseen. -Selecting the tab resets the count. The panel keeps the most recent 512 KB of the log. +While you are reading another tab, the **debug.log** tab shows an unread count, for example **debug.log (3)**, so a notice landing while you watch the server output does not go unseen. Selecting the tab resets the count. The panel keeps the most recent 512 KB of the log. Under the pane: -- The full path to the log file (inside the site's `build/wp-content/` directory) is shown and can - be selected and copied — useful for tailing it in a real terminal or attaching it to a ticket. +- The full path to the log file (inside the site's `build/wp-content/` directory) is shown and can be selected and copied — useful for tailing it in a real terminal or attaching it to a ticket. - **Show in folder** reveals the file in your file manager. -- **Copy** puts the panel's contents on the clipboard, ready to paste into a Trac ticket or a pull - request comment. -- **Clear** empties both the panel and the file on disk. If the file cannot be cleared, the panel - says so — otherwise the same lines would replay the next time the server starts. +- **Copy** puts the panel's contents on the clipboard, ready to paste into a Trac ticket or a pull request comment. +- **Clear** empties both the panel and the file on disk. If the file cannot be cleared, the panel says so — otherwise the same lines would replay the next time the server starts. ## The app's own log -The application keeps a separate log of its own activity — server starts, installs, errors. Do not -confuse it with the site's debug.log. Open it from the menu: **Help → Open App Log**, or -**Help → Show Logs Folder** to reveal the directory. This is the file to attach when -[reporting a problem with the app itself](./troubleshooting). +The application keeps a separate log of its own activity — server starts, installs, errors. Do not confuse it with the site's debug.log. Open it from the menu: **Help → Open App Log**, or **Help → Show Logs Folder** to reveal the directory. This is the file to attach when [reporting a problem with the app itself](./troubleshooting). diff --git a/docs/guide/mail.md b/docs/guide/mail.md index 3a87381..7ce748e 100644 --- a/docs/guide/mail.md +++ b/docs/guide/mail.md @@ -1,38 +1,24 @@ # The Mail panel -WordPress sends email constantly — new user notifications, password resets, comment moderation. -On a development site none of that should reach a real inbox, and with this app none of it can: -each site gets its own built-in SMTP catcher, listening only on `127.0.0.1`. WordPress's -`wp_mail()` is pointed at it, so every email the site sends lands in the **Mail** panel instead -of leaving your machine. +WordPress sends email constantly — new user notifications, password resets, comment moderation. On a development site none of that should reach a real inbox, and with this app none of it can: each site gets its own built-in SMTP catcher, listening only on `127.0.0.1`. WordPress's `wp_mail()` is pointed at it, so every email the site sends lands in the **Mail** panel instead of leaving your machine. -![The Mail panel with a captured email](/screenshots/mail-panel.png) +![A captured email with its time, sender, and subject](/screenshots/mail-panel.png) ## How it works -The SMTP catcher starts and stops together with the dev server. While the server is running, the -panel shows the address it listens on, for example `SMTP listening on 127.0.0.1:54321` — the port -is assigned by the operating system, so it varies. Before the server has started, the panel says -`SMTP will start with the dev server.` +The SMTP catcher starts and stops together with the dev server. While the server is running, the panel shows the address it listens on, for example `SMTP listening on 127.0.0.1:54321` — the port is assigned by the operating system, so it varies. Before the server has started, the panel says `SMTP will start with the dev server.` -No configuration is needed on the WordPress side: the app installs a small must-use plugin in the -Playground environment that routes `wp_mail()` through SMTP to the catcher. +No configuration is needed on the WordPress side: the app installs a small must-use plugin in the Playground environment that routes `wp_mail()` through SMTP to the catcher. ## Reading emails -Captured emails appear in a list showing the date, the sender, and the subject. Click an email -(or press Enter on it) to open it in a full-screen viewer with the **From**, **To**, **CC**, and -**Date** headers and two tabs: +Captured emails appear in a list showing the date, the sender, and the subject. Click an email (or press Enter on it) to open it in a full-screen viewer with the **From**, **To**, **CC**, and **Date** headers and two tabs: -- **Rendered** — the HTML body as a mail client would show it, or the plain-text body if the - email has no HTML part. -- **Raw** — the raw message source, headers and all. Useful when the bug you are chasing is in - how core builds the email itself. +- **Rendered** — the HTML body as a mail client would show it, or the plain-text body if the email has no HTML part. +- **Raw** — the raw message source, headers and all. Useful when the bug you are chasing is in how core builds the email itself. **Clear emails** deletes all captured emails for the site. ## Trying it out -Trigger any email-sending flow on the dev site — the password reset form at `/wp-login.php?action=lostpassword` -is the quickest — and the email appears in the panel. See [Running the site](./running-the-site) -for starting the server. +Trigger any email-sending flow on the dev site — the password reset form at `/wp-login.php?action=lostpassword` is the quickest — and the email appears in the panel. See [Running the site](./running-the-site) for starting the server. diff --git a/docs/guide/managing-sites.md b/docs/guide/managing-sites.md index e953c1e..7742104 100644 --- a/docs/guide/managing-sites.md +++ b/docs/guide/managing-sites.md @@ -1,55 +1,35 @@ # Managing sites -Everything about a site's identity and lifecycle lives in its header: the name, a few status -badges, the path, and the **More** menu (☰) at the top right. +Everything about a site's identity and lifecycle lives in its header: the name, a few status badges, the path, and the **More** menu (☰) at the top right. ![The site header with the More menu open](/screenshots/site-menu.png) ## The site header -- **Rename** — click the pencil icon next to the site title to give the site a different display - name. This changes the label in the app only; the directory on disk keeps its name. -- **Status badge** — **INITIALIZED** once the repository is cloned; **UNINITIALIZED** before - that. +- **Rename** — click the pencil icon next to the site title to give the site a different display name. This changes the label in the app only; the directory on disk keeps its name. +- **Status badge** — **INITIALIZED** once the repository is cloned; **UNINITIALIZED** before that. - **Created date** — when the site was created. -- **Trunk age** — how old the site's snapshot of `wordpress-develop` trunk is. When the snapshot - gets stale, an amber dot appears next to the label; hover it to see the age in days. See - [Staying up to date with trunk](./trunk-updates). -- **Path** — the site's directory, with a copy button next to it. The **Open directory in** menu - underneath opens it in your file manager or an editor; see [Editors](./editors). +- **Trunk age** — how old the site's snapshot of `wordpress-develop` trunk is. When the snapshot gets stale, an amber dot appears next to the label; hover it to see the age in days. See [Staying up to date with trunk](./trunk-updates). +- **Path** — the site's directory, with a copy button next to it. The **Open directory in** menu underneath opens it in your file manager or an editor; see [Editors](./editors). ## The More menu The ☰ menu at the top right of the site view contains: - **Copy path** — puts the site's directory path on the clipboard. -- **Show in Finder** (macOS) / **Show in Explorer** (Windows) — reveals the directory in - your file manager. -- **Update to latest trunk** — fetches the latest `wordpress-develop` trunk and rebuilds. Also - reachable from the staleness notice; on an already-fresh site it just prints "Already up to - date." See [Staying up to date with trunk](./trunk-updates). -- **Delete this site** — removes the site from the list **and deletes its directory from disk**. - This cannot be undone. The app will only ever delete a directory it has on record as a site — - never an arbitrary path. It takes every ticket's work in the site with it; to throw away one - ticket and keep the rest, use **Delete this ticket's work** on the tickets card instead — see - [Working on several tickets](./ticket-branches#deleting-a-ticket-s-work). +- **Show in Finder** (macOS) / **Show in Explorer** (Windows) — reveals the directory in your file manager. +- **Update to latest trunk** — fetches the latest `wordpress-develop` trunk and rebuilds. Also reachable from the staleness notice; on an already-fresh site it just prints "Already up to date." See [Staying up to date with trunk](./trunk-updates). +- **Delete this site** — removes the site from the list **and deletes its directory from disk**. This cannot be undone. The app will only ever delete a directory it has on record as a site — never an arbitrary path. It takes every ticket's work in the site with it; to throw away one ticket and keep the rest, use **Delete this ticket's work** on the tickets card instead — see [Working on several tickets](./ticket-branches#deleting-a-ticket-s-work). **Delete** asks for confirmation first. ## Updating with uncommitted changes -If you start **Update to latest trunk** while the site has edits loose in the working tree, the app -does not silently throw them away. A dialog titled **Update to latest trunk?** lists every changed -file and offers two choices: +If you start **Update to latest trunk** while the site has edits loose in the working tree, the app does not silently throw them away. A dialog titled **Update to latest trunk?** lists every changed file and offers two choices: -- **Save them as a patch first (as a local file)** — writes a `.diff` to your machine, then - updates. Nothing is sent to Trac. +- **Save them as a patch first (as a local file)** — writes a `.diff` to your machine, then updates. Nothing is sent to Trac. - **Discard them** — your changes are lost; this cannot be undone. -Confirm with **Save patch & update** or **Discard & update**, or **Cancel** to keep everything as -it is. If you meant to keep the changes as a contribution instead, see -[Submitting your changes](./submitting-changes). +Confirm with **Save patch & update** or **Discard & update**, or **Cancel** to keep everything as it is. If you meant to keep the changes as a contribution instead, see [Submitting your changes](./submitting-changes). -Work that is already parked on a ticket branch is never what this dialog is offering to discard — -the update carries it across untouched. See -[Updating while you are on a ticket](./trunk-updates#updating-while-you-are-on-a-ticket). +Work that is already parked on a ticket branch is never what this dialog is offering to discard — the update carries it across untouched. See [Updating while you are on a ticket](./trunk-updates#updating-while-you-are-on-a-ticket). diff --git a/docs/guide/running-the-site.md b/docs/guide/running-the-site.md index 9fb6e5e..879600b 100644 --- a/docs/guide/running-the-site.md +++ b/docs/guide/running-the-site.md @@ -2,7 +2,7 @@ Once the [setup wizard](./setup-wizard) is complete, the site view shows a **Start dev server** button. This starts a local WordPress that serves the code in your site's `build/` directory, so you can see your changes running. -![The site view with Start dev server and Review & submit changes](/screenshots/site-view.png) +![The site view with Start dev server, Start build watch and Review & submit changes](/screenshots/site-view.png) ## Start and stop @@ -12,9 +12,11 @@ Click **Start dev server**. The button cycles through three states: - **Starting dev server...** — the server is booting. A "Dev server is starting…" line below the button shows the elapsed time. - **Stop dev server** — the server is up. The button shows a red dot; clicking it stops the server. -When the server is ready, its URL appears below the button, for example `http://127.0.0.1:/`. Click it to open the site in your default browser. +When the server is ready, its URL appears below the button, for example `http://127.0.0.1:/`, with a **wp-admin** link beside it. Click either to open the site in your default browser. -The button is disabled while a trunk update is running, and a trunk update is blocked while the server runs — the two would fight over the same files. +![The site view with the dev server running: the site URL, the wp-admin link and the admin / password credentials below the buttons](/screenshots/dev-server-running.png) + +The button is disabled while a trunk update is running. The reverse is no longer true: an update can run with the server up, because it pauses the build watch for the rebuild and leaves the server serving — see [Applying patches and PRs](./applying-patches) and [Staying up to date with trunk](./trunk-updates). ## Logging in @@ -23,7 +25,7 @@ Every site uses the same credentials, shown next to the URL: - Username: `admin` - Password: `password` -Append `/wp-admin/` to the site URL to reach the dashboard. +The **wp-admin** link next to the site URL opens the dashboard directly. It appears in both places the URL does: the [setup checklist](./setup-wizard) step and the site page. ## What the dev server actually runs @@ -31,16 +33,37 @@ The dev server is not a stub — it is WordPress Playground running your checkou - The app spawns the [Playground CLI](https://wordpress.github.io/wordpress-playground/) (`@wp-playground/cli`) in server mode, with your site's `build/` directory mounted as the WordPress root. PHP runs as WebAssembly inside the bundled Node.js runtime, so no PHP install is needed. - The database is **SQLite**, stored inside the Playground instance. This covers most core contribution work; if a ticket specifically needs MySQL behaviour, this environment cannot reproduce it. -- Alongside the server, the app runs the core build watcher, so edits under `src/` are rebuilt into `build/` while the server is up. A page reload then shows the change. - The server binds to the loopback interface only. It is reachable from your machine, not from the rest of your network. - Outgoing mail is captured locally instead of being sent — see [Mail](./mail). -## Open Adminer +## The build watch + +The build watcher compiles what you edit under `src/` into `build/`, which is what the server actually serves. It has its own **Start build watch** button next to the dev-server button, and its own status dot: + +| Dot | State | +| --- | --- | +| Green | Watching. It stays green while it recompiles a save — that is its normal working state. | +| Amber | Either the one-off full build that runs before the watch can start on a site that has never been built, or the watch paused while another action owns the build directory. | +| Red | It exited on its own. Read the **Build watcher** log tab to find out why. | +| Grey | Stopped. | + +The watch and the server are independent in both directions: + +- Starting the dev server starts the watch first (building once if the site has no `build/` yet), then serves. +- Stopping the dev server leaves the watch running, so you can keep compile-on-save going without a server. +- The watch exiting never touches the server. +- You can start and stop the watch on its own, at any time, whether or not a server is up. + +While the watch is running, applying a patch or updating trunk uses it rather than fighting it. A patch that does not move `package-lock.json` is applied and left for the watch to recompile — the checklist shows the build step skipped and names the watch as doing it. One that does move the lockfile has to install and build, so it pauses the watch for the duration and resumes it after, with the PHP server up throughout. + +Its output goes to its own **Build watcher** log tab, not to the terminal, and it no longer holds the terminal's "running" lock — so the terminal and one-shot actions stay available while it runs. + +## DB inspect (Adminer) -While the server is running, an **Open Adminer** button appears next to **Stop dev server**. It opens Adminer, a database browser, against the site's SQLite database — useful for inspecting what a code change wrote. See [Database](./database) for details. +While the server is running, a **DB inspect (Adminer)** link joins the site URL and **wp-admin** on the same row. It opens Adminer, a database browser, against the site's SQLite database — useful for inspecting what a code change wrote. See [Database](./database) for details. -The button disappears when the server stops, because there is no database to connect to. +The link disappears when the server stops, because there is no database to connect to. ## Where the output goes -Everything the server and the watcher print streams into the site's terminal panel, which is also where startup errors land. See [Terminal](./terminal) and [Logs and debugging](./logs-and-debugging). +The server's output goes to the **Server** tab of the Logs section, and the watcher's to its own **Build watcher** tab; startup errors land there too. The terminal panel is for the commands you type. See [Logs and debugging](./logs-and-debugging) and [Terminal](./terminal). diff --git a/docs/guide/setup-wizard.md b/docs/guide/setup-wizard.md index 1b36a4d..4ead190 100644 --- a/docs/guide/setup-wizard.md +++ b/docs/guide/setup-wizard.md @@ -1,8 +1,8 @@ # The setup wizard -After [creating a site](./creating-a-site), its view shows the **Initial setup checklist**. Complete each step to prepare the site for development. The steps run in order — each one unlocks the next. +After [creating a site](./creating-a-site), its view shows the **Initial setup checklist**. It runs itself: when the clone finishes, the install and the build start on their own and run to the end, so a site you walked away from is ready to work on when you come back. The buttons are there for when something fails, or when you stopped the chain yourself. -![The Initial setup checklist showing four steps: download, install npm dependencies, run full build, and start dev server](/screenshots/setup-wizard.png) +![The Initial setup checklist running itself: a banner reading "Setting this site up for you — step 2 of 3" with a Stop setup button, above the four steps — download completed, install in progress, build and start dev server locked](/screenshots/setup-wizard.png) ## The four steps @@ -12,33 +12,40 @@ The clone of `wordpress-develop` that started when you created the site. It runs ### 2. Install npm dependencies -Click **Install npm dependencies** to run `npm install` using the Node.js runtime bundled with the app — no system Node or npm is needed. If the install fails, the button changes to **Retry npm install**; a leftover `node_modules` folder from a failed run does not count as completed. +`npm install`, run on the Node.js runtime bundled with the app — no system Node or npm is needed. It starts by itself when the clone ends, and the terminal says so: *Setting this site up — running npm install…* -Once installed, the button reads **Dependencies installed** and stays disabled. If you later add a dependency to `package.json`, run `npm install` yourself in the [Terminal](./terminal). +If it fails, the chain stops there and the button changes to **Retry npm install**; a leftover `node_modules` folder from a failed run does not count as completed. Once installed, the button reads **Dependencies installed** and stays disabled. If you later add a dependency to `package.json`, run `npm install` yourself in the [Terminal](./terminal). ### 3. Run full build -Click **Run full build** to compile WordPress core and generate the `dist` files the dev server serves. This is the longest step after the clone. +Compiles WordPress core and generates the files the dev server serves. This is the longest step after the clone, and it follows the install without asking. -You only run it manually once: [trunk updates](./trunk-updates) and [applied patches](./applying-patches) rebuild on their own. If you edit files in `src/` by hand, run `npm run build` in the Terminal so the site picks them up. +You never run it manually unless something failed: [trunk updates](./trunk-updates) and [applied patches](./applying-patches) rebuild on their own, and while the [build watch](./running-the-site#the-build-watch) is running your own edits under `src/` are compiled as you save them. ### 4. Start dev server & finish wizard -Click **Start dev server and finish the wizard** to launch the WordPress dev server for the first time. This completes the checklist and permanently replaces it with the compact action bar described below. The server URL appears next to the button once it is up — see [Running the site](./running-the-site). +Click **Start dev server and finish the wizard** to launch the WordPress dev server for the first time. This completes the checklist and permanently replaces it with the compact action bar described below. The server URL, and a **wp-admin** link beside it, appear next to the button once it is up — see [Running the site](./running-the-site). + +## Stopping the chain + +Press **Ctrl+C** in the [Terminal](./terminal) to stop setup, including a running `npm install`. The terminal says where you stand — *Setup stopped. The remaining steps are in the checklist above — run them whenever you are ready* — and the checklist buttons take over from there. The chain names its other endings too: setup complete, the install failed, or the build failed with dependencies already installed. ## Step states Each step carries a state label: - **Completed** — done, marked with a green check. -- **In progress** — the step to do next; its button is enabled. +- **In progress** — this step is running right now. +- **Ready** — the next step to do, waiting on you rather than working. The distinction matters: a step that said "In progress" while nothing was installing invited you to wait instead of to click. - **Pending** — ready but waiting for you to reach it. - **Locked** — its prerequisites are not met yet; the button is disabled. +The step to act on is also ringed in amber and scrolls itself into view, and is announced to screen readers as *Next step: …*. The ring carries no label of its own — the step's own status text names it. It moves when the step it points at is done, not on a timer. + Steps also lock temporarily while a [trunk update](./trunk-updates) is running, since the update owns the working tree during that time. ## Skipping the wizard -Below the checklist is a **Skip initialization wizard** link. Clicking it hides the checklist for this site, for good, and shows the compact action bar instead: the dev server start/stop button, **Review & submit changes**, and — while the server is running — **Open Adminer**. +Below the checklist is a **Skip initialization wizard** link. Clicking it hides the checklist for this site, for good, and shows the compact action bar instead: the dev server start/stop button, the [build watch](./running-the-site#the-build-watch) button, and **Review & submit changes**. While the server is running, the site URL, **wp-admin** and **DB inspect (Adminer)** appear on a row below it. Skipping does not run any of the steps for you. If the dependencies were never installed or the build never ran, the dev server will not have anything to serve, so only skip on a site you know is already set up (for example, a `wordpress-develop` checkout you prepared outside the app). For everything else, finishing step 4 gets you to the same action bar with the work actually done. diff --git a/docs/guide/submit-github-pr.md b/docs/guide/submit-github-pr.md index 3608731..2945e40 100644 --- a/docs/guide/submit-github-pr.md +++ b/docs/guide/submit-github-pr.md @@ -12,6 +12,8 @@ Click **Sign in with GitHub**. The app signs you in through your browser, using 2. Enter the code there — **Copy the code** puts it on your clipboard — and confirm the authorization on GitHub. 3. The app waits until GitHub reports the sign-in went through. +![The Open a pull request card showing a GitHub device code, Copy the code, the waiting state, and Cancel](/screenshots/github-sign-in.png) + You never type a password into the app, and no credential is written to disk: the authorization is held in memory and forgotten when you quit. Click **Cancel** to abandon the sign-in, or **Not now** to decline it — the patch file is still yours to save, and the other two destinations are unchanged. Once signed in, the card says which account you are on and where the fork and branch will go: **your-username/wordpress-develop**. **Sign out** discards the authorization. @@ -31,9 +33,3 @@ Two things a first-timer has no way to know, stated on the card before the butto - Nothing is merged on GitHub. A committer applies the change themselves, and the ticket is where they decide to. The card links to [the core handbook page on pull requests](https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/). - -## Test mode - -Developers of the app itself can point this flow at a sandbox repository, or make it a dry run that pushes a branch without opening a pull request. When either switch is set, a **Test mode** banner appears on the card saying exactly what will happen, and the dry-run button reads **Push branch (dry run)** instead of **Open pull request**. In a normal build you will never see this banner. - -If a build has no GitHub application configured, the card says so — it cannot open a pull request, but the other destinations still work. diff --git a/docs/guide/submitting-changes.md b/docs/guide/submitting-changes.md index c9a18a0..9d57434 100644 --- a/docs/guide/submitting-changes.md +++ b/docs/guide/submitting-changes.md @@ -6,6 +6,8 @@ When your change works and you want to contribute it, open the **Review & submit The left side of the screen is the diff, titled **Your changes**. This is exactly what will be submitted, so read it before choosing a destination. +![The Your changes pane showing a new PHP file as a highlighted diff, with Save and Copy controls above it](/screenshots/submit-changes-diff.png) + What it contains is the work on the ticket you are on, and only that: everything the ticket's branch has gained since it was created, including whatever was parked the last time you switched away from it. Another ticket's work is never in it, and neither is a change that arrived from a [trunk update](trunk-updates). On a site with no ticket linked, it is simply everything the checkout has that its copy of trunk does not. Two buttons sit above the diff: @@ -15,6 +17,14 @@ Two buttons sit above the diff: If there are no changes to send, the screen says so. If the site's WordPress code is old, a warning says the patch may not apply on Trac and suggests updating to the latest trunk first — see [Staying up to date with trunk](trunk-updates). +## Discarding it all + +Next to the heading is **Discard all changes**. It asks first — *Discard all local changes? This cannot be undone* — and then throws away exactly what the diff above it shows, which on a ticket means the whole of that ticket's work: your uncommitted edits *and* anything parked in a commit the last time you switched away from it. + +The ticket itself survives. Its branch stays, the link stays, and you carry on working on it from a clean base. Throwing the ticket's work away along with its branch is a different gesture — [Delete this ticket's work](ticket-branches#deleting-a-ticket-s-work), on the tickets card. + +The button is unavailable while an install, a build or a [trunk update](trunk-updates) is running, or while the dev server is up, since all of them are holding the files it would rewind. + ## What a patch can and cannot carry The patch carries files you added, files you edited, and files you deleted. A deletion is written the way `git apply` and `patch` expect one, so a reviewer applying your patch really does lose the file. @@ -34,6 +44,8 @@ The one gap left: adding or deleting an empty file is still not represented in t The right side lists three destinations. The pull request is the one the app sends for you; the other two save a file for you to send. Each card states what it costs to use and what happens afterwards, so you can choose with the trade-offs in front of you. +![The Where this patch goes pane showing Open a pull request, Attach to Trac, and Hand it to a mentor](/screenshots/submit-destinations.png) + - **Open a pull request** — needs a GitHub account. The app forks `wordpress-develop` to your account, pushes your change to a branch, and opens the pull request. Automated checks run on it. [Opening a pull request](submit-github-pr) - **Attach to Trac** — needs a WordPress.org account, which you need anyway for props and to comment. The app saves the patch file and opens the ticket's attach page; you upload it yourself. No automated checks run. [Attaching a patch to Trac](submit-trac) - **Hand it to a mentor** — needs no accounts at all. The app saves a patch file carrying your WordPress.org username and the event you are at; someone else pushes it, and the props still land on you. [Handing a patch to a mentor](submit-mentor) diff --git a/docs/guide/terminal.md b/docs/guide/terminal.md index 6a23c77..fd24645 100644 --- a/docs/guide/terminal.md +++ b/docs/guide/terminal.md @@ -1,9 +1,6 @@ # The Terminal panel -Each site view includes an embedded terminal. It is not a general-purpose shell: it accepts a -small, fixed set of commands, all of which run in the site's directory using the Node.js runtime -bundled with the app. This is enough to rebuild the site after editing code — the reason the panel -exists — without requiring Node or npm on your machine. +Each site view includes an embedded terminal. It is not a general-purpose shell: it accepts a small, fixed set of commands, all of which run in the site's directory using the Node.js runtime bundled with the app. This is enough to rebuild the site after editing code — the reason the panel exists — without requiring Node or npm on your machine. ![The Terminal panel with the command hints below it](/screenshots/terminal.png) @@ -17,30 +14,21 @@ Type `help` to see the list at any time: | `npm install` | Runs `npm install` in the site directory. `npm i` and `install` work too. | | `npm run