feat: add optional iCloud Keychain sync for connection passwords#364
Merged
feat: add optional iCloud Keychain sync for connection passwords#364
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an opt-in iCloud Keychain password sync: new SyncSettings flag and UI toggle, AppDelegate startup handling to persist expected state and trigger migration, KeychainHelper support for kSecAttrSynchronizable with a migratePasswordSyncState routine, and updated docs in English and Vietnamese. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as SyncSettingsView
participant App as AppDelegate
participant UD as UserDefaults
participant KHelper as KeychainHelper
participant Keychain as Keychain
App->>KHelper: loadSync() / compute passwordSyncExpected
App->>UD: persist passwordSyncEnabled flag
User->>UI: toggle Passwords
UI->>UD: persist syncPasswords state
UI->>KHelper: onPasswordSyncChanged(enabled)
KHelper->>Keychain: query items (kSecAttrSynchronizableAny)
loop per item
KHelper->>Keychain: add item with kSecAttrSynchronizable={enabled}
KHelper->>Keychain: delete old variant if needed
end
KHelper->>UI: return migration summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/AppDelegate.swift`:
- Around line 56-58: The password-sync UserDefaults flag is set too late and
must be reconciled before legacy migrations run: call
AppSettingsStorage.shared.loadSync() and compute passwordSyncExpected, then set
UserDefaults.standard.set(passwordSyncExpected, forKey:
"com.TablePro.keychainPasswordSyncEnabled") prior to invoking
migrateFromLegacyKeychainIfNeeded(); this ensures KeychainHelper.save (which
reads that key) will choose the correct local vs synchronizable storage during
migration.
In `@TablePro/Core/Storage/KeychainHelper.swift`:
- Line 69: The current keychain queries use kSecAttrSynchronizableAny (notably
in load(key:)) which breaks per-Mac opt-in; update load(key:) and related
methods (save(key:value:), remove(key:), and migrateOptOut) to never query with
kSecAttrSynchronizableAny: first query explicitly for non-synchronizable items
(kSecAttrSynchronizable = kSecAttrSynchronizableFalse) to read local-only
entries, and only if the current Mac is opted-in attempt a separate query for
synchronizable items (kSecAttrSynchronizable = kSecAttrSynchronizableTrue). For
the opt-out migration (migrateOptOut), do not delete the shared synchronizable
item; instead create or update a local non-synchronizable override (or record a
per-device ignore flag) so opted-out Macs ignore synced entries without removing
them for other devices.
In `@TablePro/Views/Settings/SyncSettingsView.swift`:
- Around line 106-120: The current code calls onPasswordSyncChanged(newValue)
and saves only the raw syncSettings.syncPasswords flag, causing the keychain
sync flag to remain true if the master syncConnections switch is turned off;
update both the Passwords toggle handler and the master Connections toggle
handler to compute the effective password-sync value as (enabled &&
syncSettings.syncConnections && syncSettings.syncPasswords) and pass that
derived Bool into onPasswordSyncChanged(_:) after persistSettings(), ensuring
KeychainHelper.save's com.TablePro.keychainPasswordSyncEnabled reflects the
combined state; adjust the Connections Toggle onChange block (where
syncSettings.syncConnections is toggled) to also recalculate and call
onPasswordSyncChanged(false or derived) when turning off the master switch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9c49324-2f34-416b-946a-a48bf27c54cd
📒 Files selected for processing (8)
CHANGELOG.mdTablePro/AppDelegate.swiftTablePro/Core/Storage/KeychainHelper.swiftTablePro/Models/Settings/SyncSettings.swiftTablePro/Resources/Localizable.xcstringsTablePro/Views/Settings/SyncSettingsView.swiftdocs/features/icloud-sync.mdxdocs/vi/features/icloud-sync.mdx
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
1 participant
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
KeychainHelperconditionally setskSecAttrSynchronizableon save, useskSecAttrSynchronizableAnyon load/delete for seamless access regardless of sync stateTest plan
com.TablePro>KeychainHelper)Summary by CodeRabbit
New Features
Documentation