fix(screen-mirror): grant display-capture permission + Flatpak pipeline - #267
Closed
Muneerali199 wants to merge 7 commits into
Closed
fix(screen-mirror): grant display-capture permission + Flatpak pipeline#267Muneerali199 wants to merge 7 commits into
Muneerali199 wants to merge 7 commits into
Conversation
…utables - Add setPermissionRequestHandler and setPermissionCheckHandler to BrowserWindow session so getDisplayMedia is not silently denied in packaged builds (Bug AOSSIE-Org#254) - Add macOS screen recording permission check at app startup - Add contextIsolation/nodeIntegration webPreferences - Add macOS activate handler for dock re-open - Add linux target (dir) to package.json for electron-builder - Add flatpak/ directory with Flatpak manifest, .desktop file and AppStream metadata - Add .github/workflows/flatpak.yml CI pipeline for Flatpak distribution (Issue AOSSIE-Org#206)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds Flatpak packaging and CI, Flatpak metadata and launcher, and updates Electron main process with session/systemPreferences and permission handling (including macOS screen-recording prompts and dock activate behavior). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… script The tool was refactored and is no longer a single flatpak-node-generator.py file at the repo root. Install it via pip from the subdirectory source and invoke it as a console command instead of running a downloaded .py file.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/flatpak.yml:
- Around line 5-8: The push trigger currently ignores only branches via the
push: branches-ignore setting; restrict its scope by adding path filters under
push (e.g., include a paths or paths-ignore block) so CI only runs when
Flatpak-relevant files change. Update the push trigger in
.github/workflows/flatpak.yml (the push: section) to mirror the pull_request
path rules—for example, add paths: for flatpak packaging files and workflow
files or add paths-ignore for unrelated code—to avoid running builds on
unrelated feature branch pushes.
- Around line 41-44: The workflow step that downloads flatpak-node-generator.py
should pin the exact commit and verify integrity: change the raw GitHub URL to
the file at a specific commit (reference flatpak-node-generator.py in the
"Download flatpak-node-generator" step), fetch or embed a known-good checksum
for that commit, and verify the downloaded file (e.g., compare SHA256) before
executing; alternatively vendor the script or use a versioned release and update
the workflow to fail if the checksum does not match so the CI never runs
unverified code.
- Around line 38-39: The workflow step named "Install flatpak-node-generator"
installs aiohttp and aiofiles without pinning; change that to install pinned
versions (or use a constraints/requirements file) to ensure reproducible builds.
Update the pip3 install command that currently lists "aiohttp" and "aiofiles" so
each dependency has an explicit version specifier (e.g., aiohttp==<version>,
aiofiles==<version>) or replace the inline install with "pip3 install -r
requirements.txt" referencing a checked-in requirements file with exact
versions.
In `@electron/main.cjs`:
- Around line 103-112: The code checks
systemPreferences.getMediaAccessStatus('screen') but then incorrectly calls
systemPreferences.askForMediaAccess('camera'); remove that call and instead
handle the macOS screen-recording limitation: delete or stop calling
askForMediaAccess('camera') in the macOS branch and replace it with a clear
user-facing action (e.g., show a dialog/notification from the main process
explaining the user must enable Screen Recording in System Preferences → Privacy
& Security → Screen Recording, or link to the docs) and/or add an inline comment
documenting that screen-recording permission cannot be requested
programmatically; reference systemPreferences.getMediaAccessStatus and
systemPreferences.askForMediaAccess to locate the change.
In `@flatpak/com.rein.app.metainfo.xml`:
- Around line 27-29: Add a brief release description inside the <release>
element to improve AppStream discoverability: update the <release
version="1.0.0" date="2026-03-08"/> element (child of <releases>) to include a
<description>...</description> child summarizing the changes in this version so
each <release> has a concise human-readable description.
In `@flatpak/com.rein.app.yml`:
- Around line 12-22: The manifest's finish-args currently include X11 and device
permissions but lack explicit Wayland portal permissions needed for
getDisplayMedia under Wayland; update finish-args to add the Wayland socket and
grant the ScreenCast portal permission (e.g., add --socket=wayland and
--talk-name=org.freedesktop.portal.ScreenCast alongside the existing
--socket=x11 and --device=dri) so the app can use XDG Desktop Portal
screen-capture on Wayland.
- Around line 42-43: The jq patch command line shown (jq
'.build.linux.target="dir"' <<<$(<package.json) > package.json) reads and writes
package.json simultaneously which will corrupt the file; since package.json
already contains "target": "dir" under build.linux, remove this redundant jq
line entirely, or if you must keep it for safety replace it with a safe in-place
pattern (write to a temp file or use sponge) so you do not redirect into the
same file while reading. Ensure the change targets the exact jq invocation in
the diff so no other package.json manipulations are affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bff0992-4352-4063-8b22-8a16c722272e
📒 Files selected for processing (6)
.github/workflows/flatpak.ymlelectron/main.cjsflatpak/com.rein.app.desktopflatpak/com.rein.app.metainfo.xmlflatpak/com.rein.app.ymlpackage.json
Comment on lines
+38
to
+39
| - name: Install flatpak-node-generator | ||
| run: pip3 install aiohttp aiofiles |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin Python dependency versions for reproducible builds.
Installing aiohttp and aiofiles without version constraints can lead to unexpected breakage if a new release introduces incompatibilities.
♻️ Proposed fix
- name: Install flatpak-node-generator
- run: pip3 install aiohttp aiofiles
+ run: pip3 install aiohttp==3.9.5 aiofiles==23.2.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/flatpak.yml around lines 38 - 39, The workflow step named
"Install flatpak-node-generator" installs aiohttp and aiofiles without pinning;
change that to install pinned versions (or use a constraints/requirements file)
to ensure reproducible builds. Update the pip3 install command that currently
lists "aiohttp" and "aiofiles" so each dependency has an explicit version
specifier (e.g., aiohttp==<version>, aiofiles==<version>) or replace the inline
install with "pip3 install -r requirements.txt" referencing a checked-in
requirements file with exact versions.
flatpak.yml:
- Add path filters to push trigger so CI only fires on Flatpak-relevant changes
- Pin flatpak-node-generator install to exact upstream commit SHA
(5de461271d05f43f267b954ac6010b410378ae47) for reproducible, supply-chain-safe builds
electron/main.cjs:
- Remove incorrect systemPreferences.askForMediaAccess('camera') call;
macOS screen recording cannot be requested programmatically — show a
dialog directing the user to System Preferences instead
flatpak/com.rein.app.yml:
- Add --socket=wayland and --talk-name=org.freedesktop.portal.ScreenCast
to finish-args so getDisplayMedia works via XDG Desktop Portal on Wayland
- Remove the destructive jq redirect that would corrupt package.json by
reading and writing the same file simultaneously; linux target is already
set in package.json
flatpak/com.rein.app.metainfo.xml:
- Add <description> child inside <release> element for AppStream discoverability
- Change '- flatpak/generated-sources.json' to '- generated-sources.json' so flatpak-builder resolves it relative to the manifest directory (flatpak/) instead of producing a double flatpak/flatpak/ path - Add 'subdir: rein' so build-commands run inside the repo subdirectory where package.json actually exists - Update electron-builder-arch-args.sh path from ../flatpak-node/ to ../../flatpak-node/ to account for the extra subdir level
With subdir: rein, build-commands run from /run/build/rein/rein/. flatpak-node/ is placed at the module root /run/build/rein/ by generated-sources.json, so ../flatpak-node/ (one level up) is correct. ../../flatpak-node/ was one level too deep.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.github/workflows/flatpak.yml (1)
9-15:⚠️ Potential issue | 🟡 MinorMirror the app-source paths here as well.
The Flatpak bundle also changes when
src/**orpublic/**changes, but those paths do not triggerpushbuilds right now. That makes branch-push coverage narrower thanpull_requestcoverage for the same packaged artifact.♻️ Proposed fix
paths: + - "src/**" + - "public/**" - "electron/**" - "flatpak/**" - "package.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/flatpak.yml around lines 9 - 15, Update the workflow's triggered paths so push builds mirror pull_request coverage by adding the missing source directories: include "src/**" and "public/**" in the paths list in the flatpak workflow (the same list currently containing "electron/**", "flatpak/**", "package.json", "package-lock.json", "vite.config.ts", and ".github/workflows/flatpak.yml") so changes to app source and public assets trigger push builds as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/main.cjs`:
- Around line 128-135: The macOS reopen bug is caused by killing serverProcess
unconditionally in the app.on('window-all-closed') handler and never clearing
mainWindow when the BrowserWindow is closed; update the window-all-closed
handler to only call serverProcess.kill() as part of the non-darwin shutdown
path (i.e., before or inside the branch where app.quit() is called) and remove
the unconditional kill for macOS, and ensure you reset mainWindow to null when
the window is destroyed (add a mainWindow.on('closed' / 'hide' callback to set
mainWindow = null inside createWindow or the window teardown code); then update
app.on('activate') to recreate the UI when mainWindow is null (use if
(!mainWindow) createWindow()) so activate can reopen the app on macOS.
- Around line 63-67: The BrowserWindow webPreferences explicitly set sandbox:
false which disables Chromium's renderer sandbox; update the webPreferences in
the BrowserWindow options (the object containing contextIsolation,
nodeIntegration, sandbox) to remove sandbox: false or set sandbox: true so the
renderer runs in the sandbox while keeping contextIsolation: true and
nodeIntegration: false; ensure any renderer-required APIs are exposed safely
(via the preload script) if switching to sandbox: true.
- Around line 73-85: The permission handlers currently auto-approve
'media','display-capture','screen' for the whole session; restrict approval to
the trusted origin and only the display-capture/getDisplayMedia flow by checking
webContents.getURL() (or webContentsFrame origin) and the specific permission
request path inside mainWindow.webContents.session.setPermissionRequestHandler
and setPermissionCheckHandler: verify the requesting origin matches your trusted
origin string and that the request corresponds to a
display-capture/getDisplayMedia flow (not generic 'media'), then call
callback(true) / return true only in that case and otherwise deny
(callback(false)/return false); update both setPermissionRequestHandler and
setPermissionCheckHandler to use these checks rather than the current
allowed.includes(permission) array.
In `@flatpak/com.rein.app.yml`:
- Around line 24-25: The flatpak permission flag
"--talk-name=org.freedesktop.portal.ScreenCast" is incorrect because
"org.freedesktop.portal.ScreenCast" is an interface, not the service; remove
this line from the manifest or replace it with the correct service name
"--talk-name=org.freedesktop.portal.Desktop" if you want to keep an explicit
permission, noting that Flatpak's session-bus filtering already allows access by
default; update the string in the manifest where
"--talk-name=org.freedesktop.portal.ScreenCast" appears (search for that exact
token) and either delete the entire flag entry or change it to
"--talk-name=org.freedesktop.portal.Desktop".
---
Duplicate comments:
In @.github/workflows/flatpak.yml:
- Around line 9-15: Update the workflow's triggered paths so push builds mirror
pull_request coverage by adding the missing source directories: include "src/**"
and "public/**" in the paths list in the flatpak workflow (the same list
currently containing "electron/**", "flatpak/**", "package.json",
"package-lock.json", "vite.config.ts", and ".github/workflows/flatpak.yml") so
changes to app source and public assets trigger push builds as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f223c23e-4d41-480a-9344-cca6ba408fde
📒 Files selected for processing (4)
.github/workflows/flatpak.ymlelectron/main.cjsflatpak/com.rein.app.metainfo.xmlflatpak/com.rein.app.yml
public/favicon.svg does not exist; the actual SVG app icon is at public/app_icon/Icon.svg
electron/main.cjs: - Remove sandbox:false from webPreferences (re-enables Chromium renderer sandbox) - Restrict permission handlers to http://localhost:3000 origin and display-capture/screen permissions only (not generic media from any origin) - Kill serverProcess only on non-macOS shutdown; macOS keeps server alive so the app can be reopened via the dock - Null mainWindow in 'closed' handler so createWindow() guard works correctly - Update activate handler to use if (!mainWindow) instead of window count flatpak/com.rein.app.yml: - Replace --talk-name=org.freedesktop.portal.ScreenCast (interface name) with --talk-name=org.freedesktop.portal.Desktop (correct D-Bus service name) .github/workflows/flatpak.yml: - Add src/** and public/** to push trigger paths to mirror pull_request coverage
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.
Summary
dirtarget in electron-builder config, Flatpak manifest + desktop/metainfo files, and a GitHub Actions Flatpak workflow.Why
Changes
electron/main.cjssetPermissionRequestHandlerandsetPermissionCheckHandlerwebPreferencesactivateflowpackage.jsondir) for electron-builderflatpak/com.rein.app.ymlflatpak/com.rein.app.desktopflatpak/com.rein.app.metainfo.xml.github/workflows/flatpak.ymlNotes
flatpak/generated-sources.jsonwhen running manually.Closes #254
Related to #206
Summary by CodeRabbit
New Features
Chores