fix: backport master fixes for 4.15.5 (rollup externals, E42 screen picker, sidebar tooltip) - #3417
Conversation
…e 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)
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.
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.
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.
WalkthroughThe change adds two-bucket desktop source caching with fallback validation, centralizes Rollup externalization and adds a build guard, documents known issues, and adjusts sidebar shortcut rendering. ChangesScreen sharing source caching
Bundle externalization validation
Known-issue documentation
Sidebar shortcut rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScreenSharePicker
participant ScreenSharingRequestTracker
participant desktopCapturerCache
participant ElectronDesktopCapturer
ScreenSharePicker->>ElectronDesktopCapturer: request desktop sources
ElectronDesktopCapturer-->>ScreenSharePicker: return source list
ScreenSharingRequestTracker->>desktopCapturerCache: resolve requested source ID
desktopCapturerCache-->>ScreenSharingRequestTracker: return cached source or empty result
ScreenSharingRequestTracker->>ElectronDesktopCapturer: enumerate sources when cache is empty
ElectronDesktopCapturer-->>ScreenSharingRequestTracker: return validation result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rollup.config.mjs (1)
205-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd fallbacks for potentially undefined manifest keys.
While standard
package.jsonfiles usually contain bothdependenciesanddevDependencies, adding a fallback preventsObject.keys()from throwing aTypeErrorif one of these fields is omitted in the future or in certain build configurations.♻️ Proposed fix
const makeExternal = (bundledModules = []) => { const externalModules = [ ...builtinModules, - ...Object.keys(appManifest.dependencies), - ...Object.keys(appManifest.devDependencies), + ...Object.keys(appManifest.dependencies || {}), + ...Object.keys(appManifest.devDependencies || {}), ].filter((moduleName) => !bundledModules.includes(moduleName));🤖 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 `@rollup.config.mjs` around lines 205 - 210, Update makeExternal’s externalModules construction to safely handle missing appManifest.dependencies and appManifest.devDependencies by supplying empty-object fallbacks before calling Object.keys(), while preserving the existing module filtering behavior.
🤖 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 `@docs/KNOWN_ISSUES.md`:
- Around line 68-97: Update the “Root cause / rationale” security claim in the
auto-updater risk-acceptance entry to distinguish Linux/AppImage updates from
Windows and macOS: describe Linux/AppImage protection as SHA-512 integrity
verification, while retaining built-in code-signature verification only for the
supported Windows/macOS paths. Ensure the “Decision” rationale uses this
accurately scoped artifact-validation behavior.
In `@scripts/check-bundle-externals.mjs`:
- Around line 42-44: Update LOCAL_CHUNK_PATTERN to match both ./ and ../
relative JavaScript paths using an optional second dot and a non-capturing path
group, while preserving require and from forms. Then update the match extraction
to use the single captured path via match[1] wherever local chunks are
traversed.
In `@src/screenSharing/desktopCapturerCache.ts`:
- Around line 145-150: Update clearDesktopCapturerCache to invalidate any
in-flight enumerateBucket operation by advancing the existing enumeration
generation/token before resetting cache state. Ensure stale enumeration
completion cannot repopulate buckets or clear a newer enumerationPromise, while
preserving single-flight behavior for new refreshes.
In `@src/screenSharing/main/desktopCapturerCache.main.spec.ts`:
- Around line 165-184: Extend the test around refreshDesktopCapturerCache with a
second empty refresh after the cooldown interval, awaiting its microtasks, then
assert that the existing emptyWarnings count remains one. Keep the current
populated-cache setup, warning filter, and timer behavior unchanged.
---
Nitpick comments:
In `@rollup.config.mjs`:
- Around line 205-210: Update makeExternal’s externalModules construction to
safely handle missing appManifest.dependencies and appManifest.devDependencies
by supplying empty-object fallbacks before calling Object.keys(), while
preserving the existing module filtering 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10be744e-95f1-48c1-9120-99f9b66a1825
📒 Files selected for processing (11)
docs/KNOWN_ISSUES.mdpackage.jsonrollup.config.mjsscripts/check-bundle-externals.mjssrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/desktopCapturerCache.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/screenSharing/screenSharePicker.tsxsrc/ui/components/SideBar/ServerButton.tsxsrc/ui/components/SideBar/index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/ui/components/SideBar/index.tsxsrc/ui/components/SideBar/ServerButton.tsxsrc/screenSharing/ScreenSharingRequestTracker.tssrc/screenSharing/screenSharePicker.tsxsrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/desktopCapturerCache.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use React functional components with hooks.
Files:
src/ui/components/SideBar/index.tsxsrc/ui/components/SideBar/ServerButton.tsxsrc/screenSharing/screenSharePicker.tsx
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{md,mdx}: Avoid subjective descriptors and use measurable descriptions.
Never invent metrics; use only numbers from actual logs, error messages, or documented sources.
PR descriptions should use straightforward language and focus on what changed and why.
Files:
docs/KNOWN_ISSUES.md
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Main-process specs use
*.main.spec.ts.Use
*.main.spec.tsfor main process tests.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
🪛 LanguageTool
docs/KNOWN_ISSUES.md
[uncategorized] ~73-~73: The official name of this software platform is spelled with a capital “H”.
Context: ...ases (electron-builder.json: provider github, owner RocketChat, repo `Rocket.Ch...
(GITHUB)
[style] ~76-~76: ‘advance notice’ might be wordy. Consider a shorter alternative.
Context: ... rotates on its own schedule, with no advance notice to Rocket.Chat. - Root cause / rational...
(EN_WORDINESS_PREMIUM_ADVANCE_NOTICE)
🔇 Additional comments (16)
src/ui/components/SideBar/index.tsx (1)
106-108: LGTM!src/ui/components/SideBar/ServerButton.tsx (1)
146-146: LGTM!rollup.config.mjs (5)
249-249: LGTM!
279-279: LGTM!
310-316: LGTM!
347-347: LGTM!
406-406: LGTM!package.json (1)
33-33: LGTM!src/screenSharing/desktopCapturerCache.ts (1)
5-143: LGTM!Also applies to: 153-193
src/screenSharing/ScreenSharingRequestTracker.ts (2)
4-4: LGTM!Also applies to: 96-113
114-117: 🎯 Functional CorrectnessConfirm the cache-empty fallback source type. The fallback still calls
desktopCapturer.getSources({ types: ['window', 'screen'] }); split it to a single source type only if this path is meant to avoid the Electron/macOS cap.src/screenSharing/screenSharePicker.tsx (1)
87-90: LGTM!docs/KNOWN_ISSUES.md (2)
5-15: 📐 Maintainability & Code QualityReference the measurement artifacts for these exact figures.
The
24s,3s,75%,700ms, and4sclaims drive the workaround constants but have no traceable log or source here. Link the captured results or upstream documentation.As per coding guidelines, “Never invent metrics; use only numbers from actual logs, error messages, or documented sources.” <coding_guidelines>
Source: Coding guidelines
26-67: LGTM!src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
5-63: LGTM!Also applies to: 112-147, 186-186
src/screenSharing/main/desktopCapturerCache.main.spec.ts (1)
6-143: LGTM!Also applies to: 145-164, 185-318, 328-337, 350-365, 378-453
| ## No certificate pinning for the auto-updater (CORE-1128) — accepted risk, not a gap | ||
|
|
||
| - Status: Resolved as risk acceptance (2022 pentest finding CORE-1128; no code change). | ||
| - Symptom: N/A — this documents a deliberate decision, not an observed bug. | ||
| - Context: CORE-1128 flagged "Missing Certificate Pinning for Connections and autoUpdater | ||
| Mechanism." The update feed is GitHub Releases (`electron-builder.json`: provider `github`, | ||
| owner `RocketChat`, repo `Rocket.Chat.Electron`); update artifacts are served from | ||
| `objects.githubusercontent.com`, a domain whose TLS certificate GitHub controls and rotates | ||
| on its own schedule, with no advance notice to Rocket.Chat. | ||
| - Root cause / rationale: electron-updater has no built-in certificate-pinning configuration. | ||
| The only available hook (`session.setCertificateVerifyProc()` via `getNetSession()`) is not | ||
| exposed on the public `autoUpdater` singleton, so pinning would require reaching into | ||
| electron-updater internals and would be fragile across version upgrades. More importantly, | ||
| electron-updater already verifies the integrity and authenticity of downloaded update | ||
| artifacts via code-signature verification (Windows: `verifyUpdateCodeSignature`; macOS/AppImage: | ||
| built-in signature validation), so a network-level MITM attacker cannot get a forged or | ||
| malicious build installed even without TLS pinning — they would need a validly-signed | ||
| Rocket.Chat build, a materially higher bar than compromising a CA. GitHub has previously | ||
| rotated certificates on `objects.githubusercontent.com` in ways that broke clients with | ||
| pinned certs/CAs (GitHub community discussion #50963 on release downloads failing after a | ||
| cert rotation; also discussed on Hacker News regarding GitHub User Content certificate expiry | ||
| incidents). Pinning any cert or CA on this GitHub-hosted domain risks a future GitHub-side | ||
| rotation silently breaking auto-update for every user, with no fix available on Rocket.Chat's | ||
| side — a full, RC-unfixable auto-update outage until a new client version ships through some | ||
| other channel. | ||
| - Decision: Do not implement certificate pinning for the auto-updater. The residual risk pinning | ||
| would mitigate (CA compromise / MITM on the update channel) is already substantially covered by | ||
| HTTPS + system trust store + code-signature verification of the downloaded artifact. The blast | ||
| radius of a stale or broken pin exceeds the risk it would remove. | ||
| - Affected files: electron-builder.json (update feed config); no source changes made. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
fd -a '^(package\.json|yarn\.lock|package-lock\.json|pnpm-lock\.yaml)$' . |
while read -r file; do
rg -n -C2 'electron-updater|electron-builder' "$file" || true
done
rg -n -C4 \
'AppImageUpdater|verifyUpdateCodeSignature|autoUpdater|electron-updater' \
. -g '*.ts' -g '*.tsx' -g '*.mjs' -g '*.json'Repository: RocketChat/Rocket.Chat.Electron
Length of output: 1977
🌐 Web query:
electron-builder auto-update AppImage signature validation Linux documentation
💡 Result:
Electron-builder supports GPG signing for AppImage builds, which allows you to embed a digital signature directly into the AppImage file [1]. This functionality was introduced to align with the official AppImage signing specification, which embeds an ASCII-armored GPG detached signature into the.sha256_sig ELF section of the AppImage runtime [1]. However, it is important to distinguish between signing an AppImage and the automated validation of that signature during the auto-update process performed by electron-updater: 1. Signing Support: You can configure electron-builder to sign your AppImages during the build process. This is achieved using the appImage.sign configuration object or via environment variables such as APPIMAGE_SIGN, APPIMAGE_GPG_KEY_ID, APPIMAGE_GPG_PATH, and APPIMAGE_GPG_PASSPHRASE [1]. 2. Auto-Update Validation: The electron-updater package facilitates updates but does not natively perform automatic GPG signature validation on the downloaded AppImage as part of its standard auto-update workflow [1][2]. While the AppImage itself can carry a signature, validating that signature during an update usually requires external tools or custom logic, as the official AppImage specification notes that validation must be handled by external tools (e.g., via the validate tool from the AppImageUpdate project) [2]. 3. Linux Package Verification: While AppImage signature verification is distinct, electron-builder recently added an opt-in feature for other Linux package formats (deb/rpm) via AppUpdater.allowUnverifiedLinuxPackages, which allows developers to enforce signature checks for those formats if they are signing their packages through their own custom pipelines [3]. In summary, while you can build and sign your AppImages using electron-builder, the electron-updater library does not currently provide an automated, built-in mechanism to verify these GPG signatures during the auto-update download and installation process [1][2]. Developers wishing to enforce this security check would need to implement custom validation logic after the update is downloaded but before it is executed [1][2].
Citations:
- 1: feat(appimage): add GPG signing support for AppImage builds electron-userland/electron-builder#9651
- 2: https://docs.appimage.org/packaging-guide/optional/signatures.html?highlight=signing
- 3: feat(updater): fix manifest sha512 hash-encoding sniffing, add opt-in Linux package-signature verification electron-userland/electron-builder#9990
🌐 Web query:
site:electron.build electron-updater AppImage auto-update signature validation Linux
💡 Result:
In electron-updater, AppImage auto-update mechanisms focus primarily on integrity verification rather than traditional cryptographic signature validation (e.g., GPG) for Linux platforms [1][2]. Key details regarding update validation for AppImage in electron-updater include: 1. Integrity Verification: electron-updater relies on SHA-512 checksums to ensure file integrity [3][4]. When you configure your update provider (such as GitHub, S3, or generic), electron-builder generates update metadata that includes the SHA-512 hash of the new version [2][4]. The updater uses this hash to verify that the downloaded AppImage has not been corrupted or tampered with before execution. 2. Blockmap Support: AppImages support differential (delta) updates by embedding a blockmap directly into the binary at build time [2]. This allows the updater to download only changed blocks, and the integrity of the resulting file is validated against the provided hash [2][3]. 3. Signature Verification Limitations: Unlike Windows (NSIS) or macOS (Squirrel.Mac), where code signing is a native requirement for OS-level update security, electron-builder and electron-updater do not provide a built-in, mandatory cryptographic signature validation flow (like GPG verification) specifically for AppImage updates on Linux [1][2]. While official documentation notes that code signature validation is supported on macOS and Windows, it is generally not listed as a feature for Linux AppImage auto-updates [1]. 4. Security Configuration: The electron-updater API includes a disableWebInstaller option [5][6]. When set to true, this prevents the loading of files that may lack proper signature verification in contexts where such verification is supported [5][6]. However, its applicability to standard Linux AppImage auto-updating is limited because the underlying AppImage update process primarily relies on the aforementioned checksum validation [2]. In summary, for Linux AppImage deployments, you should ensure your update server is securely hosted (e.g., via HTTPS) and that you rely on the built-in SHA-512 integrity checks provided by electron-updater [2][3]. If strict cryptographic signature validation is a compliance requirement, it must be implemented as a custom wrapper or post-download hook outside of the standard electron-updater process.
Citations:
- 1: https://www.electron.build/docs/features/auto-update/
- 2: https://www.electron.build/appimage
- 3: https://www.electron.build/electron-updater.interface.packagefileinfo
- 4: https://www.electron.build/electron-updater.interface.updateinfo
- 5: https://www.electron.build/electron-updater.class.appimageupdater
- 6: https://www.electron.build/electron-updater.class.appupdater
Scope the AppImage security claim to Linux integrity checks
Lines 81-85 overstate AppImage protection: electron-updater’s Linux/AppImage updates rely on SHA-512 integrity checks, not built-in code-signature validation. Split the Linux/AppImage behavior from the Windows/macOS signature path before using it to justify the risk acceptance.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~73-~73: The official name of this software platform is spelled with a capital “H”.
Context: ...ases (electron-builder.json: provider github, owner RocketChat, repo `Rocket.Ch...
(GITHUB)
[style] ~76-~76: ‘advance notice’ might be wordy. Consider a shorter alternative.
Context: ... rotates on its own schedule, with no advance notice to Rocket.Chat. - Root cause / rational...
(EN_WORDINESS_PREMIUM_ADVANCE_NOTICE)
🤖 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 `@docs/KNOWN_ISSUES.md` around lines 68 - 97, Update the “Root cause /
rationale” security claim in the auto-updater risk-acceptance entry to
distinguish Linux/AppImage updates from Windows and macOS: describe
Linux/AppImage protection as SHA-512 integrity verification, while retaining
built-in code-signature verification only for the supported Windows/macOS paths.
Ensure the “Decision” rationale uses this accurately scoped artifact-validation
behavior.
| const LOCAL_CHUNK_PATTERN = | ||
| /require\(['"](\.\/[^'"]+\.js)['"]\)|from ['"](\.\/[^'"]+\.js)['"]/g; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Support ../ paths in the local chunk regex.
The current LOCAL_CHUNK_PATTERN only matches paths starting with ./. Nested entry points like preload/preload.js will likely reference shared chunks hoisted to the parent directory using paths starting with ../. If the regex misses these paths, the script will silently fail to traverse those shared chunks, negating the regression guard for them.
You can fix this by allowing an optional second dot (\.\.?\/). Using a non-capturing group (?:...) also simplifies the regex so that the matched path is always in the first capture group.
🐛 Proposed fix
-const LOCAL_CHUNK_PATTERN =
- /require\(['"](\.\/[^'"]+\.js)['"]\)|from ['"](\.\/[^'"]+\.js)['"]/g;
+const LOCAL_CHUNK_PATTERN =
+ /(?:require\(['"]|from ['"])(\.\.?\/[^'"]+\.js)['"]/g;Update the match extraction on line 75 to reflect the single capture group:
// scripts/check-bundle-externals.mjs (line 75)
const relativePath = match[1];🤖 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 42 - 44, Update
LOCAL_CHUNK_PATTERN to match both ./ and ../ relative JavaScript paths using an
optional second dot and a non-capturing path group, while preserving require and
from forms. Then update the match extraction to use the single captured path via
match[1] wherever local chunks are traversed.
| export const clearDesktopCapturerCache = (): void => { | ||
| desktopCapturerCache = null; | ||
| desktopCapturerPromise = null; | ||
| sourceValidationCache.clear(); | ||
| sourceValidationCacheTimestamp = 0; | ||
| buckets.screens = null; | ||
| buckets.windows = null; | ||
| enumerationPromise = null; | ||
| lastEnumerationCompletedAt = 0; | ||
| warnedEmptyOnce = { screens: false, windows: false }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Invalidate in-flight enumerations when clearing the cache.
An existing enumerateBucket continues after this reset and can repopulate the cache. If a new refresh starts meanwhile, the old promise’s finalizer can also clear the new enumerationPromise, breaking single-flight tracking.
Proposed generation-based invalidation
+let cacheGeneration = 0;
-const enumerateBucket = async (bucketType: BucketType): Promise<void> => {
+const enumerateBucket = async (
+ bucketType: BucketType,
+ generation: number
+): Promise<void> => {
try {
const sources = await desktopCapturer.getSources({
types: typeForBucket(bucketType),
});
+ if (generation !== cacheGeneration) return;
// Update the bucket...
} finally {
- lastEnumerationCompletedAt = Date.now();
+ if (generation === cacheGeneration) {
+ lastEnumerationCompletedAt = Date.now();
+ }
}
};
- enumerationPromise = enumerateBucket(bucketType).finally(() => {
- enumerationPromise = null;
+ const scheduled = enumerateBucket(bucketType, cacheGeneration).finally(() => {
+ if (enumerationPromise === scheduled) enumerationPromise = null;
});
+ enumerationPromise = scheduled;
export const clearDesktopCapturerCache = (): void => {
+ cacheGeneration += 1;
buckets.screens = null;🤖 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/screenSharing/desktopCapturerCache.ts` around lines 145 - 150, Update
clearDesktopCapturerCache to invalidate any in-flight enumerateBucket operation
by advancing the existing enumeration generation/token before resetting cache
state. Ensure stale enumeration completion cannot repopulate buckets or clear a
newer enumerationPromise, while preserving single-flight behavior for new
refreshes.
| it('warns only once per occurrence of an empty result on a populated bucket', async () => { | ||
| const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); | ||
|
|
||
| getSourcesMock.mockResolvedValueOnce([ | ||
| makeSource('screen:0', 'Screen 1') as any, | ||
| ]); | ||
| refreshDesktopCapturerCache({ types: ['screen'] }); | ||
| await flushMicrotasks(); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(result[0].id).toBe('3'); | ||
| jest.advanceTimersByTime(4001); | ||
|
|
||
| getSourcesMock.mockResolvedValueOnce([]); | ||
| refreshDesktopCapturerCache({ types: ['screen'] }); | ||
| await flushMicrotasks(); | ||
|
|
||
| const emptyWarnings = warnSpy.mock.calls.filter(([msg]) => | ||
| String(msg).includes('keeping previous cache') | ||
| ); | ||
| expect(emptyWarnings).toHaveLength(1); | ||
| warnSpy.mockRestore(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Exercise repeated empty results before asserting warning throttling.
This test performs only one empty refresh, so it still passes if every empty refresh warns. Issue a second empty refresh after the cooldown and confirm the warning count remains one.
Proposed regression coverage
- const emptyWarnings = warnSpy.mock.calls.filter(([msg]) =>
- String(msg).includes('keeping previous cache')
- );
- expect(emptyWarnings).toHaveLength(1);
+ const emptyWarnings = () =>
+ warnSpy.mock.calls.filter(([msg]) =>
+ String(msg).includes('keeping previous cache')
+ );
+ expect(emptyWarnings()).toHaveLength(1);
+
+ jest.advanceTimersByTime(4001);
+ getSourcesMock.mockResolvedValueOnce([]);
+ refreshDesktopCapturerCache({ types: ['screen'] });
+ await flushMicrotasks();
+
+ expect(emptyWarnings()).toHaveLength(1);📝 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.
| it('warns only once per occurrence of an empty result on a populated bucket', async () => { | |
| const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); | |
| getSourcesMock.mockResolvedValueOnce([ | |
| makeSource('screen:0', 'Screen 1') as any, | |
| ]); | |
| refreshDesktopCapturerCache({ types: ['screen'] }); | |
| await flushMicrotasks(); | |
| expect(result).toHaveLength(1); | |
| expect(result[0].id).toBe('3'); | |
| jest.advanceTimersByTime(4001); | |
| getSourcesMock.mockResolvedValueOnce([]); | |
| refreshDesktopCapturerCache({ types: ['screen'] }); | |
| await flushMicrotasks(); | |
| const emptyWarnings = warnSpy.mock.calls.filter(([msg]) => | |
| String(msg).includes('keeping previous cache') | |
| ); | |
| expect(emptyWarnings).toHaveLength(1); | |
| warnSpy.mockRestore(); | |
| it('warns only once per occurrence of an empty result on a populated bucket', async () => { | |
| const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); | |
| getSourcesMock.mockResolvedValueOnce([ | |
| makeSource('screen:0', 'Screen 1') as any, | |
| ]); | |
| refreshDesktopCapturerCache({ types: ['screen'] }); | |
| await flushMicrotasks(); | |
| jest.advanceTimersByTime(4001); | |
| getSourcesMock.mockResolvedValueOnce([]); | |
| refreshDesktopCapturerCache({ types: ['screen'] }); | |
| await flushMicrotasks(); | |
| const emptyWarnings = () => | |
| warnSpy.mock.calls.filter(([msg]) => | |
| String(msg).includes('keeping previous cache') | |
| ); | |
| expect(emptyWarnings()).toHaveLength(1); | |
| jest.advanceTimersByTime(4001); | |
| getSourcesMock.mockResolvedValueOnce([]); | |
| refreshDesktopCapturerCache({ types: ['screen'] }); | |
| await flushMicrotasks(); | |
| expect(emptyWarnings()).toHaveLength(1); | |
| warnSpy.mockRestore(); |
🤖 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/screenSharing/main/desktopCapturerCache.main.spec.ts` around lines 165 -
184, Extend the test around refreshDesktopCapturerCache with a second empty
refresh after the cooldown interval, awaiting its microtasks, then assert that
the existing emptyWarnings count remains one. Keep the current populated-cache
setup, warning filter, and timer behavior unchanged.
1 participant
What
Backports three master fixes onto the 4.15.5 hotfix base so they ship before 4.16:
0dafe394d)9a6b84525(adapted)ReferenceError: exports is not definedfrom the bundledreact-dom/clientchunk in the video call window, which broke screen sharing there (part of SUP-1072's log signature)dddd2ccd6)0ef67bd78+2bf4e4012e405a6a6e)26bf7400d(clean cherry-pick)Jira: SUP-1072 (rollup externals item). Companion PR: #3416.
Adaptation notes
rollup.config.mjsis post-React-19, so themakeExternal()subpath matcher andscripts/check-bundle-externals.mjsguard were ported to 4.15.4's six-bundle config, preserving each bundle's intentionally-bundled exceptions verbatim (@bugsnag/js; rootWindow'smarked/marked-highlight/highlight.js/dompurify). The guard'sscreen-picker-window.jsentry was dropped: 4.15.4 has no such rollup entry — the picker mounts via avideo-call-window.jschunk, which the existingreact-dom/clientassertion already covers.yarn buildnow runs the guard (bundle externals check passed, 28 chunks scanned; verified on a clean build that no React internals are bundled in any window bundle).entry/finishActive) that doesn't exist here;2bf4e4012restores 4.15.4's owncb(...)/markComplete()convention ({ video: false }deny, consistent with all five existing deny paths in this branch). One spec assertion aligned to the same contract.docs/KNOWN_ISSUES.mdmerge kept all sections; a stray conflict-marker fragment was removed (561f2538b).Validation
yarn lint,npx tsc --noEmit: cleanyarn testfull suite: 72/72 suites, 1171 passed / 2 skipped / 0 failedyarn buildfrom scratch: all bundles + new externals guard passvideo-call-window.jschunk graph confirmed free of bundled React internals;react-dom/clientcorrectly externalizedSummary by CodeRabbit
Bug Fixes
Documentation