mirror of
https://github.com/yattee/yattee.git
synced 2026-10-23 13:12:53 +00:00
Close account lifecycle gaps found in review
A synced context change had no effect on the pre-revamp `subscriptionAccount` setting several views still read, nor on the feed cache, which is global rather than account-keyed — so a selection made on another device left this one showing the previous account's subscriptions and videos. Both now follow every context change, whoever made it. Instances replaced wholesale by iCloud bypassed `saveInstances()` and so posted no change notification. A legacy authenticated instance arriving that way never reached account migration: no account row, and no action to adopt its saved login, until an unrelated edit or a relaunch. Deleting an instance now deletes the accounts on it, while the instance is still resolvable — afterwards the account's credential manager cannot be determined from its type and the credential would be stranded. Only an explicit deletion cascades; an instance vanishing from iCloud leaves its accounts orphaned, because absence is not a deletion. A context whose instance has been switched off is temporarily unavailable rather than valid. The account stays signed in, so nothing about the account said the server had been turned off, and requests kept going to it while the switcher already hid it.
This commit is contained in:
@@ -68,6 +68,7 @@ final class AppEnvironment {
|
||||
private(set) var activeControlsCenterSettings: CenterSectionSettings = .default
|
||||
|
||||
@ObservationIgnored private var controlsSettingsObservers: [NSObjectProtocol] = []
|
||||
@ObservationIgnored private var contextMirrorObserver: NSObjectProtocol?
|
||||
|
||||
// MARK: - Shared Instance
|
||||
|
||||
@@ -379,6 +380,19 @@ final class AppEnvironment {
|
||||
subscriptionService.setContextValidator(dataContextValidator)
|
||||
subscriptionAccountValidator.setAccountsManager(accounts)
|
||||
|
||||
// The context can now change without anyone touching this device — a
|
||||
// choice made on another one, or an account deleted there. The legacy
|
||||
// setting several views still read has to follow it, and the feed cache
|
||||
// is global rather than account-keyed, so it would otherwise keep
|
||||
// serving the previous account's videos.
|
||||
contextMirrorObserver = NotificationCenter.default.addObserver(
|
||||
forName: .dataContextDidChange,
|
||||
object: nil,
|
||||
queue: .main
|
||||
) { [weak self] _ in
|
||||
MainActor.assumeIsolated { self?.mirrorContextToLegacySelection() }
|
||||
}
|
||||
|
||||
// A deletion that arrives from another device takes the credential with
|
||||
// it. Only a deletion does this — an account merely missing from a
|
||||
// fetch keeps everything, because a fetch is not an enumeration.
|
||||
@@ -503,6 +517,66 @@ final class AppEnvironment {
|
||||
|
||||
// MARK: - Accounts
|
||||
|
||||
/// Keeps the pre-revamp `subscriptionAccount` selection, and the feed cache
|
||||
/// keyed to it, in step with the current data context.
|
||||
///
|
||||
/// The two stores are written together everywhere the user changes the
|
||||
/// context by hand; this covers the changes nobody on this device made — a
|
||||
/// selection synced from another device, or the selected account being
|
||||
/// deleted there. Phase 3's switcher removes the last readers of the legacy
|
||||
/// setting and this mirror with them.
|
||||
private func mirrorContextToLegacySelection() {
|
||||
let legacy: SubscriptionAccount
|
||||
|
||||
switch accountsManager.currentContext {
|
||||
case .local:
|
||||
legacy = .local
|
||||
|
||||
case let .account(id):
|
||||
// An account whose instance has not arrived yet cannot be mapped
|
||||
// onto the legacy shape. Leaving the old value alone is right: the
|
||||
// context is preserved, and the validator reports it unavailable
|
||||
// rather than pointing the UI at something else.
|
||||
guard let account = accountsManager.account(id: id),
|
||||
let instance = accountsManager.instance(for: account) else { return }
|
||||
|
||||
switch instance.type {
|
||||
case .invidious: legacy = .invidious(instance.id)
|
||||
case .piped: legacy = .piped(instance.id)
|
||||
default: return
|
||||
}
|
||||
}
|
||||
|
||||
guard settingsManager.subscriptionAccount != legacy else { return }
|
||||
|
||||
settingsManager.subscriptionAccount = legacy
|
||||
SubscriptionFeedCache.shared.handleAccountChange()
|
||||
|
||||
LoggingService.shared.info(
|
||||
"Data context changed — cleared the feed cache and updated the legacy selection",
|
||||
category: .general
|
||||
)
|
||||
}
|
||||
|
||||
/// Deletes an instance together with the accounts that live on it.
|
||||
///
|
||||
/// The cascade has to happen here, while the instance is still resolvable:
|
||||
/// once it is gone, an account's credential manager cannot be determined
|
||||
/// from its type, and the credential would be stranded with no way to reach
|
||||
/// it. Only an explicit deletion cascades — an instance disappearing from
|
||||
/// iCloud leaves its accounts orphaned rather than deleted, because absence
|
||||
/// is not a deletion.
|
||||
func removeInstance(_ instance: Instance) {
|
||||
for account in accountsManager.accounts(forInstance: instance.id) {
|
||||
let credentials = accountCredentialsManager(for: account)
|
||||
credentials?.deleteCredential(for: account)
|
||||
credentials?.deleteLegacyCredentialIfOwned(for: account)
|
||||
accountsManager.removeAccount(account.id)
|
||||
}
|
||||
|
||||
instancesManager.remove(instance)
|
||||
}
|
||||
|
||||
/// The account-keyed credentials manager for an account's instance.
|
||||
/// `nil` when the instance is unknown or its type has no account concept.
|
||||
func accountCredentialsManager(for account: Account) -> (any AccountCredentialsManager)? {
|
||||
|
||||
@@ -95,6 +95,12 @@ final class InstancesManager {
|
||||
|
||||
// Update sync time
|
||||
settingsManager?.updateLastSyncTime()
|
||||
|
||||
// Instances arriving from iCloud are as much a change as a local edit.
|
||||
// Without this, a legacy authenticated instance delivered here never
|
||||
// reaches account migration — the account, and the action to adopt its
|
||||
// saved login, would appear only after an unrelated edit or a relaunch.
|
||||
NotificationCenter.default.post(name: .instancesDidChange, object: nil)
|
||||
}
|
||||
|
||||
/// Syncs local data to iCloud (called when enabling iCloud sync).
|
||||
@@ -126,6 +132,8 @@ final class InstancesManager {
|
||||
instances = iCloudInstances
|
||||
localDefaults.set(iCloudData, forKey: instancesKey)
|
||||
settingsManager?.updateLastSyncTime()
|
||||
|
||||
NotificationCenter.default.post(name: .instancesDidChange, object: nil)
|
||||
}
|
||||
|
||||
// MARK: - Public Methods
|
||||
|
||||
@@ -11,6 +11,10 @@ import Foundation
|
||||
enum DataContextUnavailability: Hashable, Sendable {
|
||||
/// The account exists but its instance hasn't arrived from iCloud yet.
|
||||
case instancePending
|
||||
/// The account's instance has been switched off. The selection is kept —
|
||||
/// the user can turn the instance back on — but nothing may be served from
|
||||
/// or written to a server they have disabled.
|
||||
case instanceDisabled
|
||||
/// No credential on this device yet. On tvOS this never resolves on its own
|
||||
/// — app Keychain items are not synced there — so the UI offers a local
|
||||
/// sign-in rather than implying a wait.
|
||||
@@ -87,10 +91,18 @@ final class DataContextValidator {
|
||||
return .temporarilyUnavailable(.instancePending)
|
||||
}
|
||||
|
||||
guard accountsManager.instance(for: account) != nil else {
|
||||
guard let instance = accountsManager.instance(for: account) else {
|
||||
return .temporarilyUnavailable(.instancePending)
|
||||
}
|
||||
|
||||
// The account's own status says nothing about the instance being
|
||||
// switched off, and `availableContexts` already hides disabled
|
||||
// instances — a context that stayed valid here would keep sending
|
||||
// requests to a server the user has turned off.
|
||||
guard instance.isEnabled else {
|
||||
return .temporarilyUnavailable(.instanceDisabled)
|
||||
}
|
||||
|
||||
switch accountsManager.status(for: id) {
|
||||
case .signedIn: return .valid
|
||||
case .credentialMissing: return .temporarilyUnavailable(.credentialMissing)
|
||||
|
||||
@@ -165,7 +165,7 @@ struct MediaSourcesView: View {
|
||||
|
||||
private func confirmDelete() {
|
||||
if let instance = pendingDeleteInstance {
|
||||
instancesManager?.remove(instance)
|
||||
appEnvironment?.removeInstance(instance)
|
||||
pendingDeleteInstance = nil
|
||||
}
|
||||
if let source = pendingDeleteSource {
|
||||
|
||||
@@ -308,7 +308,7 @@ private struct EditRemoteServerContent: View {
|
||||
titleVisibility: .visible
|
||||
) {
|
||||
Button(String(localized: "common.delete"), role: .destructive) {
|
||||
appEnvironment?.instancesManager.remove(instance)
|
||||
appEnvironment?.removeInstance(instance)
|
||||
dismiss()
|
||||
}
|
||||
Button(String(localized: "common.cancel"), role: .cancel) {}
|
||||
|
||||
@@ -202,6 +202,54 @@ struct AccountsManagerTests {
|
||||
}
|
||||
}
|
||||
|
||||
@Suite("DataContext validation", .serialized)
|
||||
@MainActor
|
||||
struct DataContextValidatorTests {
|
||||
private func makeValidator(
|
||||
instances: InstancesManager
|
||||
) -> (AccountsManager, DataContextValidator) {
|
||||
let accounts = AccountsManager(
|
||||
identityScope: CredentialIdentityScope(fixedScope: "identity-A"),
|
||||
instancesManager: instances,
|
||||
defaults: UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")!
|
||||
)
|
||||
let validator = DataContextValidator(
|
||||
accountsManager: accounts,
|
||||
instancesManager: instances,
|
||||
toastManager: ToastManager()
|
||||
)
|
||||
return (accounts, validator)
|
||||
}
|
||||
|
||||
@Test("A switched-off instance makes its context unavailable, not removed")
|
||||
func disabledInstanceIsUnavailable() {
|
||||
let instances = InstancesManager()
|
||||
var instance = Instance(type: .invidious, url: URL(string: "https://inv.example.com")!)
|
||||
instances.add(instance)
|
||||
|
||||
let (accounts, validator) = makeValidator(instances: instances)
|
||||
let account = accounts.addAccount(instanceID: instance.id)
|
||||
accounts.setStatus(.signedIn, for: account.id)
|
||||
accounts.setContext(.account(account.id))
|
||||
|
||||
#expect(validator.validity == .valid)
|
||||
|
||||
// Disabling the instance leaves the account signed in, so nothing about
|
||||
// the account itself says the server may no longer be used.
|
||||
instance.isEnabled = false
|
||||
instances.update(instance)
|
||||
|
||||
#expect(validator.validity == .temporarilyUnavailable(.instanceDisabled))
|
||||
// The user's selection is kept — turning the instance back on restores
|
||||
// it without them having to choose again.
|
||||
#expect(accounts.currentContext == .account(account.id))
|
||||
#expect(validator.effectiveContext == .local)
|
||||
#expect(!validator.allowsServerMutations)
|
||||
|
||||
instances.remove(instance)
|
||||
}
|
||||
}
|
||||
|
||||
@Suite("Account capabilities")
|
||||
@MainActor
|
||||
struct AccountCapabilitiesTests {
|
||||
|
||||
Reference in New Issue
Block a user