fix(desktop): align the Electron version across every place it is pinned - #171
Merged
Merged
Conversation
`apps/desktop/package.json` declared `electron: 41.10.3` while `build.electronVersion` still read `40.10.2`. Those fields do different jobs: the dependency plus the lockfile decide what `npm ci` installs — what tests run against and what `electron .` uses in dev — while `build.electronVersion` decides which Electron dist electron-builder packages. The shipped app was therefore a different Electron from the tested one, so code using a 41-only API would pass CI and fail in the packaged build. `desktop-electron-pin.test.ts` exists to catch this and was red on main. There was a fourth reference no test covered: root `package.json` `allowScripts` is keyed by `name@version` and still said `electron@40.10.2`. Electron's postinstall is what fetches the binary, so a stale key means the allowlist silently stops covering it. Nothing in this repo reads the field — it appears only as data in the root and website manifests — which is exactly why the drift went unnoticed, and is worth a look from whoever owns the external consumer. Aligned up, to what is actually installed. Aligning down would mean reverting the dependency and regenerating the lockfile, which is larger and could not be resolved offline here; up is also the safer direction on the merits, since it ships what is tested. A fourth assertion now pins the allowScripts key so the next bump cannot leave it behind. It returns early when there is no electron key at all, since not pinning there is a choice rather than drift. Verified without node_modules — installing the monorepo would drag in the known react/react-dom skew, and this test reads only two JSON files, so its assertions were executed directly in Node against the real files. Before: 2 pass, 1 fail. After: 4 pass. Positive control: reverting only the allowScripts line fails only the new assertion. Corrects a claim I published in #156 and the 2026-08-08 log: I attributed this mismatch to #155, which in fact bumped only dompurify and js-yaml. `git log` pointed there because this clone is shallow and that commit is the graft boundary, so the whole file reads as added in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
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.
Fixes the red
desktop-electron-pintest onmain.The defect
apps/desktop/package.jsondeclared the dependency aselectron: 41.10.3whilebuild.electronVersionstill read40.10.2. Those fields do different jobs:npm ciinstalls — what tests run against, and whatelectron .uses in dev;build.electronVersiondecides which Electron dist electron-builder downloads and packages.So the shipped app was a different Electron from the tested one. Code using a 41-only API would pass CI and fail in the packaged build.
desktop-electron-pin.test.tswas written for exactly this drift and is red onmain.A fourth reference, which no test covered
Root
package.jsoncarriesallowScripts, keyed byname@version, and it still saidelectron@40.10.2. Electron's postinstall is what fetches the actual binary, so a stale key means the allowlist silently stops covering it.Nothing in this repo reads that field — no lavamoat dependency, no script; it appears only as data in the root and
website/manifests. That is precisely why the drift went unnoticed, and it's the part most worth a second opinion from whoever owns the external consumer.Fix
Aligned up, to what is actually installed:
apps/desktop/package.jsondependency41.10.3apps/desktop/package.jsonbuild.electronVersion40.10.241.10.3package.jsonallowScriptselectron@40.10.2electron@41.10.3package-lock.jsonresolution41.10.3Aligning down instead would mean reverting the dependency and regenerating the lockfile — larger, and the resolution couldn't be verified offline here. Up is also the safer direction on the merits: ship what you test.
A fourth assertion now pins the
allowScriptskey so the next bump can't leave it behind. It returns early when there's no electron key at all, since not pinning there is a choice rather than drift.Verification
There's no
node_modulesin this sandbox, and installing the monorepo would drag in the knownreact/react-domskew. This test reads only two JSON files and imports nothing at runtime, so I executed its assertions directly in Node against the real files rather than claiming a vitest run I didn't do:Positive control: reverting only the
allowScriptsline fails only the new assertion.node --experimental-strip-types --checkparses the modified test file.A correction to something I published
In #156's body and in
docs/system-log/2026-08-08.mdI wrote that this mismatch "arrived in7e38fa5(#155, a dependabot bump)". That was wrong. #155 bumpeddompurifyandjs-yamland never touched electron.git log -Lpointed at it only because this clone is shallow and7e38fa5is the graft boundary, so the whole file reads as added there. When the drift actually entered isn't determinable from this clone. The correction is recorded in the new log entry.Follow-up not in this PR
The native-extract-zip install path is already live. The lockfile resolves
@electron/get@5.0.0and@electron-internal/extract-zip@1.0.4— the napi path whose win32-x64 binding is what the pin test's own header blames forERR_DLOPEN_FAILEDon some Windows hosts. That arrived with the 41.x dependency and is unchanged here: this PR doesn't add the risk, but it doesn't remove it either. Worth a Windows install check before the next release.Generated by Claude Code