docs(react-native): fix example to include initial state#10172
docs(react-native): fix example to include initial state#10172TkDodo merged 2 commits intoTanStack:mainfrom
Conversation
|
github-actions
bot
added
the
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds immediate initialization in onlineManager.setEventListener by calling Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/framework/react/react-native.md`:
- Around line 43-53: The async init in onlineManager.setEventListener can be
overwritten by a later listener event and lacks error handling: introduce a
local guard flag (e.g., "initialResultIgnored" or "stale") inside the
setEventListener callback that is flipped when the synchronous
addNetworkStateListener fires (or when its callback runs) so the Promise result
from Network.getNetworkStateAsync() only calls setOnline if the listener hasn't
already provided a newer state; also attach .catch(...) to
getNetworkStateAsync() to log/handle errors, and ensure you still return
eventSubscription.remove from the setEventListener callback (use the same
identifiers: Network.getNetworkStateAsync, Network.addNetworkStateListener,
setOnline, eventSubscription.remove).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the React Native documentation to fix the expo-network example for managing online status. The change addresses the issue where expo-network's addNetworkStateListener does not emit the current state immediately upon subscription, which causes the onlineManager to incorrectly default to "online" on cold starts even when the device is offline.
Changes:
- Added logic to explicitly fetch initial network state using
Network.getNetworkStateAsync() - Implemented a race condition guard using an
initialisedflag to prevent duplicate state updates - Added error handling for platforms where
getNetworkStateAsync()may reject
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }) | ||
| .catch(() => { | ||
| // getNetworkStateAsync can reject on some platforms/SDK versions |
There was a problem hiding this comment.
If getNetworkStateAsync() rejects (which the comment acknowledges can happen on some platforms/SDK versions) and the listener hasn't fired yet, the online state will never be initialized. The onlineManager will default to "online" (true), which could be incorrect if the device is actually offline. Consider adding logic in the catch block to handle this scenario, such as assuming offline as a safer default or attempting an alternative method to determine initial connectivity.
| // getNetworkStateAsync can reject on some platforms/SDK versions | |
| // getNetworkStateAsync can reject on some platforms/SDK versions | |
| if (!initialised) { | |
| // Fall back to a safer default when initial state is unknown | |
| setOnline(false) | |
| } |
| let initialised = false | ||
|
|
||
| const eventSubscription = Network.addNetworkStateListener((state) => { | ||
| initialised = true | ||
| setOnline(!!state.isConnected) | ||
| }) | ||
|
|
||
| Network.getNetworkStateAsync() | ||
| .then((state) => { | ||
| if (!initialised) { |
There was a problem hiding this comment.
The variable name uses British spelling "initialised" while the codebase consistently uses American spelling "initialized" (see packages/angular-query-experimental/src/inject-is-fetching.ts:42, packages/react-query-next-experimental/src/HydrationStreamProvider.tsx:159). Consider changing to "initialized" for consistency with the rest of the codebase.
| let initialised = false | |
| const eventSubscription = Network.addNetworkStateListener((state) => { | |
| initialised = true | |
| setOnline(!!state.isConnected) | |
| }) | |
| Network.getNetworkStateAsync() | |
| .then((state) => { | |
| if (!initialised) { | |
| let initialized = false | |
| const eventSubscription = Network.addNetworkStateListener((state) => { | |
| initialized = true | |
| setOnline(!!state.isConnected) | |
| }) | |
| Network.getNetworkStateAsync() | |
| .then((state) => { | |
| if (!initialized) { |
| // getNetworkStateAsync can reject on some platforms/SDK versions | ||
| }) | ||
|
|
||
| return eventSubscription.remove |
There was a problem hiding this comment.
Returning eventSubscription.remove directly may cause a this binding issue when the cleanup function is called. If remove is not bound to eventSubscription, calling it later will result in this being undefined. Consider wrapping it in an arrow function: return () => eventSubscription.remove() to ensure the correct context, similar to the pattern used in examples/react/react-native/src/hooks/useAppState.ts:9.
| return eventSubscription.remove | |
| return () => eventSubscription.remove() |
🎯 Changes
expo-networkdoes not emit the current state immediately upon subscription. This causes onlineManager to default to "online" on cold starts, even if the device is actually offline. So we need to explicitly callNetwork.getNetworkStateAsync()to initialize the state correctly before setting up the listener.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit