fix(react): useOffline auto-syncs mutations queued while already online

.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. Filed as objectui#6857.

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);
+ });
+});

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<void>; 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 () => {

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<void>((resolve) => setTimeout(resolve, 0));
@@ -337,18 +352,46 @@ 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. 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 || 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<OfflineResult>(
  () => ({