feat(suite-desktop): open system settings using desktopApi #17017
Conversation
WalkthroughThis pull request introduces new methods to the desktop API and integrates a system settings module for managing Bluetooth settings across various operating systems. Two methods are added to the API: ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/suite-desktop-core/src/modules/system-settings.ts (1)
47-55: Enhance type safety and extensibility.The IPC handler could benefit from better type safety and extensibility for future settings.
Consider this enhancement:
+type SupportedSettings = 'bluetooth' | 'camera' | 'privacy'; + export const init: ModuleInit = () => { - ipcMain.handle('system/open-settings', (_, settings) => { + ipcMain.handle('system/open-settings', (_, settings: SupportedSettings) => { if (settings === 'bluetooth') { return openBluetoothSettings(); + } else if (settings === 'camera') { + // TODO: Implement camera settings + return { success: false, error: 'Camera settings not implemented yet' }; + } else if (settings === 'privacy') { + // TODO: Implement privacy settings + return { success: false, error: 'Privacy settings not implemented yet' }; } return { success: false, error: `Unknown settings: ${settings}` }; }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/suite-desktop-api/src/api.ts(2 hunks)packages/suite-desktop-api/src/factory.ts(1 hunks)packages/suite-desktop-core/src/modules/index.ts(2 hunks)packages/suite-desktop-core/src/modules/system-settings.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (2)
packages/suite-desktop-api/src/api.ts (1)
112-112: LGTM!The API interface changes are well-defined and consistent with the implementation.
Also applies to: 179-179
packages/suite-desktop-core/src/modules/index.ts (1)
33-33: LGTM!The module integration follows the established pattern and is correctly placed in the MODULES array.
Also applies to: 67-67
szymonlesisz
force-pushed
the
feat/suite-desktop-system-settings
branch
from
1077075 to
a65ecc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/suite-desktop-core/src/modules/system-settings.ts (3)
1-1:⚠️ Potential issueUse
execFileinstead ofexecfor better security.Using
execcan lead to command injection vulnerabilities. Consider usingexecFilewhich is safer for executing commands with arguments.-import { exec } from 'child_process'; +import { execFile } from 'child_process';
12-21:⚠️ Potential issueImprove command validation and error handling.
The function needs better input validation and error handling:
- Command string is passed directly to exec without validation
- Error details might contain sensitive information
-const openSettings = (cmd: string, env?: Record<string, string>) => +const ALLOWED_COMMANDS = ['gnome-control-center', 'systemsettings5', 'open', 'start'] as const; +type AllowedCommand = typeof ALLOWED_COMMANDS[number]; + +const openSettings = (cmd: AllowedCommand, args: string[], env?: Record<string, string>) => new Promise<InvokeResult>(resolve => { - exec(cmd, { env: { ...process.env, ...env } }, error => { + execFile(cmd, args, { env: { ...process.env, ...env } }, error => { if (error) { - resolve({ success: false, error: error.toString() }); + resolve({ success: false, error: 'Failed to open system settings' }); } else { resolve({ success: true }); } }); });
23-46: 🛠️ Refactor suggestionImprove error handling for desktop environments.
The function needs better error handling:
- No error for missing desktop environment variable
- No explicit error for unsupported desktop environments
- Commands should be split into command and arguments
const openBluetoothSettings = () => { if (isLinux()) { const xdg = process.env.ORIGINAL_XDG_CURRENT_DESKTOP || process.env.XDG_CURRENT_DESKTOP; + if (!xdg) { + return { success: false, error: 'Desktop environment not detected' }; + } if (xdg?.includes('GNOME')) { - return openSettings('gnome-control-center bluetooth', { + return openSettings('gnome-control-center', ['bluetooth'], { XDG_CURRENT_DESKTOP: xdg, }); } else if (xdg?.includes('KDE')) { - return openSettings('systemsettings5', { + return openSettings('systemsettings5', [], { XDG_CURRENT_DESKTOP: xdg, }); + } else { + return { success: false, error: `Unsupported desktop environment: ${xdg}` }; } } if (isMacOs()) { - return openSettings('open "x-apple.systempreferences:com.apple.Bluetooth"'); + return openSettings('open', ['x-apple.systempreferences:com.apple.Bluetooth']); } if (isWindows()) { - return openSettings('start ms-settings:bluetooth'); + return openSettings('start', ['ms-settings:bluetooth']); } return { success: false, error: 'Unsupported os' }; };
🧹 Nitpick comments (1)
packages/suite-desktop-core/src/modules/system-settings.ts (1)
48-56: Add type safety for settings parameter.The settings parameter should be typed to ensure only valid settings are passed.
+type SystemSettings = 'bluetooth'; + export const init: ModuleInit = () => { - ipcMain.handle('system/open-settings', (_, settings) => { + ipcMain.handle('system/open-settings', (_, settings: SystemSettings) => { if (settings === 'bluetooth') { return openBluetoothSettings(); } return { success: false, error: `Unknown settings: ${settings}` }; }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/suite-desktop-api/src/api.ts(2 hunks)packages/suite-desktop-api/src/factory.ts(1 hunks)packages/suite-desktop-core/src/modules/index.ts(2 hunks)packages/suite-desktop-core/src/modules/system-settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/suite-desktop-core/src/modules/index.ts
- packages/suite-desktop-api/src/api.ts
- packages/suite-desktop-api/src/factory.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
szymonlesisz
force-pushed
the
feat/suite-desktop-system-settings
branch
from
a65ecc9 to
096a7ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/suite-desktop-core/src/modules/system-settings.ts (3)
1-1:⚠️ Potential issueUse
execFileinstead ofexecfor better security.Using
execcan lead to command injection vulnerabilities. Consider usingexecFilewhich is safer for executing commands with arguments.-import { exec } from 'child_process'; +import { execFile } from 'child_process';
12-21:⚠️ Potential issueEnhance security and error handling in
openSettings.The function needs several improvements:
- Input validation for commands
- Proper argument separation
- Better error handling with specific error types
-const openSettings = (cmd: string, env?: Record<string, string>) => +const ALLOWED_COMMANDS = ['gnome-control-center', 'systemsettings5', 'open', 'start'] as const; +type AllowedCommand = typeof ALLOWED_COMMANDS[number]; + +const openSettings = (cmd: AllowedCommand, args: string[], env?: Record<string, string>) => new Promise<InvokeResult>(resolve => { - exec(cmd, { env: { ...process.env, ...env } }, error => { + execFile(cmd, args, { env: { ...process.env, ...env } }, error => { if (error) { - resolve({ success: false, error: error.toString() }); + resolve({ + success: false, + error: `Failed to open settings: ${error.message}` + }); } else { resolve({ success: true }); } }); });
23-46: 🛠️ Refactor suggestionImprove error handling and command execution in
openBluetoothSettings.The function needs several improvements:
- Better error handling for unsupported desktop environments
- Proper command and argument separation
- Validation of environment variables
const openBluetoothSettings = () => { if (isLinux()) { const xdg = process.env.ORIGINAL_XDG_CURRENT_DESKTOP || process.env.XDG_CURRENT_DESKTOP; + if (!xdg) { + return { success: false, error: 'Desktop environment not detected' }; + } if (xdg?.includes('GNOME')) { - return openSettings('gnome-control-center bluetooth', { + return openSettings('gnome-control-center', ['bluetooth'], { XDG_CURRENT_DESKTOP: xdg, }); } else if (xdg?.includes('KDE')) { - return openSettings('systemsettings5', { + return openSettings('systemsettings5', [], { XDG_CURRENT_DESKTOP: xdg, }); + } else { + return { success: false, error: `Unsupported desktop environment: ${xdg}` }; } } if (isMacOs()) { - return openSettings('open "x-apple.systempreferences:com.apple.Bluetooth"'); + return openSettings('open', ['x-apple.systempreferences:com.apple.Bluetooth']); } if (isWindows()) { - return openSettings('start ms-settings:bluetooth'); + return openSettings('start', ['ms-settings:bluetooth']); } return { success: false, error: 'Unsupported os' }; };
🧹 Nitpick comments (1)
packages/suite-desktop-core/src/modules/system-settings.ts (1)
48-56: Add type safety to settings parameter.Consider adding a type for the settings parameter to ensure type safety at compile time.
+type SystemSettings = 'bluetooth'; + export const init: ModuleInit = () => { - ipcMain.handle('system/open-settings', (_, settings) => { + ipcMain.handle('system/open-settings', (_, settings: SystemSettings) => { if (settings === 'bluetooth') { return openBluetoothSettings(); } return { success: false, error: `Unknown settings: ${settings}` }; }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/suite-desktop-api/src/api.ts(2 hunks)packages/suite-desktop-api/src/factory.ts(1 hunks)packages/suite-desktop-core/src/modules/index.ts(2 hunks)packages/suite-desktop-core/src/modules/system-settings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/suite-desktop-core/src/modules/index.ts
- packages/suite-desktop-api/src/factory.ts
- packages/suite-desktop-api/src/api.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
|
NOTE: this PR conflicts with bluetooth-ipc PR |
Description
Another cherry pick from the
bluetoothbranch.Open system settings using desktopApi.
Originally i did it only for the bluetooth module but then i realized it could be also used to open other settings, like for example camera settings if it was once declined (useful especially on macos)
tested on linux ubuntu gnome, debian gnome, macos and windows, i didnt tried KDE or any other