From ec8c4bdb8ef08c41395f493eb27e57af8636070f Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Mon, 5 Jan 2026 11:23:46 +0100 Subject: [PATCH] refactor(focus-mode): split sessionComplete$ and breakComplete$ into single-responsibility effects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split monolithic effects into focused, testable units: sessionComplete$ (5+ concerns) → 4 effects: - incrementCycleOnSessionComplete$: Pomodoro cycle management - stopTrackingOnManualEnd$: Stop tracking on manual end (bug #5875) - autoStartBreakOnSessionComplete$: Auto-start break after session - notifyOnSessionComplete$: User notification breakComplete$ (3 concerns) → 3 effects: - resumeTrackingOnBreakComplete$: Resume tracking after break - autoStartSessionOnBreakComplete$: Auto-start next session - notifyOnBreakComplete$: User notification Added 16 new tests before refactoring (TDD approach). All 3265 tests pass. --- .../store/focus-mode.bug-5875.spec.ts | 39 +- .../store/focus-mode.effects.spec.ts | 559 +++++++++++++----- .../focus-mode/store/focus-mode.effects.ts | 166 +++--- 3 files changed, 522 insertions(+), 242 deletions(-) diff --git a/src/app/features/focus-mode/store/focus-mode.bug-5875.spec.ts b/src/app/features/focus-mode/store/focus-mode.bug-5875.spec.ts index b912579fa..9c98adfa9 100644 --- a/src/app/features/focus-mode/store/focus-mode.bug-5875.spec.ts +++ b/src/app/features/focus-mode/store/focus-mode.bug-5875.spec.ts @@ -207,88 +207,69 @@ describe('FocusMode Bug #5875: Pomodoro timer sync issues', () => { describe('Bug 2: Manual End Session should stop time tracking', () => { it('should dispatch unsetCurrentTask when session is manually ended and sync is enabled', (done) => { // Setup: Session is running, sync is enabled, task is being tracked - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); store.overrideSelector(selectFocusModeConfig, { isSyncSessionWithTracking: true, isSkipPreparation: false, - isPauseTrackingDuringBreak: false, - isManualBreakStart: true, // No auto-break, so we can isolate the tracking stop behavior }); currentTaskId$.next('task-123'); // Task is being tracked store.refreshState(); actions$ = of(actions.completeFocusSession({ isManual: true })); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const unsetAction = actionsArr.find((a) => a.type === unsetCurrentTask.type); - expect(unsetAction).toBeDefined(); + effects.stopTrackingOnManualEnd$.pipe(take(1)).subscribe((action) => { + expect(action.type).toEqual(unsetCurrentTask.type); done(); }); }); it('should NOT dispatch unsetCurrentTask when session ends automatically (timer completion)', (done) => { - // Setup: Session completes automatically (not manual), manual break start enabled - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); + // Setup: Session completes automatically (not manual) store.overrideSelector(selectFocusModeConfig, { isSyncSessionWithTracking: true, isSkipPreparation: false, - isPauseTrackingDuringBreak: false, // Don't pause during break - isManualBreakStart: true, // Manual break start, so no auto-break }); currentTaskId$.next('task-123'); store.refreshState(); actions$ = of(actions.completeFocusSession({ isManual: false })); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const unsetAction = actionsArr.find((a) => a.type === unsetCurrentTask.type); - // For automatic completion with manual break start and no pause during break, - // tracking should continue (user explicitly chose not to pause during break) - expect(unsetAction).toBeUndefined(); + effects.stopTrackingOnManualEnd$.pipe(toArray()).subscribe((actionsArr) => { + // For automatic completion, tracking should continue + expect(actionsArr.length).toBe(0); done(); }); }); it('should NOT dispatch unsetCurrentTask when sync is disabled', (done) => { // Setup: Sync is disabled - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); store.overrideSelector(selectFocusModeConfig, { isSyncSessionWithTracking: false, // Sync disabled isSkipPreparation: false, - isManualBreakStart: true, }); currentTaskId$.next('task-123'); store.refreshState(); actions$ = of(actions.completeFocusSession({ isManual: true })); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const unsetAction = actionsArr.find((a) => a.type === unsetCurrentTask.type); - expect(unsetAction).toBeUndefined(); + effects.stopTrackingOnManualEnd$.pipe(toArray()).subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); done(); }); }); it('should NOT dispatch unsetCurrentTask when no task is being tracked', (done) => { // Setup: No task is being tracked - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); store.overrideSelector(selectFocusModeConfig, { isSyncSessionWithTracking: true, isSkipPreparation: false, - isManualBreakStart: true, }); currentTaskId$.next(null); // No task tracking store.refreshState(); actions$ = of(actions.completeFocusSession({ isManual: true })); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const unsetAction = actionsArr.find((a) => a.type === unsetCurrentTask.type); - expect(unsetAction).toBeUndefined(); + effects.stopTrackingOnManualEnd$.pipe(toArray()).subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); done(); }); }); diff --git a/src/app/features/focus-mode/store/focus-mode.effects.spec.ts b/src/app/features/focus-mode/store/focus-mode.effects.spec.ts index 7caa642d7..862144149 100644 --- a/src/app/features/focus-mode/store/focus-mode.effects.spec.ts +++ b/src/app/features/focus-mode/store/focus-mode.effects.spec.ts @@ -326,173 +326,293 @@ describe('FocusModeEffects', () => { }); }); - describe('sessionComplete$', () => { - it('should dispatch incrementCycle for Pomodoro mode', (done) => { - actions$ = of(actions.completeFocusSession({ isManual: true })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); - store.refreshState(); + describe('session completion effects (refactored)', () => { + describe('incrementCycleOnSessionComplete$', () => { + it('should dispatch incrementCycle for Pomodoro mode', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: true })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.overrideSelector(selectors.selectCurrentCycle, 1); + store.refreshState(); - effects.sessionComplete$.pipe(take(1)).subscribe((action) => { - expect(action).toEqual(actions.incrementCycle()); - done(); - }); - }); - - it('should NOT dispatch incrementCycle for Flowtime mode', (done) => { - actions$ = of(actions.completeFocusSession({ isManual: true })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Flowtime); - store.refreshState(); - - strategyFactoryMock.getStrategy.and.returnValue({ - initialSessionDuration: 0, - shouldStartBreakAfterSession: false, - shouldAutoStartNextSession: false, - getBreakDuration: () => null, - }); - - const result: any[] = []; - effects.sessionComplete$.subscribe({ - next: (action) => result.push(action), - complete: () => { - const hasIncrementCycle = result.some( - (a) => a.type === actions.incrementCycle.type, - ); - expect(hasIncrementCycle).toBeFalse(); + effects.incrementCycleOnSessionComplete$.pipe(take(1)).subscribe((action) => { + expect(action).toEqual(actions.incrementCycle()); done(); - }, + }); + }); + + it('should NOT dispatch incrementCycle for Flowtime mode', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: true })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Flowtime); + store.refreshState(); + + const result: any[] = []; + effects.incrementCycleOnSessionComplete$.subscribe({ + next: (action) => result.push(action), + complete: () => { + expect(result.length).toBe(0); + done(); + }, + }); }); }); - it('should dispatch startBreak for automatic (non-manual) completions when strategy allows', (done) => { - actions$ = of(actions.completeFocusSession({ isManual: false })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); - store.refreshState(); + describe('autoStartBreakOnSessionComplete$', () => { + it('should dispatch startBreak for automatic completions when strategy allows', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: false })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.overrideSelector(selectors.selectCurrentCycle, 1); + store.refreshState(); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const startBreakAction = actionsArr.find( - (a) => a.type === actions.startBreak.type, - ); - expect(startBreakAction).toBeDefined(); - expect(startBreakAction.duration).toBe(5 * 60 * 1000); - expect(startBreakAction.isLongBreak).toBeFalse(); - done(); + effects.autoStartBreakOnSessionComplete$ + .pipe(toArray()) + .subscribe((actionsArr) => { + const startBreakAction = actionsArr.find( + (a) => a.type === actions.startBreak.type, + ); + expect(startBreakAction).toBeDefined(); + expect(startBreakAction.duration).toBe(5 * 60 * 1000); + expect(startBreakAction.isLongBreak).toBeFalse(); + done(); + }); + }); + + it('should dispatch startBreak for manual completions (to allow early break start)', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: true })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.overrideSelector(selectors.selectCurrentCycle, 1); + store.refreshState(); + + effects.autoStartBreakOnSessionComplete$ + .pipe(toArray()) + .subscribe((actionsArr) => { + const startBreakAction = actionsArr.find( + (a) => a.type === actions.startBreak.type, + ); + expect(startBreakAction).toBeDefined(); + done(); + }); + }); + + it('should NOT dispatch startBreak when isManualBreakStart is enabled', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: false })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.overrideSelector(selectors.selectCurrentCycle, 1); + store.overrideSelector(selectFocusModeConfig, { + isSyncSessionWithTracking: false, + isSkipPreparation: false, + isManualBreakStart: true, + }); + store.refreshState(); + + effects.autoStartBreakOnSessionComplete$ + .pipe(toArray()) + .subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); + done(); + }); + }); + + it('should dispatch correct isLongBreak based on cycle', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: false })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.overrideSelector(selectors.selectCurrentCycle, 4); + store.refreshState(); + + strategyFactoryMock.getStrategy.and.returnValue({ + initialSessionDuration: 25 * 60 * 1000, + shouldStartBreakAfterSession: true, + shouldAutoStartNextSession: true, + getBreakDuration: jasmine + .createSpy('getBreakDuration') + .and.returnValue({ duration: 15 * 60 * 1000, isLong: true }), + }); + + effects.autoStartBreakOnSessionComplete$ + .pipe(toArray()) + .subscribe((actionsArr) => { + const startBreakAction = actionsArr.find( + (a) => a.type === actions.startBreak.type, + ); + expect(startBreakAction).toBeDefined(); + expect(startBreakAction.isLongBreak).toBeTrue(); + expect(startBreakAction.duration).toBe(15 * 60 * 1000); + done(); + }); + }); + + it('should NOT dispatch when strategy.shouldStartBreakAfterSession is false', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: false })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Flowtime); + store.overrideSelector(selectors.selectCurrentCycle, 1); + store.refreshState(); + + strategyFactoryMock.getStrategy.and.returnValue({ + initialSessionDuration: 0, + shouldStartBreakAfterSession: false, + shouldAutoStartNextSession: false, + getBreakDuration: () => null, + }); + + effects.autoStartBreakOnSessionComplete$ + .pipe(toArray()) + .subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); + done(); + }); }); }); - it('should dispatch startBreak for manual completions (to allow early break start)', (done) => { - actions$ = of(actions.completeFocusSession({ isManual: true })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); - store.refreshState(); + describe('stopTrackingOnManualEnd$', () => { + it('should dispatch unsetCurrentTask when isManual=true AND isSyncSessionWithTracking=true AND currentTaskId exists', (done) => { + currentTaskId$.next('task-123'); + actions$ = of(actions.completeFocusSession({ isManual: true })); + store.overrideSelector(selectFocusModeConfig, { + isSyncSessionWithTracking: true, + isSkipPreparation: false, + }); + store.refreshState(); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const startBreakAction = actionsArr.find( - (a) => a.type === actions.startBreak.type, - ); - expect(startBreakAction).toBeDefined(); - done(); + effects.stopTrackingOnManualEnd$.pipe(take(1)).subscribe((action) => { + expect(action).toEqual(unsetCurrentTask()); + done(); + }); + }); + + it('should NOT dispatch unsetCurrentTask when isManual=true but isSyncSessionWithTracking=false', (done) => { + currentTaskId$.next('task-123'); + actions$ = of(actions.completeFocusSession({ isManual: true })); + store.overrideSelector(selectFocusModeConfig, { + isSyncSessionWithTracking: false, + isSkipPreparation: false, + }); + store.refreshState(); + + effects.stopTrackingOnManualEnd$.pipe(toArray()).subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); + done(); + }); + }); + + it('should NOT dispatch unsetCurrentTask when isManual=true but currentTaskId is null', (done) => { + currentTaskId$.next(null); + actions$ = of(actions.completeFocusSession({ isManual: true })); + store.overrideSelector(selectFocusModeConfig, { + isSyncSessionWithTracking: true, + isSkipPreparation: false, + }); + store.refreshState(); + + effects.stopTrackingOnManualEnd$.pipe(toArray()).subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); + done(); + }); + }); + + it('should NOT dispatch unsetCurrentTask when isManual=false (auto completion)', (done) => { + currentTaskId$.next('task-123'); + actions$ = of(actions.completeFocusSession({ isManual: false })); + store.overrideSelector(selectFocusModeConfig, { + isSyncSessionWithTracking: true, + isSkipPreparation: false, + }); + store.refreshState(); + + effects.stopTrackingOnManualEnd$.pipe(toArray()).subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); + done(); + }); }); }); - it('should NOT dispatch startBreak when isManualBreakStart is enabled', (done) => { - actions$ = of(actions.completeFocusSession({ isManual: false })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 1); - store.overrideSelector(selectFocusModeConfig, { - isSyncSessionWithTracking: false, - isSkipPreparation: false, - isManualBreakStart: true, - }); - store.refreshState(); + describe('edge cases', () => { + it('should handle missing focusModeConfig gracefully in incrementCycleOnSessionComplete$', (done) => { + actions$ = of(actions.completeFocusSession({ isManual: true })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.overrideSelector(selectors.selectCurrentCycle, 1); + store.overrideSelector(selectFocusModeConfig, null as any); + store.refreshState(); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const startBreakAction = actionsArr.find( - (a) => a.type === actions.startBreak.type, - ); - expect(startBreakAction).toBeUndefined(); - done(); - }); - }); - - it('should dispatch correct isLongBreak based on cycle', (done) => { - actions$ = of(actions.completeFocusSession({ isManual: false })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.overrideSelector(selectors.selectCurrentCycle, 4); - store.refreshState(); - - strategyFactoryMock.getStrategy.and.returnValue({ - initialSessionDuration: 25 * 60 * 1000, - shouldStartBreakAfterSession: true, - shouldAutoStartNextSession: true, - getBreakDuration: jasmine - .createSpy('getBreakDuration') - .and.returnValue({ duration: 15 * 60 * 1000, isLong: true }), - }); - - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { - const startBreakAction = actionsArr.find( - (a) => a.type === actions.startBreak.type, - ); - expect(startBreakAction).toBeDefined(); - expect(startBreakAction.isLongBreak).toBeTrue(); - expect(startBreakAction.duration).toBe(15 * 60 * 1000); - done(); + // Should still dispatch incrementCycle + effects.incrementCycleOnSessionComplete$.pipe(take(1)).subscribe({ + next: (action) => { + expect(action).toEqual(actions.incrementCycle()); + done(); + }, + error: (err) => { + fail('Should not throw error: ' + err); + }, + }); }); }); }); - describe('breakComplete$', () => { - it('should dispatch startFocusSession when strategy.shouldAutoStartNextSession is true', (done) => { - actions$ = of(actions.completeBreak({ pausedTaskId: null })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); - store.refreshState(); + describe('break completion effects (refactored)', () => { + describe('autoStartSessionOnBreakComplete$', () => { + it('should dispatch startFocusSession when strategy.shouldAutoStartNextSession is true', (done) => { + actions$ = of(actions.completeBreak({ pausedTaskId: null })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.refreshState(); - effects.breakComplete$.subscribe((action) => { - expect(action).toEqual(actions.startFocusSession({ duration: 25 * 60 * 1000 })); - done(); - }); - }); - - it('should NOT dispatch startFocusSession when shouldAutoStartNextSession is false', (done) => { - actions$ = of(actions.completeBreak({ pausedTaskId: null })); - store.overrideSelector(selectors.selectMode, FocusModeMode.Countdown); - store.refreshState(); - - strategyFactoryMock.getStrategy.and.returnValue({ - initialSessionDuration: 25 * 60 * 1000, - shouldStartBreakAfterSession: false, - shouldAutoStartNextSession: false, - getBreakDuration: () => null, - }); - - const result: any[] = []; - effects.breakComplete$.subscribe({ - next: (action) => result.push(action), - complete: () => { - expect(result.length).toBe(0); + effects.autoStartSessionOnBreakComplete$.pipe(take(1)).subscribe((action) => { + expect(action).toEqual(actions.startFocusSession({ duration: 25 * 60 * 1000 })); done(); - }, + }); + }); + + it('should NOT dispatch startFocusSession when shouldAutoStartNextSession is false', (done) => { + actions$ = of(actions.completeBreak({ pausedTaskId: null })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Countdown); + store.refreshState(); + + strategyFactoryMock.getStrategy.and.returnValue({ + initialSessionDuration: 25 * 60 * 1000, + shouldStartBreakAfterSession: false, + shouldAutoStartNextSession: false, + getBreakDuration: () => null, + }); + + effects.autoStartSessionOnBreakComplete$ + .pipe(toArray()) + .subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); + done(); + }); }); }); - 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(); + describe('resumeTrackingOnBreakComplete$', () => { + it('should dispatch setCurrentTask when pausedTaskId is provided', (done) => { + const pausedTaskId = 'test-paused-task-id'; + actions$ = of(actions.completeBreak({ pausedTaskId })); - strategyFactoryMock.getStrategy.and.returnValue({ - initialSessionDuration: 25 * 60 * 1000, - shouldStartBreakAfterSession: false, - shouldAutoStartNextSession: false, - getBreakDuration: () => null, + effects.resumeTrackingOnBreakComplete$.pipe(take(1)).subscribe((action) => { + expect(action).toEqual(setCurrentTask({ id: pausedTaskId })); + done(); + }); }); - effects.breakComplete$.pipe(take(1)).subscribe((action) => { - expect(action).toEqual(setCurrentTask({ id: pausedTaskId })); - done(); + it('should NOT dispatch setCurrentTask when pausedTaskId is null', (done) => { + actions$ = of(actions.completeBreak({ pausedTaskId: null })); + + effects.resumeTrackingOnBreakComplete$.pipe(toArray()).subscribe((actionsArr) => { + expect(actionsArr.length).toBe(0); + done(); + }); + }); + }); + + describe('combined behavior', () => { + it('should dispatch setCurrentTask from resumeTrackingOnBreakComplete$ when pausedTaskId exists', (done) => { + const pausedTaskId = 'test-paused-task-id'; + actions$ = of(actions.completeBreak({ pausedTaskId })); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.refreshState(); + + // Test resumeTrackingOnBreakComplete$ independently + effects.resumeTrackingOnBreakComplete$.pipe(take(1)).subscribe((action) => { + expect(action).toEqual(setCurrentTask({ id: pausedTaskId })); + done(); + }); }); }); }); @@ -1339,7 +1459,7 @@ describe('FocusModeEffects', () => { }); }); - describe('pauseTrackingDuringBreak', () => { + describe('pauseTrackingDuringBreak (autoStartBreakOnSessionComplete$)', () => { it('should dispatch unsetCurrentTask when break starts and isPauseTrackingDuringBreak is true', (done) => { actions$ = of(actions.completeFocusSession({ isManual: false })); store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); @@ -1352,7 +1472,7 @@ describe('FocusModeEffects', () => { currentTaskId$.next('task-123'); store.refreshState(); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { + effects.autoStartBreakOnSessionComplete$.pipe(toArray()).subscribe((actionsArr) => { const unsetAction = actionsArr.find((a) => a.type === '[Task] UnsetCurrentTask'); expect(unsetAction).toBeDefined(); done(); @@ -1371,11 +1491,162 @@ describe('FocusModeEffects', () => { currentTaskId$.next('task-123'); store.refreshState(); - effects.sessionComplete$.pipe(toArray()).subscribe((actionsArr) => { + effects.autoStartBreakOnSessionComplete$.pipe(toArray()).subscribe((actionsArr) => { const unsetAction = actionsArr.find((a) => a.type === '[Task] UnsetCurrentTask'); expect(unsetAction).toBeUndefined(); done(); }); }); }); + + describe('detectSessionCompletion$', () => { + it('should dispatch completeFocusSession when timer completes (elapsed >= duration)', (done) => { + // Setup timer that just completed + store.overrideSelector( + selectors.selectTimer, + createMockTimer({ + isRunning: false, + purpose: 'work', + duration: 25 * 60 * 1000, + elapsed: 25 * 60 * 1000, // Exactly at duration + }), + ); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.refreshState(); + + // Need to recreate effects after selector override + effects = TestBed.inject(FocusModeEffects); + + effects.detectSessionCompletion$.pipe(take(1)).subscribe((action) => { + expect(action).toEqual(actions.completeFocusSession({ isManual: false })); + done(); + }); + }); + + it('should NOT dispatch for Flowtime mode (timer runs indefinitely)', (done) => { + store.overrideSelector( + selectors.selectTimer, + createMockTimer({ + isRunning: false, + purpose: 'work', + duration: 25 * 60 * 1000, + elapsed: 25 * 60 * 1000, + }), + ); + store.overrideSelector(selectors.selectMode, FocusModeMode.Flowtime); + store.refreshState(); + + effects = TestBed.inject(FocusModeEffects); + + // Wait a bit to ensure no action is dispatched + setTimeout(() => { + done(); // If no emission occurred, test passes + }, 50); + }); + + it('should NOT dispatch when timer is still running', (done) => { + store.overrideSelector( + selectors.selectTimer, + createMockTimer({ + isRunning: true, // Still running + purpose: 'work', + duration: 25 * 60 * 1000, + elapsed: 25 * 60 * 1000, + }), + ); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.refreshState(); + + effects = TestBed.inject(FocusModeEffects); + + setTimeout(() => { + done(); + }, 50); + }); + + it('should NOT dispatch when elapsed < duration', (done) => { + store.overrideSelector( + selectors.selectTimer, + createMockTimer({ + isRunning: false, + purpose: 'work', + duration: 25 * 60 * 1000, + elapsed: 20 * 60 * 1000, // Not complete yet + }), + ); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.refreshState(); + + effects = TestBed.inject(FocusModeEffects); + + setTimeout(() => { + done(); + }, 50); + }); + + it('should NOT dispatch when purpose is break', (done) => { + store.overrideSelector( + selectors.selectTimer, + createMockTimer({ + isRunning: false, + purpose: 'break', // Not a work session + duration: 5 * 60 * 1000, + elapsed: 5 * 60 * 1000, + }), + ); + store.overrideSelector(selectors.selectMode, FocusModeMode.Pomodoro); + store.refreshState(); + + effects = TestBed.inject(FocusModeEffects); + + setTimeout(() => { + done(); + }, 50); + }); + }); + + describe('detectBreakTimeUp$', () => { + it('should call notification when break timer completes', (done) => { + store.overrideSelector( + selectors.selectTimer, + createMockTimer({ + isRunning: false, + purpose: 'break', + duration: 5 * 60 * 1000, + elapsed: 5 * 60 * 1000, + }), + ); + store.refreshState(); + + // Create new effects instance and spy on _notifyUser + effects = TestBed.inject(FocusModeEffects); + const notifyUserSpy = spyOn(effects as any, '_notifyUser'); + + effects.detectBreakTimeUp$.pipe(take(1)).subscribe(() => { + expect(notifyUserSpy).toHaveBeenCalled(); + done(); + }); + }); + + it('should NOT trigger while break timer is running (elapsed < duration)', (done) => { + store.overrideSelector( + selectors.selectTimer, + createMockTimer({ + isRunning: true, + purpose: 'break', + duration: 5 * 60 * 1000, + elapsed: 3 * 60 * 1000, // Not complete + }), + ); + store.refreshState(); + + effects = TestBed.inject(FocusModeEffects); + const notifyUserSpy = spyOn(effects as any, '_notifyUser'); + + setTimeout(() => { + expect(notifyUserSpy).not.toHaveBeenCalled(); + done(); + }, 50); + }); + }); }); diff --git a/src/app/features/focus-mode/store/focus-mode.effects.ts b/src/app/features/focus-mode/store/focus-mode.effects.ts index 646c601c3..a813d170f 100644 --- a/src/app/features/focus-mode/store/focus-mode.effects.ts +++ b/src/app/features/focus-mode/store/focus-mode.effects.ts @@ -252,8 +252,36 @@ export class FocusModeEffects { { dispatch: false }, ); - // Handle session completion - sessionComplete$ = createEffect(() => + // Session completion effects - split into separate concerns for better maintainability + + // Effect 1: Increment cycle for Pomodoro mode + incrementCycleOnSessionComplete$ = createEffect(() => + this.actions$.pipe( + ofType(actions.completeFocusSession), + withLatestFrom(this.store.select(selectors.selectMode)), + filter(([_, mode]) => mode === FocusModeMode.Pomodoro), + map(() => actions.incrementCycle()), + ), + ); + + // Effect 2: Stop tracking on manual session end (bug #5875 fix) + stopTrackingOnManualEnd$ = createEffect(() => + this.actions$.pipe( + ofType(actions.completeFocusSession), + withLatestFrom( + this.store.select(selectFocusModeConfig), + this.taskService.currentTaskId$, + ), + filter( + ([action, config, taskId]) => + !!action.isManual && !!config?.isSyncSessionWithTracking && !!taskId, + ), + map(() => unsetCurrentTask()), + ), + ); + + // Effect 3: Auto-start break after session completion + autoStartBreakOnSessionComplete$ = createEffect(() => this.actions$.pipe( ofType(actions.completeFocusSession), withLatestFrom( @@ -262,93 +290,93 @@ export class FocusModeEffects { this.store.select(selectFocusModeConfig), this.taskService.currentTaskId$, ), - switchMap(([action, mode, cycle, focusModeConfig, currentTaskId]) => { + filter(([_, mode, __, config]) => { const strategy = this.strategyFactory.getStrategy(mode); - const actionsToDispatch: any[] = []; + return strategy.shouldStartBreakAfterSession && !config?.isManualBreakStart; + }), + switchMap(([_, mode, cycle, config, currentTaskId]) => { + const strategy = this.strategyFactory.getStrategy(mode); + const breakInfo = strategy.getBreakDuration(cycle); + const shouldPauseTracking = config?.isPauseTrackingDuringBreak && currentTaskId; + const actionsArr: any[] = []; - // Show notification (sound + window focus) - this._notifyUser(); - - // For Pomodoro mode, always increment cycle after session completion - if (mode === FocusModeMode.Pomodoro) { - actionsToDispatch.push(actions.incrementCycle()); + // Pause tracking during break if configured + if (shouldPauseTracking) { + actionsArr.push(unsetCurrentTask()); } - // Stop time tracking when session is manually ended and sync is enabled - // This fixes bug #5875: "End Session" button should stop tracking - const shouldStopTrackingOnManualEnd = - action.isManual && focusModeConfig?.isSyncSessionWithTracking && currentTaskId; - if (shouldStopTrackingOnManualEnd) { - actionsToDispatch.push(unsetCurrentTask()); + // Start break with appropriate duration + if (breakInfo) { + actionsArr.push( + actions.startBreak({ + duration: breakInfo.duration, + isLongBreak: breakInfo.isLong, + pausedTaskId: shouldPauseTracking ? currentTaskId : undefined, + }), + ); + } else { + // Fallback if no break info + actionsArr.push( + actions.startBreak({ + pausedTaskId: shouldPauseTracking ? currentTaskId : undefined, + }), + ); } - // Check if we should start a break after session completion - // Skip if manual break start is enabled (user must click "Start Break") - const shouldAutoStartBreak = - strategy.shouldStartBreakAfterSession && !focusModeConfig?.isManualBreakStart; - if (shouldAutoStartBreak) { - // Pause task tracking during break if enabled - const shouldPauseTracking = - focusModeConfig?.isPauseTrackingDuringBreak && currentTaskId; - if (shouldPauseTracking) { - actionsToDispatch.push(unsetCurrentTask()); - } - - // Get break duration from strategy - const breakInfo = strategy.getBreakDuration(cycle); - if (breakInfo) { - actionsToDispatch.push( - actions.startBreak({ - duration: breakInfo.duration, - isLongBreak: breakInfo.isLong, - pausedTaskId: shouldPauseTracking ? currentTaskId : undefined, - }), - ); - } else { - // Fallback if no break info (shouldn't happen for Pomodoro) - actionsToDispatch.push( - actions.startBreak({ - pausedTaskId: shouldPauseTracking ? currentTaskId : undefined, - }), - ); - } - } - - return actionsToDispatch.length > 0 ? of(...actionsToDispatch) : EMPTY; + return of(...actionsArr); }), ), ); - // Handle break completion + // Effect 4: Notification side effect (non-dispatching) + notifyOnSessionComplete$ = createEffect( + () => + this.actions$.pipe( + ofType(actions.completeFocusSession), + tap(() => this._notifyUser()), + ), + { dispatch: false }, + ); + + // Break completion effects - split into separate concerns for better maintainability // Note: pausedTaskId is passed in action payload to avoid race condition // (reducer clears pausedTaskId before effect reads state) - breakComplete$ = createEffect(() => + + // Effect 1: Resume tracking after break + resumeTrackingOnBreakComplete$ = createEffect(() => + this.actions$.pipe( + ofType(actions.completeBreak), + filter((action) => !!action.pausedTaskId), + map((action) => setCurrentTask({ id: action.pausedTaskId! })), + ), + ); + + // Effect 2: Auto-start next session after break + autoStartSessionOnBreakComplete$ = createEffect(() => this.actions$.pipe( ofType(actions.completeBreak), withLatestFrom(this.store.select(selectors.selectMode)), - switchMap(([action, mode]) => { + filter(([_, mode]) => { const strategy = this.strategyFactory.getStrategy(mode); - const actionsToDispatch: any[] = []; - - // Show notification (sound + window focus) - this._notifyUser(); - - // Resume task tracking if we paused it during break - if (action.pausedTaskId) { - actionsToDispatch.push(setCurrentTask({ id: action.pausedTaskId })); - } - - // Auto-start next session if configured - if (strategy.shouldAutoStartNextSession) { - const duration = strategy.initialSessionDuration; - actionsToDispatch.push(actions.startFocusSession({ duration })); - } - - return actionsToDispatch.length > 0 ? of(...actionsToDispatch) : EMPTY; + return strategy.shouldAutoStartNextSession; + }), + map(([_, mode]) => { + const strategy = this.strategyFactory.getStrategy(mode); + return actions.startFocusSession({ duration: strategy.initialSessionDuration }); }), ), ); + // Effect 3: Notification side effect (non-dispatching) + notifyOnBreakComplete$ = createEffect( + () => + this.actions$.pipe( + ofType(actions.completeBreak), + tap(() => this._notifyUser()), + ), + { dispatch: false }, + ); + // Handle skip break // Note: pausedTaskId is passed in action payload to avoid race condition skipBreak$ = createEffect(() =>