From e9475dd0794f2d48974e750c8aae3a0079965a97 Mon Sep 17 00:00:00 2001 From: m Date: Wed, 29 Jul 2026 08:11:02 +1000 Subject: [PATCH] fix(desktop): make Enable OS notifications button actually enable them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent gaps meant the desktop notifications flow silently did nothing after the user clicked the settings button: 1. `LocalNotificationSettings` and `LocalNotificationReadState` were declared but never `.provides()`'d anywhere. Both `NotificationSettingsScreen` and `NotificationsScreen` fell back to a fresh `PreferencesNotificationSettings()` / `PreferencesNotification- ReadState()` instance each time — the java.util.prefs backing store kept the values consistent across cold restarts, but each Composable held its own `MutableStateFlow`, so a `setEnabled(true)` in Settings never fired the collectors in the inbox banner or in the auto- dispatcher. The user could toggle the master switch back and forth and nothing observable happened until the app was restarted. The auto-dispatcher in Main.kt was already using its own hoisted `notifSettings` instance too — just never handed down through the composition — so it never even saw the toggle. 2. The "Enable OS notifications" button (only shown on macOS when permission == NotRequested) called `dispatcher.requestPermission()` and updated `permissionState`, but it never touched `settings.enabled`. So the OS prompt appeared, user clicked Allow, the UI cheerfully said "Permission granted" — and toasts still didn't fire because the master switch was still off (defaults to false, first-launch UX choice). The button label promised the whole flow; the code did half of it. Fix: - Hoist `notifSettings` (already existed for the auto-dispatcher) into the app-level `CompositionLocalProvider` as `LocalNotificationSettings provides notifSettings`, so every consumer reads/writes the same instance. Same-instance sharing means StateFlow emissions actually propagate. - Add a per-account `PreferencesNotificationReadState`, keyed on `loggedIn?.pubKeyHex` via `remember(pubKeyHex)`, provided through `LocalNotificationReadState`. This also fixes the "Mark all as read" button in the inbox, which was `enabled = false` because the composition-local was always null. - In `NotificationSettingsScreen`, if `requestPermission()` returns Granted and the master switch is currently off, call `settings.setEnabled(true)` alongside setting the status message. The master switch UI observes `settings.enabled` and recomposes automatically, so clicking one button now does the whole handshake. Behaviour on other platforms is unchanged: Windows / Linux start in `PermissionState.NotApplicable`, so the "Enable OS notifications" branch never renders there — they see the "Send a test toast" button directly. The auto-enable is guarded by `!enabled`, so users who deliberately disabled the master switch and then re-granted permission (e.g. after denying in System Settings) don't get overridden if the switch was still on. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../vitorpamplona/amethyst/desktop/Main.kt | 23 +++++++++++++++++++ .../ui/settings/NotificationSettingsScreen.kt | 13 ++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt index a472cd2b92..a6e734efc0 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt @@ -1262,11 +1262,32 @@ private fun AppInner( // Auto-dispatcher: subscribes to newEventBundles and fires OS toasts. // Only starts once the user is logged in — pubKey and settings must exist. val loggedIn = accountState as? AccountState.LoggedIn + // Single, process-wide NotificationSettings instance. Hoisted here (not + // inside NotificationSettingsScreen / NotificationsScreen) so the + // settings UI, the inbox column's OS-toasts banner, and the auto- + // dispatcher all read + write the same MutableStateFlow. Without this, + // each screen fell back to its own PreferencesNotificationSettings() + // instance — the disk-backed prefs stayed in sync, but the in-memory + // StateFlow updates never crossed instances, so toggling the master + // switch or granting permission never actually notified the auto- + // dispatcher until the app restarted. val notifSettings = remember { com.vitorpamplona.amethyst.commons.moderation.notifications .PreferencesNotificationSettings() } + // Per-account read-state for the notification inbox. Keyed on + // pubKey so switching accounts gets a fresh cursor. `remember` here + // is intentionally keyed on pubKeyHex; a null pubKey (Loading / + // LoggedOut branches never render the settings screen anyway, but + // guard against a stale ReadState leaking between accounts). + val notifReadState = + remember(loggedIn?.pubKeyHex) { + loggedIn?.pubKeyHex?.let { pk -> + com.vitorpamplona.amethyst.commons.moderation.notifications + .PreferencesNotificationReadState(pk) + } + } DisposableEffect(loggedIn?.pubKeyHex, notifDispatcher, localCache) { val myPk = loggedIn?.pubKeyHex val autoDispatcherJob = @@ -1294,6 +1315,8 @@ private fun AppInner( com.vitorpamplona.amethyst.desktop.service.drafts.LocalNoteDraftStore provides noteDraftStore, LocalHashtagSpamSettings provides hashtagSpamSettings, com.vitorpamplona.amethyst.desktop.ui.notifications.LocalNotificationDispatcher provides notifDispatcher, + com.vitorpamplona.amethyst.desktop.ui.notifications.LocalNotificationSettings provides notifSettings, + com.vitorpamplona.amethyst.desktop.ui.notifications.LocalNotificationReadState provides notifReadState, ) { when (accountState) { is AccountState.Loading -> { 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 e5d6b4b28e..ef1c546c1d 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 @@ -183,9 +183,20 @@ fun NotificationSettingsScreen(onBack: (() -> Unit)? = null) { } finally { requestingPermission = false } + // Match the button label: "Enable OS notifications" + // must actually enable them end-to-end. Prior to this, + // clicking through the OS prompt granted permission + // but left the master toggle OFF, so the auto- + // dispatcher stayed muted and the user had to hunt + // for the switch above. Flip it here on grant — the + // switch UI observes settings.enabled and + // recomposes automatically. + if (newState == PermissionState.Granted && !enabled) { + settings.setEnabled(true) + } testStatus = when (newState) { - PermissionState.Granted -> "Permission granted. Try the test toast below." + 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.BundleRequired -> "Notifications need a bundled app — run `./gradlew :desktopApp:runDistributable`." else -> null