E2E Refactor on Token Management Flow #3680
Conversation
Jonathansoufer
requested review from
Doublemme,
HiiiiD,
hughlivingstone and
paolampilla
as code owners
📝 WalkthroughWalkthroughReplaces the Android-only token management flow with a consolidated Android-focused sequence (iOS explicitly skipped), adds onFlowStart/onFlowComplete hooks, refactors token add/search/delete steps to use unified actions and synchronized assertions, and standardizes formatting in resetApp.yaml while removing one final visibility assertion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![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)
.maestro/tokenManagement/tokenManagement.yaml (1)
67-76: Consider asserting a token-specific element for the search result.The current assertion
assertVisible: "GOLD"may match the text in the search input field itself rather than confirming a search result was returned. If the search result row has a distinct test ID (e.g.,TokenRow_GOLD), asserting on that would make the test more robust.That said, for a smoke test verifying basic search functionality, this is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.maestro/tokenManagement/tokenManagement.yaml
🧰 Additional context used
🪛 Gitleaks (8.30.0)
.maestro/tokenManagement/tokenManagement.yaml
[high] 8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: scan / unit (1)
- GitHub Check: scan / unit (3)
- GitHub Check: scan / unit (2)
- GitHub Check: scan / unit (4)
🔇 Additional comments (5)
.maestro/tokenManagement/tokenManagement.yaml (5)
5-8: Static analysis false positive - this is a token contract address, not an API key.The Gitleaks warning can be safely ignored here. The value
0x8a9844e4750f5ce5f7988c4d1e04c278c718feeais a VeChain/Ethereum smart contract address (public on-chain data), not a secret API key. Contract addresses are inherently public and required for token interactions.The tag change from
onlyAndroidtopull-requestis noted — Test 3 (custom token addition) is still gated to Android viaplatform: Android, so iOS runs will skip that portion.
10-17: LGTM!Clean state initialization and proper conditional handling for the iOS dictation prompt.
19-57: Good defensive handling of dynamic UI states.The conditional flows properly handle version update prompts, changelogs, notification permissions, and close buttons that may or may not appear. The
extendedWaitUntilwith a 15-second timeout provides adequate synchronization before proceeding to test assertions.
78-114: Well-structured Android-specific test with proper synchronization.Good implementation choices:
- Explicit platform gating with explanatory comment for why iOS is excluded
- Defensive handling of the Android autofill dialog (lines 95-101)
- Adequate timeouts for async token lookup and addition
- Verification that the CARP token appears in the list post-addition
116-123: LGTM!Good practice to verify the final state after all test operations, ensuring the user lands back on the expected wallet screen.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.maestro/tokenManagement/tokenManagement.yaml (3)
7-7: Minor typo in environment variable name.
CARP_TOKEN_ADRESSshould beCARP_TOKEN_ADDRESS(double 'D' in ADDRESS). However, since it's consistently used throughout the file (defined at line 7, used at line 138), this is a low-priority naming issue.🔎 Proposed fix
- CARP_TOKEN_ADRESS: "0x8a9844e4750f5ce5f7988c4d1e04c278c718feea" + CARP_TOKEN_ADDRESS: "0x8a9844e4750f5ce5f7988c4d1e04c278c718feea"Also update line 138:
- - inputText: ${CARP_TOKEN_ADRESS} + - inputText: ${CARP_TOKEN_ADDRESS}
24-274: Consider extracting repeated patterns into reusable flows.The Android test contains significant code duplication:
- Opening manage tokens:
tapOn EditTokensBar_BaseIcon_manageToken+ swipe pattern appears ~8 times (lines 25, 49, 87, 111, 126, 164, 223, 248)- Navigation back:
assertVisible BackButton+tapOn BackButtonpattern repeated ~10 times- Swipe from sendButton UP: Identical swipe pattern appears multiple times (lines 38-40, 68-71, 100-103, etc.)
- Network switching: Lines 205-274 repeat the same settings → Networks → switch pattern 3+ times
Maestro supports parameterized flows and
runFlowcomposition. Extracting common patterns would improve maintainability and reduce the 250+ line test to a more manageable size.Example refactor approach:
🔎 Example: Extract "openManageTokens" helper flow
Create
.maestro/tokenManagement/helpers/openManageTokens.yaml:appId: org.vechain.veworld.app --- - tapOn: id: "EditTokensBar_BaseIcon_manageToken"Then replace repeated blocks with:
- runFlow: helpers/openManageTokens.yaml
205-274: Network switching scenario is highly repetitive.The network switching test (lines 205-274) repeats the same pattern 3+ times:
- Go to settings → Networks
- Switch networks
- Go back to wallet
- Open manage tokens
- Select a token
- Go back
This 70-line section could be condensed significantly by extracting a parameterized "switchNetworkAndSelectToken" helper flow.
Consider creating a helper flow that accepts parameters for network names and token IDs, reducing this section from ~70 lines to ~10-15 lines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.maestro/setup_reset/resetApp.yaml.maestro/tokenManagement/tokenManagement.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: scan / unit (4)
- GitHub Check: scan / unit (1)
- GitHub Check: scan / unit (3)
- GitHub Check: scan / unit (2)
🔇 Additional comments (7)
.maestro/setup_reset/resetApp.yaml (2)
3-30: LGTM! Formatting improvements enhance consistency.The indentation adjustments properly align nested
idfields, improving readability without changing behavior.
30-30: The assertion removal concern is not supported. No other Maestro flows reference or depend onresetApp.yaml, so removing the final assertion does not impact any downstream flows or cause flaky test failures. TheresetAppreferences found in the codebase are TypeScript Redux actions, not Maestro flow invocations. This appears to be a standalone test flow with no dependencies.Likely an incorrect or invalid review comment.
.maestro/tokenManagement/tokenManagement.yaml (5)
13-23: Good platform-aware design with explicit iOS skip.The explicit platform handling is clear: iOS tests are skipped with a logged message (addressing known instability), while Android tests proceed. This approach is transparent and maintainable.
83-106: Good use of synchronized assertions for stability.The
extendedWaitUntilwith timeout (lines 94-97) andassertNotVisible(lines 104-105) provide robust verification that the token unselection completed before proceeding.
107-117: LGTM! Clean search functionality test.The search test is straightforward and correctly verifies that searching for "PLAIR" displays the "PLA" token.
118-159: Well-structured custom token addition with defensive handling.The conditional handling of the Android dictation close button (lines 131-137) and multiple
extendedWaitUntilassertions demonstrate good defensive coding for UI stability. The flow correctly verifies the custom token (CARP) appears after addition.
160-204: Token deletion flow looks correct.The flow correctly adds multiple custom tokens using direct addresses, then deletes one via swipe gesture and verifies removal. The assertions ensure the expected UI state.
| - repeat: | ||
| while: | ||
| notVisible: | ||
| id: "BackButtonHeader-BaseIcon-backButton" | ||
| commands: | ||
| - tapOn: | ||
| id: "BackButtonHeader-BaseIcon-backButton" | ||
| - tapOn: | ||
| id: "BackButtonHeader-BaseIcon-backButton" |
There was a problem hiding this comment.
Logic error in repeat loop condition.
The loop at lines 59-65 waits while the back button is not visible (notVisible), but then taps the back button inside the loop. This is illogical—it will try to tap a button that isn't visible, likely causing the test to fail or loop indefinitely.
Expected behavior: Loop while the back button is visible and tap it to dismiss multiple screens.
🔎 Proposed fix
- repeat:
while:
- notVisible:
+ visible:
id: "BackButtonHeader-BaseIcon-backButton"
commands:
- tapOn:
id: "BackButtonHeader-BaseIcon-backButton"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - repeat: | |
| while: | |
| notVisible: | |
| id: "BackButtonHeader-BaseIcon-backButton" | |
| commands: | |
| - tapOn: | |
| id: "BackButtonHeader-BaseIcon-backButton" | |
| - tapOn: | |
| id: "BackButtonHeader-BaseIcon-backButton" | |
| - repeat: | |
| while: | |
| visible: | |
| id: "BackButtonHeader-BaseIcon-backButton" | |
| commands: | |
| - tapOn: | |
| id: "BackButtonHeader-BaseIcon-backButton" | |
| - tapOn: | |
| id: "BackButtonHeader-BaseIcon-backButton" |
🤖 Prompt for AI Agents
In .maestro/tokenManagement/tokenManagement.yaml around lines 59 to 67, the
repeat loop currently waits "while: notVisible" but then attempts to tap the
back button inside the loop, which is wrong; change the loop condition to wait
"while: visible" (so it keeps tapping while the back button is present) and keep
the tapOn inside the loop; also remove the redundant tapOn that follows the
repeat block (or make it conditional) to avoid tapping twice.
| - runFlow: | ||
| when: | ||
| platform: Android | ||
| notVisible: "Selected" # Needed to avoid double click and deselect the token | ||
| platform: iOS | ||
| commands: | ||
| - tapOn: | ||
| text: "PLA" | ||
| - evalScript: ${console.log('Skipping tokenManagement test on iOS')} |
There was a problem hiding this comment.
Should we simply use the onlyAndroid tag?
| - tapOn: | ||
| id: "NetworkBox_testnet" | ||
| - tapOn: | ||
| id: "NetworkBox_mainnet" |
There was a problem hiding this comment.
Why are you tapping twice on the networks?
| - tapOn: | ||
| id: "NetworkBox_mainnet" | ||
| - tapOn: | ||
| id: "NetworkBox_testnet" |
| - tapOn: | ||
| id: "NetworkBox_testnet" | ||
| - tapOn: | ||
| id: "NetworkBox_mainnet" |
hughlivingstone
temporarily deployed
to
android-release
GitHub Actions
Inactive
hughlivingstone
had a problem deploying
to
ios-release
GitHub Actions
Failure
|
hughlivingstone
had a problem deploying
to
ios-release
GitHub Actions
Failure
|
@codex review |
Related Issue
This PR does:
Closes # https://github.com/vechain/veworld-private/issues/622
Description of Changes
Reviewer(s)
@Doublemme @hughlivingstone @HiiiiD
UI/UX Changes
Checklist
Thank you for contributing! 🎉
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.