diff --git a/android/app/src/play/java/com/superproductivity/superproductivity/review/InAppReview.kt b/android/app/src/play/java/com/superproductivity/superproductivity/review/InAppReview.kt index cb3f168e4c..1361b47c5d 100644 --- a/android/app/src/play/java/com/superproductivity/superproductivity/review/InAppReview.kt +++ b/android/app/src/play/java/com/superproductivity/superproductivity/review/InAppReview.kt @@ -45,7 +45,13 @@ object InAppReview { } ) } catch (e: Exception) { - Log.e(TAG, "Unable to open Play Store listing", e) + // Play Store app is absent → fall back to a plain view intent so a + // browser can still open the listing. + try { + activity.startActivity(Intent(Intent.ACTION_VIEW, Uri.parse(PLAY_URL))) + } catch (e2: Exception) { + Log.e(TAG, "Unable to open Play Store listing", e2) + } } } } diff --git a/src/app/core/startup/startup.service.spec.ts b/src/app/core/startup/startup.service.spec.ts index ed2cfd443f..2cf692626b 100644 --- a/src/app/core/startup/startup.service.spec.ts +++ b/src/app/core/startup/startup.service.spec.ts @@ -26,6 +26,7 @@ import { RatePromptService } from '../../features/dialog-please-rate/rate-prompt describe('StartupService', () => { let service: StartupService; let pluginService: jasmine.SpyObj; + let ratePromptService: jasmine.SpyObj; beforeEach(() => { // Mock localStorage @@ -150,6 +151,9 @@ describe('StartupService', () => { service = TestBed.inject(StartupService); pluginService = TestBed.inject(PluginService) as jasmine.SpyObj; + ratePromptService = TestBed.inject( + RatePromptService, + ) as jasmine.SpyObj; }); describe('init', () => { @@ -174,6 +178,9 @@ describe('StartupService', () => { flush(); + // Deferred init hands the rating prompt off to RatePromptService. + expect(ratePromptService.init).toHaveBeenCalled(); + // Restore (window as any).BroadcastChannel = originalBroadcastChannel; })); diff --git a/src/app/features/dialog-please-rate/rate-dialog-state.spec.ts b/src/app/features/dialog-please-rate/rate-dialog-state.spec.ts index 6e255c2e57..2579ffc706 100644 --- a/src/app/features/dialog-please-rate/rate-dialog-state.spec.ts +++ b/src/app/features/dialog-please-rate/rate-dialog-state.spec.ts @@ -207,6 +207,7 @@ describe('rate-dialog-state', () => { it('handles the empty/zero case', () => { expect(isProgressWin(0, 0)).toBe(false); expect(isProgressWin(0, 5)).toBe(false); + expect(isProgressWin(3, 0)).toBe(false); // no divide-by-zero win }); }); }); diff --git a/src/app/features/dialog-please-rate/rate-prompt.service.spec.ts b/src/app/features/dialog-please-rate/rate-prompt.service.spec.ts index cfbf6a01cd..a69583177e 100644 --- a/src/app/features/dialog-please-rate/rate-prompt.service.spec.ts +++ b/src/app/features/dialog-please-rate/rate-prompt.service.spec.ts @@ -3,16 +3,14 @@ import { MatDialog } from '@angular/material/dialog'; import { of } from 'rxjs'; import { provideMockStore, MockStore } from '@ngrx/store/testing'; -import { RatePromptService } from './rate-prompt.service'; +import { RatePromptService, selectTodayProgress } from './rate-prompt.service'; import { LS } from '../../core/persistence/storage-keys.const'; import { getDbDateStr } from '../../util/get-db-date-str'; -import { - selectTodayTaskIds, - selectUndoneTodayTaskIds, -} from '../work-context/store/work-context.selectors'; - -const ids = (n: number): string[] => Array.from({ length: n }, (_, i) => `t${i}`); +import { DataInitStateService } from '../../core/data-init/data-init-state.service'; +// Note: IS_ANDROID_WEB_VIEW / IS_IOS_NATIVE are false in jsdom, so _promptNow +// always takes the web-dialog branch here. The native Play/iOS card paths (and +// their cadence save) are not exercisable in unit tests — they need e2e/native. describe('RatePromptService', () => { let service: RatePromptService; let matDialog: jasmine.SpyObj; @@ -32,11 +30,12 @@ describe('RatePromptService', () => { providers: [ RatePromptService, { provide: MatDialog, useValue: matDialog }, + { + provide: DataInitStateService, + useValue: { isAllDataLoadedInitially$: of(true) }, + }, provideMockStore({ - selectors: [ - { selector: selectTodayTaskIds, value: ids(10) }, - { selector: selectUndoneTodayTaskIds, value: ids(10) }, // done = 0 - ], + selectors: [{ selector: selectTodayProgress, value: { done: 0, total: 10 } }], }), ], }); @@ -53,9 +52,8 @@ describe('RatePromptService', () => { }); }; - // Simulate completing tasks: fewer undone → more done. - const completeDownTo = (undoneCount: number): void => { - store.overrideSelector(selectUndoneTodayTaskIds, ids(undoneCount)); + const setProgress = (done: number, total = 10): void => { + store.overrideSelector(selectTodayProgress, { done, total }); store.refreshState(); }; @@ -99,14 +97,44 @@ describe('RatePromptService', () => { it('prompts once a productive win is reached this session', () => { setEligible(); service.init(); // baseline done = 0 - completeDownTo(2); // done = 8 → absolute-win threshold + setProgress(8); // done = 8 → absolute-win threshold expect(matDialog.open).toHaveBeenCalledTimes(1); }); + it('does NOT prompt when the win is already true at arm time (baseline guard)', () => { + setProgress(8); // 8 done before we ever arm — a disguised cold-launch win + setEligible(); + service.init(); + expect(matDialog.open).not.toHaveBeenCalled(); + + // ...but a genuine further completion this session still fires. + setProgress(9); + expect(matDialog.open).toHaveBeenCalledTimes(1); + }); + + it('prompts at most once per session even on further wins', () => { + setEligible(); + service.init(); + setProgress(8); + setProgress(9); + setProgress(10); + expect(matDialog.open).toHaveBeenCalledTimes(1); + }); + + it('advances the prompt cadence after showing the dialog', () => { + setEligible(); + service.init(); + setProgress(8); + expect(localStorage.setItem).toHaveBeenCalledWith( + LS.RATE_DIALOG_STATE, + jasmine.stringMatching('"lastShownAppStartDay":32'), + ); + }); + it('does not prompt for progress below the win threshold', () => { setEligible(); service.init(); - completeDownTo(8); // done = 2 → below the floor of 3 + setProgress(2); // below the floor of 3 expect(matDialog.open).not.toHaveBeenCalled(); }); @@ -120,7 +148,7 @@ describe('RatePromptService', () => { }); service.init(); - completeDownTo(0); // done = 10, a clear win — but opted out + setProgress(10); // a clear win — but opted out expect(matDialog.open).not.toHaveBeenCalled(); }); @@ -132,7 +160,7 @@ describe('RatePromptService', () => { }); service.init(); - completeDownTo(0); + setProgress(10); expect(matDialog.open).not.toHaveBeenCalled(); }); }); diff --git a/src/app/features/dialog-please-rate/rate-prompt.service.ts b/src/app/features/dialog-please-rate/rate-prompt.service.ts index 82d3d15f68..11e2c998c4 100644 --- a/src/app/features/dialog-please-rate/rate-prompt.service.ts +++ b/src/app/features/dialog-please-rate/rate-prompt.service.ts @@ -1,8 +1,9 @@ -import { inject, Injectable } from '@angular/core'; +import { DestroyRef, inject, Injectable } from '@angular/core'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; import { MatDialog } from '@angular/material/dialog'; -import { Store } from '@ngrx/store'; -import { combineLatest } from 'rxjs'; -import { filter, map, scan, take } from 'rxjs/operators'; +import { createSelector, Store } from '@ngrx/store'; +import { filter, scan, switchMap, take } from 'rxjs/operators'; +import { DataInitStateService } from '../../core/data-init/data-init-state.service'; import { LS } from '../../core/persistence/storage-keys.const'; import { getDbDateStr } from '../../util/get-db-date-str'; import { getMsSinceLastCriticalError } from '../../util/critical-error-signal'; @@ -24,6 +25,19 @@ import { } from './rate-dialog-state'; import { StoreReview } from './store-review'; +// Single composed selector so `done`/`total` are always read from the SAME +// settled state. Deriving them from two separate store.select subscriptions +// (combineLatest) glitches: a task-add emits new total with a stale undone +// count, transiently inflating `done` and firing the prompt on a non-completion. +export const selectTodayProgress = createSelector( + selectTodayTaskIds, + selectUndoneTodayTaskIds, + (allIds, undoneIds) => ({ + done: allIds.length - undoneIds.length, + total: allIds.length, + }), +); + /** * Owns the "please rate" prompt: when to ask (cadence) and when within a session * to actually show it. We never prompt on cold launch — both stores recommend @@ -35,6 +49,8 @@ import { StoreReview } from './store-review'; export class RatePromptService { private readonly _matDialog = inject(MatDialog); private readonly _store = inject(Store); + private readonly _dataInitStateService = inject(DataInitStateService); + private readonly _destroyRef = inject(DestroyRef); private _appStarts = 0; private _isArmed = false; @@ -61,15 +77,16 @@ export class RatePromptService { private _armForWin(): void { this._isArmed = true; - combineLatest([ - this._store.select(selectTodayTaskIds), - this._store.select(selectUndoneTodayTaskIds), - ]) + this._dataInitStateService.isAllDataLoadedInitially$ .pipe( - map(([all, undone]) => ({ done: all.length - undone.length, total: all.length })), - // Treat the first emission as the session baseline so we only fire after - // an in-session completion — never because the win was already true when - // the app opened (which would just be a disguised cold-launch prompt). + // Sample the baseline only once data has hydrated — otherwise the empty + // pre-hydration state is captured as the baseline and the first loaded + // emission looks like an in-session win (a disguised cold-launch prompt). + filter((isLoaded) => isLoaded), + take(1), + switchMap(() => this._store.select(selectTodayProgress)), + // First (settled) emission is the session baseline; only fire on a later + // increase, i.e. a real completion this session. scan( (acc, cur, index) => ({ ...cur, @@ -78,11 +95,12 @@ export class RatePromptService { { done: 0, total: 0, baseline: 0 }, ), filter( - ({ done, total, baseline }) => - this._isArmed && done > baseline && isProgressWin(done, total), + ({ done, total, baseline }) => done > baseline && isProgressWin(done, total), ), take(1), + takeUntilDestroyed(this._destroyRef), ) + // _promptNow re-checks _isArmed, so a stray second init() can't double-prompt. .subscribe(() => this._promptNow()); }