Skip to content

KM-15917: ensure "protected" timer shows - #348

Merged
kp-mario-nachbaur merged 9 commits into
masterfrom
KM-15917-connected-protected-timer
Jul 21, 2026
Merged

KM-15917: ensure "protected" timer shows#348
kp-mario-nachbaur merged 9 commits into
masterfrom
KM-15917-connected-protected-timer

Conversation

@kp-mario-nachbaur

Copy link
Copy Markdown
Contributor
  • Expose VPN native "connected date".
  • Use it as fallback if we didn't store the connection time.
  • If we end up in the state of being connected but without connection time, just use the current time as connection time.

@claude

This comment was marked as outdated.

Comment thread LocalPackages/PIALibrary/Sources/PIALibrary/Daemons/VPNDaemon.swift Outdated
Comment thread LocalPackages/PIALibrary/Sources/PIALibrary/VPN/DefaultVPNProvider.swift Outdated
Comment thread LocalPackages/PIALibrary/Sources/PIALibrary/VPN/PIATunnelProfile.swift Outdated
Comment thread PIA VPN/UI/Dashboard/DashboardViewController.swift
@claude

This comment was marked as resolved.

@claude

This comment was marked as resolved.

@kpkb-1f8e9813897fe9831983e89f7143

kpkb-1f8e9813897fe9831983e89f7143 Bot commented Jul 17, 2026

Copy link
Copy Markdown

KB review — cd6bcfd

No findings changes on this PR relative to the default branch. ✅

View in KB

@claude

This comment was marked as outdated.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @kp-mario-nachbaur's task in 1m 38s —— View job


Claude PR Review — KM-15917: ensure "protected" timer shows

  • Gather context (read full diff + touched files)
  • Analyze code quality, bugs, threading, memory, security
  • Post review

Re-reviewed after the latest sync. The new push (da225f11) is a clean master merge that only pulls in unrelated files (ConnectivityDaemon, ServiceQualityManager, ShowConnectionStatsViewController). No functional changes to this PR's feature landed — the fallback logic and profile plumbing are identical to the previously-approved revision. My earlier assessment stands.

🟢 Three-tier fallback — sound

getOrUpdateConnectionTime(isConnected:) (DashboardViewController.swift:100) reads cleanly:

  1. stored lastVPNConnectionSuccess → elapsed from it
  2. native vpnProvider.connectionDate → adopt & persist, then elapsed
  3. isConnected last-resort → persist now, return 0; else nil

The write side-effect fires at most once — after the first tick populates lastVPNConnectionSuccess, every subsequent 1s tick short-circuits on branch 1. All on the main thread (timer on RunLoop.current, updateConnectionTime() main), so the Client.preferences read/write is race-free here.

🟢 No stale timer after disconnect

VPNDaemon sets lastVPNConnectionSuccess = nil on .disconnected (VPNDaemon.swift:227) and connectionDate returns nil when not connected, so a disconnected tick falls through all three branches to nil. The new deinit { removeObserver } (VPNDaemon.swift:49) is correct hygiene for the selector observers.

🟢 Separation of concerns & macCatalyst branch

Splitting state-mutating getOrUpdateConnectionTime from the pure formatted(connectionTime:) resolves the "saving state in a formatter" comment. The pre-iOS-16 .title.titleView change in setNavBar(titleLabel:) is central to the fix — updateConnectionTime() mutates the same label instance, so only .titleView ticks live on macOS 12.

🔵 Intentional duplication — confirmed

The connectionDate guard duplicated across PIATunnelProfile/PIAWGTunnelProfile is intentional (each protocol implements VPNProfile independently). Fine as-is; a shared NETunnelProviderManager extension could DRY it later.

🧪 Test coverage — still the one gap

No tests accompany the new fallback. MockVPNProvider now exposes connectionDate, so all four branches are testable in PIA VPNTests with an injectable preferences store:

  • (a) stored value present → elapsed from stored value
  • (b) stored nil, connectionDate present → adopts native date & persists it
  • (c) both nil but isConnected == true → returns 0 and persists now
  • (d) all nil / disconnected → returns nil

Add these tests →

Flows to verify manually

  • Connect via each protocol (IKEv2 / OpenVPN / WireGuard) → timer appears and counts up.
  • Force-quit & relaunch while connected → timer resumes from native connected time (branch b), not from 0.
  • Disconnect → timer clears, no lingering count-up.
  • macOS 12 Catalyst → nav-bar title ticks live.

Overall: 👍 no blockers, safe to merge. Adding the unit tests above would lock in the four-branch behavior.

@kp-mario-nachbaur
kp-mario-nachbaur merged commit 96728c4 into master Jul 21, 2026
14 checks passed
@kp-mario-nachbaur
kp-mario-nachbaur deleted the KM-15917-connected-protected-timer branch July 21, 2026 11:43
@claude claude Bot mentioned this pull request Jul 21, 2026
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.

3 participants