upgrade to Electron 41, Node 22 CI, and fix coverage SPA cache#1700
upgrade to Electron 41, Node 22 CI, and fix coverage SPA cache#1700brdandu wants to merge 8 commits into
Conversation
brdandu
requested review from
dhchandw,
ethanzhouyc,
paulr34 and
tecimovic
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades Electron to version 41 and updates the minimum Node.js requirement to v20.9.0, with documentation recommending v22.12+ for compatibility. Key changes include consolidating Istanbul coverage instrumentation, updating build dependencies like @electron/rebuild, and refactoring window management to support Electron 41's title bar and event handling changes. Review feedback highlights a potential memory leak in a global IPC listener, a security regression caused by disabling the renderer sandbox, and an inconsistency between the engine requirements and the project documentation.
brdandu
changed the title
Body:
Bump Electron to ^41.2.2 and refresh related tooling: electron-builder ^26.8.1, electron-debug ^4.1.0, electron-devtools-installer ^4.0.0, node-gyp ^12.3.0.
Replace electron-packager with npm:@electron/packager@18.4.4 and add @electron/rebuild ^4.0.4 (remove legacy electron-rebuild package).
Add coverage-istanbul-loader for Webpack 5–compatible Cypress coverage.
Add overrides for @types/node 20.14.15 to stay compatible with the project’s TypeScript version.
Raise engines.node to >=20.9.0 and set package.json version to 2026.4.22 (regenerate package-lock.json).
CI: use Node.js 22.x in .github/workflows/release.yml, matter.yml, and zigbee.yml (matrix and/or setup-node where applicable).
quasar.conf.js: use coverage-istanbul-loader instead of istanbul-instrumenter-loader; drop the Vue rule that ran Istanbul before vue-loader (avoids treating SFC non-script parts as JS); switch packager afterCopy to import('@electron/rebuildhen(({ rebuild }) => …); refresh packager doc link.
babel.config.js: apply babel-plugin-istanbul only once when NODE_ENV=test OR CYPRESS_COVERAGE (avoid duplicate plugin when both are set).
src-electron/ui/window.js: remove obsolete worldSafeExecuteJavaScript; set explicit contextIsolation: true and sandbox: false; adjust macOS vs Windows title bar / titleBarOverlay / setTitleBarOverlay handling; add did-fail-load logging; support Electron 35+ console-message shape; add render-process-gone logging; optional DevTools when ZAP_DEVTOOLS=1.
src-electron/util/env.js: extend versionsCheck() for Node 22/24 and restrict expected Electron to 39.x / 40.x / 41.x.
src-script/script-util.js: persist a coverage flag in spa/hash.json and force SPA rebuild when coverage vs non-coverage runs flip (prevents reusing an Istanbul-instrumented SPA under Electron nodeIntegration: false).
Docs: development-instructions.md — recommend Node 22.12+ and note @electron/rebuild / EBADENGINE on older Node; faq.md — document npx elerebuild with @electron/rebuild.
- Github: ZAP project-chip#1502
- bump GitHub Actions workflows from Node 22 to Node 24 - raise the app engine requirement to Node >=24.0.0 and refresh docs - update the lockfile after reinstalling dependencies - harden Electron window IPC cleanup for title bar overlay handling - make zap-uitest shut down the dev server reliably on success and failure - Github: ZAP project-chip#1502
brdandu
force-pushed
the
improvement/updateElectronAndNodeVersion/ZAP#1502
branch
from
c898097 to
7704036
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the project's environment requirements and toolchain, notably increasing the minimum Node.js version to v24 and upgrading Electron to version 41. It refactors Istanbul coverage instrumentation to prevent duplicates, improves Electron window management with enhanced error logging, and ensures SPA rebuilds occur when coverage settings change. Additionally, the UI test execution script was refactored for better process management. Review feedback highlighted critical versioning discrepancies, specifically noting that Node.js v24 and Electron v41 are not current stable releases, which contradicts the PR's stated objectives. Concerns were also raised regarding UI regressions on macOS due to title bar style changes and inconsistencies in the updated documentation.
| }, | ||
| "engines": { | ||
| "node": ">= 8.9.0", | ||
| "node": ">=24.0.0", |
There was a problem hiding this comment.
There is a significant discrepancy between the engines.node requirement (>=24.0.0) and the PR title/description which mention Node 22. Furthermore, Node 24 is not yet a stable/LTS release (Node 22 is the current LTS). If the CI environment is configured to use Node 22 as stated in the description, this requirement will cause installation failures due to engine incompatibility. Please verify if this should be >=22.0.0 or >=20.9.0 as mentioned in the PR body.
| "electron-builder": "^24.0.0", | ||
| "electron-debug": "^4.0.0", | ||
| "electron-devtools-installer": "^3.2.0", | ||
| "electron": "^41.2.2", |
There was a problem hiding this comment.
Upgrading to Electron ^41.2.2 appears to be a typo. The current stable version of Electron is in the 30s (e.g., v33). An upgrade to version 41 would be a jump of many major versions that do not yet exist in the stable channel. Please check if the intended version was 31.2.2 or similar, as this also affects the toolchain compatibility.
| 'v22.x.x', | ||
| 'v24.x.x' | ||
| ] | ||
| let expectedElectronVersion = ['39.x.x', '40.x.x', '41.x.x'] |
There was a problem hiding this comment.
The expectedElectronVersion array includes versions 39.x.x, 40.x.x, and 41.x.x. As noted in the package.json review, Electron 41 is not yet a stable release. Please verify these version numbers against the intended Electron upgrade target to ensure the versionsCheck() utility provides accurate warnings to users.
| This section lists instructions for various things you might need to do in this repo. | ||
|
|
||
| This is a node.js application, so you need node environment installed. The best way is to simply download latest install of [node](https://nodejs.org/en/download/) and you will get node and npm. If you have an older version of node installed on your workstation, it may give you trouble, particularly if it's very old. So make sure you have latest node v16.x, v18.x or v20.x version, with the npm that comes with it available. Run `node --version` to check what version is picked up. v20.x is recommended. | ||
| This is a node.js application, so you need node environment installed. The best way is to simply download latest install of [node](https://nodejs.org/en/download/) and you will get node and npm. If you have an older version of node installed on your workstation, it may give you trouble, particularly if it's very old. For building the Electron UI and running the same toolchain as CI, use **Node.js v24.0 or newer** (GitHub Actions uses `24.x`). Older versions may still run parts of the stack but can show `EBADENGINE` warnings for `@electron/rebuild` and related packages. Run `node --version` to check what version is picked up. |
There was a problem hiding this comment.
The documentation recommends Node.js v24.0 or newer, which contradicts the PR title ("Node 22 CI") and the description ("CI: use Node.js 22.x"). This should be updated to align with the actual supported Node version for the project to avoid confusing contributors.
| titleBarStyle: process.platform === 'darwin' ? 'hidden' : 'default', | ||
| trafficLightPosition: { x: 15, y: 20 }, | ||
| titleBarOverlay: { | ||
| titleBarStyle: 'default' |
There was a problem hiding this comment.
Changing titleBarStyle to 'default' globally restores the standard native title bar on macOS, which was previously set to 'hidden'. While the comment explains this avoids a blank page bug in newer Electron versions when combined with titleBarOverlay, it significantly alters the UI appearance on macOS. If the custom title bar look is still desired, consider applying this change conditionally for macOS only when necessary, or verify if the bug persists with other configurations.
- Bump package version and refresh package-lock.json for dependency updates. - Update GitHub Actions (matter, release, zigbee) and Jenkinsfile for the new Node/runtime expectations. - Document Node setup in docs/ZAP-on-Windows.md and docs/development-instructions.md. - Husky: use in postinstall instead of deprecated ; drop unused package.json husky.hooks (hooks live in .husky/pre-commit); remove deprecated shim from pre-commit. - Simplify canvas rebuild in postinstall (drop --update-binary). - Github: ZAP project-chip#1502
- Upgrade axios to ^1.x and sqlite3 to ^6.x for security / audit fixes. - Add rollup@4.20.0 as a direct devDependency so optional @rollup/* platform packages are recorded in package-lock.json; avoids npm ci EUSAGE on CI when rollup was only a transitive peer. - ZAP project-chip#1502
- Replace crc with crc-32 and preserve historical unsigned 32-bit values so CRCs persisted in existing databases continue to compare equal. crc@4 pulled an optional buffer peer that caused npm install and npm ci to disagree on tree shape. - Drop the direct rollup devDependency; vite/workbox-build resolve rollup transitively as a single deduped version. - Bump typescript from 4.6 to ^5.4.5 so tsc can parse @types/babel__traverse, which now uses the syntax introduced in TS 4.7. - Regenerate package-lock.json so optional native packages (e.g. @rollup/*) and the dependency tree match clean installs on Linux runners. - Github: ZAP project-chip#1502
1 participant
Bump Electron to ^41.2.2 and refresh related tooling: electron-builder ^26.8.1, electron-debug ^4.1.0, electron-devtools-installer ^4.0.0, node-gyp ^12.3.0. Replace electron-packager with npm:@electron/packager@18.4.4 and add @electron/rebuild ^4.0.4 (remove legacy electron-rebuild package). Add coverage-istanbul-loader for Webpack 5–compatible Cypress coverage. Add overrides for @types/node 20.14.15 to stay compatible with the project’s TypeScript version. Raise engines.node to >=20.9.0 and set package.json version to 2026.4.22 (regenerate package-lock.json). CI: use Node.js 22.x in .github/workflows/release.yml, matter.yml, and zigbee.yml (matrix and/or setup-node where applicable). quasar.conf.js: use coverage-istanbul-loader instead of istanbul-instrumenter-loader; drop the Vue rule that ran Istanbul before vue-loader (avoids treating SFC non-script parts as JS); switch packager afterCopy to import('@electron/rebuildhen(({ rebuild }) => …); refresh packager doc link. babel.config.js: apply babel-plugin-istanbul only once when NODE_ENV=test OR CYPRESS_COVERAGE (avoid duplicate plugin when both are set). src-electron/ui/window.js: remove obsolete worldSafeExecuteJavaScript; set explicit contextIsolation: true and sandbox: false; adjust macOS vs Windows title bar / titleBarOverlay / setTitleBarOverlay handling; add did-fail-load logging; support Electron 35+ console-message shape; add render-process-gone logging; optional DevTools when ZAP_DEVTOOLS=1. src-electron/util/env.js: extend versionsCheck() for Node 22/24 and restrict expected Electron to 39.x / 40.x / 41.x. src-script/script-util.js: persist a coverage flag in spa/hash.json and force SPA rebuild when coverage vs non-coverage runs flip (prevents reusing an Istanbul-instrumented SPA under Electron nodeIntegration: false). Docs: development-instructions.md — recommend Node 22.12+ and note @electron/rebuild / EBADENGINE on older Node; faq.md — document npx elerebuild with @electron/rebuild.