MSC4075 Use expirationTS to define the call ringing window #4521

BillCarsonFr · 2025-09-18T13:44:33Z

Pull Request Checklist

As per MSC4075

Notify events that are older than lifetime are ignored (using the sender_ts timestamp).
Otherwise a client syncing for the first time would ring for outdated call events. In general ringing only makes sense in "real time". Any client which is not able to receive the event in the lifetime period should not ring to prohibit annoying/misleading/irrelevant/outdated rings.

I read the contributing guide.
Pull request contains a changelog label.

UI changes have been tested with:

iPhone and iPad simulators in portrait and landscape orientations.
Dark mode enabled and disabled.
Various sizes of dynamic type.
Voiceover enabled.

codecov · 2025-09-18T14:27:35Z

Codecov Report

❌ Patch coverage is 86.02941% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.95%. Comparing base (2eaad89) to head (7ebd21a).
⚠️ Report is 3 commits behind head on develop.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...rces/Services/ElementCall/ElementCallService.swift	57.14%	15 Missing ⚠️
NSE/Sources/NotificationHandler.swift	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4521      +/-   ##
===========================================
- Coverage    79.16%   78.95%   -0.22%     
===========================================
  Files          836      839       +3     
  Lines        78758    79506     +748     
===========================================
+ Hits         62346    62770     +424     
- Misses       16412    16736     +324

