fix(iOS): Connection-state-flicker#118
Conversation
… connect start() returns immediately after startVPNTunnel() while the tunnel process is still launching. The premature connectPressed=false caused vpnDisplayState to briefly show .disconnected before the tunnel reached .connected, making the UI skip the connecting animation entirely. Delay the fallback reset by 8s to give the tunnel time to start and the polling cycle to update extensionState before we decide the launch failed.
📝 WalkthroughWalkthrough
ChangesVPN Connection Retry Logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NetBird/Source/App/ViewModels/MainViewModel.swift (1)
246-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFire-and-forget Task leaves UI stuck in "Connecting…" for 8 s when the user explicitly disconnects
performClose()never resetsconnectPressed, and theTask {}handle is discarded so it cannot be interrupted from outside. When the user presses Disconnect within the 8-second window,connectPressedstaystrue— every subsequentupdateVPNDisplayState()call triggered by the.disconnecting/.disconnectedextension state continues to return.connecting, keeping the UI frozen on "Connecting…" until the timer fires ~8 s later.Additionally,
Task.sleep()automatically checks for cancellation and throws aCancellationError; silencing it withtry?means that if the Task were ever cancelled, the failure-detection block below the sleep would still execute and mutate state incorrectly.Recommended fix — store the Task, cancel it in
performClose(), and honour cancellation with ado-catch-return:🐛 Proposed fix
+ /// Cancellable handle for the post-connect failure-detection timer. + private var connectTask: Task<Void, Never>? func performConnect() { self.connectPressed = true // ... + connectTask?.cancel() // cancel any previous in-flight timer - Task { + connectTask = Task { self.logger.info("connect: Task started, calling networkExtensionAdapter.start()") await self.networkExtensionAdapter.start() self.logger.info("connect: networkExtensionAdapter.start() completed") - try? await Task.sleep(nanoseconds: 8_000_000_000) // 8 seconds + do { + try await Task.sleep(nanoseconds: 8_000_000_000) // 8 seconds + } catch { + return // Cancelled — user disconnected or a new connect was requested + } if self.extensionState == .disconnected && !self.networkExtensionAdapter.showBrowser { self.connectPressed = false self.updateVPNDisplayState() } } } func performClose() { + connectTask?.cancel() + connectTask = nil + self.connectPressed = false // clear "Connecting…" guard on explicit disconnect self.disconnectPressed = true // ... }This ensures:
- Explicit disconnect immediately clears
connectPressed, so "Disconnecting…" / "Disconnected" is shown without the 8-second delay.- Rapid re-taps on Connect cancel the previous timer before starting a new one.
- Tunnel startup noise (iOS emitting
.disconnectingduringstartVPNTunnel) is still suppressed becauseperformClose()is not called in that path.- Cancellation is honoured — stale state mutation after task cancellation is prevented.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NetBird/Source/App/ViewModels/MainViewModel.swift` around lines 246 - 263, The fire-and-forget Task started after calling networkExtensionAdapter.start() should be stored in a cancellable Task property (e.g. startTimeoutTask) so it can be cancelled from performClose(); replace the discarded Task { ... } with an assignment to that property, cancel startTimeoutTask inside performClose(), and change the sleep from try? await Task.sleep(...) to a do { try await Task.sleep(...) } catch { return } so CancellationError is honoured and the post-sleep failure-detection block (which mutates connectPressed and calls updateVPNDisplayState()) does not run after cancellation; keep all checks against extensionState and networkExtensionAdapter.showBrowser as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@NetBird/Source/App/ViewModels/MainViewModel.swift`:
- Around line 246-263: The fire-and-forget Task started after calling
networkExtensionAdapter.start() should be stored in a cancellable Task property
(e.g. startTimeoutTask) so it can be cancelled from performClose(); replace the
discarded Task { ... } with an assignment to that property, cancel
startTimeoutTask inside performClose(), and change the sleep from try? await
Task.sleep(...) to a do { try await Task.sleep(...) } catch { return } so
CancellationError is honoured and the post-sleep failure-detection block (which
mutates connectPressed and calls updateVPNDisplayState()) does not run after
cancellation; keep all checks against extensionState and
networkExtensionAdapter.showBrowser as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9f9146f-a973-4151-9a32-c301c5880d83
📒 Files selected for processing (1)
NetBird/Source/App/ViewModels/MainViewModel.swift
|
/testflight |
|
TestFlight builds uploaded |
2 participants
Problem
During VPN connection, the UI briefly flashes a "Disconnected" state and then
jumps directly to "Connected", skipping the "Connecting..." animation entirely.
Root cause
networkExtensionAdapter.start()is an async function that returns as soon asstartVPNTunnel()is called — the tunnel process hasn't actually launched yet.At this exact moment:
extensionStateis still.disconnected(the polling cycle runs every 3sand hasn't fired yet)
showBrowserisfalse(no login required)So the fallback guard immediately fires: