Add window focus refetching#13210
Open
jerelmiller wants to merge 40 commits intorelease-4.2from
Open
Conversation
🦋 Changeset detectedLatest commit: 812aef9 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 10 changed, 0 removedBuild ID: e37497985f15fe4e527d6152 URL: https://www.apollographql.com/docs/deploy-preview/e37497985f15fe4e527d6152 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
📝 WalkthroughWalkthroughAdds a RefetchEventManager to Apollo Client enabling opt-in, event-driven refetching of active queries (e.g., window focus, online). Introduces manager lifecycle, event sources/handlers, new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AC as ApolloClient
participant REM as RefetchEventManager
participant ES as EventSource
participant Browser as Browser/Window
participant OQ as ObservableQuery
App->>AC: new ApolloClient({ refetchEventManager })
AC->>REM: connect(client)
REM->>ES: invoke with emit callback
ES->>Browser: addEventListener('online'|'visibilitychange')
ES-->>REM: return cleanup
Browser->>ES: trigger event
ES->>REM: emit(event)
REM->>REM: validate client & configured source
REM->>AC: refetchQueries({ include: "active" })
loop per active ObservableQuery
AC->>OQ: consult query.refetchOn / defaults
alt should refetch
OQ->>AC: perform refetch
else skip
OQ-->>AC: no-op
end
end
App->>AC: client.stop()
AC->>REM: disconnect()
REM->>ES: invoke cleanup
ES->>Browser: removeEventListener()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/core/ApolloClient.ts (1)
1107-1124: Merge logic forrefetchOnlooks correct; consider a brief comment.The two-step approach (let
mergeOptionsoverwriterefetchOn, then merge per-event maps when both sides are objects) correctly handles thefalse/object asymmetry:falseon either side wins as a hard disable, and per-event maps are deep-merged with per-call entries taking precedence. Worth a short inline comment explaining this so the special-casing isn't surprising to future readers.📝 Optional comment
if (this.defaultOptions.watchQuery) { const defaultRefetchOn = this.defaultOptions.watchQuery.refetchOn; const refetchOn = options.refetchOn; options = mergeOptions( this.defaultOptions.watchQuery as typeof options, options ); + // When both default and per-call `refetchOn` are event maps, merge them + // so per-call entries override defaults at the per-event level. If + // either side is `false`, `mergeOptions` already produced the desired + // "hard disable" / "object overrides" behavior. if ( refetchOn && typeof refetchOn === "object" && defaultRefetchOn && typeof defaultRefetchOn === "object" ) { options.refetchOn = { ...defaultRefetchOn, ...refetchOn }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ApolloClient.ts` around lines 1107 - 1124, Add a brief inline comment above the watchQuery merge block (around the code using this.defaultOptions.watchQuery, mergeOptions, defaultRefetchOn, refetchOn, and options.refetchOn) explaining the two-step merge rationale: call mergeOptions first so a boolean false on either side disables refetching entirely, then when both sides are objects shallow-merge the per-event maps so per-call entries override defaults; this documents why the special-casing exists and prevents future surprise changes.src/core/refetchSources/windowFocusSource.ts (1)
5-15: Usedocument.addEventListenerfor thevisibilitychangeevent instead ofwindow.The
visibilitychangeevent is dispatched on theDocumentaccording to the HTML spec and MDN Web Docs. While the event bubbles towindowand the current code works in practice, listening directly ondocumentis the canonical pattern. This also aligns the implementation with the official API documentation and would require updating the test to dispatch ondocumentrather thanwindowfor consistency.Suggested change
export const windowFocusSource: RefetchEventManager.EventSource = function windowFocusSource(emit) { - if (typeof window !== "undefined" && window.addEventListener) { + if (typeof document !== "undefined" && document.addEventListener) { function handleChange() { if (document.visibilityState === "visible") { emit(); } } - window.addEventListener("visibilitychange", handleChange, false); + document.addEventListener("visibilitychange", handleChange, false); return () => { - window.removeEventListener("visibilitychange", handleChange); + document.removeEventListener("visibilitychange", handleChange); }; } };Also update the test to dispatch on
documentinstead ofwindow:-window.dispatchEvent(new Event("visibilitychange")); +document.dispatchEvent(new Event("visibilitychange"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/refetchSources/windowFocusSource.ts` around lines 5 - 15, The event listener for visibilitychange is currently attached to window; change it to use document.addEventListener and document.removeEventListener instead so the listener is registered on the Document per the spec. In the function where handleChange is defined (handleChange) and emit() is called, replace window.addEventListener("visibilitychange", handleChange, false) with document.addEventListener(...), and similarly replace window.removeEventListener(...) in the returned cleanup; also update the corresponding test to dispatch the visibilitychange event on document rather than window.src/core/RefetchEventManager.ts (1)
83-97: Consider lettingsetEventSourceaccepttruefor symmetry with the constructor.
Options.sourcesacceptstrue | EventSource, butsetEventSourceis typed only asEventSource. This means users who construct a manager without sources (or callremoveSource) and later want an imperative-only event have no typed way to register it post-construction — they have to pass a no-op() => () => {}function. Allowingtruehere would close the gap.♻️ Suggested signature change
- setEventSource(event: RefetchEvent, source: RefetchEventManager.EventSource) { - this.cleanupFns.get(event)?.(); - this.cleanupFns.delete(event); - this.sources[event] = source; - - if (this.client) { - const cleanup = source(() => { - this.emit(event); - }); - - if (cleanup) { - this.cleanupFns.set(event, cleanup); - } - } - } + setEventSource( + event: RefetchEvent, + source: true | RefetchEventManager.EventSource + ) { + this.cleanupFns.get(event)?.(); + this.cleanupFns.delete(event); + this.sources[event] = source; + + if (this.client && typeof source === "function") { + const cleanup = source(() => { + this.emit(event); + }); + + if (cleanup) { + this.cleanupFns.set(event, cleanup); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/RefetchEventManager.ts` around lines 83 - 97, The setEventSource method currently only accepts an EventSource but Options.sources allows true|EventSource; update RefetchEventManager.setEventSource to accept source: RefetchEventManager.EventSource | true, and treat true as a marker for an imperative-only source (store it in this.sources and skip invoking source when true); maintain existing cleanup logic (call and delete this.cleanupFns.get(event) etc.) and ensure when source === true you do not call source as a function and you remove any existing cleanup for the event; update type references accordingly so callers can pass true post-construction just like the constructor option.src/core/__tests__/RefetchEventManager.test.ts (1)
1472-1885: Reconnect, multi-client, and post-construction source mutation are all exercised.Reconnecting after a disconnect re-invoking source functions, the warning when a second
ApolloClientsteals the manager (with cleanup of the prior wiring),setEventSourceadding events not in the constructor, deferred invocation until connect, void-cleanup tolerance, andremoveSourceagainsttrue/never-declared/pre-connect cases — this set of edge cases is exactly what I'd want here. Nice work.Optional housekeeping: the per-test
ApolloClient+MockLinkboilerplate is repeated very heavily. If this file grows much further, a smallsetupClient({ sources, defaultRefetchOn })helper would make the suite easier to scan without losing locality. Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/__tests__/RefetchEventManager.test.ts` around lines 1472 - 1885, Tests repeat ApolloClient + MockLink + InMemoryCache + refetchEventManager setup; extract a small helper (e.g., setupClient or createTestClient) that accepts options like sources, refetchEventManager, and mock link entries and returns an instantiated ApolloClient and any emitters needed; update tests that construct new ApolloClient(...) to call this helper and reuse the created client and stream setup, keeping existing usages of RefetchEventManager, MockLink, InMemoryCache, and ObservableStream intact so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/gentle-windows-watch.md:
- Around line 80-98: The snippet uses AppStateStatus and Platform but only
imports AppState, causing a TypeScript error; update the top-level imports for
the example to include AppStateStatus and Platform from "react-native" (so the
handleChange parameter and the Platform.OS check are valid) or, alternatively,
remove the AppStateStatus type annotation and the Platform check; refer to the
RefetchEventManager construction, the reactNativeAppStatus source, and the
handleChange function when applying the import or removal change.
- Around line 7-9: Fix the prose typos in the changeset: correct "network
reconnect ship as `windowFocusSource` and `onlineSource`" to a grammatically
correct phrase such as "network reconnect are available as `windowFocusSource`
and `onlineSource`" or "are shipped as `windowFocusSource` and `onlineSource`",
and remove the duplicated word in "to activate the the event listeners" so it
reads "to activate the event listeners"; ensure mentions of RefetchEventManager
and ApolloClient remain unchanged.
- Line 67: The changeset incorrectly refers to the option path as
defaultOptions.refetchOn; update the text to use the correct option path
defaultOptions.watchQuery.refetchOn so it matches the other references (lines
showing defaultOptions.watchQuery.refetchOn) and accurately describes where the
merge with per-query refetchOn occurs; ensure both occurrences and any
surrounding explanatory sentence consistently use
defaultOptions.watchQuery.refetchOn.
In `@src/react/types/types.documentation.ts`:
- Around line 169-176: The documentation for the refetchOn property currently
only mentions disabling events but the declared type is false |
Partial<Record<RefetchEvent, boolean>>, so the per-event map can also enable
events by setting them to true; update the comment for refetchOn to state that
passing false disables all automatic refetches, while passing a
Partial<Record<RefetchEvent, boolean>> lets callers enable (true) or disable
(false) specific events (reference the refetchOn symbol and its type signature
in the comment).
---
Nitpick comments:
In `@src/core/__tests__/RefetchEventManager.test.ts`:
- Around line 1472-1885: Tests repeat ApolloClient + MockLink + InMemoryCache +
refetchEventManager setup; extract a small helper (e.g., setupClient or
createTestClient) that accepts options like sources, refetchEventManager, and
mock link entries and returns an instantiated ApolloClient and any emitters
needed; update tests that construct new ApolloClient(...) to call this helper
and reuse the created client and stream setup, keeping existing usages of
RefetchEventManager, MockLink, InMemoryCache, and ObservableStream intact so
behavior is unchanged.
In `@src/core/ApolloClient.ts`:
- Around line 1107-1124: Add a brief inline comment above the watchQuery merge
block (around the code using this.defaultOptions.watchQuery, mergeOptions,
defaultRefetchOn, refetchOn, and options.refetchOn) explaining the two-step
merge rationale: call mergeOptions first so a boolean false on either side
disables refetching entirely, then when both sides are objects shallow-merge the
per-event maps so per-call entries override defaults; this documents why the
special-casing exists and prevents future surprise changes.
In `@src/core/RefetchEventManager.ts`:
- Around line 83-97: The setEventSource method currently only accepts an
EventSource but Options.sources allows true|EventSource; update
RefetchEventManager.setEventSource to accept source:
RefetchEventManager.EventSource | true, and treat true as a marker for an
imperative-only source (store it in this.sources and skip invoking source when
true); maintain existing cleanup logic (call and delete
this.cleanupFns.get(event) etc.) and ensure when source === true you do not call
source as a function and you remove any existing cleanup for the event; update
type references accordingly so callers can pass true post-construction just like
the constructor option.
In `@src/core/refetchSources/windowFocusSource.ts`:
- Around line 5-15: The event listener for visibilitychange is currently
attached to window; change it to use document.addEventListener and
document.removeEventListener instead so the listener is registered on the
Document per the spec. In the function where handleChange is defined
(handleChange) and emit() is called, replace
window.addEventListener("visibilitychange", handleChange, false) with
document.addEventListener(...), and similarly replace
window.removeEventListener(...) in the returned cleanup; also update the
corresponding test to dispatch the visibilitychange event on document rather
than window.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4b5c9dbc-6d65-41f4-bb58-293a5308b18f
📒 Files selected for processing (18)
.changeset/gentle-windows-watch.mdsrc/core/ApolloClient.tssrc/core/ObservableQuery.tssrc/core/RefetchEventManager.tssrc/core/__tests__/RefetchEventManager.test.tssrc/core/index.tssrc/core/refetchSources/__tests__/onlineSource.test.tssrc/core/refetchSources/__tests__/windowFocusSource.test.tssrc/core/refetchSources/onlineSource.tssrc/core/refetchSources/windowFocusSource.tssrc/core/types.tssrc/react/hooks/useBackgroundQuery.tssrc/react/hooks/useLazyQuery.tssrc/react/hooks/useLoadableQuery.tssrc/react/hooks/useQuery.tssrc/react/hooks/useSuspenseQuery.tssrc/react/query-preloader/createQueryPreloader.tssrc/react/types/types.documentation.ts
jerelmiller
force-pushed
the
jerel/window-focus-refetch
branch
from
15767a0 to
5bff0bc
Compare
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.changeset/gentle-windows-watch.md (1)
85-91:⚠️ Potential issue | 🟡 MinorImport
Platformin the React Native example.The snippet uses
Platform.OSbut never importsPlatform, so the copy-paste example does not type-check as written.✏️ Proposed fix
-import { AppState, AppStateStatus } from "react-native"; +import { AppState, AppStateStatus, Platform } from "react-native";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/gentle-windows-watch.md around lines 85 - 91, The example references Platform.OS in the reactNativeAppStatus source (inside the handleChange function passed to new RefetchEventManager) but never imports Platform; add an import for Platform from "react-native" alongside the existing AppState/AppStateStatus import so the snippet type-checks and Platform.OS is available in reactNativeAppStatus/handleChange.
🧹 Nitpick comments (1)
src/core/refetchSources/__tests__/windowFocusSource.test.ts (1)
3-12: Optional test-isolation hardening: restore the original descriptor.Line 4 overrides
document.visibilityState; restoring the original descriptor inafterAllwould make this helper safer for future test evolution.🔧 Optional refactor
+const originalVisibilityStateDescriptor = Object.getOwnPropertyDescriptor( + document, + "visibilityState" +); + function setVisibilityState(state: DocumentVisibilityState) { Object.defineProperty(document, "visibilityState", { configurable: true, get: () => state, }); } afterEach(() => { setVisibilityState("visible"); }); + +afterAll(() => { + if (originalVisibilityStateDescriptor) { + Object.defineProperty( + document, + "visibilityState", + originalVisibilityStateDescriptor + ); + } +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/refetchSources/__tests__/windowFocusSource.test.ts` around lines 3 - 12, The helper setVisibilityState overrides document.visibilityState but doesn't restore the original property descriptor; capture the original descriptor (via Object.getOwnPropertyDescriptor(document, "visibilityState")) before redefining it, continue using setVisibilityState to set the getter in tests, and add an afterAll that restores the original descriptor with Object.defineProperty(document, "visibilityState", originalDescriptor) so the global document property is returned to its prior state (reference symbols: setVisibilityState and the test lifecycle hooks afterEach/afterAll).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/ApolloClient.ts`:
- Around line 1110-1111: The stop logic currently calls
RefetchEventManager.disconnect unconditionally which can tear down a manager
that has been re-bound to another client; update RefetchEventManager.disconnect
to accept an optional client parameter and early-return if a client is provided
and does not match this.client (i.e., only perform cleanup when disconnect is
called by the client that currently owns the manager), and update callers
(places setting this.refetchEventManager and stop()) to pass the invoking
ApolloClient instance when calling disconnect so connect() can safely move
managers between clients without races.
In `@src/core/refetchSources/windowFocusSource.ts`:
- Around line 5-14: The visibilitychange handler is attached to window but the
event fires on document; update the registration in the block that defines
handleChange/emit so you call document.addEventListener("visibilitychange",
handleChange, false) and document.removeEventListener("visibilitychange",
handleChange) in the returned cleanup; also ensure you check typeof document !==
"undefined" (or document && document.addEventListener) before using document to
avoid SSR errors.
In `@src/react/hooks/useLazyQuery.ts`:
- Around line 81-82: The new refetchOn option is declared but not propagated
when options change; update the incremental update path that builds
updatedOptions (the variable used inside the effect that applies option updates,
referenced as updatedOptions) to include options.refetchOn so changes to
refetchOn after mount are forwarded to the observable; locate where
updatedOptions is created/merged in useLazyQuery (and any related effect that
applies option diffs) and ensure you copy or assign options.refetchOn into
updatedOptions (alongside other forwarded fields like fetchPolicy, variables,
etc.) so the observable receives the updated refetchOn value.
In `@src/react/types/types.documentation.ts`:
- Around line 177-179: Update the documentation text for the refetchOn option to
avoid implying it is enabled by default; revise the sentence referencing
`refetchOn` and `defaultOptions.watchQuery.refetchOn` so it clearly states that
`refetchOn` is opt-in and only triggers refetch behavior when explicitly set (or
when `defaultOptions.watchQuery.refetchOn` is `true`), e.g., replace "defaults
to refetch on all events" with phrasing that makes the opt-in nature explicit
and explains when `refetchOn: true` is necessary.
---
Duplicate comments:
In @.changeset/gentle-windows-watch.md:
- Around line 85-91: The example references Platform.OS in the
reactNativeAppStatus source (inside the handleChange function passed to new
RefetchEventManager) but never imports Platform; add an import for Platform from
"react-native" alongside the existing AppState/AppStateStatus import so the
snippet type-checks and Platform.OS is available in
reactNativeAppStatus/handleChange.
---
Nitpick comments:
In `@src/core/refetchSources/__tests__/windowFocusSource.test.ts`:
- Around line 3-12: The helper setVisibilityState overrides
document.visibilityState but doesn't restore the original property descriptor;
capture the original descriptor (via Object.getOwnPropertyDescriptor(document,
"visibilityState")) before redefining it, continue using setVisibilityState to
set the getter in tests, and add an afterAll that restores the original
descriptor with Object.defineProperty(document, "visibilityState",
originalDescriptor) so the global document property is returned to its prior
state (reference symbols: setVisibilityState and the test lifecycle hooks
afterEach/afterAll).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7e8b108a-5bd9-499f-b161-2d175652c7a1
⛔ Files ignored due to path filters (1)
src/__tests__/__snapshots__/exports.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
.api-reports/api-report-core.api.md.api-reports/api-report-react.api.md.api-reports/api-report.api.md.changeset/gentle-windows-watch.md.size-limits.jsonsrc/core/ApolloClient.tssrc/core/ObservableQuery.tssrc/core/RefetchEventManager.tssrc/core/__tests__/RefetchEventManager.test.tssrc/core/index.tssrc/core/refetchSources/__tests__/onlineSource.test.tssrc/core/refetchSources/__tests__/windowFocusSource.test.tssrc/core/refetchSources/onlineSource.tssrc/core/refetchSources/windowFocusSource.tssrc/core/types.tssrc/react/hooks/useBackgroundQuery.tssrc/react/hooks/useLazyQuery.tssrc/react/hooks/useLoadableQuery.tssrc/react/hooks/useQuery.tssrc/react/hooks/useSuspenseQuery.tssrc/react/query-preloader/createQueryPreloader.tssrc/react/types/types.documentation.ts
Comment on lines
+1110
to
+1111
| this.refetchEventManager = refetchEventManager; | ||
| this.refetchEventManager?.connect(this); |
There was a problem hiding this comment.
Don't disconnect a manager that has already been rebound to another client.
connect() explicitly supports moving a RefetchEventManager between clients, but stop() always calls disconnect(). If client A shares a manager with client B and B connects later, stopping A will silently tear down B's event sources too.
🐛 Proposed fix
public stop() {
this.queryManager.stop();
- this.refetchEventManager?.disconnect();
+ this.refetchEventManager?.disconnect(this);
}// src/core/RefetchEventManager.ts
disconnect(client?: ApolloClient) {
if (client && this.client !== client) {
return;
}
this.client = undefined;
this.cleanupFns.forEach((cleanup) => cleanup());
this.cleanupFns.clear();
}Also applies to: 1203-1205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/ApolloClient.ts` around lines 1110 - 1111, The stop logic currently
calls RefetchEventManager.disconnect unconditionally which can tear down a
manager that has been re-bound to another client; update
RefetchEventManager.disconnect to accept an optional client parameter and
early-return if a client is provided and does not match this.client (i.e., only
perform cleanup when disconnect is called by the client that currently owns the
manager), and update callers (places setting this.refetchEventManager and
stop()) to pass the invoking ApolloClient instance when calling disconnect so
connect() can safely move managers between clients without races.
Comment on lines
+5
to
+14
| if (typeof window !== "undefined" && window.addEventListener) { | ||
| function handleChange() { | ||
| if (document.visibilityState === "visible") { | ||
| emit(); | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener("visibilitychange", handleChange, false); | ||
| return () => { | ||
| window.removeEventListener("visibilitychange", handleChange); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the browser visibilitychangeevent fire ondocumentorwindow, and does it bubble?
💡 Result:
The visibilitychange event fires on the document object, not the window object. It is a generic Event that does not bubble.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Document/visibilitychange_event
- 2: https://developer.mozilla.org/en-US/docs/Web/Events/visibilitychange
- 3: https://caniuse.com/mdn-api_document_visibilitychange_event
Attach visibilitychange listener to document, not window.
This handler checks document.visibilityState, but the listener is registered on window. The visibilitychange event fires on the document object only and does not bubble, so the current implementation will never emit.
🐛 Proposed fix
export const windowFocusSource: RefetchEventManager.EventSource =
function windowFocusSource(emit) {
- if (typeof window !== "undefined" && window.addEventListener) {
+ if (typeof document !== "undefined" && document.addEventListener) {
function handleChange() {
if (document.visibilityState === "visible") {
emit();
}
}
- window.addEventListener("visibilitychange", handleChange, false);
+ document.addEventListener("visibilitychange", handleChange, false);
return () => {
- window.removeEventListener("visibilitychange", handleChange);
+ document.removeEventListener("visibilitychange", handleChange);
};
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (typeof window !== "undefined" && window.addEventListener) { | |
| function handleChange() { | |
| if (document.visibilityState === "visible") { | |
| emit(); | |
| } | |
| } | |
| window.addEventListener("visibilitychange", handleChange, false); | |
| return () => { | |
| window.removeEventListener("visibilitychange", handleChange); | |
| if (typeof document !== "undefined" && document.addEventListener) { | |
| function handleChange() { | |
| if (document.visibilityState === "visible") { | |
| emit(); | |
| } | |
| } | |
| document.addEventListener("visibilitychange", handleChange, false); | |
| return () => { | |
| document.removeEventListener("visibilitychange", handleChange); | |
| }; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/refetchSources/windowFocusSource.ts` around lines 5 - 14, The
visibilitychange handler is attached to window but the event fires on document;
update the registration in the block that defines handleChange/emit so you call
document.addEventListener("visibilitychange", handleChange, false) and
document.removeEventListener("visibilitychange", handleChange) in the returned
cleanup; also ensure you check typeof document !== "undefined" (or document &&
document.addEventListener) before using document to avoid SSR errors.
Comment on lines
+81
to
+82
| /** {@inheritDoc @apollo/client!QueryOptionsDocumentation#refetchOn:member} */ | ||
| refetchOn?: RefetchEventManager.RefetchOnOption; |
There was a problem hiding this comment.
refetchOn is typed but not propagated on option updates.
Line 82 adds refetchOn, but the incremental update path (updatedOptions in the effect) never forwards it, so changing options.refetchOn after mount won’t update the observable.
💡 Proposed fix
const updatedOptions: Partial<ObservableQuery.Options<TData, TVariables>> =
{
query,
errorPolicy: stableOptions?.errorPolicy,
refetchWritePolicy: stableOptions?.refetchWritePolicy,
returnPartialData: stableOptions?.returnPartialData,
notifyOnNetworkStatusChange: stableOptions?.notifyOnNetworkStatusChange,
+ refetchOn: stableOptions?.refetchOn,
nextFetchPolicy: options?.nextFetchPolicy,
skipPollAttempt: options?.skipPollAttempt,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** {@inheritDoc @apollo/client!QueryOptionsDocumentation#refetchOn:member} */ | |
| refetchOn?: RefetchEventManager.RefetchOnOption; | |
| const updatedOptions: Partial<ObservableQuery.Options<TData, TVariables>> = | |
| { | |
| query, | |
| errorPolicy: stableOptions?.errorPolicy, | |
| refetchWritePolicy: stableOptions?.refetchWritePolicy, | |
| returnPartialData: stableOptions?.returnPartialData, | |
| notifyOnNetworkStatusChange: stableOptions?.notifyOnNetworkStatusChange, | |
| refetchOn: stableOptions?.refetchOn, | |
| nextFetchPolicy: options?.nextFetchPolicy, | |
| skipPollAttempt: options?.skipPollAttempt, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react/hooks/useLazyQuery.ts` around lines 81 - 82, The new refetchOn
option is declared but not propagated when options change; update the
incremental update path that builds updatedOptions (the variable used inside the
effect that applies option updates, referenced as updatedOptions) to include
options.refetchOn so changes to refetchOn after mount are forwarded to the
observable; locate where updatedOptions is created/merged in useLazyQuery (and
any related effect that applies option diffs) and ensure you copy or assign
options.refetchOn into updatedOptions (alongside other forwarded fields like
fetchPolicy, variables, etc.) so the observable receives the updated refetchOn
value.
Comment on lines
+177
to
+179
| * `refetchOn` defaults to refetch on all events. `refetchOn: true` is | ||
| * unnecessary except when `defaultOptions.watchQuery.refetchOn` is `false`. | ||
| * |
There was a problem hiding this comment.
Default-behavior remark appears misleading for an opt-in feature.
Lines 177-179 state that refetchOn defaults to all events, which conflicts with the expected opt-in behavior and can mislead configuration.
📝 Suggested wording update
- * `refetchOn` defaults to refetch on all events. `refetchOn: true` is
- * unnecessary except when `defaultOptions.watchQuery.refetchOn` is `false`.
+ * `refetchOn` inherits from `defaultOptions.watchQuery.refetchOn`.
+ * Use `true` to enable all automatic refetch events for this query, or
+ * an object to enable/disable specific events.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * `refetchOn` defaults to refetch on all events. `refetchOn: true` is | |
| * unnecessary except when `defaultOptions.watchQuery.refetchOn` is `false`. | |
| * | |
| * `refetchOn` inherits from `defaultOptions.watchQuery.refetchOn`. | |
| * Use `true` to enable all automatic refetch events for this query, or | |
| * an object to enable/disable specific events. | |
| * |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react/types/types.documentation.ts` around lines 177 - 179, Update the
documentation text for the refetchOn option to avoid implying it is enabled by
default; revise the sentence referencing `refetchOn` and
`defaultOptions.watchQuery.refetchOn` so it clearly states that `refetchOn` is
opt-in and only triggers refetch behavior when explicitly set (or when
`defaultOptions.watchQuery.refetchOn` is `true`), e.g., replace "defaults to
refetch on all events" with phrasing that makes the opt-in nature explicit and
explains when `refetchOn: true` is necessary.
phryneas
reviewed
Apr 28, 2026
| }; | ||
| }, | ||
| }, | ||
| }); |
Member
There was a problem hiding this comment.
Looking at this, should we use rxjs more here?
Any source (windowFocusSource, onlineSource) could be an observable.
import { AppState, AppStateStatus, Platform } from "react-native";
import {Observable, filter} from "rxjs"
const reactNativeAppStatusSource = new Observable<AppStateStatus>((subscriber) => {
const subscription = AppState.addEventListener("change", subscriber.next)
return () => subscription.remove()
}).pipe(filter(status => Platform.OS !== "web" && status === "active"))
const refetchEventManager = new RefetchEventManager({
sources: {
reactNativeAppStatus: reactNativeAppStatusSource
}
});And instead of just true in the declaration, we could even have more nuanced types, so
declare module "@apollo/client" {
interface RefetchEvents {
- reactNativeAppStatus: true;
+ reactNativeAppStatus: AppStateStatus
}
}would be possible.
RefetchEventManager itself would only treat emitted events, even with undefined as emitted events - but RefetchEvent could go from a string to { type: "windowFocus" } | { type: "reactNativeAppStatus", value: AppStateStatus }, which would give the callback even more nuanced situational control.
phryneas
reviewed
Apr 28, 2026
Comment on lines
+1242
to
+1250
| if ( | ||
| refetchOn && | ||
| typeof refetchOn === "object" && | ||
| defaultRefetchOn && | ||
| typeof defaultRefetchOn === "object" | ||
| ) { | ||
| options.refetchOn = { ...defaultRefetchOn, ...refetchOn }; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I think here we need to think about how "merging" would work for functions, this logic right now wouldn't cover it, and a defaultOption function right now wouldn't be passed on, even if no refetchOn function or object were defined, if I read this correctly.
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
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.
Closes apollographql/apollo-feature-requests#422
Adds support for window focus refetching as well as support for refetching on any custom event.
For a more detailed API overview, see the changeset
Summary by CodeRabbit
New Features
Tests
Documentation