From d19ec37aaa058f232ec2c1db68b3ba5ddb71eef5 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Mon, 8 Jun 2026 16:06:16 +0200 Subject: [PATCH] fix(notifications): defer mobile permission prompt to first use (#8120) (#8137) * fix(notifications): defer mobile permission prompt to first use (#8120) The startup effect (askPermissionsIfNotGiven$) requested notification permission ~2s after launch via ensurePermissions(), which contradicts the documented lazy-prompt design in CapacitorReminderService.initialize() and lowers iOS/Android grant rates (an unprompted launch-time dialog is dismissed more often than a contextual one). Read the permission STATE instead and only warn on explicit 'denied'; 'prompt'/'prompt-with-rationale' now stay silent so the OS dialog surfaces lazily on the first real schedule (when the user creates a reminder). - add getPermissionState() to CapacitorNotificationService (no prompt) and refactor checkPermissions() to delegate to it - add getPermissionState() to CapacitorReminderService ('granted' for the legacy Android WebView per #7408, 'denied' when unavailable) - branch the startup effect on the tri-state and defer the exact-alarm check until notifications are granted * fix(notifications): check exact alarms after lazy grant * refactor(notifications): drop dead duplicate snack in exact-alarm catch ensureExactAlarmPermission() never rejects (it swallows its own errors), so the .catch() that re-opened the EXACT_ALARM_DENIED snack was an unreachable duplicate of the .then() warning. Keep a resolving, log-only catch so a future throw can't cache a rejected promise that every scheduling effect would re-await, and document the one-shot memo and the isAndroid() gate. --- .../capacitor-notification.service.spec.ts | 7 + .../capacitor-notification.service.ts | 26 +++- .../capacitor-reminder.service.spec.ts | 28 ++++ .../platform/capacitor-reminder.service.ts | 38 ++++- .../store/mobile-notification.effects.spec.ts | 135 ++++++++++++++++++ .../store/mobile-notification.effects.ts | 73 ++++++++-- 6 files changed, 284 insertions(+), 23 deletions(-) diff --git a/src/app/core/platform/capacitor-notification.service.spec.ts b/src/app/core/platform/capacitor-notification.service.spec.ts index 5afb4c098e..29dd6755b7 100644 --- a/src/app/core/platform/capacitor-notification.service.spec.ts +++ b/src/app/core/platform/capacitor-notification.service.spec.ts @@ -74,6 +74,13 @@ describe('CapacitorNotificationService', () => { }); }); + describe('getPermissionState', () => { + it("should return 'denied' when not available", async () => { + const result = await service.getPermissionState(); + expect(result).toBe('denied'); + }); + }); + describe('checkPermissions', () => { it('should return false when not available', async () => { const result = await service.checkPermissions(); diff --git a/src/app/core/platform/capacitor-notification.service.ts b/src/app/core/platform/capacitor-notification.service.ts index 6afe117dbe..df15050318 100644 --- a/src/app/core/platform/capacitor-notification.service.ts +++ b/src/app/core/platform/capacitor-notification.service.ts @@ -1,4 +1,5 @@ import { inject, Injectable } from '@angular/core'; +import { PermissionState } from '@capacitor/core'; import { ActionPerformed, LocalNotifications, @@ -158,22 +159,35 @@ export class CapacitorNotificationService { } /** - * Check current notification permission status + * Read the current notification permission state WITHOUT prompting. + * + * Distinguishes 'denied' (user explicitly said no — they must re-enable in OS + * settings) from 'prompt' / 'prompt-with-rationale' (never asked yet — stay + * silent and let the lazy request on first real schedule surface the OS + * dialog). Returns 'denied' when notifications are unavailable here or the + * check throws. */ - async checkPermissions(): Promise { + async getPermissionState(): Promise { if (!this.isAvailable) { - return false; + return 'denied'; } try { const result = await LocalNotifications.checkPermissions(); - return result.display === 'granted'; + return result.display; } catch (error) { - Log.err('CapacitorNotificationService: Failed to check permissions', error); - return false; + Log.err('CapacitorNotificationService: Failed to read permission state', error); + return 'denied'; } } + /** + * Check current notification permission status + */ + async checkPermissions(): Promise { + return (await this.getPermissionState()) === 'granted'; + } + /** * Ensure permissions are granted, requesting if necessary */ diff --git a/src/app/core/platform/capacitor-reminder.service.spec.ts b/src/app/core/platform/capacitor-reminder.service.spec.ts index dd923fcb2c..88248141df 100644 --- a/src/app/core/platform/capacitor-reminder.service.spec.ts +++ b/src/app/core/platform/capacitor-reminder.service.spec.ts @@ -39,11 +39,13 @@ describe('CapacitorReminderService', () => { notificationServiceSpy = jasmine.createSpyObj('CapacitorNotificationService', [ 'ensurePermissions', + 'getPermissionState', 'cancel', 'cancelMultiple', 'registerReminderActions', ]); notificationServiceSpy.ensurePermissions.and.returnValue(Promise.resolve(true)); + notificationServiceSpy.getPermissionState.and.returnValue(Promise.resolve('granted')); notificationServiceSpy.cancel.and.returnValue(Promise.resolve(true)); notificationServiceSpy.cancelMultiple.and.returnValue(Promise.resolve(true)); notificationServiceSpy.registerReminderActions.and.returnValue(Promise.resolve()); @@ -183,6 +185,14 @@ describe('CapacitorReminderService', () => { }); }); + describe('getPermissionState', () => { + it("should return 'denied' when not available, without prompting", async () => { + const result = await service.getPermissionState(); + expect(result).toBe('denied'); + expect(notificationServiceSpy.getPermissionState).not.toHaveBeenCalled(); + }); + }); + describe('ensureExactAlarmPermission', () => { it('should return true on non-Android platforms', async () => { const result = await service.ensureExactAlarmPermission(); @@ -238,6 +248,14 @@ describe('CapacitorReminderService', () => { expect(result).toBe(true); expect(checkExactAlarmSpy).not.toHaveBeenCalled(); }); + + it("getPermissionState reports 'granted' without consulting Capacitor", async () => { + // Same #7408 reasoning as ensurePermissions: the upfront check is broken + // in the legacy WebView, so we trust the OS to gate at fire time. + const result = await legacyService.getPermissionState(); + expect(result).toBe('granted'); + expect(notificationServiceSpy.getPermissionState).not.toHaveBeenCalled(); + }); }); describe('with native platform', () => { @@ -303,6 +321,16 @@ describe('CapacitorReminderService', () => { expect(notificationServiceSpy.ensurePermissions).toHaveBeenCalled(); }); + it('getPermissionState delegates to notificationService (no prompt) on native', async () => { + notificationServiceSpy.getPermissionState.and.returnValue( + Promise.resolve('denied'), + ); + const result = await nativeService.getPermissionState(); + expect(result).toBe('denied'); + expect(notificationServiceSpy.getPermissionState).toHaveBeenCalledTimes(1); + expect(notificationServiceSpy.ensurePermissions).not.toHaveBeenCalled(); + }); + it('should include sound property when scheduling on iOS', async () => { await nativeService.scheduleReminder({ notificationId: 42, diff --git a/src/app/core/platform/capacitor-reminder.service.ts b/src/app/core/platform/capacitor-reminder.service.ts index d0a95d81cf..cfb46e34e3 100644 --- a/src/app/core/platform/capacitor-reminder.service.ts +++ b/src/app/core/platform/capacitor-reminder.service.ts @@ -1,5 +1,5 @@ import { inject, Injectable } from '@angular/core'; -import { Capacitor } from '@capacitor/core'; +import { Capacitor, PermissionState } from '@capacitor/core'; import { LocalNotifications } from '@capacitor/local-notifications'; import { Log } from '../log'; import { CapacitorPlatformService } from './capacitor-platform.service'; @@ -286,9 +286,35 @@ export class CapacitorReminderService { return false; } + /** + * Read notification permission state WITHOUT prompting the user. + * + * Used at startup to decide whether to warn (explicit 'denied') while + * deferring the actual OS prompt to the first real schedule (see + * `initialize()` — contextual prompts have better grant rates than an + * unprompted launch-time dialog). + * + * Legacy Android WebView: the upfront Capacitor check is unreliable there + * (no Capacitor bridge → stale `Notification.permission`, issue #7408), so + * report 'granted' and let the OS gate POST_NOTIFICATIONS at fire time — + * matching `ensurePermissions()`. + */ + async getPermissionState(): Promise { + if (!this.isAvailable) { + return 'denied'; + } + + if (this._isLegacyAndroidWebView()) { + return 'granted'; + } + + return this._notificationService.getPermissionState(); + } + /** * Ensure notification permissions are granted. - * Also handles Android 12+ exact alarm permissions. + * Android exact-alarm permission is checked separately once notification + * permission is available. */ async ensurePermissions(): Promise { if (!this.isAvailable) { @@ -310,10 +336,10 @@ export class CapacitorReminderService { return false; } - // Note: exact alarm permission is checked once at startup via - // askPermissionsIfNotGiven$ in mobile-notification.effects.ts. - // We intentionally do NOT check it here to avoid repeatedly - // opening the Android settings page on every scheduling cycle. + // Note: exact alarm permission is checked once by MobileNotificationEffects + // after notification permission is actually available. We intentionally do + // NOT check it here to avoid repeatedly opening the Android settings page + // on every scheduling cycle. return true; } diff --git a/src/app/features/mobile/store/mobile-notification.effects.spec.ts b/src/app/features/mobile/store/mobile-notification.effects.spec.ts index 5ab5eb9af1..0bae818ff0 100644 --- a/src/app/features/mobile/store/mobile-notification.effects.spec.ts +++ b/src/app/features/mobile/store/mobile-notification.effects.spec.ts @@ -100,6 +100,109 @@ describe('MobileNotificationEffects', () => { }); }); + describe('on native platform — askPermissionsIfNotGiven$ (startup, #8120)', () => { + let reminderServiceSpy: jasmine.SpyObj; + let snackServiceSpy: jasmine.SpyObj; + + // Mirrors DELAY_PERMISSIONS in the effects file. + const DELAY_PERMISSIONS_MS = 2000; + + const setup = (platform: 'ios' | 'android' = 'ios'): void => { + reminderServiceSpy = jasmine.createSpyObj('CapacitorReminderService', [ + 'getPermissionState', + 'ensureExactAlarmPermission', + 'ensurePermissions', + 'scheduleReminder', + 'cancelReminder', + ]); + reminderServiceSpy.ensureExactAlarmPermission.and.resolveTo(true); + reminderServiceSpy.scheduleReminder.and.resolveTo(); + reminderServiceSpy.cancelReminder.and.resolveTo(); + + snackServiceSpy = jasmine.createSpyObj('SnackService', ['open']); + + platformService = jasmine.createSpyObj( + 'CapacitorPlatformService', + ['isIOS', 'isAndroid'], + { platform, isNative: true }, + ); + platformService.isAndroid.and.returnValue(platform === 'android'); + + TestBed.configureTestingModule({ + imports: [EffectsModule.forRoot([])], + providers: [ + MobileNotificationEffects, + provideMockStore({ + initialState: {}, + selectors: [ + { selector: selectAllTasksWithReminder, value: [] }, + { selector: selectAllTasksWithDeadlineReminder, value: [] }, + { selector: selectUndoneTasksWithDueDayNoReminder, value: [] }, + ], + }), + { provide: SnackService, useValue: snackServiceSpy }, + { provide: CapacitorReminderService, useValue: reminderServiceSpy }, + { provide: CapacitorPlatformService, useValue: platformService }, + { provide: GlobalConfigService, useValue: { cfg$: NEVER } }, + ], + }); + effects = TestBed.inject(MobileNotificationEffects); + }; + + const runStartup = (): void => { + (effects.askPermissionsIfNotGiven$ as unknown as Observable).subscribe(); + tick(DELAY_PERMISSIONS_MS + 1); + }; + + it('stays silent and does NOT prompt when permission was never requested (prompt)', fakeAsync(() => { + setup('ios'); + reminderServiceSpy.getPermissionState.and.resolveTo('prompt'); + runStartup(); + + expect(reminderServiceSpy.getPermissionState).toHaveBeenCalled(); + // The OS prompt must be deferred to the first real schedule. + expect(reminderServiceSpy.ensurePermissions).not.toHaveBeenCalled(); + expect(reminderServiceSpy.ensureExactAlarmPermission).not.toHaveBeenCalled(); + expect(snackServiceSpy.open).not.toHaveBeenCalled(); + })); + + it('does not prompt for the prompt-with-rationale state either', fakeAsync(() => { + setup('android'); + reminderServiceSpy.getPermissionState.and.resolveTo('prompt-with-rationale'); + runStartup(); + + expect(reminderServiceSpy.ensurePermissions).not.toHaveBeenCalled(); + expect(snackServiceSpy.open).not.toHaveBeenCalled(); + })); + + it('warns once when permission is explicitly denied', fakeAsync(() => { + setup('ios'); + reminderServiceSpy.getPermissionState.and.resolveTo('denied'); + runStartup(); + + expect(snackServiceSpy.open).toHaveBeenCalledTimes(1); + expect(reminderServiceSpy.ensureExactAlarmPermission).not.toHaveBeenCalled(); + })); + + it('checks exact alarm permission when notifications are granted', fakeAsync(() => { + setup('android'); + reminderServiceSpy.getPermissionState.and.resolveTo('granted'); + runStartup(); + + expect(reminderServiceSpy.ensureExactAlarmPermission).toHaveBeenCalledTimes(1); + expect(snackServiceSpy.open).not.toHaveBeenCalled(); + })); + + it('warns when granted but exact alarm permission is denied', fakeAsync(() => { + setup('android'); + reminderServiceSpy.getPermissionState.and.resolveTo('granted'); + reminderServiceSpy.ensureExactAlarmPermission.and.resolveTo(false); + runStartup(); + + expect(snackServiceSpy.open).toHaveBeenCalledTimes(1); + })); + }); + describe('on native platform — disableReminders gating', () => { let reminderServiceSpy: jasmine.SpyObj; let cfg$: BehaviorSubject; @@ -117,10 +220,12 @@ describe('MobileNotificationEffects', () => { beforeEach(() => { reminderServiceSpy = jasmine.createSpyObj('CapacitorReminderService', [ 'ensurePermissions', + 'ensureExactAlarmPermission', 'scheduleReminder', 'cancelReminder', ]); reminderServiceSpy.ensurePermissions.and.resolveTo(true); + reminderServiceSpy.ensureExactAlarmPermission.and.resolveTo(true); reminderServiceSpy.scheduleReminder.and.resolveTo(); reminderServiceSpy.cancelReminder.and.resolveTo(); @@ -131,6 +236,7 @@ describe('MobileNotificationEffects', () => { ['isIOS', 'isAndroid'], { platform: 'android', isNative: true }, ); + platformService.isAndroid.and.returnValue(true); TestBed.configureTestingModule({ imports: [EffectsModule.forRoot([])], @@ -179,6 +285,27 @@ describe('MobileNotificationEffects', () => { expect(reminderServiceSpy.cancelReminder).not.toHaveBeenCalled(); })); + it('checks exact alarm permission once after lazy notification permission is granted', fakeAsync(() => { + store.overrideSelector(selectAllTasksWithReminder, [futureReminder('a')]); + subscribeScheduleNotifications(); + + tick(EFFECT_DELAY_MS + 1); + + expect(reminderServiceSpy.ensureExactAlarmPermission).toHaveBeenCalledTimes(1); + expect(reminderServiceSpy.ensureExactAlarmPermission).toHaveBeenCalledBefore( + reminderServiceSpy.scheduleReminder, + ); + + store.overrideSelector(selectAllTasksWithReminder, [ + futureReminder('a'), + futureReminder('b'), + ]); + store.refreshState(); + tick(1); + + expect(reminderServiceSpy.ensureExactAlarmPermission).toHaveBeenCalledTimes(1); + })); + it('skips scheduling and clears tracking when disableReminders is true from the start', fakeAsync(() => { cfg$.next(buildCfg({ disableReminders: true })); store.overrideSelector(selectAllTasksWithReminder, [futureReminder('a')]); @@ -225,10 +352,12 @@ describe('MobileNotificationEffects', () => { beforeEach(() => { reminderServiceSpy = jasmine.createSpyObj('CapacitorReminderService', [ 'ensurePermissions', + 'ensureExactAlarmPermission', 'scheduleReminder', 'cancelReminder', ]); reminderServiceSpy.ensurePermissions.and.resolveTo(true); + reminderServiceSpy.ensureExactAlarmPermission.and.resolveTo(true); reminderServiceSpy.scheduleReminder.and.resolveTo(); reminderServiceSpy.cancelReminder.and.resolveTo(); @@ -245,6 +374,7 @@ describe('MobileNotificationEffects', () => { ['isIOS', 'isAndroid'], { platform: 'android', isNative: true }, ); + platformService.isAndroid.and.returnValue(true); TestBed.configureTestingModule({ imports: [EffectsModule.forRoot([])], @@ -325,10 +455,12 @@ describe('MobileNotificationEffects', () => { beforeEach(() => { reminderServiceSpy = jasmine.createSpyObj('CapacitorReminderService', [ 'ensurePermissions', + 'ensureExactAlarmPermission', 'scheduleReminder', 'cancelReminder', ]); reminderServiceSpy.ensurePermissions.and.resolveTo(true); + reminderServiceSpy.ensureExactAlarmPermission.and.resolveTo(true); reminderServiceSpy.scheduleReminder.and.resolveTo(); reminderServiceSpy.cancelReminder.and.resolveTo(); @@ -479,10 +611,12 @@ describe('MobileNotificationEffects', () => { beforeEach(() => { reminderServiceSpy = jasmine.createSpyObj('CapacitorReminderService', [ 'ensurePermissions', + 'ensureExactAlarmPermission', 'scheduleReminder', 'cancelReminder', ]); reminderServiceSpy.ensurePermissions.and.resolveTo(true); + reminderServiceSpy.ensureExactAlarmPermission.and.resolveTo(true); reminderServiceSpy.scheduleReminder.and.resolveTo(); reminderServiceSpy.cancelReminder.and.resolveTo(); @@ -493,6 +627,7 @@ describe('MobileNotificationEffects', () => { ['isIOS', 'isAndroid'], { platform: 'android', isNative: true }, ); + platformService.isAndroid.and.returnValue(true); TestBed.configureTestingModule({ imports: [EffectsModule.forRoot([])], diff --git a/src/app/features/mobile/store/mobile-notification.effects.ts b/src/app/features/mobile/store/mobile-notification.effects.ts index b24a9fd168..844b30314a 100644 --- a/src/app/features/mobile/store/mobile-notification.effects.ts +++ b/src/app/features/mobile/store/mobile-notification.effects.ts @@ -72,6 +72,9 @@ export class MobileNotificationEffects { private _scheduledDeadlineIds = new Set(); // Track pre-scheduled recurring reminder IDs (the predicted task instance IDs) private _scheduledRepeatReminderIds = new Set(); + // One-shot guard: the Android exact-alarm check runs at most once per session. + // See _warnIfExactAlarmPermissionDeniedOnce(). + private _exactAlarmPermissionCheckPromise?: Promise; // Narrowed cfg slice so the scheduling effects only re-run on reminder-config // changes, not on every unrelated global-config edit (theme, sync, etc.). @@ -94,21 +97,25 @@ export class MobileNotificationEffects { timer(DELAY_PERMISSIONS).pipe( tap(async () => { try { - const hasPermission = await this._reminderService.ensurePermissions(); - Log.log('MobileEffects: initial permission check', { hasPermission }); - if (!hasPermission) { + // Read the permission STATE without prompting. The OS dialog is + // requested lazily on the first real schedule (see + // CapacitorReminderService.initialize()), which yields better grant + // rates than an unprompted launch-time prompt. Only nag here when + // the user has *explicitly* denied — 'prompt' means we simply + // haven't asked yet, so stay silent and let the lazy prompt run + // when a reminder actually needs scheduling. (#8120) + const permissionState = await this._reminderService.getPermissionState(); + Log.log('MobileEffects: initial permission check', { permissionState }); + if (permissionState === 'denied') { this._notifyPermissionIssue(); return; } - // Check exact alarm permission separately (Android 12+) - const hasExactAlarm = - await this._reminderService.ensureExactAlarmPermission(); - if (!hasExactAlarm) { - this._snackService.open({ - type: 'ERROR', - msg: T.NOTIFICATION.EXACT_ALARM_DENIED, - }); + if (permissionState !== 'granted') { + // Not asked yet — defer the prompt and the exact-alarm check + // until a notification actually needs scheduling. + return; } + await this._warnIfExactAlarmPermissionDeniedOnce(); } catch (error) { Log.err(error); this._notifyPermissionIssue(error?.toString()); @@ -188,6 +195,7 @@ export class MobileNotificationEffects { this._notifyPermissionIssue(); return; } + await this._warnIfExactAlarmPermissionDeniedOnce(); // Schedule each reminder using the platform-appropriate method for (const task of tasksWithReminders) { @@ -305,6 +313,7 @@ export class MobileNotificationEffects { this._notifyPermissionIssue(); return; } + await this._warnIfExactAlarmPermissionDeniedOnce(); for (const occ of upcoming) { await this._reminderService.scheduleReminder({ @@ -385,6 +394,7 @@ export class MobileNotificationEffects { if (!hasPermission) { return; } + await this._warnIfExactAlarmPermissionDeniedOnce(); const now = Date.now(); for (const task of tasks) { @@ -474,6 +484,7 @@ export class MobileNotificationEffects { if (!hasPermission) { return; } + await this._warnIfExactAlarmPermissionDeniedOnce(); const now = Date.now(); for (const task of tasks) { @@ -588,6 +599,46 @@ export class MobileNotificationEffects { return result; } + /** + * Run the Android exact-alarm check at most once per app session, warning the + * user when it is denied. Memoized via `_exactAlarmPermissionCheckPromise` so + * the underlying `ensureExactAlarmPermission()` — which can open the Android + * system settings page — never re-fires across the many scheduling effects + * that call this. A later in-session grant is intentionally not re-detected + * (it resets next launch); the trade-off avoids repeatedly opening that page. + * + * Gated on `isAndroid()`, the superset of native + legacy WebView: on legacy + * WebView `ensureExactAlarmPermission()` self-guards and returns true, so no + * spurious warning fires there. + */ + private _warnIfExactAlarmPermissionDeniedOnce(): Promise { + if (!this._platformService.isAndroid()) { + return Promise.resolve(); + } + + this._exactAlarmPermissionCheckPromise = + this._exactAlarmPermissionCheckPromise || + this._reminderService + .ensureExactAlarmPermission() + .then((hasExactAlarm) => { + if (!hasExactAlarm) { + this._snackService.open({ + type: 'ERROR', + msg: T.NOTIFICATION.EXACT_ALARM_DENIED, + }); + } + }) + // `ensureExactAlarmPermission()` swallows its own errors today, but keep + // a resolving catch so a future throw can't cache a rejected promise + // here (which every scheduling effect would then re-await). Log-only: a + // thrown check is not an explicit denial, so don't show the snack. + .catch((error: unknown) => { + Log.warn('MobileEffects: exact alarm permission check failed', error); + }); + + return this._exactAlarmPermissionCheckPromise; + } + private _notifyPermissionIssue(message?: string): void { if (this._hasShownNotificationWarning) { return;