fix: externalize dependency subpath imports in rollup bundles - #3411
Merged
Merged
Conversation
React 19 changed react-dom/client to require its cjs implementation directly instead of shimming through react-dom. The exact-name external lists no longer matched the subpath, so the production ReactDOM reconciler was bundled at build-time NODE_ENV while react itself kept resolving from the asar at runtime as a development build. Mixed dev/prod React internals crashed the root window before first paint (dispatcher.getOwner is not a function), leaving the app gray. - rollup.config.mjs: makeExternal() matches dependency subpaths (react-dom/client, react/jsx-runtime, semver/functions/gte, electron-log/renderer) as external, preserving per-bundle intentionally-bundled modules - scripts/check-bundle-externals.mjs: post-build guard that walks each entry bundle's chunk graph and fails if React internals are bundled or the external require pattern disappears - package.json: yarn build now runs the guard after rollup
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
WalkthroughRollup external matching is centralized across bundle targets, including exact and subpath matching. A new script scans reachable chunks for React internals and verifies external React requires. The build command runs this validation after Rollup. ChangesBundle externalization validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/check-bundle-externals.mjs (1)
136-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd missing
require('react-dom/client')assertion forscreen-picker-window.js.
src/screenSharing/screen-picker-window.tsximportscreateRootfromreact-dom/client, so its bundle should containrequire('react-dom/client')when externalized. TherootWindow.jsentry correctly asserts bothrequire('react')andrequire('react-dom/client'), butscreen-picker-window.jsonly assertsrequire('react'). Without this assertion, the staleness detector won't flag a pattern change for the screen picker'sreact-dom/clientexternal require.♻️ Proposed fix
assertContains('rootWindow.js', "require('react')"); assertContains('rootWindow.js', "require('react-dom/client')"); assertContains('screen-picker-window.js', "require('react)"); +assertContains('screen-picker-window.js', "require('react-dom/client')"); assertContains('log-viewer-window.js', "require('react')");🤖 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 `@scripts/check-bundle-externals.mjs` around lines 136 - 139, Add an assertContains check for "require('react-dom/client')" alongside the existing screen-picker-window.js assertion in the bundle external validation, preserving the existing React assertion and rootWindow.js checks.
🤖 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.
Nitpick comments:
In `@scripts/check-bundle-externals.mjs`:
- Around line 136-139: Add an assertContains check for
"require('react-dom/client')" alongside the existing screen-picker-window.js
assertion in the bundle external validation, preserving the existing React
assertion and rootWindow.js checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10f07995-3937-4f73-b869-016c237bbb13
📒 Files selected for processing (3)
package.jsonrollup.config.mjsscripts/check-bundle-externals.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: build (macos-latest, mac)
- GitHub Check: build (ubuntu-latest, linux)
🔇 Additional comments (3)
rollup.config.mjs (1)
201-215: LGTM!Also applies to: 219-219, 249-249, 279-279, 309-309, 340-346, 377-377, 436-436
scripts/check-bundle-externals.mjs (1)
1-135: LGTM!Also applies to: 140-143
package.json (1)
33-33: LGTM!
macOS installer download |
This was referenced Jul 14, 2026
jeanfbrito
added a commit
that referenced
this pull request
Jul 15, 2026
…icker, sidebar tooltip) (#3417) * fix: #3244 sidebar tooltip null in shortcut formatting (#3326) - index.tsx: order <= 9 -> order < 9 (off-by-one, server #10 showed shortcut) - ServerButton.tsx: conditional shortcut string in tooltip (null rendered as '(^+null)') (cherry picked from commit e405a6a) * fix: keep screen picker sources stable under Electron 42 macOS capture stack (#3414) Electron 42's macOS ScreenCaptureKit backend bounds getSources() at ~3s (upstream hang/crash fixes), returning an empty array or empty thumbnails when enumerations run back-to-back. The picker's 3s polling plus the post-selection re-enumeration turned those empty results into a blank "No windows found" list and denied valid share attempts. - Split the desktop capturer cache into per-type buckets: screen-only enumeration is fast (~700ms) and reliable; window enumeration is paced with a 4s post-completion cooldown (cold-start chain bypasses it once) - Never overwrite a non-empty bucket with an empty enumeration result; keep the bucket stale so it retries on the next opportunity - Merge thumbnails by source id so a source arriving with an empty thumbnail keeps its last good preview instead of being dropped - Validate the selected source against the cache (the same list the picker rendered from) instead of re-enumerating on Share; fall back to one direct enumeration only when the cache is empty - Keep the previous source list when a renderer fetch fails Measured on macOS with Electron 42.5.0: tight-loop getSources returned 0 sources in ~75% of calls; screen-only calls never failed; alternating per-type calls with 4s gaps returned complete results every round. Documented in docs/KNOWN_ISSUES.md with upstream refs (electron/electron#51128, electron/electron#50960) and the macOS 15+ useSystemPicker follow-up. (cherry picked from commit dddd2cc) * fix: externalize dependency subpath imports in rollup bundles (#3411) rollup externals matched module ids by exact name only, so subpath entrypoints like `react-dom/client` (used by createRoot in rootWindow.ts, log-viewer-window.tsx, and screenSharePickerMount.tsx) did not match the `react-dom` external and got bundled as build-time-NODE_ENV ReactDOM, while `react` stayed external and resolved to a different (dev) copy from the asar at runtime — mixing incompatible React internals and crashing the renderer (SUP-1072, ReferenceError: exports is not defined). makeExternal() now matches `id === moduleName || id.startsWith(moduleName + '/')` so subpath imports are externalized alongside their base package. scripts/check-bundle-externals.mjs guards the build against regressing back to bundled React internals, wired into `yarn build`. (backported to 4.15.x) Adapted from master commit 0dafe39: 4.15.4 has no `screen-picker-window.js` rollup entry (the screen picker mounts inside the video-call-window bundle via screenSharePickerMount.tsx, not a separate window/entry point in this release), so that entry and its assertions were omitted from check-bundle-externals.mjs. All other bundle configs and the check script are ported as-is. * fix: correct auto-merge artifact in ScreenSharingRequestTracker backport The cherry-pick of dddd2cc (#3414) auto-merged with references to master's entry/finishActive queue API, which doesn't exist in 4.15.4's flat cb/markComplete structure — this failed typecheck. Replaced with 4.15.4's existing cb(...) calling convention, matching the fallback path directly below it in the same function. Also fixed the corresponding cherry-picked spec assertion: it expected cb to be called with `null` (master's deny convention), but 4.15.4's DisplayMediaCallback deny convention is `{ video: false }` throughout this file — updated the assertion to match. * fix: remove leftover merge-conflict marker fragment from KNOWN_ISSUES.md The docs/KNOWN_ISSUES.md merge during the backport left a trailing `>>>>>>> dddd2cc (...)` conflict-marker remnant appended to the last line of the file. Removed; no other content was affected.
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.
What
4.16.0-alpha.0 on macOS rendered a gray window on launch. The root window crashed before first paint with:
Root cause
The rollup
externallists matched dependency names exactly. React 18'sreact-dom/client.jswas a thin shim that requiredreact-dom(external, resolved from the asar at runtime), so everything stayed consistent. React 19 (#3384) changedreact-dom/client.jsto require./cjs/react-dom-client.production.jsdirectly — the subpathreact-dom/clientno longer matched the exact-name external list, so the whole ReactDOM reconciler (andreact/jsx-runtime) was bundled with the CI build'sNODE_ENV=production. Meanwhilerequire('react')stayed external and resolved from the asar at runtime, whereNODE_ENVis undefined, loading the development build.Development React 19's
createElementcallsdispatcher.getOwner()on the shared internals dispatcher; the production ReactDOM dispatcher doesn't provide it. Mixed dev/prod React internals crashed every render. The mismatch was production-only: dev builds bundle the development ReactDOM, which matches the development React, soyarn startnever reproduced it.Changes
makeExternal()helper matches dependency subpaths (id === name || id.startsWith(name + '/')) as external. Nowreact-dom/client,react/jsx-runtime,semver/functions/gte, andelectron-log/rendererresolve from the asar at runtime like their base packages. Per-bundle intentionally-bundled modules (@bugsnag/js,marked,marked-highlight,highlight.js,dompurify) are preserved.app/), fails the build if any reachable chunk contains bundled React internals markers (React 19__CLIENT_INTERNALS…/ React 18__SECRET_INTERNALS…), and assertsrequire('react')/require('react-dom/client')remain external requires in the React window bundles.injected.jsis exempt: it is a self-contained IIFE with no externals, so dev/prod mixing cannot occur there.yarn buildruns the guard after rollup, so every platform build and CI release build is gated.yarn start/ watch mode unchanged.Verification
NODE_ENV=production yarn build: rootWindow bundle contains no ReactDOM internals;react-dom/clientandreact/jsx-runtimeappear as external requires. Guard passes (29 chunks scanned).env -u NODE_ENV npx electron ., same module resolution as the packaged app): boots and renders with nogetOwnererror and no uncaught React errors.require('react-dom/client')literal from all four reachable chunks that contain it fails the positive assertion; restoring returns it to pass.Summary by CodeRabbit
Tests
Chores