refactor(android,ios): Use native SDK deserializers for User and Breadcrumb#6261
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
Plus 2 more 🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d39457e. Configure here.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
There was a problem hiding this comment.
getCurrentScreenFrom returns screen name when category is nil or non-string
In RNSentryBreadcrumb.m, the guard condition [maybeCategory isKindOfClass:[NSString class]] && ![maybeCategory isEqualToString:@"navigation"] only returns nil for a non-navigation string. When category is nil or a non-string type (e.g., false/NSNumber) and data.to is a valid string, the function falls through and incorrectly returns a screen name instead of nil. The Android implementation has the correct logic (maybeCategory == null || !"navigation".equals(maybeCategory) → return null).
Evidence
RNSentryBreadcrumb.mline 27: condition isisKindOfClass:[NSString class] && !isEqualToString:@"navigation"— both parts must be true to return nil.- For
category = false(NSNumber),isKindOfClass:[NSString class]is NO → condition is false → execution continues. - If
datadictionary is present with a"to"string key, line 36 returns that string — incorrect for a non-navigation breadcrumb. - Swift test
testNullForNonStringCategorypasses only because there is nodatakey; the data+nil-category combination is untested. - Android counterpart in
RNSentryBreadcrumb.javauses the correct inverse guard:maybeCategory == null || !"navigation".equals(maybeCategory).
Identified by Warden find-bugs
📲 Install BuildsAndroid
|
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4966363+dirty | 400.04 ms | 431.08 ms | 31.04 ms |
| ca9d079+dirty | 411.29 ms | 455.12 ms | 43.83 ms |
| 71abba0+dirty | 496.54 ms | 525.16 ms | 28.63 ms |
| 15d4514+dirty | 406.77 ms | 428.06 ms | 21.29 ms |
| d2eadf8+dirty | 414.64 ms | 454.56 ms | 39.92 ms |
| 5569641+dirty | 406.43 ms | 428.51 ms | 22.08 ms |
| 0b5120f+dirty | 503.22 ms | 538.60 ms | 35.38 ms |
| 7436d0f+dirty | 425.90 ms | 475.56 ms | 49.66 ms |
| 5ee78d6+dirty | 551.80 ms | 568.27 ms | 16.47 ms |
| 5fe1c6c+dirty | 401.62 ms | 445.28 ms | 43.66 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4966363+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| ca9d079+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 71abba0+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| 15d4514+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| d2eadf8+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| 5569641+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| 0b5120f+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 7436d0f+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 5ee78d6+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 5fe1c6c+dirty | 43.75 MiB | 48.14 MiB | 4.39 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8929511+dirty | 469.49 ms | 502.65 ms | 33.16 ms |
| a736b76+dirty | 405.78 ms | 458.74 ms | 52.96 ms |
| 5c1e987+dirty | 444.71 ms | 475.13 ms | 30.42 ms |
| 5569641+dirty | 465.92 ms | 532.22 ms | 66.30 ms |
| ae37560+dirty | 428.96 ms | 456.86 ms | 27.90 ms |
| d2eadf8+dirty | 468.02 ms | 530.37 ms | 62.35 ms |
| 7ff4d0f+dirty | 403.38 ms | 427.06 ms | 23.68 ms |
| a0d8cf8+dirty | 533.71 ms | 564.25 ms | 30.54 ms |
| 71abba0+dirty | 411.04 ms | 453.67 ms | 42.63 ms |
| 038a6d7+dirty | 499.02 ms | 527.68 ms | 28.66 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8929511+dirty | 43.94 MiB | 49.02 MiB | 5.08 MiB |
| a736b76+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| 5c1e987+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
| 5569641+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| ae37560+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| d2eadf8+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| 7ff4d0f+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| a0d8cf8+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| 71abba0+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| 038a6d7+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4953e94+dirty | 1212.06 ms | 1214.83 ms | 2.77 ms |
| 7ff4d0f+dirty | 3838.29 ms | 1225.13 ms | -2613.16 ms |
| a3265b6+dirty | 3826.31 ms | 1207.87 ms | -2618.44 ms |
| 2c735cc+dirty | 1229.67 ms | 1221.50 ms | -8.17 ms |
| a5d243c+dirty | 3842.35 ms | 1214.29 ms | -2628.06 ms |
| 9210ae6+dirty | 3815.93 ms | 1214.14 ms | -2601.79 ms |
| 7a89652+dirty | 3861.46 ms | 1229.61 ms | -2631.85 ms |
| a0d8cf8+dirty | 3842.33 ms | 1212.40 ms | -2629.93 ms |
| 038a6d7+dirty | 3849.69 ms | 1228.40 ms | -2621.28 ms |
| 41d6254+dirty | 3845.71 ms | 1224.51 ms | -2621.20 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4953e94+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 7ff4d0f+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| a3265b6+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 2c735cc+dirty | 3.38 MiB | 4.74 MiB | 1.35 MiB |
| a5d243c+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 9210ae6+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 7a89652+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| a0d8cf8+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 038a6d7+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| 41d6254+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5c1e987+dirty | 1208.43 ms | 1220.72 ms | 12.29 ms |
| 3817909+dirty | 1210.76 ms | 1215.64 ms | 4.89 ms |
| 7d6fd3a+dirty | 1210.89 ms | 1217.63 ms | 6.74 ms |
| 5569641+dirty | 3824.35 ms | 1210.78 ms | -2613.57 ms |
| 0b5120f+dirty | 3843.24 ms | 1223.00 ms | -2620.24 ms |
| 7ac3378+dirty | 1202.35 ms | 1198.31 ms | -4.04 ms |
| 4b87b12+dirty | 1199.49 ms | 1199.78 ms | 0.29 ms |
| 890d145+dirty | 1212.98 ms | 1220.10 ms | 7.12 ms |
| ef27341+dirty | 3835.20 ms | 1212.23 ms | -2622.97 ms |
| ad66da3+dirty | 3855.02 ms | 1213.43 ms | -2641.59 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5c1e987+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 3817909+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 7d6fd3a+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| 5569641+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 0b5120f+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 7ac3378+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 4b87b12+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| 890d145+dirty | 3.38 MiB | 4.77 MiB | 1.38 MiB |
| ef27341+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| ad66da3+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| [userInstance setUsername:username]; | ||
| } | ||
| NSMutableDictionary *filteredKeys = [userKeys mutableCopy]; | ||
| [filteredKeys removeObjectForKey:@"geo"]; |
There was a problem hiding this comment.
I just checked the code for the reason, might be worth creating an issue for cocoa to support this parameter
There was a problem hiding this comment.
SentryUser.initWithDictionary: does not recognize "geo" as a known key, so it puts it into self.unknown. Later during serialization, unknown keys are written on top of the serialized dict, which overwrites the properly-set self.geo property. Stripping "geo" before passing to initWithDictionary: avoids this duplication, and we handle geo manually below (since SentryGeo lacks an initWithDictionary: initializer).
There was a problem hiding this comment.
I just checked the code for the reason, might be worth creating an issue for cocoa to support this parameter
Yes, makes sense 👍 👀
There was a problem hiding this comment.
Opened a PR and I'll follow up once released getsentry/sentry-cocoa#8026
| } else { | ||
| breadcrumb.setOrigin("react-native"); | ||
| } | ||
| final @NotNull ILogger logger = Sentry.getCurrentScopes().getOptions().getLogger(); |
There was a problem hiding this comment.
q: why not use rnLogger ?
There was a problem hiding this comment.
Good idea actually 👍 Updated with d8dfaa8
lucas-zimerman
left a comment
lucas-zimerman
left a comment
There was a problem hiding this comment.
small suggestion on the logger, other than that, LGTM!
…dcrumb Replace manual field-by-field map-to-object construction with native SDK deserialization APIs for User and Breadcrumb on both Android and iOS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…seCoupling rule Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SentryGeo inherits SENTRY_NO_INIT from SentrySerializable, so `init` is unavailable — revert to `[SentryGeo alloc]` as before. Fix pre-existing bug in getCurrentScreenFrom where nil or non-string category values would fall through instead of returning nil. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
initWithDictionary: stores non-string values for known fields in the unknown dict rather than silently ignoring them. Update tests to assert individual properties instead of isEqualToUser: which also compares unknown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ivate headers Private headers (SentryBreadcrumb+Private.h, SentryUser+Private.h) are not available in dynamic framework builds. Declare the category methods inline to avoid the header-not-found error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…of fetching from scopes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
antonis
force-pushed
the
antonis/use-native-sdk-deserializers
branch
from
8d5f78c to
d8dfaa8
Compare
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, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d8dfaa8. Configure here.
| scope.setUser(userInstance); | ||
| } catch (Exception e) { | ||
| logger.log(SentryLevel.ERROR, "Failed to deserialize user from map.", e); | ||
| scope.setUser(null); |
There was a problem hiding this comment.
userData error clears scope user
Medium Severity
The new setUser try/catch wraps both User.Deserializer and userDataKeys parsing. If deserialization succeeds but userDataKeys.getString throws on a non-string value, the catch calls scope.setUser(null), wiping the scope user instead of leaving the previous user or applying the deserialized core fields.
Reviewed by Cursor Bugbot for commit d8dfaa8. Configure here.
2 participants
📢 Type of change
📜 Description
Replace manual field-by-field map-to-object construction with native SDK deserialization APIs
💡 Motivation and Context
Closes #4373
💚 How did you test it?
CI
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps