fix: LiveQuery not working due to ethernet not recognized as connectivity state #1091
Conversation
Part 2 of 2 fixes to address issue parse-community#1042
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds ethernet connectivity support to the Flutter SDK's connectivity handling. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds support for the ethernet connectivity type to align with the connectivity_plus package, completing the fix for LiveQuery connectivity issues (issue #1042). This is part 2 of a two-part change that requires the Dart package update (PR #1090) to be merged first.
Key Changes:
- Added ethernet connectivity checks to both
checkConnectivity()andconnectivityStreammethods - Ethernet is prioritized between wifi and mobile in the connectivity type hierarchy
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
mtrezza
added
the
type:bug
|
The label |
parse-github-assistant
bot
removed
the
type:bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1091 +/- ##
==========================================
+ Coverage 43.54% 46.84% +3.30%
==========================================
Files 61 53 -8
Lines 3587 3266 -321
==========================================
- Hits 1562 1530 -32
+ Misses 2025 1736 -289 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mtrezza
changed the title
|
@kirkmorrow Does the PR title make sense? |
|
@mtrezza This is part 2 of the fix. Feel free to update the title, I just stole the title text from the original bug this addresses. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Okay... apparently the code checkers break if I try to change pubspec.yaml to pick up the required version of parse_server_sdk. @mtrezza should I just revert that change and let you handle it? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/flutter/test/parse_connectivity_mapping_test.dart (1)
118-129: Redundant assertions (same issue as lines 34-47).This test iterates through online states to verify they're not equal to
none. As with the earlier test, this checks Dart's built-in enum equality semantics and adds no meaningful coverage.
🧹 Nitpick comments (2)
packages/flutter/test/parse_connectivity_mapping_test.dart (2)
25-32: Consider removing or replacing this brittle test.Testing specific enum index values couples the test to implementation details. If the enum is reordered (e.g., alphabetically), this test breaks even though the priority logic in the if-else chain would still work correctly. The priority is determined by the order of conditions in the mapping logic, not by enum indices.
34-47: Redundant assertions add no value.These assertions test that different enum values are not equal and that the same enum value equals itself. Since Dart enum values are by definition distinct, these checks are redundant and don't add meaningful test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/flutter/test/parse_connectivity_mapping_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Agent
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Dart beta
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Dart 3.10, Ubuntu
- GitHub Check: Test Flutter 3.38, Windows
🔇 Additional comments (5)
packages/flutter/test/parse_connectivity_mapping_test.dart (5)
8-23: LGTM!The enum structure test appropriately verifies that
ParseConnectivityResultincludes all four expected values, particularlyethernet, which is critical for resolving issue #1042.
107-116: Good documentation of the fix requirement.This test clearly documents that ethernet must be treated as an online state, which is the core requirement for resolving issue #1042. While the assertion itself is trivial (testing enum inequality), the explicit test and reason string effectively document this critical behavior for future maintainers.
Note: Integration tests verifying that LiveQuery actually connects with ethernet would provide more comprehensive coverage beyond enum semantics.
132-146: Useful canary test for library updates.This test explicitly lists all 7
ConnectivityResultvalues from connectivity_plus and verifies the count. This serves as a valuable canary: if connectivity_plus adds new enum values in a future update, this test will fail and alert developers to update the mapping logic accordingly.
149-159: LGTM!This test documents and verifies that the Parse SDK explicitly supports the three main internet connection types (wifi, ethernet, mobile), which aligns with the PR's objective of adding ethernet support.
6-6: Consider removing TestWidgetsFlutterBinding initialization if these are pure unit tests.
TestWidgetsFlutterBinding.ensureInitialized()is required for widget tests that interact with the Flutter framework. If this test file only tests mapping logic without framework interactions, this initialization may be unnecessary and can be removed to reduce test overhead.
| test('mapping logic follows priority: wifi > ethernet > mobile > none', | ||
| () { | ||
| // This documents the if-else chain priority in checkConnectivity() | ||
| // If a list contains wifi, it returns wifi | ||
| // Else if it contains ethernet, it returns ethernet | ||
| // Else if it contains mobile, it returns mobile | ||
| // Else it returns none | ||
|
|
||
| // Simulating the mapping logic | ||
| ParseConnectivityResult mapResult(List<ConnectivityResult> list) { | ||
| if (list.contains(ConnectivityResult.wifi)) { | ||
| return ParseConnectivityResult.wifi; | ||
| } else if (list.contains(ConnectivityResult.ethernet)) { | ||
| return ParseConnectivityResult.ethernet; | ||
| } else if (list.contains(ConnectivityResult.mobile)) { | ||
| return ParseConnectivityResult.mobile; | ||
| } else { | ||
| return ParseConnectivityResult.none; | ||
| } | ||
| } | ||
|
|
||
| // Test single connection types | ||
| expect(mapResult([ConnectivityResult.wifi]), | ||
| ParseConnectivityResult.wifi); | ||
| expect(mapResult([ConnectivityResult.ethernet]), | ||
| ParseConnectivityResult.ethernet); | ||
| expect(mapResult([ConnectivityResult.mobile]), | ||
| ParseConnectivityResult.mobile); | ||
| expect(mapResult([ConnectivityResult.none]), | ||
| ParseConnectivityResult.none); | ||
|
|
||
| // Test priority when multiple connections exist | ||
| expect(mapResult([ConnectivityResult.wifi, ConnectivityResult.ethernet]), | ||
| ParseConnectivityResult.wifi); | ||
| expect(mapResult([ConnectivityResult.wifi, ConnectivityResult.mobile]), | ||
| ParseConnectivityResult.wifi); | ||
| expect( | ||
| mapResult([ConnectivityResult.ethernet, ConnectivityResult.mobile]), | ||
| ParseConnectivityResult.ethernet); | ||
|
|
||
| // Test that ethernet takes priority over mobile (important for issue #1042) | ||
| expect( | ||
| mapResult([ConnectivityResult.mobile, ConnectivityResult.ethernet]), | ||
| ParseConnectivityResult.ethernet); | ||
|
|
||
| // Test fallback behavior for unsupported types | ||
| expect(mapResult([ConnectivityResult.bluetooth]), | ||
| ParseConnectivityResult.none); | ||
| expect(mapResult([ConnectivityResult.vpn]), ParseConnectivityResult.none); | ||
| expect( | ||
| mapResult([ConnectivityResult.other]), ParseConnectivityResult.none); | ||
|
|
||
| // Test mixed with unsupported types | ||
| expect( | ||
| mapResult([ConnectivityResult.ethernet, ConnectivityResult.vpn]), | ||
| ParseConnectivityResult.ethernet); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the actual connectivity implementation in the Parse SDK
find . -name "*.dart" -type f | head -20Repository: parse-community/Parse-SDK-Flutter
Length of output: 210
🏁 Script executed:
# Search for ConnectivityResult usage and actual mapping implementation
rg -n "checkConnectivity|mapResult" --type=dart -B 3 -A 3Repository: parse-community/Parse-SDK-Flutter
Length of output: 210
🏁 Script executed:
# Look for the actual implementation file being tested
rg -n "ConnectivityResult\.(wifi|ethernet|mobile|none)" --type=dart -B 5 -A 5 | head -100Repository: parse-community/Parse-SDK-Flutter
Length of output: 210
🏁 Script executed:
# Find the parse_connectivity file structure
fd "parse_connectivity" --type fRepository: parse-community/Parse-SDK-Flutter
Length of output: 210
🏁 Script executed:
# Check the actual implementation that should be tested
cat -n packages/flutter/lib/src/parse_connectivity.dart 2>/dev/null | head -80Repository: parse-community/Parse-SDK-Flutter
Length of output: 210
🌐 Web query:
Parse Flutter SDK ConnectivityResult checkConnectivity implementation
💡 Result:
Short summary of how checkConnectivity() / ConnectivityResult works (connectivity_plus):
-
Signature & return: Connectivity().checkConnectivity() is async and returns a List (one or more network interface types currently active). [1][2]
-
What the values mean: ConnectivityResult enumerates interface types (wifi, mobile, ethernet, vpn, bluetooth, none, other). The list can contain multiple entries (e.g., vpn + wifi). Use .contains(...) to check for a particular type. [2]
-
Implementation responsibility: The Dart API calls a platform-specific implementation (ConnectivityPlatform). Each platform plugin (Android/iOS/macos/linux/windows) implements checkConnectivity() to query the OS network APIs and map results to ConnectivityResult values. The platform interface defines the API and throws UnimplementedError if not overridden. [3]
-
Practical notes / caveats:
- checkConnectivity reports the active network interface(s), not whether remote Internet access is reachable — you should still perform an actual network request (or ping) for “has internet.” [2]
- Platform/OS differences and bugs exist (e.g., iOS simulator/versions have reported checkConnectivity returning none); platform behavior can change and issues are tracked in the plugin repo. [4]
If you want, I can fetch and paste the exact platform implementation files (Android/iOS/Linux/Windows) and explain line-by-line. Which platform(s) do you want analyzed?
🌐 Web query:
parse-flutter-sdk connectivity mapping ethernet wifi mobile priority
💡 Result:
Short answer: Parse Flutter SDK maps only wifi -> wifi, mobile -> mobile, and everything else -> none — it doesn’t expose ethernet (or other ConnectivityResult values) and thus treats those as none. [1][2]
Sources:
- Parse Flutter SDK connectivityStream implementation (maps ConnectivityResult.wifi → ParseConnectivityResult.wifi, ConnectivityResult.mobile → ParseConnectivityResult.mobile, default → ParseConnectivityResult.none). [1]
- connectivity_plus docs / changelog showing ConnectivityResult includes ethernet (and other types) on supported platforms. [2][3]
References:
[1] Parse SDK Flutter — connectivityStream implementation.
[2] connectivity_plus package docs (connectivity types & usage).
[3] connectivity_plus changelog (ethernet support added).
🌐 Web query:
Parse Flutter SDK issue 1042 ethernet connectivity
💡 Result:
Summary: The Parse Flutter SDK’s connectivityStream maps connectivity_plus values but only handles wifi and mobile — any other ConnectivityResult (including ethernet) falls to the default branch and is returned as ParseConnectivityResult.none. That causes devices on Ethernet to appear as “no connectivity” to the Parse SDK. Source code excerpt shows the mapping only for wifi/mobile and default -> none. [1]
Context / evidence:
- Parse SDK docs (connectivityStream implementation) — maps ConnectivityResult.wifi -> ParseConnectivityResult.wifi, ConnectivityResult.mobile -> ParseConnectivityResult.mobile, all other cases -> ParseConnectivityResult.none. [1]
- Related upstream bug reports show connectivity_plus returning ConnectivityResult.ethernet on macOS/desktop when both Wi‑Fi and LAN are present (so ethernet is a real value that needs handling). [2]
Impact: Apps using Parse’s connectivityStream will treat ethernet-connected devices as offline (ParseConnectivityResult.none), affecting logic that depends on network state (e.g., LiveQuery reconnect logic, background sync).
Minimal fixes:
- Short-term: Patch parse_server_sdk_flutter to map ConnectivityResult.ethernet to an appropriate ParseConnectivityResult (e.g., wifi or add a new ethernet enum) in connectivityStream.
- Alternate: Update consumer code to use connectivity_plus directly (and handle ethernet) rather than relying on Parse’s connectivityStream.
References:
- Parse SDK connectivityStream implementation. [1]
- connectivity_plus issue reporting ethernet-only result on desktop. [2]
Test expects ethernet connectivity handling that does not exist in the actual Parse SDK implementation.
The mapResult function tests logic for mapping ConnectivityResult.ethernet to ParseConnectivityResult.ethernet (lines 65, 88), but the current Parse Flutter SDK's connectivityStream only handles wifi and mobile—ethernet and other connectivity types are mapped to ParseConnectivityResult.none. This test will pass while devices on ethernet fail silently in production.
If this test is for issue #1042 (adding ethernet support), it should either:
- Be marked as a failing test or skipped until ethernet handling is implemented, or
- Be accompanied by changes to the actual Parse SDK
connectivityStreamto map ethernet appropriately.
If this test is meant for the current implementation, remove the ethernet-related test cases (lines 75-77, 88-90, 102-104) and the ethernet priority logic from mapResult.
🤖 Prompt for AI Agents
In packages/flutter/test/parse_connectivity_mapping_test.dart around lines 49 to
105, the test asserts handling and priority for ConnectivityResult.ethernet but
the current Parse Flutter SDK maps ethernet to none; update the test to match
the implementation by either (A) removing the ethernet-specific expectations and
the ethernet branch in mapResult (remove tests at lines ~75-77, ~88-90, ~102-104
and the etherenet condition), or (B) mark those ethernet-related test cases as
skipped/expected to fail until ethernet support is implemented, or (C) implement
ethernet mapping in the SDK and then keep the tests—choose one approach and make
the corresponding change so tests reflect the actual supported behavior.
There was a problem hiding this comment.
@kirkmorrow What would be the response to this review feedback?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Revert pubspec.yaml changes
|
I'm not sure how to address the failing checks |
2 participants
Part 2 of 2 fixes to address issue #1042
PR for part 1 is #1090
Pull Request
Issue
Closes: #1042
This is part 2 of 2 required for this fix. The Dart package update will need to be committed first. Then this PR will need to update pubspec.yaml to use that new package before this can be merged.
Approach
Use new ethernet enum to return a connection type consistent with connectivity_plus.
Tasks
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.