fix(desktop): deny new window creation on shift-click - #384
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a window open handler to the Electron main window that denies any renderer-initiated attempts to open new windows or tabs, preventing uncontrolled window creation from the renderer process. ChangesWindow Security Handler
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 8 minutes and 29 seconds. Comment |
8bf85c1 to
d9f0825
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the version across multiple packages to 2026.5.2.2 and introduces a window opening handler in the Electron main process that denies all requests. Feedback points out that the versioning scheme is not compliant with Semantic Versioning (SemVer), which may interfere with update tools like electron-updater. Furthermore, the global denial of window opening is flagged as potentially disruptive to external links and authentication flows, with a recommendation to implement URL filtering instead.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/desktop-electron/src/main/windows.ts (91)
Denying all window open requests via setWindowOpenHandler is a broad stroke that may break legitimate functionality. Specifically:
- External Links: Links to external websites (e.g., documentation, help pages) that use
target="_blank"or are shift-clicked will now do nothing instead of opening in the system browser. - OAuth/Authentication: If the application or any of its dependencies rely on
window.open()for authentication flows (like GitLab or Poe auth), they will be blocked.
Consider filtering the URL and only denying internal navigation while allowing external URLs to open in the system browser using shell.openExternal(url) (which would require adding shell to the electron imports).
win.webContents.setWindowOpenHandler(({ url }) => {
// TODO: Handle external URLs (e.g., via shell.openExternal)
return { action: "deny" }
})
packages/desktop-electron/package.json (4)
The version 2026.5.2.2 is not a valid Semantic Version (SemVer). Since this package depends on electron-updater, using a non-standard version string may break update checks. Please ensure this format is intended for this specific release line, as workspace packages may follow different release lines.
References
- When performing a version bump, only update packages that belong to the specific release line being updated. Do not assume all workspace packages follow the same release line.
Summary
webContents.setWindowOpenHandler({ action: 'deny' })increateMainWindow()to prevent Electron from spawning new windows on shift+click orwindow.open()calls.preventDefault()when shiftKey is pressed, letting the browser/Electron handle the click — which defaults to opening a new window.<a>tag shift+clicks, not just sidebar conversation items.Summary by CodeRabbit