Repository navigation
Conversation
mokagio
changed the base branch from
ainfra-2597-fix-macos-code-signing-for-contributor-toolkit
to
trunk
July 10, 2026 11:05
mokagio
force-pushed
the
mokagio/trim-desktop-artifact-deps
branch
from
July 10, 2026 11:05
b6ae55b to
1f8ca57
Compare
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
) ## Why The app's user documentation is the README, and it has outgrown it: install steps, a ten-step walkthrough and the trunk-update rules all compete for the same page, and there is nowhere to put a screenshot. This PR gives the guide a home and a deploy pipeline. It carries no guide content beyond the landing page — that arrives in #232, and the harness that photographs the app in #231. ## What changes - `docs/` becomes a VitePress site with **its own npm package**, so a docs-only CI job never runs the root `postinstall` (electron-builder + esbuild) and the app's dependency tree stays free of a static-site generator. - `.github/workflows/docs.yml` builds and deploys to GitHub Pages on pushes to `trunk` that touch `docs/`. Pull requests get a **build-only** job: a dead link fails before merge, and the job holding the OIDC token never runs on PR code. - The site's `base` is derived from the repository name at run time. A project Pages site is served under `/<repo-name>/`, and this repo has already been renamed once — deriving it means another rename cannot break every asset URL. - `srcExclude` keeps `docs/testing.md` (from #70) out of the published site: it documents how to run the suites, which is contributor material, and this site is for users of the app. ## How to test this **Starting state:** this branch checked out, `npm ci` done. 1. `npm run docs:build` — expect `build complete`. Now add a link to a page that does not exist in `docs/index.md` and run it again: it must **fail** with `dead link(s) found`. Undo. 2. `npm run docs:preview`, open the printed URL — the home page renders with the WordPress Contributor Toolkit hero and the sidebar. Clean URLs work; this is exactly what Pages serves. 3. `npm run docs:dev` — pages are served at their `.html` paths in dev (`/index.html`). With the dev server open, run `npm run docs:build` in another terminal: the dev server must **not** spew reload lines (it ignores its own output directory). 4. `npm run lint` and `npm test` — both green, unchanged from `trunk`. **Must not have happened:** no `deploy` job may run on this pull request — check the Actions tab and confirm only `build site` ran. **Not testable by hand here:** the deploy itself. It needs the workflow on `trunk`, and Pages is already set to "GitHub Actions" as its source. ## Risks Merging this alone deploys a site whose sidebar names the full guide, so **those links 404 until the content PR lands**. Merge the stack in order rather than leaving this on `trunk` by itself. The deploy job holds `pages: write` and `id-token: write`. Its actions are pinned to commit SHAs rather than tags, following the reasoning already written down in `download-stats.yml`. ## Related Stack: this → #231 (screenshot harness) → #232 (guide content). Touches #70 only through `srcExclude`; that PR needs no change. --- <details> <summary><b>Self-review</b> — 5 findings, all fixed</summary> Ran `.github/instructions/code-review.instructions.md` with the judgement pass in a fresh context. Deterministic layer was clean (ESLint and the unit suite on macOS and Windows). **5 [fix here] · 1 [follow-up]. All five fixed in this branch:** | Dimension | Was | Now | |---|---|---| | security 🟡 | `checkout` kept the job's `GITHUB_TOKEN` in `.git/config` while the job runs PR-authored code (`npm ci` with lifecycle scripts, and a VitePress build that evaluates `config.mjs`) | `persist-credentials: false`, matching `lint.yml` | | security 🔵 | `if: github.event_name != 'pull_request'` let a `workflow_dispatch` on any ref publish to the live site, which is not what the comment claimed | `if: github.ref == 'refs/heads/trunk'` | | cross-platform 🔵 | `node-version: 22` hardcoded while `.nvmrc` says 24.18.0 — reintroducing the Node drift of #37/#46 for the one command CONTRIBUTING calls "what CI runs" | `node-version-file: .nvmrc` | | architecture 🔵 | one concurrency group spanning build and deploy, with `cancel-in-progress: true`, so a second push could cancel an in-flight `deploy-pages` | split: builds cancel, deploys do not | | architecture 🔵 | CONTRIBUTING documented `docs:*` without saying the nested package needs its own install, so the commands fail on a clean clone | `npm ci --prefix docs` documented | **Deferred [follow-up]:** `electron-builder` has no `files` filter, so tracked `docs/` sources ship inside `app.asar` — and #232 adds screenshots on top. Real, pre-existing, and overlaps #23; an `!docs{,/**/*}` entry closes it. Not done here because it changes what every release artifact contains, which deserves its own PR and its own testing. **Checked and clean:** all four action pins resolve to the tags their comments claim (verified against the GitHub API); `pull_request_target` is correctly not used and no fork PR can reach `pages: write` / `id-token: write`; the lockfile is 175 packages, all from registry.npmjs.org, with install scripts only on esbuild and fsevents; the `import/no-unresolved` exemption is a genuine false positive and hides nothing; no `src/` code, IPC surface or spawn path is touched. </details>
This was referenced Aug 11, 2026
juanmaguitar
added a commit
that referenced
this pull request
Aug 11, 2026
## Why No branch has produced a signed macOS or Windows artifact since the last version bump. The build dies during packaging: ``` ⨯ ENOENT: no such file or directory, open '…/vendor/npm/_logs/2026-08-10T14_15_09_297Z-debug-0.log' ``` Trunk fails the same way, re-running a job does not help, and reviewers have had no way to test a pull request on a real machine — the only reason that build layer exists. Linux is unaffected, which is the first clue: it is the one job that does not use this cache. Details in #265. ## What changes **Root cause.** The artifact build keeps npm's package cache at `vendor/npm`, *inside the project directory*, and npm writes its own debug logs into it. `package.json` had no `files` filter, so electron-builder's default of "everything in the project directory is application content" swept that cache into the app. A cache directory is not static while a build runs: a log file present when the walk begins and gone by the time it is read takes the whole build down. Two changes, addressing two separate mistakes: - **`package.json` — exclude `vendor` from packaging.** This is the fix. A directory that mutates during the build must not be a packaging input. The `files` array holds only a negation, which electron-builder handles by prepending its own `**/*` default (`fileMatcher.js` → `containsOnlyIgnore()`), so nothing else about what gets packaged changes. The `!x{,/**/*}` form is the one electron-builder uses internally for the same job. - **The CI script — clear npm's logs on both sides of the cache.** After the restore, so no build inherits logs written by another build on another machine; before the save, so none are archived onward. This is cache hygiene, not the cure: `npm ci` writes a fresh log during the build regardless, so only the exclusion above closes the failure. `.gitignore` gains `vendor/npm` alongside the existing `vendor/bundle`, so the cache cannot be committed by anyone who runs the CI script locally. **Why the version bump was the trigger.** The cache key hashes `package-lock.json`. The bump minted a fresh cache entry, and the build that created it archived its own debug log into it — the timestamp in the error is that build's `npm ci`. Every build since has restored the poisoned entry. ## How to test this **Platforms: macOS and Windows.** These are the two jobs that use this cache; Linux never had the problem. The end-to-end test is this PR's own artifact build. Locally, packaging can be driven directly: **Starting state:** a clean checkout with dependencies installed. 1. Stage a stand-in for the CI cache: ``` mkdir -p vendor/npm/_logs && echo x > vendor/npm/_logs/stale-debug-0.log ``` 2. `CSC_IDENTITY_AUTO_DISCOVERY=false npx electron-builder --dir --publish never` 3. `npx asar list "dist/mac-arm64/WordPress Contributor Toolkit.app/Contents/Resources/app.asar" | grep '^/vendor'` **Expected: no output.** On `trunk` the same command lists `/vendor`, `/vendor/npm`, `/vendor/npm/_logs` and the log file itself — that is the bug, visible without needing to reproduce the race. 4. Same command without the `grep`, confirming the app is still whole: `/src/main.js`, `/src/preload.js`, `/src/renderer/index.js`, `/package.json` and the `node_modules` tree are all present. **What must not have happened:** the app must not have got *smaller* in any way other than losing `vendor`. A wrong `files` pattern would silently drop `src` or `node_modules` and the packaging step would still report success — the app would only fail when a contributor launches it. Step 4 is what catches that. Also check the CI log still shows the npm cache being restored and saved: the cleanup must not have removed `_cacache`, or every build re-downloads every package. ## Risks and limitations - **The crash itself was not reproduced locally.** It needs the log file to vanish mid-walk, which is a race between processes on the CI machine and could not be staged. What is verified is the mechanism underneath it: the cache directory *is* packaged, and it *does* contain those logs. The green build on this PR is the confirmation. - **`vendor` is excluded; other non-app directories are not.** `docs`, `test`, `scripts` and `fastlane` still get packaged, but they are ~217 entries against 28085 for `node_modules`, so there is nothing to win there. The real artifact bulk is the dependency tree — see #23, which measured it and is a separate concern. - **No unit test.** Neither an electron-builder config key nor a CI shell script is reachable from `node --test`; a test asserting the string `"!vendor{,/**/*}"` is present would restate the diff rather than test it. Verified by actually packaging instead, twice, and the assertion is written into the testing steps above so it can be re-run. - The published artifacts up to now have been carrying the build machine's package cache. Whether that is a meaningful share of their size will show when this build's artifact size is compared against the last one. ## Related Fixes #265. Adjacent to #23 (packaged app carries more than the app), which attacks artifact size through the dependency tree and is stale and conflicting — it needs its own decision. --- <details> <summary>Design decisions and alternatives considered</summary> **Moving npm's cache out of the project directory** would have fixed the crash at its root, and was the first instinct. Rejected because the cache path is load-bearing for the CI cache save/restore, which archives that directory by relative path — relocating it means reworking the caching, on a pipeline that is currently red. Excluding it from packaging is the smaller change and the correct invariant regardless of where the cache lives. **Setting npm's `logs-dir` outside the project** is the tidier version of the log cleanup and would make the deletions unnecessary. Not done here because it needs a path that is valid on both the macOS and Windows agents, which is a second decision to get wrong while the pipeline is broken. Worth revisiting once builds are green. **An inclusive `files` list** naming what to package would be more explicit than a lone negation, and electron-builder's own docs argue against it: an inclusive list silently drops anything added later. The negation keeps the default and states only the exception. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Reviewed against the five dimensions in `.github/instructions/code-review.instructions.md`. `npm run lint` clean. `npm test` — 780 passing, 0 failing. **1 [fix here] · 1 [follow-up] — the [fix here] was fixed before this PR was opened.** - 🟡 **architecture · [fix here]** — the first draft cleared `_logs` only before `save_cache`. That leaves the already-poisoned cache entry in place, since the save does not overwrite an existing key, so builds would have stayed red until the lockfile changed again. Moved the primary cleanup to just after `restore_cache`. - 🔵 **tests · [follow-up]** — no automated coverage that `vendor` stays out of the asar. A packaging assertion would need a build in CI rather than a unit test; noted in **Risks** above rather than filed, as it is one assertion on a directory that no longer exists in the app. Considered and cleared: no new dependency, no host binary, nothing crossing IPC, no spawn, no path composition, and `rm -rf` targets a literal quoted path set two lines above under `set -u`. The bash script runs on both the macOS and Windows agents and uses nothing platform-specific. </details> <details> <summary>Implementation notes</summary> The negation-only `files` behaviour is not folklore — `app-builder-lib/out/fileMatcher.js`: ```js if (!matcher.isSpecifiedAsEmptyArray && (matcher.isEmpty() || matcher.containsOnlyIgnore())) { customFirstPatterns.push("**/*") } ``` `containsOnlyIgnore()` is true when every pattern starts with `!`, so the default include is prepended and the exclusion is layered on top. The same function adds electron-builder's own exclusions in exactly the `!${dir}{,/**/*}` form used here. Local verification, on this branch versus without the `files` key, with a stand-in cache staged: | | `/vendor` entries in `app.asar` | total entries | |---|---|---| | without `files` | 6 | 28308 | | with `files` | 0 | 28302 | The 6-entry difference is exactly the staged cache. The byte difference is trivial here only because the stand-in cache is a few hundred bytes; on a CI agent it is the full package cache. </details> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Probe for AINFRA-2597: electron-builder reports the Developer ID identity as `CSSMERR_TP_NOT_TRUSTED`, so the macOS build ships unsigned. The cert installs fine — the trust chain fails to evaluate — so dump keychain search list, `find-identity` output, intermediate-CA presence, and an explicit `verify-cert` on the CI agent to pinpoint the missing anchor before committing to a fix. Non-fatal; reverted once diagnosed. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The macOS 26 CI image ships no "Developer ID Certification Authority" G2 intermediate, and `match` installs only the leaf. With no keychain in the search list able to build `Developer ID App → Developer ID CA G2 → Apple Root`, `security` evaluates the identity as `CSSMERR_TP_NOT_TRUSTED` and electron-builder silently skips signing — the same failure and fix as the WordPress desktop (calypso) app. Fetch and import the intermediate into the temp keychain `setup_ci` creates, gated to CI. Add a `verify_code_signing` lane (`codesign --verify` + Gatekeeper `spctl --assess` + Developer ID authority assertion) and run it after `npm run dist`, so an unsigned or mis-signed build fails the job instead of shipping. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI build #72 confirmed the trust chain now resolves (`verify-cert` succeeds, `find-identity` reports a valid identity) and the app is signed, notarized, and Gatekeeper-accepted. The probe has served its purpose; the permanent `verify_code_signing` lane guards the invariant from here. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The renderer is esbuild-bundled at build time, so React, `@wordpress/*`, `@emotion/*`, and `xterm` are already inlined into `src/renderer`. Sitting in `dependencies`, electron-builder also shipped their full `node_modules` trees inside the app — dead weight the runtime never loads. Move them to `devDependencies` so packaging drops them, and remove `@xterm/xterm`, which has no references in `src` (the renderer imports the `xterm` package). Trims the packaged production tree from 1366 MB to 1196 MB (~170 MB, 12%). The bulk that remains is `@php-wasm` (PHP-WASM builds under `@wp-playground/cli`); reducing that needs a call on which PHP versions and execution strategies to support and is left as a follow-up. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mokagio
force-pushed
the
mokagio/trim-desktop-artifact-deps
branch
from
August 21, 2026 01:15
1f8ca57 to
354058e
Compare
zaerl
added a commit
that referenced
this pull request
Sep 30, 2026
… system redesign (#563) ## Why The redesign in #542 is built on `@wordpress/ui` and `@wordpress/theme`, and both are developed against the current `@wordpress/components`. The app is on 28, thirteen majors behind, so nothing from the design system can be added until the renderer stands on 41. This is the first step of #542 and changes no behaviour. ## What changes - `@wordpress/components` moves from `^28.2.0` to `^41.0.0` and `@wordpress/icons` from `^10.7.0` to `^17.0.0`. React stays on 18.3.1, which both packages support. - The `edit` icon was removed from `@wordpress/icons`; `pencil` is the same glyph under its current name. - `build.files` excludes the thirteen packages the update brought into the packaged app. `@wordpress/components` 41 depends on `@wordpress/ui` and `@wordpress/theme`, and `@wordpress/theme` names esbuild as an optional peer, so npm stopped marking esbuild as dev-only and electron-builder started shipping its binary. None of the thirteen is loaded at run time: the renderer is the esbuild bundle, and nothing under `src/` requires them. - The sidebar title is kept on one line. Buttons are set in a heavier weight now, and the wider "Collapse" button wrapped "Contributor Toolkit" onto two. - Text inputs are now 40px tall instead of 36px. So the buttons are slightly smaller. But they will be removed anyway. Deliberately not in this PR: adding `@wordpress/ui` or `@wordpress/theme` as dependencies of the app, any use of design tokens, and any layout change. Those are the next steps of #542. ## How to test this Platforms: any. Nothing here touches paths, spawning or signing. Use the Buildkite build for the current head of this branch. The journeys carry the behavior: `npm run test:e2e` from the repository root drives every flow the renderer has through the updated components, and `npm run test:e2e:packaged`, after `CSC_IDENTITY_AUTO_DISCOVERY=false npm run pack:dir`, proves the packaged app still boots without the excluded packages. What they cannot see is how the app looks, so that is what a person should check. **Starting state:** the app open with at least one site that has finished setup. 1. Look at the sidebar. "Contributor Toolkit" is on one line, beside "Collapse". 2. Click "Collapse", then the arrow to expand again. The sidebar returns to the same layout. 3. Click the pencil beside the site name. The "Rename site" dialog opens; close it. 4. Open the menu at the top right of the site. It lists "Copy path", the file manager entry, "Update to latest trunk" and "Delete this site". 5. Click "Create a site". The dialog shows "Site name", "Contribute to" with two choices, and "Site location"; close it. 6. Scroll to "Logs" and click each tab. The selected tab is underlined and its content changes. **What must not have happened:** - No button, field or menu has lost its label or become unreachable. - No text is clipped or overlapping where a control grew: text fields are 40px tall instead of 36px, and button labels are heavier. ## Risks and limitations - **The look changes slightly, and that is the design system's doing.** Button labels are heavier, text fields are taller, menu items are more compact, and the selected tab is marked in neutral grey and black where it was blue. Details under Screenshots. - **The docs screenshots are not retaken.** The twelve the harness can produce differ slightly from the app after this. #542 retakes all of them, and the harness is not reproducible across runs (#326), so refreshing them here would add churn twice. - **Checked on macOS only by hand and by local runs.** Windows is covered by CI's journeys and packaged smoke, not by a person. - **The exclusion list is by name.** A future update that adds another renderer-only package will ship it until someone adds a line. Excluding the whole renderer-only tree is the real fix and overlaps with #23. - **The first commit of this branch was pushed to `trunk` by mistake and removed again.** `trunk` is back at 0dc54da; anyone who fetched in between has b6a6151 on their local `trunk`. - Review: 2 [fix here], both fixed; 1 [follow-up] deferred. ## Related Part of #542. Fixes #548. --- <details> <summary>Design decisions and alternatives considered</summary> **Excluding in `build.files` rather than moving the renderer's dependencies to `devDependencies`.** Moving them would stop the whole renderer-only tree from shipping, not just what this update added, but that is a change to how the app is packaged and #23 is already about it. This PR only undoes the growth it would otherwise cause. **One jump from 28 to 41 rather than a major at a time.** The renderer uses seventeen components, all of which still exist with the same props. The only break was the removed icon alias, which fails the build rather than the app. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **2 [fix here] · 1 [follow-up]** — both [fix here] fixed. - Architecture, medium, [fix here], fixed: the packaged app shipped the esbuild binary, because `@wordpress/theme` names esbuild as an optional peer and electron-builder collects optional packages. - Performance, medium, [fix here], fixed: twelve more packages the update added were collected into the app and never loaded. - Performance, low, [follow-up], deferred: the whole renderer dependency tree ships although only the bundle is loaded. It predates this branch and is the subject of #23. Style notes, not applied: with `white-space: nowrap` the sidebar title can paint outside the sidebar during the 200ms expand animation; the `esbuild@0.23.1` entry in `allowScripts` has been stale since #538. - **Review:** completed — fresh agent context, local, before the PR existed; head b6a6151 / base 0dc54da; committed work only, working tree clean; outcome above. CodeRabbit: not run yet. - **Since review:** `b6a6151 → 4b2d52e` checked. The difference is the thirteen exclusion lines in `build.files` that fix the two findings. It was not reviewed again by a fresh context; it was verified by packaging and running the packaged smoke test. Checks on 4b2d52e, macOS, local: | Check | Result | |---|---| | `npm run lint` | clean | | `npm test` | 1676 tests, 1674 pass, 2 skipped, 0 fail | | `npm run test:electron` | 1676 pass, 0 fail | | `npm run test:e2e` | 33 of 33 pass | | `npm run test:e2e:packaged` | 15 of 15 pass | Lint, the unit tests and the journeys ran on b6a6151; the second commit changes `build.files` only, which none of the three reads. </details> <details> <summary>Implementation notes</summary> `app.asar` on macOS arm64, unsigned, `npm run pack:dir`: | State | Size | |---|---| | After the update, before the exclusions | 238,010,661 bytes | | With the exclusions | 199,873,553 bytes | The size on `trunk` was not measured. Packages excluded: `esbuild`, `@esbuild/*`, `@base-ui/*`, `@date-fns/*`, `@daypicker/*`, `react-day-picker`, `reselect`, `tabbable`, `colorjs.io`, `@wordpress/base-styles`, `@wordpress/kebab-case`, `@wordpress/style-runtime`, `@wordpress/ui`. The list is the difference between the two lockfiles in what is not marked dev-only. `@wordpress/components` 41 injects part of its CSS at run time through `@wordpress/style-runtime`. The renderer's content security policy already allows it (`style-src 'self' 'unsafe-inline'`). </details> <details> <summary>Screenshots or recording</summary> No images attached. The comparison was made between the committed docs screenshots and the same twelve shots captured on this branch with `npm run shots`. What differs: - Button labels are set in weight 500. - Text fields are 40px tall instead of 36px. - Menu items are more compact. - In "Logs", the selected tab has black text and a grey underline; it was blue. - The sidebar title wrapped onto two lines before the fix in this PR, and is on one line with it. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
|
@mokagio is the PR something you'll be able to move forward on? |
zaerl
added a commit
that referenced
this pull request
Oct 23, 2026
…braries and Chromium locales (#707) ## Why The packaged app is about 920 MB on disk on macOS (320 MB zipped), and about a third of that is code it never loads. That is a long download on a Contributor Day network, where everyone fetches the app at the same time. It is the same problem #23 reported. That PR has since gone stale against trunk, and its claim that `@xterm/xterm` is unused no longer holds, so this branch starts again from trunk. ## What changes Three things the app ships and never loads are left out: - **The asyncify PHP builds, about 220 MB.** Each `@php-wasm/node-*` package carries every PHP version twice, as `jspi/` and as `asyncify/`, and `@php-wasm/node` reads `asyncify/` only when `"Suspending" in WebAssembly` is false. Electron 43.6.0 has JSPI, so those folders never load. `@php-wasm/node-5-2` goes too, because the app offers only `SupportedPHPVersions` (7.4–8.5). Both are `build.files` exclusions; `node_modules` is untouched. - **The renderer's libraries, about 95 MB in `app.asar`.** esbuild already bundles React, Emotion, xterm and the `@wordpress` UI packages into `src/renderer/index.js`, but they were listed in `dependencies`, so electron-builder copied their trees in as well. They move to `devDependencies`. `@wordpress/i18n` and `@wordpress/hooks` stay, because the main process loads them. The 22 hand-kept `build.files` exclusions that trimmed part of that tree since #563 are now unnecessary and are removed. - **Chromium's locales for languages the app has no catalog for, about 43 MB.** The app shows English for those anyway, so `build.electronLanguages` keeps English and the Chromium locale of each catalog in `src/languages`. **`npm run i18n:download` now writes that list** whenever it adds or removes a catalog, so the version-bump PR carries the change with no manual step. `scripts/chromium-locales.cjs` maps a catalog to the locale Chromium itself would load, including Chromium's regional folding (`es-mx` → `es-419`, `pt` → `pt-PT`, `zh-hk` → `zh-TW`, `en-ca` → `en-GB`). Every regional locale is written in both spellings, `pt-BR` for Windows and Linux and `pt_BR` for macOS, because electron-builder matches file names. Unit tests guard the cuts: - `electron-wasm-jspi` asks the bundled Electron for `WebAssembly.Suspending`, so an Electron bump that loses JSPI fails CI instead of the first dev server start. - `electron-languages` asserts that the list equals the one the catalogs call for, which catches both a missing entry and a stale one. It also checks that electron-builder keeps every locale file the list names. - `chromium-locales` covers the mapping with both platforms' locale names. - `php-builds` fails when an installed `@php-wasm/node-X-Y` whose version `SupportedPHPVersions` does not offer would ship, and when a version the settings offer is excluded. Run against translate.wordpress.org today, the script adds `sr` and `tr` for two catalogs that are above the cut-off but not yet in the repository, and leaves out `lo`, which Chromium has no locale for. Not in this PR: dropping PHP versions the settings offer (a product decision since #559), and the `intl` extension and ICU data, which core's tests use. | macOS arm64, unsigned, same `node_modules` | trunk | this branch | |---|---|---| | `.app` on disk | 920 MB | **571 MB** | | zipped | 320 MB | **218 MB** | | `app.asar` | 205 MB | 108 MB | | `app.asar.unpacked` | 436 MB | 228 MB | | Electron Framework | 277 MB | 234 MB | ## How to test this Platforms: macOS and Windows, both needed, because the PHP runtime is chosen when the server starts and no CI job starts the real Playground server (see the Electron pin note in AGENTS.md). Use the Buildkite artifact for the current head commit. **Starting state:** the app from this branch's Buildkite build, with a site that has finished setup and whose server is stopped. 1. Open the site and click **Start development server**. The server comes up, and the site's front page and `/wp-admin/` both load in the browser. 2. Open the app's log for that site. It has no `Cannot find module` error, and no path containing `asyncify`. 3. In the settings, change the PHP version to 7.4, then start the server again. The site loads, and the log names PHP 7.4. 4. Use the app as usual: the site list, the terminal (xterm), the changed files, a dialog. Everything draws as before. The renderer no longer reaches `node_modules` for any of it. 5. On macOS, compare the size of the app with a trunk build in Finder (Get Info): about 570 MB against about 920 MB. **What must not have happened:** - The dev server failing to start, or starting on a PHP version other than the one chosen. - A blank or unstyled window, which would mean the renderer still needed a package that is no longer shipped. - Native form strings, such as a validation bubble, in a different language from before for cs, hu, nl, pl or English. ## Risks and limitations - The asyncify cut depends on Electron keeping JSPI. That is what `electron-wasm-jspi` guards. - On macOS, electron-builder also removes the grammatical-gender overlays (`cs_FEMININE.lproj` and similar) for the kept languages; each holds an 18-byte `locale.pak`. Chromium should fall back to the base locale, but I have not checked a macOS user with a grammatical gender set. - `electron-languages` copies electron-builder's matching rule, because it is not exported. An electron-builder release that changes that rule would not be caught. - Chromium's regional folding rules in `scripts/chromium-locales.cjs` follow `CheckAndResolveLocale` in Chromium's `l10n_util.cc`, but no Chromium source is vendored here to check them against. Only `en-ca` → `en-GB` affects today's list, and `en-GB` is kept for `en-gb` anyway. - `npm run i18n:download` now needs an installed Electron, which it downloads on first use. It reads Electron's locales before changing any catalog, so a failed download leaves everything as it was. - The packaged smoke suite passed locally only with `--user-data-dir` added to its launch, because a dev copy of the app held the single-instance lock. Trunk's build failed the same way without it. CI's run on this PR is the unmodified one. - Review: one completed fresh-context review, 0 findings; details below. ## Related - Fixes #703: the `asyncify/` folder of every PHP build is left out, which is the exclusion the issue proposes, and `electron-wasm-jspi` guards the JSPI it depends on. - Fixes #704: `@php-wasm/node-5-2`, the only installed build whose version `SupportedPHPVersions` does not report, is left out. `php-builds` fails when a Playground update changes which builds those are, and names the exclusion to add or remove. - Fixes #705: `build.electronLanguages` keeps only English and the Chromium locales of the app's catalogs, and `npm run i18n:download` keeps it in step. - Follow-up to #23, which reported the renderer-library half. That PR belongs to its author and is left as it is. --- <details> <summary>Design decisions and alternatives considered</summary> - **Exclusions in `build.files`, not a `postinstall` prune.** `node_modules` stays complete for development, the tests and the journeys. Only what electron-builder copies changes. - **`devDependencies` rather than more `!node_modules/...` lines.** The exclusion list from #563 had to grow every time a `@wordpress` package gained a dependency. Moving the cause leaves nothing to maintain, and lets 22 lines go. - **A hand-written `electronLanguages` list with a test, rather than one generated at build time.** A generated list would need a JS build config or a hook; a static list plus a test that names the missing entry is simpler, and changes only when a catalog is added. - **Chromium locales for the app's catalogs only.** Keeping a broader set would leave Chromium's strings in, say, Italian around an English app, which is no better than English. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 0 [fix here] · 0 [follow-up]. One style note was applied: the `electron-languages` header claimed Chromium's strings follow the app's language, but they follow the operating system's. - **Review:** completed. Reviewer: a separate Claude agent context, given only the diff and `.github/instructions/code-review.instructions.md`. Reviewed head `a7be033` against base `2dda92c` (`origin/trunk`). Evidence: `npm run lint` passed; `npm test` passed with 2202 tests, 2200 pass, 0 fail, 2 skipped; `npm run test:electron` passed with 2202 tests, 2201 pass, 0 fail, 1 skipped. The five dimensions found nothing. Specifically checked: no main-process, preload or runner import of a moved package; no production dependency still reaching a formerly excluded package; asyncify reached only when JSPI is absent, including for intl and xdebug; PHP 5.2 unreachable from the settings; electronLanguages against electron-builder's matching rule and the app's locale resolution. - **Since review:** `a7be033 → 7ac9e25` checked. The only change is the comment wording in `tests/unit/electron-languages.test.cjs`, and that test still passes. - **Since review:** `7ac9e25 → 8e1d005`, a CI fix, re-checked by the author against the same standard: 0 findings. CI's unit job failed because Electron 43's npm package downloads its binary on the first `require('electron')`, and three test files now asked for it at the same moment. I reproduced that locally by removing `node_modules/electron/dist`: two downloads ran and two tests failed. A `pretest` script now asks once before `node --test` starts, and the workflow comment that said `npm ci` fetches Electron is corrected. Run from a missing binary, `npm test` downloads once and passes 2200 tests with 0 failures, and `npm run test:electron` and `npm run lint` pass too. - **Review of `8e1d005 → dde0f94`** (the `i18n:download` change): completed by a separate Claude agent context against the same standard, in the context of `origin/trunk...dde0f94`. Result: 1 [fix here] · 0 [follow-up], fixed in `26e9250`. The finding: Electron's locale names were read after the catalogs had been replaced, so a failed Electron download would leave the run half applied. They are now read before anything is written. One style note was applied as well, a test title that claimed more than it asserted. The reviewer also ran about 130 slugs through the mapping without a crash, and checked that rewriting `package.json` reproduces the file byte for byte. Lint passes, and `npm test` and `npm run test:electron` each pass 2212 tests with 0 failures. - **Since that review:** `dde0f94 → 26e9250`. Only that fix and the renamed test changed; the suites above ran on `26e9250`. - **Since that review:** `26e9250 → 55ab855` adds only `tests/unit/php-builds.test.cjs`, for #704. I checked it against the standard myself: 0 findings. Broken both ways in turn (5.2 kept, 8.3 excluded), it fails each time, and it passes as committed. `npm test` and `npm run test:electron` each pass 2214 tests with 0 failures. - **CodeRabbit:** not requested yet. </details> <details> <summary>Implementation notes</summary> - Sizes come from `electron-builder --dir` with `-c.mac.identity=null -c.mac.notarize=false`, run twice from the same `node_modules` with trunk's and this branch's `package.json` and lockfile, then zipped with `ditto -c -k`. Signed artifacts will differ slightly. - Locally, from the trimmed package, `@php-wasm/node` booted PHP 7.4.33, 8.3.33 and 8.5.10 with `intl` loaded, run on the packaged binary with `ELECTRON_RUN_AS_NODE=1`. - `npm run test:e2e:packaged` passed all 15 tests on the trimmed build, including the first window painting, the preload bridge, and the `build.files` allow-list checks. - In the lockfile, the moved packages and their trees change from `devOptional` to `dev`. No versions change. - `pretest` runs `node -e "require('electron')"`. The electron package downloads its binary on the first `require`, so this fetches it once before `node --test` runs its files in parallel, where concurrent downloads raced. `npm run test:electron` already did this in its runner, and a single file run with `node --test <file>` has nothing to race with. - The asyncify choice is made in `@php-wasm/node/index.cjs` (`loadNodeRuntime`) and in each `@php-wasm/node-X-Y/index.cjs` (`getPHPLoaderModule` and the extension paths). </details> <details> <summary>Screenshots or recording</summary> Nothing on screen changes. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #22 (macOS code signing). Review/merge that first — GitHub will retarget this to
trunkonce it lands.Rationale
The packaged app was ~1 GB on every platform. The renderer is esbuild-bundled at build time, so React,
@wordpress/*,@emotion/*, andxtermare already inlined intosrc/renderer— yet they also sat independencies, so electron-builder shipped their fullnode_modulestrees inside the app too.@xterm/xtermwas a dependency with zero references insrc(the renderer imports thextermpackage).Moving the build-only libs to
devDependenciesand dropping@xterm/xtermtrims the packaged production tree 1366 MB → 1196 MB (~170 MB, 12%), 492 → 312 packages.Not addressed here
The remaining bulk is
@php-wasm(~1 GB:@php-wasm/node654 MB +@php-wasm/web406 MB), pulled transitively by@wp-playground/cli. It ships every PHP version (7.2–8.4) in two execution strategies (asyncify + jspi), plus a browser build (@php-wasm/web) thatsrcnever imports directly. Cutting it needs a product decision on supported PHP versions/strategies and whether@php-wasm/webis reachable — left as a follow-up.How to test
electron-store(runtimeimport()), the main process,preload.js, and the spawned runner scripts were checked — none import the moved libraries.npm run build:oncebundles the renderer cleanly with the libs as devDependencies. Confirm the macOS/Windows/Linux builds still produce working apps and that artifact sizes drop by ~170 MB on the Buildkite run.