From bfdac6d19a86e48c4776617cf00a023e94d05894 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Thu, 29 Jan 2026 18:18:24 +0100 Subject: [PATCH] refactor(op-log): merge LWWOperationFactory into ConflictResolutionService - Consolidate LWWOperationFactory methods (createLWWUpdateOp, mergeAndIncrementClocks) into ConflictResolutionService to reduce service count - Remove redundant caching in SuperSyncProvider (SyncCredentialStore already caches) - Remove _onBlurWhenNotTracking$ sync trigger from SyncTriggerService - Add documentation for dueDay/dueWithTime mutual exclusivity pattern - Change docker-compose postgres port to 55432 to avoid conflicts --- ARCHITECTURE-DECISIONS.md | 141 ++++++ docker-compose.yaml | 2 +- docs/ai/README.md | 78 ++++ .../dueDay-dueWithTime-mutual-exclusivity.md | 412 ++++++++++++++++++ docs/ai/today-tag-architecture.md | 85 ++-- src/app/features/tasks/task.model.ts | 22 + .../store/work-context.selectors.ts | 8 +- src/app/imex/sync/sync-trigger.service.ts | 24 +- .../super-sync/super-sync.spec.ts | 29 +- .../sync-providers/super-sync/super-sync.ts | 24 +- .../sync/conflict-resolution.service.ts | 78 +++- .../lww-operation-factory.service.spec.ts | 249 ----------- .../sync/lww-operation-factory.service.ts | 81 ---- .../stale-operation-resolver.service.spec.ts | 36 ++ .../sync/stale-operation-resolver.service.ts | 6 +- .../task-shared-scheduling.reducer.ts | 11 +- 16 files changed, 861 insertions(+), 425 deletions(-) create mode 100644 ARCHITECTURE-DECISIONS.md create mode 100644 docs/ai/README.md create mode 100644 docs/ai/dueDay-dueWithTime-mutual-exclusivity.md delete mode 100644 src/app/op-log/sync/lww-operation-factory.service.spec.ts delete mode 100644 src/app/op-log/sync/lww-operation-factory.service.ts diff --git a/ARCHITECTURE-DECISIONS.md b/ARCHITECTURE-DECISIONS.md new file mode 100644 index 0000000000..bc10a6eeba --- /dev/null +++ b/ARCHITECTURE-DECISIONS.md @@ -0,0 +1,141 @@ +# Architecture Decision Records + +This document tracks significant architectural decisions and patterns in the Super Productivity codebase. When making changes that affect these patterns, reference this document and update it if needed. + +## Active Patterns & Decisions + +### 1. dueDay/dueWithTime Mutual Exclusivity Pattern + +**Status**: ✅ Active (since commit `400ca8c1`, 2026-01-29) + +**Decision**: The `task.dueDay` and `task.dueWithTime` fields are mutually exclusive in new data. When setting `dueWithTime`, `dueDay` must be cleared (set to `undefined`). When reading, `dueWithTime` takes priority over `dueDay`. + +**Rationale**: + +- Prevents state inconsistency bugs where both fields had conflicting values +- Single source of truth for task scheduling +- Simpler state management + +**Implementation**: + +- **Writing**: Clear `dueDay` when setting `dueWithTime` (in meta-reducers) +- **Reading**: Check `dueWithTime` first; only check `dueDay` if `dueWithTime` is not set (in selectors) +- **Legacy Data**: Old data with both fields works via priority pattern (no migration needed) + +**Documentation**: [`docs/ai/dueDay-dueWithTime-mutual-exclusivity.md`](docs/ai/dueDay-dueWithTime-mutual-exclusivity.md) + +**Key Files**: + +- [`task.model.ts`](src/app/features/tasks/task.model.ts) - Field definitions with JSDoc +- [`task-shared-scheduling.reducer.ts`](src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts) - Write implementation +- [`work-context.selectors.ts`](src/app/features/work-context/store/work-context.selectors.ts) - Read pattern +- [`planner.selectors.ts`](src/app/features/planner/store/planner.selectors.ts) - Read pattern +- [`task.selectors.ts`](src/app/features/tasks/store/task.selectors.ts) - Read pattern + +**When to Update This Pattern**: + +- Adding new date/time scheduling fields +- Modifying task scheduling logic +- Working with task selectors that check due dates + +--- + +### 2. TODAY_TAG Virtual Tag Pattern + +**Status**: ✅ Active (established pattern) + +**Decision**: `TODAY_TAG` (ID: `'TODAY'`) is a **virtual tag** whose membership is determined by `task.dueWithTime` or `task.dueDay`, not by `task.tagIds`. The tag's `taskIds` field stores only the ordering of tasks, not membership. + +**Key Invariant**: `TODAY_TAG.id` must NEVER be added to `task.tagIds` + +**Rationale**: + +- Uniform move operations across all tags (virtual and regular) +- Single source of truth for "today" membership (date fields, not tagIds) +- Self-healing ordering (stale entries automatically filtered) +- Natural integration with planner (which uses date fields) + +**Documentation**: [`docs/ai/today-tag-architecture.md`](docs/ai/today-tag-architecture.md) + +**Related**: Uses the dueDay/dueWithTime mutual exclusivity pattern (Decision #1) + +**Key Files**: + +- [`tag.const.ts`](src/app/features/tag/tag.const.ts) - TODAY_TAG definition +- [`work-context.selectors.ts`](src/app/features/work-context/store/work-context.selectors.ts) - Membership computation +- [`task-shared-helpers.ts`](src/app/root-store/meta/task-shared-meta-reducers/task-shared-helpers.ts) - Invariant enforcement + +**When to Update This Pattern**: + +- Adding new virtual tags +- Modifying tag membership logic +- Working with today's task list + +--- + +## How to Use This Document + +### When Making Architectural Changes + +1. **Before implementing**: Check if your change affects any active pattern +2. **During implementation**: Follow the documented patterns +3. **After implementation**: Update this document if you've: + - Changed an existing pattern + - Added a new architectural pattern + - Made a decision that affects future development + +### When to Add a New Decision + +Add a new decision record when: + +- The decision affects multiple files/modules +- Future developers need to understand "why" not just "what" +- The pattern needs to be followed consistently across the codebase +- The decision prevents a specific class of bugs + +### Decision Record Template + +```markdown +### N. [Pattern/Decision Name] + +**Status**: ✅ Active | 🚧 Draft | ⚠️ Deprecated | ❌ Superseded + +**Decision**: [One-sentence summary of the decision] + +**Rationale**: + +- [Why was this decision made?] +- [What problems does it solve?] + +**Implementation**: + +- [How is it implemented?] +- [Key techniques or patterns used] + +**Documentation**: [Link to detailed docs] + +**Key Files**: [List of primary files implementing this pattern] + +**When to Update This Pattern**: [Scenarios when someone should review/update this] +``` + +--- + +## Related Documentation + +- [`docs/ai/`](docs/ai/) - AI/Developer architecture docs (detailed patterns) +- [`docs/sync-and-op-log/`](docs/sync-and-op-log/) - Operation log architecture +- [`docs/long-term-plans/`](docs/long-term-plans/) - Future architectural plans + +--- + +## Commit Reference + +When committing changes related to these patterns, reference this document and the specific decision: + +``` +feat(tasks): implement feature X + +Uses dueDay/dueWithTime mutual exclusivity pattern (ARCHITECTURE-DECISIONS.md #1) +See: docs/ai/dueDay-dueWithTime-mutual-exclusivity.md +``` diff --git a/docker-compose.yaml b/docker-compose.yaml index 7d435a0c2a..15a4b16929 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -8,7 +8,7 @@ services: POSTGRES_PASSWORD: superpassword POSTGRES_DB: supersync_db ports: - - '5432:5432' + - '55432:5432' volumes: - db_data:/var/lib/postgresql/data healthcheck: diff --git a/docs/ai/README.md b/docs/ai/README.md new file mode 100644 index 0000000000..c03ded6430 --- /dev/null +++ b/docs/ai/README.md @@ -0,0 +1,78 @@ +# AI/Developer Architecture Documentation + +This directory contains architectural documentation, design patterns, and guides specifically written for AI assistants and developers working on the Super Productivity codebase. + +## Core Patterns & Architecture + +### Task Scheduling & Date Management + +- **[dueDay/dueWithTime Mutual Exclusivity Pattern](dueDay-dueWithTime-mutual-exclusivity.md)** ⭐ CRITICAL + - Explains how `dueDay` and `dueWithTime` fields interact on tasks + - **Why it matters**: These fields are mutually exclusive; setting one clears the other + - **When to read**: Before working with task scheduling, planner, or date selectors + - **Related commit**: `400ca8c1` (2026-01-29) + +- **[TODAY_TAG Architecture](today-tag-architecture.md)** ⭐ CRITICAL + - Explains the virtual tag pattern for the TODAY_TAG + - **Why it matters**: TODAY_TAG behaves fundamentally differently from regular tags + - **When to read**: Before working with today's task list, planner, or tag operations + - **Related**: Uses the dueDay/dueWithTime mutual exclusivity pattern + +## Entity Management + +- **[Adding New Entity Type Checklist](adding-new-entity-type-checklist.md)** + - Step-by-step guide for adding new entity types to the app + - **When to use**: When adding a new feature that requires persistent state + +## Sync & Operation Log + +- **[File-Based OpLog Sync Implementation Plan](file-based-oplog-sync-implementation-plan.md)** + - Technical plan for file-based sync implementation + - **Related**: See also `docs/sync-and-op-log/` for comprehensive sync documentation + +## Plugin System + +- **[Issue Providers to Plugins Evaluation](issue-providers-to-plugins-evaluation.md)** + - Analysis of migrating issue providers to plugin architecture + +- **[Plugin UI Consistency Plan](plugin-ui-consistency-plan.md)** + - Design plan for consistent plugin UI/UX + +## Documentation Conventions + +### Document Types + +1. **Architecture Docs** - Explain fundamental patterns (e.g., virtual tags, mutual exclusivity) +2. **Implementation Plans** - Detailed technical plans for features +3. **Checklists** - Step-by-step guides for common tasks +4. **Evaluations** - Analysis of technical decisions + +### Criticality Markers + +- ⭐ **CRITICAL**: Must read before working in related areas +- 📋 **REFERENCE**: Useful reference material +- 📝 **DRAFT**: Work in progress, may be incomplete + +### When to Update + +- **After architectural changes**: Document new patterns immediately +- **When patterns emerge**: If you notice repeated code patterns, document them +- **When questions arise**: If developers ask the same question twice, document the answer +- **After major refactors**: Especially when behavior changes (like `400ca8c1`) + +## Related Documentation + +- [`docs/sync-and-op-log/`](../sync-and-op-log/) - Comprehensive operation log and sync documentation +- [`docs/wiki/`](../wiki/) - User-facing wiki documentation +- [`docs/long-term-plans/`](../long-term-plans/) - Future technical plans and proposals + +## Contributing + +When adding new documentation to this directory: + +1. **Use clear titles** that describe the pattern/feature +2. **Add a summary section** explaining what, why, and when to read +3. **Include code examples** showing correct and incorrect usage +4. **Link related files** using relative paths +5. **Update this README** with your new document +6. **Reference related commits** when documenting a specific change diff --git a/docs/ai/dueDay-dueWithTime-mutual-exclusivity.md b/docs/ai/dueDay-dueWithTime-mutual-exclusivity.md new file mode 100644 index 0000000000..a8a0861ce9 --- /dev/null +++ b/docs/ai/dueDay-dueWithTime-mutual-exclusivity.md @@ -0,0 +1,412 @@ +# dueDay/dueWithTime Mutual Exclusivity Pattern + +## Overview + +As of commit `400ca8c1` (2026-01-29), the `dueDay` and `dueWithTime` fields on [`Task`](../../src/app/features/tasks/task.model.ts) follow a **mutual exclusivity pattern**. These fields must NOT both be set simultaneously in new data. This document explains the pattern, its rationale, and implementation details. + +## The Pattern + +### Rule + +**When `dueWithTime` is set, `dueDay` MUST be `undefined` (or `null`).** + +```typescript +// CORRECT - Task scheduled with specific time +task.dueWithTime = 1706537400000; // Tomorrow at 9:00 AM +task.dueDay = undefined; // Cleared + +// CORRECT - Task scheduled without specific time (all-day) +task.dueDay = '2026-01-30'; +task.dueWithTime = undefined; + +// WRONG - Both fields set (legacy data only) +task.dueDay = '2026-01-30'; +task.dueWithTime = 1706537400000; // DO NOT create new data like this +``` + +### Priority/Precedence + +When reading task data, **`dueWithTime` takes priority over `dueDay`**: + +```typescript +// Determining if a task is "due today" +let isDueToday = false; +if (task.dueWithTime) { + // Check dueWithTime first (takes priority) + isDueToday = isToday(task.dueWithTime); + // DO NOT check dueDay if dueWithTime is set +} else if (task.dueDay === todayStr) { + // Only check dueDay if dueWithTime is not set + isDueToday = true; +} +``` + +## Rationale + +### The Problem This Solves + +**Prior to this change**, both fields could coexist with conflicting values, causing bugs: + +1. **Bug Scenario**: User moves task from "today" to "tomorrow 9am" + - Action: `scheduleTask` sets `dueWithTime = tomorrow 9am` + - **Old behavior**: Also set `dueDay = 'tomorrow'` + - **Problem**: If selector checked `dueDay` first, stale values could cause incorrect categorization + +2. **State Inconsistency**: Task could have: + ```typescript + dueDay = 'today' + dueWithTime = tomorrow 9am + ``` + Different selectors checking different fields would disagree on the task's due date. + +### Why Mutual Exclusivity? + +1. **Single Source of Truth**: No ambiguity about when a task is due +2. **Simpler State Management**: No need to keep two fields in sync +3. **Bug Prevention**: Eliminates entire class of state inconsistency bugs +4. **Clear Semantics**: + - `dueWithTime` = scheduled for specific time + - `dueDay` = scheduled for all-day (no specific time) + +## Implementation Details + +### Writing: Setting the Fields + +**File**: [`task-shared-scheduling.reducer.ts`](../../src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts) + +When scheduling a task with a specific time, `dueDay` is explicitly cleared: + +```typescript +const handleScheduleTaskWithTime = ( + state: AppDataComplete, + taskId: string, + dueWithTime: number, + // ... +): Update => { + return { + id: taskId, + changes: { + dueWithTime, + dueDay: undefined, // ← Mutual exclusivity: dueWithTime clears dueDay + remindAt, + }, + }; +}; +``` + +### Reading: Checking the Fields + +All selectors follow this pattern - **check `dueWithTime` first, only fall back to `dueDay` if `dueWithTime` is not set**: + +#### Example 1: Planner Selector + +**File**: [`planner.selectors.ts`](../../src/app/features/planner/store/planner.selectors.ts) + +```typescript +export const selectAllTasksDueToday = createSelector( + /*...*/ (taskState, todayStr) => { + // ... + for (const id of taskState.ids) { + const task = taskState.entities[id]; + if (!task) continue; + + // Check if task is due today + // Priority: dueWithTime takes precedence over dueDay (mutual exclusivity pattern) + let isDueToday = false; + if (task.dueWithTime) { + isDueToday = isToday(task.dueWithTime); + } else if (task.dueDay === todayStr) { + isDueToday = true; + } + + if (isDueToday) { + allDue.push(task); + } + } + }, +); +``` + +#### Example 2: Work Context Selector + +**File**: [`work-context.selectors.ts`](../../src/app/features/work-context/store/work-context.selectors.ts) + +```typescript +const computeOrderedTaskIdsForToday = (todayTag, taskEntities, todayStr) => { + const tasksForToday: string[] = []; + for (const taskId of Object.keys(taskEntities)) { + const task = taskEntities[taskId]; + if (task) { + // Check dueWithTime first (takes priority - mutual exclusivity) + if (task.dueWithTime) { + if (isToday(task.dueWithTime)) { + tasksForToday.push(taskId); + } + // If dueWithTime is set but not for today, skip (don't check dueDay) + } + // Fallback: check dueDay only if dueWithTime is not set + else if (task.dueDay === todayStr) { + tasksForToday.push(taskId); + } + } + } + // ... +}; +``` + +#### Example 3: Task Selector Helper + +**File**: [`task.selectors.ts`](../../src/app/features/tasks/store/task.selectors.ts) + +```typescript +// Helper to check if task is "in TODAY" via virtual tag pattern +// Priority: dueWithTime takes precedence over dueDay (mutual exclusivity) +const isInToday = (task: Task): boolean => { + if (task.dueWithTime) { + return isToday(task.dueWithTime); + } + return task.dueDay === todayStr; +}; +``` + +## Legacy Data Handling + +### The Challenge + +**Existing data may have both fields set** from before this pattern was introduced. This is unavoidable due to: + +1. Persisted local data +2. Synced data from other clients +3. Data from backups/archives + +### The Solution + +**All selectors implement the priority pattern** (`dueWithTime` first), ensuring correct behavior even with legacy data: + +- If **both fields are set**: `dueWithTime` determines the task's due date +- If **only `dueDay` is set**: Use `dueDay` (legacy all-day tasks) +- If **only `dueWithTime` is set**: Use `dueWithTime` (new data) + +### Migration Strategy + +**No data migration is performed**. Instead: + +1. **Graceful degradation**: Old data continues to work via priority pattern +2. **Natural migration**: As users interact with tasks, new operations will clear `dueDay` when setting `dueWithTime` +3. **No breaking changes**: Both fields remain in the model as optional + +## Testing + +The mutual exclusivity pattern is extensively tested: + +### Test File: [`task-shared-scheduling.reducer.spec.ts`](../../src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.spec.ts) + +```typescript +it('should set dueDay to undefined when scheduling for today (mutual exclusivity)', () => { + const now = Date.now(); + const testState = createStateWithExistingTasks(['task1'], [], [], []); + const action = createScheduleAction({}, now); + + metaReducer(testState, action); + expectStateUpdate( + expectTaskUpdate('task1', { dueWithTime: now, dueDay: undefined }), + action, + mockReducer, + testState, + ); +}); + +it('should set dueDay to undefined when scheduling for a different day (mutual exclusivity)', () => { + const testState = createStateWithExistingTasks(['task1'], [], [], ['task1']); + const tomorrowTimestamp = Date.now() + 24 * 60 * 60 * 1000; + const action = createScheduleAction({}, tomorrowTimestamp); + + metaReducer(testState, action); + expectStateUpdate( + expectTaskUpdate('task1', { + dueWithTime: tomorrowTimestamp, + dueDay: undefined, // ← Cleared + }), + action, + mockReducer, + testState, + ); +}); +``` + +### Test File: [`work-context.selectors.spec.ts`](../../src/app/features/work-context/store/work-context.selectors.spec.ts) + +```typescript +it('should INCLUDE task with dueWithTime for today even when dueDay is set to different date (dueWithTime takes priority)', () => { + const today = new Date(); + today.setHours(8, 0, 0, 0); + const todayTimestamp = today.getTime(); + + // This is a legacy data scenario - with mutual exclusivity, this shouldn't happen in new data + const taskWithBothFields = { + id: 'task1', + tagIds: [], + dueDay: '2000-01-01', // Legacy dueDay set to different date + dueWithTime: todayTimestamp, // dueWithTime for today takes priority + subTaskIds: [], + } as TaskCopy; + + const result = selectTodayTaskIds.projector(tagState, taskState); + expect(result).toEqual(['task1']); // dueWithTime takes priority - task IS in today +}); + +it('task scheduled for tomorrow via dialog should NOT appear in today (mutual exclusivity)', () => { + // This tests the mutual exclusivity pattern: + // 1. Task starts in "today" list (dueDay = today) + // 2. User schedules it for tomorrow via schedule dialog + // 3. After scheduling: dueWithTime is set, dueDay is cleared (undefined) + // 4. Task should NOT appear in today's list (dueWithTime is for tomorrow) + + const tomorrow = new Date(); + tomorrow.setDate(tomorrow.getDate() + 1); + tomorrow.setHours(9, 0, 0, 0); + const tomorrowTimestamp = tomorrow.getTime(); + + const taskScheduledForTomorrow = { + id: 'task1', + tagIds: [], + dueDay: undefined, // Mutual exclusivity: dueDay cleared when dueWithTime is set + dueWithTime: tomorrowTimestamp, // Scheduled for 9am tomorrow + subTaskIds: [], + } as TaskCopy; + + const result = selectTodayTaskIds.projector(tagState, taskState); + expect(result).toEqual([]); // Task should NOT appear in today +}); +``` + +## Code Patterns to Follow + +### ✅ DO: Check dueWithTime first + +```typescript +if (task.dueWithTime) { + // Handle time-specific scheduling + isDueToday = isToday(task.dueWithTime); +} else if (task.dueDay === todayStr) { + // Only check dueDay if dueWithTime is not set + isDueToday = true; +} +``` + +### ✅ DO: Clear dueDay when setting dueWithTime + +```typescript +updateTask({ + id: taskId, + changes: { + dueWithTime: timestamp, + dueDay: undefined, // Always clear when setting dueWithTime + }, +}); +``` + +### ❌ DON'T: Check dueDay first + +```typescript +// WRONG - Don't do this +if (task.dueDay === todayStr) { + isDueToday = true; +} else if (task.dueWithTime && isToday(task.dueWithTime)) { + isDueToday = true; +} +``` + +### ❌ DON'T: Set both fields + +```typescript +// WRONG - Never set both fields in new code +updateTask({ + id: taskId, + changes: { + dueWithTime: timestamp, + dueDay: getDbDateStr(timestamp), // DON'T DO THIS + }, +}); +``` + +### ❌ DON'T: Check both fields with OR + +```typescript +// WRONG - This violates priority pattern +const isDueByDay = task.dueDay === todayStr; +const isDueByTime = task.dueWithTime && isToday(task.dueWithTime); +if (isDueByDay || isDueByTime) { + // This can give wrong results with legacy data +} +``` + +## Related Patterns + +### TODAY_TAG Virtual Tag + +The mutual exclusivity pattern complements the **TODAY_TAG virtual tag pattern** where: + +- `TODAY_TAG.id` must NEVER be in `task.tagIds` +- Membership in "today" is determined by `dueDay` OR `dueWithTime` (via priority pattern) + +See: [`today-tag-architecture.md`](today-tag-architecture.md) + +### When to Use Each Field + +| Scenario | Use `dueWithTime` | Use `dueDay` | +| ------------------------------------ | ------------------- | ---------------------------- | +| Task scheduled for specific time | ✅ | ❌ (undefined) | +| Task scheduled for all-day (no time) | ❌ (undefined) | ✅ | +| Task not scheduled at all | ❌ (undefined) | ❌ (undefined) | +| Reading legacy data with both fields | ✅ (takes priority) | ignore if dueWithTime exists | + +## Key Files Modified + +The following files were changed as part of implementing this pattern in commit `400ca8c1`: + +| File | Change | +| ------------------------------------------------------------------------------------------------------------------------------------------ | ----------------------------------------- | +| [`task-shared-scheduling.reducer.ts`](../../src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts) | Clear `dueDay` when setting `dueWithTime` | +| [`planner.selectors.ts`](../../src/app/features/planner/store/planner.selectors.ts) | Check `dueWithTime` first, then `dueDay` | +| [`task.selectors.ts`](../../src/app/features/tasks/store/task.selectors.ts) | Check `dueWithTime` first, then `dueDay` | +| [`work-context.selectors.ts`](../../src/app/features/work-context/store/work-context.selectors.ts) | Check `dueWithTime` first, then `dueDay` | +| [`task-shared-scheduling.reducer.spec.ts`](../../src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.spec.ts) | Test mutual exclusivity on write | +| [`work-context.selectors.spec.ts`](../../src/app/features/work-context/store/work-context.selectors.spec.ts) | Test priority pattern on read | + +## Future Considerations + +### Potential Migration + +If needed in the future, a **data migration** could be implemented to clean up legacy data: + +```typescript +// Potential migration (not currently implemented) +function migrateDueDateFields(task: Task): Task { + if (task.dueWithTime && task.dueDay) { + // Clear dueDay if dueWithTime is set + return { ...task, dueDay: undefined }; + } + return task; +} +``` + +However, this is **not currently necessary** because the priority pattern handles legacy data correctly. + +### Adding New Date/Time Fields + +If new scheduling fields are added in the future: + +1. **Consider mutual exclusivity** with existing fields +2. **Document priority order** if multiple fields can express similar concepts +3. **Update all selectors** consistently with the priority pattern +4. **Add comprehensive tests** for legacy data scenarios + +## Summary + +- **Pattern**: `dueWithTime` and `dueDay` are mutually exclusive in new data +- **Priority**: `dueWithTime` takes precedence when reading data +- **Writing**: Clear `dueDay` when setting `dueWithTime` +- **Reading**: Check `dueWithTime` first, only check `dueDay` if `dueWithTime` is not set +- **Legacy**: Old data with both fields works via priority pattern +- **Testing**: Extensively tested in reducers and selectors diff --git a/docs/ai/today-tag-architecture.md b/docs/ai/today-tag-architecture.md index 0f4c9cce75..1db31fbe2e 100644 --- a/docs/ai/today-tag-architecture.md +++ b/docs/ai/today-tag-architecture.md @@ -8,25 +8,25 @@ TODAY_TAG is a **virtual tag** that behaves differently from regular tags. Under **TODAY_TAG (ID: `'TODAY'`) must NEVER be added to `task.tagIds`.** -Membership in TODAY_TAG is determined by `task.dueDay`, not by `task.tagIds`. The `TODAY_TAG.taskIds` field stores only the **ordering** of today's tasks, not membership. +Membership in TODAY_TAG is determined by **`task.dueWithTime` OR `task.dueDay`** (via [mutual exclusivity pattern](dueDay-dueWithTime-mutual-exclusivity.md)), not by `task.tagIds`. The `TODAY_TAG.taskIds` field stores only the **ordering** of today's tasks, not membership. ## Virtual Tag vs. Board-Style Pattern -| Aspect | TODAY_TAG (Virtual) | Regular Tags (Board-Style) | -| ----------------------- | ----------------------- | ----------------------------- | -| **Membership source** | `task.dueDay === today` | `task.tagIds.includes(tagId)` | -| **Stored on task** | `task.dueDay` | `task.tagIds` | -| **In task.tagIds?** | NO (invariant) | YES (required) | -| **tag.taskIds purpose** | Ordering only | Ordering only | -| **Use case** | Time-based grouping | Category/label grouping | +| Aspect | TODAY_TAG (Virtual) | Regular Tags (Board-Style) | +| ----------------------- | -------------------------------------------------------------------------------------------------------- | ----------------------------- | +| **Membership source** | `task.dueWithTime` OR `task.dueDay` (see [mutual exclusivity](dueDay-dueWithTime-mutual-exclusivity.md)) | `task.tagIds.includes(tagId)` | +| **Stored on task** | `task.dueWithTime` or `task.dueDay` | `task.tagIds` | +| **In task.tagIds?** | NO (invariant) | YES (required) | +| **tag.taskIds purpose** | Ordering only | Ordering only | +| **Use case** | Time-based grouping | Category/label grouping | ## Why This Pattern? 1. **Uniform move operations**: Drag/drop and keyboard shortcuts (Ctrl+↑/↓) work identically for TODAY_TAG and regular tags because all tags store ordering in `tag.taskIds`. -2. **Single source of truth**: `task.dueDay` is the canonical field for "is this task scheduled for today?" - no dual bookkeeping between `dueDay` and a hypothetical `tagIds` entry. +2. **Single source of truth**: `task.dueWithTime` OR `task.dueDay` (via [mutual exclusivity pattern](dueDay-dueWithTime-mutual-exclusivity.md)) is the canonical field for "is this task scheduled for today?" - no dual bookkeeping between date fields and a hypothetical `tagIds` entry. -3. **Planner integration**: The planner view uses `task.dueDay` to organize tasks by day. TODAY_TAG naturally aligns with this. +3. **Planner integration**: The planner view uses `task.dueWithTime` and `task.dueDay` to organize tasks by day. TODAY_TAG naturally aligns with this. 4. **Self-healing**: Stale ordering entries are gracefully filtered out by the selector. No manual cleanup needed when a task's `dueDay` changes. @@ -51,7 +51,7 @@ export const TODAY_TAG: Tag = { The `computeOrderedTaskIdsForToday()` function: -1. Finds all tasks where `dueDay === today` (membership) +1. Finds all tasks where `dueWithTime` is today OR `dueDay === today` (membership via [mutual exclusivity pattern](dueDay-dueWithTime-mutual-exclusivity.md)) 2. Orders them according to `TODAY_TAG.taskIds` (ordering) 3. Appends any unordered tasks at the end (self-healing) 4. Filters out stale entries from `TODAY_TAG.taskIds` (self-healing) @@ -60,10 +60,24 @@ The `computeOrderedTaskIdsForToday()` function: const computeOrderedTaskIdsForToday = (todayTag, taskEntities) => { const todayStr = getDbDateStr(); - // Membership: tasks where dueDay === today - const tasksForToday = Object.values(taskEntities) - .filter((t) => t && !t.parentId && t.dueDay === todayStr) - .map((t) => t.id); + // Membership: tasks where dueWithTime is today OR dueDay === today + // See: dueDay-dueWithTime-mutual-exclusivity.md for priority pattern + const tasksForToday: string[] = []; + for (const taskId of Object.keys(taskEntities)) { + const task = taskEntities[taskId]; + if (task) { + // Check dueWithTime first (takes priority - mutual exclusivity) + if (task.dueWithTime) { + if (isToday(task.dueWithTime)) { + tasksForToday.push(taskId); + } + } + // Fallback: check dueDay only if dueWithTime is not set + else if (task.dueDay === todayStr) { + tasksForToday.push(taskId); + } + } + } // Ordering: use TODAY_TAG.taskIds, filter stale, append missing const storedOrder = todayTag?.taskIds || []; @@ -144,11 +158,16 @@ This handles state divergence from per-entity conflict resolution during sync. task.tagIds = [...task.tagIds, TODAY_TAG.id]; ``` -### Correct: Set task.dueDay +### Correct: Set task.dueDay or task.dueWithTime ```typescript -// CORRECT - Set dueDay to add to today +// CORRECT - Set dueDay to add to today (all-day, no specific time) task.dueDay = getDbDateStr(); // Today's date string +task.dueWithTime = undefined; // See: dueDay-dueWithTime-mutual-exclusivity.md + +// ALSO CORRECT - Set dueWithTime for specific time today +task.dueWithTime = Date.now(); // Specific time today +task.dueDay = undefined; // See: dueDay-dueWithTime-mutual-exclusivity.md ``` ### Wrong: Checking tagIds for TODAY membership @@ -158,23 +177,33 @@ task.dueDay = getDbDateStr(); // Today's date string const isToday = task.tagIds.includes(TODAY_TAG.id); ``` -### Correct: Check dueDay +### Correct: Check dueWithTime and dueDay (with priority) ```typescript -// CORRECT - Check dueDay for TODAY membership -const isToday = task.dueDay === getDbDateStr(); +// CORRECT - Check dueWithTime first, then dueDay (mutual exclusivity pattern) +// See: dueDay-dueWithTime-mutual-exclusivity.md +let isInToday = false; +if (task.dueWithTime) { + isInToday = isToday(task.dueWithTime); +} else if (task.dueDay === getDbDateStr()) { + isInToday = true; +} ``` +## Related Documentation + +- **[dueDay/dueWithTime Mutual Exclusivity Pattern](dueDay-dueWithTime-mutual-exclusivity.md)** - Understanding how `dueDay` and `dueWithTime` interact (critical for TODAY_TAG membership) + ## Key Files Reference -| File | Purpose | -| ----------------------------------------------------------------------------- | --------------------------------------------- | -| `src/app/features/tag/tag.const.ts` | TODAY_TAG definition | -| `src/app/features/work-context/store/work-context.selectors.ts` | Membership computation, repair selector | -| `src/app/features/tag/store/tag.reducer.ts` | Move operations (uniform for all tags) | -| `src/app/root-store/meta/task-shared-meta-reducers/planner-shared.reducer.ts` | Multi-entity updates for planner actions | -| `src/app/root-store/meta/task-shared-meta-reducers/task-shared-helpers.ts` | `filterOutTodayTag()`, `hasInvalidTodayTag()` | -| `src/app/features/tag/store/tag.effects.ts` | `repairTodayTagConsistency$` effect | +| File | Purpose | +| ----------------------------------------------------------------------------- | ------------------------------------------------------------------------- | +| `src/app/features/tag/tag.const.ts` | TODAY_TAG definition | +| `src/app/features/work-context/store/work-context.selectors.ts` | Membership computation (uses mutual exclusivity pattern), repair selector | +| `src/app/features/tag/store/tag.reducer.ts` | Move operations (uniform for all tags) | +| `src/app/root-store/meta/task-shared-meta-reducers/planner-shared.reducer.ts` | Multi-entity updates for planner actions | +| `src/app/root-store/meta/task-shared-meta-reducers/task-shared-helpers.ts` | `filterOutTodayTag()`, `hasInvalidTodayTag()` | +| `src/app/features/tag/store/tag.effects.ts` | `repairTodayTagConsistency$` effect | ## Testing diff --git a/src/app/features/tasks/task.model.ts b/src/app/features/tasks/task.model.ts index b34451ead4..0ef0c1c602 100644 --- a/src/app/features/tasks/task.model.ts +++ b/src/app/features/tasks/task.model.ts @@ -88,7 +88,29 @@ export interface TaskCopy timeSpentOnDay: TimeSpentOnDay; // Additional app-specific fields + + /** + * Scheduled time as Unix timestamp (ms). For tasks scheduled with a specific time. + * + * IMPORTANT: dueWithTime and dueDay follow a mutual exclusivity pattern: + * - When dueWithTime is set, dueDay MUST be undefined/null (not both set) + * - When reading, check dueWithTime FIRST (it takes priority over dueDay) + * + * @see docs/ai/dueDay-dueWithTime-mutual-exclusivity.md + */ dueWithTime?: number | null; + + /** + * Scheduled date as ISO date string (YYYY-MM-DD). For tasks scheduled for all-day (no specific time). + * + * IMPORTANT: dueDay and dueWithTime follow a mutual exclusivity pattern: + * - When dueDay is set, dueWithTime should be undefined/null (for new data) + * - When dueWithTime is set, dueDay MUST be undefined/null (not both set) + * - When reading, check dueWithTime FIRST (it takes priority over dueDay) + * - Legacy data may have both fields set; handle via priority pattern + * + * @see docs/ai/dueDay-dueWithTime-mutual-exclusivity.md + */ dueDay?: string | null; hasPlannedTime?: boolean; attachments: TaskAttachment[]; diff --git a/src/app/features/work-context/store/work-context.selectors.ts b/src/app/features/work-context/store/work-context.selectors.ts index e63507b889..ad3ed444a3 100644 --- a/src/app/features/work-context/store/work-context.selectors.ts +++ b/src/app/features/work-context/store/work-context.selectors.ts @@ -49,8 +49,12 @@ const computeOrderedTaskIdsForToday = ( const todayStr = getDbDateStr(); const storedOrder = todayTag?.taskIds || []; - // Find all tasks where dueWithTime is for today OR dueDay === today - // Priority: dueWithTime takes precedence over dueDay (mutual exclusivity pattern) + // IMPORTANT: Implements dueDay/dueWithTime mutual exclusivity pattern + // - Check dueWithTime FIRST (it takes priority over dueDay) + // - Only check dueDay if dueWithTime is not set + // - If dueWithTime is set, do NOT check dueDay (even for legacy data with both fields) + // - This ensures correct behavior with both new data (only one field set) and legacy data (both fields set) + // See: docs/ai/dueDay-dueWithTime-mutual-exclusivity.md const tasksForToday: string[] = []; for (const taskId of Object.keys(taskEntities)) { const task = taskEntities[taskId]; diff --git a/src/app/imex/sync/sync-trigger.service.ts b/src/app/imex/sync/sync-trigger.service.ts index 164c1a4d00..aba8eba83d 100644 --- a/src/app/imex/sync/sync-trigger.service.ts +++ b/src/app/imex/sync/sync-trigger.service.ts @@ -16,7 +16,6 @@ import { take, tap, throttleTime, - withLatestFrom, } from 'rxjs/operators'; import { GlobalConfigService } from '../../features/config/global-config.service'; import { isOnline$ } from '../../util/is-online'; @@ -33,16 +32,14 @@ import { androidInterface } from '../../features/android/android-interface'; import { ipcResume$, ipcSuspend$ } from '../../core/ipc-events'; import { IS_TOUCH_PRIMARY } from '../../util/is-mouse-primary'; import { DataInitStateService } from '../../core/data-init/data-init-state.service'; -import { Store } from '@ngrx/store'; -import { selectCurrentTaskId } from '../../features/tasks/store/task.selectors'; import { SyncLog } from '../../core/log'; import { SyncWrapperService } from './sync-wrapper.service'; import { SyncProviderId } from '../../op-log/sync-exports'; const MAX_WAIT_FOR_INITIAL_SYNC = 25000; -const USER_INTERACTION_SYNC_CHECK_THROTTLE_TIME = 15 * 60 * 10000; +/** 15 minutes in milliseconds - throttle time for user activity sync checks */ +const USER_ACTIVITY_SYNC_THROTTLE_TIME = 15 * 60 * 1000; -// TODO naming @Injectable({ providedIn: 'root', }) @@ -50,7 +47,6 @@ export class SyncTriggerService { private readonly _globalConfigService = inject(GlobalConfigService); private readonly _dataInitStateService = inject(DataInitStateService); private readonly _idleService = inject(IdleService); - private readonly _store = inject(Store); private readonly _syncWrapperService = inject(SyncWrapperService); // Note: This was previously connected to PFAPI's onLocalMetaUpdate$, which was a no-op. @@ -87,11 +83,11 @@ export class SyncTriggerService { fromEvent(document, 'visibilitychange'), ).pipe( mapTo('I_MOUSE_TOUCH_MOVE_OR_VISIBILITYCHANGE'), - throttleTime(USER_INTERACTION_SYNC_CHECK_THROTTLE_TIME), + throttleTime(USER_ACTIVITY_SYNC_THROTTLE_TIME), ) : fromEvent(window, 'focus').pipe( mapTo('I_FOCUS_THROTTLED'), - throttleTime(USER_INTERACTION_SYNC_CHECK_THROTTLE_TIME), + throttleTime(USER_ACTIVITY_SYNC_THROTTLE_TIME), ), ), ); @@ -120,17 +116,6 @@ export class SyncTriggerService { ) : EMPTY; - private _onBlurWhenNotTracking$: Observable = fromEvent( - window, - 'blur', - ).pipe( - withLatestFrom(this._store.select(selectCurrentTaskId)), - filter(([, currentTaskId]) => !currentTaskId), - mapTo('I_BLUR_WHILE_NOT_TRACKING'), - // we throttle this to prevent lots of updates - throttleTime(10 * 60 * 1000), - ); - private _isOnlineTrigger$: Observable = isOnline$.pipe( // skip initial online which always fires on page load skip(1), @@ -216,7 +201,6 @@ export class SyncTriggerService { this._isOnlineTrigger$, this._onIdleTrigger$, this._onElectronResumeTrigger$, - this._onBlurWhenNotTracking$, ); return merge( // once immediately diff --git a/src/app/op-log/sync-providers/super-sync/super-sync.spec.ts b/src/app/op-log/sync-providers/super-sync/super-sync.spec.ts index 58edf9613c..5d3d72a42e 100644 --- a/src/app/op-log/sync-providers/super-sync/super-sync.spec.ts +++ b/src/app/op-log/sync-providers/super-sync/super-sync.spec.ts @@ -189,8 +189,11 @@ describe('SuperSyncProvider', () => { }); }); - describe('config caching', () => { - it('should cache config and only call privateCfg.load once for multiple operations', async () => { + describe('config loading', () => { + it('should call privateCfg.load for each operation (relies on SyncCredentialStore caching)', async () => { + // Note: We removed redundant caching in SuperSyncProvider since SyncCredentialStore + // already has its own in-memory caching. Each call to _cfgOrError now calls load(), + // but SyncCredentialStore returns cached value after first load. mockPrivateCfgStore.load.and.returnValue(Promise.resolve(testConfig)); fetchSpy.and.returnValue( Promise.resolve({ @@ -204,11 +207,13 @@ describe('SuperSyncProvider', () => { await provider.downloadOps(1); await provider.downloadOps(2); - // privateCfg.load should only be called once due to caching - expect(mockPrivateCfgStore.load).toHaveBeenCalledTimes(1); + // privateCfg.load is called for each operation, but SyncCredentialStore caches internally + expect(mockPrivateCfgStore.load).toHaveBeenCalledTimes(3); }); - it('should cache server seq key and only call privateCfg.load once for multiple seq operations', async () => { + it('should call privateCfg.load for each server seq operation (relies on SyncCredentialStore caching)', async () => { + // Note: We removed redundant caching in SuperSyncProvider since SyncCredentialStore + // already has its own in-memory caching. mockPrivateCfgStore.load.and.returnValue(Promise.resolve(testConfig)); localStorageSpy.getItem.and.returnValue('10'); @@ -217,8 +222,8 @@ describe('SuperSyncProvider', () => { await provider.getLastServerSeq(); await provider.getLastServerSeq(); - // privateCfg.load should only be called once due to caching - expect(mockPrivateCfgStore.load).toHaveBeenCalledTimes(1); + // privateCfg.load is called for each operation, but SyncCredentialStore caches internally + expect(mockPrivateCfgStore.load).toHaveBeenCalledTimes(3); }); }); @@ -605,8 +610,9 @@ describe('SuperSyncProvider', () => { ); }); - it('should clear cached config on auth failure', async () => { - // First call succeeds - config gets cached + it('should throw AuthFailSPError and allow next operation to proceed', async () => { + // Note: We removed the local caching in SuperSyncProvider since SyncCredentialStore + // already has its own in-memory caching. Each operation calls load() directly. mockPrivateCfgStore.load.and.returnValue(Promise.resolve(testConfig)); fetchSpy.and.returnValue( Promise.resolve({ @@ -634,7 +640,7 @@ describe('SuperSyncProvider', () => { expect(e).toBeInstanceOf(AuthFailSPError); } - // Third call should reload config from store (cache was cleared) + // Third call should succeed - config reloaded from SyncCredentialStore fetchSpy.and.returnValue( Promise.resolve({ ok: true, @@ -642,7 +648,8 @@ describe('SuperSyncProvider', () => { } as Response), ); await provider.downloadOps(0); - expect(mockPrivateCfgStore.load).toHaveBeenCalledTimes(2); + // Each operation calls load() directly (SyncCredentialStore handles caching) + expect(mockPrivateCfgStore.load).toHaveBeenCalledTimes(3); }); it('should throw AuthFailSPError for uploadOps on 401', async () => { diff --git a/src/app/op-log/sync-providers/super-sync/super-sync.ts b/src/app/op-log/sync-providers/super-sync/super-sync.ts index 943028dbb5..0664a10cb0 100644 --- a/src/app/op-log/sync-providers/super-sync/super-sync.ts +++ b/src/app/op-log/sync-providers/super-sync/super-sync.ts @@ -57,10 +57,6 @@ export class SuperSyncProvider public privateCfg: SyncCredentialStore; - // Caches to reduce repeated async loads during sync operations - private _cachedCfg?: SuperSyncPrivateCfg; - private _cachedServerSeqKey?: string; - constructor(_basePath?: string) { // basePath is ignored - SuperSync uses operation-based sync only this.privateCfg = new SyncCredentialStore(SyncProviderId.SuperSync); @@ -82,9 +78,6 @@ export class SuperSyncProvider } async setPrivateCfg(cfg: SuperSyncPrivateCfg): Promise { - // Invalidate caches when config changes - this._cachedCfg = undefined; - this._cachedServerSeqKey = undefined; await this.privateCfg.setComplete(cfg); } @@ -359,14 +352,11 @@ export class SuperSyncProvider // === Private Helper Methods === private async _cfgOrError(): Promise { - if (this._cachedCfg) { - return this._cachedCfg; - } + // Note: SyncCredentialStore.load() has its own in-memory caching const cfg = await this.privateCfg.load(); if (!cfg) { throw new MissingCredentialsSPError(); } - this._cachedCfg = cfg; return cfg; } @@ -375,9 +365,7 @@ export class SuperSyncProvider * when switching between different accounts or servers. */ private async _getServerSeqKey(): Promise { - if (this._cachedServerSeqKey) { - return this._cachedServerSeqKey; - } + // Note: SyncCredentialStore.load() has its own in-memory caching const cfg = await this.privateCfg.load(); const baseUrl = cfg?.baseUrl ?? 'default'; // Include accessToken in the hash so different users on the same server @@ -389,20 +377,14 @@ export class SuperSyncProvider .split('') .reduce((acc, char) => ((acc << 5) - acc + char.charCodeAt(0)) | 0, 0) .toString(16); - this._cachedServerSeqKey = `${LAST_SERVER_SEQ_KEY_PREFIX}${hash}`; - return this._cachedServerSeqKey; + return `${LAST_SERVER_SEQ_KEY_PREFIX}${hash}`; } /** * Check HTTP response status and throw AuthFailSPError for auth failures. - * Clears cached config so next operation will reload from store. */ private _checkHttpStatus(status: number, body?: string): void { if (status === 401 || status === 403) { - // Clear cached config so next operation will reload from store - // (allowing user to re-configure after auth failure) - this._cachedCfg = undefined; - this._cachedServerSeqKey = undefined; throw new AuthFailSPError(`Authentication failed (HTTP ${status})`, body); } } diff --git a/src/app/op-log/sync/conflict-resolution.service.ts b/src/app/op-log/sync/conflict-resolution.service.ts index e404327c97..da8e30bed2 100644 --- a/src/app/op-log/sync/conflict-resolution.service.ts +++ b/src/app/op-log/sync/conflict-resolution.service.ts @@ -20,6 +20,7 @@ import { MAX_CONFLICT_RETRY_ATTEMPTS } from '../core/operation-log.const'; import { DUPLICATE_OPERATION_ERROR_PATTERN } from '../persistence/op-log-errors.const'; import { compareVectorClocks, + incrementVectorClock, mergeVectorClocks, VectorClockComparison, } from '../../core/util/vector-clock'; @@ -33,7 +34,8 @@ import { isArrayEntity, } from '../core/entity-registry'; import { selectIssueProviderById } from '../../features/issue/store/issue-provider.selectors'; -import { LWWOperationFactory } from './lww-operation-factory.service'; +import { uuidv7 } from '../../util/uuid-v7'; +import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service'; /** * Represents the result of LWW (Last-Write-Wins) conflict resolution. @@ -81,7 +83,72 @@ export class ConflictResolutionService { private snackService = inject(SnackService); private validateStateService = inject(ValidateStateService); private clientIdProvider = inject(CLIENT_ID_PROVIDER); - private lwwOperationFactory = inject(LWWOperationFactory); + + // ═══════════════════════════════════════════════════════════════════════════ + // LWW OPERATION FACTORY METHODS + // ═══════════════════════════════════════════════════════════════════════════ + + /** + * Creates a new LWW Update operation for syncing local state. + * + * LWW Update operations are synthetic operations created during conflict resolution + * to carry the winning local state to remote clients. They are created when: + * 1. Local state wins LWW conflict resolution + * 2. Stale local operations need to be re-uploaded with merged clocks + * + * These operations use dynamically constructed action types (e.g., '[TASK] LWW Update') + * that are matched by regex in lwwUpdateMetaReducer. + * + * @param entityType - Type of the entity being updated + * @param entityId - ID of the entity being updated + * @param entityState - Current state of the entity to sync + * @param clientId - Client creating this operation + * @param vectorClock - Merged vector clock (should dominate all conflicting ops) + * @param timestamp - Preserved timestamp for correct LWW semantics + * @returns New UPDATE operation ready for upload + */ + createLWWUpdateOp( + entityType: EntityType, + entityId: string, + entityState: unknown, + clientId: string, + vectorClock: VectorClock, + timestamp: number, + ): Operation { + // NOTE: LWW Update action types (e.g., '[TASK] LWW Update') are intentionally + // NOT in the ActionType enum. They are dynamically constructed here and matched + // by regex in lwwUpdateMetaReducer. This is by design - LWW ops are synthetic, + // created during conflict resolution to carry the winning local state to remote clients. + return { + id: uuidv7(), + actionType: `[${entityType}] LWW Update` as ActionType, + opType: OpType.Update, + entityType, + entityId, + payload: entityState, + clientId, + vectorClock, + timestamp, + schemaVersion: CURRENT_SCHEMA_VERSION, + }; + } + + /** + * Merges multiple vector clocks and increments for the given client. + * Used when creating LWW Update operations that need to dominate + * all previously known clocks. + * + * @param clocks - Array of vector clocks to merge + * @param clientId - Client ID to increment in the final clock + * @returns Merged and incremented vector clock + */ + mergeAndIncrementClocks(clocks: VectorClock[], clientId: string): VectorClock { + let mergedClock: VectorClock = {}; + for (const clock of clocks) { + mergedClock = mergeVectorClocks(mergedClock, clock); + } + return incrementVectorClock(mergedClock, clientId); + } /** * Validates the current state after conflict resolution and repairs if necessary. @@ -591,10 +658,7 @@ export class ConflictResolutionService { ...conflict.localOps.map((op) => op.vectorClock), ...conflict.remoteOps.map((op) => op.vectorClock), ]; - const newClock = this.lwwOperationFactory.mergeAndIncrementClocks( - allClocks, - clientId, - ); + const newClock = this.mergeAndIncrementClocks(allClocks, clientId); // Preserve the maximum timestamp from local ops. // This is critical for LWW semantics: we're creating a new op to carry the @@ -602,7 +666,7 @@ export class ConflictResolutionService { // it to win. Using Date.now() would give it an unfair advantage in future conflicts. const preservedTimestamp = Math.max(...conflict.localOps.map((op) => op.timestamp)); - return this.lwwOperationFactory.createLWWUpdateOp( + return this.createLWWUpdateOp( conflict.entityType, conflict.entityId, entityState, diff --git a/src/app/op-log/sync/lww-operation-factory.service.spec.ts b/src/app/op-log/sync/lww-operation-factory.service.spec.ts deleted file mode 100644 index d1dd69668d..0000000000 --- a/src/app/op-log/sync/lww-operation-factory.service.spec.ts +++ /dev/null @@ -1,249 +0,0 @@ -import { TestBed } from '@angular/core/testing'; -import { LWWOperationFactory } from './lww-operation-factory.service'; -import { EntityType, OpType, VectorClock } from '../core/operation.types'; -import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service'; - -describe('LWWOperationFactory', () => { - let service: LWWOperationFactory; - - beforeEach(() => { - TestBed.configureTestingModule({ - providers: [LWWOperationFactory], - }); - service = TestBed.inject(LWWOperationFactory); - }); - - describe('createLWWUpdateOp', () => { - const entityType: EntityType = 'TASK'; - const entityId = 'task-123'; - const entityState = { id: entityId, title: 'Test Task', done: false }; - const clientId = 'client_abc'; - const vectorClock: VectorClock = { client_abc: 5, client_xyz: 3 }; - const timestamp = 1700000000000; - - it('should create operation with correct action type format [ENTITY] LWW Update', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.actionType).toBe('[TASK] LWW Update'); - }); - - it('should create action type for different entity types', () => { - const projectOp = service.createLWWUpdateOp( - 'PROJECT', - 'proj-1', - {}, - clientId, - vectorClock, - timestamp, - ); - expect(projectOp.actionType).toBe('[PROJECT] LWW Update'); - - const tagOp = service.createLWWUpdateOp( - 'TAG', - 'tag-1', - {}, - clientId, - vectorClock, - timestamp, - ); - expect(tagOp.actionType).toBe('[TAG] LWW Update'); - }); - - it('should assign correct opType (Update)', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.opType).toBe(OpType.Update); - }); - - it('should use provided entityType and entityId', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.entityType).toBe(entityType); - expect(op.entityId).toBe(entityId); - }); - - it('should include entityState as payload', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.payload).toEqual(entityState); - }); - - it('should generate UUIDv7 ID', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - // UUIDv7 format: 8-4-4-4-12 hex characters - expect(op.id).toMatch( - /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, - ); - }); - - it('should generate unique IDs for each call', () => { - const op1 = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - const op2 = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op1.id).not.toBe(op2.id); - }); - - it('should include CURRENT_SCHEMA_VERSION', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.schemaVersion).toBe(CURRENT_SCHEMA_VERSION); - }); - - it('should preserve provided vectorClock', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.vectorClock).toEqual(vectorClock); - }); - - it('should preserve provided timestamp', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.timestamp).toBe(timestamp); - }); - - it('should preserve provided clientId', () => { - const op = service.createLWWUpdateOp( - entityType, - entityId, - entityState, - clientId, - vectorClock, - timestamp, - ); - - expect(op.clientId).toBe(clientId); - }); - }); - - describe('mergeAndIncrementClocks', () => { - it('should return incremented clock for single input clock', () => { - const clock: VectorClock = { clientA: 5 }; - const result = service.mergeAndIncrementClocks([clock], 'clientA'); - - expect(result).toEqual({ clientA: 6 }); - }); - - it('should merge two clocks taking max of each component', () => { - const clock1: VectorClock = { clientA: 5, clientB: 3 }; - const clock2: VectorClock = { clientA: 3, clientB: 7 }; - const result = service.mergeAndIncrementClocks([clock1, clock2], 'clientA'); - - expect(result['clientA']).toBe(6); // max(5,3) + 1 - expect(result['clientB']).toBe(7); // max(3,7) - }); - - it('should merge three+ clocks correctly', () => { - const clock1: VectorClock = { clientA: 1 }; - const clock2: VectorClock = { clientA: 5, clientB: 2 }; - const clock3: VectorClock = { clientB: 8, clientC: 3 }; - const result = service.mergeAndIncrementClocks([clock1, clock2, clock3], 'clientA'); - - expect(result['clientA']).toBe(6); // max(1,5,0) + 1 - expect(result['clientB']).toBe(8); // max(0,2,8) - expect(result['clientC']).toBe(3); // max(0,0,3) - }); - - it('should handle empty clocks array', () => { - const result = service.mergeAndIncrementClocks([], 'clientA'); - - expect(result).toEqual({ clientA: 1 }); - }); - - it('should increment merged clock for given clientId', () => { - const clock: VectorClock = { clientA: 10, clientB: 5 }; - const result = service.mergeAndIncrementClocks([clock], 'clientB'); - - expect(result['clientA']).toBe(10); // unchanged - expect(result['clientB']).toBe(6); // incremented - }); - - it('should add new clientId if not in any input clocks', () => { - const clock: VectorClock = { clientA: 5 }; - const result = service.mergeAndIncrementClocks([clock], 'clientNew'); - - expect(result['clientA']).toBe(5); // preserved - expect(result['clientNew']).toBe(1); // new entry, starts at 1 - }); - - it('should handle clocks with non-overlapping clients', () => { - const clock1: VectorClock = { clientA: 3 }; - const clock2: VectorClock = { clientB: 7 }; - const result = service.mergeAndIncrementClocks([clock1, clock2], 'clientC'); - - expect(result['clientA']).toBe(3); - expect(result['clientB']).toBe(7); - expect(result['clientC']).toBe(1); - }); - }); -}); diff --git a/src/app/op-log/sync/lww-operation-factory.service.ts b/src/app/op-log/sync/lww-operation-factory.service.ts deleted file mode 100644 index 238a5f263a..0000000000 --- a/src/app/op-log/sync/lww-operation-factory.service.ts +++ /dev/null @@ -1,81 +0,0 @@ -import { Injectable } from '@angular/core'; -import { - ActionType, - EntityType, - Operation, - OpType, - VectorClock, -} from '../core/operation.types'; -import { uuidv7 } from '../../util/uuid-v7'; -import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service'; -import { incrementVectorClock, mergeVectorClocks } from '../../core/util/vector-clock'; - -/** - * Factory for creating LWW (Last-Write-Wins) Update operations. - * - * LWW Update operations are synthetic operations created during conflict resolution - * to carry the winning local state to remote clients. They are created when: - * 1. Local state wins LWW conflict resolution (ConflictResolutionService) - * 2. Stale local operations need to be re-uploaded with merged clocks (StaleOperationResolverService) - * - * These operations use dynamically constructed action types (e.g., '[TASK] LWW Update') - * that are matched by regex in lwwUpdateMetaReducer. - */ -@Injectable({ - providedIn: 'root', -}) -export class LWWOperationFactory { - /** - * Creates a new LWW Update operation for syncing local state. - * - * @param entityType - Type of the entity being updated - * @param entityId - ID of the entity being updated - * @param entityState - Current state of the entity to sync - * @param clientId - Client creating this operation - * @param vectorClock - Merged vector clock (should dominate all conflicting ops) - * @param timestamp - Preserved timestamp for correct LWW semantics - * @returns New UPDATE operation ready for upload - */ - createLWWUpdateOp( - entityType: EntityType, - entityId: string, - entityState: unknown, - clientId: string, - vectorClock: VectorClock, - timestamp: number, - ): Operation { - // NOTE: LWW Update action types (e.g., '[TASK] LWW Update') are intentionally - // NOT in the ActionType enum. They are dynamically constructed here and matched - // by regex in lwwUpdateMetaReducer. This is by design - LWW ops are synthetic, - // created during conflict resolution to carry the winning local state to remote clients. - return { - id: uuidv7(), - actionType: `[${entityType}] LWW Update` as ActionType, - opType: OpType.Update, - entityType, - entityId, - payload: entityState, - clientId, - vectorClock, - timestamp, - schemaVersion: CURRENT_SCHEMA_VERSION, - }; - } - - /** - * Merges multiple vector clocks and increments for the given client. - * Used when creating LWW Update operations that need to dominate - * all previously known clocks. - * - * @param clocks - Array of vector clocks to merge - * @param clientId - Client ID to increment in the final clock - * @returns Merged and incremented vector clock - */ - mergeAndIncrementClocks(clocks: VectorClock[], clientId: string): VectorClock { - let mergedClock: VectorClock = {}; - for (const clock of clocks) { - mergedClock = mergeVectorClocks(mergedClock, clock); - } - return incrementVectorClock(mergedClock, clientId); - } -} diff --git a/src/app/op-log/sync/stale-operation-resolver.service.spec.ts b/src/app/op-log/sync/stale-operation-resolver.service.spec.ts index 7e662dd147..4d48b091a0 100644 --- a/src/app/op-log/sync/stale-operation-resolver.service.spec.ts +++ b/src/app/op-log/sync/stale-operation-resolver.service.spec.ts @@ -49,6 +49,8 @@ describe('StaleOperationResolverService', () => { ]); mockConflictResolutionService = jasmine.createSpyObj('ConflictResolutionService', [ 'getCurrentEntityState', + 'mergeAndIncrementClocks', + 'createLWWUpdateOp', ]); mockLockService = jasmine.createSpyObj('LockService', ['request']); mockSnackService = jasmine.createSpyObj('SnackService', ['open']); @@ -66,6 +68,40 @@ describe('StaleOperationResolverService', () => { mockLockService.request.and.callFake( (_lockName: string, callback: () => Promise) => callback(), ); + // Mock merged clock methods - merged from LWWOperationFactory + mockConflictResolutionService.mergeAndIncrementClocks.and.callFake( + (clocks: VectorClock[], clientId: string) => { + const merged: VectorClock = {}; + for (const clock of clocks) { + for (const [k, v] of Object.entries(clock)) { + merged[k] = Math.max(merged[k] || 0, v); + } + } + merged[clientId] = (merged[clientId] || 0) + 1; + return merged; + }, + ); + mockConflictResolutionService.createLWWUpdateOp.and.callFake( + ( + entityType: EntityType, + entityId: string, + entityState: unknown, + clientId: string, + vectorClock: VectorClock, + timestamp: number, + ) => ({ + id: 'generated-id-' + Math.random().toString(36).substring(7), + actionType: `[${entityType}] LWW Update` as ActionType, + opType: OpType.Update, + entityType, + entityId, + payload: entityState, + clientId, + vectorClock, + timestamp, + schemaVersion: 1, + }), + ); TestBed.configureTestingModule({ providers: [ diff --git a/src/app/op-log/sync/stale-operation-resolver.service.ts b/src/app/op-log/sync/stale-operation-resolver.service.ts index dfe10259db..fd54da86e0 100644 --- a/src/app/op-log/sync/stale-operation-resolver.service.ts +++ b/src/app/op-log/sync/stale-operation-resolver.service.ts @@ -11,7 +11,6 @@ import { LOCK_NAMES } from '../core/operation-log.const'; import { SnackService } from '../../core/snack/snack.service'; import { T } from '../../t.const'; import { CLIENT_ID_PROVIDER } from '../util/client-id.provider'; -import { LWWOperationFactory } from './lww-operation-factory.service'; import { uuidv7 } from '../../util/uuid-v7'; import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service'; @@ -42,7 +41,6 @@ export class StaleOperationResolverService { private lockService = inject(LockService); private snackService = inject(SnackService); private clientIdProvider = inject(CLIENT_ID_PROVIDER); - private lwwOperationFactory = inject(LWWOperationFactory); /** * Resolves stale local operations by creating new LWW Update operations. @@ -123,7 +121,7 @@ export class StaleOperationResolverService { // Start with the global clock, merge in local pending ops' clocks, and increment const allClocks = [globalClock, ...entityOps.map(({ op }) => op.vectorClock)]; - const newClock = this.lwwOperationFactory.mergeAndIncrementClocks( + const newClock = this.conflictResolutionService.mergeAndIncrementClocks( allClocks, clientId, ); @@ -183,7 +181,7 @@ export class StaleOperationResolverService { const preservedTimestamp = Math.max(...entityOps.map((e) => e.op.timestamp)); // Create new UPDATE op with current state and merged clock - const newOp = this.lwwOperationFactory.createLWWUpdateOp( + const newOp = this.conflictResolutionService.createLWWUpdateOp( entityType, entityId, entityState, diff --git a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts index aee3f257ea..0f6c57baaa 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts @@ -26,6 +26,12 @@ import { filterOutId } from '../../../util/filter-out-id'; // ============================================================================= // ACTION HANDLERS // ============================================================================= +// +// IMPORTANT: These handlers implement the dueDay/dueWithTime mutual exclusivity pattern. +// When setting dueWithTime, dueDay is cleared (set to undefined). +// See: docs/ai/dueDay-dueWithTime-mutual-exclusivity.md +// +// ============================================================================= const handleScheduleTaskWithTime = ( state: RootState, @@ -62,7 +68,10 @@ const handleScheduleTaskWithTime = ( id: task.id, changes: { dueWithTime, - dueDay: undefined, // Mutual exclusivity: dueWithTime clears dueDay + // CRITICAL: Mutual exclusivity pattern - setting dueWithTime clears dueDay + // This prevents state inconsistency where both fields are set with conflicting dates + // See: docs/ai/dueDay-dueWithTime-mutual-exclusivity.md + dueDay: undefined, remindAt, }, },