feat: add iCloud sync with Pro license gating#321
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full iCloud/CloudKit synchronization feature: CloudKit engine, sync coordinator, metadata & change tracking, record mapping, conflict resolver, UI (settings, status, conflict dialog, Pro gate), entitlements, storage hooks, localization keys, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Lifecycle
participant SyncCoord as SyncCoordinator
participant License as LicenseManager
participant CKEngine as CloudKitSyncEngine
participant Local as Local Storage
participant Tracker as SyncChangeTracker
participant Resolver as ConflictResolver
App->>SyncCoord: start()
SyncCoord->>License: checkFeature(iCloudSync)
SyncCoord->>CKEngine: checkAccountStatus()
CKEngine-->>SyncCoord: account status
SyncCoord->>Tracker: dirtyRecords(for types)
Tracker-->>SyncCoord: dirty IDs & tombstones
SyncCoord->>Local: fetch local records for dirty IDs
Local-->>SyncCoord: records
SyncCoord->>CKEngine: push(records, deletions)
CKEngine-->>SyncCoord: push result (conflicts)
alt Conflicts
SyncCoord->>Resolver: addConflict(...)
Resolver-->>SyncCoord: await resolution
end
SyncCoord->>CKEngine: pull(since: token)
CKEngine-->>SyncCoord: changedRecords, deletions, newToken
SyncCoord->>Tracker: suppress()
SyncCoord->>Local: applyRemoteChanges(...)
SyncCoord->>Tracker: restore()
SyncCoord->>SyncCoord: update token & lastSyncDate
sequenceDiagram
participant User as User
participant UI as ConflictResolutionView
participant Resolver as ConflictResolver
participant SyncCoord as SyncCoordinator
participant CKEngine as CloudKitSyncEngine
SyncCoord->>Resolver: addConflict(conflict)
Resolver-->>UI: publish currentConflict
User->>UI: choose "Keep This Mac's Version"
UI->>Resolver: resolveCurrentConflict(keepLocal:true)
Resolver-->>SyncCoord: resolved record to re-push
SyncCoord->>CKEngine: push(resolvedRecord)
CKEngine-->>SyncCoord: push success
UI->>UI: dismiss when queue empty
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 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
TablePro/Core/Storage/AppSettingsManager.swift (1)
26-117:⚠️ Potential issue | 🟡 MinorNew dirty-marking hooks now amplify non-OSLog logging path.
These new
markDirtycalls run on every settings write, andSyncChangeTracker.markDirty(_ type:id:)currently emits viaTablePro/Core/Sync/SyncChangeTracker.swiftLine 28-33). Please switch that path toLoggerto avoid stdout noise and keep logging policy consistent.As per coding guidelines: "Use OSLog for logging, never print(). Import with: import os and initialize as: private static let logger = Logger(subsystem: "com.TablePro", category: "ComponentName")"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Storage/AppSettingsManager.swift` around lines 26 - 117, The SyncChangeTracker.markDirty(_:, id:) path currently uses print() and should use OSLog via Logger instead; in SyncChangeTracker.swift import os, add a private static logger (e.g. private static let logger = Logger(subsystem: "com.TablePro", category: "SyncChangeTracker")) and replace all print(...) calls inside markDirty (and any other logging in that type) with logger.debug or logger.info messages that include the same payload (type and id) so the new dirty-marking hooks in AppSettingsManager use the consistent logging channel.docs/zh/development/building.mdx (1)
207-210:⚠️ Potential issue | 🟡 MinorTroubleshooting still references the old library-prep command.
This section says to run
scripts/build-release.shfor missing libraries, but earlier updates switched acquisition toscripts/download-libs.sh. Please align the fix steps to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/zh/development/building.mdx` around lines 207 - 210, Update the troubleshooting steps to reference the current library acquisition flow: replace the instruction to run scripts/build-release.sh with running scripts/download-libs.sh, keep the checks for the Libs/ directory and verifying the target-arch libmariadb.a exist, and ensure the text mentions scripts/download-libs.sh as the canonical fix step instead of scripts/build-release.sh so readers are not directed to the obsolete command.docs/vi/development/building.mdx (1)
207-210:⚠️ Potential issue | 🟡 MinorTroubleshooting command needs to match the new download flow.
For missing-library errors, this section should direct users to
scripts/download-libs.sh(notscripts/build-release.sh) to match the updated setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/vi/development/building.mdx` around lines 207 - 210, Update the troubleshooting step so it matches the new download flow: instruct users to check the Libs/ directory and run scripts/download-libs.sh (instead of scripts/build-release.sh) to obtain the prebuilt libraries, then verify that libmariadb.a exists for the target architecture; replace the reference to scripts/build-release.sh with scripts/download-libs.sh and keep the Libs/ and libmariadb.a verification steps.
🧹 Nitpick comments (29)
TablePro/Core/Storage/AppSettingsManager.swift (1)
26-117: Consider replacing repeated string literals with typed sync IDs.
"general","appearance","editor", etc. are duplicated across setters. Centralizing these IDs (enum/static constants) reduces typo and schema-drift risk as sync mapping evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Storage/AppSettingsManager.swift` around lines 26 - 117, Replace the repeated literal sync IDs used in calls like SyncChangeTracker.shared.markDirty(.settings, id: "general") across the property setters (appearance, editor, dataGrid, history, tabs, keyboard, ai, etc.) with a single typed identifier (e.g., a SyncSettingID enum or static constants) and use that type in markDirty calls to avoid string duplication and typos; add the new enum/constant (e.g., case general, appearance, editor, dataGrid, history, tabs, keyboard, ai) and update each call to pass the typed value (or its rawValue if markDirty keeps a String signature) so all uses reference the centralized identifier.TablePro/Views/Components/ProFeatureGate.swift (3)
44-56: Use String(localized:) for user-facing strings in computed properties.These strings are inside a computed property (
proRequiredOverlay), not SwiftUI view literals, so they won't auto-localize.🌐 Proposed fix for localization
case .expired: - Text("Your license has expired") + Text(String(localized: "Your license has expired")) .font(.headline) Text(feature.featureDescription) .font(.subheadline) .foregroundStyle(.secondary) Button(String(localized: "Renew License...")) { openLicenseSettings() } .buttonStyle(.borderedProminent) case .unlicensed: - Text("\(feature.displayName) requires a Pro license") + Text(String(localized: "\(feature.displayName) requires a Pro license")) .font(.headline)As per coding guidelines: "Use String(localized:) for new user-facing strings in computed properties."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/ProFeatureGate.swift` around lines 44 - 56, The strings inside the computed property proRequiredOverlay are plain literals and won't auto-localize; update any user-facing literals in that property (e.g., the headline "Your license has expired", the button label "Renew License...", and the message "\(feature.displayName) requires a Pro license") to use String(localized:) so they are localized at runtime, keeping the same calls to feature.featureDescription and the existing openLicenseSettings action and Text/Button usage.
11-14: Missing explicit access control on struct.Per coding guidelines, types should have explicit access control.
♻️ Proposed fix
-struct ProFeatureGateModifier: ViewModifier { +internal struct ProFeatureGateModifier: ViewModifier { let feature: ProFeatureAs per coding guidelines: "Always use explicit access control on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/ProFeatureGate.swift` around lines 11 - 14, The struct ProFeatureGateModifier lacks an explicit access level; update its declaration to include an explicit access control modifier (e.g., internal or public) — e.g., change “struct ProFeatureGateModifier” to “internal struct ProFeatureGateModifier” (or public if it must be exposed), and ensure related types/extensions used here (ProFeature and LicenseManager.shared usage) follow the same explicit-access rule per guidelines.
78-83: Missing explicit access control on extension.♻️ Proposed fix
-extension View { +internal extension View { /// Gate this view behind a Pro license requirement func requiresPro(_ feature: ProFeature) -> some View {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/ProFeatureGate.swift` around lines 78 - 83, The extension on View and its helper need explicit access control: declare the extension and the requiresPro(_:) method with an explicit access level (e.g., public or internal) consistent with your module API, e.g., "public extension View" (or "internal extension View") and "public func requiresPro(...)" if you intend it to be public; also ensure ProFeatureGateModifier has at least the same access level so the modifier can be referenced from the extension.TablePro/Core/Sync/SyncRecordMapper.swift (2)
21-26: Missing explicit access control on struct.♻️ Proposed fix
/// Pure-function mapper between local models and CKRecord -struct SyncRecordMapper { +internal struct SyncRecordMapper { private static let logger = Logger(subsystem: "com.TablePro", category: "SyncRecordMapper")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncRecordMapper.swift` around lines 21 - 26, The struct SyncRecordMapper currently has no explicit access level; add an explicit access modifier (e.g., public or internal as appropriate for its usage) to the struct declaration and adjust any contained static properties (logger, encoder, decoder) to match or be more restrictive if needed so access levels are consistent (update "struct SyncRecordMapper" to "public struct SyncRecordMapper" or "internal struct SyncRecordMapper" and annotate the static lets likewise if required).
13-19: Missing explicit access control on enum.♻️ Proposed fix
/// CloudKit record types for sync -enum SyncRecordType: String, CaseIterable { +internal enum SyncRecordType: String, CaseIterable { case connection = "Connection"As per coding guidelines: "Always use explicit access control on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncRecordMapper.swift` around lines 13 - 19, The enum SyncRecordType is missing an explicit access control modifier; update the declaration of SyncRecordType (enum SyncRecordType: String, CaseIterable) to include the appropriate access level (for example internal or public as required by its usage) so the type has explicit access control per guidelines; choose public if it's used across modules or internal if only within the module, and keep the existing cases and protocols unchanged.TablePro/Core/Sync/SyncChangeTracker.swift (2)
16-18: Missing explicit access control on class.♻️ Proposed fix
/// Tracks dirty entities and deletions for sync -final class SyncChangeTracker { +internal final class SyncChangeTracker { static let shared = SyncChangeTracker()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncChangeTracker.swift` around lines 16 - 18, The SyncChangeTracker class lacks an explicit access control modifier; add the appropriate access level (e.g., public or internal) to the final class declaration (SyncChangeTracker) to make the intent explicit, and ensure the access of its singletons/properties (shared and logger) is compatible with that chosen level (adjust static let shared and private static let logger visibility if needed).
11-13: Missing explicit access control on extension.♻️ Proposed fix
-extension Notification.Name { +internal extension Notification.Name { static let syncChangeTracked = Notification.Name("com.TablePro.syncChangeTracked") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncChangeTracker.swift` around lines 11 - 13, The Notification.Name extension and its member syncChangeTracked lack explicit access control; update the declaration to include an appropriate modifier (e.g., public or internal) on the extension or on the static let itself so the visibility is explicit — locate the extension Notification.Name and add the chosen access level to either the extension or static let syncChangeTracked to match the module's intended API exposure.TablePro/Core/Services/Licensing/LicenseManager+Pro.swift (1)
10-14: Missing explicit access control on extension.Per coding guidelines, always specify access modifiers on the extension itself. Additionally, the
featureparameter is currently unused - if this is intentional for future feature-specific gating, consider adding a brief comment.♻️ Proposed fix
-extension LicenseManager { +internal extension LicenseManager { /// Check if a Pro feature is available (convenience for boolean checks) func isFeatureAvailable(_ feature: ProFeature) -> Bool { + // Currently all Pro features share the same license gate status.isValid }As per coding guidelines: "Always use explicit access control on extensions and types (private, internal, public). Specify access modifiers on extension itself, not individual members."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Licensing/LicenseManager`+Pro.swift around lines 10 - 14, Add an explicit access modifier to the extension declaration (e.g., make "extension LicenseManager" into "internal extension LicenseManager" or "public extension LicenseManager" per module API) and keep member access levels inferred, and address the unused parameter in func isFeatureAvailable(_ feature: ProFeature) by either using the parameter for future gating or adding a brief inline comment such as "// feature currently unused; retained for future feature-specific checks" (or explicitly ignore it with _ = feature) so the intent is clear; reference: extension LicenseManager, func isFeatureAvailable(_ feature: ProFeature), status.isValid.TablePro/Core/Sync/SyncMetadataStorage.swift (1)
13-14: Missing explicit access control on class.♻️ Proposed fix
/// Persistent storage for sync metadata using UserDefaults -final class SyncMetadataStorage { +internal final class SyncMetadataStorage { static let shared = SyncMetadataStorage()As per coding guidelines: "Always use explicit access control on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncMetadataStorage.swift` around lines 13 - 14, The class declaration lacks explicit access control; update the declaration of SyncMetadataStorage (the final class with static let shared) to include an explicit access modifier (e.g., change "final class SyncMetadataStorage" to "internal final class SyncMetadataStorage" or "public final class SyncMetadataStorage" as appropriate for its intended visibility) to satisfy the coding guideline requiring explicit access control on types.TablePro/Core/Storage/TagStorage.swift (1)
50-50: Consider marking only the changed tag(s) as dirty.
saveTagsmarks every tag as dirty on every save, even unchanged ones. This triggers redundant sync operations. For single-item changes likeaddTagordeleteTag, consider tracking only the affected ID.💡 Suggestion: Pass changed IDs explicitly
- func saveTags(_ tags: [ConnectionTag]) { + func saveTags(_ tags: [ConnectionTag], changedIds: [String]? = nil) { do { let data = try encoder.encode(tags) defaults.set(data, forKey: tagsKey) - SyncChangeTracker.shared.markDirty(.tag, ids: tags.map { $0.id.uuidString }) + let idsToMark = changedIds ?? tags.map { $0.id.uuidString } + if !idsToMark.isEmpty { + SyncChangeTracker.shared.markDirty(.tag, ids: idsToMark) + } } catch { Self.logger.error("Failed to save tags: \(error)") } }Then call sites like
addTagcan passchangedIds: [tag.id.uuidString].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Storage/TagStorage.swift` at line 50, Modify saveTags to accept an optional parameter (e.g. changedIds: [String]?) and only call SyncChangeTracker.shared.markDirty(.tag, ids: changedIds) when changedIds is provided; if nil, fall back to marking all tag IDs as before (tags.map { $0.id.uuidString }). Update call sites such as addTag and deleteTag to pass changedIds: [newTag.id.uuidString] or [deletedTag.id.uuidString] respectively so only the affected tag(s) are marked dirty, leaving existing behavior unchanged when callers don’t supply changedIds.TablePro/Core/Storage/GroupStorage.swift (1)
42-42: Same inefficiency asTagStorage.saveTags— marks all groups dirty on every save.Consider the same approach: pass changed IDs explicitly to avoid redundant sync traffic when only a single group is added, updated, or deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Storage/GroupStorage.swift` at line 42, The current call SyncChangeTracker.shared.markDirty(.group, ids: groups.map { $0.id.uuidString }) marks every group dirty every time; change GroupStorage to compute and pass only the actually changed IDs (e.g., in the save/add/update/delete flows) instead of mapping all groups — replicate the approach used in TagStorage.saveTags: determine which group IDs were added/updated/removed (use the local parameter(s) or diff between existing and incoming groups) and call SyncChangeTracker.shared.markDirty(.group, ids: changedIds) with that set.TablePro/Resources/Localizable.xcstrings (1)
8891-8893: Consolidate overlapping iCloud sign-in messages to avoid localization drift.You now have multiple near-duplicate keys for the same state (
iCloud account is not available...,Sign in to iCloud in System Settings...,Sign in to iCloud...). Consider reusing one canonical string/key to reduce translator overhead and inconsistent copy.Also applies to: 16284-16289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Resources/Localizable.xcstrings` around lines 8891 - 8893, There are multiple near-duplicate localization keys for the same iCloud sign-in message (for example the key "iCloud account is not available. Sign in to iCloud in System Settings."); pick one canonical key/string (e.g., that full sentence) and replace the other duplicate keys ("Sign in to iCloud in System Settings...", "Sign in to iCloud...") with references to that canonical key or remove the duplicates and reuse the canonical key in code; update the translations block for the canonical key and remove the other entries (also apply the same consolidation to the duplicates reported around the other range) so translators only maintain one source of truth.TablePro/Views/Components/SyncStatusIndicator.swift (2)
10-10: Declare explicit access forSyncStatusIndicator.Please add an explicit visibility modifier on this new type.
As per coding guidelines: "Always use explicit access control on extensions and types (private, internal, public)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/SyncStatusIndicator.swift` at line 10, Declare an explicit access level for the new Swift type by adding a visibility modifier to the struct declaration for SyncStatusIndicator (e.g., make it internal or public per project conventions) so the type is not using implicit default access; update the declaration of SyncStatusIndicator: View to include the chosen access modifier and ensure any related extensions follow the same explicit access rule.
113-117: Avoid fixed-delay tab switching; make it deterministic.Using
DispatchQueue.main.asyncAfter(... + 0.1)couples behavior to timing. Set the tab key directly and open settings immediately.💡 Suggested fix
private func openSyncSettings() { + UserDefaults.standard.set(SettingsTab.sync.rawValue, forKey: "selectedSettingsTab") NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { - UserDefaults.standard.set(SettingsTab.sync.rawValue, forKey: "selectedSettingsTab") - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/SyncStatusIndicator.swift` around lines 113 - 117, In openSyncSettings, avoid the fixed DispatchQueue delay; set UserDefaults.standard.set(SettingsTab.sync.rawValue, forKey: "selectedSettingsTab") before opening the settings window and then call NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) immediately so the tab key is deterministic; alternatively if opening must complete first, observe the settings window appearance (e.g., via NotificationCenter or delegate) and set "selectedSettingsTab" in that callback instead of using asyncAfter.TablePro/Core/Sync/SyncStatus.swift (1)
11-37: Add explicit access control to new enums.
SyncStatusandDisableReasonshould declare visibility explicitly to match repository standards.💡 Suggested fix
-enum SyncStatus: Equatable { +internal enum SyncStatus: Equatable { @@ -enum DisableReason: Equatable { +internal enum DisableReason: Equatable {As per coding guidelines: "Always use explicit access control on extensions and types (private, internal, public)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncStatus.swift` around lines 11 - 37, Declare explicit access control on the new enums and their members: add an access modifier (e.g., internal or public per repo policy) before the enum declarations for SyncStatus and DisableReason, and also add explicit access levels to the computed properties isSyncing and isEnabled on SyncStatus so the types and their APIs have explicit visibility.TablePro/Core/Sync/ConflictResolver.swift (1)
14-43: Add explicit access control on newly introduced types.Please declare visibility explicitly for
SyncConflictandConflictResolver.As per coding guidelines: "Always use explicit access control on extensions and types (private, internal, public)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/ConflictResolver.swift` around lines 14 - 43, Declare explicit access control for the new types and their members: add an access modifier (e.g., internal or public) to the SyncConflict type and its initializer/properties and to the ConflictResolver class declaration (e.g., internal final class ConflictResolver) so the type visibility is explicit; ensure member properties and init for SyncConflict and the ConflictResolver class declaration use matching explicit access levels per project conventions.TablePro/Core/Sync/CloudKitSyncEngine.swift (1)
13-20: Add explicit access control to types.Both
PullResultandCloudKitSyncEngineshould have explicit access modifiers per coding guidelines.♻️ Proposed fix
-struct PullResult: Sendable { +internal struct PullResult: Sendable { let changedRecords: [CKRecord] let deletedRecordIDs: [CKRecord.ID] let newToken: CKServerChangeToken? } /// Actor that serializes all CloudKit I/O -actor CloudKitSyncEngine { +internal actor CloudKitSyncEngine {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/CloudKitSyncEngine.swift` around lines 13 - 20, The PullResult struct and CloudKitSyncEngine actor currently lack explicit access control; update their declarations to include the appropriate visibility (e.g., public, internal, or fileprivate) per project guidelines by adding the chosen access modifier to the definitions of PullResult and CloudKitSyncEngine so their visibility is explicit and consistent across the module.TablePro/Models/Settings/SyncSettings.swift (1)
11-27: Add explicit access control to types.Per coding guidelines, always use explicit access control on types. Both
SyncSettingsandHistorySyncLimitshould have explicit access modifiers.As per coding guidelines: "Always use explicit access control on extensions and types (private, internal, public). Specify access modifiers on extension itself, not individual members."
♻️ Proposed fix
-struct SyncSettings: Codable, Equatable { +internal struct SyncSettings: Codable, Equatable {And at line 30:
-enum HistorySyncLimit: String, Codable, CaseIterable { +internal enum HistorySyncLimit: String, Codable, CaseIterable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Models/Settings/SyncSettings.swift` around lines 11 - 27, The SyncSettings and HistorySyncLimit types lack explicit access control; update their declarations to include the appropriate access modifier (e.g., public or internal) on the type itself (not on individual members) per guidelines, and do the same for any related extensions so that both SyncSettings and HistorySyncLimit have explicit visibility specified.TablePro/Core/Sync/SyncError.swift (2)
48-63: Consider usingCKErrortype directly instead of hardcoded integer codes.The current implementation uses magic numbers for CKError codes. Casting the error to
CKErrordirectly would be more maintainable and self-documenting.♻️ Proposed refactor
static func from(_ error: Error) -> SyncError { if let syncError = error as? SyncError { return syncError } - // Map CKError codes to SyncError - let nsError = error as NSError - if nsError.domain == "CKErrorDomain" { - switch nsError.code { - case 3, 36: // networkUnavailable, networkFailure + if let ckError = error as? CKError { + switch ckError.code { + case .networkUnavailable, .networkFailure: return .networkUnavailable - case 9: // notAuthenticated + case .notAuthenticated: return .accountUnavailable - case 25: // quotaExceeded + case .quotaExceeded: return .quotaExceeded - case 26: // zoneNotFound + case .zoneNotFound: return .zoneNotFound default: - return .serverError(nsError.localizedDescription) + return .serverError(ckError.localizedDescription) } } return .unknown(error.localizedDescription) }Note: This requires
import CloudKitat the top of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncError.swift` around lines 48 - 63, Replace the NSError-based magic-number mapping with a direct CKError cast: add "import CloudKit", attempt to cast the incoming Error to CKError (e.g., guard let ckError = error as? CKError), then switch on ckError.code using CKError.Code cases (.networkUnavailable, .networkFailure → return .networkUnavailable; .notAuthenticated → .accountUnavailable; .quotaExceeded → .quotaExceeded; .zoneNotFound → .zoneNotFound) and use the default case to return .serverError(ckError.localizedDescription); update the mapping logic inside the existing SyncError mapping block (where you currently reference nsError) to use ckError and CKError.Code enums.
11-11: Add explicit access control.The enum should have an explicit access modifier per coding guidelines.
♻️ Proposed fix
-enum SyncError: LocalizedError, Equatable { +internal enum SyncError: LocalizedError, Equatable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncError.swift` at line 11, The SyncError enum is missing an explicit access modifier; update the declaration for enum SyncError: LocalizedError, Equatable to include the appropriate access level (for example public or internal per project guidelines) so its visibility is explicit, and ensure any external usages still compile after changing the access control.TablePro/Core/Sync/SyncCoordinator.swift (3)
371-378: Hardcoded zone name duplicates constant fromCloudKitSyncEngine.The zone name "TableProSync" is hardcoded here but also defined as a constant in
CloudKitSyncEngine. Consider exposing the zone ID from the engine or defining a shared constant to avoid drift.♻️ Suggested approach
Since
engine.zoneIDis already accessible (line 224 doesawait engine.zoneID), you could reuse it here instead of reconstructing:if changeTracker.dirtyRecords(for: .connection).contains(remoteConnection.id.uuidString) { + let zoneID = await engine.zoneID let localRecord = SyncRecordMapper.toCKRecord( connections[index], - in: CKRecordZone.ID( - zoneName: "TableProSync", - ownerName: CKCurrentUserDefaultName - ) + in: zoneID )However, this requires making
applyRemoteConnectionasync. Alternatively, cache the zoneID at class level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 371 - 378, The code hardcodes the zone name "TableProSync" when building a CKRecordZone.ID in the block that calls SyncRecordMapper.toCKRecord; instead reuse the existing CloudKit zone ID instead of reconstructing the literal. Modify applyRemoteConnection (or the caller) so you can access engine.zoneID (or cache engine.zoneID on the SyncCoordinator instance) and pass that CKRecordZone.ID into SyncRecordMapper.toCKRecord rather than creating a new CKRecordZone.ID with the hardcoded "TableProSync"; if you choose to make applyRemoteConnection async, update its signature and call sites accordingly so you can await engine.zoneID, otherwise add a cached property on SyncCoordinator populated once from await engine.zoneID and use that cached zoneID here.
15-15: Add explicit access control.The class should have an explicit access modifier per coding guidelines.
♻️ Proposed fix
`@MainActor` `@Observable` -final class SyncCoordinator { +internal final class SyncCoordinator {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` at line 15, Add an explicit access control modifier to the class declaration for SyncCoordinator (e.g., change "final class SyncCoordinator" to "internal final class SyncCoordinator" or "public final class SyncCoordinator" depending on its intended visibility) so the class conforms to the project's coding guidelines requiring explicit access levels; update any related tests or usages if you change it to public.
628-652: Incomplete implementation: dirty groups/tags not collected for push.
collectDirtyGroupsandcollectDirtyTagsonly collect tombstones for deletions but don't collect dirty records for upserts. The TODO comments indicate this is intentional for Phase C, but this creates an asymmetry: new/modified groups and tags won't sync until Phase C is complete.Consider adding a TODO comment at the push call site (line 251-252) to make this limitation more visible, or implement the dirty record collection now.
Would you like me to generate the implementation for collecting dirty groups and tags, similar to the connection collection logic at lines 228-239?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 628 - 652, collectDirtyGroups and collectDirtyTags only append tombstone deletions and omit dirty/upsert records; either add a clear TODO at the push invocation to document this limitation or implement the missing upsert collection now: in collectDirtyGroups and collectDirtyTags, in addition to iterating metadataStorage.tombstones(for: .group/.tag) and appending SyncRecordMapper.recordID(... ) to deletions, also iterate metadataStorage.dirty(for: .group/.tag) (or the equivalent API used by collectDirtyConnections) and append SyncRecordMapper.record(type: .group/.tag, id: <dirty id>, in: zoneID) to the records array so new/modified groups and tags are included in the push; keep the existing tombstone handling unchanged.TablePro/Views/Settings/SyncSettingsView.swift (2)
159-164: String-based selector and magic key could be fragile.The
Selector(("showSettingsWindow:"))syntax and hardcoded"selectedSettingsTab"key are fragile. Consider:
- Using
#selectorif possible (requires exposing the method)- Defining the UserDefaults key as a constant to match other settings keys in the codebase
♻️ Optional improvement
+ private static let selectedSettingsTabKey = "selectedSettingsTab" + private func openLicenseSettings() { NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { - UserDefaults.standard.set(SettingsTab.license.rawValue, forKey: "selectedSettingsTab") + UserDefaults.standard.set(SettingsTab.license.rawValue, forKey: Self.selectedSettingsTabKey) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/SyncSettingsView.swift` around lines 159 - 164, The openLicenseSettings method uses a fragile string selector and a magic UserDefaults key; replace Selector(("showSettingsWindow:")) with a compile-checked `#selector` reference by making the target method visible (e.g., expose showSettingsWindow(_:) on the AppDelegate or appropriate class so you can call `#selector`(AppDelegate.showSettingsWindow(_:))) and replace the hardcoded "selectedSettingsTab" key with a named constant (e.g., SettingsKeys.selectedSettingsTab or similar shared settings key) and use that constant when calling UserDefaults.standard.set(..., forKey:). Keep the existing DispatchQueue delay and SettingsTab.license usage but swap the selector and key to the `#selector` and constant.
10-10: Add explicit access control.The struct should have an explicit access modifier per coding guidelines.
♻️ Proposed fix
-struct SyncSettingsView: View { +internal struct SyncSettingsView: View {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/SyncSettingsView.swift` at line 10, Add an explicit access modifier to the SyncSettingsView declaration (e.g., change "struct SyncSettingsView: View" to "internal struct SyncSettingsView: View" or "public struct SyncSettingsView: View" per your module visibility rules) and apply matching explicit access to any nested types or members if your guidelines require it; ensure the chosen modifier aligns with how SyncSettingsView is used across the codebase.TablePro/Views/Components/ConflictResolutionView.swift (2)
110-141: Field labels should useString(localized:)for consistency.The field labels like "Host", "Port", "Database", "User", "Name", "Color", and "Query" are displayed in the user-facing conflict resolution dialog. While some could be considered technical terms, they appear alongside other localized UI text. Consider localizing them for consistency with the rest of the view.
The use of
NSStringfor the query preview length check on lines 135-138 is good practice for O(1) performance.♻️ Suggested localization
case .connection: if let host = record["host"] as? String { - fieldRow(label: "Host", value: host) + fieldRow(label: String(localized: "Host"), value: host) } if let port = record["port"] as? Int64 { - fieldRow(label: "Port", value: "\(port)") + fieldRow(label: String(localized: "Port"), value: "\(port)") } if let database = record["database"] as? String { - fieldRow(label: "Database", value: database) + fieldRow(label: String(localized: "Database"), value: database) } if let username = record["username"] as? String { - fieldRow(label: "User", value: username) + fieldRow(label: String(localized: "User"), value: username) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/ConflictResolutionView.swift` around lines 110 - 141, The field labels in ConflictResolutionView are hard-coded strings; update all fieldRow label arguments (e.g., the calls with "Host", "Port", "Database", "User", "Name", "Color", "Query") to use localized strings via String(localized:) so they follow the view's localization pattern (e.g., fieldRow(label: String(localized: "Host"), ...)); keep the existing logic around record[...] and the query preview (nsQuery length check) unchanged—only replace the literal label strings with String(localized:) calls.
170-183: Progress indicator text shows "1 of N" but should show the current index.The progress indicator always displays "1 of (total)" regardless of which conflict is being resolved. Since conflicts are resolved from the front of the queue, this is technically correct for the current conflict, but could be clearer by showing "Conflict 1 of N" or similar phrasing.
♻️ Optional: Clarify progress text
private var progressIndicator: some View { Group { let total = conflictResolver.pendingConflicts.count if total > 1 { Text( String( - localized: "1 of \(total) conflicts" + localized: "Conflict 1 of \(total)" ) ) .font(.caption) .foregroundStyle(.secondary) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/ConflictResolutionView.swift` around lines 170 - 183, The progress text always hardcodes "1 of N"; update the progressIndicator to show "Conflict X of N" where X is the current conflict index computed from the shrinking queue. Add a stored initial total (e.g. `@State` private var initialTotal = 0) and set it onAppear to conflictResolver.pendingConflicts.count, then compute currentIndex = initialTotal - conflictResolver.pendingConflicts.count + 1 (clamp to 1..initialTotal) and display String(localized: "Conflict \(currentIndex) of \(initialTotal)") in progressIndicator (referencing the progressIndicator property and conflictResolver.pendingConflicts). Ensure localization usage remains consistent.TablePro/Core/Storage/QueryHistoryStorage.swift (1)
578-597: Batch update silently ignores errors and lacks a return value.
markHistoryEntriesSynceddoesn't return success/failure status or log errors if the update fails. Unlike other methods in this class (e.g.,addHistory,deleteHistory), callers can't tell if the operation succeeded.♻️ Proposed fix: Add return value and error logging
- func markHistoryEntriesSynced(ids: [String]) async { + func markHistoryEntriesSynced(ids: [String]) async -> Bool { guard !ids.isEmpty else { return } - await performDatabaseWork { [weak self] in - guard let self else { return } + return await performDatabaseWork { [weak self] in + guard let self else { return false } let placeholders = ids.map { _ in "?" }.joined(separator: ", ") let sql = "UPDATE history SET is_synced = 1 WHERE id IN (\(placeholders));" var statement: OpaquePointer? - guard sqlite3_prepare_v2(self.db, sql, -1, &statement, nil) == SQLITE_OK else { return } + guard sqlite3_prepare_v2(self.db, sql, -1, &statement, nil) == SQLITE_OK else { + Self.logger.error("Failed to prepare markHistoryEntriesSynced statement") + return false + } defer { sqlite3_finalize(statement) } let SQLITE_TRANSIENT = unsafeBitCast(-1, to: sqlite3_destructor_type.self) for (index, id) in ids.enumerated() { sqlite3_bind_text(statement, Int32(index + 1), id, -1, SQLITE_TRANSIENT) } - sqlite3_step(statement) + let result = sqlite3_step(statement) + return result == SQLITE_DONE } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Storage/QueryHistoryStorage.swift` around lines 578 - 597, markHistoryEntriesSynced currently swallows SQLite errors and returns nothing; change its signature to return a success flag (e.g., async -> Bool or async throws -> Void) so callers can know outcome, and add error handling/logging around sqlite3_prepare_v2, sqlite3_bind_text, sqlite3_step using sqlite3_errmsg(self.db) and the class logger (e.g., processLogger or existing logger) to record failures; ensure sqlite3_finalize is still called in a defer and return false (or throw) on any sqlite error, returning true only when step reports SQLITE_DONE/OK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 228d294d-d5f0-4a68-aa37-1be6aa14a865
📒 Files selected for processing (35)
CHANGELOG.mdCLAUDE.mdTablePro.xcodeproj/project.pbxprojTablePro/AppDelegate.swiftTablePro/Core/Services/Licensing/LicenseManager+Pro.swiftTablePro/Core/Storage/AppSettingsManager.swiftTablePro/Core/Storage/AppSettingsStorage.swiftTablePro/Core/Storage/ConnectionStorage.swiftTablePro/Core/Storage/GroupStorage.swiftTablePro/Core/Storage/QueryHistoryStorage.swiftTablePro/Core/Storage/TagStorage.swiftTablePro/Core/Sync/CloudKitSyncEngine.swiftTablePro/Core/Sync/ConflictResolver.swiftTablePro/Core/Sync/SyncChangeTracker.swiftTablePro/Core/Sync/SyncCoordinator.swiftTablePro/Core/Sync/SyncError.swiftTablePro/Core/Sync/SyncMetadataStorage.swiftTablePro/Core/Sync/SyncRecordMapper.swiftTablePro/Core/Sync/SyncStatus.swiftTablePro/Models/Settings/ProFeature.swiftTablePro/Models/Settings/SyncSettings.swiftTablePro/Resources/Localizable.xcstringsTablePro/TablePro.entitlementsTablePro/Views/Components/ConflictResolutionView.swiftTablePro/Views/Components/ProFeatureGate.swiftTablePro/Views/Components/SyncStatusIndicator.swiftTablePro/Views/Connection/WelcomeWindowView.swiftTablePro/Views/Settings/SettingsView.swiftTablePro/Views/Settings/SyncSettingsView.swiftdocs/development/building.mdxdocs/development/setup.mdxdocs/vi/development/building.mdxdocs/vi/development/setup.mdxdocs/zh/development/building.mdxdocs/zh/development/setup.mdx
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
TablePro/Core/Sync/SyncCoordinator.swift (2)
431-459: Consider usingelse iffor mutually exclusive prefix checks.Record names can only have one prefix, so early exit would be cleaner and marginally more efficient for bulk deletions.
♻️ Suggested refactor
if recordName.hasPrefix("Connection_") { let uuidString = String(recordName.dropFirst("Connection_".count)) if let uuid = UUID(uuidString: uuidString) { var connections = ConnectionStorage.shared.loadConnections() connections.removeAll { $0.id == uuid } ConnectionStorage.shared.saveConnections(connections) } - } - if recordName.hasPrefix("Group_") { + } else if recordName.hasPrefix("Group_") { let uuidString = String(recordName.dropFirst("Group_".count)) // ... - } - - if recordName.hasPrefix("Tag_") { + } else if recordName.hasPrefix("Tag_") { let uuidString = String(recordName.dropFirst("Tag_".count)) // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 431 - 459, The applyRemoteDeletion(_:) function currently checks recordName prefixes with three separate if blocks even though a record can only match one prefix; change the second and third checks to else if (or use a switch on recordName.hasPrefix or early return after handling) so only the matching branch runs—for example, keep the initial if for "Connection_" then use else if for "Group_" and else if for "Tag_" (refer to applyRemoteDeletion, recordName, prefixes "Connection_", "Group_", "Tag_") and ensure behavior (loading/removing/saving via ConnectionStorage.shared, GroupStorage.shared, TagStorage.shared) is unchanged.
375-381: Hardcoded zone name should be centralized.The zone name "TableProSync" is hardcoded here but likely defined in
CloudKitSyncEngine. Using a constant from the engine would prevent inconsistency.♻️ Suggested improvement
let localRecord = SyncRecordMapper.toCKRecord( connections[index], - in: CKRecordZone.ID( - zoneName: "TableProSync", - ownerName: CKCurrentUserDefaultName - ) + in: await engine.zoneID )Note: This would require making
applyRemoteConnectionasync or passing the zoneID as a parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 375 - 381, The call in SyncCoordinator.swift uses a hardcoded zone name ("TableProSync") when creating CKRecordZone.ID inside SyncRecordMapper.toCKRecord; instead centralize the zone identifier in CloudKitSyncEngine (e.g., expose a static/instance property like zoneID or zoneName) and change applyRemoteConnection to accept a CKRecordZone.ID parameter (or retrieve CloudKitSyncEngine.zoneID) instead of constructing it inline. Update the SyncCoordinator.applyRemoteConnection signature and all its callers to pass the engine's zoneID (or make applyRemoteConnection async if you prefer retrieving it asynchronously), and replace the hardcoded CKRecordZone.ID construction in the SyncRecordMapper.toCKRecord invocation with the centralized zoneID reference.
🤖 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/Core/Sync/SyncCoordinator.swift`:
- Around line 630-654: The collectDirtyGroups and collectDirtyTags functions
currently only append tombstone deletions; update each to also iterate
metadataStorage.dirty(for: .group) and metadataStorage.dirty(for: .tag)
respectively and append corresponding CKRecord objects to the records array
using the existing SyncRecordMapper factory (mirror the pattern used for
connections), i.e. create a CKRecord for each dirty item's id/fields via
SyncRecordMapper and append it to records so modified/new groups and tags are
pushed; keep the tombstone deletion logic unchanged.
- Around line 221-268: performPush currently omits collecting dirty query
history records; add logic mirroring connections/groups/settings to gather dirty
query history into recordsToSave and deletion tombstones into recordIDsToDelete
when AppSettingsStorage.shared.loadSync().syncQueryHistory is true. Use
changeTracker.dirtyRecords(for: .queryHistory) to get dirty IDs, map each ID to
a CKRecord via SyncRecordMapper.toCKRecord(queryHistory:in:) or the existing
toCKRecord overload for query history (use zoneID), and append tombstones from
metadataStorage.tombstones(for: .queryHistory) by creating recordIDs with
SyncRecordMapper.recordID(type: .queryHistory, id:in:). Ensure this code runs in
performPush alongside the other collection blocks and uses the same zoneID,
recordsToSave, and recordIDsToDelete variables.
In `@TablePro/Core/Sync/SyncRecordMapper.swift`:
- Around line 82-91: The JSON encoding for connection.sshConfig,
connection.sslConfig, and connection.additionalFields currently uses try? which
silences errors and can drop data; change these to do/try/catch (or use try?
with nil-check) around encoder.encode(...) for each of sshConfig, sslConfig and
additionalFields in the mapping function that builds record (where
record["sshConfigJson"], record["sslConfigJson"], record["additionalFieldsJson"]
are set) and log a warning including the field name and the encoding error
details when encoding fails so failures are visible for debugging.
- Around line 251-299: Add a decoder method named toQueryHistory(on
SyncRecordMapper) that mirrors toCKRecord and the other decoders: accept a
CKRecord and return an optional QueryHistory by extracting "entryId", "query",
"executedAt", "executionTime", "rowCount", "wasSuccessful" (convert Int64 ->
Bool), "connectionId", "databaseName", "errorMessage", and "schemaVersion";
validate required fields (entryId, query, executedAt, executionTime, rowCount,
wasSuccessful) and return nil on failure, and match types used elsewhere (e.g.,
Int64 for rowCount, Double for executionTime) so SyncCoordinator and
ConflictResolutionView can reconstruct QueryHistory objects from pulled records.
In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 1045-1048: Move the tombstone emission so it happens only after a
successful local delete: load connections via storage.loadConnections(), remove
matching entries with savedConnections.removeAll { $0.id == id }, call
storage.saveConnections(savedConnections), then if the removal actually occurred
(e.g. compare counts or check that an element with id no longer exists) call
SyncChangeTracker.shared.markDeleted(.connection, id: id.uuidString); this
ensures markDeleted is invoked only after persistence and only when a record was
actually removed.
---
Nitpick comments:
In `@TablePro/Core/Sync/SyncCoordinator.swift`:
- Around line 431-459: The applyRemoteDeletion(_:) function currently checks
recordName prefixes with three separate if blocks even though a record can only
match one prefix; change the second and third checks to else if (or use a switch
on recordName.hasPrefix or early return after handling) so only the matching
branch runs—for example, keep the initial if for "Connection_" then use else if
for "Group_" and else if for "Tag_" (refer to applyRemoteDeletion, recordName,
prefixes "Connection_", "Group_", "Tag_") and ensure behavior
(loading/removing/saving via ConnectionStorage.shared, GroupStorage.shared,
TagStorage.shared) is unchanged.
- Around line 375-381: The call in SyncCoordinator.swift uses a hardcoded zone
name ("TableProSync") when creating CKRecordZone.ID inside
SyncRecordMapper.toCKRecord; instead centralize the zone identifier in
CloudKitSyncEngine (e.g., expose a static/instance property like zoneID or
zoneName) and change applyRemoteConnection to accept a CKRecordZone.ID parameter
(or retrieve CloudKitSyncEngine.zoneID) instead of constructing it inline.
Update the SyncCoordinator.applyRemoteConnection signature and all its callers
to pass the engine's zoneID (or make applyRemoteConnection async if you prefer
retrieving it asynchronously), and replace the hardcoded CKRecordZone.ID
construction in the SyncRecordMapper.toCKRecord invocation with the centralized
zoneID reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e815923c-79a7-4b93-9e63-cbdbd9daab35
📒 Files selected for processing (9)
TablePro.xcodeproj/project.pbxprojTablePro/Core/Storage/AppSettingsStorage.swiftTablePro/Core/Storage/ConnectionStorage.swiftTablePro/Core/Sync/CloudKitSyncEngine.swiftTablePro/Core/Sync/SyncChangeTracker.swiftTablePro/Core/Sync/SyncCoordinator.swiftTablePro/Core/Sync/SyncRecordMapper.swiftTablePro/Models/Settings/ProFeature.swiftTablePro/Views/Connection/ConnectionFormView.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- TablePro.xcodeproj/project.pbxproj
- TablePro/Core/Sync/CloudKitSyncEngine.swift
- TablePro/Core/Storage/ConnectionStorage.swift
- TablePro/Models/Settings/ProFeature.swift
- TablePro/Core/Sync/SyncChangeTracker.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
TablePro/Core/Sync/CloudKitSyncEngine.swift (1)
45-49:⚠️ Potential issue | 🟠 MajorHandle
.zoneAlreadyExistserror when saving zone.The
ensureZoneExists()method will throw an error on subsequent calls when the zone already exists. CloudKit returns.zoneAlreadyExists(not.serverRejectedRequest) when saving an existing zone.🐛 Proposed fix
func ensureZoneExists() async throws { let zone = CKRecordZone(zoneID: zoneID) - _ = try await database.save(zone) - Self.logger.trace("Created or confirmed sync zone: \(Self.zoneName)") + do { + _ = try await database.save(zone) + Self.logger.trace("Created sync zone: \(Self.zoneName)") + } catch let error as CKError where error.code == .zoneAlreadyExists { + Self.logger.trace("Sync zone already exists: \(Self.zoneName)") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/CloudKitSyncEngine.swift` around lines 45 - 49, ensureZoneExists currently calls database.save(zone) and will throw when the zone already exists; update the ensureZoneExists() implementation to catch errors from the save call, detect a CKError whose code is .zoneAlreadyExists (use CKError and CKError.Code.zoneAlreadyExists) and silently ignore that case while rethrowing any other errors, keeping the existing trace log on success or when the zone already existed; reference ensureZoneExists, zone, and database.save to locate where to add the try/catch handling.
🧹 Nitpick comments (9)
TablePro/Core/Sync/SyncChangeTracker.swift (2)
34-39: Consider using.traceinstead of.infofor single-item dirty marking.Line 37 uses
.infolevel for single-item marking, but line 46 uses.tracefor batch marking. For consistency and to reduce log noise in production, consider using.tracefor both.📝 Proposed fix
func markDirty(_ type: SyncRecordType, id: String) { guard !isSuppressed else { return } metadataStorage.addDirty(type: type, id: id) - Self.logger.info("Marked dirty: \(type.rawValue)/\(id)") + Self.logger.trace("Marked dirty: \(type.rawValue)/\(id)") postChangeNotification() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncChangeTracker.swift` around lines 34 - 39, The log level in markDirty(_ type: SyncRecordType, id: String) is too noisy—replace the Self.logger.info(...) call with Self.logger.trace(...) so single-item dirty marks match the batch-marking trace level used elsewhere (see the batch path using Self.logger.trace), leaving metadataStorage.addDirty(...) and postChangeNotification() unchanged.
16-18: Add explicit access control to the class declaration.The class uses implicit
internalaccess. Per coding guidelines, types should have explicit access control.🔧 Proposed fix
/// Tracks dirty entities and deletions for sync -final class SyncChangeTracker { +internal final class SyncChangeTracker { static let shared = SyncChangeTracker()As per coding guidelines: "Always use explicit access control on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncChangeTracker.swift` around lines 16 - 18, The class declaration lacks explicit access control; change "final class SyncChangeTracker" to include an explicit modifier (e.g., "final internal class SyncChangeTracker" or "public" if it must be exposed) and ensure the static members (shared, logger) keep appropriate visibility after the change; also review any extensions or related types for the same explicit access-control requirement.TablePro/Core/Sync/SyncCoordinator.swift (6)
629-633: Hardcoded container identifier duplicatesCloudKitSyncEngine.containerIdentifier.Line 630 hardcodes
"iCloud.com.TablePro"which is also defined as a private constant inCloudKitSyncEngine. Consider exposing a shared constant or accessing via the engine.♻️ Proposed fix
Option 1 - Create a shared constants file:
// SyncConstants.swift enum SyncConstants { static let containerIdentifier = "iCloud.com.TablePro" static let zoneName = "TableProSync" }Option 2 - Expose from engine and use here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 629 - 633, The currentAccountId() function hardcodes the container identifier string ("iCloud.com.TablePro"); replace that literal with a shared constant (either make CloudKitSyncEngine.containerIdentifier public/static or introduce a SyncConstants.containerIdentifier) and use that constant when constructing CKContainer(identifier:) so the identifier is defined in one place; update CloudKitSyncEngine to expose containerIdentifier if needed and switch currentAccountId() to reference that symbol instead of the hardcoded string.
14-17: Add explicit access control to the class declaration.The class uses implicit
internalaccess. Per coding guidelines, types should have explicit access control.🔧 Proposed fix
`@MainActor` `@Observable` -final class SyncCoordinator { +internal final class SyncCoordinator { static let shared = SyncCoordinator()As per coding guidelines: "Always use explicit access control on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 14 - 17, The SyncCoordinator type currently relies on implicit internal access; update its declaration to include an explicit access control modifier (e.g., make it public or internal as your API requires) on the class declaration line for SyncCoordinator (the one with `@MainActor` `@Observable` final class SyncCoordinator) and ensure any related symbols you intend to expose (such as the static shared and logger properties) have matching explicit access levels to satisfy the coding guideline requiring explicit access control on types and their members.
448-454: Hardcoded zone name duplicatesCloudKitSyncEngine.zoneName.Line 451 hardcodes
"TableProSync"which duplicates the private constant inCloudKitSyncEngine. If the zone name changes, this would cause a mismatch.♻️ Proposed fix
Consider exposing the zone ID from the engine or using a shared constant:
if changeTracker.dirtyRecords(for: .connection).contains(remoteConnection.id.uuidString) { + let zoneID = await engine.zoneID let localRecord = SyncRecordMapper.toCKRecord( connections[index], - in: CKRecordZone.ID( - zoneName: "TableProSync", - ownerName: CKCurrentUserDefaultName - ) + in: zoneID )Since
engine.zoneIDis already aletproperty on the actor, you can access it directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 448 - 454, The code in SyncCoordinator.swift calls SyncRecordMapper.toCKRecord with a hardcoded CKRecordZone.ID("TableProSync", CKCurrentUserDefaultName) which duplicates CloudKitSyncEngine.zoneName; change this to use the engine's exposed zone ID instead (use the actor's engine.zoneID property) so SyncRecordMapper.toCKRecord receives the correct CKRecordZone.ID from the CloudKitSyncEngine instance and avoids mismatches if the zone name changes.
380-382: Per-record logging may be excessive for large syncs.Logging each pulled record at
.infolevel (line 381) could flood logs during full fetches with many records. Consider.traceor batched logging.📝 Proposed fix
for record in result.changedRecords { - Self.logger.info("Pulled record: \(record.recordType)/\(record.recordID.recordName)") + Self.logger.trace("Pulled record: \(record.recordType)/\(record.recordID.recordName)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 380 - 382, The per-record logging inside the for loop that iterates result.changedRecords uses Self.logger.info and can flood logs during large syncs; change this to a lower verbosity (e.g., Self.logger.trace) or replace with batched/log-summary behavior by removing the per-record info-level call in the loop and instead emit an aggregated summary (count, sample N record IDs, or progress checkpoints) after processing in SyncCoordinator (the loop over result.changedRecords) so large syncs no longer produce excessive info-level log lines.
530-534: Nested Task is unnecessary inside@MainActormethod.The
Task { }at line 530 is already on MainActor since the enclosing method is@MainActor. The task will inherit the actor context. If the intent is to avoid blocking, consider usingTask.detachedor moving the entire method to be async.♻️ Proposed fix
If the async storage calls should not block the current flow:
- Task { - _ = await QueryHistoryStorage.shared.addHistory(entry) - await QueryHistoryStorage.shared.markHistoryEntriesSynced(ids: [entryIdString]) - } + Task.detached { + _ = await QueryHistoryStorage.shared.addHistory(entry) + await QueryHistoryStorage.shared.markHistoryEntriesSynced(ids: [entryIdString]) + }Or make
applyRemoteQueryHistoryasync and await directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 530 - 534, The nested Task inside the `@MainActor` method (remove the Task { ... } wrapper around the calls to QueryHistoryStorage.shared.addHistory and markHistoryEntriesSynced) is unnecessary because the method is already on the MainActor; either remove the Task and call await QueryHistoryStorage.shared.addHistory(entry) and await QueryHistoryStorage.shared.markHistoryEntriesSynced(ids: [entryIdString]) directly (or make applyRemoteQueryHistory async and await them), or if you need to run off the MainActor use Task.detached to perform those storage calls off the actor—update the code accordingly around the addHistory and markHistoryEntriesSynced calls.
395-396: TODO: Storage I/O on MainActor may block UI for large datasets.The comment acknowledges this concern. For large syncs, consider dispatching storage operations to a background queue and hopping back to MainActor only for state updates.
Would you like me to open an issue to track this optimization, or generate a refactored implementation that moves storage I/O off the main actor?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncCoordinator.swift` around lines 395 - 396, The applyRemoteChanges(PullResult) method performs storage I/O on the MainActor which may block the UI for large datasets; change it to perform heavy disk/database work off the MainActor (e.g., run the storage operations inside Task.detached or DispatchQueue.global().async) and only hop back to the MainActor for brief state/UI updates using await MainActor.run { ... }. Specifically, move any code that processes PullResult and writes to storage out of applyRemoteChanges' MainActor context (or extract it to a non-@MainActor helper like processAndPersistPullResult(_:)) and keep only the minimal state mutations and notifications on MainActor so applyRemoteChanges calls the background helper then updates state with MainActor.run.TablePro/Views/Components/ConflictResolutionView.swift (1)
144-150: Localize user-facing field labels.The
fieldRowhelper passes unlocalized strings for labels like "Name", "Color", and "Query" (lines 128, 131, 139). While connection-specific terms like "Host"/"Port" could be considered technical, general labels should useString(localized:).🌐 Proposed fix
private func fieldRow(label: String, value: String) -> some View { - LabeledContent(label + ":") { + LabeledContent(String(localized: "\(label):")) { Text(value) .lineLimit(1) } .font(.caption) }Alternatively, pass already-localized labels from callers:
- fieldRow(label: "Name", value: name) + fieldRow(label: String(localized: "Name"), value: name)As per coding guidelines: "Use String(localized:) for new user-facing strings in computed properties."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/ConflictResolutionView.swift` around lines 144 - 150, The fieldRow helper currently passes raw label strings into LabeledContent; update it to use localized labels by wrapping the label in String(localized:) before appending the colon (e.g., LabeledContent(String(localized: label) + ":") { ... }) so all user-facing labels (from callers like the ones passing "Name", "Color", "Query") are localized; alternatively, if you prefer caller-side localization, ensure callers pass String(localized: "Name") etc., but make the change in fieldRow (function name fieldRow and the LabeledContent call) to comply with the guideline.
🤖 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/Views/Components/ConflictResolutionView.swift`:
- Around line 170-183: The progress text is hardcoded to "1 of N"; update
progressIndicator to compute the current position instead: derive currentIndex
by locating the currently displayed conflict in
conflictResolver.pendingConflicts (e.g., find index of
conflictResolver.currentConflict) or use a resolved/ currentConflictIndex
property on conflictResolver, then display "\(currentIndex + 1) of \(total)"
(keep total = conflictResolver.pendingConflicts.count) so the Text shows the
actual conflict number; adjust ConflictResolutionView to reference
conflictResolver.currentConflict or add a currentConflictIndex/resolvedCount to
conflictResolver if needed.
---
Duplicate comments:
In `@TablePro/Core/Sync/CloudKitSyncEngine.swift`:
- Around line 45-49: ensureZoneExists currently calls database.save(zone) and
will throw when the zone already exists; update the ensureZoneExists()
implementation to catch errors from the save call, detect a CKError whose code
is .zoneAlreadyExists (use CKError and CKError.Code.zoneAlreadyExists) and
silently ignore that case while rethrowing any other errors, keeping the
existing trace log on success or when the zone already existed; reference
ensureZoneExists, zone, and database.save to locate where to add the try/catch
handling.
---
Nitpick comments:
In `@TablePro/Core/Sync/SyncChangeTracker.swift`:
- Around line 34-39: The log level in markDirty(_ type: SyncRecordType, id:
String) is too noisy—replace the Self.logger.info(...) call with
Self.logger.trace(...) so single-item dirty marks match the batch-marking trace
level used elsewhere (see the batch path using Self.logger.trace), leaving
metadataStorage.addDirty(...) and postChangeNotification() unchanged.
- Around line 16-18: The class declaration lacks explicit access control; change
"final class SyncChangeTracker" to include an explicit modifier (e.g., "final
internal class SyncChangeTracker" or "public" if it must be exposed) and ensure
the static members (shared, logger) keep appropriate visibility after the
change; also review any extensions or related types for the same explicit
access-control requirement.
In `@TablePro/Core/Sync/SyncCoordinator.swift`:
- Around line 629-633: The currentAccountId() function hardcodes the container
identifier string ("iCloud.com.TablePro"); replace that literal with a shared
constant (either make CloudKitSyncEngine.containerIdentifier public/static or
introduce a SyncConstants.containerIdentifier) and use that constant when
constructing CKContainer(identifier:) so the identifier is defined in one place;
update CloudKitSyncEngine to expose containerIdentifier if needed and switch
currentAccountId() to reference that symbol instead of the hardcoded string.
- Around line 14-17: The SyncCoordinator type currently relies on implicit
internal access; update its declaration to include an explicit access control
modifier (e.g., make it public or internal as your API requires) on the class
declaration line for SyncCoordinator (the one with `@MainActor` `@Observable` final
class SyncCoordinator) and ensure any related symbols you intend to expose (such
as the static shared and logger properties) have matching explicit access levels
to satisfy the coding guideline requiring explicit access control on types and
their members.
- Around line 448-454: The code in SyncCoordinator.swift calls
SyncRecordMapper.toCKRecord with a hardcoded CKRecordZone.ID("TableProSync",
CKCurrentUserDefaultName) which duplicates CloudKitSyncEngine.zoneName; change
this to use the engine's exposed zone ID instead (use the actor's engine.zoneID
property) so SyncRecordMapper.toCKRecord receives the correct CKRecordZone.ID
from the CloudKitSyncEngine instance and avoids mismatches if the zone name
changes.
- Around line 380-382: The per-record logging inside the for loop that iterates
result.changedRecords uses Self.logger.info and can flood logs during large
syncs; change this to a lower verbosity (e.g., Self.logger.trace) or replace
with batched/log-summary behavior by removing the per-record info-level call in
the loop and instead emit an aggregated summary (count, sample N record IDs, or
progress checkpoints) after processing in SyncCoordinator (the loop over
result.changedRecords) so large syncs no longer produce excessive info-level log
lines.
- Around line 530-534: The nested Task inside the `@MainActor` method (remove the
Task { ... } wrapper around the calls to QueryHistoryStorage.shared.addHistory
and markHistoryEntriesSynced) is unnecessary because the method is already on
the MainActor; either remove the Task and call await
QueryHistoryStorage.shared.addHistory(entry) and await
QueryHistoryStorage.shared.markHistoryEntriesSynced(ids: [entryIdString])
directly (or make applyRemoteQueryHistory async and await them), or if you need
to run off the MainActor use Task.detached to perform those storage calls off
the actor—update the code accordingly around the addHistory and
markHistoryEntriesSynced calls.
- Around line 395-396: The applyRemoteChanges(PullResult) method performs
storage I/O on the MainActor which may block the UI for large datasets; change
it to perform heavy disk/database work off the MainActor (e.g., run the storage
operations inside Task.detached or DispatchQueue.global().async) and only hop
back to the MainActor for brief state/UI updates using await MainActor.run { ...
}. Specifically, move any code that processes PullResult and writes to storage
out of applyRemoteChanges' MainActor context (or extract it to a non-@MainActor
helper like processAndPersistPullResult(_:)) and keep only the minimal state
mutations and notifications on MainActor so applyRemoteChanges calls the
background helper then updates state with MainActor.run.
In `@TablePro/Views/Components/ConflictResolutionView.swift`:
- Around line 144-150: The fieldRow helper currently passes raw label strings
into LabeledContent; update it to use localized labels by wrapping the label in
String(localized:) before appending the colon (e.g.,
LabeledContent(String(localized: label) + ":") { ... }) so all user-facing
labels (from callers like the ones passing "Name", "Color", "Query") are
localized; alternatively, if you prefer caller-side localization, ensure callers
pass String(localized: "Name") etc., but make the change in fieldRow (function
name fieldRow and the LabeledContent call) to comply with the guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f23d777-7483-464b-937a-c02ea8aa6b17
📒 Files selected for processing (5)
TablePro/Core/Sync/CloudKitSyncEngine.swiftTablePro/Core/Sync/SyncChangeTracker.swiftTablePro/Core/Sync/SyncCoordinator.swiftTablePro/Core/Sync/SyncError.swiftTablePro/Views/Components/ConflictResolutionView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- TablePro/Core/Sync/SyncError.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
TablePro/Core/Sync/SyncRecordMapper.swift (1)
261-309:⚠️ Potential issue | 🟠 MajorMissing reverse mapper for Query History records.
SyncRecordMapperwrites query-history records viatoCKRecord(...)but has no correspondingtoQueryHistory(...)decoder. This leaves query-history mapping asymmetric and risks dropped/ignored pulled records for this type. Please add a validated reverse conversion path (required fields + warning on malformed records).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncRecordMapper.swift` around lines 261 - 309, Add a reverse mapper in SyncRecordMapper named toQueryHistory(from record: CKRecord) -> QueryHistory? (or similar optional-returning type) that decodes records with recordType .queryHistory by reading required fields entryId, query, executedAt, executionTime, rowCount, wasSuccessful and schemaVersion (use the same keys like "entryId", "query", "executedAt", "executionTime", "rowCount", "wasSuccessful", "schemaVersion"), populate optional fields connectionId, databaseName, errorMessage when present, and return a typed QueryHistory model; if any required field is missing or of the wrong type, log a clear warning (use the project logger) and return nil so malformed pulled records are ignored but visible in logs.
🧹 Nitpick comments (2)
TablePro/Core/Sync/SyncRecordMapper.swift (2)
13-23: Add explicit access control to top-level types.
SyncRecordTypeandSyncRecordMapperare missing explicit access modifiers. Please declare them explicitly (for example,internal) to match repository standards.Suggested patch
-enum SyncRecordType: String, CaseIterable { +internal enum SyncRecordType: String, CaseIterable { @@ -struct SyncRecordMapper { +internal struct SyncRecordMapper {As per coding guidelines, "Always use explicit access control on extensions and types (private, internal, public)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncRecordMapper.swift` around lines 13 - 23, The top-level types are missing explicit access control; add an explicit access level (e.g., internal) to the enum SyncRecordType and the struct SyncRecordMapper declarations so they conform to the repository guideline requiring explicit access modifiers; keep the existing private modifier on the nested logger unchanged (Logger in SyncRecordMapper) and update only the declarations for SyncRecordType and SyncRecordMapper.
129-141: Avoid silent decode fallbacks for JSON blobs.On Line 130, Line 135, and Line 140,
try?swallows decode errors forsshConfigJson,sslConfigJson, andadditionalFieldsJson. Please log decode failures so corrupted/old records are diagnosable during pull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncRecordMapper.swift` around lines 129 - 141, The JSON decodes for sshConfigJson, sslConfigJson and additionalFieldsJson currently swallow errors with try?, so replace each silent decode in SyncRecordMapper (the blocks producing sshConfig, sslConfig and additionalFields) with explicit do-catch decode logic that logs failures (include the JSON key name and the decode error) and falls back to the default value on error; specifically, change uses of decoder.decode(...) wrapped in try? to do { sshConfig = try decoder.decode(SSHConfiguration.self, from: sshData) } catch { logger.error("Failed to decode sshConfigJson: \(error)") ; sshConfig = SSHConfiguration() } (and analogous code for sslConfigJson -> SSLConfiguration and additionalFieldsJson -> [String: String]) so corrupted/old records are diagnosable during pull.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@TablePro/Core/Sync/SyncRecordMapper.swift`:
- Around line 261-309: Add a reverse mapper in SyncRecordMapper named
toQueryHistory(from record: CKRecord) -> QueryHistory? (or similar
optional-returning type) that decodes records with recordType .queryHistory by
reading required fields entryId, query, executedAt, executionTime, rowCount,
wasSuccessful and schemaVersion (use the same keys like "entryId", "query",
"executedAt", "executionTime", "rowCount", "wasSuccessful", "schemaVersion"),
populate optional fields connectionId, databaseName, errorMessage when present,
and return a typed QueryHistory model; if any required field is missing or of
the wrong type, log a clear warning (use the project logger) and return nil so
malformed pulled records are ignored but visible in logs.
---
Nitpick comments:
In `@TablePro/Core/Sync/SyncRecordMapper.swift`:
- Around line 13-23: The top-level types are missing explicit access control;
add an explicit access level (e.g., internal) to the enum SyncRecordType and the
struct SyncRecordMapper declarations so they conform to the repository guideline
requiring explicit access modifiers; keep the existing private modifier on the
nested logger unchanged (Logger in SyncRecordMapper) and update only the
declarations for SyncRecordType and SyncRecordMapper.
- Around line 129-141: The JSON decodes for sshConfigJson, sslConfigJson and
additionalFieldsJson currently swallow errors with try?, so replace each silent
decode in SyncRecordMapper (the blocks producing sshConfig, sslConfig and
additionalFields) with explicit do-catch decode logic that logs failures
(include the JSON key name and the decode error) and falls back to the default
value on error; specifically, change uses of decoder.decode(...) wrapped in try?
to do { sshConfig = try decoder.decode(SSHConfiguration.self, from: sshData) }
catch { logger.error("Failed to decode sshConfigJson: \(error)") ; sshConfig =
SSHConfiguration() } (and analogous code for sslConfigJson -> SSLConfiguration
and additionalFieldsJson -> [String: String]) so corrupted/old records are
diagnosable during pull.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89353b72-9e4a-4af9-9835-f5876e261c9f
📒 Files selected for processing (2)
TablePro/Core/Sync/SyncRecordMapper.swiftTablePro/Views/Connection/ConnectionFormView.swift
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/customization/settings.mdx (1)
429-609:⚠️ Potential issue | 🟠 MajorVietnamese settings documentation was not updated with the Sync section and related content.
The Vietnamese settings page (
docs/vi/customization/settings.mdx) is missing:
- The entire Sync Settings section (iCloud Sync Toggle, Status, Sync Categories, History Limit)
- License Settings section
- Settings Storage section
- Settings by Feature section (including Sync-Related subsection)
Per the documentation guidelines, updates to
docs/customization/settings.mdxrequire corresponding updates todocs/vi/customization/settings.mdx. Translate and add these sections to the Vietnamese version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/customization/settings.mdx` around lines 429 - 609, Add translations of the missing sections to the Vietnamese settings doc by copying and translating the English headings and content for "## Sync Settings" (including "### iCloud Sync Toggle", "### Status", "### Sync Categories", "### History Limit" and the Note about passwords), "## License Settings" (activation/deactivation, "### Offline Use"), "## Settings Storage" (with reset commands and Warning), and the "## Settings by Feature" block (including the "Sync-Related" subsection and table entries). Ensure the Vietnamese file contains the same headings ("## Sync Settings", "## License Settings", "## Settings Storage", "### Settings by Feature") and the translated tables/notes/images/captions to mirror the English content and structure so the docs remain parity-complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/customization/settings.mdx`:
- Line 429: Convert the new documentation headings that are in Title Case to
sentence case in docs/customization/settings.mdx; locate and replace headings
like "Sync Settings", "iCloud Sync Toggle" (or similar iCloud headings), "Sync
Categories", "History Limit", and "Sync-related" with their sentence-case forms
("Sync settings", "iCloud sync toggle", "Sync categories", "History limit",
"Sync-related") so all new headings follow the project's sentence-case
guideline.
In `@docs/features/icloud-sync.mdx`:
- Line 10: Update the heading text to use sentence case instead of Title Case:
change "What Syncs (and What Doesn't)" to "What syncs (and what doesn't)" and
similarly convert the other headings mentioned to sentence case (e.g., "Related
Pages" → "Related pages"); locate and edit the heading strings in
docs/features/icloud-sync.mdx by searching for those exact heading texts and
replace them with their sentence-case equivalents to comply with the
documentation style guide.
- Line 81: The navigation reference should bold UI element names: update the
sentence to use bold styling for the button and each UI path segment (change
"Use the **Sync Now** button in Settings > Sync" to "Use the **Sync Now** button
in **Settings** > **Sync**"); apply the same change to the other occurrence
mentioned (the instance referencing Settings and Sync around line 149) so all UI
element labels follow the "Click **Settings**" guideline.
In `@docs/vi/features/icloud-sync.mdx`:
- Line 24: Update the heading text "Bật Đồng bộ iCloud" to sentence case by
changing it to "Bật đồng bộ iCloud" in the docs file; locate the markdown
heading line that currently reads "## Bật Đồng bộ iCloud" and replace it so the
heading follows the documentation sentence-case style.
- Line 81: The UI navigation labels in the sentence should use bold formatting:
change the button and path references so "Đồng bộ ngay" becomes **Đồng bộ ngay**
and "Cài đặt > Đồng bộ" becomes **Cài đặt** > **Đồng bộ** to follow the
documentation convention; also apply the same bolding to the other occurrence of
the path at line 149 (the second "Cài đặt > Đồng bộ" mention).
In `@docs/zh/features/icloud-sync.mdx`:
- Line 114: Update the Chinese phrasing in the "已同步" table cell by replacing the
existing string "所有数据已是最新" with the smoother wording "所有数据均为最新" in the docs file
(look for the table row containing the header "已同步" and that exact cell text) so
the synced-status reads naturally in Chinese.
- Line 81: Update the UI label formatting by making both parts bold: change the
plain text "设置 > 同步" to "**设置** > **同步**" wherever it appears (including the
other occurrence of the same phrase in the file), ensuring consistency with the
docs style guide for referencing UI elements.
---
Outside diff comments:
In `@docs/customization/settings.mdx`:
- Around line 429-609: Add translations of the missing sections to the
Vietnamese settings doc by copying and translating the English headings and
content for "## Sync Settings" (including "### iCloud Sync Toggle", "###
Status", "### Sync Categories", "### History Limit" and the Note about
passwords), "## License Settings" (activation/deactivation, "### Offline Use"),
"## Settings Storage" (with reset commands and Warning), and the "## Settings by
Feature" block (including the "Sync-Related" subsection and table entries).
Ensure the Vietnamese file contains the same headings ("## Sync Settings", "##
License Settings", "## Settings Storage", "### Settings by Feature") and the
translated tables/notes/images/captions to mirror the English content and
structure so the docs remain parity-complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20c16a5d-72fb-490f-b009-93732e485f62
📒 Files selected for processing (5)
docs/customization/settings.mdxdocs/docs.jsondocs/features/icloud-sync.mdxdocs/vi/features/icloud-sync.mdxdocs/zh/features/icloud-sync.mdx
| | Custom plugin registry | `defaults write com.TablePro com.TablePro.customRegistryURL <url>` | Use a private plugin registry | | ||
| | Clear custom registry | `defaults delete com.TablePro com.TablePro.customRegistryURL` | Revert to default registry | | ||
|
|
||
| ## Sync Settings |
There was a problem hiding this comment.
Use sentence case for newly added settings headings.
The new headings are in Title Case. Please convert to sentence case (for example, Sync settings, iCloud sync toggle, Sync categories, History limit, Sync-related).
As per coding guidelines: Use sentence case for headings in documentation.
Also applies to: 433-433, 447-447, 456-456, 599-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/customization/settings.mdx` at line 429, Convert the new documentation
headings that are in Title Case to sentence case in
docs/customization/settings.mdx; locate and replace headings like "Sync
Settings", "iCloud Sync Toggle" (or similar iCloud headings), "Sync Categories",
"History Limit", and "Sync-related" with their sentence-case forms ("Sync
settings", "iCloud sync toggle", "Sync categories", "History limit",
"Sync-related") so all new headings follow the project's sentence-case
guideline.
|
|
||
| TablePro syncs your connections, groups, settings, and query history across all your Macs via CloudKit. iCloud Sync is a Pro feature that requires an active license. | ||
|
|
||
| ## What Syncs (and What Doesn't) |
There was a problem hiding this comment.
Use sentence case for the new headings.
These headings are in Title Case. Please convert them to sentence case (for example, What syncs (and what doesn't) and Related pages) for consistency.
As per coding guidelines: Use sentence case for headings in documentation.
Also applies to: 24-24, 47-47, 58-58, 73-73, 83-83, 108-108, 133-133, 143-143, 164-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/features/icloud-sync.mdx` at line 10, Update the heading text to use
sentence case instead of Title Case: change "What Syncs (and What Doesn't)" to
"What syncs (and what doesn't)" and similarly convert the other headings
mentioned to sentence case (e.g., "Related Pages" → "Related pages"); locate and
edit the heading strings in docs/features/icloud-sync.mdx by searching for those
exact heading texts and replace them with their sentence-case equivalents to
comply with the documentation style guide.
| - **App foreground** -- syncs when you switch back to TablePro | ||
| - **Local edits** -- pushes changes 2 seconds after you modify synced data (debounced to avoid excess writes) | ||
|
|
||
| Use the **Sync Now** button in Settings > Sync to trigger a manual sync at any time. |
There was a problem hiding this comment.
Bold UI path labels in navigation references.
Use bold styling for UI elements in paths like Settings > Sync (for example, Settings > Sync) to keep formatting consistent.
As per coding guidelines: Use bold formatting when referencing UI elements (e.g., Click Settings).
Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/features/icloud-sync.mdx` at line 81, The navigation reference should
bold UI element names: update the sentence to use bold styling for the button
and each UI path segment (change "Use the **Sync Now** button in Settings >
Sync" to "Use the **Sync Now** button in **Settings** > **Sync**"); apply the
same change to the other occurrence mentioned (the instance referencing Settings
and Sync around line 149) so all UI element labels follow the "Click
**Settings**" guideline.
| Mật khẩu không bao giờ được đồng bộ. Chúng luôn nằm trong macOS Keychain trên mỗi máy. Sau khi đồng bộ kết nối sang máy Mac mới, bạn cần nhập mật khẩu một lần trên máy đó. | ||
| </Warning> | ||
|
|
||
| ## Bật Đồng bộ iCloud |
There was a problem hiding this comment.
Normalize heading capitalization to sentence case.
Bật Đồng bộ iCloud should use sentence case (Bật đồng bộ iCloud) for consistency with the docs standard.
As per coding guidelines: Use sentence case for headings in documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/vi/features/icloud-sync.mdx` at line 24, Update the heading text "Bật
Đồng bộ iCloud" to sentence case by changing it to "Bật đồng bộ iCloud" in the
docs file; locate the markdown heading line that currently reads "## Bật Đồng bộ
iCloud" and replace it so the heading follows the documentation sentence-case
style.
| - **Ứng dụng trở lại foreground** -- đồng bộ khi bạn chuyển về TablePro | ||
| - **Chỉnh sửa cục bộ** -- đẩy thay đổi sau 2 giây khi bạn sửa dữ liệu đồng bộ (debounce để tránh ghi quá nhiều) | ||
|
|
||
| Dùng nút **Đồng bộ ngay** trong Cài đặt > Đồng bộ để đồng bộ thủ công bất cứ lúc nào. |
There was a problem hiding this comment.
Apply bold formatting to UI navigation labels.
In paths like Cài đặt > Đồng bộ, please bold UI elements (for example, Cài đặt > Đồng bộ) to match the documentation convention.
As per coding guidelines: Use bold formatting when referencing UI elements (e.g., Click Settings).
Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/vi/features/icloud-sync.mdx` at line 81, The UI navigation labels in the
sentence should use bold formatting: change the button and path references so
"Đồng bộ ngay" becomes **Đồng bộ ngay** and "Cài đặt > Đồng bộ" becomes **Cài
đặt** > **Đồng bộ** to follow the documentation convention; also apply the same
bolding to the other occurrence of the path at line 149 (the second "Cài đặt >
Đồng bộ" mention).
| - **应用回到前台** -- 切换回 TablePro 时同步 | ||
| - **本地编辑** -- 修改同步数据后 2 秒推送更改（防抖以避免过多写入） | ||
|
|
||
| 在设置 > 同步中使用 **立即同步** 按钮可随时手动触发同步。 |
There was a problem hiding this comment.
Format UI navigation labels consistently.
设置 > 同步 is presented as plain text. Please format UI labels as bold (for example, 设置 > 同步) to match the docs style guide.
As per coding guidelines: Use bold formatting when referencing UI elements (e.g., Click Settings).
Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/zh/features/icloud-sync.mdx` at line 81, Update the UI label formatting
by making both parts bold: change the plain text "设置 > 同步" to "**设置** > **同步**"
wherever it appears (including the other occurrence of the same phrase in the
file), ensuring consistency with the docs style guide for referencing UI
elements.
|
|
||
| | 状态 | 含义 | | ||
| |------|------| | ||
| | **已同步** | 所有数据已是最新 | |
There was a problem hiding this comment.
Polish the synced-status wording for natural Chinese.
Consider changing 所有数据已是最新 to 所有数据均为最新 for smoother phrasing.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~114-~114: 您不可用“是”来表达个形容词。大部分人都平常用“很”来代替“是”。您是不是想表达"很最新"？
Context: ...| 含义 | |------|------| | 已同步 | 所有数据已是最新 | | 同步中 | 正在同步 | | 错误 | 同步失败（悬停...
(SHI_ADHECTIVE_ERROR)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/zh/features/icloud-sync.mdx` at line 114, Update the Chinese phrasing in
the "已同步" table cell by replacing the existing string "所有数据已是最新" with the
smoother wording "所有数据均为最新" in the docs file (look for the table row containing
the header "已同步" and that exact cell text) so the synced-status reads naturally
in Chinese.
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
Architecture
CloudKitSyncEngine(actor): zone management, push/pull operations with retry logicSyncCoordinator(@mainactor @observable): orchestrates sync lifecycle, license gating, schedulingSyncChangeTracker: tracks dirty entities and deletion tombstonesSyncRecordMapper: CKRecord ↔ local model conversion with schema versioningConflictResolver: queues conflicts for user resolution (Keep Local / Keep Remote)ProFeatureGateModifier: reusable SwiftUI view modifier for gating any feature behind Pro licenseNew files (16)
Core/Sync/— CloudKitSyncEngine, SyncCoordinator, SyncChangeTracker, SyncMetadataStorage, SyncRecordMapper, ConflictResolver, SyncStatus, SyncErrorModels/Settings/— ProFeature, SyncSettingsCore/Services/Licensing/— LicenseManager+ProViews/— SyncSettingsView, SyncStatusIndicator, ConflictResolutionView, ProFeatureGateModified files (13)
is_syncedcolumn with migration.requiresPro(.iCloudSync)Test plan
Summary by CodeRabbit
New Features
Documentation