Conversation
Summary of ChangesHello @0xLeif, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the AppState library by upgrading its core concurrency mechanisms, ensuring robust thread safety across its components. It also updates platform and language requirements to leverage the latest Swift features and significantly enhances the test suite with comprehensive concurrency stress tests. These changes collectively aim to provide a more stable, performant, and future-proof foundation for the library, alongside improved documentation for better user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes significant improvements to thread safety by replacing manual locking with a Mutex from an updated Cache dependency. The changes are well-implemented, and the removal of NSLock and @MainActor annotations where appropriate is correct. The addition of concurrency stress tests is a great initiative to validate these changes.
My review focuses on two main points. First, the Package.swift file now depends on a Git branch, which is a risk for build stability and should be changed to a version tag. Second, many of the new concurrency stress tests are not actually testing concurrent execution because the tasks are serialized on the main actor. This undermines their goal of verifying thread safety under parallel load. I've provided detailed feedback on these points.
| ], | ||
| dependencies: [ | ||
| .package(url: "https://github.com/0xLeif/Cache", from: "2.0.0"), | ||
| .package(url: "https://github.com/0xLeif/Cache", branch: "leif/mutex"), |
There was a problem hiding this comment.
Using a git branch for a dependency is generally discouraged for shared or production code as it can lead to build instability. Branches can be force-pushed, rebased, or deleted, which would break builds for anyone using this package. For better stability and reproducible builds, it's recommended to depend on a specific version tag or a commit hash. Please consider updating this to a stable release version once the leif/mutex branch is merged and a new version of Cache is tagged.
| group.addTask { @MainActor in | ||
| var state = Application.state(\.stressString) | ||
| state.value = "value-\(i)" | ||
| } |
There was a problem hiding this comment.
The tasks within this test group are marked with @MainActor. This means all write operations will be serialized on the main thread, and the test will not actually perform concurrent writes. This defeats the purpose of a concurrency stress test, which is to verify thread safety under parallel execution. This applies to several other tests in this file as well (e.g., testHighContentionStateUpdates, testConcurrentArrayModifications).
While Application.State.value is @MainActor, which forces these writes to the main thread, the test name ConcurrencyStressTests is misleading. The testKeychainConcurrentAccess function is a good example of a true concurrency test because its tasks are not actor-isolated and execute in parallel.
Please either rename these tests to reflect that they test high-volume serial operations on the main actor, or refactor them to test true concurrency if there's a way to do so while respecting the library's API design.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the AppState library for Swift 6 by replacing manual locking mechanisms with Mutex-based concurrency primitives, significantly raising minimum platform requirements, and adding comprehensive stress tests to validate thread safety. The changes enable Swift 6 strict concurrency checking and prepare the codebase for modern Swift development.
- Migrated from
NSLock/NSRecursiveLocktoMutexfrom the Synchronization module for thread-safe state access - Updated minimum platform versions to iOS 18, macOS 15, watchOS 11, tvOS 18, and visionOS 2
- Added comprehensive concurrency stress tests covering state management, dependencies, and Keychain operations
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Package.swift | Updated platform requirements to latest versions, switched Cache dependency to leif/mutex branch, and enabled Swift 6 language mode with StrictConcurrency |
| Sources/AppState/Dependencies/Keychain.swift | Replaced NSLock with Mutex<Set>, removed @mainactor annotations, and simplified thread-safe operations using withLock |
| Sources/AppState/Application/Application.swift | Removed NSRecursiveLock property that was previously used for manual locking |
| Sources/AppState/Application/Application+internal.swift | Simplified value() method by removing manual lock/unlock calls, now relying on Cache's internal Mutex |
| Tests/AppStateTests/ConcurrencyStressTests.swift | Added new comprehensive stress test suite with 8 tests covering concurrent state operations, dependency access, and Keychain operations |
| README.md | Added Examples section documenting 31 example projects organized by complexity level with featured app highlights |
Comments suppressed due to low confidence (1)
Package.swift:21
- Using a branch dependency instead of a versioned release can lead to build instability. The "leif/mutex" branch could change or be deleted without warning, breaking builds for downstream users. Consider requesting a tagged release of the Cache library with the Mutex functionality and updating this to use a version requirement like
from: "x.x.x"instead.
.package(url: "https://github.com/0xLeif/Cache", branch: "leif/mutex"),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
README.md
Outdated
| ## Examples | ||
|
|
||
| Explore our comprehensive [Examples](Examples/) folder with 31 example projects: | ||
|
|
||
| | Category | Examples | Description | | ||
| |----------|----------|-------------| | ||
| | **Focused** | 2 | Production-quality apps (SyncNotes, MultiPlatformTracker) | | ||
| | **Moderate** | 4 | Feature-focused apps (TodoCloud, SettingsKit, DataDashboard, SecureVault) | | ||
| | **Lightweight** | 25 | Single-concept examples covering all AppState features | | ||
|
|
||
| ### Featured Examples | ||
|
|
||
| - **[SyncNotes](Examples/Focused/SyncNotes/)** - Note-taking with iCloud sync across devices | ||
| - **[MultiPlatformTracker](Examples/Focused/MultiPlatformTracker/)** - Habit tracker for iOS/macOS/watchOS | ||
| - **[TodoCloud](Examples/Moderate/TodoCloud/)** - Progressive persistence (memory → local → cloud) | ||
| - **[SecureVault](Examples/Moderate/SecureVault/)** - Password manager with Keychain storage | ||
|
|
||
| See [Examples/README.md](Examples/README.md) for the full list with descriptions. |
There was a problem hiding this comment.
The README references an "Examples/" directory and specific example projects that don't exist in the repository. All the links to Examples/Focused/, Examples/Moderate/, and Examples/README.md will be broken. Either the Examples directory needs to be added in this PR, or these references should be removed or marked as "coming soon".
| group.addTask { | ||
| keychain.set(value: "value-\(i)", forKey: "stress-key-\(i % 10)") | ||
| } | ||
| } | ||
|
|
||
| // Concurrent reads | ||
| for i in 0..<iterations { | ||
| group.addTask { | ||
| _ = keychain.get("stress-key-\(i % 10)") | ||
| } |
There was a problem hiding this comment.
The tasks within this withTaskGroup are marked @mainactor, but the test creates non-isolated concurrent tasks that shouldn't necessarily run on the MainActor. Since the Keychain methods are now properly thread-safe with Mutex and don't require @mainactor, the @mainactor annotation on the task closures should be removed to allow proper concurrent execution. This applies to both the write tasks (lines 211-213) and read tasks (lines 218-220).
| group.addTask { | |
| keychain.set(value: "value-\(i)", forKey: "stress-key-\(i % 10)") | |
| } | |
| } | |
| // Concurrent reads | |
| for i in 0..<iterations { | |
| group.addTask { | |
| _ = keychain.get("stress-key-\(i % 10)") | |
| } | |
| group.addTask(@Sendable { | |
| keychain.set(value: "value-\(i)", forKey: "stress-key-\(i % 10)") | |
| }) | |
| } | |
| // Concurrent reads | |
| for i in 0..<iterations { | |
| group.addTask(@Sendable { | |
| _ = keychain.get("stress-key-\(i % 10)") | |
| }) |
| await withTaskGroup(of: Void.self) { group in | ||
| // Writers | ||
| for i in 0..<iterations { | ||
| group.addTask { | ||
| keychain.set(value: "v\(i)", forKey: "concurrent-\(i)") | ||
| } | ||
| } | ||
|
|
||
| // Readers calling values() | ||
| for _ in 0..<iterations { | ||
| group.addTask { | ||
| _ = keychain.values() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the test above, the tasks within this withTaskGroup don't need to be confined to the MainActor since Keychain operations are now thread-safe with Mutex. The @mainactor annotations on the task closures (lines 245 and 252) should be removed to enable true concurrent execution and better stress testing.
| ], | ||
| dependencies: [ | ||
| .package(url: "https://github.com/0xLeif/Cache", from: "2.0.0"), | ||
| .package(url: "https://github.com/0xLeif/Cache", branch: "leif/mutex"), |
There was a problem hiding this comment.
The Cache dependency is pinned to a mutable Git branch (branch: "leif/mutex"), which creates a supply chain risk because future builds can silently pull different, attacker-controlled code without any change to this repository. An attacker who compromises the upstream repo or branch could inject malicious logic into your library at build time. To mitigate this, pin the dependency to an immutable reference such as a released semantic version or a specific commit SHA and update it explicitly when needed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
Package.swift:21
- The Cache dependency is pinned to a specific branch ("leif/mutex") instead of a semantic version. Branch dependencies are unstable and can change without notice, potentially breaking builds or introducing unexpected behavior. This is generally considered bad practice for production code as it makes builds non-reproducible. Once the mutex functionality is merged and released in Cache, this should be updated to use a semantic version (e.g., from: "X.Y.Z").
.package(url: "https://github.com/0xLeif/Cache", branch: "leif/mutex"),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Task { @MainActor in | ||
| keys.insert(key) | ||
| } | ||
| _ = keys.withLock { $0.insert(key) } |
There was a problem hiding this comment.
The key is added to the internal keys set regardless of whether the keychain operation succeeds. If SecItemAdd fails for reasons other than the item already existing (e.g., keychain access denied, disk full), the key will still be tracked in the set even though the value isn't actually stored in the keychain. This could lead to inconsistencies between the tracked keys and actual keychain contents. Consider only adding the key to the set if the keychain operation succeeds (status == errSecSuccess).
| SecItemDelete(query as CFDictionary) | ||
| lock.unlock() | ||
| _ = keys.withLock { $0.remove(key) } |
There was a problem hiding this comment.
The key is removed from the internal keys set regardless of whether SecItemDelete succeeds. If the deletion fails (e.g., item doesn't exist in keychain, access denied), the key will still be removed from the tracked set, causing the internal state to be inconsistent with the actual keychain contents. Consider only removing the key from the set if SecItemDelete succeeds (status == errSecSuccess) or if the item doesn't exist (status == errSecItemNotFound).
|
|
||
| XCTAssertEqual(completedWrites, iterations) | ||
| let finalState = Application.state(\.stressCounter) | ||
| XCTAssertGreaterThan(finalState.value, 0) |
There was a problem hiding this comment.
The test performs 100 read-modify-write operations sequentially on @mainactor. Since these operations are serialized by @mainactor, the final value should deterministically equal 100 (iterations). The current assertion only checks if the value is greater than 0, which is a weak validation that would pass even if only 1 operation succeeded. Consider changing this to XCTAssertEqual(finalState.value, iterations) to properly verify that all updates were applied correctly.
| } | ||
|
|
||
| let finalState = Application.state(\.stressArray) | ||
| XCTAssertGreaterThan(finalState.value.count, 0) |
There was a problem hiding this comment.
The test spawns 200 tasks that each read the array, append an element, and write it back. Since all tasks run sequentially on @mainactor, the final array should contain exactly 200 elements (one from each task). However, the assertion only checks that the count is greater than 0, which would pass even if only a single append succeeded. Consider changing line 118 to XCTAssertEqual(finalState.value.count, iterations) to properly verify that all modifications were applied.
1 participant
This pull request introduces significant improvements to concurrency safety and platform support for the AppState library. The most important changes are the adoption of a new
Mutex-based locking mechanism for thread safety, updates to platform and Swift version requirements, and the addition of comprehensive stress tests to verify concurrency robustness. Documentation has also been updated to highlight new examples.Concurrency and Thread Safety Improvements:
Replaced all uses of
NSLock/NSRecursiveLockin theKeychainandApplicationclasses with a newMutex-based locking mechanism provided by the updatedCachedependency, improving thread safety and removing manual locking code. (Sources/AppState/Dependencies/Keychain.swift,Sources/AppState/Application/Application.swift,Sources/AppState/Application/Application+internal.swift) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Updated the
Cachedependency to use theleif/mutexbranch, which provides the newMuteximplementation. (Package.swift)Platform and Language Support:
StrictConcurrencyexperimental feature for both library and test targets. (Package.swift) [1] [2]Testing Enhancements:
ConcurrencyStressTeststest suite to verify thread safety and concurrency robustness of state and dependency management, including high-contention scenarios and rapid state resets. These tests also cover concurrent access to theKeychain. (Tests/AppStateTests/ConcurrencyStressTests.swift)Documentation and Examples:
README.mdwith a new "Examples" section, listing 31 categorized example projects and highlighting featured apps, to help users explore AppState's capabilities. (README.md)These changes collectively modernize the codebase, ensure robust concurrency handling, and provide better documentation and testing for users and contributors.