chore: Convert SentrySessionReplayIntegration to Swift #7102
Conversation
…coa-996-refactor-sentrysessionreplayintegrationm-in-swift
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7102 +/- ##
=============================================
+ Coverage 84.714% 84.786% +0.072%
=============================================
Files 459 461 +2
Lines 27496 27489 -7
Branches 12121 12073 -48
=============================================
+ Hits 23293 23307 +14
+ Misses 4160 4142 -18
+ Partials 43 40 -3
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…SentryReplayDisplayLinkWrapper and add conditional extension for iOS and tvOS.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 139db8b | 1231.50 ms | 1258.19 ms | 26.69 ms |
| 56f9cc6 | 1209.47 ms | 1236.84 ms | 27.37 ms |
| 0529194 | 1237.23 ms | 1254.67 ms | 17.44 ms |
| 3b059e5 | 1229.29 ms | 1256.68 ms | 27.40 ms |
| 55f739c | 1226.06 ms | 1248.78 ms | 22.71 ms |
| fae97e5 | 1229.20 ms | 1256.27 ms | 27.06 ms |
| 7b3399c | 1193.35 ms | 1227.38 ms | 34.03 ms |
| 5ef2eed | 1229.96 ms | 1260.80 ms | 30.84 ms |
| 45482a6 | 1225.88 ms | 1254.27 ms | 28.39 ms |
| 7a8b167 | 1223.12 ms | 1251.61 ms | 28.49 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 139db8b | 23.75 KiB | 920.64 KiB | 896.89 KiB |
| 56f9cc6 | 23.75 KiB | 913.63 KiB | 889.88 KiB |
| 0529194 | 23.74 KiB | 891.02 KiB | 867.28 KiB |
| 3b059e5 | 23.75 KiB | 980.81 KiB | 957.06 KiB |
| 55f739c | 23.75 KiB | 858.73 KiB | 834.98 KiB |
| fae97e5 | 23.75 KiB | 912.37 KiB | 888.62 KiB |
| 7b3399c | 23.75 KiB | 946.68 KiB | 922.94 KiB |
| 5ef2eed | 24.14 KiB | 1023.18 KiB | 999.04 KiB |
| 45482a6 | 23.75 KiB | 919.91 KiB | 896.16 KiB |
| 7a8b167 | 24.14 KiB | 1.01 MiB | 1013.98 KiB |
Previous results on branch: itay/cocoa-996-refactor-sentrysessionreplayintegrationm-in-swift
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 37cd8e6 | 1225.06 ms | 1248.35 ms | 23.29 ms |
| eabc8b8 | 1228.08 ms | 1256.41 ms | 28.33 ms |
| 3aec3d4 | 1224.14 ms | 1251.91 ms | 27.77 ms |
| 542b03c | 1216.48 ms | 1247.83 ms | 31.35 ms |
| b4c2043 | 1211.38 ms | 1246.73 ms | 35.35 ms |
| 00ba50a | 1224.80 ms | 1257.04 ms | 32.24 ms |
| 102508e | 1218.58 ms | 1251.80 ms | 33.22 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 37cd8e6 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| eabc8b8 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 3aec3d4 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 542b03c | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| b4c2043 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 00ba50a | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 102508e | 24.14 KiB | 1.04 MiB | 1.02 MiB |
…onal extension and adding helper method for testing in SentrySessionReplayIntegrationObjC.
| fileprivate var touchTracker: SentryTouchTracker? | ||
|
|
||
| #if SENTRY_TEST || SENTRY_TEST_CI | ||
| // This non final class is used for testing |
There was a problem hiding this comment.
If the only difference here is final I think it would be cleaner just to have the non-final version and a comment saying we should update the tests not to subclass it
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| [_sessionListener | ||
| sentrySessionStartedWithSession:SENTRY_UNWRAP_NULLABLE(SentrySession, _session)]; |
There was a problem hiding this comment.
Bug: Objective-C code calls non-existent selectors sentrySessionStartedWithSession: and sentrySessionEndedWithSession: on a Swift listener, causing a crash. The actual selectors are sentrySessionStarted: and sentrySessionEnded:.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The Swift protocol SentrySessionListener defines methods like sentrySessionStarted(session:). When exposed to Objective-C via @objc, the selector becomes sentrySessionStarted:. However, the calling code in SentryHub.m attempts to invoke a method with a different selector, sentrySessionStartedWithSession:. This mismatch will lead to an unrecognized selector sent to instance exception, causing the application to crash. This crash will occur on every session start and end whenever the SentrySessionReplayIntegration is active.
💡 Suggested Fix
In the SentrySessionListener protocol, explicitly name the Objective-C selectors to match the call sites. For example, change @objc func sentrySessionStarted(session: SentrySession) to @objc(sentrySessionStartedWithSession:) func sentrySessionStarted(session: SentrySession). Apply this to both the sentrySessionStarted and sentrySessionEnded methods.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: Sources/Sentry/SentryHub.m#L130-L131
Potential issue: The Swift protocol `SentrySessionListener` defines methods like
`sentrySessionStarted(session:)`. When exposed to Objective-C via `@objc`, the selector
becomes `sentrySessionStarted:`. However, the calling code in `SentryHub.m` attempts to
invoke a method with a different selector, `sentrySessionStartedWithSession:`. This
mismatch will lead to an `unrecognized selector sent to instance` exception, causing the
application to crash. This crash will occur on every session start and end whenever the
`SentrySessionReplayIntegration` is active.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8039830
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| public func currentScreenNameForSessionReplay() -> String? { | ||
| SentrySDKInternal.currentHub().scope.currentScreen ?? application?.relevantViewControllersNames()?.first ?? "" | ||
| } |
There was a problem hiding this comment.
Screen name returns empty string instead of nil
The currentScreenNameForSessionReplay() method has a trailing ?? "" that returns an empty string when both currentScreen and relevantViewControllersNames()?.first are nil. The original ObjC implementation returned nil in this case. This behavioral change causes empty strings to appear in the screens array of video info (which uses compactMap expecting nil values to be filtered out), resulting in empty screen names being recorded in replay data where previously there would be none.
| } | ||
| guard let timestamp = replayEvent.timestamp else { return } | ||
| SentrySDKInternal.currentHub().captureReplayEvent(replayEvent, replayRecording: replayRecording, video: videoUrl) | ||
| sentrySessionReplaySync_updateInfo(UInt32(replayEvent.segmentId), timestamp.timeIntervalSinceReferenceDate) |
There was a problem hiding this comment.
Replay segment silently dropped when timestamp is nil
The new Swift code adds a guard let timestamp = replayEvent.timestamp else { return } that wasn't present in the original ObjC implementation. In the original code, captureReplayEvent was always called and sentrySessionReplaySync_updateInfo received 0 if timestamp was nil. Now, if timestamp is nil, the replay segment is silently dropped—neither the capture nor the sync update occurs. This behavioral change could cause replay data loss in edge cases where the timestamp is unexpectedly nil.
📜 Description
Converts SentrySessionReplayIntegration to Swift.
Took the chance to remove some logic from SentrySessionReplayIntegration to other classes
💡 Motivation and Context
Work to deprecate SentryDependencyContainer and lower the amount of code in ObjC
💚 How did you test it?
Ran the demo APP
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.Closes #7105