mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-01-23 02:36:05 +00:00
refactor(tasks): use satisfies instead of as for PersistentActionMeta
Improves type safety by using satisfies which validates the object matches the type at compile time, rather than as which is just an assertion.
This commit is contained in:
Johannes Millan
parent
5ec4cc6444
commit
320e57e622
6 changed files with 1 additions and 881 deletions
|
|
@ -1,92 +0,0 @@
|
|||
# NgRx Implementation Analysis
|
||||
|
||||
**Date:** December 8, 2025
|
||||
**Scope:** `src` directory, focusing on `FeatureStoresModule`, `Task` feature, and core state management patterns.
|
||||
**Last Updated:** December 8, 2025 (added cross-references, validated findings)
|
||||
|
||||
## 1. Architecture Overview
|
||||
|
||||
The application employs a mature, feature-based NgRx architecture. It avoids a monolithic root state in favor of a modular approach where each domain feature manages its own slice of the state.
|
||||
|
||||
- **Modular Organization:** State is strictly compartmentalized. The `src/app/root-store/feature-stores.module.ts` acts as a central registry, importing over 20 distinct feature modules (e.g., `tasks`, `projects`, `tags`, `config`). This is a scalable and clean pattern that keeps feature concerns separated.
|
||||
- **Root Setup:** The root store is initialized in `src/main.ts` rather than `AppModule`. This unique setup likely caters to the specific bootstrapping requirements of the Electron/Web hybrid environment, allowing for early state initialization or meta-reducer configuration before the full Angular app launches.
|
||||
- **Core State (Tasks):** The `task` feature (`src/app/features/tasks/store/`) is the most complex part of the application. It utilizes `@ngrx/entity` to manage a normalized state of tasks. This is crucial for handling the relational nature of tasks (parents, subtasks) and their efficient updates.
|
||||
- **Interaction Pattern:** The application uses a hybrid "Service-Facade" pattern. Components like `TaskListComponent` inject the `Store` directly for dispatching actions but often rely on Services (e.g., `TaskService`) to expose selectors as Observables. This provides a pragmatic balance between the strict Command/Query segregation of NgRx and the developer ergonomics of Services.
|
||||
|
||||
## 2. Potential Bugs & Risks
|
||||
|
||||
### Type Safety Violations in Effects
|
||||
|
||||
- **Location:** `src/app/features/tasks/store/task-internal.effects.ts`
|
||||
- **Issue:** The `autoSetNextTask$` effect contains distinct type safety bypasses:
|
||||
- `// @ts-ignore` is used to suppress errors.
|
||||
- `const a = action as any;` is used to access properties on union types without proper type narrowing.
|
||||
- **Risk:** This bypasses TypeScript's compile-time safety. If the structure of the actions handled by this effect (`updateTask`, `moveProjectTaskToBacklogList`, etc.) changes, this code could fail silently at runtime, potentially breaking the "auto-next-task" feature.
|
||||
|
||||
### Complex Reducer Logic
|
||||
|
||||
- **Location:** `src/app/features/tasks/store/task.reducer.ts`
|
||||
- **Issue:** The `moveSubTask` reducer handles a complex multi-step state update. It manually updates the old parent's `subTaskIds`, the new parent's `subTaskIds`, and recalculates time estimates (`reCalcTimesForParentIfParent`) for both.
|
||||
- **Risk:** Orchestrating relational data updates manually within a reducer is brittle. If one step in this sequence calculates incorrectly or fails, the state could become inconsistent (e.g., a task thinks it has a parent, but the parent doesn't list it as a subtask).
|
||||
|
||||
### ~~Critical: Compaction Counter Bug~~ - FIXED
|
||||
|
||||
- **Location:** `src/app/core/persistence/operation-log/store/operation-log-store.service.ts:434-439`
|
||||
- **Original Issue:** When no state cache existed, `incrementCompactionCounter()` returned `1` without persisting the value.
|
||||
- **Status:** Fixed on 2025-12-08. Counter now persists when no cache exists.
|
||||
|
||||
## 3. Redundancies & Complexity
|
||||
|
||||
### Logic in Effects
|
||||
|
||||
- **Observation:** The `TaskInternalEffects` class contains a private method `_findNextTask`. This method encapsulates significant business logic for determining which task should be focused next.
|
||||
- **Impact:** Coupling complex business logic with the Effects class makes it harder to test. Effects should ideally be "dumb" pipelines that connect Actions to Services.
|
||||
|
||||
### Cross-Feature Coupling
|
||||
|
||||
- **Observation:** The `Planner` feature and `Task` feature are tightly coupled via Actions. For instance, `PlannerActions.planTaskForDay` is caught and handled inside `task.reducer.ts`.
|
||||
- **Impact:** While "Event-Driven" communication via Actions is a correct NgRx pattern, this "scattered" logic means that understanding how a Task entity changes requires auditing reducers in multiple different feature directories.
|
||||
|
||||
## 4. Testing Analysis
|
||||
|
||||
- **Strengths:**
|
||||
- **Selector Coverage:** `src/app/features/tasks/store/task.selectors.spec.ts` is robust. It mocks the full state tree (including dependencies like `Project` and `Tag`) and verifies logic for complex derived data (e.g., `selectTasksDueForDay`).
|
||||
- **Artifact coverage:** Most Store artifacts (Reducers, Effects, Selectors) have corresponding `*.spec.ts` files, indicating a strong testing culture.
|
||||
- **Gaps:**
|
||||
- The complex helper logic in `task.reducer.util.ts` (used by the reducer for calculations) should be verified to have its own independent unit tests, as the reducer relies heavily on its correctness for data integrity.
|
||||
- Effects coverage is at 51% - see `docs/ai/ngrx-implementation-review.md` for the full list.
|
||||
|
||||
## 5. Well-Designed Patterns
|
||||
|
||||
### TODAY_TAG Virtual Pattern (Correctly Implemented)
|
||||
|
||||
- **Location:** Meta-reducers in `src/app/root-store/meta/task-shared-meta-reducers/`
|
||||
- **Pattern:** TODAY_TAG is correctly implemented as a "virtual tag" where:
|
||||
- Membership is determined by `task.dueDay`, NOT by `task.tagIds`
|
||||
- `TODAY_TAG.taskIds` only stores ordering
|
||||
- Code actively removes any legacy TODAY_TAG from `task.tagIds` (cleanup)
|
||||
- **Note:** This pattern was initially misidentified as a bug in early reviews. The code is correct.
|
||||
|
||||
### Meta-Reducer Pattern for Atomic Operations
|
||||
|
||||
- **Location:** `src/app/root-store/meta/task-shared-meta-reducers/`
|
||||
- **Pattern:** Multi-entity changes are handled atomically in meta-reducers to ensure sync consistency.
|
||||
- **See:** `docs/ai/today-tag-architecture.md` and CLAUDE.md point 8.
|
||||
|
||||
## 6. Recommendations
|
||||
|
||||
1. **Refactor for Type Safety:**
|
||||
- Rewrite the `autoSetNextTask$` effect in `task-internal.effects.ts`. Use proper TypeScript Discriminated Unions or type guards to narrow the `action` type safely instead of casting to `any`.
|
||||
2. **Extract Logic from Effects:**
|
||||
- Move the `_findNextTask` logic into a stateless utility function or a domain service. This would allow it to be unit tested in isolation from the RxJS stream and the Store.
|
||||
3. **Solidify Reducer Helpers:**
|
||||
- Ensure that `src/app/features/tasks/store/task.reducer.util.ts` has comprehensive unit tests covering edge cases, specifically for `reCalcTimesForParentIfParent`, to prevent data corruption during subtask moves.
|
||||
4. ~~**Fix Critical Bugs:**~~ - COMPLETED
|
||||
- ✅ Compaction counter bug fixed on 2025-12-08.
|
||||
|
||||
## 7. Related Documentation
|
||||
|
||||
- `docs/ai/ngrx-implementation-review.md` - Detailed bug list and testing gaps
|
||||
- `docs/ai/sync/operation-log-implementation-review.md` - Operation log specific issues
|
||||
- `docs/ai/operation-log-review-plan.md` - Architecture review and implementation plan
|
||||
- `docs/ai/today-tag-architecture.md` - TODAY_TAG virtual pattern explanation
|
||||
|
|
@ -1,440 +0,0 @@
|
|||
# NgRx Implementation Review
|
||||
|
||||
**Date:** December 8, 2025
|
||||
**Branch:** `feat/operation-logs`
|
||||
**Reviewed by:** Claude (AI-assisted code review)
|
||||
**Last Updated:** December 8, 2025 (corrected false positives)
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
A comprehensive review of the NgRx implementation identified **22 issues** across bugs, redundancies, architecture concerns, and testing gaps.
|
||||
|
||||
| Category | Critical | High | Medium | Total |
|
||||
| ------------ | ----------- | ---- | ------ | ----- |
|
||||
| Bugs | 0 (1 fixed) | 4 | 8 | 12 |
|
||||
| Testing Gaps | 3 | 4 | 2 | 9 |
|
||||
|
||||
### Testing Coverage
|
||||
|
||||
| Category | Total | Tested | Coverage | Status |
|
||||
| ------------- | ----- | ------ | -------- | ---------------- |
|
||||
| Reducers | 35 | 28 | 80% | Good |
|
||||
| Effects | 39 | 20 | **51%** | **Critical Gap** |
|
||||
| Selectors | 13 | 12 | 92% | Excellent |
|
||||
| Meta-reducers | 10 | 8 | 80% | Good |
|
||||
|
||||
---
|
||||
|
||||
## Corrected Analysis (False Positives Removed)
|
||||
|
||||
### ~~TODAY_TAG Pattern Inconsistency~~ - FALSE POSITIVE
|
||||
|
||||
**Status:** Not a bug - code is correct.
|
||||
|
||||
**Original Claim:** Functions like `handleTransferTask` and `handlePlanTaskForDay` use "board-style" (adding TODAY_TAG to `task.tagIds`).
|
||||
|
||||
**Actual Code Behavior:** The code explicitly **REMOVES** TODAY_TAG from `task.tagIds`:
|
||||
|
||||
```typescript
|
||||
// planner-shared.reducer.ts:102-114
|
||||
// Remove TODAY from task.tagIds if present
|
||||
if (hasTaskTodayTag) {
|
||||
state = {
|
||||
...state,
|
||||
[TASK_FEATURE_NAME]: taskAdapter.updateOne(
|
||||
{
|
||||
id: task.id,
|
||||
changes: { tagIds: currentTagIds.filter((id) => id !== TODAY_TAG.id) },
|
||||
},
|
||||
state[TASK_FEATURE_NAME],
|
||||
),
|
||||
};
|
||||
}
|
||||
```
|
||||
|
||||
The comments in the code confirm: "Ensure TODAY_TAG is NOT in task.tagIds (cleanup if present from legacy data)".
|
||||
|
||||
**Conclusion:** The code correctly enforces the virtual tag pattern by removing any legacy TODAY_TAG references from `task.tagIds`. No fix needed.
|
||||
|
||||
---
|
||||
|
||||
### ~~Repair Mutex Race Condition~~ - FALSE POSITIVE
|
||||
|
||||
**Status:** Not a bug in JavaScript's single-threaded model.
|
||||
|
||||
**Original Claim:** Between promise creation and mutex assignment, another call can pass the `if (!this._repairMutex)` check.
|
||||
|
||||
**Why It's Not a Bug:** JavaScript is single-threaded. The code:
|
||||
|
||||
```typescript
|
||||
const repairPromise = (async () => { ... })(); // Returns immediately
|
||||
this._repairMutex = repairPromise; // Assigned synchronously on next line
|
||||
```
|
||||
|
||||
There is no `await` between these lines, so the event loop cannot yield. No other code can execute between promise creation and mutex assignment.
|
||||
|
||||
**Optional Improvement:** Setting the mutex before creating the promise is a valid defensive coding practice for clarity, but the current implementation is not buggy.
|
||||
|
||||
---
|
||||
|
||||
### ~~Cache Not Invalidated on Append~~ - FALSE POSITIVE
|
||||
|
||||
**Status:** Not a bug - cache self-invalidates.
|
||||
|
||||
**Original Claim:** `append()` doesn't invalidate `_appliedOpIdsCache`, causing stale data.
|
||||
|
||||
**Actual Implementation:**
|
||||
|
||||
```typescript
|
||||
async getAppliedOpIds(): Promise<Set<string>> {
|
||||
const currentLastSeq = await this.getLastSeq();
|
||||
// Cache valid only if lastSeq hasn't changed
|
||||
if (this._appliedOpIdsCache && this._cacheLastSeq === currentLastSeq) {
|
||||
return new Set(this._appliedOpIdsCache);
|
||||
}
|
||||
// Rebuild cache if seq changed
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
When new ops are appended, `lastSeq` changes (auto-increment). The cache automatically invalidates because `currentLastSeq !== _cacheLastSeq`.
|
||||
|
||||
**Conclusion:** No bug - the sequence-based invalidation works correctly.
|
||||
|
||||
---
|
||||
|
||||
## Critical Bugs (Fix Immediately)
|
||||
|
||||
### 1. ~~Compaction Counter Never Persists~~ - FIXED
|
||||
|
||||
**Severity:** Critical
|
||||
**File:** `src/app/core/persistence/operation-log/store/operation-log-store.service.ts`
|
||||
**Lines:** 434-439
|
||||
**Status:** Fixed on 2025-12-08
|
||||
|
||||
**Original Issue:** When no state cache exists, `incrementCompactionCounter()` returned 1 without persisting the value.
|
||||
|
||||
**Fix Applied:**
|
||||
|
||||
```typescript
|
||||
if (!cache) {
|
||||
// No state cache yet - create one with counter starting at 1
|
||||
await store.put({ id: 'current', compactionCounter: 1 });
|
||||
await tx.done;
|
||||
return 1;
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### ~~Memory Leak in TaskElectronEffects~~ - FALSE POSITIVE
|
||||
|
||||
**Status:** Not a bug - `take(1)` ensures automatic cleanup.
|
||||
|
||||
**Original Claim:** Subscriptions accumulate without cleanup in the IPC handler.
|
||||
|
||||
**Why It's Not a Bug:** Looking at the actual code:
|
||||
|
||||
```typescript
|
||||
this._store$.pipe(
|
||||
select(selectCurrentTask),
|
||||
withLatestFrom(...),
|
||||
take(1), // <-- Completes after 1 emission, auto-unsubscribes
|
||||
).subscribe(...)
|
||||
```
|
||||
|
||||
The `take(1)` operator ensures the observable completes after receiving one value, which automatically triggers unsubscription. Each IPC event creates a short-lived subscription that self-destructs after emitting once.
|
||||
|
||||
---
|
||||
|
||||
## High Priority Issues
|
||||
|
||||
### 1. ~~Snapshot Saved Before Validation~~ - FIXED
|
||||
|
||||
**File:** `src/app/core/persistence/operation-log/store/operation-log-hydrator.service.ts`
|
||||
**Lines:** 166-179
|
||||
**Status:** Fixed on 2025-12-08
|
||||
|
||||
**Original Issue:** Snapshot was saved before validation ran. If validation found corruption and repaired it, the snapshot was stale.
|
||||
|
||||
**Fix Applied:** Moved validation (CHECKPOINT C) BEFORE saving snapshot in both tail replay and full replay code paths. Snapshot now contains validated/repaired state.
|
||||
|
||||
---
|
||||
|
||||
### 2. ~~Missing Subtask Cascade in Tag Deletion~~ - FALSE POSITIVE
|
||||
|
||||
**File:** `src/app/root-store/meta/task-shared-meta-reducers/tag-shared.reducer.ts`
|
||||
**Lines:** 85-116
|
||||
**Status:** Re-evaluated on 2025-12-08 - NOT A BUG
|
||||
|
||||
**Original Claim:** Subtasks of surviving parents that lose all tags become orphaned but aren't cleaned up.
|
||||
|
||||
**Why It's Not a Bug:** A subtask with a surviving parent is NOT orphaned - it's still accessible through its parent in the UI. The current logic is correct:
|
||||
|
||||
- A task is orphaned only if it has no tags, no project, AND no parent
|
||||
- Subtasks always have a `parentId`, so they're never directly orphaned
|
||||
- If a parent becomes orphaned, its subtasks ARE correctly deleted via `removeOrphanedTasks()`
|
||||
- If a parent survives, its subtasks remain accessible through the parent
|
||||
|
||||
---
|
||||
|
||||
### ~~3. Race Conditions in TaskDueEffects~~ - FALSE POSITIVE
|
||||
|
||||
**File:** `src/app/features/tasks/store/task-due.effects.ts`
|
||||
**Status:** Re-evaluated on 2025-12-08 - NOT A BUG
|
||||
|
||||
**Original Claims:**
|
||||
|
||||
- Double sync wait (before AND after debounce)
|
||||
- Multiple effects compete on same signals
|
||||
- Potential infinite loop if action triggers date change
|
||||
|
||||
**Why It's Not a Bug:**
|
||||
|
||||
1. **Double sync wait is intentional:** The pattern `wait → debounce → wait again` ensures:
|
||||
|
||||
- First wait: sync is done when date changes
|
||||
- Debounce: wait for stability
|
||||
- Second wait: confirm sync is still done (in case new sync started during debounce)
|
||||
|
||||
2. **Effects are complementary, not competing:**
|
||||
|
||||
- `createRepeatableTasksAndAddDueToday$` (1000ms debounce, adds due tasks)
|
||||
- `removeOverdueFormToday$` (1000ms debounce, removes overdue)
|
||||
- `ensureTasksDueTodayInTodayTag$` (2000ms debounce, defensive check)
|
||||
- Different purposes, staggered timing is intentional
|
||||
|
||||
3. **Infinite loop explicitly handled** (lines 128-132):
|
||||
```typescript
|
||||
// Exclude subtasks whose parent is already in TODAY
|
||||
// (preventParentAndSubTaskInTodayList$ will remove them anyway,
|
||||
// causing an infinite add/remove loop and phantom sync changes)
|
||||
.filter((task) => !task.parentId || !todayTaskIds.includes(task.parentId))
|
||||
```
|
||||
|
||||
**Note:** Test coverage exists (`task-due.effects.spec.ts`) but is disabled (`xdescribe`) due to Dropbox SDK mocking issues. Tests should be re-enabled when SDK mocking is fixed.
|
||||
|
||||
---
|
||||
|
||||
### 4. Inconsistent Error Handling Across Effects
|
||||
|
||||
| File | Has Error Handling |
|
||||
| -------------------------- | ---------------------------------- |
|
||||
| `short-syntax.effects.ts` | Yes (`catchError()`) |
|
||||
| `task-electron.effects.ts` | No |
|
||||
| `idle.effects.ts` | No |
|
||||
| `tag.effects.ts` | **FIXED** (2025-12-08) - try/catch |
|
||||
|
||||
**Status:** High-priority case (tag.effects.ts async IndexedDB) fixed on 2025-12-08.
|
||||
|
||||
---
|
||||
|
||||
## Medium Priority Issues
|
||||
|
||||
### ~~5. Missing Bounds Checking~~ - FIXED
|
||||
|
||||
**File:** `src/app/root-store/meta/task-shared-meta-reducers/planner-shared.reducer.ts`
|
||||
**Line:** 247
|
||||
**Status:** **FIXED** on 2025-12-08
|
||||
|
||||
Added bounds check for `targetIndex === -1` case. When target task is filtered out (e.g., moving task before itself), task is now appended to end as fallback instead of being inserted at wrong position.
|
||||
|
||||
Regression test added in `planner-shared.reducer.spec.ts`.
|
||||
|
||||
---
|
||||
|
||||
### 6. TOCTOU Race in Compaction
|
||||
|
||||
**File:** `src/app/core/persistence/operation-log/store/operation-log-compaction.service.ts`
|
||||
**Lines:** 66-93
|
||||
|
||||
Time-of-check-time-of-use between `getLastSeq()` and `deleteOpsWhere()`. New ops could be written between these calls.
|
||||
|
||||
---
|
||||
|
||||
### 7. State Capture ID Non-Deterministic
|
||||
|
||||
**File:** `src/app/core/persistence/operation-log/processing/state-change-capture.service.ts`
|
||||
**Lines:** 354-360
|
||||
|
||||
`JSON.stringify(action)` hash is non-deterministic for object property order. Could cause capture ID mismatches.
|
||||
|
||||
---
|
||||
|
||||
### 8. LOCAL_ACTIONS Filter Edge Case
|
||||
|
||||
**File:** `src/app/util/local-actions.token.ts`
|
||||
**Line:** 37
|
||||
|
||||
```typescript
|
||||
return actions$.pipe(filter((action: Action) => !(action as any).meta?.isRemote));
|
||||
```
|
||||
|
||||
Actions without `meta` field pass through (undefined !== true). Remote operations without explicit `isRemote: true` could trigger effects twice.
|
||||
|
||||
---
|
||||
|
||||
### 9. Duplicated TODAY_TAG Logic
|
||||
|
||||
**Locations:**
|
||||
|
||||
- `planner-shared.reducer.ts:158-234`
|
||||
- `short-syntax-shared.reducer.ts:241-309`
|
||||
|
||||
Same logic for "moving to/from today" duplicated. Changes require updates in multiple places.
|
||||
|
||||
**Fix:** Extract to shared helper function.
|
||||
|
||||
---
|
||||
|
||||
### 10. getTag() Throws But Callers Don't Handle
|
||||
|
||||
**File:** `src/app/root-store/meta/task-shared-meta-reducers/task-shared-helpers.ts`
|
||||
**Lines:** 51-80
|
||||
|
||||
Meta-reducers are synchronous and can't catch thrown errors. If `getTag()` throws, entire reducer chain fails.
|
||||
|
||||
**Fix:** Use `getTagOrUndefined()` pattern consistently, or add guards before calling `getTag()`.
|
||||
|
||||
---
|
||||
|
||||
### 11. Async Operations Without Error Handling
|
||||
|
||||
**File:** `src/app/features/tag/store/tag.effects.ts`
|
||||
**Lines:** 101-118
|
||||
|
||||
```typescript
|
||||
tap(async (tagIdsToRemove: string[]) => {
|
||||
await this._taskArchiveService.removeTagsFromAllTasks(tagIdsToRemove);
|
||||
// No error handling, effect completes before async finishes
|
||||
});
|
||||
```
|
||||
|
||||
**Fix:** Use proper async handling with `concatMap` and error boundaries.
|
||||
|
||||
---
|
||||
|
||||
### 12. Orphan Detection Doesn't Verify Project Exists
|
||||
|
||||
**File:** `src/app/root-store/meta/task-shared-meta-reducers/tag-shared.reducer.ts`
|
||||
**Lines:** 105-108
|
||||
|
||||
Checks `!task.projectId` but doesn't verify project still exists (could be deleted in same batch).
|
||||
|
||||
---
|
||||
|
||||
## Testing Gaps
|
||||
|
||||
### Critical: Effects Coverage at 51%
|
||||
|
||||
**19 untested effect files:**
|
||||
|
||||
**Priority 1 (Core):**
|
||||
|
||||
- `src/app/imex/sync/sync.effects.ts` - Core sync engine
|
||||
- `src/app/core-ui/layout/store/layout.effects.ts` - Navigation/panel management
|
||||
- `src/app/features/tasks/store/task-related-model.effects.ts` - Task model sync
|
||||
- `src/app/features/work-context/store/work-context.effects.ts` - Context switching
|
||||
|
||||
**Priority 2 (Features):**
|
||||
|
||||
- `src/app/features/project/store/project.effects.ts`
|
||||
- `src/app/features/tag/store/tag.effects.ts`
|
||||
- `src/app/features/tasks/store/short-syntax.effects.ts`
|
||||
- `src/app/features/tasks/store/task-internal.effects.ts`
|
||||
- `src/app/features/tasks/store/task-ui.effects.ts`
|
||||
- Issue provider effects (caldav, gitlab, jira, open-project)
|
||||
|
||||
---
|
||||
|
||||
### Critical: Disabled Test Files
|
||||
|
||||
- `src/app/root-store/app-state/app-state.effects.spec.ts` - Completely commented out
|
||||
- `src/app/features/note/store/note.effects.spec.ts` - Completely commented out
|
||||
- `src/app/features/boards/store/boards.selectors.spec.ts` - Disabled
|
||||
|
||||
---
|
||||
|
||||
### Missing Meta-Reducer Tests
|
||||
|
||||
- `src/app/root-store/meta/task-shared-meta-reducers/issue-provider-shared.reducer.ts`
|
||||
- `src/app/root-store/meta/task-shared-meta-reducers/task-repeat-cfg-shared.reducer.ts`
|
||||
|
||||
---
|
||||
|
||||
### Missing Integration Tests
|
||||
|
||||
- Race conditions (concurrent append + compaction)
|
||||
- Error recovery (quota exceeded mid-write)
|
||||
- Multi-tab coordination (lock deadlocks)
|
||||
- 3-way conflict resolution
|
||||
- Very large conflict sets (100+ ops on same entity)
|
||||
- Network failure mid-pagination (2 tests marked `pending()`)
|
||||
|
||||
---
|
||||
|
||||
## Architecture Observations
|
||||
|
||||
### Well-Designed Aspects
|
||||
|
||||
1. **Meta-reducer pattern** - Atomic multi-entity operations ensure sync consistency
|
||||
2. **Persistent action pattern** - Explicit `isPersistent: true` enables operation logging
|
||||
3. **LOCAL_ACTIONS token** - Filters remote operations from side-effect triggers
|
||||
4. **Entity adapters** - Normalized state with proper NgRx patterns
|
||||
5. **Self-healing selectors** - `computeOrderedTaskIdsForTag()` dynamically computes membership
|
||||
6. **TODAY_TAG virtual pattern** - Correctly implemented with cleanup for legacy data
|
||||
|
||||
### Concerns
|
||||
|
||||
1. **Meta-reducer ordering** - 10 ordered reducers create tight coupling; order changes break logic
|
||||
2. **Action metadata duplication** - Easy to forget `isPersistent` or use incorrect `opType`
|
||||
3. **Three-way relationships** - `task.tagIds`, `tag.taskIds`, `planner.days` require careful synchronization
|
||||
|
||||
---
|
||||
|
||||
## Prioritized Fix Recommendations
|
||||
|
||||
### Phase 1: Critical (Immediate) - COMPLETED
|
||||
|
||||
| # | Issue | Status |
|
||||
| --- | ------------------ | -------------------------------------- |
|
||||
| 1 | Compaction counter | **FIXED** (2025-12-08) |
|
||||
| 2 | ~~Memory leak~~ | FALSE POSITIVE - `take(1)` auto-cleans |
|
||||
|
||||
### Phase 2: High Priority (Current)
|
||||
|
||||
| # | Issue | Status |
|
||||
| --- | ------------------------------ | --------------------------- |
|
||||
| 1 | Snapshot timing | **FIXED** (2025-12-08) |
|
||||
| 2 | Subtask orphan detection | FALSE POSITIVE (2025-12-08) |
|
||||
| 3 | Race conditions | FALSE POSITIVE (2025-12-08) |
|
||||
| 4 | Error handling standardization | Multiple effects |
|
||||
|
||||
### Phase 3: Add Tests
|
||||
|
||||
| # | Test | Priority |
|
||||
| --- | -------------------------------------------- | -------- |
|
||||
| 1 | Re-enable app-state.effects.spec.ts | Critical |
|
||||
| 2 | sync.effects.spec.ts (new) | Critical |
|
||||
| 3 | Re-enable task-due.effects.spec.ts (fix SDK) | High |
|
||||
| 4 | issue-provider-shared.reducer.spec.ts (new) | High |
|
||||
| 5 | task-repeat-cfg-shared.reducer.spec.ts (new) | High |
|
||||
| 6 | layout.effects.spec.ts (new) | High |
|
||||
|
||||
### Phase 4: Medium Priority
|
||||
|
||||
- ~~Bounds checking in planner-shared.reducer.ts~~ **FIXED** (2025-12-08)
|
||||
- Extract duplicated TODAY_TAG logic
|
||||
- Fix getTag() error handling pattern
|
||||
- ~~Add async error handling in tag.effects.ts~~ **FIXED** (2025-12-08)
|
||||
|
||||
---
|
||||
|
||||
## Related Documentation
|
||||
|
||||
- `docs/ai/sync/operation-log-implementation-review.md` - Detailed operation log review
|
||||
- `docs/ai/operation-log-review-plan.md` - Original review plan
|
||||
- `src/app/core/persistence/operation-log/docs/operation-log-architecture.md` - Architecture docs
|
||||
- `docs/ai/today-tag-architecture.md` - TODAY_TAG virtual pattern
|
||||
|
|
@ -1,145 +0,0 @@
|
|||
# Operation Log Architecture Review & Plan
|
||||
|
||||
**Date:** Monday, December 8, 2025
|
||||
**Status:** Planning
|
||||
**Module:** `src/app/core/persistence/operation-log/`
|
||||
**Last Updated:** December 8, 2025 (added compaction counter bug, validated findings)
|
||||
|
||||
## Executive Summary
|
||||
|
||||
A comprehensive architectural analysis of the `operation-log` module has identified **one remaining critical issue**: a data loss risk in the file-based synchronization mechanism (WebDAV/Dropbox/etc.). The compaction counter bug has been fixed. This document outlines the findings and the proposed implementation plan to address remaining issues.
|
||||
|
||||
## 1. Critical Findings
|
||||
|
||||
### ✅ ~~Critical Bug: Compaction Counter Never Persists~~ - FIXED
|
||||
|
||||
**Severity:** Critical
|
||||
**Location:** `OperationLogStoreService.incrementCompactionCounter` (lines 434-439)
|
||||
**Status:** Fixed on 2025-12-08
|
||||
|
||||
**Original Issue:** When no state cache existed, the function returned `1` without persisting that value. The compaction threshold was never reached.
|
||||
|
||||
**Fix Applied:**
|
||||
|
||||
```typescript
|
||||
if (!cache) {
|
||||
// No state cache yet - create one with counter starting at 1
|
||||
await store.put({ id: 'current', compactionCounter: 1 });
|
||||
await tx.done;
|
||||
return 1;
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 🚨 Data Loss Risk in Manifest Sync (File-Based Providers)
|
||||
|
||||
**Severity:** Critical
|
||||
**Location:** `OperationLogManifestService.uploadRemoteManifest` & `OperationLogUploadService`
|
||||
|
||||
**The Issue:**
|
||||
When uploading the list of operation files (`manifest.json`), the service explicitly forces an overwrite (`revToMatch: null`, `force: true`). This creates a "Lost Update" race condition.
|
||||
|
||||
**Scenario:**
|
||||
|
||||
1. **Client A** reads manifest: `['file1']`.
|
||||
2. **Client B** reads manifest: `['file1']`.
|
||||
3. **Client A** uploads `file2`, writes manifest: `['file1', 'file2']`.
|
||||
4. **Client B** uploads `file3`, writes manifest: `['file1', 'file3']`.
|
||||
|
||||
**Outcome:**
|
||||
`file2` (Client A's data) is orphaned. **Client C** will never see Client A's operations. If `file3` depends on `file2` (e.g., Update Task vs Create Task), Client C will crash or have corrupted state.
|
||||
|
||||
**Recommendation:**
|
||||
Implement Optimistic Locking.
|
||||
|
||||
- `loadRemoteManifest` must return the file's `rev` (revision ID).
|
||||
- `uploadRemoteManifest` must accept `revToMatch`.
|
||||
- Implement a retry loop: if upload fails due to conflict (412), re-download, merge file lists (union), and retry.
|
||||
|
||||
---
|
||||
|
||||
### ⚠️ Performance Bottleneck in Compaction
|
||||
|
||||
**Severity:** High
|
||||
**Location:** `OperationLogStoreService.deleteOpsWhere`
|
||||
|
||||
**The Issue:**
|
||||
Compaction deletes old operations based on the `appliedAt` timestamp. However, the IndexedDB store only has indexes for `byId` and `bySyncedAt`.
|
||||
|
||||
**Impact:**
|
||||
Compaction performs a full table scan of the `ops` store. As the log grows (the very problem compaction solves), this operation becomes slower ($O(N)$), potentially causing UI jank or locking timeouts during the critical compaction phase.
|
||||
|
||||
**Recommendation:**
|
||||
Update `DB_VERSION` and add an index for `appliedAt` in `OperationLogStoreService.init()`.
|
||||
|
||||
---
|
||||
|
||||
### ⚠️ UX Deadlock in Conflict Resolution
|
||||
|
||||
**Severity:** Medium
|
||||
**Location:** `ConflictResolutionService`
|
||||
|
||||
**The Issue:**
|
||||
If the user cancels the Conflict Resolution dialog (e.g., clicks outside or presses Escape), the service returns without applying any changes or advancing the sync state.
|
||||
|
||||
**Impact:**
|
||||
The sync cycle aborts. The next sync attempt will likely hit the exact same conflict, prompting the user again. This creates a "stuck" state where the user is pestered indefinitely until they resolve it.
|
||||
|
||||
**Recommendation:**
|
||||
Treat "Cancel" as "Defer" (pause sync temporarily) or provide a "Skip/Ignore" option that allows other non-conflicting ops to proceed (with strict dependency checks). At minimum, log a clear warning that sync is stalled.
|
||||
|
||||
## 2. Architectural Observations
|
||||
|
||||
- **Redundant Logic:** `ConflictResolutionService` (Step 3) duplicates the logic of applying operations found in `OperationApplierService` (error handling, snackbars, etc.). This violates DRY and risks inconsistent behavior.
|
||||
- _Plan:_ Refactor `ConflictResolutionService` to delegate fully to `OperationApplier`.
|
||||
- **Locking Integrity:** `OperationLogEffects` (writer) and `OperationLogCompactionService` (cleaner) correctly use the `sp_op_log` lock, preventing race conditions during snapshot creation.
|
||||
- **Fresh Client Safety:** The `isWhollyFreshClient` check in `OperationLogSyncService` correctly safeguards against accidental remote overwrites by empty clients.
|
||||
- **Cache Self-Invalidation:** The `_appliedOpIdsCache` uses sequence-based invalidation which works correctly (not a bug as previously thought).
|
||||
|
||||
## 3. Missing Test Coverage
|
||||
|
||||
The current test suite lacks coverage for distributed system edge cases:
|
||||
|
||||
1. **Manifest Concurrency:** No tests simulate two clients updating the manifest simultaneously to reproduce the lost update scenario.
|
||||
2. **Compaction Race:** No tests verify that `append` waits for `compact` (locking verification).
|
||||
3. **Partial Sync:** No tests for scenarios where a client downloads `file1` but fails to download `file2` (network error).
|
||||
4. **Compaction Counter Persistence:** No test verifies counter is persisted when cache doesn't exist.
|
||||
|
||||
## 4. Implementation Plan
|
||||
|
||||
### Phase 1: Fix Critical Bugs (High Priority)
|
||||
|
||||
1. **Fix Compaction Counter Bug:**
|
||||
|
||||
- **File:** `src/app/core/persistence/operation-log/store/operation-log-store.service.ts`
|
||||
- **Lines:** 428-447
|
||||
- **Change:** Persist counter=1 when no cache exists.
|
||||
|
||||
2. **Modify `OperationLogManifestService`:**
|
||||
|
||||
- Update `loadRemoteManifest` to return `{ manifest, rev }`.
|
||||
- Update `uploadRemoteManifest` to accept `revToMatch`.
|
||||
|
||||
3. **Update `OperationLogUploadService`:**
|
||||
- Implement the read-modify-write loop with retries for `_uploadPendingOpsViaFiles`.
|
||||
- Logic: `while (retries < max) { load(rev) -> merge -> upload(rev) }`
|
||||
|
||||
### Phase 2: Optimize Store Indexes
|
||||
|
||||
1. **Update `OperationLogStoreService`:**
|
||||
- Increment `DB_VERSION`.
|
||||
- Add `opStore.createIndex('byAppliedAt', 'appliedAt')` in the upgrade callback.
|
||||
- Optimize `deleteOpsWhere` to use `IDBKeyRange.upperBound` on the new index.
|
||||
|
||||
### Phase 3: Refactor Conflict Resolution
|
||||
|
||||
1. **Refactor `ConflictResolutionService`:**
|
||||
- Remove custom batch application logic.
|
||||
- Use `OperationApplierService.applyOperations` directly.
|
||||
|
||||
### Phase 4: Enhance Test Suite
|
||||
|
||||
1. Create `manifest-concurrency.spec.ts` to reproduce the lost update scenario and verify the fix.
|
||||
2. Create `compaction-counter.spec.ts` to verify counter persistence.
|
||||
3. Add performance benchmarks for compaction with large datasets.
|
||||
|
|
@ -1,123 +0,0 @@
|
|||
# TODAY_TAG Architecture
|
||||
|
||||
This document explains the dual-system architecture for task ordering in Super Productivity and why TODAY_TAG is a "virtual tag."
|
||||
|
||||
## Overview
|
||||
|
||||
Super Productivity uses two complementary systems for task ordering:
|
||||
|
||||
| System | Purpose | Storage Location |
|
||||
| ----------------------- | --------------------------- | ------------------------------- |
|
||||
| `TODAY_TAG.taskIds` | Order tasks for today | `tag.entities['TODAY'].taskIds` |
|
||||
| `planner.days[dateStr]` | Order tasks for future days | `planner.days` |
|
||||
|
||||
This is intentional, not technical debt. The dual system keeps move operations (drag/drop, keyboard shortcuts) simple and uniform.
|
||||
|
||||
## The "Virtual Tag" Pattern
|
||||
|
||||
TODAY_TAG is special:
|
||||
|
||||
1. **Should NOT be in `task.tagIds`** - Unlike regular tags, TODAY_TAG must never appear in a task's `tagIds` array
|
||||
2. **Only exists in `tag.taskIds`** - TODAY_TAG.taskIds stores the ordered list of task IDs for today
|
||||
3. **Acts as a work context** - Users can click TODAY in the sidebar to view today's tasks
|
||||
|
||||
This pattern was enforced in commit `ca08724bd` ("fix(sync): remove TODAY_TAG from task.tagIds").
|
||||
|
||||
### Why This Pattern?
|
||||
|
||||
Regular tags use a "board-style" pattern:
|
||||
|
||||
- `task.tagIds` = source of truth for membership
|
||||
- `tag.taskIds` = ordering only
|
||||
|
||||
TODAY_TAG cannot follow this pattern because:
|
||||
|
||||
- Tasks belong to "today" based on `task.dueDay`, not `task.tagIds`
|
||||
- Adding TODAY_TAG to `task.tagIds` would create sync conflicts
|
||||
- The planner already tracks day membership via `task.dueDay`
|
||||
|
||||
## Code Paths
|
||||
|
||||
### TODAY_TAG.taskIds is modified by:
|
||||
|
||||
| File | Lines | Purpose |
|
||||
| ----------------------------------- | ------- | --------------------------------------- |
|
||||
| `task-shared-scheduling.reducer.ts` | 29-264 | scheduleTaskWithTime, planTasksForToday |
|
||||
| `planner-shared.reducer.ts` | 91-151 | Transfer to/from today |
|
||||
| `task-shared-crud.reducer.ts` | 120-156 | Add task with dueDay === today |
|
||||
| `short-syntax-shared.reducer.ts` | 215-232 | @today syntax |
|
||||
| `tag.reducer.ts` | 257-361 | Move actions (drag/drop, Ctrl+↑/↓) |
|
||||
|
||||
### planner.days is modified by:
|
||||
|
||||
| File | Lines | Purpose |
|
||||
| ----------------------------------- | ------- | ------------------------------------------- |
|
||||
| `planner.reducer.ts` | 155-183 | planTaskForDay (skips today) |
|
||||
| `planner-shared.reducer.ts` | 47-82 | Transfer between days |
|
||||
| `task-shared-scheduling.reducer.ts` | 206-226 | Remove from planner when planning for today |
|
||||
|
||||
## Why Not Unify in Planner?
|
||||
|
||||
We considered making `planner.days[todayStr]` the source of truth for today's ordering. However:
|
||||
|
||||
1. **Move handlers would need special routing:**
|
||||
|
||||
```typescript
|
||||
// tag.reducer.ts would need:
|
||||
if (workContextId === TODAY_TAG.id) {
|
||||
// Route to planner.days[todayStr]
|
||||
} else {
|
||||
// Update tag.taskIds
|
||||
}
|
||||
```
|
||||
|
||||
2. **Current architecture is simpler:**
|
||||
|
||||
```typescript
|
||||
// All tags (including TODAY) handled uniformly:
|
||||
if (workContextType === 'TAG') {
|
||||
// Update tag.taskIds - works for all tags
|
||||
}
|
||||
```
|
||||
|
||||
3. **Cost/benefit doesn't favor refactoring:**
|
||||
- 8+ files would need changes
|
||||
- Migration required for existing data
|
||||
- Conceptual benefit only (no functional improvement)
|
||||
|
||||
## Guidance for New Features
|
||||
|
||||
### When adding a task to today:
|
||||
|
||||
1. Set `task.dueDay = getDbDateStr()`
|
||||
2. Add task ID to `TODAY_TAG.taskIds` (meta-reducer handles this)
|
||||
3. Do NOT add `TODAY_TAG.id` to `task.tagIds`
|
||||
|
||||
### When moving a task away from today:
|
||||
|
||||
1. Update `task.dueDay` to the new day
|
||||
2. Remove task ID from `TODAY_TAG.taskIds`
|
||||
3. Add to `planner.days[newDay]` if not today
|
||||
|
||||
### When implementing move/reorder operations:
|
||||
|
||||
- For TODAY context: operations go through `tag.reducer.ts` like any other tag
|
||||
- The selector `selectTodayTaskIds` computes the ordered list
|
||||
|
||||
## Key Selectors
|
||||
|
||||
```typescript
|
||||
// Get ordered today task IDs (uses board-style pattern internally)
|
||||
selectTodayTaskIds;
|
||||
|
||||
// Get active work context (TODAY_TAG or project)
|
||||
selectActiveWorkContext;
|
||||
```
|
||||
|
||||
## Related Files
|
||||
|
||||
- `src/app/features/tag/tag.const.ts` - TODAY_TAG definition
|
||||
- `src/app/features/planner/store/planner.reducer.ts` - Planner state
|
||||
- `src/app/features/tag/store/tag.reducer.ts` - Tag state and move handlers
|
||||
- `src/app/features/work-context/store/work-context.selectors.ts` - Work context selectors
|
||||
- `src/app/root-store/meta/task-shared-meta-reducers/` - Meta-reducers for atomic updates
|
||||
|
|
@ -1,80 +0,0 @@
|
|||
# Reminder Refactoring Plan
|
||||
|
||||
## Objective
|
||||
|
||||
Refactor the Reminder system to eliminate the standalone `reminders` model and persistence file. Instead, reminder data (specifically `remindAt`) will be stored directly on the `Task` model. Reminders for Notes will be discontinued.
|
||||
|
||||
## Motivation
|
||||
|
||||
1. **Operation Log Consistency**: Currently, reminders are stored independently and changes bypass the main NgRx/Operation Log system used for sync. Integrating them into `Task` ensures all reminder changes are properly tracked and synced.
|
||||
2. **Performance**: Reducing the number of files synced (removing `reminders.json`) improves performance, especially for WebDAV users.
|
||||
3. **Simplicity**: Reduces complexity by removing an entire state slice and its associated management logic.
|
||||
|
||||
## Proposed Architecture
|
||||
|
||||
### 1. Data Model Changes
|
||||
|
||||
**Remove**:
|
||||
|
||||
- `Reminder` model (standalone entity).
|
||||
- `RecurringConfig` (found to be unused).
|
||||
- `Note` reminders support.
|
||||
|
||||
**Update**:
|
||||
|
||||
- **Task Model**:
|
||||
- Remove `reminderId: string`.
|
||||
- Add `remindAt: number | null` (timestamp).
|
||||
|
||||
### 2. Persistence Layer
|
||||
|
||||
- Remove `reminders` from `BASE_MODEL_CFGS` in `src/app/core/persistence/persistence.const.ts`.
|
||||
- Ensure `Task` persistence (which already exists) captures the new `remindAt` field.
|
||||
|
||||
### 3. Service Layer (`ReminderService`)
|
||||
|
||||
The `ReminderService` will transition from a state-holding service to a facade that interacts with the Task Store.
|
||||
|
||||
- **State Management**: Remove local `_reminders` array, `BehaviorSubject`, and manual persistence calls (`_saveModel`, `loadFromDatabase`).
|
||||
- **Data Source**:
|
||||
- Inject `Store`.
|
||||
- Create a selector `selectTasksWithReminders` in Task Store.
|
||||
- `onRemindersActive$` will subscribe to this selector (mapped to the format expected by the worker).
|
||||
- **Actions**:
|
||||
- `addReminder(taskId, remindAt)`: Dispatch `updateTask({ id: taskId, changes: { remindAt } })`.
|
||||
- `updateReminder(taskId, changes)`: Dispatch `updateTask({ id: taskId, changes: { remindAt: changes.remindAt } })`.
|
||||
- `removeReminder(taskId)`: Dispatch `updateTask({ id: taskId, changes: { remindAt: null } })`.
|
||||
- `snooze(taskId, snoozeTime)`: Dispatch `updateTask` with new calculated time.
|
||||
- **Worker Integration**:
|
||||
- Continue to use `reminder.worker.ts`.
|
||||
- The service will map `Task` objects to the lightweight structure expected by the worker (id, title, remindAt, type='TASK').
|
||||
|
||||
### 4. Migration Strategy
|
||||
|
||||
A migration must run **once** on startup to transfer existing reminder data to tasks.
|
||||
|
||||
**Migration Logic**:
|
||||
|
||||
1. Load the legacy `reminders` state (from `reminders.json` or IndexedDB).
|
||||
2. Load the `task` state.
|
||||
3. Iterate through all legacy reminders:
|
||||
- If `type === 'NOTE'`: Discard (log warning if needed).
|
||||
- If `type === 'TASK'`: Find the corresponding Task by `relatedId`. Update its `remindAt` property with the reminder's `remindAt`.
|
||||
4. Save the updated `task` state.
|
||||
5. Delete/Clear the `reminders` state to prevent future conflicts.
|
||||
|
||||
### 5. UI & Cleanup
|
||||
|
||||
- **Dialogs**: Update `DialogViewTaskReminders` and "Add/Edit Reminder" dialogs to read/write `task.remindAt` instead of looking up a separate reminder object.
|
||||
- **Note UI**: Remove "Add Reminder" options from Note context menus and buttons.
|
||||
- **Code Removal**: Delete unused `reminder.model.ts` (or reduce to Worker interface), remove Note-specific reminder logic.
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
1. **Backup**: Ensure data is backed up before applying changes.
|
||||
2. **Model Update**: specificy `remindAt` in `TaskCopy` / `Task`.
|
||||
3. **Migration Script**: Implement the logic to merge `reminders` into `tasks`.
|
||||
4. **Service Refactoring**: Rewrite `ReminderService` to use `TaskStore`.
|
||||
5. **UI Adjustments**: Fix compilation errors in components accessing `reminderId`.
|
||||
6. **Cleanup**: Remove dead code and `reminders` persistence config.
|
||||
7. **Verification**: Test adding, snoozing, completing, and removing reminders. Verify sync behavior.
|
||||
|
|
@ -133,7 +133,7 @@ export const removeTimeSpent = createAction(
|
|||
entityType: 'TASK',
|
||||
entityId: taskProps.id,
|
||||
opType: OpType.Update,
|
||||
} as PersistentActionMeta,
|
||||
} satisfies PersistentActionMeta,
|
||||
}),
|
||||
);
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue