fix(focus-mode): address critical focus mode and Android notification issues

- Fix pausedTaskId race condition: pass pausedTaskId in action payload
  instead of reading from state (reducer clears before effect reads)
- Fix Android focus notification: update title/isBreak on transitions,
  restart timer runnable on resume from paused state
- Fix Android reminder cancellation: track scheduled IDs and cancel
  alarms when reminders are removed
- Fix banner Start button: dispatch skipBreak first when isBreakTimeUp
  to properly resume task tracking
- Fix WebDAV validation: use translation key instead of hardcoded string

Addresses code review feedback for sync issues identified in focus mode,
Android notifications, and reminder scheduling.
This commit is contained in:
Johannes Millan 2025-12-22 19:00:02 +01:00
parent 9309c8aee5
commit d11435808e
14 changed files with 166 additions and 46 deletions

View file

@ -86,10 +86,22 @@ class FocusModeForegroundService : Service() {
}
ACTION_UPDATE -> {
val wasPaused = isPaused
title = intent.getStringExtra(EXTRA_TITLE) ?: title
remainingMs = intent.getLongExtra(EXTRA_REMAINING_MS, remainingMs)
isPaused = intent.getBooleanExtra(EXTRA_IS_PAUSED, isPaused)
isBreak = intent.getBooleanExtra(EXTRA_IS_BREAK, isBreak)
taskTitle = intent.getStringExtra(EXTRA_TASK_TITLE) ?: taskTitle
lastUpdateTimestamp = System.currentTimeMillis()
// Restart update runnable if resuming from paused state
if (wasPaused && !isPaused) {
handler.removeCallbacks(updateRunnable)
handler.post(updateRunnable)
} else if (!wasPaused && isPaused) {
handler.removeCallbacks(updateRunnable)
}
updateNotification()
}

View file

