feat(electron): wire clipboard, Tray, nativeImage, Menu, dialog, activate#5797
Closed
proggeramlug wants to merge 2 commits into
Closed
feat(electron): wire clipboard, Tray, nativeImage, Menu, dialog, activate#5797proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
…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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaces stub/no-op implementations in the Electron-compatible shell with native perry/ui-backed integrations: full ChangesNative macOS API implementations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@packages/electron/src/index.ts`:
- Around line 236-239: The app-lifetime rootedNativeUi array is currently
retaining one-shot open/save dialog callbacks longer than needed, which leaks
their closure after the native dialog resolves. Update the dialog flow in the
open/save dialog handlers to root the callback only while the dialog is pending
and remove it immediately after the native reply is handled. Use the relevant
dialog request/response paths around the open/save dialog creation and
completion logic so only persistent menubar/tray handles stay rooted, not
per-dialog callbacks.
- Around line 810-821: Add an _destroyed guard in the Tray methods that mutate
native state so they no-op after destroy() has been called. Update setImage,
setToolTip, and setContextMenu (and the other affected tray mutators referenced
in the review) to check the Tray instance’s destroyed state before calling
traySetIcon, traySetTooltip, or trayAttachMenu, using the existing _destroyed
flag or the same guard pattern used elsewhere in this class.
- Around line 423-425: The rootedOnActivate handler in index.ts is using
openWindows.length, which counts hidden windows and can incorrectly report that
a window is visible. Update the activate emission to use visible-window state
instead, checking whether any tracked window is actually visible before passing
the boolean to self.emit("activate", ...). Keep the change localized to
rootedOnActivate and reuse the existing window tracking helpers or visibility
flags if available.
- Around line 819-821: The setContextMenu method on the tray wrapper currently
only updates the native menu when a Menu is provided, so calling
setContextMenu(null) leaves the old menu attached. Update setContextMenu in the
tray implementation to explicitly detach or clear the native context menu when
menu is null, while keeping the existing trayAttachMenu path for non-null menus.
- Around line 642-645: setApplicationMenu currently clears appMenu and returns
early on null, leaving the previously attached native menubar active while
getApplicationMenu() reports null. Update the setApplicationMenu method to
either explicitly detach/replace the native menu bar when menu is null or
preserve the existing appMenu state until native removal is supported, and make
sure the native path handled by menuBarAttach stays consistent with
getApplicationMenu.
- Around line 783-787: The nativeImage factory methods currently hide
unsupported input by always returning an empty NativeImage, which causes Tray
icons and setImage() callers to fail silently. Update createFromDataURL and
createFromBuffer in the nativeImage implementation to explicitly reject
unsupported usage by throwing or adding a clear guard instead of constructing
NativeImage("") so callers get an immediate signal. Use the createFromDataURL
and createFromBuffer symbols to locate the change and keep the behavior
consistent with the rest of the electron API surface.
🪄 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: 3596f4ad-8738-4870-814c-887405e17045
📒 Files selected for processing (1)
packages/electron/src/index.ts
Comment on lines
+236
to
+239
| // Native-held menubar/tray handles + their click closures. The native side | ||
| // stores these; rooting them here keeps the menubar/tray and every menu-item | ||
| // callback alive for the app's lifetime (otherwise GC could reclaim them). | ||
| const rootedNativeUi: any[] = []; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid permanently rooting one-shot dialog callbacks.
rootedNativeUi is app-lifetime, but dialog callbacks only need to survive until native replies. Each open/save dialog currently leaks its callback closure after resolution.
Suggested cleanup pattern
+function unrootNativeUi(value: any): void {
+ const index = rootedNativeUi.indexOf(value);
+ if (index >= 0) rootedNativeUi.splice(index, 1);
+}
+
const cb = (selected: string) => {
+ unrootNativeUi(cb);
if (!selected) resolve({ canceled: true, filePaths: [] });
else resolve({ canceled: false, filePaths: [selected] });
};Also applies to: 674-681, 689-695
🤖 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 236 - 239, The app-lifetime
rootedNativeUi array is currently retaining one-shot open/save dialog callbacks
longer than needed, which leaks their closure after the native dialog resolves.
Update the dialog flow in the open/save dialog handlers to root the callback
only while the dialog is pending and remove it immediately after the native
reply is handled. Use the relevant dialog request/response paths around the
open/save dialog creation and completion logic so only persistent menubar/tray
handles stay rooted, not per-dialog callbacks.
Comment on lines
+423
to
+425
| rootedOnActivate = () => { | ||
| self.emit("activate", {}, openWindows.length > 0); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether BrowserWindow tracks visibility and whether `activate`
# can use a visible-window check instead of only openWindows.length.
rg -n -C3 '\bopenWindows\b|isVisible|show\(|hide\(|minimize\(' packages/electron/src/index.tsRepository: PerryTS/perry
Length of output: 2705
Use visible-window state for activate. openWindows.length counts hidden windows too, so this will report true even when every window is hidden. That can prevent apps from recreating a window on dock activation.
🤖 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 423 - 425, The rootedOnActivate
handler in index.ts is using openWindows.length, which counts hidden windows and
can incorrectly report that a window is visible. Update the activate emission to
use visible-window state instead, checking whether any tracked window is
actually visible before passing the boolean to self.emit("activate", ...). Keep
the change localized to rootedOnActivate and reuse the existing window tracking
helpers or visibility flags if available.
Comment on lines
+642
to
+645
| static setApplicationMenu(menu: Menu | null): void { | ||
| appMenu = menu; | ||
| if (!menu) return; // removing the native menubar isn't supported in v1 | ||
| menuBarAttach(menu._buildNativeMenuBar()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols first.
ast-grep outline packages/electron/src/index.ts --view expanded
# Show the relevant section with line numbers.
sed -n '600,700p' packages/electron/src/index.ts
# Search for application menu read/write paths and native attach/detach helpers.
rg -n "setApplicationMenu|getApplicationMenu|appMenu|menuBarAttach|menuBarDetach|_buildNativeMenuBar" packages/electron/src/index.ts packages/electron/src -g '*.ts'Repository: PerryTS/perry
Length of output: 10645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the native menu-bar helper definitions and any attach/detach behavior.
rg -n "function menuBarAttach|menuBarAttach\(|menuBarCreate|menuBarAddMenu|menuBarDetach|menuBarClear|detach" packages/electron/src -g '*.ts'
# Read the top of the file where the imports and helper wiring are defined.
sed -n '1,120p' packages/electron/src/index.tsRepository: PerryTS/perry
Length of output: 4294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all application-menu references and any tests/docs that describe null handling.
rg -n "setApplicationMenu\(|getApplicationMenu\(|application menu|menubar|native menubar|menu bar" . -g '*.ts' -g '*.md' -g '*.txt' -g '*.test.*' -g '*.spec.*'
# Read the nearby comment around the implementation for intent.
sed -n '488,650p' packages/electron/src/index.tsRepository: PerryTS/perry
Length of output: 14990
Handle setApplicationMenu(null) on the native menubar path. appMenu is cleared, but the method returns before touching the attached native menu bar, so the old menu stays active while getApplicationMenu() reports null. Either detach/replace the native bar here or keep reporting the existing menu until removal is supported.
🤖 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 642 - 645, setApplicationMenu
currently clears appMenu and returns early on null, leaving the previously
attached native menubar active while getApplicationMenu() reports null. Update
the setApplicationMenu method to either explicitly detach/replace the native
menu bar when menu is null or preserve the existing appMenu state until native
removal is supported, and make sure the native path handled by menuBarAttach
stays consistent with getApplicationMenu.
Comment on lines
+783
to
+787
| createFromDataURL(_dataUrl: string): NativeImage { | ||
| return new NativeImage(""); | ||
| }, | ||
| createFromBuffer(_buffer: any, _options?: any): NativeImage { | ||
| return new NativeImage(""); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t silently turn valid data URLs or buffers into empty images. These nativeImage factories are exposed as supported APIs, but they always return new NativeImage(""), so Tray icons and setImage() callers lose the icon with no signal. Throw or explicitly guard/document the unsupported path instead.
🤖 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 783 - 787, The nativeImage
factory methods currently hide unsupported input by always returning an empty
NativeImage, which causes Tray icons and setImage() callers to fail silently.
Update createFromDataURL and createFromBuffer in the nativeImage implementation
to explicitly reject unsupported usage by throwing or adding a clear guard
instead of constructing NativeImage("") so callers get an immediate signal. Use
the createFromDataURL and createFromBuffer symbols to locate the change and keep
the behavior consistent with the rest of the electron API surface.
Comment on lines
+810
to
+821
| setImage(image: string | NativeImage): void { | ||
| traySetIcon(this._handle, iconToPath(image)); | ||
| } | ||
| setToolTip(toolTip: string): void { | ||
| traySetTooltip(this._handle, toolTip); | ||
| } | ||
| setTitle(_title: string, _options?: any): void { | ||
| /* status-item title text — not wired in v1 */ | ||
| } | ||
| setContextMenu(menu: Menu | null): void { | ||
| if (menu) trayAttachMenu(this._handle, buildNativeMenu(menuItemList(menu))); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard Tray native calls after destruction.
After destroy(), setImage, setToolTip, and setContextMenu can still call native functions with the destroyed handle. Add an _destroyed guard before mutating native tray state.
Suggested guard
setImage(image: string | NativeImage): void {
+ if (this._destroyed) return;
traySetIcon(this._handle, iconToPath(image));
}
setToolTip(toolTip: string): void {
+ if (this._destroyed) return;
traySetTooltip(this._handle, toolTip);
}
setContextMenu(menu: Menu | null): void {
+ if (this._destroyed) return;
if (menu) trayAttachMenu(this._handle, buildNativeMenu(menuItemList(menu)));
}Also applies to: 827-831
🤖 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 810 - 821, Add an _destroyed
guard in the Tray methods that mutate native state so they no-op after destroy()
has been called. Update setImage, setToolTip, and setContextMenu (and the other
affected tray mutators referenced in the review) to check the Tray instance’s
destroyed state before calling traySetIcon, traySetTooltip, or trayAttachMenu,
using the existing _destroyed flag or the same guard pattern used elsewhere in
this class.
Comment on lines
+819
to
+821
| setContextMenu(menu: Menu | null): void { | ||
| if (menu) trayAttachMenu(this._handle, buildNativeMenu(menuItemList(menu))); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle setContextMenu(null)
packages/electron/src/index.ts:819-821 skips the native call when menu is null, so the previous tray context menu stays attached. Clear/detach it on null to match the API.
🤖 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 819 - 821, The setContextMenu
method on the tray wrapper currently only updates the native menu when a Menu is
provided, so calling setContextMenu(null) leaves the old menu attached. Update
setContextMenu in the tray implementation to explicitly detach or clear the
native context menu when menu is null, while keeping the existing trayAttachMenu
path for non-null menus.
…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.
Contributor
Author
|
Addressed all six review findings in
Note: the red |
Contributor
Author
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.
Wires electron-compat surfaces that were link errors (
nativeImage,clipboard,Tray) or true no-ops (Menu.setApplicationMenu,dialog.show*Dialog) on the stress test. The native capabilities already exist inperry-ui-macosand are exposed viaperry/ui, so this is TS-only (no compiler change) — every function used is already in theperry/uidispatch table.Wired
clipboard.readText/writeText/clear(link error)clipboardRead/clipboardWritenativeImage.createFromPath/…(link error)Tray(not exported)trayCreate/traySetIcon/traySetTooltip/trayAttachMenu/trayOnClick/trayDestroyMenu.setApplicationMenu(no-op)menuBarCreate/menuBarAddMenu/menuBarAttach+menuAddItem*dialog.showOpenDialog/showSaveDialog(stub)openFileDialog/openFolderDialog/saveFileDialogapp'activate'event (never emitted)onActivate(native dock-click callback)Menu support includes separators, nested submenus, click callbacks, accelerators normalized from Electron syntax (
CmdOrCtrl+S→Cmd+S), androleitems mapped to AppKit first-responder selectors (copy:/paste:/undo:/terminate:/…). Theapp.on('activate', …)event passes(event, hasVisibleWindows)so the standard "reopen window from the dock" pattern works.Native-held handles and the menu/tray/dialog callbacks are kept in a module-level root array so GC can't reclaim them.
Verification
Tray+ context menu + click,dialog.showOpenDialog,app.on('activate')) compiles and links — the previously-undefinednativeImage/clipboard/Traysymbols now resolve.clipboardWrite("perry-clip-test-123")→clipboardRead()→perry-clip-test-123.electronenters the native UI loop, so the harness can't run it headless.Out of scope (Tier B — need a new
perry/uibinding first)Main-process
Notification(nativenotifications.rs::sendexists but isn't exposed to TS yet),nativeTheme.shouldUseDarkColors(nativeperry_system_is_dark_mode()exists), andapp.dock.setIcon. These need a small Rust/binding change + rebuild, so they're a separate follow-up.Summary by CodeRabbit
New Features
Bug Fixes