iOS: offline cache expiry/eviction, cold-start fix, check-in queue (#25-#28) - #155
Open
willy-de7 wants to merge 1 commit into
Open
Conversation
Implements the offline-support batch (ethos-protocol#25-ethos-protocol#28): - ethos-protocol#27: NetworkMonitor no longer defaults isConnected=true at cold start; it now reads NWPathMonitor's synchronous currentPath snapshot before any async pathUpdateHandler callback fires, behind a fakeable NetworkPathProvider seam. - ethos-protocol#25: OfflineCache entries are now timestamped; OfflineCache.age(for:)/ cachedAt(for:) expose staleness, an optional maxAge threshold refuses to serve entries older than it, and VaultStore.vaultsCacheAge surfaces this to the UI as "showing vaults from X ago". - ethos-protocol#26: OfflineCache is now capped by maxBytes with LRU eviction (by file mtime, bumped on load without disturbing the separate cachedAt timestamp), and cleared entirely on sign-out. - ethos-protocol#28: check-ins attempted while offline are now durably queued (CheckInQueue, flat-file JSON) instead of lost, retried by CheckInSyncService (piggybacked on the existing BGAppRefreshTask plus an opportunistic flush on reconnect), with the same non-retryable-error distinction Android's CheckInSyncWorker makes, and surfaced via a local notification + in-app banner mirroring NotificationHelper.showQueuedCheckIn. Also fixes several pre-existing compile-breaking bugs in files this batch had to touch (a botched prior merge had left Stores.swift/Views.swift with duplicate declarations and dangling references, and a few singletons had private initializers/setters their own existing tests called across file boundaries) — see PR description for details.
|
@willy-de7 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the four related iOS offline-support issues as one coherent change, since they all touch
OfflineSupport.swift/APIClient.swift/VaultStore:NetworkMonitor.isConnected's Optimistic Default at Cold Start #27 —NetworkMonitor.isConnectedno longer optimistically defaults totrueat cold start.OfflineCacheEntries #25 —OfflineCacheentries are now timestamped, with staleness surfaced to the UI.OfflineCache#26 —OfflineCacheis now bounded by a byte-size cap with LRU eviction, and cleared on sign-out.PendingCheckInDao/CheckInSyncWorker/NotificationHelper.showQueuedCheckIn.What changed
OfflineSupport.swiftNetworkMonitorreadsNWPathMonitor.currentPathsynchronously at init (via a newNetworkPathProviderseam, mirroringAPIClient's existing testable-init pattern) instead of defaultingisConnected = trueand waiting for the first asyncpathUpdateHandlercallback. Closes the cold-start window where a request made right after launch on an offline device would attempt (and time out on) a real network call instead of using the cache path.OfflineCachenow writes a sibling.metatimestamp file alongside each cached entry. NewcachedAt(for:)/age(for:)expose it; an optionalmaxAgemakesload(for:)treat over-age entries as absent.OfflineCachegained a configurablemaxBytescap (default 20 MB) enforced on everysave(_:for:), evicting least-recently-loaded entries first (tracked via file mtime, bumped onload(for:)without touching the separatecachedAttimestamp so staleness display stays accurate even after a cache hit).OfflineCache.clearAll()wipes the cache directory; called fromAuthStore.signOut()(ties into Clear All Local State on Sign-Out, Not Just the Token #10) so a second user on the same device is never served the first user's cached vault data offline.CheckInQueue.swift/CheckInSyncService.swift(new)CheckInQueue: flat-file JSON queue ofPendingCheckIn(vaultID, queuedAt), the iOS counterpart to Android's Room-backedPendingCheckInDao.CheckInSyncService.flush(): retries every queued check-in; only drops an entry onAPIError.notFound(the vault no longer exists — a definitive rejection), leaving everything else (offline, server error, expired auth) queued for the next retry. Mirrors the non-retryable-error distinctionCheckInSyncWorkermakes on Android (which has finer-grained HTTP status codes available than iOS's currentAPIErrorsurface; widening that is out of scope for this batch).Stores.swiftVaultStore.checkInnow queues toCheckInQueueonAPIError.networkUnavailableinstead of just surfacing an error.VaultStore.load()opportunistically flushes the queue when connectivity is present, and always keepsqueuedCheckInCount(published) in sync.@Published var vaultsCacheAge: TimeInterval?, set fromAPIClient.vaultsCacheAge()whenever a load was served from cache.AuthStore.signOut()now callsOfflineCache.shared.clearAll().BackgroundRefreshService.swifthandleRefreshnow also callsCheckInSyncService.shared.flush()(piggybacking on the existing hourlyBGAppRefreshTaskrather than registering a second background task identifier, per the issue's explicitly-allowed alternative).NotificationService.swiftshowQueuedCheckIn(count:)/cancelQueuedCheckIn(), the iOS equivalent ofNotificationHelper.showQueuedCheckIn/cancelQueuedCheckIn. iOS has no true "ongoing" notification API, so this re-posts/cancels a fixed-identifier local notification;VaultStore.queuedCheckInCountbacks an in-app banner for while the app is foregrounded.Views.swiftVaultListViewshows a banner for offline cache age ("Offline — showing vaults from X ago", viaRelativeDateTimeFormatter) and for queued check-ins.VaultDetailViewshows a queued-check-in note under the Check In button.Pre-existing issues found and fixed
Several files this batch had to touch didn't compile on
mainbefore this change (a prior merge —96b0e1f, and one further back — had left duplicate/dangling declarations behind). Since these blocked compiling the exact files #25–#28 needed to modify, and blocked the pre-existing test suite entirely, they're fixed here rather than skipped:Stores.swift:VaultStorehad two conflictingscheduleReminders()bodies and was missingapplyUpdate/refreshSingle/deposit/withdraw/updateBeneficiary(already covered by existingVaultStoreTests/UI code that referenced them) — reconstructed by merging the two feature sets a bad merge had collapsed.Views.swift:VaultDetailViewhad a duplicateload2FAStatus()and referencedttlRemaining/refreshTTLPeriodically()/showDeposit/showWithdraw/showManageBeneficiarywithout declaring them.BackgroundRefreshService.swift:private init()and aprivate(set)counter were called directly fromHandleRefreshTests/BackgroundRefreshServiceTestsin another file — inaccessible under Swift's access control regardless of@testable import(verified against a real Swift compiler in isolation). Widened tointernal, matching the same patternAPIClient's own designated init already documents. Also fixedhandleRefreshassigning through an immutable, non-class-constrained protocol parameter (task.expirationHandler = ...), and avar-across-Task{}Sendable-capture error introduced by that fix.Tests/EthosProtocolTests.swift: one test method usedtry XCTSkipIfwithout being markedthrows.Tests/APIClientTests.swift:makeTestInstancecalled theprivate convenience init()via an Objective-C KVC hack that can't work (APIClientisn'tNSObject) and wouldn't compile from another file anyway — switched to theinit(baseURL:session:retryPolicy:)seamAPIClient.swiftalready exposes (and documents) for exactly this purpose.None of these touch behavior outside what #25–#28 already required changing in these files.
Verification performed
This sandbox has no Xcode/macOS, so real
xcodebuild/swift testagainst Apple frameworks (Network, CryptoKit, BackgroundTasks, UserNotifications, Combine, UIKit) couldn't be run directly. Instead:OfflineSupport.swift,Stores.swift,CheckInQueue.swift,CheckInSyncService.swift,NotificationService.swift,BackgroundRefreshService.swift,APIClient.swiftagainst it — confirms these type-check.BackgroundRefreshServiceTests,HandleRefreshTests, andCancellationTestsagainst that build: 41/41 pass (13 correctly self-skip via the existingXCTSkipIf(CI)pattern for tests needing a realUNUserNotificationCenterhost process, matching howios-ci.ymlalready runs this suite).VaultStoreTests(applyUpdate) type-checks correctly under the same harness, but couldn't execute there — Linuxswift-corelibs-xctest's test discovery crashes on@MainActor-isolatedXCTestCaseclasses, a toolchain-only limitation unrelated to this change (real Xcode/XCTest doesn't have this issue).task.expirationHandler = ...through an immutable protocol parameter,private init()cross-file access) were additionally isolated and confirmed against a real Swift 5.9 compiler outside the full package, to be certain they're genuine bugs and not toolchain artifacts.Views.swift's SwiftUI code (which the Linux toolchain can't compile at all) against the original working implementations from the commits the bad merges diverged from, and re-read the whole file for balance/consistency.Test plan
ios-ci.yml, real Xcode/simulator) runsEthosProtocol-PackageSPM tests + the hostedEthosProtocolTestsXcode scheme — should now pass where it previously wouldn't even compile.BGAppRefreshTaskfire, forced in Xcode's debugger, or by pulling to refresh).~/Library/Caches/EthosProtocolOfflineCache.Known pre-existing issues (not in scope)
Tests/APIClientTests.swift'sAPIClientOfflineCacheTestshas several tests (test_GETOfflineWithCache_returnsCachedData,test_GETOfflineNoCachedData_throwsNetworkUnavailable,test_POSTOffline_neverFallsBackToCache,test_DELETEOffline_neverFallsBackToCache) that are placeholders — they compile and pass but don't actually exerciseNetworkMonitor.shared.isConnected(there's no seam to fake it there yet) or callAPIClient.checkIn/unregisterPushToken. Left as-is: fixing them is a test-infra improvement outside this batch's scope, and out of caution against scope creep beyond Add Expiry toOfflineCacheEntries #25-Add an Offline Check-In Queue to iOS (Parity With Android) #28.Tests/HostedTests/andTests/UI/(XCUITest) weren't exercised by the Linux verification harness (they need a real app host / simulator either way) — unaffected by this change (no symbols they reference were touched).Closes #25
Closes #26
Closes #27
Closes #28