chore: add mobile platform driver#36
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution MetaMask internal reviewing guidelines:
|
| * @param targetValue - Target value for element lookup. | ||
| * @param refMap - Map of a11y refs to stable identifiers. | ||
| * @param timeoutMs - Maximum wait time in milliseconds. | ||
| * @param _within - Unused on mobile — elements are always searched globally. |
There was a problem hiding this comment.
Should we adopt a more defensive posture and throw an error if _within is passed? We can then also not mention it in the mobile skill file (that we will add to the skills repo). But returning an error would the most explicit way to tell the agent that within is not supported (if for any reason it mya think that it is). We would normally do this in the zod schema validations, but because we are not adding a new schema per platform driver, we could at least throw in the platform driver.
There was a problem hiding this comment.
good catch, will fix
| case 'selector': | ||
| return { identifier: targetValue }; |
There was a problem hiding this comment.
CSS selectors are not supported in mobile. We should throw an error and be explicit about this. Otherwise we may have agents trying to perform actions via CSS selector patterns and all that they will get is a timeout error.
| await this.#backend.waitForElement(query, timeoutMs); | ||
| await this.#backend.tapElement(query); |
There was a problem hiding this comment.
the timeoutMs does not seem correct. It is only being applied in the waitForElement and not on the tapElement. The timeoutMs should be the timeout applied to the entire click action. On the playwright driver we do some computation to apply different timeouts to the different driver actions, so that the overall timeoutMs is respected.
For example here we should:
- apply the timeoutMs to the waitForElement
- then once the waitForElement resolves compute the remaining time budget that we still have (for example if timeout is 15s and the waitForElement resolved in 5s, then we still have 10s left)
- apply the remaining time budget as a timeout to the tapElement. If the tapElement does not accept a timeout argument, we can implement an high level Promise.race
| */ | ||
| async getAppState(): Promise<ExtensionState> { | ||
| const appState = await this.#backend.getAppState(this.#bundleId); | ||
| const isRunning = appState.state === 'Running' || appState.state === '4'; |
There was a problem hiding this comment.
What does appState.state === 4 means?
There was a problem hiding this comment.
4 is the Appium numeric code for "running in foreground". IDB returns 'Running' as a string
| isLoaded: isRunning, | ||
| currentUrl: '', | ||
| extensionId: this.#bundleId, | ||
| isUnlocked: isRunning, |
There was a problem hiding this comment.
Does this means that it is actually running or that it is unlocked? (We may want to eventually refactor this app state completely.. Some fields like the chainId and the isUnlocked may not be useful at all and are introducing more harm than good)
There was a problem hiding this comment.
We can't detect wallet lock state from mobile app state. isUnlocked: isRunning is an approximation, I added this for now. It might be a bit inaccurate for now but I will fix it in the BasePlatformState refactor later, this will need to be done because right now the appState is extension specific and we return those values as null and the file needs a bigger refactor to accommodate both. I can add it here as well let me know what you think
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
No description provided.