-
-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing OS context for iOS events #958
Conversation
if (key == SentryOperatingSystem.type && | ||
currentValue is SentryOperatingSystem && | ||
value is SentryOperatingSystem) { | ||
final osMap = {...value.toJson(), ...currentValue.toJson()}; | ||
final os = SentryOperatingSystem.fromJson(osMap); | ||
eventContexts[key] = os; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a quick workaround for now, the whole process of merging contexts from the Native and Dart layer should be done differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#942
Flutter sets the os.theme
by default, so when merging the contexts, it does not take the os
from the iOS layer since we don't merge field per field recursively, but rather if the key
exists or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see. Yeah, that's a stupid behavior.
Codecov Report
@@ Coverage Diff @@
## main #958 +/- ##
=======================================
Coverage 89.52% 89.52%
=======================================
Files 105 105
Lines 3292 3292
=======================================
Hits 2947 2947
Misses 345 345 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
📜 Description
Fix missing OS context for iOS events
💡 Motivation and Context
Fix #942
💚 How did you test it?
📝 Checklist
🔮 Next steps
getsentry/team-mobile#11