E2E - User Activity Flow #3677
E2E - User Activity Flow #3677
Conversation
Jonathansoufer
requested review from
Doublemme,
HiiiiD,
hughlivingstone and
paolampilla
as code owners
📝 WalkthroughWalkthroughReworks E2E activity flows and testability: adds a minimal Maestro VET send subflow with explicit waits and history/tab navigation; propagates stable testIDs through activity list, activity box, tab bar, BaseCard, and staking CTA; adds app launch cleanup. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are primarily test IDs, minor UI wiring, and an E2E subflow that does not introduce a new multi-component runtime control flow requiring diagramming.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 (3)
src/Components/Reusable/SimpleActivitySectionList.tsx (1)
140-147: Consider using a global index for more stable test IDs.The
indexfromSectionListRenderItemInfois section-relative, meaning the same activity could receive different test IDs depending on which date section it falls into. For more stable E2E tests, consider maintaining a global index across all sections.Proposed enhancement using a global counter
+ let globalIndex = 0 const renderItem = useCallback( ({ item: activity, index }: SectionListRenderItemInfo<Activity, ActivitySection>) => { + const currentGlobalIndex = globalIndex++ return ( <ActivityItemRenderer activity={activity} onPress={onActivityPress} - testID={`ActivityListItem_${index}`} + testID={`ActivityListItem_${currentGlobalIndex}`} /> ) }, [onActivityPress], )Note: You may need to reset
globalIndexat the start of each render cycle or use a ref if this approach is adopted..maestro/userActivity/userActivity.yaml (1)
30-49: Simplify the conditional logic for tapping the first activity item.The current implementation has nested conditional flows that may be overly complex. The logic attempts to tap
ActivityListItem_0, but if it's not visible, switches to the Transfer tab. However, you've already tapped Transfer and Staking tabs in the previous step (lines 17-20), so the conditional switch back to Transfer (lines 42-43) might be unnecessary.Consider simplifying by ensuring you're on a tab with activity items before attempting to tap.
Proposed simplified approach
# 3) Tap on the first Staking item, open Explorer +- tapOn: + id: "ActivityTab_All" +- extendedWaitUntil: + visible: + id: "ActivityListItem_0" + timeout: 10000 +- tapOn: + id: "ActivityListItem_0" - runFlow: - when: - visible: - id: "ActivityListItem_0" - commands: - - tapOn: - id: "ActivityListItem_0" -- runFlow: - when: - notVisible: - id: "ActivityListItem_0" - commands: - - tapOn: - id: "ActivityTab_Transfer" - - extendedWaitUntil: - visible: - id: "ActivityListItem_0" - timeout: 10000 - - tapOn: - id: "ActivityListItem_0"This assumes the "All" tab will have at least one activity item for testing.
src/Screens/Flows/App/ActivityScreen/ActivityScreen.tsx (1)
82-97: Consider adding tabBarTestID to all tabs for consistency.Only three tabs (All, Transfer, Staking) have explicit
tabBarTestIDproperties. While theActivityTabBarcomponent has fallback logic to generate test IDs, explicitly defining them for all tabs would:
- Ensure consistent, predictable test IDs
- Make future E2E test expansion easier
- Improve maintainability
Proposed additions for remaining tabs
<Tab.Screen name={Routes.ACTIVITY_B3TR} component={ActivityB3trScreen} - options={{ title: LL.ACTIVITY_B3TR_LABEL() }} + options={{ title: LL.ACTIVITY_B3TR_LABEL(), tabBarTestID: "ActivityTab_B3TR" }} /> <Tab.Screen name={Routes.ACTIVITY_SWAP} component={ActivitySwapScreen} - options={{ title: LL.ACTIVITY_SWAP_LABEL() }} + options={{ title: LL.ACTIVITY_SWAP_LABEL(), tabBarTestID: "ActivityTab_Swap" }} /> <Tab.Screen name={Routes.ACTIVITY_NFT} component={ActivityNftScreen} - options={{ title: LL.ACTIVITY_NFT_LABEL() }} + options={{ title: LL.ACTIVITY_NFT_LABEL(), tabBarTestID: "ActivityTab_NFT" }} /> <Tab.Screen name={Routes.ACTIVITY_DAPPS} component={ActivityDappsScreen} - options={{ title: LL.ACTIVITY_DAPPS_LABEL() }} + options={{ title: LL.ACTIVITY_DAPPS_LABEL(), tabBarTestID: "ActivityTab_DApps" }} /> <Tab.Screen name={Routes.ACTIVITY_OTHER} component={ActivityOtherScreen} - options={{ title: LL.ACTIVITY_OTHER_LABEL() }} + options={{ title: LL.ACTIVITY_OTHER_LABEL(), tabBarTestID: "ActivityTab_Other" }} />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.maestro/userActivity/userActivity.yamlsrc/Components/Reusable/SimpleActivitySectionList.tsxsrc/Screens/Flows/App/ActivityScreen/ActivityScreen.tsxsrc/Screens/Flows/App/ActivityScreen/Components/ActivityBox.tsxsrc/Screens/Flows/App/ActivityScreen/Components/EmptyStakingActivities.tsxsrc/Screens/Flows/App/ActivityScreen/navigation/ActivityTabBar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/Components/Reusable/SimpleActivitySectionList.tsx (2)
src/Model/Activity/Activity.ts (1)
Activity(16-40)src/Screens/Flows/App/ActivityScreen/Components/ActivityItemRenderer.tsx (1)
ActivityItemRenderer(29-190)
⏰ 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 (3)
- GitHub Check: scan / unit (2)
- GitHub Check: scan / unit (1)
🔇 Additional comments (4)
src/Screens/Flows/App/ActivityScreen/Components/ActivityBox.tsx (1)
93-93: LGTM: Clean testID propagation pattern.The type extension to include
testIDinOverridableActivityBoxPropsis minimal and well-structured. All implementing components consistently provide sensible fallback test IDs using activity type and ID..maestro/userActivity/userActivity.yaml (1)
22-27: Clarify the purpose of the conditional tap on ActivityTab_All.The flow conditionally taps
ActivityTab_Allonly when it's visible (lines 22-27). Since this is a tab in a horizontal scrollable tab bar, it should always be present (though possibly off-screen).Can you clarify if this conditional is intended to handle scroll behavior, or if it should be an unconditional tap like the Transfer and Staking tabs above?
src/Screens/Flows/App/ActivityScreen/Components/EmptyStakingActivities.tsx (1)
44-44: LGTM: Appropriate test ID for empty state button.The test ID follows a clear naming convention and enables testing of the empty staking state flow.
src/Screens/Flows/App/ActivityScreen/navigation/ActivityTabBar.tsx (1)
37-40: No changes needed — the code is correct as-is.The nullish coalescing operator precedence is working correctly. The ternary operator already has parentheses (line 39), making the evaluation order clear: first attempt
tabBarTestID, then fallback to the formatted label if it's a string, then finally use the route key. Additional parentheses are not necessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.maestro/send/subflows/send_vet_minimal.yaml (2)
28-38: Consider increasing the fee delegation timeout.The 10-second timeout for
MAX_FEE_GALACTICAvisibility might be insufficient in slower test environments or under network latency, potentially causing flaky test failures.Suggested adjustment
- extendedWaitUntil: visible: id: "MAX_FEE_GALACTICA" - timeout: 10000 + timeout: 20000
47-49: Success verification is minimal.The flow only verifies that "Tokens" is visible after the transaction, which doesn't confirm the transaction actually succeeded or that the balance changed. If transaction processing fails silently, the test would still pass.
For more robust verification, consider adding assertions for:
- Transaction confirmation message or success indicator
- Updated balance reflecting the 1 VET transfer
However, given this is a "minimal" subflow for broader E2E scenarios, the current approach may be acceptable if transaction success is verified elsewhere.
.maestro/userActivity/userActivity.yaml (3)
33-34: Text-based selector is brittle.Tapping by text "Transfer" (line 34) will break if the label changes due to copy updates, localization, or UI refactoring. Consider using a testID selector instead.
Suggested alternative
Based on the AI summary mentioning testID propagation through activity components and tab bars, a more robust approach would be:
-# 5) Navigate to Transfer tab -- tapOn: "Transfer" +# 5) Navigate to Transfer tab +- tapOn: + id: "ACTIVITY_TRANSFER"This assumes the testID is available based on the PR's testability enhancements.
51-54: Text-based selector for activity item is fragile.Tapping by text "Sent tokens" (line 53) is susceptible to wording changes and may not uniquely identify the intended transaction if multiple "Sent tokens" items exist with different contexts.
Consider whether the testID enhancements mentioned in the AI summary provide a more stable selector for individual activity items. If
ActivityItemRenderernow passes testIDs (per the summary), leverage those for more reliable element identification.
64-66: Cleanup may not execute on test failure.The
clearState: truecleanup (line 66) only runs if all previous steps succeed. If the test fails earlier, the app state may not be reset, potentially affecting subsequent test runs.Consider whether Maestro supports teardown hooks or
onFlowCompletehandlers that run regardless of test outcome. If not, document that failed tests may require manual cleanup or setup resets between runs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.maestro/send/subflows/send_vet_minimal.yaml.maestro/userActivity/userActivity.yamlsrc/Components/Base/BaseCard/BaseCard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/Components/Base/BaseCard/BaseCard.tsx (1)
src/Components/Base/BaseView.tsx (1)
BaseView(45-127)
⏰ 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 (3)
- GitHub Check: scan / unit (2)
- GitHub Check: scan / unit (1)
🔇 Additional comments (6)
.maestro/send/subflows/send_vet_minimal.yaml (1)
19-26: Verify Account 1 availability assumption.The flow assumes Account 1 exists at index 0 without validation. If the account is missing or at a different index, the test will fail without clear diagnostics.
Consider verifying that the test setup (likely in
setupApp.yaml) guarantees Account 1 is present and positioned at index 0 before this subflow runs..maestro/userActivity/userActivity.yaml (3)
10-12: Addresses previous review feedback.This change directly addresses @HiiiiD's comment about performing a send to guarantee the indexer works properly. The integration of the VET send subflow ensures the test validates actual indexer behavior rather than relying on existing data.
42-49: 60-second timeout suggests potential indexer reliability issues.The 60-second wait for the "Today" section to appear (line 45) indicates potential performance or synchronization concerns with the indexer. While this timeout may be necessary in practice, it significantly increases test execution time and suggests the indexer may not be consistently performant.
Consider investigating whether:
- The indexer consistently requires this much time, or if it's occasionally slow (indicating flakiness).
- There's an opportunity to optimize indexer performance to reduce this wait time.
- A shorter timeout with retry logic might be more appropriate.
You can monitor test execution times to determine if this timeout is frequently hit or just a safety margin.
36-40: Swipe gesture reliability may vary.The pull-to-refresh swipe (lines 37-40) can be unreliable depending on scroll position, screen dimensions, and platform-specific gesture recognition. If the list is empty or the swipe doesn't trigger the refresh handler, subsequent assertions will fail.
Verify that:
- The swipe coordinates (50%, 30% → 50%, 80%) reliably trigger refresh across different device sizes and platforms.
- The Activity screen is in a scrollable state when this gesture executes.
If flakiness is observed, consider alternatives such as:
- Using an explicit refresh button with a testID
- Adding a wait condition to verify the refresh was triggered (e.g., checking for a loading indicator)
src/Components/Base/BaseCard/BaseCard.tsx (2)
29-30: LGTM! Correctly removed unused testID.The removal of
testIDfrom the innerBaseViewand its corresponding removal from theuseMemodependency array is correct, astestIDis now conditionally applied to either the outer container or theTouchableOpacity.
35-35: Improved testID placement for interactive elements.The conditional testID logic correctly places the identifier on the
TouchableOpacitywhen the card is pressable, and on the outerBaseViewwhen it's not. This follows E2E testing best practices by ensuring the testID is on the actual interactive element.Existing unit tests remain compatible—the test file covers the non-pressable case where testID stays on the outer view.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Components/Reusable/SimpleActivitySectionList.tsxsrc/Screens/Flows/App/ActivityScreen/Components/ActivitySectionList.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/Components/Reusable/SimpleActivitySectionList.tsx (2)
src/Model/Activity/Activity.ts (1)
Activity(16-40)src/Screens/Flows/App/ActivityScreen/Components/ActivityItemRenderer.tsx (1)
ActivityItemRenderer(29-190)
⏰ 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 (4)
- GitHub Check: scan / unit (2)
🔇 Additional comments (2)
src/Screens/Flows/App/ActivityScreen/Components/ActivitySectionList.spec.tsx (1)
300-305: Test refactoring improves maintainability.The shift from type-specific testID checks to index-based verification simplifies the test and aligns with the new stable testID generation strategy. The regex pattern and loop-based verification ensure all activity items render with the expected identifiers.
src/Components/Reusable/SimpleActivitySectionList.tsx (1)
98-104: LGTM! Index mapping preserves stable test identifiers.The Map-based approach correctly maintains the original array index for each activity, ensuring stable testIDs even when activities are grouped into sections. The memoization strategy is appropriate.
|
Related Issue
Closes # https://github.com/vechain/veworld-private/issues/623
Description of Changes
Reviewer(s)
@Doublemme @hughlivingstone @HiiiiD
UI/UX Changes
Checklist
Thank you for contributing! 🎉
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.