Comfy-Org / Comfy-Org/Comfy-Desktop
preload: build-time chunk inlining so we can re-enable sandbox without source duplication
- 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.