Flag	Coverage Δ
unittests	`71.59% <86.02%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarqubecloud · 2025-09-19T14:33:56Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

stefanceriu · 2025-09-24T04:56:01Z

UnitTests/Sources/ElementCallServiceTests.swift

+
+        let pkPushPayloadMock = PKPushPayloadMock().addSeconds(currentDate, lifetime: 30)
+
+        service.pushRegistry(pushRegistry, didReceiveIncomingPushWith: pkPushPayloadMock, for: .voIP) {


I would've moved everything in ElementCallService.pushRegistry(didReceiveIncomingPushWith) to a separate method taking just they payload's dictionary. We don't actually need anything else and there's no reason to involve CallKit in the unit tests.

We don't actually need anything else and there's no reason to involve CallKit in the unit tests.

Given the intention to test the timeouts, I think we need to involve CallKit here to assert the timeout properly?

stefanceriu · 2025-09-24T05:00:14Z

ElementX/Sources/Services/ElementCall/ElementCallService.swift

+    init(callProvider: CXProviderProtocol? = nil, timeClock: Time? = nil) {
        pushRegistry = PKPushRegistry(queue: nil)

+        self.timeClock = timeClock ?? Time(clock: ContinuousClock(), nowDate: Date.init)


Why not make it the default value in the constructor parameter directly?

It would be nice if the struct properties have default values too. Then the constructor could be timeClock: Time = Time()

stefanceriu · 2025-09-24T05:18:20Z

project.yml

+  Clocks:
+    url: https://github.com/pointfreeco/swift-clocks
+    from: 1.0.6


I'm not a big fan of non "native" solutions for root problems and more so of code from pointfreeco. We should figure out a solution that doesn't bring in a third party component for something as core as this, even if it's our own component.

In this particular case though I don't think we need any of that:

let's have that new version of that pushRegistry(didReceiveIncomingPushWith) method expose a cancellation callback

pass in a significant but small timeout in the dictionary payload

an expectation that we .fulfill() in the callback together with waitForExpectations

I'm not a big fan of non "native" solutions for root problems and more so of code from pointfreeco. We should figure out a solution that doesn't bring in a third party component for something as core as this, even if it's our own component.

Tbh, I think it would be nice to have this kind of testable clock infrastructure. I'd be happy if we implemented it ourselves but let's leave that as an exercise for us though, once this is merged?

pixlwave

Chatted with Stefan about this a bit more. I think we're happy over all with introducing the clocks library for now and we can implement something ourselves later to avoid the dependency.

Just a few comments below 👇

pixlwave · 2025-10-08T09:02:42Z

NSE/Sources/NotificationHandler.swift

        let payload = [ElementCallServiceNotificationKey.roomID.rawValue: roomID,
                       ElementCallServiceNotificationKey.roomDisplayName.rawValue: roomDisplayName,
-                       ElementCallServiceNotificationKey.rtcNotifyEventID.rawValue: rtcNotifyEventID]
+                       ElementCallServiceNotificationKey.expirationTimestampMillis.rawValue: expirationTimestamp,


Could we convert this to a Date here…

pixlwave · 2025-10-08T09:04:31Z

ElementX/Sources/Services/ElementCall/ElementCallService.swift

+        guard let expirationTimestamp = (payload.dictionaryPayload[ElementCallServiceNotificationKey.expirationTimestampMillis.rawValue] as? NSNumber)?.uint64Value else {
+            MXLog.error("Something went wrong, missing expiration timestamp for incoming voip call: \(payload)")
+            return
+        }
+        let nowTimestampMillis = UInt64(timeClock.nowDate().timeIntervalSince1970 * 1000)
+
+        guard nowTimestampMillis < expirationTimestamp else {
+            MXLog.warning("Call expired for room \(roomID), ignoring incoming push")
+            return
+        }
+
+        let ringDurationMillis = min(expirationTimestamp - nowTimestampMillis, 90000)


And then handle all of this using Date comparisons rather than millis so everything is typed properly.

pixlwave · 2025-10-08T09:07:34Z

UnitTests/Sources/ElementCallServiceTests.swift

+
+        let pkPushPayloadMock = PKPushPayloadMock().addSeconds(currentDate, lifetime: 30)
+
+        service.pushRegistry(pushRegistry, didReceiveIncomingPushWith: pkPushPayloadMock, for: .voIP) {


We don't actually need anything else and there's no reason to involve CallKit in the unit tests.

Given the intention to test the timeouts, I think we need to involve CallKit here to assert the timeout properly?

pixlwave · 2025-10-08T09:10:00Z

ElementX/Sources/Services/ElementCall/ElementCallService.swift

+// Keep this class testable
+struct Time {
+    var clock: any Clock<Duration>
+    var nowDate: () -> Date


I think we can call this now, so it aligns with Date.now, Clock.continuous.now etc.

pixlwave · 2025-10-08T09:11:58Z

ElementX/Sources/Services/ElementCall/ElementCallService.swift

-        return CXProvider(configuration: configuration)
-    }()
+    private let callProvider: CXProviderProtocol
+    private let timeClock: Time


And maybe align the names of these. so time: Time or maybe timeProvider: TimeProvider.

pixlwave · 2025-10-08T09:13:29Z

ElementX/Sources/Services/ElementCall/ElementCallService.swift

+    init(callProvider: CXProviderProtocol? = nil, timeClock: Time? = nil) {
        pushRegistry = PKPushRegistry(queue: nil)

+        self.timeClock = timeClock ?? Time(clock: ContinuousClock(), nowDate: Date.init)


It would be nice if the struct properties have default values too. Then the constructor could be timeClock: Time = Time()

pixlwave · 2025-10-08T09:36:31Z

project.yml

+  Clocks:
+    url: https://github.com/pointfreeco/swift-clocks
+    from: 1.0.6


I'm not a big fan of non "native" solutions for root problems and more so of code from pointfreeco. We should figure out a solution that doesn't bring in a third party component for something as core as this, even if it's our own component.

Tbh, I think it would be nice to have this kind of testable clock infrastructure. I'd be happy if we implemented it ourselves but let's leave that as an exercise for us though, once this is merged?

pixlwave · 2025-10-08T09:41:47Z

UnitTests/Sources/ElementCallServiceTests.swift

+        dict
+    }
+
+    func addSeconds(_ from: Date, lifetime: Int) -> Self {


Not important as it's a test, but it would help to follow the code if this was called something like withExpiration seeing as it doesn't add the seconds to the current timeout.

Also, we should use TimeInterval here (or Duration) rather than Int for clarity.

BillCarsonFr added the pr-change for updates to an existing feature label Sep 18, 2025

Base automatically changed from valere/call/subscribe_to_call_decline to develop September 18, 2025 14:11

BillCarsonFr added 3 commits September 18, 2025 22:49

Listen to call decline to stop ringing when declined from other device

2adbec5

review: revert some invisible char/tab changes

6675a85

MSC4075 Use expirationTS to define the call ringing window

6a85119

BillCarsonFr force-pushed the valere/call/notification_lifetime branch from c1e8df2 to 6a85119 Compare September 18, 2025 20:51

pixlwave and others added 3 commits September 19, 2025 09:57

Implement a simple ElementCallService test.

c92783b

Male CallService testable + tests

96edf48

swift clocks only used in tests

7ebd21a

BillCarsonFr marked this pull request as ready for review September 22, 2025 07:15

BillCarsonFr requested a review from a team as a code owner September 22, 2025 07:15

BillCarsonFr requested review from Velin92 and removed request for a team September 22, 2025 07:15

stefanceriu reviewed Sep 24, 2025

View reviewed changes

stefanceriu removed the request for review from Velin92 September 24, 2025 05:26

pixlwave reviewed Oct 8, 2025

View reviewed changes


		let pkPushPayloadMock = PKPushPayloadMock().addSeconds(currentDate, lifetime: 30)

		service.pushRegistry(pushRegistry, didReceiveIncomingPushWith: pkPushPayloadMock, for: .voIP) {

MSC4075 Use expirationTS to define the call ringing window #4521

Are you sure you want to change the base?

MSC4075 Use expirationTS to define the call ringing window #4521

Conversation

BillCarsonFr commented Sep 18, 2025

Pull Request Checklist

Uh oh!

codecov bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Sep 19, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pixlwave left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 18, 2025 •

edited

Loading