Merge branch 'task/carefully-review-all-changes-made-today-800ea4'

This commit is contained in:
johannesjo 2026-05-16 14:52:58 +02:00
commit fbc7159b85
2 changed files with 428 additions and 66 deletions

View file

@ -1,6 +1,6 @@
import { TestBed, fakeAsync, tick, flush } from '@angular/core/testing';
import { TestBed, fakeAsync, tick, flush, flushMicrotasks } from '@angular/core/testing';
import { provideMockActions } from '@ngrx/effects/testing';
import { provideMockStore } from '@ngrx/store/testing';
import { MockStore, provideMockStore } from '@ngrx/store/testing';
import { Subject, of, Subscription } from 'rxjs';
import { TimeBlockSyncEffects, COALESCE_MS } from './time-block-sync.effects';
import { TaskSharedActions } from '../../../root-store/meta/task-shared.actions';
@ -11,14 +11,26 @@ import { SnackService } from '../../../core/snack/snack.service';
import { TimeBlockDeleteSidecarService } from './time-block-delete-sidecar.service';
import { LOCAL_ACTIONS } from '../../../util/local-actions.token';
import { selectEnabledIssueProviders } from '../../issue/store/issue-provider.selectors';
import { DEFAULT_TASK, Task } from '../../tasks/task.model';
import { DEFAULT_TASK, Task, TaskWithDueTime } from '../../tasks/task.model';
import { selectAllTasksWithDueTimeSorted } from '../../tasks/store/task.selectors';
import { IssueProviderActions } from '../../issue/store/issue-provider.actions';
interface TestProvider {
id: string;
issueProviderKey: string;
pluginConfig: Record<string, unknown>;
}
describe('TimeBlockSyncEffects', () => {
let effects: TimeBlockSyncEffects;
let actions$: Subject<any>;
let sub: Subscription;
let upsertEventSpy: jasmine.Spy;
let deleteEventSpy: jasmine.Spy;
let getByIdOnce$Spy: jasmine.Spy;
let store: MockStore;
let provider: TestProvider;
let bulkDeleteSidecarIds: string[];
const createTask = (id: string, partial: Partial<Task> = {}): Task => ({
...DEFAULT_TASK,
@ -34,22 +46,27 @@ describe('TimeBlockSyncEffects', () => {
beforeEach(() => {
actions$ = new Subject<any>();
upsertEventSpy = jasmine.createSpy('upsertEvent').and.resolveTo(undefined);
deleteEventSpy = jasmine.createSpy('deleteEvent').and.resolveTo(undefined);
getByIdOnce$Spy = jasmine
.createSpy('getByIdOnce$')
.and.callFake((id: string) => of(createTask(id)));
const provider = {
provider = {
id: 'ip-1',
issueProviderKey: 'plugin:google-calendar',
pluginConfig: { isAutoTimeBlock: true },
};
bulkDeleteSidecarIds = [];
TestBed.configureTestingModule({
providers: [
TimeBlockSyncEffects,
provideMockActions(() => actions$),
provideMockStore({
selectors: [{ selector: selectEnabledIssueProviders, value: [provider] }],
selectors: [
{ selector: selectEnabledIssueProviders, value: [provider] },
{ selector: selectAllTasksWithDueTimeSorted, value: [] },
],
}),
{ provide: TaskService, useValue: { getByIdOnce$: getByIdOnce$Spy } },
{
@ -60,7 +77,7 @@ describe('TimeBlockSyncEffects', () => {
getHeaders: () => ({}),
timeBlock: {
upsertEvent: upsertEventSpy,
deleteEvent: jasmine.createSpy('deleteEvent').and.resolveTo(undefined),
deleteEvent: deleteEventSpy,
},
},
allowPrivateNetwork: false,
@ -77,13 +94,16 @@ describe('TimeBlockSyncEffects', () => {
},
{
provide: TimeBlockDeleteSidecarService,
useValue: { consume: () => [] },
useValue: {
consume: () => bulkDeleteSidecarIds,
},
},
{ provide: LOCAL_ACTIONS, useValue: actions$ },
],
});
effects = TestBed.inject(TimeBlockSyncEffects);
store = TestBed.inject(MockStore);
sub = effects.upsertOnTaskChange$.subscribe();
});
@ -149,6 +169,20 @@ describe('TimeBlockSyncEffects', () => {
flush();
}));
it('does not debounce or replay task changes when auto time-blocking is disabled', fakeAsync(() => {
provider.pluginConfig = { isAutoTimeBlock: false };
actions$.next(
TaskSharedActions.updateTask({ task: { id: 'task-1', changes: { title: 'X' } } }),
);
provider.pluginConfig = { isAutoTimeBlock: true };
tick(COALESCE_MS);
expect(getByIdOnce$Spy).not.toHaveBeenCalled();
expect(upsertEventSpy).not.toHaveBeenCalled();
flush();
}));
it('skips the upsert when the task has no dueWithTime after debounce', fakeAsync(() => {
getByIdOnce$Spy.and.callFake((id: string) =>
of(createTask(id, { dueWithTime: null })),
@ -188,14 +222,22 @@ describe('TimeBlockSyncEffects', () => {
flush();
}));
it('a late change for the same task supersedes an in-flight upsert without losing the final state', fakeAsync(() => {
it('serializes a late change behind an in-flight upsert and writes the final state last', fakeAsync(() => {
let currentTitle = 'first';
let resolveFirstUpsert!: () => void;
let upsertCallNr = 0;
getByIdOnce$Spy.and.callFake((id: string) =>
of(createTask(id, { title: currentTitle })),
);
// First upsert never resolves → it is still in flight when the next
// settled change arrives and switchMap supersedes it.
upsertEventSpy.and.returnValue(new Promise<void>(() => {}));
upsertEventSpy.and.callFake(() => {
upsertCallNr++;
if (upsertCallNr === 1) {
return new Promise<void>((resolve) => {
resolveFirstUpsert = resolve;
});
}
return Promise.resolve();
});
actions$.next(
TaskSharedActions.updateTask({
@ -214,10 +256,195 @@ describe('TimeBlockSyncEffects', () => {
);
tick(COALESCE_MS);
// The in-flight write was superseded; the final write reflects the
// latest settled state rather than being dropped.
expect(upsertEventSpy).toHaveBeenCalledTimes(1);
resolveFirstUpsert();
flushMicrotasks();
expect(upsertEventSpy).toHaveBeenCalledTimes(2);
expect(upsertEventSpy.calls.mostRecent().args[1].title).toBe('second');
flush();
}));
it('runs a queued delete after an in-flight upsert so unschedule wins', fakeAsync(() => {
const deleteSub = effects.deleteOnUnschedule$.subscribe();
let resolveFirstUpsert!: () => void;
upsertEventSpy.and.returnValue(
new Promise<void>((resolve) => {
resolveFirstUpsert = resolve;
}),
);
actions$.next(
TaskSharedActions.updateTask({
task: { id: 'task-1', changes: { title: 'first' } },
}),
);
tick(COALESCE_MS);
expect(upsertEventSpy).toHaveBeenCalledTimes(1);
actions$.next(TaskSharedActions.unscheduleTask({ id: 'task-1' }));
expect(deleteEventSpy).not.toHaveBeenCalled();
resolveFirstUpsert();
flushMicrotasks();
expect(deleteEventSpy).toHaveBeenCalledTimes(1);
expect(deleteEventSpy.calls.mostRecent().args[0]).toBe('task-1');
deleteSub.unsubscribe();
flush();
}));
it('keeps a queued delete when a later field update is no-op because the task is unscheduled', fakeAsync(() => {
const deleteSub = effects.deleteOnUnschedule$.subscribe();
let currentDueWithTime: number | null = new Date('2026-05-14T15:00:00Z').getTime();
let resolveFirstUpsert!: () => void;
getByIdOnce$Spy.and.callFake((id: string) =>
of(createTask(id, { dueWithTime: currentDueWithTime })),
);
upsertEventSpy.and.returnValue(
new Promise<void>((resolve) => {
resolveFirstUpsert = resolve;
}),
);
actions$.next(
TaskSharedActions.updateTask({
task: { id: 'task-1', changes: { title: 'first' } },
}),
);
tick(COALESCE_MS);
expect(upsertEventSpy).toHaveBeenCalledTimes(1);
currentDueWithTime = null;
actions$.next(TaskSharedActions.unscheduleTask({ id: 'task-1' }));
actions$.next(
TaskSharedActions.updateTask({
task: { id: 'task-1', changes: { title: 'after unschedule' } },
}),
);
tick(COALESCE_MS);
expect(deleteEventSpy).not.toHaveBeenCalled();
resolveFirstUpsert();
flushMicrotasks();
expect(deleteEventSpy).toHaveBeenCalledTimes(1);
expect(upsertEventSpy).toHaveBeenCalledTimes(1);
deleteSub.unsubscribe();
flush();
}));
it('queues backfill upserts so a later unschedule delete still wins', fakeAsync(() => {
const backfillSub = effects.backfillOnAutoTimeBlockEnabled$.subscribe();
const deleteSub = effects.deleteOnUnschedule$.subscribe();
const oneHourMs = 60 * 60 * 1000;
const dueWithTime = Date.now() + oneHourMs;
let currentDueWithTime: number | null = dueWithTime;
let resolveBackfillUpsert!: () => void;
store.overrideSelector(selectAllTasksWithDueTimeSorted, [
createTask('task-1', { dueWithTime }) as TaskWithDueTime,
]);
store.refreshState();
getByIdOnce$Spy.and.callFake((id: string) =>
of(createTask(id, { dueWithTime: currentDueWithTime })),
);
upsertEventSpy.and.returnValue(
new Promise<void>((resolve) => {
resolveBackfillUpsert = resolve;
}),
);
actions$.next(
IssueProviderActions.updateIssueProvider({
issueProvider: {
id: 'ip-1',
changes: { pluginConfig: { isAutoTimeBlock: true } },
},
}),
);
expect(upsertEventSpy).toHaveBeenCalledTimes(1);
currentDueWithTime = null;
actions$.next(TaskSharedActions.unscheduleTask({ id: 'task-1' }));
expect(deleteEventSpy).not.toHaveBeenCalled();
resolveBackfillUpsert();
flushMicrotasks();
expect(deleteEventSpy).toHaveBeenCalledTimes(1);
backfillSub.unsubscribe();
deleteSub.unsubscribe();
flush();
}));
it('uses the provider config captured when a queued delete is observed', fakeAsync(() => {
const deleteSub = effects.deleteOnUnschedule$.subscribe();
provider.pluginConfig = {
isAutoTimeBlock: true,
timeBlockCalendarId: 'old-calendar',
};
let resolveFirstUpsert!: () => void;
upsertEventSpy.and.returnValue(
new Promise<void>((resolve) => {
resolveFirstUpsert = resolve;
}),
);
actions$.next(
TaskSharedActions.updateTask({
task: { id: 'task-1', changes: { title: 'first' } },
}),
);
tick(COALESCE_MS);
expect(upsertEventSpy).toHaveBeenCalledTimes(1);
actions$.next(TaskSharedActions.unscheduleTask({ id: 'task-1' }));
provider.pluginConfig = {
isAutoTimeBlock: false,
timeBlockCalendarId: 'new-calendar',
};
resolveFirstUpsert();
flushMicrotasks();
expect(deleteEventSpy).toHaveBeenCalledTimes(1);
expect(deleteEventSpy.calls.mostRecent().args[1].timeBlockCalendarId).toBe(
'old-calendar',
);
deleteSub.unsubscribe();
flush();
}));
it('caps bulk-delete HTTP fan-out so it does not burst rate limits', fakeAsync(() => {
const bulkSub = effects.deleteOnBulkTaskDelete$.subscribe();
const taskIds = ['t-1', 't-2', 't-3', 't-4', 't-5'];
bulkDeleteSidecarIds = [...taskIds];
const resolvers: Array<() => void> = [];
deleteEventSpy.and.callFake(
() =>
new Promise<void>((resolve) => {
resolvers.push(resolve);
}),
);
actions$.next(TaskSharedActions.deleteTasks({ taskIds }));
// At most MAX_PARALLEL_TIME_BLOCK_HTTP (=3) HTTP calls should be in flight.
expect(deleteEventSpy).toHaveBeenCalledTimes(3);
resolvers[0]();
flushMicrotasks();
expect(deleteEventSpy).toHaveBeenCalledTimes(4);
resolvers[1]();
flushMicrotasks();
expect(deleteEventSpy).toHaveBeenCalledTimes(5);
resolvers.slice(2).forEach((r) => r());
flushMicrotasks();
expect(deleteEventSpy).toHaveBeenCalledTimes(5);
bulkSub.unsubscribe();
flush();
}));
});

View file

@ -11,7 +11,6 @@ import {
groupBy,
map,
mergeMap,
switchMap,
tap,
} from 'rxjs/operators';
import { LOCAL_ACTIONS } from '../../../util/local-actions.token';
@ -40,6 +39,24 @@ interface TimeBlockContext {
http: PluginHttp;
}
type TimeBlockOperation = 'upsert' | 'upsertIfScheduled' | 'delete';
interface TimeBlockQueueRequest {
taskId: string;
operation: TimeBlockOperation;
}
interface TimeBlockQueuedOperation {
type: TimeBlockOperation;
ctx?: TimeBlockContext;
}
interface TimeBlockTaskQueue {
isRunning: boolean;
pending: TimeBlockQueuedOperation | null;
resolvers: Array<() => void>;
}
/**
* Time-block writes for the same task are coalesced over this window. A single
* user edit dispatches several actions (e.g. applyShortSyntax + updateTask);
@ -48,6 +65,14 @@ interface TimeBlockContext {
*/
export const COALESCE_MS = 1000;
/**
* Cap on parallel HTTP writes across tasks. Each task's queue is already
* serialized internally; this limits cross-task fan-out (bulk delete, backfill)
* to stay under Google Calendar's per-user QPS while still being faster than
* strictly sequential.
*/
const MAX_PARALLEL_TIME_BLOCK_HTTP = 3;
@Injectable()
export class TimeBlockSyncEffects {
private readonly _actions$ = inject(LOCAL_ACTIONS);
@ -58,6 +83,7 @@ export class TimeBlockSyncEffects {
private readonly _snackService = inject(SnackService);
private readonly _deletesSidecar = inject(TimeBlockDeleteSidecarService);
private readonly _backfilledProviderIds = new Set<string>();
private readonly _taskWriteQueues = new Map<string, TimeBlockTaskQueue>();
/**
* When a task is scheduled, rescheduled, or a synced field (title,
@ -79,46 +105,42 @@ export class TimeBlockSyncEffects {
TaskSharedActions.applyShortSyntax,
TaskSharedActions.updateTask,
),
map((action): string | null => {
map((action): TimeBlockQueueRequest | null => {
if (action.type === TaskSharedActions.applyShortSyntax.type) {
const a = action as ReturnType<typeof TaskSharedActions.applyShortSyntax>;
return a.schedulingInfo?.dueWithTime ? a.task.id : null;
return a.schedulingInfo?.dueWithTime
? { taskId: a.task.id, operation: 'upsert' }
: null;
}
if (action.type === TaskSharedActions.updateTask.type) {
const a = action as ReturnType<typeof TaskSharedActions.updateTask>;
const changes = a.task.changes;
const isRelevant =
'title' in changes || 'timeEstimate' in changes || 'isDone' in changes;
return isRelevant ? (a.task.id as string) : null;
return isRelevant
? { taskId: a.task.id as string, operation: 'upsertIfScheduled' }
: null;
}
const a = action as ReturnType<typeof TaskSharedActions.scheduleTaskWithTime>;
return a.task.id;
return { taskId: a.task.id, operation: 'upsert' };
}),
filter((taskId): taskId is string => taskId !== null),
filter((request): request is TimeBlockQueueRequest => request !== null),
concatMap((request) =>
this._hasTimeBlockContext$().pipe(
filter(Boolean),
map(() => request),
),
),
// Coalesce rapid changes to the same task; idle groups auto-complete.
groupBy((taskId) => taskId, {
groupBy((request) => request.taskId, {
duration: (g) => g.pipe(debounceTime(COALESCE_MS * 5)),
}),
mergeMap((taskId$) =>
taskId$.pipe(
mergeMap((request$) =>
request$.pipe(
debounceTime(COALESCE_MS),
switchMap((taskId) =>
this._withTimeBlockContext$((ctx) =>
// Re-read the latest task after the debounce: the write
// reflects the settled state, trusting the store rather than
// any single triggering action's payload.
this._taskService
.getByIdOnce$(taskId)
.pipe(
concatMap((task) =>
task?.dueWithTime
? from(this._upsertEvent(ctx, task, task.dueWithTime)).pipe(
catchError((err) => this._handleError(err)),
)
: EMPTY,
),
),
),
tap(
({ taskId, operation }) =>
void this._queueTimeBlockOperation(taskId, { type: operation }),
),
),
),
@ -143,13 +165,7 @@ export class TimeBlockSyncEffects {
return a.task.dueWithTime ? a.task.id : null;
}),
filter((taskId): taskId is string => taskId !== null),
concatMap((taskId) =>
this._withTimeBlockContext$((ctx) =>
from(
ctx.definition.timeBlock!.deleteEvent(taskId, ctx.config, ctx.http),
).pipe(catchError((err) => this._handleError(err))),
),
),
concatMap((taskId) => this._queueDeleteTimeBlock$(taskId)),
),
{ dispatch: false },
);
@ -162,13 +178,7 @@ export class TimeBlockSyncEffects {
this._actions$.pipe(
ofType(TaskSharedActions.deleteTask),
filter(({ task }) => !!task.dueWithTime),
concatMap(({ task }) =>
this._withTimeBlockContext$((ctx) =>
from(
ctx.definition.timeBlock!.deleteEvent(task.id, ctx.config, ctx.http),
).pipe(catchError((err) => this._handleError(err))),
),
),
concatMap(({ task }) => this._queueDeleteTimeBlock$(task.id)),
),
{ dispatch: false },
);
@ -184,15 +194,7 @@ export class TimeBlockSyncEffects {
concatMap(() => {
const taskIds = this._deletesSidecar.consume();
if (!taskIds.length) return EMPTY;
return this._withTimeBlockContext$((ctx) =>
from(taskIds).pipe(
concatMap((taskId) =>
from(
ctx.definition.timeBlock!.deleteEvent(taskId, ctx.config, ctx.http),
).pipe(catchError((err) => this._handleError(err))),
),
),
);
return this._queueDeleteTimeBlocks$(taskIds);
}),
),
{ dispatch: false },
@ -237,10 +239,13 @@ export class TimeBlockSyncEffects {
return from(tasksInRange).pipe(
mergeMap(
(task) =>
from(this._upsertEvent(ctx, task, task.dueWithTime)).pipe(
catchError((err) => this._handleError(err)),
from(
this._queueTimeBlockOperation(task.id, {
type: 'upsertIfScheduled',
ctx,
}),
),
3,
MAX_PARALLEL_TIME_BLOCK_HTTP,
),
// Mark as backfilled only after all upserts complete successfully
tap({
@ -257,6 +262,136 @@ export class TimeBlockSyncEffects {
// --- Helpers ---
private _hasTimeBlockContext$(filterProviderId?: string): Observable<boolean> {
return this._store.select(selectEnabledIssueProviders).pipe(
first(),
map((providers) =>
providers.some(
(p): p is IssueProviderPluginType =>
(!filterProviderId || p.id === filterProviderId) &&
isPluginIssueProvider(p.issueProviderKey) &&
!!(p as IssueProviderPluginType).pluginConfig?.['isAutoTimeBlock'] &&
!!this._pluginRegistry.getProvider(p.issueProviderKey)?.definition.timeBlock,
),
),
);
}
private _queueTimeBlockOperation(
taskId: string,
operation: TimeBlockQueuedOperation,
): Promise<void> {
let queue = this._taskWriteQueues.get(taskId);
if (!queue) {
queue = { isRunning: false, pending: null, resolvers: [] };
this._taskWriteQueues.set(taskId, queue);
}
const done = new Promise<void>((resolve) => queue.resolvers.push(resolve));
if (!(queue.pending?.type === 'delete' && operation.type === 'upsertIfScheduled')) {
queue.pending = operation;
}
if (queue.isRunning) {
return done;
}
queue.isRunning = true;
void this._drainTimeBlockQueue(taskId, queue);
return done;
}
private async _drainTimeBlockQueue(
taskId: string,
queue: TimeBlockTaskQueue,
): Promise<void> {
try {
while (queue.pending) {
const operation = queue.pending;
const resolvers = queue.resolvers;
queue.pending = null;
queue.resolvers = [];
await this._runQueuedTimeBlockOperation(taskId, operation);
resolvers.forEach((resolve) => resolve());
}
} finally {
queue.isRunning = false;
if (this._taskWriteQueues.get(taskId) === queue) {
this._taskWriteQueues.delete(taskId);
}
}
}
private _runQueuedTimeBlockOperation(
taskId: string,
operation: TimeBlockQueuedOperation,
): Promise<void> {
return new Promise((resolve) => {
const op$ =
operation.type === 'upsert' || operation.type === 'upsertIfScheduled'
? this._upsertLatestTaskOnce$(taskId, operation.ctx)
: this._deleteTimeBlockOnce$(taskId, operation.ctx);
op$.pipe(catchError((err) => this._handleError(err))).subscribe({
complete: resolve,
error: () => resolve(),
});
});
}
private _upsertLatestTaskOnce$(
taskId: string,
queuedCtx?: TimeBlockContext,
): Observable<unknown> {
const runWithCtx = (ctx: TimeBlockContext): Observable<unknown> =>
// Re-read the latest task when the queued write actually starts. If
// edits arrived while a previous HTTP write was in flight, this makes
// the trailing write reflect the final settled state.
this._taskService
.getByIdOnce$(taskId)
.pipe(
concatMap((task) =>
task?.dueWithTime
? from(this._upsertEvent(ctx, task, task.dueWithTime)).pipe(
catchError((err) => this._handleError(err)),
)
: EMPTY,
),
);
return queuedCtx ? runWithCtx(queuedCtx) : this._withTimeBlockContext$(runWithCtx);
}
private _deleteTimeBlockOnce$(
taskId: string,
queuedCtx?: TimeBlockContext,
): Observable<unknown> {
const runWithCtx = (ctx: TimeBlockContext): Observable<unknown> =>
from(ctx.definition.timeBlock!.deleteEvent(taskId, ctx.config, ctx.http)).pipe(
catchError((err) => this._handleError(err)),
);
return queuedCtx ? runWithCtx(queuedCtx) : this._withTimeBlockContext$(runWithCtx);
}
private _queueDeleteTimeBlock$(taskId: string): Observable<unknown> {
return this._withTimeBlockContext$((ctx) => {
void this._queueTimeBlockOperation(taskId, { type: 'delete', ctx });
return EMPTY;
});
}
private _queueDeleteTimeBlocks$(taskIds: string[]): Observable<unknown> {
return this._withTimeBlockContext$((ctx) =>
from(taskIds).pipe(
mergeMap(
(taskId) =>
from(this._queueTimeBlockOperation(taskId, { type: 'delete', ctx })),
MAX_PARALLEL_TIME_BLOCK_HTTP,
),
),
);
}
/**
* Find an enabled plugin provider with timeBlock support and
* isAutoTimeBlock config, create an authenticated HTTP helper, and run the callback.
@ -288,7 +423,7 @@ export class TimeBlockSyncEffects {
return fn({
providerId: provider.id,
definition: registered.definition,
config: provider.pluginConfig,
config: { ...provider.pluginConfig },
http,
});
}),