From 4bd4b705e977da68b1834f8a7567ea85ee510546 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Thu, 21 May 2026 12:24:52 +0200 Subject: [PATCH] docs(sync): correct JSDoc and onClick comment per multi-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two multi-review reviewers (Architecture, Alternatives) flagged that the calendar-effect JSDoc claimed `skipDuringSyncWindow()` only covers the `\!isInSyncWindow()` half. That is wrong — the operator literally computes `initialSyncDone && \!inSyncWindow` (skip-during-sync-window.operator.ts:65-82), which is byte-for-byte the same predicate as the inline guard. Rewrite the doc to acknowledge the equivalence and explain the real reason we don't use the operator here: side effects live inside `tap(async () => ...)` and the banner branch in the same tap must keep firing during sync. Note the follow-up refactor that would let us reuse the operator on a forked sub-stream. Three reviewers (Correctness, Architecture, Simplicity) also called out the misleading "mirroring the Disable flow above" comment on the Enable Encryption `onClick`. The Disable flow does one write to the root model; the new Enable flow does two writes (sub + root). Rewrite the comment to name `_deriveEncryptionStateForSuperSync` as the consumer that needs the sub-model write and the sibling hideExpressions as the consumer that needs the root write — making the asymmetry explicit instead of pretending the flows match. Refs: #7700 --- .../store/calendar-integration.effects.ts | 16 ++++++++++------ .../features/config/form-cfgs/sync-form.const.ts | 15 +++++++++++---- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/app/features/calendar-integration/store/calendar-integration.effects.ts b/src/app/features/calendar-integration/store/calendar-integration.effects.ts index ac269d201f..7fd23dfe23 100644 --- a/src/app/features/calendar-integration/store/calendar-integration.effects.ts +++ b/src/app/features/calendar-integration/store/calendar-integration.effects.ts @@ -55,12 +55,16 @@ export class CalendarIntegrationEffects { * Poll external calendar providers for events and auto-import them as tasks. * * The auto-import branch is gated on `isInitialSyncDoneSync() && - * !isInSyncWindow()`. The `!isInSyncWindow()` half is what - * `skipDuringSyncWindow()` would give us; the `isInitialSyncDoneSync()` - * half is stricter — it also blocks before the very first sync completes, - * which the operator alone does not cover. The combined gate can't be - * lifted to the outer pipe because the banner-display branch must keep - * firing during sync. + * !isInSyncWindow()` — the same predicate as `skipDuringSyncWindow()` + * (see `src/app/util/skip-during-sync-window.operator.ts`). We hand-roll + * it here rather than using the operator because: + * 1. The operator filters emissions on a stream; this effect runs its + * side effects inside `tap(async () => …)`, not on an emission. + * 2. The banner-display branch (further down in the same tap) must + * keep firing during sync, so the gate cannot be lifted to the + * outer pipe. + * Refactoring the tap into `exhaustMap(...) → skipDuringSyncWindow() → + * tap(import)` would let the operator be reused; left as a follow-up. * * Why the gate matters: calendar task IDs are deterministic across devices * (see `generateCalendarTaskId`), so a pre-first-sync import on a second diff --git a/src/app/features/config/form-cfgs/sync-form.const.ts b/src/app/features/config/form-cfgs/sync-form.const.ts index d11179ad8d..8bc6f3cdc5 100644 --- a/src/app/features/config/form-cfgs/sync-form.const.ts +++ b/src/app/features/config/form-cfgs/sync-form.const.ts @@ -551,10 +551,17 @@ export const SYNC_FORM: ConfigFormSection = { onClick: async (field: FormlyFieldConfig) => { const result = await openEnableEncryptionDialog(); if (result?.success) { - // Sub-model write keeps the SuperSync nested form section in - // sync; the root write is what the sibling hideExpressions - // (info-panel, ENCRYPTION_ENCOURAGED, this button) read, - // mirroring the Disable flow above. + // Two writes are needed (asymmetric with the Disable flow, + // which only needs the root write): + // - Sub-model: `sync-config.service._deriveEncryptionState + // ForSuperSync` reads `superSync.isEncryptionEnabled` + // first in its fallback chain (privateCfg comes second). + // Writing it here ensures the next form-save derives + // `true` even if `privateCfg` propagation lags. + // - Root: sibling hideExpressions (info-panel, + // ENCRYPTION_ENCOURAGED, this button) read the root + // flag, so it must flip in the current Formly tick to + // hide them immediately. if (field?.model) { field.model.isEncryptionEnabled = true; }