mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(rate-dialog): glitch-free win signal + hydration-gated arming
Address multi-agent review findings on the win-moment trigger: - MAJOR: reading done/total from two separate store.select subscriptions via combineLatest glitched — a task *add* transiently paired the new total with a stale undone count, inflating `done` and firing the prompt on a non-completion. Read from a single composed selector (selectTodayProgress) so the pair is always consistent; `done` now only rises on a real completion. - MAJOR: baseline could be sampled before store hydration (empty state → done 0), so the first loaded emission looked like an in-session win. Arm only after DataInitStateService.isAllDataLoadedInitially$, so the baseline is the settled post-hydration value. - Use takeUntilDestroyed for the win subscription. - Android: Play-listing fallback now retries without setPackage so a browser can open the listing when the Play Store app is absent. Tests: cover the baseline guard (already-won-at-arm must NOT prompt, but a later real completion does), fire-at-most-once, the cadence advance after the dialog, the isProgressWin total=0 guard, and StartupService delegating to RatePromptService.init(). Spec notes that native card paths need e2e. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0171mikE4E57gmu1YeEwNhGV
This commit is contained in:
parent
1d057a291e
commit
8bed6833aa
5 changed files with 93 additions and 33 deletions
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@ import { RatePromptService } from '../../features/dialog-please-rate/rate-prompt
|
|||
describe('StartupService', () => {
|
||||
let service: StartupService;
|
||||
let pluginService: jasmine.SpyObj<PluginService>;
|
||||
let ratePromptService: jasmine.SpyObj<RatePromptService>;
|
||||
|
||||
beforeEach(() => {
|
||||
// Mock localStorage
|
||||
|
|
@ -150,6 +151,9 @@ describe('StartupService', () => {
|
|||
|
||||
service = TestBed.inject(StartupService);
|
||||
pluginService = TestBed.inject(PluginService) as jasmine.SpyObj<PluginService>;
|
||||
ratePromptService = TestBed.inject(
|
||||
RatePromptService,
|
||||
) as jasmine.SpyObj<RatePromptService>;
|
||||
});
|
||||
|
||||
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;
|
||||
}));
|
||||
|
|
|
|||
|
|
@ -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
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<MatDialog>;
|
||||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue