From e085d257d68025b56716d149afaca0cd2e03ec97 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 06:25:10 +0000 Subject: [PATCH 1/2] fix(react): useOffline auto-syncs mutations queued while already online MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auto-sync effect was keyed `[isOnline, enabled]` with exhaustive-deps suppressed, so its `queue.length === 0` guard was evaluated against the queue as it stood when `isOnline`/`enabled` last changed. `queueMutation` is not conditional on being offline, so anything queued while ALREADY online found the effect asleep and had no auto-sync path at all. Key the effect on the boolean `queue.length > 0` — the suppression's real reason (no timer restart per queued mutation) is preserved, because a boolean does not change when a second mutation lands. Mirror the queue through the same commit-phase ref the sync config already uses so `sync` stops batching a stale snapshot against a fresh `batchSize`; that makes `sync` stable and the dep list genuinely exhaustive, so the `eslint-disable` is deleted rather than reworded. Part of #6818 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yB --- .../6818-useoffline-auto-sync-stale-queue.md | 44 ++++++ .../useOffline.autoSyncWhileOnline.test.tsx | 145 ++++++++++++++++++ .../useOffline.syncConfigTiming.test.tsx | 32 ++-- packages/react/src/hooks/useOffline.ts | 61 ++++++-- 4 files changed, 262 insertions(+), 20 deletions(-) create mode 100644 .changeset/6818-useoffline-auto-sync-stale-queue.md create mode 100644 packages/react/src/hooks/__tests__/useOffline.autoSyncWhileOnline.test.tsx diff --git a/.changeset/6818-useoffline-auto-sync-stale-queue.md b/.changeset/6818-useoffline-auto-sync-stale-queue.md new file mode 100644 index 000000000..b876dca84 --- /dev/null +++ b/.changeset/6818-useoffline-auto-sync-stale-queue.md @@ -0,0 +1,44 @@ +--- +'@object-ui/react': minor +--- + +`useOffline` auto-syncs mutations queued while already online (objectui#6818). + +The auto-sync effect was keyed `[isOnline, enabled]` with +`react-hooks/exhaustive-deps` suppressed, so its `queue.length === 0` guard was +evaluated against the queue as it stood when `isOnline` or `enabled` last +changed. `queueMutation` has never been conditional on being offline — it +accepts entries whenever the hook is enabled — so anything queued while ALREADY +online found the effect asleep, and nothing re-ran it. Only an explicit `sync()` +drained those mutations; the hook whose job is auto-sync did nothing for them. + +The suppression's stated reason ("only trigger on `isOnline` changes, not on +every queue change") was about TIMER RESTARTS, and it is kept: the effect is +keyed on the **boolean** `queue.length > 0`, never on `queue` or `queue.length`, +so queueing a second mutation while the 100ms stabilization timer is already +armed still does not re-run the effect or restart the timer. What the +suppression never justified — the early return against a stale snapshot — is +what changed. + +`sync` also read `batchSize` through a ref (newest) while reading `queue` from +its own closure (a snapshot), so the two halves of one call disagreed about how +current they were, and the auto-sync effect retains such a closure by design. +The queue now reaches `sync` through the same commit-phase mirror the sync +config uses, so both halves are the newest committed values. That also takes +`queue` out of `sync`'s dependency list: `sync` is keyed `[enabled]` and is +stable across queued mutations, which is what lets the effect name every value +it reads and drop the `eslint-disable` entirely rather than reword it. + +**Behaviour change, graded `minor` deliberately.** `useOffline` is published and +its out-of-repo population is unmeasured; the single in-repo caller +(`AppHeader`) destructures `isOnline` only and is unaffected. A consumer that +called `queueMutation` while online and relied on nothing being sent until it +called `sync()` itself will now see that mutation flushed ~100ms later. +`sync`'s identity is also more stable than before — it no longer changes on +every queued mutation — which is safe for effects keyed on it but is a visible +difference. + +Not changed here: a `batchSize` smaller than the queue still drains one batch +and leaves the remainder for the next transition, because whether one auto-sync +should chain batches until the queue is empty is a separate question about what +`batchSize` means, not about this guard. diff --git a/packages/react/src/hooks/__tests__/useOffline.autoSyncWhileOnline.test.tsx b/packages/react/src/hooks/__tests__/useOffline.autoSyncWhileOnline.test.tsx new file mode 100644 index 000000000..2125d5e55 --- /dev/null +++ b/packages/react/src/hooks/__tests__/useOffline.autoSyncWhileOnline.test.tsx @@ -0,0 +1,145 @@ +/** + * ObjectUI — useOffline auto-syncs mutations queued while ONLINE (objectui#6818) + * Copyright (c) 2024-present ObjectStack Inc. + * + * The auto-sync effect was keyed `[isOnline, enabled]` with + * `react-hooks/exhaustive-deps` suppressed, so its `queue.length === 0` guard + * was evaluated against the queue as it stood when `isOnline` or `enabled` last + * changed. `queueMutation` has never been conditional on being offline, so a + * mutation queued while ALREADY online found the effect asleep: nothing + * re-ran it, and only an explicit `sync()` could drain the queue. + * + * Nothing in this repo reaches that queue — `AppHeader.tsx` is the one in-repo + * caller and it destructures `isOnline` only — so a green suite proved nothing + * about this path before these pins existed. Each one below drives a real + * mutation through the queue. + * + * Timers are faked because two of the pins are about WHEN the 100ms + * stabilization timer fires, not merely whether it does. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import { useOffline, type OfflineResult } from '../useOffline'; + +function queueOne(result: { current: OfflineResult }, resource: string) { + act(() => { + result.current.queueMutation({ operation: 'create', resource, data: { resource } }); + }); +} + +/** Advance fake time and let React flush whatever the timers scheduled. */ +async function advance(ms: number) { + await act(async () => { + await vi.advanceTimersByTimeAsync(ms); + }); +} + +function setOnline(value: boolean) { + Object.defineProperty(window.navigator, 'onLine', { configurable: true, value }); + act(() => { + window.dispatchEvent(new Event(value ? 'online' : 'offline')); + }); +} + +beforeEach(() => { + localStorage.clear(); + setOnline(true); + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); + setOnline(true); +}); + +describe('useOffline — auto-sync reaches mutations queued while online (#6818)', () => { + // ---- pin 1: the card's finding, stated as behaviour ---------------------- + // This is the assertion the whole card is about, and it fails on the base: + // the effect had already returned early against an empty queue and nothing + // re-ran it when `queueMutation` made the queue non-empty. + it('drains a mutation queued while already online', async () => { + const { result } = renderHook(() => useOffline()); + expect(result.current.isOnline).toBe(true); + + queueOne(result, 'account'); + expect(result.current.pendingCount).toBe(1); + + await advance(150); + + expect(result.current.pendingCount).toBe(0); + expect(result.current.syncState).toBe('idle'); + }); + + // ---- pin 2: DISCRIMINATING — the timer must NOT restart per mutation ----- + // The suppressed dep list existed to stop the 100ms timer restarting on every + // queued mutation, and that reason is kept: the dep is the BOOLEAN + // `queue.length > 0`. This pin is what tells the two shapes apart. The timer + // is armed at t=0 by the first mutation; a second mutation lands at t=60. If + // the effect were keyed on the queue (or on `queue.length`), it would re-run + // there and re-arm for t=160, so at t=105 nothing would have drained yet. + // Keyed on the boolean, the ORIGINAL timer fires at t=100 and — because + // `sync` now reads the queue through the same commit-phase ref it already + // read `batchSize` through — it flushes BOTH entries, not just the first. + it('keeps the original timer when a second mutation is queued before it fires', async () => { + const { result } = renderHook(() => useOffline()); + + queueOne(result, 'account'); + await advance(60); + expect(result.current.pendingCount).toBe(1); + + queueOne(result, 'contact'); + expect(result.current.pendingCount).toBe(2); + + await advance(45); // t = 105: past the ORIGINAL 100ms deadline, short of a restarted one + + expect(result.current.pendingCount).toBe(0); + }); + + // ---- pin 3: the original feature is not traded away ---------------------- + // Green on the base too, deliberately: it is the regression guard for the + // behaviour the narrow dep list did deliver ("sync when you come back + // online"), so a future edit cannot close #6818 by breaking the reconnect. + it('still auto-syncs on the offline to online transition', async () => { + const { result } = renderHook(() => useOffline()); + + setOnline(false); + expect(result.current.isOnline).toBe(false); + + queueOne(result, 'account'); + await advance(150); + // Offline: the guard is right to hold the queue. + expect(result.current.pendingCount).toBe(1); + + setOnline(true); + await advance(150); + + expect(result.current.pendingCount).toBe(0); + }); + + // ---- pin 4: point 2 of the card — one call, one notion of "current" ------ + // `sync` read `batchSize` through a ref (newest) and `queue` from its own + // closure (a snapshot), so the two halves of a RETAINED call disagreed about + // how current they were — and the auto-sync effect retains one by design. + // Mirrors #6797's pin 1 ("a retained closure reads the newest batchSize") + // with its missing half: a retained closure batches the newest QUEUE. + it('lets a retained sync closure batch the newest queue, not its own snapshot', async () => { + const { result } = renderHook(() => useOffline()); + + queueOne(result, 'account'); + const retained = result.current.sync; + + queueOne(result, 'contact'); + expect(result.current.pendingCount).toBe(2); + + await act(async () => { + const settled = retained(); + await vi.advanceTimersByTimeAsync(1); // the simulated round-trip, not the 100ms timer + await settled; + }); + + // On the base this is 1: `retained` closed over the one-entry queue and + // flushed only that, leaving the mutation queued after it behind. + expect(result.current.pendingCount).toBe(0); + }); +}); diff --git a/packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx b/packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx index 97764f802..5d42f47d1 100644 --- a/packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx +++ b/packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx @@ -10,14 +10,22 @@ * * Who reads that ref, measured on this base: exactly ONE reader — * `sync`, at `const batchSize = syncConfigRef.current?.batchSize ?? queue.length`. - * That makes this hook the odd one of the three: `sync` is NOT a stable - * callback (deps `[enabled, queue]`), so the ref is not protecting an identity + * That made this hook the odd one of the three: `sync` was NOT a stable + * callback (deps `[enabled, queue]`), so the ref was not protecting an identity * the way the other two hooks' refs are. What it protects is RETAINED closures: * a config-only change keeps the same `sync` alive, and the auto-sync effect * deliberately captures one and fires it 100ms later. Pin 1 is that exact * property — the ref's only job — and it is what rules out the alternative fix * of dropping the ref and adding `syncConfig?.batchSize` to `sync`'s deps. * + * objectui#6818 then gave the QUEUE the same commit-phase mirror, which took + * `queue` out of `sync`'s deps: `sync` is keyed `[enabled]` today and is stable + * across queued mutations. Every pin below still measures what it measured — + * a retained closure reading the newest `batchSize` — and pin 1 still fails + * under the rejected `syncConfig?.batchSize`-in-deps alternative, because that + * alternative is exactly what would make `sync` unstable again. The dep lists + * quoted below are updated where they would otherwise mislead. + * * The write now happens in `useInsertionEffect`. Pin 3 is the discriminating * one: the `batchSize` read is SYNCHRONOUS, before `sync`'s first `await`, so a * child layout effect of the same commit fails under BOTH `useEffect` and @@ -50,12 +58,14 @@ function Harness({ config, trigger }: { config: OfflineConfig; trigger: number } } function CommitPhaseCaller({ sync, trigger }: { sync: () => Promise; trigger: number }) { - // Fire EXACTLY once. `sync` drains the queue, which re-renders and hands this - // effect a new `sync` (it is keyed on `[enabled, queue]`), so an unguarded - // effect re-fires and drains the queue batch-by-batch until it is empty — the - // end state is then 0 whatever `batchSize` the first call read, and the pin - // measures nothing about timing. Measured: with no guard this test passed - // even with the ref write moved to `useEffect`. + // Fire EXACTLY once. On the base this pin was written against, `sync` drains + // the queue, which re-renders and hands this effect a new `sync` (it was + // keyed on `[enabled, queue]`), so an unguarded effect re-fires and drains + // the queue batch-by-batch until it is empty — the end state is then 0 + // whatever `batchSize` the first call read, and the pin measures nothing + // about timing. Measured: with no guard this test passed even with the ref + // write moved to `useEffect`. The guard stays now that `sync` is stable + // (objectui#6818): it is what keeps this pin honest if the deps move back. const fired = useRef(false); useLayoutEffect(() => { if (trigger > 0 && !fired.current) { @@ -103,8 +113,10 @@ describe('useOffline — sync config ref is refreshed in the commit, not in rend const syncBefore = result.current.sync; rerender({ batchSize: 5 }); - // `sync` is keyed on [enabled, queue]; neither moved, so the SAME closure - // survived the config change. That is the precondition of this pin. + // `sync` is keyed on [enabled]; it did not move, so the SAME closure + // survived the config change. That is the precondition of this pin — and + // the assertion still fails under the rejected alternative, which would put + // `batchSize` in those deps. expect(result.current.sync).toBe(syncBefore); await act(async () => { diff --git a/packages/react/src/hooks/useOffline.ts b/packages/react/src/hooks/useOffline.ts index 49fe3a386..4a6fc503d 100644 --- a/packages/react/src/hooks/useOffline.ts +++ b/packages/react/src/hooks/useOffline.ts @@ -279,6 +279,19 @@ export function useOffline(config: OfflineConfig = {}): OfflineResult { useInsertionEffect(() => { syncConfigRef.current = syncConfig; }); + // The queue reaches `sync` through the SAME commit-phase mirror the sync + // config uses (objectui#6818). It used to come from `sync`'s own closure + // while `batchSize` came from the ref above, so the two halves of one call + // disagreed about how current they were: a retained `sync` — and the + // auto-sync effect below deliberately retains one and fires it 100ms later — + // batched a queue snapshot from an older render against the newest + // `batchSize`. Mirroring the queue makes both halves the newest COMMITTED + // value, and it is what takes `queue` out of `sync`'s dependency list, so + // `sync` stops changing identity on every queued mutation. + const queueRef = useRef(queue); + useInsertionEffect(() => { + queueRef.current = queue; + }); // Persist queue to localStorage whenever it changes useEffect(() => { @@ -320,13 +333,15 @@ export function useOffline(config: OfflineConfig = {}): OfflineResult { }, []); const sync = useCallback(async () => { - if (!enabled || queue.length === 0) return; + // Newest committed queue, not this closure's snapshot — see `queueRef`. + const pending = queueRef.current; + if (!enabled || pending.length === 0) return; setSyncState('syncing'); try { // In a real implementation, this would batch-send mutations to the server. // For now, we simulate a successful sync by clearing the queue. - const batchSize = syncConfigRef.current?.batchSize ?? queue.length; - const batch = queue.slice(0, batchSize); + const batchSize = syncConfigRef.current?.batchSize ?? pending.length; + const batch = pending.slice(0, batchSize); // Simulate network round-trip await new Promise((resolve) => setTimeout(resolve, 0)); @@ -337,18 +352,44 @@ export function useOffline(config: OfflineConfig = {}): OfflineResult { } catch { setSyncState('error'); } - }, [enabled, queue]); - - // Auto-sync when coming back online (short stabilization delay) + }, [enabled]); + + // Auto-sync while online and holding queued mutations (short stabilization + // delay). + // + // The dependency list used to be `[isOnline, enabled]` with + // `react-hooks/exhaustive-deps` suppressed, on the stated grounds that + // re-running "on every queue change" would restart the 100ms timer once per + // queued mutation. That reason is real and is preserved below — the + // dependency is the BOOLEAN `hasPendingMutations`, never `queue` and never + // `queue.length`, so queueing a second mutation while a timer is already + // armed does not re-run this effect and does not restart the timer. + // + // What the suppression also did, and never justified, was evaluate the + // emptiness guard against a queue snapshot from whenever `isOnline` or + // `enabled` last changed. `queueMutation` accepts entries whenever the hook + // is enabled — it has never been conditional on being offline — so anything + // queued while ALREADY online found this effect asleep and had no auto-sync + // path at all; only an explicit `sync()` drained it (objectui#6818). Keying + // on the boolean re-evaluates the guard exactly when it can change answer. + // + // `sync` is now stable across queue changes (it reads the queue through + // `queueRef`), so naming it here costs no extra timer restart and the array + // is genuinely exhaustive — the suppression is gone rather than reworded. + // + // Known remaining edge, deliberately not widened here: a `batchSize` smaller + // than the queue drains one batch and leaves `hasPendingMutations` true, so + // the remainder waits for the next transition rather than chaining a second + // batch. Whether one auto-sync should drain the whole queue batch-by-batch is + // a separate semantics question for `batchSize`, not this guard's bug. + const hasPendingMutations = queue.length > 0; useEffect(() => { - if (!enabled || !isOnline || queue.length === 0) return; + if (!enabled || !isOnline || !hasPendingMutations) return; const timer = setTimeout(() => { void sync(); }, 100); return () => clearTimeout(timer); - // Only trigger on isOnline changes, not on every queue change - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isOnline, enabled]); + }, [isOnline, enabled, hasPendingMutations, sync]); return useMemo( () => ({ From 2e8e724b08a7cc9dfe145e5efb3b182a3c2698d1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 06:34:00 +0000 Subject: [PATCH 2/2] docs(react): name the follow-up card for the un-chained batch remainder The comment and changeset already described the edge the boolean dep leaves open; point them at the card that owns the decision so the next reader does not re-derive it. Part of #6818 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yB --- .changeset/6818-useoffline-auto-sync-stale-queue.md | 2 +- packages/react/src/hooks/useOffline.ts | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.changeset/6818-useoffline-auto-sync-stale-queue.md b/.changeset/6818-useoffline-auto-sync-stale-queue.md index b876dca84..a1783de15 100644 --- a/.changeset/6818-useoffline-auto-sync-stale-queue.md +++ b/.changeset/6818-useoffline-auto-sync-stale-queue.md @@ -41,4 +41,4 @@ difference. Not changed here: a `batchSize` smaller than the queue still drains one batch and leaves the remainder for the next transition, because whether one auto-sync should chain batches until the queue is empty is a separate question about what -`batchSize` means, not about this guard. +`batchSize` means, not about this guard. Filed as objectui#6857. diff --git a/packages/react/src/hooks/useOffline.ts b/packages/react/src/hooks/useOffline.ts index 4a6fc503d..67918e71d 100644 --- a/packages/react/src/hooks/useOffline.ts +++ b/packages/react/src/hooks/useOffline.ts @@ -380,8 +380,10 @@ export function useOffline(config: OfflineConfig = {}): OfflineResult { // Known remaining edge, deliberately not widened here: a `batchSize` smaller // than the queue drains one batch and leaves `hasPendingMutations` true, so // the remainder waits for the next transition rather than chaining a second - // batch. Whether one auto-sync should drain the whole queue batch-by-batch is - // a separate semantics question for `batchSize`, not this guard's bug. + // batch. That is the pre-existing behaviour, not a regression. Whether one + // auto-sync should drain the whole queue batch-by-batch is a semantics + // question about `batchSize` rather than a bug in this guard, and it is + // filed separately as objectui#6857. const hasPendingMutations = queue.length > 0; useEffect(() => { if (!enabled || !isOnline || !hasPendingMutations) return;