chore(suite-desktop): Unify suite desktop flags #17687
Conversation
| * Check if a switch is present in process arguments. | ||
| */ | ||
| export const hasSwitch = (switchName: SuiteSwitch) => { | ||
| const isSwitch = new RegExp(`^--${switchName}(?:=[^=]+)?=?$`); |
There was a problem hiding this comment.
regex carefully crafted to match --flag without value, but also --flag=with_value or empty value --flag=, but =?.+ would not suffice, because it would accidentally match substring.
In other words, value at end can only come after = (optional).
This is simpler in getSwitchValue, where the regex always requires =
WalkthroughThe changes refactor how command-line switches are handled throughout the application. A dedicated library for processing switches has been introduced with two new functions, Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
Lemonexe
force-pushed
the
unify-suite-desktop-flags
branch
from
3f99920 to
ff8b74f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-core/src/libs/process-switches.ts (1)
34-44: Effective implementation of getSwitchValue functionThe
getSwitchValuefunction accurately mimics Electron'sapp.commandLine.getSwitchValuebehavior, which improves maintainability by eliminating direct dependencies on Electron's API.Consider a minor optimization to improve readability:
-const valueMatch = process.argv.map(arg => arg.match(switchValueMatch)).find(Boolean); +const valueMatch = process.argv.find(arg => switchValueMatch.test(arg))?.match(switchValueMatch);This approach could be slightly more efficient by avoiding unnecessary regex matches after finding the first match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/suite-desktop-core/src/__tests__/process-switches.test.ts(1 hunks)packages/suite-desktop-core/src/app.ts(3 hunks)packages/suite-desktop-core/src/libs/app-utils.ts(1 hunks)packages/suite-desktop-core/src/libs/logger.ts(3 hunks)packages/suite-desktop-core/src/libs/process-switches.ts(1 hunks)packages/suite-desktop-core/src/modules/auto-updater.ts(2 hunks)packages/suite-desktop-core/src/modules/bridge.ts(1 hunks)packages/suite-desktop-core/src/modules/dev-tools.ts(1 hunks)packages/suite-desktop-core/src/modules/event-logging/contents.ts(1 hunks)packages/suite-desktop-core/src/modules/http-receiver.ts(2 hunks)packages/suite-desktop-core/src/modules/tor.ts(2 hunks)packages/suite-desktop-core/src/modules/trezor-connect.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/suite-desktop-core/src/modules/http-receiver.ts
- packages/suite-desktop-core/src/app.ts
- packages/suite-desktop-core/src/libs/app-utils.ts
- packages/suite-desktop-core/src/modules/dev-tools.ts
- packages/suite-desktop-core/src/modules/event-logging/contents.ts
- packages/suite-desktop-core/src/modules/tor.ts
- packages/suite-desktop-core/src/tests/process-switches.test.ts
- packages/suite-desktop-core/src/modules/auto-updater.ts
- packages/suite-desktop-core/src/modules/bridge.ts
- packages/suite-desktop-core/src/libs/logger.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (4)
packages/suite-desktop-core/src/modules/trezor-connect.ts (2)
1-7: Improved dependency management by removing direct Electron app importsThe change removes the direct dependency on Electron's
appobject and introduces the dedicatedgetSwitchValueutility function. This aligns with the PR objective to unify suite desktop flags handling.
65-65: Consistent usage of command-line switch handlingGood refactoring of the debug flag access from
app.commandLine.getSwitchValue('log-connect')to the newgetSwitchValue('log-connect'). This provides a unified approach to handling command-line arguments and eliminates direct dependencies on Electron's API.packages/suite-desktop-core/src/libs/process-switches.ts (2)
1-23: Well-defined TypeScript type for command-line switchesThe
SuiteSwitchtype alias provides a comprehensive list of all supported command-line switches, ensuring type safety throughout the application. This is a good practice for maintaining a centralized definition of supported options.
25-32: Robust implementation of hasSwitch functionThe
hasSwitchfunction implementation is well-designed with a carefully crafted regex pattern that correctly handles switches both with and without values.The regex handles
--flagwithout value,--flag=with_value, and empty value--flag=cases as mentioned in the past review comments.
Description
hasSwitch&getSwitchValueincl. unit testsapp.commandLinemethods, which are not to be used for this (see issue)app.commandLinemethods, or directprocess.argvaccessNote: only width & height remain, where the electron API is OK, because those are Chromium switches ✔️
Related Issue
Resolve #17686