Skip to content

Commit b87f15b

Browse files
claude[bot]claude
andauthored
fix(react): useOffline auto-syncs mutations queued while already online (#6860)
* fix(react): useOffline auto-syncs mutations queued while already online 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yB * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yB --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8579e34 commit b87f15b

4 files changed

Lines changed: 264 additions & 20 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
'@object-ui/react': minor
3+
---
4+
5+
`useOffline` auto-syncs mutations queued while already online (objectui#6818).
6+
7+
The auto-sync effect was keyed `[isOnline, enabled]` with
8+
`react-hooks/exhaustive-deps` suppressed, so its `queue.length === 0` guard was
9+
evaluated against the queue as it stood when `isOnline` or `enabled` last
10+
changed. `queueMutation` has never been conditional on being offline — it
11+
accepts entries whenever the hook is enabled — so anything queued while ALREADY
12+
online found the effect asleep, and nothing re-ran it. Only an explicit `sync()`
13+
drained those mutations; the hook whose job is auto-sync did nothing for them.
14+
15+
The suppression's stated reason ("only trigger on `isOnline` changes, not on
16+
every queue change") was about TIMER RESTARTS, and it is kept: the effect is
17+
keyed on the **boolean** `queue.length > 0`, never on `queue` or `queue.length`,
18+
so queueing a second mutation while the 100ms stabilization timer is already
19+
armed still does not re-run the effect or restart the timer. What the
20+
suppression never justified — the early return against a stale snapshot — is
21+
what changed.
22+
23+
`sync` also read `batchSize` through a ref (newest) while reading `queue` from
24+
its own closure (a snapshot), so the two halves of one call disagreed about how
25+
current they were, and the auto-sync effect retains such a closure by design.
26+
The queue now reaches `sync` through the same commit-phase mirror the sync
27+
config uses, so both halves are the newest committed values. That also takes
28+
`queue` out of `sync`'s dependency list: `sync` is keyed `[enabled]` and is
29+
stable across queued mutations, which is what lets the effect name every value
30+
it reads and drop the `eslint-disable` entirely rather than reword it.
31+
32+
**Behaviour change, graded `minor` deliberately.** `useOffline` is published and
33+
its out-of-repo population is unmeasured; the single in-repo caller
34+
(`AppHeader`) destructures `isOnline` only and is unaffected. A consumer that
35+
called `queueMutation` while online and relied on nothing being sent until it
36+
called `sync()` itself will now see that mutation flushed ~100ms later.
37+
`sync`'s identity is also more stable than before — it no longer changes on
38+
every queued mutation — which is safe for effects keyed on it but is a visible
39+
difference.
40+
41+
Not changed here: a `batchSize` smaller than the queue still drains one batch
42+
and leaves the remainder for the next transition, because whether one auto-sync
43+
should chain batches until the queue is empty is a separate question about what
44+
`batchSize` means, not about this guard. Filed as objectui#6857.
Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/**
2+
* ObjectUI — useOffline auto-syncs mutations queued while ONLINE (objectui#6818)
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* The auto-sync effect was keyed `[isOnline, enabled]` with
6+
* `react-hooks/exhaustive-deps` suppressed, so its `queue.length === 0` guard
7+
* was evaluated against the queue as it stood when `isOnline` or `enabled` last
8+
* changed. `queueMutation` has never been conditional on being offline, so a
9+
* mutation queued while ALREADY online found the effect asleep: nothing
10+
* re-ran it, and only an explicit `sync()` could drain the queue.
11+
*
12+
* Nothing in this repo reaches that queue — `AppHeader.tsx` is the one in-repo
13+
* caller and it destructures `isOnline` only — so a green suite proved nothing
14+
* about this path before these pins existed. Each one below drives a real
15+
* mutation through the queue.
16+
*
17+
* Timers are faked because two of the pins are about WHEN the 100ms
18+
* stabilization timer fires, not merely whether it does.
19+
*/
20+
21+
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
22+
import { renderHook, act } from '@testing-library/react';
23+
import { useOffline, type OfflineResult } from '../useOffline';
24+
25+
function queueOne(result: { current: OfflineResult }, resource: string) {
26+
act(() => {
27+
result.current.queueMutation({ operation: 'create', resource, data: { resource } });
28+
});
29+
}
30+
31+
/** Advance fake time and let React flush whatever the timers scheduled. */
32+
async function advance(ms: number) {
33+
await act(async () => {
34+
await vi.advanceTimersByTimeAsync(ms);
35+
});
36+
}
37+
38+
function setOnline(value: boolean) {
39+
Object.defineProperty(window.navigator, 'onLine', { configurable: true, value });
40+
act(() => {
41+
window.dispatchEvent(new Event(value ? 'online' : 'offline'));
42+
});
43+
}
44+
45+
beforeEach(() => {
46+
localStorage.clear();
47+
setOnline(true);
48+
vi.useFakeTimers();
49+
});
50+
51+
afterEach(() => {
52+
vi.useRealTimers();
53+
setOnline(true);
54+
});
55+
56+
describe('useOffline — auto-sync reaches mutations queued while online (#6818)', () => {
57+
// ---- pin 1: the card's finding, stated as behaviour ----------------------
58+
// This is the assertion the whole card is about, and it fails on the base:
59+
// the effect had already returned early against an empty queue and nothing
60+
// re-ran it when `queueMutation` made the queue non-empty.
61+
it('drains a mutation queued while already online', async () => {
62+
const { result } = renderHook(() => useOffline());
63+
expect(result.current.isOnline).toBe(true);
64+
65+
queueOne(result, 'account');
66+
expect(result.current.pendingCount).toBe(1);
67+
68+
await advance(150);
69+
70+
expect(result.current.pendingCount).toBe(0);
71+
expect(result.current.syncState).toBe('idle');
72+
});
73+
74+
// ---- pin 2: DISCRIMINATING — the timer must NOT restart per mutation -----
75+
// The suppressed dep list existed to stop the 100ms timer restarting on every
76+
// queued mutation, and that reason is kept: the dep is the BOOLEAN
77+
// `queue.length > 0`. This pin is what tells the two shapes apart. The timer
78+
// is armed at t=0 by the first mutation; a second mutation lands at t=60. If
79+
// the effect were keyed on the queue (or on `queue.length`), it would re-run
80+
// there and re-arm for t=160, so at t=105 nothing would have drained yet.
81+
// Keyed on the boolean, the ORIGINAL timer fires at t=100 and — because
82+
// `sync` now reads the queue through the same commit-phase ref it already
83+
// read `batchSize` through — it flushes BOTH entries, not just the first.
84+
it('keeps the original timer when a second mutation is queued before it fires', async () => {
85+
const { result } = renderHook(() => useOffline());
86+
87+
queueOne(result, 'account');
88+
await advance(60);
89+
expect(result.current.pendingCount).toBe(1);
90+
91+
queueOne(result, 'contact');
92+
expect(result.current.pendingCount).toBe(2);
93+
94+
await advance(45); // t = 105: past the ORIGINAL 100ms deadline, short of a restarted one
95+
96+
expect(result.current.pendingCount).toBe(0);
97+
});
98+
99+
// ---- pin 3: the original feature is not traded away ----------------------
100+
// Green on the base too, deliberately: it is the regression guard for the
101+
// behaviour the narrow dep list did deliver ("sync when you come back
102+
// online"), so a future edit cannot close #6818 by breaking the reconnect.
103+
it('still auto-syncs on the offline to online transition', async () => {
104+
const { result } = renderHook(() => useOffline());
105+
106+
setOnline(false);
107+
expect(result.current.isOnline).toBe(false);
108+
109+
queueOne(result, 'account');
110+
await advance(150);
111+
// Offline: the guard is right to hold the queue.
112+
expect(result.current.pendingCount).toBe(1);
113+
114+
setOnline(true);
115+
await advance(150);
116+
117+
expect(result.current.pendingCount).toBe(0);
118+
});
119+
120+
// ---- pin 4: point 2 of the card — one call, one notion of "current" ------
121+
// `sync` read `batchSize` through a ref (newest) and `queue` from its own
122+
// closure (a snapshot), so the two halves of a RETAINED call disagreed about
123+
// how current they were — and the auto-sync effect retains one by design.
124+
// Mirrors #6797's pin 1 ("a retained closure reads the newest batchSize")
125+
// with its missing half: a retained closure batches the newest QUEUE.
126+
it('lets a retained sync closure batch the newest queue, not its own snapshot', async () => {
127+
const { result } = renderHook(() => useOffline());
128+
129+
queueOne(result, 'account');
130+
const retained = result.current.sync;
131+
132+
queueOne(result, 'contact');
133+
expect(result.current.pendingCount).toBe(2);
134+
135+
await act(async () => {
136+
const settled = retained();
137+
await vi.advanceTimersByTimeAsync(1); // the simulated round-trip, not the 100ms timer
138+
await settled;
139+
});
140+
141+
// On the base this is 1: `retained` closed over the one-entry queue and
142+
// flushed only that, leaving the mutation queued after it behind.
143+
expect(result.current.pendingCount).toBe(0);
144+
});
145+
});

‎packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,22 @@
1010
*
1111
* Who reads that ref, measured on this base: exactly ONE reader —
1212
* `sync`, at `const batchSize = syncConfigRef.current?.batchSize ?? queue.length`.
13-
* That makes this hook the odd one of the three: `sync` is NOT a stable
14-
* callback (deps `[enabled, queue]`), so the ref is not protecting an identity
13+
* That made this hook the odd one of the three: `sync` was NOT a stable
14+
* callback (deps `[enabled, queue]`), so the ref was not protecting an identity
1515
* the way the other two hooks' refs are. What it protects is RETAINED closures:
1616
* a config-only change keeps the same `sync` alive, and the auto-sync effect
1717
* deliberately captures one and fires it 100ms later. Pin 1 is that exact
1818
* property — the ref's only job — and it is what rules out the alternative fix
1919
* of dropping the ref and adding `syncConfig?.batchSize` to `sync`'s deps.
2020
*
21+
* objectui#6818 then gave the QUEUE the same commit-phase mirror, which took
22+
* `queue` out of `sync`'s deps: `sync` is keyed `[enabled]` today and is stable
23+
* across queued mutations. Every pin below still measures what it measured —
24+
* a retained closure reading the newest `batchSize` — and pin 1 still fails
25+
* under the rejected `syncConfig?.batchSize`-in-deps alternative, because that
26+
* alternative is exactly what would make `sync` unstable again. The dep lists
27+
* quoted below are updated where they would otherwise mislead.
28+
*
2129
* The write now happens in `useInsertionEffect`. Pin 3 is the discriminating
2230
* one: the `batchSize` read is SYNCHRONOUS, before `sync`'s first `await`, so a
2331
* child layout effect of the same commit fails under BOTH `useEffect` and
@@ -50,12 +58,14 @@ function Harness({ config, trigger }: { config: OfflineConfig; trigger: number }
5058
}
5159

5260
function CommitPhaseCaller({ sync, trigger }: { sync: () => Promise<void>; trigger: number }) {
53-
// Fire EXACTLY once. `sync` drains the queue, which re-renders and hands this
54-
// effect a new `sync` (it is keyed on `[enabled, queue]`), so an unguarded
55-
// effect re-fires and drains the queue batch-by-batch until it is empty — the
56-
// end state is then 0 whatever `batchSize` the first call read, and the pin
57-
// measures nothing about timing. Measured: with no guard this test passed
58-
// even with the ref write moved to `useEffect`.
61+
// Fire EXACTLY once. On the base this pin was written against, `sync` drains
62+
// the queue, which re-renders and hands this effect a new `sync` (it was
63+
// keyed on `[enabled, queue]`), so an unguarded effect re-fires and drains
64+
// the queue batch-by-batch until it is empty — the end state is then 0
65+
// whatever `batchSize` the first call read, and the pin measures nothing
66+
// about timing. Measured: with no guard this test passed even with the ref
67+
// write moved to `useEffect`. The guard stays now that `sync` is stable
68+
// (objectui#6818): it is what keeps this pin honest if the deps move back.
5969
const fired = useRef(false);
6070
useLayoutEffect(() => {
6171
if (trigger > 0 && !fired.current) {
@@ -103,8 +113,10 @@ describe('useOffline — sync config ref is refreshed in the commit, not in rend
103113

104114
const syncBefore = result.current.sync;
105115
rerender({ batchSize: 5 });
106-
// `sync` is keyed on [enabled, queue]; neither moved, so the SAME closure
107-
// survived the config change. That is the precondition of this pin.
116+
// `sync` is keyed on [enabled]; it did not move, so the SAME closure
117+
// survived the config change. That is the precondition of this pin — and
118+
// the assertion still fails under the rejected alternative, which would put
119+
// `batchSize` in those deps.
108120
expect(result.current.sync).toBe(syncBefore);
109121

110122
await act(async () => {

‎packages/react/src/hooks/useOffline.ts‎

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,19 @@ export function useOffline(config: OfflineConfig = {}): OfflineResult {
279279
useInsertionEffect(() => {
280280
syncConfigRef.current = syncConfig;
281281
});
282+
// The queue reaches `sync` through the SAME commit-phase mirror the sync
283+
// config uses (objectui#6818). It used to come from `sync`'s own closure
284+
// while `batchSize` came from the ref above, so the two halves of one call
285+
// disagreed about how current they were: a retained `sync` — and the
286+
// auto-sync effect below deliberately retains one and fires it 100ms later —
287+
// batched a queue snapshot from an older render against the newest
288+
// `batchSize`. Mirroring the queue makes both halves the newest COMMITTED
289+
// value, and it is what takes `queue` out of `sync`'s dependency list, so
290+
// `sync` stops changing identity on every queued mutation.
291+
const queueRef = useRef(queue);
292+
useInsertionEffect(() => {
293+
queueRef.current = queue;
294+
});
282295

283296
// Persist queue to localStorage whenever it changes
284297
useEffect(() => {
@@ -320,13 +333,15 @@ export function useOffline(config: OfflineConfig = {}): OfflineResult {
320333
}, []);
321334

322335
const sync = useCallback(async () => {
323-
if (!enabled || queue.length === 0) return;
336+
// Newest committed queue, not this closure's snapshot — see `queueRef`.
337+
const pending = queueRef.current;
338+
if (!enabled || pending.length === 0) return;
324339
setSyncState('syncing');
325340
try {
326341
// In a real implementation, this would batch-send mutations to the server.
327342
// For now, we simulate a successful sync by clearing the queue.
328-
const batchSize = syncConfigRef.current?.batchSize ?? queue.length;
329-
const batch = queue.slice(0, batchSize);
343+
const batchSize = syncConfigRef.current?.batchSize ?? pending.length;
344+
const batch = pending.slice(0, batchSize);
330345

331346
// Simulate network round-trip
332347
await new Promise<void>((resolve) => setTimeout(resolve, 0));
@@ -337,18 +352,46 @@ export function useOffline(config: OfflineConfig = {}): OfflineResult {
337352
} catch {
338353
setSyncState('error');
339354
}
340-
}, [enabled, queue]);
341-
342-
// Auto-sync when coming back online (short stabilization delay)
355+
}, [enabled]);
356+
357+
// Auto-sync while online and holding queued mutations (short stabilization
358+
// delay).
359+
//
360+
// The dependency list used to be `[isOnline, enabled]` with
361+
// `react-hooks/exhaustive-deps` suppressed, on the stated grounds that
362+
// re-running "on every queue change" would restart the 100ms timer once per
363+
// queued mutation. That reason is real and is preserved below — the
364+
// dependency is the BOOLEAN `hasPendingMutations`, never `queue` and never
365+
// `queue.length`, so queueing a second mutation while a timer is already
366+
// armed does not re-run this effect and does not restart the timer.
367+
//
368+
// What the suppression also did, and never justified, was evaluate the
369+
// emptiness guard against a queue snapshot from whenever `isOnline` or
370+
// `enabled` last changed. `queueMutation` accepts entries whenever the hook
371+
// is enabled — it has never been conditional on being offline — so anything
372+
// queued while ALREADY online found this effect asleep and had no auto-sync
373+
// path at all; only an explicit `sync()` drained it (objectui#6818). Keying
374+
// on the boolean re-evaluates the guard exactly when it can change answer.
375+
//
376+
// `sync` is now stable across queue changes (it reads the queue through
377+
// `queueRef`), so naming it here costs no extra timer restart and the array
378+
// is genuinely exhaustive — the suppression is gone rather than reworded.
379+
//
380+
// Known remaining edge, deliberately not widened here: a `batchSize` smaller
381+
// than the queue drains one batch and leaves `hasPendingMutations` true, so
382+
// the remainder waits for the next transition rather than chaining a second
383+
// batch. That is the pre-existing behaviour, not a regression. Whether one
384+
// auto-sync should drain the whole queue batch-by-batch is a semantics
385+
// question about `batchSize` rather than a bug in this guard, and it is
386+
// filed separately as objectui#6857.
387+
const hasPendingMutations = queue.length > 0;
343388
useEffect(() => {
344-
if (!enabled || !isOnline || queue.length === 0) return;
389+
if (!enabled || !isOnline || !hasPendingMutations) return;
345390
const timer = setTimeout(() => {
346391
void sync();
347392
}, 100);
348393
return () => clearTimeout(timer);
349-
// Only trigger on isOnline changes, not on every queue change
350-
// eslint-disable-next-line react-hooks/exhaustive-deps
351-
}, [isOnline, enabled]);
394+
}, [isOnline, enabled, hasPendingMutations, sync]);
352395

353396
return useMemo<OfflineResult>(
354397
() => ({

0 commit comments

Comments
 (0)