feat(carousel): enhance analytics tracking for carousel interactions#30458
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit edbf0aa. Configure here.
|
|
||
| ///: BEGIN:ONLY_INCLUDE_IF(solana) | ||
| const isSolanaBanner = slideId === 'solana'; | ||
| const isSolanaBanner = slideName === 'solana'; |
There was a problem hiding this comment.
Analytics name used for Solana business logic check
Medium Severity
The isSolanaBanner check uses slideName from getSlideAnalyticsName(slide), which returns variableName || id. This means a slide with a falsy variableName but id === 'solana' would trigger the Solana redirect via Engine.setSelectedAddress, even though applyLocalNavigation (which checks getSlideVariableName, i.e. only variableName) would not have configured Solana navigation for it. Every other slide-type check in the file ('fund', 'solana', 'empty') consistently uses getSlideVariableName. The isSolanaBanner check controls business logic (account switching), not just analytics, so it needs getSlideVariableName(slide) instead of the analytics-fallback variant.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit edbf0aa. Configure here.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
The Carousel is rendered in SmokeWalletPlatform is selected as a conservative measure since the Carousel is part of the wallet home screen, and any regression in the component could affect the wallet view. No other tags are needed as this is a focused UI component change with no impact on confirmations, accounts, networks, swaps, or other feature areas. No direct E2E tests exist for the Carousel component itself, so the risk of breaking existing tests is minimal. The changes are low-risk internal refactoring with improved unit test coverage. Performance Test Selection: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30458 +/- ##
==========================================
- Coverage 82.13% 82.13% -0.01%
==========================================
Files 5488 5488
Lines 147743 147748 +5
Branches 33969 33970 +1
==========================================
+ Hits 121353 121357 +4
+ Misses 18084 18082 -2
- Partials 8306 8309 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Description
Changelog
CHANGELOG entry:
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/GE-245
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Note
Medium Risk
Adjusts when and how carousel analytics events fire (display/select naming and frequency), which can impact engagement metrics and downstream dashboards. UI/navigation behavior is mostly unchanged but click handler signature changes touch multiple components.
Overview
Updates carousel analytics so
Banner Displayis emitted only when a non-empty slide becomes the current card (instead of for every visible slide), and standardizes eventnameto usevariableNamewith fallback to Contentfulid.Refactors slide click handling to pass the full
CarouselSlideobject throughStackCard/props and uses the derived analytics name forBanner Select(including Solana-specificactionproperties), and adds targeted unit tests covering the new display/select tracking behavior.Reviewed by Cursor Bugbot for commit 860e897. Bugbot is set up for automated code reviews on this repo. Configure here.