ci: install Electron runtime in the e2e job - #447
Conversation
The setup job installs with --ignore-scripts (to avoid rebuilding better-sqlite3 for Electron in the lint/test/typecheck jobs), so Electron's postinstall — which downloads the runtime binary — never runs, and the e2e job restored that binary-less cache. Every e2e test then failed with "electron.launch: Electron failed to install correctly". The e2e job now runs `pnpm install --frozen-lockfile` (with scripts) after restoring the cache, completing the skipped postinstalls (Electron binary + electron-builder install-app-deps) before launching the real app. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CI ChangesE2E stability
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 178-185: The Electron postinstall completion step in the CI
workflow is using a second pnpm install, which can still skip pending scripts on
the restored node_modules tree. Replace that step with pnpm rebuild --pending so
the Electron and better-sqlite3 hooks rerun after the setup job’s ignore-scripts
install; keep the change in the workflow job that follows setup and targets the
existing “Complete install for Electron” step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e1a7f18-67a6-4d38-9ea9-f2b5acd09d26
📒 Files selected for processing (1)
.github/workflows/ci.yml
| # The `setup` job installs with --ignore-scripts, so Electron's postinstall | ||
| # (which downloads the runtime binary) and the desktop postinstall | ||
| # (electron-builder install-app-deps → better-sqlite3 rebuilt for Electron) | ||
| # never ran. e2e launches the real Electron app, so complete the install | ||
| # here — otherwise electron.launch fails with "Electron failed to install". | ||
| - name: Complete install for Electron (run postinstall scripts) | ||
| run: pnpm install --frozen-lockfile | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for pnpm build-script allowlist config and pnpm version pin.
fd -H -a 'pnpm-workspace.yaml' -x cat -n {}
rg -n 'onlyBuiltDependencies|allowBuilds|ignoredBuiltDependencies|neverBuiltDependencies|strictDepBuilds' -g '*.json' -g '*.yaml' -g '*.yml'
rg -n '"packageManager"' package.json
rg -n 'pnpm/action-setup@' .github/workflows/ci.ymlRepository: dripnex/readide
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## workflow excerpt\n'
sed -n '1,240p' .github/workflows/ci.yml | nl -ba | sed -n '150,210p'
printf '\n## root package.json\n'
if [ -f package.json ]; then
cat -n package.json | sed -n '1,220p'
fi
printf '\n## desktop package.json\n'
if [ -f apps/desktop/package.json ]; then
cat -n apps/desktop/package.json | sed -n '1,220p'
fi
printf '\n## pnpm metadata\n'
if [ -f pnpm-workspace.yaml ]; then
cat -n pnpm-workspace.yaml
fi
if [ -f pnpm-lock.yaml ]; then
rg -n 'packageManager|pnpm|postinstall|better-sqlite3|electron-builder' pnpm-lock.yaml
fiRepository: dripnex/readide
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
num() { awk '{printf "%6d %s\n", NR, $0}'; }
printf '\n## workflow excerpt\n'
sed -n '150,210p' .github/workflows/ci.yml | num
printf '\n## root package.json\n'
if [ -f package.json ]; then
sed -n '1,220p' package.json | num
fi
printf '\n## desktop package.json\n'
if [ -f apps/desktop/package.json ]; then
sed -n '1,220p' apps/desktop/package.json | num
fi
printf '\n## pnpm-workspace.yaml\n'
if [ -f pnpm-workspace.yaml ]; then
sed -n '1,120p' pnpm-workspace.yaml | num
fi
printf '\n## pnpm-lock.yaml matches\n'
if [ -f pnpm-lock.yaml ]; then
rg -n 'packageManager|pnpm|postinstall|better-sqlite3|electron-builder' pnpm-lock.yaml
fiRepository: dripnex/readide
Length of output: 15091
🌐 Web query:
pnpm 9 ignore-scripts rebuild --pending install rerun postinstall official docs
💡 Result:
In pnpm 9 and later, when you run an installation with the --ignore-scripts flag, lifecycle scripts (such as postinstall) are not executed for your project or its dependencies [1][2]. To rerun or execute these pending scripts after an installation with --ignore-scripts, you can use the pnpm rebuild command [3][4]. Key details regarding this process: 1. Executing Rebuild: Running pnpm rebuild will attempt to execute lifecycle scripts for packages present in node_modules [4]. If scripts were skipped during the initial install, they are considered pending and should be processed by the rebuild command [4]. 2. Pending Builds and Approvals: If you are using pnpm's security features to control script execution, you may need to approve builds for specific dependencies before they can run [5]. You can use the pnpm approve-builds command to interactively manage these permissions [5]. 3. Limitations and Troubleshooting: - If pnpm rebuild does not trigger the scripts as expected, ensure that the dependencies are not being excluded by your configuration (e.g., in.npmrc) [6]. - Be aware of the side-effects-cache setting [6][7]. If enabled, pnpm may skip running scripts for packages it believes have already been built and cached [6][7]. Setting side-effects-cache=false in your.npmrc can force pnpm to re-run build scripts [6][7]. - There are known edge cases, such as in certain monorepo configurations or when installing only development dependencies, where pnpm rebuild may behave inconsistently [3]. For further details on managing scripts, refer to the official pnpm documentation on scripts [8] and supply chain security [9].
Citations:
- 1: https://pnpm.io/cli/install
- 2: https://pnpm.io/10.x/cli/install
- 3:
pnpm rebuilddoesn't work when ran afterpnpm install --ignore-workspace --devpnpm/pnpm#10538 - 4: [wontfix] fix(rebuild): include all dependency types regardless of NODE_ENV pnpm/pnpm#10588
- 5: https://pnpm.io/next/cli/approve-builds
- 6: [pnpm v7] postinstall not running pnpm/pnpm#4649
- 7: pnpm doesn't run postinstall scripts of dependencies on Windows pnpm/pnpm#7482
- 8: https://pnpm.io/scripts
- 9: https://pnpm.io/supply-chain-security
Use pnpm rebuild --pending here instead of a second install.
setup restores node_modules from an --ignore-scripts install, and pnpm install --frozen-lockfile can leave electron/better-sqlite3 postinstall hooks skipped on the restored tree. pnpm rebuild --pending is the step that reruns those scripts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 178 - 185, The Electron postinstall
completion step in the CI workflow is using a second pnpm install, which can
still skip pending scripts on the restored node_modules tree. Replace that step
with pnpm rebuild --pending so the Electron and better-sqlite3 hooks rerun after
the setup job’s ignore-scripts install; keep the change in the workflow job that
follows setup and targets the existing “Complete install for Electron” step.
The prior fix ran `pnpm install --frozen-lockfile`, which re-ran the desktop postinstall (native rebuild) but not electron's own install script — the package is already "installed", so its postinstall (which downloads the runtime binary) is skipped and electron.launch still fails. Add `pnpm rebuild electron` to force-run electron's install script and fetch the binary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 187-190: The Electron install step in the CI workflow is doing a
full-tree pnpm install just to trigger apps/desktop postinstall and Electron’s
native rebuild. Update the step to use a scoped install or pending rebuild
approach tied to the `@dripnex/desktop` workspace, and keep the electron rebuild
call so the needed binary/native modules are still installed without
reprocessing the entire dependency tree.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a818b4f-dcb3-45d1-a209-201d170cc68c
📒 Files selected for processing (1)
.github/workflows/ci.yml
| - name: Complete install for Electron (binary + native modules) | ||
| run: | | ||
| pnpm install --frozen-lockfile | ||
| pnpm rebuild electron |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Consider scoping the install to avoid full-tree reinstall overhead.
Running a full pnpm install --frozen-lockfile after cache restore re-verifies/re-links the entire dependency tree just to trigger one workspace package's postinstall. Since the goal is specifically to run apps/desktop's postinstall and Electron's install script, a scoped approach (e.g., pnpm --filter @dripnex/desktop install or pnpm rebuild --pending combined with pnpm rebuild electron) could achieve the same effect with less overhead in the e2e job's critical path.
Not blocking given the job is continue-on-error: true, but worth considering since this step runs on every e2e execution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 187 - 190, The Electron install step
in the CI workflow is doing a full-tree pnpm install just to trigger
apps/desktop postinstall and Electron’s native rebuild. Update the step to use a
scoped install or pending rebuild approach tied to the `@dripnex/desktop`
workspace, and keep the electron rebuild call so the needed binary/native
modules are still installed without reprocessing the entire dependency tree.
Restoring the shared --ignore-scripts cache left Electron installed without its runtime binary, and a subsequent install/rebuild was a no-op (package already present), so electron.launch kept failing. The e2e job now skips the cache restore and runs its own `pnpm install --frozen-lockfile` with scripts against fresh node_modules, so electron's install.js downloads the binary and the desktop postinstall rebuilds better-sqlite3 for Electron. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
154-173: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop
needs: setupfrome2e
setuponly seeds the pnpm cache;e2edoes its own fresh install and doesn’t consume any artifact or output from that job. Removing the dependency would let e2e start sooner in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 154 - 173, The e2e job is unnecessarily blocked by the setup job even though it only relies on its own fresh install and does not consume any outputs or artifacts from setup. Update the e2e job in the CI workflow to remove the needs: setup dependency, keeping the existing checkout, pnpm setup, node setup, and Install dependencies (with postinstall scripts) steps unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 154-173: The e2e job is unnecessarily blocked by the setup job
even though it only relies on its own fresh install and does not consume any
outputs or artifacts from setup. Update the e2e job in the CI workflow to remove
the needs: setup dependency, keeping the existing checkout, pnpm setup, node
setup, and Install dependencies (with postinstall scripts) steps unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 307cfe12-2e81-46c8-818d-2b3fcdddb889
📒 Files selected for processing (1)
.github/workflows/ci.yml
The smoke test measured `body` boundingBox right after launchApp, which only waits for `domcontentloaded` — that fires before the first paint/layout, so the body intermittently measured 0×0 and failed `width > 0`. Wait for the body to have non-zero size before asserting; this is a render race, not a blank-window regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/e2e/smoke.spec.ts`:
- Around line 20-29: The smoke test currently waits only for
`body.boundingBox()` width to become non-zero and then re-reads the box, which
can let height still be zero and introduces a race. Update the polling around
`body` to wait for both width and height to be greater than zero, and reuse the
bounding box value from the successful poll instead of calling `boundingBox()`
again. Keep the assertions on the `body` box aligned with the polled result so
`expect(bodyBox)` reflects the same settled layout state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2fea920e-195f-4e88-af54-2e85f8920aaf
📒 Files selected for processing (1)
apps/desktop/e2e/smoke.spec.ts
| const body = window.locator('body'); | ||
| await expect | ||
| .poll(async () => (await body.boundingBox())?.width ?? 0, { | ||
| message: 'body should lay out with non-zero width', | ||
| }) | ||
| .toBeGreaterThan(0); | ||
| const bodyBox = await body.boundingBox(); | ||
| expect(bodyBox).not.toBeNull(); | ||
| expect(bodyBox!.width).toBeGreaterThan(0); | ||
| expect(bodyBox!.height).toBeGreaterThan(0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Poll only guards width; reuse the polled box and also wait on height.
The poll condition only checks width > 0, but height could still be racing to lay out; the subsequent expect(bodyBox!.height).toBeGreaterThan(0) has no retry margin if height lags behind width. Also, bodyBox is re-fetched via a second boundingBox() call instead of reusing the value already obtained during polling.
♻️ Suggested fix
const body = window.locator('body');
- await expect
- .poll(async () => (await body.boundingBox())?.width ?? 0, {
- message: 'body should lay out with non-zero width',
- })
- .toBeGreaterThan(0);
- const bodyBox = await body.boundingBox();
+ let bodyBox: Awaited<ReturnType<typeof body.boundingBox>> = null;
+ await expect
+ .poll(
+ async () => {
+ bodyBox = await body.boundingBox();
+ return bodyBox;
+ },
+ { message: 'body should lay out with non-zero size' },
+ )
+ .toEqual(
+ expect.objectContaining({
+ width: expect.any(Number),
+ height: expect.any(Number),
+ }),
+ );
expect(bodyBox).not.toBeNull();
expect(bodyBox!.width).toBeGreaterThan(0);
expect(bodyBox!.height).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/e2e/smoke.spec.ts` around lines 20 - 29, The smoke test
currently waits only for `body.boundingBox()` width to become non-zero and then
re-reads the box, which can let height still be zero and introduces a race.
Update the polling around `body` to wait for both width and height to be greater
than zero, and reuse the bounding box value from the successful poll instead of
calling `boundingBox()` again. Keep the assertions on the `body` box aligned
with the polled result so `expect(bodyBox)` reflects the same settled layout
state.
1 participant
Problem
e2e has been failing on every PR (verified on an unrelated one-line PR) with:
Root cause: the
setupjob installs deps with--ignore-scripts(intentional — it avoids rebuilding better-sqlite3 for Electron in the lint/test/typecheck jobs). But that also skips Electron's postinstall, which downloads the Electron runtime binary. Thee2ejob restores that binary-lessnode_modulescache and then tries to launch the real Electron app → fails.Fix
The
e2ejob now runspnpm install --frozen-lockfile(with scripts) after restoring the cache, which completes the skipped postinstalls — Electron's binary download andelectron-builder install-app-deps(better-sqlite3 rebuilt for the Electron ABI) — before building and launching the app.Scoped to the e2e job only; the other jobs keep the fast
--ignore-scriptspath.Note
e2e is
continue-on-error: trueand not a required check, so this was silently red rather than blocking merges — but it means e2e has been providing no signal. This restores it.🤖 Generated with Claude Code
Summary by CodeRabbit
pnpm install --frozen-lockfileduring the desktop E2E flow to ensure required native post-install rebuilds complete successfully.