FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(CORE-2400): remove startup screen capture enumeration that opens Wayland picker (#3308) by jeanfbrito · Pull Request #3400 · RocketChat/Rocket.Chat.Electron · GitHub

fix(CORE-2400): remove startup screen capture enumeration that opens Wayland picker (#3308) - #3400

Merged
jeanfbrito merged 1 commit into
masterfrom
fix/3308-picker-mount-fetch
Jul 9, 2026
Merged

fix(CORE-2400): remove startup screen capture enumeration that opens Wayland picker (#3308)#3400
jeanfbrito merged 1 commit into
masterfrom
fix/3308-picker-mount-fetch

Conversation

jeanfbrito commented Jul 9, 2026
edited by atlassian Bot
Loading

Copy link
Copy Markdown
Member

What

Removes a mount-time desktopCapturer.getSources() call from ScreenSharePicker. The component is pre-mounted (hidden) in the root window at every startup and in the video-call window; its mount effect enumerated capture sources before any user action. A visible-gated effect already fetches sources when the picker actually opens (and refreshes every 3s while open), so the mount-time fetch was redundant.

Why

On Linux Wayland with WebRTCPipeWireCapturer (forced for all Linux sessions), source enumeration routes through the XDG desktop portal, which presents the system screen-share dialog. This made the picker dialog appear at app launch with no user interaction — reported in #3308 (Fedora Flatpak, Ubuntu deb, Wayland, since 4.14.0).

This startup path never consults detectPickerType(), which is why the sandbox-safe detection fix (#3386) did not cover it.

Verification

  • tsc --noEmit, eslint, and 24 targeted tests pass (Shell/index.spec.tsx, desktopCapturerCache.main.spec.ts)
  • Electron 42 (current, 4.15.x) — GPU-passthrough Wayland VM (fedora42 + GTX 970/nouveau, GNOME Wayland), dbus-level A/B, same boot, fresh profiles:
    • Stock 4.15.2 AppImage: org.freedesktop.portal.ScreenCast CreateSession + SelectSources issued at every launch (2/2 runs) — SelectSources is the stage that shows the system dialog. Matches reporters' "picker every launch".
    • Same AppImage rebuilt with only this fix: zero ScreenCast portal calls (2/2 runs).
    • Note: this trigger only manifests with hardware GL — software-rendered VMs (llvmpipe) never initialize the PipeWire capture path, which is why earlier VM sweeps couldn't reproduce the 4.15.x reports.
  • Electron 40-era build (4.14.1) — fresh-boot visual A/B (mOSdat, fedora42): picker dialog confirmed at launch on the old build (VLM-verified screenshot); no picker across 3/3 fresh-boot launches with the fix (verify-fix verdict NOT_REPRODUCED)
  • On-demand screen sharing is unaffected: the request-driven portal/internal picker path (setDisplayMediaRequestHandler → provider) does not use the removed effect

Role of the Electron version (40 → 42)

The Electron upgrade neither caused nor fixes this bug — it only changed where it manifests, which is what made diagnosis non-obvious:

  • Electron 40 (4.14.x): Chromium's capture stack routed getSources() through the XDG portal even under software rendering — the dialog reproduced everywhere, including software-GL VMs.
  • Electron 42 (4.15.x): Chromium initializes the PipeWire capture path only when hardware GL is available. Software-rendered VMs return silently from the same call (masking the bug in test environments), while every real machine — all reporters — kept hitting the portal on each launch.

The trigger in both eras is the same application code path removed by this PR. Upgrading Electron further would not help: enumerating capture sources on Wayland goes through the portal by design; the defect was enumerating at startup without user intent.

Scope notes

Closes #3308
Jira: CORE-2400

coderabbitai Bot commented Jul 9, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

The ScreenSharePicker component's effect logic was modified to check screen-recording permission via an IPC call (ipcRenderer.invoke(permissionChannel)) triggered by changes to visible or permissionChannel. A separate effect that only called fetchSources() was removed, consolidating source fetching into the existing visibility-guarded effect.

Changes

Screen Share Picker Permission Effect

Layer / File(s) Summary
Permission check effect and fetchSources consolidation
src/screenSharing/screenSharePicker.tsx
Adds an async IPC permission check effect keyed on visible/permissionChannel to update isScreenRecordingPermissionGranted, and removes the standalone fetchSources()-only effect.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Suggested labels: type: bug

🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: removing startup screen capture enumeration that triggered the Wayland picker.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • CORE-2400: Request failed with status code 401

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

jeanfbrito merged commit e1e5740 into master Jul 9, 2026
9 checks passed
jeanfbrito deleted the fix/3308-picker-mount-fetch branch July 9, 2026 13:16
jeanfbrito added a commit that referenced this pull request Jul 9, 2026
… rule (#3402)

* docs: post-mortems for #3308 screen-picker triggers + capture-testing rule

Two-phase investigation of the launch-time screen-share picker on Linux
Wayland (#3308, fixed by #3386 and #3400). Adds the phase-2 post-mortem
covering the startup-enumeration trigger, the Electron 40 vs 42
manifestation split, and the verification protocol; commits the phase-1
sandbox-detection post-mortem that previously lived untracked.

CLAUDE.md gains the extracted testing rule: Chromium capture/portal
behavior requires hardware GL to validate — software-rendered VMs mask
it; assert on dbus ScreenCast requests, not dialog visibility.

* docs: refresh GitNexus section in CLAUDE.md

Tool-generated section update: current index stats, short tool names
(impact/query/context/rename), detect_changes compare syntax, and the
run.cjs analyze entrypoint.
jeanfbrito added a commit that referenced this pull request Jul 10, 2026
* fix: honor uniqueId-scoped support exceptions on server-signed path

The exception scope check introduced in #3323 requires
exceptions.uniqueId to equal server.uniqueID, but the server-signed
validation path returns before getUniqueId() ever runs and /api/info
does not include a uniqueId field, so the local uniqueID is missing on
that path and uniqueId-scoped exceptions were always disqualified.

Resolve the workspace uniqueID from the server before validating when
the payload carries a uniqueId-scoped exceptions block, persist it via
WEBVIEW_SERVER_UNIQUE_ID_UPDATED so subsequent runs (including offline
cache validation) keep working, and keep rejecting when the fetched
value does not match. Also compare exceptions.domain case-insensitively
(DNS names are case-insensitive per RFC 4343).

* fix: harden supported-versions validation and recovery paths

Audited the supported-versions subsystem end to end after the
uniqueId-scope escalation and fixed the confirmed defects:

- Wrap the cache/builtin fallback validation in a helper with try/catch
  so a malformed cached payload can no longer reject the update before
  the error state is dispatched, which left fetchState stuck at
  'loading' and suppressed the UnsupportedServer block gate.
- Catch rejections at all four fire-and-forget validation call sites.
- Fall back to the persisted workspace uniqueID when the fresh fetch
  fails so the cloud lookup is not skipped for previously-known servers.
- Switch uniqueID/version/gitCommitHash reducer cases from upsert to
  update so a late identity dispatch cannot resurrect a server deleted
  while a validation was in flight.
- Guard getExpirationMessageTranslated against payloads whose i18n
  dictionary lacks both the user language and 'en'; the missing guard
  crashed the async check and silently suppressed the expiring-
  workspace warning dialog.
- Revalidate all servers on powerMonitor 'resume' (the window 'online'
  event does not fire when waking with the same network connected).
- Use the real currentView state in SupportedVersionDialog's effect
  dependencies; the previous dependency was the imported reducer
  function, which never changes, so the dialog never re-checked on
  view switches.
- Add a 'Check again' button to the unsupported-workspace screen so
  users can re-trigger validation without restarting the app.
- Fix logRequestError printing a literal ${description}.

Also relax the exception scope check for UNKNOWN local identity: an
unfetchable workspace uniqueID (e.g. settings.public restricted by
enterprise API ACLs) no longer disqualifies a domain-matched exception;
a PROVEN uniqueID mismatch still rejects. The gate is client-side UX
enforcement rather than a security boundary, and wrongly blocking a
legitimate workspace is the worse failure mode.

* fix: honor tenant-scoped exceptions from self-scoped payload sources

Server, cloud, and cache supported-versions payloads are fetched from or
for the server being validated, so their exceptions block cannot belong
to another tenant. For these sources a domain/uniqueId scope mismatch is
now logged as a diagnostic warning instead of disqualifying the
exception. The bundled builtin payload is the only source that could
carry another deployment's exceptions and keeps the strict scope
requirement.

This removes the remaining paths where a valid, unexpired exception
could be rejected over unverifiable or drifted identity data (restricted
settings.public API, rotated workspace uniqueID, stale persisted
state).

* fix: do not block when enforcementStartDate is missing or malformed

A missing or unparseable enforcementStartDate previously produced an
Invalid Date that failed the future-date comparison and fell through to
the unsupported verdict — blocking the workspace based on incomplete
payload data. Blocking now requires a valid, past enforcement date:
uncertain data keeps the server usable until a payload with a valid
enforcement date proves enforcement is active.

* fix: align supported-versions types and fixtures with the real /api/info contract; try both fallback sources

The desktop's ServerInfo type declared fields the server never returns
to unauthenticated callers (verified against the server source,
apps/meteor/server/api/lib/getServerInfo.ts):

- Remove the fictional uniqueId field. No server version includes it in
  /api/info; the workspace uniqueID comes from settings.public and is
  resolved on demand by withExceptionScopeUniqueId. Remove the dead
  serverInfoResult.uniqueId fallback in updateSupportedVersionsData.
- Mark build, marketplaceApiVersion, and commit optional and document
  that they only appear for authenticated view-statistics callers; the
  desktop always calls /api/info unauthenticated. The sha-exception
  path uses the persisted server.gitCommitHash pushed by the injected
  script instead.
- Rebuild the default test fixture to mirror the real unauthenticated
  wire shape (trimmed major.minor version, no uniqueId/commit/info) so
  the suite can no longer validate assumptions against fields real
  servers do not send — the gap that let the exception-scope defect
  pass CI. Switch beforeEach to jest.resetAllMocks(), fixing latent
  cross-test mock leakage the honest fixtures surfaced.

Also rework the offline fallback to try BOTH remaining sources instead
of picking one by timestamp: the bundled builtin payload never carries
tenant exceptions, so preferring a fresher builtin over a cache that
holds the tenant's exception blocked a workspace that should stay
usable. Candidates are checked in freshness order and the first source
that supports the server wins; blocking requires every available source
to fail. This keeps the stale-cache rescue from #3388 while never
losing a cached exception to it.

* test: add live /api/info wire-contract spec

Asserts the unauthenticated /api/info response shape against a real
server (open.rocket.chat): trimmed major.minor version, no uniqueId,
no commit/info blocks, string minimumClientVersions, JWT-shaped
supportedVersions.signed. Skips only on genuine network failure or
SKIP_CONTRACT_TESTS=1, so contract drift between the desktop and the
server cannot hide behind mocks that agree with themselves.

* fix(CORE-2400): remove startup screen capture enumeration that opens Wayland picker (#3308) (#3400)

* chore: bump version to 4.15.3
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Screen share picker opens on every app launch (regression since 4.14.0, likely PR #3266)

1 participant


Back | FazBrowse Home | New Git URL