fix(op-log): count syncTimeSpent as touching the time fields on the receiver - #10076

Preview Deployment

Branch: fix/8758-deferred-entity-changes
Commit: 8c987d8

_{Deployed with Cloudflare Pages}

The Copilot finding on conflict-disjoint-merge.util.ts is correct and I verified it with production-shaped ops before changing anything: isDisjointMergeEligible returned true for a syncTimeSpent delta vs a concurrent { title } edit on the same task (delta on either side), because the extractor's { taskId, date, duration } is read as changed task fields. That is reachable on master today for direct writes; commit 1 would have extended it to deferred writes.

Fixed in 9e79a154f: an additive time op on either side makes the conflict ineligible for field-level merging → whole-entity LWW, the same fallback opaque ops get. Two regression tests in conflict-disjoint-merge.util.spec.ts (both failed before). The deferred conflict is covered by the same tests since commit 1's parity spec proves the op shape is identical on both paths.

PR description corrected — the earlier "conflict handling unaffected" line was wrong. Whether time-vs-edit should commute (apply both) rather than LWW is left as a maintainer call; happy to split commit 2 into its own PR if preferred.

🔧 Needs changes – labelled needs work; ping me once the items below are addressed and I'll re-review.

Thanks for the PR! Commit 1 is exactly what #8758 asks for and the parity spec is the right proof. Commit 2 fixes a real master bug (the synthesized { title, taskId, date, duration } patch), but the place it is fixed regresses a second conflict path that shares the same predicate.

Blocking

