-
-
Notifications
You must be signed in to change notification settings - Fork 354
fix(replay): Mask SVGs from react-native-svg when maskAllVectors=true
#3930
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
Conversation
react-native-svg when maskMedia=true
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d | 429.33 ms | 451.24 ms | 21.91 ms |
| 376301c | 445.52 ms | 474.70 ms | 29.18 ms |
| 063bfce | 469.96 ms | 516.38 ms | 46.42 ms |
| 52f5e03 | 422.50 ms | 465.69 ms | 43.19 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d | 17.73 MiB | 20.04 MiB | 2.30 MiB |
| 376301c | 17.73 MiB | 20.04 MiB | 2.30 MiB |
| 063bfce | 17.73 MiB | 20.04 MiB | 2.30 MiB |
| 52f5e03 | 17.73 MiB | 20.04 MiB | 2.31 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d+dirty | 374.57 ms | 415.25 ms | 40.68 ms |
| 063bfce+dirty | 338.00 ms | 369.88 ms | 31.88 ms |
| 52f5e03+dirty | 391.15 ms | 446.94 ms | 55.79 ms |
| 376301c+dirty | 353.80 ms | 388.54 ms | 34.74 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d+dirty | 7.15 MiB | 8.31 MiB | 1.16 MiB |
| 063bfce+dirty | 7.15 MiB | 8.31 MiB | 1.17 MiB |
| 52f5e03+dirty | 7.15 MiB | 8.32 MiB | 1.17 MiB |
| 376301c+dirty | 7.15 MiB | 8.31 MiB | 1.17 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d+dirty | 1207.36 ms | 1210.32 ms | 2.96 ms |
| 376301c+dirty | 1215.73 ms | 1219.80 ms | 4.06 ms |
| 063bfce+dirty | 1224.27 ms | 1219.66 ms | -4.61 ms |
| 52f5e03+dirty | 1221.27 ms | 1223.08 ms | 1.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d+dirty | 2.36 MiB | 3.04 MiB | 698.69 KiB |
| 376301c+dirty | 2.36 MiB | 3.05 MiB | 702.83 KiB |
| 063bfce+dirty | 2.36 MiB | 3.05 MiB | 702.78 KiB |
| 52f5e03+dirty | 2.36 MiB | 3.05 MiB | 703.38 KiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d+dirty | 1208.60 ms | 1210.47 ms | 1.87 ms |
| 376301c+dirty | 1224.74 ms | 1227.00 ms | 2.26 ms |
| 063bfce+dirty | 1225.38 ms | 1218.06 ms | -7.31 ms |
| 52f5e03+dirty | 1227.53 ms | 1231.76 ms | 4.22 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 41db11d+dirty | 2.92 MiB | 3.61 MiB | 705.84 KiB |
| 376301c+dirty | 2.92 MiB | 3.61 MiB | 709.95 KiB |
| 063bfce+dirty | 2.92 MiB | 3.61 MiB | 710.22 KiB |
| 52f5e03+dirty | 2.92 MiB | 3.61 MiB | 710.43 KiB |
react-native-svg when maskMedia=truereact-native-svg when maskAllVectors=true
brustolin
left a comment
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.
iOS part looks good.
| + (void)addReplayRNRedactClasses:(NSDictionary *_Nullable)replayOptions { | ||
| NSMutableArray *_Nonnull classesToRedact = [[NSMutableArray alloc] init]; | ||
| if ([replayOptions[@"maskAllVectors"] boolValue] == YES) { | ||
| Class _Nullable maybeRNSVGViewClass = NSClassFromString(@"RNSVGSvgView"); |
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.
l: you don't need _Nullable or _Nonnull in the implementation. Everything should be treated as nullable (because technically they are in Objc), and then we code in a safe way based on this.
* feat(replay): Add Mobile Replay Alpha (#3714) * feat(sample): add running indicator (animation overlay) (#3903) * feat(replay): Add breadcrumbs mapping from RN to RRWeb format (#3846) * feat(replay): Add network breadcrumbs (#3912) * fix(replay): Add tests for touch events (#3924) * feat(replay): Filter Sentry event breadcrumbs (#3925) * fix(changelog): Add latest native SDKs details * release: 5.25.0-alpha.2 * misc(samples): Add console anything examples for replay testing (#3928) * feat: Add Sentry Babel Transformer (#3916) * fix(replay): Add app lifecycle breadcrumbs conversion tests (#3932) * chore(deps): bump sentry-android to 7.12.0-alpha.3 * chore(deps): bump sentry-android to 7.12.0-alpha.4 * fix(replay): Mask SVGs from `react-native-svg` when `maskAllVectors=true` (#3930) * fix(replay): Add missing properties to android nav breadcrumbs (#3942) * release: 5.26.0-alpha.3 * misc(replay): Add Mobile Replay Public Beta changelog (#3943) --------- Co-authored-by: Ivan Dlugos <[email protected]> Co-authored-by: Ivan Dlugos <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: Roman Zavarnitsyn <[email protected]> Co-authored-by: Bruno Garcia <[email protected]>
📢 Type of change
📜 Description
This PR fixes visible SVG from
react-native-svgthe most popular library for handling SVGs in RN. RN doesn't have a built in support for SVGs.🛑 Blocked by
redactClassesoption sentry-java#3546💡 Motivation and Context
Web hides SVGs by default when masking all media.
💚 How did you test it?
rn sample app
📝 Checklist
sendDefaultPIIis enabled