fix(desktop): renderer boot error boundary and electron hardening#2816
fix(desktop): renderer boot error boundary and electron hardening#2816charlesvien wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/main.tsx:97-99
When `boot(container)` rejects, the `.catch()` only logs the error but the already-mounted React tree continues to run. Users either see a broken app or, if the boot failure cascades into a render crash, a `BootErrorBoundary`-generated message that doesn't match the root cause. The synchronous failure path (the outer `catch`) correctly renders `BootErrorScreen`; the async path should do the same.
```suggestion
boot(container).catch((error: unknown) => {
bootLog.error("Renderer boot sequence failed", error);
root.render(<BootErrorScreen error={error} />);
});
```
Reviews (1): Last reviewed commit: "harden renderer boot and electron securi..." | Re-trigger Greptile |
a8670ec to
9943d59
Compare
7822906 to
6c5ac61
Compare
|
Reviews (2): Last reviewed commit: "harden renderer boot and electron securi..." | Re-trigger Greptile |
There was a problem hiding this comment.
No showstoppers. Security hardening (nodeIntegration: false) is correct given contextIsolation: true and a preload script were already in place. The boot error boundary and async failure handling are well-structured fixes. The resolved bot comment was addressed.
6c5ac61 to
8272637
Compare
9943d59 to
908dceb
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
No showstoppers. The nodeIntegration: false hardening is correct given contextIsolation: true and a preload script are already in place. The boot error boundary is well-structured, and the previously flagged greptile concern about the async boot() failure path not rendering the error screen has been addressed in the current diff.
908dceb to
9e7ce55
Compare
8272637 to
8ee15e2
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
8ee15e2 to
769293d
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Security hardening (nodeIntegration: false) is correct with contextIsolation: true and a preload script already in place. The error boundary is well-structured, async boot failure now properly renders the error screen, and the greptile-flagged concern is addressed.
769293d to
d311bc4
Compare
9e7ce55 to
800c58e
Compare
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
There was a problem hiding this comment.
Security hardening (nodeIntegration: false) is correct — contextIsolation: true and a preload script were already in place, making this a fix to a contradictory config. The error boundary and async boot failure handling are well-structured additions with proper test coverage, and the greptile-flagged async path concern is addressed.

Problem
Two reliability and security gaps from the package split surfaced, deliberately split out from #2813 because they touch runtime-sensitive Electron config and warrant their own review.
boot(), a module-scope import or an Inversify resolution throws during renderer startup, the user gets a blank window with no error and no recovery short of force-quitting.nodeIntegration: true, which applies in production, not just dev.Changes
BootErrorBoundary(apps/code/src/renderer/components/BootErrorBoundary.tsx) around the root React tree, plus atry/catcharound the synchronous bootstrap inmain.tsxand a.catchon the asyncboot(container). Any of those failing now renders a minimal, theme-independent error screen with a Reload button instead of a blank window, and logs the failure.webPreferences.nodeIntegration: falseon the main window.contextIsolation: trueis already set (so the renderer main world never had Node anyway) and the renderer is Vite-bundled with zeronode:*/requireusage in the main world, so this is defense in depth with no functional change.Scope note: the dev-only
webSecurity: falseoverride is left in place. It does not affect production (production already runs withwebSecurity: true) and flipping it risks breaking dev resource loading. The remaining dev/prod parity gap is deferred.How did you test this?
All run locally in the branch worktree:
pnpm typecheck: 22/22 packages passpnpm lint: cleanpnpm test:apps/code151 passnode scripts/check-host-boundaries.mjs: no new violationsNot yet done: a manual smoke test of the running app (dev and packaged). The
nodeIntegrationflip is low risk givencontextIsolation: true, but it changes runtime Electron config, so a human should confirm the window still loads and agents/terminals work before merge. This PR is left as a draft for that reason.Automatic notifications