Skip to content

[client, android] Fix/android enable server route #3806

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pappz
Copy link
Contributor

@pappz pappz commented May 9, 2025

Describe your changes

Enable the server route; otherwise, the manager throws an error and the engine will restart.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@Copilot Copilot AI review requested due to automatic review settings May 9, 2025 12:28
Copy link

sonarqubecloud bot commented May 9, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
1 New issue
1 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an Android bug by enabling the server route, eliminating the previous error that caused the engine to restart on Android.

  • Removed the Android-specific server router file (server_android.go) that previously returned an error.
  • Removed the build tag in server.go to include Android in the supported platforms.
  • Updated notifier code to filter out dynamic (domain) routes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
client/internal/routemanager/server_android.go Removed the file that previously blocked server routes on Android.
client/internal/routemanager/server.go Removed the Android exclusion build tag to enable support in this file.
client/internal/routemanager/notifier/notifier.go Added filtering logic for domain (dynamic) routes in client routes.
Comments suppressed due to low confidence (1)

client/internal/routemanager/server.go:1

  • [nitpick] Since the removal of the Android exclusion build tag now enables the server route on Android, please ensure that any platform-specific differences are documented accordingly.
package routemanager

@@ -32,6 +32,10 @@ func (n *Notifier) SetListener(listener listener.NetworkChangeListener) {
func (n *Notifier) SetInitialClientRoutes(clientRoutes []*route.Route) {
nets := make([]string, 0)
for _, r := range clientRoutes {
// filter out domain routes
Copy link
Preview

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider clarifying in the comment that 'dynamic' specifically refers to routes based on domains to improve maintainability and clarity for future reviewers.

Suggested change
// filter out domain routes
// Filter out routes that are dynamic, specifically those based on domains

Copilot uses AI. Check for mistakes.

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