Mirrors/super-productivity
1
0
Fork 0
mirror of https://github.com/johannesjo/super-productivity.git synced 2026-07-17 16:37:43 +00:00
Code Issues Projects Releases Wiki Activity

docs(sync): correct JSDoc and onClick comment per multi-review

Browse source 
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
This commit is contained in:
Johannes Millan 2026-05-21 12:24:52 +02:00
parent 7423f2ce7c
commit e941aeeb81
2 changed files with 21 additions and 10 deletions
Download patch file Download diff file Expand all files Collapse all files

16
src/app/features/calendar-integration/store/calendar-integration.effects.ts
View file

@ -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

15
src/app/features/config/form-cfgs/sync-form.const.ts
View file

@ -551,10 +551,17 @@ export const SYNC_FORM: ConfigFormSection<SyncConfig> = {
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;
}