Report user presence on app lifecycle transitions#5838
Conversation
Build-required: Swift 6.2.4 rejects a lazy stored property on the nonisolated NSEUserSession class ('nonisolated' is not supported on lazy properties).
Signed-off-by: Jared L <48422312+lhjt@users.noreply.github.com>
Build-required: without these annotations AttributedStringBuilder produces 11 'main actor-isolated default value in a nonisolated context' errors under Xcode 26.3/Swift 6.2.4. Signed-off-by: Jared L <48422312+lhjt@users.noreply.github.com>
Adds a PresenceReporter driven by the app lifecycle handlers and the sharePresence setting: online when foreground-active and sharing, unavailable when backgrounded and sharing, offline when not sharing. Sends are immediate, deduped, and serialized latest-wins. Background app refresh and the NSE seed the process presence to offline so their sync requests carry no presence side effect. Presence rides on the SDK's process-global state, so every sync path is covered without touching sync lifecycle. Signed-off-by: Jared L <48422312+lhjt@users.noreply.github.com>
Pins the presence reporter invariants: the never-online sweep with sharing off, an immediate online send on foreground-active, dedupe of repeated states, latest-wins ordering of serialized sends, and the NSE offline seed. Signed-off-by: Jared L <48422312+lhjt@users.noreply.github.com>
pixlwave
left a comment
There was a problem hiding this comment.
Thanks @lhjt, we'd love to get this fixed. I've put some comments in line, but one there's an overarching point I haven't noted:
19307fd and a15cfc6 do not belong in this PR. It appears as though you might not have the latest Xcode installed and so you're using the Swift 6.2 compiler. Please can you fix your toolchain and drop these commits as everything compiles correctly on our side without those changes. You can always find the current version of Xcode that the project is expecting in the ci_common.sh script:
element-x-ios/ci_scripts/ci_common.sh
Line 42 in a2c1ca0
|
|
||
| func setOfflinePresence() async { | ||
| do { | ||
| try await client.setPresence(presence: .offline, immediate: true) |
There was a problem hiding this comment.
I don't think it makes sense to have this set to immediate: true – the NSE continues to run whilst the app is in the foreground, so this will potentially result in the user's status flipping from online to offline and back again when a notification arrives whilst the user is looking at the app?
There was a problem hiding this comment.
Which thinking about it still wouldn't be fixed by just immediate: false…
Shouldn't we configure the NSE to not send presence values on its sync loops, rather than explicitly marking the user as .offline here?
There was a problem hiding this comment.
I previously had a more in depth design for this problem, specifically setting state shared between the NSE and main app so that the NSE was aware of the main app's foreground/background state. I can reintroduce that here?
Shouldn't we configure the NSE to not send presence values on its sync loops, rather than explicitly marking the user as .offline here?
From my understanding, the spec states that omitting the presence value on a sync should be interpreted as being online by default, which is not what we want here (though please do correct me if I incorrect)
| /// Testable seam for the offline presence seed. The NSE must never let a sync mark the user online or idle, | ||
| /// so it only ever seeds `.offline`. | ||
| nonisolated protocol NSEPresenceSetterProtocol { | ||
| func setOfflinePresence() async | ||
| } |
There was a problem hiding this comment.
From what I can tell this isn't tested (and as there's no logic it probably doesn't make any sense to test it anyway), so we don't really need this part.
| // Copyright 2025 Element Creations Ltd. | ||
| // Copyright 2022-2025 New Vector Ltd. |
There was a problem hiding this comment.
| // Copyright 2025 Element Creations Ltd. | |
| // Copyright 2022-2025 New Vector Ltd. | |
| // Copyright 2026 Element Creations Ltd. |
| @objc | ||
| private func applicationWillResignActive() { | ||
| MXLog.info("Application will resign active") | ||
| presenceReporter?.applicationWillResignActive() |
There was a problem hiding this comment.
We can do this directly inside of the struct with e.g.
NotificationCenter.default.publisher(for: UIApplication.willResignActiveNotification)
And if we do that, this reporter would likely no longer need to live in the AppCoordinator but instead could live directly on the ClientProxy or in the UserSessionFlowCoordinator.
| } | ||
| } | ||
|
|
||
| extension PresenceReporter { |
There was a problem hiding this comment.
Do we need this, it's annoying to have test-specific helpers in the main app. We can use techniques like putting an #expect inside a mock's closure for example.
| /// foreground-active and whether the user shares presence — onto a single desired `ClientProxyPresence` and | ||
| /// pushes it to the server. Sending is deduped and serialised latest-wins so a late background update can never | ||
| /// land after a newer foreground one. | ||
| final class PresenceReporter { |
There was a problem hiding this comment.
Please could we call this PresenceService. That would be more consistent with how we make things like this in the rest of the app.
|
|
||
| /// Updates the process presence that rides on every future sync request. When `sendImmediately` is `true` the SDK | ||
| /// also issues a direct presence update instead of waiting for the next sync request to carry the new value. | ||
| func setPresence(_ presence: ClientProxyPresence, sendImmediately: Bool) async -> Result<Void, ClientProxyError> |
There was a problem hiding this comment.
Looking at how this works SDK side, could we call this one configurePresence seeing as it doesn't actually set the user's presence unless you pass sendImmediately: true. This seems more in line with what happens, it configures the presence that will be used by the sync service the next time the sync loop happens.
Sets the user's presence from the app lifecycle:
online(only while the Share presence setting is on,offlineotherwise)unavailable(orofflinewhen not sharing)offline, so their sync requests carryset_presence=offlineand have no presence side effect. Currently, a push actually marks an offline user as idle, since the SDK's default isunavailable.The goal for this change is to report accurate presence to homeservers to enable better push notification suppression handling. Ensuring that a device instantly exits the
onlinestate when suspended allows the remote homeserver to send any new push notifications without delay.I previous authored matrix-org/matrix-rust-sdk#6672 to support this change.
Known limitation: if the app is force-killed while online, the user stays online until the homeserver times the presence out. A follow-up could correct this from the NSE.
No UI changes.
Pull Request Checklist
UI changes have been tested with: