feat(util): shared cache and reconnect-reload for useRPCLoad - #29501
Merged
Conversation
An engine reset orphans in-flight RPC promises without settling them, so a forced reload/reconnect load in useRPCLoad/useCachedResource was piggybacking on the orphan and hanging forever instead of firing a fresh request.
use-cached-resource moved to util/ on this branch and master kept adding importers and tests to it, so the merge is mostly repointing those. Master's forced-load handling supersedes this branch's: it invalidates an in-flight request whose sequence predates the forced load rather than skipping adoption whenever force is set, which covers the orphaned-rpc case this branch fixed and still collapses concurrent forced reloads into one request. Kept master's version and carried this branch's regression test for the orphan case over to the moved test file. Dropped the committed plan doc; plans stay untracked.
The cache option shipped with no call sites: the commit that added it migrated nothing onto it, and nothing since imports it. It also diverged from useCachedResource in three ways that would have bitten whoever adopted it first - a forced load never bumped the store generation, so a superseded request still wrote its pre-change result and stamped a fresh loadedAt; failures never called setLoadFailed, so the backoff did not apply; and the cache-seeded initial state ignored `enabled`. Deleting is cheaper than fixing an unused path, and re-adding it later against a corrected useCachedResource beats carrying a second divergent implementation of the same semantics. The reconnect wiring stays - it never needed a cache and is live for all 15 callers. `force` was only ever read by the store branch, so load() loses its parameter. Two of the deleted tests covered behavior that still exists without a store, so they are rewritten cache-free. The superseding test now resolves the superseded request LAST; the old one used a promise that never settled, which is why it could not have caught the generation bug.
N consumers of one cache reacting to a single event cost N rpcs. React commits effects one component at a time, so the second consumer always asks after the first one's request is already on the wire; a rule of "supersede anything in flight" therefore fires for every consumer after the first, and each invalidates its predecessor's result. The comment claiming this "still collapses the N-mounted-consumers-reload-together case to one RPC" was describing a branch that could never be reached. Measured against a service built from this checkout: one reconnect cost 4 identical getAnnotatedTeam inside 106ms and 3 teamListUnverified. Reloads now carry the epoch of the event that asked for them. One event allocates one epoch and hands the same number to every consumer, so they collapse onto the first request instead of superseding it. A bare reload() allocates a fresh epoch and still supersedes, which is what an ordinary user-initiated refresh should do. Two checks are needed and the in-flight one alone fixed nothing measurable: consumers do not necessarily overlap. 75ms was enough for the first teamListUnverified to settle before the second consumer's effect ran, leaving nothing to collapse onto, so a forced load also serves the cache when its loadedEpoch already covers the event. Epoch sources: the daemon handshake generation for reconnects (memoized per generation, since consumers fire in separate effects), and one allocation per invalidation broadcast. The per-consumer engine notification handlers still call bare reload() - they debounce independently, so sharing an epoch there means allocating it after the debounce, which is not a mechanical change. reload(epoch?) guards on typeof because it gets handed straight to onClick in places, where a react event would otherwise arrive as the epoch. After: the reconnect costs 1 teamListUnverified, and the team page's 4 getAnnotatedTeam drop to 2 - one for the reconnect, one for the team notification that follows it, which is a genuinely later event.
The hook subscribed teamMetadataUpdate, teamRoleMapChanged and teamChangedByID to three separate bare reload() calls. One logical change fires all three, and a reconnect fires them on top of the reconnect's own reload - each is a distinct event, so each supersedes the last instead of collapsing. Measured against a service built from this checkout: 4 getAnnotatedTeam for one team inside 116ms after a single reconnect. Route them through one debouncer, matching useReloadOnTeamChanges in use-teams-list, which already treats these notifications as the burst they are. Leading so the ordinary single notification still reloads immediately, trailing so changes arriving mid-burst are not dropped. Takes the reconnect from 4 getAnnotatedTeam to 3: the reconnect's own load, then the burst's leading and trailing edges. Getting below that means either dropping the trailing edge or folding notifications that a reconnect itself provoked into the reconnect event, both of which change behavior beyond coalescing.
The reconnect tests never moved handshakeGeneration, so epochForGeneration memoized one epoch for the whole file and "one reconnect is one epoch" - the point of reload-epoch - went unasserted. Bump the generation the way startHandshake does, then assert the epoch itself: shared across hooks for one reconnect, strictly greater for the next. Fill in the branches with no coverage at all: clear(), a cacheKey swap, a disabled resource across a reconnect, and useRPCLoad's when: 'focus' / when: 'manual' / enabled: false paths. Focus needs a NavigationContext, without which useSafeFocusEffect no-ops and the branch is unreachable from a test. Covering the key swap showed the reset in the main effect duplicated the one in loadResource, which every load path goes through: deleting either left the behavior intact. Keep the general one and leave the effect with the case loadResource cannot see, a hook going disabled while its key changes - nothing runs loadResource to notice, so a shared cache would sit on data keyed to something nobody asked for.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades the RPC-loading utility hooks with a shared-cache-shaped model (staleness, in-flight dedupe, and event-epoch invalidation) and adds reconnect-aware reload behavior to prevent orphaned in-flight RPCs from leaving hooks stuck on stale data.
Changes:
- Introduces
useReloadOnReconnectplus a shared “reload epoch” mechanism to coordinate reload bursts across multiple consumers. - Enhances
useCachedResourceto dedupe concurrent reloads, supersede older in-flight requests safely, and avoid redundant refetches for the same event. - Repoints callers from the former teams-scoped cached-resource module to the shared
util/use-cached-resource.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/util/use-rpc-load.tsx | Adds reconnect-triggered reload to unstick orphaned in-flight loads. |
| shared/util/use-rpc-load.test.tsx | Adds unit coverage for mount/focus/manual/enabled semantics and reconnect reload for useRPCLoad. |
| shared/util/use-reload-on-reconnect.tsx | New hook that fires on observed handshake transitions into done, providing a shared epoch per reconnect. |
| shared/util/use-reload-on-reconnect.test.tsx | Tests reconnect detection semantics and epoch sharing across hooks. |
| shared/util/reload-epoch.tsx | New epoch allocator used to collapse “one event → one reload RPC” across consumers. |
| shared/util/use-cached-resource.tsx | Moves/extends cached resource logic with epoch-based in-flight superseding and reconnect reload wiring. |
| shared/util/use-cached-resource.test.tsx | Consolidated/expanded tests validating caching, backoff, epoch semantics, reconnect behavior, and disabled behavior. |
| shared/util/use-mutual-teams.tsx | Updates imports to shared util/use-cached-resource. |
| shared/teams/use-teams-list.tsx | Updates invalidation listeners to propagate a shared epoch into reload calls; import path update. |
| shared/teams/team/use-loaded-team.tsx | Updates import path and adds debounced coalescing of team-change notifications. |
| shared/teams/team/settings-tab/retention/index.tsx | Updates import path to shared util/use-cached-resource. |
| shared/teams/team/settings-tab/default-channels.tsx | Updates import path to shared util/use-cached-resource. |
| shared/teams/common/use-loaded-team-channels.tsx | Updates invalidation listeners to propagate a shared epoch into reload calls; import path update. |
| shared/teams/common/general-conv.tsx | Updates import path to shared util/use-cached-resource. |
| shared/teams/common/activity.tsx | Updates import path to shared util/use-cached-resource. |
| shared/chat/user-emoji.tsx | Updates import path to shared util/use-cached-resource. |
| shared/chat/conversation/team-hooks.tsx | Updates import path to shared util/use-cached-resource. |
| shared/stores/daemon.tsx | Adds handshakeGeneration so reconnect-aware hooks can distinguish reconnect events. |
| shared/teams/use-cached-resource.test.tsx | Removes the old teams-scoped cached-resource tests (migrated to util). |
Creating the debounced function by mutating a ref during render breaks render purity. Build it once in a useState initializer instead and cancel it from the effect cleanup.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
TanStack-Query-shaped upgrades to the RPC loading hooks, without adopting TanStack Query itself — its pull model doesn't fit push-driven stores plus interactive RPCs.
teams/use-cached-resource.tsx→util/use-cached-resource.tsx. It stopped being teams-specific a while ago; master has since added importers fromchat/,util/andteams/, all repointed here.useRPCLoadgains reconnect-triggered reload. (An earlier revision of this branch also added an optional sharedcache: {staleMs, store}touseRPCLoad; it had no callers, so it was dropped —useCachedResourcealready covers shared-cache needs.)util/use-reload-on-reconnect.tsx: fires when the daemon'shandshakeStatetransitions intodonefrom some other observed state, so mounting while already connected doesn't fire. Wired into bothuseRPCLoadanduseCachedResource, gated onenabled— a disabled instance must not touch the shared cache.useLoadedTeamcoalesces team-change notifications (metadata / role map / changedByID) behind one leading+trailing debounce, so a reconnect burst is onegetAnnotatedTeaminstead of four.Sharers of one
useCachedResourcecache need an equivalentmap, since the in-flight promise resolves to mappedDATA.Tests
util/use-rpc-load.test.tsx,util/use-reload-on-reconnect.test.tsx, and the mergedutil/use-cached-resource.test.tsx.yarn lint:allclean, 0 react-compiler bailouts.Merge note
This branch predates #29466, which reworked forced loads in
use-cached-resourceindependently. Master's version won: it invalidates an in-flight request whose sequence number predates the forced load, rather than refusing in-flight adoption wheneverforceis set. That covers the orphaned-RPC hazard this branch was fixing — an engine reset leaves in-flight RPCs that never settle, so areload()that adopts one hangs forever — and it additionally lets several consumers reloading together collapse onto a single request. The regression test for the orphan case came along and passes against master's implementation.