Skip to content

fix(android): surface ipv6 vpn limitation#29

Closed
AmirrezaFarnamTaheri wants to merge 1 commit into
Hidden-Node:mainfrom
AmirrezaFarnamTaheri:android-vpn-ipv6-warning
Closed

fix(android): surface ipv6 vpn limitation#29
AmirrezaFarnamTaheri wants to merge 1 commit into
Hidden-Node:mainfrom
AmirrezaFarnamTaheri:android-vpn-ipv6-warning

Conversation

@AmirrezaFarnamTaheri
Copy link
Copy Markdown
Contributor

Summary

  • show an in-app warning while VPN mode is active or preparing that the current Android VPN path routes IPv4 only
  • keep VPN routing behavior unchanged and simply make the existing service log visible to users

Validation

  • git diff --check
  • gradle :app:compileDebugKotlin reaches Kotlin compilation but is blocked by the existing missing gomobile mobile package/AAR setup.

Copy link
Copy Markdown
Owner

@Hidden-Node Hidden-Node left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hard work on these improvements! The new sharing credential actions and the local IP display are great features that significantly enhance the user experience.

However, given the large volume of changes in this PR, I have a couple of suggestions to ensure the long-term maintainability and robustness of the code:

Reactive Network IP Updates: Currently, the local IP seems to be remembered only when the screen is first composed. If a user changes their network state (e.g., toggles Wi-Fi or enables a Hotspot) while the screen is open, the displayed IP will become stale. It would be better to monitor network changes and update the IP address dynamically.
Code Organization & Refactoring: GlobalSettingsScreen.kt has become quite large and complex. To improve readability, I suggest moving the logic for password generation and local IP discovery into the ViewModel or a dedicated Utility/Helper class.
Also, please make sure to thoroughly test these changes on a physical device or emulator yourself before submitting the updates, specifically checking how the UI behaves during network transitions.

Could you please address these points? Once the logic is refactored and the IP display is more reactive, I'll be happy to review it again for merging. Thanks!

@Hidden-Node
Copy link
Copy Markdown
Owner

Sorry, I posted this comment here by mistake. This review was intended for PR #31. Please ignore this.

@Hidden-Node
Copy link
Copy Markdown
Owner

Thanks for your contribution and for pointing out this limitation. However, we have decided not to merge this PR because full IPv6 routing support is planned for the next update. Since this limitation will be addressed soon at the core level, we'd like to avoid adding temporary UI warnings that will become obsolete shortly. We appreciate your effort in improving the app's transparency!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants