mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
Improve on sync (#7736)
* fix(android): restore share title derivation and dedupe shared tasks Commitd32f7037a3accidentally reverted the EXTRA_SUBJECT handling fromedb102534e, so the share handler stopped sending the page subject and defaulted the title to the literal "Shared Content". The frontend's subject -> title -> derived title chain then always fell through to that placeholder, producing blank-looking shared tasks. - Restore EXTRA_SUBJECT extraction; leave title/subject empty when absent so the frontend can derive a meaningful title from the URL or note. - Ignore empty/blank shared text instead of creating a useless task. - Skip handleIntent() on Activity recreation (config change) so the same share Intent isn't re-processed into a duplicate task. - Extract buildTaskTitle/readableUrl as pure functions with unit tests and guard the effect against empty payloads. * fix(snack): scale error/warning snack duration with message length Long error messages (e.g. multi-sentence sync errors) were auto-dismissed after a fixed 8s, too short to read. Error/warning snacks now stay visible proportional to message length (~90ms/char), clamped to 10-30s. * feat(tasks): skip undo snack when deleting a blank task A sub task or parent task with an empty title and no data (notes, time, estimate, attachments, issue link, reminder, repeat, scheduling, deadline, non-blank sub tasks) no longer shows the undo-delete snack. * fix(sync): include archive data in REPAIR operations validateAndRepairCurrentState built the REPAIR op from the synchronous getStateSnapshot(), which hardcodes empty archiveYoung/archiveOld (archives live in IndexedDB, not NgRx state). The resulting REPAIR op carried empty archives, so every other client that applied it overwrote its archive with nothing — wiping archived tasks on all devices except the one that ran the repair. - Use getStateSnapshotAsync() so the REPAIR op carries real archives. - Extend the empty-archive overwrite guard in ArchiveOperationHandler._handleLoadAllData() to also cover OpType.Repair (previously only SYNC_IMPORT/BACKUP_IMPORT), as defense in depth. * test(sync): add archive REPAIR round-trip integration test Wires the real StateSnapshotService, ArchiveDbAdapter, ArchiveStoreService and ArchiveOperationHandler against real IndexedDB to verify archive data survives the REPAIR-op round-trip: - getStateSnapshotAsync() loads IndexedDB archives; getStateSnapshot() does not - archive round-trips from client A's IndexedDB through a REPAIR op into a fresh client B's IndexedDB - a REPAIR op carrying empty archives no longer wipes a client that has archive data (empty-archive guard regression) * perf(sync): skip archive IndexedDB reads when post-sync state is valid validateAndRepairCurrentState validated the full async snapshot (two IndexedDB archive reads + structured-clone deserialization) on every Checkpoint D, even when state was valid and no repair was needed. It now validates the cheap synchronous snapshot first and only loads the async snapshot (with archives) when a repair is actually required — the rare path. The REPAIR op still carries archive data. Also addresses multi-review follow-ups: - archive-operation-handler: reword the empty-archive guard comment so it no longer over-promises reconciliation for REPAIR ops. - archive-repair-roundtrip test: add isPersistent to the applied-op meta to match the real applier; scope the file docstring accurately. * fix(task-repeat-cfg): schedule inbox task for today when made recurring When an Inbox task (no dueDay) was made repeatable via the dialog with a recurrence starting today, it stayed unscheduled. The TODAY-first-occurrence branch of updateTaskAfterMakingItRepeatable$ derived currentDueDay from task.created as a fallback, so a task created today looked already scheduled and dueDay was never set. Key the decision on task.dueDay directly. Skip timed tasks and tasks that already have dueWithTime, since dueDay/dueWithTime are mutually exclusive and timed scheduling is handled by addRepeatCfgToTaskUpdateTask$. Closes #7725 * fix(tasks): correct monthly first/last-day recurrence anchoring The "Every month on the first day" and "Every month on the last day" quick settings scheduled the first task instance in the past. Both presets produced a backdated startDate (1st of the current month; hardcoded January 31), which getFirstRepeatOccurrence returns verbatim for monthly recurrences. - MONTHLY_FIRST_DAY now anchors startDate to the next 1st-of-month that is today or later. - MONTHLY_LAST_DAY anchors startDate to the current month's last day and sets a new monthlyLastDay flag, so the occurrence engine clamps to month-end every month regardless of startDate's day-of-month. - _normalizeMonthlyAnchor strips a stale monthlyLastDay flag when a config leaves the preset (CUSTOM mode has no control for it). Closes #7726 * fix(task-repeat-cfg): re-anchor start date after instance deleted When the user moved a repeat config's startDate earlier after deleting its only live task instance, the stale lastTaskCreationDay anchor kept suppressing every projected/created instance between the new startDate and the old anchor. rescheduleTaskOnRepeatCfgUpdate$ only re-anchored lastTaskCreationDay when a live task instance existed (the #7423 fix) — it returned early before the re-anchoring when there was none. Hoist the isStartDateMovedEarlier detection above that early return: when no live instance exists but startDate moved earlier, re-anchor to the day before the new first occurrence so it and every following day is created and projected fresh. Closes #7724 * test(task-repeat-cfg): cover startDate re-anchor with no live instance Add coverage for the #7724 fix beyond the effect unit test: - Selector integration tests: feed a config re-anchored to the day before the new startDate through selectTaskRepeatCfgsForExactDay and assert it projects the new startDate and every following day, while still excluding the anchor day and earlier. Also documents that the stale anchor suppresses the gap days. - E2E reproduction (recurring-move-start-date-earlier-no-instance): create a recurring task, delete its live instance, move startDate earlier via a transparent projection, and assert the new days appear in the planner. Verified to fail on pre-fix code. * feat(sync): move clientId from pf into SUP_OPS for atomic rotation Migrate the sync clientId out of the legacy `pf` IndexedDB database into `SUP_OPS` (new `client_id` store, schema v6). The clientId write now joins the atomic transaction in `runDestructiveStateReplacement`, so destructive flows (clean-slate, backup-restore) rotate it atomically with OPS/STATE_CACHE/VECTOR_CLOCK instead of a hand-rolled cross-database two-phase commit. - ClientIdService rewritten: SUP_OPS-backed via an independent connection, inline one-time pf->SUP_OPS migration, error-aware resolver. Read failures propagate (getOrGenerateClientId never mints a fresh id over a transient error — that would orphan the device's non-regenerable identity); loadClientId never throws. - Delete withRotation, generateNewClientId and the CAS/rollback machinery. - Extract pure generateClientId() + isValidClientIdFormat() into core/util/generate-client-id.ts. - pf becomes a read-only, one-time migration source (never written/deleted). Closes #7732 * fix(sync): dedup SUP_OPS connection open in ClientIdService Address multi-agent review findings on the clientId migration: - _getSupOpsDb() shares a single in-flight open via _supOpsDbPromise; concurrent cold-start callers previously each opened their own SUP_OPS connection, leaking all but the last. - _putClientIdIfAbsent() collapsed to a single tx.done / exit point. - db-upgrade.spec.ts: cover the v6 client_id store; the createObjectStore count assertions were stale and failing after the schema bump. - operation-log-migration.service.ts: correct a misleading comment about the genesis-op clientId fallback. * refactor(sync): align ClientIdService SUP_OPS open with in-house idiom Re-review of the connection-leak fix recommended matching OperationLogStoreService._ensureInit's pattern: - _getSupOpsDb() clears the in-flight promise in .catch (failure only) instead of an unconditional finally — the resolved handle lives in _supOpsDb, so the promise field is pure in-flight coordination state. - close/versionchange handlers now also null _supOpsDbPromise, so a stale (closed) connection is never re-handed-out. - Add a regression test asserting _openSupOpsDb runs exactly once for concurrent cold-start callers. * docs(sync): link the single-connection follow-up to #7735 Reference the tracked follow-up issue from the ClientIdService JSDoc and the plan's out-of-scope section, so the deliberate trade-off (one extra SUP_OPS connection) is traceable rather than forgotten. * test(sync): update ClientIdService spies for getOrGenerateClientId The op-log capture effect now resolves the clientId via getOrGenerateClientId() (was loadClientId() ?? generateNewClientId()). These two specs still mocked only loadClientId, so the effect called an undefined method, captured no op, and 4 tests failed in the full suite. - task-done-replay.integration.spec.ts - operation-log-lock-reentry.regression.spec.ts * test(sync): open SUP_OPS versionless in e2e read helpers DB_VERSION was bumped 5->6; five e2e helpers still opened SUP_OPS at the hardcoded old version 5 to read state after the app had already upgraded it to v6, throwing VersionError. Open versionless instead — matches the ~10 other e2e files that already do, and is future-proof against the next schema bump. - migration/legacy-data-migration.spec.ts (x2) - sync/supersync-legacy-migration-sync.spec.ts - sync/webdav-legacy-migration-sync.spec.ts - recurring/invalid-clock-string-bug-7067.spec.ts
This commit is contained in:
Johannes Millan
GitHub
parent
292f8b0e9a
commit
508998c6a1
60 changed files with 2754 additions and 811 deletions
|
|
@ -223,8 +223,13 @@ class CapacitorMainActivity : BridgeActivity() {
|
|||
SyncReminderScheduler.ensureScheduled(this)
|
||||
}
|
||||
|
||||
// Handle initial intent (cold start)
|
||||
handleIntent(intent)
|
||||
// Handle initial intent (cold start) only on a fresh launch.
|
||||
// On Activity recreation (config change) savedInstanceState is non-null
|
||||
// and getIntent() still holds the original share/reminder Intent — re-running
|
||||
// handleIntent() there would create a duplicate task from the same share.
|
||||
if (savedInstanceState == null) {
|
||||
handleIntent(intent)
|
||||
}
|
||||
}
|
||||
|
||||
private fun showWebViewInitFailureOrThrow(message: String, error: Throwable) {
|
||||
|
|
@ -350,13 +355,20 @@ class CapacitorMainActivity : BridgeActivity() {
|
|||
if (Intent.ACTION_SEND == intent.action && intent.type != null) {
|
||||
if (intent.type?.startsWith("text/") == true) {
|
||||
val sharedText = intent.getStringExtra(Intent.EXTRA_TEXT)
|
||||
val sharedTitle = intent.getStringExtra(Intent.EXTRA_TITLE) ?: "Shared Content"
|
||||
// Leave title/subject empty when absent so the frontend can derive a
|
||||
// meaningful title from the URL or note content. Defaulting to a literal
|
||||
// "Shared Content" here masks that derivation (issue: blank shared tasks).
|
||||
val sharedTitle = intent.getStringExtra(Intent.EXTRA_TITLE) ?: ""
|
||||
val sharedSubject = intent.getStringExtra(Intent.EXTRA_SUBJECT) ?: ""
|
||||
Log.d("SP_SHARE", "Shared text: $sharedText")
|
||||
Log.d("SP_SHARE", "Shared title: $sharedTitle")
|
||||
Log.d("SP_SHARE", "Shared subject: $sharedSubject")
|
||||
|
||||
if (sharedText != null) {
|
||||
// Ignore empty/blank shares — they only produce useless blank tasks.
|
||||
if (!sharedText.isNullOrBlank()) {
|
||||
val json = JSONObject()
|
||||
json.put("title", sharedTitle)
|
||||
json.put("subject", sharedSubject)
|
||||
val type = if (sharedText.startsWith("http")) "LINK" else "NOTE"
|
||||
json.put("type", type)
|
||||
json.put("path", sharedText)
|
||||
|
|
|
|||
491
docs/plans/2026-05-22-clientid-migrate-to-sup-ops.md
Normal file
491
docs/plans/2026-05-22-clientid-migrate-to-sup-ops.md
Normal file
|
|
@ -0,0 +1,491 @@
|
|||
# Migrate sync `clientId` from `pf` into `SUP_OPS`
|
||||
|
||||
**Issue:** #7732 — follow-up to PR #7712 / issue #7709
|
||||
**Status:** Draft plan — revised after three multi-agent review rounds
|
||||
**Prerequisite:** #7712 merged (2026-05-22) ✅
|
||||
|
||||
## Goal
|
||||
|
||||
Move the sync `clientId` out of the legacy `pf` IndexedDB database into
|
||||
`SUP_OPS` so the clientId write joins the atomic multi-store transaction in
|
||||
`OperationLogStoreService.runDestructiveStateReplacement()`. Once it does, the
|
||||
hand-rolled two-phase commit `ClientIdService.withRotation()` (and its CAS guard
|
||||
and rollback-failure logging) is deleted.
|
||||
|
||||
## Design decisions (and why)
|
||||
|
||||
Three review rounds converged on these. Two reverse earlier-draft mistakes —
|
||||
kept here as explicit decisions so they are not "re-improved" into bugs again.
|
||||
|
||||
1. **No permanent `pf` mirror.** Downgrading past this schema bump opens
|
||||
`SUP_OPS` at a lower version than stored → `VersionError` → the op-log/sync
|
||||
subsystem is dead regardless of where the clientId lives. A mirror cannot
|
||||
rescue that. `pf` becomes a **read-only, one-time migration source.**
|
||||
|
||||
2. **Self-healing read, no separate migration service.** The `pf → SUP_OPS`
|
||||
copy happens inline in the clientId resolver, triggered lazily by the first
|
||||
read. This removes the init-ordering failure mode — the clientId is read very
|
||||
early and is _non-regenerable_, so a self-gating read is safer than call-order
|
||||
discipline plus an ordering test.
|
||||
|
||||
3. **`getOrGenerateClientId()` must never generate on a read _failure_.**
|
||||
_(Reverses an earlier draft.)_ An earlier draft made `loadClientId()` "never
|
||||
throw" and had `getOrGenerateClientId()` generate whenever it returned
|
||||
`null`. That converts a transient IndexedDB hiccup into a brand-new clientId
|
||||
that orphans the device's real, history-bearing id — the exact
|
||||
non-regenerable loss this issue exists to prevent. The resolver therefore
|
||||
**propagates IndexedDB read errors**; generation happens _only_ when reads
|
||||
succeed and confirm no id exists anywhere. This matches today's behavior
|
||||
(today `getOrGenerateClientId` throws on a DB-open failure rather than
|
||||
generating).
|
||||
|
||||
4. **`OperationLogMigrationService`'s genesis-op clientId resolution is left
|
||||
unchanged.** _(Reverses an earlier draft.)_ An earlier draft routed it
|
||||
through `getOrGenerateClientId()`. That is unsafe: the legacy genesis op is
|
||||
built as `{ clientId, vectorClock: meta.vectorClock || { [clientId]: 1 } }`,
|
||||
and `meta.vectorClock` is keyed by the _legacy PFAPI_ identity — the `pf`
|
||||
`CLIENT_ID` key. The migration must keep resolving the genesis clientId from
|
||||
`CLIENT_ID` so the op's `clientId` matches its own `vectorClock` keys.
|
||||
`persistClientId` is therefore **kept** (not deleted), and now also seeds the
|
||||
new `SUP_OPS` store.
|
||||
|
||||
5. **`ClientIdService` is _not_ relocated in this PR.** The relocation to
|
||||
`op-log/util/` is a pure rename touching ~12 import sites for zero behavioral
|
||||
benefit (the `core ↔ op-log` coupling already exists via
|
||||
`client-id.provider.ts`). Per "minimize changes / stay in scope" it is a
|
||||
separate follow-up. `ClientIdService` stays in `core/util/` and imports the
|
||||
`SUP_OPS` schema constants from `op-log/persistence/` (a layering smell, but
|
||||
no lint rule forbids it and the provider already crosses that boundary).
|
||||
|
||||
Net effect: roughly line-neutral versus today's `withRotation` machinery, but a
|
||||
clear win in _conceptual_ complexity — cross-database two-phase commit is
|
||||
replaced by a single in-transaction `put` plus a one-time idempotent copy.
|
||||
|
||||
## Files touched
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `src/app/op-log/persistence/db-keys.const.ts` | `DB_VERSION` 5→6; add `STORE_NAMES.CLIENT_ID` |
|
||||
| `src/app/op-log/persistence/db-upgrade.ts` | Version 6 branch: create `client_id` store |
|
||||
| `src/app/op-log/persistence/operation-log-store.service.ts` | `OpLogDB` schema entry; `client_id` `put` in `runDestructiveStateReplacement` + post-commit `clearCache()`; `_clearAllDataForTesting` |
|
||||
| `src/app/core/util/generate-client-id.ts` | **New** — pure `generateClientId()` + `isValidClientIdFormat()` |
|
||||
| `src/app/core/util/client-id.service.ts` | Rewritten in place — `SUP_OPS`-backed, inline one-time `pf` migration, error-aware resolver, no `withRotation` |
|
||||
| `src/app/op-log/util/client-id.provider.ts` | Add `clearCache()` to the `ClientIdProvider` interface + factory; doc-note the migration side effect |
|
||||
| `src/app/op-log/clean-slate/clean-slate.service.ts` | Rewrite: pure id gen, no `withRotation`, no cache handling |
|
||||
| `src/app/op-log/backup/backup.service.ts` | Rewrite: pure id gen, no `withRotation`, no cache handling |
|
||||
| `src/app/op-log/capture/operation-log.effects.ts` | `loadClientId() ?? generateNewClientId()` → `getOrGenerateClientId()` |
|
||||
| `src/app/op-log/persistence/operation-log-migration.service.ts` | `generateNewClientId()` → `getOrGenerateClientId()` for the fallback only; **fix `:249` — stop logging the full clientId**. Clientid resolution otherwise unchanged. |
|
||||
| `docs/sync-and-op-log/` | Note the clientId now lives in `SUP_OPS` v6 (per CLAUDE.md: op-log changes need doc updates) |
|
||||
| Specs | see Step 6 |
|
||||
|
||||
`LegacyPfDbService` is **not** modified or used by `ClientIdService` — see Step 4
|
||||
for why its error-swallowing `load()` is unsuitable here.
|
||||
|
||||
## Step 1 — Schema bump
|
||||
|
||||
### `db-keys.const.ts`
|
||||
|
||||
```ts
|
||||
export const DB_VERSION = 6; // was 5
|
||||
|
||||
export const STORE_NAMES = {
|
||||
// ...existing...
|
||||
/** Client ID - sync device identity (singleton, key = SINGLETON_KEY) */
|
||||
CLIENT_ID: 'client_id' as const,
|
||||
} as const;
|
||||
```
|
||||
|
||||
`SINGLETON_KEY = 'current'` is reused as the key.
|
||||
|
||||
> **Hard pre-merge gate:** confirm no other in-flight PR also bumps `DB_VERSION`
|
||||
> to 6. `db-upgrade.ts` runs exactly one callback per version transition; a
|
||||
> collided version corrupts the `SUP_OPS` schema irrecoverably.
|
||||
|
||||
### `db-upgrade.ts`
|
||||
|
||||
```ts
|
||||
// Version 6: Add client_id store for atomic clientId rotation.
|
||||
// Consolidates the sync clientId from legacy 'pf' (key '__client_id_') into
|
||||
// SUP_OPS so destructive-flow rotation joins runDestructiveStateReplacement's
|
||||
// atomic transaction. See issue #7732. The runtime copy from 'pf' happens in
|
||||
// ClientIdService (a versionchange tx cannot read another database).
|
||||
if (oldVersion < 6) {
|
||||
db.createObjectStore(STORE_NAMES.CLIENT_ID);
|
||||
}
|
||||
```
|
||||
|
||||
Keyless store (out-of-line key, like `vector_clock`).
|
||||
|
||||
### `operation-log-store.service.ts` — `OpLogDB` schema
|
||||
|
||||
```ts
|
||||
[STORE_NAMES.CLIENT_ID]: {
|
||||
key: string; // SINGLETON_KEY
|
||||
value: string; // the clientId
|
||||
};
|
||||
```
|
||||
|
||||
Add `STORE_NAMES.CLIENT_ID` to the `_clearAllDataForTesting()` store list and a
|
||||
matching `.clear()`. (`ArchiveDBSchema` in `archive-store.service.ts` does _not_
|
||||
need the entry — that service never touches the store; the shared `runDbUpgrade`
|
||||
still creates it.)
|
||||
|
||||
## Step 2 — `generate-client-id.ts` (new pure util)
|
||||
|
||||
Extract the existing pure generation logic out of `ClientIdService` into
|
||||
`src/app/core/util/generate-client-id.ts`:
|
||||
|
||||
```ts
|
||||
/** Generates a compact client ID: {platform}_{4-char-base62}, e.g. "B_a7Kx". */
|
||||
export const generateClientId = (): string => {
|
||||
/* _generateClientId body */
|
||||
};
|
||||
|
||||
/** True if the id matches a known valid format (legacy length>=10, or new). */
|
||||
export const isValidClientIdFormat = (id: unknown): id is string => {
|
||||
/* ... */
|
||||
};
|
||||
```
|
||||
|
||||
`_getEnvironmentId` / `_generateBase62` move here as module-private helpers.
|
||||
Pure, no DI, no I/O — unit-testable directly, and importable by the
|
||||
destructive-flow callers (`op-log → core` is the legal dependency direction)
|
||||
without going through the stateful service. `isValidClientIdFormat` is a type
|
||||
guard so callers narrow `unknown` reads from IndexedDB cleanly. No external code
|
||||
imports the current `private _isValidClientIdFormat`, so the extraction is clean.
|
||||
|
||||
## Step 3 — `runDestructiveStateReplacement` joins the clientId write
|
||||
|
||||
In `operation-log-store.service.ts`, in `runDestructiveStateReplacement` (~line
|
||||
1584):
|
||||
|
||||
- Add `STORE_NAMES.CLIENT_ID` to the `storeNames` array **unconditionally**
|
||||
(~line 1597) — both callers always rotate; unlike the archive stores it is not
|
||||
conditional.
|
||||
- Inside the `try`, **before `await opsStore.clear()` (~line 1615)**, write the
|
||||
clientId first:
|
||||
|
||||
```ts
|
||||
await tx.objectStore(STORE_NAMES.CLIENT_ID).put(syncImportOp.clientId, SINGLETON_KEY);
|
||||
```
|
||||
|
||||
Use an inline `tx.objectStore(...)` call (the value is written once; no hoisted
|
||||
handle needed). The rotated id is already on the op — no new parameter.
|
||||
**First-in-tx is deliberate:** the interrupt tests inject failure into
|
||||
`opsStore.add`; placing the `client_id` `put` first means that injected
|
||||
failure occurs _after_ the `client_id` `put` is queued, so the abort genuinely
|
||||
exercises "client_id put queued → tx aborts → `client_id` unchanged."
|
||||
Atomicity itself is order-independent.
|
||||
|
||||
- After `await tx.done` (~line 1654), invalidate the clientId cache so the next
|
||||
read sees the rotated value:
|
||||
|
||||
```ts
|
||||
this.clientIdProvider.clearCache();
|
||||
```
|
||||
|
||||
`OperationLogStoreService` already injects `CLIENT_ID_PROVIDER`. Doing the
|
||||
cache-clear _inside_ `runDestructiveStateReplacement`, bound to `tx.done`,
|
||||
makes it impossible for a future edit to open a window between commit and
|
||||
cache-clear. On `catch`/abort, `clearCache()` is not reached and the cache
|
||||
correctly keeps the old id.
|
||||
|
||||
- Replace the doc-comment paragraph about "Atomicity holds within the `SUP_OPS`
|
||||
database only … callers own the clientId rollback" with: the clientId now
|
||||
lives in `SUP_OPS` and rotates atomically with `OPS`/`STATE_CACHE`/`VECTOR_CLOCK`.
|
||||
|
||||
## Step 4 — `ClientIdService` rewrite
|
||||
|
||||
Rewritten **in place** at `src/app/core/util/client-id.service.ts` (no
|
||||
relocation — decision 5).
|
||||
|
||||
### Databases this service touches
|
||||
|
||||
- **`SUP_OPS`** — an **independent connection** opened via the shared
|
||||
`runDbUpgrade` + `DB_NAME`/`DB_VERSION`. Independent because
|
||||
`OperationLogStoreService` injects `CLIENT_ID_PROVIDER` (→ `ClientIdService`),
|
||||
so delegating back would be a DI cycle. Two same-origin connections to one
|
||||
store are fine — IndexedDB serializes transactions across them. Register a
|
||||
`'close'` handler (null the cached handle, reopen on next access — mirror
|
||||
`OperationLogStoreService.init` at `:194-200`) and a `versionchange` handler
|
||||
(`db.close()`) so a future v7 upgrade is not blocked. The open does **not**
|
||||
replicate `OperationLogStoreService`'s heavy retry logic; a transient
|
||||
`SUP_OPS` open failure surfaces as a thrown error (handled per the resolver
|
||||
contract below).
|
||||
- **`pf`** — opened **read-only, directly by this service**, per-call
|
||||
(open-read-close, no cached handle). It is **not** routed through
|
||||
`LegacyPfDbService`: that service's `load()`/`loadClientId()` _swallow_
|
||||
IndexedDB errors and return `null`, which makes "key genuinely absent"
|
||||
indistinguishable from "read failed" — and that distinction is exactly what
|
||||
decision 3 depends on. `ClientIdService`'s own `pf` read lets IndexedDB errors
|
||||
**propagate**. (Opening a non-existent `pf` creates an empty one; this is
|
||||
harmless and is already the current behavior.)
|
||||
|
||||
### Final public surface
|
||||
|
||||
```ts
|
||||
loadClientId(): Promise<string | null> // never throws; null on absence OR read failure
|
||||
getOrGenerateClientId(): Promise<string> // resolves, else generates; throws on read failure
|
||||
persistClientId(id: string): Promise<void> // legacy-migration genesis seed; validated; unconditional
|
||||
clearCache(): void // invalidate the in-memory cache
|
||||
```
|
||||
|
||||
`getOrGenerateClientId` and `loadClientId` keep their current names/signatures,
|
||||
so `CLIENT_ID_PROVIDER` and its consumers are unaffected. `clearCache()` is
|
||||
promoted from a test helper to a documented production method (used by
|
||||
`runDestructiveStateReplacement`); its JSDoc must say so.
|
||||
|
||||
**Deleted:** `withRotation`, `_restorePriorClientIdIfCurrentMatches`,
|
||||
`_errorName`, `generateNewClientId`, and the in-service generation helpers
|
||||
(moved to `generate-client-id.ts`).
|
||||
|
||||
### `_resolve()` — the shared resolver (private)
|
||||
|
||||
The one place that answers "what is this device's clientId, migrating it
|
||||
forward if needed". **Read failures propagate; only a failed copy-forward is
|
||||
swallowed.**
|
||||
|
||||
```
|
||||
private async _resolve(): Promise<string | null> {
|
||||
const fromOps = await this._readSupOps(); // throws on IndexedDB read error
|
||||
if (fromOps) return fromOps;
|
||||
const fromPf = await this._readPf(); // throws on IndexedDB read error
|
||||
if (!fromPf) return null; // reads succeeded -> confirmed: no id anywhere
|
||||
try {
|
||||
return await this._putClientIdIfAbsent(() => fromPf);
|
||||
} catch {
|
||||
// Copy-forward to SUP_OPS failed (quota, closed conn). The pf id is valid;
|
||||
// return it and let a later launch retry the copy. Worst case: redundant copy.
|
||||
return fromPf;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- `_readSupOps()` — open `SUP_OPS`, `get(client_id, SINGLETON_KEY)`,
|
||||
`isValidClientIdFormat`-gate (invalid → `null`, never throw on bad _format_ —
|
||||
issue #6197). IndexedDB _errors_ propagate.
|
||||
- `_readPf()` — open `pf` read-only, read `__client_id_` then `CLIENT_ID`,
|
||||
format-gate, return first valid or `null`. IndexedDB _errors_ propagate.
|
||||
|
||||
> **`pf` key precedence.** `__client_id_` is the key `ClientIdService` has always
|
||||
> operated on (every current `loadClientId()` reads it); on an op-log-era device
|
||||
> it is the live identity. `CLIENT_ID` is the original PFAPI key, read only as a
|
||||
> fallback to seed a legacy profile. On a legacy device migrating for the first
|
||||
> time, `OperationLogMigrationService` resolves the genesis op from `CLIENT_ID`
|
||||
> and `persistClientId` writes it **unconditionally** to `SUP_OPS` — so it wins
|
||||
> over any `__client_id_`-derived copy, keeping the genesis op consistent with
|
||||
> its `meta.vectorClock` (decision 4).
|
||||
|
||||
### `_putClientIdIfAbsent(factory)` — shared single-tx CAS (private)
|
||||
|
||||
```
|
||||
private async _putClientIdIfAbsent(factory: () => string | null): Promise<string | null> {
|
||||
const tx = supOpsDb.transaction(CLIENT_ID, 'readwrite');
|
||||
const raced = await tx.store.get(SINGLETON_KEY);
|
||||
if (isValidClientIdFormat(raced)) { await tx.done; return raced; }
|
||||
const next = factory();
|
||||
if (next) await tx.store.put(next, SINGLETON_KEY);
|
||||
await tx.done;
|
||||
return next;
|
||||
}
|
||||
```
|
||||
|
||||
The in-tx re-check is load-bearing: IndexedDB serializes same-store transactions
|
||||
across same-origin connections, so a write that committed first (another tab's
|
||||
generate, or a rotation) is observed by `raced` and **wins** — the helper never
|
||||
clobbers it. Comment it as a _multi-tab / rotation_ guard. `persistClientId` and
|
||||
`runDestructiveStateReplacement` are the two _unconditional_ writers (they know
|
||||
the exact intended value); `_putClientIdIfAbsent` is the _establish-if-absent_
|
||||
writer — that asymmetry is deliberate.
|
||||
|
||||
### `loadClientId()` — swallowing reader
|
||||
|
||||
```
|
||||
if (_cachedClientId) return _cachedClientId;
|
||||
try {
|
||||
const id = await this._resolve();
|
||||
if (id) _cachedClientId = id;
|
||||
return id;
|
||||
} catch {
|
||||
return null; // never throws — callers (hydrator, sync readers) tolerate null
|
||||
}
|
||||
```
|
||||
|
||||
The cache is the migration's memoization: once warm, `_resolve()` (and the `pf`
|
||||
read) never run again. Concurrent first-launch callers may each run `_resolve()`
|
||||
— harmless, `_putClientIdIfAbsent` is idempotent.
|
||||
|
||||
### `getOrGenerateClientId()` — resolves or generates
|
||||
|
||||
```
|
||||
if (_cachedClientId) return _cachedClientId;
|
||||
const existing = await this._resolve(); // PROPAGATES read failures — does not swallow
|
||||
if (existing) { _cachedClientId = existing; return existing; }
|
||||
// Reads succeeded and confirmed empty everywhere -> safe to generate.
|
||||
const id = await this._putClientIdIfAbsent(() => generateClientId());
|
||||
_cachedClientId = id;
|
||||
return id;
|
||||
```
|
||||
|
||||
If `_resolve()` throws (transient `SUP_OPS`/`pf` read failure),
|
||||
`getOrGenerateClientId()` throws — it does **not** generate. This is the same
|
||||
contract as today (`generateNewClientId` already throws on IDB failure); callers
|
||||
(`operation-log.effects.ts:171-175`, and via the provider
|
||||
`file-based-encryption.service.ts`, `snapshot-upload.service.ts`) already treat
|
||||
a failed clientId resolution as fatal-and-retryable.
|
||||
|
||||
### `persistClientId(id)` — legacy-migration genesis seed
|
||||
|
||||
Validate format, **unconditionally** `put` into `SUP_OPS.client_id`, set the
|
||||
cache. Unconditional (not CAS) because it carries the authoritative legacy
|
||||
`CLIENT_ID` value that the genesis op is built from and must win over any
|
||||
`__client_id_`-derived migration copy. No `pf` write.
|
||||
|
||||
## Step 5 — Rewrite the callers
|
||||
|
||||
### `clean-slate.service.ts` / `backup.service.ts`
|
||||
|
||||
Both already rotate inside `lockService.request(LOCK_NAMES.OPERATION_LOG, …)`.
|
||||
The rewrite — no `withRotation`, no try/catch, no cache handling:
|
||||
|
||||
```ts
|
||||
import { generateClientId } from '../../core/util/generate-client-id';
|
||||
|
||||
const newClientId = generateClientId(); // pure — persisted only inside the tx
|
||||
const syncImportOp: Operation = {
|
||||
/* ...clientId: newClientId... */
|
||||
};
|
||||
await this.opLogStore.runDestructiveStateReplacement({ syncImportOp /* ... */ });
|
||||
// runDestructiveStateReplacement committed the new clientId and cleared the
|
||||
// cache; nothing else to do. On throw, the tx aborted and the old id stands.
|
||||
```
|
||||
|
||||
Update the class/method doc comments that describe the cross-DB rollback.
|
||||
|
||||
### `operation-log.effects.ts`
|
||||
|
||||
Replace `loadClientId() ?? generateNewClientId()` (`:168-170`) with
|
||||
`await this.clientIdService.getOrGenerateClientId()`.
|
||||
|
||||
### `operation-log-migration.service.ts`
|
||||
|
||||
Clientid resolution is **unchanged** (decision 4) — keep `:239`
|
||||
(`loadMetaModel`), keep `legacyPfDb.loadClientId()` reading `CLIENT_ID`, keep
|
||||
`persistClientId(legacyClientId)`. Two edits only:
|
||||
|
||||
- `:241` fallback: `legacyClientId || generateNewClientId()` →
|
||||
`legacyClientId ?? (await this.clientIdService.getOrGenerateClientId())`
|
||||
(`generateNewClientId` is deleted; the fallback only fires when there is no
|
||||
legacy identity to preserve, so generating is correct).
|
||||
- `:249`: stop logging the literal clientId
|
||||
(`OpLog.normal(\`...Using client ID: ${clientId}\`)`— a CLAUDE.md sync-rule-9
|
||||
violation, log history is user-exportable). Log a 3-char suffix only,
|
||||
consistent with`clean-slate.service.ts`. Audit the remaining `OpLog` calls in
|
||||
every touched file (spot-checked: the rest are already value-free).
|
||||
|
||||
## Step 6 — Tests
|
||||
|
||||
### `client-id.service.spec.ts` (rewritten, stays in `core/util/`)
|
||||
|
||||
Drop all `withRotation` tests. Behavioral matrix:
|
||||
|
||||
| Case | Expectation |
|
||||
| ------------------------------------------------- | --------------------------------------------------------------------------------------------- |
|
||||
| `SUP_OPS.client_id` populated | returned directly; `pf` not opened |
|
||||
| `SUP_OPS` empty, `pf.__client_id_` valid | migrated into `SUP_OPS`; id unchanged |
|
||||
| `SUP_OPS` empty, only `pf.CLIENT_ID` | migrated; covers the bridge-ordering gap |
|
||||
| `SUP_OPS` empty, both `pf` keys valid and differ | `__client_id_` wins |
|
||||
| `SUP_OPS` invalid format, `pf` valid | `pf` value wins, overwrites `SUP_OPS` |
|
||||
| nothing anywhere | `loadClientId()` → `null`; `getOrGenerateClientId()` generates |
|
||||
| multi-tab fresh generate | two `getOrGenerateClientId()` over one fake-IDB converge on one id |
|
||||
| **`SUP_OPS` read throws** | `loadClientId()` → `null` (no throw); `getOrGenerateClientId()` **throws, does not generate** |
|
||||
| **`pf` read throws** | same — `loadClientId()` → `null`; `getOrGenerateClientId()` throws, no generation |
|
||||
| **migration copy-forward write fails (quota)** | `loadClientId()` & `getOrGenerateClientId()` return the `pf` id; no throw, no generation |
|
||||
| `persistClientId` | unconditional `SUP_OPS` write; cache set; rejects invalid format |
|
||||
| `generateClientId` / `isValidClientIdFormat` util | pure; correct format / guard |
|
||||
|
||||
The three **bold** rows are the data-safety core — they prove a transient
|
||||
IndexedDB failure cannot mint a new clientId. They must use a fake-IDB that can
|
||||
be made to throw, not just be empty.
|
||||
|
||||
### Destructive-flow atomicity
|
||||
|
||||
`clean-slate-interrupt.integration.spec.ts` must be **extended**: seed
|
||||
`SUP_OPS.client_id` with a valid-format id, run the existing `opsStore.add`-throw
|
||||
interrupt, and assert `SUP_OPS.client_id` is unchanged after the abort (this is
|
||||
the property `withRotation` used to provide by hand). Existing fixtures that seed
|
||||
short ids like `'cPrior'` must switch to valid-format ids (`B_xxxx` / length ≥
|
||||
10), or the new format guard treats them as absent.
|
||||
|
||||
### Other specs to update
|
||||
|
||||
Grep `withRotation`, `generateNewClientId`, `persistClientId`. Known set:
|
||||
|
||||
- **`withRotation` removed:** `clean-slate.service.spec.ts`,
|
||||
`backup.service.spec.ts`, `clean-slate-interrupt.integration.spec.ts`,
|
||||
`operation-log.effects.spec.ts` — delete `withRotation` mocks/expectations;
|
||||
the `ClientIdService` spy surface becomes `getOrGenerateClientId` (+ no manual
|
||||
cache handling — `runDestructiveStateReplacement` owns `clearCache`).
|
||||
- **`generateNewClientId` removed:** `client-id.provider.spec.ts`,
|
||||
`sync-hydration.service.spec.ts`,
|
||||
`legacy-data-migration.integration.spec.ts` — remove it from
|
||||
`jasmine.createSpyObj` arrays; `client-id.provider.spec.ts:15` assertion
|
||||
dropped.
|
||||
- **`operation-log-migration.service.spec.ts`:** `persistClientId` is **kept**,
|
||||
so its tests at `:418`/`:434` largely stand; only swap the `generateNewClientId`
|
||||
spy/`callFake` (incl. the manual logic at `:374-379`) for
|
||||
`getOrGenerateClientId`.
|
||||
|
||||
### Test teardown
|
||||
|
||||
`_clearAllDataForTesting()` clears `SUP_OPS.client_id` but not
|
||||
`ClientIdService._cachedClientId` (separate service/connection). Specs that clear
|
||||
data then expect a fresh id must also call `clientIdService.clearCache()`. Most
|
||||
`_clearAllDataForTesting()` callers never mint a clientId mid-test, so the blast
|
||||
radius is small — but the relocated/rewritten spec and the caller specs must be
|
||||
audited.
|
||||
|
||||
## Risks & mitigations
|
||||
|
||||
1. **Non-regenerable clientId (lead risk).** `pf` is never deleted or written.
|
||||
`_resolve()` only ever _copies_; a failed copy still returns the valid `pf`
|
||||
id. `getOrGenerateClientId()` generates **only** after reads succeed and
|
||||
confirm absence everywhere — a transient failure throws, never generates.
|
||||
Worst case is a redundant copy.
|
||||
2. **No downgrade support.** Downgrading past v6 → `VersionError` → op-log dead
|
||||
regardless of the clientId. True of every prior schema bump; not regressed,
|
||||
not pretended-to-be-supported. No `pf` mirror.
|
||||
3. **Init ordering.** Eliminated — `_resolve()` self-heals on first read.
|
||||
4. **Multi-tab.** `_putClientIdIfAbsent`'s single-tx CAS converges concurrent
|
||||
same-origin runs. Mixed-version tabs cannot serialize, but an old app post-v6
|
||||
has a `VersionError`'d op-log anyway — non-functional, not a data-loss path.
|
||||
5. **Schema-upgrade coordination.** The new `ClientIdService` connection gets a
|
||||
`versionchange` handler. The pre-existing absence of `versionchange` handlers
|
||||
on `OperationLogStoreService`/`ArchiveStoreService` is **out of scope** —
|
||||
adding them helps only future (v6→v7) upgrades, not this one, and is left as
|
||||
a follow-up to keep this PR's diff minimal.
|
||||
6. **Legacy genesis-op continuity.** `OperationLogMigrationService`'s clientId
|
||||
resolution is unchanged (decision 4); the genesis op keeps using `CLIENT_ID`,
|
||||
matching `meta.vectorClock`'s keys.
|
||||
|
||||
## Out of scope / follow-ups
|
||||
|
||||
The first three are tracked together in **#7735**:
|
||||
|
||||
- Relocating `ClientIdService` to `op-log/util/` (a pure rename, ~12 import
|
||||
sites) — separate PR.
|
||||
- Adding `versionchange` handlers to `OperationLogStoreService` /
|
||||
`ArchiveStoreService`.
|
||||
- Breaking the `OperationLogStoreService` ↔ `CLIENT_ID_PROVIDER` DI cycle to
|
||||
collapse onto one shared `SUP_OPS` connection.
|
||||
|
||||
Not yet tracked:
|
||||
|
||||
- Tightening `isValidClientIdFormat` (the legacy `length >= 10` branch accepts
|
||||
almost any string) — pre-existing, not this PR.
|
||||
- Deleting the `pf` database — it remains a read-only fallback.
|
||||
|
||||
## Sequencing
|
||||
|
||||
#7712 is merged. Land as a single PR: the schema bump, the service rewrite, and
|
||||
the caller rewrites are interdependent and cannot be split safely.
|
||||
|
|
@ -196,6 +196,7 @@ interface StateCache {
|
|||
│ │ meta - Vector clocks, sync state │ │
|
||||
│ │ archive_young - Recent archived tasks │ │
|
||||
│ │ archive_old - Old archived tasks │ │
|
||||
│ │ client_id - Sync device identity (v6) │ │
|
||||
│ └──────────────────────────────────────────────────────────┘ │
|
||||
│ │
|
||||
│ ALL model data persisted here │
|
||||
|
|
@ -1841,6 +1842,17 @@ What if data exists in both `pf` AND `SUP_OPS` databases?
|
|||
|
||||
**Mitigation (current):** Genesis migration is a one-time event. Once SUP_OPS is established, all writes go there. Risk is limited to the migration moment itself.
|
||||
|
||||
**Sync `clientId` (SUP_OPS schema v6, issue #7732):** The sync `clientId` —
|
||||
the device's stable sync identity — lives in the `SUP_OPS` `client_id` store
|
||||
(key `current`). It used to live in the legacy `pf` database; storing it in
|
||||
`SUP_OPS` lets destructive flows (clean-slate, backup-restore) rotate it
|
||||
atomically inside `runDestructiveStateReplacement`'s transaction, instead of a
|
||||
hand-rolled cross-database two-phase commit. `pf` remains a read-only, one-time
|
||||
migration source: the first read on a not-yet-migrated device copies the id
|
||||
forward (`ClientIdService`). The clientId is non-regenerable (it keys the vector
|
||||
clock), so a transient IndexedDB read failure propagates rather than minting a
|
||||
fresh id.
|
||||
|
||||
### Compaction During Active Sync
|
||||
|
||||
**Status:** Handled via Locks
|
||||
|
|
|
|||
|
|
@ -74,7 +74,7 @@ const readMigratedState = async (
|
|||
}> => {
|
||||
return page.evaluate(async () => {
|
||||
return new Promise((resolve, reject) => {
|
||||
const request = indexedDB.open('SUP_OPS', 5);
|
||||
const request = indexedDB.open('SUP_OPS');
|
||||
request.onsuccess = (event) => {
|
||||
const db = (event.target as IDBOpenDBRequest).result;
|
||||
const tx = db.transaction('state_cache', 'readonly');
|
||||
|
|
@ -107,7 +107,7 @@ const readMigratedArchive = async (
|
|||
}> => {
|
||||
return page.evaluate(async (storeKey) => {
|
||||
return new Promise((resolve, reject) => {
|
||||
const request = indexedDB.open('SUP_OPS', 5);
|
||||
const request = indexedDB.open('SUP_OPS');
|
||||
request.onsuccess = (event) => {
|
||||
const db = (event.target as IDBOpenDBRequest).result;
|
||||
const tx = db.transaction(storeKey, 'readonly');
|
||||
|
|
|
|||
|
|
@ -113,7 +113,7 @@ test('should not crash when a repeat config has an invalid startTime in the stor
|
|||
// hydrator replays it.
|
||||
const idbCorrupted = await page.evaluate(async (): Promise<boolean> => {
|
||||
return new Promise<boolean>((resolve) => {
|
||||
const req = indexedDB.open('SUP_OPS', 5);
|
||||
const req = indexedDB.open('SUP_OPS');
|
||||
req.onerror = () => resolve(false);
|
||||
req.onsuccess = () => {
|
||||
const db = req.result;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,144 @@
|
|||
import { Locator, Page } from '@playwright/test';
|
||||
import { expect, test } from '../../fixtures/test.fixture';
|
||||
|
||||
/**
|
||||
* Bug: https://github.com/super-productivity/super-productivity/issues/7724
|
||||
*
|
||||
* Sibling of #7423. A recurring config carries `lastTaskCreationDay` — a
|
||||
* watermark that the planner's projection logic treats as a hard lower bound:
|
||||
* days at or before it are never projected/created.
|
||||
*
|
||||
* `rescheduleTaskOnRepeatCfgUpdate$` re-anchors that watermark when the user
|
||||
* moves `startDate` earlier — but it used to bail out early when no live task
|
||||
* instance existed, so the re-anchoring never ran. Result: after the user
|
||||
* DELETES the materialised instance and then moves `startDate` earlier, the
|
||||
* stale watermark keeps suppressing every day between the new `startDate` and
|
||||
* the old watermark.
|
||||
*
|
||||
* Repro from the issue (today fixed to 2026-05-01):
|
||||
* 1. Create task → make recurring (daily) → startDate = 2026-05-04 → save
|
||||
* 2. Delete the live (non-transparent) instance
|
||||
* 3. Open the repeat config from a transparent projection → startDate =
|
||||
* 2026-05-02 → save
|
||||
* Expected post-fix: the task projects onto May 2, 3 and 4.
|
||||
* Bug: May 2, 3 and 4 stay empty; projections resume on May 5.
|
||||
*
|
||||
* Dates kept close to today so they fit inside the planner's default 15-day
|
||||
* desktop window without horizontal scrolling.
|
||||
*/
|
||||
|
||||
const FIXED_TODAY = new Date('2026-05-01T10:00:00');
|
||||
|
||||
const openRecurDialog = async (page: Page): Promise<Locator> => {
|
||||
const recurItem = page
|
||||
.locator('task-detail-item')
|
||||
.filter({ has: page.locator('mat-icon', { hasText: /^repeat$/ }) });
|
||||
await expect(recurItem).toBeVisible({ timeout: 5000 });
|
||||
await recurItem.click();
|
||||
const dialog = page.locator('mat-dialog-container');
|
||||
await dialog.waitFor({ state: 'visible', timeout: 10000 });
|
||||
return dialog;
|
||||
};
|
||||
|
||||
// After the live instance is deleted, the repeat config can only be edited via
|
||||
// a transparent projection in the planner — clicking it opens the same dialog.
|
||||
const openRecurDialogFromProjection = async (
|
||||
page: Page,
|
||||
taskTitle: string,
|
||||
): Promise<Locator> => {
|
||||
const projection = page
|
||||
.locator('planner-repeat-projection')
|
||||
.filter({ hasText: taskTitle })
|
||||
.first();
|
||||
await expect(projection).toBeVisible({ timeout: 15000 });
|
||||
await projection.click();
|
||||
const dialog = page.locator('mat-dialog-container');
|
||||
await dialog.waitFor({ state: 'visible', timeout: 10000 });
|
||||
return dialog;
|
||||
};
|
||||
|
||||
// Set the Start date by typing into the matInput directly (en-GB → "DD/MM/YYYY").
|
||||
const setStartDate = async (page: Page, ddmmyyyy: string): Promise<void> => {
|
||||
const dialog = page.locator('mat-dialog-container');
|
||||
const startDateInput = dialog
|
||||
.locator('mat-form-field')
|
||||
.filter({ hasText: /Start date/i })
|
||||
.locator('input')
|
||||
.first();
|
||||
await expect(startDateInput).toBeVisible({ timeout: 5000 });
|
||||
await startDateInput.fill('');
|
||||
await startDateInput.fill(ddmmyyyy);
|
||||
await startDateInput.press('Tab');
|
||||
await expect(startDateInput).toHaveValue(ddmmyyyy, { timeout: 3000 });
|
||||
};
|
||||
|
||||
const saveDialog = async (page: Page): Promise<void> => {
|
||||
const dialog = page.locator('mat-dialog-container');
|
||||
const saveBtn = dialog.getByRole('button', { name: /Save/i });
|
||||
await expect(saveBtn).toBeEnabled({ timeout: 5000 });
|
||||
await saveBtn.click();
|
||||
await dialog.waitFor({ state: 'hidden', timeout: 10000 });
|
||||
};
|
||||
|
||||
test.describe('Recurring Task - Move Start Date Earlier With No Live Instance (#7724)', () => {
|
||||
test('moving startDate earlier still projects the new days after the instance is deleted', async ({
|
||||
page,
|
||||
workViewPage,
|
||||
taskPage,
|
||||
testPrefix,
|
||||
}) => {
|
||||
const taskTitle = `${testPrefix}-MoveStartEarlierNoInstance7724`;
|
||||
|
||||
// Fix today to Friday, May 1, 2026 so calendar interactions are deterministic.
|
||||
await page.clock.setFixedTime(FIXED_TODAY);
|
||||
await page.reload();
|
||||
await workViewPage.waitForTaskList();
|
||||
|
||||
// 1. Create the task and make it recurring (daily) starting May 4, 2026.
|
||||
await workViewPage.addTask(taskTitle);
|
||||
const task = taskPage.getTaskByText(taskTitle).first();
|
||||
await expect(task).toBeVisible({ timeout: 10000 });
|
||||
await taskPage.openTaskDetail(task);
|
||||
await openRecurDialog(page);
|
||||
await setStartDate(page, '04/05/2026');
|
||||
await saveDialog(page);
|
||||
|
||||
// 2. Delete the live (non-transparent) instance. After the first save the
|
||||
// task has dueDay = May 4; the Inbox project view lists it regardless of
|
||||
// due day. A plain task delete does NOT touch the repeat config's
|
||||
// lastTaskCreationDay or deletedInstanceDates.
|
||||
await page.goto('/#/project/INBOX_PROJECT/tasks');
|
||||
await page.waitForLoadState('networkidle');
|
||||
const taskRows = page.locator('task').filter({ hasText: taskTitle });
|
||||
await expect(taskRows.first()).toBeVisible({ timeout: 15000 });
|
||||
await taskRows.first().click({ button: 'right' });
|
||||
await page.locator('.mat-mdc-menu-content button.color-warn').click();
|
||||
const confirmBtn = page.locator('[e2e="confirmBtn"]');
|
||||
await expect(confirmBtn).toBeVisible({ timeout: 5000 });
|
||||
await confirmBtn.click();
|
||||
await expect(taskRows).toHaveCount(0, { timeout: 10000 });
|
||||
|
||||
// 3. Re-open the repeat config from a transparent projection and move
|
||||
// startDate to May 2 — earlier than the stale anchor (May 4).
|
||||
await page.goto('/#/planner');
|
||||
await page.waitForLoadState('networkidle');
|
||||
await openRecurDialogFromProjection(page, taskTitle);
|
||||
await setStartDate(page, '02/05/2026');
|
||||
await saveDialog(page);
|
||||
|
||||
// 4. Verify: the task now projects onto May 2, 3 and 4 — the days the stale
|
||||
// anchor used to suppress. May 5 is the control (it projected pre-fix
|
||||
// too). No live instance was recreated, so every day is a projection.
|
||||
await page.goto('/#/planner');
|
||||
await page.waitForLoadState('networkidle');
|
||||
|
||||
for (const date of [/^2\/5$/, /^3\/5$/, /^4\/5$/, /^5\/5$/]) {
|
||||
const day = page
|
||||
.locator('planner-day')
|
||||
.filter({ has: page.locator('.date', { hasText: date }) });
|
||||
await expect(
|
||||
day.locator('planner-repeat-projection').filter({ hasText: taskTitle }),
|
||||
).toHaveCount(1, { timeout: 15000 });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -589,7 +589,7 @@ test.describe('@supersync @migration SuperSync Legacy Migration Sync', () => {
|
|||
// Verify archive data via IndexedDB (archived tasks aren't visible in UI by default)
|
||||
const archiveData = await pageB.evaluate(async () => {
|
||||
return new Promise((resolve, reject) => {
|
||||
const dbRequest = indexedDB.open('SUP_OPS', 5);
|
||||
const dbRequest = indexedDB.open('SUP_OPS');
|
||||
dbRequest.onsuccess = (event) => {
|
||||
const db = (event.target as IDBOpenDBRequest).result;
|
||||
const tx = db.transaction('archive_young', 'readonly');
|
||||
|
|
|
|||
|
|
@ -729,7 +729,7 @@ test.describe('@webdav @migration WebDAV Legacy Migration Sync', () => {
|
|||
// Verify archive data via IndexedDB (archived tasks aren't visible in UI by default)
|
||||
const archiveData = await pageB.evaluate(async () => {
|
||||
return new Promise((resolve, reject) => {
|
||||
const dbRequest = indexedDB.open('SUP_OPS', 5);
|
||||
const dbRequest = indexedDB.open('SUP_OPS');
|
||||
dbRequest.onsuccess = (event) => {
|
||||
const db = (event.target as IDBOpenDBRequest).result;
|
||||
const tx = db.transaction('archive_young', 'readonly');
|
||||
|
|
|
|||
|
|
@ -46,8 +46,13 @@ export class SnackService {
|
|||
}
|
||||
}
|
||||
|
||||
private _getDefaultDuration(type: SnackParams['type']): number {
|
||||
return type === 'ERROR' || type === 'WARNING' ? 8000 : DEFAULT_SNACK_CFG.duration;
|
||||
// ERROR/WARNING snacks scale with message length so long messages stay readable.
|
||||
private _getDefaultDuration(type: SnackParams['type'], msg: unknown): number {
|
||||
if (type !== 'ERROR' && type !== 'WARNING') {
|
||||
return DEFAULT_SNACK_CFG.duration;
|
||||
}
|
||||
const length = typeof msg === 'string' ? msg.length : 0;
|
||||
return Math.min(Math.max(10000, length * 90), 30000);
|
||||
}
|
||||
|
||||
@debounce(100)
|
||||
|
|
@ -71,16 +76,18 @@ export class SnackService {
|
|||
isSpinner,
|
||||
} = params;
|
||||
|
||||
const translatedMsg = isSkipTranslate
|
||||
? msg
|
||||
: typeof (msg as unknown) === 'string' &&
|
||||
this._translateService.instant(msg, translateParams);
|
||||
|
||||
const cfg = {
|
||||
...DEFAULT_SNACK_CFG,
|
||||
duration: this._getDefaultDuration(type),
|
||||
duration: this._getDefaultDuration(type, translatedMsg),
|
||||
...config,
|
||||
data: {
|
||||
...params,
|
||||
msg: isSkipTranslate
|
||||
? msg
|
||||
: typeof (msg as unknown) === 'string' &&
|
||||
this._translateService.instant(msg, translateParams),
|
||||
msg: translatedMsg,
|
||||
},
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -1,285 +1,241 @@
|
|||
import { TestBed } from '@angular/core/testing';
|
||||
import { ClientIdService } from './client-id.service';
|
||||
import { SnackService } from '../snack/snack.service';
|
||||
import { openDB } from 'idb';
|
||||
import { OpLog } from '../log';
|
||||
import { ClientIdService } from './client-id.service';
|
||||
import {
|
||||
DB_NAME,
|
||||
DB_VERSION,
|
||||
SINGLETON_KEY,
|
||||
STORE_NAMES,
|
||||
} from '../../op-log/persistence/db-keys.const';
|
||||
import { runDbUpgrade } from '../../op-log/persistence/db-upgrade';
|
||||
|
||||
// Constants that mirror the private constants in ClientIdService
|
||||
const DB_NAME = 'pf';
|
||||
const DB_STORE_NAME = 'main';
|
||||
const DB_VERSION = 1;
|
||||
const CLIENT_ID_KEY = '__client_id_';
|
||||
// --- raw IndexedDB helpers (bypass ClientIdService to set up / inspect state) ---
|
||||
|
||||
/**
|
||||
* Helper to write a raw value directly to the 'pf' IndexedDB store,
|
||||
* bypassing ClientIdService validation so we can test recovery paths.
|
||||
*/
|
||||
const writeRawClientId = async (value: unknown): Promise<void> => {
|
||||
const db = await openDB(DB_NAME, DB_VERSION, {
|
||||
upgrade: (database) => {
|
||||
if (!database.objectStoreNames.contains(DB_STORE_NAME)) {
|
||||
database.createObjectStore(DB_STORE_NAME);
|
||||
}
|
||||
},
|
||||
const openSupOps = (): ReturnType<typeof openDB> =>
|
||||
openDB(DB_NAME, DB_VERSION, {
|
||||
upgrade: (db, oldVersion, _newVersion, tx) => runDbUpgrade(db, oldVersion, tx),
|
||||
});
|
||||
await db.put(DB_STORE_NAME, value, CLIENT_ID_KEY);
|
||||
|
||||
const seedSupOps = async (value: unknown): Promise<void> => {
|
||||
const db = await openSupOps();
|
||||
await db.put(STORE_NAMES.CLIENT_ID, value, SINGLETON_KEY);
|
||||
db.close();
|
||||
};
|
||||
|
||||
const readSupOps = async (): Promise<unknown> => {
|
||||
const db = await openSupOps();
|
||||
const value = await db.get(STORE_NAMES.CLIENT_ID, SINGLETON_KEY);
|
||||
db.close();
|
||||
return value;
|
||||
};
|
||||
|
||||
const clearSupOps = async (): Promise<void> => {
|
||||
const db = await openSupOps();
|
||||
await db.clear(STORE_NAMES.CLIENT_ID);
|
||||
db.close();
|
||||
};
|
||||
|
||||
const PF_DB_NAME = 'pf';
|
||||
const PF_STORE = 'main';
|
||||
const PF_CLIENT_ID_KEY = '__client_id_';
|
||||
const PF_LEGACY_CLIENT_ID_KEY = 'CLIENT_ID';
|
||||
|
||||
const openPf = (): ReturnType<typeof openDB> =>
|
||||
openDB(PF_DB_NAME, 1, {
|
||||
upgrade: (db) => {
|
||||
if (!db.objectStoreNames.contains(PF_STORE)) {
|
||||
db.createObjectStore(PF_STORE);
|
||||
}
|
||||
},
|
||||
});
|
||||
|
||||
const seedPf = async (key: string, value: unknown): Promise<void> => {
|
||||
const db = await openPf();
|
||||
await db.put(PF_STORE, value, key);
|
||||
db.close();
|
||||
};
|
||||
|
||||
const clearPf = async (): Promise<void> => {
|
||||
const db = await openPf();
|
||||
await db.delete(PF_STORE, PF_CLIENT_ID_KEY);
|
||||
await db.delete(PF_STORE, PF_LEGACY_CLIENT_ID_KEY);
|
||||
db.close();
|
||||
};
|
||||
|
||||
const idbError = (): DOMException => new DOMException('read failed', 'UnknownError');
|
||||
|
||||
describe('ClientIdService', () => {
|
||||
let service: ClientIdService;
|
||||
let mockSnackService: jasmine.SpyObj<SnackService>;
|
||||
|
||||
beforeEach(() => {
|
||||
mockSnackService = jasmine.createSpyObj('SnackService', ['open']);
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
providers: [ClientIdService, { provide: SnackService, useValue: mockSnackService }],
|
||||
});
|
||||
beforeEach(async () => {
|
||||
TestBed.configureTestingModule({ providers: [ClientIdService] });
|
||||
service = TestBed.inject(ClientIdService);
|
||||
await clearSupOps();
|
||||
await clearPf();
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
// Clean up IndexedDB between tests
|
||||
const db = await openDB(DB_NAME, DB_VERSION, {
|
||||
upgrade: (database) => {
|
||||
if (!database.objectStoreNames.contains(DB_STORE_NAME)) {
|
||||
database.createObjectStore(DB_STORE_NAME);
|
||||
}
|
||||
},
|
||||
});
|
||||
await db.delete(DB_STORE_NAME, CLIENT_ID_KEY);
|
||||
db.close();
|
||||
await clearSupOps();
|
||||
await clearPf();
|
||||
service.clearCache();
|
||||
});
|
||||
|
||||
describe('loadClientId()', () => {
|
||||
it('should return null when no clientId is stored', async () => {
|
||||
const result = await service.loadClientId();
|
||||
expect(result).toBeNull();
|
||||
describe('resolution from SUP_OPS', () => {
|
||||
it('returns a populated SUP_OPS id directly without opening pf', async () => {
|
||||
await seedSupOps('B_H8AR');
|
||||
const pfSpy = spyOn(service as any, '_readPf').and.callThrough();
|
||||
|
||||
expect(await service.loadClientId()).toBe('B_H8AR');
|
||||
expect(pfSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should return a valid new-format clientId (e.g. "B_H8AR")', async () => {
|
||||
await writeRawClientId('B_H8AR');
|
||||
const result = await service.loadClientId();
|
||||
expect(result).toBe('B_H8AR');
|
||||
});
|
||||
|
||||
it('should return a valid old-format clientId (10+ chars)', async () => {
|
||||
const oldFormatId = 'LongClientId123';
|
||||
await writeRawClientId(oldFormatId);
|
||||
const result = await service.loadClientId();
|
||||
expect(result).toBe(oldFormatId);
|
||||
});
|
||||
|
||||
it('should return null (not throw) for an invalid clientId format, enabling recovery (#6197)', async () => {
|
||||
// Simulate a corrupted or truncated clientId that doesn't match either format
|
||||
await writeRawClientId('BAD');
|
||||
// Must NOT throw - returning null allows caller to generate a fresh clientId
|
||||
const result = await service.loadClientId();
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('should warn the user when clientId is invalid and will be regenerated', async () => {
|
||||
await writeRawClientId('BAD');
|
||||
await service.loadClientId();
|
||||
expect(mockSnackService.open).toHaveBeenCalledWith(
|
||||
jasmine.objectContaining({ type: 'WARNING' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('should return null (not throw) for empty string clientId', async () => {
|
||||
await writeRawClientId('');
|
||||
const result = await service.loadClientId();
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('should cache a valid clientId after first load', async () => {
|
||||
await writeRawClientId('B_H8AR');
|
||||
it('caches the id after the first load', async () => {
|
||||
await seedSupOps('B_H8AR');
|
||||
const first = await service.loadClientId();
|
||||
// Delete from DB to confirm cache is used on second call
|
||||
const db = await openDB(DB_NAME, DB_VERSION);
|
||||
await db.delete(DB_STORE_NAME, CLIENT_ID_KEY);
|
||||
db.close();
|
||||
const second = await service.loadClientId();
|
||||
expect(second).toBe(first);
|
||||
// Remove the stored value — a second load must still return the cache.
|
||||
await clearSupOps();
|
||||
expect(await service.loadClientId()).toBe(first);
|
||||
});
|
||||
});
|
||||
|
||||
describe('generateNewClientId()', () => {
|
||||
it('should generate a clientId matching the new format', async () => {
|
||||
const id = await service.generateNewClientId();
|
||||
expect(/^[BEAI]_[a-zA-Z0-9]{4}$/.test(id)).toBeTrue();
|
||||
describe('one-time migration from legacy pf', () => {
|
||||
it('migrates pf.__client_id_ into SUP_OPS, unchanged', async () => {
|
||||
await seedPf(PF_CLIENT_ID_KEY, 'B_H8AR');
|
||||
|
||||
expect(await service.loadClientId()).toBe('B_H8AR');
|
||||
expect(await readSupOps()).toBe('B_H8AR');
|
||||
});
|
||||
|
||||
it('should persist the generated clientId so loadClientId() returns it', async () => {
|
||||
const generated = await service.generateNewClientId();
|
||||
it('migrates pf.CLIENT_ID when __client_id_ is absent (bridge-ordering gap)', async () => {
|
||||
await seedPf(PF_LEGACY_CLIENT_ID_KEY, 'LegacyId123456');
|
||||
|
||||
expect(await service.loadClientId()).toBe('LegacyId123456');
|
||||
expect(await readSupOps()).toBe('LegacyId123456');
|
||||
});
|
||||
|
||||
it('prefers __client_id_ over CLIENT_ID when both pf keys are valid', async () => {
|
||||
await seedPf(PF_CLIENT_ID_KEY, 'B_aaaa');
|
||||
await seedPf(PF_LEGACY_CLIENT_ID_KEY, 'LegacyId123456');
|
||||
|
||||
expect(await service.loadClientId()).toBe('B_aaaa');
|
||||
expect(await readSupOps()).toBe('B_aaaa');
|
||||
});
|
||||
|
||||
it('lets a valid pf id win over an invalid-format SUP_OPS value', async () => {
|
||||
await seedSupOps('BAD');
|
||||
await seedPf(PF_CLIENT_ID_KEY, 'B_good');
|
||||
|
||||
expect(await service.loadClientId()).toBe('B_good');
|
||||
expect(await readSupOps()).toBe('B_good');
|
||||
});
|
||||
});
|
||||
|
||||
describe('nothing stored anywhere', () => {
|
||||
it('loadClientId() resolves to null', async () => {
|
||||
expect(await service.loadClientId()).toBeNull();
|
||||
});
|
||||
|
||||
it('getOrGenerateClientId() generates and persists a fresh id', async () => {
|
||||
const id = await service.getOrGenerateClientId();
|
||||
expect(/^[BEAI]_[a-zA-Z0-9]{4}$/.test(id)).toBeTrue();
|
||||
expect(await readSupOps()).toBe(id);
|
||||
|
||||
// Persisted: a fresh service instance resolves to the same id.
|
||||
service.clearCache();
|
||||
const loaded = await service.loadClientId();
|
||||
expect(loaded).toBe(generated);
|
||||
expect(await service.loadClientId()).toBe(id);
|
||||
});
|
||||
|
||||
it('converges two concurrent getOrGenerateClientId() calls on one id', async () => {
|
||||
const [a, b] = await Promise.all([
|
||||
service.getOrGenerateClientId(),
|
||||
service.getOrGenerateClientId(),
|
||||
]);
|
||||
expect(a).toBe(b);
|
||||
expect(await readSupOps()).toBe(a);
|
||||
});
|
||||
|
||||
it('opens the SUP_OPS connection once for concurrent cold-start callers', async () => {
|
||||
// Regression guard: without the in-flight-promise dedup in _getSupOpsDb,
|
||||
// each racing caller opens — and leaks — its own SUP_OPS connection.
|
||||
const openSpy = spyOn(service as any, '_openSupOpsDb').and.callThrough();
|
||||
|
||||
await Promise.all([
|
||||
service.getOrGenerateClientId(),
|
||||
service.getOrGenerateClientId(),
|
||||
]);
|
||||
|
||||
expect(openSpy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
// The data-safety core: a transient IndexedDB read failure must NEVER mint a
|
||||
// brand new clientId — that would orphan the device's real identity (#7732).
|
||||
describe('IndexedDB read failures never generate a new id', () => {
|
||||
it('SUP_OPS read throws: loadClientId() -> null, getOrGenerateClientId() throws', async () => {
|
||||
spyOn(service as any, '_getSupOpsDb').and.returnValue(
|
||||
Promise.resolve({ get: () => Promise.reject(idbError()) }),
|
||||
);
|
||||
|
||||
expect(await service.loadClientId()).toBeNull();
|
||||
service.clearCache();
|
||||
await expectAsync(service.getOrGenerateClientId()).toBeRejected();
|
||||
// Nothing was minted into SUP_OPS.
|
||||
expect(await readSupOps()).toBeUndefined();
|
||||
});
|
||||
|
||||
it('pf read throws: loadClientId() -> null, getOrGenerateClientId() throws', async () => {
|
||||
spyOn(service as any, '_readPf').and.returnValue(Promise.reject(idbError()));
|
||||
|
||||
expect(await service.loadClientId()).toBeNull();
|
||||
service.clearCache();
|
||||
await expectAsync(service.getOrGenerateClientId()).toBeRejected();
|
||||
expect(await readSupOps()).toBeUndefined();
|
||||
});
|
||||
|
||||
it('copy-forward write fails: returns the valid pf id, no throw, no generation', async () => {
|
||||
await seedPf(PF_CLIENT_ID_KEY, 'B_good');
|
||||
spyOn(service as any, '_putClientIdIfAbsent').and.returnValue(
|
||||
Promise.reject(new DOMException('quota', 'QuotaExceededError')),
|
||||
);
|
||||
|
||||
expect(await service.loadClientId()).toBe('B_good');
|
||||
service.clearCache();
|
||||
expect(await service.getOrGenerateClientId()).toBe('B_good');
|
||||
// The failed copy means SUP_OPS stays empty — a later launch retries it.
|
||||
expect(await readSupOps()).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('getOrGenerateClientId()', () => {
|
||||
it('should return existing valid clientId', async () => {
|
||||
await writeRawClientId('B_H8AR');
|
||||
const result = await service.getOrGenerateClientId();
|
||||
expect(result).toBe('B_H8AR');
|
||||
it('returns an existing valid SUP_OPS id without generating', async () => {
|
||||
await seedSupOps('B_H8AR');
|
||||
expect(await service.getOrGenerateClientId()).toBe('B_H8AR');
|
||||
});
|
||||
|
||||
it('should generate and persist a new clientId when none is stored', async () => {
|
||||
const result = await service.getOrGenerateClientId();
|
||||
expect(/^[BEAI]_[a-zA-Z0-9]{4}$/.test(result)).toBeTrue();
|
||||
// Verify it was persisted: clear cache and reload
|
||||
service.clearCache();
|
||||
const loaded = await service.loadClientId();
|
||||
expect(loaded).toBe(result);
|
||||
});
|
||||
|
||||
it('should generate a new clientId when stored value is invalid', async () => {
|
||||
await writeRawClientId('BAD');
|
||||
const result = await service.getOrGenerateClientId();
|
||||
expect(/^[BEAI]_[a-zA-Z0-9]{4}$/.test(result)).toBeTrue();
|
||||
it('generates when the stored value is an invalid format', async () => {
|
||||
await seedSupOps('BAD');
|
||||
const id = await service.getOrGenerateClientId();
|
||||
expect(/^[BEAI]_[a-zA-Z0-9]{4}$/.test(id)).toBeTrue();
|
||||
});
|
||||
});
|
||||
|
||||
describe('persistClientId()', () => {
|
||||
it('should persist a valid new-format clientId', async () => {
|
||||
it('writes the id into SUP_OPS and sets the cache', async () => {
|
||||
await service.persistClientId('E_abcd');
|
||||
service.clearCache();
|
||||
const loaded = await service.loadClientId();
|
||||
expect(loaded).toBe('E_abcd');
|
||||
expect(await readSupOps()).toBe('E_abcd');
|
||||
// Cache is set — loadClientId() returns it without re-reading.
|
||||
expect(await service.loadClientId()).toBe('E_abcd');
|
||||
});
|
||||
|
||||
it('should persist a valid old-format clientId', async () => {
|
||||
const oldId = 'OldFormatClientId1';
|
||||
await service.persistClientId(oldId);
|
||||
service.clearCache();
|
||||
const loaded = await service.loadClientId();
|
||||
expect(loaded).toBe(oldId);
|
||||
it('writes unconditionally, overwriting an existing SUP_OPS id', async () => {
|
||||
await seedSupOps('B_old1');
|
||||
await service.persistClientId('LegacyId123456');
|
||||
expect(await readSupOps()).toBe('LegacyId123456');
|
||||
});
|
||||
|
||||
it('should throw for invalid format without persisting', async () => {
|
||||
await expectAsync(service.persistClientId('INVALID')).toBeRejected();
|
||||
});
|
||||
});
|
||||
|
||||
describe('withRotation()', () => {
|
||||
it('should call fn with a fresh clientId and return its value', async () => {
|
||||
await service.persistClientId('B_Prio');
|
||||
service.clearCache();
|
||||
|
||||
const result = await service.withRotation('[Test]', async (newId) => {
|
||||
expect(newId).not.toBe('B_Prio');
|
||||
return { ok: true, id: newId };
|
||||
});
|
||||
|
||||
expect(result.ok).toBe(true);
|
||||
const persisted = await service.loadClientId();
|
||||
expect(persisted).toBe(result.id);
|
||||
});
|
||||
|
||||
it('should restore the prior clientId when fn throws', async () => {
|
||||
await service.persistClientId('B_Prio');
|
||||
service.clearCache();
|
||||
|
||||
await expectAsync(
|
||||
service.withRotation('[Test]', async () => {
|
||||
throw new Error('work failed');
|
||||
}),
|
||||
).toBeRejectedWith(jasmine.objectContaining({ message: 'work failed' }));
|
||||
|
||||
service.clearCache();
|
||||
expect(await service.loadClientId()).toBe('B_Prio');
|
||||
});
|
||||
|
||||
it('should restore the persisted prior clientId even when the cache is stale', async () => {
|
||||
await service.persistClientId('B_Cach');
|
||||
await writeRawClientId('B_Fres');
|
||||
|
||||
await expectAsync(
|
||||
service.withRotation('[Test]', async () => {
|
||||
throw new Error('work failed');
|
||||
}),
|
||||
).toBeRejectedWith(jasmine.objectContaining({ message: 'work failed' }));
|
||||
|
||||
service.clearCache();
|
||||
expect(await service.loadClientId()).toBe('B_Fres');
|
||||
});
|
||||
|
||||
it('should not roll back over a newer persisted clientId from another context', async () => {
|
||||
await service.persistClientId('B_Prio');
|
||||
service.clearCache();
|
||||
|
||||
await expectAsync(
|
||||
service.withRotation('[Test]', async () => {
|
||||
await writeRawClientId('B_Othr');
|
||||
throw new Error('work failed');
|
||||
}),
|
||||
).toBeRejectedWith(jasmine.objectContaining({ message: 'work failed' }));
|
||||
|
||||
service.clearCache();
|
||||
expect(await service.loadClientId()).toBe('B_Othr');
|
||||
});
|
||||
|
||||
it('should leave the rotated clientId in place when there was no prior id', async () => {
|
||||
// Wholly fresh device — `pf` is empty.
|
||||
expect(await service.loadClientId()).toBeNull();
|
||||
|
||||
await expectAsync(
|
||||
service.withRotation('[Test]', async () => {
|
||||
throw new Error('work failed');
|
||||
}),
|
||||
).toBeRejectedWith(jasmine.objectContaining({ message: 'work failed' }));
|
||||
|
||||
service.clearCache();
|
||||
const persisted = await service.loadClientId();
|
||||
expect(persisted).not.toBeNull();
|
||||
});
|
||||
|
||||
it('should propagate the original fn error when rollback also fails', async () => {
|
||||
await service.persistClientId('B_Prio');
|
||||
service.clearCache();
|
||||
spyOn(service as any, '_restorePriorClientIdIfCurrentMatches').and.rejectWith(
|
||||
new Error('pf write also broken'),
|
||||
);
|
||||
|
||||
await expectAsync(
|
||||
service.withRotation('[Test]', async () => {
|
||||
throw new Error('work failed');
|
||||
}),
|
||||
).toBeRejectedWith(jasmine.objectContaining({ message: 'work failed' }));
|
||||
});
|
||||
|
||||
it('should not log clientIds or raw error messages when rollback fails', async () => {
|
||||
await service.persistClientId('B_Prio');
|
||||
service.clearCache();
|
||||
spyOn(service as any, '_restorePriorClientIdIfCurrentMatches').and.rejectWith(
|
||||
new Error('rollback failed with B_Prio'),
|
||||
);
|
||||
const opLogSpy = spyOn(OpLog, 'critical');
|
||||
|
||||
await expectAsync(
|
||||
service.withRotation('[Test]', async () => {
|
||||
throw new Error('work failed with B_Prio');
|
||||
}),
|
||||
).toBeRejectedWith(
|
||||
jasmine.objectContaining({ message: 'work failed with B_Prio' }),
|
||||
);
|
||||
|
||||
expect(opLogSpy).toHaveBeenCalled();
|
||||
const payload = opLogSpy.calls.mostRecent().args[1] as Record<string, unknown>;
|
||||
const serializedPayload = JSON.stringify(payload);
|
||||
expect(payload).toEqual(
|
||||
jasmine.objectContaining({
|
||||
hadPriorClientId: true,
|
||||
originalErrorName: 'Error',
|
||||
rollbackErrorName: 'Error',
|
||||
}),
|
||||
);
|
||||
expect(serializedPayload).not.toContain('B_Prio');
|
||||
expect(serializedPayload).not.toContain('work failed');
|
||||
expect(serializedPayload).not.toContain('rollback failed');
|
||||
it('rejects an invalid-format id without persisting', async () => {
|
||||
await expectAsync(service.persistClientId('BAD')).toBeRejected();
|
||||
expect(await readSupOps()).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,287 +1,274 @@
|
|||
import { Injectable, inject } from '@angular/core';
|
||||
import { openDB, IDBPDatabase } from 'idb';
|
||||
import { Injectable } from '@angular/core';
|
||||
import { IDBPDatabase, openDB } from 'idb';
|
||||
import { OpLog } from '../log';
|
||||
import { SnackService } from '../snack/snack.service';
|
||||
import { T } from '../../t.const';
|
||||
import { generateClientId, isValidClientIdFormat } from './generate-client-id';
|
||||
import {
|
||||
DB_NAME as SUP_OPS_DB_NAME,
|
||||
DB_VERSION as SUP_OPS_DB_VERSION,
|
||||
SINGLETON_KEY,
|
||||
STORE_NAMES,
|
||||
} from '../../op-log/persistence/db-keys.const';
|
||||
import { runDbUpgrade } from '../../op-log/persistence/db-upgrade';
|
||||
|
||||
// Database constants - must match PFAPI's storage
|
||||
const DB_NAME = 'pf';
|
||||
const DB_STORE_NAME = 'main';
|
||||
const DB_VERSION = 1;
|
||||
const CLIENT_ID_KEY = '__client_id_';
|
||||
// Legacy 'pf' database — a read-only, one-time migration source for the
|
||||
// clientId. Never written or deleted by this service. See issue #7732.
|
||||
const PF_DB_NAME = 'pf';
|
||||
const PF_DB_STORE_NAME = 'main';
|
||||
const PF_DB_VERSION = 1;
|
||||
/**
|
||||
* Key ClientIdService has always operated on in `pf`. On an op-log-era device
|
||||
* this is the live identity.
|
||||
*/
|
||||
const PF_CLIENT_ID_KEY = '__client_id_';
|
||||
/**
|
||||
* The original PFAPI key. Read only as a fallback to seed a legacy profile
|
||||
* that never received a `__client_id_` entry.
|
||||
*/
|
||||
const PF_LEGACY_CLIENT_ID_KEY = 'CLIENT_ID';
|
||||
|
||||
/**
|
||||
* Service for managing the sync client ID.
|
||||
* Service for managing the sync client ID — the device's stable sync identity.
|
||||
*
|
||||
* Reads/writes directly to IndexedDB to preserve existing client IDs
|
||||
* and avoid dependency on PfapiService.
|
||||
* The clientId lives in the `SUP_OPS` database (`client_id` store, schema v6).
|
||||
* Storing it there lets destructive flows (clean-slate, backup-restore) rotate
|
||||
* it atomically inside runDestructiveStateReplacement's transaction, instead of
|
||||
* a hand-rolled cross-database two-phase commit (issue #7732).
|
||||
*
|
||||
* Uses the same database name ('pf') and key ('__client_id_') as the
|
||||
* legacy MetaModelCtrl to ensure backward compatibility.
|
||||
* The legacy `pf` database is a read-only, one-time migration source: the first
|
||||
* read on a device whose clientId still lives only in `pf` copies it forward
|
||||
* into `SUP_OPS`. The clientId is non-regenerable (it keys the vector clock),
|
||||
* so the resolver propagates IndexedDB read errors rather than risk minting a
|
||||
* fresh id over a transient failure.
|
||||
*/
|
||||
@Injectable({
|
||||
providedIn: 'root',
|
||||
})
|
||||
export class ClientIdService {
|
||||
private _snackService = inject(SnackService, { optional: true });
|
||||
private _db: IDBPDatabase | null = null;
|
||||
private _supOpsDb: IDBPDatabase | null = null;
|
||||
private _supOpsDbPromise: Promise<IDBPDatabase> | null = null;
|
||||
private _cachedClientId: string | null = null;
|
||||
|
||||
/**
|
||||
* Loads the client ID.
|
||||
*
|
||||
* Uses caching to avoid repeated IndexedDB reads. Rotation paths bypass or
|
||||
* refresh the cache when they need cross-context freshness.
|
||||
*
|
||||
* @returns The client ID, or null if not yet generated
|
||||
* Loads the client ID. Never throws — returns null on absence OR on a
|
||||
* transient IndexedDB read failure. Callers (hydrator, sync readers) treat
|
||||
* a null clientId as a tolerable, retryable condition.
|
||||
*/
|
||||
async loadClientId(): Promise<string | null> {
|
||||
if (this._cachedClientId) {
|
||||
return this._cachedClientId;
|
||||
}
|
||||
|
||||
const clientId = await this._readPersistedValidClientId({ warnOnInvalid: true });
|
||||
|
||||
if (clientId) {
|
||||
this._cachedClientId = clientId;
|
||||
OpLog.normal('ClientIdService.loadClientId() loaded');
|
||||
try {
|
||||
const id = await this._resolve();
|
||||
if (id) {
|
||||
this._cachedClientId = id;
|
||||
}
|
||||
return id;
|
||||
} catch {
|
||||
// Read failure — never throw, never generate. Returning null lets the
|
||||
// caller retry on a later launch without orphaning the real clientId.
|
||||
return null;
|
||||
}
|
||||
return clientId;
|
||||
}
|
||||
|
||||
/**
|
||||
* Generates a new client ID and saves it.
|
||||
* Returns the existing client ID, or generates and persists a new one if
|
||||
* none exists. The preferred entry point for callers that always need an ID.
|
||||
*
|
||||
* Format: {platform}_{4-char-base62}
|
||||
* Examples: "B_a7Kx", "E_m2Pq", "A_x9Yz"
|
||||
*
|
||||
* @returns The newly generated client ID
|
||||
* Propagates IndexedDB read failures: if a read throws, this throws too — it
|
||||
* does NOT generate. Generating on a transient failure would mint a brand
|
||||
* new clientId that orphans the device's real, history-bearing identity (the
|
||||
* non-regenerable loss issue #7732 exists to prevent). Generation happens
|
||||
* only after reads succeed and confirm no id exists anywhere.
|
||||
*/
|
||||
async generateNewClientId(): Promise<string> {
|
||||
const newClientId = this._generateClientId();
|
||||
|
||||
const db = await this._getDb();
|
||||
await db.put(DB_STORE_NAME, newClientId, CLIENT_ID_KEY);
|
||||
|
||||
this._cachedClientId = newClientId;
|
||||
OpLog.normal('ClientIdService.generateNewClientId() generated');
|
||||
return newClientId;
|
||||
async getOrGenerateClientId(): Promise<string> {
|
||||
if (this._cachedClientId) {
|
||||
return this._cachedClientId;
|
||||
}
|
||||
// PROPAGATES read failures — does not swallow, does not generate on error.
|
||||
const existing = await this._resolve();
|
||||
if (existing) {
|
||||
this._cachedClientId = existing;
|
||||
return existing;
|
||||
}
|
||||
// Reads succeeded and confirmed empty everywhere — safe to generate.
|
||||
const id = await this._putClientIdIfAbsent(generateClientId);
|
||||
this._cachedClientId = id;
|
||||
OpLog.normal('ClientIdService.getOrGenerateClientId() generated');
|
||||
return id;
|
||||
}
|
||||
|
||||
/**
|
||||
* Persists an existing client ID (e.g., from legacy migration).
|
||||
* Persists an existing client ID (legacy-migration genesis seed).
|
||||
*
|
||||
* Validates the format before writing to prevent invalid IDs from
|
||||
* being stored. loadClientId() treats an invalid stored ID as missing
|
||||
* and returns null, causing the caller to regenerate a fresh clientId.
|
||||
* Writes UNCONDITIONALLY (not a CAS) into SUP_OPS: it carries the
|
||||
* authoritative legacy PFAPI `CLIENT_ID` value that OperationLogMigration's
|
||||
* genesis op is built from, and must win over any `__client_id_`-derived
|
||||
* migration copy so the genesis op stays consistent with its vectorClock.
|
||||
*/
|
||||
async persistClientId(clientId: string): Promise<void> {
|
||||
if (!this._isValidClientIdFormat(clientId)) {
|
||||
throw new Error(`Cannot persist invalid clientId: ${clientId}`);
|
||||
if (!isValidClientIdFormat(clientId)) {
|
||||
// The clientId value is sensitive (it keys the vector clock) and log
|
||||
// history is user-exportable — never interpolate it into the message.
|
||||
throw new Error('Cannot persist invalid clientId');
|
||||
}
|
||||
const db = await this._getDb();
|
||||
await db.put(DB_STORE_NAME, clientId, CLIENT_ID_KEY);
|
||||
const db = await this._getSupOpsDb();
|
||||
await db.put(STORE_NAMES.CLIENT_ID, clientId, SINGLETON_KEY);
|
||||
this._cachedClientId = clientId;
|
||||
OpLog.normal('ClientIdService.persistClientId() persisted');
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns the existing client ID, or generates and persists a new one if
|
||||
* none is stored or the stored value is invalid.
|
||||
*
|
||||
* This is the preferred entry point for callers that always need a valid ID.
|
||||
*/
|
||||
async getOrGenerateClientId(): Promise<string> {
|
||||
return (await this.loadClientId()) ?? (await this.generateNewClientId());
|
||||
}
|
||||
|
||||
/**
|
||||
* Clears the cached client ID.
|
||||
*
|
||||
* Used for testing or when the client ID storage needs to be re-read.
|
||||
* Invalidates the cached client ID so the next read re-resolves from
|
||||
* IndexedDB. A documented production method (not just a test helper):
|
||||
* runDestructiveStateReplacement calls it after rotating the clientId.
|
||||
*/
|
||||
clearCache(): void {
|
||||
this._cachedClientId = null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Rotate the clientId for the duration of `fn`. Captures the prior id,
|
||||
* generates and persists a new one, runs `fn(newClientId)`. If `fn` throws,
|
||||
* the prior id is restored so the `pf` database stays consistent with any
|
||||
* caller-side state that didn't get updated.
|
||||
* Resolves "what is this device's clientId", migrating it forward from the
|
||||
* legacy `pf` database if needed.
|
||||
*
|
||||
* Edge case: if there was no prior clientId (wholly fresh device), the new
|
||||
* id is intentionally left in `pf` on failure — there is nothing to restore
|
||||
* to. If the restore itself throws, the original `fn` error is propagated
|
||||
* and the restore failure is logged at critical level for forensics.
|
||||
*
|
||||
* `logPrefix` is used to tag the critical-log entry on rollback failure so
|
||||
* forensics can attribute it to the caller (e.g. `'[CleanSlate]'`).
|
||||
* Read failures propagate (the caller decides whether to swallow). Only a
|
||||
* failed copy-forward is swallowed — the `pf` id is still valid and a later
|
||||
* launch retries the copy.
|
||||
*/
|
||||
async withRotation<T>(
|
||||
logPrefix: string,
|
||||
fn: (newClientId: string) => Promise<T>,
|
||||
): Promise<T> {
|
||||
const priorClientId = await this._readPersistedValidClientId();
|
||||
const newClientId = await this.generateNewClientId();
|
||||
private async _resolve(): Promise<string | null> {
|
||||
const fromOps = await this._readSupOps(); // throws on IndexedDB read error
|
||||
if (fromOps) {
|
||||
return fromOps;
|
||||
}
|
||||
const fromPf = await this._readPf(); // throws on IndexedDB read error
|
||||
if (!fromPf) {
|
||||
// Both reads succeeded -> confirmed: no id stored anywhere.
|
||||
return null;
|
||||
}
|
||||
try {
|
||||
return await fn(newClientId);
|
||||
} catch (e) {
|
||||
if (priorClientId) {
|
||||
try {
|
||||
await this._restorePriorClientIdIfCurrentMatches(priorClientId, newClientId);
|
||||
} catch (rollbackErr) {
|
||||
OpLog.critical(
|
||||
`${logPrefix} Failed to roll back clientId rotation after failure`,
|
||||
{
|
||||
hadPriorClientId: true,
|
||||
originalErrorName: this._errorName(e),
|
||||
rollbackErrorName: this._errorName(rollbackErr),
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
throw e;
|
||||
return await this._putClientIdIfAbsent(() => fromPf);
|
||||
} catch {
|
||||
// Copy-forward to SUP_OPS failed (quota, closed connection). The `pf` id
|
||||
// is valid — return it and let a later launch retry the copy. Worst case
|
||||
// is a redundant copy, never a lost identity.
|
||||
return fromPf;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns true if the clientId matches a known valid format.
|
||||
* Old format: any string of length >= 10 (legacy IDs).
|
||||
* New format: {platform}_{4-char-base62} e.g. "B_a7Kx".
|
||||
* Reads the clientId from `SUP_OPS.client_id`. An invalid format is treated
|
||||
* as absent (returns null — never throws on bad format; see issue #6197).
|
||||
* IndexedDB *errors* propagate.
|
||||
*/
|
||||
private _isValidClientIdFormat(clientId: string): boolean {
|
||||
return clientId.length >= 10 || /^[BEAI]_[a-zA-Z0-9]{4}$/.test(clientId);
|
||||
}
|
||||
|
||||
private async _readPersistedValidClientId(
|
||||
options: { warnOnInvalid?: boolean } = {},
|
||||
): Promise<string | null> {
|
||||
const db = await this._getDb();
|
||||
const clientId = await db.get(DB_STORE_NAME, CLIENT_ID_KEY);
|
||||
|
||||
if (typeof clientId !== 'string') {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (!this._isValidClientIdFormat(clientId)) {
|
||||
if (options.warnOnInvalid) {
|
||||
// Unrecognized format — log but treat as missing rather than throwing.
|
||||
// Throwing here permanently blocks sync (issue #6197: "Invalid clientId loaded: B_H8AR").
|
||||
// Returning null causes the caller to generate a fresh clientId, which unblocks sync.
|
||||
// Length only — the literal clientId value is sensitive (vector-clock
|
||||
// key) and log history is user-exportable (CLAUDE.md sync rule 9).
|
||||
OpLog.critical(
|
||||
'ClientIdService.loadClientId() Invalid clientId format, will regenerate:',
|
||||
{
|
||||
length: clientId.length,
|
||||
},
|
||||
);
|
||||
this._snackService?.open({
|
||||
msg: T.F.SYNC.S.WARN_CLIENT_ID_REGENERATED,
|
||||
type: 'WARNING',
|
||||
});
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
return clientId;
|
||||
}
|
||||
|
||||
private async _restorePriorClientIdIfCurrentMatches(
|
||||
priorClientId: string,
|
||||
expectedCurrentClientId: string,
|
||||
): Promise<void> {
|
||||
const db = await this._getDb();
|
||||
const tx = db.transaction(DB_STORE_NAME, 'readwrite');
|
||||
const store = tx.objectStore(DB_STORE_NAME);
|
||||
const currentClientId = await store.get(CLIENT_ID_KEY);
|
||||
|
||||
if (currentClientId === expectedCurrentClientId) {
|
||||
await store.put(priorClientId, CLIENT_ID_KEY);
|
||||
await tx.done;
|
||||
this._cachedClientId = priorClientId;
|
||||
return;
|
||||
}
|
||||
|
||||
await tx.done;
|
||||
this._cachedClientId =
|
||||
typeof currentClientId === 'string' && this._isValidClientIdFormat(currentClientId)
|
||||
? currentClientId
|
||||
: null;
|
||||
}
|
||||
|
||||
private _errorName(error: unknown): string {
|
||||
return error instanceof Error ? error.name : typeof error;
|
||||
private async _readSupOps(): Promise<string | null> {
|
||||
const db = await this._getSupOpsDb();
|
||||
const raw = await db.get(STORE_NAMES.CLIENT_ID, SINGLETON_KEY);
|
||||
return isValidClientIdFormat(raw) ? raw : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets or opens the IndexedDB database.
|
||||
* Reads the clientId from the legacy `pf` database, read-only, per-call.
|
||||
*
|
||||
* Routed directly through `openDB` rather than LegacyPfDbService because that
|
||||
* service's load() swallows IndexedDB errors and returns null — which makes
|
||||
* "key absent" indistinguishable from "read failed". This service needs that
|
||||
* distinction: a read failure must propagate (it must never generate over a
|
||||
* transient failure).
|
||||
*
|
||||
* `__client_id_` (the live identity on op-log-era devices) wins over the
|
||||
* original PFAPI `CLIENT_ID` key. IndexedDB *errors* propagate.
|
||||
*/
|
||||
private async _getDb(): Promise<IDBPDatabase> {
|
||||
if (this._db) {
|
||||
return this._db;
|
||||
}
|
||||
|
||||
this._db = await openDB(DB_NAME, DB_VERSION, {
|
||||
upgrade: (db) => {
|
||||
if (!db.objectStoreNames.contains(DB_STORE_NAME)) {
|
||||
db.createObjectStore(DB_STORE_NAME);
|
||||
private async _readPf(): Promise<string | null> {
|
||||
const db = await openDB(PF_DB_NAME, PF_DB_VERSION, {
|
||||
upgrade: (database) => {
|
||||
if (!database.objectStoreNames.contains(PF_DB_STORE_NAME)) {
|
||||
database.createObjectStore(PF_DB_STORE_NAME);
|
||||
}
|
||||
},
|
||||
});
|
||||
|
||||
return this._db;
|
||||
try {
|
||||
const live = await db.get(PF_DB_STORE_NAME, PF_CLIENT_ID_KEY);
|
||||
if (isValidClientIdFormat(live)) {
|
||||
return live;
|
||||
}
|
||||
const legacy = await db.get(PF_DB_STORE_NAME, PF_LEGACY_CLIENT_ID_KEY);
|
||||
return isValidClientIdFormat(legacy) ? legacy : null;
|
||||
} finally {
|
||||
db.close();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Generates a compact 6-char client ID.
|
||||
* Format: {platform}_{4-char-base62-random}
|
||||
* Establish-if-absent writer: writes `factory()` into SUP_OPS.client_id only
|
||||
* if no valid id is already there, in a single transaction.
|
||||
*
|
||||
* MULTI-TAB / ROTATION GUARD: the in-tx re-check is load-bearing. IndexedDB
|
||||
* serializes same-store transactions across same-origin connections, so an
|
||||
* id that committed first (another tab's generate, or a destructive
|
||||
* rotation) is observed by `raced` and WINS — this helper never clobbers it.
|
||||
*
|
||||
* persistClientId and runDestructiveStateReplacement are the *unconditional*
|
||||
* writers (they know the exact intended value); this is the conditional one.
|
||||
*/
|
||||
private _generateClientId(): string {
|
||||
const prefix = this._getEnvironmentId();
|
||||
const randomPart = this._generateBase62(4);
|
||||
return `${prefix}_${randomPart}`;
|
||||
private async _putClientIdIfAbsent(factory: () => string): Promise<string> {
|
||||
const db = await this._getSupOpsDb();
|
||||
const tx = db.transaction(STORE_NAMES.CLIENT_ID, 'readwrite');
|
||||
const raced = await tx.store.get(SINGLETON_KEY);
|
||||
// A valid id already there (another tab, a rotation) wins — never clobbered.
|
||||
const resolved = isValidClientIdFormat(raced) ? raced : factory();
|
||||
if (resolved !== raced) {
|
||||
await tx.store.put(resolved, SINGLETON_KEY);
|
||||
}
|
||||
await tx.done;
|
||||
return resolved;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns a single-character platform identifier for compact client IDs.
|
||||
* B = Browser, E = Electron, A = Android, I = iOS
|
||||
* Opens (and caches) an independent connection to the `SUP_OPS` database.
|
||||
*
|
||||
* Independent — not delegated to OperationLogStoreService — because that
|
||||
* service injects CLIENT_ID_PROVIDER (-> this service), so delegating back
|
||||
* would form a DI cycle. Two same-origin connections to one store are safe:
|
||||
* IndexedDB serializes transactions across them. Collapsing onto a single
|
||||
* shared connection (by breaking that DI cycle) is tracked in #7735.
|
||||
*
|
||||
* Concurrent first callers share one in-flight open via `_supOpsDbPromise`
|
||||
* (mirrors OperationLogStoreService._ensureInit): without it each racing
|
||||
* caller would open — and leak — its own connection. The in-flight promise
|
||||
* is cleared on open failure so the next call retries, and in the
|
||||
* close/versionchange handlers so a stale handle is never re-handed-out.
|
||||
*/
|
||||
private _getEnvironmentId(): string {
|
||||
// Detect Electron
|
||||
const isElectron =
|
||||
typeof process !== 'undefined' && (process as any).versions?.electron;
|
||||
if (isElectron) {
|
||||
return 'E';
|
||||
private async _getSupOpsDb(): Promise<IDBPDatabase> {
|
||||
if (this._supOpsDb) {
|
||||
return this._supOpsDb;
|
||||
}
|
||||
|
||||
// Detect Android WebView
|
||||
if (/Android/.test(navigator.userAgent) && /wv/.test(navigator.userAgent)) {
|
||||
return 'A';
|
||||
if (!this._supOpsDbPromise) {
|
||||
this._supOpsDbPromise = this._openSupOpsDb().catch((e) => {
|
||||
// A failed open must be retryable — clear so the next call reopens.
|
||||
this._supOpsDbPromise = null;
|
||||
throw e;
|
||||
});
|
||||
}
|
||||
|
||||
// Detect iOS
|
||||
if (
|
||||
navigator.userAgent.includes('iOS') ||
|
||||
navigator.userAgent.includes('iPhone') ||
|
||||
navigator.userAgent.includes('iPad')
|
||||
) {
|
||||
return 'I';
|
||||
}
|
||||
|
||||
// Default: Browser
|
||||
return 'B';
|
||||
return this._supOpsDbPromise;
|
||||
}
|
||||
|
||||
/**
|
||||
* Generates a random base62 string of the specified length.
|
||||
* Uses crypto.getRandomValues() for non-predictable randomness.
|
||||
*/
|
||||
private _generateBase62(length: number): string {
|
||||
const chars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
|
||||
const bytes = new Uint8Array(length);
|
||||
crypto.getRandomValues(bytes);
|
||||
return Array.from(bytes, (b) => chars[b % chars.length]).join('');
|
||||
private async _openSupOpsDb(): Promise<IDBPDatabase> {
|
||||
const db = await openDB(SUP_OPS_DB_NAME, SUP_OPS_DB_VERSION, {
|
||||
upgrade: (database, oldVersion, _newVersion, transaction) => {
|
||||
runDbUpgrade(database, oldVersion, transaction);
|
||||
},
|
||||
});
|
||||
// Browser closed the connection — drop both handles, reopen on next access.
|
||||
db.addEventListener('close', () => {
|
||||
this._supOpsDb = null;
|
||||
this._supOpsDbPromise = null;
|
||||
});
|
||||
// Don't block a future (v7) upgrade opened by another connection/tab.
|
||||
db.addEventListener('versionchange', () => {
|
||||
db.close();
|
||||
this._supOpsDb = null;
|
||||
this._supOpsDbPromise = null;
|
||||
});
|
||||
this._supOpsDb = db;
|
||||
return db;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
48
src/app/core/util/generate-client-id.spec.ts
Normal file
48
src/app/core/util/generate-client-id.spec.ts
Normal file
|
|
@ -0,0 +1,48 @@
|
|||
import { generateClientId, isValidClientIdFormat } from './generate-client-id';
|
||||
|
||||
describe('generate-client-id', () => {
|
||||
describe('generateClientId()', () => {
|
||||
it('produces an id matching the new {platform}_{4-char} format', () => {
|
||||
expect(/^[BEAI]_[a-zA-Z0-9]{4}$/.test(generateClientId())).toBeTrue();
|
||||
});
|
||||
|
||||
it('produces a distinct id on each call', () => {
|
||||
// 50 random 4-char base62 ids — a collision is astronomically unlikely.
|
||||
const ids = new Set(Array.from({ length: 50 }, () => generateClientId()));
|
||||
expect(ids.size).toBe(50);
|
||||
});
|
||||
|
||||
it('always passes its own format guard', () => {
|
||||
for (let i = 0; i < 20; i++) {
|
||||
expect(isValidClientIdFormat(generateClientId())).toBeTrue();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('isValidClientIdFormat()', () => {
|
||||
it('accepts the new compact format', () => {
|
||||
expect(isValidClientIdFormat('B_a7Kx')).toBeTrue();
|
||||
expect(isValidClientIdFormat('E_0000')).toBeTrue();
|
||||
expect(isValidClientIdFormat('I_ZzZz')).toBeTrue();
|
||||
});
|
||||
|
||||
it('accepts legacy ids of length >= 10', () => {
|
||||
expect(isValidClientIdFormat('LongClientId123')).toBeTrue();
|
||||
expect(isValidClientIdFormat('0123456789')).toBeTrue();
|
||||
});
|
||||
|
||||
it('rejects short, non-conforming strings', () => {
|
||||
expect(isValidClientIdFormat('BAD')).toBeFalse();
|
||||
expect(isValidClientIdFormat('')).toBeFalse();
|
||||
expect(isValidClientIdFormat('B_a7K')).toBeFalse(); // 3-char suffix
|
||||
expect(isValidClientIdFormat('X_a7Kx')).toBeFalse(); // unknown platform
|
||||
});
|
||||
|
||||
it('rejects non-string values', () => {
|
||||
expect(isValidClientIdFormat(undefined)).toBeFalse();
|
||||
expect(isValidClientIdFormat(null)).toBeFalse();
|
||||
expect(isValidClientIdFormat(42)).toBeFalse();
|
||||
expect(isValidClientIdFormat({})).toBeFalse();
|
||||
});
|
||||
});
|
||||
});
|
||||
73
src/app/core/util/generate-client-id.ts
Normal file
73
src/app/core/util/generate-client-id.ts
Normal file
|
|
@ -0,0 +1,73 @@
|
|||
/**
|
||||
* Pure client-ID generation and format validation.
|
||||
*
|
||||
* Extracted from ClientIdService so destructive-flow callers (clean-slate,
|
||||
* backup-restore) can mint an id without going through the stateful service —
|
||||
* the new id is persisted only inside the atomic SUP_OPS transaction in
|
||||
* OperationLogStoreService.runDestructiveStateReplacement. See issue #7732.
|
||||
*
|
||||
* No DI, no I/O — directly unit-testable.
|
||||
*/
|
||||
|
||||
/**
|
||||
* Returns a single-character platform identifier for compact client IDs.
|
||||
* B = Browser, E = Electron, A = Android, I = iOS.
|
||||
*/
|
||||
const _getEnvironmentId = (): string => {
|
||||
// Detect Electron
|
||||
const isElectron =
|
||||
typeof process !== 'undefined' &&
|
||||
!!(process as { versions?: { electron?: string } }).versions?.electron;
|
||||
if (isElectron) {
|
||||
return 'E';
|
||||
}
|
||||
|
||||
// Detect Android WebView
|
||||
if (/Android/.test(navigator.userAgent) && /wv/.test(navigator.userAgent)) {
|
||||
return 'A';
|
||||
}
|
||||
|
||||
// Detect iOS
|
||||
if (
|
||||
navigator.userAgent.includes('iOS') ||
|
||||
navigator.userAgent.includes('iPhone') ||
|
||||
navigator.userAgent.includes('iPad')
|
||||
) {
|
||||
return 'I';
|
||||
}
|
||||
|
||||
// Default: Browser
|
||||
return 'B';
|
||||
};
|
||||
|
||||
/**
|
||||
* Generates a random base62 string of the specified length.
|
||||
* Uses crypto.getRandomValues() for non-predictable randomness.
|
||||
*/
|
||||
const _generateBase62 = (length: number): string => {
|
||||
const chars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
|
||||
const bytes = new Uint8Array(length);
|
||||
crypto.getRandomValues(bytes);
|
||||
return Array.from(bytes, (b) => chars[b % chars.length]).join('');
|
||||
};
|
||||
|
||||
/**
|
||||
* Generates a compact client ID: {platform}_{4-char-base62}, e.g. "B_a7Kx".
|
||||
*/
|
||||
export const generateClientId = (): string => {
|
||||
return `${_getEnvironmentId()}_${_generateBase62(4)}`;
|
||||
};
|
||||
|
||||
/**
|
||||
* Type guard: true if `id` matches a known valid client-ID format.
|
||||
* - Legacy format: any string of length >= 10 (legacy IDs).
|
||||
* - New format: {platform}_{4-char-base62}, e.g. "B_a7Kx".
|
||||
*
|
||||
* Used to narrow `unknown` values read from IndexedDB. An invalid format is
|
||||
* treated as "absent" rather than fatal — see issue #6197.
|
||||
*/
|
||||
export const isValidClientIdFormat = (id: unknown): id is string => {
|
||||
return (
|
||||
typeof id === 'string' && (id.length >= 10 || /^[BEAI]_[a-zA-Z0-9]{4}$/.test(id))
|
||||
);
|
||||
};
|
||||
|
|
@ -4,6 +4,13 @@ import { BehaviorSubject, merge, Observable, ReplaySubject, Subject } from 'rxjs
|
|||
import { mapTo } from 'rxjs/operators';
|
||||
import { DroidLog } from '../../core/log';
|
||||
|
||||
export interface AndroidShareData {
|
||||
title: string;
|
||||
subject: string;
|
||||
type: 'FILE' | 'LINK' | 'IMG' | 'COMMAND' | 'NOTE';
|
||||
path: string;
|
||||
}
|
||||
|
||||
export interface AndroidInterface {
|
||||
getVersion?(): string;
|
||||
|
||||
|
|
@ -100,12 +107,7 @@ export interface AndroidInterface {
|
|||
isInBackground$: Observable<boolean>;
|
||||
isKeyboardShown$: Subject<boolean>;
|
||||
|
||||
onShareWithAttachment$: Subject<{
|
||||
title: string;
|
||||
subject: string;
|
||||
type: 'FILE' | 'LINK' | 'IMG' | 'COMMAND' | 'NOTE';
|
||||
path: string;
|
||||
}>;
|
||||
onShareWithAttachment$: Subject<AndroidShareData>;
|
||||
|
||||
// Notification action callbacks
|
||||
onPauseTracking$: Subject<void>;
|
||||
|
|
|
|||
87
src/app/features/android/store/android.effects.spec.ts
Normal file
87
src/app/features/android/store/android.effects.spec.ts
Normal file
|
|
@ -0,0 +1,87 @@
|
|||
import { buildTaskTitle, readableUrl } from './android.effects';
|
||||
|
||||
describe('android share helpers', () => {
|
||||
describe('buildTaskTitle', () => {
|
||||
it('prefers the subject (page title from browsers) over everything else', () => {
|
||||
expect(
|
||||
buildTaskTitle({
|
||||
subject: 'Great Article',
|
||||
title: 'Some Title',
|
||||
type: 'LINK',
|
||||
path: 'https://example.com/post',
|
||||
}),
|
||||
).toBe('Great Article');
|
||||
});
|
||||
|
||||
it('falls back to the explicit title when there is no subject', () => {
|
||||
expect(
|
||||
buildTaskTitle({
|
||||
subject: '',
|
||||
title: 'Some Title',
|
||||
type: 'LINK',
|
||||
path: 'https://example.com/post',
|
||||
}),
|
||||
).toBe('Some Title');
|
||||
});
|
||||
|
||||
// Regression: a share without subject/title must derive a readable title
|
||||
// from the link, not the generic "Shared Content" placeholder.
|
||||
it('derives a readable title from the URL for links without subject/title', () => {
|
||||
expect(
|
||||
buildTaskTitle({
|
||||
subject: '',
|
||||
title: '',
|
||||
type: 'LINK',
|
||||
path: 'https://www.example.com/some-cool-article',
|
||||
}),
|
||||
).toBe('example.com: some cool article');
|
||||
});
|
||||
|
||||
it('uses the first line of content for notes without subject/title', () => {
|
||||
expect(
|
||||
buildTaskTitle({
|
||||
subject: '',
|
||||
title: '',
|
||||
type: 'NOTE',
|
||||
path: 'Buy milk\nand bread',
|
||||
}),
|
||||
).toBe('Buy milk');
|
||||
});
|
||||
|
||||
it('falls back to "Shared note" for empty note content', () => {
|
||||
expect(buildTaskTitle({ subject: '', title: '', type: 'NOTE', path: '' })).toBe(
|
||||
'Shared note',
|
||||
);
|
||||
});
|
||||
|
||||
it('tolerates missing fields', () => {
|
||||
expect(buildTaskTitle({})).toBe('Shared note');
|
||||
});
|
||||
|
||||
it('truncates very long titles to 150 chars', () => {
|
||||
const result = buildTaskTitle({
|
||||
subject: 'x'.repeat(300),
|
||||
type: 'NOTE',
|
||||
path: 'p',
|
||||
});
|
||||
expect(result.length).toBe(150);
|
||||
expect(result.endsWith('...')).toBeTrue();
|
||||
});
|
||||
});
|
||||
|
||||
describe('readableUrl', () => {
|
||||
it('returns host and decoded path', () => {
|
||||
expect(readableUrl('https://www.example.com/foo/bar-baz')).toBe(
|
||||
'example.com: foo bar baz',
|
||||
);
|
||||
});
|
||||
|
||||
it('returns just the host when there is no path', () => {
|
||||
expect(readableUrl('https://example.com/')).toBe('example.com');
|
||||
});
|
||||
|
||||
it('returns the original string for invalid URLs', () => {
|
||||
expect(readableUrl('not a url')).toBe('not a url');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -4,7 +4,7 @@ import { tap } from 'rxjs/operators';
|
|||
import { SnackService } from '../../../core/snack/snack.service';
|
||||
import { IS_ANDROID_WEB_VIEW } from '../../../util/is-android-web-view';
|
||||
import { DroidLog } from '../../../core/log';
|
||||
import { androidInterface } from '../android-interface';
|
||||
import { androidInterface, AndroidShareData } from '../android-interface';
|
||||
import { TaskService } from '../../tasks/task.service';
|
||||
import { TaskAttachmentService } from '../../tasks/task-attachment/task-attachment.service';
|
||||
import { T } from '../../../t.const';
|
||||
|
|
@ -23,7 +23,13 @@ export class AndroidEffects {
|
|||
() =>
|
||||
androidInterface.onShareWithAttachment$.pipe(
|
||||
tap((shareData) => {
|
||||
const taskTitle = this._buildTaskTitle(shareData);
|
||||
// Guard against empty payloads (e.g. stale share data persisted by an
|
||||
// older app version) so we never create a blank, attachment-less task.
|
||||
if (!shareData?.path?.trim()) {
|
||||
DroidLog.warn('Ignoring share intent with empty content');
|
||||
return;
|
||||
}
|
||||
const taskTitle = buildTaskTitle(shareData);
|
||||
const taskId = this._taskService.add(taskTitle);
|
||||
const icon = shareData.type === 'LINK' ? 'link' : 'file_present';
|
||||
this._taskAttachmentService.addAttachment(taskId, {
|
||||
|
|
@ -150,51 +156,6 @@ export class AndroidEffects {
|
|||
{ dispatch: false },
|
||||
);
|
||||
|
||||
private _buildTaskTitle(shareData: {
|
||||
title: string;
|
||||
subject: string;
|
||||
type: string;
|
||||
path: string;
|
||||
}): string {
|
||||
const subject = shareData.subject?.trim() || '';
|
||||
const title = shareData.title?.trim() || '';
|
||||
const path = shareData.path?.trim() || '';
|
||||
|
||||
let taskTitle: string;
|
||||
|
||||
// Prefer subject (page title from browsers), then title, then type-specific fallback
|
||||
if (subject) {
|
||||
taskTitle = subject;
|
||||
} else if (title) {
|
||||
taskTitle = title;
|
||||
} else if (shareData.type === 'LINK') {
|
||||
taskTitle = this._readableUrl(path);
|
||||
} else {
|
||||
const firstLine = path.split('\n')[0].trim();
|
||||
taskTitle = firstLine || 'Shared note';
|
||||
}
|
||||
|
||||
return taskTitle.length > 150 ? taskTitle.substring(0, 147) + '...' : taskTitle;
|
||||
}
|
||||
|
||||
private _readableUrl(url: string): string {
|
||||
try {
|
||||
const parsed = new URL(url);
|
||||
const host = parsed.hostname.replace(/^www\./, '');
|
||||
const pathPart = parsed.pathname.replace(/\/$/, '');
|
||||
if (pathPart && pathPart !== '/') {
|
||||
const decoded = decodeURIComponent(pathPart)
|
||||
.replace(/[/_-]/g, ' ')
|
||||
.replace(/\s+/g, ' ')
|
||||
.trim();
|
||||
return decoded ? `${host}: ${decoded}` : host;
|
||||
}
|
||||
return host;
|
||||
} catch {
|
||||
return url;
|
||||
}
|
||||
}
|
||||
|
||||
// Check for pending share data on resume (catches app killed after receiving share)
|
||||
checkPendingShareOnResume$ =
|
||||
IS_ANDROID_WEB_VIEW &&
|
||||
|
|
@ -217,3 +178,51 @@ export class AndroidEffects {
|
|||
{ dispatch: false },
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a meaningful task title from Android share intent data.
|
||||
* Prefers the page subject (EXTRA_SUBJECT, sent by browsers), then an explicit
|
||||
* title (EXTRA_TITLE), then a type-specific fallback derived from the shared
|
||||
* content itself. Never returns the unhelpful literal "Shared Content".
|
||||
*/
|
||||
export const buildTaskTitle = (shareData: Partial<AndroidShareData>): string => {
|
||||
const subject = shareData.subject?.trim() || '';
|
||||
const title = shareData.title?.trim() || '';
|
||||
const path = shareData.path?.trim() || '';
|
||||
|
||||
let taskTitle: string;
|
||||
|
||||
if (subject) {
|
||||
taskTitle = subject;
|
||||
} else if (title) {
|
||||
taskTitle = title;
|
||||
} else if (shareData.type === 'LINK') {
|
||||
taskTitle = readableUrl(path);
|
||||
} else {
|
||||
const firstLine = path.split('\n')[0].trim();
|
||||
taskTitle = firstLine || 'Shared note';
|
||||
}
|
||||
|
||||
return taskTitle.length > 150 ? taskTitle.substring(0, 147) + '...' : taskTitle;
|
||||
};
|
||||
|
||||
/**
|
||||
* Turn a URL into a human-readable "host: path" string for use as a task title.
|
||||
*/
|
||||
export const readableUrl = (url: string): string => {
|
||||
try {
|
||||
const parsed = new URL(url);
|
||||
const host = parsed.hostname.replace(/^www\./, '');
|
||||
const pathPart = parsed.pathname.replace(/\/$/, '');
|
||||
if (pathPart && pathPart !== '/') {
|
||||
const decoded = decodeURIComponent(pathPart)
|
||||
.replace(/[/_-]/g, ' ')
|
||||
.replace(/\s+/g, ' ')
|
||||
.trim();
|
||||
return decoded ? `${host}: ${decoded}` : host;
|
||||
}
|
||||
return host;
|
||||
} catch {
|
||||
return url;
|
||||
}
|
||||
};
|
||||
|
|
|
|||
|
|
@ -397,6 +397,35 @@ describe('DialogEditTaskRepeatCfgComponent', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('_normalizeMonthlyAnchor strips stale monthlyLastDay (#7726)', () => {
|
||||
it('clears monthlyLastDay when quickSetting is no longer MONTHLY_LAST_DAY', async () => {
|
||||
const fixture = await setupTestBed({ task: mockTask });
|
||||
const normalized = (fixture.componentInstance as any)._normalizeMonthlyAnchor({
|
||||
quickSetting: 'CUSTOM',
|
||||
monthlyLastDay: true,
|
||||
});
|
||||
expect(normalized.monthlyLastDay).toBeUndefined();
|
||||
});
|
||||
|
||||
it('keeps monthlyLastDay for the MONTHLY_LAST_DAY preset', async () => {
|
||||
const fixture = await setupTestBed({ task: mockTask });
|
||||
const normalized = (fixture.componentInstance as any)._normalizeMonthlyAnchor({
|
||||
quickSetting: 'MONTHLY_LAST_DAY',
|
||||
monthlyLastDay: true,
|
||||
});
|
||||
expect(normalized.monthlyLastDay).toBe(true);
|
||||
});
|
||||
|
||||
it('still converts the monthlyWeekOfMonth null sentinel to undefined', async () => {
|
||||
const fixture = await setupTestBed({ task: mockTask });
|
||||
const normalized = (fixture.componentInstance as any)._normalizeMonthlyAnchor({
|
||||
quickSetting: 'CUSTOM',
|
||||
monthlyWeekOfMonth: null,
|
||||
});
|
||||
expect(normalized.monthlyWeekOfMonth).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('save button disabled state (issue #5828)', () => {
|
||||
it('should not allow save while isLoading is true', fakeAsync(async () => {
|
||||
const taskWithRepeatCfg = {
|
||||
|
|
|
|||
|
|
@ -284,11 +284,8 @@ export class DialogEditTaskRepeatCfgComponent {
|
|||
}
|
||||
}
|
||||
|
||||
// The form uses `null` as the "(Day of month)" sentinel on the
|
||||
// monthlyWeekOfMonth select. Persisted cfgs use `undefined` for absent
|
||||
// optional fields (project convention). Normalize at the boundary so
|
||||
// existing day-of-month cfgs don't produce spurious change diffs and the
|
||||
// op-log stays consistent with the model type.
|
||||
// Normalize the monthly anchor fields at the boundary: convert the form's
|
||||
// `null` sentinel to `undefined`, and strip a stale `monthlyLastDay` flag.
|
||||
const finalRepeatCfg = this._normalizeMonthlyAnchor(this.repeatCfg());
|
||||
|
||||
if (this.isEdit()) {
|
||||
|
|
@ -321,11 +318,29 @@ export class DialogEditTaskRepeatCfgComponent {
|
|||
}
|
||||
}
|
||||
|
||||
private _normalizeMonthlyAnchor<T extends { monthlyWeekOfMonth?: unknown }>(cfg: T): T {
|
||||
if (cfg.monthlyWeekOfMonth === null) {
|
||||
return { ...cfg, monthlyWeekOfMonth: undefined };
|
||||
private _normalizeMonthlyAnchor<
|
||||
T extends {
|
||||
monthlyWeekOfMonth?: unknown;
|
||||
monthlyLastDay?: boolean;
|
||||
quickSetting?: string;
|
||||
},
|
||||
>(cfg: T): T {
|
||||
let result = cfg;
|
||||
// The form uses `null` as the "(Day of month)" sentinel on the
|
||||
// monthlyWeekOfMonth select. Persisted cfgs use `undefined` for absent
|
||||
// optional fields (project convention). Normalizing here keeps existing
|
||||
// day-of-month cfgs from producing spurious change diffs.
|
||||
if (result.monthlyWeekOfMonth === null) {
|
||||
result = { ...result, monthlyWeekOfMonth: undefined };
|
||||
}
|
||||
return cfg;
|
||||
// `monthlyLastDay` has no CUSTOM-mode form control, so a flag left over
|
||||
// from the MONTHLY_LAST_DAY preset would silently override the
|
||||
// day-of-month a CUSTOM cfg shows. It is only ever valid for that
|
||||
// preset — strip it for any other quick setting (#7726).
|
||||
if (result.monthlyLastDay && result.quickSetting !== 'MONTHLY_LAST_DAY') {
|
||||
result = { ...result, monthlyLastDay: undefined };
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
remove(): void {
|
||||
|
|
|
|||
|
|
@ -135,6 +135,8 @@ describe('getQuickSettingUpdates', () => {
|
|||
});
|
||||
|
||||
describe('MONTHLY_FIRST_DAY', () => {
|
||||
afterEach(() => jasmine.clock().uninstall());
|
||||
|
||||
it('should return MONTHLY cycle with repeatEvery 1', () => {
|
||||
const result = getQuickSettingUpdates('MONTHLY_FIRST_DAY');
|
||||
expect(result).toBeDefined();
|
||||
|
|
@ -142,14 +144,39 @@ describe('getQuickSettingUpdates', () => {
|
|||
expect(result!.repeatEvery).toBe(1);
|
||||
});
|
||||
|
||||
it('should set startDate to the 1st of the month', () => {
|
||||
it('should set startDate to the 1st of a month', () => {
|
||||
const result = getQuickSettingUpdates('MONTHLY_FIRST_DAY');
|
||||
const startDate = new Date(result!.startDate + 'T00:00:00');
|
||||
expect(startDate.getDate()).toBe(1);
|
||||
});
|
||||
|
||||
it('should anchor to the NEXT 1st when today is not the 1st (#7726)', () => {
|
||||
// Issue scenario: setting this up on 2026-05-22 must schedule June 1,
|
||||
// never the already-past May 1.
|
||||
jasmine.clock().install();
|
||||
jasmine.clock().mockDate(new Date(2026, 4, 22));
|
||||
const result = getQuickSettingUpdates('MONTHLY_FIRST_DAY');
|
||||
expect(result!.startDate).toBe('2026-06-01');
|
||||
});
|
||||
|
||||
it('should anchor to today when today IS the 1st', () => {
|
||||
jasmine.clock().install();
|
||||
jasmine.clock().mockDate(new Date(2026, 4, 1));
|
||||
const result = getQuickSettingUpdates('MONTHLY_FIRST_DAY');
|
||||
expect(result!.startDate).toBe('2026-05-01');
|
||||
});
|
||||
|
||||
it('should roll over the year in December', () => {
|
||||
jasmine.clock().install();
|
||||
jasmine.clock().mockDate(new Date(2026, 11, 15));
|
||||
const result = getQuickSettingUpdates('MONTHLY_FIRST_DAY');
|
||||
expect(result!.startDate).toBe('2027-01-01');
|
||||
});
|
||||
});
|
||||
|
||||
describe('MONTHLY_LAST_DAY', () => {
|
||||
afterEach(() => jasmine.clock().uninstall());
|
||||
|
||||
it('should return MONTHLY cycle with repeatEvery 1', () => {
|
||||
const result = getQuickSettingUpdates('MONTHLY_LAST_DAY');
|
||||
expect(result).toBeDefined();
|
||||
|
|
@ -157,10 +184,25 @@ describe('getQuickSettingUpdates', () => {
|
|||
expect(result!.repeatEvery).toBe(1);
|
||||
});
|
||||
|
||||
it('should always set startDate with day=31 regardless of current month', () => {
|
||||
it('should set the monthlyLastDay flag', () => {
|
||||
const result = getQuickSettingUpdates('MONTHLY_LAST_DAY');
|
||||
const startDate = new Date(result!.startDate + 'T00:00:00');
|
||||
expect(startDate.getDate()).toBe(31);
|
||||
expect(result!.monthlyLastDay).toBe(true);
|
||||
});
|
||||
|
||||
it('should anchor startDate to the last day of the current month (#7726)', () => {
|
||||
// Issue scenario: setting this up on 2026-05-22 must schedule May 31,
|
||||
// never the already-past Jan 31.
|
||||
jasmine.clock().install();
|
||||
jasmine.clock().mockDate(new Date(2026, 4, 22));
|
||||
const result = getQuickSettingUpdates('MONTHLY_LAST_DAY');
|
||||
expect(result!.startDate).toBe('2026-05-31');
|
||||
});
|
||||
|
||||
it('should anchor to a short month-end when set up in a 30-day month', () => {
|
||||
jasmine.clock().install();
|
||||
jasmine.clock().mockDate(new Date(2026, 5, 10));
|
||||
const result = getQuickSettingUpdates('MONTHLY_LAST_DAY');
|
||||
expect(result!.startDate).toBe('2026-06-30');
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -220,6 +262,25 @@ describe('getQuickSettingUpdates', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('monthlyLastDay anchor is mutually exclusive with other presets', () => {
|
||||
it('MONTHLY_CURRENT_DATE clears monthlyLastDay', () => {
|
||||
expect(
|
||||
getQuickSettingUpdates('MONTHLY_CURRENT_DATE')!.monthlyLastDay,
|
||||
).toBeUndefined();
|
||||
});
|
||||
|
||||
it('MONTHLY_FIRST_DAY clears monthlyLastDay', () => {
|
||||
expect(getQuickSettingUpdates('MONTHLY_FIRST_DAY')!.monthlyLastDay).toBeUndefined();
|
||||
});
|
||||
|
||||
it('MONTHLY_NTH_WEEKDAY clears monthlyLastDay', () => {
|
||||
const ref = new Date(2026, 0, 12);
|
||||
expect(
|
||||
getQuickSettingUpdates('MONTHLY_NTH_WEEKDAY', ref)!.monthlyLastDay,
|
||||
).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('CUSTOM', () => {
|
||||
it('should return undefined', () => {
|
||||
const result = getQuickSettingUpdates('CUSTOM');
|
||||
|
|
|
|||
|
|
@ -23,12 +23,13 @@ const _buildWeeklyForDay = (date: Date): Partial<TaskRepeatCfg> => {
|
|||
};
|
||||
};
|
||||
|
||||
// Switching from a day-of-week preset to a day-of-month preset must clear the
|
||||
// Nth-weekday anchor — anchor presence is the discriminator, so stale fields
|
||||
// would silently take effect.
|
||||
const DAY_OF_MONTH_RESET: Partial<TaskRepeatCfg> = {
|
||||
// Switching between monthly presets must clear every monthly anchor —
|
||||
// anchor presence is the discriminator, so a stale Nth-weekday or last-day
|
||||
// field would silently take effect.
|
||||
const MONTHLY_ANCHOR_RESET: Partial<TaskRepeatCfg> = {
|
||||
monthlyWeekOfMonth: undefined,
|
||||
monthlyWeekday: undefined,
|
||||
monthlyLastDay: undefined,
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
@ -74,30 +75,38 @@ export const getQuickSettingUpdates = (
|
|||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
startDate: getDbDateStr(referenceDate || today),
|
||||
...DAY_OF_MONTH_RESET,
|
||||
...MONTHLY_ANCHOR_RESET,
|
||||
};
|
||||
}
|
||||
|
||||
case 'MONTHLY_FIRST_DAY': {
|
||||
const firstDay = new Date(today.getFullYear(), today.getMonth(), 1);
|
||||
// Anchor to the next 1st-of-month that is today or later, so the first
|
||||
// generated instance is never backdated (#7726). `month + 1` rolls the
|
||||
// year over correctly in December.
|
||||
const firstDay =
|
||||
today.getDate() === 1
|
||||
? new Date(today.getFullYear(), today.getMonth(), 1)
|
||||
: new Date(today.getFullYear(), today.getMonth() + 1, 1);
|
||||
return {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
startDate: getDbDateStr(firstDay),
|
||||
...DAY_OF_MONTH_RESET,
|
||||
...MONTHLY_ANCHOR_RESET,
|
||||
};
|
||||
}
|
||||
|
||||
case 'MONTHLY_LAST_DAY': {
|
||||
// Always use day=31 so the occurrence calculator clamps via
|
||||
// Math.min(31, lastDayOfMonth), producing true "last day" behavior.
|
||||
// Using the current month's last day would fail in short months (e.g. Feb=28).
|
||||
const day31 = new Date(today.getFullYear(), 0, 31);
|
||||
// First occurrence = the upcoming last day of the current month, which
|
||||
// is always today or later. The `monthlyLastDay` flag tells the
|
||||
// occurrence engine to clamp to month-end every month, so `startDate`'s
|
||||
// day-of-month no longer needs to be a hardcoded 31 (#7726).
|
||||
const lastDayThisMonth = new Date(today.getFullYear(), today.getMonth() + 1, 0);
|
||||
return {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
startDate: getDbDateStr(day31),
|
||||
...DAY_OF_MONTH_RESET,
|
||||
startDate: getDbDateStr(lastDayThisMonth),
|
||||
...MONTHLY_ANCHOR_RESET,
|
||||
monthlyLastDay: true,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -115,6 +124,7 @@ export const getQuickSettingUpdates = (
|
|||
startDate: getDbDateStr(ref),
|
||||
monthlyWeekOfMonth: weekOfMonth,
|
||||
monthlyWeekday: weekday,
|
||||
monthlyLastDay: undefined,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -173,6 +173,62 @@ describe('getFirstRepeatOccurrence', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('MONTHLY last day of month (issue #7726)', () => {
|
||||
it('returns the last day of a 31-day startDate month', () => {
|
||||
const result = getFirstRepeatOccurrence(
|
||||
mkCfg({
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
startDate: '2026-05-31',
|
||||
monthlyLastDay: true,
|
||||
}),
|
||||
);
|
||||
expect(result!.getFullYear()).toBe(2026);
|
||||
expect(result!.getMonth()).toBe(4);
|
||||
expect(result!.getDate()).toBe(31);
|
||||
});
|
||||
|
||||
it('returns the last day of a 30-day startDate month', () => {
|
||||
const result = getFirstRepeatOccurrence(
|
||||
mkCfg({
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
startDate: '2026-06-30',
|
||||
monthlyLastDay: true,
|
||||
}),
|
||||
);
|
||||
expect(result!.getMonth()).toBe(5);
|
||||
expect(result!.getDate()).toBe(30);
|
||||
});
|
||||
|
||||
it('resolves to the month-end regardless of startDate day-of-month', () => {
|
||||
const result = getFirstRepeatOccurrence(
|
||||
mkCfg({
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
startDate: '2026-02-15',
|
||||
monthlyLastDay: true,
|
||||
}),
|
||||
);
|
||||
expect(result!.getMonth()).toBe(1);
|
||||
expect(result!.getDate()).toBe(28);
|
||||
});
|
||||
|
||||
it('resolves to February 29 in a leap year', () => {
|
||||
const result = getFirstRepeatOccurrence(
|
||||
mkCfg({
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
startDate: '2024-02-10',
|
||||
monthlyLastDay: true,
|
||||
}),
|
||||
);
|
||||
expect(result!.getFullYear()).toBe(2024);
|
||||
expect(result!.getMonth()).toBe(1);
|
||||
expect(result!.getDate()).toBe(29);
|
||||
});
|
||||
});
|
||||
|
||||
describe('MONTHLY Nth weekday (issue #6040)', () => {
|
||||
it('returns the Nth weekday in the start month when on/after startDate', () => {
|
||||
// 2026-01-01 (Thu) is the 1st Thursday of Jan 2026 → returns Jan 1.
|
||||
|
|
|
|||
|
|
@ -44,6 +44,13 @@ export const getFirstRepeatOccurrence = (taskRepeatCfg: TaskRepeatCfg): Date | n
|
|||
accept: (candidate) => candidate >= checkDate,
|
||||
});
|
||||
}
|
||||
if (taskRepeatCfg.monthlyLastDay) {
|
||||
// Last calendar day of startDate's month — day 0 of the next month
|
||||
// (#7726).
|
||||
const lastDay = new Date(checkDate.getFullYear(), checkDate.getMonth() + 1, 0);
|
||||
lastDay.setHours(12, 0, 0, 0);
|
||||
return lastDay;
|
||||
}
|
||||
return checkDate;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -473,6 +473,40 @@ describe('getNewestPossibleDueDate()', () => {
|
|||
);
|
||||
});
|
||||
|
||||
describe('MONTHLY monthlyLastDay (issue #7726)', () => {
|
||||
it('returns the last day of the current month', () => {
|
||||
const cfg = dummyRepeatable('ID1', {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
monthlyLastDay: true,
|
||||
lastTaskCreationDay: '2026-05-31',
|
||||
});
|
||||
testCase(cfg, new Date(2026, 5, 30), new Date(2026, 4, 31), new Date(2026, 5, 30));
|
||||
});
|
||||
|
||||
it('clamps to month-end even when startDate day-of-month is 30', () => {
|
||||
// A recurrence set up in a 30-day month must still hit 31 in long
|
||||
// months — the anchor is decoupled from startDate's day.
|
||||
const cfg = dummyRepeatable('ID1', {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
monthlyLastDay: true,
|
||||
lastTaskCreationDay: '2026-06-30',
|
||||
});
|
||||
testCase(cfg, new Date(2026, 6, 31), new Date(2026, 5, 30), new Date(2026, 6, 31));
|
||||
});
|
||||
|
||||
it('returns null when the latest month-end is already created', () => {
|
||||
const cfg = dummyRepeatable('ID1', {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
monthlyLastDay: true,
|
||||
lastTaskCreationDay: '2026-06-30',
|
||||
});
|
||||
testCase(cfg, new Date(2026, 6, 15), new Date(2026, 4, 31), null);
|
||||
});
|
||||
});
|
||||
|
||||
describe('MONTHLY Nth weekday (issue #6040)', () => {
|
||||
it('returns the Nth weekday of this month when today equals it', () => {
|
||||
// 1st Thursday of Jan 2026 = Jan 1 (Thu). today = Jan 1.
|
||||
|
|
|
|||
|
|
@ -107,7 +107,11 @@ export const getNewestPossibleDueDate = (
|
|||
});
|
||||
}
|
||||
|
||||
const dayOfMonthRepeat = startDateDate.getDate();
|
||||
// `monthlyLastDay` anchors to month-end: day 31 makes setDateSafely's
|
||||
// Math.min(31, lastDayOfMonth) clamp to the true last day every month.
|
||||
const dayOfMonthRepeat = taskRepeatCfg.monthlyLastDay
|
||||
? 31
|
||||
: startDateDate.getDate();
|
||||
|
||||
// Handle month-end dates properly
|
||||
const setDateSafely = (date: Date, day: number): void => {
|
||||
|
|
|
|||
|
|
@ -264,6 +264,50 @@ describe('getNextRepeatOccurrence()', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('MONTHLY monthlyLastDay (issue #7726)', () => {
|
||||
it('returns the last day of the next month', () => {
|
||||
const cfg = dummyRepeatable('ID1', {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
monthlyLastDay: true,
|
||||
lastTaskCreationDay: getDbDateStr(new Date(2026, 4, 31)),
|
||||
});
|
||||
testCase(cfg, new Date(2026, 5, 1), new Date(2026, 4, 31), new Date(2026, 5, 30));
|
||||
});
|
||||
|
||||
it('clamps to month-end even when startDate day-of-month is 30', () => {
|
||||
// A recurrence set up in a 30-day month must still hit 31 in long
|
||||
// months — the anchor is decoupled from startDate's day.
|
||||
const cfg = dummyRepeatable('ID1', {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
monthlyLastDay: true,
|
||||
lastTaskCreationDay: getDbDateStr(new Date(2026, 5, 30)),
|
||||
});
|
||||
testCase(cfg, new Date(2026, 6, 1), new Date(2026, 5, 30), new Date(2026, 6, 31));
|
||||
});
|
||||
|
||||
it('clamps to February month-end', () => {
|
||||
const cfg = dummyRepeatable('ID1', {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
monthlyLastDay: true,
|
||||
lastTaskCreationDay: getDbDateStr(new Date(2026, 0, 31)),
|
||||
});
|
||||
testCase(cfg, new Date(2026, 1, 1), new Date(2026, 0, 31), new Date(2026, 1, 28));
|
||||
});
|
||||
|
||||
it('clamps to February 29 in a leap year', () => {
|
||||
const cfg = dummyRepeatable('ID1', {
|
||||
repeatCycle: 'MONTHLY',
|
||||
repeatEvery: 1,
|
||||
monthlyLastDay: true,
|
||||
lastTaskCreationDay: getDbDateStr(new Date(2024, 0, 31)),
|
||||
});
|
||||
testCase(cfg, new Date(2024, 1, 1), new Date(2024, 0, 31), new Date(2024, 1, 29));
|
||||
});
|
||||
});
|
||||
|
||||
describe('MONTHLY Nth weekday (issue #6040)', () => {
|
||||
it('returns the first Thursday of next month', () => {
|
||||
const startDate = new Date(2026, 0, 1); // Jan 1, 2026 (Thursday)
|
||||
|
|
|
|||
|
|
@ -98,7 +98,11 @@ export const getNextRepeatOccurrence = (
|
|||
});
|
||||
}
|
||||
|
||||
const dayOfMonthRepeat = startDateDate.getDate();
|
||||
// `monthlyLastDay` anchors to month-end: day 31 makes setDateSafely's
|
||||
// Math.min(31, lastDayOfMonth) clamp to the true last day every month.
|
||||
const dayOfMonthRepeat = taskRepeatCfg.monthlyLastDay
|
||||
? 31
|
||||
: startDateDate.getDate();
|
||||
|
||||
// Handle month-end dates properly
|
||||
const setDateSafely = (date: Date, day: number): void => {
|
||||
|
|
|
|||
|
|
@ -332,6 +332,98 @@ describe('TaskRepeatCfgEffects - Repeatable Subtasks', () => {
|
|||
expect(taskService.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should set dueDay to today when an Inbox task (no dueDay) is made repeatable starting today (#7725)', (done) => {
|
||||
// Scenario from issue #7725: a task added to the Inbox (no dueDay) is
|
||||
// converted to a recurring task via the dialog with start date = today.
|
||||
// It must be scheduled for today instead of staying unscheduled.
|
||||
const today = new Date();
|
||||
const todayStr = getDbDateStr(today);
|
||||
|
||||
const inboxTask: TaskWithSubTasks = {
|
||||
...mockTask,
|
||||
subTasks: [],
|
||||
dueDay: undefined, // Inbox task — not scheduled
|
||||
dueWithTime: undefined,
|
||||
created: today.getTime(),
|
||||
};
|
||||
|
||||
const dailyRepeatCfg: TaskRepeatCfgCopy = {
|
||||
...mockRepeatCfg,
|
||||
repeatCycle: 'DAILY',
|
||||
repeatEvery: 1,
|
||||
startDate: todayStr,
|
||||
};
|
||||
|
||||
const action = addTaskRepeatCfgToTask({
|
||||
taskRepeatCfg: dailyRepeatCfg,
|
||||
taskId: 'parent-task-id',
|
||||
});
|
||||
|
||||
actions$ = of(action);
|
||||
taskService.getByIdWithSubTaskData$.and.returnValue(of(inboxTask));
|
||||
|
||||
spyOn(effects as any, '_updateRegularTaskInstance');
|
||||
|
||||
let emitted = false;
|
||||
effects.updateTaskAfterMakingItRepeatable$.subscribe(() => {
|
||||
emitted = true;
|
||||
});
|
||||
|
||||
setTimeout(() => {
|
||||
expect(emitted).toBe(false); // today occurrence = no planTaskForDay
|
||||
expect(taskService.update).toHaveBeenCalledWith('parent-task-id', {
|
||||
dueDay: todayStr,
|
||||
});
|
||||
done();
|
||||
}, 0);
|
||||
});
|
||||
|
||||
it('should NOT set dueDay for a non-timed config when the task already has dueWithTime', (done) => {
|
||||
// A task scheduled with a time (dueWithTime, no dueDay) is made
|
||||
// repeatable, but the user cleared the start-time field so the config is
|
||||
// non-timed. dueDay must NOT be set: dueDay/dueWithTime are mutually
|
||||
// exclusive and a plain update would not clear dueWithTime.
|
||||
const today = new Date();
|
||||
const todayStr = getDbDateStr(today);
|
||||
const todayNoon = new Date(today);
|
||||
todayNoon.setHours(12, 0, 0, 0);
|
||||
|
||||
const timedTask: TaskWithSubTasks = {
|
||||
...mockTask,
|
||||
subTasks: [],
|
||||
dueDay: undefined,
|
||||
dueWithTime: todayNoon.getTime(),
|
||||
created: today.getTime(),
|
||||
};
|
||||
|
||||
const dailyRepeatCfg: TaskRepeatCfgCopy = {
|
||||
...mockRepeatCfg,
|
||||
repeatCycle: 'DAILY',
|
||||
repeatEvery: 1,
|
||||
startDate: todayStr,
|
||||
};
|
||||
|
||||
const action = addTaskRepeatCfgToTask({
|
||||
taskRepeatCfg: dailyRepeatCfg,
|
||||
taskId: 'parent-task-id',
|
||||
});
|
||||
|
||||
actions$ = of(action);
|
||||
taskService.getByIdWithSubTaskData$.and.returnValue(of(timedTask));
|
||||
|
||||
spyOn(effects as any, '_updateRegularTaskInstance');
|
||||
|
||||
effects.updateTaskAfterMakingItRepeatable$.subscribe();
|
||||
|
||||
setTimeout(() => {
|
||||
const dueDayUpdate = taskService.update.calls
|
||||
.allArgs()
|
||||
.find(([, changes]) => 'dueDay' in (changes as object));
|
||||
expect(dueDayUpdate).toBeUndefined();
|
||||
done();
|
||||
}, 0);
|
||||
});
|
||||
|
||||
it('should update task created when first occurrence is today but task was created earlier', () => {
|
||||
const today = new Date();
|
||||
const todayStr = getDbDateStr(today);
|
||||
|
|
@ -2099,6 +2191,9 @@ describe('TaskRepeatCfgEffects - Repeatable Subtasks', () => {
|
|||
|
||||
setTimeout(() => {
|
||||
expect(emitted).toBe(false);
|
||||
// #7724 guard: startDate present in changes but not moved earlier
|
||||
// (equal to lastTaskCreationDay) must not re-anchor the config.
|
||||
expect(taskRepeatCfgService.updateTaskRepeatCfg).not.toHaveBeenCalled();
|
||||
done();
|
||||
}, 0);
|
||||
});
|
||||
|
|
@ -2500,6 +2595,58 @@ describe('TaskRepeatCfgEffects - Repeatable Subtasks', () => {
|
|||
done();
|
||||
});
|
||||
});
|
||||
|
||||
// Issue #7724: moving startDate earlier must re-anchor lastTaskCreationDay
|
||||
// even when no live task instance exists (e.g. the user deleted it). The
|
||||
// stale anchor would otherwise suppress every projected/created instance
|
||||
// between the new startDate and the old anchor.
|
||||
it('should re-anchor lastTaskCreationDay when startDate moved earlier and no live instance exists (#7724)', (done) => {
|
||||
const today = new Date();
|
||||
// lastTaskCreationDay set when the (now deleted) instance was created.
|
||||
const oldStartDateStr = getDbDateStr(addDays(today, 8));
|
||||
const newStartDateStr = getDbDateStr(addDays(today, 3));
|
||||
// The fix anchors to the day before the new first occurrence so the new
|
||||
// startDate itself is created/projected fresh.
|
||||
const expectedAnchorStr = getDbDateStr(addDays(today, 2));
|
||||
|
||||
const updatedCfg: TaskRepeatCfgCopy = {
|
||||
...mockRepeatCfg,
|
||||
repeatCycle: 'DAILY',
|
||||
repeatEvery: 1,
|
||||
startDate: newStartDateStr,
|
||||
lastTaskCreationDay: oldStartDateStr,
|
||||
};
|
||||
|
||||
const action = updateTaskRepeatCfg({
|
||||
taskRepeatCfg: {
|
||||
id: 'repeat-cfg-id',
|
||||
changes: { startDate: newStartDateStr },
|
||||
},
|
||||
});
|
||||
|
||||
actions$ = of(action);
|
||||
taskRepeatCfgService.getTaskRepeatCfgById$.and.returnValue(of(updatedCfg));
|
||||
// No live instance — it was deleted.
|
||||
taskService.getTasksByRepeatCfgId$.and.returnValue(of([]));
|
||||
|
||||
let emitted = false;
|
||||
effects.rescheduleTaskOnRepeatCfgUpdate$.subscribe(() => {
|
||||
emitted = true;
|
||||
});
|
||||
|
||||
setTimeout(() => {
|
||||
// No live task to reschedule, so no action is dispatched...
|
||||
expect(emitted).toBe(false);
|
||||
// ...but the stale anchor is still corrected.
|
||||
expect(taskRepeatCfgService.updateTaskRepeatCfg).toHaveBeenCalledWith(
|
||||
'repeat-cfg-id',
|
||||
jasmine.objectContaining({
|
||||
lastTaskCreationDay: expectedAnchorStr,
|
||||
}),
|
||||
);
|
||||
done();
|
||||
}, 0);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -199,12 +199,17 @@ export class TaskRepeatCfgEffects {
|
|||
// TODAY FIRST OCCURRENCE:
|
||||
// Keep the stable occurrence identity aligned even if the task was
|
||||
// originally created before it became repeatable.
|
||||
const currentDueDay = task.dueDay || getDbDateStr(task.created);
|
||||
const update: Partial<TaskCopy> = {};
|
||||
if (firstOccurrence && getDbDateStr(task.created) !== firstOccurrenceStr) {
|
||||
update.created = firstOccurrence.getTime();
|
||||
}
|
||||
if (currentDueDay !== firstOccurrenceStr) {
|
||||
// Schedule the task for its first occurrence day. An Inbox task has
|
||||
// no dueDay, so set it explicitly (#7725); task.created is not a
|
||||
// valid fallback — being created today does not imply it is
|
||||
// scheduled. Skip already time-scheduled tasks: dueDay/dueWithTime
|
||||
// are mutually exclusive and this plain update cannot clear
|
||||
// dueWithTime (timed scheduling: addRepeatCfgToTaskUpdateTask$).
|
||||
if (!isTimedTask && !task.dueWithTime && task.dueDay !== firstOccurrenceStr) {
|
||||
update.dueDay = firstOccurrenceStr;
|
||||
}
|
||||
if (Object.keys(update).length > 0) {
|
||||
|
|
@ -268,12 +273,6 @@ export class TaskRepeatCfgEffects {
|
|||
first(),
|
||||
switchMap((liveInstances) => {
|
||||
const undoneInstances = liveInstances.filter((t) => !t.isDone);
|
||||
if (undoneInstances.length === 0) {
|
||||
return EMPTY;
|
||||
}
|
||||
const task = undoneInstances.reduce((a, b) =>
|
||||
a.created > b.created ? a : b,
|
||||
);
|
||||
|
||||
// If the user moved startDate earlier than the existing
|
||||
// lastTaskCreationDay, the anchor is stale — re-anchor on
|
||||
|
|
@ -293,6 +292,37 @@ export class TaskRepeatCfgEffects {
|
|||
const firstOccurrence = isStartDateMovedEarlier
|
||||
? getFirstRepeatOccurrence(fullCfg)
|
||||
: getNextRepeatOccurrence(fullCfg, new Date());
|
||||
|
||||
if (undoneInstances.length === 0) {
|
||||
// No live instance to reschedule. But when startDate moved
|
||||
// earlier, the stale lastTaskCreationDay would still suppress
|
||||
// every projected/created instance between the new startDate
|
||||
// and the old anchor (#7724). Re-anchor to the day before the
|
||||
// new first occurrence so it — and every following day — is
|
||||
// created and projected fresh.
|
||||
// The re-dispatched updateTaskRepeatCfg only touches
|
||||
// lastTaskCreation* fields, which are absent from
|
||||
// SCHEDULE_AFFECTING_FIELDS, so it does not re-enter this
|
||||
// effect.
|
||||
if (isStartDateMovedEarlier && firstOccurrence) {
|
||||
const dayBeforeFirstOccurrence = new Date(firstOccurrence);
|
||||
dayBeforeFirstOccurrence.setDate(
|
||||
dayBeforeFirstOccurrence.getDate() - 1,
|
||||
);
|
||||
this._taskRepeatCfgService.updateTaskRepeatCfg(cfgId, {
|
||||
lastTaskCreationDay: this._dateService.todayStr(
|
||||
dayBeforeFirstOccurrence,
|
||||
),
|
||||
lastTaskCreation: dayBeforeFirstOccurrence.getTime(),
|
||||
});
|
||||
}
|
||||
return EMPTY;
|
||||
}
|
||||
|
||||
const task = undoneInstances.reduce((a, b) =>
|
||||
a.created > b.created ? a : b,
|
||||
);
|
||||
|
||||
const firstOccurrenceStr = firstOccurrence
|
||||
? this._dateService.todayStr(firstOccurrence)
|
||||
: this._dateService.todayStr();
|
||||
|
|
|
|||
|
|
@ -436,6 +436,55 @@ describe('selectTaskRepeatCfgsDueOnDay', () => {
|
|||
expect(resultIds).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
// Issue #7724: after the user moves a DAILY config's startDate earlier with no
|
||||
// live instance, rescheduleTaskOnRepeatCfgUpdate$ re-anchors lastTaskCreationDay
|
||||
// to the day BEFORE the new startDate. This verifies the projector then surfaces
|
||||
// the config for the new startDate and every following day (the days the bug
|
||||
// suppressed), and still excludes days on/before the anchor.
|
||||
describe('#7724 - DAILY startDate moved earlier, re-anchored to day before', () => {
|
||||
const reAnchoredCfg = (): TaskRepeatCfg =>
|
||||
dummyRepeatable('R1', {
|
||||
repeatCycle: 'DAILY',
|
||||
repeatEvery: 1,
|
||||
startDate: '2025-06-25',
|
||||
// value written by the fix: the day before the new startDate
|
||||
lastTaskCreationDay: '2025-06-24',
|
||||
});
|
||||
|
||||
['2025-06-25', '2025-06-26', '2025-06-30', '2025-07-05'].forEach((dayStr) => {
|
||||
it(`projects the config on ${dayStr}`, () => {
|
||||
const result = selectTaskRepeatCfgsForExactDay.projector([reAnchoredCfg()], {
|
||||
dayDate: dateStrToUtcDate(dayStr).getTime(),
|
||||
});
|
||||
expect(result.map((r) => r.id)).toEqual(['R1']);
|
||||
});
|
||||
});
|
||||
|
||||
['2025-06-24', '2025-06-23'].forEach((dayStr) => {
|
||||
it(`does not project on ${dayStr} (on/before the anchor)`, () => {
|
||||
const result = selectTaskRepeatCfgsForExactDay.projector([reAnchoredCfg()], {
|
||||
dayDate: dateStrToUtcDate(dayStr).getTime(),
|
||||
});
|
||||
expect(result.map((r) => r.id)).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
it('is suppressed by the stale anchor without the fix', () => {
|
||||
// Pre-fix state: lastTaskCreationDay still points at the old startDate,
|
||||
// so a day between the new startDate and the old anchor is suppressed.
|
||||
const staleCfg = dummyRepeatable('R1', {
|
||||
repeatCycle: 'DAILY',
|
||||
repeatEvery: 1,
|
||||
startDate: '2025-06-25',
|
||||
lastTaskCreationDay: '2025-06-30',
|
||||
});
|
||||
const result = selectTaskRepeatCfgsForExactDay.projector([staleCfg], {
|
||||
dayDate: dateStrToUtcDate('2025-06-27').getTime(),
|
||||
});
|
||||
expect(result.map((r) => r.id)).toEqual([]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -72,6 +72,14 @@ export interface TaskRepeatCfgCopy {
|
|||
monthlyWeekOfMonth?: MonthlyWeekOfMonth;
|
||||
monthlyWeekday?: MonthlyWeekday;
|
||||
|
||||
// MONTHLY-only: when true, the recurrence anchors to the last calendar day
|
||||
// of every month (28/29/30/31) regardless of `startDate`'s day-of-month.
|
||||
// Decouples the anchor from `startDate` so the first occurrence is never
|
||||
// backdated. Mutually exclusive with the Nth-weekday anchor above; if a
|
||||
// malformed payload sets both, the Nth-weekday anchor wins (checked first
|
||||
// by all recurrence calc utils). Issue #7726.
|
||||
monthlyLastDay?: boolean;
|
||||
|
||||
// advanced
|
||||
notes: string | undefined;
|
||||
// ... possible sub tasks & attachments
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ import { GlobalConfigService } from '../../config/global-config.service';
|
|||
import { Router } from '@angular/router';
|
||||
import { TaskSharedActions } from '../../../root-store/meta/task-shared.actions';
|
||||
import { T } from '../../../t.const';
|
||||
import { Task } from '../task.model';
|
||||
import { Task, TaskWithSubTasks } from '../task.model';
|
||||
import { WorkContextType } from '../../work-context/work-context.model';
|
||||
import { selectProjectById } from '../../project/store/project.selectors';
|
||||
import { LOCAL_ACTIONS } from '../../../util/local-actions.token';
|
||||
|
|
@ -248,6 +248,107 @@ describe('TaskUiEffects', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('snackDelete$', () => {
|
||||
const createMockTaskWithSubTasks = (
|
||||
overrides: Partial<TaskWithSubTasks> = {},
|
||||
): TaskWithSubTasks => ({
|
||||
...createMockTask(),
|
||||
subTasks: [],
|
||||
...overrides,
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
actions$ = new Subject<Action>();
|
||||
snackServiceMock = jasmine.createSpyObj('SnackService', ['open']);
|
||||
taskServiceMock = jasmine.createSpyObj('TaskService', ['setSelectedId']);
|
||||
navigateToTaskServiceMock = jasmine.createSpyObj('NavigateToTaskService', [
|
||||
'navigate',
|
||||
]);
|
||||
layoutServiceMock = jasmine.createSpyObj('LayoutService', ['hideAddTaskBar']);
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
TaskUiEffects,
|
||||
{ provide: LOCAL_ACTIONS, useValue: actions$ },
|
||||
provideMockStore({
|
||||
initialState: {},
|
||||
selectors: [{ selector: selectProjectById, value: null }],
|
||||
}),
|
||||
{ provide: SnackService, useValue: snackServiceMock },
|
||||
{ provide: TaskService, useValue: taskServiceMock },
|
||||
{ provide: NavigateToTaskService, useValue: navigateToTaskServiceMock },
|
||||
{ provide: LayoutService, useValue: layoutServiceMock },
|
||||
{ provide: WorkContextService, useValue: { mainListTaskIds$: of([]) } },
|
||||
{
|
||||
provide: NotifyService,
|
||||
useValue: jasmine.createSpyObj('NotifyService', ['notify']),
|
||||
},
|
||||
{
|
||||
provide: BannerService,
|
||||
useValue: jasmine.createSpyObj('BannerService', ['open', 'dismiss']),
|
||||
},
|
||||
{
|
||||
provide: GlobalConfigService,
|
||||
useValue: { sound$: of({ doneSound: null }) },
|
||||
},
|
||||
{ provide: Router, useValue: jasmine.createSpyObj('Router', ['navigate']) },
|
||||
],
|
||||
});
|
||||
|
||||
effects = TestBed.inject(TaskUiEffects);
|
||||
});
|
||||
|
||||
it('should show the undo snack when deleting a task that has a title', () => {
|
||||
const sub = effects.snackDelete$.subscribe();
|
||||
actions$.next(
|
||||
TaskSharedActions.deleteTask({
|
||||
task: createMockTaskWithSubTasks({ title: 'Real task' }),
|
||||
}),
|
||||
);
|
||||
|
||||
expect(snackServiceMock.open).toHaveBeenCalled();
|
||||
const snackParams = snackServiceMock.open.calls.mostRecent().args[0] as SnackParams;
|
||||
expect(snackParams.actionStr).toBe(T.G.UNDO);
|
||||
sub.unsubscribe();
|
||||
});
|
||||
|
||||
it('should NOT show the undo snack when deleting a blank task', () => {
|
||||
const sub = effects.snackDelete$.subscribe();
|
||||
actions$.next(
|
||||
TaskSharedActions.deleteTask({
|
||||
task: createMockTaskWithSubTasks({ title: '' }),
|
||||
}),
|
||||
);
|
||||
|
||||
expect(snackServiceMock.open).not.toHaveBeenCalled();
|
||||
sub.unsubscribe();
|
||||
});
|
||||
|
||||
it('should NOT show the undo snack when deleting a blank sub task', () => {
|
||||
const sub = effects.snackDelete$.subscribe();
|
||||
actions$.next(
|
||||
TaskSharedActions.deleteTask({
|
||||
task: createMockTaskWithSubTasks({ title: ' ', parentId: 'parent-1' }),
|
||||
}),
|
||||
);
|
||||
|
||||
expect(snackServiceMock.open).not.toHaveBeenCalled();
|
||||
sub.unsubscribe();
|
||||
});
|
||||
|
||||
it('should show the undo snack when deleting a blank-titled task that has data', () => {
|
||||
const sub = effects.snackDelete$.subscribe();
|
||||
actions$.next(
|
||||
TaskSharedActions.deleteTask({
|
||||
task: createMockTaskWithSubTasks({ title: '', timeSpent: 5000 }),
|
||||
}),
|
||||
);
|
||||
|
||||
expect(snackServiceMock.open).toHaveBeenCalled();
|
||||
sub.unsubscribe();
|
||||
});
|
||||
});
|
||||
|
||||
describe('deadlineTodayBanner$', () => {
|
||||
let store: MockStore;
|
||||
|
||||
|
|
|
|||
|
|
@ -42,6 +42,7 @@ import { LayoutService } from '../../../core-ui/layout/layout.service';
|
|||
import { LS } from '../../../core/persistence/storage-keys.const';
|
||||
import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator';
|
||||
import { DateService } from '../../../core/date/date.service';
|
||||
import { isBlankTask } from '../util/is-blank-task';
|
||||
|
||||
@Injectable()
|
||||
export class TaskUiEffects {
|
||||
|
|
@ -116,6 +117,8 @@ export class TaskUiEffects {
|
|||
() =>
|
||||
this._actions$.pipe(
|
||||
ofType(TaskSharedActions.deleteTask),
|
||||
// Skip the undo snack for accidentally created blank tasks
|
||||
filter(({ task }) => !isBlankTask(task)),
|
||||
tap(({ task }) => {
|
||||
this._snackService.open({
|
||||
translateParams: {
|
||||
|
|
|
|||
118
src/app/features/tasks/util/is-blank-task.spec.ts
Normal file
118
src/app/features/tasks/util/is-blank-task.spec.ts
Normal file
|
|
@ -0,0 +1,118 @@
|
|||
import { isBlankTask } from './is-blank-task';
|
||||
import { Task, TaskWithSubTasks } from '../task.model';
|
||||
|
||||
const createTask = (overrides: Partial<Task> = {}): Task =>
|
||||
({
|
||||
id: 'task-1',
|
||||
title: '',
|
||||
projectId: 'project-1',
|
||||
tagIds: [],
|
||||
subTaskIds: [],
|
||||
parentId: undefined,
|
||||
timeSpentOnDay: {},
|
||||
timeSpent: 0,
|
||||
timeEstimate: 0,
|
||||
isDone: false,
|
||||
notes: '',
|
||||
attachments: [],
|
||||
created: Date.now(),
|
||||
...overrides,
|
||||
}) as Task;
|
||||
|
||||
const createTaskWithSubTasks = (
|
||||
overrides: Partial<TaskWithSubTasks> = {},
|
||||
): TaskWithSubTasks => ({
|
||||
...createTask(),
|
||||
subTasks: [],
|
||||
...overrides,
|
||||
});
|
||||
|
||||
describe('isBlankTask', () => {
|
||||
it('should be true for a freshly created task with an empty title', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks())).toBe(true);
|
||||
});
|
||||
|
||||
it('should be true when the title is only whitespace', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ title: ' ' }))).toBe(true);
|
||||
});
|
||||
|
||||
it('should ignore context-derived fields (tagIds, projectId)', () => {
|
||||
expect(
|
||||
isBlankTask(
|
||||
createTaskWithSubTasks({ tagIds: ['some-tag'], projectId: 'other-project' }),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('should be false when the task has a title', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ title: 'Real task' }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when the task has notes', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ notes: 'some note' }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when time was tracked', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ timeSpent: 1000 }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when a time estimate is set', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ timeEstimate: 60000 }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when the task has attachments', () => {
|
||||
expect(
|
||||
isBlankTask(
|
||||
createTaskWithSubTasks({
|
||||
attachments: [{ id: 'a', type: 'LINK', title: 't', path: 'p' }],
|
||||
}),
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when the task is linked to an issue', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ issueId: 'issue-1' }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when the task has a reminder', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ reminderId: 'reminder-1' }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when the task has a repeat config', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ repeatCfgId: 'repeat-1' }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when the task is scheduled', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ dueWithTime: Date.now() }))).toBe(false);
|
||||
expect(isBlankTask(createTaskWithSubTasks({ dueDay: '2026-05-22' }))).toBe(false);
|
||||
});
|
||||
|
||||
it('should be false when the task has a deadline', () => {
|
||||
expect(isBlankTask(createTaskWithSubTasks({ deadlineDay: '2026-05-22' }))).toBe(
|
||||
false,
|
||||
);
|
||||
expect(isBlankTask(createTaskWithSubTasks({ deadlineWithTime: Date.now() }))).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it('should be false for a parent task with a sub task that has data', () => {
|
||||
expect(
|
||||
isBlankTask(
|
||||
createTaskWithSubTasks({
|
||||
subTasks: [createTask({ id: 'sub-1', title: 'Sub task' })],
|
||||
}),
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it('should be true for a parent task whose sub tasks are all blank', () => {
|
||||
expect(
|
||||
isBlankTask(
|
||||
createTaskWithSubTasks({
|
||||
subTasks: [createTask({ id: 'sub-1' }), createTask({ id: 'sub-2' })],
|
||||
}),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
30
src/app/features/tasks/util/is-blank-task.ts
Normal file
30
src/app/features/tasks/util/is-blank-task.ts
Normal file
|
|
@ -0,0 +1,30 @@
|
|||
import { Task, TaskWithSubTasks } from '../task.model';
|
||||
|
||||
/**
|
||||
* Returns true when a task carries no user data worth undoing: an empty title
|
||||
* plus no notes, time tracking, estimate, attachments, issue link, scheduling,
|
||||
* deadline, repeat config or non-blank sub tasks.
|
||||
*
|
||||
* Used to suppress the undo-delete snack when an accidentally created blank
|
||||
* task (or sub task) is deleted right away. Context-derived fields like
|
||||
* `tagIds` and `projectId` are intentionally ignored — they are not data the
|
||||
* user would lose.
|
||||
*/
|
||||
export const isBlankTask = (task: Task | TaskWithSubTasks): boolean => {
|
||||
const subTasks = (task as TaskWithSubTasks).subTasks;
|
||||
return (
|
||||
!task.title.trim() &&
|
||||
!task.notes?.trim() &&
|
||||
!task.timeSpent &&
|
||||
!task.timeEstimate &&
|
||||
!task.attachments?.length &&
|
||||
!task.issueId &&
|
||||
!task.reminderId &&
|
||||
!task.repeatCfgId &&
|
||||
!task.dueWithTime &&
|
||||
!task.dueDay &&
|
||||
!task.deadlineDay &&
|
||||
!task.deadlineWithTime &&
|
||||
(!subTasks?.length || subTasks.every(isBlankTask))
|
||||
);
|
||||
};
|
||||
|
|
@ -1096,6 +1096,50 @@ describe('ArchiveOperationHandler', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('REPAIR', () => {
|
||||
// Regression: a REPAIR op built from the sync getStateSnapshot() carries
|
||||
// empty archives. Without this guard it overwrites (wipes) the local
|
||||
// archive on every other client that applies the REPAIR op.
|
||||
it('should preserve local archiveYoung when REPAIR has empty archive (likely bug)', async () => {
|
||||
const action = {
|
||||
type: loadAllData.type,
|
||||
appDataComplete: { archiveYoung: emptyArchive },
|
||||
meta: { isPersistent: true, isRemote: true, opType: OpType.Repair },
|
||||
} as unknown as PersistentAction;
|
||||
|
||||
await service.handleOperation(action);
|
||||
|
||||
expect(mockArchiveDbAdapter.saveArchiveYoung).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should preserve local archiveOld when REPAIR has empty archive (likely bug)', async () => {
|
||||
const action = {
|
||||
type: loadAllData.type,
|
||||
appDataComplete: { archiveOld: emptyArchive },
|
||||
meta: { isPersistent: true, isRemote: true, opType: OpType.Repair },
|
||||
} as unknown as PersistentAction;
|
||||
|
||||
await service.handleOperation(action);
|
||||
|
||||
expect(mockArchiveDbAdapter.saveArchiveOld).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should allow writing non-empty archive over existing archive', async () => {
|
||||
const newArchive = createArchiveModelForTest(['new-task']);
|
||||
const action = {
|
||||
type: loadAllData.type,
|
||||
appDataComplete: { archiveYoung: newArchive },
|
||||
meta: { isPersistent: true, isRemote: true, opType: OpType.Repair },
|
||||
} as unknown as PersistentAction;
|
||||
|
||||
await service.handleOperation(action);
|
||||
|
||||
expect(mockArchiveDbAdapter.saveArchiveYoung).toHaveBeenCalledWith(
|
||||
newArchive,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('BACKUP_IMPORT', () => {
|
||||
let originalConfirm: typeof window.confirm;
|
||||
|
||||
|
|
|
|||
|
|
@ -469,8 +469,9 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
|
|||
const originalArchiveOld = await this._archiveDbAdapter.loadArchiveOld();
|
||||
|
||||
// Safety guard: Check if we're about to overwrite non-empty archives with empty ones
|
||||
// This can happen if SYNC_IMPORT was created with getStateSnapshot() (sync) instead
|
||||
// of getStateSnapshotAsync() (async), which returns DEFAULT_ARCHIVE (empty)
|
||||
// This can happen if a full-state op (SYNC_IMPORT/REPAIR) was created with the sync
|
||||
// getStateSnapshot() instead of getStateSnapshotAsync(), which returns
|
||||
// DEFAULT_ARCHIVE (empty)
|
||||
const hasExistingYoung = (originalArchiveYoung?.task?.ids?.length ?? 0) > 0;
|
||||
const hasExistingOld = (originalArchiveOld?.task?.ids?.length ?? 0) > 0;
|
||||
const isIncomingYoungEmpty = !archiveYoung?.task?.ids?.length;
|
||||
|
|
@ -480,12 +481,17 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
|
|||
if (archiveYoung !== undefined) {
|
||||
if (hasExistingYoung && isIncomingYoungEmpty) {
|
||||
const existingCount = originalArchiveYoung?.task?.ids?.length ?? 0;
|
||||
if (action.meta.opType === OpType.SyncImport) {
|
||||
// SYNC_IMPORT with empty archives is likely a bug (sync snapshot used sync instead of async)
|
||||
// Preserve local archives - the next SYNC_IMPORT (after the fix) will have correct data
|
||||
if (
|
||||
action.meta.opType === OpType.SyncImport ||
|
||||
action.meta.opType === OpType.Repair
|
||||
) {
|
||||
// SYNC_IMPORT/REPAIR with empty archives is most often a bug (full-state
|
||||
// op built with the sync getStateSnapshot() instead of
|
||||
// getStateSnapshotAsync()). Preserve local archives: discarding non-empty
|
||||
// local data for an empty payload is never the safe choice.
|
||||
OpLog.warn(
|
||||
`[ArchiveOperationHandler] SYNC_IMPORT has empty archiveYoung but local has ${existingCount} tasks. ` +
|
||||
'Preserving local archives (this is likely a bug in the SYNC_IMPORT source).',
|
||||
`[ArchiveOperationHandler] ${action.meta.opType} has empty archiveYoung but local has ${existingCount} tasks. ` +
|
||||
'Preserving local archives (this is likely a bug in the full-state op source).',
|
||||
);
|
||||
// Skip writing empty archive - preserve local
|
||||
} else if (action.meta.opType === OpType.BackupImport) {
|
||||
|
|
@ -514,12 +520,17 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
|
|||
|
||||
if (hasExistingOld && isIncomingOldEmpty) {
|
||||
const existingCount = originalArchiveOld?.task?.ids?.length ?? 0;
|
||||
if (action.meta.opType === OpType.SyncImport) {
|
||||
// SYNC_IMPORT with empty archives is likely a bug (sync snapshot used sync instead of async)
|
||||
// Preserve local archives - the next SYNC_IMPORT (after the fix) will have correct data
|
||||
if (
|
||||
action.meta.opType === OpType.SyncImport ||
|
||||
action.meta.opType === OpType.Repair
|
||||
) {
|
||||
// SYNC_IMPORT/REPAIR with empty archives is most often a bug (full-state
|
||||
// op built with the sync getStateSnapshot() instead of
|
||||
// getStateSnapshotAsync()). Preserve local archives: discarding non-empty
|
||||
// local data for an empty payload is never the safe choice.
|
||||
OpLog.warn(
|
||||
`[ArchiveOperationHandler] SYNC_IMPORT has empty archiveOld but local has ${existingCount} tasks. ` +
|
||||
'Preserving local archives (this is likely a bug in the SYNC_IMPORT source).',
|
||||
`[ArchiveOperationHandler] ${action.meta.opType} has empty archiveOld but local has ${existingCount} tasks. ` +
|
||||
'Preserving local archives (this is likely a bug in the full-state op source).',
|
||||
);
|
||||
shouldWriteArchiveOld = false;
|
||||
} else if (action.meta.opType === OpType.BackupImport) {
|
||||
|
|
|
|||
|
|
@ -4,7 +4,6 @@ import { BackupService } from './backup.service';
|
|||
import { ImexViewService } from '../../imex/imex-meta/imex-view.service';
|
||||
import { StateSnapshotService } from './state-snapshot.service';
|
||||
import { OperationLogStoreService } from '../persistence/operation-log-store.service';
|
||||
import { ClientIdService } from '../../core/util/client-id.service';
|
||||
import { ArchiveModel } from '../../features/archive/archive.model';
|
||||
import { loadAllData } from '../../root-store/meta/load-all-data.action';
|
||||
import { OpType } from '../core/operation.types';
|
||||
|
|
@ -18,7 +17,6 @@ describe('BackupService', () => {
|
|||
let mockImexViewService: jasmine.SpyObj<ImexViewService>;
|
||||
let mockStateSnapshotService: jasmine.SpyObj<StateSnapshotService>;
|
||||
let mockOpLogStore: jasmine.SpyObj<OperationLogStoreService>;
|
||||
let mockClientIdService: jasmine.SpyObj<ClientIdService>;
|
||||
let mockOperationWriteFlushService: jasmine.SpyObj<OperationWriteFlushService>;
|
||||
let mockLockService: jasmine.SpyObj<LockService>;
|
||||
|
||||
|
|
@ -107,7 +105,6 @@ describe('BackupService', () => {
|
|||
'saveImportBackup',
|
||||
'runDestructiveStateReplacement',
|
||||
]);
|
||||
mockClientIdService = jasmine.createSpyObj('ClientIdService', ['withRotation']);
|
||||
mockOperationWriteFlushService = jasmine.createSpyObj('OperationWriteFlushService', [
|
||||
'flushPendingWrites',
|
||||
]);
|
||||
|
|
@ -119,12 +116,6 @@ describe('BackupService', () => {
|
|||
);
|
||||
mockOpLogStore.saveImportBackup.and.resolveTo();
|
||||
mockOpLogStore.runDestructiveStateReplacement.and.resolveTo();
|
||||
// ClientIdService.withRotation owns the rollback semantics (see its own
|
||||
// spec); here we just invoke the callback with a fresh id.
|
||||
mockClientIdService.withRotation.and.callFake(
|
||||
async (_logPrefix: string, fn: (newClientId: string) => Promise<any>) =>
|
||||
fn('newClientId'),
|
||||
);
|
||||
mockOperationWriteFlushService.flushPendingWrites.and.resolveTo();
|
||||
mockLockService.request.and.callFake(async (_lockName, fn) => fn());
|
||||
|
||||
|
|
@ -135,7 +126,6 @@ describe('BackupService', () => {
|
|||
{ provide: ImexViewService, useValue: mockImexViewService },
|
||||
{ provide: StateSnapshotService, useValue: mockStateSnapshotService },
|
||||
{ provide: OperationLogStoreService, useValue: mockOpLogStore },
|
||||
{ provide: ClientIdService, useValue: mockClientIdService },
|
||||
{
|
||||
provide: OperationWriteFlushService,
|
||||
useValue: mockOperationWriteFlushService,
|
||||
|
|
@ -308,21 +298,22 @@ describe('BackupService', () => {
|
|||
|
||||
const args = mockOpLogStore.runDestructiveStateReplacement.calls.mostRecent()
|
||||
.args[0] as Parameters<typeof mockOpLogStore.runDestructiveStateReplacement>[0];
|
||||
expect(args.syncImportOp.vectorClock).toEqual({ newClientId: 1 });
|
||||
// The clientId is freshly minted (generateClientId) — new compact format.
|
||||
const op = args.syncImportOp;
|
||||
expect(op.clientId).toMatch(/^[BEAI]_[a-zA-Z0-9]{4}$/);
|
||||
expect(op.vectorClock).toEqual({ [op.clientId]: 1 });
|
||||
});
|
||||
|
||||
it('should pass a fresh clock to the atomic helper on force-import', async () => {
|
||||
mockClientIdService.withRotation.and.callFake(
|
||||
async (_logPrefix: string, fn: (newClientId: string) => Promise<any>) =>
|
||||
fn('newForceClient'),
|
||||
);
|
||||
const backupData = createMinimalValidBackup();
|
||||
|
||||
await service.importCompleteBackup(backupData as any, true, true, true);
|
||||
|
||||
const args = mockOpLogStore.runDestructiveStateReplacement.calls.mostRecent()
|
||||
.args[0] as Parameters<typeof mockOpLogStore.runDestructiveStateReplacement>[0];
|
||||
expect(args.syncImportOp.vectorClock).toEqual({ newForceClient: 1 });
|
||||
const op = args.syncImportOp;
|
||||
expect(op.clientId).toMatch(/^[BEAI]_[a-zA-Z0-9]{4}$/);
|
||||
expect(op.vectorClock).toEqual({ [op.clientId]: 1 });
|
||||
});
|
||||
|
||||
/**
|
||||
|
|
@ -349,17 +340,14 @@ describe('BackupService', () => {
|
|||
});
|
||||
|
||||
it('should produce fresh { [clientId]: 1 } clock on import', async () => {
|
||||
mockClientIdService.withRotation.and.callFake(
|
||||
async (_logPrefix: string, fn: (newClientId: string) => Promise<any>) =>
|
||||
fn('newForceClient'),
|
||||
);
|
||||
const backupData = createMinimalValidBackup();
|
||||
|
||||
await service.importCompleteBackup(backupData as any, true, true, true);
|
||||
|
||||
const args = mockOpLogStore.runDestructiveStateReplacement.calls.mostRecent()
|
||||
.args[0] as Parameters<typeof mockOpLogStore.runDestructiveStateReplacement>[0];
|
||||
expect(args.syncImportOp.vectorClock).toEqual({ newForceClient: 1 });
|
||||
const op = args.syncImportOp;
|
||||
expect(op.vectorClock).toEqual({ [op.clientId]: 1 });
|
||||
});
|
||||
|
||||
it('should abort the import (and not dispatch loadAllData) when the pre-import backup fails', async () => {
|
||||
|
|
@ -393,19 +381,6 @@ describe('BackupService', () => {
|
|||
expect(mockOpLogStore.saveImportBackup).toHaveBeenCalledWith(currentState);
|
||||
});
|
||||
|
||||
it('should delegate cross-DB clientId rollback to ClientIdService.withRotation', async () => {
|
||||
// ClientIdService.withRotation owns the rollback semantics — capture
|
||||
// prior id, run callback, restore on failure, log critical if rollback
|
||||
// also fails. Tested directly in client-id.service.spec.ts; here we
|
||||
// only verify BackupService routes through it with the right log tag.
|
||||
await service.importCompleteBackup(createMinimalValidBackup() as any, true, true);
|
||||
|
||||
expect(mockClientIdService.withRotation).toHaveBeenCalledWith(
|
||||
'BackupService:',
|
||||
jasmine.any(Function),
|
||||
);
|
||||
});
|
||||
|
||||
it('should pass snapshotEntityKeys derived from the imported data', async () => {
|
||||
const backupData = createMinimalValidBackup();
|
||||
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ import { Store } from '@ngrx/store';
|
|||
import { ImexViewService } from '../../imex/imex-meta/imex-view.service';
|
||||
import { StateSnapshotService } from './state-snapshot.service';
|
||||
import { OperationLogStoreService } from '../persistence/operation-log-store.service';
|
||||
import { ClientIdService } from '../../core/util/client-id.service';
|
||||
import { generateClientId } from '../../core/util/generate-client-id';
|
||||
import { Operation, OpType, ActionType } from '../core/operation.types';
|
||||
import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service';
|
||||
import { uuidv7 } from '../../util/uuid-v7';
|
||||
|
|
@ -39,7 +39,6 @@ export class BackupService {
|
|||
private _store = inject(Store);
|
||||
private _stateSnapshotService = inject(StateSnapshotService);
|
||||
private _opLogStore = inject(OperationLogStoreService);
|
||||
private _clientIdService = inject(ClientIdService);
|
||||
private _operationWriteFlushService = inject(OperationWriteFlushService);
|
||||
private _lockService = inject(LockService);
|
||||
|
||||
|
|
@ -188,8 +187,7 @@ export class BackupService {
|
|||
} catch (e) {
|
||||
// `message` is intentionally omitted: log history is user-exportable
|
||||
// (CLAUDE.md sync rule 9), and a future validator/IDB error type could
|
||||
// interpolate user content into its message. Match the `name`-only
|
||||
// pattern used by `ClientIdService.withRotation` rollback logging.
|
||||
// interpolate user content into its message. Log the error `name` only.
|
||||
OpLog.warn('BackupService: Failed to backup state before import:', {
|
||||
name: (e as Error | undefined)?.name,
|
||||
});
|
||||
|
|
@ -198,40 +196,40 @@ export class BackupService {
|
|||
);
|
||||
}
|
||||
|
||||
// Rotate the clientId for the duration of the destructive replacement.
|
||||
// `pf` (clientId) is a separate IDB DB and so cannot share the atomic
|
||||
// SUP_OPS tx; ClientIdService.withRotation rolls it back on failure.
|
||||
await this._clientIdService.withRotation('BackupService:', async (clientId) => {
|
||||
const newClock = { [clientId]: 1 };
|
||||
const opId = uuidv7();
|
||||
// IMPORTANT: Uses OpType.BackupImport which maps to reason='recovery' on the server.
|
||||
// This allows backup imports to succeed even when a SYNC_IMPORT already exists.
|
||||
// See server validation at sync.routes.ts:703-733
|
||||
const op: Operation = {
|
||||
id: opId,
|
||||
actionType: ActionType.LOAD_ALL_DATA,
|
||||
opType: OpType.BackupImport,
|
||||
entityType: 'ALL',
|
||||
entityId: opId,
|
||||
payload: importedData,
|
||||
clientId,
|
||||
vectorClock: newClock,
|
||||
timestamp: Date.now(),
|
||||
schemaVersion: CURRENT_SCHEMA_VERSION,
|
||||
syncImportReason: 'BACKUP_RESTORE',
|
||||
};
|
||||
// Mint a fresh clientId for the new sync baseline. It is pure here —
|
||||
// persisted only inside runDestructiveStateReplacement's atomic SUP_OPS
|
||||
// transaction, which also clears the ClientIdService cache. On a throw the
|
||||
// tx aborts and the prior id stands.
|
||||
const clientId = generateClientId();
|
||||
const newClock = { [clientId]: 1 };
|
||||
const opId = uuidv7();
|
||||
// IMPORTANT: Uses OpType.BackupImport which maps to reason='recovery' on the server.
|
||||
// This allows backup imports to succeed even when a SYNC_IMPORT already exists.
|
||||
// See server validation at sync.routes.ts:703-733
|
||||
const op: Operation = {
|
||||
id: opId,
|
||||
actionType: ActionType.LOAD_ALL_DATA,
|
||||
opType: OpType.BackupImport,
|
||||
entityType: 'ALL',
|
||||
entityId: opId,
|
||||
payload: importedData,
|
||||
clientId,
|
||||
vectorClock: newClock,
|
||||
timestamp: Date.now(),
|
||||
schemaVersion: CURRENT_SCHEMA_VERSION,
|
||||
syncImportReason: 'BACKUP_RESTORE',
|
||||
};
|
||||
|
||||
// Issue #7709: replace OPS + state_cache + vector_clock atomically so an
|
||||
// interrupt during backup-restore can't leave the device in the
|
||||
// `isWhollyFreshClient + meaningful store data` state that triggers the
|
||||
// multi-device data-loss chain.
|
||||
OpLog.normal('BackupService: Replacing op-log + state cache atomically');
|
||||
await this._opLogStore.runDestructiveStateReplacement({
|
||||
syncImportOp: op,
|
||||
snapshotEntityKeys: extractEntityKeysFromState(importedData),
|
||||
archiveYoung: importedData.archiveYoung,
|
||||
archiveOld: importedData.archiveOld,
|
||||
});
|
||||
// Issue #7709: replace OPS + state_cache + vector_clock + clientId
|
||||
// atomically so an interrupt during backup-restore can't leave the device
|
||||
// in the `isWhollyFreshClient + meaningful store data` state that triggers
|
||||
// the multi-device data-loss chain.
|
||||
OpLog.normal('BackupService: Replacing op-log + state cache atomically');
|
||||
await this._opLogStore.runDestructiveStateReplacement({
|
||||
syncImportOp: op,
|
||||
snapshotEntityKeys: extractEntityKeysFromState(importedData),
|
||||
archiveYoung: importedData.archiveYoung,
|
||||
archiveOld: importedData.archiveOld,
|
||||
});
|
||||
|
||||
OpLog.normal('BackupService: Import persisted to operation log.');
|
||||
|
|
|
|||
|
|
@ -72,8 +72,7 @@ describe('OperationLogEffects', () => {
|
|||
'trigger',
|
||||
]);
|
||||
mockClientIdService = jasmine.createSpyObj('ClientIdService', [
|
||||
'loadClientId',
|
||||
'generateNewClientId',
|
||||
'getOrGenerateClientId',
|
||||
]);
|
||||
mockOperationCaptureService = jasmine.createSpyObj('OperationCaptureService', [
|
||||
'dequeue',
|
||||
|
|
@ -92,9 +91,8 @@ describe('OperationLogEffects', () => {
|
|||
mockCompactionService.compact.and.returnValue(Promise.resolve());
|
||||
mockCompactionService.emergencyCompact.and.returnValue(Promise.resolve(true));
|
||||
mockStore.select.and.returnValue(of({})); // Return empty state observable
|
||||
mockClientIdService.loadClientId.and.returnValue(Promise.resolve('testClient'));
|
||||
mockClientIdService.generateNewClientId.and.returnValue(
|
||||
Promise.resolve('generatedClient'),
|
||||
mockClientIdService.getOrGenerateClientId.and.returnValue(
|
||||
Promise.resolve('testClient'),
|
||||
);
|
||||
mockOperationCaptureService.dequeue.and.returnValue([]);
|
||||
|
||||
|
|
@ -198,7 +196,7 @@ describe('OperationLogEffects', () => {
|
|||
return fn();
|
||||
},
|
||||
);
|
||||
mockClientIdService.loadClientId.and.callFake(async () => {
|
||||
mockClientIdService.getOrGenerateClientId.and.callFake(async () => {
|
||||
callOrder.push('clientId');
|
||||
return 'testClient';
|
||||
});
|
||||
|
|
@ -219,11 +217,11 @@ describe('OperationLogEffects', () => {
|
|||
});
|
||||
|
||||
it('should capture the post-rotation clientId when a destructive replacement rotates it while this op is queued (#7709)', (done) => {
|
||||
// Simulates the race the fix at operation-log.effects.ts:163-171 closes:
|
||||
// a clean-slate / backup-import is already holding the operation-log lock
|
||||
// and rotates the clientId via ClientIdService.withRotation. A queued op
|
||||
// waiting behind that lock must read the rotated id, not the pre-rotation
|
||||
// id captured before lock acquisition.
|
||||
// Simulates the race the fix at operation-log.effects.ts closes: a
|
||||
// clean-slate / backup-import is already holding the operation-log lock
|
||||
// and rotates the clientId inside runDestructiveStateReplacement. A
|
||||
// queued op waiting behind that lock must read the rotated id, not the
|
||||
// pre-rotation id captured before lock acquisition.
|
||||
let persistedClientId = 'oldClient';
|
||||
|
||||
mockLockService.request.and.callFake(
|
||||
|
|
@ -234,7 +232,9 @@ describe('OperationLogEffects', () => {
|
|||
return fn();
|
||||
},
|
||||
);
|
||||
mockClientIdService.loadClientId.and.callFake(async () => persistedClientId);
|
||||
mockClientIdService.getOrGenerateClientId.and.callFake(
|
||||
async () => persistedClientId,
|
||||
);
|
||||
|
||||
const action = createPersistentAction(ActionType.TASK_SHARED_UPDATE);
|
||||
actions$ = of(action);
|
||||
|
|
@ -448,9 +448,10 @@ describe('OperationLogEffects', () => {
|
|||
mockOpLogStore.appendWithVectorClockUpdate.calls.mostRecent().args[0];
|
||||
expect(firstOp.clientId).toBe('testClient');
|
||||
|
||||
// Simulate backup import generating a new client ID
|
||||
// (BackupService calls generateNewClientId which updates the cached value)
|
||||
mockClientIdService.loadClientId.and.returnValue(
|
||||
// Simulate a backup import rotating the client ID (BackupService's
|
||||
// destructive replacement persists a fresh id; the next read of
|
||||
// getOrGenerateClientId() picks it up after the cache is cleared).
|
||||
mockClientIdService.getOrGenerateClientId.and.returnValue(
|
||||
Promise.resolve('newImportClient'),
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -165,14 +165,10 @@ export class OperationLogEffects implements DeferredLocalActionsPort {
|
|||
// Clean-slate and backup import rotate the clientId while holding this
|
||||
// same lock; reading here prevents a queued operation from capturing
|
||||
// the old id before waiting behind a destructive replacement.
|
||||
const clientId =
|
||||
(await this.clientIdService.loadClientId()) ??
|
||||
(await this.clientIdService.generateNewClientId());
|
||||
if (!clientId) {
|
||||
throw new Error(
|
||||
'Failed to load or generate clientId - cannot persist operation',
|
||||
);
|
||||
}
|
||||
// getOrGenerateClientId() throws on a transient IndexedDB read failure
|
||||
// rather than minting a fresh id — the catch below treats that as a
|
||||
// retryable persistence failure.
|
||||
const clientId = await this.clientIdService.getOrGenerateClientId();
|
||||
|
||||
// MULTI-TAB SAFETY: Clear vector clock cache to ensure fresh read after other tabs
|
||||
// may have written while we were waiting for the lock. Each tab has its own in-memory
|
||||
|
|
|
|||
|
|
@ -2,7 +2,6 @@ import { TestBed } from '@angular/core/testing';
|
|||
import { CleanSlateService } from './clean-slate.service';
|
||||
import { StateSnapshotService } from '../backup/state-snapshot.service';
|
||||
import { OperationLogStoreService } from '../persistence/operation-log-store.service';
|
||||
import { ClientIdService } from '../../core/util/client-id.service';
|
||||
import { OpType, OperationLogEntry } from '../core/operation.types';
|
||||
import { ActionType } from '../core/action-types.enum';
|
||||
import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service';
|
||||
|
|
@ -15,7 +14,6 @@ describe('CleanSlateService', () => {
|
|||
let service: CleanSlateService;
|
||||
let mockStateSnapshotService: jasmine.SpyObj<StateSnapshotService>;
|
||||
let mockOpLogStore: jasmine.SpyObj<OperationLogStoreService>;
|
||||
let mockClientIdService: jasmine.SpyObj<ClientIdService>;
|
||||
let mockOperationWriteFlushService: jasmine.SpyObj<OperationWriteFlushService>;
|
||||
let mockLockService: jasmine.SpyObj<LockService>;
|
||||
|
||||
|
|
@ -35,7 +33,6 @@ describe('CleanSlateService', () => {
|
|||
'getVectorClock',
|
||||
'getUnsynced',
|
||||
]);
|
||||
mockClientIdService = jasmine.createSpyObj('ClientIdService', ['withRotation']);
|
||||
mockOperationWriteFlushService = jasmine.createSpyObj('OperationWriteFlushService', [
|
||||
'flushPendingWrites',
|
||||
]);
|
||||
|
|
@ -46,7 +43,6 @@ describe('CleanSlateService', () => {
|
|||
CleanSlateService,
|
||||
{ provide: StateSnapshotService, useValue: mockStateSnapshotService },
|
||||
{ provide: OperationLogStoreService, useValue: mockOpLogStore },
|
||||
{ provide: ClientIdService, useValue: mockClientIdService },
|
||||
{
|
||||
provide: OperationWriteFlushService,
|
||||
useValue: mockOperationWriteFlushService,
|
||||
|
|
@ -59,13 +55,6 @@ describe('CleanSlateService', () => {
|
|||
|
||||
// Setup default mock responses
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(mockState as any);
|
||||
// Default: withRotation invokes its callback with the new clientId and
|
||||
// propagates whatever the callback returns or throws. ClientIdService's
|
||||
// own spec covers the rollback semantics.
|
||||
mockClientIdService.withRotation.and.callFake(
|
||||
async (_logPrefix: string, fn: (newClientId: string) => Promise<any>) =>
|
||||
fn('eNewC'),
|
||||
);
|
||||
mockOpLogStore.runDestructiveStateReplacement.and.resolveTo();
|
||||
mockOpLogStore.getVectorClock.and.resolveTo(null);
|
||||
mockOpLogStore.getUnsynced.and.resolveTo([]);
|
||||
|
|
@ -80,13 +69,7 @@ describe('CleanSlateService', () => {
|
|||
// Should get current state (async version to include archives)
|
||||
expect(mockStateSnapshotService.getStateSnapshotAsync).toHaveBeenCalled();
|
||||
|
||||
// Should rotate client ID via the shared helper
|
||||
expect(mockClientIdService.withRotation).toHaveBeenCalledWith(
|
||||
'[CleanSlate]',
|
||||
jasmine.any(Function),
|
||||
);
|
||||
|
||||
// Should route through the atomic helper (issue #7709)
|
||||
// Should route through the atomic helper (issues #7709, #7732)
|
||||
expect(mockOpLogStore.runDestructiveStateReplacement).toHaveBeenCalledTimes(1);
|
||||
const args = mockOpLogStore.runDestructiveStateReplacement.calls.mostRecent()
|
||||
.args[0] as Parameters<typeof mockOpLogStore.runDestructiveStateReplacement>[0];
|
||||
|
|
@ -96,8 +79,10 @@ describe('CleanSlateService', () => {
|
|||
expect(appendedOp.opType).toBe(OpType.SyncImport);
|
||||
expect(appendedOp.entityType).toBe('ALL');
|
||||
expect(appendedOp.payload).toBe(mockState);
|
||||
expect(appendedOp.clientId).toBe('eNewC');
|
||||
expect(appendedOp.vectorClock).toEqual({ eNewC: 1 });
|
||||
// The clientId is freshly minted (generateClientId) — new compact format.
|
||||
// runDestructiveStateReplacement persists it inside its atomic tx.
|
||||
expect(appendedOp.clientId).toMatch(/^[BEAI]_[a-zA-Z0-9]{4}$/);
|
||||
expect(appendedOp.vectorClock).toEqual({ [appendedOp.clientId]: 1 });
|
||||
expect(appendedOp.schemaVersion).toBe(CURRENT_SCHEMA_VERSION);
|
||||
});
|
||||
|
||||
|
|
@ -199,7 +184,8 @@ describe('CleanSlateService', () => {
|
|||
|
||||
const args = mockOpLogStore.runDestructiveStateReplacement.calls.mostRecent()
|
||||
.args[0] as Parameters<typeof mockOpLogStore.runDestructiveStateReplacement>[0];
|
||||
expect(args.syncImportOp.vectorClock).toEqual({ eNewC: 1 });
|
||||
const op = args.syncImportOp;
|
||||
expect(op.vectorClock).toEqual({ [op.clientId]: 1 });
|
||||
});
|
||||
|
||||
it('should create operation with valid UUIDv7', async () => {
|
||||
|
|
@ -223,25 +209,10 @@ describe('CleanSlateService', () => {
|
|||
).toBeRejectedWith(jasmine.objectContaining({ message: 'State error' }));
|
||||
});
|
||||
|
||||
it('should propagate errors from withRotation', async () => {
|
||||
// ClientIdService.withRotation owns the cross-DB rollback semantics
|
||||
// (see its own spec). Here we only verify that CleanSlateService
|
||||
// surfaces failures from the rotation/replacement chain to its caller.
|
||||
mockClientIdService.withRotation.and.rejectWith(
|
||||
new Error('Atomic replacement failed'),
|
||||
);
|
||||
|
||||
await expectAsync(
|
||||
service.createCleanSlate('ENCRYPTION_CHANGE', 'PASSWORD_CHANGED'),
|
||||
).toBeRejectedWith(
|
||||
jasmine.objectContaining({ message: 'Atomic replacement failed' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('should propagate errors from runDestructiveStateReplacement through withRotation', async () => {
|
||||
// The destructive helper runs inside the withRotation callback.
|
||||
// withRotation must re-throw whatever the callback throws so the
|
||||
// caller sees the real failure.
|
||||
it('should propagate errors from runDestructiveStateReplacement', async () => {
|
||||
// The clientId now rotates inside runDestructiveStateReplacement's atomic
|
||||
// transaction; a failure there aborts the whole replacement and the
|
||||
// caller must see the real error.
|
||||
mockOpLogStore.runDestructiveStateReplacement.and.rejectWith(
|
||||
new Error('Atomic replacement failed'),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ import { inject, Injectable } from '@angular/core';
|
|||
import { uuidv7 } from '../../util/uuid-v7';
|
||||
import { StateSnapshotService } from '../backup/state-snapshot.service';
|
||||
import { OperationLogStoreService } from '../persistence/operation-log-store.service';
|
||||
import { ClientIdService } from '../../core/util/client-id.service';
|
||||
import { generateClientId } from '../../core/util/generate-client-id';
|
||||
import { OpLog } from '../../core/log';
|
||||
import { Operation, OpType, SyncImportReason } from '../core/operation.types';
|
||||
import { ActionType } from '../core/action-types.enum';
|
||||
|
|
@ -22,15 +22,16 @@ export type CleanSlateReason = 'ENCRYPTION_CHANGE' | 'MANUAL';
|
|||
/**
|
||||
* Service for performing "clean slate" operations on the sync state.
|
||||
*
|
||||
* Atomically replaces the local op-log + state_cache + vector_clock with a
|
||||
* fresh baseline derived from current state. Used by encryption-password
|
||||
* Atomically replaces the local op-log + state_cache + vector_clock + clientId
|
||||
* with a fresh baseline derived from current state. Used by encryption-password
|
||||
* changes (which need a fresh sync baseline) and by user-initiated sync
|
||||
* recovery.
|
||||
*
|
||||
* Atomicity within `SUP_OPS` is guaranteed by
|
||||
* `OperationLogStoreService.runDestructiveStateReplacement`; cross-DB
|
||||
* rotation of the clientId (which lives in `pf`) is rolled back on failure
|
||||
* — see issue #7709.
|
||||
* Atomicity is guaranteed by
|
||||
* `OperationLogStoreService.runDestructiveStateReplacement`. The clientId now
|
||||
* lives in `SUP_OPS` too, so it rotates atomically with the op-log inside that
|
||||
* single transaction — no cross-database rollback is needed (issues #7709,
|
||||
* #7732).
|
||||
*
|
||||
* @example
|
||||
* ```typescript
|
||||
|
|
@ -44,7 +45,6 @@ export type CleanSlateReason = 'ENCRYPTION_CHANGE' | 'MANUAL';
|
|||
export class CleanSlateService {
|
||||
private stateSnapshotService = inject(StateSnapshotService);
|
||||
private opLogStore = inject(OperationLogStoreService);
|
||||
private clientIdService = inject(ClientIdService);
|
||||
private operationWriteFlushService = inject(OperationWriteFlushService);
|
||||
private lockService = inject(LockService);
|
||||
|
||||
|
|
@ -52,10 +52,9 @@ export class CleanSlateService {
|
|||
* Creates a clean slate by resetting local operation log and preparing
|
||||
* a fresh SYNC_IMPORT operation for upload.
|
||||
*
|
||||
* The destructive sequence (clear OPS, append SYNC_IMPORT, write vector
|
||||
* clock, write state_cache) runs as a single atomic transaction. On
|
||||
* failure, the rotated clientId is rolled back so `pf` and `SUP_OPS` stay
|
||||
* consistent.
|
||||
* The destructive sequence (rotate clientId, clear OPS, append SYNC_IMPORT,
|
||||
* write vector clock, write state_cache) runs as a single atomic
|
||||
* transaction. On failure it aborts wholesale and the prior id stands.
|
||||
*
|
||||
* Does NOT upload to the server — that happens on the next sync, with the
|
||||
* `isCleanSlate=true` flag, which makes the server delete its operations
|
||||
|
|
@ -101,43 +100,41 @@ export class CleanSlateService {
|
|||
// which causes data loss.
|
||||
const currentState = await this.stateSnapshotService.getStateSnapshotAsync();
|
||||
|
||||
// Rotate the clientId for the duration of the destructive replacement.
|
||||
// The clientId lives in a separate IDB database (`pf`) and so cannot
|
||||
// share the atomic SUP_OPS tx below; ClientIdService.withRotation rolls
|
||||
// it back on failure so `pf` and `SUP_OPS` agree on the device's
|
||||
// clientId after this method returns or throws.
|
||||
return this.clientIdService.withRotation('[CleanSlate]', async (newClientId) => {
|
||||
OpLog.normal('[CleanSlate] Generated new client ID', {
|
||||
newClientIdSuffix: newClientId.slice(-3),
|
||||
});
|
||||
|
||||
const newVectorClock = { [newClientId]: 1 };
|
||||
const syncImportOp: Operation = {
|
||||
id: uuidv7(),
|
||||
actionType: ActionType.LOAD_ALL_DATA,
|
||||
opType: OpType.SyncImport,
|
||||
entityType: 'ALL',
|
||||
entityId: undefined,
|
||||
payload: currentState,
|
||||
clientId: newClientId,
|
||||
vectorClock: newVectorClock,
|
||||
timestamp: Date.now(),
|
||||
schemaVersion: CURRENT_SCHEMA_VERSION,
|
||||
syncImportReason,
|
||||
};
|
||||
|
||||
OpLog.normal('[CleanSlate] Created SYNC_IMPORT operation', {
|
||||
opId: syncImportOp.id,
|
||||
});
|
||||
|
||||
OpLog.normal('[CleanSlate] Replacing op-log + state cache atomically');
|
||||
await this.opLogStore.runDestructiveStateReplacement({
|
||||
syncImportOp,
|
||||
snapshotEntityKeys: extractEntityKeysFromState(currentState),
|
||||
});
|
||||
|
||||
return { syncImportId: syncImportOp.id };
|
||||
// Mint a fresh clientId for the new sync baseline. It is pure here —
|
||||
// persisted only inside runDestructiveStateReplacement's atomic
|
||||
// SUP_OPS transaction, which also clears the ClientIdService cache.
|
||||
// On a throw the tx aborts and the prior id stands.
|
||||
const newClientId = generateClientId();
|
||||
OpLog.normal('[CleanSlate] Generated new client ID', {
|
||||
newClientIdSuffix: newClientId.slice(-3),
|
||||
});
|
||||
|
||||
const newVectorClock = { [newClientId]: 1 };
|
||||
const syncImportOp: Operation = {
|
||||
id: uuidv7(),
|
||||
actionType: ActionType.LOAD_ALL_DATA,
|
||||
opType: OpType.SyncImport,
|
||||
entityType: 'ALL',
|
||||
entityId: undefined,
|
||||
payload: currentState,
|
||||
clientId: newClientId,
|
||||
vectorClock: newVectorClock,
|
||||
timestamp: Date.now(),
|
||||
schemaVersion: CURRENT_SCHEMA_VERSION,
|
||||
syncImportReason,
|
||||
};
|
||||
|
||||
OpLog.normal('[CleanSlate] Created SYNC_IMPORT operation', {
|
||||
opId: syncImportOp.id,
|
||||
});
|
||||
|
||||
OpLog.normal('[CleanSlate] Replacing op-log + state cache atomically');
|
||||
await this.opLogStore.runDestructiveStateReplacement({
|
||||
syncImportOp,
|
||||
snapshotEntityKeys: extractEntityKeysFromState(currentState),
|
||||
});
|
||||
|
||||
return { syncImportId: syncImportOp.id };
|
||||
},
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ import { CompleteBackup } from '../sync-exports';
|
|||
export const DB_NAME = 'SUP_OPS';
|
||||
|
||||
/** Current database schema version */
|
||||
export const DB_VERSION = 5;
|
||||
export const DB_VERSION = 6;
|
||||
|
||||
/** Object store names */
|
||||
export const STORE_NAMES = {
|
||||
|
|
@ -30,6 +30,8 @@ export const STORE_NAMES = {
|
|||
ARCHIVE_OLD: 'archive_old' as const,
|
||||
/** Profile data - stores complete backups for user profile switching */
|
||||
PROFILE_DATA: 'profile_data' as const,
|
||||
/** Client ID - sync device identity (singleton, key = SINGLETON_KEY) */
|
||||
CLIENT_ID: 'client_id' as const,
|
||||
} as const;
|
||||
|
||||
/** Common key used for singleton entries */
|
||||
|
|
|
|||
|
|
@ -153,8 +153,8 @@ describe('runDbUpgrade', () => {
|
|||
|
||||
// Version 2 upgrade doesn't create any stores (only version 3+ run)
|
||||
// Version 3 only adds an index, doesn't create stores
|
||||
// Version 4 creates archive stores, version 5 creates profile_data
|
||||
expect(db.createObjectStore).toHaveBeenCalledTimes(3); // archive_young, archive_old, profile_data
|
||||
// Version 4 creates archive stores, version 5 profile_data, version 6 client_id
|
||||
expect(db.createObjectStore).toHaveBeenCalledTimes(4); // archive_young, archive_old, profile_data, client_id
|
||||
expect(db.createObjectStore).not.toHaveBeenCalledWith(
|
||||
STORE_NAMES.OPS,
|
||||
jasmine.anything(),
|
||||
|
|
@ -200,7 +200,27 @@ describe('runDbUpgrade', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('full upgrade path (version 0 to 4)', () => {
|
||||
describe('version 6 upgrade (from version 5)', () => {
|
||||
it('should create client_id store', () => {
|
||||
const preExisting = new Map([[STORE_NAMES.OPS, { store: createMockStore() }]]);
|
||||
const { db, tx } = createMocks(preExisting);
|
||||
|
||||
runDbUpgrade(db, 5, tx);
|
||||
|
||||
expect(db.createObjectStore).toHaveBeenCalledWith(STORE_NAMES.CLIENT_ID);
|
||||
});
|
||||
|
||||
it('should not recreate earlier stores', () => {
|
||||
const preExisting = new Map([[STORE_NAMES.OPS, { store: createMockStore() }]]);
|
||||
const { db, tx } = createMocks(preExisting);
|
||||
|
||||
runDbUpgrade(db, 5, tx);
|
||||
|
||||
expect(db.createObjectStore).toHaveBeenCalledTimes(1); // client_id only
|
||||
});
|
||||
});
|
||||
|
||||
describe('full upgrade path (version 0 to 6)', () => {
|
||||
it('should create all stores and indexes when upgrading from version 0', () => {
|
||||
const { db, tx } = createMocks();
|
||||
|
||||
|
|
@ -239,8 +259,11 @@ describe('runDbUpgrade', () => {
|
|||
jasmine.anything(),
|
||||
);
|
||||
|
||||
// Total: 7 stores created
|
||||
expect(db.createObjectStore).toHaveBeenCalledTimes(7);
|
||||
// Version 6 store
|
||||
expect(db.createObjectStore).toHaveBeenCalledWith(STORE_NAMES.CLIENT_ID);
|
||||
|
||||
// Total: 8 stores created
|
||||
expect(db.createObjectStore).toHaveBeenCalledTimes(8);
|
||||
});
|
||||
|
||||
it('should create all indexes on ops store when upgrading from version 0', () => {
|
||||
|
|
|
|||
|
|
@ -67,4 +67,13 @@ export const runDbUpgrade = (
|
|||
if (oldVersion < 5) {
|
||||
db.createObjectStore(STORE_NAMES.PROFILE_DATA, { keyPath: 'id' });
|
||||
}
|
||||
|
||||
// Version 6: Add client_id store for atomic clientId rotation.
|
||||
// Consolidates the sync clientId from legacy 'pf' (key '__client_id_') into
|
||||
// SUP_OPS so destructive-flow rotation joins runDestructiveStateReplacement's
|
||||
// atomic transaction. See issue #7732. The runtime copy from 'pf' happens in
|
||||
// ClientIdService (a versionchange tx cannot read another database).
|
||||
if (oldVersion < 6) {
|
||||
db.createObjectStore(STORE_NAMES.CLIENT_ID);
|
||||
}
|
||||
};
|
||||
|
|
|
|||
|
|
@ -47,7 +47,7 @@ describe('OperationLogMigrationService', () => {
|
|||
mockMatDialog = jasmine.createSpyObj('MatDialog', ['open']);
|
||||
mockStore = jasmine.createSpyObj('Store', ['dispatch']);
|
||||
mockClientIdService = jasmine.createSpyObj('ClientIdService', [
|
||||
'generateNewClientId',
|
||||
'getOrGenerateClientId',
|
||||
'persistClientId',
|
||||
]);
|
||||
mockTranslateService = jasmine.createSpyObj('TranslateService', [
|
||||
|
|
@ -373,7 +373,7 @@ describe('OperationLogMigrationService', () => {
|
|||
const meta = await mockLegacyPfDb.loadMetaModel();
|
||||
const legacyClientId = await mockLegacyPfDb.loadClientId();
|
||||
const clientId =
|
||||
legacyClientId || (await mockClientIdService.generateNewClientId());
|
||||
legacyClientId ?? (await mockClientIdService.getOrGenerateClientId());
|
||||
|
||||
if (legacyClientId) {
|
||||
await mockClientIdService.persistClientId(legacyClientId);
|
||||
|
|
@ -427,18 +427,18 @@ describe('OperationLogMigrationService', () => {
|
|||
expect(mockClientIdService.persistClientId).toHaveBeenCalledWith(
|
||||
'legacyClientId1234',
|
||||
);
|
||||
expect(mockClientIdService.generateNewClientId).not.toHaveBeenCalled();
|
||||
expect(mockClientIdService.getOrGenerateClientId).not.toHaveBeenCalled();
|
||||
expect(mockOpLogStore.append).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should generate new client ID and NOT call persistClientId when legacy ID is null', async () => {
|
||||
mockLegacyPfDb.loadClientId.and.resolveTo(null);
|
||||
mockLegacyPfDb.loadMetaModel.and.resolveTo({ vectorClock: {} });
|
||||
mockClientIdService.generateNewClientId.and.resolveTo('B_xYz1');
|
||||
mockClientIdService.getOrGenerateClientId.and.resolveTo('B_xYz1');
|
||||
|
||||
await service.checkAndMigrate();
|
||||
|
||||
expect(mockClientIdService.generateNewClientId).toHaveBeenCalled();
|
||||
expect(mockClientIdService.getOrGenerateClientId).toHaveBeenCalled();
|
||||
expect(mockClientIdService.persistClientId).not.toHaveBeenCalled();
|
||||
expect(mockOpLogStore.append).toHaveBeenCalled();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -235,18 +235,28 @@ export class OperationLogMigrationService {
|
|||
OpLog.normal('OperationLogMigrationService: Data repair successful');
|
||||
}
|
||||
|
||||
// 3. Get client ID (inherit from legacy or generate new)
|
||||
// 3. Get client ID (inherit from legacy or resolve via ClientIdService).
|
||||
// The genesis op's clientId MUST come from the legacy PFAPI `CLIENT_ID`
|
||||
// key, because meta.vectorClock below is keyed by that same identity.
|
||||
// getOrGenerateClientId() is the fallback only when `CLIENT_ID` is absent:
|
||||
// it resolves whatever id this device already has (e.g. pf `__client_id_`)
|
||||
// and generates a fresh one only when no id exists anywhere.
|
||||
const meta = await this.legacyPfDb.loadMetaModel();
|
||||
const legacyClientId = await this.legacyPfDb.loadClientId();
|
||||
const clientId = legacyClientId || (await this.clientIdService.generateNewClientId());
|
||||
const clientId =
|
||||
legacyClientId ?? (await this.clientIdService.getOrGenerateClientId());
|
||||
|
||||
// Persist legacy client ID under __client_id_ so loadClientId() finds it.
|
||||
// Persist the legacy client ID into SUP_OPS so loadClientId() finds it.
|
||||
// Without this, a brand new ID is generated on next write, doubling IDs.
|
||||
if (legacyClientId) {
|
||||
await this.clientIdService.persistClientId(legacyClientId);
|
||||
}
|
||||
|
||||
OpLog.normal(`OperationLogMigrationService: Using client ID: ${clientId}`);
|
||||
// Log only a short suffix — the literal clientId keys the vector clock and
|
||||
// log history is user-exportable (CLAUDE.md sync rule 9).
|
||||
OpLog.normal('OperationLogMigrationService: Resolved client ID', {
|
||||
clientIdSuffix: clientId.slice(-3),
|
||||
});
|
||||
|
||||
// 4. Create MIGRATION genesis operation
|
||||
const migrationOp: Operation = {
|
||||
|
|
|
|||
|
|
@ -30,6 +30,7 @@ describe('OperationLogStoreService', () => {
|
|||
const mockClientIdProvider: ClientIdProvider = {
|
||||
loadClientId: () => Promise.resolve('testClient'),
|
||||
getOrGenerateClientId: () => Promise.resolve('testClient'),
|
||||
clearCache: () => {},
|
||||
};
|
||||
|
||||
// Helper to create test operations
|
||||
|
|
@ -1712,6 +1713,62 @@ describe('OperationLogStoreService', () => {
|
|||
priorArchiveOld,
|
||||
);
|
||||
});
|
||||
|
||||
it('writes the rotated clientId into the client_id store and clears the cache', async () => {
|
||||
const clearCacheSpy = spyOn(mockClientIdProvider, 'clearCache');
|
||||
|
||||
await service.runDestructiveStateReplacement({
|
||||
syncImportOp: createTestOperation({
|
||||
opType: OpType.SyncImport,
|
||||
entityType: 'ALL' as EntityType,
|
||||
entityId: undefined,
|
||||
clientId: 'rotatedClient',
|
||||
vectorClock: { rotatedClient: 1 },
|
||||
payload: { task: { ids: [], entities: {} } },
|
||||
}),
|
||||
snapshotEntityKeys: [],
|
||||
});
|
||||
|
||||
const db = (service as any).db;
|
||||
expect(await db.get(STORE_NAMES.CLIENT_ID, SINGLETON_KEY)).toBe('rotatedClient');
|
||||
// Cache-clear runs after a committed tx.done so the next clientId read
|
||||
// sees the rotated value.
|
||||
expect(clearCacheSpy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('leaves the client_id store and cache untouched when the destructive tx aborts', async () => {
|
||||
const db = (service as any).db;
|
||||
// Seed a prior clientId — the aborted put must roll back to this.
|
||||
await db.put(STORE_NAMES.CLIENT_ID, 'priorClient', SINGLETON_KEY);
|
||||
const clearCacheSpy = spyOn(mockClientIdProvider, 'clearCache');
|
||||
|
||||
const realTransaction = db.transaction.bind(db);
|
||||
spyOn(db, 'transaction').and.callFake((stores: any, mode: any) => {
|
||||
const tx = realTransaction(stores, mode);
|
||||
if (Array.isArray(stores) && stores.includes(STORE_NAMES.OPS)) {
|
||||
const opsStore = tx.objectStore(STORE_NAMES.OPS);
|
||||
opsStore.add = async () => {
|
||||
throw new Error('Simulated interrupt inside destructive tx');
|
||||
};
|
||||
}
|
||||
return tx;
|
||||
});
|
||||
|
||||
await expectAsync(
|
||||
service.runDestructiveStateReplacement({
|
||||
syncImportOp: createTestOperation({
|
||||
opType: OpType.SyncImport,
|
||||
entityType: 'ALL' as EntityType,
|
||||
clientId: 'abortClient',
|
||||
vectorClock: { abortClient: 1 },
|
||||
}),
|
||||
snapshotEntityKeys: [],
|
||||
}),
|
||||
).toBeRejected();
|
||||
|
||||
expect(await db.get(STORE_NAMES.CLIENT_ID, SINGLETON_KEY)).toBe('priorClient');
|
||||
expect(clearCacheSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('appendWithVectorClockUpdate', () => {
|
||||
|
|
|
|||
|
|
@ -158,6 +158,15 @@ interface OpLogDB extends DBSchema {
|
|||
key: string; // profile ID
|
||||
value: ProfileDataStoreEntry;
|
||||
};
|
||||
/**
|
||||
* Stores the sync clientId (device identity). Consolidated from legacy 'pf'
|
||||
* in version 6 so destructive-flow rotation joins the atomic transaction in
|
||||
* runDestructiveStateReplacement. See issue #7732.
|
||||
*/
|
||||
[STORE_NAMES.CLIENT_ID]: {
|
||||
key: string; // SINGLETON_KEY ('current')
|
||||
value: string; // the clientId
|
||||
};
|
||||
}
|
||||
|
||||
type OpLogStoreName = (typeof STORE_NAMES)[keyof typeof STORE_NAMES];
|
||||
|
|
@ -1207,6 +1216,7 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
STORE_NAMES.ARCHIVE_YOUNG,
|
||||
STORE_NAMES.ARCHIVE_OLD,
|
||||
STORE_NAMES.PROFILE_DATA,
|
||||
STORE_NAMES.CLIENT_ID,
|
||||
],
|
||||
'readwrite',
|
||||
);
|
||||
|
|
@ -1217,6 +1227,7 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
await tx.objectStore(STORE_NAMES.ARCHIVE_YOUNG).clear();
|
||||
await tx.objectStore(STORE_NAMES.ARCHIVE_OLD).clear();
|
||||
await tx.objectStore(STORE_NAMES.PROFILE_DATA).clear();
|
||||
await tx.objectStore(STORE_NAMES.CLIENT_ID).clear();
|
||||
await tx.done;
|
||||
// Invalidate all caches
|
||||
this._appliedOpIdsCache = null;
|
||||
|
|
@ -1568,12 +1579,12 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
* `isWhollyFreshClient + meaningful store data` branch on next launch.
|
||||
*
|
||||
* If any step throws, the IndexedDB transaction aborts and no committed
|
||||
* change to OPS / STATE_CACHE / VECTOR_CLOCK survives.
|
||||
* change to OPS / STATE_CACHE / VECTOR_CLOCK / CLIENT_ID survives.
|
||||
*
|
||||
* Atomicity holds within the `SUP_OPS` database only. Callers that rotate
|
||||
* the clientId (stored in the separate `pf` database) own that rollback —
|
||||
* including the no-prior-clientId edge case, where the rotated id is
|
||||
* intentionally left in `pf` on failure (see ClientIdService.withRotation).
|
||||
* The clientId now lives in `SUP_OPS` (`client_id` store, since schema v6),
|
||||
* so the rotated id on `syncImportOp.clientId` is written inside this same
|
||||
* transaction and rotates atomically with OPS / STATE_CACHE / VECTOR_CLOCK.
|
||||
* No cross-database two-phase commit is needed (issue #7732).
|
||||
*
|
||||
* The new baseline is taken entirely from `syncImportOp`: its `payload` is
|
||||
* written to OPS (the snapshot the uploader sends) and re-used as the
|
||||
|
|
@ -1598,6 +1609,9 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
STORE_NAMES.OPS,
|
||||
STORE_NAMES.STATE_CACHE,
|
||||
STORE_NAMES.VECTOR_CLOCK,
|
||||
// Unconditional: both callers (clean-slate, backup-restore) always rotate
|
||||
// the clientId. Unlike the archive stores it is never conditional.
|
||||
STORE_NAMES.CLIENT_ID,
|
||||
];
|
||||
if (archiveYoung != null) {
|
||||
storeNames.push(STORE_NAMES.ARCHIVE_YOUNG);
|
||||
|
|
@ -1612,6 +1626,15 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
const stateCacheStore = tx.objectStore(STORE_NAMES.STATE_CACHE);
|
||||
const vcStore = tx.objectStore(STORE_NAMES.VECTOR_CLOCK);
|
||||
|
||||
// Rotate the clientId first, inside this same atomic transaction. Writing
|
||||
// it before opsStore.clear() means an interrupt injected into a later
|
||||
// step still aborts this queued put — exercising the genuine
|
||||
// "queued -> tx aborts -> client_id unchanged" path. Atomicity itself is
|
||||
// order-independent.
|
||||
await tx
|
||||
.objectStore(STORE_NAMES.CLIENT_ID)
|
||||
.put(syncImportOp.clientId, SINGLETON_KEY);
|
||||
|
||||
await opsStore.clear();
|
||||
|
||||
const entry: Omit<StoredOperationLogEntry, 'seq'> = {
|
||||
|
|
@ -1657,6 +1680,11 @@ export class OperationLogStoreService implements RemoteOperationApplyStorePort<O
|
|||
this._cacheLastSeq = 0;
|
||||
this._invalidateUnsyncedCache();
|
||||
this._vectorClockCache = newVectorClock;
|
||||
// The clientId rotated atomically with the stores above. Invalidate the
|
||||
// ClientIdService cache so the next read sees the rotated value. Bound to
|
||||
// a committed `tx.done`: on catch/abort this is not reached, so the cache
|
||||
// correctly keeps the old id.
|
||||
this.clientIdProvider.clearCache();
|
||||
} catch (e) {
|
||||
// idb auto-aborts the tx on any rejected request, but a spy that
|
||||
// throws synchronously instead of rejecting the IDB request (used by
|
||||
|
|
|
|||
|
|
@ -54,7 +54,6 @@ describe('SyncHydrationService', () => {
|
|||
mockOpLogStore.loadStateCache.and.resolveTo(null);
|
||||
mockClientIdService = jasmine.createSpyObj('ClientIdService', [
|
||||
'loadClientId',
|
||||
'generateNewClientId',
|
||||
'getOrGenerateClientId',
|
||||
]);
|
||||
mockVectorClockService = jasmine.createSpyObj('VectorClockService', [
|
||||
|
|
|
|||
|
|
@ -103,8 +103,10 @@ describe('regression #7700: operation-log lock reentry', () => {
|
|||
const immediateUploadSpy = jasmine.createSpyObj('ImmediateUploadService', [
|
||||
'trigger',
|
||||
]);
|
||||
const clientIdSpy = jasmine.createSpyObj('ClientIdService', ['loadClientId']);
|
||||
clientIdSpy.loadClientId.and.resolveTo('testClient');
|
||||
const clientIdSpy = jasmine.createSpyObj('ClientIdService', [
|
||||
'getOrGenerateClientId',
|
||||
]);
|
||||
clientIdSpy.getOrGenerateClientId.and.resolveTo('testClient');
|
||||
|
||||
const operationCaptureSpy = jasmine.createSpyObj('OperationCaptureService', [
|
||||
'dequeue',
|
||||
|
|
|
|||
|
|
@ -0,0 +1,148 @@
|
|||
import { TestBed } from '@angular/core/testing';
|
||||
import { provideMockStore } from '@ngrx/store/testing';
|
||||
import { OperationLogStoreService } from '../../persistence/operation-log-store.service';
|
||||
import { StateSnapshotService } from '../../backup/state-snapshot.service';
|
||||
import { ArchiveDbAdapter } from '../../../core/persistence/archive-db-adapter.service';
|
||||
import { ArchiveOperationHandler } from '../../apply/archive-operation-handler.service';
|
||||
import { ArchiveService } from '../../../features/archive/archive.service';
|
||||
import { TaskArchiveService } from '../../../features/archive/task-archive.service';
|
||||
import { TimeTrackingService } from '../../../features/time-tracking/time-tracking.service';
|
||||
import { loadAllData } from '../../../root-store/meta/load-all-data.action';
|
||||
import { OpType } from '../../core/operation.types';
|
||||
import { PersistentAction } from '../../core/persistent-action.interface';
|
||||
import { ArchiveModel } from '../../../features/archive/archive.model';
|
||||
|
||||
/**
|
||||
* Integration test for archive data surviving the REPAIR-op round-trip,
|
||||
* exercised against real IndexedDB.
|
||||
*
|
||||
* Guards the *consumer* side of the bug where a data repair on one client
|
||||
* wiped archived tasks on every *other* client. The *producer* side
|
||||
* (`validateAndRepairCurrentState()` calling `getStateSnapshotAsync()`) is
|
||||
* guarded by the unit spec `validate-state.service.spec.ts`; these tests do
|
||||
* not exercise `validateAndRepairCurrentState()` itself. The bug had two parts:
|
||||
*
|
||||
* 1. `validateAndRepairCurrentState()` built the REPAIR op from the synchronous
|
||||
* `getStateSnapshot()`, which hardcodes empty archives (archiveYoung/archiveOld
|
||||
* live in IndexedDB, not NgRx state).
|
||||
* 2. Other clients applied that REPAIR op via `ArchiveOperationHandler`, whose
|
||||
* empty-archive guard only covered SYNC_IMPORT/BACKUP_IMPORT — so a REPAIR op
|
||||
* with empty archives overwrote (wiped) the local archive.
|
||||
*
|
||||
* These tests wire the *real* `StateSnapshotService`, `ArchiveDbAdapter`,
|
||||
* `ArchiveStoreService` and `ArchiveOperationHandler` against real IndexedDB,
|
||||
* so they exercise the actual archive read/write seams — not mocks.
|
||||
*
|
||||
* The complementary unit spec `validate-state.service.spec.ts` proves that
|
||||
* `validateAndRepairCurrentState()` itself now calls `getStateSnapshotAsync()`.
|
||||
*/
|
||||
describe('Archive REPAIR round-trip integration', () => {
|
||||
let storeService: OperationLogStoreService;
|
||||
let stateSnapshot: StateSnapshotService;
|
||||
let archiveDb: ArchiveDbAdapter;
|
||||
let archiveHandler: ArchiveOperationHandler;
|
||||
|
||||
// Cast: the archive round-trip only reads `task.ids`; entity contents are
|
||||
// stored/loaded as an opaque blob, so minimal task stubs are sufficient.
|
||||
const archiveModel = (taskIds: string[]): ArchiveModel =>
|
||||
({
|
||||
task: {
|
||||
ids: taskIds,
|
||||
entities: Object.fromEntries(
|
||||
taskIds.map((id) => [id, { id, title: `Archived ${id}`, isDone: true }]),
|
||||
),
|
||||
},
|
||||
timeTracking: { project: {}, tag: {} },
|
||||
lastTimeTrackingFlush: 0,
|
||||
}) as unknown as ArchiveModel;
|
||||
|
||||
/** Builds the action `OperationApplierService` dispatches when applying a REPAIR op. */
|
||||
const repairApplyAction = (appDataComplete: unknown): PersistentAction =>
|
||||
({
|
||||
...loadAllData({ appDataComplete: appDataComplete as never }),
|
||||
meta: { isPersistent: true, isRemote: true, opType: OpType.Repair },
|
||||
}) as unknown as PersistentAction;
|
||||
|
||||
beforeEach(async () => {
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
provideMockStore(),
|
||||
OperationLogStoreService,
|
||||
StateSnapshotService,
|
||||
ArchiveDbAdapter,
|
||||
ArchiveOperationHandler,
|
||||
// Lazy deps of ArchiveOperationHandler — never reached by the loadAllData
|
||||
// path, stubbed so construction does not pull their real dependency trees.
|
||||
{ provide: ArchiveService, useValue: {} },
|
||||
{ provide: TaskArchiveService, useValue: {} },
|
||||
{ provide: TimeTrackingService, useValue: {} },
|
||||
],
|
||||
});
|
||||
|
||||
storeService = TestBed.inject(OperationLogStoreService);
|
||||
stateSnapshot = TestBed.inject(StateSnapshotService);
|
||||
archiveDb = TestBed.inject(ArchiveDbAdapter);
|
||||
archiveHandler = TestBed.inject(ArchiveOperationHandler);
|
||||
|
||||
await storeService.init();
|
||||
await storeService._clearAllDataForTesting();
|
||||
// Archive stores are SUP_OPS singletons shared across tests — reset explicitly.
|
||||
await archiveDb.saveArchiveYoung(archiveModel([]));
|
||||
await archiveDb.saveArchiveOld(archiveModel([]));
|
||||
});
|
||||
|
||||
it('getStateSnapshotAsync() includes IndexedDB archives; getStateSnapshot() returns them empty', async () => {
|
||||
await archiveDb.saveArchiveYoung(archiveModel(['y1', 'y2']));
|
||||
await archiveDb.saveArchiveOld(archiveModel(['o1']));
|
||||
|
||||
// The sync snapshot hardcodes empty archives — this is what made REPAIR ops
|
||||
// built from it wipe other clients.
|
||||
const syncSnap = stateSnapshot.getStateSnapshot();
|
||||
expect(syncSnap.archiveYoung.task.ids).toEqual([]);
|
||||
expect(syncSnap.archiveOld.task.ids).toEqual([]);
|
||||
|
||||
// The async snapshot loads the real archives — the REPAIR op now relies on this.
|
||||
const asyncSnap = await stateSnapshot.getStateSnapshotAsync();
|
||||
expect(asyncSnap.archiveYoung.task.ids).toEqual(['y1', 'y2']);
|
||||
expect(asyncSnap.archiveOld.task.ids).toEqual(['o1']);
|
||||
});
|
||||
|
||||
it('archive round-trips from client A IndexedDB through a REPAIR op into client B IndexedDB', async () => {
|
||||
// --- Client A has archived tasks ---
|
||||
await archiveDb.saveArchiveYoung(archiveModel(['y1', 'y2']));
|
||||
await archiveDb.saveArchiveOld(archiveModel(['o1']));
|
||||
|
||||
// Client A builds the REPAIR op payload from the async snapshot (the fix).
|
||||
const repairAppData = await stateSnapshot.getStateSnapshotAsync();
|
||||
|
||||
// --- Handoff: client B is a fresh client with no archive ---
|
||||
await archiveDb.saveArchiveYoung(archiveModel([]));
|
||||
await archiveDb.saveArchiveOld(archiveModel([]));
|
||||
expect((await archiveDb.loadArchiveYoung())!.task.ids).toEqual([]);
|
||||
expect((await archiveDb.loadArchiveOld())!.task.ids).toEqual([]);
|
||||
|
||||
// --- Client B applies the REPAIR op ---
|
||||
await archiveHandler.handleOperation(repairApplyAction(repairAppData));
|
||||
|
||||
// --- Client B now has client A's archive ---
|
||||
expect((await archiveDb.loadArchiveYoung())!.task.ids).toEqual(['y1', 'y2']);
|
||||
expect((await archiveDb.loadArchiveOld())!.task.ids).toEqual(['o1']);
|
||||
});
|
||||
|
||||
it('a REPAIR op built from the sync snapshot has empty archives and no longer wipes a client that has data', async () => {
|
||||
// --- Client B already has archived tasks ---
|
||||
await archiveDb.saveArchiveYoung(archiveModel(['keep-young-1']));
|
||||
await archiveDb.saveArchiveOld(archiveModel(['keep-old-1']));
|
||||
|
||||
// A pre-fix REPAIR op, built from the sync snapshot, carries empty archives.
|
||||
const buggyAppData = stateSnapshot.getStateSnapshot();
|
||||
expect(buggyAppData.archiveYoung.task.ids).toEqual([]);
|
||||
expect(buggyAppData.archiveOld.task.ids).toEqual([]);
|
||||
|
||||
await archiveHandler.handleOperation(repairApplyAction(buggyAppData));
|
||||
|
||||
// The empty-archive guard preserves the local archive instead of wiping it.
|
||||
expect((await archiveDb.loadArchiveYoung())!.task.ids).toEqual(['keep-young-1']);
|
||||
expect((await archiveDb.loadArchiveOld())!.task.ids).toEqual(['keep-old-1']);
|
||||
});
|
||||
});
|
||||
|
|
@ -2,12 +2,11 @@ import { TestBed } from '@angular/core/testing';
|
|||
import { OperationLogStoreService } from '../../persistence/operation-log-store.service';
|
||||
import { CleanSlateService } from '../../clean-slate/clean-slate.service';
|
||||
import { StateSnapshotService } from '../../backup/state-snapshot.service';
|
||||
import { ClientIdService } from '../../../core/util/client-id.service';
|
||||
import { SyncLocalStateService } from '../../sync/sync-local-state.service';
|
||||
import { TranslateService } from '@ngx-translate/core';
|
||||
import { CURRENT_SCHEMA_VERSION } from '../../persistence/schema-migration.service';
|
||||
import { Operation } from '../../core/operation.types';
|
||||
import { STORE_NAMES } from '../../persistence/db-keys.const';
|
||||
import { SINGLETON_KEY, STORE_NAMES } from '../../persistence/db-keys.const';
|
||||
|
||||
/**
|
||||
* Integration tests for issue #7709 — `createCleanSlate` / `BackupService` import
|
||||
|
|
@ -30,7 +29,6 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
let syncLocalState: SyncLocalStateService;
|
||||
let cleanSlate: CleanSlateService;
|
||||
let mockStateSnapshot: jasmine.SpyObj<StateSnapshotService>;
|
||||
let mockClientId: jasmine.SpyObj<ClientIdService>;
|
||||
let mockTranslate: jasmine.SpyObj<TranslateService>;
|
||||
|
||||
const meaningfulState = {
|
||||
|
|
@ -53,14 +51,6 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
mockStateSnapshot.getStateSnapshot.and.returnValue(meaningfulState as any);
|
||||
mockStateSnapshot.getStateSnapshotAsync.and.resolveTo(meaningfulState as any);
|
||||
|
||||
mockClientId = jasmine.createSpyObj('ClientIdService', ['withRotation']);
|
||||
// Default: invoke the rotation callback with a fresh id. Rollback
|
||||
// semantics are exercised in ClientIdService's own spec.
|
||||
mockClientId.withRotation.and.callFake(
|
||||
async (_logPrefix: string, fn: (newClientId: string) => Promise<any>) =>
|
||||
fn('cNew1'),
|
||||
);
|
||||
|
||||
mockTranslate = jasmine.createSpyObj('TranslateService', ['instant']);
|
||||
mockTranslate.instant.and.callFake((k: string) => k);
|
||||
|
||||
|
|
@ -70,7 +60,6 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
SyncLocalStateService,
|
||||
CleanSlateService,
|
||||
{ provide: StateSnapshotService, useValue: mockStateSnapshot },
|
||||
{ provide: ClientIdService, useValue: mockClientId },
|
||||
{ provide: TranslateService, useValue: mockTranslate },
|
||||
],
|
||||
});
|
||||
|
|
@ -94,6 +83,13 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
expect(cache).not.toBeNull();
|
||||
expect(cache!.state).toEqual(meaningfulState as any);
|
||||
expect(await syncLocalState.isWhollyFreshClient()).toBe(false);
|
||||
|
||||
// The clientId rotated atomically inside the destructive replacement.
|
||||
const rotatedId = await (storeService as any).db.get(
|
||||
STORE_NAMES.CLIENT_ID,
|
||||
SINGLETON_KEY,
|
||||
);
|
||||
expect(rotatedId).toMatch(/^[BEAI]_[a-zA-Z0-9]{4}$/);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -108,8 +104,8 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
entityType: 'TASK' as any,
|
||||
entityId: `t${i}`,
|
||||
payload: { id: `t${i}` },
|
||||
clientId: 'cPrior',
|
||||
vectorClock: { cPrior: i + 1 },
|
||||
clientId: 'cPriorClient',
|
||||
vectorClock: { cPriorClient: i + 1 },
|
||||
timestamp: Date.now() + i,
|
||||
schemaVersion: CURRENT_SCHEMA_VERSION,
|
||||
}));
|
||||
|
|
@ -119,11 +115,15 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
await storeService.saveStateCache({
|
||||
state: { sentinel: 'prior-state' } as any,
|
||||
lastAppliedOpSeq: 0,
|
||||
vectorClock: { cPrior: 3 },
|
||||
vectorClock: { cPriorClient: 3 },
|
||||
compactedAt: Date.now(),
|
||||
schemaVersion: CURRENT_SCHEMA_VERSION,
|
||||
});
|
||||
await storeService.setVectorClock({ cPrior: 3 });
|
||||
await storeService.setVectorClock({ cPriorClient: 3 });
|
||||
|
||||
// Seed a prior clientId in SUP_OPS — the aborted rotation must not touch
|
||||
// it. This is the property withRotation used to provide by hand (#7732).
|
||||
await (storeService as any).db.put(STORE_NAMES.CLIENT_ID, 'B_seed', SINGLETON_KEY);
|
||||
|
||||
const seqBefore = await storeService.getLastSeq();
|
||||
const cacheBefore = await storeService.loadStateCache();
|
||||
|
|
@ -159,6 +159,11 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
expect((cacheAfter!.state as any).sentinel).toBe('prior-state');
|
||||
expect(cacheAfter!.vectorClock).toEqual(cacheBefore!.vectorClock);
|
||||
expect(await storeService.getVectorClock()).toEqual(clockBefore);
|
||||
// The rotated clientId was queued first inside the tx; the abort unwinds
|
||||
// it — SUP_OPS.client_id still holds the prior id.
|
||||
expect(
|
||||
await (storeService as any).db.get(STORE_NAMES.CLIENT_ID, SINGLETON_KEY),
|
||||
).toBe('B_seed');
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -172,8 +177,8 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => {
|
|||
entityType: 'TASK' as any,
|
||||
entityId: `t${i}`,
|
||||
payload: { id: `t${i}` },
|
||||
clientId: 'cPrior',
|
||||
vectorClock: { cPrior: i + 1 },
|
||||
clientId: 'cPriorClient',
|
||||
vectorClock: { cPriorClient: i + 1 },
|
||||
timestamp: Date.now() + i,
|
||||
schemaVersion: CURRENT_SCHEMA_VERSION,
|
||||
}));
|
||||
|
|
|
|||
|
|
@ -38,7 +38,7 @@ describe('Legacy Data Migration Integration', () => {
|
|||
'loadClientId',
|
||||
]);
|
||||
mockClientIdService = jasmine.createSpyObj('ClientIdService', [
|
||||
'generateNewClientId',
|
||||
'getOrGenerateClientId',
|
||||
]);
|
||||
mockMatDialog = jasmine.createSpyObj('MatDialog', ['open']);
|
||||
mockTranslateService = jasmine.createSpyObj('TranslateService', [
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ describe('RemoteOperationApplyStorePort Integration', () => {
|
|||
const mockClientIdProvider: ClientIdProvider = {
|
||||
loadClientId: () => Promise.resolve('testClient'),
|
||||
getOrGenerateClientId: () => Promise.resolve('testClient'),
|
||||
clearCache: () => {},
|
||||
};
|
||||
|
||||
const createOperation = (
|
||||
|
|
|
|||
|
|
@ -122,7 +122,7 @@ describe('done task operation replay', () => {
|
|||
['trigger'],
|
||||
);
|
||||
const mockClientIdService = jasmine.createSpyObj<ClientIdService>('ClientIdService', [
|
||||
'loadClientId',
|
||||
'getOrGenerateClientId',
|
||||
]);
|
||||
const mockSuperSyncStatusService = jasmine.createSpyObj<SuperSyncStatusService>(
|
||||
'SuperSyncStatusService',
|
||||
|
|
@ -140,7 +140,7 @@ describe('done task operation replay', () => {
|
|||
mockCompactionService.compact.and.returnValue(Promise.resolve());
|
||||
mockCompactionService.emergencyCompact.and.returnValue(Promise.resolve(true));
|
||||
mockOperationCaptureService.dequeue.and.returnValue([]);
|
||||
mockClientIdService.loadClientId.and.returnValue(Promise.resolve('clientA'));
|
||||
mockClientIdService.getOrGenerateClientId.and.returnValue(Promise.resolve('clientA'));
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
|
|
|
|||
|
|
@ -9,10 +9,9 @@ describe('CLIENT_ID_PROVIDER', () => {
|
|||
beforeEach(() => {
|
||||
mockClientIdService = jasmine.createSpyObj('ClientIdService', [
|
||||
'loadClientId',
|
||||
'generateNewClientId',
|
||||
'getOrGenerateClientId',
|
||||
'clearCache',
|
||||
]);
|
||||
mockClientIdService.generateNewClientId.and.resolveTo('B_new1');
|
||||
mockClientIdService.getOrGenerateClientId.and.resolveTo('test-client-id-123');
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
|
|
@ -59,4 +58,10 @@ describe('CLIENT_ID_PROVIDER', () => {
|
|||
expect(result).toBe('B_a7Kx');
|
||||
expect(mockClientIdService.getOrGenerateClientId).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('should delegate clearCache to ClientIdService', () => {
|
||||
provider.clearCache();
|
||||
|
||||
expect(mockClientIdService.clearCache).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -15,13 +15,27 @@ import { ClientIdService } from '../../core/util/client-id.service';
|
|||
* - ClientIdService handles lazy PfapiService resolution
|
||||
*/
|
||||
export interface ClientIdProvider {
|
||||
/**
|
||||
* Returns the stored client ID, or null if none exists (or a read failed).
|
||||
*
|
||||
* SIDE EFFECT: the first read of a device that still has its clientId only
|
||||
* in the legacy 'pf' database copies it forward into SUP_OPS (one-time,
|
||||
* idempotent migration — see ClientIdService). All later reads are cached.
|
||||
*/
|
||||
loadClientId(): Promise<string | null>;
|
||||
/**
|
||||
* Returns the stored client ID, or generates and persists a new one if
|
||||
* none is stored or the stored value is invalid. Preferred over calling
|
||||
* loadClientId() with a manual fallback.
|
||||
* none is stored. Preferred over calling loadClientId() with a manual
|
||||
* fallback. Propagates IndexedDB read failures (it must never mint a fresh
|
||||
* id on a transient error — that would orphan the device's real identity).
|
||||
*/
|
||||
getOrGenerateClientId(): Promise<string>;
|
||||
/**
|
||||
* Invalidates the in-memory clientId cache so the next read re-resolves
|
||||
* from IndexedDB. Called by runDestructiveStateReplacement after it rotates
|
||||
* the clientId inside the atomic SUP_OPS transaction.
|
||||
*/
|
||||
clearCache(): void;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -45,6 +59,7 @@ export const CLIENT_ID_PROVIDER = new InjectionToken<ClientIdProvider>(
|
|||
return {
|
||||
loadClientId: () => clientIdService.loadClientId(),
|
||||
getOrGenerateClientId: () => clientIdService.getOrGenerateClientId(),
|
||||
clearCache: () => clientIdService.clearCache(),
|
||||
};
|
||||
},
|
||||
},
|
||||
|
|
|
|||
|
|
@ -64,6 +64,7 @@ describe('ValidateStateService', () => {
|
|||
|
||||
mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [
|
||||
'getStateSnapshot',
|
||||
'getStateSnapshotAsync',
|
||||
]);
|
||||
mockClientIdProvider = {
|
||||
loadClientId: jasmine
|
||||
|
|
@ -246,19 +247,16 @@ describe('ValidateStateService', () => {
|
|||
}
|
||||
});
|
||||
|
||||
it('should return true when state is valid', async () => {
|
||||
it('should return true (and skip the async archive snapshot) when state is valid', async () => {
|
||||
const validState = createEmptyState();
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(validState as any);
|
||||
|
||||
// Mock validateAndRepair to return valid state (empty state doesn't pass full Typia validation)
|
||||
spyOn(service, 'validateAndRepair').and.resolveTo({
|
||||
isValid: true,
|
||||
wasRepaired: false,
|
||||
});
|
||||
// Quick validation passes → the expensive async (archive) snapshot is skipped.
|
||||
spyOn(service, 'validateState').and.resolveTo({ isValid: true, typiaErrors: [] });
|
||||
|
||||
const result = await service.validateAndRepairCurrentState('test-context');
|
||||
|
||||
expect(result).toBeTrue();
|
||||
expect(mockStateSnapshotService.getStateSnapshotAsync).not.toHaveBeenCalled();
|
||||
expect(mockRepairService.createRepairOperation).not.toHaveBeenCalled();
|
||||
expect(store.dispatch).not.toHaveBeenCalled();
|
||||
});
|
||||
|
|
@ -275,6 +273,7 @@ describe('ValidateStateService', () => {
|
|||
projectTree: [{ id: 'ORPHAN', k: MenuTreeKind.PROJECT }],
|
||||
};
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as any);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(invalidState as any);
|
||||
mockClientIdProvider.loadClientId.and.returnValue(Promise.resolve(null));
|
||||
|
||||
const result = await service.validateAndRepairCurrentState('sync');
|
||||
|
|
@ -289,6 +288,7 @@ describe('ValidateStateService', () => {
|
|||
it('should pass skipLock option to repair service when callerHoldsLock is true', async () => {
|
||||
const state = createEmptyState();
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any);
|
||||
|
||||
// Mock validateAndRepair to return repaired state
|
||||
spyOn(service, 'validateAndRepair').and.resolveTo({
|
||||
|
|
@ -311,6 +311,7 @@ describe('ValidateStateService', () => {
|
|||
it('should start/end hydration state for sync contexts', async () => {
|
||||
const state = createEmptyState();
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any);
|
||||
|
||||
// Mock validateAndRepair to return repaired state
|
||||
spyOn(service, 'validateAndRepair').and.resolveTo({
|
||||
|
|
@ -338,6 +339,7 @@ describe('ValidateStateService', () => {
|
|||
projectTree: [{ id: 'ORPHAN', k: MenuTreeKind.PROJECT }],
|
||||
};
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as any);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(invalidState as any);
|
||||
|
||||
await service.validateAndRepairCurrentState('other-context');
|
||||
|
||||
|
|
@ -359,6 +361,7 @@ describe('ValidateStateService', () => {
|
|||
projectTree: [{ id: 'ORPHAN', k: MenuTreeKind.PROJECT }],
|
||||
};
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as any);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(invalidState as any);
|
||||
mockHydrationStateService.isApplyingRemoteOps.and.returnValue(true);
|
||||
|
||||
await service.validateAndRepairCurrentState('sync');
|
||||
|
|
@ -374,6 +377,7 @@ describe('ValidateStateService', () => {
|
|||
it('should dispatch loadAllData with isRemote flag for sync contexts', async () => {
|
||||
const state = createEmptyState();
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any);
|
||||
|
||||
// Mock validateAndRepair to return repaired state
|
||||
spyOn(service, 'validateAndRepair').and.resolveTo({
|
||||
|
|
@ -393,6 +397,7 @@ describe('ValidateStateService', () => {
|
|||
it('should dispatch loadAllData without isRemote flag for non-sync contexts', async () => {
|
||||
const state = createEmptyState();
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any);
|
||||
|
||||
// Mock validateAndRepair to return repaired state
|
||||
spyOn(service, 'validateAndRepair').and.resolveTo({
|
||||
|
|
@ -408,5 +413,50 @@ describe('ValidateStateService', () => {
|
|||
const dispatchedAction = (store.dispatch as jasmine.Spy).calls.mostRecent().args[0];
|
||||
expect(dispatchedAction.meta?.isRemote).toBeFalsy();
|
||||
});
|
||||
|
||||
// Regression: archives (archiveYoung/archiveOld) live in IndexedDB, not NgRx.
|
||||
// The sync getStateSnapshot() hardcodes empty archives, so a REPAIR op built
|
||||
// from it wiped archives on every other client that applied it. When a repair
|
||||
// is needed, the REPAIR op must be built from the async snapshot.
|
||||
it('should build the REPAIR op from the async snapshot so it carries archive data', async () => {
|
||||
const archivedTaskId = 'archived-task-1';
|
||||
const stateWithArchive = createEmptyState();
|
||||
stateWithArchive.archiveYoung = {
|
||||
task: {
|
||||
ids: [archivedTaskId],
|
||||
entities: { [archivedTaskId]: { id: archivedTaskId } },
|
||||
},
|
||||
timeTracking: initialTimeTrackingState,
|
||||
lastTimeTrackingFlush: 0,
|
||||
};
|
||||
// Quick (sync) validation fails → the repair path loads the async snapshot.
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(
|
||||
createEmptyState() as any,
|
||||
);
|
||||
mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(
|
||||
stateWithArchive as any,
|
||||
);
|
||||
|
||||
const validateAndRepairSpy = spyOn(service, 'validateAndRepair').and.resolveTo({
|
||||
isValid: true,
|
||||
wasRepaired: true,
|
||||
repairedState: stateWithArchive,
|
||||
repairSummary: { typeErrorsFixed: 1 } as any,
|
||||
});
|
||||
|
||||
await service.validateAndRepairCurrentState('sync');
|
||||
|
||||
// The repair path must load the async snapshot (archives from IndexedDB).
|
||||
expect(mockStateSnapshotService.getStateSnapshotAsync).toHaveBeenCalled();
|
||||
|
||||
// The state fed into validation/repair must include the archive...
|
||||
const validatedState = validateAndRepairSpy.calls.mostRecent().args[0];
|
||||
expect((validatedState.archiveYoung as any).task.ids).toEqual([archivedTaskId]);
|
||||
|
||||
// ...and so must the REPAIR operation payload.
|
||||
const repairedState = mockRepairService.createRepairOperation.calls.mostRecent()
|
||||
.args[0] as any;
|
||||
expect(repairedState.archiveYoung.task.ids).toEqual([archivedTaskId]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -99,7 +99,25 @@ export class ValidateStateService {
|
|||
`[ValidateStateService:${context}] Running post-operation validation...`,
|
||||
);
|
||||
|
||||
const currentState = this.stateSnapshotService.getStateSnapshot();
|
||||
// Validate cheaply first using the synchronous snapshot. archiveYoung/
|
||||
// archiveOld live in IndexedDB (not NgRx state), so the sync snapshot omits
|
||||
// them — but Checkpoint D only detects NgRx-entity corruption, so the sync
|
||||
// snapshot is enough to decide whether a repair is needed. This keeps the
|
||||
// common (valid) path off the IndexedDB archive reads that
|
||||
// getStateSnapshotAsync() performs on every sync.
|
||||
const quickState = this.stateSnapshotService.getStateSnapshot();
|
||||
const quickValidation = await this.validateState(
|
||||
quickState as unknown as Record<string, unknown>,
|
||||
);
|
||||
if (quickValidation.isValid) {
|
||||
OpLog.normal(`[ValidateStateService:${context}] State valid`);
|
||||
return true;
|
||||
}
|
||||
|
||||
// State is invalid — load the full snapshot including archives so the REPAIR
|
||||
// operation carries archive data. A REPAIR op built from the sync snapshot
|
||||
// would ship empty archives and wipe them on every client that applies it.
|
||||
const currentState = await this.stateSnapshotService.getStateSnapshotAsync();
|
||||
|
||||
const result = await this.validateAndRepair(
|
||||
currentState as unknown as Record<string, unknown>,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue