fix: cp-7.41.0 Handle better errors by getting tags in pre-init #13598
Conversation
tommasini
added
needs-dev-review
|
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.
Left a few suggestions.
Also, this broader try..catch will definitely solve the problem, but we could also consider splitting it up by selector or chunks of related selector. e.g. if unlocked is retrieved in a separate try..catch block, we might be able to retrieve that even before Engine initialization.
app/util/sentry/tags/index.ts
Outdated
| }; | ||
| } catch (error) { | ||
| captureException(`Error getting tags: ${error}`); | ||
| return null; |
There was a problem hiding this comment.
It looks like this function returns tags if they're found, or undefined if they're not. Perhaps we can simplify this and not introduce a third return type? We should be able to use undefined here as well
app/util/sentry/tags/index.ts
Outdated
| return; | ||
| if (!state?.engine?.backgroundState?.ApprovalController) return; | ||
| try { | ||
| if (!state?.engine?.backgroundState?.AccountsController) return; |
There was a problem hiding this comment.
Nit: These early exits can be removed now as well, they were intended to guard against the same crashes we're now catching more broadly.
|
@Gudahtt Kept the optional chaining, but based on your comment I think this is in a much better state! Let me know your thoughts! |
tommasini
added
Run Smoke E2E
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13598 +/- ##
==========================================
+ Coverage 62.53% 62.66% +0.13%
==========================================
Files 2005 2014 +9
Lines 44261 44353 +92
Branches 6006 6023 +17
==========================================
+ Hits 27679 27796 +117
+ Misses 14748 14726 -22
+ Partials 1834 1831 -3 ☔ View full report in Codecov by Sentry. |
|
Description
Handle errors in a more scalable way by adding a try catch to get tags function. Also added test coverage
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist