-
-
Notifications
You must be signed in to change notification settings - Fork 340
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
feat: Session replay masking preview for SwiftUI #4737
base: main
Are you sure you want to change the base?
Conversation
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c151c14 | 1223.18 ms | 1246.56 ms | 23.38 ms |
9324d27 | 1242.25 ms | 1255.15 ms | 12.90 ms |
a176fc4 | 1250.29 ms | 1257.88 ms | 7.59 ms |
e19cca3 | 1211.69 ms | 1231.04 ms | 19.35 ms |
c586015 | 1221.20 ms | 1236.80 ms | 15.59 ms |
279841c | 1237.29 ms | 1254.12 ms | 16.83 ms |
1a4da1b | 1222.14 ms | 1239.50 ms | 17.36 ms |
9f57953 | 1232.83 ms | 1249.87 ms | 17.04 ms |
e71cf92 | 1201.69 ms | 1226.52 ms | 24.83 ms |
5c6ef23 | 1237.33 ms | 1252.58 ms | 15.25 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c151c14 | 21.58 KiB | 678.19 KiB | 656.61 KiB |
9324d27 | 21.58 KiB | 707.43 KiB | 685.85 KiB |
a176fc4 | 22.84 KiB | 403.24 KiB | 380.39 KiB |
e19cca3 | 21.58 KiB | 669.74 KiB | 648.16 KiB |
c586015 | 21.58 KiB | 616.73 KiB | 595.15 KiB |
279841c | 22.84 KiB | 403.19 KiB | 380.34 KiB |
1a4da1b | 21.58 KiB | 418.33 KiB | 396.75 KiB |
9f57953 | 22.31 KiB | 773.84 KiB | 751.53 KiB |
e71cf92 | 20.76 KiB | 419.85 KiB | 399.10 KiB |
5c6ef23 | 21.90 KiB | 708.96 KiB | 687.05 KiB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4737 +/- ##
=============================================
+ Coverage 91.194% 91.202% +0.007%
=============================================
Files 623 623
Lines 72498 72507 +9
Branches 26411 26413 +2
=============================================
+ Hits 66114 66128 +14
+ Misses 6285 6280 -5
Partials 99 99
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -20,24 +20,24 @@ class DefaultViewRenderer: ViewRenderer { | |||
} | |||
|
|||
@objcMembers | |||
class SentryViewPhotographer: NSObject, SentryViewScreenshotProvider { | |||
public class SentryViewPhotographer: NSObject, SentryViewScreenshotProvider { |
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.
@philipphofmann will probably not agree with changing this to public, we should at least somehow mark this class as "internal use only"
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.
We should stop using hacky solutions and add a lot of "code smells" into the code.
If the user try to use an undocumented class, that is not part of the main product, let them try.
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.
I see potential, that we can use the solution for debugging any iOS app. If I'm not mistaken we don't need to limit this for SwiftUI Previews. Or am I missing something, @brustolin?
@@ -1,7 +1,7 @@ | |||
import Foundation | |||
|
|||
@objc | |||
protocol SentryRedactOptions { | |||
public protocol SentryRedactOptions { |
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.
m
: I'm a bit confused about why we need a protocol for this and two separate implementations: SentryRedactDefaultOptions
and PreviewRedactOptions
. Can't we ditch the protocol and have one public final class for this?
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.
IMO protocol is the best approach, it gives a lot of flexibility and separation of concerns.
We can make SentryReplayOptions implement SentryRedactOptionsSentry without the need to have another level in the replay options.
And we can have a simpler object for screenshot redact, and then another one for preview redact, maybe with more options in there specific for preview.
func sentryReplayPreviewMask(redactOptions: SentryRedactOptions? = nil, opacity: Float = 1) -> some View { | ||
let options = redactOptions ?? SentrySDK.options?.sessionReplay ?? PreviewRedactOptions() |
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.
m
: What's the use case for passing different redactOptions than the ones from SentrySDK.options?.sessionReplay
?
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 up to the user to decide how they will use this. They might try different configurations directly from the preview without changing the project options.
if ProcessInfo.processInfo.environment[SENTRY_XCODE_PREVIEW_ENVIRONMENT_KEY] == "1" { | ||
displayLink = CADisplayLink(target: self, selector: #selector(update)) | ||
displayLink?.add(to: .main, forMode: .common) | ||
} else { | ||
print("[SENTRY] [WARNING] SentryReplayMaskPreview is not meant to be used in your app, only with SwiftUI Previews.") | ||
} |
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.
h
: Why limit this functionality only to previews? If somebody wants to use it when running the app on a simulator, let them. I quickly tried it, and it works. Or am I missing something?
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.
If somebody wants to use it...
This could be said for so many things.
I choose to make it harder for the user to forget this on the app by mistake.
We can discuss this approach, but so far we had zero request for it.
@_implementationOnly import SentryInternal | ||
#endif | ||
|
||
class SentryReplayMaskPreviewUIView: UIView { |
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.
h
: We can move this to the main Sentry package as it has no dependencies on SwiftUI. @brustolin I think we could also use this for non SwiftUI apps. We could port this so people can use this with an option for any iOS app while debugging. With that we could also keep the SentryViewPhotographer
internal.
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.
We could, not sure if we should. It think session replay should have its own lib.
Moving this to main just change what needs to be public, because this would need to be public in order for SentrySwiftUI to use it.
Lets go step-by-step.
After finishing this PR for SwiftUI we can debate debugging it for UIKit that has no preview.
Yes, technically we could also use it while running in debug. |
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
…y/sentry-cocoa into feat/sr-masking-preview
📜 Description
Allows the user to preview how its app session replay will be masked during development.
💡 Motivation and Context
closes #4633
💚 How did you test it?
Sample
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps