From 94976dbe744d8e8a787148a988d0345ef395acc4 Mon Sep 17 00:00:00 2001 From: mstrofnone Date: Sun, 23 Aug 2026 08:35:14 +1000 Subject: [PATCH] fix(desktop): surface macOS notification-permission OS errors + timeout the request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Enable OS notifications" button still fails on macOS even after e9475dd0 + the auto-enable follow-up, and the failure mode gives the user nothing to act on: 1. Nucleus's requestAuthorization callback carries the OS error string (UNErrorDomain), but the dispatcher discarded it ({ granted, _ -> }) and mapped every non-grant to PermissionState.Denied. The settings UI then showed "Enable in System Settings → Notifications → Amethyst" — a dead end when macOS refused the request outright ("Notifications are not allowed for this application"), because a refused app never gets a System Settings entry. 2. On recent macOS the permission prompt is an auto-dismissing banner. If the user misses it, UNUserNotificationCenter may never invoke the completion handler, leaving requestPermission()'s suspendCancellableCoroutine parked forever and the UI stuck on "Requesting…". Fixes: - requestPermission() now captures the OS error string and exposes it via NotificationDispatcher.lastRequestError (new interface property, null-defaulted so other implementations are unaffected). - The request is wrapped in withTimeoutOrNull(90s); on timeout the coroutine returns, the spinner clears, and lastRequestError tells the user to watch for the banner and retry. - The Denied branch of NotificationSettingsScreen gains an "Ask again" button (re-request re-surfaces the banner) and both branches render the raw OS error when one is present. - sendMac() now uses Nucleus's add(request, callback) overload and reports SendResult.Failed with the OS error instead of unconditionally returning Delivered for a request the notification center may have rejected. Timeout without an ack still counts as delivered (the request was queued). Reproduced the hang + the silent-error path on macOS 26.4 with a minimal Nucleus harness: first requestAuthorization call from a freshly-installed bundle never fired its callback (30s timeout), subsequent calls returned granted=false with "Notifications are not allowed for this application" — neither observable from the Amethyst UI before this change. --- .../notifications/NotificationDispatcher.kt | 12 +++ .../NucleusNotificationDispatcher.kt | 102 ++++++++++++++---- ...ificationDispatcherLastRequestErrorTest.kt | 52 +++++++++ .../ui/settings/NotificationSettingsScreen.kt | 38 ++++++- 4 files changed, 183 insertions(+), 21 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcherLastRequestErrorTest.kt diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcher.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcher.kt index a99cb9a327..ff9f29016d 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcher.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcher.kt @@ -39,6 +39,18 @@ interface NotificationDispatcher { */ val nativeAvailable: StateFlow + /** + * Human-readable error from the most recent [requestPermission] call, + * or null if the last request succeeded / none has run yet. macOS can + * refuse an authorization request outright (UNErrorDomain "Notifications + * are not allowed for this application") without ever showing a prompt — + * a different failure from the user clicking "Don't Allow", with a + * different recovery. The settings UI surfaces this so the user gets + * the real OS message instead of a generic "denied". + */ + val lastRequestError: StateFlow + get() = kotlinx.coroutines.flow.MutableStateFlow(null) + /** * Trigger the OS-level permission prompt (macOS only — no-op elsewhere). * Suspends until the user answers. Updates [permission] as a side effect. diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NucleusNotificationDispatcher.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NucleusNotificationDispatcher.kt index 3f14d2f00a..01f9275df4 100644 --- a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NucleusNotificationDispatcher.kt +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NucleusNotificationDispatcher.kt @@ -28,6 +28,7 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.suspendCancellableCoroutine import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull import java.util.UUID import kotlin.coroutines.resume @@ -53,6 +54,15 @@ class NucleusNotificationDispatcher( ) : NotificationDispatcher { private val host: HostOs = detectHostOs() + private companion object { + const val PERMISSION_REQUEST_TIMEOUT_MS = 90_000L + const val SEND_ACK_TIMEOUT_MS = 10_000L + const val TIMEOUT_ERROR_MESSAGE = + "macOS never answered the permission request. The permission banner may have " + + "auto-dismissed — click again and watch the top-right of the screen for the " + + "Amethyst banner, then click Allow." + } + // Nucleus module availability probed once at construction. Native lib load // is triggered by the first class access — wrap in Throwable catch so a // missing JAR or unsatisfied link doesn't crash the app. @@ -62,6 +72,22 @@ class NucleusNotificationDispatcher( private val _permission = MutableStateFlow(initialPermissionState()) override val permission: StateFlow = _permission.asStateFlow() + /** + * Human-readable error from the most recent [requestPermission] call, or + * null if the last request succeeded / none has run yet. macOS can refuse + * the authorization request outright (e.g. UNErrorDomain + * "Notifications are not allowed for this application" when the system + * blocks the app before any prompt), which is NOT the same as the user + * clicking "Don't Allow" — the recovery for each is different, and the + * settings UI needs the raw OS message to show actionable guidance. + * A timeout marker is also written here when the OS never invokes the + * completion handler (the permission banner can be auto-dismissed on + * recent macOS versions without firing the callback, which previously + * left the request coroutine suspended forever). + */ + private val _lastRequestError = MutableStateFlow(null) + override val lastRequestError: StateFlow = _lastRequestError.asStateFlow() + @Volatile private var winInitialized: Boolean = false @@ -98,25 +124,42 @@ class NucleusNotificationDispatcher( _permission.value = PermissionState.BundleRequired return _permission.value } - val granted = + val outcome = withContext(Dispatchers.IO) { - suspendCancellableCoroutine { cont -> - try { - io.github.kdroidfilter.nucleus.notification.NotificationCenter - .requestAuthorization( - options = - setOf( - io.github.kdroidfilter.nucleus.notification.AuthorizationOption.ALERT, - io.github.kdroidfilter.nucleus.notification.AuthorizationOption.SOUND, - io.github.kdroidfilter.nucleus.notification.AuthorizationOption.BADGE, - ), - callback = { granted, _ -> cont.resume(granted) }, - ) - } catch (t: Throwable) { - cont.resume(false) + // Hard timeout: recent macOS versions present the permission + // request as an auto-dismissing banner; if the user misses it + // the completion handler may never fire, which would + // otherwise leave this coroutine (and the settings UI's + // "Requesting…" spinner) suspended forever. + withTimeoutOrNull(PERMISSION_REQUEST_TIMEOUT_MS) { + suspendCancellableCoroutine> { cont -> + try { + io.github.kdroidfilter.nucleus.notification.NotificationCenter + .requestAuthorization( + options = + setOf( + io.github.kdroidfilter.nucleus.notification.AuthorizationOption.ALERT, + io.github.kdroidfilter.nucleus.notification.AuthorizationOption.SOUND, + io.github.kdroidfilter.nucleus.notification.AuthorizationOption.BADGE, + ), + callback = { granted, error -> cont.resume(granted to error) }, + ) + } catch (t: Throwable) { + cont.resume(false to (t.message ?: t::class.simpleName)) + } } } } + if (outcome == null) { + _lastRequestError.value = TIMEOUT_ERROR_MESSAGE + // Leave _permission at its current value: the OS never answered, + // so we genuinely don't know the state. A follow-up + // refreshPermission() (window focus / settings re-entry) will + // reconcile it if the user did grant via the banner. + return _permission.value + } + val (granted, osError) = outcome + _lastRequestError.value = if (granted) null else osError _permission.value = if (granted) PermissionState.Granted else PermissionState.Denied return _permission.value @@ -202,7 +245,7 @@ class NucleusNotificationDispatcher( } } - private fun sendMac(spec: NotificationSpec): SendResult { + private suspend fun sendMac(spec: NotificationSpec): SendResult { val content = io.github.kdroidfilter.nucleus.notification.NotificationContent( title = spec.title, @@ -220,9 +263,30 @@ class NucleusNotificationDispatcher( identifier = UUID.randomUUID().toString(), content = content, ) - io.github.kdroidfilter.nucleus.notification.NotificationCenter - .add(request) - return SendResult.Delivered + // Nucleus delivers add() asynchronously; the optional callback carries + // the OS error string (null on success). Without it, a rejected + // notification (e.g. unauthorized at send time) is silently swallowed + // and the caller logs a phantom "Delivered". The Result wrapper + // disambiguates "callback fired with null error" from "callback never + // fired" (timeout — treat as delivered, since UNUserNotificationCenter + // still posts the request in that case on some macOS versions). + val addOutcome = + withTimeoutOrNull(SEND_ACK_TIMEOUT_MS) { + suspendCancellableCoroutine> { cont -> + try { + io.github.kdroidfilter.nucleus.notification.NotificationCenter + .add(request) { error -> cont.resume(Result.success(error)) } + } catch (t: Throwable) { + cont.resume(Result.success(t.message ?: t::class.simpleName)) + } + } + } + val addError = addOutcome?.getOrNull() + return when { + addOutcome == null -> SendResult.Delivered // no ack from OS; request was queued + addError.isNullOrBlank() -> SendResult.Delivered + else -> SendResult.Failed(IllegalStateException("UNUserNotificationCenter.add failed: $addError")) + } } // ---------- Windows ---------- diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcherLastRequestErrorTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcherLastRequestErrorTest.kt new file mode 100644 index 0000000000..99609e046c --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/moderation/notifications/NotificationDispatcherLastRequestErrorTest.kt @@ -0,0 +1,52 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.moderation.notifications + +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlin.test.Test +import kotlin.test.assertNull + +/** + * Pins the [NotificationDispatcher.lastRequestError] contract: implementations + * that never surface OS-level request errors must default to a null-valued + * flow so the settings UI can rely on the property existing on every + * dispatcher (avoids `is` checks against NucleusNotificationDispatcher). + */ +class NotificationDispatcherLastRequestErrorTest { + private class MinimalDispatcher : NotificationDispatcher { + override val permission: StateFlow = MutableStateFlow(PermissionState.NotApplicable) + override val nativeAvailable: StateFlow = MutableStateFlow(false) + + override suspend fun requestPermission(): PermissionState = PermissionState.NotApplicable + + override suspend fun refreshPermission(): PermissionState = PermissionState.NotApplicable + + override suspend fun send(spec: NotificationSpec): SendResult = SendResult.Suppressed("test") + + override fun release() {} + } + + @Test + fun `default lastRequestError is null-valued`() { + assertNull(MinimalDispatcher().lastRequestError.value) + } +} diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/settings/NotificationSettingsScreen.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/settings/NotificationSettingsScreen.kt index 3e559f4f92..54170ba447 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/settings/NotificationSettingsScreen.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/settings/NotificationSettingsScreen.kt @@ -120,6 +120,8 @@ fun NotificationSettingsScreen(onBack: (() -> Unit)? = null) { var testStatus by remember { mutableStateOf(null) } var requestingPermission by remember { mutableStateOf(false) } var sendingTest by remember { mutableStateOf(false) } + val lastRequestError by dispatcher?.lastRequestError?.collectAsState() + ?: remember { mutableStateOf(null) } // Re-sync permission state whenever this screen enters composition // and whenever the window regains focus — user may have toggled @@ -219,9 +221,11 @@ fun NotificationSettingsScreen(onBack: (() -> Unit)? = null) { testStatus = when (newState) { PermissionState.Granted -> "Permission granted. Notifications are on — try the test toast below." - PermissionState.Denied -> "Permission denied. Enable in System Settings if you change your mind." + PermissionState.Denied -> + dispatcher?.lastRequestError?.value + ?: "Permission denied. Enable in System Settings if you change your mind." PermissionState.BundleRequired -> "Notifications need a bundled app — run `./gradlew :desktopApp:runDistributable`." - else -> null + else -> dispatcher?.lastRequestError?.value } } }, @@ -310,6 +314,36 @@ fun NotificationSettingsScreen(onBack: (() -> Unit)? = null) { } }, ) { Text("Open System Settings") } + // When the OS refused the request outright (no prompt + // ever shown — e.g. UNErrorDomain "Notifications are + // not allowed for this application"), Amethyst never + // gets a System Settings entry, so the deep link above + // is a dead end. Offer a retry alongside it: on recent + // macOS the permission prompt is an auto-dismissing + // banner, and a fresh request re-surfaces it. + OutlinedButton( + onClick = { + if (requestingPermission) return@OutlinedButton + coroutineScope.launch { + requestingPermission = true + testStatus = "Waiting for OS prompt…" + val newState = + try { + dispatcher?.requestPermission() ?: PermissionState.Denied + } finally { + requestingPermission = false + } + testStatus = + when (newState) { + PermissionState.Granted -> "Permission granted. Try the test toast below." + else -> + dispatcher?.lastRequestError?.value + ?: "Permission denied. Enable in System Settings if you change your mind." + } + } + }, + enabled = dispatcher != null && !requestingPermission, + ) { Text(if (requestingPermission) "Requesting…" else "Ask again") } Text( "Enable in System Settings → Notifications → Amethyst", style = MaterialTheme.typography.bodySmall,