Add an update path so sites can move to the latest trunk - #111
Conversation
Sites were pinned forever to the trunk of the day they were cloned:
patches aged against that frozen snapshot and stopped applying on Trac.
- Record the trunk snapshot date per site and surface it ("trunk as of
<date>") in the site header, with a staleness notice and sidebar dot
once the snapshot is more than 14 days old.
- Add an "Update to latest trunk" action running the full chain: fetch
and reset to the latest remote trunk, npm install only when
package-lock.json changed between the two tips (the skipped step is
named, not hidden), then a rebuild — reusing the existing npm
machinery and terminal streaming.
- Ask before updating a dirty tree: save the changes as a local patch
file (default) or discard them.
- Persist an "update incomplete" state when trunk moved but
install/build did not finish, with a retry path, so a crash mid-chain
is visible instead of a silently broken site.
- Document that the patch diff base deliberately stays at local HEAD
(diffing against a moved remote trunk would embed reversed upstream
changes), and correct the wrong origin/trunk claim in CLAUDE.md.
Closes #94
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The "Update to latest trunk" action now lives in the staleness notice
(plus the More menu for on-demand updates), not as a standing button
in the action row — so it disappears once the site is up to date.
- The running update renders as a card with a vertical step checklist
("step N of 3", skipped install named in place, build pointing at the
Terminal), matching mockup 3.
- The dirty-tree dialog matches mockup 4: it names the changed files,
offers selectable options with "Save patch & update" as the default
primary, and the destructive path is explicit.
- A green "Up to date with trunk as of today" card summarizes the
finished update (dependencies changed or not, rebuild time, where the
pre-reset patch was saved).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
How to testNew sites clone today's trunk, so the stale states never show up on their own. Simulate staleness by rewinding a site to an old trunk with system git (fine on a dev machine — only the app must not need git), then restart the app so it re-reads the snapshot date. Rewind a site (the staleness simulator)cd <site-directory>
git fetch --depth=1 origin <OID>
git update-ref refs/heads/trunk <OID>
git checkout -f trunkAll three commands are needed — Pick an
Scenarios
🤖 Generated with Claude Code |
…hanges A site checked out by native git on Windows (default core.autocrlf=true, set in the global config isomorphic-git never reads) has CRLF on disk while wordpress-develop's blobs are LF-only. statusMatrix then hashes the raw CRLF bytes and reports every text file as modified — the update path's dirty dialog listed 4947 "changed" files on a pristine site (upstream: isomorphic-git#1275). - Write core.autocrlf=true into each site's local .git/config (at clone, when adding an existing dir, and before every status/patch/update git op, covering pre-existing sites) so isomorphic-git strips CRLF before hashing workdir files. LF checkouts are unaffected. - Normalize CRLF→LF on both sides of patch generation, so a CRLF checkout doesn't produce a patch of line-ending churn that would apply nowhere on Trac. Verified against isomorphic-git 1.37.6: a CRLF-rewritten file reports (1,2,1) without the config and (1,1,1) with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Windows fix pushed ( Root cause: a site checked out by native git on Windows (default Fix: the app now writes Verified against isomorphic-git 1.37.6: a CRLF-rewritten file reports Note on #36: complementary, not overlapping — that PR excludes untracked dotfiles from patches via 🤖 Generated with Claude Code |
…changed The core.autocrlf normalization only covers files that decode as valid UTF-8 — isomorphic-git skips the rest. wordpress-develop's encoding test fixtures (tests/phpunit/data/formatting/big5.txt and remove_accents-01.input.txt) are deliberately non-UTF8, so after a native-git checkout smudges them to CRLF they still hashed as modified and the update dialog reported two phantom changes on a pristine tree. The dirty check now confirms each statusMatrix candidate with a byte-level CRLF-insensitive comparison (encoding-agnostic) before reporting the tree dirty. Real edits to those same files are still detected — covered by a simulation against isomorphic-git 1.37.6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Follow-up Windows fix ( The dirty check now confirms every statusMatrix candidate with a byte-level, encoding-agnostic CRLF-insensitive comparison before reporting the tree dirty. Verified in a simulation against isomorphic-git 1.37.6: the smudged fixture is cleared as clean, and a real edit to the same file is still detected. 🤖 Generated with Claude Code |
main.js was accreting git logic; the trunk update path now lives in src/trunk-update.js (no Electron dependency), with main.js reduced to IPC plumbing and electron-store writes. The dirty check's CRLF verification collapsed into a single matrix pass, and the scratchpad simulations used during review became real integration tests: the module now runs under node --test against on-disk repositories, covering the CRLF-smudge false positives (UTF-8 and non-UTF8), real edit detection, discard semantics, and trunk-info reads. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The staleness copy assumed vocabulary the target user doesn't have yet: "trunk snapshot" and "diffed against" are insider terms in the very sentence meant to warn a first-time contributor. The notice now reads "This site's WordPress code is N days old — patches you create now may not apply on Trac. Updating takes a few minutes." (the reassurance line comes from the issue's mockup). Same phrasing in the patch-modal warning and sidebar tooltip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hes-age-against-a
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| isBusy={isServerStarting} | ||
| variant={isDevProcessActive ? 'secondary' : 'primary'} | ||
| onClick={toggleDevServer} | ||
| disabled={isUpdating} |
There was a problem hiding this comment.
[fix here] 🟡 Architecture — checklist install/build aren't gated by isUpdating
This line (and its twin at 1960) disables the dev-server toggle and "Submit patch" for the whole update chain, but the initial-setup checklist's "Install npm dependencies" / "Run first full build" buttons (lines ~1667 and ~1681, driven by computeSetupStepState in setup-steps.cjs) are never gated by isUpdating — only by their own installing/building flags.
Those flags stay false during the fetching step: updateToLatestTrunk (src/trunk-update.js) runs git.fetch + git.checkout({ dir, ref: 'trunk', force: true }) against the site directory before runInstall/runScript (and therefore installing/building) are ever set. installing/building only start reflecting the update once its own install/build steps kick in, so for the fetch window the checklist buttons look idle and clickable.
Concretely: a contributor downloads the app, doesn't finish the setup wizard (skipInit stays false, so the checklist keeps rendering), and comes back after the site's trunk snapshot has passed the 14-day staleness threshold — both the checklist and the new staleness banner render together. Clicking "Update to latest trunk" starts the forced checkout while the checklist's "Install npm dependencies" button is still enabled; clicking it fires a second npm install on the same directory concurrently with git.checkout({force:true}) rewriting package.json/package-lock.json mid-read — no locking exists at the IPC layer (npm:install/npm:run-script in src/main.js key their state only by a fresh installId/runId, not by directory).
Suggest threading isUpdating into computeSetupStepState's disabled for the install/build steps (or gating runInstallWithTerminal/runBuildWithTerminal directly), matching the guard already applied to the dev-server and patch buttons.
There was a problem hiding this comment.
Fixed in 97f761d: setup-steps.cjs's computeSetupStepState now takes an isUpdating flag and folds it into disabled for the install, build, and dev steps (same rationale as the existing isPending guard), and index.jsx passes it through. Added two unit tests in test/setup-steps.test.cjs covering the exact window this flagged — hasNodeModules/hasBuilt already true, isUpdating: true, before the update's own installing/building flags would ever be set.
AI review — 0 findings (0 fix here, 0 follow-up)Reconciliation with previous pass: the one earlier finding is resolved.
No new findings across the five dimensions in this round's diff ( Style / process notes (non-blocking)
|
…updates ESLint flagged 95 errors on this PR's changed files; ~40 were genuinely new (missing JSDoc @PARAM, nested ternaries, alert() in the dirty-tree flow). The rest are pre-existing backlog in index.jsx/main.js/preload.js from before this repo had a linter — left for a separate cleanup PR. Also addresses the AI review finding: the setup checklist's install/build/ dev buttons weren't gated by isUpdating, so during an update's fetch step (before its own installing/building flags are set) they stayed clickable and could race the update's forced checkout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d.js (#114) ## Summary - The repo had no linter until #112, so `eslint .` reports pre-existing findings across files nobody had touched since — 57 of them concentrated in `src/renderer/index.jsx`, `src/main.js`, and `src/preload.js`. These three kept blocking #111's lint check on lines that PR never modified. - Clears all of them in these three files. `lint.yml` still only lints changed files, but once eslint eventually moves to the full repo, these three won't be part of the remaining work. - Mechanical fixes (unused vars/imports, nested ternaries, `no-unused-expressions`, `no-shadow`, `eqeqeq`, `prefer-const`, JSDoc `@param`) preserve behavior exactly. - Judgment calls, each documented inline: - `console.*` in the SMTP handler → the existing `logEvent`/`logError` helpers from `./logging`, matching the rest of the main process. - `alert()`/`confirm()` are left in place with justified `eslint-disable` comments — replacing this file's whole alert-based UX convention is a separate, larger change than a lint cleanup should make. - Two dead zip-download-era leftovers removed: `downloadFile()` and `WORDPRESS_ZIP_URL` (and their now-unused `https`/`extract-zip` imports) — the app clones via git now, nothing calls either. - A couple of genuine accessibility gaps fixed: the email list row gets a keyboard handler alongside its click handler, and an anchor styled as a button became a real `<button>`. ## Test plan - [x] `npx eslint --max-warnings=0 --no-warn-ignored src/renderer/index.jsx src/main.js src/preload.js` — clean - [x] `npm test` — 113/113 passing - [x] `npx eslint .` — remaining findings are all outside these three files (untouched by this PR) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…hes-age-against-a # Conflicts: # src/main.js # src/renderer/index.jsx
Stacked on #111 — merge that first; this shows only the copy diff. Now that the trunk update chain rebuilds automatically, "Run first full build" reads as if later builds were also the user's job. The one-time framing is already carried by the "Initial setup checklist" heading, so the copy drops "first"/"once"/"initial" and says explicitly where later builds come from: - Step title: "Run first full build" → **"Run full build"** - Description: "Compile WordPress Core once to generate the initial dist files." → **"Compile WordPress Core to generate the dist files. Later updates rebuild automatically."** - Button: "First build complete" → **"Build complete"** - Dev-server gate alert: "complete the first full build" → "complete the full build" Copy-only; no logic changes. Stacked on the update-path branch because the new description's claim ("later updates rebuild automatically") is only true with #111's changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The esbuild output at src/renderer/index.js and index.css was committed so the app could run without a build step. Regenerating it was a manual step at the end of any renderer change, and forgetting it was silent: the file is not reviewed (2.6 MB of generated output), the linter ignores it, and no job compared it against a fresh build. It had drifted. Rebuilding on a clean `npm ci`, with the committed lockfile and the same esbuild, produced 323 insertions and 177 deletions — 33 hunks from index.jsx, 4 from setup-steps.cjs, 1 from update-plan.cjs, and ~60 inside vendored @wordpress/* modules. Most visibly, the `isUpdating` flag added by the update path (#111) was absent, so the Install, Build and Dev buttons were not gated while a site update ran. The signed artifacts were never affected: all three Buildkite steps already ran `npm run build:once` before `npm run dist`. That left the committed file with no consumer in the shipping path — pure liability. So delete it and make the build unskippable, rather than adding CI to police a file nobody needs. `npm install` is already mandatory (main.js needs electron-store, isomorphic-git, ...), so hanging the build off the existing entry points costs contributors nothing. - The output is gitignored and built by `postinstall`, `start` and every `dist` script. Prefixing `start` also closes a latent race: `concurrently` could hand Electron a bundle the first watch build had not written yet, which until now the committed file happened to mask. - Buildkite's separate "Build renderer" steps go, so the build lives in one place and covers a local `npm run dist` too. - src/renderer/bundle.js and bundle.css are removed. Nothing loaded them; they were left over from when the output had a different name. src/renderer/index.js stays in the eslint ignores. It is no longer committed, but it still sits next to its own source after any build — dropping the ignore would lint 55k lines of generated code on a developer machine and nothing on a fresh CI checkout. Fixes #120 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The esbuild output at src/renderer/index.js and index.css was committed so the app could run without a build step. Regenerating it was a manual step at the end of any renderer change, and forgetting it was silent: the file is not reviewed (2.6 MB of generated output), the linter ignores it, and no job compared it against a fresh build. It had drifted. Rebuilding on a clean `npm ci`, with the committed lockfile and the same esbuild, produced 323 insertions and 177 deletions — 33 hunks from index.jsx, 4 from setup-steps.cjs, 1 from update-plan.cjs, and ~60 inside vendored @wordpress/* modules. Most visibly, the `isUpdating` flag added by the update path (#111) was absent, so the Install, Build and Dev buttons were not gated while a site update ran. The signed artifacts were never affected: all three Buildkite steps already ran `npm run build:once` before `npm run dist`. That left the committed file with no consumer in the shipping path — pure liability. So delete it and make the build unskippable, rather than adding CI to police a file nobody needs. `npm install` is already mandatory (main.js needs electron-store, isomorphic-git, ...), so hanging the build off the existing entry points costs contributors nothing. - The output is gitignored and built by `postinstall`, `start` and every `dist` script. Prefixing `start` also closes a latent race: `concurrently` could hand Electron a bundle the first watch build had not written yet, which until now the committed file happened to mask. - Buildkite's separate "Build renderer" steps go, so the build lives in one place and covers a local `npm run dist` too. - src/renderer/bundle.js and bundle.css are removed. Nothing loaded them; they were left over from when the output had a different name. src/renderer/index.js stays in the eslint ignores. It is no longer committed, but it still sits next to its own source after any build — dropping the ignore would lint 55k lines of generated code on a developer machine and nothing on a fresh CI checkout. Fixes #120 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #94.
Sites were pinned forever to the trunk of the day they were cloned — the shallow single-branch clone was never fetched again, so patches aged against a frozen snapshot and stopped applying on Trac. This implements the core update path from the issue; replay-on-top and the conflict UI (mockups 5–6) are deferred to a follow-up.
Trunk age, everywhere it matters
readCommitof HEAD — offline, no network probe). The site header shows "trunk as of <date>" next to the created date.Update to latest trunk — the whole chain, as the issue frames it
git:update-trunkIPC streaming over anupdateId, same idiom asinstallId): depth-1 re-fetch, index hygiene for files a previous patch generation left staged,writeRef+ forced checkout. Untracked local files survive.package-lock.jsonchanged between the two tips (blob-oid comparison before touching the worktree). The skipped step is named — "Dependencies unchanged — skipping npm install" — not hidden.Steps 2–3 reuse the existing npm machinery (engine retry, exit-code signaling, kill), chained in the renderer exactly like the dev-server start.
Dirty trees and failure states
.diff, nothing sent to Trac) or Discard changes. Saving then updates on a clean tree.One deliberate deviation from the issue text
The issue suggests diffing patches against the remote trunk ref. This PR keeps the diff base at local HEAD: diffing local edits against a moved remote trunk would embed reversed upstream changes and foreign context into the patch, and it would apply nowhere. The route to Trac-applicable patches is the staleness notice + the update action, after which HEAD == origin/trunk and the two definitions coincide. This is documented at the diff site, and CLAUDE.md's wrong claim (that the base was
origin/trunk) is corrected.Testing
src/renderer/update-plan.cjs,src/git-update.cjs) per the repo convention; 25 new tests, 138/138 passing.git fetch --depth=1 origin <old-oid> && git update-ref refs/heads/trunk <old-oid> && git checkout -f trunk, relaunch, and run the update — once with a lockfile-changing gap (install runs) and once without (skip message).🤖 Generated with Claude Code