Skip to content

refactor(ios): encode persistence state in Loadable enum, throw on save#1261

Merged
datlechin merged 1 commit into
mainfrom
refactor/ios-loadable-persistence-state
May 14, 2026
Merged

refactor(ios): encode persistence state in Loadable enum, throw on save#1261
datlechin merged 1 commit into
mainfrom
refactor/ios-loadable-persistence-state

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Follow-up to #1260. That PR closed the bug chain that wiped iCloud-synced connections after the build 12 to 13 update. This PR cleans up the three architecture smells that fix carried over, without changing user-visible behavior.

Smells fixed

  1. Parallel persistenceIntegrity flag ran alongside the data arrays. Two fields described one concept ("is the load valid?"), and the initial .ok value lied for the brief window before loadPersistedData() ran. Replaced with a Loadable<T> enum (.loading | .loaded(T) | .failed(Error)) used internally for connections, groups, and tags. A computed loadStatus: LoadStatus exposes the combined state. Views still read flat [Connection] / [Group] / [Tag] arrays as before; the load state is encoded in the storage itself.
  2. Silent save() plus blind integrity flip. The old callbacks set persistenceIntegrity = .ok after a save() that swallowed every error through try?. ConnectionPersistence, GroupPersistence, and TagPersistence now have save() throws. The single persist(connections:) / persist(groups:) / persist(tags:) bridge inside AppState is the only place that writes; it transitions the Loadable state and logs save failures via OSLog. State and disk no longer drift silently.
  3. Sync coordinator decided when to overwrite local files. The defense added in fix(ios): stop silent connection loss after TestFlight update #1260 (if !remoteChanges.changedConnections.isEmpty || ... { onConnectionsChanged?(merged) }) put the "is this worth saving?" check in the wrong layer. Moved to the receiver: each callback in AppState now diffs merged != self.connections before calling persist. Coordinator fires callbacks unconditionally; the persistence layer decides whether to commit.

Other small wins

  • getCurrentState returns optional; sync skips when loadStatus != .ready.
  • Initial state is .loading, not a flag that claims .ok before any load attempts.
  • Removed unused os imports and dead loggers from GroupPersistence and TagPersistence after their save lost the swallowed-error path.

No new strings, no new public types, no behavior change visible from any view.

Test plan

  • Cold launch — connections list loads as before, sync runs once on .active.
  • Tap Sync Now in Settings, then tap it again immediately — second tap is a no-op (no disk write, no Widget reload).
  • Refresh from iCloud — full pull merges, list updates, file on disk matches.
  • Add a new connection — appears in the list, file written, sync marks it dirty.
  • Delete a connection — removed from the list, file written, Keychain entries cleaned, tombstone scheduled.
  • Background refresh while device is locked — runBackgroundSync logs "Background sync skipped: persistence load not ready" and bails before touching anything.
  • Simulate corrupt connections.json — load logs the error, sidebar shows the empty state, Refresh from iCloud recovers the data and load state transitions to .ready.
  • swiftlint lint --strict passes (0 violations).

@datlechin datlechin merged commit 1d56437 into main May 14, 2026
2 of 3 checks passed
@datlechin datlechin deleted the refactor/ios-loadable-persistence-state branch May 14, 2026 01:13
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.

1 participant