feat: Electron-compat shim + experimental Servo web-engine switch#5811
Open
proggeramlug wants to merge 11 commits into
Open
feat: Electron-compat shim + experimental Servo web-engine switch#5811proggeramlug wants to merge 11 commits into
proggeramlug wants to merge 11 commits into
Conversation
… apps natively
Adds `packages/electron`, a drop-in Electron-compat toolkit, plus the native
support it needs (macOS first). Existing Electron app code (`app`,
`BrowserWindow`, `ipcMain`, `contextBridge`, …) runs unmodified: app logic
compiles native, the view runs in the OS-native webview (WKWebView) — Tauri-model
internals, Electron API surface, no bundled Chromium, no Rust to write.
Native (perry-ui-macos / perry-runtime / perry-codegen / perry-dispatch):
- webview.rs: bidirectional IPC bridge — WKScriptMessageHandler (renderer→main),
document-start user-script injection (add_user_script), set_on_message; file://
loads via loadFileURL:allowingReadAccessToURL: + allowFileAccessFromFileURLs so a
file:// page can load its own subresources.
- app.rs: app_run_loop (body-less event loop resolving whenReady), app_quit,
full-window Auto-Layout pinning in window_set_body, window_show activates +
orderFrontRegardless.
- ui_loop.rs + codegen entry.rs hook: js_ui_loop_take_over runs the UI loop at the
TOP LEVEL of generated main (not from a microtask) — required or windows created
from app.whenReady().then(...) never composite on macOS.
- FFI exports, perry-dispatch table rows, and types/perry/ui/index.d.ts decls.
TS toolkit (packages/electron, a nativeModule package):
- app, BrowserWindow, ipcMain, webContents, Menu, dialog, shell, contextBridge/
ipcRenderer implemented on perry/ui + node:*.
- Renderer-side bridge runtime (bridge/preload-runtime.js): require('electron'),
a CommonJS loader for local renderer requires, ipcRenderer/contextBridge,
console forwarding to the main process.
- examples/system-explorer: a full IPC demo (invoke/handle, send/on, main→renderer
push, fs/os, persisted userData).
Verified end-to-end on macOS: czonios/pomodoro-electron (vanilla) and
electron-vite-react (React + Vite + Tailwind, contextBridge) run unmodified with
rendering windows and working IPC.
Known limits: macOS only so far (Windows/Linux bridge is the same pattern);
dialog/Menu are v1 stubs; JSON-only IPC; renderer-side Node usage (old
nodeIntegration + `remote` apps) is unsupported by design (system webview has no
Node — same boundary as Tauri).
…bs (electron/EventEmitter link); + electron-compat contributor DX fixes Eight items from contributor feedback on the feat/electron-compat branch. PerryTS#7 (the real bug): perry/ui + events apps failed to link from source with `Undefined symbols: _js_event_emitter_*`. build_optimized_libs splits well-known bindings into CPU-only (events -> perry-ext-events.a, resolved immediately into well_known_libs) and tokio-using (deferred). The warm-cache early-return rebuilt the list from the tokio-using set ALONE, dropping every CPU-only wrapper from the link line while the stdlib still stripped bundled-events -> a stdlib calling js_event_emitter_* with no provider. Only bit on a warm auto-optimize cache (cold cache took the fresh path and linked), the intermittent "can't compile from source" report. Fix: cached path now extends well_known_libs with the tokio-resolved set, mirroring the fresh path. Reproduced + verified with the system-explorer example (links + runs). PerryTS#8: perry run . now resolves an entry inside a directory arg (perry.toml entry, src/main.ts, main.ts) instead of erroring 'Is a directory'. PerryTS#3: perry init mirrors perry.packageAliases into tsconfig compilerOptions.paths (create + best-effort merge into an existing tsconfig). PerryTS#4/PerryTS#5: new packages/perry-react (@perryts/react) - bundles @types/react* and augments react-dom/client with createRoot(null, PerryRootOptions). Validated with tsc under moduleResolution bundler + node. PerryTS#1: root package.json gains name/version/private (npm arborist crash). PerryTS#2: packages/electron README documents the install workaround + scoped alias. PerryTS#6: verified webviewAddUserScript is wired on this branch (release-lag only). Version 0.5.1204; changelog updated.
…vate
Several electron-compat surfaces were link errors (nativeImage, clipboard,
Tray) or true no-ops (Menu.setApplicationMenu, dialog.show*Dialog) reported on
the stress test. The native capabilities already exist in perry-ui-macos and
are exposed via perry/ui — this wires the shim over them (no compiler change):
- clipboard.readText/writeText/clear -> clipboardRead/clipboardWrite
- nativeImage.createFromPath/… -> thin wrapper carrying the source path
- Tray (image, setImage, setToolTip, -> trayCreate/traySetIcon/traySetTooltip/
setContextMenu, on('click'), destroy) trayAttachMenu/trayOnClick/trayDestroy
- Menu.setApplicationMenu -> menuBarCreate/menuBarAddMenu/menuBarAttach
(separators, nested submenus, click callbacks, accelerators normalized from
"CmdOrCtrl+S", and roles mapped to AppKit selectors: copy/paste/undo/quit/…)
- dialog.showOpenDialog/showSaveDialog -> openFileDialog/openFolderDialog/saveFileDialog
(accepts both (options) and (window, options) call shapes)
- app emits 'activate' on dock-icon -> onActivate (native ON_ACTIVATE_CALLBACK)
click (event, hasVisibleWindows)
Native-held handles + menu/tray/dialog callbacks are kept in a module-level
root array so GC can't reclaim them. Verified: a program exercising all of the
above compiles and links (the previously-undefined symbols now resolve);
clipboard round-trips at runtime via the perry/ui binding.
Still Tier B (need a new perry/ui binding first, separate change): main-process
Notification, nativeTheme.shouldUseDarkColors, app.dock.setIcon.
…vate, null detach, tray guards CodeRabbit feedback on the Tier A wiring: - dialog open/save callbacks now unroot from the GC-root array once the native side replies (were leaking one-shot closures for the app lifetime). - app 'activate' reports hasVisibleWindows from actual window visibility (BrowserWindow now tracks _visible via show/hide/close and exposes isVisible()); a hidden window no longer suppresses the dock-reopen pattern. - Menu.setApplicationMenu(null) attaches a fresh empty menubar so the previous app menu is cleared instead of left live. - Tray.setContextMenu(null) attaches an empty menu to detach the previous one; setImage/setToolTip/setContextMenu are guarded after destroy(). - nativeImage.createFromDataURL/createFromBuffer warn (unsupported → empty image) instead of silently dropping Tray/dock icons.
…at-servo # Conflicts: # CHANGELOG.md # CLAUDE.md # Cargo.lock # Cargo.toml # crates/perry-dispatch/src/ui_table.rs # crates/perry/src/commands/compile/optimized_libs.rs
… 0.3.2 Servo now compiles as a perry workspace member (servo-webview feature). Two dependency conflicts with perry's tree resolved: - libsqlite3-sys: via the rusqlite 0.37 + sqlx 0.9 upgrade (separate PR). - ml-kem: perry needs 0.3.2+pkcs8 (stable kem); servo-script needs 0.2.x (pre-release kem, no pkcs8). Forked servo-script and migrated its WebCrypto ML-KEM code 0.2 -> 0.3.2. Captured as docs/servo/servo-script-ml-kem-0.3.2.patch and applied via a local [patch.crates-io]. Verified: perry-ui-macos --features servo-webview builds the full Servo stack (servo v0.1.1) cleanly. ml-dsa is not a conflict (perry doesn't use it). The ml-kem migration is type-correct; behavioral crypto correctness needs Servo's WebCrypto vectors (niche surface; rendering unaffected). See docs/servo/README.md. WIP: ServoEngine integration (render into NSView + event loop + toggle) follows. (cherry picked from commit 9464ac517ed4b8808adc6386c954f90865b9f534)
…W=servo) Adds an experimental Servo-backed implementation of the WebView widget, selectable at runtime via PERRY_WEBVIEW=servo (feature `servo-webview`, off by default — the default build still links only the system WKWebView). - servo_webview.rs: ServoEngine renders offscreen via SoftwareRenderingContext (the proven probe path) and blits each frame into a layer-backed NSImageView using NSBitmapImageRep, driven by a shared ~60 Hz NSTimer on the main run loop. Software+blit (not WindowRenderingContext) because perry adds widget NSViews to the window lazily, which would race a GPU context's window-handle requirement. - webview.rs: create() routes to the Servo engine when requested (falling back to WKWebView on engine-construction failure); load_url/reload/evaluate_js/ set_on_loaded dispatch to the engine when the handle is Servo-backed. - evaluate_javascript bridges Servo's JSValue back to the TS callback as a string (matching the WKWebView contract); onLoaded fires on LoadStatus::Complete. Verified: builds with --features servo-webview (full Servo stack) AND the default build is unaffected by the cfg gating. On-screen rendering is GUI-verified. (cherry picked from commit fa2c7944ea44093d8e3cde59e47cfd188dc06907)
Adds a `--webview <system|servo>` flag to `perry compile` (default system). `--webview servo` links the Servo-enabled macOS UI library variant (libperry_ui_macos_servo.a, built with --features servo-webview) so a compiled app defaults to the Servo web engine; PERRY_WEBVIEW still overrides at runtime. Plumbed via a thread-local in library_search (mirroring the other cross-cutting compile flags) read by find_ui_library, which prefers the _servo variant on macOS and warns + falls back to the system WKWebView lib if it isn't present. Ignored on non-macOS targets. (cherry picked from commit e99019e706f754dbca6a3cb8796612ea9b730de2)
The Servo-backed WebView is now interactive. Replaces the passive NSImageView with a PerryServoView (flipped, first-responder NSView subclass) that holds the image view as an autoresizing subview and: - forwards mouse down/up/drag + left/right buttons and the scroll wheel to Servo as InputEvents (coordinates converted to Servo device pixels), and - resizes the engine — rendering context + WebView + readback size — on layout via setFrameSize:, so the page tracks the widget bounds. Reuses webview::nanbox_str (now pub(super)) for the evaluate_js result instead of re-declaring the string FFI. Follow-ups (documented in docs/servo/README.md): keyboard input, hover (NSTrackingArea), and HiDPI backing scale. Builds with --features servo-webview; default build unaffected (cfg-gated). (cherry picked from commit 0d47c463223281761a7fb7a7ce1f24ddbc099b3a)
…ript-mlkem) Point [patch.crates-io] servo-script at the git fork by rev instead of a local path, so the branch resolves on any machine (incl. CI) — the servo optional dep must resolve even when the servo-webview feature is off. Update docs/servo.
📝 WalkthroughWalkthroughAdds an Electron-compatibility package ( ChangesElectron Compat, Servo WebView, and Native Plumbing
CLI Improvements
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant PRELOAD_RUNTIME
participant WKWebView
participant ipcMain
participant ipcHandler
Note over Renderer,PRELOAD_RUNTIME: Injected at document-start
Renderer->>PRELOAD_RUNTIME: ipcRenderer.invoke(channel, ...args)
PRELOAD_RUNTIME->>WKWebView: postToNative({ type:"invoke", id, channel, args })
WKWebView->>ipcMain: _dispatch(message, webContents)
ipcMain->>ipcHandler: registered handler(event, ...args)
ipcHandler-->>ipcMain: result
ipcMain->>WKWebView: evaluate __perryResolve(id, true, resultJson)
WKWebView->>PRELOAD_RUNTIME: __perryResolve(id, true, payloadJson)
PRELOAD_RUNTIME-->>Renderer: Promise resolves with result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jun 30, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/commands/run/entry.rs (1)
108-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThis
entryextraction is too loose to parse TOML safely.
starts_with("entry")will also match keys likeentrypoint, and the string slicing ignores valid TOML constructs such as inline comments or single-quoted strings. A validperry.tomlcan therefore resolve to a bogus path here.🤖 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 `@crates/perry/src/commands/run/entry.rs` around lines 108 - 115, The `read_perry_toml_entry_in` parser is too permissive and can misread non-`entry` keys or TOML values. Replace the manual line scanning and string slicing with real TOML parsing for `perry.toml`, then extract only the exact `entry` field from the parsed structure. Update the logic in `read_perry_toml_entry_in` so it properly handles TOML syntax like comments and quoted strings, and does not match keys such as `entrypoint`.
🧹 Nitpick comments (1)
crates/perry-ui-macos/src/widgets/servo_webview.rs (1)
80-81: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
dirtyto skip static-frame readbacks.
dirtyis maintained buttick()still paints, presents, allocates, and blits every engine every 16ms. Gate the expensive readback/blit path ondirtyso static pages do not burn CPU continuously.Suggested shape
fn tick(&self) { self.servo.spin_event_loop(); + if !self.state.dirty.replace(false) { + return; + } self.webview.paint(); self.rc.present(); let (w, h) = self.size.get(); let rect = DeviceIntRect::from_size(DeviceIntSize::new(w as i32, h as i32)); if let Some(img) = self.rc.read_to_image(rect) { blit_rgba_to_image_view(&self.view, img.as_raw(), img.width(), img.height()); } - self.state.dirty.set(false); }Also applies to: 112-123, 409-429
🤖 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 `@crates/perry-ui-macos/src/widgets/servo_webview.rs` around lines 80 - 81, `dirty` is currently tracked on the webview state but `tick()` still performs the full paint/present/readback/blit path every frame. Update `ServoWebView::tick` (and the related blit/readback helpers it calls) to check `dirty` first and skip the expensive static-frame work when no new frame has been reported, clearing `dirty` only after a successful blit so static pages stop consuming CPU continuously.
🤖 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 `@crates/perry-ui-macos/src/app.rs`:
- Around line 696-719: The deferred `on_ready` callback is only stored in
`PENDING_APP_LOOP_ON_READY`, so it can be collected before `ui_loop_entry` calls
`app_run_loop`. Update `request_app_loop` and `ui_loop_entry` in `app.rs` to
move the callback into a GC-rooted/runtime-managed handle instead of a
`thread_local Cell<f64>`, and ensure it stays pinned until `js_closure_call0`
runs. Use the existing `perry_runtime_register_ui_loop` / `app_run_loop` flow as
the place to wire the rooted handle through.
In `@crates/perry-ui-macos/src/widgets/servo_webview.rs`:
- Around line 54-57: The `is_servo_requested` check only reads `PERRY_WEBVIEW`,
so the compile-time Servo path selected by `--webview servo` can still fall back
to WKWebView unless the env var is set. Update `is_servo_requested` in
`servo_webview.rs` to also honor a baked-in compile-time preference or a UI
runtime setter that the codegen/selection path can call, and wire that setter
into the Servo selection flow so the UI library choice is preserved without
requiring `PERRY_WEBVIEW=servo`.
- Around line 439-468: The RGBA size checks in blit_rgba_to_image_view can
overflow because width * height * 4 is computed before casting, so large
dimensions may wrap and bypass the guard. Update the function to use checked
arithmetic or usize-based size computation for the buffer length, bytes-per-row,
and copy length, and then use the resulting safe size in both the early return
condition and the std::ptr::copy_nonoverlapping call.
In `@crates/perry-ui-macos/src/widgets/webview.rs`:
- Around line 560-568: The Servo-backed path in webview creation can return a
handle, but the Electron-facing API wiring still only looks up WKWebView state
through HANDLE_TO_KEY and webview_for_handle. Update the handle routing in the
relevant webview methods (set_on_message, add_user_script, and related
IPC/preload setup) so Servo handles either get equivalent message/script support
or are explicitly rejected/disabled for Electron windows until those contracts
exist. Ensure the Servo branch in create and the handle-to-webview lookup logic
use the same backend-specific path.
- Around line 620-636: Remove the global `allowUniversalAccessFromFileURLs`
toggle in `webview.rs` and scope file-origin relaxation to the existing
`loadFileURL:allowingReadAccessToURL:` path instead. Keep
`allowFileAccessFromFileURLs` only if needed for local subresources, and update
the WebView configuration setup around `cfg`, `prefs`, and the `NSNumber` KVC
writes so universal file access is not enabled for every `WKWebView`.
In `@crates/perry/src/commands/compile/library_search.rs`:
- Around line 998-1007: The fallback in library_search::find_library for the
macos_servo path should not silently return the WKWebView archive when --webview
servo was explicitly requested. Update the logic around the macOS Servo lookup
to treat missing libperry_ui_macos_servo.a as a build failure instead of
emitting a warning and falling back, so the explicit Servo selection in
find_library is enforced consistently.
In `@crates/perry/src/commands/compile/types.rs`:
- Around line 124-131: The `CompileArgs::webview` option currently accepts any
string and unknown values silently behave like `system` because callers only
compare `args.webview == "servo"`. Update the `webview` argument in `types.rs`
to restrict accepted values to `system` and `servo` (for example by using a
value parser or enum-backed CLI option), and make sure any parsing/validation
path rejects typos instead of falling through. Keep the existing
`CompileArgs`/`webview` symbol names as the place to apply the fix so the
runtime selection logic only sees valid values.
In `@crates/perry/src/commands/init.rs`:
- Around line 305-309: The fast path in init/tsconfig sync is incorrectly
skipping updates when packageAliases is empty, which prevents existing
tsconfig.json files from getting the always-present perry/* path mapping from
tsconfig_paths(). Update the logic in the init command so tsconfig
synchronization still runs even when aliases are empty, and only preserve the
“already exists” skip behavior when there is truly nothing to merge; use the
existing tsconfig_paths() and aliases handling in the init flow to ensure the
default Perry mapping is written for existing projects.
- Around line 161-194: The `merge_paths_into_existing` helper is preserving any
preexisting `compilerOptions.baseUrl`, which breaks the injected `paths` aliases
when a project already sets a different base directory. Update the merge logic
so the generated `paths` entries are only written when the config will resolve
them from the repo root, either by forcing `compilerOptions.baseUrl` to `"."` or
by rejecting/rewriting configs with a conflicting `baseUrl`. Keep the fix
localized in `merge_paths_into_existing` and use the existing
`render_paths_block` fallback behavior for unsupported shapes.
In `@crates/perry/src/commands/run/entry.rs`:
- Around line 86-95: The entry resolution in read_perry_toml_entry_in should not
silently fall back to src/main.ts or main.ts when perry.toml explicitly declares
an entry that cannot be resolved. Distinguish the “no entry configured” case
from an invalid configured entry, and return or propagate an error for the
broken entry path instead of continuing. Update the logic around
read_perry_toml_entry_in and its caller in the run entry flow to surface this
failure clearly.
In `@docs/servo/README.md`:
- Around line 51-59: Update the Servo README wording so it matches the actual
selection behavior in servo_webview and webview: `--webview servo` should be
described as a build-time step that links the Servo-enabled UI library, but
runtime selection still depends on `PERRY_WEBVIEW=servo` with WKWebView as the
fallback. Adjust the “At build time” and “At runtime” bullets to make it clear
that compiling with `perry compile ... --webview servo` does not by itself force
Servo at launch unless the environment variable is also set.
In `@docs/servo/servo-script-ml-kem-0.3.2.patch`:
- Around line 1-2: The patch headers currently use absolute local paths, which
makes the checked-in patch non-portable. Regenerate the patch for
servo-script-ml-kem-0.3.2.patch using repository-relative paths in the diff
headers (for example stable a/... and b/... prefixes) so the apply flow works
consistently in README, CI, and for other contributors. Update all affected diff
hunks in the patch file to use the same relative-path format.
In `@packages/electron/bridge/preload-runtime.js`:
- Around line 150-197: The preload runtime is leaking privileged APIs into the
page global scope by assigning require, electron, and __perryIpcRenderer
directly on window, which exposes ipcRenderer and the CommonJS loader to any
loaded content. Update the preload bridge in makeRequire and the window.*
exports so these capabilities are only exposed through a controlled
preload/bridge API, not as page-accessible globals. Keep the Electron shim
private to the preload context and remove direct global publication unless it is
explicitly mediated through contextBridge.
In `@packages/electron/examples/system-explorer/renderer/renderer.js`:
- Around line 23-25: The sysinfo rendering in renderer.js is building HTML
strings from untrusted IPC/filesystem values, which can lead to injection;
update the row-building logic in the sysinfo display code and the related list
rendering around the same area to use DOM APIs instead of innerHTML. In the
functions that populate `#sysinfo` and the affected rows in the 35-55 range,
create elements with `document.createElement`, assign values with `textContent`,
and use `dataset` or attributes for metadata rather than interpolating `info.*`,
`e.name`, or other dynamic fields into HTML.
In `@packages/electron/package.json`:
- Around line 2-3: The package name is conflicting with the existing public npm
package, so update the package metadata for the electron package to avoid
publishing under the plain electron name. In package.json for the electron
package, either mark it private if it is workspace-only or rename it to a
scoped/non-conflicting name, and keep any require("electron") compatibility
handled separately from the published package name.
In `@packages/electron/src/index.ts`:
- Around line 304-344: The load methods in the BrowserWindow implementation
resolve too early, so callers awaiting loadURL and loadFile can proceed before
navigation is complete. Update the BrowserWindow methods loadURL and loadFile to
return a promise that settles only after the page finishes loading, and wire
that resolution through the existing webContents/webview load lifecycle instead
of resolving immediately. Also address the file:// onLoaded gap noted in the
create-window setup so loadFile can honor the same completion contract as
loadURL.
- Around line 63-76: Serialization in resolveJs(), deliverJs(), and the
webContents.send() path can throw for non-JSON-serializable values like cycles
or BigInt, which leaves ipcRenderer.invoke() and send-based flows hanging.
Update the IPC handling in packages/electron/src/index.ts so JSON.stringify
failures are caught before building the script payload, and route those failures
through the existing error reply path used for rejected IPC calls instead of
allowing the Promise fulfillment or send path to throw. Use the
resolveJs/deliverJs helpers and the invoke/send handler code that calls them to
locate and wrap the serialization step.
In `@packages/perry-react/package.json`:
- Around line 5-10: The package export for "." in
packages/perry-react/package.json only provides types, so a bare import like
import '`@perryts/react`' has no runtime target. Update the package entrypoint so
the existing activation path resolves at runtime by adding a real runtime
export/stub for the main symbol, or remove the bare-import guidance and make the
type-only reference path the supported mechanism. Check the package.json exports
map and the related index.d.ts/package entrypoint wiring to keep runtime and
type resolution aligned.
- Around line 48-50: The React type dependencies in the package manifest are
using a shared union range, which can resolve to the wrong major for consumers.
Update the `@types/react` and `@types/react-dom` entries in the package.json
dependencies to target a single supported major, or move them to
peerDependencies so the consuming app controls the matching typings. Use the
existing dependency keys to locate the entries and split the majors cleanly.
---
Outside diff comments:
In `@crates/perry/src/commands/run/entry.rs`:
- Around line 108-115: The `read_perry_toml_entry_in` parser is too permissive
and can misread non-`entry` keys or TOML values. Replace the manual line
scanning and string slicing with real TOML parsing for `perry.toml`, then
extract only the exact `entry` field from the parsed structure. Update the logic
in `read_perry_toml_entry_in` so it properly handles TOML syntax like comments
and quoted strings, and does not match keys such as `entrypoint`.
---
Nitpick comments:
In `@crates/perry-ui-macos/src/widgets/servo_webview.rs`:
- Around line 80-81: `dirty` is currently tracked on the webview state but
`tick()` still performs the full paint/present/readback/blit path every frame.
Update `ServoWebView::tick` (and the related blit/readback helpers it calls) to
check `dirty` first and skip the expensive static-frame work when no new frame
has been reported, clearing `dirty` only after a successful blit so static pages
stop consuming CPU continuously.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c4dd21c-4061-49f9-aaac-49bb113b2f1f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
Cargo.tomlcrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-dispatch/src/ui_table/part_b.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/ui_loop.rscrates/perry-ui-macos/Cargo.tomlcrates/perry-ui-macos/src/app.rscrates/perry-ui-macos/src/lib_ffi/core_widgets.rscrates/perry-ui-macos/src/lib_ffi/interactivity.rscrates/perry-ui-macos/src/widgets/mod.rscrates/perry-ui-macos/src/widgets/servo_webview.rscrates/perry-ui-macos/src/widgets/webview.rscrates/perry/src/commands/compile/library_search.rscrates/perry/src/commands/compile/run_pipeline.rscrates/perry/src/commands/compile/types.rscrates/perry/src/commands/dev.rscrates/perry/src/commands/init.rscrates/perry/src/commands/run/entry.rscrates/perry/src/commands/run/mod.rsdocs/servo/README.mddocs/servo/servo-script-ml-kem-0.3.2.patchpackage.jsonpackages/electron/.gitignorepackages/electron/DESIGN.mdpackages/electron/README.mdpackages/electron/bridge/preload-runtime.jspackages/electron/examples/system-explorer/main.tspackages/electron/examples/system-explorer/package.jsonpackages/electron/examples/system-explorer/preload.jspackages/electron/examples/system-explorer/renderer/index.htmlpackages/electron/examples/system-explorer/renderer/renderer.jspackages/electron/package.jsonpackages/electron/src/index.tspackages/electron/src/preload-runtime.tspackages/perry-react/LICENSEpackages/perry-react/README.mdpackages/perry-react/index.d.tspackages/perry-react/package.jsonpackages/perry-react/tsconfig.jsontypes/perry/ui/index.d.ts
Comment on lines
+696
to
+719
| thread_local! { | ||
| /// `on_ready` closure for the deferred top-level UI loop (Electron-compat). | ||
| static PENDING_APP_LOOP_ON_READY: std::cell::Cell<f64> = std::cell::Cell::new(0.0); | ||
| } | ||
|
|
||
| /// Request the Electron-compat UI event loop be entered at the TOP LEVEL after | ||
| /// `main` (not from a microtask). The shim calls this during module init; it | ||
| /// stores `on_ready` and registers `ui_loop_entry` with the runtime so the | ||
| /// generated `main`'s `js_ui_loop_take_over()` hands control to `app_run_loop` | ||
| /// at the top level. Entering `[NSApp run]` from a microtask leaves windows | ||
| /// off-screen, so this top-level entry is required for windows to composite. | ||
| pub fn request_app_loop(on_ready: f64) { | ||
| extern "C" { | ||
| fn perry_runtime_register_ui_loop(f: extern "C" fn()); | ||
| } | ||
| PENDING_APP_LOOP_ON_READY.with(|c| c.set(on_ready)); | ||
| unsafe { | ||
| perry_runtime_register_ui_loop(ui_loop_entry); | ||
| } | ||
| } | ||
|
|
||
| extern "C" fn ui_loop_entry() { | ||
| let on_ready = PENDING_APP_LOOP_ON_READY.with(|c| c.get()); | ||
| app_run_loop(on_ready); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Root the deferred on_ready callback instead of storing it only in TLS.
PENDING_APP_LOOP_ON_READY keeps the JS closure only in a thread_local Cell<f64>. That value is not a stack local or a registered GC root, so a GC between request_app_loop and ui_loop_entry can reclaim the closure before app_run_loop reaches js_closure_call0. There is already such a gap in crates/perry-codegen/src/codegen/entry.rs Lines 745-764: generated main still runs the initial microtask/timer drains and js_run_stdlib_pump before js_ui_loop_take_over(). Please move this callback into a GC-rooted slot / runtime handle instead of TLS. Based on learnings, values are only pinned across allocations while they remain directly reachable from a Rust stack local; once they live only in a heap/TLS slot, they need explicit rooting.
🤖 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 `@crates/perry-ui-macos/src/app.rs` around lines 696 - 719, The deferred
`on_ready` callback is only stored in `PENDING_APP_LOOP_ON_READY`, so it can be
collected before `ui_loop_entry` calls `app_run_loop`. Update `request_app_loop`
and `ui_loop_entry` in `app.rs` to move the callback into a
GC-rooted/runtime-managed handle instead of a `thread_local Cell<f64>`, and
ensure it stays pinned until `js_closure_call0` runs. Use the existing
`perry_runtime_register_ui_loop` / `app_run_loop` flow as the place to wire the
rooted handle through.
Source: Learnings
Comment on lines
+54
to
+57
| /// `true` when the embedder asked for the Servo backend (`PERRY_WEBVIEW=servo`). | ||
| pub fn is_servo_requested() -> bool { | ||
| matches!(std::env::var("PERRY_WEBVIEW").as_deref(), Ok("servo")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Honor compile-time Servo selection, not only PERRY_WEBVIEW.
Line 56 only enables Servo from the runtime environment. The compile path described for --webview servo appears to select the Servo-capable UI library, but this function still returns false unless the user also sets PERRY_WEBVIEW=servo, so the binary can silently fall back to WKWebView. Add a baked-in preference or a UI runtime setter that the compile path/codegen can call. Based on the PR objective that Servo can be selected with --webview servo.
🤖 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 `@crates/perry-ui-macos/src/widgets/servo_webview.rs` around lines 54 - 57, The
`is_servo_requested` check only reads `PERRY_WEBVIEW`, so the compile-time Servo
path selected by `--webview servo` can still fall back to WKWebView unless the
env var is set. Update `is_servo_requested` in `servo_webview.rs` to also honor
a baked-in compile-time preference or a UI runtime setter that the
codegen/selection path can call, and wire that setter into the Servo selection
flow so the UI library choice is preserved without requiring
`PERRY_WEBVIEW=servo`.
Comment on lines
+439
to
+468
| fn blit_rgba_to_image_view(view: &NSImageView, rgba: &[u8], width: u32, height: u32) { | ||
| if width == 0 || height == 0 || rgba.len() < (width * height * 4) as usize { | ||
| return; | ||
| } | ||
| unsafe { | ||
| let rep_cls = AnyClass::get(c"NSBitmapImageRep").expect("NSBitmapImageRep"); | ||
| let alloc: *mut AnyObject = msg_send![rep_cls, alloc]; | ||
| // planes = NULL → the rep allocates its own buffer we then fill. | ||
| let null_planes: *mut *mut u8 = std::ptr::null_mut(); | ||
| let color_space = NSString::from_str("NSDeviceRGBColorSpace"); | ||
| let rep: *mut AnyObject = msg_send![ | ||
| alloc, | ||
| initWithBitmapDataPlanes: null_planes, | ||
| pixelsWide: width as isize, | ||
| pixelsHigh: height as isize, | ||
| bitsPerSample: 8isize, | ||
| samplesPerPixel: 4isize, | ||
| hasAlpha: true, | ||
| isPlanar: false, | ||
| colorSpaceName: &*color_space, | ||
| bytesPerRow: (width * 4) as isize, | ||
| bitsPerPixel: 32isize, | ||
| ]; | ||
| if rep.is_null() { | ||
| return; | ||
| } | ||
| let rep = Retained::from_raw(rep).expect("rep"); | ||
| let dst: *mut u8 = msg_send![&*rep, bitmapData]; | ||
| if !dst.is_null() { | ||
| std::ptr::copy_nonoverlapping(rgba.as_ptr(), dst, (width * height * 4) as usize); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use checked arithmetic for RGBA buffer sizes.
width * height * 4 is evaluated as u32 before casting. Very large create/resize dimensions can wrap before the length guard and copy-size calculations.
Suggested guard
fn blit_rgba_to_image_view(view: &NSImageView, rgba: &[u8], width: u32, height: u32) {
- if width == 0 || height == 0 || rgba.len() < (width * height * 4) as usize {
+ let Some(byte_len) = (width as usize)
+ .checked_mul(height as usize)
+ .and_then(|px| px.checked_mul(4))
+ else {
+ return;
+ };
+ let Some(bytes_per_row) = (width as usize).checked_mul(4) else {
+ return;
+ };
+ if width == 0 || height == 0 || byte_len > isize::MAX as usize || bytes_per_row > isize::MAX as usize || rgba.len() < byte_len {
return;
}
@@
- bytesPerRow: (width * 4) as isize,
+ bytesPerRow: bytes_per_row as isize,
@@
- std::ptr::copy_nonoverlapping(rgba.as_ptr(), dst, (width * height * 4) as usize);
+ std::ptr::copy_nonoverlapping(rgba.as_ptr(), dst, byte_len);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn blit_rgba_to_image_view(view: &NSImageView, rgba: &[u8], width: u32, height: u32) { | |
| if width == 0 || height == 0 || rgba.len() < (width * height * 4) as usize { | |
| return; | |
| } | |
| unsafe { | |
| let rep_cls = AnyClass::get(c"NSBitmapImageRep").expect("NSBitmapImageRep"); | |
| let alloc: *mut AnyObject = msg_send![rep_cls, alloc]; | |
| // planes = NULL → the rep allocates its own buffer we then fill. | |
| let null_planes: *mut *mut u8 = std::ptr::null_mut(); | |
| let color_space = NSString::from_str("NSDeviceRGBColorSpace"); | |
| let rep: *mut AnyObject = msg_send![ | |
| alloc, | |
| initWithBitmapDataPlanes: null_planes, | |
| pixelsWide: width as isize, | |
| pixelsHigh: height as isize, | |
| bitsPerSample: 8isize, | |
| samplesPerPixel: 4isize, | |
| hasAlpha: true, | |
| isPlanar: false, | |
| colorSpaceName: &*color_space, | |
| bytesPerRow: (width * 4) as isize, | |
| bitsPerPixel: 32isize, | |
| ]; | |
| if rep.is_null() { | |
| return; | |
| } | |
| let rep = Retained::from_raw(rep).expect("rep"); | |
| let dst: *mut u8 = msg_send![&*rep, bitmapData]; | |
| if !dst.is_null() { | |
| std::ptr::copy_nonoverlapping(rgba.as_ptr(), dst, (width * height * 4) as usize); | |
| fn blit_rgba_to_image_view(view: &NSImageView, rgba: &[u8], width: u32, height: u32) { | |
| let Some(byte_len) = (width as usize) | |
| .checked_mul(height as usize) | |
| .and_then(|px| px.checked_mul(4)) | |
| else { | |
| return; | |
| }; | |
| let Some(bytes_per_row) = (width as usize).checked_mul(4) else { | |
| return; | |
| }; | |
| if width == 0 || height == 0 || byte_len > isize::MAX as usize || bytes_per_row > isize::MAX as usize || rgba.len() < byte_len { | |
| return; | |
| } | |
| unsafe { | |
| let rep_cls = AnyClass::get(c"NSBitmapImageRep").expect("NSBitmapImageRep"); | |
| let alloc: *mut AnyObject = msg_send![rep_cls, alloc]; | |
| // planes = NULL → the rep allocates its own buffer we then fill. | |
| let null_planes: *mut *mut u8 = std::ptr::null_mut(); | |
| let color_space = NSString::from_str("NSDeviceRGBColorSpace"); | |
| let rep: *mut AnyObject = msg_send![ | |
| alloc, | |
| initWithBitmapDataPlanes: null_planes, | |
| pixelsWide: width as isize, | |
| pixelsHigh: height as isize, | |
| bitsPerSample: 8isize, | |
| samplesPerPixel: 4isize, | |
| hasAlpha: true, | |
| isPlanar: false, | |
| colorSpaceName: &*color_space, | |
| bytesPerRow: bytes_per_row as isize, | |
| bitsPerPixel: 32isize, | |
| ]; | |
| if rep.is_null() { | |
| return; | |
| } | |
| let rep = Retained::from_raw(rep).expect("rep"); | |
| let dst: *mut u8 = msg_send![&*rep, bitmapData]; | |
| if !dst.is_null() { | |
| std::ptr::copy_nonoverlapping(rgba.as_ptr(), dst, byte_len); |
🤖 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 `@crates/perry-ui-macos/src/widgets/servo_webview.rs` around lines 439 - 468,
The RGBA size checks in blit_rgba_to_image_view can overflow because width *
height * 4 is computed before casting, so large dimensions may wrap and bypass
the guard. Update the function to use checked arithmetic or usize-based size
computation for the buffer length, bytes-per-row, and copy length, and then use
the resulting safe size in both the early return condition and the
std::ptr::copy_nonoverlapping call.
Comment on lines
+560
to
+568
| // Experimental Servo backend (PERRY_WEBVIEW=servo). On engine-construction | ||
| // failure we fall through to the system WKWebView below. | ||
| #[cfg(feature = "servo-webview")] | ||
| if super::servo_webview::is_servo_requested() { | ||
| let handle = super::servo_webview::create(&url, width, height); | ||
| if handle != 0 { | ||
| return handle; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Route Electron preload and IPC APIs for Servo-backed handles.
Line 563 can return a Servo handle, but Lines 948-955 and 967-991 only update WK-backed state via HANDLE_TO_KEY / webview_for_handle. With PERRY_WEBVIEW=servo, Electron windows silently skip set_on_message and add_user_script, so the preload bridge and renderer→main IPC never come up. Either implement Servo equivalents for these APIs or keep the Servo path disabled for Electron windows until those contracts exist. Based on the PR objective that the Servo switch applies to Electron apps too.
Also applies to: 946-993
🤖 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 `@crates/perry-ui-macos/src/widgets/webview.rs` around lines 560 - 568, The
Servo-backed path in webview creation can return a handle, but the
Electron-facing API wiring still only looks up WKWebView state through
HANDLE_TO_KEY and webview_for_handle. Update the handle routing in the relevant
webview methods (set_on_message, add_user_script, and related IPC/preload setup)
so Servo handles either get equivalent message/script support or are explicitly
rejected/disabled for Electron windows until those contracts exist. Ensure the
Servo branch in create and the handle-to-webview lookup logic use the same
backend-specific path.
Comment on lines
+620
to
+636
| // 2c. Allow a file:// document to load file:// subresources (its own | ||
| // <script src>, <link href>, images). WKWebView blocks this by | ||
| // default (separate from loadFileURL's navigation read-access), | ||
| // which would break `BrowserWindow.loadFile(...)` apps that split | ||
| // their renderer across files. These flags are KVC-settable on | ||
| // WKPreferences / the configuration (stable since WebKit's early | ||
| // WKWebView and what Electron's `webSecurity:false`-style file | ||
| // loading relies on under the hood). | ||
| let nsnumber_cls = AnyClass::get(c"NSNumber").unwrap(); | ||
| let yes_num: *mut AnyObject = msg_send![nsnumber_cls, numberWithBool: true]; | ||
| let prefs: *mut AnyObject = msg_send![cfg, preferences]; | ||
| if !prefs.is_null() { | ||
| let k = NSString::from_str("allowFileAccessFromFileURLs"); | ||
| let _: () = msg_send![prefs, setValue: yes_num, forKey: &*k]; | ||
| } | ||
| let k2 = NSString::from_str("allowUniversalAccessFromFileURLs"); | ||
| let _: () = msg_send![cfg, setValue: yes_num, forKey: &*k2]; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid enabling universal file access globally.
loadFileURL:allowingReadAccessToURL: already grants sibling asset access for loadFile(...). Setting allowUniversalAccessFromFileURLs for every WebView weakens the file-origin boundary beyond that need.
Suggested narrowing
if !prefs.is_null() {
let k = NSString::from_str("allowFileAccessFromFileURLs");
let _: () = msg_send![prefs, setValue: yes_num, forKey: &*k];
}
- let k2 = NSString::from_str("allowUniversalAccessFromFileURLs");
- let _: () = msg_send![cfg, setValue: yes_num, forKey: &*k2];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 2c. Allow a file:// document to load file:// subresources (its own | |
| // <script src>, <link href>, images). WKWebView blocks this by | |
| // default (separate from loadFileURL's navigation read-access), | |
| // which would break `BrowserWindow.loadFile(...)` apps that split | |
| // their renderer across files. These flags are KVC-settable on | |
| // WKPreferences / the configuration (stable since WebKit's early | |
| // WKWebView and what Electron's `webSecurity:false`-style file | |
| // loading relies on under the hood). | |
| let nsnumber_cls = AnyClass::get(c"NSNumber").unwrap(); | |
| let yes_num: *mut AnyObject = msg_send![nsnumber_cls, numberWithBool: true]; | |
| let prefs: *mut AnyObject = msg_send![cfg, preferences]; | |
| if !prefs.is_null() { | |
| let k = NSString::from_str("allowFileAccessFromFileURLs"); | |
| let _: () = msg_send![prefs, setValue: yes_num, forKey: &*k]; | |
| } | |
| let k2 = NSString::from_str("allowUniversalAccessFromFileURLs"); | |
| let _: () = msg_send![cfg, setValue: yes_num, forKey: &*k2]; | |
| // 2c. Allow a file:// document to load file:// subresources (its own | |
| // <script src>, <link href>, images). WKWebView blocks this by | |
| // default (separate from loadFileURL's navigation read-access), | |
| // which would break `BrowserWindow.loadFile(...)` apps that split | |
| // their renderer across files. These flags are KVC-settable on | |
| // WKPreferences / the configuration (stable since WebKit's early | |
| // WKWebView and what Electron's `webSecurity:false`-style file | |
| // loading relies on under the hood). | |
| let nsnumber_cls = AnyClass::get(c"NSNumber").unwrap(); | |
| let yes_num: *mut AnyObject = msg_send![nsnumber_cls, numberWithBool: true]; | |
| let prefs: *mut AnyObject = msg_send![cfg, preferences]; | |
| if !prefs.is_null() { | |
| let k = NSString::from_str("allowFileAccessFromFileURLs"); | |
| let _: () = msg_send![prefs, setValue: yes_num, forKey: &*k]; | |
| } |
🤖 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 `@crates/perry-ui-macos/src/widgets/webview.rs` around lines 620 - 636, Remove
the global `allowUniversalAccessFromFileURLs` toggle in `webview.rs` and scope
file-origin relaxation to the existing `loadFileURL:allowingReadAccessToURL:`
path instead. Keep `allowFileAccessFromFileURLs` only if needed for local
subresources, and update the WebView configuration setup around `cfg`, `prefs`,
and the `NSNumber` KVC writes so universal file access is not enabled for every
`WKWebView`.
Comment on lines
+2
to
+3
| "name": "electron", | ||
| "version": "0.1.0", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
npm view electron name versionRepository: PerryTS/perry
Length of output: 337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'packages/electron/package.json' 'packages/electron/**' 'package.json' 'pnpm-workspace.yaml' 'npm-workspace.yaml' 'yarn.lock' 'pnpm-lock.yaml' 'package-lock.json' | sed 's#^`#FILE` #'
echo '--- packages/electron/package.json ---'
cat -n packages/electron/package.json
echo '--- root package.json (if present) ---'
if [ -f package.json ]; then cat -n package.json; fi
echo '--- workspace config (if present) ---'
for f in pnpm-workspace.yaml npm-workspace.yaml; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
doneRepository: PerryTS/perry
Length of output: 2471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
p = pathlib.Path('packages/electron/package.json')
data = json.loads(p.read_text())
print('name=', data.get('name'))
print('private=', data.get('private'))
print('publishConfig=', data.get('publishConfig'))
print('files=', data.get('files'))
PYRepository: PerryTS/perry
Length of output: 267
Use a non-conflicting npm name for this package
A public publish under name: "electron" will collide with the existing npm package. Mark it private if it’s workspace-only, or rename/scoped-publish it and preserve the require("electron") alias separately.
🤖 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 `@packages/electron/package.json` around lines 2 - 3, The package name is
conflicting with the existing public npm package, so update the package metadata
for the electron package to avoid publishing under the plain electron name. In
package.json for the electron package, either mark it private if it is
workspace-only or rename it to a scoped/non-conflicting name, and keep any
require("electron") compatibility handled separately from the published package
name.
Comment on lines
+63
to
+76
| function resolveJs(id: number, ok: boolean, value: unknown): string { | ||
| let arg3: string; | ||
| if (value === undefined) { | ||
| arg3 = "undefined"; | ||
| } else { | ||
| arg3 = jsStringLiteral(JSON.stringify(value)); | ||
| } | ||
| return "window.__perryResolve(" + id + "," + (ok ? "true" : "false") + "," + arg3 + ")"; | ||
| } | ||
|
|
||
| function deliverJs(channel: string, args: unknown[]): string { | ||
| const argsJson = JSON.stringify(args); | ||
| return "window.__perryDeliver(" + jsStringLiteral(channel) + "," + jsStringLiteral(argsJson) + ")"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail IPC calls explicitly when the payload is not JSON-serializable.
resolveJs() / deliverJs() assume JSON.stringify always succeeds. If a handler returns a cycle or BigInt, the throw on Lines 142-145 happens inside the Promise fulfillment path, so no reply is sent and the renderer's ipcRenderer.invoke() stays pending forever. webContents.send() has the same issue on Lines 190-191. Route serialization failures through the existing error reply path instead of letting the channel hang.
Suggested fix
const result = handler(invokeEvent, ...msg.args);
Promise.resolve(result).then(
- (value) => wc._eval(resolveJs(msg.id, true, value)),
+ (value) => {
+ try {
+ wc._eval(resolveJs(msg.id, true, value));
+ } catch (err) {
+ wc._eval(resolveJs(msg.id, false, { message: "IPC payload must be JSON-serializable: " + errMessage(err) }));
+ }
+ },
(err) => wc._eval(resolveJs(msg.id, false, { message: errMessage(err) }))
);Also applies to: 140-145, 190-191
🤖 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 `@packages/electron/src/index.ts` around lines 63 - 76, Serialization in
resolveJs(), deliverJs(), and the webContents.send() path can throw for
non-JSON-serializable values like cycles or BigInt, which leaves
ipcRenderer.invoke() and send-based flows hanging. Update the IPC handling in
packages/electron/src/index.ts so JSON.stringify failures are caught before
building the script payload, and route those failures through the existing error
reply path used for rejected IPC calls instead of allowing the Promise
fulfillment or send path to throw. Use the resolveJs/deliverJs helpers and the
invoke/send handler code that calls them to locate and wrap the serialization
step.
Comment on lines
+304
to
+344
| // Fire webContents 'did-finish-load' / 'dom-ready' when the page loads, so | ||
| // apps that push to the renderer after load (a very common pattern) work. | ||
| const self = this; | ||
| // NOTE: wires webContents 'did-finish-load'/'dom-ready' from the webview's | ||
| // onLoaded delegate. The native onLoaded currently doesn't fire for file:// | ||
| // loads (tracked gap) so apps that push to the renderer on load don't yet | ||
| // get the event; renderer→main and main→renderer IPC otherwise work. | ||
| webviewSetOnLoaded(wv, (_url: string) => { | ||
| self.webContents.emit("did-finish-load"); | ||
| self.webContents.emit("dom-ready"); | ||
| }); | ||
|
|
||
| // Route inbound IPC from this window's renderer to ipcMain. | ||
| webviewSetOnMessage(wv, (json: string) => { | ||
| let msg: any; | ||
| try { | ||
| msg = JSON.parse(json); | ||
| } catch (e) { | ||
| return; | ||
| } | ||
| ipcMain._dispatch(self, msg); | ||
| }); | ||
|
|
||
| win.setBody(wv); | ||
| if (opts.show !== false) { | ||
| win.show(); | ||
| this._visible = true; | ||
| } | ||
| openWindows.push(this); | ||
| } | ||
|
|
||
| loadURL(url: string): Promise<void> { | ||
| webviewLoadUrl(this.webContents._wv, url); | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| loadFile(filePath: string, _options?: any): Promise<void> { | ||
| // Resolve relative to cwd; Electron resolves relative to the app dir. | ||
| const abs = path.isAbsolute(filePath) ? filePath : path.join(process.cwd(), filePath); | ||
| webviewLoadUrl(this.webContents._wv, "file://" + abs); | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don't resolve loadURL() / loadFile() before navigation finishes.
Lines 335-344 resolve immediately even though the load is still in flight. In Electron, callers commonly await these methods before touching the page; here that code can run before the document exists. The comment on Lines 307-310 also shows file:// loads still miss the native onLoaded callback, so loadFile() cannot match Electron's contract until that gap is fixed too.
🤖 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 `@packages/electron/src/index.ts` around lines 304 - 344, The load methods in
the BrowserWindow implementation resolve too early, so callers awaiting loadURL
and loadFile can proceed before navigation is complete. Update the BrowserWindow
methods loadURL and loadFile to return a promise that settles only after the
page finishes loading, and wire that resolution through the existing
webContents/webview load lifecycle instead of resolving immediately. Also
address the file:// onLoaded gap noted in the create-window setup so loadFile
can honor the same completion contract as loadURL.
Comment on lines
+5
to
+10
| "types": "./index.d.ts", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./index.d.ts" | ||
| }, | ||
| "./package.json": "./package.json" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ship a runtime entry for import '@perryts/react'.
packages/perry-react/package.json only exposes types for ".", but the docs and index.d.ts tell consumers to use a bare side-effect import. That import has no runtime target, so emitted JS can fail to resolve it. Either publish an empty runtime stub or make the type-reference path the only supported activation path.
🤖 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 `@packages/perry-react/package.json` around lines 5 - 10, The package export
for "." in packages/perry-react/package.json only provides types, so a bare
import like import '`@perryts/react`' has no runtime target. Update the package
entrypoint so the existing activation path resolves at runtime by adding a real
runtime export/stub for the main symbol, or remove the bare-import guidance and
make the type-only reference path the supported mechanism. Check the
package.json exports map and the related index.d.ts/package entrypoint wiring to
keep runtime and type resolution aligned.
Comment on lines
+48
to
+50
| "dependencies": { | ||
| "@types/react": "^18.3.0 || ^19.0.0", | ||
| "@types/react-dom": "^18.3.0 || ^19.0.0" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== package manifest ==\n'
cat -n packages/perry-react/package.json | sed -n '1,220p'
printf '\n== workspace/package manager hints ==\n'
git ls-files | rg '(^|/)(pnpm-lock\.yaml|package-lock\.json|yarn\.lock|npm-shrinkwrap\.json|pnpm-workspace\.yaml|package\.json)$' -nRepository: PerryTS/perry
Length of output: 5244
🏁 Script executed:
set -euo pipefail
printf '\n== references in perry-react ==\n'
rg -n --no-heading '`@types/react`|`@types/react-dom`|react-dom/client|createRoot' packages/perry-react
printf '\n== package metadata for root + perry-react ==\n'
cat -n package.json | sed -n '1,240p'Repository: PerryTS/perry
Length of output: 3616
Split the React type majors
@types/react and @types/react-dom should not share a single ^18.3.0 || ^19.0.0 dependency range. That resolves to one installed major, so React 18 consumers can end up with React 19 typings. Pin one supported major here, or move these to peerDependencies so the app owns the matching type major.
🤖 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 `@packages/perry-react/package.json` around lines 48 - 50, The React type
dependencies in the package manifest are using a shared union range, which can
resolve to the wrong major for consumers. Update the `@types/react` and
`@types/react-dom` entries in the package.json dependencies to target a single
supported major, or move them to peerDependencies so the consuming app controls
the matching typings. Use the existing dependency keys to locate the entries and
split the majors cleanly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One branch combining the Electron-compat shim (run existing Electron apps natively on perry/ui) and an experimental Servo web-engine switch, so Tom can run his Electron app and compare the system WebView (WKWebView) against in-process Servo.
Stacks on current
main(which already has the libsqlite3-sys alignment, #5802).Electron-compat shim
Run an existing Electron app natively (Tauri model — perry/ui WebView instead of bundled Chromium):
app,BrowserWindow,ipcMain/ipcRenderer,webContents, a body-less run loop (appRunLoop/appRequestLoop/appQuit), and the renderer↔main IPC bridge (webviewAddUserScriptpreload injection +webviewSetOnMessage).clipboard,Tray,nativeImage,Menu/Menu.setApplicationMenu,dialog, andappactivate.Servo web-engine switch
In-process Servo as an alternative to WKWebView, off by default:
perry compile app.ts --webview servo(links the Servo-enabled UI lib variant) orPERRY_WEBVIEW=servoat runtime. Ignored on non-macOS.ServoEngine: renders Servo offscreen (SoftwareRenderingContext) and blits each frame into a layer-backed view at ~60 Hz;loadUrl/reload/evaluateJavaScript/onLoaded; mouse (down/up/drag, L/R), scroll wheel, and resize forwarded to Servo.BrowserWindowcreates its webview viaWebView(...)fromperry/ui→webview::create→ the servo toggle. Compile the Electron app with--webview servoand its windows render in Servo.Unblocking Servo (two dependency conflicts)
Embedding Servo's ~835-crate tree in-process collided with perry's deps twice:
ml-kem 0.3.2+pkcs8(stablekem); servo-script needsml-kem 0.2.x(a pre-releasekem, nopkcs8). Irreconcilable, soservo-scriptis forked and its WebCrypto ML-KEM code migrated 0.2→0.3.2. The fork lives at PerryTS/servo-script-mlkem (only change vs upstream is that migration; diff also indocs/servo/servo-script-ml-kem-0.3.2.patch) and is pulled via a git[patch.crates-io]by rev.ml-dsais not a conflict (perry doesn't use it).Verification
--webview+ electron compile path) ✅, and--features servo-webview(full Servo stack from the git fork) ✅. fmt clean.Known limitations / follow-ups (Servo is experimental)
messageHandlers, which Servo 0.1.x lacks — so on the Servo backend pages render but the Electron IPC/preload bridge is inert (a documented servo-upstream gap). The shim is fully functional on the default WKWebView backend.NSTrackingArea), and HiDPI backing scale are follow-ups.See
docs/servo/README.mdfor the full design + fork details.Summary by CodeRabbit
New Features
createRoot(null, ...).Bug Fixes
Documentation