Uh oh!
There was an error while loading. Please reload this page.
fix(react): useOffline auto-syncs mutations queued while already online - #6860
Merged
Conversation
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
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
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sam
marked this pull request as ready for review
August 30, 2026 06:52
os-sam
enabled auto-merge
August 30, 2026 06:52
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6818
The intent question, settled first
Triage required establishing whether the early return was intended before
changing behaviour, and ruled that the
eslint-disablecomment ("Only triggeron
isOnlinechanges, not on every queue change") is evidence about timerrestarts and about nothing else. Measured on this base, the early return was
not a decision:
queueMutationhas neverbeen conditional on being offline — it appends whenever
enabledis true. Aqueue that accepts entries while online but whose only drain trigger is an
isOnline/enabledtransition is an incoherent pair, not a design. If thequeue were meant to fill only while offline,
queue.length > 0 && isOnlinewould be reachable only through a reconnect and the stale guard would be
harmless; it is harmful precisely because the producer says otherwise.
bdf6f9d51introduced it with the narrow deps and the suppression already inplace, and the day-one tests it shipped with queue mutations while online
and then drain them with an explicit
sync()— so the online-queue path wasexercised from the start, just never auto-synced.
a1d01ca6a("address codereview feedback — ... auto-sync delay") changed the delay from
retryIntervalto a fixed 100ms and left the deps and the guard untouched: nostatement about either.
⇒ the code branch of triage's fork, not the documentation branch.
What changed
Point 1 — the stale guard. The effect is keyed on the boolean
queue.length > 0, never onqueueand never onqueue.length. Thesuppression's real reason is preserved intact: a boolean does not change when a
second mutation lands, so a mutation queued while the 100ms stabilization timer
is already armed does not re-run the effect and does not restart the timer.
What the suppression never justified — evaluating the emptiness guard against a
snapshot from whenever
isOnlinelast changed — is what moved.Point 2 — the two halves of one call.
syncreadbatchSizethrough a ref(newest) and
queuefrom its own closure (a snapshot), and the auto-sync effectretains such a closure by design. The queue now reaches
syncthrough the samecommit-phase mirror the sync config already uses (
useInsertionEffect, theshape objectui#6797 established here), so both halves are the newest committed
values.
The
eslint-disableis deleted, not reworded. That is a consequence ratherthan a separate edit: mirroring the queue takes
queueout ofsync'sdependency list, so
syncis keyed[enabled]and is stable across queuedmutations — which means it can be named in the effect's dependency array
without costing a timer restart. The array is
[isOnline, enabled, hasPendingMutations, sync]and is genuinely exhaustive. Proven, not asserted:dropping
syncfrom it makesreact-hooks/exhaustive-depsreport "React HookuseEffect has a missing dependency: 'sync'" at
useOffline.ts:392, so the ruleis live on this effect and its silence with the full array is a reading.
No option or parameter is added:
OfflineConfigandOfflineResultareuntouched, and the exported symbol lines of the file are byte-identical to the
base. This is not a public-surface widening.
Verification
A green suite proves nothing here — nothing in this repo reaches the sync queue
— so every pin drives a real mutation through it.
packages/react/src/hooks/__tests__/useOffline.autoSyncWhileOnline.test.tsx:syncbatches the newest queue, not its snapshotPin 2 is the discriminating one: keyed on the queue (or on
queue.length) theeffect would re-arm at t=60 for t=160, so at t=105 nothing would have drained.
Ablation — implementation committed first, then
useOffline.tsreplacedwith its
1e14d70content. Mutation confirmed on disk before measuring (blob4a6fc50to49fe3a3;hasPendingMutations0 occurrences, theeslint-disableback to 1). Result:
Tests 3 failed | 5 passed (8)— pins 1, 2 and 4 red, pin 3and all four objectui#6797 pins green. Restore proven, not assumed:
git checkout HEAD -- ABS_PATH, blob back to4a6fc50,git diff HEADempty.No rebuild leg applies — both test files import
../useOfflineby relativesource path, so no
distis on the resolution path.Gates, all at the final head
2e8e724(union re-run after the last commit):pnpm exec vitest run --maxWorkers=2 packages/react/ packages/app-shell/src/layout/__tests__/ packages/app-shell/src/hooks/__tests__/sharedInboxFeed.twoSurfaces.test.tsx—Test Files 97 passed (97) · Tests 1033 passed (1033)pnpm --filter @object-ui/react type-check— clean (tsc --noEmit && tsc -p tsconfig.test.json). Both edited test files are inside that program:--listFilesreports 1 occurrence each, so "typecheck is clean" is a statement about them.pnpm --filter @object-ui/react build— cleancheck:control-bytes—check-control-bytes: OK (scanned 5710 tracked text file(s)); plus a raw scan over the diff, no hitscheck:changeset-no-major—No changeset declares a major bump.check:vi-mock-specifiers,check:doc-fences,check:phantom-deps,check:self-import,check:spec-symbols,check:skills-paths— all OKcheck:readme-exportsis NOT MEASURED, not green and not red: it exits 1with
the population COLLAPSED -- this run proves nothing · packagesRead: found 5, floor is 25, because it needs a repo-widepnpm buildand this worktreebuilt only
@object-ui/reactand its dependency closure. It is CI-owned. Italso cannot move on this diff: the file's exported symbol lines are identical to
the base.
Lint is a declared narrowing, with its three measurements. Repo-wide
turbo run lintis CI's run; here eslint ran on the diff. (1) The populationcomes from eslint's own config, not from a guess — the changeset
.mdisreported
File ignored because no matching configuration was supplied, so thepopulation is the 3
.ts/.tsxfiles. (2) The count is read from--format json: 3 files, 0 errors, 1 warning, the same pre-existingreact-hooks/set-state-in-effecton an untouched line that the base carries.(3) Untouched files cannot move:
eslint.config.jssets noparserOptions.project,no
projectServiceand norecommendedTypeChecked, so no linting here istype-aware and a change to these three files cannot alter any verdict elsewhere.
Premise checks (the dispatch asked for these to be re-measured)
like. The card cited
useOffline.ts:343-351ona77a00c2c; PR fix(react): stop writing config refs during render in useETagCache, useGlobalUndo and useOffline #6815 hassince landed on this same hook, and the auto-sync effect on
1e14d70isstill at 343-351 — the ref work moved where
syncConfigRefis written, notwhat the auto-sync closure reads. The premise holds:
syncstill readbatchSizefrom the ref andqueuefrom its closure.useOfflinestill hasexactly one in-repo consumer destructuring
isOnlineonly, but it is atpackages/app-shell/src/layout/AppHeader.tsx:132, not:140. Its three__tests__neighbours andsharedInboxFeed.twoSurfacesmock the hookoutright. Nothing in this repo reaches the queue — confirmed, and it is why
the pins above had to drive it directly.
unmeasured out-of-repo population, so the changeset is graded
minorandspells the change out, rather than
patch.No
skills/**file is in this diff, so the published-skill line budget does notapply.
Out of scope, filed unassigned
useOfflinesnippet destructuresqueue, whichOfflineResultdoes not have, and lists asyncedsync statethat
SyncStatedoes not have (while omittingoffline). A concrete instanceof the systemic gap recorded in objectui#5465; not repaired here, since a
published skill document is outside this card's file surface.
batchSizesmaller than the queuestill drains one batch and leaves the remainder for the next transition. That
is the pre-existing behaviour and the fix strictly shrinks the stranded set,
but whether one auto-sync should chain batches to empty is a decision about
what
batchSizemeans, not a bug in this guard. The code comment points atthat card so the edge is not re-derived a third time.
Neither is touched by this PR.
Generated by Claude Code