@ -153,11 +153,13 @@ class JavaScriptInterface(
@Suppress("unused")
@JavascriptInterface
fun updateFocusModeService(remainingMs: Long, isPaused: Boolean, taskTitle: String?) {
fun updateFocusModeService(title: String, remainingMs: Long, isPaused: Boolean, isBreak: Boolean, taskTitle: String?) {
val intent = Intent(activity, FocusModeForegroundService::class.java).apply {
action = FocusModeForegroundService.ACTION_UPDATE
putExtra(FocusModeForegroundService.EXTRA_TITLE, title)
putExtra(FocusModeForegroundService.EXTRA_REMAINING_MS, remainingMs)
putExtra(FocusModeForegroundService.EXTRA_IS_PAUSED, isPaused)
putExtra(FocusModeForegroundService.EXTRA_IS_BREAK, isBreak)
putExtra(FocusModeForegroundService.EXTRA_TASK_TITLE, taskTitle)
}
activity.startService(intent)

View file

@ -55,8 +55,10 @@ export interface AndroidInterface {
): void;
stopFocusModeService?(): void;
updateFocusModeService?(
title: string,
remainingMs: number,
isPaused: boolean,
isBreak: boolean,
taskTitle: string | null,
): void;

View file

@ -8,6 +8,7 @@ import {
selectIsBreakActive,
selectIsLongBreak,
selectMode,
selectPausedTaskId,
selectTimeRemaining,
selectTimer,
} from '../../focus-mode/store/focus-mode.selectors';
@ -87,12 +88,16 @@ export class AndroidFocusModeEffects {
} else if (this._hasStateChanged(prev?.timer, timer, taskTitle, curr)) {
// Only update if something significant changed
DroidLog.log('AndroidFocusModeEffects: Updating focus mode service', {
title,
remaining: remainingMs,
isPaused: !timer.isRunning,
isBreak: isBreakActive,
});
androidInterface.updateFocusModeService?.(
title,
remainingMs,
!timer.isRunning,
isBreakActive,
taskTitle,
);
}
@ -133,7 +138,8 @@ export class AndroidFocusModeEffects {
createEffect(() =>
androidInterface.onFocusSkip$.pipe(
tap(() => DroidLog.log('AndroidFocusModeEffects: Skip action received')),
map(() => focusModeActions.skipBreak()),
withLatestFrom(this._store.select(selectPausedTaskId)),
map(([_, pausedTaskId]) => focusModeActions.skipBreak({ pausedTaskId })),
),
);

View file

@ -23,9 +23,11 @@ export class AndroidEffects {
private _snackService = inject(SnackService);
private _taskService = inject(TaskService);
private _taskAttachmentService = inject(TaskAttachmentService);
// Single-shot guard so we dont spam the user with duplicate warnings.
// Single-shot guard so we don't spam the user with duplicate warnings.
private _hasShownNotificationWarning = false;
private _hasCheckedExactAlarm = false;
// Track scheduled reminder IDs to cancel removed ones
private _scheduledReminderIds = new Set<string>();
askPermissionsIfNotGiven$ =
IS_ANDROID_WEB_VIEW &&
@ -64,7 +66,24 @@ export class AndroidEffects {
switchMap(() => this._reminderService.reminders$),
tap(async (reminders) => {
try {
const currentReminderIds = new Set(
(reminders || []).map((r) => r.relatedId),
);
// Cancel reminders that are no longer in the list
for (const previousId of this._scheduledReminderIds) {
if (!currentReminderIds.has(previousId)) {
const notificationId = generateNotificationId(previousId);
DroidLog.log('AndroidEffects: cancelling removed reminder', {
relatedId: previousId,
notificationId,
});
androidInterface.cancelNativeReminder?.(notificationId);
}
}
if (!reminders || reminders.length === 0) {
this._scheduledReminderIds.clear();
return;
}
DroidLog.log('AndroidEffects: scheduling reminders natively', {
@ -101,6 +120,9 @@ export class AndroidEffects {
);
}
// Update tracked IDs
this._scheduledReminderIds = currentReminderIds;
DroidLog.log('AndroidEffects: scheduled native reminders', {
reminderCount: reminders.length,
});

View file

@ -10,6 +10,7 @@ import {
Signal,
} from '@angular/core';
import { T } from '../../../t.const';
import { of } from 'rxjs';
describe('FocusModeBreakComponent', () => {
let component: FocusModeBreakComponent;
@ -20,9 +21,11 @@ describe('FocusModeBreakComponent', () => {
isBreakLong: Signal<boolean>;
};
let environmentInjector: EnvironmentInjector;
const mockPausedTaskId = 'test-task-id';
beforeEach(() => {
mockStore = jasmine.createSpyObj('Store', ['dispatch']);
mockStore = jasmine.createSpyObj('Store', ['dispatch', 'select']);
mockStore.select.and.returnValue(of(mockPausedTaskId));
mockFocusModeService = {
timeRemaining: signal(300000),
@ -78,18 +81,22 @@ describe('FocusModeBreakComponent', () => {
});
describe('skipBreak', () => {
it('should dispatch skipBreak action', () => {
it('should dispatch skipBreak action with pausedTaskId', () => {
component.skipBreak();
expect(mockStore.dispatch).toHaveBeenCalledWith(skipBreak());
expect(mockStore.dispatch).toHaveBeenCalledWith(
skipBreak({ pausedTaskId: mockPausedTaskId }),
);
});
});
describe('completeBreak', () => {
it('should dispatch completeBreak action', () => {
it('should dispatch completeBreak action with pausedTaskId', () => {
component.completeBreak();
expect(mockStore.dispatch).toHaveBeenCalledWith(completeBreak());
expect(mockStore.dispatch).toHaveBeenCalledWith(
completeBreak({ pausedTaskId: mockPausedTaskId }),
);
});
});
});

View file

@ -5,10 +5,12 @@ import { FocusModeService } from '../focus-mode.service';
import { MsToClockStringPipe } from '../../../ui/duration/ms-to-clock-string.pipe';
import { Store } from '@ngrx/store';
import { completeBreak, skipBreak } from '../store/focus-mode.actions';
import { selectPausedTaskId } from '../store/focus-mode.selectors';
import { MatIcon } from '@angular/material/icon';
import { T } from '../../../t.const';
import { TranslatePipe } from '@ngx-translate/core';
import { TaskTrackingInfoComponent } from '../task-tracking-info/task-tracking-info.component';
import { toSignal } from '@angular/core/rxjs-interop';
@Component({
selector: 'focus-mode-break',
@ -30,6 +32,9 @@ export class FocusModeBreakComponent {
private readonly _store = inject(Store);
T: typeof T = T;
// Get pausedTaskId before break ends (passed in action to avoid race condition)
private readonly _pausedTaskId = toSignal(this._store.select(selectPausedTaskId));
readonly remainingTime = computed(() => {
return this.focusModeService.timeRemaining() || 0;
});
@ -45,10 +50,10 @@ export class FocusModeBreakComponent {
);
skipBreak(): void {
this._store.dispatch(skipBreak());
this._store.dispatch(skipBreak({ pausedTaskId: this._pausedTaskId() }));
}
completeBreak(): void {
this._store.dispatch(completeBreak());
this._store.dispatch(completeBreak({ pausedTaskId: this._pausedTaskId() }));
}
}

View file

@ -42,8 +42,14 @@ export const startBreak = createAction(
'[FocusMode] Start Break',
props<{ duration?: number; isLongBreak?: boolean; pausedTaskId?: string | null }>(),
);
export const skipBreak = createAction('[FocusMode] Skip Break');
export const completeBreak = createAction('[FocusMode] Complete Break');
export const skipBreak = createAction(
'[FocusMode] Skip Break',
props<{ pausedTaskId?: string | null }>(),
);
export const completeBreak = createAction(
'[FocusMode] Complete Break',
props<{ pausedTaskId?: string | null }>(),
);
export const incrementCycle = createAction('[FocusMode] Next Cycle');
export const resetCycles = createAction('[FocusMode] Reset Cycles');

View file

@ -12,7 +12,7 @@ import { FocusModeStorageService } from '../focus-mode-storage.service';
import * as actions from './focus-mode.actions';
import * as selectors from './focus-mode.selectors';
import { FocusModeMode, FocusScreen, TimerState } from '../focus-mode.model';
import { unsetCurrentTask } from '../../tasks/store/task.actions';
import { unsetCurrentTask, setCurrentTask } from '../../tasks/store/task.actions';
import { openIdleDialog } from '../../idle/store/idle.actions';
import { selectTaskById } from '../../tasks/store/task.selectors';
import {
@ -424,7 +424,7 @@ describe('FocusModeEffects', () => {
describe('breakComplete$', () => {
it('should dispatch startFocusSession when strategy.shouldAutoStartNextSession is true', (done) => {
actions$ = of(actions.completeBreak());
actions$ = of(actions.completeBreak({ pausedTaskId: null }));
store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro);
store.refreshState();
@ -435,7 +435,7 @@ describe('FocusModeEffects', () => {
});
it('should NOT dispatch startFocusSession when shouldAutoStartNextSession is false', (done) => {
actions$ = of(actions.completeBreak());
actions$ = of(actions.completeBreak({ pausedTaskId: null }));
store.overrideSelector(selectors.selectMode, FocusModeMode.Countdown);
store.refreshState();
@ -455,11 +455,30 @@ describe('FocusModeEffects', () => {
},
});
});
it('should dispatch setCurrentTask when pausedTaskId is provided', (done) => {
const pausedTaskId = 'test-paused-task-id';
actions$ = of(actions.completeBreak({ pausedTaskId }));
store.overrideSelector(selectors.selectMode, FocusModeMode.Countdown);
store.refreshState();
strategyFactoryMock.getStrategy.and.returnValue({
initialSessionDuration: 25 * 60 * 1000,
shouldStartBreakAfterSession: false,
shouldAutoStartNextSession: false,
getBreakDuration: () => null,
});
effects.breakComplete$.pipe(take(1)).subscribe((action) => {
expect(action).toEqual(setCurrentTask({ id: pausedTaskId }));
done();
});
});
});
describe('skipBreak$', () => {
it('should dispatch startFocusSession when strategy.shouldAutoStartNextSession is true', (done) => {
actions$ = of(actions.skipBreak());
actions$ = of(actions.skipBreak({ pausedTaskId: null }));
store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro);
store.refreshState();
@ -470,7 +489,7 @@ describe('FocusModeEffects', () => {
});
it('should NOT dispatch startFocusSession when shouldAutoStartNextSession is false', (done) => {
actions$ = of(actions.skipBreak());
actions$ = of(actions.skipBreak({ pausedTaskId: null }));
store.overrideSelector(selectors.selectMode, FocusModeMode.Countdown);
store.refreshState();
@ -490,6 +509,25 @@ describe('FocusModeEffects', () => {
},
});
});
it('should dispatch setCurrentTask when pausedTaskId is provided', (done) => {
const pausedTaskId = 'test-paused-task-id';
actions$ = of(actions.skipBreak({ pausedTaskId }));
store.overrideSelector(selectors.selectMode, FocusModeMode.Countdown);
store.refreshState();
strategyFactoryMock.getStrategy.and.returnValue({
initialSessionDuration: 25 * 60 * 1000,
shouldStartBreakAfterSession: false,
shouldAutoStartNextSession: false,
getBreakDuration: () => null,
});
effects.skipBreak$.pipe(take(1)).subscribe((action) => {
expect(action).toEqual(setCurrentTask({ id: pausedTaskId }));
done();
});
});
});
describe('cancelSession$', () => {

View file

@ -304,14 +304,13 @@ export class FocusModeEffects {
);
// Handle break completion
// Note: pausedTaskId is passed in action payload to avoid race condition
// (reducer clears pausedTaskId before effect reads state)
breakComplete$ = createEffect(() =>
this.actions$.pipe(
ofType(actions.completeBreak),
withLatestFrom(
this.store.select(selectors.selectMode),
this.store.select(selectors.selectPausedTaskId),
),
switchMap(([_, mode, pausedTaskId]) => {
withLatestFrom(this.store.select(selectors.selectMode)),
switchMap(([action, mode]) => {
const strategy = this.strategyFactory.getStrategy(mode);
const actionsToDispatch: any[] = [];
@ -319,8 +318,8 @@ export class FocusModeEffects {
this._notifyUser();
// Resume task tracking if we paused it during break
if (pausedTaskId) {
actionsToDispatch.push(setCurrentTask({ id: pausedTaskId }));
if (action.pausedTaskId) {
actionsToDispatch.push(setCurrentTask({ id: action.pausedTaskId }));
}
// Auto-start next session if configured
@ -335,20 +334,18 @@ export class FocusModeEffects {
);
// Handle skip break
// Note: pausedTaskId is passed in action payload to avoid race condition
skipBreak$ = createEffect(() =>
this.actions$.pipe(
ofType(actions.skipBreak),
withLatestFrom(
this.store.select(selectors.selectMode),
this.store.select(selectors.selectPausedTaskId),
),
switchMap(([_, mode, pausedTaskId]) => {
withLatestFrom(this.store.select(selectors.selectMode)),
switchMap(([action, mode]) => {
const strategy = this.strategyFactory.getStrategy(mode);
const actionsToDispatch: any[] = [];
// Resume task tracking if we paused it during break
if (pausedTaskId) {
actionsToDispatch.push(setCurrentTask({ id: pausedTaskId }));
if (action.pausedTaskId) {
actionsToDispatch.push(setCurrentTask({ id: action.pausedTaskId }));
}
// Auto-start next session if configured
@ -701,18 +698,39 @@ export class FocusModeEffects {
label: T.F.FOCUS_MODE.B.START,
icon: 'play_arrow',
fn: () => {
// Start a new session using the current mode's strategy
this.store
.select(selectors.selectMode)
.pipe(take(1))
.subscribe((mode) => {
const strategy = this.strategyFactory.getStrategy(mode);
this.store.dispatch(
actions.startFocusSession({
duration: strategy.initialSessionDuration,
}),
);
});
// When starting from break completion, first properly complete/skip the break
// to resume task tracking and clean up state
if (isBreakTimeUp) {
combineLatest([
this.store.select(selectors.selectMode),
this.store.select(selectors.selectPausedTaskId),
])
.pipe(take(1))
.subscribe(([mode, pausedTaskId]) => {
// Skip break (with pausedTaskId to resume tracking)
this.store.dispatch(actions.skipBreak({ pausedTaskId }));
// Then start new session
const strategy = this.strategyFactory.getStrategy(mode);
this.store.dispatch(
actions.startFocusSession({
duration: strategy.initialSessionDuration,
}),
);
});
} else {
// Start a new session using the current mode's strategy
this.store
.select(selectors.selectMode)
.pipe(take(1))
.subscribe((mode) => {
const strategy = this.strategyFactory.getStrategy(mode);
this.store.dispatch(
actions.startFocusSession({
duration: strategy.initialSessionDuration,
}),
);
});
}
},
}
: {

View file

@ -354,7 +354,7 @@ describe('FocusModeReducer', () => {
currentScreen: FocusScreen.Break,
};
const action = a.skipBreak();
const action = a.skipBreak({ pausedTaskId: null });
const result = focusModeReducer(breakState, action);
expect(result.currentScreen).toBe(FocusScreen.Main);
@ -376,7 +376,7 @@ describe('FocusModeReducer', () => {
currentScreen: FocusScreen.Break,
};
const action = a.completeBreak();
const action = a.completeBreak({ pausedTaskId: null });
const result = focusModeReducer(breakState, action);
expect(result.currentScreen).toBe(FocusScreen.Main);

View file

@ -230,7 +230,7 @@ export class ConfigPageComponent implements OnInit, OnDestroy {
) {
this._snackService.open({
type: 'ERROR',
msg: 'Please fill in all WebDAV fields first',
msg: T.F.SYNC.FORM.WEB_DAV.S_FILL_ALL_FIELDS,
});
return;
}

View file

@ -1166,6 +1166,7 @@ const T = {
L_SYNC_FOLDER_PATH: 'F.SYNC.FORM.WEB_DAV.L_SYNC_FOLDER_PATH',
L_TEST_CONNECTION: 'F.SYNC.FORM.WEB_DAV.L_TEST_CONNECTION',
L_USER_NAME: 'F.SYNC.FORM.WEB_DAV.L_USER_NAME',
S_FILL_ALL_FIELDS: 'F.SYNC.FORM.WEB_DAV.S_FILL_ALL_FIELDS',
S_TEST_FAIL: 'F.SYNC.FORM.WEB_DAV.S_TEST_FAIL',
S_TEST_SUCCESS: 'F.SYNC.FORM.WEB_DAV.S_TEST_SUCCESS',
},

View file

@ -1148,6 +1148,7 @@
"L_SYNC_FOLDER_PATH": "Sync Folder Path",
"L_TEST_CONNECTION": "Test Connection",
"L_USER_NAME": "Username",
"S_FILL_ALL_FIELDS": "Please fill in all WebDAV fields first",
"S_TEST_FAIL": "Connection test failed: {{error}} - Target URL: {{url}}",
"S_TEST_SUCCESS": "Connection test successful! Target URL: {{url}}"
}