feat(web-desktop): resolve web clients to a live window for window-scoped IPC - #1313
feat(web-desktop): resolve web clients to a live window for window-scoped IPC#1313user79697 wants to merge 2 commits into
Conversation
The remote page (web-desktop mirror) loaded its splash screen and hung
there forever, on localhost, LAN IP and cloudflared tunnel alike.
`FAKE_EVENT` — the synthetic IpcMainInvokeEvent that web clients invoke
ipcMain handlers through — defined senderFrame, frameId, processId and
type, but no `sender`. Handlers that resolve their calling window via
`BrowserWindow.fromWebContents(event.sender)` therefore passed
`undefined`, and Electron threw
Cannot read properties of undefined (reading 'getOwnerBrowserWindow')
`windows:getState` is one of those handlers and web-desktop calls it
during boot, treating the rejection as fatal.
The error is thrown in the main process, serialized over the WebSocket
and rethrown in the browser, so the stack trace points at preload-*.js
and the identifier appears nowhere in the bundle — which is why this
looked like a renderer or network problem.
Resolve `sender` to the oldest live BrowserWindow's webContents. A web
client mirrors the desktop, so that is the window it should act as. It
is a getter because the module is evaluated before any window exists and
the answer changes over the app's lifetime; it still yields `undefined`
when no window is open, leaving each handler's existing "unknown caller"
path intact.
Covered by three regression tests: sender resolves to a live window,
destroyed windows are skipped, and no-windows still yields undefined.
Verified by mutation — reverting the getter fails two of them.
📝 WalkthroughWalkthroughBridge invoke dispatch now provides a synthetic event sender from the oldest live Electron window. Tests mock window enumeration and verify sender behavior for live, destroyed, and absent windows. ChangesBridge invoke sender resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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 |
Greptile SummaryAdds lazy resolution of a live Electron window for synthetic web-desktop IPC events.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "docs(web-desktop): correct the event.sen..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2593c7e33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * as fatal — the remote page hung on the splash screen forever. | ||
| * | ||
| * A web client mirrors the desktop UI, so it resolves to the oldest live | ||
| * window — `getAllWindows()` is creation-ordered, making that the main |
There was a problem hiding this comment.
The added production comment contains U+2014 on lines 51 and 54, and the added regression-test comment repeats it. Repository guidance explicitly bans this character everywhere, including code comments, so replace all three occurrences with ASCII punctuation before merging.
AGENTS.md reference: AGENTS.md:L163-L169
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/__tests__/main/web-server/handlers/bridgeHandlers.test.ts`:
- Around line 170-174: Update the regression comment around FAKE_EVENT to
replace both em dashes with standard punctuation, preserving the existing
meaning and wording.
In `@src/main/web-server/handlers/bridgeHandlers.ts`:
- Around line 53-56: Replace the getter’s getAllWindows().find() logic with
explicit main-window tracking. Update the relevant window creation and close
handling so the tracked main window is assigned when created and cleared or
reassigned when closed, while preserving the getter’s live resolution behavior.
- Around line 43-62: In the FAKE_EVENT.sender JSDoc comment, replace both em
dashes with permitted punctuation such as periods, commas, or parentheses, while
preserving the explanation and code behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dcdc301d-99ca-45c6-9ab3-22de79894d9b
📒 Files selected for processing (2)
src/__tests__/main/web-server/handlers/bridgeHandlers.test.tssrc/main/web-server/handlers/bridgeHandlers.ts
| * A web client mirrors the desktop UI, so it resolves to the oldest live | ||
| * window — `getAllWindows()` is creation-ordered, making that the main | ||
| * window. Must be a getter, not a value: this module is evaluated before any | ||
| * BrowserWindow exists, and the resolved window changes over the app's life. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Electron's BrowserWindow.getAllWindows() guarantee creation order in its official API documentation?
💡 Result:
No, the official Electron API documentation does not explicitly guarantee any specific order for the array returned by BrowserWindow.getAllWindows [1][2]. While historically the method has returned windows in the order of their creation [3], this behavior is not documented as a guarantee. Electron maintainers have previously noted that relying on the ordering of this array is not recommended, and developers who require specific ordering—such as z-order or focus order—should manage that state manually within their own application logic [3]. Because it is not an official guarantee, developers should not rely on any specific sequence (e.g., creation order) remaining consistent across future Electron versions [3].
Citations:
- 1: https://electronjs.org/docs/latest/api/browser-window
- 2: https://github.com/electron/electron/blob/main/docs/api/browser-window.md
- 3: Add BrowserWindow.getOrderedWindows() to return front-to-back ordered array electron/electron#8256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files and relevant symbols"
git ls-files | rg '(^|/)bridgeHandlers\.ts$|package\.json$|electron' | head -100
echo
echo "bridgeHandlers excerpt"
file="$(git ls-files | rg '(^|/)bridgeHandlers\.ts$' | head -1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,120p' "$file" | cat -n
fi
echo
echo "Electron version from manifests/scripts"
rg -n '"electron"|electron|`@electron`' package.json package-lock.json pnpm-lock.yaml yarn.lock || trueRepository: RunMaestro/Maestro
Length of output: 17918
Don’t rely on getAllWindows() creation order.
BrowserWindow.getAllWindows() does not document a guaranteed creation order, and Electron has advised relying on explicit app state for z-order/focus/creation order instead. Track the main window explicitly and update that tracking on window close/create rather than using .find() on getAllWindows().
🤖 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 `@src/main/web-server/handlers/bridgeHandlers.ts` around lines 53 - 56, Replace
the getter’s getAllWindows().find() logic with explicit main-window tracking.
Update the relevant window creation and close handling so the tracked main
window is assigned when created and cleared or reassigned when closed, while
preserving the getter’s live resolution behavior.
Three review bots flagged em dashes in the comments added by the previous commit; the repo bans them in authored text. Removed from both the production comment and the regression-test comment. CodeRabbit also questioned the claim that `getAllWindows()` is creation-ordered. It is not documented as such by Electron, so the comment no longer asserts it. The getter picks *a* live window rather than claiming to pick the main one. `getMainWindow` is dependency injected everywhere it is used (see web-server-factory.ts) and is not importable from a module-level const, so `getAllWindows()` remains the only option here. With a single window open, the common case, the two are the same window. Corrected the stated motivation as well. The crash the previous message described cannot occur on this base: 4765eda (2026-07-04) already made `resolveCallingWindow` return undefined when `event.sender` is missing. What remains is a behavioral gap, not a crash. Without a sender, every window-scoped handler resolves to "unknown caller", so `windows:getState` returns null and claiming an agent or setting panel state silently no-ops for web clients. The comments now describe that. No functional change. Tests unchanged and still passing (11/11).
user79697
changed the title
|
Pushed b7fbe65 addressing the review, and I have retitled and rewritten Review items
The part worth your attention I opened this as a fix for if (!event?.sender) return undefined;That commit is an ancestor of both That makes this a semantics change rather than a crash fix, and it cuts Since that is a product decision about what a web client is, it is |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Changed files and latest relevant commit =='
git show --stat --oneline --decorate --no-renames HEAD
git log --oneline -5
printf '%s\n' '== Candidate source files =='
fd -a 'bridgeHandlers\.ts|bridgeHandlers\.test\.ts|web-server-factory\.ts' src || true
rg -n -C 4 --glob '*.ts' \
'resolveCallingWindow|windows:getState|FAKE_EVENT|handleBridgeInvoke|BrowserWindow\.fromWebContents|getMainWindow' \
srcLength of output: 211671
The existing guard in Injecting an arbitrary live window’s I recommend closing this PR as superseded rather than merging it as a fix. If remote window-scoped behavior is desired later, it should be a deliberate design with explicit remote-client/window identity rather than a synthetic 🐇🔍 ✏️ Learnings added
|
|
@user79697 Thanks for the contribution, and genuinely thank you for the follow-up comment. Coming back to say "my original framing was wrong, here is what this actually is, and here is why you might want to close it" is a rare and very welcome thing to see on a PR. You also nailed the review items: em dashes are gone from both comments, the To answer your direct question first: yes, the gap you found is real. The blast radius
1. // Open the consent window. Only the trusted main renderer may ask.
ipcMain.handle('plugins:request-consent', async (event, pluginId: unknown) => {
if (event.sender !== mainWindow?.webContents) throw new Error('UntrustedConsentRequester');Today a bridged call has no The mint itself stays safe, for the record. There is a second-order wrinkle here too: the 2. if (event.sender.getType() !== 'window') { ...reject... }Currently that throws a TypeError for bridged calls and the bridge returns it as an error. After this change it passes, so a web client can ( The multi-window mutationsSeparately, within
With one window open, resolving a web client to it is harmless. With two or more, a web client creating an agent claims it into whichever window The read-only half ( What I would take insteadScope the resolution to the handlers that want it, rather than putting it on the shared event. The cleanest version: leave On reaching the real main window: your point about If you would rather split it, a PR that fixes only the read-only handlers is an easy yes from me, and we can decide the Happy to keep this open while you consider which way you want to go - the investigation here is solid and I would like to land the fix, just with a narrower reach. |
|
@user79697 Following up on this after a month. Your call to hand the semantics decision back was the right one, and the decision is: the gap is real and should be fixed, but not by giving I have implemented the narrower shape from my earlier comment in #1420, so this does not sit open indefinitely. What went in
Two details worth flagging since they came out of your investigation:
Closing this oneClosing as superseded by #1420, not as rejected. You found the gap, characterized it correctly, and then came back to say your own original framing was wrong once you found If you would rather have carried it yourself, say so and I will hand it back - the door is open on the |
Web-desktop clients invoke ipcMain through a synthetic event with no sender, so every window-scoped handler resolved to 'unknown caller': windows:getState returned null and the remote page booted without the window state it mirrors. Narrow fix, per the discussion on #1313. resolveCallingWindow takes an allowBridge option that only the two READ handlers (getState, getBounds) pass. A bridge event resolves to the registry's isMain entry, not to BrowserWindow.getAllWindows()[0] - Electron documents no ordering there, and a nondeterministic answer would be read as the caller's identity. The writes deliberately keep degrading. registerSession and setPanelState both document that a window only ever mutates itself; with two windows open a remote agent-create would claim the agent into a window the remote user never chose, and a remote panel collapse would rewrite a desktop window's state. FAKE_EVENT keeps having no sender, which is what keeps the blast radius to this file: plugins:request-consent still rejects web clients with UntrustedConsentRequester, and browser:clearSessionData still rejects them on event.sender.getType(). Giving the shared event a sender would have opened both. Tests pin it from both sides: red against rc without the fix, and red again if allowBridge is extended to the write handlers. Supersedes #1313 by user79697, whose investigation found the gap.
2 participants
What this does
FAKE_EVENT, the syntheticIpcMainInvokeEventthat web-desktop clientsinvoke
ipcMainhandlers through, definessenderFrame,frameId,processIdandtype, but nosender. This adds a lazily-resolvedsenderpointing at a live window'swebContents.Why (revised, please read)
This PR was opened as a crash fix. That framing was wrong for this base
branch, and I would rather correct it than let it merge under a stale
description.
The crash I originally hit is real, but it is already handled on
rcby4765eda (2026-07-04), which added to
resolveCallingWindow:My installed 0.19.0-RC build predates that commit, which is why I saw
Cannot read properties of undefined (reading 'getOwnerBrowserWindow')and the remote page hanging on the splash screen. On current
rcthatcannot happen.
So what is left is not a crash but a behavioral gap. With no
sender,every window-scoped handler resolves to "unknown caller" and silently
degrades for web clients:
windows:getStatereturnsnullsetPanelStateand window naming are no-opsGiving the bridge a
sendermakes those work remotely, on the theorythat a web client is a mirror of the desktop and should act as the window
it mirrors.
That directly contradicts the intent stated in 4765eda ("a web
client is not a window"), so this is your call, not mine. If the
degradation is deliberate, close this and I will not object. I have left
the guard in
resolveCallingWindowuntouched either way, so nothing heredepends on removing it.
Implementation notes
senderis a getter, not a value: the module is evaluated before anyBrowserWindowexists, and the correct answer changes over the app'slifetime. It still yields
undefinedwhen no window is open(headless/tray), so every handler's existing "unknown caller" path stays
intact.
Per review feedback, the comment no longer claims
getAllWindows()iscreation-ordered, since Electron does not document that. It picks a
live window; with a single window open, the common case, that is the main
window. I looked at using
getMainWindowinstead, but it is dependencyinjected at every call site (
web-server-factory.ts) and is notimportable from a module-level const.
Testing
Three regression tests: sender resolves to a live window, destroyed
windows are skipped, no-windows still yields
undefined. Verified bymutation (reverting the getter fails two). Full file: 11/11 passing.
Manually verified against a local web server over loopback with a CDP
probe. Zero failed
windows:getStateresponses out of 11,803 successfulbridge.responseframes, nogetOwnerBrowserWindowin console, and theDOM renders the real UI. The only remaining failures are pre-existing
fs:readDirEPERM against Windows junction points(
My Music/My Pictures/My Videos), unrelated to this change.Review feedback addressed
CodeRabbit, Greptile, and Codex).
getAllWindows()ordering claim dropped (CodeRabbit).Summary by CodeRabbit