fix(suite-desktop-core): refactor desktop state patches to avoid breaking change #17602
Conversation
| ]; | ||
|
|
||
| describe('process state patch', () => { | ||
| FIXTURES.map(([name, args, value]) => { | ||
| it(name, () => { | ||
| process.argv = args.map(arg => `--${arg}`); | ||
| process.argv = args; |
There was a problem hiding this comment.
I think mapping the -- just obfuscated the test flow, so I simplified it even further..
| .reduce<ProcessStatePatchResult>( | ||
| (prev, cur) => mergeDeepObject.withOptions({ dotNotation: true }, prev ?? {}, cur), | ||
| undefined, | ||
| ); | ||
| )?.state; |
There was a problem hiding this comment.
with this implementation, everything is under { state: {...patches}} until this line, where I extract it or dump the undefined..
WalkthroughThe changes update how state-related string assignments are formatted and processed across the application. The Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)🔇 Additional comments (3)
✨ 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 (
|
|
|
||
| return { [key]: tryParseJson(value) }; | ||
| }; | ||
|
|
||
| const STATE_ASSIGNMENT_REGEX = /^--state[^=]*=[^=]+$/; |
There was a problem hiding this comment.
The only tiny thing which came to my mind is that this regex disallow to pass a json value containing string with equal sign, which in theory may be wanted.
There was a problem hiding this comment.
True! I'll fix that.
There was a problem hiding this comment.
Done. Now I only split by the first equality sign, so I changed .test+.split back to .match with two capturing groups
6b17f57 to
aa33352
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-core/src/libs/app-utils.ts (1)
46-52: The implementation properly handles command-line state patches.The refactored code effectively extracts and processes state patches from command-line arguments using regex matching instead of a separate parsing function. This approach is more direct and maintainable.
However, consider replacing the sequential map-filter-map operations with a single reduce for better performance:
- process.argv - .map(arg => arg.match(STATE_ASSIGNMENT_REGEX)) - .filter(match => match !== null) - .map((assignment: RegExpMatchArray) => { - const [_, key, value] = assignment; - - return { [key]: tryParseJson(value) }; - }) + process.argv.reduce<Record<string, any>[]>((acc, arg) => { + const match = arg.match(STATE_ASSIGNMENT_REGEX); + if (match !== null) { + const [_, key, value] = match; + acc.push({ [key]: tryParseJson(value) }); + } + return acc; + }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite-desktop-core/e2e/support/electron.ts(1 hunks)packages/suite-desktop-core/src/__tests__/app-utils.test.ts(1 hunks)packages/suite-desktop-core/src/libs/app-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/suite-desktop-core/e2e/support/electron.ts
- packages/suite-desktop-core/src/tests/app-utils.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-e2e-suite-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
packages/suite-desktop-core/src/libs/app-utils.ts (2)
56-56: The null-safe extraction of the state property is correct.This line correctly extracts the
stateproperty from the result or returns undefined if the reduction produces no results.
38-38:✅ Verification successful
Review the regex pattern to ensure it handles all valid JSON values.
The regex pattern captures state assignments well, but it may have limitations when parsing JSON strings containing equal signs within the value part. Consider if this is an intended limitation or if the regex should be refined to handle more complex JSON values.
🏁 Script executed:
#!/bin/bash # Check if there are any use cases in the codebase that might pass JSON with equal signs echo "Searching for potential JSON values with equal signs passed to state patches..." rg -A 3 "state.*=" --no-filename | grep -E "(\".*=.*\"|\{.*=.*\})" || echo "No instances found"Length of output: 2549
State Assignment Regex Verified
After investigating potential cases where a JSON value containing equal signs might be passed, there is no evidence in the codebase that the current regex causes issues. The pattern
const STATE_ASSIGNMENT_REGEX = /^--(state[^=]*)=(.+)$/;correctly captures state assignments—even if the value includes equal signs. If more complex JSON values ever become a requirement, consider revisiting the parsing strategy.
|
/rebase |
aa33352 to
683bd11
Compare
Description
Partial revert of 5a84a1f (#17526)
processStatePatch+ tests were also refactored for better clarity, so this PR keeps that...Notes
👉 I will still did file a bug with Electron electron/electron#45991, because the function started throwing without mention in changelog. But they said the function is not supposed to be used for custom application parameters, it's an API to set Chromium flags. We are supposed to use
process.argvfor custom params, as it currently is 🙂