Conversation
* Fix 'NULL DEFAULT _' migrations. * clean up * Add test for when old device makes change and syncs to new device. * fixes * clean up * filter columnNames by allColumnNames * clean up
* Fix user modification timestamp. * wip
* Add a test for what happens with outside records. * wip * fix tests * Skip records that don't have a userModificationTime.
* Fixes to date synchronization. * add test for bad representation * back out of pending changes stuff * clean up * wip
* Update start behavior for sync engine. * wip * wip * wip --------- Co-authored-by: Brandon Williams <mbrandonw@hey.com>
* Detect `SyncEngine.isSynchronizing` misuse in triggers A warning is raised to suggest `$isSynchronizing`, instead. * re-record snaps * wip * wip * wip
* Fix preview temp directory for CKAsset * reverted last commit * forgot the foundation import from the revision * fix * added tests * Use InMemoryDataManager for previews and fixed tests. * Update inline snapshots. * Delete Tests/SQLiteDataTests/Internal/TemporaryDirectoryDataManager.swift --------- Co-authored-by: Brandon Williams <mbrandonw@hey.com> Co-authored-by: Brandon Williams <135203+mbrandonw@users.noreply.github.com>
"Claude PR Assistant workflow" "Claude Code Review workflow"
… only (#1) - Remove `import Perception` from FetchSubscription.swift (was unused) - Add explicit `import ConcurrencyExtras` for LockIsolated - Remove swift-perception package dependency from Package.swift - Update platforms to macOS 15+ only (drop iOS/tvOS/watchOS) - Update Package@swift-6.0.swift similarly The Perception import was never actually used - the file only uses LockIsolated (from ConcurrencyExtras) and SharedReader (from Sharing). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
… Swift 6.2.3+ - Fix #/ → */ typo in Table+GRDB.swift unterminated block comment - Wrap PrimaryKeyMigrationTests.swift in #if !compiler(>=6.2.3) guard since migratePrimaryKeys is disabled on Swift 6.2.3+ due to compiler crash - Update Package.resolved after rebase onto upstream 1.5.2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request rebases a fork onto upstream pointfreeco/sqlite-data version 1.5.2, incorporating 10 upstream commits while maintaining fork-specific changes. The PR addresses Swift 6.2.3/6.3 compiler compatibility issues by disabling problematic features through conditional compilation guards.
Changes:
- Rebased onto upstream 1.5.2 with 10 new commits including CloudKit bug fixes, date synchronization improvements, and trigger enhancements
- Added compiler guards to disable CloudKit on Swift 6.3+ and primary key migration on Swift 6.2.3+ due to compiler crashes
- Switched to doozMen forks for GRDB, swift-sharing, and swift-perception to work around Swift 6.3 issues
- Reduced platform support to macOS 15+ only (removed iOS, tvOS, watchOS support)
- Changed test schemas from
INTEGER PRIMARY KEY AUTOINCREMENTtoINT PRIMARY KEY - Added GitHub Actions workflows for Claude Code integration
Reviewed changes
Copilot reviewed 65 out of 67 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/SQLiteData/CloudKit/*.swift | Added && !compiler(>=6.3) guards to disable CloudKit on Swift 6.3+ |
| Sources/SQLiteData/CloudKit/PrimaryKeyMigration.swift | Disabled primary key migration on Swift 6.2.3+ with compiler guards |
| Sources/SQLiteData/CloudKit/SyncEngine.swift | Multiple sync engine improvements including timestamp handling, record filtering, and API changes |
| Sources/SQLiteData/StructuredQueries+GRDB/Table+GRDB.swift | Disabled PrimaryKeyedTable extension on Swift 6.2.3+ |
| Sources/SQLiteData/Internal/ISO8601.swift | Added .nextUp workaround for date precision issues |
| Sources/SQLiteData/FetchOne.swift | Changed _OptionalProtocol to StructuredQueriesCore._OptionalProtocol |
| Tests/SQLiteDataTests/Internal/Schema.swift | Changed schemas from INTEGER PRIMARY KEY AUTOINCREMENT to INT PRIMARY KEY |
| Tests/SQLiteDataTests/CloudKitTests/*.swift | Updated test expectations for upstream changes and timestamp handling |
| Package.swift | Reduced platforms to macOS 15+, switched to doozMen forks, updated swift-tools-version to 6.2.1 |
| .github/workflows/*.yml | Added Claude Code and Claude Code Review workflows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,14 +1,11 @@ | |||
| // swift-tools-version: 6.1 | |||
| // swift-tools-version: 6.2.1 | |||
There was a problem hiding this comment.
The swift-tools-version has been increased from 6.1 to 6.2.1. This means the package now requires Swift 6.2.1+ to build, which may not be compatible with all development environments. Document this requirement clearly, especially since the PR also adds guards for Swift 6.2.3 and 6.3 issues.
| // swift-tools-version: 6.2.1 | |
| // swift-tools-version: 6.2.1 | |
| // | |
| // This package requires Swift 6.2.1 or newer to build. | |
| // The manifest includes workarounds for known Swift 6.2.3 and 6.3 issues | |
| // (for example, the Swift-DocC plugin conditional and forks with Swift 6.3 fixes), | |
| // so older toolchains are not supported. |
| // NB: migratePrimaryKeys is disabled on Swift 6.2.3+ due to compiler crash | ||
| // See: Sources/SQLiteData/CloudKit/PrimaryKeyMigration.swift | ||
| #if !compiler(>=6.2.3) |
There was a problem hiding this comment.
The entire PrimaryKeyMigrationTests test suite is disabled on Swift 6.2.3+. This means critical migration functionality is untested on newer Swift versions. Consider:
- Adding a placeholder test that documents this limitation
- Tracking when the compiler bug is fixed so tests can be re-enabled
- Testing the migration feature manually on affected Swift versions before release
| await withErrorReporting(.sqliteDataCloudKitFailure) { | ||
| // NB: Fake 'sending' result. | ||
| nonisolated(unsafe) var result: T.QueryOutput? |
There was a problem hiding this comment.
The nonisolated(unsafe) var result workaround to fake 'sending' is concerning. This bypasses Swift's concurrency checking and could introduce data races. The comment acknowledges this is a workaround. Consider filing an issue with the Swift compiler team and tracking this technical debt, as it undermines the safety guarantees of Swift 6's strict concurrency.
| guard | ||
| let recordPrimaryKey = serverRecord.recordID.recordPrimaryKey, | ||
| serverRecord.encryptedValues[CKRecord.userModificationTimeKey] != nil | ||
| else { | ||
| return | ||
| } |
There was a problem hiding this comment.
The condition serverRecord.encryptedValues[CKRecord.userModificationTimeKey] != nil is added to guard against processing records without modification times. However, this silently skips records that might be valid but missing this field. Consider logging a warning or returning an error to make debugging easier when records are unexpectedly skipped.
| ? try DatabaseQueue(path: path) | ||
| : try DatabasePool(path: path) | ||
| _ = try database.write { db in | ||
| _ = try database.read { db in |
There was a problem hiding this comment.
The change from database.write to database.read when checking database connectivity appears to be intentional (line 2244), but using a read operation to verify write access could miss write permission issues. If this change is necessary for metadatabase setup, add a comment explaining why read-only access is sufficient here.
| containerIdentifier: String? = nil, | ||
| defaultZone: CKRecordZone = CKRecordZone(zoneName: "co.pointfree.SQLiteData.defaultZone"), | ||
| startImmediately: Bool = true, | ||
| startImmediately: Bool? = nil, |
There was a problem hiding this comment.
The startImmediately parameter has been changed from Bool to Bool? with a default value of nil, which is then resolved to !isTesting. This is a breaking API change that could affect existing code that explicitly passes true or false. Consider keeping the signature as Bool with a default of nil internally, or document this as a breaking change.
| stateSerialization: try? metadatabase.read { db in | ||
| try StateSerialization | ||
| .find(#bind(.private)) | ||
| .where { $0.scope == #bind(.private) } |
There was a problem hiding this comment.
The change from .find(#bind(.private)) to .where { $0.scope == #bind(.private) } alters the query semantics. The original used a primary key lookup which is more efficient, while the new version performs a table scan with a WHERE clause. Unless there's a specific reason for this change (e.g., a compiler bug), consider keeping the more efficient .find() approach.
| self.userModificationTime = #sql(""" | ||
| max(\(self.userModificationTime), \(lastKnownServerRecord.userModificationTime)) | ||
| """) |
There was a problem hiding this comment.
Using max() to compare timestamps when updating userModificationTime could lead to unexpected behavior. If a stale server record arrives after a newer local change, this would prevent the timestamp from being rolled back. Consider if this is the intended behavior and document why max() is appropriate here.
| extension Date { | ||
| @usableFromInline | ||
| var iso8601String: String { | ||
| let nextUpDate = Date(timeIntervalSinceReferenceDate: timeIntervalSinceReferenceDate.nextUp) |
There was a problem hiding this comment.
The timeIntervalSinceReferenceDate.nextUp operation on Date is used to handle floating-point precision issues with ISO8601 formatting. This is a clever workaround but could cause subtle bugs if dates near the boundaries of representable values are used. Consider documenting why this is necessary and any edge cases where this could fail.
| @@ -1,4 +1,4 @@ | |||
| import Perception | |||
| import ConcurrencyExtras | |||
There was a problem hiding this comment.
The import has changed from Perception to ConcurrencyExtras. Ensure that all functionality previously provided by the Perception module is still available through ConcurrencyExtras or other imports. This could be a breaking change if any public API exposed Perception types.
5 participants
Summary
#/→*/) in Table+GRDB.swift#if !compiler(>=6.2.3)guard to PrimaryKeyMigrationTests.swiftUpstream commits gained
SyncEngine.isSynchronizingmisuse in triggers pointfreeco/sqlite-data#381)startImmediatelyin tests pointfreeco/sqlite-data#375)Test plan
swift buildpassesswift testpasses (221 tests, 2 known issues)🤖 Generated with Claude Code