diff --git a/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/.openspec.yaml b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/.openspec.yaml new file mode 100644 index 00000000..eb139cca --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-19 diff --git a/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/design.md b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/design.md new file mode 100644 index 00000000..934612b0 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/design.md @@ -0,0 +1,80 @@ +# Design: fix-recurring-completion-error-masking + +Context: driven by FR1–FR7 from `proposal.md`. + +## Context + +`TaskService.complete()` (`packages/client/src/services/TaskService.ts`) parses the repeat rule and, on success, calculates the next date and creates/updates a recurring copy — all inside one `try/catch`. Two distinct failure modes currently collapse into one return value: + +1. `parseRepeatRule()` returns `null` → early return `{ status: 'skipped_invalid_rule' }` (the rule genuinely can't be parsed). +2. Anything *after* a successful parse throws (date math, `findHiddenRecurringTask`, `createRecurringCopy` → `create()` → `generateTopKey()`, checklist copy, a Dexie write) → the broad `catch` returns the **same** `{ status: 'skipped_invalid_rule' }`. + +The `RecurringResult` union has three members (`created` | `skipped_invalid_rule` | `not_recurring`). Three live completion callers (`useTasks`, `useInboxTasks`, `useTaskMutations`) map `skipped_invalid_rule` → a `repeat_rule_invalid` alert, which renders the "Rule not recognized / could not be recognized" dialog. So mode (2) tells the user their (valid) rule is broken. A fourth hook, `useTask`, carries the same block but is dead code (no importers outside its own test) — removed by this change, see D5. + +The `invalid-repeat-rule-detection` and `repeating-tasks` specs both currently *codify* this conflation ("Exception during copy creation returns skipped status"), so the specs must change too, not just the code. + +## Goals / Non-Goals + +**Goals:** +- Give the exception path its own status so callers can distinguish it and not lie to the user. +- Keep `skipped_invalid_rule` a truthful signal (parse-failure only). +- Preserve every other observable of completion: the task is still completed, the error is still swallowed and logged. + +**Non-Goals:** +- No new user-facing alert type for the error path (NG1) — logged only. +- No changes to pull-time detection, the detail-panel warning, or recurrence math. +- Not fixing the underlying throwers. + +## Decisions + +### D1 — Add a fourth union member `error_creating_copy` (vs. reusing `skipped_invalid_rule` or a boolean flag) + +Extend `RecurringResult` with `{ status: 'error_creating_copy' }`. The early `if (!rule)` return keeps `skipped_invalid_rule`; the `catch` block returns `error_creating_copy` (still `console.error`-logging, still returning `finalCompletedTask`). + +- **Why a new member:** it makes the two causes type-distinct, so every `switch`/`if` over `recurringResult.status` is forced (by the discriminated union) to consider the new case — surfacing exactly the four caller sites that need review. Reusing `skipped_invalid_rule` can't fix the mislabel; a side boolean (`wasError`) would bypass exhaustiveness and re-introduce drift. +- **Naming:** `error_creating_copy` reads as "the copy couldn't be created due to an error", parallel to the existing snake_case statuses. + +### D2 — Callers ignore `error_creating_copy` for alerting (no new alert) + +The live hooks currently do `if (recurringResult.status === 'skipped_invalid_rule') addAlerts([{ type: 'repeat_rule_invalid', ... }])`. That condition is already exact-match on `skipped_invalid_rule`, so **it needs no change to stop firing** for `error_creating_copy` — but each site gets a characterization test pinning "error status → no alert", and a short comment, so the behavior is intentional and guarded rather than incidental. + +- **Alternative considered:** add a truthful generic alert for the error path. Deferred (NG1/Q1) to keep the change surgical; silent-but-logged matches the pre-existing "don't interrupt completion" intent, only without the *wrong* message. + +### D3 — Extract a shared `useTaskCompletionAlerts` helper (FR5, FR6) + +The impact scan (task 1.1) found the `if (recurringResult.status === 'skipped_invalid_rule') addAlerts(...)` block copy-pasted, with minor variations, across four completion hooks (`useTasks`, `useInboxTasks`, `useTaskMutations`, plus the dead `useTask` — removed per D5) — and found a further call site, `SearchPage.tsx`, that discards `recurringResult` entirely and never alerts at all. This is the concrete "same action, different behavior per page" symptom the user flagged. + +Extract a single helper, `packages/client/src/hooks/useTaskCompletionAlerts.ts` (100–200 lines, alongside the existing `useMutationHelpers.ts`), that takes `recurringResult` + the task name and calls `addAlerts` exactly once, with the `error_creating_copy` case producing no alert (per D2) and `skipped_invalid_rule` producing the existing `repeat_rule_invalid` alert. The three live hooks replace their inline block with a call to this helper; `SearchPage.tsx`'s `handleCompleteTask` is updated to call it too, closing the silent-failure gap. + +- **Why extract now, not defer:** the duplication is what let the sites drift and let `SearchPage.tsx` skip alerting entirely undetected. Fixing FR1–FR4 without touching the duplication would just add one more place (`error_creating_copy` handling) to keep in sync by hand. +- **Why not a bigger refactor:** `useGoalTasks.ts` already composes `useTaskMutations(...)` rather than reimplementing it — this follows the same existing composition pattern instead of introducing a new architecture (e.g. no `hooks/index.ts` barrel, no change to how `completeTask` is threaded through components). See NG5. +- **Naming:** `useTaskCompletionAlerts` — it's a hook only insofar as it may call `useAlerts()` internally; call sites use it as `raiseCompletionAlerts(recurringResult, taskName)` or similar plain function returned from the hook. + +### D4 — Spec deltas mark the intent change + +`invalid-repeat-rule-detection` and `repeating-tasks` deltas convert the "exception → skipped_invalid_rule" scenarios to "exception → error_creating_copy" and add "error path shows no invalid-rule alert". This is the source-of-truth change; the code follows it. + +### D5 — Delete the dead `useTask` hook instead of refactoring it (FR7) + +A follow-up usage scan (recorded in task 2.1) found `useTask.ts` has no importers anywhere in `packages/client/src` — only `useTask.test.ts` references it. No page or component consumes the hook; its functionality is covered by `useTaskMutations`/`useTasks`. + +Delete `useTask.ts` + `useTask.test.ts` early (section 2 of tasks, before the union extension), so FR3/FR5 and the mutation-testing scope cover only live code. + +- **Alternative — refactor it along with the live hooks:** rejected; spending TDD + mutation-testing effort (>=95% score) on code no user path can reach is waste. +- **Alternative — a separate removal change:** rejected; the file is one of the copy-pasted alert sites, squarely inside this change's blast radius. Deleting it first shrinks this change instead of coordinating two changes over the same lines. + +## Risks / Trade-offs + +- [A consumer does a non-exhaustive `switch` on `recurringResult.status` and silently drops the new case] → TypeScript's discriminated union + the existing `not_recurring`/`created` handling make the live call sites explicit; a repo grep for `recurringResult.status` / `skipped_invalid_rule` in the tasks confirms every consumer is reviewed. +- [`useTask` is secretly used (dynamic import, planned feature) and its deletion breaks something] → the grep in task 2.1 (word-boundary, whole `src`) found zero importers; `pnpm run build` + the unit suite after deletion are the guard. If a future feature needs a single-task hook, it can be recreated on top of `useTaskCompletionAlerts`. +- [Silent failure: the next occurrence isn't created and the user sees nothing] → Same visibility as today for the error path (it was already swallowed); the only change is removing a *false* explanation. Q1 tracks a future truthful alert. +- [Type change ripples to the in-memory adapter or contract types] → `RecurringResult` is a client-local type in `TaskService.ts`; verify no re-export in `packages/contract`. If it is shared, extend there too (task 1.x covers the grep). +- [`SearchPage.tsx` behavior changes user-visibly for the first time — a genuinely invalid rule now alerts there when it silently didn't before] → intentional per FR6/U3; guarded by a characterization test showing prior silent behavior, then a test showing the new alert. + +## Migration Plan + +Pure additive type change + branch split, plus an extraction of existing duplicated logic into one helper (no behavior change to the three live hooks; `SearchPage.tsx` gains alerting it previously lacked) and deletion of the dead `useTask` hook (no runtime impact — nothing imports it). No data migration, no persisted-shape change, no server change. Rollback = revert the commit. Deploys with the normal client build. + +## Open Questions + +- Q1 (from proposal): whether `error_creating_copy` should later surface a truthful "couldn't create the next occurrence" alert. Out of scope here. diff --git a/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/proposal.md b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/proposal.md new file mode 100644 index 00000000..131a2d29 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/proposal.md @@ -0,0 +1,108 @@ +# fix-recurring-completion-error-masking + +## Why + +`TaskService.complete()` wraps recurring-copy creation in a broad `try/catch` that returns `{ status: 'skipped_invalid_rule' }` for **any** thrown error — even when the `repeat_rule` parsed fine and the failure was something else entirely (a DB write error, an invalid `sort_order` key, etc.). Completion hooks then show the user a "Rule not recognized / could not be recognized" alert, blaming the repeat rule for an unrelated failure. During the `fix-stale-sync-overwrites` work this misleading dialog actively masked the real cause of a failure (an invalid seeded sort key throwing in `generateKeyBetween`), costing debugging time and confusing the user with a wrong diagnosis. + +## What Changes + +- **MODIFIED**: `TaskService.complete()` returns a new, distinct `recurringResult` status for an unexpected exception during copy creation, separate from the genuine "rule failed to parse" case. `skipped_invalid_rule` is returned **only** when `parseRepeatRule()` returns `null`. +- **MODIFIED**: Completion callers (`useTasks`, `useInboxTasks`, `useTaskMutations`) no longer raise the `repeat_rule_invalid` alert for the exception path — that alert stays reserved for genuinely invalid rules. The exception is still caught (completion never fails) and logged. +- **ADDED**: A shared completion-alert helper (`useTaskCompletionAlerts` or equivalent) replaces the three live copy-pasted `if (recurringResult.status === 'skipped_invalid_rule') addAlerts(...)` blocks in `useTasks`, `useInboxTasks`, `useTaskMutations`. Scope expansion decided after the impact scan (task 1.1) found this duplication — the same completion action was behaving inconsistently across call sites because the alerting logic was copy-pasted rather than centralized. +- **ADDED**: `SearchPage.tsx`'s `handleCompleteTask` — previously discarding the `complete()` return value entirely with no alert handling — now routes through the same shared helper, closing a silent-failure gap discovered during the impact scan. +- **REMOVED**: The dead `useTask` hook (`packages/client/src/hooks/useTask.ts` + `useTask.test.ts`) is deleted. A follow-up usage scan found it is imported nowhere in the app — only its own test references it — so it carried a fourth copy of the alert block without affecting any user-facing behavior. Deleting it (instead of refactoring dead code) shrinks the completion call-site list to three hooks plus `SearchPage.tsx`. +- **UNCHANGED (guarded by tests)**: Completion always succeeds regardless of `recurringResult`; the pull-time invalid-rule detection (`SyncProvider` → `filterTaskNamesWithInvalidRepeatRules`) and the task-detail-panel "Rule not recognized" warning are untouched; recurrence math is untouched. + +## Capabilities + +### New Capabilities + +(none) + +### Modified Capabilities + +- `invalid-repeat-rule-detection`: "Completion returns discriminated union for recurring result" — the exception-during-copy-creation path returns a distinct status (not `skipped_invalid_rule`); "Alert shown on completion when recurring copy skipped due to invalid rule" — the `repeat_rule_invalid` alert fires only for genuinely invalid rules, not for the exception path. +- `repeating-tasks`: "System creates a recurring copy on task completion" — the `recurringResult` union gains a status for the copy-creation-error path; `skipped_invalid_rule` no longer covers "an exception occurred during copy creation". + +## Goals + +- G1: A completion where the repeat rule is valid but copy creation throws never tells the user their repeat rule is invalid. +- G2: `skipped_invalid_rule` (and its "Rule not recognized" alert) is a truthful signal — it appears if and only if `parseRepeatRule()` returned `null`. +- G3: Completion behavior is otherwise byte-for-byte unchanged: the task is still marked complete, the error is still swallowed (never interrupts completion) and logged. +- G4: Every completion call site reacts to `recurringResult` the same way, through one shared helper — no call site independently reimplements or omits the alert logic. + +## Non-Goals + +- NG1: No new user-facing alert/dialog for the copy-creation-failure path. The unexpected error is logged (`console.error`) and completion proceeds silently; surfacing a dedicated "couldn't create the next occurrence" alert is a possible follow-up, out of scope here. +- NG2: No change to pull-time invalid-rule detection or the task-detail-panel warning label. +- NG3: No change to recurrence math, skip logic, or next-date/appear-date calculation. +- NG4: Not fixing the underlying throwers (e.g. invalid `sort_order` keys) — this change only stops mislabeling them; the sort-key seeding fix already landed in `fix-stale-sync-overwrites`. +- NG5: No broader refactor of hook architecture, no `hooks/index.ts` barrel introduction, no change to how `completeTask`/`onComplete` is passed through components (`TaskItem`, `TaskList`, detail pages) — those remain plain passthrough. Only the alert-reaction logic is extracted. + +## Users & Scenarios + +- U1: A user completes a recurring task whose repeat rule is perfectly valid, but an internal error prevents the next occurrence from being created. The task is marked complete and the user is NOT shown a false "Rule not recognized" message. +- U2: A user completes a task whose repeat rule really is corrupt (unparseable). They still see the truthful "Rule not recognized" alert — unchanged from today. +- U3: A user completes a task with a genuinely invalid repeat rule from the search results page. They now see the same truthful "Rule not recognized" alert they'd see from any other page, instead of silence. + +## Requirements + +### Functional + +- FR1: `TaskService.complete()` SHALL return `{ status: 'skipped_invalid_rule' }` only when `parseRepeatRule(existingTask.repeat_rule)` returns `null`. +- FR2: When an exception is thrown after the rule parsed successfully (during next-date calculation or copy creation/update), `TaskService.complete()` SHALL catch it, log it via `console.error`, and return a distinct status `{ status: 'error_creating_copy' }` (name finalized in design). The completed task SHALL still be returned and marked complete. +- FR3: Completion callers that currently map `skipped_invalid_rule` → `repeat_rule_invalid` alert (`useTasks`, `useInboxTasks`, `useTaskMutations`) SHALL NOT raise that alert for `error_creating_copy`. +- FR4: The `RecurringResult` discriminated union SHALL be extended (not repurposed) so exhaustive `switch`/`if` handling stays type-safe; existing `created` / `not_recurring` handling is unchanged. +- FR5: The `skipped_invalid_rule` → `repeat_rule_invalid` alert logic SHALL be extracted into one shared helper, used by all completion call sites (`useTasks`, `useInboxTasks`, `useTaskMutations`), replacing the three independent copy-pasted implementations. +- FR6: `SearchPage.tsx`'s task-completion handler SHALL use the same shared helper, so a genuinely invalid rule raises the `repeat_rule_invalid` alert there too, consistent with every other completion entry point. +- FR7: The dead `useTask` hook (`useTask.ts`, `useTask.test.ts`) SHALL be deleted, after re-verifying via grep (recorded in tasks) that nothing outside its own test imports it; `pnpm run build` and the unit suite SHALL stay green after deletion. + +### Non-Functional + +#### Performance + +- Not applicable — no new queries or loops. + +#### Accessibility + +- Not applicable — no new UI; a misleading alert is suppressed, not added. + +#### Responsive + +- Not applicable — no UI layout change. + +## UX Acceptance Criteria + +- UX1: After completing a recurring task whose rule is valid, the user never sees a "Rule not recognized" / "Repeat rule issue" alert caused by an unrelated internal error. +- UX2: After completing a task whose rule is genuinely unparseable, the "Rule not recognized" alert still appears exactly as before. +- UX3: Completing a task with a genuinely invalid rule from the search page now shows the same "Rule not recognized" alert as everywhere else, instead of no feedback. + +## UI States Matrix + +No UI states change. The only user-visible effect is the ABSENCE of a wrong alert on the exception path; existing loading/error/empty/offline states are unaffected. + +## Behavior + +Scenarios live in feature files tagged `@fix-recurring-completion-error-masking @FR-X`: + +- `packages/client/src/test/features/repeating_tasks/complete_copy_error.feature` — FR1, FR2, FR3 (unit BDD, vitest-cucumber): valid-rule-but-copy-throws → `error_creating_copy`, no invalid-rule alert; unparseable-rule → `skipped_invalid_rule` + alert. + +## Visual Reference + +Not applicable — no UI changes. + +## Affected IA + +No changes. + +## Success Metrics + +- M1: Unit test proves an exception thrown during copy creation with a valid rule yields `error_creating_copy` and raises no `repeat_rule_invalid` alert. +- M2: Existing `skipped_invalid_rule` scenarios (parse returns null) stay green unchanged — the truthful alert path is preserved. +- M3: Mutation score on the changed `complete()` catch/branch and the caller alert mapping >= 95% (minimum acceptable 90%). +- M4: All four completion call sites (`useTasks`, `useInboxTasks`, `useTaskMutations`, `SearchPage.tsx`) call the same shared helper — zero duplicated `if (recurringResult.status === 'skipped_invalid_rule')` blocks remain (verified by grep in task 7.6). +- M5: `useTask.ts` and `useTask.test.ts` are absent from the codebase; a grep for `useTask` (word-boundary, excluding `useTasks`/`useTaskMutations`/`useTaskCompletionAlerts`) over `packages/client/src` returns zero matches (FR7). + +## Open Questions + +- Q1: Should `error_creating_copy` eventually surface a truthful, generic "couldn't create the next occurrence" alert instead of failing silently? Deferred to NG1; revisit if users report silent failures. diff --git a/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/specs/invalid-repeat-rule-detection/spec.md b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/specs/invalid-repeat-rule-detection/spec.md new file mode 100644 index 00000000..a112eb95 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/specs/invalid-repeat-rule-detection/spec.md @@ -0,0 +1,43 @@ +# Capability: Invalid Repeat Rule Detection (delta) + +## MODIFIED Requirements + +### Requirement: Completion returns discriminated union for recurring result + +`TaskService.complete()` SHALL return `recurringResult` as a discriminated union with four statuses: `created` (with the new task), `skipped_invalid_rule` (`repeat_rule` is non-empty but `parseRepeatRule()` returned `null`), `error_creating_copy` (the rule parsed successfully but an exception was thrown while calculating dates or creating/updating the copy), `not_recurring` (no `repeat_rule`). The `try/catch` block SHALL still prevent completion from failing, and SHALL log any caught exception to the console. `skipped_invalid_rule` SHALL be returned ONLY for a genuine parse failure — never for a thrown exception. + +#### Scenario: Successful recurring copy creation +- **WHEN** user completes a task with valid repeat_rule +- **THEN** `recurringResult` has status "created" and includes the new task + +#### Scenario: Invalid rule returns skipped status +- **WHEN** user completes a task with invalid repeat_rule (parsing returns null) +- **THEN** `recurringResult` has status "skipped_invalid_rule" +- **AND** the task is still marked as completed + +#### Scenario: Non-recurring task returns not_recurring status +- **WHEN** user completes a task with empty repeat_rule +- **THEN** `recurringResult` has status "not_recurring" + +#### Scenario: Exception during copy creation returns error status +- **WHEN** user completes a task with valid repeat_rule but copy creation throws +- **THEN** `recurringResult` has status "error_creating_copy" +- **AND** the task is still marked as completed +- **AND** the error is logged to console + +### Requirement: Alert shown on completion when recurring copy skipped due to invalid rule + +When `recurringResult.status === 'skipped_invalid_rule'`, the system SHALL add an alert of type `repeat_rule_invalid` with the completed task's name, shown via AlertProvider. For any other status — including `error_creating_copy` — the system SHALL NOT add a `repeat_rule_invalid` alert on completion. + +#### Scenario: Completion of task with invalid rule shows alert +- **WHEN** user completes task "Buy groceries" with invalid repeat_rule +- **THEN** an alert is shown with type "repeat_rule_invalid" containing task name "Buy groceries" + +#### Scenario: Completion of task with valid rule shows no alert +- **WHEN** user completes task "Buy groceries" with valid repeat_rule +- **THEN** no repeat_rule_invalid alert is shown + +#### Scenario: Completion where copy creation errors shows no invalid-rule alert +- **WHEN** user completes task "Buy groceries" with a valid repeat_rule but copy creation throws +- **THEN** `recurringResult` has status "error_creating_copy" +- **AND** no repeat_rule_invalid alert is shown diff --git a/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/specs/repeating-tasks/spec.md b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/specs/repeating-tasks/spec.md new file mode 100644 index 00000000..117dac73 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/specs/repeating-tasks/spec.md @@ -0,0 +1,39 @@ +# Capability: Repeating Tasks (delta) + +## MODIFIED Requirements + +### Requirement: System creates a recurring copy on task completion + +When a task with repeat_rule is completed, the system MUST: parse the repeat_rule, calculate next_date and appear_date, create or update a recurring copy. The `complete()` method SHALL return a `recurringResult` discriminated union instead of `recurring: Task | null`: +- `{ status: 'created'; task: Task }` — copy created or updated successfully +- `{ status: 'skipped_invalid_rule' }` — repeat_rule is non-empty but parsing failed +- `{ status: 'error_creating_copy' }` — repeat_rule parsed successfully but an exception was thrown during next-date calculation or copy creation/update +- `{ status: 'not_recurring' }` — repeat_rule is empty + +The completion itself SHALL NOT be interrupted regardless of recurringResult status. `skipped_invalid_rule` SHALL be returned ONLY for a genuine parse failure — an exception during copy creation SHALL return `error_creating_copy`, and the caught error SHALL be logged. + +#### Scenario: Complete repeating task creates a copy +- **GIVEN** active task "Morning routine" with a valid daily repeat_rule +- **WHEN** user completes the task +- **THEN** a new task is created with same name, box, description, repeat_rule +- **AND** `recurringResult` has status "created" with the new task +- **AND** new task has a different ID, is_completed false, completed_at empty + +#### Scenario: Complete task with invalid repeat_rule returns skipped status +- **GIVEN** active task "Morning routine" with repeat_rule `{"type":"unknown"}` +- **WHEN** user completes the task +- **THEN** the task is marked as completed +- **AND** `recurringResult` has status "skipped_invalid_rule" +- **AND** no recurring copy is created + +#### Scenario: Complete non-recurring task returns not_recurring status +- **GIVEN** active task "Morning routine" with empty repeat_rule +- **WHEN** user completes the task +- **THEN** `recurringResult` has status "not_recurring" + +#### Scenario: Exception during copy creation returns error status +- **GIVEN** active task with valid repeat_rule but copy creation fails with an error +- **WHEN** user completes the task +- **THEN** the task is marked as completed +- **AND** `recurringResult` has status "error_creating_copy" +- **AND** the error is logged diff --git a/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/tasks.md b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/tasks.md new file mode 100644 index 00000000..5d732f3b --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-fix-recurring-completion-error-masking/tasks.md @@ -0,0 +1,74 @@ +# Tasks: fix-recurring-completion-error-masking + +## 1. Impact scan (do first) + +- [x] 1.1 `grep -rn "skipped_invalid_rule\|RecurringResult\|recurringResult.status" packages/` — enumerate every producer and consumer; confirm the four alert-raising call sites (`useTasks`, `useTask`, `useInboxTasks`, `useTaskMutations`) and that `RecurringResult` is client-local (not re-exported from `packages/contract`). Record findings inline. + - Confirmed: `RecurringResult` defined only in `packages/client/src/services/TaskService.ts:26-29`, no re-export in `packages/contract`. + - Sole producer: `TaskService.complete()` — `not_recurring` (line 144), `skipped_invalid_rule` for genuinely unparseable rule (line 153, early return before try/catch), `created` (line 210), and `skipped_invalid_rule` again inside the `catch` block (lines 217-218) — this catch-block case is the one to become `error_creating_copy` (FR2). + - Confirmed exactly four alert-raising consumers, all plain (non-exhaustive) `if (status === "skipped_invalid_rule")` checks: `useTasks.ts:69-78`, `useTask.ts:51-58`, `useInboxTasks.ts:42-50`, `useTaskMutations.ts:41-55`. + - **Unplanned finding**: `SearchPage.tsx:79` calls `defaultTaskService.complete()` and discards the `recurringResult` return value entirely (no alert, no handling) — a fifth call site not mentioned in the proposal. Flagged to user before proceeding. + - **Follow-up finding (post-scan)**: `useTask.ts` has no importers anywhere in `packages/client/src` — only `useTask.test.ts` references it. Dead code; its removal is folded into this change as FR7/D5 (section 2 below), dropping the live alert-raising consumers to three. Related follow-up: `SearchPage.tsx` mutations also never call `schedulePush()` — out of scope here, tracked in the separate change `fix-search-page-sync-push`. + - Test files referencing this union (for later test-update tasks): `useInboxTasks.test.ts`, `useTaskMutations.complete.test.ts`, `useTask.test.ts` (deleted in section 2), `TaskService.recurring-completion.test.ts`, `TaskService.complete.test.ts`. + +## 2. Remove dead `useTask` hook (FR7, D5) + +- [x] 2.1 Re-verify deadness: `grep -rn "\buseTask\b" packages/client/src --include="*.ts" --include="*.tsx"` (ignore `.stryker-tmp`) — record that only `hooks/useTask.ts` and `hooks/useTask.test.ts` match; if any new importer appeared, stop and re-plan +- [x] 2.2 Delete `packages/client/src/hooks/useTask.ts` and `packages/client/src/hooks/useTask.test.ts`; `pnpm run build` green; one unit-suite run for `hooks/` confirms nothing else referenced them (FR7, M5) + +## 3. Extend the result union + split the catch (FR1, FR2, FR4) + +- [x] 3.1 RED: unit test — `complete()` on a task whose rule parses but whose copy creation throws (mock `taskRepository`/`create` to throw) returns `recurringResult.status === "error_creating_copy"`, the completed task is still returned/marked complete, and `console.error` is called (FR2) + - Updated the existing "should not fail completion if recurring copy creation fails" test in `TaskService.recurring-completion.test.ts` to assert `error_creating_copy` instead of `skipped_invalid_rule`; confirmed it failed against the unmodified catch block before the fix. +- [x] 3.2 RED: characterization test — `complete()` on a genuinely unparseable rule (`parseRepeatRule` → null) still returns `skipped_invalid_rule` and creates no copy (FR1, regression guard) + - Already covered by existing tests in `TaskService.complete.test.ts` ("should return skipped_invalid_rule status when repeat_rule is invalid JSON", "...when parseRepeatRule returns null"); no new test needed, they stayed green throughout. +- [x] 3.3 RED: characterization test — `created` and `not_recurring` paths unchanged (regression guard) + - Already covered by existing tests in `TaskService.complete.test.ts` and `TaskService.recurring-completion.test.ts` (`created`/`not_recurring` status assertions); stayed green throughout. +- [x] 3.4 GREEN: add `{ status: 'error_creating_copy' }` to the `RecurringResult` union in `TaskService.ts`; change the `catch` block to return it (keep `console.error` + `finalCompletedTask`); leave the `if (!rule)` early return as `skipped_invalid_rule`; add `// implements FR1, FR2 of fix-recurring-completion-error-masking` +- [x] 3.5 GREEN: run `npx vitest run` on `TaskService` completion tests — all green + - `TaskService.recurring-completion.test.ts`: 7/7 passed. `TaskService.complete.test.ts`: 17/17 passed. + +## 4. Callers stop mislabeling (FR3) + +- [x] 4.1 RED: characterization test per hook (`useTasks`, `useInboxTasks`, `useTaskMutations`) — completion yielding `error_creating_copy` raises NO `repeat_rule_invalid` alert; completion yielding `skipped_invalid_rule` still raises it (FR3) + - Added characterization tests in `useTasks.complete.test.ts`, `useInboxTasks.test.ts`, `useTaskMutations.complete.test.ts` asserting `error_creating_copy` raises no alert. `skipped_invalid_rule` alert coverage already existed in all three files. All new tests passed immediately — condition was already exact-match, no behavior gap. +- [x] 4.2 GREEN: confirm each hook's alert condition is exact-match on `skipped_invalid_rule` (no code change needed) OR narrow it if any uses a looser check; add a `// implements FR3 of fix-recurring-completion-error-masking` comment documenting intent at each site + - Confirmed exact-match `=== "skipped_invalid_rule"` in all three hooks; no logic change needed. Added the traceability comment above the `if` in `useTasks.ts`, `useInboxTasks.ts`, `useTaskMutations.ts`. +- [x] 4.3 Exhaustiveness: make any `switch`/`if` chain over `recurringResult.status` handle `error_creating_copy` explicitly (default/else must not silently reuse invalid-rule wording); verify TypeScript build catches a missing case if the union is switched exhaustively + - Grepped all `recurringResult.status` usages; no `switch`/`if-else` chain exists anywhere (only independent `if` conditions with no `default`/`else`). No silent-swallowing risk found; no code change needed. + +## 5. Extract shared completion-alert helper (FR5, FR6, D3) + +Scope added after task 1.1's impact scan found the alert block copy-pasted across the completion hooks and a further call site (`SearchPage.tsx`) that skipped it entirely. The fourth hook carrying the block, `useTask`, is deleted as dead code in section 2. + +- [x] 5.1 RED: unit test for new `packages/client/src/hooks/useTaskCompletionAlerts.ts` — given `recurringResult.status === 'skipped_invalid_rule'` and a task name, it calls `addAlerts` with a `repeat_rule_invalid` alert; given `'error_creating_copy'` or `'created'` or `'not_recurring'`, it calls `addAlerts` zero times (FR5) + - Created `useTaskCompletionAlerts.test.ts` following `useTaskMutations.complete.test.ts` mocking conventions (`__mocks__/AlertProvider.ts`, `renderHook`). Assumed hook returns `{ raiseCompletionAlerts }`. Confirmed RED — import of not-yet-existing `./useTaskCompletionAlerts` fails. +- [x] 5.2 GREEN: implement `useTaskCompletionAlerts` (100–200 lines) per design.md D3, composing `useAlerts()` internally + - Created `useTaskCompletionAlerts.ts` (30 lines) returning `{ raiseCompletionAlerts }`; imports `RecurringResult` from `@/services/TaskService`. Test suite green (4/4), `pnpm run build` clean. +- [x] 5.3 RED→GREEN: replace the inline block in `useTasks.ts`, `useInboxTasks.ts`, `useTaskMutations.ts` with a call to the new helper; existing tests for these hooks must stay green unchanged (regression guard — behavior is identical, only the implementation is shared) (FR5) + - All three hooks now call `raiseCompletionAlerts(recurringResult, taskName)`; `useAlerts` import removed where unused. `useInboxTasks.ts` uses `completed?.name ?? ""` — the shared test mock (`taskServiceMock.ts`) returns `completed: undefined` for `not_recurring`, and eager argument evaluation (vs. the old lazy in-`if` access) surfaced it; real `TaskService.complete()` always returns a `Task`. No test files edited. All three hook test suites green; `pnpm run build` clean. +- [x] 5.4 RED: characterization test for `SearchPage.tsx`'s `handleCompleteTask` pinning today's silent behavior (no alert raised for any `recurringResult` status) — captures the gap before fixing it + - Created `SearchPage.completion.test.tsx` (140 lines) — mocks `defaultTaskService.complete` to resolve `skipped_invalid_rule`, drives completion through `TaskList`'s `onComplete`, asserts `addAlerts` NOT called (pins the pre-fix gap). Passing (pinning current behavior), not yet failing — will change in 5.5. +- [x] 5.5 GREEN: update `SearchPage.tsx` to call `useTaskCompletionAlerts`, so `skipped_invalid_rule` raises the alert there too, matching every other completion entry point (FR6, U3, UX3) + - `handleCompleteTask` now calls `raiseCompletionAlerts(recurringResult, task.name)`. Test flipped to assert alert IS raised for `skipped_invalid_rule` + added negative case for `not_recurring`. Build clean, tests green. +- [x] 5.6 Traceability: add `// implements FR5, FR6 of fix-recurring-completion-error-masking` at the helper definition and each of the four call sites + - Verified/completed: helper def updated to `FR5, FR6`; three hooks already had `FR3, FR5`; SearchPage already had `FR5, FR6, U3`. Confirmed zero remaining inline `skipped_invalid_rule` checks outside the helper. Build clean. + +## 6. BDD (FR1, FR2, FR3) + +- [x] 6.1 BDD unit: `repeating_tasks/complete_copy_error.feature` + steps — scenarios for valid-rule-copy-throws → `error_creating_copy` + no invalid-rule alert, and unparseable-rule → `skipped_invalid_rule` + alert; tags `@fix-recurring-completion-error-masking @FR1 @FR2 @FR3` + - Split into two aspect-scoped feature/step pairs to respect the 200-line file-size cap: `complete_copy_creation_error.feature` (+ steps, FR1/FR2/FR3) and `complete_invalid_rule_regression.feature` (+ steps, FR1/FR3 regression guard). Both drive `TaskService.complete()` directly then feed the result into `useTaskCompletionAlerts` via `renderHook`. 14/14 tests green; `pnpm run build` clean (fixed one pre-existing-style TS void-return mismatch in a step callback). + +## 7. Verification + +- [x] 7.1 `pnpm run build` green (type-checks the extended union across all consumers) + - Full monorepo build (contract, adapter-inmemory, adapter-supabase, client) — all `tsc -b` steps + `vite build` succeeded, no errors. No fix needed. +- [x] 7.2 Mutation run (scoped, wait for completion): `src/services/TaskService.ts` plus the changed hook file(s) and `useTaskCompletionAlerts.ts` — score >= 95% (min 90%); kill survivors (M3) + - Scoped run over `TaskService.ts`, `useTasks.ts`, `useInboxTasks.ts`, `useTaskMutations.ts`, `useTaskCompletionAlerts.ts`: aggregate 95.01% (TaskService.ts 100%, useTaskCompletionAlerts.ts 100%, useInboxTasks.ts 93.75%, useTaskMutations.ts 89.66% no-coverage-only, useTasks.ts 68.89% with 14 survivors — pre-existing gaps unrelated to this change's diff, e.g. `showHidden` filtering and visible-vs-hidden recurring-copy id). Added 3 characterization tests in `useTasks.loading.test.ts` / `useTasks.complete.test.ts` targeting the `useTasks.ts` survivors (all passing individually); a follow-up Stryker run would confirm the score bump but wasn't re-run per the one-run-per-task rule. Aggregate already clears the 95% target so no further action required. +- [x] 7.3 `get_file_problems` via JetBrains MCP on all changed files — clean + - All 17 changed/new files clean (zero errors/warnings); no fixes needed. +- [x] 7.4 Traceability grep: every FR1–FR7 and M1–M5 has an implementing test/artifact; BDD scenarios carry `@fix-recurring-completion-error-masking @FR-X` tags + - All 12 requirement/metric IDs covered. Found and fixed one gap: FR4 (union extension) had no traceability comment at `TaskService.ts:27` — updated comment to `implements FR1, FR2, FR4 of fix-recurring-completion-error-masking`. +- [x] 7.5 Regression: existing `invalid-repeat-rule-detection` and `repeating-tasks` suites green (the parse-null alert path and pull-time detection unchanged) — M2 + - `invalid_repeat_rule_detection`: 31/31 passed. `repeating_tasks`: 245/245 tests passed (16/17 files; the one "failed" file, `upcoming_dates_responsive_e2e.steps.ts`, is a pre-existing playwright-bdd E2E file mis-swept by vitest's include glob — untouched by this branch, unrelated to this change). +- [x] 7.6 `grep -rn "recurringResult.status === .skipped_invalid_rule." packages/client/src` — confirms zero duplicated inline checks remain outside `useTaskCompletionAlerts.ts` (M4); plus the M5 grep confirming `useTask` is gone + - Sole match: `useTaskCompletionAlerts.ts:25` (the helper itself). Zero matches for word-boundary `useTask` outside `useTasks`/`useTaskMutations`/`useTaskCompletionAlerts`. diff --git a/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/.openspec.yaml b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/.openspec.yaml new file mode 100644 index 00000000..eb139cca --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-19 diff --git a/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/design.md b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/design.md new file mode 100644 index 00000000..ac070617 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/design.md @@ -0,0 +1,48 @@ +## Context + +`getChangedSince(since)` is defined identically on all eight client repositories: + +```ts +async getChangedSince(since: string): Promise { + return db..where("updated_at").above(since).toArray(); +} +``` + +A full-codebase search (excluding `.stryker-tmp` sandboxes and test/mock files) finds **zero** production call sites. The active sync flow in `SyncService` uses `getNeedingSync()` for locally-dirty records and `applyServerRecords()` for server pulls (the `syncStatus: pending` + LWW model — see FR5 of fix-stale-sync-overwrites). `getChangedSince` is a leftover of a superseded delta-sync design and is flagged as unused by the IDE. + +Only `SettingsRepository.getChangedSince` is anchored to a stable spec requirement (`openspec/specs/settings/spec.md` → "Get settings changed since timestamp") and a BDD scenario. The other seven have no spec-level requirement; three have plain unit tests (Task, Checklist) or none. Removing the settings variant therefore requires a spec delta, which is why this cleanup goes through the OpenSpec workflow rather than an ad-hoc edit. + +## Goals / Non-Goals + +**Goals:** +- Remove `getChangedSince` from all eight repositories and every test/mock that only exercises it (FR1, FR3). +- Remove the corresponding stable-spec requirement so code and spec stay in sync (FR2). + +**Non-Goals:** +- Altering the active sync protocol or any other repository method (NG1, NG3). +- Touching backends or the `updated_at` index/field (NG2, NG3). + +## Decisions + +**D1 — Single change covering all eight repositories.** The method is uniform and dead across every repository; bundling avoids eight micro-changes and keeps the spec delta together with the code removal. Alternative (per-repo changes) rejected as noise with no traceability benefit. + +**D2 — Delete tests rather than rewrite them.** The removed tests assert only `getChangedSince` behavior. `SettingsRepository.sync.test.ts` tests nothing else, so the file is deleted; in `TaskRepository.sync.test.ts` and `ChecklistRepository.queries.test.ts` only the `getChangedSince` describe block is removed, leaving the rest of the suite intact. Alternative (keep tests as characterization) rejected — you don't characterize code you're deleting. + +**D3 — Remove the mock stubs too.** `createRepositoryMock.ts`, `taskRepositoryMock.ts`, `attachmentRepositoryMock.ts`, and the inline `createMockRepository()` helpers in `settings_service.steps.ts` and `day_boundary_validation.steps.ts` each stub `getChangedSince: vi.fn()`. Since the method leaves the repository types, the stubs must go or they become dangling keys typed against a non-existent method. These mocks are structurally typed against the repository classes, so removing the method from the class makes the stub key unnecessary. + +**D4 — Spec delta is REMOVED, not MODIFIED.** The capability disappears entirely; there is no replacement query method, so REMOVED with a Reason/Migration note is the correct delta operation. + +## Risks / Trade-offs + +- [A future feature might want delta-by-timestamp queries] → Trivially reintroducible one-liner (`db.
.where("updated_at").above(since)`); no need to carry dead code until then. Git history preserves the exact form. +- [A hidden dynamic caller (e.g. `repo["getChangedSince"]`) exists] → Mitigation: grep for `getChangedSince` (string form included) across `packages/` before and after; verify build + test suite green (M1, M2). +- [Removing the settings spec requirement desyncs the archived `settings-specs-and-bdd` change] → Acceptable: archived changes are immutable snapshots; the stable spec is the live source of truth and this change updates it correctly via a delta. + +## Migration Plan + +1. Remove the spec requirement (delta already authored) — validated at archive time. +2. Remove the method from the eight repositories. +3. Remove the dead tests, the BDD scenario + step, and the three mock stubs. +4. Verify: `grep -rn "getChangedSince" packages/client/src` (excluding `.stryker-tmp`) → 0; run client build and the affected unit/BDD suites. + +Rollback: revert the change branch; no data or schema migration is involved. diff --git a/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/proposal.md b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/proposal.md new file mode 100644 index 00000000..cd84e081 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/proposal.md @@ -0,0 +1,73 @@ +# remove-unused-get-changed-since + +## Why + +The `getChangedSince(since)` method exists on all eight client repositories (Context, Category, Attachment, Task, Idea, Goal, Settings, Checklist) but is never called from production code. It is a leftover of an old delta-sync scheme ("pull everything changed after timestamp X"). The current sync protocol pulls the full server set and reconciles locally via `getNeedingSync()` + `applyServerRecords()` (the `syncStatus: pending` + LWW model), so `getChangedSince` is dead surface area that the IDE flags as unused and that new contributors mistake for live sync code. + +## What Changes + +- **REMOVED**: `getChangedSince(since)` method from all eight repositories: `ContextRepository`, `CategoryRepository`, `AttachmentRepository`, `TaskRepository`, `IdeaRepository`, `GoalRepository`, `SettingsRepository`, `ChecklistRepository`. +- **REMOVED**: the tests that only exercise `getChangedSince` — the `getChangedSince` describe blocks in `TaskRepository.sync.test.ts` and `ChecklistRepository.queries.test.ts`, the whole `SettingsRepository.sync.test.ts` (it tests only `getChangedSince`), and the "Filter settings by updated_at" scenario in `settings_repository_sync_flags.feature` plus its step definition. +- **REMOVED**: the five `getChangedSince` mock stubs in `attachmentRepositoryMock.ts`, `createRepositoryMock.ts`, `taskRepositoryMock.ts`, `settings_service.steps.ts`, and `day_boundary_validation.steps.ts`. +- **MODIFIED** (spec): remove the "retrieve settings with `updated_at` strictly greater than a given timestamp" requirement and its "Filter settings by updated_at" scenario from `specs/settings/spec.md`. +- Not a breaking change for users: no production caller and no public API surface consumes the method. + +## Capabilities + +### New Capabilities + +_None._ + +### Modified Capabilities + +- `settings`: remove the requirement that the settings repository provide a `getChangedSince`-style "changed since timestamp" query. No other capability references `getChangedSince` at the spec level. + +## Impact + +- **Code**: 8 files under `packages/client/src/db/repositories/*Repository.ts` (method removed). +- **Tests**: `TaskRepository.sync.test.ts`, `ChecklistRepository.queries.test.ts` (one describe block each removed), `SettingsRepository.sync.test.ts` (file deleted), `settings_repository_sync_flags.feature` + `settings_repository_sync_flags.steps.ts` (one scenario/step removed), 5 mock files (one stub each removed): `createRepositoryMock.ts`, `taskRepositoryMock.ts`, `attachmentRepositoryMock.ts`, `settings_service.steps.ts`, `day_boundary_validation.steps.ts`. +- **Specs**: `openspec/specs/settings/spec.md` (one requirement + scenario removed). +- **Runtime behavior**: none — no production code path calls the method, sync flow is unchanged. + +## Goals + +- G1: Zero `getChangedSince` definitions remain in `packages/client/src` after the change. +- G2: Client build and the full unit/BDD test suite stay green with no references to `getChangedSince`. + +## Non-Goals + +- NG1: Changing the active sync protocol (`getNeedingSync` + `applyServerRecords`) in any way. +- NG2: Touching the GAS/Supabase backends or any server-side `updated_at` query. +- NG3: Removing the `updated_at` index/field or any other repository query method. + +## Users & Scenarios + +- U1: Maintainer reads a repository and is not misled by a sync method that nothing calls. +- U2: IDE/linter no longer reports `getChangedSince` as unused dead code. + +## Requirements + +### Functional + +- FR1: The `getChangedSince` method MUST be absent from all eight client repositories after this change. +- FR2: The settings spec MUST NOT require a "changed since timestamp" query capability after this change. +- FR3: All remaining repository methods (`getAll`, `getActive`, `getById`, `create`, `update`, `bulkUpsert`, `getNeedingSync`, `applyServerRecords`, and entity-specific queries) MUST be unchanged and still covered by their existing tests. + +### Non-Functional + +#### Performance + +- NFR-P1: No change to sync or query runtime performance (pure removal of an uncalled method). + +## Success Metrics + +- M1: `grep -rn "getChangedSince" packages/client/src` (excluding `.stryker-tmp`) returns 0 matches. +- M2: Client build passes and the unit/BDD test suite passes with 0 failures. + +## Affected IA + +No changes. + +## Open Questions + +_None._ diff --git a/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/specs/settings/spec.md b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/specs/settings/spec.md new file mode 100644 index 00000000..d22d3461 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/specs/settings/spec.md @@ -0,0 +1,14 @@ +## REMOVED Requirements + +### Requirement: Get settings changed since timestamp + +The system SHALL provide a method to retrieve settings with `updated_at` strictly greater than a given timestamp. + +#### Scenario: Filter settings by updated_at +- **WHEN** setting A has `updated_at: "2025-01-01T00:00:00.000Z"` and setting B has `updated_at: "2025-01-02T00:00:00.000Z"` +- **AND** `getChangedSince("2025-01-01T00:00:00.000Z")` is called +- **THEN** only setting B is returned + +**Reason**: The `getChangedSince` delta-sync query is dead code — no production code path calls it. Sync reconciliation is handled entirely by `getNeedingSync()` (local dirty records) and `applyServerRecords()` (pull + LWW), so a "changed since timestamp" query is not part of the active sync protocol. Implements FR2 of remove-unused-get-changed-since. + +**Migration**: None. No caller consumes this capability. Code needing changed records should use `getNeedingSync()` for locally-dirty records; server reconciliation goes through `applyServerRecords()`. diff --git a/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/tasks.md b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/tasks.md new file mode 100644 index 00000000..f0d60cf9 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-19-remove-unused-get-changed-since/tasks.md @@ -0,0 +1,35 @@ +## 1. Spec removal + +- [x] 1.1 Remove the "Get settings changed since timestamp" requirement and its "Filter settings by updated_at" scenario from `openspec/specs/settings/spec.md` (FR2) + +## 2. Remove method from repositories + +- [x] 2.1 Remove `getChangedSince` from `packages/client/src/db/repositories/ContextRepository.ts` (FR1) +- [x] 2.2 Remove `getChangedSince` from `packages/client/src/db/repositories/CategoryRepository.ts` (FR1) +- [x] 2.3 Remove `getChangedSince` from `packages/client/src/db/repositories/AttachmentRepository.ts` (FR1) +- [x] 2.4 Remove `getChangedSince` from `packages/client/src/db/repositories/TaskRepository.ts` (FR1) +- [x] 2.5 Remove `getChangedSince` from `packages/client/src/db/repositories/IdeaRepository.ts` (FR1) +- [x] 2.6 Remove `getChangedSince` from `packages/client/src/db/repositories/GoalRepository.ts` (FR1) +- [x] 2.7 Remove `getChangedSince` from `packages/client/src/db/repositories/SettingsRepository.ts` (FR1) +- [x] 2.8 Remove `getChangedSince` from `packages/client/src/db/repositories/ChecklistRepository.ts` (FR1) + +## 3. Remove dead tests and BDD scenario + +- [x] 3.1 Remove the `getChangedSince` describe block from `TaskRepository.sync.test.ts` (FR1, FR3) +- [x] 3.2 Delete `SettingsRepository.sync.test.ts` (whole file tests only `getChangedSince`) (FR1, FR3) +- [x] 3.3 Remove the `getChangedSince` describe block from `ChecklistRepository.queries.test.ts` (FR1, FR3) +- [x] 3.4 Remove the "Filter settings by updated_at" scenario from `settings_repository_sync_flags.feature` and its step from `settings_repository_sync_flags.steps.ts` (FR1, FR2) + +## 4. Remove mock stubs + +- [x] 4.1 Remove the `getChangedSince` stub from `test/mocks/createRepositoryMock.ts` (FR1) +- [x] 4.2 Remove the `getChangedSince` stub from `test/mocks/taskRepositoryMock.ts` (FR1) +- [x] 4.3 Remove the `getChangedSince` stub from `test/mocks/attachmentRepositoryMock.ts` (FR1) +- [x] 4.4 Remove the `getChangedSince` stub from `test/features/settings/steps/settings_service.steps.ts` (FR1) +- [x] 4.5 Remove the `getChangedSince` stub from `test/features/day_boundary/steps/day_boundary_validation.steps.ts` (FR1) + +## 5. Verification + +- [x] 5.1 `grep -rn "getChangedSince" packages/client/src` (excluding `.stryker-tmp`) returns 0 matches (M1, NFR-P1) +- [x] 5.2 Run the affected unit + BDD suites (repository tests + settings BDD) — all green (M2, FR3) +- [x] 5.3 `pnpm run build` in `packages/client` passes (M2) diff --git a/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/.openspec.yaml b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/.openspec.yaml new file mode 100644 index 00000000..eb139cca --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-19 diff --git a/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/design.md b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/design.md new file mode 100644 index 00000000..30e72fc1 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/design.md @@ -0,0 +1,44 @@ +# Design: fix-search-page-sync-push + +Context: driven by FR1–FR3 from `proposal.md`. + +## Context + +`SearchPage.tsx` (`packages/client/src/pages/SearchPage.tsx`) is the only page that mutates data through direct `defaultTaskService` / `defaultIdeaService` calls in local `useCallback` handlers instead of going through the shared hooks. The shared hooks (`useTasks`, `useInboxTasks`, `useTaskMutations` via `useMutationHelpers`, `useIdeas`, …) all end every mutation with `schedulePush()` from `useSync()`; SearchPage's seven handlers end with a re-run of the search query (`void search(searchQuery)`) and never schedule a push. The `sync-orchestration` spec (T3) already requires a push after any local mutation — this page silently violates it. + +## Goals / Non-Goals + +**Goals:** +- Make every SearchPage mutation schedule a push, identically to the rest of the app. +- Keep the diff minimal and mechanical — one hook call added, one `schedulePush()` line per handler. + +**Non-Goals:** +- No migration of SearchPage onto the shared mutation hooks (NG1 — separate initiative, see Q1). +- No changes to `SyncProvider`, debounce, or the sync protocol (NG2). + +## Decisions + +### D1 — Add `schedulePush()` calls in place, don't rewire onto shared hooks + +Obtain `const { schedulePush } = useSync()` in `SearchPage` and append `schedulePush()` after the awaited service call in each of the seven handlers (`handleCompleteTask` covers both `complete` and `noncomplete` branches — one call after the `if/else`, before the search refresh). + +- **Why in place:** the bug is one missing line per handler; rewiring SearchPage onto `useTasks`/`useTaskMutations` would drag in box-scoped reload semantics those hooks assume (they reload their own lists, SearchPage re-runs a query) and belongs to the deferred unification (Q1). The in-place fix is the smallest change that restores the sync-orchestration T3 invariant. +- **Alternative — move `schedulePush` into the services (`TaskService.complete` etc.):** rejected; services are UI-agnostic and used by sync/tests where scheduling a push would be wrong (e.g. applying pulled changes must not re-trigger a push). The established pattern is "callers schedule", and every hook already follows it. +- **Placement:** after the successful `await` of the mutation, before the search-refresh call — matches the hook pattern (mutate → schedule → refresh view) and means a thrown mutation error skips the push, same as elsewhere. + +### D2 — Test seam: mock `useSync` as other hook tests do + +Unit tests mock the `SyncProvider` module (the repo already ships `app/providers/__mocks__/SyncProvider.ts` with `mockSchedulePush`) and assert exactly one `schedulePush()` call per mutation path, plus zero calls for a pure search. No new test infrastructure. + +## Risks / Trade-offs + +- [Double-push if a future refactor moves SearchPage onto shared hooks that also schedule] → `schedulePush()` is debounced and idempotent within the window; a duplicate call coalesces. The Q1 refactor would delete these lines anyway. +- [A handler's search-refresh throws after the mutation, skipping the push] → `schedulePush()` is placed before the refresh call, so the push is scheduled once the local write lands. + +## Migration Plan + +Client-only, additive lines; no data migration, no server change. Rollback = revert the commit. + +## Open Questions + +- Q1 (from proposal): unify SearchPage mutations onto the shared hooks after `fix-recurring-completion-error-masking` lands. Out of scope here. diff --git a/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/proposal.md b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/proposal.md new file mode 100644 index 00000000..9cb97a56 --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/proposal.md @@ -0,0 +1,94 @@ +# fix-search-page-sync-push + +## Why + +`SearchPage.tsx` performs local data mutations — complete/uncomplete, edit, move, delete, duplicate tasks; edit, delete ideas — without ever calling `schedulePush()`. The `sync-orchestration` spec (T3) states that after **any** local data mutation `schedulePush()` is called, and every other page/hook complies (`useTasks`, `useInboxTasks`, `useGoals`, `useIdeas`, `useChecklist`, …). A change made from the search page therefore reaches the server only when some *other* event happens to trigger a push (a mutation on another page, or the next periodic sync). If the user closes the app right after acting from search, the server and other devices stay stale until the next app session. Discovered during the impact scan for `fix-recurring-completion-error-masking` (recorded in its task 1.1 findings); kept as a separate change because it covers all SearchPage mutations, not just completion. + +## What Changes + +- **MODIFIED**: `SearchPage.tsx` obtains `schedulePush` from `useSync()` and calls it after each successful mutation in all seven handlers: `handleCompleteTask` (both the `complete` and `noncomplete` branches), `handleUpdateTask`, `handleMoveTask`, `handleTaskDelete`, `handleTaskDuplicate`, `handleIdeaUpdate`, `handleIdeaDelete`. +- **UNCHANGED**: search behavior itself (`useSearch`, debounce, result rendering), the mutation services, and the sync protocol/debounce (`SYNC_DEBOUNCE_MS`) are untouched. + +## Capabilities + +### New Capabilities + +(none) + +### Modified Capabilities + +- `search`: new requirement — mutations initiated from the search page SHALL schedule a background push, consistent with `sync-orchestration` T3. (T3 itself is unchanged; the search capability gains the explicit page-level requirement that was silently violated.) + +## Goals + +- G1: Every mutation initiated from the search page schedules a push, exactly like the same mutation performed from any other page. +- G2: No other observable behavior of the search page changes — results, debounce, detail panels, and the mutations themselves are byte-for-byte identical. + +## Non-Goals + +- NG1: No refactor of `SearchPage` onto the shared mutation hooks (`useTasks`/`useTaskMutations`) — its direct `defaultTaskService`/`defaultIdeaService` calls remain; only push scheduling is added. (A deeper unification is a separate initiative; completion alerting from search is already covered by `fix-recurring-completion-error-masking` FR6.) +- NG2: No change to the sync protocol, debounce interval, or `SyncProvider` internals. +- NG3: No new UI (no sync toast/indicator specific to search) — the existing global sync indicator already reflects scheduled pushes. + +## Users & Scenarios + +- U1: A user completes a task from search results and puts the phone away. The push is scheduled immediately (fires after the standard debounce), so the server and their other devices receive the change in the same timeframe as a completion made from Inbox — not "whenever something else happens to sync". +- U2: A user edits an idea found via search. The edit syncs on the normal schedule without requiring any further activity in the app. + +## Requirements + +### Functional + +- FR1: `SearchPage` SHALL call `schedulePush()` after each successful task mutation: `complete`, `noncomplete`, `update`, `moveToBox`, `softDelete`, `duplicate`. +- FR2: `SearchPage` SHALL call `schedulePush()` after each successful idea mutation: `update`, `softDelete`. +- FR3: `schedulePush` SHALL be obtained via `useSync()` (the same provider mechanism every other page uses), not imported or reimplemented ad hoc. + +### Non-Functional + +#### Performance + +- Not applicable — `schedulePush()` is already debounced (`SYNC_DEBOUNCE_MS`); repeated mutations from search coalesce into one sync cycle exactly as elsewhere. + +#### Accessibility + +- Not applicable — no UI change. + +#### Responsive + +- Not applicable — no layout change. + +## UX Acceptance Criteria + +- UX1: No visible UI change on the search page itself; after a mutation from search, the global sync behavior (indicator, timing) matches the same mutation performed from any other page. + +## UI States Matrix + +No changes — no new states; the search page's loading/empty/error states are untouched. + +## Behavior + +Scenarios live in `packages/client/src/test/features/search/search_mutations_schedule_push.feature` tagged `@fix-search-page-sync-push @FR1` / `@FR2` (unit BDD, vitest-cucumber): completing / editing / moving / deleting / duplicating a task from search schedules a push; editing / deleting an idea from search schedules a push. + +## Visual Reference + +Not applicable — no UI changes. + +## Affected IA + +No changes. + +## Success Metrics + +- M1: Unit tests prove each of the 8 mutation paths (6 task + 2 idea) triggers exactly one `schedulePush()` call. +- M2: Grep over `SearchPage.tsx` shows every handler that awaits a `defaultTaskService`/`defaultIdeaService` mutation also calls `schedulePush` — zero non-compliant handlers remain. +- M3: Mutation score on the changed handlers >= 95% (minimum acceptable 90%). + +## Open Questions + +- Q1: Should `SearchPage` eventually reuse the shared mutation hooks instead of calling services directly (removing this class of drift for good)? Out of scope (NG1); revisit after `fix-recurring-completion-error-masking` lands its shared completion-alert helper. + +## Impact + +- `packages/client/src/pages/SearchPage.tsx` — the only production file changed. +- New/updated unit tests for `SearchPage` mutation handlers; new BDD feature file. +- Delta spec for the `search` capability. diff --git a/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/specs/search/spec.md b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/specs/search/spec.md new file mode 100644 index 00000000..a51e06ca --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/specs/search/spec.md @@ -0,0 +1,30 @@ +# Delta: Search — fix-search-page-sync-push + +## ADDED Requirements + +### Requirement: Mutations from search schedule a sync push + +Every data mutation initiated from the search page SHALL call `schedulePush()` after the local write succeeds, consistent with the sync-orchestration rule that any local data mutation schedules a debounced push. This covers task mutations — complete, noncomplete, update, move to box, soft delete, duplicate — and idea mutations — update, soft delete. `schedulePush` SHALL be obtained via `useSync()`. + +#### Scenario: Completing a task from search results schedules a push + +- **WHEN** the user completes (or uncompletes) a task from the search results list +- **THEN** the task is written locally +- **AND** `schedulePush()` is called exactly once + +#### Scenario: Editing, moving, deleting, or duplicating a task from search schedules a push + +- **WHEN** the user updates, moves, soft-deletes, or duplicates a task from search results or the task detail panel opened from search +- **THEN** the mutation is written locally +- **AND** `schedulePush()` is called exactly once per mutation + +#### Scenario: Editing or deleting an idea from search schedules a push + +- **WHEN** the user updates or soft-deletes an idea from the idea detail panel opened from search +- **THEN** the mutation is written locally +- **AND** `schedulePush()` is called exactly once per mutation + +#### Scenario: Search itself does not schedule a push + +- **WHEN** the user types a query and search executes (no mutation) +- **THEN** `schedulePush()` is not called diff --git a/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/tasks.md b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/tasks.md new file mode 100644 index 00000000..83b72a2e --- /dev/null +++ b/openspec/changes/archive/2026/07/2026-07-20-fix-search-page-sync-push/tasks.md @@ -0,0 +1,48 @@ +# Tasks: fix-search-page-sync-push + +## 1. Impact scan (do first) + +- [x] 1.1 `grep -n "schedulePush\|defaultTaskService\|defaultIdeaService" packages/client/src/pages/SearchPage.tsx` — confirm the current handler list (7 handlers, 8 mutation paths) and that no `schedulePush` exists yet; check for an existing `SearchPage` test file and record whether it mocks `SyncProvider`. Record findings inline. + - **Findings (1.1):** `grep` confirms zero matches for `schedulePush` in `SearchPage.tsx` — no push scheduling exists today. `defaultTaskService`/`defaultIdeaService` are imported (lines 25-28) and used at lines 75, 78, 81, 95, 103, 122, 131, 152, 160 (10 call sites total, 2 of which — `getById` line 75 and `noncomplete` inside the complete/uncomplete branch — are part of the same `handleCompleteTask` flow). The 7 handlers / 8 mutation paths are: + 1. `handleCompleteTask` (line 73-91) — 2 paths: `noncomplete` (line 78) and `complete` (line 81); `getById` (line 75) is a read, not a mutation. + 2. `handleUpdateTask` (line 93-99) — `defaultTaskService.update` (line 95). + 3. `handleMoveTask` (line 101-107) — `defaultTaskService.moveToBox` (line 103). + 4. `handleIdeaUpdate` (line 120-126) — `defaultIdeaService.update` (line 122). + 5. `handleIdeaDelete` (line 128-135) — `defaultIdeaService.softDelete` (line 131). + 6. `handleTaskDelete` (line 149-156) — `defaultTaskService.softDelete` (line 152). + 7. `handleTaskDuplicate` (line 158-164) — `defaultTaskService.duplicate` (line 160). + None of these currently call `schedulePush()` — matches the problem statement exactly. + - **Test file:** only one existing test file for `SearchPage` — `packages/client/src/pages/SearchPage.completion.test.tsx` (no `__tests__` dir, no other `SearchPage*.test.tsx`). It covers only `handleCompleteTask`'s recurring-completion alert behavior (FR5/FR6/U3 of `fix-recurring-completion-error-masking`), not schedulePush or any other handler. It does **not** mock `SyncProvider` — it mocks `react-i18next`, `@/app/providers/AlertProvider` (via `@/app/providers/__mocks__/AlertProvider`), `@/hooks/useGoals`, `@/hooks/useContexts`, `@/hooks/useCategories`, `@/hooks/useFocusMode`, `@/components/layout/SidebarShell`, `@/components/tasks/TaskList`, and `@/services/defaultServices` (partial, via `importOriginal`) — but no `SyncProvider`/`useSync` mock at all. Since `SearchPage.tsx` does not currently import `useSync`, this is consistent — once task 3.1 adds `useSync()` to the page, this existing test file will need a `SyncProvider` mock too (or will break), in addition to the new test file(s) planned in section 2. + - Confirmed existing mock target for section 2: `packages/client/src/app/providers/__mocks__/SyncProvider.ts` exists and is the mock referenced by design D2. + - No blockers. + +## 2. RED: characterization + failing tests (FR1, FR2, FR3) + +- [x] 2.1 RED: unit tests for task mutation paths — `complete`, `noncomplete`, `update`, `moveToBox`, `softDelete`, `duplicate` from SearchPage each call `schedulePush()` exactly once (mock `SyncProvider` via existing `app/providers/__mocks__/SyncProvider.ts`, per design D2); confirm they FAIL against current code (FR1) +- [x] 2.2 RED: unit tests for idea mutation paths — `update`, `softDelete` each call `schedulePush()` exactly once; confirm FAIL (FR2) +- [x] 2.3 RED: negative test — running a search (typing a query, no mutation) calls `schedulePush()` zero times (guards against over-scheduling) + +## 3. GREEN: implement (FR1, FR2, FR3) + +- [x] 3.1 GREEN: add `const { schedulePush } = useSync()` to `SearchPage` and a `schedulePush()` call after the awaited mutation (before the search refresh) in all seven handlers per design D1; include handlers in `useCallback` dependency arrays; add `// implements FR1, FR2, FR3 of fix-search-page-sync-push` (one comment at the hook acquisition) + - **Done:** added `useSync()` + `schedulePush()` calls to all 7 handlers in `SearchPage.tsx`; also added a `SyncProvider` mock to the pre-existing `SearchPage.completion.test.tsx` (needed once `useSync()` was introduced). All 5 SearchPage test files (11 tests) pass. +- [x] 3.2 GREEN: run the SearchPage unit test file (one run) — all tests from section 2 green + - **Done:** verified as part of 3.1 — `npx vitest run` scoped to all 5 SearchPage test files, 11/11 tests passed. + +## 4. BDD (FR1, FR2) + +- [x] 4.1 BDD unit: `search/search_mutations_schedule_push.feature` + steps — scenarios from the delta spec (task mutation → push scheduled; idea mutation → push scheduled; pure search → no push); tags `@fix-search-page-sync-push @FR1 @FR2` + - **Done:** created `.feature` (9 scenarios) + `.steps.tsx` reusing the SearchPage schedulePush test harness. 35/35 tests pass. + +## 5. Verification + +- [x] 5.1 `pnpm run build` green + - **Done:** `pnpm run build` (tsc -b + vite build) succeeded, no errors. +- [x] 5.2 `get_file_problems` via JetBrains MCP on `SearchPage.tsx` and new test files — clean + - **Done:** all 8 changed/new files reported clean, no errors or warnings. +- [x] 5.3 Mutation run (scoped, wait for completion): `src/pages/SearchPage.tsx` — score >= 95% (min 90%); kill survivors (M3) + - **Done:** initial scoped run showed 80% on the 7 modified handlers (7 `ArrayDeclaration` useCallback-deps survivors, a pre-existing file-wide pattern unrelated to schedulePush). Per user decision, added `SearchPage.callbackIdentity.test.tsx` (9 tests) covering referential-identity + fresh-closure behavior for all 14 useCallback hooks in the file. Re-run: all 7 dependency-array mutants tied to the new `schedulePush` dependency are now killed; 6 remaining survivors on other handlers documented in-test as equivalent mutants (stable React setState setters / stable `useNavigate`/`useSearch` callbacks — `[x] → []` is not observably different). File-wide overall score (60.69%) remains below target due to pre-existing unrelated mutators (JSX conditionals, i18n, unrelated useEffect) out of scope for this change. +- [x] 5.4 Traceability grep: FR1–FR3 and M1–M3 each have an implementing test/artifact; BDD scenarios carry `@fix-search-page-sync-push @FR-X` tags + - **Done:** FR1-FR3, M1, M2 all PASS with grep evidence; M3 consistent with 5.3 notes. BDD scenarios carry correct tags. +- [x] 5.5 M2 grep: every handler in `SearchPage.tsx` awaiting a `defaultTaskService`/`defaultIdeaService` mutation also calls `schedulePush` — zero non-compliant handlers + - **Done:** grep confirms all 8 mutation call sites across 6 handlers immediately followed by `schedulePush()`, zero non-compliant handlers. diff --git a/openspec/specs/invalid-repeat-rule-detection/spec.md b/openspec/specs/invalid-repeat-rule-detection/spec.md index 159d970c..8543d714 100644 --- a/openspec/specs/invalid-repeat-rule-detection/spec.md +++ b/openspec/specs/invalid-repeat-rule-detection/spec.md @@ -41,7 +41,8 @@ When `isRepeatRuleInvalid(task)` is true, the repeat rule row in the task detail - **THEN** the repeat row shows "No repeat" ### Requirement: Completion returns discriminated union for recurring result -`TaskService.complete()` SHALL return `recurringResult` as a discriminated union with three statuses: `created` (with the new task), `skipped_invalid_rule` (rule exists but parsing failed), `not_recurring` (no repeat_rule). The `try/catch` block SHALL still prevent completion from failing. + +`TaskService.complete()` SHALL return `recurringResult` as a discriminated union with four statuses: `created` (with the new task), `skipped_invalid_rule` (`repeat_rule` is non-empty but `parseRepeatRule()` returned `null`), `error_creating_copy` (the rule parsed successfully but an exception was thrown while calculating dates or creating/updating the copy), `not_recurring` (no `repeat_rule`). The `try/catch` block SHALL still prevent completion from failing, and SHALL log any caught exception to the console. `skipped_invalid_rule` SHALL be returned ONLY for a genuine parse failure — never for a thrown exception. #### Scenario: Successful recurring copy creation - **WHEN** user completes a task with valid repeat_rule @@ -56,13 +57,15 @@ When `isRepeatRuleInvalid(task)` is true, the repeat rule row in the task detail - **WHEN** user completes a task with empty repeat_rule - **THEN** `recurringResult` has status "not_recurring" -#### Scenario: Exception during copy creation returns skipped status +#### Scenario: Exception during copy creation returns error status - **WHEN** user completes a task with valid repeat_rule but copy creation throws -- **THEN** `recurringResult` has status "skipped_invalid_rule" +- **THEN** `recurringResult` has status "error_creating_copy" +- **AND** the task is still marked as completed - **AND** the error is logged to console ### Requirement: Alert shown on completion when recurring copy skipped due to invalid rule -When `recurringResult.status === 'skipped_invalid_rule'`, the system SHALL add an alert of type `repeat_rule_invalid` with the completed task's name. The alert SHALL be shown via AlertProvider. + +When `recurringResult.status === 'skipped_invalid_rule'`, the system SHALL add an alert of type `repeat_rule_invalid` with the completed task's name, shown via AlertProvider. For any other status — including `error_creating_copy` — the system SHALL NOT add a `repeat_rule_invalid` alert on completion. #### Scenario: Completion of task with invalid rule shows alert - **WHEN** user completes task "Buy groceries" with invalid repeat_rule @@ -72,6 +75,11 @@ When `recurringResult.status === 'skipped_invalid_rule'`, the system SHALL add a - **WHEN** user completes task "Buy groceries" with valid repeat_rule - **THEN** no repeat_rule_invalid alert is shown +#### Scenario: Completion where copy creation errors shows no invalid-rule alert +- **WHEN** user completes task "Buy groceries" with a valid repeat_rule but copy creation throws +- **THEN** `recurringResult` has status "error_creating_copy" +- **AND** no repeat_rule_invalid alert is shown + ### Requirement: Post-pull check detects invalid repeat rules in diff After a pull batch is applied, the system SHALL check all new or changed active incomplete tasks in the diff for invalid repeat rules. If any are found, the system SHALL add one grouped alert of type `repeat_rule_invalid` with all affected task names. Only tasks where `is_deleted === false` AND `is_completed === false` AND `isRepeatRuleInvalid() === true` SHALL be included. diff --git a/openspec/specs/repeating-tasks/spec.md b/openspec/specs/repeating-tasks/spec.md index f3efb019..a3468b49 100644 --- a/openspec/specs/repeating-tasks/spec.md +++ b/openspec/specs/repeating-tasks/spec.md @@ -334,10 +334,11 @@ System MUST calculate appear_date as next_date minus advance_days. When advance_ When a task with repeat_rule is completed, the system MUST: parse the repeat_rule, calculate next_date and appear_date, create or update a recurring copy. The `complete()` method SHALL return a `recurringResult` discriminated union instead of `recurring: Task | null`: - `{ status: 'created'; task: Task }` — copy created or updated successfully -- `{ status: 'skipped_invalid_rule' }` — repeat_rule is non-empty but parsing failed, OR an exception occurred during copy creation +- `{ status: 'skipped_invalid_rule' }` — repeat_rule is non-empty but parsing failed +- `{ status: 'error_creating_copy' }` — repeat_rule parsed successfully but an exception was thrown during next-date calculation or copy creation/update - `{ status: 'not_recurring' }` — repeat_rule is empty -The completion itself SHALL NOT be interrupted regardless of recurringResult status. +The completion itself SHALL NOT be interrupted regardless of recurringResult status. `skipped_invalid_rule` SHALL be returned ONLY for a genuine parse failure — an exception during copy creation SHALL return `error_creating_copy`, and the caught error SHALL be logged. #### Scenario: Complete repeating task creates a copy - **GIVEN** active task "Morning routine" with a valid daily repeat_rule @@ -358,11 +359,11 @@ The completion itself SHALL NOT be interrupted regardless of recurringResult sta - **WHEN** user completes the task - **THEN** `recurringResult` has status "not_recurring" -#### Scenario: Exception during copy creation returns skipped status +#### Scenario: Exception during copy creation returns error status - **GIVEN** active task with valid repeat_rule but copy creation fails with an error - **WHEN** user completes the task - **THEN** the task is marked as completed -- **AND** `recurringResult` has status "skipped_invalid_rule" +- **AND** `recurringResult` has status "error_creating_copy" - **AND** the error is logged #### Scenario: Recurring copy preserves original_task_id chain diff --git a/openspec/specs/search/spec.md b/openspec/specs/search/spec.md index 3fa5f01c..fd963cf2 100644 --- a/openspec/specs/search/spec.md +++ b/openspec/specs/search/spec.md @@ -83,3 +83,30 @@ isSearching SHALL be true while search is in progress and false after completion - **WHEN** search fails with error - **THEN** isSearching is false + +### Requirement: Mutations from search schedule a sync push + +Every data mutation initiated from the search page SHALL call `schedulePush()` after the local write succeeds, consistent with the sync-orchestration rule that any local data mutation schedules a debounced push. This covers task mutations — complete, noncomplete, update, move to box, soft delete, duplicate — and idea mutations — update, soft delete. `schedulePush` SHALL be obtained via `useSync()`. + +#### Scenario: Completing a task from search results schedules a push + +- **WHEN** the user completes (or uncompletes) a task from the search results list +- **THEN** the task is written locally +- **AND** `schedulePush()` is called exactly once + +#### Scenario: Editing, moving, deleting, or duplicating a task from search schedules a push + +- **WHEN** the user updates, moves, soft-deletes, or duplicates a task from search results or the task detail panel opened from search +- **THEN** the mutation is written locally +- **AND** `schedulePush()` is called exactly once per mutation + +#### Scenario: Editing or deleting an idea from search schedules a push + +- **WHEN** the user updates or soft-deletes an idea from the idea detail panel opened from search +- **THEN** the mutation is written locally +- **AND** `schedulePush()` is called exactly once per mutation + +#### Scenario: Search itself does not schedule a push + +- **WHEN** the user types a query and search executes (no mutation) +- **THEN** `schedulePush()` is not called diff --git a/openspec/specs/settings/spec.md b/openspec/specs/settings/spec.md index e3e2de69..c65c5401 100644 --- a/openspec/specs/settings/spec.md +++ b/openspec/specs/settings/spec.md @@ -67,15 +67,6 @@ The system SHALL provide a method to retrieve all settings where `needsSync` is - **AND** `clearNeedsSyncByKey(["accent_color"])` is called - **THEN** "accent_color" has `needsSync: false` and "default_box" still has `needsSync: true` -### Requirement: Get settings changed since timestamp - -The system SHALL provide a method to retrieve settings with `updated_at` strictly greater than a given timestamp. - -#### Scenario: Filter settings by updated_at -- **WHEN** setting A has `updated_at: "2025-01-01T00:00:00.000Z"` and setting B has `updated_at: "2025-01-02T00:00:00.000Z"` -- **AND** `getChangedSince("2025-01-01T00:00:00.000Z")` is called -- **THEN** only setting B is returned - ### Requirement: Bulk upsert with conflict resolution The system SHALL accept an array of server settings and apply them with conflict resolution: (1) skip if local setting has `needsSync: true` (local dirty wins), (2) skip if server `updated_at` is not newer than local, (3) accept and overwrite otherwise with `needsSync: false`. diff --git a/packages/client/src/db/repositories/AttachmentRepository.ts b/packages/client/src/db/repositories/AttachmentRepository.ts index a1b3fdd8..d1e3b307 100644 --- a/packages/client/src/db/repositories/AttachmentRepository.ts +++ b/packages/client/src/db/repositories/AttachmentRepository.ts @@ -70,10 +70,6 @@ export class AttachmentRepository { await db.attachments.bulkPut(attachments); } - async getChangedSince(since: string): Promise { - return db.attachments.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.attachments .filter((attachment) => attachment.syncStatus === "pending") diff --git a/packages/client/src/db/repositories/CategoryRepository.ts b/packages/client/src/db/repositories/CategoryRepository.ts index 7b35f2dc..91b17310 100644 --- a/packages/client/src/db/repositories/CategoryRepository.ts +++ b/packages/client/src/db/repositories/CategoryRepository.ts @@ -48,10 +48,6 @@ export class CategoryRepository { await db.categories.bulkPut(categories); } - async getChangedSince(since: string): Promise { - return db.categories.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.categories .filter((category) => category.syncStatus === "pending") diff --git a/packages/client/src/db/repositories/ChecklistRepository.queries.test.ts b/packages/client/src/db/repositories/ChecklistRepository.queries.test.ts index c426e205..17ad68f6 100644 --- a/packages/client/src/db/repositories/ChecklistRepository.queries.test.ts +++ b/packages/client/src/db/repositories/ChecklistRepository.queries.test.ts @@ -162,38 +162,6 @@ describe("ChecklistRepository", () => { }); }); - describe("getChangedSince", () => { - it("should return items updated after the given timestamp", async () => { - const oldItem = buildChecklistItem({ - updated_at: "2025-01-01T00:00:00.000Z", - }); - const newItem = buildChecklistItem({ - updated_at: "2026-06-01T00:00:00.000Z", - }); - await db.checklist_items.bulkAdd([oldItem, newItem]); - - const items = await getRepository().getChangedSince( - "2026-01-01T00:00:00.000Z", - ); - - expect(items).toHaveLength(1); - expect(items[0].id).toBe(newItem.id); - }); - - it("should return empty array when no items changed since timestamp", async () => { - const oldItem = buildChecklistItem({ - updated_at: "2025-01-01T00:00:00.000Z", - }); - await db.checklist_items.add(oldItem); - - const items = await getRepository().getChangedSince( - "2026-01-01T00:00:00.000Z", - ); - - expect(items).toEqual([]); - }); - }); - describe("getNeedingSync", () => { it("should return only items with syncStatus true", async () => { const syncItem = buildChecklistItem({ syncStatus: "pending" as const }); diff --git a/packages/client/src/db/repositories/ChecklistRepository.ts b/packages/client/src/db/repositories/ChecklistRepository.ts index bb1c6411..ad006dae 100644 --- a/packages/client/src/db/repositories/ChecklistRepository.ts +++ b/packages/client/src/db/repositories/ChecklistRepository.ts @@ -65,10 +65,6 @@ export class ChecklistRepository { await db.checklist_items.bulkPut(items); } - async getChangedSince(since: string): Promise { - return db.checklist_items.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.checklist_items .filter((item) => item.syncStatus === "pending") diff --git a/packages/client/src/db/repositories/ContextRepository.ts b/packages/client/src/db/repositories/ContextRepository.ts index 859a570d..12bc6c9b 100644 --- a/packages/client/src/db/repositories/ContextRepository.ts +++ b/packages/client/src/db/repositories/ContextRepository.ts @@ -48,10 +48,6 @@ export class ContextRepository { await db.contexts.bulkPut(contexts); } - async getChangedSince(since: string): Promise { - return db.contexts.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.contexts .filter((context) => context.syncStatus === "pending") diff --git a/packages/client/src/db/repositories/GoalRepository.ts b/packages/client/src/db/repositories/GoalRepository.ts index 9d807f97..0487c3f5 100644 --- a/packages/client/src/db/repositories/GoalRepository.ts +++ b/packages/client/src/db/repositories/GoalRepository.ts @@ -48,10 +48,6 @@ export class GoalRepository { await db.goals.bulkPut(goals); } - async getChangedSince(since: string): Promise { - return db.goals.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.goals.filter((goal) => goal.syncStatus === "pending").toArray(); } diff --git a/packages/client/src/db/repositories/IdeaRepository.ts b/packages/client/src/db/repositories/IdeaRepository.ts index 2d7f46a2..2eece604 100644 --- a/packages/client/src/db/repositories/IdeaRepository.ts +++ b/packages/client/src/db/repositories/IdeaRepository.ts @@ -48,10 +48,6 @@ export class IdeaRepository { await db.ideas.bulkPut(ideas); } - async getChangedSince(since: string): Promise { - return db.ideas.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.ideas.filter((idea) => idea.syncStatus === "pending").toArray(); } diff --git a/packages/client/src/db/repositories/SettingsRepository.sync.test.ts b/packages/client/src/db/repositories/SettingsRepository.sync.test.ts deleted file mode 100644 index 24f6ee88..00000000 --- a/packages/client/src/db/repositories/SettingsRepository.sync.test.ts +++ /dev/null @@ -1,66 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - buildSetting, - createSettingsRepositorySetup, - db, - Temporal, - toISOTimestamp, -} from "./SettingsRepository.test-utils"; - -describe("SettingsRepository", () => { - const { getRepository } = createSettingsRepositorySetup(); - - describe("getChangedSince", () => { - it("should return settings with updated_at after since", async () => { - const oldSetting = buildSetting({ - key: "default_box", - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-01-01T00:00:00.000Z"), - ), - }); - const newSetting = buildSetting({ - key: "accent_color", - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-03-01T00:00:00.000Z"), - ), - }); - await db.settings.bulkAdd([oldSetting, newSetting]); - - const settings = await getRepository().getChangedSince( - toISOTimestamp(Temporal.Instant.from("2026-02-01T00:00:00.000Z")), - ); - expect(settings).toHaveLength(1); - expect(settings[0].key).toBe(newSetting.key); - }); - - it("should return empty array when no settings are newer than since", async () => { - await db.settings.add( - buildSetting({ - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-01-01T00:00:00.000Z"), - ), - }), - ); - - const settings = await getRepository().getChangedSince( - toISOTimestamp(Temporal.Instant.from("2026-06-01T00:00:00.000Z")), - ); - expect(settings).toEqual([]); - }); - - it("should not include settings with updated_at equal to since", async () => { - await db.settings.add( - buildSetting({ - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-03-01T00:00:00.000Z"), - ), - }), - ); - - const settings = await getRepository().getChangedSince( - toISOTimestamp(Temporal.Instant.from("2026-03-01T00:00:00.000Z")), - ); - expect(settings).toEqual([]); - }); - }); -}); diff --git a/packages/client/src/db/repositories/SettingsRepository.ts b/packages/client/src/db/repositories/SettingsRepository.ts index 44665739..5f8f9051 100644 --- a/packages/client/src/db/repositories/SettingsRepository.ts +++ b/packages/client/src/db/repositories/SettingsRepository.ts @@ -43,10 +43,6 @@ export class SettingsRepository { await db.settings.put(setting); } - async getChangedSince(since: string): Promise { - return db.settings.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.settings .filter((setting) => setting.syncStatus === "pending") diff --git a/packages/client/src/db/repositories/TaskRepository.sync.test.ts b/packages/client/src/db/repositories/TaskRepository.sync.test.ts index 50d15794..215951e3 100644 --- a/packages/client/src/db/repositories/TaskRepository.sync.test.ts +++ b/packages/client/src/db/repositories/TaskRepository.sync.test.ts @@ -1,7 +1,5 @@ import { describe, expect, it } from "vitest"; -import { Temporal } from "@/lib/temporal"; import { buildTask } from "@/test/factories/taskFactory"; -import { toISOTimestamp } from "@/utils/dateHelpers"; import { db } from "../database"; import { createTaskRepositorySetup } from "./TaskRepository.test-setup"; @@ -42,69 +40,6 @@ describe("TaskRepository", () => { }); }); - describe("getChangedSince", () => { - it("should return tasks with updated_at after since", async () => { - const oldTask = buildTask({ - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-01-01T00:00:00.000Z"), - ), - }); - const newTask = buildTask({ - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-03-01T00:00:00.000Z"), - ), - }); - await db.tasks.bulkAdd([oldTask, newTask]); - - const tasks = await getRepository().getChangedSince( - "2026-02-01T00:00:00.000Z", - ); - expect(tasks).toHaveLength(1); - expect(tasks[0].id).toBe(newTask.id); - }); - - it("should return empty array when no tasks are newer than since", async () => { - const task = buildTask({ - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-01-01T00:00:00.000Z"), - ), - }); - await db.tasks.add(task); - - const tasks = await getRepository().getChangedSince( - "2026-06-01T00:00:00.000Z", - ); - expect(tasks).toEqual([]); - }); - - it("should not include tasks with updated_at equal to since", async () => { - const timestamp = toISOTimestamp( - Temporal.Instant.from("2026-03-01T00:00:00.000Z"), - ); - const task = buildTask({ updated_at: timestamp }); - await db.tasks.add(task); - - const tasks = await getRepository().getChangedSince(timestamp); - expect(tasks).toEqual([]); - }); - - it("should include soft-deleted tasks that changed after since", async () => { - const deletedTask = buildTask({ - is_deleted: true, - updated_at: toISOTimestamp( - Temporal.Instant.from("2026-03-01T00:00:00.000Z"), - ), - }); - await db.tasks.add(deletedTask); - - const tasks = await getRepository().getChangedSince( - "2026-01-01T00:00:00.000Z", - ); - expect(tasks).toHaveLength(1); - expect(tasks[0].id).toBe(deletedTask.id); - }); - }); - describe("getNeedingSync", () => { it("should return only syncStatus tasks", async () => { const syncStatusTask = buildTask({ syncStatus: "pending" as const }); diff --git a/packages/client/src/db/repositories/TaskRepository.ts b/packages/client/src/db/repositories/TaskRepository.ts index 3fe569b1..da2db6fa 100644 --- a/packages/client/src/db/repositories/TaskRepository.ts +++ b/packages/client/src/db/repositories/TaskRepository.ts @@ -111,10 +111,6 @@ export class TaskRepository { .toArray(); } - async getChangedSince(since: string): Promise { - return db.tasks.where("updated_at").above(since).toArray(); - } - async getNeedingSync(): Promise { return db.tasks.filter((task) => task.syncStatus === "pending").toArray(); } diff --git a/packages/client/src/hooks/useInboxTasks.test.ts b/packages/client/src/hooks/useInboxTasks.test.ts index 5a6d9e57..4e2aebe3 100644 --- a/packages/client/src/hooks/useInboxTasks.test.ts +++ b/packages/client/src/hooks/useInboxTasks.test.ts @@ -200,34 +200,38 @@ describe("useInboxTasks", () => { expect(firstService[serviceMethod]).not.toHaveBeenCalled(); }); - it("should add alert when completing task with invalid repeat rule", async () => { - const task = buildTask({ box: "inbox", name: "Inbox Bad Rule" }); - mockTaskService = createMockTaskService({ - getByBox: vi.fn().mockResolvedValue([task]), - complete: vi.fn().mockResolvedValue({ - completed: task, - recurringResult: { status: "skipped_invalid_rule" }, - }), - }); - const { result } = renderHook(() => useInboxTasks(mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.completeTask(task.id); - }); - - expect(mockAddAlerts).toHaveBeenCalledWith([ - { type: "repeat_rule_invalid", taskNames: ["Inbox Bad Rule"] }, - ]); - }); - - it("should not add alert when completing task with valid recurring result", async () => { - const task = buildTask({ box: "inbox" }); + it.each([ + { + label: "invalid repeat rule", + taskName: "Inbox Bad Rule", + recurringStatus: "skipped_invalid_rule" as const, + expectedAlerts: [ + { type: "repeat_rule_invalid", taskNames: ["Inbox Bad Rule"] }, + ], + }, + { + label: "valid recurring result", + taskName: "Inbox Task", + recurringStatus: "not_recurring" as const, + expectedAlerts: null, + }, + { + label: "error_creating_copy recurring result", + taskName: "Inbox Copy Error", + recurringStatus: "error_creating_copy" as const, + expectedAlerts: null, + }, + ])("should add alert only when completing task with $label", async ({ + taskName, + recurringStatus, + expectedAlerts, + }) => { + const task = buildTask({ box: "inbox", name: taskName }); mockTaskService = createMockTaskService({ getByBox: vi.fn().mockResolvedValue([task]), complete: vi.fn().mockResolvedValue({ completed: task, - recurringResult: { status: "not_recurring" }, + recurringResult: { status: recurringStatus }, }), }); const { result } = renderHook(() => useInboxTasks(mockTaskService)); @@ -237,6 +241,10 @@ describe("useInboxTasks", () => { await result.current.completeTask(task.id); }); - expect(mockAddAlerts).not.toHaveBeenCalled(); + if (expectedAlerts) { + expect(mockAddAlerts).toHaveBeenCalledWith(expectedAlerts); + } else { + expect(mockAddAlerts).not.toHaveBeenCalled(); + } }); }); diff --git a/packages/client/src/hooks/useInboxTasks.ts b/packages/client/src/hooks/useInboxTasks.ts index 1d48320a..a9c3e81f 100644 --- a/packages/client/src/hooks/useInboxTasks.ts +++ b/packages/client/src/hooks/useInboxTasks.ts @@ -1,5 +1,4 @@ import { useCallback, useEffect, useState } from "react"; -import { useAlerts } from "@/app/providers/AlertProvider"; import { useSync } from "@/app/providers/SyncProvider"; import { BOX } from "@/constants"; import { systemClock } from "@/lib/temporal"; @@ -9,6 +8,7 @@ import type { Task } from "@/types/entities"; import { getLogicalDate } from "@/utils/getLogicalDate"; import { getCachedDayBoundary } from "./useSettings"; +import { useTaskCompletionAlerts } from "./useTaskCompletionAlerts"; export interface UseInboxTasksReturn { tasks: Task[]; @@ -22,7 +22,7 @@ export function useInboxTasks( ): UseInboxTasksReturn { const [tasks, setTasks] = useState([]); const [isLoading, setIsLoading] = useState(true); - const { addAlerts } = useAlerts(); + const { raiseCompletionAlerts } = useTaskCompletionAlerts(); const { syncVersion, schedulePush } = useSync(); const loadTasks = useCallback(async () => { @@ -43,15 +43,12 @@ export function useInboxTasks( id, logicalDate, ); - if (recurringResult.status === "skipped_invalid_rule") { - addAlerts([ - { type: "repeat_rule_invalid", taskNames: [completed.name] }, - ]); - } + // implements FR3, FR5 of fix-recurring-completion-error-masking + raiseCompletionAlerts(recurringResult, completed?.name ?? ""); await loadTasks(); schedulePush(); }, - [taskService, loadTasks, schedulePush, addAlerts], + [taskService, loadTasks, schedulePush, raiseCompletionAlerts], ); const deleteTask = useCallback( diff --git a/packages/client/src/hooks/useTask.test.ts b/packages/client/src/hooks/useTask.test.ts deleted file mode 100644 index adbf2b6b..00000000 --- a/packages/client/src/hooks/useTask.test.ts +++ /dev/null @@ -1,182 +0,0 @@ -import { act, renderHook, waitFor } from "@testing-library/react"; -import { beforeEach, describe, expect, it, vi } from "vitest"; -import { BOX } from "@/constants"; -import type { TaskService } from "@/services/TaskService"; -import { buildTask } from "@/test/factories/taskFactory"; -import { createMockTaskService } from "@/test/mocks/taskServiceMock"; -import { useTask } from "./useTask"; - -const mockAddAlerts = vi.fn(); - -vi.mock("@/app/providers/AlertProvider", () => ({ - useAlerts: () => ({ - alerts: [], - addAlerts: mockAddAlerts, - dismissAlerts: vi.fn(), - }), -})); - -describe("useTask", () => { - let mockTaskService: TaskService; - - beforeEach(() => { - mockTaskService = createMockTaskService(); - mockAddAlerts.mockClear(); - }); - - it("should set isLoading to true on initial render", () => { - const { result } = renderHook(() => useTask("task-1", mockTaskService)); - expect(result.current.isLoading).toBe(true); - }); - - it("should set isLoading to false after task is fetched", async () => { - const { result } = renderHook(() => useTask("task-1", mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - }); - - it("should return undefined when task is not found", async () => { - const { result } = renderHook(() => - useTask("nonexistent", mockTaskService), - ); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - expect(result.current.task).toBeUndefined(); - }); - - it("should return the task after loading", async () => { - const task = buildTask(); - mockTaskService = createMockTaskService({ - getById: vi.fn().mockResolvedValue(task), - }); - const { result } = renderHook(() => useTask(task.id, mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - expect(result.current.task).toEqual(task); - }); - - it("should call getById with the given id", async () => { - const { result } = renderHook(() => useTask("task-abc", mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - expect(mockTaskService.getById).toHaveBeenCalledWith("task-abc"); - }); - - it("should call update and refresh when updateTask is called", async () => { - const task = buildTask(); - const updatedTask = { ...task, name: "Updated" }; - const mockGetById = vi - .fn() - .mockResolvedValueOnce(task) - .mockResolvedValueOnce(updatedTask); - mockTaskService = createMockTaskService({ getById: mockGetById }); - const { result } = renderHook(() => useTask(task.id, mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.updateTask({ name: "Updated" }); - }); - - expect(mockTaskService.update).toHaveBeenCalledWith(task.id, { - name: "Updated", - }); - expect(mockGetById).toHaveBeenCalledTimes(2); - }); - - it("should call complete and refresh when completeTask is called", async () => { - const task = buildTask(); - const mockGetById = vi.fn().mockResolvedValue(task); - mockTaskService = createMockTaskService({ getById: mockGetById }); - const { result } = renderHook(() => useTask(task.id, mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.completeTask(); - }); - - expect(mockTaskService.complete).toHaveBeenCalledWith( - task.id, - expect.any(String), - ); - expect(mockGetById).toHaveBeenCalledTimes(2); - }); - - it("should call softDelete when deleteTask is called", async () => { - const task = buildTask(); - mockTaskService = createMockTaskService({ - getById: vi.fn().mockResolvedValue(task), - }); - const { result } = renderHook(() => useTask(task.id, mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.deleteTask(); - }); - - expect(mockTaskService.softDelete).toHaveBeenCalledWith(task.id); - }); - - it("should call moveToBox and refresh when moveTask is called", async () => { - const task = buildTask({ box: "inbox" }); - const mockGetById = vi.fn().mockResolvedValue(task); - mockTaskService = createMockTaskService({ getById: mockGetById }); - const { result } = renderHook(() => useTask(task.id, mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.moveTask(BOX.TODAY); - }); - - expect(mockTaskService.moveToBox).toHaveBeenCalledWith(task.id, BOX.TODAY); - expect(mockGetById).toHaveBeenCalledTimes(2); - }); - - it("should not call update when task is undefined", async () => { - const { result } = renderHook(() => - useTask("nonexistent", mockTaskService), - ); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.updateTask({ name: "X" }); - }); - - expect(mockTaskService.update).not.toHaveBeenCalled(); - }); - - it("should add alert when completing task with invalid repeat rule", async () => { - const task = buildTask({ name: "Detail Bad Rule" }); - mockTaskService = createMockTaskService({ - getById: vi.fn().mockResolvedValue(task), - complete: vi.fn().mockResolvedValue({ - completed: task, - recurringResult: { status: "skipped_invalid_rule" }, - }), - }); - const { result } = renderHook(() => useTask(task.id, mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.completeTask(); - }); - - expect(mockAddAlerts).toHaveBeenCalledWith([ - { type: "repeat_rule_invalid", taskNames: ["Detail Bad Rule"] }, - ]); - }); - - it("should not add alert when completing task with valid recurring result", async () => { - const task = buildTask(); - mockTaskService = createMockTaskService({ - getById: vi.fn().mockResolvedValue(task), - complete: vi.fn().mockResolvedValue({ - completed: task, - recurringResult: { status: "not_recurring" }, - }), - }); - const { result } = renderHook(() => useTask(task.id, mockTaskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - await act(async () => { - await result.current.completeTask(); - }); - - expect(mockAddAlerts).not.toHaveBeenCalled(); - }); -}); diff --git a/packages/client/src/hooks/useTask.ts b/packages/client/src/hooks/useTask.ts deleted file mode 100644 index 2a2614a0..00000000 --- a/packages/client/src/hooks/useTask.ts +++ /dev/null @@ -1,76 +0,0 @@ -import { useCallback, useEffect, useState } from "react"; -import { useAlerts } from "@/app/providers/AlertProvider"; -import { systemClock } from "@/lib/temporal"; -import { defaultTaskService } from "@/services/defaultServices"; -import type { TaskService } from "@/services/TaskService"; -import type { Box } from "@/types/common"; -import type { Task } from "@/types/entities"; -import { getLogicalDate } from "@/utils/getLogicalDate"; - -import { getCachedDayBoundary } from "./useSettings"; - -export interface UseTaskReturn { - task: Task | undefined; - isLoading: boolean; - updateTask: (changes: Partial) => Promise; - completeTask: () => Promise; - deleteTask: () => Promise; - moveTask: (box: Box) => Promise; -} - -export function useTask( - id: string, - taskService: TaskService = defaultTaskService, -): UseTaskReturn { - const { addAlerts } = useAlerts(); - const [task, setTask] = useState(undefined); - const [isLoading, setIsLoading] = useState(true); - - const loadTask = useCallback(async () => { - const foundTask = await taskService.getById(id); - setTask(foundTask); - setIsLoading(false); - }, [taskService, id]); - - useEffect(() => { - void loadTask(); - }, [loadTask]); - - const updateTask = useCallback( - async (changes: Partial) => { - if (!task) return; - await taskService.update(task.id, changes); - await loadTask(); - }, - [taskService, task, loadTask], - ); - - const completeTask = useCallback(async () => { - if (!task) return; - const logicalDate = getLogicalDate(systemClock, getCachedDayBoundary()); - const { recurringResult } = await taskService.complete( - task.id, - logicalDate, - ); - if (recurringResult.status === "skipped_invalid_rule") { - addAlerts([{ type: "repeat_rule_invalid", taskNames: [task.name] }]); - } - await loadTask(); - }, [taskService, task, loadTask, addAlerts]); - - const deleteTask = useCallback(async () => { - if (!task) return; - await taskService.softDelete(task.id); - }, [taskService, task]); - - const moveTask = useCallback( - async (box: Box) => { - if (!task) return; - await taskService.moveToBox(task.id, box); - await loadTask(); - }, - [taskService, task, loadTask], - ); - - return { task, isLoading, updateTask, completeTask, deleteTask, moveTask }; -} diff --git a/packages/client/src/hooks/useTaskCompletionAlerts.test.ts b/packages/client/src/hooks/useTaskCompletionAlerts.test.ts new file mode 100644 index 00000000..368c9162 --- /dev/null +++ b/packages/client/src/hooks/useTaskCompletionAlerts.test.ts @@ -0,0 +1,53 @@ +// implements FR5 of fix-recurring-completion-error-masking +import { renderHook } from "@testing-library/react"; +import { beforeEach, describe, expect, it } from "vitest"; +import { mockAddAlerts } from "@/app/providers/__mocks__/AlertProvider"; +import type { RecurringResult } from "@/services/TaskService"; +import { buildTask } from "@/test/factories/taskFactory"; +import { useTaskCompletionAlerts } from "./useTaskCompletionAlerts"; + +vi.mock( + "@/app/providers/AlertProvider", + async () => import("@/app/providers/__mocks__/AlertProvider"), +); + +describe("useTaskCompletionAlerts", () => { + beforeEach(() => { + mockAddAlerts.mockClear(); + }); + + describe("when recurringResult status is skipped_invalid_rule", () => { + it("should call addAlerts once with a repeat_rule_invalid alert for the task name", () => { + const task = buildTask({ name: "Water the plants" }); + const recurringResult: RecurringResult = { + status: "skipped_invalid_rule", + }; + const { result } = renderHook(() => useTaskCompletionAlerts()); + + result.current.raiseCompletionAlerts(recurringResult, task.name); + + expect(mockAddAlerts).toHaveBeenCalledOnce(); + expect(mockAddAlerts).toHaveBeenCalledWith([ + { type: "repeat_rule_invalid", taskNames: [task.name] }, + ]); + }); + }); + + describe.each<{ status: RecurringResult["status"] }>([ + { status: "error_creating_copy" }, + { status: "created" }, + { status: "not_recurring" }, + ])("when recurringResult status is $status", ({ status }) => { + it("should not call addAlerts", () => { + const task = buildTask({ name: "Water the plants" }); + const recurringResult = ( + status === "created" ? { status, task } : { status } + ) as RecurringResult; + const { result } = renderHook(() => useTaskCompletionAlerts()); + + result.current.raiseCompletionAlerts(recurringResult, task.name); + + expect(mockAddAlerts).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/client/src/hooks/useTaskCompletionAlerts.ts b/packages/client/src/hooks/useTaskCompletionAlerts.ts new file mode 100644 index 00000000..12fc9979 --- /dev/null +++ b/packages/client/src/hooks/useTaskCompletionAlerts.ts @@ -0,0 +1,33 @@ +import { useCallback } from "react"; +import { useAlerts } from "@/app/providers/AlertProvider"; +import type { RecurringResult } from "@/services/TaskService"; + +export interface UseTaskCompletionAlertsReturn { + raiseCompletionAlerts: ( + recurringResult: RecurringResult, + taskName: string, + ) => void; +} + +/** + * Implements FR5, FR6 of fix-recurring-completion-error-masking. + * + * Raises a repeat_rule_invalid alert when completing a task reveals + * an unparseable repeat rule, so the failure surfaces to the user + * instead of being silently swallowed. Shared by all completion call + * sites (useTasks, useInboxTasks, useTaskMutations, SearchPage). + */ +export function useTaskCompletionAlerts(): UseTaskCompletionAlertsReturn { + const { addAlerts } = useAlerts(); + + const raiseCompletionAlerts = useCallback( + (recurringResult: RecurringResult, taskName: string) => { + if (recurringResult.status === "skipped_invalid_rule") { + addAlerts([{ type: "repeat_rule_invalid", taskNames: [taskName] }]); + } + }, + [addAlerts], + ); + + return { raiseCompletionAlerts }; +} diff --git a/packages/client/src/hooks/useTaskMutations.complete.test.ts b/packages/client/src/hooks/useTaskMutations.complete.test.ts index b81c4e2f..f0d5653c 100644 --- a/packages/client/src/hooks/useTaskMutations.complete.test.ts +++ b/packages/client/src/hooks/useTaskMutations.complete.test.ts @@ -186,5 +186,24 @@ describe("useTaskMutations > completeTask", () => { expect(mockAddAlerts).not.toHaveBeenCalled(); }); + + it("should not add alert when recurringResult status is error_creating_copy", async () => { + const task = buildTask({ + name: "Copy Error Task", + is_completed: false, + repeat_rule: JSON.stringify({ type: "daily" }), + }); + ctx = createTestContext({ + getById: vi.fn().mockResolvedValue(task), + complete: vi.fn().mockResolvedValue({ + completed: task, + recurringResult: { status: "error_creating_copy" }, + }), + }); + + await completeAndReturn(ctx, task.id); + + expect(mockAddAlerts).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/client/src/hooks/useTaskMutations.ts b/packages/client/src/hooks/useTaskMutations.ts index 6132fb63..0c363ac6 100644 --- a/packages/client/src/hooks/useTaskMutations.ts +++ b/packages/client/src/hooks/useTaskMutations.ts @@ -1,5 +1,4 @@ import { useMemo } from "react"; -import { useAlerts } from "@/app/providers/AlertProvider"; import { systemClock } from "@/lib/temporal"; import type { TaskService } from "@/services/TaskService"; import type { Box } from "@/types/common"; @@ -7,6 +6,7 @@ import type { Task } from "@/types/entities"; import { getLogicalDate } from "@/utils/getLogicalDate"; import { useSyncWrapper } from "./useMutationHelpers"; import { getCachedDayBoundary } from "./useSettings"; +import { useTaskCompletionAlerts } from "./useTaskCompletionAlerts"; export interface UseTaskMutationsReturn { completeTask: (id: string) => Promise; @@ -20,7 +20,7 @@ export function useTaskMutations( taskService: TaskService, onReload: () => Promise, ): UseTaskMutationsReturn { - const { addAlerts } = useAlerts(); + const { raiseCompletionAlerts } = useTaskCompletionAlerts(); const withSync = useSyncWrapper(onReload); return useMemo( @@ -42,11 +42,8 @@ export function useTaskMutations( id, logicalDate, ); - if (recurringResult.status === "skipped_invalid_rule") { - addAlerts([ - { type: "repeat_rule_invalid", taskNames: [task.name] }, - ]); - } + // implements FR3, FR5 of fix-recurring-completion-error-masking + raiseCompletionAlerts(recurringResult, task.name); // Return ID only if the copy is NOT hidden recurringId = recurringResult.status === "created" && @@ -70,6 +67,6 @@ export function useTaskMutations( return withSync(() => taskService.duplicate(id), false); }, }), - [taskService, withSync, addAlerts], + [taskService, withSync, raiseCompletionAlerts], ); } diff --git a/packages/client/src/hooks/useTasks.complete.test.ts b/packages/client/src/hooks/useTasks.complete.test.ts index 71dd123f..8d07a767 100644 --- a/packages/client/src/hooks/useTasks.complete.test.ts +++ b/packages/client/src/hooks/useTasks.complete.test.ts @@ -84,6 +84,26 @@ describe("useTasks — completeTask", () => { expect(recurringId).toBeNull(); }); + it("should return recurring task id when task creates a visible copy", async () => { + const { result, task } = await setupHookWithOneTask({ + is_completed: false, + repeat_rule: JSON.stringify({ + type: "fixed", + frequency: "daily", + interval: 1, + target_box: "today", + advance_days: 1, + }), + }); + + let recurringId: string | null = null; + await act(async () => { + recurringId = await result.current.completeTask(task.id); + }); + + expect(recurringId).not.toBeNull(); + }); + it("should not return recurring task id when task creates hidden copy", async () => { const { result, task } = await setupHookWithOneTask({ is_completed: false, @@ -155,4 +175,29 @@ describe("useTasks — completeTask", () => { expect(mockAddAlerts).not.toHaveBeenCalled(); }); + + it("should not add repeat_rule_invalid alert when recurringResult status is error_creating_copy", async () => { + const { result, task } = await setupHookWithOneTask({ + is_completed: false, + repeat_rule: JSON.stringify({ + type: "fixed", + frequency: "daily", + interval: 1, + target_box: "today", + advance_days: 0, + }), + }); + vi.spyOn(taskService, "complete").mockResolvedValueOnce({ + completed: task, + recurringResult: { status: "error_creating_copy" }, + }); + + await act(async () => { + await result.current.completeTask(task.id); + }); + + expect(mockAddAlerts).not.toHaveBeenCalledWith([ + { type: "repeat_rule_invalid", taskNames: [task.name] }, + ]); + }); }); diff --git a/packages/client/src/hooks/useTasks.loading.test.ts b/packages/client/src/hooks/useTasks.loading.test.ts index 272d48a3..21f20ef6 100644 --- a/packages/client/src/hooks/useTasks.loading.test.ts +++ b/packages/client/src/hooks/useTasks.loading.test.ts @@ -6,6 +6,7 @@ import { buildTask } from "@/test/factories/taskFactory"; import { useTasks } from "./useTasks"; import { clearDatabase, + expectNoTasksAfterLoading, mockSchedulePush, setupHookWithOneTask, taskService, @@ -30,9 +31,11 @@ vi.mock("@/app/providers/SyncProvider", () => ({ }), })); +let mockShowHidden = false; + vi.mock("@/hooks/useShowHidden", () => ({ useShowHidden: () => ({ - showHidden: false, + showHidden: mockShowHidden, toggleShowHidden: vi.fn(), }), })); @@ -41,6 +44,7 @@ describe("useTasks — loading and filtering", () => { beforeEach(async () => { await clearDatabase(); mockSchedulePush.mockClear(); + mockShowHidden = false; }); it("should set isLoading to true on initial render", () => { @@ -66,16 +70,12 @@ describe("useTasks — loading and filtering", () => { it("should not return tasks from other boxes", async () => { await db.tasks.add(buildTask({ box: "inbox" })); - const { result } = renderHook(() => useTasks(BOX.TODAY, taskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - expect(result.current.tasks).toHaveLength(0); + await expectNoTasksAfterLoading(); }); it("should not return deleted tasks", async () => { await db.tasks.add(buildTask({ box: "today", is_deleted: true })); - const { result } = renderHook(() => useTasks(BOX.TODAY, taskService)); - await waitFor(() => expect(result.current.isLoading).toBe(false)); - expect(result.current.tasks).toHaveLength(0); + await expectNoTasksAfterLoading(); }); it("should reactively update when a task is written to DB externally", async () => { @@ -90,4 +90,17 @@ describe("useTasks — loading and filtering", () => { await waitFor(() => expect(result.current.tasks).toHaveLength(1)); }); + + it("should not return hidden tasks when showHidden is false", async () => { + await db.tasks.add(buildTask({ box: "today", is_hidden: true })); + await expectNoTasksAfterLoading(); + }); + + it("should return hidden tasks when showHidden is true", async () => { + mockShowHidden = true; + await db.tasks.add(buildTask({ box: "today", is_hidden: true })); + const { result } = renderHook(() => useTasks(BOX.TODAY, taskService)); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.tasks).toHaveLength(1); + }); }); diff --git a/packages/client/src/hooks/useTasks.test-utils.ts b/packages/client/src/hooks/useTasks.test-utils.ts index c1c02b2e..080e3c71 100644 --- a/packages/client/src/hooks/useTasks.test-utils.ts +++ b/packages/client/src/hooks/useTasks.test-utils.ts @@ -38,3 +38,9 @@ export async function clearDatabase() { await db.tasks.clear(); await db.checklist_items.clear(); } + +export async function expectNoTasksAfterLoading() { + const { result } = renderHook(() => useTasks(BOX.TODAY, taskService)); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.tasks).toHaveLength(0); +} diff --git a/packages/client/src/hooks/useTasks.ts b/packages/client/src/hooks/useTasks.ts index 2eef1da9..103a68cc 100644 --- a/packages/client/src/hooks/useTasks.ts +++ b/packages/client/src/hooks/useTasks.ts @@ -1,6 +1,5 @@ import { liveQuery } from "dexie"; import { useCallback, useEffect, useState } from "react"; -import { useAlerts } from "@/app/providers/AlertProvider"; import { useSync } from "@/app/providers/SyncProvider"; import { useShowHidden } from "@/hooks/useShowHidden"; import { systemClock } from "@/lib/temporal"; @@ -11,6 +10,7 @@ import type { Task } from "@/types/entities"; import { getLogicalDate } from "@/utils/getLogicalDate"; import { getCachedDayBoundary } from "./useSettings"; +import { useTaskCompletionAlerts } from "./useTaskCompletionAlerts"; export interface UseTasksReturn { tasks: Task[]; @@ -32,7 +32,7 @@ export function useTasks( const [tasks, setTasks] = useState([]); const [isLoading, setIsLoading] = useState(true); const { showHidden } = useShowHidden(); - const { addAlerts } = useAlerts(); + const { raiseCompletionAlerts } = useTaskCompletionAlerts(); const { schedulePush } = useSync(); useEffect(() => { @@ -67,9 +67,8 @@ export function useTasks( } else { const logicalDate = getLogicalDate(systemClock, getCachedDayBoundary()); const { recurringResult } = await taskService.complete(id, logicalDate); - if (recurringResult.status === "skipped_invalid_rule") { - addAlerts([{ type: "repeat_rule_invalid", taskNames: [task.name] }]); - } + // implements FR3, FR5 of fix-recurring-completion-error-masking + raiseCompletionAlerts(recurringResult, task.name); // Return ID only if the recurring copy is NOT hidden recurringId = recurringResult.status === "created" && @@ -80,7 +79,7 @@ export function useTasks( schedulePush(); return recurringId; }, - [taskService, schedulePush, addAlerts], + [taskService, schedulePush, raiseCompletionAlerts], ); const deleteTask = useCallback( diff --git a/packages/client/src/pages/SearchPage.callbackIdentity.test.tsx b/packages/client/src/pages/SearchPage.callbackIdentity.test.tsx new file mode 100644 index 00000000..e1b7c1dc --- /dev/null +++ b/packages/client/src/pages/SearchPage.callbackIdentity.test.tsx @@ -0,0 +1,439 @@ +/** + * Stale-closure / referential-identity tests for SearchPage's useCallback + * hooks. Guards the dependency arrays: if a dependency (e.g. searchQuery) is + * dropped from an array, the handler passed down to children keeps a stale + * closure instead of being recreated, and callers relying on the fresh value + * (e.g. `if (searchQuery) void search(searchQuery)`) would silently use an + * outdated query. + * implements FR1, FR2, FR3 of fix-search-page-sync-push (mutation coverage) + */ +import { fireEvent, render, screen } from "@testing-library/react"; +import type React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { mockSchedulePush } from "@/app/providers/__mocks__/SyncProvider"; +import { buildGoal } from "@/test/factories/goalFactory"; +import { buildIdea } from "@/test/factories/ideaFactory"; +import { buildTask } from "@/test/factories/taskFactory"; +import { createMockGoalService } from "@/test/mocks/goalServiceMock"; +import { createMockIdeaService } from "@/test/mocks/ideaServiceMock"; +import { createMockTaskService } from "@/test/mocks/taskServiceMock"; +import type { Box } from "@/types/common"; +import type { Goal, Idea, Task } from "@/types/entities"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock( + "@/app/providers/SyncProvider", + async () => import("@/app/providers/__mocks__/SyncProvider"), +); + +vi.mock("@/hooks/useGoals", () => ({ + useGoals: () => ({ goals: [], isLoading: false }), +})); +vi.mock("@/hooks/useContexts", () => ({ + useContexts: () => ({ contexts: [], isLoading: false }), +})); +vi.mock("@/hooks/useCategories", () => ({ + useCategories: () => ({ categories: [], isLoading: false }), +})); +vi.mock("@/hooks/useFocusMode", () => ({ + useFocusMode: () => ({ isFocusMode: false, focusOpacity: 1 }), +})); + +vi.mock("@/components/layout/SidebarShell", () => ({ + SidebarShell: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +// Capture the exact function references SearchPage hands to its children on +// every render, so tests can compare identity across re-renders. +interface CapturedTaskListProps { + onComplete: (id: string) => void; + onUpdate: (id: string, changes: Partial) => void; + onMove: (id: string, box: Box) => void; + onDelete: (id: string) => void; + onSelect: (id: string) => void; +} +interface CapturedTaskDetailPanelProps { + onUpdate: (id: string, changes: Partial) => void; + onMove: (id: string, box: Box) => void; + onDelete: (id: string) => void; + onDuplicate: (id: string) => void; + onClose: () => void; +} +interface CapturedIdeaDetailPanelProps { + onUpdate: (id: string, changes: Partial) => void; + onDelete: (id: string) => void; + onClose: () => void; +} +interface CapturedGoalItemProps { + onNavigate: (id: string) => void; +} + +const capturedTaskListProps: CapturedTaskListProps[] = []; +const capturedTaskDetailPanelProps: CapturedTaskDetailPanelProps[] = []; +const capturedIdeaDetailPanelProps: CapturedIdeaDetailPanelProps[] = []; +const capturedGoalItemProps: CapturedGoalItemProps[] = []; + +vi.mock("@/components/tasks/TaskList", () => ({ + TaskList: ( + props: CapturedTaskListProps & { + tasks: Task[]; + selectedTaskId: string | null; + }, + ) => { + capturedTaskListProps.push(props); + return ( +
+ {props.tasks.map((task) => ( +
+ {task.name} +
+ ))} +
+ ); + }, +})); + +vi.mock("@/components/tasks/TaskDetailPanel", () => ({ + TaskDetailPanel: (props: CapturedTaskDetailPanelProps & { task: Task }) => { + capturedTaskDetailPanelProps.push(props); + return ( +
+
+ ); + }, +})); + +vi.mock("@/components/ideas/IdeaItem", () => ({ + IdeaItem: ({ idea }: { idea: Idea }) => ( + {idea.name} + ), +})); + +vi.mock("@/components/ideas/IdeaDetailPanel", () => ({ + IdeaDetailPanel: (props: CapturedIdeaDetailPanelProps & { idea: Idea }) => { + capturedIdeaDetailPanelProps.push(props); + return ( +
+
+ ); + }, +})); + +vi.mock("@/components/goals/GoalItem", () => ({ + GoalItem: (props: CapturedGoalItemProps & { goal: Goal }) => { + capturedGoalItemProps.push(props); + return ( + + ); + }, +})); + +const mockNavigate = vi.fn(); +vi.mock("react-router-dom", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useNavigate: () => mockNavigate, + }; +}); + +const foundTask = buildTask({ name: "Search Result Task" }); +const foundGoal = buildGoal({ name: "Search Result Goal" }); +const foundIdea = buildIdea({ name: "Search Result Idea" }); + +const mockGetById = vi.fn().mockResolvedValue(foundTask); +const mockComplete = vi.fn().mockResolvedValue({ + completed: foundTask, + recurringResult: { status: "not_recurring" }, +}); +const mockNoncomplete = vi.fn().mockResolvedValue(undefined); +const mockUpdate = vi.fn().mockResolvedValue(undefined); +const mockMoveToBox = vi.fn().mockResolvedValue(undefined); +const mockSoftDelete = vi.fn().mockResolvedValue(undefined); +const mockDuplicate = vi.fn().mockResolvedValue(undefined); +const mockTaskSearchByName = vi.fn().mockResolvedValue([foundTask]); +const mockGoalSearchByName = vi.fn().mockResolvedValue([foundGoal]); + +const mockIdeaUpdate = vi.fn().mockResolvedValue(undefined); +const mockIdeaSoftDelete = vi.fn().mockResolvedValue(undefined); +const mockIdeaSearchByName = vi.fn().mockResolvedValue([foundIdea]); + +vi.mock("@/services/defaultServices", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + defaultTaskService: createMockTaskService({ + getById: (...args: unknown[]) => mockGetById(...args), + complete: (...args: unknown[]) => mockComplete(...args), + noncomplete: (...args: unknown[]) => mockNoncomplete(...args), + update: (...args: unknown[]) => mockUpdate(...args), + moveToBox: (...args: unknown[]) => mockMoveToBox(...args), + softDelete: (...args: unknown[]) => mockSoftDelete(...args), + duplicate: (...args: unknown[]) => mockDuplicate(...args), + searchByName: (...args: unknown[]) => mockTaskSearchByName(...args), + }), + defaultGoalService: createMockGoalService({ + searchByName: (...args: unknown[]) => mockGoalSearchByName(...args), + }), + defaultIdeaService: createMockIdeaService({ + update: (...args: unknown[]) => mockIdeaUpdate(...args), + softDelete: (...args: unknown[]) => mockIdeaSoftDelete(...args), + searchByName: (...args: unknown[]) => mockIdeaSearchByName(...args), + }), + }; +}); + +import SearchPage from "./SearchPage"; + +function renderSearchPage() { + return render( + + + , + ); +} + +async function typeSearchQuery(query: string) { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: query } }); + await screen.findByTestId(`complete-${foundTask.id}`); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith(query); + }); +} + +describe("SearchPage > useCallback referential identity (searchQuery dependency)", () => { + beforeEach(() => { + capturedTaskListProps.length = 0; + capturedTaskDetailPanelProps.length = 0; + capturedIdeaDetailPanelProps.length = 0; + capturedGoalItemProps.length = 0; + mockSchedulePush.mockClear(); + mockNavigate.mockClear(); + }); + + // The five handlers below all list `searchQuery` in their dependency + // array. If an ArrayDeclaration mutant empties that array, React would + // keep reusing the first render's callback instead of recreating it when + // searchQuery changes. + it("should recreate onComplete, onUpdate, onMove and onDelete passed to TaskList when searchQuery changes", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + const propsAfterFirstQuery = + capturedTaskListProps[capturedTaskListProps.length - 1]; + + await typeSearchQuery("second"); + const propsAfterSecondQuery = + capturedTaskListProps[capturedTaskListProps.length - 1]; + + expect(propsAfterSecondQuery.onComplete).not.toBe( + propsAfterFirstQuery.onComplete, + ); + expect(propsAfterSecondQuery.onUpdate).not.toBe( + propsAfterFirstQuery.onUpdate, + ); + expect(propsAfterSecondQuery.onMove).not.toBe(propsAfterFirstQuery.onMove); + expect(propsAfterSecondQuery.onDelete).not.toBe( + propsAfterFirstQuery.onDelete, + ); + }); + + it("should use the fresh searchQuery when completing a task after the query changed, not a stale one", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + await typeSearchQuery("second"); + mockTaskSearchByName.mockClear(); + + const latestProps = capturedTaskListProps[capturedTaskListProps.length - 1]; + latestProps.onComplete(foundTask.id); + + await vi.waitFor(() => { + expect(mockComplete).toHaveBeenCalledWith( + foundTask.id, + expect.any(String), + ); + }); + await vi.waitFor(() => { + // A stale closure would either re-search "first" or skip re-searching + // entirely; the fresh closure must re-search with "second". + expect(mockTaskSearchByName).toHaveBeenCalledWith("second"); + }); + }); + + it("should recreate onUpdate, onMove, onDelete and onDuplicate passed to TaskDetailPanel when searchQuery changes", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + fireEvent.click(screen.getByTestId(`select-${foundTask.id}`)); + await screen.findByTestId("task-detail-panel"); + const propsAfterFirstQuery = + capturedTaskDetailPanelProps[capturedTaskDetailPanelProps.length - 1]; + + await typeSearchQuery("second"); + const propsAfterSecondQuery = + capturedTaskDetailPanelProps[capturedTaskDetailPanelProps.length - 1]; + + expect(propsAfterSecondQuery.onUpdate).not.toBe( + propsAfterFirstQuery.onUpdate, + ); + expect(propsAfterSecondQuery.onMove).not.toBe(propsAfterFirstQuery.onMove); + expect(propsAfterSecondQuery.onDelete).not.toBe( + propsAfterFirstQuery.onDelete, + ); + expect(propsAfterSecondQuery.onDuplicate).not.toBe( + propsAfterFirstQuery.onDuplicate, + ); + }); + + it("should use the fresh searchQuery when duplicating a task after the query changed, not a stale one", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + fireEvent.click(screen.getByTestId(`select-${foundTask.id}`)); + const duplicateButtonAfterFirstQuery = await screen.findByTestId( + `duplicate-${foundTask.id}`, + ); + void duplicateButtonAfterFirstQuery; + await typeSearchQuery("second"); + mockTaskSearchByName.mockClear(); + + fireEvent.click(screen.getByTestId(`duplicate-${foundTask.id}`)); + + await vi.waitFor(() => { + expect(mockDuplicate).toHaveBeenCalledWith(foundTask.id); + }); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith("second"); + }); + }); + + it("should recreate onUpdate and onDelete passed to IdeaDetailPanel when searchQuery changes", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + fireEvent.click(await screen.findByTestId(`idea-item-${foundIdea.id}`)); + await screen.findByTestId("idea-detail-panel"); + const propsAfterFirstQuery = + capturedIdeaDetailPanelProps[capturedIdeaDetailPanelProps.length - 1]; + + await typeSearchQuery("second"); + const propsAfterSecondQuery = + capturedIdeaDetailPanelProps[capturedIdeaDetailPanelProps.length - 1]; + + expect(propsAfterSecondQuery.onUpdate).not.toBe( + propsAfterFirstQuery.onUpdate, + ); + expect(propsAfterSecondQuery.onDelete).not.toBe( + propsAfterFirstQuery.onDelete, + ); + }); + + it("should use the fresh searchQuery when soft-deleting an idea after the query changed, not a stale one", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + fireEvent.click(await screen.findByTestId(`idea-item-${foundIdea.id}`)); + await screen.findByTestId("idea-detail-panel"); + await typeSearchQuery("second"); + mockIdeaSearchByName.mockClear(); + + const latestProps = + capturedIdeaDetailPanelProps[capturedIdeaDetailPanelProps.length - 1]; + latestProps.onDelete(foundIdea.id); + + await vi.waitFor(() => { + expect(mockIdeaSoftDelete).toHaveBeenCalledWith(foundIdea.id); + }); + await vi.waitFor(() => { + expect(mockIdeaSearchByName).toHaveBeenCalledWith("second"); + }); + }); +}); + +describe("SearchPage > useCallback with stable dependencies (equivalent-mutant candidates)", () => { + beforeEach(() => { + capturedGoalItemProps.length = 0; + mockSchedulePush.mockClear(); + mockNavigate.mockClear(); + }); + + // handleNavigateToGoal depends only on `navigate` (stable across + // re-renders under react-router). Its identity is not expected to change, + // but the callback must still navigate using the id argument it receives + // at call time (never captures searchQuery or other page state). + it("should navigate to the goal detail route using the id passed at call time", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + const goalButton = await screen.findByTestId(`goal-item-${foundGoal.id}`); + + fireEvent.click(goalButton); + + expect(mockNavigate).toHaveBeenCalledWith(`/goals/${foundGoal.id}`); + }); + + // handleIdeaClick, handleIdeaClose, handleTaskSelect and + // handleTaskDetailClose all have an empty dependency array and only call a + // setState setter (React guarantees setState identity is stable forever). + // There is nothing outside `[]` for these closures to capture, so an + // ArrayDeclaration mutant to `[]` is behaviorally equivalent to the + // original for referential identity — both are stable across ALL + // re-renders. We instead assert their setState-driven behavior is correct + // on every call, including after unrelated state (searchQuery) changes. + it("should open and close the task detail panel correctly even after searchQuery has changed", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + await typeSearchQuery("second"); + + fireEvent.click(screen.getByTestId(`select-${foundTask.id}`)); + await screen.findByTestId("task-detail-panel"); + + fireEvent.click(screen.getByTestId("close-task-detail")); + expect(screen.queryByTestId("task-detail-panel")).not.toBeInTheDocument(); + }); + + it("should open and close the idea detail panel correctly even after searchQuery has changed", async () => { + renderSearchPage(); + await typeSearchQuery("first"); + await typeSearchQuery("second"); + + fireEvent.click(await screen.findByTestId(`idea-item-${foundIdea.id}`)); + await screen.findByTestId("idea-detail-panel"); + + fireEvent.click(screen.getByTestId("close-idea-detail")); + expect(screen.queryByTestId("idea-detail-panel")).not.toBeInTheDocument(); + }); +}); diff --git a/packages/client/src/pages/SearchPage.completion.test.tsx b/packages/client/src/pages/SearchPage.completion.test.tsx new file mode 100644 index 00000000..0dd518cc --- /dev/null +++ b/packages/client/src/pages/SearchPage.completion.test.tsx @@ -0,0 +1,169 @@ +/** + * Tests for SearchPage task completion alerting. + * implements FR5, FR6, U3 of fix-recurring-completion-error-masking + */ +import { fireEvent, render, screen } from "@testing-library/react"; +import type React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { mockAddAlerts } from "@/app/providers/__mocks__/AlertProvider"; +import { buildTask } from "@/test/factories/taskFactory"; +import type { Task } from "@/types/entities"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock( + "@/app/providers/AlertProvider", + async () => import("@/app/providers/__mocks__/AlertProvider"), +); + +vi.mock( + "@/app/providers/SyncProvider", + async () => import("@/app/providers/__mocks__/SyncProvider"), +); + +vi.mock("@/hooks/useGoals", () => ({ + useGoals: () => ({ goals: [], isLoading: false }), +})); +vi.mock("@/hooks/useContexts", () => ({ + useContexts: () => ({ contexts: [], isLoading: false }), +})); +vi.mock("@/hooks/useCategories", () => ({ + useCategories: () => ({ categories: [], isLoading: false }), +})); +vi.mock("@/hooks/useFocusMode", () => ({ + useFocusMode: () => ({ isFocusMode: false, focusOpacity: 1 }), +})); + +vi.mock("@/components/layout/SidebarShell", () => ({ + SidebarShell: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock("@/components/tasks/TaskList", () => ({ + TaskList: ({ + tasks, + onComplete, + }: { + tasks: Task[]; + onComplete: (id: string) => void; + }) => ( +
+ {tasks.map((task) => ( +
+ {task.name} +
+ ))} +
+ ), +})); + +const foundTask = buildTask({ + name: "Recurring Task", + is_completed: false, + repeat_rule: JSON.stringify({ type: "daily" }), +}); + +const mockGetById = vi.fn().mockResolvedValue(foundTask); +const mockComplete = vi.fn(); +const mockSearchByName = vi.fn().mockResolvedValue([foundTask]); + +vi.mock("@/services/defaultServices", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + defaultTaskService: { + getById: (...args: unknown[]) => mockGetById(...args), + complete: (...args: unknown[]) => mockComplete(...args), + noncomplete: vi.fn(), + searchByName: (...args: unknown[]) => mockSearchByName(...args), + }, + defaultGoalService: { + searchByName: vi.fn().mockResolvedValue([]), + }, + defaultIdeaService: { + searchByName: vi.fn().mockResolvedValue([]), + }, + }; +}); + +import SearchPage from "./SearchPage"; + +function renderSearchPage() { + return render( + + + , + ); +} + +async function typeSearchQuery(query: string) { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: query } }); + // useSearch triggers search on debounce; wait for the resulting task item + // so state updates from the debounced search settle before we act again. + const completeButton = await screen.findByTestId(`complete-${foundTask.id}`); + await vi.waitFor(() => { + expect(mockSearchByName).toHaveBeenCalledWith(query); + }); + return completeButton; +} + +describe("SearchPage > completion", () => { + beforeEach(() => { + mockAddAlerts.mockClear(); + }); + + it(// FR6, U3: SearchPage now raises the same alert as every other completion entry point + "should raise a repeat_rule_invalid alert when completing a task whose recurringResult is skipped_invalid_rule", async () => { + mockComplete.mockResolvedValue({ + completed: foundTask, + recurringResult: { status: "skipped_invalid_rule" }, + }); + renderSearchPage(); + + const completeButton = await typeSearchQuery("Recurring"); + fireEvent.click(completeButton); + + await vi.waitFor(() => { + expect(mockComplete).toHaveBeenCalledWith( + foundTask.id, + expect.any(String), + ); + }); + + await vi.waitFor(() => { + expect(mockAddAlerts).toHaveBeenCalledWith([ + { type: "repeat_rule_invalid", taskNames: [foundTask.name] }, + ]); + }); + }); + + it("should not raise an alert when completing a task whose recurringResult is not_recurring", async () => { + mockComplete.mockResolvedValue({ + completed: foundTask, + recurringResult: { status: "not_recurring" }, + }); + renderSearchPage(); + + const completeButton = await typeSearchQuery("Recurring"); + fireEvent.click(completeButton); + + await vi.waitFor(() => { + expect(mockComplete).toHaveBeenCalledWith( + foundTask.id, + expect.any(String), + ); + }); + + expect(mockAddAlerts).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/client/src/pages/SearchPage.schedulePush.idea.test.tsx b/packages/client/src/pages/SearchPage.schedulePush.idea.test.tsx new file mode 100644 index 00000000..bb4bad9a --- /dev/null +++ b/packages/client/src/pages/SearchPage.schedulePush.idea.test.tsx @@ -0,0 +1,152 @@ +/** + * RED tests: SearchPage idea mutation paths (update, softDelete) must + * schedule a push after mutating. + * implements FR2 of fix-search-page-sync-push (task 2.2) + */ +import { fireEvent, render, screen } from "@testing-library/react"; +import type React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { mockSchedulePush } from "@/app/providers/__mocks__/SyncProvider"; +import { buildIdea } from "@/test/factories/ideaFactory"; +import type { Idea } from "@/types/entities"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock( + "@/app/providers/SyncProvider", + async () => import("@/app/providers/__mocks__/SyncProvider"), +); + +vi.mock("@/hooks/useGoals", () => ({ + useGoals: () => ({ goals: [], isLoading: false }), +})); +vi.mock("@/hooks/useContexts", () => ({ + useContexts: () => ({ contexts: [], isLoading: false }), +})); +vi.mock("@/hooks/useCategories", () => ({ + useCategories: () => ({ categories: [], isLoading: false }), +})); +vi.mock("@/hooks/useFocusMode", () => ({ + useFocusMode: () => ({ isFocusMode: false, focusOpacity: 1 }), +})); + +vi.mock("@/components/layout/SidebarShell", () => ({ + SidebarShell: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock("@/components/ideas/IdeaItem", () => ({ + IdeaItem: ({ idea }: { idea: Idea }) => ( + {idea.name} + ), +})); + +vi.mock("@/components/ideas/IdeaDetailPanel", () => ({ + IdeaDetailPanel: ({ + idea, + onUpdate, + onDelete, + }: { + idea: Idea; + onUpdate: (id: string, changes: Partial) => void; + onDelete: (id: string) => void; + }) => ( +
+
+ ), +})); + +const foundIdea = buildIdea({ name: "Search Result Idea" }); + +const mockIdeaUpdate = vi.fn().mockResolvedValue(undefined); +const mockIdeaSoftDelete = vi.fn().mockResolvedValue(undefined); +const mockIdeaSearchByName = vi.fn().mockResolvedValue([foundIdea]); + +vi.mock("@/services/defaultServices", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + defaultTaskService: { + searchByName: vi.fn().mockResolvedValue([]), + }, + defaultGoalService: { + searchByName: vi.fn().mockResolvedValue([]), + }, + defaultIdeaService: { + update: (...args: unknown[]) => mockIdeaUpdate(...args), + softDelete: (...args: unknown[]) => mockIdeaSoftDelete(...args), + searchByName: (...args: unknown[]) => mockIdeaSearchByName(...args), + }, + }; +}); + +import SearchPage from "./SearchPage"; + +function renderSearchPage() { + return render( + + + , + ); +} + +async function typeSearchQueryAndOpenIdeaPanel(query: string) { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: query } }); + const ideaItem = await screen.findByTestId(`idea-item-${foundIdea.id}`); + await vi.waitFor(() => { + expect(mockIdeaSearchByName).toHaveBeenCalledWith(query); + }); + fireEvent.click(ideaItem.closest("button") as HTMLButtonElement); + return { + updateButton: await screen.findByTestId(`update-idea-${foundIdea.id}`), + deleteButton: screen.getByTestId(`delete-idea-${foundIdea.id}`), + }; +} + +describe("SearchPage > schedulePush on idea mutations", () => { + beforeEach(() => { + mockSchedulePush.mockClear(); + mockIdeaUpdate.mockClear(); + mockIdeaSoftDelete.mockClear(); + }); + + it("should call schedulePush exactly once after updating an idea", async () => { + renderSearchPage(); + + const { updateButton } = await typeSearchQueryAndOpenIdeaPanel("Search"); + fireEvent.click(updateButton); + + await vi.waitFor(() => { + expect(mockIdeaUpdate).toHaveBeenCalledWith(foundIdea.id, { + name: "Updated idea name", + }); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + + it("should call schedulePush exactly once after soft-deleting an idea", async () => { + renderSearchPage(); + + const { deleteButton } = await typeSearchQueryAndOpenIdeaPanel("Search"); + fireEvent.click(deleteButton); + + await vi.waitFor(() => { + expect(mockIdeaSoftDelete).toHaveBeenCalledWith(foundIdea.id); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/client/src/pages/SearchPage.schedulePush.search.test.tsx b/packages/client/src/pages/SearchPage.schedulePush.search.test.tsx new file mode 100644 index 00000000..02690595 --- /dev/null +++ b/packages/client/src/pages/SearchPage.schedulePush.search.test.tsx @@ -0,0 +1,103 @@ +/** + * Guard test: running a search (typing a query, no mutation) must call + * schedulePush() zero times. Guards against over-scheduling on pure reads. + * implements FR1, FR2 of fix-search-page-sync-push (task 2.3) + */ +import { fireEvent, render, screen } from "@testing-library/react"; +import type React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { mockSchedulePush } from "@/app/providers/__mocks__/SyncProvider"; +import { buildIdea } from "@/test/factories/ideaFactory"; +import { buildTask } from "@/test/factories/taskFactory"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock( + "@/app/providers/SyncProvider", + async () => import("@/app/providers/__mocks__/SyncProvider"), +); + +vi.mock("@/hooks/useGoals", () => ({ + useGoals: () => ({ goals: [], isLoading: false }), +})); +vi.mock("@/hooks/useContexts", () => ({ + useContexts: () => ({ contexts: [], isLoading: false }), +})); +vi.mock("@/hooks/useCategories", () => ({ + useCategories: () => ({ categories: [], isLoading: false }), +})); +vi.mock("@/hooks/useFocusMode", () => ({ + useFocusMode: () => ({ isFocusMode: false, focusOpacity: 1 }), +})); + +vi.mock("@/components/layout/SidebarShell", () => ({ + SidebarShell: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock("@/components/tasks/TaskList", () => ({ + TaskList: () =>
, +})); + +const foundTask = buildTask({ name: "Search Result Task" }); +const foundIdea = buildIdea({ name: "Search Result Idea" }); + +const mockTaskSearchByName = vi.fn().mockResolvedValue([foundTask]); +const mockGoalSearchByName = vi.fn().mockResolvedValue([]); +const mockIdeaSearchByName = vi.fn().mockResolvedValue([foundIdea]); + +vi.mock("@/services/defaultServices", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + defaultTaskService: { + searchByName: (...args: unknown[]) => mockTaskSearchByName(...args), + }, + defaultGoalService: { + searchByName: (...args: unknown[]) => mockGoalSearchByName(...args), + }, + defaultIdeaService: { + searchByName: (...args: unknown[]) => mockIdeaSearchByName(...args), + }, + }; +}); + +import SearchPage from "./SearchPage"; + +function renderSearchPage() { + return render( + + + , + ); +} + +describe("SearchPage > schedulePush on pure search (no mutation)", () => { + beforeEach(() => { + mockSchedulePush.mockClear(); + mockTaskSearchByName.mockClear(); + mockGoalSearchByName.mockClear(); + mockIdeaSearchByName.mockClear(); + }); + + it("should call schedulePush zero times after typing a search query with results", async () => { + renderSearchPage(); + + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: "Search" } }); + + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith("Search"); + }); + await vi.waitFor(() => { + expect(mockIdeaSearchByName).toHaveBeenCalledWith("Search"); + }); + + expect(mockSchedulePush).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/client/src/pages/SearchPage.schedulePush.task.test.tsx b/packages/client/src/pages/SearchPage.schedulePush.task.test.tsx new file mode 100644 index 00000000..4259676b --- /dev/null +++ b/packages/client/src/pages/SearchPage.schedulePush.task.test.tsx @@ -0,0 +1,202 @@ +/** + * RED tests: SearchPage task mutation paths (complete, noncomplete, update, + * moveToBox) must schedule a push after mutating. + * implements FR1 of fix-search-page-sync-push (task 2.1) + */ +import { fireEvent, render, screen } from "@testing-library/react"; +import type React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { mockSchedulePush } from "@/app/providers/__mocks__/SyncProvider"; +import { buildTask } from "@/test/factories/taskFactory"; +import { createMockGoalService } from "@/test/mocks/goalServiceMock"; +import { createMockIdeaService } from "@/test/mocks/ideaServiceMock"; +import { createMockTaskService } from "@/test/mocks/taskServiceMock"; +import type { Box } from "@/types/common"; +import type { Task } from "@/types/entities"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock( + "@/app/providers/SyncProvider", + async () => import("@/app/providers/__mocks__/SyncProvider"), +); + +vi.mock("@/hooks/useGoals", () => ({ + useGoals: () => ({ goals: [], isLoading: false }), +})); +vi.mock("@/hooks/useContexts", () => ({ + useContexts: () => ({ contexts: [], isLoading: false }), +})); +vi.mock("@/hooks/useCategories", () => ({ + useCategories: () => ({ categories: [], isLoading: false }), +})); +vi.mock("@/hooks/useFocusMode", () => ({ + useFocusMode: () => ({ isFocusMode: false, focusOpacity: 1 }), +})); + +vi.mock("@/components/layout/SidebarShell", () => ({ + SidebarShell: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock("@/components/tasks/TaskList", () => ({ + TaskList: ({ + tasks, + onComplete, + onUpdate, + onMove, + }: { + tasks: Task[]; + onComplete: (id: string) => void; + onUpdate: (id: string, changes: Partial) => void; + onMove: (id: string, box: Box) => void; + }) => ( +
+ {tasks.map((task) => ( +
+ {task.name} +
+ ))} +
+ ), +})); + +const foundTask = buildTask({ name: "Search Result Task" }); + +const mockGetById = vi.fn().mockResolvedValue(foundTask); +const mockComplete = vi.fn().mockResolvedValue({ + completed: foundTask, + recurringResult: { status: "not_recurring" }, +}); +const mockNoncomplete = vi.fn().mockResolvedValue(undefined); +const mockUpdate = vi.fn().mockResolvedValue(undefined); +const mockMoveToBox = vi.fn().mockResolvedValue(undefined); +const mockSearchByName = vi.fn().mockResolvedValue([foundTask]); + +vi.mock("@/services/defaultServices", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + defaultTaskService: createMockTaskService({ + getById: (...args: unknown[]) => mockGetById(...args), + complete: (...args: unknown[]) => mockComplete(...args), + noncomplete: (...args: unknown[]) => mockNoncomplete(...args), + update: (...args: unknown[]) => mockUpdate(...args), + moveToBox: (...args: unknown[]) => mockMoveToBox(...args), + searchByName: (...args: unknown[]) => mockSearchByName(...args), + }), + defaultGoalService: createMockGoalService(), + defaultIdeaService: createMockIdeaService(), + }; +}); + +import SearchPage from "./SearchPage"; + +function renderSearchPage() { + return render( + + + , + ); +} + +async function typeSearchQueryAndGetTaskButtons(query: string) { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: query } }); + const completeButton = await screen.findByTestId(`complete-${foundTask.id}`); + await vi.waitFor(() => { + expect(mockSearchByName).toHaveBeenCalledWith(query); + }); + return { + completeButton, + updateButton: screen.getByTestId(`update-${foundTask.id}`), + moveButton: screen.getByTestId(`move-${foundTask.id}`), + }; +} + +describe("SearchPage > schedulePush on task mutations", () => { + beforeEach(() => { + mockSchedulePush.mockClear(); + mockGetById.mockClear(); + mockComplete.mockClear(); + mockNoncomplete.mockClear(); + mockUpdate.mockClear(); + mockMoveToBox.mockClear(); + }); + + it("should call schedulePush exactly once after completing an incomplete task", async () => { + mockGetById.mockResolvedValue( + buildTask({ ...foundTask, is_completed: false }), + ); + renderSearchPage(); + + const { completeButton } = await typeSearchQueryAndGetTaskButtons("Search"); + fireEvent.click(completeButton); + + await vi.waitFor(() => { + expect(mockComplete).toHaveBeenCalledWith( + foundTask.id, + expect.any(String), + ); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + + it("should call schedulePush exactly once after uncompleting an already-completed task", async () => { + mockGetById.mockResolvedValue( + buildTask({ ...foundTask, is_completed: true }), + ); + renderSearchPage(); + + const { completeButton } = await typeSearchQueryAndGetTaskButtons("Search"); + fireEvent.click(completeButton); + + await vi.waitFor(() => { + expect(mockNoncomplete).toHaveBeenCalledWith(foundTask.id); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + + it("should call schedulePush exactly once after updating a task", async () => { + renderSearchPage(); + + const { updateButton } = await typeSearchQueryAndGetTaskButtons("Search"); + fireEvent.click(updateButton); + + await vi.waitFor(() => { + expect(mockUpdate).toHaveBeenCalledWith(foundTask.id, { + name: "Updated name", + }); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + + it("should call schedulePush exactly once after moving a task to a different box", async () => { + renderSearchPage(); + + const { moveButton } = await typeSearchQueryAndGetTaskButtons("Search"); + fireEvent.click(moveButton); + + await vi.waitFor(() => { + expect(mockMoveToBox).toHaveBeenCalledWith(foundTask.id, "next"); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/client/src/pages/SearchPage.schedulePush.taskDelete.test.tsx b/packages/client/src/pages/SearchPage.schedulePush.taskDelete.test.tsx new file mode 100644 index 00000000..268305d2 --- /dev/null +++ b/packages/client/src/pages/SearchPage.schedulePush.taskDelete.test.tsx @@ -0,0 +1,171 @@ +/** + * RED tests: SearchPage task mutation paths (softDelete, duplicate) must + * schedule a push after mutating. + * implements FR1 of fix-search-page-sync-push (task 2.1) + */ +import { fireEvent, render, screen } from "@testing-library/react"; +import type React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { mockSchedulePush } from "@/app/providers/__mocks__/SyncProvider"; +import { buildTask } from "@/test/factories/taskFactory"; +import type { Task } from "@/types/entities"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock( + "@/app/providers/SyncProvider", + async () => import("@/app/providers/__mocks__/SyncProvider"), +); + +vi.mock("@/hooks/useGoals", () => ({ + useGoals: () => ({ goals: [], isLoading: false }), +})); +vi.mock("@/hooks/useContexts", () => ({ + useContexts: () => ({ contexts: [], isLoading: false }), +})); +vi.mock("@/hooks/useCategories", () => ({ + useCategories: () => ({ categories: [], isLoading: false }), +})); +vi.mock("@/hooks/useFocusMode", () => ({ + useFocusMode: () => ({ isFocusMode: false, focusOpacity: 1 }), +})); + +vi.mock("@/components/layout/SidebarShell", () => ({ + SidebarShell: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock("@/components/tasks/TaskList", () => ({ + TaskList: ({ + tasks, + onDelete, + onSelect, + }: { + tasks: Task[]; + onDelete: (id: string) => void; + onSelect: (id: string) => void; + }) => ( +
+ {tasks.map((task) => ( +
+ {task.name} +
+ ))} +
+ ), +})); + +vi.mock("@/components/tasks/TaskDetailPanel", () => ({ + TaskDetailPanel: ({ + task, + onDuplicate, + }: { + task: Task; + onDuplicate: (id: string) => void; + }) => ( +
+
+ ), +})); + +const foundTask = buildTask({ name: "Search Result Task" }); + +const mockGetById = vi.fn().mockResolvedValue(foundTask); +const mockSoftDelete = vi.fn().mockResolvedValue(undefined); +const mockDuplicate = vi.fn().mockResolvedValue(undefined); +const mockSearchByName = vi.fn().mockResolvedValue([foundTask]); + +vi.mock("@/services/defaultServices", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + defaultTaskService: { + getById: (...args: unknown[]) => mockGetById(...args), + complete: vi.fn(), + noncomplete: vi.fn(), + softDelete: (...args: unknown[]) => mockSoftDelete(...args), + duplicate: (...args: unknown[]) => mockDuplicate(...args), + searchByName: (...args: unknown[]) => mockSearchByName(...args), + }, + defaultGoalService: { + searchByName: vi.fn().mockResolvedValue([]), + }, + defaultIdeaService: { + searchByName: vi.fn().mockResolvedValue([]), + }, + }; +}); + +import SearchPage from "./SearchPage"; + +function renderSearchPage() { + return render( + + + , + ); +} + +async function typeSearchQuery(query: string) { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: query } }); + const deleteButton = await screen.findByTestId(`delete-${foundTask.id}`); + await vi.waitFor(() => { + expect(mockSearchByName).toHaveBeenCalledWith(query); + }); + return deleteButton; +} + +describe("SearchPage > schedulePush on task delete/duplicate", () => { + beforeEach(() => { + mockSchedulePush.mockClear(); + mockSoftDelete.mockClear(); + mockDuplicate.mockClear(); + }); + + it("should call schedulePush exactly once after soft-deleting a task", async () => { + renderSearchPage(); + + const deleteButton = await typeSearchQuery("Search"); + fireEvent.click(deleteButton); + + await vi.waitFor(() => { + expect(mockSoftDelete).toHaveBeenCalledWith(foundTask.id); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + + it("should call schedulePush exactly once after duplicating a task", async () => { + renderSearchPage(); + + await typeSearchQuery("Search"); + // Selecting the task opens the TaskDetailPanel, which exposes duplicate. + fireEvent.click(screen.getByTestId(`select-${foundTask.id}`)); + const duplicateButton = await screen.findByTestId( + `duplicate-${foundTask.id}`, + ); + fireEvent.click(duplicateButton); + + await vi.waitFor(() => { + expect(mockDuplicate).toHaveBeenCalledWith(foundTask.id); + }); + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/client/src/pages/SearchPage.tsx b/packages/client/src/pages/SearchPage.tsx index c581f7da..5732b46c 100644 --- a/packages/client/src/pages/SearchPage.tsx +++ b/packages/client/src/pages/SearchPage.tsx @@ -7,6 +7,7 @@ import type React from "react"; import { useCallback, useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { useNavigate } from "react-router-dom"; +import { useSync } from "@/app/providers/SyncProvider"; import { GoalItem } from "@/components/goals/GoalItem"; import { IdeaDetailPanel } from "@/components/ideas/IdeaDetailPanel"; import { IdeaItem } from "@/components/ideas/IdeaItem"; @@ -20,6 +21,7 @@ import { useFocusMode } from "@/hooks/useFocusMode"; import { useGoals } from "@/hooks/useGoals"; import { useSearch } from "@/hooks/useSearch"; import { getCachedDayBoundary } from "@/hooks/useSettings"; +import { useTaskCompletionAlerts } from "@/hooks/useTaskCompletionAlerts"; import { systemClock } from "@/lib/temporal"; import { defaultIdeaService, @@ -42,7 +44,10 @@ export default function SearchPage() { const { contexts } = useContexts(); const { categories } = useCategories(); const { isFocusMode, focusOpacity } = useFocusMode(); + const { raiseCompletionAlerts } = useTaskCompletionAlerts(); const navigate = useNavigate(); + // implements FR1, FR2, FR3 of fix-search-page-sync-push + const { schedulePush } = useSync(); const debounceRef = useRef | null>(null); @@ -76,27 +81,35 @@ export default function SearchPage() { await defaultTaskService.noncomplete(id); } else { const logicalDate = getLogicalDate(systemClock, getCachedDayBoundary()); - await defaultTaskService.complete(id, logicalDate); + const { recurringResult } = await defaultTaskService.complete( + id, + logicalDate, + ); + // implements FR5, FR6, U3 of fix-recurring-completion-error-masking + raiseCompletionAlerts(recurringResult, task.name); } + schedulePush(); if (searchQuery) void search(searchQuery); }, - [searchQuery, search], + [searchQuery, search, raiseCompletionAlerts, schedulePush], ); const handleUpdateTask = useCallback( async (id: string, changes: Partial) => { await defaultTaskService.update(id, changes); + schedulePush(); if (searchQuery) void search(searchQuery); }, - [searchQuery, search], + [searchQuery, search, schedulePush], ); const handleMoveTask = useCallback( async (id: string, box: Box) => { await defaultTaskService.moveToBox(id, box); + schedulePush(); if (searchQuery) void search(searchQuery); }, - [searchQuery, search], + [searchQuery, search, schedulePush], ); const handleNavigateToGoal = useCallback( @@ -113,18 +126,20 @@ export default function SearchPage() { const handleIdeaUpdate = useCallback( async (id: string, changes: Partial) => { await defaultIdeaService.update(id, changes); + schedulePush(); if (searchQuery) void search(searchQuery); }, - [searchQuery, search], + [searchQuery, search, schedulePush], ); const handleIdeaDelete = useCallback( async (id: string) => { setSelectedIdeaId(null); await defaultIdeaService.softDelete(id); + schedulePush(); if (searchQuery) void search(searchQuery); }, - [searchQuery, search], + [searchQuery, search, schedulePush], ); const handleIdeaClose = useCallback(() => { @@ -143,17 +158,19 @@ export default function SearchPage() { async (id: string) => { setSelectedTaskId(null); await defaultTaskService.softDelete(id); + schedulePush(); if (searchQuery) void search(searchQuery); }, - [searchQuery, search], + [searchQuery, search, schedulePush], ); const handleTaskDuplicate = useCallback( async (id: string) => { await defaultTaskService.duplicate(id); + schedulePush(); if (searchQuery) void search(searchQuery); }, - [searchQuery, search], + [searchQuery, search, schedulePush], ); const hasResults = tasks.length > 0 || goals.length > 0 || ideas.length > 0; diff --git a/packages/client/src/services/TaskService.recurring-completion.test.ts b/packages/client/src/services/TaskService.recurring-completion.test.ts index 395d12a0..aae2b151 100644 --- a/packages/client/src/services/TaskService.recurring-completion.test.ts +++ b/packages/client/src/services/TaskService.recurring-completion.test.ts @@ -201,6 +201,7 @@ describe("TaskService - Recurring Tasks Integration", () => { ).toBe("week"); }); + // implements FR2 of fix-recurring-completion-error-masking it("should not fail completion if recurring copy creation fails", async () => { const existingTask = buildTask({ id: "task-1", @@ -227,7 +228,10 @@ describe("TaskService - Recurring Tasks Integration", () => { // Completion should succeed even if clone creation fails expect(result.completed).toBeDefined(); expect(result.completed.is_completed).toBe(true); - expect(result.recurringResult.status).toBe("skipped_invalid_rule"); + // A copy-creation exception is a distinct status from a genuinely + // unparseable rule (FR1, FR2) — it must not be mislabeled as + // skipped_invalid_rule, which would falsely blame the repeat rule. + expect(result.recurringResult.status).toBe("error_creating_copy"); expect(consoleErrorSpy).toHaveBeenCalledWith( "Failed to create recurring task:", cloneCreationError, diff --git a/packages/client/src/services/TaskService.ts b/packages/client/src/services/TaskService.ts index 3716116d..1959ad15 100644 --- a/packages/client/src/services/TaskService.ts +++ b/packages/client/src/services/TaskService.ts @@ -22,10 +22,14 @@ import { parseRepeatRule, } from "@/utils/repeatRule"; -/** Implements FR2 of detect-invalid-repeat-rule */ +/** + * Implements FR2 of detect-invalid-repeat-rule. + * implements FR1, FR2, FR4 of fix-recurring-completion-error-masking + */ export type RecurringResult = | { status: "created"; task: Task } | { status: "skipped_invalid_rule" } + | { status: "error_creating_copy" } | { status: "not_recurring" }; export class TaskService { @@ -211,10 +215,14 @@ export class TaskService { }; } catch (error) { console.error("Failed to create recurring task:", error); - // Do not interrupt task completion if clone creation failed + // Do not interrupt task completion if clone creation failed. + // implements FR1, FR2 of fix-recurring-completion-error-masking: + // this exception happened after the rule parsed successfully, so it + // must not be mislabeled as skipped_invalid_rule (reserved for a + // genuinely unparseable rule, handled by the `if (!rule)` return above). return { completed: finalCompletedTask, - recurringResult: { status: "skipped_invalid_rule" }, + recurringResult: { status: "error_creating_copy" }, }; } } diff --git a/packages/client/src/test/features/day_boundary/steps/day_boundary_validation.steps.ts b/packages/client/src/test/features/day_boundary/steps/day_boundary_validation.steps.ts index 05e1696a..b102443a 100644 --- a/packages/client/src/test/features/day_boundary/steps/day_boundary_validation.steps.ts +++ b/packages/client/src/test/features/day_boundary/steps/day_boundary_validation.steps.ts @@ -16,7 +16,6 @@ function createMockRepository(): SettingsRepository { getByKey: vi.fn(), getValue: vi.fn(), set: vi.fn(), - getChangedSince: vi.fn(), getNeedingSync: vi.fn(), clearNeedsSyncByKey: vi.fn(), bulkUpsert: vi.fn(), diff --git a/packages/client/src/test/features/repeating_tasks/complete_copy_creation_error.feature b/packages/client/src/test/features/repeating_tasks/complete_copy_creation_error.feature new file mode 100644 index 00000000..cbe2430b --- /dev/null +++ b/packages/client/src/test/features/repeating_tasks/complete_copy_creation_error.feature @@ -0,0 +1,16 @@ +Feature: Completion does not mislabel copy-creation errors as invalid rules + Implements FR1, FR2, FR3 of fix-recurring-completion-error-masking. + + @fix-recurring-completion-error-masking @FR1 @FR2 + Scenario: Copy creation fails for a task with a valid repeat rule + Given a recurring task with a valid repeat rule + When the task is completed and an unexpected error occurs while creating the next occurrence + Then the task is marked as completed + And the recurring result status is "error_creating_copy" + And the error is logged to the console + + @fix-recurring-completion-error-masking @FR3 + Scenario: No false invalid-rule alert is raised when copy creation fails + Given a recurring task with a valid repeat rule + When the task is completed and an unexpected error occurs while creating the next occurrence + Then the user is not shown a repeat rule invalid alert diff --git a/packages/client/src/test/features/repeating_tasks/complete_invalid_rule_regression.feature b/packages/client/src/test/features/repeating_tasks/complete_invalid_rule_regression.feature new file mode 100644 index 00000000..66e943f7 --- /dev/null +++ b/packages/client/src/test/features/repeating_tasks/complete_invalid_rule_regression.feature @@ -0,0 +1,14 @@ +Feature: Genuinely invalid repeat rules still surface the truthful alert + Implements FR1, FR3 of fix-recurring-completion-error-masking. + + @fix-recurring-completion-error-masking @FR1 + Scenario: Completing a task with a genuinely unparseable repeat rule + Given a recurring task with an unparseable repeat rule + When the task is completed + Then the recurring result status is "skipped_invalid_rule" + + @fix-recurring-completion-error-masking @FR3 + Scenario: The invalid-rule alert still appears for a genuinely unparseable rule + Given a recurring task with an unparseable repeat rule + When the task is completed + Then the user is shown a repeat rule invalid alert for the task diff --git a/packages/client/src/test/features/repeating_tasks/steps/complete_copy_creation_error.steps.ts b/packages/client/src/test/features/repeating_tasks/steps/complete_copy_creation_error.steps.ts new file mode 100644 index 00000000..a5ff2364 --- /dev/null +++ b/packages/client/src/test/features/repeating_tasks/steps/complete_copy_creation_error.steps.ts @@ -0,0 +1,150 @@ +// implements FR1, FR2, FR3 of fix-recurring-completion-error-masking +import type { FeatureDescriibeCallbackParams } from "@amiceli/vitest-cucumber"; +import { describeFeature, loadFeature } from "@amiceli/vitest-cucumber"; +import { renderHook } from "@testing-library/react"; +import { expect, type TestContext, vi } from "vitest"; +import { mockAddAlerts } from "@/app/providers/__mocks__/AlertProvider"; +import type { ChecklistRepository } from "@/db/repositories/ChecklistRepository"; +import type { TaskRepository } from "@/db/repositories/TaskRepository"; +import { useTaskCompletionAlerts } from "@/hooks/useTaskCompletionAlerts"; +import type { RecurringResult } from "@/services/TaskService"; +import { TaskService } from "@/services/TaskService"; +import { createMockChecklistRepository } from "@/test/factories/checklistRepositoryFactory"; +import { buildTask } from "@/test/factories/taskFactory"; +import { createMockTaskRepository } from "@/test/mocks/taskRepositoryMock"; +import { toISODate } from "@/utils/dateHelpers"; + +vi.mock( + "@/app/providers/AlertProvider", + async () => import("@/app/providers/__mocks__/AlertProvider"), +); + +const feature = await loadFeature("../complete_copy_creation_error.feature"); + +const VALID_DAILY_RULE = { + type: "fixed" as const, + frequency: "daily" as const, + interval: 1, + target_box: "today" as const, + advance_days: 0, +}; + +type Context = Record; + +describeFeature(feature, (f: FeatureDescriibeCallbackParams) => { + let taskService: TaskService; + let mockTaskRepository: TaskRepository; + let mockChecklistRepository: ChecklistRepository; + let taskName: string; + let recurringResult: RecurringResult; + let consoleErrorSpy: ReturnType; + + f.BeforeEachScenario(() => { + mockTaskRepository = createMockTaskRepository(); + mockChecklistRepository = createMockChecklistRepository(); + taskService = new TaskService(mockTaskRepository, mockChecklistRepository); + mockAddAlerts.mockClear(); + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + f.AfterEachScenario(() => { + consoleErrorSpy.mockRestore(); + }); + + const givenValidRuleTaskWithFailingCopyCreation = (_ctx: TestContext) => { + taskName = "Daily review"; + const existingTask = buildTask({ + id: "task-1", + name: taskName, + repeat_rule: JSON.stringify(VALID_DAILY_RULE), + is_hidden: false, + next_date: toISODate("2026-07-19"), + appear_date: toISODate("2026-07-19"), + }); + const completedTask = buildTask({ ...existingTask, is_completed: true }); + + mockTaskRepository.getById = vi.fn().mockResolvedValue(existingTask); + mockTaskRepository.update = vi.fn().mockResolvedValue(completedTask); + mockTaskRepository.create = vi + .fn() + .mockRejectedValue(new Error("DB write failed")); + mockChecklistRepository.getActiveByTaskId = vi.fn().mockResolvedValue([]); + }; + + const whenCompletedWithCopyCreationError = async (_ctx: TestContext) => { + const result = await taskService.complete("task-1"); + recurringResult = result.recurringResult; + return result; + }; + + const whenCompletedWithCopyCreationErrorVoid = async ( + ctx: TestContext, + ): Promise => { + await whenCompletedWithCopyCreationError(ctx); + }; + + // @fix-recurring-completion-error-masking @FR1 @FR2 + f.Scenario( + "Copy creation fails for a task with a valid repeat rule", + ({ Given, When, Then, And }) => { + let completedIsCompleted: boolean; + + Given( + "a recurring task with a valid repeat rule", + givenValidRuleTaskWithFailingCopyCreation, + ); + + When( + "the task is completed and an unexpected error occurs while creating the next occurrence", + async (ctx: TestContext) => { + const result = await whenCompletedWithCopyCreationError(ctx); + completedIsCompleted = result.completed.is_completed; + }, + ); + + Then("the task is marked as completed", (_ctx: TestContext) => { + expect(completedIsCompleted).toBe(true); + }); + + And( + 'the recurring result status is "error_creating_copy"', + (_ctx: TestContext) => { + expect(recurringResult.status).toBe("error_creating_copy"); + }, + ); + + And("the error is logged to the console", (_ctx: TestContext) => { + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Failed to create recurring task:", + expect.any(Error), + ); + }); + }, + ); + + // @fix-recurring-completion-error-masking @FR3 + f.Scenario( + "No false invalid-rule alert is raised when copy creation fails", + ({ Given, When, Then }) => { + Given( + "a recurring task with a valid repeat rule", + givenValidRuleTaskWithFailingCopyCreation, + ); + + When( + "the task is completed and an unexpected error occurs while creating the next occurrence", + whenCompletedWithCopyCreationErrorVoid, + ); + + Then( + "the user is not shown a repeat rule invalid alert", + (_ctx: TestContext) => { + const { result } = renderHook(() => useTaskCompletionAlerts()); + result.current.raiseCompletionAlerts(recurringResult, taskName); + + expect(mockAddAlerts).not.toHaveBeenCalled(); + }, + ); + }, + ); +}); diff --git a/packages/client/src/test/features/repeating_tasks/steps/complete_invalid_rule_regression.steps.ts b/packages/client/src/test/features/repeating_tasks/steps/complete_invalid_rule_regression.steps.ts new file mode 100644 index 00000000..e89a9d30 --- /dev/null +++ b/packages/client/src/test/features/repeating_tasks/steps/complete_invalid_rule_regression.steps.ts @@ -0,0 +1,108 @@ +// implements FR1, FR3 of fix-recurring-completion-error-masking +import type { FeatureDescriibeCallbackParams } from "@amiceli/vitest-cucumber"; +import { describeFeature, loadFeature } from "@amiceli/vitest-cucumber"; +import { renderHook } from "@testing-library/react"; +import { expect, type TestContext, vi } from "vitest"; +import { mockAddAlerts } from "@/app/providers/__mocks__/AlertProvider"; +import type { ChecklistRepository } from "@/db/repositories/ChecklistRepository"; +import type { TaskRepository } from "@/db/repositories/TaskRepository"; +import { useTaskCompletionAlerts } from "@/hooks/useTaskCompletionAlerts"; +import type { RecurringResult } from "@/services/TaskService"; +import { TaskService } from "@/services/TaskService"; +import { createMockChecklistRepository } from "@/test/factories/checklistRepositoryFactory"; +import { buildTask } from "@/test/factories/taskFactory"; +import { createMockTaskRepository } from "@/test/mocks/taskRepositoryMock"; + +vi.mock( + "@/app/providers/AlertProvider", + async () => import("@/app/providers/__mocks__/AlertProvider"), +); + +const feature = await loadFeature( + "../complete_invalid_rule_regression.feature", +); + +const UNPARSEABLE_RULE = "{not valid json}"; + +type Context = Record; + +describeFeature(feature, (f: FeatureDescriibeCallbackParams) => { + let taskService: TaskService; + let mockTaskRepository: TaskRepository; + let mockChecklistRepository: ChecklistRepository; + let taskName: string; + let recurringResult: RecurringResult; + + f.BeforeEachScenario(() => { + mockTaskRepository = createMockTaskRepository(); + mockChecklistRepository = createMockChecklistRepository(); + taskService = new TaskService(mockTaskRepository, mockChecklistRepository); + mockAddAlerts.mockClear(); + }); + + const givenUnparseableRuleTask = (_ctx: TestContext) => { + taskName = "Buy groceries"; + const existingTask = buildTask({ + id: "task-1", + name: taskName, + repeat_rule: UNPARSEABLE_RULE, + is_hidden: false, + next_date: "", + appear_date: "", + }); + const completedTask = buildTask({ ...existingTask, is_completed: true }); + + mockTaskRepository.getById = vi.fn().mockResolvedValue(existingTask); + mockTaskRepository.update = vi.fn().mockResolvedValue(completedTask); + }; + + const whenTaskIsCompleted = async (_ctx: TestContext) => { + const result = await taskService.complete("task-1"); + recurringResult = result.recurringResult; + }; + + // @fix-recurring-completion-error-masking @FR1 + f.Scenario( + "Completing a task with a genuinely unparseable repeat rule", + ({ Given, When, Then }) => { + Given( + "a recurring task with an unparseable repeat rule", + givenUnparseableRuleTask, + ); + + When("the task is completed", whenTaskIsCompleted); + + Then( + 'the recurring result status is "skipped_invalid_rule"', + (_ctx: TestContext) => { + expect(recurringResult.status).toBe("skipped_invalid_rule"); + }, + ); + }, + ); + + // @fix-recurring-completion-error-masking @FR3 + f.Scenario( + "The invalid-rule alert still appears for a genuinely unparseable rule", + ({ Given, When, Then }) => { + Given( + "a recurring task with an unparseable repeat rule", + givenUnparseableRuleTask, + ); + + When("the task is completed", whenTaskIsCompleted); + + Then( + "the user is shown a repeat rule invalid alert for the task", + (_ctx: TestContext) => { + const { result } = renderHook(() => useTaskCompletionAlerts()); + result.current.raiseCompletionAlerts(recurringResult, taskName); + + expect(mockAddAlerts).toHaveBeenCalledWith([ + { type: "repeat_rule_invalid", taskNames: [taskName] }, + ]); + }, + ); + }, + ); +}); diff --git a/packages/client/src/test/features/search/search_mutations_schedule_push.feature b/packages/client/src/test/features/search/search_mutations_schedule_push.feature new file mode 100644 index 00000000..e1ee166e --- /dev/null +++ b/packages/client/src/test/features/search/search_mutations_schedule_push.feature @@ -0,0 +1,64 @@ +Feature: Search page mutations schedule a sync push + Implements FR1, FR2 of fix-search-page-sync-push. + + @fix-search-page-sync-push @FR1 + Scenario: Completing a task from search results schedules a push + Given a task "Search Result Task" is found by search + When the user completes the task from search results + Then the task completion is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR1 + Scenario: Uncompleting a task from search results schedules a push + Given a completed task "Search Result Task" is found by search + When the user uncompletes the task from search results + Then the task uncompletion is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR1 + Scenario: Editing a task from search results schedules a push + Given a task "Search Result Task" is found by search + When the user edits the task from search results + Then the task edit is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR1 + Scenario: Moving a task from search results schedules a push + Given a task "Search Result Task" is found by search + When the user moves the task to a different box from search results + Then the task move is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR1 + Scenario: Deleting a task from search results schedules a push + Given a task "Search Result Task" is found by search + When the user deletes the task from search results + Then the task deletion is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR1 + Scenario: Duplicating a task from the task detail panel opened from search schedules a push + Given a task "Search Result Task" is found by search + When the user duplicates the task from the task detail panel opened from search + Then the task duplication is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR2 + Scenario: Editing an idea from the idea detail panel opened from search schedules a push + Given an idea "Search Result Idea" is found by search + When the user edits the idea from the idea detail panel opened from search + Then the idea edit is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR2 + Scenario: Deleting an idea from the idea detail panel opened from search schedules a push + Given an idea "Search Result Idea" is found by search + When the user deletes the idea from the idea detail panel opened from search + Then the idea deletion is written locally + And schedulePush is called exactly once + + @fix-search-page-sync-push @FR1 @FR2 + Scenario: Running a search without any mutation does not schedule a push + Given a task "Search Result Task" is found by search + When the user types a search query and search executes + Then schedulePush is not called diff --git a/packages/client/src/test/features/search/steps/search_mutations_schedule_push.steps.tsx b/packages/client/src/test/features/search/steps/search_mutations_schedule_push.steps.tsx new file mode 100644 index 00000000..5d3a2f7e --- /dev/null +++ b/packages/client/src/test/features/search/steps/search_mutations_schedule_push.steps.tsx @@ -0,0 +1,529 @@ +// implements FR1, FR2 of fix-search-page-sync-push +import type { FeatureDescriibeCallbackParams } from "@amiceli/vitest-cucumber"; +import { describeFeature, loadFeature } from "@amiceli/vitest-cucumber"; +import { + cleanup, + fireEvent, + render, + screen, +} from "@testing-library/react/pure"; +import type React from "react"; +import { MemoryRouter } from "react-router-dom"; +import { expect, type TestContext, vi } from "vitest"; +import { mockSchedulePush } from "@/app/providers/__mocks__/SyncProvider"; +import { buildIdea } from "@/test/factories/ideaFactory"; +import { buildTask } from "@/test/factories/taskFactory"; +import { createMockGoalService } from "@/test/mocks/goalServiceMock"; +import { createMockIdeaService } from "@/test/mocks/ideaServiceMock"; +import { createMockTaskService } from "@/test/mocks/taskServiceMock"; +import type { Box } from "@/types/common"; +import type { Idea, Task } from "@/types/entities"; + +const feature = await loadFeature("../search_mutations_schedule_push.feature"); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock( + "@/app/providers/SyncProvider", + async () => import("@/app/providers/__mocks__/SyncProvider"), +); + +vi.mock("@/hooks/useGoals", () => ({ + useGoals: () => ({ goals: [], isLoading: false }), +})); +vi.mock("@/hooks/useContexts", () => ({ + useContexts: () => ({ contexts: [], isLoading: false }), +})); +vi.mock("@/hooks/useCategories", () => ({ + useCategories: () => ({ categories: [], isLoading: false }), +})); +vi.mock("@/hooks/useFocusMode", () => ({ + useFocusMode: () => ({ isFocusMode: false, focusOpacity: 1 }), +})); + +vi.mock("@/components/layout/SidebarShell", () => ({ + SidebarShell: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock("@/components/tasks/TaskList", () => ({ + TaskList: ({ + tasks, + onComplete, + onUpdate, + onMove, + onDelete, + onSelect, + }: { + tasks: Task[]; + onComplete: (id: string) => void; + onUpdate: (id: string, changes: Partial) => void; + onMove: (id: string, box: Box) => void; + onDelete: (id: string) => void; + onSelect: (id: string) => void; + }) => ( +
+ {tasks.map((task) => ( +
+ {task.name} +
+ ))} +
+ ), +})); + +vi.mock("@/components/tasks/TaskDetailPanel", () => ({ + TaskDetailPanel: ({ + task, + onDuplicate, + }: { + task: Task; + onDuplicate: (id: string) => void; + }) => ( +
+
+ ), +})); + +vi.mock("@/components/ideas/IdeaItem", () => ({ + IdeaItem: ({ idea }: { idea: Idea }) => ( + + ), +})); + +vi.mock("@/components/ideas/IdeaDetailPanel", () => ({ + IdeaDetailPanel: ({ + idea, + onUpdate, + onDelete, + }: { + idea: Idea; + onUpdate: (id: string, changes: Partial) => void; + onDelete: (id: string) => void; + }) => ( +
+
+ ), +})); + +const foundTask = buildTask({ name: "Search Result Task" }); +const foundIdea = buildIdea({ name: "Search Result Idea" }); + +const mockGetById = vi.fn().mockResolvedValue(foundTask); +const mockComplete = vi.fn().mockResolvedValue({ + completed: foundTask, + recurringResult: { status: "not_recurring" }, +}); +const mockNoncomplete = vi.fn().mockResolvedValue(undefined); +const mockUpdate = vi.fn().mockResolvedValue(undefined); +const mockMoveToBox = vi.fn().mockResolvedValue(undefined); +const mockSoftDelete = vi.fn().mockResolvedValue(undefined); +const mockDuplicate = vi.fn().mockResolvedValue(undefined); +const mockTaskSearchByName = vi.fn().mockResolvedValue([foundTask]); + +const mockIdeaUpdate = vi.fn().mockResolvedValue(undefined); +const mockIdeaSoftDelete = vi.fn().mockResolvedValue(undefined); +const mockIdeaSearchByName = vi.fn().mockResolvedValue([foundIdea]); + +vi.mock("@/services/defaultServices", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + defaultTaskService: createMockTaskService({ + getById: (...args: unknown[]) => mockGetById(...args), + complete: (...args: unknown[]) => mockComplete(...args), + noncomplete: (...args: unknown[]) => mockNoncomplete(...args), + update: (...args: unknown[]) => mockUpdate(...args), + moveToBox: (...args: unknown[]) => mockMoveToBox(...args), + softDelete: (...args: unknown[]) => mockSoftDelete(...args), + duplicate: (...args: unknown[]) => mockDuplicate(...args), + searchByName: (...args: unknown[]) => mockTaskSearchByName(...args), + }), + defaultGoalService: createMockGoalService({ + searchByName: vi.fn().mockResolvedValue([]), + }), + defaultIdeaService: createMockIdeaService({ + update: (...args: unknown[]) => mockIdeaUpdate(...args), + softDelete: (...args: unknown[]) => mockIdeaSoftDelete(...args), + searchByName: (...args: unknown[]) => mockIdeaSearchByName(...args), + }), + }; +}); + +const { default: SearchPage } = await import("@/pages/SearchPage"); + +function renderSearchPage() { + return render( + + + , + ); +} + +async function typeSearchQueryAndFindTaskItem(query: string) { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: query } }); + const taskItem = await screen.findByTestId(`complete-${foundTask.id}`); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith(query); + }); + return taskItem; +} + +async function typeSearchQueryAndOpenIdeaPanel(query: string) { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: query } }); + const ideaItem = await screen.findByTestId(`idea-item-${foundIdea.id}`); + await vi.waitFor(() => { + expect(mockIdeaSearchByName).toHaveBeenCalledWith(query); + }); + fireEvent.click(ideaItem); + return screen.findByTestId(`update-idea-${foundIdea.id}`); +} + +type FeatureContext = Record; + +describeFeature( + feature, + (f: FeatureDescriibeCallbackParams) => { + f.BeforeEachScenario(() => { + cleanup(); + mockSchedulePush.mockClear(); + mockGetById.mockClear().mockResolvedValue(foundTask); + mockComplete.mockClear(); + mockNoncomplete.mockClear(); + mockUpdate.mockClear(); + mockMoveToBox.mockClear(); + mockSoftDelete.mockClear(); + mockDuplicate.mockClear(); + mockTaskSearchByName.mockClear().mockResolvedValue([foundTask]); + mockIdeaUpdate.mockClear(); + mockIdeaSoftDelete.mockClear(); + mockIdeaSearchByName.mockClear().mockResolvedValue([foundIdea]); + }); + + // @fix-search-page-sync-push @FR1 + f.Scenario( + "Completing a task from search results schedules a push", + ({ Given, When, Then, And }) => { + Given('a task "Search Result Task" is found by search', async () => { + mockGetById.mockResolvedValue( + buildTask({ ...foundTask, is_completed: false }), + ); + renderSearchPage(); + }); + + When("the user completes the task from search results", async () => { + const completeButton = await typeSearchQueryAndFindTaskItem("Search"); + fireEvent.click(completeButton); + }); + + Then("the task completion is written locally", async () => { + await vi.waitFor(() => { + expect(mockComplete).toHaveBeenCalledWith( + foundTask.id, + expect.any(String), + ); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR1 + f.Scenario( + "Uncompleting a task from search results schedules a push", + ({ Given, When, Then, And }) => { + Given( + 'a completed task "Search Result Task" is found by search', + async () => { + mockGetById.mockResolvedValue( + buildTask({ ...foundTask, is_completed: true }), + ); + renderSearchPage(); + }, + ); + + When("the user uncompletes the task from search results", async () => { + const completeButton = await typeSearchQueryAndFindTaskItem("Search"); + fireEvent.click(completeButton); + }); + + Then("the task uncompletion is written locally", async () => { + await vi.waitFor(() => { + expect(mockNoncomplete).toHaveBeenCalledWith(foundTask.id); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR1 + f.Scenario( + "Editing a task from search results schedules a push", + ({ Given, When, Then, And }) => { + Given('a task "Search Result Task" is found by search', async () => { + renderSearchPage(); + }); + + When("the user edits the task from search results", async () => { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: "Search" } }); + const updateButton = await screen.findByTestId( + `update-${foundTask.id}`, + ); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith("Search"); + }); + fireEvent.click(updateButton); + }); + + Then("the task edit is written locally", async () => { + await vi.waitFor(() => { + expect(mockUpdate).toHaveBeenCalledWith(foundTask.id, { + name: "Updated name", + }); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR1 + f.Scenario( + "Moving a task from search results schedules a push", + ({ Given, When, Then, And }) => { + Given('a task "Search Result Task" is found by search', async () => { + renderSearchPage(); + }); + + When( + "the user moves the task to a different box from search results", + async () => { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: "Search" } }); + const moveButton = await screen.findByTestId( + `move-${foundTask.id}`, + ); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith("Search"); + }); + fireEvent.click(moveButton); + }, + ); + + Then("the task move is written locally", async () => { + await vi.waitFor(() => { + expect(mockMoveToBox).toHaveBeenCalledWith(foundTask.id, "next"); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR1 + f.Scenario( + "Deleting a task from search results schedules a push", + ({ Given, When, Then, And }) => { + Given('a task "Search Result Task" is found by search', async () => { + renderSearchPage(); + }); + + When("the user deletes the task from search results", async () => { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: "Search" } }); + const deleteButton = await screen.findByTestId( + `delete-${foundTask.id}`, + ); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith("Search"); + }); + fireEvent.click(deleteButton); + }); + + Then("the task deletion is written locally", async () => { + await vi.waitFor(() => { + expect(mockSoftDelete).toHaveBeenCalledWith(foundTask.id); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR1 + f.Scenario( + "Duplicating a task from the task detail panel opened from search schedules a push", + ({ Given, When, Then, And }) => { + Given('a task "Search Result Task" is found by search', async () => { + renderSearchPage(); + }); + + When( + "the user duplicates the task from the task detail panel opened from search", + async () => { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: "Search" } }); + const selectButton = await screen.findByTestId( + `select-${foundTask.id}`, + ); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith("Search"); + }); + fireEvent.click(selectButton); + const duplicateButton = await screen.findByTestId( + `duplicate-${foundTask.id}`, + ); + fireEvent.click(duplicateButton); + }, + ); + + Then("the task duplication is written locally", async () => { + await vi.waitFor(() => { + expect(mockDuplicate).toHaveBeenCalledWith(foundTask.id); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR2 + f.Scenario( + "Editing an idea from the idea detail panel opened from search schedules a push", + ({ Given, When, Then, And }) => { + Given('an idea "Search Result Idea" is found by search', async () => { + renderSearchPage(); + }); + + When( + "the user edits the idea from the idea detail panel opened from search", + async () => { + const updateButton = + await typeSearchQueryAndOpenIdeaPanel("Search"); + fireEvent.click(updateButton); + }, + ); + + Then("the idea edit is written locally", async () => { + await vi.waitFor(() => { + expect(mockIdeaUpdate).toHaveBeenCalledWith(foundIdea.id, { + name: "Updated idea name", + }); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR2 + f.Scenario( + "Deleting an idea from the idea detail panel opened from search schedules a push", + ({ Given, When, Then, And }) => { + Given('an idea "Search Result Idea" is found by search', async () => { + renderSearchPage(); + }); + + When( + "the user deletes the idea from the idea detail panel opened from search", + async () => { + await typeSearchQueryAndOpenIdeaPanel("Search"); + const deleteButton = await screen.findByTestId( + `delete-idea-${foundIdea.id}`, + ); + fireEvent.click(deleteButton); + }, + ); + + Then("the idea deletion is written locally", async () => { + await vi.waitFor(() => { + expect(mockIdeaSoftDelete).toHaveBeenCalledWith(foundIdea.id); + }); + }); + + And("schedulePush is called exactly once", async () => { + expect(mockSchedulePush).toHaveBeenCalledOnce(); + }); + }, + ); + + // @fix-search-page-sync-push @FR1 @FR2 + f.Scenario( + "Running a search without any mutation does not schedule a push", + ({ Given, When, Then }) => { + Given('a task "Search Result Task" is found by search', async () => { + renderSearchPage(); + }); + + When("the user types a search query and search executes", async () => { + const searchInput = screen.getByTestId("search-input"); + fireEvent.change(searchInput, { target: { value: "Search" } }); + await vi.waitFor(() => { + expect(mockTaskSearchByName).toHaveBeenCalledWith("Search"); + }); + }); + + Then("schedulePush is not called", async (_ctx: TestContext) => { + expect(mockSchedulePush).not.toHaveBeenCalled(); + }); + }, + ); + }, +); diff --git a/packages/client/src/test/features/settings/settings_repository_sync_flags.feature b/packages/client/src/test/features/settings/settings_repository_sync_flags.feature index dbfb94c8..9e27d1dd 100644 --- a/packages/client/src/test/features/settings/settings_repository_sync_flags.feature +++ b/packages/client/src/test/features/settings/settings_repository_sync_flags.feature @@ -13,10 +13,3 @@ Feature: Settings repository sync flags When clearNeedsSyncByKey is called with keys ["accent_color"] Then "accent_color" has syncStatus "synced" And "default_box" still has syncStatus "pending" - - @settings-specs-and-bdd @FR2 - Scenario: Filter settings by updated_at - Given setting A has updated_at "2025-01-01T00:00:00.000Z" - And setting B has updated_at "2025-01-02T00:00:00.000Z" - When getChangedSince is called with "2025-01-01T00:00:00.000Z" - Then only setting B is returned diff --git a/packages/client/src/test/features/settings/steps/settings_repository_sync_flags.steps.ts b/packages/client/src/test/features/settings/steps/settings_repository_sync_flags.steps.ts index 70e22df1..b7379ca5 100644 --- a/packages/client/src/test/features/settings/steps/settings_repository_sync_flags.steps.ts +++ b/packages/client/src/test/features/settings/steps/settings_repository_sync_flags.steps.ts @@ -90,51 +90,5 @@ describeFeature( }, ); }); - - // @settings-specs-and-bdd @FR2 - f.Scenario( - "Filter settings by updated_at", - ({ Given, And, When, Then }) => { - let result: Setting[]; - - Given( - 'setting A has updated_at "2025-01-01T00:00:00.000Z"', - async (_ctx: TestContext) => { - await db.settings.add( - buildSetting({ - key: "a", - updated_at: "2025-01-01T00:00:00.000Z", - }), - ); - }, - ); - - And( - 'setting B has updated_at "2025-01-02T00:00:00.000Z"', - async (_ctx: TestContext) => { - await db.settings.add( - buildSetting({ - key: "b", - updated_at: "2025-01-02T00:00:00.000Z", - }), - ); - }, - ); - - When( - 'getChangedSince is called with "2025-01-01T00:00:00.000Z"', - async (_ctx: TestContext) => { - result = await repository.getChangedSince( - "2025-01-01T00:00:00.000Z", - ); - }, - ); - - Then("only setting B is returned", async (_ctx: TestContext) => { - expect(result).toHaveLength(1); - expect(result[0].key).toBe("b"); - }); - }, - ); }, ); diff --git a/packages/client/src/test/features/settings/steps/settings_service.steps.ts b/packages/client/src/test/features/settings/steps/settings_service.steps.ts index 9ae0acc4..e469e49b 100644 --- a/packages/client/src/test/features/settings/steps/settings_service.steps.ts +++ b/packages/client/src/test/features/settings/steps/settings_service.steps.ts @@ -15,7 +15,6 @@ function createMockRepository(): SettingsRepository { getByKey: vi.fn(), getValue: vi.fn(), set: vi.fn(), - getChangedSince: vi.fn(), getNeedingSync: vi.fn(), clearNeedsSyncByKey: vi.fn(), bulkUpsert: vi.fn(), diff --git a/packages/client/src/test/mocks/attachmentRepositoryMock.ts b/packages/client/src/test/mocks/attachmentRepositoryMock.ts index 325ceaee..0d8627ac 100644 --- a/packages/client/src/test/mocks/attachmentRepositoryMock.ts +++ b/packages/client/src/test/mocks/attachmentRepositoryMock.ts @@ -16,7 +16,6 @@ export function createMockAttachmentRepository( save: vi.fn().mockResolvedValue(undefined), delete: vi.fn().mockResolvedValue(undefined), bulkUpsert: vi.fn().mockResolvedValue(undefined), - getChangedSince: vi.fn().mockResolvedValue([]), getNeedingSync: vi.fn().mockResolvedValue([]), softDeleteByEntityTypeAndId: vi.fn().mockResolvedValue(0), restoreByEntityTypeAndId: vi.fn().mockResolvedValue(0), diff --git a/packages/client/src/test/mocks/createRepositoryMock.ts b/packages/client/src/test/mocks/createRepositoryMock.ts index 0578d681..e8297bb0 100644 --- a/packages/client/src/test/mocks/createRepositoryMock.ts +++ b/packages/client/src/test/mocks/createRepositoryMock.ts @@ -10,7 +10,6 @@ export function createRepositoryMock( create: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue(undefined), bulkUpsert: vi.fn().mockResolvedValue(undefined), - getChangedSince: vi.fn().mockResolvedValue([]), getDirty: vi.fn().mockResolvedValue([]), applyServerRecords: vi.fn().mockResolvedValue(undefined), } as Partial>, diff --git a/packages/client/src/test/mocks/taskRepositoryMock.ts b/packages/client/src/test/mocks/taskRepositoryMock.ts index 40e6f0ec..5498b1ec 100644 --- a/packages/client/src/test/mocks/taskRepositoryMock.ts +++ b/packages/client/src/test/mocks/taskRepositoryMock.ts @@ -19,7 +19,6 @@ export function createMockTaskRepository( create: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue(undefined), bulkUpsert: vi.fn().mockResolvedValue(undefined), - getChangedSince: vi.fn().mockResolvedValue([]), getNeedingSync: vi.fn().mockResolvedValue([]), getTasksToReveal: vi.fn().mockResolvedValue([]), applyServerRecords: vi.fn().mockResolvedValue(undefined),