fix(perps): deduplicate error view and analytics across connection providers#29655
fix(perps): deduplicate error view and analytics across connection providers#29655michalconsensys wants to merge 18 commits into
Conversation
Three PerpsConnectionProvider instances (PerpsScreenStack, PerpsModalStack, PerpsClosePositionBottomSheetStack) all rendered their own error view on connection failure, stacking identical screens and firing PERPS_SCREEN_VIEWED analytics 3x. Introduce PerpsGlobalErrorGate — a single centralized error gate mounted at the root of PerpsScreenStack that polls PerpsConnectionManager and renders one PerpsConnectionErrorView when an error is set. All three route-level providers now pass suppressErrorView so they never render their own error UI. Analytics are debounced (1s) to suppress rapid error→null→error flap cycles.
|
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. |
Cover four previously untested code paths: - isConnecting forwarded as isRetrying to the error view - Polling picks up isConnecting changes - Sentry breadcrumb emitted on retry failure - Gate + suppressErrorView provider integration (error blocks children, no-error renders all children)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29655 +/- ##
==========================================
+ Coverage 81.86% 81.97% +0.10%
==========================================
Files 5255 5294 +39
Lines 138980 140265 +1285
Branches 31518 31887 +369
==========================================
+ Hits 113774 114979 +1205
- Misses 17465 17471 +6
- Partials 7741 7815 +74 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ionErrorView PerpsGlobalErrorGate already emits a debounced PERPS_SCREEN_VIEWED event when a connection error occurs. The useEffect in PerpsConnectionErrorView was firing the same event immediately on mount, producing 2x analytics per error occurrence. Remove it so analytics fires exactly once via the gate's debounced path.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ount point PerpsModalStack is mounted independently in MainNavigator.js as its own screen, separate from PerpsScreenStack. Adding suppressErrorView without wrapping it in PerpsGlobalErrorGate caused a regression where no error UI or retry mechanism would be displayed when a connection error occurs while a Perps modal is accessed via the MainNavigator route.
PerpsGlobalErrorGate was mounted in both PerpsModalStack and PerpsScreenStack. Since both are registered as separate screens in MainNavigator, opening a perps modal while the perps screen is active results in two independent gate instances with duplicate polling, error views, and analytics events. Remove the gate from PerpsModalStack so only PerpsScreenStack owns the single error gate.
The closure in handleRetry captured the stale pre-increment value of retryAttempts, causing the Sentry breadcrumb to always report a count one behind the actual attempt number. Compute the incremented value into a local variable so both setState and the breadcrumb use the correct value.
…orGate Lower debounce window so error analytics fire sooner while still suppressing rapid error→null→error flap cycles.
Three routes were registered twice in the same Stack.Navigator with functionally identical options, causing React Navigation warnings and dead code. Keep the first set (using shared constants) and remove the redundant inline duplicates.
Automated Review — PR #29655
SummaryREQUEST_CHANGES. Live validation fails when entering Perps: React Navigation throws because The PR also does not reference a Jira ticket or linked issue, so there is no external acceptance-criteria source. I am treating the PR body as review claims, not as ticket ACs. Full review detailsReview Claims
Prior ReviewsNo prior Live Validation
Code Quality
Fix Quality
Correctness
Static Analysis
Architecture & DomainThe gate is architecturally sound, but Risk Assessment
Recommended ActionREQUEST_CHANGES Remove the duplicate Line comments (2 comments: 1 must_fix, 1 suggestion)
Recipe (1/4 steps PASS){
"title": "PR 29655 runtime validation - Perps stack enters without navigator crash",
"schema_version": 1,
"pr": 29655,
"validate": {
"workflow": {
"pre_conditions": [
"wallet.unlocked",
"perps.feature_enabled"
],
"entry": "ac7-navigate-perps-home",
"nodes": {
"ac7-navigate-perps-home": {
"action": "navigate",
"target": "PerpsHomeView",
"next": "ac7-assert-perps-route"
},
"ac7-assert-perps-route": {
"action": "wait_for",
"route": "PerpsMarketListView",
"timeout_ms": 8000,
"next": "ac7-assert-no-duplicate-screen-error"
},
"ac7-assert-no-duplicate-screen-error": {
"action": "log_watch",
"window_seconds": 5,
"must_not_appear": [
"A navigator cannot contain multiple 'Screen' components with the same name",
"found duplicate screen named 'PerpsClosePositionModals'"
],
"next": "done"
},
"done": {
"action": "end",
"status": "pass"
}
}
}
}
}No video evidence recorded. |
abretonc7s
left a comment
abretonc7s
left a comment
There was a problem hiding this comment.
Automated review — see comment above for full details.
| import { ensureError } from '../../../../../util/errorUtils'; | ||
| import { isE2E } from '../../../../../util/test/utils'; | ||
|
|
||
| const ANALYTICS_DEBOUNCE_MS = 150; |
There was a problem hiding this comment.
must fix — The PR body says rapid error flaps are debounced with a 1-second window, but this constant is 150 ms. That means an error that clears after 200 ms and reappears before 1 second can still emit two PERPS_SCREEN_VIEWED events, which violates the stated behavior. Please either restore the 1-second window or update the PR/tests/acceptance criteria if 150 ms is the intended product behavior.
|
|
||
| return ( | ||
| <PerpsConnectionProvider isFullScreen> | ||
| <PerpsConnectionProvider isFullScreen suppressErrorView> |
There was a problem hiding this comment.
suggestion — Because PerpsConnectionProvider still records its PerpsConnectionErrorView shown Sentry breadcrumb whenever connectionState.error changes, passing suppressErrorView here suppresses the UI but not the provider breadcrumb. With the screen, modal, and close-position providers mounted, that can still duplicate and mislabel Sentry breadcrumbs even though the global gate owns rendering. Consider moving that breadcrumb to PerpsGlobalErrorGate or guarding it with !suppressErrorView.
…rendered Guard the "PerpsConnectionErrorView shown" breadcrumb with !suppressErrorView so that provider instances mounted with suppressErrorView no longer emit misleading breadcrumbs when the error UI is not actually displayed.
The provider's breadcrumb is suppressed when suppressErrorView is true, but the gate never emitted a replacement. Add an addBreadcrumb call inside the debounced analytics callback so the initial error-view display produces Sentry context for crash-report debugging.
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 20e26e5. Configure here.
michalconsensys
changed the title
… criteria The debounce window was 150ms but the requirement specifies a 1-second window to suppress rapid error→null→error flap cycles. At 150ms, transient errors clearing after ~200ms could still emit duplicate PERPS_SCREEN_VIEWED events.
|
@abretonc7s that is now updated |
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
Tag Selection Rationale:
The risk is medium because while the changes are well-scoped to Perps, the routing structure change (new wrapper component around the entire PerpsScreenStack) could potentially affect navigation flows if the error gate has any unexpected behavior. Performance Test Selection: |
|
Automated Review — PR #29655
SummaryLine comments (1 comments: 1 must_fix)
Recipe (1/4 steps PASS){
"title": "PR 29655 runtime validation - Perps stack enters without navigator crash",
"schema_version": 1,
"pr": 29655,
"validate": {
"workflow": {
"pre_conditions": [
"wallet.unlocked",
"perps.feature_enabled"
],
"entry": "ac7-navigate-perps-home",
"nodes": {
"ac7-navigate-perps-home": {
"action": "navigate",
"target": "PerpsHomeView",
"next": "ac7-assert-perps-route"
},
"ac7-assert-perps-route": {
"action": "wait_for",
"route": "PerpsMarketListView",
"timeout_ms": 8000,
"next": "ac7-assert-no-duplicate-screen-error"
},
"ac7-assert-no-duplicate-screen-error": {
"action": "log_watch",
"window_seconds": 5,
"must_not_appear": [
"A navigator cannot contain multiple 'Screen' components with the same name",
"found duplicate screen named 'PerpsClosePositionModals'"
],
"next": "done"
},
"done": {
"action": "end",
"status": "pass"
}
}
}
}
} |
abretonc7s
left a comment
abretonc7s
left a comment
There was a problem hiding this comment.
Automated review — see comment above for full details.
|
|
||
| return ( | ||
| <PerpsConnectionProvider isFullScreen> | ||
| <PerpsConnectionProvider isFullScreen suppressErrorView> |
There was a problem hiding this comment.
must fix — I think this should be wrapped for the direct MainNavigator route. PerpsModalStack is still registered at top level for Routes.PERPS.MODALS.ROOT, so this suppressErrorView is only safe when the stack is mounted under PerpsScreenStack's PerpsGlobalErrorGate. A small concrete fix would be to export a PerpsModalStackWithErrorGate wrapper, use that from MainNavigator, and keep the plain PerpsModalStack for the nested route that is already under the gate.
4 participants
Description
Multiple
PerpsConnectionProviderinstances (main stack, modal stack, close-position bottom-sheet stack) each independently rendered their ownPerpsConnectionErrorViewand fired their ownPERPS_SCREEN_VIEWEDanalytics event. This caused duplicate error screens to stack on top of each other and duplicate analytics events to fire on every connection failure.This PR introduces a centralized
PerpsGlobalErrorGatecomponent that wraps the entirePerpsScreenStack. When the connection manager reports an error, the gate renders a singlePerpsConnectionErrorViewand suppresses the error view in all nestedPerpsConnectionProviderinstances via the existingsuppressErrorViewprop.Key changes:
PerpsGlobalErrorGatecomponent — pollsPerpsConnectionManager.getConnectionState()and renders a single error view with retry logic when an error is present.PERPS_SCREEN_VIEWEDevents are debounced (1-second window) to suppress rapid error→null→error flap cycles from emitting duplicate events.PerpsConnectionErrorView— the removeduseEffectinPerpsConnectionErrorViewis now handled centrally by the gate.suppressErrorView—PerpsModalStackandPerpsClosePositionBottomSheetStackconnection providers now passsuppressErrorViewto defer error display to the gate.PerpsScreenStack.Changelog
CHANGELOG entry: null
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/TAT-2988
Manual testing steps
Screenshots/Recordings
N/A — no visual changes; this is a structural refactor of error handling logic.
Before
N/A
After
N/A
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
Moderate risk because it changes how connection errors are surfaced across the entire Perps navigation stack and alters analytics/breadcrumb emission timing via polling and debouncing, which could affect user recovery flows and event accuracy.
Overview
Deduplicates Perps connection error handling across nested stacks. Introduces a new
PerpsGlobalErrorGatethat wrapsPerpsScreenStackand renders a singlePerpsConnectionErrorViewwheneverPerpsConnectionManagerreports an error, with centralized retry + retry-attempt tracking.Moves and debounces error analytics/breadcrumbs. Removes the
PERPS_SCREEN_VIEWEDtracking side effect fromPerpsConnectionErrorViewand instead emits it (plus a Sentry breadcrumb) from the gate with a 1s debounce to suppress rapid error flaps; retry failures also add a breadcrumb.Suppresses provider-level error screens and breadcrumbs. All Perps
PerpsConnectionProviderusages in the stack/modals now passsuppressErrorView, and provider breadcrumb emission is skipped when suppressed to avoid duplicates.Adds comprehensive unit tests for the gate covering render/clear behavior, retry logic, polling cleanup, and analytics/breadcrumb deduping/debouncing.
Reviewed by Cursor Bugbot for commit c90bbdf. Bugbot is set up for automated code reviews on this repo. Configure here.