• src/app/op-log/sync/conflict-disjoint-merge.util.ts:252 – isDisjointMergeEligible has a second caller with the opposite meaning: _buildNoPendingConcurrentConflict (conflict-resolution.service.ts:4643, op-log: concurrent already-synced updates diverge by arrival order (no deterministic reconciliation in the no-pending branch) #9073) reads true as "this crossing commutes, apply both sides as-is, forward no conflict". On master a retained (already synced) non-time edit crossing a remote syncTimeSpent op on the same task, with no pending local ops, passes every guard at 4572-4636 (the time-vs-time exemption at 4606 does not fire because the local side is a plain task edit), then isDisjointMergeEligible sees { taskId, date, duration } vs e.g. { isDone } and returns true → null → both sides survive on both clients. With line 252 the same crossing returns false, the conflict is forwarded and resolved by whole-entity LWW. The two clients pick opposite sides of the same pair, and the local-win side runs _createLocalWinUpdateOp (generic branch, conflict-resolution.service.ts:2809+), which emits a full-entity snapshot that dominates both clocks – so either the tracked time or the edit is discarded fleet-wide. Concrete case: desktop tracks time on task T while the phone marks T done; today both survive, with this PR one of them is lost.

  Fix: keep the exclusion out of the shared predicate and apply it only where a merged patch would be synthesized – in _tryCreateDisjointMergeOp (conflict-resolution.service.ts:2677) return undefined when [...localOps, ...remoteOps].some(isAdditiveTimeOp) before calling isDisjointMergeEligible (export the helper, or give the predicate a flag). For the no-pending caller, rather than reporting { taskId, date, duration } as the delta's changed fields, treat a syncTimeSpent op as touching timeSpent / timeSpentOnDay for the disjointness test: an edit that does not touch those keys keeps commuting (apply both, lossless), while an edit that overlaps them falls to LWW, which is the convergent choice there because the delta is additive and apply-both is order-dependent. Please add a spec in the _buildNoPendingConcurrentConflict describe of conflict-resolution.service.spec.ts (the detect() helper near line 7415, next to "keeps concurrent task-time deltas non-conflicting") pinning that a syncTimeSpent remote op vs a retained { title } edit still yields conflicts: [], and one where the edit writes timeSpentOnDay and does not.

Nits

• src/app/op-log/sync/conflict-disjoint-merge.util.ts:230 – isAdditiveTimeOp sits between the JSDoc block for isDisjointMergeEligible and the function, so the doc ("True iff this conflict is safe to resolve by a disjoint-field merge ...") now attaches to the helper. Move the const above that block, e.g. right after nonNoiseKeys.

_{Automated review pass (Claude Code). Anything unclear or wrong – say so and I'll take a look.}

@johannesjo addressed in 909cc3f.

Confirmed the regression: the no-pending caller reads the same predicate as "apply both", so the line 252 exclusion sent a retained { isDone } edit crossing a remote syncTimeSpent op to whole-entity LWW.

Changes:

• isDisjointMergeEligible no longer excludes time ops. For the disjointness test a syncTimeSpent op now counts as touching timeSpent / timeSpentOnDay instead of its { taskId, date, duration } arguments (new sideNonNoiseKeys helper, which also keeps a bare time payload from being read as opaque). An edit of other fields keeps commuting; an edit that overlaps those keys falls to LWW.
• The exclusion moved to _tryCreateDisjointMergeOp, ahead of the predicate call, since that is the only place a merged patch is synthesized. isAdditiveTimeOp is exported and now sits above the predicate's JSDoc.
• Specs: in the _buildNoPendingConcurrentConflict describe, remote syncTimeSpent vs retained { title } yields conflicts: [], and vs retained { timeSpentOnDay } yields one conflict. The util spec's two "refuses" cases became "disjoint from other fields" / "overlaps timeSpentOnDay" / "overlaps timeSpent" plus the no-entityChanges case.

Full src/app/op-log suite green locally (4111 specs).

Note on the red Tests job on 909cc3f: the unit suites all pass (the op-log Vitest/Karma runs are green in the log). The single failure is the e2e case board-keyboard-navigation.spec.ts:360 ("Board touch selection › enters selection from the card menu and toggles cards by tapping", task-multi-select-bar stays empty after tapping "Select several tasks"). That spec was added on master in dade017 (2026-09-13) and is not touched by this branch; the same case fails today on every other PR run I checked (35352608703, 35350971122, 35344942992, 35339976163). Looks like a master-side flake or regression rather than something this PR introduces. I cannot rerun the job from a fork; happy to push a rebase once it is fixed on master.

Filed the underlying analysis as #10146 and #10147 so it doesn't live only in this thread — please read #10146 before doing more work here, because it argues against the approach in the second commit.

Short version: the isDisjointMergeEligible problem is real, but blanket-excluding by actionType is the wrong lever. It gates both call sites — conflict-resolution.service.ts:2678 (pending) and :4643 (no-pending) — and the second one is a lossless path that has shipped since v18.15.0. Excluding there trades a correctness bug for a data-loss regression, and no spec currently covers the crossing, so CI stays green while it lands.

The actual root cause is upstream of the predicate: _captureTaskTimeSyncFromAction() (operation-capture.service.ts:322) declares changes: { taskId, date, duration }, none of which are fields on the Task model. The op really mutates timeSpent / timeSpentOnDay. Because the declared set can never intersect the real set, the disjointness test can never see the overlap — so the machinery happily commutes precisely the case that doesn't commute. Fix the declaration and both bugs go away without touching genuinely disjoint edits:

changes: { timeSpent, timeSpentOnDay }

Two things to carry over if you take that route:

• removeTimeSpent must stay excluded explicitly. It's a second persistent delta that clamps at zero, so two of them don't commute either. It's safe today only by accident — its extraction yields {}, which reads as opaque and falls back to whole-entity LWW. Don't "fix" its extraction at the same time.
• Don't copy the existing time-vs-time fixture at conflict-resolution.service.spec.ts:7414. It uses a bare payload that extracts to {}, i.e. already opaque and already ineligible, so a test modelled on it passes on both the broken and the fixed code and proves nothing. Write the regression test against the real captured payload shape.

Thanks for digging into this one — the diagnosis that something is wrong around isDisjointMergeEligible was right, and it turned up a second shipped bug (#10147) that nobody had noticed.

@johannesjo read #10146 and #10147 and took that route. The second commit is replaced (force-pushed, now b4ed8b7 on top of the unchanged commit 1); the actionType exclusion inside isDisjointMergeEligible and the follow-up key mapping are gone.

Declaration. _captureTaskTimeSyncFromAction now declares { timeSpent, timeSpentOnDay }. One deviation from the issue text I want to flag: the values are the delta (timeSpent: duration, timeSpentOnDay: { [date]: duration }), not the resulting absolute state. extractEntityChanges is documented as a pure function of the action with no state access, it runs on the deferred-write path where the store may have moved on, and the effects layer has no store either, so absolute values are not available there without changing that contract. The keys are what the disjointness test needs; with them isDisjointMergeEligible is back to its master body with no time-op special case.

Consequences on the two paths.

• No-pending (sync: concurrent absolute timeSpentOnDay write vs. syncTimeSpent diverges between clients (no-pending path) #10146): a retained edit of other fields vs a remote syncTimeSpent keeps commuting (conflicts: []). A retained absolute timeSpentOnDay write vs the same op now overlaps and is forwarded to LWW.
• Pending (sync: pending-path merge drops tracked time and writes taskId/date/duration as junk fields onto Task #10147): because the declared values are a delta, a synthesized patch would still write it as an absolute value and reject the original op. _tryCreateDisjointMergeOp therefore refuses additive time ops before calling the predicate and falls back to whole-entity LWW. That also covers your point 1: removeTimeSpent is listed there explicitly via isAdditiveTimeOp; its extraction is untouched and still opaque.

Specs. All use the real captured payload shape, not the bare-payload fixture at line 7414. In the _buildNoPendingConcurrentConflict describe the remote op is built through new OperationCaptureService().extractEntityChanges(...), so the spec fails if the declaration regresses (checked by mutating the extractor back to { taskId, date, duration }). Also pinned: the declaration itself in the capture specs, the pending-path refusal in conflict-resolution.disjoint-merge.spec.ts (fails if the guard is removed), and removeTimeSpent staying ineligible in the util spec.

If you would rather carry absolute values by having the time-tracking effect put the post-state into the syncTimeSpent action payload, that is a wider change touching the wire payload and the remote reducer contract; I left it out of this PR. Full src/app/op-log suite green locally (4113 specs).

🔧 Needs changes – labelled needs work; ping me once the items below are addressed and I'll re-review.

Thanks for the updates. Re-checked the 2 item(s) from the earlier review at b4ed8b7:

• ✅ isDisjointMergeEligible exclusion regressed the no-pending path – fixed: the predicate is back to its master body (conflict-disjoint-merge.util.ts:247-283), the additive-time refusal now lives only in _tryCreateDisjointMergeOp (conflict-resolution.service.ts:2683, ahead of the predicate call), and the two requested specs sit in the _buildNoPendingConcurrentConflict describe (conflict-resolution.service.spec.ts:7463, :7474) and go through the production extractor, so they can actually fail.
• ✅ isAdditiveTimeOp stole the predicate's JSDoc – fixed in conflict-disjoint-merge.util.ts:193-205.

I've flagged a question for the maintainer about the approach below; you may want to wait for that before reworking the details.

Still blocking

• src/app/op-log/capture/operation-capture.service.ts:340 – the new wire shape is misapplied by every already-released client. v18.15.0 through v19.0.1 all carry the disjoint merge (git tag --contains 962c5bbeb1) and none carry the new isAdditiveTimeOp guard, so on such a client a pending non-time edit on task T crossing a new-shape remote syncTimeSpent for T passes isDisjointMergeEligible (['isDone'] vs ['timeSpent','timeSpentOnDay']), _tryCreateDisjointMergeOp synthesizes a 'patch' LWW op { isDone, timeSpent: <delta>, timeSpentOnDay: { [date]: <delta> } }, and lwwUpdateMetaReducer applies it via a shallow updateOne: timeSpent becomes the delta and the whole timeSpentOnDay map is replaced by the single-day delta. The merged op's clock dominates both sides, so up-to-date clients apply it too. Concrete: phone on 19.0.1 marks T done offline, desktop on this branch tracks 1 min on T, phone syncs – T's 5 h across 12 days becomes 1 min on one day, fleet-wide. Today the same crossing only loses the delta and adds junk keys (sync: pending-path merge drops tracked time and writes taskId/date/duration as junk fields onto Task #10147); this PR turns it into loss of the task's time history during the upgrade window. CLAUDE.md sync rule 10 (old clients must be able to tolerate a new op shape) is the rule this trips. Fix direction: keep the wire entityChanges in a shape a 18.15.0+ client's extractOpChanges reads as before or as opaque ({} → whole-entity LWW), and give only the receiving side the honest key set – e.g. map ActionType.TIME_TRACKING_SYNC_TIME_SPENT → { timeSpent, timeSpentOnDay } inside extractOpChanges / the disjointness test rather than in the captured payload. Which shape is acceptable is the maintainer's call (see note above).

Should fix before merge

Two more readers on the new client treat the declared keys as an absolute write, and the delta values now break them where the old junk keys were merely ignored:

• src/app/op-log/sync/conflict-resolution.service.ts:2325 – _createLocalMultiReconciliationOps: a pending roundTimeSpentForDay (TASK_ROUND_TIME_SPENT, static fields { timeSpent, timeSpentOnDay }) losing to a remote syncTimeSpent now hits the full-overlap branch (remoteOverlappingFields.length === fields.length → continue), so the reconciliation op that re-uploaded the rounding is dropped. The local store keeps the rounded values, every other device keeps the unrounded ones – permanent divergence of time data. With the old shape there was no overlap and the patch op was emitted.
• src/app/op-log/sync/sync-conflict-ui.service.ts:131 – journal FLIP: for a pending { title } vs remote syncTimeSpent (now whole-entity LWW), buildConflictJournalEntry stores the loser's mergeChangedFields verbatim, i.e. { timeSpent: <delta>, timeSpentOnDay: { [date]: <delta> } }. FLIP_UNSAFE_FIELDS lists no time field, so canFlip passes and flip() dispatches updateTask with the delta as absolute values – same time-history wipe as above, triggered from the shipped /sync-conflicts page with a button labelled as restoring the discarded change. Excluding syncTimeSpent's keys from the loser diff (or adding timeSpent/timeSpentOnDay to FLIP_UNSAFE_FIELDS) closes this one.

All three are the same root cause: entityChanges now names real Task fields while carrying delta values, so any reader that treats declared keys as an absolute write misapplies them. Keeping the delta out of the field-named wire slot fixes them together.

_{Automated follow-up pass (Claude Code). Anything unclear or wrong – say so and I'll take a look.}

Thanks for working through this, @GabeSilvaDev. I need to correct my earlier guidance: asking you to put timeSpent / timeSpentOnDay into the captured changes led to the unsafe wire shape. That was a problem with my suggested direction, and I'm sorry for the extra rework.

Let's take the receiver-side approach from your 909cc3f76 revision, with the deferred-write change separated out. The current head (b4ed8b7) should not merge with delta values under real task-field names: released clients without the new guard can synthesize an absolute patch from them, replacing the task's time history with a single batch and syncing that result back to updated clients.

Concretely:

• Preserve the existing captured syncTimeSpent payload. Derive timeSpent / timeSpentOnDay from the action type only for the receiver's disjointness test; do not turn that mapping into values returned by the general mergeChangedFields path.
• Keep the additive-op refusal in _tryCreateDisjointMergeOp, covering both syncTimeSpent and removeTimeSpent. Keep it out of the shared predicate so a retained non-time edit and a time delta can still commute on the no-pending path.
• Split out/postpone the op-log: deferred capture writes emit entityChanges: [] — timing-dependent wire shape for the same user intent #8758 deferred-extraction change and retain entityChanges: [] for deferred writes for now. Even restoring the old argument names while keeping deferred extraction would expose previously opaque ops to the existing dropped-delta/junk-field merge on older clients. The receiver fix alone cannot protect those clients.
• Keep regression coverage using production-captured payloads, including the legacy direct and empty deferred forms. Cover non-time edits versus absolute time edits, the pending synthesis guard, and removeTimeSpent. Before landing, include a reproduction checking resulting state and convergence, not only predicate results, and pin that outgoing payloads remain unchanged.
• Update the PR title/body to the narrowed receiver fix and remove Fixes #8758 while that change is deferred. No schema bump is needed.

The earlier objection was to blanket exclusion from the shared predicate; it does not rule out this scoped receiver mapping. This remains a bounded correction: pending conflicts still fall back to LWW, which can discard a change. Preserving both sides there is separate work, not a prerequisite for this fix.

Let's handle the existing journal/Flip issue separately: additive operations should be represented as kind: 'action' diffs and excluded from field-value diffs. Merely adding timeSpent / timeSpentOnDay to FLIP_UNSAFE_FIELDS would not stop today's { taskId, date, duration } payload from being flipped. Restoring the original wire shape also avoids the new delta-as-absolute exposure described in the follow-up review.

The mixed-version finding is supported by the released-code path and predicate/synthesizer probe; it has not yet been reproduced through a live mixed-version sync. I'll correct the suggested fixes in #10146 and #10147 as well.

Reworked as the scoped receiver fix in 8c987d8 (single commit, rebased on 19.1.0). Point by point:

• Captured payload preserved. _captureTaskTimeSyncFromAction is back to master's { taskId, date, duration }; deferred writes keep entityChanges: []. Both pinned by spec (operation-capture.service.spec.ts, operation-log.effects.spec.ts, and the new integration spec).
• Receiver-side mapping only. isDisjointMergeEligible derives timeSpent / timeSpentOnDay for a syncTimeSpent op from its action type inside sideNonNoiseKeys. mergeChangedFields still returns the wire changes, so _tryCreateDisjointMergeOp, _createLocalMultiReconciliationOps and the journal diff never see delta values under task-field names (spec: "does not surface the mapped time fields through mergeChangedFields").
• Refusal stays in _tryCreateDisjointMergeOp, ahead of the predicate, covering syncTimeSpent and removeTimeSpent. Predicate body otherwise unchanged from master.
• Deferred extraction dropped. The op-log: deferred capture writes emit entityChanges: [] — timing-dependent wire shape for the same user intent #8758 commit is gone; PR body says why and no longer claims Fixes #8758.
• Coverage with production-captured payloads in both forms (legacy direct + empty deferred): non-time edit vs absolute time edit on the no-pending path, the pending synthesis guard, removeTimeSpent. Reproduction in task-time-sync-crossing.integration.spec.ts checks resulting task state via convertOpToAction + taskReducer in both arrival orders (identical for edit + delta; order-dependent for absolute write + delta, which is what routes it to LWW). The pending-path spec applies the local-win snapshot through lwwUpdateMetaReducer on the other client and asserts the whole timeSpentOnDay history survives.
• Title/body updated, no schema bump.

Mutation check: the 8 new behavioural specs fail with util/service at master. src/app/op-log/** 4127/4129 green locally (2 pre-existing skips).

Journal/Flip left for the separate kind: 'action' work as agreed.

@johannesjo ready for re-review: 8c987d8 is the scoped receiver-side fix from your last comment (single commit, rebased on 19.1.0, CI green).

✅ Ready to merge – no blocking issues at 8c987d8. Merge confidence 82% – I read the code and the new specs but did not run the suite locally (CI is green at this head); the mixed-version behaviour is reasoned from the released code path, not reproduced against a live 19.0.x client.

Thanks for the updates. Re-checked the 3 item(s) from the earlier review at b4ed8b7:

• ✅ Delta values under real task-field names on the wire (released clients would synthesize a history-wiping patch) – fixed: operation-capture.service.ts and operation-log.effects.ts are back to master, so _captureTaskTimeSyncFromAction emits { taskId, date, duration } and deferred writes keep entityChanges: [] (both pinned by spec). The time-field mapping now lives only in sideNonNoiseKeys (src/app/op-log/sync/conflict-disjoint-merge.util.ts:227), is derived from the action type, and is not surfaced through mergeChangedFields; the additive-op refusal sits ahead of the predicate in _tryCreateDisjointMergeOp (src/app/op-log/sync/conflict-resolution.service.ts:2684). A 18.15.0+ client receiving an op from this head reads exactly what it read before.
• ➖ _createLocalMultiReconciliationOps full-overlap branch dropping the rounding reconciliation op – no longer applies: remoteWinnerChanges is built from mergeChangedFields (conflict-resolution.service.ts:2256), which returns the wire { taskId, date, duration } again, so it never overlaps the static { timeSpent, timeSpentOnDay }.
• ➖ Journal FLIP applying the delta as absolute values – no longer applies: with the wire shape restored the new delta-as-absolute exposure is gone, and the pre-existing junk-key flip is being handled separately as kind: 'action' diffs, as agreed in the thread.

The new coverage is the right shape: production-captured payloads in both wire forms, the no-pending crossing checked for resulting state and convergence in both arrival orders, and the pending-path local-win snapshot applied through the production lwwUpdateMetaReducer.

_{Automated follow-up pass (Claude Code). Anything unclear or wrong – say so and I'll take a look.}