-
Notifications
You must be signed in to change notification settings - Fork 18
App stuck in NetBird logo on cold start #50
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
Conversation
Since the slow configureManager() call was removed from NetworkExtensionAdapter.init() and the remaining operations are lightweight, this asynchronous pattern isn't necessary anymore. Reason for this commit: app is stuck on netbird logo display on cold starts. This was corrected but reintroduced when this ViewModelLoader was introduced.
Issue wasn't related to Firebase
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughUpdates project build/version settings and moves ViewModel creation to an async Changes
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant NetBirdApp
participant MainActor
participant ViewModel
participant MainView
participant Polling
participant ScenePhase
AppDelegate->>NetBirdApp: application launch
NetBirdApp->>MainActor: spawn async Task (`@MainActor`)
MainActor->>ViewModel: async initialize MainViewModel
ViewModel-->>MainActor: initialization complete
MainActor->>MainView: render with ViewModel
MainView->>ViewModel: onAppear / activationTask -> loadCurrentConnectionState()
ViewModel->>Polling: evaluate -> startPollingDetails()
Polling-->>Polling: polling loop runs
Note over ScenePhase,MainView: tvOS uses scenePhase for lifecycle
ScenePhase->>Polling: active -> startPollingDetails()
ScenePhase->>Polling: inactive -> stopPollingDetails()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@NetBird.xcodeproj/project.pbxproj`:
- Line 1411: Update the NetbirdNetworkExtension target so its MARKETING_VERSION
matches the main app by changing NetbirdNetworkExtension's MARKETING_VERSION to
0.0.16 and bump its CURRENT_PROJECT_VERSION (currently 7) to 8; locate the
NetbirdNetworkExtension block in the project.pbxproj and update the
MARKETING_VERSION and CURRENT_PROJECT_VERSION keys for that target to keep the
extension version consistent with the main app.
🧹 Nitpick comments (1)
NetBird/Source/App/NetBirdApp.swift (1)
121-129: Consider documenting or verifying the Task scheduling behavior.The ViewModel is created inside a
Task {@mainactorin }block within the initializer. While this correctly offloads the Go runtime initialization, be aware that:
- The Task starts asynchronously, so there's a brief window where
viewModelisnil- The
@Publishedproperty update will trigger a view refresh when the ViewModel becomes availableThis is the intended behavior for this fix, but ensure that the Go SDK initialization within
ViewModel.init()doesn't have any main-thread requirements that could cause issues when called from an async context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes an iOS cold-start issue where the app could remain stuck on the NetBird logo because polling never started if didBecomeActiveNotification fired before the asynchronously-created ViewModel was available.
Changes:
- Adds a
.onAppearfallback to start polling onceMainViewis shown (covers misseddidBecomeActiveNotificationduring async ViewModel init). - Updates the loading/splash UI layout (safe area handling + logo sizing).
- Updates project version settings (marketing/build versions and search path formatting).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| NetBird/Source/App/Views/MainView.swift | Adjusts the “logo-only” state UI layout and sizing. |
| NetBird/Source/App/NetBirdApp.swift | Adds .onAppear polling fallback and updates the async ViewModel loading screen. |
| NetBird.xcodeproj/project.pbxproj | Changes app version settings (marketing/build versions, library search paths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NetBird.xcodeproj/project.pbxproj (1)
1173-1173: NetBirdTVNetworkExtension version mismatch with tvOS app.The tvOS app (
NetBird TV) was bumped toMARKETING_VERSION = 0.0.16, but its bundled network extension (NetBirdTVNetworkExtension) remains atMARKETING_VERSION = 1.0. For consistency and to avoid potential App Store validation issues, consider aligning the extension version with the host app.🔧 Suggested fix
Update both Debug (line 1173) and Release (line 1210) configurations for
NetBirdTVNetworkExtension:- MARKETING_VERSION = 1.0; + MARKETING_VERSION = 0.0.16;Also applies to: 1210-1210
♻️ Duplicate comments (2)
NetBird.xcodeproj/project.pbxproj (2)
1231-1243: Good – iOS network extension version synced to 0.0.16.This addresses the earlier feedback about the
NetbirdNetworkExtensionversion mismatch.Regarding
CURRENT_PROJECT_VERSION = 3: this was already flagged in a previous review concerning the build number potentially decreasing. If the value was previously higher (e.g., 7), App Store Connect will reject uploads with a lowerCFBundleVersion. Please ensure the build number is monotonically increasing.Also applies to: 1261-1273
1411-1411: Build number decrease concern (duplicate).
CURRENT_PROJECT_VERSION = 3for the main NetBird app may be problematic if previously submitted builds had a higher value. This was already flagged in a prior review. Please verify the build history and ensureCFBundleVersionincreases monotonically before release.Also applies to: 1457-1457
The app is stuck displaying the NetBird logo even when things are done loading in the background; when switching contexts to other apps then back to itself, things are displayed correctly.
This PR was created to investigate reasons as to why this was happening and fix the issue.
startPollingDetailswas never called because thedidBecomeActiveNotificationnotification was fired, but no handler existed yet since ViewModel creation is done asynchronously.startPollingDetailsis the method that eventually sets thestatusDetailsValidproperty in viewModel to true, which is what tells the app whether to display the logo or the main view itself.The fix was to also start polling in .onAppear, which serves as a fallback to ensure polling starts even if didBecomeActiveNotification was missed during async ViewModel initialization.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.