Comfy-Org / Comfy-Org/Comfy-Desktop

preload: build-time chunk inlining so we can re-enable sandbox without source duplication

Open
#521 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
458
Forks
59
Avg merge
22h 18m
Merged PRs (30d)
45

Description

## Context

PR #519 hit a wall when Rollup split a shared preload module (`src/preload/api.ts`) into a chunk file (`out/preload/chunks/api-*.js`). Electron 40 defaults preloads to `sandbox: true`, and sandboxed preloads can only `require()` from a whitelist (`electron`, `events`, `timers`, `url`). The chunk `require()` failed silently, leaving `window.api` / `window.__comfyTitleBar` undefined and every renderer blank.

## Current workaround

`#519` resolves the immediate issue with **Option 1**:

- Disable `sandbox` on the host webPreferences that load preloads using the api builder (title-bar `WebContentsView` + panel `WebContentsView`).
- Keep `contextIsolation: true` and `nodeIntegration: false` — the real wall between renderer JS and Node — unchanged.
- Restore `src/preload/api.ts` as the single source of truth so we don't duplicate ~330 lines across two preloads.

This matches the historical posture of VS Code, Slack, Teams, Notion, etc., and removes the duplication, but it is one step back from the post-Electron-20 hardening trend.

## Proposed upgrade (this issue)

**Option 2: build-time chunk inlining for preloads.**

Write a tiny Vite/Rollup plugin (`scripts/inline-preload-chunks.ts` or similar) that, after Rollup emits preload bundles:

1. For each preload entry in `out/preload/*.js`, scan for `require("./chunks/...")` calls.
2. Read the referenced chunk source from `out/preload/chunks/`.
3. Replace the `require()` call with the chunk's exported namespace inlined into the entry.
4. Delete the `out/preload/chunks/` directory.
5. Verify source maps still resolve to original `src/preload/*.ts` for breakpoints / stack traces.

This lets us restore `sandbox: true` on every preload-using webContents while keeping `src/preload/api.ts` as a single source of truth — best of both worlds.

### Acceptance criteria
- [ ] `out/preload/chunks/` does not exist after `pnpm run build`.
- [ ] Each `out/preload/*.js` is self-contained (no `require()` of relative file paths).
- [ ] All host webPreferences that load preloads can re-enable `sandbox: true`.
- [ ] Source maps in DevTools point at original `.ts` source for breakpoints in inlined code.
- [ ] CI test asserts no chunks directory is produced (regression guard).
- [ ] App boots, telemetry events still flow per #519's title-bar relay architecture.

### Notes
- The plugin needs to handle both `import { x } from './api'` (ESM-style) and the CJS `require()` Rollup currently emits for preloads.
- Future preloads that import third-party packages may produce additional chunks; the plugin should inline those too or fail loudly.
- Defense-in-depth value: re-enabling sandbox specifically protects against supply-chain compromise of preload-transitive deps. Currently low risk (preloads import only `electron` + `webUtils`) but the trend is toward sandbox-on.

### Out of scope
- Migrating `comfyPreload.js` (the body-view preload) — already self-contained.
- Migrating `comfyTitleMenuPreload.js` — already self-contained.

## References
- PR #519 (where Option 1 lands)
- Issue #515 (the original telemetry investigation that surfaced this)
- [Electron sandbox docs](https://www.electronjs.org/docs/latest/tutorial/sandbox)

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.